Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
28 changes: 28 additions & 0 deletions docs/plans/2026-10-24-code-rules-reach-the-reviewer.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,28 @@
---
canonical: decisions:docs/plans/2026-10-24-code-rules-reach-the-reviewer.md
phases: [2]
status: done
---
# Code rules reach the reviewer (pointer)

Canonical plan: [decisions/docs/plans/2026-10-24-code-rules-reach-the-reviewer.md](https://github.com/fredericrous/decisions/blob/main/docs/plans/2026-10-24-code-rules-reach-the-reviewer.md) (ADR-0044).

This repository carries Phase 2's skill and agent edits:
- worktree-task F4b step 2 builds the implementation-review brief with `amont-agent review-brief` (amont-agent 3.2.0), never by hand;
- step 3 and the implementation-review agent carry the `<<<RULES sha=…>>>` line the push check reads.

The `Candidates:` block (phase 4) comes in a later PR.

## Verification

- `amont-agent --version` after `brew upgrade fredericrous/tap/amont-agent` → 3.2.0, listing `rules-sha` and `review-brief` → 3.2.0, both listed.
- `amont-agent review-brief -C <amont-agent worktree> --plan=<canonical plan>` → a brief whose TREE and RULES lines bound amont-agent#142's own implementation review → it built that brief, and the review bound to it.
- `amont-agent review-brief -C <this worktree> --plan <canonical plan>` → exit 3, since this repository has no `.adr.yaml` → exit 3, aval says "no .adr.yaml".

`chezmoi apply` runs after the merge; its result is recorded in the decisions PR that records Phase 2.

## Implementation review

- approve-with-changes, then approve-with-changes on the Delta. Fixed: the reviewer contract's no-corpus case, exit 1 named, the repeated lines kept, the Delta's fresh RULES line.
- deliberate: the Delta step does not yet say "none in a repository with no `.adr.yaml`"; phase 4's dotfiles PR edits this same paragraph for the `Candidates:` block and adds it.
- 41k + 44k tokens.
7 changes: 5 additions & 2 deletions dot_claude/agents/implementation-review.md
Original file line number Diff line number Diff line change
Expand Up @@ -12,8 +12,11 @@ never waive a rule, and never approve on the author's behalf.

Your prompt carries, in order: the review block
(`<<<TREE repo=<name> sha=<64 hex>>>>` — the tree you are reading, which
the hook will check against the push), `Round 1.` or `Delta.`, the brief
the session built, and this contract. Read the plan at the path the brief
the hook will check against the push), `Round 1.` or `Delta.`, the
`<<<RULES sha=…>>>` line, the brief `amont-agent review-brief` built (inline,
or its path to read in full), and this contract. In a repository with no
`.adr.yaml` there is no RULES line and the brief is hand-built: that is not a
defect. Read the plan at the path the brief
names first, then the diff the brief carries (or the files its stat names,
when the diff was too large to inline). Open at most 10 more files or
1,500 lines: the files the diff touches, then their tests.
Expand Down
47 changes: 30 additions & 17 deletions dot_claude/skills/worktree-task/SKILL.md
Original file line number Diff line number Diff line change
Expand Up @@ -173,22 +173,31 @@ below names one of them.
`implementation-review` hook checks at the push that it happened for
the tree being pushed, so run it after F4, on the committed tree:
1. Post `📍 <project>: implementation review, round 1, about 2 min.`
2. Get the block: `amont-agent tree-sha --block` (the canonical tree,
`HEAD` without `docs/plans/`; a worktree named after its task still
says the repository's name). Build the brief once:
- the landed plan's path, and its Verification section verbatim;
- `git diff origin/main...HEAD --stat -- . ':!docs/plans'`, and the
full diff when it is under 60 KB; above that, the stat and "Read
the files";
- the code rules in full: `aval --json rules --level constraint
--governs code --full`; the other constraints: `aval rules --level
constraint`; and the decisions that apply: `aval relevant --json
--top 20` with one `--path` per file of `git diff --name-only
origin/main...HEAD` (not `--changed`, which is empty on a committed
branch; skipped when no file changed), each returned key resolved
with `aval resolve <key>`; all run in the worktree (the pack
refreshed at Start);
2. Build the brief with one command, in the worktree (the pack refreshed
at Start), never by hand (ADR-0044: a hand-built brief once carried
rule statements only, and the reviewer passed a placeholder name):
```bash
amont-agent review-brief --plan <landed plan> > <scratch>/brief.md
```
It prints, in order:
- the `<<<TREE repo=… sha=…>>>` block (the same as `tree-sha --block`:
`HEAD` without `docs/plans/`) and a `<<<RULES sha=…>>>` line;
- the plan's Verification section;
- the diff stat, and the full diff under 60 KB;
- the code rules in full;
- the constraint index;
- the decisions that apply to the changed paths, each resolved;
- the plan's Non-goals.

For a pointer plan, pass the canonical plan's path. The merge-base is
the remote's default branch; pass `--base <ref>` for a stacked branch.
Exit 1 means the repository or its base could not be read: pass
`--base <ref>` (a stacked or local-only branch). Exit 3 means aval
failed and nothing was printed; fix aval, never hand-build the brief
instead. One exception, until review-brief
handles it: a repository with no `.adr.yaml` has no decided code
rules, and aval exits 3 there. Build that brief by hand from the
parts above, with no rules section and no RULES line.
2b. **Interface review** (advisory; skipped unless the diff touches the
repository's UI). Trigger: the `ui` areas from `aval traits`, matched
against `git diff --name-only <base>..HEAD`; no match, no step. Build
Expand All @@ -205,7 +214,10 @@ below names one of them.
before the closing commit; a fix that changes code returns to F1, and
the implementation-review pass on the new tree is its Delta.
3. Launch the `implementation-review` agent once, in the foreground,
with, in order: the block line verbatim; `Round 1.`; the brief;
with, in order: the TREE line verbatim; `Round 1.`; the RULES line
verbatim (the push check journals a review whose prompt lacks it, or
carries a stale one); the brief, inline or as its path with "read it
in full" (an inline brief repeats both lines at its top; leave them);
"Read the plan and the diff. Open at most 10 more files or 1,500
lines. Follow your output contract." A background launch counts only
after its completion notification.
Expand All @@ -224,7 +236,8 @@ below names one of them.
6. `approve`, or `approve-with-changes` with every finding fixed or
named deliberate: continue to F5. A fix that changes code returns to
F1, and the **second** pass through F4b is the `Delta`: a fresh block
for the new tree, `Delta.`, the round-1 findings, the diff since,
for the new tree, `Delta.`, the RULES line from a fresh
`review-brief`, the round-1 findings, the diff since,
"answer resolved / not resolved / new blocker for each". The Delta may
resume the same reviewer with `SendMessage` (the block first in the
message) instead of a fresh launch: the hook counts a resumed round on
Expand Down
Loading