Repository navigation
Automatically open routed workspaces with isolated WARP sidecars - #249
Merged
Merged
Conversation
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2cde25760b
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Owner
Author
Code Lawyer Activity Summary
Verification
CodeRabbit was rate-limited on the latest delta, so the merge gate remains locked pending a substantive current-head review. @codex review please |
|
Codex Review: Didn't find any major issues. Keep them coming! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
…pen-workspaces # Conflicts: # CHANGELOG.md # docs/method/backlog/dependency-dag.dot # docs/method/backlog/dependency-dag.svg # package.json # schemas/graft-structural-history.echo-package.json # src/mcp/daemon-server.ts # src/mcp/server.ts # src/mcp/warp-pool.ts # src/mcp/workspace-router-model.ts # src/mcp/workspace-router-runtime.ts # src/mcp/workspace-router.ts # test/helpers/daemon.ts # test/unit/helpers/mcp.test.ts # test/unit/mcp/warp-pool.test.ts # test/unit/mcp/workspace-binding.test.ts
…not refused as symlink aliases
…yed sidecar resident identity
Brings in #261 (GRAFT_ROOT_PATH), #260, #256 and #254. Conflicts resolved: - src/mcp/daemon-server.ts: keep main's injected env for the daemon root, socket, startedAt and incarnationId, and this branch's options-object InMemoryWarpPool with its graph root. - vitest.config.ts: register both setup files for now; the next commits replace this branch's HOME-redirecting setup with main's GRAFT_ROOT_PATH one. - CHANGELOG.md, docs/MCP.md: keep both sides' entries and paragraphs. - docs/method/backlog/dependency-dag.{dot,svg}: regenerated with scripts/generate-backlog-dependency-dag.ts (bad-code 35). Known at this commit: pnpm lint fails on src/warp/sidecar.ts reading os.homedir, which main's new rule forbids; fixed by the next commit.
… the home directory The default graph root is now <graft root>/graphs: GRAFT_ROOT_PATH/graphs, or ~/.graft/graphs while the variable is unset. createGraftServer and startDaemonServer resolve it from their injected env, as #261 did for the daemon root, so a host's GRAFT_ROOT_PATH decides it rather than the process's. src/warp/sidecar.ts no longer reads os.homedir, which main's lint rule forbids. RED (before this change): graft-root.test.ts failed three tests, the graph root resolving to /srv/graft/.graft/graphs, to the redirected test HOME, and os.homedir being called twice; the daemon test's sidecar was absent under the injected root (ENOENT). GREEN: both files pass.
… temp directory Removes test/setup-hermetic-env.ts and test/unit/helpers/hermetic-environment.test.ts (b899e38, 8232fff). Main's test/setup-graft-root.ts now isolates every per-user default, the WARP graph root included, through GRAFT_ROOT_PATH, and graft-root.test.ts checks that HOME is left as the process received it. The canonical-TMPDIR half of 8232fff is still needed: the suite's Graft root and many helpers' graph roots are created under the temp directory, and sidecar storage refuses a symlink-aliased root. Without it a full run failed 275 tests with "Refusing symlinked Graft graph storage path". It moves to its own setup file, test/setup-canonical-tmpdir.ts, registered before the Graft-root setup, with its test in test/unit/helpers/canonical-tmpdir.test.ts. RED: with setup-graft-root registered before the HOME redirect, "leaves HOME as the process received it" failed (/Users/james vs a graft-test-home temp dir); canonical-tmpdir.test.ts failed both tests with the canonical setup unregistered (config restored byte for byte, sha256 14dae289...). GREEN: both files pass.
… re-deriving a default createDaemonSessionHost now carries the daemon's graph root to each session's createGraftServer. Before, every daemon session resolved its own default graph root from the env; since the previous commit that goes through graftRootPath, so a daemon started with an explicit graftDir, socketPath and graphRoot but a relative GRAFT_ROOT_PATH in its env refused every session. RED: the new daemon-server test "gives its sessions the daemon's explicit graph root instead of re-deriving a default" got HTTP 500 for initialize. GREEN: daemon-server.test.ts passes, 15 of 15.
… user-facing docs README, ADVANCED_GUIDE, ARCHITECTURE, docs/CLI.md, docs/SETUP.md and the indexing invariant still named ~/.graft/graphs as the only location. Since the graph root now derives from graftRootPath(), they say it moves with GRAFT_ROOT_PATH and is ~/.graft/graphs only while that is unset. Documentation only; no runtime change.
…_PATH, must be a canonical path Sidecar storage refuses a graph root reached through a symlink. With the default now under GRAFT_ROOT_PATH, a value spelled through /tmp or /var on macOS is refused for graph storage although the daemon root accepts it; the test suite needed test/setup-canonical-tmpdir.ts for exactly this reason.
…s by worktree too The card, merged from main, says the pool holds four (repoId, writerId) handles. On this branch each resident is keyed by (repoId, worktreeId, writerId); the card now says so.
…test replaces project-root-resolution.test.ts gives createGraftServer an explicit env, and server.test.ts starts its stdio child with only the SDK's default environment plus its own. Neither carried GRAFT_ROOT_PATH, so once the WARP graph root derived from the injected env (and HOME was no longer redirected) both opened sidecars under the developer's real ~/.graft/graphs. RED (first failure, full run at 533e8dc's successor 3d8bd4e..08d81fd, one run only, not replayed because replaying writes into the real home): the run added ~/.graft/graphs/graft-root-test-{huwajh,pqjord}--* and graft-mcp-stdio-zsicot--*. GREEN: both files pass; the next full run's listing comparison is the check.
…where it is first read, and keep refusing symlinks after that
Owner
Author
Code Lawyer Activity Summary
Local gates at |
… without this change; it stays under Unreleased
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Background Context
Graft's daemon keeps a WARP graph per repository. Until now that graph lived in the source repository's own Git objects and refs, and a routed tool call could only run in a workspace the session had opened first.
The Problem
Writing graph state into source repositories mixes Graft's data with the user's history, and linked worktrees and separate sessions could share one working graph. Routed calls given an explicit
cwdin an unopened worktree also failed instead of admitting it.The fix
<graft root>/graphs, derived fromGRAFT_ROOT_PATHand so~/.graft/graphswhile that is unset. Each sidecar is keyed by repository, worktree and actor, and the source repository's refs, objects, config and hooks are left alone.cwdadmits its canonical containing worktree, without changing the session's active binding./tmp) works. Storage then refuses symlinks anywhere in its tree, and refuses roots that overlap the source worktree or its Git directory.test:localandtest:watchare removed andrelease:surface-gateruns throughpnpm test, so every supported Vitest entry point uses the copy-in, no-mount Docker harness.The branch is merged with current
main, including the bounded resident pool (residents are now keyed by worktree as well) andGRAFT_ROOT_PATH. The test suite no longer redirectsHOME.Validation
At
adc9a1b1,pnpm lintandpnpm typecheckpass. A full local run passes 2,460 of 2,463 tests; the 3 failures are playback tests that time out locally onmaintoo. A full run leaves the developer's~/.graftunchanged. Sidecar tests cover symlinked roots resolving to one location, and a root swapped for a symlink after startup being refused.Known limits:
refs/warpstate is not migrated.Summary by CodeRabbit
New Features
Bug Fixes