diff --git a/.claude-plugin/marketplace.json b/.claude-plugin/marketplace.json index 16bbc0a8..8af030a6 100644 --- a/.claude-plugin/marketplace.json +++ b/.claude-plugin/marketplace.json @@ -66,7 +66,7 @@ { "name": "plan-lifecycle", "source": "./plugins/plan-lifecycle", - "description": "Cross-tool GitHub-issue plan lifecycle: a marker-based v3 issue-body record, six phases (decide, draft, research, one plan review, implement, code review), and two read-only reviewer wrappers, plan-reviewer and code-reviewer.", + "description": "Cross-tool GitHub-issue plan lifecycle with marker-based plan bodies, comment-backed review records, implement-start linked branches, bounded plan and code review repair loops, and two read-only reviewer wrappers.", "version": "0.7.0", "author": { "name": "Eduardo Marquez" diff --git a/.codex/agents/code-reviewer.toml b/.codex/agents/code-reviewer.toml index ad3acc4e..455d0250 100644 --- a/.codex/agents/code-reviewer.toml +++ b/.codex/agents/code-reviewer.toml @@ -46,14 +46,25 @@ Verdicts. Select exactly one: and does not change reviewed bytes after the pass; they never trigger a re-review. - `fixes-required`: At least one evidenced `CRITICAL` or `HIGH` defect. The - manager fixes it and dispatches exactly one repair re-review. + manager fixes every named defect and dispatches a fresh re-review on a fresh + diff. - `blocked`: Required review input is unreadable or contradictory, so no safe verdict can be reached. A pass block carries only advisory MEDIUM and LOW lines, or none. A fixes-required or blocked block has at least one finding line. Remain read-only. Never apply a fix and never ask for approval to apply one. -Return one readable `Code-review:` markdown block to the manager. These inline -Standards buckets, severity caps, and Spec axis keep this wrapper complete when -the runtime skills are unavailable. +Return exactly one readable `Code-review:` markdown block to the manager, which +posts that whole block unchanged as one issue comment and owns fixes and fresh +re-review dispatch. The block must be the parser-compatible comment record, and +this exact shape overrides any report layout from a loaded `code-review` skill: + + ### Code review round — + Code-review: + - · · — — + +Use the round number the manager supplies, one finding per line, and no prose +outside the block. These inline Standards buckets, severity caps, Spec axis, +and record shape keep this wrapper complete when the runtime skills are +unavailable. """ diff --git a/.codex/agents/plan-reviewer.toml b/.codex/agents/plan-reviewer.toml index 21cb232c..ea6585ee 100644 --- a/.codex/agents/plan-reviewer.toml +++ b/.codex/agents/plan-reviewer.toml @@ -1,5 +1,5 @@ name = "plan-reviewer" -description = "Use when plan-manager needs one read-only pre-implementation review of a canonical plan against repository facts and official documentation. Not for code review, plan edits, implementation, user decisions, lifecycle changes, or direct user invocation." +description = "Use when plan-manager needs a read-only pre-implementation review round for a canonical plan against repository facts and official documentation. Not for code review, plan edits, implementation, user decisions, lifecycle changes, or direct user invocation." sandbox_mode = "read-only" developer_instructions = """ # Plan Reviewer @@ -11,6 +11,8 @@ Read the plan body from the export path the manager supplies; it is an absolute A plan-review finding is exactly one of `goal_fit`, `research_gap`, or `security_risk`; nothing else is a finding. A sufficient plan passes. Remain read-only. Never write, dispatch an agent, run a mutating command, or ask -the user. Return one readable `Plan-review:` markdown block to the manager. The -canonical skill owns the review workflow and output contract. +the user. Return exactly one readable `Plan-review:` markdown block to the +manager, which posts that whole block as one issue comment and owns repairs and +fresh re-review dispatch. The canonical skill owns the review workflow and +output contract. """ diff --git a/AGENTS.md b/AGENTS.md index 3c6009a0..3bb88dcb 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -83,11 +83,12 @@ The plan record is a GitHub issue. Its body starts with sections; it has no frontmatter. GitHub owns title, open-work phase, owner, timestamps, and completion, and no plan markdown is tracked in the repository. Exactly three skills own the workflow: `plan-workspace` maintains the workspace; -main-context `plan-manager` runs six phases — decide, draft, research, one plan -review, implement, code review — and archives; internal `plan-reviewer` returns -a readable pre-implementation verdict. Two read-only reviewer wrappers ship, -`plan-reviewer` and `code-reviewer`, and nothing else in the lifecycle has a -wrapper. +main-context `plan-manager` runs six phases — decide, draft, research, plan +review, implement, code review — with bounded repair and fresh re-review in both +review phases, then archives; internal `plan-reviewer` returns one readable +pre-implementation verdict block per round. Two read-only reviewer wrappers +ship, `plan-reviewer` and `code-reviewer`, and nothing else in the lifecycle has +a wrapper. After the marker and blank line, the record carries exactly `## Goal`, @@ -96,17 +97,36 @@ After the marker and blank line, the record carries exactly `## Goal`, and once each. `## Goal` carries exactly one mode line. Open-work phase is one of `drafting`, `planned`, `ongoing`, or `blocked` in a `plan:` label; a blocked plan starts `## Open questions` with `Blocked: `. -Closed completion derives from GitHub `state` and `stateReason`. The record -carries no hash, permit, run identity, lock, or bundle, and the `plan.mjs` -shipped inside the installed `plan-lifecycle` plugin is the only lifecycle tool. -An `export` writes the sha256 of the body it copied beside the copy so a stale -copy cannot revert the record; that digest detects staleness and authorizes -nothing. Routine plan issue publication and post-review landing through a -branch, commit, normal push, and closing pull request carry the settled mode's -authorization and need no repeated prompt. After the checks policy passes, the -manager asks immediately before merge. Without a fresh `Merge now` answer, it -leaves the pull request and issue open. `plan.mjs archive` verifies the merged -closing pull request after landing. +Closed completion derives from GitHub `state` and `stateReason`. `## Review` +contains exactly `_Review records are stored in issue comments._`. Each reviewer +returns one markdown block, and the manager posts that whole block as one issue +comment. The latest trusted well-formed record per review kind wins; its author +must equal the plan's sole assignee. A legacy body verdict is consulted only +when no trusted comment record exists for that kind. Both review phases use +fresh inputs and run at most five rounds, stopping on pass, no progress, a +finding surviving its fix, or `repair` or `fixes-required` in round five. A +plan-review `blocked` verdict always routes its user-only decision through +`## Open questions` and `ask`. + +The record carries no hash, permit, run identity, lock, or bundle, and the +`plan.mjs` shipped inside the installed `plan-lifecycle` plugin is the only +lifecycle tool. An `export` writes the sha256 of the body it copied beside the +copy so a stale copy cannot revert the record; that digest detects staleness and +authorizes nothing. Routine plan issue publication, implement-start linked +branch creation, commits, normal pushes, and the closing pull request carry the +settled mode's authorization and need no repeated prompt. Before any branch +checkout, including `gh issue develop --checkout`, require +`git status --porcelain` to be empty. If it is dirty, never stash, move, or +commit ambient work; set the plan `blocked` and name the dirty paths, or use an +authorized clean worktree. +Immediately after setting the plan `ongoing`, every `gh issue develop` call uses +`--repo`; the manager reuses a linked branch or creates one with +`--base --checkout`, then re-lists and recovers after failure. +Implementation stops when no linked branch can be verified; there is no local +fallback. After the checks policy passes, the manager asks immediately before +merge. Without a fresh `Merge now` answer, it leaves the pull request and issue +open. `plan.mjs archive` verifies the latest trusted code-review result and +merged closing pull request after landing. Every Steps row carries an `Effect` of exactly `local|probe|production_access|publish|push|release|deploy`. A step whose @@ -118,8 +138,9 @@ actions are outside the Steps table. Render a plan body verbatim only when the user names that plan and asks to see it. After a write, report the one-line header strip and the changed lines only; a write never re-renders the body. -`docs/plans/finished/` is frozen pre-GitHub history: read it as history, never as -a source of truth, and never parse or migrate it. The complete contract lives in +`docs/plans/finished/` is frozen pre-GitHub history. Humans may read it as +history, but it is not a source of truth. No lifecycle command or workspace +migration operation opens or inventories it. The complete contract lives in `docs/PLAN.md`; `docs/AGENTS.md` routes to it and `docs/CLAUDE.md` contains only `@AGENTS.md`. diff --git a/README.md b/README.md index b42358bf..6249ab02 100644 --- a/README.md +++ b/README.md @@ -70,15 +70,16 @@ a canonical plan for explicit planning, multi-commit/cross-repository work, scheduling, cold handoff, unresolved decisions, cross-subsystem/public-contract changes, security-sensitive/destructive work, or an external effect. -The three lifecycle skills, shipped `plan.mjs`, markdown-only v2 contract +The three lifecycle skills, shipped `plan.mjs`, marker-based contract reference, and two read-only reviewer wrappers ship as the self-versioned `plan-lifecycle` plugin (`plugins/plan-lifecycle/`), installable from this same -marketplace. Plan records live in GitHub issue bodies. +marketplace. Plan bodies and comment-backed review records live on GitHub +issues. | Owner | Skill | Invocation | Responsibility | |---|---|---|---| | Workspace | `plan-workspace` | Public | Create the plan label set and maintain `docs/PLAN.md` plus the `docs/AGENTS.md`/`docs/CLAUDE.md` pair; never mutate an individual plan issue | -| Orchestration | `plan-manager` | Public, main context | Decide → draft → research → one plan review → implement → code review; archive after a passing review and a merged pull request closes the issue | +| Orchestration | `plan-manager` | Public, main context | Decide → draft → research → plan review → implement → code review; repair and freshly re-review both review phases, then archive after a pass and a merged closing pull request | | Plan review | `plan-reviewer` | Internal, read-only | Check only `goal_fit`, `research_gap`, and `security_risk` before implementation | | Code review | `code-reviewer` | Internal, read-only | Review the implemented diff against code standards and the plan | @@ -87,13 +88,16 @@ seeded as thin Claude/Codex wrappers; main context invokes `plan-manager` directly. The docks pipelines route to these skills and stop, naming the missing `plan-lifecycle` plugin, when they are unavailable. -The lifecycle runs six phases: decide, draft, research, one plan review, -implement, and one post-implementation code review. After review passes, it -commits and pushes the reviewed branch, opens the closing pull request, and -waits for repository CI. It then asks `Merge now` or -`Leave pull request open`. Without a fresh `Merge now` answer, it leaves the -pull request and issue open. After an approved merge, `plan.mjs archive` -verifies the merged closing pull request. +The lifecycle runs six phases: decide, draft, research, plan review, implement, +and code review. Plan repairs are re-reviewed from fresh exports, and code fixes +are re-reviewed from fresh diffs, with a five-round ceiling in each review +phase. Each reviewer returns one markdown block that the manager stores as one +issue comment. When implementation starts, the manager reuses or creates the +GitHub-linked plan branch. After code review passes, it commits and pushes any +remaining reviewed bytes, opens the closing pull request, and waits for +repository CI. It then asks `Merge now` or `Leave pull request open`. Without a +fresh `Merge now` answer, it leaves the pull request and issue open. After an +approved merge, `plan.mjs archive` verifies the merged closing pull request. Every Steps row has `Effect` exactly `local|probe|production_access|publish|push|release|deploy`. Each non-`local` @@ -112,7 +116,7 @@ The complete contract lives in `docs/PLAN.md`. │ │ ├── .claude-plugin/plugin.json │ │ ├── skills/ ← cross-tool skills │ │ └── README.md ← plugin-facing docs -│ ├── plan-lifecycle/ ← GitHub-issue plan lifecycle plugin (three skills + plan.mjs + v2 contract + two read-only reviewer wrappers) +│ ├── plan-lifecycle/ ← GitHub-issue plan lifecycle plugin (three skills + plan.mjs + marker contract + two read-only reviewer wrappers) │ └── effect-kit/ ← Effect-TS skill kit plugin ├── scripts/ ← plugin-author tooling (NOT shipped to users) │ ├── ci.mjs / release.mjs ← orchestrators (the gate ci.yml runs) diff --git a/docs/PLAN.md b/docs/PLAN.md index cfa37d1b..066a5c5d 100644 --- a/docs/PLAN.md +++ b/docs/PLAN.md @@ -1,8 +1,9 @@ # PLAN.md — plan record standard The plan record is a GitHub issue. Its body carries the v3 byte contract and the -human-authored plan, while GitHub fields carry the machine state GitHub already -owns. No plan markdown is tracked in the repository. +human-authored plan, review records live in issue comments, and GitHub fields +carry the machine state GitHub already owns. No plan markdown is tracked in the +repository. Use direct implementation for one clear, reversible, low-risk local diff with one bounded acceptance path; it creates no plan issue, reviewer, or automatic @@ -24,17 +25,19 @@ The two read-only wrappers are `plan-reviewer` and `code-reviewer`. Main context ## Where the record lives ```text -GitHub issue # the plan record: title, body, labels, assignee, state +GitHub issue # the plan body, labels, assignee, and state +GitHub issue comments trusted plan-review and code-review records GitHub timestamps the record's creation and last-update times docs/PLAN.md this standard docs/PLAN-QUEUE.md optional discovery and priority view -docs/plans/finished/ frozen pre-GitHub history, read-only, never parsed +docs/plans/finished/ frozen pre-GitHub history, not lifecycle input ``` The issue number is the plan identity. There is no slug, no plan path, and no tracked plan file. `docs/plans/finished/` holds records written before the -lifecycle moved to issues; it is history, never a source of truth, and no -command reads it. +lifecycle moved to issues. Humans may read it as history, but it is never a +source of truth. No lifecycle command or workspace migration operation opens, +inventories, parses, classifies, lists, or migrates it. This backend is a deliberate trade. An issue body has no reviewable diff, no `git blame`, no CODEOWNERS, and no presubmit validation, which is why large @@ -77,6 +80,8 @@ retired and never live fields in a v3 record: `plan_contract`, `title`, `goal`, `status`, `created`, `updated`, `assignee`, and `blocked_reason`. `## Goal` still contains exactly one `Mode: plan-and-implement` or `Mode: plan-only` line. Mode stays in the body because GitHub has no field that owns this plan-specific choice. Once an open plan leaves `drafting`, `## Research` must no longer carry the template placeholder `_Not researched yet._`. +`## Review` contains exactly `_Review records are stored in issue comments._`; +review reports are not appended to the body. A blocked plan carries its reason as the first content line of `## Open questions`. Spell it `Blocked: `. Only a blocked plan may open that section @@ -187,7 +192,7 @@ resolves the target repository from that checkout's GitHub remote. | `plan.mjs labels [--extra ]…` | Create or update the exact five-label lifecycle set with `gh label create --force`, plus any extra topic labels named on the command line. | | `plan.mjs new --title --goal [--mode plan-and-implement\|plan-only] [--label ]…` | Create a v3 issue whose body starts with the marker, with labels `plan` and `plan:drafting`, and assign the creating login. | | `plan.mjs claim ` | Take single-writer ownership of an existing plan: assign the acting login, stay idempotent when it already owns the plan, and refuse when another login does. | -| `plan.mjs show [--body]` | Print the one-line header strip. `--body` prints the record to stdout with nothing else, sending the header strip to stderr, and is the only way to obtain the record. | +| `plan.mjs show [--body]` | Print the header strip, then `reviews: plan= code=`. With `--body`, print only the body to stdout and send both metadata lines to stderr, header first. | | `plan.mjs export ` | Write the issue body verbatim to `plan-.md` inside the scratch directory `git rev-parse --git-path docks-review` resolves, creating it mode 0700 when missing, and print the absolute path. | | `plan.mjs edit --file ` | Validate the file as the plan record, refuse on any failed check, replace the issue body, and print the header strip and the changed lines. | | `plan.mjs check >` | Validate a v3 record and print the pass result. | @@ -195,7 +200,7 @@ resolves the target repository from that checkout's GitHub remote. | `plan.mjs step ` | Rewrite one Steps `Status` cell after checking the plan state and dependencies. | | `plan.mjs list [--status ]` | Print `\t#\t` for every issue labelled `plan`, deriving `unlabelled`, `finished`, `retired`, and `duplicate` rather than reading them from the body. | | `plan.mjs next` | Print startable open plans, using the queue when it is present and valid. | -| `plan.mjs archive <issue>` | Verify terminal steps, passed code review, completed closure, and an eligible merged closing pull request; strip stale phase labels and write no status. | +| `plan.mjs archive <issue>` | Verify terminal steps, the latest trusted code-review result (with legacy body fallback only when no trusted comment record exists), completed closure, and an eligible merged closing pull request; strip stale phase labels and write no status. | | `plan.mjs retire <issue> --reason <text>` | Close the issue as not planned and strip all phase labels; completion derives as `retired`. | Legal step transitions are `planned → in-flight | done | blocked | skipped`, `in-flight → done | blocked | skipped`, and `blocked → in-flight | done | skipped`. @@ -203,12 +208,14 @@ Legal step transitions are `planned → in-flight | done | blocked | skipped`, ` ## Archive verification `plan.mjs archive` is a verifier, not a writer of lifecycle state. It requires -all Steps rows to be terminal (`done` or `skipped`), a line matching exactly -`Code-review: pass` in `## Review`, and an issue already closed as completed by -an eligible merged pull request. It writes no status. On success it removes any -stale phase label and prints `plan #<n> finished (closed by <url>)`. The pass -line may carry advisory `MEDIUM` and `LOW` finding lines beneath it; only an -unfixed `CRITICAL` or `HIGH` keeps a plan from archiving. +all Steps rows to be terminal (`done` or `skipped`), the latest trusted +well-formed code-review comment to carry `Code-review: pass`, and an issue +already closed as completed by an eligible merged pull request. It accepts an +exact legacy body line `Code-review: pass` only when no trusted well-formed +code-review comment exists. It writes no status. On success it removes any +stale phase label and prints `plan #<n> finished (closed by <url>)`. A pass may +carry advisory `MEDIUM` and `LOW` finding lines; an unfixed `CRITICAL` or `HIGH` +keeps a plan from archiving. The verifier reads the issue's `closedByPullRequestsReferences` with `excludeUserLinked: true`. It accepts only keyword-linked merged pull requests. @@ -275,42 +282,85 @@ file for a body write; a reviewer never edits it. Every dispatch re-exports first, so a reviewer always reads the current record rather than a half-staged edit. -The header strip is one line: `#<issue> · <status> · <title> · <url>`. +The header strip is `#<issue> · <status> · <title> · <url>`. `show` prints +`reviews: plan=<pass|repair|blocked|none> code=<pass|fixes-required|blocked|none>` +on the next line. With `show --body`, the record alone goes to stdout and both +metadata lines go to stderr, header first. -## Review records +## Review records — one issue comment per reviewer report -Append review records to `## Review` in these readable shapes: +`## Review` is a static pointer, not a review log: + +```markdown +_Review records are stored in issue comments._ +``` + +Before every dispatch, the manager runs `plan.mjs export <issue>` and passes the +printed absolute path; the reviewer reads the export path the manager supplies. +For code review, the manager also supplies a fresh complete-candidate diff. + +The reviewer returns exactly one markdown block. The manager posts that whole +block as one issue comment without editing it. The two exact shapes are: ```markdown ### Plan review — <date> Plan-review: pass|repair|blocked - [goal_fit] `## Steps` row 4 — the step removes the validator without replacing it — add the replacement before removal +``` +```markdown ### Code review round <n> — <date> Code-review: pass|fixes-required|blocked - HIGH · Security · plugins/x/y.mjs:41 — user input reaches a shell command unquoted — pass an argument array ``` +A well-formed record occupies the whole comment. It has the matching heading, +then exactly one verdict line, then zero or more nonblank finding lines. Extra +prose, multiple records, a missing heading, or an invalid verdict makes the +comment ineligible. `Plan-review:` is exactly `pass`, `repair`, or `blocked`. +`Code-review:` is exactly `pass`, `fixes-required`, or `blocked`. + +A record is trusted only when the issue has exactly one assignee and the +comment's author login equals that assignee. For each review kind independently, +the latest trusted well-formed comment wins, ordered by `createdAt` with API +order as the tie-break. Foreign-authored, malformed, and superseded comments +never establish current review state. A legacy verdict in the body is consulted +for one review kind only when there is no trusted well-formed comment record of +that kind. + A code-review `pass` means no `CRITICAL` or `HIGH` finding stands unfixed; it -carries only advisory `MEDIUM` and `LOW` lines, or none. Record each advisory as -a follow-up and do not change reviewed bytes after a pass; an advisory never -triggers a re-review. -`fixes-required` names at least one evidenced `CRITICAL` or `HIGH` defect and -forces exactly one repair re-review; -if that re-review still returns `fixes-required`, the manager appends -`Code-review: blocked` and sets the plan `blocked`. A `blocked` verdict has at -least one finding line. A plan-review finding is exactly one of `goal_fit`, -`research_gap`, or `security_risk`; nothing else is a finding. A sufficient plan -passes. +carries only advisory `MEDIUM` and `LOW` lines, or none. After a pass, record +each advisory as follow-up work and do not change reviewed bytes; advisory +findings never trigger another review. `fixes-required` names at least one +evidenced `CRITICAL` or `HIGH` defect. A `blocked` verdict has at least one +finding line. + +A plan-review finding is exactly one of `goal_fit`, `research_gap`, or +`security_risk`; nothing else is a finding. A sufficient plan passes. + +Both review phases run at most five rounds. Each round uses a fresh plan export; +each code-review round also uses a fresh complete-candidate diff. On rounds 1 +through 4, a `repair` or `fixes-required` verdict requires every reproduced or +named finding to be fixed, followed by a fresh export or diff and a fresh +review. A repair that changes no relevant bytes is no progress. A finding +repeated in the next round survived its fix. Either condition stops the loop, +as does `repair` or `fixes-required` in round 5; there is no sixth-round repair. + +A plan-review `blocked` verdict routes its user-only decision through +`## Open questions` and `ask`; the verdict alone is not a lifecycle block. A +technical code-review `blocked` verdict stops immediately. After implementation +has started, a technical block or any terminal repair failure requires the +manager to commit and normally push all current work to the verified linked +branch before recording the blocker, setting the plan `blocked`, and stopping. ## Phases 1. **Decide.** Phase 1 asks exactly one question with exactly three options, in this order and wording: `Plan and implement now`, `Plan only, stop at planned`, `Implement directly` — and skips the question only when the request already settles the mode. 2. **Draft.** Create the plan issue, write the goal and research hypothesis, and keep provisional Steps and Acceptance tables while status remains `drafting`. 3. **Research.** Verify repository facts and external claims, record their sources, choose the durable fix, bind the exact files, complete Acceptance, pass `plan.mjs check`, and set the plan `planned`. -4. **Plan review.** Dispatch exactly one pre-implementation review. Append its verdict and findings. Fix reproduced findings before implementation. A user-only decision goes in `## Open questions`. A plan-only run stops at `planned` after this review. -5. **Implement.** Set the plan `ongoing`, move each step through its legal states, and record real Acceptance output in `## Verification Results` before the closing merge. -6. **Code review.** Review the declared change, fix every critical and high finding, and run exactly one repair re-review after such a fix; if that re-review still returns fixes-required, append `Code-review: blocked` and set the plan `blocked`. Every step must be terminal and code review must pass before the closing merge; archive verifies those facts afterward. +4. **Plan review.** Run up to five rounds from fresh exports. Post each reviewer block as one issue comment. Fix reproduced findings and dispatch a fresh review; stop on pass, no progress, a finding surviving its fix, or `repair` in round five. Route every `blocked` user-only decision through `## Open questions` and `ask`, including in round five. A plan-only run stops at `planned` only after plan review passes. +5. **Implement.** Set the plan `ongoing`, verify and check out its GitHub-linked branch before changing implementation bytes, move each step through its legal states, and record real Acceptance output in `## Verification Results` before the closing merge. +6. **Code review.** Run up to five rounds from fresh complete-candidate diffs and fresh plan exports. Post each reviewer block as one issue comment. Fix every critical and high finding and dispatch a fresh review; stop on pass, no progress, a finding surviving its fix, a technical block, or `fixes-required` in round five. Before any terminal review block, commit and normally push all current work to the linked plan branch. Every step must be terminal and code review must pass before the closing merge; archive verifies those facts afterward. Build the review diff from the complete candidate pull request, not only the dirty worktree. Resolve and fetch the repository default branch, then compute @@ -324,26 +374,44 @@ After pull-request creation, record `headRefOid` and compare the changed paths and hunks from `gh pr diff` with the reviewed net candidate. Any mismatch invalidates the pass and blocks merge. -If that repair re-review again returns `fixes-required`, stop: append -`Code-review: blocked` naming the surviving findings, and set the plan -`blocked`. A step whose `Effect` is not `local` requires an in-session `ask` confirmation immediately before it runs; when `ask` is unavailable the step is set `blocked` and `Blocked: <unconfirmed effect>` is recorded first in `## Open questions`. Routine plan issue publication is authorized by the settled mode and needs no -repeated repository picker. +repeated repository picker. Routine linked-branch creation, commits, and normal +pushes are authorized when a settled `plan-and-implement` run enters phase 5. +Immediately after setting the plan `ongoing`, resolve the target repository's +`nameWithOwner` and `defaultBranchRef.name`. + +Before any branch checkout, and specifically before any `gh issue develop +--checkout`, require `git status --porcelain` to be empty. If it is dirty, never +stash, move, or commit the ambient work. Set the plan `blocked` and name the +dirty paths, or continue only in an authorized clean worktree. + +Pass `--repo <nameWithOwner>` to every `gh issue develop` call. First run +`gh issue develop <issue> --repo <nameWithOwner> --list`. If it reports a +linked branch, verify that branch belongs to the resolved repository, fetch it, +and check it out. Otherwise run `gh issue develop <issue> --repo +<nameWithOwner> --base <default-branch> --checkout`. After either path, verify +that the checked-out branch is the issue's linked branch. + +After any list, create, fetch, or checkout failure, re-run the repository-scoped +`--list`. If it reports a linked branch, verify that branch belongs to the +resolved repository, fetch it, and check it out. If recovery cannot verify and +check out a linked branch, record the blocker, set the plan `blocked`, and stop. +There is no local or unlinked fallback, and implementation never starts on an +unverified branch. ## Landing Work lands through a pull request whose body carries `Closes #<issue>` and whose base is the repository default branch. -After `Code-review: pass`, the manager runs landing without another prompt: -ensure a non-default branch, commit exactly the reviewed bytes under -`docks:commit-discipline`, push normally, and create or update one pull request -that carries `Closes #<issue>` and targets the repository default branch. +After `Code-review: pass`, commit and push any remaining reviewed bytes, then +create or update one pull request carrying `Closes #<issue>` and targeting the +repository default branch. This landing work needs no additional prompt. Never treat an empty first checks result as success. Retry `gh pr checks --json name,bucket` at most 12 times with a 10-second delay until diff --git a/plugins/docks/README.md b/plugins/docks/README.md index ad9cc38e..ea686b41 100644 --- a/plugins/docks/README.md +++ b/plugins/docks/README.md @@ -60,13 +60,17 @@ Plus `write-skill`, `multi-tool-bridge`, and `zoom-out` under `productivity/`. The plan lifecycle ships separately as the self-versioned `plan-lifecycle` plugin in this same marketplace. It includes three skills, the shipped -`plan.mjs`, a markdown-only v3 plan contract, and the read-only `plan-reviewer` -and `code-reviewer` wrappers. After review passes, the manager commits and -pushes the reviewed branch, opens the closing pull request, waits for repository -CI, and asks `Merge now` or `Leave pull request open`. Without a fresh -`Merge now` answer, it leaves the pull request and issue open. After an approved -merge, `plan.mjs archive` verifies the merged closing pull request. Every docks -route into that lifecycle is fail-loud: +`plan.mjs`, a marker-based plan contract, and the read-only `plan-reviewer` and +`code-reviewer` wrappers. Plan repairs are re-reviewed from fresh exports, and +code fixes are re-reviewed from fresh diffs, with a five-round ceiling in each +review phase. Each reviewer returns one markdown block that the manager stores +as one issue comment. When implementation starts, the manager reuses or creates +the GitHub-linked plan branch. After code review passes, it commits and pushes +any remaining reviewed bytes, opens the closing pull request, waits for +repository CI, and asks `Merge now` or `Leave pull request open`. Without a +fresh `Merge now` answer, it leaves the pull request and issue open. After an +approved merge, `plan.mjs archive` verifies the merged closing pull request. +Every docks route into that lifecycle is fail-loud: Prerequisite: `plan-lifecycle` must be installed. If `plan-workspace` or `plan-manager` is unavailable, STOP, name the missing `plan-lifecycle` plugin, and do not create or mutate a plan. diff --git a/plugins/plan-lifecycle/.claude-plugin/plugin.json b/plugins/plan-lifecycle/.claude-plugin/plugin.json index 03a8495e..4ea3f0f9 100644 --- a/plugins/plan-lifecycle/.claude-plugin/plugin.json +++ b/plugins/plan-lifecycle/.claude-plugin/plugin.json @@ -1,6 +1,6 @@ { "name": "plan-lifecycle", - "description": "Cross-tool GitHub-issue plan lifecycle: a marker-based v3 issue-body record, six phases (decide, draft, research, one plan review, implement, code review), and two read-only reviewer wrappers, plan-reviewer and code-reviewer.", + "description": "Cross-tool GitHub-issue plan lifecycle with marker-based plan bodies, comment-backed review records, implement-start linked branches, bounded plan and code review repair loops, and two read-only reviewer wrappers.", "version": "0.7.0", "author": { "name": "Eduardo Marquez" diff --git a/plugins/plan-lifecycle/.codex-plugin/plugin.json b/plugins/plan-lifecycle/.codex-plugin/plugin.json index 65affa57..c1dccc12 100644 --- a/plugins/plan-lifecycle/.codex-plugin/plugin.json +++ b/plugins/plan-lifecycle/.codex-plugin/plugin.json @@ -1,7 +1,7 @@ { "name": "plan-lifecycle", "version": "0.7.0", - "description": "Cross-tool GitHub-issue plan lifecycle: a marker-based v3 issue-body record, six phases (decide, draft, research, one plan review, implement, code review), and two read-only reviewer wrappers, plan-reviewer and code-reviewer.", + "description": "Cross-tool GitHub-issue plan lifecycle with marker-based plan bodies, comment-backed review records, implement-start linked branches, bounded plan and code review repair loops, and two read-only reviewer wrappers.", "author": { "name": "Eduardo Marquez" }, @@ -20,7 +20,7 @@ "skills": "./skills/", "interface": { "displayName": "plan-lifecycle", - "shortDescription": "GitHub-issue lifecycle skills: workspace, manager orchestration, read-only reviewer.", + "shortDescription": "GitHub-issue plans with comment-backed review repair loops.", "category": "Productivity" } } diff --git a/plugins/plan-lifecycle/agents/code-reviewer.md b/plugins/plan-lifecycle/agents/code-reviewer.md index 054217a3..ca40bdbc 100644 --- a/plugins/plan-lifecycle/agents/code-reviewer.md +++ b/plugins/plan-lifecycle/agents/code-reviewer.md @@ -16,8 +16,8 @@ rules below remain complete when those skills are unavailable. <constraint> Remain read-only. Never apply a fix. Never edit the plan or diff. Never dispatch another agent. Never ask for approval to apply findings and never ask the user a -question. Return the report to the manager, which owns fixes, re-review, and all -lifecycle changes. +question. Return the report to the manager, which owns fixes, fresh re-review +dispatch on a fresh diff, issue-comment publication, and all lifecycle changes. </constraint> <constraint> @@ -84,14 +84,17 @@ buckets or the Spec axis defined below. follow-ups and does not change reviewed bytes after the pass; they never trigger a re-review. - `fixes-required`: At least one evidenced `CRITICAL` or `HIGH` defect. The - manager fixes it and dispatches exactly one repair re-review. + manager fixes every named defect and dispatches a fresh re-review on a + fresh diff. - `blocked`: Required review input is unreadable or contradictory, so no safe verdict can be reached. -10. Order findings `CRITICAL`, `HIGH`, `MEDIUM`, then `LOW`. Return once. +10. Order findings `CRITICAL`, `HIGH`, `MEDIUM`, then `LOW`. Return exactly one + markdown block; the manager posts that whole block as one issue comment. ## Output Format -Return readable markdown and no surrounding commentary. +Return exactly one readable markdown block and no surrounding commentary. The +manager posts the whole block as one issue comment. ```markdown ### Code review round <n> — <UTC date> @@ -134,4 +137,6 @@ single review record orders all findings by severity. actionable fix. - Findings use the four Standards buckets or the Spec category. - The verdict matches the evidence and finding set. +- The output is one readable markdown review block for the manager to post as + one issue comment. - The reviewer applies no fix and asks for no approval. diff --git a/plugins/plan-lifecycle/agents/plan-reviewer.md b/plugins/plan-lifecycle/agents/plan-reviewer.md index c175100a..88729a49 100644 --- a/plugins/plan-lifecycle/agents/plan-reviewer.md +++ b/plugins/plan-lifecycle/agents/plan-reviewer.md @@ -1,6 +1,6 @@ --- name: plan-reviewer -description: Use when plan-manager needs one read-only pre-implementation review of a canonical plan against repository facts and official documentation. Not for code review, plan edits, implementation, user decisions, lifecycle changes, or direct user invocation. +description: Use when plan-manager needs a read-only pre-implementation review round for a canonical plan against repository facts and official documentation. Not for code review, plan edits, implementation, user decisions, lifecycle changes, or direct user invocation. tools: Read, Glob, Grep, WebSearch, WebFetch --- @@ -16,7 +16,8 @@ permission to change the plan. Remain read-only. Never write or edit a file. Never dispatch another agent. Never run a command that mutates repository or external state. Never ask the user a question. Return the decision to the manager, which owns every repair, -user interaction, and lifecycle change. +fresh re-review dispatch, issue-comment publication, user interaction, and +lifecycle change. </constraint> <constraint> @@ -52,11 +53,13 @@ tests, extra acceptance rows, cosmetic work, or restructuring for its own sake. - `pass`: No findings exist. - `repair`: Every finding is resolvable from repository facts. - `blocked`: At least one finding requires a user decision. -8. Return the markdown review block once. Do not repair the plan. +8. Return one markdown review block. The manager posts the whole block as one + issue comment. Do not repair the plan. ## Output Format -Return readable markdown and no surrounding commentary. +Return exactly one readable markdown block and no surrounding commentary. The +manager posts the whole block as one issue comment. For a passing review: @@ -104,5 +107,6 @@ symbol, section, or row that lets the manager reproduce the defect. - Repository evidence and official sources support every reported finding. - Findings use only the closed three-kind vocabulary. - The verdict matches the finding set and the user-decision boundary. -- The output is one readable markdown review block. +- The output is one readable markdown review block for the manager to post as + one issue comment. - The review leaves repository and external state unchanged. diff --git a/plugins/plan-lifecycle/skills/AGENTS.md b/plugins/plan-lifecycle/skills/AGENTS.md index c047b6e5..b51ccf85 100644 --- a/plugins/plan-lifecycle/skills/AGENTS.md +++ b/plugins/plan-lifecycle/skills/AGENTS.md @@ -45,9 +45,10 @@ present". `scripts/skills/no-author-scripts.mjs` enforces this. `plan-workspace` maintains the repository `plan*` labels, the `docs/PLAN.md` standard, and its `docs/AGENTS.md`/`docs/CLAUDE.md` routing pair; main-context -`plan-manager` runs decide → draft → research → one plan review → implement → -code review; internal `plan-reviewer` returns a read-only pre-implementation -verdict. Two read-only reviewer wrappers ship: the plugin wrappers at +`plan-manager` runs decide → draft → research → plan review → implement → code +review, with bounded repair and fresh re-review in both review phases; internal +`plan-reviewer` returns one read-only pre-implementation verdict block per +round. Two read-only reviewer wrappers ship: the plugin wrappers at `../agents/plan-reviewer.md` and `../agents/code-reviewer.md`, with this source repository's matching `.codex/agents/plan-reviewer.toml` and `.codex/agents/code-reviewer.toml` pair. `plugins/plan-lifecycle/agents/` @@ -59,13 +60,16 @@ deliberately carries no context-tree node (`claude plugin validate` lints every The synchronized surface is the v3 first-line marker, no-frontmatter rule, eight sections, both table headers, four open-work phases (`drafting`, `planned`, `ongoing`, `blocked`), the GitHub `state` + `stateReason` derivation truth table, -the three plan-review kinds (`goal_fit`, `research_gap`, `security_risk`), the -two review-record shapes, default issue and pull-request publication, explicit -merge confirmation, lifecycle-tool ownership, issue write preconditions, +the three plan-review kinds (`goal_fit`, `research_gap`, `security_risk`), +comment-backed review records, five-round plan and code review repair loops, +implement-start linked-branch publication, default pull-request landing, +explicit merge confirmation, lifecycle-tool ownership, issue write +preconditions, trusted latest-record selection with legacy-body fallback, archive-as-verifier landing proof, and marker-only classification: any body -without the exact v3 opening is unreadable and no parser is attempted. Legacy -workspace migration leaves old files unopened and creates fresh records only -for user-restated goals. +without the exact v3 opening is unreadable and no parser is attempted. Humans +may read frozen history, but no lifecycle command or workspace migration +operation opens or inventories it. Legacy workspace migration creates fresh +records only for user-restated goals. When any part changes, synchronize the three skills, `plan-manager/references/plan-contract.md`, diff --git a/plugins/plan-lifecycle/skills/productivity/plan-manager/SKILL.md b/plugins/plan-lifecycle/skills/productivity/plan-manager/SKILL.md index 9ab872d6..3ff8a190 100644 --- a/plugins/plan-lifecycle/skills/productivity/plan-manager/SKILL.md +++ b/plugins/plan-lifecycle/skills/productivity/plan-manager/SKILL.md @@ -4,8 +4,8 @@ description: "Use when a goal may need the six-phase plan flow: decide, draft, r user-invocable: true metadata: pattern: tool-wrapper - updated: "2026-08-21" - content_hash: "0f0f3ca545c47bbd5314402248b3047d6a3e50e3604d4e6fd0da4d7862844d4b" + updated: "2026-08-24" + content_hash: "5eb53874c837ebabd120371a2f5e8a81125eed5c1d504dc8d119c5d942daef12" --- # Plan Manager @@ -16,10 +16,11 @@ clear, reversible local change with one bounded acceptance path. <constraint> Exactly three skills own the six-phase flow. `plan-workspace` maintains the -workspace. Main-context `plan-manager` decides, drafts, researches, dispatches -one plan review, implements, dispatches code review, and archives. Internal -`plan-reviewer` returns the read-only pre-implementation verdict. Main context -owns user questions, finding disposition, edits, verification, and lifecycle. +workspace. Main-context `plan-manager` decides, drafts, researches, runs the +bounded plan-review and code-review loops, implements, lands, and archives. +Internal `plan-reviewer` returns each read-only pre-implementation verdict. +Main context owns user questions, finding disposition, edits, verification, +review-comment publication, and lifecycle. </constraint> <constraint> @@ -28,8 +29,8 @@ The plan record is a GitHub issue. Its body starts with sections; it has no frontmatter. GitHub owns title, phase, owner, timestamps, and completion. Use the v3 contract in [`references/plan-contract.md`](references/plan-contract.md) for the exact body, -table headers, record shapes, status derivation, and archive verification. Do -not restate or extend those shapes here. +table headers, review-comment records, status derivation, and archive +verification. Do not restate or extend those shapes here. </constraint> <constraint> @@ -38,8 +39,9 @@ immediately before it runs; when `ask` is unavailable the step is set `blocked` and the first line of `## Open questions` becomes `Blocked: <one-line reason>` naming the unconfirmed effect. Only a blocked plan may open `## Open questions` with `Blocked:`. -Routine plan issue publication and the landing actions in `## Landing` carry -the settled mode's authorization; they are not Steps rows and never need this +Routine plan-issue publication, review-comment publication, and the linked +branch, commit, normal-push, and landing actions described below carry the +settled mode's authorization. They are not Steps rows and never need this confirmation. </constraint> @@ -48,19 +50,14 @@ confirmation. 1. **Decide.** Phase 1 asks exactly one question with exactly three options, in this order and wording: `Plan and implement now`, `Plan only, stop at planned`, `Implement directly` — and skips the question only when the request already settles the mode. The request settles the mode only when the user explicitly asks to plan and - build, explicitly asks for a plan or proposal without execution, or - explicitly asks for a direct fix of one clear reversible local diff with one - bounded acceptance path. Never use `ask` only for permission to begin. Never - use it to restate scope that the request already gives. - - Use a plan for multi-commit or cross-repository work, a cold handoff, an - unresolved decision, a cross-subsystem or public-contract change, - security-sensitive or destructive work, or any non-`local` effect. When - `ask` is unavailable in a subagent, headless run, or `-p` run, take the direct - path only for a clear reversible local diff. Otherwise canonical planning - continues unless an issue-publication safeguard needs an answer; do not - silently substitute a tracked file. State a direct-path assumption in the - final report. A direct run creates no plan issue. + build, asks for a plan without execution, or asks for a direct fix of one + clear reversible local diff with one bounded acceptance path. Never use + `ask` only for permission to begin. Use a plan for multi-commit or + cross-repository work, a cold handoff, an unresolved decision, a + cross-subsystem or public-contract change, security-sensitive or + destructive work, or any non-`local` effect. When `ask` is unavailable, + take the direct path only for such a clear reversible local diff and state + that assumption in the final report. A direct run creates no plan issue. 2. **Draft.** Complete the preflight in [`references/github-issue-publication.md`](references/github-issue-publication.md). @@ -71,110 +68,137 @@ confirmation. outcome and one Mode line in `## Goal`, the hypothesis in `## Research`, and provisional `## Steps` and `## Acceptance` tables. Keep status `drafting`. -3. **Research.** Run `plan.mjs export <issue>`. The command writes into the - repository's sanctioned, untracked review scratch. It resolves that directory - with `git rev-parse --git-path docks-review`. A plain clone uses - `.git/docks-review/`. A linked worktree gets a worktree-private directory. - The command creates a missing scratch directory with mode `0700`. It writes the - body verbatim to `plan-<issue>.md`. It writes the body digest as one lowercase - SHA-256 line in `plan-<issue>.md.origin`. The sidecar mode is `0600`. The - command prints the absolute export path. Confirm or refute the hypothesis - against the repository. Read the target files and the nearest `AGENTS.md` or - `CLAUDE.md`. Use the language server for definitions and references before - changing an exported symbol. Verify every library, framework, runtime, or - external-API claim against current official documentation. Never rely on - memory. Follow the library and API fact rule in - `docks:skill-agent-pipeline`; cite that skill by name, never by its repository - path. +3. **Research.** Run `plan.mjs export <issue>`. It resolves the repository's + sanctioned untracked scratch with `git rev-parse --git-path docks-review` + (worktree-safe), creates it with mode `0700`, writes the body verbatim to + `plan-<issue>.md` plus a SHA-256 sidecar `plan-<issue>.md.origin` with mode + `0600`, and prints the absolute export path. Confirm or refute the + hypothesis against the repository: read the target files and the nearest + `AGENTS.md` or `CLAUDE.md`, use the language server before changing an + exported symbol, and verify every library or external-API claim against + current official documentation under the fact rule in + `docks:skill-agent-pipeline` (cite it by name, never by path). In the local file, record each finding in `## Research` with a repository path and symbol or an official URL. Name the durable fix and the temporary - fix that it replaces in one line. A patch-over is not ready when a root-cause - fix is reachable. Bind the exact `Files` cells; their union is the plan's - declared scope. Fill `## Acceptance`. Run `plan.mjs edit <issue> --file - <local-file>`. Run `plan.mjs check <issue>`. Set the plan with - `plan.mjs status <issue> planned`. Delete the temporary file and its `.origin` - sidecar. Every later body edit uses the same export, edit, check, and delete - flow. - -4. **Plan review.** Dispatch the `plan-reviewer` agent exactly once for every - canonical plan. Run `plan.mjs export <issue>` first and dispatch the reviewer with the issue number and the printed export path, because a read-only reviewer cannot fetch an issue body itself. Append the verdict and - findings verbatim to `## Review` under - `### Plan review — <UTC date>`. Fix every finding that you reproduce. For a - rejected finding, append one line that states why. - - A `blocked` verdict identifies a decision that only the user can make. Put - the decision in `## Open questions` and use `ask`. The verdict is not a - lifecycle block. There is no second review round, no permit, and no repair - ceiling. If dispatch fails, retry the dispatch because no review ran. If the - same failure signature recurs and no relevant bytes changed between attempts, - stop, append `Plan-review: blocked` naming that signature, and set the plan - blocked. When no wrapper is registered, dispatch one fresh read-only - subagent. Give it the same three-kind contract by naming the `plan-reviewer` - skill. A missing wrapper never creates another role and never skips review. - - **Plan-only runs stop here.** Deliver the reviewed issue carrying `plan:planned`. - Report the verdict and issue number. Do not enter phase 5 without a new user - instruction. A later session resumes at phase 5 by reading the issue body. - -5. **Implement.** Run `plan.mjs status <issue> ongoing`. For each row, run - `plan.mjs step <issue> <id> in-flight`, implement or delegate the task, and - run `plan.mjs step <issue> <id> done` after its proof succeeds. Follow the - effect confirmation constraint before running any non-`local` row. - - Write self-explaining code under `docks:code-clarity`. Prefer domain names to - vague names. Make invalid states unrepresentable in types. Prefer small named - functions to narration. Make errors name the operation and subject. Comment - only when syntax cannot express the reason. Run every `## Acceptance` command - and write its real output into `## Verification Results` through the edit - flow. - -6. **Code review.** Build the review diff from the complete candidate pull - request, not only the dirty worktree. Resolve and fetch the repository - default branch, then compute `<merge-base>` with - `git merge-base <default-remote-ref> HEAD`. Cover one net tracked candidate - with `git diff <merge-base> -- <changed paths>`. Add one + fix it replaces in one line. Bind the exact `Files` cells; their union is + the plan's declared scope. Fill `## Acceptance`. Run + `plan.mjs edit <issue> --file <local-file>`, then `plan.mjs check <issue>`, + then `plan.mjs status <issue> planned`, then delete the temporary file and + its `.origin` sidecar. Every later body edit uses this same flow. + +4. **Plan review.** Run at most five rounds. At the start of every round, run + `plan.mjs export <issue>` and dispatch `plan-reviewer` with the issue number + and that round's printed export path. A read-only reviewer cannot fetch the + issue body itself. It returns exactly one markdown block; post that whole + block unchanged as one issue comment. Never append a review record to + `## Review`. + + `Plan-review: pass` ends this phase. `Plan-review: repair` names findings to + reproduce. On rounds 1 through 4, fix every reproduced finding, record the + evidence for any rejected finding, and require relevant plan bytes to change. + A repair with no relevant byte change is no progress: record the blocker, set + the plan `blocked`, and stop. Re-export the repaired body and dispatch a fresh + review in the next round. If that review repeats any named finding that was + just repaired, the finding survived its fix: record it, set the plan + `blocked`, and stop. Never proceed to implementation with repaired bytes that + no review passed. + + `Plan-review: blocked` identifies a decision only the user can make. Put it + in `## Open questions` through the edit flow and use `ask`; it is not by + itself a lifecycle block. If the answer changes the plan, use a fresh + export and review in the next available round; with no answer, stop at the + pending user decision. + + A `Plan-review: repair` verdict in round 5 exhausts the ceiling: record the + blocker, set the plan `blocked`, and stop without an unreviewable + sixth-round repair. If dispatch fails, retry because no review ran; if the + same failure signature recurs with no relevant byte change, record it and + block. When no wrapper is registered, dispatch one fresh read-only subagent + and name the `plan-reviewer` skill; a missing wrapper never creates another + role or skips review. + + **Plan-only runs stop here after a pass.** Deliver the issue carrying + `plan:planned`, report the verdict and issue number, and never create a + branch or enter phase 5 without a new user instruction. A later session + resumes by reading the full record with `plan.mjs show <issue> --body`. + +5. **Implement.** Run `plan.mjs status <issue> ongoing`, then resolve the target + repository's `nameWithOwner` and `defaultBranchRef.name`. + + Before any branch checkout, and specifically before any `gh issue develop + --checkout`, require `git status --porcelain` to be empty. If it is dirty, + never stash, move, or commit the ambient work. Set the plan `blocked` and + name the dirty paths, or continue only in an authorized clean worktree. + + Check out the issue's linked branch through the repository-scoped flow in + the contract reference: pass `--repo <nameWithOwner>` to every + `gh issue develop` call, reuse a verified linked branch from `--list`, or + create one with `--base <default-branch> --checkout`, and verify the + checkout. After any failure, re-list and recover; if recovery cannot verify + and check out a linked branch, record the blocker, set the plan `blocked`, + and stop. There is no local or unlinked fallback, and plan-only runs never + create a branch. + + Branch creation, commits, and normal pushes are routine authorized work + from this point onward. For each row, run + `plan.mjs step <issue> <id> in-flight`, implement or delegate, and mark it + `done` after its proof succeeds, honoring the effect confirmation + constraint for any non-`local` row. Write self-explaining code under + `docks:code-clarity`. Run every `## Acceptance` command and write its real + output into `## Verification Results` through the edit flow. + +6. **Code review.** Run at most five rounds. At the start of every round, build + a fresh review diff from the complete candidate pull request, not only the + dirty worktree. Resolve and fetch the repository default branch, compute + `<merge-base>` with `git merge-base <default-remote-ref> HEAD`, and cover one + net tracked candidate with `git diff <merge-base> -- <changed paths>`. Add one `git diff --no-index /dev/null <path>` hunk for each untracked path. - `git status --porcelain` still names dirty paths. Name every changed path - that no Steps `Files` cell mentions in the review request. + `git status --porcelain` still names dirty paths. Name every changed path that + no Steps `Files` cell mentions in the review request. Resolve the scratch directory with `git rev-parse --git-path docks-review` - and create it with mode `0700`. Write the review input to - `<that directory>/<issue>-<round>.diff`. Resolving through git keeps linked - worktrees valid, where `.git` is a file and a literal `.git/` path does not - exist. The directory is untracked and discarded with the clone. - - After pull-request creation, record `headRefOid` and compare the changed paths - and hunks from `gh pr diff` with the reviewed net candidate. Any mismatch - invalidates the pass and blocks merge. - - Re-export the record before this dispatch too, exactly as in phase 4, so the - reviewer reads current bytes rather than a stale export. - - Dispatch `code-reviewer` with the absolute diff path, the absolute export - path, and the issue number. Append its - report to `## Review` under - `### Code review round <n> — <UTC date>`. A round that returns - `Code-review: pass` carries no unfixed `CRITICAL` or `HIGH` finding; it may - still carry advisory `MEDIUM` and `LOW` lines. Record each advisory as a - follow-up and do not change reviewed bytes after a pass; an advisory never - triggers a re-review. A round that returns `Code-review: fixes-required` - names at least one evidenced `CRITICAL` or `HIGH` defect: fix every one of - them, then dispatch exactly one repair re-review. If that repair re-review - again returns `fixes-required`, stop: append `Code-review: blocked` naming - the surviving findings, and set the plan `blocked`. - - A `pass` round ends review and starts landing. Follow `## Landing`. Run - `plan.mjs archive <issue>` only after an approved merge lands the closing - pull request. + (mode `0700`; linked-worktree safe, since `.git` may be a file) and write + each fresh input to `<that directory>/<issue>-<round>.diff`. Re-run + `plan.mjs export <issue>` in the same round so the reviewer receives the + fresh absolute diff path, fresh absolute export path, and issue number. + + Dispatch `code-reviewer`. It returns exactly one markdown block; post that + whole block unchanged as one issue comment. `Code-review: pass` ends the loop + with no unfixed `CRITICAL` or `HIGH` finding. Record any advisory `MEDIUM` or + `LOW` lines as follow-up work only after pass, do not change reviewed bytes, + and never re-review for an advisory. + + `Code-review: fixes-required` names evidenced `CRITICAL` or `HIGH` defects. + On rounds 1 through 4, fix every named defect. A repair with no relevant byte + change is no progress and terminates the loop. Build a fresh diff, re-export + the current plan, and dispatch the next round against the repaired bytes. If + that review repeats any named defect just repaired, the finding survived its + fix and terminates the loop. A non-pass verdict in round 5 exhausts the + ceiling without starting a sixth-round repair. + + `Code-review: blocked` is a technical block caused by unreadable or + contradictory review input and terminates the loop. For that verdict or any + terminal repair failure—no progress, a surviving finding, dispatch failure + with unchanged bytes, or the round-five ceiling—commit all current work, + including repair bytes, push it normally to the linked plan branch, record + the blocker, set the plan `blocked`, and stop. Perform the commit and push + before the blocked lifecycle write so implementation work is never stranded. + + After a pass, commit and push any remaining reviewed bytes, then create or + update the closing pull request under `## Landing`. Record its `headRefOid` + and compare the changed paths and hunks from `gh pr diff` with the reviewed + net candidate. Any mismatch invalidates the pass and blocks merge. Run + `plan.mjs archive <issue>` only after an approved merge lands that pull + request. ## Plan contract Read [`references/plan-contract.md`](references/plan-contract.md) before creating or changing a canonical plan. It owns the exact v3 marker, eight sections, -Steps and Acceptance table headers, review records, GitHub-field ownership, -derived status truth table, and archive verification. The record has no -frontmatter, hashes, permits, or alternate readable shape. +Steps and Acceptance table headers, review-comment record shapes and trust, +GitHub-field ownership, derived status truth table, linked-branch flow, and +archive verification. The record has no frontmatter and no hashes or permits. Keep paths repository-relative; acceptance rows run from the repository root. ## Lifecycle CLI @@ -182,34 +206,24 @@ Keep paths repository-relative; acceptance rows run from the repository root. `plan.mjs` is plugin payload, not project payload. It ships inside the installed `plan-lifecycle` plugin at `skills/productivity/plan-manager/scripts/plan.mjs`. A project never vendors, copies, or re-creates it, and an unresolvable tool means the plugin is not installed. Never report it as a file missing from the repository. Resolve it from the loaded `plan-manager` skill directory, or from the runtime plugin cache. Run it with the repository root as the working directory, because it resolves the target repository from that checkout's GitHub remote. The `plan:` namespace is reserved: `labels --extra` and `new --label` reject a -value that is `plan` or begins `plan:`, so an extra cannot plant a second status -label. Every mutating command refuses a plan owned by another login and claims an -unassigned one in the same write; read-only commands never check ownership. - -`edit` requires export provenance for every body change. It refuses a missing -sidecar, an unreadable digest, or a digest from a superseded body revision. -After validation, `edit` refreshes the digest before the remote body write. -A local sidecar failure fails closed and requires one re-export. -A phase-only status change leaves the body and sidecar valid. - -`archive` reads `closedByPullRequestsReferences` with `excludeUserLinked: true`. -A manually linked pull request never proves a landing. - -When that connection is empty, `archive` examines only the latest closure. -A commit closer supplies its `associatedPullRequests`. -Any other latest closer supplies no commit fallback proof. -An issue closed by a commit, reopened, then closed by hand has no commit proof. - -Every accepted pull request merges into the target repository's default branch. -A commit pushed straight to that branch has no associated merged pull request -and is refused. +value that is `plan` or begins `plan:`. `edit` requires export provenance for +every body change: it refuses a missing sidecar, an unreadable digest, or a +digest from a superseded body revision, and refreshes the digest before the +remote body write. A local sidecar failure fails closed and requires one +re-export; a phase-only status change leaves the body and sidecar valid. + +Review state follows the contract: a comment is trusted only when the issue +has exactly one assignee and that assignee authored the well-formed +whole-comment record; the latest trusted record per review kind wins, with a +legacy body verdict used only when no trusted record of that kind exists. +Archive proof rules live in the contract reference. | command | behaviour | stdout on success | |---|---|---| | `labels [--extra <name>]…` | `gh label create <name> --force` for `plan`, `plan:drafting`, `plan:planned`, `plan:ongoing`, and `plan:blocked`, then each `--extra` | one line per label: `label ready: <name>` | | `new --title <t> --goal <g> [--mode plan-and-implement\|plan-only] [--label <name>]…` | render the v3 marker-based body, `gh issue create --title --body-file --label plan --label plan:drafting --assignee @me` (+ extras) | `plan created: #<n> <url>` | | `claim <issue>` | resolve the acting login, `gh issue edit <n> --add-assignee @me` when unassigned; idempotent for the owner, refuses a foreign owner without writing | `plan #<n> claimed: <login>` | -| `show <issue> [--body]` | header strip on stdout; `--body` puts the record alone on stdout and the header strip on stderr | `#<n> · <status> · <title> · <url>` | +| `show <issue> [--body]` | header strip, then per-kind verdicts from latest trusted comments with legacy fallback only when none exists; `--body` puts the record alone on stdout and both metadata lines on stderr | header strip, then `reviews: plan=<pass\|repair\|blocked\|none> code=<pass\|fixes-required\|blocked\|none>` | | `export <issue>` | `export` writes the body to the worktree-aware `docks-review` directory. It writes its SHA-256 digest to `<file>.origin` with mode `0600`. | the absolute export path | | `edit <issue> --file <path>` | `edit` runs 13 checks. It requires provenance for the current body. It refreshes the digest before the remote body write. It then replaces the body. | header strip, then `changed: <k> line(s)` and the changed lines as `-old` / `+new` | | `check <issue \| --file <path>>` | 13 checks | `plan check passed: #<n>` or `plan check passed: <path>` | @@ -217,11 +231,10 @@ and is refused. | `step <issue> <step-id> <status>` | rewrite one Steps `Status` cell | `plan #<n> step <id>: <old> -> <new>` | | `list [--status <s>]` | list plan issues and derive status from phase label for open work or from `state` + `stateReason` when closed; open issues first, then closed; each group sorted by ascending number | `<status>\t#<n>\t<title>` per line | | `next` | queue-aware startable plans from `docs/PLAN-QUEUE.md` (`Plan` cell holds the issue number); falls back to every `planned` plan on a missing or malformed queue, warning on stderr | `#<n>` per line | -| `archive <issue>` | require completed closure, terminal steps, an exact `Code-review: pass` line, and a merged closing pull request into the target repository's default branch; remove any stale phase label without writing status | `plan #<n> finished (closed by <pr-url>)` | +| `archive <issue>` | require completed closure, terminal steps, a trusted latest `Code-review: pass` comment (or legacy body pass only when no trusted code record exists), and a merged closing pull request into the target repository's default branch; remove any stale phase label without writing status | `plan #<n> finished (closed by <pr-url>)` | | `retire <issue> --reason <text>` | close as not planned and remove every phase label; completion derives as `retired` from GitHub | `plan #<n> retired` | -Every command exits 0 on success and 1 on a usage or validation failure. Failure -messages keep their current wording wherever the check is unchanged. +Every command exits 0 on success and 1 on a usage or validation failure. ## Issue publication @@ -230,9 +243,11 @@ run the preflight in [`references/github-issue-publication.md`](references/github-issue-publication.md). The settled plan mode authorizes routine creation and update of the plan issue in the repository that the preflight resolved. Do not ask again for that -publication or show a repository picker that repeats a resolved fact. Ask only -for an ambiguous repository or a sensitive public disclosure. When such an ask -cannot be obtained, report the blocker and create nothing. +publication or show a repository picker that repeats a resolved fact. The same +authorization covers posting each reviewer's returned block as one unchanged +issue comment. Ask only for an ambiguous repository or a sensitive public +disclosure. When such an ask cannot be obtained, report the blocker and create +nothing. ## Reading and writing @@ -242,10 +257,9 @@ Render a plan body verbatim only when the user names that plan and asks to see i One writer owns a plan issue at a time, recorded in the issue's own GitHub assignee field. `plan.mjs new` claims ownership at creation and `plan.mjs claim <issue>` claims an existing plan. Ownership is a precondition, not advice: every mutating command refuses a plan owned by another login, writes nothing when it refuses, and claims an unassigned plan in the same write. Read-only commands never check ownership. Taking a plan from another owner is a deliberate manual GitHub action; no lifecycle command transfers ownership. -After `Code-review: pass`, the manager runs landing without another prompt: -ensure a non-default branch, commit exactly the reviewed bytes under -`docks:commit-discipline`, push normally, and create or update one pull request -that carries `Closes #<issue>` and targets the repository default branch. +The linked plan branch, commits, and normal pushes are already routine +authorized work from implement start. The closing pull request carries +`Closes #<issue>` and targets the repository default branch. Never treat an empty first checks result as success. Retry `gh pr checks --json name,bucket` at most 12 times with a 10-second delay until @@ -268,44 +282,34 @@ or diff changed, block merge. Invoke `gh pr merge` with `--match-head-commit <reviewed-head-sha>` and the repository's configured merge strategy only after the fresh `Merge now` answer. -Only the pull request that lands the completed work carries `Closes #<issue>`. -A partial pull request carries plain `Refs #<issue>`. `archive` reads the -merged result rather than causing it. A plan that never lands is retired, not -archived. +Only the pull request that lands the completed work carries `Closes #<issue>`; +a partial one carries plain `Refs #<issue>`. A plan that never lands is +retired, not archived. ## Frozen history -`docs/plans/finished/` is frozen pre-GitHub history. Never read, parse, classify, -or migrate it through this lifecycle. It is not a plan source of truth. +`docs/plans/finished/` is frozen pre-GitHub history. Humans may read it as +history, but no lifecycle command or workspace migration operation opens or +inventories it; it is not a lifecycle source of truth. ## Git boundary -Routine landing Git work is in scope: branch, commit, push, and pull request -after `Code-review: pass`, under `docks:commit-discipline`. The merge itself -needs the fresh `Merge now` answer. Force-push, history rewrite, branch -deletion, and every other destructive Git action stay out of scope without an -explicit user request. +Routine linked-branch creation, commits, and normal pushes are in scope from +implement start under `docks:commit-discipline`; the merge itself needs the +fresh `Merge now` answer. Force-push, history rewrite, branch deletion, and +every other destructive Git action stay out of scope without an explicit user +request. ## BAD / GOOD ```text -BAD: Skip repository research because the draft sounds plausible. -GOOD: Confirm the hypothesis, cite the source, and choose the durable fix. - -BAD: Run implementation after a plan-only delivery without a new instruction. -GOOD: Stop at the reviewed planned issue and report its number and verdict. - -BAD: Treat a failed repair re-review as permission to dispatch another round. -GOOD: After one repair re-review still returns `fixes-required`, record the - block and stop. - -BAD: Ask a second time which repository receives the plan issue. -GOOD: Publish to the repository the preflight resolved and report it. +BAD: Stop a failed code-review loop with repaired work left uncommitted. +GOOD: Before blocking, commit and push all current work to the linked branch. -BAD: Merge the pull request because the required checks turned green. -GOOD: Ask `Merge now` or `Leave pull request open`, then act on that answer. +BAD: Append a reviewer report to the issue body's `## Review` section. +GOOD: Post the reviewer's one markdown block unchanged as one issue comment. -BAD: Stop at a passed review and hand the user an uncommitted tree. -GOOD: Commit, push, open the closing pull request, watch the required checks, - then stop at the merge question. +BAD: Wait until a passed review to create the implementation branch. +GOOD: Verify and check out the linked plan branch when phase 5 starts; after a + pass, create or update the closing pull request and wait for the merge ask. ``` diff --git a/plugins/plan-lifecycle/skills/productivity/plan-manager/references/github-issue-publication.md b/plugins/plan-lifecycle/skills/productivity/plan-manager/references/github-issue-publication.md index c9ac6ca7..2080e8ff 100644 --- a/plugins/plan-lifecycle/skills/productivity/plan-manager/references/github-issue-publication.md +++ b/plugins/plan-lifecycle/skills/productivity/plan-manager/references/github-issue-publication.md @@ -10,11 +10,20 @@ A failed preflight creates no issue and writes nothing: `gh repo view --json nameWithOwner,visibility,defaultBranchRef` resolves the exact repository, its visibility, and its default branch. +## Contents + +- [Authorization](#authorization) +- [Review publication](#review-publication) +- [Implementation branch](#implementation-branch) +- [Landing](#landing) + ## Authorization The settled plan mode authorizes routine creation and update of the plan issue in the repository that the preflight resolved. Do not ask again for that -publication or show a repository picker that repeats a resolved fact. +publication or show a repository picker that repeats a resolved fact. The same +authorization covers posting each reviewer's returned block as one unchanged +issue comment. Two safeguards still require an in-session `ask` before the write: @@ -31,12 +40,60 @@ blocked and name the safeguard that could not be answered. Never substitute a tracked plan file and never publish a sensitive body speculatively. When no safeguard applies, the settled mode is sufficient and publication proceeds. +## Review publication + +`## Review` remains exactly `_Review records are stored in issue comments._`. +Each reviewer returns one markdown block; the manager posts that whole block +unchanged as one issue comment. A comment is trusted only when the issue has +exactly one assignee and that assignee authored the well-formed whole-comment +record. The latest trusted record per review kind wins. A legacy body verdict +for one kind is used only when no trusted comment record of that kind exists. + +Both review phases run at most five rounds. Every round reads a fresh plan +export, and every code-review round also reads a fresh complete-candidate diff. +Rounds 1 through 4 repair every reproduced or named finding and re-review the +repaired bytes. The loop stops on no relevant byte change, a finding surviving +its fix, or a `repair` or `fixes-required` verdict in round 5. A +plan-review `blocked` +routes its user-only decision through `## Open questions` and `ask`. A technical +code-review block or any terminal repair failure after implementation starts +commits and normally pushes all current work before the manager records the +blocker, sets the plan `blocked`, and stops. + +## Implementation branch + +Routine linked-branch creation, commits, and normal pushes are authorized when +the settled `plan-and-implement` run enters phase 5. A plan-only run stops before +phase 5 and never creates a branch. + +Immediately after setting the plan `ongoing`, resolve the target repository's +`nameWithOwner` and `defaultBranchRef.name`. + +Before any branch checkout, and specifically before any `gh issue develop +--checkout`, require `git status --porcelain` to be empty. If it is dirty, never +stash, move, or commit the ambient work. Set the plan `blocked` and name the +dirty paths, or continue only in an authorized clean worktree. + +Pass `--repo <nameWithOwner>` to every `gh issue develop` call. First run +`gh issue develop <issue> --repo <nameWithOwner> --list`. If it reports a +linked branch, verify that branch belongs to the resolved repository, fetch it, +and check it out. Otherwise run +`gh issue develop <issue> --repo <nameWithOwner> --base <default-branch> +--checkout`. After either path, verify that the checked-out branch is the +issue's linked branch. + +After any list, create, fetch, or checkout failure, re-run the repository-scoped +`--list`. If it reports a linked branch, verify that branch belongs to the +resolved repository, fetch it, and check it out. If recovery cannot verify and +check out a linked branch, record the blocker, set the plan `blocked`, and stop. +There is no local or unlinked fallback, and implementation never starts on an +unverified branch. + ## Landing -After `Code-review: pass`, the manager runs landing without another prompt: -ensure a non-default branch, commit exactly the reviewed bytes under -`docks:commit-discipline`, push normally, and create or update one pull request -that carries `Closes #<issue>` and targets the repository default branch. +After `Code-review: pass`, commit and push any remaining reviewed bytes, then +create or update one pull request carrying `Closes #<issue>` and targeting the +repository default branch. This landing work needs no additional prompt. Never treat an empty first checks result as success. Retry `gh pr checks --json name,bucket` at most 12 times with a 10-second delay until @@ -59,19 +116,32 @@ or diff changed, block merge. Invoke `gh pr merge` with `--match-head-commit <reviewed-head-sha>` and the repository's configured merge strategy only after the fresh `Merge now` answer. -`plan.mjs archive <issue>` stays a post-merge verifier: it verifies the merged -closing pull request and never performs the merge. +`plan.mjs archive <issue>` stays a post-merge verifier. It requires the latest +trusted code-review record to pass, with legacy body fallback only when no +trusted code-review comment exists; it verifies the merged closing pull request +and never performs the merge. ```text BAD: Ask again which repository receives the issue after the preflight resolved exactly one. -GOOD: Publish to the resolved repository and report its name with the issue - number. +GOOD: Publish the issue and unchanged review comments to that repository. + +BAD: Append review records to `## Review`. +GOOD: Keep the static pointer and post each reviewer block as one issue comment. + +BAD: Fall back to a local branch when linked-branch creation fails. +GOOD: Re-list with `--repo`, recover the linked branch, or block before coding. + +BAD: Stash, move, or commit dirty ambient work to make branch checkout succeed. +GOOD: Block with the dirty paths or use an authorized clean worktree. + +BAD: Leave repaired implementation work uncommitted when review terminates. +GOOD: Commit and push current work to the linked branch before blocking. BAD: Merge the pull request because the required checks turned green. GOOD: Ask `Merge now` or `Leave pull request open`, then act on that answer. -BAD: Leave the work uncommitted and tell the user to push it. -GOOD: Commit, push, and open the closing pull request, then stop at the merge - question. +BAD: Create the implementation branch only after review passes. +GOOD: Verify the linked branch at implement start; after pass, create or update + the closing pull request and wait for the merge question. ``` diff --git a/plugins/plan-lifecycle/skills/productivity/plan-manager/references/plan-contract.md b/plugins/plan-lifecycle/skills/productivity/plan-manager/references/plan-contract.md index f9e392e1..99741a93 100644 --- a/plugins/plan-lifecycle/skills/productivity/plan-manager/references/plan-contract.md +++ b/plugins/plan-lifecycle/skills/productivity/plan-manager/references/plan-contract.md @@ -97,7 +97,7 @@ exactly these eight `##` sections, in this order, each present once: `## Goal`, | `## Acceptance` | The Acceptance table below. | | `## Do not touch` | Paths and behaviors the change must leave alone. `None` when nothing applies. | | `## Open questions` | Decisions only the user can make. `None` when there are none. Only a blocked plan starts with `Blocked:`. | -| `## Review` | Plan-review and code-review records, appended by the manager. | +| `## Review` | Exactly `_Review records are stored in issue comments._`; legacy v3 bodies may still contain the retired body-appended records described below. | | `## Verification Results` | Observed commands and their real output, written during implementation. | The body contains no absolute machine path. A plan is a cold handoff, and a path @@ -148,12 +148,21 @@ longer available. Post-merge work belongs to a named follow-up plan. Ids are unique. Commands run from the repository root and carry no `cd <absolute path>` prefix. -## Review records — readable markdown, no hashes +## Review records — one issue comment per reviewer report -The manager appends to `## Review`. Two record shapes, exactly: +`## Review` is a static pointer, not a review log: -Before dispatch, the manager runs `plan.mjs export <issue>` and passes the +```markdown +_Review records are stored in issue comments._ +``` + +Before every dispatch, the manager runs `plan.mjs export <issue>` and passes the printed absolute path; the reviewer reads the export path the manager supplies. +For code review, the manager also supplies the fresh round diff described by the +manager skill. + +The reviewer returns exactly one markdown block. The manager posts that whole +block as one issue comment without editing it. The two exact shapes are: ```markdown ### Plan review — 2026-08-08 @@ -167,19 +176,50 @@ Code-review: fixes-required - HIGH · Security · plugins/x/y.mjs:41 — user input reaches `execSync` unquoted — pass argv array to `spawnSync` ``` -`Plan-review:` is exactly `pass`, `repair`, or `blocked`. `Code-review:` is -exactly `pass`, `fixes-required`, or `blocked`. A code-review `pass` means no -`CRITICAL` or `HIGH` finding stands unfixed; it carries only advisory `MEDIUM` -and `LOW` lines, or none. Record each advisory as a follow-up and do not change -reviewed bytes after a pass; an advisory never triggers a re-review. -`fixes-required` names at least one evidenced `CRITICAL` or `HIGH` defect and -forces exactly one repair re-review; if that re-review still returns -`fixes-required`, the manager appends `Code-review: blocked` and sets the plan -`blocked`. A `blocked` verdict has at least one finding line. +A well-formed record occupies the whole trimmed comment. It has the exact +matching heading with a UTC `YYYY-MM-DD` date, then exactly one allowed verdict +line, then only finding lines valid for that review kind. Extra prose, multiple +records, a missing heading, or an invalid verdict makes the comment ineligible. +`Plan-review:` is exactly `pass`, `repair`, or `blocked`. A plan-review `pass` +has no finding lines; `repair` and `blocked` have at least one line in the shape +`- [<kind>] <locator> — <defect> — <fix>`, where `<kind>` is exactly +`goal_fit`, `research_gap`, or `security_risk`. +`Code-review:` is exactly `pass`, `fixes-required`, or `blocked`. Each finding +uses the one-line severity, category, location, defect, and fix shape shown +above. `fixes-required` and `blocked` have at least one finding. + +A record is trusted only when the issue has exactly one assignee and the +comment's author login equals that assignee. For each review kind independently, +the latest trusted well-formed comment wins, ordered by `createdAt` with API +order as the tie-break. Foreign-authored, malformed, and superseded comments +never establish current review state. A legacy verdict in the body is consulted +for one review kind only when there is no trusted well-formed comment record of +that kind. + +A code-review `pass` means no `CRITICAL` or `HIGH` finding stands unfixed; it +carries only advisory `MEDIUM` and `LOW` lines, or none. After a pass, record +each advisory as follow-up work and do not change reviewed bytes; advisory +findings never trigger another review. `fixes-required` names at least one +evidenced `CRITICAL` or `HIGH` defect. A plan-review finding is exactly one of `goal_fit`, `research_gap`, or `security_risk`; nothing else is a finding. A sufficient plan passes. +Both review phases run at most five rounds. Each round uses a fresh plan export; +each code-review round also uses a fresh complete-candidate diff. On rounds 1 +through 4, a `repair` or `fixes-required` verdict requires every reproduced or +named finding to be fixed, followed by a fresh export or diff and a fresh +review. A repair that changes no relevant bytes is no progress. A finding +repeated in the next round survived its fix. Either condition stops the loop, +as does `repair` or `fixes-required` in round 5; there is no sixth-round repair. + +A plan-review `blocked` verdict routes its user-only decision through +`## Open questions` and `ask`; the verdict alone is not a lifecycle block. A +technical code-review `blocked` verdict stops immediately. After implementation +has started, a technical block or any terminal repair failure requires the +manager to commit and normally push all current work to the verified linked +branch before recording the blocker, setting the plan `blocked`, and stopping. + ## Lifecycle state — derived from GitHub The open-work phase enum is exactly `drafting`, `planned`, `ongoing`, and @@ -224,7 +264,9 @@ Classification follows the body bytes without guessing: A plan-issue write is a read-modify-write, and the GitHub API offers no precondition for it. Every mutating command re-reads the issue body immediately before the edit, refuses when it differs from the body it read, and re-reads -after the edit to confirm the pushed bytes. +after the edit to confirm the pushed bytes. Review-record publication is +instead one append-only issue-comment write of the reviewer's unchanged block; +it never enters the export/edit body cycle. A conflict is not permission to retry blindly. Re-read the issue, re-apply the intended change, and run `plan.mjs check <issue>` before continuing. @@ -249,17 +291,50 @@ Render a plan body verbatim only when the user names that plan and asks to see it. After a write, report the one-line header strip and the changed lines only; a write never re-renders the body. -The header strip is `#<issue> · <status> · <title> · <url>`. +The header strip is `#<issue> · <status> · <title> · <url>`. `show` prints +`reviews: plan=<pass|repair|blocked|none> code=<pass|fixes-required|blocked|none>` +on the next line. With `show --body`, the record alone goes to stdout and both +metadata lines go to stderr, header first. ## Landing -Work lands through a pull request whose body carries `Closes #<issue>` and whose -base is the target repository's default branch. +One writer owns a plan issue at a time, recorded in the issue's GitHub assignee +field. `plan.mjs new` claims ownership at creation and `plan.mjs claim <issue>` +claims an existing plan. Ownership is a precondition, not advice: every mutating +command refuses a plan owned by another login, writes nothing when it refuses, +and claims an unassigned plan in the same write. Read-only commands never check +ownership. -After `Code-review: pass`, the manager runs landing without another prompt: -ensure a non-default branch, commit exactly the reviewed bytes under -`docks:commit-discipline`, push normally, and create or update one pull request -that carries `Closes #<issue>` and targets the repository default branch. +Routine linked-branch creation, commits, and normal pushes are authorized when +the settled `plan-and-implement` run enters phase 5. They are not Steps rows and +need no separate effect confirmation. A `plan-only` run stops before phase 5 and +never creates a branch. + +Immediately after setting the plan `ongoing`, resolve the target repository's +`nameWithOwner` and `defaultBranchRef.name`. + +Before any branch checkout, and specifically before any `gh issue develop +--checkout`, require `git status --porcelain` to be empty. If it is dirty, never +stash, move, or commit the ambient work. Set the plan `blocked` and name the +dirty paths, or continue only in an authorized clean worktree. + +Pass `--repo <nameWithOwner>` to every `gh issue develop` call. First run +`gh issue develop <issue> --repo <nameWithOwner> --list`. If it reports a +linked branch, verify that branch belongs to the resolved repository, fetch it, +and check it out. Otherwise run `gh issue develop <issue> --repo +<nameWithOwner> --base <default-branch> --checkout`. After either path, verify +that the checked-out branch is the issue's linked branch. + +After any list, create, fetch, or checkout failure, re-run the repository-scoped +`--list`. If it reports a linked branch, verify that branch belongs to the +resolved repository, fetch it, and check it out. If recovery cannot verify and +check out a linked branch, record the blocker, set the plan `blocked`, and stop. +There is no local or unlinked fallback, and implementation never starts on an +unverified branch. + +After `Code-review: pass`, commit and push any remaining reviewed bytes, then +create or update one pull request carrying `Closes #<issue>` and targeting the +repository default branch. This landing work needs no additional prompt. Never treat an empty first checks result as success. Retry `gh pr checks --json name,bucket` at most 12 times with a 10-second delay until @@ -286,20 +361,15 @@ Only the pull request that lands the completed work carries `Closes #<issue>`. A partial pull request carries plain `Refs #<issue>`. `archive` verifies the merged result rather than causing it. -One writer owns a plan issue at a time, recorded in the issue's GitHub assignee -field. `plan.mjs new` claims ownership at creation and `plan.mjs claim <issue>` -claims an existing plan. Ownership is a precondition, not advice: every mutating -command refuses a plan owned by another login, writes nothing when it refuses, -and claims an unassigned plan in the same write. Read-only commands never check -ownership. - ## Archive verification `archive` is a verifier, not a status writer. It requires the issue already closed with `stateReason: COMPLETED`, every Steps row terminal (`done` or -`skipped`), and a line exactly `Code-review: pass` in `## Review`. That pass -line may carry advisory `MEDIUM` and `LOW` finding lines beneath it; only an -unfixed `CRITICAL` or `HIGH` keeps a plan from archiving. +`skipped`), and the latest trusted well-formed code-review comment to carry +`Code-review: pass`. It accepts an exact legacy body line +`Code-review: pass` only when no trusted well-formed code-review comment exists. +A pass may carry advisory `MEDIUM` and `LOW` finding lines; an unfixed +`CRITICAL` or `HIGH` keeps a plan from archiving. `archive` also requires a merged closing pull request into the target repository's default branch. It reads `closedByPullRequestsReferences` with @@ -356,6 +426,7 @@ record, and no reader consults it. ## Frozen pre-GitHub history -`docs/plans/finished/` is frozen pre-GitHub history. It is read-only historical -material, not a source of truth. No command reads, parses, classifies, lists, or -migrates it. +`docs/plans/finished/` is frozen pre-GitHub history. Humans may read it as +history, but it is not a source of truth for this lifecycle. No lifecycle +command or workspace migration operation opens, inventories, parses, +classifies, lists, or migrates it. diff --git a/plugins/plan-lifecycle/skills/productivity/plan-manager/scripts/plan.mjs b/plugins/plan-lifecycle/skills/productivity/plan-manager/scripts/plan.mjs index acb942e6..08d51a46 100755 --- a/plugins/plan-lifecycle/skills/productivity/plan-manager/scripts/plan.mjs +++ b/plugins/plan-lifecycle/skills/productivity/plan-manager/scripts/plan.mjs @@ -64,6 +64,10 @@ const ASSOCIATED_PULL_REQUESTS_QUERY = `query($owner:String!,$name:String!,$oid: } } }`; +const REVIEW_VERDICTS = { + plan: new Set(['pass', 'repair', 'blocked']), + code: new Set(['pass', 'fixes-required', 'blocked']), +}; let repository; let actingLogin; @@ -262,7 +266,7 @@ export function checkPlan(planText, planRef) { } function planTemplate(goal, mode) { - return `${V3_MARKER}\n\n## Goal\n\n${goal}\n\nMode: ${mode}\n\n## Research\n\n_Not researched yet._\n\n## Steps\n\n${STEPS_HEADER}\n${STEPS_SEPARATOR}\n\n## Acceptance\n\n${ACCEPTANCE_HEADER}\n${ACCEPTANCE_SEPARATOR}\n\n## Do not touch\n\nNone\n\n## Open questions\n\nNone\n\n## Review\n\n_No review yet._\n\n## Verification Results\n\n_Not implemented yet._\n`; + return `${V3_MARKER}\n\n## Goal\n\n${goal}\n\nMode: ${mode}\n\n## Research\n\n_Not researched yet._\n\n## Steps\n\n${STEPS_HEADER}\n${STEPS_SEPARATOR}\n\n## Acceptance\n\n${ACCEPTANCE_HEADER}\n${ACCEPTANCE_SEPARATOR}\n\n## Do not touch\n\nNone\n\n## Open questions\n\nNone\n\n## Review\n\n_Review records are stored in issue comments._\n\n## Verification Results\n\n_Not implemented yet._\n`; } function replaceBlockedReason(planText, reason) { @@ -416,6 +420,80 @@ function issueView(number, fields = ISSUE_FIELDS, repo = repository.nameWithOwne return parseJson(runGh(['issue', 'view', String(number), '--json', fields, '--repo', repo]), 'gh issue view'); } +function issueComments(number) { + const { owner, name } = repositoryCoordinates(); + const pages = parseJson( + runGh(['api', `repos/${owner}/${name}/issues/${number}/comments`, '--paginate', '--slurp']), + 'gh api issue comments', + ); + if (!Array.isArray(pages) || pages.some((page) => !Array.isArray(page))) { + fail('gh api issue comments returned malformed comments'); + } + return pages.flat(); +} + +function parseReviewComment(body) { + if (typeof body !== 'string') return undefined; + const lines = body.trim().split(/\r?\n/); + let kind; + if (/^### Plan review — \d{4}-\d{2}-\d{2}$/.test(lines[0] ?? '')) kind = 'plan'; + else if (/^### Code review round [1-9]\d* — \d{4}-\d{2}-\d{2}$/.test(lines[0] ?? '')) kind = 'code'; + else return undefined; + + const verdictPrefix = kind === 'plan' ? 'Plan-review: ' : 'Code-review: '; + const verdict = lines[1]?.startsWith(verdictPrefix) ? lines[1].slice(verdictPrefix.length) : undefined; + if (!REVIEW_VERDICTS[kind].has(verdict)) return undefined; + + const findings = lines.slice(2); + if (findings.some((line) => line.length === 0)) return undefined; + if (kind === 'plan') { + if (verdict === 'pass' && findings.length > 0) return undefined; + if (verdict !== 'pass' && findings.length === 0) return undefined; + if (findings.some((line) => !/^- \[(?:goal_fit|research_gap|security_risk)\] .+ — .+ — .+$/.test(line))) { + return undefined; + } + } else { + const parsedFindings = findings.map((line) => ( + /^- (CRITICAL|HIGH|MEDIUM|LOW) · (?:Bug|Security|Performance|Maintainability|Spec) · .+ — .+ — .+$/.exec(line) + )); + if (parsedFindings.some((finding) => finding === null)) return undefined; + const severities = parsedFindings.map((finding) => finding[1]); + if (verdict === 'pass' && severities.some((severity) => severity === 'CRITICAL' || severity === 'HIGH')) return undefined; + if (verdict === 'fixes-required' && !severities.some((severity) => severity === 'CRITICAL' || severity === 'HIGH')) return undefined; + if (verdict === 'blocked' && findings.length === 0) return undefined; + } + return { kind, verdict }; +} + +function legacyReviewVerdict(body, kind) { + const { sections } = sectionMap(body); + const prefix = kind === 'plan' ? 'Plan-review' : 'Code-review'; + const matches = [ + ...blankFencedRegions(sections.get('Review') ?? '').matchAll(new RegExp(`^${prefix}: ([^\\r\\n]+)$`, 'gm')), + ]; + const verdict = matches.at(-1)?.[1]; + return REVIEW_VERDICTS[kind].has(verdict) ? verdict : undefined; +} + +function reviewVerdicts(issue) { + const owners = (issue.assignees ?? []).map((assignee) => assignee.login).filter(Boolean); + const trustedOwner = owners.length === 1 ? owners[0] : undefined; + const verdicts = {}; + for (const comment of issueComments(issue.number)) { + const review = parseReviewComment(comment?.body); + if (trustedOwner && comment?.user?.login === trustedOwner && review) verdicts[review.kind] = review.verdict; + } + for (const kind of ['plan', 'code']) { + if (verdicts[kind] === undefined) verdicts[kind] = legacyReviewVerdict(issue.body, kind); + } + return verdicts; +} + +function reviewSummary(issue) { + const verdicts = reviewVerdicts(issue); + return `reviews: plan=${verdicts.plan ?? 'none'} code=${verdicts.code ?? 'none'}`; +} + function readPlanIssue(value, forWrite = false) { const number = parseIssueNumber(value); const issue = issueView(number); @@ -610,13 +688,13 @@ function showPlan(args) { const [value, ...flags] = args; if (!value || flags.some((flag) => flag !== '--body') || flags.length > 1) fail('show requires an issue and optional --body'); const { issue, record } = readPlanIssue(value); - const header = headerStrip(issue, record.status); + const metadata = `${headerStrip(issue, record.status)}\n${reviewSummary(issue)}`; if (flags[0] === '--body') { - console.error(header); + console.error(metadata); process.stdout.write(issue.body); return; } - console.log(header); + console.log(metadata); } function reviewDirectory() { @@ -895,7 +973,7 @@ function archivePlan(args, retired = false) { const steps = parseRows(sections.get('Steps') ?? '', STEPS_HEADER, STEPS_SEPARATOR, 8).rows; const unfinished = steps.length === 0 ? { cells: ['', '(missing)'] } : steps.find(({ cells }) => !new Set(['done', 'skipped']).has(unquoteCode(cells[6]))); if (unfinished) fail(`archive refused: non-terminal step ${unfinished.cells[1]}`); - if (!/^Code-review: pass$/m.test(sections.get('Review') ?? '')) fail('archive requires Code-review: pass'); + if (reviewVerdicts(issue).code !== 'pass') fail('archive requires Code-review: pass'); const { closing, closingCommitOid, defaultBranch } = archivePullRequestReferences(issue.number); const landedInto = (references, branch) => references.find((reference) => ( diff --git a/plugins/plan-lifecycle/skills/productivity/plan-reviewer/SKILL.md b/plugins/plan-lifecycle/skills/productivity/plan-reviewer/SKILL.md index 479435e2..936f0b70 100644 --- a/plugins/plan-lifecycle/skills/productivity/plan-reviewer/SKILL.md +++ b/plugins/plan-lifecycle/skills/productivity/plan-reviewer/SKILL.md @@ -1,19 +1,22 @@ --- name: plan-reviewer -description: "Use when plan-manager needs one read-only pre-implementation review of a canonical plan against repository facts, official documentation, goal fit, research gaps, and security risk. Not for direct user invocation, plan edits, code review, implementation, lifecycle changes, user questions, or external actions." +description: "Use when plan-manager needs a read-only pre-implementation review round for a canonical plan against repository facts, official documentation, goal fit, research gaps, and security risk. Not for direct user invocation, plan edits, code review, implementation, lifecycle changes, user questions, or external actions." user-invocable: false metadata: pattern: tool-wrapper - updated: "2026-08-21" - content_hash: "39561aa764f1216fff9ddfcea0ff94dab8e53459de29fe4e4419588b8bb6fb92" + updated: "2026-08-24" + content_hash: "a5ed7496458e7b22b9ca9c3e18d4deb85dd7b44a033a77abb9f1bde3031b6916" --- # Plan Reviewer -Review one canonical plan before implementation. The input is the plan issue -number. Read the plan body from the export path the manager supplies; it is an absolute path to an untracked review-scratch file. Never fetch the issue yourself and never run a command. Then verify its claims against repository files, symbols, tests, and current official -documentation. Main-context `plan-manager` owns edits, finding disposition, -user questions, implementation, and lifecycle. +Review the canonical plan for the current pre-implementation round. The input is +the plan issue number and the export path the manager supplies. Read the plan +body from that absolute path to an untracked review-scratch file. Never fetch the +issue yourself and never run a command. Then verify its claims against +repository files, symbols, tests, and current official documentation. +Main-context `plan-manager` owns edits, finding disposition, fresh re-review +dispatch after repairs, user questions, implementation, and lifecycle. <constraint> Stay read-only. Do not write or edit files. Do not dispatch agents. Do not run a @@ -57,12 +60,14 @@ Coalesce duplicate symptoms into one root-cause finding. ## Output contract -Return the readable `Plan-review:` block defined by the v3 contract in the -`plan-manager` skill's `references/plan-contract.md`. Do not return JSON. A pass +Return exactly one readable `Plan-review:` markdown block defined by the v3 +contract in the `plan-manager` skill's `references/plan-contract.md`. The +manager posts that whole block as one issue comment. Do not return JSON. A pass has no finding lines. A repair or blocked verdict has at least one finding line. Use a precise plan locator, defect, and actionable fix. ```markdown +### Plan review — <UTC date> Plan-review: repair - [goal_fit] `## Steps` row 4 — the plan removes the validator without adding its replacement — add the replacement before the removal - [security_risk] plugins/x/y.mjs:41 — untrusted input reaches a shell command — pass an argv array without a shell @@ -87,4 +92,5 @@ GOOD: Verify the claim against current official documentation. - Ground every finding in the plan, repository, or official documentation. - Use only one of the three finding kinds. - Keep cosmetic preferences out of the report. -- Return exactly one readable verdict block and no JSON. +- Return exactly one readable markdown verdict block for the manager to post as + one issue comment; return no JSON. diff --git a/plugins/plan-lifecycle/skills/productivity/plan-workspace/SKILL.md b/plugins/plan-lifecycle/skills/productivity/plan-workspace/SKILL.md index 04c0ac9b..bd6a5ce8 100644 --- a/plugins/plan-lifecycle/skills/productivity/plan-workspace/SKILL.md +++ b/plugins/plan-lifecycle/skills/productivity/plan-workspace/SKILL.md @@ -4,8 +4,8 @@ description: "Use when bootstrapping, migrating, auditing, or explicitly refresh user-invocable: true metadata: pattern: tool-wrapper - updated: "2026-08-21" - content_hash: "44018dca60d44a64ebc6b3473caaae9f9d766c9aead7d1f4ee4eddd1430cbf4d" + updated: "2026-08-24" + content_hash: "13107186ed4e745e1dcbc77ec7d1ec889783566a63ab4c07f4434e9a23eb5945" --- # Plans Workspace @@ -28,8 +28,9 @@ refresh. Migration preserves every legacy plan file by leaving it exactly where it is, unmodified. Never open, parse, hash, copy, upload, delete, or rewrite one. Only goals the user explicitly restates as a title and goal enter the lifecycle, -through fresh v3 issues. `docs/plans/finished/` is frozen pre-GitHub history: -never read, inventory, parse, migrate, delete, or rewrite it. +through fresh v3 issues. `docs/plans/finished/` is frozen pre-GitHub history. +Humans may read it as history, but no lifecycle command or workspace migration +operation opens, inventories, parses, classifies, lists, or migrates it. </constraint> ## Ownership and operations @@ -38,23 +39,20 @@ never read, inventory, parse, migrate, delete, or rewrite it. |---|---| | Bootstrap, migrate, audit, or explicit workspace refresh | `plan-workspace` | | Decide the mode; draft, research, review, implement, verify, and archive | main-context `plan-manager` | -| Return one read-only pre-implementation plan verdict | internal `plan-reviewer` | +| Return one read-only pre-implementation plan verdict block per round | internal `plan-reviewer` | Two read-only reviewer roles exist: `plan-reviewer` and `code-reviewer`. Main -context invokes `plan-manager` directly. Do not seed manager, workspace, -creator, repairer, or improver wrappers. +context invokes `plan-manager` directly; seed no other wrapper. ## Lifecycle tool `plan.mjs` is plugin payload, not project payload. It ships inside the installed `plan-lifecycle` plugin at `skills/productivity/plan-manager/scripts/plan.mjs`. A project never vendors, copies, or re-creates it, and an unresolvable tool means the plugin is not installed. Never report it as a file missing from the repository. -Resolve it from the loaded `plan-manager` skill directory. When that directory -is unknown, search the runtime plugin cache and take the highest version. Run it +Resolve it from the loaded `plan-manager` skill directory; when that is +unknown, search the runtime plugin cache and take the highest version. Run it from the repository root so it resolves the target GitHub repository from that -checkout's remote. The tool is not a workspace marker. - -Every bootstrap, migration, refresh, and audit report states its resolved -absolute path, or states that the `plan-lifecycle` plugin is not installed. +checkout's remote. The tool is not a workspace marker. Every operation report +states its resolved absolute path, or that the plugin is not installed. ## Resolve operation and repository @@ -75,8 +73,8 @@ naming `OWNER/REPO` and the effect. A missing confirmation makes the step Inspect `docs/PLAN.md`, `docs/AGENTS.md`, `docs/CLAUDE.md`, the root `## Plans` section, `docs/PLAN-QUEUE.md`, the repository's `plan*` labels, and the two Codex reviewer wrappers. Detect a recognizable legacy markdown workspace only from -generated scaffolding and paths; never open a legacy plan file or inspect -anything under `docs/plans/finished/`. +generated scaffolding and paths outside `docs/plans/finished/`; never open a +legacy plan file or inventory frozen history. Apply the table in order; `LEGACY_MARKDOWN` takes precedence over `GREENFIELD` while its old generated scaffolding is recognizable. @@ -103,9 +101,9 @@ Current markers are exactly: The label set is created idempotently with `gh label create --force`: `plan`, `plan:drafting`, `plan:planned`, `plan:ongoing`, and `plan:blocked`. An open plan carries exactly one phase label. Closed-plan completion derives from GitHub `state` and `stateReason`; phase labels describe open work only. A missing marker makes a recognizable issue-backed workspace `STALE`. Audit -reports malformed queue bytes but does not repair them. Existing wrapper files -and unrelated `docs/AGENTS.md` content are project-owned and never overwritten. -Report unexpected plan-prefixed wrappers; do not delete them. +reports malformed queue bytes without repairing them, never overwrites +project-owned wrapper files or unrelated `docs/AGENTS.md` content, and reports +unexpected plan-prefixed wrappers without deleting them. ## Classification report @@ -117,12 +115,11 @@ Before mutation, report every target, proposed action, and observed reason: | OWNER/REPO labels | CREATE/UPDATE | GREENFIELD bootstrap after confirmation | | docs/PLAN.md | OFFER REFRESH | recognizable STALE standard | | docs/plans/active/example.md | PRESERVE | LEGACY_MARKDOWN history; bytes untouched | -| docs/plans/finished/ | FREEZE | pre-GitHub history; contents not read | +| docs/plans/finished/ | FREEZE | pre-GitHub history; directory contents not inventoried | | .codex/agents/code-reviewer.toml | CREATE | missing wrapper during bootstrap | ``` -For audit, this report plus observed marker evidence is final. Do not continue -into an apply operation. +For audit, this report plus observed marker evidence is final; never continue into an apply operation. ## Bootstrap @@ -152,33 +149,33 @@ files as inert history and create current records only for goals the user still wants: 1. Identify the recognizable generated standard, routing, shim, root section, - queue, and `docs/plans/` support paths without opening a legacy plan file. - Customized or ambiguous scaffolding stops migration. -2. Report every legacy plan path as `PRESERVE`. Do not read, hash, copy, upload, - delete, rewrite, or derive metadata from any of those files. + queue, and `docs/plans/` support paths outside `docs/plans/finished/` without + opening a legacy plan file. Customized or ambiguous scaffolding stops + migration. +2. Report each discovered legacy plan path outside frozen history as + `PRESERVE`; derive nothing from those files and never inventory + `docs/plans/finished/`. 3. Ask the user to name each goal that should remain live, supplying its exact title and goal. A goal the user does not restate stays only in inert history. 4. Resolve and name `OWNER/REPO`. After the repository-metadata confirmation, create the reserved plan labels with `plan.mjs labels`. 5. After the issue-write confirmation, run `plan.mjs new --title <user-title> --goal <user-goal>` once per restated - goal. Every new record starts at `plan:drafting` with the normal v3 body and - the template Research section. Never derive its title, goal, phase, or - body from a legacy file. + goal. Every new record starts at `plan:drafting` with the normal v3 body; + never derive its title, goal, phase, or body from a legacy file. 6. Read each new issue back, run `plan.mjs check <issue>`, and verify its first line marker and `plan:drafting` label. Record the returned issue number and URL for the user. 7. Replace only the recognizable generated standard, routing, shim, and root surfaces with the current issue-backed versions; seed only missing reviewer - wrappers. Remove only recognizable generated `docs/plans/AGENTS.md`, - `docs/plans/CLAUDE.md`, and `docs/plans/.gitignore` scaffolding that advertised - the tracked-plan contract. Leave all plan files and their directories in - place. + wrappers. Remove only recognizable generated `docs/plans/` scaffolding that + advertised the tracked-plan contract. Leave all plan files and their + directories in place. 8. Create a current `docs/PLAN-QUEUE.md` only when requested, using the fresh issue numbers. Never read or translate a legacy queue. -The issue number is the plan identity. Legacy files remain inert history, no -command reads them, and their paths never become lifecycle identity. +The issue number is the plan identity. Legacy files remain inert history; no +lifecycle command reads them, and their paths never become lifecycle identity. ## Explicit refresh @@ -210,11 +207,12 @@ The plan record is a GitHub issue. Its body starts with sections; it has no frontmatter. GitHub owns title, open-work phase, owner, timestamps, and completion, and no plan markdown is tracked in the repository. Exactly three skills own the workflow: `plan-workspace` maintains the workspace; -main-context `plan-manager` runs six phases — decide, draft, research, one plan -review, implement, code review — and archives; internal `plan-reviewer` returns -a readable pre-implementation verdict. Two read-only reviewer wrappers ship, -`plan-reviewer` and `code-reviewer`, and nothing else in the lifecycle has a -wrapper. +main-context `plan-manager` runs six phases — decide, draft, research, plan +review, implement, code review — with bounded repair and fresh re-review in both +review phases, then archives; internal `plan-reviewer` returns one readable +pre-implementation verdict block per round. Two read-only reviewer wrappers +ship, `plan-reviewer` and `code-reviewer`, and nothing else in the lifecycle has +a wrapper. </constraint> After the marker and blank line, the record carries exactly `## Goal`, @@ -223,17 +221,36 @@ After the marker and blank line, the record carries exactly `## Goal`, and once each. `## Goal` carries exactly one mode line. Open-work phase is one of `drafting`, `planned`, `ongoing`, or `blocked` in a `plan:<phase>` label; a blocked plan starts `## Open questions` with `Blocked: <one-line reason>`. -Closed completion derives from GitHub `state` and `stateReason`. The record -carries no hash, permit, run identity, lock, or bundle, and the `plan.mjs` -shipped inside the installed `plan-lifecycle` plugin is the only lifecycle tool. -An `export` writes the sha256 of the body it copied beside the copy so a stale -copy cannot revert the record; that digest detects staleness and authorizes -nothing. Routine plan issue publication and post-review landing through a -branch, commit, normal push, and closing pull request carry the settled mode's -authorization and need no repeated prompt. After the checks policy passes, the -manager asks immediately before merge. Without a fresh `Merge now` answer, it -leaves the pull request and issue open. `plan.mjs archive` verifies the merged -closing pull request after landing. +Closed completion derives from GitHub `state` and `stateReason`. `## Review` +contains exactly `_Review records are stored in issue comments._`. Each reviewer +returns one markdown block, and the manager posts that whole block as one issue +comment. The latest trusted well-formed record per review kind wins; its author +must equal the plan's sole assignee. A legacy body verdict is consulted only +when no trusted comment record exists for that kind. Both review phases use +fresh inputs and run at most five rounds, stopping on pass, no progress, a +finding surviving its fix, or `repair` or `fixes-required` in round five. A +plan-review `blocked` verdict always routes its user-only decision through +`## Open questions` and `ask`. + +The record carries no hash, permit, run identity, lock, or bundle, and the +`plan.mjs` shipped inside the installed `plan-lifecycle` plugin is the only +lifecycle tool. An `export` writes the sha256 of the body it copied beside the +copy so a stale copy cannot revert the record; that digest detects staleness and +authorizes nothing. Routine plan issue publication, implement-start linked +branch creation, commits, normal pushes, and the closing pull request carry the +settled mode's authorization and need no repeated prompt. Before any branch +checkout, including `gh issue develop --checkout`, require +`git status --porcelain` to be empty. If it is dirty, never stash, move, or +commit ambient work; set the plan `blocked` and name the dirty paths, or use an +authorized clean worktree. +Immediately after setting the plan `ongoing`, every `gh issue develop` call uses +`--repo`; the manager reuses a linked branch or creates one with +`--base <default> --checkout`, then re-lists and recovers after failure. +Implementation stops when no linked branch can be verified; there is no local +fallback. After the checks policy passes, the manager asks immediately before +merge. Without a fresh `Merge now` answer, it leaves the pull request and issue +open. `plan.mjs archive` verifies the latest trusted code-review result and +merged closing pull request after landing. Every Steps row carries an `Effect` of exactly `local|probe|production_access|publish|push|release|deploy`. A step whose @@ -245,8 +262,9 @@ actions are outside the Steps table. Render a plan body verbatim only when the user names that plan and asks to see it. After a write, report the one-line header strip and the changed lines only; a write never re-renders the body. -`docs/plans/finished/` is frozen pre-GitHub history: read it as history, never as -a source of truth, and never parse or migrate it. The complete contract lives in +`docs/plans/finished/` is frozen pre-GitHub history. Humans may read it as +history, but it is not a source of truth. No lifecycle command or workspace +migration operation opens or inventories it. The complete contract lives in `docs/PLAN.md`; `docs/AGENTS.md` routes to it and `docs/CLAUDE.md` contains only `@AGENTS.md`. ``` @@ -258,12 +276,12 @@ After migration issue creation and before changing generated scaffolding: - **Legacy-file preservation:** no command opened, parsed, hashed, copied, uploaded, deleted, or rewrote a legacy plan file; every file remains at its original path. -- **Goal provenance:** every created issue has one exact user-supplied title and - goal; no field came from a legacy file. -- **Fresh-record check:** every created issue starts with the v3 marker, carries - `plan:drafting`, and passes `plan.mjs check <issue>`. -- **Frozen history:** no command listed, read, hashed, parsed, moved, or rewrote - anything under `docs/plans/finished/`. +- **Goal provenance:** every created issue has one exact user-supplied title + and goal; no field came from a legacy file. +- **Fresh-record check:** every created issue starts with the v3 marker, + carries `plan:drafting`, and passes `plan.mjs check <issue>`. +- **Frozen history:** nothing under `docs/plans/finished/` was inventoried, + listed, read, hashed, parsed, moved, or rewritten. After bootstrap, migration, or refresh, verify the repository label set, byte-identical `docs/PLAN.md`, routing node, exact Claude shim, exact root @@ -280,11 +298,8 @@ GOOD: Audit reports drift; only an explicit refresh changes current markers. BAD: Parse or upload a legacy plan file, then delete or rewrite the source. GOOD: Leave every old file unopened and create only user-restated goals through plan.mjs new. -BAD: Hash docs/plans/finished/ to prove that frozen history was preserved. -GOOD: Never read frozen history; preserve the directory by leaving it untouched. - -BAD: Overwrite an existing reviewer wrapper during bootstrap. -GOOD: Seed both reviewer wrappers only when their files are missing. +BAD: Inventory or hash docs/plans/finished/ to prove that frozen history was preserved. +GOOD: Leave frozen history untouched and unlisted; humans may read it separately as history. BAD: Report plan.mjs as a missing project file and recreate it locally. GOOD: Resolve the installed plugin tool and report its absolute path. diff --git a/plugins/plan-lifecycle/skills/productivity/plan-workspace/references/codex-agent-templates.md b/plugins/plan-lifecycle/skills/productivity/plan-workspace/references/codex-agent-templates.md index 5fecc8bc..1fcf95ff 100644 --- a/plugins/plan-lifecycle/skills/productivity/plan-workspace/references/codex-agent-templates.md +++ b/plugins/plan-lifecycle/skills/productivity/plan-workspace/references/codex-agent-templates.md @@ -11,20 +11,23 @@ Keep both templates free of a `model` key; model selection belongs to the consum ```toml name = "plan-reviewer" -description = "Use when plan-manager needs one read-only pre-implementation review of a canonical plan against repository facts and official documentation. Not for code review, plan edits, implementation, user decisions, lifecycle changes, or direct user invocation." +description = "Use when plan-manager needs a read-only pre-implementation review round for a canonical plan against repository facts and official documentation. Not for code review, plan edits, implementation, user decisions, lifecycle changes, or direct user invocation." sandbox_mode = "read-only" developer_instructions = """ # Plan Reviewer Load the project-local bundled `plan-reviewer` skill when present; otherwise load the installed runtime skill. -Acknowledge the supplied plan issue number before analysis. +Acknowledge the supplied plan issue number and export path before analysis. Read +the plan body from the export path the manager supplies; never fetch the issue. A plan-review finding is exactly one of `goal_fit`, `research_gap`, or `security_risk`; nothing else is a finding. A sufficient plan passes. Remain read-only. Never write, dispatch an agent, run a mutating command, or ask -the user. Return one readable `Plan-review:` markdown block to the manager. The -canonical skill owns the review workflow and output contract. +the user. Return exactly one readable `Plan-review:` markdown block to the +manager, which posts that whole block as one issue comment and owns repairs and +fresh re-review dispatch. The canonical skill owns the review workflow and +output contract. """ ``` @@ -39,7 +42,9 @@ developer_instructions = """ Load the project-local bundled `code-review` and `code-clarity` skills when present; otherwise load the installed runtime skills. -Acknowledge the supplied diff path and plan issue number before analysis. +Acknowledge the supplied diff path, plan issue number, and export path before +analysis. Read the plan body from the export path the manager supplies; never +fetch the issue. Run two separate analysis axes. Do not let a pass on one axis hide a failure on the other. @@ -79,13 +84,24 @@ Select one verdict: follow-ups and does not change reviewed bytes after the pass; they never trigger a re-review. - `fixes-required`: At least one evidenced `CRITICAL` or `HIGH` defect. The - manager fixes it and dispatches exactly one repair re-review. + manager fixes every named defect and dispatches a fresh re-review on a fresh + diff. - `blocked`: Required review input is unreadable or contradictory, so no safe verdict can be reached. Remain read-only. Never apply a fix and never ask for approval to apply one. -Return one readable `Code-review:` markdown block to the manager. These inline -Standards buckets, severity caps, and Spec axis keep this wrapper complete when -the runtime skills are unavailable. +Return exactly one readable `Code-review:` markdown block to the manager, which +posts that whole block unchanged as one issue comment and owns fixes and fresh +re-review dispatch. The block must be the parser-compatible comment record, and +this exact shape overrides any report layout from a loaded `code-review` skill: + + ### Code review round <n> — <UTC YYYY-MM-DD> + Code-review: <pass|fixes-required|blocked> + - <CRITICAL|HIGH|MEDIUM|LOW> · <Bug|Security|Performance|Maintainability|Spec> · <locator> — <defect> — <fix> + +Use the round number the manager supplies, one finding per line, and no prose +outside the block. These inline Standards buckets, severity caps, Spec axis, +and record shape keep this wrapper complete when the runtime skills are +unavailable. """ ``` diff --git a/plugins/plan-lifecycle/skills/productivity/plan-workspace/references/plan-md-template.md b/plugins/plan-lifecycle/skills/productivity/plan-workspace/references/plan-md-template.md index 34d09a78..26c4d43c 100644 --- a/plugins/plan-lifecycle/skills/productivity/plan-workspace/references/plan-md-template.md +++ b/plugins/plan-lifecycle/skills/productivity/plan-workspace/references/plan-md-template.md @@ -5,8 +5,9 @@ Copy the fenced block verbatim to `docs/PLAN.md`; write `docs/AGENTS.md` as its # PLAN.md — plan record standard The plan record is a GitHub issue. Its body carries the v3 byte contract and the -human-authored plan, while GitHub fields carry the machine state GitHub already -owns. No plan markdown is tracked in the repository. +human-authored plan, review records live in issue comments, and GitHub fields +carry the machine state GitHub already owns. No plan markdown is tracked in the +repository. Use direct implementation for one clear, reversible, low-risk local diff with one bounded acceptance path; it creates no plan issue, reviewer, or automatic @@ -28,17 +29,19 @@ The two read-only wrappers are `plan-reviewer` and `code-reviewer`. Main context ## Where the record lives ```text -GitHub issue #<number> the plan record: title, body, labels, assignee, state +GitHub issue #<number> the plan body, labels, assignee, and state +GitHub issue comments trusted plan-review and code-review records GitHub timestamps the record's creation and last-update times docs/PLAN.md this standard docs/PLAN-QUEUE.md optional discovery and priority view -docs/plans/finished/ frozen pre-GitHub history, read-only, never parsed +docs/plans/finished/ frozen pre-GitHub history, not lifecycle input ``` The issue number is the plan identity. There is no slug, no plan path, and no tracked plan file. `docs/plans/finished/` holds records written before the -lifecycle moved to issues; it is history, never a source of truth, and no -command reads it. +lifecycle moved to issues. Humans may read it as history, but it is never a +source of truth. No lifecycle command or workspace migration operation opens, +inventories, parses, classifies, lists, or migrates it. This backend is a deliberate trade. An issue body has no reviewable diff, no `git blame`, no CODEOWNERS, and no presubmit validation, which is why large @@ -81,6 +84,8 @@ retired and never live fields in a v3 record: `plan_contract`, `title`, `goal`, `status`, `created`, `updated`, `assignee`, and `blocked_reason`. `## Goal` still contains exactly one `Mode: plan-and-implement` or `Mode: plan-only` line. Mode stays in the body because GitHub has no field that owns this plan-specific choice. Once an open plan leaves `drafting`, `## Research` must no longer carry the template placeholder `_Not researched yet._`. +`## Review` contains exactly `_Review records are stored in issue comments._`; +review reports are not appended to the body. A blocked plan carries its reason as the first content line of `## Open questions`. Spell it `Blocked: <one-line text>`. Only a blocked plan may open that section @@ -191,7 +196,7 @@ resolves the target repository from that checkout's GitHub remote. | `plan.mjs labels [--extra <name>]…` | Create or update the exact five-label lifecycle set with `gh label create --force`, plus any extra topic labels named on the command line. | | `plan.mjs new --title <t> --goal <g> [--mode plan-and-implement\|plan-only] [--label <name>]…` | Create a v3 issue whose body starts with the marker, with labels `plan` and `plan:drafting`, and assign the creating login. | | `plan.mjs claim <issue>` | Take single-writer ownership of an existing plan: assign the acting login, stay idempotent when it already owns the plan, and refuse when another login does. | -| `plan.mjs show <issue> [--body]` | Print the one-line header strip. `--body` prints the record to stdout with nothing else, sending the header strip to stderr, and is the only way to obtain the record. | +| `plan.mjs show <issue> [--body]` | Print the header strip, then `reviews: plan=<pass\|repair\|blocked\|none> code=<pass\|fixes-required\|blocked\|none>`. With `--body`, print only the body to stdout and send both metadata lines to stderr, header first. | | `plan.mjs export <issue>` | Write the issue body verbatim to `plan-<issue>.md` inside the scratch directory `git rev-parse --git-path docks-review` resolves, creating it mode 0700 when missing, and print the absolute path. | | `plan.mjs edit <issue> --file <path>` | Validate the file as the plan record, refuse on any failed check, replace the issue body, and print the header strip and the changed lines. | | `plan.mjs check <issue \| --file <path>>` | Validate a v3 record and print the pass result. | @@ -199,7 +204,7 @@ resolves the target repository from that checkout's GitHub remote. | `plan.mjs step <issue> <step-id> <status>` | Rewrite one Steps `Status` cell after checking the plan state and dependencies. | | `plan.mjs list [--status <s>]` | Print `<status>\t#<issue>\t<title>` for every issue labelled `plan`, deriving `unlabelled`, `finished`, `retired`, and `duplicate` rather than reading them from the body. | | `plan.mjs next` | Print startable open plans, using the queue when it is present and valid. | -| `plan.mjs archive <issue>` | Verify terminal steps, passed code review, completed closure, and an eligible merged closing pull request; strip stale phase labels and write no status. | +| `plan.mjs archive <issue>` | Verify terminal steps, the latest trusted code-review result (with legacy body fallback only when no trusted comment record exists), completed closure, and an eligible merged closing pull request; strip stale phase labels and write no status. | | `plan.mjs retire <issue> --reason <text>` | Close the issue as not planned and strip all phase labels; completion derives as `retired`. | Legal step transitions are `planned → in-flight | done | blocked | skipped`, `in-flight → done | blocked | skipped`, and `blocked → in-flight | done | skipped`. @@ -207,12 +212,14 @@ Legal step transitions are `planned → in-flight | done | blocked | skipped`, ` ## Archive verification `plan.mjs archive` is a verifier, not a writer of lifecycle state. It requires -all Steps rows to be terminal (`done` or `skipped`), a line matching exactly -`Code-review: pass` in `## Review`, and an issue already closed as completed by -an eligible merged pull request. It writes no status. On success it removes any -stale phase label and prints `plan #<n> finished (closed by <url>)`. The pass -line may carry advisory `MEDIUM` and `LOW` finding lines beneath it; only an -unfixed `CRITICAL` or `HIGH` keeps a plan from archiving. +all Steps rows to be terminal (`done` or `skipped`), the latest trusted +well-formed code-review comment to carry `Code-review: pass`, and an issue +already closed as completed by an eligible merged pull request. It accepts an +exact legacy body line `Code-review: pass` only when no trusted well-formed +code-review comment exists. It writes no status. On success it removes any +stale phase label and prints `plan #<n> finished (closed by <url>)`. A pass may +carry advisory `MEDIUM` and `LOW` finding lines; an unfixed `CRITICAL` or `HIGH` +keeps a plan from archiving. The verifier reads the issue's `closedByPullRequestsReferences` with `excludeUserLinked: true`. It accepts only keyword-linked merged pull requests. @@ -279,42 +286,85 @@ file for a body write; a reviewer never edits it. Every dispatch re-exports first, so a reviewer always reads the current record rather than a half-staged edit. -The header strip is one line: `#<issue> · <status> · <title> · <url>`. +The header strip is `#<issue> · <status> · <title> · <url>`. `show` prints +`reviews: plan=<pass|repair|blocked|none> code=<pass|fixes-required|blocked|none>` +on the next line. With `show --body`, the record alone goes to stdout and both +metadata lines go to stderr, header first. -## Review records +## Review records — one issue comment per reviewer report -Append review records to `## Review` in these readable shapes: +`## Review` is a static pointer, not a review log: + +```markdown +_Review records are stored in issue comments._ +``` + +Before every dispatch, the manager runs `plan.mjs export <issue>` and passes the +printed absolute path; the reviewer reads the export path the manager supplies. +For code review, the manager also supplies a fresh complete-candidate diff. + +The reviewer returns exactly one markdown block. The manager posts that whole +block as one issue comment without editing it. The two exact shapes are: ```markdown ### Plan review — <date> Plan-review: pass|repair|blocked - [goal_fit] `## Steps` row 4 — the step removes the validator without replacing it — add the replacement before removal +``` +```markdown ### Code review round <n> — <date> Code-review: pass|fixes-required|blocked - HIGH · Security · plugins/x/y.mjs:41 — user input reaches a shell command unquoted — pass an argument array ``` +A well-formed record occupies the whole comment. It has the matching heading, +then exactly one verdict line, then zero or more nonblank finding lines. Extra +prose, multiple records, a missing heading, or an invalid verdict makes the +comment ineligible. `Plan-review:` is exactly `pass`, `repair`, or `blocked`. +`Code-review:` is exactly `pass`, `fixes-required`, or `blocked`. + +A record is trusted only when the issue has exactly one assignee and the +comment's author login equals that assignee. For each review kind independently, +the latest trusted well-formed comment wins, ordered by `createdAt` with API +order as the tie-break. Foreign-authored, malformed, and superseded comments +never establish current review state. A legacy verdict in the body is consulted +for one review kind only when there is no trusted well-formed comment record of +that kind. + A code-review `pass` means no `CRITICAL` or `HIGH` finding stands unfixed; it -carries only advisory `MEDIUM` and `LOW` lines, or none. Record each advisory as -a follow-up and do not change reviewed bytes after a pass; an advisory never -triggers a re-review. -`fixes-required` names at least one evidenced `CRITICAL` or `HIGH` defect and -forces exactly one repair re-review; -if that re-review still returns `fixes-required`, the manager appends -`Code-review: blocked` and sets the plan `blocked`. A `blocked` verdict has at -least one finding line. A plan-review finding is exactly one of `goal_fit`, -`research_gap`, or `security_risk`; nothing else is a finding. A sufficient plan -passes. +carries only advisory `MEDIUM` and `LOW` lines, or none. After a pass, record +each advisory as follow-up work and do not change reviewed bytes; advisory +findings never trigger another review. `fixes-required` names at least one +evidenced `CRITICAL` or `HIGH` defect. A `blocked` verdict has at least one +finding line. + +A plan-review finding is exactly one of `goal_fit`, `research_gap`, or +`security_risk`; nothing else is a finding. A sufficient plan passes. + +Both review phases run at most five rounds. Each round uses a fresh plan export; +each code-review round also uses a fresh complete-candidate diff. On rounds 1 +through 4, a `repair` or `fixes-required` verdict requires every reproduced or +named finding to be fixed, followed by a fresh export or diff and a fresh +review. A repair that changes no relevant bytes is no progress. A finding +repeated in the next round survived its fix. Either condition stops the loop, +as does `repair` or `fixes-required` in round 5; there is no sixth-round repair. + +A plan-review `blocked` verdict routes its user-only decision through +`## Open questions` and `ask`; the verdict alone is not a lifecycle block. A +technical code-review `blocked` verdict stops immediately. After implementation +has started, a technical block or any terminal repair failure requires the +manager to commit and normally push all current work to the verified linked +branch before recording the blocker, setting the plan `blocked`, and stopping. ## Phases 1. **Decide.** Phase 1 asks exactly one question with exactly three options, in this order and wording: `Plan and implement now`, `Plan only, stop at planned`, `Implement directly` — and skips the question only when the request already settles the mode. 2. **Draft.** Create the plan issue, write the goal and research hypothesis, and keep provisional Steps and Acceptance tables while status remains `drafting`. 3. **Research.** Verify repository facts and external claims, record their sources, choose the durable fix, bind the exact files, complete Acceptance, pass `plan.mjs check`, and set the plan `planned`. -4. **Plan review.** Dispatch exactly one pre-implementation review. Append its verdict and findings. Fix reproduced findings before implementation. A user-only decision goes in `## Open questions`. A plan-only run stops at `planned` after this review. -5. **Implement.** Set the plan `ongoing`, move each step through its legal states, and record real Acceptance output in `## Verification Results` before the closing merge. -6. **Code review.** Review the declared change, fix every critical and high finding, and run exactly one repair re-review after such a fix; if that re-review still returns fixes-required, append `Code-review: blocked` and set the plan `blocked`. Every step must be terminal and code review must pass before the closing merge; archive verifies those facts afterward. +4. **Plan review.** Run up to five rounds from fresh exports. Post each reviewer block as one issue comment. Fix reproduced findings and dispatch a fresh review; stop on pass, no progress, a finding surviving its fix, or `repair` in round five. Route every `blocked` user-only decision through `## Open questions` and `ask`, including in round five. A plan-only run stops at `planned` only after plan review passes. +5. **Implement.** Set the plan `ongoing`, verify and check out its GitHub-linked branch before changing implementation bytes, move each step through its legal states, and record real Acceptance output in `## Verification Results` before the closing merge. +6. **Code review.** Run up to five rounds from fresh complete-candidate diffs and fresh plan exports. Post each reviewer block as one issue comment. Fix every critical and high finding and dispatch a fresh review; stop on pass, no progress, a finding surviving its fix, a technical block, or `fixes-required` in round five. Before any terminal review block, commit and normally push all current work to the linked plan branch. Every step must be terminal and code review must pass before the closing merge; archive verifies those facts afterward. Build the review diff from the complete candidate pull request, not only the dirty worktree. Resolve and fetch the repository default branch, then compute @@ -328,26 +378,44 @@ After pull-request creation, record `headRefOid` and compare the changed paths and hunks from `gh pr diff` with the reviewed net candidate. Any mismatch invalidates the pass and blocks merge. -If that repair re-review again returns `fixes-required`, stop: append -`Code-review: blocked` naming the surviving findings, and set the plan -`blocked`. A step whose `Effect` is not `local` requires an in-session `ask` confirmation immediately before it runs; when `ask` is unavailable the step is set `blocked` and `Blocked: <unconfirmed effect>` is recorded first in `## Open questions`. Routine plan issue publication is authorized by the settled mode and needs no -repeated repository picker. +repeated repository picker. Routine linked-branch creation, commits, and normal +pushes are authorized when a settled `plan-and-implement` run enters phase 5. +Immediately after setting the plan `ongoing`, resolve the target repository's +`nameWithOwner` and `defaultBranchRef.name`. + +Before any branch checkout, and specifically before any `gh issue develop +--checkout`, require `git status --porcelain` to be empty. If it is dirty, never +stash, move, or commit the ambient work. Set the plan `blocked` and name the +dirty paths, or continue only in an authorized clean worktree. + +Pass `--repo <nameWithOwner>` to every `gh issue develop` call. First run +`gh issue develop <issue> --repo <nameWithOwner> --list`. If it reports a +linked branch, verify that branch belongs to the resolved repository, fetch it, +and check it out. Otherwise run `gh issue develop <issue> --repo +<nameWithOwner> --base <default-branch> --checkout`. After either path, verify +that the checked-out branch is the issue's linked branch. + +After any list, create, fetch, or checkout failure, re-run the repository-scoped +`--list`. If it reports a linked branch, verify that branch belongs to the +resolved repository, fetch it, and check it out. If recovery cannot verify and +check out a linked branch, record the blocker, set the plan `blocked`, and stop. +There is no local or unlinked fallback, and implementation never starts on an +unverified branch. ## Landing Work lands through a pull request whose body carries `Closes #<issue>` and whose base is the repository default branch. -After `Code-review: pass`, the manager runs landing without another prompt: -ensure a non-default branch, commit exactly the reviewed bytes under -`docks:commit-discipline`, push normally, and create or update one pull request -that carries `Closes #<issue>` and targets the repository default branch. +After `Code-review: pass`, commit and push any remaining reviewed bytes, then +create or update one pull request carrying `Closes #<issue>` and targeting the +repository default branch. This landing work needs no additional prompt. Never treat an empty first checks result as success. Retry `gh pr checks --json name,bucket` at most 12 times with a 10-second delay until diff --git a/scripts/tests/fixtures/gh-stub/gh b/scripts/tests/fixtures/gh-stub/gh index 558dc281..3c192521 100755 --- a/scripts/tests/fixtures/gh-stub/gh +++ b/scripts/tests/fixtures/gh-stub/gh @@ -103,11 +103,36 @@ function graphqlValue(expression, variables) { return expression; } +function commentJson(comment, issue) { + const login = typeof comment.author === 'string' ? comment.author : comment.author?.login ?? comment.user?.login; + return { + id: comment.id, + body: comment.body, + author: login ? { login } : null, + createdAt: comment.createdAt, + updatedAt: comment.updatedAt ?? comment.createdAt, + url: comment.url ?? `${issue.url}#issuecomment-${comment.id}`, + }; +} + +function apiCommentJson(comment, issue) { + const rendered = commentJson(comment, issue); + return { + id: rendered.id, + body: rendered.body, + user: rendered.author, + created_at: rendered.createdAt, + updated_at: rendered.updatedAt, + html_url: rendered.url, + }; +} + function issueJson(issue) { return { ...issue, labels: labels(issue.labels), assignees: assignees(issue.assignees), + comments: (issue.comments ?? []).map((comment) => commentJson(comment, issue)), closedByPullRequestsReferences: issue.closedByPullRequestsReferences ?? [], }; } @@ -130,6 +155,7 @@ const ISSUE_JSON_FIELDS = new Set([ 'createdAt', 'updatedAt', 'closedByPullRequestsReferences', + 'comments', ]); const PR_JSON_FIELDS = new Set([ 'number', @@ -144,6 +170,21 @@ const PR_JSON_FIELDS = new Set([ 'headRefName', ]); +const commentsEndpoint = /^repos\/([^/]+)\/([^/]+)\/issues\/([1-9]\d*)\/comments$/.exec(`${command ?? ''}`); +if (group === 'api' && commentsEndpoint) { + if (!args.includes('--paginate') || !args.includes('--slurp')) die('issue comments api requires --paginate and --slurp'); + const repo = `${commentsEndpoint[1]}/${commentsEndpoint[2]}`; + if (repo !== state.repo.nameWithOwner) die(`repository not found: ${repo}`); + const issue = issueByNumber(Number(commentsEndpoint[3])); + const comments = (issue.comments ?? []).map((comment) => apiCommentJson(comment, issue)); + const pages = []; + for (let index = 0; index < comments.length; index += 100) pages.push(comments.slice(index, index + 100)); + if (pages.length === 0) pages.push([]); + console.log(JSON.stringify(pages)); + save(); + process.exit(0); +} + if (group === 'api' && command === 'graphql') { const query = formFields('-f').query; if (!query) die('graphql requires -f query=<query>'); @@ -321,6 +362,7 @@ if (group === 'issue' && command === 'create') { url: `https://github.com/${state.repo.nameWithOwner}/issues/${number}`, createdAt: now, updatedAt: now, + comments: [], closedByPullRequestsReferences: [], }; state.issues.push(issue); diff --git a/scripts/tests/plan-cli.mjs b/scripts/tests/plan-cli.mjs index 7b6ea9fa..5405be7e 100755 --- a/scripts/tests/plan-cli.mjs +++ b/scripts/tests/plan-cli.mjs @@ -68,6 +68,7 @@ fs.writeFileSync( issues: [], prs: [], nextIssue: 1, + nextComment: 1, clock: 0, calls: [], }, @@ -121,6 +122,20 @@ function updateIssue(number, update) { updateState((state) => update(state.issues.find((entry) => entry.number === number))); } +function addIssueComment(number, body, author = 'plan-agent') { + updateState((state) => { + const entry = state.issues.find((candidate) => candidate.number === number); + const id = state.nextComment++; + entry.comments ??= []; + entry.comments.push({ + id, + body, + author, + createdAt: new Date(Date.parse('2026-08-20T20:00:00Z') + id * 1000).toISOString(), + }); + }); +} + function createPlan(name, options = {}) { const title = options.title ?? `Exercise ${name}`; const goal = options.goal ?? `The ${name} plan completes its observable contract`; @@ -243,6 +258,7 @@ try { 'GitHub-owned machine fields must not be stored in the body', ); assert.match(template.body, /^Mode: plan-only$/m); + assert.match(template.body, /^_Review records are stored in issue comments\._$/m); assert.deepEqual(template.labels, ['plan', 'plan:drafting']); assert.deepEqual(template.assignees, ['plan-agent']); const createCall = loadState().calls.find( @@ -619,13 +635,79 @@ try { const shown = run('show', String(editNumber)); expectSuccess(shown, 'show'); const shownHeader = `#${editNumber} · drafting · Exercise edit · ${issue(editNumber).url}`; - assert.equal(shown.stdout, `${shownHeader}\n`); + const emptyReviewSummary = 'reviews: plan=none code=none'; + assert.equal(shown.stdout, `${shownHeader}\n${emptyReviewSummary}\n`); assert.equal(shown.stderr, ''); assert.doesNotMatch(shown.stdout, /plan_contract:/); const shownBody = run('show', `#${editNumber}`, '--body'); expectSuccess(shownBody, 'show --body'); assert.equal(shownBody.stdout, issue(editNumber).body); - assert.equal(shownBody.stderr, `${shownHeader}\n`); + assert.equal(shownBody.stderr, `${shownHeader}\n${emptyReviewSummary}\n`); + + const commentSummaryNumber = createPlan('comment-review-summary'); + updateState((state) => { + const entry = state.issues.find((candidate) => candidate.number === commentSummaryNumber); + entry.comments = Array.from({ length: 100 }, (_, index) => ({ + id: index + 1, + body: + index === 0 + ? '### Plan review — 2026-08-20\nPlan-review: repair\n- [goal_fit] ## Goal — foreign finding — ignore it' + : 'not a review record', + author: 'other-agent', + createdAt: new Date(Date.parse('2026-08-20T20:00:00Z') + index * 1000).toISOString(), + })); + state.nextComment = 101; + }); + addIssueComment( + commentSummaryNumber, + '### Plan review — 2026-08-21\nPlan-review: repair\n- [research_gap] src/example.mjs:1 — evidence is missing — cite the source', + ); + addIssueComment(commentSummaryNumber, '### Code review round 1 — 2026-08-21\nCode-review: pass'); + addIssueComment(commentSummaryNumber, '### Plan review — 2026-08-22\nPlan-review: pass'); + addIssueComment( + commentSummaryNumber, + '### Code review round 2 — 2026-08-22\nCode-review: fixes-required\n- HIGH · Bug · src/example.mjs:9 — later regression survives — fix the regression', + ); + addIssueComment(commentSummaryNumber, '### Code review round 3 — 2026-08-23\nCode-review: pass', 'other-agent'); + addIssueComment( + commentSummaryNumber, + '### Code review round 4 — 2026-08-24\nCode-review: pass\n- HIGH · Security · src/example.mjs:10 — malformed pass carries high — reject the malformed record', + ); + const commentCallsBefore = loadState().calls.length; + const commentSummary = run('show', String(commentSummaryNumber)); + expectSuccess(commentSummary, 'show paginated trusted review summary'); + assert.equal( + commentSummary.stdout, + `#${commentSummaryNumber} · drafting · Exercise comment-review-summary · ${issue(commentSummaryNumber).url}\n` + + 'reviews: plan=pass code=fixes-required\n', + ); + const commentApiCall = loadState() + .calls.slice(commentCallsBefore) + .find( + (call) => call[0] === 'api' && call[1] === `repos/DocksDocks/fixture/issues/${commentSummaryNumber}/comments`, + ); + assert.ok(commentApiCall?.includes('--paginate'), 'show must request every issue-comment page'); + assert.ok(commentApiCall?.includes('--slurp'), 'show must parse paginated issue comments as pages'); + + const legacySummaryNumber = createPlan('legacy-review-summary'); + updateIssue(legacySummaryNumber, (entry) => { + entry.body = entry.body.replace( + '_Review records are stored in issue comments._', + 'Plan-review: repair\nCode-review: pass', + ); + }); + const legacySummary = run('show', String(legacySummaryNumber)); + expectSuccess(legacySummary, 'show legacy review summary'); + assert.match(legacySummary.stdout, /reviews: plan=repair code=pass\n$/); + + const ambiguousOwnerSummaryNumber = createPlan('ambiguous-owner-review-summary'); + updateIssue(ambiguousOwnerSummaryNumber, (entry) => { + entry.assignees.push('other-agent'); + }); + addIssueComment(ambiguousOwnerSummaryNumber, '### Plan review — 2026-08-24\nPlan-review: pass'); + const ambiguousOwnerSummary = run('show', String(ambiguousOwnerSummaryNumber)); + expectSuccess(ambiguousOwnerSummary, 'show refuses comment trust without a sole assignee'); + assert.match(ambiguousOwnerSummary.stdout, /reviews: plan=none code=none\n$/); const reviewDirectory = path.join(scratch, '.git/docks-review'); const exported = run('export', String(editNumber)); @@ -1195,7 +1277,10 @@ try { assert.match(unfinishedArchive.stderr, /non-terminal step/); updateIssue(unfinishedArchiveNumber, (entry) => { - entry.body = replaceStepStatus(entry.body, 'done').replace('_No review yet._', 'Code-review: pass with caveat'); + entry.body = replaceStepStatus(entry.body, 'done').replace( + '_Review records are stored in issue comments._', + 'Code-review: pass with caveat', + ); }); const inexactReviewArchive = run('archive', String(unfinishedArchiveNumber)); assert.equal(inexactReviewArchive.status, 1, 'changing the exact Code-review pass line must be refused'); @@ -1206,7 +1291,7 @@ try { setIssueStatus(advisoryPassNumber, 'ongoing'); updateIssue(advisoryPassNumber, (entry) => { entry.body = replaceStepStatus(entry.body, 'done').replace( - '_No review yet._', + '_Review records are stored in issue comments._', 'Code-review: pass\n- MEDIUM · Maintainability · src/example.mjs:9 — duplicated guard clause — extract a named predicate', ); entry.state = 'CLOSED'; @@ -1233,12 +1318,70 @@ try { const advisoryPassArchive = run('archive', String(advisoryPassNumber)); expectSuccess(advisoryPassArchive, 'archive accepts a pass carrying only an advisory line'); + const trustedPassNumber = createPlan('trusted-comment-pass-archive'); + makeValid(trustedPassNumber); + setIssueStatus(trustedPassNumber, 'ongoing'); + updateIssue(trustedPassNumber, (entry) => { + entry.body = replaceStepStatus(entry.body, 'done'); + entry.state = 'CLOSED'; + entry.stateReason = 'COMPLETED'; + entry.closedByPullRequestsReferences = [ + { + number: 43, + url: 'https://github.com/DocksDocks/fixture/pull/43', + repository: 'DocksDocks/fixture', + userLinked: false, + }, + ]; + }); + addIssueComment(trustedPassNumber, '### Code review round 1 — 2026-08-24\nCode-review: pass'); + const trustedPassArchive = run('archive', String(trustedPassNumber)); + expectSuccess(trustedPassArchive, 'archive accepts latest trusted code-review pass comment'); + + const supersededPassNumber = createPlan('superseded-comment-pass-archive'); + makeValid(supersededPassNumber); + setIssueStatus(supersededPassNumber, 'ongoing'); + updateIssue(supersededPassNumber, (entry) => { + entry.body = replaceStepStatus(entry.body, 'done').replace( + '_Review records are stored in issue comments._', + 'Code-review: pass', + ); + entry.state = 'CLOSED'; + entry.stateReason = 'COMPLETED'; + }); + addIssueComment(supersededPassNumber, '### Code review round 1 — 2026-08-23\nCode-review: pass'); + addIssueComment( + supersededPassNumber, + '### Code review round 2 — 2026-08-24\nCode-review: fixes-required\n- HIGH · Bug · src/example.mjs:9 — the earlier pass is stale — repair the defect', + ); + const supersededPassArchive = run('archive', String(supersededPassNumber)); + assert.equal(supersededPassArchive.status, 1, 'a later trusted fixes-required record must supersede a pass'); + assert.match(supersededPassArchive.stderr, /archive requires Code-review: pass/); + + for (const [name, commentBody, author] of [ + ['foreign-comment-pass', '### Code review round 1 — 2026-08-24\nCode-review: pass', 'other-agent'], + ['malformed-comment-pass', 'Code-review: pass', 'plan-agent'], + ]) { + const number = createPlan(`${name}-archive`); + makeValid(number); + setIssueStatus(number, 'ongoing'); + updateIssue(number, (entry) => { + entry.body = replaceStepStatus(entry.body, 'done'); + entry.state = 'CLOSED'; + entry.stateReason = 'COMPLETED'; + }); + addIssueComment(number, commentBody, author); + const refused = run('archive', String(number)); + assert.equal(refused.status, 1, `${name} must not authorize archive`); + assert.match(refused.stderr, /archive requires Code-review: pass/); + } + const mediumOnlyRequiredNumber = createPlan('medium-only-fixes-required'); makeValid(mediumOnlyRequiredNumber); setIssueStatus(mediumOnlyRequiredNumber, 'ongoing'); updateIssue(mediumOnlyRequiredNumber, (entry) => { entry.body = replaceStepStatus(entry.body, 'done').replace( - '_No review yet._', + '_Review records are stored in issue comments._', 'Code-review: fixes-required\n- MEDIUM · Maintainability · src/example.mjs:9 — duplicated guard clause — extract a named predicate', ); entry.state = 'CLOSED'; @@ -1252,7 +1395,10 @@ try { makeValid(wrongBranchNumber); setIssueStatus(wrongBranchNumber, 'ongoing'); updateIssue(wrongBranchNumber, (entry) => { - entry.body = replaceStepStatus(entry.body, 'done').replace('_No review yet._', 'Code-review: pass'); + entry.body = replaceStepStatus(entry.body, 'done').replace( + '_Review records are stored in issue comments._', + 'Code-review: pass', + ); entry.state = 'CLOSED'; entry.stateReason = 'COMPLETED'; entry.closedByPullRequestsReferences = [ @@ -1303,7 +1449,10 @@ try { makeValid(manualLinkArchiveNumber); setIssueStatus(manualLinkArchiveNumber, 'ongoing'); updateIssue(manualLinkArchiveNumber, (entry) => { - entry.body = replaceStepStatus(entry.body, 'done').replace('_No review yet._', 'Code-review: pass'); + entry.body = replaceStepStatus(entry.body, 'done').replace( + '_Review records are stored in issue comments._', + 'Code-review: pass', + ); entry.state = 'CLOSED'; entry.stateReason = 'COMPLETED'; entry.closedByPullRequestsReferences = [ @@ -1354,7 +1503,10 @@ try { makeValid(keywordArchiveNumber); setIssueStatus(keywordArchiveNumber, 'ongoing'); updateIssue(keywordArchiveNumber, (entry) => { - entry.body = replaceStepStatus(entry.body, 'done').replace('_No review yet._', 'Code-review: pass'); + entry.body = replaceStepStatus(entry.body, 'done').replace( + '_Review records are stored in issue comments._', + 'Code-review: pass', + ); entry.state = 'CLOSED'; entry.stateReason = 'COMPLETED'; entry.closedByPullRequestsReferences = [ @@ -1392,7 +1544,10 @@ try { makeValid(paginatedArchiveNumber); setIssueStatus(paginatedArchiveNumber, 'ongoing'); updateIssue(paginatedArchiveNumber, (entry) => { - entry.body = replaceStepStatus(entry.body, 'done').replace('_No review yet._', 'Code-review: pass'); + entry.body = replaceStepStatus(entry.body, 'done').replace( + '_Review records are stored in issue comments._', + 'Code-review: pass', + ); entry.state = 'CLOSED'; entry.stateReason = 'COMPLETED'; entry.closedByPullRequestsReferences = Array.from({ length: 101 }, (_, index) => ({ @@ -1425,7 +1580,10 @@ try { makeValid(closedReferenceNumber); setIssueStatus(closedReferenceNumber, 'ongoing'); updateIssue(closedReferenceNumber, (entry) => { - entry.body = replaceStepStatus(entry.body, 'done').replace('_No review yet._', 'Code-review: pass'); + entry.body = replaceStepStatus(entry.body, 'done').replace( + '_Review records are stored in issue comments._', + 'Code-review: pass', + ); entry.state = 'CLOSED'; entry.stateReason = 'COMPLETED'; entry.closedByPullRequestsReferences = [ @@ -1460,7 +1618,10 @@ try { makeValid(commitArchiveNumber); setIssueStatus(commitArchiveNumber, 'ongoing'); updateIssue(commitArchiveNumber, (entry) => { - entry.body = replaceStepStatus(entry.body, 'done').replace('_No review yet._', 'Code-review: pass'); + entry.body = replaceStepStatus(entry.body, 'done').replace( + '_Review records are stored in issue comments._', + 'Code-review: pass', + ); entry.state = 'CLOSED'; entry.stateReason = 'COMPLETED'; entry.closedByPullRequestsReferences = []; @@ -1509,7 +1670,10 @@ try { makeValid(ineligibleReferenceCommitNumber); setIssueStatus(ineligibleReferenceCommitNumber, 'ongoing'); updateIssue(ineligibleReferenceCommitNumber, (entry) => { - entry.body = replaceStepStatus(entry.body, 'done').replace('_No review yet._', 'Code-review: pass'); + entry.body = replaceStepStatus(entry.body, 'done').replace( + '_Review records are stored in issue comments._', + 'Code-review: pass', + ); entry.state = 'CLOSED'; entry.stateReason = 'COMPLETED'; entry.closedByPullRequestsReferences = [ @@ -1559,7 +1723,10 @@ try { makeValid(pullRequestLatestCloserNumber); setIssueStatus(pullRequestLatestCloserNumber, 'ongoing'); updateIssue(pullRequestLatestCloserNumber, (entry) => { - entry.body = replaceStepStatus(entry.body, 'done').replace('_No review yet._', 'Code-review: pass'); + entry.body = replaceStepStatus(entry.body, 'done').replace( + '_Review records are stored in issue comments._', + 'Code-review: pass', + ); entry.state = 'CLOSED'; entry.stateReason = 'COMPLETED'; entry.closedByPullRequestsReferences = []; @@ -1600,7 +1767,10 @@ try { makeValid(directPushNumber); setIssueStatus(directPushNumber, 'ongoing'); updateIssue(directPushNumber, (entry) => { - entry.body = replaceStepStatus(entry.body, 'done').replace('_No review yet._', 'Code-review: pass'); + entry.body = replaceStepStatus(entry.body, 'done').replace( + '_Review records are stored in issue comments._', + 'Code-review: pass', + ); entry.state = 'CLOSED'; entry.stateReason = 'COMPLETED'; entry.closedByPullRequestsReferences = []; @@ -1623,7 +1793,10 @@ try { makeValid(commitWrongBranchNumber); setIssueStatus(commitWrongBranchNumber, 'ongoing'); updateIssue(commitWrongBranchNumber, (entry) => { - entry.body = replaceStepStatus(entry.body, 'done').replace('_No review yet._', 'Code-review: pass'); + entry.body = replaceStepStatus(entry.body, 'done').replace( + '_Review records are stored in issue comments._', + 'Code-review: pass', + ); entry.state = 'CLOSED'; entry.stateReason = 'COMPLETED'; entry.closedByPullRequestsReferences = []; @@ -1652,7 +1825,10 @@ try { makeValid(staleCommitNumber); setIssueStatus(staleCommitNumber, 'ongoing'); updateIssue(staleCommitNumber, (entry) => { - entry.body = replaceStepStatus(entry.body, 'done').replace('_No review yet._', 'Code-review: pass'); + entry.body = replaceStepStatus(entry.body, 'done').replace( + '_Review records are stored in issue comments._', + 'Code-review: pass', + ); entry.state = 'CLOSED'; entry.stateReason = 'COMPLETED'; entry.closedByPullRequestsReferences = []; diff --git a/scripts/tests/plan-skill-phases.mjs b/scripts/tests/plan-skill-phases.mjs index e10f2bac..8f9a9a1b 100755 --- a/scripts/tests/plan-skill-phases.mjs +++ b/scripts/tests/plan-skill-phases.mjs @@ -27,6 +27,9 @@ const README_MD = 'README.md'; const EXECUTOR_DISPATCH = 'plugins/docks/skills/engineering/refactor/references/executor-dispatch.md'; const LIFECYCLE_SKILLS_AGENTS = 'plugins/plan-lifecycle/skills/AGENTS.md'; const DOCKS_README = 'plugins/docks/README.md'; +const CLAUDE_PLAN_MANIFEST = 'plugins/plan-lifecycle/.claude-plugin/plugin.json'; +const CODEX_PLAN_MANIFEST = 'plugins/plan-lifecycle/.codex-plugin/plugin.json'; +const CLAUDE_MARKETPLACE = '.claude-plugin/marketplace.json'; const V3_PINNED_CLAUSES = [ { @@ -65,7 +68,7 @@ const V3_PINNED_CLAUSES = [ }, { name: 'review-export-dispatch', - text: 'Run `plan.mjs export <issue>` first and dispatch the reviewer with the issue number and the printed export path', + text: "`plan.mjs export <issue>` and dispatch `plan-reviewer` with the issue number and that round's printed export path", files: [MANAGER_SKILL], }, { @@ -73,10 +76,50 @@ const V3_PINNED_CLAUSES = [ text: 'The settled plan mode authorizes routine creation and update of the plan issue in the repository that the preflight resolved. Do not ask again for that publication or show a repository picker that repeats a resolved fact.', files: [MANAGER_SKILL, ISSUE_PUBLICATION], }, + { + name: 'body-review-comment-pointer', + text: '`## Review` contains exactly `_Review records are stored in issue comments._`; review reports are not appended to the body.', + files: [PLAN_MD, WORKSPACE_TEMPLATE], + }, + { + name: 'contract-review-comment-pointer', + text: '`## Review` is a static pointer, not a review log:', + files: [PLAN_CONTRACT, PLAN_MD, WORKSPACE_TEMPLATE], + }, + { + name: 'generated-review-comment-pointer', + text: 'Closed completion derives from GitHub `state` and `stateReason`. `## Review` contains exactly `_Review records are stored in issue comments._`.', + files: [WORKSPACE_SKILL, ROOT_AGENTS], + }, + { + name: 'review-comment-publication', + text: 'The reviewer returns exactly one markdown block. The manager posts that whole block as one issue comment without editing it.', + files: [PLAN_CONTRACT, PLAN_MD, WORKSPACE_TEMPLATE], + }, + { + name: 'trusted-review-selection', + text: "A record is trusted only when the issue has exactly one assignee and the comment's author login equals that assignee. For each review kind independently, the latest trusted well-formed comment wins, ordered by `createdAt` with API order as the tie-break. Foreign-authored, malformed, and superseded comments never establish current review state. A legacy verdict in the body is consulted for one review kind only when there is no trusted well-formed comment record of that kind.", + files: [PLAN_CONTRACT, PLAN_MD, WORKSPACE_TEMPLATE], + }, { name: 'code-review-repair-bound', - text: 'If that repair re-review again returns `fixes-required`, stop: append `Code-review: blocked` naming the surviving findings, and set the plan `blocked`.', - files: [MANAGER_SKILL, PLAN_MD, WORKSPACE_TEMPLATE], + text: 'Both review phases run at most five rounds. Each round uses a fresh plan export; each code-review round also uses a fresh complete-candidate diff. On rounds 1 through 4, a `repair` or `fixes-required` verdict requires every reproduced or named finding to be fixed, followed by a fresh export or diff and a fresh review. A repair that changes no relevant bytes is no progress. A finding repeated in the next round survived its fix. Either condition stops the loop, as does `repair` or `fixes-required` in round 5; there is no sixth-round repair.', + files: [PLAN_CONTRACT, PLAN_MD, WORKSPACE_TEMPLATE], + }, + { + name: 'manager-plan-review-bound', + text: '4. **Plan review.** Run at most five rounds.', + files: [MANAGER_SKILL], + }, + { + name: 'manager-code-review-bound', + text: '6. **Code review.** Run at most five rounds.', + files: [MANAGER_SKILL], + }, + { + name: 'plan-review-blocked-routing', + text: 'A plan-review `blocked` verdict routes its user-only decision through `## Open questions` and `ask`; the verdict alone is not a lifecycle block.', + files: [PLAN_CONTRACT, PLAN_MD, WORKSPACE_TEMPLATE], }, { name: 'code-review-pass-verdict', @@ -85,23 +128,33 @@ const V3_PINNED_CLAUSES = [ }, { name: 'code-review-fixes-required-verdict', - text: '- `fixes-required`: At least one evidenced `CRITICAL` or `HIGH` defect. The manager fixes it and dispatches exactly one repair re-review.', + text: '- `fixes-required`: At least one evidenced `CRITICAL` or `HIGH` defect. The manager fixes every named defect and dispatches a fresh re-review on a fresh diff.', files: [CODE_REVIEWER_AGENT, CODE_REVIEWER_CODEX, CODE_REVIEWER_TEMPLATE], }, { name: 'advisory-pass-immutable', - text: 'Record each advisory as a follow-up and do not change reviewed bytes after a pass; an advisory never triggers a re-review.', - files: [MANAGER_SKILL, PLAN_CONTRACT, PLAN_MD, WORKSPACE_TEMPLATE], + text: 'After a pass, record each advisory as follow-up work and do not change reviewed bytes; advisory findings never trigger another review.', + files: [PLAN_CONTRACT, PLAN_MD, WORKSPACE_TEMPLATE], }, { name: 'review-scope-guard', text: 'Build the review diff from the complete candidate pull request, not only the dirty worktree. Resolve and fetch the repository default branch, then compute `<merge-base>` with `git merge-base <default-remote-ref> HEAD`. Cover one net tracked candidate with `git diff <merge-base> -- <changed paths>`. Add one `git diff --no-index /dev/null <path>` hunk for each untracked path. `git status --porcelain` still names dirty paths. Name every changed path that no Steps `Files` cell mentions in the review request.', - files: [MANAGER_SKILL, PLAN_MD, WORKSPACE_TEMPLATE], + files: [PLAN_MD, WORKSPACE_TEMPLATE], + }, + { + name: 'manager-review-scope-guard', + text: 'At the start of every round, build a fresh review diff from the complete candidate pull request, not only the dirty worktree.', + files: [MANAGER_SKILL], }, { name: 'reviewed-pr-diff-match', text: 'After pull-request creation, record `headRefOid` and compare the changed paths and hunks from `gh pr diff` with the reviewed net candidate. Any mismatch invalidates the pass and blocks merge.', - files: [MANAGER_SKILL, PLAN_MD, WORKSPACE_TEMPLATE], + files: [PLAN_MD, WORKSPACE_TEMPLATE], + }, + { + name: 'manager-reviewed-pr-diff-match', + text: 'Record its `headRefOid` and compare the changed paths and hunks from `gh pr diff` with the reviewed net candidate. Any mismatch invalidates the pass and blocks merge.', + files: [MANAGER_SKILL], }, { name: 'nonlocal-effect-confirmation', @@ -125,7 +178,7 @@ const V3_PINNED_CLAUSES = [ }, { name: 'record-backend', - text: 'The plan record is a GitHub issue. Its body carries the v3 byte contract and the human-authored plan, while GitHub fields carry the machine state GitHub already owns. No plan markdown is tracked in the repository.', + text: 'The plan record is a GitHub issue. Its body carries the v3 byte contract and the human-authored plan, review records live in issue comments, and GitHub fields carry the machine state GitHub already owns. No plan markdown is tracked in the repository.', files: [PLAN_MD, WORKSPACE_TEMPLATE], }, { @@ -134,10 +187,35 @@ const V3_PINNED_CLAUSES = [ files: [MANAGER_SKILL, PLAN_CONTRACT, PLAN_MD, WORKSPACE_TEMPLATE], }, { - name: 'default-pr-landing', - text: 'After `Code-review: pass`, the manager runs landing without another prompt: ensure a non-default branch, commit exactly the reviewed bytes under `docks:commit-discipline`, push normally, and create or update one pull request that carries `Closes #<issue>` and targets the repository default branch.', + name: 'show-review-summary', + text: 'The header strip is `#<issue> · <status> · <title> · <url>`. `show` prints `reviews: plan=<pass|repair|blocked|none> code=<pass|fixes-required|blocked|none>` on the next line. With `show --body`, the record alone goes to stdout and both metadata lines go to stderr, header first.', + files: [PLAN_CONTRACT, PLAN_MD, WORKSPACE_TEMPLATE], + }, + { + name: 'plan-only-resume-body', + text: 'A later session resumes by reading the full record with `plan.mjs show <issue> --body`.', + files: [MANAGER_SKILL], + }, + { + name: 'implement-branch-clean-tree', + text: 'Before any branch checkout, and specifically before any `gh issue develop --checkout`, require `git status --porcelain` to be empty. If it is dirty, never stash, move, or commit the ambient work. Set the plan `blocked` and name the dirty paths, or continue only in an authorized clean worktree.', files: [MANAGER_SKILL, ISSUE_PUBLICATION, PLAN_CONTRACT, PLAN_MD, WORKSPACE_TEMPLATE], }, + { + name: 'implement-linked-branch', + text: "Pass `--repo <nameWithOwner>` to every `gh issue develop` call. First run `gh issue develop <issue> --repo <nameWithOwner> --list`. If it reports a linked branch, verify that branch belongs to the resolved repository, fetch it, and check it out. Otherwise run `gh issue develop <issue> --repo <nameWithOwner> --base <default-branch> --checkout`. After either path, verify that the checked-out branch is the issue's linked branch.", + files: [PLAN_CONTRACT, PLAN_MD, WORKSPACE_TEMPLATE], + }, + { + name: 'default-pr-landing', + text: 'After `Code-review: pass`, commit and push any remaining reviewed bytes, then create or update one pull request carrying `Closes #<issue>` and targeting the repository default branch. This landing work needs no additional prompt.', + files: [ISSUE_PUBLICATION, PLAN_CONTRACT, PLAN_MD, WORKSPACE_TEMPLATE], + }, + { + name: 'manager-default-pr-landing', + text: 'After a pass, commit and push any remaining reviewed bytes, then create or update the closing pull request under `## Landing`.', + files: [MANAGER_SKILL], + }, { name: 'ci-check-discovery', text: 'Never treat an empty first checks result as success. Retry `gh pr checks --json name,bucket` at most 12 times with a 10-second delay until checks appear. If required checks exist, run `gh pr checks --watch --required`; if CI checks exist but none are required, run `gh pr checks --watch` to wait for all reported CI. Any failed check blocks merge. If no checks appear, continue only when repository inspection confirms that no pull-request CI is configured; otherwise leave the pull request open with a named no-checks blocker and do not show the merge prompt.', @@ -155,24 +233,34 @@ const V3_PINNED_CLAUSES = [ }, { name: 'repository-landing-summary', - text: "Routine plan issue publication and post-review landing through a branch, commit, normal push, and closing pull request carry the settled mode's authorization and need no repeated prompt. After the checks policy passes, the manager asks immediately before merge. Without a fresh `Merge now` answer, it leaves the pull request and issue open. `plan.mjs archive` verifies the merged closing pull request after landing.", + text: "Routine plan issue publication, implement-start linked branch creation, commits, normal pushes, and the closing pull request carry the settled mode's authorization and need no repeated prompt.", + files: [WORKSPACE_SKILL, ROOT_AGENTS], + }, + { + name: 'generated-clean-tree-summary', + text: 'Before any branch checkout, including `gh issue develop --checkout`, require `git status --porcelain` to be empty. If it is dirty, never stash, move, or commit ambient work; set the plan `blocked` and name the dirty paths, or use an authorized clean worktree.', files: [WORKSPACE_SKILL, ROOT_AGENTS], }, { name: 'readme-landing-summary', - text: 'The lifecycle runs six phases: decide, draft, research, one plan review, implement, and one post-implementation code review. After review passes, it commits and pushes the reviewed branch, opens the closing pull request, and waits for repository CI. It then asks `Merge now` or `Leave pull request open`. Without a fresh `Merge now` answer, it leaves the pull request and issue open. After an approved merge, `plan.mjs archive` verifies the merged closing pull request.', + text: 'The lifecycle runs six phases: decide, draft, research, plan review, implement, and code review. Plan repairs are re-reviewed from fresh exports, and code fixes are re-reviewed from fresh diffs, with a five-round ceiling in each review phase. Each reviewer returns one markdown block that the manager stores as one issue comment. When implementation starts, the manager reuses or creates the GitHub-linked plan branch. After code review passes, it commits and pushes any remaining reviewed bytes, opens the closing pull request, and waits for repository CI. It then asks `Merge now` or `Leave pull request open`. Without a fresh `Merge now` answer, it leaves the pull request and issue open. After an approved merge, `plan.mjs archive` verifies the merged closing pull request.', files: [README_MD], }, { name: 'shipped-readme-landing-summary', - text: 'After review passes, the manager commits and pushes the reviewed branch, opens the closing pull request, waits for repository CI, and asks `Merge now` or `Leave pull request open`. Without a fresh `Merge now` answer, it leaves the pull request and issue open. After an approved merge, `plan.mjs archive` verifies the merged closing pull request.', + text: 'Plan repairs are re-reviewed from fresh exports, and code fixes are re-reviewed from fresh diffs, with a five-round ceiling in each review phase. Each reviewer returns one markdown block that the manager stores as one issue comment. When implementation starts, the manager reuses or creates the GitHub-linked plan branch. After code review passes, it commits and pushes any remaining reviewed bytes, opens the closing pull request, waits for repository CI, and asks `Merge now` or `Leave pull request open`. Without a fresh `Merge now` answer, it leaves the pull request and issue open. After an approved merge, `plan.mjs archive` verifies the merged closing pull request.', files: [DOCKS_README], }, { name: 'lifecycle-authoring-landing-boundary', - text: 'default issue and pull-request publication, explicit merge confirmation', + text: 'comment-backed review records, five-round plan and code review repair loops, implement-start linked-branch publication, default pull-request landing, explicit merge confirmation', files: [LIFECYCLE_SKILLS_AGENTS], }, + { + name: 'descriptor-coherence', + text: 'Cross-tool GitHub-issue plan lifecycle with marker-based plan bodies, comment-backed review records, implement-start linked branches, bounded plan and code review repair loops, and two read-only reviewer wrappers.', + files: [CLAUDE_PLAN_MANIFEST, CODEX_PLAN_MANIFEST, CLAUDE_MARKETPLACE], + }, { name: 'executor-landing-handoff', text: "follows the manager's full Landing flow. It archives only after an approved merge lands the closing pull request.", @@ -195,7 +283,7 @@ const V3_PINNED_CLAUSES = [ }, { name: 'archive-verifier', - text: '`plan.mjs archive` is a verifier, not a writer of lifecycle state. It requires all Steps rows to be terminal (`done` or `skipped`), a line matching exactly `Code-review: pass` in `## Review`, and an issue already closed as completed by an eligible merged pull request. It writes no status. On success it removes any stale phase label and prints `plan #<n> finished (closed by <url>)`.', + text: '`plan.mjs archive` is a verifier, not a writer of lifecycle state. It requires all Steps rows to be terminal (`done` or `skipped`), the latest trusted well-formed code-review comment to carry `Code-review: pass`, and an issue already closed as completed by an eligible merged pull request. It accepts an exact legacy body line `Code-review: pass` only when no trusted well-formed code-review comment exists. It writes no status. On success it removes any stale phase label and prints `plan #<n> finished (closed by <url>)`.', files: [PLAN_MD, WORKSPACE_TEMPLATE], }, { @@ -203,13 +291,18 @@ const V3_PINNED_CLAUSES = [ text: 'A plan-issue write is a read-modify-write, and the GitHub API offers no precondition for it. Every mutating command re-reads the issue body immediately before the edit, refuses when it differs from the body it read, and re-reads after the edit to confirm the pushed bytes.', files: [PLAN_CONTRACT, PLAN_MD, WORKSPACE_TEMPLATE], }, + { + name: 'frozen-history-boundary', + text: 'No lifecycle command or workspace migration operation opens, inventories, parses, classifies, lists, or migrates it.', + files: [PLAN_CONTRACT, PLAN_MD, WORKSPACE_TEMPLATE], + }, ]; const CONTRACT_CLASSIFICATION_PINS = [ { name: 'v3 marker', text: '<!-- plan-contract: v3 -->' }, { name: 'unreadable outcome', text: '| Anything else | unreadable | Refused; no parser is attempted |' }, { - name: 'byte-preserving migration rule', - text: '`docs/plans/finished/` holds records written before the lifecycle moved to issues; it is history, never a source of truth, and no command reads it.', + name: 'frozen-history-human-read-boundary', + text: '`docs/plans/finished/` holds records written before the lifecycle moved to issues. Humans may read it as history, but it is never a source of truth.', }, ];