feat(state): Preserve selected project roots

[reneleonhardt]

What does this PR do?

Keeps setup and runtime roots consistent across configuration, handoffs, scanning, skills, and watch state, so runtime state (handoff storage, watch state/pid/events) lives with its selected project root. State is owned per project: State.Root + ReadState reject foreign-project state in shared runtime dirs; Stop keeps the pid file for an unverified PID.

Also fixes git diff --numstat parsing: stdout is numstat only, error text comes from stderr, and a strict count guard rejects non-numstat rows — git warnings can never fabricate changed files.

Verification

• go test ./config ./handoff ./scanner ./skills ./watch

Developed with carefully directed, manually reviewed AI assistance.

[JordanCoin]

Reviewed. The root-selection work itself is coherent and the concern I raised on #94 doesn't bite here — I checked, SetSetupRoot is called once from main.go and mcp/ never imports projectpath, so there's no cross-project leak in the multi-project server. -race -count=3 on watch/ is clean.

But there's one change I'd call a blocker, and it's unrelated to the PR's stated purpose.

scanner/git.go:38 — Output() → CombinedOutput() feeds git's stderr into the numstat parser

The change was presumably to enrich the error path, but it also changes the success path: any warning git writes to stderr on an exit-0 run is now handed to the parser, which accepts any line with ≥3 whitespace fields and Sscanfs non-numeric fields to 0.

Reproduced with a repo that has a branch and a tag of the same name (routine when release tags match branch names):

$ git diff --numstat dup ; echo EXIT=$?
warning: refname 'dup' is ambiguous.
4	0	f.txt
EXIT=0

Probing scanner.GitDiffInfo directly on that repo:

Changed map has 2 entries:
   "'dup' is ambiguous."
   "f.txt"

The main --diff view happens to mask this, because it intersects the changed set against actually-scanned files. But consumers that gate on len(diffInfo.Changed) don't — main.go:404 and mcp/main.go:477 both do. So on a repo with zero real changes:

# origin/main
No files changed vs dup

# this PR
╭──────────────── ambignochange ────────────────╮
│ Changed: 0 files | +0 lines vs dup            │
╰───────────────────────────────────────────────╯
(renders a tree)

It takes the "there are changes" branch and then reports "Changed: 0 files" — self-contradictory, and it says there's a diff when there isn't.

Other realistic triggers: the CRLF warning: in the working copy of 'x', LF will be replaced by CRLF message, which fires routinely on Windows with core.autocrlf and also splits into ≥3 fields; and any textconv or external diff driver that writes to stderr. Interleaving isn't atomic either, so a warning can splice mid-line into a real numstat row.

Suggested fix: keep cmd.Output() and read err.(*exec.ExitError).Stderr for the error message — that gets the better error text with none of the contamination. Note GitDiffStats at git.go:105 runs the same command and still uses Output(), so the file is currently inconsistent with itself.

Two design questions I'd rather you decide than have me guess

Watch PID and state now follow --setup-root. watch.pid is process ownership, not reusable project policy. With setup baking --setup-root into the hook command, two sandboxes sharing a setup root can't both run a daemon, watch stop in the canonical checkout SIGTERMs the sandbox's daemon (Unix ignores root in terminateDaemon), and on Windows the ownership check returns ErrForeignDaemonPID and then RemovePID orphans a live daemon. State has no Root field and ReadState does no validation, so foreign state is served as truth — and the 30s staleness check defers to IsRunning, which reads the shared pid file and sees the sandbox daemon alive, so it never expires. If PID/state should follow the setup root, State probably needs a Root field and ReadState needs to reject mismatches.

skills/loader_test.go drops TestLoadSkillsUsesSelectedSetupRoot and its linked-worktree fixture, replacing it with a test that passes unmodified against origin/main. I restored the deleted test against this PR's loader.go and it passes — so it wasn't removed because it broke. After this PR nothing in skills/ covers linked-worktree discovery.

Minor: the whole config/config.go diff is one inserted blank line, but the PR ships 54 lines of config tests that also pass on main — config/ reads as participating in the feature when functionally it doesn't. And skills.LoadSkills can now return an error where it previously always fell back to builtins, so list_skills / GET /api/skills can return zero skills on a malformed root; that's defensible fail-closed behavior but isn't mentioned in the body.

Happy to be wrong on the watch-root question — that one's a genuine design call, not a defect.

[reneleonhardt]

Blocker — fixed as you suggested. git diff --numstat now reads stdout only and takes the error detail from err.(*exec.ExitError).Stderr; a strict count guard rejects non-numstat rows. Your ambiguous-refname repro is a regression test.

Watch PID/state vs --setup-root — decided: keep setup-root-scoped state, hardened. Runtime state follows the selected project root, and it's owned per project: State records its Root, ReadState rejects foreign-project state when several projects share one runtime dir (callers inside the owning project still see it), and Stop never removes the pid file for an unverified foreign PID.

Also: restored TestLoadSkillsUsesSelectedSetupRoot (dropped in an earlier conflict resolution); dropped the config tests that passed unchanged on main; condensed comments. Head ed9e18bf pushed, full suite green.

[JordanCoin]

The three things I raised are all genuinely fixed, and I verified each rather than taking the diff at face value:

• scanner/git.go is back to Output() at all three call sites. The ambiguous-ref repo now reports No files changed vs dup, and GitDiffInfo returns 0 entries instead of "'dup' is ambiguous." — while a repo with a real change still correctly reports it, so no over-correction.
• State.Root isn't just stored, it's enforced: the filepath.Rel containment check in ReadState rejects foreign state, allows subdirectories, and stays backward-compatible with old state files (Root == ""). I confirmed the write-time and read-time canonicalization agree, including across the macOS /var → /private/var symlink.
• Skills and handoff test coverage restored, plus new watch/state_test.go and scanner/numstat_test.go.

But holding on one thing, because it's a regression this PR introduces rather than a pre-existing gap.

watch.pid moved from project-local to the shared runtime dir:

// origin/main
pidFile := filepath.Join(root, ".codemap", "watch.pid")
// this PR
pidFile := filepath.Join(projectpath.RuntimeCodemapDir(root), "watch.pid")

Under an explicit --setup-root, RuntimeCodemapDir returns the same directory for every project, so two projects now share one pid file. Reproduced end to end:

$ codemap --setup-root shared watch start projA
Watch daemon started (pid 36396)
  pid file written to: shared/.codemap/watch.pid
  projA-local pid file exists? NO

$ codemap --setup-root shared watch start projB
Watch daemon already running          <- projB silently gets NO daemon

$ codemap --setup-root shared watch stop projB
Watch daemon stopped

$ ps -p 36396                          <- projA's daemon
(none — killed)

Two distinct failures: projB never gets a watcher and doesn't say so, and stopping projB kills projA's daemon. On origin/main neither can happen, because the pid path is unconditionally project-local.

This isn't only manual-CLI reachable — finishSessionDaemon in cmd/hooks.go calls stopDaemon(root) from the session-stop hook, and that inherits --setup-root through PrependSetupRootArgs. Windows is safe here (process_windows.go checks daemonOwnershipForPID and refuses on a foreign pid); macOS and Linux are not, and process_unix.go:39 says so in a comment.

You solved exactly this class of bug on the read side with State.Root — the pid path needs the same treatment, either a containment check on the pid file or the ownership check on Unix that Windows already does.

Same shape applies to handoff/storage.go, which this PR also routes through RuntimeRoot: LatestPath/PrefixPath/DeltaPath/MetricsPath will collide across projects sharing a setup root, with no equivalent containment check.

Genuinely close — the read-side design is right, it just needs to extend to the write/kill side. Everything else in the PR is good to go.

For context: #125 and #128 are merged (they're independent of this — zero file overlap), so this is the only one outstanding from the ready set.

[JordanCoin]

Heads-up on the red check — it isn't yours. Don't spend time on it.

Test (ubuntu-latest, 1.25) fails on TestDebounce; the other 11 jobs pass, including Go 1.24 and 1.26 on the same ubuntu image and all three macOS versions. A failure that lands on exactly one Go minor and not its neighbours is a timing artifact, not a compilation or logic difference.

Confirmed three ways:

1. The PR doesn't touch the code under test. The only change to watch/events.go anywhere near the debouncer is a comment reword (per save — so a trailing-edge debounce... → debounce keeps one save to one graph rebuild). No logic, no timing constants.
2. Not reproducible locally: go test ./watch/ -run TestDebounce -count=12 passes, and -count=6 -race passes.
3. The test is structurally wall-clock dependent (watch/watch_test.go:258):

time.Sleep(100 * time.Millisecond)        // hope the daemon is watching by now
for i := 0; i < 5; i++ { os.WriteFile(...) }
time.Sleep(300 * time.Millisecond)        // hope the debounce flushed by now
...
if writeCount == 0 || writeCount > 2 { ... }
if lastWriteLines != 6 { ... }

Fixed sleeps around real fsnotify delivery on a shared CI runner. Either the 100ms start-up window elapses before the watcher is actually registered (writes missed, writeCount == 0), or the 300ms doesn't cover debounce-flush plus processing (lastWriteLines != 6). Both are load-dependent, which is why it moves between runs rather than between code versions.

I've filed it separately so it stops costing contributors time on unrelated PRs. Re-running the job should go green; nothing to change here.

Separately — I still owe you a re-review of the watch.pid fix now that the head moved to d018cadd. Will get to that next.

[reneleonhardt]

No stress, my next dozen PRs keep waiting for you 😄

Of course we saw that red, and I think we even fixed it in one of the open PRs... but which one is the question, I can't keep dozens of worktrees open, MacBooks have only tiny expensive SSDs, can't even install all those amazing new small models of the last 7 days alone.

[reneleonhardt]

It's a cold-cache flake, we found a stabilization, decoupled the test's timing windows at the cost of ~4.5s/job.

TestAstGrepCallerContextOutcomes had two tight races baked into a semantics test:

1. The fake ast-grep subprocess spawn was raced against the 1s scan deadline (the cold-runner failure mode).
2. The deadline-termination was raced against a 2s post-deadline window (the second failure I hit once the first was fixed).

Both are now generous, decoupled budgets (5s each) — the test verifies cancellation semantics (deadline → subprocess terminated), not subprocess timing on loaded runners.