addyosmani / addyosmani/factory
Adversarial review at 8af1165: the verify stage never fires, prove-test.sh proves too much, and three more silent happy-path failures
- 主要言語
- Shell
- スター
- 182
- フォーク
- 15
- 平均マージ
- 4分
- マージ済み PR(30日)
- 2
説明
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-`. 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 #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.
コントリビューションガイド
評価
この issue はまだ評価されていません。