Skip to content

Commit 253157d

Browse files
TMogdansclaude
andcommitted
fix(verify-review): tier-aware §9 gating — T0/T1 need no approval (regression)
High Obol finding: verify-review was tier-blind and effectively required approval on every PR, losing §9 'T0/T1 -> auto-merge'. (The acute T1-stays-red symptom was already cleared by v0.2.4's pending-pass, but the tier was never consumed.) - verify-review now consumes the tier (from tier.json via the template): after the always-on gate-tamper check, T0/T1 -> success with NO approval (§9). Unknown tier defaults to T3 (conservative). T2/T3: approval-validity (bot/author/stale present -> fail) + pending-pass on missing (the authoritative T2/T3 block is branch protection's required CODEOWNER review). - template passes tier to verify-review. - USAGE: the §9 merge-gate is CODEOWNERS-by-path; tier-map T2/T3 paths MUST be ⊆ CODEOWNER paths (else a T2/T3 PR could auto-merge unreviewed) — documented as a hard requirement. Reconciles with the v0.2.4 stale-FAILURE fix (T2/T3 block stays out of a failing check). v0.2.5, 130 tests green. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01RjNiTNchAZoTmkmovVngnR
1 parent f1e0448 commit 253157d

7 files changed

Lines changed: 69 additions & 6 deletions

File tree

.claude-plugin/plugin.json

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,6 @@
11
{
22
"name": "devloop",
3-
"version": "0.2.4",
3+
"version": "0.2.5",
44
"description": "Generische agentische Dev-Loop-Kette: /devloop:specify|spec-to-tests|implement|critic|loop + Bindungs-Anker (Wächter-Vorbedingung, content-gebundene Stopps, CI-Required-Check). Projektübergreifend.",
55
"author": { "name": "Tobias Mogdans" }
66
}

USAGE.md

Lines changed: 9 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -43,9 +43,15 @@ Danach **von Hand** (das ist der Anker, der Selbst-Freigabe verhindert):
4343
Admin-Override statt eines normalen Reviews.
4444
- `.devloop/bot-logins.json` — die GitHub-Login(s) **deines Agenten** (damit seine
4545
„Approvals" nie als Mensch zählen).
46-
5. **CODEOWNERS auf das Spec-Verzeichnis** (z.B. `/.specify/specs/` bzw. wo deine Specs liegen) →
47-
erzwingt das Spec-Review per Branch-Protection, tier-unabhängig, ohne Admin-Override. (Ohne das
48-
würde ein Spec-only-Diff zu T0/T1 ableiten und ohne Review auto-mergen — der Spec-Review ginge verloren.)
46+
5. **CODEOWNERS = das §9-Merge-Tor (wichtig).** CODEOWNERS muss decken:
47+
- das **Spec-Verzeichnis** (z.B. `/.specify/specs/`) → erzwingt den Spec-Review-Stopp, tier-unabhängig;
48+
- **alle T2/T3-Pfade aus deiner `tier-map`** (auth, migrations, contracts, `src/**`, …).
49+
So funktioniert §9 serverseitig: **T0/T1 berührt keine CODEOWNER-Pfade → kein Review nötig → Auto-Merge;
50+
T2/T3 berührt CODEOWNER-Pfade → required Review.** `verify-review` failt **nicht** auf „noch nicht
51+
approved" (sonst hinge ein veralteter FAILURE-Lauf, s. v0.2.4) — der T2/T3-Block kommt aus CODEOWNERS.
52+
> **Drift-Achtung:** Die **tier-map-T2/T3-Pfade müssen ⊆ CODEOWNER-Pfade** sein. Klafft das auseinander,
53+
> kann ein T2/T3-PR ohne Review auto-mergen. `verify-review` ist tier-bewusst (T0/T1 grün, Gate-Tamper +
54+
> Approval-Gültigkeit immer), erzwingt das „muss approved sein" aber **nicht** selbst — das tut CODEOWNERS.
4955
6. **Trace-/Coverage-Gate muss `.skip`'te Tests als Abdeckung zählen** (Regex über Quelltext).
5056
Davon hängt ab, dass der Spec-PR `main` grün hält. Wer das weghärtet, bricht das Spec-PR-Modell still.
5157
7. Sicherstellen, dass die **anderen drei Wächter** stehen: Mutation-Ratchet (Stryker),

dist/cli/verify-review.js

Lines changed: 9 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -19,6 +19,15 @@ if (req.diffPaths &&
1919
touchesProtectedSet(req.diffPaths, req.protectedGlobs)) {
2020
fail("protected-set-touched: the diff edits a guardian/gate config (reward-hacking alarm)");
2121
}
22+
// 2. Tier gate (§9): T0/T1 auto-merge — no approval required. (Gate-tamper above still applied.)
23+
// Unknown/absent tier defaults to conservative "T3" (approval required). The authoritative
24+
// T2/T3 merge block is branch protection's required CODEOWNER review on the high-tier paths;
25+
// this check additionally validates a present approval (human, on HEAD) and the protected set.
26+
const tier = req.tier ?? "T3";
27+
if (tier === "T0" || tier === "T1") {
28+
process.stdout.write(JSON.stringify({ ok: true, tier, note: "tier T0/T1: no approval required (§9)" }) + "\n");
29+
process.exit(0);
30+
}
2231
const status = evaluateApproval(reviews, req);
2332
if (status === "ok") {
2433
process.stdout.write(JSON.stringify({ ok: true }) + "\n");

package.json

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,6 @@
11
{
22
"name": "devloop",
3-
"version": "0.2.4",
3+
"version": "0.2.5",
44
"private": true,
55
"type": "module",
66
"license": "MIT",

src/cli/verify-review.ts

Lines changed: 11 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -13,6 +13,7 @@ interface Request extends ApprovalContext {
1313
reviews?: Review[];
1414
diffPaths?: string[];
1515
protectedGlobs?: string[];
16+
tier?: string; // from tier.json (derive-tier). Absent -> conservative default (require approval).
1617
}
1718

1819
function fail(reason: string): never {
@@ -33,6 +34,16 @@ if (
3334
fail("protected-set-touched: the diff edits a guardian/gate config (reward-hacking alarm)");
3435
}
3536

37+
// 2. Tier gate (§9): T0/T1 auto-merge — no approval required. (Gate-tamper above still applied.)
38+
// Unknown/absent tier defaults to conservative "T3" (approval required). The authoritative
39+
// T2/T3 merge block is branch protection's required CODEOWNER review on the high-tier paths;
40+
// this check additionally validates a present approval (human, on HEAD) and the protected set.
41+
const tier = req.tier ?? "T3";
42+
if (tier === "T0" || tier === "T1") {
43+
process.stdout.write(JSON.stringify({ ok: true, tier, note: "tier T0/T1: no approval required (§9)" }) + "\n");
44+
process.exit(0);
45+
}
46+
3647
const status = evaluateApproval(reviews, req);
3748
if (status === "ok") {
3849
process.stdout.write(JSON.stringify({ ok: true }) + "\n");

templates/ci-precondition-check.yml

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -89,10 +89,11 @@ jobs:
8989
--argjson reviews "$REVIEWS" \
9090
--arg prAuthor "$AUTHOR" \
9191
--arg headSha "$HEAD_SHA" \
92+
--arg tier "$(jq -r .tier tier.json)" \
9293
--argjson bots "$(cat .devloop/bot-logins.json)" \
9394
--argjson diff "$DIFF" \
9495
--argjson protected "$(cat .devloop/protected-globs.json)" \
95-
'{reviews:$reviews, prAuthor:$prAuthor, headSha:$headSha, botLogins:$bots, diffPaths:$diff, protectedGlobs:$protected}' \
96+
'{reviews:$reviews, prAuthor:$prAuthor, headSha:$headSha, tier:$tier, botLogins:$bots, diffPaths:$diff, protectedGlobs:$protected}' \
9697
| node node_modules/devloop/dist/cli/verify-review.js
9798
9899
- name: Verify implement only un-skipped tests (test<->code seam, §11 #3)

test/cli/verify-review.test.ts

Lines changed: 36 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -22,6 +22,42 @@ test("exit 0 when a human approved HEAD and the diff is clean", () => {
2222
expect(JSON.parse(r.stdout).ok).toBe(true);
2323
});
2424

25+
test("tier T0/T1 passes WITHOUT any approval (§9 auto-merge)", () => {
26+
for (const tier of ["T0", "T1"]) {
27+
const r = run({ ...base, tier, reviews: [] });
28+
expect(r.status).toBe(0);
29+
}
30+
});
31+
32+
test("tier T1 passes even with a (bot/stale) approval present — T1 needs no approval at all", () => {
33+
// tier-blind code would FAIL here (an approval is present but not a valid human one); §9 says
34+
// T0/T1 auto-merge, so the tier-aware check must pass regardless of who/whether anyone approved.
35+
const r = run({ ...base, tier: "T1", reviews: [{ user: "agent-bot", state: "APPROVED", commit_id: "abc123" }] });
36+
expect(r.status).toBe(0);
37+
});
38+
39+
test("tier T1 still FAILS on gate-tamper (protected-set is a human gate at every tier)", () => {
40+
const r = run({
41+
...base,
42+
tier: "T1",
43+
reviews: [],
44+
diffPaths: ["stryker.conf.json"],
45+
protectedGlobs: ["**/stryker.conf.*"],
46+
});
47+
expect(r.status).toBe(1);
48+
expect(JSON.parse(r.stdout).reason).toMatch(/protected/);
49+
});
50+
51+
test("tier T2 with a valid human approval -> success", () => {
52+
const r = run({ ...base, tier: "T2", reviews: [{ user: "alice", state: "APPROVED", commit_id: "abc123" }] });
53+
expect(r.status).toBe(0);
54+
});
55+
56+
test("tier T2 with only a bot approval -> fail (self-approve attempt)", () => {
57+
const r = run({ ...base, tier: "T2", reviews: [{ user: "agent-bot", state: "APPROVED", commit_id: "abc123" }] });
58+
expect(r.status).toBe(1);
59+
});
60+
2561
test("exit 0 (pending) when NOBODY has approved yet — no stale FAILURE on a fresh PR", () => {
2662
// The authoritative 'must be approved' block is branch protection's CODEOWNER requirement,
2763
// not this check. A fresh PR with zero approvals must NOT leave a failing run that lingers

0 commit comments

Comments
 (0)