joshtronic/igor

Watch

fix: anchor the closing-keyword match to a word start #445

Merged

joshtronic merged 1 commit from fix/444-closing-keyword-word-boundary into master

2026-07-28 15:19:36 +00:00

igor commented

2026-07-28 05:24:00 +00:00

Collaborator

What this PR does

Follow-up to #442, worked directly rather than queued.

fix: anchor the closing-keyword match to a word start
assert the stubbed Forgejo helpers actually exist before stubbing them
document the default-branch trust model in lib/review.sh

The bug

The alternation had no leading boundary, so any word ending in a closing keyword matched:

prefixes #12   -> 12        suffixes #99  -> 99
postfixes #42  -> 42        unfixed #3    ->  3

Not hypothetical here — "issue prefixes" is standing vocabulary in this fleet's tickets. A false match splices a completely unrelated issue's body into the reviewer's prompt as "the requirements this PR claims to satisfy", which is strictly worse than giving it no issue at all and undermines the one thing #442 exists to do.

The shape that actually matters is prose containing an in-word match earlier in the body than the real keyword:

Some prose about issue prefixes #12 in the nav.

Closes #77

Returned 12 before. Returns 77 now.

Two things folded in

The suite could not have caught a rename. It stubs forgejo_get_issue and forgejo_repo_get_file, and every production call site is guarded with || return 0. So if either helper were renamed or changed arity, the feature would degrade to a permanent silent no-op with the suite still green — the same silent-pass class as igor#430/#431. It now asserts both exist in lib/forgejo.sh before any stub shadows them.

The trust model is now written down. forgejo_repo_get_file reads the default branch (no ref), which is what makes pasting Makefile and script contents into the prompt unfenced safe — they are already-merged content. Passing the PR head so the facts reflect the PR's own changes is a plausible-sounding improvement that would open prompt injection against the reviewer that gates auto-merge: a PR could edit its own Makefile or bin/*.sh to inject a fence break or a fake ## Unified diff heading into the prompt deciding whether to merge it.

I verified the default-branch behaviour empirically rather than trusting the API default: bin/test-review.sh exists only on the #442 branch, and the contents API returns empty without a ref but the blob with ref=<branch>.

Test plan

7 new cases: the four false positives, foreclosed #8, the earlier-in-word-match-vs-real-keyword body, and punctuation before the keyword ((closes #5) still matches)
All pre-existing keyword cases still pass
Verified the new tests fail against the pre-fix regex — 6 failures, clean after
The existence guard passes against real lib/forgejo.sh
make test and make lint pass

Known and deliberately not changed

The untrusted-issue fence is a delimiter, not an escape — an issue body containing the END marker breaks out. That matches the existing lib/feedback.sh convention; fixing it in one place only would make the codebase inconsistent. If it gets fixed, both should change together.

Closes #444

## What this PR does Follow-up to #442, worked directly rather than queued. - [x] fix: anchor the closing-keyword match to a word start - [x] assert the stubbed Forgejo helpers actually exist before stubbing them - [x] document the default-branch trust model in `lib/review.sh` ## The bug The alternation had no leading boundary, so any word **ending** in a closing keyword matched: ``` prefixes #12 -> 12 suffixes #99 -> 99 postfixes #42 -> 42 unfixed #3 -> 3 ``` Not hypothetical here — *"issue prefixes"* is standing vocabulary in this fleet's tickets. A false match splices a completely **unrelated** issue's body into the reviewer's prompt as "the requirements this PR claims to satisfy", which is strictly worse than giving it no issue at all and undermines the one thing #442 exists to do. The shape that actually matters is prose containing an in-word match **earlier** in the body than the real keyword: ``` Some prose about issue prefixes #12 in the nav. Closes #77 ``` Returned **12** before. Returns **77** now. ## Two things folded in **The suite could not have caught a rename.** It stubs `forgejo_get_issue` and `forgejo_repo_get_file`, and every production call site is guarded with `|| return 0`. So if either helper were renamed or changed arity, the feature would degrade to a permanent silent no-op **with the suite still green** — the same silent-pass class as igor#430/#431. It now asserts both exist in `lib/forgejo.sh` before any stub shadows them. **The trust model is now written down.** `forgejo_repo_get_file` reads the **default branch** (no `ref`), which is what makes pasting Makefile and script contents into the prompt unfenced safe — they are already-merged content. Passing the PR head so the facts reflect the PR's own changes is a plausible-sounding improvement that would open **prompt injection against the reviewer that gates auto-merge**: a PR could edit its own `Makefile` or `bin/*.sh` to inject a fence break or a fake `## Unified diff` heading into the prompt deciding whether to merge it. I verified the default-branch behaviour empirically rather than trusting the API default: `bin/test-review.sh` exists only on the #442 branch, and the contents API returns **empty** without a ref but the blob with `ref=<branch>`. ## Test plan - [x] 7 new cases: the four false positives, `foreclosed #8`, the earlier-in-word-match-vs-real-keyword body, and punctuation before the keyword (`(closes #5)` still matches) - [x] All pre-existing keyword cases still pass - [x] **Verified the new tests fail against the pre-fix regex — 6 failures**, clean after - [x] The existence guard passes against real `lib/forgejo.sh` - [x] `make test` and `make lint` pass ## Known and deliberately not changed The untrusted-issue fence is a delimiter, not an escape — an issue body containing the END marker breaks out. That matches the existing `lib/feedback.sh` convention; fixing it in one place only would make the codebase inconsistent. If it gets fixed, both should change together. Closes #444

igor added 1 commit

2026-07-28 05:24:00 +00:00

fix: anchor the closing-keyword match to a word start

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

Details

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

Details

8f51ed2516

Follow-up to #442. The alternation had no leading boundary, so any word
ENDING in a closing keyword matched:

  prefixes #12       -> 12     suffixes #99  -> 99
  postfixes #42      -> 42     unfixed #3    ->  3

Not a curiosity here -- "issue prefixes" is standing vocabulary in this
fleet's tickets. A false match splices a completely UNRELATED issue's body
into the reviewer's prompt as "the requirements this PR claims to satisfy",
which is strictly worse than giving it no issue at all, and undermines the
one thing #442 exists to do.

The case that matters is prose mentioning an in-word match EARLIER in the
body than the real keyword: a body saying "issue prefixes #12 in the nav"
and then "Closes #77" returned 12. It now returns 77.

Also in this change:

- The suite stubs forgejo_get_issue and forgejo_repo_get_file, and every
  production call site is guarded with `|| return 0`. A rename or arity
  change would therefore degrade the feature to a permanent silent no-op
  with the suite still green -- the same silent-pass class as igor#430.
  It now asserts both exist in lib/forgejo.sh BEFORE any stub shadows them.

- Documented the trust model in lib/review.sh. forgejo_repo_get_file reads
  the DEFAULT BRANCH (no `ref`), which is what makes pasting Makefile and
  script contents into the prompt unfenced safe. Passing the PR head so the
  facts reflect the PR's own changes is a plausible-sounding improvement
  that would open prompt injection against the reviewer gating auto-merge:
  a PR could edit its own Makefile to inject a fence break or a fake
  section heading. Verified empirically (a file present only on a PR branch
  returns empty) rather than assumed from the API default, and written down
  so the next person doesn't "fix" it.

Verified the new tests fail against the pre-fix regex: 6 failures, clean
after.

Closes #444

igor commented

2026-07-28 05:32:38 +00:00

Author

Collaborator

🤖 Review — `APPROVE` (automated)

CI for 8f51ed25: success

Anchors the closing-keyword regex in lib/review.sh with a leading (^|[^[:alnum:]]), adds 7 regression cases, an existence guard for the stubbed Forgejo helpers, and a trust-model comment. Focused, honestly described, CI green — approving.

Correctness of the fix

lib/review.sh:review_closed_issue_number — the new leading group can never match a digit ([^[:alnum:]] excludes 0-9), so the downstream grep -oE '[0-9]+' still extracts the issue number and not a stray leading char. ^ under grep -oiE is per-line, so the multi-line "real keyword wins" case works as claimed. No regression in the existing first-match-wins behavior: grep resumes scanning after the consumed match, and the following keyword still has its own separator available.
Each of the five false positives (prefixes/suffixes/postfixes/unfixed/foreclosed) is preceded by an alnum char, so they now correctly return empty; (closes #5) still matches. That's 6 of the 7 new cases failing pre-fix, matching the description's stated "6 failures" — the checklist is consistent with the diff.

Nice confirmation of the bug, for the human's benefit

The "Linked issue" block in this very review prompt is issue #12 ("tier 2 skips bot-owned repos") — completely unrelated to this PR, spliced in because the currently deployed pre-fix regex matched prefixes #12 in the PR body. That's live evidence the bug is real, not a curiosity. Not a defect of the diff.

Nits / follow-ups (non-blocking)

bin/test-review.sh:20-21 — the comment claims the guard asserts the helpers exist "with the arity lib/review.sh calls them with". declare -F checks existence only; arity drift is not caught. The PR description states this accurately ("asserts both exist"); the in-code comment overstates it and could mislead a future maintainer. Suggest dropping the arity clause or actually checking it.
Residual fragility, out of scope: first-match-wins still scans fenced code blocks, so a PR body that quotes an example like Closes #77 before its real Closes #444 will still resolve to the quoted number (this PR's own body has exactly that shape). Worth a separate ticket if it bites; the fix here is a strict improvement regardless.
[^[:alnum:]] admits _, so a hypothetical pre_fixes #12 would still match. Vanishingly unlikely in prose; noting only for completeness.

Scope / safety

Two extra items beyond the titular fix (existence guard, trust-model comment) are disclosed up front, each <20 lines, and both live in the module under change — acceptable creep, not a drive-by refactor. Total diff ~60 lines.
No workflow files touched. The trust-model note in lib/review.sh correctly documents why the default-branch read must not be "improved" to the PR head; that's a real prompt-injection vector against the merge gate and is worth having in-tree.
The existence check runs in a subshell, so sourcing lib/forgejo.sh can't leak real helpers into the suite and shadow the later stubs.

_{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 — `APPROVE` _(automated)_ CI for `8f51ed25`: **success** Anchors the closing-keyword regex in `lib/review.sh` with a leading `(^|[^[:alnum:]])`, adds 7 regression cases, an existence guard for the stubbed Forgejo helpers, and a trust-model comment. Focused, honestly described, CI green — approving. **Correctness of the fix** - `lib/review.sh:review_closed_issue_number` — the new leading group can never match a digit (`[^[:alnum:]]` excludes `0-9`), so the downstream `grep -oE '[0-9]+'` still extracts the issue number and not a stray leading char. `^` under `grep -oiE` is per-line, so the multi-line "real keyword wins" case works as claimed. No regression in the existing first-match-wins behavior: grep resumes scanning after the consumed match, and the following keyword still has its own separator available. - Each of the five false positives (`prefixes`/`suffixes`/`postfixes`/`unfixed`/`foreclosed`) is preceded by an alnum char, so they now correctly return empty; `(closes #5)` still matches. That's 6 of the 7 new cases failing pre-fix, matching the description's stated "6 failures" — the checklist is consistent with the diff. **Nice confirmation of the bug, for the human's benefit** - The "Linked issue" block in this very review prompt is issue **#12** ("tier 2 skips bot-owned repos") — completely unrelated to this PR, spliced in because the *currently deployed* pre-fix regex matched `prefixes #12` in the PR body. That's live evidence the bug is real, not a curiosity. Not a defect of the diff. **Nits / follow-ups (non-blocking)** - `bin/test-review.sh:20-21` — the comment claims the guard asserts the helpers exist "with the arity `lib/review.sh` calls them with". `declare -F` checks existence only; arity drift is *not* caught. The PR description states this accurately ("asserts both exist"); the in-code comment overstates it and could mislead a future maintainer. Suggest dropping the arity clause or actually checking it. - Residual fragility, out of scope: first-match-wins still scans fenced code blocks, so a PR body that *quotes* an example like ``Closes #77`` before its real `Closes #444` will still resolve to the quoted number (this PR's own body has exactly that shape). Worth a separate ticket if it bites; the fix here is a strict improvement regardless. - `[^[:alnum:]]` admits `_`, so a hypothetical `pre_fixes #12` would still match. Vanishingly unlikely in prose; noting only for completeness. **Scope / safety** - Two extra items beyond the titular fix (existence guard, trust-model comment) are disclosed up front, each <20 lines, and both live in the module under change — acceptable creep, not a drive-by refactor. Total diff ~60 lines. - No workflow files touched. The trust-model note in `lib/review.sh` correctly documents *why* the default-branch read must not be "improved" to the PR head; that's a real prompt-injection vector against the merge gate and is worth having in-tree. - The existence check runs in a subshell, so sourcing `lib/forgejo.sh` can't leak real helpers into the suite and shadow the later stubs. --- <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 05:32:38 +00:00

1 minute 18 seconds