fix(skills): restore worktree step in brainstorming workflow#675
fix(skills): restore worktree step in brainstorming workflow#675qishaoyumu wants to merge 1 commit intoobra:mainfrom
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThe PR inserts a mandatory "Set up worktree" step into the brainstorming flow after design-doc creation and before writing-plans, updates the process graph and terminal-state wording, and adds a "Workspace isolation" section explaining git worktree usage. Integration docs for using-git-worktrees are adjusted to this new positioning. Changes
Sequence Diagram(s)sequenceDiagram
participant User as User
participant Brain as Brainstorming Skill
participant WG as using-git-worktrees Skill
participant WP as writing-plans Skill
User->>Brain: Start brainstorming / write design doc
Brain->>User: Ask "User reviews spec?"
alt User approves
Brain->>WG: Set up worktree (using-git-worktrees)
WG-->>Brain: Worktree created
Brain->>WP: Invoke writing-plans (in worktree)
WP-->>Brain: Plans written (isolated)
else User requests changes
Brain->>User: Continue brainstorming
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
skills/brainstorming/SKILL.md (1)
62-62: Approve with optional wording refinement suggestion.The guidance correctly establishes the terminal state and skill invocation boundaries. The intent is clear: after design approval, only using-git-worktrees and writing-plans should be invoked, with no other implementation skills allowed.
📝 Optional wording refinement for precision
The phrase "after brainstorming" might be slightly ambiguous since using-git-worktrees is step 7 of the brainstorming checklist. Consider:
-**The terminal state is invoking writing-plans.** Do NOT invoke frontend-design, mcp-builder, or any other implementation skill. The ONLY skills you invoke after brainstorming are using-git-worktrees (for workspace isolation) then writing-plans. +**The terminal state is invoking writing-plans.** Do NOT invoke frontend-design, mcp-builder, or any other implementation skill. The ONLY skills you invoke from brainstorming are using-git-worktrees (for workspace isolation) then writing-plans.This clarifies that these skills are invoked as part of the brainstorming workflow, not after it completes.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@skills/brainstorming/SKILL.md` at line 62, Update the wording that currently reads "The terminal state is invoking writing-plans. Do NOT invoke frontend-design, mcp-builder, or any other implementation skill. The ONLY skills you invoke after brainstorming are using-git-worktrees (for workspace isolation) then writing-plans." to remove ambiguity around "after brainstorming": explicitly state that within the brainstorming workflow (specifically step 7 of the brainstorming checklist) the only allowed follow-up skills are using-git-worktrees and then writing-plans, and that no other implementation skills (e.g., frontend-design, mcp-builder) should be invoked at that point; reference the exact phrase "The terminal state is invoking writing-plans." and the skill names "using-git-worktrees" and "writing-plans" so reviewers can locate and replace the sentence with a clearer version indicating these skills are invoked as part of the brainstorming steps rather than only after completion.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@skills/brainstorming/SKILL.md`:
- Line 62: Update the wording that currently reads "The terminal state is
invoking writing-plans. Do NOT invoke frontend-design, mcp-builder, or any other
implementation skill. The ONLY skills you invoke after brainstorming are
using-git-worktrees (for workspace isolation) then writing-plans." to remove
ambiguity around "after brainstorming": explicitly state that within the
brainstorming workflow (specifically step 7 of the brainstorming checklist) the
only allowed follow-up skills are using-git-worktrees and then writing-plans,
and that no other implementation skills (e.g., frontend-design, mcp-builder)
should be invoked at that point; reference the exact phrase "The terminal state
is invoking writing-plans." and the skill names "using-git-worktrees" and
"writing-plans" so reviewers can locate and replace the sentence with a clearer
version indicating these skills are invoked as part of the brainstorming steps
rather than only after completion.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 08bbb055-9fd5-4c70-878c-f1ef2971caca
📒 Files selected for processing (2)
skills/brainstorming/SKILL.mdskills/using-git-worktrees/SKILL.md
|
Re: CodeRabbit nitpick on "after brainstorming" → "from brainstorming" Thanks for the suggestion. I considered this but decided to keep "after brainstorming" because:
Happy to revisit if the maintainer prefers different wording. |
The brainstorming skill never invoked using-git-worktrees despite writing-plans expecting to run in a worktree "created by brainstorming skill" and using-git-worktrees listing brainstorming as a caller. This was unintentionally dropped during workflow simplification (8e38ab8, 7f2ee61) and remained missing after v5.0.0 rework. Changes to skills/brainstorming/SKILL.md: - Checklist: insert step 7 (Set up worktree) between design doc and writing-plans - Process flow diagram: add worktree node with correct edges - Terminal state note: update skill whitelist to include using-git-worktrees - After the Design: add Workspace isolation section, adjust Implementation prohibition from "any other skill" to "any implementation skill (frontend-design, mcp-builder, etc.)" Changes to skills/using-git-worktrees/SKILL.md: - Integration: fix stale "Phase 4" reference to "Step 7" Closes obra#574 Ref obra#186
5d2079a to
6079cd2
Compare
|
Rebased onto latest main to resolve conflicts. The upstream added spec review loop (steps 7-8) since this PR was opened. Updated this PR to insert the worktree step as step 9 (after spec review and user review gate), and updated the cross-reference in using-git-worktrees/SKILL.md accordingly. |
Summary
using-git-worktreesinvocation lost during brainstorming skillsimplification (8e38ab8, 7f2ee61) and still missing after the v5.0.0 rework
using-git-worktreesIntegration sectionwriting-plansexpectation that brainstormingcreates a worktree (line 16)
Problem
brainstormingnever invokesusing-git-worktrees, despitewriting-plansexpecting it (line 16: "This should be run in a dedicated worktree (created by
brainstorming skill).") and
using-git-worktreesdocumenting it (line 212:"Called by: brainstorming (Phase 4)").
In practice, after brainstorming completes,
writing-plansruns directly inthe main working tree — no workspace isolation occurs.
Changes
skills/brainstorming/SKILL.md(4 edits):and Transition to implementation
using-git-worktreesLoop and Implementation; adjust Implementation prohibition from
"any other skill" to "any implementation skill (frontend-design,
mcp-builder, etc.)" so
using-git-worktreesis not blockedskills/using-git-worktrees/SKILL.md(1 edit):Context
This is a re-implementation of #483 against the v5.0.0 codebase. The original
PR was closed as stale after v5.0.0 substantially reworked the brainstorming
skill (Visual Companion, Scope Assessment, Spec Review Loop). The core intent
is the same — restore the missing worktree step — but adapted to v5.0.0's
expanded checklist (step 7 instead of 6) and refined prohibition wording. All
v5.0.0 features are unaffected.
Related: #483 (original PR, closed as stale)
Closes #574
Ref #186
Test Plan
checklist as step 7
using-git-worktreesis invoked beforewriting-plansusing-git-worktreesIntegration section references "Step 7"work normally