refactor(server): effectify external result route#1034
Conversation
Astro-Han
added
P2
|
Warning Review limit reached
More reviews will be available in 55 minutes and 6 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}
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request refactors the external-result routes to retrieve sessions via AppRuntime and Session.Service instead of direct static calls, updates MessageV2.get calls to be synchronous, and adds comprehensive unit tests for these routes. The feedback suggests simplifying the session lookup in external-result.ts by using Session.Service.use instead of Effect.gen, which would also allow removing the unused Effect import.
Migrate the POST /permission/:requestID/reply route handler from a direct `await Permission.reply(...)` call to the Effect runtime path, resolving `Permission.Service` through `AppRuntime.runPromise`. Part of #936 Hono-to-Effect route migration; no route, schema, or response-shape change (still returns `c.json(true)`). Change boundary: - packages/opencode/src/server/instance/permission.ts: POST reply handler body only. Route registration, param/json validators, and the `true` JSON response are unchanged. - packages/opencode/test/server/permission-routes.test.ts: new focused test driving a pending permission ask -> reply through the route runtime and asserting 200 + true. Verification (isolated worktree, rebased onto origin/dev including #1033/#1034/#1035): - bun install --frozen-lockfile - bun test test/server/permission-routes.test.ts -> 1 pass - bun run typecheck (opencode) -> clean - codex review --base origin/dev -> no actionable issues Review follow-up: Gemini flagged that the test's `onPending: () => Deferred.succeed(pending, undefined)` returns `Effect<boolean>` against an `Effect<void>` parameter and suggested `Effect.asVoid`. Resolved as a non-issue: Effect's success channel is covariant (encoded as a function returning A), so TypeScript's void-return-position rule makes `Effect<boolean>` assignable to `Effect<void>`; tsgo confirms (typecheck clean), and the boolean is discarded by Permission.ask anyway. Thread resolved; no change. github-actions triage suggested P2 (already labeled). Linked: #936 Residual risk: low. Single fire-and-forget reply route, response unchanged, covered by the new route test.
1 participant
Summary
Migrate
GET /external-resultto use the existingAppRuntime.runPromise(Effect.gen(...))route runtime pattern for session lookup.Why
This continues the #936 ordinary JSON route migration for a PawWork-owned hydration route while preserving the existing stale-entry skip behavior and pending tool-call response shape.
Related Issue
Refs #936
Human Review Status
Pending
Review Focus
Please check that stale session/message entries are still skipped and that successful pending external-result hydration returns the same session/message/part trio.
Risk Notes
This route has race-sensitive recovery behavior. The focused test covers both stale pending entries and a live pending external-result tool part.
Skipped conditional checklist items:
How To Verify
Screenshots or Recordings
Not applicable; no visible UI changes.
Checklist
bug,enhancement,task,documentation. Type labels are author-added; the labeler bot does NOT assign them. Add the label in the GitHub UI, then tick this.app,ui,platform,harness,ci. The labeler bot assigns these on PR open based on changed paths. Confirm the bot's choice (or override if wrong), then tick this.P0,P1,P2,P3. The priority-triage bot suggests one on PR open. Confirm or override, then tick this.Pending,Approved by @<reviewer>, orNot required: <reason>(default isPending; "not required" is restricted to bot-authored low-risk PRs).dev, and my PR title and commit messages use Conventional Commits in English.