fix: validation-gate the review's rework loop (don't autonomously push to an unvalidated repo) #242

New issue

Closed

opened 2026-06-25 01:39:36 +00:00 by igor · 0 comments

igor commented

2026-06-25 01:39:36 +00:00

Collaborator

Problem

The binding review (3b) exposed a gap: the harness REVIEWED and then
REWORKED + PUSHED to an UNVALIDATED repo (stonks#2), violating the core rule
"validation gates WORK (issue pickup, PR pushes)".

Chain (all confirmed in code):

do_review_tick runs on the ANALYSIS set -- every bot-accessible repo, NOT
just validated ones. That's by design (a verdict is information, useful
everywhere). Fine.
On REQUEST_CHANGES it does forgejo_assign "$target_repo" "$target_num" "$BOT_USER" with no validation check.
The PR-review pickup's Signal 2 (forgejo_my_assigned_prs ->
/repos/issues/search?type=pulls&state=open&assigned=true) is not
validation-filtered -- unlike Signal 1, which iterates
VALIDATED_REPOS_JSON.
So: an autonomous RC verdict on an unvalidated repo -> bot assigned -> Signal 2
picks it up -> reworks -> pushes. stonks (no CI, never validated) got a pushed
commit it should never have received.

Fix -- validation-gate the WORK, not the review

The review still RUNS everywhere (info); it just must not drive autonomous WORK
(a rework pushes) on an unvalidated repo.

do_review_tick, the REQUEST_CHANGES branch: only assign-the-bot (drive
the rework loop) when the target repo is VALIDATED. Use
maintenance_repo_validated "$target_repo" (it checks VALIDATED_REPOS_JSON,
already built before the cascade). On an UNVALIDATED repo, treat RC like the
round-cap escalation -- request the human reviewer (with a note: repo isn't
validated, so the change can't be CI-verified / autonomously reworked) instead
of assigning the bot. Don't leave the PR stuck. New shape of the RC branch:
if validated AND rework_rounds < 3 -> assign bot (rework); else -> request human.
PR-review pickup Signal 2 (defense-in-depth): filter the
forgejo_my_assigned_prs results to repos present in VALIDATED_REPOS_JSON
before selecting one to rework. A bot-assigned PR on an unvalidated repo (e.g.
a manual reassignment) must not be autonomously reworked+pushed -- same rule.
Signal 1 is already validated-only; make Signal 2 match.

Watch out for

Do NOT gate the review itself -- it stays on the analysis set. Only the
assign-bot / rework-push is gated. Posting the verdict + requesting the human
are fine on any repo.
A repo unvalidated due to an open onboarding ticket gets the same treatment
(no autonomous rework); the normal flow resumes once it validates.
Keep the manual assignment-dance working for VALIDATED repos (Signal 2 there is
unchanged).
maintenance_repo_validated already exists (lib in tick.sh ~L1195) -- reuse it,
don't reimplement the membership check.

Definition of done

An RC verdict on an UNVALIDATED repo requests the human and does NOT assign the
bot / rework / push.
An RC verdict on a VALIDATED repo still drives the rework loop (unchanged).
Signal 2 only reworks bot-assigned PRs on validated repos.
Regression intent: a bot PR on a stonks-like (no-CI, unvalidated) repo gets a
verdict + human request, never a push.
bash -n bin/tick.sh clean; bash bin/check-sync.sh passes.

## Problem The binding review (3b) exposed a gap: the harness REVIEWED and then REWORKED + PUSHED to an UNVALIDATED repo (stonks#2), violating the core rule "validation gates WORK (issue pickup, PR pushes)". Chain (all confirmed in code): - `do_review_tick` runs on the ANALYSIS set -- every bot-accessible repo, NOT just validated ones. That's by design (a verdict is information, useful everywhere). Fine. - On `REQUEST_CHANGES` it does `forgejo_assign "$target_repo" "$target_num" "$BOT_USER"` with **no validation check**. - The PR-review pickup's **Signal 2** (`forgejo_my_assigned_prs` -> `/repos/issues/search?type=pulls&state=open&assigned=true`) is **not validation-filtered** -- unlike Signal 1, which iterates `VALIDATED_REPOS_JSON`. - So: an autonomous RC verdict on an unvalidated repo -> bot assigned -> Signal 2 picks it up -> reworks -> pushes. stonks (no CI, never validated) got a pushed commit it should never have received. ## Fix -- validation-gate the WORK, not the review The review still RUNS everywhere (info); it just must not drive autonomous WORK (a rework pushes) on an unvalidated repo. 1. **`do_review_tick`, the `REQUEST_CHANGES` branch:** only assign-the-bot (drive the rework loop) when the target repo is VALIDATED. Use `maintenance_repo_validated "$target_repo"` (it checks `VALIDATED_REPOS_JSON`, already built before the cascade). On an UNVALIDATED repo, treat RC like the round-cap escalation -- **request the human reviewer** (with a note: repo isn't validated, so the change can't be CI-verified / autonomously reworked) instead of assigning the bot. Don't leave the PR stuck. New shape of the RC branch: `if validated AND rework_rounds < 3 -> assign bot (rework); else -> request human`. 2. **PR-review pickup Signal 2 (defense-in-depth):** filter the `forgejo_my_assigned_prs` results to repos present in `VALIDATED_REPOS_JSON` before selecting one to rework. A bot-assigned PR on an unvalidated repo (e.g. a manual reassignment) must not be autonomously reworked+pushed -- same rule. Signal 1 is already validated-only; make Signal 2 match. ## Watch out for - Do NOT gate the review itself -- it stays on the analysis set. Only the assign-bot / rework-push is gated. Posting the verdict + requesting the human are fine on any repo. - A repo unvalidated due to an open onboarding ticket gets the same treatment (no autonomous rework); the normal flow resumes once it validates. - Keep the manual assignment-dance working for VALIDATED repos (Signal 2 there is unchanged). - `maintenance_repo_validated` already exists (lib in tick.sh ~L1195) -- reuse it, don't reimplement the membership check. ## Definition of done - An RC verdict on an UNVALIDATED repo requests the human and does NOT assign the bot / rework / push. - An RC verdict on a VALIDATED repo still drives the rework loop (unchanged). - Signal 2 only reworks bot-assigned PRs on validated repos. - Regression intent: a bot PR on a stonks-like (no-CI, unvalidated) repo gets a verdict + human request, never a push. - `bash -n bin/tick.sh` clean; `bash bin/check-sync.sh` passes.

igor self-assigned this

2026-06-25 01:41:00 +00:00

igor referenced this issue from a pull request that will close it,

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

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

igor added spent time

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

2 minutes 30 seconds

igor removed their assignment

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