Repository navigation
feat(llm): record the model that actually served each turn - #6
Merged
Merged
Conversation
brentyates-swx
force-pushed
the
feat/record-served-model
branch
from
September 20, 2026 19:42
4e77bf8 to
e888367
Compare
|
This PR doesn't fully meet our contributing guidelines and PR template. What needs to be fixed:
Please edit this PR description to address the above within 2 hours, or it will be automatically closed. If you believe this was flagged incorrectly, please let a maintainer know. |
This was referenced Sep 20, 2026
test suite has at least 5 environment-sensitive tests: which one fails varies by run and machine
#10
Closed
brentyates-swx
force-pushed
the
swxtch
branch
from
September 20, 2026 21:51
c54185d to
417937c
Compare
opencode records the model it REQUESTED and discards the one the provider
reports back. For a direct model those are the same value, so nothing was
visibly wrong. For a ROUTED model they are not: Fireworks FireRouter and Azure
model-router take a route in the request and pick a member per request, so the
stored modelID names the route and nothing says who answered.
Two consequences. A session's history and footer show `firerouter/...` no
matter which member replied, and cost is computed from the single `cost` block
configured for the route, so every turn is priced at one rate regardless of
whether the cheap or the expensive member served it. Both are wrong by
construction, not by degree.
The value was already on the wire and was being dropped. Plumb it through:
schema/events.ts responseModelID on StepFinish and Finish
protocols/utils/lifecycle native runtime forwards it to both events
protocols/openai-chat response `model` (also covers openai-compatible-chat,
a thin wrapper, which is the FireRouter path)
protocols/anthropic-messages message_start.message.model (the Anthropic-
compatible FireRouter route)
protocols/openai-responses response.model
protocols/gemini modelVersion, which can be more specific than the
requested alias
session/llm/ai-sdk.ts response.modelId; the AI SDK puts it on finish-step
but NOT on finish, so adapter state carries it
session/processor.ts writes onto the assistant message and the step part
schema/v1/session.ts Assistant.responseModelIDs, StepFinishPart.responseModelID
bedrock-converse is deliberately NOT wired: its modelId is request-side only
and Bedrock invokes a model directly with no router, so there is nothing to
report and the field correctly stays absent.
Two properties the tests pin, because both are ways this could mislead a
reader rather than merely fail:
Absent means "the provider did not report one", never "same as requested". A
consumer must be able to tell those apart to know when falling back to modelID
is legitimate, so every extractor rejects a blank string rather than storing ""
as though it were an identity.
The message-level field is an ARRAY. A router may pick a different member
between steps of one turn; collapsing that to a single id would silently
misreport a mixed turn as if one model served all of it. Usually length 1, with
the exact per-step value on each StepFinishPart.
Claude-Session: https://claude.ai/code/session_017iG7xA9wRvNyFYZSXm8inU
brentyates-swx
force-pushed
the
feat/record-served-model
branch
from
September 20, 2026 21:53
e888367 to
88c84c7
Compare
|
This pull request has been automatically closed because it was not updated to meet our contributing guidelines within the 2-hour window. Feel free to open a new pull request that follows our guidelines. |
brentyates-swx
marked this pull request as ready for review
September 20, 2026 23:09
brentyates-swx
added a commit
that referenced
this pull request
Sep 21, 2026
* feat(tui): show the model that actually served a routed turn The turn footer rendered only the CONFIGURED model name, so every turn on a router route read "Build · Firerouter glm" no matter which member model answered. The route slug is not a model, so that line was hiding both what ran and what the turn cost. Renders the configured name decorated with what served it: Build · Firerouter glm (glm-5p3-flash) Build · Firerouter glm (glm-5p3-flash → glm-5p3) multi-step turn A direct provider echoes its own id back on every response, so a served list that resolves to the configured name is dropped and every non-routed footer stays byte-identical. Covers all three places that render the line - the interactive TUI, the markdown transcript export, and `opencode run` (both session replay and the live turn). The live path never saw the assistant message, so the served ids are carried to the footer as new FooterState, reset at turn.send so one turn's route cannot decorate the next turn's summary. Also regenerates the SDK types and openapi spec, which #6 left stale, and fixes the repo's one lint error (a Tailwind CSS escape read as a deprecated octal escape) so `bun run lint` is clean. Claude-Session: https://claude.ai/code/session_017iG7xA9wRvNyFYZSXm8inU * fix(run): label a finished turn with the model it ran on The live turn summary took its model from the COMPOSER SELECTION, so switching models while a turn was in flight labelled the finished turn with the model that had just been selected - naming a model that never saw the turn, with no sign anything was wrong. The assistant message records the identity the turn was actually dispatched to, so carry that to the footer alongside the served ids and prefer it. The selection survives only as the fallback for a turn that produced no assistant message at all. FooterState.served becomes FooterState.turnModel, one field carrying the identity and its served ids from one event with one reset, and patch() now merges it on key PRESENCE so turn.send's explicit undefined actually clears it instead of being read as "unchanged". Precedence lives in turnSummaryModel() rather than inline in the event handler, so the rule this commit exists for is directly testable; the test fails if the two branches are swapped back. Drops appendServedLabel, which is unreachable now that served ids never arrive without an identity. Claude-Session: https://claude.ai/code/session_017iG7xA9wRvNyFYZSXm8inU * fix(run): accumulate served models per turn and stop borrowing the selection Review round 1 (codex) found three real defects in the previous two commits. 1. A prompt produces one assistant message PER STEP - session/prompt.ts loops and creates a new Assistant each pass - while the run footer prints ONE summary for the whole prompt. Keeping the last message's record meant a tool-calling turn that routed A then B reported only B, silently dropping exactly the multi-model sequence this feature exists to show. Served ids now accumulate in first-seen order across every assistant message sharing a parent user message, keyed by that parentID so a new prompt starts a new record on its own evidence. 2. A turn that failed before recording any model still fell back to the composer selection, which is mutable while a turn is in flight - so a failed turn could be confidently labelled with a model chosen after it died, reproducing the bug the previous commit set out to fix. turn.send now snapshots the dispatched identity, which is the last moment the selection IS the truth, and the summary says "unknown model" rather than borrowing a selection. 3. Suppression compared resolved DISPLAY NAMES, so two different ids sharing a friendly name hid the served id - different versions, routes and prices read as identical. It compares raw ids again; only an id identical to the requested one is redundant. Accumulation lives in session-data rather than the footer's patch merge: turn.send writes a seed, so a merging patch would have folded the previous turn's models into it. Also pins the turn.send reset as a present-key-carrying-undefined (a value-based merge would drop it), and pins subagent isolation, which a reviewer suspected was broken and which nothing asserted. Truncation of the summary line at narrow widths is filed as #13. Claude-Session: https://claude.ai/code/session_017iG7xA9wRvNyFYZSXm8inU * test(run): gate the anomalyco#27371 regression on the exit code, not the clock The unknown-model regression test bounded its wall-clock duration by the SAME value it gave the harness as a kill timeout, so a slow-but-healthy run under parallel load was killed and then failed the duration assertion as if it had hung. It failed in every full-directory run of this suite and passed in isolation, on this branch and on swxtch alike. Worse, the assertion it was propping up did not do its job: the harness synthesizes exitCode -1 when it kills a run, so `not.toBe(0)` PASSED on a genuine hang. The wall clock was the only thing catching the very regression the test is named for. Asserting exitCode > 0 tests the real signal - the CLI exited with an error of its own accord - and a hang now fails on -1 with a message that says so. Verified by forcing the kill path: the assertion fails with "Expected: > 0, Received: -1". Elapsed time stays where it belongs, as the harness timeout backstop. Claude-Session: https://claude.ai/code/session_017iG7xA9wRvNyFYZSXm8inU * fix: aggregate served models per turn in every surface, not just the footer Review round 2 (codex, corroborated by glm-5.3) found the round-1 fix was applied in one place out of three, plus a wrong turn boundary. 1. The classic TUI and persisted replay still read a SINGLE assistant message. A turn's intermediate messages finish with "tool-calls", so final() is false and only the LAST message renders a footer - which means a tool-calling turn routed A then B displayed just B. The last-message-wins defect fixed in the live footer was still live in the two surfaces a user actually looks at. Both now fold in every assistant message of the turn via servedAcrossTurn(). 2. The accumulation boundary was derived from parentID, which is not a turn boundary: auto-compaction mints a synthetic user message mid-turn, so a compaction would have discarded every model used before it. Accumulation moves to the footer, where turn.send IS the boundary, and session-data goes back to reporting one message. 3. Conversely, compaction and subtask messages share the turn's parentID while running on a DIFFERENT model, so folding by parentID alone credited their work to the user's turn. Aggregation now skips a compaction summary and anything dispatched to another model. 4. Suppressing by raw id was right, but both formatters then resolved ids back to display names, so two distinct ids sharing a name printed as "GLM-5.3 (GLM-5.3)" - true but useless. A colliding entry now carries its raw id: "GLM-5.3 (GLM-5.3 [glm-5p3])". The turn-model lifecycle is now ONE pure reducer, reduceTurnModel, so both transitions are pinned: "send" REPLACES (a new turn must never inherit the last one's models) and "observe" ACCUMULATES. Routing send through the accumulating path - the exact regression - fails the tests. Claude-Session: https://claude.ai/code/session_017iG7xA9wRvNyFYZSXm8inU
brentyates-swx
added a commit
that referenced
this pull request
Sep 22, 2026
… 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
brentyates-swx
added a commit
that referenced
this pull request
Sep 22, 2026
…22) * ci: add a check that every inherited workflow job is guarded to upstream 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 * ci: guard every inherited workflow to upstream, and refuse cross-repo 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 * ci: make the workflow-guard allowlist per-job and detect stale entries 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 * ci: require the repository guard to be the leading top-level conjunct 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 * ci: pin allowlisted job contents, and test that enforcement is wired 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 * ci: make the guard scanner string-literal aware, and stop test debris 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 * ci: fail closed on an uncheckable condition, and tolerate any && spacing 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 * ci: enforce the audit's own delivery path, and accept a bracketed guard 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 * ci: assert branch coverage, pair envelope pins, and see nested workflows 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 * test: correct the comment overstating one assertion as decisive 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 * ci: stop treating a quoted 'false' as disabled, and attest the audit'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 * ci: read the default branch from GitHub, and pin the actions allowlisted 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 * ci: check that the audit steps RUN, not that they are named 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 * ci: fail when the audit runs without gating, and pin the checks themselves 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 * ci: require the audit's own job to be allowlisted, never guarded 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 * ci: require the audit's own job to be unconditional, not merely unguarded 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 * ci: test the audit's command line, and catch `|| exit 0` 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 * ci: pin the audit steps' scripts exactly, and reject every way its job 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 * ci: opt out of Turborepo telemetry in the two workflows this fork runs 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
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
opencode records the model it requested and discards the one the provider reports back. For a direct model those are the same value, so nothing looks wrong. For a routed model they are not — Fireworks FireRouter and Azure model-router take a route in the request and pick a member per request:
Two consequences, both wrong by construction rather than by degree:
firerouter/...no matter which member replied.costis computed from the singlecostblock configured for the route, so every turn prices at one rate whether the cheap or expensive member served it.Measured on the live route, same prompt, varying only
x-routing-preference:glm-5p3at level 1,glm-5p3-flashat 3-5. Same recordedmodelID, ~3.6x apart on output rate.The value was on the wire and being dropped
session/llm/ai-sdk.tsbuiltLLMEvent.stepFinish({index, reason, usage, providerMetadata})from an AI SDK event that also carriesresponse.modelId.Change
packages/llm/src/schema/events.ts—responseModelIDonStepFinish+Finishprotocols/utils/lifecycle.ts— accepts and forwards to both eventsprotocols/openai-chat.ts— responsemodel(also coversopenai-compatible-chat, a thin wrapper — the FireRouter path)protocols/anthropic-messages.ts—message_start.message.modelprotocols/openai-responses.ts—response.modelprotocols/gemini.ts—modelVersionsession/llm/ai-sdk.ts—response.modelId; the SDK puts it onfinish-stepbut NOTfinish, so adapter state carries it forwardsession/processor.ts— onto the assistant message and the step partschema/v1/session.ts—Assistant.responseModelIDs,StepFinishPart.responseModelIDNo migration:
messageandpartstore a JSONdatablob, so the schema field flows.bedrock-converseis deliberately not wired. ItsmodelIdis request-side only and Bedrock invokes a model directly with no router, so the field correctly stays absent.Two properties, both pinned by tests
Absent means "not reported", never "same as requested". A consumer must distinguish those to know when falling back to
modelIDis legitimate, so every extractor trims and rejects blanks rather than storing""as an identity. This test failed against my own first implementation —openai-chatwas the one extractor I had not trimmed.The message-level field is an ARRAY. A router may pick a different member between steps of one turn; collapsing to one id would silently misreport a mixed turn. Usually length 1, with the exact per-step value on each
StepFinishPart.Verification
End-to-end on a real FireRouter turn through this build:
bun typecheck(30 tasks, what the pre-push hook runs)packages/llmtestspackages/opencodetestsBoth opencode failures are pre-existing or flaky, proven not assumed. I set this work aside as a WIP commit, reverted
packages/toorigin/swxtch, and re-ran:tool.write > sets file permissions when writing sensitive datastill fails on the clean base.snapshot-tool-racefailed in the full run and passed in isolation — which is also why one run reported 1 failure and the next 2. Neither touches LLM events, protocols, or the message schema.Note there are no CI checks on this fork — every workflow except Dependabot is dormant, so the local results above are the only verification this change gets. Being addressed separately.
Not included
bun install --frozen-lockfilefails onswxtch:bun.lockpinsastro@5.7.13whilepackages/web/package.jsonsays7.1.1(drift from02986e2d). A plainbun installfixes it but rewrites 842 lockfile lines, so it is excluded here and filed separately.🤖 Generated with Claude Code
https://claude.ai/code/session_017iG7xA9wRvNyFYZSXm8inU