fix(typos-format,markdown-format): single-writer default ends the unconditional rewriter race

[kyle-sexton]

Fixes #1809

Summary

Implements the single-writer-by-default decision recorded on #1809 (delegated-authority comment), dissolving the undefined-precedence race between the marketplace's two unconditional in-place rewriters instead of adjudicating it:

• typos-format 0.4.4 → 0.5.0: typos_format_write_changes default flips true → false in both the manifest and the script fallback. Out of the box the hook is report-only and never modifies a file; write mode is an explicit opt-in whose gate now requires the literal true (the mutating direction needs the exact opt-in spelling). Removes the silent-corruption class including typos-format: hook silently corrupts abbreviated git SHAs, including inside backticks #1257, and lands before any block-hook-bypass write-leak closure per the hard ordering constraint in the issue.
• markdown-format 0.8.6 → 0.9.0: the run is gated on a markdownlint config the repository itself carries — one of the ten file names markdownlint-cli2 documents as automatically discovered, anywhere between the edited file's directory and the repo root. No config → no run: neither --fix rewrites nor default-rule findings (the ~115-finding MD013 class) are imposed on a repo that never chose a Markdown style. Same doctrine as bash-format's shfmt gate. A package.json markdownlint-cli2 property does not open the gate (honored by markdownlint-cli2 only under an explicit --config flag; its README, fetched 2026-07-31).
• docs/conventions/hook-budget/README.md (new): the always-on cost ceiling marketplace: two unconditional in-place rewriters race on every Write with no defined precedence, nobody sums the always-on per-turn cost, and thirteen CRITICALs need an ordering #1809's item 2 asked for — ≤ 1 s typical / 2 s worst-case parallel wall per tool call, ≤ 500 ms per turn — with the measured 2026-07-31 accounting (per-Bash-call always-on set ≈ 5.9 s parallel wall on the Windows reference host; per-Write set ≈ 1.9 s) and the rule that the ceiling never relaxes to absorb an overage.

Resulting invariant: a default install has zero unconditional writers on any file class. The one re-openable overlap — typos write mode opted on in a repo that also carries a markdownlint config — is a deliberate double opt-in documented in both READMEs as last-writer-wins. Residual scoped-writer overlap stays tracked in #875; the evidence-packet integrity item routes to #1808; stage 2 of the ordering (guardrails rc=2 one-liner) is #1858.

Test plan

• plugins/typos-format/hooks/typos-format.test.sh: 81/81 pass. New stub/default-report-only case pins the race fix — with no write-mode option set the file stays byte-identical while findings are still reported (fails if the default ever flips back); explicit-false override retained; all write-path disclosure/scale/telemetry contracts re-pinned under the opt-in.
• plugins/markdown-format/hooks/markdown-format.test.sh: 120/120 pass. New opt-in-gate cases: a config-less repo with fixable violations is left byte-identical with no output; a config nested below the root still opens the gate and --fix applies.
• Gates run locally: shellcheck (hooks + tests, clean), check-shell-portability --paths (clean), check-changelog-parity --check / --check-bump origin/main / --check-order (all pass), check-silent-skips, check-hook-userconfig-argv, check-changed-skills (no skills changed), markdownlint-cli2 on all five touched/added Markdown files (0 issues), manifests parse.

Related

• marketplace: two unconditional in-place rewriters race on every Write with no defined precedence, nobody sums the always-on per-turn cost, and thirteen CRITICALs need an ordering #1809 — decision comment with rationale, rejected lock/dispatcher alternative, and measured cost accounting
• guardrails: block-noncanonical-commit exits 0 on classify rc=2 (stage 2 of #1809's ordering) #1858 — stage 2 (guardrails block-noncanonical-commit rc=2 exit), staged per the ordering decision
• typos-format: hook silently corrupts abbreviated git SHAs, including inside backticks #1257 — silent SHA corruption instance removed by the report-only default
• fleet: concurrent PostToolUse hook writes can silently clobber each other (no locking/ordering) #875 — fleet tracker for the residual scoped-writer overlap class
• plugin-quality: the resume rule breaks on any multi-target audit, retention is a model-executed recursive delete over the tree holding the deliverable, and packets are mutable by sibling hooks #1808 — evidence-packet integrity (item 4 of marketplace: two unconditional in-place rewriters race on every Write with no defined precedence, nobody sums the always-on per-turn cost, and thirteen CRITICALs need an ordering #1809), owned there

🤖 Generated with Claude Code

https://claude.ai/code/session_01FVoZoMYXqf8ZVbQYixPVPW

[claude]

Claude finished @kyle-sexton's task in 1m 50s —— View job

---

Claude finished the review

Todo list

• Gather context (diff, changed files, CLAUDE.md/REVIEW.md scope)
• Review typos-format changes (plugin.json, hook script, tests, README, CHANGELOG)
• Review markdown-format changes (plugin.json, hook script, tests, README, CHANGELOG)
• Review new docs/conventions/hook-budget/README.md
• Post review findings

Summary

This is a well-scoped, well-documented fix for the undefined-precedence race between the two unconditional in-place rewriters, per the decision recorded on #1809. I reviewed the full diff (origin/main...HEAD, both commits) against the code-review lane's scope (correctness, design, error handling, tests, docs — no security findings to report; this repo has no claude-security-review workflow, but nothing security-relevant surfaced here anyway).

No blocking issues found. A few observations, all non-blocking:

1. The fail-closed fix already landed in the second commit is correct. markdown-format.sh#L189 now returns 1 when REPO_ROOT can't resolve to a real directory, rather than leaving root="" and letting the upward walk run to the filesystem root. Given hook::repo_root (hook-utils.sh) always falls back to a real directory (never empty) when git rev-parse fails, this is a defensive belt-and-suspenders fix rather than a live bug fix, but it's the right posture and cheap.

2. Test coverage is solid and pins the actual regression class. The new stub/default-report-only case (typos-format.test.sh#L290) asserts byte-identical output with no write-mode option set at all (not just an explicit false), which is exactly the out-of-the-box posture that matters. The markdown-format opt-in-gate cases cover both the no-config (no run at all) and nested-config-below-root (gate opens) paths — matching the discovery span markdownlint-cli2 itself documents.

3. Docs/CHANGELOG/README are internally consistent. Both plugin READMEs, CHANGELOGs, and the manifest descriptions all agree on the new default and the opt-in semantics; the hook-budget doc's second-commit edit correctly scopes its own claim ("these figures remain the binding accounting until re-measured") rather than overclaiming the per-Write set shrank universally.

4. Minor/non-blocking: the version bumps (0.4.4→0.5.0, 0.8.6→0.9.0) are minor bumps for what is, behaviorally, a breaking default flip for any consumer relying on the old unconditional-write behavior. That's consistent with typical pre-1.0 semver practice (0.x minor = breaking) and I don't see a documented convention in this repo that says otherwise, so I'm not flagging it as an error — just noting it in case the intent was a 1.0.0.

5. Per REVIEW.md's code-review-lane checks: no multi-write atomicity concern applies here (the change removes an unconditional writer rather than adding coordinated writes), and no high-risk security action requiring an audit-log entry is introduced.

I was unable to execute the hook test suites or shellcheck directly in this sandboxed environment (Bash required interactive approval I couldn't obtain for tool-availability checks or git fetch), so I relied on static review of the diff and the pre-existing test files rather than re-running the 81/81 and 120/120 suites the PR description reports.

[claude]

Claude finished @kyle-sexton's task in 2m 0s —— View job

---

Security Review — PR #1859

• Gather context (gh pr diff / git diff origin/main...HEAD, read changed files)
• Review shell scripts for command/path injection, unsafe eval, quoting issues
• Review default-flip logic (typos-format write gate, markdown-format config gate) for logic/bypass flaws
• Review manifest/config changes for permission-widening
• Post findings

---

Result: No security issues found

Scope: all 11 changed files (plugins/typos-format/**, plugins/markdown-format/**,
docs/conventions/hook-budget/README.md). This PR is security-positive — it removes an
unconditional-writer race rather than introducing one — and I found nothing to flag under the
categories in scope for this lane (logic flaws, authz/access-control gaps, injection, unsafe
secret/token handling, dangerous Actions patterns). Notes on what I specifically checked:

typos-format write-gate flip — typos-format.sh:
the gate changed from [[ "$WRITE_CHANGES" != "false" ]] (write unless explicitly opted out) to
[[ "$WRITE_CHANGES" == "true" ]] (write only on the exact opt-in spelling), and the manifest
default (plugin.json#L30) and script fallback
(typos-format.sh#L200) agree (false in both). No mismatch that would silently
re-open write mode. A malformed/typo'd option value now fails closed to report-only rather than
failing open to write — the correct direction for a mutating default.

markdown-format config opt-in gate — markdown-format.sh#L185-L211
(markdownlint_config_discoverable): both the file-dir resolution and the repo-root resolution
fail closed (return 1 → skip, no run) if cd/pwd -P can't resolve them, so an unresolvable
root can't degrade into an unbounded upward directory walk. This is a genuine improvement over the
naive version — an empty/unset root would otherwise never satisfy the loop's $dir == $root
termination check. This new gate sits before the existing collect_risky_configs /
code-execution trust-gate logic (.cjs/.mjs config, customRules/markdownItPlugins/
outputFormatters) further down the file — that trust gate is untouched by this PR and still
applies in full to any repo that does open the opt-in gate, so no bypass of the code-execution
approval flow was introduced.

Manifest/config changes — both plugin.json diffs only tighten defaults
(typos_format_write_changes: true → false) or narrow when a hook fires (markdown-format now
requires a discoverable config); no permission widening, no new userConfig keys read via
${user_config.*} interpolation (both hooks correctly continue reading options via the
CLAUDE_PLUGIN_OPTION_<KEY> env mirror rather than shell-interpolating a configured value).

Tests / docs / CHANGELOGs — the new test cases in both .test.sh files exercise exactly the
gate/default behavior described above (byte-identical-file assertions), and the README/CHANGELOG
prose is consistent with the code; nothing security-relevant there beyond documentation.

Per the review brief, I deferred to zizmor's advisory lane for supply-chain/unpinned-action risk,
dangerous trigger patterns, excessive permissions, and template injection — none of the changed
files are GitHub Actions workflows, so that lane isn't in play for this PR anyway.

[github-actions]

Claude has reviewed this PR 3 times. The lane skips further automatic reviews after 5; deleting this comment resets the count.

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 5ed23360c8

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

[claude]

Claude finished @kyle-sexton's task in 3m 45s —— View job

---

Code review — PR #1859

Todo list

• Gather context (git diff origin/main...HEAD, review the 3rd commit's response to Codex's prior findings)
• Re-check the two Codex P2 findings on stale SKILL.md setup docs (typos-format, markdown-format)
• Re-check the Codex P2 finding on jq notice ordering in markdown-format.sh
• Review the new commit 201f438 on its own merits
• Post findings

Summary

Commit 201f438 correctly resolves all three P2 findings from Codex's earlier pass on this PR, on top of the already-reviewed single-writer-precedence fix. No blocking issues.

1. jq-notice-before-opt-in ordering — fixed correctly. markdown-format.sh#L113-L122 now pre-decides the opt-in gate before hook::require_jq can fire its once-per-session notice, restoring the "no config, no run, no notice" contract even when jq is absent. The jq-free decode (\" → \/ → \\, in that order) reconstructs a Windows-escaped path correctly — I hand-traced several backslash/quote adjacency cases (including runs of backslashes abutting an escaped quote) and the ordering holds up. The [[ -f "$DECODED_FILE" ]] existence check is a sound safety net: if decoding ever mis-fires on some escape sequence the 3-pattern decode doesn't handle (e.g. \uXXXX), the check just fails and falls through to the normal hook::require_jq notice — i.e. worst case reverts to the pre-fix behavior, never a false skip of a real notice.

2. Both SKILL.md setup docs — fixed correctly. typos-format/skills/setup/SKILL.md#L56 now documents the shipped false default and the exact-true opt-in; markdown-format/skills/setup/SKILL.md#L44-L58 now reports a config-less repo as INFO — inactive, not PASS rather than "markdownlint defaults apply" (which is no longer true post-gate). Both apply sections were updated in step with check.

3. Minor, non-blocking: no test exercises the actual backslash-decode path. The new test at markdown-format.test.sh#L580-L593 pins "missing jq + no config → no notice," but its fixture path has no backslashes to escape, so DECODED_FILE equals RAW_FILE unchanged — the escape-decode logic itself (lines 113-116) isn't exercised by any test, only reasoned about in the code comment. Since the JSON payload is just a string the test constructs (not OS-dependent), a case simulating a Windows-style path — e.g. a file_path containing literal \\ sequences — could be added even on Linux CI to pin the decode order against future regressions. Not blocking; the existence-check fallback already keeps a decode bug from producing worse-than-before behavior.

4. Everything else from the earlier reviews on this PR (the fail-closed REPO_ROOT resolution, the write-gate flip, the config opt-in gate itself, the hook-budget doc) still holds — this commit only touches the notice-ordering fix and the two setup docs, and I re-verified none of it regressed those.

[claude]

Claude finished @kyle-sexton's task in 2m 28s —— View job

---

Security Review — PR #1859

Scope: full diff origin/main...HEAD (14 files) via git diff origin/main...HEAD, with focused re-review of the third commit (201f438, "decide the opt-in before any prerequisite notice") that landed since the prior security review on 5ed2336.

Result: No security issues found

What's new since the last review — 201f438 reorders markdown-format.sh so the config opt-in gate is evaluated before hook::require_jq's prerequisite notice, adding a jq-free pre-check:
markdown-format.sh#L112-L121

if ! command -v jq >/dev/null 2>&1; then
  DECODED_FILE="${RAW_FILE//\\\"/\"}"
  DECODED_FILE="${DECODED_FILE//\\\//\/}"
  DECODED_FILE="${DECODED_FILE//\\\\/\\}"
  if [[ -f "$DECODED_FILE" ]] &&
    ! markdownlint_config_discoverable "$DECODED_FILE" \
      "$(hook::repo_root "$(dirname "$DECODED_FILE")")"; then
    exit 0
  fi
fi
hook::require_jq PostToolUse markdown-format "$INPUT"

Traced this for injection/bypass risk:

• No shell/path injection. $RAW_FILE comes from hook::raw_file_path's regex extraction of the hook payload's file_path (not eval'd), and every use of $DECODED_FILE here (-f test, dirname, hook::repo_root → git -C "$hint" ...) is quoted and passed as a discrete argument, never interpolated into a command string.
• No gate bypass. This block only decides whether to suppress the "jq missing" notice — it does not perform any formatting or write. When jq is genuinely absent, hook::require_jq unconditionally exit 0s immediately afterward regardless of what this pre-check decided, so no code path reaches --fix without jq. The authoritative opt-in gate remains the one further down at markdown-format.sh#L234-L238, unchanged by this commit.
• Fails safe on bad decode. If the JSON-unescape produces a name nothing on disk answers to, -f "$DECODED_FILE" is false and the block falls through to the normal notice path — it never fails open into skipping a gate that should apply.
• Membership checks are for notice-suppression only. Unlike the authoritative path, this pre-check doesn't run in_git_working_tree / symlink-physical-path scoping or clear GIT_DIR/GIT_WORK_TREE before hook::repo_root's git -C call. That's a real asymmetry, but the only thing it can influence is whether a "jq not found" message is printed — it can't cause a write, a lint run, or disclosure of file contents outside the repo. Not a security-relevant gap.

Rest of the diff (re-confirmed, consistent with the prior two reviews on this PR):

• typos-format.sh's write-gate flip (WRITE_CHANGES == "true" opt-in, default false in both the script fallback and plugin.json) fails closed on any unset/malformed value — correct direction for a mutating default.
• markdown-format.sh's markdownlint_config_discoverable root/dir resolution still fails closed (return 1) when cd/pwd -P can't resolve, so an unresolvable root can't degrade into an unbounded upward walk.
• No userConfig values are shell-interpolated (${user_config.*}) anywhere touched by this PR — both hooks continue reading options via the CLAUDE_PLUGIN_OPTION_<KEY> env mirror.
• Manifest diffs (plugin.json for both plugins) only tighten defaults / narrow when a hook fires; no permission widening, no new secrets/token handling.
• No GitHub Actions workflow files are touched by this PR, so the pull_request_target/workflow_run/unpinned-action/permissions lane doesn't apply here.

No CRITICAL, IMPORTANT, or SUGGESTION-level security findings to report. (Note: the two Codex P2 comments on this PR concern setup-skill diagnostic staleness — correctness/UX, not security — and are out of scope for this lane.)

[claude]

Claude finished @kyle-sexton's task in 1m 41s —— View job

---

Code review — PR #1859

Todo list

• Gather context (git diff origin/main...HEAD, identify what's new since the last review at commit 201f438)
• Review the newest commit (10e80d9) on its own merits
• Confirm no regression in previously-reviewed commits
• Post findings

Summary

The new commit 10e80d9 is the only change since the last code-review pass (which covered 201f438). It's a 4-line, comment-only addition to plugins/markdown-format/hooks/markdown-format.sh — no behavior change. No blocking issues.

1. Correct and precisely targeted at the gate it satisfies. markdown-format.sh#L120-L124 adds a # silent-skip-ok: <reason> annotation immediately above the jq-free pre-check's exit 0. I read scripts/check-silent-skips.sh directly: its awk state machine (lines 66–78, 80–94) treats an if ! command -v jq ...; then ... fi block reaching exit 0 as a silent-skip violation unless the block carries a sanctioned visibility call or an annotation comment matching #[[:space:]]*silent-skip-ok: on the guard line, in the comment block immediately above it, or inside the block. The new comment sits inside the block, directly above the exit 0 at line 124 — exactly the sanctioned position (block_annotated is set per-line inside in_block), so this correctly resolves what would otherwise be a false-positive flag from that gate on the pre-check introduced in 201f438.
2. The annotation's own reasoning is sound, not just gate-satisfying boilerplate. The comment correctly distinguishes what verdict the exit 0 actually reports (the opt-in's, i.e. "this repo never enabled the hook") from what the gate is designed to catch (a missing-CLI skip that owes the user a notice). It also correctly notes the escape hatch is preserved: jq's own absence still surfaces via hook::require_jq on line 127 for any repo that did opt in — so this doesn't create a second, undocumented silent-skip path.
3. No CHANGELOG update needed. This is a pure internal-gate annotation with zero behavioral change (nothing here alters what a user observes), and the [0.9.0] entry already documents the "no config → no run, no notice" contract this annotation is just making legible to the repo's own tooling. Consistent with how the repo scopes CHANGELOG entries to user-observable behavior.
4. Style/formatting consistent with the rest of the file — indentation matches the surrounding block, and the comment style matches the existing block comment above the same if at lines 97–112.

I was unable to execute scripts/check-silent-skips.sh or the hook test suites directly in this sandboxed environment (Bash required interactive approval I couldn't obtain), so I verified the fix by hand-tracing the gate script's awk logic against the new annotation's exact placement rather than running it. Static review gives me high confidence this passes, given how directly the annotation lines up with the gate's documented sanctioning rule.

Everything from the prior reviews on this PR (the fail-closed REPO_ROOT resolution, the write-gate flip, the config opt-in gate, the notice-ordering fix, the hook-budget doc) still holds — this commit only touches the silent-skip annotation and doesn't regress any of it.

[claude]

Claude finished @kyle-sexton's task in 58s —— View job

---

Security Review — PR #1859

Scope: full diff origin/main...HEAD (14 files), with a focused re-check of what changed since the prior security review on 201f438 — this PR's fourth commit, 10e80d9 ("record the opt-in exit as a quiet classification").

Result: No security issues found

What's new since the last review — 10e80d9 touches only markdown-format.sh#L120-L123, adding a four-line silent-skip-ok: justification comment above the pre-existing exit 0 in the jq-free opt-in pre-check. It is a comment-only change (presumably to satisfy the repo's check-silent-skips gate) — no control flow, quoting, or gating logic differs from what the previous two reviews already cleared on 201f438.

Re-confirmed on the full diff (nothing new to add beyond the last two passes on this PR):

• typos-format.sh#L200-L211 — write-gate flip to WRITE_CHANGES == "true" fails closed on any unset/malformed value; script fallback (false) and plugin.json default agree.
• markdown-format.sh#L59-L131 — markdownlint_config_discoverable's dir/root resolution still fails closed (return 1) on unresolvable paths; the jq-free pre-check only suppresses a notice, never bypasses the authoritative opt-in gate at L235-L242, which is unchanged; no shell/path injection in the backslash-unescape or the -f/dirname/hook::repo_root uses of $DECODED_FILE (all quoted, passed as discrete args, never interpolated into a command string).
• Both plugin.json manifests (markdown-format, typos-format) only tighten defaults / narrow when a hook fires; no new userConfig keys, no permission widening.
• No userConfig value is shell-interpolated (${user_config.*}) anywhere in this diff — both hooks read options via the CLAUDE_PLUGIN_OPTION_<KEY> env mirror.
• No GitHub Actions workflow files are touched by this PR, so the unpinned-action/permissions/pull_request_target lane doesn't apply.

No CRITICAL, IMPORTANT, or SUGGESTION-level security findings to report.