Repository navigation
feat(herdr): create worktrees under ~/Projects/worktrees and trust them in agy and Copilot - #2504
Conversation
…em in agy and Copilot
|
Warning Review limit reachedYou've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Next included review available in 16 minutes. View limit details
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
| "trustedWorkspaces": [ | ||
| "{HOME}/Projects/*", | ||
| "{HOME}/Projects/Workspace/*" | ||
| "{HOME}/Projects/Workspace/*", | ||
| "{HOME}/Projects/worktrees/*/*" | ||
| ], |
There was a problem hiding this comment.
Harness compliance
Harness compliance (AGENTS.md / CLAUDE.md): the PR body and branch are in English, but the PR title and body state the change is partial and defer the ticket. Standing Order 4 (clean as you go, track-or-fix) requires the deferred half (dotf worktree add/done, skills, AGENTS.md §9 still on sibling layout) to be ticketed with root cause. The PR body names it but does not reference a ticket for the remainder, only #2503 which is the umbrella. REAL but low severity; confirm a follow-up issue exists for the deferred half.
| "{HOME}/Projects/Workspace", | ||
| "{HOME}/Projects/Workspace/*" | ||
| "{HOME}/Projects/Workspace/*", | ||
| "{HOME}/Projects/worktrees/*/*" |
There was a problem hiding this comment.
Possible Issue
The trustedFolders hunk is malformed JSON. The diff removes the closing "{HOME}/Projects/Workspace/*" line as shown in the old hunk, but the new hunk adds "{HOME}/Projects/Workspace/*" after "{HOME}/Projects/Workspace" with a trailing comma structure that is valid only because the new array is present. Verify the file parses: jq . ai/copilot/config.json. REAL if the trailing element lacks a comma; the diff shows lines 6-7 correctly comma-separated, so this is THEORETICAL and should be confirmed by running jq.
| @test "every harness that trusts folders trusts the worktrees herdr creates (#2503)" { | ||
| # herdr puts a worktree at <directory>/<repo>/<branch-slug>. A harness that | ||
| # does not trust that path asks, or refuses, in every worktree herdr opens. | ||
| run sed -n 's/^directory[[:space:]]*=[[:space:]]*"~\(.*\)"$/{HOME}\1\/*\/*/p' "$SRC" | ||
| [ -n "$output" ] || { echo "no ~-relative [worktrees] directory in $SRC"; false; } | ||
| want="$output" | ||
| run jq -e --arg w "$want" '.trustedWorkspaces | index($w)' "$REPO/ai/agy/settings.json" | ||
| [ "$status" -eq 0 ] || { echo "agy trustedWorkspaces lacks $want"; false; } | ||
| run jq -e --arg w "$want" '.trustedFolders | index($w)' "$REPO/ai/copilot/config.json" | ||
| [ "$status" -eq 0 ] || { echo "copilot trustedFolders lacks $want"; false; } | ||
| } |
There was a problem hiding this comment.
Vacuous Assertion
The new @test derives the expected trust entry with sed -n and only asserts [ -n "$output" ]. The sed pattern requires directory = "~..." with no leading whitespace and a closing quote at end of line, so if the TOML is reformatted (e.g. a trailing comment after the value), $output is empty and the guard fires with a clear message, which is correct. However, the run jq -e ... index($w) checks exit status only, and index returns null (exit 1) on miss, so the [ "$status" -eq 0 ] check is sound. REAL behaviour is correct; no defect found on re-trace. THEORETICAL risk only: sed on a BSD host with the \/ and [[:space:]] forms is portable, so no action needed.
PR Reviewer Guide 🔍(Review updated until commit 3ce2912)Here are some key observations to aid the review process:
⚙️ Agent run details
✅ Resolved findingsai/agy/settings.json:17-21Harness compliance Harness compliance (AGENTS.md / CLAUDE.md): the PR body and branch are in English, but the PR title and body state the change is partial and defer the ticket. Standing Order 4 (clean as you go, track-or-fix) requires the deferred half (dotf worktree add/done, skills, AGENTS.md §9 still on sibling layout) to be ticketed with root cause. The PR body names it but does not reference a ticket for the remainder, only #2503 which is the umbrella. REAL but low severity; confirm a follow-up issue exists for the deferred half. tests/herdr-config.bats:43-53Vacuous Assertion The new @test derives the expected trust entry with ai/copilot/config.json:5-7Possible Issue The trustedFolders hunk is malformed JSON. The diff removes the closing |
|
/review |
| # Copilot's half is untouched: `trustedFolders` is still ours to ship. | ||
| [ "$(jq -c '.trustedFolders' "$CFG")" = '["{HOME}/Projects","{HOME}/Projects/*","{HOME}/Projects/Workspace","{HOME}/Projects/Workspace/*"]' ] | ||
| [ "$(jq -c '.trustedFolders' "$CFG")" = '["{HOME}/Projects","{HOME}/Projects/*","{HOME}/Projects/Workspace","{HOME}/Projects/Workspace/*","{HOME}/Projects/worktrees/*/*"]' ] |
There was a problem hiding this comment.
Stale Comment
REAL: the comment above the new assertion says "Copilot's half is untouched: trustedFolders is still ours to ship." This PR adds {HOME}/Projects/worktrees/*/* and {HOME}/Projects/Workspace/* to trustedFolders, so the comment is now false and will mislead the next editor of this test. Trace: a reader trusts the comment, assumes the exact-match assertion on line 136 was not changed, and skips verifying the new entry.
Review triageReviewers: PR-Agent (automatic review on
CI: every check green on |
Summary
Refs #2503. The config half of "one worktree root": herdr creates its worktrees where every harness trusts them.
ai/herdr/config.toml:[worktrees] directorymoves from~/Projects/.worktreesto~/Projects/worktrees. herdr's layout is fixed at<directory>/<repo>/<branch-slug>(only the root is configurable, per herdr 0.9.3's config reference), and a dot-prefixed root is excluded by agy (**/.worktrees/**inworkspace.excludePatterns). The comment said "not in a hidden dir" while the value was hidden; it now says what it means.ai/agy/settings.jsontrustedWorkspacesandai/copilot/config.jsontrustedFoldersgain{HOME}/Projects/worktrees/*/*. Both deploy bymerge(union), so entries a tool added at runtime survive.cli/internal/harness/model_pins.go:stripJSONCommentsis now string-aware. Its regex treated the/*inside a glob string as a comment opener; onmainit already cutai/agy/settings.jsonfrom"{HOME}/Projects/*"to the*/in"**/node_modules/**"and happened to leave valid JSON. One more glob made it invalid, and CI'sTestEveryRepoRoutingPinResolvesInTheMapandTestEveryDeclaredPinRejectsADeadIDfailed on every OS (pin "agy-model": unexpected end of JSON input). A table test pins globs, URLs, an escaped quote, the three comment forms and an unterminated block comment. Lesson 431.tests/herdr-config.bats: a new case derives<directory>/*/*from the herdr config and asserts both trust lists hold it, so the three files cannot drift apart until CLI-118: one worktree root for dotf, herdr and every harness #2503's spec renders them from one declared root.Not in this PR, and why:
dotf worktree add/done, theusing-git-worktreesandhandoffskills and AGENTS.md §9 still use the sibling<repo>-wt-<slug>, so two layouts coexist until #2503 lands. That half changes the worktree reaper and gate f (BUG-093/094 territory), migrates existing worktrees, and needs a spec;spec initrefuses while 11 specs are active against a limit of 10.Verification
bats tests/antigravity.bats tests/copilot-config.bats tests/herdr-config.bats tests/verify-setup.bats: 107 tests, 0not ok.HERDR_CONFIG_PATH=ai/herdr/config.toml herdr config check:config: ok.cd cli && go build ./... && go vet ./... && GOOS=windows go vet ./... && go test ./...: all ok.golangci-lint run(2.12.2, the pin, aftercache clean): 0 issues.TestStripJSONCommentsLeavesStringsAlone(glob, whole-line and trailing cases) andTestEveryRepoRoutingPinResolvesInTheMap.go run ./cmd/dotf deploy --dry-runfrom this worktree:would deployforcopilot-config,agy-settingsandherdr, every other entry in sync.agyandcopilotstarted in a herdr-created worktree under~/Projects/worktrees/<repo>/<slug>open without a trust prompt. The one-level*the existing entries use is taken as a path-segment glob; that is the claim the launch checks.SDD skip rationale
A config alignment plus the defect fix it exposed, whose cause was measured on this PR's CI and is pinned by a table test. The design work (one declared worktree root read by
dotf worktree, the skills, AGENTS.md and every harness) is #2503 and gets its own spec.Knowledge