Post-merge review request: 260806 bug-campaign stack landed on dev without maintainer approval #1166

Activity

1. github-actions commented on Aug 7, 2026

   

   github-actionsbot

   on Aug 7, 2026 – with GitHub Actions

   Last edited by github-actions

   Contributor

   More actions

   _{Automated translation bookkeeping — detected language: unknown.}
2. Wibias commented on Aug 7, 2026

   

   Wibias

   on Aug 7, 2026 – with ChatGPT Codex Connector

   Contributor

   More actions

   Post-merge review completed

   This is a post-merge review, not retroactive approval. I reviewed the integrated range and the four areas this issue specifically calls out, with the current dev implementation checked where later changes could have affected the conclusion.

   Verdict

   No product or security defect found in the requested focus areas. I do not see a reason to revert the landed stack.

   The process violation described in this issue remains real: the required pre-merge maintainer approval did not happen, and a review after the fact does not repair that checkpoint. This comment only records the technical post-merge review that was requested.

   #1134 — explicit security review: PASS

   I checked the native-profile stage-sweep fast path rather than relying on the PR description.

   NativeProfileManager.stageSweepRequired() returns false only when both stageRegistryPath and stagingRoot produce ENOENT. Any existing entry — including a symlink or wrong-type entry — returns true, and any observation error other than ENOENT also returns true. Those cases therefore enter the existing locked sweep instead of taking the shortcut.

   The locked path remains fail-closed:

   • the registry reader validates file type/identity and bounds before parsing;
   • the staging-root path is canonical/type checked;
   • unsafe or unreadable registered stages set the cleanup/plaintext signal rather than being treated as absent;
   • startup keeps the native-main gate blocked when plaintextMayRemain is possible.

   I also checked the check-then-skip race inherent in an absence preflight. A concurrently created new stage does not let pre-existing residue evade cleanup: prepareStage() itself runs under the native-profile transaction lock, sweeps existing state before creating a stage, creates/hardens the staging root before reserving the registry record, and the periodic startup cleaner repeats the same preflight/sweep. A stage created after the absence check is new live state, not previously hidden plaintext residue. I do not see a credential-confidentiality regression from the lock-free empty-state shortcut.

   The other source changes in #1134 are the local-maintenance error wording/mapping; they do not weaken native-main owner/claim protection.

   Security conclusion for #1134: the stated safety boundary holds. Empty state avoids an unnecessary SQLite transaction; present or unprovable state still takes the fail-closed path.

   #1138 — Anthropic-only response identity: PASS

   The gate is where claimed in src/server/responses/core.ts.

   The original client-facing selector is copied to _responseModelId only when the selected route is Anthropic by provider name or Anthropic adapter. The upstream request still receives the normalized physical model id.

   Downstream response builders/loops use _responseModelId ?? parsed.modelId, so the alternate identity is inert for every route where the gate did not set it. The added regression test explicitly covers a non-Anthropic qualified provider and pins its pre-fix bare response.model behavior.

   Request logging separately preserves the physical routed model so rewriting client-facing metadata does not rewrite observability into the public selector.

   I found no scope leak to non-Anthropic providers.

   #1139 — forged ingress-span read withheld: PASS

   The merged #1139 source set contains the vision raw-body synchronization and ordinary-attempt persistence work, but does not include the public server ingress/auth files from #1093's withheld correlation-header half.

   I also directly inspected current dev src/server/index.ts: there is no x-opencodex-ingress-span read there. The ordinary attempt is created only after final adapter resolution; no client-supplied value is promoted into a guard-issued/persisted ingress identity by this change.

   The decision to withhold that half of #1093 was correct. A regex-valid client header is not evidence that a trusted guard issued it.

   #1144 — reasoning replay stays memory-only: PASS

   The current src/responses/reasoning-replay-cache.ts still implements the documented memory-only contract:

   • in-process Map only;
   • bounded entry count and total bytes;
   • one-hour TTL;
   • no filesystem/config persistence imports or serialization path.

   The persistence helper proposed by unmerged #1126 (src/lib/config-dir.ts) is not present on dev, and the current config/OpenAI-chat paths do not contain the proposed persistence hook.

   #1144 also added a subprocess regression that sets OPENCODEX_REASONING_REPLAY_PERSIST=1 and OPENCODEX_REASONING_REPLAY_FILE and asserts that no file or directory is created. The empty-delta fix itself only changes when an in-memory replay candidate is cleared; it does not add a new storage surface.

   I found no on-disk reasoning persistence in this landed change.

   Integrated-tree evidence

   I independently verified the cited CI run rather than treating the PR-head checks as integrated evidence:

   • range end: 20c5bd230be4b06f37a9259fe0fade2ac9a4f139 (test(usage),docs(codex-app-models): isolate fixtures and document the Desktop allowlist #1150 merge)
   • GitHub Actions run 31137393645
   • branch: dev
   • head SHA: exactly 20c5bd230be4b06f37a9259fe0fade2ac9a4f139
   • final status: completed / success
   • successful execution is run attempt 2

   That supports the issue's distinction between the original PR-head CI and the later integrated-tree validation.

   Final disposition

   For the four explicitly requested review points:

   • fix(codex): skip empty native-profile stage sweeps (#1120) #1134 security boundary: sound
   • fix(anthropic): preserve the provider-qualified response model (#1117) #1138 Anthropic response-identity gate: sound and scoped
   • fix(vision,telemetry): sync captions into the passthrough body and persist ordinary attempts #1139 untrusted ingress-span half: correctly withheld
   • fix(cursor,responses): convert structured edits and keep replay across empty deltas (#1017) #1144 reasoning persistence: correctly withheld; memory-only contract intact

   No revert recommended. No follow-up defect issue identified from this review.

   This does not convert the original merges into approved PRs. The useful follow-up from the incident is procedural/enforcement work (branch protection or an equivalent hard gate), not a product rollback.
3. Wibias

   closed this as completedon Aug 7, 2026