feat(forgejo-opencode-review): Submit review verdicts in Forgejo #52

martin · 2026-10-10T11:53:39Z

martin commented

2026-10-10 11:53:39 +00:00

Summary

Map OpenCode approve and changes verdicts to Forgejo APPROVE and REQUEST_CHANGES review events.
Submit verdict reviews even when there are no inline comments; retain COMMENT reviews for new inline findings.
Update plugin documentation and add API/event mapping test coverage.

Verification

cargo test -p forgejo-opencode-review
cargo clippy -p forgejo-opencode-review -- -D warnings
cargo fmt --check
git diff --check

## Summary - Map OpenCode `approve` and `changes` verdicts to Forgejo `APPROVE` and `REQUEST_CHANGES` review events. - Submit verdict reviews even when there are no inline comments; retain `COMMENT` reviews for new inline findings. - Update plugin documentation and add API/event mapping test coverage. ## Verification - `cargo test -p forgejo-opencode-review` - `cargo clippy -p forgejo-opencode-review -- -D warnings` - `cargo fmt --check` - `git diff --check`

martin added 1 commit

2026-10-10 11:53:39 +00:00

feat(forgejo-opencode-review): Mark approval using API

ci/woodpecker/pr/global-rust-verify Pipeline was successful

Details

ci/woodpecker/pr/pr-review Pipeline was successful

Details

e420e3ec4d

martin requested review from clank-bot

2026-10-10 11:53:39 +00:00

clank-bot commented

2026-10-10 12:17:11 +00:00

Review Summary

Core logic looks correct: review_event() maps approve/changes to APPROVE/REQUEST_CHANGES case-insensitively, submitting an event with an empty comment list is valid for Forgejo, a failed review submission only logs a warning (consistent with the rest of the plugin), the new unit test covers the mapping, and README step 11 matches the new flow.

But the integration tests never reach the code they now claim to verify:

wiremock serves the first mounted mock whose matchers all pass, and the GET .../pulls/42/files mock registered without a page matcher returns []. It therefore answers the page=1 request, so the plugin exits at the filtered.is_empty() check before running opencode or POSTing anything. The expected_review mock added in this PR — and the summary expectation edited here — are never exercised; these tests would pass even if review_event always returned None. Same applies to integration_posts_summary_and_inline_review. Suggested fix: mount the page=1 mock before the empty one (or add a page=1 query matcher to it) and assert with server.verify() / .expect(1).
After that fix, the summary-body expectation becomes the next failure: summary_body appends a trailing \n\n that the exact-match expectation omits.

Other notes:

README (touched by this PR) still documents a diff-position map and hallucinated line -> file-level handling (diagram + step 12) that the code does not implement — c.line goes straight to new_position/old_position, and file-level fallback only happens when line is None.
Security consideration: the prompt embeds PR title/body/discussion authored by the PR submitter, and this PR makes the model's verdict a formal APPROVE/REQUEST_CHANGES that can gate merges — submitter-controlled text can now steer merge-blocking state. Consider an opt-in setting for submitting APPROVE, or document that the bot's approval must not be counted as a trusted review.

*🤖 Review by opencode (opencode/mimo-v2.6-flash-free) — verdict: changes

# Review Summary **Core logic looks correct:** `review_event()` maps `approve`/`changes` to `APPROVE`/`REQUEST_CHANGES` case-insensitively, submitting an event with an empty comment list is valid for Forgejo, a failed review submission only logs a warning (consistent with the rest of the plugin), the new unit test covers the mapping, and README step 11 matches the new flow. **But the integration tests never reach the code they now claim to verify:** - wiremock serves the first mounted mock whose matchers all pass, and the `GET .../pulls/42/files` mock registered without a `page` matcher returns `[]`. It therefore answers the `page=1` request, so the plugin exits at the `filtered.is_empty()` check before running opencode or POSTing anything. The `expected_review` mock added in this PR — and the summary expectation edited here — are never exercised; these tests would pass even if `review_event` always returned `None`. Same applies to `integration_posts_summary_and_inline_review`. Suggested fix: mount the `page=1` mock before the empty one (or add a `page=1` query matcher to it) and assert with `server.verify()` / `.expect(1)`. - After that fix, the summary-body expectation becomes the next failure: `summary_body` appends a trailing `\n\n` that the exact-match expectation omits. **Other notes:** - README (touched by this PR) still documents a diff-position map and `hallucinated line -> file-level` handling (diagram + step 12) that the code does not implement — `c.line` goes straight to `new_position`/`old_position`, and file-level fallback only happens when `line` is `None`. - Security consideration: the prompt embeds PR title/body/discussion authored by the PR submitter, and this PR makes the model's verdict a formal `APPROVE`/`REQUEST_CHANGES` that can gate merges — submitter-controlled text can now steer merge-blocking state. Consider an opt-in setting for submitting `APPROVE`, or document that the bot's approval must not be counted as a trusted review. --- *🤖 Review by opencode (opencode/mimo-v2.6-flash-free) — verdict: **changes**

clank-bot reviewed

2026-10-10 12:17:11 +00:00

clank-bot left a comment

Code review findings from opencode.

Code review findings from opencode.

plugins/forgejo-opencode-review/README.md

					
				@ -89,0 +84,4 @@

				    alt Verdict is approve or changes

				        Plugin->>Forgejo: POST /pulls/{index}/reviews (APPROVE or REQUEST_CHANGES)

				    else Has new inline comments

				        Plugin->>Plugin: Build diff position map and drop duplicate findings

[style] Docs still describe a diff position map (`(file, new_file_line) -> diff_position`), position lookups, and `hallucinated lines attach at file level` (diagram here + step 12 bullets), but the implementation passes `c.line` straight to `new_position`/`old_position` with no diff parsing, and file-level fallback only occurs when `line` is `None`. Update the README to match the code (or implement the mapping).

[style] Docs still describe a diff position map (`(file, new_file_line) -> diff_position`), position lookups, and `hallucinated lines attach at file level` (diagram here + step 12 bullets), but the implementation passes `c.line` straight to `new_position`/`old_position` with no diff parsing, and file-level fallback only occurs when `line` is `None`. Update the README to match the code (or implement the mapping).

plugins/forgejo-opencode-review/src/main.rs

					
				@ -204,3 +213,2 @@

				            .collect();

				        if !new_comments.is_empty() {

				            info!(count = new_comments.len(), "posting new inline comments");

				        let review_event = review_event(&parsed.verdict, !new_comments.is_empty());

[security] The verdict now becomes a formal APPROVE/REQUEST_CHANGES review that can gate merges, but the prompt includes PR title/body/discussion authored by the PR submitter (`load_pr_context`), so submitter-controlled text can steer the model into approving changes — or griefing with REQUEST_CHANGES. Consider an opt-in setting for submitting `APPROVE` (defaulting to COMMENT-only), or document that the bot's approval must not be counted as a trusted human review.

[security] The verdict now becomes a formal APPROVE/REQUEST_CHANGES review that can gate merges, but the prompt includes PR title/body/discussion authored by the PR submitter (`load_pr_context`), so submitter-controlled text can steer the model into approving changes — or griefing with REQUEST_CHANGES. Consider an opt-in setting for submitting `APPROVE` (defaulting to COMMENT-only), or document that the bot's approval must not be counted as a trusted human review.

This will be used in a closed environment so its not a problem

martin marked this conversation as resolved

plugins/forgejo-opencode-review/src/main.rs

					
				@ -702,3 +726,3 @@

				        let expected_body: serde_json::Value = match serde_json::from_str(

				            r##"{"body": "# Review Summary\n\nLooks good.\n\n---\n*🤖 Review by opencode (anthropic/claude-sonnet-4.5) — verdict: **comment**"}"##,

				            r##"{"body": "# Review Summary\n\nLooks good.\n\n---\n*🤖 Review by opencode (anthropic/claude-sonnet-4.5) — verdict: **approve**"}"##,

[bug] Once the test actually reaches this POST, this exact-match expectation will fail: `summary_body` (main.rs:181-184) appends `\n\n` after the verdict line, which this expected body omits. It has never failed only because the test returns early before posting.

[bug] Once the test actually reaches this POST, this exact-match expectation will fail: `summary_body` (main.rs:181-184) appends `\n\n` after the verdict line, which this expected body omits. It has never failed only because the test returns early before posting.

plugins/forgejo-opencode-review/src/main.rs

					
				@ -719,6 +743,24 @@ mod tests {

				            )

				            .await;

				        let expected_review = serde_json::json!({

[bug] This new mock (and the summary-body mock above it) is never exercised. wiremock picks the first mounted mock whose matchers all pass, and the earlier `GET .../pulls/42/files` mock at line 708 has no `page` matcher, so it answers the page=1 request with `[]`. The plugin then exits at `filtered.is_empty()` before running opencode or POSTing anything — this test passes even if `review_event` always returns `None` (same for `integration_posts_summary_and_inline_review`). Mount the page=1 mock before the empty one (or add `query_param("page", "1")` to it) and assert the calls with `server.verify()` / `.expect(1)` so the APPROVE submission is actually verified.

[bug] This new mock (and the summary-body mock above it) is never exercised. wiremock picks the first mounted mock whose matchers all pass, and the earlier `GET .../pulls/42/files` mock at line 708 has no `page` matcher, so it answers the page=1 request with `[]`. The plugin then exits at `filtered.is_empty()` before running opencode or POSTing anything — this test passes even if `review_event` always returns `None` (same for `integration_posts_summary_and_inline_review`). Mount the page=1 mock before the empty one (or add `query_param("page", "1")` to it) and assert the calls with `server.verify()` / `.expect(1)` so the APPROVE submission is actually verified.

martin self-assigned this

2026-10-10 12:34:02 +00:00

ci/woodpecker/pr/global-rust-verify Pipeline was successful

Details

ci/woodpecker/pr/pr-review Pipeline was successful

Details

This pull request can be merged automatically.

You are not authorized to merge this pull request.

View command line instructions

Checkout

From your project repository, check out a new branch and test the changes.

git fetch -u origin forgejo-opencode-review/approve-or-changes:forgejo-opencode-review/approve-or-changes

git switch forgejo-opencode-review/approve-or-changes

Merge

Merge the changes and update on Forgejo.

Warning: The "Autodetect manual merge" setting is not enabled for this repository, you will have to mark this pull request as manually merged afterwards.

git switch main

git merge --no-ff forgejo-opencode-review/approve-or-changes

git switch forgejo-opencode-review/approve-or-changes

git rebase main

git switch main

git merge --ff-only forgejo-opencode-review/approve-or-changes

git switch forgejo-opencode-review/approve-or-changes

git rebase main

git switch main

git merge --no-ff forgejo-opencode-review/approve-or-changes

git switch main

git merge --squash forgejo-opencode-review/approve-or-changes