Surfaced by fresh-context verification of #2843.
Scope note, stated up front: parse_attribution_field and the
[<sha>=<n>,…] attribution grammar do not exist on main (5958e1faf) —
main's verify_known_incidents still reads while read -r expect sha note
and asserts exit status alone. This defect is in #2843's branch code, measured
at 39ecf3088 (scripts/check-silent-revert.sh:575-593). Filed as an issue
rather than left as a PR note because it is a doctrine/code disagreement worth
a decision, and because the fix is a one-line grammar tightening either way.
The doctrine
The attribution block's own header in check-silent-revert.sh:560-562 states
the contract without qualification:
A malformed field is exit 2 (cannot run), never a FAIL and never a pass: an
expectation silently misread is the same false-green this whole file exists
to remove.
And parse_attribution_field's docstring (:569-570):
Parses [<sha>=<n>,<sha>=<n>] into normalized <sha> <n> lines on stdout.
Exits 2 on anything that is not exactly that grammar.
A trailing comma is not that grammar. It is accepted anyway.
Reproduction
Temp incidents file outside the checkout, one row, real shipped sha and count
(a95f240f75…=346, copied from scripts/silent-revert-incidents.txt:50 —
that file was not modified):
fires f603880dad4003477ec7cac27654fce92c75eec8 [a95f240f7502ff9f1e5411602e79ccbd4b9056c0=346,] trailing comma
$ SILENT_REVERT_INCIDENTS=comma-trailing.txt bash scripts/check-silent-revert.sh --verify-known-incidents
ok f603880da fires as recorded, 1 attribution(s) reproduced exactly (trailing comma)
Canary reproduces every recorded incident at the shipped settings.
EXIT=0
Exit 0, and the row reports itself as reproduced exactly.
Cause
parse_attribution_field splits with word splitting rather than a parser
(:580-581):
local IFS=','
for entry in $field; do
Unquoted expansion under IFS=',' drops trailing empty fields — that is
word splitting's defined behavior, not a bug in the loop body. The empty entry
never reaches the *=* case that would die. Leading and internal empties do
survive splitting and are caught.
Only the trailing position elides — verified
Every other malformed comma placement correctly hard-stops at exit 2:
$ # leading: [,a95f240f75…=346]
$ SILENT_REVERT_INCIDENTS=comma-leading.txt bash scripts/check-silent-revert.sh --verify-known-incidents
check-silent-revert: malformed attribution entry '' in comma-leading.txt (want <40-char-sha>=<lines>)
EXIT=2
$ # doubled / internal-empty: [a95f240f75…=346,,eda5ae5ed5…=1]
$ SILENT_REVERT_INCIDENTS=comma-doubled.txt bash scripts/check-silent-revert.sh --verify-known-incidents
check-silent-revert: malformed attribution entry '' in comma-doubled.txt (want <40-char-sha>=<lines>)
EXIT=2
$ # comma-only: [,]
$ SILENT_REVERT_INCIDENTS=comma-only.txt bash scripts/check-silent-revert.sh --verify-known-incidents
check-silent-revert: malformed attribution entry '' in comma-only.txt (want <40-char-sha>=<lines>)
EXIT=2
So the grammar is enforced in three of four positions and silently relaxed in
the fourth.
Impact — LOW, and here is why
Stating this plainly so it is triaged as the paper cut it is, not as a
false-green:
- The parsed set is still correct. The trailing empty is discarded, not
misread as an entry. [X=346,] parses to exactly the same one-element
expectation as [X=346], so no wrong assertion can pass — the comparison
against the detector's findings is unaffected. This is not the false-green
class the header paragraph is defending against.
- Unreachable for the shipped corpus.
t_shipped_data_files_are_wellformed
in scripts/check-silent-revert.test.sh requires every shipped fires row
to match
^fires[[:space:]]+[0-9a-f]{40}[[:space:]]+\[[0-9a-f]{40}=[0-9]+(,[0-9a-f]{40}=[0-9]+)*\][[:space:]],
which rejects a trailing comma outright. A trailing comma cannot ship in
silent-revert-incidents.txt without that test going red.
- Reachable only via a custom
SILENT_REVERT_INCIDENTS path
(check-silent-revert.sh:192), which the regex test does not read.
Filed so the doctrine and the code agree. A file that says "exits 2 on
anything that is not exactly that grammar" and then accepts something outside
that grammar is a claim a future reader will trust and a future change will
lean on — the same class of drift #2846 and #2865 are about, at a much smaller
scale.
Suggested fix
Reject the empty entry explicitly rather than relying on splitting. The
narrowest change is to fold '' into the existing case:
case "$entry" in
'') die "empty attribution entry in $INCIDENTS_FILE (trailing or doubled comma?)" ;;
*=*) ;;
*) die "malformed attribution entry '$entry' …" ;;
esac
That alone does not help, because word splitting removed the trailing empty
before the loop ever runs — so the field needs a shape check before splitting,
e.g. rejecting a field whose contents start or end with ,, or validating the
whole field against the same regex t_shipped_data_files_are_wellformed
already uses before splitting it. Whichever shape is chosen, a test pinning all
four comma positions to exit 2 is what keeps them from diverging again.
Related
Surfaced by fresh-context verification of #2843.
Scope note, stated up front:
parse_attribution_fieldand the[<sha>=<n>,…]attribution grammar do not exist onmain(5958e1faf) —main'sverify_known_incidentsstill readswhile read -r expect sha noteand asserts exit status alone. This defect is in #2843's branch code, measured
at
39ecf3088(scripts/check-silent-revert.sh:575-593). Filed as an issuerather than left as a PR note because it is a doctrine/code disagreement worth
a decision, and because the fix is a one-line grammar tightening either way.
The doctrine
The attribution block's own header in
check-silent-revert.sh:560-562statesthe contract without qualification:
And
parse_attribution_field's docstring (:569-570):A trailing comma is not that grammar. It is accepted anyway.
Reproduction
Temp incidents file outside the checkout, one row, real shipped sha and count
(
a95f240f75…=346, copied fromscripts/silent-revert-incidents.txt:50—that file was not modified):
Exit 0, and the row reports itself as reproduced exactly.
Cause
parse_attribution_fieldsplits with word splitting rather than a parser(
:580-581):Unquoted expansion under
IFS=','drops trailing empty fields — that isword splitting's defined behavior, not a bug in the loop body. The empty entry
never reaches the
*=*case that woulddie. Leading and internal empties dosurvive splitting and are caught.
Only the trailing position elides — verified
Every other malformed comma placement correctly hard-stops at exit 2:
So the grammar is enforced in three of four positions and silently relaxed in
the fourth.
Impact — LOW, and here is why
Stating this plainly so it is triaged as the paper cut it is, not as a
false-green:
misread as an entry.
[X=346,]parses to exactly the same one-elementexpectation as
[X=346], so no wrong assertion can pass — the comparisonagainst the detector's findings is unaffected. This is not the false-green
class the header paragraph is defending against.
t_shipped_data_files_are_wellformedin
scripts/check-silent-revert.test.shrequires every shippedfiresrowto match
^fires[[:space:]]+[0-9a-f]{40}[[:space:]]+\[[0-9a-f]{40}=[0-9]+(,[0-9a-f]{40}=[0-9]+)*\][[:space:]],which rejects a trailing comma outright. A trailing comma cannot ship in
silent-revert-incidents.txtwithout that test going red.SILENT_REVERT_INCIDENTSpath(
check-silent-revert.sh:192), which the regex test does not read.Filed so the doctrine and the code agree. A file that says "exits 2 on
anything that is not exactly that grammar" and then accepts something outside
that grammar is a claim a future reader will trust and a future change will
lean on — the same class of drift #2846 and #2865 are about, at a much smaller
scale.
Suggested fix
Reject the empty entry explicitly rather than relying on splitting. The
narrowest change is to fold
''into the existingcase:That alone does not help, because word splitting removed the trailing empty
before the loop ever runs — so the field needs a shape check before splitting,
e.g. rejecting a field whose contents start or end with
,, or validating thewhole field against the same regex
t_shipped_data_files_are_wellformedalready uses before splitting it. Whichever shape is chosen, a test pinning all
four comma positions to exit 2 is what keeps them from diverging again.
Related
parse_attribution_field; this is adefect in that branch's code, not on
main.of it.