refactor(opencode): add pty control HttpApi coverage - #1384
Conversation
Astro-Han
added
P2
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
|
Warning Review limit reached
More reviews will be available in 34 minutes and 26 seconds. Learn how PR review limits work. Your organization has used up its prepaid credits, and credit purchases are no longer available. Enable the review add-on in the billing tab to keep reviews running — you're only billed for reviews past your plan's rate limits ($0.25/file). ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based credits. 🚦 How do rate limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan refill rate. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, the refill rate gradually slows as usage increases. The highest same-day bursts are limited more strictly. 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 (7)
📝 WalkthroughWalkthroughTwo new Effect ChangesControl and PTY HttpApi Route Migration
Sequence Diagram(s)sequenceDiagram
participant Client
rect rgba(100, 149, 237, 0.5)
note over Client,Auth.Service: Control API
Client->>controlHandlers: PUT /auth/:providerID {credentials}
controlHandlers->>parseJsonBody: validate body
parseJsonBody-->>controlHandlers: typed credentials or 400
controlHandlers->>Auth.Service: authSet(providerID, credentials)
controlHandlers-->>Client: 200 true
Client->>controlHandlers: POST /log {level, message, extra}
controlHandlers->>parseJsonBody: validate LogPayload
controlHandlers->>Log.create: writeLog(level, message, extra)
controlHandlers-->>Client: 200 true
end
rect rgba(60, 179, 113, 0.5)
note over Client,Pty.Service: PTY API
Client->>ptyHandlers: POST /pty {command, args}
ptyHandlers->>Pty.Service: create(input)
ptyHandlers-->>Client: 200 PtyInfo
Client->>ptyHandlers: POST /pty/:ptyID/connect-token
ptyHandlers->>Pty.Service: get(ptyID) — existence check
ptyHandlers->>Pty.Service: issueTicket(ptyID)
ptyHandlers-->>Client: 200 {ticket, expires_in}
Client->>ptyHandlers: DELETE /pty/:ptyID
ptyHandlers->>Pty.Service: get(ptyID) — existence check
ptyHandlers->>Pty.Service: remove(ptyID)
ptyHandlers-->>Client: 200 true
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
🚥 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 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
packages/opencode/src/server/routes/instance/httpapi/groups/pty.ts (1)
12-43: ⚡ Quick winUse
Schema.Classfor multi-field PTY schema models.Lines 12-43 define multi-field models (
PtyInfo,PtyCreateInput,PtyUpdateInput,ConnectToken) withSchema.Struct; these should beSchema.Classper repo conventions.As per coding guidelines:
Use Schema.Class for multi-field data in Effect schemas.🤖 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/src/server/routes/instance/httpapi/groups/pty.ts` around lines 12 - 43, The four schema definitions PtyInfo, PtyCreateInput, PtyUpdateInput, and ConnectToken are currently using Schema.Struct but should be converted to Schema.Class per repository conventions for multi-field data models in Effect schemas. Replace Schema.Struct with Schema.Class for all four of these schema definitions while keeping the field definitions and structure unchanged.Source: Coding guidelines
packages/opencode/src/server/routes/instance/httpapi/groups/control.ts (1)
11-16: ⚡ Quick winUse
Schema.Classfor the multi-field log payload schema.Lines 11-16 define a multi-field Effect schema with
Schema.Struct. Converting this toSchema.Classkeeps this file aligned with the repo’s Effect schema conventions.♻️ Proposed refactor
-const LogPayload = Schema.Struct({ - service: Schema.String, - level: Schema.Literals(["debug", "info", "error", "warn"]), - message: Schema.String, - extra: Schema.optional(Schema.Record(Schema.String, Schema.Any)), -}) +class LogPayload extends Schema.Class<LogPayload>("ControlLogPayload")({ + service: Schema.String, + level: Schema.Literals(["debug", "info", "error", "warn"]), + message: Schema.String, + extra: Schema.optional(Schema.Record(Schema.String, Schema.Any)), +}) {}As per coding guidelines:
Use Schema.Class for multi-field data in Effect schemas.🤖 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/src/server/routes/instance/httpapi/groups/control.ts` around lines 11 - 16, The LogPayload schema definition is currently using Schema.Struct for a multi-field data structure. Convert this schema to use Schema.Class instead, as per the repository's Effect schema conventions for multi-field data. Keep the same fields (service, level, message, extra) with their existing types and optional properties when refactoring from Schema.Struct to Schema.Class.Source: Coding guidelines
packages/opencode/src/server/routes/instance/httpapi/handlers/control.ts (1)
20-42: ⚡ Quick winExtract JSON parsing helpers into a shared module.
Lines 20-42 are duplicated in
packages/opencode/src/server/routes/instance/httpapi/handlers/pty.ts. Pulling these into one shared helper will prevent behavior drift in 400 handling and content-type matching.🤖 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/src/server/routes/instance/httpapi/handlers/control.ts` around lines 20 - 42, The functions isJsonContentType, badRequestJson, and parseJsonBody are duplicated across control.ts and pty.ts, which can lead to behavior drift during maintenance. Extract these three functions into a new shared utility module in a common location within the httpapi handlers directory, then import and use these functions from the shared module in both control.ts and pty.ts, removing the duplicate definitions from each file.
🤖 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.
Inline comments:
In `@packages/opencode/src/server/routes/instance/httpapi/groups/pty.ts`:
- Around line 40-43: The ConnectToken schema structure defines expires_in as a
plain Schema.Number, but the actual implementation in ticket.ts issues
expires_in as a positive integer. Update the expires_in field definition in the
ConnectToken schema to apply a positive integer constraint instead of accepting
any number, ensuring the schema accurately reflects the contract of what is
actually being issued and prevents downstream consumers from receiving invalid
values.
---
Nitpick comments:
In `@packages/opencode/src/server/routes/instance/httpapi/groups/control.ts`:
- Around line 11-16: The LogPayload schema definition is currently using
Schema.Struct for a multi-field data structure. Convert this schema to use
Schema.Class instead, as per the repository's Effect schema conventions for
multi-field data. Keep the same fields (service, level, message, extra) with
their existing types and optional properties when refactoring from Schema.Struct
to Schema.Class.
In `@packages/opencode/src/server/routes/instance/httpapi/groups/pty.ts`:
- Around line 12-43: The four schema definitions PtyInfo, PtyCreateInput,
PtyUpdateInput, and ConnectToken are currently using Schema.Struct but should be
converted to Schema.Class per repository conventions for multi-field data models
in Effect schemas. Replace Schema.Struct with Schema.Class for all four of these
schema definitions while keeping the field definitions and structure unchanged.
In `@packages/opencode/src/server/routes/instance/httpapi/handlers/control.ts`:
- Around line 20-42: The functions isJsonContentType, badRequestJson, and
parseJsonBody are duplicated across control.ts and pty.ts, which can lead to
behavior drift during maintenance. Extract these three functions into a new
shared utility module in a common location within the httpapi handlers
directory, then import and use these functions from the shared module in both
control.ts and pty.ts, removing the duplicate definitions from each file.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 75646aec-41c7-4630-b385-a2f0220cf7a1
📒 Files selected for processing (7)
packages/opencode/src/server/routes/instance/httpapi/groups/control.tspackages/opencode/src/server/routes/instance/httpapi/groups/pty.tspackages/opencode/src/server/routes/instance/httpapi/handlers/control.tspackages/opencode/src/server/routes/instance/httpapi/handlers/pty.tspackages/opencode/test/server/control-routes.test.tspackages/opencode/test/server/pty-routes.test.tspackages/opencode/test/server/route-inventory-harness.test.ts
1b34ccd to
ff17a18
Compare
ff17a18 to
6fc530d
Compare
1 participant
Summary
Adds local Effect HttpApi coverage for the remaining PTY JSON/control routes and the small control-plane auth/log routes:
localHttpApi: true.Why
This continues the #936 HttpApi coverage migration while keeping the production server on Hono. The PR intentionally leaves
/doc, PTY WebSocket connect, permission e2e ask, automation, session, SSE, workspace WebSocket, and static UI routes outside this slice.Related Issue
Related to #936
Human Review Status
Pending
Review Focus
Please focus on whether the new local HttpApi handlers preserve the existing Hono wire behavior for JSON parsing, 400/404 bodies, PTY missing targets, auth credential writes/removes, and log level dispatch.
Risk Notes
No production server switch. The local HttpApi coverage mirrors existing Hono behavior and keeps the WebSocket/OpenAPI-source/test-only routes Hono-only. No visible UI or copy changed, so the UI screenshot checklist item is not applicable.
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.