diff --git a/.claude/commands/reviewer.md b/.claude/commands/reviewer.md index a2c6e1b..b05909e 100644 --- a/.claude/commands/reviewer.md +++ b/.claude/commands/reviewer.md @@ -13,7 +13,9 @@ You are a single-shot PR reviewer worker. `task-reviewer` spawned this tab with 4. Run the `code-review` skill on the diff. 5. Post **one** substantive PR comment with the full analysis (spec compliance + code-review findings + verdict). 6. Radio PM back with `review-complete-clean` or `review-complete-with-findings` and a short summary. + 7. **Auto-destruct.** Your analysis lives in the PR comment (durable) — the tab does not need to stay open. Immediately after the PR comment is posted AND PM is radioed, run `task-done --remove-worktree` as your final action to remove this review worktree and close the tab. Do NOT idle. + Refer to the project's `.claude/gh-workflow.md` for GitHub owner/repo conventions. Use the `gh` CLI for PR + issue I/O: `gh issue view`, `gh pr view`, `gh pr diff`, `gh pr comment`. Read-only `gh` patterns are pre-allowed in `.claude/settings.json`; mutations stay confirmation-gated. @@ -47,6 +49,7 @@ $ARGUMENTS - **Code-review findings** — correctness, security, edge cases, anything substantive from the skill's output. - **Verdict** — `clean`, `clean-with-nits`, or `changes-requested`. Be explicit. + **Tight-PR norm — prefer fix-in-this-PR over deferral.** Frame every finding as something the worker should fix *in this PR*, not as a follow-up ticket, whenever the fix is feasible within the PR's scope. Do NOT suggest "defer to a follow-up" / "track separately" for anything in-scope — that's a deferral the team is trying to avoid. Only flag a deferral when the work is genuinely out of this PR's scope (a separate subsystem, a large refactor), and say so explicitly — PM will decide whether to groom it into a ticket. Default: tight PR, everything fixed before merge. 7. Post the comment — **on stdin, never through a double-quoted shell string.** Inside `"…"` the shell command-substitutes every backticked token in your prose: `` `pwd -P` `` posts as your working directory, `` `radio orphans` `` posts as that command's output, and a backticked identifier that names no command — `` `$SOME_VAR` ``, `` `_some_helper()` `` — posts as **nothing at all**, deleting the finding's subject and leaving a sentence that reads as if a word were missing. Findings here name functions, variables and paths in inline code constantly, and this comment is the only thing that outlives your tab, so feed the body in with `--body-file -` and a **quote-delimited** heredoc: ```bash @@ -71,6 +74,7 @@ $ARGUMENTS 9. **Auto-destruct.** Run `task-done --remove-worktree` as your final action — it removes this review worktree and closes the tab, and `--remove-worktree` skips the confirmation prompt, so it needs no input from you. PM reads your verdict from the PR comment + the radio ping (on its next `radio check`), never from this tab, so there is nothing to keep open. Self-cleaning is mandatory, not optional. **The one thing that is not in the PR comment** is step 8's delivery outcome. If `radio send` reported anything other than a landed delivery — `WARNING — no session for pm-…`, `WARNING — pm-… looks dead …`, or `queued — pm is idle but wake failed …` — that warning exists nowhere but this tab, and destroying it would strand the verdict unread. In that case only: say so to the user, and hold the tab until they have seen it. Otherwise, destruct. + ### Authority boundaries diff --git a/CHANGELOG.md b/CHANGELOG.md index 79a36b0..1b0e02e 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -7,6 +7,12 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.1.0/), ## [Unreleased] ### Added +- **kiro-notion and kiro-local ship a reviewer agent, and every kiro reviewer now self-destructs like the claude one (#320).** Only kiro-gh had `agents/reviewer.json`, so `task-reviewer` refused on kiro-notion and kiro-local ("Reviewer support currently ships for …"), the one gap in the reviewer column; it was hit on a kiro-notion repo, where the way round was `--agent claude` on a dual install. Both loadouts now ship one, ported from kiro-gh's and adjusted the way each loadout's `worker.json` is: kiro-notion's carries the `@notion` tool and the `notion` MCP server and reads the spec page passed as the second arg through it; kiro-local's reads the `tasks/` file named by a slug or path from disk, with no tracker MCP. Neither auto-detects a spec from `Closes #N`, which is GitHub-only. `task-init` installs the new agent with its radio hooks merged in, as it does every file under `agents/`, so `task-config add assistant kiro` on a notion or local repo brings it along. + - **The kiro reviewer self-destructs.** kiro-gh's reviewer still ended "Idle — the tab stays open … Do NOT auto-cleanup": #162 made the claude reviewers single-shot and self-cleaning and left kiro out. All three kiro reviewers now end the way the claude ones do: post the comment on stdin, radio the verdict and read `radio send`'s outcome, then `task-done --remove-worktree`, holding the tab open only when the verdict never reached PM. They also gain the tight-PR norm. That prose is now one drift-checked block in all eight reviewer prompts (the four claude `commands/reviewer.md`, the dogfood `.claude/commands/reviewer.md` and the three kiro `agents/reviewer.json`), the new `reviewer-contract` group in `tools/check-drift.sh`, so the two assistants cannot part ways on it again. + - **`tools/check-drift.sh` reads prompts.** A kiro agent config (`*.json`) is now checked on its decoded `prompt`, whose lines exist only once the JSON string is decoded, and a region may be marked `` / `` where a `# region:` line would render as a heading in a markdown prompt. + - **`task-reviewer`'s refusal** no longer lists loadouts: it is still keyed on the shipped file, so it now names the missing `/agents/reviewer.json` and the `--agent claude` way round. No shipped loadout hits it. + New tests: the kiro-notion and kiro-local launches pass their spec through (stubbed zellij and kiro-cli), a `Closes #N` is not their spec, the refusal on a tree copy with the agent taken out, `task-init` installing `reviewer.json` with its register hook on both, `add assistant kiro` bringing it along, and drift-checker cases for JSON prompts, comment sentinels and a broken kiro copy of the reviewer contract. The prompt sweeps (`prose_body_file`, `worker_checklist`, `kiro_radio_pull`) now cover all eight reviewers. Removing either new `reviewer.json` turns its launch tests red. README, the kiro-gh workflow doc (its reviewer tab no longer "stays open") and the kiro-notion / kiro-local workflow docs, which had no reviewer paragraph at all, describe it. + **Upgrading: re-run `task-init kiro-notion` / `task-init kiro-local`** to install the reviewer agent, and `task-init kiro-gh` so its reviewer self-destructs. The claude `commands/reviewer.md` changed only by the two invisible region comments, so a claude re-run is optional. - **PM handoff across assistants: a board digest at a fresh PM start, an optional handoff note, and no second live PM (#301).** With both assistants installed (#293) the PM role moves from claude to kiro and back. The inbox carried over, since the address is the same, but the working picture did not: a new PM started blind and spent tokens re-reading the tracker to rebuild what task-monitor already knew. - **Board digest.** A fresh `pm-*` register now prints `task-monitor-snapshot --local --brief` under the offline-backlog summary: attention signals first, then in flight (ref, title, PR, review state), the queue with its notes, and the last 24h of merges with each PR summary's first sentence. `--brief` is a new output mode; the text comes from `lib/monitor-digest.jq`, and `tm_board_digest` in `lib/monitor-text.sh` caps it at 40 lines, the last one saying how many were cut. `--local` means no forge call, so start-up stays fast and works offline. It is printed under the same gate as the backlog summary (`fresh=1`, not an alias), so never on a `/compact` or `/clear` re-register, and never for a worker. Hook stdout reaches the model on claude and kiro alike. `TASK_FORCE_NO_PM_DIGEST=1` turns it off. - **Handoff note.** `task-monitor-queue handoff write` saves 5–10 lines from stdin to `$TASK_FORCE_HOME/monitor//handoff.md`, with `written_at`, the PM role and its agent in frontmatter; `handoff show` prints it and `handoff clear` removes it. It is a `task-monitor-queue` subcommand rather than a new command, so there is nothing new to install. The snapshot gains a `handoff` field (`{text, written_at, role, agent}` or null). The next fresh PM start prints the note under the digest with its age, capped at 20 lines, and it stays until overwritten. task-monitor shows it as the repo view's last row, collapsed to its first line with its agent, age and length, and `⏎` expands it. diff --git a/README.md b/README.md index 755d5ad..6ae9e57 100644 --- a/README.md +++ b/README.md @@ -1289,14 +1289,14 @@ task-reviewer [] `task-reviewer` spawns a fresh zellij tab + git worktree on the PR's head ref, then runs the `/reviewer` slash command (or the kiro `reviewer` agent) inside it on Sonnet 5.5 — `ANTHROPIC_MODEL=claude-sonnet-5-5` by default on claude (pre-set the env var to override), `--model claude-sonnet-5.5` on kiro (`-m` to override). The kiro reviewer also reads `$TASK_WORK_MODEL` ahead of that default, so a worker override exported in your shell dispatches reviewers on the same model, at its price (`claude-opus-5.5` is 2.00x kiro credits, Sonnet 1.30x); unset it or pass `-m` to keep reviewers on Sonnet. Claude Code and kiro-cli spell the same model differently, hyphens versus dots. The PM's tab stays focused. Add `Bash(task-reviewer *)` to the project's `.claude/settings.json` `permissions.allow` so the PM can dispatch a reviewer without a permission prompt. The reviewer: -1. Reads the spec (passed as the second arg — issue number / URL for `claude-gh` / `kiro-gh`; Jira key for `claude-jira`; Notion page URL for `claude-notion`; local task slug or path for `claude-local`). On `claude-gh` / `kiro-gh` only, the wrapper also auto-detects from the PR body's first `Closes #N` / `Fixes #N` / `Resolves #N` line (case-insensitive); non-gh loadouts require the spec identifier explicitly because PR bodies don't carry their tracker's linking convention. +1. Reads the spec (passed as the second arg — issue number / URL for `claude-gh` / `kiro-gh`; Jira key for `claude-jira`; Notion page URL for `claude-notion` / `kiro-notion`; local task slug or path for `claude-local` / `kiro-local`). On `claude-gh` / `kiro-gh` only, the wrapper also auto-detects from the PR body's first `Closes #N` / `Fixes #N` / `Resolves #N` line (case-insensitive); non-gh loadouts require the spec identifier explicitly because PR bodies don't carry their tracker's linking convention. 2. Reads the PR diff + comments. -3. Cross-checks the diff against the spec, then runs the `code-review` skill on top (claude variants — kiro stays prompt-driven). +3. Cross-checks the diff against the spec, then runs the `code-review` skill on top (claude variants — kiro stays prompt-driven). The kiro-notion reviewer reads its spec page through the Notion MCP, as its worker does; the kiro-local one reads the task file from disk. 4. Posts **one** thorough PR comment with spec-compliance findings, code-review findings, and a verdict (`clean`, `clean-with-nits`, or `changes-requested`) — via `gh pr comment --body-file -` off a quoted heredoc, so the backticked code its findings are made of survives the shell. 5. Radios PM back with `review-complete-clean` or `review-complete-with-findings`. -6. **Auto-destructs** — on the claude loadouts the reviewer's final action is `task-done --remove-worktree`, which removes the review worktree and closes its own tab. It is single-shot and self-cleaning: the analysis is durable in the PR comment, so nothing is lost when the tab goes. The kiro `reviewer` agents still idle with the tab open — clean those up by hand. +6. **Auto-destructs** — on every loadout, claude and kiro, the reviewer's final action is `task-done --remove-worktree`, which removes the review worktree and closes its own tab. It is single-shot and self-cleaning: the analysis is durable in the PR comment, so nothing is lost when the tab goes. Until #320 the kiro reviewer idled with its tab open instead. Posting on stdin, the radio verdict and the self-destruct are one block of prose in all eight reviewer prompts, drift-checked by `tools/check-drift.sh` (the `reviewer-contract` group), so the two assistants cannot part ways on it again. -PM still decides whether to merge or request changes — the reviewer never approves, merges, closes, or mutates Status. Re-read a review with `gh pr view --comments` rather than hunting for a tab that cleaned itself up. The one case a claude reviewer keeps its tab is a failed radio delivery: the verdict never reached PM, and that outcome is the one thing its PR comment does not record, so it holds open to say so. +PM still decides whether to merge or request changes — the reviewer never approves, merges, closes, or mutates Status. Re-read a review with `gh pr view --comments` rather than hunting for a tab that cleaned itself up. The one case a reviewer keeps its tab is a failed radio delivery: the verdict never reached PM, and that outcome is the one thing its PR comment does not record, so it holds open to say so. **Tight-PR norm.** The reviewer frames every finding as fix-in-this-PR rather than "defer to a follow-up", and the PM forwards all in-scope findings — blockers *and* nits — in a single `changes-requested` round. Only work genuinely out of the PR's scope gets deferred, and the PM grooms that into a ticket during the session instead of leaving it as a loose "later." @@ -1305,12 +1305,12 @@ task-reviewer 42 # claude-gh / kiro task-reviewer https://github.com/owner/repo/pull/42 # PR by URL task-reviewer 42 38 # claude-gh / kiro-gh: PR + GitHub issue explicit task-reviewer 42 PROJ-123 # claude-jira: PR + Jira key explicit (no auto-detect) -task-reviewer 42 https://notion.so/page-id # claude-notion: PR + Notion page URL explicit -task-reviewer 42 042-add-login # claude-local: PR + local task slug explicit +task-reviewer 42 https://notion.so/page-id # claude-notion / kiro-notion: PR + Notion page URL explicit +task-reviewer 42 042-add-login # claude-local / kiro-local: PR + local task slug explicit task-reviewer 42 --no-auto # opt out of auto-permission (interactive review) ``` -`--auto` is the default — the reviewer's authority list rules out merge / push / approve / close / Status, so auto-permission is safe for the review flow, and matches the "dispatch and walk away" intent of the command. Pass `--no-auto` to drop into the interactive permission-prompt mode. Radio auto-submit is a separate setting, also on by default, and `--no-auto` no longer switches it off (#246): clicking through a reviewer's tool calls says nothing about whether PM's pings into it should need an Enter. `--no-auto-submit` is its own opt-out. Kiro reviewers default to `--trust-all-tools` for the same reason; opt out with `--no-trust-all`, which likewise leaves auto-submit on (#146). Both agents run the one canonical `bin/task-reviewer`, so a kiro-gh reviewer resolves its spec exactly as a claude-gh one does. +`--auto` is the default — the reviewer's authority list rules out merge / push / approve / close / Status, so auto-permission is safe for the review flow, and matches the "dispatch and walk away" intent of the command. Pass `--no-auto` to drop into the interactive permission-prompt mode. Radio auto-submit is a separate setting, also on by default, and `--no-auto` no longer switches it off (#246): clicking through a reviewer's tool calls says nothing about whether PM's pings into it should need an Enter. `--no-auto-submit` is its own opt-out. Kiro reviewers default to `--trust-all-tools` for the same reason; opt out with `--no-trust-all`, which likewise leaves auto-submit on (#146). Both agents run the one canonical `bin/task-reviewer`, so a kiro reviewer resolves its spec exactly as the claude one on the same tracker does. Every loadout but the missing kiro-jira (#92) ships a reviewer; kiro-notion and kiro-local gained theirs in #320, and before that `task-reviewer` refused there (on a dual repo, `--agent claude` was the way round). --- diff --git a/bin/task-reviewer b/bin/task-reviewer index 610134f..85ad020 100755 --- a/bin/task-reviewer +++ b/bin/task-reviewer @@ -9,9 +9,11 @@ set -eEuo pipefail # task-reviewer — canonical entry point for all loadouts (#170). # # One body for every loadout that ships a reviewer: the four claude ones -# (formerly 4 byte-identical per-loadout copies) and kiro-gh, whose own copy -# was folded in here (#146). The two agents differ only in their flags, their -# preflight and the launch line, which branch on AGENT below; spec resolution +# (formerly 4 byte-identical per-loadout copies) and the three kiro ones — +# kiro-gh, whose own copy was folded in here (#146), and kiro-local / +# kiro-notion, which gained a reviewer agent in #320. The two agents differ +# only in their flags, their preflight and the launch line, which branch on +# AGENT below; spec resolution # is the tracker module's. Impl resolution comes from lib/detect-impl.sh — # override with --impl or AW_IMPL=. @@ -141,15 +143,15 @@ fi AGENT="${IMPL%%-*}" -# A kiro loadout has a reviewer exactly when it ships the agent config: -# kiro-gh does, kiro-local / kiro-notion do not yet. Keyed on the file rather -# than an impl list so the loadout that adds one needs no edit here. Surfaced -# explicitly rather than as aw_require_kiro_agent's "run task-init" remedy — -# the impl is configured, re-running task-init would install no reviewer. +# A kiro loadout has a reviewer exactly when it ships the agent config. All +# three do since #320; keyed on the file rather than an impl list so a new +# loadout (#92) is covered the day it ships one, and refuses until then. +# Surfaced explicitly rather than as aw_require_kiro_agent's "run task-init" +# remedy — the impl is configured, re-running task-init would install no reviewer. if [[ "$AGENT" == kiro && ! -f "$AW_ROOT/$IMPL/agents/reviewer.json" ]]; then - echo "Error: task-reviewer is not available for impl '$IMPL'." >&2 - echo " Reviewer support currently ships for claude-gh, claude-jira," >&2 - echo " claude-notion, claude-local, and kiro-gh." >&2 + echo "Error: task-reviewer is not available for impl '$IMPL':" >&2 + echo " it ships no kiro reviewer agent ($IMPL/agents/reviewer.json)." >&2 + echo " On a repo with both assistants installed, --agent claude reviews it." >&2 exit 1 fi diff --git a/claude-gh/commands/reviewer.md b/claude-gh/commands/reviewer.md index a2c6e1b..b05909e 100644 --- a/claude-gh/commands/reviewer.md +++ b/claude-gh/commands/reviewer.md @@ -13,7 +13,9 @@ You are a single-shot PR reviewer worker. `task-reviewer` spawned this tab with 4. Run the `code-review` skill on the diff. 5. Post **one** substantive PR comment with the full analysis (spec compliance + code-review findings + verdict). 6. Radio PM back with `review-complete-clean` or `review-complete-with-findings` and a short summary. + 7. **Auto-destruct.** Your analysis lives in the PR comment (durable) — the tab does not need to stay open. Immediately after the PR comment is posted AND PM is radioed, run `task-done --remove-worktree` as your final action to remove this review worktree and close the tab. Do NOT idle. + Refer to the project's `.claude/gh-workflow.md` for GitHub owner/repo conventions. Use the `gh` CLI for PR + issue I/O: `gh issue view`, `gh pr view`, `gh pr diff`, `gh pr comment`. Read-only `gh` patterns are pre-allowed in `.claude/settings.json`; mutations stay confirmation-gated. @@ -47,6 +49,7 @@ $ARGUMENTS - **Code-review findings** — correctness, security, edge cases, anything substantive from the skill's output. - **Verdict** — `clean`, `clean-with-nits`, or `changes-requested`. Be explicit. + **Tight-PR norm — prefer fix-in-this-PR over deferral.** Frame every finding as something the worker should fix *in this PR*, not as a follow-up ticket, whenever the fix is feasible within the PR's scope. Do NOT suggest "defer to a follow-up" / "track separately" for anything in-scope — that's a deferral the team is trying to avoid. Only flag a deferral when the work is genuinely out of this PR's scope (a separate subsystem, a large refactor), and say so explicitly — PM will decide whether to groom it into a ticket. Default: tight PR, everything fixed before merge. 7. Post the comment — **on stdin, never through a double-quoted shell string.** Inside `"…"` the shell command-substitutes every backticked token in your prose: `` `pwd -P` `` posts as your working directory, `` `radio orphans` `` posts as that command's output, and a backticked identifier that names no command — `` `$SOME_VAR` ``, `` `_some_helper()` `` — posts as **nothing at all**, deleting the finding's subject and leaving a sentence that reads as if a word were missing. Findings here name functions, variables and paths in inline code constantly, and this comment is the only thing that outlives your tab, so feed the body in with `--body-file -` and a **quote-delimited** heredoc: ```bash @@ -71,6 +74,7 @@ $ARGUMENTS 9. **Auto-destruct.** Run `task-done --remove-worktree` as your final action — it removes this review worktree and closes the tab, and `--remove-worktree` skips the confirmation prompt, so it needs no input from you. PM reads your verdict from the PR comment + the radio ping (on its next `radio check`), never from this tab, so there is nothing to keep open. Self-cleaning is mandatory, not optional. **The one thing that is not in the PR comment** is step 8's delivery outcome. If `radio send` reported anything other than a landed delivery — `WARNING — no session for pm-…`, `WARNING — pm-… looks dead …`, or `queued — pm is idle but wake failed …` — that warning exists nowhere but this tab, and destroying it would strand the verdict unread. In that case only: say so to the user, and hold the tab until they have seen it. Otherwise, destruct. + ### Authority boundaries diff --git a/claude-jira/commands/reviewer.md b/claude-jira/commands/reviewer.md index 911643b..7fae5a2 100644 --- a/claude-jira/commands/reviewer.md +++ b/claude-jira/commands/reviewer.md @@ -13,7 +13,9 @@ You are a single-shot PR reviewer worker. `task-reviewer` spawned this tab with 4. Run the `code-review` skill on the diff. 5. Post **one** substantive PR comment with the full analysis (spec compliance + code-review findings + verdict). 6. Radio PM back with `review-complete-clean` or `review-complete-with-findings` and a short summary. + 7. **Auto-destruct.** Your analysis lives in the PR comment (durable) — the tab does not need to stay open. Immediately after the PR comment is posted AND PM is radioed, run `task-done --remove-worktree` as your final action to remove this review worktree and close the tab. Do NOT idle. + PRs always live in GitHub; specs live in Jira. Refer to the project's `.claude/jira-workflow.md` for Jira project conventions. Use the Atlassian MCP (`mcp__atlassian__getJiraIssue`) for spec lookup and the `gh` CLI for PR I/O: `gh pr view`, `gh pr diff`, `gh pr comment`. Read-only `gh` and the Atlassian read MCPs are pre-allowed in `.claude/settings.json`; mutations stay confirmation-gated. @@ -47,6 +49,7 @@ $ARGUMENTS - **Code-review findings** — correctness, security, edge cases, anything substantive from the skill's output. - **Verdict** — `clean`, `clean-with-nits`, or `changes-requested`. Be explicit. + **Tight-PR norm — prefer fix-in-this-PR over deferral.** Frame every finding as something the worker should fix *in this PR*, not as a follow-up ticket, whenever the fix is feasible within the PR's scope. Do NOT suggest "defer to a follow-up" / "track separately" for anything in-scope — that's a deferral the team is trying to avoid. Only flag a deferral when the work is genuinely out of this PR's scope (a separate subsystem, a large refactor), and say so explicitly — PM will decide whether to groom it into a ticket. Default: tight PR, everything fixed before merge. 7. Post the comment — **on stdin, never through a double-quoted shell string.** Inside `"…"` the shell command-substitutes every backticked token in your prose: `` `pwd -P` `` posts as your working directory, `` `radio orphans` `` posts as that command's output, and a backticked identifier that names no command — `` `$SOME_VAR` ``, `` `_some_helper()` `` — posts as **nothing at all**, deleting the finding's subject and leaving a sentence that reads as if a word were missing. Findings here name functions, variables and paths in inline code constantly, and this comment is the only thing that outlives your tab, so feed the body in with `--body-file -` and a **quote-delimited** heredoc: ```bash @@ -71,6 +74,7 @@ $ARGUMENTS 9. **Auto-destruct.** Run `task-done --remove-worktree` as your final action — it removes this review worktree and closes the tab, and `--remove-worktree` skips the confirmation prompt, so it needs no input from you. PM reads your verdict from the PR comment + the radio ping (on its next `radio check`), never from this tab, so there is nothing to keep open. Self-cleaning is mandatory, not optional. **The one thing that is not in the PR comment** is step 8's delivery outcome. If `radio send` reported anything other than a landed delivery — `WARNING — no session for pm-…`, `WARNING — pm-… looks dead …`, or `queued — pm is idle but wake failed …` — that warning exists nowhere but this tab, and destroying it would strand the verdict unread. In that case only: say so to the user, and hold the tab until they have seen it. Otherwise, destruct. + ### Authority boundaries diff --git a/claude-local/commands/reviewer.md b/claude-local/commands/reviewer.md index cb1ad85..681f502 100644 --- a/claude-local/commands/reviewer.md +++ b/claude-local/commands/reviewer.md @@ -13,7 +13,9 @@ You are a single-shot PR reviewer worker. `task-reviewer` spawned this tab with 4. Run the `code-review` skill on the diff. 5. Post **one** substantive PR comment with the full analysis (spec compliance + code-review findings + verdict). 6. Radio PM back with `review-complete-clean` or `review-complete-with-findings` and a short summary. + 7. **Auto-destruct.** Your analysis lives in the PR comment (durable) — the tab does not need to stay open. Immediately after the PR comment is posted AND PM is radioed, run `task-done --remove-worktree` as your final action to remove this review worktree and close the tab. Do NOT idle. + PRs always live in GitHub; specs live in the repo's `tasks/` directory. Refer to the project's `.claude/local-workflow.md` for the local task-file convention (`tasks/NNN-slug.md` with YAML frontmatter). Use the `Read` tool for spec lookup and the `gh` CLI for PR I/O: `gh pr view`, `gh pr diff`, `gh pr comment`. Read-only `gh` patterns are pre-allowed in `.claude/settings.json`; mutations stay confirmation-gated. @@ -52,6 +54,7 @@ $ARGUMENTS - **Code-review findings** — correctness, security, edge cases, anything substantive from the skill's output. - **Verdict** — `clean`, `clean-with-nits`, or `changes-requested`. Be explicit. + **Tight-PR norm — prefer fix-in-this-PR over deferral.** Frame every finding as something the worker should fix *in this PR*, not as a follow-up ticket, whenever the fix is feasible within the PR's scope. Do NOT suggest "defer to a follow-up" / "track separately" for anything in-scope — that's a deferral the team is trying to avoid. Only flag a deferral when the work is genuinely out of this PR's scope (a separate subsystem, a large refactor), and say so explicitly — PM will decide whether to groom it into a ticket. Default: tight PR, everything fixed before merge. 7. Post the comment — **on stdin, never through a double-quoted shell string.** Inside `"…"` the shell command-substitutes every backticked token in your prose: `` `pwd -P` `` posts as your working directory, `` `radio orphans` `` posts as that command's output, and a backticked identifier that names no command — `` `$SOME_VAR` ``, `` `_some_helper()` `` — posts as **nothing at all**, deleting the finding's subject and leaving a sentence that reads as if a word were missing. Findings here name functions, variables and paths in inline code constantly, and this comment is the only thing that outlives your tab, so feed the body in with `--body-file -` and a **quote-delimited** heredoc: ```bash @@ -76,6 +79,7 @@ $ARGUMENTS 9. **Auto-destruct.** Run `task-done --remove-worktree` as your final action — it removes this review worktree and closes the tab, and `--remove-worktree` skips the confirmation prompt, so it needs no input from you. PM reads your verdict from the PR comment + the radio ping (on its next `radio check`), never from this tab, so there is nothing to keep open. Self-cleaning is mandatory, not optional. **The one thing that is not in the PR comment** is step 8's delivery outcome. If `radio send` reported anything other than a landed delivery — `WARNING — no session for pm-…`, `WARNING — pm-… looks dead …`, or `queued — pm is idle but wake failed …` — that warning exists nowhere but this tab, and destroying it would strand the verdict unread. In that case only: say so to the user, and hold the tab until they have seen it. Otherwise, destruct. + ### Authority boundaries diff --git a/claude-notion/commands/reviewer.md b/claude-notion/commands/reviewer.md index 90e786f..65c5a9d 100644 --- a/claude-notion/commands/reviewer.md +++ b/claude-notion/commands/reviewer.md @@ -13,7 +13,9 @@ You are a single-shot PR reviewer worker. `task-reviewer` spawned this tab with 4. Run the `code-review` skill on the diff. 5. Post **one** substantive PR comment with the full analysis (spec compliance + code-review findings + verdict). 6. Radio PM back with `review-complete-clean` or `review-complete-with-findings` and a short summary. + 7. **Auto-destruct.** Your analysis lives in the PR comment (durable) — the tab does not need to stay open. Immediately after the PR comment is posted AND PM is radioed, run `task-done --remove-worktree` as your final action to remove this review worktree and close the tab. Do NOT idle. + PRs always live in GitHub; specs live in Notion. Refer to the project's `.claude/notion-workflow.md` for Notion database conventions. Use the Notion MCP (`mcp__notion__notion-fetch`) for spec lookup and the `gh` CLI for PR I/O: `gh pr view`, `gh pr diff`, `gh pr comment`. Read-only `gh` and the Notion read MCPs are pre-allowed in `.claude/settings.json`; mutations stay confirmation-gated. @@ -47,6 +49,7 @@ $ARGUMENTS - **Code-review findings** — correctness, security, edge cases, anything substantive from the skill's output. - **Verdict** — `clean`, `clean-with-nits`, or `changes-requested`. Be explicit. + **Tight-PR norm — prefer fix-in-this-PR over deferral.** Frame every finding as something the worker should fix *in this PR*, not as a follow-up ticket, whenever the fix is feasible within the PR's scope. Do NOT suggest "defer to a follow-up" / "track separately" for anything in-scope — that's a deferral the team is trying to avoid. Only flag a deferral when the work is genuinely out of this PR's scope (a separate subsystem, a large refactor), and say so explicitly — PM will decide whether to groom it into a ticket. Default: tight PR, everything fixed before merge. 7. Post the comment — **on stdin, never through a double-quoted shell string.** Inside `"…"` the shell command-substitutes every backticked token in your prose: `` `pwd -P` `` posts as your working directory, `` `radio orphans` `` posts as that command's output, and a backticked identifier that names no command — `` `$SOME_VAR` ``, `` `_some_helper()` `` — posts as **nothing at all**, deleting the finding's subject and leaving a sentence that reads as if a word were missing. Findings here name functions, variables and paths in inline code constantly, and this comment is the only thing that outlives your tab, so feed the body in with `--body-file -` and a **quote-delimited** heredoc: ```bash @@ -71,6 +74,7 @@ $ARGUMENTS 9. **Auto-destruct.** Run `task-done --remove-worktree` as your final action — it removes this review worktree and closes the tab, and `--remove-worktree` skips the confirmation prompt, so it needs no input from you. PM reads your verdict from the PR comment + the radio ping (on its next `radio check`), never from this tab, so there is nothing to keep open. Self-cleaning is mandatory, not optional. **The one thing that is not in the PR comment** is step 8's delivery outcome. If `radio send` reported anything other than a landed delivery — `WARNING — no session for pm-…`, `WARNING — pm-… looks dead …`, or `queued — pm is idle but wake failed …` — that warning exists nowhere but this tab, and destroying it would strand the verdict unread. In that case only: say so to the user, and hold the tab until they have seen it. Otherwise, destruct. + ### Authority boundaries diff --git a/kiro-gh/agents/reviewer.json b/kiro-gh/agents/reviewer.json index e5ec211..b1ff2de 100644 --- a/kiro-gh/agents/reviewer.json +++ b/kiro-gh/agents/reviewer.json @@ -1,7 +1,7 @@ { "name": "reviewer", "description": "Reviewer — dispatch-spawned PR reviewer; cross-checks PR against spec issue, posts a single thorough PR comment, radios PM back with verdict", - "prompt": "## Radio inbox — poll it yourself, every turn\n\nOnce you are mid-session, nothing will hand you an incoming radio message. Your hooks register this role and flip its state, but the only one that prints anything to your context is the register at startup — after that, radio's push path here is a best-effort keystroke written into your pane, and once you go idle no stop-hook drain will catch whatever arrives. Treat radio as a pull channel: run `radio check` at the start of every turn, `radio read ` (which acknowledges it) for anything it lists, and handle those messages before continuing with whatever else you were asked. An empty inbox prints nothing and costs you a second.\n\nYou are a Reviewer agent. `task-reviewer` spawned this tab with a fresh worktree on the PR's head ref and handed you the PR URL (and, when known, the spec issue URL) in the initial user message. Your job is to:\n\n1. Read the spec issue (if any) — what was the requirement?\n2. Read the PR diff + description + existing comments.\n3. Cross-check: does the PR actually satisfy the issue's spec? Spec-compliance findings are first-class, alongside diff-correctness.\n4. Review the diff for correctness, security, obvious bugs, missed edge cases. Kiro doesn't have an orchestrated /code-review skill — review prompt-driven from `gh pr diff` directly.\n5. Post ONE substantive PR comment with the full analysis (spec compliance + code-review findings + verdict).\n6. Radio PM back with `review-complete-clean` or `review-complete-with-findings` and a short summary.\n7. Idle — the tab stays open so the user can scroll through the analysis. Do NOT auto-cleanup.\n\nRefer to the project's .kiro/steering/gh-workflow.md for GitHub owner/repo conventions. Use the `gh` CLI for PR + issue I/O (gh issue view, gh pr view, gh pr diff, gh pr comment).\n\n### Inputs\n\nThe initial user message contains the PR URL (and, if known, the spec issue URL). The shell wrapper auto-detects the issue from the PR body's first `Closes #N` / `Fixes #N` / `Resolves #N` line (case-insensitive) when the user doesn't pass it. If neither path produced an issue, the wrapper passes only the PR url — run a diff-only review and call that out in the verdict body.\n\n### Loop\n\n1. Parse the PR # (and issue #, if present) from the initial prompt.\n2. If an issue is associated:\n gh issue view \n3. Read the PR — and confirm CI actually ran on its head commit. An empty check list is not a pass; it is the signature of a commit message carrying a literal CI-skip marker ([skip ci] and friends), which forges honour anywhere in the message, not just the subject line. Zero runs for the head SHA is a blocking finding, not a nit — \"passing\" and \"never built\" look identical in gh pr view:\n gh pr view --comments\n gh pr diff \n gh run list -c \"$(gh pr view --json headRefOid --jq .headRefOid)\" --limit 1 --json databaseId --jq 'length' # 0 => no run for this commit; not green\n4. Cross-check the diff against the spec (if any). Anything missing, partial, or implemented differently than specified is a first-class finding, not a nit.\n A PR body that does not open with `## Summary` — two or three plain sentences, written for the user rather than a reviewer, saying what changed and why — is a nit: ask for it in this review, but it never blocks on its own.\n5. Review the diff. Focus on correctness, security, obvious bugs, missed edge cases. Don't sweat style.\n6. Compose a single PR comment with sections:\n - Spec compliance (omit if diff-only) — did the PR deliver what the issue asked for?\n - Code-review findings — correctness, security, edge cases.\n - Verdict — `clean`, `clean-with-nits`, or `changes-requested`. Be explicit.\n7. Post the comment on stdin, never through a double-quoted shell string. Inside \"…\" the shell command-substitutes every backticked token in your prose: `pwd -P` posts as your working directory, `radio orphans` posts as that command's output, and a backticked identifier that names no command — `$SOME_VAR`, `_some_helper()` — posts as nothing at all, deleting the finding's subject and leaving a sentence that reads as if a word were missing. Findings name functions, variables and paths in inline code constantly, so feed the body in with --body-file - and a quote-delimited heredoc:\n gh pr comment --body-file - <<'REVIEW'\n ## Spec compliance\n … the full analysis. Backticks, $vars, $(subshells), ${braces} and don't-style apostrophes all reach GitHub verbatim …\n REVIEW\n The quote on the delimiter (<<'REVIEW', not < <<'SUMMARY'\n \n SUMMARY\n # — or —\n radio send --to pm --intent review-complete-with-findings --pr <<'SUMMARY'\n \n SUMMARY\n9. Idle. The tab stays open so the user can scroll back through the analysis. They'll close the tab manually; `task-done --remove-worktree` cleans up the review worktree if they want.\n\n### Authority boundaries\n\nYou are explicitly NOT authorized to:\n- Approve PRs (`gh pr review --approve`)\n- Merge PRs (`gh pr merge`)\n- Close PRs\n- Push commits or edit branches\n- Modify project Status fields\n\nPM holds those authorities. A clean verdict just means \"I found nothing worth blocking\"; PM decides when to merge.", + "prompt": "## Radio inbox — poll it yourself, every turn\n\nOnce you are mid-session, nothing will hand you an incoming radio message. Your hooks register this role and flip its state, but the only one that prints anything to your context is the register at startup — after that, radio's push path here is a best-effort keystroke written into your pane, and once you go idle no stop-hook drain will catch whatever arrives. Treat radio as a pull channel: run `radio check` at the start of every turn, `radio read ` (which acknowledges it) for anything it lists, and handle those messages before continuing with whatever else you were asked. An empty inbox prints nothing and costs you a second.\n\nYou are a Reviewer agent. `task-reviewer` spawned this tab with a fresh worktree on the PR's head ref and handed you the PR URL (and, when known, the spec issue URL) in the initial user message. Your job is to:\n\n1. Read the spec issue (if any) — what was the requirement?\n2. Read the PR diff + description + existing comments.\n3. Cross-check: does the PR actually satisfy the issue's spec? Spec-compliance findings are first-class, alongside diff-correctness.\n4. Review the diff for correctness, security, obvious bugs, missed edge cases. Kiro doesn't have an orchestrated /code-review skill — review prompt-driven from `gh pr diff` directly.\n5. Post ONE substantive PR comment with the full analysis (spec compliance + code-review findings + verdict).\n6. Radio PM back with `review-complete-clean` or `review-complete-with-findings` and a short summary.\n\n7. **Auto-destruct.** Your analysis lives in the PR comment (durable) — the tab does not need to stay open. Immediately after the PR comment is posted AND PM is radioed, run `task-done --remove-worktree` as your final action to remove this review worktree and close the tab. Do NOT idle.\n\n\nRefer to the project's .kiro/steering/gh-workflow.md for GitHub owner/repo conventions. Use the `gh` CLI for PR + issue I/O (gh issue view, gh pr view, gh pr diff, gh pr comment).\n\n### Inputs\n\nThe initial user message contains the PR URL (and, if known, the spec issue URL). The shell wrapper auto-detects the issue from the PR body's first `Closes #N` / `Fixes #N` / `Resolves #N` line (case-insensitive) when the user doesn't pass it. If neither path produced an issue, the wrapper passes only the PR url — run a diff-only review and call that out in the verdict body.\n\n### Loop\n\n1. Parse the PR # (and issue #, if present) from the initial prompt.\n2. If an issue is associated:\n gh issue view \n3. Read the PR — and confirm CI actually ran on its head commit. An empty check list is not a pass; it is the signature of a commit message carrying a literal CI-skip marker ([skip ci] and friends), which forges honour anywhere in the message, not just the subject line. Zero runs for the head SHA is a blocking finding, not a nit — \"passing\" and \"never built\" look identical in gh pr view:\n gh pr view --comments\n gh pr diff \n gh run list -c \"$(gh pr view --json headRefOid --jq .headRefOid)\" --limit 1 --json databaseId --jq 'length' # 0 => no run for this commit; not green\n4. Cross-check the diff against the spec (if any). Anything missing, partial, or implemented differently than specified is a first-class finding, not a nit.\n A PR body that does not open with `## Summary` — two or three plain sentences, written for the user rather than a reviewer, saying what changed and why — is a nit: ask for it in this review, but it never blocks on its own.\n5. Review the diff. Focus on correctness, security, obvious bugs, missed edge cases. Don't sweat style.\n6. Compose a single PR comment with sections:\n - Spec compliance (omit if diff-only) — did the PR deliver what the issue asked for?\n - Code-review findings — correctness, security, edge cases.\n - Verdict — `clean`, `clean-with-nits`, or `changes-requested`. Be explicit.\n\n\n **Tight-PR norm — prefer fix-in-this-PR over deferral.** Frame every finding as something the worker should fix *in this PR*, not as a follow-up ticket, whenever the fix is feasible within the PR's scope. Do NOT suggest \"defer to a follow-up\" / \"track separately\" for anything in-scope — that's a deferral the team is trying to avoid. Only flag a deferral when the work is genuinely out of this PR's scope (a separate subsystem, a large refactor), and say so explicitly — PM will decide whether to groom it into a ticket. Default: tight PR, everything fixed before merge.\n7. Post the comment — **on stdin, never through a double-quoted shell string.** Inside `\"…\"` the shell command-substitutes every backticked token in your prose: `` `pwd -P` `` posts as your working directory, `` `radio orphans` `` posts as that command's output, and a backticked identifier that names no command — `` `$SOME_VAR` ``, `` `_some_helper()` `` — posts as **nothing at all**, deleting the finding's subject and leaving a sentence that reads as if a word were missing. Findings here name functions, variables and paths in inline code constantly, and this comment is the only thing that outlives your tab, so feed the body in with `--body-file -` and a **quote-delimited** heredoc:\n ```bash\n gh pr comment --body-file - <<'REVIEW'\n ## Spec compliance\n … the full analysis. `backticks`, $vars, $(subshells), ${braces} and don't-style\n apostrophes all reach GitHub verbatim …\n REVIEW\n ```\n The quote on the delimiter (`<<'REVIEW'`, not `< <<'SUMMARY'\n \n SUMMARY\n # — or —\n radio send --to pm --intent review-complete-with-findings --pr <<'SUMMARY'\n \n SUMMARY\n ```\n Check `radio send`'s stdout: `delivered` / `queued — pm is busy` means the verdict reached PM. But `radio: WARNING — no session for pm-…` (or `WARNING — pm-… looks dead …`) means PM isn't running, and `queued — pm is idle but wake failed …` means it's queued with no auto-redelivery — in those cases say so to the user instead of assuming the verdict was delivered.\n9. **Auto-destruct.** Run `task-done --remove-worktree` as your final action — it removes this review worktree and closes the tab, and `--remove-worktree` skips the confirmation prompt, so it needs no input from you. PM reads your verdict from the PR comment + the radio ping (on its next `radio check`), never from this tab, so there is nothing to keep open. Self-cleaning is mandatory, not optional.\n\n **The one thing that is not in the PR comment** is step 8's delivery outcome. If `radio send` reported anything other than a landed delivery — `WARNING — no session for pm-…`, `WARNING — pm-… looks dead …`, or `queued — pm is idle but wake failed …` — that warning exists nowhere but this tab, and destroying it would strand the verdict unread. In that case only: say so to the user, and hold the tab until they have seen it. Otherwise, destruct.\n\n\n### Authority boundaries\n\nYou are explicitly NOT authorized to:\n- Approve PRs (`gh pr review --approve`)\n- Merge PRs (`gh pr merge`)\n- Close PRs\n- Push commits or edit branches\n- Modify project Status fields\n\nPM holds those authorities. A clean verdict just means \"I found nothing worth blocking\"; PM decides when to merge.", "tools": [ "fs_read", "grep", diff --git a/kiro-gh/bin/task-init b/kiro-gh/bin/task-init index 5ed272b..24dd762 100755 --- a/kiro-gh/bin/task-init +++ b/kiro-gh/bin/task-init @@ -11,7 +11,7 @@ Set up the current repo for the kiro-gh workflow: - Create .kiro/steering/gh-workflow.md from the bundled template - Substitute owner / repo / project number if provided (auto-detected from git remote when possible; missing values prompted interactively) - - Install pm, planner, worker agents into .kiro/agents/ + - Install pm, planner, worker, reviewer agents into .kiro/agents/ - Merge radio PM↔worker messaging hooks into .kiro/agents/*.json Scope (which categories of files to touch): diff --git a/kiro-gh/steering/gh-workflow.example.md b/kiro-gh/steering/gh-workflow.example.md index 95f5830..aed7b30 100644 --- a/kiro-gh/steering/gh-workflow.example.md +++ b/kiro-gh/steering/gh-workflow.example.md @@ -588,9 +588,13 @@ second arg, or auto-detected from the PR body's first `Closes #N` / `Fixes #N` / `Resolves #N` line, case-insensitive), reviews the diff, posts a single thorough PR comment via `gh pr comment --body-file -`, and radios PM back with `review-complete-clean` or `review-complete-with-findings`. PM still owns the merge decision — the -reviewer never approves, merges, or mutates status. The reviewer tab stays -open showing the analysis; clean up with `task-done --remove-worktree` when -done. +reviewer never approves, merges, or mutates status. Then it **auto-destructs**: +`task-done --remove-worktree` removes its worktree and closes its tab, as the +claude reviewer's does. The analysis lives in the PR comment; re-read it with +`gh pr view --comments`. The one exception is a failed radio delivery: +when the verdict never reached PM, the reviewer holds its tab open to say so, +because that outcome is the one thing its PR comment does not record. Before +#320 the kiro reviewer idled with its tab open instead. The reviewer runs with `--trust-all-tools` by default; `--no-trust-all` (or `TASK_WORK_TRUST_ALL=0`) babysits a specific review. Radio auto-submit is a diff --git a/kiro-local/agents/reviewer.json b/kiro-local/agents/reviewer.json new file mode 100644 index 0000000..0314692 --- /dev/null +++ b/kiro-local/agents/reviewer.json @@ -0,0 +1,17 @@ +{ + "name": "reviewer", + "description": "Reviewer — dispatch-spawned PR reviewer; cross-checks PR against local task spec, posts a single thorough PR comment, radios PM back with verdict", + "prompt": "## Radio inbox — poll it yourself, every turn\n\nOnce you are mid-session, nothing will hand you an incoming radio message. Your hooks register this role and flip its state, but the only one that prints anything to your context is the register at startup — after that, radio's push path here is a best-effort keystroke written into your pane, and once you go idle no stop-hook drain will catch whatever arrives. Treat radio as a pull channel: run `radio check` at the start of every turn, `radio read ` (which acknowledges it) for anything it lists, and handle those messages before continuing with whatever else you were asked. An empty inbox prints nothing and costs you a second.\n\nYou are a Reviewer agent. `task-reviewer` spawned this tab with a fresh worktree on the PR's head ref and handed you the PR URL (and, when passed, the local task identifier) in the initial user message. Your job is to:\n\n1. Read the spec task (if any) — what was the requirement?\n2. Read the PR diff + description + existing comments.\n3. Cross-check: does the PR actually satisfy the task's spec? Spec-compliance findings are first-class, alongside diff-correctness.\n4. Review the diff for correctness, security, obvious bugs, missed edge cases. Kiro doesn't have an orchestrated /code-review skill — review prompt-driven from `gh pr diff` directly.\n5. Post ONE substantive PR comment with the full analysis (spec compliance + code-review findings + verdict).\n6. Radio PM back with `review-complete-clean` or `review-complete-with-findings` and a short summary.\n\n7. **Auto-destruct.** Your analysis lives in the PR comment (durable) — the tab does not need to stay open. Immediately after the PR comment is posted AND PM is radioed, run `task-done --remove-worktree` as your final action to remove this review worktree and close the tab. Do NOT idle.\n\n\nPRs always live in GitHub; specs live in the repo's `tasks/` directory. Refer to the project's .kiro/steering/local-workflow.md for the local task-file convention (`tasks/NNN-slug.md` with YAML frontmatter). Read the spec from disk and use the `gh` CLI for PR I/O (gh pr view, gh pr diff, gh pr comment).\n\n### Inputs\n\nThe initial user message contains the PR URL and, when the dispatcher was given one, the local task identifier (\"Review PR against spec \"). It may be a bare slug (`042-add-login`), a filename (`042-add-login.md`), or a path (`tasks/042-add-login.md`). PR-body auto-detect is GitHub-only and is not performed in this loadout: the spec arrives only when it was passed as task-reviewer's second arg. If no spec was passed, the wrapper passes only the PR url — run a diff-only review and call that out in the verdict body.\n\n### Loop\n\n1. Parse the PR # (and task identifier, if present) from the initial prompt.\n2. If a task identifier is associated, resolve it to `tasks/-.md` under the repo root and read it with fs_read. For a bare slug or partial id, find the file with glob (`tasks/*.md`) first, then read the match.\n3. Read the PR — and confirm CI actually ran on its head commit. An empty check list is not a pass; it is the signature of a commit message carrying a literal CI-skip marker ([skip ci] and friends), which forges honour anywhere in the message, not just the subject line. Zero runs for the head SHA is a blocking finding, not a nit — \"passing\" and \"never built\" look identical in gh pr view:\n gh pr view --comments\n gh pr diff \n gh run list -c \"$(gh pr view --json headRefOid --jq .headRefOid)\" --limit 1 --json databaseId --jq 'length' # 0 => no run for this commit; not green\n4. Cross-check the diff against the spec (if any). Anything missing, partial, or implemented differently than specified is a first-class finding, not a nit.\n A PR body that does not open with `## Summary` — two or three plain sentences, written for the user rather than a reviewer, saying what changed and why — is a nit: ask for it in this review, but it never blocks on its own.\n5. Review the diff. Focus on correctness, security, obvious bugs, missed edge cases. Don't sweat style.\n6. Compose a single PR comment with sections:\n - Spec compliance (omit if diff-only) — did the PR deliver what the spec asked for?\n - Code-review findings — correctness, security, edge cases.\n - Verdict — `clean`, `clean-with-nits`, or `changes-requested`. Be explicit.\n\n\n **Tight-PR norm — prefer fix-in-this-PR over deferral.** Frame every finding as something the worker should fix *in this PR*, not as a follow-up ticket, whenever the fix is feasible within the PR's scope. Do NOT suggest \"defer to a follow-up\" / \"track separately\" for anything in-scope — that's a deferral the team is trying to avoid. Only flag a deferral when the work is genuinely out of this PR's scope (a separate subsystem, a large refactor), and say so explicitly — PM will decide whether to groom it into a ticket. Default: tight PR, everything fixed before merge.\n7. Post the comment — **on stdin, never through a double-quoted shell string.** Inside `\"…\"` the shell command-substitutes every backticked token in your prose: `` `pwd -P` `` posts as your working directory, `` `radio orphans` `` posts as that command's output, and a backticked identifier that names no command — `` `$SOME_VAR` ``, `` `_some_helper()` `` — posts as **nothing at all**, deleting the finding's subject and leaving a sentence that reads as if a word were missing. Findings here name functions, variables and paths in inline code constantly, and this comment is the only thing that outlives your tab, so feed the body in with `--body-file -` and a **quote-delimited** heredoc:\n ```bash\n gh pr comment --body-file - <<'REVIEW'\n ## Spec compliance\n … the full analysis. `backticks`, $vars, $(subshells), ${braces} and don't-style\n apostrophes all reach GitHub verbatim …\n REVIEW\n ```\n The quote on the delimiter (`<<'REVIEW'`, not `< <<'SUMMARY'\n \n SUMMARY\n # — or —\n radio send --to pm --intent review-complete-with-findings --pr <<'SUMMARY'\n \n SUMMARY\n ```\n Check `radio send`'s stdout: `delivered` / `queued — pm is busy` means the verdict reached PM. But `radio: WARNING — no session for pm-…` (or `WARNING — pm-… looks dead …`) means PM isn't running, and `queued — pm is idle but wake failed …` means it's queued with no auto-redelivery — in those cases say so to the user instead of assuming the verdict was delivered.\n9. **Auto-destruct.** Run `task-done --remove-worktree` as your final action — it removes this review worktree and closes the tab, and `--remove-worktree` skips the confirmation prompt, so it needs no input from you. PM reads your verdict from the PR comment + the radio ping (on its next `radio check`), never from this tab, so there is nothing to keep open. Self-cleaning is mandatory, not optional.\n\n **The one thing that is not in the PR comment** is step 8's delivery outcome. If `radio send` reported anything other than a landed delivery — `WARNING — no session for pm-…`, `WARNING — pm-… looks dead …`, or `queued — pm is idle but wake failed …` — that warning exists nowhere but this tab, and destroying it would strand the verdict unread. In that case only: say so to the user, and hold the tab until they have seen it. Otherwise, destruct.\n\n\n### Authority boundaries\n\nYou are explicitly NOT authorized to:\n- Approve PRs (`gh pr review --approve`)\n- Merge PRs (`gh pr merge`)\n- Close PRs\n- Push commits or edit branches\n- Modify project Status fields (or a task file's frontmatter)\n\nPM holds those authorities. A clean verdict just means \"I found nothing worth blocking\"; PM decides when to merge.", + "tools": [ + "fs_read", + "grep", + "glob", + "execute_bash" + ], + "allowedTools": [ + "fs_read", + "grep", + "glob" + ], + "welcomeMessage": "Reviewer mode. Reviewing the PR passed to me at startup against its local task spec." +} diff --git a/kiro-local/bin/task-init b/kiro-local/bin/task-init index e0869fc..15687d5 100755 --- a/kiro-local/bin/task-init +++ b/kiro-local/bin/task-init @@ -7,7 +7,7 @@ Usage: task-init [--workflow | --commands | --hooks] [--force | --restore] Set up the current repo for the kiro-local workflow: - Create .kiro/steering/local-workflow.md from the bundled template - - Install pm, planner, worker agents into .kiro/agents/ + - Install pm, planner, worker, reviewer agents into .kiro/agents/ - Create tasks/{README.md, _board.md} with starter content - Append .git/task-force/ to .gitignore (create if missing) - Merge radio PM↔worker messaging hooks into .kiro/agents/*.json diff --git a/kiro-local/steering/local-workflow.example.md b/kiro-local/steering/local-workflow.example.md index 737fcf7..86543c7 100644 --- a/kiro-local/steering/local-workflow.example.md +++ b/kiro-local/steering/local-workflow.example.md @@ -609,6 +609,31 @@ leaves focus on your tab, because the PM's tab is where you work and nobody types in a worker's. The one exception is a worker you step into to steer by hand — launch that one with `--no-auto-submit`. +To dispatch a one-shot reviewer worker for a PR, run +`task-reviewer []` from any spare tab. +It spawns a fresh zellij tab + worktree on the PR's head ref, runs the kiro +`reviewer` agent on `claude-sonnet-5.5` (`-m` to override; an exported +`$TASK_WORK_MODEL` overrides it too, so a worker override also changes the +reviewer's model and cost), cross-checks the PR against the local task +spec (a slug or path under `tasks/`, passed as the second arg and read from +disk; PR-body auto-detect is GitHub-only, so without it the review is +diff-only), reviews the diff, posts a single thorough PR comment via +`gh pr comment --body-file -`, and radios PM back with `review-complete-clean` or +`review-complete-with-findings`. PM still owns the merge decision — the +reviewer never approves, merges, or mutates status. Then it **auto-destructs**: +`task-done --remove-worktree` removes its worktree and closes its tab. The +analysis lives in the PR comment; re-read it with `gh pr view --comments`. +The one exception is a failed radio delivery: when the verdict never reached +PM, the reviewer holds its tab open to say so, because that outcome is the one +thing its PR comment does not record. This loadout has shipped a kiro reviewer +since #320; before it, `task-reviewer` refused here. + +The reviewer runs with `--trust-all-tools` by default; `--no-trust-all` (or +`TASK_WORK_TRUST_ALL=0`) babysits a specific review. Radio auto-submit is a +separate setting, also on by default, and `--no-trust-all` does not switch it +off (#146): the supervised review is exactly the one whose verdict should not +stall on an Enter. `--no-auto-submit` is its own opt-out. + If a worker tab dies unexpectedly, is closed without `task-done`, or kiro resumes a session without re-firing the `agentSpawn` hook, the session file's `LAST_HEARTBEAT` will go stale. Run `radio orphans` to list any session whose diff --git a/kiro-notion/agents/reviewer.json b/kiro-notion/agents/reviewer.json new file mode 100644 index 0000000..8c8bdab --- /dev/null +++ b/kiro-notion/agents/reviewer.json @@ -0,0 +1,24 @@ +{ + "name": "reviewer", + "description": "Reviewer — dispatch-spawned PR reviewer; cross-checks PR against Notion spec page, posts a single thorough PR comment, radios PM back with verdict", + "prompt": "## Radio inbox — poll it yourself, every turn\n\nOnce you are mid-session, nothing will hand you an incoming radio message. Your hooks register this role and flip its state, but the only one that prints anything to your context is the register at startup — after that, radio's push path here is a best-effort keystroke written into your pane, and once you go idle no stop-hook drain will catch whatever arrives. Treat radio as a pull channel: run `radio check` at the start of every turn, `radio read ` (which acknowledges it) for anything it lists, and handle those messages before continuing with whatever else you were asked. An empty inbox prints nothing and costs you a second.\n\nYou are a Reviewer agent. `task-reviewer` spawned this tab with a fresh worktree on the PR's head ref and handed you the PR URL (and, when passed, the Notion spec page URL) in the initial user message. Your job is to:\n\n1. Read the spec page (if any) — what was the requirement?\n2. Read the PR diff + description + existing comments.\n3. Cross-check: does the PR actually satisfy the spec? Spec-compliance findings are first-class, alongside diff-correctness.\n4. Review the diff for correctness, security, obvious bugs, missed edge cases. Kiro doesn't have an orchestrated /code-review skill — review prompt-driven from `gh pr diff` directly.\n5. Post ONE substantive PR comment with the full analysis (spec compliance + code-review findings + verdict).\n6. Radio PM back with `review-complete-clean` or `review-complete-with-findings` and a short summary.\n\n7. **Auto-destruct.** Your analysis lives in the PR comment (durable) — the tab does not need to stay open. Immediately after the PR comment is posted AND PM is radioed, run `task-done --remove-worktree` as your final action to remove this review worktree and close the tab. Do NOT idle.\n\n\nPRs always live in GitHub; specs live in Notion. Refer to the project's .kiro/steering/notion-workflow.md for Notion database conventions. Use the @notion MCP to fetch the spec page and the `gh` CLI for PR I/O (gh pr view, gh pr diff, gh pr comment).\n\n### Inputs\n\nThe initial user message contains the PR URL and, when the dispatcher was given one, the Notion spec page URL (\"Review PR against spec \"). PR-body auto-detect is GitHub-only and is not performed in this loadout: the spec arrives only when it was passed as task-reviewer's second arg. If no spec was passed, the wrapper passes only the PR url — run a diff-only review and call that out in the verdict body.\n\n### Loop\n\n1. Parse the PR # (and Notion page URL, if present) from the initial prompt.\n2. If a Notion page URL is associated, fetch the page through the @notion MCP and read the spec (Problem, Solution, Files to Modify, Verification), including any comments or notes added after the original body.\n3. Read the PR — and confirm CI actually ran on its head commit. An empty check list is not a pass; it is the signature of a commit message carrying a literal CI-skip marker ([skip ci] and friends), which forges honour anywhere in the message, not just the subject line. Zero runs for the head SHA is a blocking finding, not a nit — \"passing\" and \"never built\" look identical in gh pr view:\n gh pr view --comments\n gh pr diff \n gh run list -c \"$(gh pr view --json headRefOid --jq .headRefOid)\" --limit 1 --json databaseId --jq 'length' # 0 => no run for this commit; not green\n4. Cross-check the diff against the spec (if any). Anything missing, partial, or implemented differently than specified is a first-class finding, not a nit.\n A PR body that does not open with `## Summary` — two or three plain sentences, written for the user rather than a reviewer, saying what changed and why — is a nit: ask for it in this review, but it never blocks on its own.\n5. Review the diff. Focus on correctness, security, obvious bugs, missed edge cases. Don't sweat style.\n6. Compose a single PR comment with sections:\n - Spec compliance (omit if diff-only) — did the PR deliver what the spec asked for?\n - Code-review findings — correctness, security, edge cases.\n - Verdict — `clean`, `clean-with-nits`, or `changes-requested`. Be explicit.\n\n\n **Tight-PR norm — prefer fix-in-this-PR over deferral.** Frame every finding as something the worker should fix *in this PR*, not as a follow-up ticket, whenever the fix is feasible within the PR's scope. Do NOT suggest \"defer to a follow-up\" / \"track separately\" for anything in-scope — that's a deferral the team is trying to avoid. Only flag a deferral when the work is genuinely out of this PR's scope (a separate subsystem, a large refactor), and say so explicitly — PM will decide whether to groom it into a ticket. Default: tight PR, everything fixed before merge.\n7. Post the comment — **on stdin, never through a double-quoted shell string.** Inside `\"…\"` the shell command-substitutes every backticked token in your prose: `` `pwd -P` `` posts as your working directory, `` `radio orphans` `` posts as that command's output, and a backticked identifier that names no command — `` `$SOME_VAR` ``, `` `_some_helper()` `` — posts as **nothing at all**, deleting the finding's subject and leaving a sentence that reads as if a word were missing. Findings here name functions, variables and paths in inline code constantly, and this comment is the only thing that outlives your tab, so feed the body in with `--body-file -` and a **quote-delimited** heredoc:\n ```bash\n gh pr comment --body-file - <<'REVIEW'\n ## Spec compliance\n … the full analysis. `backticks`, $vars, $(subshells), ${braces} and don't-style\n apostrophes all reach GitHub verbatim …\n REVIEW\n ```\n The quote on the delimiter (`<<'REVIEW'`, not `< <<'SUMMARY'\n \n SUMMARY\n # — or —\n radio send --to pm --intent review-complete-with-findings --pr <<'SUMMARY'\n \n SUMMARY\n ```\n Check `radio send`'s stdout: `delivered` / `queued — pm is busy` means the verdict reached PM. But `radio: WARNING — no session for pm-…` (or `WARNING — pm-… looks dead …`) means PM isn't running, and `queued — pm is idle but wake failed …` means it's queued with no auto-redelivery — in those cases say so to the user instead of assuming the verdict was delivered.\n9. **Auto-destruct.** Run `task-done --remove-worktree` as your final action — it removes this review worktree and closes the tab, and `--remove-worktree` skips the confirmation prompt, so it needs no input from you. PM reads your verdict from the PR comment + the radio ping (on its next `radio check`), never from this tab, so there is nothing to keep open. Self-cleaning is mandatory, not optional.\n\n **The one thing that is not in the PR comment** is step 8's delivery outcome. If `radio send` reported anything other than a landed delivery — `WARNING — no session for pm-…`, `WARNING — pm-… looks dead …`, or `queued — pm is idle but wake failed …` — that warning exists nowhere but this tab, and destroying it would strand the verdict unread. In that case only: say so to the user, and hold the tab until they have seen it. Otherwise, destruct.\n\n\n### Authority boundaries\n\nYou are explicitly NOT authorized to:\n- Approve PRs (`gh pr review --approve`)\n- Merge PRs (`gh pr merge`)\n- Close PRs\n- Push commits or edit branches\n- Modify project Status fields (Notion or otherwise)\n\nPM holds those authorities. A clean verdict just means \"I found nothing worth blocking\"; PM decides when to merge.", + "tools": [ + "fs_read", + "grep", + "glob", + "execute_bash", + "@notion" + ], + "allowedTools": [ + "fs_read", + "grep", + "glob", + "@notion" + ], + "mcpServers": { + "notion": { + "url": "https://mcp.notion.com/mcp" + } + }, + "welcomeMessage": "Reviewer mode. Reviewing the PR passed to me at startup against its Notion spec page." +} diff --git a/kiro-notion/bin/task-init b/kiro-notion/bin/task-init index 7e44b48..d8a3a88 100755 --- a/kiro-notion/bin/task-init +++ b/kiro-notion/bin/task-init @@ -9,7 +9,7 @@ Usage: task-init [--tasks ID] [--projects ID] [--board ID] Set up the current repo for the kiro-notion workflow: - Create .kiro/steering/notion-workflow.md from the bundled template - - Install pm, planner, worker agents into .kiro/agents/ + - Install pm, planner, worker, reviewer agents into .kiro/agents/ - Merge radio PM↔worker messaging hooks into .kiro/agents/*.json Scope (which categories of files to touch): diff --git a/kiro-notion/steering/notion-workflow.example.md b/kiro-notion/steering/notion-workflow.example.md index 49b5952..e139b09 100644 --- a/kiro-notion/steering/notion-workflow.example.md +++ b/kiro-notion/steering/notion-workflow.example.md @@ -571,6 +571,30 @@ leaves focus on your tab, because the PM's tab is where you work and nobody types in a worker's. The one exception is a worker you step into to steer by hand — launch that one with `--no-auto-submit`. +To dispatch a one-shot reviewer worker for a PR, run +`task-reviewer []` from any spare tab. +It spawns a fresh zellij tab + worktree on the PR's head ref, runs the kiro +`reviewer` agent on `claude-sonnet-5.5` (`-m` to override; an exported +`$TASK_WORK_MODEL` overrides it too, so a worker override also changes the +reviewer's model and cost), cross-checks the PR against the Notion spec page +(passed as the second arg and read through the Notion MCP; PR-body auto-detect +is GitHub-only, so without it the review is diff-only), reviews the diff, posts a single thorough PR comment via +`gh pr comment --body-file -`, and radios PM back with `review-complete-clean` or +`review-complete-with-findings`. PM still owns the merge decision — the +reviewer never approves, merges, or mutates status. Then it **auto-destructs**: +`task-done --remove-worktree` removes its worktree and closes its tab. The +analysis lives in the PR comment; re-read it with `gh pr view --comments`. +The one exception is a failed radio delivery: when the verdict never reached +PM, the reviewer holds its tab open to say so, because that outcome is the one +thing its PR comment does not record. This loadout has shipped a kiro reviewer +since #320; before it, `task-reviewer` refused here. + +The reviewer runs with `--trust-all-tools` by default; `--no-trust-all` (or +`TASK_WORK_TRUST_ALL=0`) babysits a specific review. Radio auto-submit is a +separate setting, also on by default, and `--no-trust-all` does not switch it +off (#146): the supervised review is exactly the one whose verdict should not +stall on an Enter. `--no-auto-submit` is its own opt-out. + If a worker tab dies unexpectedly, is closed without `task-done`, or kiro resumes a session without re-firing the `agentSpawn` hook, the session file's `LAST_HEARTBEAT` will go stale. Run `radio orphans` to list any session whose diff --git a/tests/drift_check.bats b/tests/drift_check.bats index 48bfd4f..77bb934 100644 --- a/tests/drift_check.bats +++ b/tests/drift_check.bats @@ -161,3 +161,79 @@ EOF assert_success done } + +# --------------------------------------------------------------------------- +# Prompt regions (#320): markdown comment sentinels and kiro agent JSON +# --------------------------------------------------------------------------- + +# A markdown prompt and a kiro agent config carrying the same region. +make_prompt_pair() { + local md="$1" json="$2" json_line="${3:-shared line}" + printf '%s\n' intro '' 'shared line' '' outro >"$md" + jq -n --arg l "$json_line" \ + '{name: "reviewer", prompt: ("kiro intro\n\n" + $l + "\n\nkiro outro")}' >"$json" +} + +@test "check-drift reads a kiro agent's decoded prompt and the HTML-comment sentinel form" { + make_prompt_pair "$TMP_DIR/reviewer.md" "$TMP_DIR/reviewer.json" + echo "demo|shared|$TMP_DIR/reviewer.md $TMP_DIR/reviewer.json" >"$TMP_DIR/manifest" + CHECK_DRIFT_MANIFEST="$TMP_DIR/manifest" run bash "$CHECK_DRIFT" + assert_success + assert_output --partial "1 group(s) checked" +} + +@test "check-drift catches a drifted region inside a kiro agent's prompt" { + make_prompt_pair "$TMP_DIR/reviewer.md" "$TMP_DIR/reviewer.json" "DRIFTED line" + echo "demo|shared|$TMP_DIR/reviewer.md $TMP_DIR/reviewer.json" >"$TMP_DIR/manifest" + CHECK_DRIFT_MANIFEST="$TMP_DIR/manifest" run bash "$CHECK_DRIFT" + assert_failure + assert_output --partial "DRIFT [demo/shared]" + assert_output --partial "DRIFTED line" +} + +@test "check-drift reports a kiro agent prompt without the region as missing" { + make_prompt_pair "$TMP_DIR/reviewer.md" "$TMP_DIR/reviewer.json" + jq -n '{name: "reviewer", prompt: "no region here"}' >"$TMP_DIR/reviewer.json" + echo "demo|shared|$TMP_DIR/reviewer.md $TMP_DIR/reviewer.json" >"$TMP_DIR/manifest" + CHECK_DRIFT_MANIFEST="$TMP_DIR/manifest" run bash "$CHECK_DRIFT" + assert_failure + assert_output --partial "region missing in: $TMP_DIR/reviewer.json" +} + +@test "the reviewer contract is drift-guarded across all 8 reviewer prompts (#320)" { + # Post on stdin, radio the verdict, self-destruct, hold the tab open only + # when the verdict never reached PM: one copy, in every claude and kiro + # reviewer. kiro's reviewer once idled with the tab open while claude's + # self-destructed, because nothing tied the two together. + local rel=(.claude/commands/reviewer.md claude-gh/commands/reviewer.md + claude-jira/commands/reviewer.md claude-local/commands/reviewer.md + claude-notion/commands/reviewer.md kiro-gh/agents/reviewer.json + kiro-local/agents/reviewer.json kiro-notion/agents/reviewer.json) + local f real="" broken="" + for f in "${rel[@]}"; do + real+=" $REPO_ROOT_REAL/$f" + if [[ "$f" == kiro-local/* ]]; then broken+=" $TMP_DIR/$f"; else broken+=" $REPO_ROOT_REAL/$f"; fi + done + # Both regions are in the default manifest, over exactly these files. + local listed="${real//$REPO_ROOT_REAL\//}"; listed="${listed# }" + run grep -cF "\"reviewer-contract|reviewer-post-radio-destruct|$listed\"" "$CHECK_DRIFT" + assert_output 1 + run grep -cF "\"reviewer-contract|reviewer-destruct-summary|$listed\"" "$CHECK_DRIFT" + assert_output 1 + # The real copies agree. + printf 'reviewer-contract|%s|%s\n' reviewer-post-radio-destruct "$real" reviewer-destruct-summary "$real" >"$TMP_DIR/manifest" + CHECK_DRIFT_MANIFEST="$TMP_DIR/manifest" run bash "$CHECK_DRIFT" + assert_success + assert_output --partial "2 group(s) checked" + # Break one kiro copy's region and watch the group go red. + mkdir -p "$TMP_DIR/kiro-local/agents" + jq '.prompt |= sub("Otherwise, destruct\\."; "Otherwise, idle.")' \ + "$REPO_ROOT_REAL/kiro-local/agents/reviewer.json" >"$TMP_DIR/kiro-local/agents/reviewer.json" + run diff -q "$REPO_ROOT_REAL/kiro-local/agents/reviewer.json" "$TMP_DIR/kiro-local/agents/reviewer.json" + assert_failure + echo "reviewer-contract|reviewer-post-radio-destruct|$broken" >"$TMP_DIR/manifest" + CHECK_DRIFT_MANIFEST="$TMP_DIR/manifest" run bash "$CHECK_DRIFT" + assert_failure + assert_output --partial "DRIFT [reviewer-contract/reviewer-post-radio-destruct]" + assert_output --partial "Otherwise, idle." +} diff --git a/tests/dual_assistant.bats b/tests/dual_assistant.bats index 0a25ca3..d6e4d52 100644 --- a/tests/dual_assistant.bats +++ b/tests/dual_assistant.bats @@ -134,6 +134,9 @@ _sum() { # Both blocks carry the same ids, and there is no drift to report. assert_equal "$(grep -c 'tasks=collection://t1 projects=collection://p2 board=b3' <<<"$output")" 2 refute_output --partial "Drift" + # The kiro reviewer comes along, hooked, so task-reviewer --agent kiro works (#320). + run jq -r '.hooks.agentSpawn[0].command' "$MAIN_REPO/.kiro/agents/reviewer.json" + assert_output --partial "radio register" } @test "add assistant kiro on claude-local: the tasks/ backlog and scaffolding are untouched" { @@ -150,6 +153,9 @@ _sum() { assert_success assert_output --partial "loadout : claude-local (active)" assert_output --partial "loadout : kiro-local" + # The kiro reviewer comes along, hooked, so task-reviewer --agent kiro works (#320). + run jq -r '.hooks.agentSpawn[0].command' "$MAIN_REPO/.kiro/agents/reviewer.json" + assert_output --partial "radio register" } @test "add assistant kiro on claude-jira: refused, naming #92, and nothing is written" { diff --git a/tests/kiro_gh_task_init.bats b/tests/kiro_gh_task_init.bats index 32ecee3..5a7afff 100644 --- a/tests/kiro_gh_task_init.bats +++ b/tests/kiro_gh_task_init.bats @@ -171,7 +171,7 @@ teardown() { run "$KIRO_GH_TASK_INIT" --commands assert_success assert [ ! -f "$TARGET_DIR/.kiro/steering/gh-workflow.md" ] - for agent in pm planner worker; do + for agent in pm planner worker reviewer; do assert [ -f "$TARGET_DIR/.kiro/agents/$agent.json" ] done } @@ -205,10 +205,10 @@ teardown() { # Project-level agents # --------------------------------------------------------------------------- -@test "installs pm/planner/worker into .kiro/agents/ as real files" { +@test "installs pm/planner/worker/reviewer into .kiro/agents/ as real files" { run "$KIRO_GH_TASK_INIT" assert_success - for agent in pm planner worker; do + for agent in pm planner worker reviewer; do assert [ -f "$TARGET_DIR/.kiro/agents/$agent.json" ] assert [ ! -L "$TARGET_DIR/.kiro/agents/$agent.json" ] done @@ -524,7 +524,7 @@ teardown() { @test "installed kiro-gh agents do not declare GitHub MCP" { run "$KIRO_GH_TASK_INIT" assert_success - for agent in pm planner worker; do + for agent in pm planner worker reviewer; do local f="$TARGET_DIR/.kiro/agents/$agent.json" # jq must parse — guards against any malformed JSON from edits. run jq . "$f" diff --git a/tests/kiro_local_task_init.bats b/tests/kiro_local_task_init.bats index 09605ff..1414071 100644 --- a/tests/kiro_local_task_init.bats +++ b/tests/kiro_local_task_init.bats @@ -148,7 +148,7 @@ teardown() { run "$KIRO_LOCAL_TASK_INIT" --commands assert_success assert [ ! -f "$TARGET_DIR/.kiro/steering/local-workflow.md" ] - for agent in pm planner worker; do + for agent in pm planner worker reviewer; do assert [ -f "$TARGET_DIR/.kiro/agents/$agent.json" ] done } @@ -174,10 +174,10 @@ teardown() { # Project-level agents # --------------------------------------------------------------------------- -@test "installs pm/planner/worker into .kiro/agents/ as real files" { +@test "installs pm/planner/worker/reviewer into .kiro/agents/ as real files" { run "$KIRO_LOCAL_TASK_INIT" assert_success - for agent in pm planner worker; do + for agent in pm planner worker reviewer; do assert [ -f "$TARGET_DIR/.kiro/agents/$agent.json" ] assert [ ! -L "$TARGET_DIR/.kiro/agents/$agent.json" ] done @@ -196,6 +196,37 @@ teardown() { assert_success } +@test "installs the reviewer agent kiro-local ships, with its radio register hook (#320)" { + # The reviewer is what task-reviewer launches, and the register hook is what + # makes its verdict-bearing role reachable (#304). + run "$KIRO_LOCAL_TASK_INIT" + assert_success + jq -S 'del(.hooks)' "$TARGET_DIR/.kiro/agents/reviewer.json" > "$TARGET_DIR/installed.norm" + jq -S '.' "$REPO_ROOT_REAL/kiro-local/agents/reviewer.json" > "$TARGET_DIR/source.norm" + run diff -u "$TARGET_DIR/source.norm" "$TARGET_DIR/installed.norm" + assert_success + run jq -r '.hooks.agentSpawn[0].command' "$TARGET_DIR/.kiro/agents/reviewer.json" + assert_output --partial "radio register" + assert_output --partial "--loadout \${TASK_FORCE_LOADOUT:-kiro-local}" +} + +@test "the shipped kiro-local reviewer.json is a well-formed reviewer agent (#320)" { + local f="$REPO_ROOT_REAL/kiro-local/agents/reviewer.json" + run jq -e '.name == "reviewer" and (.prompt | length > 0) and (.description | length > 0) and (.welcomeMessage | length > 0)' "$f" + assert_success + # It posts and radios through the shell; everything it auto-allows it has. + run jq -e '(.tools | index("execute_bash")) and ((.allowedTools - .tools) == [])' "$f" + assert_success + # Hooks are task-init's to merge (#218), never shipped in the source. + run jq -e 'has("hooks")' "$f" + assert_failure + # The spec is a file under tasks/: no tracker MCP, no @tool. + run jq -e '.mcpServers // empty' "$f" + assert_failure + run jq -e '[.tools[] | select(startswith("@"))] | length == 0' "$f" + assert_success +} + @test "--force overwrites pre-existing project-level agent" { mkdir -p "$TARGET_DIR/.kiro/agents" echo "stale" > "$TARGET_DIR/.kiro/agents/pm.json" @@ -259,7 +290,7 @@ teardown() { run "$KIRO_LOCAL_TASK_INIT" assert_success assert [ ! -d "$TARGET_DIR/.kiro/hooks" ] - for agent in pm planner worker; do + for agent in pm planner worker reviewer; do run jq -r '[.hooks.agentSpawn[0].command, .hooks.userPromptSubmit[0].command, .hooks.stop[0].command] | @tsv' \ "$TARGET_DIR/.kiro/agents/$agent.json" assert_success diff --git a/tests/kiro_notion_task_init.bats b/tests/kiro_notion_task_init.bats index fece9ad..97267da 100644 --- a/tests/kiro_notion_task_init.bats +++ b/tests/kiro_notion_task_init.bats @@ -233,7 +233,7 @@ _ids() { grep -E '^- \*\*(Tasks|Projects|Board page)\*\*' "$TARGET_DIR/.kiro/ste run "$KIRO_TASK_INIT" --commands assert_success assert [ ! -f "$TARGET_DIR/.kiro/steering/notion-workflow.md" ] - for agent in pm planner worker; do + for agent in pm planner worker reviewer; do assert [ -f "$TARGET_DIR/.kiro/agents/$agent.json" ] done } @@ -249,10 +249,10 @@ _ids() { grep -E '^- \*\*(Tasks|Projects|Board page)\*\*' "$TARGET_DIR/.kiro/ste # Project-level agents # --------------------------------------------------------------------------- -@test "installs pm/planner/worker into .kiro/agents/ as real files" { +@test "installs pm/planner/worker/reviewer into .kiro/agents/ as real files" { run "$KIRO_TASK_INIT" assert_success - for agent in pm planner worker; do + for agent in pm planner worker reviewer; do assert [ -f "$TARGET_DIR/.kiro/agents/$agent.json" ] assert [ ! -L "$TARGET_DIR/.kiro/agents/$agent.json" ] done @@ -271,6 +271,37 @@ _ids() { grep -E '^- \*\*(Tasks|Projects|Board page)\*\*' "$TARGET_DIR/.kiro/ste assert_success } +@test "installs the reviewer agent kiro-notion ships, with its radio register hook (#320)" { + # The reviewer is what task-reviewer launches, and the register hook is what + # makes its verdict-bearing role reachable (#304). + run "$KIRO_TASK_INIT" + assert_success + jq -S 'del(.hooks)' "$TARGET_DIR/.kiro/agents/reviewer.json" > "$TARGET_DIR/installed.norm" + jq -S '.' "$REPO_ROOT_REAL/kiro-notion/agents/reviewer.json" > "$TARGET_DIR/source.norm" + run diff -u "$TARGET_DIR/source.norm" "$TARGET_DIR/installed.norm" + assert_success + run jq -r '.hooks.agentSpawn[0].command' "$TARGET_DIR/.kiro/agents/reviewer.json" + assert_output --partial "radio register" + assert_output --partial "--loadout \${TASK_FORCE_LOADOUT:-kiro-notion}" +} + +@test "the shipped kiro-notion reviewer.json is a well-formed reviewer agent (#320)" { + local f="$REPO_ROOT_REAL/kiro-notion/agents/reviewer.json" + run jq -e '.name == "reviewer" and (.prompt | length > 0) and (.description | length > 0) and (.welcomeMessage | length > 0)' "$f" + assert_success + # It posts and radios through the shell; everything it auto-allows it has. + run jq -e '(.tools | index("execute_bash")) and ((.allowedTools - .tools) == [])' "$f" + assert_success + # Hooks are task-init's to merge (#218), never shipped in the source. + run jq -e 'has("hooks")' "$f" + assert_failure + # Notion is reached the way the worker reaches it: the @notion tool and the + # same notion MCP server. + run jq -e '(.tools | index("@notion")) and (.allowedTools | index("@notion"))' "$f" + assert_success + assert_equal "$(jq -c .mcpServers.notion "$f")" "$(jq -c .mcpServers.notion "$REPO_ROOT_REAL/kiro-notion/agents/worker.json")" +} + @test "--force overwrites pre-existing project-level agent" { mkdir -p "$TARGET_DIR/.kiro/agents" echo "stale" > "$TARGET_DIR/.kiro/agents/pm.json" @@ -334,7 +365,7 @@ _ids() { grep -E '^- \*\*(Tasks|Projects|Board page)\*\*' "$TARGET_DIR/.kiro/ste run "$KIRO_TASK_INIT" assert_success assert [ ! -d "$TARGET_DIR/.kiro/hooks" ] - for agent in pm planner worker; do + for agent in pm planner worker reviewer; do run jq -r '[.hooks.agentSpawn[0].command, .hooks.userPromptSubmit[0].command, .hooks.stop[0].command] | @tsv' \ "$TARGET_DIR/.kiro/agents/$agent.json" assert_success diff --git a/tests/kiro_radio_pull.bats b/tests/kiro_radio_pull.bats index 619159a..b93d504 100644 --- a/tests/kiro_radio_pull.bats +++ b/tests/kiro_radio_pull.bats @@ -28,9 +28,11 @@ KIRO_AGENTS=( "$REPO_ROOT_REAL/kiro-gh/agents/worker.json" "$REPO_ROOT_REAL/kiro-local/agents/planner.json" "$REPO_ROOT_REAL/kiro-local/agents/pm.json" + "$REPO_ROOT_REAL/kiro-local/agents/reviewer.json" "$REPO_ROOT_REAL/kiro-local/agents/worker.json" "$REPO_ROOT_REAL/kiro-notion/agents/planner.json" "$REPO_ROOT_REAL/kiro-notion/agents/pm.json" + "$REPO_ROOT_REAL/kiro-notion/agents/reviewer.json" "$REPO_ROOT_REAL/kiro-notion/agents/worker.json" ) @@ -61,7 +63,7 @@ poll_block() { done } -@test "the poll instruction is byte-identical across all 10 kiro agents (#177 loadout-neutral)" { +@test "the poll instruction is byte-identical across all 12 kiro agents (#177 loadout-neutral)" { local ref ref=$(poll_block "${KIRO_AGENTS[0]}") assert [ -n "$ref" ] diff --git a/tests/prose_body_file.bats b/tests/prose_body_file.bats index bf9a9d1..3a1e87f 100644 --- a/tests/prose_body_file.bats +++ b/tests/prose_body_file.bats @@ -152,6 +152,8 @@ ALL_REVIEWER_PROMPTS=( "$REPO_ROOT_REAL/claude-notion/commands/reviewer.md" "$REPO_ROOT_REAL/claude-local/commands/reviewer.md" "$REPO_ROOT_REAL/kiro-gh/agents/reviewer.json" + "$REPO_ROOT_REAL/kiro-local/agents/reviewer.json" + "$REPO_ROOT_REAL/kiro-notion/agents/reviewer.json" ) ALL_PM_PROMPTS=( @@ -179,20 +181,20 @@ all_prompts() { "$REPO_ROOT_REAL"/kiro-notion/agents/*.json } -@test "the sweep below covers all 30 shipped prompts — 20 markdown + 10 kiro JSON" { +@test "the sweep below covers all 32 shipped prompts — 20 markdown + 12 kiro JSON" { # Load-bearing: the negative assertions that follow are globs in a loop, so a # glob that matched nothing would pass them vacuously. It is also the exact # shape of the trap in this bug — `grep -rn -- '-b "'` finds the 20 markdown - # prompts and MISSES all 10 kiro agent JSONs, because inside a JSON string the + # prompts and MISSES all 12 kiro agent JSONs, because inside a JSON string the # quotes are backslash-escaped (`-b \"`). Fix by grep alone and you ship 20 of - # 30 files fixed with kiro silently still broken. + # 32 files fixed with kiro silently still broken. local all md json all=$(all_prompts | wc -l | tr -d ' ') md=$(all_prompts | grep -c '\.md$') json=$(all_prompts | grep -c '\.json$') - assert_equal "$all" 30 + assert_equal "$all" 32 assert_equal "$md" 20 - assert_equal "$json" 10 + assert_equal "$json" 12 # Every path resolved to a real file (an unexpanded glob would count as 1). local f while read -r f; do assert [ -f "$f" ]; done < <(all_prompts) diff --git a/tests/task_reviewer.bats b/tests/task_reviewer.bats index 99d4e69..cfbfc42 100644 --- a/tests/task_reviewer.bats +++ b/tests/task_reviewer.bats @@ -877,6 +877,61 @@ teardown() { assert [ ! -d "$WORKTREE_BASE/review-pr42" ] } +# kiro-notion and kiro-local ship a reviewer agent since #320; before it they +# refused. The spec is the tracker's own identifier, passed as the second arg: +# no Closes #N auto-detect off GitHub. +@test "kiro-notion task-reviewer: launches the kiro reviewer with the Notion page URL (#320)" { + local notion_url="https://www.notion.so/myws/Spec-Page-1234abcd" + AW_IMPL=kiro-notion run "$TASK_REVIEWER" 42 "$notion_url" + assert_success + assert [ -d "$WORKTREE_BASE/review-pr42" ] + assert_stub_called zellij "kiro-cli chat --agent reviewer --model claude-sonnet-5.5 " + assert_stub_called zellij "Review PR https://github.com/owner/repo/pull/42 against spec $notion_url" +} + +@test "kiro-local task-reviewer: launches the kiro reviewer with the task slug (#320)" { + AW_IMPL=kiro-local run "$TASK_REVIEWER" 42 042-add-login + assert_success + assert [ -d "$WORKTREE_BASE/review-pr42" ] + assert_stub_called zellij "kiro-cli chat --agent reviewer --model claude-sonnet-5.5 " + assert_stub_called zellij "Review PR https://github.com/owner/repo/pull/42 against spec 042-add-login" +} + +@test "kiro-notion task-reviewer: a GitHub Closes #N is not its spec — diff-only (#320)" { + export GH_STUB_PR_BODY="Closes #38" + AW_IMPL=kiro-notion run "$TASK_REVIEWER" 42 + assert_success + assert_output --partial "auto-detect is GitHub-only" + assert_stub_called zellij "kiro-cli chat --agent reviewer" + run grep -F "against spec" "$STUB_CALLS_DIR/zellij.calls" + assert_failure +} + +@test "kiro-local task-reviewer: a GitHub Closes #N is not its spec — diff-only (#320)" { + export GH_STUB_PR_BODY="Closes #38" + AW_IMPL=kiro-local run "$TASK_REVIEWER" 42 + assert_success + assert_output --partial "auto-detect is GitHub-only" + assert_stub_called zellij "kiro-cli chat --agent reviewer" + run grep -F "against spec" "$STUB_CALLS_DIR/zellij.calls" + assert_failure +} + +# The guard is keyed on the shipped file, not an impl list, so a kiro loadout +# without one still refuses. No shipped loadout lacks one now: prove it on a +# copy of the tree with the agent taken out. +@test "kiro task-reviewer: a loadout that ships no reviewer.json is refused (#320)" { + local root="$BATS_TEST_TMPDIR/aw-root" + mkdir -p "$root" + cp -R "$REPO_ROOT_REAL/bin" "$REPO_ROOT_REAL/lib" "$REPO_ROOT_REAL/kiro-local" "$root/" + rm "$root/kiro-local/agents/reviewer.json" + AW_IMPL=kiro-local run "$root/bin/task-reviewer" 42 042-add-login + assert_failure + assert_output --partial "task-reviewer is not available for impl 'kiro-local'" + assert_output --partial "kiro-local/agents/reviewer.json" + assert [ ! -d "$WORKTREE_BASE/review-pr42" ] +} + @test "kiro task-reviewer: claude-only --no-auto is an unknown flag" { AW_IMPL=kiro-gh run "$TASK_REVIEWER" 42 --no-auto assert_failure @@ -960,12 +1015,20 @@ teardown() { assert_output --partial "no agentic-workflow impl configured" } -@test "top-level task-reviewer: errors cleanly for impls without a reviewer variant" { +@test "top-level task-reviewer: dispatches to kiro-notion variant (#320)" { mkdir -p "$MAIN_REPO/.kiro/steering" touch "$MAIN_REPO/.kiro/steering/notion-workflow.md" run "$TASK_REVIEWER" 42 - assert_failure - assert_output --partial "task-reviewer is not available for impl" + assert_success + assert_stub_called zellij "kiro-cli chat --agent reviewer" +} + +@test "top-level task-reviewer: dispatches to kiro-local variant (#320)" { + mkdir -p "$MAIN_REPO/.kiro/steering" + touch "$MAIN_REPO/.kiro/steering/local-workflow.md" + run "$TASK_REVIEWER" 42 + assert_success + assert_stub_called zellij "kiro-cli chat --agent reviewer" } @test "top-level task-reviewer: forwards positional + flags through to loadout" { diff --git a/tests/worker_checklist.bats b/tests/worker_checklist.bats index 3192bce..ecac119 100644 --- a/tests/worker_checklist.bats +++ b/tests/worker_checklist.bats @@ -127,7 +127,7 @@ ALL_WORKER_PROMPTS=( done } -@test "all 5 reviewer prompts apply the same empty-vs-passing distinction" { +@test "all 8 reviewer prompts apply the same empty-vs-passing distinction" { # A reviewer that cross-checks "CI green" off an empty list repeats the bug # at one remove. local reviewers=( @@ -137,6 +137,8 @@ ALL_WORKER_PROMPTS=( "$REPO_ROOT_REAL/claude-notion/commands/reviewer.md" "$REPO_ROOT_REAL/claude-local/commands/reviewer.md" "$REPO_ROOT_REAL/kiro-gh/agents/reviewer.json" + "$REPO_ROOT_REAL/kiro-local/agents/reviewer.json" + "$REPO_ROOT_REAL/kiro-notion/agents/reviewer.json" ) for f in "${reviewers[@]}"; do run worker_prompt "$f" @@ -217,6 +219,8 @@ closes_line_for() { "$REPO_ROOT_REAL/claude-notion/commands/reviewer.md" "$REPO_ROOT_REAL/claude-local/commands/reviewer.md" "$REPO_ROOT_REAL/kiro-gh/agents/reviewer.json" + "$REPO_ROOT_REAL/kiro-local/agents/reviewer.json" + "$REPO_ROOT_REAL/kiro-notion/agents/reviewer.json" ) for f in "${reviewers[@]}"; do run worker_prompt "$f" diff --git a/tools/check-drift.sh b/tools/check-drift.sh index 757feba..3d637d5 100755 --- a/tools/check-drift.sh +++ b/tools/check-drift.sh @@ -6,6 +6,12 @@ # || ... # # Override the manifest for tests by setting CHECK_DRIFT_MANIFEST to a file path. +# +# A region is the lines between `# region:` and `# endregion:`. In a +# markdown prompt, where a `#` line renders as a heading, the sentinels may be +# written `` / `` instead. A kiro +# agent config (*.json) is checked on its `prompt` string, since its lines exist +# only once the JSON is decoded (#320). set -euo pipefail @@ -27,16 +33,34 @@ GROUPS_DEFAULT=( "tracker-settings-notion|notion-settings-render|claude-notion/bin/task-init kiro-notion/bin/task-init" "tracker-settings-gh|tracker-preserve-gh|claude-gh/bin/task-init kiro-gh/bin/task-init" "install-shared-symlinks|install-shared-symlinks|claude-gh/install.sh claude-jira/install.sh claude-local/install.sh claude-notion/install.sh kiro-gh/install.sh kiro-local/install.sh kiro-notion/install.sh" + "reviewer-contract|reviewer-destruct-summary|.claude/commands/reviewer.md claude-gh/commands/reviewer.md claude-jira/commands/reviewer.md claude-local/commands/reviewer.md claude-notion/commands/reviewer.md kiro-gh/agents/reviewer.json kiro-local/agents/reviewer.json kiro-notion/agents/reviewer.json" + "reviewer-contract|reviewer-post-radio-destruct|.claude/commands/reviewer.md claude-gh/commands/reviewer.md claude-jira/commands/reviewer.md claude-local/commands/reviewer.md claude-notion/commands/reviewer.md kiro-gh/agents/reviewer.json kiro-local/agents/reviewer.json kiro-notion/agents/reviewer.json" ) # endregion:default-manifest +# The text a file's sentinels live in: a kiro agent config's decoded prompt, +# otherwise the file itself. +region_source() { + case "$1" in + *.json) jq -r '.prompt // empty' "$1" ;; + *) cat "$1" ;; + esac +} + extract_region() { local file="$1" region="$2" - awk -v r="$region" ' - $0 == "# region:" r { in_region=1; next } - $0 == "# endregion:" r { in_region=0; next } + region_source "$file" | awk -v r="$region" ' + $0 == "# region:" r || $0 == "" { in_region=1; next } + $0 == "# endregion:" r || $0 == "" { in_region=0; next } in_region { print } - ' "$file" + ' +} + +has_region() { + local file="$1" region="$2" + # Not a pipe: grep -q exits on the first match, and under pipefail the + # writer's SIGPIPE would turn a found region into a missing one. + grep -qFx -e "# region:$region" -e "" <<<"$(region_source "$file")" } # Compare a single (group, region, files...) entry. Returns 0 if all files @@ -57,7 +81,7 @@ check_group() { # both produce empty extract output, but the former is almost always the bug # (someone renamed the region or forgot to mark a new file), while the # latter is a deliberate placeholder. - if ! grep -qFx "# region:$region" "$file"; then + if ! has_region "$file" "$region"; then echo "FAIL [$group/$region]: region missing in: $file" >&2 failed=1 continue