diff --git a/devlog/_plan/260816_wave34_closeout/090_1524_capability_preflight.md b/devlog/_plan/260816_wave34_closeout/090_1524_capability_preflight.md index 9fdcef9486..9502ccc5a5 100644 --- a/devlog/_plan/260816_wave34_closeout/090_1524_capability_preflight.md +++ b/devlog/_plan/260816_wave34_closeout/090_1524_capability_preflight.md @@ -48,3 +48,61 @@ Cover BOTH paths explicitly: - A candidate with unknown modality support is skipped for an image request but still usable for a text one. - No compatible candidate produces a typed failure naming the constraint, not a silent truncation. - Cooldown and prior-attempt exclusion still apply unchanged. + +--- + +## Implementation outcome (verified at `812e7c40b`) + +The plan above proposed two mechanisms. Tracing the actual code changed what was needed. + +### What shipped + +A distinct `input_admission_refused` code (`src/server/responses/core.ts:1979`) that is +`hop`-eligible in `comboFailureDecision` (`src/combos/failover.ts:132`), while upstream +`context_length_exceeded` still stops (`:124`). + +This covers BOTH fallback paths, which the plan assumed needed separate fixes: + +- **Combo fallback** consults `comboFailureDecision` directly. +- **Policy fallback** consults the SAME function — `shouldHopPolicyCandidate` + (`src/server/responses/policy-fallback.ts:72`) delegates to it, which the plan missed + when it proposed re-evaluating candidates inside `policy-fallback.ts:153`. + +So the context-window half of #1524 is closed by one change rather than two. + +### What the plan got wrong + +- **"Fallback reuses the frozen verdict, so a candidate is never re-checked" understates what + already worked.** Every concrete retry re-enters `handleResponses`, which runs + `checkInputAdmission` against the NEW candidate's own ceiling before upstream I/O + (`core.ts:1970`). The candidate was always checked; the defect was that its refusal + TERMINATED the chain instead of advancing it. +- **Modality was already filtered.** `evidenceFromBody` sets `imageInputRequired` + (`src/routing/request-evidence.ts:43`), and the evaluator turns it into a + `request-image-input` requirement per candidate (`src/routing/evaluator.ts:209`), producing + a `capability-unsatisfied` exclusion. `rankPolicyFallbackCandidates` only considers + candidates with `eligible === true` and zero exclusions + (`policy-fallback.ts:32`), so an image request can never hop onto a text-only candidate. + The plan's proposed image tests would have passed before any change. +- **Unknown capability is already operator-controlled, not silently permissive.** + `excludedByUnknown` (`evaluator.ts:291`) excludes on unknown evidence when the profile sets + `unknownEvidence.capability = "exclude"`. The plan asked to make conservative-unknown + unconditional; doing so would change routing for every profile that deliberately allows + unknown evidence, which is a behavior change the issue does not ask for. + +### Remaining gap + +Request context size is still `unknown` in `PolicyRequestEvidence`, so the INITIAL policy +evaluation cannot pre-exclude an oversized candidate — it is discovered at admission and then +hopped. That is correct but wasteful: the chain walks candidates one refusal at a time instead +of ranking only those that fit. + +Closing that needs a model-independent size estimate computed once and compared against each +candidate's ceiling at the same `ADMISSION_TOLERANCE = 2.5` the admission gate uses +(`src/server/responses/input-admission.ts:32`). Using a stricter threshold at evaluation time +would refuse candidates that admission would have accepted — an outage in the name of a fix. + +This is an optimization of an already-correct chain, not the reported defect. #1524's +acceptance behavior ("reject candidates that cannot accept the request before retrying") holds +today for both context and modality. + diff --git a/devlog/_plan/260816_wave34_closeout/101_1049_legacy_adoption.md b/devlog/_plan/260816_wave34_closeout/101_1049_legacy_adoption.md index 0a08d1052a..e0e4683604 100644 --- a/devlog/_plan/260816_wave34_closeout/101_1049_legacy_adoption.md +++ b/devlog/_plan/260816_wave34_closeout/101_1049_legacy_adoption.md @@ -24,3 +24,86 @@ Preserving legacy operability on refusal is the non-negotiable part: a failed ad ## Tests `tests/codex-inject-write-lock.test.ts:144` asserts the bypass being removed — update it. Add per-state fixtures: adoptable home adopts and then uses the lock; indeterminate residue refuses; invalid record refuses; unversioned/rowless database refuses; a kill at each I/O boundary leaves either the pre-adoption state or a resumable pending row, never a half-published home. + +--- + +## Audit correction (independent read-only audit at `ba456bdcf`) + +The plan above was written against a mid-run snapshot and is materially stale. A parallel +audit checked every symbol, path, and line it names against the tree. Adopting it as written +would produce uncompilable or vacuous work in four places. + +### Confirmed accurate + +`applyNativeArtifacts()` (`src/codex/inject.ts:883`, legacy branch at `:901`), +`codexWriteCoordinationEligibility()` (`src/codex/inject-coordination.ts:46`, `legacy-uncoordinated` +returned at `:84`), `assertInitialStateCanBeCreated()` (`src/codex/transition-state.ts:269`, +unversioned refusal at `:288`, rowless at `:295`), `NativeRoutedResidueResult` +(`src/codex/native-residue.ts:46`), `readIntegrationRecordUnlocked()` +(`src/codex/integration-record.ts:98`), and the archived contract at +`devlog/_fin/260804_codex_write_substrate/005_contract.md:709` / `:735`. + +Two anchors drifted: the bypass-pinning test is at `tests/codex-inject-write-lock.test.ts:150` +(line 144 is now the `describe`), and the `ready | legacy-ambiguous | unavailable` union moved to +`src/codex/convergence-types.ts:319-322`. + +### Wrong in substance + +- **"Write a pending adoption row before publishing anything" is dangerous as written.** Read as + an insert into the final coordinator database it is wrong: the archived contract's publication + unit is a COMPLETE temporary SQLite database, validated, fsynced, and published atomically + without replacement, with `EEXIST` treated as a lost race that reopens the winner under strict + validation. An insert-then-publish sequence has a crash window the contract exists to remove. +- **The exact-byte fingerprint has no schema field and contradicts the recovery contract.** Byte + equality cannot distinguish "callback never started" from "partially completed" from "completed + then externally rewritten". Resumption must idempotently rerun the requested apply/restore. +- **"Clear the row" is incompatible with the singleton model.** Normal transitions update the + durable singleton to a terminal state; they never delete it. +- **"Routed + valid record" is narrower than the archived contract**, which admits a missing OR + valid v1 record with no legacy transition fields. "Clean-or-explainable residue" has no defined + predicate and would be vacuous policy language if implemented literally. + +### Vacuous as written + +Implementing adoption in `inject.ts` alone changes nothing: `withCodexWriteLock()` +(`src/codex/codex-write-lock.ts:249`) always calls `openCodexCoordinatorTransaction()`, which +refuses routed residue before any callback runs. Adding `"adoption-pending"` to +`transition-state.ts` alone does not compile meaningfully either — `CodexHistoryState` +(`src/codex/convergence-types.ts:36`), the runtime status set (`transition-state.ts:41`), and the +SQL `CHECK` (`:69`) all have to move together. + +### What the plan missed + +Call-time home resolution (`src/codex/paths.ts:32`), WSL home selection (`src/codex/home.ts:135`), +canonicalization and identity-derived database location (`src/codex/user-identity.ts:355`), and the +zero-byte first-use race already handled in +`tests/codex-transition-state-first-use-regression.test.ts:54`. + +Also worth recording: today's eligibility deliberately lets an `indeterminate` home keep writing +directly rather than refusing it. The plan's required shape contradicts that, and the change would +be user-visible. + +### Corrected shape + +1. `convergence-types.ts:36` — add `adoption-pending` plus the authority/intent fields needed to + resume an authorized apply or restore. A bare fingerprint is not sufficient. +2. `transition-state.ts:40` — extend the status validator and SQL schema together, and add a + compatibility-adoption publisher that builds a complete v1 database at a unique same-directory + temp path and publishes it atomically without replacement. +3. `inject-coordination.ts:41` — replace the broad legacy verdict with `adoptable | + legacy-uncoordinated | refused`, derived from the three existing classifiers. +4. `codex-write-lock.ts:249` — add the adoption-capable entry path, without which nothing above + is reachable. +5. `inject.ts:860` / `:1431` — route authorized legacy apply/restore through adoption, then the + normal N-protected transition. +6. Tests — new `tests/codex-transition-state-adoption.test.ts` for publication and crash/race + boundaries; end-to-end cases in `tests/codex-inject-write-lock.test.ts`; and update the + bypass-pinning test at `:150`, which currently asserts the very behavior being removed. + +### Disposition + +**Deferred, not attempted.** This is a crash-safe durable-state change across five files with a +publication protocol whose failure mode is an unusable Codex home — larger than the remaining +work-phases in this loop and not safely compressible into one. #1049 stays OPEN with this +corrected plan recorded; the audit above is the deliverable of this cycle. + diff --git a/devlog/_plan/260816_wave34_closeout/102_1798_restore_merge.md b/devlog/_plan/260816_wave34_closeout/102_1798_restore_merge.md index b5bbbe3040..9b26869869 100644 --- a/devlog/_plan/260816_wave34_closeout/102_1798_restore_merge.md +++ b/devlog/_plan/260816_wave34_closeout/102_1798_restore_merge.md @@ -31,3 +31,55 @@ When the merge cannot classify a key confidently, leave it and report it. A rest - User edits an unrelated key after injection: it survives restore byte-identical. - Hash mismatch no longer means give-up: the merge path runs and the home ends clean. - The catalog fallback at `parsing.ts:545` keeps working; add a regression if none pins it. + +--- + +## Audit correction and outcome (at `ba456bdcf`) + +An independent read-only audit checked this document against the tree before implementation. +Three of its claims were wrong in ways that would have produced vacuous work. + +- **A true baseline/injected/current three-way merge is not implementable from today's journal.** + `Journal` (`src/codex/journal.ts:19`) stores the full baseline but only `sha256` of the injected + state, so `I` does not exist as bytes. A real B/I/C merge requires a journal version that + persists the injected config, and that is a larger change than the defect needs. +- **"A user's own pre-injection `openai_base_url` is returned by restore" is vacuous.** Under + Design B, injection refuses to overwrite an unmarked user URL at all (`src/codex/inject.ts:258`), + so there is nothing to return. The real risk is the inverse — restore DELETING a URL we never + wrote — and that is what the shipped test pins. +- **`saveConfigPreservingClaudeCode()` and `mutatePersistedConfig()` are not reusable here.** They + mutate OpenCodex's own JSON config, not `$CODEX_HOME/config.toml`. Treating them as restore + machinery would be a domain error. + +The audit also corrected the catalog claim: the generic legacy backup is applied only when the +resolved path is the DEFAULT catalog (`opencodex-catalog.json`, `src/codex/catalog/parsing.ts:544`). +`models_cache.json` is not that path, and `tests/codex-catalog-restore.test.ts` already pins that a +custom path does not get the generic backup. So the "legacy fallback partially satisfies #1798" +line was false for the reported case. + +### What shipped + +A narrower fix keyed on evidence rather than formatting: `markJournalInjectedState` records the +exact root `openai_base_url` the injection wrote, and the fallback strip removes a root URL whose +value equals it. That survives the app's comment-dropping rewrite, which is the actual mechanism +in the report, while an exact-value match keeps restore from touching a user's own gateway. + +- `src/codex/journal.ts` — record and expose the injected URL. +- `src/codex/injected-marker.ts` — `stripJournaledOpenaiBaseUrl()`, value evidence beside the + existing marker-adjacency rule. +- `src/codex/inject.ts` — `removeCodexConfig()` reads the journal once and uses it for both the + ownership verdict and the strip. +- `tests/codex-restore-app-rewrite.test.ts` — reproduces the app rewrite literally; driven red by + pinning the accessor to null. + +Journals written before this change carry no recorded URL and fall back to today's marker rule, so +the change is backward compatible. + +### Still open + +The `models_cache.json` half. Catalog restore re-resolves its target from the post-rewrite TOML +(`src/codex/catalog/parsing.ts:185`), so a rewrite that dropped `model_catalog_json` sends restore +to the default catalog and never touches the proxy-written cache. The fix is to capture the +INJECTED catalog path in the journal and pass it into `restoreCodexCatalogWithPermit()` +(`src/codex/catalog/sync.ts:1686`) as an explicit target. #1798 stays OPEN for that. + diff --git a/devlog/_plan/260816_wave34_closeout/120_outcome.md b/devlog/_plan/260816_wave34_closeout/120_outcome.md index 23ae3b9460..370a861a88 100644 --- a/devlog/_plan/260816_wave34_closeout/120_outcome.md +++ b/devlog/_plan/260816_wave34_closeout/120_outcome.md @@ -62,3 +62,82 @@ remote before reading a suite result as evidence — a stale checkout produces a - `#1791` generic quota-window storage; `#1524` full capability preflight. - `#1795` stays open pending a live SenseNova/Kimi reproduction. + +--- + +# Continuation record — Wave 3/4 second pass + +Written after the merge of `#1861` and `#1862`. The table in the section above covers the +first pass; this records what the continuation established. + +## Landed in this pass + +| Unit | Issue | PR | State | +|---|---|---|---| +| `100` admission threading (second half) | `#1686` | `#1861` | merged, issue CLOSED | +| `102` restore after app rewrite, config + catalog | `#1798` | `#1862` | merged, issue CLOSED | +| `050` burst-window retention | `#1791` | `#1863` | open | +| `090` admission-hop ordering | `#1524` | `#1864` | open | + +## Findings that changed the work + +**`#1686` was two facts that never met.** `DataPlaneAdmission.source` was already resolved at +the door and `materializeCodexUpstreamAuth` already knew how to substitute — but the source +was dropped one frame later, so `resolveResponsesCodexAuth` ran the forward guard against a +bearer it had just admitted. The fix is threading, not new machinery. + +**`#1798` was a formatting proxy for an ownership question.** Marker adjacency cannot survive +a reserializing writer. Recording the injected VALUE makes ownership provable from evidence, +and an exact match keeps restore from deleting a user's own gateway. The catalog half was the +same shape one layer down: restore re-resolved its target from the post-rewrite config, so the +file it actually wrote became undiscoverable. + +**`#1791`'s first fix created the second defect.** Stopping the 5-hour window from being +mislabeled as weekly was done by DISCARDING it. The issue reports both windows as live upstream +limits, so an account could sit at 100% of its burst quota while opencodex showed a healthy +weekly bar and routed straight into a 429. + +**`#1524`'s hop rule existed but never fired.** `comboFailureDecision` tested the generic stop +list before the admission rule, and `classifyError` maps a real 413 admission body to +`context_length_exceeded` — so the request returned `stop` two lines before the rule written to +hop it. The existing test missed this because it used a top-level `{"code":...}` shape, which +classifies to `upstream_error`, misses the stop list, and reaches the rule. The rule looked +alive while the shape the proxy actually emits kept stopping. + +That last one is the ablation lesson of this pass: disabling the structured-code arm alone +still passes, because a `message.includes` fallback catches it. Only the ORDERING ablation +fails. An ablation that does not fail has not proven the mechanism it was aimed at. + +## Deferred with recorded evidence + +- **`#1049`** — corrected plan in `101`. The original was uncompilable in two places and + vacuous in one: adoption implemented in `inject.ts` alone is unreachable because + `withCodexWriteLock` refuses routed residue first, and the proposed "pending row before + publishing" reintroduces the crash window the archived contract removes. Five files, a + publication protocol, and a failure mode of an unusable Codex home — not compressible into + one cycle. +- **`#1795`** — `130`. The guard is a deliberate fail-closed contract; the request is to relax + it. Silently dropping an undeclared `exec` call trades a visible failure for an invisible + one, and the correct SCOPE (global / per-provider / subagent-only) is a product decision. + Evidence requested on the issue. +- **`#1524` remainder** — request context size is still unknown at initial policy evaluation, + so the chain discovers incompatibility at admission and hops rather than ranking only + candidates that fit. An optimization of an already-correct chain, not the reported defect. + +## Verification + +Remote Linux suite (`ssh lidge`, `bun test --isolate tests`), run at each PR head: + +| Head | pass | skip | fail | +|---|---|---|---| +| `798ecbfb7` (earlier baseline) | 12684 | 15 | 16 | +| `acfedae0a` (`#1861`) | 12687 | 15 | 16 | +| `6cd5b04b3` (`#1862`) | 12687 | 15 | 16 | + +The 16 failures are identical across all three and are `bun`-not-on-PATH harness cases +(`doctor-gui-if-changed`, `lint-gui-if-changed`, the two-process lock contention group, and +the generated-metadata sync check). No regression. + +Every merge had exact-head CI green apart from the macOS job, which is queued rather than +failing — the same pattern recorded in the first pass. + diff --git a/devlog/_plan/260816_wave34_closeout/130_1795_undeclared_tools.md b/devlog/_plan/260816_wave34_closeout/130_1795_undeclared_tools.md new file mode 100644 index 0000000000..b620884e30 --- /dev/null +++ b/devlog/_plan/260816_wave34_closeout/130_1795_undeclared_tools.md @@ -0,0 +1,57 @@ +# 130 — #1795: undeclared tool calls from a routed provider + +## Verified state (at `812e7c40b`) + +The guard is real and deliberate. Both bridge paths refuse an undeclared tool name: +streaming at `src/bridge.ts:1006` and non-streaming at `:1715`, each emitting a 502 +`upstream_error`. Pinned by `tests/bridge.test.ts:345` and +`tests/responses-stream-tool-events.test.ts:30`. + +So the reported behavior is not a defect in the sense of "code doing something nobody +intended". It is the designed fail-closed contract, and the reporter is asking for that +contract to be relaxed. + +## Why this is NEEDS_HUMAN rather than a fix + +The request — "tolerate undeclared tool calls, drop them with a warning" — changes a +safety boundary, and the failure modes on the other side are not obviously smaller than +the one being reported. + +An `exec` call the client never declared is, by construction, a request the client has no +handler for. Dropping it silently means the model believes it ran a command and receives +either nothing or a fabricated absence, and the turn continues on that false premise. For +`exec` specifically the current 502 is the honest outcome: the turn genuinely cannot be +completed as the model intended. + +There is also a real question of WHERE the tolerance belongs. Three candidate answers, +with materially different blast radii: + +1. **Global tolerance.** Every routed provider may emit any tool name and have it dropped. + Largest blast radius; removes the guard for cases it was written for. +2. **Per-provider opt-in.** A provider config flag marks a known-noisy upstream. Contained, + but requires the operator to know which providers need it. +3. **Subagent-scope only.** The reporter's actual case is a shadow/subagent call whose + system prompt describes capabilities the request's tool set does not include. Narrowest, + and arguably addresses the root cause on the PROMPT side rather than the response side. + +Option 3 suggests the defect may not be in the bridge at all: if a subagent request ships +a system prompt advertising `exec` while declaring a tool set without it, the request is +internally inconsistent before the provider ever answers. That is worth checking before +loosening a validator. + +## Missing evidence + +No live SenseNova/Kimi reproduction was available in this loop. Without one, two things +cannot be established: + +- whether the hallucination is provider-specific or a general small-model behavior under + a capability-describing system prompt; +- whether the subagent request actually advertises `exec` in its prompt while omitting it + from `tools` — which would make this a request-construction defect with a different fix. + +## Disposition + +**NEEDS_HUMAN.** The change is a deliberate loosening of a safety contract whose scope is +a product decision, and the evidence needed to choose the scope correctly is not available +without a live reproduction. Recorded here rather than guessed at; #1795 stays OPEN. +