Merge pull request #91 from eduralph/feat/64-advisory-reviewers

feat(check): generalized advisory reviewer leaves

Author: eduralph

docs/05-check.md

@@ -104,6 +104,18 @@ hands it over as **`$PDCA_TARGET`** (read-only; for a `claude` reviewer also via
 into other checkouts on the machine — without this it can't ground, or hunts the
 filesystem for "the target" (issue #75).
 
+### Optional advisory reviewers (a second lens)
+
+The `reviewer` judges fix *adequacy*. For other lenses — correctness bugs the patch
+introduces, reuse/simplification/efficiency cleanups — add **advisory reviewer leaves**
+(issue #64): an open `[[leaves.advisory]]` list in `pdca.toml`, each a role-distinct,
+model-agnostic (`family` + `argv`) leaf. Each writes `check-advisory-<id>.md`; its
+`- NEEDS-HUMAN —` findings fold into §6 like the reviewer's. They are **always advisory**
+(never gate). Condition one on a brief field with `when = { field = …, substring = … }`
+(e.g. run a deeper review only when the brief says so) — the way gate targets condition
+on the bundle. A shipped `code-review` agent realizes the correctness+cleanup lens for a
+`claude` instance; `family = "codex"` swaps the vendor.
+
 ## 3. Assembly — the SUMMARY the human signs
 
 The driver folds brief + gates + review into `SUMMARY.md`, a 10-section document.

template/.claude/agents/code-review.md.jinja

@@ -0,0 +1,36 @@
+---
+name: code-review
+description: >-
+  Optional ADVISORY code reviewer for {{ project_name }} (issue #64) — a lens the
+  `reviewer` leaf does not cover: correctness bugs the patch introduces, and
+  reuse / simplification / efficiency cleanups in the diff. Advisory only; it never
+  gates. Execute + read, no write to the fix. Invoke as a configured advisory leaf.
+tools: Read, Bash, Grep, Glob
+model: inherit
+---
+
+# Code review (Check, advisory — issue #64)
+
+A **second lens** on the patch, distinct from the `reviewer` leaf (which judges fix
+*adequacy* — causal adequacy, scope, validation). You hunt for:
+
+- **Correctness bugs the patch introduces** — off-by-one, error/edge-case handling,
+  resource leaks, concurrency, API misuse, a test that doesn't actually exercise the fix.
+- **Reuse / simplification / efficiency** — duplicated logic that an existing helper
+  already covers, a simpler equivalent, needless work in a hot path.
+
+You are **advisory: you never gate accept.** Deterministic gates block; you annotate.
+
+## Inputs
+
+`{patch.diff, brief.md, check-gates.json}` only — **not** `build-notes.md` (don't anchor
+on the builder's framing). Ground every cited `path:line` on the **target source at
+`$PDCA_TARGET`** (read-only; the driver resolves and adds it); do not search other
+checkouts. You have **no Write/Edit** — you cannot patch what you judge.
+
+## Output — `check-advisory-code-review.md`
+
+A short list of findings, each a Markdown bullet citing `path:line`. For any finding a
+human must adjudicate, prefix the bullet `- NEEDS-HUMAN — ` (the harness lifts those into
+`SUMMARY.md` §6). Scope each finding to **this diff** — don't file pre-existing debt the
+patch didn't touch. If the diff is clean on both lenses, say so explicitly.

template/pdca.toml.jinja

@@ -132,6 +132,22 @@ mode = "{{ leaves_mode }}"
 family = "claude"
 interactive = true
 argv = ["claude", "--agent", "publisher", "--permission-mode", "acceptEdits"]
+
+# Optional advisory reviewer leaves (issue #64) — an OPEN list. The `reviewer` leaf above
+# judges fix ADEQUACY (causal adequacy, scope, validation); add role-distinct ADVISORY
+# reviewers here for other lenses (e.g. correctness bugs the patch introduces + reuse /
+# simplification / efficiency cleanups). Each writes check-advisory-<id>.md; its
+# `- NEEDS-HUMAN —` findings route into SUMMARY §6. Always advisory — never gates.
+# Decorrelate: prefer a `family` distinct from the builder AND the reviewer where you can.
+# `when = { field = "<brief field>", substring = "<v>" }` runs it only when that brief
+# field matches (e.g. a "Review depth" field an Act rule tunes) — omit to always run.
+# [[leaves.advisory]]
+# id = "code-review"
+# role = "correctness bugs introduced by the patch + reuse/simplification/efficiency cleanups"
+# mode = "{{ leaves_mode }}"
+# family = "claude"
+# argv = ["claude", "-p", "--agent", "code-review", "--permission-mode", "acceptEdits", "--allowedTools", "Read,Bash,Grep,Glob"]
+# when = { field = "review depth", substring = "deep" }
 # Deterministic Check gates (docs 04). Each check runs a shell command; it PASSES
 # iff the command exits 0. `scope = "repo"` runs against the working tree (and is
 # what CI re-runs via `pdca gates --working-tree`); `scope = "bundle"` needs the

template/src/pdca_harness/assemble.py

@@ -28,10 +28,23 @@ def assemble_summary(d: Path, cfg: Config) -> None:
         if review_path.exists()
         else _missing_review_text()
     )
-    # §6 is fed by the reviewer's NEEDS-HUMAN verdicts AND any gate that declared itself
-    # unverifiable (issue #46) — both become `- [ ]` items the C6 guard makes the human
-    # clear before accept.
-    needs_human = _needs_human(review_text) + _unverifiable_items(gates)
+    # Optional advisory reviewers (issue #64): each check-advisory-<id>.md is folded into
+    # §5 and its NEEDS-HUMAN findings into §6, exactly like the main reviewer.
+    advisory_paths = sorted(d.glob("check-advisory-*.md"))
+    advisory_texts = [p.read_text(encoding="utf-8") for p in advisory_paths]
+
+    # §6 is fed by the reviewer's NEEDS-HUMAN verdicts, the advisory reviewers', AND any
+    # gate that declared itself unverifiable (issue #46) — all become `- [ ]` items the C6
+    # guard makes the human clear before accept.
+    needs_human = _needs_human(review_text)
+    for atext in advisory_texts:
+        needs_human += _needs_human(atext)
+    needs_human += _unverifiable_items(gates)
+
+    advisory_block = "\n".join(
+        f"\n### Advisory — {p.stem.removeprefix('check-advisory-')}\n\n{t.strip()}"
+        for p, t in zip(advisory_paths, advisory_texts)
+    )
 
     issue = d.name.replace("issue_", "")
     out = "\n".join(
@@ -60,6 +73,7 @@ def assemble_summary(d: Path, cfg: Config) -> None:
             "Reviewer ran without build-notes.md. Summary:",
             "",
             review_text.strip(),
+            advisory_block,
             "",
             "## 6. NEEDS-HUMAN — items the human must clear before sign-off",
             _needs_human_block(needs_human),

template/src/pdca_harness/config.py

@@ -90,6 +90,14 @@ class Config:
     issue_trailer: str = "Fixes #{id}"  # commit/PR trailer; "" → none enforced
     repo_checkouts: dict[str, str] = field(default_factory=dict)  # repo_spec → local path
     gates_checks: list[dict] = field(default_factory=list)
+    # Optional advisory reviewer leaves (issue #64): an OPEN list of extra, role-distinct
+    # advisory reviewers ([[leaves.advisory]] in pdca.toml), so an instance adds N of them
+    # (e.g. a correctness/cleanup code-review lens) with no driver change. Each:
+    # {id, role, family, mode, argv, when?}. Always advisory (never gates); their
+    # NEEDS-HUMAN findings route into SUMMARY §6. ``when`` ({field, substring}) conditions
+    # a leaf on a brief field (e.g. a "Review depth" field), the way gate targets do — empty
+    # ⇒ always run.
+    advisory_leaves: list[dict] = field(default_factory=list)
     # Delegated gates (issue #67): a host runner that single-sources its own gates
     # (e.g. "cargo xtask"). A check's bare ``subcmd`` is run as ``<runner> <subcmd>``, so
     # PDCA orchestrates the host runner instead of re-declaring the gates. "" ⇒ inline only.
@@ -171,6 +179,13 @@ def leaf(name: str) -> LeafConfig:
                 interactive=bool(d.get("interactive", False)),
             )
 
+        # Advisory reviewer leaves (issue #64) — an open list under [[leaves.advisory]].
+        # PDCA_LEAVES_MODE forces their mode too (CI / offline determinism).
+        advisory_leaves = [
+            {**spec, "mode": mode_override or spec.get("mode", "stub")}
+            for spec in leaves.get("advisory", [])
+        ]
+
         # PDCA_BUNDLE_ROOT redirects bundles to a throwaway location so an offline
         # `rehearse` never collides with the real `results/` a live run would use.
         bundle_root = root / paths.get("bundle_root", "results")
@@ -218,6 +233,7 @@ def leaf(name: str) -> LeafConfig:
             act=leaf("act"),
             author=data.get("project", {}).get("author", ""),
             gates_checks=gates_checks,
+            advisory_leaves=advisory_leaves,
             gates_runner=gates_runner,
             lanes=lanes,
             close_dispositions=close_dispositions,

template/src/pdca_harness/driver.py

@@ -64,6 +64,9 @@ def advance(d: Path, cfg: Config) -> None:
             gates.run_gates(d, cfg)  # deterministic gates
             _say(f"→ {d.name}: Check — advisory reviewer{_headless_note(cfg.reviewer)}…")
             leaves.run_review(d, cfg)  # leaf 2 — reviewer (advisory)
+            if cfg.advisory_leaves:  # optional extra advisory reviewers (issue #64)
+                _say(f"→ {d.name}: Check — advisory reviewers ({len(cfg.advisory_leaves)})…")
+                leaves.run_advisory_leaves(d, cfg)
     elif s == state.CHECKED:
         _say(f"→ {d.name}: assembling SUMMARY…")
         assemble.assemble_summary(d, cfg)  # pure code → SUMMARY.md §1–8
@@ -237,6 +240,7 @@ def _archive_iteration(d: Path, n: int, *, include_brief: bool) -> None:
     """
     arch = d / f"iteration-v{n}"
     names = list(DOWNSTREAM_OF_BRIEF)
+    names += [p.name for p in d.glob("check-advisory-*.md")]  # advisory artifacts (#64)
     if include_brief:
         names.append("brief.md")
     if (d / "brief.md").exists():

template/src/pdca_harness/leaves.py

@@ -487,6 +487,104 @@ def _stub_review(d: Path, cfg: Config) -> None:
     )
 
 
+# ----------------------------------------------------------------------------
+# Optional advisory reviewer leaves (issue #64) — an OPEN, role-distinct set of extra
+# advisory reviewers (e.g. a correctness-bug + reuse/cleanup code-review lens), each a
+# reviewer-shaped leaf. Always advisory: they write check-advisory-<id>.md and their
+# NEEDS-HUMAN findings route into SUMMARY §6; they never gate. Conditioned per-bundle by
+# an optional ``when`` ({field, substring}) brief match — empty ⇒ always run.
+# ----------------------------------------------------------------------------
+def advisory_artifact(d: Path, leaf_id: str) -> Path:
+    """The artifact path an advisory leaf writes (parallel to check-review.md)."""
+    return d / f"check-advisory-{leaf_id}.md"
+
+
+def _advisory_applies(spec: dict, d: Path) -> bool:
+    """True iff this advisory leaf should run for bundle ``d``. ``when`` ({field,
+    substring}) matches a brief field case-insensitively (like a gate target flag);
+    absent ⇒ always run."""
+    when = spec.get("when") or {}
+    needle = (when.get("substring") or "").lower()
+    if not needle:
+        return True
+    return needle in brief.field(d / "brief.md", when.get("field", "")).lower()
+
+
+def _advisory_prompt(spec: dict, leaf_id: str) -> str:
+    role = spec.get("role") or "review the patch for correctness bugs and reuse / " \
+        "simplification / efficiency cleanups"
+    return (
+        f"You are an ADVISORY code reviewer — lens: {role}. You have ONLY patch.diff, "
+        "brief.md and check-gates.json here (build-notes.md is withheld); ground every "
+        "cited path:line on the target source at $PDCA_TARGET, never other checkouts. "
+        f"Write check-advisory-{leaf_id}.md: a short list of findings, each a Markdown "
+        "bullet with a path:line. For any finding a human must adjudicate, prefix the "
+        "bullet '- NEEDS-HUMAN — ' (it becomes a SUMMARY §6 item). You are ADVISORY — you "
+        "never gate; the human decides at sign-off. If you find nothing, say so explicitly."
+    )
+
+
+def run_advisory_leaves(d: Path, cfg: Config) -> None:
+    """Run each configured advisory reviewer that applies (issue #64). Each writes
+    check-advisory-<id>.md; failures degrade to a §6 NEEDS-HUMAN placeholder, never crash
+    the cycle (advisory, like the main reviewer)."""
+    for spec in cfg.advisory_leaves:
+        leaf_id = spec.get("id") or "advisory"
+        if not _advisory_applies(spec, d):
+            continue
+        leaf = LeafConfig(mode=spec.get("mode", "stub"), family=spec.get("family", ""),
+                          argv=list(spec.get("argv", [])))
+        if leaf.mode == "command":
+            _run_advisory_sandboxed(d, cfg, leaf, spec, leaf_id)
+        else:
+            _stub_advisory(d, spec, leaf_id)
+
+
+def _run_advisory_sandboxed(d: Path, cfg: Config, leaf: LeafConfig, spec: dict, leaf_id: str) -> None:
+    """Run one advisory leaf in a temp dir holding ONLY the reviewer inputs (the same
+    independence sandbox as the main reviewer), grounding on $PDCA_TARGET (#75)."""
+    with tempfile.TemporaryDirectory(prefix="pdca-advisory-") as tmp:
+        sandbox = Path(tmp)
+        for name in REVIEWER_INPUTS:
+            if (d / name).exists():
+                shutil.copy2(d / name, sandbox / name)
+        target = _reviewer_target(d, cfg)
+        env = {"PDCA_TARGET": str(target)} if target else None
+        extra = ["--add-dir", str(target)] if target and leaf.family == "claude" else None
+        out = sandbox / f"check-advisory-{leaf_id}.md"
+        try:
+            _invoke(leaf, sandbox, _advisory_prompt(spec, leaf_id),
+                    label=f"Advisory {leaf_id} {d.name}",
+                    status=lambda: progress.bundle_activity(sandbox, (out.name,)),
+                    stream_json=True, env=env, extra_argv=extra)
+        except Exception as exc:  # noqa: BLE001 — advisory must never crash the cycle
+            _advisory_unavailable(d, leaf_id, f"leaf failed: {exc}")
+            return
+        if out.exists():
+            shutil.copy2(out, advisory_artifact(d, leaf_id))
+        else:
+            _advisory_unavailable(d, leaf_id, "produced no artifact")
+
+
+def _stub_advisory(d: Path, spec: dict, leaf_id: str) -> None:
+    role = spec.get("role") or "correctness bugs + reuse/simplification cleanups"
+    advisory_artifact(d, leaf_id).write_text(
+        f"# Advisory review — {leaf_id} (stub)\n\nLens: {role}.\n\n"
+        "- NEEDS-HUMAN — advisory code-review lens is a stub here; a real "
+        f"`{leaf_id}` leaf (family/argv in [[leaves.advisory]]) reviews the patch and "
+        "lists findings. The human adjudicates at sign-off.\n",
+        encoding="utf-8")
+
+
+def _advisory_unavailable(d: Path, leaf_id: str, reason: str) -> None:
+    print(f"leaves: {d.name} — advisory '{leaf_id}' unavailable ({reason})", file=sys.stderr)
+    advisory_artifact(d, leaf_id).write_text(
+        f"# Advisory review — {leaf_id} — NOT COMPLETED\n\n"
+        f"- NEEDS-HUMAN — advisory leaf '{leaf_id}' did not produce findings ({reason}); "
+        "re-run it or adjudicate by hand.\n",
+        encoding="utf-8")
+
+
 # ----------------------------------------------------------------------------
 # Leaf 3 — Check sign-off (signoff, interactive): Claude + human reach the OK.
 # ----------------------------------------------------------------------------

template/tests/test_driver_slice.py

@@ -310,6 +310,52 @@ def boom(*a, **k):
                       (self.d / "check-review.md").read_text(encoding="utf-8"))
 
 
+class AdvisoryReviewers(unittest.TestCase):
+    """Optional advisory reviewer leaves (issue #64): an open, role-distinct set that
+    write check-advisory-<id>.md and route NEEDS-HUMAN into §6; conditioned by `when`."""
+
+    def setUp(self) -> None:
+        self.tmp = Path(tempfile.mkdtemp())
+        self.cfg = _stub_config(self.tmp)
+        self.d = self.cfg.bundle("ADV")
+        self.d.mkdir(parents=True)
+        shutil.copyfile(TOY_BRIEF, self.d / "brief.md")
+
+    def tearDown(self) -> None:
+        shutil.rmtree(self.tmp, ignore_errors=True)
+
+    def test_advisory_leaf_runs_and_routes_to_section6(self) -> None:
+        self.cfg.advisory_leaves = [{"id": "code-review", "role": "bugs+cleanups", "mode": "stub"}]
+        driver.run_issue(self.d, self.cfg)
+        self.assertTrue((self.d / "check-advisory-code-review.md").exists())
+        summary = self.d / "SUMMARY.md"
+        self.assertIn("Advisory — code-review", summary.read_text(encoding="utf-8"))  # §5
+        items = signoff.open_needs_human(summary)  # the advisory NEEDS-HUMAN → §6
+        self.assertTrue(any("advisory" in it.lower() for it in items))
+
+    def test_when_condition_skips_non_matching(self) -> None:
+        self.cfg.advisory_leaves = [{"id": "deep", "role": "x", "mode": "stub",
+                                     "when": {"field": "review depth", "substring": "deep"}}]
+        driver.run_issue(self.d, self.cfg)  # toy brief has no "Review depth" → skipped
+        self.assertFalse((self.d / "check-advisory-deep.md").exists())
+
+    def test_when_condition_runs_on_match(self) -> None:
+        bp = self.d / "brief.md"
+        bp.write_text(bp.read_text() + "\n- **Review depth:** deep\n", encoding="utf-8")
+        self.cfg.advisory_leaves = [{"id": "deep", "role": "x", "mode": "stub",
+                                     "when": {"field": "review depth", "substring": "deep"}}]
+        driver.run_issue(self.d, self.cfg)
+        self.assertTrue((self.d / "check-advisory-deep.md").exists())
+
+    def test_iterate_archives_advisory_artifact(self) -> None:
+        self.cfg.advisory_leaves = [{"id": "code-review", "role": "x", "mode": "stub"}]
+        driver.run_issue(self.d, self.cfg)
+        signoff.record(self.d / "SUMMARY.md", action="iterate-do", by="t", date="2026-01-01")
+        driver.advance(self.d, self.cfg)  # archive the attempt
+        self.assertFalse((self.d / "check-advisory-code-review.md").exists())
+        self.assertTrue((self.d / "iteration-v1" / "check-advisory-code-review.md").exists())
+
+
 class ConfiguredGates(unittest.TestCase):
     """The config-driven, single-sourced gates (docs 04)."""