joshtronic/igor

Watch

fix: append block findings to the issue body so a re-queued ticket can see them #436

Merged

joshtronic merged 3 commits from agent/434-security-block-findings-are-written-to-a-comment into master

2026-07-28 00:28:35 +00:00

igor commented

2026-07-27 21:45:40 +00:00

Collaborator

What this PR does

fix: append block findings to the issue body so a re-queued ticket can see them
Add forgejo_append_issue_body (lib/forgejo.sh): fetches the issue's current body, appends the reason under a ## Blocked (<date>) heading, and PATCHes it back -- best-effort, returns 1 without touching anything on a fetch/PATCH failure.
bin/agent-block.sh now calls it before commenting, and the comment gets a trailing note that matches the real mechanism ("Appended to the issue description above...") instead of the old comment-only instruction that never actually reached the next run's prompt.
This fixes the bug for every agent-block.sh call site (security gate, scope cap, vacuous tests, off-limits files, repeated noop, etc.), not just the security-review one in the original report, since they all funnel through the same helper.

Test plan

New: bin/test-agent-block.sh -- runs the real agent-block.sh as a subprocess (only curl stubbed at the transport layer; lib/forgejo.sh's real _fj executes) and replays bin/tick.sh's exact ISSUE_BODY/USER_MSG construction to prove the blocked findings land in what the next run's prompt would actually contain, not just "somewhere in Forgejo". Verified this test fails against the pre-fix agent-block.sh (reverted it locally, confirmed 5 checks fail for the right reason, restored the fix).
New: unit tests for forgejo_append_issue_body in bin/test-forgejo.sh (success path preserves + appends, fetch-failure path never PATCHes).
make test passes (all bin/test-*.sh, including the two new/extended ones).
make lint passes (shellcheck + mdl clean).

Closes #434

## What this PR does - [x] fix: append block findings to the issue body so a re-queued ticket can see them - [x] Add `forgejo_append_issue_body` (`lib/forgejo.sh`): fetches the issue's current body, appends the reason under a `## Blocked (<date>)` heading, and PATCHes it back -- best-effort, returns 1 without touching anything on a fetch/PATCH failure. - [x] `bin/agent-block.sh` now calls it before commenting, and the comment gets a trailing note that matches the real mechanism ("Appended to the issue description above...") instead of the old comment-only instruction that never actually reached the next run's prompt. - [x] This fixes the bug for every `agent-block.sh` call site (security gate, scope cap, vacuous tests, off-limits files, repeated noop, etc.), not just the security-review one in the original report, since they all funnel through the same helper. ## Test plan - [x] New: `bin/test-agent-block.sh` -- runs the real `agent-block.sh` as a subprocess (only `curl` stubbed at the transport layer; `lib/forgejo.sh`'s real `_fj` executes) and replays `bin/tick.sh`'s exact `ISSUE_BODY`/`USER_MSG` construction to prove the blocked findings land in what the *next* run's prompt would actually contain, not just "somewhere in Forgejo". Verified this test fails against the pre-fix `agent-block.sh` (reverted it locally, confirmed 5 checks fail for the right reason, restored the fix). - [x] New: unit tests for `forgejo_append_issue_body` in `bin/test-forgejo.sh` (success path preserves + appends, fetch-failure path never PATCHes). - [x] `make test` passes (all `bin/test-*.sh`, including the two new/extended ones). - [x] `make lint` passes (shellcheck + mdl clean). Closes #434

igor added 1 commit

2026-07-27 21:45:40 +00:00

fix: append block findings to the issue body so a re-queued ticket can see them

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

Details

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

Details

29502c99c5

igor commented

2026-07-27 21:48:44 +00:00

Author

Collaborator

🤖 Review — `REQUEST_CHANGES` (automated)

CI for 29502c99: success

Adds forgejo_append_issue_body and wires it into agent-block.sh so block reasons land in the issue body (which is what tick.sh feeds the next run) instead of only a comment. Scope is tight (~200 lines, one issue), the checklist matches the diff item-for-item, and CI is green. One correctness defect blocks: the "best-effort, never touches anything on failure" guarantee isn't actually enforced, and the failure mode destroys the issue description.

Blocking

lib/forgejo.sh:160-165 — a failed/error fetch can silently overwrite the issue body with only the block note.

current=$(forgejo_get_issue "$repo" "$number" | jq -r '.body // empty') || return 1

Two ways this guard doesn't hold:

Pipeline status. $? for a pipeline is the last command's status (jq), not forgejo_get_issue's, unless pipefail is active in the shell that sources the lib. bin/test-forgejo.sh evidently has pipefail on (that's the only reason the GET) return 22 stub produces the asserted rc 1 / "never PATCHes" — the stub emits no output, and jq -r '.body // empty' on empty stdin exits 0). The diff doesn't show agent-block.sh's set line, so I can't confirm the production caller has the same option set. The test therefore proves the guard under the test harness's options, not the caller's.
Valid-JSON error responses. Even with pipefail, a Forgejo error body ({"message":"...","url":"..."} on 403/404/500) is well-formed JSON and curl exits 0 unless _fj uses --fail (not visible here). .body // empty → empty string, exit 0 → the function proceeds to PATCH {"body": "\n\n---\n## Blocked (…)\n\n<reason>\n"}, replacing the entire issue description with just the block note. That's precisely the data the fix exists to preserve, and it's the path most likely to fire in the real world (auth blip, rate limit, renumbered issue).

Fixed looks like: validate the fetch result before writing — e.g. capture the raw response, require jq -e '.number' (or has("body")) to succeed, and return 1 otherwise; don't rely on the pipeline's exit status. A regression test that stubs the GET as an HTTP-error JSON payload (not just a non-zero return with no output) and asserts no PATCH occurs would lock this in.

Non-blocking

bin/test-agent-block.sh only exercises the happy path through the real script. The interesting branch — append fails, so NOTE stays empty, the warning goes to stderr, and the comment/label/unassign still run — is never executed end-to-end. Worth a second case where the stubbed curl fails the GET.
bin/test-forgejo.sh:390-393 — the check labelled "PATCHes the issues endpoint (not labels/comments)" doesn't test the endpoint at all; the _fj stub drops $2, so nothing asserts the path is /repos/acme/x/issues/42. Capture $2 and assert on it, or rename the check.
bin/test-forgejo.sh redefines _fj globally and never restores it. Safe today because these blocks are last in the file, but it's a trap for whoever appends the next test.
Unbounded body growth. Every block/re-queue cycle appends another ## Blocked (<date>) section. After a few rounds the issue body is mostly stale findings, and the date-only heading means same-day repeats produce duplicate headings. Consider replacing a prior ## Blocked section rather than always appending, or at least noting the growth as accepted.

_{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 — `REQUEST_CHANGES` _(automated)_ CI for `29502c99`: **success** Adds `forgejo_append_issue_body` and wires it into `agent-block.sh` so block reasons land in the issue *body* (which is what `tick.sh` feeds the next run) instead of only a comment. Scope is tight (~200 lines, one issue), the checklist matches the diff item-for-item, and CI is green. One correctness defect blocks: the "best-effort, never touches anything on failure" guarantee isn't actually enforced, and the failure mode destroys the issue description. ## Blocking **`lib/forgejo.sh:160-165` — a failed/error fetch can silently overwrite the issue body with only the block note.** ```sh current=$(forgejo_get_issue "$repo" "$number" | jq -r '.body // empty') || return 1 ``` Two ways this guard doesn't hold: 1. **Pipeline status.** `$?` for a pipeline is the *last* command's status (`jq`), not `forgejo_get_issue`'s, unless `pipefail` is active in the shell that sources the lib. `bin/test-forgejo.sh` evidently has `pipefail` on (that's the only reason the `GET) return 22` stub produces the asserted `rc 1` / "never PATCHes" — the stub emits no output, and `jq -r '.body // empty'` on empty stdin exits **0**). The diff doesn't show `agent-block.sh`'s `set` line, so I can't confirm the production caller has the same option set. The test therefore proves the guard under the test harness's options, not the caller's. 2. **Valid-JSON error responses.** Even with `pipefail`, a Forgejo error body (`{"message":"...","url":"..."}` on 403/404/500) is well-formed JSON and `curl` exits 0 unless `_fj` uses `--fail` (not visible here). `.body // empty` → empty string, exit 0 → the function proceeds to `PATCH {"body": "\n\n---\n## Blocked (…)\n\n<reason>\n"}`, **replacing the entire issue description with just the block note**. That's precisely the data the fix exists to preserve, and it's the path most likely to fire in the real world (auth blip, rate limit, renumbered issue). Fixed looks like: validate the fetch result before writing — e.g. capture the raw response, require `jq -e '.number'` (or `has("body")`) to succeed, and `return 1` otherwise; don't rely on the pipeline's exit status. A regression test that stubs the GET as an *HTTP-error JSON payload* (not just a non-zero return with no output) and asserts no PATCH occurs would lock this in. ## Non-blocking - **`bin/test-agent-block.sh`** only exercises the happy path through the real script. The interesting branch — append fails, so `NOTE` stays empty, the warning goes to stderr, and the comment/label/unassign still run — is never executed end-to-end. Worth a second case where the stubbed `curl` fails the GET. - **`bin/test-forgejo.sh:390-393`** — the check labelled "PATCHes the issues endpoint (not labels/comments)" doesn't test the endpoint at all; the `_fj` stub drops `$2`, so nothing asserts the path is `/repos/acme/x/issues/42`. Capture `$2` and assert on it, or rename the check. - **`bin/test-forgejo.sh`** redefines `_fj` globally and never restores it. Safe today because these blocks are last in the file, but it's a trap for whoever appends the next test. - **Unbounded body growth.** Every block/re-queue cycle appends another `## Blocked (<date>)` section. After a few rounds the issue body is mostly stale findings, and the date-only heading means same-day repeats produce duplicate headings. Consider replacing a prior `## Blocked` section rather than always appending, or at least noting the growth as accepted. --- <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-27 21:48:44 +00:00

1 minute 8 seconds

igor self-assigned this

2026-07-27 21:48:44 +00:00

igor commented

2026-07-27 21:56:51 +00:00

Author

Collaborator

🔧 Rework — round 1 (automated)

Addressed the review on claude-opus-5 at effort high — 1 new commit(s).

### 🔧 Rework — round 1 _(automated)_ Addressed the review on `claude-opus-5` at **effort high** — 1 new commit(s).

igor removed their assignment

2026-07-27 21:57:14 +00:00

igor added 1 commit

2026-07-27 21:57:14 +00:00

fix: validate the issue fetch before PATCHing the appended block note

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

Details

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

Details

760b564684

The "best-effort, never touches anything on failure" guarantee in
forgejo_append_issue_body wasn't enforced, and the failure mode
overwrote the whole issue description with just the block note --
exactly the data the helper exists to preserve.

Two holes, both now closed:

  1. `current=$(forgejo_get_issue ... | jq -r '.body // empty')` takes
     the PIPELINE's status, i.e. jq's, unless the caller has pipefail
     set -- and jq on empty stdin exits 0. The fetch is now captured on
     its own line, so no caller shell option can change what the guard
     sees.
  2. Even with pipefail, a 2xx payload that isn't an issue is still
     well-formed JSON whose `.body // empty` is "" with exit 0. The
     payload must now look like an issue (`has("number")`, `has("body")`)
     before anything is written.

Also from review:

- test-forgejo.sh asserts the PATCH path is /repos/acme/x/issues/42
  (the old check named the endpoint but only grepped the payload), and
  restores the real _fj after the stub block instead of leaving it
  redefined for whatever gets appended next.
- test-agent-block.sh covers the other end-to-end branch: the append
  fails, so the note stays off the comment, the warning goes to stderr,
  and the label + unassign still land.
- agent-block.sh's heading carries the time, so a ticket blocked twice
  in one day doesn't grow two identical `## Blocked (date)` headings.
  Growth itself stays accepted and is documented as such -- prior
  attempts are context a re-queued run wants.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

igor added spent time

2026-07-27 21:57:15 +00:00

5 minutes 58 seconds

igor commented

2026-07-27 22:01:00 +00:00

Author

Collaborator

🤖 Review — `COMMENT` (automated)

CI for 760b5646: success

Adds forgejo_append_issue_body to lib/forgejo.sh, wires it into bin/agent-block.sh so block reasons land in the issue body (which is what the next tick's prompt is built from), plus new/extended tests. The core fix is small, correct-looking, and the description matches the diff. CI is green. Two things I can't verify from the diff and a few design notes keep me off APPROVE.

Can't verify from the diff

Is bin/test-agent-block.sh actually executed by CI? The Makefile isn't in the diff. If make test globs bin/test-*.sh the new file is picked up automatically and this is a non-issue; if it enumerates test scripts explicitly, the new 160-line test never runs and the green CI signal says nothing about it. Please confirm (or add the Makefile entry). This is the difference between "tested" and "test file exists".
Nothing else parses the issue body structurally. forgejo_append_issue_body now injects --- + ## Blocked (<ts>) sections into human-authored descriptions. If any other harness path greps the body for markers (task checklists, Closes #, scope hints), this could interact with it. Nothing in the diff touches that, but the diff also doesn't show me the consumers.

Coverage caveat (the "acceptance test" doesn't test `tick.sh`)

Both bin/test-agent-block.sh:118-127 and bin/test-forgejo.sh (the rebuilt tick.sh prompt block) re-implement tick.sh's extraction by hand:

ISSUE_BODY=$(jq -r '.body // ""' <<<"$WINNER")
USER_MSG="You are working Forgejo issue #42 ...
Body:
${ISSUE_BODY}"

That is a copy of the logic, not an invocation of it. If tick.sh changed to build the prompt from something other than .body, both of these checks would still pass while the regression they guard against silently returned. The structural greps in test-forgejo.sh (agent-block.sh calls forgejo_append_issue_body) are the more honest guard here. Consider grepping tick.sh for the .body extraction shape too, so the assumption this whole fix rests on is pinned. Also: the same acceptance assertion is duplicated across both new test blocks — one of them is redundant.

Design notes (non-blocking, but worth a human eye)

Unbounded body growth. Every block appends another section, forever; a ticket that blocks repeatedly grows a body that is then fed verbatim into the next prompt. The comment in lib/forgejo.sh says growth is "accepted" — fine as a decision, but it's a decision about prompt size and issue readability that the human should knowingly own. No cap, no dedupe of identical consecutive reasons.
Lost-update window. Fetch-then-PATCH with no ETag/optimistic concurrency: a human editing the description while the agent blocks will have their edit clobbered. Low probability, but the helper exists precisely to preserve body text.
Block reasons now enter the next run's prompt. Previously the reason was comment-only (human-read); now it's model-read input. If any block reason is derived from untrusted content (e.g. a security-gate finding that quotes attacker-controlled diff text), this is a new prompt-injection surface. It's an inherent consequence of the fix, not a bug in it — flagging so it's a conscious tradeoff.

What I checked and found fine

The two-step fetch validation in forgejo_append_issue_body (separate assignment so caller pipefail can't mask the fetch status, plus has("number") and has("body") shape check with jq -e) correctly avoids the destructive "PATCH the block note as the whole description" failure mode, and both branches are tested.
agent-block.sh only emits the "Appended to the issue description above" note when the append actually succeeded, and falls through to comment + label + unassign on failure — the honest-message path is tested (append failure -> the comment does not claim the body was updated).
Scope is tight to igor#434; no drive-by refactors; no CI-config changes; no secrets or injection in the shell quoting I can see.
Checklist items all correspond to real diff content.

Happy to re-review once the make test wiring question is answered.

_{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 `760b5646`: **success** Adds `forgejo_append_issue_body` to `lib/forgejo.sh`, wires it into `bin/agent-block.sh` so block reasons land in the issue *body* (which is what the next tick's prompt is built from), plus new/extended tests. The core fix is small, correct-looking, and the description matches the diff. CI is green. Two things I can't verify from the diff and a few design notes keep me off APPROVE. ## Can't verify from the diff - **Is `bin/test-agent-block.sh` actually executed by CI?** The Makefile isn't in the diff. If `make test` globs `bin/test-*.sh` the new file is picked up automatically and this is a non-issue; if it enumerates test scripts explicitly, the new 160-line test never runs and the green CI signal says nothing about it. Please confirm (or add the Makefile entry). This is the difference between "tested" and "test file exists". - **Nothing else parses the issue body structurally.** `forgejo_append_issue_body` now injects `---` + `## Blocked (<ts>)` sections into human-authored descriptions. If any other harness path greps the body for markers (task checklists, `Closes #`, scope hints), this could interact with it. Nothing in the diff touches that, but the diff also doesn't show me the consumers. ## Coverage caveat (the "acceptance test" doesn't test `tick.sh`) Both `bin/test-agent-block.sh:118-127` and `bin/test-forgejo.sh` (the `rebuilt tick.sh prompt` block) *re-implement* tick.sh's extraction by hand: ```sh ISSUE_BODY=$(jq -r '.body // ""' <<<"$WINNER") USER_MSG="You are working Forgejo issue #42 ... Body: ${ISSUE_BODY}" ``` That is a copy of the logic, not an invocation of it. If `tick.sh` changed to build the prompt from something other than `.body`, both of these checks would still pass while the regression they guard against silently returned. The structural greps in `test-forgejo.sh` (`agent-block.sh calls forgejo_append_issue_body`) are the more honest guard here. Consider grepping `tick.sh` for the `.body` extraction shape too, so the assumption this whole fix rests on is pinned. Also: the same acceptance assertion is duplicated across both new test blocks — one of them is redundant. ## Design notes (non-blocking, but worth a human eye) - **Unbounded body growth.** Every block appends another section, forever; a ticket that blocks repeatedly grows a body that is then fed verbatim into the next prompt. The comment in `lib/forgejo.sh` says growth is "accepted" — fine as a decision, but it's a decision about prompt size and issue readability that the human should knowingly own. No cap, no dedupe of identical consecutive reasons. - **Lost-update window.** Fetch-then-PATCH with no ETag/optimistic concurrency: a human editing the description while the agent blocks will have their edit clobbered. Low probability, but the helper exists precisely to preserve body text. - **Block reasons now enter the next run's prompt.** Previously the reason was comment-only (human-read); now it's model-read input. If any block reason is derived from untrusted content (e.g. a security-gate finding that quotes attacker-controlled diff text), this is a new prompt-injection surface. It's an inherent consequence of the fix, not a bug in it — flagging so it's a conscious tradeoff. ## What I checked and found fine - The two-step fetch validation in `forgejo_append_issue_body` (separate assignment so caller `pipefail` can't mask the fetch status, plus `has("number") and has("body")` shape check with `jq -e`) correctly avoids the destructive "PATCH the block note as the whole description" failure mode, and both branches are tested. - `agent-block.sh` only emits the "Appended to the issue description above" note when the append actually succeeded, and falls through to comment + label + unassign on failure — the honest-message path is tested (`append failure -> the comment does not claim the body was updated`). - Scope is tight to igor#434; no drive-by refactors; no CI-config changes; no secrets or injection in the shell quoting I can see. - Checklist items all correspond to real diff content. Happy to re-review once the `make test` wiring question is answered. --- <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-27 22:01:00 +00:00

1 minute 24 seconds

igor requested review from joshtronic

2026-07-27 22:01:00 +00:00

joshtronic added 1 commit

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