fix(dashboard): stop rendering an approval-parked subagent as running - #7477
Conversation
github-actions
Bot
added
the
readiness: checking
Design Review (Fable 5) — ✅ PASSDesign-level review of Design-Verdict: PASS A real UI lie fixed at its actual root — one exported predicate replacing per-renderer re-derivation — with the backend dependency correctly deferred to the open PR that owns it. Watch
[DESIGN-REVIEWED] d435335 |
github-actions
Bot
added
readiness: action required
First Principles Review (Fable 5) — ✅ PASSPremise-level review of All claims verified: the predicate now has 5 real consumers across the two renderers plus the slice's own selectors; First-Principles-Verdict: PASS Two tally surfaces asserted "running" for a run blocked on the user; this exports the one existing predicate and repairs both, deferring the backend hop that can't build yet. What this change shipsIntent: stop the dashboard telling the user work is in progress when a sub-agent wave is actually waiting on their approval — a FIX.
The fix sits at cause level for what the frontend can reach (the predicate was store-private, so renderers re-derived it wrong), and the deferred backend field is a deletion this lane would have demanded anyway: [FIRST-PRINCIPLES-REVIEWED] d435335 |
UX Review (Fable 5) — ✅ PASSUX-level review of UX-Verdict: PASS Parked runs now tell the truth — the awaiting state is named, colored distinctly, and the Approve/Reject banner sits right below it. Suggestions
[UX-REVIEWED] d435335 |
Opus 4.8 Review — ✅ no blocking findingsReviewed Review detailsI've traced the candidate to its root. The claimed defect requires a run to simultaneously hold No new groundable findings emerged from tracing the changed lines. No findings. [OPUS-REVIEWED] d435335 Verdict parsed from the review's SHA-scoped output markers for commit False positive or not applicable? A repository writer can comment: |
GPT 5.6 Review — ✅ no blocking findingsGPT 5.6 completed its review of This comment is updated in place on each push. Review detailsNo findings. False positive or not applicable? A repository writer can comment: |
d9fb50a to
0353413
Compare
github-actions
Bot
added
readiness: checking
|
Round 1 triage. Three lanes were red on the first head; two were mine and are Fixed -- Fixed -- Getting there needed a workaround worth flagging for anyone else on this host: Not mine -- All four AI review lanes that had reported (Design Review, First Principles, Opus |
0353413 to
0723d35
Compare
|
Round 2. Fixed -- The gap that let it through is worth naming, since it is the reusable lesson here: Still not mine -- Screenshots in the description are re-pinned to the new head so the evidence tracks |
0723d35 to
18f320d
Compare
A sub-agent parked on an unanswered spawn-approval prompt was reported by the dashboard as running. Two surfaces folded status 'pending' into their running tally: the wave chip above the composer (SubagentProgressBar) put the parked run behind a spinning loader and rendered its row as a bare task label with a ticking elapsed timer, and the inline launch card in the transcript (SubagentRunCard) printed "1 agent running". The run had in fact launched no process at all -- it was registered, counted, and blocked on a prompt the user had not answered. The state was already in the store. sseSubagentPending writes status 'pending' plus an approval_id from the WS approval frame, and the Subagents side panel already renders it as "Awaiting approval" with Approve/Reject. The defect was that the two tally surfaces never asked the question, and the predicate that answers it (status 'pending' AND an approval_id) was a private const in chatSlice with three call sites, none of them a renderer. - Export isAwaitingSpawnApproval from chatSlice so the renderers share the one definition instead of re-deriving it from status alone. - SubagentProgressBar: parked runs leave the running count and get their own count with a distinct glyph; the per-agent row names the approval. The mount predicate gains the awaiting term, because a wave whose only member is parked now has running === 0 and would otherwise unmount the one surface naming what it is blocked on. The row's approval branch is checked before retrying/stalled: a run that never executed produces the watchdog's silence trivially, and the approval is the more specific explanation. - SubagentRunCard: same split in tally(), plus an awaiting chip and a leading glyph ranked above failed -- a failure is history, an unanswered approval is still actionable. - One new catalog key per namespace, translated across all 12 authored locales, en-XA regenerated. Tests: 6 new assertions in SubagentProgressBar.parkedApproval.test.tsx and 4 added to SubagentRunCard.test.tsx, including negative controls that a 'pending' entry with no approval_id keeps its previous treatment. Both suites were verified red against the base tree. Visual evidence: website/scripts/capture-parked-subagent-chip.mjs drives the real built SPA, pushes the same WS frames the gateway sends (subagent_spawn plus an `approval` whose id is `spawn:<agent_id>`), and ASSERTS the rendered strings before it photographs, so a stale bundle reds instead of quietly capturing the old copy. Before/after frames under temp-screenshots/parked-subagent-approval/; the before run reproduces the defect (2 running claimed, no awaiting count). The hi translation uses the informal second-person possessive, not the formal honorific pronoun: style/hi.md section 4 requires the informal register, and src/i18n/style/hiStyle.test.ts is a ceiling that fails on growth, so two formal values would have raised it from 118 to 120. Closes #7318
18f320d to
d435335
Compare
github-actions
Bot
added
readiness: passed
bolichen97
left a comment
bolichen97
left a comment
There was a problem hiding this comment.
CI green, no blocking bot findings, diff matches description. Approved.
github-actions
Bot
removed
the
readiness: passed
A default install has no YOLO override, no auto_approve_subagent_spawn and no session trust, so every spawn_run is gated behind the interactive spawn approval. While that prompt is unanswered the run is registered in _agents and counted by the manager's running count, so every reader that goes through /api/spawn reports it exactly like an agent that is executing: no child ACP process, subagents_spawned still 0, and nothing in the payload, the CLI spawn list, the MCP roster or the log naming the gate. An unowned spawn (the CLI posts no parent_session) raises its prompt with slot="", so it is surfaced only on the global approvals feed and appears in no chat tab either. Two adjacent halves of #6484 have already landed and are not redone here. #7325 stopped the reap of such a run blaming a deadline it never reached, and in doing so put info._awaiting_approval on the spawn gate. #7477 stopped a chat tab rendering an owned parked run as executing, deriving its cue from the WS approval event (status 'pending' + approval_id), not from this payload -- so it is scoped to a slot, and the unowned spawn still reaches no tab. What is left is the wait's NAME, on every path that reads a run: * _spawn_with_approval logs at INFO under the run id, with the parent (or "<unowned>"). #7325 marked the wait in machine state for the reaper; a mark is not a message, and nothing was written at all -- which is exactly how #6484 was reported, the reporter's only lead being that no log record mentioned the affected run id. * BOTH /api/spawn read paths carry awaiting_approval while parked, through ONE shared predicate _awaiting_spawn_approval(), present only then so the default payload is unchanged. The list endpoint feeds `kirocrew spawn list`; the single-run status endpoint is what a BLOCKING `kirocrew spawn run` polls every 2s, so reporting it on the list alone would have left the CLI reproduction exactly as silent as before. * That predicate requires _exec_started is None as well as the flag, because the flag is SHARED: run.py sets it at three in-run tool-approval sites, so a bare read would render a run at turn 5 waiting on a tool prompt as "waiting for spawn approval" and tell a still-polling caller to approve it "to start this run" that already started. _exec_started is stamped once when execution begins (_run_inner_impl), so None means the run never entered execution. terminal.py picks the reap message off the same pair, arrived at independently; the predicate is not extracted onto SubagentInfo because this read must survive the info doubles the handlers are tested with, and unifying would mean editing a reap path this change does not touch. One predicate rather than two inlined conditions: the handlers build their payloads independently, and a drift between them is invisible to a behavioural test, so a source ratchet pins both call sites. * MCP `spawn_list` reports [awaiting-approval] rather than [running] -- the surface an LLM reads, and the one spawn.py itself points a failing caller at ("Check spawn_list"). * `kirocrew spawn list` renders the wait instead of the bare hourglass it shared with a running agent, and the blocking poll announces it once rather than on every poll. * Prunes src/kiro_crew/mcp_tools/spawn.py from .github/black-baseline.txt: the file was listed as known-unformatted and this change makes it black-clean, and that baseline is shrink-only, so the gate requires the graduated entry be removed. Fixes #6484
…al (#7299) A default install has no YOLO override, no auto_approve_subagent_spawn and no session trust, so every spawn_run is gated behind the interactive spawn approval. While that prompt is unanswered the run is registered in _agents and counted by the manager's running count, so every reader that goes through /api/spawn reports it exactly like an agent that is executing: no child ACP process, subagents_spawned still 0, and nothing in the payload, the CLI spawn list, the MCP roster or the log naming the gate. An unowned spawn (the CLI posts no parent_session) raises its prompt with slot="", so it is surfaced only on the global approvals feed and appears in no chat tab either. Two adjacent halves of #6484 have already landed and are not redone here. #7325 stopped the reap of such a run blaming a deadline it never reached, and in doing so put info._awaiting_approval on the spawn gate. #7477 stopped a chat tab rendering an owned parked run as executing, deriving its cue from the WS approval event (status 'pending' + approval_id), not from this payload -- so it is scoped to a slot, and the unowned spawn still reaches no tab. What is left is the wait's NAME, on every path that reads a run: * _spawn_with_approval logs at INFO under the run id, with the parent (or "<unowned>"). #7325 marked the wait in machine state for the reaper; a mark is not a message, and nothing was written at all -- which is exactly how #6484 was reported, the reporter's only lead being that no log record mentioned the affected run id. * BOTH /api/spawn read paths carry awaiting_approval while parked, through ONE shared predicate _awaiting_spawn_approval(), present only then so the default payload is unchanged. The list endpoint feeds `kirocrew spawn list`; the single-run status endpoint is what a BLOCKING `kirocrew spawn run` polls every 2s, so reporting it on the list alone would have left the CLI reproduction exactly as silent as before. * That predicate requires _exec_started is None as well as the flag, because the flag is SHARED: run.py sets it at three in-run tool-approval sites, so a bare read would render a run at turn 5 waiting on a tool prompt as "waiting for spawn approval" and tell a still-polling caller to approve it "to start this run" that already started. _exec_started is stamped once when execution begins (_run_inner_impl), so None means the run never entered execution. terminal.py picks the reap message off the same pair, arrived at independently; the predicate is not extracted onto SubagentInfo because this read must survive the info doubles the handlers are tested with, and unifying would mean editing a reap path this change does not touch. One predicate rather than two inlined conditions: the handlers build their payloads independently, and a drift between them is invisible to a behavioural test, so a source ratchet pins both call sites. * MCP `spawn_list` reports [awaiting-approval] rather than [running] -- the surface an LLM reads, and the one spawn.py itself points a failing caller at ("Check spawn_list"). * `kirocrew spawn list` renders the wait instead of the bare hourglass it shared with a running agent, and the blocking poll announces it once rather than on every poll. * Prunes src/kiro_crew/mcp_tools/spawn.py from .github/black-baseline.txt: the file was listed as known-unformatted and this change makes it black-clean, and that baseline is shrink-only, so the gate requires the graduated entry be removed. Fixes #6484 Co-authored-by: gh-autofix#2887 <chenmingwei23@users.noreply.github.com>
2 participants
1. What is the problem?
A sub-agent parked on an unanswered spawn-approval prompt was rendered by the
dashboard as running.
Two surfaces folded status
'pending'into their running tally:SubagentProgressBar(the wave chip above the composer) counted the parkedrun behind the spinning
Loader2, and its per-agent row rendered a bare tasklabel with a ticking elapsed timer -- pixel-identical to an agent that had
launched a process and was working.
SubagentRunCard(the inline launch card in the transcript) printed1 agent runningfor a wave that had started nothing.The run really had launched nothing: it is registered in
_agents, counted by themanager, and blocked on a prompt the user has not answered.
turns == 0,_pid is None,_exec_started is None.One correction to the issue's stated mechanism. The issue attributes this to
the dashboard not consuming the backend
awaiting_approvalfield from #7299, andsays the chip reads it off
/api/spawn. Neither is how the chip gets its state:the chip is fed by WS frames and polls
/api/spawnonly to reconcile phantomrows. The dashboard already had the equivalent state, and had it before #7299:
sseSubagentPendingwritesstatus: 'pending'plus anapproval_idfrom the WSapprovalframe whose id isspawn:<agent_id>, and the Subagents side panelalready renders that as "Awaiting approval" with inline Approve/Reject.
So the defect is not a missing wire field. It is that the two tally surfaces
never asked the question, and the predicate that answers it --
status === 'pending' && !!approval_id-- was a module-private const inchatSlicewiththree call sites, none of them a renderer.
2. Why this issue matters to the user
The running count is the one number these two surfaces exist to publish, and for
a parked run it asserted the opposite of the truth: the user was told work was in
progress while the wave was in fact waiting on them. There is no timeout on the
far side, so the run sits there until it is answered.
The launch card makes it worse than a transient glitch. The chip disappears when
the wave ends and only ever covers the slot you are viewing; the card stays
anchored in scrollback. So the false "running" claim is the one that persists.
3. How our fix solves it
Chain from the symptom back to the cause:
The fix cuts at the last link, then repairs the two tallies:
chatSlice.ts-- exportisAwaitingSpawnApprovalso the renderers share theONE definition.
approval_idis the load-bearing half:sseSubagentPendingisthe only writer of
'pending'and always sets it, so its absence means a cardbuilt some other way and must not be claimed as blocked on the user.
SubagentProgressBar.tsx-- parked runs leave the spinningrunningcountand are reported under their own count with a distinct glyph; the per-agent row
names the approval instead of rendering blank. Two details that are not
cosmetic:
awaitingterm. Excluding parked runs fromrunningmeans a wave whose only member is parked now hasrunning === 0,and the old
running > 0 || queued > 0would have unmounted the one surfacenaming what the wave is blocked on. There is a test for exactly this.
retrying/stalled. A run thatnever executed produces the watchdog's silence trivially, so a stall badge on
it is describing an absence this row can already explain exactly; the
approval is strictly more specific, and it is the state the user can act on.
SubagentRunCard.tsx-- the same split intally(), plus an awaiting chipand a leading glyph ranked above
failed: a failure is history, an unansweredapproval is still actionable. The chip needs no
settled === 0guard (unlikethe queued chip, which is slot-keyed and can report another wave's queue) --
awaitingis derived from this launch's own ids.locales,
en-XAregenerated.Deliberately NOT in this PR: the backend hop. #7318's comments ask for
awaiting_approvalon the subagent WS payload indashboard/ws.py, gated on_awaiting_approval is True AND _exec_started is None. That is unbuildable onmaintoday and would be dead code if merged:mainnever sets_awaiting_approvalon the spawn path at all (its three writers inrun.pyareall in-run TOOL approvals), so the gate is always false for a spawn-parked run.
The writer, and the
_awaiting_spawn_approvalhelper the comment says to reuse,both live in PR #7299, which is still open. Landing that hop here would
duplicate an open PR's diff to ship an always-false field.
The same dependency bounds the other half of the issue: an unowned spawn (from
the CLI,
slot="") gets no pending card at all --useWebSocketrequiresdata.slotbefore dispatching, deliberately, so an approval cannot bemisattributed to whatever chat the user happens to be viewing. Giving that run a
dashboard representation, and the click-through the issue asks for, needs #7299's
HTTP field. It is a clean follow-up once #7299 merges.
4. What tests we did
website/src/test/SubagentProgressBar.parkedApproval.test.tsx, 6assertions -- parked run excluded from
runningand reported asawaiting; therow names the approval; the chip stays mounted when the parked run is the whole
wave; no awaiting count when nothing is parked; the approval wins over a stall
verdict on the same run; a
'pending'entry with noapproval_idkeeps itsplace in the running count.
SubagentRunCard.test.tsx+4 -- a wholly parked wave does notclaim to be running, a mixed wave counts only the members that started, no chip
when nothing is parked, and the same no-
approval_idnegative control.SubagentProgressBar.tsxalonereds 4 of the 6 new assertions; reverting
SubagentRunCard.tsxalone reds 2 ofthe 4. The cases that stay green in both are the negative controls, which is
what they are for.
(
SubagentProgressBar.*x4,SubagentProgress/Scale/Resilience.reducers,ChatSidebar.subagentApproval,ChatSidebar.subagentRunning,selectSlotSubagents,subagentActivity).SubagentRunCard.test.tsxis 28/28including its 24 pre-existing cases.
npm run i18n:checkexits 0, all 19 checks PASS, with[source-strings] 2 new English key(s) / 0 badly shapedand[pseudolocale] en-XA matches en. The 11 catalog data suites pass, 233 tests.no-consolewarning that is already on base (verified by linting the stashed base file --
line 157 before, 165 after); zero new warnings.
tsc -bproduces abyte-identical error set before and after my diff (16 lines, all downstream of
one missing devDependency in this sandbox -- see below).
Screenshots
Captured from the REAL built SPA by
website/scripts/capture-parked-subagent-chip.mjs(committed here), which pushes the same WS frames the gateway sends -- a
subagent_spawnfor the member that started, and anapprovalwhose id isspawn:<agent_id>for the one parked on the user. The scenario is a two-memberwave: one executing, one parked.
The harness ASSERTS the rendered strings before it photographs, so a stale bundle
reds rather than quietly capturing the old copy, and
--expect-beforeinverts theverdict so the BEFORE frame has to actually REPRODUCE the defect (a blank page
would otherwise pass as convincing evidence).
The wave chip above the composer. Before, the parked run is a bare label with a
ticking timer, indistinguishable from the row below it that is doing work, and the
spinner claims 2:
After -- 1 running, 1 awaiting, and the parked row says what it is waiting for:
The launch card in scrollback, which is the one that persists after the chip
drops. Before, then after:
Whole surface after the fix, showing every surface finally agreeing -- the sidebar
row ("1 sub-agent needs approval"), the launch card, the wave chip, and the
composer's pre-existing approval banner:
Harness output on the two trees:
5. Any other suggestions on the work
upstream now.
Frontend Lint & Type Checkrunseslint src/ --max-warnings 659and main had drifted to 660, so it failed on every PR whose changed surfacereached that lane.
Backend Testsfailed on two main-owned tests in turn: thedashboard/handlers/files.pylog-site census, andtest_irq.py::test_an_entry_joining_after_a_partial_fire_serves_its_own_floor.None of them are reachable from this diff, which contains zero Python files. The
branch is now rebased onto main at
c412c2ff9, which carries fix(ci): re-measure the files.py log-site census after #7293 #7492, fix(ci): repair two cross-merge breakages reding main's own tip #7508 andfix(ci): bring two drifted ratchet gates back in line with the code #7512; measured on that tree,
eslint src/reports 653 warnings, 0 errors(six under the ceiling),
test_irq.pyis 60/60, and the census test passes.Attribution for the one warning this PR's files do carry: it is the pre-existing
no-consoleinSubagentProgressBar.tsx, already on base at line 157 and movedto 165 by my diff -- CI's own per-file warning list named that file for that
warning only, and none of my other five files appeared at all.
mainadded@radix-ui/react-tabstowebsite/package.json; this host's registry rejectsauth (
E401, and it is not in the npm cache), sotsc -bandvite buildbothfail on that one unresolved import. I confirmed it is entirely upstream of this
diff --
tsc -bproduces a byte-identical 16-line error set before and after mychange, every line downstream of
src/components/ui/tabs.tsx-- and produced thescreenshots against a local, uncommitted shim for that package alone. CI installs
it properly and its
tsc -bstep passed.ChatInput.tsx(~line 2787) hardcodes'1 sub-agent is awaiting your approval to run'/`${n} sub-agents are awaiting...`-- an untranslated Englishliteral with the plural chosen in JS, which is precisely the
[plurals-hardcoded]classwebsite/docs/i18n-catalog.mddocuments. It isvisible in the whole-surface screenshot above. Fixing it needs a plural key
across 12 catalogs and would ratchet the ceiling; it is unrelated to the tally
bug and belongs in its own PR.
run should leave the backend's
/api/spawncountis a semantics change withexternal consumers (
kirocrew spawn list). Nothing here changes that field --the split is presentational and dashboard-local.
Pattern harvest
Rule candidate: a status predicate with more than one consumer must not be private
to its module.
This defect class is "N surfaces answer the same question, and the ones that
re-derive it get it wrong". The predicate existed, was correct, and was three lines
above two counters that duplicated its subject badly --
isAwaitingSpawnApprovalwas
const, so both renderers reached forstatus === 'pending'instead. #7318'sown comment counts five separate serializers of per-run subagent state for the
same reason.
Mechanically catchable, cheapest first:
no-restricted-syntaxrule banning a barestatus === 'pending'comparison on a
SubagentActivityoutsidestore/chatSlice.ts, the way thei18n gate bans
toLocaleDateString()with no locale. Naming the exportedpredicate is the opt-out, so there is no allowlist to maintain.
SubagentActivitystatus predicate isexported, i.e. that no
const is<X> = (a: SubagentActivity)exists unexportedin
chatSlice.ts. That is a convention detector rather than a per-site pin, soit also covers the next predicate someone adds.
Not a one-off:
countActiveSubagents,selectSlotSubagentsActive,tally()andthe chip's
activeListare four independent spellings of "is this run active",and this PR only reconciled the two that render a count.
Closes #7318