Adversarial review at 8af1165: the verify stage never fires, prove-test.sh proves too much, and three more silent happy-path failures #3

I ran an adversarial review of this template at 8af1165 (structural pass plus a defect hunt, every finding below re-verified against the tree before writing this). The design essay holds up well — the five design rules, the autonomy-tracks-consequence inversion, ADVICE.md's escalation ladder. These findings are all in the deterministic layer, and the five big ones share a property worth naming: they are silent on the documented happy path, with no attacker and no concurrency, and each one removes exactly the guarantee the green signal is supposed to carry.

1. The PR-verify routine never fires as documented

ROUTINES.md:131 has the implement routine open a draft PR unconditionally; ROUTINES.md:144 has the verify routine trigger on pull_request.opened with "Is draft is false". Follow both as written and stage 3 never runs on any factory PR — and it's worse than a filter mismatch, because GitHub emits ready_for_review, not opened, when a draft is later promoted, so the routine misses promotion too.

Downstream effect: nothing in the repo machine-parses the FACTORY_GATES: line. With the cold verify stage dead, every check on a factory PR runs inside the implementer's own session, and the verbatim-quoted verdict line degrades to a string the writer pasted into its own PR body. The writer≠grader architecture quietly becomes writer-grades-writer.

(Also: template/.claude/skills/factory-implement/SKILL.md:144 makes draft conditional on four triggers, contradicting ROUTINES.md:131 — the two documents disagree about what a non-draft factory PR means.)

2. prove-test.sh accepts any non-zero exit as proof

template/.factory/scripts/prove-test.sh:94-107 — the entire signal is test_status != 0. Reverting the non-test hunks deletes the implementation, so for the most common change shape (new module + test that imports it), the post-revert failure is an import/collection error, not an assertion failure. A test containing import { slugify } from './slug'; it('works', () => expect(1).toBe(1)) gets PROOF: status=PROVEN — an assertion-free test certified as proving the fix, by the check factory-verifier.md:48 calls "the check that catches the most real defects."

Fixing this properly means classifying the failure (assertion vs. load/collection) from runner output or per-runner exit codes (pytest usefully distinguishes 1 from 4/5; jest/vitest need output parsing), and failing closed to something like status=UNPROVEN reason=failure-not-classified when classification isn't possible.

3. factory:awaiting-review is never cleared, so back-pressure jams permanently

CONTRACT.md:50-51 keeps the label until a human merges or closes — but nothing removes it, and the PR body template (factory-implement/SKILL.md:118-142) links the issue without a Closes #n keyword, so merging doesn't close the issue either. Merge N ≥ STOP_IF items over a couple of weeks and every subsequent implement run stops at the throttle check forever, over an empty review queue; a week later the monitor files an issue blaming the human review queue for work that already shipped.

4. Claim branches are never released; any post-claim abort makes the issue permanently unclaimable

On failure after claiming, factory-implement/SKILL.md:155-159 returns the issue to a live label but nothing deletes refs/heads/claude/fq-<n>. A fresh branch can never fast-forward the surviving ref, so every subsequent run selects the same item (highest confidence first), loses the push race against a ghost, treats it as "already claimed," and stops — burning the entire daily run. It's invisible to monitoring: staleness checks only watch factory:in-progress, and an item stuck at ready-to-implement looks like a healthy queued item.

Related: triage's sweep (factory-triage/SKILL.md:24,89) doesn't exclude in-progress/awaiting-review issues from re-triage — with the instant trigger installed, triage can strip in-progress from an item mid-implementation, advertising it as claimable while its claim ref is live, which manufactures the same deadlock.

5. gates.sh reports GREEN for a required gate the stack detection never reaches

The MISCONFIGURED path only exists inside skip() (gates.sh:85-91). A required gate that the DETECT block never emits a run or skip for leaves STATUS=GREEN. Two configs that follow gates.conf's own comments hit it: REQUIRED_FULL="... mutation" (mutation is gated to deep, so at full it's neither run nor skipped), and REQUIRED_FULL="... build" on a Python repo (the python arm has no build line). The stock config avoids it on all four detected stacks, so tests/test-gates.sh passes; it bites the first time someone tunes gates.conf as invited. A closing sweep — any name in the required list that appears in neither PASSED/FAILED/SKIPPED → MISCONFIGURED — would fail this closed.

Smaller, verified

• block-merge.sh:47: git push origin +main (force push to main) and git push origin mybranch:main both pass the regex — the + alternative only matches before refs/heads/ or HEAD:, and a source-branch push never reaches the protected-name check.
• The hook guards .factory/gates.conf as a file but not .factory/scripts/ as a directory, so sed -i into prove-test.sh trips nothing — the proof mechanism is the one load-bearing script with no mechanical write protection. block-merge.sh itself is also absent from the settings.json Edit deny list.
• doctor.sh doesn't check that .claude/settings.json/block-merge.sh were actually installed — on a mature repo where install.sh's skip-if-exists left them out, doctor prints PASS on a factory with no shell-level merge guard.
• prove-test.sh:66 expands "${test_paths[@]}" on a possibly-empty array before its own emptiness guard at line 72 — under set -u on bash 3.2 (macOS /bin/bash) every invocation without --test-path dies with unbound variable before printing a PROOF: line.
• The default test-file patterns (prove-test.sh:74) miss pytest's co-located layout: src/utils/test_foo.py classifies as non-test, so the new test is bundled into the revert and deleted; the focused run then exits 4/5, which (per docs: add pragmatic Factory adoption advice #2) still prints PROVEN.
• factory-handoff:v1 comments carry no authorship check, and the "issue text is untrusted" rule appears in the triage skill and CONTRACT.md but in neither factory-implement/SKILL.md nor factory-verifier.md — the two consumers of the fields. On a public repo, any commenter can post a second handoff (the duplicate-handoff rule then drains the item to needs-info) or a gate_level: fast downgrade that both the implementer and verifier honor.
• Back-pressure is read at run start (factory-implement/SKILL.md:30) but the counted label is applied at run end (line 151), so two overlapping runs both pass a limit of 3 at depth 2 and land the queue at 4.

What checked out clean

Worth saying since I went looking: factory-fire.yml is injection-safe (env: + jq --arg, no ${{ }} in run:, minimal permissions), the claim push race is a genuine compare-and-swap and correct for the case tests/test-claim.sh covers (the defects are in release/re-entry, not the primitive), and prove-test.sh's trap/restore ordering, clean-tree refusal, and -z path handling are all right.

Happy to split any of these into separate issues if that's easier to track. 1, 3, and 5 look like small unambiguous fixes; 2 is the deep one.