joshtronic/igor

Watch

fix(review): truthful, fault-tolerant review-request (#377) #379

Merged

joshtronic merged 2 commits from fix/review-request-idempotent into master

2026-07-11 21:44:26 +00:00

igor commented

2026-07-11 05:24:15 +00:00

Collaborator

Fixes the intermittent review-request to joshtronic failed warning on APPROVE verdicts.

Root cause (verified)

do_review_tick logged requested review from joshtronic unconditionally, right after a request that could fail — hence the "warning-then-requested" pair in the journal. And a live probe showed re-requesting an already-requested reviewer is a harmless Forgejo no-op (HTTP 201, empty body) — so the real failures are transient (5xx / timeout), not "already requested."

Fix

forgejo_request_review captures HTTP status+body (via a stubbable _forgejo_post_reviewers seam), retries once on a transient code (5xx / curl 000), and on persistent failure surfaces the status+body + returns non-zero.
New review_request_human wrapper logs the outcome truthfully — a success line only when the request lands, else one warning carrying the API reason. Replaces the three duplicated request-then-log blocks (APPROVE/COMMENT, unvalidated, escalation).

Tests

bin/test-forgejo.sh: success / transient-retry / persistent-error / client-error. make test green; shellcheck clean on changed lines.

Note

WIP draft on purpose so the live review loop leaves it alone overnight. Touches lib/forgejo.sh + bin/test-forgejo.sh like #376 does but in disjoint regions — if #376 merges first this may want a trivial rebase; I'll keep it current.

Closes #377

Fixes the intermittent `review-request to joshtronic failed` warning on APPROVE verdicts. ## Root cause (verified) `do_review_tick` logged `requested review from joshtronic` **unconditionally**, right after a request that could fail — hence the "warning-then-requested" pair in the journal. And a live probe showed re-requesting an already-requested reviewer is a **harmless Forgejo no-op (HTTP 201, empty body)** — so the real failures are **transient** (5xx / timeout), not "already requested." ## Fix - `forgejo_request_review` captures HTTP status+body (via a stubbable `_forgejo_post_reviewers` seam), **retries once** on a transient code (5xx / curl `000`), and on persistent failure **surfaces the status+body** + returns non-zero. - New `review_request_human` wrapper logs the outcome **truthfully** — a success line only when the request lands, else one warning carrying the API reason. Replaces the three duplicated request-then-log blocks (APPROVE/COMMENT, unvalidated, escalation). ## Tests `bin/test-forgejo.sh`: success / transient-retry / persistent-error / client-error. `make test` green; shellcheck clean on changed lines. ## Note **WIP draft on purpose** so the live review loop leaves it alone overnight. Touches `lib/forgejo.sh` + `bin/test-forgejo.sh` like #376 does but in disjoint regions — if #376 merges first this may want a trivial rebase; I'll keep it current. Closes #377

igor added 1 commit

2026-07-11 05:24:15 +00:00

fix(review): truthful review-request logging + fault-tolerant request

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

Details

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

Details

6461f5e142

do_review_tick logged "requested review from <human>" UNCONDITIONALLY right
after a request that could have failed, so a failed review-request produced a
contradictory warning-then-"requested" pair in the journal (#377).

Empirically re-requesting an already-requested reviewer is a harmless Forgejo
no-op (HTTP 201, empty body) -- so the intermittent failures are transient
(5xx / timeout), not "already requested."

- forgejo_request_review captures the HTTP status+body (via a stubbable
  _forgejo_post_reviewers seam), retries ONCE on a transient code (5xx /
  curl 000), and on persistent failure emits the status+body on stderr and
  returns non-zero -- so the caller can log WHY, not a bare warning.
- New review_request_human wrapper logs the outcome truthfully: a success line
  only when the request actually lands, otherwise ONE warning carrying the API
  reason. Replaces the three duplicated request-then-log blocks in
  do_review_tick (APPROVE/COMMENT, unvalidated, escalation).

Tests: forgejo_request_review success / transient-retry / persistent-error /
client-error in bin/test-forgejo.sh.

Closes #377

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Q4YUVfbfUWdUwifGJ2TvkP

igor changed title from ~~WIP: fix(review): truthful, fault-tolerant review-request (#377)~~ to fix(review): truthful, fault-tolerant review-request (#377)

2026-07-11 21:31:01 +00:00

igor commented

2026-07-11 21:35:04 +00:00

Author

Collaborator

🤖 Review — `COMMENT` (automated)

CI for 6461f5e1: success

Truthful/fault-tolerant review-request refactor: splits the HTTP POST into a stubbable seam, retries once on transient (5xx/000) codes, surfaces the real HTTP status+body on persistent failure, and consolidates three duplicated request-then-log blocks into review_request_human. Small (~90 lines), focused on #377, CI green. The retry/classification logic is correct and well-covered.

Strengths

forgejo_request_review state machine is right: 2*→rc 0, 5*|000→retry once then fail, everything else→immediate fail-no-retry. The four test cases (success / transient-retry / persistent-5xx / client-422) exercise each path, including asserting exactly 2 attempts.
resp parsing (${resp##*$'\n'} for code, ${resp%$'\n'*} for body) handles empty-body responses correctly since -w $'\n%{http_code}' always appends the code as the last line.
review_request_human now only logs "requested" when the request actually landed — the stated bug (unconditional success log after a failable request) is genuinely fixed. Checklist matches the diff; tests are present.

One thing a human should confirm (why this is COMMENT, not APPROVE)

_forgejo_post_reviewers bypasses the existing _fj helper and hand-builds the URL as "$FORGEJO_URL/api/v1/repos/${repo}/pulls/${number}/requested_reviewers". The old code was _fj POST "/repos/...". Because the tests stub the seam, the real URL string is never exercised — a mismatch with _fj's actual base (trailing slash, or _fj not prepending exactly /api/v1) would 404 in production while every test stays green. I can't see _fj/FORGEJO_URL definitions in this diff to verify they agree. Please confirm the constructed URL matches what _fj produces.

Minor

The error string caps body at 200 chars via tr '\n' ' ' | head -c 200 — fine; no secret leakage risk since FORGEJO_TOKEN is only in the request header, never echoed.
PR is intentionally a WIP/draft per the description, so it won't auto-merge — no concern, just noting the approve/merge gate is moot until it's un-drafted.

No blocking defect found; the only open question is the untested real-URL construction, which is worth a human glance before merge.

_{Independent review by the harness on claude-opus-4-8 (effort: high). The human reviewer is requested once Igor has reviewed; a human still merges.}

### 🤖 Review — `COMMENT` _(automated)_ CI for `6461f5e1`: **success** Truthful/fault-tolerant review-request refactor: splits the HTTP POST into a stubbable seam, retries once on transient (5xx/000) codes, surfaces the real HTTP status+body on persistent failure, and consolidates three duplicated request-then-log blocks into `review_request_human`. Small (~90 lines), focused on #377, CI green. The retry/classification logic is correct and well-covered. **Strengths** - `forgejo_request_review` state machine is right: `2*`→rc 0, `5*|000`→retry once then fail, everything else→immediate fail-no-retry. The four test cases (success / transient-retry / persistent-5xx / client-422) exercise each path, including asserting exactly 2 attempts. - `resp` parsing (`${resp##*$'\n'}` for code, `${resp%$'\n'*}` for body) handles empty-body responses correctly since `-w $'\n%{http_code}'` always appends the code as the last line. - `review_request_human` now only logs "requested" when the request actually landed — the stated bug (unconditional success log after a failable request) is genuinely fixed. Checklist matches the diff; tests are present. **One thing a human should confirm (why this is COMMENT, not APPROVE)** - `_forgejo_post_reviewers` bypasses the existing `_fj` helper and hand-builds the URL as `"$FORGEJO_URL/api/v1/repos/${repo}/pulls/${number}/requested_reviewers"`. The old code was `_fj POST "/repos/..."`. Because the tests **stub the seam**, the real URL string is never exercised — a mismatch with `_fj`'s actual base (trailing slash, or `_fj` not prepending exactly `/api/v1`) would 404 in production while every test stays green. I can't see `_fj`/`FORGEJO_URL` definitions in this diff to verify they agree. Please confirm the constructed URL matches what `_fj` produces. **Minor** - The error string caps body at 200 chars via `tr '\n' ' ' | head -c 200` — fine; no secret leakage risk since `FORGEJO_TOKEN` is only in the request header, never echoed. - PR is intentionally a WIP/draft per the description, so it won't auto-merge — no concern, just noting the approve/merge gate is moot until it's un-drafted. No blocking defect found; the only open question is the untested real-URL construction, which is worth a human glance before merge. --- <sub>Independent review by the harness on `claude-opus-4-8` (effort: high). The human reviewer is requested once Igor has reviewed; a human still merges.</sub>

igor added spent time

2026-07-11 21:35:04 +00:00

55 seconds

igor requested review from joshtronic

2026-07-11 21:35:04 +00:00

igor added 1 commit

2026-07-11 21:42:47 +00:00

Merge remote-tracking branch 'origin/master' into fix/review-request-idempotent

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

Details

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

Details

480b4b75b4

# Conflicts:
#	bin/test-forgejo.sh

joshtronic approved these changes

2026-07-11 21:44:12 +00:00

joshtronic merged commit fc8c7f58cf into master

2026-07-11 21:44:26 +00:00

joshtronic referenced this pull request from a commit

2026-07-11 21:44:26 +00:00

Merge pull request 'fix(review): truthful, fault-tolerant review-request (#377)' (#379) from fix/review-request-idempotent into master

joshtronic deleted branch fix/review-request-idempotent

2026-07-11 21:44:26 +00:00

igor commented

2026-07-11 21:45:10 +00:00

Author

Collaborator

🤖 Review — `COMMENT` (automated)

CI for 480b4b75: success

Clean, well-tested fix for the contradictory "warning-then-requested" journal pair: forgejo_request_review now classifies the HTTP result, retries once on transient (5xx/000), surfaces the real reason on persistent failure, and a new review_request_human wrapper logs the outcome truthfully. In scope (only the review-request path), small diff, honest checklist — the four described tests (success / transient-retry / persistent-error / client-error) all exist in the diff. CI green. My only reservations are verification gaps I can't close from the diff alone.

Logic — looks correct:

Response parse (code="${resp##*$'\n'}" / body="${resp%$'\n'*}") correctly splits curl's trailing \n%{http_code}, and handles the empty-body 201 case (\n201 → code 201, body empty).
Retry control flow is right: 2*→return 0; 5*|000 on attempt 1 → sleep+continue, on attempt 2 → falls through to break; 4xx skips the case arms and hits break immediately (no retry). Matches the 422 "no retry" test.
review_request_human captures 2>&1, and forgejo_request_review writes nothing to stdout on success, so the success/warning branches log cleanly.

Reservation 1 (main) — unverifiable URL base + new DRY divergence. The extracted _forgejo_post_reviewers hardcodes "$FORGEJO_URL/api/v1/repos/...", whereas the old code went through _fj POST "/repos/...". This bakes in an assumption that _fj prepends exactly $FORGEJO_URL/api/v1 (and that FORGEJO_URL carries no /api/v1 itself). I can't see _fj in this diff to confirm, and because every test stubs the _forgejo_post_reviewers seam, no test exercises the real curl/URL path — so a base-URL mismatch would pass CI and only surface in production. Please confirm the base matches _fj's convention; ideally the base construction would be shared rather than duplicated, so the two can't drift.

Reservation 2 (minor): --max-time 30 and the direct auth/content-type headers are now duplicated from whatever _fj does; same drift risk as above.

Non-blocking:

review_request_human re-checks FORGEJO_REVIEWER internally while all three call sites still guard with if [ -n "${FORGEJO_REVIEWER:-}" ] — harmless redundancy.
PR is marked WIP/draft on purpose; the human should un-draft before merging.

Nothing here is a defect I can point to as wrong — it's the untested real-HTTP seam plus the duplicated base URL that keep me from a confident APPROVE. A quick human glance at _fj's URL construction should clear it.

_{Independent review by the harness on claude-opus-4-8 (effort: high). The human reviewer is requested once Igor has reviewed; a human still merges.}

### 🤖 Review — `COMMENT` _(automated)_ CI for `480b4b75`: **success** Clean, well-tested fix for the contradictory "warning-then-requested" journal pair: `forgejo_request_review` now classifies the HTTP result, retries once on transient (5xx/000), surfaces the real reason on persistent failure, and a new `review_request_human` wrapper logs the outcome truthfully. In scope (only the review-request path), small diff, honest checklist — the four described tests (success / transient-retry / persistent-error / client-error) all exist in the diff. CI green. My only reservations are verification gaps I can't close from the diff alone. **Logic — looks correct:** - Response parse (`code="${resp##*$'\n'}"` / `body="${resp%$'\n'*}"`) correctly splits curl's trailing `\n%{http_code}`, and handles the empty-body 201 case (`\n201` → code `201`, body empty). - Retry control flow is right: `2*`→return 0; `5*|000` on attempt 1 → sleep+continue, on attempt 2 → falls through to `break`; `4xx` skips the case arms and hits `break` immediately (no retry). Matches the 422 "no retry" test. - `review_request_human` captures `2>&1`, and `forgejo_request_review` writes nothing to stdout on success, so the success/warning branches log cleanly. **Reservation 1 (main) — unverifiable URL base + new DRY divergence.** The extracted `_forgejo_post_reviewers` hardcodes `"$FORGEJO_URL/api/v1/repos/..."`, whereas the old code went through `_fj POST "/repos/..."`. This bakes in an assumption that `_fj` prepends exactly `$FORGEJO_URL/api/v1` (and that `FORGEJO_URL` carries no `/api/v1` itself). I can't see `_fj` in this diff to confirm, and because every test stubs the `_forgejo_post_reviewers` seam, **no test exercises the real curl/URL path** — so a base-URL mismatch would pass CI and only surface in production. Please confirm the base matches `_fj`'s convention; ideally the base construction would be shared rather than duplicated, so the two can't drift. **Reservation 2 (minor):** `--max-time 30` and the direct auth/content-type headers are now duplicated from whatever `_fj` does; same drift risk as above. **Non-blocking:** - `review_request_human` re-checks `FORGEJO_REVIEWER` internally while all three call sites still guard with `if [ -n "${FORGEJO_REVIEWER:-}" ]` — harmless redundancy. - PR is marked WIP/draft on purpose; the human should un-draft before merging. Nothing here is a defect I can point to as wrong — it's the untested real-HTTP seam plus the duplicated base URL that keep me from a confident APPROVE. A quick human glance at `_fj`'s URL construction should clear it. --- <sub>Independent review by the harness on `claude-opus-4-8` (effort: high). The human reviewer is requested once Igor has reviewed; a human still merges.</sub>

igor added spent time

2026-07-11 21:45:10 +00:00

1 minute 5 seconds