fix(cli): canonicalise the workspace root at the pool - #270
Conversation
|
Live end-to-end verification added — the gap noted in the PR body is closed. The original body said this was not exercised against a live daemon, because doing so needed restarting the shared daemon several concurrent agent sessions were using. That turned out to be avoidable, and the attempt surfaced a trap worth recording. The trap: pointing every The evidence, two real
Rather than leave that as a one-off, it is now The unit tests cover the resolution; this exists because the failure was a whole-pipeline one — pin → session registry → peer lookup → routing decision → store selection → delivery, five layers that each looked correct in isolation. The remaining follow-up (the duplicate canonicaliser in |
|
Second independent review — one BLOCKING defect, now fixed, plus everything else it found. The first review's finding produced a fix that nobody had reviewed, so a fresh reviewer went at the current state. It found the fix incomplete in one place that mattered. BLOCKING — A dangerous regression in my own fix — Also fixed: Two of my changes had zero test coverage — the reviewer applied both mutations together and Recorded rather than fixed: a project trusted under the old spelling reads as untrusted under the new one, since Confirmed fine by the reviewer, by running: the two-pass shortcut is sound versus what it replaced (probed for symlink-escape, alias-inside-workspace,
|
908fbad to
831d889
Compare
Two sessions on one project reached by different path spellings — the macOS /tmp → /private/tmp firmlink, a symlinked checkout, a $TMPDIR scratch project — stored different roots, and every consumer compared them textually. So leave_note routed a same-project message cross-project, where the default config drops it unread while telling the sender the peer is in another project; and the sticky-pin guard refused a redundant session_start naming the same project by its other spelling as a pin steal. workspacePool.Detect and SynthesiseRoot now return the root through a new paths.Canonical, so the pin, the session registry, the boundary policy, the collab store and the language-server pool key all agree by construction. Canonicalising only the acquisition sites is not enough: route() compares Detect's answer against primaryRoot, so a pin canonicalised alone would acquire a second, never-reclaimed language server for every aliased path. Not in NotifyKey — that puts a syscall on the delivery hot path and fixes only the latency symptom, leaving the silent drop. Closes #263
Canonicalising the workspace root fixes the consumers fed by the pool and breaks the ones that compare the root against a path the AGENT names: one side became always-resolved while the other stayed whatever spelling the client knows. filepath.Rel then reports a file sitting in the project as an escape, which every one of these callers treats as "drop it silently". Worst case: hintRelPath returns "", so memory hint injection stops firing for the whole project with no error and no log. relevant_memories answers "not inside workspace" about a path the boundary guard admitted two lines earlier, because the guard resolves both sides and the tool did not. New paths.WorkspaceRel is the one place that question is answered. It compares lexically first — no filesystem access on the common path, which matters because hintRelPath is on the per-tool-call enrich path — and only canonicalises both sides when the raw spellings disagree, since that is either a real escape or a second spelling and only the filesystem can tell. It also fixes a latent over-rejection: three call sites used a bare HasPrefix(rel, ".."), which excluded an in-workspace directory named "..config". Found by independent adversarial review, verified end-to-end before and after.
Two real plumb serve processes, one real daemon, one project pinned by two spellings: the note between them must route same-project, arrive, and leave an aliased path resolvable inside the workspace. The unit tests already cover the resolution. This exists because the failure was a whole-pipeline one — pin, session registry, peer lookup, routing decision, store selection, delivery — five layers that each looked correct in isolation. Run against main it reproduces every symptom; the alias is built explicitly rather than relying on the platform's own, so it means the same thing on Linux as on macOS.
Round two of independent review found the first fix incomplete in one place that mattered and two that were cosmetic, plus two changes with no test at all. BLOCKING — topology.Store.toRelative had the same shape as the five sites already migrated, and this one is functional. s.workspace is the canonical root; the paths reaching it come from the agent (file_outline, workspace_symbols, topology_affected, the peer annotation). On a mismatch it handed the absolute path to WHERE f.path = ?, which matches nothing: every topology query for an aliased project returned empty, no error, nothing logged. That also half-broke peerArea, whose directory was fixed but whose topology annotation went through this. resolveCLIWorkspaceDetailed's markerless branch is reverted from SynthesiseRoot to paths.Canonical. SynthesiseRoot walks up to the nearest .git and, unlike Detect, has no $HOME guard — under a dotfiles repo it escaped to $HOME, and that value reaches config.UnsetProjectValue, so 'plumb config unset --workspace ~/scratch' would have edited $HOME/.plumb/config.toml. The daemon-parity argument for it was wrong too: the daemon only synthesises when auto_attach is on. gitCommitRepo joins the migrated sites for the same reason. Tests: the two changes that survived mutation (rootFromClient's Detect-failure fallback, the CLI markerless branch) now have guards, and four migrated sites that had none gained aliasing tests.
… TUI root WorkspaceRel's doc claimed to be 'the one place' plumb answers where a file sits within a project, which was never true — a walk rooted at the workspace, or an LSP path against the root that server was initialised with, share an origin and rightly still use filepath.Rel. It now says when to reach for it instead, and states plainly that it answers a NAMING question: because the lexical pass goes first, a path inside the workspace lexically but escaping via a symlink reads as inside. That matches what these callers always did and is harmless for choosing how to display or match a path, but it is not a containment check and the doc should not read like one. The TUI's detectWorkspaceFolder is canonicalised because it is compared against session.Folder: launched from an aliased cwd it listed one project twice in the memory picker. Also records the third upgrade effect in the changelog — a project trusted under the old spelling reads as untrusted under the new one, since the trust store keys on filepath.Abs alone. It fails closed and says so.
…the TUI root Two findings from a third independent review of the delta the second review caused — the same position where round 2 found round 1's fix wanting. toRelative lost its 'if filepath.IsAbs' guard when it moved onto paths.WorkspaceRel, which put an EvalSymlinks chain on a call that returns its input unchanged. WorkspaceRel can never answer for a relative path — Rel errors when the base is absolute and the target is not — so both passes fail after resolving the workspace root. The fswatcher hands Enqueue a relative path for every filesystem event, so a checkout or an npm install paid an lstat chain thousands of times on the single consumer goroutine: measured 2.2 ns guarded against 9.5 us unguarded. Behaviour is identical either way, so this is not unit-testable; a benchmark documents the line instead of a test that would only appear to guard it. detectWorkspaceFolder's canonicalisation was the one production line on this branch with no test — reverting it left the whole internal/tui package green, so 'every test mutation-verified' was not true of it. It has a test now.
831d889 to
0200076
Compare
|
Merging on the user's explicit instruction. Recording how this was reviewed, since the merge is not gated on a human approval. Three independent adversarial review rounds, each on a different state of the branch, because each round's fix created code the previous round had not seen:
Every fix is mutation-verified — each canonicalisation site, each behaviour of Beyond unit coverage:
Follow-up left open and tracked: #273 (two canonicalisers that disagree on the #181 case). |
2 participants
Closes #263.
Workspace roots were stored exactly as reported, never symlink-resolved, so two sessions on one project reached by different spellings — the everyday macOS
/tmp→/private/tmpfirmlink, a checkout under a symlinked parent, a$TMPDIRscratch project — disagreed about where they were.The visible damage was in the mailbox.
sameWorkspacecompares withfilepath.Cleanalone, soleave_notetook the cross-project branch for a peer sitting in the same folder, the note went to the daemon-level store, and delivery dropped it unread because[collab] cross_projectis off by default — while telling the sender the recipient "is pinned to /private/tmp/myproj". A same-project message, silently dropped, with an explanation that points away from the cause. The sticky-pin guard (#182) was hit the same way: a redundantsession_startnaming the project by its other spelling looked like a move to a different root and was refused as a pin steal.The fix, and the two traps in it
Canonicalise at the producer, not at each comparison.
workspacePool.DetectandSynthesiseRootreturn their root through a newpaths.Canonical. The pool is where plumb answers "which project is this?", so it is where that answer gets one spelling — the pin,session.Folder, the boundary policy, the collab store, the persisted pin, and the(root, language)key the language-server pool is indexed by all derive from it.Only the result is canonicalised, never
Detect's starting point: the marker walk must keep following the caller's spelling, or a project reached through a symlinked parent would search a different ancestor chain and miss the.plumb/marker beside the link.Trap 1 — canonicalising only the acquisition sites is wrong. That is the obvious reading of the issue's "fix it at acquisition", it was the first version of this change, and it introduces a worse bug than it fixes.
routingProxy.routecompares the root it detects for a file against the registeredprimaryRootand, on a miss, acquires a separate language server withpin=false— an entry the refcount path never reclaims, so it lives until the daemon exits. A canonical pin against a rawDetectdisagrees for every absolute path a client names in its own spelling: a permanently duplicated gopls per aliased project. Caught in the test logs (pool: reusing LS root=/var/…besidenew workspace root=/private/var/…) and now guarded byTestCanonicalRoot_AliasedURIRoutesToThePrimaryServer.Trap 2 — canonicalising the root breaks every consumer that compares it to an agent-supplied path. Found by independent adversarial review after this change was already "done", and verified by running it before and after. One side became always-resolved while the other stayed whatever spelling the client knows, so
filepath.Relreports a file sitting in the project as an escape — and each of these callers treats an escape as "drop it silently":hintRelPath""→ memory hint injection stops firing for the whole project, no error, no logrelevant_memoriesepisodicRelPathpeerAreasession_start's peer digest renders an empty areaworkspace_sessionsA new
paths.WorkspaceRelis now the one place "where is this file, within this project?" is answered, used by all five. It compares the raw spellings first and canonicalises only when they disagree — the common case costs no filesystem access, which matters becausehintRelPathis on the per-tool-call enrich path, and the resolution is paid only when a lexical mismatch has to be told apart from a real escape. It also retires a latent over-rejection: three of those sites used a barestrings.HasPrefix(rel, ".."), which excluded an in-workspace directory named..config.Two smaller spelling leaks closed alongside:
resolveCLIWorkspaceDetailedsynthesises a markerless root the way the daemon does instead of echoing the raw path (soplumb stats/config show/trust/run_taskkey on the daemon's spelling), androotFromClientcanonicalises its Detect-failure fallback, whichsession_startcompares against peers'session.Folder.paths.CanonicalHands the uncleaned absolute path to
EvalSymlinks.filepath.Cleancollapses..lexically, which diverges from the kernel's left-to-right resolution the moment a..follows a symlink (the divergence #264 is fixing in the boundary layer) — cleaning first would let two paths naming different directories canonicalise to one string, turning a same-place test into a false positive.A relative path is cleaned and returned with no filesystem access: resolving it would anchor it to the daemon's working directory, the silent cross-repository write of #181. A path that does not exist yet resolves its nearest existing ancestor and re-joins the tail. Resolution failure degrades to
filepath.Cleanrather than refusing — a workspace that cannot be canonicalised is still a usable workspace. It is an identity function, not an authorisation check; the boundary policy still decides what is safe.An earlier draft refused the ancestor walk whenever the path contained
... Measured rather than assumed, that guard recovers nothing in the case it targeted (the lexical collapse has already happened, so guarded and unguarded produce the same string) and loses canonicalisation where the parent is an ordinary alias. Removed.Deliberately not changed
sameWorkspaceandcollab.NotifyKeykeep theirfilepath.Cleancomparison. With canonical inputs it is sufficient, and puttingEvalSymlinksinNotifyKey— the fix the issue names to avoid — would place a syscall on the delivery hot path the in-process notifier exists to avoid, while fixing only the latency symptom and leaving the silent drop.sameWorkspace's doc comment now states that dependency, and says to fix a future mismatch at the producer rather than hardening the comparison.Upgrade effects
Both one-time and self-healing, neither a correctness loss. A persisted pin written under the old spelling is re-canonicalised and re-persisted on its first restore (verified against the schema:
pinned_workspaceisPRIMARY KEY(proxy_session_id), and every restore re-resolves through the pool, so no stale row can restore a pin to the wrong project). Strict-mode read records keyed to the old spelling will not rehydrate, so the first read of such a file is asked for again —read_trackingisPRIMARY KEY(proxy_session_id, workspace, path), so they orphan rather than conflict. Stats rows attributed to the old spelling stay in their own bucket.Verification
make verifygreen (build, full suite,golangci-lint0 issues, integration/clients vet, size, brief, tidy);make lint-crossgreen;go test -race ./internal/cli/ ./internal/paths/ ./internal/tools/green.Canonical, and each half ofWorkspaceRelreverted in turn, failing exactly the tests that claim to cover it. One test was rewritten after mutation testing showed it had become a tautology (it calledattachSyntheticdirectly, which no longer canonicalises), and one mutation was redone because the first attempt did not compile and so proved nothing.clifixtures now hand out canonical temp dirs, because the roots they stand in for are: on macOSt.TempDir/os.MkdirTempland under/var, itself a symlink to/private/var. Reverting them while keeping the fix breaks ~30 pre-existing test functions, so they are load-bearing, not masking — the aliasing they can no longer catch is covered by the new dedicated alias tests.Follow-up left open
internal/tools/boundary.go'scanonicalRoot/canonicalPathForBoundaryis a near-duplicate ofpaths.Canonical, and the two disagree on the one case that caused #181 (canonicalRootanchors a relative path withfilepath.Abs;paths.Canonicalrefuses to). They should become one definition, with the..refusal staying above it inPathPolicy.Check. Deliberately not touched here: #264 is in flight in that exact file, and it lands a property this PR must not undercut.🤖 Generated with Claude Code
https://claude.ai/code/session_01Vf6UjcJdQ8THD9NsBREAhR