fix(server): declare tool/respond route-local failures, harden errors() helper - #1104
Merged
Conversation
|
Warning You have reached your daily quota limit. Please wait up to 24 hours and I will start processing your requests again! |
Contributor
|
Warning Review limit reached
More reviews will be available in 9 minutes and 20 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 (4)
✨ 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 |
Astro-Han
added
bug
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
…() helper
The errors() response helper silently dropped any status code not present
in its ERRORS map: it built `{ code: ERRORS[code] }` with ERRORS[code]
undefined, and JSON.stringify omits undefined, so the declaration vanished
from the generated OpenAPI. POST /session/:sessionID/tool/respond used
errors(400, 404, 409, 422), so its 409 and 422 were no-ops, and its 404
was declared with the shared NotFoundError schema even though the handler
returns route-local bodies ({ error } / { error, details }).
- Harden errors(): tighten the signature to (keyof typeof ERRORS)[] so an
unregistered code is a typecheck error at the call site, and throw at
runtime as a defensive guard for casted/JS callers, instead of emitting
an empty declaration.
- Fix tool/respond: keep errors(400) for the zod-validator bad-request,
and declare 404 / 409 / 422 inline with a route-local ToolRespondFailure
schema ({ error: string, details?: unknown }) that matches what the
handler actually returns. This also corrects the pre-existing 404
schema mismatch.
Runtime behavior is unchanged; this only corrects the published contract.
Per the line's strategy the stale hand-maintained packages/sdk/openapi.json
snapshot and generated SDK are not regenerated here.
Verify: bun test test/server/error.test.ts test/server/tool-respond-route.test.ts
(new cases assert errors() throws on an unregistered code, and that the
tool/respond OpenAPI declares 404/409/422 as ToolRespondFailure rather
than NotFoundError); tsgo --noEmit clean; route-inventory harness green.
Astro-Han
force-pushed
the
claude/i936-toolrespond-contract
branch
from
June 3, 2026 03:38
64a4cd9 to
d3ba4e2
Compare
Astro-Han
added a commit
that referenced
this pull request
Jun 3, 2026
…1112) * chore(sdk): regenerate v2 SDK types for the migrated error contracts Regenerate packages/sdk/js/src/v2/gen from the current server contract (`cd packages/sdk/js && bun run build`). This closes the SDK-lag P2 that the #936 contract slices kept surfacing: the generated v2 SDK error types now match the live server's declared 4xx responses. Picked up: - SessionForkErrors (400, 404) and the fork method's error generic — #1101. - SessionToolRespondErrors now declares the inline 404/409/422 bodies ({ error, details? }) instead of only 400/404 — #1104. - 409 Conflict (UnknownError) on the nine busy-guarded session routes (turnChangeUndo/Redo, turnChangesAggregateUndo/Redo, summarize, deleteMessage, shell, revert, unrevert) — #1106. The regen also absorbs minor pre-existing ordering/description drift from earlier merges (an EventSessionCompacted union reordering, a VCS diffRaw doc string), which is just the canonical generator output catching up. Scope note: this regenerates only the consumed SDK artifact (src/v2/gen). The checked-in packages/sdk/openapi.json reporting snapshot is intentionally left to a separate change — it is read only by the route-inventory harness, whose tests are built to detect (and tolerate) that snapshot's lag, so resyncing it there would fight that harness by design. All changes are additive (new error types on existing methods), so SDK consumers are unaffected. `bun tsc` (run by the SDK build) and the route-inventory harness tests pass. Refs #936 * chore(ci): route packages/sdk changes to the harness label This SDK regen is the first packages/sdk-only PR, which surfaced that the labeler has no routing glob for packages/sdk. With sync-labels enabled, a SDK-only PR therefore gets no primary routing label (and any manually added one is stripped), so the pr-triage label policy ("at least one routing label") cannot be satisfied. Route packages/sdk/** to the harness area: the SDK is the opencode server's generated client, part of the same contract surface as packages/opencode. Lock the routing in the pr-triage workflow test. Refs #936
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
1 participant
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.
Root cause
The
errors()response helper silently dropped any status code not present in itsERRORSmap. It built{ [code]: ERRORS[code] }withERRORS[code]undefined, andJSON.stringifyomitsundefined, so the declaration vanished from the generated OpenAPI.POST /session/:sessionID/tool/responddeclarederrors(400, 404, 409, 422), so:NotFoundErrorschema even though the handler returns route-local bodies ({ error: "no_pending_tool_call" },{ error: "already_resolved" },{ error, details }).Change
errors(): tighten the signature to(keyof typeof ERRORS)[]so an unregistered status is a typecheck error at the call site, andthrowat runtime as a defensive guard for casted/JS callers — instead of emitting an empty declaration. AsERRORSgrows (e.g. when 409 is added for busy conflicts), the accepted set grows with it.errors(400)for the zod-validator bad-request, and declare404/409/422inline with a route-localToolRespondFailureschema ({ error: string, details?: unknown }) that matches what the handler actually returns. This also corrects the pre-existing 404 schema mismatch.Runtime behavior is unchanged — this only corrects the published contract. The handler already returned these bodies and statuses (covered by the existing
tool-respond-routeruntime tests); now the contract documents them accurately.On the SDK snapshot
Per this line's strategy, the hand-maintained
packages/sdk/openapi.jsonsnapshot and generated SDK are not regenerated here (it is ~24 operations behind the live code and has no committed generator; batch-resynced separately). This PR changes only the live served OpenAPI source.Verification
bun test test/server/error.test.ts test/server/tool-respond-route.test.ts— new cases asserterrors()throws on an unregistered code, and that the tool/respond OpenAPI declares 404/409/422 asToolRespondFailurerather thanNotFoundError; 10/10 pass.tsgo --noEmitclean (the tightenederrors()signature type-checks all ~70 existing call sites).Context
#936 Effect error-contract line, contract-completion phase. Enabler/cleanup that unblocks the upcoming busy-conflict 409 slice (which will register
409inERRORS) without that change retroactively mis-declaring tool/respond's 409. Code-only slice, no SDK regen.