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