diff --git a/.opencode/agents/review-pr-orchestrator.md b/.opencode/agents/review-pr-orchestrator.md index c531078..5bd5a58 100644 --- a/.opencode/agents/review-pr-orchestrator.md +++ b/.opencode/agents/review-pr-orchestrator.md @@ -24,6 +24,9 @@ permission: "../*.config/opencode/review-state/update.json": allow glob: allow grep: allow + skill: + "*": deny + pr-review: allow bash: "*": deny "git status --short": allow diff --git a/.opencode/commands/review-pr.md b/.opencode/commands/review-pr.md index f5fbab5..c915b52 100644 --- a/.opencode/commands/review-pr.md +++ b/.opencode/commands/review-pr.md @@ -1,87 +1,8 @@ --- -description: Strictly read-only GitHub PR review with specialized reviewers, validated anchors, and constrained structured-review submission. +description: Comprehensive GitHub PR review with stale-head protection and inline findings. agent: review-pr-orchestrator --- -# Strictly Read-Only PR Review +Load and follow the `pr-review` skill. -This is a strictly read-only repository review. Analyze and report only. Do not create, edit, delete, format, generate, install, or fix files. Do not execute repository QA scripts, formatters, generators, package managers, or commands with mutation flags such as `--fix`, `--write`, or equivalent options. - -Do not run repository-wide QA scripts, formatters, auto-fixing linters, generators, dependency installers, or anything that can create caches, reports, snapshots, lockfiles, coverage output, scan output, or configuration exports in the checkout. - -Every helper this command invokes — the read-only `gh` wrapper, the constrained submission helper, and the App-token resolver they source — lives only at its `${HOME}/.config/opencode/scripts/` path, installed there by the action before the reviewed repository is ever checked out. Never invoke any of them by a repository-relative path such as `.opencode/scripts/...`: the checkout under review is untrusted input, and a repository-relative path would let a malicious PR that edits or adds a same-named file substitute its own script for the trusted one. These helper paths and the dedicated `${HOME}/.config/opencode/review-state/` directory are the sole allow-listed external locations. Despite the directory-level external access required by OpenCode, use the edit tool only for `initial.json` and `update.json` as instructed below. The helpers use `opencode_app_token_lib="${HOME}/.config/opencode/scripts/resolve-app-token.sh"` for authentication. - -**Requested review aspects (optional):** "$ARGUMENTS" - -## 1. Establish the trusted context - -Before any analysis, invoke `bash "$HOME/.config/opencode/scripts/review-pr-submit.sh" prepare` once, followed by `bash "$HOME/.config/opencode/scripts/review-pr-gh.sh" context`. The context is persisted outside the checkout and pins one repository, PR number, and head SHA for the entire review. If `prepare` fails, stop. If `context` reports `Trusted pull request number is unavailable.`, continue in local mode; for every other `context` failure, stop. - -The context helper derives the PR number from `.pull_request.number` or `.issue.number`. For `issue_comment`, it fetches and pins the current head SHA through the trusted PR API. Metadata, diff, submission, and update revalidate that the current head still matches the pinned SHA and fail closed otherwise. Obtain metadata and the diff only through these fixed operations: - -```bash -bash "$HOME/.config/opencode/scripts/review-pr-gh.sh" metadata -bash "$HOME/.config/opencode/scripts/review-pr-gh.sh" diff -``` - -If no PR context can be established, use local mode: `git status --short`, `git diff --name-only HEAD`, and `git diff --no-ext-diff`; do not infer a PR from the current branch. Once `context` succeeds, any later metadata, diff, or validation failure must abort the review rather than falling back to local mode. - -Capture the full diff, changed-file list, PR title/body, base and head branch names, head SHA, and relevant source context using the read, glob, and grep tools. Pass that complete context to reviewers; they have no shell access and must not need it. - -## 2. Select and launch reviewers - -Supported aspects are: - -- `code`: `code-reviewer`, `code-quality-reviewer` -- `quality`: `code-quality-reviewer` -- `performance`: `performance-reviewer` -- `security`: `security-code-reviewer` -- `tests` or `coverage`: `test-coverage-reviewer`, `pr-test-analyzer` -- `docs` or `documentation`: `documentation-accuracy-reviewer` -- `comments`: `comment-analyzer` -- `errors`: `silent-failure-hunter` -- `types`: `type-design-analyzer` -- `simplify`: `code-simplifier`, returning behavior-preserving simplification proposals as review findings without modifying files -- `all`, or no aspect: the core reviewers `code-quality-reviewer`, `performance-reviewer`, `test-coverage-reviewer`, `documentation-accuracy-reviewer`, `security-code-reviewer`, and `code-reviewer`; include specialty reviewers when the supplied diff is relevant. Run `code-simplifier` only when `simplify` is explicitly requested; never include it in `all`. - -Launch only the explicitly permitted reviewer agents. For each, supply the captured diff, changed-file list, metadata, and relevant source context. Tell each reviewer to inspect changed lines and their containing functions only, return high-confidence findings only, and use: - -```yaml -- file: path/to/file - line: - severity: critical | important | suggestion - source: - message: |- - -``` - -Do not let a reviewer post to GitHub. - -## 3. Normalize and anchor findings - -Drop praise, nitpicks, style-only feedback, findings outside the changed-file list, and duplicates. Keep the most specific actionable finding for each root cause. Classify every remaining finding as inline when its file and head-side changed line can be anchored in the captured diff; adjust only to a nearby relevant changed line. When a finding's own reported line is not itself the changed line used for its anchor, strip any `suggestion` block from its message before submission: GitHub would apply the block to the moved anchor rather than the line the finding actually describes. Put genuine but unanchorable findings in `summary_only` with a short reason. - -Before returning any top-level text in PR mode, including no-finding and summary-only fallback results, invoke `bash "$HOME/.config/opencode/scripts/review-pr-gh.sh" validate`. If validation fails, stop. If there are no findings, then return exactly `No noteworthy issues found.` Do not post an empty review. - -For findings, the `prepare` and `context` operations in section 1 have already created the empty payload files and pinned the review context. Do not run them again. Use the edit tool only for `$HOME/.config/opencode/review-state/initial.json`, writing exactly `{body, comments}` with a nonempty body and inline comments array. The helper validates the payload and adds the trusted `commit_id` and `event` itself. Preserve each finding message's Markdown, including paragraph breaks and fenced code or `suggestion` blocks, except for a `suggestion` block already stripped in section 3 for a relocated anchor. Each inline body is `** · **`, followed by a blank line and the unmodified finding message. - -Every finding with a valid diff anchor must be included in the `comments` array and submitted as an inline review comment. Never return anchorable findings only as top-level assistant text. If structured submission fails, fail the run instead of emitting the findings as a top-level completion comment. - -When there are summary-only findings, the body begins `OpenCode PR Review: inline finding(s), summary-only finding(s).` and lists them. Otherwise it begins `OpenCode PR Review: inline finding(s).` Never use issue comments or `gh pr comment`. - -## 4. Submit through the constrained helper - -Use only these exact commands: - -```bash -bash "$HOME/.config/opencode/scripts/review-pr-submit.sh" submit-initial -bash "$HOME/.config/opencode/scripts/review-pr-submit.sh" update -``` - -After the single `prepare` in section 1, write the initial payload only to `$HOME/.config/opencode/review-state/initial.json`. Before `update`, write exactly `{body}` only to `$HOME/.config/opencode/review-state/update.json`. Never add arguments, redirections, pipelines, or process substitutions to helper commands. - -You never pass a repository, PR number, target commit, or review ID: the helper derives the repository and PR number from the trusted GitHub Actions context, pins the write to the head commit from the same context, and updates only the review ID it recorded when the initial submission succeeded in this run. It validates the trusted event context, temporary payload, target commit, HTTP method, and exact pull-request-review endpoint. It sources the existing App-token resolver and calls `opencode_require_app_token_for_review` immediately before its permitted POST or PUT. This preserves verified `opencode-agent[bot]` attribution when available, preserves the explicit `use-github-token: true` fallback, and never accepts an unverified candidate for a write. - -After successful inline submission, do not repeat findings in the final assistant output. Update the submitted review with final status and the run URL when available; the helper targets the review it recorded, so no review ID is passed. If GitHub rejects inline anchors, retry once only after converting the identified invalid anchors to summary-only; never lose a finding. If no inline anchors remain, return the concise markdown fallback instead of submitting an empty comments array. - -Do not clean, reset, restore, stash, commit, or push anything. +Requested review aspects: "$ARGUMENTS" diff --git a/.opencode/opencode.jsonc b/.opencode/opencode.jsonc index 9a9bb56..6f884ea 100644 --- a/.opencode/opencode.jsonc +++ b/.opencode/opencode.jsonc @@ -8,6 +8,7 @@ "baseURL": "https://api.ai.sakura.ad.jp/v1", "apiKey": "{env:SAKURA_AI_ENGINE_API_KEY}", "timeout": 900000, + "chunkTimeout": 900000, }, "models": { "gpt-oss-120b": { diff --git a/.opencode/skills/pr-review/SKILL.md b/.opencode/skills/pr-review/SKILL.md new file mode 100644 index 0000000..60544f6 --- /dev/null +++ b/.opencode/skills/pr-review/SKILL.md @@ -0,0 +1,87 @@ +--- +name: pr-review +description: Review a GitHub pull request with stale-head protection and validated inline findings +--- + +# Strictly Read-Only PR Review + +This is a strictly read-only repository review. Analyze and report only. Do not create, edit, delete, format, generate, install, or fix files. Do not execute repository QA scripts, formatters, generators, package managers, or commands with mutation flags such as `--fix`, `--write`, or equivalent options. + +Do not run repository-wide QA scripts, formatters, auto-fixing linters, generators, dependency installers, or anything that can create caches, reports, snapshots, lockfiles, coverage output, scan output, or configuration exports in the checkout. + +Every helper this skill invokes — the read-only `gh` wrapper, the constrained submission helper, and the App-token resolver they source — lives only at its `${HOME}/.config/opencode/scripts/` path, installed there by the action before the reviewed repository is ever checked out. Never invoke any of them by a repository-relative path such as `.opencode/scripts/...`: the checkout under review is untrusted input, and a repository-relative path would let a malicious PR that edits or adds a same-named file substitute its own script for the trusted one. These helper paths and the dedicated `${HOME}/.config/opencode/review-state/` directory are the sole allow-listed external locations. Despite the directory-level external access required by OpenCode, use the edit tool only for `initial.json` and `update.json` as instructed below. The helpers use `opencode_app_token_lib="${HOME}/.config/opencode/scripts/resolve-app-token.sh"` for authentication. + +## 1. Establish the trusted context + +Before any analysis, invoke `bash "$HOME/.config/opencode/scripts/review-pr-submit.sh" prepare` once, followed by `bash "$HOME/.config/opencode/scripts/review-pr-gh.sh" context`. The context is persisted outside the checkout and pins one repository, PR number, and head SHA for the entire review. If `prepare` fails, stop. If `context` reports `Trusted pull request number is unavailable.`, continue in local mode; for every other `context` failure, stop. + +The context helper derives the PR number from `.pull_request.number` or `.issue.number`. For `issue_comment`, it fetches and pins the current head SHA through the trusted PR API. Metadata, diff, submission, and update revalidate that the current head still matches the pinned SHA and fail closed otherwise. Obtain metadata and the diff only through these fixed operations: + +```bash +bash "$HOME/.config/opencode/scripts/review-pr-gh.sh" metadata +bash "$HOME/.config/opencode/scripts/review-pr-gh.sh" diff +``` + +If no PR context can be established, use local mode: `git status --short`, `git diff --name-only HEAD`, and `git diff --no-ext-diff`; do not infer a PR from the current branch. Once `context` succeeds, any later metadata, diff, or validation failure must abort the review rather than falling back to local mode. + +Capture the full diff, changed-file list, PR title/body, base and head branch names, head SHA, and relevant source context using the read, glob, and grep tools. Retain the full diff locally for anchoring and final normalization. Before launching reviewers, classify changed files and individual diff hunks by concern. For each concern, collect only the changed files, hunks, and containing-function source context needed to review it; exclude unchanged files, unrelated hunks, and unrelated full-file contents. + +## 2. Select and launch reviewers + +Explicit aspects select these reviewers: + +- `code`: `code-reviewer`, `code-quality-reviewer` +- `quality`: `code-quality-reviewer` +- `performance`: `performance-reviewer` +- `security`: `security-code-reviewer` +- `tests` or `coverage`: `test-coverage-reviewer`, `pr-test-analyzer` +- `docs` or `documentation`: `documentation-accuracy-reviewer` +- `comments`: `comment-analyzer` +- `errors`: `silent-failure-hunter` +- `types`: `type-design-analyzer` +- `simplify`: `code-simplifier`, returning behavior-preserving simplification proposals as review findings without modifying files +- `all`, or no aspect: the core reviewers `code-quality-reviewer`, `performance-reviewer`, `test-coverage-reviewer`, `documentation-accuracy-reviewer`, `security-code-reviewer`, and `code-reviewer`; include specialty reviewers when the supplied diff is relevant. Run `code-simplifier` only when `simplify` is explicitly requested; never include it in `all`. + +Requested aspects always force their mapped reviewers. + +Build a separate, minimal Task request for every selected reviewer. Include only its relevant files, diff hunks, and containing-function source context, plus only the metadata needed for that specialty. Exclude unchanged files and unrelated hunks. `code-reviewer` may receive the complete changed-file list, but do not include unrelated full-file contents. Reviewers have no shell access, so each subset must be self-contained. Tell each reviewer to inspect changed lines and their containing functions only, return high-confidence findings only, and use: + +```yaml +- file: path/to/file + line: + severity: critical | important | suggestion + source: + message: |- + +``` + +Do not let a reviewer post to GitHub. + +## 3. Normalize and anchor findings + +Drop praise, nitpicks, style-only feedback, findings outside the changed-file list, and duplicates. Keep the most specific actionable finding for each root cause. Classify every remaining finding as inline when its file and head-side changed line can be anchored in the captured diff; adjust only to a nearby relevant changed line. When a finding's own reported line is not itself the changed line used for its anchor, strip any `suggestion` block from its message before submission: GitHub would apply the block to the moved anchor rather than the line the finding actually describes. Put genuine but unanchorable findings in `summary_only` with a short reason. + +Before returning any top-level text in PR mode, including no-finding and summary-only fallback results, invoke `bash "$HOME/.config/opencode/scripts/review-pr-gh.sh" validate`. If validation fails, stop. If there are no findings, then return exactly `No noteworthy issues found.` Do not post an empty review. + +For findings, the `prepare` and `context` operations in section 1 have already created the empty payload files and pinned the review context. Do not run them again. Use the edit tool only for `$HOME/.config/opencode/review-state/initial.json`, writing exactly `{body, comments}` with a nonempty body and inline comments array. The helper validates the payload and adds the trusted `commit_id` and `event` itself. Preserve each finding message's Markdown, including paragraph breaks and fenced code or `suggestion` blocks, except for a `suggestion` block already stripped in section 3 for a relocated anchor. Each inline body is `** · **`, followed by a blank line and the unmodified finding message. + +Every finding with a valid diff anchor must be included in the `comments` array and submitted as an inline review comment. Never return anchorable findings only as top-level assistant text. If structured submission fails, fail the run instead of emitting the findings as a top-level completion comment. + +When there are summary-only findings, the body begins `OpenCode PR Review: inline finding(s), summary-only finding(s).` and lists them. Otherwise it begins `OpenCode PR Review: inline finding(s).` Never use issue comments or `gh pr comment`. + +## 4. Submit through the constrained helper + +Use only these exact commands: + +```bash +bash "$HOME/.config/opencode/scripts/review-pr-submit.sh" submit-initial +bash "$HOME/.config/opencode/scripts/review-pr-submit.sh" update +``` + +After the single `prepare` in section 1, write the initial payload only to `$HOME/.config/opencode/review-state/initial.json`. Before `update`, write exactly `{body}` only to `$HOME/.config/opencode/review-state/update.json`. Never add arguments, redirections, pipelines, or process substitutions to helper commands. + +You never pass a repository, PR number, target commit, or review ID: the helper derives the repository and PR number from the trusted GitHub Actions context, pins the write to the head commit from the same context, and updates only the review ID it recorded when the initial submission succeeded in this run. It validates the trusted event context, temporary payload, target commit, HTTP method, and exact pull-request-review endpoint. It sources the existing App-token resolver and calls `opencode_require_app_token_for_review` immediately before its permitted POST or PUT. This preserves verified `opencode-agent[bot]` attribution when available, preserves the explicit `use-github-token: true` fallback, and never accepts an unverified candidate for a write. + +After successful inline submission, do not repeat findings in the final assistant output. Update the submitted review with final status and the run URL when available; the helper targets the review it recorded, so no review ID is passed. If GitHub rejects inline anchors, retry once only after converting the identified invalid anchors to summary-only; never lose a finding. If no inline anchors remain, return the concise markdown fallback instead of submitting an empty comments array. + +Do not clean, reset, restore, stash, commit, or push anything. diff --git a/README.md b/README.md index 3c51f89..6d9b5f7 100644 --- a/README.md +++ b/README.md @@ -75,19 +75,19 @@ The provider account must have sufficient credits or quota. For providers not bu ## Inputs -| Input | Default | Description | -| --------------------- | ------------------------- | ------------------------------------------------------------- | -| `model` | Required | Model in `provider/model` format. | -| `agent` | `build` | Primary agent. A slash command can override it. | -| `prompt` | Event comment | Fixed prompt to use instead of the triggering comment. | -| `mentions` | `/opencode,/oc` | Comma-separated trigger phrases. | -| `variant` | - | Provider-specific reasoning effort. | -| `share` | `false` | Share the OpenCode session. | -| `use-github-token` | `false` | Use the workflow token instead of the default App-token flow. | -| `opencode-version` | `latest` | OpenCode version to install. `/review-pr` requires 1.2.14+. | -| `use-bundled-toolkit` | `true` | Use the bundled agents, commands, skills, and configuration. | -| `timeout-minutes` | `60` | Stop OpenCode after this many minutes. | -| `oidc-base-url` | `https://api.opencode.ai` | OIDC exchange URL for a custom GitHub App installation. | +| Input | Default | Description | +| --------------------- | ------------------------- | ----------------------------------------------------------------------------------------------------------------------------------------------------------------------- | +| `model` | Required | Model in `provider/model` format. | +| `agent` | `build` | Primary agent. A slash command can override it. | +| `prompt` | Event comment | Fixed prompt to use instead of the triggering comment. | +| `mentions` | `/opencode,/oc` | Comma-separated trigger phrases. | +| `variant` | - | Provider-specific reasoning effort. | +| `share` | `false` | Share the OpenCode session. | +| `use-github-token` | `false` | Use the workflow token instead of the default App-token flow. | +| `opencode-version` | `latest` | OpenCode version to install. `/review-pr` requires 1.2.14+; the bundled Sakura provider's `chunkTimeout` needs 1.2.25+ (older pins fall back to the request `timeout`). | +| `use-bundled-toolkit` | `true` | Use the bundled agents, commands, skills, and configuration. | +| `timeout-minutes` | `60` | Stop OpenCode after this many minutes. | +| `oidc-base-url` | `https://api.opencode.ai` | OIDC exchange URL for a custom GitHub App installation. | When `use-github-token: true`, keep `GITHUB_TOKEN` in `env` and grant only the permissions needed for the task. @@ -95,6 +95,8 @@ Outputs are `opencode-version` and `cache-hit`. `cache-hit` is empty on review-o ## Pull request reviews -Set `prompt: /review-pr` to run the bundled read-only pull request review workflow. Findings are deduplicated, validated against the diff, and posted inline when they can be anchored to changed lines. +Set `prompt: /review-pr` to run the bundled read-only `pr-review` skill through its backward-compatible command wrapper. Findings are deduplicated, validated against the diff, and posted inline when they can be anchored to changed lines. Agents can also load the skill directly through OpenCode's native skill tool, but only `/review-pr` carries the read-only guarantees; see [Pull request reviews](docs/pull-request-reviews.md#review-isolation). + +The default review runs the core quality, performance, coverage, documentation, security, and correctness reviewers; specialty reviewers beyond that set are added only when the diff matches their documented concern or an aspect such as `security`, `tests`, `docs`, or `performance` explicitly requests them. Provider request and chunk timeouts and the action's `timeout-minutes` watchdog are safety limits; they do not replace bounded request context or guarantee that a provider gateway or inference request will remain open. See [Pull request reviews](docs/pull-request-reviews.md) for setup, supported review aspects, submission behavior, and security guarantees. diff --git a/docs/custom-providers.md b/docs/custom-providers.md index 5b53811..61c039f 100644 --- a/docs/custom-providers.md +++ b/docs/custom-providers.md @@ -66,7 +66,8 @@ Sakura AI Engine exposes an OpenAI-compatible `/v1/chat/completions` endpoint. A "options": { "baseURL": "https://api.ai.sakura.ad.jp/v1", "apiKey": "{env:SAKURA_AI_ENGINE_API_KEY}", - "timeout": 900000 + "timeout": 900000, + "chunkTimeout": 900000 }, "models": { "gpt-oss-120b": { diff --git a/docs/pull-request-reviews.md b/docs/pull-request-reviews.md index 838a77e..891aa91 100644 --- a/docs/pull-request-reviews.md +++ b/docs/pull-request-reviews.md @@ -1,10 +1,12 @@ # Pull request reviews -The bundled `/review-pr` command runs a read-only, multi-agent review and submits validated findings through GitHub's pull request review API. +The bundled `pr-review` skill runs a read-only, multi-agent review and submits validated findings through GitHub's pull request review API. The `/review-pr` command remains a thin wrapper that loads the skill and forwards any requested review aspects. + +Agents can also load `pr-review` directly through OpenCode's native skill tool, but only `/review-pr` carries the read-only guarantees below: those come from `review-pr-orchestrator`'s `permission` config (denying edit and unrestricted `bash`), which only applies when the command routes to that agent. Loading the skill directly injects the same instructions into whatever agent calls it, and that agent's own permissions still apply, so the read-only behavior is advisory rather than enforced. ## Setup -Review workflows require OpenCode 1.2.14 or newer, `use-bundled-toolkit: true`, `pull-requests: write`, and an API key for the selected model provider. +Review workflows require OpenCode 1.2.14 or newer, `use-bundled-toolkit: true`, `pull-requests: write`, and an API key for the selected model provider. The bundled Sakura provider's `chunkTimeout` setting requires OpenCode 1.2.25 or newer; pins between 1.2.14 and 1.2.24 fall back to the top-level request `timeout` instead of the inter-chunk timeout. Request, chunk, and action timeouts are safety limits, not substitutes for bounding request size, and `chunkTimeout` cannot guarantee that a provider-side gateway or inference timeout will not end a request sooner. ```yaml permissions: @@ -42,11 +44,11 @@ Use one or more keywords after `/review-pr`: | `/review-pr types` | Type design | | `/review-pr simplify` | Read-only simplification suggestions | -A full review runs the core quality, performance, coverage, documentation, security, and correctness reviewers. Specialty reviewers are added when relevant to the diff. The simplifier runs only when explicitly requested. +A full review runs the core quality, performance, coverage, documentation, security, and correctness reviewers. Specialty reviewers are added when relevant to the diff. Explicit aspects always force their mapped reviewers; for example, `security` forces the security reviewer and `tests` forces both test reviewers. The simplifier runs only when explicitly requested. ## Finding and submission behavior -The orchestrator receives the pull request metadata, changed-file list, diff, and relevant source context. It then: +The orchestrator retains the full pull request context for anchoring and normalization, but classifies files and hunks before delegation. Each reviewer receives only its relevant diff subset and containing-function context. The orchestrator then: 1. keeps only high-confidence, actionable findings on changed files 2. removes style-only feedback and duplicates @@ -66,7 +68,7 @@ If no finding can be anchored, the command returns a concise Markdown fallback i ### Review isolation -When the effective prompt starts with `/review-pr`, the action installs a fresh bundled OpenCode configuration, disables project-provided configuration, removes inherited plugins and agents, and resolves the review command only from the action bundle. +When the effective prompt starts with `/review-pr`, the action installs a fresh bundled OpenCode configuration, disables project-provided configuration and externally discovered skills, removes inherited plugins and agents, and resolves the review command only from the action bundle. External-directory access is denied by default. Only the trusted review helpers and their dedicated state directory under `~/.config/opencode/` are allowed. Review-only mode does not modify the checkout, run mutating repository commands, or allow reviewer agents to post directly to GitHub. diff --git a/scripts/run-opencode.sh b/scripts/run-opencode.sh index 0b36dfa..acf8c5b 100755 --- a/scripts/run-opencode.sh +++ b/scripts/run-opencode.sh @@ -15,11 +15,26 @@ opencode_select_timeout_command() { } opencode_report_failure() { - local status="${1}" output_file="${2}" timeout_minutes="${3}" model="${4:-unknown}" + local status="${1}" output_file="${2}" timeout_minutes="${3}" model="${4:-unknown}" terminal_error if [[ "${status}" -eq 124 ]]; then echo "::error::OpenCode timed out after ${timeout_minutes} minutes for model '${model:-unknown}'." - elif grep -Eiq 'AI_APICallError|Insufficient credits|statusCode:[[:space:]]*402|"code":[[:space:]]*402' "${output_file}"; then - echo "::error::OpenCode failed with a model provider API error for model '${model:-unknown}'. Check provider credentials, quota, and billing." + return + fi + + terminal_error="$( + grep -Ei \ + 'Request timed out|SSE read timed out|TimeoutError|AI_APICallError|Insufficient credits|rate[ -]?limit|HTTP[^[:digit:]]*(402|429)|status(Code)?[^[:digit:]]*(402|429)|"code"[^[:digit:]]*(402|429)' \ + "${output_file}" | tail -n 1 || true + )" + + if grep -Eiq 'Request timed out|SSE read timed out|TimeoutError' <<<"${terminal_error}"; then + echo "::error::OpenCode provider request timed out for model '${model:-unknown}'." + elif grep -Eiq 'rate[ -]?limit|HTTP[^[:digit:]]*429|status(Code)?[^[:digit:]]*429|"code"[^[:digit:]]*429' <<<"${terminal_error}"; then + echo "::error::OpenCode failed because the model provider rate limited the request (HTTP 429) for model '${model:-unknown}'." + elif grep -Eiq 'Insufficient credits|HTTP[^[:digit:]]*402|status(Code)?[^[:digit:]]*402|"code"[^[:digit:]]*402' <<<"${terminal_error}"; then + echo "::error::OpenCode failed because of model provider billing or quota (HTTP 402 or insufficient credits) for model '${model:-unknown}'." + elif grep -Eiq 'AI_APICallError' <<<"${terminal_error}"; then + echo "::error::OpenCode failed with a model provider API error for model '${model:-unknown}'. Check provider credentials and service status." else echo "::error::OpenCode failed with exit code ${status} for model '${model:-unknown}'." fi @@ -33,6 +48,11 @@ opencode_configure_run() { if [[ "${REVIEW_ONLY:-false}" == "true" ]]; then export XDG_CONFIG_HOME="${HOME}/.config" export OPENCODE_DISABLE_PROJECT_CONFIG=1 + # OPENCODE_DISABLE_PROJECT_CONFIG does not stop OpenCode's separate + # discovery of project-level .claude/skills/**/SKILL.md and + # .agents/skills/**/SKILL.md; only this flag does. Without it, a PR could + # add a same-named external skill alongside the trusted bundled one. + export OPENCODE_DISABLE_EXTERNAL_SKILLS=1 # Composite steps inherit caller env. Remove every explicit config # override before constructing the action's trusted inline config. unset OPENCODE_CONFIG OPENCODE_CONFIG_DIR OPENCODE_CONFIG_CONTENT diff --git a/tests/test-run-opencode.bats b/tests/test-run-opencode.bats index 38448a2..20f2b72 100644 --- a/tests/test-run-opencode.bats +++ b/tests/test-run-opencode.bats @@ -84,6 +84,7 @@ setup() { [[ -z "${OPENCODE_CONFIG+x}" ]] [[ -z "${OPENCODE_CONFIG_DIR+x}" ]] [[ "$OPENCODE_DISABLE_PROJECT_CONFIG" == 1 ]] + [[ "$OPENCODE_DISABLE_EXTERNAL_SKILLS" == 1 ]] [[ "$XDG_CONFIG_HOME" == "$HOME/.config" ]] jq -e '\'' .default_agent == "build" and @@ -119,7 +120,8 @@ EOF source "$1" opencode_configure_run [[ "$OPENCODE_RESOLVED_COMMAND_FILE" == "$2/.opencode/commands/review-pr.md" ]] - [[ "$PROMPT" == *"Strictly Read-Only PR Review"* ]] + [[ "$PROMPT" == *"Load and follow the "*" skill."* ]] + [[ "$PROMPT" == *"pr-review"* ]] [[ "$PROMPT" == *"security"* ]] [[ "$PROMPT" != *"MALICIOUS PROJECT REVIEW"* ]] jq -e '\'' @@ -131,6 +133,51 @@ EOF [ "${status}" -eq 0 ] } +@test "review-only runtime loads the bundled skill and excludes external skills" { + workspace="${BATS_TEST_TMPDIR}/workspace" + mkdir -p "${workspace}/.agents/skills/untrusted-review" + cat >"${workspace}/.agents/skills/untrusted-review/SKILL.md" <<'EOF' +--- +name: untrusted-review +description: untrusted project skill +--- + +# Untrusted review +EOF + + run env \ + HOME="${fake_home}" \ + ACTION_PATH="${repo_root}" \ + GITHUB_WORKSPACE="${workspace}" \ + PROMPT="/review-pr security" \ + AGENT="build" \ + MENTIONS="/oc" \ + REVIEW_ONLY="true" \ + USE_BUNDLED_TOOLKIT="true" \ + bash -euo pipefail -c ' + source "$1" + source "$2" + opencode_prepare_config "$3" true + opencode_configure_run + cd "$GITHUB_WORKSPACE" + opencode debug skill >/dev/null + skills="$(opencode debug skill)" + jq -e \ + --arg location "$HOME/.config/opencode/skills/pr-review/SKILL.md" \ + '\'' + any( + .[]; + .name == "pr-review" + and .location == $location + and (.content | contains("# Strictly Read-Only PR Review")) + ) + and all(.[]; .name != "untrusted-review") + '\'' <<<"$skills" + ' _ "${run_script}" "${repo_root}/scripts/prepare-opencode-config.sh" "${repo_root}" + + [ "${status}" -eq 0 ] +} + @test "normal run configuration falls back to the bundled toolkit's commands" { workspace="${BATS_TEST_TMPDIR}/workspace" mkdir -p "${workspace}" "${fake_action}/.opencode/commands" @@ -163,18 +210,20 @@ EOF [ "${status}" -eq 0 ] } -@test "OpenCode failure classification distinguishes timeout provider and generic errors" { +@test "OpenCode failure classification uses the terminal provider error" { output_file="${BATS_TEST_TMPDIR}/output" - printf '%s\n' 'AI_APICallError: statusCode: 402' >"${output_file}" + printf '%s\n' \ + 'AI_APICallError: rate limit exceeded (statusCode: 429)' \ + 'UnknownError: "Request timed out"' >"${output_file}" run bash -euo pipefail -c ' source "$1" opencode_report_failure 1 "$2" 10 provider/model ' _ "${run_script}" "${output_file}" [ "${status}" -eq 0 ] - [[ "${output}" == *"model provider API error"* ]] + [[ "${output}" == *"provider request timed out"* ]] + [[ "${output}" != *"rate limited"* ]] - printf '%s\n' unrelated >"${output_file}" run bash -euo pipefail -c ' source "$1" opencode_report_failure 124 "$2" 10 provider/model @@ -182,6 +231,47 @@ EOF [ "${status}" -eq 0 ] [[ "${output}" == *"timed out after 10 minutes"* ]] + printf '%s\n' 'Error: SSE read timed out' >"${output_file}" + run bash -euo pipefail -c ' + source "$1" + opencode_report_failure 1 "$2" 10 provider/model + ' _ "${run_script}" "${output_file}" + [ "${status}" -eq 0 ] + [[ "${output}" == *"provider request timed out"* ]] + + printf '%s\n' 'TimeoutError: The operation timed out' >"${output_file}" + run bash -euo pipefail -c ' + source "$1" + opencode_report_failure 1 "$2" 10 provider/model + ' _ "${run_script}" "${output_file}" + [ "${status}" -eq 0 ] + [[ "${output}" == *"provider request timed out"* ]] + + printf '%s\n' 'AI_APICallError: statusCode: 429' >"${output_file}" + run bash -euo pipefail -c ' + source "$1" + opencode_report_failure 1 "$2" 10 provider/model + ' _ "${run_script}" "${output_file}" + [ "${status}" -eq 0 ] + [[ "${output}" == *"rate limited"* ]] + + printf '%s\n' 'AI_APICallError: Insufficient credits (statusCode: 402)' >"${output_file}" + run bash -euo pipefail -c ' + source "$1" + opencode_report_failure 1 "$2" 10 provider/model + ' _ "${run_script}" "${output_file}" + [ "${status}" -eq 0 ] + [[ "${output}" == *"billing or quota"* ]] + + printf '%s\n' 'AI_APICallError: provider unavailable' >"${output_file}" + run bash -euo pipefail -c ' + source "$1" + opencode_report_failure 1 "$2" 10 provider/model + ' _ "${run_script}" "${output_file}" + [ "${status}" -eq 0 ] + [[ "${output}" == *"model provider API error"* ]] + + printf '%s\n' unrelated >"${output_file}" run bash -euo pipefail -c ' source "$1" opencode_report_failure 17 "$2" 10 provider/model @@ -224,7 +314,7 @@ EOF printf 'unexpected status %s: %s\n' "${status}" "${output}" >&2 fi [ "${status}" -eq 23 ] - [[ "${output}" == *"model provider API error"* ]] + [[ "${output}" == *"billing or quota"* ]] [ "$(cat "${invocation_file}")" = "github run" ] } diff --git a/tests/validate-opencode.bats b/tests/validate-opencode.bats index df881ad..6d70b9d 100644 --- a/tests/validate-opencode.bats +++ b/tests/validate-opencode.bats @@ -1,18 +1,19 @@ #!/usr/bin/env bats -# Validate .opencode/ agent frontmatter, review-pr command/skill references, -# that opencode.jsonc parses, and that its external_directory permission -# allow-lists the resolver path review-pr.md actually sources and the runtime -# review-state directory pattern. +# Validate .opencode/ agent and skill frontmatter, pr-review references, that +# opencode.jsonc parses, and that its external_directory permission allow-lists +# the resolver path the skill actually sources and the runtime review-state +# directory pattern. setup() { repo_root="$(git -C "${BATS_TEST_DIRNAME}" rev-parse --show-toplevel)" agents_dir="${repo_root}/.opencode/agents" - review_pr_doc="${repo_root}/.opencode/commands/review-pr.md" + review_pr_command="${repo_root}/.opencode/commands/review-pr.md" + review_pr_skill="${repo_root}/.opencode/skills/pr-review/SKILL.md" opencode_jsonc="${repo_root}/.opencode/opencode.jsonc" # shellcheck source=scripts/opencode-action-lib.sh source "${repo_root}/scripts/opencode-action-lib.sh" required_keys=(name description mode permission) - # Backtick-quoted identifiers in review-pr.md that are skills, toolkits, or + # Backtick-quoted identifiers in the pr-review skill that are skills, toolkits, or # config inputs rather than agents. non_agents=(pr-feedback-triage pr-review-toolkit use-github-token) } @@ -68,11 +69,80 @@ frontmatter() { } } -@test "every agent referenced in review-pr.md exists under .opencode/agents/" { +@test "pr-review skill has valid frontmatter" { + local fm name description + + fm="$(frontmatter "${review_pr_skill}")" + name="$(grep -E '^name:' <<<"${fm}" | sed -E 's/^name:[[:space:]]*//')" + description="$(grep -E '^description:' <<<"${fm}" | sed -E 's/^description:[[:space:]]*//')" + + [ "${name}" = "pr-review" ] + [ -n "${description}" ] +} + +@test "review-pr command is a thin wrapper around the pr-review skill" { + local body + + body="$(awk ' + NR == 1 && $0 == "---" { in_frontmatter = 1; next } + in_frontmatter && $0 == "---" { in_frontmatter = 0; next } + !in_frontmatter && NF { print } + ' "${review_pr_command}")" + + [ "${body}" = $'Load and follow the `pr-review` skill.\nRequested review aspects: "$ARGUMENTS"' ] +} + +@test "review-pr orchestrator may load only the pr-review skill" { + local orchestrator="${agents_dir}/review-pr-orchestrator.md" + + grep -Fq ' skill:' "${orchestrator}" + grep -Fq ' "*": deny' "${orchestrator}" + grep -Fq ' pr-review: allow' "${orchestrator}" +} + +@test "review-pr default selection names the core six reviewers" { + local reviewer + + # shellcheck disable=SC2016 + grep -Fq 'the core reviewers `code-quality-reviewer`, `performance-reviewer`, `test-coverage-reviewer`, `documentation-accuracy-reviewer`, `security-code-reviewer`, and `code-reviewer`' "${review_pr_skill}" + grep -Fq 'include specialty reviewers when the supplied diff is relevant' "${review_pr_skill}" + + for reviewer in \ + code-quality-reviewer \ + performance-reviewer \ + test-coverage-reviewer \ + documentation-accuracy-reviewer \ + security-code-reviewer \ + code-reviewer; do + grep -Fq "${reviewer}" "${review_pr_skill}" + done +} + +@test "review-pr explicit core aspects force their documented reviewers" { + # shellcheck disable=SC2016 + grep -Fq -- '- `performance`: `performance-reviewer`' "${review_pr_skill}" + # shellcheck disable=SC2016 + grep -Fq -- '- `security`: `security-code-reviewer`' "${review_pr_skill}" + # shellcheck disable=SC2016 + grep -Fq -- '- `tests` or `coverage`: `test-coverage-reviewer`, `pr-test-analyzer`' "${review_pr_skill}" + # shellcheck disable=SC2016 + grep -Fq -- '- `docs` or `documentation`: `documentation-accuracy-reviewer`' "${review_pr_skill}" + grep -Fq 'Requested aspects always force their mapped reviewers.' "${review_pr_skill}" +} + +@test "review-pr sends reviewer-specific context subsets" { + grep -Fq 'classify changed files and individual diff hunks by concern' "${review_pr_skill}" + grep -Fq 'Build a separate, minimal Task request for every selected reviewer.' "${review_pr_skill}" + grep -Fq 'Include only its relevant files, diff hunks, and containing-function source context' "${review_pr_skill}" + # shellcheck disable=SC2016 + grep -Fq '`code-reviewer` may receive the complete changed-file list, but do not include unrelated full-file contents.' "${review_pr_skill}" +} + +@test "every agent referenced in the pr-review skill exists under .opencode/agents/" { local bt pattern refs ref skip na missing=() bt=$(printf '\x60') pattern="${bt}[a-z][a-z0-9]+(-[a-z0-9]+)+${bt}" - mapfile -t refs < <(grep -hoE "${pattern}" "${review_pr_doc}" | tr -d "${bt}" | sort -u) + mapfile -t refs < <(grep -hoE "${pattern}" "${review_pr_skill}" | tr -d "${bt}" | sort -u) for ref in "${refs[@]}"; do skip=0 @@ -95,9 +165,9 @@ frontmatter() { @test "review-pr local fallback is limited to a missing trusted PR number" { # shellcheck disable=SC2016 - grep -Fq 'If `context` reports `Trusted pull request number is unavailable.`, continue in local mode; for every other `context` failure, stop.' "${review_pr_doc}" + grep -Fq 'If `context` reports `Trusted pull request number is unavailable.`, continue in local mode; for every other `context` failure, stop.' "${review_pr_skill}" # shellcheck disable=SC2016 - grep -Fq 'Once `context` succeeds, any later metadata, diff, or validation failure must abort the review rather than falling back to local mode.' "${review_pr_doc}" + grep -Fq 'Once `context` succeeds, any later metadata, diff, or validation failure must abort the review rather than falling back to local mode.' "${review_pr_skill}" } @test "code-quality findings retain rich actionable Markdown" { @@ -108,10 +178,10 @@ frontmatter() { grep -Fq 'why it matters to users or maintainers' "${quality_reviewer}" grep -Fq '' "${quality_reviewer}" grep -Fq '```suggestion' "${quality_reviewer}" - grep -Fq "Preserve each finding message's Markdown" "${review_pr_doc}" - grep -Fq 'followed by a blank line and the unmodified finding message' "${review_pr_doc}" - grep -Fq 'message: |-' "${review_pr_doc}" - grep -Fq '' "${review_pr_doc}" + grep -Fq "Preserve each finding message's Markdown" "${review_pr_skill}" + grep -Fq 'followed by a blank line and the unmodified finding message' "${review_pr_skill}" + grep -Fq 'message: |-' "${review_pr_skill}" + grep -Fq '' "${review_pr_skill}" } @test "suggestion blocks are withheld or stripped for relocated anchors" { @@ -119,19 +189,19 @@ frontmatter() { grep -Fq 'reported line is not itself a head-side changed' "${quality_reviewer}" # shellcheck disable=SC2016 - grep -Fq 'strip any `suggestion` block from its message before submission' "${review_pr_doc}" + grep -Fq 'strip any `suggestion` block from its message before submission' "${review_pr_skill}" } opencode_jsonc_json() { opencode_jsonc_to_json < "${opencode_jsonc}" } -@test "review-pr.md sources the resolver from a path opencode.jsonc allow-lists under external_directory" { +@test "pr-review skill sources the resolver from a path opencode.jsonc allow-lists under external_directory" { local resolver_suffix resolver_path default_action allow_patterns pattern expanded matched=0 - resolver_suffix="$(grep -oE 'opencode_app_token_lib="\$\{HOME\}/[^"]+"' "${review_pr_doc}" | head -1 | sed -E 's/^opencode_app_token_lib="\$\{HOME\}\/(.*)"$/\1/')" + resolver_suffix="$(grep -oE 'opencode_app_token_lib="\$\{HOME\}/[^"]+"' "${review_pr_skill}" | head -1 | sed -E 's/^opencode_app_token_lib="\$\{HOME\}\/(.*)"$/\1/')" [ -n "${resolver_suffix}" ] || { - echo "review-pr.md does not set opencode_app_token_lib to a \${HOME}-relative path" + echo "pr-review skill does not set opencode_app_token_lib to a \${HOME}-relative path" return 1 } resolver_path="${HOME}/${resolver_suffix}" @@ -156,7 +226,7 @@ opencode_jsonc_json() { done [ "${matched}" -eq 1 ] || { - echo "no external_directory allow pattern (${allow_patterns[*]}) matches the resolver path ${resolver_path} that review-pr.md sources" + echo "no external_directory allow pattern (${allow_patterns[*]}) matches the resolver path ${resolver_path} that the pr-review skill sources" return 1 } }