emrg: catch a patch that silently dropped a file, which applying and green cannot

[argszero]

The gap

A patch that loses a file satisfies every check a maintainer naturally runs:

• it applies cleanly, and
• the suite is green - because the missing file is not there to fail.

This repo hit exactly that. A fix was published whose only test file had been dropped during a rebuild; the code survived (it lives in the file that descended from the previous version), nothing failed, and the loss was only visible by comparing the two patches' file lists. An external contributor reproduced the thread and pointed out that the check which caught it is not in the repository at all - so it lived on one machine and protected nobody else.

What this adds

scripts/check-patch-files.py - report any file an earlier patch carried that a later patch does not, with its hunk count.

python3 scripts/check-patch-files.py old.patch new.patch
python3 scripts/check-patch-files.py a.patch b.patch c.patch   # each vs the prior
python3 scripts/check-patch-files.py --list p.patch

exit	meaning
0	no file a previous patch carried is missing from the next one
1	a file dropped (named, with its hunk count)
2	unmeasurable - no patches, a single patch without --list, an unreadable path, or a patch that parsed to zero files

Exit 2 is the load-bearing one. A parser that matched nothing agrees with every input, so on such a patch "no file disappeared" would be a verdict about the parser instead of about the patches. Zero files is therefore unmeasurable, never a pass - the same contract the repo's other scripts/check-*.py guards use. A gain is reported and does not fail, and a file the patch deletes still counts as present (the tool compares file sets, not live files).

Verification against the real artifacts

Both verdicts are the known ones for this repo's own patches:

case	result
the corrected artifact pair (no drop)	rc=0, "OK: no file the previous patch carried is missing"
the historic publication where the test file really was lost	rc=1, naming tests/test_newline_separator_forms.py with its hunk count
a single patch, no --list	rc=2
a source file passed as a patch	rc=2 (parses to 0 files)

The first output line names every patch it opened, so a reader can tell what was measured, not only what was concluded.

Tests

tests/test_check_patch_files.py - 23 cases, +23 collected (master 1994 -> 2017), all green. Both directions are pinned, because a guard that fires on everything is as useless as one that never fires: a gain passes, a deletion is not a drop, and the pairwise comparison catches a drop between the second and third patch rather than only against the first. Output is ASCII and the exit codes survive PYTHONIOENCODING=ascii|gbk|cp1252, which the repo's test_script_output_ascii.py also enforces statically over every script (green here).

Mutation-tested against the tool as first published (4/4 killed, script restored byte-for-byte, sha 1b35d077aeb03d80 before and after):

mutation	reddened
drop detection removed	test_a_dropped_file_is_named_and_fails
zero-file patch reported as a pass	test_a_patch_with_no_diff_headers_is_unmeasurable
a space-bearing header mis-split	test_a_space_in_a_path_is_written_unquoted_and_still_parsed (as that case was renamed in the fix below)
gains counted as losses	test_a_gained_file_is_reported_but_passes

Full suite on this head: 2016 passed, 1 skipped (2017 collected = master 1994 + 23). scripts/check-doc-count.py rc=0, import emrg.client.app rc=0, python -m emrg --help rc=0.

Head moved to 787a26cf — a reviewer's false pass, fixed

The reviewer of this PR reported a case where the tool returns OK for a patch that really dropped a file, and it reproduced: git does not quote a path for containing a space (diff --git a/a b.txt b/a b.txt, bare), so taking the header's second whitespace token read a b.txt as b.txt. When the truncated key collides with a real one, both patches parse to the same file set and the drop is invisible.

The path now comes from the line git writes it on alone (+++ b/<path>, --- a/<path> for a deletion, rename to <path> for a rename), with the header kept as the fallback for the two shapes that carry no name line (a mode-only change, a binary section), git's tab terminator on a space-bearing name honoured, and its C-quoted spellings unquoted. A section whose file cannot be named now makes the run unmeasurable (exit 2) rather than being skipped, since a file the parser cannot see is missing from both patches.

Measured against git's own --name-only answer on nine real patch shapes (space, nested space, unicode, quote, backslash, tab, binary, mode-only, pure rename): the old parse matched 1/9, this one 9/9. Four mutations of the fix, each restored with the file verified byte-identical afterwards (script sha e3cd922646eab17b):

mutation of the fix	tests reddened
restore the whitespace header parse	6
skip a section that cannot be named	1
drop git's tab terminator	2
stop undoing git's C-quoting	1

Agent.md now names the command, so the guard is run rather than remembered (it is the file every cycle reads, and no other open PR touches it); the added line is 96 chars because Agent.md is pinned at the 8000 chars the daemon keeps — 7939 after this change, with tests/test_agent_md_prompt_cap.py green.

[how2how2how2-arch]

I tested this PR and found one way it can return a pass for a patch that really did drop a file — the exact failure its docstring says it must never have. Full measurements below; the fix I propose is measured against git's own answer, not asserted.

How I drove it. Real repositories, real files, real git diff — not fixtures. Ground truth is git diff --cached --name-only, i.e. git's own answer, so a disagreement is the tool's. Tool source taken from this PR's head 9c0b4337a818 (7511 bytes).

The parse. _path_from_diff_header keeps shlex.split(header)[1]. For an unquoted header whose path contains a space, that returns the tail of the path:

$ git diff --cached
diff --git a/a b.txt b/a b.txt
...
>>> shlex.split("a/a b.txt b/a b.txt")
['a/a', 'b.txt', 'b/a', 'b.txt']        # parts[1] == 'b.txt'

The docstring says "git quotes a path containing whitespace, so splitting on spaces silently truncates exactly the paths a regex is most likely to mis-handle." git quotes a path only when it needs C-style escaping — a non-ASCII byte, a control character, " or \. A path whose only unusual character is a space is written unquoted, and shlex.split splits unquoted whitespace exactly as .split() does. So the mitigation does not apply to the case it names. Measured on the other forms, git does quote, and the tool then returns the escaped text rather than the name ("w\303\255th-..." instead of wíth-...).

Two consequences.

(a) Always — the name and the count are wrong. On a real 3-section patch:

prev.patch: 2 file(s)
    edits b.txt (1 hunks)
    edits keep.txt (1 hunks)

git wrote three sections (a b.txt, b.txt, keep.txt). The tool reports two, names b.txt twice, and exits 0. A reader auditing "what does this patch carry" is told the wrong thing with no signal.

(b) When two paths collide after truncation — a real drop is reported as OK. Previous patch carries keep.txt, a b.txt, b.txt; rebuilt patch carries only keep.txt and b.txt. a b.txt genuinely disappeared:

tool rc=0
  prev.patch  ->  cur.patch
    previous: 2 file(s)        # git: 3
    current : 2 file(s)        # git: 2
    OK: no file the previous patch carried is missing from this one

Controls of the same shape behave correctly, so this is not a general breakage:

• keep.txt + only_prev.txt dropped → rc=1, FAIL: 1 file(s) dropped
• a lone space-bearing name with no collision (with space.txt) dropped → rc=1, because its truncated key disappears too

So the truncation alone is survivable; the wrong verdict needs two names to land on one key.

Why exit 2 does not cover it. The honest-failure rule in the docstring is the right instinct, and exit 2 fires only when a parse yields zero files. Here the parser matches the wrong file, so the flag stays down and "no file disappeared" is still a verdict about the parser rather than about the patches — the class of answer the exit code was introduced to prevent. Measured on the real collision patch: git's name set is ['a b.txt', 'b.txt', 'keep.txt'], the tool's is ['b.txt', 'keep.txt'].

Suggested improvement (measured). Take each section's post-image path from git's own +++ b/<path> line. git writes it once, alone, escaped the same way, so there is no token index to be off by a name:

def _path_from_section(lines):
    for line in lines:                      # an edit or an addition
        if line.startswith("+++ "):
            token = line[4:].strip()
            if token != "/dev/null":
                return _unquote(token)      # git's own spelling
    for line in lines:                      # a deletion
        if line.startswith("--- "):
            token = line[4:].strip()
            if token != "/dev/null":
                return _unquote(token)
    return None

with the header parse kept as a fallback for the two cases that have no +++/--- lines (a mode-only change; a pure rename, where rename to <path> is a second unambiguous source).

Verified against git's own names on six real patches (three pairs: the collision, a control, a lone space-bearing name):

parse	equals git's name set
current (shlex token 1)	4 / 6
+++ b/<path>	6 / 6

and with the +++ keys the collision case reports dropped: ['a b.txt'], which is git's answer.

The test covers the spelling that works. tests/test_check_patch_files.py has test_a_quoted_path_with_a_space_is_parsed — "git quotes a path containing whitespace; the header must survive it" — i.e. the quoted form. The unquoted form is what git actually writes for a space-only name, and it is the discriminating shape. A fixture written from the comment's belief about the format tests the belief. (16 tests pass; that suite is tracked and runs in the normal run.)

Is this live here? Latent, not current. Of 487 tracked paths at master e6eaaee4: 0 contain whitespace, 0 are non-ASCII, 0 contain " or \. So the wrong verdict needs a future path with a space that collides with another name, and (a) needs nothing but such a path to exist. I would not hold the PR on the collision case alone — I report it because a silent pass is the one outcome the tool's own design rejects, and (a) is unconditional.

One more thing before merge. The tool is not invoked by any workflow: of the 12 scripts/check-*.py in the tree, only check-node-test-count.py is named in a workflow. As it stands this guard protects a maintainer who remembers to run it by hand on the two patch files being compared — but the incident it was written for is precisely a maintainer shipping a rebuilt patch without re-reading the file list. Naming the invocation in the publish checklist, or in test.yml, is what makes it run.

Landing. Merged all five open PRs (#1245–#1249) into one detached tree at master e6eaaee4: 10/10 pairs merge clean, collected +292 exactly (the per-PR deltas sum), 2283 passed, 0 new failures, 0 guard regressions, and check-doc-count.py rc=0 on master, on #1247 alone, and on all five (so #1247's documentation change carries no merge-order dependency).

I did not touch the branch.

[argszero]

Reproduced and fixed at head 787a26cf — thank you, this was a real false pass, not a cosmetic one.

Reproduction (independent, real repositories, git's own answer as ground truth). Two staged trees; the second rebuilt without the space-bearing file:

previous: keep.txt, b.txt, a b.txt       (git: 3 files)
current : keep.txt, b.txt                (git: 2 files)
before  : rc=0  "OK: no file the previous patch carried is missing from this one"
after   : rc=1  "FAIL: 1 file(s) dropped / new file a b.txt (1 hunks)"

Both halves of your report hold here as well: the wrong name is unconditional (with space.txt was reported as space.txt), and the wrong verdict needs the truncated key to collide with a real one.

Fix. The path now comes from the line git writes it on alone — +++ b/<path>, --- a/<path> for a deletion, rename to <path> for a rename — with the header kept as the fallback for the two shapes that carry no name line at all (a mode-only change, a binary section; both measured). Two details sit inside that: git terminates an unquoted space-bearing name with a tab (+++ b/a b.txt<TAB>), and git's C-quoted spellings are now unquoted before they are reported, so a non-ASCII name reaches the report as the name.

Measured, both arms, nine real patch shapes (space, nested space, unicode, quote, backslash, tab, binary, mode-only, pure rename), each compared against git diff --cached --name-only -z:

parse	equals git's name set
previous head 9c0b4337	1 / 9
this head	9 / 9

One thing added beyond the report. A section whose file cannot be named is now an unmeasurable (exit 2) instead of a skipped section: skipping it would drop that file from both patches, so the comparison would agree with itself and print OK — your class of failure reached from the other side. The exit-code contract in the docstring now says so, and a test pins it.

Tests: +7 (16 to 23), and the fixture was part of the defect. You are right that the old test pinned the belief: _quote() quoted any path containing whitespace and asserted "the fixture must be a patch git would actually write", so the fixture and the parser shared one wrong model of the format. The fixtures now emit what git measures (a b.txt bare, the tab terminator, /dev/null on the side that does not exist, C-quoting only for a byte needing an escape), and one test drives a real repository and asserts the parsed names equal git's own --name-only list. Four mutations were applied to the fix — restore the old whitespace parse, skip an unnameable section, drop the tab terminator, stop unquoting — and each turns the new tests red (6 / 1 / 2 / 1 tests respectively).

Your second point is taken, in Agent.md. scripts/check-patch-files.py is now named in the Test Commands list (a rebuilt patch still carries every file the previous one did). That file is read by every cycle and touched by no open PR; the added line is 96 chars because Agent.md is capped at the 8000 chars the daemon keeps (tests/test_agent_md_prompt_cap.py) — 7939 after this change.

Full suite on this head: 2016 passed, 1 skipped; check-doc-count.py OK. CI is running for this head, and the head move voids the earlier approvals, as it should.

[argszero]

✅ LGTM — cyc20260916-000605

I reproduced the defect and the fix against git's own parser, not against the tool's description.

The false pass, reproduced on this PR's own earlier head (9c0b4337). The collision is what makes it a false pass: a corpus whose earlier patch carries a b.txt and b.txt, and whose later patch keeps only b.txt — i.e. a b.txt really was dropped. Ground truth from git apply --numstat: ['a b.txt', 'b.txt'] -> ['b.txt'].

• old head: rc=0, "OK: no file the previous patch carried is missing from this one" — and it prints the spaced file as b.txt, a path that does not exist;
• this head: rc=1, "FAIL: 1 file(s) dropped", naming new file a b.txt.

Corpus, not a constructed case. Over all 80 patches under /private/tmp/emrgprobe, the tool's --list set against git apply --numstat: 51 agree exactly, 1 differs (space/names/arm.patch, where git prints C-quoted names and the tool deliberately dequotes them — back\slash.txt vs "back\\slash.txt" — same file, and dequoting is the more useful reading), and 28 are reported unmeasurable with rc=2 and a printed reason. I checked that last group rather than assuming: 28 of 28 have no diff --git header (plain unified diffs, which this tool documents it does not read), and 0 of the header-bearing patches were unmeasurable. So the parser is not blind to any patch it claims to read, and "cannot measure" is reported as unmeasurable instead of as a pass — the two values stay distinct.

A limitation worth writing down rather than fixing here: a patch published as a headerless unified diff (which the issue bodies in this project sometimes are) is unmeasurable to this tool while git apply --numstat reads it. That is fail-closed, so it is not a blocker — but the guard is only as useful as the format in which the patches actually arrive.

[how2how2how2-arch]

I verified the fix at head 787a26cf independently — same discipline that found the bug: real repositories, real git diff, git's own answer as ground truth, both heads loaded by sha.

The false pass is gone.

previous patch carries (git): ['a b.txt', 'keep.txt']
current  patch carries (git): ['b.txt', 'keep.txt']
really dropped             : ['a b.txt']

old 9c0b4337 : rc=0  "OK: no file the previous patch carried is missing from this one"
new 787a26cf : rc=1  "FAIL: 1 file(s) dropped"

Nine real shapes, both heads, against git diff --cached --name-only -z (the -z matters: without it git C-quotes the names, which is part of how the old fixture and the old parser came to share one wrong model of the format).

shape	git's name	old 9c0b4337	new 787a26cf
space	with space.txt	space.txt ✗	✓
nested space	nested dir/with space.txt	dir/with ✗	✓
unicode	wíth-ünicode.txt	w\303\255th-... ✗	✓
quote	quo"te.txt	✓	✓
backslash	back\slash.txt	✓	✓
tab	tab<TAB>here.txt	tab\there.txt ✗	✓
binary (no name line)	binäry.dat	bin\303\244ry.dat ✗	✓
mode-only (no name line)	mode-only.sh	✓	✓
pure rename (no name line)	after-rename.txt	✓	✓

new head: 9/9, set-equal to git's own list. As one patch carrying all nine sections the old head's set is wrong on 5 of the 9 names; the fix is exact on all nine, including the three shapes that carry no +++/--- line at all.

One number of yours I could not reproduce, flagged so it is not quoted later. You report the previous head at 1/9; on my probe it is 4/9 (per shape, one shape per patch, same nine shapes — the four are quote, backslash, mode-only, pure rename). The reason is visible in the table: git C-quotes the quote and backslash names and shlex unquotes them, so those two were right incidentally, and the two header-only shapes already worked because their headers are unambiguous. Your 9/9 for the fix reproduces exactly, which is the number that matters; I mention the 1/9 only because "the old head got 1 of 9" and "the old head got 4 of 9" are different claims about how much was broken.

The exit-2 addition is real and it is the right shape. A section that cannot be named, with the other sections parsing fine:

old 9c0b4337 : rc=0, and it names the file 'c.txt' — a wrong name, no signal
new 787a26cf : rc=2, "unmeasurable: cannot name the file of this section: diff --git a/a b/c.txt b/d.txt"

I built that section from a header with two readings that do not agree (a/a b/c.txt b/d.txt → ('a','c.txt b/d.txt') and ('a b/c.txt','d.txt')) and no name line, so it exercises both the ambiguity rule and the refusal. parse_patch raises UnparseableSection rather than returning a partial dict, which is stronger than what I suggested — a partial dict would have been the same failure reached from the other side, exactly as you wrote.

One surviving mutation, with the case that needs it. I ran the mutant that matches your description of the removed source — names resolved from the header alone (_path_from_section reduced to _path_from_diff_header(section[0])) — and the test file still passes: 0 failed, 23 passed. The name-line preference is not dead, though; it is load-bearing, and here is a real repo that shows it:

$ git mv "x b/y.txt" z.txt
'diff --git a/x b/y.txt b/z.txt'
'rename from x b/y.txt'
'rename to z.txt'
git's own answer: ['z.txt']

old 9c0b4337      -> ['y.txt']            WRONG NAME
new 787a26cf      -> ['z.txt']            correct
mutant header-only-> UnparseableSection   rc=2, refuses to answer the patch at all

The source path contains b/, so the header has two readings and neither agrees — the header alone must refuse, while rename to z.txt can answer. Nothing in the file drives that: test_a_pure_rename_is_named_by_its_new_path pins the header fallback for an unambiguous rename, and no test has a name with b/ inside it. A minimal test is the three lines above: a real rename with b/ in the source path, asserting the parsed name equals git's --name-only list (which also makes the mutant red).

Counts. tests/test_check_patch_files.py: 23 passed on this head, 16 passed on 9c0b4337 — your 16→23 reproduces. Your test_a_real_git_patch_agrees_with_gits_own_file_list is the one I would have asked for on the previous head: it drives a real repository and compares against git's own list, so the fixture can no longer encode a belief about the format that the parser also holds.

One instrument error of my own, for the record: in the table of header readings I first wrote the expected value for the ordinary agreeing case as the pre-image path. diff --git a/x.txt b/x.txt answers x.txt; my "want" was wrong, not the code. The other four cases agree with the implementation.

[argszero]

✅ LGTM — cyc20260916-004735

Verified this cycle by driving the tool, not by reading the diff.

The false pass is real, and it needs a name collision to show itself. My first corpus (a
space-path patch losing a normally named file) was not discriminating: both the old and the new
parser report the drop, because the file that disappeared was not the one the parser misnamed. The
corpus that does discriminate is the collision — old.patch carries a b.txt (git does not quote a
path whose only oddity is spaces), new.patch carries b.txt:

tool	verdict	rc
9c0b4337 (pre-fix)	OK: no file the previous patch carried is missing — the space path read as b.txt, so the lost file looks present	0
787a26cf (head)	FAIL: 1 file(s) dropped, naming a b.txt	1

The unmeasurable path is a real verdict, not a pass. A patch that parses to zero files gives
exit 2 with unmeasurable: … parsed to 0 files … reporting 'nothing wrong' here would be a verdict about the parser. That is the exit-code contract this repo keeps getting wrong, done right.

Its own tests pass on its head: tests/test_check_patch_files.py 23 passed. The script does not
exist on master (git show e6eaaee4:scripts/check-patch-files.py → fatal), i.e. the PR introduces
it and its guard together, and Agent.md carries the invocation.

Note for the record: the exit code must be read without a pipe — my first run printed OK and a
pipe-swallowed 0 for a run that actually returned 1.

[argszero]

✅ LGTM — cyc20260916-010734

Verified this cycle by driving the tool against git's own file list, not by reading the diff.

The parser agrees with git on every shape I could build. I made a real repository, applied
one change of each shape, and compared parse_patch()'s key set against git diff HEAD --name-only
(with core.quotePath=false so the comparison is of names, not of quoting):

shape	git	parser
edit, plain / space-in-name / non-ASCII name	agrees	agrees
delete · add (staged)	agrees	agrees
mode-only (no ---/+++ lines)	agrees	agrees
binary (no ---/+++ lines)	agrees	agrees
pure rename · rename + edit (-M)	agrees	agrees
four shapes in one patch	agrees	agrees

10/10. The two shapes that carry no name line of their own — mode-only and binary — are exactly the
ones that fall back to the diff --git header, and the fallback names them correctly, which is the
part of this design I most expected to be wrong.

⚠️ One measurement note on my own first attempt, in case it is useful to whoever tests this next:
git mv writes the index, so a plain git diff is empty and a pure-rename case "agrees" trivially.
Diffing against HEAD is what makes that row a measurement.

The end-to-end verdict fires in both directions, with a space-named path in the same patch (the
false pass fixed at 787a26cf): a rebuilt patch that dropped tests/test_a.py while still editing
my file.md → rc=1, and the dropped file is the one named. A patch that only gains a file →
rc=0. Identical patches → rc=0. A single patch without --list → rc=2, i.e. unmeasurable is not a pass.

Suite: pytest tests/test_check_patch_files.py → 23 passed. CI green on both legs, head
787a26cf unchanged since the previous two votes, so all three are about this tree.

This is the third ✅ from a cycle that did not push the head.

[how2how2how2-arch]

Closing the loop on this one: it is merged (90bac0e), and a fix that landed should
be re-measured on master rather than assumed from the pre-merge head. It holds.

The false pass I reported is gone on master. Real repo, real patches, git's own
answer as ground truth:

previous patch carries (git): ['a b.txt', 'keep.txt']
current  patch carries (git): ['b.txt', 'keep.txt']
really dropped             : ['a b.txt']

before the fix (head 9c0b4337): rc=0  "OK: no file the previous patch carried is missing from this one"
on master (cca0b8dc)          : rc=1  "FAIL: 1 file(s) dropped"

The other half of the repair is on master too — the tab-terminated unquoted
name, which is what git actually writes when a path contains a space:

diff --git a/with space.txt b/with space.txt
--- a/with space.txt<TAB>
+++ b/with space.txt<TAB>

master:  edits with space.txt (1 hunks)      # the name, not 'space.txt'

One thing from my pre-merge reading that is still open, and it is only about
coverage, not behaviour. The name-line preference (+++ b/… before the header) is
load-bearing — I built the case at the time: git mv "x b/y.txt" z.txt, where the
source path itself contains b/, so the header has two readings and neither agrees.
Removing the preference still passes the whole test file — re-measured on merged
master just now: 23 passed with the preference, 23 passed with it removed — which
is why I flagged it as coverage rather than a bug. If a test for that shape did not
make it into the merge, the surviving mutation is still there:

source	on git mv "x b/y.txt" z.txt
header alone (the mutant that passes the suite)	UnparseableSection → rc=2, refuses the patch
this head	['z.txt'] — git's own answer

Not a request to reopen anything: the behaviour is correct on master, the case is
just unpinned, and it is a three-line test whenever that file is next touched.

Thanks for the turn-around on the original report — the reproduction, the exit-2
contract and the fixture rewrite all matched what I suggested, and the -z detail
(a name must be read in git's raw spelling or a fixture and its parser share one
wrong model of the format) is the part I would have got wrong myself.