Fix effective permissions for inline review submission

[dceoy]

Fixes #34.

What changed

• Allow the runtime $HOME/.config/opencode/review-state/* directory pattern in the global OpenCode config.
• Repeat the trusted helper and review-state allow-list in review-pr-orchestrator, because agent-level permissions override the global rules.
• Allow only the two review payload files through both OpenCode edit-path forms: the expanded $HOME path and the worktree-relative path used by the write tool.
• Exercise the actual review-pr-orchestrator write path with OpenCode, including a negative assertion for unrelated external files.
• Install the minimum supported OpenCode version in the Bats CI job so the runtime regression cannot be skipped.

Root cause

The structured review payload write crossed three permission checks that the previous configuration and tests did not model correctly:

1. OpenCode evaluates external-file access using the parent directory glob $HOME/.config/opencode/review-state/*, which did not match the previous individual-file external_directory entries.
2. review-pr-orchestrator declares permission."*": deny. Agent rules take precedence over global rules, so the agent-level wildcard denied external_directory even after the global directory wildcard was added.
3. After external-directory validation, OpenCode evaluates edit against a path relative to the worktree. The existing absolute $HOME/.../initial.json and update.json rules therefore did not match on the Linux Actions runner.

As a result, initial.json was not created, review-pr-submit.sh submit-initial was never reached, and opencode github run published the model output as a top-level comment while the workflow still concluded successfully.

Validation

• Bats installs OpenCode 1.2.14 and invokes the actual review-pr-orchestrator write tool.
• The positive case requires creation of review-state/initial.json.
• The negative case requires an unrelated external write to remain denied.
• Existing shell, workflow, security, YAML, and CodeQL checks remain enabled.

[dceoy]

Blocking findings:

1. The new Bats test only asserts that a literal review-state/* entry exists. It does not exercise OpenCode's runtime permission evaluation or prove that the orchestrator can now write initial.json, which is the behavior that failed silently in #34. Add a focused integration/regression test that runs the relevant OpenCode permission/write path (or an equivalent executable permission-evaluation test), so the test fails when structured payload creation is denied.
2. .opencode/commands/review-pr.md still states that the three fixed review-state JSON files are the sole allow-listed external paths. That is no longer accurate after allowing the entire directory wildcard. Update this security-boundary instruction to match the actual configuration while retaining the agent-level edit restriction to initial.json and update.json.

CI is green, and I found no issue with the wildcard ordering or the existing agent-level edit constraints. GitHub does not allow requesting changes on one's own PR, so this is submitted as a comment review but should be treated as blocking.

[opencode-agent]

I have completed my analysis. Unfortunately, I cannot submit this review through the structured review process (initial.json write is denied by the runtime permission system — the very issue this PR aims to fix). I will present my findings here instead.

---

Review of PR #35: "fix inline review submission"

Summary

The PR correctly identifies the root cause (directory-wildcard access pattern vs. individual file-path allow-listing) and the three documentation/config changes are accurate. However, I found three actionable issues in the new regression test.

---

Finding 1 (critical) — --pure may bypass the permission it intends to validate

File: .agents/skills/local-qa/scripts/validate-opencode.bats, line 152–153
Test: "OpenCode permits writing the installed runtime initial review payload"

The test invokes:

opencode debug agent build --pure --tool write --params ...

The --pure flag is not documented or justified in the test. If --pure runs the agent in a sandbox that ignores OPENCODE_CONFIG and/or skips external_directory permission enforcement, then the test would pass even when the review-state/* wildcard is missing from the config — the exact regression it is meant to prevent.

Without a comment or citation explaining that --pure does not bypass permission checks (and ideally a reference to the OpenCode source that confirms this), a future reader has no way to know this test is valid.

Fix: Either:

• Remove --pure so the agent loads the config normally, or
• Add a non-pure variant that loads the config from OPENCODE_CONFIG, or
• Add an inline comment documenting why --pure is safe (citing the source/docs).

---

Finding 2 (important) — No negative test case

File: .agents/skills/local-qa/scripts/validate-opencode.bats, line 140 (the test as a whole)

The new regression test only validates the positive case — a write to an allow-listed path succeeds. It does not verify that writes to non-allow-listed external paths are correctly denied.

Without a negative test, a future change that accidentally disables the entire external_directory permission system (e.g., changing "*": "deny" to "*": "allow", or a code change that bypasses permission evaluation entirely) would not be caught. The test suite would still pass despite the security boundary being wide open.

Fix: Add a second assertion that attempts to write a file outside the allow-list (e.g., ${config_home}/.config/opencode/unauthorized.txt) and expects a non-zero exit status or a denial message.

---

Finding 3 (suggestion) — local output shadows Bats' global $output

File: .agents/skills/local-qa/scripts/validate-opencode.bats, line 141

local config_home state_dir payload output

The local output declaration shadows the global $output variable that Bats' run function sets. While bash's dynamic scoping may make this work in current Bats versions (because run does not declare local output internally), it is against the convention established by every other test in this repository (none declare output, status, or lines as local). It could cause incorrect test behavior with different Bats versions or configurations.

Fix: Remove output from the local declaration on line 141:

local config_home state_dir payload

---

Meta-observation: Permission evaluation order issue

I was unable to submit this review through the structured process because the runtime denied write access to $HOME/.config/opencode/review-state/initial.json despite the review-state/* wildcard being present in the external_directory allow rules. The root cause appears to be that the catch-all "*": "ask" rule is evaluated before the specific review-state/*: allow rule, causing the permission to default to "deny" in non-interactive mode regardless of the wildcard pattern. This suggests the fix in this PR may be necessary but not sufficient — the permission evaluation order (or the "*": "ask" catch-all) may need attention as well. This is worth verifying after the PR is merged and deployed.

github run

[dceoy]

PR Review Summary

Important Issues

• [MEDIUM][Confidence: HIGH][behavioral test coverage] The new executable permission regression currently fails on the reviewed head — .agents/skills/local-qa/scripts/validate-opencode.bats:139

  • Impact: The PR’s primary claim is that OpenCode can write the structured review payload while denying unrelated external writes, but the current-head bats-test / test job fails in the newly added runtime regression. The intended fix therefore is not verified and the branch does not satisfy its own required validation.
  • Recommendation: Inspect the failing Bats output, correct the test invocation/configuration or the permission rules as appropriate, and require the runtime regression to pass on OpenCode 1.2.14 before merging.

Checked

• Effective external_directory rules in the global config and review-pr-orchestrator agent.
• The agent-level edit restriction for initial.json and update.json.
• Updated command and README descriptions of the review-state directory boundary.
• The executable positive and negative permission regression added to validate-opencode.bats.
• Current-head CI for fb4d03b13e94d4470594ba0e9ce3e88df771a186: shell lint, workflow/security scans, CodeQL, and YAML lint pass; bats-test / test fails.
• Existing review discussion; the earlier missing-runtime-test and stale-documentation findings are addressed by the current diff.

Recommended Action

Do not merge until the current-head Bats regression passes and demonstrates the intended structured-review write path.

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: b30b675cc8

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

[opencode-agent]

OpenCode PR Review: 4 inline finding(s). Review submitted via structured review process.

[opencode-agent]

The structured review was submitted successfully. The review is now live at #35 (review) as opencode-agent[bot].

Four inline findings were submitted, covering the three actionable issues found in the new Bats regression test:

1. critical — Missing --tool edit test coverage for the separate edit: permission section
2. important — Missing mktemp -d failure guard could cause unclear test failures
3. important — Only initial.json is tested; update.json is not verified
4. important — Cleanup (rm -rf) runs before assertions, destroying forensic evidence on test failure

github run

[dceoy]

No blocking findings on the current head.

The effective permission chain is now covered at the relevant boundaries: the runtime review-state directory is allowed for external_directory, the orchestrator repeats that allow-list despite its agent-level catch-all deny, and edit remains restricted to initial.json and update.json through both path forms observed on the runner.

The new Bats case executes the actual OpenCode 1.2.14 review-pr-orchestrator write path, verifies initial.json is created, and confirms an unrelated external write remains denied. The current PR workflow also successfully produced inline review comments, providing end-to-end evidence that the structured submission path no longer falls back as described in #34.

The unresolved requests for an additional edit invocation, an update.json case, stronger mktemp handling, and retaining temporary files on assertion failure are not merge blockers. OpenCode's write tool is governed by the edit permission, the issue's failing path is the initial payload, and the remaining items are incremental test-hardening rather than correctness gaps.