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 0000000000..33a9e9b3b2 --- /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 0000000000..5f80ff294e --- /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 0000000000..bba3a92d83 --- /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 0000000000..033d64e281 --- /dev/null +++ b/devlog/_plan/260904_bug_backlog_closeout/030_wp4_account_pool.md @@ -0,0 +1,70 @@ +# 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 +- 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 0000000000..5135dacaa7 --- /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 0000000000..1e386f8a28 --- /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 0000000000..1f3cc95e14 --- /dev/null +++ b/devlog/_plan/260904_bug_backlog_closeout/060_ledger.md @@ -0,0 +1,133 @@ +# 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 | pending | must reach MERGED or CLOSED | +| 3432 | luvs01 | wp3 | pending | | +| 3407 | turin-dev | wp3 | pending | | +| 3394 | kremnyi | wp3 | pending | | +| 3388 | zleo-ai | wp3 | pending | | +| 3348 | RHODIZSECURITY | wp3 | pending | supersede; needs Co-authored-by | +| 3332 | full999 | wp3 | pending | | +| 3325 | luvs01 | wp3 | pending | needs maintainer sponsorship | + +## 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 | pending | | +| 3352 | wp4 | pending | security-review class | +| 3433 | wp5 | pending | provenance decision required | +| 3424 | wp5 | pending | | +| 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". diff --git a/src/cli/star-prompt.ts b/src/cli/star-prompt.ts index b304c9a010..3135e86bc6 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/loopback-listener-integration.test.ts b/tests/loopback-listener-integration.test.ts index 6eca0c252a..e98b7e18d3 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 2bd30824b0..3561d45a93 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);