From 7945635ab125ed30ca5c710d979ee66c0b4e81e8 Mon Sep 17 00:00:00 2001 From: JUN Date: Thu, 10 Sep 2026 09:54:37 +0900 Subject: [PATCH] docs(devlog): close the round-2 record at the screenshot gate Lane A is complete. #4141 landed as 95a3f6a59, joining #4129 and #4148, and with #4147 and #3859 that is five issues delivered and closed. The remaining four are code-complete and audited PASS, and each sits at twenty-three green checks with enforce-target as the only failure. Its message is literally "missing UI screenshot". Satisfying it needs a GUI build, which this round forbids, so the choice is the maintainer's: allow a build for screenshots, integrate past the gate with admin rights, or carry these four forward. It is not a false positive. PR #4162 changed nothing but documentation and tripped the same gate merely by quoting the trigger token in its description; rewording made it pass. On four PRs that do change the dashboard, the requirement is real. Records the second-round audit and its four findings, all since fixed, and the answer to the question worth asking about #4165's earlier CI failure: the repair filled a missing stamp on the deriveEntry fallback and extended the new test to both derivation paths, rather than relaxing an existing catalog equality to go green. Also records a direct cost of this round's constraints. Fixing the last finding broke the typecheck, and with local typecheck forbidden that was only discoverable from the remote gates job, whose failing step has to be read out of the workflow rather than seen locally. NOT RUN: local test suite, typecheck, build, lint. Remote CI is the gate. --- .../110_delivery_record.md | 58 +++++++++++++++-- .../_research/_audit_wp3b.md | 65 +++++++++++++++++++ 2 files changed, 117 insertions(+), 6 deletions(-) create mode 100644 devlog/_plan/260910_post249_round2/_research/_audit_wp3b.md diff --git a/devlog/_plan/260910_post249_round2/110_delivery_record.md b/devlog/_plan/260910_post249_round2/110_delivery_record.md index 81c81f3bb25..7831d67b2ff 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 00000000000..d09f5612cd2 --- /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