fix(server): surface the declared 404 when deleting a missing message - #1109
Conversation
The DELETE /session/:sessionID/message/:messageID route declares a 404, but Session.removeMessage was a silent no-op: it fired MessageV2.Event.Removed and returned the id without checking existence, so deleting a message that does not exist returned 200. The declared 404 was a phantom. Make removeMessage verify the message exists first via MessageV2.get (which throws NotFoundError for a missing row, mapped to 404 by ErrorMiddleware). The existence check lives in the service rather than the route because removeMessage is the "delete a message" domain operation and has exactly one caller (the route, no top-level wrapper), so the blast radius is contained. assertNotBusy stays in the route, so a busy session still wins with 409 and only the idle+missing case flips from a silent 200 to 404. No frontend impact: the desktop app does not call this endpoint directly (it reacts to the message.removed SSE event). The only behavior change is that a double-delete (or deleting an unknown id) now returns the already-declared 404. Refs #936
|
Warning You have reached your daily quota limit. Please wait up to 24 hours and I will start processing your requests again! |
|
Warning Review limit reached
More reviews will be available in 56 minutes and 24 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
1 participant
Summary
Part of the staged Effect error-contract migration (#936): turn a silent 200
into the route's already-declared 404.
DELETE /session/:sessionID/message/:messageID(session.deleteMessage)declares
errors(400, 404, 409), butSession.removeMessagewas a silentno-op: it fired
MessageV2.Event.Removedand returned the id without checkingexistence, so deleting a message that does not exist returned
200. Thedeclared 404 was a phantom that never fired.
Change
Session.removeMessagenow verifies the message exists first viaMessageV2.get(the same throwing lookup the siblingGET .../message/:messageIDroute relies on for its 404).
MessageV2.getthrowsNotFoundErrorfor amissing row, which
ErrorMiddlewaremaps to 404 (and does not error-log).Why in the service, not the route:
removeMessageis the "delete a message"domain operation and has exactly one caller — the route handler (no top-level
wrapper) — so putting the existence check there makes the operation correct with
a contained blast radius.
assertNotBusystays in the route, so the ordering ispreserved: a busy session still wins with 409, and only the idle + missing
case flips from a silent 200 to 404.
Behavior change & de-risk
The only change is that deleting a message that does not exist (e.g. a
double-delete, or an unknown id) now returns the already-declared 404 instead
of a misleading 200. The desktop app does not call this endpoint directly —
it reacts to the
message.removedSSE event — so there is no frontend impact.Verified there are no internal callers of
removeMessagethat relied on thesilent no-op (the full session suite stays green).
Scope (deliberately one route)
This is the first, smallest slice of the silent-200 cleanup. The sibling
candidates are intentionally not bundled, each for a concrete reason:
part.delete—removePartis also reused internally byprocessor.ts, so its fix must be route-level, not service-level; different shape.permission.reply— separate subsystem with CLI/ACP/test callers; larger surface.experimental.console.switchOrg—account.useis a blind upsert and the account errors are EffectTaggedErrorClass(not the mappedNamedError); needs new validation + a mapped error type (design).permission.respond— alreadydeprecatedand fire-and-forget.Verification
bun test test/server/session-messages.test.ts— 12 pass (new test covers both the 200 success delete and the 404 on a missing id)bun test test/server/— 332 passbun test test/session/— 790 pass / 0 fail (confirms no internal consumer relied on the silent no-op)bun run typecheck— cleanDeferred (known, tracked)
The checked-in
packages/sdk/openapi.jsonsnapshot /src/v2/gentypes areregenerated in a single batched resync PR at the end of the #936 series, not
per-slice. No functional impact (the app reacts to SSE, not this response).
Refs #936