-
Notifications
You must be signed in to change notification settings - Fork 843
fix(subagents): keep a saved roster slot listed when its model is disabled #2134
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
5df624b
06e1313
d527c12
a83ee51
92178ea
ed6de17
f639a45
8c8fe80
06e0ac0
0a720d1
c253ce0
4a04ff9
df94a43
a19140a
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Large diffs are not rendered by default.
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,70 @@ | ||
| > **SUPERSEDED IN PART — read `000_research_inventory.md` amendments 1 and 2 first.** | ||
| > | ||
| > Two things in this document are wrong and were corrected after it was written: | ||
| > | ||
| > 1. **It is NOT a stack layer and does NOT root on #2134.** No dependency edge exists; | ||
| > the shipped PR (#2137) is based on `dev` as a sibling. | ||
| > 2. **The substitution predicate is NOT "native ChatGPT pool".** Pool-only would exclude | ||
| > `codexAccountMode: "direct"` and re-break #1686, whose Direct admission is only safe | ||
| > BECAUSE substitution still runs. The shipped predicate is | ||
| > `route.codexAccountMode !== undefined`, covering pool AND direct. Do not "correct" it back. | ||
|
|
||
|
|
||
| # 010 — Layer 1 (stack bottom): fix issue #2132, bearer admission must not force a ChatGPT credential | ||
|
|
||
| Work-phase: wp2. Branch: `codex/fix-bearer-admission-2132`. Base: `codex/fix-subagent-roster-truncation` (PR #2134). | ||
| Absorbs: nothing (no PR exists). Closes: #2132. | ||
|
|
||
| ## Why this is the stack bottom | ||
|
|
||
| It is the highest-scoring item in the backlog (96) and it shares `src/server/responses/core.ts` | ||
| with layer 2 (#2131). Layer 2 must be based on this, or the two edits to that file collide. | ||
|
|
||
| ## Defect | ||
|
|
||
| Reported in #2132: after v2.23.0, a key-auth provider (Cloudflare/etc.) returns 401 | ||
| `No usable Codex main credential` when `~/.codex/auth.json` holds no ChatGPT token. Bearer | ||
| admission sets `substituteMainCredential` unconditionally, so a route that needs no ChatGPT | ||
| identity is still gated on one. | ||
|
|
||
| ## P-phase re-verification required (stale check) | ||
|
|
||
| Before editing, confirm against the CURRENT tree — the lane read `dev`, not this branch: | ||
| 1. `rg -n "substituteMainCredential" src/` — enumerate every producer and consumer. | ||
| 2. Read `src/server/responses/core.ts`, `src/server/responses/compact.ts`, | ||
| `src/codex/auth-context.ts` and establish where the flag is set and where it is read. | ||
| 3. Reproduce the admission decision in a unit context with a key-auth provider and an | ||
| auth.json containing no ChatGPT token. If the current code does NOT reproduce, stop and | ||
| amend this doc rather than writing a fix for a defect that is not there. | ||
|
|
||
| ## Intended change | ||
|
|
||
| Make the substitution conditional on the resolved route actually requiring a native/ChatGPT | ||
| credential. A key-auth routed provider carries its own credential and must be admitted | ||
| without one. Exact call sites are fixed during the stale check above; the invariant is: | ||
| `substituteMainCredential` is set only when the route's credential source is the native | ||
| ChatGPT pool. | ||
|
|
||
| Out of scope: changing what happens once a native route legitimately lacks a credential, | ||
| and any change to the pool/account selection itself. | ||
|
|
||
| ## Test plan (must fail RED first) | ||
|
|
||
| New `tests/bearer-admission-key-auth.test.ts`: | ||
| 1. key-auth routed provider + auth.json with NO ChatGPT token -> request is admitted (no 401). | ||
| 2. native gpt route + no ChatGPT token -> still fails closed with the existing error. | ||
| 3. key-auth provider + ChatGPT token present -> unchanged behavior (no regression). | ||
|
|
||
| Drive the file against the unpatched tree first and record the failure output; a test that | ||
| passes before the fix does not prove anything. | ||
|
|
||
| ## Verification | ||
|
|
||
| `bun run typecheck`; `bun test --isolate` on the new file plus the existing responses/auth | ||
| suites; full `bun test --isolate tests` before marking review-ready; `bun run privacy:scan`. | ||
|
|
||
| ## Standalone thesis (DEV-STACK-03) | ||
|
|
||
| "A provider that carries its own key must not be gated on a ChatGPT credential." Builds and | ||
| passes its own tests at its own tip, independent of layer 2. | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,43 @@ | ||
| # 020 — Layer 2: absorb PR #2131, backfill missing Responses output ids | ||
|
|
||
| Work-phase: wp3. Branch: `codex/absorb-responses-id-backfill`. Base: `codex/fix-bearer-admission-2132` (layer 1). | ||
| Absorbs: **PR #2131 by @bet4it**. Closes: PR #2131 as superseded, with attribution. | ||
|
|
||
| ## Dependency edge (the only real one in this backlog) | ||
|
|
||
| #2131 adds `src/server/responses/responses-field-backfill.ts` and calls it from | ||
| `src/server/responses/core.ts` — the same file layer 1 edits. This is why it stacks rather | ||
| than sitting beside layer 1. | ||
|
|
||
| ## Defect | ||
|
|
||
| Strict decoders (grok-build) reject Responses output items that omit `id` on | ||
| `message` / `reasoning` / `function_call`. #1941 landed earlier but some relays still omit it. | ||
|
|
||
| ## Change to carry over | ||
|
|
||
| @bet4it's implementation, preserved in substance: synthesize stable `msg_ocx_N` / `rs_ocx_N` / | ||
| `fc_ocx_N` ids keyed on `output_index`, never overwriting an id that is already present. | ||
|
|
||
| ## Correction to apply on top (audit finding, lane: quality) | ||
|
|
||
| An invalid or missing `output_index` collapses to `0`, so two unindexed items can both become | ||
| `msg_ocx_0` — duplicate ids, which is the exact class of bug this fixes. Replace the | ||
| collapse-to-zero fallback with a monotonic per-response counter so synthesized ids are unique | ||
| even when `output_index` is absent or malformed. Add the regression test that pins it. | ||
|
|
||
| Docs: the locale files in #2131 are uneven (EN/FR rewritten, JA/KO/ZH/TR only first sentence). | ||
| Carry only the EN change in this layer; locale parity is not this layer's thesis. | ||
|
|
||
| ## Test plan (must fail RED first) | ||
|
|
||
| Carry @bet4it's tests (SSE `response.completed`, `output_item.done` via `output_index`, JSON | ||
| passthrough, preserve-existing-id, inherited `toString` type) and ADD: | ||
| - two items with missing `output_index` receive DISTINCT ids (fails on #2131 as written). | ||
|
|
||
| ## Verification | ||
|
|
||
| Same gate as layer 1, plus explicit confirmation that layer 2's branch contains layer 1's | ||
| commit (`git log --oneline <layer1>..<layer2>` shows only layer-2 commits) and that the PR | ||
| base ref names layer 1's branch. | ||
|
|
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,38 @@ | ||
| # 030 — Sibling A: consolidate prompt_cache_retention (issue #2092) | ||
|
|
||
| Work-phase: wp4. Branch: `codex/consolidate-prompt-cache-retention`. Base: **dev** (sibling, not a stack layer). | ||
| Absorbs: **PR #2102 by @lilinxiong** (base implementation). Supersedes: **#2099 by @yzxcj797**, **#2091 by @luvs01**. Closes #2092. | ||
|
|
||
| ## Why a sibling and not a layer | ||
|
|
||
| It touches only `src/adapters/openai-responses.ts`, which no other absorbed item touches. It has | ||
| no dependency on layers 1-2, so stacking it would impose a false merge order (DEV-STACK-01). | ||
|
|
||
| ## Chosen contract | ||
|
|
||
| @lilinxiong's #2102: strip `prompt_cache_retention` only when | ||
| `forward && isCanonicalOpenAiForwardProvider(provider)` AND the model is `gpt-5.6` or | ||
| `gpt-5.6-*`. This matches the issue's own correction — the reporter withdrew the "strip | ||
| everywhere" claim, and some non-5.6 deployments still honor the field. | ||
|
|
||
| Rejected: #2091's blanket strip for every forward provider and every model (it inverts the | ||
| existing gpt-5.5 preserve pin at tests/openai-responses-passthrough.test.ts:807). | ||
| Rejected: #2099's `startsWith("gpt-5.6")`, which also matches `gpt-5.60`, and its stray | ||
| package.json 2.24.2 -> 2.25.0 bump. | ||
|
|
||
| ## Carried from the superseded PRs | ||
|
|
||
| From @yzxcj797's #2099: the `Fixes #2092` issue link and the repro-shaped fixture | ||
| (`store:false`, streamed input array). From @luvs01's #2091: nothing — its key-auth preserve | ||
| case is already covered by #2102. | ||
|
|
||
| ## Tightening to apply | ||
|
|
||
| Replace the string-prefix family match with the catalog/native-slug predicate if one exists | ||
| in the current tree (`rg -n "isGpt56NativeSlug|NATIVE_OPENAI_MODELS" src/`); otherwise keep | ||
| the exact `gpt-5.6` / `gpt-5.6-*` match and pin `gpt-5.60` as a NON-match in tests. | ||
|
|
||
| ## Test plan (must fail RED first) | ||
|
|
||
| Carry #2102's tests; add `gpt-5.60` non-match; keep the gpt-5.5 preserve pin intact. | ||
|
|
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,31 @@ | ||
| # 040 — Sibling B: routing capability + lab behavior evidence | ||
|
|
||
| Work-phase: wp5. Branch: `codex/absorb-capability-evidence`. Base: **dev**. | ||
| Absorbs: **PR #2100 and PR #2077, both by @ntdatt812**. Closes both as superseded. | ||
|
|
||
| ## Why these two together, and why a sibling | ||
|
|
||
| #2100 touches `src/routing/capability.ts`; #2077 touches | ||
| `src/routing/compatibility/behavior.ts`. Disjoint files, one author, one thesis: *model-keyed | ||
| lookups must use the same resolution rules the runtime uses*. Neither depends on layers 1-2. | ||
|
|
||
| Note: #2077 is Lab-adjacent. Verify `tests/core-lab-boundary.test.ts` stays green — the file | ||
| already imports Lab types, so this must not newly puncture the boundary. | ||
|
|
||
| ## Defects | ||
|
|
||
| #2100: bare map lookups made `gpt-oss:120b` inherit the provider-wide 8k window instead of the | ||
| `gpt-oss` family's 131072, and `noVisionModels` was ignored. | ||
| #2077: `map[modelId]` missed family/case overrides, and `constructor` resolved to | ||
| `Object.prototype.constructor`, making `jcsStringify` throw and silently dropping Lab subjects. | ||
|
|
||
| ## Change | ||
|
|
||
| Route both through `modelRecordValue` / `isModelTextOnly` as @ntdatt812 wrote them. Prototype-id | ||
| safety (`constructor`, `toString`) is the load-bearing part; keep those tests verbatim. | ||
|
|
||
| ## Test plan | ||
|
|
||
| Carry both test files. Confirm the exact-own maps (`modelPreferHostedTools`, | ||
| `modelOpenRouterRouting`) still do NOT family-spread. | ||
|
|
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,25 @@ | ||
| # 050 — Sibling C: K12 short-window quota (issue #2047) | ||
|
|
||
| Work-phase: wp6. Branch: `codex/absorb-k12-short-window`. Base: **dev**. | ||
| Absorbs: **PR #2056 by @Ingwannu**. Supersedes: **#2062 by @yzxcj797**. Closes #2047. | ||
|
|
||
| ## Chosen base | ||
|
|
||
| #2056 is a strict superset of #2062: `snapshotHasShort`, partial-snapshot preservation, | ||
| `updateAccountQuota` carry, and the parse -> cache -> DTO path #2047 actually requires. #2062 | ||
| drops short on a later weekly/monthly partial snapshot and carries a stray version bump. | ||
|
Comment on lines
+8
to
+10
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win Do not call The inventory amendment acknowledges that 🧰 Tools🪛 markdownlint-cli2 (0.23.2)[warning] 8-8: No space after hash on atx style heading (MD018, no-missing-space-atx) 🤖 Prompt for AI Agents |
||
|
|
||
| ## Blocker to fix before this can land (raised by the maintainer on both PRs) | ||
|
|
||
| A short-only snapshot with `shortPercent: 0` scores `0` instead of `CODEX_UNKNOWN_USAGE_SCORE`, | ||
| so `pickLowestUsageAmong` prefers an account whose long windows are unverified. Fix: | ||
| include `shortPercent` in `computeCodexUsageScore` only when the plan's governing long window | ||
| is finite; otherwise return `CODEX_UNKNOWN_USAGE_SCORE`. Add the short-only regression. | ||
|
|
||
| This blocker is why #2056 is absorbed-and-corrected rather than simply approved. | ||
|
|
||
| ## Also close | ||
|
|
||
| **#2063 by @yzxcj797** — superseded by ALREADY-MERGED #2055 (`2648ffa87`), which classifies | ||
| `detail.code` with a stricter own-property lookup. Close with attribution; fold nothing. | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,37 @@ | ||
| # 060 — Close-out: supersede operations and attribution | ||
|
|
||
| Work-phase: wp7. No code. GitHub state only. | ||
|
|
||
| ## Ordering rule | ||
|
|
||
| A PR is closed ONLY after its replacement exists and is pushed. Never close first. | ||
|
|
||
| ## Operations | ||
|
|
||
| | Close | Author | Replaced by | Carried over | | ||
| |---|---|---|---| | ||
| | #2131 | @bet4it | layer 2 (020) | full implementation + tests, plus unique-id correction | | ||
| | #2099 | @yzxcj797 | sibling A (030) | issue link, repro fixture | | ||
| | #2091 | @luvs01 | sibling A (030) | nothing; contract deliberately narrower | | ||
| | #2100 | @ntdatt812 | sibling B (040) | full implementation + tests | | ||
| | #2077 | @ntdatt812 | sibling B (040) | full implementation + tests | | ||
| | #2102 | @lilinxiong | sibling A (030) | full implementation + tests (base) | | ||
| | #2062 | @yzxcj797 | sibling C (050) | nothing; #2056 supersedes | | ||
| | #2063 | @yzxcj797 | merged #2055 | nothing | | ||
| | #2056 | @Ingwannu | sibling C (050) | full implementation + scorer correction | | ||
| | #2029 | @yzxcj797 | maintainer PR #2130 | nothing; #2130 is a superset | | ||
|
Comment on lines
+11
to
+22
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win Synchronize the close-out operations with the final campaign state. Line 13 still names layer 2 for Also applies to: 29-37 🤖 Prompt for AI Agents |
||
|
|
||
| ## Comment template | ||
|
|
||
| > Thanks for this, @<login> — closing as superseded by #<n>, which carries <what> from your | ||
| > patch. <What changed and why.> Your work is credited in that PR's description. | ||
|
|
||
| ## NOT closed, with reasons stated publicly | ||
|
|
||
| - **#2109 / #2110** (@drakonkat): unresolved security gap in the override gate; needs a human | ||
| security pass (AGENTS.md security boundary). | ||
| - **#2053** (@Ingwannu): C4 OAuth surface; MAINTAINERS.md mandates security review. | ||
| - **#2101, #2040**: large (20 and 14 files); each needs its own PABCD cycle. | ||
| - **#2115, #2082, #2027, #2067, #2054, #2032**: below the 60 threshold. | ||
| - **#2104, #2075, #2127**: #2075/#2054 CONFLICTING; #2127 is an active draft by its author. | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Synchronize all remediation plans with the corrected topology and phase map.
The inventory amendment and execution log define independent sibling PRs from
dev, but several plans still contain obsolete ancestry, dependency, predicate, or phase metadata.devlog/_plan/260820_bug_pr_backlog_consolidation/010_layer1_bearer_admission_2132.md#L13-L21: describe the work aswp2fromdev, not as a stack layer based on PR#2134.devlog/_plan/260820_bug_pr_backlog_consolidation/010_layer1_bearer_admission_2132.md#L40-L46: useroute.codexAccountMode !== undefined, which coverspoolanddirect.devlog/_plan/260820_bug_pr_backlog_consolidation/020_layer2_responses_id_backfill_2131.md#L3-L10: describe the work aswp6, based ondev, with no dependency on layer 1.devlog/_plan/260820_bug_pr_backlog_consolidation/020_layer2_responses_id_backfill_2131.md#L40-L42: remove verification that requires a layer-1 ancestor.devlog/_plan/260820_bug_pr_backlog_consolidation/030_sibling_prompt_cache_retention.md#L3-L4: change the work phase towp3.devlog/_plan/260820_bug_pr_backlog_consolidation/040_sibling_routing_capability.md#L3-L4: change the work phase towp4.devlog/_plan/260820_bug_pr_backlog_consolidation/050_sibling_k12_short_window.md#L3-L4: change the work phase towp5.📍 Affects 5 files
devlog/_plan/260820_bug_pr_backlog_consolidation/010_layer1_bearer_admission_2132.md#L13-L21(this comment)devlog/_plan/260820_bug_pr_backlog_consolidation/010_layer1_bearer_admission_2132.md#L40-L46devlog/_plan/260820_bug_pr_backlog_consolidation/020_layer2_responses_id_backfill_2131.md#L3-L10devlog/_plan/260820_bug_pr_backlog_consolidation/020_layer2_responses_id_backfill_2131.md#L40-L42devlog/_plan/260820_bug_pr_backlog_consolidation/030_sibling_prompt_cache_retention.md#L3-L4devlog/_plan/260820_bug_pr_backlog_consolidation/040_sibling_routing_capability.md#L3-L4devlog/_plan/260820_bug_pr_backlog_consolidation/050_sibling_k12_short_window.md#L3-L4🤖 Prompt for AI Agents