Repository navigation
fix(machines): keep a split made over SSH when the machine's app is open - #297
Conversation
Splitting a pane in a project on another Mac and spawning into it made the agent a new sidebar row whenever PurePoint was also open on that Mac. Its app saved .pu/workspaces.json without checking for changes and gave the unknown agent its own workspace; the SSH client's conditional save then conflicted and took that layout, dropping the split. - Spawn requests carry an optional placement (workspace + tab). The daemon records it on the manifest entry and status report, and every app's reconciler puts the agent in that empty tab. - Local layout saves check the file's modification time and adopt a document another Mac wrote instead of overwriting it. The app also watches the file and re-checks it before reconciling. - A remote client writes its pending layout before sending the spawn. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_013bBnogGfBnCLmktcEpVNJo
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
|
Warning Review limit reachedYou've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Next included review available in 49 minutes. View limit details
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info
📝 Walkthrough
Merge Risk: ⚪ Minimal · up to
|
| Check name | Status | Explanation | Resolution |
|---|---|---|---|
| Docstring Coverage |  |
Docstring coverage is 43.09% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 123 functions across 33 files. (1 skipped… | Write docstrings for the functions missing them to satisfy the coverage threshold. |
-
Check name Status Explanation Description Check Check skipped - CodeRabbit’s high-level summary is enabled. Title check The title clearly and concisely describes the primary change: preserving an SSH-created pane split when the machine's app is also open. Linked Issues check Check skipped because no linked issues were found for this pull request. Out of Scope Changes check Check skipped because no linked issues were found for this pull request.
Full details: Docstring Coverage
-
Explanation
Docstring coverage is 43.09% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 123 functions across 33 files. (1 skipped: 1 unsupported.)
Resolution
Write docstrings for the functions missing them to satisfy the coverage threshold.
✨ Finishing Touches: 💡 1
-
📝 Generate docstrings: 💡
-
- Commit to this branch
- Create a new PR
🧪 Generate unit tests (beta)
-
- Commit to this branch
- Create a new PR
-
- Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.
Comment @coderabbitai help to get the list of available commands.
![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: c18b6cd9bf
ℹ️ 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".
| if onDisk != expectedModifiedMs { | ||
| guard let current = try? host.read(path: path, maxBytes: maxRemoteLayoutBytes), !current.truncated, | ||
| !isNewerThanSupported(data: current.data) | ||
| else { return .skipped } |
There was a problem hiding this comment.
If .pu/workspaces.json is deleted while the project is open, onDisk becomes nil while expectedModifiedMs retains the previous timestamp; this branch then tries to read the missing file and permanently returns .skipped. Because the stored timestamp is never updated, every later local layout save follows the same path, so pane and tab changes stop persisting until the project is reopened. Treat a missing file as an external empty layout or otherwise advance the expected state so the next save can recreate it.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Good catch, fixed in 8e44490. In WorkspacePersistence.saveLocal, a missing document no longer counts as a conflict: there is nothing to adopt, so the save writes the file again. saveRemote had the same problem (after not_found it still sent the old timestamp, so every save conflicted), so it now writes against the timestamp of what is actually on disk: none if the file is gone. New tests: WorkspaceRegistryTests.givenLayoutFileDeletedASaveShouldWriteItAgain and MachinesTests.givenDocumentDeletedSinceReadSaveShouldWriteItAgain. I am leaving this thread open for the reviewer to confirm.
A save expected the modification time it last saw, so once .pu/workspaces.json was deleted every later local or remote save conflicted, found nothing to adopt, and was skipped until the project was reopened. A missing document is now written again. Also stop the invalid-UTF-8 stream test failing on a broken pipe: the daemon may close the connection before the trailing request is written, which is the behaviour under test. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_013bBnogGfBnCLmktcEpVNJo
Independent review of the shared-layout fix found three ways a split could still be lost: - A save that conflicted adopted the other Mac's document wholesale, dropping this app's unsaved edits (the Mac running the project could now lose its own split to a write from an SSH client). Conflicts and external changes are now merged per workspace against the document last read or written (WorkspaceLayoutMerge): a workspace only this app changed keeps this app's version, anything the other Mac changed takes theirs, and the merged result is written back. - Point Guard workspace IDs embed each app's project key, so a placement made in Point Guard on another Mac never matched. Placements are re-keyed the way persistence re-keys the workspace. - A local project spawned before its layout was written; it now saves first too. The layout file's stamp is also taken before it is read. The isolated pane/registry verification package compiles again (it was already missing signal and transport types on main) and now also runs WorkspaceRegistryTests: 126 tests pass. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_013bBnogGfBnCLmktcEpVNJo
From a second independent review of the layout merge: - When both Macs changed the same workspace, the other Mac's version won, which could drop the tab just split here for a spawn and bring back the original bug. The Mac that saves second now keeps its version. "Changed" is measured against the base as reconciled with the current agents, so groupings both Macs derive from the manifest never count as an edit. The other Mac then adopts the result unchanged, so the writes stop. - A save that conflicted only scheduled the merged write a second later, so a spawn (or quit) could go ahead before it landed. Local saves now write the merge immediately, and a remote spawn flushes again before sending. - A remote save made when no document was known overwrote one another Mac had created since. Any document this app has not seen is now a conflict. - A spawn waiting on the layout write is dropped if its tab was closed in the meantime. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_013bBnogGfBnCLmktcEpVNJo
From a third independent review of the layout merge: - A document adopted from another Mac can place an agent this app's manifest does not list yet (that Mac saw the spawn first). The reconciler dropped such tabs as dead, collapsing the very split this branch keeps. Agents an adopted document names are now treated as live until the next manifest arrives; a current manifest that lacks them still removes their tabs. - A workspace closed here could come back: the other Mac's untouched copy was compared with the base as reconciled with this app's live agents, which had already lost the killed agent, so it looked changed there. Their changes are now measured against the base document itself; only ours are measured against the reconciled base. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_013bBnogGfBnCLmktcEpVNJo
From a fourth independent review: this app publishes its layout with the agents an adopted document placed before the manifest lists them, but the merge base was reconciled without them. A workspace holding such an agent then looked edited here, so a second split from the other Mac (for its next spawn) was overridden and written back, putting that spawn in its own row again. The base is now reconciled with the same agents. Unconfirmed agents are also taken from the merged layout rather than the other Mac's document, so an agent this app's version of a workspace does not hold is not given a stray row of its own. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_013bBnogGfBnCLmktcEpVNJo
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: de78f076c6
ℹ️ 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".
| let restored = try loadWorkspaces(projectRoot) | ||
| workspacesByProject[projectRoot] = restored | ||
| loadedProjects.insert(projectRoot) | ||
| localLayoutStamps[projectRoot] = stamp |
There was a problem hiding this comment.
When a project is loaded before .pu/workspaces.json exists, stamp is nil, and assigning it through this dictionary subscript removes the key rather than recording that the file was known to be missing. Consequently adoptLocalLayoutIfChanged cannot detect the first layout subsequently created over SSH, and saveLocal falls back to that unseen file's current timestamp and overwrites it instead of reporting a conflict. This loses the other Mac's layout when it creates the initial document before this Mac's first successful save; represent the known-missing state explicitly or track whether a stamp was initialized separately.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Thanks for checking this, but the key isn't removed. stamp has static type Int64?. Assigning it through the subscript of a [String: Int64?] (whose setter takes Int64??) wraps it as .some(nil) and stores a nil value. Only the literal nil removes the key. I checked with:
var stamps: [String: Int64?] = [:]
let stamp: Int64? = nil
stamps["p"] = stamp // keys.contains("p") == true, and `if let known = stamps["p"]` binds known == nil
stamps["p"] = nil // keys.contains("p") == falseThe exact scenario you describe is covered by WorkspaceRegistryTests.givenAnotherMacSplitAPaneShouldPlaceItsNewAgentInThatPane. It loads the project with no workspaces.json, has another Mac create it, then reconciles. The test expects this Mac to adopt that file, not overwrite it, and it passes (131/131 in apps/purepoint-macos/verification/check-point-guard-workspace.py).
The subscript form clearly reads as a removal, though, so in f52bb4d I changed it to localLayoutStamps.updateValue(stamp, forKey: projectRoot) with a comment, which says the intent outright. I'm leaving this thread open for you to confirm.
Assigning a nil Int64? through the subscript of an [String: Int64?] already stores a nil value (only a literal nil removes the key), but that reads as the removal a reviewer took it for. updateValue states the intent. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_013bBnogGfBnCLmktcEpVNJo
Conflict: crates/pu-engine/src/ipc_server/tests.rs. Main fixed the same flaky invalid-UTF-8 stream test by sending both lines in one write; took main's version. The isolated pane/registry package (check-point-guard-workspace.py) compiles main's new project-subscription code with strict concurrency and warnings as errors. To keep it green: - AppState.followLocalProjects captures self weakly in the outer Task too (the inner [weak self] was overridden by an implicit strong capture). - The DaemonProjectsSubscription stub's callback is @sendable. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_013bBnogGfBnCLmktcEpVNJo
Summary
Splitting a pane in a project on another machine (over SSH) and spawning an agent into it gave the agent a new sidebar row instead of the split, whenever PurePoint was also open on that machine.
Cause: two apps write the project's
.pu/workspaces.json:Fix:
Request::Spawnhas an optionalplacement({workspace, surface}). The daemon only records it on the agent'sAgentEntryandAgentStatusReport.WorkspaceReconcilerplaces an agent no tab holds yet into the empty tab it was spawned for. If that tab is gone, already holds an agent, or is in a different worktree, the agent gets its own workspace as before. Point Guard workspace IDs include each app's own project key, so placements are re-keyed the same way persistence re-keys the workspace.workspaces.jsonand re-checks it before reconciling, so it sees the client's split before the agent shows up.WorkspaceLayoutMerge) works per workspace against the document this app last read or wrote, reconciled with the current agents. That way grouping the reconciler derives from the manifest, which comes out the same on both Macs, never counts as an edit.adoptRemoteLayoutis renamedadoptExternalLayout. Old daemons and apps ignore the new optional field. The fix takes effect once both Macs and the remote daemon run this build.Known limits
Also in this PR
ipc_server::tests::given_invalid_utf8_in_grid_stream_should_close_the_connectionfailed in CI with a broken pipe.mainhas since fixed it in its own way (one write), and the merge takesmain's version.main, its new project-subscription code tripped the harness's strict concurrency checks.AppState.followLocalProjectsnow capturesselfweakly in its outerTaskas well; the inner[weak self]had no effect. The harness stub forDaemonProjectsSubscriptiontakes a@Sendablecallback.apps/purepoint-macos/verification/check-point-guard-workspace.pyfailed to compile onmaintoo, because signal and transport types were missing from its package. It now compiles, its stubs match the new host and saver code, and it also runsWorkspaceRegistryTests.Verification (head 650bfaa)
cargo fmt --all -- --check,cargo clippy --workspace --all-targets: clean.cargo test --workspace: all pass, including:pu-core;given_spawn_with_placement_should_record_it_on_manifest_and_statusinpu-engine;DEVELOPER_DIR=/Applications/Xcode.app/Contents/Developer python3 apps/purepoint-macos/verification/check-point-guard-workspace.py: 138 tests pass. It runs with strict concurrency and warnings as errors, and compiles the realWorkspace,WorkspaceRegistry,WorkspacePersistence,DaemonProtocolandspawnAgentForSurfacecode. New tests it covers:WorkspaceReconcilerTests;WorkspaceRegistryTestsfor: another Mac's split, adopt rather than overwrite, both changes kept, a new tab surviving when both Macs changed the same workspace (written in one save), a tab for an agent this Mac hasn't seen yet waiting for the manifest (including through a second split from the other Mac), a Point Guard placement from another Mac, recreating a deleted file, and not treating our own saves as another Mac's.MachinesTeststarget. Agents must not runxcodebuild. My changes there are the twosaveRemotefixes with their new tests (givenDocumentDeletedSinceReadSaveShouldWriteItAgainandgivenDocumentCreatedSinceItWasReadAsMissingSaveShouldAdoptIt), the optionalonWrittenonRemoteLayoutSaver, and theadoptExternalLayoutrename. CI'smacOS Build & Testjob covers the full app; owner, please also build in Xcode.PageSpace
project.config.jsonis absent, and neither CLAUDE.md nor AGENTS.md names one), so there are no task, plan or prompt pages to link. This came from the owner's direct bug report: "can't split panes on another machine — it spawns a second agent as a new row instead of a split pane, over SSH".updateValuein f52bb4d). An independent review of this exact head is still pending.Builder: ag-cjxl4rcd
Autonomous agents never merge directly into main/default/protected release targets. Main acceptance requires applicable checks and independent exact-candidate review; production retains human sign-offs.
🤖 Generated with Claude Code
https://claude.ai/code/session_013bBnogGfBnCLmktcEpVNJo