feat: show bounded monitors in the dashboard - #5185
Conversation
github-actions
Bot
added
the
readiness: checking
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: |
Design Review (Fable 5) — 🟡 CONCERNSDesign-level review of I have a complete picture of the design: backend adds typed classification fields and a fail-closed public projection; the frontend replaces the goal-loop map with a discriminated Design-Verdict: CONCERNS Sound consolidation onto one normalizer and contract, but correctness now rests on a hand-rolled client-side cache-coherence protocol that will be expensive to extend safely. Watch
[DESIGN-REVIEWED] 38f9ead |
UX Review (Fable 5) — 🟡 CONCERNSUX-level review of I have everything I need. Reconciling: the diff adds a full new user-visible surface — the composer's radar trigger button, the "Bounded pull request monitor" popover (URL field, five budget fields, wake instructions, Start/Save/Stop/Confirm/Restart controls), the legacy-form switch with warning and back link, the snapshot-failure Retry state, the session-mode notice, and two new sidebar status rows ("Monitor · {status}"). This revision commits zero screenshots, so the blind read never ran — none of these controls has been seen by any cold reader. The only image is in the PR body, pinned to a different commit and admittedly showing a later PR's helper text. Nothing meets the non-tiebreaker BLOCK predicates (no blind-read misread exists to quote; the bounded↔legacy switch is explicit navigation with a labeled back link, not an unexplained hard swap; no hedging or action-less notices). Verdict below. UX-Verdict: CONCERNS A large, jargon-dense monitor surface ships with zero committed screenshots — the blind read never ran, so no cold reader has validated any of it. Watch
Evidence gaps
Suggestions
[UX-REVIEWED] 38f9ead |
First Principles Review (Fable 5) — 🟡 CONCERNSPremise-level review of All lenses run and counts verified. Final review: First-Principles-Verdict: CONCERNS Spec added here says the dashboard normalizer validates GitHub observation facts; the shipped normalizer never reads them — and two new legacy-loop fields have zero consumers. Not justified as shipped
What this change shipsIntent: let an operator create, inspect, stop, and explicitly restart bounded PR monitors from the chat dashboard — an ADDITION (dashboard layer of the in-repo token-efficient-monitors RFC).
Watch
Subtractions
[FIRST-PRINCIPLES-REVIEWED] 38f9ead |
Opus 4.8 Review — ✅ no blocking findingsReviewed Verdict parsed from the review's SHA-scoped output markers for commit False positive or not applicable? A repository writer can comment: |
github-actions
Bot
added
readiness: action required
81b2e1e to
b5198cc
Compare
github-actions
Bot
added
readiness: checking
b5198cc to
399d373
Compare
399d373 to
b70178b
Compare
b70178b to
b5a8f5f
Compare
github-actions
Bot
added
readiness: action required
b5a8f5f to
3382b7a
Compare
|
Addressed the current-head data-loss race in the owning backend commit. The RFC-required legacy fallback remains available, but browser POST creation is now create-only at the durable service lock: a monitor or legacy loop armed after the client snapshot wins and the stale request returns 409 without replacing its record. Added red-to-green coverage at the route, authorization, and persistence boundaries; the owning 195-test slice and static/docs gates pass. The stack was restacked bottom-up, and CI was not manually rerun. |
|
Addressed the actionable 320px finding: the explicitly labeled legacy fallback now has a viewport-bounded, vertically scrollable popover and stacks its numeric controls at the narrowest supported width. The structured workflow and zero-means-unlimited legacy semantics are unchanged. Focused verification: 50 popover tests and TypeScript compilation passed. |
|
/ai-review override gpt 749938f: The service’s exact-then-dashboard-fold find_by_slot lookup resolves both slack: and slack, so the cold REST snapshot does not miss the channel monitor. |
Human judgment recorded@kyleseaman marked the gpt AI finding as false positive, not applicable, or explicitly accepted for
This decision applies only to this commit. A new push requires a new judgment. |
|
Addressed the current review finding in a7ecdf2: delayed legacy mutation responses now use the same captured automation-identity guard as structured mutations, so they cannot replace a newer WebSocket monitor. Focused verification: SessionAutomationPopover 36/36 tests and TypeScript build passed. |
|
Addressed the current-head review blocker by disabling the monitor draft fields while a save is pending, so accepted edits cannot be discarded when the successful mutation closes the popover. Added a focused regression; the popover suite passes (39 tests on the stack head), along with ESLint and TypeScript checks. |
|
Addressed the current-head review blocker by capturing the originating slot in every monitor mutation and invalidating that slot on completion. Switching from slot A to B during an in-flight save can no longer leave A stale by invalidating B. Added a focused red/green regression; the dashboard slice passes 27 tests, and the restacked top passes 40 tests plus ESLint and TypeScript checks. |
|
Fresh CI exposed a 6 KB overage in the all-locale chunk after this PR’s translated monitor labels. The later source-provider PR had been carrying the combined catalog allowance, so I moved the structured-monitor portion into this owning PR, measured the emitted chunk at 9,990,151 bytes, and kept approximately 5% headroom. The analyze build and per-chunk gate now pass locally; #5186 and #5305 were restacked above it with explicit leases. |
|
Fixed the current Frontend Tests (2) failure by removing the stale |
|
Addressed the reconnect-seed race in 6b05616. The seed records its start time and only tombstones per-slot React Query entries older than that seed, preserving mutation or focused REST writes that land while the list request is in flight. The focused WebSocket seed suite passes (11 tests), with TypeScript clean and focused ESLint at 0 errors. |
bolichen97
left a comment
bolichen97
left a comment
There was a problem hiding this comment.
Review: bounded monitors in the dashboard
Reviewed origin/token-monitors-tools...origin/token-monitors-dashboard (54 files, +4098/−286) at b7beaf9a2. Findings were verified by executing the real normalizeAutomationRecord/deriveAutomationStatus/dashboardAutomationSlotKey (bundled with esbuild) against ~45 constructed wire shapes, by tracing the contract end-to-end into the Python backend, and by running a real vite build --mode analyze. The changed frontend suite passes (373 tests / 9 files), tsc -b is clean, eslint adds no new warnings.
Two negatives worth stating up front, so nobody re-spends on them: i18n coverage is genuinely complete — 48 new keys, identical key set in all 13 translated catalogs, en.json correctly untouched (it is generated), all 8 MONITOR_STATUS_KEYS present and really translated everywhere, 0 of 447 code-referenced keys missing from merged EN/de/ja, and lint:i18n + a correctly-scoped i18n:check both pass 19/19. And terminal records really are read-only: the whole <fieldset> unmounts, no Save button renders, and there is no <form>, so Enter cannot submit — a generic Save can never revive them. The gap is the inverse.
Blocking
1. website/src/monitoring/automation.ts:207 + SessionAutomationPopover.tsx:559 — a monitor the client cannot validate loses its Stop control entirely, while the backend keeps probing and spending tokens. structuredFallback hardcodes active:false, actionable:false and a synthetic terminal: {outcome:'blocked', reason:'invalid_monitor_record'} — discarding loop.active, which it has. A non-null terminal then unmounts the whole form (line 411) and takes the terminal button branch (558), which renders only <SendBtn disabled={busy || !monitor.actionable}>Restart</SendBtn> — permanently disabled. No Stop, no edit, no dismiss. api.monitorStop is the SPA's only stop route, and the server's POST /api/monitors/{id}/stop has no version gate and would accept the call; the client simply never offers it. Executed: a live monitor with active:true, outcome:null and one unrecognized enum value (e.g. last_decision:'wake_deferred') yields active=false actionable=false terminal={"outcome":"blocked"} status=blocked. I confirmed all six frontend enum allowlists match monitoring/models.py exactly today, so the trigger is the first backend enum addition — see finding 15. The sidebar compounds it: automationRunningKeys filters on record.active, so the row also stops reporting background work.
2. website/src/monitoring/automation.ts:316 — wake_instructions length is validated post-redaction on the client but pre-redaction on the server, and redaction lengthens the string. autonudge_authz.py:369 checks len(...) > MAX_MONITOR_WAKE_INSTRUCTIONS_CHARS, then lines 374-375 redact, and :592 persists the expanded value; the PATCH path (:65-69) redacts with no length re-check at all. redact_exfiltration_urls substitutes [REDACTED: suspicious URL to {domain}] — 30+len(domain) chars — for the matched URL, and _redact_monitor_value (handlers/autonudge.py:110) redacts again on every egress. So 990 chars containing https://bit.ly/x (16 → 34) persists at 1018 and fails supported on every client forever. This is the concrete, no-schema-change-needed trigger for finding 1. Same class applies to target: a redacted PR URL still passes !!text(monitor.target) and is displayed and prefilled into the edit form as [REDACTED: suspicious URL to …].
3. website/src/monitoring/automation.ts:300 — lifecycleValid rejects active=false, outcome=null, a state the backend legitimately produces, and both sides then refuse to clear it. AutoNudgeService._deactivate_unwired_monitor (autonudge.py:2463) is the one deactivation site of seven that sets staged.active = False without an outcome. Executed on that shape: actionable=false, terminal={"outcome":"blocked","reason":"invalid_monitor_record"}, status=blocked, so Restart is disabled; and POST /api/monitors/{id}/restart independently rejects it (if loop.monitor.outcome is None: return _monitor_error("only terminal monitors can restart"), handlers/autonudge.py:492). The record can never be revived or cleared from the dashboard.
4. website/src/components/SessionAutomationPopover.tsx:92 — legacyWire() hardcodes next_due_ts: 0, killing the legacy goal-loop countdown for every legacy loop. LegacyGoalLoop never models the field (the normalizer reads last_fire_ts, not next_due_ts), so the bridge has nothing to pass. AutoNudgePopover drives its live countdown off exactly that field in three places — ticking (:229), the next_cycle_unscheduled string (:248), and titleCountdown (:265) — and this is now the only production render of that component. Before this PR, ChatPage fetched /api/autonudge/slot/{slot} and passed d.loop, which carries the field. So an armed loop due in 4 minutes reads "next cycle unscheduled" forever and the chip tooltip loses its countdown. The field's own doc comment says it was added by #6482 precisely because it "was not surfaced here before"; this reverts that. AutoNudgePopover.test.tsx passes a real next_due_ts directly, so unit tests stay green — the break exists only in the new bridge. max_runtime_secs and stoppedReason are dropped the same way, so the popover also cannot explain a bound-expired loop.
5. website/src/pages/ChatPage.tsx:1380 — one failed snapshot permanently disables both "Start monitor" and the legacy fallback, with no error surfaced anywhere. The useQuery at 1347 sets no staleTime, so it inherits the app-wide staleTime: Infinity (which the file's own comment says defeats refetchOnWindowFocus), and its queryFn throws on an unrecognized or conflicting payload. automationCreationReady = !!automation || (isSuccess && !isFetching) therefore stays false forever, disabling <Btn disabled={!creationReady}>Use legacy (costly)</Btn> (555) and <SendBtn …>Start monitor</SendBtn> (556). Concretely reachable: /api/monitors/slot/{slot} is owner-gated (_require_monitor_owner → is_owner_dashboard_request, false whenever request["user"] is empty or ≠ owner_id) while /api/autonudge/slot/{slot} is not, so a single 403 on the structured read also kills the legacy path — which worked before this PR, since the old code gated Start on no read at all. errors.request is only written from mutation.onError, so the popover shows a full form with two dead buttons and no message.
6. website/src/pages/ChatSidebar.tsx:1712 — the row's running status line is gated on monitor presence, not activity. when: isRunning && (!monitor || s.running) discards the workflow-fan-out half of the widened isRunning. Because this PR deliberately retains terminal monitors forever (automations was removed from slotKeyedMaps, and chat_handlers.py:3443 states "Legacy loops are removed. Structured monitors instead retain their durable outcome"), the suppression is permanent for any slot that ever ran one: a session whose monitor succeeded weeks ago starts a workflow fan-out and shows a stale "Monitor · succeeded" instead of the live status. The predicate automationRunningKeys already uses (active && !terminal) is the intended one. In the opposite direction, a quarantined-but-armed monitor is active:false, so it is excluded from busySet, isRunning and inferLane's backgroundWork while still probing server-side.
7. website/src/pages/ChatSidebar.tsx:1731 — the new structured_monitor_passive branch never empties, so a session that ran a monitor permanently loses its last-message preview. when excludes only action_running and isUnread, and line 2129 is {rowState ?? ((s.last_message || unreadDot) ? … : null)}, so a non-null rowState suppresses both the preview and the unread-dot fallback. website/AUTOSDE.yaml session-row-fixed-height (blocking: true, file-patterns includes this file) states the chain as "pending approval > sub-agent approval > goal loop > workflow > sub-agents > running > last message" and justifies it: "The rows that exist are earned: they are mutually exclusive turn state, or they disappear when empty." A terminal monitor is neither. The pre-existing goal_loop branch has the same shape but does disappear, because sseAutomation deletes an inactive legacy loop. The documented last message tail becomes unreachable for every session that ever ran a monitor, across reloads (re-seeded from GET /api/monitors by design), with no way to dismiss it.
8. website/src/monitoring/automation.ts:380 — backing_off tests wakeDelivery === 'busy' without requiring wakeInFlight, unlike the action_running guard one line above, and the backend never clears wake_delivery on completion. record_monitor_turn_completion (autonudge.py:1956) sets wake_in_flight=False and completion_evidence_deadline=0 but leaves wake_delivery untouched; only a new actionable wake resets it. So wake_in_flight=false, wake_delivery='busy', active=true, terminal=null is reachable — a path the backend explicitly contemplates at :1955 — and deriveAutomationStatus returns backing_off through every subsequent healthy no_change probe. Both the popover badge and the sidebar row report a retry-throttled monitor that is probing normally.
Should fix
9. website/src/store/chatSlice.ts:299 — automations was removed from slotKeyedMaps and nothing replaces the eviction path, so the collection is unbounded. slotKeyedMaps feeds both slotKeysWithResidue and evictSlotState, so deleteSlot no longer touches automations and reconcileSlotResidue never even visits a slot whose only residue is one. The docstring's claimed replacement ("only its authoritative seed/tombstone lifecycle removes it") does not exist for session close: remove_by_slot (autonudge.py:1503) routes a monitor to retire_monitor_for_session_close, which emits "updated" — not "removed" — and leaves the loop in _loops; removeAutomation's only production dispatcher is the event === 'removed' branch (useWebSocket.ts:1850), so it never fires. GET /api/monitors is svc.list_all() filtered on lp.monitor with no cap, no terminal filter and no retention window, so every reconnect re-adds the deleted session's record. Slot keys embed a timestamp and never recycle. And chatSlice.slotPrune.test.ts:207 made this uncatchable: 'goalLoops' was dropped from perSlotMaps and replaced by an assertion that the record survives, so no test covers automation eviction anywhere. (That fixture also sets latest.summary, a property StructuredMonitor['latest'] does not declare — tsconfig.app.json excludes src/test, so it never fails tsc.) automationLiveGenRef has the same shape: one Map entry per slot ever seen, never pruned.
10. website/src/pages/ChatSidebar.tsx:2716 — the memoized createSelector was replaced by an unmemoized inline selector, so it now runs on every dispatched action. useAppSelector(s => Object.values(s.chat.automations ?? {}).filter(...).map(...), shallowEqual) re-runs its body on every store notification; shallowEqual suppresses the re-render, not the work. website/src/lib/streamHold.ts documents the rate ("nearly all main-thread churn is the per-rAF flush pipelines in useWebSocket"), so this allocates three arrays ~60×/sec per tab over a collection that never shrinks (finding 9). The deleted selectGoalLoopKeys was a createSelector keyed on state.chat.goalLoops, costing one === check. Second hazard: Object.values returns insertion order and setAutomations rebuilds via spread→delete→re-insert, so a reconnect seed with 2+ automations yields the same set in a different array order, shallowEqual returns false, and the whole sidebar shell re-renders (recomputing runningSet, recentSet, busySet, isStaleExempt, every column filter) though membership is unchanged. Every sibling in the same block kept its createSelector, and the surrounding comment still claims the shell "re-renders one row, never the whole sidebar."
11. website/src/hooks/useWebSocket.ts:1842 — the per-slot invalidation fires on every autonudge_state frame, not just the removed frame its own comment scopes it to. invalidateQueries defaults to refetchType: 'active' and ChatPage keeps that key observed, so each frame triggers a refetch whose queryFn does Promise.all([api.autonudgeForSlot, api.monitorForSlot]) = 2 GETs, each running _require_monitor_owner → _audit_monitor_access → sel().log_api_access. The result is provably dead on a non-removed frame: dispatch(sseAutomation(record)) two lines later populates Redux, and ChatPage computes automation = liveAutomation ?? automationSnapshot.data so live always wins. At the 15s minimum cadence that is 8 wasted GETs + 4 audit writes per minute per open tab, plus 2 extra full ChatPage renders — on the surface whose entire premise is token/cost efficiency. setQueryData(['session-automation', slot], record) on the normal path costs nothing and is already the pattern used for the removed tombstone.
12. website/src/pages/ChatPage.tsx:6752 — the whole automation object is a dependency of the transcript-wide renderMessage memo, but only automation?.id is read (line 6630, nudgeMatchesLoop). renderMessage feeds renderTurnItem, which renders every turn, and the comment at 6581 states the invariant explicitly. Both sources of automation mint a fresh object per frame (Redux stores a freshly-normalized record; the invalidate above resolves a refetch that mints another), so one frame invalidates the renderer up to twice, each time re-rendering every visible row on a 2,000-row transcript — to recompute one boolean. Hoist const automationId = automation?.id.
13. website/scripts/check-bundle-size.mjs:49 — the all ceiling was raised 500 KB with the measurement deleted, and the raise is unnecessary. I ran vite build --mode analyze + bundle-report.mjs on this checkout: assets/all-*.js measures 9974.4 KB using the exact chunk.size field checkChunkBudgets compares — 515 KB under the pre-existing 10490 KB ceiling, and 11 KB smaller than the base comment's recorded 9985 KB. node scripts/check-bundle-size.mjs is green without the raise. The file's own contract (unchanged, lines 33-38) says each ceiling "is the size measured by an analyze build, plus roughly 5% headroom … raising one is a bundle-size regression and needs to be justified in the PR that does it"; the replacement note is an unresolved TODO ("re-measure after build"). Slack is now 10.2%, and +500 KB is exactly DEFAULT_BUDGET_BYTES, so a whole default-budget chunk of new code can land in the eager all-catalogs chunk — every non-English user's first load — and ship green. That is precisely the "NEW library or surface landing in the catalog chunk" the header says this gate exists to catch. Meanwhile t measures 728.0 KB against its new 740 KB (1.6% headroom, not ~5%) while its comment still reads // measured 702 KB and gained a sentence claiming it "includes the structured-monitor form's labels" — a measurement never taken, 26 KB stale. The comment block three lines above warns about exactly this ("the previous measured 641 KB note was ~60 KB stale … so any PR adding an English string tripped this gate rather than the new library or surface it exists to catch"); the next PR adding ~12 KB of strings reproduces it.
14. website/src/monitoring/automation.ts:374 — session_close and target_unavailable collapse into blocked. Executed all six MonitorOutcome values. So closing a tab with an armed monitor (retire_monitor_for_session_close, a completely normal teardown) paints a warn-coloured "Monitor · Blocked" in the sidebar and an err badge in the popover, indistinguishable from a genuinely blocked review; a wake that couldn't be delivered attributes a transport failure to the pull request. MONITOR_STATUS_KEYS has no key for either, and the distinguishing reason code is only visible inside the popover's terminal block.
15. website/src/monitoring/automation.ts:1 — 11 backend bounds and 6 backend enum vocabularies are hand-copied with no codegen and no parity gate, and they are used for hard rejection. STRUCTURED_MONITOR_LIMITS restates MIN/MAX_MONITOR_CADENCE_SECS, MAX_MONITOR_RUNTIME_SECS, MAX_MONITOR_AGENT_TURNS, MAX_MONITOR_TOKENS, MAX_MONITOR_PROVIDER_ERRORS, MAX_MONITOR_WAKE_INSTRUCTIONS_CHARS from monitoring/models.py:24-30, and lines 284-297 restate all six enums plus hardcoded version === 1, kind === 'github_pull_request', objective === 'review_ready'. I diffed all six: they match exactly today, which is why nothing in CI will catch the first divergence. MONITOR_STATE_VERSION gates persistence compat, not enum growth, so adding one MonitorDecision member is a purely additive backend change needing no bump — and it converts every live record carrying that decision into the fabricated blocked / invalid_monitor_record dead end of finding 1. Same for raising MAX_MONITOR_AGENT_TURNS, or a second monitor kind. No compile error, no test failure, no signal that a frontend file needed editing. The repo already has the fix pattern twice — scripts/gen-settings-registry.mjs and scripts/check-app-manifest-sync.mjs, whose preamble is the argument here ("Two copies is a drift machine unless something pins them together"). code-style.md, which AGENTS.md routes to for "where a limit lives", states: "Every limit … has ONE owning module … the second copy of it drifts silently."
Notes, lower severity
automation.ts:168 — structuredFallback recomputes slotKey without the envelope.slot fallback the caller used to pass its own !slotKey → null guard, so it can return slotKey:''; executed, it lands in state.automations[''], matches no sidebar row, cannot be removed by any tombstone, and two slots' malformed frames overwrite each other there. · automation.ts:332 — the loop.next_due_ts fallback for nextProbeAt is unreachable (finite() falls back on type, not emptiness, and supported already proved it is a number), so a monitor with a real loop-level deadline renders "Not scheduled"; shadow.py:72 writes next_probe_at directly without _set_monitor_deadline, so the two genuinely can diverge. Same shape for text(monitor.stopped_reason, text(loop.stopped_reason)) at :361. · SessionAutomationPopover.tsx:277 — the error-focus id remap special-cases providerErrors but misses wakeInstructions, whose control id is ${id}-wake, so getElementById returns null and focus() silently no-ops; naming every id ${id}-${field} removes both the hack and the bug. · SessionAutomationPopover.tsx:144 — the open/id effect unconditionally calls setLegacyMode() and setErrors({}) on any automation?.id change, so a monitor created in another tab discards a user's explicit legacy-mode opt-in mid-typing, and any unrelated frame erases the request-failure banner. · SessionAutomationPopover.tsx:138 — automationRef.current = automation is written during render, which React forbids; this ref is the entire identity guard for applying a mutation result (onSuccess compares automationRef.current === request.captured), so an abandoned concurrent render can leave it holding uncommitted state. · SessionAutomationPopover.tsx:327 — the interrupted prop is accepted but forwarded only on the legacy branch, so the documented invariant ("whenever the composer says press Resume, the loop chip must not pulse as if a cycle were executing") is unenforced for every structured monitor, and MonitorRadar pulses for up to the 7260s evidence window while nothing runs. · SessionAutomationPopover.tsx:383 glues an ASCII ": " in JSX for input/output tokens while the five sibling labels are label-first and the catalogs localize that colon (ja トークン：, zh-CN 令牌数：), so Japanese renders two half-width and two full-width colons in one row; [added-lines] scores 0 because punctuation is not an English word. · en.manual.json:1281 — "unknown": "Unknown" is never shown alone (it is substituted into another string's {{count}} slot) and has no en.context.json entry, and the guess is visible in the shipped values: es Desconocido, it Sconosciuto, pt Desconhecido — masculine singular adjectives against a plural noun. Same pattern for none_yet/not_scheduled/awaiting_first_probe. contextSidecar.test.ts's hard-zero gate only covers values ≤3 chars. · useWebSocket.ts:359 is the fourth bespoke "reconcile a REST snapshot against live WS frames" protocol in one file, and the only one that didn't extract its decision logic — reconcileQuestions (:122) is right there, pure and exported precisely so "the race can be tested directly," while this copy inlines ~125 lines in the hook body (untestable) and couples to queryClient.getQueryState(...).dataUpdateCount. Within it: stillFresh(record) and !protectedSlotSet.has(record.slotKey) are the same predicate, so the filter at 452 is X && X; automationSeedGenRef's guard at 423 is unreachable given the in-flight/queued refs; the two legacy/structuredStarted try/catch blocks guard a synchronous throw fetchQuery cannot produce; and the cache is walked three times per seed with 3N redundant JSON.stringify key re-hashes (measurable on a 100-session dashboard). · automationStore.test.ts:15's terminal fixture is silently a quarantined record (it omits config_generation, wake_in_flight and every timestamp), so both tests claiming to cover terminal-structured-monitor retention never exercise a valid one — and this PR already added the correct structuredMonitorLoop fixture that the sibling suites use. · AutoNudgePopover.tsx:188, the legacy half this PR now owns and renders, still issues raw fetch() with hand-built headers, bypassing client.ts's five-helper transport — so it sends no X-Session-Key (the fail-open path that comment block exists to prevent) and no j(), so an expired session surfaces as a raw parse error. The four new api.monitor* methods are correct but also omit sessionKey, so they carry the shared dashboard:ui placeholder rather than the real slot. · structuredFallback is a 48-line copy of the happy-path literal differing in 5 fields; the happy path re-coerces ~15 values supported already proved; and the five numeric budget fields are spelled out four times (~140 lines encoding one 5-row table), which is where the providerErrors → 'errors' id remap came from.
Execution-verified AI-assisted review (Claude Code) across 10 parallel angles plus a gap sweep, run against a local checkout at b7beaf9a2. Nothing was modified. Also checked and clean: slot-key folding is identity for dashboard keys (_normalize_slot_key already folds them), so ChatPage's raw-activeSlot query key and the WS handler's folded key hash identically; Object.entries + delete in setAutomations is snapshot-safe; the stale-seed vs. live-tombstone race matrix holds; and the removed mechanism in the normalizer is dead code (the sole enveloped caller short-circuits removals first) rather than broken. Findings name the input that reproduces them — please push back where one misreads intent.
|
Addressed the dashboard review feedback and restacked this branch in 513bc1a. Session-close tombstones are non-actionable, stale create responses cannot reclaim control, and normalized goal-loop fields survive updates. The full frontend run passes 1,757 files and 27,686 tests; lint, TypeScript, production analysis/build, and bundle checks also pass. |
|
Addressed the current reconnect blocker in 26024f5: structured terminal monitors now remain present for per-slot cache reconciliation while staying out of the active Redux seed. Added a focused reconnect regression test; the 14-test file, ESLint (no errors), and TypeScript check pass. |
Audit note — this PR is one slice of a declared branch stack, not a duplicateA duplicate-detection sweep flagged #5184 / #5185 / #5186 / #5305 as overlapping at FULL coverage. That is a stacking artifact, and it is worth stating plainly so nobody acts on it: each branch physically contains the previous one, so the shared code is inherited and each PR's review diff overstates what it actually authored. Proved mechanically, not by reading code:
The two PRs are also cleanly split by surface, which is why they are complementary rather than redundant. #5184 is backend-only: 26 files under src/kiro_crew/ (monitoring/controller.py, mcp_tools/control.py, validation.py, session_directive.py, dashboard/handlers/autonudge.py, slack/gateway.py, discord/gateway.py, ...) and ZERO files under website/. #5185's own delta ( The 11 non-website files in #5185's own delta are strictly ADDITIVE refinements in service of that frontend, not a competing design: monitoring/models.py gains Neither has landed: 550ea7b, fc82d95, 4c7b130 and 331c07d are all absent from origin/main; main carries monitoring/{init,completion,decision,github_pull_request,models,shadow}.py but NOT controller.py, and website/src/monitoring/ and SessionAutomationPopover.tsx do not exist on main. Ironically the first-pass record itself, the cached PR data, states in prose "Stacked descendant (PR 6). Its branch is #5184's head plus 4c7b130 + 331c07d; the shared code is inherited, which per the brief is a stacked branch, not duplicate" — the FULL/complete_coverage=true coding of that same relation is what the scan surfaced as a duplicate signal.
Proof of stacking: #5186's own delta ( Per-file judgment of the 14 shared paths: every one is a file #5184 creates or extends and #5186 then builds further on. src/kiro_crew/monitoring/controller.py does not exist on origin/main at all (git ls-tree of the monitoring package shows only init.py, completion.py, decision.py, github_pull_request.py, models.py, shadow.py) -- #5184 creates it, #5186 adds 9 lines to it. mcp_tools/control.py: #5184 introduces the monitor_watch schema and #5186 edits the schema #5184 authored. That is a build-on-top dependency, the opposite of two implementations of one behaviour. Deciding question: if #5184 merged, would #5186 still carry real, wanted work? Yes -- the whole babysit-skill migration, the shipped monitoring doc, the unbounded-loop ban, the pre-probe budget stop, evidence_scope, and the Slack terminal notifications. The reverse direction is not a redundancy signal: #5186 "contains" #5184 only because it is branched off it.
The task premise that both are "open against main" is FACTUALLY WRONG for 5305. Its GitHub base ref is On its OWN range (7ce67a7..9a6f511) 5305 is 77 files / +5,642 / -432 — matching GitHub's own 77 / +5605 / -432 for the PR. That range EXTENDS the four files 5184 created (src/kiro_crew/monitoring/controller.py, test/test_monitor_controller.py, test/test_monitor_directive_apply.py, test/test_monitor_mcp.py); it does not reimplement any of them. It also ADDS six modules 5184 has no counterpart for at all: monitoring/pull_request.py (the provider-neutral review-readiness contract), monitoring/targets.py (strict target parsers), monitoring/gitlab_merge_request.py, monitoring/azure_devops_pull_request.py, monitoring/bitbucket_pull_request.py, monitoring/provider_cli.py — plus dashboard/handlers/source_providers.py and a whole frontend surface (SessionAutomationPopover.tsx +626, monitoring/automation.ts +455, MonitorRadar.tsx) across 12 locale catalogs. That is a capability 5184 STRUCTURALLY CANNOT DELIVER: 5184's MonitorController drives only the single GitHub probe already on main (monitoring/github_pull_request.py, landed by the stack's merged #5183 = dc35c19, which is 5184's merge base). GitLab / Azure DevOps / Bitbucket are different addressing schemes and different providers. Conversely 5184 is the substrate 5305 consumes — MonitorController, the monitor_watch/inspect/update/stop MCP tools, the owner-gated REST routes, format_monitor_wake, MonitorDispatchResult. Neither can be closed without destroying real, wanted work: closing 5184 deletes the controller 5305 imports; closing 5305 deletes four provider probes, the neutral readiness contract, and the entire multi-provider dashboard surface. The maintainer agrees in practice. bolichen97 reviewed each slice on its own per-slice range — 5184 as Consequence for reviewLand the stack in order. Reviewing a later slice before its base lands means reviewing the base's code again, and the FULL-overlap signal a keyword or file-overlap sweep produces here means nothing. Separately, #5184 vs #7634 (the zero-token-probe monitor gate) was adjudicated as independent — same monitor area, different behaviour — so neither blocks the other. From a repository-wide duplicate/overlap audit of every pull request open against |
Review — head
|
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. |
|
/ai-review override gpt 6d4630a: The current head preserves the exact quarantined raw monitor payload across rewrites and retains each pending draft and late mutation error under its originating slot, with focused persistence and slot-switch regressions passing on the byte-identical composed tree. |
Human judgment recorded@kyleseaman marked the gpt AI finding as false positive, not applicable, or explicitly accepted for
This decision applies only to this commit. A new push requires a new judgment. |
GPT 5.6 Review —
|
|
Addressed both current-head review blockers in |
2 participants
Problem / Motivation
The dashboard models only legacy goal loops, keeps multiple copies of automation state, and cannot present bounded monitor evidence or terminal outcomes truthfully.
Why it matters
Operators need one consistent view of target, decision, usage, budgets, in-flight work, and stop reason without accidentally reviving terminal state through a generic Save action.
What changed (motivation → approach → change)
Add a discriminated legacy-loop/structured-monitor model and one normalized Redux automation collection for REST and WebSocket data, folding channel session keys to their dashboard slot keys before reconciliation.
Add bounded create, inspect, sparse update, stop, and explicit restart controls.
Disable create, edit, restart, and legacy-loop controls in crew/member session modes while leaving Stop available for an existing monitor.
Lazy-load the optional monitor editor so the dashboard main chunk stays within its existing budget. The translated catalog additions raise the measured
allcatalog ceiling from 10490 to 10585 KiB; the main code chunk ceiling is unchanged.Show authoritative target, typed classification, decision, budgets, probe/wake/turn/token usage, and retained terminal evidence.
Align sidebar precedence with real work and unread state, support reduced motion, and translate the complete surface across all shipped locales.
Treat WebSocket frames as authoritative over mutation responses and delete removed tombstones before validating their stale payloads.
Guard delayed legacy editor responses with the same captured automation identity so they cannot replace a newer structured WebSocket record.
Hydrate the active slot from both per-slot REST projections and keep bounded creation disabled until absence is authoritative; the server-guarded legacy fallback remains available after read failure, so failed WebSocket setup cannot replace an unseen automation.
Retain terminal automation evidence across slot teardown until an authoritative automation seed or tombstone removes it.
Fail closed on malformed persisted canonical enum fields rather than aborting monitor list, WebSocket, or inspect responses.
Read the legacy active-slot snapshot from its canonical
/api/autonudge/slot/{slot}route and keep both per-slot REST results query-local, so stale presence or absence cannot overwrite or delete a newer WebSocket frame.Clear the active-slot legacy REST snapshot after a successful stop so an offline reopen cannot revive a stopped loop.
Stack monitor evidence into one column at the narrowest composer width and expand to two columns only when the popover has room.
Wrap unbroken terminal wake instructions within narrow layouts.
Coalesce overlapping reconnect seeds through stable React Query keys while preserving fresh authoritative reads after settlement.
Tombstone only per-slot snapshots older than the reconnect seed, preserving mutation or focused REST writes that land while the seed is in flight, including null tombstones and non-null mutation results.
Explain failed session-state reads and offer a slot-scoped Retry loading action that preserves the draft and keeps bounded creation guarded until the refreshed read succeeds.
Render rejected monitor mutations and failed snapshot reads through the shared ErrorNotice, with navigation hand-off disabled to preserve the unsaved monitor draft; retain in-editor retries.
Document the persisted classification/reason fields without claiming a persisted probe summary.
Refresh existing per-slot queries from terminal reconnect evidence, including cached absence and predecessor monitor or legacy-loop IDs. Resolve slots from normalized query keys; do not create unvisited queries or overwrite newer REST writes, live frames, or removal tombstones.
Tests
Latest main-restack verification on
995e6b12049ae60990aea5d36c276f06aa2dad95: 259 focused backend tests and 114 focused frontend tests passed, along with TypeScript compilation and diff checks. The conflict resolution retains both the bounded-monitor UI imports andmain's composer-resize instrumentation. Screenshot bytes are unchanged. Fresh CI and reviews are pending; no manual CI rerun was made.Manual verification
Automated component, production-build, pseudolocale, and composed-stack browser checks are complete. The screenshot below illustrates the stacked monitor form, excluding the read-failure recovery state; this PR supplies the monitor form and state model, while later PR #5305 expands the provider choices.
Screenshots / video
The composed stack render below shows the bounded monitor form introduced here. The helper text includes the provider expansion from later PR #5305; this layer owns the form, budgets, action controls, and shared automation state.
Related Issues
N/A — implements the dashboard layer specified in #5180; #5184 is already merged into main.
Checklist
Contribution License Agreement
N/A — the repository template does not yet supply final CLA wording.
Current main-restack verification on
85e0baa2c4e24b322e9fa1c1af8e08a21fe6589a: rebased directly onto currentmainat2d69e083657c00284dd50b7f92c8a9105ee3cbe4. The sole sidebar conflict preserves main’s interrupted-session lane semantics and this stack’s structured automation activity. The composed stack passes all 2,430 changed backend tests and all 101 focused sidebar/popover frontend tests; TypeScript compilation and the production frontend build also pass. Screenshot bytes and the immutable screenshot URL are unchanged. Fresh CI and reviews are running; no manual CI rerun was made.Current main-restack verification on
c5688956f5159c509895e7e1a96a3aad96ef1bfb: rebased directly onto currentmainat6d1b51704295d22fc1f097e90944dd8aa4d9b717; #5186 and #5305 were then restacked on this head. The composed open stack passes all 446 focused controller, provider, AutoNudge, and Slack gateway tests. Screenshot bytes and the immutable screenshot URL are unchanged. Fresh CI and reviews are running; no manual CI rerun was made.Current main-restack verification on
d3dfa8d704b8e48b90bb5d60a49b432115fb2728: rebased directly onto currentmainat834e1ea421e9a6d320cd1183ece2e422a109479c; #5186 and #5305 were restacked bottom-up on this head. The ChatPage conflict preserves main’s registry-driven renderer and this layer’s unified automation state. The composed open tail passes all 373 focused backend tests and all 218 focused frontend tests; TypeScript compilation, focused Black, and flake8 pass. Screenshot bytes and the immutable screenshot URL are unchanged. Fresh CI and reviews are running; no manual CI rerun was made.Current review-fix verification on
0085af85de0c715529148d61fea3eb4c6f87791e: user-triggered close requests are ignored while a monitor mutation is pending, so a late failure remains visible with the submitted draft intact; successful mutations still close directly after scheduling the authoritative refetch. The 38-test automation-popover suite, TypeScript compilation, targeted ESLint, and docs lint pass. #5186 and #5305 were restacked bottom-up on this head. Screenshot bytes and its immutable URL are unchanged. Fresh CI and reviews are running; no manual CI rerun was made.Current main-restack verification on
12eecef59dcb20f1f973969d91aea2df0c4ec1cf: rebased directly onto currentmainat30946933cf3dc7ac60fa93f0a3d8b202148be2e6; #5186 and #5305 were restacked bottom-up on this head. The locale conflict resolution preserves both main’s new catalog entries and this layer’s 57 monitor keys across every shipped locale. The composed top passes 125 focused frontend tests, TypeScript compilation, all 19 i18n gates againstorigin/main, 117 focused monitor/provider tests, and docs lint. Screenshot bytes and its immutable URL are unchanged. Fresh CI and reviews are running; no manual CI rerun was made.Current review-fix verification on 5866cc1: malformed current-version monitor payloads are quarantined inactive while preserving their complete raw state across rewrites; dashboard drafts and late mutation errors remain scoped to the originating slot; and structured/legacy mode changes retain one shared trigger identity. #5186 and #5305 were restacked bottom-up on this head. The composed open tail passes 284 focused backend tests and 160 focused frontend tests, plus TypeScript, targeted ESLint, all 19 i18n gates, and docs lint. The screenshot URL now names the current composed top commit. Fresh CI and reviews are running; no manual CI rerun was made.
Current hygiene-clean verification on 6d4630a: this PR is exactly two commits. The new review fixes were folded into the existing fix commit; #5186 and #5305 were restacked bottom-up, and the composed top tree remains byte-for-byte identical to the verified pre-fold tree. No CI job was manually rerun.
Current non-finite quarantine verification on 5afa6d3: permissive JSON non-finite monitor values are sanitized into an inactive quarantine record without dropping the enclosing loop or its instructions. The regression test fails on the prior head and passes with the fix. The composed open tail passes all 285 focused monitor, MCP, GitHub, and source-provider tests; focused flake8, Linux mypy, Black, docs lint, and diff checks pass. #5186 and #5305 were restacked bottom-up with explicit leases. The screenshot URL now names the current composed top commit. Fresh CI and reviews are running; no manual CI rerun was made.
Current main-restack verification on 47fede7: this two-commit PR is rebased directly onto main at 8aef8fe. The bundle-ceiling conflict retains main’s newly measured 10,975 KiB all-catalog budget and attributes this stack’s monitor/provider strings within that headroom. #5186 and #5305 were restacked bottom-up with explicit leases. The composed top passes TypeScript, the analyze build, the 812-chunk bundle gate, and all 19 base-aware i18n checks. The screenshot URL now names the current composed top. Fresh CI and reviews are running; no manual CI rerun was made.
Current main-restack verification on e677cac: this two-commit PR is rebased directly onto main at 6309382. The locale resolution preserves main’s sidebar-pin copy and this layer’s bounded-monitor strings across every shipped catalog; #5186 and #5305 were restacked bottom-up with explicit leases. The composed top passes 1,013 focused backend tests, 145 focused dashboard/WebSocket tests, TypeScript, all 19 i18n gates, docs lint, focused flake8 and Linux mypy, the analyze build, and the 812-chunk bundle gate. Screenshot bytes are unchanged and the immutable URL names the current composed top. Fresh CI and reviews are running; no manual CI rerun was made.
Current inherited-fix restack note: #5185 remains unchanged at e677cac. Its Linux shard 4 failure is a pre-existing notification snapshot ordering race outside this stack; coverage and readiness failed downstream. #5186 owns and fixes the separate deterministic babysit-guidance failure, and #5305 was restacked on it with explicit leases. Screenshot bytes are unchanged and the immutable URL now names the current composed top. No CI job was manually rerun.
Current main-restack verification on e993142: this two-commit PR is rebased directly onto current main at 018c9ec. The locale resolution preserves both main’s session-control host copy and this layer’s bounded-monitor automation copy across all shipped catalogs. #5186 and #5305 were restacked bottom-up with explicit leases. The composed top passes 314 focused backend tests, the production website build, all 19 base-aware i18n gates, Black, subprocess encoding, brand, and docs lint. Screenshot bytes are unchanged and the immutable URL names the current composed top. Fresh CI and reviews are running; no manual CI rerun was made.
Current main-restack verification on 8c0de58: this two-commit PR is rebased directly onto current main at 3933456. The conflict resolution retains main's AutoNudge query invalidation and manual-fire route while preserving this layer's bounded-monitor editor, persistent trigger, and viewport-safe scroll container. #5186 and #5305 were restacked bottom-up with explicit leases. The composed top passes the 3,628-test selected backend run, all 1,344 focused frontend tests, the production website build, all 19 base-aware i18n gates, Black, subprocess encoding, brand, and docs lint. Screenshot bytes are unchanged and the immutable URL names the current composed top. Fresh CI and reviews are running; no manual CI rerun was made.
Current main-restack verification on 8c0de58: this two-commit PR is rebased directly onto current main at 3933456. The conflict resolution retains main's AutoNudge query invalidation and manual-fire route while preserving this layer's bounded-monitor editor, persistent trigger, and viewport-safe scroll container. #5186 and #5305 were restacked bottom-up with explicit leases. The composed top passes the 3,628-test selected backend run, all 1,344 focused frontend tests, the production website build, all 19 base-aware i18n gates, Black, subprocess encoding, brand, and docs lint. Screenshot bytes are unchanged and the immutable URL names the current composed top. Fresh CI and reviews are running; no manual CI rerun was made.
Current review-fix verification on
9f55d44ad993f46c63b9517aa7775eec077cc5ee: a legacy loop that arrives while the bounded editor is open now remounts the legacy editor against that loop’s identity, and removal frames invalidate both the slot snapshot and full legacy-loop registry. Both regressions failed on the prior head and pass after the fix; all 142 focused tests pass with targeted ESLint, TypeScript, andgit diff --checkclean. #5186 and #5305 were restacked and force-pushed bottom-up with explicit leases. Screenshot bytes and its immutable URL are unchanged. Fresh CI and reviews are running; no CI job was manually rerun.Current main-restack verification on
2534f95361c6e1e8f9a833cb2e199925a9887aea: this two-commit PR is rebased directly onto current main at575a8390eb12016688f8ef86c0bfa3a96cf136f0. The conflict resolution preserves main’s voice playback and ChatPage recovery dependencies alongside this layer’s structured automation seeding and rendering. #5186 and #5305 were restacked bottom-up with explicit leases. The composed top passes 876 focused backend tests and 275 focused frontend tests, TypeScript, targeted ESLint, Black, subprocess encoding, all 19 base-aware i18n gates, docs lint, and brand lint. Screenshot bytes are unchanged. Fresh CI and reviews are running; no CI job was manually rerun.Current final main-restack verification on
ee3e3a232d5ef7687c227a839aa6bd1d340c5869: this two-commit PR is rebased directly onto current main at2f9ed9724f852186cd497dc2b5ab7682a8948289; both patches replayed without conflicts. The composed top passes 1,264 focused self-arm, directive, provider, dashboard, and Slack tests with 2 platform skips, plus TypeScript, i18n, Black, isort, flake8, Linux-targeted mypy, docs, brand, and repository gates. Screenshot bytes are unchanged. Fresh CI and reviews are running; no CI job was manually rerun.Current main-restack verification on
bc9105c4497d976920b384b8ea00928112c26ca4: this two-commit PR is rebased directly onto currentmainat8a9c269b48e6a0a74708a69d4949929abbe128ad; #5186 and #5305 were restacked bottom-up with explicit leases. The composed top passes 1,265 focused self-arm, directive, provider, dashboard, and Slack tests with 2 platform skips, plus focused Black, isort, flake8, Linux-targeted mypy, docs, subprocess-encoding, and brand gates. Screenshot bytes are unchanged and the immutable URL names the current composed top. Fresh CI and reviews are running; no CI job was manually rerun.Current main-restack verification on
89dc2b30051e8af39d828824dce08e8169cc5122: this two-commit PR is rebased directly onto currentmainat7177fa4400c5c463c183b35c6ee67d2ec73cf2bd; #5186 and #5305 were restacked bottom-up with explicit leases and no conflicts. The composed top passes all 134 focused monitor, provider, directive, AutoNudge, and Slack tests plus all 42 security-conductor contract tests; focused Black, isort, flake8, Linux mypy, subprocess encoding, and brand gates pass. Screenshot bytes are unchanged and the immutable URL names the current composed top. Fresh CI and reviews are running; no CI job was manually rerun.Current main-restack verification on
38f9ead96: this dashboard layer is rebased directly ontomainat7fa305f35; #5186 and #5305 are restacked bottom-up on this head. The composed open tail passes 485 focused backend tests, all 30,540 website tests, and the 127 directly affected frontend tests, plus Black, flake8, Linux mypy, comment-history, subprocess-encoding, brand, harness-parity, and clean-clone diff checks. The unrelated Electron packaging suite is unavailable in this worktree because its nestedelectron-updaterandapp-builder-libfixtures are absent. Screenshot bytes are unchanged and the immutable URL names the current composed top. Fresh CI and reviews are running; no CI job was manually rerun.