fix(meta): clerk-review — incomplete reviews no longer false-reject
The per-commit clerk had three flaws, exposed by a 20-commit push where 6 of 7
rejections were false (incl. a CHANGELOG-only commit):
1. No-verdict / max-turns / timeout defaulted to REJECTED — an unfinished review
read as 'hard contradiction found'. Now a third outcome, INCOMPLETE, which is
non-blocking (the push proceeds with a warning); only a real REJECTED blocks.
2. Turn/time budget too tight (6 turns / 150s) for decision-heavy commits. Raised
defaults to 15 turns / 300s, and the prompt now biases to APPROVED when no
concrete contradiction is found ('unsure' means APPROVED, never REJECTED).
3. The skip valve matched its own feature commit because it scanned for the token
anywhere in the message. Moved to a trailer-line match so prose/subject mentions
no longer trip it.
Pre-push hook updated to treat INCOMPLETE as a non-blocking warning. The git-commit
skill documents the trailer form.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
This commit is contained in:
Claude Opus 4.7
@@ -88,17 +88,22 @@ Use the project subsystem as scope. Examples:
|
||||
- Never combine unrelated changes (e.g., don't mix a crash fix with new features).
|
||||
- When in doubt, prefer more smaller commits over fewer large ones.
|
||||
|
||||
### Clerk safety valve (`[clerk-skip]`)
|
||||
### Clerk safety valve (`Clerk-Skip:` trailer)
|
||||
|
||||
The pre-push clerk reviews each commit for D-record consistency. For a pure bulk
|
||||
content commit — e.g. shipping thousands of generated planetary description files
|
||||
— that review is moot and just burns agents on noise. Put the token `[clerk-skip]`
|
||||
anywhere in the commit message and `tooling/clerk-review` auto-approves that commit
|
||||
— that review is moot and just burns agents on noise. Add a `Clerk-Skip:` **trailer
|
||||
line** to the commit message and `tooling/clerk-review` auto-approves that commit
|
||||
without spawning an agent. Use it only for content/data dumps, never for commits
|
||||
that touch `decisions/`, code, or ticket-bearing work.
|
||||
|
||||
It must be a trailer (a line starting with `Clerk-Skip:`), not inline prose — that
|
||||
way a commit that merely *mentions* the token in its subject or body isn't skipped.
|
||||
|
||||
```
|
||||
data(content): generate 5000 planetary description files [clerk-skip]
|
||||
data(content): generate 5000 planetary description files
|
||||
|
||||
Clerk-Skip: bulk generated content, no D-record surface
|
||||
```
|
||||
|
||||
### Examples
|
||||
|
||||
@@ -199,12 +199,16 @@ if [ -x "$REPO_ROOT/tooling/clerk-review" ] && [ "${SR_SKIP_CLERK:-0}" != "1" ];
|
||||
CLERK_VERDICT=$("$REPO_ROOT/tooling/clerk-review" 2>&1 | tee /dev/stderr | tail -1)
|
||||
if [ "$CLERK_VERDICT" = "APPROVED" ]; then
|
||||
echo "pre-push: clerk — APPROVED"
|
||||
elif [ "$CLERK_VERDICT" = "TIMEOUT" ]; then
|
||||
echo "pre-push: clerk — TIMEOUT (findings may arrive later at .cache/pre-push-review.md)"
|
||||
ERRORS=$((ERRORS + 1))
|
||||
else
|
||||
elif [ "$CLERK_VERDICT" = "REJECTED" ]; then
|
||||
echo "pre-push: clerk — REJECTED (see .cache/pre-push-review.md)"
|
||||
ERRORS=$((ERRORS + 1))
|
||||
elif [ "$CLERK_VERDICT" = "INCOMPLETE" ]; then
|
||||
echo "pre-push: clerk — INCOMPLETE (some reviews didn't finish; NOT blocking)"
|
||||
echo " See .cache/pre-push-review.md. For a full verdict: raise SR_CLERK_MAX_TURNS / SR_CLERK_TIMEOUT,"
|
||||
echo " or add a 'Clerk-Skip:' trailer to bulk-content commits."
|
||||
else
|
||||
echo "pre-push: clerk — '$CLERK_VERDICT' unrecognized; treating as block (see .cache/pre-push-review.md)"
|
||||
ERRORS=$((ERRORS + 1))
|
||||
fi
|
||||
else
|
||||
echo "pre-push: clerk review — skipped (not installed or SR_SKIP_CLERK=1)"
|
||||
|
||||
+68
-44
@@ -3,32 +3,41 @@
|
||||
Clerk pre-push review — checks D-record consistency, ticket drift, and decision
|
||||
contradictions against the diff that would be pushed.
|
||||
|
||||
Large pushes are reviewed one commit at a time — commits are the logical units
|
||||
(each /git-commit is one coherent change), so each clerk agent reviews a
|
||||
self-contained change *and its commit message* (enabling the "does this commit's
|
||||
implementation match ticket #NNN?" check). Commits are reviewed by parallel clerk
|
||||
agents and the verdicts aggregated. This avoids the single-agent failure mode
|
||||
where one `claude -p` ran out of turns on a huge combined diff and never emitted
|
||||
a verdict (which defaulted to REJECTED).
|
||||
Reviewed one commit at a time — commits are the logical units (each /git-commit is
|
||||
one coherent change), so each clerk agent reviews a self-contained change *and its
|
||||
commit message*. Commits are reviewed by a bounded pool of parallel clerk agents
|
||||
and the per-commit verdicts aggregated.
|
||||
|
||||
Safety valve: a commit whose message contains the token `[clerk-skip]` is
|
||||
auto-approved without spawning an agent — for bulk content commits (e.g. shipping
|
||||
thousands of generated planetary description files) where D-record review is moot
|
||||
and the diff would only burn agents on noise.
|
||||
Three outcomes per commit:
|
||||
APPROVED no contradiction found.
|
||||
REJECTED a concrete contradiction with a named, active D-record. HARD BLOCK.
|
||||
INCOMPLETE the review could not finish — timed out, ran out of turns, or emitted
|
||||
no clear verdict. This is NOT a contradiction; it is non-blocking by
|
||||
default (the push proceeds with a warning). Raise the budget knobs or
|
||||
add a `Clerk-Skip:` trailer to avoid it.
|
||||
|
||||
Outputs exactly one word as the LAST stdout line: APPROVED, REJECTED, or TIMEOUT.
|
||||
Why INCOMPLETE exists: an unfinished review must never read as "hard contradiction
|
||||
found." The previous version defaulted no-verdict / max-turns to REJECTED, which
|
||||
turned every slow review into a false block (e.g. a CHANGELOG-only commit).
|
||||
|
||||
Safety valve: a commit message with a `Clerk-Skip:` trailer line is auto-approved
|
||||
without spawning an agent — for bulk content commits (e.g. shipping thousands of
|
||||
generated planetary description files) where D-record review is moot. A *trailer*
|
||||
(a line starting with `Clerk-Skip:`) is required, so merely mentioning the token in
|
||||
prose or a subject line does not trip the valve.
|
||||
|
||||
Outputs exactly one word as the LAST stdout line: APPROVED, REJECTED, or INCOMPLETE.
|
||||
Writes verbose findings to .cache/pre-push-review.md.
|
||||
|
||||
Exit codes:
|
||||
0 = APPROVED
|
||||
1 = REJECTED (hard contradiction with an active D-record in >=1 commit)
|
||||
2 = TIMEOUT (>=1 commit review timed out, none rejected)
|
||||
0 = APPROVED or INCOMPLETE (non-blocking)
|
||||
1 = REJECTED (>=1 commit contradicts an active D-record)
|
||||
|
||||
Env knobs:
|
||||
SR_CLERK_COMMIT_BUDGET per-commit diff char cap (default 120000, truncates)
|
||||
SR_CLERK_WORKERS parallel clerk agents (default 4)
|
||||
SR_CLERK_MAX_TURNS turns per clerk agent (default 6)
|
||||
SR_CLERK_TIMEOUT per-commit timeout seconds (default 150)
|
||||
SR_CLERK_WORKERS parallel clerk agents (default 4)
|
||||
SR_CLERK_MAX_TURNS turns per clerk agent (default 15)
|
||||
SR_CLERK_TIMEOUT per-commit timeout seconds (default 300)
|
||||
|
||||
Usage:
|
||||
tooling/clerk-review run the review, print verdict
|
||||
@@ -36,6 +45,7 @@ Usage:
|
||||
"""
|
||||
|
||||
import os
|
||||
import re
|
||||
import subprocess
|
||||
import sys
|
||||
from concurrent.futures import ThreadPoolExecutor, as_completed
|
||||
@@ -50,11 +60,13 @@ FINDINGS_FILE = CACHE_DIR / "pre-push-review.md"
|
||||
|
||||
COMMIT_BUDGET = int(os.environ.get("SR_CLERK_COMMIT_BUDGET", "120000"))
|
||||
WORKERS = int(os.environ.get("SR_CLERK_WORKERS", "4"))
|
||||
MAX_TURNS = os.environ.get("SR_CLERK_MAX_TURNS", "6")
|
||||
TIMEOUT_SECONDS = int(os.environ.get("SR_CLERK_TIMEOUT", "150"))
|
||||
MAX_TURNS = os.environ.get("SR_CLERK_MAX_TURNS", "15")
|
||||
TIMEOUT_SECONDS = int(os.environ.get("SR_CLERK_TIMEOUT", "300"))
|
||||
|
||||
# Safety valve: a commit message containing this token is auto-approved (no agent).
|
||||
SKIP_TOKEN = "[clerk-skip]"
|
||||
# Safety valve: a commit message with a "Clerk-Skip:" trailer line is auto-approved
|
||||
# (no agent). A trailer (line-start) is required so that merely mentioning the token
|
||||
# in prose or a subject line does not trip the valve.
|
||||
SKIP_TRAILER = re.compile(r"(?im)^[ \t]*clerk-skip[ \t]*:")
|
||||
|
||||
CLERK_PROMPT = """You are the CLERK, the institutional guardrail for The Settled Reach.
|
||||
|
||||
@@ -71,16 +83,21 @@ consistency, ticket drift, and decision contradictions.
|
||||
|
||||
## Your task
|
||||
|
||||
- Does this commit contradict any active D-record? (grep decisions/*.md for relevant keywords)
|
||||
- If the commit's code/text references a D/Q/R-ID, does that ID exist and is it active?
|
||||
- If the commit message references ticket #NNN, does the change match the ticket?
|
||||
Surface any open Q-records relevant to the changed files.
|
||||
Be efficient — a few targeted greps of decisions/*.md, then conclude. Check:
|
||||
- Does this commit contradict any active D-record?
|
||||
- If the commit's code/text cites a D/Q/R-ID, does that ID exist and is it active?
|
||||
- If the commit message cites ticket #NNN, does the change match the ticket?
|
||||
Note any relevant open Q-records.
|
||||
|
||||
## Output format
|
||||
## Output format (REQUIRED)
|
||||
|
||||
First write your detailed findings. Then, on the VERY LAST LINE, output exactly
|
||||
one word — APPROVED or REJECTED. REJECTED only for a hard contradiction with an
|
||||
active D-record; drift, open Q-records, and suggestions are findings, not blocks.
|
||||
Write brief findings, then on the VERY LAST LINE output exactly one word:
|
||||
APPROVED or REJECTED.
|
||||
|
||||
- REJECTED *only* for a concrete contradiction with a named, active D-record.
|
||||
- Drift, stale references, open Q-records, and suggestions are findings, NOT blocks -> APPROVED.
|
||||
- If you did not find a concrete contradiction, output APPROVED — even if you could
|
||||
not check everything. "Unsure / didn't finish" means APPROVED, never REJECTED.
|
||||
"""
|
||||
|
||||
|
||||
@@ -112,8 +129,8 @@ def list_commits(rng):
|
||||
def commit_unit(sha):
|
||||
"""Return (subject, text, skip) for one commit.
|
||||
|
||||
`skip` is True when the commit message contains SKIP_TOKEN (the safety valve);
|
||||
`text` is the message + diff, truncated to COMMIT_BUDGET.
|
||||
`skip` is True when the commit message has a `Clerk-Skip:` trailer (the safety
|
||||
valve); `text` is the message + diff, truncated to COMMIT_BUDGET.
|
||||
"""
|
||||
subject = subprocess.check_output(
|
||||
["git", "show", "-s", "--format=%h %s", sha], text=True
|
||||
@@ -121,7 +138,7 @@ def commit_unit(sha):
|
||||
message = subprocess.check_output(
|
||||
["git", "show", "-s", "--format=%B", sha], text=True
|
||||
)
|
||||
skip = SKIP_TOKEN.lower() in message.lower()
|
||||
skip = bool(SKIP_TRAILER.search(message))
|
||||
text = subprocess.check_output(
|
||||
["git", "show", "--format=fuller", sha], text=True
|
||||
)
|
||||
@@ -131,7 +148,11 @@ def commit_unit(sha):
|
||||
|
||||
|
||||
def run_clerk(label, diff_text, index):
|
||||
"""Spawn one clerk agent over a commit; return (verdict, findings)."""
|
||||
"""Spawn one clerk agent over a commit; return (verdict, findings).
|
||||
|
||||
verdict is APPROVED, REJECTED, or INCOMPLETE. INCOMPLETE covers timeout, empty
|
||||
output, and no-clear-verdict — none of which is a contradiction.
|
||||
"""
|
||||
prompt = CLERK_PROMPT.format(label=label, index=index, diff=diff_text)
|
||||
try:
|
||||
result = subprocess.run(
|
||||
@@ -141,17 +162,17 @@ def run_clerk(label, diff_text, index):
|
||||
)
|
||||
output = result.stdout.strip()
|
||||
except subprocess.TimeoutExpired:
|
||||
return "TIMEOUT", f"{label}: clerk timed out after {TIMEOUT_SECONDS}s."
|
||||
return "INCOMPLETE", f"{label}: review timed out after {TIMEOUT_SECONDS}s (not a contradiction)."
|
||||
|
||||
if not output:
|
||||
return "REJECTED", f"{label}: clerk produced no output."
|
||||
return "INCOMPLETE", f"{label}: clerk produced no output (not a contradiction)."
|
||||
|
||||
last_line = output.splitlines()[-1].strip().upper()
|
||||
if last_line == "APPROVED":
|
||||
return "APPROVED", output
|
||||
if last_line == "REJECTED":
|
||||
return "REJECTED", output
|
||||
return "REJECTED", output + "\n\n(No clear verdict on last line — defaulting to REJECTED)"
|
||||
return "INCOMPLETE", output + "\n\n(No clear verdict on last line — recorded as INCOMPLETE, not a contradiction.)"
|
||||
|
||||
|
||||
def load_index():
|
||||
@@ -178,20 +199,20 @@ def main():
|
||||
print(f"clerk plan: {len(commits)} commit(s) over {rng} "
|
||||
f"(budget {COMMIT_BUDGET} chars, {WORKERS} workers, {MAX_TURNS} turns each)")
|
||||
for i, (subject, text, skip) in enumerate(units):
|
||||
tag = "SKIP (auto-approve)" if skip else "review"
|
||||
tag = "SKIP (Clerk-Skip:)" if skip else "review"
|
||||
print(f" commit {i + 1}/{len(commits)}: {len(text):>9} chars [{tag}] {subject}")
|
||||
return 0
|
||||
|
||||
index = load_index()
|
||||
reviewable = sum(1 for _, _, skip in units if not skip)
|
||||
print(f" clerk: {len(commits)} commit(s) — {reviewable} to review, "
|
||||
f"{len(commits) - reviewable} auto-approved ([clerk-skip]); {WORKERS} parallel...",
|
||||
f"{len(commits) - reviewable} auto-approved (Clerk-Skip:); {WORKERS} parallel...",
|
||||
file=sys.stderr)
|
||||
|
||||
def task(i, subject, text, skip):
|
||||
label = f"commit {i + 1}/{len(commits)} ({subject})"
|
||||
if skip:
|
||||
return i, label, "APPROVED", f"{label}: auto-approved via [clerk-skip] safety valve."
|
||||
return i, label, "APPROVED", f"{label}: auto-approved via Clerk-Skip: trailer."
|
||||
verdict, findings = run_clerk(label, text, index)
|
||||
return i, label, verdict, findings
|
||||
|
||||
@@ -206,14 +227,17 @@ def main():
|
||||
verdicts = [r[1] for r in results]
|
||||
if "REJECTED" in verdicts:
|
||||
overall = "REJECTED"
|
||||
elif "TIMEOUT" in verdicts:
|
||||
overall = "TIMEOUT"
|
||||
elif "INCOMPLETE" in verdicts:
|
||||
overall = "INCOMPLETE"
|
||||
else:
|
||||
overall = "APPROVED"
|
||||
|
||||
n_rej = verdicts.count("REJECTED")
|
||||
n_inc = verdicts.count("INCOMPLETE")
|
||||
parts = [
|
||||
f"# Clerk Pre-Push Review\n\n**Overall verdict: {overall}**\n",
|
||||
f"Reviewed {len(commits)} commit(s) over {rng}.\n",
|
||||
f"Reviewed {len(commits)} commit(s) over {rng} — "
|
||||
f"{verdicts.count('APPROVED')} approved, {n_rej} rejected, {n_inc} incomplete.\n",
|
||||
]
|
||||
for label, verdict, findings in results:
|
||||
parts.append(f"\n---\n\n## {label} — {verdict}\n\n{findings}\n")
|
||||
@@ -222,7 +246,7 @@ def main():
|
||||
print(f" clerk: overall verdict — {overall} (details: {FINDINGS_FILE})", file=sys.stderr)
|
||||
sys.stderr.flush()
|
||||
print(overall)
|
||||
return {"APPROVED": 0, "REJECTED": 1, "TIMEOUT": 2}[overall]
|
||||
return 1 if overall == "REJECTED" else 0
|
||||
|
||||
|
||||
if __name__ == "__main__":
|
||||
|
||||
Reference in New Issue
Block a user