fix: address LCM/SM review blockers R1, RA, R11, RB#23
Merged
Conversation
3641ebc to
44cc6e9
Compare
Four narrowly-scoped fixes against the LCM + Session Manager lane from an external review of the current backend state. R2 (failed-restore lifecycle stranding) is intentionally deferred to PR #15, which already closes it via the new OnSpawnInitiated path; R3 also stays on that PR. - R1 (BLOCKER): Manager.Spawn never persisted AgentSessionID, so Manager.Restore's hard-required metadata key was always missing and every restore failed. Persist the assembled launch prompt as MetaPrompt at spawn time and add a fresh-launch fallback to Restore that uses Agent.GetLaunchCommand with the seeded prompt when the captured agent session id is absent (the id-capture hook is a separate path that may never have run). Restore still fails fast when neither the id nor a prompt is on hand — there is nothing to relaunch from. - RA (BLOCKER): adapters/workspace/gitworktree/commands.go's worktreeRemoveForceArgs passed --force, which deletes uncommitted agent work. Renamed to worktreeRemoveArgs and dropped --force so the post-prune "still registered" guard in Workspace.Destroy surfaces the refusal to Manager.Cleanup, which routes the session to Skipped instead of destroying in-progress changes. - R11 (SHOULD-FIX): reactions.go's two Notifier.Notify call sites (executeReaction's notify and escalate) built OrchestratorEvent without ProjectID. Captured projectID on the transition (via a store.Get in mutate) and on reactionTracker (so TickEscalations can still populate it on duration-based escalations), and threaded it through executeReaction/sendToAgent/escalate. - RB (SHOULD-FIX): gitworktree.Workspace.managedPath used filepath.Join which cleans .. segments before validateManagedPath ran, so session=\"../other\" stayed inside managedRoot while breaking per-project isolation. validateConfig now rejects path separators and the . / .. components on ProjectID and SessionID at the source. go build ./..., go vet ./..., and go test -race ./... all pass. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
44cc6e9 to
650ecdf
Compare
7 tasks
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
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.
Summary
Four narrowly-scoped fixes against the LCM + Session Manager lane, from an external review of the current backend state.
Out of scope for this PR:
fix/lcm-writer-contract), which introducesLifecycleManager.OnSpawnInitiatedand re-routes Manager.Restore through the LCM. Implementing R2 here would duplicate logic and conflict.Fixes
R1 (BLOCKER) — Spawn never persisted AgentSessionID so Restore always failed
Manager.SpawnbuiltSpawnOutcomewith an emptyAgentSessionID, soMetaAgentSessionIDwas never written, andManager.Restore's metadata check hard-failed every time.Picked the fresh-launch fallback path (the agent's id-capture is a separate hook that may never have run, so there is nowhere to plumb an id-bearing return today):
Prompttoports.SpawnOutcomeandMetaPromptto the lifecycle metadata key set.Manager.Spawnnow passes the assembled prompt;spawnMetadatapersists it.Manager.Restorefalls back toAgent.GetLaunchCommandwith the seeded prompt when no agent session id is on hand. Restore still fails fast when neither is present — there's nothing to relaunch from.RA (BLOCKER) —
git worktree remove --forcedeleted dirty worktreesadapters/workspace/gitworktree/commands.gopassed--force, destroying uncommitted agent work and violating the worktree-remove safety invariant.worktreeRemoveForceArgs→worktreeRemoveArgsand dropped--force. The existing post-prune "still registered" guard inWorkspace.Destroynow surfaces the refusal toManager.Cleanup, which routes the session toSkipped.--force/-fare NEVER passed.R11 (SHOULD-FIX) —
OrchestratorEvent.ProjectIDnever populatedBoth Notify call sites in
reactions.gobuilt events withoutProjectIDeven though the struct supports it.projectIDon thetransitionstruct (read from the record already loaded at the top ofmutate) and onreactionTracker(soTickEscalations, which has no transition on hand, can still populate it).react → executeReaction → sendToAgent → escalateand set it on both Notify call sites.RB (SHOULD-FIX) — session/project ids could escape the project root
Workspace.managedPathusedfilepath.Joinwhich cleans..segments beforevalidateManagedPathran, sosession="../other"stayed insidemanagedRootwhile breaking per-project isolation.validateConfignow rejects path separators (/,\) and the./..components onProjectIDandSessionIDat the source.Test plan
go build ./...passesgo vet ./...passesgo test -race ./...passes (238 tests across 12 packages)TestRestore_NoAgentSessionID_FreshLaunchFallback— restore succeeds via fresh launch when no id was capturedTestRestore_NoIDAndNoPrompt_Errors— restore still fails fast with no id and no promptTestReaction_ProjectIDOnNotifyAndEscalateEvents(notify path, numeric-escalate path, TickEscalations path)TestValidateConfigRejectsPathEscapingIDs+TestValidateConfigAcceptsBenignIDsTestDestroyRefusesStillRegisteredPathAndPreservesDirectoryextended to assert no--forceflagTestSpawn_HappyPathupdated to assertMetaPromptis persisted🤖 Generated with Claude Code