Repository navigation
fix(workspace): name a workspace by its resolved path - #1798
ningmao-hlyz wants to merge 9 commits into
Conversation
|
Hey, thanks for the PR. Could you sign the CLA when you get a chance so we can review it? |
A folder reached through a symlink became two projects, two conversation histories and two engines: engineWorkspace and /project stopped at filepath.Clean, so each spelling hashed to its own enginehost host directory and its own project row. Resolve symlinks (filepath.EvalSymlinks) at both doors where a person's path becomes a key: engineWorkspace (--daemon/--status/--stop and the host's workspace name) and runProjectCommand (the project pin). Fixes Agent-Field#1761
fd94fdf to
fa7d438
Compare
done |
ZeroPoint95
left a comment
There was a problem hiding this comment.
Hi @ningmao-hlyz, thanks for working on this fix!
I’m requesting changes because resolving the path in only some entry points introduces inconsistent workspace identities.
-
Resolve the local chat workspace before dialing or spawning. In a non-Git folder reached through a symlink,
PWDcan preserve the alias. The local surface still hashes that alias, while the daemon now resolves it and listens under a different key. The connection fails and chat falls back to running in the terminal. Please use the same resolved workspace for host lookup, dialing and spawning. -
Resolve the default home directory too.
engineWorkspace("")returnsHOMEbefore reaching the new canonicalization. WhenHOMEis a symlink, omitting--workspaceand explicitly passing the same home now select different hosts. Please route the default through the same resolution. -
Update the affected tests and cover these entry points. Five existing macOS tests now fail because their fixtures or expectations use unresolved paths, including two failures in
make test-laws. Please normalize the fixture paths and expected results, and add symlink regressions for local host startup, defaultHOME, and typed/project.
Both functional reproductions fail at this head and pass on the base. Builds, vet, changelog validation and packed-manual checks passed. CI is green, but the local laws and race runs expose the failures above; unrelated failures were separately confirmed on the base.
|
Thanks for the thorough review, and for taking the time to run the repros at this head. All three points are confirmed as real gaps in my patch:
Working on the fixes now; I will push as incremental commits and follow up here with the new SHA once all three points are addressed. |
Every identity question in this package — host lookup, dial, spawn, lock, status, stop — funnels through where, so where is the one place a symlinked spelling must resolve. A surface that reached the folder through a link and a daemon named by the resolved path hashed to two host directories, and the launch spent its wait failing to meet (Agent-Field#1761). A path that does not resolve — a workspace not on disk yet, a test's imagined directory — keeps its spelling.
engineWorkspace("") returned home without the resolution a typed home goes
through, so a home behind a symlink keyed two hosts depending on whether
--workspace was omitted or spelled out (Agent-Field#1761). The default now walks the same
door.
And the launch-side regression: a surface that dials through a symlinked
spelling must meet the host standing at the folder it reaches.
A pin names the folder the link reaches, not the spelling that was typed — the typed-path half of Agent-Field#1761's regression, beside the daemon and host halves already on this branch.
|
All three points are addressed at d26cae9, pushed as three incremental commits on top of the dev merge.
Also added the typed /project regression the review asked for (internal/tui3). Builds, gofmt, GOOS=linux vet and the touched suites pass here; the full cmd/codeaf suite still defers to Linux CI for the same Windows limitations stated in the PR description. |
The surface's own answer — v3Workspace — minted the session bucket and the tools root from whatever spelling the terminal walked in with, so on the in-process doors one folder reached through a symlink was still a second project beside the same folder named by its resolved path. The review of Agent-Field#1798 caught the host half of that split; this closes the surface half the same way, at the one place a project key is minted.
ProjectKey's own doc calls the fallback "the canonical path's hash", but filepath.Abs canonicalizes nothing about links — a folder reached through a symlink keyed two memory projects (Agent-Field#1761). EvalSymlinks after Abs, with the absolute spelling kept when the path does not resolve.
The entry described the engineWorkspace door alone. The review pass that followed spread the resolution across the host identity, the default home, the surface's project key and the memory path fallback — the entry says so, because a memory that says "fixed at engineWorkspace" would send the next reader looking for one door and finding four.
|
A second pass over the identity question before you re-run the suites, and it caught two doors the first round left split - both now resolved at ac09e71.
Both carry regressions that fail on the previous head and pass here, and the changelog entry now names every door that mints a key instead of just the first one. tui3's full suite, the touched cmd/codeaf suites, gofmt and the changelog check are green here; the Windows-only failures this machine shows are the same set the base shows. That closes every place a path becomes a key that I could find: engineWorkspace (named and default), enginehost.where, v3Workspace, gitidentity, and the /project pin. If your macOS run still surfaces a fixture mismatch, it will be a genuine one now. |
Fixes #1761.
What changed
A folder reached through a symlink became two projects, two conversation
histories and two engines. Both doors where a person's path becomes a key now
resolve symlinks with
filepath.EvalSymlinksbefore keying:engineWorkspace(cmd/codeaf/engine.go) — the resolver every enginecommand goes through (
--daemon,--status,--stop) and the source ofthe host's workspace name. It stopped at
filepath.Clean, sosha256(Clean(...))inenginehost.whereminted one host directory perspelling:
--status <link>missed the engine started at<dir>./project(internal/tui3/projectcmd.go) — the pin stored the raw typedspelling, so home keyed the folder under the alias while a launch from the
resolved path (already canonicalized by
v3OpenTarget) keyed it under thetarget: two project rows, one folder.
Regression test:
cmd/codeaf/engine_symlink_test.godrives a symlink throughengineWorkspaceand asserts both spellings land on the same directory, andthat a plain path still resolves to itself.
Changelog entry:
docs/rules/changelog.mdnames the entry for its pullrequest, and the number exists only once the pull request does — so it
arrives in the next push as
docs/changes/unreleased/<this-pr>-symlink-workspace-keys.md(
kind: fixed,surface: [engine, workspace]). The first red oncheckis that workflow, not a mistake.
How it was checked
go build ./cmd/codeaf ./internal/tui3— exit 0 (go1.26.5).go test ./cmd/codeaf -run TestEngineWorkspace -count=1 -v— both testspass:
TestEngineWorkspaceResolvesSymlink,TestEngineWorkspaceKeepsNonSymlinkPathUnchanged(ok … 2.434s). To runthis on Windows the other
cmd/codeaftest files were moved aside for therun and put back — see the limitation below.
GOOS=linux go vet ./cmd/codeaf ./internal/tui3— exit 0, the platform CIruns, so every test file (including the added one) typechecks there.
gofmton the committed blobs — clean (engine.go,projectcmd.go,engine_symlink_test.go).8761488a: a stdlib probe mirroringenginehost.whereshowed
sha256(Clean(<link>))≠sha256(Clean(<real>))— two spellings,two host directories — and that
EvalSymlinkscollapses both spellings toone host directory.
Honest limitation:
cmd/codeaf's test package does not build on Windows atall —
carried_child_test.gousesfakeFolderProgram, which lives behind//go:build !windows; the identical failure exists at base8761488a. Sothe full suite cannot run on my Windows checkout and defers to CI on Linux
(where
GOOS=linux go vetpasses).Checklist
(the number does not exist until the pull request does; see
docs/rules/changelog.md).or refusal moved (keying fix only), so
internal/manual/chat/isunchanged.
.github/known-red.txt.cmd/codeaf/engine.go,cmd/codeaf/engine_symlink_test.go,internal/tui3/projectcmd.go.