diff --git a/.claude-plugin/marketplace.json b/.claude-plugin/marketplace.json index e6004a6..f54234c 100644 --- a/.claude-plugin/marketplace.json +++ b/.claude-plugin/marketplace.json @@ -10,7 +10,7 @@ "name": "human-fsm", "source": "./plugins/human-fsm", "description": "Slash commands for the human's side of the issue→PR pipeline FSM: read the next ai:ready PR and check the vetter's verdict against the diff and the issue it closes, then rule on a close-candidate, reject, raise a design question, or keep an issue open. Every input comes from pr-review-report; none writes GitHub state itself.", - "version": "0.5.0", + "version": "0.6.0", "author": { "name": "Rain Open Source Software Ltd" }, "keywords": [ "rainlanguage", diff --git a/README.md b/README.md index cc0fc0e..6cdafbf 100644 --- a/README.md +++ b/README.md @@ -373,6 +373,29 @@ sweep, and this server runs with no `WORK_DIR`, so its clones land in the temp-dir fallback the producer's `clone_gc` may never look in. A human-gate leak has no collector behind it. +**The lens's SCOPE is an argument, because prose does not survive the invocation +(#154).** `nr.md` described the scope correctly and at length — the diff plus +the callees, callers, siblings sharing the changed invariant and every +current-behaviour claim, with _"would understanding it change the ruling on THIS +diff?"_ as the inclusion test — and none of it reached the run. `Skill audit` +loads a document whose own first rule is _"whole-repo snapshot, never a diff"_, +and once it is loaded `nr.md` is not in the room: on `rain.deploy#21` the lens +ran whole-repo, twelve findings, five bearing on the PR and seven in code the +diff never touches, with the scope hand-typed as free text nothing could +validate. So `/nr` DECLARES the scope — the literal `pr:` — beside the +`dir` and the changed-file list. The spelling is the skill's own and not this +repo's: its whole vocabulary is three literals, `whole-repo` / `pr:` / +`paths:`, the same strings its run stamp records verbatim, and a key +wrapped round one of them would be a fourth spelling. The value is built from +typed results only: the number out of the row's own `pr` field, the file list +out of `pr_context`. And the ruling `/nr` presents NAMES the scope it was formed +at, so a PR-scoped review is distinguishable from a whole-repo sweep without +counting how many findings missed the diff. `whole-repo` stays reachable as a +separate deliberate invocation on a repo somebody named — never as the default +that happens when nothing is passed, which is precisely how the defect read as +working. The step-5 prose is kept as the REASON the PR scope admits the +surrounding files it does, rather than as the only thing carrying it. + #### `next_ready` — the merge decision as one typed result Ruling on an `ai:ready` PR used to take six `gh` reads assembled by hand, in an diff --git a/plugins/human-fsm/.claude-plugin/plugin.json b/plugins/human-fsm/.claude-plugin/plugin.json index 0401127..10fd750 100644 --- a/plugins/human-fsm/.claude-plugin/plugin.json +++ b/plugins/human-fsm/.claude-plugin/plugin.json @@ -1,7 +1,7 @@ { "name": "human-fsm", "displayName": "Human FSM transitions", - "version": "0.5.0", + "version": "0.6.0", "description": "Slash commands for the human's side of the rainlanguage issue→PR pipeline finite state machine: the reads that precede a ruling and the transitions that make it. Every input arrives from pr-review-report — each ruling as one subcommand, /nr as typed MCP reads — and the guards (vocabulary, mandatory note, provenance anchor, stranded-flag refusal, subject type, terminal-is-moot, idempotence) live in that binary, not here.", "author": { "name": "Rain Open Source Software Ltd", diff --git a/plugins/human-fsm/commands/nr.md b/plugins/human-fsm/commands/nr.md index 4a0ef5d..c582a19 100644 --- a/plugins/human-fsm/commands/nr.md +++ b/plugins/human-fsm/commands/nr.md @@ -1,5 +1,5 @@ --- -description: The next ai:ready PR to rule on — the vetter's verdict, checked against an independent read of the diff, the issue it claims to close, and the audit skill run over the PR's own source. +description: The next ai:ready PR to rule on — the vetter's verdict, checked against an independent read of the diff, the issue it claims to close, and the audit skill run over the PR's own source at a declared pr: scope. argument-hint: [1-3] allowed-tools: mcp__plugin_human-fsm_fsm__next_ready, mcp__plugin_human-fsm_fsm__pr_context, mcp__plugin_human-fsm_fsm__pr_checkout, mcp__plugin_human-fsm_fsm__clone_release, Skill, Read --- @@ -77,22 +77,48 @@ reading is written inside. in `pr_checkout`'s own result is the only path that is this PR's source — you do not glob for one, and a `vet-*` directory you happened to find is a DIFFERENT PR's tree. -- **Invoke the skill with the `Skill` tool** — `audit` — seeded with the changed - file list and that `dir`. Never hand-copy its checks: invoking it is how this - command inherits every upgrade to it, and the two findings that motivated this - step were both stated plainly in it while a hand-rolled read missed them +- **Invoke the skill with the `Skill` tool** — `audit` — and DECLARE THE SCOPE + AS AN ARGUMENT, not as something the reader is asked to remember. The declared + scope is `pr:`; the `dir` the checkout returned and the changed-file + list go beside it. Why an argument at all: the skill's own top-line rule is + _"whole-repo snapshot, never a diff — do not scope by recent changes / PR + diff"_, so once it is loaded this file is not in the room, and a scope carried + only in prose loses to the document the invocation just pulled in. Measured on + `rain.deploy#21`, it did: twelve findings, five bearing on the PR and seven in + code the diff never touches, with the scope hand-typed as free text that + nothing could check. +- **A scope is one of three literals, and this command declares the middle + one.** The skill's whole vocabulary is `whole-repo`, `pr:` and + `paths:` — the same three strings its run stamp records + verbatim. Declare the literal itself. A key wrapped round it, a synonym, "the + changed files", or a sentence describing which files you meant are each a + fourth spelling, which is free text with a colon in it, which is the thing + being removed. +- **Never hand-copy the skill's checks.** Invoking it is how this command + inherits every upgrade to it, and the two findings that motivated this step + were both stated plainly in it while a hand-rolled read missed them (`rain.deploy#20`, a newly added concrete test mock carrying a caret pragma; `rain.deploy#21`, a canonical CREATE2 derivation added and then hardcoded 22 times beside 4 real calls). Run it INLINE and serial. -- **Scope it to the middle ground**, exactly as the vetter's is scoped: the - changed lines PLUS the code whose behaviour decides whether the diff is - correct — the callees the changed lines invoke, the callers relying on the - changed behaviour, sibling implementations sharing the invariant being - changed, and every claim the PR body or the issue makes about how the code - CURRENTLY behaves, since a stated current behaviour the source contradicts is - a false premise and a finding in itself. NOT the diff alone; NOT a whole-repo - audit on every `/nr`. The test for reading a file is "would understanding it - change the ruling on THIS diff?". +- **Every part of that argument comes from a typed result.** The `` in + `pr:` is the one inside the row's own `pr` field — `owner/repo#n`, the + same string step 2 was addressed with — never a number read off a title, a URL + or the vetter's note. The changed-file list is `pr_context`'s `files`, whole, + each with its additions and deletions; if `filesTruncated` is true the list is + a PAGE of `filesTotal`, and the scope you can honestly declare covers only + what you were handed, so say which. The tree is `pr_checkout`'s `dir`. A scope + you assembled yourself is the defect `#132` removed one level up: it looks + exactly like a derived one, and nothing downstream can tell them apart. +- **What `pr:` admits, and why — this is the REASON for the argument, + not a second carrier of it.** In scope is the middle ground: the changed lines + PLUS the code whose behaviour decides whether the diff is correct — the + callees the changed lines invoke, the callers relying on the changed + behaviour, sibling implementations sharing the invariant being changed, and + every claim the PR body or the issue makes about how the code CURRENTLY + behaves, since a stated current behaviour the source contradicts is a false + premise and a finding in itself. NOT the diff alone; NOT a whole-repo audit on + every `/nr`. The test for reading a file is "would understanding it change the + ruling on THIS diff?". - **Your read surface inside that tree is `Read`.** This harness has no `Grep` and no `Glob` — measured on 2.1.220, neither is listed and neither resolves through `ToolSearch` — so the lens navigates by path, from the changed-file @@ -105,6 +131,13 @@ reading is written inside. merge. A question the diff raises and cannot settle itself is a `/design` rather than a quiet merge. And the skill finding nothing is not "clean": it never read the issue, so it cannot tell you the diff answered it. +- **Carry the declared scope through to what you present.** The value you passed + is the one thing that says which code was read, so it is reported verbatim + beside the findings rather than left to be inferred from them — a reader + counting seven findings in untouched files should not have to work out that + the lens swept the repo. If the skill's report contradicts the scope you + declared, the scope is what the ruling is measured against and the divergence + is the finding: say which one you got. **6. `clone_release`** the checkout, passing the `dir` name `pr_checkout` returned, before you present anything. A checkout left behind sits on the box @@ -112,8 +145,10 @@ until a sweep reaches it — and this server has no `WORK_DIR`, so its clones la in the temp-dir fallback, which the producer's sweep may never look in. Unreleased checkouts are how this box filled its disk. If `pr_checkout` ERRORS there is nothing to release and no audit lens either: re-call it ONCE, and if it -fails again present the read WITHOUT that half and say so in as many words. A -missing lens, named, is worth more than a lens implied. +fails again present the read WITHOUT that half and say so in as many words — a +lens that never ran has no scope, and "no lens" is what you report, never a +scope you would have declared. A missing lens, named, is worth more than a lens +implied. **7. Put your read beside the vetter's, and say plainly where they diverge.** Agreement reached independently is worth something; agreement by restatement is @@ -158,6 +193,26 @@ catches what upstream skipped is not redundant with it. So: the skill supplies the rules, this command supplies the judgement, and step 5 is subordinate to steps 3 and 4 rather than a substitute for them. +## `whole-repo` is a different job, and it is asked for on purpose + +A genuine whole-repo audit is a real thing to want — most obviously a repo you +are about to take a dependency on, before you depend on it. It is not this +command. `/nr` rules on ONE PR and its lens exists to decide THAT merge; a sweep +of every file answers a question nobody asked at this gate, and it answers it by +burying the findings that bear on the diff under the ones that do not — five +among twelve, on `rain.deploy#21`. So this command declares `pr:` on +every invocation and has no mode, flag or argument that declares anything else: +the whole argument is the LIMIT, and a scope is not something a caller passes +here. + +That is deliberately not the same as removing whole-repo. It stays available as +a SEPARATE, explicit invocation of the same skill with `whole-repo` declared, on +a repo somebody named, outside this command — which is the only shape the skill +writes a `whole-repo` run stamp for anyway. What must never happen is a +whole-repo sweep arriving because no scope was passed. A scope that defaults to +the widest reading is indistinguishable from one that was chosen, and that is +exactly how this behaviour read as working for as long as it did. + ## Typed reads and the lens, and no shell at all Every input about the PR arrives from a typed tool call — the queue row, the @@ -167,6 +222,14 @@ is a decision whose inputs nobody can audit, and its shape drifts with whoever assembled it. If a tool is unavailable, say so and stop — the answer is to connect the plugin's MCP server, not to work around it. +The lens's SCOPE is one of those inputs and not an exception to the rule. It is +a value derived from two of those results — the PR ref the row named and the +file list `pr_context` returned — and passed as an argument the skill reads, +rather than a sentence about the PR written into an args string. Free text is +what nothing can check: the scope that produced twelve findings on a +five-finding question was typed out in full, correctly, and lost to the first +rule of the document it was handed to. + The grant is four typed calls plus `Skill` and `Read`, and `Read` applies to the `pr_checkout` tree and nothing else. All four typed calls are reads except `clone_release`, which disposes of what this command itself created and writes @@ -232,6 +295,14 @@ up to: whether anything blocks a merge, and if the deploy gate is set, that this is deploy-before-merge and **not** a plain merge, because landing it as if it were ordinary is a production error. +**The lens's findings arrive under the scope they were formed at, stated.** Name +the literal you declared — `pr:`, with the number in it — or, where the +checkout failed, that there was no lens at all. It is one line and it decides +how every finding under it should be read: a PR-scoped review and a whole-repo +sweep produce different lists, and a reader handed the list without the scope +has to reverse-engineer which one they got from the proportion of findings that +miss the diff. That is the inference this line removes. + Clean is a conclusion you are allowed to reach, not one to reach for: say it only about a diff you read against an issue you read, with a lens you actually pointed at the source, and say which those were. **This command does not merge diff --git a/pr-review-report-rs/src/main.rs b/pr-review-report-rs/src/main.rs index 4594a2e..6dc84f1 100644 --- a/pr-review-report-rs/src/main.rs +++ b/pr-review-report-rs/src/main.rs @@ -8208,6 +8208,40 @@ enum CommandKind { #[cfg(test)] const MCP_COMMAND_NATIVE_TOOLS: &[&str] = &["Skill", "Read"]; +/// A full sweep: every first-party file, whatever the diff touched. +/// +/// One of the three scopes the `audit` skill accepts, spelled as its own run-stamp section spells it +/// — the only value that means "fully audited" to a consumer of that stamp. +#[cfg(test)] +const AUDIT_SCOPE_WHOLE_REPO: &str = "whole-repo"; + +/// A PR-scoped review, `pr:` — the diff PLUS the code whose behaviour decides whether the +/// diff is correct. This is the scope every `/nr` invocation declares. +#[cfg(test)] +const AUDIT_SCOPE_PR: &str = "pr:"; + +/// A path-scoped review, `paths:`. +#[cfg(test)] +const AUDIT_SCOPE_PATHS: &str = "paths:"; + +/// The `audit` skill's whole scope vocabulary: a declared scope must BEGIN with one of these, and +/// `whole-repo` is complete in itself while the other two carry a payload after the colon. +/// +/// A scope has to be a value from this set or it is free text again, which is the whole of #154. +/// `nr.md` described the middle-ground scope correctly and at length — the changed lines plus the +/// callees, callers, siblings sharing the changed invariant, and every current-behaviour claim — and +/// none of it survived the invocation: `Skill audit` loads a document whose own first rule is +/// "whole-repo snapshot, never a diff", so once it is loaded the calling command is not in the room +/// and the skill's rule wins. Measured on `rain.deploy#21`, the lens ran whole-repo — twelve +/// findings, five bearing on the PR and seven in code the diff never touches — with the scope +/// hand-typed as free text in an args string that nothing could validate. +/// +/// Held here as ONE vocabulary rather than re-typed per caller: the value `/nr` declares, the value a +/// lens ledger records, and the value a gate refuses on are definitionally the same three strings, +/// and a second copy is a second spelling waiting to drift. +#[cfg(test)] +const AUDIT_SCOPES: &[&str] = &[AUDIT_SCOPE_WHOLE_REPO, AUDIT_SCOPE_PR, AUDIT_SCOPE_PATHS]; + /// PURE: what one shipped command invokes, and whether its body keeps that promise. /// /// A command's `allowed-tools` line is its DECLARED surface, and that line decides what the body is @@ -29564,6 +29598,66 @@ mod marketplace_tests { releases the checkout it took" ); } + + // #154: the grant above buys an invocation, and the invocation is only worth what its SCOPE is. + // `nr.md` used to carry the scope as a paragraph — correct, detailed, and overridden the moment + // `Skill audit` loaded a document whose first rule is "whole-repo snapshot, never a diff". The + // fix is a declared literal, and three things about it are pinned here: it is IN the invocation, + // it is the PR one, and the file states the closed vocabulary it comes from. + #[test] + fn nr_declares_the_lens_scope_as_a_value_from_the_skills_own_vocabulary() { + let Some(text) = repo_root_text("plugins/human-fsm/commands/nr.md") else { + return; // not checked out (nix build sandbox) + }; + // Asserted on the INVOCATION bullet, not on the file. The literal appearing somewhere in a + // document that also explains the scope at length is exactly the state #154 describes — the + // scope has to be part of the call, and the call is this bullet. + let Some((_, after)) = text.split_once("**Invoke the skill") else { + panic!( + "step 5 has no INVOKE bullet — the scope is declared in the invocation, so there is \ + nowhere for it to be declared" + ) + }; + let invocation = after.split("\n- ").next().unwrap_or(after); + let pr_scope = format!("`{AUDIT_SCOPE_PR}`"); + assert!( + invocation.contains(&pr_scope), + "the bullet that invokes the skill must DECLARE {pr_scope} as the scope; a scope only \ + described in prose is the #154 defect, and the skill's own whole-repo rule wins over a \ + description. The bullet reads: {invocation:?}" + ); + // And declares no OTHER scope, so the invocation cannot carry two. Matched on the backticked + // literal specifically: the bullet quotes the skill's whole-repo RULE in prose, and a quoted + // rule is not a declared value — which is the whole distinction this test is about. + for other in [AUDIT_SCOPE_WHOLE_REPO, AUDIT_SCOPE_PATHS] { + assert!( + !invocation.contains(&format!("`{other}")), + "the invocation bullet also names `{other}`. /nr rules on ONE PR, so it declares \ + {pr_scope} and nothing else — a whole-repo sweep is an invocation somebody asked \ + for, never one this gate can reach" + ); + } + // The vocabulary is CLOSED, and ONE bullet has to say so — asserted there rather than over + // the file, because `whole-repo` is discussed elsewhere for its own sake (it is a scope this + // command deliberately does not pass) and a mention is not a statement of the closed set. + // Without the list, "declare a literal" is advice with nothing behind it and the next + // spelling gets invented in good faith. + let Some((_, after)) = text.split_once("**A scope is one of three literals") else { + panic!( + "nr.md states no scope VOCABULARY — a command told to declare a literal, and not \ + told which literals exist, is one invented spelling away from free text again" + ) + }; + let vocabulary = after.split("\n- ").next().unwrap_or(after); + for scope in AUDIT_SCOPES { + assert!( + vocabulary.contains(&format!("`{scope}")), + "the vocabulary bullet omits {scope:?}. The skill's whole vocabulary is \ + {AUDIT_SCOPES:?} and the bullet has to name all of it; a partial list reads as the \ + complete one. The bullet reads: {vocabulary:?}" + ); + } + } } // ─────────────────────────────────────────────────────────────────────────────