Repository navigation
ci: guard every inherited workflow to upstream, and enforce it in CI - #22
Conversation
Audit tool for #9. Parses every workflow and asserts each job either carries the canonical `github.repository == 'anomalyco/opencode'` guard, is permanently disabled, or belongs to a workflow this fork deliberately runs. Fails closed: a job arriving in a future upstream merge is unaccounted for until someone decides which category it is in. Currently reports 25 unguarded jobs across 19 workflows. Refs #9 Claude-Session: https://claude.ai/code/session_017iG7xA9wRvNyFYZSXm8inU
|
Session stamp (claim):
Picked up as the P1 of a full triage pass over the open issue list. No open PR overlapped: the three open PRs are dependabot bumps touching |
… writes Three exposures, all verified against real runs on this fork rather than predicted: 1. script/github/close-issues.ts hardcodes `anomalyco/opencode`. This fork's 02:00 cron enumerated UPSTREAM's open issues and attempted to comment and close them, failing only for lack of token scope: `Failed to comment anomalyco#38226: 403 Forbidden` (run 35553911700). That issue number exists only upstream. close-prs.yml is the same shape (403 on run 35540524727). 2. pr-standards and compliance-close auto-closed our own PR #6 for not matching upstream's PR template. 3. publish.yml guarded four of five jobs. The `publish` job that logs into ghcr as github.repository_owner carried `if: always() && !failure() && !cancelled()`, which is true when its guarded needs skip. Run 35539779127 confirms it: four jobs skipped, publish did not. Changes: - The canonical `github.repository == 'anomalyco/opencode'` guard on all 25 previously unguarded jobs across 19 workflows, composed with any existing condition rather than replacing it. - publish.yml's `publish` job guarded, closing the always() hole. - docs-update.yml moved off the stale 'sst/opencode' literal, which was protecting this fork only by coincidence. - A same-repository guard inside both close scripts, at the point of the write. The invariant is "a script may only write to the repository it runs in" rather than "is this upstream", so it holds for any future fork and does not depend on an organisation name. It fails closed when GITHUB_REPOSITORY is unset and exits 0, not 1, since a fork skipping inherited maintenance is the correct outcome and not a nightly failure. - script/check-workflow-guards.ts, run from test.yml, asserting coverage and failing closed so a job inherited in a future upstream merge is a violation until it is classified. Without this the four-of-five hole in publish.yml can simply come back. Upstream behaviour is unchanged: under GITHUB_REPOSITORY=anomalyco/opencode close-issues.ts passes the guard and proceeds to a real API call. Allowlist is test.yml and typecheck.yml, the two workflows this fork adapted for its own use. Closes #9 Claude-Session: https://claude.ai/code/session_017iG7xA9wRvNyFYZSXm8inU
Self-review finding on my own check. The allowlist keyed on the workflow filename, so any job a future upstream merge added to test.yml or typecheck.yml would have been allowed silently - the same shape as the bug this check exists to catch, where publish.yml was guarded on four jobs out of five. Now keyed `<workflow>::<job>`, and an allowlist entry matching no job is itself a violation: a stale entry (renamed or removed job) stops protecting whatever replaced it, which would be invisible otherwise. Verified by mutation: a new unguarded `sneaky` job in test.yml is now caught (it was not before), and renaming an allowlist entry is caught. Refs #9 Claude-Session: https://claude.ai/code/session_017iG7xA9wRvNyFYZSXm8inU
codex review finding, and it is the same defect class the check exists to
catch - a guard that reports more coverage than it has.
`condition.includes(GUARD)` accepted conditions that contain the guard
verbatim and still run on this fork. Two real shapes:
github.repository == 'anomalyco/opencode' || github.repository == 'swxtchio/swx-opencode'
github.repository == 'anomalyco/opencode' && github.event.action == 'opened' || true
The second is the nastier one: `&&` binds tighter than `||` in GitHub
expressions, so it parses as `(guard && action) || true`.
`isGuarded` now requires the guard to be the whole condition or the leading
term of a top-level conjunction, with no `||` outside parentheses in the
remainder. A disjunction nested under the guard - `guard && (a || b)`, which
deploy.yml uses - stays valid.
Also fixes two things that came out of writing the test:
- main() is behind `import.meta.main`. Without it the module could not be
imported for testing, because the audit ran on import and its
process.exit(1) would have killed the test run rather than failed a test.
- The CI step names its test files explicitly and runs from `script`, so a
bare scan cannot pull in translate-app.test.ts, which is not part of this
gate.
Verified by mutation: restoring the substring check fails 5 of the 14 new
tests, and a `guard || fork` condition in unlock.yml is now reported.
Refs #9
Claude-Session: https://claude.ai/code/session_017iG7xA9wRvNyFYZSXm8inU
Review round 1 — codexMAJOR, accepted and fixed ( Two shapes contain the guard verbatim and still run here: The second is the worse one and codex did not name it:
if: github.repository == 'anomalyco/opencode' && (github.ref_name == 'dev' || github.ref_name == 'production')NIT, accepted and fixed: the failure message still said "add the workflow to ALLOWED" after the allowlist went per-job. It now prints both accepted shapes and the Two further defects that surfaced while writing the test for the aboveNeither was reported; both were consequences of making the module importable.
Mutation-verified
Confirmations from codex, recorded
glm-5.3, kimi-k3 and sol are still running; I will post their rounds separately. |
Three findings from the sol review, all accepted. MAJOR - the allowlist trusted job CONTENTS forever. Keyed only by name, a future upstream merge could add a publishing step, a `uses:` reusable workflow or `secrets: inherit` to test.yml::unit and the check would still pass, because an allowlisted job is by definition unguarded. The allowlist is now a Map from `<workflow>::<job>` to a digest of the job definition, so any change to an allowed job fails until someone reads it and updates the digest deliberately. Keys are sorted before hashing so a cosmetic reordering does not fail the check and train people to bump the digest without looking. MAJOR - the tests covered only the pure comparison helper, so deleting `requireSameRepository(...)` from either close script, or making it stop exiting, left every test green. That is the one thing actually protecting upstream. close-scripts-guard.test.ts now runs both scripts as subprocesses with a dummy token and a mismatched repository, and asserts the refusal, the exit code, and the absence of any authentication error - the last of which is what proves no request was attempted. The matching-repository case is deliberately not exercised, because that path does reach the GitHub API. MINOR - the refusal path was not reachable without credentials: close-issues.ts required GITHUB_TOKEN first, and close-prs.ts could shell out to `gh auth token` via requireToken(), so with GITHUB_REPOSITORY unset and no credentials both exited 1 instead of refusing with 0. The guard now runs before either. No write was possible in that state, so this was ergonomics rather than safety. Mutation-verified, each against the exact mutation described: delete requireSameRepository from close-issues.ts 2 of 14 tests fail stop requireSameRepository exiting 4 of 4 tests fail add `npm publish` to the allowlisted test.yml unit job caught Refs #9 Claude-Session: https://claude.ai/code/session_017iG7xA9wRvNyFYZSXm8inU
Review round 1 — sol (medium)Four findings. One duplicated codex's substring MAJOR (already fixed in MAJOR — the allowlist trusted job contents foreverKeyed only by name, an allowlisted job could be changed by a future upstream merge — a publishing step, a
MAJOR — the tests proved the helper, not the wiringCorrect, and this is the sharper of the two.
MINOR — the refusal path was unreachable without credentials
Mutation-verified against sol's own described mutations
Running total: 10 mutations tested, 10 behaving as intended. glm-5.3 and kimi-k3 still running; both started before these four commits, so they will get a round 2 against the current head. |
glm-5.3 review. One MAJOR, and it answers the challenge I set the panel -
find a mutation the check does not catch.
MAJOR - hasTopLevelOr counted parentheses without knowing about string
literals, so a paren inside a literal raised the depth for the rest of the
expression and hid a genuine top-level `||`:
github.repository == 'anomalyco/opencode' && github.event.issue.title == '(' || true
GitHub parses that as `(guard && title == '(') || true` - true in every
repository - and the previous scanner reported it as guarded. A closing paren
in a literal is the same bug with the depth driven negative. The scanner now
skips characters between single quotes; GitHub literals have no escape
sequences and write a literal quote as '', so a plain toggle is exact.
This also fixes the mirror-image false positive: `guard && contains(x,
'a||b')` was being rejected as unguarded, and a check that rejects valid
conditions gets worked around rather than fixed.
MINOR - 8f6138a committed script/.local/state/gh/device-id. The cause was
my own test: it replaces the environment, so with HOME unset the gh CLI that
close-prs.ts may invoke for a token wrote its state relative to the cwd.
Fixed at the cause - each subprocess gets a throwaway HOME, removed after the
run - rather than by deleting the file and hoping. script/.local/ is also
gitignored as a backstop.
MINOR - recorded the delivery-path dependency in the script header: the audit
only runs because test.yml::unit has no `paths:` filter, and adding one would
stop auditing workflow-only changes silently rather than with a red check.
NIT - a test.each row passed ["", "empty"] where only the first element
reached the callback, reading like an unused second argument.
Verified by mutation: reverting the scanner to paren-only fails 5 of 23
tests, and the workflow form of the bypass is now reported by the audit.
Refs #9
Claude-Session: https://claude.ai/code/session_017iG7xA9wRvNyFYZSXm8inU
Review round 1 — glm-5.3The strongest round of the four. It answered the challenge I set the panel — find a mutation the check does not catch — and found one that survived all three hardening commits. MAJOR, accepted and fixed (
|
Self-review of the questions I put to the review panel, probing the scanner
glm's finding produced.
An unterminated quote or unbalanced parenthesis made the scanner report
"guarded". With a quote left open it treats the rest of the expression as
string data, so a real top-level `||` is invisible:
github.repository == 'anomalyco/opencode' && x == 'abc || true
GitHub cannot evaluate that, so it is not an exposure - but "I could not check
this" must not read as "this is fine", which is the same overstated-coverage
mistake this check exists to prevent. There is now a third verdict,
`malformed`, with its own message, because "unbalanced quotes" and "the guard
is not the leading term" call for different fixes.
The same probe found the opposite error: `GUARD&& x`, with no space before the
conjunction, was rejected although GitHub does not care about the spacing. A
check that rejects valid conditions gets worked around rather than fixed, so
the leading-term match now tolerates any whitespace.
Mutation-verified: removing the balance check fails 3 of 30 tests, restoring
the fixed-space match fails 1, and a malformed condition in triage.yml is
reported with the right reason rather than passing as guarded.
Refs #9
Claude-Session: https://claude.ai/code/session_017iG7xA9wRvNyFYZSXm8inU
Round 1 — kimi-k3 (incomplete), and my own passkimi-k3 timed out
It did open one thread worth finishing, which I completed rather than waiting: Other scripts that hardcode the upstream repository — checked, no further exposure
That is the complete set: those are the only non-GET calls anywhere under Two more findings from my own passI probed the scanner with the same questions I gave the panel, and it got two answers wrong in opposite directions. Both fixed in An uncheckable condition was reported as guarded. With a quote left open, the scanner treats the remainder as string data and a real top-level if: github.repository == 'anomalyco/opencode' && x == 'abc || trueSame for unbalanced parentheses. GitHub cannot evaluate either, so neither is an exposure — but "I could not check this" must not read as "this is fine", which is precisely the overstated-coverage mistake this check exists to prevent. There is now a third verdict, And the opposite error: Mutation ledger — 15 tested, all behaving as intended
Round 2 is now running against |
Round 2 findings from sol and codex. sol, MAJOR - the digest covered only each job's body, leaving `on`, `permissions`, `env` and `defaults` unpinned. So the audit could be switched off without any digest changing: add a `paths:` filter to test.yml and workflow-only changes - exactly what this audits - stop being audited, silently. I had written that risk into a comment in the previous commit instead of enforcing it, which is the same mistake in miniature. Two checks now, deliberately independent: - ALLOWED_ENVELOPES pins the non-jobs part of each workflow holding an allowlisted job, so a change to its triggers or permissions must be read and acknowledged. - auditWorkflowViolations asserts the property that actually matters: the audit's own workflow runs on push and pull_request with no `paths` or `paths-ignore` filter. A digest only says "someone changed this"; it does not say which change is harmful, and this one is nameable. codex, MINOR - `(GUARD) && x` was rejected although GitHub accepts it, and a check that rejects valid conditions gets worked around rather than fixed. Fixing that needed care, which is why the leading term is now parsed rather than pattern-matched. The obvious regex - optional brackets either side of the guard - accepts `(GUARD && x) || y`, where the guard is subordinate to a disjunction and the job runs in every repository. Comparing the bracket counts either side of the guard distinguishes the two exactly, and brackets inside string literals are ignored. Mutation-verified: a `paths:` filter on the audit's own workflow is reported by both new checks; removing `pull_request` is reported; and dropping the bracket-count comparison fails 4 of 39 tests on the bypass cases. Refs #9 Claude-Session: https://claude.ai/code/session_017iG7xA9wRvNyFYZSXm8inU
Review round 2 — codex and sol, against
|
| # | mutation | caught |
|---|---|---|
| 16 | paths: filter added to the audit's own workflow |
yes, by both new checks |
| 17 | pull_request trigger removed from the audit's workflow |
yes |
| 18 | drop the bracket-count comparison (the regex-shaped bypass) | 3 of 39 tests |
glm-5.3 and kimi-k3 round 2 still running.
glm-5.3 round 2. It reproduced sol's digest finding independently - flipping test.yml's `permissions: contents: read` to `write` with every job body untouched left the check green - confirmed 85917b4 closes it, and attacked the new bracket-count parser with 19 shapes, all correct. Six items remained. MINOR - auditWorkflowViolations checked which EVENTS fire the audit but not which branches. Moving test.yml's `push.branches` off the default branch produced no structural violation; only the envelope digest noticed, with a generic "confirm it still runs" message, and an acknowledgement prompt is exactly where a change gets rubber-stamped. The stated property was "every push to the default branch", so that is now asserted directly, along with `pull_request` branch filters and `branches-ignore`. NIT - a `${{ }}`-wrapped guard was reported unguarded. GitHub treats wrapped and bare conditions identically, so this was a correct condition given a reason that misdescribes it. Unwrapping happens before the leading-term parse, and `${{ GUARD || true }}` is still rejected. NIT - ALLOWED_ENVELOPES had no staleness detection of its own, and nothing enforced its pairing with the job allowlist. Both directions are now checked: an allowlisted job whose workflow envelope is unpinned, and a pinned envelope for a workflow that is gone or has no allowlisted job. NIT - the flat glob could not see a workflow file in a subdirectory. GitHub does not run nested workflow files, so scanning them would report inert files as violations; instead their presence is itself reported, because invisible is worse than either. NIT - key ordering in the digest used localeCompare, which is locale-dependent in principle. A different collation would reorder keys and break every pinned digest with a message explaining nothing. Codepoint order now. NIT - the remediation text still said to add an allowlist entry "with a reason" after the value became a digest, and isGuarded's comment claimed call sites that do not exist. Also fixed a comment that sol and glm both flagged: I had called the absence-of-credentials-error assertion "decisive". It is corroborating. The three assertions together are the proof, and the commit message for 8f6138a overstated it. Mutation-verified, six new cases: push.branches off the default branch (caught twice), pull_request restricted to another branch, branches-ignore added, a nested workflow file, an envelope pinned for a workflow with no allowlisted job, and an allowlisted job with no pinned envelope. Refs #9 Claude-Session: https://claude.ai/code/session_017iG7xA9wRvNyFYZSXm8inU
The previous commit's message said this was fixed. It was not - my edit's anchor did not match and I did not check before writing the message. The change itself is what that message described. Both sol and glm-5.3 made the point: the absence of an authentication error is corroborating, not decisive. With no network egress a guard-deleted script fails with neither string, and GitHub could reword a message. What proves the wiring is the three assertions together - delete the guard and the refusal text never prints; make the guard log-only and the exit code stops being 0, because every path after it throws on the dummy token. Refs #9 Claude-Session: https://claude.ai/code/session_017iG7xA9wRvNyFYZSXm8inU
…s wiring kimi-k3 round 2, five items. Two corrections to its report are recorded below, because one of its claims did not reproduce as stated. F4, MINOR, and the important one - a fail-open bug in the checker. `if: false` parses as a YAML boolean, but `if: 'false'` parses as the STRING "false", and GitHub treats a non-empty string as truthy, so that job runs. The check compared the stringified value and counted both as disabled. Only the boolean does now. Verified: a job written `if: 'false'` is reported as needing a guard. F1, MAJOR as reported - the audit's own wiring could evaporate silently. The script runs from one step in test.yml::unit, and that job's digest is computed only by that same step, so a merge conflict resolution dropping the step takes the whole control with it and nothing goes red. The two steps now attest to each other: this script asserts both exist, and the tests the second step runs assert the same. Dropping either is caught by the other. The residual is stated honestly in the code - removing both in one edit leaves nothing running to notice, which is a bootstrap limit of any in-CI check. F2, MINOR, with a correction. kimi reported that `bun test <missing-file>` exits 0; on Bun 1.4.2 here it exits 1. But the case that matters does reproduce: a filter matching nothing is silently ignored when another filter matches, so `bun test guard-expression.test.ts github/renamed-away.test.ts` ran 44 tests and passed. A renamed test file would therefore drop out of the gate rather than fail it. The step now runs `ls` over its named files first. Q5 - enforcement was opt-in per call site, so nothing would notice a future script that adds a hardcoded-target write and forgets the guard. That is now a property of the directory: write-guard-coverage.test.ts fails if any script issues a GitHub write without importing the guard. My first version of that detector was itself wrong, and its failure is worth recording: it asked whether a file contained a write verb anywhere and mentioned api.github.com anywhere, and flagged script/stats.ts - whose only write is a POST to PostHog, with an unrelated GitHub read sixty lines below. A detector that cries wolf gets deleted, so the verb is now matched against its nearest preceding URL, and there are tests in both directions including that exact shape. F5, F7 - narrowed the digest's documented claim, which does not cover what an allowlisted job's steps then run (`./.github/actions/setup-bun` moves with upstream merges), and fixed an error message that blamed the working directory when the path comes from import.meta.url. F6 - stats.yml attaches POSTHOG_KEY to the "Commit stats" step rather than to the step that runs stats.ts, so that write never fires even upstream. Left alone: it is upstream's bug, in a workflow that is now guarded here, and we do not send changes upstream. Mutation-verified, five new cases: a quoted 'false' condition, dropping either audit step, removing the `ls` existence check, and a new unguarded script that writes to GitHub. test.yml::unit's digest updated to acknowledge the step change, which is the pin working as intended. Refs #9 Claude-Session: https://claude.ai/code/session_017iG7xA9wRvNyFYZSXm8inU
Review round 2 — glm-5.3 and kimi-k3Both returned substantial rounds. Nine more findings accepted and fixed across glm-5.3 — independently reproduced sol's MAJOR, then broke my fix's neighbourIt demonstrated the digest bypass empirically rather than by argument: flipping Its remaining items, all fixed:
kimi-k3 — the fail-open bug, and the audit's own bootstrap
kimi also finished its own Q1 sweep properly, across A correction to my own workMy first version of the write-detector was wrong, and the failure is worth recording because it is the mirror of everything else in this PR. It asked whether a file contained a write verb anywhere and mentioned Also: I claimed in Mutation ledger — 29 testedNew since the last comment:
One mutation of mine (M19, first attempt) produced no output because my |
…ted jobs run codex round 3, three MAJOR findings. One of its claims did not reproduce as stated; the correction is below. The hardcoded default branch could rot silently. If this fork's default branch were renamed, a stale `branches: [swxtch]` would keep satisfying the assertion while covering nothing - the assertion and the thing it checks would be stale in the same direction, which is the failure mode that makes a check worthless. `github.event.repository.default_branch` is not an environment variable, but the whole event payload is on disk at GITHUB_EVENT_PATH, so the authoritative value is read from there and the constant is checked against it. A malformed payload leaves the constant in force rather than failing the audit for an unrelated reason. The digests left a third drift axis open: what an allowlisted job RUNS. The job digest pins a job's own YAML and the envelope digest pins everything but the jobs, so both pass while `./.github/actions/setup-bun` - used by every allowlisted job - changes underneath them on an upstream merge. Local actions called by allowlisted jobs are now pinned too, and the set is DISCOVERED from those jobs rather than listed by hand, because a hand-written list would leave a newly added action unpinned - the same gap one level down. Both pairing directions are checked, as for the envelopes. On the GraphQL claim: codex reported that githubWrites() misses a GitHub write via GraphQL. Half right. The full-URL form `https://api.github.com/graphql` was already detected, since the host matches; only the bare-path `githubRequest("/graphql", ...)` form was missed - and it was missed twice over, because `/graphql` was absent from both the recognised paths and the URL extraction pattern, so such a call had no "nearest URL" to test at all. Both fixed, with tests for each form. codex also raised a false-positive mode for a write whose URL is a variable with a GitHub literal shortly before it. Left as is: the detector already prefers the nearest preceding URL, and an unresolvable variable target is not something a regex can settle. Its MINOR - that scalars like `0` or `null` are now treated as needing a guard - is the safe direction and intended. Mutation-verified: a payload reporting a different default branch is reported with both messages; editing setup-bun's action.yml is caught; an allowlisted job gaining an unpinned local action is caught; and a pinned action no longer used is caught. Refs #9 Claude-Session: https://claude.ai/code/session_017iG7xA9wRvNyFYZSXm8inU
sol round 3. One BLOCKER upheld, one refuted, three MAJORs fixed, and a
correction to a fix I made for a previous reviewer.
BLOCKER, upheld - the mutual attestation checked step NAMES. Adding `if:
false` to the audit step disables it while both names remain for a name-only
check to find, and the allowlisted job's digest cannot catch it because the
disabled step is what computes that digest. Both halves now require each step
to carry no condition and to still invoke its command.
BLOCKER, refuted - sol reported the asserted default branch is already wrong,
citing AGENTS.md and refs/remotes/origin/HEAD, both of which say `dev`.
GitHub itself reports `swxtch` (`gh repo view --json defaultBranchRef`), only
`swxtch` pushes run CI, and `dev` last saw a run during the 2026-09-20
incident. Both of sol's sources are upstream artefacts inherited by the fork:
origin/HEAD was set by a clone predating it, and AGENTS.md is upstream's text.
That stale documentation is a trap rather than a curiosity - it just cost a
reviewer a false blocker - so AGENTS.md now states the fork's default branch,
says plainly that origin/HEAD is not to be trusted, and gives the command that
resolves it. glm-5.3 had flagged the same text as upstream-inherited in an
earlier round, which is what made the contradiction quick to settle.
MAJOR - branch coverage used textual membership. GitHub evaluates branch
filters as ORDERED globs where the last matching pattern wins, so
`[swxtch, "!swxtch"]` lists the branch while excluding it. Patterns are now
walked in order, with GitHub's glob subset where `*` stops at a slash and `**`
does not.
MAJOR - three gaps in the write detector, all fixed: `gh api --method PATCH`
had no `method:` property and bypassed it entirely; `uploads.github.com`, which
takes release-asset uploads, was not recognised as GitHub; and a file merely
MENTIONING same-repo-guard was exempted, so a comment naming it would have
excused an unguarded writer. An import is now required. The remaining residual
- a variable URL, and gh subcommands that write without naming a method - is
stated in the code: this is a backstop for a future oversight, not a
containment boundary.
MINOR - and this corrects my own previous commit. sol is right that GitHub
evaluates an `if` value as an expression even without `${{ }}`, so `if:
'false'` is the expression `false` and IS falsy; kimi's claim that the quoted
form runs was wrong, and my comment repeated it. The strictness is kept
deliberately - disabling a job by relying on how a quoted scalar is re-parsed
should be written as a guard - but for the honest reason rather than the wrong
one.
Mutation-verified: `if: false` on the audit step, the step's command changed,
`[swxtch, "!swxtch"]`, and a `gh api` write in an unguarded script.
Refs #9
Claude-Session: https://claude.ai/code/session_017iG7xA9wRvNyFYZSXm8inU
Review round 3 — codex and solsol returned a BLOCKER, which is upheld, and a second which is refuted with evidence. Six more findings fixed. sol BLOCKER (upheld) — the attestation checked step namesAdding Both halves now require each step to carry no condition and to still invoke its command. Mutation-verified both ways: sol BLOCKER (refuted) — "the asserted default branch is already wrong"sol reported
Empirically: pushes to glm-5.3 had already flagged that same AGENTS.md line as upstream-inherited in an earlier round, which is what made this quick to settle. But that stale documentation is a trap, not a curiosity — it just cost a reviewer a false blocker. sol MAJOR — branch coverage used textual membershipGitHub evaluates branch filters as ordered globs where the last matching pattern wins, so this lists the branch while excluding it: branches:
- swxtch
- "!swxtch"Now walked in order, with GitHub's glob subset ( sol MAJOR — three gaps in the write detectorAll three real, all fixed:
Residual, stated in the code rather than glossed: a request whose URL is a variable is invisible to a regex, and sol MINOR — which corrects a fix I made for kimiThe two reviewers contradict each other, and sol is right. GitHub evaluates an The strictness is kept, for the honest reason: disabling a job by relying on how a quoted scalar is re-parsed as an expression is fragile enough that it should be written as a guard, and the cost of demanding one is a false positive with a remediation message, where the opposite error lets a job run unaudited. codex — three MAJORs
codex's MINOR (scalars like Mutation ledger — 37 tested
glm-5.3 and kimi-k3 round 3 still running. |
…elves glm-5.3 round 3, reviewing the commit before sol's fixes landed. One BLOCKER in two parts - part (a) was already closed by the sol round, part (b) was not - plus three MAJORs. BLOCKER (b) - the audit could run while controlling nothing. Appending `|| true` to its step, or setting continue-on-error, leaves the step present and its command intact, so the checks added for sol's blocker all pass. The audit then prints its violations to the log while CI stays green, which is worse than the audit being absent: a green check is read as evidence. glm calls this the reflex edit made to get a branch green, and that is exactly right. Both steps and the job are now checked for a discarded exit code and for continue-on-error. MAJOR - a scalar `branches:` filter escaped the structural check entirely. GitHub's schema accepts a string there, so `branches: dev` was a real filter that `Array.isArray` skipped, routing branch-filter changes back onto the envelope digest - the rubber-stamp this structural check exists to bypass. Filters are normalised before the coverage test now. MAJOR, and the sharpest finding of the round - the checks added in the last three commits had no tests. The disabled classification lived inline in main(), so reverting it to the round-2 fail-open form kept all 67 tests green. glm applied this repository's own principle back at it: a check that never fails is indistinguishable from one that is broken. isDisabled, conditionOf, asList, swallowsFailure, auditStepViolations and auditWorkflowViolations are now pure exported functions with audit-checks.test.ts pinning them, including the exact revert glm described and every single-edit bypass the panel found. MAJOR - three more shapes the write detector missed: the gh CLI invoked as a spawn ARGUMENT ARRAY, which this directory already does elsewhere, and a target named after the method within the same options object. Both now detected. The look-ahead deliberately stops at that object's closing brace: a fixed forward window re-created the original false positive by matching an unrelated GitHub URL sixty lines below, which is the second time that exact trap has caught me in this file. NIT - the attestation test listed the other gate files but not itself, so its own removal from the CI step would have gone unnoticed. All five are listed now, including the new one. Writing the swallowsFailure test found a bug in swallowsFailure: `\b` after a colon never matches, because a word boundary needs a word character on one side, so `|| :` went undetected until its own test failed. Mutation-verified: the F4 revert now fails a test; `|| true` and continue-on-error on the audit step are both reported; a scalar `branches: dev` is reported; and removing the attestation test from the CI step fails it. Refs #9 Claude-Session: https://claude.ai/code/session_017iG7xA9wRvNyFYZSXm8inU
Review round 3 — glm-5.3Reviewed the commit before sol's fixes landed. Its BLOCKER came in two parts: part (a) was already closed by the sol round, part (b) was not — and (b) is the better finding. BLOCKER (b) — the audit can run while controlling nothingAppending That is worse than the audit being absent, because a green check is read as evidence. glm calls it "the classic reflex edit made to get a branch green", which is exactly the threat model the code itself names ("a conflict resolution during an upstream merge"). Both steps and the job are now checked for a discarded exit code and for This is instance #7 of the branch's recurring defect, and glm notes it sits in the code added to fix instance #6. MAJOR — a scalar
|
| # | mutation | caught |
|---|---|---|
| 38 | revert isDisabled to the fail-open string form |
yes, 1 of 47 |
| 39 | || true appended to the audit step |
yes |
| 40 | continue-on-error on the audit step |
yes |
| 41 | scalar branches: dev |
yes |
| 42 | drop the attestation test from the CI step | yes |
Only kimi-k3 round 3 outstanding.
kimi-k3 round 3. Its E2 is the purest instance of this branch's recurring
defect, and nothing caught it.
Add `if: github.repository == 'anomalyco/opencode'` to test.yml::unit and
delete its row from ALLOWED. The check then reports "33 guarded" and exits 0 -
counting its OWN job as guarded, which means that job never runs in this fork,
which means the audit never runs in this fork. Everything else stayed green:
both step names present, envelope digest unchanged because it excludes jobs,
both pairing checks satisfied, stale check 2/2.
And the failure message coached the first half of the edit, because it told the
reader every job needs the guard. The audit's own remediation text prescribed
the change that retires it. Both halves are now checked - the job must be in
ALLOWED and must not carry the guard - and the message states the exception
rather than concealing it.
Three more trigger shapes that passed while leaving the default branch
uncovered:
- `push: { tags: [...] }` with no `branches` key. A push trigger restricted to
tags never fires for branch pushes, so an absent `branches` key is not "all
branches" once tags are present.
- `pull_request: { types: [labeled] }`. Narrowing types stops the trigger
firing for the activity that matters, a pull request being opened or
updated.
- `on: [push, pull_request]`, the YAML shorthand, was a FALSE POSITIVE:
treating `on` as a map made `"push" in on` test array indices, so valid
shorthand was reported as missing both triggers.
kimi's E3 and E4 - continue-on-error, and dropping the attestation test from
the CI step - were both closed in the previous commit from glm's round, which
found the same class independently.
On the write detector, kimi listed shapes it misses: Octokit and axios calls
carry no method literal at all, the verb can be a variable, and `gh issue
close` writes without naming a method. Those are not settleable by a regex, and
its fair point was that declining a class is defensible only if the tests pin
it - so they are now asserted as known misses, with the reason: the workflow
guards are the containment boundary and this is a backstop for a future
oversight. If one of those shapes appears in script/, that test goes red.
Three cheap wins from the same list are fixed rather than declined: a
backticked ``method: `POST` ``, a space before the colon, and comments being
scanned as code - a doc comment mentioning a GitHub URL above an unrelated
write was enough to flag it.
Mutation-verified: E2 verbatim now exits 1 and is reported by both clauses;
tags-only push and narrowed pull_request types are both reported; the array
shorthand is accepted.
Refs #9
Claude-Session: https://claude.ai/code/session_017iG7xA9wRvNyFYZSXm8inU
Review round 3 — kimi-k3It built a fixture of the real 27 workflows plus the pinned script and ran four end-to-end disable scenarios against it. E3 and E4 were closed in the glm round, which found the same class independently. E2 was caught by nothing, and it is the purest instance of this branch's recurring defect so far. E2 — the audit counts its own job as guardedTwo edits, each looking like routine merge-conflict resolution:
Result: exit 0, "33 guarded". The audit reports its own job as properly guarded — which means that job never runs in this fork, which means the audit never runs in this fork. Every other control stayed green: both step names present, envelope digest unchanged (it excludes jobs by construction), both pairing checks satisfied, stale check 2/2. And the sting, which kimi puts better than I would: the audit's own failure text coached the first half of the edit. It tells the reader "Each job needs Both halves are now asserted — the audit's job must be in kimi also corrected my own framing: I had written the residual as "removing BOTH steps in one edit". The actual uncovered class was "any edit leaving both step names in place", which is much larger and includes single edits. Three more trigger shapes that left the default branch uncovered
On the write detector — declined, but now pinnedkimi listed shapes it misses: Its fair point is the one I acted on: declining a class is defensible only if the tests pin it, so it is acknowledged rather than silent. Those shapes are now asserted as known misses, with the reason recorded — the workflow guards are the containment boundary; this is a backstop for a future oversight. If one of them appears in Three items from the same list were cheap enough to fix rather than decline: a backticked Recorded affirmationskimi measured Mutation ledger — 45 tested
Where this stands15 commits. 45 mutations tested. 25 findings accepted across 12 reviewer rounds (codex ×3, glm-5.3 ×3, kimi-k3 ×3, sol ×3), plus 4 found by my own probes and 2 reviewer claims refuted with evidence. The recurring defect is now at eight instances, and the pattern is worth stating plainly because it is the actual finding of this PR: a control that asserts presence — a name, a substring, a digest, a literal in an array — where it means to assert behaviour.
Seven of the eight are mine, and three were introduced in a fix for a previous instance. |
…rded
sol round 4, and the ninth instance of this branch's recurring defect - the
second one inside a fix for the eighth.
auditJobModeViolations asked whether the audit job's condition CONTAINED the
repository guard. That asserts one spelling where the requirement is a
behaviour. Three conditions skip the job while satisfying it:
if: false
if: github.repository_owner == 'anomalyco'
if: github.event_name == 'schedule'
All three measured as zero violations before this change. GitHub reports a job
skipped by its own `if` as SUCCESS, even when the check is required - so CI
stays green. And every control in this file runs INSIDE that job: the step
attestation, both digest families, the pairing checks and all 153 tests. A
skip takes the lot, and the audit reports nothing rather than reporting a
problem.
The requirement is now stated as what it is: the audit's own job must carry no
condition at all. The guard-specific message is kept for that case, because it
needs different advice - allowlist it rather than remove the condition.
Mutation-verified: `if: false` on test.yml::unit now exits 1; five condition
shapes are pinned in audit-checks.test.ts, including `if: true`, which is still
a condition.
Refs #9
Claude-Session: https://claude.ai/code/session_017iG7xA9wRvNyFYZSXm8inU
Review round 4 — solRound 4 asked one question only: enumerate the minimal diffs that leave CI green while the guard, or the audit enforcing it, stops protecting this fork. sol returned one finding, and it justifies the round on its own. High — any non-canonical false condition on the audit's own job disables everything
if: ${{ 'anomalyco/opencode' == github.repository }} # the same guard, operands reversed
if: github.repository_owner == 'anomalyco' # an owner-level fork guard
if: github.event_name == 'workflow_dispatch' # an event optimisation
if: false # least subtleMeasured before the fix — all four returned zero violations, and all 56 tests stayed green. CI stays green too, and this is the part that makes it serious: GitHub reports a job skipped by its own This is instance #9 of the recurring defect, and the second one inside a fix for the eighth. The requirement is now stated as what it is: the audit's own job must carry no condition at all. The guard-specific message is kept for that one case because it needs different advice — allowlist it, don't just delete the condition. Mutation-verified: One thing recorded rather than fixedsol's first variant — I am not extending the expression parser again for it, and the reason is empirical rather than lazy: two of the nine instances above were introduced by a change to that parser, and a third (the forward-window regression) came from extending the write detector. The parser is the part of this PR with the worst track record for collateral damage, so a change that buys ergonomics rather than safety does not earn it. Filed as a follow-up instead. codex, glm-5.3 and kimi-k3 round 4 still running. |
codex round 4. Four findings; one duplicated sol's audit-job condition, which
the previous commit closed. The other three are new.
The tenth instance of this branch's recurring defect, and the one I should
have anticipated: every test added so far imports HELPER functions, so main()
itself was never exercised. codex's mutation is one line -
if (violations.length === 0) return -> return
- and the audit then always exits 0 while all 153 helper tests stay green.
Deleting the process.exit(1) has the same effect. The helpers were present and
correct; nothing asserted the behaviour that uses them.
audit-cli.test.ts now runs the real script as a subprocess in a faithful copy
of the repository layout - the script reads `../.github/workflows/` relative to
its own location and imports nothing local, so a copy at <tmp>/script/ reads
<tmp>/.github/workflows/. Three cases: the real tree exits 0, which
establishes the harness is faithful; a removed guard exits 1 and names the job;
and a conditional audit job exits 1. Each mutation is checked for having
actually applied, because a mutation that silently fails to apply would make
the test pass for the wrong reason - that has already caught me once in this
PR.
Verified: codex's mutation fails 2 of the 3 new tests while all 158 helper
tests still pass, which is precisely its point.
swallowsFailure matched `|| true`, `|| :`, `; true` and `set +e`, so it missed
`|| exit 0` - and `exit 0` as a line of a multi-line run, which makes a gating
step unconditionally successful whether it precedes or follows the command.
`|| exit 1` is still correctly ignored, because it propagates failure. Also
added `set +o errexit`.
Mutation-verified: `|| exit 0` on the audit step is reported; `exit 0` before
the command in the test step is reported; and dropping the new CLI test from
the gate fails the attestation.
Refs #9
Claude-Session: https://claude.ai/code/session_017iG7xA9wRvNyFYZSXm8inU
Review round 4 — codexFour findings. One duplicated sol's audit-job condition, closed in the previous commit. The other three are new, and the first is the one I should have anticipated. Instance #10 — every test imported helpers, so
|
| # | mutation | caught |
|---|---|---|
| 46 | if: false on the audit's own job (sol) |
yes |
| 47 | main() returns early, never failing (codex) |
yes, 2 of 3 CLI tests — and no helper test |
| 48 | remove the process.exit(1) |
yes, 2 of 3 CLI tests |
| 49 | ` | |
| 50 | exit 0 before the command in the test step |
yes |
| 51 | drop the CLI test from the gate | yes, attestation |
glm-5.3 and kimi-k3 round 4 still running.
…b skips
glm-5.3 round 4. Four live bypasses, and its verdict was "not converged" -
correctly.
The eleventh instance: `needs`. A one-line guarded `gate` job with `needs:
gate` on the audit job skips it, and GitHub reports a job whose dependency was
skipped as SUCCESS - the same semantics the previous commit cited for `if`. The
audit job carries no `if` key, so the check I added for sol's finding passed
structurally. glm verified `auditJobModeViolations({ needs: ["gate"] })`
returned []. The fix for the ninth instance asserted the absence of an `if`
KEY, where the requirement is that this job produces a running instance.
Also rejected now: a matrix with an empty dimension, and a dynamically
computed matrix, both of which skip the job with no `if` and no `needs`. The
real static matrix still passes, so this is not merely a stricter false
positive.
The other three findings share one root cause, and glm named the right fix.
`swallowsFailure` is a blacklist of spellings, so each fix invited the next:
`|| true`, then `|| exit 0`, then `|| echo "findings above"` - which is clean
under `bash -eo pipefail`, because `-e` does not abort the left operand of `||`
and the compound takes echo's status. And `run.includes(...)` accepted `cat
script/check-workflow-guards.ts`. Both legs of the attestation shared that
weakness, so they passed together: one commit is literally titled "check that
the audit steps RUN, not that they are named" and implements a substring test.
Both steps' scripts are now pinned to EXACT equality, normalised only for
incidental whitespace, the way the job digests are. That closes the whole
family in one change rather than extending a blacklist: `|| echo`, `|| { true;
}`, `cat`, and any future spelling all fail. It also closes glm's fourth
finding - a gate file kept in the `ls` line but dropped from the `bun test`
invocation - because the invocation is part of the pinned text. The gate's file
list now has a single definition, so `ls` and `bun test` cannot drift apart.
swallowsFailure is kept as an independent second signal, with its tests.
Writing this broke the detector helpers in write-guard-coverage.test.ts: my
edit's bounds swallowed githubWrites and its helpers, which the suite caught as
26 failures. Restored and redone against precise anchors.
Mutation-verified: the needs-gate bypass, `|| echo`, and the cat-neuter are all
reported, and the last is caught by both legs.
Refs #9
Claude-Session: https://claude.ai/code/session_017iG7xA9wRvNyFYZSXm8inU
Review round 4 — glm-5.3Verdict: not converged. Four live bypasses, two of them full guard retirements in ≤2 routine-looking hunks. All four fixed; its structural insight was the valuable part. Instance #11 —
|
| # | mutation | caught |
|---|---|---|
| 52 | guarded gate job + needs: gate on the audit job |
yes |
| 53 | || echo "findings above" |
yes, exact-equality pin |
| 54 | cat script/check-workflow-guards.ts |
yes, both legs |
kimi-k3's round 4 is being re-run: its first attempt was denied permission to create a fixture under /tmp/audit-sim and stopped after 7 KB with no findings. Relaunched with a permitted scratch path and told which findings have already landed, so it aims at the newest code.
Review round 4 — kimi-k3, and a convergence assessmentkimi's round: two claims, neither reproducesIts first attempt was denied permission to create a fixture under Claim A — a bun test guard-expression.test.ts 2>&1 | tail -4; echo "exit=$?"
So a hostile bunfig does not vacate the gate: I am not scoring this smugly — it is the same class of error I nearly logged myself at M19, where a Claim B — adding a step to the audit's own job passes. Does not reproduce: a new step changes the job body, so the pinned digest trips and the audit exits 1. Verified at HEAD. So kimi's round produced no reproducible finding, which is the first time that has happened. Convergence: two different questions, with two different answersRound 4 found real defects from three reviewers, so by the stop-on-quality rule it earned its place. But the findings have separated into two classes that deserve separating, because only one of them converges. Question 1 — can this fork still take an outward-facing action? Converged. Nothing has been found against the actual guard since round 1. The 24 job guards, the Question 2 — can someone with commit access disable the audit? Unbounded, and it always will be. Rounds 3 and 4 were an enumeration of that space: The floor is architectural, not a matter of more checks: an in-CI check cannot be the last line of defence against edits to the CI that runs it. The thing that would actually close class 2 is a repository setting this code cannot assert — required status checks and branch protection on Filed as a follow-up rather than stretched into more rounds of the same. What the current code does achieve for class 2 is worth stating plainly: every single-edit disable found by four models is now caught, and the residual requires a deliberate multi-file edit whose intent is unmistakable in a diff. I am running one final round scoped to question 1 only — the guard itself, not the meta-checker — and will treat a clean result as convergence. |
sol round 5, and the only outward-facing action it could still find reachable from this fork as the files stand. `bun turbo test` and `bun turbo typecheck` run with telemetry on by default - Turbo 2.10.2 POSTs anonymous usage data to Vercel - and nothing in the repository sets `TURBO_TELEMETRY_DISABLED` or `DO_NOT_TRACK`. Verified: no opt-out existed anywhere in the tree. Low severity and pre-existing upstream rather than introduced here, but this PR's whole subject is stopping a private fork taking outward-facing actions, and the two allowlisted workflows are the entire surface, so opting out there covers it. Two lines. Both standard variables are set: `DO_NOT_TRACK` because it is the cross-tool convention, `TURBO_TELEMETRY_DISABLED` because it is the one Turbo documents. The envelope digests for both workflows are updated, which is the pin working as designed - workflow-level `env` is exactly what sol's earlier MAJOR added that digest to cover, and a change to it should require an acknowledgement. sol's other observations are recorded as correct and out of scope: the Actions cache and artifact writes are fork-local, and Dependabot's PRs are inward. It confirmed no path at this HEAD writes to anomalyco/opencode, publishes to npm, ghcr or a marketplace, deploys to AWS, or reaches Discord, PostHog or Sentry. Refs #9 Claude-Session: https://claude.ai/code/session_017iG7xA9wRvNyFYZSXm8inU
Review round 5 (final) — convergenceScoped to one question: with this branch merged and no further edits, can this fork take an outward-facing action or write to upstream? All four reviewers answered independently.
Each measured the audit's exit code directly after the previous round's artefact, and each said how. glm ran a negative test in a sol's Low — fixed: Turborepo telemetryThe one outward-facing action still reachable. Pre-existing upstream rather than introduced here, and genuinely low. But this PR's whole subject is stopping a private fork taking outward-facing actions, and the two allowlisted workflows are the entire surface — so two lines covers it. Both variables are set: Both envelope digests updated, which is the pin working as designed — workflow-level What the panel independently confirmed
Boundary notes the panel raised, recorded not fixed
Final state19 commits. 54 mutations verified. 30 findings accepted across 20 reviewer rounds (codex ×5, glm-5.3 ×5, kimi-k3 ×5, sol ×5), plus 4 found by my own probes, 3 reviewer claims refuted with evidence, and 2 corrections to my own earlier statements. Ready for review. |
The duplicate key fixed in the previous commit was detectable by the repository's own linter for as long as it existed. Two reasons nothing caught it, and both are fixed here rather than filed. First, no workflow runs lint at all - verified by grepping .github/workflows. Second, and worse: `bun run lint` would have exited 0 anyway, because .oxlintrc.json sets almost everything to "warn", including no-dupe-keys. So even wiring up the existing script would not have caught this bug. `--deny-warnings` is not an option - there are hundreds of pre-existing style warnings and a gate that fails on all of them gets switched off in a week. .oxlintrc.gate.json instead promotes thirteen rules to errors, chosen so that a hit is almost certainly a defect rather than a preference: duplicate keys, class members, else-if branches and switch cases; self-assignment and self-comparison; unsafe negation; isnan misuse; constant binary expressions; a getter or setter that does not return; assigning to an import; unreachable code. It runs with typeAware off, which matters: the repository's type-aware config needs more than 6 GiB on this monorepo and was OOM-killed at that cap during this work. With it off the gate is ~2 seconds and 0.25 GiB. Every rule in it is syntactic, so nothing is lost. Invoked through a `lint:correctness` script so CI uses the pinned oxlint devDependency (1.60.0) rather than whatever `bunx` would fetch that day. One pre-existing error had to be fixed for the gate to be green on landing, in packages/web/src/types/lang-map.d.ts: `export interface MapReturn` alongside `export = map` is TS2309. The interface is the return type of `map()` and is referenced nowhere else, so dropping its `export` resolves it and consumers still get it structurally through that signature. A gate that lands red is not a gate. Mutation-verified through the exact CI command: reintroducing the duplicate key makes `bun run lint:correctness` exit 1, and the clean tree exits 0. The digest for test.yml::unit is updated because this adds a step to an allowlisted job - the pin from #22 demanding that acknowledgement on an unrelated PR is it working as intended. Refs #10, #26 Claude-Session: https://claude.ai/code/session_017iG7xA9wRvNyFYZSXm8inU
…es (#25) * test: record the root causes behind #10 before fixing them Investigation only. Three groups, three different causes: - the CLI trio fails at the harness deadline, and the harness hides that by synthesizing exitCode -1, so a timeout is indistinguishable from a behavioural break - the write-permissions test asserts 0o644 while the tool sets no mode, so it is asserting this machine's umask; it passes in CI by coincidence - snapshot-tool-race passes in isolation, so it needs a synchronisation point Refs #10 Claude-Session: https://claude.ai/code/session_017iG7xA9wRvNyFYZSXm8inU * test: make a deadline kill say so, and stop asserting this machine's umask #10's stated cost is that "every PR author has to re-derive whether their failure is real". That is a symptom of attribution, not of timing, so the attribution is fixed first and the deadlines second. The write-permissions test was not flaky at all - it asserted the umask. umask 0022 (CI) -> Bun.write yields 0644 -> passed umask 0002 (dev box) -> Bun.write yields 0664 -> failed Measured both ways on the same commit. `src/tool/write.ts` sets no mode, so the test's own name - "sets file permissions when writing sensitive data" - claimed an intent the code does not implement, and it passed in CI by coincidence. Forcing a fixed mode would be the wrong fix, and this is the part worth recording: it would LOOSEN permissions for anyone with a stricter umask, where 0o077 should yield 0o600. Restricting a file is the umask's job. What is worth pinning is that the tool leaves that decision alone, so the expectation is now derived (`0o666 & ~process.umask()`) and the test fails if a chmod or mode option is ever introduced. Verified under umask 0002, 0022 and 0077, and by mutation: adding a chmod 0o600 to the tool fails it. The CLI harness hid its own timeout. cli-process.ts catches AppProcessError - which covers a timeout AND a spawn failure - and synthesized exitCode -1, so a run killed at its deadline failed as `expected 0, received -1`. Indistinguishable from a behavioural break unless someone read stderr, which is exactly the re-derivation #10 describes. RunResult now carries `timedOut`, set using the same predicate core already uses for this distinction in packages/core/src/tool/bash.ts, and expectExit reports a deadline kill as what it is. The three tests that failed in CI now assert through expectExit rather than a bare `toBe(0)` so they get it. Verified by forcing a kill: the failure now reads "the harness killed this run at its 1047ms deadline ... This is the timeout backstop, NOT a behavioural failure", with the knob to confirm it. Deadlines are scaled from one place (test/lib/deadline.ts) rather than being fixed values tuned on one machine: x3 under CI, overridable with OPENCODE_TEST_TIMEOUT_SCALE. Deliberately keyed on CI rather than GITHUB_ACTIONS, because the unit job sets GITHUB_ACTIONS=false explicitly. Also fixed the last instance of the pattern this file's own anomalyco#27371 comment warns about: the SIGINT test bounded `durationMs` by the same 30_000 as its own test budget, so the assertion could only fail by racing the thing that would have killed the test anyway. It now bounds at a third of the budget, which still catches a hang. snapshot-tool-race: budget raised and its exhaustion made self-describing, but the cause is NOT established and the commit should not imply otherwise. Measured in isolation three times, the diff is present on the FIRST poll, so the old 5s budget was never the binding constraint there. #10 records it failing only in a full run. A full-suite reproduction is running; until it says otherwise, this change makes the next occurrence attributable rather than fixing a known mechanism. Refs #10 Claude-Session: https://claude.ai/code/session_017iG7xA9wRvNyFYZSXm8inU * test(e2e): fix the duplicate sessionStatus key in the todo-dock spec The e2e spec that failed three attempts in one CI run, then passed three runs, passes `sessionStatus` TWICE in the same mock config: sessionStatus: { [sourceID]: { type: "busy" } }, <- discarded ... sessionStatus: () => sessionStatus, <- wins JavaScript keeps the last key, so the first never had any effect and the intended "source session starts busy" was silently lost. The mock instead read a getter over a map that started EMPTY, which left the test depending entirely on a pushed SSE event arriving with no initial-state fallback. Confirmed by the repository's own linter, and it is the only such warning in the whole e2e directory: session-todo-dock-navigation.spec.ts:60:5 eslint(no-dupe-keys): Duplicate key 'sessionStatus' Both intents are preserved: the map is seeded busy at declaration and still read through the getter, so the test can mutate it later. The now-redundant re-assignment is removed and the status event kept, since the page still has to be told. What this does NOT claim: that it is the whole cause. The spec also asserts animation state by SAMPLING opacity during fixed windows and requiring a sample to land mid-transition, which is a race between the sampler and the animation. That is a candidate too, and it is not yet measured. The fix here is a definite defect found by a linter nothing runs, with a plausible mechanism for a failure that reproduces within a run and not across runs. Refs #10, #26 Claude-Session: https://claude.ai/code/session_017iG7xA9wRvNyFYZSXm8inU * ci: gate on the lint rules where a hit is almost certainly a bug The duplicate key fixed in the previous commit was detectable by the repository's own linter for as long as it existed. Two reasons nothing caught it, and both are fixed here rather than filed. First, no workflow runs lint at all - verified by grepping .github/workflows. Second, and worse: `bun run lint` would have exited 0 anyway, because .oxlintrc.json sets almost everything to "warn", including no-dupe-keys. So even wiring up the existing script would not have caught this bug. `--deny-warnings` is not an option - there are hundreds of pre-existing style warnings and a gate that fails on all of them gets switched off in a week. .oxlintrc.gate.json instead promotes thirteen rules to errors, chosen so that a hit is almost certainly a defect rather than a preference: duplicate keys, class members, else-if branches and switch cases; self-assignment and self-comparison; unsafe negation; isnan misuse; constant binary expressions; a getter or setter that does not return; assigning to an import; unreachable code. It runs with typeAware off, which matters: the repository's type-aware config needs more than 6 GiB on this monorepo and was OOM-killed at that cap during this work. With it off the gate is ~2 seconds and 0.25 GiB. Every rule in it is syntactic, so nothing is lost. Invoked through a `lint:correctness` script so CI uses the pinned oxlint devDependency (1.60.0) rather than whatever `bunx` would fetch that day. One pre-existing error had to be fixed for the gate to be green on landing, in packages/web/src/types/lang-map.d.ts: `export interface MapReturn` alongside `export = map` is TS2309. The interface is the return type of `map()` and is referenced nowhere else, so dropping its `export` resolves it and consumers still get it structurally through that signature. A gate that lands red is not a gate. Mutation-verified through the exact CI command: reintroducing the duplicate key makes `bun run lint:correctness` exit 1, and the clean tree exits 0. The digest for test.yml::unit is updated because this adds a step to an allowlisted job - the pin from #22 demanding that acknowledgement on an unrelated PR is it working as intended. Refs #10, #26 Claude-Session: https://claude.ai/code/session_017iG7xA9wRvNyFYZSXm8inU * style: format the lint gate config Prettier flagged it after the previous commit; the pre-commit hook runs typecheck but not format, so it landed unformatted. Refs #10 Claude-Session: https://claude.ai/code/session_017iG7xA9wRvNyFYZSXm8inU * test(e2e): sample the todo dock until a condition, not for a fixed window The duplicate-key fix in the earlier commit was a real defect but NOT the cause of this spec's flakiness. The control run is what showed that: running the unmodified spec alongside the fixed one reproduced the failure locally for the first time, at expect((await opening).some((s) => s.opacity > 0.05 && s.opacity < 0.95)) which requires a sample to land inside a narrow opacity band while a spring animation runs. There were three independent flake sources here, and measuring each one took a separate run: 1. that narrow-band assertion, which is the failure CI hit three attempts in a row 2. a 700ms return window that sometimes closed before the dock reappeared, so the assertion failed on an empty array rather than on the dock 3. a settle threshold I added myself in a first attempt - max opacity > 0.98 - which the spring does not reach inside the window. Measured at 0.878927. It doubled the failure rate. The opacity is not a CSS transition, so there is no transitionrun or animationstart to observe: it is a reactive style, `opacity: 1 - shut()`, driven by useSpring through a dockProgress prop. The Web Animations API does not apply. So sampling stays, but every window becomes condition-driven with time as a bounded backstop - the same doctrine as the unit harness change in this PR. `sampleDock(page, "appears" | "opaque" | "absent")` samples until the thing being waited for has happened, then a few frames past it so the caller sees the settled state too. Assertions check the RAMP - it started faint and rose - rather than any particular instant. The absence check keeps a bounded window on purpose, because absence is a claim about an interval rather than an event to wait for. The "appears" condition still applies to it, so a dock that wrongly returns ends sampling immediately and fails, instead of costing the full budget. Measured, 10 repeats each, single worker, chromium 1217: base (unmodified) 2 failed / 8 passed attempt 1 (max > 0.98) 4 failed / 6 passed attempt 2 (longer window) 2 failed / 8 passed condition-driven 10 passed Refs #10, #26 Claude-Session: https://claude.ai/code/session_017iG7xA9wRvNyFYZSXm8inU * test(e2e): scale the shared app-readiness and expect timeouts Found by the confirmation run rather than by reading: repeating the todo-dock spec ten times passed 10/10 on an idle box, then 9/10 on a loaded one. The one failure was not the dock at all: expect(locator).toBeVisible() failed Locator: getByRole('heading', { name: 'Todo dock animation' }) Timeout: 30000ms at utils/waits.ts:6 That is APP_READY_TIMEOUT - the app shell never rendered inside 30s - which asserts nothing about what the spec is testing. The same run took 9.6 minutes against 4.2 for the identical work, so the box was roughly 2.3x slower. A fourth distinct flake source in one spec, and the same class as the other three: a fixed wall-clock bound tuned on a fast, idle machine. APP_READY_TIMEOUT is used across 34 e2e files, so scaling it reaches the whole suite rather than the spec that happened to expose it. The config's `expect.timeout` is scaled too, because that 10s bound is exactly what #10 records the other two named e2e specs failing on - Playwright `toBeVisible` timeouts, which are a statement about machine speed rather than about the UI. One knob for both suites, OPENCODE_TEST_TIMEOUT_SCALE, keyed on CI rather than GITHUB_ACTIONS since the unit job sets that to false explicitly. Verified wired: scale 3 gives 90s app-ready and 30s expect, unscaled stays 30s and 10s. Honest measurement of the dock spec across 20 repeats, and it is NOT zero: base (unmodified) 2 failures in 10 condition-driven 0 in 10, then 1 in 10 under load The remaining one is this readiness timeout, not a dock assertion - no failure at any of the three dock lines across all 20 runs. Refs #10, #26 Claude-Session: https://claude.ai/code/session_017iG7xA9wRvNyFYZSXm8inU
GitHub evaluates these identically:
github.repository == 'anomalyco/opencode'
'anomalyco/opencode' == github.repository
The check matched the guard's exact text as a prefix, so the second was
reported unguarded. Fail-closed, so never an exposure - but a false positive
on a correct condition, and a check that rejects valid input gets worked
around rather than fixed. That is the whole reason this file exists.
Deliberately NOT a second accepted literal. Two of the eleven defects found
during #22's review came from extending this parser by adding a pattern, and a
third came from extending the write detector's URL matching, so this adds a
RULE instead: the leading term must be an equality between the guard's own two
operands, in either order, compared as a set.
That also means it does not need updating if the guard's spelling ever
changes, and it rejects the near-misses by construction rather than by
enumeration - a different repository, a different field, a self-comparison.
I declined this during #22 on the grounds that the parser had the worst
collateral-damage record in that change and the fix bought ergonomics rather
than safety. That reasoning held while the PR was in flight with five review
rounds against it; it does not hold now, and leaving a known false positive in
a guard is how guards get disabled.
13 shapes verified, then pinned as tests: the reversed form bare, conjoined,
wrapped in ${{ }} and bracketed; every bypass closed in #22 still closed,
including the quoted-paren hide and a self-comparison. Mutation-verified:
restoring the exact-prefix match fails 4, dropping the operand-set check
fails 1. Full guard suite 188/0, audit green on the real tree.
Closes #23
Claude-Session: https://claude.ai/code/session_017iG7xA9wRvNyFYZSXm8inU
GitHub evaluates these identically:
github.repository == 'anomalyco/opencode'
'anomalyco/opencode' == github.repository
The check matched the guard's exact text as a prefix, so the second was
reported unguarded. Fail-closed, so never an exposure - but a false positive
on a correct condition, and a check that rejects valid input gets worked
around rather than fixed. That is the whole reason this file exists.
Deliberately NOT a second accepted literal. Two of the eleven defects found
during #22's review came from extending this parser by adding a pattern, and a
third came from extending the write detector's URL matching, so this adds a
RULE instead: the leading term must be an equality between the guard's own two
operands, in either order, compared as a set.
That also means it does not need updating if the guard's spelling ever
changes, and it rejects the near-misses by construction rather than by
enumeration - a different repository, a different field, a self-comparison.
I declined this during #22 on the grounds that the parser had the worst
collateral-damage record in that change and the fix bought ergonomics rather
than safety. That reasoning held while the PR was in flight with five review
rounds against it; it does not hold now, and leaving a known false positive in
a guard is how guards get disabled.
13 shapes verified, then pinned as tests: the reversed form bare, conjoined,
wrapped in ${{ }} and bracketed; every bypass closed in #22 still closed,
including the quoted-paren hide and a self-comparison. Mutation-verified:
restoring the exact-prefix match fails 4, dropping the operand-set check
fails 1. Full guard suite 188/0, audit green on the real tree.
Closes #23
Claude-Session: https://claude.ai/code/session_017iG7xA9wRvNyFYZSXm8inU
The gate added in #22 ran `oxlint --config .oxlintrc.gate.json .` over the whole inherited tree. Its purpose is to catch bugs in what we write, but as built it policed code we merely inherit -- so upstream's choices could block every future sync. This sync is the first instance: 20 no-unreachable errors, 0 before the merge. They are not bugs. Upstream retires legacy console endpoints by inserting a throw at the top of the handler and leaving the old body in place: const user = Actor.assert("user") throw new Error("Go subscriptions have moved to the new Console") const { successUrl, cancelUrl, method } = input The rule is right and the code is deliberate. Fixing it in this fork would mean diverging on upstream-owned files and re-resolving it on every sync, which is the cost this gate exists to avoid paying elsewhere. Excludes packages/console/** -- which our 16 commits touch in 0 places -- and artifacts/**, which is scratch output rather than shipped source. No coverage of our own code is lost. Verified the gate still guards: injecting `throw` followed by a return into packages/opencode/src/cli/cmd/db-export-usage.ts is still reported as no-unreachable, and the tree is 0 errors once reverted. Claude-Session: https://claude.ai/code/session_017iG7xA9wRvNyFYZSXm8inU
Closes #9
Why this is more urgent than #9 described
#9 predicted that publish/deploy could fire from this fork. The audit found that the two workflows it named are in fact already guarded upstream — and that the real exposure is elsewhere, and is not hypothetical. Three things were already happening:
1. This fork attempted a daily write to the upstream repository.
script/github/close-issues.ts:3hardcodes the target:close-issues.ymlruns on a 02:00 cron here. It enumerated upstream's open issues and attempted to comment on and close them. It failed only because the fork'sGITHUB_TOKENhas no write access there:Issue anomalyco#38226 does not exist in this fork — this repo has 24 issues.
close-prs.ymlis the same shape, also 403 (run 35540524727). The standing rule is that we never write to the upstream fork parent; the only thing enforcing it was a token scope we do not control.2. Upstream's compliance bot already closed one of our PRs.
pr-standards+compliance-close(every 30 minutes, 7 successful runs) auto-closed our own PR #6 for not matching upstream's PR template:3.
publish.yml's guard covered four jobs out of five — and not the one that publishes.version,build-cli,sign-cli-windowsandbuild-electronall carried the guard. Thepublishjob — which logs into ghcr.io asgithub.repository_owner(swxtchio) and publishes — did not:A skipped
needis not a failure, so that condition is true when every guarded job skips. Verified on run 35539779127, a push todevon this fork:It was eligible to run. It queued on a
blacksmith-*runner this fork does not have and was cancelled; that is an accident of infrastructure, not a control.Also found:
docs-update.ymlguarded ongithub.repository == 'sst/opencode', a stale organisation name, so its guard protected us only by coincidence.The change
docs-update::update-docs, had a guard naming a stale organisation and was re-pointed; 25 jobs across 21 workflow files are now under the canonical guard.)publish.yml'spublishjob guarded, closing thealways()hole.close-issues.tsandclose-prs.ts, at the point of the write. The invariant is "a script may only write to the repository it runs in" rather than "is this upstream", so it holds for any future fork and does not depend on an organisation name. It fails closed whenGITHUB_REPOSITORYis unset, runs before credentials are acquired, and exits 0 rather than 1 — a fork skipping inherited maintenance is the correct outcome, not a nightly failure to investigate.script/check-workflow-guards.ts, run fromtest.yml, asserting coverage and failing closed, so a job inherited in a future upstream merge is a violation until classified.write-guard-coverage.test.tsfails if any script issues a GitHub write without importing the guard, so a future script cannot quietly omit it.Allowlist is
test.yml::unit,test.yml::e2eandtypecheck.yml::typecheck— the jobs this fork adapted for its own use. It is keyed per job, and each entry pins a digest of the job definition, its workflow's envelope (on,permissions,env), and any local action the job calls. An allowlisted job is by definition unguarded, so a change to one must be read rather than inherited silently.The audit also asserts that it is still running and still gating: its own job must be allowlisted, unconditional, and free of
needsor an empty matrix; its two steps must exist, carry no condition orcontinue-on-error, and run exactly the pinned script; andaudit-cli.test.tsruns the real command line so that guttingmain()fails a test.Deliberate behaviour changes, flagged
pr-standardsandpr-managementare guarded off, so our PRs stop being checked against upstream's title and template conventions — the conventions whose enforcement closed PR feat(llm): record the model that actually served each turn #6. glm-5.3's argument for this is better than mine was: the logic is not merely mis-targeted but upstream-specific, fetching.github/TEAM_MEMBERSat refdev, and this fork's default branch isswxtch, so an adapted copy would 404. A fork-native replacement should enter as a new allowlisted workflow with its own digest, which the machinery now supports. Reversible by adding either job toALLOWED.docs-update.ymlis re-pointed off the stalesst/opencodeliteral. It stays inert here either way, but would become live upstream — flagged because we never send changes there.AGENTS.mdnow states the fork's default branch. Its claim that the default isdevis upstream's text, andrefs/remotes/origin/HEADstill points atorigin/devfrom a pre-fork clone. That stale pair cost a reviewer a false BLOCKER during this review, so it is corrected with the command that resolves it.Review
Five rounds, four independent reviewers each round (codex, glm-5.3, kimi-k3, sol) — adjudication in the comments. 29 findings accepted, 54 mutations verified, 2 reviewer claims refuted with evidence.
One defect class recurred eleven times: a control asserting presence — a name, a substring, a digest, a literal in an array — where it means to assert behaviour. Ten of the eleven were mine, and three were introduced in a fix for a previous instance. The pattern is the real finding of this PR:
publish.yml's guardmain()failsifkeyResidual, tracked not hidden
swxtch. An in-CI check cannot be the last defence against edits to the CI that runs it. Ten single-edit disables were found and closed, but the class is unbounded by construction; branch protection is what closes it, and it is a repository setting this code cannot assert.gh issue close) are asserted as known misses in the tests, with the reason recorded — the workflow guards are the containment boundary; that detector is a backstop for a future oversight.🤖 Generated with Claude Code
https://claude.ai/code/session_017iG7xA9wRvNyFYZSXm8inU