diff --git a/devlog/_plan/260917_l7_native_control_stack_audit/000_plan.md b/devlog/_plan/260917_l7_native_control_stack_audit/000_plan.md new file mode 100644 index 0000000000..74fb88be8a --- /dev/null +++ b/devlog/_plan/260917_l7_native_control_stack_audit/000_plan.md @@ -0,0 +1,52 @@ +# L7 — native control stack: read-only audit + +Lane R-L7. Four open pull requests that GitHub shows as four independent +branches off `dev` are, in commit terms, one four-deep stack: + +```text +#4782 native WebSocket steering + └─ #4858 multi-agent function-result injection + └─ #4861 typed result continuations + hosted output preservation + └─ #4864 bounded steering waits + sparse replay output +``` + +This lane does not implement anything. It separates what each stage actually +adds, states the four stages as one contract a reviewer can check, decides in +code whether issue #4850 gates the stack, and records what still has to happen +before either flag is turned on. Corrections that belong to an author go to that +author's pull request as a review comment; no pull request is superseded, +rebased or reimplemented here. + +## Units + +- 010 — parent-relative diff of each stage. +- 020 — the four stages as one integration contract, with verdicts. +- 030 — whether #4850 (native-main read fence) is a precondition. +- 040 — stack hygiene, upstream evidence, and the activation decision. + +## Write scope + +`devlog/_plan/260917_l7_native_control_stack_audit/` only. No `src/`, no +`tests/`, no `structure/`, no `docs-site/`. The four audited branches are read +through `git show` and `git diff` against fetched `refs/pull/*/head`; nothing in +this branch touches them. + +## Verification posture + +Nothing is executed. No suite, no focused file, no typecheck, no build, no +proxy. Every claim below is either a source read at a named commit or a hosted +CI fact read from GitHub, and each is written so a reviewer can re-derive it +from the same command. Where a claim could not be established from source it is +recorded as unproven rather than assumed. + +Reference points, all read on 2026-09-17: + +| Ref | Commit | +|---|---| +| `origin/dev` | `f1dfda8e48b52a1734eb202550a0225f3e5f8ab1` | +| #4782 head | `76d7452afb38fd7cc5d9ff7fa4d573b06a9507e3` | +| #4858 head | `7a9a6d28dd8680cce890e06813e4a08796624d0a` | +| #4861 head | `59a1d6357e018d44104a50b1126350b72368c81d` | +| #4864 head | `7b548ad85e8f2a6af313198fa68a4111003cbb05` | +| #4868 head (undeclared fifth level) | `15a8e715851d53d13d3718b16c8ad4cdc8e6ec32` | +| openai/codex pinned checkout | `095da4b7e` | diff --git a/devlog/_plan/260917_l7_native_control_stack_audit/010_stage_diffs.md b/devlog/_plan/260917_l7_native_control_stack_audit/010_stage_diffs.md new file mode 100644 index 0000000000..ba00b4524e --- /dev/null +++ b/devlog/_plan/260917_l7_native_control_stack_audit/010_stage_diffs.md @@ -0,0 +1,110 @@ +# 010 — what each stage actually adds + +## Why the GitHub diff is misleading + +All four pull requests declare base `dev`, and none of them uses the stacked-child +workflow described in `AGENTS.md`. Each therefore shows its parents' commits in +its own diff. #4858 is the extreme case: against `dev` it reads as 120 files and +`+6441/-1004`, because its branch also carries a pinned-`dev` merge +(`9c411a1048`) that drags in unrelated integration work. Its own feature delta is +39 files. + +The ranges below are the parent-relative deltas. Each one is reproducible: + +```bash +git fetch origin pull/4782/head:pr4782 pull/4858/head:pr4858 \ + pull/4861/head:pr4861 pull/4864/head:pr4864 +git diff --stat "$(git merge-base origin/dev pr4782)" pr4782 # L7.1 +git diff --stat 9c411a1048 pr4858 # L7.2 +git diff --stat de600be5f3 pr4861 # L7.3 +git diff --stat b00654b368 pr4864 # L7.4 +``` + +## Per-stage delta + +| Stage | PR | Range | Total | `src/` | `tests/` | `structure/` + `docs-site/` | +|---|---|---|---|---|---|---| +| L7.1 | #4782 | merge-base → `76d7452afb` | 40 files, +1278/-33 | 15 files, +669/-31 | 3 files, +478/-1 | 21 files, +129 | +| L7.2 | #4858 | `9c411a1048` → `7a9a6d28dd` | 39 files, +1165/-71 | 16 files, +495/-52 | 5 files, +519/-19 | 17 files, +150 | +| L7.3 | #4861 | `de600be5f3` → `59a1d6357e` | 29 files, +621/-58 | 7 files, +230/-38 | 5 files, +292/-3 | 16 files, +98/-17 | +| L7.4 | #4864 | `b00654b368` → `7b548ad85e` | 25 files, +496/-41 | 5 files, +121/-39 | 3 files, +275/-1 | 16 files, +98 | + +The `structure/` rows are almost entirely ownership-table lines required by +`structure:check`, not new architecture prose. + +## L7.1 — #4782, native WebSocket steering + +New modules: `native-steering.ts` (the channel: envelope validation, parent/steer +bookkeeping, settings pinning by digest, chain and byte caps), +`native-steering-replay.ts` (connection-local journal where only a +`response.created` successor commits queued input), `native-steering-log.ts` +(per-response usage aggregation that never samples control frames). + +Wiring: `codexNativeSteering` in the config schema and `OcxConfig`; the inbound +WS handler recognizes `response.steer` and routes `response.create` through +`continue()`; `ws-upstream` skips the idle-socket pool when a control channel is +present; `codex-ws-exchange` attaches the channel and owns control sends; +`passthrough-delivery` keeps the bounded upstream as the sole reader of a +multi-terminal stream; `ws-bridge` gains `untilEof` so one SSE body may carry +several response terminals. + +One refactor rides along: `markBodyNonPersistable` moves from `responses/state.ts` +to a new `responses/state/body-policy.ts` so the dispatch path can also read it. + +## L7.2 — #4858, multi-agent function-result injection + +New modules: `native-injection.ts` (a second, separate owner with a FIFO of at +most one in-flight frame, because the acknowledgement names the response rather +than the injection), `native-injection-protocol.ts`, `native-injection-replay.ts`, +`native-response-control.ts` (the `NativeResponseControl` interface, the +eligibility predicate and the mode selector). + +This is the stage that widens the route surface. `nativeResponseControlEligible` +keeps canonical ChatGPT forwarding for both modes and additionally admits, for +injection only, an `openai-responses` provider pinned to exactly +`https://api.openai.com/v1` with `upstreamWebsocket: true` and a non-forward auth +mode; on that route `ws-upstream` appends `responses_multi_agent=v1` to +`openai-beta` without discarding configured tokens. Mode selection reads the +frame, never the model name: a request carrying `multi_agent.enabled: true` can +only obtain the injection channel, and the steering channel's constructor rejects +it outright. + +## L7.3 — #4861, typed result continuations and hosted output + +New modules: `native-tool-results.ts` (the wider saved-result schema — +`custom_tool_call_output`, `mcp_approval_response`, rich content parts with +bounded image/file references, and caller provenance reduced to a digest) and +`native-response-output.ts` (merges completed `response.output_item.done` items +with a sparse terminal `output`, preserving relative order and failing on a +contradiction instead of dropping items). + +Two later commits on this branch are corrections, and they matter to L7.4: +`4670525d48` rejects a continuation that *omits* a pinned setting — before it, +only a changed setting was caught, so dropping a key bypassed the pin — and +`59a1d6357e` refunds the exact reserved byte count of an injection batch instead +of recomputing it from a possibly different serialization. + +## L7.4 — #4864, bounded waits and sparse replay output + +Replaces the steering channel's single re-armable `wait()` with absolute +deadlines. Before this stage each control re-armed a fresh 90-second window, so a +client that kept sending controls could hold the socket indefinitely, and late +wire activity could win a race against an expired-but-unfired timer. After it, +every stage carries its own absolute deadline (`nextDeadline` takes the earliest, +`armTimer` never extends one, `assertTimely` settles on arrival, `expire` settles +exactly once and reports unknown delivery rather than retrying). + +It also extracts `native-response-json.ts` so the JSON record/fingerprint helpers +no longer live in the injection protocol module, and routes the steering replay's +terminal output through `nativeResponseOutput` — previously it took the terminal +`output` whenever non-empty and silently dropped observed items a sparse terminal +omitted. + +## The divergence a reviewer must see + +#4864 is based on `b00654b368`, which is #4861's *feature* commit, not #4861's +head. The two corrections above are not in #4864's branch, so nothing has been +built or tested on the combination `dev` will actually receive. A squash merge in +#4861 → #4864 order does not revert them, because neither correction touches a +line #4864 edits, but the combined behavior is unverified. #4864 should be rebased +onto #4861's head before it is treated as the stack tip. diff --git a/devlog/_plan/260917_l7_native_control_stack_audit/020_integration_contract.md b/devlog/_plan/260917_l7_native_control_stack_audit/020_integration_contract.md new file mode 100644 index 0000000000..4b77e6e4d4 --- /dev/null +++ b/devlog/_plan/260917_l7_native_control_stack_audit/020_integration_contract.md @@ -0,0 +1,141 @@ +# 020 — the four stages as one contract + +The stages are separate pull requests but a single runtime object graph: one +downstream WebSocket turn owns one control channel, that channel owns one +physical upstream socket, and every later control frame is emitted through the +closure that opened it. The five clauses below are what that graph has to +guarantee for the stack to be safe to enable. Each verdict is a source read at +the heads listed in `000_plan.md`. + +## C1 — the selected account and the original physical socket are preserved + +**Holds by construction.** + +`codexWsUpstreamFetch` computes a pool-reuse identity only when no control +channel is present (`const identity = control ? null : codexWsReuseIdentity(...)`), +so an owned connection is never taken from, and never returned to, the idle +socket pool. Control frames are sent through the `ws` captured by +`codexWsExchange`, which is the same physical connection that carried the +original create. A `response.create` continuation is not re-routed: the exchange +rebuilds it from the original `frameText` and overlays only `input` and +`previous_response_id`, so a caller-supplied `previous_response_id` never reaches +the REST sanitizer or the account selector. The per-send guard receives +`new Headers(headers)` — a copy — so it can reserve quota and refuse a send but +cannot swap the credential underneath an open socket. + +Both channels also pin the request's non-envelope settings as SHA-256 digests at +construction and reject any continuation whose settings differ. After +`4670525d48` (on #4861, not on #4864) the injection channel also rejects a +continuation that omits a pinned key. + +Residual: preservation is the point, so the credential chosen at create time is +the credential the whole chain uses, for as long as the chain lives. See C4 for +how long that can be, and 030 for why that makes #4850 an activation +precondition. + +## C2 — an unsupported route does not detour to another account or to HTTP + +**Holds, with one seam worth an explicit branch.** + +Four independent gates have to agree before a channel is constructed or used: +`nativeResponseControlMode` requires the matching flag to be exactly `true`; +`nativeResponseControlEligible` requires canonical ChatGPT forwarding, or — for +injection only — an `openai-responses` provider pinned to exactly +`https://api.openai.com/v1` with `upstreamWebsocket: true` and a non-forward auth +mode; `preparePassthroughExchange` additionally requires `inboundTransport === +"websocket"`, no Combo attempt, and no plaintext-v2 agent-message tool rewriting; +`codexWsUpstreamFetch` re-checks `prepared.canonical` (or the exact public API +URL) and, for injection, re-parses the outgoing frame to confirm +`multi_agent.enabled`. A route that fails any of them gets `undefined`, and a +later `response.steer` or `response.inject` is answered with an explicit +`steering_not_supported` / `injection_not_supported` error frame rather than +being discarded or retried elsewhere. + +The seam is in `codex-ws-exchange`. `nativeSteering.attach()` is called inside the +same `try` block as `ws.send(frameText)`, and that block's `catch` treats a +pre-activity failure as "the frame never left" and resolves `sseFallback(url, +init)`. For steering that is reachable but harmless, because `attach` only +refuses when the channel is already bound. For injection it is reachable and +consequential: `NativeInjectionChannel.attach` throws permanently once +`everAttached` is set, so a second physical WebSocket attempt for the same turn — +the transient-retry wrapper passes the same channel to every dispatch site — +converts a multi-agent turn into an ordinary HTTP turn while the client still +holds a channel that can never attach. No credential moves and no success is +invented; the client learns only when its first `response.inject` is refused. +An attach failure should be distinguishable from a send failure rather than +sharing the fallback path. Raised on #4858. + +## C3 — a steering or injection failure is never presented as success + +**Holds.** + +Every settle path reports uncertainty instead of inventing an outcome. +`NativeSteeringChannel.expire()` settles once, disposes the replay journal and +calls `onFailure`, which fails the client stream; the message states that +delivery is unknown and that tools and steering input must not be replayed. +`NativeInjectionChannel.fail()` does the same and never falls back to HTTP, +re-sends or re-runs a tool. An acknowledgement must match the sole in-flight +submission by response ID and strictly increasing sequence number, and a +`response.inject.failed` must carry a fingerprint of exactly the submitted +results, so a rejection cannot be attributed to a different batch. Only a +`response_already_completed` rejection is marked recoverable, keyed by a digest +of the saved result rather than a second copy of it. A response terminal does not +finish the owner while submitted results are unacknowledged. + +In the replay journals, only a validated `response.created` successor commits +queued input, and only `response.completed` reaches shared continuation state — +a steered or failed parent's output is used solely as a successor's prefix. + +One suppression exists and is correct: `createNativeSteeringLogObserver` does not +record a parent's `response.incomplete` with `incomplete_details.reason === +"steered"` as an upstream failure. That affects the request log only; the frame +itself is still relayed to the client unchanged. + +## C4 — cancellation and confirmation waits terminate finitely + +**Holds per stage after #4864 — but the chain has no aggregate bound.** + +After #4864 every wait is an absolute deadline rather than a re-armable window: +90 s per unacknowledged steer, 90 s for an automatic successor after a parent +terminal, 90 s for a sent continuation, 30 minutes for a server-requested +required-input wait, and `stallTimeoutSec` (default 300 s) of idle while a +response is streaming. The injection channel uses the same 90 s acknowledgement +bound, explicitly non-extendable by unrelated stream activity, plus the same +30-minute saved-result wait. `assertTimely()` closes the race where a late frame +arrives after a deadline passed but before its timer fired. + +What is not bounded is their composition. A chain may run up to 128 responses on +one owned connection, and each response may legitimately consume its own idle and +required-input waits, so a single downstream turn can hold one physical socket +and one pinned credential for far longer than any ordinary turn — on the order of +tens of hours in the worst case, without any individual deadline being violated. +Nothing in the stack caps the lifetime of the owned connection itself. That is an +operational number the activation decision needs, not a correctness defect. +Raised on #4864. + +Client disconnect and supersession are handled: a new `response.create` clears +`ws.data.nativeSteering` before admission and calls the previous turn's +`cancel()`, and the exchange's `cleanup()` runs `detachSteering`, which drops the +timers, the retained prefix and every queued submission. + +## C5 — the default-off boundary the original PR proposed is intact + +**Holds.** + +`codexNativeSteering` and `codexNativeInjection` are optional booleans in the +config schema, absent by default, and the mode selector requires `=== true`. +Neither channel can be constructed outside the inbound WebSocket create path, +which itself requires the already-opt-in `websockets: true`. Injection requires a +third, client-supplied gate: the create frame must carry +`multi_agent.enabled: true`. No model name, catalog entry or capability flag +turns any of this on, and rollback is unsetting the flag and restarting. + +## Summary + +| Clause | Verdict | Follow-up | +|---|---|---| +| C1 account and socket identity | Holds | — | +| C2 no unsupported-route detour | Holds, one seam | Comment on #4858 | +| C3 no failure reported as success | Holds | — | +| C4 finite waits | Holds per stage, chain unbounded | Comment on #4864 | +| C5 default-off boundary | Holds | — | diff --git a/devlog/_plan/260917_l7_native_control_stack_audit/030_l1_precondition.md b/devlog/_plan/260917_l7_native_control_stack_audit/030_l1_precondition.md new file mode 100644 index 0000000000..11ad767524 --- /dev/null +++ b/devlog/_plan/260917_l7_native_control_stack_audit/030_l1_precondition.md @@ -0,0 +1,61 @@ +# 030 — is #4850 a precondition for this stack? + +Issue #4850 reports that a `thread_spawn` request authenticating with its own +forwardable Codex bearer still opens the operator's physical native-main +`auth.json` during request preview. The existing fence +(`nativeMainReadsForbidden`, threaded through `request-prepare.ts`, +`auth-context.ts`, `core-normalize.ts` and `subagent-model-fallback.ts`) already +covers quota priming, entitlement discovery, denial-cache validation, +reconciliation and final selection. Pool eligibility is outside it: +`isCodexAccountUsable` reaches `isMainAccountCredentialUsable()` for the main +account unless `nativeMainSelectionOnly` is set, and the preview closures at +`request-prepare.ts:550` and `:731` call `previewCodexAccountForRequest` without +that suppression. + +## Mechanically, no + +The four pull requests touch nothing in that path. Taking the whole stack at +#4864's head against its included `dev`: + +```bash +git diff --name-only 7ef3f67452 pr4864 -- src/codex src/routing \ + src/server/responses/request-prepare.ts +# (no output) +``` + +Nothing under `src/codex/`, nothing under `src/routing/`, and not +`request-prepare.ts`. The stack adds a control channel below the point where +preview and selection have already run. It does not re-enter them either: a +continuation is rebuilt from the original create frame inside +`codex-ws-exchange`, so `previous_response_id` never reaches the account +selector, and the per-frame guard gets a copy of the original headers. So #4850 +is not a merge-order blocker, and none of the four PRs can fix or worsen the +read itself. + +## Substantively, yes — for turning the flags on + +Two facts make it a precondition for activation rather than for merging. + +First, the request class is the same one. #4850's reproduction is a +`thread_spawn` request carrying a caller-owned bearer, and a multi-agent +injection turn is exactly that class: every injection create traverses +`prepareResponsesRequest`, and therefore the unfenced pool-eligibility preview, +before any channel exists. Enabling `codexNativeInjection` does not introduce the +read, but it makes the affected request class the primary use of the feature. + +Second, the stack's whole value is that the create-time decision is pinned. C1 +holds precisely because the account chosen at create time is the account the +entire chain uses, and C4 shows that chain can be long. #4850's observable +consequence — operator-main liveness, cached quota and plan state influencing a +subagent model rewrite for a request that owns its own credential — is a +one-request inconsistency today. Under this stack the same preview result governs +up to 128 responses on one pinned credential, with no re-evaluation point in +between, because there deliberately is none. + +## Determination + +#4850 does not gate landing #4782, #4858, #4861 or #4864. It gates documenting +or recommending `codexNativeInjection: true`, and it should be resolved before +any operator is told to enable it. The lane that owns #4850 should know that +fixing pool eligibility inside the fence is enough for this stack; no additional +seam is needed on the native control path. diff --git a/devlog/_plan/260917_l7_native_control_stack_audit/040_activation_decision.md b/devlog/_plan/260917_l7_native_control_stack_audit/040_activation_decision.md new file mode 100644 index 0000000000..4c71deaccd --- /dev/null +++ b/devlog/_plan/260917_l7_native_control_stack_audit/040_activation_decision.md @@ -0,0 +1,95 @@ +# 040 — stack hygiene, upstream evidence, and what to do next + +## No repository CI has run on any of the four heads + +Read on 2026-09-17, every one of the four pull requests shows the same five +checks and no others: `enforce-target`, `hygiene`, `label`, `resolve-pr` and +CodeRabbit. The repository's own typecheck and test matrix has not run at any of +these heads. The verification tables in the pull request descriptions are real +but they are fork runs under `luvs01/opencodex`, not this repository's CI. + +```bash +for n in 4782 4858 4861 4864; do gh pr checks "$n"; done +``` + +This lane therefore cannot report exact-head CI evidence for the stack, and +neither can the PR authors: contributor pull requests cannot start repository CI, +so a maintainer dispatch is required. Three of the four (#4858, #4861, #4864) are +still drafts, and CodeRabbit skips drafts, so even the automated review only +covers #4782 and #4864. + +## The stack is five deep, not four + +`#4868` (`codex/steering-completion-20260917`, head `15a8e715851d`, draft) is a +child of #4864 and adds "safe steering settings overrides, public API transport +and executable probes". It declares base `dev` like the rest. Any decision about +activation scope has to account for it, because it widens the settings a +continuation may change — which is the pin C1 currently relies on. + +All five should use the stacked-child workflow from `AGENTS.md`: target the +parent's head branch while the parent is open, and retarget to `dev` once it +lands. `enforce-target` skips the wrong-base gate for those children. Doing that +would also make each PR's GitHub diff show only its own stage. + +## No available source proves the wire exists + +At the pinned openai/codex checkout (`095da4b7e`), the client-to-server WebSocket +request enum has exactly one variant: + +```rust +// codex-rs/codex-api/src/common.rs +pub enum ResponsesWsRequest<'a> { + #[serde(rename = "response.create")] + ResponseCreate(ResponseCreateWsRequest<'a>), +} +``` + +There is no `response.steer` and no `response.inject` frame. Upstream steering is +a local mechanism — `codex-rs/core/src/session/input_queue.rs` queues pending +steers into the next turn's input — and `Feature::Steer` is registered +`Stage::Removed` with `default_enabled: true`, meaning always-on locally rather +than negotiated on the wire. `session/inject.rs` likewise injects into local +session state, not upstream. Nothing in the pinned tree sends +`responses_multi_agent=v1`. + +The pull requests are honest about their source: both cite public +`developers.openai.com` guides, and #4858 states plainly that public API +documentation is not evidence that a ChatGPT subscription backend or a Codex +App/CLI build implements the same execution mode. That is the right caveat, and +it has a consequence the stack's framing should carry: on the canonical +ChatGPT forward route, no known client sends these frames and no captured wire +shows the backend answering them. The public API route in #4858 is the only leg +with published documentation behind it. + +This does not argue against the code. It argues against enabling the canonical +route first, and for treating a captured wire exchange as the gate. + +## Recommended order + +1. Rebase #4864 onto #4861's head so the two corrections in `4670525d48` and + `59a1d6357e` are inside the tested combination, and restack all five on their + parents' head branches. +2. Maintainer-dispatch repository CI at each exact head, bottom-up. Until that + exists there is no evidence this repository can cite. +3. Land #4782 → #4858 → #4861 → #4864 with both flags off. Every clause in 020 + holds at the tip, and default-off means landing them changes no behavior for + any existing user. +4. Resolve #4850 before recommending `codexNativeInjection: true` to an operator + (030). +5. Decide activation scope last, and decide the public API injection route and + the canonical ChatGPT route separately. The first has documentation behind it; + the second needs a captured wire. + +Turning steering on first and finishing in production is the one order to avoid. +The steering channel is the layer whose waits were unbounded until #4864, whose +settings pin #4868 proposes to relax, and whose canonical route has the least +evidence. It is the last thing that should be enabled, not the first. + +## Review comments filed + +| PR | Point | +|---|---| +| #4782 | Stack topology and restacking; no repository CI at the exact head; upstream wire evidence gap on the canonical route | +| #4858 | `attach()` failure shares the `sseFallback` path with a send failure, permanently disabling injection for the turn | +| #4861 | Its two corrections are not in #4864's base; ask for a rebase rather than a carry | +| #4864 | Base divergence from #4861's head; per-stage deadlines are bounded but the owned connection's lifetime is not |