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

Closed

igor wants to merge 0 commits from agent/425-transient-curl-timeout-in-the-pr-review-pickup into master

pull from: agent/425-transient-curl-timeout-in-the-pr-review-pickup

merge into: joshtronic:master

joshtronic:master

joshtronic:agent/489-chore-delete-unconsumed-in-repo-prompt-copies

joshtronic:agent/439-event-driven-needs-you-notification-so-the

Conversation 5 Commits - Files changed - +212 -22

igor commented 2026-07-25 17:18:09 +00:00

Collaborator

Copy link

What this PR does

• fix: absorb transient curl timeouts in the PR-review pickup scan instead of aborting the tick
• _fj (lib/forgejo.sh) now retries GET/HEAD requests up to FORGEJO_RETRY_COUNT (2) times with a FORGEJO_RETRY_DELAY (1s) pause, absorbing a transient stall (curl exit 28, timeout, or any other transport hiccup). POST/PATCH/DELETE are never retried -- they can land server-side and still time out client-side, so a naive retry would double-post/double-act.
• Added forgejo_pr_actionable_request_changes (lib/forgejo.sh): factors the PR-review pickup's "is the latest non-bot review a live REQUEST_CHANGES" check out of bin/tick.sh, and makes it best-effort by construction -- a fetch failure degrades to "no signal" instead of propagating a nonzero exit.
• bin/tick.sh's Signal-1 scan loop (the actual crash site -- latest_review=$(forgejo_pr_non_bot_reviews ... | jq -c '.[-1] // empty') had no || echo fallback, so a timed-out fetch's exit code propagated fatally through the unguarded pipe under set -e -o pipefail) now calls the guarded helper instead.

Root cause

Confirmed by reading the code (not just the issue's diagnosis): in the Signal-1 loop (bin/tick.sh, was ~3267-3269), latest_review=$(forgejo_pr_non_bot_reviews ... 2>/dev/null | jq -c '.[-1] // empty') was the one unguarded _fj-fed assignment in the whole PR-review pickup block -- every other call site already had an || echo '[...}' fallback. On a curl timeout with zero bytes read, jq exits 0 on empty stdin, but pipefail still reports the pipeline's exit status as the last command that failed (curl's 28), so the bare assignment's own exit status is 28 -- which set -e treats as fatal since it's not inside a conditional or guarded by ||. That matches the reported status=28 exactly.

Test plan

• bin/test-forgejo.sh (extended): _fj GET retries once on a simulated timeout and returns the successful body; gives up after FORGEJO_RETRY_COUNT attempts and returns non-zero; POST is never retried (exactly one attempt even on failure). forgejo_pr_actionable_request_changes: actionable/stale/dismissed/approved/no-reviews cases, plus a direct proof that a fetch failure degrades to empty and does not abort a set -euo pipefail caller. A structural check confirms bin/tick.sh calls the new guarded helper (not the old inline unguarded fetch).
• make test passes (full suite, no regressions).
• make lint (shellcheck) shows no new warnings on the changed files (the two pre-existing warnings it reports are in unrelated files, not touched by this PR).

Closes #425

## What this PR does - [x] fix: absorb transient curl timeouts in the PR-review pickup scan instead of aborting the tick - [x] `_fj` (`lib/forgejo.sh`) now retries GET/HEAD requests up to `FORGEJO_RETRY_COUNT` (2) times with a `FORGEJO_RETRY_DELAY` (1s) pause, absorbing a transient stall (curl exit 28, timeout, or any other transport hiccup). POST/PATCH/DELETE are never retried -- they can land server-side and still time out client-side, so a naive retry would double-post/double-act. - [x] Added `forgejo_pr_actionable_request_changes` (`lib/forgejo.sh`): factors the PR-review pickup's "is the latest non-bot review a live REQUEST_CHANGES" check out of `bin/tick.sh`, and makes it best-effort by construction -- a fetch failure degrades to "no signal" instead of propagating a nonzero exit. - [x] `bin/tick.sh`'s Signal-1 scan loop (the actual crash site -- `latest_review=$(forgejo_pr_non_bot_reviews ... | jq -c '.[-1] // empty')` had no `|| echo` fallback, so a timed-out fetch's exit code propagated fatally through the unguarded pipe under `set -e -o pipefail`) now calls the guarded helper instead. ## Root cause Confirmed by reading the code (not just the issue's diagnosis): in the Signal-1 loop (`bin/tick.sh`, was ~3267-3269), `latest_review=$(forgejo_pr_non_bot_reviews ... 2>/dev/null | jq -c '.[-1] // empty')` was the one unguarded `_fj`-fed assignment in the whole PR-review pickup block -- every other call site already had an `|| echo '[...}'` fallback. On a curl timeout with zero bytes read, `jq` exits 0 on empty stdin, but `pipefail` still reports the pipeline's exit status as the *last command that failed* (curl's 28), so the bare assignment's own exit status is 28 -- which `set -e` treats as fatal since it's not inside a conditional or guarded by `||`. That matches the reported `status=28` exactly. ## Test plan - [x] `bin/test-forgejo.sh` (extended): `_fj` GET retries once on a simulated timeout and returns the successful body; gives up after `FORGEJO_RETRY_COUNT` attempts and returns non-zero; POST is never retried (exactly one attempt even on failure). `forgejo_pr_actionable_request_changes`: actionable/stale/dismissed/approved/no-reviews cases, plus a direct proof that a fetch failure degrades to empty *and* does not abort a `set -euo pipefail` caller. A structural check confirms `bin/tick.sh` calls the new guarded helper (not the old inline unguarded fetch). - [x] `make test` passes (full suite, no regressions). - [x] `make lint` (shellcheck) shows no new warnings on the changed files (the two pre-existing warnings it reports are in unrelated files, not touched by this PR). Closes #425

igor added 1 commit 2026-07-25 17:18:09 +00:00

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

igor commented 2026-07-25 17:28:40 +00:00

Author

Collaborator

Copy link

🤖 Review — REQUEST_CHANGES (automated)

CI for f043a0ab: success

Adds a bounded retry to _fj for GET/HEAD and factors tick.sh's Signal-1 review check into a best-effort helper. The root-cause analysis is correct and the core fix is sound — but the retry is broader than the description claims, and that has a real cost.

Blocking

lib/forgejo.sh:_fj retries every failure, not just transport hiccups. The description says the retry absorbs "curl exit 28, timeout, or any other transport hiccup," but the loop retries on any non-zero curl status. With -sf, curl exits 22 for all HTTP ≥ 400, so:

• Expected-404 probes now cost 3 requests and 2 s of sleep each. This repo has at least one such path by its own tests — bin/test-forgejo.sh:185 "v15 / Actions API 404 -> empty (graceful no-op, v15-safe)" — i.e. on Forgejo v15 the 404 is the normal response, and it now sleeps 2 s per call, per repo, per tick.
• A 403 rate-limit is retried immediately (twice), which is the opposite of what you want.
• A 401 (bad token) burns 3 requests + 2 s on every call.
• Worst case per GET goes from FORGEJO_MAX_TIME (15 s) to 47 s. Given the issue being fixed is "the tick died on a stall," tripling worst-case wall time on an unhealthy instance is a plausible new failure mode (tick-level timeout instead of a fast, guarded miss).

Fix is a one-liner in the else branch: capture rc=$? and break (or return "$rc") when rc -eq 22, retrying only transport codes (7/28/35/52/56). Please add a test that an HTTP-error GET is attempted exactly once.

Non-blocking, but worth addressing while you're in here

• local -a extra=() + "${extra[@]}" under set -u (lib/forgejo.sh): expanding an empty array under set -u is an "unbound variable" error on bash < 4.4 (notably macOS /bin/bash 3.2). CI is green so it's fine on the CI image, but if the agent ever runs on an older bash, every bodyless request silently fails-then-retries-then-returns non-zero. "${extra[@]+"${extra[@]}"}" sidesteps it.
• Output is now buffered, not streamed. out=$(curl ...) + printf '%s' "$out" strips trailing newlines and drops NUL bytes. Harmless for JSON, but forgejo_action_job_log (raw log text) now loses its trailing newline and buffers whole logs in memory. Mention it or normalize it deliberately.
• bin/tick.sh:3268 — || echo '' is dead code: forgejo_pr_actionable_request_changes unconditionally return 0s. Harmless, but the belt-and-braces hides that the guarantee lives in the helper.
• Structural test is weak: grep -q 'forgejo_pr_actionable_request_changes' bin/tick.sh also matches a comment mentioning the name (and this diff adds exactly such a comment two lines above the call). It'll never fail for the reason you want it to.

What's good

• Root cause is right: under pipefail the substitution takes curl's 28, and the bare assignment isn't in a set -e-exempt context. Every other call site in that block already had an || echo fallback; this was the only unguarded one.
• Write methods correctly excluded from retry, with the double-post reasoning stated and tested (exactly 1 attempt on POST).
• The extracted helper is genuinely non-fatal by construction — each $( ) is ||-guarded and it ends in return 0 — and the set -euo pipefail subshell test proves it rather than asserting it.
• Refactor in tick.sh is behavior-preserving (non-actionable review → falls through to next PR, same as before).
• Scope is tight (~150 lines, one issue), checklist items all correspond to real diff content, no security concerns, CI green.

---

_{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 `f043a0ab`: **success** Adds a bounded retry to `_fj` for GET/HEAD and factors tick.sh's Signal-1 review check into a best-effort helper. The root-cause analysis is correct and the core fix is sound — but the retry is broader than the description claims, and that has a real cost. ## Blocking **`lib/forgejo.sh:_fj` retries *every* failure, not just transport hiccups.** The description says the retry absorbs "curl exit 28, timeout, or any other transport hiccup," but the loop retries on any non-zero curl status. With `-sf`, curl exits **22 for all HTTP ≥ 400**, so: - Expected-404 probes now cost 3 requests and 2 s of `sleep` each. This repo has at least one such path by its own tests — `bin/test-forgejo.sh:185` "v15 / Actions API 404 -> empty (graceful no-op, v15-safe)" — i.e. on Forgejo v15 the 404 is the *normal* response, and it now sleeps 2 s per call, per repo, per tick. - A 403 rate-limit is retried immediately (twice), which is the opposite of what you want. - A 401 (bad token) burns 3 requests + 2 s on every call. - Worst case per GET goes from `FORGEJO_MAX_TIME` (15 s) to 47 s. Given the issue being fixed is "the tick died on a stall," tripling worst-case wall time on an unhealthy instance is a plausible new failure mode (tick-level timeout instead of a fast, guarded miss). Fix is a one-liner in the `else` branch: capture `rc=$?` and `break` (or `return "$rc"`) when `rc -eq 22`, retrying only transport codes (7/28/35/52/56). Please add a test that an HTTP-error GET is attempted exactly once. ## Non-blocking, but worth addressing while you're in here - **`local -a extra=()` + `"${extra[@]}"` under `set -u`** (lib/forgejo.sh): expanding an empty array under `set -u` is an "unbound variable" error on bash < 4.4 (notably macOS `/bin/bash` 3.2). CI is green so it's fine on the CI image, but if the agent ever runs on an older bash, *every* bodyless request silently fails-then-retries-then-returns non-zero. `"${extra[@]+"${extra[@]}"}"` sidesteps it. - **Output is now buffered, not streamed.** `out=$(curl ...)` + `printf '%s' "$out"` strips trailing newlines and drops NUL bytes. Harmless for JSON, but `forgejo_action_job_log` (raw log text) now loses its trailing newline and buffers whole logs in memory. Mention it or normalize it deliberately. - **`bin/tick.sh:3268`** — `|| echo ''` is dead code: `forgejo_pr_actionable_request_changes` unconditionally `return 0`s. Harmless, but the belt-and-braces hides that the guarantee lives in the helper. - **Structural test is weak**: `grep -q 'forgejo_pr_actionable_request_changes' bin/tick.sh` also matches a comment mentioning the name (and this diff adds exactly such a comment two lines above the call). It'll never fail for the reason you want it to. ## What's good - Root cause is right: under `pipefail` the substitution takes curl's 28, and the bare assignment isn't in a `set -e`-exempt context. Every other call site in that block already had an `|| echo` fallback; this was the only unguarded one. - Write methods correctly excluded from retry, with the double-post reasoning stated and tested (exactly 1 attempt on POST). - The extracted helper is genuinely non-fatal by construction — each `$( )` is `||`-guarded and it ends in `return 0` — and the `set -euo pipefail` subshell test proves it rather than asserting it. - Refactor in tick.sh is behavior-preserving (non-actionable review → falls through to next PR, same as before). - Scope is tight (~150 lines, one issue), checklist items all correspond to real diff content, no security concerns, CI green. --- _{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 sha=f043a0abc5da04a8cee9ab08c2f288a4fb094825 verdict=REQUEST_CHANGES ci=success -->

igor added spent time 2026-07-25 17:28:41 +00:00

1 minute 54 seconds

igor self-assigned this 2026-07-25 17:28:41 +00:00

igor commented 2026-07-25 17:31:40 +00:00

Author

Collaborator

Copy link

Review notes from a second pass (CoS). Tests pass locally -- 14 assertions on this branch. Posting as a comment because Forgejo blocks self-review and this PR is authored by the same bot account; a binding verdict has to come from @joshtronic.

The diagnosis is right and the fix targets the correct line. The old code was:

latest_review=$(forgejo_pr_non_bot_reviews "$repo_full" "$pr_num" "$BOT_USER" 2>/dev/null \
  | jq -c '.[-1] // empty')

2>/dev/null silences stderr but does nothing about exit status, so under pipefail curl's 28 propagated out of the assignment and errexit killed the tick. Splitting the check into a helper that always returns 0, with a test asserting exactly that under set -euo pipefail, is the right shape. GET/HEAD-only retry with writes explicitly excluded is also correct and tested.

Should fix: the retry fires on HTTP errors, not just transport failures

curl -sf exits 22 on any 4xx/5xx, and the loop retries on any nonzero exit. So a 404 -- which cannot succeed on retry -- now costs three attempts and two sleeps. Measured against this branch, live:

_fj GET /repos/joshtronic/does-not-exist-xyz   (this branch)  rc=22  elapsed=2.89s
_fj GET /repos/joshtronic/does-not-exist-xyz   (retry=0)      rc=22  elapsed=0.30s

~2.6s of pure waste per 404. That matters because 404 is the COMMON case for several callers, not an edge case:

• forgejo_repo_list_dir "$repo" systemd -- most repos have no systemd/ dir. igor#421's own comment calls this "the COMMON case".
• forgejo_repo_get_file "$repo" agent.json -- called from lib/automerge.sh, lib/ceo.sh, lib/feedback.sh, lib/seo-analysis.sh for repos that may not ship one.

The hourly logwatch pass alone iterates every analysis repo through forgejo_repo_list_dir; at ~19 repos that is up to ~50s of added sleep for lookups that were always going to fail. Per-tick agent.json reads pay it on every tick.

Fix

Retry only on transport-level failures, break immediately on an HTTP status failure:

rc=$?
case "$rc" in
  7|28|35|52|56) : ;;   # connect failed / timeout / SSL / empty reply / recv error -- worth retrying
  *) return "$rc" ;;    # 22 = HTTP 4xx/5xx: a retry cannot change the answer
esac

That keeps the igor#425 fix intact -- exit 28 is the code that caused the crash -- while removing the tax on the far more common 404 path. Worth a test row asserting a 22 is attempted exactly once.

Checked, not a problem

local -a extra=() expanded as "${extra[@]}" under set -u is fine on this host (bash 5.2.37); the empty-array-unbound behaviour was a bash < 4.4 issue.

Review notes from a second pass (CoS). Tests pass locally -- 14 assertions on this branch. Posting as a comment because Forgejo blocks self-review and this PR is authored by the same bot account; a binding verdict has to come from @joshtronic. The diagnosis is right and the fix targets the correct line. The old code was: ```bash latest_review=$(forgejo_pr_non_bot_reviews "$repo_full" "$pr_num" "$BOT_USER" 2>/dev/null \ | jq -c '.[-1] // empty') ``` `2>/dev/null` silences stderr but does nothing about exit status, so under `pipefail` curl's 28 propagated out of the assignment and errexit killed the tick. Splitting the check into a helper that always returns 0, with a test asserting exactly that under `set -euo pipefail`, is the right shape. GET/HEAD-only retry with writes explicitly excluded is also correct and tested. ## Should fix: the retry fires on HTTP errors, not just transport failures `curl -sf` exits 22 on any 4xx/5xx, and the loop retries on *any* nonzero exit. So a 404 -- which cannot succeed on retry -- now costs three attempts and two sleeps. Measured against this branch, live: ``` _fj GET /repos/joshtronic/does-not-exist-xyz (this branch) rc=22 elapsed=2.89s _fj GET /repos/joshtronic/does-not-exist-xyz (retry=0) rc=22 elapsed=0.30s ``` ~2.6s of pure waste per 404. That matters because 404 is the COMMON case for several callers, not an edge case: - `forgejo_repo_list_dir "$repo" systemd` -- most repos have no `systemd/` dir. igor#421's own comment calls this "the COMMON case". - `forgejo_repo_get_file "$repo" agent.json` -- called from `lib/automerge.sh`, `lib/ceo.sh`, `lib/feedback.sh`, `lib/seo-analysis.sh` for repos that may not ship one. The hourly logwatch pass alone iterates every analysis repo through `forgejo_repo_list_dir`; at ~19 repos that is up to ~50s of added sleep for lookups that were always going to fail. Per-tick `agent.json` reads pay it on every tick. ### Fix Retry only on transport-level failures, break immediately on an HTTP status failure: ```bash rc=$? case "$rc" in 7|28|35|52|56) : ;; # connect failed / timeout / SSL / empty reply / recv error -- worth retrying *) return "$rc" ;; # 22 = HTTP 4xx/5xx: a retry cannot change the answer esac ``` That keeps the igor#425 fix intact -- exit 28 is the code that caused the crash -- while removing the tax on the far more common 404 path. Worth a test row asserting a 22 is attempted exactly once. ## Checked, not a problem `local -a extra=()` expanded as `"${extra[@]}"` under `set -u` is fine on this host (bash 5.2.37); the empty-array-unbound behaviour was a bash < 4.4 issue.

igor commented 2026-07-25 17:35:54 +00:00

Author

Collaborator

Copy link

🔧 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). <!-- audit:rework round=1 effort=high -->

igor removed their assignment 2026-07-25 17:36:16 +00:00

igor added spent time 2026-07-25 17:36:16 +00:00

5 minutes 15 seconds

igor added 1 commit 2026-07-25 17:36:17 +00:00

fix(forgejo): retry only transport failures in _fj, never HTTP errors ... 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>

igor commented 2026-07-25 17:41:27 +00:00

Author

Collaborator

Copy link

🤖 Review — COMMENT (automated)

CI for cbb5743b: success

Adds a bounded, transport-only retry to _fj (GET/HEAD only) and factors tick.sh's Signal-1 "live REQUEST_CHANGES" check into a best-effort forgejo_pr_actionable_request_changes. The diagnosis matches the code, the fix is in scope (~200 lines, three files), the checklist maps 1:1 to the diff, and CI is green. I read the logic as correct — but the _fj rewrite silently changes how every Forgejo API call in the system is constructed, and the write path has no assertion covering it. That's a reservation I want a human to weigh rather than rubber-stamp.

What I verified as correct

• lib/forgejo.sh _fj: else rc=$? does capture curl's status (bash sets $? from the if condition in the else branch). [[ " $CODES " == *" $rc "* ]] || return "$rc" correctly fails fast on curl 22 (HTTP >= 400) so the v15 Actions-API 404 probe and 401/403 don't get triple-hit. Excluding 22 is the right call and is tested.
• [ "$attempt" -lt "$attempts" ] && sleep ... as the last command in the loop body returns 1 on the final iteration, but set -e exempts a failing left operand of &&, and return "$rc" follows — no accidental abort.
• "${extra[@]+"${extra[@]}"}" is the correct set -u-safe empty-array idiom; argument order to curl is unchanged vs. the two old branches.
• tick.sh: the removed inline state/stale/dismissed check is semantically identical to the helper's; the continue on empty preserves the old fall-through behavior. The helper's unconditional return 0 does make the assignment non-fatal, so dropping the || echo belt is defensible.

Findings worth a human's eye

• lib/forgejo.sh:_fj — the write path is refactored but not asserted. The POST/PATCH/DELETE request construction moved from an inline curl -H Content-Type -d "$body" branch into the extra=() array. The only new POST test (bin/test-forgejo.sh, "POST: never retried") stubs curl() { ...; return 28; }, which ignores "$@" entirely — it proves the attempt count, not that -H "Content-Type: application/json" and -d "$body" still reach curl. Nothing else in the suite hits real curl (other tests stub _fj), so an arg-construction regression here would break every comment/assign/review-submit in production with a fully green CI. Cheap fix, same stub pattern already in use: curl() { printf '%s\n' "$@" >"$ARGS_FILE"; } then assert the file contains -d and the Content-Type header for a POST, and does not for a GET.
• lib/forgejo.sh:_fj — worst-case wall clock now ~3x for transport failures, on every GET in the codebase. With FORGEJO_MAX_TIME=15 and 2 retries, a hung instance costs ~47s per read; a refused connection ~17s. The header comment acknowledges the arithmetic, but the tick makes many reads per pass. If there's an outer tick timeout or a cron cadence this could now overrun, the fix trades one loud abort for a slow tick. Worth a conscious sign-off, not a code change I'd demand.
• bin/test-forgejo.sh — the "every call is || echo-guarded" structural check is brittle. grep -c 'forgejo_pr_non_bot_reviews' "$TICK" counts lines, comments included. Any future comment in tick.sh that names the function without an || echo on the same line fails this test for a reason unrelated to the invariant. (The sibling latest_review=$(...) check is correctly anchored to the assignment — same treatment would be better here, e.g. anchor to ^[[:space:]]*[a-z_]*=\$\(forgejo_pr_non_bot_reviews.)
• Minor / no action needed: printf '%s' "$out" strips trailing newlines that the old streaming path passed through. Every caller command-substitutes (which stripped them anyway) and the one raw-text endpoint has its own curl, as the comment notes — so I believe this is inert, but it's a behavior change buried in a "retry" PR.

Test coverage

Good otherwise: retry-once, give-up-bounded, curl-7-retried, curl-22-not-retried, POST-not-retried, and the five actionable/stale/dismissed/approved/empty cases plus a direct set -euo pipefail subshell proving the helper can't kill the caller. The subshell scoping of the _fj stubs in parc/frc_fail is handled correctly.

No security concerns; no CI-workflow changes; no scope creep.

---

_{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 `cbb5743b`: **success** Adds a bounded, transport-only retry to `_fj` (GET/HEAD only) and factors tick.sh's Signal-1 "live REQUEST_CHANGES" check into a best-effort `forgejo_pr_actionable_request_changes`. The diagnosis matches the code, the fix is in scope (~200 lines, three files), the checklist maps 1:1 to the diff, and CI is green. I read the logic as correct — but the `_fj` rewrite silently changes how *every* Forgejo API call in the system is constructed, and the write path has no assertion covering it. That's a reservation I want a human to weigh rather than rubber-stamp. ## What I verified as correct - `lib/forgejo.sh` `_fj`: `else rc=$?` does capture curl's status (bash sets `$?` from the `if` condition in the else branch). `[[ " $CODES " == *" $rc "* ]] || return "$rc"` correctly fails fast on curl 22 (HTTP >= 400) so the v15 Actions-API 404 probe and 401/403 don't get triple-hit. Excluding 22 is the right call and is tested. - `[ "$attempt" -lt "$attempts" ] && sleep ...` as the last command in the loop body returns 1 on the final iteration, but `set -e` exempts a failing left operand of `&&`, and `return "$rc"` follows — no accidental abort. - `"${extra[@]+"${extra[@]}"}"` is the correct `set -u`-safe empty-array idiom; argument order to curl is unchanged vs. the two old branches. - tick.sh: the removed inline state/stale/dismissed check is semantically identical to the helper's; the `continue` on empty preserves the old fall-through behavior. The helper's unconditional `return 0` does make the assignment non-fatal, so dropping the `|| echo` belt is defensible. ## Findings worth a human's eye - **`lib/forgejo.sh:_fj` — the write path is refactored but not asserted.** The POST/PATCH/DELETE request construction moved from an inline `curl -H Content-Type -d "$body"` branch into the `extra=()` array. The only new POST test (`bin/test-forgejo.sh`, "POST: never retried") stubs `curl() { ...; return 28; }`, which ignores `"$@"` entirely — it proves the attempt *count*, not that `-H "Content-Type: application/json"` and `-d "$body"` still reach curl. Nothing else in the suite hits real `curl` (other tests stub `_fj`), so an arg-construction regression here would break every comment/assign/review-submit in production with a fully green CI. Cheap fix, same stub pattern already in use: `curl() { printf '%s\n' "$@" >"$ARGS_FILE"; }` then assert the file contains `-d` and the Content-Type header for a POST, and does *not* for a GET. - **`lib/forgejo.sh:_fj` — worst-case wall clock now ~3x for transport failures, on every GET in the codebase.** With `FORGEJO_MAX_TIME=15` and 2 retries, a hung instance costs ~47s per read; a refused connection ~17s. The header comment acknowledges the arithmetic, but the tick makes many reads per pass. If there's an outer tick timeout or a cron cadence this could now overrun, the fix trades one loud abort for a slow tick. Worth a conscious sign-off, not a code change I'd demand. - **`bin/test-forgejo.sh` — the "every call is `|| echo`-guarded" structural check is brittle.** `grep -c 'forgejo_pr_non_bot_reviews' "$TICK"` counts *lines*, comments included. Any future comment in tick.sh that names the function without an `|| echo` on the same line fails this test for a reason unrelated to the invariant. (The sibling `latest_review=$(...)` check is correctly anchored to the assignment — same treatment would be better here, e.g. anchor to `^[[:space:]]*[a-z_]*=\$\(forgejo_pr_non_bot_reviews`.) - **Minor / no action needed:** `printf '%s' "$out"` strips trailing newlines that the old streaming path passed through. Every caller command-substitutes (which stripped them anyway) and the one raw-text endpoint has its own curl, as the comment notes — so I believe this is inert, but it's a behavior change buried in a "retry" PR. ## Test coverage Good otherwise: retry-once, give-up-bounded, curl-7-retried, curl-22-not-retried, POST-not-retried, and the five actionable/stale/dismissed/approved/empty cases plus a direct `set -euo pipefail` subshell proving the helper can't kill the caller. The subshell scoping of the `_fj` stubs in `parc`/`frc_fail` is handled correctly. No security concerns; no CI-workflow changes; no scope creep. --- _{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 sha=cbb5743b10e7c75171a83d7cd766961d3b8a821c verdict=COMMENT ci=success -->

igor added spent time 2026-07-25 17:41:28 +00:00

2 minutes 30 seconds

igor requested review from joshtronic 2026-07-25 17:41:28 +00:00

igor referenced this pull request 2026-07-26 02:15:47 +00:00

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

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

Author

Collaborator

Copy link

Superseded by #429, which carries both commits from this branch unchanged plus a _fj give-up diagnostic that closes #424.

The review point from the comment above is resolved on this branch already -- verified rather than assumed, by re-timing a live 404 through _fj:

this branch:            rc=22   0.30s
before the scoping:     rc=22   2.89s

No changes were needed to either of your commits. Opened as a separate branch only because pushing onto an open bot PR re-triggers the rework loop.

Closing in favour of #429. Branch retained.

Superseded by #429, which carries both commits from this branch **unchanged** plus a `_fj` give-up diagnostic that closes #424. The review point from the comment above is resolved on this branch already -- verified rather than assumed, by re-timing a live 404 through `_fj`: ``` this branch: rc=22 0.30s before the scoping: rc=22 2.89s ``` No changes were needed to either of your commits. Opened as a separate branch only because pushing onto an open bot PR re-triggers the rework loop. Closing in favour of #429. Branch retained.

igor closed this pull request 2026-07-26 02:15:59 +00:00

igor referenced this pull request 2026-07-26 02:54:17 +00:00

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

All checks were successful

Hide checks

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

Required

Details

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

Required

Details

Pull request closed

This pull request cannot be reopened because the branch was deleted.

Sign in to join this conversation.

Reviewers

No reviewers

joshtronic

Labels

Clear labels   

Agent

  

Compat/Breaking

Breaking change that won't be backward compatible

  

Kind/Bug

Something is not working

  

Kind/Documentation

Documentation changes

  

Kind/Enhancement

Improve existing functionality

  

Kind/Feature

New functionality

  

Kind/Security

This is security issue

  

Kind/Testing

Issue or pull request related to testing

  

Priority

Critical

The priority is critical

  

Priority

High

The priority is high

  

Priority

Low

The priority is low

  

Priority

Medium

The priority is medium

  

Reviewed

Confirmed

Issue has been confirmed

  

Reviewed

Duplicate

This issue or pull request already exists

  

Reviewed

Invalid

Invalid issue

  

Reviewed

Won't Fix

This issue won't be fixed

  

Status

Abandoned

Somebody has started to work on this but abandoned work

  

Status

Blocked

Something is blocking this issue or pull request

  

Status

Need More Info

Feedback is required to reproduce issue or to continue work

No labels

Agent

Compat/Breaking

Kind/Bug

Kind/Documentation

Kind/Enhancement

Kind/Feature

Kind/Security

Kind/Testing

Priority

Critical

Priority

High

Priority

Low

Priority

Medium

Reviewed

Confirmed

Reviewed

Duplicate

Reviewed

Invalid

Reviewed

Won't Fix

Status

Abandoned

Status

Blocked

Status

Need More Info

Milestone

Clear milestone

No items

No milestone

Assignees

Clear assignees

No assignees

1 participant

Notifications

Subscribe

Total time spent: 9 minutes 39 seconds

igor

9 minutes 39 seconds

Due date

The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set

Reference

joshtronic/igor!426

No description provided.