refactor: restrict PR review tick to validated repos only #245

Merged

joshtronic merged 1 commit from agent/244-fix-gate-the-shadow-review-to-validated-repos-don into master

2026-06-25 04:10:26 +00:00

igor commented

2026-06-25 03:44:12 +00:00

Collaborator

What this PR does

refactor: restrict PR review tick to validated repos only
Remove review pass from analysis/unvalidated repos; skip repos failing maintenance_repo_validated check
Update CLAUDE.md and inline comments to reflect the new validated-set-only behavior

Test plan

No new tests added; logic change is a single guard (maintenance_repo_validated "$repo" || continue) matching updated docs
Manually confirm that onboarding-open or unvalidated repos are skipped during a live do_review_tick run

Closes #244

## What this PR does - [x] refactor: restrict PR review tick to validated repos only - [x] Remove review pass from analysis/unvalidated repos; skip repos failing `maintenance_repo_validated` check - [x] Update CLAUDE.md and inline comments to reflect the new validated-set-only behavior ## Test plan - [x] No new tests added; logic change is a single guard (`maintenance_repo_validated "$repo" || continue`) matching updated docs - [ ] Manually confirm that onboarding-open or unvalidated repos are skipped during a live `do_review_tick` run Closes #244

igor added 1 commit

2026-06-25 03:44:12 +00:00

refactor: restrict PR review tick to validated repos only

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

Details

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

Details

01747d3503

igor commented

2026-06-25 04:08:47 +00:00

Author

Collaborator

🤖 Review — `COMMENT` (automated)

CI for 01747d35: success

Summary: Adds a single guard (maintenance_repo_validated "$repo" || continue) to do_review_tick so the PR-review pass only runs on the validated set, plus matching doc/comment updates. The diff is small, in scope, honestly described, and CI is green. My reservations are about intent and test coverage, not a clear defect — so this goes back to a human.

Findings

Deliberate design reversal — worth a human gut-check (bin/tick.sh ~2425, CLAUDE.md ~263). The prior comment made an explicit argument for running review on the ANALYSIS set: "a verdict is pure information and useful on EVERY repo (this harness's own PRs included)." This PR reverses exactly that decision. The key question the human should answer: is the harness's own repo in the validated set? If not, this change silently stops Igor from reviewing its own PRs — which the old code called out as a value. The new rationale ("can never become a merge signal") is plausible, but it directly contradicts the documented intent it replaces, so someone who knows whether self-review is intended should sign off.
Guard placement is correct. It sits inside the outer while read repo_line loop after repo is extracted; || continue advances to the next repo. Logic looks right.
No test for the new behavior (contract gap). The author is honest about this (test-plan checkbox checked: "No new tests added… single guard"; manual-confirm left unchecked). But this is a behavior change — a class of repos that previously got reviewed no longer will. CI being green only tells us nothing currently asserts the old behavior, not that the new behavior is verified. If the suite has any existing coverage of do_review_tick, a case asserting "an unvalidated repo is skipped" would lock this in cheaply. Recommend the human decide whether a regression test is warranted.

Could not verify from the diff

That maintenance_repo_validated exists with the expected semantics (truthy = validated) and has no side effects — it's referenced as an established function but isn't shown.
Whether the harness's own repo / onboarding-open repos are intended to lose review. This is the load-bearing assumption of the whole change.

No security, scope, or CI-config concerns. The checklist is honest and matches the diff.

_{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 `01747d35`: **success** **Summary:** Adds a single guard (`maintenance_repo_validated "$repo" || continue`) to `do_review_tick` so the PR-review pass only runs on the validated set, plus matching doc/comment updates. The diff is small, in scope, honestly described, and CI is green. My reservations are about intent and test coverage, not a clear defect — so this goes back to a human. **Findings** - **Deliberate design reversal — worth a human gut-check (bin/tick.sh ~2425, CLAUDE.md ~263).** The prior comment made an *explicit* argument for running review on the ANALYSIS set: "a verdict is pure information and useful on EVERY repo (this harness's own PRs included)." This PR reverses exactly that decision. The key question the human should answer: **is the harness's own repo in the validated set?** If not, this change silently stops Igor from reviewing its own PRs — which the old code called out as a value. The new rationale ("can never become a merge signal") is plausible, but it directly contradicts the documented intent it replaces, so someone who knows whether self-review is intended should sign off. - **Guard placement is correct.** It sits inside the outer `while read repo_line` loop after `repo` is extracted; `|| continue` advances to the next repo. Logic looks right. - **No test for the new behavior (contract gap).** The author is honest about this (test-plan checkbox checked: "No new tests added… single guard"; manual-confirm left unchecked). But this *is* a behavior change — a class of repos that previously got reviewed no longer will. CI being green only tells us nothing currently asserts the old behavior, not that the new behavior is verified. If the suite has any existing coverage of `do_review_tick`, a case asserting "an unvalidated repo is skipped" would lock this in cheaply. Recommend the human decide whether a regression test is warranted. **Could not verify from the diff** - That `maintenance_repo_validated` exists with the expected semantics (truthy = validated) and has no side effects — it's referenced as an established function but isn't shown. - Whether the harness's own repo / onboarding-open repos are intended to lose review. This is the load-bearing assumption of the whole change. No security, scope, or CI-config concerns. The checklist is honest and matches the diff. --- <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 04:08:48 +00:00

54 seconds

igor requested review from joshtronic

2026-06-25 04:08:48 +00:00