Skip to content

feat: add make-plan-v2 and do-v2 skills with 5 competitive enhancements#2070

Open
thedotmack wants to merge 1 commit intomainfrom
thedotmack/gsd-superpowers-research
Open

feat: add make-plan-v2 and do-v2 skills with 5 competitive enhancements#2070
thedotmack wants to merge 1 commit intomainfrom
thedotmack/gsd-superpowers-research

Conversation

@thedotmack
Copy link
Copy Markdown
Owner

Summary

  • Adds make-plan-v2 and do-v2 skills alongside the originals for side-by-side testing.
  • Five enhancements: structured plan files with YAML frontmatter, task sizing constraints, 4-status subagent protocol, human gate before commits, and goal-backward verification with must-be-true conditions.
  • No changes to existing make-plan/do skills — v2 variants are additive so the originals stay stable while we evaluate.

Test plan

  • Invoke /make-plan-v2 on a representative task and confirm the plan file includes YAML frontmatter and sized tasks
  • Run /do-v2 against a v2 plan and verify the 4-status subagent protocol and pre-commit human gate fire correctly
  • Spot-check that goal-backward verification surfaces must-be-true conditions at the end of execution
  • Confirm existing /make-plan and /do skills still work unchanged

🤖 Generated with Claude Code

New v2 skill variants alongside originals for side-by-side testing.
Enhancements: structured plan files with YAML frontmatter, task sizing
constraints, 4-status subagent protocol, human gate before commits,
and goal-backward verification with must-be-true conditions.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
@coderabbitai
Copy link
Copy Markdown
Contributor

coderabbitai Bot commented Apr 19, 2026

Summary by CodeRabbit

Release Notes

  • New Features
    • Orchestrator skill for executing multi-phase implementation plans with verification gates, status tracking, phase validation, and managed subagent handoffs between phases
    • Planning skill for structured phased planning with mandatory documentation of sources, findings with exact file locations, confidence assessment, and executable verification commands

Walkthrough

Two new Markdown skill definition files were added: do-v2/SKILL.md establishes a multi-phase orchestrator execution protocol with subagent coordination, plan staleness validation, verification requirements, and status tracking; make-plan-v2/SKILL.md defines a phased planning workflow emphasizing documentation discovery, structured reporting contracts, and concrete API references.

Changes

Cohort / File(s) Summary
Orchestrator Skill Definitions
plugin/skills/do-v2/SKILL.md, plugin/skills/make-plan-v2/SKILL.md
Two new Markdown skill specifications defining orchestrator workflows: do-v2 prescribes execution of multi-phase plans with subagent coordination, plan validation, per-phase verification gates, anti-pattern scanning, and explicit human confirmation before commits; make-plan-v2 establishes planning protocol with mandatory Phase 0 documentation discovery, subagent reporting contract enforcing sources and status taxonomy (DONE / DONE_WITH_CONCERNS / NEEDS_CONTEXT / BLOCKED), and plan output requirements including YAML frontmatter and .gitignore handling.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

Poem

🐰 A rabbit's ode to orchestrated toil:

Phase by phase, the plan takes shape,
Subagents dance without escape,
Documentation mapped with care,
Verification guards the lair—
No commit till all is blessed and bright!

🚥 Pre-merge checks | ✅ 3
✅ Passed checks (3 passed)
Check name Status Explanation
Title check ✅ Passed The title 'feat: add make-plan-v2 and do-v2 skills with 5 competitive enhancements' directly describes the main change—adding two new v2 skill files with specific enhancements—and is concise and specific.
Description check ✅ Passed The description is directly related to the changeset, clearly explaining what was added (two new skill files), the five enhancements they introduce, and confirming that existing skills remain unchanged.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch thedotmack/gsd-superpowers-research

Comment @coderabbitai help to get the list of available commands and usage tips.

@claude
Copy link
Copy Markdown

claude Bot commented Apr 19, 2026

Code Review: make-plan-v2 and do-v2 Skills

Overview

This PR adds two v2 skill variants alongside the originals, introducing five enhancements: YAML frontmatter in plan files, task sizing constraints, a 4-status subagent protocol, a human gate before commits, and must-be-true conditions replacing the checklist approach. The additive strategy (no changes to v1) is the right call for side-by-side evaluation.


What Works Well

  • 4-status protocol (DONE / DONE_WITH_CONCERNS / NEEDS_CONTEXT / BLOCKED) is a meaningful upgrade. The NEEDS_CONTEXT retry-once-then-escalate rule is particularly well-specified.
  • Must-be-true conditions + executable verification commands is a strong semantic improvement over the v1 "verification checklist." Framing completion as observable outcomes rather than task completion prevents the common failure mode of "file was created" counting as success.
  • Human gate before commit fills an important gap in v1 — agents could commit without user visibility into what changed.
  • Plan staleness check in do-v2 is a good defensive move to catch plan/branch drift before execution begins.
  • plan.md to .gitignore is a sensible default; plan files are ephemeral work artifacts, not project history.

Issues and Suggestions

1. Staleness check may false-positive during execution (do-v2)

The staleness check compares commit_sha frontmatter against current HEAD. But do-v2 itself commits after each verified phase — so HEAD advances legitimately during execution. This means the check will warn on every phase after the first commit.

Suggestion: Narrow the staleness check to branch mismatch only, or document that commit_sha reflects the SHA before execution began and is expected to drift.

2. Self-contained phase guidance is misplaced (make-plan-v2, line ~43)

"Each phase must be self-contained enough that a fresh subagent with only the plan file and the codebase can execute it without needing context from prior phases."

This sentence appears inside the Phase 0 section but applies to all phases. A reader scanning the Phase 0 instructions could miss it entirely.

Suggestion: Move it to the intro of ### Each Implementation Phase Must Include.

3. Status protocol is duplicated across both files

Both make-plan-v2 and do-v2 define the 4-status protocol identically. If statuses evolve, both files need synchronized updates — and they'll drift.

Suggestion: Extract to a shared plugin/skills/_shared/subagent-protocol.md and @include it (if the skill runtime supports includes), or at minimum add a note pointing from one to the other.

4. current_phase update semantics are underspecified (do-v2)

"Set current_phase to the next phase number"

Phase 0 is discovery; Phase 1 is the first implementation phase. It's not obvious whether current_phase is the phase just completed or the phase about to run, and how Phase 0 is handled (does it start at 0 and increment, or start at 1?).

Suggestion: Add a one-liner clarifying: "current_phase reflects the phase currently executing (or complete when all phases finish).", and show a before/after example in the frontmatter block.

5. .gitignore instruction missing from do-v2

make-plan-v2 instructs adding plan.md to .gitignore. do-v2 reads plan.md but has no corresponding note. If someone runs do-v2 without make-plan-v2, or the gitignore step was missed, they may accidentally commit a plan file.

Suggestion: Add a brief check in do-v2's plan-reading section: "If plan.md is not listed in .gitignore, add it before proceeding."

6. YAML frontmatter example date is anachronistic

The example uses date: 2026-03-28, which is in the past relative to today (2026-04-19). Minor, but can be confusing.

Suggestion: Update to a current or future example date, or make it abstract (date: <today>).

7. No guidance on what happens when plan file write fails mid-execution (do-v2)

make-plan-v2 says "If the plan file cannot be written, tell the user and stop." do-v2 has no equivalent for mid-execution write failures (updating checkboxes, current_phase, etc.).

Suggestion: Add: "If the plan file cannot be updated after a phase, warn the user but do not proceed to the next phase until the file is writable."


Minor Nits

  • do-v2 says "If the plan file cannot be read or is missing, tell the user to run make-plan-v2 first and stop" — should also handle the case where plan.md exists but has no valid YAML frontmatter (i.e., it's a v1 plan).
  • The PR description mentions "goal-backward verification" but neither SKILL.md uses that term — the must-be-true conditions are the mechanism, but the framing is absent. Consider adding a one-sentence definition in make-plan-v2 so future readers understand the intent.

Summary

The v2 enhancements are well-reasoned and address real failure modes in the v1 skills. The staleness check false-positive and the current_phase ambiguity are the most important issues to address before promoting v2 to replace v1. The self-contained phase guidance placement and the duplicated status protocol are worth cleaning up but won't block evaluation.

Reviewed by Claude Sonnet 4.6

@greptile-apps
Copy link
Copy Markdown
Contributor

greptile-apps Bot commented Apr 19, 2026

Greptile Summary

This PR adds make-plan-v2 and do-v2 as additive skill variants alongside the originals, introducing YAML frontmatter for plan files, a 4-status subagent reporting protocol, goal-backward must-be-true verification, and a human gate before each commit. The existing make-plan/do skills are untouched.

  • Both v2 skills write to the same plan.md filename as v1, risking silent overwriting if both skill versions are used on the same project — consider a distinct filename or an existence check.
  • The PR description lists "task sizing constraints" as one of the five enhancements, but no such guidance appears in either SKILL.md file.

Confidence Score: 5/5

Safe to merge — purely additive skill files with no changes to existing skills and no runtime code.

All findings are P2: a shared filename that could cause collisions only if both skill generations are used on the same project, a missing task-sizing section claimed in the PR description, and a silent no-op in the staleness check for v1 plans. None block the primary use-case and the originals are fully intact.

No files require special attention; the plan.md filename collision in make-plan-v2/SKILL.md is worth addressing before the v2 skills are promoted to replace v1.

Important Files Changed

Filename Overview
plugin/skills/make-plan-v2/SKILL.md New skill adding YAML frontmatter to plan files, the 4-status subagent protocol, goal-backward must-be-true conditions, and executable verification commands; minor concern around shared plan.md filename with v1 and missing task-sizing guidance claimed in PR description.
plugin/skills/do-v2/SKILL.md New execution skill adding plan-file staleness checks, 4-status subagent protocol, and a human gate before each commit; staleness check silently no-ops when YAML frontmatter is absent (v1 plans).

Sequence Diagram

sequenceDiagram
    participant User
    participant Orchestrator
    participant DocSubagent as Documentation Subagent
    participant ImplSubagent as Implementation Subagent
    participant VerifySubagent as Verification Subagent
    participant CommitSubagent as Commit Subagent

    User->>Orchestrator: /make-plan-v2
    Orchestrator->>DocSubagent: Phase 0 – discover docs & APIs
    DocSubagent-->>Orchestrator: DONE (Allowed APIs list)
    Orchestrator->>Orchestrator: Author plan.md with YAML frontmatter
    Orchestrator-->>User: plan.md written

    User->>Orchestrator: /do-v2
    Orchestrator->>Orchestrator: Read plan.md, check branch/commit_sha staleness
    loop Each Phase
        Orchestrator->>ImplSubagent: Implement phase (copy from docs)
        ImplSubagent-->>Orchestrator: DONE | DONE_WITH_CONCERNS | NEEDS_CONTEXT | BLOCKED
        Orchestrator->>VerifySubagent: Check must-be-true conditions
        VerifySubagent-->>Orchestrator: DONE | BLOCKED
        Orchestrator-->>User: Human gate – show changes, confirm commit?
        User-->>Orchestrator: Confirmed
        Orchestrator->>CommitSubagent: Commit & push
        CommitSubagent-->>Orchestrator: DONE
        Orchestrator->>Orchestrator: Update plan.md (current_phase, checkboxes)
    end
    Orchestrator-->>User: overall_status: complete
Loading

Fix All in Claude Code

Reviews (1): Last reviewed commit: "feat: add make-plan-v2 and do-v2 skills ..." | Re-trigger Greptile


## Plan Output Format

Write the plan to `plan.md` in the project root. Add `plan.md` to `.gitignore` if not already present.
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 plan.md filename collision with original make-plan

Both make-plan and make-plan-v2 write to the same plan.md in the project root. If a developer runs make-plan to create a v1 plan and then later runs make-plan-v2 for a different task (or vice versa), the second invocation will silently overwrite the first plan. Since do-v2 relies on the YAML frontmatter that only make-plan-v2 produces, running do-v2 against an overwritten v1 plan will hit the staleness check immediately with no frontmatter to compare against. Consider writing to a distinct filename (e.g. plan-v2.md) or prompting the user if a plan.md already exists.

Fix in Claude Code

Comment on lines +43 to +51
### Each Implementation Phase Must Include

1. **What to implement** — Frame tasks to COPY from docs, not transform existing code
- Good: "Copy the V2 session pattern from docs/examples.ts:45-60"
- Bad: "Migrate the existing code to V2"
2. **Documentation references** — Cite specific files/lines for patterns to follow
3. **Must-be-true conditions** — What observable conditions must be true for this phase to be considered complete? Frame as outcomes ("the API returns 200 on POST /foo"), not tasks ("the route file was created").
4. **Executable verification commands** — Specific commands to run that prove the must-be-true conditions hold (tests, curl commands, grep checks).
5. **Anti-pattern guards** — What NOT to do (invented APIs, undocumented params)
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Task sizing constraints not reflected in skill body

The PR description lists "task sizing constraints" as one of the five headline enhancements, but neither make-plan-v2 nor do-v2 contains any guidance on task size limits (e.g., max lines changed per task, time budget, or context-window fit). If task sizing was intentionally deferred, the PR description and test plan checklist should be updated to reflect that; if it was meant to be included, a guideline like "each task should be small enough to complete in a single subagent context" belongs in the Each Implementation Phase Must Include section here.

Fix in Claude Code

Comment on lines +13 to +14
- If the plan file cannot be read or is missing, tell the user to run make-plan-v2 first and stop.
- Check staleness: compare the plan's `branch` and `commit_sha` frontmatter against the current git branch and HEAD. If they differ, warn the user before proceeding.
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Staleness check silently no-ops on v1 plans

When do-v2 is invoked against a plan file produced by the original make-plan (which has no YAML frontmatter), the branch and commit_sha fields will be absent. The instruction only says "warn the user before proceeding" when the values differ, but it does not specify what to do when the fields are missing entirely. In practice the staleness check will be skipped silently, giving the user no indication that the plan is a v1 format. Consider adding an explicit check: if frontmatter fields are absent, surface a warning that this plan may not be a make-plan-v2 plan and prompt the user to confirm.

Fix in Claude Code

Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 5

🧹 Nitpick comments (2)
plugin/skills/make-plan-v2/SKILL.md (1)

31-41: Consider emphasizing the self-contained phase requirement.

Line 41 states a critical constraint: "Each phase must be self-contained enough that a fresh subagent with only the plan file and the codebase can execute it without needing context from prior phases."

This is foundational to the v2 execution model but appears as the last sentence in the Phase 0 section. Since it applies to all phases (not just Phase 0), consider moving this to a more prominent location or reformatting as a callout.

💡 Suggested restructuring

Add a subsection or callout before "Phase 0":

### Phase 0: Documentation Discovery (ALWAYS FIRST)

> **Key Constraint**: Each phase must be self-contained enough that a fresh subagent with only the plan file and the codebase can execute it without needing context from prior phases. This enables execution in new chat contexts.

Before planning implementation, deploy "Documentation Discovery" subagents to:
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@plugin/skills/make-plan-v2/SKILL.md` around lines 31 - 41, The Phase 0
section puts the "self-contained phase" constraint at the end but it applies to
all phases; move or duplicate that sentence into a prominent callout before
Phase 0 (or create a new "Key Constraint" subsection) so it's visible up-front;
update SKILL.md's Phase 0 header or insert a highlighted note that reads the
exact constraint about a fresh subagent being able to execute a phase with only
the plan file and codebase, and remove or keep the trailing sentence to avoid
duplication.
plugin/skills/do-v2/SKILL.md (1)

42-42: Specify the format for showing changes in the human gate.

The human gate instruction says "briefly show the user what changed in this phase (files modified, key changes)" but doesn't specify the format. Different implementations might choose:

  • git status or git diff --stat (summary)
  • git diff (full diff)
  • A narrative summary of changes
  • A list of modified files with descriptions

Specifying the format would improve consistency and user experience.

💬 Suggested clarification
-4. **Human gate** — Before deploying the Commit subagent, briefly show the user what changed in this phase (files modified, key changes) and confirm they want to commit
+4. **Human gate** — Before deploying the Commit subagent, show the user:
+   - Run `git status` to show modified files
+   - Run `git diff --stat` to show change summary
+   - Provide a brief narrative of key changes (2-3 sentences)
+   - Ask for explicit confirmation before proceeding to commit
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@plugin/skills/do-v2/SKILL.md` at line 42, The human gate step for the Commit
subagent lacks a concrete output format; update the SKILL.md "Human gate"
section to specify the exact format to present changes (e.g., a concise header,
a list of modified files with path and brief one-line description, a git diff
--stat-style summary, and an optional short excerpt or patch link for each file)
so implementations of the "Human gate" in the Commit subagent produce a
consistent summary; reference the "Human gate" and "Commit subagent" text and
require the format include: (1) file list with one-line descriptions, (2)
diffstat summary, and (3) optional short snippets or links for full diffs.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@plugin/skills/do-v2/SKILL.md`:
- Around line 15-18: The SKILL.md section that tells users to update plan.md
after each phase lacks guidance for write failures; update the documentation
around the plan file update steps (references: plan.md, current_phase,
overall_status) to specify a clear error-handling policy: attempt a configurable
number of retries (e.g., 3) with exponential backoff on transient errors, log a
clear error including the file path and error details if a write fails, and on
persistent failure either (a) halt further execution and mark the plan/update as
failed (recommended) or (b) provide an explicit optional flag to continue with a
warning—describe the default behavior, retry parameters, and required manual
remediation steps so implementers know how to handle permission/disk/lock
errors.
- Around line 47-48: The doc instructs the "Branch/Sync" subagent to "Push to
working branch after each verified phase" but lacks error-handling guidance;
update SKILL.md to specify an explicit policy for git push failures: detect
common failure classes (network, permission, merge conflict), implement a retry
strategy with exponential backoff for transient errors, abort the current phase
and surface an error (stop execution) for permission/auth errors, and on merge
conflicts require human intervention (mark as failed, create issue/notification)
rather than continuing; document the expected behavior of the "Branch/Sync"
subagent on each failure type, the retry limit, and where it should log/notify
(e.g., CI logs and issue tracker).
- Around line 12-14: Update the staleness check guidance to handle missing
frontmatter and git errors: when comparing plan.md frontmatter fields "branch"
and "commit_sha" in the SKILL.md staleness step, specify that if either field is
missing treat the plan as "unknown/stale" and warn the user to run make-plan-v2
(or re-generate the plan) before proceeding; likewise instruct to run git
commands (git rev-parse --abbrev-ref HEAD and git rev-parse HEAD) in a try/catch
manner and, on failure (e.g., not a git repo or git error), surface a clear
error telling the user to run the skill from a git repository or provide
branch/commit info, and abort/ask for confirmation instead of proceeding
silently.

In `@plugin/skills/make-plan-v2/SKILL.md`:
- Line 61: Clarify and harden the ".gitignore" update: when writing the plan to
plan.md in the project root, first ensure the project root file plan.md is
created/overwritten as intended, then handle .gitignore by (1) creating
.gitignore if it does not exist, (2) reading existing lines and ignoring blank
lines and comments, (3) checking whether plan.md is already ignored either by an
exact entry "plan.md" or by a broader pattern that would match it (e.g., "*.md",
"/*.md", "docs/*.md", or path entries that match "plan.md"), and only append the
literal "plan.md" entry if no existing line or pattern would already ignore it;
avoid adding duplicate entries and trim whitespace when comparing.
- Around line 63-72: Update the SKILL.md frontmatter example to instruct the
orchestrator/LLM to populate git-dependent fields by reading the repo state: set
branch to the current branch name (e.g., from git rev-parse --abbrev-ref HEAD)
and commit_sha to the current commit hash (e.g., from git rev-parse HEAD), and
ensure these values are inserted into the plan frontmatter for staleness checks
(fields "branch" and "commit_sha"); also clarify the initial "current_phase"
value (use 0 for Phase 0: Documentation Discovery as the default for newly
created plans, and describe when/ how to increment it to 1+ for implementation
phases).

---

Nitpick comments:
In `@plugin/skills/do-v2/SKILL.md`:
- Line 42: The human gate step for the Commit subagent lacks a concrete output
format; update the SKILL.md "Human gate" section to specify the exact format to
present changes (e.g., a concise header, a list of modified files with path and
brief one-line description, a git diff --stat-style summary, and an optional
short excerpt or patch link for each file) so implementations of the "Human
gate" in the Commit subagent produce a consistent summary; reference the "Human
gate" and "Commit subagent" text and require the format include: (1) file list
with one-line descriptions, (2) diffstat summary, and (3) optional short
snippets or links for full diffs.

In `@plugin/skills/make-plan-v2/SKILL.md`:
- Around line 31-41: The Phase 0 section puts the "self-contained phase"
constraint at the end but it applies to all phases; move or duplicate that
sentence into a prominent callout before Phase 0 (or create a new "Key
Constraint" subsection) so it's visible up-front; update SKILL.md's Phase 0
header or insert a highlighted note that reads the exact constraint about a
fresh subagent being able to execute a phase with only the plan file and
codebase, and remove or keep the trailing sentence to avoid duplication.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: d359ff5d-a9fe-4096-87f1-ecd0cd09c7f7

📥 Commits

Reviewing files that changed from the base of the PR and between beea789 and 2d938c5.

📒 Files selected for processing (2)
  • plugin/skills/do-v2/SKILL.md
  • plugin/skills/make-plan-v2/SKILL.md

Comment on lines +12 to +14
- Read the plan from `plan.md` in the project root at start.
- If the plan file cannot be read or is missing, tell the user to run make-plan-v2 first and stop.
- Check staleness: compare the plan's `branch` and `commit_sha` frontmatter against the current git branch and HEAD. If they differ, warn the user before proceeding.
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

Specify handling for missing staleness fields.

Line 14 instructs checking staleness by comparing branch and commit_sha frontmatter against current git state, but doesn't specify what to do if these fields are missing from the frontmatter. Given that make-plan-v2/SKILL.md doesn't explicitly instruct populating these fields (see my comment on that file), this is a likely scenario.

Additionally, no guidance is provided for handling git command failures (e.g., when executed in a non-git directory).

🔍 Suggested addition after line 13
 - If the plan file cannot be read or is missing, tell the user to run make-plan-v2 first and stop.
-- Check staleness: compare the plan's `branch` and `commit_sha` frontmatter against the current git branch and HEAD. If they differ, warn the user before proceeding.
+- Check staleness (if git is available and frontmatter includes `branch` and `commit_sha`):
+  - Run `git branch --show-current` and `git rev-parse HEAD`
+  - If these differ from the frontmatter values, warn the user that the plan may be stale before proceeding
+  - If `branch` or `commit_sha` fields are missing from frontmatter, warn the user that staleness cannot be verified
+  - If git commands fail (e.g., not a git repository), skip staleness check and log a warning
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
- Read the plan from `plan.md` in the project root at start.
- If the plan file cannot be read or is missing, tell the user to run make-plan-v2 first and stop.
- Check staleness: compare the plan's `branch` and `commit_sha` frontmatter against the current git branch and HEAD. If they differ, warn the user before proceeding.
- Read the plan from `plan.md` in the project root at start.
- If the plan file cannot be read or is missing, tell the user to run make-plan-v2 first and stop.
- Check staleness (if git is available and frontmatter includes `branch` and `commit_sha`):
- Run `git branch --show-current` and `git rev-parse HEAD`
- If these differ from the frontmatter values, warn the user that the plan may be stale before proceeding
- If `branch` or `commit_sha` fields are missing from frontmatter, warn the user that staleness cannot be verified
- If git commands fail (e.g., not a git repository), skip staleness check and log a warning
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@plugin/skills/do-v2/SKILL.md` around lines 12 - 14, Update the staleness
check guidance to handle missing frontmatter and git errors: when comparing
plan.md frontmatter fields "branch" and "commit_sha" in the SKILL.md staleness
step, specify that if either field is missing treat the plan as "unknown/stale"
and warn the user to run make-plan-v2 (or re-generate the plan) before
proceeding; likewise instruct to run git commands (git rev-parse --abbrev-ref
HEAD and git rev-parse HEAD) in a try/catch manner and, on failure (e.g., not a
git repo or git error), surface a clear error telling the user to run the skill
from a git repository or provide branch/commit info, and abort/ask for
confirmation instead of proceeding silently.

Comment on lines +15 to +18
- After each phase, update the plan file:
- Set `current_phase` to the next phase number
- Set `overall_status` to `in-progress` (or `complete` when all phases are done)
- Check off completed tasks (`- [ ]` to `- [x]`)
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor

Specify error handling for plan file updates.

This section instructs updating plan.md after each phase but doesn't specify what to do if the file write fails (e.g., due to permissions, disk space, or file locks). Should execution stop, continue with a warning, or retry?

📝 Suggested addition
 - After each phase, update the plan file:
   - Set `current_phase` to the next phase number
   - Set `overall_status` to `in-progress` (or `complete` when all phases are done)
   - Check off completed tasks (`- [ ]` to `- [x]`)
+  - If the plan file cannot be written, warn the user but continue execution (plan state will be stale)
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
- After each phase, update the plan file:
- Set `current_phase` to the next phase number
- Set `overall_status` to `in-progress` (or `complete` when all phases are done)
- Check off completed tasks (`- [ ]` to `- [x]`)
- After each phase, update the plan file:
- Set `current_phase` to the next phase number
- Set `overall_status` to `in-progress` (or `complete` when all phases are done)
- Check off completed tasks (`- [ ]` to `- [x]`)
- If the plan file cannot be written, warn the user but continue execution (plan state will be stale)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@plugin/skills/do-v2/SKILL.md` around lines 15 - 18, The SKILL.md section that
tells users to update plan.md after each phase lacks guidance for write
failures; update the documentation around the plan file update steps
(references: plan.md, current_phase, overall_status) to specify a clear
error-handling policy: attempt a configurable number of retries (e.g., 3) with
exponential backoff on transient errors, log a clear error including the file
path and error details if a write fails, and on persistent failure either (a)
halt further execution and mark the plan/update as failed (recommended) or (b)
provide an explicit optional flag to continue with a warning—describe the
default behavior, retry parameters, and required manual remediation steps so
implementers know how to handle permission/disk/lock errors.

Comment on lines +47 to +48
Deploy a "Branch/Sync" subagent to:
- Push to working branch after each verified phase
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor

Specify error handling for git push failures.

Line 48 instructs pushing to the working branch but doesn't specify what to do if the push fails (e.g., network errors, permission issues, or merge conflicts). Should execution stop, continue with a warning, or retry?

🔄 Suggested addition
 Deploy a "Branch/Sync" subagent to:
-- Push to working branch after each verified phase
+- Push to working branch after each verified phase:
+  - If push fails, report DONE_WITH_CONCERNS and show the error to the user
+  - User decides whether to retry, continue without pushing, or stop
 - Prepare the next phase handoff so the next phase's subagents start fresh but have plan context
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
Deploy a "Branch/Sync" subagent to:
- Push to working branch after each verified phase
Deploy a "Branch/Sync" subagent to:
- Push to working branch after each verified phase:
- If push fails, report DONE_WITH_CONCERNS and show the error to the user
- User decides whether to retry, continue without pushing, or stop
- Prepare the next phase handoff so the next phase's subagents start fresh but have plan context
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@plugin/skills/do-v2/SKILL.md` around lines 47 - 48, The doc instructs the
"Branch/Sync" subagent to "Push to working branch after each verified phase" but
lacks error-handling guidance; update SKILL.md to specify an explicit policy for
git push failures: detect common failure classes (network, permission, merge
conflict), implement a retry strategy with exponential backoff for transient
errors, abort the current phase and surface an error (stop execution) for
permission/auth errors, and on merge conflicts require human intervention (mark
as failed, create issue/notification) rather than continuing; document the
expected behavior of the "Branch/Sync" subagent on each failure type, the retry
limit, and where it should log/notify (e.g., CI logs and issue tracker).


## Plan Output Format

Write the plan to `plan.md` in the project root. Add `plan.md` to `.gitignore` if not already present.
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor

Clarify the .gitignore update procedure.

The instruction "Add plan.md to .gitignore if not already present" is ambiguous. It doesn't specify:

  • How to check if the entry already exists (exact match? pattern match?)
  • What to do if .gitignore doesn't exist (create it?)
  • How to handle existing patterns that might cover plan.md (e.g., *.md)

This could lead to duplicate entries, failed file operations, or inconsistent behavior.

📋 Suggested clarification
-Write the plan to `plan.md` in the project root. Add `plan.md` to `.gitignore` if not already present.
+Write the plan to `plan.md` in the project root. After writing the plan, check if `.gitignore` exists in the project root. If it exists, check whether it contains a line matching `plan.md` (exact match or pattern like `/plan.md` or `*.md`). If no such entry exists, append a new line `plan.md` to `.gitignore`. If `.gitignore` doesn't exist, create it with the single entry `plan.md`.
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
Write the plan to `plan.md` in the project root. Add `plan.md` to `.gitignore` if not already present.
Write the plan to `plan.md` in the project root. After writing the plan, check if `.gitignore` exists in the project root. If it exists, check whether it contains a line matching `plan.md` (exact match or pattern like `/plan.md` or `*.md`). If no such entry exists, append a new line `plan.md` to `.gitignore`. If `.gitignore` doesn't exist, create it with the single entry `plan.md`.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@plugin/skills/make-plan-v2/SKILL.md` at line 61, Clarify and harden the
".gitignore" update: when writing the plan to plan.md in the project root, first
ensure the project root file plan.md is created/overwritten as intended, then
handle .gitignore by (1) creating .gitignore if it does not exist, (2) reading
existing lines and ignoring blank lines and comments, (3) checking whether
plan.md is already ignored either by an exact entry "plan.md" or by a broader
pattern that would match it (e.g., "*.md", "/*.md", "docs/*.md", or path entries
that match "plan.md"), and only append the literal "plan.md" entry if no
existing line or pattern would already ignore it; avoid adding duplicate entries
and trim whitespace when comparing.

Comment on lines +63 to +72
The plan file must start with YAML frontmatter:

---
name: add-user-auth
date: 2026-03-28
branch: feature/user-auth
commit_sha: abc1234
current_phase: 1
overall_status: in-progress
---
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

Add instructions for populating git-dependent frontmatter fields.

The frontmatter example includes branch and commit_sha fields that do-v2 uses for staleness checking (line 14 in do-v2/SKILL.md), but this section doesn't instruct the orchestrator to populate these fields from the current git state. Without explicit instructions, an LLM might use placeholder values or omit them, breaking the staleness check feature.

Additionally, current_phase: 1 lacks guidance on the correct initial value—should it be 0 (for Phase 0: Documentation Discovery) or 1 (for the first implementation phase)?

📝 Suggested addition after line 62
 Write the plan to `plan.md` in the project root. Add `plan.md` to `.gitignore` if not already present.
 
+Populate the frontmatter fields as follows:
+- `name`: A short kebab-case identifier for the plan (e.g., `add-user-auth`)
+- `date`: Current date in YYYY-MM-DD format
+- `branch`: Current git branch name (run `git branch --show-current`)
+- `commit_sha`: Current git HEAD commit (run `git rev-parse HEAD`)
+- `current_phase`: Set to `0` initially (Phase 0: Documentation Discovery)
+- `overall_status`: Set to `not-started` initially
+
 The plan file must start with YAML frontmatter:
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@plugin/skills/make-plan-v2/SKILL.md` around lines 63 - 72, Update the
SKILL.md frontmatter example to instruct the orchestrator/LLM to populate
git-dependent fields by reading the repo state: set branch to the current branch
name (e.g., from git rev-parse --abbrev-ref HEAD) and commit_sha to the current
commit hash (e.g., from git rev-parse HEAD), and ensure these values are
inserted into the plan frontmatter for staleness checks (fields "branch" and
"commit_sha"); also clarify the initial "current_phase" value (use 0 for Phase
0: Documentation Discovery as the default for newly created plans, and describe
when/ how to increment it to 1+ for implementation phases).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant