fix(review): finish the #315 ban lift -- stop hard-rejecting workflow changes #318

Merged

joshtronic merged 1 commit from fix/review-directive-workflows into master

2026-07-02 03:29:11 +00:00

igor commented

2026-07-02 03:26:26 +00:00

Collaborator

The actual root cause

#315 lifted the workflow ban — but only in two of the three places it lives:

✅ AGENTS.md (the author directive)
✅ list_offlimits_violations (the push guard)
❌ bin/lib/review-directive.md — the SHADOW REVIEWER's rubric, which still says "any .forgejo/workflows/ change is a hard REQUEST_CHANGES."

So the reviewer kept hard-rejecting parsley's CI PRs (its verdict on #9 said exactly that), and the rework loop "resolved" the RC by deleting the validate workflow — on #8 AND #9. That's why parsley has no CI and it "won't magically show up." My fault: I lifted the ban incompletely, then reported "done" without verifying master.

Fix

Bring the review directive in line with the lifted ban: a workflow change is no longer an automatic RC. Scrutinize it hard (secrets, gate-weakening, plainly wrong) but don't reject it just for touching CI.

After this merges

Once this deploys, the reviewer stops sabotaging workflow PRs. Then I'll re-open parsley's CI PR and it will finally stick — and I will verify validate.yml is on master and the repo genuinely validates before I say parsley is done. No more claiming done off a PR or a checkmark.

Follow-up (noted, not in this PR)

The rework loop's instinct — "resolve an RC by deleting the contested change" — is dangerous beyond workflows. Worth a guard so it can't strip a change to go green. (logwatch already flagged the parsley#8 instance as igor#316.)

check-sync green.

## The actual root cause #315 lifted the workflow ban — but only in **two** of the **three** places it lives: - ✅ `AGENTS.md` (the author directive) - ✅ `list_offlimits_violations` (the push guard) - ❌ **`bin/lib/review-directive.md`** — the SHADOW REVIEWER's rubric, which still says *"any `.forgejo/workflows/` change is a hard REQUEST_CHANGES."* So the reviewer kept hard-rejecting parsley's CI PRs (its verdict on #9 said exactly that), and the rework loop "resolved" the RC by **deleting the validate workflow** — on #8 AND #9. That's why parsley has no CI and it "won't magically show up." My fault: I lifted the ban incompletely, then reported "done" without verifying master. ## Fix Bring the review directive in line with the lifted ban: a workflow change is no longer an automatic RC. Scrutinize it hard (secrets, gate-weakening, plainly wrong) but don't reject it just for touching CI. ## After this merges Once this deploys, the reviewer stops sabotaging workflow PRs. Then I'll re-open parsley's CI PR and it will finally stick — and I will **verify `validate.yml` is on master and the repo genuinely validates** before I say parsley is done. No more claiming done off a PR or a checkmark. ## Follow-up (noted, not in this PR) The rework loop's instinct — "resolve an RC by deleting the contested change" — is dangerous beyond workflows. Worth a guard so it can't strip a change to go green. (logwatch already flagged the parsley#8 instance as igor#316.) `check-sync` green.

igor added 1 commit

2026-07-02 03:26:26 +00:00

fix(review): stop hard-rejecting workflow changes -- finish the #315 ban lift

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

Details

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

Details

22e7bd261c

#315 lifted the CI-workflow ban in AGENTS.md + list_offlimits_violations, but
MISSED bin/lib/review-directive.md, which still told the shadow reviewer that
any .forgejo/workflows/ change is a hard REQUEST_CHANGES. So the reviewer kept
rejecting legitimate CI additions, and the rework loop 'resolved' the rejection
by DELETING the workflow -- stripping parsley's validate.yml on both #8 and #9
before merge. That's why parsley still has no CI.

Bring the review directive in line with the lifted ban: a workflow change is no
longer an automatic RC; scrutinize it hard (secrets/gate-weakening/plainly-
wrong) but don't reject it merely for touching CI.

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

igor requested review from joshtronic

2026-07-02 03:26:28 +00:00

igor commented

2026-07-02 03:27:13 +00:00

Author

Collaborator

🤖 Review — `APPROVE` (automated)

CI for 22e7bd26: success

Single-hunk doc change to bin/lib/review-directive.md that replaces the hard "CI config is off-limits → REQUEST_CHANGES" rubric line with a "allowed but scrutinized" policy, aligning the shadow reviewer's directive with the ban lift described in #315.

In scope & honest. The diff does exactly what the description claims: one paragraph in the review directive, ~14 lines. No drive-by edits, no unrelated churn. The checklist/narrative matches the diff.
Not a forbidden CI-config change. The edited file is bin/lib/review-directive.md (reviewer rubric text), not anything under .forgejo/workflows/ or .github/workflows/, so the operator-managed-workflow prohibition doesn't apply here.
Policy content is sane. The replacement still requires REQUEST_CHANGES for secret exfiltration, gate-weakening, running untrusted input with credentials, or plainly-wrong workflows — it only removes the reflexive "touches CI = reject." No security regression in the wording itself.
CI: success. No tests are meaningfully applicable to a prose directive.

Note (non-blocking): I can't independently verify from this diff that #315 actually lifted the ban in AGENTS.md and list_offlimits_violations — I'm taking that on the author's word. If those two weren't actually loosened, this change would leave the three sources inconsistent in the other direction. Worth a glance by the human, but nothing in this diff is wrong on its own terms.


---
<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>
<!-- review sha=22e7bd261c53c22d19ec22249de9883904763c16 verdict=APPROVE ci=success -->

### 🤖 Review — `APPROVE` _(automated)_ CI for `22e7bd26`: **success** Single-hunk doc change to `bin/lib/review-directive.md` that replaces the hard "CI config is off-limits → REQUEST_CHANGES" rubric line with a "allowed but scrutinized" policy, aligning the shadow reviewer's directive with the ban lift described in #315. - **In scope & honest.** The diff does exactly what the description claims: one paragraph in the review directive, ~14 lines. No drive-by edits, no unrelated churn. The checklist/narrative matches the diff. - **Not a forbidden CI-config change.** The edited file is `bin/lib/review-directive.md` (reviewer rubric text), not anything under `.forgejo/workflows/` or `.github/workflows/`, so the operator-managed-workflow prohibition doesn't apply here. - **Policy content is sane.** The replacement still requires REQUEST_CHANGES for secret exfiltration, gate-weakening, running untrusted input with credentials, or plainly-wrong workflows — it only removes the reflexive "touches CI = reject." No security regression in the wording itself. - **CI:** `success`. No tests are meaningfully applicable to a prose directive. Note (non-blocking): I can't independently verify from this diff that #315 actually lifted the ban in `AGENTS.md` and `list_offlimits_violations` — I'm taking that on the author's word. If those two weren't actually loosened, this change would leave the three sources inconsistent in the other direction. Worth a glance by the human, but nothing in this diff is wrong on its own terms. ``` --- <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-02 03:27:14 +00:00

17 seconds