Complete record-plan lock file enforcement implementation #169
Conversation
- Tested with RED-GREEN-REFACTOR using 4 pressure scenarios - Scenarios validate mechanical vs judgment decision-making - All tests pass: agents correctly identify automation opportunities - Word count optimized to 424 words (target: ≤500) - Integration points added to SKILL.md line 500 Test results: 4/4 scenarios pass (GREEN phase: first try) Test methodology: Baseline without doc showed documentation failures, tested version correctly guided agents to mechanical enforcement
- Use emoji library for production-ready approach - Include commented regex alternative for zero dependencies - Addresses CodeRabbit feedback: code examples must be runnable - Word count: 442 words (still under 500 target)
Add brief example showing automation-over-documentation applies to silent dependency failures, not just rationalization. Based on production incident where file-track registration was gated by Python module import, causing silent failures in subagent environments. Pattern: Remove dependency gates that hide failures - CLI registration should be independent of Python module availability.
Removes .claude/settings.local.json from git tracking and adds it to .gitignore to prevent committing user-specific paths and permissions. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
Updates all skill and documentation references from the deprecated writing-plans to the new record-plans skill name following the rename in commit 8853d53. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
Following TDD methodology for skills: - RED: Baseline tests showed agents skip ws consultation when modifying skills - GREEN: Enhanced triggers and added explicit enforcement blocks - REFACTOR: Extended rationalization tables with skill-modification scenarios Changes: - writing-skills: Enhanced description with explicit triggers (modifying, debugging, adding sections) - writing-skills: Added EXTREMELY-IMPORTANT block requiring consultation before any skill file edits - writing-skills: Extended rationalization table with 3 new skill-modification entries - using-superpowers: Added specific rationalization "Just editing a skill file" - CLAUDE.md: Added local plugin development workflow Testing revealed systemic limitation: subagents don't auto-invoke MANDATORY FIRST RESPONSE PROTOCOL. Content improvements help when agents do consult the skill. Co-Authored-By: Claude <noreply@anthropic.com>
…ecution Changes: - Execute all tasks continuously instead of stopping every 3 tasks - Only stop for actual blockers or ambiguous instructions - Add "Common Rationalizations" section to prevent early stopping - Update description, overview, and all process steps for consistency - Remove "batch" terminology throughout Testing: - Baseline: confirmed agent stopped after 3/6 tasks - Green: verified agent executes all 6/6 tasks continuously - Pressure: verified agent executes 10/10 tasks through milestone Addresses user feedback that checkpoint pausing interrupts flow when there are no blockers requiring input.
Wrapper script lives in user's .claude/scripts directory, not in skill directory. Changes: - write_plan.py path: ~/.claude/scripts/record-plan/ - check_lock.py path: ~/.claude/scripts/record-plan/ This separates skill documentation (in plugin) from user scripts (in ~/.claude).
CLAUDE.md changes: - Add blank lines for better markdown list formatting - Simplify plugin reload instructions Scripts added: - scripts/reload-plugin.sh: Helper to display plugin reload commands
WalkthroughThis PR refactors the Claude Code skill system, deprecating the Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~35 minutes
Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Nitpick comments (11)
skills/writing-skills/SKILL.md (2)
3-3: Description expansion is consistent, consider brevity as an optional tweakThe expanded description clearly encodes triggers and purpose and fits the YAML constraints. Given the later guidance about keeping frequently-loaded skill descriptions tight, you might optionally trim a bit of redundancy (e.g., some of the TDD explanation now also covered below) to save tokens, but it’s not strictly necessary.
518-519: Good deep-dive pointer; consider matching cross-reference styleThe deep-dive reference to
automation-over-documentation.mdis a useful bridge to the newer automation-oriented workflow (e.g., record-plan tooling) and fits the surrounding narrative. If you want tighter consistency, you could mirror the “Official guidance” phrasing from Line 20 (briefly stating what the doc covers), but the current form is clear enough.skills/writing-skills/automation-over-documentation.md (2)
20-30: Clarifyemojidependency and regex limitations in the example codeThe example is useful, but a couple of clarifications would reduce surprises for readers implementing it:
emoji.replace_emojiis only available in newer versions of theemojipackage; older environments may not have this API.- The regex fallback only covers a subset of emoji codepoint ranges (no flags, skin tones, ZWJ sequences, etc.), so it is illustrative rather than exhaustive.
Consider updating the snippet and surrounding text to make these constraints explicit, for example:
-import emoji +import emoji # pip install 'emoji>=2.2.0' (for replace_emoji) @@ -# Or using regex for zero dependencies: -# import re -# emoji_pattern = re.compile(r'[\U0001F600-\U0001F64F\U0001F300-\U0001F5FF\U0001F680-\U0001F6FF]+') -# return emoji_pattern.sub('', text) +# Or using a simple regex for zero dependencies (covers common emoji blocks, not exhaustive): +# import re +# emoji_pattern = re.compile(r'[\U0001F600-\U0001F64F\U0001F300-\U0001F5FF\U0001F680-\U0001F6FF]+') +# return emoji_pattern.sub('', text)This keeps the example lightweight while setting correct expectations about robustness and versioning.
61-62: Ensure cross-reference to persuasion-principles.md is valid and discoverableYou reference
persuasion-principles.mdhere; double-check that:
- The file exists in the repo under that exact name/path.
- There is a clear navigation path (e.g., from a main index of writing skills) so readers can actually find the referenced document.
If not, consider adding a brief note earlier in this file about where to locate that companion doc or adding it to whatever index you use for skills.
skills/record-plan/scripts/install-hook.sh (1)
7-7: Verify regex pattern catches all non-conforming filenames.The regex pattern
llm/implementation-plans/[^0-9][^/]*\.md$only checks that files don't start with a digit. This would correctly flag files likemy-plan.mdbut may not be the most precise pattern for enforcing the YYMMDD-XX format.Consider whether the pattern should be more specific, such as:
- Positive match for valid format:
llm/implementation-plans/[0-9]{6}-[0-9]{2}\.md$- Or more explicit negative match to catch edge cases
The current pattern works for the common case but may allow through unusual filenames.
Apply this diff if you want to use a positive match approach:
-if git diff --cached --name-only | grep -qE "llm/implementation-plans/[^0-9][^/]*\.md$"; then +if git diff --cached --name-only | grep -E "llm/implementation-plans/.*\.md$" | grep -qvE "llm/implementation-plans/[0-9]{6}-[0-9]{2}\.md$"; thenThis approach finds all
.mdfiles in the directory, then excludes those matching the valid format.CLAUDE.md (1)
53-53: Consider adding language identifiers to fenced code blocks.Lines 53, 64, and 74 have fenced code blocks without language identifiers. Adding them improves syntax highlighting and readability.
Apply these diffs:
Line 53:
-``` +```text skills/ skill-name/ # lowercase-with-hyphens only (no special chars)Line 64:
-```yaml +```yaml ---Line 74:
-``` +```text - Self-contained skill → Only `SKILL.md`Also applies to: 64-64, 74-74
skills/record-plan/scripts/generate_acceptance.py (1)
19-19: Remove unused loop variable.The loop variable
idxis not used within the loop body. Replace with_to indicate it's intentionally unused.Apply this diff:
- for idx, line in enumerate(plan_content.split('\n')): + for _, line in enumerate(plan_content.split('\n')):skills/record-plan/SKILL.md (1)
205-205: Consider adding language specifiers to code blocks.Lines 205, 343, and 413 contain code blocks without language specifiers. While these are templates/output examples, adding language hints can improve readability:
- Line 205: Consider ````markdown`
- Line 343: Consider
text` orbash`- Line 413: Consider ````text`
Also applies to: 343-345, 413-419
skills/record-plan/scripts/write_plan.py (3)
40-52: Consider validating target directory existence.The script constructs target paths but doesn't verify that the directories exist. While the instructions are for the user/agent to follow, adding a check could provide earlier feedback if the directory structure is missing.
if args.in_plan_mode: staging_path = os.path.expanduser("~/.claude/plans") + os.makedirs(staging_path, exist_ok=True) target_file = f"{staging_path}/{args.plan_name}.md" final_dir = os.path.join(args.working_dir, args.target_dir) print(f"📝 PLAN MODE DETECTED") print(f"✓ Write plan to: {target_file}") print(f"✓ Then copy to: {final_dir}/{args.plan_name}.md") else: target_dir = os.path.join(args.working_dir, args.target_dir) + os.makedirs(target_dir, exist_ok=True) target_file = f"{target_dir}/{args.plan_name}.md" print(f"📝 REGULAR MODE") print(f"✓ Write plan to: {target_file}")
54-60: Add error handling and stale lock detection.The lock file creation (lines 54-60) lacks error handling and doesn't check for existing locks, which could indicate concurrent usage or stale state from a previous interrupted workflow.
# Create lock file to enable Write tool for this plan lock_file = os.path.join(args.working_dir, '.record-plan-active') + + # Check for stale lock + if os.path.exists(lock_file): + print(f"⚠️ WARNING: Lock file already exists: {lock_file}") + print(f" This may indicate:") + print(f" - Another record-plan session is active") + print(f" - Previous session was interrupted") + print(f" Review and remove manually if stale") + return 1 + + # Create lock with error handling + try: - with open(lock_file, 'w') as f: + with open(lock_file, 'w') as f: f.write(f"{target_file}\n") f.write(f"created: {datetime.now().isoformat()}\n") + except (IOError, OSError) as e: + print(f"❌ ERROR: Failed to create lock file: {e}") + return 1
45-45: Remove unnecessary f-string prefixes.Multiple print statements use f-string prefixes without any placeholders (lines 45, 51, 62-68, 71, 73-75). While this maintains visual consistency, removing the unnecessary
fprefixes would be cleaner.Apply these changes:
- print(f"📝 PLAN MODE DETECTED") + print("📝 PLAN MODE DETECTED") print(f"✓ Write plan to: {target_file}") print(f"✓ Then copy to: {final_dir}/{args.plan_name}.md") else: target_dir = os.path.join(args.working_dir, args.target_dir) target_file = f"{target_dir}/{args.plan_name}.md" - print(f"📝 REGULAR MODE") + print("📝 REGULAR MODE") print(f"✓ Write plan to: {target_file}") # ... lock file creation ... - print(f"\n🔧 REQUIRED ACTIONS:") - print(f"1. Use Write tool to create file with:") - print(f" - First line: <!-- jot:md-rename -->") - print(f" - YAML frontmatter (title, date, type, status)") - print(f" - H1 heading with feature name") - print(f" - Implementation tasks following record-plan format") - print(f"\n2. After writing, run post-write workflow:") + print("\n🔧 REQUIRED ACTIONS:") + print("1. Use Write tool to create file with:") + print(" - First line: <!-- jot:md-rename -->") + print(" - YAML frontmatter (title, date, type, status)") + print(" - H1 heading with feature name") + print(" - Implementation tasks following record-plan format") + print("\n2. After writing, run post-write workflow:") # ... (keep lines with {target_file} placeholder as f-strings) - print(f" ⚠️ IMPORTANT: Rename script will remove lock file") + print(" ⚠️ IMPORTANT: Rename script will remove lock file") - print(f"\n⚠️ CRITICAL: DO NOT just describe the plan!") - print(f"⚠️ You MUST use Write tool to create the actual file!") - print(f"⚠️ Describing = BUG. Writing = CORRECT.") + print("\n⚠️ CRITICAL: DO NOT just describe the plan!") + print("⚠️ You MUST use Write tool to create the actual file!") + print("⚠️ Describing = BUG. Writing = CORRECT.")Also applies to: 51-51, 62-68, 71-71, 73-75
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (19)
.claude/settings.local.json(0 hunks).gitignore(1 hunks)CLAUDE.md(1 hunks)README.md(2 hunks)commands/write-plan.md(1 hunks)scripts/reload-plugin.sh(1 hunks)skills/brainstorming/SKILL.md(1 hunks)skills/executing-plans/SKILL.md(3 hunks)skills/record-plan/CHANGELOG.md(1 hunks)skills/record-plan/SKILL.md(1 hunks)skills/record-plan/scripts/check_lock.py(1 hunks)skills/record-plan/scripts/generate_acceptance.py(1 hunks)skills/record-plan/scripts/install-hook.sh(1 hunks)skills/record-plan/scripts/write_plan.py(1 hunks)skills/subagent-driven-development/SKILL.md(1 hunks)skills/using-superpowers/SKILL.md(1 hunks)skills/writing-plans/SKILL.md(0 hunks)skills/writing-skills/SKILL.md(4 hunks)skills/writing-skills/automation-over-documentation.md(1 hunks)
💤 Files with no reviewable changes (2)
- .claude/settings.local.json
- skills/writing-plans/SKILL.md
🧰 Additional context used
🧬 Code graph analysis (1)
skills/record-plan/scripts/write_plan.py (1)
skills/record-plan/scripts/generate_acceptance.py (1)
main(71-82)
🪛 LanguageTool
CLAUDE.md
[grammar] ~180-~180: Use a hyphen to join words.
Context: ...- graphviz-conventions.dot - Flowchart style rules - persuasion-principles.md...
(QB_NEW_EN_HYPHEN)
🪛 markdownlint-cli2 (0.18.1)
CLAUDE.md
53-53: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
64-64: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
74-74: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
102-102: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
113-113: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
164-164: Bare URL used
(MD034, no-bare-urls)
165-165: Bare URL used
(MD034, no-bare-urls)
skills/record-plan/SKILL.md
182-182: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
187-187: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
194-194: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
199-199: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
205-205: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
343-343: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
413-413: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
README.md
113-113: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
🪛 Ruff (0.14.8)
skills/record-plan/scripts/check_lock.py
14-14: f-string without any placeholders
Remove extraneous f prefix
(F541)
16-16: f-string without any placeholders
Remove extraneous f prefix
(F541)
skills/record-plan/scripts/write_plan.py
45-45: f-string without any placeholders
Remove extraneous f prefix
(F541)
51-51: f-string without any placeholders
Remove extraneous f prefix
(F541)
62-62: f-string without any placeholders
Remove extraneous f prefix
(F541)
63-63: f-string without any placeholders
Remove extraneous f prefix
(F541)
64-64: f-string without any placeholders
Remove extraneous f prefix
(F541)
65-65: f-string without any placeholders
Remove extraneous f prefix
(F541)
66-66: f-string without any placeholders
Remove extraneous f prefix
(F541)
67-67: f-string without any placeholders
Remove extraneous f prefix
(F541)
68-68: f-string without any placeholders
Remove extraneous f prefix
(F541)
71-71: f-string without any placeholders
Remove extraneous f prefix
(F541)
73-73: f-string without any placeholders
Remove extraneous f prefix
(F541)
74-74: f-string without any placeholders
Remove extraneous f prefix
(F541)
75-75: f-string without any placeholders
Remove extraneous f prefix
(F541)
skills/record-plan/scripts/generate_acceptance.py
19-19: Loop control variable idx not used within loop body
Rename unused idx to _idx
(B007)
🔇 Additional comments (19)
skills/writing-skills/SKILL.md (2)
22-35: EXTREMELY-IMPORTANT block is well-aligned with the “skills = code” stanceThis admonition block is consistent with the rest of the document’s TDD framing and use of custom tags like
<Bad>/<Good>. It makes the “consult this first” expectation explicit for any skill edits, which is helpful given how often these files change.
438-440: Nice reinforcement of common “no-test” rationalizationsThese added rows plug concrete, high-frequency excuses that match how skills actually get edited (“just adding a section / documentation / quick fix”). They tighten the rationalization table without adding noise.
skills/writing-skills/automation-over-documentation.md (1)
1-88: Strong, coherent articulation of automation-over-documentation philosophyThe structure and messaging are clear and actionable: concrete failure/success case, explicit criteria for automation vs documentation, and good TDD framing. This should be easy for contributors and future skill authors to operationalize.
.gitignore (1)
3-3: LGTM! Appropriate gitignore addition.Adding
.claude/settings.local.jsonto gitignore prevents local environment-specific configuration from being committed, which aligns with the deletion of this file in the PR.skills/subagent-driven-development/SKILL.md (1)
179-179: LGTM! Terminology update aligns with the record-plans workflow.The reference update from
writing-planstorecord-plansis consistent with the repository-wide terminology shift introduced in this PR.skills/brainstorming/SKILL.md (1)
45-45: LGTM! Consistent terminology update.The reference update from
writing-planstorecord-plansmaintains consistency with the new workflow naming introduced in this PR.skills/using-superpowers/SKILL.md (1)
46-46: LGTM! Valuable addition to prevent rationalization.Adding this denial addresses a common temptation to skip the TDD process when editing skills. The guidance is clear: skills are code and require the same discipline.
commands/write-plan.md (1)
5-5: LGTM! Command reference updated correctly.The skill reference update from
writing-planstorecord-plansensures the command invokes the correct workflow.scripts/reload-plugin.sh (1)
1-20: LGTM! Clear and helpful utility script.This script provides a simple, user-friendly way to get the plugin reload commands. The instructions are clear and the reminder about starting a new session is valuable.
CLAUDE.md (2)
1-48: Comprehensive and well-structured documentation.The CLAUDE.md file provides excellent guidance for Claude Code integration, covering project structure, development workflow, and testing methodology. The content is clear and actionable.
183-185: Excellent articulation of the project's philosophy.The philosophy section effectively captures the essence of treating documentation as testable artifacts. This aligns perfectly with the TDD methodology used throughout the skills system.
skills/record-plan/CHANGELOG.md (1)
1-42: LGTM! Well-structured changelog.The changelog clearly documents the two major updates (file-track integration and executable wrapper) with appropriate migration notes. The chronological structure and clear categorization (Added/Changed/Fixed/Removed) make it easy to understand the evolution of the record-plan skill.
skills/executing-plans/SKILL.md (1)
1-83: LGTM! Clear workflow improvements.The updates effectively enforce continuous execution without unnecessary pauses. The expanded "Common Rationalizations for Stopping" section (lines 39-49) is particularly valuable for preventing premature interruptions. The changes align well with the separation between record-plan (writing) and executing-plans (execution) introduced elsewhere in this PR.
skills/record-plan/scripts/generate_acceptance.py (1)
71-85: LGTM! Clean CLI interface.The main function follows good practices with clear error messages, file existence validation, and proper exit codes.
README.md (1)
86-86: LGTM! Terminology updates are consistent.The updates correctly replace "writing-plans" with "record-plans" throughout the documentation, and add the new collaboration skills to the list. The changes maintain consistency with the broader PR refactoring.
Also applies to: 115-122
skills/record-plan/scripts/check_lock.py (1)
30-38: LGTM! Clean CLI implementation.The CLI entry point properly validates arguments and returns appropriate exit codes for success/failure.
skills/record-plan/SKILL.md (2)
146-163: LGTM! Comprehensive workflow documentation.The enforcement mechanism (lines 146-163), Red Flags section (lines 214-252), and Post-Write Workflow (lines 254-320) provide clear, actionable guidance. The defensive checks in progress.md initialization and the explicit violation examples are particularly helpful for preventing common mistakes.
Also applies to: 214-252, 254-320
378-434: LGTM! Clear scope boundaries and testing evidence.The Common Mistakes (lines 378-399), STOP directive (lines 400-428), and Testing Verification (lines 429-434) sections effectively reinforce the skill's scope boundaries. The inclusion of testing evidence and clear anti-patterns helps prevent misuse.
skills/record-plan/scripts/write_plan.py (1)
13-38: LGTM! Clear CLI interface.The argument parser is well-structured with appropriate defaults and helpful descriptions for each parameter.
| 1. **Edit skills** in `/Users/ethan.stark/dev/claude-code-resources/superpowers/skills/` | ||
| 2. **Commit changes** to your branch (e.g., `ethan-mod`) |
There was a problem hiding this comment.
Replace user-specific paths and branch names with placeholders.
Lines 38-39 contain hardcoded user-specific values that won't work for other contributors:
/Users/ethan.stark/dev/claude-code-resources/superpowers/skills/- Branch:
ethan-mod
Apply this diff to use placeholders:
-1. **Edit skills** in `/Users/ethan.stark/dev/claude-code-resources/superpowers/skills/`
-2. **Commit changes** to your branch (e.g., `ethan-mod`)
+1. **Edit skills** in `<your-local-superpowers-path>/skills/`
+2. **Commit changes** to your feature branch (e.g., `add-my-skill`)Or use an environment variable approach:
-1. **Edit skills** in `/Users/ethan.stark/dev/claude-code-resources/superpowers/skills/`
+1. **Edit skills** in your local superpowers clone (typically `~/dev/superpowers/skills/`)📝 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.
| 1. **Edit skills** in `/Users/ethan.stark/dev/claude-code-resources/superpowers/skills/` | |
| 2. **Commit changes** to your branch (e.g., `ethan-mod`) | |
| 1. **Edit skills** in `<your-local-superpowers-path>/skills/` | |
| 2. **Commit changes** to your feature branch (e.g., `add-my-skill`) |
🤖 Prompt for AI Agents
In CLAUDE.md around lines 38 to 39, replace the hardcoded user-specific path
(/Users/ethan.stark/dev/claude-code-resources/superpowers/skills/) and branch
name (ethan-mod) with generic placeholders (e.g.,
<path-to-repo>/superpowers/skills/ and <your-branch-name>) or show how to use an
environment variable (e.g., $REPO_ROOT and $BRANCH_NAME) so instructions work
for other contributors; update the examples and any commands to reference these
placeholders or env vars and include a short note on how to set them.
| print(f" python3 ~/.claude/skills/record-plan/scripts/write_plan.py \\") | ||
| print(f" --working-dir {working_dir} \\") | ||
| print(f" --plan-name <descriptive-name>") |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Search for all references to write_plan.py to determine the canonical path
rg -n "write_plan\.py" -C 2Repository: obra/superpowers
Length of output: 2410
🏁 Script executed:
#!/bin/bash
# First, locate the check_lock.py file and examine it
fd "check_lock.py" -x cat -n {}Repository: obra/superpowers
Length of output: 1589
🏁 Script executed:
#!/bin/bash
# Find where write_plan.py is actually located in the repo
fd "write_plan\.py"Repository: obra/superpowers
Length of output: 100
🏁 Script executed:
#!/bin/bash
# Check the directory structure to understand the layout
find . -type d -name "scripts" | head -20
find . -type d -name "record-plan" | head -20
find . -type d -name "record-tools" | head -20Repository: obra/superpowers
Length of output: 119
Update script path to match documentation and remove unnecessary f-string prefixes.
Line 14 references the incorrect path ~/.claude/skills/record-plan/scripts/write_plan.py, but SKILL.md documents the correct installation path as ~/.claude/scripts/record-plan/write_plan.py. Update this path to be consistent with the documented user instructions.
Additionally, lines 14 and 16 use f-string prefixes without any template placeholders:
- print(f" python3 ~/.claude/skills/record-plan/scripts/write_plan.py \\")
+ print(f" python3 ~/.claude/scripts/record-plan/write_plan.py \\")
print(f" --working-dir {working_dir} \\")
- print(f" --plan-name <descriptive-name>")
+ print(" --plan-name <descriptive-name>")📝 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.
| print(f" python3 ~/.claude/skills/record-plan/scripts/write_plan.py \\") | |
| print(f" --working-dir {working_dir} \\") | |
| print(f" --plan-name <descriptive-name>") | |
| print(f" python3 ~/.claude/scripts/record-plan/write_plan.py \\") | |
| print(f" --working-dir {working_dir} \\") | |
| print(" --plan-name <descriptive-name>") |
🧰 Tools
🪛 Ruff (0.14.8)
14-14: f-string without any placeholders
Remove extraneous f prefix
(F541)
16-16: f-string without any placeholders
Remove extraneous f prefix
(F541)
🤖 Prompt for AI Agents
In skills/record-plan/scripts/check_lock.py around lines 14 to 16, update the
printed example path to match SKILL.md by changing
"~/.claude/skills/record-plan/scripts/write_plan.py" to
"~/.claude/scripts/record-plan/write_plan.py" and remove unnecessary f-string
prefixes on lines 14 and 16 (use plain string literals since there are no
placeholders); keep the working_dir interpolation on line 15 as an f-string only
if it contains a variable, otherwise convert that line to a plain string as
well.
| if not file_path.endswith(os.path.basename(authorized_path)): | ||
| print(f"❌ ERROR: Lock file authorizes {authorized_path}, not {file_path}") | ||
| return False |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# First, locate and read the check_lock.py file
find . -name "check_lock.py" -type f | head -5Repository: obra/superpowers
Length of output: 102
🏁 Script executed:
#!/bin/bash
# Read the full check_lock.py file to understand context
cat -n skills/record-plan/scripts/check_lock.pyRepository: obra/superpowers
Length of output: 1589
🏁 Script executed:
#!/bin/bash
# Search for usages of check_lock in the codebase
rg -n "check_lock" -B 2 -A 5 --type pyRepository: obra/superpowers
Length of output: 1356
🏁 Script executed:
#!/bin/bash
# Search for where check_lock is imported or called
rg -n "check_lock\|from.*check_lock\|import.*check_lock" -B 3 -A 3 --type pyRepository: obra/superpowers
Length of output: 42
🏁 Script executed:
#!/bin/bash
# Look for write_plan.py to understand how it creates the lock file and what it stores
find . -name "write_plan.py" -o -name "*write*plan*" -type fRepository: obra/superpowers
Length of output: 127
🏁 Script executed:
#!/bin/bash
# Search for .record-plan-active lock file references to understand the format
rg -n "record-plan-active\|\.record-plan" -B 2 -A 2Repository: obra/superpowers
Length of output: 42
🏁 Script executed:
#!/bin/bash
# Read write_plan.py to understand how the lock file is created and what path it stores
cat -n skills/record-plan/scripts/write_plan.pyRepository: obra/superpowers
Length of output: 3516
🏁 Script executed:
#!/bin/bash
# Check if there are any tests or examples that show how check_lock is used
find . -name "*test*" -o -name "*example*" | grep -i "lock\|plan" | head -10Repository: obra/superpowers
Length of output: 42
🏁 Script executed:
#!/bin/bash
# Search for references to the lock file creation or any path handling
rg -n "\.record-plan-active\|write.*lock\|create.*lock" -B 2 -A 2 --type pyRepository: obra/superpowers
Length of output: 42
🏁 Script executed:
#!/bin/bash
# Search for actual invocations of check_lock function or script
rg -n "check_lock\|subprocess.*check_lock\|os.system.*check_lock" -B 3 -A 3Repository: obra/superpowers
Length of output: 42
🏁 Script executed:
#!/bin/bash
# Look for Write tool or any tool that might call check_lock
find . -name "*.py" -type f | xargs rg -l "check_lock" | head -10Repository: obra/superpowers
Length of output: 102
🏁 Script executed:
#!/bin/bash
# Search for any path normalization (realpath, abspath) used with these files
rg -n "realpath\|abspath\|normpath" --type py | grep -i "lock\|plan\|path" | head -20Repository: obra/superpowers
Length of output: 42
🏁 Script executed:
#!/bin/bash
# Search for shell scripts that might invoke check_lock.py
find . -name "*.sh" -type f | xargs rg -l "check_lock" 2>/dev/nullRepository: obra/superpowers
Length of output: 44
🏁 Script executed:
#!/bin/bash
# Look for any markdown documentation about record-plan and how it's meant to work
find . -name "*.md" -type f | xargs rg -l "check_lock\|record-plan" 2>/dev/null | head -10Repository: obra/superpowers
Length of output: 42
🏁 Script executed:
#!/bin/bash
# Check the write-plan.md command file to understand the full workflow
cat -n ./commands/write-plan.mdRepository: obra/superpowers
Length of output: 220
🏁 Script executed:
#!/bin/bash
# Search broadly for how write tool or skills invoke check_lock
rg -n "write_plan\|record-plan" --type py -B 2 -A 2 | head -80Repository: obra/superpowers
Length of output: 42
🏁 Script executed:
#!/bin/bash
# Search for tool definitions or skill configurations that might show how check_lock is used
find . -type f \( -name "*.json" -o -name "*.yaml" -o -name "*.yml" -o -name "*.md" \) | xargs rg -l "check_lock" 2>/dev/nullRepository: obra/superpowers
Length of output: 89
🏁 Script executed:
#!/bin/bash
# Look at directory structure of record-plan skill
find ./skills/record-plan -type f | head -20Repository: obra/superpowers
Length of output: 306
🏁 Script executed:
#!/bin/bash
# Check if there's a tool definition or usage guide in the skill directory
cat -n ./skills/record-plan/scripts/*.py | head -150Repository: obra/superpowers
Length of output: 5764
Path validation uses basename-only matching, creating false positive vulnerability.
Line 23 validates with file_path.endswith(os.path.basename(authorized_path)), which only checks filenames rather than full paths. Since the lock file stores the complete target path (from write_plan.py line 57), this allows files in different directories to incorrectly pass validation.
Example false positive:
- Lock file authorizes:
/home/user/repo/llm/implementation-plans/feature.md - File path provided:
/tmp/feature.md - Validation passes because both end with
feature.md
The error message on line 24 shows full paths, indicating intent to validate specific file paths. Compare file_path against the stored authorized_path directly, or use os.path.abspath() to normalize and compare full resolved paths.
🤖 Prompt for AI Agents
In skills/record-plan/scripts/check_lock.py around lines 23 to 25, the code
currently validates by comparing only basenames
(file_path.endswith(os.path.basename(authorized_path))) which allows
different-directory files with the same name to pass; update the check to
normalize and compare full resolved paths (e.g., use os.path.abspath or
os.path.realpath on both file_path and authorized_path) and compare them for
equality so the lock authorizes the exact file path; adjust the error handling
to preserve the existing message but rely on the normalized comparison.
| def generate_acceptance(plan_file: Path, output_file: Path): | ||
| """Generate acceptance.json from plan file.""" | ||
| plan_content = plan_file.read_text() | ||
| features = extract_tasks(plan_content) | ||
|
|
||
| # Warn if no tasks were found | ||
| if not features: | ||
| print(f"WARNING: No tasks found in {plan_file.name}") | ||
| print(" Expected pattern: ### Task N: Description") | ||
| print(" Generating empty acceptance.json") | ||
|
|
||
| acceptance = { | ||
| "plan": str(plan_file.relative_to(Path.cwd())), | ||
| "generated": date.today().isoformat(), | ||
| "total_features": len(features), | ||
| "passing_features": 0, | ||
| "features": features, | ||
| "_rules": { | ||
| "immutable_fields": ["id", "category", "description", "steps"], | ||
| "mutable_fields": ["passes", "notes"], | ||
| "catastrophic_actions": [ | ||
| "remove features", | ||
| "edit descriptions", | ||
| "modify steps" | ||
| ] | ||
| } | ||
| } | ||
|
|
||
| output_file.write_text(json.dumps(acceptance, indent=2) + '\n') | ||
| print(f"✓ Generated {len(features)} acceptance criteria") | ||
| print(f" Output: {output_file}") |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's examine the full generate_acceptance.py file
cat -n skills/record-plan/scripts/generate_acceptance.pyRepository: obra/superpowers
Length of output: 3358
🏁 Script executed:
# Look for wrapper scripts or entry points that call this function
fd . skills/record-plan/ -type f -name "*.py" -o -name "*.sh" | head -20Repository: obra/superpowers
Length of output: 230
🏁 Script executed:
# Search for invocations of generate_acceptance function
rg "generate_acceptance" skills/record-plan/ -n -B 2 -A 2Repository: obra/superpowers
Length of output: 1293
🏁 Script executed:
# Check for __main__ block and how arguments are parsed
rg "if __name__" skills/record-plan/scripts/generate_acceptance.py -A 20Repository: obra/superpowers
Length of output: 103
Handle relative_to() failure for plan files outside cwd.
Line 51 uses Path.relative_to(Path.cwd()) which raises ValueError if the plan file is not under the current working directory. Since the CLI accepts arbitrary --plan-file paths, this can crash the script if invoked from an unexpected directory or with an absolute path outside cwd.
Either wrap the call in error handling, use an absolute path instead, or document that plan_file must reside under the current working directory.
🤖 Prompt for AI Agents
In skills/record-plan/scripts/generate_acceptance.py around lines 39 to 69, the
call to Path.relative_to(Path.cwd()) can raise ValueError for plan files outside
the current working directory; change the code to compute a safe plan path by
attempting relative_to in a try/except and falling back to an absolute/resolved
or os.path.relpath representation: try to use plan_file.relative_to(Path.cwd())
and on ValueError use str(plan_file.resolve()) (or os.path.relpath(plan_file,
Path.cwd())) and assign that string into the acceptance["plan"] field so the
script no longer crashes for external or absolute plan paths.
| ' | ||
|
|
||
| # Find all repos with llm/implementation-plans/ directory | ||
| for repo in /Users/ethan.stark/dev/*/; do |
There was a problem hiding this comment.
Hardcoded user path prevents portability.
The path /Users/ethan.stark/dev/ is hardcoded, making this script user-specific and non-portable. This will fail for other users or on different systems.
Consider these solutions:
Solution 1 (Recommended): Make path configurable
+# Usage: install-hook.sh [base_dir]
+BASE_DIR="${1:-$HOME/dev}"
+
# Find all repos with llm/implementation-plans/ directory
-for repo in /Users/ethan.stark/dev/*/; do
+for repo in "$BASE_DIR"/*/; doSolution 2: Auto-detect from current directory
# Find all repos with llm/implementation-plans/ directory
-for repo in /Users/ethan.stark/dev/*/; do
+# Run from a parent directory containing your repos
+for repo in ./*/; doCommittable suggestion skipped: line range outside the PR's diff.
| echo "$HOOK_CONTENT" > "$hook_path" | ||
| chmod +x "$hook_path" |
There was a problem hiding this comment.
Hook installation overwrites existing pre-commit hooks without warning.
Lines 27-28 unconditionally overwrite any existing .git/hooks/pre-commit file. This could break existing workflows that rely on pre-commit hooks.
Consider backing up existing hooks or merging logic:
+# Backup existing hook if present
+if [ -f "$hook_path" ]; then
+ backup_path="${hook_path}.backup-$(date +%s)"
+ cp "$hook_path" "$backup_path"
+ echo "⚠ Backed up existing hook to: $backup_path"
+fi
+
echo "$HOOK_CONTENT" > "$hook_path"
chmod +x "$hook_path"
echo "✓ Installed hook: $hook_path"Alternatively, check if the hook already contains this logic before overwriting:
+if [ -f "$hook_path" ] && grep -q "YYMMDD-XX format" "$hook_path"; then
+ echo "⏭ Hook already installed: $hook_path"
+ continue
+fi
echo "$HOOK_CONTENT" > "$hook_path"📝 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.
| echo "$HOOK_CONTENT" > "$hook_path" | |
| chmod +x "$hook_path" | |
| # Backup existing hook if present | |
| if [ -f "$hook_path" ]; then | |
| backup_path="${hook_path}.backup-$(date +%s)" | |
| cp "$hook_path" "$backup_path" | |
| echo "⚠ Backed up existing hook to: $backup_path" | |
| fi | |
| echo "$HOOK_CONTENT" > "$hook_path" | |
| chmod +x "$hook_path" | |
| echo "✓ Installed hook: $hook_path" |
🤖 Prompt for AI Agents
In skills/record-plan/scripts/install-hook.sh around lines 27-28, the script
currently overwrites .git/hooks/pre-commit unconditionally; change it to first
detect an existing hook and avoid destructive replacement: if a hook file
exists, create a timestamped backup (or copy to .bak), then check whether the
existing file already contains the marker/logic you intend to install and only
append the new content if absent (or merge by inserting a unique marker so
future runs can idempotently update), and ensure the final hook retains
executable permissions; if no hook exists simply create it and chmod +x as now.
| **FIRST ACTION (mandatory):** Invoke wrapper script - DO NOT describe plan in chat first: | ||
|
|
||
| ```bash | ||
| python3 ~/.claude/scripts/record-plan/write_plan.py \ |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Search for all script path references across the codebase
echo "=== Searching for script path references ==="
rg "\.claude/(scripts|skills)/(record-plan|record-tools)" -n -C 1
echo ""
echo "=== Finding actual script files ==="
fd -t f "(write_plan|check_lock|validate-frontmatter|rename_jot|generate_acceptance)\.py" --type f
echo ""
echo "=== Checking check_lock.py line 14 ==="
find . -name "check_lock.py" -exec head -20 {} +Repository: obra/superpowers
Length of output: 4020
Update script paths in documentation to match actual locations.
The documentation references scripts at incorrect paths. The actual scripts are located in ~/.claude/skills/record-plan/scripts/, but SKILL.md references ~/.claude/scripts/record-plan/ in lines 19, 39, 137, and 160. This contradicts:
- check_lock.py line 14 (error message showing correct path)
- install-hook.sh line 11 (installer showing correct path)
- SKILL.md line 359 (single correct reference)
Update lines 19, 39, 137, 160 from ~/.claude/scripts/record-plan/ to ~/.claude/skills/record-plan/scripts/.
Note: Lines 323 and 335 reference ~/.claude/scripts/record-tools/ as documented paths, which are output by write_plan.py's own error messages; verify these shared tool locations are correct separately if needed.
🤖 Prompt for AI Agents
In skills/record-plan/SKILL.md around lines 19, 39, 137, and 160, the
documentation references scripts under ~/.claude/scripts/record-plan/ but the
actual scripts live at ~/.claude/skills/record-plan/scripts/; update those four
lines to replace the incorrect path with ~/.claude/skills/record-plan/scripts/
so they match check_lock.py and install-hook.sh and the correct reference at
line 359; do not change other references (lines 323 and 335) without separate
verification.
1 participant
Summary
Implements the missing
write_plan.pywrapper script to complete the lock file enforcement mechanism for the record-plan skill.Key Changes:
/Users/ethan.stark/.claude/scripts/record-plan/write_plan.pywrapper script/Users/ethan.stark/.claude/scripts/record-plan/check_lock.pyvalidation scriptskills/record-plan/SKILL.mdwith correct script paths (4 references)Architecture:
.record-plan-activelock files authorizing plan file creationrename_jot.pyautomatically removes lock after successful workflowRoot Cause:
Previous plans (251213-03, 251213-05) were written but never executed. Documentation was updated as if enforcement was complete, but the actual implementation code was missing. This PR completes the actual implementation.
Test Plan
rename_jot.pyremoves lock after complete workflowImplementation Details
Phase 1: Wrapper Script
write_plan.py: Creates lock files, prints directives, supports cleanup--working-dir,--plan-name,--target-dir,--cleanupmodesPhase 2: Skill Updates
~/.claude/skills/record-plan/scripts/to~/.claude/scripts/record-plan/Phase 3: Validation
check_lock.py: Optional validation script for agent self-checksPhase 4: Integration Testing
Phase 5: Documentation
🤖 Generated with Claude Code
Co-Authored-By: Claude noreply@anthropic.com
Summary by CodeRabbit
New Features
Documentation
Chores
✏️ Tip: You can customize this high-level summary in your review settings.