diff --git a/devlog/_plan/260910_post249_round2/110_delivery_record.md b/devlog/_plan/260910_post249_round2/110_delivery_record.md index 81c81f3bb2..7831d67b2f 100644 --- a/devlog/_plan/260910_post249_round2/110_delivery_record.md +++ b/devlog/_plan/260910_post249_round2/110_delivery_record.md @@ -19,12 +19,12 @@ Append-only. One row per deliverable, filled when it actually lands on `dev`. | roadmap | — | #4155 | `3b9fab90e` | green | `a7509fe00` | — | | A1 | #4129 | #4157 | `421aea87a` | green | `4498fb910` | yes | | A2 | #4148 | #4161 | `799330bcf` | green | `5b8f1fcfa` | yes | -| A3 | #4141 | — | | | | in progress, #4152 landed | -| B1 | #3666 | #4156 | `3ff57ce49` | held | | | -| B2 | #4075 | #4158 | `3dc7bd19b` | held | | | +| A3 | #4141 | #4164 | `ae057c421` | green | `95a3f6a59` | yes | +| B1 | #3666 | #4156 | `db846c65d` | 23 green, gate red | | | +| B2 | #4075 | #4158 | `3dc7bd19b` | 23 green, gate red | | | | B3 | #3859 | #4160 | `c8a734cd0` | green | `8a5cfd366` | yes | -| B4 | #1711 | — | | | | awaiting decision | -| B5 | #4038 | — | | | | awaiting decision | +| B4 | #1711 | #4165 | `5c3e7e7ff` | 23 green, gate red | | | +| B5 | #4038 | #4166 | `d5e042c9c` | 23 green, gate red | | | | — | #4147 | #4153 | `abf35fa94` | green | `2ce5f381f` | yes | ### #4147 landed as the contributor's own commit @@ -70,7 +70,53 @@ The Lane B audit (`_research/_audit_wp3.md`) passed all three diffs but found on real defect: `gui/src/pages/Models.tsx:1405` and `:1475` still count the group header and `activeCount` from the unfiltered rows, so with the free-only filter on the header claims more models than the list shows. The empty state at `:1681` does -it correctly. Assigned to Lane B. +it correctly. Fixed by Lane B; #4156 is now at `db846c65d`. + +### Where the round stopped, and why + +Lane A is complete: #4129, #4148 and #4141 are all on `dev` and closed. #4147 and +#3859 are closed too. Five issues delivered. + +The remaining four — #3666, #4075, #1711, #4038 — are **code-complete and audited +PASS**, and every one of them sits at twenty-three green checks with +`enforce-target` as the only failure. Its message is literally +`missing UI screenshot`. + +That is the whole blocker. The gate requires a screenshot of the interface change, +producing one requires `bun run build:gui`, and this round forbids local builds. It +is not a false positive: PR #4162, which changed nothing but documentation, tripped +the same gate merely by quoting the trigger token in its description, and rewording +the description made the gate pass. So on four PRs that genuinely do change the +dashboard, the requirement is real and the maintainer has to choose between +allowing a build for screenshots, integrating past the gate with admin rights, or +carrying these four into a later round. + +#1711 and #4038 were started on the recommendations already recorded in their plan +docs rather than waiting further, because the round's instruction was to finish the +work. Each PR body states the contested choice: #1711 says plainly that a custom +catalog field cannot grey out the native Codex picker, which only understands +`list` and `hide`; #4038 names #4040 and explains that the minimum-decode-window +guard is what answers the objection that closed it. Both are cheap to revert. + +### Second-round audit findings, all fixed + +`_research/_audit_wp3b.md` passed #4165 and #4166 and raised four items, all since +addressed by Lane B: custom dashboard rows dropped `quotaInactiveReason` on the +rebuild from `config.customModels`; the zero-credit test covered the helper and +`deriveEntry` but not the gather-to-served-entry path; the Logs attempt table +rendered only the end-to-end rate although the DTO already carried the decode rate; +and the new field reached `/api/request-history`, which the plan had asked to keep +out. + +The audit also answered the question worth asking about #4165's earlier CI failure: +the fix filled a missing stamp on the `deriveEntry(null, …)` fallback and extended +the new test to cover both derivation paths. No existing catalog equality was +relaxed to make the suite pass. + +Fixing the last of those broke the typecheck, which is worth recording because it +is a direct cost of this round's constraints: with local typecheck forbidden, a +signature change is only discovered by remote CI, and the `gates` job's failing +step has to be read out of the workflow rather than seen locally. 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 diff --git a/devlog/_plan/260910_post249_round2/_research/_audit_wp3b.md b/devlog/_plan/260910_post249_round2/_research/_audit_wp3b.md new file mode 100644 index 0000000000..d09f5612cd --- /dev/null +++ b/devlog/_plan/260910_post249_round2/_research/_audit_wp3b.md @@ -0,0 +1,65 @@ +Read-only plan audit against `origin/dev` `95a3f6a59`. Heads: [#4165](https://github.com/lidge-jun/opencodex/pull/4165) `593360f28`, [#4166](https://github.com/lidge-jun/opencodex/pull/4166) `a3578aae5`. Local product tests were not run. + +Neither diff touches Lane A (`src/server/responses/core.ts`, `src/claude/inbound.ts`, `src/service.ts`). + +## #4165 — Closes #1711 + +The first-head CI failure was the new exact field assertion, not an old catalog `toEqual` being relaxed. At `04c4a8041`, `test 3/4` and `macos 1/2` failed with: + +``` +tests/codex-integration/catalog-zero-credit-picker.test.ts:131 +Expected: "no_credit" +Received: undefined +``` + +`deriveEntry(null, …)` took the no-template fallback, and that path did not stamp `opencodex_inactive_reason`. `593360f28` copied the existing stamp onto that fallback and looped the same assertions over both `template: null` and a cached template. The equalities stayed `visibility === "list"` and `entry[CATALOG_INACTIVE_REASON_FIELD] === "no_credit"`. + +```440:447:src/codex/catalog/sync.ts + if (model?.catalogKind) entry.opencodex_catalog_kind = model.catalogKind; + // Same additive stamp as the templated path above. + if (model?.quotaInactiveReason) entry[CATALOG_INACTIVE_REASON_FIELD] = model.quotaInactiveReason; +``` + +**NOTE** — CI-fix did not weaken an existing catalog-row equality. It extended the new test and filled the missing stamp. Before: one `deriveEntry(null, …)` path. After: both derivation paths, same `toBe("no_credit")` / `toBe("list")`. + +The inactive marker uses the combo runtime predicate. `quotaInactiveReason` filters disabled/unknown providers out of the vote, exempts canonical ChatGPT forward, treats a null/stale cache as not-exhausted, and returns `"no_credit"` only when every remaining usable target fails `cachedProviderQuotaIsExhausted` (`percent >= 100` and `remaining <= 0`, not Dashboard `quotaStateFromReport`). Tests pin stale cache, `percent: 40` at zero remaining, unlimited, ChatGPT exemption, and “every usable target.” + +`visibility` is still hardcoded `"list"` on both derive paths. The stamp is additive in `gatherRoutedModelsUncached` and `deriveEntry`; `filterCatalogVisibleModels` is unchanged. `ManagementModelRow.disabled` is not reused. + +The PR body states the native-picker limit in “The limitation, stated plainly”: Codex only understands `list`/`hide`, so this greys OpenCodex-aware consumers and does nothing in the native picker. It also records the catalog-plus-dashboard choice. + +**SHOULD-FIX** `src/server/management/model-rows.ts:129-147` — Custom dashboard rows are rebuilt from `config.customModels` and drop `quotaInactiveReason`. Routed/combo rows spread `CatalogModel` and keep it. Custom Models-tab chips will not show even when gather stamped the catalog row. + +**SHOULD-FIX** `tests/codex-integration/catalog-zero-credit-picker.test.ts:48-145` — Plan asked to seed quota and build through `buildCatalogEntriesFromObservedState`. Coverage is helper + `deriveEntry` only; gather→served-entry is not exercised end-to-end. + +**NOTE** `gui/src/pages/Models.tsx:1678-1686` — Dashboard chip is present for rows that carry the field. Exact-head `ci` is green; `enforce-target` still fails `missing UI screenshot`. + +Lane A files: no. + +## #4166 — Closes #4038 + +The #4040 failure mode is actually guarded. `MIN_DECODE_WINDOW_MS = 1_000`; a positive window under that floor returns `{ kind: "unavailable", reason: "decode_window_too_short" }`, not a rate. The test uses 240 tokens / 50 ms remainder and asserts no `4800`. + +```157:160:src/server/management/shared.ts + if (windowMs <= 0) return { kind: "unavailable", reason: "invalid_duration" }; + if (windowMs < MIN_DECODE_WINDOW_MS) return { kind: "unavailable", reason: "decode_window_too_short" }; +``` + +Existing e2e metric is unchanged: `tokensPerSecond` still divides by full duration, `tokPerSecondResult` still calls it, `filterLogs` still sorts on `tokPerSecond`, `RequestLogEntry` / `usage.jsonl` are untouched. New sibling: 240 tokens, 10s, TTFT 2s → decode 30, e2e 24. + +Parent uses the request’s `firstOutputMs`; each attempt uses its own. The combo test has TTFT only on the child: parent is `ttft_missing`, attempt is 30. + +`ttft_missing` and `decode_window_too_short` are on the server union, in `METRIC_REASON_KEYS` (`satisfies Record`), and in all nine locale files with real copy. + +The PR body states the contested choice: #4040 closed as an unreliable estimate, and this retry is the floor plus `estimated: true` because proxy TTFT is not the provider generation start. + +**SHOULD-FIX** `gui/src/pages/Logs.tsx:1151` — Plan said label the rate in the detail **and attempt** tables. Parent detail/list stack it; the attempt table still renders only `tokPerSecond`. The DTO already has `attempt.displayMetrics.decodeTokPerSecond`. + +**SHOULD-FIX** `src/server/management/request-history-routes.ts:109,187` — Plan said keep this out of `/api/request-history` even though it shares the DTO. `decodeTokPerSecond` is computed inside `requestLogDto`, so history responses now carry it. Additive, not persisted, but it is not logs-only. + +**NOTE** — `enforce-target` also fails `missing UI screenshot` on this GUI change. Exact-head `ci` is green. + +Lane A files: no. + +#4165 PASS +#4166 PASS