fix: validation-gate the review's rework loop so unvalidated repos never receive autonomous pushes #243

Merged

joshtronic merged 1 commit from agent/242-fix-validation-gate-the-review-s-rework-loop-don into master

2026-06-25 01:46:17 +00:00

igor commented

2026-06-25 01:43:46 +00:00

Collaborator

What this PR does

fix: validation-gate the review's rework loop so unvalidated repos never receive autonomous pushes
In do_review_tick, check maintenance_repo_validated before assigning the bot on REQUEST_CHANGES; unvalidated repos get a human-reviewer request with an explanatory comment instead
Signal 2 (assignment dance) now filters forgejo_my_assigned_prs to validated repos only, matching Signal 1's existing behavior

Test plan

bash -n bin/tick.sh passes (syntax clean)
bash bin/check-sync.sh passes (AGENTS.md <-> tick.sh contract intact)
Manual: trigger a REQUEST_CHANGES verdict on a repo not in VALIDATED_REPOS_JSON -- verify the bot posts the "not validated" comment and requests the human reviewer, does NOT assign itself
Manual: confirm a REQUEST_CHANGES verdict on a validated repo still assigns the bot and drives the rework loop as before

Closes #242

## What this PR does - [x] fix: validation-gate the review's rework loop so unvalidated repos never receive autonomous pushes - [x] In `do_review_tick`, check `maintenance_repo_validated` before assigning the bot on REQUEST_CHANGES; unvalidated repos get a human-reviewer request with an explanatory comment instead - [x] Signal 2 (assignment dance) now filters `forgejo_my_assigned_prs` to validated repos only, matching Signal 1's existing behavior ## Test plan - [x] `bash -n bin/tick.sh` passes (syntax clean) - [x] `bash bin/check-sync.sh` passes (AGENTS.md <-> tick.sh contract intact) - [ ] Manual: trigger a REQUEST_CHANGES verdict on a repo not in `VALIDATED_REPOS_JSON` -- verify the bot posts the "not validated" comment and requests the human reviewer, does NOT assign itself - [ ] Manual: confirm a REQUEST_CHANGES verdict on a validated repo still assigns the bot and drives the rework loop as before Closes #242

igor added 1 commit

2026-06-25 01:43:46 +00:00

fix: validation-gate the review's rework loop so unvalidated repos never receive autonomous pushes

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

Details

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

Details

9347732a93

joshtronic merged commit be6498abbc into master

2026-06-25 01:46:17 +00:00

joshtronic deleted branch agent/242-fix-validation-gate-the-review-s-rework-loop-don

2026-06-25 01:46:17 +00:00

joshtronic referenced this pull request from a commit

2026-06-25 01:46:19 +00:00

Merge pull request 'fix: validation-gate the review's rework loop so unvalidated repos never receive autonomous pushes' (#243) from agent/242-fix-validation-gate-the-review-s-rework-loop-don into master

igor commented

2026-06-25 01:48:18 +00:00

Author

Collaborator

🤖 Review — `COMMENT` (automated)

CI for 9347732a: success

Adds two validation gates to bin/tick.sh: (1) a new unvalidated-repo branch in do_review_tick's REQUEST_CHANGES case that hands off to a human instead of self-assigning, and (2) a filter in Signal 2's assignment dance so only validated assigned PRs are reworked. The code itself is clean and the change only ever restricts autonomous behavior (no path expands it), so it's low-risk — but a couple of things I can't confirm from the diff alone are worth a human glance.

What's solid

Signal 2 uses done < <(jq -c '.[]' ...) process substitution, which correctly keeps the while loop in the current shell so REVIEW_PR assignment persists — the right pattern.
maintenance_repo_validated "$target_repo" && repo_validated=true is set -e-safe (the failing command precedes the final &&).
local repo_validated=false is fine since this is inside do_review_tick.
Validated-repo behavior is preserved untouched; no regression for the existing rework/escalation path.
Scope is tight (~30 lines, one issue), CI is green, and the checklist is honest — the three checked items all correspond to real diff content, and the unverified manual items are correctly left unchecked.

Reservations I'd want you to weigh

Reachability of change 1. Signal 1 iterates <<<"$VALIDATED_REPOS_JSON" and the change-2 comment asserts that's "the same rule as Signal 1." If both signals already exclude unvalidated repos, can target_repo in do_review_tick ever actually be unvalidated? If not, the new branch is harmless defense-in-depth rather than the operative fix; if the fix depends on it firing, confirm what path delivers an unvalidated repo into do_review_tick. (I can't see Signal 1's loop body or the definition of VALIDATED_REPOS_JSON to settle this.)
Idempotency of the new branch. Unlike the rc_rounds >= 3 escalation (which is gated by tracked round state), the unvalidated branch posts a comment + requests review unconditionally every time it's hit. If the review selector can re-pick the same PR on later ticks, this could re-post "Handing to you" repeatedly. Presumably there's upstream dedup that also protects the existing escalation path — worth confirming that protection covers this branch too.
Behavior is unverified. The two automated checks in the test plan (bash -n, check-sync.sh) confirm syntax and the AGENTS.md contract, but neither exercises the actual gating logic. Both manual verification items are unchecked, so the central claims — "posts the comment and requests the human, does NOT self-assign" and "validated repos still drive the rework loop" — have not been demonstrated. Given it's a bash script, a manual run-through (or a sample assigned-PRs payload through the Signal 2 loop) before merge would be reassuring.

Nothing here is a hard blocker, but the reachability/idempotency questions plus the unrun behavioral tests are exactly the kind of thing I'd rather you confirm than rubber-stamp.

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

### 🤖 Review — `COMMENT` _(automated)_ CI for `9347732a`: **success** Adds two validation gates to `bin/tick.sh`: (1) a new unvalidated-repo branch in `do_review_tick`'s `REQUEST_CHANGES` case that hands off to a human instead of self-assigning, and (2) a filter in Signal 2's assignment dance so only validated assigned PRs are reworked. The code itself is clean and the change only ever *restricts* autonomous behavior (no path expands it), so it's low-risk — but a couple of things I can't confirm from the diff alone are worth a human glance. **What's solid** - Signal 2 uses `done < <(jq -c '.[]' ...)` process substitution, which correctly keeps the `while` loop in the current shell so `REVIEW_PR` assignment persists — the right pattern. - `maintenance_repo_validated "$target_repo" && repo_validated=true` is `set -e`-safe (the failing command precedes the final `&&`). - `local repo_validated=false` is fine since this is inside `do_review_tick`. - Validated-repo behavior is preserved untouched; no regression for the existing rework/escalation path. - Scope is tight (~30 lines, one issue), CI is green, and the checklist is honest — the three checked items all correspond to real diff content, and the unverified manual items are correctly left unchecked. **Reservations I'd want you to weigh** - **Reachability of change 1.** Signal 1 iterates `<<<"$VALIDATED_REPOS_JSON"` and the change-2 comment asserts that's "the same rule as Signal 1." If both signals already exclude unvalidated repos, can `target_repo` in `do_review_tick` ever actually be unvalidated? If not, the new branch is harmless defense-in-depth rather than the operative fix; if the fix depends on it firing, confirm what path delivers an unvalidated repo into `do_review_tick`. (I can't see Signal 1's loop body or the definition of `VALIDATED_REPOS_JSON` to settle this.) - **Idempotency of the new branch.** Unlike the `rc_rounds >= 3` escalation (which is gated by tracked round state), the unvalidated branch posts a comment + requests review unconditionally every time it's hit. If the review selector can re-pick the same PR on later ticks, this could re-post "Handing to you" repeatedly. Presumably there's upstream dedup that also protects the existing escalation path — worth confirming that protection covers this branch too. - **Behavior is unverified.** The two automated checks in the test plan (`bash -n`, `check-sync.sh`) confirm syntax and the AGENTS.md contract, but neither exercises the actual gating logic. Both manual verification items are unchecked, so the central claims — "posts the comment and requests the human, does NOT self-assign" and "validated repos still drive the rework loop" — have not been demonstrated. Given it's a bash script, a manual run-through (or a sample assigned-PRs payload through the Signal 2 loop) before merge would be reassuring. Nothing here is a hard blocker, but the reachability/idempotency questions plus the unrun behavioral tests are exactly the kind of thing I'd rather you confirm than rubber-stamp. --- <sub>Independent review by the harness on `claude-opus-4-8`. The human reviewer is requested once Igor has reviewed; a human still merges.</sub>

igor added spent time

2026-06-25 01:48:18 +00:00

2 minutes 26 seconds

igor requested review from joshtronic

2026-06-25 01:48:18 +00:00

igor referenced this pull request

2026-06-25 03:40:29 +00:00

fix: gate the shadow review to validated repos (don't review not-ready repos) #244