Repository navigation
fix(server): report not found when removing an unknown shell - #56809
reynold-hu wants to merge 1 commit into
Conversation
|
The following comment was made by an LLM, it may be inaccurate: |
![opencode-agent[bot]](https://avatars.githubusercontent.com/in/1549082?s=60&v=4){kind=small}
There was a problem hiding this comment.
This does what it says: the new test fails on the base (DELETE returns 204) and passes here, and typecheck passes for packages/server and packages/protocol. But I don't think it should go in as it is.
Public API: DELETE /api/shell/{id} on an unknown or already removed id changes from 204 to 404 with ShellNotFoundError, and that error is added to the endpoint's 404 response in the OpenAPI spec. This is not backwards compatible. Callers that rely on removing a shell twice safely will now get an error.
Concerns:
- The 204 was an intentional API decision.
b1f6d21ef("refactor(protocol): refine vcs and shell APIs") replaced this same 404 mapping with a no-op, andV2_HTTP_API_AUDIT.md(entry 135) says: "Shell deletion is idempotent and returns204when already absent." A change to that contract needs a maintainer's agreement on #56804 first, and the audit entry should change with it. - It doesn't clear the stuck item from #56804. The ghost "1 running" entry comes from client and session state that never gets settled. With this PR the app's stop action shows a "Request failed: Shell command not found" toast, but the item stays. Settling that state (the follow-up you mention) is what fixes the user-facing problem. Once it lands, the stop action no longer needs a 404.
- New error cases for existing callers. A shell that exits just before the user clicks stop now gives an error toast in the app. The TUI kill action (
packages/tui/src/routes/session/composer/shell-tab.tsx) callsvoid client.api.shell.remove(...)without a.catch, so a 404 there becomes an unhandled promise rejection. If the API does change, these callers should treat "not found" as "already stopped".
Related: #56804
| Effect.fn(function* (ctx) { | ||
| const shell = yield* Shell.Service | ||
| yield* shell.remove(ctx.params.id).pipe(Effect.catchTag("Shell.NotFoundError", () => Effect.void)) | ||
| yield* shell.remove(ctx.params.id).pipe( |
There was a problem hiding this comment.
This reverses b1f6d21ef, which removed this mapping on purpose: V2_HTTP_API_AUDIT.md entry 135 says shell deletion returns 204 when the shell is already absent. If the contract should change, please get a maintainer's agreement first, update the audit entry, and make the TUI and app callers treat a 404 as "already stopped". As it stands, the app shows an error toast and the stale item stays.
|
Closing per the review: the 204 on an absent shell is an intentional contract (V2_HTTP_API_AUDIT entry 135, b1f6d21), and this change alone would not clear the stuck entry. The user-facing fix belongs in settling session/timeline state when a Location is torn down, which is the remaining part of #56804. Thanks for the detailed check. |
Issue for this PR
Part of #56804. This fixes the API contract that makes a stale shell undetectable; settling running tool rows when a Location is torn down is a separate change.
Type of change
What does this PR do?
DELETE /api/shell/:idsilently succeeded for an unknown id: the handler caughtShell.NotFoundErrorand returned204, whileGET /api/shell/:idalready returns404. The silent success is why a client cannot tell that a shell vanished (for example after its Location was evicted), so a stale "running" item cannot be detected or cleared through this route.The handler now maps the missing shell to the declared
ShellNotFoundError(404), and the endpoint declares that error so the response is part of the public API instead of surfacing as an internal error.How did you verify your code works?
packages/server/test/shell.test.ts:DELETEof an unknownsh_…id returns404with_tag: "ShellNotFoundError", whileGETkeeps returning404. On the parent commit the same request returns204.packages/serversuite: 79 pass, 6 skip, 0 fail.check:generatedpasses forpackages/protocolandservices/www.packages/serverandpackages/protocoltypecheck pass.Screenshots / recordings
No UI change.
Checklist
Added by the opencode bot (AI-generated).
Related: #56804
Public API: DELETE /api/shell/{id} for an unknown id returns 404 ShellNotFoundError instead of 204 (not backwards compatible)