From 2f5f2620b31c47e9b2d57d8ad507cd19e6b8fc16 Mon Sep 17 00:00:00 2001 From: n Date: Wed, 5 Aug 2026 23:47:43 -0500 Subject: [PATCH] Verify review findings in the container instead of on the host The host skill read every lens finding against the code to decide what to post, which put a judgement about code in the session that has no lens discipline and no worktree. In practice it drifted further than that: with an examine run that produced nothing, reading the diff and forming findings directly looked like the reasonable way to fill the gap. Findings now go to /crypter-devcontainer-verify, which already rules on findings from anywhere else and already rejects preferences and findings about untouched code. It grows a second input shape for this: given the lenses' own directory it assigns ids per lens rather than expecting a collected report, so the host hands over a path and reads nothing until verdicts exist. Only findings that hold reach the pull request. One that a verifier ruled against is the pipeline checking itself, and it stays in .claude/runs where a later pass can read it, rather than costing the author a read to reach a conclusion the verifier already reached. Both container calls exit 0 whichever way they go, so each step now checks what was written instead of trusting the status, and distinguishes an empty result that means success from one that means the run never happened. Co-Authored-By: Claude Opus 5 --- .../crypter-devcontainer-verify/SKILL.md | 21 +++- .claude/skills/crypter-review/SKILL.md | 105 ++++++++++++++---- .../Agentic Development Pipeline.md | 18 ++- 3 files changed, 115 insertions(+), 29 deletions(-) diff --git a/.claude/skills/crypter-devcontainer-verify/SKILL.md b/.claude/skills/crypter-devcontainer-verify/SKILL.md index c95fa34d..db52c5fb 100644 --- a/.claude/skills/crypter-devcontainer-verify/SKILL.md +++ b/.claude/skills/crypter-devcontainer-verify/SKILL.md @@ -1,6 +1,6 @@ --- name: crypter-devcontainer-verify -description: Rule on each finding in a report against the code, one verifier per finding. Invoked as /crypter-devcontainer-verify {run-id} {ref} {findings-path} by the crypter-triage-review skill. +description: Rule on each finding in a report against the code, one verifier per finding. Invoked as /crypter-devcontainer-verify {run-id} {ref} {findings-path} by the crypter-triage-review and crypter-review skills. --- # Crypter devcontainer verify @@ -16,8 +16,23 @@ and no findings of your own. You are given a run id, a ref, and a findings path: `/crypter-devcontainer-verify {run-id} {ref} {findings-path}`. -The findings file lives under `/runs/{run-id}/`. Each finding in it carries an id. **If the file -is absent, stop and say so.** +The findings path lives under `/runs/{run-id}/` and is either a file or a directory. **If it is +absent, stop and say so.** + +**A file** is a report someone collected, and each finding in it already carries an id. Use those +ids. + +**A directory** is the lenses' own output, one report per lens and no ids in it. Every `.md` in +it is a lens report named for its lens. Read each one, split it into its individual findings, and +give each an id of `{lens}-{n}` numbered from 1 in the order the lens reported them — the lenses +rank most severe first, so that order is information worth keeping. + +A lens that found nothing still writes its file saying so. It contributes no findings and no ids, +which is a result rather than a problem. Where every lens reported that way there is nothing to +verify, and that is the answer — say so and stop. + +**A directory holding no files at all is a different thing:** whatever should have filled it did +not run. Stop and say so, and do not report it as lenses finding nothing. `/runs/{run-id}/verification/` already exists; the caller creates it. **If it is missing, stop and say so** rather than creating it. diff --git a/.claude/skills/crypter-review/SKILL.md b/.claude/skills/crypter-review/SKILL.md index 445dc719..92d5cd10 100644 --- a/.claude/skills/crypter-review/SKILL.md +++ b/.claude/skills/crypter-review/SKILL.md @@ -1,6 +1,6 @@ --- name: crypter-review -description: Review an existing pull request with the container's reviewer lenses and post what they found to the pull request. Use when asked to scrutinise a pull request, or invoked as /crypter-review {pr-number}. +description: Review an existing pull request with the container's reviewer lenses, verify what they found, and post the findings that hold. Use when asked to scrutinise a pull request, or invoked as /crypter-review {pr-number}. --- # Crypter review @@ -11,8 +11,16 @@ Use it on a pull request that deserves more scrutiny than a read, and on pull re people raised. The lenses run in the container, against a copy of the pull request fetched into its clone. -The findings land on disk and on the pull request, as one review that comments and neither -approves nor requests changes. +Every finding lands on disk. The ones that survive verification also land on the pull request, as +one review that comments and neither approves nor requests changes. + +The lenses do the reviewing and the verifiers rule on what they found. Both run in the container. +You start them, carry what survives to the pull request, and report the rest — **you never review +the diff yourself, and you never decide whether a finding is true.** + +Everything that reaches the author came from a lens that read the code in the container and a +verifier that checked it there. A review posted from here is attributed to them, so anything of +your own inside it is a claim made in someone else's name. ## Setup @@ -23,8 +31,9 @@ Run from the root of the main checkout. The container's mounts resolve against i Use `pr-{number}` as the run id. ```bash -mkdir -p .claude/runs/pr-{number}/findings -chmod 777 .claude/runs/pr-{number} .claude/runs/pr-{number}/findings +mkdir -p .claude/runs/pr-{number}/findings .claude/runs/pr-{number}/verification +chmod 777 .claude/runs/pr-{number} .claude/runs/pr-{number}/findings \ + .claude/runs/pr-{number}/verification ``` The container's `agent` is uid 1001 and your files are uid 1000, so the agents write into @@ -36,6 +45,7 @@ on it: ```bash docker exec crypter-pipeline test -w /runs/pr-{number}/findings && \ + docker exec crypter-pipeline test -w /runs/pr-{number}/verification && \ docker exec crypter-pipeline test -x /usr/local/bin/crypter-workspace ``` @@ -66,6 +76,10 @@ Read its title, description and diff with whatever GitHub access this session ha CLI, or the GitHub MCP server's `pull_request_read`. What the author says it does is context for reading the diff, and worth carrying into your report where the two disagree. +Read for orientation and for the base branch, not for defects. This read is how you follow the +lenses later, not a first pass at the review. Whatever you notice here is not a finding, and +noticing it is not a reason to go looking for more. + Take its **base branch** from the same read — `base.ref` from `pull_request_read` with method `get`, or `.baseRefName` from `gh pr view`. Most pull requests here target `stable`, but a release targets `main`, and nothing about the number tells you which. Everything below diffs @@ -103,18 +117,47 @@ adherence phase sits out and the lenses do the work. Findings land in `.claude/runs/pr-{number}/findings/{lens}.md`. -## 4. Triage +`claude -p` exits 0 whether or not the run worked. An unknown command, an expired session and a +clean review all come back as success, so the exit code tells you nothing. Look at what it wrote +instead: + +```bash +ls .claude/runs/pr-{number}/findings/ +``` + +An empty directory is a failed run, not four quiet lenses — a lens with nothing to say still +writes its file. **Stop and say so.** + +Do not stand in for the lenses that did not run. Reading the diff and writing up what you would +have found produces a review with nothing behind it, wearing their name, at exactly the moment +there is nothing to post and the pull request looks unreviewed. The run failing is the result; +report that instead. + +## 4. Verify + +A lens that has already been wrong once will happily be wrong again, and a finding posted is a +finding the author has to answer. So every finding is ruled on against the code before it goes +anywhere — by a verifier in the container, one per finding, not by you. -Read every finding against the code before you carry it to the pull request. A lens that has -already been wrong once will happily be wrong again, and a finding posted is a finding the author -has to answer. +```bash +docker exec -w /work/pr-{number} crypter-pipeline \ + claude --permission-mode auto -p "/crypter-devcontainer-verify pr-{number} pr-{number} /runs/pr-{number}/findings/" +``` + +Given the findings directory, verify treats every file in it as a lens report, assigns each +finding an id, and gives one verifier the finding and nothing else. + +Verdicts land in `.claude/runs/pr-{number}/verification/{id}.md`, each with the evidence behind +it. -Keep anything with a concrete failure behind it. Drop preferences, restatements of what the -author already chose, and findings about code the diff did not touch. +An empty `verification/` means one of two things and they are not the same. Either every lens +found nothing, which verify reports and which ends in no review — a real and welcome result — or +the run failed the silent way step 3 describes. Verify's own report distinguishes them. **If it +failed, stop and say so**; do not read the absence of verdicts as a clean diff. -Write what you kept and what you dropped, with a reason for each, to -`.claude/runs/pr-{number}/triage.md`. That file is how the user checks this judgement, and it is -what a later remediation run reads. +**Do not read the findings before the verdicts exist.** A finding you have already formed a view +on is one you will post or bury on your own authority, which is the whole thing this step moves +into the container. Wait for the verdict and route by it. ## 5. Post the review @@ -125,15 +168,26 @@ Use the GitHub MCP server's `pull_request_review_write` with method `create` to review, `add_comment_to_pending_review` for each finding that names a file and a line **in the diff**, then `submit_pending`. +**Only findings that held go up.** A finding the verifier ruled against is the process working, +and the pull request is not where that belongs — it would cost the author a read to reach the +same conclusion the verifier already reached with the code in front of it. Unsettled findings do +not go up either; nothing unverified reaches the author. + +They are not lost. The verdicts and their evidence stay under `.claude/runs/pr-{number}/`, which +is where a later pass over this pipeline reads what the lenses claimed and how it turned out. + The review body carries: - Which lenses ran, and which found nothing. A quiet lens is a result worth stating. -- Every finding you kept that has no line to hang on, in full. +- Every held finding that has no line to hang on, in full. - That the lenses read the diff rather than the discussion around it, so a finding resting on an assumption about intent says so. -Attribute it. The body opens by naming the lenses as its author, so the person reading knows what -produced it. +Attribute it. The body opens by naming the lenses as its author and the verifiers as what ruled +on them, so the person reading knows what produced it. **Nothing in the review is yours.** + +**If nothing held, post no review.** Say so to the user instead. A review that reports only that +it found nothing still costs everyone subscribed a notification. A line comment that the API rejects for being outside the diff goes in the body instead. **Do not retry it against a different line.** @@ -144,8 +198,17 @@ retry it against a different line.** docker exec crypter-pipeline crypter-workspace remove pr-{number} ``` -Remove it on every exit path, including the ones where you stopped early. The findings under -`.claude/runs/pr-{number}` are the record and they stay. +Remove it on every exit path, including the ones where you stopped early. The findings and +verdicts under `.claude/runs/pr-{number}` are the record and they stay. + +Then report: + +- The review URL, and what went up. +- The counts: how many findings each lens raised, how many held, how many did not, how many are + unsettled. +- The unsettled ones in full. They reached nobody else, so this is the only place they surface. +- Which lenses found nothing. +- Where the artifacts are. -Tell the user the review URL, what you kept and dropped, which findings you checked against the -code yourself and stand behind, and where the artifacts are. +A lens that raised plenty and had none of it hold is worth a sentence of its own. That is the +pipeline telling you something about the lens rather than about the pull request. diff --git a/Documentation/Development/Agentic Development Pipeline.md b/Documentation/Development/Agentic Development Pipeline.md index 255b6110..9b16f614 100644 --- a/Documentation/Development/Agentic Development Pipeline.md +++ b/Documentation/Development/Agentic Development Pipeline.md @@ -19,9 +19,12 @@ and it invokes the rest. | `/crypter-step-open-pull-request` | Your session | Pushes the branch and opens or updates the draft pull request | Every skill that reads or writes code runs in the container, against the container's own clone. -Your session plans, decides what to act on, and talks to GitHub. `/crypter-review` reviews -nothing itself: it fetches the pull request into the container and runs -`/crypter-devcontainer-examine` there. +Your session plans, decides what to act on, and talks to GitHub. It does not read code to form a +view on it — reviewing a diff and ruling on a finding are both judgements made in the container, +by an agent with the code in front of it. `/crypter-review` reviews nothing itself and settles +nothing itself: it fetches the pull request into the container, runs +`/crypter-devcontainer-examine` there, has `/crypter-devcontainer-verify` rule on what came back, +and carries the survivors to GitHub. Both prefixes say the same thing: an orchestrator invokes this, you do not. `crypter-step-` runs in your session, and `crypter-devcontainer-` runs in the container, which expects a workspace, @@ -137,8 +140,13 @@ pull request, and holds it against CI for at most three fix attempts. The approv stop, and the pull request stays a draft until you take it out of one. `/crypter-review {pr-number}` is the second entry point. It fetches a pull request's head into -the container, runs the lenses against it with no plan to audit, triages what they raise, and -posts one review that comments. It never approves and never requests changes. +the container, runs the lenses against it with no plan to audit, then gives one verifier per +finding the same treatment `/crypter-triage-review` gives findings from anywhere else. Only the +findings that survive that are posted, as one review that comments. It never approves and never +requests changes. + +What a lens raised and a verifier then ruled against stays in `.claude/runs`. It is a record of +the pipeline checking itself, and not something the pull request has to carry. `/crypter-triage-review {pr-number}` is the third. It reads the findings already on a pull request, whoever left them, and gives one verifier per finding a worktree and nothing else to