refactor(opencode): retire session Hono route source - #1427
Merged
Conversation
Astro-Han
added
P2
Contributor
📝 WalkthroughWalkthroughThe PR retires the legacy Hono-based ChangesLegacy Hono SessionRoutes Retirement
Home Composer E2E Submit Button Assertions
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested labels
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
ESLint install failed. For unrecoverable errors, disable the tool in CodeRabbit configuration. 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}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
packages/opencode/test/server/permission-routes.test.ts (1)
50-65: 💤 Low valueConsider extracting shared helper logic.
The
requestSessionHttpApihelper duplicates most of the logic fromrequestPermissionHttpApi(lines 32-48). Both follow the identical pattern: buildHttpRouter, provide handlers and platform layers, construct and route the request.While acceptable for this migration PR, consolidating into a parameterized factory would reduce duplication:
function requestHttpApi<A>(api: HttpApi.HttpApi<any, any>, handlers: Layer.Layer<any, any, any>) { return (path: string, init?: RequestInit) => AppRuntime.runPromise( Effect.scoped( Effect.gen(function* () { const router = yield* HttpRouter.toHttpEffect( HttpApiBuilder.layer(api).pipe( Layer.provide(handlers), Layer.provide(Layer.mergeAll(NodeFileSystem.layer, NodeHttpPlatform.layer, NodePath.layer, Etag.layer)), ), ) const request = HttpServerRequest.fromWeb(new Request(`http://localhost${path}`, init)) const response = yield* router.pipe(Effect.provideService(HttpServerRequest.HttpServerRequest, request), Effect.orDie) return HttpServerResponse.toWeb(response) }), ) as Effect.Effect<Response>, ) }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/opencode/test/server/permission-routes.test.ts` around lines 50 - 65, The requestSessionHttpApi and requestPermissionHttpApi functions contain duplicated logic that differs only in the api and handlers parameters. Extract this shared logic into a single parameterized factory function that accepts the HttpApi and handlers as parameters, then have both requestSessionHttpApi and requestPermissionHttpApi call this factory with their respective api and handlers arguments to eliminate the duplication.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@packages/opencode/test/server/permission-routes.test.ts`:
- Around line 50-65: The requestSessionHttpApi and requestPermissionHttpApi
functions contain duplicated logic that differs only in the api and handlers
parameters. Extract this shared logic into a single parameterized factory
function that accepts the HttpApi and handlers as parameters, then have both
requestSessionHttpApi and requestPermissionHttpApi call this factory with their
respective api and handlers arguments to eliminate the duplication.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: f7a51567-0264-4351-aca2-f214b9379c13
📒 Files selected for processing (10)
packages/app/e2e/app/home.spec.tspackages/opencode/script/route-inventory.tspackages/opencode/src/server/instance/index.tspackages/opencode/src/server/instance/session.tspackages/opencode/src/server/routes/instance/session.tspackages/opencode/test/server/permission-routes.test.tspackages/opencode/test/server/production-boundary.test.tspackages/opencode/test/server/route-inventory-harness.test.tspackages/opencode/test/server/session-e2e-routes.test.tspackages/opencode/test/server/session-messages.test.ts
💤 Files with no reviewable changes (4)
- packages/opencode/src/server/routes/instance/session.ts
- packages/opencode/src/server/instance/index.ts
- packages/opencode/script/route-inventory.ts
- packages/opencode/src/server/instance/session.ts
13 tasks
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.
Summary
Retire the legacy session Hono route source now that production
/session*traffic is handled by the Effect HttpApi dispatcher and sharedSessionRouteEffects.SessionRoutes()from the instance Hono tree and delete the stale route re-export.SessionRouteEffectsas the shared implementation used by the HttpApi session handlers./session*rows are no longer Hono-backed while PawWork-owned session rows keep their classification.SessionRoutes()construction and onto the production dispatcher or HttpApi handlers.Why
The session owner had already moved its production source of truth to Effect HttpApi handlers, but
packages/opencode/src/server/instance/session.tsstill exported and built a large legacy Hono route tree. This kept session rows marked as Hono sources in the inventory and left tests coupled to an implementation that production no longer uses.Related Issue
Related to #936
Human Review Status
Pending— waiting for a human reviewer to approve.Review Focus
Please focus on whether all legacy
SessionRoutes()entrypoints are gone without losing/session*OpenAPI, v2 SDK, local HttpApi, production dispatcher routing, or PawWork-owned inventory coverage.Risk Notes
Behavior risk is low because production
/session*traffic already routes throughproduction-httpapi.tsandsessionHandlers. The main risk is test or inventory drift, covered by the focused session route tests, production boundary tests, route inventory harness,route:inventory, typecheck, and the home submit E2E smoke.Skipped checklist items:
route:inventorywas run for verification, but the local-only generated report is not part of this PR.Fresh-eye result: no P0/P1 findings. I considered renaming the internal
Effect.fn("SessionRoutes.*")trace labels, but left them unchanged because they do not export or build the Hono route tree and changing trace names would add observability churn without improving the migration boundary. The CI follow-up tightened an over-specific inventory classification assertion for ordinary upstream-covered/sessionrows while preserving the PawWork-owned classification checks, and added production dispatcher coverage for header-scopedPOST /sessioncreation.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.Summary by CodeRabbit
Tests
Refactor