Make PR reviews strictly read-only

[dceoy]

Summary

Fixes #24.

This PR establishes a review-only boundary while preserving opencode github run:

• uses a dedicated fail-closed orchestrator and read-only reviewer agents;
• disables reviewed project config/plugins and installs a fresh global toolkit for /review-pr;
• exposes only exact argument-free helper operations;
• pins one trusted PR context and fails if the head changes;
• preserves .env read protections;
• keeps /review-pr simplify proposal-only.

Validation

• Regression coverage for pull-request and issue_comment context, structured submission, head movement, shell composition, and a malicious project plugin.
• ShellCheck source directives and intentional-literal suppressions applied.
• All review threads resolved.
• action.yml still invokes opencode github run.

[dceoy]

Two security-boundary issues remain. The reviewer subagents are substantially safer, but the orchestrator still has unrestricted general capabilities, and the new read permissions weaken OpenCode's default secret-file protection. CI is passing, but these should be corrected before merge.

[dceoy]

Two security/correctness gaps remain. CI is passing, but the review-only boundary can still be bypassed by project-local OpenCode extensions from the reviewed checkout, and the analyzed head is not pinned across context gathering and submission. Also, the action still installs security-critical toolkit files with cp -rn, so stale versions survive on persistent/self-hosted runners. Use an isolated fresh config directory for review mode and persist one immutable trusted PR context for the entire run before treating #24 as resolved.

[opencode-agent]

PR Review: Make PR reviews strictly read-only

Overall the boundary is well-defended: fail-closed "*": deny for every reviewer, explicit .env denials restored, exact argument-free helper allowlist in the orchestrator, project config disabled in review mode, fresh toolkit reinstall with cp -r (not cp -rn), pinned {repository, pr_number, head_sha} revalidated on every helper call, and a malicious-plugin regression fixture. All 35 bats tests pass; shellcheck is clean on the new shell scripts; the existing shell-parse, jq-validate, and identity-verification coverage still holds. I have a small handful of findings, plus a few observations, in priority order.

Findings

1. review-pr.md Section 3 contradicts Sections 1 and 4 about prepare (important)

.opencode/commands/review-pr.md:18 says prepare is run "once" in section 1, and .opencode/commands/review-pr.md:78 ("the single prepare in section 1") reaffirms that. But .opencode/commands/review-pr.md:65 says:

"For findings, first run the fixed prepare operation. Then use the edit tool only for $HOME/.config/opencode/review-state/initial.json..."

prepare is rm -rf "${state_dir}" followed by recreating context.json empty (review-pr-submit.sh:38). If the orchestrator actually follows Section 3, the second prepare wipes the just-pinned context.json, and submit-initial then fails at trusted_context ("Pinned PR context is unavailable or the PR head changed."). The instruction is a leftover from before prepare was moved to Section 1 by 0f91a28. Remove or rewrite this sentence — for example, "For findings, the prepare operation has already created the empty initial.json from Section 1; write the structured payload there with the edit tool only."

2. The malicious-plugin test only checks source, not runtime (suggestion)

test-review-pr-read-only.bats:129 verifies that action.yml contains the OPENCODE_DISABLE_PROJECT_CONFIG key and that the fixture file contains the writeFileSync literal. It does not run the action end-to-end against the fixture, so a regression that silently drops the env var (e.g. someone rewrites the condition with a string-comparison typo) would still pass. The static check is reasonable as a smoke test, but worth annotating in a comment that the real defense-in-depth here is the env-var name in source; a true end-to-end test would need an OpenCode runtime, which is outside what this suite covers.

3. Review-mode detection is substring-based on the comment body (suggestion)

Both the Copy bundled OpenCode config step (action.yml:106, *"/review-pr"*) and the Run OpenCode step (action.yml:135, contains(github.event.comment.body, '/review-pr')) match /review-pr as a substring of the event comment. A comment like "FYI /review-pr-mode is unrelated" would still trip review mode. The Bash EVENT_COMMENT check is anchored to the prompt regex, but the EVENT_COMMENT branch uses a glob with no anchoring. The two-step if filter on the workflow is the primary gate; for safety, consider restricting the comment match to *"/review-pr"* || *"/review-pr "* || *"/review-pr\n"* or matching an anchored regex, so a stray mention cannot silently switch the action into a stricter mode.

4. Helper scripts use globals for cross-function state (nit)

read_context in review-pr-gh.sh:25 and trusted_context in review-pr-submit.sh:18 set repo, pr_number/number, head_sha, and current_head as script-global variables that the surrounding case arms then consume. This is consistent across the two scripts, but it means metadata and diff cannot run if a future change shadows any of those names. Returning values via stdout (or moving to a single case arm-local scope) would be sturdier; not a security issue today.

Observations (no action needed)

• The simplify aspect is correctly carved out from all (review-pr.md:45) and the code-simplifier agent has the same "*": deny block as the other reviewers, so the simplify path is genuinely read-only.
• The action's find ... ! -name bin -exec rm -rf {} + (action.yml:114) preserves only the cached opencode binary across runs in review mode, which is exactly the right surface area to keep — the binary cache key is content-addressed, so surviving state is benign.
• The orchestrator's task allowlist covers all 11 reviewer agents and nothing else; general/build are not reachable from /review-pr.
• The external_directory allowlist in .opencode/opencode.jsonc is now down to seven paths (three scripts + three state files + the resolver), which is the minimum needed for the helper flow.
• ShellCheck is clean on all three new shell scripts; the bats tests pass locally; the resolve-app-token and opencode-config tests still hold.

The security boundary around /review-pr looks solid. The main concrete ask is to fix the prepare documentation in Section 3 of review-pr.md; the others are tightening suggestions.

github run

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: b290da6342

ℹ️ 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".

[dceoy]

PR Review Summary

No actionable issues meeting the review’s reporting threshold were identified.

Checked

• Review-only mode detection and project-config/plugin isolation in action.yml.
• Fail-closed orchestrator and reviewer-agent permissions, including .env protections and exact helper allowlists.
• Trusted PR context pinning, head-SHA revalidation, structured review submission, and failure handling in the review helper scripts.
• Pull-request and issue_comment regression coverage, shell-composition rejection, malicious-plugin fixture coverage, and current-head CI status.
• Existing review threads and prior findings; all are resolved or outdated for this head.

Notes

The current-head CI/CD workflow completed successfully.

Recommended Action

No automated blocking concerns were identified. Merge remains a maintainer decision after required checks pass.

[dceoy]

Post-merge review found two residual issues that should be handled in a follow-up:

1. critical · security (action.yml): OPENCODE_DISABLE_PROJECT_CONFIG=1 is not a complete fail-closed boundary. Current OpenCode still loads explicit OPENCODE_CONFIG, OPENCODE_CONFIG_DIR, and OPENCODE_CONFIG_CONTENT; OPENCODE_CONFIG_DIR is included even while project discovery is disabled, and config content can declare local plugins. Composite steps inherit workflow/job environment variables, so any of these can reintroduce project-controlled configuration/plugins. Review mode should clear these variables (or replace them with a known-safe config path/content) and cover the behavior with a real installed-runtime malicious-plugin test.

   The action also permits arbitrary pinned OpenCode versions, while OPENCODE_DISABLE_PROJECT_CONFIG was added upstream later. Review mode should enforce a minimum compatible OpenCode version or fail closed after verifying the capability; otherwise an older pinned binary silently ignores the isolation flag.

2. important · correctness (.opencode/scripts/review-pr-submit.sh): the pinned head is checked before opencode_require_app_token_for_review, but token identity verification performs one or more API create/delete operations before the actual POST/PUT. A new commit arriving during that interval can still receive a review anchored to the old head. Revalidate the pinned head after token verification, immediately before each review write (including retry/update paths), and add a regression test that changes the head during verification.