Add trail checkout --worktree Support

Using the brainstorming skill, I'd like to add support for entire tail checkout --worktree which should add support for creating a worktree before checking out a trail.
Use entire-worktree create as an example of that that could look like. The repo is in ~/entire/entire-worktree. For simplicity, don't port over the mise trust code for now and focus on the worktree mechanics, .worktreeinclude and such.
Can you take a look at this PR and see what difference ther are in this implementation? https://github.com/entireio/cli/pull/1686
One of the main things I'd like to use here as well is storing the worktrees in the .entire/worktrees directory. Also printing a shell-safe path when the command is done would be great. I'm wondering if we should offer the user a choice between ading .entire/worktrees to the .gitignore or adding it to ~/.git/info/exclude if the command is run via an interactive terminal. If it isn't, maybe use .git/info/exclude but make it clear what's happening via the CLI output somehow? I'd be open to suggestions in that regard.
Base directory for this skill: /Users/pfleidi/.claude/plugins/cache/claude-plugins-official/superpowers/6.0.3/skills/brainstorming
Brainstorming Ideas Into Designs
Help turn ideas into fully formed designs and specs through natural collaborative dialogue.
Start by understanding the current project context, then ask questions one at a time to refine the idea. Once you understand what you're building, present the design and get user approval.
<HARD-GATE> Do NOT invoke any implementation skill, write any code, scaffold any project, or take any implementation action until you have presented a design and the user has approved it. This applies to EVERY project regardless of perceived simplicity. </HARD-GATE>Anti-Pattern: "This Is Too Simple To Need A Design"
Every project goes through this process. A todo list, a single-function utility, a config change — all of them. "Simple" projects are where unexamined assumptions cause the most wasted work. The design can be short (a few sentences for truly simple projects), but you MUST present it and get approval.
Checklist
You MUST create a task for each of these items and complete them in order:
- Explore project context — check files, docs, recent commits
- Offer the visual companion just-in-time — NOT upfront. The first time a question would genuinely be clearer shown than described, offer it then (its own message); on approval its browser tab opens for you. If no visual question ever arises, never offer it. See the Visual Companion section below.
- Ask clarifying questions — one at a time, understand purpose/constraints/success criteria
- Propose 2-3 approaches — with trade-offs and your recommendation
- Present design — in sections scaled to their complexity, get user approval after each section
- Write design doc — save to
docs/superpowers/specs/YYYY-MM-DD-<topic>-design.mdand commit - Spec self-review — quick inline check for placeholders, contradictions, ambiguity, scope (see below)
- User reviews written spec — ask user to review the spec file before proceeding
- Transition to implementation — invoke writing-plans skill to create implementation plan
Process Flow
The terminal state is invoking writing-plans. Do NOT invoke frontend-design, mcp-builder, or any other implementation skill. The ONLY skill you invoke after brainstorming is writing-plans.
The Process
Understanding the idea:
- Check out the current project state first (files, docs, recent commits)
- Before asking detailed questions, assess scope: if the request describes multiple independent subsystems (e.g., "build a platform with chat, file storage, billing, and analytics"), flag this immediately. Don't spend questions refining details of a project that needs to be decomposed first.
- If the project is too large for a single spec, help the user decompose into sub-projects: what are the independent pieces, how do they relate, what order should they be built? Then brainstorm the first sub-project through the normal design flow. Each sub-project gets its own spec → plan → implementation cycle.
- For appropriately-scoped projects, ask questions one at a time to refine the idea
- Prefer multiple choice questions when possible, but open-ended is fine too
- Only one question per message - if a topic needs more exploration, break it into multiple questions
- Focus on understanding: purpose, constraints, success criteria
Exploring approaches:
- Propose 2-3 different approaches with trade-offs
- Present options conversationally with your recommendation and reasoning
- Lead with your recommended option and explain why
Presenting the design:
- Once you believe you understand what you're building, present the design
- Scale each section to its complexity: a few sentences if straightforward, up to 200-300 words if nuanced
- Ask after each section whether it looks right so far
- Cover: architecture, components, data flow, error handling, testing
- Be ready to go back and clarify if something doesn't make sense
Design for isolation and clarity:
- Break the system into smaller units that each have one clear purpose, communicate through well-defined interfaces, and can be understood and tested independently
- For each unit, you should be able to answer: what does it do, how do you use it, and what does it depend on?
- Can someone understand what a unit does without reading its internals? Can you change the internals without breaking consumers? If not, the boundaries need work.
- Smaller, well-bounded units are also easier for you to work with - you reason better about code you can hold in context at once, and your edits are more reliable when files are focused. When a file grows large, that's often a signal that it's doing too much.
Working in existing codebases:
- Explore the current structure before proposing changes. Follow existing patterns.
- Where existing code has problems that affect the work (e.g., a file that's grown too large, unclear boundaries, tangled responsibilities), include targeted improvements as part of the design - the way a good developer improves code they're working in.
- Don't propose unrelated refactoring. Stay focused on what serves the current goal.
After the Design
Documentation:
- Write the validated design (spec) to
docs/superpowers/specs/YYYY-MM-DD-<topic>-design.md- (User preferences for spec location override this default)
- Use elements-of-style:writing-clearly-and-concisely skill if available
- Commit the design document to git
Spec Self-Review: After writing the spec document, look at it with fresh eyes:
- Placeholder scan: Any "TBD", "TODO", incomplete sections, or vague requirements? Fix them.
- Internal consistency: Do any sections contradict each other? Does the architecture match the feature descriptions?
- Scope check: Is this focused enough for a single implementation plan, or does it need decomposition?
- Ambiguity check: Could any requirement be interpreted two different ways? If so, pick one and make it explicit.
Fix any issues inline. No need to re-review — just fix and move on.
User Review Gate: After the spec review loop passes, ask the user to review the written spec before proceeding:
"Spec written and committed to
<path>. Please review it and let me know if you want to make any changes before we start writing out the implementation plan."
Wait for the user's response. If they request changes, make them and re-run the spec review loop. Only proceed once the user approves.
Implementation:
- Invoke the writing-plans skill to create a detailed implementation plan
- Do NOT invoke any other skill. writing-plans is the next step.
Key Principles
- One question at a time - Don't overwhelm with multiple questions
- Multiple choice preferred - Easier to answer than open-ended when possible
- YAGNI ruthlessly - Remove unnecessary features from all designs
- Explore alternatives - Always propose 2-3 approaches before settling
- Incremental validation - Present design, get approval before moving on
- Be flexible - Go back and clarify when something doesn't make sense
Visual Companion
A browser-based companion for showing mockups, diagrams, and visual options during brainstorming. Available as a tool — not a mode. Accepting the companion means it's available for questions that benefit from visual treatment; it does NOT mean every question goes through the browser.
Offering the companion (just-in-time): Do NOT offer it upfront. Wait until a question would genuinely be clearer shown than told — a real mockup / layout / diagram question, not merely a UI topic. The first time that happens, offer it then, as its own message:
"This next part might be easier if I show you — I can put together mockups, diagrams, and comparisons in a browser tab as we go. It's still new and can be token-intensive. Want me to? I'll open it for you."
This offer MUST be its own message. Only the offer — no clarifying question, summary, or other content. Wait for the user's response. If they accept, start the server with --open so their browser opens to the first screen automatically. If they decline, continue text-only and don't offer again unless they raise it.
Per-question decision: Even after the user accepts, decide FOR EACH QUESTION whether to use the browser or the terminal. The test: would the user understand this better by seeing it than reading it?
- Use the browser for content that IS visual — mockups, wireframes, layout comparisons, architecture diagrams, side-by-side visual designs
- Use the terminal for content that is text — requirements questions, conceptual choices, tradeoff lists, A/B/C/D text options, scope decisions
A question about a UI topic is not automatically a visual question. "What does personality mean in this context?" is a conceptual question — use the terminal. "Which wizard layout works better?" is a visual question — use the browser.
If they agree to the companion, read the detailed guide before proceeding:
skills/brainstorming/visual-companion.md
ARGUMENTS: Add support for entire trail checkout --worktree which creates a worktree before checking out a trail, modeled on entire-worktree create, storing worktrees under .entire/worktrees, printing a shell-safe path, and handling gitignore/exclude choices
Here's the full design. Given your low-input preference I'm presenting it in one pass — object to any section and I'll revise.
Design: entire trail checkout --worktree
1. Command surface
entire trail checkout [<trail>] --worktree gets a new --worktree bool flag. Trail resolution, --trail/--force semantics, and the ensureNoTrailRepoOverride guard are untouched. When --worktree is set, instead of switching the current checkout, the trail's branch is checked out into a new worktree and the command prints a shell-safe cd '<path>' line as its final output. --force keeps its existing meaning (skip the remote-fetch prompt) and additionally skips the ignore-rule confirm (non-interactive answers apply).
2. Worktree mechanics (largely the PR #1686 shape)
- Location:
<main-root>/.entire/worktrees/trail-<number>-<sanitized-branch>(sanitizer: non-[a-zA-Z0-9._-]→-, trimmed; falls back tobranchif empty; notrail-<n>-prefix when the trail has no number). - Main root resolved via
git rev-parse --git-common-dir, so running from inside a linked worktree creates a sibling under the main repo root, never a nested worktree. - If the branch is already checked out under
.entire/worktrees/(pergit worktree list --porcelain), print "Worktree already exists at …" + thecdhint and exit 0. No include-file copying on reuse. - Branch availability: local branch → use directly; remote-only → prompt to fetch (
--force/non-TTY skips prompt and fetches), fetch via explicit refspec; neither → error. - Creation via
git worktree add <path> <branch>(git CLI, per the repo's go-git-checkout ban). - The current checkout is never touched, so no uncommitted-changes guard is needed.
3. Ignore-rule handling (your decision, .gitignore-first)
Before creating the worktree:
git check-ignore -q .entire/worktreesfrom the main root — already ignored by any mechanism → do nothing, silently.- Interactive TTY (
interactive.CanPromptInteractively(), accessible-mode aware viaNewAccessibleForm): confirm "Add .entire/worktrees/ to .gitignore? (No = local-only .git/info/exclude)". Yes → append.entire/worktrees/to the repo-root.gitignoreand printAdded .entire/worktrees/ to .gitignore — commit this when convenient.No → append to.git/info/exclude(common dir, works from linked worktrees) and printAdded .entire/worktrees/ to .git/info/exclude (local to this clone). - Non-interactive (or
--force): the exclude path, with the same printed notice.
Both writers are idempotent (skip if the exact rule line already present) and preserve a trailing newline.
4. .worktreeinclude copying (ported 1:1 from entire-worktree)
- Read
<main-root>/.worktreeinclude; missing file → copy nothing. Lines are gitignore-style patterns; blank lines and#comments skipped. - Candidate files come from
git ls-files --others --ignored --exclude-standardat the main root (untracked-but-ignored files:.env,.claude/settings.local.json, …). - Matching via go-git v6's
plumbing/format/gitignorematcher, exactly as entire-worktree does; paths under.entire/worktrees/are excluded from candidates. - Copy after
git worktree addsucceeds: regular files only, parent dirs created, permissions preserved, symlinks/irregular files and per-file errors are warn-and-skip (warning to stderr) — a failed copy never fails the checkout. - Not ported: mise trust,
tmp/creation, new-branch-from-origin/main mode.
5. Code layout & errors
- New file
cmd/entire/cli/trail_checkout_worktree.go+trail_checkout_worktree_test.go;trail_cmd.goonly gains the flag, an options struct, and the branch intocheckoutTrailWorktree(...). - Errors return wrapped
fmt.Errorf(default main.go printing); noSilentErrorneeded. Git command failures include trimmed combined output.
6. Testing
Unit tests with real temp repos (testutil.InitRepo, t.Chdir), following the PR's test list plus the new behavior:
- path naming + shell quoting (pure,
t.Parallel()) - create from main root; from linked worktree → sibling; reuse existing; remote-only fetch; invalid branch rejection
- ignore handling: already-ignored → untouched; non-TTY → exclude + notice; idempotency of both writers (gitignore append tested via the non-TTY path by seeding state; the huh confirm itself isn't driven in tests)
.worktreeinclude: patterns copy matching ignored files, missing file copies nothing, comments/blank lines ignored, copy failure warns but succeeds
No E2E/integration additions — this is plumbing around local git, fully coverable in-package.
Does this look right, or should any section change?
Spec written. Self-review done: no placeholders, sections consistent (--force/non-TTY both take the exclude path; reuse skips copying), scope fits a single implementation plan. One deliberate call worth noting: for a remote-only branch in a non-TTY context the spec says fetch without prompting — that matches the agent-safe-fallback convention in this repo.
Per your global instructions I have not committed the spec (plan files aren't checked in unless you ask).
Spec written to docs/superpowers/specs/2026-07-09-trail-checkout-worktree-design.md. Please review it and let me know if you want changes — once you approve, I'll move on to writing the implementation plan.
Base directory for this skill: /Users/pfleidi/.claude/plugins/cache/claude-plugins-official/superpowers/6.0.3/skills/writing-plans
Writing Plans
Overview
Write comprehensive implementation plans assuming the engineer has zero context for our codebase and questionable taste. Document everything they need to know: which files to touch for each task, code, testing, docs they might need to check, how to test it. Give them the whole plan as bite-sized tasks. DRY. YAGNI. TDD. Frequent commits.
Assume they are a skilled developer, but know almost nothing about our toolset or problem domain. Assume they don't know good test design very well.
Announce at start: "I'm using the writing-plans skill to create the implementation plan."
Context: If working in an isolated worktree, it should have been created via the superpowers:using-git-worktrees skill at execution time.
Save plans to: docs/superpowers/plans/YYYY-MM-DD-<feature-name>.md
- (User preferences for plan location override this default)
Scope Check
If the spec covers multiple independent subsystems, it should have been broken into sub-project specs during brainstorming. If it wasn't, suggest breaking this into separate plans — one per subsystem. Each plan should produce working, testable software on its own.
File Structure
Before defining tasks, map out which files will be created or modified and what each one is responsible for. This is where decomposition decisions get locked in.
- Design units with clear boundaries and well-defined interfaces. Each file should have one clear responsibility.
- You reason best about code you can hold in context at once, and your edits are more reliable when files are focused. Prefer smaller, focused files over large ones that do too much.
- Files that change together should live together. Split by responsibility, not by technical layer.
- In existing codebases, follow established patterns. If the codebase uses large files, don't unilaterally restructure - but if a file you're modifying has grown unwieldy, including a split in the plan is reasonable.
This structure informs the task decomposition. Each task should produce self-contained changes that make sense independently.
Task Right-Sizing
A task is the smallest unit that carries its own test cycle and is worth a fresh reviewer's gate. When drawing task boundaries: fold setup, configuration, scaffolding, and documentation steps into the task whose deliverable needs them; split only where a reviewer could meaningfully reject one task while approving its neighbor. Each task ends with an independently testable deliverable.
Bite-Sized Task Granularity
Each step is one action (2-5 minutes):
- "Write the failing test" - step
- "Run it to make sure it fails" - step
- "Implement the minimal code to make the test pass" - step
- "Run the tests and make sure they pass" - step
- "Commit" - step
Plan Document Header
Every plan MUST start with this header:
Task Structure
No Placeholders
Every step must contain the actual content an engineer needs. These are plan failures — never write them:
- "TBD", "TODO", "implement later", "fill in details"
- "Add appropriate error handling" / "add validation" / "handle edge cases"
- "Write tests for the above" (without actual test code)
- "Similar to Task N" (repeat the code — the engineer may be reading tasks out of order)
- Steps that describe what to do without showing how (code blocks required for code steps)
- References to types, functions, or methods not defined in any task
Remember
- Exact file paths always
- Complete code in every step — if a step changes code, show the code
- Exact commands with expected output
- DRY, YAGNI, TDD, frequent commits
Self-Review
After writing the complete plan, look at the spec with fresh eyes and check the plan against it. This is a checklist you run yourself — not a subagent dispatch.
1. Spec coverage: Skim each section/requirement in the spec. Can you point to a task that implements it? List any gaps.
2. Placeholder scan: Search your plan for red flags — any of the patterns from the "No Placeholders" section above. Fix them.
3. Type consistency: Do the types, method signatures, and property names you used in later tasks match what you defined in earlier tasks? A function called clearLayers() in Task 3 but clearFullLayers() in Task 7 is a bug.
If you find issues, fix them inline. No need to re-review — just fix and move on. If you find a spec requirement with no task, add the task.
Execution Handoff
After saving the plan, offer execution choice:
"Plan complete and saved to docs/superpowers/plans/<filename>.md. Two execution options:
1. Subagent-Driven (recommended) - I dispatch a fresh subagent per task, review between tasks, fast iteration
2. Inline Execution - Execute tasks in this session using executing-plans, batch execution with checkpoints
Which approach?"
If Subagent-Driven chosen:
- REQUIRED SUB-SKILL: Use superpowers:subagent-driven-development
- Fresh subagent per task + two-stage review
If Inline Execution chosen:
- REQUIRED SUB-SKILL: Use superpowers:executing-plans
- Batch execution with checkpoints for review
ARGUMENTS: Create an implementation plan for entire trail checkout --worktree based on the approved spec at docs/superpowers/specs/2026-07-09-trail-checkout-worktree-design.md
Plan complete and saved to docs/superpowers/plans/2026-07-09-trail-checkout-worktree.md (not committed, per your plans rule). Self-review done: all six spec sections map to tasks, no placeholders, signatures consistent across tasks, and I verified runTrailCheckout has exactly one caller before planning its signature change.
The plan has 5 tasks, each a full TDD cycle with complete code and its own commit:
- Pure path/name helpers — worktree naming under
.entire/worktrees, shell quoting - Ignore-rule handling —
git check-ignoreno-op, .gitignore-first confirm, exclude fallback with notice .worktreeincludecopying — pattern loading,ls-files --ignoredmatching via go-git's gitignore matcher, warn-and-skip copies- Worktree creation core — reuse detection, remote-only fetch,
git worktree add, wiring of 1–3 - CLI wiring — the
--worktreeflag, options struct, help text, plus fmt/lint and a full-package test run
Two execution options:
1. Subagent-Driven (recommended) — I dispatch a fresh subagent per task, review between tasks, fast iteration
2. Inline Execution — execute tasks in this session using executing-plans, batch execution with checkpoints
Which approach? (Either way, each task's commit will wait for your approval per your checkpoint rules.)
Base directory for this skill: /Users/pfleidi/.claude/plugins/cache/claude-plugins-official/superpowers/6.0.3/skills/subagent-driven-development
Subagent-Driven Development
Execute plan by dispatching a fresh implementer subagent per task, a task review (spec compliance + code quality) after each, and a broad whole-branch review at the end.
Why subagents: You delegate tasks to specialized agents with isolated context. By precisely crafting their instructions and context, you ensure they stay focused and succeed at their task. They should never inherit your session's context or history — you construct exactly what they need. This also preserves your own context for coordination work.
Core principle: Fresh subagent per task + task review (spec + quality) + broad final review = high quality, fast iteration
Narration: between tool calls, narrate at most one short line — the ledger and the tool results carry the record.
Continuous execution: Do not pause to check in with your human partner between tasks. Execute all tasks from the plan without stopping. The only reasons to stop are: BLOCKED status you cannot resolve, ambiguity that genuinely prevents progress, or all tasks complete. "Should I continue?" prompts and progress summaries waste their time — they asked you to execute the plan, so execute it.
When to Use
vs. Executing Plans (parallel session):
- Same session (no context switch)
- Fresh subagent per task (no context pollution)
- Review after each task (spec compliance + code quality), broad review at the end
- Faster iteration (no human-in-loop between tasks)
The Process
Pre-Flight Plan Review
Before dispatching Task 1, scan the plan once for conflicts:
- tasks that contradict each other or the plan's Global Constraints
- anything the plan explicitly mandates that the review rubric treats as a defect (a test that asserts nothing, verbatim duplication of a logic block)
Present everything you find to your human partner as one batched question — each finding beside the plan text that mandates it, asking which governs — before execution begins, not one interrupt per discovery mid-plan. If the scan is clean, proceed without comment. The review loop remains the net for conflicts that only emerge from implementation.
Model Selection
Use the least powerful model that can handle each role to conserve cost and increase speed.
Mechanical implementation tasks (isolated functions, clear specs, 1-2 files): use a fast, cheap model. Most implementation tasks are mechanical when the plan is well-specified.
Integration and judgment tasks (multi-file coordination, pattern matching, debugging): use a standard model.
Architecture and design tasks: use the most capable available model. The final whole-branch review is one of these — dispatch it on the most capable available model, not the session default.
Review tasks: choose the model with the same judgment, scaled to the diff's size, complexity, and risk. A small mechanical diff does not need the most capable model; a subtle concurrency change does.
Always specify the model explicitly when dispatching a subagent. An omitted model inherits your session's model — often the most capable and most expensive — which silently defeats this section.
Turn count beats token price. Wall-clock and context cost scale with how many turns a subagent takes, and the cheapest models routinely take 2-3× the turns on multi-step work — costing more overall. Use a mid-tier model as the floor for reviewers and for implementers working from prose descriptions. When the task's plan text contains the complete code to write, the implementation is transcription plus testing: use the cheapest tier for that implementer. Single-file mechanical fixes also take the cheapest tier.
Task complexity signals (implementation tasks):
- Touches 1-2 files with a complete spec → cheap model
- Touches multiple files with integration concerns → standard model
- Requires design judgment or broad codebase understanding → most capable model
Handling Implementer Status
Implementer subagents report one of four statuses. Handle each appropriately:
DONE: Generate the review package (scripts/review-package BASE HEAD, from this skill's directory — it prints the unique file path it wrote; BASE is the commit you recorded before dispatching the implementer — never HEAD~1, which silently drops all but the last commit of a multi-commit task), then dispatch the task reviewer with the printed path.
DONE_WITH_CONCERNS: The implementer completed the work but flagged doubts. Read the concerns before proceeding. If the concerns are about correctness or scope, address them before review. If they're observations (e.g., "this file is getting large"), note them and proceed to review.
NEEDS_CONTEXT: The implementer needs information that wasn't provided. Provide the missing context and re-dispatch.
BLOCKED: The implementer cannot complete the task. Assess the blocker:
- If it's a context problem, provide more context and re-dispatch with the same model
- If the task requires more reasoning, re-dispatch with a more capable model
- If the task is too large, break it into smaller pieces
- If the plan itself is wrong, escalate to the human
Never ignore an escalation or force the same model to retry without changes. If the implementer said it's stuck, something needs to change.
Handling Reviewer ⚠️ Items
The task reviewer may report "⚠️ Cannot verify from diff" items — requirements that live in unchanged code or span tasks. These do not block the rest of the review, but you must resolve each one yourself before marking the task complete: you hold the plan and cross-task context the reviewer lacks. If you confirm an item is a real gap, treat it as a failed spec review — send it back to the implementer and re-review.
Constructing Reviewer Prompts
Per-task reviews are task-scoped gates. The broad review happens once, at the final whole-branch review. When you fill a reviewer template:
- Do not add open-ended directives like "check all uses" or "run race tests if useful" without a concrete, task-specific reason
- Do not ask a reviewer to re-run tests the implementer already ran on the same code — the implementer's report carries the test evidence
- Do not pre-judge findings for the reviewer — never instruct a reviewer to ignore or not flag a specific issue. If you believe a finding would be a false positive, let the reviewer raise it and adjudicate it in the review loop. If the prompt you are writing contains "do not flag," "don't treat X as a defect," "at most Minor," or "the plan chose" — stop: you are pre-judging, usually to spare yourself a review loop.
- The global-constraints block you hand the reviewer is its attention lens. Copy the binding requirements verbatim from the plan's Global Constraints section or the spec: exact values, exact formats, and the stated relationships between components ("same layout as X", "matches Y"). The reviewer's template already carries the process rules (YAGNI, test hygiene, review method) — the constraints block is for what THIS project's spec demands.
- Hand the reviewer its diff as a file: run this skill's
scripts/review-package BASE HEADand pass the reviewer the file path it prints (or, without bash:git log --oneline,git diff --stat, andgit diff -U10for the range, redirected to one uniquely named file). The output never enters your own context, and the reviewer sees the commit list, stat summary, and full diff with context in one Read call. Use the BASE you recorded before dispatching the implementer — neverHEAD~1, which silently truncates multi-commit tasks. - A dispatch prompt describes one task, not the session's history. Do not paste accumulated prior-task summaries ("state after Tasks 1-3") into later dispatches — a real session's dispatch hit 42k chars of which 99% was pasted history. A fresh subagent needs its task, the interfaces it touches, and the global constraints. Nothing else.
- Dispatch fix subagents for Critical and Important findings. Record Minor findings in the progress ledger as you go, and point the final whole-branch review at that list so it can triage which must be fixed before merge. A roll-up nobody reads is a silent discard.
- A finding labeled plan-mandated — or any finding that conflicts with what the plan's text requires — is the human's decision, like any plan contradiction: present the finding and the plan text, ask which governs. Do not dismiss the finding because the plan mandates it, and do not dispatch a fix that contradicts the plan without asking.
- The final whole-branch review gets a package too: run
scripts/review-package MERGE_BASE HEAD(MERGE_BASE = the commit the branch started from, e.g.git merge-base main HEAD) and include the printed path in the final review dispatch, so the final reviewer reads one file instead of re-deriving the branch diff with git commands. - Every fix dispatch carries the implementer contract: the fix subagent re-runs the tests covering its change and reports the results. Name the covering test files in the dispatch — a one-line fix does not need the whole suite. Before re-dispatching the reviewer, confirm the fix report contains the covering tests, the command run, and the output; dispatch the re-review once all three are present.
- If the final whole-branch review returns findings, dispatch ONE fix subagent with the complete findings list — not one fixer per finding. Per-finding fixers each rebuild context and re-run suites; a real session's final-review fix wave cost more than all its tasks combined.
File Handoffs
Everything you paste into a dispatch prompt — and everything a subagent prints back — stays resident in your context for the rest of the session and is re-read on every later turn. Hand artifacts over as files:
- Task brief: before dispatching an implementer, run this skill's
scripts/task-brief PLAN_FILE N— it extracts the task's full text to a uniquely named file and prints the path. Compose the dispatch so the brief stays the single source of requirements. Your dispatch should contain: (1) one line on where this task fits in the project; (2) the brief path, introduced as "read this first — it is your requirements, with the exact values to use verbatim"; (3) interfaces and decisions from earlier tasks that the brief cannot know; (4) your resolution of any ambiguity you noticed in the brief; (5) the report-file path and report contract. Exact values (numbers, magic strings, signatures, test cases) appear only in the brief. - Report file: name the implementer's report file after the brief
(brief
…/task-N-brief.md→ report…/task-N-report.md) and put it in the dispatch prompt. The implementer writes the full report there and returns only status, commits, a one-line test summary, and concerns. - Reviewer inputs: the task reviewer gets three paths — the same brief file, the report file, and the review package — plus the global constraints that bind the task.
- Fix dispatches append their fix report (with test results) to the same report file and return a short summary; re-reviews read the updated file.
Durable Progress
Conversation memory does not survive compaction. In real sessions, controllers that lost their place have re-dispatched entire completed task sequences — the single most expensive failure observed. Track progress in a ledger file, not only in todos.
- At skill start, check for a ledger:
cat "$(git rev-parse --show-toplevel)/.superpowers/sdd/progress.md". Tasks listed there as complete are DONE — do not re-dispatch them; resume at the first task not marked complete. - When a task's review comes back clean, append one line to the ledger in
the same message as your other bookkeeping:
Task N: complete (commits <base7>..<head7>, review clean). - The ledger is your recovery map: the commits it names exist in git even
when your context no longer remembers creating them. After compaction,
trust the ledger and
git logover your own recollection. git clean -fdxwill destroy the ledger (it's git-ignored scratch); if that happens, recover fromgit log.
Prompt Templates
- implementer-prompt.md - Dispatch implementer subagent
- task-reviewer-prompt.md - Dispatch task reviewer subagent (spec compliance + code quality)
- Final whole-branch review: use superpowers:requesting-code-review's code-reviewer.md
Example Workflow
Advantages
vs. Manual execution:
- Subagents follow TDD naturally
- Fresh context per task (no confusion)
- Parallel-safe (subagents don't interfere)
- Subagent can ask questions (before AND during work)
vs. Executing Plans:
- Same session (no handoff)
- Continuous progress (no waiting)
- Review checkpoints automatic
Efficiency gains:
- Controller curates exactly what context is needed; bulk artifacts move as files, not pasted text
- Subagent gets complete information upfront
- Questions surfaced before work begins (not after)
Quality gates:
- Self-review catches issues before handoff
- Task review carries two verdicts: spec compliance and code quality
- Review loops ensure fixes actually work
- Spec compliance prevents over/under-building
- Code quality ensures implementation is well-built
Cost:
- More subagent invocations (implementer + reviewer per task)
- Controller does more prep work (extracting all tasks upfront)
- Review loops add iterations
- But catches issues early (cheaper than debugging later)
Red Flags
Never:
- Start implementation on main/master branch without explicit user consent
- Skip task review, or accept a report missing either verdict (spec compliance AND task quality are both required)
- Proceed with unfixed issues
- Dispatch multiple implementation subagents in parallel (conflicts)
- Make a subagent read the whole plan file (hand it its task brief —
scripts/task-brief— instead) - Skip scene-setting context (subagent needs to understand where task fits)
- Ignore subagent questions (answer before letting them proceed)
- Accept "close enough" on spec compliance (reviewer found spec issues = not done)
- Skip review loops (reviewer found issues = implementer fixes = review again)
- Let implementer self-review replace actual review (both are needed)
- Tell a reviewer what not to flag, or pre-rate a finding's severity in the dispatch prompt ("treat it as Minor at most") — the plan's example code is a starting point, not evidence that its weaknesses were chosen
- Dispatch a task reviewer without a diff file — generate it first
(
scripts/review-package BASE HEAD) and name the printed path in the prompt - Move to next task while the review has open Critical/Important issues
- Re-dispatch a task the progress ledger already marks complete — check
the ledger (and
git log) after any compaction or resume
If subagent asks questions:
- Answer clearly and completely
- Provide additional context if needed
- Don't rush them into implementation
If reviewer finds issues:
- Implementer (same subagent) fixes them
- Reviewer reviews again
- Repeat until approved
- Don't skip the re-review
If subagent fails task:
- Dispatch fix subagent with specific instructions
- Don't try to fix manually (context pollution)
Integration
Required workflow skills:
- superpowers:using-git-worktrees - Ensures isolated workspace (creates one or verifies existing)
- superpowers:writing-plans - Creates the plan this skill executes
- superpowers:requesting-code-review - Code review template for the final whole-branch review
- superpowers:finishing-a-development-branch - Complete development after all tasks
Subagents should use:
- superpowers:test-driven-development - Subagents follow TDD for each task
Alternative workflow:
- superpowers:executing-plans - Use for parallel session instead of same-session execution
ARGUMENTS: Execute docs/superpowers/plans/2026-07-09-trail-checkout-worktree.md task-by-task with fresh subagents per task
Task 1 fix landed (amended commit 5e630a99b7, tests 3/3, lint clean). Re-review is running in the background; Task 2's brief is prepared. I'll dispatch Task 2 as soon as the re-review confirms clean, since it modifies the same files.
Re-review running in the background — I'll report the verdict and the commit question for the fix wave when it lands.
Committed: 93556b593c harden trail worktree checkout edge cases. The branch now carries six commits, all reviewed, tests and lint green. Run concluded.
Final state of feat/entire-trail-checkout-worktree:
- Six commits, ending at
93556b593c; only the pre-existing.opencode/package-lock.jsondiff remains in the working tree (untouched, as found). - Spec and plan docs live uncommitted under
docs/superpowers/per your plans rule; the spec's reuse section now documents the already-checked-out-elsewhere contract. - Open ship-as-is minors are recorded in
.superpowers/sdd/progress.mdif you ever want a polish pass — the two worth remembering: fetched remote-only branches get no upstream tracking (agit push -uaway), and the.worktreeincludecandidate scan enumerates all ignored files, which could crawl in anode_modules-heavy repo.
When you're ready to publish, say the word and the PR flow will handle the rest.
/simplify → 4 cleanup agents in parallel → apply the fixes
You are improving the quality of the changed code, not hunting for bugs. Review
it for reuse, simplification, efficiency, and altitude issues, then fix what you
find. Do not look for correctness bugs — that is what /code-review is for.
Phase 0 — Gather the diff
Run git diff @{upstream}...HEAD (or git diff main...HEAD / git diff HEAD~1
if there's no upstream) to get the unified diff under review. If there are
uncommitted changes, or the range diff is empty, also run git diff HEAD and
include the working-tree changes in scope — the review often runs before the
commit. If a PR number, branch name, or file path was passed as an argument,
review that target instead. Treat this diff as the review scope.
Phase 1 — Review (4 cleanup agents in parallel)
Launch 4 independent review agents via the Agent tool, all in a
single message so they run concurrently. Pass each agent the diff and one of
the four angles below. Each returns its findings with file, line, a
one-line summary, and the concrete cost (what is duplicated, wasted, or
harder to maintain).
Reuse
Flag new code that re-implements something the codebase already has — Grep shared/utility modules and files adjacent to the change, and name the existing helper to call instead.
Simplification
Flag unnecessary complexity the diff adds: redundant or derivable state, copy-paste with slight variation, deep nesting, dead code left behind. Name the simpler form that does the same job.
Efficiency
Flag wasted work the diff introduces: redundant computation or repeated I/O, independent operations run sequentially, blocking work added to startup or hot paths. Also flag long-lived objects built from closures or captured environments — they keep the entire enclosing scope alive for the object's lifetime (a memory leak when that scope holds large values); prefer a class/struct that copies only the fields it needs. Name the cheaper alternative.
Altitude
Check that each change is implemented at the right depth, not as a fragile bandaid. Special cases layered on shared infrastructure are a sign the fix isn't deep enough — prefer generalizing the underlying mechanism over adding special cases.
Phase 2 — Apply the fixes
Wait for all four agents to complete, dedup findings that point at the same line or mechanism, and fix each remaining one directly. Skip any finding whose fix would change intended behavior, require changes well outside the reviewed diff, or that you judge to be a false positive — note the skip rather than arguing with it. Finish with a brief summary of what was fixed and what was skipped (or confirm the code was already clean).
Simplify pass complete — net −34 lines, all verified. Full package test green (30s), mise run fmt && mise run lint clean.
Fixed (7 findings, deduped from 4 agents):
shellQuotePathdeleted — the package already had a byte-identicalshellQuoteinresume_picker.go, with its own test; our duplicate function and test are gone.findWorktreeForBranchno longer hand-parses porcelain — it now calls the existingparseWorktreeForBranch(which documents the detached-HEAD/block-boundary subtleties) with an emptycurrentRootso it matches any worktree, keeping only the managed-dir classification.fetchTrailWorktreeBranchroutes throughremote.Fetchlike every other fetch in the codebase — regainingGIT_TERMINAL_PROMPT=0,--no-auto-gc, and the consistent "fetch timed out after 2 minutes" message.appendIgnoreRulelost its always-constantruleparameter — the const lives inside, and the//nolint:unparamsuppression is gone.- Root resolved once —
checkoutTrailWorktreeresolves the main root a single time and threads it intofindWorktreeForBranch; the exclude path derives<root>/.gitinstead of a thirdgit rev-parsespawn (was 2–3 spawns per invocation). runTrailWorktreeGitdeleted — tests use the existingrunGithelper fromfetch_no_config_pollution_test.go.slices.Equalreplaces two hand-rolled slice comparisons in tests.
Skipped (3, with reasons):
- Reusing
session.GetGitCommonDir— false positive: itsfilepath.Join(".", dir)never absolutizes, so swapping it in would turn thecdhints into relative paths. Our absolutizing variant stays. - Unifying
ensureTrailWorktreeBranchAvailablewithswitchToBranchForResume— real duplication of the check-local/check-remote/prompt flow, but the fix means refactoring resume machinery well outside this diff, and the fetch actions genuinely differ. Left as a noted future cleanup. - Consolidating six tests onto
newTrailWorktreeTestRepo— would add commits and cache-clears those tests don't need, to save ~12 lines. Not worth the coupling.
The changes are uncommitted in the working tree (touching trail_checkout_worktree.go and its test). Want them as a 7th commit — e.g. simplify trail worktree checkout internals — or folded elsewhere?
Base directory for this skill: /Users/pfleidi/.agents/skills/pfleidi/skills/review
Code Review
1. Determine Diff Against origin/main
Always review changes against origin/main directly. Do not use local main, git merge-base, a PR base branch, or a user-specified alternate base.
Get the CURRENT diff — never use cached results from earlier in the conversation. Include both committed and uncommitted changes (staged + unstaged):
Note: git diff origin/main (not origin/main...HEAD) diffs origin/main against the working tree, capturing committed changes, staged changes, and unstaged changes in one pass.
Show the user the file list and total count. If there are 0 changed files, report that to the user and stop — there is nothing to review. Otherwise, immediately proceed to the review agents. Do NOT wait for confirmation.
Before launching agents, build a concise review context and pass it to every agent. Show the context as a table before launching agents so assumptions are visible:
| Context | Source | Value |
|---|---|---|
| User goal | Conversation | One-line summary, or not provided |
| Implementation plan | Conversation / docs | One-line summary, or not provided |
| PR context | PR title/body | One-line summary, or no PR found |
| Commits | git log --oneline origin/main..HEAD | One-line summary of commit intent |
| Changed surface | diff file list | Main packages/files touched |
| Inferred behavior | commits/tests/docs/user text | Intended behavior change, or diff-only inference |
- The user's request and any implementation plan, design notes, or acceptance criteria provided in the conversation.
- Branch commit messages from
git log --oneline origin/main..HEAD. - PR title/body when a PR exists for the branch.
- The changed-file list and any obvious intended behavior changes inferred from commits, tests, docs, or user-facing text.
Treat this context as the statement of intent. If no implementation plan or PR context exists, say that intent is inferred from the diff and commits only.
2. Spawn Parallel Review Agents
Review Philosophy
Pass these rules to every agent:
- It is OK to find nothing. A clean review is a valid outcome. Do NOT manufacture findings to justify the review. Only flag issues you are confident are real problems.
- Be opinionated and consistent. If a pattern is acceptable, don't flag it. If you flag something, commit to that position — don't suggest the opposite approach on a re-review.
- Don't flag trade-offs with no clear winner. If there are two reasonable approaches and neither is clearly better, don't flag it. The author already made a choice.
- High confidence only. Every finding must pass the bar: "I am confident this is a problem, and I can explain specifically what goes wrong if it's not fixed." Vague unease is not a finding.
- Permission-friendly reads. Avoid shell pipelines, command separators, subshells, and output filters for read-only investigation because they create extra permission prompts and block background review agents. Do not run commands like
git show HEAD:path | sed -n '10,40p'. Use workspace file range reads,rgwith path limits, path-scopedgit diff $BASE -- <path>, or one standalonegit show <rev>:<path>only when the output is acceptably small. - Intent-aware review. Review changed code against the review context, not against the old behavior alone. Do not classify an intentional behavior change as Required merely because it differs from
origin/main. A Required finding must either contradict stated intent, break an existing contract that the intent did not change, introduce a concrete bug/security issue, or leave the intended behavior unverified in a way that would likely fail.
Launch four baseline sub-agents in parallel using the Agent tool. Pass each agent origin/main as the base ref, the full list of changed files, the review context, and the review philosophy above.
When the repository is a Go project and the diff includes Go-related files (*.go, go.mod, or go.sum), also launch Agent 5 in the same batch. Do not run the Go-specific agent for non-Go diffs.
Agent 1: Security & Adversarial
Review git diff $BASE with fresh eyes for:
- Injection — command injection, SQL injection, path traversal
- TOCTOU and race conditions — check-then-act patterns, concurrent access without synchronization
- Unvalidated input at system boundaries — user input, API parameters, external data
- Auth/authz gaps — missing permission checks, privilege escalation paths
- Secrets or credentials — hardcoded tokens, leaked keys, credentials in code or config
For EACH finding: read the actual source file and trace whether the code path is reachable in production. Discard any finding you cannot confirm with a concrete code reference.
Agent 2: Correctness & Quality
Review git diff $BASE for:
- Logic errors — off-by-one, wrong comparison, inverted conditions
- Nil/null handling — unchecked nil dereferences, missing error checks (especially unchecked errors in Go)
- Edge cases in concurrency — goroutine leaks, missing locks, channel misuse, deferred unlock ordering
- Redundant state — state that duplicates existing state, cached values that could be derived
- Production test seams — mutable function variables, package-wide settings, reset hooks, or exported knobs added only so tests can swap behavior instead of using dependency injection or a higher-scope test
- Parameter sprawl — adding new parameters instead of restructuring
- Leaky abstractions — exposing internal details, breaking existing abstraction boundaries
- Stringly-typed code — using raw strings where constants or typed values already exist in the codebase
- Test coverage and scope gaps — changed behavior, edge cases, or error paths not exercised by meaningful tests; tests that prove implementation details instead of behavior; or unit tests used where integration/e2e coverage is the right confidence boundary
- Test helper over-abstraction — helpers that hide the behavior, expected values, or assertions and make the test harder to understand than a small amount of duplication
For EACH finding: verify the claim by reading the source. Check call sites to confirm the issue is real, not hypothetical.
Agent 3: Simplification & De-slop
Use the local pfleidi:de-slop skill's slop taxonomy as the source of truth: skills/pfleidi/skills/de-slop/SKILL.md ("What Counts as Slop"). Apply its criteria as a detection lens only — find and report slop; do not run de-slop's remediation workflow, create commits, or open a PR.
Review git diff $BASE for:
- Dead code — unreachable branches, unused functions, struct fields never read, data computed but never used
- Reinvention — hand-rolled solutions to problems already solved by the repo, the standard library, or a dependency in use; name the existing mechanism to use instead
- Code reuse & duplication — existing utilities and helpers that could replace newly written code; near-duplicate blocks that should be unified
- Unnecessary abstractions — wrapper types, indirection, single-caller layers, or overly defensive fallbacks that mask errors
- Premature optimization — complexity added for performance no one measured; prefer the clear version unless a bottleneck was shown
- Unnecessary work — redundant computations, repeated file reads, duplicate API calls, N+1 patterns
- Missed concurrency — independent operations run sequentially when they could be parallel
- Hot-path bloat — blocking work added to startup or per-request paths
- Unnecessary existence checks — pre-checking file/resource existence before operating (TOCTOU anti-pattern); operate directly and handle the error
- Unnecessary comments — comments explaining WHAT the code does (well-named identifiers already do that); keep only non-obvious WHY
For EACH suggestion: verify it does not break existing behavior by checking call sites and usages. Discard cosmetic-only suggestions (renames, formatting).
Agent 4: Readability & Go Idioms
Review git diff $BASE for code that is hard to read, maintain, or reason about:
- Poor factoring — functions doing multiple jobs, tangled control flow, or missing helper extraction where a small local helper would clarify behavior
- Mixed abstraction levels — high-level orchestration mixed with low-level IO, parsing, protocol, or data-structure details; low-level helpers that also make workflow or policy decisions
- Generated-code smell — repetitive pasted logic, shallow wrappers, generic names, or code that reads like it was assembled without domain intent
- Data-flow opacity — values transformed across too many steps, unclear ownership, hidden mutation, pass-through helper chains, or state threaded through unrelated code
- Control-flow complexity — deeply nested conditionals, boolean flag plumbing, early returns used inconsistently, or error paths that obscure the main path
- Naming clarity — names that hide domain meaning or force callers to inspect implementation to understand usage
- Go API readability — ambiguous
(result, bool)returns outside clear comma-ok/presence checks, oversized interfaces, unnecessary pointer indirection, or cleverness where explicit Go would be clearer - Error readability — errors that lose operation/context, wrap inconsistently, or make call sites branch on strings/booleans instead of clear errors or typed status
For EACH finding: explain the readability cost in concrete maintenance terms. Prefer small, local refactor suggestions. Discard formatting-only, gofmt-only, or personal taste comments.
Agent 5: Clean Go & Modern Go (Go diffs only)
Use the local pfleidi:clean-go skill as the source of truth: skills/pfleidi/clean-go/SKILL.md.
Review only changed Go code plus surrounding source, tests, interfaces, and call sites needed to verify findings. Apply the skill's Clean Go checks and version-gated Modern Go checks. This includes the modern-go guidance incorporated from JetBrains' use-modern-go skill: detect the relevant go.mod target version, only suggest features available for that version, and do not perform blanket modernization.
Focus on concrete changed-code findings around composable functions, abstraction level, function size/signatures, errors, pointers, small interfaces, any/interface{}, testing guidance from skills/pfleidi/testing/SKILL.md, and modern standard-library helpers. Discard findings that would merely restyle existing code or require a broad rewrite unrelated to the current diff.
Second-Pass Coverage Sweep
After the first-pass agents complete, run a second independent review pass before synthesis. The goal is recall: catch high-confidence findings that the lens-specific agents may have missed.
Launch one fresh coverage agent with origin/main as the base ref, the full list of changed files, the review context, and the review philosophy above. Do not pass the first-pass findings to this agent.
Ask the coverage agent to:
- Re-read the changed files and the surrounding code needed to understand each changed path.
- Trace changed behavior through callers, callees, tests, configuration, migrations, generated interfaces, and user/API entry points where relevant.
- Search the repository for related patterns, duplicated logic, and existing helpers that affect the changed code.
- Look across all lenses together: security, correctness, tests, de-slop, readability, performance, and Go cleanliness when applicable.
- Prioritize missed Required findings over optional improvements.
- Return only high-confidence findings with concrete file:line evidence and a short explanation of the traced path.
Then compare the second-pass findings with the first-pass findings. Deduplicate overlaps, verify any new claim by reading source yourself, and discard anything that cannot be confirmed.
3. Synthesize Report
After all launched agents complete:
- Collect findings from both the first-pass agents and the second-pass coverage sweep
- Deduplicate — merge findings from different agents that point to the same underlying issue
- Verify — for any finding where the agent did not cite a specific file:line with evidence, read the source and confirm or discard it
- Group by file
- Sort by severity within each file: Critical > High > Medium > Low
Severity Definitions
- Critical — Must fix before merge. Bugs, security vulnerabilities, data loss risk, race conditions with observable impact.
- High — Should fix before merge. Missing error handling, meaningful test gaps, performance issues on hot paths.
- Medium — Worth fixing. Code reuse opportunities, unnecessary complexity, readability problems that make future changes error-prone, minor efficiency improvements.
- Low — Optional. Minor readability improvements or cosmetic suggestions.
Relevance Classification
For each finding, classify as:
- Required — The change does not work correctly without this fix in light of the review context. Bugs, missing error handling that causes failures, security vulnerabilities, race conditions, contradictions of stated intent, or missing tests for intended behavior that would likely fail. The branch should not merge without addressing these.
- Improvement — Valid finding, but the change works correctly without it. Better factoring, clearer Go APIs, using existing helpers, code reuse, unnecessary complexity, style. Worth addressing in a follow-up, not in this branch.
Autofix Eligibility
Mark each Required finding as Autofix eligible or Needs decision:
- Autofix eligible — source-backed, high confidence, minimal fix is clear, no new dependencies, no shared/public interface change, no product/design choice, no broad refactor, and the directly related verification path is clear.
- Needs decision — any Required finding that fails one of the autofix checks, including intentional behavior questions, API shape changes, cross-cutting refactors, or fixes where multiple reasonable approaches exist.
Present findings as compact tables, not prose blocks. Use one summary table for scanning and one details table for evidence and fixes.
Summary table format:
| # | Severity | Sources | Location | Classification | Autofix | Issue | Impact |
|---|---|---|---|---|---|---|---|
| 1 | Medium | correctness + coverage | cmd/entire/cli/checkpoint/v2_committed.go:234 | Required | Eligible | One-sentence problem. | Concrete consequence if not fixed. |
Details table format:
| # | Evidence | Suggested fix | Trade-offs |
|---|---|---|---|
| 1 | Source-backed confirmation from code path, call site, or test gap. | Concrete code change, not vague advice. | One sentence, or None if strictly better. |
Keep table cells short and scannable. Put the smallest useful quote or evidence in the table rather than full paragraphs. Escape | characters inside code or text so the table remains valid Markdown. Use n/a for Autofix on Improvements. The Sources column lists the agents that independently found or confirmed the issue, such as security, correctness, de-slop, readability, clean-go, or coverage.
If no findings exist at a severity level, omit that section.
If there are 0 findings across all agents, report that the review is clean and stop.
4. Present Report and Proceed With Default Fixes
Present findings in two sections:
Required
Table of findings classified as Required, sorted by severity. Include the Autofix value for each finding. Follow it with the details table for those same Required findings.
Improvements (follow-up)
Table of findings classified as Improvement, continuing the numbering. These are presented for awareness but are NOT included in the fix cycle by default. Follow it with the details table for those same Improvement findings.
End with a one-paragraph summary: total required vs improvement findings, overall merge-readiness assessment, and any patterns across files.
Before editing, present a planned-autofix table for Autofix eligible Required findings:
| # | Location | Planned change | Related test/verification | Files expected |
|---|---|---|---|---|
| 1 | path/file.go:42 | Minimal code change to address the finding. | Focused test or lint/build command. | path/file.go, path/file_test.go |
Do not ask the user to choose a mode. Immediately proceed to Step 5 for Autofix eligible Required findings after showing the planned-autofix table. Do not fix Improvements by default.
If there are Required findings but none are Autofix eligible, stop after the report and list the exact decisions needed.
5. Fix Cycle
Scope Rules
- Make the MINIMAL change that addresses the finding
- Keep the diff limited to files and lines directly required by the finding
- First decide whether the finding is local or systemic. Fix at the narrowest correct level; do not add a local workaround that hides a shared/root-cause bug.
- If the finding requires a behavior-changing code fix, add or update the directly related test in the same fix step. Prefer TDD, but complete the focused red-to-green cycle before stopping: write/update the failing test, confirm it fails, implement the fix, confirm the focused test passes. Do not stop after only adding the failing test unless the user explicitly asks.
- Do NOT rename variables, reformat code, or touch lines outside the finding scope
- Do NOT refactor adjacent code, even if it looks related
- Do NOT create any git commits — code changes only
Default Batched Fixes
Fix all Autofix eligible Required findings in report order by default. Do not ask which findings to fix.
Choose an artifact directory using the AGENTS.md temporary artifact rule with agent name pfleidi-review:
- Use
./tmp/pfleidi-review/only when./tmp/already exists and is already ignored. - If no project-local artifact directory is available, do not create file artifacts by default; keep ledger/log/cache information in the response and mark file paths
n/a. Ask before using/tmp/pfleidi-review/or modifying ignore files.
When an artifact directory is available, create a temporary fix ledger at <artifact-dir>/review-<repo-name>-<timestamp>.md before editing. If no artifact directory is available, keep the same ledger fields in the final summary table instead. Update the ledger after each finding with:
- Finding number, status, and source location.
- Files touched.
- What changed and why.
- Related tests or verification commands.
- Rollback notes sufficient for the user to understand how to revert the finding-specific change manually.
For each Autofix eligible finding:
- Read the relevant code to confirm the fix approach
- Re-check eligibility before editing; if the fix is no longer clearly eligible, mark it
Needs decisionand continue to the next finding - Implement the fix — ONLY the code changes for that single finding
- Add or update the directly related test in the same diff when the fix changes behavior; if using TDD, complete red-to-green before moving on; if no test is added, state why
- Keep the diff limited to files and lines directly required by that finding
- If a fix would require changing a function signature in a shared interface, adding a dependency, expanding scope outside the finding, or making an ambiguous product/design choice, skip that finding as
Needs decisionand continue - Track the exact files changed, what changed, and why the change addresses the finding
If a skipped finding has partial edits, remove only your own partial edits for that finding before continuing. If you cannot safely isolate those partial edits, stop and explain the conflict.
After all eligible fixes are applied, proceed directly to Step 6 (Verify Fixes). Do NOT show a diff yet.
6. Verify Fixes
Run the project's compile/build, lint, and test commands scoped to only the changed files and their directly related tests. Use safe background batches for independent validators instead of running every command sequentially.
When selecting verification commands, reuse <artifact-dir>/verification-<repo-name>.md if an artifact directory is available and the cache is fresh under the cache rules from pfleidi:pr; otherwise discover the smallest relevant lint/test/build commands. Update the cache only when an artifact directory is available.
- Build / compile — run a relevant compile/build command when one is discoverable for the changed production code.
- Lint / static analysis — run the project's documented lint task, scoped to the files that were modified by the fixes when the task supports scoping. Prefer lint-specific task wrappers such as
make lintormise run lintover invoking linter binaries directly. Do not use aggregatecheck,ci, orverifytasks unless you have confirmed they only run lint/static analysis. If the documented lint task cannot be scoped, run the smallest relevant project lint task. - Tests — run only the test files that cover the modified code (same package, same module, co-located test files). Do NOT run the full test suite.
If no compile/build command or project lint task exists, state that explicitly instead of assuming an unavailable command.
Run formatters, generators, snapshot updates, or other mutating commands alone before validators that depend on their output. Run independent read-only validators concurrently when they do not require the same exclusive service, port, database, fixture directory, or generated output. Keep integration/e2e/service-backed commands separate unless the project documents that they are parallel-safe.
For each background batch, start every command from the same working-tree state, capture stdout/stderr/exit status from the tool, do not edit files while the batch is running, and wait for every command to finish. Run each selected validator directly, for example mise run lint, go test ..., or npm test -- .... Do not wrap validators in sh -c, shell redirection, tee, command separators, or pipelines solely to write logs; that defeats command-prefix approvals and causes extra permission prompts. If an artifact directory is available and file logs can be written after the command completes without rerunning through a shell wrapper, save them under <artifact-dir>/logs-<repo-name>-<timestamp>/; otherwise mark the full-log path as n/a. If files change after a failed batch, none of that batch's successful results count as current verification.
Show verification as a compact table:
| Command | Exit | Relevant output | Full log |
|---|---|---|---|
go test ./pkg/foo -run TestBar -count=1 | 0 | Short success excerpt. | <artifact-dir>/logs-.../go-test-pkg-foo.log or n/a |
For failures or short outputs, show complete output in the relevant-output column or immediately below the table. For long successful outputs, show the relevant excerpt and log path.
On Failure: Auto-Fix
If compile/build, lint, or tests fail due to issues introduced by the review fixes:
- Read the error output and identify every failure
- Fix all issues — apply the minimal changes needed
- Re-run the failing commands using the same safe batching rules
- Show the complete output again
Cap at 2 fix attempts. If still failing after 2 rounds, present the remaining failures to the user with full output. Do not describe the work as done or commit-ready while compile/build status is failing or unknown for changed production code.
7. Present Results
Once verification passes (or the user has seen the remaining failures), present a summary table. If compile/build did not pass for code changes, say the work is not commit-ready before the table. Do NOT show a diff — the Edit tool already showed each change inline.
| # | Finding | File | Status | What changed | Why |
|---|---|---|---|---|---|
| 1 | Description | path:line | Fixed | Added nil check in foo | Prevents panic on missing config |
| 3 | Description | path:line | Fixed | Added focused test for invalid input | Verifies intended error path |
| 6 | Description | path:line | Needs decision | None | API shape has two reasonable options |
Also present a rollback/traceability table and the ledger path:
| # | Files touched | Rollback notes |
|---|---|---|
| 1 | path/file.go, path/file_test.go | Revert the nil-check and focused test added for finding #1. |
Ledger: <artifact-dir>/review-<repo-name>-<timestamp>.md, or n/a when no project-local artifact directory was available.
STOP and wait. The user decides what to commit.
8. Post-Fix
- If the user asks for changes, apply them
- If the user asks to revert specific fixes, revert ONLY those changes using surgical git checkout of the affected lines
- Do NOT commit. The user handles commits on their own terms.
All passes complete, everything verified against source. The verdict is unusually pleasant, Crawler:
Review Report — feat/entire-trail-checkout-worktree vs origin/main
Security: clean (injection, traversal, symlink, TOCTOU, and quoting all traced and confirmed guarded). No Required findings — nothing contradicts the spec, breaks a contract, or is a concrete bug on a reachable normal path. Everything below is classified Improvement (follow-up material, not blocking this branch).
Improvements (follow-up)
| # | Severity | Sources | Location | Classification | Autofix | Issue | Impact |
|---|---|---|---|---|---|---|---|
| 1 | Medium | correctness | trail_checkout_worktree.go:28 | Improvement | n/a | Unnumbered trails (Number <= 0) get no trail-<n>- prefix, so two unnumbered trails whose branches sanitize identically (feature/test vs feature-test) collide on one directory. | Second checkout fails with a raw git worktree add error instead of a clear message. Rare precondition; sibling update/delete commands guard unnumbered trails explicitly. |
| 2 | Medium | coverage | trail_checkout_worktree.go:330 | Improvement | n/a | Reuse path trusts git worktree list --porcelain without checking the directory still exists. | A hand-deleted (unpruned) worktree yields "Worktree already exists" + a cd hint to a missing path. Mirrors git's own stale-worktree model, but a stat + prune hint would be friendlier. |
| 3 | Medium | readability | trail_checkout_worktree.go:50 | Improvement | n/a | gitCommonDirForTrailWorktree looks like a duplicate of session.GetGitCommonDir with no note on why it isn't. | Next maintainer must rediscover that the session helper never absolutizes relative paths (its filepath.Join(".", dir) is a no-op) — one why-comment prevents a bad "cleanup". |
| 4 | Low | de-slop | trail_checkout_worktree.go:256 | Improvement | n/a | cleanRelativeIncludeFile inlines the ..-traversal check that paths.IsRelativeTraversal centralizes. | Duplicated safety logic; the helper also rejects backslash-prefixed traversal. |
| 5 | Low | readability | trail_checkout_worktree.go:251 | Improvement | n/a | isManagedTrailWorktreePath filter has no why-comment. | It prevents sibling worktrees' ignored files (their .envs appear in ls-files --ignored once .entire/worktrees/ is ignored) from being copied into new worktrees — non-obvious, deletable-looking. |
| 6 | Low | readability + clean-go | trail_checkout_worktree.go:371 | Improvement | n/a | (path, managed, found, err) 4-value return; managed is meaningless when !found. | Both agents agree it's fine at the single current call site; a match-struct would guard future call sites. |
| 7 | Low | clean-go | trail_checkout_worktree.go:138,206 | Improvement | n/a | strings.Split used purely for iteration; Go 1.26 has strings.SplitSeq. | Avoids an intermediate slice; cosmetic-adjacent. |
| 8 | Low | correctness | trail_checkout_worktree.go:104 | Improvement | n/a | Interactive .gitignore-confirm branch has no direct test (huh confirm can't run under go test). | Spec-accepted gap; covered transitively via appendIgnoreRule tests. Awareness only. |
Details
| # | Evidence | Suggested fix | Trade-offs |
|---|---|---|---|
| 1 | trail_cmd.go:1149 and :1417 reject unnumbered trails for update/delete; the worktree call site (trail_cmd.go:1297) passes found.Number through unguarded. | Either reject Number <= 0 with the sibling message, or disambiguate unnumbered names (short-id suffix). | Rejecting blocks a legitimate (if rare) unnumbered-trail checkout — product choice, hence not autofixed. |
| 2 | findWorktreeForBranch returns the porcelain path unstat'ed; git keeps stale entries until worktree prune. | os.Stat the reuse path; on ENOENT print a git worktree prune hint instead of the cd hint. | Multiple reasonable shapes (auto-prune vs hint). |
| 3 | session/state.go:768: filepath.Join(".", commonDir) — never absolutizes; our variant joins against cwd. | One-line comment on gitCommonDirForTrailWorktree stating why the session helper is unsuitable; optionally fix the shared helper separately. | None. |
| 4 | paths/paths.go:148 exists; cli package already imports paths. | if clean == "." || paths.IsRelativeTraversal(clean) { return "", false }. | None — behavior-preserving per existing tests. |
| 5 | Filter is load-bearing only after the ignore rule ships — invisible from the code alone. | Add the one-line why-comment the readability agent drafted. | None. |
| 6 | Single call site at :326 handles it correctly today. | Optional worktreeMatch struct; clean-go agent recommends no action now. | Churn for a hypothetical second caller. |
| 7 | LSP stringsseq hints agree; repo lint doesn't enforce it. | for line := range strings.SplitSeq(...). | None. |
| 8 | interactive.CanPromptInteractively() is hard-false under go test. | Accept, or add an injectable confirm seam — which the repo's own rules discourage as a test-only production hook. | Testing the branch would violate the no-test-seams rule. |
Summary: 0 Required, 8 Improvements (3 Medium, 5 Low). The branch is merge-ready as it stands — security-clean across two independent passes, real-behavior test coverage of every mainline path, and the only pattern across findings is "rare edge states could fail friendlier" plus two missing why-comments. Since no Required findings exist, there is no fix cycle to run; per the review contract I'm stopping here. If you want any of the Improvements (the two why-comments, #4, and #7 are five-minute batch material; #1 and #2 need your product call on the fix shape), say which.
Ignore 8, address 3-7. Once that's done present me 1 and 2 in more detail so we can chat about them
Findings 3–7 are in — lint and focused tests green, changes left uncommitted for your review. What changed: why-comments on gitCommonDirForTrailWorktree (why session.GetGitCommonDir is unsuitable) and isManagedTrailWorktreePath (the sibling-worktree .env leak it prevents), paths.IsRelativeTraversal replacing the inline traversal check, findWorktreeForBranch now returning a trailWorktreeMatch struct + found bool (the linter's nilnil rule vetoed the pointer-nil variant), and the three split-loops moved to strings.SplitSeq/bytes.SplitSeq.
Now the two you wanted to discuss, Crawler — both are "rare state meets blunt error" cases:
Finding 1 — Unnumbered-trail worktree collision
The mechanics. defaultTrailWorktreePath builds trail-<number>-<sanitized-branch>, but when the trail has no number yet (found.Number <= 0) the spec deliberately drops the prefix, leaving just the sanitized branch name. Sanitization maps / and friends to -, so it's lossy: feature/test and feature-test both become feature-test.
The failure. Two distinct unnumbered trails whose branches sanitize identically: the first checkout creates .entire/worktrees/feature-test; the second passes the branch-already-checked-out check (different branch!) and dies inside git worktree add with a raw fatal: '.entire/worktrees/feature-test' already exists — after the ignore rule was written and "Checking out …" printed. Notably, the sibling commands already treat unnumbered trails as second-class: trail update and trail delete refuse them outright with trail for branch %q has no number yet (trail_cmd.go:1149, :1417).
Options:
- (a) Guard like the siblings — refuse
--worktreefor unnumbered trails with the same message. Two lines, consistent, but blocks a legitimate (if rare) checkout that would usually work fine. - (b) Disambiguate the name — when unnumbered, append a short stable suffix (e.g. first 6 hex of a hash of the raw branch):
feature-test-3f9a2c. Nothing breaks, names get uglier, and the same branch still maps to the same dir on reuse. - (c) Just improve the error — wrap the
git worktree addfailure with "worktree directory already exists at<path>— is another trail's branch using it?". No behavior change, collision stays but stops being cryptic.
My lean: (b) if you expect unnumbered trails to be a real state users hit, (c) if they're transient enough that update/delete's refusal pattern suggests nobody lingers there. Option (a) feels heavy — this command doesn't actually need the number.
Finding 2 — Stale registered worktree
The mechanics. If someone rm -rfs a managed worktree instead of git worktree remove, git keeps the registration until a prune — git worktree list --porcelain still lists it (flagged with a prunable attribute we don't parse). Our reuse check matches it and cheerfully prints "Worktree already exists at <path>" plus a cd hint into the void.
The kicker: skipping the match wouldn't help by itself — git worktree add would then refuse because the branch is still "used by" the stale registration. So the state is genuinely wedged until a prune, whoever performs it.
Options:
- (a) Detect + hint —
os.Statthe matched path; if gone, error with "worktree for branch<b>is registered but its directory is missing — rungit worktree prune". Honest, no magic, user runs one command. - (b) Self-heal for managed worktrees — if the missing worktree lives under
.entire/worktrees/(our directory, our mess), rungit worktree pruneand fall through to fresh creation; non-managed stale matches still get the (a) error. Best UX, but it's the CLI silently mutating git worktree state — the exact category your global rules treat with suspicion, though here it's the feature's own managed namespace. - (c) Parse
prunable— same outcomes as (b) but decided from the porcelain output instead of a stat; slightly more parsing, no extra syscall race.
My lean: (b) — a .entire/worktrees entry whose directory vanished has exactly one sane interpretation, and making the user learn git worktree prune to unwedge a directory we manage is punting our cleanup to them. But it does cross the "CLI runs git worktree prune" line, so it's genuinely your call.
Where do you want to land on each?
Sorry there must have been a misunderstanding. I meant to choose the option that shows a hint with the prune command, not to execute it immediately. I'd like to give a user a choice and not make decision on their behalf.
Done — hint-only it is. No decisions made on the user's behalf; the System merely gestures at the door.
Finding 1 (option a): checkoutTrailWorktree now rejects unnumbered trails up front with trail for branch "<b>" has no number yet; cannot check out into a worktree (matching the trail update/delete pattern), before any validation, output, or side effects. Since the number is now guaranteed, defaultTrailWorktreePath always prefixes trail-<n>- — the unprefixed branch was dead code and is gone.
Finding 2 (option a, corrected): a stale registration (worktree directory deleted by hand) now fails for managed and non-managed worktrees alike with branch "<b>" is registered to a missing worktree at <path>; run 'git worktree prune' to clear it. The pruneTrailWorktrees helper and auto-heal path are removed — the CLI never runs git worktree prune itself.
Verification: focused TestCheckoutTrailWorktree* + TestDefaultTrailWorktreePath pass (ok … 1.051s), mise run fmt && mise run lint clean. Three tests cover the new behavior: unnumbered rejection (asserts no .entire/worktrees created), stale managed → prune hint + not recreated, stale non-managed → prune hint. The spec's naming and stale-worktree sections were updated to match, including the line "the CLI never prunes on the user's behalf."
Changes are uncommitted (production file, test file, spec doc) — say the word when you want them committed.
Committed: 6d7465ea2e reject unnumbered trails and stale worktrees. The branch now stands at nine commits, tests and lint green. The spec doc stays uncommitted with the rest of docs/superpowers/, per your plans rule.
That closes out both review discussion items — the trail number is now mandatory for worktree checkout, and stale registrations get a git worktree prune hint instead of unsolicited housekeeping. Ready for the PR flow whenever you are, Crawler.
For similicity's sake, I changed my mind on .git/info/exclude: We should only update the .gitignore and if a user wants to persist that ignore rule, they should commit the change. Ideally, I'd like to prompt users as little as possible, especially given that this is setup-only and adds a lot of code complexity.
Generally speaking, I think we shouldn't be as concerned about what is git ignored and what isn't since that's the user's choice.
Done — the whole choice apparatus is gone. Net effect on trail_checkout_worktree.go: the huh import, the confirm form, the exclude writer path, and the force parameter on ensureTrailWorktreeIgnoreRule are all deleted; what remains is check-ignore → silent no-op, otherwise append to .gitignore and print Added .entire/worktrees/ to .gitignore — commit it to keep the rule. The user persists the rule by committing the .gitignore change, or discards it — their repo, their call.
Also updated:
--forcehelp string back to its single meaning (fetch prompt only), and the same in the spec.- Tests:
TestEnsureTrailWorktreeIgnoreRule_AppendsGitignorereplaces the exclude test; the already-ignored no-op test now also asserts.gitignoreis byte-for-byte untouched; the create/unnumbered tests assert against.gitignoreinstead of.git/info/exclude. - Spec section 3 rewritten (two steps, no prompt, "persisting the rule is the user's choice"), and the
--worktreeextra--forcemeaning removed from section 1.
Verification: focused ignore-rule + checkout + append tests pass (ok … 1.104s), mise run fmt && mise run lint clean.
One consequence worth naming: every fresh clone that runs --worktree before the .gitignore rule is committed will dirty .gitignore once. That's the intended trade — one nudge toward committing the shared rule, zero prompts.
Uncommitted and ready for your eyes — commit message suggestion when you want it: always add worktree ignore rule to .gitignore.
Committed: e8a33903ed always add worktree ignore rule to .gitignore — net −36 lines, which is the correct direction for a feature that just lost a decision tree.
The branch sits at ten commits, working tree clean except the perennial .opencode/package-lock.json squatter. Ignore handling is now a single sentence of behavior: not ignored → append to .gitignore, tell the user, move on. Ready for the PR whenever you say so, Crawler.
/simplify → 4 cleanup agents in parallel → apply the fixes
You are improving the quality of the changed code, not hunting for bugs. Review
it for reuse, simplification, efficiency, and altitude issues, then fix what you
find. Do not look for correctness bugs — that is what /code-review is for.
Phase 0 — Gather the diff
Run git diff @{upstream}...HEAD (or git diff main...HEAD / git diff HEAD~1
if there's no upstream) to get the unified diff under review. If there are
uncommitted changes, or the range diff is empty, also run git diff HEAD and
include the working-tree changes in scope — the review often runs before the
commit. If a PR number, branch name, or file path was passed as an argument,
review that target instead. Treat this diff as the review scope.
Phase 1 — Review (4 cleanup agents in parallel)
Launch 4 independent review agents via the Agent tool, all in a
single message so they run concurrently. Pass each agent the diff and one of
the four angles below. Each returns its findings with file, line, a
one-line summary, and the concrete cost (what is duplicated, wasted, or
harder to maintain).
Reuse
Flag new code that re-implements something the codebase already has — Grep shared/utility modules and files adjacent to the change, and name the existing helper to call instead.
Simplification
Flag unnecessary complexity the diff adds: redundant or derivable state, copy-paste with slight variation, deep nesting, dead code left behind. Name the simpler form that does the same job.
Efficiency
Flag wasted work the diff introduces: redundant computation or repeated I/O, independent operations run sequentially, blocking work added to startup or hot paths. Also flag long-lived objects built from closures or captured environments — they keep the entire enclosing scope alive for the object's lifetime (a memory leak when that scope holds large values); prefer a class/struct that copies only the fields it needs. Name the cheaper alternative.
Altitude
Check that each change is implemented at the right depth, not as a fragile bandaid. Special cases layered on shared infrastructure are a sign the fix isn't deep enough — prefer generalizing the underlying mechanism over adding special cases.
Phase 2 — Apply the fixes
Wait for all four agents to complete, dedup findings that point at the same line or mechanism, and fix each remaining one directly. Skip any finding whose fix would change intended behavior, require changes well outside the reviewed diff, or that you judge to be a false positive — note the skip rather than arguing with it. Finish with a brief summary of what was fixed and what was skipped (or confirm the code was already clean).
osroot verified: MkdirAll is a pure pass-through kept as the codebase's documented "consistent helper surface", and osroot.Remove tolerates ENOENT — both fit our call sites, so I'll adopt them. Verdict so far:
- Apply:
osroot.MkdirAll/osroot.RemoveincopyIncludedFile(convention + drops two nolints), and the triple-cleanup → single deferred cleanup in the same function. - Skip:
fetchTrailWorktreeBranchfetch-block dedup — the fix would refactorgit_operations.gooutside the diff, and the altitude agent explicitly judged the current shape correct (two agents, opposite verdicts → author's existing choice stands).ensureTrailWorktreeBranchAvailablevsswitchToBranchForResume— re-flagged despite being on the accepted list; still skipped. - Efficiency: clean bill.
Waiting on the altitude agent to restate its swallowed finding, then I'll apply the fixes in one pass.
Simplify round 2 complete — focused tests pass, fmt + lint clean. The changes are uncommitted.
Fixed (3):
- Right-altitude exclusion —
matchIncludePatternsis now a pure gitignore matcher; the.entire/worktrees/exclusion moved intolistIgnoredFiles, the layer that actually scanned the repo, with the why-comment relocated onto its doc. NewTestListIgnoredFiles_ExcludesManagedWorktreePathspins the behavior against a real repo (the matcher test dropped its now-out-of-place worktree entry). osrootwrappers —copyIncludedFile's destination side uses the codebase's establishedosroot.MkdirAll/osroot.Removehelpers instead of raw*os.Rootmethods.- Single deferred cleanup — the three copy-pasted
_ = destRoot.Remove(rel)failure blocks collapsed into onecopiedflag + deferred close-and-remove; the error paths now just return.
Skipped (2):
fetchTrailWorktreeBranch's fetch-with-timeout block duplicatingFetchAndCheckoutRemoteBranch— deduping means refactoringgit_operations.gooutside the diff, and the altitude agent independently judged the current placement correct (it must land the ref without touching the checkout). Two agents, opposite verdicts; the existing shape stands.ensureTrailWorktreeBranchAvailablevsswitchToBranchForResume— re-flagged despite being on the accepted-trade-offs list; the flows differ materially and the refactor is out of scope. Still skipped.
Efficiency and reuse otherwise came back clean — the round-1 pass had already taken the substantial wins. Commit when ready; suggested subject: move worktree exclusion to ignored-file listing.
About copyIncludedFile(): Shouldn't git worktree add already ensure that the destination path exists? Are we talking about includes multiple levels deep here? Can any of this error handling be simplified?
Good instinct to poke at it — but each piece turns out to be load-bearing. Let me walk through the three questions:
Doesn't git worktree add create the destination directories? Only the ones git knows about. The checkout materializes dest itself plus every directory that contains tracked files. But the files we copy are by definition gitignored, and an ignored file often lives in a directory that holds only ignored content — secrets/.env where the whole secrets/ dir is ignored. Git never creates that directory, so MkdirAll(filepath.Dir(rel)) is doing real work exactly there. That's also your "multiple levels deep" answer: depth doesn't matter per se — one level (sub/.env) already fails without it if sub/ has no tracked files. TestCopyWorktreeIncludeFiles pins this case with a sub/.env that exists nowhere in the branch. For the common rel = ".env" case, Dir is "." and the call is a no-op.
Can the error handling be simplified? It just was, in the pass you haven't committed yet — the three copy-pasted Remove cleanup blocks are now one deferred copied-flag cleanup, and every error path is a bare one-line return. What remains looks decorative but isn't:
- The
Lstat→Open→Stat→SameFiledance enforces "regular files only, never follow symlinks" (the spec's symlink-skip policy) portably — Go has no cross-platformO_NOFOLLOW.Lstatalone rejects symlinks; theSameFilere-check closes the swap race between the check and the open. Since this function's whole diet is secrets-shaped files, that's cheap insurance. Dropping theSameFileline is the one defensible cut if you consider a local symlink-swap race out of your threat model — I'd keep it. ChmodafterCloselooks redundant with the mode passed toOpenFile, but the umask filters creation modes — a source file with unusual permissions (0600 key file created under a permissive umask, or vice versa) would come out wrong without it. It's preserving exact source perms.- The six
//nolint:wrapcheckreturns exist because the caller already prefixes every error withwarning: skipped <rel>: …— wrapping here would just double the context. That's linter tax, not complexity.
So: the floor has been reached, give or take one SameFile line. Want me to drop that check, or does it stay?
Committed: 89e4db06c8 clean up worktree include copy internals. Eleven commits on the branch, working tree clean (minus the usual .opencode bystander), SameFile check retained as discussed. The feature is polished to a shine, Crawler — say the word for the PR.
Base directory for this skill: /Users/pfleidi/.agents/skills/pfleidi/skills/pr
Create Pull Request
1. Understand the Branch
Use origin/main as the comparison base for understanding branch scope. Do not use local main, a local PR target branch, or direct tree diffs such as git diff main or git diff origin/main for scope checks; those can include upstream-only changes and make this branch look like it reverted unrelated work.
Read the commit history to understand the full scope of changes on this branch.
Review the changed file list from the merge base to the current working tree and confirm every changed file belongs to the PR's stated goal:
If unrelated files or commits are present, STOP and report them. Do not create a PR that bundles unrelated work.
2. Sync with origin/main
Before discovering verification commands, bring the branch up to date with origin/main so verification runs against the merged state.
Check that the working tree is clean:
If there are uncommitted changes, STOP and ask the user to commit or stash them before continuing. A sync into a dirty tree creates ambiguous failure states.
Fetch and merge:
Three outcomes:
- Already up to date — no commits to merge. Proceed to step 3.
- Clean merge — merge commit created (or fast-forward applied). Proceed to step 3.
- Conflicts — merge halts with conflicted files. STOP and report each conflicted file. Do NOT auto-resolve; the user must resolve the conflicts and complete the merge commit themselves. Re-run the PR skill after resolution.
3. Discover Project Verification Commands
Inspect the project to determine how to build, lint, and test. Collect candidate commands from these sources, then deduplicate them before running anything:
- Makefile — look for
build,lint,check,test,ci,verifytargets. Read the target recipes to understand what they run. - mise — check for
.mise.tomlor.mise/*.toml. Look for[tasks]definitions covering build, lint, test. If found, usemise run <task>. - CI workflows — read
.github/workflows/*.yml(or.gitlab-ci.yml, etc.) to understand required coverage. CI is the ground truth for what must pass, but CI matrix shards and CI-only wrappers are not automatically local verification commands. - README.md — look for "Development", "Contributing", "Building", or "Testing" sections that document how to run checks.
- Package manager conventions — detect from project files:
go.mod→go build ./...,go vet ./...,go test ./...; do NOT infer a lint command from Go alonepackage.json→ checkscriptsforbuild,lint,testCargo.toml→cargo build,cargo clippy,cargo testpyproject.toml/setup.py→ check for configured linters,pytest
If no lint command exists after checking all sources, state that explicitly instead of assuming an unavailable linter binary.
Reuse Cached Verification Discovery
Before rediscovering commands from scratch, choose an artifact directory using the AGENTS.md temporary artifact rule with agent name pfleidi-pr:
- Use
./tmp/pfleidi-pr/only when./tmp/already exists and is already ignored. - If no project-local artifact directory is available, do not use a verification cache by default. Ask before using
/tmp/pfleidi-pr/or modifying ignore files.
When an artifact directory is available, check for a verification cache at <artifact-dir>/verification-<repo-name>.md. The cache is only an input-token optimization; never commit it and never trust it blindly. If no artifact directory is available, perform normal discovery and skip writing the cache.
Reuse the cache only when all of these are true:
- It names the same worktree root and remote.
- It lists the verification source files it was based on, such as
Makefile,.mise.toml,.mise/*.toml, CI workflow files, README files, and package manifests. - Those source files still exist or are still intentionally absent.
git diff --name-only origin/main -- <source files>shows no branch changes to those source files.
If the cache is missing, stale, or incomplete, perform normal discovery. After discovery, update the cache with:
- Repository root and remote.
- Verification source files inspected.
- Selected command plan grouped by coverage area.
- Commands intentionally skipped as duplicates, aggregate/subtask overlaps, CI-only jobs, or too-slow shard matrices.
- Any assumptions, such as "no documented lint task found."
Deduplicate Verification Commands
Build a command plan by coverage area, not by source. Do not run every command discovered.
- Run at most one command for each coverage area: build/compile, lint/static analysis, unit/core tests, integration tests, e2e/smoke tests.
- Prefer documented local developer tasks over CI-specific commands when they cover the same area.
- Do not run both an aggregate task and its constituent tasks. For example, if
mise run checkruns lint and tests, either runmise run checkalone or run the narrower lint/test tasks, not both. - Treat CI matrix shards as duplicated slices of one suite. Do not run every
*:shard:*command locally when an unsharded local task covers the suite. - If CI has only sharded commands and no local equivalent, ask before running all shards. Otherwise, run the smallest representative or changed-scope test command and note that the full shard matrix remains for CI.
- Do not run CI-only canary/e2e jobs locally by default. Run them only when the PR changes that surface, when the user asks, or when the project documents them as required local PR verification.
Log which sources you used, which duplicate/CI-only commands you skipped, and what commands you will run. If the deduplication rules require asking before slow CI-only coverage, STOP for confirmation; otherwise immediately proceed to step 4.
4. Run Verification and Auto-Fix
Run the deduplicated command plan in the fewest safe batches. Prefer background processing for independent validation tasks instead of running everything sequentially.
The commands should cover, at minimum:
- Build — the project compiles without errors
- Lint / static analysis — no lint warnings or static analysis failures
- Tests — the selected local test coverage passes without duplicating CI shards or aggregate/subtask combinations
Use the exact commands, flags, and build tags found in step 3 for the commands you selected. Do not invent your own flags.
Parallel Verification Rules
Partition the selected commands into dependency-safe batches before running them:
- Run mutating commands alone and before validators that depend on their output. This includes formatters, generators, codegen, migrations, package installation, or commands known to update snapshots, lockfiles, generated files, caches in the repo, or test fixtures.
- Run dependent commands after their prerequisite batch passes. For example, do not start tests that require generated code until generation succeeds.
- Run independent read-only validation commands concurrently in the same background batch. Build, lint/static analysis, typecheck/vet, and unit tests can usually share a batch when they do not mutate the working tree and do not require the same exclusive service, port, database, or fixture directory.
- Keep integration, e2e, or service-backed commands separate unless the project documents that they are parallel-safe.
- If unsure whether two commands are independent, run them sequentially. Correctness of validation beats speed.
For each background batch:
-
Start every command from the same working-tree state.
-
Run each selected validator directly, for example
mise run lint,go test ..., ornpm test -- .... Do not wrap validators insh -c, shell redirection,tee, command separators, or pipelines solely to capture logs; that defeats command-prefix approvals and causes extra permission prompts. -
Capture each command's stdout, stderr, exit status, and command line from the tool output separately.
-
While the batch is running, do not edit files, start auto-fixes, or treat partial output as a result.
-
Wait for every command in the batch to finish, then show verification as a compact table:
Command Exit Relevant output go test ./pkg/foo -run TestBar -count=10 Short success excerpt. -
For failures or short outputs, show complete output in the relevant-output column or immediately below the table. For long successful outputs, show the relevant excerpt and state that the rest was truncated.
-
If any command in the batch fails, treat the whole batch as failed for the fix loop. Results from other commands in that stale batch may help diagnose, but they do not count as passing verification after files change.
On Failure: Fix and Re-verify
If any command fails, do NOT stop. Instead:
- Read the error output and identify every failure
- Fix all issues — apply the minimal changes needed to make the failing command pass
- Re-run the deduplicated verification plan from the top, using the same safe batching rules (not just the previously failing command — fixes can introduce new issues)
- Show the updated verification table again, including complete failure output for any command that still fails
Repeat this cycle until all commands pass. Cap at 3 fix attempts. If verification still fails after 3 rounds, STOP and present the remaining failures to the user with full failure output — do not keep looping.
5. Prompt for Commit
After all verification passes, check for uncommitted changes:
If there are uncommitted changes (from auto-fixes in step 4):
- Show the diff of all uncommitted changes
- Propose a semantically correct commit message using the subject-plus-context style from
AGENTS.md. The message must describe the net fix (e.g., "fix lint warnings in config parser" not "fix issues found during PR prep"). - If compile/build did not pass for code changes, say the work is not commit-ready and do not ask to commit until the gap is resolved or the user explicitly takes over.
- STOP and wait for user approval. The user may edit the message, split the changes, or commit themselves.
If the user approves the commit, do not rerun the full verification suite before committing unless files changed after step 4. If another sanity check is needed, use the commit-time verification scope from AGENTS.md: lint tasks, a compile/build check for code changes, and tests directly related to the changed code only.
If there are no uncommitted changes, proceed directly to step 6.
6. Push the Branch
If the branch has no upstream yet, use git push -u origin HEAD.
7. Create the PR
Determine a concise PR title (under 70 characters) from the commit history and diff.
Set the target base branch from the user-provided PR base, or main when the user did not provide one. Scope checks still use origin/main; the PR target base controls only the GitHub PR destination.
If the user provided a PR target base, set PR_BASE to that branch name instead.
Determine the pushed source branch:
If HEAD_BRANCH is empty, STOP and report that PR creation needs a named local branch.
Determine the GitHub repository slug from the origin remote before writing the PR body:
Extract GITHUB_REPO as <owner>/<repo> from these origin URL forms:
git@github.com:<owner>/<repo>.githttps://github.com/<owner>/<repo>.gitssh://git@github.com/<owner>/<repo>.gitentire://<mirror-host>/gh/<owner>/<repo>
Strip a trailing .git when present. For entire:// remotes, ignore the mirror host and use only the suffix after /gh/; do not use any checkpoint-storage repository URL as the PR target when the entire://.../gh/... origin is available.
If the origin URL does not expose a GitHub repository, try:
If that still cannot identify a repository, STOP and ask the user for the GitHub target.
Use the same branch-only comparison from step 1 ($MERGE_BASE to the current working tree) when deriving the title, PR body, changed-file list, and mostly-Markdown detection. Do not use local main or direct git diff origin/main output for PR description decisions.
Write the PR body to help a reviewer (human or bot) understand the change without re-deriving it from the diff. Include these sections; omit any that genuinely don't apply:
- Why — the motivation: what problem this solves, what behavior was broken or missing, what constraint forced the change. This is the most important section. Be specific so neither a reviewer nor a bot has to infer the reason from the diff alone.
- What changed — a short, factual summary of the net change. One or two sentences; the diff is the source of truth.
- Usage examples — for a new or changed command, API, config option, workflow, or user-facing behavior, show a small realistic example of how to use it and what to expect. For UI work, add screenshot placeholders such as
Before: <screenshot>andAfter: <screenshot>when actual screenshots are not available yet. - Decisions made during development — non-obvious choices from the development process: why one approach over another, why an existing abstraction wasn't reused, why a check lives where it does, what assumptions shaped the implementation, and what constraints were intentionally accepted.
- Technical tradeoffs — when a real engineering tradeoff was made, name the options weighed, what the chosen approach gives up, and why that tradeoff is acceptable. Skip if the change was mechanical with no meaningful alternatives.
- Reviewer notes — only for migrations, deployment ordering, backwards-incompatible behavior, or known follow-up work not in this PR. Skip otherwise.
- Rendered Markdown (for mostly Markdown PRs) — links to the changed Markdown files rendered on GitHub.
Do NOT include:
- A "Test plan" or "Verification" section listing the CLI commands run. Verification already happened in step 4; the transcript doesn't help the reviewer.
- A list of newly added tests. The diff already shows them; the list rots when tests are renamed or moved.
- A file-by-file changes summary. The diff already shows this too.
Choose the PR creation command from the origin URL.
For entire://.../gh/<owner>/<repo> origins, do not use gh pr create. It may still inspect the local git remote and fail to infer a GitHub host even when --repo "$GITHUB_REPO" is supplied. Use the GitHub API through gh api with the already-derived repository slug and pushed branch:
For normal GitHub remotes, use gh pr create:
If a non-entire:// origin hits the same host-inference failure from gh pr create, use this gh api creation path once instead of retrying gh pr create.
Create a ready-for-review PR only when the user explicitly asks for it with wording such as "non-draft", "ready for review", or "publish it as ready". In that case, omit --draft from gh pr create or omit -F draft=true from the gh api call.
Show the PR URL and STOP.
Rules
- Do NOT add "Generated with Claude Code" or any AI attribution anywhere
- Do NOT summarize what you just did after showing the URL
- Treat a PR as "mostly Markdown" when more than half of changed files are Markdown (
.md,.mdx,.markdown) or the main review target is Markdown documentation. In that case, include GitHub rendered links for the changed Markdown files using the pushed branch, e.g.https://github.com/$GITHUB_REPO/blob/<branch>/<path>.md. Link all changed Markdown files when the list is short; for large docs PRs, link the main entry points or most important changed files. - If the user provides a title or description, use it verbatim instead of generating one, but append rendered Markdown links for mostly Markdown PRs unless the user explicitly says not to modify the description.
- If the PR description contains checklist items (
- [ ]), check them (- [x]) if and only if the corresponding step was actually verified during this workflow (e.g., build passed, tests passed, lint passed). Do NOT check items that were not verified. Do NOT leave items unchecked if they were verified.
ACHIEVEMENT UNLOCKED. Draft PR is live:
Base directory for this skill: /Users/pfleidi/.agents/skills/pfleidi/skills/pr-feedback
PR Feedback
1. Identify the PR
If the user provided a PR number or URL, use that. Otherwise, detect from the current branch:
If no PR exists for the current branch, stop and tell the user.
2. Fetch Review Comments
Fetch PR metadata first so comment decisions have intent context:
Also collect branch commits and changed files for context:
Show the PR context as a table before classifying comments:
| Context | Source | Value |
|---|---|---|
| PR | title/body | One-line PR intent |
| Branch | commits | One-line commit summary |
| Changed surface | diff file list | Main packages/files touched |
| Base/head | PR metadata | base <- head |
Fetch unresolved review threads with GraphQL as the primary source of truth. Group work by thread, not by individual REST comment:
Filter to unresolved threads only. If there are no unresolved threads, report that to the user and stop — there is nothing to fix.
If GraphQL pagination indicates more review threads or thread comments are available, paginate before classifying. Do not classify a partial thread set as complete.
Use REST pull-review comments only as a fallback when GraphQL data is incomplete or a thread cannot be mapped to a review comment ID:
When REST fallback is used, deduplicate by GraphQL thread ID first, then by file/line/body/author. Do not present or fix the same review request twice.
3. Parse, Classify, and Group
Use permission-friendly reads while investigating comments. Avoid shell pipelines, command separators, subshells, and output filters for read-only source inspection because they create extra permission prompts and can block background work. Do not run commands like git show HEAD:path | sed -n '10,40p'. Use workspace file range reads, rg with path limits, path-scoped diffs, or one standalone git show <rev>:<path> only when the output is acceptably small.
For each comment, extract:
- Author — who left it
- Author type — bot, automated reviewer, human reviewer, or maintainer
- File and line — where it points
- Body — the actual feedback (verbatim, not paraphrased)
- Thread context — any replies in the same thread (to understand if it was already discussed or resolved conversationally)
- Thread ID and top-level comment ID — the GraphQL review thread ID and the original top-level review comment
databaseIdneeded to reply and resolve. Replies to replies are not supported; if only a reply ID is available, fetch the full thread and use the first/top-level review comment ID.
Group each unresolved review thread into a single finding. If multiple comments in one thread refine or supersede each other, use the latest unresolved reviewer request as the finding and retain the earlier messages as context.
Classify each finding source:
- Bot — GitHub bot, CI system, or linter/static-analysis account such as
github-actions[bot]orcodecov[bot] - Automated reviewer — review-assistant accounts that produce natural-language suggestions, such as Copilot or CodeRabbit
- Human reviewer — non-bot reviewer
- Maintainer — repository owner/member/maintainer when that can be inferred from GitHub metadata
4. Present Findings
Present two separate sections:
Human Comments
Table ordered by:
- Bugs / correctness issues — reviewer identified broken logic or missing error handling
- Design / architecture feedback — structural changes, API shape, naming of public interfaces
- Style / nits — formatting, naming of local variables, minor readability
Use this table format:
| # | Priority | Location | Reviewer | Request | Key quote | Autofix |
|---|---|---|---|---|---|---|
| 1 | Bug | file.go:42 | reviewer | One-line summary of what the reviewer is asking for. | Short verbatim excerpt. | Eligible, or Needs decision with the exact decision needed. |
For automated reviewers, use the same table and set Reviewer to the tool account, with Priority based on the substance of the request.
Bot Comments (batched)
Table continuing the numbering from above, grouped by tool/bot:
| # | Bot | Location | Required fix | Autofix |
|---|---|---|---|---|
| 8 | linter-name | file.go:42 | One-line summary of the required fix. | Eligible, or Needs decision with the exact decision needed. |
Keep table cells short and scannable. Use the smallest useful verbatim quote, not the full comment body. Escape | characters inside code or text so the table remains valid Markdown.
End with a summary: total human comments, total bot comments, overall assessment of effort.
Do not stop for mode selection. After the decision gate below, proceed by default with bot comments and human comments marked Autofix eligible. Mark a human comment Autofix eligible only when the requested change is source-backed, high confidence, minimal, unambiguous, does not require a product/design decision, does not add a dependency, does not change a shared/public interface, and has a clear verification path.
Leave all other human comments unresolved as Needs decision, with the exact decision needed. Do not reject a reviewer comment by default; rejection requires a user-provided public rationale.
Decision Gate Before Fixes
Before applying any fixes, handle every Needs decision finding first. Do not let bot comments or easy autofixes push these questions to the end.
-
Present a short "Decision needed first" table:
# Location Reviewer Decision needed Why it blocks 3 file.go:42reviewerChoose whether the API should return nilor an empty slice.Either answer changes caller behavior. -
Try to answer each decision from source, PR context, existing project patterns, and the full review thread before asking the user.
-
If the answer is source-backed, low risk, and has one clear implementation, reclassify the finding as Autofix eligible and record the reasoning.
-
If the correct answer is "do not change this", record it as a proposed rejection, but do not publish the rejection without a user-provided public rationale.
-
If any finding still needs a product/design call, shared/public interface decision, dependency choice, or other user judgment, STOP before bot or autofix work. Ask for all remaining decisions in one concise list.
-
Continue to Step 5 only after every decision is either answered, reclassified, proposed for rejection with a user-provided rationale, or explicitly deferred by the user. Deferred Needs decision findings remain unresolved and must be listed again in the final summary.
Before applying any fixes, record the starting commit:
Choose an artifact directory using the AGENTS.md temporary artifact rule with agent name pfleidi-pr-feedback:
- Use
./tmp/pfleidi-pr-feedback/only when./tmp/already exists and is already ignored. - If no project-local artifact directory is available, do not create file artifacts by default; keep ledger/log/cache information in the response and mark file paths
n/a. Ask before using/tmp/pfleidi-pr-feedback/or modifying ignore files.
When an artifact directory is available, create a temporary thread ledger at <artifact-dir>/pr-feedback-<pr-number>.md. If no artifact directory is available, keep the same ledger fields in the final summary table instead. Update the ledger after each thread with:
- Thread ID, source category, reviewer, location, and status.
- Files touched.
- What changed and why.
- Related tests or verification commands.
- Planned review-thread reply body, if any.
- Resolve decision: yes/no and why.
5. Fix Bot Comments (batched)
After the decision gate, fix all bot comments first — these are mechanical and clearing them reduces noise before the human-comment phase.
- For each bot finding:
- Read the relevant code
- Implement the fix — ONLY the changes needed for that single finding
- Track the files changed for this finding so the review-thread reply can identify the commit that contains the fix
- If a fix is ambiguous or would conflict with a human-comment fix already applied, mark it Needs decision and continue
- After all bot fixes are applied, present a summary table. Do NOT show a diff — the Edit tool already showed each change inline.
| # | Finding | File | Bot | Status |
|---|---|---|---|---|
| 8 | Description | path:line | linter-name | Fixed |
| 9 | Description | path:line | linter-name | Fixed |
| 11 | Description | path:line | linter-name | Skipped — conflicts with #3 |
- Proceed directly to Step 6.
6. Fix Human Comments (batched)
After bot fixes, work through Autofix eligible human comments in report order:
- State which finding you are addressing (number and one-line description)
- Read the relevant code and the full comment thread to understand intent
- Re-check eligibility before editing; if the fix is no longer clearly eligible, mark it Needs decision and continue
- Implement the fix — ONLY the changes needed for that single finding
- Track the files changed for this finding so the review-thread reply can identify the commit that contains the fix
- If a comment needs a product/design decision, shared/public interface change, dependency, broad refactor, or has multiple reasonable fixes, mark it Needs decision and continue
- If the user rejects the comment instead of fixing it, record the specific rationale to use in the review-thread reply
Scope Rules
- Make the MINIMAL change that addresses the reviewer's feedback
- Keep the diff limited to files and lines directly required by the feedback
- First decide whether the feedback points to a local or systemic issue. Fix at the narrowest correct level; do not add a local workaround that hides a shared/root-cause bug.
- If the feedback requires a behavior-changing code fix, add or update the directly related test in the same fix. Prefer TDD, but complete the focused red-to-green cycle before stopping: write/update the failing test, confirm it fails, implement the fix, confirm the focused test passes. Do not stop after only adding the failing test unless the user explicitly asks.
- Do NOT rename variables, reformat code, or touch lines outside the feedback scope
- Do NOT refactor adjacent code, even if it looks related
- If the reviewer's comment is ambiguous, mark it Needs decision and continue with unrelated unambiguous comments
- Do NOT create any git commits during the fix cycle. Commits are handled only in the publish step, and only with explicit user approval when needed.
7. Verify Fixes
After all fixes are applied, run the project's compile/build, lint, and test commands scoped to only the changed files and their directly related tests. If no code changed, skip verification and proceed to Step 8. Use safe background batches for independent validators instead of running every command sequentially.
When selecting verification commands, reuse <artifact-dir>/verification-<repo-name>.md if an artifact directory is available and the cache is fresh under the cache rules from pfleidi:pr; otherwise discover the smallest relevant lint/test/build commands. Update the cache only when an artifact directory is available.
- Build / compile — run a relevant compile/build command when one is discoverable for the changed production code.
- Lint / static analysis — run the project's documented lint task, scoped to the files that were modified when the task supports scoping. Prefer lint-specific task wrappers such as
make lintormise run lintover invoking linter binaries directly. Do not use aggregatecheck,ci, orverifytasks unless you have confirmed they only run lint/static analysis. If the documented lint task cannot be scoped, run the smallest relevant project lint task. - Tests — run only the test files that cover the modified code (same package, same module, co-located test files). Do NOT run the full test suite.
If no compile/build command or project lint task exists, state that explicitly instead of assuming an unavailable command.
Run formatters, generators, snapshot updates, or other mutating commands alone before validators that depend on their output. Run independent read-only validators concurrently when they do not require the same exclusive service, port, database, fixture directory, or generated output. Keep integration/e2e/service-backed commands separate unless the project documents that they are parallel-safe.
For each background batch, start every command from the same working-tree state, capture stdout/stderr/exit status from the tool, do not edit files while the batch is running, and wait for every command to finish. Run each selected validator directly, for example mise run lint, go test ..., or npm test -- .... Do not wrap validators in sh -c, shell redirection, tee, command separators, or pipelines solely to write logs; that defeats command-prefix approvals and causes extra permission prompts. If an artifact directory is available and file logs can be written after the command completes without rerunning through a shell wrapper, save them under <artifact-dir>/logs-<pr-number>-<timestamp>/; otherwise mark the full-log path as n/a. If files change after a failed batch, none of that batch's successful results count as current verification.
Show verification as a compact table:
| Command | Exit | Relevant output | Full log |
|---|---|---|---|
go test ./pkg/foo -run TestBar -count=1 | 0 | Short success excerpt. | <artifact-dir>/logs-.../go-test-pkg-foo.log or n/a |
For failures or short outputs, show complete output in the relevant-output column or immediately below the table. For long successful outputs, show the relevant excerpt and log path.
If compile/build, lint, or tests fail due to issues introduced by the fixes:
- Read the error output and identify every failure
- Fix all issues — apply the minimal changes needed
- Re-run the failing commands using the same safe batching rules
- Show the complete output again
Cap at 2 fix attempts. If still failing after 2 rounds, present the remaining failures to the user with full output. Do not describe the work as done or commit-ready while compile/build status is failing or unknown for changed production code.
Once verification passes, show a summary: how many comments were addressed, rejected, intentionally left unresolved, or still blocked. If compile/build did not pass for code changes, say the work is not commit-ready before the summary. Do NOT show a diff — the Edit tool already showed each change inline.
Proceed to Step 8 for threads that were addressed or intentionally rejected. Leave Needs decision threads unresolved and do not post replies to them unless the user provided a public rejection rationale. Do not block publishing addressed threads just because unrelated threads still need a decision.
8. Publish PR Updates
After addressed/rejected threads are ready to publish:
-
Check branch state:
-
If there are uncommitted fix changes, STOP and ask the user whether to commit them now or let the user commit manually. Do not push until the fixes are committed. If compile/build did not pass for code changes, say the work is not commit-ready and do not ask to commit until the gap is resolved or the user explicitly takes over. If the user approves committing after verification, stage only files changed for the PR feedback fixes and write the commit message from the actual diff using the subject-plus-context style from
AGENTS.md. -
Push the committed changes for the current branch:
If the branch has no upstream and the push fails for that reason, use:
Never force-push.
-
Map each addressed finding to the commit or commits that contain its fix. Use the recorded starting commit, changed-file tracking, ledger, and
git log/git showto identify the relevant short SHA(s). If one commit fixes multiple comments, reference the same commit in each review-thread reply. -
Build and show a direct review-thread reply plan before calling the API:
Thread Top-level comment ID Status Reply body Resolve PRRT_...123456789Addressed Addressed in abc1234 by adding the nil check before dereferencing.Yes PRRT_...n/aNeeds decision n/aNo Proceed without asking when every actionable reply body is either addressed or a user-approved rejection. Needs decision rows with
Reply body = n/aandResolve = Nodo not block publishing addressed threads only if they already passed the decision gate and were explicitly deferred by the user. Stop before API calls if a rejection lacks a public rationale, if a top-level comment ID is missing for a thread that should receive a reply, or if any reply body is uncertain. -
Post one direct review-thread reply for each addressed or user-approved rejected thread:
-
For addressed comments, state what changed and the commit SHA(s), e.g.
Addressed in abc1234 by adding the nil check before dereferencing. -
For rejected comments, state why it was not changed, e.g.
Not changed: this path already returns before allocation failure can be observed. -
Keep replies concise but specific. Do not over-explain or repeat the diff.
-
Do not reply to comments the user explicitly chose to leave unresolved unless needed to publish a user-approved rejection rationale.
-
Use the REST review-comment reply endpoint:
This endpoint posts an immediate reply to the existing review thread. Do not use
gh pr review, GraphQLaddPullRequestReviewThreadReply,gh pr comment, or the regular issue comment endpoint as a fallback. -
-
Resolve each addressed or user-approved rejected thread after its direct review-thread reply succeeds:
Do not resolve a thread that did not receive a successful direct reply, is still ambiguous, or still needs user approval.
-
Present a final summary table:
Thread Reviewer Location Status Commented Resolved PRRT_...reviewerfile.go:42Addressed Yes Yes PRRT_...reviewerfile.go:99Needs decision No No Include push result, branch, ledger path, rejected comments and rationale, and any comments intentionally left unresolved or not resolvable via API.
9. Post-Publish
- If the user asks for adjustments, apply them
- If the user asks to revert specific fixes, revert ONLY those changes
- Do NOT commit or push additional changes unless explicitly asked
Both fixes verified:
| Command | Exit | Relevant output | Full log |
|---|---|---|---|
go test -run 'TestCheckoutTrailWorktree|TestFindWorktreeForBranch' ./cmd/entire/cli/ -count=1 | 0 | ok … 1.243s (includes the two new tests) | n/a |
mise run fmt && mise run lint | 0 | fmt idempotent, lint 0 issues | n/a |
Status: #1 fixed (os.Lstat + IsDir gate — a symlink or file planted at the registered path is rejected with is not a directory, pinned by a symlink test), #3 fixed (cmd.Dir = root + git stderr surfaced via ExitError.Stderr, keeping the parsed porcelain clean — pinned by a non-repo-dir test), #2 needs no code (your c3c14108f4 made the creation-mode complaint unreachable; reply-only).
The fixes are uncommitted. Proposed commit (staging only the two feature files):
harden worktree checks from review feedback
Use Lstat and require a directory when reusing a registered worktree path, so a planted symlink is rejected instead of passing the health check. Run
git worktree listfrom the main root and include git's stderr in the failure message.
Approve the commit and I'll push (your unpushed c3c14108f4 rides along), then post the three thread replies and resolve them:
| Thread | Top-level comment ID | Status | Reply body | Resolve |
|---|---|---|---|---|
…P_JNY (cursor) | 3561359382 | Addressed | Addressed in <new sha>: the reuse check now uses os.Lstat and requires the registered path to be a directory; a symlink or file planted there is rejected with a clear error. | Yes |
…P_KTU (copilot) | 3561365568 | Addressed | Addressed in c3c1410: appendIgnoreRule no longer creates .gitignore — a missing file is a no-op — so the creation mode no longer applies (WriteFile's perm only takes effect on create). | Yes |
…P_KTT (copilot) | 3561365599 | Addressed | Addressed in <new sha>: git worktree list now runs with cmd.Dir set to the main root, and failures surface git's stderr (via ExitError.Stderr rather than CombinedOutput, so stderr can't pollute the parsed porcelain). | Yes |
It's the price of wanting git's actual complaint in the error message without corrupting the parsed output. Three forces at play:
-
cmd.Output()'s error is nearly mute. On a non-zero exit it returns an*exec.ExitErrorwhoseError()is literallyexit status 128. Git's real diagnostic —fatal: not a git repository,fatal: this operation must be run in a work tree— went to stderr. Without this block, the user seesfailed to list worktrees: exit status 128and gets to guess which of git's dozen 128-failures they hit. That opacity is exactly what Copilot's comment complained about. -
Output()stashes stderr inExitError.Stderr, but onlyerrors.Asgets you there. The captured stderr isn't part of the error string; you have to unwrap to the concrete type to read the field. Theerrors.Asalso guards the cases where the error isn't anExitErrorat all — context cancelled,gitbinary missing (*exec.Error) — where there's no stderr to read and the plain%wfallback below is the right message. -
The
len(TrimSpace(...)) > 0check just prevents the degeneratefailed to list worktrees: : exit status 1when the process died without writing anything to stderr.
The obvious shortcut — CombinedOutput(), which Copilot actually suggested — was rejected deliberately: it interleaves stderr into stdout, and we parse that stdout as porcelain. A stray git warning on stderr would then flow into parseWorktreeForBranch as if it were worktree data.
So: As is correctness (not every failure is an ExitError), the length check is cosmetics, and the whole shape is the standard Go idiom for "run, parse stdout, but report stderr on failure." If the empty-stderr cosmetic bothers you, that check is the only optional line.
All published. Final state:
| Thread | Reviewer | Location | Status | Commented | Resolved |
|---|---|---|---|---|---|
…P_JNY | cursor (Bugbot) | trail_checkout_worktree.go:314 | Addressed in 11febb69cc (Lstat + IsDir gate) | Yes | Yes |
…P_KTU | copilot | trail_checkout_worktree.go:124 | Addressed in c3c14108f4 (creation path removed) | Yes | Yes |
…P_KTt | copilot | trail_checkout_worktree.go:378 | Addressed in 11febb69cc (cmd.Dir + stderr surfacing) | Yes | Yes |
Push landed bac53dacd8..11febb69cc (your tighten trail worktree checkout guards rode along), all three replies posted with commit SHAs, all three threads resolved. Zero human comments, zero deferred decisions, ledger kept inline. PR #1709 has a clean review queue again — the tribunal is appeased, Crawler.
One additional thing that would be nice: When running from a non-interactive terminal allow something like this:
cd "$(entire trail checkout 575 --worktree)"
As in: Don't output all of the other information if someone wants to use the command for scripts
Done — cd "$(entire trail checkout 575 --worktree)" now works. The shape:
- All progress moved to stderr unconditionally: "Checking out … in a worktree", the fetch message, the
.gitignorenotice, and the cancel message. On a terminal you see everything exactly as before (both streams land in the same place). - stdout is gated on
interactive.IsTerminalWritervia a newprintTrailWorktreeLocationhelper: terminal →Worktree ready at <path>+cd '<path>'; non-terminal → the bare path, one line, nothing else. The reuse path behaves identically, so a script that re-runs the command still gets a usable path. - Help text and spec updated to document the script form.
TDD evidence: updated CreatesWorktree/ReusesExistingWorktree tests to demand bare-path stdout + stderr notices, confirmed both fail against the old behavior (the reuse red also caught a macOS /var vs /private/var symlink wrinkle — the test compares normalized paths), then implemented and confirmed ok … 1.242s with fmt + lint clean. Buffers aren't terminals, so tests natively exercise the script branch; the TTY branch is the same untestable-under-go test category as the prompt paths.
Uncommitted, awaiting your word. Proposed message:
print bare worktree path when stdout is not a terminal
Progress and notices for trail worktree checkout now go to stderr, and a non-terminal stdout carries only the worktree path, so scripts can use cd "$(entire trail checkout <trail> --worktree)".
After the commit I'd push and update the PR body's Usage section with the script example — say the word.
Committed as 30c2a356ed, pushed (11febb69cc..30c2a356ed), and PR #1709's Usage section now shows the cd "$(entire trail checkout 575 --worktree)" form. The branch is current, the review queue is empty, and the command is now fit for both humans and their scripts.
Base directory for this skill: /Users/pfleidi/.agents/skills/pfleidi/skills/pr-feedback
PR Feedback
1. Identify the PR
If the user provided a PR number or URL, use that. Otherwise, detect from the current branch:
If no PR exists for the current branch, stop and tell the user.
2. Fetch Review Comments
Fetch PR metadata first so comment decisions have intent context:
Also collect branch commits and changed files for context:
Show the PR context as a table before classifying comments:
| Context | Source | Value |
|---|---|---|
| PR | title/body | One-line PR intent |
| Branch | commits | One-line commit summary |
| Changed surface | diff file list | Main packages/files touched |
| Base/head | PR metadata | base <- head |
Fetch unresolved review threads with GraphQL as the primary source of truth. Group work by thread, not by individual REST comment:
Filter to unresolved threads only. If there are no unresolved threads, report that to the user and stop — there is nothing to fix.
If GraphQL pagination indicates more review threads or thread comments are available, paginate before classifying. Do not classify a partial thread set as complete.
Use REST pull-review comments only as a fallback when GraphQL data is incomplete or a thread cannot be mapped to a review comment ID:
When REST fallback is used, deduplicate by GraphQL thread ID first, then by file/line/body/author. Do not present or fix the same review request twice.
3. Parse, Classify, and Group
Use permission-friendly reads while investigating comments. Avoid shell pipelines, command separators, subshells, and output filters for read-only source inspection because they create extra permission prompts and can block background work. Do not run commands like git show HEAD:path | sed -n '10,40p'. Use workspace file range reads, rg with path limits, path-scoped diffs, or one standalone git show <rev>:<path> only when the output is acceptably small.
For each comment, extract:
- Author — who left it
- Author type — bot, automated reviewer, human reviewer, or maintainer
- File and line — where it points
- Body — the actual feedback (verbatim, not paraphrased)
- Thread context — any replies in the same thread (to understand if it was already discussed or resolved conversationally)
- Thread ID and top-level comment ID — the GraphQL review thread ID and the original top-level review comment
databaseIdneeded to reply and resolve. Replies to replies are not supported; if only a reply ID is available, fetch the full thread and use the first/top-level review comment ID.
Group each unresolved review thread into a single finding. If multiple comments in one thread refine or supersede each other, use the latest unresolved reviewer request as the finding and retain the earlier messages as context.
Classify each finding source:
- Bot — GitHub bot, CI system, or linter/static-analysis account such as
github-actions[bot]orcodecov[bot] - Automated reviewer — review-assistant accounts that produce natural-language suggestions, such as Copilot or CodeRabbit
- Human reviewer — non-bot reviewer
- Maintainer — repository owner/member/maintainer when that can be inferred from GitHub metadata
4. Present Findings
Present two separate sections:
Human Comments
Table ordered by:
- Bugs / correctness issues — reviewer identified broken logic or missing error handling
- Design / architecture feedback — structural changes, API shape, naming of public interfaces
- Style / nits — formatting, naming of local variables, minor readability
Use this table format:
| # | Priority | Location | Reviewer | Request | Key quote | Autofix |
|---|---|---|---|---|---|---|
| 1 | Bug | file.go:42 | reviewer | One-line summary of what the reviewer is asking for. | Short verbatim excerpt. | Eligible, or Needs decision with the exact decision needed. |
For automated reviewers, use the same table and set Reviewer to the tool account, with Priority based on the substance of the request.
Bot Comments (batched)
Table continuing the numbering from above, grouped by tool/bot:
| # | Bot | Location | Required fix | Autofix |
|---|---|---|---|---|
| 8 | linter-name | file.go:42 | One-line summary of the required fix. | Eligible, or Needs decision with the exact decision needed. |
Keep table cells short and scannable. Use the smallest useful verbatim quote, not the full comment body. Escape | characters inside code or text so the table remains valid Markdown.
End with a summary: total human comments, total bot comments, overall assessment of effort.
Do not stop for mode selection. After the decision gate below, proceed by default with bot comments and human comments marked Autofix eligible. Mark a human comment Autofix eligible only when the requested change is source-backed, high confidence, minimal, unambiguous, does not require a product/design decision, does not add a dependency, does not change a shared/public interface, and has a clear verification path.
Leave all other human comments unresolved as Needs decision, with the exact decision needed. Do not reject a reviewer comment by default; rejection requires a user-provided public rationale.
Decision Gate Before Fixes
Before applying any fixes, handle every Needs decision finding first. Do not let bot comments or easy autofixes push these questions to the end.
-
Present a short "Decision needed first" table:
# Location Reviewer Decision needed Why it blocks 3 file.go:42reviewerChoose whether the API should return nilor an empty slice.Either answer changes caller behavior. -
Try to answer each decision from source, PR context, existing project patterns, and the full review thread before asking the user.
-
If the answer is source-backed, low risk, and has one clear implementation, reclassify the finding as Autofix eligible and record the reasoning.
-
If the correct answer is "do not change this", record it as a proposed rejection, but do not publish the rejection without a user-provided public rationale.
-
If any finding still needs a product/design call, shared/public interface decision, dependency choice, or other user judgment, STOP before bot or autofix work. Ask for all remaining decisions in one concise list.
-
Continue to Step 5 only after every decision is either answered, reclassified, proposed for rejection with a user-provided rationale, or explicitly deferred by the user. Deferred Needs decision findings remain unresolved and must be listed again in the final summary.
Before applying any fixes, record the starting commit:
Choose an artifact directory using the AGENTS.md temporary artifact rule with agent name pfleidi-pr-feedback:
- Use
./tmp/pfleidi-pr-feedback/only when./tmp/already exists and is already ignored. - If no project-local artifact directory is available, do not create file artifacts by default; keep ledger/log/cache information in the response and mark file paths
n/a. Ask before using/tmp/pfleidi-pr-feedback/or modifying ignore files.
When an artifact directory is available, create a temporary thread ledger at <artifact-dir>/pr-feedback-<pr-number>.md. If no artifact directory is available, keep the same ledger fields in the final summary table instead. Update the ledger after each thread with:
- Thread ID, source category, reviewer, location, and status.
- Files touched.
- What changed and why.
- Related tests or verification commands.
- Planned review-thread reply body, if any.
- Resolve decision: yes/no and why.
5. Fix Bot Comments (batched)
After the decision gate, fix all bot comments first — these are mechanical and clearing them reduces noise before the human-comment phase.
- For each bot finding:
- Read the relevant code
- Implement the fix — ONLY the changes needed for that single finding
- Track the files changed for this finding so the review-thread reply can identify the commit that contains the fix
- If a fix is ambiguous or would conflict with a human-comment fix already applied, mark it Needs decision and continue
- After all bot fixes are applied, present a summary table. Do NOT show a diff — the Edit tool already showed each change inline.
| # | Finding | File | Bot | Status |
|---|---|---|---|---|
| 8 | Description | path:line | linter-name | Fixed |
| 9 | Description | path:line | linter-name | Fixed |
| 11 | Description | path:line | linter-name | Skipped — conflicts with #3 |
- Proceed directly to Step 6.
6. Fix Human Comments (batched)
After bot fixes, work through Autofix eligible human comments in report order:
- State which finding you are addressing (number and one-line description)
- Read the relevant code and the full comment thread to understand intent
- Re-check eligibility before editing; if the fix is no longer clearly eligible, mark it Needs decision and continue
- Implement the fix — ONLY the changes needed for that single finding
- Track the files changed for this finding so the review-thread reply can identify the commit that contains the fix
- If a comment needs a product/design decision, shared/public interface change, dependency, broad refactor, or has multiple reasonable fixes, mark it Needs decision and continue
- If the user rejects the comment instead of fixing it, record the specific rationale to use in the review-thread reply
Scope Rules
- Make the MINIMAL change that addresses the reviewer's feedback
- Keep the diff limited to files and lines directly required by the feedback
- First decide whether the feedback points to a local or systemic issue. Fix at the narrowest correct level; do not add a local workaround that hides a shared/root-cause bug.
- If the feedback requires a behavior-changing code fix, add or update the directly related test in the same fix. Prefer TDD, but complete the focused red-to-green cycle before stopping: write/update the failing test, confirm it fails, implement the fix, confirm the focused test passes. Do not stop after only adding the failing test unless the user explicitly asks.
- Do NOT rename variables, reformat code, or touch lines outside the feedback scope
- Do NOT refactor adjacent code, even if it looks related
- If the reviewer's comment is ambiguous, mark it Needs decision and continue with unrelated unambiguous comments
- Do NOT create any git commits during the fix cycle. Commits are handled only in the publish step, and only with explicit user approval when needed.
7. Verify Fixes
After all fixes are applied, run the project's compile/build, lint, and test commands scoped to only the changed files and their directly related tests. If no code changed, skip verification and proceed to Step 8. Use safe background batches for independent validators instead of running every command sequentially.
When selecting verification commands, reuse <artifact-dir>/verification-<repo-name>.md if an artifact directory is available and the cache is fresh under the cache rules from pfleidi:pr; otherwise discover the smallest relevant lint/test/build commands. Update the cache only when an artifact directory is available.
- Build / compile — run a relevant compile/build command when one is discoverable for the changed production code.
- Lint / static analysis — run the project's documented lint task, scoped to the files that were modified when the task supports scoping. Prefer lint-specific task wrappers such as
make lintormise run lintover invoking linter binaries directly. Do not use aggregatecheck,ci, orverifytasks unless you have confirmed they only run lint/static analysis. If the documented lint task cannot be scoped, run the smallest relevant project lint task. - Tests — run only the test files that cover the modified code (same package, same module, co-located test files). Do NOT run the full test suite.
If no compile/build command or project lint task exists, state that explicitly instead of assuming an unavailable command.
Run formatters, generators, snapshot updates, or other mutating commands alone before validators that depend on their output. Run independent read-only validators concurrently when they do not require the same exclusive service, port, database, fixture directory, or generated output. Keep integration/e2e/service-backed commands separate unless the project documents that they are parallel-safe.
For each background batch, start every command from the same working-tree state, capture stdout/stderr/exit status from the tool, do not edit files while the batch is running, and wait for every command to finish. Run each selected validator directly, for example mise run lint, go test ..., or npm test -- .... Do not wrap validators in sh -c, shell redirection, tee, command separators, or pipelines solely to write logs; that defeats command-prefix approvals and causes extra permission prompts. If an artifact directory is available and file logs can be written after the command completes without rerunning through a shell wrapper, save them under <artifact-dir>/logs-<pr-number>-<timestamp>/; otherwise mark the full-log path as n/a. If files change after a failed batch, none of that batch's successful results count as current verification.
Show verification as a compact table:
| Command | Exit | Relevant output | Full log |
|---|---|---|---|
go test ./pkg/foo -run TestBar -count=1 | 0 | Short success excerpt. | <artifact-dir>/logs-.../go-test-pkg-foo.log or n/a |
For failures or short outputs, show complete output in the relevant-output column or immediately below the table. For long successful outputs, show the relevant excerpt and log path.
If compile/build, lint, or tests fail due to issues introduced by the fixes:
- Read the error output and identify every failure
- Fix all issues — apply the minimal changes needed
- Re-run the failing commands using the same safe batching rules
- Show the complete output again
Cap at 2 fix attempts. If still failing after 2 rounds, present the remaining failures to the user with full output. Do not describe the work as done or commit-ready while compile/build status is failing or unknown for changed production code.
Once verification passes, show a summary: how many comments were addressed, rejected, intentionally left unresolved, or still blocked. If compile/build did not pass for code changes, say the work is not commit-ready before the summary. Do NOT show a diff — the Edit tool already showed each change inline.
Proceed to Step 8 for threads that were addressed or intentionally rejected. Leave Needs decision threads unresolved and do not post replies to them unless the user provided a public rejection rationale. Do not block publishing addressed threads just because unrelated threads still need a decision.
8. Publish PR Updates
After addressed/rejected threads are ready to publish:
-
Check branch state:
-
If there are uncommitted fix changes, STOP and ask the user whether to commit them now or let the user commit manually. Do not push until the fixes are committed. If compile/build did not pass for code changes, say the work is not commit-ready and do not ask to commit until the gap is resolved or the user explicitly takes over. If the user approves committing after verification, stage only files changed for the PR feedback fixes and write the commit message from the actual diff using the subject-plus-context style from
AGENTS.md. -
Push the committed changes for the current branch:
If the branch has no upstream and the push fails for that reason, use:
Never force-push.
-
Map each addressed finding to the commit or commits that contain its fix. Use the recorded starting commit, changed-file tracking, ledger, and
git log/git showto identify the relevant short SHA(s). If one commit fixes multiple comments, reference the same commit in each review-thread reply. -
Build and show a direct review-thread reply plan before calling the API:
Thread Top-level comment ID Status Reply body Resolve PRRT_...123456789Addressed Addressed in abc1234 by adding the nil check before dereferencing.Yes PRRT_...n/aNeeds decision n/aNo Proceed without asking when every actionable reply body is either addressed or a user-approved rejection. Needs decision rows with
Reply body = n/aandResolve = Nodo not block publishing addressed threads only if they already passed the decision gate and were explicitly deferred by the user. Stop before API calls if a rejection lacks a public rationale, if a top-level comment ID is missing for a thread that should receive a reply, or if any reply body is uncertain. -
Post one direct review-thread reply for each addressed or user-approved rejected thread:
-
For addressed comments, state what changed and the commit SHA(s), e.g.
Addressed in abc1234 by adding the nil check before dereferencing. -
For rejected comments, state why it was not changed, e.g.
Not changed: this path already returns before allocation failure can be observed. -
Keep replies concise but specific. Do not over-explain or repeat the diff.
-
Do not reply to comments the user explicitly chose to leave unresolved unless needed to publish a user-approved rejection rationale.
-
Use the REST review-comment reply endpoint:
This endpoint posts an immediate reply to the existing review thread. Do not use
gh pr review, GraphQLaddPullRequestReviewThreadReply,gh pr comment, or the regular issue comment endpoint as a fallback. -
-
Resolve each addressed or user-approved rejected thread after its direct review-thread reply succeeds:
Do not resolve a thread that did not receive a successful direct reply, is still ambiguous, or still needs user approval.
-
Present a final summary table:
Thread Reviewer Location Status Commented Resolved PRRT_...reviewerfile.go:42Addressed Yes Yes PRRT_...reviewerfile.go:99Needs decision No No Include push result, branch, ledger path, rejected comments and rationale, and any comments intentionally left unresolved or not resolvable via API.
9. Post-Publish
- If the user asks for adjustments, apply them
- If the user asks to revert specific fixes, revert ONLY those changes
- Do NOT commit or push additional changes unless explicitly asked
Three fresh Copilot threads on the latest push — all circling your c3c14108f4 no-op change and the stderr pattern:
| Context | Source | Value |
|---|---|---|
| PR | #1709 | Add --worktree to entire trail checkout (draft) |
| Branch | commits | up to date, pushed through 30c2a356ed |
| Base/head | PR metadata | main <- feat/entire-trail-checkout-worktree |
Bot Comments (batched)
| # | Bot | Location | Required fix | Autofix |
|---|---|---|---|---|
| 1 | copilot | trail_checkout_worktree.go:83 | Doc comment on ensureTrailWorktreeIgnoreRule still claims the rule "is appended to the repo-root .gitignore", but appendIgnoreRule now no-ops when .gitignore doesn't exist. | Eligible — align the comment (and the PR body says the same thing; I'll fix that at publish). |
| 2 | copilot | trail_checkout_worktree.go:111 | Repo without .gitignore → silent no-op → worktree dir "can remain unignored and accidentally committed"; suggests creating .gitignore or "emitting a clear warning". | Needs decision — see below. |
| 3 | copilot | trail_checkout_worktree.go:58 | gitCommonDirForTrailWorktree failures lose git's stderr; include ExitError.Stderr like findWorktreeForBranch now does. | Eligible — the pattern now exists twice, so I'd extract a tiny gitOutputError helper and use it at both sites, with a test. |
Decision needed first
| # | Location | Reviewer | Decision needed | Why it blocks |
|---|---|---|---|---|
| 2 | trail_checkout_worktree.go:111 | copilot | Silent no-op vs. stderr notice vs. create .gitignore when the repo has none. | Your c3c14108f4 made this a deliberate silent no-op — your own test (TestEnsureTrailWorktreeIgnoreRule_MissingGitignoreNoop) pins the silence. Copilot is asking to undo that. |
The evidence says you chose silence on purpose ("that's the user's choice"). Three ways to answer Copilot:
- Reject with rationale — e.g.: "Intentional: the CLI doesn't impose ignore policy. A repo without a
.gitignorehas made that choice; creating one or nagging on every checkout oversteps. The worktree location is documented in the command help." (I need your sign-off on a public rationale to post.) - One-line stderr notice — not a prompt, just
note: .entire/worktrees is not git-ignoredwhen no.gitignoreexists. Softens the silence but reverses the behavior your test pins. - Create
.gitignore(0644) — what Copilot prefers; contradicts your stated direction, mentioned for completeness.
Which way for #2? (#1 and #3 proceed as autofixes once you answer — the gate blocks them until the decision's made.)
I've already responded to the comment and resolved it. We should, however update the PR description and make this behavior explicit.
Fixes verified and PR description updated:
| Command | Exit | Relevant output | Full log |
|---|---|---|---|
go test -run '…SurfacesGitError|…IgnoreRule|TestCheckoutTrailWorktree' ./cmd/entire/cli/ -count=1 | 0 | ok … 1.272s (new common-dir stderr test red→green) | n/a |
mise run fmt && mise run lint | 0 | clean | n/a |
What happened per thread: #1 — doc comment now states the exists-only, opt-in behavior explicitly; #2 — yours, already resolved; the PR body's ignore-handling bullet now says a repo without .gitignore is deliberately left untouched (no creation, no warning), and the spec matches. #3 — extracted gitOutputError (the ExitError.Stderr surfacing now lives in one helper used by both gitCommonDirForTrailWorktree and findWorktreeForBranch), with a red→green test pinning the common-dir path.
The code fixes are uncommitted. Proposed commit (the two feature files):
surface git stderr from common-dir resolution
Extract the ExitError.Stderr formatting into gitOutputError and use it for both git invocations that parse stdout. Align the ignore-rule doc comment with the append-only-to-existing-.gitignore behavior.
Approve and I'll commit, push, then reply-and-resolve threads #1 and #3:
| Thread | Top-level comment ID | Status | Reply body | Resolve |
|---|---|---|---|---|
…QBLve | 3562109209 | Addressed | Addressed in <sha>: the comment now documents the append-only-to-an-existing-.gitignore behavior; the PR description was updated to make it explicit too. | Yes |
…QBLvt | 3562109228 | Handled by you | n/a | already resolved |
…QBLv0 | 3562109241 | Addressed | Addressed in <sha>: extracted gitOutputError so both stdout-parsing git calls (rev-parse --git-common-dir, worktree list) surface git's stderr on failure. | Yes |
All published:
| Thread | Reviewer | Location | Status | Commented | Resolved |
|---|---|---|---|---|---|
…QBLve | copilot | trail_checkout_worktree.go:83 | Addressed in 5362c3787f | Yes | Yes |
…QBLvt | copilot | trail_checkout_worktree.go:111 | Handled by you directly | by you | by you |
…QBLv0 | copilot | trail_checkout_worktree.go:58 | Addressed in 5362c3787f | Yes | Yes |
Push landed 30c2a356ed..5362c3787f, both replies reference the commit, both threads resolved, and the PR description now states the opt-in ignore behavior in plain terms. Review queue: empty. Ledger: kept inline (no project-local artifact dir).