fix(#3707): degrade the fold for every UAT gap class, and stop line endings hiding rows from the audit and the acceptance gate (#3903)

* test(#3707): failing-first coverage for reverting the fence-shortfall fold shield

Pins the post-revert contract: a phase whose only gap is a fence shortfall must
degrade the fold and withhold the milestone percentages, like every other gap class.

Five of the eight rows are CONTROLS that pass before the change, and they carry more
weight than the failing row. The failure mode of this revert is degrading TOO MUCH:
a revert that sets foldScope outside the headingsSeen > 0 branch would withhold every
percentage in the project, and only the no-gap control catches that. Another control
catches a revert that collapses the two scopes into one and loses the distinction
between what a phase reports and what the fold folds -- uat.scope must stay TRUNCATED
for every gap either way, which it already is.

The row that pinned the shielded behavior is rewritten rather than deleted. Deleting
a test because the behavior it asserts is being reversed leaves the reversal
unguarded.

* fix(#3707): degrade the fold for every UAT gap class, reverting the fence-shortfall shield

Maintainer decision. The two orthogonal engines split on this during #3707 and
neither filed it as blocking, so it shipped in the shape the engine that raised the
objection endorsed after verifying seven fixtures. The call has now gone the other
way, restoring the fail-safe direction chosen twice already on this issue.

The shield exempted one gap class from the fold's teeth. It could not do that
safely: shortfallBlocks is a single tally incremented at exactly one site and spans
BOTH a harmless fenced documentation sample AND a genuinely fence-straddled
result: blocked row. Exempting it therefore could not exempt only the harmless case
-- it also published a milestone percentage over a real, unread outstanding row.
SCOPE.TRUNCATED means the scan could not SEE part of the evidence, which is exactly
that case.

scope and foldScope now agree: every gap class degrades both. The accepted
over-report documented in uat.cts is unchanged and still documented there; what
changed is only that it no longer buys an exemption from the fold.

The comment block above it argued FOR the shield and is rewritten, because a
comment defending behavior the code no longer has is worse than no comment.
shortfallBlocks leaves this function's destructure but is untouched upstream, where
audit-uat still consumes it.

* fix(#3707): correct the caller comment, add the changeset, and name what the order tests guard

Review found a SECOND comment still documenting the removed shield -- the caller's,
beside the worstScope fold, stating that foldScope differs from scope for exactly
one case which must not raise phase_scope_degraded or withhold the milestone's
percentages. That is now the opposite of what the code does. I rewrote the
buildUatRows comment in the previous commit and asserted in its message that a
comment defending behavior the code no longer has is worse than no comment, then
left exactly that one standing a few hundred lines away.

The change had no changeset. It is user-visible: a milestone's percentage goes from
published to withheld whenever any phase has a fence-shortfall-only gap. PR gates
hard-fail a user-facing code diff without one.

The two scopes are now identical at every return site. They are NOT collapsed --
that would change the return shape and the caller on what is meant to be a
one-condition revert, and the seam is worth keeping if the distinction is ever
wanted again -- but the declaration now says plainly that they agree by decision
rather than by accident, so a reader does not have to re-derive it.

The two order-independence tests were renamed. foldScope is monotonic with no reset
path, so file order is structurally irrelevant and those rows could never have
failed for the ordering reason their names promised. They do guard something real --
a multi-file phase degrading when any one file has a shortfall-only gap -- so they
now say that instead.

* test(#3707): failing-first coverage for the lone-CR UAT false-clean

The parser splits on newline only, and the heading tokenizer agrees with it, so a
lone carriage return is not a line boundary anywhere in it. CommonMark treats a lone
CR as a line ending, so such a row renders to a human reader while being invisible
to BOTH sides of the parser's symmetry invariant: no item, no shortfall, no
headingsSeen. A phase hiding a result: blocked row this way reports 100 percent with
zero diagnostics.

Found by the security review of the fold-shield revert. It is the one false-clean
class that revert does not reach, and it is the same bug class this issue exists to
fix -- an unreadable row reported as clean.

Nine rows. The LF control is what proves this is a separator defect rather than a
content defect: identical bodies, one separator apart, and only one of them hides
the row. CRLF and CR-inside-a-fence controls guard the coming normalization against
double-counting or tearing content that legitimately contains a carriage return.
Two further manifestations turned up while writing them: a leading CR breaks
column-0 anchoring of the first heading, and an all-CR document flags a shortfall it
cannot attribute to any row.

* fix(#3707): treat a lone carriage return as a line ending in the UAT parser

A lone CR was not a line boundary anywhere in the parser -- it split on newline
only, and the heading tokenizer agreed with it. CommonMark treats a lone CR as a
line ending, so such a row rendered to a human reader while being invisible to BOTH
sides of the parser's symmetry invariant: no item, no shortfall, no headingsSeen. A
phase hiding a result: blocked row that way reported 100 percent with zero
diagnostics.

Line endings are now normalized once at document ingress -- CRLF and lone CR both to
newline -- at the two independent entry points, rather than teaching each split site
about CR. Every downstream scan, offset and span therefore reads one convention.
That single-frame property is deliberate: this issue already cost a HIGH when two
scans read the same document through different frames.

MY OWN END-TO-END TEST WAS WRONG and is replaced rather than weakened. It asserted
that a lone-CR document must withhold its percentage, which reasons from the
pre-fix symptom: after the fix the row is not hidden, it is surfaced, and this
module deliberately keeps visible outstanding UAT work separate from completion
percentages -- only unreadable evidence degrades scope. The success of the fix is
what made the assertion false. The implementing agent refused to satisfy both it and
the architecture and asked instead of bending either; it was right.

What replaces it is a stronger contract: a lone-CR document and its LF twin, built
from one source, must produce identical audit output -- scope, percent, every
unresolved row by identity, and the diagnostic set. That is what 'a line-ending
convention must not change what the audit reports' actually means, and it carries a
non-vacuity check so it cannot pass with both sides empty.

shortfallBlocks keeps being returned, now documented as currently unconsumed. An
earlier reviewer told me audit-uat still consumed it and I passed that on as an
instruction; it was wrong, and it was caught by checking rather than by me.

* fix(#3707): normalize at the document read boundary, not at two call sites

The lone-CR fix was half-applied and both review engines caught it independently.
cmdAuditUat has four document ingresses, not the two I normalized: VERIFICATION.md
and deferred-items.md still handed raw text to newline-only splitters, and the
frontmatter extract in the UAT loop read raw content while its parser read
normalized -- one audit entry mixing the two frames the fix exists to unify.
Measured: a phase written twice from one source gave total_files 2 / total_items 4
under LF and results [] / total_items 0 under lone CR, with zero diagnostics.

Normalizing two call sites and declaring it done is exactly why two were missed, so
this moves it to the read boundary: every document now enters through a helper that
normalizes, in audit-uat, in planning-inspect's readDocument, and in the shared
verification-status read. Future parsers downstream get normalized text by
construction rather than because someone remembered.

That last seam also fixes an under-reporting case of the same root: a lone-CR
VERIFICATION.md saying status: passed was read as missing, telling the user a verify
step that had completed never ran.

The parity test's load-bearing assertion is now marked as such. Four of its five
equality checks still pass with the bug present -- only the unresolved-row identity
differs -- so trimming that one as redundant would make the row vacuous.

Second changeset added: the CR fix is user-visible independently of the fold revert,
and one fragment covering both would have described neither.

* test(#3707): failing-first coverage for the U+2028 and duplicate-result false-cleans

Two more of the same class, both found by the security review of this branch and
both reproduced before writing a line.

normalizeLineEndings folds only carriage returns, but a JS /m anchor also treats
U+2028 and U+2029 as line terminators while split on newline does not. That is the
identical asymmetry the carriage-return bug exploited, one separator over, and worse
in one respect: these are not CommonMark line endings, so a reader still sees the
column-0 result: blocked that the tool discards. Measured: a scalar-internal
result: pass placed after U+2028 wins over the real blocked line and the row
disappears with no gap raised.

Separately, and independent of any separator, a block with two column-0 result:
lines resolves to the first with no ambiguity signalled. Prepending result: pass to
a block therefore deletes an outstanding row silently; reversing the order surfaces
it. Order deciding meaning is the defect, so the pair of rows pins the contract as
ambiguity-is-a-gap rather than last-one-wins, leaving the fix room to implement the
gap sensibly.

Four controls: an ordinary marker in the same position (proving separator not
content), legitimate U+2028 inside prose that must not be torn, a single result line,
and a result line inside a fence that must not count as a second occurrence.

* fix(#3707): scan result lines by split, not by a multiline anchor

Two more false-cleans from the security review, both closed by the same change.

A JS /m anchor treats U+2028 and U+2029 as line terminators while split on newline
does not. A scalar-internal result: pass placed after one of those separators
therefore matched as a line start and beat the real column-0 result: blocked, and
the row vanished at 100 percent with no gap. Worse than the carriage-return case in
one respect: these are not CommonMark line endings, so a reader still saw the
blocked row the tool discarded.

Separately, the non-global match returned the leftmost hit, so a block with two
column-0 result: lines silently resolved to the first. Prepending result: pass
deleted an outstanding row; reversing the order surfaced it. Order deciding meaning
was the defect.

Both close by scanning lines produced by split rather than by anchoring a regex
inside the whole document: each line is tested on its own, and a count other than
exactly one is reported as a parse gap instead of resolved to either candidate.

I asked for U+2028 to be folded in normalizeLineEndings and that was wrong. Folding
is length-preserving, so it would have made the U+2028 fixture byte-identical to the
genuine two-result-line fixture -- while one requires a confident item and the other
requires an ambiguity gap. No implementation can satisfy both once the distinguishing
character is erased. The agent proved that and deviated rather than forcing it, which
is why normalizeLineEndings still folds only carriage returns, now with a comment
saying why.

* fix(#3707): bound the ambiguity scan at the next heading-shaped line

The split-based result scan regressed four pre-existing #3078/#3707 guards, each
off by exactly one gap.

My diagnosis was wrong. I read the off-by-one as double counting -- zero-result
blocks taking both the new path and the pre-existing one -- and said to change the
ambiguity condition from not-equal-one to greater-than-one. The agent checked and
refused: the zero path was never duplicated. The real cause is double ATTRIBUTION.
A block is sliced to the next TOKENIZED heading, so when the next row is untokenized
-- hidden by a straddling fence, or indented and already counted by the shortfall
scan -- that row's own result: line is absorbed into the previous block. The scan
then saw two result lines across what are really two rows and raised a second,
redundant gap on top of the one already counted elsewhere.

Had the greater-than-one change gone in, the counts would have matched while the
double attribution stayed. That is the compensating-adjustment failure I had asked
it to refuse, and it did.

The scan is now bounded at the first following heading-shaped line, either indent
class, so a genuine same-block ambiguity is untouched while spillover from a row
counted elsewhere is excluded.

* fix(#3707): keep the U+2028 immunity, revert the ambiguity detection

The ambiguity half of this change regressed the suite twice and is coming out.

Attempt one double-attributed: a block is sliced to the next TOKENIZED heading, so
when the real next row is untokenized its result: line was absorbed into the
previous block and raised a second gap on a row already counted elsewhere. Four
guards broke.

Attempt two bounded the scan at the next heading-shaped line and broke thirty. An
indented ### N. inside a block scalar is legitimate scalar CONTENT, not a heading,
and truncating there defeats every #3078 guard that exists to stop scalar bodies
being read as rows. Telling a genuinely hidden indented row apart from indented
scalar text is a classification countUnattributedIndentedRows already owns; a raw
regex does not have that information.

What survives is the half that is sound and was never implicated in either
regression: the result scan tests each line produced by split rather than anchoring
a regex with the multiline flag over the whole block. split never treats U+2028 or
U+2029 as a delimiter, so those separators can no longer manufacture a line start
and steal a row. Everything else returns to first-match-wins, byte-identical to
origin/next.

The two tests pinning ambiguity-as-a-gap are removed with it, since the contract is
no longer implemented here. The defect they described is real, pre-existing and
independent of any separator -- result: pass before result: blocked silently deletes
an outstanding row -- and it needs its own change with a scalar-aware counter rather
than being wedged into a branch already carrying three fixes.

* fix(#3707): correct the shared-seam rationale and restore U+2028 trailing text

The revert left a stale rationale in core-utils, justifying the decision not to fold
U+2028 by claiming uat.cts must tell a fake line start apart from a real second
column-0 result: declaration that gets flagged as ambiguous. Nothing flags ambiguity
any more; that behavior was reverted and the same file says so a few lines away. The
decision is still right, the stated reason was false.

This is the third stale comment this branch has shipped and had to fix, and the worst
placed of them: core-utils is a shared leaf that every future document consumer will
read for guidance. Rewritten to the true reason -- the scan tests each split line
individually rather than anchoring over the block, so an exotic separator cannot
manufacture a line start and folding is unnecessary.

Also a real behavior delta I had not noticed. Dropping the multiline flag left the
pattern's trailing .*$ in place, and dot never matches U+2028, so a genuine column-0
result: blocked whose TRAILING text contained one stopped parsing entirely -- a
visible parse gap rather than a false clean, so fail-safe, but a regression against
origin/next that nothing pinned. The trailing portion now matches any character and
a test pins it by identity against its plain-LF twin.

Plus the JSDoc orphaned when normalizeLineEndings moved to core-utils, and the
changeset, which described neither the separator fix nor planning-inspect surfacing
lone-CR rows.

* fix(#3707): harden the acceptance gate, which had both halves of the same bug

uat-predicate is a SECOND, independent UAT parser, and it is the one that decides
phase uat-passed. It read raw bytes and anchored a multiline regex over unsplit
text -- exactly the two defects this branch closed one module away in uat.cts.

The consequence is worse than the audit surface it mirrors. Measured on identical
bytes: a U+2028 scalar injection made the gate return passed true while planning
inspect reported the same row as blocked and outstanding. The hardened surface and
the gate disagreed, and the gate was the permissive one -- so a phase could be
accepted over a row the audit could see and the gate could not.

Both raw reads now go through the shared normalize seam and both scans test lines
produced by split rather than anchoring over the document. First-match-wins,
matching uat.cts; no ambiguity counting is reintroduced. Tests assert the AGREEMENT
between the two surfaces rather than each separately, because divergence is the
defect.

Also finishes the same root cause one module over: phase complete's advisory
pre-scan read raw bytes, so a lone-CR VERIFICATION.md lost its human_needed or
gaps_found warning -- the fix verification.cts already got on this branch.

And narrows the core-utils rationale I reworded last commit, which claimed consumers
already avoid multiline anchors. uat.cts still has five over unsplit text. That is
the fourth comment on this branch to assert something the code does not do, so it
now states only what is true of core-utils itself.

* fix(#3707): give structure and attribution different line frames, normalize the close audit

Two more from review, and the first was a regression I introduced one commit
earlier.

Converting the gate's heading scan to split-then-match removed a detection
origin/next had: a ### N. heading delimited by U+2028 was found by the old multiline
scan and was not found after. So hardening the result scan quietly weakened the
heading scan, and the gate stopped blocking on rows origin/next blocked -- the
permissive direction, on the surface that decides acceptance.

The insight I had missed is that the two scans need DIFFERENT frames. Heading
detection is structure: there is no distinction to preserve, so it splits on newline
or either exotic separator and finds a heading however it is delimited. The result
scan is attribution: the newline-only frame is exactly what stops a scalar-internal
result: from being read as a column-0 line, so it stays. One frame applied uniformly
was the error.

Second, a THIRD unnormalized parser family: the milestone-close audit read every
artifact raw. A lone-CR VERIFICATION.md degraded to status unknown and was skipped,
and deferred entries vanished outright -- measured as three items requiring
decisions under LF and one under CR, on identical bytes. All nine scanner reads now
normalize; six of them had the identical defect beyond the three review named. The
acknowledge path stays deliberately raw, since it splices by byte offset, and now
says so.

Also pins the cross-newline result: divergence, and replaces three raw U+2028
literals in test source with escapes. A raw separator in a fixture is one formatter
away from becoming an ordinary-character control that still passes -- vacuous in the
only test pinning the separator fix.

* fix(#3707): share one frame between the acknowledge writer and the audit reader

Normalizing the audit scanners left the writer and the reader on different frames.
cmdAuditAcknowledge derives its stored snapshot values from raw content -- correct
for the SPLICE, which rewrites by byte offset -- but scanUatGaps and
scanContextQuestions now recompute those same values from normalized content. For a
lone-CR artifact the two can never match, so an acknowledgement never suppresses its
item and it resurfaces on every audit: acknowledge became a silent no-op.

Fail-safe in direction, since the item stays visible rather than being wrongly
suppressed, but it is the writer and reader disagreeing about what a line is -- the
exact class this branch exists to eliminate, and the fourth instance of it here.

The derive functions now read a normalized copy while the splice keeps raw bytes and
raw offsets, so both sides share one frame and the byte-offset rewrite is untouched.
Round trip pinned for lone-CR and LF, with an existing LF marker asserted still
recognised so the change cannot silently invalidate acknowledgements already in
users' files.

Also tightens an assertion that pinned this branch's own heading fix with a proxy:
notStrictEqual against 'passed' also passes on 'pass', which IS a passing token, so
it could not have caught a regression attributing a passing result to the recovered
heading. It now pins the exact token.

* chore(#3707): backfill changeset pr numbers

Both fragments still carried the pr: 0 placeholder, which failed changeset-lint and
docs-lint on PR 3903. The review had flagged the backfill as pending and I opened
the PR without doing it.

---------

Co-authored-by: sim <sim@local>
This commit is contained in:
Tom Boucher
2026-08-26 20:17:35 -04:00
committed by GitHub
parent 6b7df61938
commit 4e8927b0b9
13 changed files with 1522 additions and 96 deletions

View File

@@ -2409,5 +2409,95 @@ describe('bug #950: quick-task SUMMARY must carry status: complete', () => {
const result = ack(tmpDir, ['--category', 'uat_gaps', '--milestone', 'v1.0']); // no --phase/--file
assert.equal(result.success, false, 'missing --phase/--file must be refused');
});
// ── mixed-frame fix (security review, #3078-CR follow-up): the writer's
// snapshot value and the scanners' recomputed value must share ONE
// frame (both normalized) even though the writer's SPLICE stays raw. A
// lone-CR artifact discriminates this: normalizing shifts every byte
// offset, so if the splice used normalized text it would corrupt the
// file, and if the snapshot used raw text it would never match the
// scanner's normalized recomputation — acknowledge would silently never
// suppress. An LF control proves the round trip isn't accidentally
// broken for the common case while fixing the CR case. ──────────────
test('mixed-frame fix: context_questions acknowledge on a lone-CR artifact actually suppresses the item', () => {
// A lone-CR CONTEXT.md is the clean discriminator: `deriveOpenQuestions`
// splits the `## Open Questions` body on `\n` — raw lone-CR content has
// NO `\n` at all, so pre-fix the writer's raw-content digest is
// computed over a ZERO-question set (sha256 of ''), which can never
// equal what the scanner (reading normalized content) recomputes — the
// acknowledge is a silent no-op. This file carries no frontmatter
// fence, so it is not entangled with the separate, pre-existing
// splice-vs-lone-CR-fence limitation a UAT/VERIFICATION file with an
// EXISTING lone-CR frontmatter block would hit.
const phaseDir = planningPath('phases', '01-alpha');
fs.mkdirSync(phaseDir, { recursive: true });
const filePath = path.join(phaseDir, '01-CONTEXT.md');
fs.writeFileSync(filePath, '# Context\r\r## Open Questions\r\r- Which backend?\r- What about auth?\r');
const before = audit(tmpDir);
assert.equal(before.counts.context_questions, 1, 'lone-CR CONTEXT file must be parsed as having open questions before acknowledge');
const result = ack(tmpDir, ['--category', 'context_questions', '--phase', '01', '--file', '01-CONTEXT.md', '--milestone', 'v1.0', '--at', '2026-08-15']);
assert.ok(result.success, `acknowledge must succeed. stderr: ${result.error}`);
assert.notEqual(
JSON.parse(result.output).questions_digest,
'e3b0c44298fc1c149afbf4c8996fb92427ae41e4649b934ca495991b7852b855',
'the recorded digest must reflect the REAL 2-question set, not sha256 of an empty set (the pre-fix raw-content bug)',
);
const after = audit(tmpDir);
assert.equal(after.counts.context_questions, 0, 'lone-CR context_questions item must be SUPPRESSED, not resurface as a silent no-op');
assert.equal(after.acknowledged.context_questions, 1);
assert.deepEqual(
(after.items.context_questions || []).filter((i) => !i.scan_error).map((i) => i.file),
[],
'the specific acknowledged file must be gone from the open items, by identity — not just a smaller count',
);
});
test('mixed-frame fix control: context_questions acknowledge on an LF artifact still suppresses the item (round trip not broken by the CR fix)', () => {
const phaseDir = planningPath('phases', '02-beta');
fs.mkdirSync(phaseDir, { recursive: true });
const filePath = path.join(phaseDir, '02-CONTEXT.md');
fs.writeFileSync(filePath, '# Context\n\n## Open Questions\n\n- Which backend?\n- What about auth?\n');
const before = audit(tmpDir);
assert.equal(before.counts.context_questions, 1);
const result = ack(tmpDir, ['--category', 'context_questions', '--phase', '02', '--file', '02-CONTEXT.md', '--milestone', 'v1.0', '--at', '2026-08-15']);
assert.ok(result.success, `acknowledge must succeed. stderr: ${result.error}`);
const after = audit(tmpDir);
assert.equal(after.counts.context_questions, 0, 'LF context_questions item must still be suppressed after the fix');
assert.equal(after.acknowledged.context_questions, 1);
assert.deepEqual(
(after.items.context_questions || []).filter((i) => !i.scan_error).map((i) => i.file),
[],
'the specific acknowledged file must be gone from the open items, by identity',
);
assert.ok(!fs.readFileSync(filePath, 'utf-8').includes('\r'), 'LF file stays LF — the fix must not introduce CR bytes on the common path');
});
test('mixed-frame fix compatibility: a HAND-AUTHORED pre-existing LF acknowledgment marker (never touched by this change\'s writer) is still recognised', () => {
const phaseDir = planningPath('phases', '03-gamma');
fs.mkdirSync(phaseDir, { recursive: true });
const filePath = path.join(phaseDir, '03-UAT.md');
// The marker's `gap_snapshot` value is computed BY HAND here, per
// `deriveUatGapSnapshotValue`'s documented shape
// (`${status}::scenarios=${openScenarioCount}`) — NOT produced by
// calling the writer — so this exercises the scanner's READ side in
// isolation against a marker that predates this change entirely. This
// content has zero `result: pending`/`[pending]` matches, so the open
// scenario count is 0.
fs.writeFileSync(
filePath,
'---\nstatus: gaps_found\naudit_acknowledged:\n milestone: v1.0\n at: "2026-08-15"\n gap_snapshot: "gaps_found::scenarios=0"\n---\n# UAT\n\n## Gaps\n\n- truth: "something broke"\n status: open\n',
);
const after = audit(tmpDir);
assert.equal(after.counts.uat_gaps, 0, 'a pre-existing LF acknowledgment marker must still suppress the item after the mixed-frame fix');
assert.equal(after.acknowledged.uat_gaps, 1);
});
});
}