fix(dashboard): carry slot effective model id - #7834
Conversation
Open PR relationship auditThis 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
No PR, Issue, label, branch, or review state was changed by the relationship-note portion of this audit. |
ba13254 to
1aafd90
Compare
GPT 5.6 Review (fork) — ✅ no blocking findingsReviewed Review detailsFINDING -- src/kiro_crew/dashboard/state.py:4537 -- An unpinned slot’s |
UX Review (Fable 5, fork) — 🟡 CONCERNSUX-level review of 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
Evidence gaps
[UX-REVIEWED] c689606 |
Design Review (Fable 5, fork) — 🔴 BLOCK (blocking)Design-level review of Design-Verdict: BLOCK
BlockersBackend WatchRemoving the membership inference flips the pre-first-turn plan-downgrade window from Suggestions
[DESIGN-REVIEWED] c689606 |
First Principles Review (Fable 5, fork) — 🟡 CONCERNSPremise-level review of I have everything I need. All counts verified: First-Principles-Verdict: CONCERNS The fix is real and cause-level, but its "backward compat" riders — the legacy boolean arm, the dead What this change shipsIntent: stop the composer chip mislabeling a deprecated pinned model as
Watch
Subtractions
[FIRST-PRINCIPLES-REVIEWED] c689606 |
Opus 4.8 Review (fork) — ✅ no blocking findingsReviewed Review detailsI've verified all three grounding elements from code I opened at the PR commit:
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 Composer chip shows "Auto" for every unpinned slot whose agent resolves to a concrete model, defeating the FINDING — website/src/pages/ChatPage.tsx:6267 — an unpinned slot ( [OPUS-REVIEWED] c689606 |
|
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.
dee3c1f to
c689606
Compare
|
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). |
|
@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 What is still missing on main: nothing named Blocking defect before we can merge: Also please: declare 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 Posted from the 2026-09-08 open-PR relationship audit (read-only, one auditor per PR); reply here if any of this is wrong. |
|
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):
Net: every behavior this diff adds is already produced by Thanks to the reviewers — the Design/FP findings were correct against the September base and forced the re-verification that surfaced this. Leaving |
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, frontenddisplayModelmembership fallback +modelsDegradedgate, and the mirrorednormalizeModelKey/_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_modelsdrops deprecated ids (claude-opus-4.6-1m,claude-sonnet-4.6-1m) before entitlement narrowing, so a slot pinned to a deprecated id readsautountil a turn starts (_normalize_modelrewrites it to its replacement at turn start, which makes it self-correct). The label is wrong for that window, andautois not what will run — the replacement is.Why it matters
Two costs, both currently bounded but growing:
Mislabeled deprecated pin before first turn. The chip shows
autofor a deprecated pin that will actually run as its replacement. Any future catalog filter (curation, dedup) widens the class with no test tripping.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_modelis"auto"when withheld, the deprecated replacement when runnable,nullwhen entitlement is still unknown (fail open). The frontend prefers it; the boolean is kept for backward compat.Changes:
src/kiro_crew/dashboard/state.py: addSlot.effective_modelproperty (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 staysnull).src/kiro_crew/dashboard/slot_projection.py: exposeeffective_modelinto_dictalongsidemodel_withheld.website/src/lib/model.ts: extenddisplayModelto acceptstring | boolean | nulleffective; string path returns the effective's list spelling (or the effective itself) without needing membership inference;nullfails open to the pin;normalizeModelKeykept for fallback but no longer needed for the effective path.website/src/components/ChatPane.tsx,website/src/pages/ChatPage.tsx: passeffective_model ?? model_withheldtodisplayModel.Constraints kept: display stays firewalled from writes (pin-to-agent row writes
slot.model, nevereffective_model), pin is kept not cleared when withheld, unknown remains its own state and fails open.Tests
Backend:
isortandflake8clean onstate.py+slot_projection.py;mypy --platform linux --follow-imports=skipno issues;pytest -k PinnedModelWithheld23 passed (same as fix: carry the backend's model-withhold verdict in the slots payload #7546 coverage).Frontend:
tsconsrc/lib/model.tswith--skipLibCheckpasses; changed components compile via junctionednode_modules;displayModelnow 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. BackendTestPinnedModelWithheld(chat_runner + dashboard_chat) 30 passed.New behavior verified manually via
pythonimport: deprecatedclaude-opus-4.6-1m→claude-opus-4.6even without verdict, withheld →auto, runnable → pin, unknown non-deprecated →null.Manual verification
Created
_ChatSlotwith deprecated pin before verdict:effective_modelreturns replacement, chip would show replacement notauto.Verified
withheld True→auto,withheld False→ normalized pin, re-pin invalidates, teardown forgets, empty/auto pins returnauto.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 adisplayModelmembership check should be aneffective_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