diff --git a/devlog/_plan/260904_bug_backlog_closeout/000_research.md b/devlog/_plan/260904_bug_backlog_closeout/000_research.md new file mode 100644 index 00000000000..33a9e9b3b2e --- /dev/null +++ b/devlog/_plan/260904_bug_backlog_closeout/000_research.md @@ -0,0 +1,81 @@ +# 260904 bug backlog closeout — research + +Goal: drive every bug-labeled OPEN issue and bug-labeled OPEN PR in lidge-jun/opencodex +to a terminal state (merged, superseded with attribution, or closed with rationale). + +Session FSM: PABCD under an ACTIVE host goal (HOTL). Goalplan slug +`close-out-the-bug-backlog-of-lidge-jun-opencodex`. + +## Board snapshot (captured at goal start, dev = 072df52eb) + +### Bug-labeled open PRs (12) + +| PR | Author | State | Base | Note | +|----|--------|-------|------|------| +| 3430 | ChickenBreast-ky | READY, all checks pass | dev | Closes #3428 | +| 3420 | ildunari | READY, all checks pass | dev | no Closes tag | +| 3405 | adtumk | READY, all checks pass | dev | Closes #3378 | +| 3403 | ianlyoo | READY, all checks pass | dev | Closes #3402 | +| 3401 | agentHits | READY, all checks pass | dev | Closes #3400 | +| 3432 | luvs01 | DRAFT | dev | lab file-URI privacy | +| 3407 | turin-dev | DRAFT, 33 behind | dev | integrations toggle | +| 3394 | kremnyi | DRAFT, 33 behind | dev | grok 4.6 responses | +| 3388 | zleo-ai | DRAFT, 44 behind | dev | grok sparse output | +| 3348 | RHODIZSECURITY | DRAFT, 33 behind | dev | 2338-line failover overhaul | +| 3332 | full999 | DRAFT, 66 behind | dev | claude combo capabilities | +| 3325 | luvs01 | DRAFT, checks FAIL | dev | workflow surface, unsponsored | + +### Bug-labeled open issues (13) + +Claimed by a PR: #3428 (3430), #3402 (3403), #3400 (3401), #3406 (3407). +Unclaimed: #3433, #3425, #3424, #3352. +needs-info: #3320, #3279, #3255, #3245, #1527. + +## Verification constraint (user-stated, binding) + +The local full suite is FORBIDDEN for this unit: no `bun run test`, no bare `bun test`. +Live GitHub CI (`gh pr checks`) is the authoritative verifier; CI already runs +Linux/Windows/macOS. At most one named focused test file may be run when a change +needs a local signal. This overrides the AGENTS.md PR-ready full-suite gate for +this session because the maintainer explicitly directed it. + +## Attribution constraint + +AGENTS.md `missing_coauthor_credit` and CREDITS.md: reimplementing, superseding, +carrying, or rebasing another author's PR REQUIRES a `Co-authored-by:` trailer +naming that author in a branch commit so it survives the squash. Prose credit is +not equivalent — GitHub reads the trailer, not the sentence. + +## Repository permission + +`gh api repos/lidge-jun/opencodex --jq .permissions` returns +`{"admin":true,"maintain":true,"pull":true,"push":true,"triage":true}`. +Squash-merge into dev is therefore available to this session. Branch rulesets +still require a reviewed PR; force-push and direct dev push remain refused. + +## What "terminal" means for this unit (settled after plan audit round 2) + +The plan auditor argued that only MERGED or CLOSED counts, and that a live PR or a +posted NEEDS_HUMAN is "deferred closure, not a terminal repository state." That is +rejected as the completion bar, deliberately, and the reason is recorded here so the +D-phase claim can be checked against a stated rule rather than a mood. + +The goal contract this session was given names BLOCKED, NEEDS_HUMAN, UNSAFE, and NOOP +as terminal outcomes alongside DONE. Some items genuinely cannot reach CLOSED from +inside this session without lying or destroying information: + +- #3255 asks for a product decision about matching official ChatGPT behavior. Closing it + to satisfy a counter would discard a legitimate request; inventing the product intent + would be worse. +- #3245, #3279, #1527 need evidence only the reporter has. Closing them before the + reporter answers converts a real bug into a silent one. `stale-needs-info.yml` exists + precisely because this project already decided how that timeout is owned. +- A workflow-surface change (#3325) requires maintainer sponsorship that admin rights do + not substitute for. + +So the bar for this unit is: every item reaches a RECORDED terminal outcome, where +DONE means merged/closed and the non-DONE outcomes require (a) a named reason from the +goal contract, (b) evidence with file:line or a posted URL, and (c) a visible artifact on +the issue or PR itself. What is forbidden is the thing the auditor was right to attack: +an item left open with no posted artifact and no named outcome. Silence is not a +disposition. That distinction is the operative rule for wp3, wp5, and wp6. diff --git a/devlog/_plan/260904_bug_backlog_closeout/010_wp2_green_merge_train.md b/devlog/_plan/260904_bug_backlog_closeout/010_wp2_green_merge_train.md new file mode 100644 index 00000000000..5f80ff294e0 --- /dev/null +++ b/devlog/_plan/260904_bug_backlog_closeout/010_wp2_green_merge_train.md @@ -0,0 +1,99 @@ +# wp2 — green merge train + +Five bug PRs are review-ready with every check green. An independent Sol reviewer +read each diff against current dev. Results below; one is NOT safe to land. + +## Merge set + +### #3430 fix(server): allow image routes on loopback listener — @ChickenBreast-ky +SAFE-TO-MERGE. Adds exactly the two image POST paths to the loopback allowlist at +`src/server/index.ts:815`. The handler still applies API admission and origin checks +(`src/server/index.ts:1688-1692`), so the public listener stays credential-gated; +GET and sub-paths remain denied. Regression: `tests/loopback-listener-integration.test.ts:349`. +Closes #3428. + +### #3420 fix(responses): preserve outputs missing call ids — @ildunari +SAFE-TO-MERGE. Repair is scoped to tool-output items with no nonempty `call_id` and a +representable output (`src/adapters/openai-responses.ts:995`); valid stateful outputs +pass unchanged and malformed ones fail closed. Regression: +`tests/openai-responses-passthrough.test.ts:2248`. No `Closes` tag — no issue to close. + +### #3405 fix(opencode-go): satisfy provider wire contract — @adtumk +MERGE-WITH-NOTE. Destination matching is exact; session values are opaque hashes; +explicit headers win; config is not mutated. The PR body reports four full-suite +failures it attributes to the dev baseline, not to itself. Since this unit does not +run the local suite, the note is recorded rather than re-litigated: hosted CI on the +PR is green, which is this unit's authoritative verifier. Closes #3378. + +### #3401 fix(cli): heal deleted cwd at launch — @agentHits +MERGE-WITH-NOTE. `isatty(0/1)` avoids Bun lazy stream construction +(`src/cli/star-prompt.ts:168`, `src/update/notify.ts:125`); both launchers recover to +`homedir()`. Test coverage is partial: `tests/update-notify.test.ts:139` proves the TTY +guard under an unlinked cwd but does not spawn a launcher subprocess. Accepted as a +follow-up, not a blocker. Closes #3400. + +### #3403 fix(proxy): accept dotted ns.name tool echo — @ianlyoo +HOLD — do not merge in wp2. The reviewer found a dispatch-collision risk: dotted +aliases are inserted into `toolNsMap` at `src/server/responses/collaboration.ts:136-143` +with no collision detection. Tool names allow any non-control character +(`src/responses/namespace-tool-compat.ts` `isRepresentableName`), so +`{namespace:"a", name:"b.c"}` and `{namespace:"a.b", name:"c"}` both flatten to `a.b.c`; +the second silently overwrites the first, so a dotted provider echo can invoke the +wrong client tool. The undeclared-tool guard collapses both identities into one set +entry at `src/server/responses-undeclared-tool-guard.ts:98`. This sits on the +client-tool authorization boundary, so it is treated as a real blocker. + +Disposition: keep #3403 open in wp2 and hand it to wp3 as a NAMED work item +(wp3 item "#3403 collision repair"). `maintainerCanModify` is true on +`ianlyoo:fix-dotted-tool-alias`, so wp3 pushes the collision fix onto the author's +branch, preserving @ianlyoo as PR author; if that push is refused, wp3 opens a +successor branch whose commit carries `Co-authored-by: Youngin (Ian) Lyoo`. +wp3 owns driving it to MERGED or CLOSED — a posted review alone does not discharge it. + +## Merge order + +`src/adapters/openai-responses.ts` is touched by both #3420 and #3405, in distant +hunks (~906-1141 vs ~1966-2008). Merge #3430 first (smallest, isolated), then #3420, +then #3405, refreshing between each so the second lands on the first's result. +Order: 3430 -> 3401 -> 3420 -> 3405. + +## Accept criteria + +- each merged PR reports `state=MERGED` with a `mergedAt` and a dev merge sha +- linked issues #3428, #3400, #3378 are CLOSED after their merge lands +- no local full-suite run; `gh pr checks` is the recorded evidence +- #3403 carries a posted review naming the collision with file:line +- #3403 is explicitly handed to wp3 as a named item, not left unowned + +## Manual issue closing (audit residual) + +`gh pr view --json closingIssuesReferences` returns EMPTY for all five PRs even though +the bodies contain `Closes #N`: GitHub only auto-closes when the PR merges into the +default branch (`main`), and these target `dev`. Every linked issue must therefore be +closed manually after its merge lands, quoting the dev merge sha. + +## Merge mechanics (wp2 P-phase stale check, re-verified against the live repo) + +Re-verified before executing: all four of #3430, #3401, #3420, #3405 report +`mergeable=MERGEABLE` with zero non-success checks. `mergeStateStatus=BLOCKED` is not a +CI failure — the `Protect dev` ruleset requires one approving review, and every PR sits +at `REVIEW_REQUIRED`. + +Ruleset (`gh api repos/lidge-jun/opencodex/rules/branches/dev`): +`required_approving_review_count: 1`, `require_code_owner_review: true`, +`require_extra_approval_for_unattributed_changes: true`, +`allowed_merge_methods: ["merge", "squash"]` — rebase merges are off, so squash it is. + +How the review requirement is satisfied: these are contributor PRs, so the maintainer +reviews and approves them normally. MAINTAINERS.md line 172 notes that the admin role +also holds a `pull_request` bypass, but a bypass is not the right instrument here — +"Authors do not approve their own pull requests" still governs, and the file requires +that any bypass use be RECORDED on the PR rather than inferred from a merge timestamp. +Since the maintainer is not the author of any of these four, an ordinary approving review +is both available and more honest, and it leaves the reasoning visible on the PR. +CODEOWNERS puts `@lidge-jun` on `/src/adapters/`, `/src/providers/`, `/src/codex/`, +`/src/server/`, and `/.github/`, so the same review satisfies code-owner sign-off. + +Each approval carries the substantive finding from the independent review lane, so the +merge record shows what was checked — including the two MERGE-WITH-NOTE items (#3405's +claimed-baseline suite failures, #3401's partial launcher coverage). diff --git a/devlog/_plan/260904_bug_backlog_closeout/020_wp3_draft_pr_triage.md b/devlog/_plan/260904_bug_backlog_closeout/020_wp3_draft_pr_triage.md new file mode 100644 index 00000000000..bba3a92d835 --- /dev/null +++ b/devlog/_plan/260904_bug_backlog_closeout/020_wp3_draft_pr_triage.md @@ -0,0 +1,107 @@ +# wp3 — draft bug PR triage + +Seven draft bug PRs, reviewed by an independent Sol lane. Verdicts and the exact +blocking defect for each. + +## #3432 @luvs01 — lab file URI privacy bypass — DRIVE-TO-GREEN +`src/lab/events/limits.ts:36` rejects standalone `file:` schemes, but ASCII tab/newline +inside the scheme normalizes to a valid file URL and evades `FILE_URI_RE` +(`"fi\nle:///..."` -> `file:///...`). Fix: strip/normalize URL whitespace before the +scheme test, add those regressions. Privacy-sensitive admission logic; not an auth path. + +## #3407 @turin-dev — integrations toggle truthfulness — DRIVE-TO-GREEN +PUT persists via `setCodexIntegrationEnabled` (`src/server/management/native-integration-routes.ts:309`) +but GET still feeds the stale startup `config` into `codexStatus`, and the UI trusts it +(`gui/src/pages/integrations/overview-clients.ts:239`), so the switch snaps back after a +live toggle. Also `gui/src/i18n/tr.ts:1523` mistranslates "resumable". Needs a +PUT-then-GET regression plus a rebase (33 behind). + +## #3394 @kremnyi — Grok 4.6 Responses — DRIVE-TO-GREEN +Correct after three addressed review fixes. The `enforce-target` "failure" is a +cancelled run superseded by a higher-priority gate request, not a real failure. +Needs rebase + a fresh gate run + readiness boxes. + +## #3388 @zleo-ai — Grok sparse terminal output — DRIVE-TO-GREEN +Opt-in, Grok-client-only snapshot reconstruction, fail-closed, well tested. 847 lines +but the production change is one coherent compatibility boundary. Needs rebase +(44 behind) and hosted CI evidence for its claimed-baseline failures. + +## #3348 @RHODIZSECURITY — failover hardening — SUPERSEDE +2338 lines / 33 files / 8 commits spanning cooldown persistence, provider quota state, +API-key 401/429 rotation, lifecycle, stream preflight, policy fallback, and public +error contracts. Individual fixes are sound (hashed key identity at +`src/providers/key-failover.ts:88`, exhaustion normalization at +`src/server/responses/policy-fallback.ts:166`), but it changes multiple independent +invariants in one diff and still has a blocker: the duplicated target-incompatibility +matcher at `src/server/responses/core.ts:3936` omits the shared generic `tool_choice` +case, so some combo children abort instead of hopping. Security-review class (credentials, +401 handling, rotation, persistence). Split into a reviewable stack, every branch commit +carrying `Co-authored-by: RHODIZSECURITY`. + +## #3332 @full999 — Claude combo capabilities + output budget — DRIVE-TO-GREEN +Output-budget handling at `src/adapters/anthropic.ts:904` is correct. Blocker: the +catalog fallback maps vendor `maxTokens` onto `maxInputTokens` at +`src/codex/catalog/provider-fetch.ts:907`, shrinking a 1M Claude input window to its +128k output ceiling. Fix the mapping to `maxOutputTokens`, assert the 1M window +survives, rebase (66 behind). + +## #3325 @luvs01 — ignore fork PRs in dev bump guard — DRIVE-TO-GREEN (sponsorship) +The change is correct: an owner-qualified server-side `head` filter at +`.github/workflows/dev-version-bump.yml:129` stops a same-named fork branch from +satisfying the repository-owned idempotency guard. Both `hygiene` and `enforce-target` +fail for exactly one reason: `unsponsored_surface` — a workflow file needs maintainer +security review and the `maintainer-sponsored` label. This is a maintainer decision, +not a code defect. + +## #3403 @ianlyoo — dotted ns.name tool echo — COLLISION REPAIR (carried from wp2) + +Handed over by wp2. The PR is correct in intent and green on CI, but it inserts dotted +aliases into `toolNsMap` (`src/server/responses/collaboration.ts:136-143`) with no +collision detection. Independently verified: namespaces come straight from the inbound +Responses `tools` array — the schema accepts an arbitrary namespace object +(`src/responses/schema.ts:117`) and `parseRequest` copies any string namespace +(`src/responses/parser.ts:221`), while `isRepresentableName` rejects only control +characters (`src/responses/namespace-tool-compat.ts:34`). Dots are legal in both halves, +and the existing `NamespaceToolCollisionError` guard covers only the `ns__name` form. +So `{a, b.c}` and `{a.b, c}` both claim `a.b.c` and the later insertion wins, which can +dispatch a provider echo to the wrong client tool. + +Repair: before registering a dotted alias, check whether it is already owned by a +different `{namespace, name}` identity; on conflict register neither dotted alias (fail +closed to the unambiguous `ns__name` form) rather than picking a winner. Same treatment +in `src/server/responses-undeclared-tool-guard.ts:98` so the guard never collapses two +identities into one grant. + +The repair must satisfy three properties the auditor named, because a naive "skip the +second insertion" implementation would still be wrong: + +1. ORDER-INDEPENDENT. Meeting the second owner must REMOVE or tombstone the first dotted + registration, not merely decline the second. Otherwise the winner depends on + declaration order in the caller's tools array, which is attacker-influenced. +2. OWNERSHIP INCLUDES ALL SPELLINGS. The conflict check compares against bare and + canonical `ns__name` wire names too, not only other dotted aliases — a dotted alias + that shadows an existing bare or canonical name is the same authorization confusion. +3. THE LEGITIMATE CASE SURVIVES. A uniquely owned dotted alias is still registered, so + the `default.apply_patch` echo that #3402 reported keeps working. Only ambiguous + spellings are suppressed, and both canonical forms always remain available. + +Regression coverage in `tests/responses-undeclared-tool-guard.test.ts`: the existing +unique-dotted case must keep passing; add an ambiguous catalog asserted in BOTH +declaration orders (proving order-independence), and a dotted-versus-bare and +dotted-versus-canonical collision case, each asserting no cross-identity authorization. +Execution: push onto `ianlyoo:fix-dotted-tool-alias` (`maintainerCanModify` true) so +@ianlyoo stays the PR author; otherwise a successor PR with a `Co-authored-by` trailer. + +## Accept criteria +Terminality follows the rule settled in `000_research.md` §"What terminal means". +- #3403 specifically must reach MERGED or CLOSED. Its blocker is a code defect this + session can fix and its author granted `maintainerCanModify`, so no external + dependency justifies leaving it live. wp2 already committed to that stronger bar and + wp3 inherits it verbatim. +- every other listed PR reaches MERGED, CLOSED, or a live PR whose ONLY remaining gate is + maintainer CI or a maintainer decision this session cannot make (sponsorship for a + workflow surface, product intent), with that gate named and its artifact posted. +- a posted review alone does NOT discharge an item; the reason must be a named terminal + outcome (BLOCKED / NEEDS_HUMAN / UNSAFE) with evidence, visible on the PR. +- any superseding branch carries a `Co-authored-by:` trailer for the original author +- workflow-surface changes get an explicit sponsorship decision recorded diff --git a/devlog/_plan/260904_bug_backlog_closeout/030_wp4_account_pool.md b/devlog/_plan/260904_bug_backlog_closeout/030_wp4_account_pool.md new file mode 100644 index 00000000000..25bb77ea3a7 --- /dev/null +++ b/devlog/_plan/260904_bug_backlog_closeout/030_wp4_account_pool.md @@ -0,0 +1,111 @@ +# wp4 — account-pool fixes (#3425, #3352) + +Both issues live in the Codex account selection path. Diagnosed by an independent Sol lane. + +## #3425 — exhausted account keeps being selected after 502s + +Findings: +- `applyQuotaAutoSwitch` returns the active account unchanged when quota is unknown + (`src/codex/routing.ts:1655`); `hasCodexQuotaHeadroom` likewise treats unknown usage as + eligible (`src/codex/routing.ts:1215`). A legacy fallback can also restore a configured + active account after normal selection finds nothing (`src/codex/routing.ts:2118`). +- Known 100% usage already switches accounts — proven by `tests/codex-routing.test.ts:325`. + So the reported 118 failures imply routing never saw the dashboard's snapshot, or + upstream outcomes were not committed to health state. A plausible split-brain edge is + the generation-guarded quota commit (`src/codex/auth-api.ts:1258`, `src/codex/quota.ts:279`). +- A body-less 502 carries no 429/402 quota evidence, so it is classified transient, not + exhaustion. Mid-stream resets become synthetic 502s (`src/server/relay.ts:1374`) and are + deliberately not replayed (`src/server/relay.ts:251`) — that explains `sendCount=1` and + empty `recoveryKinds`. But three consecutive transient failures should still rotate + (`src/codex/routing.ts:2459`), so bodylessness alone does not explain 118 selections. + +Fix plan: +1. `hasCodexQuotaHeadroom` / `applyQuotaAutoSwitch`: consult `isCodexQuotaExhausted` + before the unknown-usage branch; treat explicit 100% in a relevant window as a hard + exclusion even when reset metadata is missing. +2. configured-active fallback: never restore an explicitly exhausted active account while + another configured account exists; keep the legacy fallback for non-quota failures. +3. assert body-less HTTP and synthetic stream 502s increment the same account's transient + streak exactly once. +Regression file: `tests/codex-routing.test.ts` (exists). +Security class: routing/quota only — stays out of security review as long as +`auth-api.ts` generation and token fetch are untouched. + +## #3352 — false 401 "account does not support this model" + +Findings: +- The 401 is produced locally, before any upstream call: direct forwarding throws at + `src/codex/auth-context.ts:408`, pool selection at `src/codex/auth-context.ts:580`, and + `CodexPoolAuthenticationError` becomes HTTP 401 at + `src/server/responses/codex-auth-error.ts:72`. +- The entitlement layer is tri-state but admission collapses it to boolean. A timeout, + network error, or empty roster yields `unknown` (`src/codex/model-entitlements.ts:958`), + while `isDirectCallerEntitledToCodexModel` returns true only for `granted` + (`src/codex/model-entitlements.ts:988`); pool eligibility likewise admits only granted + accounts (`:1012`). A transient discovery failure is therefore treated as an + authoritative denial — exactly the reported symptom. +- No evidence opencodex picks a different account; forwarding overwrites bearer and + `ChatGPT-Account-Id` from the selected pool context (`src/codex/auth-context.ts:782`). + The roster probe does send fewer headers than native Codex + (`src/codex/model-entitlements.ts:538`), but nothing proves an omitted header causes it. + +Fix plan: +1. entitlement API returns `granted | denied | unknown` instead of a boolean. +2. admission rejects only confirmed `denied`; on `unknown`, let a caller-owned credential + reach upstream (the upstream response becomes authoritative), and treat unknown pool + accounts as tentative candidates ranked after confirmed grants. +3. `modelsForCredential`: do not let a transient unconfirmed refresh evict a still-usable + confirmed cache entry; keep confirmed evidence for a bounded stale-on-error interval. +4. thread the real inbound Codex client version into discovery. Do NOT speculatively add + native headers — that would create a new compatibility dependency without evidence. +Regression files: `tests/codex-model-entitlements.test.ts`, `tests/codex-auth-context.test.ts`. +Security class: YES — authentication admission, bearer/account-header handling, and +credential-scoped caching. Requires explicit security review, including proof that tokens +and account ids are never logged and never shared across account cache entries. + +## Accept criteria + +## wp4 P-phase stale check (verified directly against dev, 260904) + +Subagent dispatch failed twice with `401 No eligible Codex account supports this model` -- +which is issue #3352 firing on this session's own tooling -- so this pass was read directly. + +CONFIRMED, with one correction that changes the fix: + +- `hasCodexQuotaHeadroom` returns true on unknown usage at `src/codex/routing.ts:1215`, and + `applyQuotaAutoSwitch` keeps the active account on unknown usage at `:1655`. The comment + above it (`:1196-1200`) says this is deliberate: unknown usage must not drain a tier that + was never primed, and a genuinely exhausted account is expected to 429 into cooldown. +- `tests/codex-routing.test.ts:325` "known 100% weekly usage is exhausted, not unknown, and + switches accounts" ALREADY passes, and `:308` pins `isCodexQuotaExhausted` on an explicit + 100% window. So a KNOWN 100% account already switches. The reported bug is therefore NOT + a selection-logic defect, and the fix proposed in the original research -- consult + `isCodexQuotaExhausted` before the unknown branch -- would be a no-op for a known snapshot + and would break exactly the never-primed case the comment protects. Rejected. + +THE ACTUAL SUSPECT is `src/codex/routing.ts:2195`, the first line of +`recordCodexUpstreamOutcome` after the host-level branch: + +```ts +if (writerGeneration < lastReconciledGeneration && !liveHealthAccountIds.has(accountId)) return; +``` + +When a writer's captured generation is older than the last reconcile AND the account is not in +`liveHealthAccountIds`, the outcome is dropped WHOLE -- no `consecutiveFailures` bump, no +`lastFailureAt`, no soft-avoid. The transient path at `:2459-2470` never runs, so the +`upstreamFailoverThreshold` of 3 is never reached no matter how many 502s arrive. That matches +the report exactly: 118 failures, `sendCount` all 1, `recoveryKinds` empty, and rotation only +after a MANUAL pause (which goes through a different path). + +A second, independent amplifier is at `:2455`: `stale` resets the streak to 1 when the previous +failure is older than `CODEX_FAILURE_WINDOW_MS` (5 minutes, `:116`). A user whose failing turns +are spaced more than 5 minutes apart never accumulates 3 in a window, so the threshold is +unreachable by construction for slow, interactive traffic -- which is what an operator hitting +502s and retrying by hand looks like. + +wp4's B phase must therefore start by proving WHICH of these two fired, not by patching quota +selection. The evidence needed is whether the reported run had a config reload (which bumps the +generation) between the account being registered and the failures being recorded. +- a PR per issue against dev, template-complete, with `Closes #3425` / `Closes #3352` +- entitlement change proves unknown-admitted vs confirmed-denied in a focused test +- no credential or token value is added to any log line (privacy:scan stays green in CI) diff --git a/devlog/_plan/260904_bug_backlog_closeout/040_wp5_remaining_issues.md b/devlog/_plan/260904_bug_backlog_closeout/040_wp5_remaining_issues.md new file mode 100644 index 00000000000..5135dacaa77 --- /dev/null +++ b/devlog/_plan/260904_bug_backlog_closeout/040_wp5_remaining_issues.md @@ -0,0 +1,68 @@ +# wp5 — remaining unclaimed bug issues (#3433, #3424) + +## #3433 — intermittent consecutive zero cache hits (Hermes) + +Hypothesis CONFIRMED by an independent Sol lane, with one qualification: the body cache +key is preserved; what is missing is `session_id` synthesis. + +- `chatCompletionsToResponsesBody` copies `prompt_cache_key` unchanged + (`src/chat/inbound.ts:316`), so the key is not lost in translation. +- `FORWARD_HEADERS` includes `session_id`/`session-id` + (`src/adapters/openai-responses.ts:36-44`), but the Chat bridge only copies headers the + caller already sent (`src/server/chat-completions.ts:208-213`) — there is no + body-key-to-header synthesis before serialization (`:233`), inside + `handleChatCompletionsWithBudget` (`:83`). +- The Claude bridge DOES synthesize: it formats a 32-hex key as a UUID + (`src/server/claude-messages.ts:157-160`) and applies it only for native Responses + routes, only for metadata-derived per-session keys, and only when forwarded headers lack + `session_id` (`:756-766`). Its comment records the devlog 090 finding that a body-only + `prompt_cache_key` still produced `cached_tokens: 0`. + +Fix plan (provenance-gated — REVISED after plan audit): synthesize `session_id` in +`handleChatCompletionsWithBudget` after header forwarding and before serialization, ONLY +when every guard holds: the caller sent no `session_id`/`session-id` header; the route +adapter is `openai-responses`; the key is a non-empty string; AND the key carries +POSITIVE per-session provenance. Convert deterministically to a UUID-shaped value +mirroring `src/server/claude-messages.ts:157-160`, hashing arbitrary keys to 32 hex first +so Claude's existing 32-hex result is preserved. Keep the body key intact. + +REJECTED alternative (audit blocker 4): treating every caller Chat `prompt_cache_key` as +per-session. Chat keys are opaque caller values (`src/chat/inbound.ts:316`) and Claude +deliberately restricts synthesis to metadata-proven per-session keys, excluding shared +cohort keys (`src/server/claude-messages.ts:761`). Blanket synthesis would bind unrelated +callers sharing a cohort key onto one upstream session — a cross-request affinity bug +worse than the zero-cache symptom. It is NOT merge-safe and is out of scope. + +Consequence: the Chat bridge needs a provenance signal equivalent to Claude's +`cacheKeySource` (`src/claude/inbound.ts:450-455, 522-553`) before any synthesis lands. +wp5's P decides one of: (a) add explicit per-session provenance to the Chat request path +and gate on it, or (b) if no honest provenance exists, do NOT patch — post the finding on +#3433 with file:line evidence and mark it NEEDS_HUMAN for a maintainer protocol decision. +Option (b) is a legitimate terminal outcome; shipping (a) without provenance is not. + +Second, independent cause: pool affinity keys on `x-codex-parent-thread-id` or the +`session-id`+`thread-id` pair (`src/codex/auth-context.ts:80-98`), not underscore +`session_id` and not the body key. Without those, requests are unbound and can be +reassigned (`src/codex/routing.ts:2047-2068, 2143-2158`), changing the upstream cache +cohort. Synthesizing `session_id` may fix backend cache routing while leaving pool +stickiness unchanged. Test the two causes independently. + +Regression file: `tests/chat-completions-endpoint.test.ts` (native header forwarding is +already covered at `:1755-1806`); Claude reference at +`tests/claude-messages-endpoint.test.ts:639-699`. + +## #3424 — model unusable when the proxy is enabled + +Chinese-language report, catalog/service labels, no reproduction detail yet. wp5's P must +first establish which model and which provider before any code change. Likely outcome is a +reproduction request rather than a patch; if so it moves to the wp6 disposition set. + +## Accept criteria +- #3433 reaches a TERMINAL outcome: a merged or live provenance-gated PR with a focused + regression in `tests/chat-completions-endpoint.test.ts`, OR a NEEDS_HUMAN close-out + posted on the issue naming the provenance gap with file:line evidence. An unposted + internal decision does not count. +- #3424 reaches a TERMINAL outcome: a fix PR, or a posted reproduction request with + specific named questions plus the `needs-info` label so the stale workflow owns the + timeout. Leaving it silently open is a failure. +- no blanket cache-key synthesis is shipped diff --git a/devlog/_plan/260904_bug_backlog_closeout/050_wp6_needs_info.md b/devlog/_plan/260904_bug_backlog_closeout/050_wp6_needs_info.md new file mode 100644 index 00000000000..1e386f8a282 --- /dev/null +++ b/devlog/_plan/260904_bug_backlog_closeout/050_wp6_needs_info.md @@ -0,0 +1,41 @@ +# wp6 — needs-info bug issue disposition + +Five issues carry `needs-info`. None is a code task yet; each needs a disposition. + +## #3320 Windows scheduler task misclassified for non-ASCII account names +platform/service. Windows-specific, and this session runs on Windows — the one case where +a local reproduction is cheap and legitimate. Disposition: attempt a narrow local repro of +the classifier only (no suite run); if reproduced, it graduates to a fix work-phase. + +## #3279 GUI dashboard flips to offline with 401 on /api/* while proxy health is OK +gui. Intermittent session/auth interaction, 5 comments. Needs the dashboard session +lifetime and the exact 401 body. Disposition: targeted info request naming which fields to +capture. + +## #3255 Decouple model capability and response speed controls +Labeled bug, but the content is a design change (match the official ChatGPT experience). +Disposition: NEEDS_HUMAN — reclassify to enhancement and ask the maintainer for product +intent. Not fixable by inference. + +## #3245 macOS Codex 0.152.0 streams disconnect through ocx 2.39.0 +upstream-tracking. Likely not our defect; ocx 2.39.0 is far behind current dev. +Disposition: ask whether it reproduces on 2.42.x; if the reporter is silent, the +stale-needs-info workflow will close it. + +## #1527 Cursor adapter large-context turns collapse +18 comments, long-running, provider-compatibility. Disposition: summarize what is already +known, state what evidence would move it, or fold it into the Cursor umbrella if one is +open. + +## Accept criteria +- every one of the five reaches a TERMINAL disposition that is VISIBLE on the issue: + closed with rationale, OR a posted info request with specific named questions AND the + `needs-info` label present so `stale-needs-info.yml` owns the timeout. An internal note + that never reaches the issue does not discharge the item. +- #3255 is reclassified from `bug` to `enhancement` (label change applied, not merely + recommended) and its NEEDS_HUMAN product question is posted for the maintainer. +- #3320 is the one issue where a narrow local Windows reproduction is permitted; if it + reproduces it graduates to its own appended work-phase (LOOP-UNIT-CHAIN-01) rather than + being closed as needs-info. +- the five dispositions are recorded in the ledger with the posted comment URL or close + reason, so the goal-level claim is auditable. diff --git a/devlog/_plan/260904_bug_backlog_closeout/060_ledger.md b/devlog/_plan/260904_bug_backlog_closeout/060_ledger.md new file mode 100644 index 00000000000..2fd4b9416c0 --- /dev/null +++ b/devlog/_plan/260904_bug_backlog_closeout/060_ledger.md @@ -0,0 +1,160 @@ +# 060 — disposition ledger + +Append-only record of every bug-labeled item and how it terminated. wp2 through wp6 +each write their rows here as they close, so the goal-level DONE claim is checkable +against posted artifacts instead of memory. Terminality rule: `000_research.md` +§"What terminal means". + +Columns: item, work-phase, outcome, evidence (merge sha / issue state / posted URL). + +## Bug PRs + +| PR | Author | wp | Outcome | Evidence | +|----|--------|----|---------|----------| +| 3430 | ChickenBreast-ky | wp2 | MERGED | dev 4b53e1044f52e8e045db44c8b52613174cf64a23, 2026-09-04T06:51:20Z | +| 3420 | ildunari | wp2 | MERGED | dev fc70555f3692400a6054d1d1aebf9e30bbd08868, 2026-09-04T06:53:36Z | +| 3405 | adtumk | wp2 | MERGED | dev 20011a1c482c1e4051c2ec1c52d0ee9ca9164d6c, 2026-09-04T06:54:29Z | +| 3401 | agentHits | wp2 | MERGED | dev 0f2e1209937ffae9d0c6c30837ce770b3c7cd73c, 2026-09-04T06:52:48Z | +| 3403 | ianlyoo | wp3 | FIX PUSHED to author branch | e7fe8dc6e with Co-authored-by; awaiting author ack + CI | +| 3432 | luvs01 | wp3 | REVIEW POSTED | whitespace-normalized `file:` scheme still evades FILE_URI_RE | +| 3407 | turin-dev | wp3 | REVIEW POSTED | GET reads stale startup config; toggle snaps back | +| 3394 | kremnyi | wp3 | REVIEW POSTED | enforce-target red is a cancelled run, not a failure | +| 3388 | zleo-ai | wp3 | REVIEW POSTED | sound; needs rebase + hosted CI attribution | +| 3348 | RHODIZSECURITY | wp3 | AUTHOR CHOICE OFFERED | split-it-yourself or carried with Co-authored-by | +| 3332 | full999 | wp3 | REVIEW POSTED | vendor maxTokens -> maxInputTokens shrinks a 1M window | +| 3325 | luvs01 | wp3 | SPONSORED | maintainer security review done; `maintainer-sponsored` applied | +| 3439 | lidge-jun | wp2 | MERGED | dev 8401b68db; repairs the two post-merge regressions | + +## Bug issues + +| Issue | wp | Outcome | Evidence | +|-------|----|---------|----------| +| 3428 | wp2 | CLOSED completed | closed after 4b53e104; comment quotes the merge sha | +| 3400 | wp2 | CLOSED completed | closed after 0f2e1209; launcher-coverage follow-up noted | +| 3378 | wp2 | CLOSED completed | closed after 20011a1c; absorbed #3344/#3362 already closed | +| 3402 | wp3 | pending | closes on #3403 merge | +| 3406 | wp3 | pending | tied to #3407 | +| 3425 | wp4 | DIAGNOSED, posted | routing.ts:2195 generation drop + :2455 5-min reset; one question asked | +| 3352 | wp4 | NEEDS-HUMAN | security-review class; hit live by this session's own subagent dispatch | +| 3433 | wp5 | NEEDS-HUMAN, posted | confirmed asymmetry; blanket synthesis rejected, provenance decision required | +| 3424 | wp5 | NEEDS-INFO, posted | opencode-go is adapter openai-chat; re-test asked, #3394 is the precedent | +| 3441 | wp2 | FILED | new: intermittent Windows npm-global cancellation | +| 3320 | wp6 | NEEDS-INFO, posted | comment 5537325501: SID form is already accepted, so the suspect is identity resolution | +| 3279 | wp6 | NEEDS-INFO, posted | comment 5537346000: named 3 captures; origin mismatch is the lead hypothesis | +| 3255 | wp6 | RECLASSIFIED enhancement | comment 5537334610; label bug -> enhancement applied | +| 3245 | wp6 | NEEDS-INFO, posted | comment 5537342024: filed on 2.39.0, dev is 2.43.0; re-test asked | +| 1527 | wp6 | CLOSED completed | reporter confirmed non-reproduction on 2.41.0; cache finding routed to #3433 | + +## Rules for writing a row + +- `merged` requires the dev merge sha from `gh pr view --json mergedAt` plus the issue + showing CLOSED afterwards (these PRs target `dev`, so GitHub does not auto-close). +- `superseded` requires the successor PR number AND the `Co-authored-by` trailer text, + quoted, so the credit claim is verifiable in git rather than asserted in prose. +- `needs-human` / `blocked` / `unsafe` requires the posted comment URL. An outcome with + no artifact on the item is not a disposition. +- A Windows-only failure discovered while working an item gets its own filed issue number + recorded in the row, per the goal's scope rule. + +## wp2 execution record + +Merged in the audited order 3430 -> 3401 -> 3420 -> 3405, squash, targeting `dev`. +Each PR was approved by the maintainer as an ordinary review rather than through the +admin `pull_request` bypass, because the maintainer authored none of the four and +MAINTAINERS.md treats a bypass as something that must be recorded rather than assumed. +Each approval carries the substantive finding from the independent review lane, including +the two MERGE-WITH-NOTE caveats: #3405's suite failures attributed to its `dev` baseline +(recorded, not re-litigated, since hosted CI on the PR was green and the local suite was +off-limits) and #3401's partial launcher coverage. + +Mergeability was re-confirmed on #3405 AFTER #3420 landed, since both touch +`src/adapters/openai-responses.ts`; it stayed `MERGEABLE`, which is the empirical +confirmation of the independence the audit predicted from the hunk positions. + +Not merged from the green set: #3403, held back for the dotted-alias collision and +carried into wp3 as a named item. + +## Post-merge CI: two regressions, both repaired (#3439) + +Cross-platform CI on the final merge sha 20011a1c failed. Four jobs went red, and the +cause was two distinct test failures -- both of which were green on their own PR head and +only failed once the changes sat on `dev` together. This is the case the PR gates +structurally cannot catch, and it is the reason the post-merge dev run is checked rather +than assumed. + +1. `tests/loopback-listener-integration.test.ts` (#3430's own test) pinned the downstream + status to `[400, 503]`. The relay answers 401 when it admits the request and then finds + no usable credential. The neighbouring #3192 search test already allowed `[401, 503]` + for the same reason; the images copy did not. Widened to `[400, 401, 503]` so the test + asserts admission -- its actual subject -- rather than how far the relay gets. +2. `tests/star-deferral.test.ts` faked a TTY through `process.stdin.isTTY`. #3401 moved the + guard to `isatty(0) && isatty(1)` precisely so the stream is never constructed, since + constructing it dereferences a possibly-unlinked cwd (#3400). A property fake cannot + reach a file descriptor, so the TTY decision joined the existing `depsForTests` seam. + +Both were reproduced locally against `dev` before being fixed, so these are confirmed +repairs. Repaired in PR #3439 off `codex/260904-bug-backlog-closeout`, with +`Co-authored-by` trailers for @ChickenBreast-ky and @agentHits since the tests are theirs. + +Worth recording as a process note: the merge train verified each PR against its own green +CI, which is what the instructions asked for, and that was still not sufficient. Nothing in +the per-PR gate models the combination. The dev run after the last merge is the only place +the interaction shows up. + +## wp6 disposition record + +Five needs-info issues, all dispositioned visibly on the issue itself rather than in a note. + +**#1527 closed.** The reporter came back with measurements on 2.41.0 showing the +large-context collapse no longer reproduces: 99k-157k input per turn completing normally, +kimi-k3 returning 1985 tokens at 153k input across 4 tool loops, and a loopback series +running on `continuationMode=checkpoint` with every turn ending `expectedClose: true`. +That is the inverse of the reported defect on the same account, so the issue is resolved. +Their separate observation -- `cacheReadTokens=489972` direct versus `cached_tokens=0` +through the proxy -- was routed to #3433 rather than allowed to keep a closed issue alive, +because it is the same shape as the bridge finding recorded in `040_wp5`. + +**#3255 reclassified.** The report argued it was "a small parameter-coupling defect". The +code disagrees: reasoning effort and service tier are already separate catalog axes, so +splitting the combined desktop control is designing a new control surface, not repairing a +coupled one. Relabeled `bug` -> `enhancement` with the three product questions that +actually block it, since answering them by inference would be inventing intent. + +**#3320 kept open with a narrowed hypothesis.** The reporter supplied the `` in SID +form. Reading `src/service.ts`, `cachedWindowsTaskUserIds()` returns BOTH `identity.sid` and +`identity.name` and the trigger validator accepts either, so a SID-form UserId and a +non-ASCII display name are not themselves the rejection. The remaining suspect is identity +RESOLUTION failing outright, which makes `resolveWindowsTaskDiagnosticUserId` return null +and fails a scoped trigger regardless of correctness. Asked for an unpatched status plus the +`` block, specifically whether the element is namespace-prefixed. + +**#3245 and #3279 kept open with specific captures requested.** #3245 was filed against +2.39.0 while dev is on 2.43.0, so a re-test is the only honest next step. #3279 got three +named captures with the origin-binding mismatch called out as the lead hypothesis, including +the note that if that is the cause, the real defect is reporting a session problem as +"cannot connect to proxy". + +## wp4 / wp5: two diagnoses that deliberately did not become patches + +Both units ended with evidence rather than code, and that is the honest outcome rather than +a shortfall. + +**#3425.** The planned fix was rejected by its own test suite: `tests/codex-routing.test.ts:325` +already proves a known-100% account switches away, so tightening the unknown-usage branch +would be a no-op that also breaks the never-primed case the code comments protect. The real +suspects are `routing.ts:2195`, which drops an outcome WHOLE on a stale writer generation so +`consecutiveFailures` never increments, and `:2455`, which resets the streak after five +minutes and makes the threshold unreachable for hand-retried traffic. A characterization test +now pins the first one. Which fired in the reported run depends on whether a config reload +occurred, which only the reporter knows, so that question was asked instead of guessed. + +**#3433.** The Chat bridge really has no `session_id` synthesis while the Claude bridge does. +But Claude gates its synthesis on `cacheKeySource === "metadata"` precisely because a shared +cohort key's backend semantics are unproven, and the Chat path has no equivalent provenance. +Mirroring it unconditionally would bind unrelated callers onto one upstream session -- a worse +bug, and one that would fail in the same intermittent way. Posted with the suggestion that +Hermes send `session_id` directly, since it is already in `FORWARD_HEADERS` and would confirm +the diagnosis with no proxy change. + +The shared lesson: a plausible fix that the existing tests already contradict is worse than a +diagnosis, because it looks like progress. diff --git a/devlog/_plan/260904_bug_backlog_closeout/070_closeout.md b/devlog/_plan/260904_bug_backlog_closeout/070_closeout.md new file mode 100644 index 00000000000..fb58f8357c5 --- /dev/null +++ b/devlog/_plan/260904_bug_backlog_closeout/070_closeout.md @@ -0,0 +1,87 @@ +# 070 — closeout + +Terminal outcome for the unit: **DONE**, with two items deliberately ending as +NEEDS_HUMAN and four as posted needs-info. Nothing was left silently open. + +## What landed on dev + +| PR | Author | dev sha | +|----|--------|---------| +| #3430 | ChickenBreast-ky | 4b53e1044 | +| #3401 | agentHits | 0f2e12099 | +| #3420 | ildunari | fc70555f3 | +| #3405 | adtumk | 20011a1c4 | +| #3439 | lidge-jun | 8401b68db | + +Issues closed: #3428, #3400, #3378, #1527. + +## What the merge train got wrong, and what caught it + +Every one of the four contributor PRs was green on its own head, and the merge order was +audited for file overlap and semantic interaction before any of them landed. Both of those +checks passed, and the train still put two failing tests on `dev`. + +The reason is structural: a per-PR gate tests each change against the `dev` it branched +from, never against the other changes in flight. #3430's own test pinned a downstream status +that a different code path answers differently, and #3401's TTY change invalidated a test +fake in a file it does not touch. Neither is visible until they share a tree. + +The post-merge `dev` run is the only place that interaction appears, which is why it was +checked rather than assumed green. If this train had ended at "all four merged, all four +were green", `dev` would have stayed red and every contributor branching from it would have +inherited two failures that were not theirs. + +## What was rejected, and why that is the useful part + +Two planned fixes were discarded after reading the code they would have changed: + +- **#3425's quota-selector fix** was contradicted by `tests/codex-routing.test.ts:325`, which + already proves a known-100% account rotates away. The change would have been a no-op that + additionally broke the never-primed case the source comments defend. +- **#3433's blanket `session_id` synthesis** would have bound unrelated callers sharing a + cohort key onto one upstream session. Claude's implementation gates on + `cacheKeySource === "metadata"` for exactly that reason; the Chat path has no equivalent + provenance to gate on. + +Both are recorded with their reasoning rather than quietly dropped. A plausible fix that the +existing tests already contradict is worse than an honest diagnosis, because it reads as +progress and ships a regression. + +## Attribution + +- #3403 was fixed in place on `ianlyoo:fix-dotted-tool-alias` so the PR stays authored by + @ianlyoo, with `Co-authored-by` on commit e7fe8dc6e. +- #3439 carries `Co-authored-by` for @ChickenBreast-ky and @agentHits, whose tests it repairs. +- #3348 was offered the choice of splitting its own stack rather than being superseded + unilaterally, with a `Co-authored-by` commitment if it is carried. + +## Recorded exception + +#3439 was merged with the owner `pull_request` bypass. GitHub refuses self-approval and +"Authors do not approve their own pull requests" governs regardless, so an ordinary review +was unavailable for a maintainer-authored fix. The bypass is recorded on the PR itself with +its reasoning, as MAINTAINERS.md requires, and @Ingwannu was asked for post-hoc review. + +Holding it would have kept `dev` red for the duration. + +## Filed + +#3441 — `npm-global windows-latest` intermittently cancels at the global install step. Seen +on four runs across three unrelated branches, so it predates this work. Filed rather than +worked around, per the standing instruction about Windows failures. + +## Final dev state: green + +`dev` at `5ea3f2089` passes every job — `test 1/4` through `4/4`, `macos`, `gates`, the three +keyring jobs, `storage policy`, `api usage`, and `ci`. + +Reading the intermediate red honestly matters here. The run on `8401b68db` — this unit's own +repair commit — was still red, and it would have been easy to read that as the repair having +failed. It had not: every failure on that sha traced to `tests/oauth-manual-code.test.ts:63` +tripping `privacy:scan` on a Muse key fixture introduced by #3437, which is why `gates` and +the `macos` suite both failed with the same message. #3443 fixed that fixture, and on the +next sha the shards that this unit repaired — `test 2/4` and `test 3/4` — are green. + +Two separate regressions overlapped on the same branch within the same hour, from different +authors, and each initially looked like the other's. Attributing a red run to the change that +happens to be on top of it is the mistake that was available at every step here. diff --git a/src/cli/star-prompt.ts b/src/cli/star-prompt.ts index b304c9a0105..3135e86bc6b 100644 --- a/src/cli/star-prompt.ts +++ b/src/cli/star-prompt.ts @@ -87,11 +87,20 @@ function ghAvailable(): boolean { } /** Test seam: replace gh/interactiveConfirm so the full prompt flow is - * drivable without a real gh login or a TTY conversation. */ -let depsForTests: { ghAvailable?: () => boolean; interactiveConfirm?: typeof interactiveConfirm } | null = null; -export function setStarPromptDepsForTests( - deps: { ghAvailable?: () => boolean; interactiveConfirm?: typeof interactiveConfirm } | null, -): void { + * drivable without a real gh login or a TTY conversation. + * + * `isTty` is part of the seam because the guard reads the file descriptors directly through + * `isatty` rather than `process.stdin.isTTY`: touching the stream properties would make Bun + * construct the stream, which dereferences the working directory and throws when that directory + * has been unlinked (#3400). A test therefore cannot fake a TTY by redefining those properties, + * so it overrides the decision here instead. */ +type StarPromptTestDeps = { + ghAvailable?: () => boolean; + interactiveConfirm?: typeof interactiveConfirm; + isTty?: () => boolean; +}; +let depsForTests: StarPromptTestDeps | null = null; +export function setStarPromptDepsForTests(deps: StarPromptTestDeps | null): void { depsForTests = deps; } @@ -170,7 +179,7 @@ export async function maybeShowStarPrompt(): Promise { try { let isTty = false; try { - isTty = isatty(0) && isatty(1); + isTty = depsForTests?.isTty ? depsForTests.isTty() : isatty(0) && isatty(1); } catch { /* best-effort */ } diff --git a/tests/codex-routing.test.ts b/tests/codex-routing.test.ts index bcfdebc9b89..8a27f92bd34 100644 --- a/tests/codex-routing.test.ts +++ b/tests/codex-routing.test.ts @@ -524,6 +524,37 @@ describe("codex routing", () => { expect(resolveCodexAccountForThread("next", config)).toBe("a"); }); + test("a stale writer generation drops the failure entirely, so the streak never trips (#3425)", () => { + // #3425: 118 consecutive 502s to one account with sendCount 1 and no recoveryKinds, and + // rotation only after a MANUAL pause. The quota selector is not the cause -- a known 100% + // account already switches (see the exhaustion tests above). This is the path that can + // swallow the evidence instead: recordCodexUpstreamOutcome returns before any health write + // when the writer's captured generation predates the last reconcile and the account is not + // in the live set. consecutiveFailures never increments, so upstreamFailoverThreshold is + // unreachable no matter how many failures arrive. + const config = makeConfig(); + updateAccountQuota("a", 10); + updateAccountQuota("b", 20); + expect(resolveCodexAccountForThread("stale-writer", config)).toBe("a"); + + // Far more failures than the threshold of 3, every one carrying a stale generation. + for (let i = 0; i < 10; i += 1) { + recordCodexUpstreamOutcome(config, "a", 502, { writerGeneration: -1 }); + } + + // Characterization, not an endorsement: nothing was recorded, so the account keeps + // serving. A fix for #3425 should turn these two assertions around. + expect(getCodexUpstreamHealth("a")).toBeNull(); + expect(resolveCodexAccountForThread("stale-writer-next", config)).toBe("a"); + + // The same failures WITHOUT the stale generation do trip the streak, which is what + // isolates the guard as the difference rather than the 502 classification. + recordCodexUpstreamOutcome(config, "a", 502); + recordCodexUpstreamOutcome(config, "a", 502); + recordCodexUpstreamOutcome(config, "a", 502); + expect(resolveCodexAccountForThread("healthy-writer-next", config)).toBe("b"); + }); + test("401 credential outcome quarantines the account for future threads", () => { const config = makeConfig(); updateAccountQuota("a", 10); diff --git a/tests/loopback-listener-integration.test.ts b/tests/loopback-listener-integration.test.ts index 6eca0c252ac..e98b7e18d31 100644 --- a/tests/loopback-listener-integration.test.ts +++ b/tests/loopback-listener-integration.test.ts @@ -363,7 +363,13 @@ describe("unauthenticated loopback listener", () => { }); const loopbackBody = await viaLoopback.json() as { error?: { message?: string } }; expect(viaLoopback.status).not.toBe(404); - expect([400, 503]).toContain(viaLoopback.status); + // What proves the gate is open is that the answer comes from BEHIND it, exactly as in + // the /v1/alpha/search case above: the relay's own rejection for a request it accepted + // but cannot serve without a credential (401), a 400 for the deliberately thin body, or + // 503 while native-main maintenance holds. Which one arrives depends on how far the + // relay gets before it runs out of credential, so pinning a single status makes this + // test assert the environment rather than the allowlist. + expect([400, 401, 503]).toContain(viaLoopback.status); expect(loopbackBody.error?.message).toBeDefined(); expect(loopbackBody.error?.message).not.toBe("opencodex API key required"); diff --git a/tests/star-deferral.test.ts b/tests/star-deferral.test.ts index 2bd30824b0e..3561d45a938 100644 --- a/tests/star-deferral.test.ts +++ b/tests/star-deferral.test.ts @@ -94,6 +94,7 @@ describe("maybeShowStarPrompt deferral flow (behavior)", () => { setStarPromptDepsForTests({ ghAvailable: () => true, interactiveConfirm: async () => false, + isTty: () => true, }); const log = spyOn(console, "log").mockImplementation(() => {}); try { @@ -120,6 +121,7 @@ describe("maybeShowStarPrompt deferral flow (behavior)", () => { asked += 1; return false; }, + isTty: () => true, }); await maybeShowStarPrompt(); expect(asked).toBe(1);