diff --git a/devlog/_plan/260910_post249_round2/110_delivery_record.md b/devlog/_plan/260910_post249_round2/110_delivery_record.md index 0a9f02d0dc..54c6eafa2a 100644 --- a/devlog/_plan/260910_post249_round2/110_delivery_record.md +++ b/devlog/_plan/260910_post249_round2/110_delivery_record.md @@ -14,18 +14,32 @@ Append-only. One row per deliverable, filled when it actually lands on `dev`. ## Ledger -| Item | Issue | Branch | PR | Head | CI | Merged | Issue closed | -|---|---|---|---|---|---|---|---| -| roadmap | — | `codex/devlog-post249-round2` | #4155 | `9abb66387` | green | | — | -| A1 | #4129 | `lane-a/1-4129` | | | | | | -| A2 | #4148 | `lane-a/2-4148` | | | | | | -| A3 | #4141 | `lane-a/3-4141` | | | | | | -| B1 | #3666 | `lane-b/1-3666` | | | | | | -| B2 | #4075 | `lane-b/2-4075` | | | | | | -| B3 | #3859 | `lane-b/3-3859` | | | | | | -| B4 | #1711 | `lane-b/4-1711` | | | | | | -| B5 | #4038 | `lane-b/5-4038` | | | | | | -| — | #4147 | contributor `fix/zcode-export-reasoning` | #4153 | | | | | +| Item | Issue | PR | Head | CI | Merged as | Issue closed | +|---|---|---|---|---|---|---| +| roadmap | — | #4155 | `3b9fab90e` | green | `a7509fe00` | — | +| A1 | #4129 | #4157 | `421aea87a` | green | `4498fb910` | yes | +| A2 | #4148 | #4161 | `799330bcf` | green | `5b8f1fcfa` | yes | +| A3 | #4141 | — | | | | held on #4152 | +| B1 | #3666 | #4156 | `3ff57ce49` | blocked | | | +| B2 | #4075 | #4158 | `3dc7bd19b` | blocked | | | +| B3 | #3859 | #4160 | | green | | parent blocked | +| B4 | #1711 | — | | | | awaiting decision | +| B5 | #4038 | — | | | | awaiting decision | +| — | #4147 | #4153 | `abf35fa94` | green | | | + +A1 and A2 were audited again **after** they landed, against `origin/dev` rather +than against the lane's own report. Both match the fix the plan chose, both +regression tests are genuinely red on the old code, and nothing the plan named as +"must stay green" was deleted to make the suite pass. Record: +`_research/_audit_wp2.md`. + +One behaviour worth knowing, found by that audit and not by the change itself: a +Claude request carrying **only** in-messages system text and no `metadata.user_id` +now emits no `prompt_cache_key` at all, because the fallback hashes `systemParts` +and that is empty once the reminders move into the timeline. That is absence, not +rotation — before the change those turns produced a key that moved every turn — so +it is an improvement, but a request in that exact shape no longer gets a proxy-set +key. ## Decisions taken during the round @@ -33,6 +47,28 @@ Record each one here as it happens, with who decided and on what evidence. A dropped item is a decision, not a gap — say why it was dropped and leave the issue open with a comment explaining the state it was left in. +**Lane re-split, main session, after the roadmap audit.** #1711 moved from Lane A +to Lane B because the two lanes' write sets overlapped in +`src/codex/catalog/parsing.ts`, `provider-fetch.ts` and `Models.tsx`. Lanes are +now 3 and 5. Evidence: `_research/_audit.md` finding 8, commit `9abb66387`. + +**#4148 scope, main session, recorded in the plan and the PR body.** Every +in-messages system message becomes a developer item, not only the ones after the +first user turn. A leading-only hoist keeps the old test green while still +mutating `instructions` when a client injects a fresh leading system message each +turn, which is the reported failure. + +**#4141 held, main session.** It must rebase onto PR #4152, which rewrites the +same `runLaunchctl` runner and belongs to the separate task investigating the +live-proxy shutdowns. Lane A was told to hold rather than invent a second seam. + +**Lane B dammed by the UI-screenshot gate, escalated to the maintainer.** +`enforce-target` requires a screenshot whenever a PR mentions `gui` and auto-drafts +until one exists, so #4156 and #4158 fail on that alone with every other check +green. Producing one needs `bun run build:gui`, which this round's no-local-build +constraint forbids. #4160 is fully green but sits behind them. Lane B reported +itself blocked rather than working around the gate, which is the correct behaviour. + ## Rules this record exists to enforce - A PR is only "merged" here once `git fetch origin && git merge-base --is-ancestor` diff --git a/devlog/_plan/260910_post249_round2/_research/_audit_pr4153.md b/devlog/_plan/260910_post249_round2/_research/_audit_pr4153.md new file mode 100644 index 0000000000..62e980e6ae --- /dev/null +++ b/devlog/_plan/260910_post249_round2/_research/_audit_pr4153.md @@ -0,0 +1,53 @@ +I applied the `cxc-dev` / `cxc-dev-code-reviewer` review path. This is a C2 exporter-contract review of #4153 at `abf35fa94`; I did not run the product suite. + +**1. Import and sanitizer — NOTE** +`sanitizeCodexReasoningEfforts` exists at [src/reasoning-effort.ts](/Users/jun/.codex/worktrees/ae6a/opencodex/src/reasoning-effort.ts:130). The new import in [src/clients/config-export/zcode.ts](/Users/jun/.codex/worktrees/ae6a/opencodex/src/clients/config-export/zcode.ts:4) (`../../reasoning-effort`) is the same path [src/clients/config-export/mcode.ts](/Users/jun/.codex/worktrees/ae6a/opencodex/src/clients/config-export/mcode.ts:4) already uses, and it resolves to `src/reasoning-effort.ts`. + +It keeps only exact `none` / `minimal` plus the Codex set `low|medium|high|xhigh|max|ultra`, drops duplicates, then sorts by that ladder ([src/reasoning-effort.ts](/Users/jun/.codex/worktrees/ae6a/opencodex/src/reasoning-effort.ts:5), [src/reasoning-effort.ts](/Users/jun/.codex/worktrees/ae6a/opencodex/src/reasoning-effort.ts:137)). It does not trim or lowercase. `turbo` is rejected there, not in the ZCode filter: it is not a sentinel and not in `CODEX_REASONING_SET`. After sanitize, `["none","high","ultra","turbo"]` is `["none","high","ultra"]`; [src/clients/config-export/zcode.ts](/Users/jun/.codex/worktrees/ae6a/opencodex/src/clients/config-export/zcode.ts:78) then drops `none`, which is why the second test expects `["high","ultra"]` ([tests/providers/zcode-client.test.ts](/Users/jun/.codex/worktrees/ae6a/opencodex/tests/providers/zcode-client.test.ts:129)). + +**2. Export model fields vs omp/dsh — NOTE** +The loop is `for (const model of normalizeExportModels(ctx.models))` ([src/clients/config-export/zcode.ts](/Users/jun/.codex/worktrees/ae6a/opencodex/src/clients/config-export/zcode.ts:59)). `normalizeExportModels` returns `ExportModel[]` unchanged except Fast expansion/sort ([src/clients/config-export/model-metadata.ts](/Users/jun/.codex/worktrees/ae6a/opencodex/src/clients/config-export/model-metadata.ts:91)). `ExportModel` has `reasoningEfforts` and `defaultReasoningEffort` ([src/clients/config-export/contracts.ts](/Users/jun/.codex/worktrees/ae6a/opencodex/src/clients/config-export/contracts.ts:69)). + +The three exporters all read `model.reasoningEfforts`, but they are not the same schema: + +- omp: client vocab without `ultra`/`none`; `reasoning: true` + `thinking.efforts` + optional `defaultLevel` ([src/clients/config-export/omp.ts](/Users/jun/.codex/worktrees/ae6a/opencodex/src/clients/config-export/omp.ts:40)) +- dsh: map `low|medium|high|xhigh|max`, with `ultra` as `max: "ultra"`; no default ([src/clients/config-export/dsh.ts](/Users/jun/.codex/worktrees/ae6a/opencodex/src/clients/config-export/dsh.ts:48)) +- zcode: same sanitize-then-drop-`none` ladder as mcode, plus omp-style default gating + +That is consistent for this client. The issue’s “like omp/dsh” means “read the catalog fields,” not copy those on-disk shapes. + +**3. 3.7.7 / 3.8.1 comment vs new `reasoning` — NOTE** +The 3.7.7 / 3.8.1 JSDoc is the provider-entry observation (`kind`, `apiKeyRequired`, loopback key) sitting above `ZcodeModelEntry` ([src/clients/config-export/zcode.ts](/Users/jun/.codex/worktrees/ae6a/opencodex/src/clients/config-export/zcode.ts:8)). It never asserted `levels` or `variants`. Optional `reasoning` does not contradict those provider claims. + +This repo already treats on-disk `reasoning` as a ZCode 3.8.1 model key, with `enabled` / `variants`, not `levels`: [src/integrations/ownership-policy.ts](/Users/jun/.codex/worktrees/ae6a/opencodex/src/integrations/ownership-policy.ts:58) and the writer fixture `{ enabled: true, variants: ["off", "high"] }` ([tests/clients/integrations-writer.test.ts](/Users/jun/.codex/worktrees/ae6a/opencodex/tests/clients/integrations-writer.test.ts:369)). That agrees with the new field and with the issue’s Expected block. The issue reproduction’s `model.reasoning.levels` is not a disk contract this tree records. `openCodeReasoningToModelReasoning` is contributor evidence, not something this repository proves. + +**4. Goldens / snapshots — NOTE** +The only byte-pinned ZCode export string is the facade golden in [tests/config/client-config-export.test.ts](/Users/jun/.codex/worktrees/ae6a/opencodex/tests/config/client-config-export.test.ts:123), and the PR updates it. [tests/providers/zcode-client.test.ts](/Users/jun/.codex/worktrees/ae6a/opencodex/tests/providers/zcode-client.test.ts:52) still exact-equals models that have no `reasoningEfforts`, so they stay reasoning-free. Writer tests use models without ladders ([tests/clients/integrations-writer.test.ts](/Users/jun/.codex/worktrees/ae6a/opencodex/tests/clients/integrations-writer.test.ts:40)). Docs samples do not pin ZCode model JSON. No other snapshot in this tree now disagrees. + +**5. Consumers — NOTE** +Callers that need the optional field: + +- [src/clients/config-export/zcode.ts](/Users/jun/.codex/worktrees/ae6a/opencodex/src/clients/config-export/zcode.ts:57) `buildZcodeClientConfig` (writes it) +- [src/clients/config-export/zcode.ts](/Users/jun/.codex/worktrees/ae6a/opencodex/src/clients/config-export/zcode.ts:113) `buildZcodeContribution` (whole provider fragment, field flows) +- [src/clients/config-export.ts](/Users/jun/.codex/worktrees/ae6a/opencodex/src/clients/config-export.ts:49) registry `build` / `buildContribution` +- type re-export [src/clients/config-export.ts](/Users/jun/.codex/worktrees/ae6a/opencodex/src/clients/config-export.ts:37) +- tests above + +`summarizeZcode` only counts `limit` ([src/clients/config-export/zcode.ts](/Users/jun/.codex/worktrees/ae6a/opencodex/src/clients/config-export/zcode.ts:108)). Ownership already lists `models.*.reasoning` as refreshable ([src/integrations/ownership-policy.ts](/Users/jun/.codex/worktrees/ae6a/opencodex/src/integrations/ownership-policy.ts:79)), so ZCode rewriting that key stays stale/refreshable, not a foreign edit. No extra consumer update is required. + +**6. `defaultVariant` omission and case — NOTE** +Omission is correct: `defaultVariant` is spread only when the trimmed/lowercased default is still in the emitted ladder ([src/clients/config-export/zcode.ts](/Users/jun/.codex/worktrees/ae6a/opencodex/src/clients/config-export/zcode.ts:81)). Default `"none"` is filtered out, so the Muse Spark case has no `defaultVariant` ([tests/providers/zcode-client.test.ts](/Users/jun/.codex/worktrees/ae6a/opencodex/tests/providers/zcode-client.test.ts:148)). + +Lowercase/trim is safe on the catalog path. Management/CLI store defaults as exact declared tokens (`none|minimal|low|medium|high|xhigh|max|ultra`) with no mixed case ([src/server/management/model-routes.ts](/Users/jun/.codex/worktrees/ae6a/opencodex/src/server/management/model-routes.ts:63), [src/cli/models.ts](/Users/jun/.codex/worktrees/ae6a/opencodex/src/cli/models.ts:66)). `toLowerCase()` is idempotent there and matches omp ([src/clients/config-export/omp.ts](/Users/jun/.codex/worktrees/ae6a/opencodex/src/clients/config-export/omp.ts:74)). Sanitize still requires exact lowercase on the ladder itself; that is also how the catalog is stored. + +**7. Maintainer issues** + +- **NOTE (preference, not defect):** #4147’s Expected snippet filters `ultra`. Keep `ultra`. ZCode forwards the selected variant as `reasoning_effort`, and `ultra` is a real Codex rung ([src/reasoning-effort.ts](/Users/jun/.codex/worktrees/ae6a/opencodex/src/reasoning-effort.ts:11)). omp’s no-`ultra` vocab is omp-only. +- **NOTE (preference, not defect):** `minimal` survives, same as mcode and OpenCode V2 ([src/clients/config-export.ts](/Users/jun/.codex/worktrees/ae6a/opencodex/src/clients/config-export.ts:585)). Hide it only if ZCode’s picker must not show that sentinel. +- **NOTE:** Emitting `reasoning` while still marking it refreshable means ZCode can rewrite OpenCodex’s ladder (the writer test uses `off` / `enabled: false`) and refresh puts the catalog block back. That matches existing 3.8.1 policy, not a merge blocker. +- **NOTE:** Docs checklist is ticked with no docs-site change. The integrations guide does not pin this schema; not a correctness fail. +- **NOTE:** Fork CI in `gh pr checks` is hygiene/target/label/resolve-pr/CodeRabbit only. Cross-platform CI still needs maintainer workflow approval before treating remote tests as proof. Touched files are unchanged between PR base `a7509fe00` and current `origin/dev` `4498fb910`, so this slice should merge cleanly. + +No correctness, contract, or security defect that should block merge. The loopback placeholder is unchanged. + +PASS diff --git a/devlog/_plan/260910_post249_round2/_research/_audit_wp2.md b/devlog/_plan/260910_post249_round2/_research/_audit_wp2.md new file mode 100644 index 0000000000..e98c4dfdd7 --- /dev/null +++ b/devlog/_plan/260910_post249_round2/_research/_audit_wp2.md @@ -0,0 +1,74 @@ +Post-merge audit against `origin/dev` `5b8f1fcfa` (local worktree HEAD is still `3b9fab90e`; all citations are `origin/dev`). No product tests were run. + +Both landings match the chosen fixes. The regression tests would be red on the old code, and the “must stay green” cases were not weakened. + +**NOTE** Local checkout is behind `origin/dev`. Grep in this worktree will still show the old hoist test. + +## #4129 / PR #4157 (`4498fb910`, head `421aea87a`) + +The lane implemented the plan, not a swap of the two blocks. + +In `handleResponsesInner`, a combo-named `shadowCallIntercept` is rewritten **before** `comboIdFromRawBody`: + +```3313:3336:src/server/responses/core.ts + if (!options.comboAttempt && body && typeof body === "object" && !Array.isArray(body)) { + const shadowIntercept = config.shadowCallIntercept; + const rawShadowModel = (body as { model?: unknown }).model; + if (shadowIntercept?.enabled && shadowIntercept.model && typeof rawShadowModel === "string" + && isShadowSourceModel(rawShadowModel, shadowIntercept.sourceModels)) { + const shadowComboId = resolveComboId(config, shadowIntercept.model); + if (shadowComboId && Object.hasOwn(config.combos ?? {}, shadowComboId)) { + (body as Record).model = shadowIntercept.model; + logCtx.shadowCallRewrittenFrom = sanitizeLogMetadataString( + shadowSourceModelPrefix(rawShadowModel, shadowIntercept.sourceModels), + ); + } + } + } + const comboId = !options.comboAttempt ? comboIdFromRawBody(body, config) : null; + if (comboId && Object.hasOwn(config.combos ?? {}, comboId)) { + return handleComboResponses(...) +``` + +Identity is `resolveComboId` in [src/combos/identifiers.ts](/Users/jun/.codex/worktrees/ae6a/opencodex/src/combos/identifiers.ts) (`parseComboModelId` + alias scan). That is config lookup. It does not go through `routeModel` / `tryPickComboModel`. + +The late site still uses `shouldInterceptShadowCall` for direct replacements ([src/server/responses/core.ts](/Users/jun/.codex/worktrees/ae6a/opencodex/src/server/responses/core.ts) ~3504–3527). Combo children pass `comboAttempt: true` ([src/server/responses/core.ts](/Users/jun/.codex/worktrees/ae6a/opencodex/src/server/responses/core.ts) ~2880), so they skip both the early rewrite and `comboIdFromRawBody`. Child bodies are `provider/model`; `isShadowSourceModel` hard-excludes slash ids, so the late site cannot recurse either. + +`shadowCallRewrittenFrom` is still the sanitized prefix, same helper as the late site. Success-path `Object.assign(logCtx, childLog, …)` does not copy that field off the child, and the new tests assert the marker after combo return. + +**Tests.** [tests/responses/responses-shadow-intercept.test.ts](/Users/jun/.codex/worktrees/ae6a/opencodex/tests/responses/responses-shadow-intercept.test.ts) gained the planned cases: + +- Hop: 429 then 200 → `urls` length 2, `logCtx.provider === "combo"`, `routeKind === "combo"`, `shadowCallRewrittenFrom === "gpt-5.6-luna"`, attempts `xai/grok-4.5` then `alt/grok-4.5` (lines 298–326). Red without the rewrite: `comboIdFromRawBody` still sees `gpt-5.6-luna`, so there is no failover loop. +- Intersecting first target: one upstream call, marker set, `routeKind === "combo"` (lines 329–359). Red without the rewrite: collapsed pick hits `shouldInterceptShadowCall` and leaves a native route with no marker. +- Extra keep-green: non-combo replacement still uses the late intercept (lines 362–377). + +Existing #2706 self-target and prefix-log tests were not edited. `421aea87a` only retargeted the **new** fixture off the pinned `openai` provider after CI proved the combo hop, then 401’d on `chatgpt.com`. Same intersect condition, using `sourceModels: ["custom-helper"]` and first target `xai/custom-helper`. + +**NOTE** [src/server/management/shadow-call-validation.ts](/Users/jun/.codex/worktrees/ae6a/opencodex/src/server/management/shadow-call-validation.ts):7 still validates via `routeModel`, so a dashboard PUT of a Luna-first `combo/shadow` can still 400. Plan called this follow-up unless trivial. + +**NOTE** `parsed._cursorIsolateConversation` is still not plumbed onto combo children. Plan deferred that. + +## #4148 / PR #4161 (`5b8f1fcfa`, head `799330bcf`) + +Matches the policy choice: **every** in-messages `role: "system"` becomes a chronological `developer` item, not a leading-only hoist. + +[src/claude/inbound.ts](/Users/jun/.codex/worktrees/ae6a/opencodex/src/claude/inbound.ts):329–360: top-level Anthropic `system` still goes through `systemToInstructions` into `systemParts` → `body.instructions`. In-messages system is pushed as `{ type: "message", role: "developer", content: [{ type: "input_text", text }] }`. No `role: "system"` input item. + +`parseRequest` still re-hoists `role: "system"` onto `systemPrompt` and keeps `developer` as a timeline message ([src/responses/parser.ts](/Users/jun/.codex/worktrees/ae6a/opencodex/src/responses/parser.ts) ~246–258). Using `developer` is the only shape that survives that parser. + +**`prompt_cache_key`.** The fallback still hashes `systemParts` ([src/claude/inbound.ts](/Users/jun/.codex/worktrees/ae6a/opencodex/src/claude/inbound.ts):385–402), now **only** top-level system. With top-level `system: "S"` and no `metadata.user_id`, the key is stable across turns (tested). + +If a request has **only** in-messages system and no `user_id`, `systemParts` is empty, so **no key is emitted** (`cacheKeySource = null`). That is absence, not rotation. Previously those turns hashed the reminders and the key moved. The plan’s test used top-level `"S"`; the existing “no metadata + no system: no key” case (line 532) still describes this edge. + +**Tests.** [tests/claude-integration/claude-inbound.test.ts](/Users/jun/.codex/worktrees/ae6a/opencodex/tests/claude-integration/claude-inbound.test.ts): + +- Old hoist test rewritten at 335: `instructions === "top-level"`, input roles `["developer","developer","user"]`, no `role === "system"`. Red on the old fold (`"top-level\n\nbe terse\n\nblock form"`). +- New #4148 case at 359: `turn1.instructions === turn2.instructions === "S"`, roles `["user","developer","assistant","user","developer"]`, texts `u1,r1,a1,u2,r2`, `prompt_cache_key` equal, schema + `parseRequest` both succeed. Red if reminders still land in `instructions`. + +Top-level-system cases at the old :66 / :429 / :439–492 sites were left as-is (now ~83, 497, 511+). Nothing in that file was deleted to stay green; the one rewritten test is the contract change the plan named. + +**NOTE** Out of scope as planned: `src/adapters/openai-chat.ts` still re-hoists developer text into a leading `system` chat message for non-`api.openai.com` Chat Completions. DeepSeek/SenseNova stay on the old prefix-bust path. Anthropic/Google outbound still present developer items as chronological `user`. + +No new secret logging, no `role: "system"` on the Responses wire, marker still sanitized. I do not see a contract or security blocker in either diff. + +PASS