review

Review Skill

Leaf skill for reviewing impl output. Effect first, code second: confirm the change is right via the cheapest appropriate check (static gates for UI, cheap non-browser runtime for API/CLI), then do a light pass over the code against the plan note. Agent-driven browser inspection and screenshots are opt-in (AGENTS.md), not a default review step. Produces a review note. Does not change code, commit, or merge.

Loaded via y chat --skill review, usually dispatched by the dev coordinator after the impl callback (and before y dev commit), or directly for an ad-hoc audit.

Working mode

Runs inside the worktree being reviewed (the caller passes --work-dir <worktree_path>), or in the project main directory if reviewing a PR.

Workflow

1. y todo get <todo_id>: read the todo desc plus the linked plan note.
2. Get the diff:
   • Worktree-local: git status + git diff + git diff --cached (uncommitted changes)
   • Already committed in the worktree: git log <base>..HEAD + git diff <base>..HEAD
   • PR: gh pr diff <pr-number> (or gh pr view <pr-number>)
3. Read the plan note (linked to the todo). That is your spec.
4. Smoke-verify the effect FIRST, before reading code line by line: confirm the change is right with the cheapest appropriate check. A clean-looking diff that clearly cannot produce the plan's effect is still request-changes.
   • UI / visual-delta changes (layout or style, a new or restyled component, a changed rendered value or format, hover/menu state, responsive fit): the default is the AGENTS.md static gate (typecheck or build, lint, non-browser tests). Do not start a dev server or drive a browser as routine review self-verification; screenshots are also opt-in. Runtime UI acceptance is left to the user unless they explicitly asked. When they did ask and you capture a PNG, persist it under $Y_AGENT_HOME/assets/screenshots/, never /tmp, and cite the full path.
   • Non-UI runtime effects (CLI output, API response, cheap scriptable smoke): reproduce with the command, curl, or test suite and compare to what the plan or todo said. These stay allowed and are preferred over pure code reading when cheap.
   • If a requested runtime check cannot be done here (no runnable surface, needs production data or secrets you lack), say so explicitly in the note and fall back to verifying that the plan's stated verify step is satisfiable. Do not silently skip it and claim it works.
5. Light code pass, only after the effect checks out. Review against:
   • Plan adherence: did impl do what the plan said? Anything skipped, anything out-of-scope added?
   • Surgical scope: every changed line should trace to a plan sub-task. Flag refactors of adjacent code, reformatting, or unrequested "improvements". Pre-existing dead code deleted by impl is also a finding unless the plan called for it.
   • Verifiability: each completed sub-task has an obvious way to confirm it works (the plan's verify step is satisfied, or impl recorded a concrete check). "I think it works" is a finding.
   • Correctness: obvious bugs, off-by-ones, wrong types, missing error paths.
   • Code quality: dead code, duplicated logic, unclear naming, comments stating the obvious.
   • Consistency: matches surrounding code style and existing conventions.
6. Write the review note. First decide whether this is a new note or a new round on an existing one (see "One deliverable, one note"). The path is $Y_AGENT_HOME/pages/review-<todo_id>-<slug>.md (absolute path: --work-dir is a worktree, but the review file must land in the top-level pages/), where <slug> names the deliverable, never the round:

   ---
   title: review for todo <id> — <slug>
   type: review
   project: <project>
   todo: <todo-id>
   verdict: approve | request-changes | partial
   rounds: <n>
   ---
   
   ## Verdict
   approve | request-changes | partial (context handoff, see "Not yet reviewed")
   Round <n>, <YYYY-MM-DD>. <one line: what this round covered / what changed since the last one>
   
   ## Open findings (blocking)
   - [file:line] description — why it matters
   (empty on approve; this section is the live list across rounds, not per-round history)
   
   ## Round <n> — <YYYY-MM-DD> — <verdict>
   
   ### Smoke verification (effect)
   - checks run: static gate (typecheck/build/tests) and/or cheap non-browser runtime; what was observed
   - browser UI / screenshot only if the user asked: how it was inspected + path(s) under assets/screenshots/
   
   ### Plan adherence
   - …
   
   ### Findings (request-changes)
   - [file:line] description — why it matters
   
   ### Suggestions (nit, non-blocking)
   - …
   

   Round 1 fills ## Round 1. A re-review appends ## Round <n+1> at the end and rewrites the header block (front-matter verdict / rounds, ## Verdict, ## Open findings) so the top of the file always states the current state. Never edit a past round's text; resolve its findings by saying so in the new round.
7. Link the review to the todo: y assoc note pages/review-<todo_id>-<slug>.md --todo <todo_id> (this CLI takes the path relative to $Y_AGENT_HOME). Run it on appended rounds too; it is idempotent when the note is already linked.
8. Call back to the caller with the verdict and the review note path, then stop. No repeat of the findings in this chat (AGENTS.md "Callback rules"): the note holds them and the coordinator reports to the user.

   y chat --chat-id <from_chat> -m "review done: <approve|request-changes>, see $Y_AGENT_HOME/pages/review-<todo_id>-<slug>.md" \
     ${Y_TOPIC:+--from-topic $Y_TOPIC} --trace-id <todo_id>
   

   On request-changes, the verdict plus the note path is the complete callback; at most add a clause naming how many blocking findings there are. Never enumerate findings in -m. The coordinator re-dispatches impl pointed at the note.