From b0f56a7cf4badeaef1db859c404c7c5eb5a63a81 Mon Sep 17 00:00:00 2001 From: Matthew Maclay Date: Wed, 11 Nov 2026 08:26:17 -0700 Subject: [PATCH] feat(implement-change): add verify mode for work written with no plan - Add `implement-change verify [goal]`: runs the build's review wave on the branch's diff with `Plan: none` and a one-line goal as the brief, asking for the goal once when none is given - Let implementation-reviewer and focused-reviewer judge goal and scope against the brief when there is no plan - Bump implement-change to 1.20.0 - Document verify in README, GUIDE.md and docs/design.md; narrow the ROADMAP route item and defer carrying a declined finding's reason between waves, with the sandbox results - Add sandbox scenarios verify-adhoc-swallowed, verify-asks-goal and trigger-implement-change-verify Co-Authored-By: Claude Opus 5.5 --- GUIDE.md | 6 ++- README.md | 4 +- ROADMAP.md | 3 +- agents/focused-reviewer.md | 12 ++--- agents/implementation-reviewer.md | 8 ++-- docs/design.md | 2 +- sandbox/README.md | 2 + sandbox/scenarios.yaml | 62 +++++++++++++++++++++++++ sandbox/triggers/coverage.yaml | 2 +- sandbox/triggers/scenarios.yaml | 15 ++++++ skills/implement-change/SKILL.md | 26 +++++++---- skills/implement-change/reviews/wave.md | 19 ++++---- skills/implement-change/verify.md | 52 +++++++++++++++++++++ tests/test_sandbox.py | 2 +- 14 files changed, 180 insertions(+), 35 deletions(-) create mode 100644 skills/implement-change/verify.md diff --git a/GUIDE.md b/GUIDE.md index bf02414..701c2a7 100644 --- a/GUIDE.md +++ b/GUIDE.md @@ -180,7 +180,9 @@ Every finding is a proposal. Every count the revision reports is per rule, never **Automatic once you start it.** The plan review loop and the build gates run on their own until they pass or hit a stop. How the build ended is always reported as it happened; a run that hit a cap is never called passed. -**Depth.** A build runs light, standard or deep. Light is the implementation reviewer, running lint and the tests that touch the change, beside one focused reviewer, for a change whose every changed line is on the profile's allowlist, such as a changelog entry; standard is the round of reviewers the diff calls for; deep always plans first, with the planner and the plan reviewer, for what `implement-change`'s depth floor names deep. A script sets the floor from the diff; you may raise it, and you may lower a standard floor to light with a reason, which the pull request's `Procedure:` line carries. Deep is never lowered. After the first round, only the implementation reviewer runs again, beside any focused reviewer that had a blocking finding and the tests reviewer when a fix changed a test. +**Depth.** A build runs light, standard or deep. Light is the implementation reviewer, running lint and the tests that touch the change, beside one focused reviewer, for a change whose every changed line is on the profile's allowlist, such as a changelog entry; standard is the round of reviewers the diff calls for; deep plans first, with the planner and the plan reviewer, for what `implement-change`'s depth floor names deep, except in a verify, which has no plan to draft and names a planned build as the alternative. A script sets the floor from the diff; you may raise it, and you may lower a standard floor to light with a reason, which the pull request's `Procedure:` line carries. Deep is never lowered. After the first round, only the implementation reviewer runs again, beside any focused reviewer that had a blocking finding and the tests reviewer when a fix changed a test. + +**Code you wrote yourself.** `/lasp-dev:implement-change verify [goal]`, or "review what I just wrote", runs the same review on work written with no plan: the diff is your commits since the default branch plus the working tree, a one-line goal stands in for the plan, the wave fixes blocking findings in your working tree, and it ends at the diff gate, before any commit. **One session per branch.** Starting a build (`implement-change`) or a fix (`pr-fix`) takes a lock under `.review/`; a second session starting either in the same tree is told who holds it and asks whether to take it over. The skill releases the lock when it finishes or when its session ends, and a lock untouched for four hours, from a session that crashed or was left open, is stale and taken without asking. Reviewing a pull request or drafting a ticket takes none. A session in a repository with a `.review/` directory opens with one line: where the ticket stands and the next command. It never touches GitHub. @@ -215,7 +217,7 @@ Every finding is a proposal. Every count the revision reports is per rule, never - **Provisional, established, graduated:** a rule's status on its ledger entry. A rule is admitted provisional, becomes established when the Established Dial is met, and is graduated when the revision judges that a check enforces it; one that leaves is retired. - **Profile:** one repository's values the skills read, such as its ticket pattern, test and lint commands and depth allowlist, kept in its standard: its own `standards/profile.yaml`, or `profiles/.yaml` in a standard for several repositories. A key left out is read from the repository's own files, then the plugin's default profile. - **Sitting:** a stretch of work on a ticket; `work` names three, the author's (plan, build, open and review), the reviewers' on GitHub, and the author's again (fix and reply), as Figure 1 shows. -- **Wave:** one round of reviewers on a build or a fix, the implementation reviewer beside a focused reviewer per focus the diff calls for; `skills/implement-change/reviews/wave.md` defines it. +- **Wave:** one round of reviewers on a build, a verify or a fix, the implementation reviewer beside a focused reviewer per focus the diff calls for; `skills/implement-change/reviews/wave.md` defines it. - **Write plan:** every Jira write a `ticket-draft` or `ticket-audit` pass proposes, shown as one numbered list, approved once with any items dropped, then run in order and read back (`using-jira`, "Writing to Jira"). - **AI assistance level:** the `n/5` on prose a skill composes for a pull request, from 1, written by a person, to 5, not read by one; `5/5` until you lower it (`skills/pr-review/comment-style.md`). - **Build outcome, Build line:** `.review/build/outcome.md`, which `implement-change` writes when the build loop stops; its first line, `Build: `, says how the build ended, and `pr-create` copies it into the Already checked block. diff --git a/README.md b/README.md index 34ba8e3..2a81d73 100644 --- a/README.md +++ b/README.md @@ -445,7 +445,7 @@ The skills a person types, in the order they meet them: - `work`: works out where a ticket or its pull request stands and runs the next phase; each phase keeps its own approvals. - `implement-change`: takes one change to an agreed plan, code verified by a wave of reviewers, and - an offered pull request. + an offered pull request; `verify` runs the same wave on work you wrote with no plan. ![Inside implement-change: a plan loop of up to three rounds, your approval of the plan, up to five review rounds with fresh agents, up to two simplify passes, then your review of the diff](docs/img/build-loop.svg) @@ -495,7 +495,7 @@ Most days need one command, `work`. Started, in the skills table: `typed only` r | | `quickstart` | typed or asked | Say where this machine and repository stand, what to fix first and what to type next, with `onboard` first in a repository with no standard, and the skills by group; with a question, answer it from the plugin's own files. Reads only | | | `onboard` | typed or asked | Survey a repository for the facts the plugin works from, its committed templates, contributing guide, test commands (instruction files, docs, CI) and pre-commit, and its history, and the tracker's statuses and fields when the servers are connected; say where each fact would be read from, the repository's own file first; first asking where the repository's rules live, here or in another repository's standard; then ask where facts should live, which value holds where the repository's files disagree, and which proposals to keep. Writes only `.review/onboard/` unless the person says yes to a named write: the repository's standard, or its `standard:` pointer and the standard's `profiles/.yaml` | | Phases, also run alone | `ticket-draft` | typed or asked | Turn a prompt, an existing ticket or a pull request with none into one in the team's format, a Jira ticket or, on a GitHub tracker, an issue: duplicates merged, dependencies, follow-up tickets for out-of-scope work, an in-session estimate, and an Open Questions section for what cannot be settled yet. `investigate` answers your questions about the code first; `settle` folds later answers in and records a departure the work keeps as a comment; `reformat` moves a drafted ticket to the current format, asking only what the format needs. Run directly for an epic or a group ticket session | -| | `implement-change` | typed or asked | Take a change request to an agreed plan, an implementation and a diff verified by a wave of reviewers | +| | `implement-change` | typed or asked | Take a change request to an agreed plan, an implementation and a diff verified by a wave of reviewers; `verify` runs the wave on work written with no plan | | | `pr-create` | typed or asked | Write a reviewer-focused description and open the pull request | | | `pr-review` | typed or asked | Review a pull request and present the findings at their lines, each citing a rule, decision, term, ticket criterion or test check when one applies, and no key otherwise; draft comments when asked, and post one review with the verdict you choose only on your yes. A posted review's threads are what the revision reads; a review kept local leaves it nothing | | | `pr-fix` | typed or asked | Assess every thread, your own comments included, take your agree, disagree or discuss on each (posting them is optional, and each run links the ones not yet on GitHub), offer a rebase when the branch is behind, then plan, implement and reply to the agreed feedback on a pull request, people's comments before AI findings | diff --git a/ROADMAP.md b/ROADMAP.md index eee55ae..4142b61 100644 --- a/ROADMAP.md +++ b/ROADMAP.md @@ -31,6 +31,7 @@ merged pull request is its record. Owners are roles. Status is `planned`, `defer | Item | Why | Owner | Trigger | Status | | --- | --- | --- | --- | --- | | A neutral thread JSON (#60), with a `gh api` adapter for `record.py`'s four inputs (`pull_request_read` `get`, `get_reviews`, `get_review_comments`, `get_comments`) | `pr-fix` and the harvest stop without a GitHub MCP server, and `pr-review` on `gh` presents a re-review's findings without posting them, since it cannot deduplicate against the threads; one documented thread shape lets `gh` feed them | plugin maintainer | step 4 of #24's order | planned | +| A declined finding's reason carried between review waves | A finding the person declines at a stop or the diff gate would be marked with their reason in the wave file and passed to the next wave's reviewers, who raise it again only when the reason is factually wrong. On Haiku, 10 runs a side (2026-11-10 to 11), the declined code survived 10/10 on main and 9/10 with it; the decline was written into a wave file 7/10 (main 0/10), and a contradicted reason was marked disputed to the person 4/5, two of them only as what a later wave would do (main 0/5, though main's session said the reason did not hold in 3/5). The session sometimes did not write the decline down, and a consolidated `wave-.md` is written in only 1 or 2 of 10 runs on main too | plugin maintainer | recording a decline in the wave file is reliable | deferred | | `pr-review` on the shared wave, with one finding format | `pr-review`'s reviewer would run as the build's wave does, one focus each, and the build's digests and `pr-review`'s findings would share one shape | plugin maintainer | after a sandbox comparison of the two | planned | | `next_phase.py` reading `pr-fix`'s local dispositions | A decision kept in `.review/pr--fix-plan.md` `## Dispositions` and not posted leaves its thread unanswered in the record, so `work` routes to `pr-fix` each time and `pr-fix` raises it with its link. A `--fix-plan` read in `next_phase.py` would route on instead, at the cost of a GitHub record the revision cannot read | plugin maintainer | the repeated prompt proves a burden, or the team decides a local decision is enough | deferred | | Keep it simple and don't repeat yourself, with a tiebreak | The planner and its reviewer look for the smallest change and `standards/review-contract.md` "New surface" makes a duplicate of an existing function a finding, but nothing says which wins when the two pull apart: strict deduplication extracts abstractions early. A few lines in the contract would name both and settle it: a copy is a finding at its third occurrence, or at its second when the copies must stay in step (a constant, a unit conversion, a validation); similar blocks that may change independently are left. A sandbox check confirms the simplicity reviewer does not then flag every pair of similar lines | plugin maintainer | a review flags, or misses, a duplication the team disagrees with | idea | @@ -53,7 +54,7 @@ merged pull request is its record. Owners are roles. Status is `planned`, `defer | Reviewer counts from `SubagentStop` | The build hook counting reviewers from `SubagentStop` rather than from the Agent tool's prompt | plugin maintainer | a miscounted wave | idea | | `make`, `tox` and `nox` as gates | The build hook counting them | plugin maintainer | a repository adopts one | idea | | A simpler-design check against the requirements | Given a ticket's requirements and a drafted design or plan, look for a simpler implementation that meets every requirement; where one exists and is rejected, write into the design why it is not acceptable, as an "Alternatives considered" line naming the requirement it fails. `implementation-plan-reviewer` hunts complexity in a plan and the build's simplicity reviewer reads a finished diff, but neither starts from the requirements, and neither leaves the rejected option written down for the next reader. A mode of `implementation-plan-reviewer`, or a step in `implement-change` and `ticket-draft`, before a new skill | plugin maintainer | a design where a reviewer asks "why not just X?" after the build | idea | -| A route where the person writes the code | Some people want to write a change themselves and keep Claude for the ticket, the plan and the review. `ticket-draft`, the planner, `pr-review` and `pr-fix` already serve a pull request whoever wrote it; the gap is between them: `implement-change` goes from the approved plan straight to the build. An "I'll write it" option at the plan gate would keep the plan as a checklist and skip the build, `work` would route a branch with commits and no build to review, and while the person codes Claude would answer questions with pointers rather than edits, which a hook refusing edits to source files could enforce. Open: whether it serves keeping one's own skills sharp (hints and explanations) or distrust of generated code (the strict guard and a closer review) | plugin maintainer | a teammate asks to write a change themselves within the workflow | idea | +| A route where the person writes the code | Some people want to write a change themselves and keep Claude for the ticket, the plan and the review. `ticket-draft`, the planner, `pr-review` and `pr-fix` already serve a pull request whoever wrote it, and `implement-change verify` runs the build's review on work written with no plan; the gap is the route between them: `implement-change` goes from the approved plan straight to the build. An "I'll write it" option at the plan gate would keep the plan as a checklist and skip the build, `work` and the session-start line would route a branch with commits and no build, or a verify's build, to review, and while the person codes Claude would answer questions with pointers rather than edits, which a hook refusing edits to source files could enforce. Open: whether it serves keeping one's own skills sharp (hints and explanations) or distrust of generated code (the strict guard and a closer review) | plugin maintainer | a teammate asks to write a change themselves within the workflow | idea | | Where a long run stands | After about ten minutes of work, or when the person asks, say where things stand and which skills ran, with the review loops inside them, such as `implement-change` with two review rounds. `/status` is Claude Code's own command, so this is a skill's report or a status line entry | plugin maintainer | | idea | | `bug-fix` and `release` skills | A bug taken from its report to a failing test that reproduces it, then the fix; a release that follows the repository's version and changelog convention and hands off to `downstream release`, with the tag behind a yes | plugin maintainer | | idea | diff --git a/agents/focused-reviewer.md b/agents/focused-reviewer.md index 09e7dff..fe37491 100644 --- a/agents/focused-reviewer.md +++ b/agents/focused-reviewer.md @@ -23,15 +23,15 @@ in place of the digest, its first line naming the error. ## Inputs -The caller gives you: the path of your brief, the plan file path, the ticket's context page -when one exists, `.review/standard.md` when it exists, and the base to diff against: a commit; -given a branch name, use `git merge-base HEAD`, so commits that branch gained since the -change began are not read, reversed, as part of it. On a later wave, also the delta since the -previous wave and your focus's previous findings. +The caller gives you: the path of your brief, the plan file path, or `none` for work written +with no plan, the ticket's context page when one exists, `.review/standard.md` when it exists, +and the base to diff against: a commit; given a branch name, use `git merge-base HEAD`, +so commits that branch gained since the change began are not read, reversed, as part of it. On a +later wave, also the delta since the previous wave and your focus's previous findings. Read the brief first and hold to it: its "Raise" is your whole job, its "Leave to others" is owned by another reviewer in the same wave, and its "Read first" says where the rules live. -Then read the plan, then `git diff ` plus untracked files; the change is usually +Then read the plan, if there is one, then `git diff ` plus untracked files; the change is usually uncommitted. Read the code around each hunk, not only the hunk. ## What you raise diff --git a/agents/implementation-reviewer.md b/agents/implementation-reviewer.md index 4a6962d..aa64740 100644 --- a/agents/implementation-reviewer.md +++ b/agents/implementation-reviewer.md @@ -2,8 +2,8 @@ name: implementation-reviewer description: > Verifies a finished change against its agreed plan, running the tests and linters; returns - SATISFIED or NOT SATISFIED. Run by the review wave, or asked to "verify this implementation" or - "check this is ready to commit". + SATISFIED or NOT SATISFIED. Run by the review wave, or asked to "verify this implementation". + Work written with no plan reaches it through implement-change's verify, not a direct request. tools: - Bash - Read @@ -21,7 +21,7 @@ Decide whether a finished change is ready to commit: it passes, it does what was ## Step 1: Take the Inputs -The caller gives you the plan file path, the brief (goal, scope and non-goals, acceptance criteria), and the base to diff against: a commit; given a branch name, use `git merge-base HEAD`, so commits that branch gained since the change began are not read, reversed, as part of it. Read the plan file itself, not a summary of it. Review `git diff ` plus untracked files — the change is usually uncommitted. +The caller gives you the plan file path, the brief (goal, scope and non-goals, acceptance criteria), and the base to diff against: a commit; given a branch name, use `git merge-base HEAD`, so commits that branch gained since the change began are not read, reversed, as part of it. Read the plan file itself, not a summary of it; with `Plan: none` there is no plan file, so read none and judge against the brief. Review `git diff ` plus untracked files — the change is usually uncommitted. On a later round the caller also gives your previous findings. Check each was fixed, and review the whole change again: a fix can break something that passed before. @@ -41,7 +41,7 @@ The caller does not run them, so every round runs them fresh; never carry a resu ## Step 4: Judge the Change Against the Plan and Scope -Implementation always teaches something the plan could not know, so expect small deviations. Judge the spirit: did the change reach the brief's goal by roughly the agreed route? +Implementation always teaches something the plan could not know, so expect small deviations. Judge the spirit: did the change reach the brief's goal by roughly the agreed route? With no plan, judge the goal, the scope and the acceptance criteria against the brief; the plan's steps and deviations do not apply. - **Every acceptance criterion is met**, by whichever step ended up meeting it. - **Every plan step is done** or visibly superseded — not stubbed, half-done, or quietly dropped. diff --git a/docs/design.md b/docs/design.md index 290b2c6..5012416 100644 --- a/docs/design.md +++ b/docs/design.md @@ -95,7 +95,7 @@ Libera's standard. **The agents that fetch or focus run on Sonnet, and your session decides.** The readers and the wave's focused reviewers fetch or check against a brief rather than make the decisions, so they are pinned to Sonnet, which costs less: a Pro teammate's session is unchanged, and an Opus session pays Opus only for itself and the inheriting agents. Each returns a digest or a fixed block, so your session holds what it decides on rather than the raw material. (The README, "Models".) -**The build's review scales with the change.** Each reviewer in a wave is fresh, so none defends what it said in an earlier wave, and every digest is merged before anything is fixed, so two reviewers pulling against each other are seen together; where they do, conformance to the agreed plan wins. A focus runs only when the diff gives it something to judge: simplicity on changed code, documentation on changed code or docs, tests on changed code or tests, so a test-only change pays for no documentation reviewer. The coverage run is the implementation reviewer's suite run, so a wave runs the suite once rather than twice at once in one working tree, and untested new code is caught when the diff touches no test. A later wave reruns the implementation reviewer and only the focuses that still have a blocking finding, and the tests reviewer when a fix changed a test, since those tests are new to it. A build is light only by the profile's allowlist, its default included, or the person's word, and the floor rechecked over the diff only raises the depth. A removal runs `downstream check`, since libera_utils 5.10.3 removed product identifiers under a patch bump and libera_rad's main went red with no change of its own. A `pr-fix` round writes its wave reports under `.review/fix/pr-/`, since its waves count from 1 again and would otherwise overwrite the finished build's. A standard or deep build that changed code and converges is then tightened in up to two honing passes: a fresh simplicity reviewer reads the whole diff for what keeps the behavior and shortens the code, such as a comment that restates its line or runs longer than its file's, and a fresh implementation reviewer checks what was applied. Honing runs after convergence rather than inside the waves so a fix for a blocking finding never competes with tidying, and a tidy that breaks a test is undone rather than fixed forward. What a comment may say is the review contract's "Comments" section, read by the build's simplicity reviewer and by `pr-review`'s reviewer alike: a session note or an unkeyed follow-up is out of date once the change merges, so it is a should-fix, and later work is one `TODO` line with a ticket key. `comment_scan.py` marks the candidates by pattern before either reviewer reads the diff, rather than a further agent reading the same diff, since a keyless TODO or a phrase like "as requested" needs no judgment to find, only to settle. (`implement-change/reviews/wave.md`; `implement-change` step 8.) +**The build's review scales with the change.** Each reviewer in a wave is fresh, so none defends what it said in an earlier wave, and every digest is merged before anything is fixed, so two reviewers pulling against each other are seen together; where they do, conformance to the agreed plan wins. A focus runs only when the diff gives it something to judge: simplicity on changed code, documentation on changed code or docs, tests on changed code or tests, so a test-only change pays for no documentation reviewer. The coverage run is the implementation reviewer's suite run, so a wave runs the suite once rather than twice at once in one working tree, and untested new code is caught when the diff touches no test. A later wave reruns the implementation reviewer and only the focuses that still have a blocking finding, and the tests reviewer when a fix changed a test, since those tests are new to it. A build is light only by the profile's allowlist, its default included, or the person's word, and the floor rechecked over the diff only raises the depth. A removal runs `downstream check`, since libera_utils 5.10.3 removed product identifiers under a patch bump and libera_rad's main went red with no change of its own. A `pr-fix` round writes its wave reports under `.review/fix/pr-/`, since its waves count from 1 again and would otherwise overwrite the finished build's. A `verify` runs the same wave on work written with no plan, its goal standing in for the plan, rather than a skill of its own or a one-pass review, since the build hook already records an `implement-change` run and `pr-create` already reads a build with no plan. A standard or deep build that changed code and converges is then tightened in up to two honing passes: a fresh simplicity reviewer reads the whole diff for what keeps the behavior and shortens the code, such as a comment that restates its line or runs longer than its file's, and a fresh implementation reviewer checks what was applied. Honing runs after convergence rather than inside the waves so a fix for a blocking finding never competes with tidying, and a tidy that breaks a test is undone rather than fixed forward. What a comment may say is the review contract's "Comments" section, read by the build's simplicity reviewer and by `pr-review`'s reviewer alike: a session note or an unkeyed follow-up is out of date once the change merges, so it is a should-fix, and later work is one `TODO` line with a ticket key. `comment_scan.py` marks the candidates by pattern before either reviewer reads the diff, rather than a further agent reading the same diff, since a keyless TODO or a phrase like "as requested" needs no judgment to find, only to settle. (`implement-change/reviews/wave.md`; `implement-change` step 8; `implement-change/verify.md`.) **The ticket comes before the code.** The most useful review comments were often design questions asked after the code was written, so `ticket-draft` asks them first. It asks them against a draft: placement and a rough size come back first, so a duplicate or a split is decided before any question, then the draft with a recommended answer on each Open Question, since walking eight rounds of questions before any ticket text asked the person to hold the ticket in their head. Every question it raises is answered, marked open or dropped by a person; an open one goes to the ticket's Open Questions with why it stays open and what it affects, and every later step offers to settle it. A ticket may keep a question open, since an answer invented to pass a readiness gate is worse than an honest open question; the ticket writer judges readiness when moving it to To Do. The agent never invents a component, an epic key or a link target. No question crosses into the build unanswered, but an answer can be provisional: researched and chosen so the work can go on, and flagged to revisit, with why it is not final and what would make it final, since implementation often answers what planning could not. A provisional decision is shown again at the plan and diff gates, listed in the pull request's description and given to the reviewer as a focus point, since it is where the human review needs to look. Making a decision provisional costs a reason, so the flag keeps its meaning. (`ticket-draft` step 6; `using-jira`, "Never"; `GUIDE.md`, "Why".) diff --git a/sandbox/README.md b/sandbox/README.md index c522267..e48ed14 100644 --- a/sandbox/README.md +++ b/sandbox/README.md @@ -37,6 +37,8 @@ the review contract's product scenarios, `review-specified-retry`, whose review a retry the plan specifies, `review-fail-loud-decision`, whose wave raises a silent cache fallback under an adopted fail-loud decision, and `review-swallowed-exception`, whose wave raises a swallowed exception under the contract's floor alone, +`verify-adhoc-swallowed`, which runs `implement-change verify` on the same change written with no +plan, and `verify-asks-goal`, which asks for the goal of an unplanned change before any reviewer, `review-agent-guidance`, whose wave loads a standard's agent guidance and data conventions and raises a deprecated alias under its agent guidance against compatibility shims, and its control, `review-agent-guidance-control`, the same without the agent guidance, diff --git a/sandbox/scenarios.yaml b/sandbox/scenarios.yaml index 1188a5a..9692762 100644 --- a/sandbox/scenarios.yaml +++ b/sandbox/scenarios.yaml @@ -665,6 +665,68 @@ scenarios: agents: [implementation-reviewer] says: ["(?i)swallow|except Exception|silently|broad except|skip"] + - id: verify-adhoc-swallowed + repo: widgets + setup: + - git remote set-head origin main + - git config commit.gpgsign false + - git checkout -q -B inventory-total + - mkdir -p standards && printf '# Decisions\n' > standards/decisions.md && echo standards/ >> .git/info/exclude + - cp "$SBX_FIXTURES/swallow-total.py" src/widgets/total.py + - cp "$SBX_FIXTURES/swallow-test_total.py" tests/test_total.py + - git add src tests && git commit -qm "Total an inventory CSV" + - perl -pi -e 's/The sum of the count column\./The sum of the count column, 0 for a file with no rows./' src/widgets/total.py + - grep -q '0 for a file with no rows' src/widgets/total.py + origin: "ROADMAP.md, A route where the person writes the code: review for work written with no plan" + why: >- + Verifies the swallowed-exception change as work the person wrote with no plan, committed on + a branch with one edit uncommitted, so implement-change's verify runs the build's wave with + the goal as its brief and the diff since the default branch, and writes no plan. + turns: + - /lasp-dev:implement-change verify Total an inventory CSV + must_not_run: ["mcp__*", "Bash(git push*)", "Bash(git commit*)"] + look_at: >- + implement-change reads verify.md, takes the base from origin/HEAD's merge-base, writes + .review/standard.md and runs the wave with Plan none and the goal as the brief. A reviewer + raises the swallowed exception as a should-fix or must-fix; the wave may fix it, and step 9's + gate lists each fix by finding and file. No plan file is written and nothing is committed. + verify: + - test -f .review/build/wave-1-conformance.md + - "! ls .review/implementation-plan-* 2>/dev/null" + expect: + ends: success + agents: [implementation-reviewer] + says: ["(?i)swallow|except Exception|silently|broad except|skip"] + files_absent: [".review/implementation-plan-*"] + + - id: verify-asks-goal + repo: widgets + setup: + - git remote set-head origin main + - git config commit.gpgsign false + - git checkout -q -B inventory-total + - mkdir -p standards && printf '# Decisions\n' > standards/decisions.md && echo standards/ >> .git/info/exclude + - cp "$SBX_FIXTURES/swallow-total.py" src/widgets/total.py + - cp "$SBX_FIXTURES/swallow-test_total.py" tests/test_total.py + - git add src tests && git commit -qm "Total an inventory CSV" + - perl -pi -e 's/The sum of the count column\./The sum of the count column, 0 for a file with no rows./' src/widgets/total.py + - grep -q '0 for a file with no rows' src/widgets/total.py + origin: "ROADMAP.md, A route where the person writes the code: review for work written with no plan" + why: >- + Asks for verify on work written with no plan and gives no goal, so implement-change's + verify asks for the goal once before any reviewer runs. Plain words reaching verify is + trigger-implement-change-verify's to show. + turns: + - /lasp-dev:implement-change verify + must_not_run: ["mcp__*", "Bash(git push*)", "Bash(git commit*)", "Edit", "Write(*/repos/widgets/[!.]*)"] + look_at: >- + implement-change says when the person is needed and asks one + question: the goal of the change, beside which untracked files belong to it when there are + any. No reviewer runs before the answer. + expect: + ends: success + agents_absent: [implementation-reviewer] + - id: review-agent-guidance repo: widgets setup: diff --git a/sandbox/triggers/coverage.yaml b/sandbox/triggers/coverage.yaml index f33a80b..bb99169 100644 --- a/sandbox/triggers/coverage.yaml +++ b/sandbox/triggers/coverage.yaml @@ -1,4 +1,4 @@ # The trigger pack's set: every scenario in scenarios.yaml beside this file, so sandbox/smoke.py # with --scenarios sandbox/triggers/scenarios.yaml runs the whole pack on both checkouts. -smoke: [trigger-architecture-doc, trigger-architecture-doc-confluence, trigger-commit, trigger-commit-history, trigger-design-doc, trigger-downstream, trigger-downstream-notice, trigger-drawio, trigger-implement-change, trigger-implement-change-finished, trigger-onboard, trigger-pr-create, trigger-pr-fix, trigger-pr-fix-push-back, trigger-pr-review, trigger-pr-review-once-over, trigger-process-doc, trigger-process-doc-wiki, trigger-quickstart, trigger-rebase, trigger-skill-feedback, trigger-test-suite-review, trigger-test-suite-review-solid, trigger-ticket-draft, trigger-ticket-draft-github-issue, trigger-work, trigger-writing, trigger-writing-rambles, notrigger-ticket-draft-other-project, notrigger-ticket-draft-plugin-issue] +smoke: [trigger-architecture-doc, trigger-architecture-doc-confluence, trigger-commit, trigger-commit-history, trigger-design-doc, trigger-downstream, trigger-downstream-notice, trigger-drawio, trigger-implement-change, trigger-implement-change-finished, trigger-implement-change-verify, trigger-onboard, trigger-pr-create, trigger-pr-fix, trigger-pr-fix-push-back, trigger-pr-review, trigger-pr-review-once-over, trigger-process-doc, trigger-process-doc-wiki, trigger-quickstart, trigger-rebase, trigger-skill-feedback, trigger-test-suite-review, trigger-test-suite-review-solid, trigger-ticket-draft, trigger-ticket-draft-github-issue, trigger-work, trigger-writing, trigger-writing-rambles, notrigger-ticket-draft-other-project, notrigger-ticket-draft-plugin-issue] diff --git a/sandbox/triggers/scenarios.yaml b/sandbox/triggers/scenarios.yaml index 31c7653..524fd12 100644 --- a/sandbox/triggers/scenarios.yaml +++ b/sandbox/triggers/scenarios.yaml @@ -125,6 +125,21 @@ scenarios: ends: success skills: [implement-change] + - id: trigger-implement-change-verify + repo: widgets + setup: + - perl -pi -e 's/raise ValueError\(f"part \{w\.part\} is listed twice"\)/counts[w.part] += w.count; continue/' src/widgets/inventory.py + - grep -qF 'counts[w.part] += w.count; continue' src/widgets/inventory.py + origin: "ROADMAP.md, A route where the person writes the code: review for work written with no plan" + why: Asks in plain words for a review of a change the person wrote by hand, with no plan and no goal, so a run shows whether it reaches implement-change's verify, which stops at its goal question. + turns: + - I changed by_part by hand. Can you check it's ready to commit? + must_not_run: ["mcp__*", "Bash(git push*)", "Bash(gh *)", "Bash(git commit*)", "Edit", "Write(*/repos/widgets/[!.]*)"] + look_at: The request reaches implement-change, which says when the person is needed and asks for the goal of the uncommitted change. + expect: + ends: success + skills: [implement-change] + - id: trigger-onboard repo: widgets turns: diff --git a/skills/implement-change/SKILL.md b/skills/implement-change/SKILL.md index 1011ba3..a281f5a 100644 --- a/skills/implement-change/SKILL.md +++ b/skills/implement-change/SKILL.md @@ -2,13 +2,16 @@ name: implement-change description: > Takes one change request (a file, a Jira ticket, a GitHub issue or the prompt) to an agreed - plan, verified code and an offered pull request. Use when asked to "implement this", "build - this feature" or "plan and implement". Not for routing a ticket to its next phase, which is - work. -argument-hint: "[file | ticket key | issue | description]" + plan, verified code and an offered pull request; `verify` puts work written with no plan through + the same review. Use when asked to "implement this", "build this feature", "plan and + implement", "verify this change", "review what I just wrote" or "check this is ready to + commit". Not for routing a ticket to its next phase, which is work; a pull request's review, + which is pr-review; the tests alone, which is test-suite-review; or one pass over a diff, which + is /code-review. +argument-hint: "[file | ticket key | issue | description] | verify [goal]" metadata: author: llm-tooling - version: 1.19.0 + version: 1.20.0 --- # Implement Change @@ -22,7 +25,7 @@ it writes, follows `${CLAUDE_PLUGIN_ROOT}/skills/work/decision-points.md`. The p | Option | Default | |---|---| -| Source | A markdown or text file, a Jira ticket, a GitHub issue or discussion, or the change described in the prompt; asked for when none is given | +| Source | A markdown or text file, a Jira ticket, a GitHub issue or discussion, or the change described in the prompt; asked for when none is given; a verify asks for its goal instead | ## How Each Turn Reads @@ -71,6 +74,9 @@ steps that build on it. One with no reason it is not final is a final decision. **Read on these paths only:** +- **Invoked as `verify`, or asked to review or verify work written with no plan.** Read + `${CLAUDE_SKILL_DIR}/verify.md` in place of steps 1 to 7 and of the next bullet: only an + approved plan at `where.sh`'s `plan=` for this branch or ticket goes to `resume.md`. - **A plan for this work may exist.** Before step 1, run `where.sh --fields`, with `--ticket ` when the invocation names a ticket. When its `plan=` file exists, or with no ticket a draft `.review/implementation-plan-*.md`, one with no `Approved by:` line, not yet branched for, @@ -417,8 +423,10 @@ that waits for their answer is not an end. ## Rules - **The person approves the plan before any file is touched**, and approves the diff before it is - committed. The plan's approval is an answer to step 5's gate, or a request that names it: an - instruction given before the plan is shown that does not, such as "take your recommended answer - to every question", settles the gate's other questions, never the approval. + committed. A verify has no plan: the person asked for a review of work already written, and + the wave's fixes are shown at the diff gate. The plan's approval is an answer to step 5's gate, + or a request that names it: an instruction given before the plan is shown that does not, such + as "take your recommended answer to every question", settles the gate's other questions, never + the approval. - **Never invent a requirement the source does not carry.** A gap is an assumption you state in the brief or a question you ask, not a decision you bury in the code. diff --git a/skills/implement-change/reviews/wave.md b/skills/implement-change/reviews/wave.md index df1c0e6..93ae2bf 100644 --- a/skills/implement-change/reviews/wave.md +++ b/skills/implement-change/reviews/wave.md @@ -1,12 +1,14 @@ # The review wave -How a change is verified: `implement-change` step 8 runs it for a build, and `pr-fix` step 9 for -a round of review fixes. The plan file, the brief and the report folder are the caller's: -`implement-change` writes the wave reports to `.review/build/`, `pr-fix` to `.review/fix/pr-/`, -so a round of fixes leaves the build's reports alone. The base is a commit, never a branch name: -`implement-change` passes `git merge-base HEAD`, and `pr-fix` passes -`HEAD`. Every reviewer and `surface_diff.py` get that commit, so commits the base branch gains -during the build are not read, reversed, as part of the change. Each profile value on this page +How a change is verified: `implement-change` step 8 runs it for a build, `pr-fix` step 9 for a +round of review fixes, and `implement-change verify` for work written with no plan, its plan file +`none` and its brief the goal the person gave. The plan file, the brief and the report folder are +the caller's: `implement-change` writes the wave reports to `.review/build/`, a verify included, +`pr-fix` to `.review/fix/pr-/`, so a round of fixes leaves the build's reports alone. The base +is a commit, never a branch name: `implement-change` passes `git merge-base HEAD`, a verify `git merge-base HEAD`, and `pr-fix` passes `HEAD`. +Every reviewer and `surface_diff.py` get that commit, so commits the base branch gains during the +build are not read, reversed, as part of the change. Each profile value on this page is `python3 "/scripts/profile.py" --field `. ## Depth @@ -163,7 +165,8 @@ Stop and bring the person the latest wave's reports on any of these, the one lis - A fix would weaken an existing test: a loosened tolerance or assertion, a removed `pytest.raises`, a new skip or xfail, a dropped parametrize case (`stopped for a person: weakens `). -- A finding would change the agreed plan (`stopped for a person: changes the plan`). +- A finding would change the agreed plan (`stopped for a person: changes the plan`), or, with no + plan, the brief's goal or scope (`stopped for a person: changes the brief`). - A finding would move code across a repository boundary (`stopped for a person: crosses a repository`). - `downstream check` says a consumer breaks (`stopped for a person: breaks `). diff --git a/skills/implement-change/verify.md b/skills/implement-change/verify.md new file mode 100644 index 0000000..4b443b9 --- /dev/null +++ b/skills/implement-change/verify.md @@ -0,0 +1,52 @@ +# Verifying work written with no plan + +`implement-change` reads this when invoked as `verify [goal]`, or asked to review or verify work +the person wrote without a plan. It runs steps 8 to 12 on the branch as it stands, with the goal in +place of the plan. Open by saying when the person is needed (the goal, then the diff) and that the +wave fixes blocking findings in the working tree and commits nothing. + +**An approved plan for this branch.** When `bash "/hooks/where.sh" --fields` gives a +`plan=` file with an `Approved by:` line, this is that build: read +`/skills/implement-change/resume.md` and go on where it says, unless the person says +to verify without it. + +**The base.** `git symbolic-ref --short refs/remotes/origin/HEAD` names the default branch; the +base is `git merge-base HEAD`, so the change is the commits since then, the working tree and +the untracked files the person says belong to it. With no `origin/HEAD`, ask which branch is the +default; with no answer, stop. With nothing changed against the base and no untracked files, say +so and stop. A stop here releases the working tree as step 12 says. + +**Ask in one widget**, skipping a question the invocation answers: the goal, one line on what is +different when this is done, when the invocation gives none; and, when the tree has untracked +files, which belong to the change, the rest going on the wave's `Not part of the change:` line. +The brief is that goal, what the goal needs within the change's files as its scope and the goal +met as its acceptance criterion; nothing else is asked about it. + +**Before the wave.** In a repository with a standard, write `.review/standard.md` as +`/skills/implement-change/standard-repo.md`'s "Every agent gets the standard" says. +When `.review/build/outcome.md` exists, go back into the build as "The Build Record" says; the +waves number on from the last. + +**The depth.** Run `python3 "/scripts/depth_floor.py" ` as step 8 does, an +untracked path the change does not own handled as it says there. Light runs the light build and +standard the wave, as `/skills/implement-change/reviews/wave.md` "Depth" sets out; +the depth is never lowered. At deep, run the wave as at standard and record `Depth: deep`: step +8's return to step 3 does not apply, since there is no plan to draft, and the close names a +planned build, `/lasp-dev:implement-change `, as the alternative. + +**Step 8** runs as written, each reviewer's header carrying `Plan: none` and the goal as `Brief:`, +and a finding that would change the goal or the scope stopping as `wave.md`'s "Stops" says. +`outcome.md`'s `Nominated:` and `Procedure:` lines hold the corrections made at step 9, a verify +having no plan's `## Corrections`, and read `none` when there are none. + +**Step 9** shows the diff grouped by file, not by plan step, and lists each fix the wave made in +the person's work, by finding and file. On the default branch, the same widget asks for a branch +name, in the repository's branch naming, and step 10 branches before it commits, unless the +repository's instructions say it commits to its default branch, as step 6 says. A change asked +for here is a diff-gate correction, kept for those lines: go back into the build as "The Build +Record" says, make the change directly, since verify has no step 7, and run step 8 again. + +**Steps 10 to 12** run as written. Step 11 hands `pr-create` the goal, in the person's words, as +the why. The last line of step 12 names `/lasp-dev:pr-create` when no pull request was opened, or +`/lasp-dev:implement-change verify` to go back in, since `/lasp-dev:work` routes no build +without a plan; once one is open, it names `/lasp-dev:work`. diff --git a/tests/test_sandbox.py b/tests/test_sandbox.py index 85a5cd4..201f6f3 100644 --- a/tests/test_sandbox.py +++ b/tests/test_sandbox.py @@ -515,7 +515,7 @@ def test_a_line_separator_inside_a_string_is_not_a_line_break(self): def test_every_generic_scenario_is_well_formed_and_runs_in_the_fixture(self): known = run.scenarios() - self.assertEqual(len(known), 36) + self.assertEqual(len(known), 38) well_formed(self, known, {"widgets"}) def test_every_fixture_a_scenario_copies_is_in_the_pack(self):