chore: move worktrees in-repo, ship hooks with the skill - #77
Conversation
Every entry is something hit for real this session: a branch locked by an idle agent's worktree, the placeholder worktree-<id> branch that isn't where the work is, `mv` breaking a worktree where `git worktree move` preserves it, and worktrees surviving between agent runs. Also separates the failures that merely surfaced during worktree work from the ones worktrees actually cause. --force-with-lease failing on every branch is a shallow clone; a fan-out of rebases is main moving under an open stack; a rejected push may be a remote that was rebased independently, where replaying the missing commit beats force-pushing over it. Misdiagnosing those as worktree problems is what cost the most time, so they are named rather than left to be rediscovered. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01CDv57zHq7abms3RReSQw6q
thecodedrift
added
the
skip-changeset
Worktrees live at ../<repo>-worktrees/ now, which is outside the project root, so a fresh session or a teammate would be prompted for every file operation there — the placement fixed the tooling problem but left the permission scope behind it. Scoped to that one sibling directory rather than the parent, and the read.deny globs are all **/-prefixed so keys, .env files, and credentials stay denied inside worktrees exactly as they are inside the repo. Note the path is literal: it matches a clone directory named `skills`, which is what the create hook derives `skills-worktrees` from. A clone under a different directory name gets a correspondingly different worktree directory and would need this entry adjusted. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01CDv57zHq7abms3RReSQw6q
thecodedrift
changed the title
The sibling location could not be referenced portably from committed config: its path derives from the clone directory name, so a teammate whose checkout is not named `skills` gets permission prompts for every file operation — silently, and per person. `<repo>/worktrees/` is the same path in every clone. The cost is that root-level tooling walks into it, so `worktrees/` is excluded in .gitignore, .prettierignore, and eslint.config.js. Prettier is a new entry rather than an oversight: its globs skip dotfiles, so `.claude/worktrees/` had been invisible to it. Measured as NOT needing one: pnpm workspaces (the glob is root-anchored) and tsc (per-package via turbo). All three ignores verified against a live worktree. The hook scripts move to `scripts/` under the skill, the way iterate-pr ships its own, so a repo installing this skill through dotagents gets them. settings.json references them by project-relative path, which is stable across repos. The skill gains a Setup section with the settings.json block and a way to test the hooks by piping them a payload, plus ignore examples for eslint (flat and legacy), prettier, biome, ruff/black, tsconfig, and a verification recipe — since which tools traverse differs per repo and guessing is what caused the prettier miss here. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01CDv57zHq7abms3RReSQw6q
thecodedrift
changed the title
|
@claude /review |
|
Claude finished @thecodedrift's task in 3m 21s —— View job Review of PR #77
SummaryGood change overall — the hook scripts, ignore entries, and the new gotchas/remediation sections in Findings1.
This directly contradicts the "Setup (per repo)" section this same PR adds (lines 63–195), which correctly documents the new in-repo As written, the doc opens by confidently telling readers the opposite of what the hooks now do, and closes the same way. Since the whole point of this PR is the operational knowledge in this skill, this self-contradiction is the one thing worth fixing before merge — please update the intro and cleanup examples to use 2. Stale Line 62: What looks solid
Did not run build/lint/test locally per instructions — CI will report those separately. Branch: |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
Review caught the doc opening by telling readers the opposite of the setup section further down. I rewrote the intro for the sibling model, then moved worktrees in-repo and only *added* the Setup section without going back — so the intro, its rationale, and the cleanup example all still described `../skills-worktrees/`, and the intro credited the hook scripts to `.claude/hooks/` after they moved under the skill. For a skill whose entire purpose is telling people where worktrees go, contradicting itself is the worst available bug. The sibling model is now recorded as the rejected alternative with the reason it lost — its path depends on the clone directory name, which committed config cannot know — rather than deleted, so the next person does not re-propose it. Also fixes the `statusMessage` left reading "Creating worktree beside the repo". Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01CDv57zHq7abms3RReSQw6q
Both fixed. Finding 1 was the important one and you characterised it correctly — the doc opened by telling readers the opposite of its own setup section. The cause: I rewrote the intro for the sibling-directory model, then moved worktrees in-repo and only added the Setup section without going back through what was already there. So the intro, its rationale, the cleanup example, and the I swept for the pattern rather than fixing only the lines you cited — five stale references in total across The sibling model is kept as a recorded rejected alternative with the reason it lost — its path depends on the clone directory name, which committed config cannot reference portably — rather than deleted, so it does not get re-proposed later. — AI Coding Agent |
A top-level review comment has no thread to resolve, so the skill had to dedupe by scanning existing comments for a reference marker citing the same author and snippet. That is fragile, and it failed in practice — on #75 and again on #77 the summary comment kept reporting as unaddressed after it had been answered, leaving a permanent needs_attention to reason about by hand every pass. A hooray reaction on the original is a machine-readable acknowledgement. fetch_pr_feedback now reads reactions.hooray, marks the item acknowledged, and buckets it as resolved, so a re-run reports zero instead of re-surfacing it. Verified on #77: needs_attention went 2 to 0 with no other change. The reaction step is documented against the PR-scoped comments endpoint specifically. The repo-wide repos/{owner}/{repo}/issues/comments returns every comment in the repository, and selecting from it will eventually react on another PR's comment — I did exactly that here and got the right answer by luck. Items now carry comment_id so the id comes from the feedback data rather than from matching body text. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01CDv57zHq7abms3RReSQw6q
A top-level review comment has no thread to resolve, so the skill had to dedupe by scanning existing comments for a reference marker citing the same author and snippet. That is fragile, and it failed in practice — on #75 and again on #77 the summary comment kept reporting as unaddressed after it had been answered, leaving a permanent needs_attention to reason about by hand every pass. A hooray reaction on the original is a machine-readable acknowledgement. fetch_pr_feedback now reads reactions.hooray, marks the item acknowledged, and buckets it as resolved, so a re-run reports zero instead of re-surfacing it. Verified on #77: needs_attention went 2 to 0 with no other change. The reaction step is documented against the PR-scoped comments endpoint specifically. The repo-wide repos/{owner}/{repo}/issues/comments returns every comment in the repository, and selecting from it will eventually react on another PR's comment — I did exactly that here and got the right answer by luck. Items now carry comment_id so the id comes from the feedback data rather than from matching body text.
1 participant
Follow-up to #75, which landed the worktree hooks. This adds the operational knowledge that only came out of actually running agents in worktrees — every entry is something hit for real, with the remediation rather than just the warning.
Worktree gotchas: a branch can live in only one worktree, so editing a branch an agent holds needs a decision (hand it to the agent, work in its worktree only while idle, or reclaim it); an idle agent's worktree keeps its branch locked, with the checks to run before reclaiming it; the
worktree-<id>branch ingit worktree listis a placeholder and not where the work is;mvbreaks a worktree wheregit worktree movepreserves it and itsnode_modules; and a worktree survives between agent runs, so a resumed agent pays no second install.The more valuable half is the second section: failures that look like worktree problems and are not. Misdiagnosing these is what actually cost time.
--force-with-leasefailingstale infoon every branch is a shallow or single-branch clone — there is no remote-tracking ref to lease against. Nothing to do with worktrees.mainmoving under an open stack, which happens every time a PR merges.git rebase --onto origin/<branch> <old-base> <branch>) instead of force-pushing over it.That last one earned its place twice over while preparing this change.
mainmoved to #75's own merge commit mid-session, so a routinegit rebase origin/mainreplayed commits thatmainalready contained and--force-with-leasecorrectly refused the result. The fix was not a force-push — it was recognising that the branch was merged history and moving the one unlanded commit to a fresh branch, which is what this PR is.