fix(#4774): plan-criteria R4 requires the pipe not to be doubled — a logical-OR fallback is handled, not swallowed (#4877)

* test(#4774): failing-first — R4 must not read a logical-OR fallback as a pipeline stage

* fix(#4774): R4 requires the pipe not to be doubled — a logical-OR fallback is a handled failure, not a swallowed one

Also corrects the rows-3/4 test's makeCriteriaPlan usage (second arg is the
<verify> block, not a second criteria line).

* chore(#4774): backfill changeset PR number (4877)

---------

Co-authored-by: sim <sim@local>
This commit is contained in:
Tom Boucher
2026-09-19 15:38:16 -04:00
committed by GitHub
parent a87b83d485
commit 4fe2837ab5
3 changed files with 116 additions and 1 deletions

View File

@@ -0,0 +1,5 @@
---
type: Fixed
pr: 4877
---
**verify plan-structure stops warning that a logical-OR fallback swallows a failure** — the R4 scan read "||" as a pipe, so the standard "git cat-file -e <sha> || echo missing" ghost-control idiom was flagged as a swallowed failure when "||" is exactly the construct handling it; single pipes (including zsh "|&") still warn. (#4774)

View File

@@ -922,8 +922,15 @@ function scanQuantitativeCriteria(content: string): { errors: string[]; warnings
// Not a markdown table: the negated-pipe class matches a SHELL pipeline
// stage boundary (git before the next `|`), the same shape the
// #429/#968 scanners use; there is no table row to parse.
// #4774: the lookahead keeps `||` — a logical OR, the construct that
// HANDLES the failure (e.g. the `git cat-file -e <sha> || echo missing`
// ghost-control idiom, reachable here when prose apostrophes leave the
// segment's quote state unclosed) — quiet, while `|` and `|&` (a real
// pipeline stage, stderr-merged or not) still warn. Accepted trade-off:
// a QUOTED `||` literal ahead of a real pipe (``git grep 'a||b' f | wc -l``)
// also goes quiet — the lookahead cannot reach past the doubled pipe.
// allow-adhoc-markdown: shell pipeline stage boundary, not a table cell (#4024)
if (/\bgit\s+[a-z][^\n|]*\|/.test(seg)) {
if (/\bgit\s+[a-z][^\n|]*\|(?!\|)/.test(seg)) {
record(
warnings,
'R4',

View File

@@ -5199,6 +5199,109 @@ describe('#4024: scanQuantitativeCriteria — pure unit tests', () => {
`R4 is warn-only, got errors: ${JSON.stringify(result.errors)}`);
});
// #4774 — `||` is a logical OR: the construct REACTS to a non-zero exit, so
// warning that the failure is "swallowed" inverts the semantics and fires on
// the git cat-file -e <sha> || echo missing ghost-control idiom. The R4
// regex must require the pipe NOT to be doubled.
//
// Reachability (adversarial review): splitShellSegments already splits at
// UNQUOTED ||, so the plain form never reaches R4 — the false positive
// reaches it when prose leaves the segment's quote state unclosed (an
// apostrophe: "doesn't"). These tests use that shape, which is genuinely
// red under the old regex.
test('#4774 R4: a logical-OR fallback is a handled failure, not a swallowed one (apostrophe-confused prose)', () => {
const content = makeCriteriaPlan(
" - Doesn't assume tags: `git cat-file -e $sha || echo missing` exits 0 (ghost control: a bogus hash must print \"missing\").",
'',
);
const result = scanQuantitativeCriteria(content);
assert.ok(
!result.warnings.some(w => w.includes('[plan-criteria R4]')),
`|| must not read as a pipeline stage boundary, got: ${JSON.stringify(result.warnings)}`,
);
});
test('#4774 R4: a bare logical-OR fallback stays quiet in apostrophe prose (issue row 1)', () => {
const content = makeCriteriaPlan(
" - Doesn't assume history: `git log --oneline || echo none` prints at least the header line.",
'',
);
const result = scanQuantitativeCriteria(content);
assert.ok(
!result.warnings.some(w => w.includes('[plan-criteria R4]')),
`got: ${JSON.stringify(result.warnings)}`,
);
});
test('#4774 R4: logical-OR into a block stays quiet (issue row 8)', () => {
const content = makeCriteriaPlan(
" - Doesn't assume a clean tree: `git status --porcelain || { echo dirty; exit 1; }` must not print \"dirty\".",
'',
);
const result = scanQuantitativeCriteria(content);
assert.ok(
!result.warnings.some(w => w.includes('[plan-criteria R4]')),
`got: ${JSON.stringify(result.warnings)}`,
);
});
test('#4774 R4: real single-pipe pipelines still warn (issue rows 3/4)', () => {
// One criterion per test: the scanner dedupes findings BY MESSAGE, so two
// R4-worthy lines report a single warning — per-line is the suite's idiom.
for (const line of [
' - `git rev-parse HEAD | cut -c1-10` is 10 chars long.',
' - `git diff --name-only | wc -l` is 0.',
]) {
const result = scanQuantitativeCriteria(makeCriteriaPlan(line, ''));
assert.ok(
result.warnings.some(w => w.includes('[plan-criteria R4]')),
`single-pipe stage must still warn, got: ${JSON.stringify(result.warnings)}`,
);
}
});
test('#4774 R4: a pipeline boundary before the OR still warns (issue row 6)', () => {
const content = makeCriteriaPlan(
" - Doesn't assume tags: `git log --oneline | head -5 || echo none` prints 5 lines.",
'',
);
const result = scanQuantitativeCriteria(content);
assert.ok(
result.warnings.some(w => w.includes('[plan-criteria R4]')),
`git|head is a real non-final stage, got: ${JSON.stringify(result.warnings)}`,
);
});
test('#4774 R4: a |& stderr-merge is a real pipeline and still warns (issue row 7)', () => {
const content = makeCriteriaPlan(
" - Doesn't assume tags: `git grep -l \"pattern\" |& wc -l` is 0.",
'',
);
const result = scanQuantitativeCriteria(content);
assert.ok(
result.warnings.some(w => w.includes('[plan-criteria R4]')),
`|& merges stderr into the pipeline — git is still non-final, got: ${JSON.stringify(result.warnings)}`,
);
});
test('#4774 R4: a quoted || ahead of a real pipe goes quiet — pinned accepted trade-off', () => {
// Adversarial review Finding 3: the lookahead cannot reach past a doubled
// pipe, so a QUOTED '||' literal inside a grep pattern ahead of a real
// pipeline boundary is no longer flagged. This is a deliberate, disclosed
// trade-off (warn-only, narrow class, and the quoted form is exactly the
// case where "||" is most likely to be DATA, not control). Pinned here so
// a future grammar change makes the trade-off decision consciously.
const content = makeCriteriaPlan(
" - `git grep 'a||b' f | wc -l` is 0.",
'',
);
const result = scanQuantitativeCriteria(content);
assert.ok(
!result.warnings.some(w => w.includes('[plan-criteria R4]')),
`pinned trade-off: quoted || ahead of a real pipe is quiet; got: ${JSON.stringify(result.warnings)}`,
);
});
// Row 10 — phase 443 row 24: BSD wc pads, grep -x 0 never matches
test('#4024 R5: wc output compared by grep -x string equality is an error', () => {
const content = makeCriteriaPlan(