From 8d751bbfed0661a3f2bd8d115f77a6cb7ddbfa93 Mon Sep 17 00:00:00 2001 From: JUN Date: Thu, 10 Sep 2026 08:22:33 +0900 Subject: [PATCH] docs(devlog): record the round-2 landings and the two blockers Fills the ledger for what has actually landed: the roadmap as a7509fe00, #4129 as 4498fb910 and #4148 as 5b8f1fcfa, each with the head SHA whose CI was proven by exit code. Also records the four decisions taken so far and the two things currently holding the round. A1 and A2 were audited a second time after they landed, against origin/dev rather than against the lanes' own reports. Both match the fix the plan chose and both regression tests are red on the old code. That audit found one behaviour the change itself did not mention. A Claude request carrying only in-messages system text and no metadata.user_id now emits no prompt_cache_key, because the fallback hashes systemParts and that is empty once the reminders move into the timeline. Before the change those turns produced a key that moved every turn, so this is an improvement, but a request in that exact shape no longer gets a proxy-set key and that is worth having written down. NOT RUN: local test suite, typecheck, build, lint. Remote CI is the gate. --- .../110_delivery_record.md | 60 ++++++++++++--- .../_research/_audit_pr4153.md | 53 +++++++++++++ .../_research/_audit_wp2.md | 74 +++++++++++++++++++ 3 files changed, 175 insertions(+), 12 deletions(-) create mode 100644 devlog/_plan/260910_post249_round2/_research/_audit_pr4153.md create mode 100644 devlog/_plan/260910_post249_round2/_research/_audit_wp2.md 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