feat: feed the shadow PR reviewer the linked issue body + test-runner facts #442

Merged

joshtronic merged 2 commits from agent/438-feed-the-pr-reviewer-the-linked-issue-test-runner into master

2026-07-28 04:54:22 +00:00

igor commented

2026-07-28 04:11:55 +00:00

Collaborator

What this PR does

feat: feed the shadow PR reviewer the linked issue body + test-runner facts
Add lib/review.sh: parses the Closes #NN (or Fixes/Resolves, any inflection) keyword from the PR body, fetches that issue, and folds its title + body into the review prompt (bounded to 4000 chars, truncated-not-blocked; skips gracefully on no match or a failed fetch)
Same module also inspects the unified diff for test-shaped changed files and assembles a "Test-runner facts" section: the repo's Makefile test target recipe (one hop through a delegated prerequisite, e.g. this repo's own test: check-sync), the contents of any .sh script that recipe invokes, package.json's test script, and a presence-only note for pytest.ini/go.mod/Cargo.toml (each ecosystem's convention-based auto-discovery)
review_build_prompt composes the full user-turn prompt (PR metadata + the two new sections, each collapsing to nothing when empty + the unified diff) and is called from do_review_tick in bin/tick.sh, replacing the inline heredoc; confirmed byte-identical output to the pre-#438 prompt shape when a PR closes no issue and touches no test files
No change to bin/lib/review-directive.md -- verdict thresholds and approve criteria are untouched, per the issue's scope discipline
Fence the linked issue's body as untrusted data (--- BEGIN/END UNTRUSTED ISSUE TEXT ---), matching the existing convention in lib/feedback.sh's player-feedback block -- an issue can originate from a lower-trust pipeline (e.g. feedback-triage) than the PR author, and an unfenced body could otherwise try to forge a fake ## Unified diff heading to blur the section boundary in the reviewer's prompt
bin/test-review.sh: unit tests covering the closing-keyword parser, the linked-issue section (fetch, truncation, untrusted-data fencing, graceful skip), the test-file diff filter, the Makefile recipe scraper (incl. the one-hop delegation case), the assembled test-runner-facts section, and an acceptance test asserting both new sections land in review_build_prompt's output for a PR that closes an issue

Test plan

bin/test-review.sh passes (new suite, picked up automatically by bin/check-sync.sh's bin/test-*.sh glob)
make test passes (full suite, all bin/test-*.sh + the AGENTS.md/tick.sh sync check)
make lint passes (shellcheck on bin/ + lib/, mdl clean)

Closes #438

## What this PR does - [x] feat: feed the shadow PR reviewer the linked issue body + test-runner facts - [x] Add `lib/review.sh`: parses the `Closes #NN` (or Fixes/Resolves, any inflection) keyword from the PR body, fetches that issue, and folds its title + body into the review prompt (bounded to 4000 chars, truncated-not-blocked; skips gracefully on no match or a failed fetch) - [x] Same module also inspects the unified diff for test-shaped changed files and assembles a "Test-runner facts" section: the repo's `Makefile` `test` target recipe (one hop through a delegated prerequisite, e.g. this repo's own `test: check-sync`), the contents of any `.sh` script that recipe invokes, `package.json`'s `test` script, and a presence-only note for `pytest.ini`/`go.mod`/`Cargo.toml` (each ecosystem's convention-based auto-discovery) - [x] `review_build_prompt` composes the full user-turn prompt (PR metadata + the two new sections, each collapsing to nothing when empty + the unified diff) and is called from `do_review_tick` in `bin/tick.sh`, replacing the inline heredoc; confirmed byte-identical output to the pre-#438 prompt shape when a PR closes no issue and touches no test files - [x] No change to `bin/lib/review-directive.md` -- verdict thresholds and approve criteria are untouched, per the issue's scope discipline - [x] Fence the linked issue's body as untrusted data (`--- BEGIN/END UNTRUSTED ISSUE TEXT ---`), matching the existing convention in `lib/feedback.sh`'s player-feedback block -- an issue can originate from a lower-trust pipeline (e.g. feedback-triage) than the PR author, and an unfenced body could otherwise try to forge a fake `## Unified diff` heading to blur the section boundary in the reviewer's prompt - [x] `bin/test-review.sh`: unit tests covering the closing-keyword parser, the linked-issue section (fetch, truncation, untrusted-data fencing, graceful skip), the test-file diff filter, the Makefile recipe scraper (incl. the one-hop delegation case), the assembled test-runner-facts section, and an acceptance test asserting both new sections land in `review_build_prompt`'s output for a PR that closes an issue ## Test plan - [x] `bin/test-review.sh` passes (new suite, picked up automatically by `bin/check-sync.sh`'s `bin/test-*.sh` glob) - [x] `make test` passes (full suite, all `bin/test-*.sh` + the AGENTS.md/tick.sh sync check) - [x] `make lint` passes (`shellcheck` on `bin/` + `lib/`, `mdl` clean) Closes #438

igor added 1 commit

2026-07-28 04:11:55 +00:00

WIP: issue #438 checkpoint -- Feed the PR reviewer the linked issue + test-runner facts so it stops punting to the human

Lint / check-sync (pull_request) Successful in 6s

Details

Lint / check-sync (push) Successful in 4s

Details

7ea6465cd0

igor changed title from ~~WIP: issue #438 checkpoint -- Feed the PR reviewer the linked issue + test-runner facts so it stops punting to the human~~ to feat: feed the shadow PR reviewer the linked issue body + test-runner facts

2026-07-28 04:23:09 +00:00

igor added 1 commit

2026-07-28 04:23:10 +00:00

feat: feed the shadow PR reviewer the linked issue body + test-runner facts

Lint / check-sync (push) Successful in 5s

Details

Lint / check-sync (pull_request) Successful in 5s

Details

06c519e664

igor commented

2026-07-28 04:27:42 +00:00

Author

Collaborator

🤖 Review — `COMMENT` (automated)

CI for 06c519e6: success

Adds lib/review.sh (linked-issue text + test-runner facts) and routes do_review_tick's prompt through the new review_build_prompt. Scope is tight, CI is green, and every checked box maps to real diff content — including the byte-identical-plain-prompt claim, which is genuinely asserted in bin/test-review.sh. Not approving because the one thing that determines whether this feature does anything at all is exactly the thing the tests stub out.

Blocking-ish questions (can't resolve from the diff)

forgejo_get_issue / forgejo_repo_get_file signatures are unverified. lib/review.sh:57 and :110 call forgejo_get_issue "$repo" "$number" / forgejo_repo_get_file "$repo" "$path", and bin/test-review.sh overrides both with its own stubs. Every call site is wrapped in || return 0 / 2>/dev/null || true, so if either helper doesn't exist, or takes a different arity (e.g. a required ref arg), the feature degrades to a permanent silent no-op and the test suite still passes green. Please confirm both exist with this arity in lib/forgejo.sh; ideally add a cheap guard to the suite (source lib/forgejo.sh and assert declare -F forgejo_get_issue forgejo_repo_get_file before stubbing) so a future rename is caught.
Which ref does forgejo_repo_get_file read? If it reads the default branch, the Makefile/script contents pasted unfenced into the prompt (lib/review.sh:117-135) are already-merged, trusted content — fine. If it reads the PR head, then a PR can edit its own Makefile/bin/*.sh to inject arbitrary text (including a ``` fence break and a fake ## Unified diff heading) into the prompt of the reviewer that gates auto-merge — the exact attack the issue-body fence at :66 was added to prevent. Worth stating explicitly in the module comment either way.

Non-blocking findings

lib/review.sh:66 — the untrusted fence is a delimiter, not an escape: an issue body containing --- END UNTRUSTED ISSUE TEXT --- breaks out. Matches the existing lib/feedback.sh convention, so this is consistency-preserving, but stripping the marker from $body before interpolation is a one-liner.
lib/review.sh:29 — (close[sd]?|fix(e[sd])?|resolve[sd]?) has no leading word boundary, so prefixes #12 / unfixed #3 false-positive and would splice an unrelated issue into the prompt. (^|[^[:alnum:]]) or \b would tighten it. Also note Closes owner/repo#12 (cross-repo form) doesn't match — graceful, but silently.
lib/review.sh:96 — for prereq in $prereqs is unquoted, so a Makefile prerequisite like $(SOURCES) or *.o gets glob-expanded against the agent's cwd, and $(...) then lands inside grep -E "^${prereq}[[:space:]]*:" where $ and () are regex metacharacters. Worst case is prompt noise rather than a bug, but set -f-guarding the loop or skipping prereqs matching [^A-Za-z0-9_./-] would make it deterministic.
lib/review.sh:16-18 — the log fallback is defined but log is never called anywhere in the module. Dead code.
review_test_runner_facts fires up to ~9 contents-API calls per review tick (Makefile + 3 scripts + package.json + 3 unconditional ecosystem probes) whenever a test-shaped file changes. The three pytest.ini/go.mod/Cargo.toml probes run even in an obviously bash-only repo. Bounded and per-PR, so probably fine — just flagging the added latency on the hot path.

Test coverage notes

The suite is genuinely good — parser, truncation-length assertion via the x-count trick, fence open/close, graceful-skip, the delegation hop, the "no Makefile fabricated" negative, and the byte-identical plain-prompt acceptance test all pull their weight. Two gaps: truncated_note is "" in both review_build_prompt calls (the truncated-diff shape is never exercised), and malformed/non-object JSON from forgejo_get_issue is guarded at :58 but not tested.

_{Independent review by the harness on claude-opus-5 (effort: high). The human reviewer is requested once Igor has reviewed; a human still merges.}

### 🤖 Review — `COMMENT` _(automated)_ CI for `06c519e6`: **success** Adds `lib/review.sh` (linked-issue text + test-runner facts) and routes `do_review_tick`'s prompt through the new `review_build_prompt`. Scope is tight, CI is green, and every checked box maps to real diff content — including the byte-identical-plain-prompt claim, which is genuinely asserted in `bin/test-review.sh`. Not approving because the one thing that determines whether this feature does anything at all is exactly the thing the tests stub out. ## Blocking-ish questions (can't resolve from the diff) - **`forgejo_get_issue` / `forgejo_repo_get_file` signatures are unverified.** `lib/review.sh:57` and `:110` call `forgejo_get_issue "$repo" "$number"` / `forgejo_repo_get_file "$repo" "$path"`, and `bin/test-review.sh` overrides both with its own stubs. Every call site is wrapped in `|| return 0` / `2>/dev/null || true`, so if either helper doesn't exist, or takes a different arity (e.g. a required `ref` arg), the feature degrades to a permanent silent no-op **and the test suite still passes green**. Please confirm both exist with this arity in `lib/forgejo.sh`; ideally add a cheap guard to the suite (source `lib/forgejo.sh` and assert `declare -F forgejo_get_issue forgejo_repo_get_file` before stubbing) so a future rename is caught. - **Which ref does `forgejo_repo_get_file` read?** If it reads the *default branch*, the Makefile/script contents pasted unfenced into the prompt (`lib/review.sh:117-135`) are already-merged, trusted content — fine. If it reads the **PR head**, then a PR can edit its own `Makefile`/`bin/*.sh` to inject arbitrary text (including a ``` fence break and a fake `## Unified diff` heading) into the prompt of the reviewer that gates auto-merge — the exact attack the issue-body fence at `:66` was added to prevent. Worth stating explicitly in the module comment either way. ## Non-blocking findings - `lib/review.sh:66` — the untrusted fence is a delimiter, not an escape: an issue body containing `--- END UNTRUSTED ISSUE TEXT ---` breaks out. Matches the existing `lib/feedback.sh` convention, so this is consistency-preserving, but stripping the marker from `$body` before interpolation is a one-liner. - `lib/review.sh:29` — `(close[sd]?|fix(e[sd])?|resolve[sd]?)` has no leading word boundary, so `prefixes #12` / `unfixed #3` false-positive and would splice an unrelated issue into the prompt. `(^|[^[:alnum:]])` or `\b` would tighten it. Also note `Closes owner/repo#12` (cross-repo form) doesn't match — graceful, but silently. - `lib/review.sh:96` — `for prereq in $prereqs` is unquoted, so a Makefile prerequisite like `$(SOURCES)` or `*.o` gets glob-expanded against the agent's cwd, and `$(...)` then lands inside `grep -E "^${prereq}[[:space:]]*:"` where `$` and `()` are regex metacharacters. Worst case is prompt noise rather than a bug, but `set -f`-guarding the loop or skipping prereqs matching `[^A-Za-z0-9_./-]` would make it deterministic. - `lib/review.sh:16-18` — the `log` fallback is defined but `log` is never called anywhere in the module. Dead code. - `review_test_runner_facts` fires up to ~9 contents-API calls per review tick (Makefile + 3 scripts + package.json + 3 unconditional ecosystem probes) whenever a test-shaped file changes. The three `pytest.ini`/`go.mod`/`Cargo.toml` probes run even in an obviously bash-only repo. Bounded and per-PR, so probably fine — just flagging the added latency on the hot path. ## Test coverage notes The suite is genuinely good — parser, truncation-length assertion via the `x`-count trick, fence open/close, graceful-skip, the delegation hop, the "no Makefile fabricated" negative, and the byte-identical plain-prompt acceptance test all pull their weight. Two gaps: `truncated_note` is `""` in both `review_build_prompt` calls (the truncated-diff shape is never exercised), and malformed/non-object JSON from `forgejo_get_issue` is guarded at `:58` but not tested. </review> --- <sub>Independent review by the harness on `claude-opus-5` (effort: high). The human reviewer is requested once Igor has reviewed; a human still merges.</sub>

igor added spent time

2026-07-28 04:27:43 +00:00

2 minutes 7 seconds

igor requested review from joshtronic

2026-07-28 04:27:43 +00:00