Add workspaces - #68
Conversation
Mira PR WalkthroughThis PR adds a composed workspace system to Patchlane, enabling agents to develop on a complete fork view (all lanes composed together) and then land commits onto a single target lane with exact round-trip tree validation. It introduces the graph LR
cli["src/cli.ts"]
create["src/workspace-create.ts"]
land["src/workspace-land.ts"]
status["src/workspace-status.ts"]
remove["src/workspace-remove.ts"]
composition["src/composition.ts"]
state["src/workspace-state.ts"]
config["src/config.ts"]
git["src/git.ts"]
cli --> create
cli --> land
cli --> status
cli --> remove
create --> composition
create --> state
create --> config
land --> composition
land --> status
land --> state
land --> config
status --> state
status --> git
remove --> status
remove --> state
remove --> git
composition --> config
composition --> git
state --> git
Confidence: 4/5 ◉◉◉◉○ Safe with minor fixes
⏳ Code review in progress…
|
There was a problem hiding this comment.
Mira Review Summary
This is a large, well-structured PR implementing composed workspaces for Patchlane 0.5.3. The architecture is sound: separating composition from sync, pinning lane SHAs at workspace creation time, and enforcing exact tree equality via round-trip validation. Two issues need attention before merge: (1) the CLI workspace land handler does not propagate --origin-remote-name, --upstream-remote-name, or UPSTREAM_REMOTE_URL options to landWorkspace(), making those CLI flags silently ignored; (2) no tests were added alongside the ~1,700 lines of new source code — the workspace creation, landing (including projection, recomposition, and exact tree comparison), state persistence, and cleanup paths all need coverage.
Key Issues
| Issue | Location | |
|---|---|---|
| 🔴 | CLI options --origin-remote-name, --upstream-remote-name, and UPSTREAM_REMOTE_URL are not propagated from the CLI workspace command handler to landWorkspace(), making the advertised CLI flags silently ineffective for the land action. |
src/cli.ts:170 |
| 🔴 | No tests added for ~1,700 lines of new workspace logic including commit replay, exact tree comparison, atomic state persistence, and cleanup error paths. | src/workspace-create.ts:118 |
| try { | ||
| git(['worktree', 'add', '--detach', destination, plan.source.sha], cwd); | ||
| worktreeCreated = true; | ||
| const composition = composeIntoWorktree(plan, { cwd: destination }); | ||
| git(['branch', branch, composition.headSha], cwd); | ||
| branchCreated = true; | ||
| git(['switch', branch], destination); | ||
| git(['config', '--local', `branch.${branch}.patchlane-workspace`, id], cwd); | ||
|
|
||
| const state: WorkspaceState = { | ||
| version: 1, | ||
| id, | ||
| path: destination, | ||
| branch, | ||
| createdAt: new Date().toISOString(), | ||
| configRef, | ||
| originRemoteName, | ||
| upstreamRemoteName, | ||
| source: plan.source, | ||
| targetLane, | ||
| baselineCommit: composition.headSha, | ||
| baselineTree: composition.treeSha, | ||
| laneOrder: plan.lanes.map((lane) => lane.ref), | ||
| laneTips: Object.fromEntries(plan.lanes.map((lane) => [lane.ref, lane.tipSha])), | ||
| laneDiffBases: Object.fromEntries(plan.lanes.map((lane) => [lane.ref, lane.diffBaseSha])), | ||
| landedLaneSha: null, | ||
| }; | ||
| // Validate against the registered worktree before atomically publishing state. | ||
| parseWorkspaceState(state, { cwd, requireRegisteredWorktree: true }); | ||
| writeWorkspaceState(state, cwd); | ||
| return { state, plan }; | ||
| } catch (error) { | ||
| if (worktreeCreated || branchCreated) cleanUpWorkspace(cwd, destination, branch, id); | ||
| throw error; |
There was a problem hiding this comment.
Refactor suggestion
Missing test coverage
This PR adds ~1,700 lines of new source code across 7 new files (src/composition.ts, src/composition-errors.ts, src/git.ts, src/workspace-state.ts, src/workspace-create.ts, src/workspace-status.ts, src/workspace-land.ts, src/workspace-remove.ts) with non-trivial logic including Git worktree management, commit replay, exact tree comparison, lease-based ref updates, and atomic state persistence. The PR contains no corresponding test additions. Critical untested paths include the lane projection and exact-tree-comparison round trip in landWorkspace, the freshness validation in validateLaneFreshness, the atomic state write/read cycle in writeWorkspaceState/parseWorkspaceState, and the cleanup path in createWorkspace when worktree creation succeeds but state registration fails.
Not useful? Reply
@not-adam rejectto dismiss this suggestion.
| if (action === 'land') { | ||
| const result = landWorkspace({ | ||
| lane: args.lane, | ||
| dryRun: args.dryRun === true, | ||
| push: args.push === true, | ||
| }); | ||
| process.stdout.write(`${args.json ? formatWorkspaceLandJson(result) : formatWorkspaceLand(result)}\n`); | ||
| return; |
There was a problem hiding this comment.
Bug
CLI options not propagated to workspace land
The land action handler at line 170-177 passes only lane, dryRun, and push to landWorkspace(). It does not propagate --origin-remote-name, --upstream-remote-name, or UPSTREAM_REMOTE_URL (which create does propagate at lines 148-157). While landWorkspace defaults originRemoteName and upstreamRemoteName from workspace state (lines 333-334 of workspace-land.ts) and upstreamRemoteUrl from process.env, this means the CLI flags are silently ignored for land. Users who need to override these values for the land operation (e.g., when the remote name has changed since workspace creation) will get unexpected behavior with no error.
| if (action === 'land') { | |
| const result = landWorkspace({ | |
| lane: args.lane, | |
| dryRun: args.dryRun === true, | |
| push: args.push === true, | |
| }); | |
| process.stdout.write(`${args.json ? formatWorkspaceLandJson(result) : formatWorkspaceLand(result)}\n`); | |
| return; | |
| if (action === 'land') { | |
| const result = landWorkspace({ | |
| lane: args.lane, | |
| dryRun: args.dryRun === true, | |
| push: args.push === true, | |
| originRemoteName: args.originRemoteName, | |
| upstreamRemoteName: args.upstreamRemoteName, | |
| upstreamRemoteUrl: env('UPSTREAM_REMOTE_URL'), | |
| }); | |
| process.stdout.write(`${args.json ? formatWorkspaceLandJson(result) : formatWorkspaceLand(result)}\n`); | |
| return; | |
| } |
Prompt for AI Agents
In src/cli.ts, in the workspace command action handler under the `action === 'land'` branch (around line 170), add `originRemoteName: args.originRemoteName`, `upstreamRemoteName: args.upstreamRemoteName`, and `upstreamRemoteUrl: env('UPSTREAM_REMOTE_URL')` to the `landWorkspace()` call options object so the CLI flags are propagated to the land function, consistent with how they are passed in the `create` action handler.
Apply this code change:
if (action === 'land') {
const result = landWorkspace({
lane: args.lane,
dryRun: args.dryRun === true,
push: args.push === true,
originRemoteName: args.originRemoteName,
upstreamRemoteName: args.upstreamRemoteName,
upstreamRemoteUrl: env('UPSTREAM_REMOTE_URL'),
});
process.stdout.write(`${args.json ? formatWorkspaceLandJson(result) : formatWorkspaceLand(result)}\n`);
return;
}
Not useful? Reply
@not-adam rejectto dismiss this suggestion.
No description provided.