fix: scope _fj retry to transport failures + log when it gives up

igor commented

2026-07-26 02:15:47 +00:00

Collaborator

What this PR does

Supersedes #426 -- carries both of its commits unchanged, plus one addition that closes #424. Opened as a separate branch rather than pushed onto the bot's, which collides with the rework loop.

fix: absorb transient curl timeouts in the PR-review pickup scan (from #426)
fix(forgejo): retry only transport failures in _fj, never HTTP errors (from #426)
fix(forgejo): log when _fj exhausts its retries (new -- closes #424)

Review of the inherited work (verified, not assumed)

Both #426 commits check out. I re-ran the suites and re-measured the thing I flagged in review:

_fj GET /repos/joshtronic/does-not-exist-xyz   rc=22   0.30s

That was 2.89s before the transport-code scoping -- so the 404 tax is gone, and 404 is the common path for forgejo_repo_list_dir/forgejo_repo_get_file across automerge, ceo, feedback and seo-analysis. No changes needed to either commit.

The addition

igor#424 reported a tick dying status=28 with no error line at all. #425's work stops that path aborting a tick, but an exhausted _fj still returned nonzero silently, so the next occurrence would be just as opaque. Now it logs the method, path, attempt count and curl exit code.

Kept deliberately narrow:

Only the exhausted-transport case logs. An HTTP status returned above is an answer, not a hiccup; a 404 for an absent agent.json is the common path and logging it would bury the signal.
stderr, not stdout. Every _fj caller command-substitutes the response and several guard with || true then use the captured value -- on stdout this diagnostic would silently become that value. There is an explicit test for it.

Test plan

bash bin/test-forgejo.sh passes, including 5 new assertions
make test passes (24 suites)
shellcheck lib/forgejo.sh bin/test-forgejo.sh clean
Live 404 re-timed at 0.30s against git.sherver.org
Verified the give-up line renders: [agent] forgejo: GET /repos/acme/x/pulls/1/reviews failed after 3 attempt(s) (curl exit 28)

One thing worth knowing about the suite

My first draft of these assertions used a has helper. bin/test-forgejo.sh defines only eq -- so three assertions died as has: command not found on stderr while the suite still printed "all checks passed", because it runs under set -uo pipefail without -e and FAIL was never incremented. Caught by reading the output rather than the exit code; rewritten in this file's eq + grep idiom.

That is a latent trap for anything else added to this suite -- a typo'd helper name is a silently skipped test, not a failure. Not fixing it here (it would touch every bin/test-*.sh), but it is worth a follow-up.

Closes #424

Closes #425

## What this PR does Supersedes #426 -- carries both of its commits unchanged, plus one addition that closes #424. Opened as a separate branch rather than pushed onto the bot's, which collides with the rework loop. - [x] fix: absorb transient curl timeouts in the PR-review pickup scan (from #426) - [x] fix(forgejo): retry only transport failures in `_fj`, never HTTP errors (from #426) - [x] fix(forgejo): log when `_fj` exhausts its retries (new -- closes #424) ## Review of the inherited work (verified, not assumed) Both #426 commits check out. I re-ran the suites and re-measured the thing I flagged in review: ``` _fj GET /repos/joshtronic/does-not-exist-xyz rc=22 0.30s ``` That was **2.89s** before the transport-code scoping -- so the 404 tax is gone, and 404 is the common path for `forgejo_repo_list_dir`/`forgejo_repo_get_file` across automerge, ceo, feedback and seo-analysis. No changes needed to either commit. ## The addition igor#424 reported a tick dying `status=28` with no error line at all. #425's work stops that path aborting a tick, but an exhausted `_fj` still returned nonzero **silently**, so the next occurrence would be just as opaque. Now it logs the method, path, attempt count and curl exit code. Kept deliberately narrow: - **Only the exhausted-transport case logs.** An HTTP status returned above is an answer, not a hiccup; a 404 for an absent `agent.json` is the common path and logging it would bury the signal. - **stderr, not stdout.** Every `_fj` caller command-substitutes the response and several guard with `|| true` then use the captured value -- on stdout this diagnostic would silently *become* that value. There is an explicit test for it. ## Test plan - [x] `bash bin/test-forgejo.sh` passes, including 5 new assertions - [x] `make test` passes (24 suites) - [x] `shellcheck lib/forgejo.sh bin/test-forgejo.sh` clean - [x] Live 404 re-timed at 0.30s against git.sherver.org - [x] Verified the give-up line renders: `[agent] forgejo: GET /repos/acme/x/pulls/1/reviews failed after 3 attempt(s) (curl exit 28)` ### One thing worth knowing about the suite My first draft of these assertions used a `has` helper. `bin/test-forgejo.sh` defines only `eq` -- so three assertions died as `has: command not found` on stderr while the suite still printed **"all checks passed"**, because it runs under `set -uo pipefail` without `-e` and `FAIL` was never incremented. Caught by reading the output rather than the exit code; rewritten in this file's `eq` + `grep` idiom. That is a latent trap for anything else added to this suite -- a typo'd helper name is a silently skipped test, not a failure. Not fixing it here (it would touch every `bin/test-*.sh`), but it is worth a follow-up. Closes #424 Closes #425

igor added 3 commits

2026-07-26 02:15:48 +00:00

fix: absorb transient curl timeouts in the PR-review pickup scan instead of aborting the tick

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

Details

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

Details

f043a0abc5

fix(forgejo): retry only transport failures in _fj, never HTTP errors

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

Details

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

Details

cbb5743b10

Review follow-up on igor#425. The bounded GET/HEAD retry retried ANY
nonzero curl status, and with `-sf` curl exits 22 for every HTTP >= 400.
That made an expected 404 probe (the Actions API on Forgejo v15) cost 3
requests and 2s of sleep per repo per tick, retried a 403 rate-limit
immediately, and pushed worst-case per-GET wall clock from 15s to 47s --
turning a fast guarded miss on an unhealthy instance into a slow one,
which is the failure mode the change exists to avoid.

- `_fj` now retries only the transport codes in FORGEJO_RETRY_CURL_CODES
  (7/28/35/52/56) and returns curl's own status for anything else.
- `"${extra[@]+"${extra[@]}"}"` so an empty array expansion doesn't trip
  `set -u` on bash < 4.4.
- Documented the buffered (not streamed) response deliberately: an
  attempt that may be retried can't emit a partial body, every _fj
  response is small JSON, and the one raw-text endpoint
  (forgejo_action_job_log) has its own curl and is untouched.
- Dropped the dead `|| echo ''` in tick.sh's Signal-1 call; the
  non-fatal guarantee lives in the helper's `return 0`.
- Tests: HTTP-error GET is attempted exactly once and preserves rc 22, a
  connect failure (7) still retries. Structural check now anchors on the
  assignment rather than a bare name grep (which the added comment would
  have satisfied) and asserts every surviving forgejo_pr_non_bot_reviews
  call site is `|| echo`-guarded.

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

fix(forgejo): log when _fj exhausts its retries

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

Details

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

Details

ab06166927

igor#424 reported a tick dying `status=28` with no error line, no trace
and no partial output -- nothing to separate a network blip from a
harness bug. The retry work on #425 stops that specific path from
aborting a tick, but an exhausted _fj still returned nonzero silently,
so the next one would be just as opaque.

Log one line when every attempt is burned on a retryable transport
failure, naming the method, path, attempt count and curl exit code.

Deliberately narrow:
- Only the exhausted-transport case logs. An HTTP status returned above
  is an ANSWER, and a 404 for an absent agent.json is the COMMON path
  across automerge/ceo/feedback/seo -- logging those would be noise
  loud enough to bury the real thing.
- To stderr, not stdout. Every _fj caller command-substitutes the
  response, and several guard with `|| true` and use the captured value
  anyway; on stdout this diagnostic would silently BECOME the value.

Adds the same `declare -F log` fallback lib/http-reap.sh uses, since
bin/agent-*.sh and the unit tests source this without tick.sh's log().

igor referenced this pull request

2026-07-26 02:15:58 +00:00

fix: absorb transient curl timeouts in the PR-review pickup scan instead of aborting the tick #426

igor commented

2026-07-26 02:36:36 +00:00

Author

Collaborator

🤖 Review — `REQUEST_CHANGES` (automated)

CI for ab061669: success

Adds a bounded, transport-only retry to _fj, extracts the PR-review Signal-1 check into a best-effort helper, and logs when _fj exhausts its retries. Well-tested and honestly described — but the new give-up diagnostic is swallowed on the exact code path that motivated igor#424, which I think needs fixing before merge.

Blocking

1. The igor#424 diagnostic never reaches the path igor#424 was filed about. (bin/tick.sh:3272, lib/forgejo.sh helper)

The description's rationale is: "#425's work stops that path aborting a tick, but an exhausted _fj still returned nonzero silently, so the next occurrence would be just as opaque." But stderr is discarded twice on that very path:

lib/forgejo.sh: reviews=$(forgejo_pr_non_bot_reviews "$repo" "$number" "$bot" 2>/dev/null) || reviews='[]'
bin/tick.sh:3272: latest_review=$(forgejo_pr_actionable_request_changes "$repo_full" "$pr_num" "$BOT_USER" 2>/dev/null)

So the next exit-28 in the PR-review pickup scan — the reported occurrence, per your own test comment ("two exit-28 ticks landing in this PR-review pickup scan") — will still produce zero output. It won't kill the tick anymore (that's #425), but the #424 half of this PR is a no-op precisely where it was asked for. Both of those 2>/dev/nulls are added by this diff, so it's in scope to fix.

Fixed looks like: drop the 2>/dev/null on the tick.sh call (the helper's return 0 already provides the non-fatal guarantee, per your own comment there), and narrow or drop the inner one so the _fj give-up line survives while jq noise stays suppressed. Add an assertion that the give-up line is visible through forgejo_pr_actionable_request_changes, not just through _fj directly — the current tests only exercise _fj in isolation, which is why this gap passes CI.

Non-blocking, but please answer

2. printf '%s' "$out" drops the trailing newline that curl used to emit. (lib/forgejo.sh, _fj)

The comment says the cost is "a trailing newline that $( ) would have stripped regardless" — true for command substitution, but not for a caller that pipes _fj output. _fj GET ... | while IFS= read -r line loses the final (often only) line, since read at EOF without a delimiter returns non-zero. printf '%s\n' "$out" restores the old byte stream exactly at zero cost. Unless you've grepped every _fj call site for pipe-into-read (I can't from the diff), I'd just use '%s\n'.

3. Worst-case wall clock on a wedged instance. A hung GET is now 3 × FORGEJO_MAX_TIME + 2 × FORGEJO_RETRY_DELAY ≈ 47s per call, and the pickup scan does this per PR per repo. The transport-code scoping keeps the common 404 path fast (good — and the 2.89s → 0.30s measurement is the right thing to have checked), but a genuinely unreachable host now costs 3× per read across the whole scan. Worth a sentence on why that's acceptable, or a global deadline.

4. Fragile structural assertion. bin/test-forgejo.sh: grep -c 'forgejo_pr_non_bot_reviews' "$TICK" vs grep -c '|| echo' on the same lines will false-fail if a future call site puts || echo on a continuation line, and passes vacuously if the count is 0 (which it may be now — this diff removed what looks like the only tick.sh call site). Not worth blocking on; just be aware it may currently be asserting 0 == 0.

Verified

All three description checkboxes correspond to real changes in the diff — no fabrication.
Retry scoping is correct: case "$method" in GET|HEAD) gates attempts, and non-transport codes return "$rc" before any sleep, so POST/PATCH/DELETE can't double-act and a 22 isn't retried. local -a extra=() with "${extra[@]+"${extra[@]}"}" is the right set -u-safe idiom.
forgejo_pr_actionable_request_changes is behaviour-preserving vs. the inlined block it replaces (same REQUEST_CHANGES / !stale / !dismissed predicate, same last-match-wins loop semantics) and genuinely ends in return 0.
The set -euo pipefail subshell test is the right shape for the regression being guarded.
The log() fallback guard (declare -F) is safe and matches the stated lib/http-reap.sh precedent.
CI green; diff ~250 lines, within budget; no CI-config, secret, or injection surface touched.

The note about bin/test-*.sh silently passing on a typo'd helper name is a good catch and correctly left out of this diff — please do file it.

_{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 `ab061669`: **success** Adds a bounded, transport-only retry to `_fj`, extracts the PR-review Signal-1 check into a best-effort helper, and logs when `_fj` exhausts its retries. Well-tested and honestly described — but the new give-up diagnostic is swallowed on the exact code path that motivated igor#424, which I think needs fixing before merge. ## Blocking **1. The igor#424 diagnostic never reaches the path igor#424 was filed about.** (`bin/tick.sh:3272`, `lib/forgejo.sh` helper) The description's rationale is: "#425's work stops that path aborting a tick, but an exhausted `_fj` still returned nonzero **silently**, so the next occurrence would be just as opaque." But stderr is discarded twice on that very path: - `lib/forgejo.sh`: `reviews=$(forgejo_pr_non_bot_reviews "$repo" "$number" "$bot" 2>/dev/null) || reviews='[]'` - `bin/tick.sh:3272`: `latest_review=$(forgejo_pr_actionable_request_changes "$repo_full" "$pr_num" "$BOT_USER" 2>/dev/null)` So the next exit-28 in the PR-review pickup scan — the reported occurrence, per your own test comment ("two exit-28 ticks landing in this PR-review pickup scan") — will still produce zero output. It won't kill the tick anymore (that's #425), but the #424 half of this PR is a no-op precisely where it was asked for. Both of those `2>/dev/null`s are added by *this* diff, so it's in scope to fix. Fixed looks like: drop the `2>/dev/null` on the tick.sh call (the helper's `return 0` already provides the non-fatal guarantee, per your own comment there), and narrow or drop the inner one so the `_fj` give-up line survives while jq noise stays suppressed. Add an assertion that the give-up line is visible through `forgejo_pr_actionable_request_changes`, not just through `_fj` directly — the current tests only exercise `_fj` in isolation, which is why this gap passes CI. ## Non-blocking, but please answer **2. `printf '%s' "$out"` drops the trailing newline that curl used to emit.** (`lib/forgejo.sh`, `_fj`) The comment says the cost is "a trailing newline that `$( )` would have stripped regardless" — true for command substitution, but not for a caller that *pipes* `_fj` output. `_fj GET ... | while IFS= read -r line` loses the final (often only) line, since `read` at EOF without a delimiter returns non-zero. `printf '%s\n' "$out"` restores the old byte stream exactly at zero cost. Unless you've grepped every `_fj` call site for pipe-into-`read` (I can't from the diff), I'd just use `'%s\n'`. **3. Worst-case wall clock on a wedged instance.** A hung `GET` is now `3 × FORGEJO_MAX_TIME + 2 × FORGEJO_RETRY_DELAY` ≈ 47s per call, and the pickup scan does this per PR per repo. The transport-code scoping keeps the common 404 path fast (good — and the 2.89s → 0.30s measurement is the right thing to have checked), but a genuinely unreachable host now costs 3× per read across the whole scan. Worth a sentence on why that's acceptable, or a global deadline. **4. Fragile structural assertion.** `bin/test-forgejo.sh`: `grep -c 'forgejo_pr_non_bot_reviews' "$TICK"` vs `grep -c '|| echo'` on the same lines will false-fail if a future call site puts `|| echo` on a continuation line, and passes vacuously if the count is 0 (which it may be now — this diff removed what looks like the only tick.sh call site). Not worth blocking on; just be aware it may currently be asserting `0 == 0`. ## Verified - All three description checkboxes correspond to real changes in the diff — no fabrication. - Retry scoping is correct: `case "$method" in GET|HEAD)` gates attempts, and non-transport codes `return "$rc"` before any sleep, so POST/PATCH/DELETE can't double-act and a 22 isn't retried. `local -a extra=()` with `"${extra[@]+"${extra[@]}"}"` is the right `set -u`-safe idiom. - `forgejo_pr_actionable_request_changes` is behaviour-preserving vs. the inlined block it replaces (same REQUEST_CHANGES / !stale / !dismissed predicate, same last-match-wins loop semantics) and genuinely ends in `return 0`. - The `set -euo pipefail` subshell test is the right shape for the regression being guarded. - The `log()` fallback guard (`declare -F`) is safe and matches the stated `lib/http-reap.sh` precedent. - CI green; diff ~250 lines, within budget; no CI-config, secret, or injection surface touched. The note about `bin/test-*.sh` silently passing on a typo'd helper name is a good catch and correctly left out of this diff — please do file it. --- <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-26 02:36:36 +00:00

1 minute 56 seconds

igor self-assigned this

2026-07-26 02:36:36 +00:00

igor commented

2026-07-26 02:47:36 +00:00

Author

Collaborator

🔧 Rework — round 1 (automated)

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

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

igor removed their assignment

2026-07-26 02:49:01 +00:00

igor added spent time

2026-07-26 02:49:02 +00:00

8 minutes 56 seconds

igor added 2 commits

2026-07-26 02:49:03 +00:00

Merge remote-tracking branch 'origin/master' into fix/425-scope-retry-to-transport 4e2c70f2a1

fix(forgejo): let the give-up line reach the path igor#424 was filed about

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

Details

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

Details

94c7b953d2

The reviewer caught that the new diagnostic was swallowed on exactly the
call chain it was added for. Two `2>/dev/null`s stood between `_fj` and
the journal, both added by this PR, so the next exit-28 in the PR-review
pickup scan would have printed nothing -- the #424 half was a no-op where
it was asked for.

- tick.sh's pickup call drops its redirect. The helper's `return 0` is
  what makes it non-fatal, so the redirect bought nothing and cost the
  diagnostic.
- The inner one is narrowed rather than dropped: jq's stderr is now
  suppressed inside `forgejo_pr_non_bot_reviews`, on the stage that
  produces the noise. A pipeline's two stages can't be separated from
  outside, and both callers already discarded jq's parse errors, so this
  keeps the noise gone and lets the transport failure through.
- A test drives the REAL `_fj` through the REAL helper with only `curl`
  and `sleep` stubbed, so a redirect reintroduced at either hop fails it.
  The existing tests only exercised `_fj` in isolation, which is why the
  gap passed CI.

Also from review:

- `printf '%s\n'` restores curl's byte stream. `$( )` strips the trailing
  newline either way, but a caller piping into `read` loses the final
  (often only) line, since `read` returns nonzero at EOF without a
  delimiter. An empty body still emits nothing, so a 204 doesn't grow a
  phantom line. Both cases are tested; the pipe test failed before this.
- The worst-case wall clock (~47s per read on a wedged instance) now has
  its rationale written down next to the knobs instead of left implicit.
- The structural assertion that compared two greps is asserted non-vacuous
  first -- it happens to be 1 == 1 today, but an equality between two
  greps that both find nothing is a test with nothing to check.

Not touched: the `2>/dev/null` on tick.sh's other
forgejo_pr_non_bot_reviews call, in the rework flow. It predates this PR
and is one of three sibling lines; #424's occurrence was the pickup scan.

igor commented

2026-07-26 02:54:17 +00:00

Author

Collaborator

🤖 Review — `COMMENT` (automated)

CI for 94c7b953: success

Scopes _fj's new bounded retry to transport-only curl codes and adds a stderr give-up log; carries #426's two commits plus one new one. CI green, tests are unusually thorough, checklist matches the diff. Nothing I can prove is broken — but a few reservations I'd want you to weigh before merging, so COMMENT rather than approve.

Findings

lib/forgejo.sh forgejo_pr_non_bot_reviews — the new docstring makes a claim the diff only half-delivers. The comment says jq's stderr is suppressed here "rather than by the callers (igor#424): both of them want the parse noise gone… blanketing the whole function with 2>/dev/null also swallows _fj's give-up line." Only one call site is touched in the diff (bin/tick.sh:3335). If the other forgejo_pr_non_bot_reviews call site in tick.sh still carries a 2>/dev/null, the give-up line is still swallowed there and the stated rationale doesn't hold for it. The new structural test only asserts || echo on those lines, not the absence of 2>/dev/null — so nothing catches it. Please confirm the second call site (and add the stderr assertion if it applies).
_fj now normalizes the response's trailing bytes for every caller. out=$(curl …) strips all trailing newlines and printf '%s\n' "$out" re-emits exactly one. Callers that command-substitute or pipe to jq can't tell (and the pipe-into-read case is tested), but any caller that writes _fj output straight to a file or compares bytes now sees foo\n\n\n → foo\n, and a body with no trailing newline gains one. The other call sites aren't in the diff, so I can't verify none of them do that.
Retry amplification on legitimately slow reads, not just blips. A large response that genuinely exceeds --max-time 15 (job-log / CI-log fetches go through _fj GET) exits 28 — indistinguishable from a transient stall — so it now burns 3 × 15s + 2 × 1s ≈ 47s and still fails, where it used to fail in 15s. The description accepts the 47s worst case for an unreachable instance; the "response is just big" case is the same cost for a deterministic failure. Worth confirming the log-fetch path can't hit it. Related: the full body is now buffered in a shell variable rather than streamed, which matters most on exactly that path.
Process, not code: this PR closes #424 and #425 and supersedes #426. #426 won't auto-close — remember to close it manually so the rework loop doesn't keep working the stale branch.

Test-coverage notes

The give-up assertions are good, and running the real _fj through the real forgejo_pr_actionable_request_changes (stubbing only curl/sleep) is the right shape — that's the assertion that actually protects igor#424's reported path.
grep 'forgejo_pr_non_bot_reviews' "$TICK" | grep -c '|| echo' matches lines, including comments. A future comment mentioning the function name, or a call whose || echo wraps to the next line, fails this suite for the wrong reason. Consider anchoring it to the assignment shape the way the latest_review= check already is.
The has: command not found trap you documented (missing helper ⇒ silently skipped test, suite still prints "all checks passed") is a genuinely important find. Agreed it's out of scope here, but it should get an issue — until then, every bin/test-*.sh can green-light assertions that never ran.

No security concerns; no CI-config changes; diff is in scope and under budget.

_{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 `94c7b953`: **success** Scopes `_fj`'s new bounded retry to transport-only curl codes and adds a stderr give-up log; carries #426's two commits plus one new one. CI green, tests are unusually thorough, checklist matches the diff. Nothing I can prove is broken — but a few reservations I'd want you to weigh before merging, so **COMMENT** rather than approve. ## Findings - **`lib/forgejo.sh` `forgejo_pr_non_bot_reviews` — the new docstring makes a claim the diff only half-delivers.** The comment says jq's stderr is suppressed *here* "rather than by the callers (igor#424): **both of them** want the parse noise gone… blanketing the whole function with `2>/dev/null` also swallows `_fj`'s give-up line." Only one call site is touched in the diff (`bin/tick.sh:3335`). If the *other* `forgejo_pr_non_bot_reviews` call site in tick.sh still carries a `2>/dev/null`, the give-up line is still swallowed there and the stated rationale doesn't hold for it. The new structural test only asserts `|| echo` on those lines, not the absence of `2>/dev/null` — so nothing catches it. Please confirm the second call site (and add the stderr assertion if it applies). - **`_fj` now normalizes the response's trailing bytes for *every* caller.** `out=$(curl …)` strips all trailing newlines and `printf '%s\n' "$out"` re-emits exactly one. Callers that command-substitute or pipe to `jq` can't tell (and the pipe-into-`read` case is tested), but any caller that writes `_fj` output straight to a file or compares bytes now sees `foo\n\n\n` → `foo\n`, and a body with no trailing newline gains one. The other call sites aren't in the diff, so I can't verify none of them do that. - **Retry amplification on legitimately slow reads, not just blips.** A large response that genuinely exceeds `--max-time 15` (job-log / CI-log fetches go through `_fj` GET) exits 28 — indistinguishable from a transient stall — so it now burns 3 × 15s + 2 × 1s ≈ 47s and still fails, where it used to fail in 15s. The description accepts the 47s worst case for an unreachable instance; the "response is just big" case is the same cost for a deterministic failure. Worth confirming the log-fetch path can't hit it. Related: the full body is now buffered in a shell variable rather than streamed, which matters most on exactly that path. - **Process, not code:** this PR closes #424 *and* #425 and supersedes #426. #426 won't auto-close — remember to close it manually so the rework loop doesn't keep working the stale branch. ## Test-coverage notes - The give-up assertions are good, and running the real `_fj` through the real `forgejo_pr_actionable_request_changes` (stubbing only `curl`/`sleep`) is the right shape — that's the assertion that actually protects igor#424's reported path. - `grep 'forgejo_pr_non_bot_reviews' "$TICK" | grep -c '|| echo'` matches *lines*, including comments. A future comment mentioning the function name, or a call whose `|| echo` wraps to the next line, fails this suite for the wrong reason. Consider anchoring it to the assignment shape the way the `latest_review=` check already is. - The `has: command not found` trap you documented (missing helper ⇒ silently skipped test, suite still prints "all checks passed") is a genuinely important find. Agreed it's out of scope here, but it should get an issue — until then, every `bin/test-*.sh` can green-light assertions that never ran. No security concerns; no CI-config changes; diff is in scope and under budget. --- <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-26 02:54:17 +00:00

2 minutes 19 seconds

igor requested review from joshtronic

2026-07-26 02:54:17 +00:00

joshtronic added 1 commit

2026-07-26 16:36:27 +00:00