diff --git a/docs/plans/2026-10-23-code-rules-reach-writers-reviewers.md b/docs/plans/2026-10-23-code-rules-reach-writers-reviewers.md new file mode 100644 index 0000000..9144ac7 --- /dev/null +++ b/docs/plans/2026-10-23-code-rules-reach-writers-reviewers.md @@ -0,0 +1,53 @@ +--- +canonical: github:fredericrous/aval/docs/plans/2026-10-23-code-rules-reach-writers-reviewers.md +phases: [4] +status: done +branch: docs/code-rules-full-text +--- +# Code rules reach the agent that writes and the agent that reviews + +Pointer. The plan and its review panel live in aval (aval#42); the ADR is +decisions ADR-0043 (decisions#86). This repository carries Phase 4: the +implementation-review brief built by `worktree-task` F4b carries the full text +of the `governs: code` constraints and the decisions that apply to the changed +files, and the `implementation-review` agent blocks on a code-rule violation the +diff introduces, deletions included. + +## Verification record + +The two files were patched into the live `~/.claude` (only this branch's hunks: +the live `SKILL.md` carries a newer preview step not on `origin/main`, which a +`chezmoi apply` would have reverted; backups kept). Then the real +`implementation-review` agent read briefs built exactly as the new F4b step +says, on the relais branch (55 code rules in full, other constraints, +`aval relevant --top 20` for the changed file): +- Seeded diff adding `checkout_names` with `let mut v`, `for r in roots`, + `Some(n)` in `crates/relais/src/repo.rs` → blocking finding citing + `names.reveal-intent` → `Verdict: rework`, blocking, `repo.rs:115-117`, the + rule's own exception quoted and ADR-0043 named as why it blocks. +- Seeded deletion-only diff removing `crate::tooling::GUIDANCE_TIMEOUT` from a + `run_aval` call in `crates/relais/src/context/mod.rs` → blocking finding + citing `reliability.every-wait-is-bounded` at the old-side line → + `Verdict: rework`, blocking, `context/mod.rs:219` (the deleted line). +- What the drills prove: the new brief (code rules in full, decisions for the + changed files) yields blocking findings citing the right rule id at the right + line. What they do not prove: which agent text the subagent ran. The live + file already held `04ee438` (verdict wording), yet the rename drill answered + `rework` where that wording says a blocking finding under + `approve-with-changes`; Claude Code may load agent definitions at session + start, so the next session's first review is the check of the new wording. +- The brief's commands → `aval --json rules --level constraint --governs code + --full` 55 rules; `aval relevant --json --top 20` 18 ranked keys. + +## Decision log + +- 2026-10-24 — `aval resolve ` runs without `--scope`: aval's `relevant` + already reports each key's resolved verdict at the default scope, and no + fleet repository resolves scoped decisions in review today. Follow-up if one + does. + +## Implementation review + +- Round 1 approve-with-changes (31k, 34 s): verification recorded above (two + drills on the live agent), verdict wording for a locally fixable rule finding + (`04ee438`), scope kept as a recorded decision. diff --git a/dot_claude/agents/implementation-review.md b/dot_claude/agents/implementation-review.md index 5493f51..0d050f2 100644 --- a/dot_claude/agents/implementation-review.md +++ b/dot_claude/agents/implementation-review.md @@ -38,13 +38,19 @@ Check, in this order: "Tests pass" is a finding (`work.verification-loop-before-push`). A recorded check with no code path that could have produced it is a blocking finding. -3. **Fleet constraints.** The brief lists the repository's active - constraints (`aval rules --level constraint`). A violated one is a - finding that names the rule id and the `file:line` — a swallowed error +3. **Fleet constraints.** The brief carries the code rules in full and the + decisions that apply, and lists the other active constraints + (`aval rules --level constraint`). A code-rule violation introduced by + the diff, including deletions (cite the old-side `file:line`), is a + blocking finding with the rule id, which makes the verdict `rework` or + a blocking `approve-with-changes` — a swallowed error (`errors.never-swallowed`), a disabled lint or a skipped test (`general.no-disabled-safety`), a non-exhaustive match - (`types.exhaustive-matching`), a boolean flag argument, and so on. Cite - only rules the brief lists. + (`types.exhaustive-matching`), a boolean flag argument, and so on. + Unchanged pre-existing violations are outside the check. An applicable + decision in the brief outranks a rule; where decision evidence is + missing, say so under "Would still check by hand" rather than assuming + an exception. Cite only rules the brief lists. 4. **Tests.** An added test that cannot fail (no assertion, an assertion on a constant, a mock that returns what is asserted), or a changed assertion that weakens a contract the plan did not change. @@ -69,8 +75,12 @@ Would still check by hand: `; all run in the worktree (the pack + refreshed at Start); - the plan's Non-goals. 2b. **Interface review** (advisory; skipped unless the diff touches the repository's UI). Trigger: the `ui` areas from `aval traits`, matched