refactor: route babysit through structured monitors - #5186
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 Design-Verdict: CONCERNS Sound migration aimed at a measured harm, but it deliberately reverses the pinned "explicit 0 = unlimited" contract — a human should confirm that reversal. Watch
Suggestions
[DESIGN-REVIEWED] f93c40c |
First Principles Review (Fable 5) — 🟡 CONCERNSPremise-level review of All evidence gathered and counted. Final review: First-Principles-Verdict: CONCERNS Two shipped surfaces have no reader: the inner Not justified as shipped
What this change shipsIntent: stop pull-request babysitting from spending a model turn per poll and coasting into caps, by routing it through the structured monitor with finite budgets — an ADDITION (migration step 7/8), not a fix.
More than 10: Webex refusal-text alignment (verified against Watch
Subtractions
[FIRST-PRINCIPLES-REVIEWED] f93c40c |
Opus 4.8 Review — ✅ no blocking findingsReviewed Review detailsI have independently verified the subtlest new machinery: the notification-persistence future is captured synchronously before any yield ( No findings. [OPUS-REVIEWED] f93c40c 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
github-actions
Bot
added
readiness: checking
6ce882d to
5f93b09
Compare
github-actions
Bot
added
readiness: action required
5f93b09 to
cf12433
Compare
cf12433 to
4e7bf86
Compare
4e7bf86 to
59ba569
Compare
github-actions
Bot
added
readiness: checking
109e3e5 to
9d9b230
Compare
github-actions
Bot
added
readiness: action required
|
Addressed the current review finding in 3478ea9: structured watches are now documented and enforced for dashboard, Slack, and Discord only; Webex routes to the finite legacy loop. The stateless tool and authoritative consumer both refuse structured Webex creation before persistence. Focused verification: 38 monitor/babysit tests passed. |
bolichen97
left a comment
bolichen97
left a comment
There was a problem hiding this comment.
Review: route babysit through structured monitors
Reviewed origin/token-monitors-dashboard...origin/token-monitors-babysit (15 files, +775/−682) at 94f4c00df. Findings were verified by executing the real tool → directive → applier → controller path, plus mutation testing against a scratch copy. The full surface passes on the PR head (928 tests via -k "monitor or autonudge or babysit or directive", 459 skills/docs tests, mypy clean on both changed Python modules, scripts/docs-lint.sh all checks passed), so every finding below is invisible to CI. The working tree was never modified.
The headline is finding 1: the PR's central behaviour change is unreachable on the default path. Everything else is downstream of that.
Blocking
1. config/prompt.md:106 — the always-on system prompt still hard-routes every babysit request to legacy monitor_start and never mentions monitor_watch, and the rewritten skill cannot be trigger-delivered at all. prompt.md is the installed system prompt (agent.py:354 registers it as the prompt file:// URI) and is in context for every session. Line 106 reads verbatim **Long task or "keep an eye on it" / "babysit" / "monitor":** use monitor_start.; line 113 lists the trigger phrases; line 119 gives the exact PR-polling recipe. It is not among the diff's 15 files. Meanwhile the skill is on-demand only — and worse, unreachable by trigger: the diff adds inject_on_trigger: false but the skill has no triggers: field, and SkillsLoader.get_triggered_skills (skills.py:4033-4035) continues on an empty triggers. Verified with max_triggered=5: "babysit PR 247 until it is review-ready" triggers prepare-pr, "monitor this pull request" triggers ops-mission-control, babysit never appears. Net: a user saying "babysit this PR" gets a legacy full-turn poll loop, and the RFC exit criterion "The supported pull-request path contains no repeated polling turns" is not met by default. prompt.md also still claims monitor_start "Works from dashboard chat, Slack threads, and Discord DMs" (no Webex, contradicting the descriptor this diff changed) and still offers HEARTBEAT.md at line 128 — the anti-pattern whose evidence-backed warning this diff deleted. Relatedly, the new spec sentence at memory-skills-hooks.md:912 ("an on-demand, pointer-on-trigger recipe") describes a delivery mode the code cannot produce.
2. builtin_skills/kirocrew-dev/babysit/SKILL.md:73 — the structured→legacy transition the skill instructs in three documented cases silently DELETES the active structured monitor and its retained evidence. An agent arms monitor_watch, then follows SKILL.md:55-57 / 66-69 / 17-19 ("use the finite legacy path") and calls monitor_start. _monitor_start → authorize_and_add_nudge → _add_unserialized hits existing = self._find_by_slot(slot_key) and, with the default replace_existing=True and no wake in flight, calls self.remove_sync(existing.id, persist=False, emit=False) (autonudge.py:1128-1137). Executed through the full path: watch applied -> Structured monitor ba3862b3 started, then legacy applied -> Monitor loop 51405ff4 started, then loops in memory: [('51405ff4', False)], persisted loops: [('51405ff4', False)], STRUCTURED RECORD SURVIVED? False — no terminal outcome, no stopped_reason, no dashboard event (emit=False), and the applied message never mentions what it destroyed. This directly contradicts src/kiro_crew/docs/monitoring.md, added by this same diff: "Stopping is durable: the dashboard and agent inspection keep a user_stop outcome rather than deleting the evidence." The reverse direction clobbers a legacy loop the same way. A second route in: same-turn monitor_inspect returns {"enabled":true,"monitor":null} — a success, indistinguishable from "no monitor exists" — which reads as SKILL.md:41's warning coming true and drives exactly this fallback; and because /api/autonudge/session-monitor returns the same null once a legacy loop is bound, the next turn's inspect still says null, so the loss is undiscoverable. SKILL.md:43's "The agent cannot inspect after ending that same turn" reads as the opposite of its intent and makes the same-turn call more likely.
3. monitoring/controller.py:138 — the new BUSY-branch budget stop bypasses record_monitor_dispatch_busy's _accepted_monitor_turns.pop, leaving a terminal BUDGET record with wake_in_flight=True that blocks re-arming the session with HTTP 409 for up to 2h1m. record_monitor_dispatch_busy pops the accepted-turn entry (autonudge.py:2198-2199) before applying the budget stop, forcing _retain_accepted_terminal_completion to see accepted=False and clear the claim. Routing the same transition through stop_monitor_if_budget_exhausted skips that pop, so the claim is retained and completion_evidence_deadline = now + 7260 is written. _add_monitor_locked (:1132-1136) then raises MonitorUpdateConflict('existing monitor cannot be replaced while a wake is in flight') → 409 unconditionally, with no exemption for terminal records. Reachable in production: _accept_dashboard_turn (slack/gateway.py:6017-6022) calls mark_monitor_turn_accepted and only then _settle_admission(DISPATCHED), a no-op if a BUSY settle already resolved the future — and mark_monitor_turn_accepted's guard permits wake_delivery is BUSY. Driven through the real _timer/_firing/tick path: decisions: [STOP_BUDGET], terminal record: active=False outcome=BUDGET wake_in_flight=True evidence_deadline_in=7260s, then monitor_watch again -> MonitorUpdateConflict. The superseded route in the identical state gave wake_in_flight=False and re-arm -> SUCCEEDED. stop_monitor cannot clear it; only remove() or a gateway restart does. The same record also carries next_due_ts ~2h in the future on a terminal row — a shape the loader itself flags as corrupt (_store_dirty, :633-638) and the WS broadcasts verbatim.
4. monitoring/controller.py:142 — the pre-existing BUSY-branch budget stop is now unreachable dead code, and it is the more correct of the two. stop_monitor_if_budget_exhausted resolves self._loops[monitor_id], which is the loop tick holds, and re-runs monitor_budget_reason(state, now=now) — the identical predicate on the identical object with the identical now. Proven by mutation: replacing lines 142-148 with raise AssertionError and running all 20 monitor/autonudge/babysit test files gives 661 passed, zero hits. Independently proven by spying: on the budget tick, record_monitor_dispatch_busy calls total: 1 — only the one from _dispatch_claimed; line 143 never runs. Not cosmetic: the dead branch is the one that popped _accepted_monitor_turns and called _sync_terminal_completion_timer — i.e. findings 3 and 5. The diff silently substitutes a less-correct stop while leaving the correct one in place for future readers to maintain. The minimal fix that achieves the stated goal without new code is to MOVE lines 142-148 above the if now < state.next_probe_at guard and delete the new call. Note the new placement also changes behaviour no test pins: a BUSY monitor now budget-stops on an early tick (now=121 < next_probe_at=125 -> STOP_BUDGET) where it previously returned NO_CHANGE, dropping the claimed wake's last redelivery attempt.
5. autonudge.py:1723 — stop_monitor_if_budget_exhausted ends with a bare _cancel_timer(loop.id) where all five sibling terminal paths use _sync_terminal_completion_timer, destroying the only timer that can expire an accepted in-flight claim. _apply_monitor_budget_stop → _retain_accepted_terminal_completion (:2052-2073) deliberately keeps wake_in_flight=True and sets a finite future completion_evidence_deadline/next_due_ts precisely so a timer can later expire it. Direct call in that state: outcome=BUDGET wake_in_flight=True completion_evidence_deadline=7385.0 next_due_ts=7385.0 waits_for_terminal_completion=True timer_registered=False accepted entry leaked: fp-1. The identical state via stop_monitor gives timer_registered=True. So record_monitor_completion_evidence_unavailable can never fire, the accepted turn's tokens are never charged (agent_turns: 0 total_tokens: 0), and the missing-evidence retirement never runs. Masked in production only because the sole caller sits inside _timer, whose finally (:2571-2576) independently re-derives _waits_for_terminal_completion and re-arms — but the method is a public _Service protocol member, so any future caller (a dashboard "stop if spent" endpoint, a start() budget sweep) reintroduces it. This is the sixth copy of the deepcopy → monitor_budget_reason → _apply_monitor_budget_stop → _persist_staged_monitor_locked → timer → _emit shape (:1687, 1938, 1991, 2213) and the only one that diverges from the shared epilogue; one extracted _publish_staged_monitor_locked(loop, staged) would make the divergence impossible.
6. autonudge.py:1726 — every structured-monitor terminal outcome is user-silent: all paths emit only "updated", never "expired", so _notify_nudge_expired never fires. The gateway observer calls it only for event == "expired" (slack/gateway.py:6202), and every _emit("expired") site (:2600/2616/2640/2760) sits in the legacy fire branch that structured monitors return before reaching (_timer returns at 2577). Executed: STOP_SUCCESS(review_ready), STOP_BLOCKED(pull_request_closed), STOP_BLOCKED(provider_authentication) and STOP_BUDGET(runtime_budget) all produce events=['updated'] expired_notify=False; the legacy cycle_cap case for contrast produces ['updated','expired'] expired_notify=True. So: Slack user says "babysit this PR and tell me when it's ready"; 4h later the runtime budget is spent with the PR still red, the monitor stops, and the Slack thread receives nothing. _notify_nudge_expired's own docstring names this: "the only signals were a log line and an active=False state change that looks identical to a manual Stop … the most confusing failure mode of the babysit feature." The skill documents only the terminal-SUCCESS silence (SKILL.md:55-57) and says nothing about budget/blocked/auth outcomes. The zero-cost mechanism the fix needs already exists (NotificationBus + dashboard_state.notify, with system.cron/system.agent precedents).
7. builtin_skills/.../babysit/SKILL.md:66 — review_ready SUCCESS can fire — zero turns, monitor deactivated — over an unanswered advisory review comment, and that routing rule is enforced only by prose. Executed against the real GitHubPullRequestProvider with a fake gh runner: on a PR whose advisory lanes post CONCERNS in a top-level comment while their own checks exit SUCCESS, the provider issues only gh pr view --json … and the reviewThreads GraphQL query — never comments or reviews — and returns status=SUCCESS reason_code=review_ready with blocking_review: none, unresolved_review_threads: 0. End to end: monitor_watch ACCEPTED with no evidence gate, decision=STOP_SUCCESS, outcome=SUCCESS, stopped_reason=review_ready, agent_turns=0, wake_count=0. Nothing refuses a comment-dependent objective — MONITOR_WATCH_SCHEMA's objective allowlist has exactly one value, so the boundary is the model's judgement about the user's private definition of readiness. The sibling Webex rule is code-gated (structured_monitor_binding_key_for returns None for webex:), making the asymmetry a same-PR inconsistency; the existing objective enum at validation.py:1148 is the seam a code-enforced version would use. Worse, src/kiro_crew/docs/monitoring.md — the only doc a user reads, indexed by this same diff — never states the caveat: its "Unsupported targets use the costly legacy loop" section lists only "Tickets, deployments, other forges, and custom objectives" (grep for comment/advisory/thread returns one unrelated hit), while the fact is stated five times in agent-facing files.
Should fix
8. builtin_skills/.../babysit/SKILL.md:63 — the skill instructs monitor_stop({"reason": …}) but the reason is discarded end to end. session_directive_apply.py:165-166 is elif kind == "monitor_stop": result = await _monitor_stop(session_key) — the only monitor directive dispatched without its args (monitor_watch, monitor_update and autonudge_stop all receive them). control.monitor_stop faithfully encodes the reason; _monitor_stop never reads it; stop_monitor hardcodes MONITOR_STOP_USER; _audit records no payload. Executed: payload {'reason': 'Merged by the release manager; watch no longer needed.'} → applied "Structured monitor 748a929f stopped and retained for inspection." → stopped_reason: user_stop, reason text anywhere in the store: False. So an operator inspecting a retained user_stop row can never learn why the watch ended — the exact failure the deleted skill section ("autonudge_stop should carry a real sentence naming the exit condition") existed to prevent. The diff's own new test test_babysit_user_stop_uses_real_tool_and_retains_terminal_state passes a reason and asserts stopped_reason == "user_stop", baking the discard in without noticing it.
9. mcp_tools/control.py:947 — Webex was added to two model-facing strings and left out of three others, including the acknowledgement actually delivered TO a Webex session. The same diff makes the skill route every Webex babysit to monitor_start and advertises "Works from dashboard chat, Slack threads, Discord DMs, and Webex conversations" (:347), and updated autonudge_stop's refusal to "a dashboard, Slack, Discord, or Webex session" (:883). But :947 still refuses with "monitor_start only works from within a dashboard, Slack, or Discord session … For other contexts use cron_add or a HEARTBEAT.md task", :1171 says the same for monitor_update, and :990 — the acknowledgement returned to a Webex session that just armed a loop — says "only a live dashboard/Slack/Discord session can host a loop, so do NOT assume it armed". Executed with session_key='telegram:1': monitor_start and monitor_update give the three-surface message while autonudge_stop gives the four-surface one, so three strings in one file disagree about the allowlist binding_key_for (autonudge.py:214) resolves. Two further gaps: monitor_watch's refusal text contains neither "monitor_start" nor "legacy", so a Webex agent is refused without being steered to the prescribed fallback; and the skill's blanket claim covers only Webex, while binding_key_for returns None for the eight other _CHANNEL_KEY_PREFIXES namespaces (telegram, wecom, whatsapp, teams, weixin, imessage, feishu, unified) where both paths refuse — verified as a double refusal, with no third branch documented anywhere.
10. mcp_tools/control.py:173 — the new autonudge_stop descriptor promises the record is "retained for inspection", but the applied confirmation the model reads is the unchanged legacy "No further nudges will fire." The behaviour is correct — _autonudge_stop routes structured loops to authorize_and_stop_monitor (session_directive_apply.py:571-581), and executing it gives outcome=user_stop with the record retained. But that branch falls through to the shared legacy return string (:585-589): "Auto-nudge loop 623d4a71 stopped on this session (reason: …). No further nudges will fire." — no mention of retention or user_stop, in contrast to _monitor_stop's own "Structured monitor {id} stopped and retained for inspection." (:542). The skill tells the model the retained outcome is user_stop, and both monitor_start's and monitor_update's descriptions name autonudge_stop as the stop verb — so the path the tool descriptions steer models toward is the one whose confirmation describes removal. The agent reports the watch was removed, does not inspect the retained state, and contradicts itself next turn. The diff's new registry test asserts "structured"/"durable"/"retain" only in the description, never in the applied result.
11. builtin_skills/.../babysit/SKILL.md:85 — the legacy path is bounded by prose only: "All three limits must be nonzero" is unenforced for two of the three, and the always-present schema text says the opposite. monitor_start's handler is max_runtime_secs = int(args.get("max_runtime_secs") or 0) and MONITOR_START_SCHEMA sets no minimum on max_cycles/max_runtime_secs; the schema descriptions the model sees every turn say "0 = unlimited, the default" and "Pass 0 for unlimited". Executed: {'interval_secs':300,'max_cycles':0,'max_runtime_secs':0} → accepted, ack reads "…with NO cycle cap" and no wall-clock clause, armed loop max_cycles=0 max_runtime_secs=0; omitting max_runtime_secs entirely also yields 0. Only interval_secs is actually bounded. The structured path it defers to enforces the same idea in code (MonitorBudgets.__post_init__ rejects <=0). Meanwhile the diff deletes the measured justification (4 of 4 real loops ended exactly at their cap, 0 of 4 carried an agent-supplied stop reason, none set max_runtime_secs), the stall tripwire that was the legacy path's only no-progress exit, and the GitHub/GitLab/Bitbucket verdict protocol — leaving "perform the smallest safe status check" as the entire read guidance for the non-GitHub hosts legacy now exclusively serves. The replacement verification instruction is also unexecutable by the agent: /api/autonudge is in neither _STRICT_INTERNAL_API_PATHS nor _MIXED_INTERNAL_API_PATHS (verified by importing both sets), the old ~/.kiro/crew/autonudge.json file-read fallback was deleted, and the repo's purpose-built prepare-pr/scripts/monitor_armed.py is not mentioned.
12. autonudge.py:1718 — because the pre-probe stop bypasses apply_monitor_probe entirely, a terminal BUDGET record no longer records the decision that stopped it. apply_monitor_probe sets probe_count += 1, last_probe_at = now and last_decision for every decision (:1654-1656), so pre-diff a budget-stopped row always advertised last_decision='stop_budget'. All four fields are in MONITOR_PUBLIC_FIELDS and reach three consumers: the REST /api/autonudge/session-monitor response, website/src/monitoring/automation.ts:188,191 (latest.decision, usage.probes), and the agent's monitor_inspect projection. Executed: in-budget tick → probe_count=1 last_decision=record_only; next tick → decision=stop_budget outcome=budget stopped_reason=runtime_budget but probe_count=1 last_probe_at=120.0 last_decision=record_only. On a restart-shaped stop: outcome=budget probe_count=0 last_decision=None — a budget-stopped monitor that reports it never made a decision. No test pins these on the new path. Related seam: start()'s monitor recovery (:640-666) never evaluates monitor_budget_reason either, so a monitor whose runtime elapsed while the process was down is restored active=True and rendered as "active" until its next persisted deadline.
13. docs/system-specs/features/babysit-pr-watch.md:177 — deleting the prompt-owned polling protocol leaves dangling references in five untouched files and orphans ~1,600 shipped lines with no reachable entry point. AGENTS.md § Specification management: "MUST update the spec in the SAME commit when an API, schema, or documented behavior changes" (CONTRIBUTING.md: "A PR that changes behavior and leaves its doc stale will be sent back"). babysit-pr-watch.md is not in the diff and is now false at line 5 ("offers two current monitoring modes"), line 21, line 177 ("the babysit skill's watch recipe copies the bundled pr_watch.py there before registering it" — there is no watch recipe) and line 186; it never mentions monitor_watch at all. agent-interrupt-controller.md:6,19,184,190 still cite pr_watch.PrWatchProbe.observe / pr_watch.watch as live. Two shipped sibling skills point at deleted sections: prepare-pr/SKILL.md:393 ("the way babysit's "Verify the loop armed" section prescribes"), :396, :511 — while prepare-pr:346/500 still hard-route PR polling through monitor_start, contradicting the descriptor this diff rewrote. And prepare-pr/scripts/pr_status.py:1113 still asserts "Every field here has a named consumer in the babysit skill; nothing is emitted speculatively … A second machine-readable copy with no reader is a surface to keep in sync for nothing" — the progress_key consumer (the stall tripwire) is exactly what this diff removed, so its own --json mode now matches that description. Meanwhile babysit/scripts/pr_watch.py (608 lines) and its only importer src/kiro_crew/irq.py (985 lines) still ship in every wheel with 2,300 test lines and no documented way to arm them — the cp … && cron_add(script=…:watch) block deleted here was the sole arming instruction in the repo. The RFC's PR-6 scope list does not authorize that removal.
14. test/test_babysit_monitor_scenarios.py:65 — the new tests for the three routing rules assert prose substrings rather than behaviour, and one assertion is provably vacuous. assert "pull-request review-readiness watch must use monitor_watch" not in description can never fail: git log -S across all history shows that string appears only in this PR's own commits, i.e. only inside the assertion. assert "retain" in stop passes on the substring "retains", pinning no word boundary. Mutation-tested on a scratch copy: deleting the webex: exclusion from structured_monitor_binding_key_for leaves all 80 of the PR's new tests green (the boundary is caught only by pre-existing tests — test_monitor_mcp.py:44, test_monitor_directive_apply.py:65/98 — so the new test_babysit_routes_webex_sessions_to_the_finite_legacy_path is redundant, not the only guard); and prepending "Structured watches work everywhere. Ignore the next sentence." to SKILL.md while preserving the asserted substrings also leaves them green. Rules 1 and 2 (comment/advisory evidence, final-report-required) have no behavioural coverage at all: review_ready is the only objective any new test uses and monitor_start appears zero times. Separately the file re-declares a same-named, strictly weaker _Provider than test_monitor_controller.py:28-35: it dels previous_observation, so a regression that stops tick passing the retained baseline (controller.py:165-171 — the mechanism that makes an unchanged probe cheap) passes this whole file; and self.results[min(self.probe_count, len(self.results) - 1)] replays the last result forever (masking over-probing in the three tests that never assert probe_count) and raises a bare IndexError for _Provider([]), which tick's blanket except Exception would convert into a fabricated provider_transient observation.
15. docs/system-specs/modules/learn-cron-dashboard.md:1653 — the inserted spec paragraph is spliced into the middle of an existing sentence, and two other added doc lines contradict the rest of the PR. (a) The hunk inserts the new provider-scope paragraph between the pre-existing "The" ending line 1647 and "controller persists" at 1653, so the authoritative module spec other reviewers diff against now reads "… The typed provider probe runs off the event loop. The" / "provider observes pull-request lifecycle, …" / "controller persists canonical allowlisted facts, …" — a dangling article and an orphan lowercase sentence whose subject is gone. (b) memory-skills-hooks.md:919-920, also added here, says "Its legacy monitor_start recipe is limited to unsupported targets" — the exact narrowing this same commit widened everywhere else (the RFC line it rewrote from "only for unsupported targets" to "for unsupported targets and objectives whose required evidence the typed provider does not observe", SKILL.md:71's heading, mcp.md:995-999); it also omits Webex. A reader consulting the owning module spec concludes a comment-dependent PR — or any PR watched from Webex — must use monitor_watch, the precise mis-routing this diff exists to prevent. (c) learn-cron-dashboard.md:1685 "Token enforcement uses the usage values the provider reports" overloads "the provider", which the same file defines 40 lines earlier as the typed GitHub probe; tokens come from monitoring/completion.py's AgentTurnUsage, and nothing in the GitHub provider reports tokens. SKILL.md:51 repeats the collision; only monitoring.md:31 disambiguates ("the model provider").
Execution-verified AI-assisted review (Claude Code) across 10 parallel angles plus a gap sweep, run against a local checkout at 94f4c00df; git status --porcelain was empty throughout, all scratch work lived in /tmp, and no other branch or worktree was examined. Checked and cleared, so please don't re-spend on them: no test double breaks on the _Service protocol change (every MonitorController( site passes a real AutoNudgeService); token_usage_known is correctly wired through MONITOR_PUBLIC_FIELDS, REST, WebSocket and automation.ts; the skill's monitor_watch example validates against every limit; Webex genuinely is refused at both claimed boundaries and the consumer boundary has pre-existing tests; not touching CHANGELOG.md is correct per AGENTS.md; and the persist-before-disarm ordering the PR claims for the pre-probe budget stop does hold under execution — the defects in that area are all in the lifecycle bookkeeping that runs after the write succeeds. Findings name the input that reproduces them, so please push back where one misreads intent.
|
Addressed the current babysit-controller feedback and restacked this branch in e187edf. Pull-request babysitting now routes through the bounded structured monitor path without weakening the eight-PR stack. Focused controller/backend verification passes, and the composed monitor suite passes 234/234. |
|
Checked the current review finding against the exact PR head and diff: |
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
|
|
/ai-review override gpt 7bdedcb: The reported skills.py digest line is absent from this PR diff; at this exact head line 645 is an app-manifest try block, so the finding reviewed code outside the current change. |
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 GPT blocking findings in 95fe341:
Both regressions failed on the prior head and now pass. Focused verification: 348 AutoNudge/persistence/Slack gateway tests passed; 410 broader Slack/babysit tests passed; Black, subprocess encoding, flake8, Linux mypy, docs lint, brand, and diff checks passed. #5305 was restacked and pushed bottom-up with explicit leases. No CI job was manually rerun. |
|
PR Hygiene correctly caught the temporary third commit. I folded the new fix into the existing terminal-notification commit and force-pushed with the recorded lease. Current #5186 head is 91ce5df; it remains exactly two commits above #5185 and its tree is byte-for-byte identical to the verified 95fe341 tree. #5305 is restacked at eb09468, also exactly two commits above #5186. |
2 participants
Problem / Motivation
The shipped babysit guidance teaches prompt-owned polling, voluntary completion detection, and manually managed progress state for GitHub pull requests.
Why it matters
That guidance spends a full agent turn on unchanged state and empirically tends to stop at its cap instead of stopping because the objective was actually observed.
What changed (motivation → approach → change)
Replace GitHub polling instructions with one bounded
monitor_watchcall, later authoritativemonitor_inspectcalls, and explicitmonitor_updatere-arming.Document conductor self-patrol as a first-class monitor use while keeping the compatibility watcher finite and secondary.
Document zero-turn unchanged probes, one wake per actionable fingerprint, finite budgets, completed-turn accounting, durable stop, and explicit restart.
Persist pre-probe budget exhaustion before disarming the live monitor, keeping memory and restart state aligned when a terminal write fails.
Route Webex sessions through the finite legacy path because structured wake delivery and completion correlation are unavailable there.
Keep a clearly labeled finite legacy fallback for unsupported targets and readiness definitions that depend on generic PR comments or advisory findings outside the typed provider facts.
Add packaged monitoring guidance and executable lifecycle scenarios using the real tool-to-directive-to-controller path.
Scope the legacy tool advertisement to public GitHub at this layer and to the same evidence boundary: structured review readiness is preferred only when typed provider facts fully determine it.
Route requests that require a final report or notification through the finite legacy path, because terminal structured success deliberately consumes zero model turns.
Deliver gated legacy Slack terminal notifications with durable at-least-once acknowledgement: terminal state is persisted before delivery, startup replays retained undelivered notifications, and the exact monitor generation is marked only after emission; audit unsupported evidence scopes separately from unsupported session bindings.
Render terminal notifications from the stored stop reason: state closed-unmerged PRs directly, distinguish merged from ready-for-review success, name known provider/delivery blockers, and identify the watched PR URL even for channel-bound watches. Unknown reasons retain a safe details fallback; unavailable conversations point to a new watch from an active conversation.
Scope the finite-budget policy to the legacy MCP tools: shared REST/workflow AutoNudge helpers retain
0 = unlimited, with the seven-day ceiling applying to positive runtime grants.Tests
Previous notification verification: all 32 focused notification/observer tests pass, including 12 reason-specific regressions and target identification for dashboard and channel-bound watches. Merged success alone says no action is needed; review readiness asks the operator to review the PR, and only a recorded closed-unmerged reason offers reopening. Focused Linux-platform mypy, Black, flake8, isort, docs lint, brand, and diff checks pass. The notification-only follow-up does not change budgets, delivery, or terminal-wake policy. The overall PR does change legacy MCP defaults: 24 cycles and 4 hours, with explicit zero/unlimited caps rejected. Long-interval watches need an explicitly larger runtime (up to 7 days); the runtime limit can stop the watch before all 24 cycles are used. Shared REST/workflow helpers retain zero/unlimited compatibility. Fresh CI and UX review on the pushed head are pending.
Previous recipe follow-up: the finite legacy recipe explicitly sets
gate: false, so unchanged typed provider facts cannot suppress cycles that must inspect generic comments or advisory feedback. The executable recipe regression failed with the old omitted flag and passes with the correction; a fresh reader also includes the opt-out. All 22 focused babysit/ack tests pass. The specification now accurately distinguishes positive MCP budgets from REST/workflow zero/unlimited compatibility. Docs lint, Black, flake8, brand, and diff checks pass. Screenshot bytes and runtime implementation are unchanged by this follow-up; fresh CI is pending on the restacked head.Previous security follow-up: redact the completed terminal notification body, including retained targets, through the shared URL and credential redactors before dashboard persistence. Three regressions cover credential-shaped GitHub paths, encoded credentials in URLs, and malformed retained plain credentials; all three fail without the fix. All 54 focused notification, gateway-initialization, babysit, and acknowledgement tests pass on this layer. Black, isort, flake8, docs lint, and diff checks pass. The composed top also passes all 187 focused tests and Linux-targeted handler mypy. Screenshot bytes are unchanged; fresh CI and review on
5c975cb4467f15bdde12340785f4680908ec5fe3are pending. No manual CI rerun.Latest main-restack verification on
c3169529b90019775a65cecdfd7429556ba01926: all 112 focused monitor, babysit, acknowledgement, directive, dashboard-handler, and Slack notification tests passed. Docs lint and diff checks also passed. The documentation conflict preservesmain's shared loop-by-id accessor invariant while retaining this layer's structured-monitor contract. Screenshot bytes are unchanged. Fresh CI and reviews are pending; no manual CI rerun was made.Manual verification
N/A — executable lifecycle scenarios cover runtime outcomes, and a focused policy contract pins the skill's structured-versus-legacy evidence boundary.
Related Issues
N/A — completes the migration specified in #5180 and depends on #5185.
Checklist
Contribution License Agreement
N/A — the repository template does not yet supply final CLA wording.
Current review/CI follow-up on
a46b9635bc317874b4a28d8d274da9b9e3cecbad: restore the documentation tail lost during conflict reconstruction; use inferable pull-request URLs in both monitor examples; distinguish Webex's stop-and-recreate path from monitor_update; and align the banner regression with this layer's intentional four-hour finite default while continuing to require the banner field to be absent. All 300 focused monitor, babysit, acknowledgement, directive, and registry tests pass. Docs lint, Black, flake8, and diff checks pass. Screenshot bytes are unchanged. Fresh CI and reviews are pending; no manual CI rerun was made.Current main-restack verification on
c0056fdc0b4457f148876457c528985aeae0b1d9: this layer is directly based on #5185 at85e0baa2c4e24b322e9fa1c1af8e08a21fe6589a. Terminal notification delivery is durable and replay-safe for the exact monitor generation. 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 are unchanged. Fresh CI and reviews are running; no manual CI rerun was made.Current durability/restack verification on
3b5f883a3721d35179b1b7d7caeb14c8df6dd317: this layer is directly based on #5185 atc5688956f5159c509895e7e1a96a3aad96ef1bfb. Terminal notification persistence is awaited successfully before the exact generation is marked delivered, so failed writes remain replayable. The composed open stack passes all 446 focused controller, provider, AutoNudge, and Slack gateway tests; focused Linux mypy, flake8, and docs lint pass. Screenshot bytes are unchanged. Fresh CI and reviews are running; no manual CI rerun was made.Current main-restack verification on
b2e22498966e2c2aa4f016e85426a5facb616c6f: this two-commit layer is directly based on #5185 atd3dfa8d704b8e48b90bb5d60a49b432115fb2728. The test overlap retains dashboard directive-delivery assertions and the structured babysit evidence-scope contract. 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 are unchanged. Fresh CI and reviews are running; no manual CI rerun was made.Current review-fix/restack verification on
e8ddb34e3211e8c417dabfa0b543773d0bec72a9: this two-commit layer is directly based on #5185 at0085af85de0c715529148d61fea3eb4c6f87791e. The introducedtest/test_monitor_mcp.pyformatting failure is corrected in its owning commit; Black leaves the file unchanged and all 19 monitor MCP tests pass. Screenshot bytes are unchanged. Fresh CI and reviews are running; no manual CI rerun was made.Current main-restack verification on
7e2a294b73b34eb5e9300a8a5755c03ea0884c1f: this two-commit layer is directly based on #5185 at12eecef59dcb20f1f973969d91aea2df0c4ec1cf. The sole conflict preserves main’s concurrent skill-catalog scan contract alongside this layer’s structured babysit recipe. 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 are unchanged. Fresh CI and reviews are running; no manual CI rerun was made.Current review-fix/restack verification on edc01d9: this two-commit layer is directly based on #5185 at 5866cc1; its notification-persistence commit is patch-equivalent to the prior head, while the first commit inherits #5185's raw-quarantine specification. 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. Fresh CI and reviews are running; no manual CI rerun was made.
Current hygiene-clean verification on 51f975e: this PR is exactly two commits and is directly based on #5185 at 6d4630a. The composed top tree remains byte-for-byte identical to the already verified pre-fold tree. No CI job was manually rerun.
Current inherited-fix restack verification on 03cb97f: this two-commit layer is directly based on #5185 at 5afa6d3 and inherits its non-finite quarantine fix without changing this layer’s own patch. The composed open tail passes all 285 focused monitor, MCP, GitHub, and source-provider tests. Fresh CI and reviews are running; no manual CI rerun was made.
Current main-restack verification on a3284eb: this two-commit layer is directly based on #5185 at 47fede7, preserving the linear stack on main at 8aef8fe. Its own patch is unchanged; the composed top passes TypeScript, the analyze build, the 812-chunk bundle gate, and all 19 base-aware i18n checks. Fresh CI and reviews are running; no manual CI rerun was made.
Current main-restack verification on e444ec9: this two-commit layer is directly based on #5185 at e677cac. Its structured-monitor and durable terminal-notification patches replay cleanly on main at 6309382. 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. Fresh CI and reviews are running; no manual CI rerun was made.
Current guidance-fix verification on 2ea5aa6: the shipped monitor_start instructions again include a concrete full GitHub pull-request URL that the real subject inference accepts, so copied legacy examples activate observation gating. The regression reproduced on the prior head and all 3 guidance-gate tests now pass; all 92 focused guidance, babysit, controller, and monitor-MCP tests plus docs lint and diff checks pass. This layer remains exactly two commits on #5185 at e677cac. #5305 was restacked with an explicit lease. Fresh CI and reviews are running; no manual CI rerun was made.
Current source-path review fix on 3389378: babysit scenario fixtures now derive repository files from
Path(__file__).resolve(), clearing the current GPT blocking finding without changing runtime behavior. All 502 focused babysit and monitor tests pass, along with docs lint. This layer remains exactly two commits directly on #5185; #5305 was restacked and pushed bottom-up with explicit leases. Fresh CI and reviews are running; no manual CI rerun was made.Current source-path gate fix on f904536: babysit scenario fixtures derive repository files from an absolute resolved root and the touched test is Black-clean. All 502 focused babysit and monitor tests pass, plus focused Black and docs lint. This layer remains exactly two commits directly on #5185; #5305 was restacked and pushed bottom-up with explicit leases. Fresh CI and reviews are running; no manual CI rerun was made.
Current main-restack verification on fc34b5d: this two-commit PR is directly based on #5185 at e993142. The prepare-pr guidance resolution preserves main’s current babysit verification wording and this layer’s finite runtime/cycle budget, explicit monitor_update re-arm, and gate:false fallback requirements. #5305 was restacked on this head with an explicit lease. 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. Fresh CI and reviews are running; no manual CI rerun was made.
Current inherited main-restack verification on 950ee12: this two-commit PR is directly based on #5185 at 8c0de58, preserving the linear open tail. Its babysit patch is unchanged. 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. Fresh CI and reviews are running; no manual CI rerun was made.
Current inherited main-restack verification on 950ee12: this two-commit PR is directly based on #5185 at 8c0de58, preserving the linear open tail. Its babysit patch is unchanged. 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. Fresh CI and reviews are running; no manual CI rerun was made.
Current terminal-delivery review fix on 91ce5df: the Slack observer is installed before service startup, so a terminal transition during startup cannot escape both live delivery and replay. Delivery acknowledgements now persist the exact terminal generation on the stable outer loop record, preserving opaque future-version monitor payloads while preventing repeat notices after restart. Both regressions failed on the prior head and pass after the fix; all 348 focused AutoNudge, persistence, and Slack gateway tests pass, plus 410 broader Slack/babysit tests. Focused Black, subprocess encoding, flake8, Linux mypy, docs lint, brand, and diff checks pass. Screenshot bytes are unchanged. #5305 was restacked and pushed bottom-up with explicit leases. Fresh CI and reviews are running; no manual CI rerun was made.
Current inherited main-restack verification on
0de1af3e4bdb993b3137478b9db6276f4e8277cb: this two-commit layer is directly based on #5185 at8cb33d195d3e91dbf18b595f038e5238641b26fa. All 426 focused AutoNudge, monitor, directive, and terminal-notification tests pass. #5305 was restacked and force-pushed above it with an explicit lease. Screenshot bytes are unchanged and any immutable screenshot URL now names the current composed top. Fresh CI and reviews are running; no CI job was manually rerun.Current inherited dashboard review fix on
e2a3efceed983ede5ded0333052e17b461569943: this two-commit layer is directly based on #5185 at9f55d44ad993f46c63b9517aa7775eec077cc5eeand inherits its live legacy-editor reseed and full-registry removal invalidation. Git range-diff confirms both babysit patches are unchanged. #5305 was restacked and force-pushed above it with an explicit lease. Screenshot bytes are unchanged. Fresh CI and reviews are running; no CI job was manually rerun.Current inherited main-restack verification on
fbd19a2b70e0d1ae647dc9a2c45fd90e7e3baf3b: this two-commit PR is directly based on #5185 at2534f95361c6e1e8f9a833cb2e199925a9887aea; both of its patches replayed without conflicts. #5305 was restacked above it. 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
be1a2de18a0bd15a3afa51abba6b3f9e4edaeff7: this two-commit PR is directly based on #5185 atee3e3a232d5ef7687c227a839aa6bd1d340c5869. The AutoNudge resolution preserves main’s trusted crew/member self-arm and visible arm outcome together with this layer’s channel-specific monitor guidance; closing-slot admission retains the authoritative_closingfence. 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. Fresh CI and reviews are running; no CI job was manually rerun.Current main-restack verification on
f93c40c10f01327fad422a3a35c53ba2e3c8bc01: this two-commit PR is directly based on #5185 atbc9105c4497d976920b384b8ea00928112c26ca4, preserving the linear open tail; #5305 was restacked on this head with an explicit lease. 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. Fresh CI and reviews are running; no CI job was manually rerun.