Revert "Make PR reviews strictly read-only" - #29
Conversation
This reverts commit af593ef.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit:baa3c2fa7b
ℹ️ 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".
| echo "Copied bundled OpenCode config, preserving existing files" | ||
| fi | ||
| mkdir -p "${HOME}/.config/opencode" | ||
| cp -rn "${ACTION_PATH}/.opencode/." "${HOME}/.config/opencode/" |
There was a problem hiding this comment.
On /review-pr runs against a checked-out PR, this preserving copy is now paired with the removal of OPENCODE_DISABLE_PROJECT_CONFIG, so OpenCode can still discover .opencode project config/plugins from the repository under review instead of only the trusted toolkit. In workflows that run the reviewer with provider secrets and pull-requests: write, a PR can add a project plugin/command that executes inside the review job before the bundled reviewer starts; the deleted malicious-plugin regression was guarding this exact path. Please keep the fresh trusted config and disable project config for review prompts.
Useful? React with 👍 / 👎.
| trap 'rm -f "${review_payload:-}" "${review_update_payload:-}"' EXIT | ||
| jq -n \ | ||
| --arg commit_id "$head_oid" \ |
There was a problem hiding this comment.
When a review takes long and the PR is pushed after the initial headRefOid/diff are captured, this payload still uses the old $head_oid without first checking that it equals the current gh pr view --json headRefOid. GitHub's review API accepts non-latest commit IDs and documents that they can make comments outdated after subsequent commits, so the POST can succeed with inline comments against a stale diff instead of failing or restarting; re-fetch and compare the head SHA immediately before each review write as the removed helper did.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
OpenCode PR Review (this review covers .opencode/agents/code-simplifier.md:6; companion reviews on .opencode/commands/review-pr.md:89, action.yml:104, and action.yml:118 carry the other three inline findings).
This PR is a clean revert of #27 (the fix for #24). The local QA suite (validate-opencode.bats, test-resolve-app-token.bats, prettier, yamllint, shellcheck) still passes on the reverted tree, so the basic invariants hold. The open question is whether the maintainer is intentionally rolling back the review-only boundary that #27 added; if so, the findings below are the most concrete regressions to call out so they are at least visible in the PR history.
Context: #24 documents an incident in which /review-pr executed the reviewed repo's QA script, picked up ruff format / ruff check --fix / prettier --write / zizmor --fix, generated a github_conf/branch_protection_rules.json containing a 404 response, and opencode github run then committed and pushed the dirty worktree to the PR branch as opencode-agent[bot]. #27 closed the resulting trust gap; this PR reopens it.
Summary-only findings (covered in the body of the companion reviews on this PR):
review-pr.md(TOCTOU) — Loss of head-SHA pinning. #27'sreview-pr-gh.shandreview-pr-submit.shre-read the liveheadRefOidimmediately before eachPOST/PUTtopulls/.../reviewsand aborted on a mismatch. The reverted state has no such guard, so a force-push betweengh pr viewand the review write can attach the review to a different SHA than the one that was originally analyzed.action.yml(coverage) — Loss oftest-review-pr-read-only.batsregression coverage (140 lines covering context detection, head-movement rejection, shell-composition rejection, project-plugin exclusion). The token-resolution path is still covered, but nothing now verifies the review-only guard, the toolkitrm -rf, orOPENCODE_DISABLE_PROJECT_CONFIG.review-pr.md(untested shell) — Token resolution and the 60-linegh apireview-submission block are inlined as untested markdown code blocks.set -euo pipefailis gone,shellcheckdoes not see them, and identity attribution (opencode-agent[bot]vsgithub-actions[bot]vs fail-closed) now lives in prose rather than in a script with bats tests.code-simplifier.md(cosmetic) — Frontmatter is the only one of 11 agent files without acolor:key. The only agent withedit: allowis also the only one missingcolor:, which makes the inconsistency stand out.README.md(docs/contract drift) —README.md:143says/review-pr simplifyis "refinement only, does not return a review", but withcode-simplifierreverted toedit: allowthat wording is misleading. Either the agent should remain read-only or the README should describe the new mutating behaviour.
Net effect: the maintainer appears to be trading #27's hard review-only boundary for the simpler pre-#27 workflow, accepting that /review-pr can now mutate the reviewed repo (via the simplify aspect, via the reviewed repo's own .opencode/ config, and via any shell command the agent decides to run) and that head-SHA pinning is best-effort. Token attribution, the external_directory allow-list, and opencode-agent[bot] identity verification are unchanged from #27, so the worst-case identity attribution is still bounded; the remaining holes are data-integrity holes, not token holes.
| "*.env.example": allow | ||
| glob: allow | ||
| grep: allow | ||
| edit: allow |
There was a problem hiding this comment.
critical · opencode-agent[bot]: edit: allow re-opens the exact failure mode #24 documented. With this and the new long-form body (You operate autonomously and proactively, refining code immediately after it's written or modified without requiring explicit requests), /review-pr simplify will now write to the reviewed checkout instead of producing proposals. #27's read-only simplify was proposal-only; if the maintainer wants simplify to actually modify files, the AGENTS.md and README should say so explicitly and the agent should at least carry a color: key for consistency with the other ten agents.
There was a problem hiding this comment.
critical · opencode-agent[bot] (replaced placeholder): the simplify aspect dispatches code-simplifier with no allow-list or read-only constraint, and the agent at .opencode/agents/code-simplifier.md:6 is now edit: allow. Functionally this hands the LLM a write tool over the reviewed PR. If a malicious PR is being reviewed, the agent's own "Recently modified code" framing is whatever files that PR just added, and there is no scope guard distinguishing simplify-on-the-PR from simplify-on-the-worktree. Either gate simplify behind the read-only simplify that #27 had, or document the new mutating behaviour in README.md:143 (which currently still says "refinement only, does not return a review"). The full review body that replaces these placeholder review entries follows in a separate structured review.
| - `comments` → `comment-analyzer` | ||
| - `errors` → `silent-failure-hunter` | ||
| - `types` → `type-design-analyzer` | ||
| - `simplify` → run `code-simplifier` as a refinement step only; do not return a review; stop after simplification |
There was a problem hiding this comment.
critical · opencode-agent[bot]: the simplify aspect dispatches code-simplifier with no allow-list or read-only constraint, and the agent at .opencode/agents/code-simplifier.md:6 is now edit: allow. Functionally this hands the LLM a write tool over the reviewed PR. If a malicious PR is being reviewed, the agent's own "Recently modified code" framing is whatever files that PR just added, and there is no scope guard distinguishing simplify-on-the-PR from simplify-on-the-worktree. Either gate simplify behind the read-only simplify that #27 had, or document the new mutating behaviour in README.md:143 (which currently still says "refinement only, does not return a review").
There was a problem hiding this comment.
critical · opencode-agent[bot] (replaced placeholder): cp -rn unconditionally merges the bundled .opencode/ into ~/.config/opencode/, with no rm -rf branch for /review-pr runs. #27's REVIEW_ONLY step wiped the global config first and then copied a fresh toolkit, so a reviewed repo's .opencode/config or .opencode/plugins/* was never loaded. That isolation is gone. The malicious-project/.opencode/plugins/pwn.ts fixture that documented this exact path has also been deleted in this PR, removing the regression test that would catch a re-introduction of the bug. The full review body that replaces these placeholder review entries follows in a separate structured review.
| echo "Copied bundled OpenCode config, preserving existing files" | ||
| fi | ||
| mkdir -p "${HOME}/.config/opencode" | ||
| cp -rn "${ACTION_PATH}/.opencode/." "${HOME}/.config/opencode/" |
There was a problem hiding this comment.
critical · opencode-agent[bot]: cp -rn unconditionally merges the bundled .opencode/ into ~/.config/opencode/, with no rm -rf branch for /review-pr runs. #27's REVIEW_ONLY step wiped the global config first and then copied a fresh toolkit, so a reviewed repo's .opencode/config or .opencode/plugins/* was never loaded. That isolation is gone. The malicious-project/.opencode/plugins/pwn.ts fixture that documented this exact path has also been deleted in this PR, removing the regression test that would catch a re-introduction of the bug.
There was a problem hiding this comment.
important · opencode-agent[bot] (replaced placeholder): the Run OpenCode env block no longer passes OPENCODE_DISABLE_PROJECT_CONFIG. #27 set it to 1 for /review-pr runs (if: steps.review_mode.outputs.enabled == 'true'); the revert drops it (this TIMEOUT_MINUTES line is the last env-var entry that survived the diff hunk for the deleted line, hence the anchor). Combined with the unconditional cp -rn two steps above, the reviewed repo's own .opencode/ config is back inside the OpenCode trust boundary, which is the same exposure that produced the #24 incident. If the isolation is intentionally removed, please also drop the line in AGENTS.md that documents the review-only mode so the documentation does not contradict the action. The full review body that replaces these placeholder review entries follows in a separate structured review.
| @@ -139,7 +116,6 @@ runs: | |||
| VARIANT: ${{ inputs.variant }} | |||
| OIDC_BASE_URL: ${{ inputs.oidc-base-url }} | |||
| TIMEOUT_MINUTES: ${{ inputs.timeout-minutes }} | |||
There was a problem hiding this comment.
important · opencode-agent[bot]: the Run OpenCode env block no longer passes OPENCODE_DISABLE_PROJECT_CONFIG. #27 set it to 1 for /review-pr runs (if: steps.review_mode.outputs.enabled == 'true'); the revert drops it (this TIMEOUT_MINUTES line is the last env-var entry that survived the diff hunk for the deleted line, hence the anchor). Combined with the unconditional cp -rn two steps above, the reviewed repo's own .opencode/ config is back inside the OpenCode trust boundary, which is the same exposure that produced the #24 incident. If the isolation is intentionally removed, please also drop the line in AGENTS.md that documents the review-only mode so the documentation does not contradict the action.
Structured PR review submitted to #29 as 4 inline comments plus 4 review bodies ( Findings (4 inline, 5 summary-only):
Summary-only: loss of head-SHA pinning (TOCTOU on review submission), loss of Validation: local QA (validate-opencode.bats, test-resolve-app-token.bats, prettier, yamllint, shellcheck) passes on the reverted tree, so the basic invariants still hold. The Review posted at #29 (review) (and companion reviews 4678566162, 4678566237, 4678566485). |
Reverts #27