diff --git a/devlog/_plan/260816_wave012_closeout/000_research.md b/devlog/_plan/260816_wave012_closeout/000_research.md new file mode 100644 index 0000000000..1a2531469b --- /dev/null +++ b/devlog/_plan/260816_wave012_closeout/000_research.md @@ -0,0 +1,69 @@ +# 000 — Wave 0/1/2 closeout research + +Source roadmap: external audit conversation (2026-08-16) that produced a six-wave plan over the open `bug` issue/PR set. This unit executes Wave 0, Wave 1 and Wave 2 only. Waves 3-5 stay out of scope. + +Baseline: `origin/dev` = `b81314cd29b78fecb447df882dc4fc1a987434b9`. + +## Wave scope as received + +Wave 0 (triage only, no issue closes): + +- `#1802` needs the `bug` label (currently `cli` only). +- `#92` / `#417` stay out of the release-blocker count as `upstream-tracking`. +- PR `#1822` label review (`bug` vs GUI UX follow-up). +- `#1049` records `#1798` / `#1802` as independent acceptance cases. + +Wave 1 (low-risk merges): `#1805`, `#1806` then close `#1786`, `#1741`, `#1825` then close/reclassify `#1824`, absorb `#1801` tests into `#1817` then close `#1801`. + +Wave 2 (blocker work): `#1819` to `#1785`, `#1788` to `#1700`, `#1780` to `#1767`, `#1792` to `#1668`, `#1703` to `#1697`. + +## Verified triage state (gh, 2026-08-16) + +| Item | State | Labels | +|---|---|---| +| `#1802` | OPEN | `cli` | +| `#92` | OPEN | `bug`, `upstream-tracking`, `tools` | +| `#417` | OPEN | `bug`, `upstream-tracking`, `cli` | +| `#1049` | OPEN | `bug`, `cli` | +| `#1798` | OPEN | `bug`, `cli` | +| PR `#1822` | OPEN | `bug` | + +`#92` and `#417` already carry `upstream-tracking`, so Wave 0 for them is a reporting convention, not a label mutation. The repository label set already contains `upstream-tracking`, `provider-compatibility`, `needs-info`, `maintainer-sponsored` and `intake: hygiene-blocked`; no new label needs creating. + +Target issues, all OPEN at baseline: `#1786`, `#1824`, `#1785`, `#1700`, `#1767`, `#1668`, `#1697`. `#1795` stays open (live SenseNova/Kimi reproduction still missing). + +## Audit findings per PR (5 parallel read-only audits) + +`#1805` — head `8946a1026120c59870310575c1623873a3660f4f`. Windows test-sandbox profile shape in `scripts/test.ts` plus two Windows-only regressions in `tests/test-runner.test.ts`. No diff-level blocker; only `REVIEW_REQUIRED` governance. + +`#1806` — head `960013e312970494e6eb7a3209b33491627f4316`. Makes `currentCodexSqliteHomeAbsolute()` target-aware in `src/service.ts:112` so POSIX artifacts keep `/var/...` literals on a Windows host. Body says `Closes #1786`. Live blocker: `tests/service.test.ts:692` ("still absolutizes a relative sqlite home") never asserts absoluteness — it only rejects one exact raw string, so it stays green for an arbitrary non-absolute transform. + +`#1741` — head `9118aef00e537f2abb09a3de3445f0cfd6b9bb18`. Request-local map for tool-call name recovery in `src/chat/inbound.ts` plus a 1,000-call linearity regression. No diff blocker; exact-head Cross-platform CI and React Doctor are `action_required` (never executed). + +`#1825` — head `65dac3fe651ea091f4ccf924b6691929002789fc`. Classifies malformed provider tool calls as `502` / `upstream_error` and adds bounded, debug-gated structural fingerprints. Exact-head Cross-platform CI green. No diff blocker. History note: commit `c8d136c21` temporarily added a `contents: write` workflow that `2e28fb2cc` removed; the final aggregate diff has no `.github/` change. + +`#1817` — head `b8cea1d0d2f33a5bce8277425461ab245efeda28`. Shared tool-catalog nudge contract plus `apply_patch`-scoped envelope guidance. Exact-head CI green. `src/bridge.ts` is untouched (identical blob), so fail-closed 502 enforcement is preserved. No diff blocker. + +`#1801` — head `f1db2592b398b5a49ec875c75c140a981542356f`, draft. Cursor Code Mode guidance with a real correctness blocker at `src/adapters/cursor/tool-definitions.ts:217`: `cursorRequestUsesCodeMode()` returns true whenever freeform `exec` is visible without a bare shell bridge, even when another non-shell top-level tool is also visible, and the guidance then declares those tools non-top-level. Its three tests are unique coverage, not duplicates of `#1817`. + +`#1819` — head `65822b008740af75cd2568595cb60602de80d899`, draft. All four claimed blockers verified at head: + +1. `configDiagnosticsFromRaw` still returns `getDefaultConfig()` with `source: "fallback"` (`src/config.ts:2479`); the salvage added to `loadConfig` (`src/config.ts:2175-2195`) has no diagnostics counterpart, and the file comment says that result can be persisted over providers/keys. +2. Salvage is one-pass (`src/config.ts:2179-2198`): dropping an invalid combo can invalidate a routing profile that referenced it, and the second parse failure discards the whole config. +3. Raw operator-controlled entry IDs reach `console.error` (`src/config.ts:3505-3523`) without `redactSecretString`. +4. No management-auth regression accompanies the new fallback path. + +`#1788` — head `cfbb3cdec5bc85154bfe8a672d9e34f9acceb2f6`, draft. No-catalog passthrough, caller/outbound catalog union and non-streaming state exclusion are correct. Remaining blocker: `rememberPassthroughResponseChecked` (`src/server/responses/core.ts:2311-2320`) only inspects the terminal response `output`, so a stream that emits an undeclared `apply_patch` in `response.output_item.added` and then a `response.completed` with `output: []` can still enter continuation state after the client received `response.failed`. + +`#1780` — head `388c4c25174ff5a709c5e1ca15ef625b55a6e9e8`. Three verified defects: `?? rawId` restores empty IDs (`src/adapters/anthropic.ts:578` and `:649`); `requiredIds` holds normalized IDs but the result scan compares raw ones (`src/adapters/anthropic.ts:660-667`), so every rewritten pair produces an orphan plus a synthetic missing result; and the stateless transform is not injective (`src/adapters/tool-call-id.ts:6,20`). + +`#1792` — head `8f41eb78e9a751fadb9a9043a7ed3e9e35d9c956`, draft, `CHANGES_REQUESTED`. The loader rejects `null` (`src/config.ts:738`) while the management validator accepts it (`src/config.ts:917`) and POST persists the provider as submitted (`src/server/management/provider-routes.ts:475,547`); PATCH already deletes the key. It also touches `src/server/auth-cors.ts`, which is on the restricted list in `.github/scripts/pr-sponsored-surface.cjs`, so PR hygiene fails with `unsponsored_surface` and the PR carries `intake: hygiene-blocked`. + +`#1703` — head `52cdbfc81adef7e3cc17a5e43a1b17428661fffc`, draft. Affinity comes from static `claudeCode.model` (`src/claude/inbound.ts:75`), classifier detection is slug-only (`:28`), and `src/router.ts:699` picks the first enabled Anthropic-adapter provider by object insertion order with no model-availability check. That is the silent provider/privacy/billing crossing the issue warns about; the PR is on design hold. + +## Cross-cutting facts + +- Contributor branches show `action_required` CI: workflow runs need maintainer authorization before any exact-head test matrix exists. `gh pr checks --required` reporting "no required checks" is not evidence of a green suite. +- Full-suite validation runs on `ssh lidge` (`/home/lidgeai/Developer/opencodex`, `bun test --isolate tests`), never on the local workstation. +- `main` promotion, tags and npm publish are out of scope. Issues and PRs are closed under maintainer authority once the fix is on `dev`, with a release-pending note. + diff --git a/devlog/_plan/260816_wave012_closeout/010_wave0_triage.md b/devlog/_plan/260816_wave012_closeout/010_wave0_triage.md new file mode 100644 index 0000000000..ab041bd2a3 --- /dev/null +++ b/devlog/_plan/260816_wave012_closeout/010_wave0_triage.md @@ -0,0 +1,23 @@ +# 010 — Wave 0: triage mutations + +Status: **executed 2026-08-16**. Every action below is already reflected on GitHub; the section is retained as the record. Re-running it would duplicate external communication, so treat each step as complete and verify state instead of re-posting. + +No issue is closed in this phase. GitHub metadata only; no repository code. + +## Actions + +1. `#1802` — add label `bug`. It currently has `cli` only, yet its title and body describe a real regression (`ocx sync` overwriting a hand-edited config from stale server memory). Keeps the release-blocker count honest at 27 rather than 26. +2. `#92`, `#417` — already labelled `upstream-tracking`. No mutation; record in this unit that they are excluded from the release-blocker statistic, and post one short comment on each confirming they remain upstream trackers and are not counted as ocx release blockers. +3. PR `#1822` — `bug` label review. The PR clarifies Log Guard storage UX and adds a write-load poster; that is GUI UX follow-up, not a defect fix. Replace `bug` with `gui`. +4. `#1049` — post a comment linking `#1798` and `#1802` as independent acceptance cases under the write-coordinator umbrella, stating explicitly that closing `#1049` does not close either. + +## Evidence (captured) + +- `gh issue view 1802 --json labels` → `bug,cli`. +- `gh pr view 1822 --json labels` → `gui`. +- `#1049` comment `5307288259`, `#92` comment `5307288327`, `#417` comment `5307288395`. + +## Non-goals + +- Do not close `#92`, `#417`, `#1049`, `#1798`, `#1802`. +- Do not retitle anything. diff --git a/devlog/_plan/260816_wave012_closeout/020_wave1_1805_1806_1786.md b/devlog/_plan/260816_wave012_closeout/020_wave1_1805_1806_1786.md new file mode 100644 index 0000000000..0b0e85c242 --- /dev/null +++ b/devlog/_plan/260816_wave012_closeout/020_wave1_1805_1806_1786.md @@ -0,0 +1,46 @@ +# 020 — Wave 1: #1805, #1806, close #1786 + +## 020.1 #1805 (no code change needed) + +Head `8946a1026120c59870310575c1623873a3660f4f`. `scripts/test.ts` creates `AppData/Local` and `AppData/Roaming` inside the disposable sandbox root before exporting `USERPROFILE`; `tests/test-runner.test.ts` gains two Windows-only regressions, one of which drives the actual PowerShell/.NET known-folder expression. + +Disposition: merge as-is. Production runtime untouched. + +## 020.2 #1806 blocker fix + +Head `960013e312970494e6eb7a3209b33491627f4316`. `src/service.ts` change is correct: `currentCodexSqliteHomeAbsolute()` becomes target-aware and uses `path.posix` for native/POSIX targets, so `/var/lib/opencodex/codex-sqlite` survives generation on a Windows host. + +Blocker is test-only, at `tests/service.test.ts` in the case named "still absolutizes a relative sqlite home". With input `relative-sqlite-home` it asserts only `plist` contains `CODEX_SQLITE_HOME` and does not contain `relative-sqlite-home`. That stays green for any non-empty transformed value, absolute or not. + +The two artifact formats are different and must be asserted separately — launchd emits XML (`KV`), systemd emits `Environment="K=V"`. A single `K=` line scan finds nothing in the plist and keeps a trailing quote in the unit. + +Required diff (test file only): + +```diff + const plist = buildPlist(); +- expect(plist).toContain("CODEX_SQLITE_HOME"); +- expect(plist).not.toContain("relative-sqlite-home"); ++ const plistValue = /CODEX_SQLITE_HOME<\/key>\s*([^<]*)<\/string>/.exec(plist)?.[1]; ++ expect(plistValue).toBeDefined(); ++ expect(isAbsolute(plistValue!) || posix.isAbsolute(plistValue!) || win32.isAbsolute(plistValue!)).toBe(true); ++ expect(plistValue!.endsWith("relative-sqlite-home")).toBe(true); ++ ++ const unit = buildUnit(); ++ const unitValue = /Environment="CODEX_SQLITE_HOME=([^"]*)"/.exec(unit)?.[1]; ++ expect(unitValue).toBeDefined(); ++ expect(isAbsolute(unitValue!) || posix.isAbsolute(unitValue!) || win32.isAbsolute(unitValue!)).toBe(true); ++ expect(unitValue!.endsWith("relative-sqlite-home")).toBe(true); +``` + +Import `isAbsolute` plus `posix` and `win32` from `node:path` at the top of the test file. The absolute check accepts either separator family because this test runs on both POSIX hosts and native Windows. + +## 020.3 Merge and close + +Both PRs change Windows-specific behavior, so a Linux/macOS run — including the `ssh lidge` suite — is not equivalent evidence. Native Windows CI must run at the exact head. + +1. Authorize the pending workflow runs so Cross-platform CI (including the Windows shards) executes at each exact head. +2. Push the strengthened oracle onto the `#1806` head branch, then re-authorize CI for the new head. +3. Record maintainer approval on each PR before merging; do not merge on `action_required` CI. +4. Merge `#1805`, then `#1806`. +5. Focused evidence: `bun test tests/service.test.ts tests/test-runner.test.ts` plus the Windows shard result at the merged head. +6. Close `#1786` with a comment naming the merged commit and stating release is pending. diff --git a/devlog/_plan/260816_wave012_closeout/030_wave1_1741_1825_1824.md b/devlog/_plan/260816_wave012_closeout/030_wave1_1741_1825_1824.md new file mode 100644 index 0000000000..296bfcd54a --- /dev/null +++ b/devlog/_plan/260816_wave012_closeout/030_wave1_1741_1825_1824.md @@ -0,0 +1,23 @@ +# 030 — Wave 1: #1741, #1825, close/reclassify #1824 + +## 030.1 #1741 + +Head `9118aef00e537f2abb09a3de3445f0cfd6b9bb18`. `toolCallsToItems` now takes one request-local `Map` instead of rescanning previously translated items, turning quadratic name recovery into linear. Equivalence holds because every emitted function call is indexed immediately before `input.push`. + +No diff blocker. The only gap is that exact-head Cross-platform CI is `action_required`. Substitute evidence: focused run of `tests/chat-completions-endpoint.test.ts` plus the `ssh lidge` full suite after the merge lands on `dev`. + +## 030.2 #1825 + +Head `65dac3fe651ea091f4ccf924b6691929002789fc`, exact-head Cross-platform CI green (shards 1-4, gates, macOS, keyring, npm-global). Reclassifies malformed provider tool calls to `502` / `upstream_error`, adds allowlisted structural fingerprints gated behind `isDebugEnabled()`, and keeps both stream and buffered rejection fail-closed. + +Security note for the record: the `.github/workflows/ocx-temp-apply-invalid-tool-call-fix.yml` file with `contents: write` existed only in intermediate commit `c8d136c21` and was removed by `2e28fb2cc`. The aggregate diff at head touches no workflow, so the restricted-surface concern does not apply to what lands. + +## 030.3 #1824 disposition + +`#1825` is mitigation plus diagnostics, not an upstream fix. It does not coerce, normalize or dispatch the malformed name, and it does not change what OpenCode Free / DeepSeek emit. + +Closing comment text: + +> Closing the OpenCodex-side scope via #1825. OpenCodex continues to reject object-valued function names without guessing executable intent, now classifies the malformed provider response as `502` / `upstream_error`, and exposes only bounded value-free diagnostics with optional privacy-safe structural fingerprints. This is a mitigation and diagnostics improvement; it does not normalize the provider payload or claim the upstream defect is fixed. + +Also add `provider-compatibility` so the remaining upstream defect stays discoverable. diff --git a/devlog/_plan/260816_wave012_closeout/040_wave1_1817_1801.md b/devlog/_plan/260816_wave012_closeout/040_wave1_1817_1801.md new file mode 100644 index 0000000000..cba9e8cc5c --- /dev/null +++ b/devlog/_plan/260816_wave012_closeout/040_wave1_1817_1801.md @@ -0,0 +1,30 @@ +# 040 — Wave 1: absorb #1801 into #1817, close #1801 + +## 040.1 What each PR is + +`#1817` (head `b8cea1d0d2f33a5bce8277425461ab245efeda28`, CI green) changes the shared surfaces: `src/adapters/tool-catalog-nudge.ts` gains three guidance sentences defining the complete top-level surface and routing nested helpers through their listed parent; `src/responses/parser.ts` scopes patch-envelope argument guidance to logical `apply_patch` only. `src/bridge.ts` is untouched, so fail-closed 502 enforcement survives. + +`#1801` (head `f1db2592b398b5a49ec875c75c140a981542356f`, draft) changes only `src/adapters/cursor/tool-definitions.ts` and its test. They are not diff duplicates; they are the same intent on two surfaces. + +## 040.2 The #1801 correctness blocker + +`cursorRequestUsesCodeMode()` (`src/adapters/cursor/tool-definitions.ts:217-223`) returns true when freeform `exec` is visible and no bare shell bridge exists — without requiring `exec` to be the only visible tool. The guidance block (`:608-615`) then says shell, file edits and MCP are nested helpers and "they are not separate top-level tools", contradicting the catalog prefix at `:603-604` that lists them as callable. + +The existing test at `tests/cursor-tool-definitions.test.ts:425-437` hides `exec` via `toolChoice`, so the mixed visible-catalog case is untested. + +## 040.3 Absorption plan + +Land, on top of `#1817` as it merges to `dev`: + +1. Port `isCursorCodeModeExecTool()` and `cursorRequestUsesCodeMode()` from `#1801`, with the fix: Code Mode nested-helper wording must be scoped to helpers described by `exec` itself, and every separately listed top-level tool must stay described as callable. Concretely, compute the set of visible non-`exec` top-level tools and, when non-empty, emit the nested-helper paragraph without the blanket "they are not separate top-level tools" sentence, naming the still-callable top-level tools instead. +2. Port the three unique tests from `tests/cursor-tool-definitions.test.ts` in `#1801`: + - "detects code mode only when freeform exec has no bare shell bridge" (freeform/bare-bridge mutual exclusivity) + - "teaches the nested-helper contract instead of a top-level shell bridge" (V8 isolate, `await tools.(...)`, `text(...)`, no `require`) + - "keeps flat-catalog shell-bridge guidance when a bare bridge is advertised" (opposite branch preserved) +3. Add the missing mixed-catalog regression: visible `exec` plus a visible `mcp__fs__read_file` under automatic tool choice must not tell the model that `mcp__fs__read_file` is non-top-level. + +## 040.4 Close-out + +- Merge `#1817` into `dev`, then land the Cursor absorption commit on `dev`. +- Close `#1801` as superseded by `#1817` plus the absorption commit, listing the three ported tests and the added mixed-catalog case. +- Keep `#1795` OPEN: no live SenseNova/Kimi reproduction has been run against the guidance change. diff --git a/devlog/_plan/260816_wave012_closeout/050_wave2_1819_1785.md b/devlog/_plan/260816_wave012_closeout/050_wave2_1819_1785.md new file mode 100644 index 0000000000..36934fa405 --- /dev/null +++ b/devlog/_plan/260816_wave012_closeout/050_wave2_1819_1785.md @@ -0,0 +1,103 @@ +# 050 — Wave 2: #1819 config salvage, close #1785 + +Head `65822b008740af75cd2568595cb60602de80d899` (draft). Direction is right; four blockers must clear before it can close `#1785`. + +## 050.1 Shared iterative salvage helper + +Replace the single-pass block in `loadConfig` (`src/config.ts:2175-2198`) with a helper both `loadConfig` and `configDiagnosticsFromRaw` call: + +```ts +type SalvageOutcome = { + candidate: unknown; // merged candidate minus the dropped entries + rawCandidate: unknown; // raw parsed document minus the SAME entries + parsed: OcxConfig; + dropped: string[]; + issues: z.ZodIssue[]; // accumulated across every pass +}; + +function salvageConfigCandidate( + merged: unknown, + initialError: z.ZodError, + rawDocument?: unknown, +): SalvageOutcome | null { + let candidate = merged; + let rawCandidate = rawDocument; + let error = initialError; + const dropped: string[] = []; + const issues: z.ZodIssue[] = []; + const budget = countSalvageableEntries(merged); + for (let i = 0; i <= budget; i++) { + const step = dropInvalidConfigSections(candidate, error); + if (!step || step.dropped.length === 0) return null; + dropped.push(...step.dropped); + issues.push(...error.issues); + candidate = step.candidate; + rawCandidate = deleteEntryPaths(rawCandidate, step.droppedPaths); + const result = configSchema.safeParse(candidate); + if (result.success) return { candidate, rawCandidate, parsed: result.data, dropped, issues }; + error = result.error; + } + return null; +} +``` + +`dropInvalidConfigSections` gains a `droppedPaths: string[][]` field beside its existing `dropped` labels so the raw document can be pruned by the same paths. `deleteEntryPaths` is a small pure helper that walks each path and deletes the leaf when the container is a plain object; it returns the input unchanged when `rawCandidate` is `undefined`, which keeps `loadConfig` (which has no separate raw document) on the two-argument call. + +`countSalvageableEntries` counts combo plus routing-profile entries so the loop is bounded by construction, and a pass that removes nothing terminates immediately. This is what makes the combo-then-dependent-profile cascade recoverable instead of discarding the config. + +## 050.2 Diagnostics parity + +`configDiagnosticsFromRaw` currently falls to `getDefaultConfig()` with `source: "fallback"` at `src/config.ts:2479`, and its own comment (`:2464-2466`) warns that this result can be written back over the operator's providers and keys. Insert the salvage call after the merged retry and before that return: + +The existing code deliberately validates a *merged* candidate (raw plus injected defaults) while passing the *original raw* document into `validFileConfigDiagnostics`, so that absent or malformed optional settings stay distinguishable from operator-supplied ones. Handing the merged candidate to diagnostics would make injected defaults look operator-supplied and can suppress degradation warnings. Salvage must therefore carry two candidates and apply the same deletions to both: + +In `configDiagnosticsFromRaw(raw: string)` the parameter `raw` is the JSON **text**; the parsed document is the local `parsed`, which is what the success path already passes as the second argument to `validFileConfigDiagnostics` (`src/config.ts:2455`). The salvage call must thread `parsed`, not `raw`: + +```diff + const retryResult = configSchema.safeParse(merged); + if (retryResult.success) { + return validFileConfigDiagnostics(normalizeApiKeyIds(retryResult.data as OcxConfig), parsed); + } ++ const salvaged = salvageConfigCandidate(merged, retryResult.error, parsed); ++ if (salvaged) { ++ warnDroppedConfigSections(getConfigPath(), salvaged.dropped, salvaged.issues); ++ return validFileConfigDiagnostics( ++ normalizeApiKeyIds(salvaged.parsed), // validated, from the merged candidate ++ salvaged.rawCandidate, // raw document minus the same entries ++ ); ++ } + return { config: getDefaultConfig(), source: "fallback", error: schemaDiagnosticsError(result.error) }; +``` + +`warnDroppedConfigSections` takes `z.ZodIssue[]` rather than a single `ZodError` so it can explain the whole cascade. Update its signature and the `loadConfig` call site together. + +Note the path argument: `loadConfig` has a local `configPath`, but `configDiagnosticsFromRaw(raw: string)` does not — it must call `getConfigPath()` (already used by `readConfigFileSnapshot`) or receive the path explicitly. Copying `configPath` into the diagnostics path would not compile. + +`salvageConfigCandidate` takes the optional raw document, deletes each dropped entry path from both candidates in lockstep, and returns the accumulated `errors` from every pass so the warning explains the whole cascade rather than only the first failure. + +Fallback remains only for invalid JSON, non-salvageable issues, or a salvage that made no progress. + +## 050.3 Redaction at the logging boundary + +`dropInvalidConfigSections` (`src/config.ts:3505-3507`) keeps raw keys and `warnDroppedConfigSections` (`:3517-3523`) serializes `issue.path` straight into `console.error`. Operator-controlled combo/profile IDs can be token-shaped, and this file already imports `redactSecretString` for provider names. + +Keep raw IDs internally for deletion. At the log boundary, map each dynamic path component through `redactSecretString`, keep static section names (`routingProfiles`, `combos`) verbatim, and never serialize the raw joined path first. + +Redacting `issue.path` and `dropped` is not sufficient on its own: `routingProfileIssues()` and `comboConfigIssues()` interpolate operator-controlled provider/combo IDs into the validation *message* text. Run those messages through the same redaction before they reach `console.error`, or build them from redacted components in the first place. + +The current test at `tests/config.test.ts:1614-1616` asserts the raw ID appears — update it to assert the static section plus the redacted marker, and add a case where the token-shaped ID appears inside an interpolated validation message. + +## 050.4 Management-auth regression + +Add to `tests/server-management-auth.test.ts`: start the server on a config that has one salvageable invalid routing profile and distinct data-plane/admin credentials, then assert `/api/config` returns 401 anonymously, 401 for the data-plane token, 401 for a wrong admin token, 200 for the valid admin token, and that the authenticated body still contains the unrelated providers with only the invalid profile missing. Add an unsalvageable control asserting the fallback path does not leave the management plane unauthenticated. + +## 050.5 New tests in `tests/config.test.ts` + +- `readConfigDiagnostics()` with an invalid profile: `source === "file"`, `error === null`, providers/API keys/model costs preserved. +- `readConfigDiagnostics()` with an invalid combo: same assertions. +- Cascade: invalid combo plus a profile referencing it plus unrelated valid entries — both invalid entries dropped, everything else preserved. +- Token-shaped combo ID: captured `console.error` contains the redacted marker and not the raw token. + +## 050.6 Close-out + +Focused evidence: `bun test tests/config.test.ts tests/server-management-auth.test.ts`. Merge, then close `#1785` naming the salvage semantics (invalid entries dropped, everything else preserved, diagnostics no longer collapses to defaults). diff --git a/devlog/_plan/260816_wave012_closeout/060_wave2_1788_1700.md b/devlog/_plan/260816_wave012_closeout/060_wave2_1788_1700.md new file mode 100644 index 0000000000..218ba4fde7 --- /dev/null +++ b/devlog/_plan/260816_wave012_closeout/060_wave2_1788_1700.md @@ -0,0 +1,91 @@ +# 060 — Wave 2: #1788 undeclared-tool guard, close #1700 + +Head `cfbb3cdec5bc85154bfe8a672d9e34f9acceb2f6` (draft). Three of the four close conditions already hold. + +## 060.1 Verified good + +- No-catalog passthrough: guard activates only when `declaredWireToolNames.size > 0` (`src/server/responses/core.ts:2305-2306`); both omitted `tools` and `tools: []` are tested on both transports. +- Hosted-tool rewrite union: outbound wire names plus `toolBridgeMaps.declaredToolNames` are merged before transport branching (`:2298-2304`), proven by the `image_generation` / `image_gen.generate` regression. +- Non-streaming rejected turns do not enter continuation state: validation at `:2917-2932` precedes the cache write at `:2933-2938`. + +## 060.2 Remaining blocker: streaming continuation state + +`rememberPassthroughResponseChecked` (`:2311-2320`) inspects only the terminal response's `output`. A stream that emits an undeclared `apply_patch` in `response.output_item.added` and then terminates with `response.completed` carrying `output: []` gives the client `response.failed` while the inspection side sees nothing undeclared and can still remember the turn. The PR's own unit fixture (`tests/responses-undeclared-tool-guard.test.ts:148-160`) constructs exactly that stream. + +Required change: make rejection sticky per turn rather than terminal-snapshot-derived. + +```diff ++ let inspectionSawUndeclaredTool = false; ++ const noteInspectedPayload = (payload: unknown) => { ++ if (!undeclaredToolGuardActive || inspectionSawUndeclaredTool) return; ++ if (undeclaredToolCallName(payload, declaredWireToolNames) !== undefined) { ++ inspectionSawUndeclaredTool = true; ++ } ++ }; + const rememberPassthroughResponseChecked = (response: unknown) => { +- if (undeclaredToolCallNameInResponse(response, declaredWireToolNames) !== undefined) return; ++ if (inspectionSawUndeclaredTool) return; ++ if (undeclaredToolCallNameInResponse(response, declaredWireToolNames) !== undefined) return; + rememberPassthroughResponse(response); + }; +``` + +The `undeclaredToolGuardActive` gate is load-bearing, not defensive: it is the same condition computed at `src/server/responses/core.ts:2305-2306` (non-empty catalog and non-forward auth). Without it, a no-catalog or `authMode: "forward"` stream would mark every tool call undeclared and silently stop persisting continuation state — breaking the passthrough contract that 060.1 verifies. Reuse the existing flag; do not recompute the condition. + +### Required seam in `src/server/relay.ts` + +`SseInspectorHandlers` (`src/server/relay.ts:602-613`) exposes `onTerminal`, `onCompletedResponse`, `onFirstOutput` and `pinCompletedResponseIdToFirstSeen` — there is **no per-payload callback**, so without an API change `inspectionSawUndeclaredTool` is never set and the fix is inert. Add one: + +```diff + export type SseInspectorHandlers = { + onTerminal?: (status: ResponsesTerminalStatus, httpStatusOverride?: number) => void; + logCtx?: RequestLogContext; + onCompletedResponse?: (response: { id?: unknown; output?: unknown; status?: unknown }) => void; ++ /** Every parsed SSE payload, delivered before onCompletedResponse for the same turn. */ ++ onParsedPayload?: (payload: unknown) => void; + onFirstOutput?: () => void; + ``` + +Ordering is the load-bearing property: the inspector must invoke `onParsedPayload` for a payload before invoking `onCompletedResponse` derived from that same payload. Otherwise the terminal callback can remember the turn before the flag is set. Invoke it from `scanPayload`, at the top, before any terminal classification. + +### Second seam: the tee consumers + +`consumeForInspection` (`src/server/relay.ts:1095`) builds its inspector from a fixed positional list — `onTerminal`, `logCtx`, `onCompletedResponse`, `onFirstOutput`, `pinCompletedResponseIdToFirstSeen` — so adding the handler to `SseInspectorHandlers` alone leaves the tee path unwired. Do not add a ninth positional parameter; carry it on the existing options object instead: + +```diff + export type InspectionConsumerOptions = { + ... ++ onParsedPayload?: (payload: unknown) => void; + }; + + const inspector = (options?.inspectorFactory ?? createSseInspector)({ + onTerminal, + logCtx, + onCompletedResponse, + onFirstOutput, ++ onParsedPayload: options?.onParsedPayload, + pinCompletedResponseIdToFirstSeen: options?.pinCompletedResponseIdToFirstSeen, + }); +``` + +Apply the same addition to `consumeForResponseLogMetadata` if it constructs its own inspector. + +Then call `noteInspectedPayload` through `onParsedPayload` for every parsed payload — `response.output_item.added`, `response.output_item.done`, `response.completed`, `response.incomplete` — on both the eager-inspector path (`src/server/responses/core.ts:2774-2778`) and the tee path (`:2854-2873`). + +Add regressions at both levels. Inspector level: a stream whose only undeclared item arrives in `output_item.added` delivers that payload to `onParsedPayload` strictly before `onCompletedResponse` fires. Wiring level: driving the same stream through `consumeForInspection` with an `onParsedPayload` option actually invokes it — a direct `createSseInspector` test would pass even while the tee path stayed unwired, which is exactly the failure mode this seam exists to prevent. + +## 060.3 Regression + +Add a streaming end-to-end case in `tests/responses-undeclared-tool-guard.test.ts`: undeclared `apply_patch` in `output_item.added`, terminal `response.completed` with `output: []`, assert the client saw `response.failed` and that `expandPreviousResponseInput(responseId)` retains only the follow-up input. The existing `turn()` helper hardcodes `stream: false` (`:365-381`); add a streaming variant rather than mutating it. + +Also correct the stale comment at `:104-108` ("a request declaring no tools authorizes none"), which contradicts the implemented and tested passthrough contract. + +## 060.4 Review-thread dispositions + +- `r3789663344` (reject on readable-empty catalog): rebut. It conflicts with the Copilot/DeepSeek passthrough contract and is contradicted by the intentional tests at `:419-495`. +- `r3789663346` (do not record a completed SSE response after the guard trips): accept — that is 060.2. +- `r3789663350` (validate JSON before storing): already fixed at head; resolve as addressed. + +## 060.5 Close-out + +Focused evidence: `bun test tests/responses-undeclared-tool-guard.test.ts tests/responses-custom-tool-repair.test.ts`. Merge, then close `#1700` naming fail-closed behavior on both transports and the continuation-state exclusion. diff --git a/devlog/_plan/260816_wave012_closeout/070_wave2_1780_1767.md b/devlog/_plan/260816_wave012_closeout/070_wave2_1780_1767.md new file mode 100644 index 0000000000..fa9fc870d4 --- /dev/null +++ b/devlog/_plan/260816_wave012_closeout/070_wave2_1780_1767.md @@ -0,0 +1,95 @@ +# 070 — Wave 2: #1780 tool-call ID normalization, close #1767 + +Head `388c4c25174ff5a709c5e1ca15ef625b55a6e9e8`. Direction correct, three defects to clear. + +## 070.1 Defects + +1. Empty-ID restoration: `anthropicToolCallId(x) ?? x` at `src/adapters/anthropic.ts:578` and `:649` puts `""` back on the wire even though the helper deliberately returns `undefined` for it (`src/adapters/tool-call-id.ts:17`). +2. Mixed-domain matching: `requiredIds` holds normalized IDs (`src/adapters/anthropic.ts:648-650`) while the result scan compares raw `tr.toolCallId` (`:660-667`). Every rewritten pair therefore loses its result to orphan text and gains a synthetic missing-result block. +3. Non-injective transform: `call:a` maps to `call_a_`, and a distinct raw ID already equal to `call_a_` passes through unchanged; the 8-hex suffix also allows ordinary 32-bit collisions (`src/adapters/tool-call-id.ts:6,20`). + +## 070.2 Request-scoped allocator + +Replace the stateless helper's role (keep the pure transform as the candidate generator) with an allocator created once per `buildRequest`: + +```ts +export function createToolCallIdAllocator() { + const rawToWire = new Map(); + const occupied = new Set(); + return { + reserve(rawId: string) { // pre-pass over already-conforming ids + // Conforming includes the length bound: an over-max id made only of allowed + // characters must still be rewritten, not reserved verbatim. + if (isConformingToolCallId(rawId)) { rawToWire.set(rawId, rawId); occupied.add(rawId); } + }, + allocate(rawId: string): string | undefined { + const existing = rawToWire.get(rawId); + if (existing) return existing; + const base = toolCallIdComponents(rawId); // { cleaned, hash } | undefined + if (base === undefined) return undefined; // empty/unusable: caller decides + const { cleaned, hash } = base; // components, not a finished string + let candidate = fitToolCallId(cleaned, hash); + for (let n = 2; occupied.has(candidate); n++) { + const suffix = `_${n}`; + candidate = fitToolCallId(cleaned, hash, suffix.length) + suffix; + } + rawToWire.set(rawId, candidate); + occupied.add(candidate); + return candidate; + }, + lookup(rawId: string) { return rawToWire.get(rawId); }, + }; +} +``` + +Reserve every already-conforming source ID before allocating any rewritten one, so a rewritten candidate can never squat on an ID another call legitimately owns. + +### Length bound + +Anthropic constrains tool-call ID length, and the issue names it explicitly. Two things follow. + +First, `isConformingToolCallId` must include the length condition — `id.length <= MAX_TOOL_CALL_ID_LENGTH` alongside the character-class check. Otherwise `reserve()` stores an over-max ID verbatim and the allocator hands back something the API rejects. + +Second, disambiguation must not push a valid candidate over the limit. The allocator therefore works on *components*, not a finished string: split the existing `anthropicToolCallId` into `toolCallIdComponents(rawId): { cleaned: string; hash: string } | undefined` (the sanitized prefix and the deterministic hash) plus a formatter, keeping `anthropicToolCallId` as a thin wrapper for existing callers. Build candidates as `truncatedCleaned + "_" + hash` (+ collision suffix), never as a plain slice of the finished string: + +```ts +const MAX_TOOL_CALL_ID_LENGTH = 64; // confirm against the Anthropic tool_use id constraint + // before implementing; record the source in this unit. +const TOOL_CALL_ID_HASH_WIDTH = 8; // hex chars, matching what the current helper emits + +function fitToolCallId(cleaned: string, hash: string, reserve = 0): string { + const tail = `_${hash}`; + const room = MAX_TOOL_CALL_ID_LENGTH - reserve - tail.length; + return cleaned.slice(0, Math.max(1, room)) + tail; +} +``` + +`hash` stays at `TOOL_CALL_ID_HASH_WIDTH` (8 hex chars, what the current helper already produces), so `tail` is 9 characters and a 64-character budget always leaves room for a non-empty prefix plus a collision suffix. The hash width is a constant of the format, not something scaled to the input — a hash wide enough to fill the budget would leave no prefix at all. + +The hash tail carries the discriminating bits, so it is preserved intact and only the cleaned prefix is truncated. Collision resolution passes `reserve = suffix.length` so the final string, suffix included, still fits. + +## 070.3 Adapter wiring + +- Build the allocator at the top of the Anthropic request construction and pre-reserve all source IDs. +- Calls: `const wireCallId = alloc.allocate(tc.id)`. When it is `undefined`, do not emit a `tool_use`; convert that call and any matching result to ordinary text (or throw a typed adapter error under the strict path). Never emit an empty `id`. +- Results: `const wireResultId = alloc.lookup(tr.toolCallId)`. Match `requiredIds`/`seen` on `wireResultId` only. A result with no allocation stays orphan text and must not mint a new tool-use identity. +- Synthetic missing-result blocks are emitted only for calls that received a valid allocation and genuinely have no result. +- `src/adapters/google.ts` shares the same allocator rather than aliasing the stateless helper; otherwise the collision defect persists there. + +## 070.4 Regressions + +Host them in `tests/adapter-usage.test.ts` under the existing `describe("anthropic tool result history repair")`, since the PR's helper-only test never exercises `buildRequest`: + +- invalid `call:a` call plus its result yields exactly one normalized pair, no orphan, no synthetic missing result; +- empty ID is textified (or rejected) and never appears as `tool_use.id` or `tool_result.tool_use_id`; +- invalid ID whose normalized candidate equals a distinct conforming ID present in the same history keeps both distinct; +- forced hash collision keeps both distinct; +- result-only ID allocates nothing; +- already-conforming pairs pass through byte-identical. +- a source ID at exactly `MAX_TOOL_CALL_ID_LENGTH` stays valid, and its collision variant also stays within the bound; +- an already-conforming-by-characters ID that exceeds `MAX_TOOL_CALL_ID_LENGTH` is rewritten rather than reserved, and its rewrite fits; +- the same boundary and collision cases hold for Google request construction, not only Anthropic. + +## 070.5 Close-out + +Focused evidence: `bun test tests/adapter-usage.test.ts tests/anthropic-tool-call-id.test.ts`. Merge, then close `#1767`. diff --git a/devlog/_plan/260816_wave012_closeout/080_wave2_1792_1668.md b/devlog/_plan/260816_wave012_closeout/080_wave2_1792_1668.md new file mode 100644 index 0000000000..7963e781ce --- /dev/null +++ b/devlog/_plan/260816_wave012_closeout/080_wave2_1792_1668.md @@ -0,0 +1,66 @@ +# 080 — Wave 2: #1792 upstreamHttpVersion, close #1668 + +Head `8f41eb78e9a751fadb9a9043a7ed3e9e35d9c956` (draft, CHANGES_REQUESTED). + +## 080.1 The contract mismatch + +The loader rejects `null`: + +```ts +// src/config.ts:738 +upstreamHttpVersion: z.enum(UPSTREAM_HTTP_VERSION_VALUES).optional(), +``` + +The management validator accepts it (`src/config.ts:917`: `if (value === undefined || value === null) return null;`), and POST persists the provider as submitted (`src/server/management/provider-routes.ts:475` then `:547`). So a POST body carrying `upstreamHttpVersion: null` is written to disk and the next `loadConfig()` cannot parse it, dropping the operator into invalid-config recovery. PATCH already deletes the key for null/empty (`:198`). + +## 080.2 Fix on both sides + +Canonicalize at the write boundary. There is no local `httpVersionError` in the POST handler: validation happens inside `providerManagementConfigError(name, body.provider)` (`src/server/management/provider-routes.ts:464`), which the PR extends in `src/server/auth-cors.ts:521`. The deletion therefore belongs immediately after that validation call succeeds, on the object that is about to be persisted: + +```diff + const providerError = providerManagementConfigError(name, body.provider); + if (providerError) return jsonResponse({ error: providerError }, 400); + const serviceTierError = providerServiceTierConfigError(name, body.provider); + if (serviceTierError) return jsonResponse({ error: serviceTierError }, 400); + const prov = body.provider ? stripCodexRuntimeProviderFields(body.provider as OcxProviderConfig) : undefined; ++ if (prov && (prov as Record).upstreamHttpVersion === null) { ++ delete (prov as Record).upstreamHttpVersion; ++ } +``` + +The handler returns via `jsonResponse({ error: ... }, 400)`, not a `jsonError` helper; and the deletion belongs on `prov` — the object that is actually persisted — after `stripCodexRuntimeProviderFields`. + +Scope it to POST only. The validator already rejects `""` (only `undefined`/`null` short-circuit at `src/config.ts:917`), so no empty-string arm is needed. PATCH and its locked replay already clear null/empty through `applyProviderPatchFields`, and the reload path at `:405` reads an already schema-normalized provider. POST is the one persistence hole; loader tolerance covers nulls already on disk. + +And make the loader tolerant so any already-persisted candidate still loads: + +```diff +- upstreamHttpVersion: z.enum(UPSTREAM_HTTP_VERSION_VALUES).optional(), ++ upstreamHttpVersion: z.enum(UPSTREAM_HTTP_VERSION_VALUES) ++ .nullish() ++ .transform((value) => value ?? undefined), +``` + +PATCH semantics stay as they are. + +## 080.3 Restricted surface + +`.github/scripts/pr-sponsored-surface.cjs` lists `src/server/auth-cors.ts` as a restricted exact file, and the gate requires either author push permission or the `maintainer-sponsored` label (`:69`). `#1792` touches it, so PR hygiene fails with `unsponsored_surface` and the PR carries `intake: hygiene-blocked`. + +Disposition: the `auth-cors.ts` hunk **is** load-bearing and cannot be dropped. That file owns `providerManagementConfigError()` and `safeConfigDTO()`; the PR adds the `upstreamHttpVersionConfigError` call into the former (`src/server/auth-cors.ts:521`) and the field into the latter. Removing it would leave the new option unvalidated on every management write path. + +So the correct path is explicit security review of that hunk as a validation-boundary change, then `maintainer-sponsored` applied and `intake: hygiene-blocked` removed. The review question is narrow: does the added validation call change any existing accept/reject outcome, and does the DTO addition expose anything beyond the enum value? Neither should touch credential material. + +## 080.4 Regressions + +In `tests/management-provider-validation.test.ts`: POST a provider with `upstreamHttpVersion: null`, assert 200, assert the live config has no such property, assert the serialized config on disk has none, assert a subsequent `loadConfig()` succeeds, and assert GET / `safeConfigDTO` omit the field. + +The other unresolved thread (test-only `fetch` typing) is already satisfied at head by the `TestProvider` intersection in `tests/upstream-http-version.test.ts:8-14`; resolve it without a code change. + +`upstreamHttpVersion` is a new operator-facing provider option, so `docs-site/` needs a short entry describing the values, the default, and when to pin HTTP/1.1. A user-facing option that exists only in code is a docs-sync defect under the review guidelines. + +Sequencing: `#1819` also edits `src/config.ts`. Land `#1819` first, then rebase `#1792` onto the new `dev` head and re-audit before requesting sponsorship and approval — otherwise the hygiene/CI evidence is against a stale base. + +## 080.5 Close-out + +Focused evidence: `bun test tests/upstream-http-version.test.ts tests/management-provider-validation.test.ts`. Merge, then close `#1668` describing per-provider `upstreamHttpVersion` as the operator-facing mitigation for the HTTP/2 SSE hang, and note that a long-SSE smoke against the affected provider is the remaining live confirmation. diff --git a/devlog/_plan/260816_wave012_closeout/090_wave2_1703_1697.md b/devlog/_plan/260816_wave012_closeout/090_wave2_1703_1697.md new file mode 100644 index 0000000000..e532fce471 --- /dev/null +++ b/devlog/_plan/260816_wave012_closeout/090_wave2_1703_1697.md @@ -0,0 +1,208 @@ +# 090 — Wave 2: #1703 classifier affinity redesign, close #1697 + +Head `52cdbfc81adef7e3cc17a5e43a1b17428661fffc` (draft, design hold). The current draft must not merge as-is. + +## 090.1 Why it cannot merge + +- Affinity is read from static `claudeCode.model` (`src/claude/inbound.ts:75`), which is the injected/default config slot, not the provider/model the live session actually selected. It goes stale the moment the user changes the model picker — the issue's primary failure. +- Classifier detection is slug-only (`:28`), so it cannot separate an Auto Mode side request from an ordinary user-selected bare Opus request. +- `src/router.ts:699` picks the first enabled provider whose adapter is `anthropic`/`anthropic-messages` by object insertion order, checking neither `provider.models`, nor `selectedModels`, nor `disabledModels`, nor discovery state. That is a silent provider/privacy/billing crossing. +- Once an affinity string exists, `classifierFallbacks` is never consulted, even if that provider is disabled or lacks the model. + +## 090.2 Target contract + +Keep `resolveInboundModel(model, cc)` a pure syntax/config override: marker stripping, aliases, Desktop aliases, exact and date-stripped `modelMap`. `modelMap` stays the highest-precedence terminal override. + +Add an availability-aware resolver at a layer that has full state: + +```ts +resolveClaudeClassifierRoute(options: { + requestedModel: string; + isClassifierRequest: boolean; + config: OcxConfig; + conversationId?: string; + affinity?: ClaudeSessionModelAffinity; + availableModels: readonly CatalogModel[]; +}): RouteResult; + +class NoAvailableClaudeClassifierRouteError extends Error { + constructor( + readonly requestedModel: string, + readonly rejections: readonly ClaudeClassifierCandidateRejection[], + ) { super(...); } +} +``` + +### The classifier discriminator + +Nothing in the model slug distinguishes an Auto Mode side request from a user-selected bare Opus request, and the issue explicitly asks for that separation. Define a concrete predicate over the request body — `isClaudeClassifierRequest(body)` — evaluated before any affinity is consumed. + +The observable shape is already captured in-repo by the #545 classifier fixture (`tests/anthropic-reasoning.test.ts:411-430`), which is the reporter's exact Claude Code permission-classifier body: + +```json +{ "model": "claude-sonnet-5", "max_tokens": 64, "stop_sequences": [""], + "thinking": { "type": "disabled" }, "system": "decide whether this tool call is allowed", + "messages": [{ "role": "user", "content": "ls" }] } +``` + +Bind the predicate to that shape as an explicit conjunction: + +There is no `AnthropicMessagesBody` type in `src/`; `anthropicToResponsesBody(raw: unknown, cc?)` (`src/claude/inbound.ts:424`) takes `unknown`. Declare a narrow local shape and validate structurally: + +```ts +type ClassifierProbe = { + tools?: unknown; + messages?: unknown; + max_tokens?: unknown; + thinking?: { type?: unknown }; + stop_sequences?: unknown; + system?: unknown; +}; + +export const CLASSIFIER_MAX_TOKENS_CEILING = 512; // named constant, not config + +export function isClaudeClassifierRequest(raw: unknown): boolean { + if (typeof raw !== "object" || raw === null) return false; + const body = raw as ClassifierProbe; + if (Array.isArray(body.tools) && body.tools.length > 0) return false; + if (!Array.isArray(body.messages) || body.messages.length !== 1) return false; + const only = body.messages[0] as { role?: unknown } | undefined; + if (only?.role !== "user") return false; // no assistant/tool history + if (typeof body.max_tokens !== "number" || body.max_tokens > CLASSIFIER_MAX_TOKENS_CEILING) return false; + if (body.thinking?.type !== "disabled") return false; + if (!Array.isArray(body.stop_sequences) || body.stop_sequences.length === 0) return false; + return matchesClassifierSystemMarker(body.system); // REQUIRED, not optional +} +``` + +The structural conditions alone are not a discriminator: an ordinary short single-turn request with thinking disabled and a stop sequence would match. The `system` marker is what actually identifies Auto Mode traffic, so `matchesClassifierSystemMarker` is a required conjunct rather than a nicety. Derive its exact form from the live capture below and keep it a normalized substring/prefix test over the system text, never a full-text comparison. Add explicit false-positive tests: a short user question with a stop sequence and disabled thinking, and a single-turn request whose system prompt is the ordinary Claude Code prompt, must both return false. + +The ceiling is a code constant with a recorded rationale (the observed budget is 64), not a new config knob — an operator-tunable ceiling would let a misconfiguration reclassify ordinary traffic. Record every matched/failed condition in the route decision so a misclassification is diagnosable from logs rather than guessed at. + +Before implementing, capture one live Claude Code Auto Mode classifier request against the affected version and diff it against this fixture. **Capture into `.tmp/` or a `mktemp -d` path only.** A request body can carry user commands, paths and identifiers, so the raw capture must never be written into `devlog/`, which is public and tracked. What lands in this unit is the redacted result: which fields were present, their types, the normalized marker form, and the pass/fail verdict — no body content. Route-decision telemetry follows the same rule: booleans and rejection codes, never prompt text. + +**Ambiguity is not simply "route as ordinary".** A bare `claude-opus-5` that fails the predicate must not fall through to an incompatible `defaultProvider` — that is #1697's original symptom arriving by another path. Ambiguous Claude-family requests get the same compatibility guard: the selected route must expose the requested model on a classifier-compatible adapter, or the request fails with the typed error. Failing closed means refusing an incompatible route, not borrowing affinity. + +Candidate precedence, explicit and tested. There are **two orders**, selected by `isClassifierRequest`. + +For a classifier request: + +1. exact / date-stripped `modelMap` override — terminal by operator intent. It is not availability-filtered, because an explicit operator mapping must not be silently overridden; if that target is unavailable the request fails with the typed error naming the mapping, rather than falling through to another provider; +2. live same-session provider affinity, only if that provider exposes the requested classifier model; +3. configured `classifierModel`; +4. ordered, user-approved `classifierFallbacks`; +5. typed failure. + +There is no implicit "first Anthropic adapter" candidate. + +For a **non-classifier** request (including an ordinary user-selected bare `claude-opus-5`), affinity is neither read nor written, and `classifierModel`/`classifierFallbacks` are not consulted at all: + +1. exact / date-stripped `modelMap` override; +2. ordinary configured routing for the requested model; +3. the same compatibility guard — the selected route must expose the requested model on a compatible adapter — otherwise the typed error, rather than a silent fall-through to an incompatible `defaultProvider`. + +Keeping the two orders textually separate is what makes the "ordinary requests never consume affinity" rule checkable; a single merged list with conditional steps is exactly where that rule gets lost. + +Every candidate is validated on three dimensions: the provider exists and is enabled; its effective adapter is classifier-compatible; and the exact provider/model pair exists in the canonical filtered catalog. + +`gatherRoutedModels(config)` returns the **unfiltered** routed set (`src/codex/catalog/provider-fetch.ts:1633` returns `models` directly). Visibility filtering is a separate step in `filterCatalogVisibleModels(models, config)` (`:1515`), which is what applies `disabledModels` and per-provider `selectedModels`. The resolver must therefore receive: + +```ts +const availableModels = filterCatalogVisibleModels(await gatherRoutedModels(config), config); +``` + +Passing the raw `gatherRoutedModels` output would leave disabled and unselected models eligible — the exact silent-crossing failure this unit exists to close. Add a test that a model excluded by `disabledModels` or omitted from a provider's `selectedModels` is rejected as a classifier candidate. + +## 090.3 Session affinity store + +Bounded, TTL-governed, keyed by the opaque ID from `conversationIdFromClaudeMetadata()` (`src/server/claude-messages.ts:641-644`). It stores the resolved `{providerName, modelId}` from a non-classifier main turn. Classifier turns read affinity and never write it, so a classifier call cannot pin the session to its own route. + +## 090.4 Call-site changes + +Translation accepts an already-resolved model instead of re-resolving: + +```ts +anthropicToResponsesTranslation(raw, { + claudeCode?: OcxClaudeCodeConfig; + resolvedModel?: string; +}); +``` + +Update `anthropicToResponsesBody`, `handleClaudeMessagesWithBudget`, the debug capture (`src/server/claude-messages.ts:625-632`), translation (`:656`) and route preflight (`:683`). The endpoint derives conversation identity, resolves the classifier route, then translates with the selected model — one resolution, not two under different rules. + +On `NoAvailableClaudeClassifierRouteError`, the Claude endpoint returns a stable Anthropic-shaped `invalid_request_error` with an actionable message and a machine code, rather than letting generic routing fall through to `defaultProvider`. + +## 090.5 Tests + +- `tests/claude-inbound.test.ts`: alias and `modelMap` precedence, resolved-model injection. +- `tests/claude-messages-endpoint.test.ts`: two requests sharing one `metadata.user_id`; a live main `RelayA/claude-fable-5` turn establishes affinity and the later classifier call routes to `RelayA/claude-opus-5`. +- Router/classifier: disabled affinity provider falls through to an approved fallback; incompatible adapter falls through; affinity provider lacking Opus falls through; explicit `modelMap` wins; `classifierModel` and each fallback require model availability; no candidate throws the typed error. +- An ordinary bare-Opus request that is not classifier-shaped must not consume classifier affinity. +- Canonical catalog filtering (`selectedModels`, `disabledModels`, discovery state) affects eligibility. +- The route decision records affinity source, per-candidate rejection reason and the selected provider/model. +- `isClaudeClassifierRequest` returns false for a user-selected bare Opus request that carries tools or history, and that request never consumes or writes affinity. + +### Management contract (diff level) + +`src/server/management/agent-settings-routes.ts` currently exposes neither field. Three edits: + +1. **GET projection** (`/api/claude-code`, near `modelMap` at `:1008`): + +```diff + modelMap: config.claudeCode?.modelMap ?? {}, ++ classifierModel: config.claudeCode?.classifierModel ?? "", ++ classifierFallbacks: config.claudeCode?.classifierFallbacks ?? [], +``` + +2. **PUT body whitelist** (`:1045`): add `classifierModel?: unknown; classifierFallbacks?: unknown` to the destructured type. + +3. **PUT handling**: `classifierModel` joins the existing `["model", "smallFastModel"]` string loop (trim, empty string clears). `classifierFallbacks` needs its own array validator mirroring the `modelMap` block: + +```ts +if (body.classifierFallbacks !== undefined) { + if (body.classifierFallbacks === null) delete next.classifierFallbacks; + else { + if (!Array.isArray(body.classifierFallbacks)) { + return jsonResponse({ error: "classifierFallbacks must be an array of strings, or null" }, 400); + } + const list = body.classifierFallbacks.map(v => (typeof v === "string" ? v.trim() : "")); + if (list.some(v => v === "")) { + return jsonResponse({ error: "classifierFallbacks entries must be non-empty strings" }, 400); + } + if (list.length > 0) next.classifierFallbacks = list; + else delete next.classifierFallbacks; + } +} +``` + +Persistence tests belong beside the existing claude-code settings tests: PUT then GET round-trips both fields, `null` clears them, a non-string entry is rejected with 400, and an untouched config is not mutated. The GUI stays config-only in this unit — the API is the contract, and a GUI surface is a separate follow-up rather than a hidden dependency of the routing fix. + +### File-config validation + +The PUT validator does not protect a hand-authored config: `configSchema` only requires `claudeCode` to be an object, and `normalizePersistedClaudeCode()` (`src/config.ts:1992`) normalizes only `subagentEffort`. A malformed `classifierFallbacks` — a bare string, or an array with non-string entries — would reach the resolver unchecked. + +Extend `normalizePersistedClaudeCode()` to coerce both fields: drop `classifierModel` unless it is a non-empty string after trimming; drop `classifierFallbacks` unless it is an array, filter it to non-empty trimmed strings, and drop the key when nothing survives. The resolver additionally treats a missing or empty fallback list as "no fallback candidates" instead of assuming shape. + +**Extending the helper is not enough — it must actually run.** Today `normalizePersistedClaudeCode()` is only reached through `normalizeClaudeSubagentEffort()` (`src/config.ts:2003`), which returns the config unchanged whenever `subagentEffort` is absent or already valid. A config whose only defect is a malformed `classifierFallbacks` therefore never gets normalized. + +Call Claude-config normalization unconditionally on both successful `loadConfig()` paths and on the diagnostics path, independently of the `subagentEffort` short-circuit: + +```diff +-function normalizeClaudeSubagentEffort(config: OcxConfig, rawParsed: unknown): OcxConfig { +- const rawEffort = rawClaudeSubagentEffort(rawParsed); +- if (rawEffort === undefined || isClaudeSubagentEffort(rawEffort)) return config; +- return { ...config, claudeCode: normalizePersistedClaudeCode(config.claudeCode) }; +-} ++function normalizeClaudeConfig(config: OcxConfig): OcxConfig { ++ if (!config.claudeCode) return config; ++ return { ...config, claudeCode: normalizePersistedClaudeCode(config.claudeCode) }; ++} +``` + +Keep the specialized `subagentEffort` warning logic (`warnDegradedClaudeSubagentEffort`) exactly as it is — it reports a degradation the operator should see, and it is a separate concern from normalization. Add load-time tests proving that a config whose only defect is `classifierFallbacks: "opus"` or `classifierFallbacks: [1, "ok"]` is normalized on load, with `subagentEffort` absent from the fixture so the old short-circuit would have skipped it. + +## 090.6 Close-out + +This is the largest unit in the wave. If the redesign cannot land with the above evidence inside this loop, the honest outcome for `#1703`/`#1697` is to record the contract in this document, keep the PR on design hold, and report the phase rather than merging a silent-crossing implementation. + +Focused evidence when it does land: `bun test tests/claude-inbound.test.ts tests/router.test.ts tests/claude-messages-endpoint.test.ts`. diff --git a/devlog/_plan/260816_wave012_closeout/100_closeout.md b/devlog/_plan/260816_wave012_closeout/100_closeout.md new file mode 100644 index 0000000000..51ffce3f39 --- /dev/null +++ b/devlog/_plan/260816_wave012_closeout/100_closeout.md @@ -0,0 +1,31 @@ +# 100 — Closeout + +## Order of execution + +1. `010` Wave 0 triage (GitHub metadata only). +2. `020` `#1805`, `#1806` + test strengthening, close `#1786`. +3. `030` `#1741`, `#1825`, close/reclassify `#1824`. +4. `040` `#1817` + Cursor absorption, close `#1801`. +5. `050` `#1819` hardening, close `#1785`. +6. `060` `#1788` streaming state fix, close `#1700`. +7. `070` `#1780` allocator, close `#1767`. +8. `080` `#1792` null normalization + sponsorship, close `#1668`. +9. `090` `#1703` redesign, close `#1697` **only if the redesign actually lands with its evidence**; otherwise record the contract, keep the PR on design hold, and report the phase outcome honestly. + +Each numbered unit is one PABCD work-phase. No two decade docs are implemented in one build phase. + +## Verification contract + +- Per unit: focused `bun test ` named in that unit's doc. +- Per wave: `ssh lidge` full suite in `/home/lidgeai/Developer/opencodex` with `bun test --isolate tests`, reporting pass/fail counts. Raw `bun test` in that checkout bleeds environment state across files and is not acceptable evidence. +- Per merge: `git merge-base --is-ancestor origin/dev`. +- Per close: `gh issue view --json state` showing `CLOSED`. + +## Authority and constraints + +- Merges land on `dev` under maintainer authority. `main` promotion, tags and npm publish are out of scope; every close comment says the fix is on `dev` and release is pending. +- Local pushes use `--no-verify` per the operator's standing instruction for this loop; the substitute evidence is the remote suite, not the local hook. +- Contributor-branch CI is `action_required` and cannot be treated as green. Authorize the workflow runs and get the exact-head matrix **before** merging; `MAINTAINERS.md` requires maintainer approval plus successful CI, and post-merge testing is not a substitute. Windows-specific changes (`#1805`, `#1806`) specifically require the native Windows shards — the Linux `ssh lidge` suite cannot stand in for them. +- Any push to a contributor head resets review readiness and exact-head evidence: re-authorize CI and re-request approval after each such push. +- Units that touch the same file must be sequenced and rebased, not merged in parallel. `050` (`#1819`), `080` (`#1792`) and `090` (`#1703`) all edit `src/config.ts`, and `080`/`090` both edit `src/types.ts`. Execute in that order, rebasing each onto the previous head and re-auditing before sponsorship or approval. +- `#1795` stays open. `#92`, `#417`, `#1049`, `#1798`, `#1802` stay open. diff --git a/src/adapters/cursor/tool-definitions.ts b/src/adapters/cursor/tool-definitions.ts index 48a18c2ebe..057399dacf 100644 --- a/src/adapters/cursor/tool-definitions.ts +++ b/src/adapters/cursor/tool-definitions.ts @@ -191,6 +191,38 @@ export function isCursorWaitTool(tool: Pick): boo return isCursorResponsesProvider(tool.namespace) && tool.name === CODEX_WAIT_TOOL; } +/** + * True for Codex's unified-exec "code mode" tool: a freeform `exec` whose body is JavaScript + * evaluated in a V8 isolate, not a shell command string. + */ +export function isCursorCodeModeExecTool( + tool: Pick, +): boolean { + return isCursorResponsesProvider(tool.namespace) + && tool.name === CODEX_UNIFIED_EXEC_TOOL + && tool.freeform === true; +} + +/** + * Codex code mode advertises ONE freeform `exec` tool and no bare shell bridge. Shell, file + * edits, and MCP calls are reachable only as nested `tools.(...)` helpers described inside + * that tool's own description, so a flat catalog scan cannot see them. + * + * This matters because the shell-bridge guidance below is written for a flat catalog. Emitting + * "call \`exec_command\`" into a code-mode turn names a top-level tool that does not exist: the + * model calls it, gets nothing back, and burns turns rediscovering the real contract from error + * messages (empty output until \`text()\` is called, \`require is not defined\` because the isolate + * is not Node, \`apply_patch\` rejected because it too is only a nested helper here). + */ +export function cursorRequestUsesCodeMode( + tools: readonly Pick[] | undefined, + toolChoice?: OcxRequestOptions["toolChoice"], +): boolean { + const catalog = tools ?? []; + const visible = catalog.filter(tool => cursorToolAllowedByChoice(tool, toolChoice, catalog)); + return visible.some(isCursorCodeModeExecTool) && !visible.some(isBareCodexShellBridgeTool); +} + /** @deprecated Prefer isBareCodexShellBridgeTool; kept for older call sites/tests. */ function isBareCodexExecCommandTool(tool: Pick): boolean { return isBareCodexShellBridgeTool(tool); @@ -554,6 +586,14 @@ export function buildCursorToolGuidanceSystemNote( const listedNames = quotedNames(wireNames); const shellBridgeNames = wireNames.filter(isCodexShellBridgeToolName); const hasBareExec = shellBridgeNames.length > 0; + const codeMode = cursorRequestUsesCodeMode(tools, toolChoice); + // Code mode describes how the freeform exec tool works; it does not suppress the rest of the + // catalog. A turn can advertise freeform `exec` AND ordinary top-level tools at once, and + // telling the model those are "not separate top-level tools" would make it refuse tools that + // are right there in its catalog. Name the ones that stay callable instead. + const codeModeOtherTopLevelNames = codeMode + ? wireNames.filter(name => name !== CODEX_UNIFIED_EXEC_TOOL && !isCodexShellBridgeToolName(name)) + : []; const shellBridgeLabel = quotedNames(shellBridgeNames.length > 0 ? shellBridgeNames : [...CODEX_SHELL_BRIDGE_TOOL_NAMES]); const hasApplyPatch = cursorRequestAdvertisesApplyPatch(tools, toolChoice); const structuredEditNames = tools @@ -572,6 +612,14 @@ export function buildCursorToolGuidanceSystemNote( unavailableNeighborNames.length > 0 ? `This turn does not expose neighboring-agent tool names ${quotedNames(unavailableNeighborNames)}; do not call or suggest them unless the catalog lists them.` : undefined, + // Code mode: shell/edit/MCP live inside freeform `exec` as nested helpers. Without this the + // model probes for a top-level shell tool that is not there. + codeMode + ? `\`${CODEX_UNIFIED_EXEC_TOOL}\` is Codex code mode: its body is JavaScript evaluated in a V8 isolate, not a shell command and not Node. Shell, file edits, and MCP are nested helpers called INSIDE that body as \`await tools.(...)\`, for example \`await tools.exec_command({cmd: \"ls\"})\`. Read the tool description for the exact nested helpers this turn provides. Those nested helpers are not themselves top-level tools, so do not call \`exec_command\`, \`shell_command\`, or \`apply_patch\` at the top level here${codeModeOtherTopLevelNames.length > 0 ? `; every other tool this turn lists, including ${quotedNames(codeModeOtherTopLevelNames)}, remains callable at the top level as usual` : ""}.` + : undefined, + codeMode + ? "In code mode the isolate returns nothing on its own: call `text(...)` (or `notify(...)`) on any value you need to see, or the call completes with empty output. There is no `require`, no `module`, and no filesystem or network globals; reach the host only through the nested helpers." + : undefined, hasBareExec ? `${shellBridgeLabel} is the Codex Responses shell bridge for this turn, exposed through Cursor's tool protocol; it is not an external MCP server tool. \`shell_command\` and \`exec_command\` are aliases of the same bridge.` : undefined, diff --git a/tests/cursor-tool-definitions.test.ts b/tests/cursor-tool-definitions.test.ts index 2f2c644525..45c674219b 100644 --- a/tests/cursor-tool-definitions.test.ts +++ b/tests/cursor-tool-definitions.test.ts @@ -9,6 +9,8 @@ import { buildCursorToolGuidanceSystemNote, CURSOR_EXEC_COMMAND_INPUT_SCHEMA, cursorRequestAdvertisesApplyPatch, + cursorRequestUsesCodeMode, + isCursorCodeModeExecTool, cursorToolArgNormalizeSchema, cursorToolInputSchema, cursorToolWireName, @@ -411,3 +413,72 @@ describe("Cursor tool definitions", () => { expect(allowedNote).not.toContain("`mcp__fs__read_file`"); }); }); + +describe("Cursor code mode tool guidance", () => { + const codeModeExec = (): OcxTool => ({ + name: "exec", + description: "Run JavaScript code to orchestrate tool calls. Nested tools are available on the global `tools` object.", + parameters: {}, + freeform: true, + }); + + test("detects code mode only when freeform exec has no bare shell bridge", () => { + expect(cursorRequestUsesCodeMode([codeModeExec()])).toBe(true); + expect(isCursorCodeModeExecTool(codeModeExec())).toBe(true); + + // A non-freeform `exec` is not code mode. + expect(cursorRequestUsesCodeMode([{ name: "exec", description: "Run", parameters: {} }])).toBe(false); + // A bare shell bridge alongside it means the flat-catalog guidance still applies. + expect(cursorRequestUsesCodeMode([codeModeExec(), { name: "exec_command", description: "Run", parameters: {} }])).toBe(false); + expect(cursorRequestUsesCodeMode([{ name: "exec_command", description: "Run", parameters: {} }])).toBe(false); + expect(cursorRequestUsesCodeMode(undefined)).toBe(false); + // Tool choice that hides exec also hides code mode. + expect(cursorRequestUsesCodeMode([codeModeExec(), { name: "read_file", namespace: "mcp__fs", description: "R", parameters: {} }], { name: "read_file" })).toBe(false); + }); + + test("teaches the nested-helper contract instead of a top-level shell bridge", () => { + const note = buildCursorToolGuidanceSystemNote([codeModeExec()]); + expect(note).toBeDefined(); + if (!note) throw new Error("Expected Cursor tool guidance note"); + + expect(note).toContain("is Codex code mode"); + expect(note).toContain("V8 isolate"); + expect(note).toContain("await tools.(...)"); + expect(note).toContain("await tools.exec_command({cmd: " + "\"" + "ls" + "\"" + "})"); + expect(note).toContain("text(...)"); + expect(note).toContain("There is no `require`"); + + // The flat-catalog shell-bridge guidance must NOT appear: naming a top-level + // `exec_command` in code mode sends the model after a tool that does not exist. + expect(note).not.toContain("is the Codex Responses shell bridge for this turn"); + expect(note).not.toContain("mcp_opencodex-responses_shell_command"); + expect(note).not.toContain("For file read/search/listing, use"); + }); + + test("keeps other visible top-level tools callable in code mode", () => { + // Code mode is about how `exec` works, not a claim that the rest of the catalog is nested. + // A turn can advertise freeform `exec` alongside ordinary top-level tools, and describing + // those as non-top-level would make the model refuse tools it can actually call. + const note = buildCursorToolGuidanceSystemNote([ + codeModeExec(), + { name: "mcp__fs__read_file", description: "Read a file", parameters: {} }, + ]); + expect(note).toBeDefined(); + if (!note) throw new Error("Expected Cursor tool guidance note"); + + expect(note).toContain("is Codex code mode"); + expect(note).toContain("remains callable at the top level as usual"); + expect(note).toContain("mcp__fs__read_file"); + expect(note).not.toContain("they are not separate top-level tools"); + }); + + test("keeps flat-catalog shell-bridge guidance when a bare bridge is advertised", () => { + const note = buildCursorToolGuidanceSystemNote([{ name: "exec_command", description: "Run", parameters: {} }]); + expect(note).toBeDefined(); + if (!note) throw new Error("Expected Cursor tool guidance note"); + + expect(note).toContain("is the Codex Responses shell bridge for this turn"); + expect(note).not.toContain("is Codex code mode"); + expect(note).not.toContain("V8 isolate"); + }); +});