From bdc455a8f2a29f2107df78ba85303efdafcbb541 Mon Sep 17 00:00:00 2001 From: bitkyc08-arch Date: Sun, 16 Aug 2026 20:50:03 +0900 Subject: [PATCH 1/7] docs(devlog): plan Wave 0/1/2 closeout with diff-level decade docs --- .../260816_wave012_closeout/000_research.md | 69 ++++++++++++++++ .../010_wave0_triage.md | 21 +++++ .../020_wave1_1805_1806_1786.md | 35 ++++++++ .../030_wave1_1741_1825_1824.md | 23 ++++++ .../040_wave1_1817_1801.md | 30 +++++++ .../050_wave2_1819_1785.md | 69 ++++++++++++++++ .../060_wave2_1788_1700.md | 49 ++++++++++++ .../070_wave2_1780_1767.md | 62 +++++++++++++++ .../080_wave2_1792_1668.md | 53 +++++++++++++ .../090_wave2_1703_1697.md | 79 +++++++++++++++++++ .../260816_wave012_closeout/100_closeout.md | 29 +++++++ 11 files changed, 519 insertions(+) create mode 100644 devlog/_plan/260816_wave012_closeout/000_research.md create mode 100644 devlog/_plan/260816_wave012_closeout/010_wave0_triage.md create mode 100644 devlog/_plan/260816_wave012_closeout/020_wave1_1805_1806_1786.md create mode 100644 devlog/_plan/260816_wave012_closeout/030_wave1_1741_1825_1824.md create mode 100644 devlog/_plan/260816_wave012_closeout/040_wave1_1817_1801.md create mode 100644 devlog/_plan/260816_wave012_closeout/050_wave2_1819_1785.md create mode 100644 devlog/_plan/260816_wave012_closeout/060_wave2_1788_1700.md create mode 100644 devlog/_plan/260816_wave012_closeout/070_wave2_1780_1767.md create mode 100644 devlog/_plan/260816_wave012_closeout/080_wave2_1792_1668.md create mode 100644 devlog/_plan/260816_wave012_closeout/090_wave2_1703_1697.md create mode 100644 devlog/_plan/260816_wave012_closeout/100_closeout.md 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..267731925a --- /dev/null +++ b/devlog/_plan/260816_wave012_closeout/010_wave0_triage.md @@ -0,0 +1,21 @@ +# 010 — Wave 0: triage mutations + +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 + +- `gh issue view 1802 --json labels` shows `bug` present afterwards. +- `gh pr view 1822 --json labels` shows `gui`, not `bug`. +- Comment URLs on `#1049`, `#92`, `#417`. + +## 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..3d64d41992 --- /dev/null +++ b/devlog/_plan/260816_wave012_closeout/020_wave1_1805_1806_1786.md @@ -0,0 +1,35 @@ +# 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:692` in the case named "still absolutizes a relative sqlite home". It asserts the key exists and that the exact raw relative string is absent. That passes for any non-empty transformed value, absolute or not. + +Required diff (test file only): + +```diff +- expect(unit).toContain("CODEX_SQLITE_HOME"); +- expect(unit).not.toContain("Environment=CODEX_SQLITE_HOME=relative/sqlite"); ++ const line = unit.split("\n").find(l => l.includes("CODEX_SQLITE_HOME=")); ++ expect(line).toBeDefined(); ++ const value = line!.split("CODEX_SQLITE_HOME=")[1]!.trim(); ++ expect(value.length).toBeGreaterThan(0); ++ expect(isAbsolute(value) || posix.isAbsolute(value) || win32.isAbsolute(value)).toBe(true); ++ expect(value.endsWith("relative/sqlite") || value.endsWith("relative\\sqlite")).toBe(true); +``` + +Apply the same shape to the launchd variant if it shares the weak oracle. Import `isAbsolute` plus `posix`/`win32` from `node:path` at the top of the test file. + +## 020.3 Merge and close + +1. Merge `#1805` into `dev`. +2. Land the strengthened `#1806` (either push the test fix onto the PR head branch or fold it into the dev-side merge commit) and merge. +3. Run `bun test tests/service.test.ts tests/test-runner.test.ts` focused evidence. +4. 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..8d9b4cc7e8 --- /dev/null +++ b/devlog/_plan/260816_wave012_closeout/050_wave2_1819_1785.md @@ -0,0 +1,69 @@ +# 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 +function salvageConfigCandidate( + merged: unknown, + initialError: z.ZodError, +): { candidate: unknown; parsed: OcxConfig; dropped: string[] } | null { + let candidate = merged; + let error = initialError; + const dropped: string[] = []; + 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); + candidate = step.candidate; + const result = configSchema.safeParse(candidate); + if (result.success) return { candidate, parsed: result.data, dropped }; + error = result.error; + } + return null; +} +``` + +`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: + +```diff + const retryResult = configSchema.safeParse(merged); + if (retryResult.success) { ... } ++ const salvaged = salvageConfigCandidate(merged, retryResult.error); ++ if (salvaged) { ++ warnDroppedConfigSections(configPath, salvaged.dropped, retryResult.error); ++ return validFileConfigDiagnostics(normalizeApiKeyIds(salvaged.parsed), salvaged.candidate); ++ } + return { config: getDefaultConfig(), source: "fallback", ... }; +``` + +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. 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. + +## 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..d356fd6aa1 --- /dev/null +++ b/devlog/_plan/260816_wave012_closeout/060_wave2_1788_1700.md @@ -0,0 +1,49 @@ +# 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 (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); + }; +``` + +Call `noteInspectedPayload` from the SSE inspector for every parsed payload — `response.output_item.added`, `response.output_item.done`, `response.completed`, `response.incomplete` — on both the eager-inspector path (`:2774-2778`) and the tee path (`:2854-2873`). + +## 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..d86ca4b4df --- /dev/null +++ b/devlog/_plan/260816_wave012_closeout/070_wave2_1780_1767.md @@ -0,0 +1,62 @@ +# 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 + 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 = anthropicToolCallId(rawId); + if (base === undefined) return undefined; // empty/unusable: caller decides + let candidate = base; + for (let n = 2; occupied.has(candidate); n++) candidate = `${base}_${n}`; + 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. + +## 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. + +## 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..368af7a806 --- /dev/null +++ b/devlog/_plan/260816_wave012_closeout/080_wave2_1792_1668.md @@ -0,0 +1,53 @@ +# 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: + +```diff + const httpVersionError = upstreamHttpVersionConfigError(prov.upstreamHttpVersion); + if (httpVersionError) return jsonError(httpVersionError, 400); ++ if (prov.upstreamHttpVersion === null || prov.upstreamHttpVersion === "") { ++ delete prov.upstreamHttpVersion; ++ } +``` + +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: review the `auth-cors.ts` hunk explicitly as a security-boundary change, then apply `maintainer-sponsored` and remove `intake: hygiene-blocked`. If the hunk is not load-bearing for the HTTP-version fix, prefer dropping it and letting the PR leave the restricted surface entirely — that is the cheaper and safer path. + +## 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. + +## 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..632dad8cfb --- /dev/null +++ b/devlog/_plan/260816_wave012_closeout/090_wave2_1703_1697.md @@ -0,0 +1,79 @@ +# 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; + config: OcxConfig; + conversationId?: string; + affinity?: ClaudeSessionModelAffinity; + availableModels: readonly CatalogModel[]; +}): RouteResult; + +class NoAvailableClaudeClassifierRouteError extends Error { + constructor( + readonly requestedModel: string, + readonly rejections: readonly ClaudeClassifierCandidateRejection[], + ) { super(...); } +} +``` + +Candidate precedence, explicit and tested: + +1. exact / date-stripped `modelMap` override; +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. + +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)` (`src/codex/catalog/provider-fetch.ts:1527`) is the canonical source and already applies `disabledModels` and per-provider `selectedModels` (`:1510`). + +## 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. + +## 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..39006f56c3 --- /dev/null +++ b/devlog/_plan/260816_wave012_closeout/100_closeout.md @@ -0,0 +1,29 @@ +# 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`. + +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. Where a PR never ran an exact-head matrix, the `dev`-side suite after the merge is the evidence of record. +- `#1795` stays open. `#92`, `#417`, `#1049`, `#1798`, `#1802` stay open. From 017f03ba85cd8ae5ce5aab76ad6a352f28602aa2 Mon Sep 17 00:00:00 2001 From: bitkyc08-arch Date: Sun, 16 Aug 2026 21:04:59 +0900 Subject: [PATCH 2/7] docs(devlog): fix Wave 0/1/2 plan defects found in adversarial review --- .../010_wave0_triage.md | 10 +++-- .../020_wave1_1805_1806_1786.md | 39 ++++++++++++------- .../050_wave2_1819_1785.md | 19 +++++++-- .../060_wave2_1788_1700.md | 20 +++++++++- .../070_wave2_1780_1767.md | 13 ++++++- .../080_wave2_1792_1668.md | 23 ++++++++--- .../090_wave2_1703_1697.md | 15 ++++++- .../260816_wave012_closeout/100_closeout.md | 6 ++- 8 files changed, 111 insertions(+), 34 deletions(-) diff --git a/devlog/_plan/260816_wave012_closeout/010_wave0_triage.md b/devlog/_plan/260816_wave012_closeout/010_wave0_triage.md index 267731925a..ab041bd2a3 100644 --- a/devlog/_plan/260816_wave012_closeout/010_wave0_triage.md +++ b/devlog/_plan/260816_wave012_closeout/010_wave0_triage.md @@ -1,5 +1,7 @@ # 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 @@ -9,11 +11,11 @@ No issue is closed in this phase. GitHub metadata only; no repository code. 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 +## Evidence (captured) -- `gh issue view 1802 --json labels` shows `bug` present afterwards. -- `gh pr view 1822 --json labels` shows `gui`, not `bug`. -- Comment URLs on `#1049`, `#92`, `#417`. +- `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 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 index 3d64d41992..0b0e85c242 100644 --- a/devlog/_plan/260816_wave012_closeout/020_wave1_1805_1806_1786.md +++ b/devlog/_plan/260816_wave012_closeout/020_wave1_1805_1806_1786.md @@ -10,26 +10,37 @@ Disposition: merge as-is. Production runtime untouched. 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:692` in the case named "still absolutizes a relative sqlite home". It asserts the key exists and that the exact raw relative string is absent. That passes for any non-empty transformed value, absolute or not. +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 -- expect(unit).toContain("CODEX_SQLITE_HOME"); -- expect(unit).not.toContain("Environment=CODEX_SQLITE_HOME=relative/sqlite"); -+ const line = unit.split("\n").find(l => l.includes("CODEX_SQLITE_HOME=")); -+ expect(line).toBeDefined(); -+ const value = line!.split("CODEX_SQLITE_HOME=")[1]!.trim(); -+ expect(value.length).toBeGreaterThan(0); -+ expect(isAbsolute(value) || posix.isAbsolute(value) || win32.isAbsolute(value)).toBe(true); -+ expect(value.endsWith("relative/sqlite") || value.endsWith("relative\\sqlite")).toBe(true); + 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); ``` -Apply the same shape to the launchd variant if it shares the weak oracle. Import `isAbsolute` plus `posix`/`win32` from `node:path` at the top of the test file. +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 -1. Merge `#1805` into `dev`. -2. Land the strengthened `#1806` (either push the test fix onto the PR head branch or fold it into the dev-side merge commit) and merge. -3. Run `bun test tests/service.test.ts tests/test-runner.test.ts` focused evidence. -4. Close `#1786` with a comment naming the merged commit and stating release is pending. +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/050_wave2_1819_1785.md b/devlog/_plan/260816_wave012_closeout/050_wave2_1819_1785.md index 8d9b4cc7e8..1c263f2a65 100644 --- a/devlog/_plan/260816_wave012_closeout/050_wave2_1819_1785.md +++ b/devlog/_plan/260816_wave012_closeout/050_wave2_1819_1785.md @@ -34,24 +34,35 @@ function salvageConfigCandidate( `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: + ```diff const retryResult = configSchema.safeParse(merged); if (retryResult.success) { ... } -+ const salvaged = salvageConfigCandidate(merged, retryResult.error); ++ const salvaged = salvageConfigCandidate(merged, retryResult.error, raw); + if (salvaged) { -+ warnDroppedConfigSections(configPath, salvaged.dropped, retryResult.error); -+ return validFileConfigDiagnostics(normalizeApiKeyIds(salvaged.parsed), salvaged.candidate); ++ warnDroppedConfigSections(configPath, salvaged.dropped, salvaged.errors); ++ return validFileConfigDiagnostics( ++ normalizeApiKeyIds(salvaged.parsed), // from the merged candidate ++ salvaged.rawCandidate, // raw doc minus the same dropped entries ++ ); + } return { config: getDefaultConfig(), source: "fallback", ... }; ``` +`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. 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. +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 diff --git a/devlog/_plan/260816_wave012_closeout/060_wave2_1788_1700.md b/devlog/_plan/260816_wave012_closeout/060_wave2_1788_1700.md index d356fd6aa1..01ac9ed086 100644 --- a/devlog/_plan/260816_wave012_closeout/060_wave2_1788_1700.md +++ b/devlog/_plan/260816_wave012_closeout/060_wave2_1788_1700.md @@ -30,7 +30,25 @@ Required change: make rejection sticky per turn rather than terminal-snapshot-de }; ``` -Call `noteInspectedPayload` from the SSE inspector for every parsed payload — `response.output_item.added`, `response.output_item.done`, `response.completed`, `response.incomplete` — on both the eager-inspector path (`:2774-2778`) and the tee path (`:2854-2873`). +### 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. + +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 inspector-level regressions in the relay tests proving the ordering contract for both consumers: a stream whose only undeclared item arrives in `output_item.added` must deliver that payload to `onParsedPayload` strictly before `onCompletedResponse` fires. ## 060.3 Regression diff --git a/devlog/_plan/260816_wave012_closeout/070_wave2_1780_1767.md b/devlog/_plan/260816_wave012_closeout/070_wave2_1780_1767.md index d86ca4b4df..c76f640957 100644 --- a/devlog/_plan/260816_wave012_closeout/070_wave2_1780_1767.md +++ b/devlog/_plan/260816_wave012_closeout/070_wave2_1780_1767.md @@ -25,8 +25,11 @@ export function createToolCallIdAllocator() { if (existing) return existing; const base = anthropicToolCallId(rawId); if (base === undefined) return undefined; // empty/unusable: caller decides - let candidate = base; - for (let n = 2; occupied.has(candidate); n++) candidate = `${base}_${n}`; + let candidate = fitToolCallId(base); + for (let n = 2; occupied.has(candidate); n++) { + const suffix = `_${n}`; + candidate = fitToolCallId(base, suffix.length) + suffix; + } rawToWire.set(rawId, candidate); occupied.add(candidate); return candidate; @@ -38,6 +41,10 @@ export function createToolCallIdAllocator() { 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. Naive `_2`/`_3` disambiguation can push a previously valid candidate past the limit. `fitToolCallId(base, reserve = 0)` truncates `base` to `MAX_TOOL_CALL_ID_LENGTH - reserve`, truncating the deterministic hash tail last so the discriminating bits survive. The allocator therefore never emits an over-length ID, before or after collision resolution. + ## 070.3 Adapter wiring - Build the allocator at the top of the Anthropic request construction and pre-reserve all source IDs. @@ -56,6 +63,8 @@ Host them in `tests/adapter-usage.test.ts` under the existing `describe("anthrop - 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; +- the same boundary and collision cases hold for Google request construction, not only Anthropic. ## 070.5 Close-out diff --git a/devlog/_plan/260816_wave012_closeout/080_wave2_1792_1668.md b/devlog/_plan/260816_wave012_closeout/080_wave2_1792_1668.md index 368af7a806..eec59d2f00 100644 --- a/devlog/_plan/260816_wave012_closeout/080_wave2_1792_1668.md +++ b/devlog/_plan/260816_wave012_closeout/080_wave2_1792_1668.md @@ -15,16 +15,21 @@ The management validator accepts it (`src/config.ts:917`: `if (value === undefin ## 080.2 Fix on both sides -Canonicalize at the write boundary: +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 httpVersionError = upstreamHttpVersionConfigError(prov.upstreamHttpVersion); - if (httpVersionError) return jsonError(httpVersionError, 400); -+ if (prov.upstreamHttpVersion === null || prov.upstreamHttpVersion === "") { -+ delete prov.upstreamHttpVersion; + const providerError = providerManagementConfigError(name, body.provider); + if (providerError) return jsonError(providerError, 400); ++ if ( ++ (body.provider as Record).upstreamHttpVersion === null || ++ (body.provider as Record).upstreamHttpVersion === "" ++ ) { ++ delete (body.provider as Record).upstreamHttpVersion; + } ``` +Note that the validator already rejects `""` (only `undefined`/`null` short-circuit at `src/config.ts:917`), so the empty-string arm is defensive only; `null` is the case that actually reaches persistence. Apply the same canonicalization on the other write paths that call `providerManagementConfigError` (`:405`, `:617`, `:646`) so replay and sync cannot reintroduce the null. + And make the loader tolerant so any already-persisted candidate still loads: ```diff @@ -40,7 +45,9 @@ PATCH semantics stay as they are. `.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: review the `auth-cors.ts` hunk explicitly as a security-boundary change, then apply `maintainer-sponsored` and remove `intake: hygiene-blocked`. If the hunk is not load-bearing for the HTTP-version fix, prefer dropping it and letting the PR leave the restricted surface entirely — that is the cheaper and safer path. +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 @@ -48,6 +55,10 @@ In `tests/management-provider-validation.test.ts`: POST a provider with `upstrea 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 index 632dad8cfb..3f876d660f 100644 --- a/devlog/_plan/260816_wave012_closeout/090_wave2_1703_1697.md +++ b/devlog/_plan/260816_wave012_closeout/090_wave2_1703_1697.md @@ -18,6 +18,7 @@ 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; @@ -32,9 +33,18 @@ class NoAvailableClaudeClassifierRouteError extends Error { } ``` +### 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 itself — `isClaudeClassifierRequest(body)` — before any affinity is consumed: + +- Auto Mode classifier turns carry a distinctive small system prompt and a single short user turn with no tool definitions and a low `max_tokens`; +- ordinary turns carry the Claude Code system prompt, tool definitions, or prior assistant/tool history. + +Implement the predicate as an explicit conjunction of stable, observable body fields (no tools declared, no assistant/tool history, `max_tokens` under the configured classifier ceiling, and the classifier system-prompt marker), record which conditions matched in the route decision, and default to `false` when the evidence is ambiguous. Failing closed here means an ambiguous request is routed as an ordinary request, which is the safe direction: it can never silently borrow another session's provider. + Candidate precedence, explicit and tested: -1. exact / date-stripped `modelMap` override; +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`; @@ -71,6 +81,9 @@ On `NoAvailableClaudeClassifierRouteError`, the Claude endpoint returns a stable - 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. + +`classifierModel` and `classifierFallbacks` are operator-facing configuration, so they need management-API validation, an entry in `docs-site/`, and a visible route-decision reason in diagnostics. Shipping the routing contract without the config surface would leave the fallback list unusable in practice. ## 090.6 Close-out diff --git a/devlog/_plan/260816_wave012_closeout/100_closeout.md b/devlog/_plan/260816_wave012_closeout/100_closeout.md index 39006f56c3..2c4a3e5683 100644 --- a/devlog/_plan/260816_wave012_closeout/100_closeout.md +++ b/devlog/_plan/260816_wave012_closeout/100_closeout.md @@ -10,7 +10,7 @@ 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`. +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. @@ -25,5 +25,7 @@ Each numbered unit is one PABCD work-phase. No two decade docs are implemented i - 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. Where a PR never ran an exact-head matrix, the `dev`-side suite after the merge is the evidence of record. +- 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`) and `080` (`#1792`) both edit `src/config.ts`; `#1792` rebases onto the post-`#1819` head and is re-audited before sponsorship. - `#1795` stays open. `#92`, `#417`, `#1049`, `#1798`, `#1802` stay open. From 8d9b1ab8b1cc327bb7f7fd2a6b8f5f329751fa35 Mon Sep 17 00:00:00 2001 From: bitkyc08-arch Date: Sun, 16 Aug 2026 21:10:06 +0900 Subject: [PATCH 3/7] docs(devlog): make Wave 2 plan diffs executable against real code --- .../050_wave2_1819_1785.md | 37 +++++++--- .../060_wave2_1788_1700.md | 26 ++++++- .../070_wave2_1780_1767.md | 22 +++++- .../080_wave2_1792_1668.md | 16 +++-- .../090_wave2_1703_1697.md | 68 +++++++++++++++++-- 5 files changed, 146 insertions(+), 23 deletions(-) diff --git a/devlog/_plan/260816_wave012_closeout/050_wave2_1819_1785.md b/devlog/_plan/260816_wave012_closeout/050_wave2_1819_1785.md index 1c263f2a65..5731af506f 100644 --- a/devlog/_plan/260816_wave012_closeout/050_wave2_1819_1785.md +++ b/devlog/_plan/260816_wave012_closeout/050_wave2_1819_1785.md @@ -7,27 +7,42 @@ Head `65822b008740af75cd2568595cb60602de80d899` (draft). Direction is right; fou 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, -): { candidate: unknown; parsed: OcxConfig; dropped: string[] } | null { + 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, parsed: result.data, dropped }; + 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 @@ -36,20 +51,26 @@ function salvageConfigCandidate( 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) { ... } -+ const salvaged = salvageConfigCandidate(merged, retryResult.error, raw); + if (retryResult.success) { + return validFileConfigDiagnostics(normalizeApiKeyIds(retryResult.data as OcxConfig), parsed); + } ++ const salvaged = salvageConfigCandidate(merged, retryResult.error, parsed); + if (salvaged) { -+ warnDroppedConfigSections(configPath, salvaged.dropped, salvaged.errors); ++ warnDroppedConfigSections(configPath, salvaged.dropped, salvaged.issues); + return validFileConfigDiagnostics( -+ normalizeApiKeyIds(salvaged.parsed), // from the merged candidate -+ salvaged.rawCandidate, // raw doc minus the same dropped entries ++ normalizeApiKeyIds(salvaged.parsed), // validated, from the merged candidate ++ salvaged.rawCandidate, // raw document minus the same entries + ); + } - return { config: getDefaultConfig(), source: "fallback", ... }; + 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. + `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. diff --git a/devlog/_plan/260816_wave012_closeout/060_wave2_1788_1700.md b/devlog/_plan/260816_wave012_closeout/060_wave2_1788_1700.md index 01ac9ed086..9e03bcb30d 100644 --- a/devlog/_plan/260816_wave012_closeout/060_wave2_1788_1700.md +++ b/devlog/_plan/260816_wave012_closeout/060_wave2_1788_1700.md @@ -44,11 +44,33 @@ Required change: make rejection sticky per turn rather than terminal-snapshot-de 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. +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 inspector-level regressions in the relay tests proving the ordering contract for both consumers: a stream whose only undeclared item arrives in `output_item.added` must deliver that payload to `onParsedPayload` strictly before `onCompletedResponse` fires. +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 diff --git a/devlog/_plan/260816_wave012_closeout/070_wave2_1780_1767.md b/devlog/_plan/260816_wave012_closeout/070_wave2_1780_1767.md index c76f640957..59d4e52ade 100644 --- a/devlog/_plan/260816_wave012_closeout/070_wave2_1780_1767.md +++ b/devlog/_plan/260816_wave012_closeout/070_wave2_1780_1767.md @@ -18,6 +18,8 @@ export function createToolCallIdAllocator() { 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 { @@ -43,7 +45,24 @@ Reserve every already-conforming source ID before allocating any rewritten one, ### Length bound -Anthropic constrains tool-call ID length, and the issue names it explicitly. Naive `_2`/`_3` disambiguation can push a previously valid candidate past the limit. `fitToolCallId(base, reserve = 0)` truncates `base` to `MAX_TOOL_CALL_ID_LENGTH - reserve`, truncating the deterministic hash tail last so the discriminating bits survive. The allocator therefore never emits an over-length ID, before or after collision resolution. +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. Build candidates as `truncatedPrefix + "_" + fullHash` (+ 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. + +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; +} +``` + +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 @@ -64,6 +83,7 @@ Host them in `tests/adapter-usage.test.ts` under the existing `describe("anthrop - 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 diff --git a/devlog/_plan/260816_wave012_closeout/080_wave2_1792_1668.md b/devlog/_plan/260816_wave012_closeout/080_wave2_1792_1668.md index eec59d2f00..7963e781ce 100644 --- a/devlog/_plan/260816_wave012_closeout/080_wave2_1792_1668.md +++ b/devlog/_plan/260816_wave012_closeout/080_wave2_1792_1668.md @@ -19,16 +19,18 @@ Canonicalize at the write boundary. There is no local `httpVersionError` in the ```diff const providerError = providerManagementConfigError(name, body.provider); - if (providerError) return jsonError(providerError, 400); -+ if ( -+ (body.provider as Record).upstreamHttpVersion === null || -+ (body.provider as Record).upstreamHttpVersion === "" -+ ) { -+ delete (body.provider as Record).upstreamHttpVersion; + 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; + } ``` -Note that the validator already rejects `""` (only `undefined`/`null` short-circuit at `src/config.ts:917`), so the empty-string arm is defensive only; `null` is the case that actually reaches persistence. Apply the same canonicalization on the other write paths that call `providerManagementConfigError` (`:405`, `:617`, `:646`) so replay and sync cannot reintroduce the null. +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 --git a/devlog/_plan/260816_wave012_closeout/090_wave2_1703_1697.md b/devlog/_plan/260816_wave012_closeout/090_wave2_1703_1697.md index 3f876d660f..0fc196a15b 100644 --- a/devlog/_plan/260816_wave012_closeout/090_wave2_1703_1697.md +++ b/devlog/_plan/260816_wave012_closeout/090_wave2_1703_1697.md @@ -35,12 +35,37 @@ class NoAvailableClaudeClassifierRouteError extends Error { ### 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 itself — `isClaudeClassifierRequest(body)` — before any affinity is consumed: +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. -- Auto Mode classifier turns carry a distinctive small system prompt and a single short user turn with no tool definitions and a low `max_tokens`; -- ordinary turns carry the Claude Code system prompt, tool definitions, or prior assistant/tool history. +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: -Implement the predicate as an explicit conjunction of stable, observable body fields (no tools declared, no assistant/tool history, `max_tokens` under the configured classifier ceiling, and the classifier system-prompt marker), record which conditions matched in the route decision, and default to `false` when the evidence is ambiguous. Failing closed here means an ambiguous request is routed as an ordinary request, which is the safe direction: it can never silently borrow another session's provider. +```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: + +```ts +export const CLASSIFIER_MAX_TOKENS_CEILING = 512; // named constant, not config + +export function isClaudeClassifierRequest(body: AnthropicMessagesBody): boolean { + if (Array.isArray(body.tools) && body.tools.length > 0) return false; + const messages = body.messages ?? []; + if (messages.length !== 1 || messages[0]?.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 true; +} +``` + +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. If a field has drifted, update the predicate to the captured shape and record the capture in this unit — the predicate must be bound to observed traffic, not to a 545-era fixture assumed still current. + +**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: @@ -83,7 +108,40 @@ On `NoAvailableClaudeClassifierRouteError`, the Claude endpoint returns a stable - 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. -`classifierModel` and `classifierFallbacks` are operator-facing configuration, so they need management-API validation, an entry in `docs-site/`, and a visible route-decision reason in diagnostics. Shipping the routing contract without the config surface would leave the fallback list unusable in practice. +### 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. ## 090.6 Close-out From e658eb133aa5712d446d8da6edf0e314158a8baa Mon Sep 17 00:00:00 2001 From: bitkyc08-arch Date: Sun, 16 Aug 2026 21:15:44 +0900 Subject: [PATCH 4/7] docs(devlog): bind classifier predicate to a real type, keep captures out of the public devlog --- .../070_wave2_1780_1767.md | 12 ++++--- .../090_wave2_1703_1697.md | 32 ++++++++++++++++--- 2 files changed, 35 insertions(+), 9 deletions(-) diff --git a/devlog/_plan/260816_wave012_closeout/070_wave2_1780_1767.md b/devlog/_plan/260816_wave012_closeout/070_wave2_1780_1767.md index 59d4e52ade..fa9fc870d4 100644 --- a/devlog/_plan/260816_wave012_closeout/070_wave2_1780_1767.md +++ b/devlog/_plan/260816_wave012_closeout/070_wave2_1780_1767.md @@ -25,12 +25,13 @@ export function createToolCallIdAllocator() { allocate(rawId: string): string | undefined { const existing = rawToWire.get(rawId); if (existing) return existing; - const base = anthropicToolCallId(rawId); + const base = toolCallIdComponents(rawId); // { cleaned, hash } | undefined if (base === undefined) return undefined; // empty/unusable: caller decides - let candidate = fitToolCallId(base); + 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(base, suffix.length) + suffix; + candidate = fitToolCallId(cleaned, hash, suffix.length) + suffix; } rawToWire.set(rawId, candidate); occupied.add(candidate); @@ -49,11 +50,12 @@ Anthropic constrains tool-call ID length, and the issue names it explicitly. Two 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. Build candidates as `truncatedPrefix + "_" + fullHash` (+ collision suffix), never as a plain slice of the finished string: +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}`; @@ -62,6 +64,8 @@ function fitToolCallId(cleaned: string, hash: string, reserve = 0): string { } ``` +`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 diff --git a/devlog/_plan/260816_wave012_closeout/090_wave2_1703_1697.md b/devlog/_plan/260816_wave012_closeout/090_wave2_1703_1697.md index 0fc196a15b..38b26efa50 100644 --- a/devlog/_plan/260816_wave012_closeout/090_wave2_1703_1697.md +++ b/devlog/_plan/260816_wave012_closeout/090_wave2_1703_1697.md @@ -47,23 +47,39 @@ The observable shape is already captured in-repo by the #545 classifier fixture 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(body: AnthropicMessagesBody): boolean { +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; - const messages = body.messages ?? []; - if (messages.length !== 1 || messages[0]?.role !== "user") return false; // no assistant/tool history + 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 true; + 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. If a field has drifted, update the predicate to the captured shape and record the capture in this unit — the predicate must be bound to observed traffic, not to a 545-era fixture assumed still current. +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. @@ -143,6 +159,12 @@ if (body.classifierFallbacks !== undefined) { 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. Add load-time tests for both malformed shapes. + ## 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. From 78d4dec5378de66388e69a9972b45566b25ae9af Mon Sep 17 00:00:00 2001 From: bitkyc08-arch Date: Sun, 16 Aug 2026 21:20:10 +0900 Subject: [PATCH 5/7] docs(devlog): filter the classifier catalog and actually activate claude config normalization --- .../090_wave2_1703_1697.md | 30 +++++++++++++++++-- 1 file changed, 28 insertions(+), 2 deletions(-) diff --git a/devlog/_plan/260816_wave012_closeout/090_wave2_1703_1697.md b/devlog/_plan/260816_wave012_closeout/090_wave2_1703_1697.md index 38b26efa50..cdcc0dc1ab 100644 --- a/devlog/_plan/260816_wave012_closeout/090_wave2_1703_1697.md +++ b/devlog/_plan/260816_wave012_closeout/090_wave2_1703_1697.md @@ -93,7 +93,15 @@ Candidate precedence, explicit and tested: There is no implicit "first Anthropic adapter" candidate. -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)` (`src/codex/catalog/provider-fetch.ts:1527`) is the canonical source and already applies `disabledModels` and per-provider `selectedModels` (`:1510`). +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 @@ -163,7 +171,25 @@ Persistence tests belong beside the existing claude-code settings tests: PUT the 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. Add load-time tests for both malformed shapes. +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 From 68f5308337ad6d4cec3cc62c79be99af03c9f2b5 Mon Sep 17 00:00:00 2001 From: bitkyc08-arch Date: Sun, 16 Aug 2026 21:31:54 +0900 Subject: [PATCH 6/7] docs(devlog): gate the inspection flag, fix the diagnostics path binding, split classifier precedence --- .../260816_wave012_closeout/050_wave2_1819_1785.md | 4 +++- .../260816_wave012_closeout/060_wave2_1788_1700.md | 4 +++- .../260816_wave012_closeout/090_wave2_1703_1697.md | 12 +++++++++++- devlog/_plan/260816_wave012_closeout/100_closeout.md | 2 +- 4 files changed, 18 insertions(+), 4 deletions(-) diff --git a/devlog/_plan/260816_wave012_closeout/050_wave2_1819_1785.md b/devlog/_plan/260816_wave012_closeout/050_wave2_1819_1785.md index 5731af506f..36934fa405 100644 --- a/devlog/_plan/260816_wave012_closeout/050_wave2_1819_1785.md +++ b/devlog/_plan/260816_wave012_closeout/050_wave2_1819_1785.md @@ -60,7 +60,7 @@ In `configDiagnosticsFromRaw(raw: string)` the parameter `raw` is the JSON **tex } + const salvaged = salvageConfigCandidate(merged, retryResult.error, parsed); + if (salvaged) { -+ warnDroppedConfigSections(configPath, salvaged.dropped, salvaged.issues); ++ warnDroppedConfigSections(getConfigPath(), salvaged.dropped, salvaged.issues); + return validFileConfigDiagnostics( + normalizeApiKeyIds(salvaged.parsed), // validated, from the merged candidate + salvaged.rawCandidate, // raw document minus the same entries @@ -71,6 +71,8 @@ In `configDiagnosticsFromRaw(raw: string)` the parameter `raw` is the JSON **tex `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. diff --git a/devlog/_plan/260816_wave012_closeout/060_wave2_1788_1700.md b/devlog/_plan/260816_wave012_closeout/060_wave2_1788_1700.md index 9e03bcb30d..218ba4fde7 100644 --- a/devlog/_plan/260816_wave012_closeout/060_wave2_1788_1700.md +++ b/devlog/_plan/260816_wave012_closeout/060_wave2_1788_1700.md @@ -17,7 +17,7 @@ Required change: make rejection sticky per turn rather than terminal-snapshot-de ```diff + let inspectionSawUndeclaredTool = false; + const noteInspectedPayload = (payload: unknown) => { -+ if (inspectionSawUndeclaredTool) return; ++ if (!undeclaredToolGuardActive || inspectionSawUndeclaredTool) return; + if (undeclaredToolCallName(payload, declaredWireToolNames) !== undefined) { + inspectionSawUndeclaredTool = true; + } @@ -30,6 +30,8 @@ Required change: make rejection sticky per turn rather than terminal-snapshot-de }; ``` +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 --git a/devlog/_plan/260816_wave012_closeout/090_wave2_1703_1697.md b/devlog/_plan/260816_wave012_closeout/090_wave2_1703_1697.md index cdcc0dc1ab..e532fce471 100644 --- a/devlog/_plan/260816_wave012_closeout/090_wave2_1703_1697.md +++ b/devlog/_plan/260816_wave012_closeout/090_wave2_1703_1697.md @@ -83,7 +83,9 @@ Before implementing, capture one live Claude Code Auto Mode classifier request a **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: +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; @@ -93,6 +95,14 @@ Candidate precedence, explicit and tested: 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: diff --git a/devlog/_plan/260816_wave012_closeout/100_closeout.md b/devlog/_plan/260816_wave012_closeout/100_closeout.md index 2c4a3e5683..51ffce3f39 100644 --- a/devlog/_plan/260816_wave012_closeout/100_closeout.md +++ b/devlog/_plan/260816_wave012_closeout/100_closeout.md @@ -27,5 +27,5 @@ Each numbered unit is one PABCD work-phase. No two decade docs are implemented i - 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`) and `080` (`#1792`) both edit `src/config.ts`; `#1792` rebases onto the post-`#1819` head and is re-audited before sponsorship. +- 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. From cb48c2e11f3100d4c170409d78554752f9a44579 Mon Sep 17 00:00:00 2001 From: bitkyc08-arch Date: Sun, 16 Aug 2026 21:37:09 +0900 Subject: [PATCH 7/7] fix(cursor): teach the code-mode nested-helper contract in tool guidance Absorbs #1801 onto the #1817 contract. Codex code mode advertises one freeform `exec` whose body is JavaScript in a V8 isolate; shell, file edits and MCP are nested `tools.(...)` helpers described inside that tool, so the flat shell-bridge guidance sent the model after a top-level tool that does not exist. Fixes the mixed-catalog defect found while absorbing: code mode describes how `exec` works, it does not make the rest of the catalog nested. When other top-level tools are visible, the note now names them as still callable instead of declaring them "not separate top-level tools". Supersedes #1801. --- src/adapters/cursor/tool-definitions.ts | 48 +++++++++++++++++ tests/cursor-tool-definitions.test.ts | 71 +++++++++++++++++++++++++ 2 files changed, 119 insertions(+) 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"); + }); +});