Follow-up to #442: closing-keyword regex matches inside words; guard the stubbed helpers #444

New issue

Closed

opened 2026-07-28 04:54:32 +00:00 by igor · 0 comments

igor commented

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

Collaborator

Follow-up to #442 (merged). Three items, all decided — build them, no deliberation needed.

1. The closing-keyword regex has no word boundary (real bug)

lib/review.sh extracts the linked issue with:

grep -oiE '(close[sd]?|fix(e[sd])?|resolve[sd]?)[[:space:]]+#[0-9]+'

No leading boundary, so any word ENDING in one of those matches. Measured:

Closes #12         -> 12   correct
prefixes #12       -> 12   WRONG
unfixed #3         ->  3   WRONG
this suffixes #99  -> 99   WRONG
postfixes #42      -> 42   WRONG
see also #5        -> none correct

This is not hypothetical for this fleet — "issue prefixes" is standing vocabulary here (the persona convention uses it). When it fires, the reviewer is handed a completely UNRELATED issue's body as "the requirements this PR claims to satisfy", which is worse than giving it no issue at all and directly undercuts what #442 was for.

Decision: anchor the alternation to a word start — (^|[^[:alnum:]]) before the group, or \b if you keep GNU grep. Add the four false-positive strings above as test cases; the current suite has none of them.

2. The test suite stubs the Forgejo helpers without checking they exist

bin/test-review.sh overrides forgejo_get_issue and forgejo_repo_get_file with its own stubs. Every production call site is wrapped in || return 0 / 2>/dev/null || true, so if either helper is renamed or changes arity, the feature degrades to a permanent silent no-op and the suite still passes green.

I verified both exist today with the arity lib/review.sh uses (forgejo_get_issue "$repo" "$number", forgejo_repo_get_file "$repo" "$path"), so this is about the future, not a live defect.

Decision: source lib/forgejo.sh and assert declare -F forgejo_get_issue forgejo_repo_get_file BEFORE installing the stubs. Same class of silent-pass hole as igor#430/#431.

3. Document the trust model in lib/review.sh (prevents a future security regression)

The shadow reviewer's review of #442 asked which ref forgejo_repo_get_file reads, because the answer decides whether a PR can inject text into the prompt of the reviewer that gates its own auto-merge. Verified empirically:

forgejo_repo_get_file calls /repos/{repo}/contents/{path} with no ref -> the DEFAULT BRANCH.
Proof: bin/test-review.sh exists only on the #442 branch. The contents API returns empty without a ref, and the blob only with ref=<branch>.

So today a PR cannot edit its own Makefile/bin/*.sh to inject into the reviewer's prompt. That is the correct and safe behaviour, and it is currently load-bearing but undocumented.

Decision: state it in the module comment — this deliberately reads the default branch, and adding a ref= to read the PR head would let a PR inject arbitrary text (including fence breaks and fake section headings) into the prompt of the reviewer gating its merge. A future "bug fix" to make the test-runner facts reflect the PR's own changes is exactly how that vector would get opened by someone acting in good faith.

Known and accepted, NOT in scope here: the untrusted-issue fence is a delimiter rather than an escape, so an issue body containing the END marker breaks out. That matches the existing lib/feedback.sh convention; changing it in one place only would make the codebase inconsistent. If it gets fixed, fix both together.

Acceptance criteria

The four false-positive strings above extract no issue number; Closes #12 / Fixes #7 still extract theirs.
The suite fails if forgejo_get_issue or forgejo_repo_get_file stops existing.
lib/review.sh documents the default-branch read and why changing it is a security decision.

Follow-up to #442 (merged). Three items, all decided — build them, no deliberation needed. ## 1. The closing-keyword regex has no word boundary (real bug) `lib/review.sh` extracts the linked issue with: ```sh grep -oiE '(close[sd]?|fix(e[sd])?|resolve[sd]?)[[:space:]]+#[0-9]+' ``` No leading boundary, so any word ENDING in one of those matches. Measured: ``` Closes #12 -> 12 correct prefixes #12 -> 12 WRONG unfixed #3 -> 3 WRONG this suffixes #99 -> 99 WRONG postfixes #42 -> 42 WRONG see also #5 -> none correct ``` This is not hypothetical for this fleet — "issue prefixes" is standing vocabulary here (the persona convention uses it). When it fires, the reviewer is handed a completely UNRELATED issue's body as "the requirements this PR claims to satisfy", which is worse than giving it no issue at all and directly undercuts what #442 was for. **Decision:** anchor the alternation to a word start — `(^|[^[:alnum:]])` before the group, or `\b` if you keep GNU grep. Add the four false-positive strings above as test cases; the current suite has none of them. ## 2. The test suite stubs the Forgejo helpers without checking they exist `bin/test-review.sh` overrides `forgejo_get_issue` and `forgejo_repo_get_file` with its own stubs. Every production call site is wrapped in `|| return 0` / `2>/dev/null || true`, so if either helper is renamed or changes arity, the feature degrades to a **permanent silent no-op and the suite still passes green**. I verified both exist today with the arity `lib/review.sh` uses (`forgejo_get_issue "$repo" "$number"`, `forgejo_repo_get_file "$repo" "$path"`), so this is about the future, not a live defect. **Decision:** source `lib/forgejo.sh` and assert `declare -F forgejo_get_issue forgejo_repo_get_file` BEFORE installing the stubs. Same class of silent-pass hole as igor#430/#431. ## 3. Document the trust model in lib/review.sh (prevents a future security regression) The shadow reviewer's review of #442 asked which ref `forgejo_repo_get_file` reads, because the answer decides whether a PR can inject text into the prompt of the reviewer that gates its own auto-merge. Verified empirically: - `forgejo_repo_get_file` calls `/repos/{repo}/contents/{path}` with **no `ref`** -> the DEFAULT BRANCH. - Proof: `bin/test-review.sh` exists only on the #442 branch. The contents API returns **empty** without a ref, and the blob only with `ref=<branch>`. So today a PR **cannot** edit its own `Makefile`/`bin/*.sh` to inject into the reviewer's prompt. That is the correct and safe behaviour, and it is currently load-bearing but undocumented. **Decision:** state it in the module comment — this deliberately reads the default branch, and adding a `ref=` to read the PR head would let a PR inject arbitrary text (including fence breaks and fake section headings) into the prompt of the reviewer gating its merge. A future "bug fix" to make the test-runner facts reflect the PR's own changes is exactly how that vector would get opened by someone acting in good faith. Known and accepted, NOT in scope here: the untrusted-issue fence is a delimiter rather than an escape, so an issue body containing the END marker breaks out. That matches the existing `lib/feedback.sh` convention; changing it in one place only would make the codebase inconsistent. If it gets fixed, fix both together. ## Acceptance criteria - The four false-positive strings above extract no issue number; `Closes #12` / `Fixes #7` still extract theirs. - The suite fails if `forgejo_get_issue` or `forgejo_repo_get_file` stops existing. - `lib/review.sh` documents the default-branch read and why changing it is a security decision.

igor referenced this issue from a commit

2026-07-28 05:23:41 +00:00

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

igor referenced this issue from a pull request that will close it,

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

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