-
Notifications
You must be signed in to change notification settings - Fork 864
docs(devlog): record the release-safety audit of the main..dev range #2174
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,163 @@ | ||
| # 100 — Release-safety audit of origin/main..origin/dev | ||
|
|
||
| Unit: 260820_bug_pr_backlog_consolidation | ||
| Range: `8e01dd4e8..a584890f8` — 87 commits, 22 merges, 108 changed files. | ||
| Verification host: `ssh lidge:~/ci-wp3/opencodex`. Nothing heavy ran on the workstation. | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win Make the release state and evidence provenance unambiguous. The verdict says “Ship-able after two fixes,” but Lines 154-162 say both fixes were pushed and the full suite passed at a tip carrying both. State whether The document also says each pass has its own artifact, but the RED proofs, SHA-256 comparison, invariant checks, and final suite have no artifact path, command, hash output, or CI run ID. Add those references so reviewers can verify that the results belong to the audited range and the claimed post-fix tip. Also applies to: 7-9, 11-15, 52-52, 89-109, 154-162 🤖 Prompt for AI Agents |
||
|
|
||
| Most of this range landed hours earlier through an admin-override merge run, so it had never | ||
| been read as one body of work. Five passes, each with its own evidence. A pass that says "looks | ||
| fine" without its own artifact is not a pass. | ||
|
|
||
| ## Verdict | ||
|
|
||
| **Ship-able after two fixes**, both of which are in this record with a RED-proven regression. | ||
| Neither was found by per-PR CI, and one of them could not have been: it only exists when two | ||
| independently-correct changes compose. | ||
|
|
||
| ## Pass 1 — security boundary (7 credential-touching PRs) | ||
|
|
||
| | PR | Verdict | | ||
| |---|---| | ||
| | #2137 bearer admission | SAFE — narrows substitution. `admission.source === "bearer"` requires an exact match against `OPENCODEX_API_AUTH_TOKEN` or a configured key, so it is not caller-selectable by an unauthenticated party. | | ||
| | #2147 xAI OAuth 401 replay | SAFE — refresh is pinned to the rejected snapshot's account, one replay (an `if`, not a loop), refreshed token never logged. | | ||
| | #2149 OAuth commit ownership | SAFE — assertion runs under the lock with no `await` before persistence, fails closed, atomic 0600 rename. | | ||
| | #2164 OpenCode Go quota | SAFE — the widened predicate still requires exact URL + adapter + key auth, and the fetch ignores the configured URL entirely in favor of a compile-time constant with `redirect: "error"`. | | ||
| | #2166 shadow marker | **FINDING, FIXED** — see below. | | ||
| | #2146 entitlement discovery | FINDING, recorded not fixed — see "Deliberately left". | | ||
| | #2148 baseUrl override | FINDING, recorded not fixed — see "Deliberately left". | | ||
|
|
||
| ### FIXED — #2166: caller-controlled text reached a durable log | ||
|
|
||
| The shadow-call intercept matches by **prefix**, so a caller can send `gpt-5.6-luna` plus | ||
| arbitrary trailing text and still be intercepted. The whole raw string was recorded as | ||
| `shadowCallRewrittenFrom`, which is persisted to `usage.jsonl` and served from `/api/logs`. | ||
|
Comment on lines
+31
to
+33
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
At this commit, the referenced #2170 fix is not present— AGENTS.md reference: AGENTS.md:L115-L119 Useful? React with 👍 / 👎. |
||
|
|
||
| The sanitizer on that path is not sufficient, for two independent reasons — both run against | ||
| the shipped code, not reasoned about: | ||
|
|
||
| ``` | ||
| "gpt-5.6-luna\nBearer sk-abc123def456ghi789jkl" -> "gpt-5.6-lunaBearer [REDACTED]" | ||
| "gpt-5.6-luna\nAIzaSyA1B2C3D4E5F6G7H8I9J0K1L2M3N4O5P6" -> unchanged, key intact | ||
| ``` | ||
|
|
||
| 1. Control characters are stripped **before** redaction, so the newline that separated marker | ||
| from credential is gone by the time the `Bearer` rule looks for a word boundary. | ||
| 2. The runtime redactor is a deny-list. An `AIza`-shaped Google key has no rule and survives. | ||
|
|
||
| Fix (PR #2170): record the operator-configured prefix that matched, via | ||
| `shadowSourceModelPrefix()`. The field can then only hold a value the operator configured. | ||
| That removes the class instead of adding one more pattern to a deny-list — which matters, | ||
| because the next unrecognized credential family would reopen a pattern-based fix. | ||
|
|
||
| RED proof: reverting the two source files fails exactly the two new tests, 15 pass / 2 fail. | ||
|
|
||
| ## Pass 2 — cross-PR interaction | ||
|
|
||
| The shared-contract map found six files touched by 2+ PRs. Three interactions were reproduced; | ||
| two were already closed by the time of this audit, one was not. | ||
|
|
||
| **Closed already, recorded for the history:** an admission bearer could escape through a | ||
| custom-named canonical transport, because #2137 decided substitution from the provider NAME | ||
| while the adapter recognized the same row by TRANSPORT. Two predicates answering one question. | ||
| Fixed by #2169, verified present at the current `dev` tip. | ||
|
|
||
| **FIXED here — `tool_search_call` got the wrong id namespace.** #2145 restores a lowered | ||
| tool_search as `tool_search_call` with no id; #2142's universal backfill then names it. The | ||
| backfill's prefix table had no entry for the type, so it produced `item_ocx_0`: | ||
|
|
||
| ``` | ||
| {"type":"tool_search_call",...} -> id: "item_ocx_0" | ||
| {"type":"function_call",...} -> id: "fc_ocx_0" | ||
| ``` | ||
|
|
||
| Not cosmetic: `stripInvalidItemIds` deletes any id whose prefix does not match its type, and | ||
| it lists `tsc_` as the only valid prefix for that type. The id survived the turn that created | ||
| it and was silently dropped on the next one, leaving the client an item it could not correlate. | ||
| `custom_tool_call` had the identical gap. Fix: PR #2173, with the superset invariant between | ||
| the two tables written at the table. | ||
|
|
||
| Both PRs' focused suites pass in isolation — neither composes restoration with backfill. This | ||
| is the whole argument for a cross-PR pass existing. | ||
|
Comment on lines
+79
to
+80
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick win Record the composed regression test. The defect appears only when restoration and universal ID backfill run together, but neither focused suite covers that sequence. Add or reference a regression test in the fixing PR that executes restoration, universal backfill, and The downstream contract is implemented in 🤖 Prompt for AI Agents |
||
|
|
||
| **No interaction found**, each with its reason: #2160 static headers × #2148 overrides (neither | ||
| overridden provider has static headers); #2160 × #2164 (opencode-go owns no static headers); | ||
| #2166 log field × #2147 tier writers (independent fields, no last-writer overwrite); #2151 × | ||
| #2165 (request construction vs request-local parseStream state, no shared lifecycle); #2138 × | ||
| #2145 (canonical-forward vs noncanonical, activation sets do not intersect); #2149 × #2147 | ||
| (both account- and generation-guarded). | ||
|
|
||
| ## Pass 3 — default-install wire/behavior regression | ||
|
|
||
| **PASS.** Byte-identical on both revisions for: OpenAI-compatible key provider, direct Anthropic | ||
| key provider, Google AI Studio, plain native Chat, unsupported caller `service_tier`, caller | ||
| reasoning `high`. Verified by SHA-256 over the built request. | ||
|
|
||
| Six differences found, every one attributed to an intending PR: AgentRouter framing (#2162, | ||
| exact-host gated — a non-AgentRouter Anthropic request is byte-identical), `opencode-free` | ||
| static headers (#2160, the only registry row with them), `service_tier: priority` on an exact | ||
| Fast-capable model (#2151), `prompt_cache_retention` removal on canonical GPT-5.6 (#2138), | ||
| tool_search lowering on noncanonical upstreams (#2145), xAI OAuth Responses routing (#2147). | ||
|
|
||
| `shadowCallRewrittenFrom` appears in `usage.jsonl` only when the explicitly-enabled shadow gate | ||
| matches; an ordinary request writes the same key set as before. | ||
|
|
||
| ## Pass 4 — repository invariants | ||
|
|
||
| All green on `ssh lidge`: `tests/core-lab-boundary.test.ts` + `tests/repo-hygiene.test.ts` | ||
| 24 pass / 0 fail; `bun run privacy:scan` passed; `bun x tsc --noEmit` exit 0; no `160000` | ||
| gitlink in the tree; no `src/lab/` file changed in the range and every protected core path | ||
| stayed transitively Lab-free. | ||
|
|
||
| ## Pass 5 — release mechanics | ||
|
|
||
| Version line `2.27.0` on both `main` and `dev` — coherent, and this campaign did not move it. | ||
| No half-finished migration found. No behavior observed that depends on a particular branch | ||
| being checked out. | ||
|
|
||
| ## Deliberately left — not fixed, recorded with the reason | ||
|
|
||
| **#2146 — entitlement discovery uses more accounts than the request needs.** An authenticated | ||
| `/v1/models`, or any account-gated request, enumerates main plus every pool row with a | ||
| syntactically valid id and queries each one's credential concurrently. A request bound to | ||
| account B can therefore cause account A's token to be refreshed, persisted, and sent to ChatGPT | ||
| discovery. Paused and needs-reauth accounts are not excluded. | ||
|
|
||
| Each token stays paired with its own `chatgptAccountId`, so this is not credential | ||
| misbinding, and no token reaches a log or the cache (only model sets and SHA-256 fingerprints). | ||
| It is excessive credential use with a cross-account side effect. Narrowing the candidate set to | ||
| the accounts a request actually needs is a product decision about what `/v1/models` is meant | ||
| to enumerate — that is a NEEDS_HUMAN call, not something to infer from the code. | ||
|
|
||
| **#2148 — an old config silently activates on upgrade.** `allowBaseUrlOverride` is registry-only | ||
| and never persisted, and the router honors any already-saved resolved `baseUrl` the moment the | ||
| flag appears. Reproduced: | ||
|
|
||
| ``` | ||
| routedProviderConfig("anthropic", {baseUrl:"https://relay.example.com/v1"}) -> honored | ||
| routedProviderConfig("google-antigravity", {baseUrl:"https://relay.example.com/v1"}) -> honored | ||
| ``` | ||
|
|
||
| So a custom URL that older releases accepted and ignored starts receiving the OAuth bearer | ||
| after upgrade, with no consent step. The transport gate does hold: public cleartext HTTP is | ||
| refused for these providers, and URL userinfo is rejected at config validation. | ||
|
|
||
| Scope, measured rather than assumed: this range newly opts in exactly **two** providers | ||
| (`anthropic`, `google-antigravity`). Thirteen others already had the flag on `main`, so the | ||
| upgrade-activation shape is pre-existing behavior, not introduced here. Whether it needs a | ||
| persisted consent marker or a release note is a product decision. | ||
|
|
||
| **link-local / unspecified addresses under `allowPrivateNetwork`.** `https://169.254.1.1` and | ||
| `https://0.0.0.0` are permitted when the flag is set, which reads oddly against the policy | ||
| comment. Confirmed **pre-existing**: the classification and the waiver are both unchanged in | ||
| this range. Recorded so it is not rediscovered as new, but it is not this release's regression. | ||
|
|
||
| ## Fixes pushed | ||
|
|
||
| | Finding | PR | RED proof | | ||
| |---|---|---| | ||
| | #2166 caller-controlled marker reaches usage.jsonl and /api/logs | #2170 | 15 pass / 2 fail with the fix reverted | | ||
| | tool_search_call / custom_tool_call id namespace | #2173 | 20 pass / 2 fail with the fix reverted | | ||
|
|
||
| Full suite at the tip carrying both: **13717 pass / 15 skip / 0 fail** across 866 files; | ||
| typecheck exit 0; privacy scan passed. All on `ssh lidge`. | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The parent already contains
100_release_safety_audit.mdin this same unit, but this commit adds a second100audit with conflicting scope and conclusions: the existing record says 92 commits and no second cross-PR interaction, while this one says 87 commits and reports an additional interaction. Readers following the numbered plan now have two apparently authoritative step-100 verdicts, so replace or reconcile the existing record rather than adding a competing file.Useful? React with 👍 / 👎.