Repository navigation
feat(agent): select workspaces by path, not by directory selection - #6632
Conversation
EnglishThanks for tracking this down and putting up a draft quickly! Looking closer at the contract, caller-supplied
简体中文感谢快速定位问题并提交初版实现! 仔细审视现有契约后,让调用方传入
|
|
Reworked along those lines in e74cf5b (on top of the earlier commit, for squash) and the description is rewritten.
Not done: the task-creation check in WorkHub now compares the created session's path with the route's resolved path rather than identities, since the worker resolves the path on its side. Say if you would rather it re-resolve through the worker's |
Astro-Han
left a comment
There was a problem hiding this comment.
Automated review notice: This comment was posted by an automated review agent. It is not an independent human review and does not replace one.
This is a focused review of WorkHub coordinator recovery and workspace-identity handling at dcb77eec7a9aaa82967440838f0d836bb7b9ff25, not a complete review of all 44 changed files. I found two P2 issues, detailed inline: existing coordinator intents no longer decode, and new pending intents lose the directory identity needed to prevent retargeting during recovery.
The path-only public interface removes the need for callers to construct Host-owned directory selections. The new resolver expands the supplied home directory, normalizes and canonicalizes paths, selects a configured location and captures a directory identity. That interface change does not require dropping the Host's durable physical-identity expectation after it has resolved a path.
I read the old/current coordinator intent declarations, their plugin-data write/read and coordinator/resolve dispatch, the pending-to-create boundary, Owner::create and project-workspace creation, and the workspace resolver/initializer. A controlled Rust decoder check reproduces the object-to-string incompatibility using the actual declarations; recovery metadata is represented opaquely and the directory-identity adapter uses a supplied valid-format string, so this is not a native Root-reopen test. A separate check runs the actual resolve_path function against real directories and confirms that replacement preserves the canonical path while changing the physical directory identity. The two inline comments explain how these observations reach the production recovery paths.
The actual Git merge against rust-preview at c00ee9921a3df098ca9c9723d850c34307476370 succeeds. CI 41860448684 is still running at publication; its dependency audit 41860449457 succeeds. I did not run the complete native Host or verify all new UI, ACP, import, task-routing and worktree paths. The remaining persisted task-target/allocation shape changes also need a complete compatibility review. The author-reported model behavior and test results remain the author's evidence.
Automated review notice: This comment was posted by an automated review agent operated by Astro-Han. It is not an independent human review and does not replace one.
| operation: RecoveryRef, | ||
| workspace: crate::contract::DirectorySelection, | ||
| /// The coordinator's directory as the caller named it. | ||
| workspace: String, |
There was a problem hiding this comment.
P2: Read or migrate the existing durable coordinator intent before requiring a string.
The previous build writes Intent.workspace as a DirectorySelection object (rootId, segments, directoryIdentity) under this same WorkHub plugin-data key. The new intent() immediately deserializes that record into this struct, so it returns workhub_intent_invalid for an existing record. Both coordinator and resolve call intent() before observe(): this also prevents returning an already-created, otherwise healthy coordinator, not just resuming an unfinished creation.
A controlled Rust codec check using the actual old/new Intent declarations accepts the old workspace object with the old decoder and fails with invalid type: map, expected a string with the new decoder. Unrelated recovery metadata is opaque in that check; the production write/read and pre-observation order were checked separately in this file. The description's no-compatibility statement does not provide migration or operator recovery for these persisted records.
Add a versioned legacy reader or migration which preserves the original operation and already-created coordinator, and test reopening a Root with an actual pre-change intent. Do not silently discard the intent and create another coordinator.
Automated review notice: This comment was posted by an automated review agent operated by Astro-Han. It is not an independent human review and does not replace one.
| labels: vec![], | ||
| workspace: crate::contract::WorkspaceSelection::Directory { | ||
| selection: intent.workspace, | ||
| path: intent.workspace, |
There was a problem hiding this comment.
P2: Keep the first directory's physical identity in the durable intent used for resumed creation.
For a new intent, resolve() calls workspace::selected, but only tests Ok(Some(_)) and discards the returned Located.identity; the durable record keeps just the caller's path. If execution stops after that record commits and before coordinator creation, replacing the directory at the same path causes retry to reach this branch with the original operation but a different directory. Owner::create → create_project_workspace → workspace::initialize captures that replacement's new identity, with no original identity left to reject it. The original DirectorySelection carried that expectation across the same boundary.
I checked the actual new resolve_path function with a real rename-and-recreate operation: the resolved canonical path stays equal while the directory inode changes; retaining the original identity would detect the replacement. This is a controlled filesystem check plus the inspected production call chain, not a full Host restart test. Persisted Workspace validation after a Session exists does not cover this earlier pending-intent window.
Keep the public path-only input, but freeze the Host-resolved path and identity internally before recording the intent and require that identity when resuming creation. Add a stop/reopen/replace-directory regression which refuses the replacement under the original operation.
Automated review notice: This comment was posted by an automated review agent operated by Astro-Han. It is not an independent human review and does not replace one.
Astro-Han
left a comment
There was a problem hiding this comment.
Automated review notice: This comment was posted by an automated review agent. It is not an independent human review and does not replace one.
A further P2 is now confirmed in the running CI for dcb77eec7a9aaa82967440838f0d836bb7b9ff25: the shared real-Host fixture still sends the removed directory-selection input. This supplements the two recovery findings above; it remains a scoped result rather than completion of the 44-file review.
In CI 41860448684, desktop shards 1/4, 2/4 and 3/4 each fail all five tests at support.rs:464 while preparing agent.create: {"kind":"directory","selection":null} fails the new workspace schema. The unchanged fixture at support.rs:326 reads roots["roots"][0]["selection"], but this PR removes that output field and requires a path input. The side-pane test also builds that old request directly at desktop_new_session.rs:669.
I compared the actual source and read all three failed job logs (126686086921, 126686087188, 126686086980). On the current target c00ee992, the matching five tests in desktop shard 1/4 pass in CI 41853005558, job 126681643056; I read its original passing log as the control. This attributes these 15 failures to the incomplete caller/fixture migration in this PR, separately from the older external-agent UI wait.
Please migrate the shared fixture and remaining direct test callers to path and rerun the desktop matrix. These failed cases stop during fixture setup and do not validate the intended UI behaviors. I did not rerun the full native application locally.
Automated review notice: This comment was posted by an automated review agent operated by Astro-Han. It is not an independent human review and does not replace one.
| @@ -377,7 +386,6 @@ pub enum Directories { | |||
| pub struct Directory { | |||
| pub name: String, | |||
| pub path: String, | |||
There was a problem hiding this comment.
P2: Migrate the shared real-Host fixture and direct test callers when removing Directory.selection.
Fixture::open_with_configuration in terminal-shell/tests/real_host/support.rs:326 still sends workspace: {kind:"directory", selection: roots["roots"][0]["selection"]}. With this output field removed, indexing produces null, and the new WorkspaceSelection::Directory { path } schema rejects the input before the test UI starts. desktop_new_session.rs:669 repeats the old request directly.
Exact-head CI 41860448684 jobs 126686086921 / 126686087188 / 126686086980 each have 0 passes and 5 failures with that same selection:null error at support.rs:464. The same five shard-1 cases pass on target c00ee992 in job126681643056, and the helper source itself is unchanged. This is a PR-introduced CI regression. Update these callers to use the returned path and run the affected matrix so these tests can exercise their actual UI scenarios.
Automated review notice: This comment was posted by an automated review agent operated by Astro-Han. It is not an independent human review and does not replace one.
EnglishThanks for the clean rework—the overall contract direction looks great! A few concrete issues to fix before merging, plus notes on WorkHub and storage migration:
简体中文感谢快速完成重构,整体契约方向非常清晰!合并前还有几处具体问题需要处理,顺便回答 WorkHub 校验与本地存储迁移的问题:
|
SessionCreate takes a directory workspace as rootId, segments and directoryIdentity, which no model tool returned, so models asked for a conversation in a directory guessed them. The agent service's directories query, which the workspace picker uses, is now the model tool WorkspaceLookup, and it resolves an absolute path (~ allowed) to the directory with its selection within the configured locations, or to null with the locations listed. The Bot holds it with its other coordination tools, and the SessionCreate description says where a selection comes from. Closes #6630. Generated-by: Claude Code
Callers named a workspace as rootId, segments and directoryIdentity, values only the Host could know, so models asked for a conversation in a directory guessed them and the UI and ACP walked the directory tree to look them up. Workspace-selecting inputs now take a path: the Host expands a leading ~, normalizes and canonicalizes it, finds the most specific configured location holding it, excludes managed worktrees and private paths, and captures the directory identity as it opens the directory. Workspace.directory_identity stays Host-persisted state that validate checks. The directories query remains for UI browsing, paged by path; the ACP tree walk is removed, and so is the WorkspaceLookup tool this branch had added. Closes #6630. Generated-by: Claude Code
Generated-by: Claude Code
Migrate the remaining callers that still sent a directory selection: the real-Host desktop fixtures, the crash integration test, the Host benchmark client and the host-comparison script. Recent workspaces keep only the path, which the Host resolves on creation. maka-workspace-support now compares canonical paths with canonical roots and exclusions, so Windows verbatim prefixes no longer defeat the managed-worktree and private-path checks, and resolves paths inside the blocking task rather than on the executor. WorkHub freezes the Host-resolved path in its coordinator intent and checks only the shape of a worker's created workspace, leaving path resolution to the worker. Generated-by: Antigravity
dcb77ee to
6c6a4cb
Compare
Astro-Han
left a comment
There was a problem hiding this comment.
Automated review notice: This comment was posted by an automated review agent. It is not an independent human review and does not replace one.
I reviewed the final 59-file diff at 6c6a4cbc62e6771d40b56924cd4721553b3711d2, including the path resolver, Agent/WorkHub inputs and persisted records, project/import/worktree handling, ACP, recent-workspace storage, UI callers, generated contracts and tests. I separated the rebased changes with git range-diff and checked the final follow-up commit. The merged source tree at 445767cc23447e97411457d7affd10e2f01b5abf is identical to this PR head.
One previously reported P2 remains: a pending WorkHub coordinator intent still drops the first directory's physical identity. Canonicalizing its path fixes relative/home-path reinterpretation, but does not prevent replacement at that same canonical path. The inline comment updates the finding for this final commit.
The other two earlier findings have changed:
- The shared real-Host fixture and direct callers now use
path. I read the original logs from all four desktop jobs in the successful exact-head CI: 5 + 5 + 5 + 4 tests pass. This resolves the earlierselection:nullsetup failure; I did not rerun the native application locally. - Existing records still require migration, but there is now a documented manual migration script. I ran its actual downloaded bytes on disposable SQLite samples: report-only, four record transformations with revision increments, idempotence, refusal of an unmapped root, refusal of an unknown selection-bearing record, and refusal while the writer lease is held. An extracted Rust codec accepts its transformed coordinator intent and preserves the opaque original operation; an unmigrated object remains rejected. This verifies the explicit migration path on synthetic records, not a complete pre-upgrade Root reopen or a valid native recovery operation. Users must stop Maka, back up the state root and perform the documented migration; there is no automatic compatibility reader.
Local verification also ran the actual four conversation-group tests on Node 22: all pass; substituting the pre-change grouping module makes one fail. A separate Rust check used the actual final Intent declaration and resolve_path function with a real rename/recreate operation: after persisting the canonical path, it resolves to the same pathname but a different inode. Keeping the original identity would reject that replacement. Recovery metadata and physical identity have controlled adapters in this check; it is not a native Host restart test.
I did not verify live model selection behavior, complete old-state migration, native UI operation or Windows/macOS path behavior locally. The model observations in the PR description remain the author's evidence. The PR is already merged; the old pre-merge conflicts no longer apply. At this check the successor Rust commit is a61a85d1; its CI is still running, so this review does not claim that successor is fully validated.
Automated review notice: This comment was posted by an automated review agent operated by Astro-Han. It is not an independent human review and does not replace one.
| let candidate = Intent { | ||
| operation, | ||
| workspace: input.workspace, | ||
| workspace: located.path, |
There was a problem hiding this comment.
P2: Preserve the original physical identity alongside the canonical coordinator path.
located.path now freezes the Host-resolved pathname, but located.identity is still discarded. If the durable intent commits and creation stops before a Session exists, renaming that directory away and creating another directory at the same path leaves an intent with the original operation and no expected identity. On retry, resolve() skips first-time selection, then Owner::create → create_project_workspace → workspace::initialize captures the replacement's identity.
I reran a controlled Rust check with the actual final Intent declaration and resolve_path: a serialized/reloaded intent keeps the canonical path and original opaque operation, while a real rename/recreate produces a different inode at that path. An original-identity guard rejects the replacement. This is source-chain inspection plus an actual filesystem/codec check, not a native Host restart.
Persist the Host-captured identity in the intent and require it before resumed creation; canonical path equality alone does not close the previously reported recovery window.
Automated review notice: This comment was posted by an automated review agent operated by Astro-Han. It is not an independent human review and does not replace one.
Closes #6630.
Callers no longer hand the Host an
AgentDirectorySelection(rootId,segments,directoryIdentity); they name a directory bypath, and the Host resolves and identifies it. Models asked to create a conversation in a directory had been guessing those three values (K2.8 Preview sent the SHA-256 of the empty string; K3.1 Pro spent 59 tool calls disassembling the binary for it), and the UI and ACP had been walking the directory tree to look them up.Resolution lives in
maka-workspace-support: a leading~is replaced by the home directory the owning service passes in (the Agent owner reads it once from the process environment),.and..are resolved before symbolic links are followed, the most specific configured location holding the path wins when locations overlap, managed worktrees and private Host paths are excluded, anddirectory_identityis captured as the directory is opened.Workspace.directory_identitystays as it was: Host-persisted state thatvalidatechecks so a session fails closed when its directory is replaced on disk.Inputs that now take a
path:agent.create/SessionCreateworkspace:isolated_git,projectanddirectory;sessionis unchanged.selectWorkspace({path}),projectskind=directory,projectregisterandrelocate.importlocations andcopy.resolve,workhub_tasksroutetargets androute_preset, and the graph routing.directoriesstays for tree browsing:roots, andchildrenkeyed bypath+after.AgentDirectorycarriesnameandpath;AgentProjectandAgentProjectLocationlost theirselection. The workspace picker browses by path, and "open folder" only has to make sure a location holds the path. ACP'slist::selecttree walk is gone;NewSessionpasses the cwd.Desktop recent workspaces keep only the path; the stored
workspacefield of older entries is ignored.Migrating local data
The code keeps no compatibility readers. Plugin-data records written before this change no longer decode: Agent import sources (
location.directory), worktree allocations (selection), WorkHub coordinator intents and task targets (workspace). A one-off script rewrites them in place: migrate-pr6632-workspace-paths.py.It takes each database's writer lock and refuses while a Host holds it. Import sources and allocations take the path already stored on the same record. WorkHub records hold none, so the script asks for
--root root-N=/pathfor each configured workspace location they name (root-1is the first). Any other record still holding a directory selection aborts the run before anything is written.Verification: workspace-support unit tests (paging with inaccessible children; path,
./.., overlapping locations, outside/missing/file/relative/managed;~expansion), the native suites for workspaces, projects, imports, worktrees, WorkHub, skills, session context, file browsing, MCP and the Bot, the CLI ACP tests, the crash integration test, the desktopdesktop_new_sessionsuite, SDK/bundle/workbench type checks and the workbench script tests.AI use
Tool(s) and scope: Claude Code wrote the change and this description; its commits carry
Generated-by: Claude Code. Antigravity wrote the review follow-up (remainingselectioncallers, Windows path comparison, blocking resolution, WorkHub creation check), the migration script and its section here; its commit carriesGenerated-by: Antigravity.