Skip to content

refactor(opencode): serve control docs from HttpApi source - #1398

Merged
Astro-Han merged 1 commit into
devfrom
codex/i936-openapi-doc-httpapi
Jun 19, 2026
Merged

refactor(opencode): serve control docs from HttpApi source#1398
Astro-Han merged 1 commit into
devfrom
codex/i936-openapi-doc-httpapi

Conversation

@Astro-Han

@Astro-Han Astro-Han commented Jun 19, 2026

Copy link
Copy Markdown
Owner

Summary

Move the production /doc OpenAPI source onto an HttpApi-owned builder for the control-plane document.

Why

The production HttpApi /doc handler still generated its response by calling ControlPlaneRoutes().request("/doc"), which pulled the legacy Hono control route tree back into the production documentation path. This keeps the existing HTTP contract while making the production /doc path independent from that legacy route tree.

Related to #936

Related Issue

Related to #936

Human Review Status

Pending

Review Focus

Please focus on the production /doc import boundary, especially that the handler now calls controlOpenApi() from a module that does not import the legacy Hono route tree, and that Server.openapi() remains the only lazy entry point for the legacy spec-generation module.

Risk Notes

  • No visible UI or copy changed, so no screenshots or recordings are included.
  • This changes a server OpenAPI document source, but the focused tests verify the existing /doc metadata, global SSE paths, schema refs, and stale /question exclusion.
  • No platform, packaging, dependency, permission, credential, deletion, generated-content, or local-file behavior changed.

How To Verify

Focused server tests: 39 passed with bun test test/server/control-routes.test.ts test/server/production-boundary.test.ts test/server/route-inventory-harness.test.ts from packages/opencode
Route inventory: counts remained hono=128, openapi=100, legacySdk=64, v2Sdk=124, localHttpApi=122; /question remains openapi-only; /doc remains localHttpApi=true with specialSurface=OpenAPI source
Typecheck: GOMAXPROCS=2 bun run typecheck passed from packages/opencode
Diff check: git diff --check passed from the worktree root
Fresh-eye review: final fresh-eye reviewer found no P0/P1 and no remaining P2/P3 worth fixing before PR

Screenshots or Recordings

Not applicable: no visible UI changes.

Checklist

How to use this checklist:

  • Tick a box by replacing [ ] with [x]. Do not edit, add, or remove items.
  • The bot-applied label items can only be honestly ticked AFTER the PR is opened and the labeler / priority-triage bots have run — return to the PR description and tick them then.
  • Most items are required. The few that are conditional are explicitly marked (conditional); for those, leave unticked if they truly do not apply and explain why in Risk Notes. All other items must be ticked before requesting human review.
  • Type label — this PR carries exactly one of 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.
  • Routing labels — this PR carries at least one of 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.
  • Priority label — this PR carries exactly one of P0, P1, P2, P3. The priority-triage bot suggests one on PR open. Confirm or override, then tick this.
  • Human Review Status above is set to Pending, Approved by @<reviewer>, or Not required: <reason> (default is Pending; "not required" is restricted to bot-authored low-risk PRs).
  • I linked the related issue, or stated in Summary why there is no issue.
  • I described the review focus and any meaningful risks.
  • I replaced the example block in How To Verify with the real verification steps and the key result for each.
  • I did not introduce unrelated refactors, dependencies, generated files, or file changes beyond the stated scope.
  • (conditional) I manually checked visible UI or copy changes when needed, with screenshots or recordings. Leave unticked only if no visible UI or copy changed.
  • (conditional) I considered macOS and Windows impact for platform, packaging, updater, signing, paths, shell, or permissions changes. Leave unticked only if no platform/packaging surface was touched.
  • (conditional) I called out docs, release notes, dependencies, permissions, credentials, deletion behavior, generated content, or local file changes when relevant. Leave unticked only if none of those surfaces was touched.
  • I reviewed the final diff for unrelated changes and suspicious dependency changes.
  • I am targeting dev, and my PR title and commit messages use Conventional Commits in English.

Summary by CodeRabbit

  • Refactor

    • Reorganized OpenAPI schema definitions for improved maintainability and code structure.
    • Optimized OpenAPI document generation for the control API.
  • Tests

    • Added production boundary tests ensuring proper API documentation handling.
    • Updated existing tests to verify API documentation completeness.

@Astro-Han Astro-Han added task Narrow execution, audit, spike, migration, tracking, or upstream follow-up work harness Model harness, prompts, tool descriptions, and session mechanics P2 Medium priority labels Jun 19, 2026

@github-actions github-actions Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested priority: P2 (includes non-doc, non-test paths outside the low-risk bucket).

P1/P0 are reserved for maintainer confirmation. Please relabel manually if this is a release blocker, security issue, data-loss risk, or updater/runtime failure.

@coderabbitai

coderabbitai Bot commented Jun 19, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

📝 Walkthrough

Walkthrough

Extracts a new global-openapi-schema.ts module with shared Zod schema builders for global and sync events, creates a dedicated control-openapi.ts builder that assembles the full control HTTP API OpenAPI document, removes local schema-merge logic from the control handler, and converts server.ts to use a dynamic import for openapi.

Changes

Control OpenAPI document builder extraction

Layer / File(s) Summary
Global event schema extraction
packages/opencode/src/server/global-openapi-schema.ts, packages/opencode/src/server/instance/global.ts
Introduces global-openapi-schema.ts exporting globalEventOpenApiSchema() and globalSyncEventOpenApiSchema(), and removes the equivalent in-file definitions from global.ts, replacing them with imports from the new module.
Control OpenAPI document builder
packages/opencode/src/server/control-openapi.ts
Adds OpenApiDocument type, ControlDocApi composition of ControlApi+GlobalApi, mergeSchemas helper, and controlOpenApi() which clones the generated spec, removes /doc, injects two SSE endpoints, merges component schemas, and returns the final document.
Control handler and server.ts wiring
packages/opencode/src/server/routes/instance/httpapi/handlers/control.ts, packages/opencode/src/server/server.ts
Removes local collectSchemaRefs and schema-merge helpers from the control handler and switches the "doc" route to call controlOpenApi(); converts server.ts openapi() from a static import to a dynamic await import("./openapi").
Test coverage
packages/opencode/test/server/control-routes.test.ts, packages/opencode/test/server/production-boundary.test.ts
Drops the Server.openapi() precondition from the control-routes test and adds a /question path absence assertion; adds a production boundary test asserting /doc is absent from legacy control-plane wiring.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

Possibly related PRs

  • Astro-Han/pawwork#1384: Modifies the same packages/opencode/src/server/routes/instance/httpapi/handlers/control.ts file, adding controlHandlers group wiring for /auth and /log routes that this PR subsequently refactors.

Suggested labels

upstream, tech-debt

Poem

🐇 Hop, hop! The schemas flew free,
Extracted to their own cozy burrow with glee.
No more local merging, all tangled and wide—
The /doc path now builds with clean, tidy pride.
Dynamic imports, a lighter load to bear,
This rabbit approves of the fresh tidy lair! 🌿

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Title check ✅ Passed The title accurately describes the main refactoring: moving OpenAPI doc serving from legacy Hono routes to HttpApi source.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
Description check ✅ Passed The PR description comprehensively covers all required sections with clear explanations of changes, rationale, related issue, review focus, risks, and verification steps.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch codex/i936-openapi-doc-httpapi

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 Nitpick comments (1)
packages/opencode/src/server/routes/instance/httpapi/handlers/control.ts (1)

124-124: ⚡ Quick win

Normalize /doc failures through controlFailure like the other control handlers.

Line 124 only maps success; rejected promises/defects from controlOpenApi() won’t use the local error response mapping.

Suggested patch
       .handleRaw("doc", () =>
-        Effect.promise(() => controlOpenApi()).pipe(Effect.map((document) => HttpServerResponse.jsonUnsafe(document))),
+        Effect.promise(() => controlOpenApi()).pipe(
+          Effect.map((document) => HttpServerResponse.jsonUnsafe(document)),
+          Effect.catch(controlFailure),
+          Effect.catchDefect(controlFailure),
+        ),
       )
🤖 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` at
line 124, The effect pipeline starting with Effect.promise(() =>
controlOpenApi()) only maps successful responses using Effect.map to return
HttpServerResponse.jsonUnsafe(document), but lacks error handling for rejected
promises or defects. Add error handling to the effect pipeline (using catchAll
or similar approach) that maps failures through the controlFailure function,
consistent with how other control handlers normalize error responses, ensuring
all failure cases are processed through the same error response mapping.
🤖 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/src/server/routes/instance/httpapi/handlers/control.ts`:
- Line 124: The effect pipeline starting with Effect.promise(() =>
controlOpenApi()) only maps successful responses using Effect.map to return
HttpServerResponse.jsonUnsafe(document), but lacks error handling for rejected
promises or defects. Add error handling to the effect pipeline (using catchAll
or similar approach) that maps failures through the controlFailure function,
consistent with how other control handlers normalize error responses, ensuring
all failure cases are processed through the same error response mapping.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro Plus

Run ID: 8c0e259d-9872-4d9a-ad65-ab8a8560c04b

📥 Commits

Reviewing files that changed from the base of the PR and between 7837d82 and 2fecc8b.

📒 Files selected for processing (7)
  • packages/opencode/src/server/control-openapi.ts
  • packages/opencode/src/server/global-openapi-schema.ts
  • packages/opencode/src/server/instance/global.ts
  • packages/opencode/src/server/routes/instance/httpapi/handlers/control.ts
  • packages/opencode/src/server/server.ts
  • packages/opencode/test/server/control-routes.test.ts
  • packages/opencode/test/server/production-boundary.test.ts

@Astro-Han
Astro-Han merged commit 4424ae7 into dev Jun 19, 2026
45 of 46 checks passed
@Astro-Han
Astro-Han deleted the codex/i936-openapi-doc-httpapi branch June 19, 2026 09:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

harness Model harness, prompts, tool descriptions, and session mechanics P2 Medium priority task Narrow execution, audit, spike, migration, tracking, or upstream follow-up work

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant