Mechanical closure of 'a verification that passes when the thing it verifies never happened.'
This commit is contained in:
@@ -0,0 +1,107 @@
|
|||||||
|
# push-guard
|
||||||
|
|
||||||
|
Mechanical closure of one defect: **a verification that passes when the thing it verifies never happened.**
|
||||||
|
|
||||||
|
Three incidents in one evening, three agents, no coordination, same shape:
|
||||||
|
|
||||||
|
| # | Incident | Why the check passed |
|
||||||
|
|---|---|---|
|
||||||
|
| 1 | `PUSH VERIFIED` reported after nothing was pushed | Commit had aborted, so local HEAD trivially equalled the remote ref |
|
||||||
|
| 2 | An **empty commit** carrying another commit's message was pushed | A push was chained after a *failed* commit and ran on stale state |
|
||||||
|
| 3 | Conflict markers + invalid JSON committed into 70 generated files | Nothing asserted the generated output still parsed |
|
||||||
|
|
||||||
|
Each is an assertion satisfied by the null case. `push-guard.sh` asserts the **positive** fact instead: a new object exists, the remote *moved*, the content *parses*.
|
||||||
|
|
||||||
|
## Checks
|
||||||
|
|
||||||
|
| Sub-command | Asserts | Exit on failure |
|
||||||
|
|---|---|---|
|
||||||
|
| `check-staged` | index has no unmerged paths | `2` |
|
||||||
|
| | no conflict markers in staged content | `2` |
|
||||||
|
| | the conflict scan actually *completed* | `2` |
|
||||||
|
| | staged JSON under `--json-path` parses | `3` |
|
||||||
|
| | **something is actually staged** | `5` |
|
||||||
|
| `push` | HEAD advanced past `--since-head` | `5` |
|
||||||
|
| | HEAD is a non-empty commit | `5` |
|
||||||
|
| | remote **moved**, and now equals local HEAD | `4` |
|
||||||
|
|
||||||
|
## Usage
|
||||||
|
|
||||||
|
```bash
|
||||||
|
push-guard.sh check-staged --json-path 'data/**/*.json'
|
||||||
|
|
||||||
|
BEFORE=$(git rev-parse HEAD)
|
||||||
|
git commit -m "..."
|
||||||
|
push-guard.sh push --remote origin --branch main --since-head "$BEFORE"
|
||||||
|
```
|
||||||
|
|
||||||
|
`--since-head` is what distinguishes "committed nothing" from "committed something", and capturing the remote ref *before* pushing is what makes `remote == local` mean anything. Both were missing from the guard that produced incident 1.
|
||||||
|
|
||||||
|
## Design decisions that measurement forced
|
||||||
|
|
||||||
|
These are the interesting part; each one was wrong in the first draft.
|
||||||
|
|
||||||
|
**A bare `=======` is deliberately NOT treated as a conflict marker.** It collides with reStructuredText underlines and ASCII rules. Every genuine conflict git writes also contains `<<<<<<<` and `>>>>>>>`, so requiring those loses no real detection. Measured against a real repository: **zero** false positives across the entire tracked tree.
|
||||||
|
|
||||||
|
**The JSON check is opt-in by path, not on-by-default.** The first draft checked every staged `*.json`, on the reasoning that broader is strictly stronger. Measured against the same repository: **17 of 119** tracked `.json` files fail a strict parse, *all legitimately* — every `tsconfig*.json` is JSONC (comments are legal there) and the CA templates are Go templates that merely carry a `.json` suffix. An on-by-default check fires on ~14% of the repo's JSON, and a guard that cries wolf gets routed around until the bypass is habitual — at which point the bypass covers the true positives too. Broader was **weaker**.
|
||||||
|
|
||||||
|
**`--json-path` values are normalized to `:(glob)` magic.** Git's default pathspec matching makes `data/**/*.json` require at least one intermediate directory: it matches `data/sub/b.json` and *silently skips* `data/a.json`. The obvious spelling would have delivered partial coverage with no warning.
|
||||||
|
|
||||||
|
## Found by independent review, after the needles were already green
|
||||||
|
|
||||||
|
An adversarial review (author ≠ reviewer) found two genuine fail-opens that 17 self-written needles, five mutation runs and a repo-wide false-positive measurement had all missed. Both were reproduced before being fixed, and both now have needles.
|
||||||
|
|
||||||
|
**Renamed files were invisible to every content check.** Git detects renames by default (`diff.renames=true` since 2.9), so `git mv` plus a small edit is reported as a single `R` entry — which `--diff-filter=ACM` does not match. Reproduced at 97% similarity: the guard printed `staged content OK` and exited 0 with conflict markers in the staged blob. Fixed with `--no-renames`.
|
||||||
|
|
||||||
|
There is a trap inside this one worth recording. With *only* the renamed file staged, the guard exits 5 — the nothing-staged check fires because the file list came back empty. That looks like a catch and is pure accident; co-stage one ordinary file and the fail-open is total. The needle deliberately co-stages a clean file so it cannot pass for the wrong reason.
|
||||||
|
|
||||||
|
**`-I` skipped files marked binary in `.gitattributes`,** while the summary still counted them as scanned — a clean pass *and* a false count. Fixed with `-a`.
|
||||||
|
|
||||||
|
The review also correctly identified one **vacuous control**: the positive `--since-head` case asserted only exit 0, which the push produces anyway, so deleting the entire `--since-head` block left it passing. It now asserts on output.
|
||||||
|
|
||||||
|
Two reported findings did **not** reproduce and were not acted on: `:(glob)` does still match a bare directory pathspec, and my first rename repro failed only because the edit dropped similarity below the detection threshold — a badly built test, not an absent bug. Re-testing properly is what confirmed it.
|
||||||
|
|
||||||
|
## Known limitations — stated, not hidden
|
||||||
|
|
||||||
|
- **The JSON check is unprotected until a repo wires up `--json-path`.** This is a real fail-open. The mitigation is that the guard prints `JSON check NOT REQUESTED` on *every* run, so the unprotected state is visible in every log rather than silent. That printed line is the only thing standing between "not configured" and "not noticed".
|
||||||
|
- `check-staged` inspects the index. Content added to the working tree *after* it runs is not covered — it belongs in a `pre-commit` hook, where the window is smallest.
|
||||||
|
|
||||||
|
## Test harness
|
||||||
|
|
||||||
|
`test-push-guard.sh` — 19 cases, each check in **both polarities**:
|
||||||
|
|
||||||
|
- **NEEDLE** — a deliberately broken fixture that must trip the guard.
|
||||||
|
- **CONTROL** — a clean fixture that must pass.
|
||||||
|
|
||||||
|
Controls are not decoration. A guard that failed unconditionally would satisfy every needle and look fully covered. Several controls additionally assert on the guard's *output* (`--out`), because exit 0 cannot distinguish "checked the files and they were fine" from "matched no files and had nothing to check" — two controls in an earlier revision were passing vacuously for exactly that reason.
|
||||||
|
|
||||||
|
### Mutation results — the harness was itself tested by breaking the guard
|
||||||
|
|
||||||
|
| Mutant | Result |
|
||||||
|
|---|---|
|
||||||
|
| fail-open (every failure downgraded to exit 0) | 9/16 fail — exactly the needles |
|
||||||
|
| always-fail | 16/16 fail — controls catch it |
|
||||||
|
| revert `:(glob)` normalization | 3/16 fail — caught **only** by the `--out` assertions |
|
||||||
|
| drop the remote-did-not-move assertion | 1/16 fail — the incident-1 needle |
|
||||||
|
| restore blanket `\|\| true` on the scan | 1/17 fail — the fault-injection needle |
|
||||||
|
| revert `--no-renames` | 1/19 fail — the rename needle |
|
||||||
|
| revert `-a` to `-I` | 1/19 fail — the binary-attribute needle |
|
||||||
|
| delete the `--since-head` block | 2/19 fail — incl. the control that *was* vacuous |
|
||||||
|
| unmodified | 19/19 pass |
|
||||||
|
|
||||||
|
A guard nobody has watched fail is not a guard. The same applies to the harness: mutant C would have passed silently without the output assertions, so the assertions are load-bearing rather than ornamental.
|
||||||
|
|
||||||
|
**A blanket `|| true` on the scan was a fail-open in the guard's own error handling.** `git grep` exits `1` for "no match" but `>=2` for a real failure. `|| true` collapsed the two, so a malformed pathspec or unreadable index would have been reported as "ok, no conflict markers" — a clean pass from a scan that never ran. The status is now discriminated, and a PATH-shim fault-injection needle proves the refusal.
|
||||||
|
|
||||||
|
**The `-E` flag on `git grep` is load-bearing and was caught by a needle, not by review.** `git grep` defaults to *basic* regex, in which `(`, `|` and `{7}` are literal characters. Without `-E` the patterns match nothing and the check reports a clean pass over a file full of conflict markers — the exact defect this tool exists to prevent, shipped inside the tool itself.
|
||||||
|
|
||||||
|
## Proposed framework path
|
||||||
|
|
||||||
|
```
|
||||||
|
framework/tools/git/push-guard.sh
|
||||||
|
framework/tools/git/test-push-guard.sh
|
||||||
|
```
|
||||||
|
|
||||||
|
Matches the existing `tools/git/test-*.sh` convention. Dependencies: bash 4.4+, git, python3 — `python3` is already an accepted dependency of `ci-queue-wait.sh`.
|
||||||
|
|
||||||
|
Operator-agnostic: no hostnames, credentials, remotes, or operator-specific paths. Clean under the framework-PR firewall.
|
||||||
Reference in New Issue
Block a user