fix(markdown-format): make the carriage-return test able to fail, and test the histogram overflow - #1608
Conversation
… the histogram overflow tested Closes three defects shipped by #1600, found by a code review that completed after that PR merged -- its automated review lanes had errored out, so the review was delegated and arrived too late. The worst was a test that could not fail. It grepped the report for a carriage return while the stub producing that report emitted plain LF, so it passed identically whether the stripping code existed or was reverted. #1600's own changelog had argued for the opposite discipline about a different assertion; this was exactly the decoration that reasoning warns against. The first rewrite of it was STILL vacuous on three of four channels, which only a revert-probe caught: on Git Bash, reading a value back through printf | jq -r | $(...) normalizes CRLF pairs away, and every CR here sits at end of line. The assertion now inspects the two-character escape in the raw emitted document instead of a decoded value that has been through a pipe. The review also reported carriage returns leaking into data.findings. Measurement says otherwise: that array is built by piping into jq -R, and on Git Bash the pipe performs the translation itself, so it never carried one. Normalization still moves to the source -- depending on an incidental property of one platform's pipe behavior is not something the next reader should have to rediscover, and one strip replaces four that each have to be remembered when a fifth consumer appears. The telemetry assertion stays but is labelled as documenting an invariant rather than guarding one, because an assertion that cannot fail on its own host must not read as coverage. Third: the +N more rule(s) suffix had only a negative test, and the stub could emit at most two rule codes, so the path this feature was named for was structurally unreachable. The stub now spreads findings across a configurable number of rule codes; the suffix is asserted with its count alongside the unchanged top-five histogram. Closes #1605 Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
Claude encountered an error after 0s —— View job I'll analyze this and get back to you. |
|
Claude encountered an error after 0s —— View job I'll analyze this and get back to you. |
|
Warning Automated security review did not complete — this is an infrastructure failure, not a review verdict. Treat any Claude comment on this PR (including a placeholder like "I'll analyze this and get back to you") as incomplete, not "no findings."
Re-running the job, or pushing a new commit, will retry the review. |
|
Warning Automated review did not complete — this is an infrastructure failure, not a review verdict. Treat any Claude comment on this PR (including a placeholder like "I'll analyze this and get back to you") as incomplete, not "no findings."
Re-running the job, or pushing a new commit, will retry the review. |
|
Claude encountered an error after 0s —— View job I'll analyze this and get back to you. |
|
Claude encountered an error after 0s —— View job I'll analyze this and get back to you. |
|
Both automated lanes errored at 0s on this PR (and on #1600 before it), so I had the diff reviewed out of band. It recommended merge, and confirmed by independent revert-probe that the two load-bearing assertions fail when the production strip is removed — 115 pass / 2 fail, exactly the two channels claimed, nothing else regressing. It also caught something I got wrong, which I have corrected in the diff rather than leaving in the record. My CRLF attribution was probably mis-assigned, and in the direction that matters. I wrote that "the pipe performs CRLF→LF translation." The mechanism is more likely Corrected in the code comment, the test comment, the assertion's own Also hardened: Left as-is: the |
…'s rule-code knob Review follow-up. I claimed "on Git Bash the pipe performs CRLF->LF translation," and the load-bearing part of that is wrong in the direction that matters. The mechanism is more likely jq.exe's own text-mode stdio, but the real problem is what I did not check: CI runs on ubuntu-24.04, Linux jq has no text/binary mode distinction, and it is documented not to strip a trailing CR from CRLF input. So the platform I measured is the one where that assertion cannot discriminate, and the platform that gates merges is the one I never tested. A Windows-only observation was written up as universal. The code comment, the test comment, the assertion's own label and the changelog now all say the discriminating power is platform-dependent: on Linux it guards, on Windows it documents. Shipped behavior is unchanged -- the strip is unconditional either way -- but the next reader is no longer told the tested half is the untested half. Also bounds STUB_RULE_KINDS. Above the eight codes the stub knows, it expanded an out-of-range positional parameter and aborted under `set -u` several frames from the cause. It now fails with a named message. Unreachable today, since only 2 and 8 are used, but a landmine for whoever raises the knob. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
Claude encountered an error after 1s —— View job I'll analyze this and get back to you. |
|
Claude encountered an error after 1s —— View job I'll analyze this and get back to you. |
1 participant
Summary
Closes three defects shipped by #1600. They were found by a code review of that PR that completed
after it merged — its automated review lanes had errored out for infrastructure reasons, so I
delegated the review, and the result arrived too late. Worth stating plainly rather than quietly fixing:
the process failure was mine, and the most useful finding is the one that shows why.
1. The carriage-return assertion could not fail
bounded/scale: report text carries no stray carriage returnsgrepped the report for\r— while thestub that produced that report emitted plain LF via
echo. It passed identically whether theCR-stripping code existed or was reverted.
That is a false safety signal, and #1600's own changelog had argued for the opposite discipline: the
histogram negative-case exists specifically "so it cannot become permanent decoration." This assertion
was exactly that decoration.
The stub now takes a
STUB_CRLFknob and emits real\r\n.And then the first rewrite of that assertion was still vacuous on three of its four channels — which
the revert-probe caught, and reasoning would not have. On Git Bash, reading a value back through
printf | jq -r | $(…)normalizes CRLF pairs away, and every CR here sits at end of line. So adecoded-value check structurally cannot see the CRs it is about. Only the fix-count line, whose CR is
mid-string, was visible; with the fix reverted, 3 of 4 assertions still passed.
The assertion now inspects the two-character
\rescape in the raw emitted document — the bytes thehook actually produces — rather than a decoded value that has been through a pipe. Both rewrites were
confirmed by revert-probe rather than by argument, which is the only way this class of defect gets
caught.
2. Carriage returns normalized at the source — with one review claim corrected
CTXandSYSMSGwere stripped after composition, leavingfindings_raw— which becomesdata.findings— reading rawFIX_OUTPUT. The review called that a live leak on the telemetry channel.Measurement says otherwise, and I would rather correct the record than quietly ship the fix as
described. That array is built by piping into
jq -R, and on Git Bash the pipe performs CRLF→LFtranslation itself, so the array never carried a CR. Confirmed by revert-probe: with the hook's
normalization removed, the report and user-message assertions fail while the telemetry one still passes.
The normalization is kept regardless, for reasons that survive the correction: relying on an incidental
property of one platform's pipe behavior is not something the next reader should have to rediscover, and
one strip at the source replaces four downstream strips that would each have to be remembered when a
fifth consumer of this output appears. The two tail-end strips are deleted.
The telemetry assertion is kept too, and labelled in the test as documenting an invariant rather than
guarding one — an assertion that cannot fail on the host that runs it must not be mistaken for
coverage. That is the same mistake as finding #1, and naming it is cheaper than repeating it.
One consequence, called out in a code comment and the changelog so it is not misread as a regression:
the delta digest is hashed over
findings_raw, so this changes that hash. Every digest recorded beforethis version invalidates once, producing one extra full-detail report per file. Self-correcting.
3.
+N more rule(s)had no positive testOnly the negative case existed (suffix absent at two rule kinds), and the stub could emit at most two
distinct rule codes — so the overflow path that feature was named for was structurally unreachable
from the suite.
The stub now takes
STUB_RULE_KINDS, defaulting to 2 so every existing case's histogram is byte-for-byteunchanged. The new case runs eight kinds and asserts both halves: the
+3 more rule(s)suffix with itscount, and that the histogram still names exactly five.
Test plan
bash plugins/markdown-format/hooks/markdown-format.test.sh— PASS=117 FAIL=0, with six newassertions (three CR channels, CR finding-count parse, histogram overflow suffix, histogram top-five
retained).
FIX_OUTPUT="${FIX_OUTPUT//$'\r'/}"removed,the
bounded/crlfassertions fail. The assertion shipped in fix(markdown-format): stop the telemetry payload lying when it exceeds argv #1600 passed in that same state, and sodid the first rewrite on three of its four channels. That difference is the whole fix — and it is why
every claim here was checked by removing the code rather than by reading it.
check-changelog-parity.sh --check-bump origin/main,check-shell-portability.sh,check-silent-skips.sh,validate-plugins.sh,shellcheck -xon bothshell files,
markdownlint-cli2andtyposover the plugin tree.Deliberately not in this PR
Three lower-priority items from the same review stay open on #1605. They are latent rather than shipped,
and folding them in would make this diff harder to check against the findings it exists to close:
build_data_jsonreturns an empty string rather than its documented fixed-shape object if ever handedan empty-string argument (unreachable from both current call sites).
files; self-corrects the moment any session touches a new one.
Related
Closes #1605
Follows #1600 and #1591.