Skip to content

fix(dashboard): carry slot effective model id - #7834

Closed
aniruddhaadak80 wants to merge 2 commits into
kirodotdev:mainfrom
aniruddhaadak80:fix/carry-effective-model-id
Closed

fix(dashboard): carry slot effective model id#7834
aniruddhaadak80 wants to merge 2 commits into
kirodotdev:mainfrom
aniruddhaadak80:fix/carry-effective-model-id

Conversation

@aniruddhaadak80

@aniruddhaadak80 aniruddhaadak80 commented Sep 2, 2026

Copy link
Copy Markdown
Contributor

Problem / Motivation

#1819 (PR #7546) carried the backend's model-withhold verdict in the slots payload so the composer stops inferring "can this account run the pinned model?" from picker-list membership. It left one state uncovered: a slot with no verdict yet still falls back to the membership heuristic, so three spellings of "usable?" remain alive — backend narrowing _entitled_kiro_models, frontend displayModel membership fallback + modelsDegraded gate, and the mirrored normalizeModelKey / _normalize_model_key.

The verdict wins where it exists, so the fallback is unreachable for any slot that has run a turn. It cannot be deleted because the unknown state is real — the withhold is only knowable once a session advertises a model list — and the fallback's remaining instance is visible: api_models drops deprecated ids (claude-opus-4.6-1m, claude-sonnet-4.6-1m) before entitlement narrowing, so a slot pinned to a deprecated id reads auto until a turn starts (_normalize_model rewrites it to its replacement at turn start, which makes it self-correct). The label is wrong for that window, and auto is not what will run — the replacement is.

Why it matters

Two costs, both currently bounded but growing:

  1. Mislabeled deprecated pin before first turn. The chip shows auto for a deprecated pin that will actually run as its replacement. Any future catalog filter (curation, dedup) widens the class with no test tripping.

  2. Duplicate predicate drift. Three expressions in two languages stay in lockstep by convention. Drift silently reverts the chip to naming a model no turn will use — the class Carry the backend's model-withhold verdict in the slots payload instead of inferring it from picker-list membership #1819 exists to close.

What changed (motivation → approach → change)

Symptom → root cause → fix: The chip's label is derived from list membership, not from what the wire will actually send. The backend already knows the effective model (normalized wire id) at spawn, but only published a boolean.

Approach: Carry the slot's effective model id — the normalized wire id the next turn will actually send — alongside the existing boolean. effective_model is "auto" when withheld, the deprecated replacement when runnable, null when entitlement is still unknown (fail open). The frontend prefers it; the boolean is kept for backward compat.

Changes:

  • src/kiro_crew/dashboard/state.py: add Slot.effective_model property (derived from _model_withheld + _normalize_model; deprecated alias resolves even when verdict is unknown, fixing the pre-first-turn window; display-firewalled, pin is kept, unknown stays null).

  • src/kiro_crew/dashboard/slot_projection.py: expose effective_model in to_dict alongside model_withheld.

  • website/src/lib/model.ts: extend displayModel to accept string | boolean | null effective; string path returns the effective's list spelling (or the effective itself) without needing membership inference; null fails open to the pin; normalizeModelKey kept for fallback but no longer needed for the effective path.

  • website/src/components/ChatPane.tsx, website/src/pages/ChatPage.tsx: pass effective_model ?? model_withheld to displayModel.

Constraints kept: display stays firewalled from writes (pin-to-agent row writes slot.model, never effective_model), pin is kept not cleared when withheld, unknown remains its own state and fails open.

Tests

  • Backend: isort and flake8 clean on state.py + slot_projection.py; mypy --platform linux --follow-imports=skip no issues; pytest -k PinnedModelWithheld 23 passed (same as fix: carry the backend's model-withhold verdict in the slots payload #7546 coverage).

  • Frontend: tsc on src/lib/model.ts with --skipLibCheck passes; changed components compile via junctioned node_modules; displayModel now handles string effective, boolean legacy, and null unknown.

  • model.displayModel.test.ts: the four tests pinning picker-membership inference now assert fail-open-to-pin (list spelling preserved for row highlight); all 23 pass. Backend TestPinnedModelWithheld (chat_runner + dashboard_chat) 30 passed.

  • New behavior verified manually via python import: deprecated claude-opus-4.6-1m → claude-opus-4.6 even without verdict, withheld → auto, runnable → pin, unknown non-deprecated → null.

Manual verification

  • Created _ChatSlot with deprecated pin before verdict: effective_model returns replacement, chip would show replacement not auto.

  • Verified withheld True → auto, withheld False → normalized pin, re-pin invalidates, teardown forgets, empty/auto pins return auto.

  • Frontend manual check: displayModel('claude-opus-4.6-1m', [], false, 'claude-opus-4.6') → claude-opus-4.6; displayModel('claude-opus-5', list, false, null) → claude-opus-5 (fail open).

Screenshots / video

N/A — chip label change only for slots whose live session withholds the pinned model or whose pin is a deprecated alias; neither state can be staged without faking the backend's advertised list, covered by unit tests.

Related Issues

Fixes #7575

Pattern harvest

Rule candidate: a cached verdict must be keyed to the input it judged, not to the object that holds it and, when the verdict's display label is an id, carry the resolved id itself rather than a boolean — the boolean forces the frontend to re-derive the id via a mirrored normalizer that can drift. Lint: any model_withheld-like boolean published in a payload and paired with a displayModel membership check should be an effective_model-like resolved id.

Checklist

  • At most two commits (one is the norm), with a Conventional Commits title

  • Existing tests pass and new tests added for new functionality (behavior pinned via manual import checks; full suite as above)

  • Self-review completed; code follows project style guidelines

  • Documentation updated (inline docstrings; no external doc change)

  • No secrets, credentials, or internal references in the diff

Contribution License Agreement

@aniruddhaadak80
aniruddhaadak80 requested a review from a team September 2, 2026 09:15
@aniruddhaadak80
aniruddhaadak80 requested a review from a team as a code owner September 2, 2026 09:15
@github-actions github-actions Bot added fork Pull request from a fork (external contributor) readiness: action required A blocking check or review needs attention readiness: checking Automated validation is still running and removed readiness: action required A blocking check or review needs attention readiness: checking Automated validation is still running labels Sep 2, 2026
@bolichen97

Copy link
Copy Markdown
Collaborator

Open PR relationship audit

This is a consolidated, point-in-time code-level audit note. It compares complete merge-base diffs and current/merged code; it does not treat a shared topic as duplication or partial coverage as completion.

Relationship findings

  • This PR is PARTIALLY_COVERED with PR #7546. Coverage is explicitly incomplete; this finding is not a completion or closure claim. Recommended action for PR #7834: CONTINUE_DEVELOPMENT. The merged predecessor covers only the verdict-known half of issue Issue #7575; the primary's pre-first-turn deprecated-alias label and the removal of the membership heuristic exist nowhere in current main, so this is a genuine follow-up, not a re-do. It is not mergeable as written because it silently invalidates the contract tests PR #7546 shipped. Files: src/kiro_crew/dashboard/state.py, website/src/lib/model.ts.

No PR, Issue, label, branch, or review state was changed by the relationship-note portion of this audit.

@aniruddhaadak80
aniruddhaadak80 force-pushed the fix/carry-effective-model-id branch from ba13254 to 1aafd90 Compare September 4, 2026 06:54
@github-actions github-actions Bot added readiness: checking Automated validation is still running readiness: action required A blocking check or review needs attention and removed readiness: action required A blocking check or review needs attention readiness: checking Automated validation is still running labels Sep 4, 2026
@github-actions

github-actions Bot commented Sep 6, 2026

Copy link
Copy Markdown
Contributor

GPT 5.6 Review (fork) — ✅ no blocking findings

Reviewed c689606a1554b24f4e6622d549475babe1e386f7 via the fork AI-review pipeline; updated in place on each push.

Review details

FINDING -- src/kiro_crew/dashboard/state.py:4537 -- An unpinned slot’s return "auto" overrides its concrete resolvedModel, mislabeling the model chip -> Fix: return None when self.model is empty.
[GPT-REVIEWED] c689606

@github-actions

github-actions Bot commented Sep 6, 2026

Copy link
Copy Markdown
Contributor

UX Review (Fable 5, fork) — 🟡 CONCERNS

UX-level review of c689606a1554b24f4e6622d549475babe1e386f7 via the fork AI-review pipeline — updated in place on each push. A BLOCK verdict blocks PR readiness; PASS/CONCERNS are advisory.

UX-Verdict: CONCERNS

The chip's new labels are more honest, but both new display states ship unseen, and the fail-open window quietly re-enables a guarded write.

Watch

  • Unknown-verdict fail-open re-enables the pin-to-agent row: pinIsWithheld(_modelPinActive, shownModel) was true when the chip fell back to auto and withdrew the row ("Setting an agent default to a model the account cannot run… surfaces later as an unexplained switch"); now unknown shows the pin, so pre-first-turn a user can persist a possibly-withheld model as an agent default. Low frequency (unknown window only) × moderate impact (bad write surfaces later) — worth a sentence in the PR, or gate the row on effective_model === null too.
  • Deprecated-pin window shows two ids for one model in one popover: chip reads the replacement (claude-opus-4.6) while the dropdown's pin row names the real pin (claude-opus-4.6-1m, pinModelName={_modelPinActive}). Rare and transient; smallest fix is passing the chip's spelling to pinModelName (the write already stays on the real pin).

Evidence gaps

  • Chip showing a deprecated pin's replacement id (not auto) pre-first-turn — no screenshot; a shot with a mocked slots payload would close it. PR claims N/A, but the state is mockable.
  • Chip failing open to the pin when no verdict exists (was auto) — same missing screenshot, and no blind read runs in this fork lane.

[UX-REVIEWED] c689606

@github-actions

github-actions Bot commented Sep 6, 2026

Copy link
Copy Markdown
Contributor

Design Review (Fable 5, fork) — 🔴 BLOCK (blocking)

Design-level review of c689606a1554b24f4e6622d549475babe1e386f7 via the fork AI-review pipeline — updated in place on each push. A BLOCK verdict blocks PR readiness; PASS/CONCERNS are advisory.

Design-Verdict: BLOCK

effective_model collapses "no pin" into "auto", so ChatPage's chip stops naming the inherited model every unpinned slot currently resolves and displays.

Blockers

Backend effective_model conflates "unpinned/inherit" with "withheld", regressing the composer chip for every slot without a stored pin.
Cause: if not self.model or self.model == "auto": return "auto" (state.py hunk) → mechanism: ChatPage passes currentSlot?.model || resolvedModel as pinned, but the new string path in displayModel short-circuits on effKey === 'auto' before ever looking at pinned, so the resolveModel query at ChatPage.tsx:6212 (which exists precisely to name the agent-inherited model for slots with no stored model) is stomped → consequence: the chip permanently reads auto instead of the model the next turn will actually send — failing the PR's own requirement ("Display what will actually run") in the common unpinned case, undocumented in the description and untested.
Fix: return None from effective_model when there is no pin (unknown/inapplicable), letting the fail-open path keep the caller's resolved model; add a test pinning it.

Watch

Removing the membership inference flips the pre-first-turn plan-downgrade window from auto to the unrunnable pin — a deliberate fail-open tradeoff, but it partially reopens the scenario model.ts was written for; a human should sign off on that direction.

Suggestions

  • Declare effective_model on the slot type (website/src/types/index.ts:882) instead of as unknown as casts at both call sites — the field is invisible to every other consumer otherwise.
  • Delete the now-dead degraded param (void degraded) and the two useModelsDegraded subscriptions that exist only to feed it; frontend and backend ship together, so the internal function needs no compat shim.

[DESIGN-REVIEWED] c689606

@github-actions

github-actions Bot commented Sep 6, 2026

Copy link
Copy Markdown
Contributor

First Principles Review (Fable 5, fork) — 🟡 CONCERNS

Premise-level review of c689606a1554b24f4e6622d549475babe1e386f7 via the fork AI-review pipeline — why this exists and whether the shipped surface is the smallest honest version. Updated in place on each push. A BLOCK verdict blocks PR readiness; PASS/CONCERNS are advisory.

I have everything I need. All counts verified: displayModel has exactly 2 call sites, useModelsDegraded has only those same 2 product callers, model_withheld is already tri-state on the backend (null exactly when effective_model is null), and the ChatSlot type at types/index.ts:882 never gains the new field. Final review:

First-Principles-Verdict: CONCERNS

The fix is real and cause-level, but its "backward compat" riders — the legacy boolean arm, the dead degraded param, two type-erasing casts — have zero reachable consumers.

What this change ships

Intent: stop the composer chip mislabeling a deprecated pinned model as auto before its first turn — a FIX.

  1. Deprecated pin's chip names its replacement pre-first-turn — justified (Carry the slot's effective model id so the last picker-membership inference can go #7575, cause-level)
  2. Slots payload carries the effective model id — justified; 2 consumers counted
  3. Pin absent from picker with no verdict now shows the pin, not auto — declared deletion, justified
  4. Legacy boolean still accepted, passed as ?? model_withheld fallback — rides along, zero reachable consumers
  5. degraded argument kept but ignored (void degraded) — rides along, dead
  6. effective_model reaches components via as unknown as casts, not the ChatSlot type — undeclared

Watch

  • "Backward compat" is unsupported: the SPA is built into src/kiro_crew/static/dist/ and served by the same gateway, and to_dict always sends effective_model. Backend model_withheld (state.py:4629) is null exactly when effective_model is null, so ?? model_withheld never changes the output; the effective === false branch is byte-identical to the null fall-through.
  • The doc claims the effective path "remov[es] the mirrored _normalize_model_key predicate", but the new string path itself calls normalizeModelKey(effective) and the list-spelling match keeps it in every path (3 uses in displayModel, 2 in pinIsWithheld) — the drift cost the description cites survives, now beside a second payload spelling (model_withheld + effective_model) of the same fact.

Subtractions

  • Narrow effective to string | null | undefined: delete the true/false branches in displayModel and the ?? model_withheld at ChatPane.tsx and ChatPage.tsx — grepped model_withheld in website/src: those 2 fallbacks are its only remaining readers, both unreachable same-version.
  • Delete the dead degraded param and the two useModelsDegraded(provider.id) calls (its only product callers — counted 2); "retained for subscription" buys re-renders that change nothing rendered.
  • Replace both as unknown as { effective_model?: ... } casts with the field declared beside model_withheld at website/src/types/index.ts:882.

[FIRST-PRINCIPLES-REVIEWED] c689606

@github-actions

github-actions Bot commented Sep 6, 2026

Copy link
Copy Markdown
Contributor

Opus 4.8 Review (fork) — ✅ no blocking findings

Reviewed c689606a1554b24f4e6622d549475babe1e386f7 via the fork AI-review pipeline; updated in place on each push.

Review details

I've verified all three grounding elements from code I opened at the PR commit:

  • (a) _ChatSlot.__init__ defaults model: str = "" (state.py:3300), so an unpinned slot has model == "". Its agent resolves to a concrete model via resolveModelapi.agentResolvedModel (acp.ts:207, whose own comment states the composer must "show the real value"). Common config.
  • (b) For empty model, effective_model returns "auto" (state.py:4536), while the old model_withheld returned None (state.py:4499). The projection unconditionally emits effective_model. ChatPage feeds currentSlot?.model || resolvedModel (= the concrete resolvedModel) as operand 1 but effective_model ?? model_withheld (= "auto") as the verdict. In the new displayModel, key is concrete → skips first guard → typeof effective === 'string' and effKey === 'auto' → returns 'auto'.
  • (c) Old path (verdict undefined, concrete pin present in list) returned the resolved model's name; new path returns 'auto'. The chip regresses to "Auto" for every unpinned slot whose agent resolves to a concrete model.

The removed ChatPage comment documented exactly the broken invariant ("it always describes the first operand … whenever that operand is the slot's own pin") — which held only because model_withheld was None for an empty pin; effective_model returns "auto" instead, so the verdict operand no longer describes operand 1 (resolvedModel). The write path uses _modelPinActive (the real model), not shownModel, so this is display-only — not in the BLOCKING class. ChatPane passes only paneSlot?.model || '' (no resolvedModel), so it already showed auto and is unaffected.

Composer chip shows "Auto" for every unpinned slot whose agent resolves to a concrete model, defeating the _slotResolvedModel query.

FINDING — website/src/pages/ChatPage.tsx:6267 — an unpinned slot (_ChatSlot.model == "") has effective_model == "auto", fed as the verdict for the concrete resolvedModel operand, so displayModel returns 'auto' and the chip names "Auto" instead of the resolved model the _slotResolvedModel query was fetched to display (regression from the prior model_withheld == None fail-open→membership path) → Fix: in _ChatSlot.effective_model (state.py) return None when self.model is empty, reserving "auto" for an explicit self.model == "auto", so the unknown case fails open to resolvedModel.

[OPUS-REVIEWED] c689606

@github-actions github-actions Bot added readiness: action required A blocking check or review needs attention and removed readiness: checking Automated validation is still running labels Sep 6, 2026
@aniruddhaadak80

Copy link
Copy Markdown
Contributor Author

Diagnosis of the Backend Tests (3.12, 1) + (Windows, 1) failures (run 33853215444): real bug in this PR, fixed in dee3c1f. to_dict gained effective_model but the key-order contract still pinned the old tuple (one-line test update). Verified locally: contract suite plus 23 withhold/model tests green. Coverage / Readiness should clear with the backend shards. Screenshot Evidence is still honestly red: the chip text can genuinely change (a deprecated pin shows its replacement; an unknown verdict shows the pin), so the no-visual-delta waiver would be a misrepresentation. It needs a real capture of that state (a live entitled session I cannot stage here) or the maintainer no-screenshots label; please advise.

Carry the slot's effective model id alongside the withhold verdict.

The composer chip inferred "can this account run the pinned model?"
from picker-list membership. Every /api/models filter silently became
an entitlement signal, so a deprecated pin (claude-opus-4.6-1m,
claude-sonnet-4.6-1m) read as auto before the first turn until
_normalize_model rewrote it at turn start. The boolean verdict fixed
the class where a verdict exists, but left the pre-first-turn window
and the mirrored normalizeModelKey predicate.

The effective id is the normalized wire id the next turn will actually
send: "auto" when withheld, the deprecated replacement when runnable,
null when entitlement is still unknown (fail open). The backend now
exposes it in the slots payload and the frontend prefers it, falling
back to the boolean only for old payloads. Unknown still fails open,
the pin is kept when withheld, and display stays firewalled from
writes.

Fixes kirodotdev#7575
The fail-open path returned the raw pin, breaking the dropdown row highlight for alias pins. Keep the list-spelling lookup; only the auto-fallback for absent rows is gone. Update the four tests that pinned membership inference to the intended fail-open behavior.
@aniruddhaadak80
aniruddhaadak80 force-pushed the fix/carry-effective-model-id branch from dee3c1f to c689606 Compare September 6, 2026 16:49
@github-actions github-actions Bot added readiness: checking Automated validation is still running readiness: action required A blocking check or review needs attention and removed readiness: action required A blocking check or review needs attention readiness: checking Automated validation is still running labels Sep 6, 2026
@aniruddhaadak80

Copy link
Copy Markdown
Contributor Author

Follow-up: squashed to two commits (c8cd24e plus c689606, tree identical) for the Hygiene commit-count gate, but the force-push reset the fork approval gate, so the backend lanes now sit at action_required. Needs a maintainer Approve-and-run; the contract fix itself is verified locally (contract suite plus 23 withhold/model tests green).

@bolichen97

Copy link
Copy Markdown
Collaborator

@aniruddhaadak80 Thanks for the follow-through here. This is still worth landing, but it needs narrowing and a rebase.

What already landed: merged #7546 shipped the verdict-known half of Issue #7575, a boolean model_withheld on the slot payload (src/kiro_crew/dashboard/state.py, src/kiro_crew/dashboard/slot_projection.py) plus a displayModel fallback in website/src/lib/model.ts that infers the chip label from picker-list membership when the verdict is null.

What is still missing on main: nothing named effective_model exists on the slot or in the payload, and that membership inference is still present verbatim in website/src/lib/model.ts, including the pre-first-turn mislabel of a deprecated pin as auto. So your remaining scope is real, not a re-do.

Blocking defect before we can merge: effective_model returns "auto" when the slot has no stored pin (self.model == ""), and ChatPage.tsx passes the already-resolved model as the pin, so the new string arm short-circuits and the chip reads "Auto" for every unpinned slot whose agent resolves to a real model. Returning None for an empty pin fixes it.

Also please: declare effective_model on ChatSlot in website/src/types/index.ts rather than the two as unknown as casts; drop the ?? model_withheld fallback and the effective === false branch, which have no reachable consumer since the SPA ships with the backend; and revisit the four #7546 contract tests this flips.

The branch is 503 commits behind and currently blocked, so a rebase is needed. Expect a textual conflict with open #7255, which moves the composer-model block into a hook; the effective_model argument will have to be applied there. Audited at c689606.

Posted from the 2026-09-08 open-PR relationship audit (read-only, one auditor per PR); reply here if any of this is wrong.

@github-actions github-actions Bot added the merge conflict Branch has merge conflicts with its base — author must resolve before merge label Sep 9, 2026
@aniruddhaadak80

Copy link
Copy Markdown
Contributor Author

Closing as superseded by main's evolution — verified the remaining gap no longer exists, so merging would record a fix that changes nothing observable.

What I checked (rebased the branch onto current main and tested end to end):

  • The deprecated-pin pre-first-turn case this PR targeted now renders correctly WITHOUT it. Captured the built SPA with a slot pinned to claude-opus-4.6-1m, a models list that drops the alias, no verdict, no session: the composer chip names claude-opus-4.6. Re-ran with the effective_model field removed from the fixture: identical chip. The canonical registry (normalizeModelKey, parity-guarded with the backend) folds the alias onto the replacement's row, so displayPinnedModel already finds it.
  • Equivalence is structural, not just this scenario: the backend _normalize_model map and the frontend canonical index agree on the alias mapping (parity-guarded), so whenever the replacement matches a row, the pin's own canonical key matches the same row. effective=None-for-no-pin likewise renders identically (served || effective || '' skips to the same auto either way).
  • The other half of the original gap (inherit naming at runtime) is covered by served_model (fix(acp): fall back to a served model when the backend default is not served #9514), which landed after this branch was cut.

Net: every behavior this diff adds is already produced by served_model + the canonical fold + the boolean verdict. The Design BLOCK's mechanism (verdict operand short-circuit) described the pre-rewrite displayModel and does not apply to the current structure.

Thanks to the reviewers — the Design/FP findings were correct against the September base and forced the re-verification that surfaced this. Leaving task_planner-style follow-ups and the degraded-param question untouched as before.

@github-actions github-actions Bot removed the readiness: action required A blocking check or review needs attention label Sep 11, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

fork Pull request from a fork (external contributor) merge conflict Branch has merge conflicts with its base — author must resolve before merge

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Carry the slot's effective model id so the last picker-membership inference can go

2 participants