fix(markdown-format): bound lint emission and disclose applied fixes - #1591
Conversation
The hook appended every line of markdownlint's whole-file output to additionalContext on every Write/Edit -- no cap, no baseline, no dedup -- so a repeatedly-edited file produced a full re-dump each time. Measured in one consuming session: 21 dumps, ~378 KB (~95K tokens), one file dumped eight times with byte-identical content, and 97% of one real file's 324 findings from a single rule that repository intentionally violates. Every run now reports the finding count and a rule histogram computed before truncation, lists at most 20 individual violations (markdown_format_max_findings, 0 = unlimited), and reports the omitted remainder as a count. An unchanged finding set drops its per-finding detail but never its summary -- a delta gate that went fully silent would recreate the invisible-hook defect on this plugin. markdownlint's banner lines no longer enter the report at all. The clean-after-fix path used to emit nothing, so --fix rewriting the user's file was indistinguishable from the hook not running. The Attempted-fixes count now goes to both channels. Telemetry stays uncapped: a sink is a machine, and the cap protects the model's context, not a log file. Closes #1589 Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
Claude finished @kyle-sexton's task in 4m 21s —— View job Review: fix(markdown-format): bound lint emission and disclose applied fixes
Summary. The changes are correct and address a real production problem (95 KB / ~21 context dumps per session). The design decisions documented in the PR body — cap in digest key, always-visible summary, uncapped telemetry — are well-reasoned and match the implementation. No security issues introduced. A few minor observations below. CorrectnessFinding 1 — find "$digest_dir" -type f -mtime +7 -delete 2>/dev/nullThe cleanup scan runs unconditionally every time a Markdown file is linted. For a consumer with a well-used Finding 2 — find "$digest_dir" -type f -mtime +7 -delete 2>/dev/nullThe Finding 3 — Histogram awk 'NR<=5 {printf "%s%s x%s", (NR>1 ? ", " : ""), $2, $1} END {print ""}' 2>/dev/nullIf a file has more than 5 distinct rules, the histogram silently omits them. Given the histogram's purpose — showing which rules dominate — this is rarely a problem, but a trailing Finding 4 — if [[ "$line" =~ [[:space:]](MD[0-9]+)/ ]]; then
findings_raw+="$line"$'\n'
FINDING_COUNT=$((FINDING_COUNT + 1))
RULE_TALLY+="${BASH_REMATCH[1]}"$'\n'
fiNo intervening commands between the Finding 5 — If Security
Design ObservationsThe cap-in-digest design is correct. Including The always-visible summary is the right choice. The PR's explicit test at Telemetry deliberately uncapped. Double TestsThe new test phase (lines 1445–1627) is well-structured. Specific strengths:
One observation: Overall verdict: This is a solid, production-ready fix. The correctness is sound, security is properly handled, and the test suite covers the advertised invariants. The items above are minor — Finding 1 (find frequency) and Finding 2 (maxdepth) are the only ones worth a follow-up. None are blockers. |
|
Claude finished @kyle-sexton's task in 5m 11s —— View job Security Review — PR #1591
Verdict: No exploitable security vulnerabilities found.The three new attack surfaces this PR introduces are all handled correctly. Analysis by surface: 1.
|
…s argv (#1600) ## Summary Follow-ups to #1591, which merged before these landed. One is a real defect that #1591's own claim about telemetry made visible; the rest are that PR's non-blocking review findings. **The telemetry payload could be falsified, not merely lost.** `build_data_json` handed the findings array to `jq -n` as an `--argjson` value. Windows caps a process command line at 32767 characters, and `data.findings` is deliberately uncapped — so the array blew past it between 300 and 600 entries. Reproduced with the same jq the hooks use: ```text n=100 serialized 6401 B rc=0 findings preserved n=300 serialized 19201 B rc=0 findings preserved n=600 serialized 38401 B rc=126 <- argument list too long n=1200 serialized 76801 B rc=126 ``` `jq` never ran, and the `|| printf '{"tool":"","file":"","findings":[]}'` fallback emitted an envelope claiming **zero** findings with `tool` and `file` blanked — for the noisiest files in the repository, which are exactly the ones a sink is most likely wired for. A real 324-finding file is past the limit, so this was not a corner case; #1591's body and CHANGELOG both affirmatively claimed the payload keeps its full contents. The array now reaches `jq` on stdin. `tool` and `file` stay as arguments — both bounded by a path length. **There are two such call sites and only one is this plugin's.** `hook::emit_telemetry` hands the *finished* payload over the same way from the byte-synced `lib/hook-utils.sh`; fixing it there is a sync-and-bump wave across every carrying plugin, filed as #1595. So the honest end-to-end behaviour today is that an oversized envelope is **dropped**, not delivered — and that is the point. Telemetry is documented best-effort and lossy, so a dropped envelope is inside contract; one that arrives reporting a 600-finding file as clean is not. **This PR moves the failure from a lie to a loss**, and the new case asserts that invariant rather than a count, so it keeps passing unchanged once #1595 lands. The existing 50-finding telemetry assertion could not have caught this — 50 findings is about 6 KB, comfortably under the limit. Two runs that each passed for a different reason. Also in this PR, the three non-blocking findings from #1591's review: - **Prune frequency.** The digest sweep ran on every Markdown edit. It now runs only when a *new* digest file is created — the steady state for a repeatedly-edited file already has one, so the common path no longer walks the directory at all. That needs no session sentinel and no extra state. - **`-maxdepth 1`.** Added, and for a stronger reason than a hypothetical subdirectory: `CLAUDE_PLUGIN_DATA` is shared with the `trust-approvals` tree and with whatever a future version of this plugin puts there. A recursive age-based `-delete` has no business reaching into a sibling's state, so the bound is stated as a boundary rather than a micro-optimization. - **Silent histogram truncation.** The histogram now carries `+N more rule(s)` when more than five rules fire, so `MD013 x48` cannot read as the whole story on a file where twelve others are firing. And one more found while verifying the above: **carriage returns leaked into the report**. `markdownlint-cli2` is a Node process whose stdout is CRLF-terminated on Windows, and command substitution strips only the trailing newline — so every retained violation line carried a CR that survived JSON-escaping into `additionalContext` as a literal `\r`. Pre-existing, but it lands in the report format #1591 restructured. Findings 4 and 5 of that review were self-resolved as non-issues; I agree with both readings and made no change. ## Test plan - `bash plugins/markdown-format/hooks/markdown-format.test.sh` — **PASS=112 FAIL=0**, including six new or changed assertions: the oversized payload is dropped rather than falsified, the report still states the true count at 601 findings, no carriage returns in the report text, the histogram's omitted-rule suffix appears when it should, and does **not** appear when every rule fits. - The argv ceiling itself was reproduced directly against the hooks' own jq at 100 / 300 / 600 / 1200 findings (table above) before and after the change. - Repo gates run locally, all green: `check-changelog-parity.sh --check-bump origin/main`, `check-shell-portability.sh`, `validate-plugins.sh`, `shellcheck -x` on both shell files, and `markdownlint-cli2` on the changed Markdown. ## Related Closes #1597 Follows #1591. The shared-library half is #1595. Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
… test the histogram overflow (#1608) ## 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 returns` grepped the report for `\r` — while the stub that produced that report emitted plain LF via `echo`. **It passed identically whether the CR-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_CRLF` knob 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 a decoded-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 `\r` escape in the **raw emitted document** — the bytes the hook 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 `CTX` and `SYSMSG` were stripped after composition, leaving `findings_raw` — which becomes `data.findings` — reading raw `FIX_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→LF translation 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 before this version invalidates once, producing one extra full-detail report per file. Self-correcting. ### 3. `+N more rule(s)` had no positive test Only 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-byte unchanged. The new case runs eight kinds and asserts both halves: the `+3 more rule(s)` suffix with its count, 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 new assertions (three CR channels, CR finding-count parse, histogram overflow suffix, histogram top-five retained). - **Revert probe**, which is the point of finding #1: with `FIX_OUTPUT="${FIX_OUTPUT//$'\r'/}"` removed, the `bounded/crlf` assertions fail. The assertion shipped in #1600 passed in that same state, and so did 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. - Repo gates green locally: `check-changelog-parity.sh --check-bump origin/main`, `check-shell-portability.sh`, `check-silent-skips.sh`, `validate-plugins.sh`, `shellcheck -x` on both shell files, `markdownlint-cli2` and `typos` over 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_json` returns an empty string rather than its documented fixed-shape object if ever handed an empty-string argument (unreachable from both current call sites). - Prune-on-create can leave stale digests untouched in a long session that only re-edits already-seen files; self-corrects the moment any session touches a new one. - The scale test's negative branch cannot distinguish "correctly dropped" from "harness broken". ## Related Closes #1605 Follows #1600 and #1591. --------- Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
1 participant
Summary
markdown-formatappended every line of markdownlint's whole-file output toadditionalContexton every
Write/Edit— no cap, no baseline, no dedup — so a repeatedly-edited file produced a fullre-dump each time. Measured in one consuming session: 21 dumps, ~378 KB (~95K tokens), one file
dumped eight times with byte-identical content, and 97% of one real file's 324 findings from a single
rule that repository intentionally violates as house style.
What changed:
noisy.md has 50 markdownlint finding(s) — MD013 x48, MD032 x2.A truncated first-N list destroys exactly the information that makes a bulkreport actionable — which rule is firing — so the histogram is computed before truncation.
count and a pointer at the one action that actually fixes a dominant rule: configure it once in the
repository's markdownlint config.
file per session under
CLAUDE_PLUGIN_DATA; a repeat reports its summary and omits the per-findinglines. The summary always goes out — a delta gate that went fully silent would recreate, on this
plugin, exactly the invisible-hook defect typos-format rewrites file content on every edit with no user-visible disclosure #1578 is about. There is a test asserting the repeat is
not silent, phrased as such.
Finding: <path> !**/node_modules/** …glob list,Linting:,Summary:— none says anything aboutthe edited file, and all of them were paid for on every edit.
making
--fixrewriting the user's file indistinguishable from the hook not running. TheAttempted: N fixes in 1 filecount now goes to both channels, per the content-mutation clauseadded to
docs/conventions/hook-observability/README.mdin fix(typos-format): disclose every applied correction on both channels #1580. It is only a count —markdownlint-cli2 offers no per-fix detail, so neither can this hook, and the report says so rather
than implying more.
markdown_format_max_findingsuserConfig (default20,0= unlimited), read from theCLAUDE_PLUGIN_OPTION_environment mirror because shell-form hook commands reject${user_config.*}substitution outright (Plugins reference, "User configuration",https://code.claude.com/docs/en/plugins-reference, fetched this session). A value that is not a
non-negative integer falls back to the default and is never interpolated anywhere; there is a test
that feeds it
not-a-number; rm -rf /.Two design points worth a reviewer's attention
markdown_format_max_findingsto see the rest. A digest over the finding set alone would answerthat with "unchanged, detail omitted" — making the hook's own advice impossible to act on. The
digest covers
MAX_FINDINGS+ the finding set, so raising the cap brings the detail back on anotherwise unchanged file. There is a test for it.
model's context, not a log file.
data.findingskeeps its shape and its full contents, so theschema is unchanged.
Test plan
bash plugins/markdown-format/hooks/markdown-format.test.sh— full suite green, including 17 newassertions: the default cap, the configurable cap,
0= unlimited, a garbage cap value falling backsafely, the rule histogram surviving truncation, banner exclusion, the delta gate suppressing detail,
the delta gate not going silent, a changed set restoring detail, a cap change defeating the delta
gate, uncapped telemetry (50 findings still reach the sink), and applied-fix disclosure on both
channels plus silence when nothing was fixed.
controllable finding count nor the
Attempted:line), so they execute on the CI runner, which has noreal
markdownlint-cli2.check-changelog-parity.sh --check-bump origin/main,check-hook-userconfig-argv.sh,check-silent-skips.sh,check-shell-portability.sh,validate-plugins.sh,shellcheck -xon both shell files,markdownlint-cli2on the changedMarkdown, and
typosover the plugin tree.(banner lines,
Attempted: N fixes in 1 file,Summary: N issues, then violation lines carryingMD<digits>/).Not addressed here
typos-format,markdown-format, andeol-normalizerall matchWrite|Edit, run in parallel with no documented ordering and no locking). No hook-level lockingprimitive exists in Claude Code today; it stays documented as a known limitation.
INPUT=$(hook::buffer_stdin) || exit 0discards the helper'sBLOCKED diagnostic). That idiom lives in the shared
lib/hook-utils.shand is fleet-wide across~15 plugins, so fixing it here would either fork the shared library or force a version bump wave
across every carrying plugin. Deferred deliberately, not overlooked.
Related
Closes #1589