fix: avoid truncate permission import cycle#18292
Conversation
Move permission rule evaluation into a leaf helper so truncate effect can check task access without importing the runtime-backed permission module.
Run truncation through a fresh Bun process so the suite fails if the truncate, permission, and runtime modules form a cycle again.
Load the real truncate-effect module in a fresh Bun process so the regression fails when the permission/runtime circular dependency is reintroduced.
There was a problem hiding this comment.
Pull request overview
This PR breaks a circular dependency between truncation and the runtime-backed permission module by extracting permission rule evaluation into a leaf helper and updating truncation to use it, plus adding a regression test that ensures the module graph remains loadable in a fresh process.
Changes:
- Extracted permission rule matching into
permission/evaluate.tsand delegatedPermissionNext.evaluate()to it. - Updated
truncate-effectto use the leafevaluate()helper instead of importing the permission runtime module. - Added a fresh-process test to catch future import-cycle regressions.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| packages/opencode/test/tool/truncation.test.ts | Adds a regression test that loads truncate-effect in a fresh process to detect circular imports. |
| packages/opencode/src/tool/truncate-effect.ts | Switches task-permission checks to use the leaf evaluate() helper, avoiding the permission module import cycle. |
| packages/opencode/src/permission/index.ts | Delegates PermissionNext.evaluate() implementation to the extracted leaf helper while preserving logging. |
| packages/opencode/src/permission/evaluate.ts | Introduces a leaf helper implementing the wildcard rule evaluation logic. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| (rule) => Wildcard.match(permission, rule.permission) && Wildcard.match(pattern, rule.pattern), | ||
| ) | ||
| return match ?? { action: "ask", permission, pattern: "*" } | ||
| log.info("evaluate", { permission, pattern, ruleset: rulesets.flat() }) |
There was a problem hiding this comment.
PermissionNext.evaluate() now flattens rulesets for logging and then evalRule() flattens again internally. This adds avoidable work on every permission check. Consider flattening once and passing the flattened list into the helper (e.g., change the helper to accept a single rules array), or move the logging/flattening into the helper so it only happens once.
| log.info("evaluate", { permission, pattern, ruleset: rulesets.flat() }) | |
| log.info("evaluate", { permission, pattern, rulesets }) |
Summary
truncate-effectcan check task access without importing the runtime-backed permission moduleTesting