Skip to content

refactor(server): effectify permission reply route - #1032

Merged
Astro-Han merged 1 commit into
devfrom
codex/i936-permission-route-effect
Jun 2, 2026
Merged

refactor(server): effectify permission reply route#1032
Astro-Han merged 1 commit into
devfrom
codex/i936-permission-route-effect

Conversation

@Astro-Han

@Astro-Han Astro-Han commented May 31, 2026

Copy link
Copy Markdown
Owner

Summary

Migrate POST /permission/:requestID/reply to the existing AppRuntime.runPromise(Effect.gen(...)) route runtime pattern.

Why

This continues the #936 ordinary JSON route migration for a narrow permission JSON route without changing permission semantics or touching auth, streaming, automation, workspace sync, v2, or SDK/OpenAPI source areas.

Related Issue

Refs #936

Human Review Status

Pending

Review Focus

Please check that the route still forwards the same request ID, reply, and optional message to Permission.Service.reply.

Risk Notes

This PR does not change the e2e permission seed route or permission list pruning behavior.

Skipped conditional checklist items:

  • Visible UI/copy check: not applicable; server-only route refactor.
  • Platform/packaging check: not applicable; no platform or packaging surface touched.
  • Docs/release/dependency/local file check: not applicable; no docs, dependencies, generated content, credentials, or local-only files changed.

How To Verify

Focused route tests: bun test test/server/permission-routes.test.ts -> 1 pass, 0 fail
Diff check: git diff --cached --check -> passed before commit

Screenshots or Recordings

Not applicable; no visible UI changes.

Checklist

  • 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

    • Improved internal permission request handling logic.
  • Tests

    • Added comprehensive test coverage for permission request routes to ensure reliability.

@Astro-Han Astro-Han added P2 Medium priority upstream Tracked upstream or vendor behavior harness Model harness, prompts, tool descriptions, and session mechanics task Narrow execution, audit, spike, migration, tracking, or upstream follow-up work tech-debt Supplemental cleanup, maintainability, architecture, test, or quality debt context labels May 31, 2026
@coderabbitai

coderabbitai Bot commented May 31, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro Plus

Run ID: 64c23482-135a-4155-b088-f33b0584cd7e

📥 Commits

Reviewing files that changed from the base of the PR and between c019c11 and ce695ae.

📒 Files selected for processing (2)
  • packages/opencode/src/server/instance/permission.ts
  • packages/opencode/test/server/permission-routes.test.ts

📝 Walkthrough

Walkthrough

This PR refactors the permission reply HTTP route handler to use AppRuntime.runPromise with an Effect.gen block for invoking the permission service, replacing a prior direct call. A comprehensive test suite validates the new runtime-integrated behavior end-to-end.

Changes

Permission Reply Handler Refactor

Layer / File(s) Summary
Permission reply handler refactored to Effect runtime
packages/opencode/src/server/instance/permission.ts
The /:requestID/reply route handler now executes permission processing via AppRuntime.runPromise using Effect.gen to invoke Permission.Service instead of calling Permission.reply(...) directly, while maintaining the c.json(true) response.
Permission routes HTTP test suite
packages/opencode/test/server/permission-routes.test.ts
Test suite for permission HTTP routes that validates posting to /permission/:id/reply by wiring a pending Permission.Service.ask, sending a JSON reply request, asserting a 200 response with true body, and confirming the ask resolves successfully.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

Poem

🐰 A handler hops from direct calls to the Effect-ful way,
Runtime promises dance where logic used to stay,
Tests now verify the flow, ensuring truth rings clear,
Permission replies bloom—the refactor's finally here!

🚥 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 effort—migrating the permission reply route to use the Effect-based runtime pattern.
Description check ✅ Passed The description covers all required sections including summary, rationale, related issue, review focus, risk notes, verification steps, and properly completed checklist items.
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.

✏️ 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-permission-route-effect

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.

@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.

@gemini-code-assist gemini-code-assist 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.

Code Review

This pull request refactors the permission reply route to execute within the AppRuntime using Effect.gen and introduces a new test suite for verifying permission routes. The feedback identifies a potential TypeScript type mismatch in the test file where onPending expects an Effect<void> but receives an Effect<boolean> from Deferred.succeed, suggesting the use of Effect.asVoid to ensure strict type safety.

Comment thread packages/opencode/test/server/permission-routes.test.ts
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 tech-debt Supplemental cleanup, maintainability, architecture, test, or quality debt context upstream Tracked upstream or vendor behavior

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant