fix(session-manager): refuse to restore sessions with merged/closed PRs
When a session's PR has reached a terminal state (merged or closed) and
the session is later restored, two things conspire to attach the session
to an unrelated PR:
1. The original head branch is typically deleted on merge, so on
workspace restore `session.branch` may drift to the project default
branch (or be rewritten by an agent-side `git checkout main`).
2. With branch == default branch, downstream `detectPR` matches
whatever PR happens to share that head ref name (including fork PRs).
Fix 1 (scm-github) and Fix 2 (lifecycle) defend in depth, but the cleanest
guarantee is to refuse the restore up-front: a session whose PR has
already merged/closed has no remaining work to do. If the user wants
follow-up work, they should spawn a new session.
The previous behavior — silently clearing terminal `lifecycle.pr.*`
fields on restore and resetting `session.state` to "working" — is removed
because it's now unreachable: the new check at step 3a throws before
we get there.
Invariants preserved:
- Workers with `pr.state` of "open"/"none" still restore normally.
- `isRestorable` semantics unchanged for terminated/done/runtime-lost.
- Errors surface via `SessionNotRestorableError`, the same channel used
by other restore-blockers (workspace missing, OpenCode mapping, etc).
Refs #1724.
This commit is contained in:
parent
5323103f59
commit
2adf6e08cd
|
|
@ -197,12 +197,13 @@ describe("restore", () => {
|
|||
expect(mockRuntime.create).toHaveBeenCalled();
|
||||
});
|
||||
|
||||
it("allows restoring merged sessions", async () => {
|
||||
it("refuses to restore sessions whose PR has merged (would silently re-spawn against the default branch)", async () => {
|
||||
const ws = "/tmp/mock-ws/app-1";
|
||||
writeMetadata(sessionsDir, "app-1", {
|
||||
worktree: ws,
|
||||
branch: "main",
|
||||
status: "merged",
|
||||
pr: "https://github.com/acme/repo/pull/1723",
|
||||
project: "my-app",
|
||||
runtimeHandle: makeHandle("rt-old"),
|
||||
});
|
||||
|
|
@ -222,8 +223,7 @@ describe("restore", () => {
|
|||
};
|
||||
|
||||
const sm = createSessionManager({ config, registry });
|
||||
const restored = await sm.restore("app-1");
|
||||
expect(restored.id).toBe("app-1");
|
||||
await expect(sm.restore("app-1")).rejects.toThrow(/is merged/);
|
||||
});
|
||||
|
||||
it("throws SessionNotRestorableError for working sessions", async () => {
|
||||
|
|
|
|||
|
|
@ -2801,6 +2801,23 @@ export function createSessionManager(deps: SessionManagerDeps): OpenCodeSessionM
|
|||
throw new SessionNotRestorableError(sessionId, reason);
|
||||
}
|
||||
|
||||
// 3a. Refuse to silently revive a session whose PR has reached a terminal
|
||||
// state. The original branch has typically been deleted on merge, so
|
||||
// `session.branch` may have drifted to the project default branch
|
||||
// between the kill and the restore. Re-spawning would associate the
|
||||
// session with whatever PR `detectPR` finds on that branch — including
|
||||
// unrelated fork PRs (see #1724). If the user wants a fresh session
|
||||
// in this workspace, they should spawn a new one.
|
||||
const prState = session.lifecycle.pr.state;
|
||||
if (prState === "merged" || prState === "closed") {
|
||||
const prRef = session.lifecycle.pr.url ?? session.pr?.url ?? raw["pr"] ?? "(unknown)";
|
||||
throw new SessionNotRestorableError(
|
||||
sessionId,
|
||||
`PR ${prRef} is ${prState}; the session's work is complete. ` +
|
||||
`Spawn a new session for follow-up work instead of restoring.`,
|
||||
);
|
||||
}
|
||||
|
||||
// 4. Validate required plugins (plugins already resolved above for enrichment)
|
||||
if (!plugins.runtime) {
|
||||
throw new Error(`Runtime plugin '${project.runtime ?? config.defaults.runtime}' not found`);
|
||||
|
|
@ -2967,16 +2984,8 @@ export function createSessionManager(deps: SessionManagerDeps): OpenCodeSessionM
|
|||
restoredLifecycle.runtime.handle = handle;
|
||||
restoredLifecycle.runtime.lastObservedAt = now;
|
||||
|
||||
// Reset terminal PR state so the lifecycle manager doesn't immediately
|
||||
// re-terminate the session. The old PR is done — if the agent creates
|
||||
// a new one, PR auto-detect will pick it up.
|
||||
if (restoredLifecycle.pr.state === "merged" || restoredLifecycle.pr.state === "closed") {
|
||||
restoredLifecycle.pr.state = "none";
|
||||
restoredLifecycle.pr.reason = "cleared_on_restore";
|
||||
restoredLifecycle.pr.number = null;
|
||||
restoredLifecycle.pr.url = null;
|
||||
restoredLifecycle.pr.lastObservedAt = null;
|
||||
}
|
||||
// Terminal PR state (merged/closed) is rejected up-front in step 3a, so
|
||||
// by here the lifecycle.pr is non-terminal and is preserved as-is.
|
||||
|
||||
updateMetadata(sessionsDir, sessionId, {
|
||||
...buildLifecycleMetadataPatch(restoredLifecycle),
|
||||
|
|
|
|||
Loading…
Reference in New Issue