feat: implement issue spawn rejection for closed or cancelled states#2064
feat: implement issue spawn rejection for closed or cancelled states#2064nikhilachale wants to merge 2 commits into
Conversation
Added the IssueNotSpawnableError class to handle cases where spawning a session is attempted for issues that are closed or cancelled. Updated session manager logic to reject spawning in these scenarios and log appropriate activity events. Enhanced API error handling to return a 409 status code for these specific errors. Added tests to verify the new behavior.
Greptile SummaryThis PR adds a spawn guard that rejects new worker sessions for tracker issues already in a
Confidence Score: 5/5Safe to merge — the guard fires before any resources are created, the error is correctly typed and caught at the API boundary, and existing spawn paths for valid issue states are unaffected. The change is narrowly scoped: it adds a single pre-flight check that short-circuits before touching the workspace or runtime, wires a new typed error to a 409 response, and includes tests for the rejection cases. No existing behaviour is altered for spawnable issues. No files require special attention; the test file is missing coverage for the "open" spawnable state but this does not affect correctness of the guard itself.
|
| Filename | Overview |
|---|---|
| packages/core/src/types.ts | Adds isSpawnableIssueState() guard and IssueNotSpawnableError class; both are well-typed and consistent with the existing Issue["state"] union. |
| packages/core/src/session-manager.ts | Adds pre-resource-creation spawn guard that fetches the issue state and throws IssueNotSpawnableError for non-spawnable states; logic is correct and well-placed after the existing issue-fetch try/catch. |
| packages/web/src/app/api/spawn/route.ts | Maps IssueNotSpawnableError to HTTP 409 and enriches recordApiObservation with issue state metadata; straightforward and correct. |
| packages/core/src/tests/session-manager/spawn.test.ts | Adds three new spawn-guard tests covering closed, cancelled, and in_progress; missing a test for the "open" spawnable state. |
Sequence Diagram
sequenceDiagram
participant Client
participant SpawnRoute as /api/spawn
participant SessionManager
participant Tracker
Client->>SpawnRoute: "POST /api/spawn { projectId, issueId }"
SpawnRoute->>SessionManager: "spawn({ projectId, issueId })"
SessionManager->>Tracker: getIssue(issueId, project)
Tracker-->>SessionManager: "Issue { state }"
alt state is closed or cancelled
SessionManager->>SessionManager: recordActivityEvent(session.spawn_rejected)
SessionManager-->>SpawnRoute: throw IssueNotSpawnableError
SpawnRoute->>SpawnRoute: recordApiObservation(failure, 409)
SpawnRoute-->>Client: 409 Conflict
else state is open or in_progress
SessionManager->>SessionManager: create workspace + runtime
SessionManager-->>SpawnRoute: Session
SpawnRoute-->>Client: 201 Created
end
Prompt To Fix All With AI
Fix the following 2 code review issues. Work through them one at a time, proposing concise fixes.
---
### Issue 1 of 2
packages/core/src/__tests__/session-manager/spawn.test.ts:1151-1163
The test suite covers `closed`, `cancelled`, and `in_progress` but skips the `"open"` state, which is the other explicitly spawnable value in `isSpawnableIssueState`. Without it, a regression that accidentally blocks `"open"` issues would go undetected by these tests.
```suggestion
it("allows spawn when tracker issue is in_progress", async () => {
const mockTracker = baseMockTracker({ id: "INT-50", state: "in_progress" });
const sm = createSessionManager({
config,
registry: registryWithTracker(mockTracker),
});
const session = await sm.spawn({ projectId: "my-app", issueId: "INT-50" });
expect(session.issueId).toBe("INT-50");
expect(mockWorkspace.create).toHaveBeenCalled();
expect(mockRuntime.create).toHaveBeenCalled();
});
it("allows spawn when tracker issue is open", async () => {
const mockTracker = baseMockTracker({ id: "INT-51", state: "open" });
const sm = createSessionManager({
config,
registry: registryWithTracker(mockTracker),
});
const session = await sm.spawn({ projectId: "my-app", issueId: "INT-51" });
expect(session.issueId).toBe("INT-51");
expect(mockWorkspace.create).toHaveBeenCalled();
expect(mockRuntime.create).toHaveBeenCalled();
});
```
### Issue 2 of 2
packages/core/src/types.ts:2008-2010
The error message hardcodes `"closed/cancelled"`, so if a new non-spawnable state is ever added to `Issue["state"]` and `isSpawnableIssueState` is updated, the static text here will silently misrepresent the actual rejection reason. Interpolating `state` directly keeps the message accurate regardless of how many non-spawnable states exist.
```suggestion
super(
`Issue ${issueId} is ${state}. Cannot spawn a session for a ${state} issue.`,
);
```
Reviews (2): Last reviewed commit: "refactor: enhance issue state handling i..." | Re-trigger Greptile
Updated the session manager to utilize the new isSpawnableIssueState function for improved state validation. Modified the IssueNotSpawnableError class to accept a broader range of issue states, ensuring accurate error messaging. Removed redundant error handling in the API spawn route for cleaner code.
Summary
This PR prevents AO from spawning new worker sessions for tracker issues that are already
closedorcancelled.Previously, spawn requests could continue even when the linked tracker issue was no longer actionable. The session manager now checks
the resolved issue state before creating a workspace or runtime, records a rejection activity event, and throws a typed
IssueNotSpawnableError.Changes
IssueNotSpawnableErrorfor closed/cancelled issue spawn attemptsisSpawnableIssueState()helper for spawn-eligible issue statesSessionManager.spawn()to reject closed or cancelled tracker issues before workspace/runtime creation/api/spawnto return409 Conflictfor non-spawnable issues instead of500closedissuescancelledissuesin_progressissuesTesting
packages/core/src/__tests__/session-manager/spawn.test.ts