fix(settings): show resolved model in Local CLI tests - #6386
Conversation
|
Thanks @MumuTW — appreciate the clean resubmission and the explicit regression coverage around the Local CLI test path. I’m routing this now so the remaining review and validation steps can pick it up. |
lefarcen
added
size/M
lefarcen
added
the
needs-validation
|
🧪 This PR has changes that need a manual QA pass before merge — please hold off self-merging for now; we’ll loop QA in once it’s merge-ready (and any required design/product sign-off is in place). |
nettee
left a comment
nettee
left a comment
There was a problem hiding this comment.
@MumuTW I reviewed all five changed ranges and verified that Claude’s system/init model is captured through the connection-test sink, returned as the additive resolvedModel field, and rendered only for Local CLI successes when it differs from the selected slot. The alias/default semantics and existing Codex path details remain intact.
Validation completed locally: pnpm guard; contracts, daemon, and web typechecks; the daemon connection-test suite (157 tests); and the full Settings execution suite (150 tests).
This is a focused, well-tested fix—nice job keeping the contract backward-compatible and the UI output scoped to the intended case.
🔁 Powered by Looper · runner=reviewer · agent=codex · An autonomous AI dev team for your GitHub repos.
Visual regression reviewHead: 0 changed · 53 unchanged · 0 new without baseline · 0 failed Unchanged cases
Visual diff is advisory only and does not block merging. |
4 participants
Re-submission of #6183
This is a re-submission of
#6183(closed by the repo's stale-PR queue bot on Aug 3 after 5 days without human activity — "queue-management step, not a rejection"). GitHub does not allow reopening a PR whose head branch was force-pushed after it closed, so I'm opening fresh from the same branch.Since the original: the branch was rebased onto current
main(was 34 commits behind). The change itself is untouched — still a single commit (54da9104a), 121 insertions / 4 deletions across the same 5 files.Note on the previous CI run (30378493238): the failures there were infrastructure, not this diff — jobs were canceled by @AmyShang-alt and several others (
Validate workspace,Runtime summary, …) timed out after 24h awaiting a runner. The change already has:validatedlabel (Jul 29) — daemon/web regressions, typechecks, and related web tests all passedNothing about the diff changed since those approvals. @lefarcen @AmyShang-alt @mrcfps — could you merge when CI is green? Thanks!
Why
When I selected a rolling Claude Code alias such as
opusin Settings and clicked Test, Open Design only reported that Claude Code replied successfully. That proved connectivity, but not which concrete model actually handled the request.Claude Code already includes the concrete model in its
system/initstream event, and Open Design already parses that value. The connection-test sink discarded the status event, however, so the response preserved only the requested slot (opusordefault) and the UI could not show what the alias resolved to.This addresses the resolved-model transparency portion of #6180. The stale Claude fallback catalog is intentionally not changed here because #6095 already refreshes that list and includes Fable 5/current-generation coverage.
What users will see
A successful Local CLI connection test now appends the concrete model reported by the agent when it differs from the selected alias/default slot. For example:
Exact pinned selections are not repeated when the reported model equals the selected model.
Surface area
odsubcommand or flag, newtools-dev/tools-pack/tools-prflag, or newOD_*env varConnectionTestResponse.resolvedModelfieldskills/,design-systems/,design-templates/, orcraft/, or change to the skills protocolsettings.modellabelCLI parity
This does not add a new connection-test capability or endpoint; it preserves metadata in an existing Settings-only test flow. There is currently no
odconnection-test command to extend. Adding a new CLI command would be a separate user-facing capability and outside this focused bug fix. Any future CLI consumer of/api/test/connectionwill receive the same optionalresolvedModelfield from the shared contract.Screenshots
The visible change is a single appended detail in the existing Local CLI test status row:
The rendered behavior is covered by
apps/web/tests/components/SettingsDialog.execution.test.tsx.Bug fix verification
apps/daemon/tests/connection-test.test.ts—reports the concrete model resolved from a Claude aliasapps/web/tests/components/SettingsDialog.execution.test.tsx—shows the concrete model reported by a Local CLI connection testmain, green on this branch? Yes. Onmain, the daemon result lacksresolvedModel, and Settings omits the model from the success message. Both focused specs pass after the fix.Validation
Passed:
pnpm guardpnpm typecheck(full workspace)pnpm --filter @open-design/contracts typecheckpnpm --filter @open-design/daemon typecheckpnpm --filter @open-design/web typecheckFull daemon suite result on this machine:
main-baseline failures already documented on fix(daemon): refresh Claude Code fallback model list to the current generation #6095. The AMR/xAI failures do not import or exercise the changed contract, Claude connection-test sink, or Settings result rendering. The directly affected connection-test regression is green.Implementation
status/initializingmodel in the connection-test sink.modelremains the requested alias/default slot;resolvedModelcontains the concrete model reported by the CLI.