[WRONG BRANCH] chore: temporary dev delta extraction for #1206 rebase - #1234
Closed
Wibias wants to merge 56 commits into
Closed
[WRONG BRANCH] chore: temporary dev delta extraction for #1206 rebase#1234Wibias wants to merge 56 commits into
Wibias wants to merge 56 commits into
Conversation
The space after the colon is optional in text/event-stream, so `data:{...}` is
as valid as `data: {...}`. Six parsers hardcoded the spaced form and silently
dropped every frame from a producer that omits it. To the user that looked like
a completed turn with no content.
The same wire format was already accepted on the relay path
(`sse-decoder.ts`, `relay.ts`, `google.ts`) and rejected on the adapter path.
That split is the bug.
Adds two primitives beside the decoder whose rule they mirror:
- `sseFieldValue(line, field)` for the five string-slicing parsers
- `sseFieldOffset(text, start, end, field)` for the live Claude relay, whose
translator-budget accounting reserves bytes by offset — materializing the
line first would allocate the string the budget exists to bound
Call sites fixed: `adapters/openai-chat.ts`, `chat/outbound.ts`,
`claude/outbound.ts` (two independent parsers, both `event` and `data`),
`web-search/parse.ts`, `server/claude-messages.ts`.
Both helpers strip at most one leading space, matching the decoder, so a
payload that legitimately begins with whitespace keeps the rest of it. Neither
trims: callers own that choice and some intentionally keep trailing bytes.
Deferred deliberately, not fixed here: `\r\n\r\n` frame delimiting and
multiline `data` joining without the spec's `\n` separator. Both are
frame-level rather than field-level and carry a different blast radius.
Verification: each new test was confirmed to fail with the fix reverted.
…ll six call sites
Audit found two real gaps in the previous commit.
`sseFieldValue("data", "data")` returned null while `decodeServerSentEvents`
treats a colonless field as an empty value (`colon < 0` -> valueStart =
line.length, sse-decoder.ts:240). Two helpers that mirror the decoder must not
disagree with it. Both now return the empty value, and `sseFieldOffset` returns
the end-of-line offset for the same case.
Regression coverage was also incomplete: `chat/outbound.ts` and
`claude/outbound.ts` had no unspaced test, so two of the six fixed call sites
were unverified. Both now have one, and each was confirmed to fail with its fix
reverted.
Also corrects the remaining stale "5 call sites" claims in the plan unit.
…elds (#1170) The offset-based parser inside responsesSseToAnthropicSse was covered only indirectly. This drives it head-on: the same six Responses frames in spaced and unspaced form, asserting identical event names, identical translated text, and identical budget accounting. The budget assertion compares the two paths rather than asserting zero. This translator leaves a small residue at stream end on the spaced path too (51 bytes for this fixture), so zero would be asserting something that was never true. Equality is the contract the offset arithmetic must satisfy. Confirmed to fail when the offset fix is reverted.
Add model-scoped reasoning-summary capability defaults for the registry-backed DeepSeek V4 and evidence-backed GLM models so Codex keeps sending the selected reasoning object. Merge the registry map per key, with defaults spread first and user configuration second. This preserves an explicit false for one model without suppressing defaults for every other model, and creates a detached object instead of aliasing registry metadata. Cover the catalog contract, partial override behavior, conservative no-opt-in default, and red-green production ablation for #1100.
On a machine where icacls is slow — Defender real-time scanning, a roaming profile, a domain-controller round trip — a complete ACL sequence could not finish inside the 5-second envelope. The harden failed closed, the native-main owner published a permanent `unavailable`, and every native request returned 503 until the user restarted. One correction to the issue's framing: PR #1135's retry is not the problem. Owner-level recovery calls hardenSecret again and does receive a fresh deadline. The defect is that one complete sequence — `/grant:r`, `/inheritance:r`, `/remove:g`, plus the conditional `/findsid` verification — had only five seconds for all of it. Raises the default to 30s and keeps everything else: the 60s cap, the OPENCODEX_ACL_TIMEOUT_MS override, the clamp, and the shared-envelope structure. Independent per-command budgets were rejected: with /findsid fallbacks they multiply into the multi-minute startup stall the shared budget was introduced to prevent. The cost is stated in the source comment rather than hidden. Because loadConfig hardens three paths sequentially, the timeout-path worst case at load becomes ~90s, and the owner path ~60.25s. Both need icacls to be pathologically slow on every call; a healthy machine finishes in milliseconds. A slow start is recoverable, a permanent 503 is not, and the failure stays fail-closed either way. Four existing tests depended on the 5s default while actually asserting something else — envelope sharing, fresh-budget-on-second-call, recovery cardinality. Each now pins OPENCODEX_ACL_TIMEOUT_MS explicitly so it tests its real subject, and beforeEach/afterEach isolate the variable so a stray value in a developer's environment cannot change what any of them assert. The new test deliberately does not pin: it exercises the shipped default with 13s of slow-but-successful work. Confirmed to fail with the default reverted to 5s.
…ff disk (#1100) Audit found the first commit fixed the canonical provider ids but missed the shape the issue was actually reported against. REACHING CUSTOM PROVIDERS. `enrichProviderFromRegistry` matches on the provider NAME. The reporter's row is a hand-added provider literally called "GLM" pointing at a vendor endpoint we recognize. Routing worked, so the row looked healthy, but no registry id is called "GLM" — so every piece of registry metadata was skipped, the effort ladder was advertised with summaries left false, and Codex dropped the inbound reasoning object. Exactly the bug, on the exact configuration that was reported. On the name-lookup miss we now fall back to `registryEntryForProviderDestination`, which already answers "which vendor endpoint is this row talking to" and is restricted to fixed key destinations — no templated or overridable base URL can be claimed by it. Scope is deliberately one field: a custom row keeps its own identity for everything else. PER-KEY, EVERYWHERE. The fallback first bailed whenever the user had any map, which recreated the whole-record bug the per-key merge exists to prevent: one model's flag would have suppressed every sibling default. Both paths now share `applyReasoningSummaryDefaults`, so an explicit value — including `false` — wins for its own key and nothing else. NOT PERSISTED. `enrichProviderFromCatalog` feeds a config about to be written to disk. Writing today's registry defaults there would freeze them as the user's own overrides, so a later correction — learning a model's backend rejects summary delivery — would never reach anyone who created their provider first, and they would keep getting 400s with no way to know why. It now restores exactly what the caller submitted. Catalog gathering enriches a detached runtime clone, so the defaults still apply where they matter. KNOWN REMAINING GAP: the reporter's endpoint, open.bigmodel.cn/api/coding/paas/v4, is in no registry entry — only /api/paas/v4 is. Closing that route needs a new registry entry with its own id (glm/glm-cn are bound in FREE_PROVIDER_DIRECTORY), its own evidence-backed model set, and registry-parity updates. That is a provider addition, not a bug fix, so it stays out of this stack and is recorded in the plan unit instead. Each new test was confirmed to fail with its production change reverted.
…el coding endpoint The first implementation passed its tests and was still wrong about the reported configuration: enrichment matches on provider NAME, and the reporter's row is a hand-added provider called "GLM". Recording the failure mode because it generalizes — canonical-id tests were green against a configuration no user had. Also records why the BigModel coding endpoint is deferred rather than fixed here, with the safety analysis for adding it later.
…was actually reported on The destination fallback added here matched two GLM routes: Z.AI's coding plan and BigModel's pay-as-you-go `/api/paas/v4`. The configuration in the issue is neither. It is `https://open.bigmodel.cn/api/coding/paas/v4` — a third endpoint with no registry row — so the lookup found nothing, `modelSupportsReasoningSummaries` stayed unset, and Codex kept dropping the reasoning object. Effort still displayed as `-`. The test hid this. It was captioned "the reporter's actual shape" and used provider name "GLM" and model glm-5.2, both correct, but substituted Z.AI's host. It passed against a route that already worked while the reported one stayed broken. Found in pre-merge review, not by the suite. A prefix match on `open.bigmodel.cn` would have covered both endpoints in one line. It is also how a config pointed at one vendor route inherits another route's metadata, which is the failure the exact-endpoint rule exists to prevent — so this is a separate row. Two details that are not arbitrary. The id is not `glm-cn`, which the free-provider directory already binds to this same path; registering it here would let routedProviderConfig() canonicalize a saved `glm-cn` config onto our baseUrl. And the model list follows Z.AI's coding-plan set rather than the pay-as-you-go one, because this endpoint is the subscription product and glm-5.2 only exists on that side. Ablation: pointing the new row's baseUrl elsewhere makes the reproduction test red. Refs #1100
ocx serves gui/dist, which is generated and gitignored. A fast-forward advances gui/src while dist stays at whatever was last built, so the dashboard keeps rendering the OLD bundle and nothing in git status hints at why. That cost a real debugging session today: the symlink and the source were both correct while the served bundle was seven hours stale, still drawing sidebar rows the merge had deleted. post-merge runs build:gui only when the merge range touched gui/, using the same slash-guarded match as lint-gui-if-changed so all three agree on what counts. No usable ORIG_HEAD means skip rather than rebuild — a hook that taxes every unrelated pull gets disabled, and a stale dist is one command to fix. A failed build never fails the merge; it prints the command to run. setup-hooks now installs each hook independently. The old single-hook early return would have stopped post-merge from installing whenever pre-push was already current.
The #875 bounded-JSON force (modelResponsesUpstreamStreaming) delayed every byte of a deepseek-v4-flash turn until generation finished — 28-46 s of silence on long turns, which users read as a hang. The official Responses guide documents a response.completed/incomplete/failed terminal with no data: [DONE] sentinel, live probes (including the tool-result replay shape behind the original stall) close on the terminal, and the relay's terminal-output boundary already cuts the stream there and synthesizes [DONE]. Drop the deepseek registry opt-in; keep the mechanism suite-reachable through a synthetic-registry fixture, add streamed #938 id-repair integration coverage, and record the supersession in the #1065 RCA. Unit: devlog/_plan/260807_deepseek_responses_streaming/
#938) Live streaming re-exposed a leak the bounded-JSON era masked: DeepSeek wraps streamed reasoning text in content parts, and content_part.* events were mapped to the message id table only, so their item_id kept the raw upstream UUID while the parent reasoning item was repaired. A live tool-call probe showed 13 leaked UUID item_ids per turn. rewriteItemIdField now falls back to the sibling table — an output_index identifies exactly one item — and the streamed #938 regression test pins a content_part frame riding a reasoning item. Live re-probe: 0 UUID leaks; function_call ids remain untouched.
…g the product code The serialization test excluded two pre-approval race outcomes and treated everything else as a seam failure. A macOS CI run produced a third: `SQLiteError: database is locked` on stderr, which failed the build. That contradicts the runtime. `configGenerationFailureReason` classifies that exact message as "busy" rather than a database fault, and the storage and history paths do the same. So the product already treats it as ordinary contention while this test called it a defect. Found by a red CI run rather than by the suite, which is the part worth noting: the test enumerated the races it had seen instead of the races the code recognizes, so a third one was a matter of timing rather than of whether it could happen. The two-process seam itself is unchanged and still fails on a genuine convergence error.
…replacement, round 2)
Audit found four defects in the first cut, two of which would have shipped a
feature worse than the bug it fixes.
BUDGET EXHAUSTION IS NOT FAILURE. `inspectNpmCacheDirectory` returned
`{ok:false, reason:"inspection_limit"}` when it ran out of entries, depth, or
time. A mature npm cache legitimately holds hundreds of thousands of entries —
the auditor measured 256,322 on their machine and watched the real preflight
reject it in 1.13s. Every one of those users would have been locked out of
updating. "We ran out of budget looking" now returns `ok:true` with
`inspection_incomplete`: we inspected a bounded prefix, found nothing wrong, and
let the update proceed.
NESTED SYMLINKS ARE SKIPPED BEFORE THE OWNERSHIP CHECK. npm creates symlinks
constantly below `_npx`, `node_modules`, and `.bin`. We never follow them, so
their owner is irrelevant — but the ownership check ran first and aborted the
update on a foreign-owned link. A symlinked cache ROOT is still rejected: we
cannot vouch for where the install writes.
SANITIZATION SURVIVES WRAPPED PATHS. npm and the OS wrap long paths, and the
line-bound regexes let `C:\Users\<newline>Jane Doe\...` through with the
username intact. Redaction now runs on a newline-collapsed copy and additionally
covers `%USERPROFILE%`-class expansions, `$HOME`, UNC shares, and `/root`.
GATE ORDERING IS TESTED BY BEHAVIOR. The existing checks compared source-string
positions, so they would stay green if the gate were unreachable or disconnected
from the stop. `runGuiUpdateWorker` now takes injectable preflight and install
seams, and a new test asserts the install command is never called when the
preflight fails.
The symlink test also needed a real seam: `!stat.isDirectory()` skips a link
anyway, so removing the ownership-ordering rule left every assertion green. An
injected `uidOf` binds the assertion to ownership specifically. Each of the four
fixes was confirmed to fail its test when reverted.
…t wrapped paths (round 3)
Audit round 2 found the previous commit's headline fix was inert.
THE PROTOCOL REJECTED ITS OWN SUCCESS. `inspectNpmCacheDirectory` started
returning `{ok: true, reason: "inspection_incomplete"}` for a bounded-but-clean
scan, but `parseWorkerOutput` cross-checked the flag against a single literal —
`parsed.ok !== (parsed.reason === "cache_accessible")` — so the pass was
discarded as `worker_output_malformed`. Every large cache still failed, now with
a misleading reason. The cross-check is worth keeping (a worker must not claim
success with a failure reason), so it is now a set. Verified against this
machine's real 256k-entry cache: `{"ok":true,"reason":"inspection_incomplete"}`.
`inspection_limit` became unreachable and is removed.
WRAPPED PATHS STILL LEAKED. Rejoining wrapped lines was the wrong shape: joining
aggressively enough to catch a wrap inside the username also merged genuinely
separate log entries, and joining conservatively enough to keep them apart let
`C:\Us<newline>ers\Zoe [Admin]+` through. Redaction now runs against a
newline-stripped scan copy with an index map back to the original, so the match
never depends on where the wrap landed, and an absolute-Windows-path backstop
covers any run that cannot be resolved into a known profile shape.
THE GATE TEST NEVER REACHED THE GATE. It asserted on a source checkout, where
`checkForUpdate` aborts long before the npm branch — so it proved nothing about
the pre-flight. `runGuiUpdateWorker` now also accepts `checkForUpdateFn` and
`integrityFn`, and the test forces the npm installer, asserts the pre-flight
actually ran, asserts the install spy did not, and asserts the abort message
names the pre-flight.
Two source-position tests were updated to match the new seam strings. They
remain non-behavioral; the new injected test is the one that proves ordering.
Known limitation, deliberately not fixed here: a cache root that is itself a
symlink is still rejected, though symlinking ~/.npm to another volume is
legitimate. Resolving the root target safely is a separate change.
…linked cache root (round 4) INDENTED CONTINUATIONS STILL LEAKED. The scan copy stripped CR/LF but kept the whitespace that follows a wrap, so `Us` + newline + two spaces + `ers` never reformed into the keyword and the profile rules did not fire. Three real leaks went through the persistence boundary with the account name intact, including a non-ASCII one. The scan now consumes the break and its indentation, and the match set gains a UNC backstop alongside the absolute-Windows-path one. Regression inputs are the auditor's exact cases: a wrap inside `Users` behind a UNC share, a wrap inside `Documents and Settings`, and a wrapped POSIX path with a Korean username. A SYMLINKED CACHE ROOT IS NO LONGER REJECTED. Pointing ~/.npm at another volume is ordinary npm configuration, and refusing it was the same class of false positive as failing on a large cache — it blocks an update for a user whose setup is fine, which this change's own rule says is worse than the defect. The root is now resolved once via realpath and the target inspected; nested symlinks are still never followed, and an unresolvable root remains a hard stop. Both fixes confirmed to fail their tests when reverted.
… it (round 5) The scan copy removed every line boundary, so the `[^\r\n]*` backstops ran to the end of the text: one redacted path consumed every following log entry. Privacy was intact; the diagnostics were destroyed. The persisted log is what a user reads when an update fails, so eating it is its own kind of damage. Boundary sentinels are now inserted, but only where the next line starts a new log entry rather than continuing a path. Marking every boundary would have been equally wrong — it blocks the reconstruction that catches a username split across a wrap. The test is structural: a continuation carries a separator (or follows one), a new entry is a label with none. That distinction is what lets `Mary Jane van der Berg\Documents\...` still reconstruct while `KEEP diagnostic code E42` survives untouched. Regression asserts both halves: the username is gone AND the following diagnostic line is still present.
… continuations (round 6) The separator heuristic could not work, and the audit proved it with two inputs that fail in opposite directions: C:\Users\Z / " oë [Admin]+" continuation with NO separator -> leaked ...\Users\Jane\x / "npm ERR! /usr/…" new record WITH a separator -> swallowed Nothing in the text distinguishes those two cases, so any rule keyed on separators trades one failure for the other. Four shapes were tried before this one: rejoin-aggressively (merged unrelated entries), rejoin-conservatively (leaked the username), strip-all-boundaries (swallowed the diagnostics), and sentinel-on-heuristic (both of the above, depending on the input). The redaction is now line-aware with one carry bit. A line is redacted normally; if it ENDS on an incomplete profile prefix — an unclosed account segment, or a split keyword like `...\Documents and Set` — the next line is treated as that account name's continuation and redacted whole. This is deliberately asymmetric. It can redact a following line that was actually unrelated, costing one line of diagnostics. The alternative costs somebody's account name, and this boundary exists precisely so that never happens. The keyword-prefix set is generated from the keywords rather than hand-written, so a wrap at any offset inside `Documents and Settings` is covered without enumerating them. Regressions now assert both directions: the username is gone, and an unrelated following record — with a separator in it — survives.
Six rounds of redaction, six new leaks. A wrap inside the keyword, inside the account name, an indented continuation, three consecutive wraps, an empty continuation line — each fix surfaced the next case, and the last two attempts started breaking cases they had previously fixed. That is not a tuning problem. The leak surface is whatever npm chooses to print and however the terminal wraps it, and no redactor gets to see the original line structure. Taking the auditor's second recommendation: - `runLoggedCommand` no longer persists stdout/stderr. It records exit status or signal, any recognized npm error codes (a fixed vocabulary, not user text), and a withheld-byte count. Detailed output stays ephemeral. - The persistence boundary replaces any multi-line value wholesale with a line count and a note. Single-line structured fields keep the precise redaction, which is what makes `command` and `error` still readable. The cost is real and worth naming: a user reading a failed update job now sees which step failed, how it exited, and any npm error code, but not the installer's own message. That is a genuine diagnostic loss. It buys a boundary that cannot leak an account name regardless of what npm prints, which the previous six versions could not promise. The auditor's five leaking inputs are kept as regressions. They now pass structurally rather than by pattern-matching.
…ingle-line path leaks (round 8)
Three more findings, all real.
THE "FIXED VOCABULARY" WAS A SHAPE PATTERN. `E[A-Z]{3,}` matches `ERROR`, so
`npm ERR! path C:\Users\ERROR\.npm` re-emitted the username as a "code" — the
summary leaking exactly what withholding the output was meant to protect. It is
now an explicit Set of recognized npm/libc codes, extracted only from npm's
canonical `code <CODE>` position rather than scanned out of free text.
SINGLE-LINE PATHS STILL LEAKED. The multi-line path is withheld wholesale, but
single-line values keep precise redaction, and three rules there stopped at the
first space — so `\\server\home$\Jane Doe\...` and `D:\Profiles\Mary Jane\...`
kept the surname. Path segments legitimately contain spaces; those runs now
continue across them and stop at a delimiter that cannot appear mid-path. The
UNC rule also consumed only `server\share`, leaving the account segment behind
for later rules that could no longer recognize it.
BYTE COUNT WAS A CODE-UNIT COUNT. `Buffer.byteLength(..., "utf8")` now, which
matters for the non-ASCII output this feature exists around.
… does not (round 9) Eight rounds of redaction failed in both directions at once, and the audit proved it with one input each: D:\Profiles\Mary O'Connor\... leaked — an apostrophe was a terminator installed at C:\x and then ... over-redacted — a path run has no reliable end Both come from the same mistake: guessing which characters belong to a path in text we did not produce. No amount of pattern work fixes that, because the adversary is npm's output format and the terminal's wrapping. The boundary now asks a question it can answer — is this value KNOWN safe? — and withholds everything else. Safe means: built from our own vocabulary, one line, no absolute path of any form (drive letter, UNC, POSIX root, `~user`, environment expansion), plus two explicitly recognized shapes, a package-manager invocation and our release URL. Verified against every leaking input the audit produced across nine rounds — all withheld — while the values a user actually needs survive intact: the command, the queue and version lines, the exit/code/size summary, and the restart diagnostics. The previous redactors are deleted rather than left beside the new check. Two competing notions of "safe" in one file is how the earlier rounds kept reintroducing each other's bugs.
…held error text (round 10) The previous "allow-list" defaulted to `return true`, which makes it a denylist wearing an allowlist's name — and the audit walked straight through the exception carved out for our own endpoint: `probe /healthz?path=/Users/Jane-Doe` passed. Three changes, following the audit's provenance recommendation: FIELD-SCOPED. Only `command`, `error`, `log`, and `releaseNotesUrl` go through the check. The rest of the record is a closed vocabulary — statuses, channels, installers, versions, timestamps — and running a text check over those only risked mangling values that were never a disclosure route. RENDERED, NOT FILTERED. `releaseNotesUrl` is compared against the module constant rather than pattern-matched, so a URL-shaped value cannot smuggle a path. `command` is rebuilt from a recognized shape: a known tool, fixed subcommands and flags, our own package spec, and `<path>` placeholders for absolute arguments. Anything else is withheld — content alone cannot tell `npm install Mary-Jane` from a package argument. ERROR TEXT IS DESCRIBED, NOT COPIED. Every site that interpolated an `Error.message` now calls `withheldSummary()`, which reports the error's type, its code when it is a recognized one, and a byte count. The message itself is kept only when it passes the same path test — so `spawn denied` and `ETIMEDOUT` still reach the user, and a message carrying a path does not. Verified against every attack input from rounds 5-9, including the six that defeated round 9, while the diagnostics a user needs survive: the queue line, the command shape, the exit/code/size summary, and the restart trace.
… provenance code (round 11) The audit caught something worse than a bug: I built a provenance mechanism and never wired it up. `ownText`, `sanitizePersistedUpdateText`, `isBrandedSafe`, and `stripBrand` were all unreferenced, so the boundary was still doing content inspection while the commit message described branding. Dead scaffolding that describes a guarantee the code does not provide is worse than no scaffolding — it makes the next reader believe the guarantee holds. All of it is deleted. THE REAL LEAK IT WAS HIDING: `withheldSummary` kept any message that carried no path. That sounds reasonable and is wrong — `spawn denied for Jane Doe` has no path in it and still names a person. There is no test on message CONTENT that separates a diagnostic from an identity, so message text no longer crosses the boundary at all. The record keeps the error's type, a recognized code, and a byte count. Also closed: - A raw `err.message` catch in the GUI worker that never went through any check. - `error.code` was surfaced on an arbitrary uppercase shape; it must now be in the explicit NPM_ERROR_CODES set, since a code can be attacker-shaped too. - The npm code extractor is anchored to a complete canonical line (`^npm ERR! code <CODE>$`, multiline) rather than matching `code` anywhere in free text — closing `npm ERR! path code EACCES\private`. Cost, stated plainly: a user no longer sees npm's own error text. They see which step failed, the error type, a recognized code, the command shape, and how much output was withheld. Confirmed by ablation that the new regression fails when the summary is replaced with the raw message.
…on at entry (round 12)
Two more channels, both external text reaching disk through a field that looked
like ours.
`Error.name` IS EXTERNAL. It is writable, so `error.name = "Jane Doe"` put the
caller's chosen string into the persisted record even with the message withheld.
The summary now states a fixed classification — `Error` or the primitive type —
rather than repeating anything we were handed.
`/healthz` VERSION IS EXTERNAL. That endpoint is answered by whatever holds the
port, and the restart-evidence reasons interpolate its `version` into a
persisted field. A responder returning `{version: "Jane Doe"}` persisted it. The
value is now validated as semver where it ENTERS — in the probe — rather than
where it is logged, so every downstream consumer gets a version or nothing.
Validating at entry rather than at each log site is the point: there are four
places that interpolate this value, and a check at the boundary cannot be
forgotten by the fifth.
The design changed twice under audit. The first proposal was an opt-in switch that would have admitted loopback socket peers on a remote bind; it rode resolveApiAuth into eight endpoints unrelated to #1102, and a public listener's peer address only proves the last transport hop, which Docker Desktop, host-network containers, WSL mirrored networking and tunnels all terminate locally. The shipped design leaves public admission untouched and opens a separate 127.0.0.1-bound listener, so the kernel refuses remote connections instead of us judging addresses. Later rounds closed the implementation contracts: a fixed port (an ephemeral one would manufacture the restart breakage the issue claimed and we disproved), a per-request auth/CORS policy view narrow enough that it cannot masquerade as business config, one startup transaction across both binds, composite stop that completes cleanup AND propagates failure, and GET /v1/models on the allowlist because Codex falls back to it when no catalog is installed. The last blocker was the sharpest: the /v1/models ablation would not have gone red, because the models-manager catches refresh failures and returns its bundled list. The acceptance test now turns on a runtime-generated unique routed model that no bundled catalog can synthesize.
…ound 13) Shape validation was not enough: `2.7.41-JaneDoe` is valid semver, so the mismatch reason echoed it straight into a persisted field. `/healthz` is answered by whatever holds the port, which makes its version external input no matter how well-formed it looks. Mismatch reasons now state THAT the reported version did not match and name only the version we expected — which is ours. On a match the reported value equals the expectation by definition, so the trusted one is rendered instead. This closes the last channel the audit's persistence inventory found. Regression drives the hostile value from ingress through to the evidence reason and asserts the name is absent while our own version still appears.
…pawn Codex (#1102) A `codex app-server` launched by a host that resolves the entrypoint directly never passes through the generated shim, so it never inherits OPENCODEX_API_AUTH_TOKEN. On a wildcard bind every model call then 401s at admission, before any SSE frame. The tempting fix — exempt callers whose socket peer looks like loopback — is unsound. `requestIP()` proves only the last transport hop, and Docker Desktop port forwarding, host-network containers, WSL mirrored networking and tunnel terminators all re-open remote connections locally. It would also have ridden `resolveApiAuth` into eight endpoints unrelated to this issue. Instead a second listener binds 127.0.0.1. The kernel refuses remote connections outright, so there is no address to judge, and the public listener's admission policy is byte-for-byte unchanged. The parts that are load-bearing: Auth and CORS read a `RequestPolicyView` — a Pick of hostname, corsAllowOrigins and apiKeys — chosen per request from the receiving listener. Rewriting the whole config and holding it would go stale on the next management change; adding an `allowUnauthenticated` parameter to the resolvers would put an admission bypass on the wrong side of the boundary. The narrow type also means a policy view that leaks into a routing path fails to typecheck. The bind is loopback but the boundary is not the bind alone: an attacker page can make a victim's browser connect to 127.0.0.1. The listener therefore takes the same Host/Origin branch a plain loopback bind always has. A test asserts the same hostile Origin that the loopback policy rejects would be accepted under the public policy. The port is required in config, never OS-assigned. An ephemeral port would change across restarts while running app-servers kept the old base_url — the exact symptom this issue reported for token rotation, which does not actually happen. `GET /v1/models` is on the four-route allowlist because when catalog materialization fails, `syncCodex` injects with `catalogPath: null` and Codex falls back to an online model manager that refreshes through it. Both binds are one startup transaction, and composite stop completes cleanup on both listeners while still propagating failure — swallowing it would let drainAndShutdown report success while a socket is held. Off by default. When on, every local process can spend account quota and paid provider credentials, which the startup warning and the docs say plainly. Ablation: reverting the policy view to the shared config makes 3 tests red including the hostile-Host case; dropping the auth-header rule makes 1 red; removing the port validator makes 2 red; rewriting the public bind to a literal 127.0.0.1 makes the F4 symmetry guard red. Refs #1102
… 14) Withholding the whole stream was too blunt. A user whose update fails deserves to know why, and `exit 1 · 359 bytes withheld` tells them nothing. The insight I missed for thirteen rounds: npm's failure output is STRUCTURED, not prose. It prints `npm error <field> <value>`, one field per line. That means the useful parts can be read BY NAME instead of reconstructed from text — which is what made every redaction attempt fail, since it had to guess where a path started and ended. Kept fields, each because its value cannot be a local path: `code`, `syscall`, `errno`, `notarget`, and the HTTP-status lines (`404`, `401`, `403`, `409`, `429`) whose value is a registry URL. Explicitly not kept: `path`, `dest`, `file`, `stack`, the bare `Error:` line, and the debug-log location — every one of those is a filesystem path by definition. Each kept value still passes the path test before use, is length-capped, and `code` must additionally be in the recognized vocabulary. Convention is not a guarantee. Node exceptions get the same treatment: `syscall` and `errno` are named properties, shape-validated (a short lowercase identifier, an integer), so an error summary now reads `Error EACCES · syscall: mkdir · errno: -13` instead of a byte count. Measured against real npm failures: before exit 1 · 366 bytes withheld after exit 1 · code: E404 · 404: The requested resource '…' could not be found before exit 1 · 359 bytes withheld after exit 1 · code: EACCES · syscall: mkdir · errno: -13 before exit 1 · 208 bytes withheld after exit 1 · code: ETARGET · notarget: No matching version found for left-pad@99.99.99 The regression drives a real EACCES dump containing `/Users/Jane Doe/...` and asserts the cause survives while the account name and paths do not. Ablation confirmed.
Allowlisting the field name and leaving its value free-form just moved the leak
one level in. `npm error syscall janedoe` was kept verbatim, because `syscall`
was a recognized field and nothing ever checked what followed it.
Every kept field is now RENDERED from a validated value:
code must be in the recognized npm/libc vocabulary
syscall must be in an explicit POSIX syscall set — a shape check accepts `janedoe`
errno must parse as an integer, and is re-rendered from the parsed number
notarget reduced to `no matching version for <pkg>@<version>`, or the bare fact;
the surrounding prose is never borrowed
HTTP 4xx parsed as a URL, rendering only the registry HOST — the path can name a
private scope and userinfo is a credential
Node exceptions use the same syscall vocabulary rather than the shape check.
This also fixes a bug the audit found in the previous round: the HTTP diagnostic
never actually worked, because the raw line contains `https:/` and the path test
read that as a drive letter. Parsing the URL fixes the false positive and the
disclosure risk in one move.
Verified against every forged input the audit produced — `syscall janedoe`,
`errno JaneDoe`, `notarget ... Jane Doe`, mixed-case `NpM ErRoR`, a private
scope in the URL path, and userinfo — while the real failures stay legible:
exit 1 · code: EACCES · syscall: mkdir · errno: -13
exit 1 · code: E404 · 404: HTTP 404 from registry.npmjs.org
exit 1 · code: ETARGET · notarget: no matching version for left-pad@99.99.99
Ablation confirmed: restoring the shape check fails the new test.
…ch the real inject wiring Two of the previous tests were watching nothing, and the audit was right about both. The allowlist check probed POST routes with GET. Chat Completions, Messages, Images, search and Live all 404 on method mismatch inside their handlers, so widening the allowlist to admit one of them would have kept the assertion green. Each route is now requested with the method its handler accepts, plus two cases proving the allowlisted paths still reject methods they do not serve. Ablation: adding /v1/messages to the allowlist now fails it. The injection test handed the loopback port straight to buildProviderTableBlock, so deleting the substitution inside injectCodexConfig changed nothing it observed. It now runs the real injector in a subprocess — CODEX_CONFIG_PATH resolves at module load, so an in-process CODEX_HOME would write somewhere else — passes the PUBLIC port the way every caller does, and reads the written config.toml. Ablation: removing the substitution fails it. Also added: POST /v1/responses and /v1/responses/compact admitted on the loopback listener and 401 on the public one, asserted as neither 401 nor 404 because "not 401" alone would survive removing the route; a Responses WebSocket handshake on both listeners; and a rollback test that binds a fixed public port and rebinds it after the throw, since throwing while leaving the public listener up is the failure the rollback exists to prevent. Two honest gaps are recorded rather than papered over. The WebSocket test cannot defend requestServer.upgrade over server.upgrade: that ablation stayed green because this Bun version accepts an upgrade issued from a sibling Bun.serve. And composite stop's failure propagation has no test, because the composite captures the underlying stop at construction and there is no seam to inject a rejection through; its sibling property, cleanup across both listeners, is covered. Port selection fixes from the same review: the ephemeral redraw is now a bounded loop behind an injectable allocator rather than unbounded async recursion, and it has tests that actually reach the redraw branch. An explicit --port collision with the reserved port is rejected before the 60-second reclaim path instead of after it, with a message naming the real cause. runAdmittedHttpTurn now takes the policy explicitly, so no CORS-emitting helper falls back to the shared config. Refs #1102
…(round 16) Two things I treated as "safe shapes" that were not. A PACKAGE SPEC IS NOT A SAFE SHAPE. `name@version` also matches `jane.doe@example.com` and `JaneDoe@2.7.41`, so extracting "the spec" from a `notarget` line was itself a disclosure channel. This updater resolves exactly one package, so a spec is echoed only when it IS ours; everything else reports the bare fact. A HOSTNAME IS NOT A SAFE SHAPE. `^[\w.-]+$` accepts `janedoe.example`, a numeric host, and a punycode host. Knowing whether a 404 came from the public registry or somewhere else is the useful part, and that fits in an allowlist — four known registry hosts. Anything else reports the status alone. Also fixes the spec matcher itself: `PKG` is scoped (`@bitkyc08/opencodex`), so it needed escaping and a boundary that works with a leading `@` — `\b` does not. Verified our own spec is kept while an email is not. Every attack input from this round is a regression test.
Adopted from PR #1159 by @jonathanli12, rebuilt on the current stack. Original closed in favor of this commit. Discovery normalizes Cursor's optional `cursor-` prefix so catalog matching can compare canonical ids. Requests then inherited that prefix-free form, so regular Grok 4.5 went out as `grok-4.5-{tier}` when Cursor's live discovery advertises `cursor-grok-4.5-{tier}`. The fix keeps the two paths separate rather than changing normalization: `cursorRequestWireModelIdWithEffort` composes the request-side id and leaves `cursorWireModelIdWithEffort` alone for discovery. Touching normalization would have fixed the request and broken catalog matching in the same edit. Grok Fast is untouched: it keeps the canonical `grok-4.5` model id with `effort` and `fast=true` as separate parameters. Scope note: only regular Grok 4.5 gets the prefix. This does not change Claude-family ids, and it does not address #1162 (Cursor Claude-family resource_exhausted), which has no code-level cause identified yet. Both tests confirmed to fail with the request-builder change reverted.
Xiaomi MiMo's paid endpoint answers the Responses wire for plain turns, so a
user configuring it by hand picks `openai-responses` — MiMo documents Responses
support. But its gateway rejects `type: "custom"` tools with
`400 responses_feature_not_supported`, and `apply_patch` is a custom tool. The
result is a provider where chat works and every agentic turn fails.
Only `xiaomi` (Anthropic wire) and `mimo-free` (free tier, own adapter) existed,
so token-plan users had no preset to start from. This adds one pinned to
`openai-chat`, which is the wire the reporter confirmed works end to end. The
Chat path already lowers custom tools to `{input: string}` functions and
restores them as `custom_tool_call`, so the capability survives intact.
Stripping the custom tools instead would stop the 400 and disable the Codex
agent loop — a provider that no longer errors and no longer edits files.
Reasoning tiers above `high` are clamped: the gateway validates the ladder
strictly and rejects anything higher.
`preserveCustomDestination` is set because a user may already have a hand-rolled
provider under this id. Without it, routing canonicalizes their base URL onto
ours and sends their key to a host they never chose — the hazard the
`zhipu-bigmodel` comment documents.
That last property is the one worth testing rather than asserting: the preset
shape assertions do not exercise it at all, since the mechanism only engages
when endpoint, adapter, or auth differ. A routing-level regression covers it,
and both it and the adapter pin were confirmed to fail when reverted.
Resolve the current token SID instead of trusting USERDOMAIN and USERNAME on workgroup hosts. Keep identity lookup failures separate from icacls timeouts.\n\nRefs #1149
…two blind seams The shutdown orchestration held two properties that pull against each other — keep cleaning up after a failure, yet still report it — and neither was testable in place, because the composite captures the underlying stop at construction. Extracted to `runListenerShutdown`, which now has four cases including both failure shapes. Ablation: removing the per-step catch fails 2, swallowing the collected failures fails 3. Two seams have no runtime oracle on this Bun version, and both would regress silently. Swapping `requestServer.upgrade` for `server.upgrade` stays green because this Bun accepts an upgrade issued from a sibling Bun.serve; another version is not promised to, and the loopback listener would then fail to upgrade at all. And the connection-refused test degrades to a warning on a host with no external IPv4, so the 0.0.0.0 ablation would pass there. Source assertions are a weak instrument, but one aimed at a known blind spot beats a comment nobody runs — and the upgrade assertion does go red on that swap. Smaller review fixes: the WebSocket handshake helper now clears its timer and settles once, so a late timeout cannot fire into the next test; and the rollback test draws its public port with the loopback port reserved, since two back-to-back freePort() calls can return the same port and the test would squat itself. Refs #1102
…reverting concurrent changes Adopts #1203 by @estelledc — the first three commits are theirs, cherry-picked with authorship intact. The approach was right: expose the existing `providers.<id>.contextWindow` / `modelContextWindows` contract at the management and UI layers without touching catalog derivation, which already materializes those values when upstream metadata is absent. Four corrections, all found by independent audit. **Only the selected model was saved.** The drafts map held edits for every model but the PATCH was keyed on `contextModelId`, so a value typed into model A and then abandoned by switching to B vanished — no error, no warning. The PR's own test pinned that as correct. It now sends every model the user typed into. **But "every model that differs" would have been wrong the other way.** The 10s poll can refresh a field while the modal is open; diffing drafts against live state would then call an untouched field dirty and revert someone else's change. Two conditions are required: the user touched it, AND the value differs from what the modal opened with. Both apply to the provider default too, which was previously sent unconditionally and would stamp a stale number over a concurrent update. The snapshot holds canonical numbers, not the raw text. Retyping 64000 as "64,000" is not an edit, and treating it as one would resurrect the same stale-write. When nothing survives the comparison, no PATCH is sent at all and the feedback says so rather than claiming an update. **`Number.isInteger(1e100)` is true.** Both the management validator and the form accepted it; it would persist and serialize into the catalog as an enormous number that can make Codex reject the file. Both now require a safe integer. The default is only validated when touched, so a value inherited from a hand-edited config cannot block an unrelated per-model save. **An override for a model that left live discovery was unreachable.** It sat in the drafts map, absent from the picker, impossible to inspect or clear. Tests: the exact #1073 reproduction is split in two, because a single case setting `modelContextWindows` keeps passing with the provider-wide fallback deleted. Ablations were driven red in their real defect shape rather than as artificially strong mutants — notably, comparing against live `groups` while keeping the touched guard is only visible when a field is edited, reverted, and changed server-side, which the suite now covers. Translated provider docs (ko/ja/ru/zh-cn) described both fields as caps only, which reads as the opposite of the fix for non-English users. Co-authored-by: zhouxun <zhouxun.13@bytedance.com> Closes #1073
Pinning the package NAME to our own still left the VERSION free: `@bitkyc08/opencodex@99.99.99-JaneDoe` is a valid-looking spec, and a semver prerelease identifier can encode anything. That is the same lesson the `/healthz` version taught in round 13 — I applied it there and not here. There is no trusted resolved version available at this call site, so the spec is not rendered at all. `code: ETARGET · notarget: no matching version` already tells a user their requested version does not exist, which is the diagnostic that matters. Both attack inputs are regression tests.
Adopted from PR #1171 by @byongshintv, rebuilt on the current stack. Original closed in favor of this commit. An unlimited A6API key reports zero finite credit totals. Finite-total validation then treated that as a terminal failure and returned before the key could be represented at all, so a perfectly working key looked dead in the dashboard. The unlimited branch now runs ahead of that validation and emits the generic `customWindows` row the GUI and CLI already consume, preserving expiry. It accepts `true`, `1`, and `"true"` for the upstream flag. Two known limitations, stated rather than discovered later: `creditsUsd` and its expiry are not yet surfaced by the GUI — visibility comes from `customWindows` — and neither changes existing behavior for finite keys. Confirmed to fail with the unlimited branch disabled.
… plan Audit found resolveTrustedWindowsPowerShellExe() already resolves and validates the executable through GetSystemDirectoryW. Writing a third SystemRoot/PATH lookup would reintroduce the substitution surface the plan exists to close.
…dable Builds on the contributor fix by luvs01 (#1180), which replaced the USERDOMAIN\USERNAME ACL principal with the effective token SID. Three things that fix left open: The synthetic principal POSIX CI needs lived in windows-secret-acl.ts and was chosen before the injected runner. That ordering made a lookup FAILURE unreachable outside Windows, so the two cases that defend the fail-closed boundary and the timedOutPaths isolation were guarded with `if (process.platform !== "win32") return;` and never ran on Linux or macOS. A test that silently returns on two of three CI platforms is not coverage of a security boundary. The synthetic value moves to the resolver as its own seam, runner selection becomes explicit > synthetic > default, and both guards are gone. sanitizedAclError re-attaches only allow-listed codes, and EACLIDENTITY was not among them. A required-mode harden therefore threw with the cause in the message but `error.code === undefined`, so no caller could branch on "the SID could not be resolved" versus "icacls stalled". The existing test matched the message and hid this. The absence of a name-shaped fallback is now stated as the fix rather than left as an omission. `DOMAIN\User` has a valid shape, but shape is not evidence of the token's subject, and both variables are writable by whatever launched us. runIcacls grants the principal Full Control and then removes inheritance, so a wrong principal either leaves another account holding the secret or strands the file with no usable ACE. An independent audit rejected an earlier draft of this change that restored that fallback for the optional read path. Coverage now runs the sync and async paths across required and optional on every platform, and asserts zero icacls invocations when the environment names a plausible-looking account. Ablation: reverting the runner ordering makes identityCalls 0 and the required harden succeed (2 red); dropping EACLIDENTITY from the allow-list makes both toMatchObject assertions fail (2 red). Co-authored-by: luvs01 <27862058+luvs01@users.noreply.github.com> Closes #1149
The audit named this the ready gate, and it was right to. Every existing test proves a piece — admission, the route allowlist, CORS, the bind scope, the injected port — and none of them prove the thing the feature exists for: that a real `codex app-server`, spawned the way a third-party host spawns it, reaches the proxy without a credential. That seam is between two processes. Not a `bun test` file. The repository does not depend on `@openai/codex`, so a test that skips when it is absent would report green on machines that never ran it. This fails loudly and its output is the evidence. The oracle is a routed model id generated at run time. Codex caches model lists and falls back to a bundled catalog when a refresh fails, so "did model/list succeed" proves nothing — a broken path looks identical to a working one. A name no bundled catalog can contain can only have arrived through our listener. Writing it surfaced two things worth recording. `model/list` reads `model_catalog_json`; it does not call the provider's `/v1/models`, so the first version watched Codex return its five bundled ids while never touching the listener — the exact false-negative shape the unique id exists to expose, pointed at the harness instead of the feature. And a hand-written catalog fixture is a liability: Codex rejects the whole file on any schema mismatch and silently falls back, so the catalog is now built with our own serializer, which also means the script exercises the bytes `ocx sync` writes. Result on Codex 0.146.0, isolated CODEX_HOME with no models_cache.json and OPENCODEX_API_AUTH_TOKEN stripped from the child environment: 9/9, ending with POST /v1/responses observed on the loopback listener. Ablation: pointing base_url at a dead port makes the last check red, so the harness is watching the hop rather than asserting its own setup. Refs #1102
react-doctor's prefer-module-scope-pure-function, and it is right: the function closes over nothing, so rebuilding it on every render is wasted work. The prepush doctor gate rejected the push over it.
…t_index tables Final-gate audit found the cross-table fallback could hand a reasoning content_part the MESSAGE canonical id when both tables held the same output_index, and an out-of-order part event passed through unrepaired. Track raw upstream id -> canonical id at item registration and rewrite part/delta events by exact raw-id match first; the index table remains only for events without a known raw id. A reused index can no longer borrow the sibling item's id, and function_call part events stay untouched.
…d drop the positional guess Final-audit round 2 reproduced two defects in the raw-id rewrite: a flat raw-id map collapsed items sharing one placeholder id into the last item's canonical id, and the index fallback could still hand a function_call part event (or an already-canonical id) the sibling message's identity on a reused index. The map is now keyed by (output_index, rawId), and events that carry an item_id are rewritten only on an exact key match — an unknown id is left alone instead of guessed by position. The index table serves only item_id-less events behind repairMissingTerminalIds, the pre-existing contract. Regression tests pin all three reproductions.
Contributor
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Contributor
|
✅ Deterministic PR hygiene checks passed. |
github-actions
Bot
changed the title
Contributor
⏳ DRAFT
What to do
Its title has been prefixed with |
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
4 participants
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.
Temporary maintainer-only PR used to extract the exact current-dev delta from #1206's previous base. Do not review or merge.