diff --git a/devlog/_plan/260805_bug_stack_campaign/000_plan.md b/devlog/_plan/260805_bug_stack_campaign/000_plan.md new file mode 100644 index 000000000..641b3b5d5 --- /dev/null +++ b/devlog/_plan/260805_bug_stack_campaign/000_plan.md @@ -0,0 +1,99 @@ +# 000 — Bug stack campaign: triage the open bug surface, fix the 400 regression, stack the repairs + +## Objective + +As of 2026-08-05 (KST), bring the open bug surface of lidge-jun/opencodex to +the state where every item is either landed, closed with evidence, or carried +by a stacked PR grounded in code somebody actually read. Three fronts: + +1. PR #988 — review against the GUI design system, small fixes if needed, + merge to `dev` (user authorized this merge/push). +2. Full triage of open bug issues + bug/code-improvement PRs against + `origin/dev` HEAD `e44d234f08e03dd4dbf0c4aa13af43046d86b0a6`. Items + already fixed on `dev` get closed with evidence; the rest enter the stack. +3. New regression: 400 `invalid_request_error` when a new Codex session/thread + spawns against `anthropic/claude-opus-5` (2026-08-05 00:22 KST, request + `ocx-mset3rk6-1i6`, conversation `2c0f87c664e130b3927c4884fafd8283`). + Root-cause and fix with a regression test. + +Items opened after 2026-08-05 00:00 KST are out of scope. Feature programs +(provider batches, RI router-intelligence stack, localization programs, +large infrastructure PRs) are out of scope. + +## Loop spec + +- Loop archetype: spec-satisfaction repair (each fix has a checkable verifier). +- Trigger: user-directed campaign, HOTL goal loop (goalplan + `opencodex-pr-988-988-uiux-bug-pr-dev-head-close`). +- Goal: open bug surface fully dispositioned; #988 landed; 400 fixed. +- Non-goals: feature PRs, new providers, RI stack, docs-only improvements + without code defect, anything opened after the campaign cutoff. +- Verifier: `bun run typecheck`, focused `bun test` per fix, full + `bun run test` on `ssh lidge` (Linux baseline recorded below), + `bun run privacy:scan`, plus per-fix activation evidence. +- Stop condition: every in-scope issue/PR dispositioned (landed / closed with + evidence / stacked PR open with CI). +- Memory artifact: this unit + goalplan ledger. +- Resource bounds: sol-medium explorer subagents for research/verification + (unlimited); B phases owned by the main session; wall-clock unbounded but + checkpoint every B step with commits (LOOP-GIT-01). +- Expected terminal outcomes: DONE (all dispositioned), or per-item + BLOCKED/NEEDS_HUMAN named with evidence. +- Escalation: push/merge only within the user-approved scope (#988 merge, + stack PR creation, evidence-based closes). + +## Environment baselines + +- `origin/dev` HEAD: `e44d234f08e03dd4dbf0c4aa13af43046d86b0a6`. +- Worktree: `/Users/jun/.codex/worktrees/250c/opencodex`, branch + `codex/bug-stack-campaign` (forked from `origin/dev`; the previously + detached other-unit HEAD `9289891a5` remains untouched, contained in local + `dev`). +- lidge Linux baseline (`bun run test` on dev HEAD, 2026-08-05): the first + cold-cache run showed 12 fails with a phantom + `ROLLUP_COST_SEMANTICS_VERSION` import error (an identifier that exists + nowhere in the repo) plus storage/keyring suites — a first-run module-cache + race, not repo state. Warm-cache rerun: **8212 pass / 10 skip / 0 fail, + EXIT=0** (`/tmp/ocx-baseline-test2.log` on lidge). Campaign deltas are + measured against the green warm baseline. + +## Work-phase map + +Honest dependency structure (amended after audit round 1): only three real +dependencies exist — phase 2 builds on the 003 probes, phase 3's core.ts +cluster (stack 02/03/05) must be internally ordered, and each phase-4 +disposition waits for its corresponding landing. Everything else is +independent and does not inherit merge blockage from earlier items. + +| Phase | Doc | Content | Depends on | +|-------|-----|---------|------------| +| 0 | this doc + 001/002/003 | triage + research (docs-only) | — | +| 1 | 010 | PR #988 design review + merge | none (independent GUI slice) | +| 2 | 020 | anthropic sidecar-bridge error fidelity (`formatErrorBody`) | 003 probes + bridge analysis | +| 3 | 030 | #914 DNS transport attribution | 001/002 triage | +| 4 | 040 | #893 sparse snapshot repair | none (independent) | +| 5 | 050 | #875 DeepSeek Flash stall | none (independent) | +| 6 | 060 | #938 UUID item IDs | 050 (shared JSON→event boundary) | +| 7 | 070 | #907 jawcode prices (+ external jawcode source write) | none | +| 8 | 080 | #1007 login URL flush | none | +| 9 | 090 | #1001 forced-answer validation | none | +| 10 | 100 | #992 routed context_window | none | +| 11 | 110 | #993 Kiro profileArn | none | +| 12 | 120 | #959 provider headers (adopt PR #961) | none | +| 13 | 130 | dispositions: #806 close (anytime); supersede-closes per landing | respective phases | + +Dependency honesty (audit round 3): 030, 040, 050 are semantically +independent — none consumes another's artifact. Only 050→060 is a real +dependency (060 extends 050's bounded-JSON event boundary). To control +`core.ts` merge conflicts the RECOMMENDED merge order is 030 → 040 → 050 → +060, then the independent lanes 070-120 in any order, each branching from +`origin/dev` (or the campaign base when doc files ride along). Each phase is +one full PABCD cycle whose P re-verifies its decade doc against the +then-current tree. + +## Triage inputs + +- `001_issue_triage.md` — 38 open issues (sol-medium lane A). +- `002_pr_triage.md` — 30 open PRs (sol-medium lane B). +- `003_anthropic_400_research.md` — 400 regression research (lane C + local + log analysis). diff --git a/devlog/_plan/260805_bug_stack_campaign/001_issue_triage.md b/devlog/_plan/260805_bug_stack_campaign/001_issue_triage.md new file mode 100644 index 000000000..4a45f767b --- /dev/null +++ b/devlog/_plan/260805_bug_stack_campaign/001_issue_triage.md @@ -0,0 +1,43 @@ +# 001 — Open issue triage (2026-08-05, vs origin/dev e44d234f0) + +Source: sol-medium explorer lane A. 38 open issues. Classification: 14 bug +(10 unresolved, 4 needs-verification), 4 code-level improvement (1 already +resolved on dev), 20 out of scope (feature/provider/tracker/upstream). + +## Confirmed unresolved bugs — stack candidates + +| Issue | Defect | Code evidence | +|------:|--------|---------------| +| #1007 | `ocx account login` withholds the authorization URL under non-TTY stdout | `src/cli/account-auth.ts:84-101` — console.log then long polling, no explicit flush | +| #1001 | Hosted web-search forced-answer pass accepts a malformed tool call and completes without an assistant message | `src/web-search/loop.ts:72-75,681-705` — empty tool name judged real; forced-answer ends without visible message check | +| #993 | Kiro provider: profileArn required 400 for Builder ID accounts on gated models | `src/adapters/kiro.ts:1723-1729` — profileArn silently omitted, CLI wire used; no per-account ARN acquisition | +| #992 | Routed models inherit native template context_window when /models omits context metadata | `src/codex/catalog/sync.ts:205-228`, `src/codex/catalog/parsing.ts:290-301` | +| #959 | No management-plane support for provider `headers` | `src/server/management/provider-routes.ts:210-289` — PATCH mask lacks `headers`, unknown fields 400; CLI lacks the option (`src/cli/provider-runtime.ts:16-55`) | +| #938 | Responses passthrough UUID item IDs leave Codex stuck on Thinking | `src/server/responses-item-id-repair.ts:84-94,220-223` — repair only handles pre-registered placeholders / missing terminal IDs, never validates arbitrary UUID prefixes | +| #914 | DNS/network reachability failures incorrectly rotate Codex pool accounts | `src/server/responses/core.ts:1737`, `src/server/responses/compact.ts:419` — fetch rejections still recorded as `connect_error` against account health; candidate fix `fe693ae62` not an ancestor of origin/dev | +| #907 | Bundled jawcode prices for gpt-5.6-terra/luna are pre-price-cut rates | `src/generated/jawcode-model-metadata.ts:47` — stale Luna 1/6/0.1/1.25, Terra 2.5/15/0.25/3.125 | +| #893 | Responses-compatible gateways can return sparse lifecycle snapshots Codex clients do not commit | no `responsesSnapshotRepair`; current SSE assembly only repairs images and item IDs (`src/server/responses/core.ts:1967-1976`) | +| #875 | DeepSeek V4 Flash Responses route stalls after tool calls | `5dd965a13` only disabled WebSocket upstream streaming (`src/providers/registry.ts:1146`); reporter reproduced on default HTTP/SSE path at e44d234f0, still SSE relay (`src/server/responses/core.ts:1880-1881,1967`) | + +## Needs verification before stack entry + +| Issue | Why unresolved status is uncertain | +|------:|-----------------------------------| +| #994 | `reasoning_content` replay is model-allowlisted (`src/providers/registry.ts:918-958,1637-1655`); report lacks provider/model + wire capture | +| #904 | `eeef7a32a` fixed astral-surrogate boundaries but does not explain the original Hangul-only corruption; needs the failing wire capture | +| #796 | Structured empty-content fix `d3abf4345` + regression test landed (`src/adapters/openai-chat.ts:366-379`), but no live Ark credential verification | +| #418 | Current code provides catalog/roster guidance (`src/server/responses/collaboration.ts:243-304`); latest same-run trace does not reproduce the original failure | + +## Already resolved on dev — close candidates + +| Issue | Evidence | +|------:|----------| +| #806 | `d52b387db` (ancestor of origin/dev) split/fixed GUI+CLI+docs wording; current GUI distinguishes "Usage-based proactive switching" from cache warnings (`gui/src/i18n/en.ts:1296-1315`) | + +## Out of scope (20) + +Feature/provider/tracker/upstream items: #974, #823, #822, #821, #755, #695, +#657, #572, #561, #540, #241, #201, #178, #177, #95, #92, #417, #415, #414, +#386. (Improvement #820 and #809 are code-level and stay in the campaign's +improvement bucket; #820 is a larger architecture epic — deferred to its own +unit, not this stack.) diff --git a/devlog/_plan/260805_bug_stack_campaign/002_pr_triage.md b/devlog/_plan/260805_bug_stack_campaign/002_pr_triage.md new file mode 100644 index 000000000..67bdd9a3b --- /dev/null +++ b/devlog/_plan/260805_bug_stack_campaign/002_pr_triage.md @@ -0,0 +1,47 @@ +# 002 — Open PR triage (2026-08-05, vs origin/dev e44d234f0) + +Source: sol-medium explorer lane B. 30 open PRs, all targeting `dev`. +Classification: 15 bug, 3 code/docs improvement, 12 feature/program. +No bug/improvement PR has an equivalent fix already merged on dev. + +## Bug PRs + +| PR | Author | Subject | Draft | CI | Merge state | +|---:|--------|---------|:-----:|----|-------------| +| #988 | Wibias | GUI providers quota/auth, Claude pool toggle, combos/models layout, dev session bootstrap | no | full PASS | MERGEABLE/CLEAN — only clean bug PR | +| #983 | DevMello | stop counting base64 attachments as raw characters in token estimates | no | policy-only | MERGEABLE/UNSTABLE | +| #978 | DevMello | send thinkingLevel for any model with an effort ladder (google) | no | policy-only | MERGEABLE/UNSTABLE | +| #985 | DevMello | deliver structured output to routed openai-chat models | yes | policy-only | MERGEABLE/UNSTABLE | +| #1006 | Michael-Han0608 | bounded JSON policy on HTTP SSE for DeepSeek Flash | no | policy-only | MERGEABLE/UNSTABLE | +| #1000 | WZBbiao | avoid false project config warnings | no | policy-only | MERGEABLE/UNSTABLE | +| #947 | WZBbiao | prevent Darwin rewrite stalls (streaming) | no | policy-only | MERGEABLE/UNSTABLE | +| #997 | Yuxin-Qiao | isolate usage-log fixtures from the real OpenCodex home | no | policy-only | MERGEABLE/UNSTABLE | +| #966 | Yuxin-Qiao | keep pre-connection DNS/network failures off account health (#914) | yes | policy-only | CONFLICTING/DIRTY; prior audit left mixed-5xx/redirect counterexamples | +| #928 | 0xWinner98 | repair sparse Responses snapshots (streaming) | yes | policy-only | CONFLICTING/DIRTY; changes requested | +| #933 | IMHinnG | enforce type:"object" on all tool parameters (openai-chat) | yes | FAIL (Ubuntu/Windows/macOS + enforce-target) | MERGEABLE/UNSTABLE | +| #940 | mouzhi | DeepSeek Responses UUID item ids for Codex | yes | policy-only | CONFLICTING/DIRTY; changes requested | +| #922 | luvs01 | isolate provider host transport health | no | policy-only | MERGEABLE/UNSTABLE; changes requested; #966 supersedes but is unfinished | +| #936 | lidge-jun | harden credential and runtime trust boundaries (rebase of #916) | yes | full PASS | CONFLICTING/DIRTY; needs explicit security review | +| #557 | lidge-jun | harden npm cache recovery preflight logs (update) | no | FAIL (Ubuntu/Windows; macOS cancelled) | MERGEABLE/UNSTABLE | + +## Improvement PRs + +| PR | Author | Subject | Note | +|---:|--------|---------|------| +| #999 | Yuxin-Qiao | docs: Desktop remote allowlist limitation (#241) | docs-only, draft | +| #569 | diegocantarero | post-sync readiness endpoint + bounded `ocx ready` wait | draft, CI PASS but CONFLICTING/DIRTY | +| #1002 | hanjianjun | configurable sidecar reasoning (vision) | includes ignored-CLI-setting defect | + +## Feature programs — out of scope (12) + +#811, #1003, #1004, #1005, #715, #961, #581, #998, #937, #872, #870, #812. + +## Issue↔PR coupling for the stack + +- #914 ↔ PR #966 (conflicting, audit counterexamples) and PR #922 (superseded + but unfinished) — the stack should land its own clean fix and route both + PRs to a disposition. +- #938 ↔ PR #940 (conflicting, changes requested) — same handling. +- #893 ↔ PR #928 (conflicting, changes requested). +- #875 ↔ PR #1006 (mergeable but policy-only CI; unverified against live + DeepSeek). diff --git a/devlog/_plan/260805_bug_stack_campaign/003_anthropic_400_research.md b/devlog/_plan/260805_bug_stack_campaign/003_anthropic_400_research.md new file mode 100644 index 000000000..c04c4f89c --- /dev/null +++ b/devlog/_plan/260805_bug_stack_campaign/003_anthropic_400_research.md @@ -0,0 +1,119 @@ +# 003 — Anthropic 400 on new-session spawn: research + +## Symptom + +2026-08-05 00:21-00:23 KST: three consecutive 400 `invalid_request_error` +responses against `anthropic/claude-opus-5` (effort high), all in conversation +`2c0f87c664e130b3927c4884fafd8283` (`ocx-mset3lcs-1i1`, `ocx-mset3rk6-1i6`, +`ocx-mset4i2p-1ik`), plus one in `7f8dbd4a06336b01da0d476fb65b38f2` at 00:23. +Same model+effort returned 200 for other conversations from 00:26 onward, +including 319K-620K-token inputs. Model alias failure, auth failure, and +context overflow are excluded. + +Local evidence gap: `usage.jsonl` records only `Provider error 400` — the +upstream error body was not retained for these responses-inbound failures +(compare 2026-08-01 rows, which preserved the full Anthropic error JSON). + +## Root cause — REFUTED hypothesis and what the live probes proved + +The initial lane-C hypothesis (unsigned foreign tool_use + adaptive thinking → +400) is **refuted by two live probes** run through the local proxy against the +real Anthropic API on 2026-08-05 ~01:00 KST: + +- Probe 1 (synthetic): inline `custom_tool_call` + `custom_tool_call_output` + history, `anthropic/claude-opus-5`, effort high → **200** (134 tokens). +- Probe 2 (exact parent state): the full 923,656-byte item list of + `resp_050cd54528dd6d7f…` (the last successful state at 00:21:12, 46s before + the first failure) + a new user message, same model/effort → **200** + (297,376 input tokens). + +Anthropic's current extended-thinking contract matches probe 1: adaptive +thinking does not require a thinking preface on replayed tool-use turns. +The history shape is exonerated. (A-gate audit round 1, blocker 1.) + +Transport-branch analysis (audit round 2, verified in code): the bare +`Provider error 400` does NOT come from the direct path — that path appends +`: ` even when the body is empty (`src/server/responses/core.ts:2822`, +trailing colon preserved). The exact bare shape is produced by the +**web-search bridge** (`src/web-search/loop.ts:460-484`): a body-read failure +returns status-only, and the formatted suffix requires +`prepared.responseAdapter.formatErrorBody`, **which the anthropic adapter +does not implement** — so on this path even a well-formed Anthropic JSON +error envelope is discarded and the client + ledger see only +`Provider error 400`. The images bridge has analogous status-only behavior. +Thread-spawn requests from the Codex app carry the `web_search` tool, and +routed (non-OpenAI) models dispatch through `runWithWebSearch` +(`src/server/responses/core.ts:2295-2310`), which returns before the normal +recovery loop. + +Conclusion: the failing requests almost certainly went through the web-search +bridge, and the 400's own cause (validation vs edge) is **unknowable from +surviving evidence because the bridge discarded the upstream body**. The +diagnosability defect is precisely located; the underlying 400 is reproduced +only if it recurs after the observability fix. Anthropic's error contract +(every API error carries a JSON envelope) makes "proxy discarded the body" +the more probable reading than "empty-body edge rejection", though both +remain possible. + +## Original research record (kept for provenance) + +Each failure happened exactly when resuming the next turn after a +`custom_tool_call_output`. Reconstruction of the failing body from saved +Responses state + rollout items shows: + +```json +{ "thinking": { "type": "adaptive" }, "output_config": { "effort": "high" }, "max_tokens": 24576 } +``` + +with history containing assistant turns of shape `[text, tool_use]` with **no +thinking/redacted_thinking block**, followed by `user: [tool_result]`. + +Mechanism (later refuted by probes 1-2 above): + +1. `previous_response_id` expansion merges stored input regardless of which + provider produced the earlier turns (`src/responses/state.ts:857-860`). +2. The Responses parser restores foreign `custom_tool_call` items as assistant + tool calls but cannot fabricate a valid Anthropic-signed thinking block + from OpenAI/Grok reasoning (`src/responses/parser.ts:510-517`, envelope + handling at 439-475; unsigned fallbacks fail + `isLikelyRealAnthropicThinkingSignature`, `src/adapters/anthropic.ts:240-244`). +3. The Anthropic adapter drops unsigned thinking parts + (`isLikelyRealAnthropicThinkingSignature`, `src/adapters/anthropic.ts:601`) + but still emits the `tool_use` blocks (`src/adapters/anthropic.ts:604-607`), + then turns adaptive thinking on for the whole request + (`src/adapters/anthropic.ts:834-835`). +4. Anthropic 400s: with thinking enabled, an assistant tool-use turn must be + preceded by a thinking/redacted_thinking block. + +Secondary hypothesis (prefixed model ID missing the adaptive gate on older +runtimes, fixed by `930efdf60`/`f728dc0fb`) does not fit: the running process +started 2026-08-04 22:45 KST from a checkout containing both fixes, and the +reconstructed body was already `adaptive`. + +Latent-path activation: `787bd1541` exposed Opus 5 in the catalog +(`src/providers/registry.ts:240`); the regression is not the alias but the +first time an adaptive-thinking model replayed foreign tool history. + +## "New session" interpretation + +`conversationId` is a log-correlation hash, not a continuation key +(`src/server/request-log-conversation.ts:30-38,64-75`). History replay is decided by +`previous_response_id`. The failing "new prompt/spawn" inherited the parent's +Responses chain — the proxy must not clear state on thread-spawn headers +(that would break intentional context inheritance). + +## Fix direction + +Revised after the probes (see `020_phase2_anthropic_400_fix.md`): + +1. Observability (phase 2): implement `formatErrorBody` on the anthropic + adapter so the web-search/images bridges surface the upstream error + message instead of discarding it; persisted `upstreamError` then flows + through the existing capture (`src/server/relay.ts:429` → + `src/server/request-log.ts:648`) with no production change there. +2. Resilience (deferred): any 400 retry decision waits until the observability + fix distinguishes empty-body edge rejections from discarded JSON bodies in + the wild. Premature retry design was audit-blocked twice and is dropped + from phase 2. +3. NO history flattening (refuted hypothesis); no signature fabrication; no + state clearing on thread spawn. diff --git a/devlog/_plan/260805_bug_stack_campaign/010_phase1_pr988_review_merge.md b/devlog/_plan/260805_bug_stack_campaign/010_phase1_pr988_review_merge.md new file mode 100644 index 000000000..fc57489a7 --- /dev/null +++ b/devlog/_plan/260805_bug_stack_campaign/010_phase1_pr988_review_merge.md @@ -0,0 +1,131 @@ +# 010 — Phase 1: PR #988 design review, small fixes, merge + +## Goal + +Land the only CI-clean bug PR (#988, Wibias: GUI providers quota/auth, Claude +pool toggle, combos/models layout, dev session bootstrap) after verifying it +against the repo's GUI design system. Fix only small deviations; no redesign. + +## Review protocol (B of this phase) + +This document is the review protocol; the concrete file-by-file verdicts and +any correction diff are appended here at this phase's own cycle P after the +actual PR diff is read, making 010 the complete record before its B starts. + +## PR head under review + +Head `3fc1eb52e25982606189219a3c0a6ced4352b1f3`, 24 files: + +- Session bootstrap: `gui/src/api.ts`, `gui/vite.config.ts`, + `src/server/gui-static.ts`, `src/server/index.ts`, + `gui/tests/api-auth-memory.test.ts`, `tests/server-management-auth.test.ts` +- Provider auth surfaces: `gui/src/components/provider-workspace/*` + (`ProviderAuthPanel`, `ProviderOverview`, `ProviderOverviewDashboard`, + `ProviderDetails`, `ProviderCapacityQuota`, + `AnthropicAccountPoolSettings`), `gui/src/components/CodexAccountPool.tsx`, + `tests/provider-workspace-auth.test.ts`, + `gui/tests/codex-account-pool-controller.test.ts` +- Loading/layout: `gui/src/pages/Combos.tsx`, `gui/src/pages/Models.tsx`, + `gui/tests/page-loading-contract.test.tsx` +- Presentation: `gui/src/components/combo-workspace-add-modal.tsx`, + `gui/src/components/combo-workspace-detail-panel.tsx`, + `gui/src/styles-combos-workspace.css`, + `gui/src/styles-dashboard-workspace.css`, + `gui/src/styles-models-workspace.css`, + `gui/src/styles/provider-overview-dashboard.css` + +Server-side files (`gui-static.ts`, `index.ts`) get the same security read as +the GUI: the `/opencodex-session` bootstrap must keep origin-bound session +minting and must not weaken packaged-build auth. + +1. Fetch the PR head into a local branch (`codex/review-pr988`) from + `origin/dev` — never commit on top of the detached other-unit HEAD. +2. Read the full diff (`gh pr diff 988`). +3. Check against the design contract: + - tokens: new spacing/typography uses existing CSS custom properties and + the shared token files, no one-off magic values where a token exists; + - UX states: loading/empty/error states keep their meaning + (UX-STATE-01) — the removed "Loading combos..." line must be replaced by + the documented `aria-busy` contract, not by silence; + - accessibility: the pool toggle is a real switch control with + `aria-pressed`/`role=switch`; capacity-warning contrast claim (WCAG AA) + holds in both themes; + - emoji ban: no emoji as UI visual elements; + - the `/opencodex-session` dev bootstrap does not weaken the session + contract for packaged builds (server-side change in + `src/server/gui-static.ts` / `src/server/index.ts` gets the same read as + the GUI files). +4. Verify: `bun run typecheck`, `bun run lint:gui`, `bun run build:gui`, + focused GUI tests (`cd gui && bun test tests`), plus + `bun test tests/provider-workspace-auth.test.ts tests/server-management-auth.test.ts`. + Full `bun run test` on `ssh lidge` if any non-GUI file was touched by a + fix. +5. If deviations are found: apply the smallest correction on the PR branch + (author's fork permitting) or carry a follow-up commit on top of the merge. +6. Merge: `gh pr merge 988 --repo lidge-jun/opencodex --merge` (user + authorized). Confirm the merge commit on `origin/dev`. + +## Scope boundary + +- IN: the 24 files in the PR diff, corrections within their existing lines. +- OUT: any redesign, token system changes, new components, other providers' + pages, the CodeRabbit docstring-coverage warning (repo has no docstring + convention; not a blocker). + +## Accept criteria + +- Review verdict recorded here with file:line citations for any fix applied. +- All gates in step 4 pass; evidence pasted into the phase record. +- `gh pr view 988 --json state` shows `MERGED`; `origin/dev` contains the + merge commit. + +## Review verdict (main + Franklin sol-medium independent pass) + +Security design: PASS. `/opencodex-session` responds only when +`issueGuiSession` succeeds (`src/server/index.ts:995-1004`); issuance +requires GET, local mode, allowed origin, loopback Host +(`src/server/management-auth.ts:213-237`); session use rechecks origin + +CSRF (`management-auth.ts:287-300`); bootstrap docs are no-store, +frame-denied, attribute-escaped (`src/server/gui-static.ts:59-105`). +`changeOrigin:false` keeps the Vite origin across bootstrap + `/api` +(`gui/vite.config.ts:19-27`); the client holds credentials in memory, +same-origin only (`gui/src/api.ts:28-35,81-87,108-120,145-156`). +Emoji ban: PASS. Contrast: PASS with margin after the fix (`--amber` = +6.26:1 light, 9.65:1 dark, better than the PR's hardcoded pair). + +Corrections applied as commit `e2d8ca430` on the PR branch +(maintainerCanModify, pushed after full pre-push gates): + +1. a11y FAIL (merge-blocking): silent revalidation was announced by + `aria-busy` alone — no live region. Added an sr-only + `role="status" aria-live="polite" aria-atomic="true"` span carrying + `common.loading` inside the Combos shell body + (`gui/src/pages/Combos.tsx`) and the Models combos pending strut + (`gui/src/pages/Models.tsx`), matching the `DataSurfaceStatus` contract + (`gui/src/components/data-surface.tsx:64-89`). + `gui/tests/page-loading-contract.test.tsx` now asserts the + role/live/sr-only/text contract, not just the attribute flip. +2. Claude pool toggle accessible name was state-only ("On"/"Off") — now the + stable `anthropicPool.title`; `aria-pressed` carries state + (`gui/src/components/provider-workspace/AnthropicAccountPoolSettings.tsx`). +3. Token bypasses: warning hex pair → `var(--amber)` + (`gui/src/styles/provider-overview-dashboard.css`); `72ch` → + `var(--prose-measure)`, stale status-strip comment rewritten + (`gui/src/styles-combos-workspace.css`); `48px` → `var(--space-12)` + (`gui/src/styles-models-workspace.css`). + +Left as-is (not merge-blocking): inline `8px`/`fontSize: 12` in the combo +forms (pre-existing file-local pattern); extra rendered-toggle and live +cross-origin negative tests (unit-level origin/CSRF negatives already cover +the gate; follow-up candidate). + +## Gate evidence (pre-push, local) + +- `bun run typecheck` — 0 errors +- `bun run lint:gui` — pass +- `bun run build:gui` — pass +- `cd gui && bun test tests` — 584 pass / 0 fail +- `bun test tests/provider-workspace-auth.test.ts tests/server-management-auth.test.ts` + — 35 pass / 0 fail +- Pre-push hook (typecheck + lint + full `bun run test` + privacy:scan + + doctor) — passed, push exit 0 diff --git a/devlog/_plan/260805_bug_stack_campaign/020_phase2_anthropic_400_fix.md b/devlog/_plan/260805_bug_stack_campaign/020_phase2_anthropic_400_fix.md new file mode 100644 index 000000000..ad452f91d --- /dev/null +++ b/devlog/_plan/260805_bug_stack_campaign/020_phase2_anthropic_400_fix.md @@ -0,0 +1,73 @@ +# 020 — Phase 2: anthropic error fidelity in the sidecar bridges + +Evidence basis: `003_anthropic_400_research.md`. The bare `Provider error 400` +that made the incident undiagnosable comes from the web-search/images bridge +error path (`src/web-search/loop.ts:460-484`, `src/images/loop.ts:577+`), +which only formats the upstream body when the adapter implements +`formatErrorBody` (`src/adapters/base.ts:24`). The anthropic adapter lacks +it; openai-chat (`src/adapters/openai-chat.ts:34,729`), google +(`src/adapters/google.ts:314`), and kiro (`src/adapters/kiro.ts:1899`) have +one. Scope: error fidelity only. Any retry policy is deferred until the +ledger distinguishes real upstream 400 reasons in the wild (003 fix +direction 2). + +## Diff-level plan + +### MODIFY `src/adapters/anthropic.ts` + +1. ADD exported `formatAnthropicErrorBody(status, headers, payloadText)` next + to the other module-level helpers. Contract mirrors + `formatOpenAIChatErrorBody` (`src/adapters/openai-chat.ts:34-44`): + - JSON.parse the payload; on parse failure return `""` (HTML/non-JSON is + never echoed). + - Extract the Anthropic envelope `{ type: "error", error: { type, message } }` + → render as `": "`; tolerate a bare + `{ error: { message } }` and a string `error` field the same way + `extractErrorDetail` does. + - Pass through `redactSecretString`, bound to 400 chars (bridge slices to + 400 again — keep the producer bound identical to the openai-chat one). +2. REGISTER it in the adapter object returned by `createAnthropicAdapter` + (`src/adapters/anthropic.ts:773+`): `formatErrorBody: formatAnthropicErrorBody,` + beside `name: "anthropic"`. The `ProviderAdapter` interface already + declares the optional hook (`src/adapters/base.ts:24`) — no interface + change, no other caller change. Both bridges pick it up through their + existing `prepared.responseAdapter.formatErrorBody` checks. + +### Tests — extend the anthropic adapter suite (or new `tests/anthropic-error-body.test.ts`) + +3. Unit: `formatAnthropicErrorBody` on a real Anthropic 400 envelope + (`{"type":"error","error":{"type":"invalid_request_error","message":"…"}}`) + → `"invalid_request_error: …"`; on HTML → `""`; on JSON without error + fields → `""`; secret-shaped content inside `message` is redacted. +4. Bridge integration: fake anthropic upstream returning a JSON 400 through + the web-search loop → the client-facing JSON error response carries + `Provider error 400: invalid_request_error: …` (not the bare status; the + thrown `LoopError` is converted by `runWithWebSearch`, + `src/web-search/loop.ts:657-662`). Follow the existing web-search loop + test harness (find the suite covering `loop.ts` error paths and extend + it). +5. Persistence integration: same fake 400 through `/v1/responses` with + web_search enabled against a routed anthropic model → the usage entry's + `upstreamError` contains the upstream message (proves the + `relay.ts:429` → `request-log.ts:648` capture chain end to end; no + production change expected in those files). +6. Pin the read-failure branch: upstream body that fails to read still + produces the bare status-only message (today's deliberate behavior, + `loop.ts:466-471`). + +## Scope boundary + +- IN: `src/adapters/anthropic.ts`, new/extended tests. +- OUT: `request-log.ts`/`relay.ts` (capture chain already works — test 5 + pins it), retry policy, combo classification, parser/state changes, + formatErrorBody for other adapters that lack it (separate follow-up if + wanted). + +## Accept criteria (activation scenarios) + +- Test 4 activates the exact bridge branch that produced the incident's bare + message and proves the suffix now appears. +- Test 5 proves the ledger becomes diagnosable without manual log spelunking. +- `bun run typecheck` 0 errors; focused suites green; full `bun run test` on + `ssh lidge` green against the warm baseline; `bun run privacy:scan` passes + (redaction is part of test 3). diff --git a/devlog/_plan/260805_bug_stack_campaign/030_issue914_dns_transport_attribution.md b/devlog/_plan/260805_bug_stack_campaign/030_issue914_dns_transport_attribution.md new file mode 100644 index 000000000..e4533b636 --- /dev/null +++ b/devlog/_plan/260805_bug_stack_campaign/030_issue914_dns_transport_attribution.md @@ -0,0 +1,101 @@ +# 030 — Issue #914: pre-connection transport failures must not rotate pool accounts + +core.ts cluster 1/3. Research: explorer batch D (verified citations below). + +## Verified current state + +- Regular Responses maps every non-timeout fetch rejection to `connect_error` + against the selected pool account: `src/server/responses/core.ts:1734,1737`. +- Native Compact does the same on primary and bounded alternate sends: + `src/server/responses/compact.ts:410,480`. +- `connect_error`, timeout, and 5xx share one account-transient class: + `src/codex/routing.ts:323`; threshold creates soft-avoid, drops affinities, + advances the active account: `src/codex/routing.ts:1615,1650`. +- `fetchWithTransientRetry()` discards earlier 5xx evidence when a later + retry rejects: `src/lib/upstream-retry.ts:314,337`. +- Quota-probe lease release owner: `src/codex/auth-context.ts:82`. + +## Classification contract + +| Evidence | Account health | +|---|---| +| `ENOTFOUND`, `EAI_AGAIN`, `ENETUNREACH`, `ENETDOWN`, `EHOSTUNREACH`, `ECONNREFUSED`; Bun `ConnectionRefused`/`FailedToOpenSocket` | neutral — record provider-origin host ledger only | +| `ECONNRESET`, `EPIPE`, established/read-then-close | keep `connect_error` | +| TLS/cert/handshake or unknown rejection | keep `connect_error` | +| `TimeoutError` | keep `timeout` | +| client abort | 499 path, no health evidence | +| any real HTTP response (incl. manual 3xx) | clears the host streak; existing status policy applies | +| 5xx → then reachability rejection | account-attributed (credential path was reached) | + +Never classify from message text; walk at most three `Error.cause` links, +stop on cycles/non-Errors. + +## Diff-level plan + +ADD `src/lib/upstream-reachability.ts` — `isPreConnectReachabilityError()`, +`classifyTransportFailureKind()`, stable code extraction, canonical +provider-origin keying. + +ADD `src/codex/upstream-host-health.ts` — observational ledger keyed by +`(provider, canonical origin)`; record/reset/window helpers only, NO circuit +breaker in this issue. Retention bound (audit round 3): maximum 128 entries; +on overflow, prune the stalest entries by last-touch timestamp before +inserting; entries idle beyond the ledger window are reconciled away on the +next record. A churn test proves cardinality stays bounded across repeated +provider/base-URL changes for the process lifetime. + +MODIFY `src/lib/upstream-retry.ts` — attach ordered prior-attempt evidence +(5xx, credential-visible resets) to the terminal rejection via an +observation callback, so mixed sequences classify correctly. + +MODIFY `src/codex/routing.ts` — add a neutral outcome branch: releases only a +matching account/scoped probe lease and returns before account streak, +soft-avoid, affinity, or active-account mutation. Add the neutral variant to +`CodexUpstreamOutcome` and the downstream recorder contracts so the new +outcome is type-checked end to end (audit round 3 note). + +MODIFY `src/server/responses/core.ts` — `transportFailureResponse` classifies +via the new helper; neutral → account-neutral settlement + host-ledger +record; `redirect: "manual"` on credential-bearing sends (primary, 429 +replay, `retryCodexPoolOnAlternateAccount`), preserving 3xx + `Location`; +any real HTTP response clears the host streak. + +MODIFY `src/server/responses/compact.ts` — same classification in primary and +alternate catches; same manual-redirect policy in `sendCompactAttempt`. + +DOCS: five provider-configuration locale files, scoped to regular Responses + +native Compact (no sidecar claims). + +## Contributor PR equivalence (040/130 gate input) + +- #966 (`78c824dd`): closest semantic source — extract classifier/retry + semantics + regular/Compact tests; do NOT inherit its 22-file sidecar + blast radius. +- #922 (`d6c37343`): not equivalent — host-health module + ordered + attempt observations are useful; its admission/circuit-breaking, host-only + timeouts, and redirect-to-502 are policy expansions beyond #914. + +## Tests / activation + +Activation: two pool accounts, thread pinned to A, threshold 3; fake fetch +rejects three concurrent requests with Bun `ConnectionRefused` → all return +502, A keeps health/affinity/active-account, host ledger records 3; restore +fetch, same thread still uses A; repeat via `/v1/responses/compact`. + +Matrix: every accepted code at cause-depth 0-2, depth-3, cycle, non-Error, +message-only negative; reset/EPIPE/TLS/timeout/unknown account-attributed; +`503→ConnectionRefused` and `ECONNRESET→ConnectionRefused` account-attributed; +all five send sites; manual 307 (credential once at origin, dead target never +contacted); concurrent neutral failures; owned probe lease released, +unrelated untouched; real Bun dead-port activation. + +ADD `tests/upstream-reachability.test.ts`, +`tests/issue-914-transport-attribution.test.ts`; extend +`tests/upstream-transient-retry.test.ts`, `tests/server-auth.test.ts`, +`tests/responses-compaction-routing.test.ts`. + +## Accept criteria + +- Activation scenario passes on fake-fetch and real dead-port Bun runs. +- Gates: focused suites + `bun run typecheck` + `bun run privacy:scan` + full + `bun run test` on ssh lidge. diff --git a/devlog/_plan/260805_bug_stack_campaign/040_issue893_sparse_snapshot_repair.md b/devlog/_plan/260805_bug_stack_campaign/040_issue893_sparse_snapshot_repair.md new file mode 100644 index 000000000..29e0c685d --- /dev/null +++ b/devlog/_plan/260805_bug_stack_campaign/040_issue893_sparse_snapshot_repair.md @@ -0,0 +1,89 @@ +# 040 — Issue #893: sparse lifecycle snapshots Codex clients never commit + +core.ts cluster 2/3. Research: explorer batch D. + +## Verified current state + +- Client SSE composes only image restore + item-ID repair: + `src/server/responses/core.ts:1967,1971`; rewrite enters eager and tee + relays (`core.ts:2018,2115`); non-stream JSON is inspected raw then only + image-restored (`core.ts:2142`). +- The rewrite contract is one-payload-in/one-payload-out and cannot inject + events: `src/server/sse-payload-rewrite.ts:10`, `src/server/relay-eager.ts:36`. +- Codex does not commit text without closing events (`src/bridge.ts:509`); + canonical message lifecycle: `output_item.added` → `content_part.added` → + `output_text.delta` → `output_text.done` → `content_part.done` → + `output_item.done` → `completed` (`bridge.ts:838,517,1130`). +- The issue fixture (`created → output_item.added → output_text.delta → + completed`) lacks `content_part.added` and all three closing events. + +## Diff-level plan + +ADD `src/server/responses-snapshot-repair.ts` — provider-opt-in state +machine: repair lifecycle snapshot fields, track unambiguous open +message/reasoning items + accumulated text, expand one upstream event into +zero-or-more canonical client events. + +MODIFY `src/types.ts` — `responsesSnapshotRepair?: boolean` beside +`responsesItemIdRepair`. MODIFY `src/config.ts` — strict optional boolean. +MODIFY `src/server/auth-cors.ts` — permit the option on canonical OpenAI +pool/direct writes without weakening seed/custom-forward restrictions. + +MODIFY `src/server/sse-payload-rewrite.ts` — preserve `SsePayloadRewrite`; +add a block/event rewrite contract that can emit multiple SSE blocks with +correct `event:` names; untouched events stay byte-identical. + +MODIFY `src/server/relay-eager.ts` — accept the block/event rewriter; +inspection keeps receiving raw chunks pre-rewrite. + +MODIFY `src/server/responses/core.ts` — include the opt-in in +`needsClientRewrite`; compose image restore → item-ID repair → lifecycle +repair in both eager and tee branches; JSON snapshot repair only after raw +inspection + continuation persistence. + +Synthesis policy: backfill lifecycle snapshot fields (`status`, `output`, +`parallel_tool_calls`, `tool_choice`, `tools` from request values); message +items get `status`/`role`/`content`; reasoning gets `summary`; inject +`content_part.added` after a repaired item-added; inject +`output_text.done`/`content_part.done`/`output_item.done` immediately before +`response.completed`; reconstruct terminal `output` only when absent — +explicit `output: []` is authoritative. Ambiguous/gapped/malformed/oversized +or contradictory shape → fail closed to canonical empty output, release +retained budget. Bounds reuse existing relay collector limits + translator +budget. + +## Contributor PR equivalence + +- #928 (`6261d3dc`): strong source for config shape, field backfills, JSON + path, explicit-empty preservation, bounds. NOT equivalent to a proven fix: + its exact-issue test expects no `output_item.done` and terminal + `output: []` — normalization, not commitment. Unproven collapse + hypothesis: if real Codex activation shows a canonical empty terminal + snapshot commits the stream, the implementation collapses to the narrower + field-only design. + +## Tests / activation + +Primary fixture: the exact issue stream → client sees the full canonical +sequence and one committed assistant message `"hello"`, terminal `output[0]` +matching the done item. Feed the repaired stream into the Codex-facing +lifecycle consumer (not JSON-shape assertions only); real Codex CLI/App +against a fake gateway shows the final answer and a successful continuation. + +Matrix: opt-in false byte-passthrough; explicit `output: []` preserved; +valid/future fields preserved; missing vs malformed field tables; message / +reasoning / function-call / multi-item streams; ambiguous items fail closed; +duplicate done events don't double-close; bounds/budget release on +completed/failed/incomplete; cancellation; malformed JSON; EOF; eager/tee +parity incl. Windows; composition with image restore + item-ID repair; JSON +mode field repair without altering raw inspection. + +ADD `tests/responses-snapshot-repair.test.ts`; extend +`tests/sse-payload-rewrite.test.ts`, `tests/relay-eager.test.ts`, +`tests/config.test.ts`, `tests/management-provider-validation.test.ts`; five +provider-config locale docs. + +## Accept criteria + +- Primary fixture + Codex lifecycle consumer test pass; eager/tee parity. +- Gates as 030. diff --git a/devlog/_plan/260805_bug_stack_campaign/050_issue875_deepseek_flash_stall.md b/devlog/_plan/260805_bug_stack_campaign/050_issue875_deepseek_flash_stall.md new file mode 100644 index 000000000..664fcd469 --- /dev/null +++ b/devlog/_plan/260805_bug_stack_campaign/050_issue875_deepseek_flash_stall.md @@ -0,0 +1,83 @@ +# 050 — Issue #875: DeepSeek V4 Flash HTTP/SSE stall after tool calls + +core.ts cluster 3/3. Research: explorer batch E (read PR #1006 fully). +Decision: **supersede PR #1006 as a diff, retain contributor attribution and +its core policy** (bounded upstream JSON for this model). + +## Verified current state + +- Bounded upstream JSON applies only when `inboundTransport === "websocket"`: + `src/server/responses/core.ts:846`. The registry flag only disables WS + upstream streaming: `src/providers/registry.ts:1143`. Ordinary HTTP keeps + `stream:true`, classifies the response as SSE (`core.ts:1877`), enters the + native relay (`core.ts:1967`), which closes early only on a protocol + terminal (`src/server/relay.ts:196`). A stream emitting tool output but no + usable terminal hangs — the reporter's `websockets:false, bodyKind:sse` + reproduction at e44d234f0. +- #1006's mechanism is causally right (force `stream:false`) but: duplicates + the JSON→event algorithm already in `sendResponsesJsonAsEvents()` + (`src/server/ws-bridge.ts:295`); adds persistent parser state + `_clientRequestedStream`; omits the `[DONE]` trailer; its test never proves + the old path hangs; and bounded JSON would bypass the SSE item-ID repair + branch (undermines #938). + +## Diff-level plan + +MODIFY `src/providers/registry.ts` — rename the registry-only WS concept to +transport-neutral `modelResponsesUpstreamStreaming`; DeepSeek Flash stays +`false`; apply only after final wire resolution and only when the resolved +adapter is `openai-responses`. + +ADD `src/server/responses-json-events.ts` — extract the pure event sequence +from `sendResponsesJsonAsEvents()` (`response.created` → one +`response.output_item.done` per output item → status-preserving terminal) + +an SSE serializer appending exactly one `data: [DONE]\n\n`; optional +client-facing payload rewrite hook (060 composes ID normalization here). + +MODIFY `src/server/ws-bridge.ts` — use the shared helper; preserve payload +observation + terminal callbacks. + +MODIFY `src/server/responses/core.ts` — capture +`const clientRequestedStream = parsed.stream` immediately before +`applyFinalRouteRequestNormalization()` (`core.ts:1479`); after final adapter +resolution apply the transport-neutral policy (`stream:false` upstream); in +the bounded JSON branch (`core.ts:2124`): parse once after existing +byte/inactivity limits; record the raw response first; apply client-facing +ID normalization; return JSON unchanged for non-streaming clients and WS; +serialize terminal SSE + `[DONE]` for HTTP clients that requested streaming; +strip stale `content-length`/`content-encoding`, set SSE content type + +no-store. + +MODIFY `tests/deepseek-inbound-wire.test.ts` — replace the HTTP-keeps- +streaming assertion (`:135`); add the stall activation below. +MODIFY `tests/ws-endpoint.test.ts` — WS/HTTP serializer parity over +completed/failed/incomplete/function-call/empty outputs. +ADD `tests/responses-json-events.test.ts`. +MODIFY `structure/04_transports-and-sidecars.md` — document the +transport-neutral policy. + +## Tests / activation + +Decisive regression: fake DeepSeek fetch — if the outgoing body has +`stream:true`, return an SSE stream with a completed `function_call` but no +terminal frame and never close (old code: times out); if `stream:false`, +return the completed JSON. Fixed code: downstream emits `created`, +`output_item.done(function_call)`, `completed`, `[DONE]`, closes before a +200-500ms deadline. + +Matrix: plain message; one/multiple function calls; failed/incomplete status +preservation; malformed/oversized/total-timeout/inactivity-timeout JSON; +chat+anthropic inbound stays streaming on `/chat/completions`; WS unchanged; +UUID message/reasoning IDs normalized on both bounded-JSON paths (with 060). + +## Risks + +- DeepSeek Flash loses progressive token delivery — intended provider + reliability tradeoff, stated in the PR body. +- HTTP event parity must be tested (WS proves `output_item.done` suffices for + function calls). +- Bounded-body limits stay mandatory (HTTP now materializes routinely). + +## Accept criteria + +- Stall activation test red→green; WS/HTTP parity green; gates as 030. diff --git a/devlog/_plan/260805_bug_stack_campaign/060_issue938_uuid_item_ids.md b/devlog/_plan/260805_bug_stack_campaign/060_issue938_uuid_item_ids.md new file mode 100644 index 000000000..0326cf24e --- /dev/null +++ b/devlog/_plan/260805_bug_stack_campaign/060_issue938_uuid_item_ids.md @@ -0,0 +1,77 @@ +# 060 — Issue #938: UUID item IDs leave Codex stuck on Thinking + +Composes with 050's JSON→client-event boundary (implement after 050). +Research: explorer batch E (read PR #940 fully, 1473 lines). +Decision: **supersede PR #940 with a narrow reimplementation.** + +## Verified current state + +- Repair state knows only configured placeholder sets + missing-terminal + behavior: `src/server/responses-item-id-repair.ts:53`; `rememberMappedId()` + mints only for exact placeholders (`:84`); activation recognizes the three + existing options (`:220`). Public type + strict schema have no invalid-ID + option: `src/types.ts:920`, `src/config.ts:596`. Rewrite is client-facing, + confined to the SSE branch (`core.ts:1967`), raw snapshots preserved. +- #940's ID-prefix idea is equivalent to the needed fix, but the 15-file, + ~750-line PR bundles response-ID rewrites + continuation aliases, dropped + `response.in_progress`/raw reasoning events, synthesized envelopes, + logprobs stripping, relay surgery, eager-relay changes, `[DONE]` synthesis — + all beyond #938's acceptance boundary. Draft, conflicting, changes + requested. + +## Diff-level plan + +MODIFY `src/types.ts` — add `repairInvalidIds?: boolean` to +`ResponsesItemIdRepairConfig`: for message/reasoning items, an existing ID is +invalid when it lacks the expected `msg_`/`rs_` prefix. + +MODIFY `src/config.ts` — add the boolean to the strict provider schema +(unknown nested keys still rejected). + +MODIFY `src/server/responses-item-id-repair.ts` — extend state with +`repairInvalidIds`; extract one type-scoped resolver used by item objects and +lifecycle `item_id` fields; mint when exact-placeholder OR +(`repairInvalidIds` and wrong type prefix); maps stay keyed by item type + +`output_index`; NEVER rewrite `function_call.id`, `function_call.item_id`, +or `call_id`; recognize the option in `hasResponsesItemIdRepair()`; do NOT +rewrite `response.id` in this patch (no evidence response IDs block +completion; avoids previous-response aliasing). + +MODIFY `src/providers/registry.ts` — registry-only `responsesItemIdRepair`; +enable `{ repairInvalidIds: true, repairMissingTerminalIds: true }` for +built-in DeepSeek; never seeded into saved config. + +MODIFY `src/providers/derive.ts` — fill the registry policy only when the +runtime provider has no explicit policy; deep-clone `message`/`reasoning` +arrays. + +MODIFY `src/server/responses/core.ts` — keep raw-SSE-only rewrite; ALSO +apply the same client normalization when 050 converts bounded JSON to HTTP +SSE or WS events; raw state recording happens before normalization. + +MODIFY `tests/config.test.ts`, `tests/responses-item-id-repair.test.ts`, +`tests/service-tier-capability.test.ts`. +ADD `tests/deepseek-responses-item-id-repair.test.ts`. +DOCS: five provider-reference locale files. + +## Tests / activation + +Primary stream: UUID reasoning item added→delta→done; UUID message item +added→delta→done; UUIDs repeated in `response.completed.response.output`; +terminal + `[DONE]`. Assert stable `rs_ocx_*`/`msg_ocx_*` IDs across every +lifecycle occurrence; function-call ID/`call_id` byte-unchanged; raw +snapshot replayable; client stream reaches terminal. + +Matrix: invalid UUIDs; canonical IDs unchanged; legacy placeholders still +repaired; missing terminal IDs backfilled; type separation on shared +`output_index`; delta-before-added; disabled → byte passthrough; +malformed/unknown events unchanged; translator-budget bounds; explicit +provider config overrides registry default; 050 bounded-JSON HTTP + WS +paths normalize identically. + +Ablation: ID-only normalization first; only if the reporter still stalls, +evaluate #940's reasoning-event conversion or response-ID aliasing separately. + +## Accept criteria + +- Primary stream + matrix green; composition with 050 proven; gates as 030. diff --git a/devlog/_plan/260805_bug_stack_campaign/070_issue907_jawcode_prices.md b/devlog/_plan/260805_bug_stack_campaign/070_issue907_jawcode_prices.md new file mode 100644 index 000000000..c9e5a3065 --- /dev/null +++ b/devlog/_plan/260805_bug_stack_campaign/070_issue907_jawcode_prices.md @@ -0,0 +1,64 @@ +# 070 — Issue #907: stale jawcode prices for gpt-5.6-terra/luna + +Independent lane. Research: explorer batch F. Authority note: the generated +artifact must not be hand-edited (`src/generated/jawcode-model-metadata.ts:2`); +the source of truth is the EXTERNAL jawcode checkout — writing it needs the +user's repo, so this phase has a two-repo write boundary (see below). + +## Verified current state + +- `src/generated/jawcode-model-metadata.ts:47`: Luna `1/6/0.1/1.25`, Terra + `2.5/15/0.25/3.125` (pre-cut). +- Generator: `scripts/generate-jawcode-metadata.ts:22` — + `JAWCODE_MODELS_JSON` or `../jawcode/packages/ai/src/models.json`; copies + cost fields untransformed (`:82`). +- Local jawcode source `/Users/jun/Developer/new/700_projects/jawcode/packages/ai/src/models.json` + is stale in four bundles: `github-copilot` (:10667,:10723), `openai` + (:59069,:59121), `openai-codex` (:59822,:59876), `opencode-zen` + (:61734,:61784). +- Current official short-context tuples (input/output/cacheRead/cacheWrite): + Terra `2/12/0.20/2.50`, Luna `0.20/1.20/0.02/0.25`. +- `src/usage/expected-prices.ts:32` already has the correct base tuples, but + nonzero jawcode rows take precedence (`src/usage/cost.ts:207`). +- Fast multipliers coupled to stale bases: Terra `1.6`, Luna `0.4` + (`expected-prices.ts:163`); official Fast = 2× standard for both. + +## Diff-level plan + +EXTERNAL FIRST (jawcode repo, scoped clean branch — its checkout is dirty): +`packages/ai/src/models.json` — correct Terra + Luna in all four bundles; Sol +unchanged. + +MODIFY `src/generated/jawcode-model-metadata.ts` — REGENERATE only: +`JAWCODE_MODELS_JSON=/Users/jun/Developer/new/700_projects/jawcode/packages/ai/src/models.json bun run generate:jawcode-metadata`; +inspect the generated diff — only intended price changes allowed. + +MODIFY `src/usage/expected-prices.ts` — Terra and Luna +`PRIORITY_MULTIPLIERS` → `2`; Sol stays `2`. + +MODIFY `tests/usage-cost.test.ts` — table-driven +`resolveMatchedPrice("openai", model)` pinning Sol/Terra/Luna four-tuples; +Luna Fast: standard `$0.064`, Fast `$0.128`; Terra: `$0.64` / `$1.28`. + +No change: `scripts/generate-jawcode-metadata.ts`, +`tests/jawcode-metadata-sync.test.ts` (already byte-compares regeneration). + +## Tests / activation + +Identical short-context usage against OpenAI Luna/Terra → Logs/API estimates +use the post-cut tuple; `service_tier=priority` → published Fast totals. +Matrix: exact four-tuples; cache-read + cache-write inputs; long-context +boundaries stay green; regeneration sync with explicit `JAWCODE_MODELS_JSON`. + +## Risks + +- Dirty jawcode checkout: use a scoped clean branch/worktree there; never + sweep unrelated changes. +- Fixing bases without multipliers silently undercharges Fast. +- Generated-only patch would be reverted by the next regeneration and + contradicts the sync guard — regeneration is mandatory. + +## Accept criteria + +- Price snapshot tests green; generated diff contains only intended changes; + gates as 030. diff --git a/devlog/_plan/260805_bug_stack_campaign/080_issue1007_login_url_flush.md b/devlog/_plan/260805_bug_stack_campaign/080_issue1007_login_url_flush.md new file mode 100644 index 000000000..70cfef6e8 --- /dev/null +++ b/devlog/_plan/260805_bug_stack_campaign/080_issue1007_login_url_flush.md @@ -0,0 +1,59 @@ +# 080 — Issue #1007: login authorization URL withheld under non-TTY stdout + +Independent lane. Research: explorer batch F. + +## Verified current state + +- Codex login prints URL/instructions via `console.log` then enters up to + 150 two-second polls: `src/cli/account-auth.ts:79`; generic OAuth repeats + the shape at `:118`. `RuntimeApiDeps` has stdin injection but no stdout + writer (`src/cli/runtime-api.ts:17`). Existing tests capture `console.log` + in-process, never a real pipe (`tests/cli-account.test.ts:306,367`). +- Diagnostic on pinned Bun 1.3.14: `console.log`, `process.stdout.write`, + `writeSync`, and awaited `Bun.write(Bun.stdout,…)` were all pipe-readable + within 500ms on the test machine — the sync fd write is still the + strongest regression-resistant contract. + +## Diff-level plan + +MODIFY `src/cli/account-auth.ts`: +- Import `writeSync` from `node:fs`. +- ADD local `writeStdoutFully(text)`: UTF-8 encode; loop on partial + synchronous writes to fd 1; reject zero-byte progress. +- Build ONE initial human-readable block per login branch (URL + + instructions + flow id together — never the URL alone, which would + reorder output) and synchronously write it before code submission, + `--no-wait` handling, or polling. +- `--json` mode emits no human block. + +MODIFY `tests/cli-account.test.ts` — parent pipe-timing regression + +JSON/no-wait assertions. + +ADD `tests/helpers/account-login-pipe-child.ts` — invokes the real account +command handler with a fake management fetch; returns a URL/flow, then keeps +status unresolved long enough for the parent to prove the child is still +authenticating. + +No production stdout-injection abstraction (test-only need). + +## Tests / activation + +Spawn the child with `stdout: "pipe"`, read incrementally with a sub-second +deadline: the authorization URL arrives while the child is still polling. +Matrix: Codex account URL under pipe; generic OAuth URL under pipe; stdout +redirected to a file; TTY ordering preserved; `--json --no-wait` emits one +valid JSON doc and no human prefix; missing URL keeps existing flow-ID/error +behavior; closed consumer/EPIPE → controlled CLI failure, not hidden +polling. + +## Risks + +- Sync writes can block on a full pipe — the bounded login block is far + below pipe capacity. +- Partial writes handled explicitly; one unverified `writeSync` is weaker on + unusual runtimes. +- The listener-lifetime concern in #1007 is out of scope. + +## Accept criteria + +- Pipe regression red→green; gates as 030. diff --git a/devlog/_plan/260805_bug_stack_campaign/090_issue1001_forced_answer_validation.md b/devlog/_plan/260805_bug_stack_campaign/090_issue1001_forced_answer_validation.md new file mode 100644 index 000000000..8939ea87c --- /dev/null +++ b/devlog/_plan/260805_bug_stack_campaign/090_issue1001_forced_answer_validation.md @@ -0,0 +1,69 @@ +# 090 — Issue #1001: forced-answer pass accepts malformed tool calls + +Independent lane. Research: explorer batch F. + +## Verified current state + +- `scanEventsForWebSearch` sets `hasRealToolCall=true` for any + non-`web_search` name including `""`: `src/web-search/loop.ts:62`; no + identity or closing-event requirement. `consumeIterationEvents` validates + one terminal event then returns (`:506`). The forced pass has no + enforcement (`:279`); `forceAnswer` unconditionally ends the loop (`:677`) + and replays/returns without checking visible text or call validity + (`:695-705`). Existing test helpers always emit `"final answer"` on later + passes (`tests/web-search.test.ts:298,1315`) — the empty case is uncovered. + +## Contract + +Well-formed real tool call: nonblank `id`, nonblank `name`, one structurally +matched `tool_call_start … tool_call_end`. Arguments stay bridge-owned +(freeform tools can't universally require JSON). + +Successful forced-answer `done`: non-whitespace non-commentary `text_delta`, +OR at least one well-formed real non-web tool call. Thinking-only, +hallucinated web-search calls, and malformed fragments do not qualify. + +## Diff-level plan + +MODIFY `src/web-search/loop.ts`: +- Extend `scanEventsForWebSearch` with `hasMalformedToolCall`; pending-call + flushing preserves event order and distinguishes completed vs unterminated + calls; blank identity, orphan delta/end, nested replacement of an unclosed + call, and EOF/terminal before `tool_call_end` are malformed; + `hasRealToolCall` only after a structurally complete call with nonblank + id/name. +- Add a small visible-text predicate over buffered passthrough text + (commentary excluded). +- When `forceAnswer` ends with `done`: reject malformed calls and reject a + pass with neither visible text nor a valid real call — emit + `LoopError(502, "forced-answer pass produced no usable assistant output")`, + which the existing catch converts to in-stream `response.failed`. +- Preserve `incomplete` and upstream error semantics; never log malformed + arguments/IDs (may contain sensitive material). + +MODIFY `tests/web-search.test.ts` — scanner-level structural cases + the +issue's deterministic two-pass SSE regression. + +## Tests / activation + +Primary: pass 1 emits a batched `web_search` consuming the budget; forced +pass emits `id=""`, `name=""`, arguments, `tool_call_end`, `done` → assert +one `response.failed`, no `response.completed`, no completed malformed call. + +Matrix: empty id/name; whitespace-only name; unterminated start; orphan +delta/end; nested start; hallucinated valid `web_search` on a forced pass; +`done` with no text/tool; whitespace-only text; thinking-only; nonblank +text + `done` succeeds; valid closed non-web call without text allowed; +malformed JSON arguments remain bridge-owned; `incomplete` stays +`incomplete`. + +## Risks + +- Requiring text unconditionally would break legitimate post-search + shell/apply-patch calls — the valid-real-tool alternative is necessary. +- No retry: it would spend tokens and complicate hard-cap accounting; + explicit failure is bounded and allowed by the issue. + +## Accept criteria + +- Primary regression red→green; gates as 030. diff --git a/devlog/_plan/260805_bug_stack_campaign/100_issue992_routed_context_window.md b/devlog/_plan/260805_bug_stack_campaign/100_issue992_routed_context_window.md new file mode 100644 index 000000000..74abf841c --- /dev/null +++ b/devlog/_plan/260805_bug_stack_campaign/100_issue992_routed_context_window.md @@ -0,0 +1,59 @@ +# 100 — Issue #992: routed models must not inherit template context_window + +Independent lane. Research: explorer batch G. + +## Verified current state + +- `deriveEntry()` deep-clones the native template + (`src/codex/catalog/sync.ts:205-213`); routed entries apply optional + metadata (`:216-228`) without clearing template context fields first. +- `applyCatalogModelMetadata()` overwrites only when the window is known + (`src/codex/catalog/effort.ts:125-132`). +- `ensureStrictCatalogFields()` supplies the conservative fallback + `128000 / 128000 / 115200` (`src/codex/catalog/parsing.ts:290-301`) — the + desired owner of last resort; no `parsing.ts` change. +- Provider caps deliberately don't invent unknown context + (`src/providers/context-cap.ts:24`; pinned at + `tests/codex-catalog.test.ts:2479-2545`). Decision: explicit 128k + conservative default — not omission (codex-rs yields `None`, no + auto-compaction) and never cap-as-fallback. + +## Diff-level plan + +MODIFY `src/codex/catalog/sync.ts` — at the start of the `if (isRouted)` +branch (`:216`), before `applyJawcodeCatalogMetadata()` / +`applyCatalogModelMetadata()`: + +```ts +delete e.context_window; +delete e.max_context_window; +delete e.auto_compact_token_limit; +``` + +Known live/configured/Jawcode metadata then restores exact values; otherwise +the strict-fields fallback supplies the conservative triple. + +MODIFY `tests/codex-catalog.test.ts` — regression beside `unknown routed +entries receive conservative strict catalog defaults` (`:2290`). + +## Tests / activation + +`/models` returns `{data:[{id:"relay-model"}]}` with a 372000 native +template → routed entry becomes `128000/128000/115200`, never `372000`. +Matrix: ID-only routed model → conservative triple; + provider cap 950000 → +still 128k (cap doesn't invent capacity); configured +`contextWindow`/`modelContextWindows` → configured value + 90% compaction; +discovered 500k + 350k cap → 350k; discovered 64k + 350k cap → 64k; known +Jawcode metadata → exact (optionally cap-lowered); native entries unchanged; +null-template fallback → 128k. + +## Risks + +- 128k can still overstate an undocumented smaller model — users set + explicit `contextWindow`. +- Clearing only `context_window` leaves max/compaction inconsistent — all + three must go together. + +## Accept criteria + +- Activation + matrix green; gates as 030. diff --git a/devlog/_plan/260805_bug_stack_campaign/110_issue993_kiro_profile_arn.md b/devlog/_plan/260805_bug_stack_campaign/110_issue993_kiro_profile_arn.md new file mode 100644 index 000000000..35b490661 --- /dev/null +++ b/devlog/_plan/260805_bug_stack_campaign/110_issue993_kiro_profile_arn.md @@ -0,0 +1,69 @@ +# 110 — Issue #993: Kiro profileArn 400 for Builder ID accounts + +Independent lane. Research: explorer batch G (incl. external Kiro/AWS +evidence). + +## Verified current state + +- `createKiroAdapter()` resolves an optional ARN, omits it when absent, + selects CLI wire: `src/adapters/kiro.ts:1695,1723-1754`. +- `resolveKiroProfileArn()` reads account metadata / accountless env / + imported CLI state only: `src/oauth/kiro.ts:425`. CLI SQLite import checks + `api.codewhisperer.profile`: `src/oauth/kiro-credentials.ts:282-303,347-356`. +- `kiro-cli whoami --format json` is the documented introspection surface + (schema undocumented). No verified safe formula or public Builder-ID + lookup API exists. Never hardcode an ARN, derive from undocumented token + claims, borrow another account's metadata, or auto-pick from an + administrative profile list. +- Generic Kiro 400s → `invalid_request_error` with raw detail but no stable + actionable code: `src/adapters/kiro-errors.ts:95-151`. + +## Diff-level plan + +MODIFY `src/oauth/kiro.ts`: +- Extend `readKiroCliIdentity()` (`:175-185`) to return + `{email?, profileArn?}`; parse only documented `whoami --format json` + shapes (`profileArn`, `profile_arn`, `profile.arn`); validate length + + `arn::codewhisperer:::profile/` structure. +- `oauthCredentialFromImported()` (`:249-264`): + `resolvedProfileArn = imported.profileArn ?? identity.profileArn` → + `credential.kiro.profileArn` + `accountId`. Account-safe: `whoami` runs + against the same active CLI session just imported. Fail closed when + absent. + +MODIFY `src/adapters/kiro-errors.ts` — before the generic validation branch, +recognize evidence containing both `profilearn` and `required` → status 400, +`invalid_request_error`, stable code `kiro_profile_required`, non-retryable, +actionable redacted message (re-login/re-import the matching account). + +No `src/adapters/kiro.ts` change (it already sends a resolved ARN in header ++ payload). + +MODIFY `tests/kiro-oauth.test.ts` (whoami parsing + persistence), +`tests/kiro-retry.test.ts` (400 classification), `tests/kiro-stream.test.ts` +(event-stream ValidationException classification). + +## Tests / activation + +Activation: import active CLI credential → SQLite lacks ARN → `whoami` +exposes a valid ARN for that session → persisted account-scoped → adapter +sends `x-amzn-kiro-profile-arn` + payload `profileArn`. + +Matrix (fake CLI runner, no AWS): valid top-level/nested shapes; +malformed/wrong-service/oversized ARN ignored; imported SQLite ARN wins; +whoami failure/no ARN → login still usable for ungated models; no +cross-account borrowing; `ksk_...` API-key path still omits; exact upstream +"profileArn is required" → `kiro_profile_required`, no retry, no leakage; +unrelated ValidationException → existing behavior; enterprise ARN paths +unchanged. + +## Risks / limitations + +- whoami JSON schema undocumented — discovery optional, fail closed. +- Builder ID may expose only a username — then gated models still fail and + the improved error is the safe terminal behavior. +- No live Kiro/AWS verification — PR body states this. + +## Accept criteria + +- Structural matrix green; gates as 030. diff --git a/devlog/_plan/260805_bug_stack_campaign/120_issue959_provider_headers.md b/devlog/_plan/260805_bug_stack_campaign/120_issue959_provider_headers.md new file mode 100644 index 000000000..1083ce899 --- /dev/null +++ b/devlog/_plan/260805_bug_stack_campaign/120_issue959_provider_headers.md @@ -0,0 +1,68 @@ +# 120 — Issue #959: management-plane provider headers (adopt PR #961) + +Independent lane. Research: explorer batch G (read PR #961 head +`2a295b46f803b1b8a5ad365956f4cb6b5fc08389`). +Decision: **adopt PR #961; do not compete.** Add one hardening slice. + +## Verified current state + +- PATCH `/api/providers` recognizes fields at + `src/server/management/provider-routes.ts:219-287`; unknown-only → 400 + (`:289`). CLI has no headers option + (`src/cli/provider-runtime.ts:30-55`). +- `OcxProviderConfig.headers` exists (`src/types.ts:1058`); + `providerHeadersConfigError()` rejects malformed names, non-string/CRLF + values, and credential headers (`src/config.ts:605-614,651`). Adapters + apply configured headers AFTER generated auth + (`src/adapters/openai-chat.ts:840-847`, `src/adapters/anthropic.ts:875-894`). +- Secret-DTO precedent: GET exposes `hasApiKey` never the key + (`provider-routes.ts:81-93`); `safeConfigDTO()` exposes `hasHeaders`, + never names/values (`src/server/auth-cors.ts:506-514`); regression proof + `tests/server-auth.test.ts:389-428`. +- #961 already carries the right architecture: reusable PATCH helper, + shallow case-insensitive merge, `null`/`{}` clear, registry static-header + preservation, validation reuse, mutation-lock replay, CLI parsing, + argument redaction, tests + locale docs. Still draft; no full CI. + +## Diff-level plan (on top of #961's adoption) + +1. Review #961 forward (or rebase-carry it with authorship preserved): + require ready-for-review, full CI green, privacy scan. +2. Hardening addition: GET `/api/providers` + (`provider-routes.ts:81-93`) gains only + `hasHeaders: !!p.headers && Object.keys(p.headers).length > 0`. Never + return header names/values from GET or PATCH responses; add GET/PATCH + serialization assertions proving a sentinel name/value is absent. +3. Semantics: `Authorization` and other credential headers stay rejected + (apiKey/authMode/apiKeyTransport own those); other values treated as + secret-ish in logs/CLI diagnostics; keep #961's `runtime-api.ts` + redaction; docs state `--headers` is non-authentication metadata. + +Files (retained from #961): `src/server/management/provider-routes.ts`, +`src/cli/provider-runtime.ts`, `src/cli/runtime-api.ts`, +`tests/management-provider-validation.test.ts`, +`tests/cli-headless-parity.test.ts`, locale CLI/management docs. + +## Tests / activation + +`ocx provider edit AGR-OAI --headers '{"x-app":"cli"}'` merges + persists; +next adapter request sends the header; management GET shows `hasHeaders` +only. Matrix: valid PATCH persists; second PATCH shallow case-insensitive +merge; `null`/`{}` clears (registry provider keeps registry static headers); +invalid body/array/non-string/CRLF → 400 no mutation; credential headers → +400; `X-Foo` then `x-foo` → one final value; concurrent independent PATCHes +both survive; unknown-only → 400; GET/PATCH/logs never expose names/values; +CLI object/`-`/`{}`/malformed/repeated flag/inline redaction; adapter +structural test (headers reach the wire, management can't override generated +auth). + +## Risks + +- CLI JSON lives in shell history — documented as non-secret metadata only. +- Merge-not-replace semantics documented. +- POST `/api/providers` overwrite removing omitted headers is a different + (non-)defect. + +## Accept criteria + +- #961 adopted with CI + the GET non-disclosure hardening; gates as 030. diff --git a/devlog/_plan/260805_bug_stack_campaign/130_dispositions.md b/devlog/_plan/260805_bug_stack_campaign/130_dispositions.md new file mode 100644 index 000000000..38555566d --- /dev/null +++ b/devlog/_plan/260805_bug_stack_campaign/130_dispositions.md @@ -0,0 +1,45 @@ +# 130 — Dispositions (closes and verdicts with evidence) + +User authorization on record: close issues/PRs that are already resolved on +`dev`. Anything beyond that (closing contributor PRs as superseded, asking +reporters for info) is executed only when this document's table names it, and +ambiguous cases go back to the user first. + +## Close as already-fixed (issue) + +| Item | Action | Evidence to cite in the close comment | +|------|--------|----------------------------------------| +| #806 | close | `d52b387db` is an ancestor of `origin/dev` (verified `git merge-base --is-ancestor`); GUI/CLI/docs wording split shipped (`gui/src/i18n/en.ts:1296-1315`) | + +## Verification-needed issues — campaign verdicts + +| Item | Disposition | Basis | +|------|-------------|-------| +| #994 | leave open; allowlist location identified, needs reporter's provider/model + wire capture | `src/providers/registry.ts:918-958,1637-1655` | +| #904 | leave open; `eeef7a32a` fixed surrogate boundaries but the original capture is still needed | 001 triage | +| #796 | leave open pending live Ark credential verification; structural fix `d3abf4345` + regression test already on dev | `tests/volcengine-ark-assistant-content.test.ts:90-125` | +| #418 | leave open; latest same-run trace does not reproduce; needs reporter's current trace | `src/server/responses/collaboration.ts:243-304` | + +## Contributor PRs superseded by stack PRs + +Disposition happens only after the corresponding stack PR lands on `dev`, +and only after a semantic-equivalence comparison (audit round 1, blocker 5): +the landed behavior and tests are compared against the contributor PR's full +scope, useful authorship is preserved where the contributor's approach was +adopted, and any contributor behavior intentionally not matched is recorded +with the reason. "The linked issue is fixed" alone is never sufficient. + +| PR | Successor | Equivalence basis (from the decade docs) | Disposition when landed | +|---:|-----------|------------------------------------------|-------------------------| +| #966, #922 | 030 (#914) | #966 closest semantic source (classifier/retry extracted, sidecar blast radius excluded); #922 not equivalent (policy expansions rejected) | close with pointer, user confirm | +| #928 | 040 (#893) | field-only normalization ≠ commitment; event-synthesis design recorded | close with pointer, user confirm | +| #940 | 060 (#938) | ID-prefix idea equivalent; 750-line bundle's extra mechanisms deliberately excluded, named in the close comment | close with pointer, user confirm | +| #1006 | 050 (#875) | core policy adopted (bounded upstream JSON), diff superseded with attribution | adopt-or-close at that cycle's P | +| #961 | 120 (#959) | ADOPTED, not superseded — authorship preserved, hardening slice added | review forward, not close | + +## Stale/broken PRs outside the stack + +#933 (CI fail), #557 (CI fail), #936 (conflicting, needs security review per +MAINTAINERS), #569 (conflicting), #997/#947/#1000/#978/#983/#985 (unverified, +policy-only CI). These stay open; the campaign does not close contributor work +that merely needs the author. Recorded here so the surface state is complete. diff --git a/src/adapters/anthropic.ts b/src/adapters/anthropic.ts index 4fa629e4c..60b27d43a 100644 --- a/src/adapters/anthropic.ts +++ b/src/adapters/anthropic.ts @@ -19,6 +19,7 @@ import { parseDataUrl } from "./image"; import { enforceAnthropicImageLimits } from "./anthropic-image-guard"; import { normalizeAnthropicImages } from "./anthropic-image-normalize"; import { identifyRoutedModel } from "./identity"; +import { redactSecretString } from "../lib/redact"; import { CLAUDE_CODE_HEADERS, claudeCodeSessionId } from "./client-fingerprint"; import { buildNonOpenAIToolCatalogNudgeForTools } from "./tool-catalog-nudge"; import { decodeServerSentEvents } from "../lib/sse-decoder"; @@ -243,6 +244,43 @@ function isLikelyRealAnthropicThinkingSignature(signature: string | undefined): return /^[A-Za-z0-9+/_=-]+$/.test(signature); } +/** + * Bridge error fidelity (web-search/images loops): extract a display-safe summary from an + * Anthropic JSON error envelope so `Provider error ` carries the upstream reason. + * JSON-only extraction — HTML/non-JSON bodies yield "" so raw markup is never echoed. + */ +export function formatAnthropicErrorBody(status: number, _headers: Headers, payloadText: string): string { + let parsed: unknown; + try { + parsed = JSON.parse(payloadText); + } catch { + return ""; + } + const detail = extractAnthropicErrorDetail(parsed); + if (!detail) return ""; + return redactSecretString(detail).slice(0, 400); +} + +function extractAnthropicErrorDetail(parsed: unknown): string | undefined { + if (typeof parsed === "string") return parsed.trim() || undefined; + if (parsed === null || typeof parsed !== "object" || Array.isArray(parsed)) return undefined; + const obj = parsed as Record; + // Anthropic envelope: { type: "error", error: { type, message } }; tolerate a bare + // { error: { message } } and a string error field. + const err = obj.error; + if (typeof err === "string" && err.trim()) return err.trim(); + if (err !== null && typeof err === "object" && !Array.isArray(err)) { + const e = err as Record; + const msg = e.message; + if (typeof msg !== "string" || !msg.trim()) return undefined; + const type = e.type; + return typeof type === "string" && type.trim() + ? `${type.trim()}: ${msg.trim()}` + : msg.trim(); + } + return undefined; +} + function usesNativeAnthropicEndpoint(provider: OcxProviderConfig): boolean { try { return new URL(provider.baseUrl).hostname === "api.anthropic.com"; @@ -777,6 +815,8 @@ export function createAnthropicAdapter(provider: OcxProviderConfig, cacheRetenti return { name: "anthropic", + formatErrorBody: formatAnthropicErrorBody, + async buildRequest(parsed: OcxParsedRequest, incoming?: IncomingMeta) { if (typeof provider.apiKey !== "string" || provider.apiKey.trim() === "") { if (isOAuth) { diff --git a/src/lib/redact.ts b/src/lib/redact.ts index 72dd29bd4..49ad81b4a 100644 --- a/src/lib/redact.ts +++ b/src/lib/redact.ts @@ -12,6 +12,13 @@ const SECRET_VALUE_PATTERNS: Array<[RegExp, string]> = [ // a Bearer-prefix rule alone leaves the suffix intact. [/\btid=[A-Za-z0-9-]+(?:;[A-Za-z0-9_.-]+=[^;\s"']*)+(?::[A-Za-z0-9+/=_-]+)?/g, REDACTED_SECRET], [/\b((?:api[_-]?key|access[_-]?token|refresh[_-]?token|id[_-]?token|client[_-]?secret|refreshToken|accessToken|clientSecret|apiKey)=)([^&\s"',;]+)/gi, `$1${REDACTED_SECRET}`], + // Colon-labelled credentials. Upstream error bodies quote the offending header + // or field back at us ("x-api-key: abc…"), and the `=` rule above never fires + // for that shape, so the credential survived into client-visible error text. + // Header-style names are included because that is exactly what a provider + // echoes when it rejects a request. A `Bearer ` value is left to the + // dedicated rule above so its scheme prefix stays readable in diagnostics. + [/\b((?:x-api-key|x-goog-api-key|x-amz-security-token|api[_-]?key|apiKey|access[_-]?token|accessToken|refresh[_-]?token|refreshToken|id[_-]?token|client[_-]?secret|clientSecret|authorization|proxy-authorization|cookie|password|secret|token)\s*:\s*)(?!\s)(?!Bearer\b)([^\s"',;]+)/gi, `$1${REDACTED_SECRET}`], [/((?:"(?:api[_-]?key|access[_-]?token|refresh[_-]?token|id[_-]?token|client[_-]?secret|refreshToken|accessToken|clientSecret|apiKey)"\s*:\s*"))([^"]+)(")/gi, `$1${REDACTED_SECRET}$3`], // Raw JSON "token" field values (Copilot token exchange bodies echo the credential here). [/(("token"\s*:\s*"))([^"]+)(")/gi, `$1${REDACTED_SECRET}$4`], diff --git a/tests/anthropic-error-body.test.ts b/tests/anthropic-error-body.test.ts new file mode 100644 index 000000000..e880fa49d --- /dev/null +++ b/tests/anthropic-error-body.test.ts @@ -0,0 +1,162 @@ +import { afterEach, describe, expect, test } from "bun:test"; +import type { ProviderAdapter } from "../src/adapters/base"; +import { createAnthropicAdapter, formatAnthropicErrorBody } from "../src/adapters/anthropic"; +import { parseRequest } from "../src/responses/parser"; +import { responseWithDeferredRequestLog, type RequestLogEntry } from "../src/server"; +import { runWithWebSearch as runWithWebSearchProduction, type WebSearchLoopDeps } from "../src/web-search/loop"; +import type { OcxProviderConfig } from "../src/types"; +import { createTestTranslatorBudget } from "./helpers/translator-budget"; + +const originalFetch = globalThis.fetch; + +afterEach(() => { + globalThis.fetch = originalFetch; +}); + +describe("formatAnthropicErrorBody", () => { + const headers = new Headers(); + + test("renders the Anthropic envelope as type: message", () => { + const body = JSON.stringify({ + type: "error", + error: { type: "invalid_request_error", message: "messages.0: broken" }, + }); + expect(formatAnthropicErrorBody(400, headers, body)) + .toBe("invalid_request_error: messages.0: broken"); + }); + + test("tolerates a bare error.message without a type", () => { + const body = JSON.stringify({ error: { message: "plain failure" } }); + expect(formatAnthropicErrorBody(400, headers, body)).toBe("plain failure"); + }); + + test("tolerates a string error field and a JSON string body", () => { + expect(formatAnthropicErrorBody(500, headers, JSON.stringify({ error: "boom" }))).toBe("boom"); + expect(formatAnthropicErrorBody(500, headers, JSON.stringify("raw string"))).toBe("raw string"); + }); + + test("HTML and non-JSON bodies yield empty so markup is never echoed", () => { + expect(formatAnthropicErrorBody(400, headers, "Bad")).toBe(""); + expect(formatAnthropicErrorBody(400, headers, "not json at all")).toBe(""); + expect(formatAnthropicErrorBody(400, headers, JSON.stringify({ unrelated: true }))).toBe(""); + }); + + test("secret-shaped content inside the message is redacted", () => { + const body = JSON.stringify({ + type: "error", + error: { type: "invalid_request_error", message: "bad key sk-redact-me-please" }, + }); + const out = formatAnthropicErrorBody(400, headers, body); + expect(out).not.toContain("sk-redact-me-please"); + expect(out).toContain("[REDACTED]"); + }); +}); + +const anthropicProvider: OcxProviderConfig = { + adapter: "anthropic", + baseUrl: "https://api.anthropic.com", + authMode: "key", + apiKey: "test-key", +} as OcxProviderConfig; + +const forwardProvider: OcxProviderConfig = { + adapter: "openai-responses", + baseUrl: "https://chatgpt.test/v1", + authMode: "forward", +} as OcxProviderConfig; + +function parsed() { + return parseRequest({ + model: "routed/claude-opus-5", + input: "Search current docs", + stream: true, + tools: [{ type: "web_search" }], + }); +} + +function deps(adapter: ProviderAdapter, overrides: Record = {}) { + return { + parsed: parsed(), + adapter, + forwardProvider, + hostedTool: { type: "web_search" }, + selectedForwardHeaders: new Headers({ authorization: "Bearer forwarded" }), + settings: { model: "gpt-5.6-luna", reasoning: "low" as const, timeoutMs: 1_000 }, + maxSearches: 1, + ...overrides, + }; +} + +function runWithWebSearch( + d: Omit & { incomingMeta?: WebSearchLoopDeps["incomingMeta"] }, +): Promise { + return runWithWebSearchProduction({ + ...d, + incomingMeta: d.incomingMeta ?? { + headers: new Headers(), + translatorBudget: createTestTranslatorBudget(), + }, + }); +} + +describe("anthropic adapter through the web-search bridge", () => { + test("a JSON 400 envelope reaches the client message instead of the bare status", async () => { + const envelope = JSON.stringify({ + type: "error", + error: { type: "invalid_request_error", message: "tools.0.custom.name: pattern mismatch" }, + }); + globalThis.fetch = (async () => new Response(envelope, { + status: 400, + headers: { "content-type": "application/json" }, + })) as typeof fetch; + + const adapter = createAnthropicAdapter(anthropicProvider); + const response = await runWithWebSearch(deps(adapter)); + expect(response.status).toBe(400); + const body = await response.json() as { error: { message: string } }; + expect(body.error.message).toBe( + "Provider error 400: invalid_request_error: tools.0.custom.name: pattern mismatch", + ); + }); + + test("the formatted message flows into the persisted request log upstreamError", async () => { + const envelope = JSON.stringify({ + type: "error", + error: { type: "invalid_request_error", message: "prompt shape rejected" }, + }); + globalThis.fetch = (async () => new Response(envelope, { + status: 400, + headers: { "content-type": "application/json" }, + })) as typeof fetch; + + const adapter = createAnthropicAdapter(anthropicProvider); + const response = await runWithWebSearch(deps(adapter)); + const entries: RequestLogEntry[] = []; + const logged = responseWithDeferredRequestLog( + response, + "ocx-test-anthropic-400-fidelity", + Date.now(), + { model: "claude-opus-5", provider: "anthropic" }, + entry => entries.push(entry), + ); + await logged.text(); + expect(entries).toHaveLength(1); + expect(entries[0].upstreamError).toBe("Provider error 400: invalid_request_error: prompt shape rejected"); + expect(entries[0].status).toBe(400); + }); + + test("a body-read failure still falls back to the bare status (no formatter, no leak)", async () => { + const errorBody = new ReadableStream(); + Object.defineProperty(errorBody, "getReader", { + value: () => { throw new Error("reader-secret"); }, + }); + const adapter = createAnthropicAdapter(anthropicProvider); + adapter.fetchResponse = async () => new Response(errorBody, { status: 400 }); + + const response = await runWithWebSearch(deps(adapter)); + expect(response.status).toBe(400); + const body = await response.json() as { error: { message: string } }; + expect(body.error.message).toBe("Provider error 400"); + expect(JSON.stringify(body)).not.toContain("reader-secret"); + }); +}); diff --git a/tests/redact.test.ts b/tests/redact.test.ts index e550ceb07..a7556cc0b 100644 --- a/tests/redact.test.ts +++ b/tests/redact.test.ts @@ -34,6 +34,32 @@ describe("redactSecretString", () => { test("preserves non-secret diagnostic text", () => { expect(redactSecretString("status=429 model=gpt-5.5")).toBe("status=429 model=gpt-5.5"); }); + + test("masks colon-labelled credentials echoed back by an upstream error", () => { + // #1020 review: upstream 4xx bodies quote the offending header at us. The + // `=` rules never fire for `header: value`, so a custom credential used to + // survive into the client-visible error text. + const input = [ + "x-api-key: customcredential123456", + "X-Goog-Api-Key: another-live-credential", + "client_secret: not-a-sk-shaped-value", + "token: opaque-session-value", + ].join("\n"); + + const redacted = redactSecretString(input); + expect(redacted).toContain(`x-api-key: ${REDACTED_SECRET}`); + expect(redacted).toContain(`X-Goog-Api-Key: ${REDACTED_SECRET}`); + expect(redacted).not.toContain("customcredential123456"); + expect(redacted).not.toContain("another-live-credential"); + expect(redacted).not.toContain("not-a-sk-shaped-value"); + expect(redacted).not.toContain("opaque-session-value"); + }); + + test("leaves non-credential colon labels readable", () => { + // The colon rule must not swallow ordinary diagnostics. + expect(redactSecretString("model: gpt-5.5\nstatus: 429\nrequest: ocx-abc123")) + .toBe("model: gpt-5.5\nstatus: 429\nrequest: ocx-abc123"); + }); }); describe("redactSecrets", () => {