From 96bd051dce622ff1fd7612abcda6ec1e5de96195 Mon Sep 17 00:00:00 2001 From: Gabriele Pavanello Date: Tue, 13 Oct 2026 12:41:14 +0200 Subject: [PATCH] Only verified findings block; reviewer rules only in the agent prompt Fix two findings that `shall review` found in findings.py. JTOOL-0002.6: a finding must not block unless the verifier confirmed it. Finding.blocking returned corpus_blocking when there was no verdict, so `--no-verify`, `review.verify: false` and `shall validate` gave blocking findings that nobody verified. - Finding.blocking is true only with a `confirmed` verdict. - needs_verification reads corpus_blocking. Before verification, blocking is now false for every finding. - sort_key sorts by corpus_blocking second, so the comment cap keeps the findings that can block. - A finding without a verdict shows "Not verified - not blocking". - `shall validate` loses --fail-on-blocking: it could never fire. The gate is `shall verify --fail-on-blocking`. - /code-review: `shall review --fail-on-blocking` with verification off can never fail (cli.py cmd_review, `verify` false). It now prints a warning. JTOOL-0004.2: review instructions must live only in agents/shall-reviewer.md. chunk_prompt repeated rules 1, 2 and 4 of that file (only violations, empty list, line anchors). chunk_prompt now sends only data. The bundle header tells the steps (reviewer, validate, verify) and gives no review rules. Eval: same Haiku selection on the 11 eval/cases, Sonnet reviewer, 2 runs per prompt, no cache. Recall 0.868 in all 4 runs. Precision old 0.825/0.786, new 0.805/0.846. Dropped by validation old 1/1, new 1/0. The change is inside the noise. Plugin version 0.4.2: a skill reference changed. Co-Authored-By: Claude Opus 5.5 --- docs/architecture.mdx | 2 +- docs/configuration.mdx | 2 +- plugins/shall/.claude-plugin/plugin.json | 2 +- .../shall-review/references/in-session.md | 2 +- plugins/shall/tooling/src/shall/cli.py | 12 ++++-- plugins/shall/tooling/src/shall/findings.py | 42 +++++++++---------- plugins/shall/tooling/src/shall/verify.py | 5 ++- .../shall/tooling/tests/test_boundaries.py | 16 +++++++ plugins/shall/tooling/tests/test_verify.py | 19 +++++++++ 9 files changed, 69 insertions(+), 33 deletions(-) diff --git a/docs/architecture.mdx b/docs/architecture.mdx index b526e33..d829803 100644 --- a/docs/architecture.mdx +++ b/docs/architecture.mdx @@ -78,7 +78,7 @@ The reviewer sees one chunk. A finding can be wrong because of code in another p shall parses files lazily (`git grep` first). It marks hops that it resolved only by name with "(by name)". A keyword search runs at the same time. The search uses the YAML key path, `depends_on`, code spans of the statement and identifiers of the quote (code before docs). The search also covers files that have no grammar. For a diff, it also covers the file of the finding, except the lines that the verifier sees as the chunk: a diff shows only the changed hunks, and the setting that decides a finding (a variable, a client configuration) is frequently in another part of the same file. 3. **Jev filters the excerpts** when there are more than 6 (one Noul per excerpt). 4. **The `shall-verifier` agent** (no tools) answers confirmed / refuted / unknown. It must cite excerpt ids. -5. **Code applies the verdict. Severity only goes down.** Refuted → code drops the finding only if the cited evidence exists. Unknown or confirmed-without-evidence → advisory. Confirmed → unchanged. +5. **Code applies the verdict. Severity only goes down.** Refuted → code drops the finding only if the cited evidence exists. Unknown or confirmed-without-evidence → advisory. Confirmed → unchanged. A finding without a verdict (`--no-verify`, `review.verify: false`, or before `shall verify`) is advisory: only a confirmed finding blocks. Limits: shall resolves by imports and names, not by types. A language server or a SCIP index would resolve `this.partner.notify` by type. The graph cannot see dependency injection, reflection, and clients configured in YAML or environment variables. diff --git a/docs/configuration.mdx b/docs/configuration.mdx index 51fa761..f5ff3fa 100644 --- a/docs/configuration.mdx +++ b/docs/configuration.mdx @@ -51,7 +51,7 @@ The organisation sets the settings in `shall.yaml`. A repository can change them | `selection.include_status` | `[approved, enforced]` | The statuses that reviews use. `--include-draft` adds drafts. | | `selection.facts` | `true` | Add known effects to the input of Jev and to the review | | `review.model` | `claude-sonnet-5` | The model of the reviewer and the verifier | -| `review.verify` | `true` | Verify blocking findings against the repository before they block | +| `review.verify` | `true` | Verify blocking findings against the repository. Only a verified finding blocks: with `false` or `--no-verify`, all findings are advisory. | | `review.cache` | `true` | Use the earlier answer of Claude again when its input is the same. `--no-cache` forces new calls. | | `review.max_comments` | `25` | The maximum number of comments for each review | | `conflicts.fail_threshold` | `0.75` | `conflicts --local` fails at this value or higher. | diff --git a/plugins/shall/.claude-plugin/plugin.json b/plugins/shall/.claude-plugin/plugin.json index 4efce36..6b98eb0 100644 --- a/plugins/shall/.claude-plugin/plugin.json +++ b/plugins/shall/.claude-plugin/plugin.json @@ -1,6 +1,6 @@ { "name": "shall", - "version": "0.4.1", + "version": "0.4.2", "description": "shall engineering standards: author and maintain RFC-2119 standards, build the deterministic index, find which statements apply to a diff/spec/doc/task with Jev, review against them, lint whole files (scan), and check an agent's tool calls against them while it works (hooks).", "author": { "name": "gpavanello" }, "keywords": ["standards", "rfc", "review", "governance", "jev", "typesafe"] diff --git a/plugins/shall/skills/shall-review/references/in-session.md b/plugins/shall/skills/shall-review/references/in-session.md index 03c8967..1d84097 100644 --- a/plugins/shall/skills/shall-review/references/in-session.md +++ b/plugins/shall/skills/shall-review/references/in-session.md @@ -9,7 +9,7 @@ Same pipeline as `shall review`, with the reviewer as an in-session agent. `.shall-out/pr/findings.agent.json` as `{"findings": [{"chunk": "c0", ...}]}`. 4. `shall validate .shall-out/pr/findings.agent.json /tmp/pr.diff --selection .shall-out/pr/selection.json --out-dir .shall-out/pr/validated` (drops findings whose statement was not selected for the chunk or whose line is not changed, - dedupes, recomputes blocking, caps comments) + dedupes, caps comments; nothing blocks before step 5) 5. `shall verify .shall-out/pr/validated/findings.json /tmp/pr.diff --selection .shall-out/pr/selection.json --out-dir .shall-out/pr` from the repository root (gathers evidence; refuted findings drop only with cited evidence, unknown ones stay advisory). diff --git a/plugins/shall/tooling/src/shall/cli.py b/plugins/shall/tooling/src/shall/cli.py index e776cd1..7c6197e 100644 --- a/plugins/shall/tooling/src/shall/cli.py +++ b/plugins/shall/tooling/src/shall/cli.py @@ -355,8 +355,9 @@ def cmd_validate(args, cfg) -> ExitCode: raw = json.loads(Path(args.findings).read_text(encoding="utf-8")) raw = raw.get("findings", raw) if isinstance(raw, dict) else raw findings, dropped = validate_findings(corpus, artifact, selection, raw, int(cfg.get("review.max_comments"))) - blocking = _write_outputs(cfg, Path(args.out_dir), artifact, selection, findings, dropped, args.format) - return ExitCode.BLOCKING if (blocking and args.fail_on_blocking) else ExitCode.OK + # nothing blocks before `shall verify` (JTOOL-0002.6): no --fail-on-blocking here + _write_outputs(cfg, Path(args.out_dir), artifact, selection, findings, dropped, args.format) + return ExitCode.OK def cmd_review(args, cfg) -> ExitCode: @@ -368,6 +369,9 @@ def cmd_review(args, cfg) -> ExitCode: corpus = load_corpus(cfg) out_dir = Path(args.out_dir) verify = cfg.get("review.verify") and not args.no_verify + if args.fail_on_blocking and not verify: + print("shall review: verification is off, so no finding blocks and --fail-on-blocking " + "cannot fail (JTOOL-0002.6)", file=sys.stderr) # incremental re-review: unchanged chunks and unchanged evidence reuse earlier answers cache = answer_cache(cfg, enabled=not args.no_cache) @@ -805,7 +809,8 @@ def artifact_args(s): s.add_argument("--format", choices=["markdown", "github"], default="markdown") s.add_argument("--fail-on-blocking", action="store_true", help=f"exit {ExitCode.BLOCKING} on blocking findings") s.add_argument("--dry-run", action="store_true", help="select only and write bundle.md, no agent call") - s.add_argument("--no-verify", action="store_true", help="skip verification of blocking findings") + s.add_argument("--no-verify", action="store_true", + help="skip verification: no finding blocks, blocking findings stay advisory") s.set_defaults(fn=cmd_review) s = sub.add_parser("scan", help="lint whole files: secret scanner + Jev per file (warnings, no agent)") @@ -842,7 +847,6 @@ def artifact_args(s): s.add_argument("--selection", required=True) s.add_argument("--out-dir", default=".shall-out") s.add_argument("--format", choices=["markdown", "github"], default="markdown") - s.add_argument("--fail-on-blocking", action="store_true") s.set_defaults(fn=cmd_validate) s = sub.add_parser("publish", help="post a review to a GitHub PR idempotently (via gh)") diff --git a/plugins/shall/tooling/src/shall/findings.py b/plugins/shall/tooling/src/shall/findings.py index 77cea90..94e999a 100644 --- a/plugins/shall/tooling/src/shall/findings.py +++ b/plugins/shall/tooling/src/shall/findings.py @@ -6,7 +6,7 @@ -> validate_findings statement selected, line or quote found (JTOOL-0002.2) Finding severity from the corpus (corpus_blocking) -> Finding.with_verification(Verification) (verify.py) - Finding.blocking computed, never stored: verification can only lower it (JTOOL-0002.5, .6) + Finding.blocking computed, never stored: only a `confirmed` verdict keeps it (JTOOL-0002.5, .6) Only this module makes a Finding from data (tests/test_boundaries.py checks it). `from_json` reads back a findings.json that shall itself wrote; agent output always goes through `validate_findings`. @@ -103,12 +103,13 @@ class Finding: @property def blocking(self) -> bool: - """The corpus decides; a verification that is not `confirmed` can only lower it.""" - return self.corpus_blocking and (self.verification is None or self.verification.verdict == "confirmed") + """The corpus decides, and only a `confirmed` verdict keeps it: a finding that was not + verified (`--no-verify`, `review.verify: false`, `shall validate`) is advisory (JTOOL-0002.6).""" + return self.corpus_blocking and self.verification is not None and self.verification.verdict == "confirmed" @property def downgraded(self) -> bool: - """Blocking by the corpus, advisory because its evidence was not seen.""" + """Blocking by the corpus, advisory because no verifier confirmed it.""" return self.corpus_blocking and not self.blocking def with_verification(self, verification: Verification) -> Finding: @@ -125,7 +126,7 @@ def to_json(self) -> dict: if self.verification is not None: out["verification"] = self.verification.to_json() if self.downgraded: - out["downgraded"] = True # evidence not seen: a comment, never a failed check + out["downgraded"] = True # not confirmed: a comment, never a failed check return out @classmethod @@ -151,7 +152,9 @@ def read_findings(path: Path) -> list[Finding]: def sort_key(f: Finding) -> tuple: - return (not f.blocking, f.path, f.line or 0, f.statement_id) + # corpus_blocking second: before verification nothing blocks, and the comment cap + # (validate_findings) must still keep the findings that can block + return (not f.blocking, not f.corpus_blocking, f.path, f.line or 0, f.statement_id) # ---------------------------------------------------------------- prompts and bundle @@ -168,20 +171,14 @@ def render_statements(corpus: Corpus, ids: list[str]) -> str: def chunk_prompt(corpus: Corpus, chunk: Chunk, ids: list[str], facts: list[str] | None = None) -> str: - anchor = ( - "Only lines marked '+' (added) can carry a finding; use their line number." - if chunk.kind == "diff" else - "Use the line number shown before '|'. Use line null for a finding about something missing from the whole document." - ) + """The data of one review call. The instructions are only in agents/shall-reviewer.md (JTOOL-0004.2).""" return ( f"\n" f"{chunk.rendered}\n\n\n" + (f"\nFacts found by code (effects registry and code graph) about what the " f"code in the chunk calls:\n" + "\n".join(f"- {x}" for x in facts) + "\n\n\n" if facts else "") - + f"\n{render_statements(corpus, ids)}\n\n\n" - f"{anchor}\nReturn only violations of the standards listed above. " - "Return an empty findings list when the chunk complies." + + f"\n{render_statements(corpus, ids)}\n" ) @@ -191,10 +188,9 @@ def write_bundle(corpus: Corpus, artifact: Artifact, selection: dict, out: Path) parts = [ f"# shall review bundle — {artifact.source}", "", - "For each chunk, check only the listed standards. Write all findings to one JSON file", - "with the shape `{\"findings\": [{\"chunk\": \"c0\", \"statement_id\": ..., \"line\": ..., " - "\"quote\": ..., \"message\": ..., \"suggestion\": ...}]}`, then run", - f"`shall validate --selection {artifact.source}`.", + "Give each chunk section below to the `shall-reviewer` agent. Collect its findings in one JSON", + "file as `{\"findings\": [{\"chunk\": \"c0\", ...}]}`, then run `shall validate` and `shall verify`", + "(skill shall-review, references/in-session.md). Only a verified finding blocks.", "", ] for c in selection["chunks"]: @@ -283,7 +279,7 @@ def validate_findings(corpus: Corpus, artifact: Artifact, selection: dict, raw: def verification_line(f: Finding) -> str: v = f.verification if v is None: - return "" + return "_Not verified — not blocking._" if f.corpus_blocking else "" where = f" ({', '.join(v.evidence)})" if v.evidence else "" if v.verdict == "confirmed": return f"_Verified{where}:_ {v.reason}" @@ -317,8 +313,8 @@ def render_markdown(artifact: Artifact, selection: dict, findings: list[Finding] out.append(f" {f.message}") if f.suggestion: out.append(f" _Suggestion:_ {f.suggestion}") - if f.verification: - out.append(f" {verification_line(f)}") + if note := verification_line(f): + out.append(f" {note}") return "\n".join(out).rstrip() + "\n" @@ -340,8 +336,8 @@ def comment_body(f: Finding, key: str | None = None, lhash: str | None = None) - body = f"{tag} · **{f.statement_id}** {f.title} ({f.level})\n\n{f.message}" if f.suggestion: body += f"\n\n**Suggestion:** {f.suggestion}" - if f.verification: - body += f"\n\n{verification_line(f)}" + if note := verification_line(f): + body += f"\n\n{note}" body += f"\n\n> {f.statement_text}\n\nStandard: `{f.source}`" if key: body += f"\n\n" diff --git a/plugins/shall/tooling/src/shall/verify.py b/plugins/shall/tooling/src/shall/verify.py index ffcfee8..cf82408 100644 --- a/plugins/shall/tooling/src/shall/verify.py +++ b/plugins/shall/tooling/src/shall/verify.py @@ -2,7 +2,8 @@ The reviewer sees one chunk. A finding such as "claude runs with tools" can be wrong because another file adds `--tools ""`. Which findings are verified is decided by code, not by the -reviewer: every blocking finding, plus advisory findings that declare `depends_on`. +reviewer: every finding the corpus marks blocking (it blocks only when confirmed), plus advisory +findings that declare `depends_on`. terms (depends_on + identifiers of the quote) code -> excerpts from the repository (git grep) code @@ -88,7 +89,7 @@ class Claim: def needs_verification(finding: Finding) -> bool: - return finding.blocking or bool(finding.depends_on) + return finding.corpus_blocking or bool(finding.depends_on) CODE_SPAN_RE = re.compile(r"`([^`]+)`") diff --git a/plugins/shall/tooling/tests/test_boundaries.py b/plugins/shall/tooling/tests/test_boundaries.py index 934e3d3..a743b0b 100644 --- a/plugins/shall/tooling/tests/test_boundaries.py +++ b/plugins/shall/tooling/tests/test_boundaries.py @@ -82,6 +82,22 @@ def test_findings_schema_matches_raw_finding(): "agent returns, RawFinding is how code reads it.") +def test_review_instructions_live_only_in_the_agent_prompt(): + """JTOOL-0004.2: code sends the reviewer data only; its rules are in agents/shall-reviewer.md, + which the in-session agent and CI both use. A second copy in code drifts from the first.""" + from shall.artifact import build_artifact + from shall.corpus import load_corpus + from shall.findings import chunk_prompt + corpus = load_corpus(load_config(str(REPO / "shall.yaml"))) + diff = "diff --git a/a.py b/a.py\n--- a/a.py\n+++ b/a.py\n@@ -0,0 +1 @@\n+print(1)\n" + for name, raw, kind in (("a.diff", diff, "diff"), ("a.md", "# Title\n\nText.\n", "doc")): + chunk = build_artifact(name, raw, kind, 4000).chunks[0] + prompt = chunk_prompt(corpus, chunk, ["JRFC-0003.1"], ["a fact"]) + rest = re.sub(r"<(chunk|known_effects|standards)\b.*?", "", prompt, flags=re.S).strip() + assert not rest, (f"chunk_prompt ({kind}) has text outside its data tags: {rest!r}. Put review " + "rules in plugins/shall/agents/shall-reviewer.md (JTOOL-0004.2)") + + def test_every_claude_call_is_tool_less(): cfg = type("Cfg", (), {"get": lambda self, key: ["claude", "-p"]})() for schema in (None, {"type": "object"}): diff --git a/plugins/shall/tooling/tests/test_verify.py b/plugins/shall/tooling/tests/test_verify.py index ae01f75..5083c81 100644 --- a/plugins/shall/tooling/tests/test_verify.py +++ b/plugins/shall/tooling/tests/test_verify.py @@ -105,6 +105,25 @@ def test_confirmed_without_existing_evidence_does_not_block(): assert kept.verification is not None and kept.verification.verdict == "unknown" +def test_a_finding_that_was_not_verified_never_blocks(): + """JTOOL-0002.6: `--no-verify`, `review.verify: false` and `shall validate` give no verdict, + so the exit code, the GitHub review and the Stop hook see an advisory finding.""" + from shall.findings import Finding, render_github, render_markdown, sort_key + f = finding() + assert f.blocking is False and f.downgraded is True + assert needs_verification(f) # the corpus still decides what verification looks at + data = f.to_json() + assert data["blocking"] is False and data["corpus_blocking"] is True + assert Finding.from_json(data).blocking is False # findings.json read back (Stop hook) stays advisory + artifact = build_artifact("shall.yaml", "jev:\n command: [claude, -p]\n", "code", 4000) + selection = {"applicable": [], "model": "m", "strategy": "flat"} + assert render_github(artifact, selection, [f], [])["event"] == "COMMENT" + assert "Not verified — not blocking" in render_markdown(artifact, selection, [f], []) + # before verification nothing blocks: the comment cap must still keep what can block + advisory = finding(corpus_blocking=False, path="a.yaml") + assert sorted([advisory, f], key=sort_key) == [f, advisory] + + def test_missing_verdict_counts_as_unknown(): kept, _ = apply_verdict(finding(), None, []) assert kept is not None and kept.blocking is False