Simplify PR review to a thin orchestrator and dynamic worker - #101
Conversation
dceoy
changed the title
dceoy
changed the title
dceoy
left a comment
dceoy
left a comment
There was a problem hiding this comment.
No actionable issues found. I reviewed the dynamic review-worker architecture, the thin orchestrator permission boundary, aspect-to-lens routing, independent validation flow, OpenCode v1 compatibility assumptions, agent-scoped trusted-path permissions, documentation updates, and contract-test coverage. The current head is internally consistent, and CI/CD run #487 passed for 4a73cbac7ed0ae835642a820d11835d67081d89b.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4a73cbac7e
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
The review identified 7 validated inline findings across the orchestrator permission boundary, the skill procedure contract, and the contract tests. However, structured submission failed: Per the |
dceoy
left a comment
dceoy
left a comment
There was a problem hiding this comment.
Reviewed the current head with a focus on correctness and KISS/DRY/YAGNI.
I found one actionable authorization regression: the PR removes the global external_directory deny boundary together with the trusted-path allowlist. That broadens the policy for every non-orchestrator agent instead of limiting the exception to review-pr-orchestrator.
The smallest coherent fix is to keep a global wildcard deny for external_directory and let only review-pr-orchestrator.md add the trusted-path overrides it requires. I found no additional actionable maintainability findings beyond this authorization issue.
dceoy
left a comment
dceoy
left a comment
There was a problem hiding this comment.
Reviewed head 4a73cbac7ed0ae835642a820d11835d67081d89b with a focus on correctness, permission boundaries, and KISS/DRY/YAGNI.
One actionable issue remains on the current head and is already covered by the unresolved inline feedback on .opencode/opencode.jsonc: removing the global permission.external_directory."*": "deny" rule makes non-orchestrator agents fall back to OpenCode's default ask behavior for external paths. In a headless GitHub Actions run, that can wait indefinitely for interactive approval. The smallest coherent fix is to retain the global wildcard deny while keeping only the narrowly required trusted-path allows in review-pr-orchestrator.md.
I found no additional actionable issues in the dynamic review-worker architecture, thin orchestrator, pr-review discovery/validation flow, or updated contract tests. I am not duplicating the existing inline thread for the same root cause.
dceoy
left a comment
dceoy
left a comment
There was a problem hiding this comment.
Reviewed current head 4a73cbac7ed0ae835642a820d11835d67081d89b with a focus on correctness, permission boundaries, and KISS/DRY/YAGNI.
One actionable issue remains: removing the global permission.external_directory."*": "deny" rule makes non-orchestrator agents fall back to OpenCode's default ask behavior for external paths. In a headless GitHub Actions run, that can wait for interactive approval instead of failing closed. The smallest coherent fix is to retain the global wildcard deny while keeping the narrowly required trusted-path allows scoped to review-pr-orchestrator.
This root cause is already covered by the unresolved inline feedback on .opencode/opencode.jsonc, so I am not duplicating the same line-specific finding with another inline comment. I found no additional actionable issues in the dynamic review-worker architecture, the thin orchestrator, independent discovery/validation flow, or the updated contract tests.
dceoy
left a comment
dceoy
left a comment
There was a problem hiding this comment.
Independent review pass at head b5c898e42eaf0b88133cd22a290ed24a0f812d72, run via three independent lenses (correctness, tests/docs, security/performance).
The previously-flagged authorization regression (global external_directory deny removal) is confirmed fixed on this head: .opencode/opencode.jsonc retains "*": "deny", and trusted-path overrides are scoped solely to review-pr-orchestrator. No other agent config carries stray elevated permissions, and review-worker.md has no bash/edit/task grants.
One actionable regression found in the test that guards that fix, plus two smaller test-coverage gaps and one dead-config note. Full findings below (inline comments on anchorable ones); unanchorable notes summarized here.
Unanchorable note — SKILL.md frontmatter metadata keys don't match a recognized OpenCode schema field (low severity): .opencode/skills/pr-review/SKILL.md sets metadata: {opencode/slash: "false", opencode/autoinvoke: "false"}. The installed OpenCode v1 runtime's skill-frontmatter decoder recognizes only a flat name/description/slash struct — no metadata wrapper and no autoinvoke concept. This is likely inert (the actual /review-pr invocation path doesn't depend on it), but the docs' claim that the skill is "marked non-slash and non-autoinvokable" isn't enforced by this frontmatter on the current runtime. If suppressing auto-registration is intended, use the real top-level slash: false field instead.
No other actionable correctness, security, or performance issues were found in the dynamic review-worker architecture, the thin orchestrator, permission scoping, or documentation.
- restore the global external_directory allow-list check that was silently overwritten before it ran - assert review-worker denies bash/edit/task and only allows read/glob/grep - assert routed aspects map to the correct lens body, not just any bullet
dceoy
left a comment
dceoy
left a comment
There was a problem hiding this comment.
Independent review pass at head 2f670c3348408bf6c020311a8db71ce31a0af3e5, run via the same three lenses (correctness, tests/docs, security/performance). This head only added test coverage (tests/validate-opencode.bats) fixing the previous round's findings; those fixes were independently re-verified as correct and effective by all three lenses.
Two smaller, corroborated refinements to that new test coverage found below (inline). No other actionable issues.
- assert review-worker's frontmatter has exactly the expected top-level permission keys instead of only denylisting three - add an explicit failing default case to the aspect-to-lens keyword check so an unmapped aspect can't silently reuse the previous iteration's keyword
dceoy
left a comment
dceoy
left a comment
There was a problem hiding this comment.
Independent review pass at head 68cca3018c06cd5dd89339d7fa16bc04c10b5c91, run via the same three lenses. The two prior-round test-hardening fixes were re-verified as correct.
Three more test-coverage gaps found (inline below), all in the same "assert the exact allow-list/key-set instead of a partial denylist" family as the fixes already applied this loop.
Two lower-priority items from this round's lenses were considered and not carried forward: (1) a suggestion to add an explicit hard cap on discovery/validation Task fan-out in SKILL.md — flagged at only medium-low confidence and explicitly not a regression from main, so left as an optional future hardening rather than a blocking finding; (2) a suggestion to add a *) fallback to an unrelated case in the variant-validation test — lower risk than the aspect-routing case (that loop variable is a fixed, hardcoded list with one arm per value), so also left as optional polish rather than a finding.
Unanchorable finding — orchestrator's edit permission block is untested (high confidence): .opencode/agents/review-pr-orchestrator.md's edit block restricts writes to exactly two review-state payload files (plus their ../* relative variants) — the mechanism preventing arbitrary file writes. Unlike task, skill, bash, and external_directory, no test in tests/validate-opencode.bats asserts this allow-list, so a regression broadening it to "*": allow or dropping a required relative-path variant would pass every current test. Suggested fix: add a test asserting permission_allow_keys(orchestrator, edit) equals exactly the four expected state-file paths, mirroring the existing task/skill/bash/external_directory tests. (This line isn't in the diff hunks for this head, so it can't be anchored inline.)
- assert the agents_dir file set equals exactly the two current agents instead of denylisting nine legacy names - extend the review-worker key-extraction regex to also match single-quoted and hyphenated/digit permission keys - add a test asserting the orchestrator's edit allow-list is exactly the four review-state payload paths
dceoy
left a comment
dceoy
left a comment
There was a problem hiding this comment.
Independent review pass at head c29f3a201514fa75439648026ba55769b784a863, run via the same three lenses (correctness, tests/docs, security/performance), framed explicitly as a convergence check after three prior hardening rounds on this branch.
No new actionable findings were found in this review pass. All three lenses independently confirmed: the permission boundaries (global external_directory deny-by-default, review-worker's read/glob/grep-only profile, and the orchestrator's task/skill/bash/edit/external_directory allow-lists) are each now backed by an exact allow-set/key-set assertion in tests/validate-opencode.bats rather than a partial denylist; the aspect-to-lens routing test has a failing default case; and docs remain consistent with the two-agent architecture. This does not imply prior feedback on this PR has been newly cleared beyond what earlier rounds already resolved — it reflects that this pass, on this exact head, surfaced nothing further.
2 participants
What changed
review-workerreview-workerchild sessions for both discovery and independent candidate validationcode,performance,security,tests,docs,comments,errors,types,simplify) as lens constraints rather than agent routingreview-pr-orchestratora thin primary-agent permission profile; all review behavior lives inpr-review/SKILL.md/review-pras the supported entrypoint and mark the internalpr-reviewskill non-slash/non-autoinvokable for OpenCode v2 discoveryexternal_directoryallowances from globalopencode.jsonc; only the dedicated orchestrator grants themreview-workerto the read-only boundary and task contract instead of duplicating review policyArchitecture
/review-prremains bound to the dedicatedreview-pr-orchestratorprimary agent instead of the general main agent. This keeps the GitHub review path fail-closed: the primary agent may load onlypr-review, invoke only the review worker, write only the two review-state payloads, execute only the fixed trusted helper commands, and access only the trusted helper/state external paths.The orchestrator itself contains no review procedure beyond following the skill. Discovery, dynamic task decomposition, candidate arbitration, independent validation, anchoring, and submission sequencing are defined once in
pr-review/SKILL.md.review-workeris the current-runtime read-only child-session boundary. It has only read/glob/grep permissions and cannot edit files, run shell commands, launch subagents, or mutate GitHub state. Its prompt now describes only that boundary and the generic discovery/validation contract; detailed review policy remains in the skill.OpenCode v2 direction
OpenCode v2 defines a built-in read-only
exploresubagent and a parent-side subagent permission model suitable for this workflow. Once the action moves to a v2 runtime with those guarantees,review-workercan be removed and fresh built-inexploresessions can perform discovery and validation. The dedicated primary-agent permission profile and/review-prcommand binding should remain because the generic main agent has broader mutation capabilities than PR review requires.The internal
pr-reviewskill carriesopencode/slash: "false"andopencode/autoinvoke: "false"metadata so V2 does not expose it as an alternate user/model entrypoint. V1 accepts this metadata as portable skill metadata and continues to use the explicit/review-prcommand path.Why
The previous fixed reviewer files duplicated review policy and coupled coverage to a static set of agent identities. A single bounded worker plus dynamic task packets keeps the adapter smaller and allows concerns such as migrations, concurrency, infrastructure, compatibility, and observability without adding more agents.
Keeping orchestration policy in the skill avoids duplication, while moving trusted external-path allowances out of global config ensures ordinary agents do not inherit review-specific capabilities.
Impact
/review-pragent bindingreview-workerto OpenCode v2 built-inexploreValidation
review-pr-orchestratorismode: primary, selected explicitly by/review-pr, and acts only as a permission-constrained wrapper aroundpr-reviewreview-workerismode: subagent, hidden, and limited to read/glob/grep permissionsreview-pr-orchestratorpermits onlyreview-workerthroughpermission.taskopencode.jsoncno longer grants trusted review external pathsCI/CD run #487 is queued for head
4a73cbac7ed0ae835642a820d11835d67081d89b.