Skip to content

fix(arc-0d80.06mq9p): server-authoritative CWD resolution for git worktrees - #37

Merged
bfirestone merged 15 commits into
mainfrom
fix/arc-ai-path-resolution
Apr 30, 2026
Merged

fix(arc-0d80.06mq9p): server-authoritative CWD resolution for git worktrees#37
bfirestone merged 15 commits into
mainfrom
fix/arc-ai-path-resolution

Conversation

@bfirestone

Copy link
Copy Markdown
Contributor

Summary

  • Fixes arc-0d80.06mq9p: arc ai session start --stdin (Claude Code SessionStart hook) failed with CWD ... does not resolve to a known project when invoked from a git worktree.
  • Centralizes path-to-project resolution on the server side. CLI client now makes a single HTTP call instead of running its own 4-stage fallback chain.
  • Net: 13 commits, +796/-131 across 15 files. New internal/gitfs package (pure-Go worktree detection, no git binary or third-party library at runtime). New internal/testutil/gittest shared test helpers.

Root cause

The CLI's resolveFromServer had 4 fallback stages (exact / normalized / git-main-worktree / subdirectory). The server's createAISession revalidated CWDs using only stage 1 (exact match). So a worktree CWD would resolve client-side via detectGitMainWorktree, then get rejected when the client POSTed the raw worktree path back to the server.

Approach

Server becomes authoritative for path resolution (consistent with the existing workspace_paths.hostname column that already implies local-per-host scoping):

  1. New internal/gitfs package — pure-Go parser for .git pointer files. Handles normal repos AND bare-repo worktrees. ~30 lines of runtime code, no dependencies.
  2. Broadened (storage.Storage).ResolveProjectByPath from exact-match to component-wise longest-prefix match, with literal _/%/\ characters in registered paths properly escaped via SQL REPLACE (ESCAPE '\\' alone wasn't sufficient — w.path is interpolated, not bound).
  3. New (*Server).resolveProjectForPath helper — two-stage resolver (storage prefix match, then gitfs.DetectMainRepo fallback for worktrees outside any registered prefix).
  4. Migrated createAISession and GET /projects/resolve to the new helper.
  5. Collapsed CLI's 4-stage resolveFromServer to a single server call. Deleted ~88 lines of now-redundant client-side resolution logic.

Notable details

  • ResolveProjectByPath is hand-written (not sqlc) because sqlc 1.30.0 cannot parse SQLite's ESCAPE clause. Documented precedent in CLAUDE.md (ListIssues, GetLabelsForIssues).
  • The ESCAPE handling escapes w.path (the column), not the bound input — the metacharacter source is the registered path, not user input. Without this, a registered path like /home/john_doe/proj would falsely match /home/johnXdoe/projXfile.
  • Bare-repo worktrees are supportedDetectMainRepo recognizes both <workdir>/.git/worktrees/<name> and <repo>.git/worktrees/<name> layouts. Validates the latter by checking for HEAD and objects/.
  • Server is assumed local-per-host for path-aware features. Anyone running ARC_SERVER=https://remote/ for path-related features was already broken by the existing hostname column scoping; this PR doesn't change that, but documents it via behavior.

Test plan

  • Unit: internal/gitfs — 11 tests including bare-repo worktree, malformed .git, relative gitdir, deep subdirectory walk
  • Unit: internal/storage/sqlite/workspaces — 6 tests including longest-prefix-wins, component-boundary rejection, AND negative assertions that _/% in registered paths do NOT act as wildcards (lock the ESCAPE protection)
  • Integration: internal/api/ai_sessions_test.goTestCreateAISession_LinkedWorktreeOutsideRegistered does real git init + git worktree add and verifies end-to-end resolution. TestCreateAISession_RejectCrossProject confirms cross-project drift is still rejected (422).
  • Integration: internal/api/workspace_paths_test.goTestResolveProject_GitWorktree exercises the public REST endpoint against a real linked worktree.
  • Full suite (make test) green.
  • Runtime smoke: server restart with new binary required, then verify arc ai session start --stdin from any worktree no longer emits the old error. Tests prove the fix in code; activation is a deployment step.

Adversarial review pass

After the initial epic landed, an adversarial code review caught two real bugs and one coverage gap that the orthodox per-task review missed:

  • The original ESCAPE '\\' clause was performative — it declared \\ as the pattern escape character but didn't actually escape _/%/\\ in the column value. Fixed via SQL REPLACE chain on w.path. New negative-assertion tests lock it down.
  • DetectMainRepo silently failed for bare-repo worktrees. Now supported.
  • Three near-identical git-test helpers lived in three files with inconsistent CI hardening (GIT_CONFIG_NOSYSTEM/GIT_CONFIG_GLOBAL set in two of three). Consolidated into internal/testutil/gittest.

T1's history was squashed (5 commits → 1) to remove a noisy buggy/revert/reapply sandwich; backup branch retained locally.

Files

cmd/arc/ai.go                                       (-1 / +1)
cmd/arc/main.go                                     (-94 / +6)
internal/api/ai_sessions.go                         (-1 / +1)
internal/api/ai_sessions_test.go                    (+92)
internal/api/resolver.go                            (+35)
internal/api/workspace_paths.go                     (-1 / +1)
internal/api/workspace_paths_test.go                (+45)
internal/gitfs/worktree.go                          (+109)
internal/gitfs/worktree_test.go                     (+165)
internal/storage/sqlite/db/queries/workspaces.sql   (-5)
internal/storage/sqlite/db/workspaces.sql.go        (-24)
internal/storage/sqlite/workspaces.go               (-4 / +35)
internal/storage/sqlite/workspaces_test.go          (+185)
internal/testutil/gittest/gittest.go                (+63)
internal/testutil/gittest/gittest_test.go           (+59)

Closes arc-0d80.06mq9p.

ResolveProjectByPath now does longest-prefix component-wise matching with
SQL ESCAPE handling. Hand-written query in workspaces.go (sqlc 1.30.0
cannot parse ESCAPE); SQL lifted to a package-level const for auditability.
Swap s.store.ResolveProjectByPath for s.resolveProjectForPath so that
linked git worktrees outside a registered workspace prefix are accepted.
Add three behavior tests covering prefix match, linked-worktree resolution
via DetectMainRepo, and cross-project rejection.
… + escaping)

- Add GIT_CONFIG_NOSYSTEM=1 and GIT_CONFIG_GLOBAL=/dev/null to mustRun helper
  to prevent system-level git config interference on CI
- Add explicit t.Cleanup for git worktree removal in LinkedWorktreeOutsideRegistered test
- Use fmt.Sprintf with %q for proper JSON path escaping instead of string concatenation
Switch resolveProject handler to use s.resolveProjectForPath so the
GET /projects/resolve endpoint gains worktree-aware resolution. Update
mockWPStore.ResolveProjectByPath to do longest-prefix match mirroring
the real storage semantics, and add TestResolveProject_GitWorktree.
Collapse resolveFromServer to a single ResolveProjectByPath call.
Delete resolveBySubdirectory, detectGitMainWorktree, and isSubdirectory
which are now dead code — the server handles all path resolution
including exact match, longest-prefix match, and git worktree detection.
The new resolveFromServer contract always returns a non-nil error on
miss (per spec). Checks for empty projectID after a nil-error path were
unreachable dead code. Simplify both call sites and update the
resolveProject doc comment to reflect server-side delegation.

- main.go line 216: Remove unreachable serverWsID != "" clause
- ai.go line 145: Remove unreachable resolvedProjectID == "" clause
- main.go line 197: Update doc comment for resolveProject
The LIKE prefix match in ResolveProjectByPath was using raw w.path values
concatenated into the pattern, meaning _, %, and \ in workspace paths
acted as SQL wildcards and caused false-positive prefix matches. Fix by
wrapping w.path in triple-nested REPLACE calls (\ first, then %, then _)
so the column value is treated as a literal substring under ESCAPE '\'.
Three new regression tests lock in the protection for each metacharacter.
Extend DetectMainRepo to handle the case where a linked worktree's .git
pointer resolves to a bare repo (e.g. repo.git/worktrees/<name>) rather
than a normal repo (.git/worktrees/<name>). Validates the bare-repo case
by checking for the presence of HEAD and objects/ before returning the
bare repo path as the canonical main-repo path.
Consolidates three near-identical git test helper implementations across
gitfs and api packages into a single hardened package with consistent
CI-safety env vars (GIT_CONFIG_NOSYSTEM, GIT_CONFIG_GLOBAL=/dev/null).
Removes all duplicated mustRun/initRepo/addWorktree/mustRunGit functions
and their redundant t.Cleanup worktree-removal registrations.
- gofmt: trim trailing newline in ai_sessions_test.go
- gofumpt: group adjacent var decls in worktree_test.go
- gosec G306: tighten test fixture file perms 0o644 → 0o600
- gosec G703: nolint with justification on isBareRepo path joins
  (dir is a validated absolute gitdir; leaves are constants)
- mnd: extract dirPerm const in gittest for 0o755 MkdirAll
- go.mod: promote fatih/color to direct (go mod tidy)
@bfirestone
bfirestone merged commit 7b76fcb into main Apr 30, 2026
3 checks passed
@bfirestone
bfirestone deleted the fix/arc-ai-path-resolution branch April 30, 2026 22:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant