Skip to content

fix(typos-format): remove opt-in config-gate, run typos unconditionally - #901

Closed
kyle-sexton wants to merge 1 commit into
mainfrom
fix/884-typos-format-remove-opt-in-gate-v2
Closed

fix(typos-format): remove opt-in config-gate, run typos unconditionally#901
kyle-sexton wants to merge 1 commit into
mainfrom
fix/884-typos-format-remove-opt-in-gate-v2

Conversation

@kyle-sexton

Copy link
Copy Markdown
Contributor

Summary

typos-format's hook gated its entire spell-fix behavior behind a pre-existing consumer typos config (typos.toml/_typos.toml/.typos.toml/Cargo.toml/pyproject.toml), mirroring the ruff-format/biome-format opt-in pattern. That pattern is wrong for typos specifically: it ships a built-in spelling dictionary and runs with zero configuration, so the gate made the hook a silent no-op on any repo without a hand-authored typos config — i.e. most repos on day one, defeating the plugin's zero-config auto-fix purpose.

Fix

Removed the opt-in config-gate entirely. typos --write-changes now runs unconditionally on every edited file (no extension filter, same as before), matching markdown-format's existing unconditional pattern instead of ruff-format's config-gated one. This is a removal of an activation gate only — typos's own file-anchored config discovery still applies automatically when a config is present, to widen its allowlist (extend-words/extend-identifiers/extend-ignore-re) and excludes ([files] extend-exclude); the hook no longer implements its own ancestor config walk-up at all, since it was only ever used to decide whether to invoke typos, never to pass the config to it.

Updated:

  • plugins/typos-format/hooks/typos-format.sh — removed the CONFIG_FOUND walk-up loop and its gate check; reworded the missing-binary skip notice (no longer references "a typos config governs this repo"); rewrote the header comment block to describe unconditional behavior.
  • plugins/typos-format/hooks/typos-format.test.sh — replaced "opt-in gate OFF -> file left untouched" with a test proving the hook fixes a typo with no config present anywhere; reframed the config-precedence test as proving typos' own native discovery (not the hook's); flipped the "gate OFF -> telemetry status skipped" case to "no-config -> status ok"; updated the missing-binary test's message assertion.
  • plugins/typos-format/README.md, plugins/typos-format/skills/setup/SKILL.md — updated to describe unconditional behavior; config presence in setup check is now informational only (INFO), never a FAIL/PASS gate.
  • plugins/typos-format/.claude-plugin/plugin.json — bumped 0.1.00.2.0 (minor, not patch): this changes runtime behavior for existing installs — a repo with no typos config goes from inert to actively rewriting files on edit, a surprise-on-upgrade consumers should see flagged in the version.
  • plugins/typos-format/CHANGELOG.md — new [0.2.0] entry.

Verification

All 43 hook contract tests pass (bash plugins/typos-format/hooks/typos-format.test.sh) against a real typos-cli 1.44.0 binary.

Empirical before/after against a zero-config repo (no _typos.toml anywhere), same input this has teh typo:

  • Before (old gated hook, checked out at the parent commit): file left untouched — this has teh typo.
  • After (this PR's hook): file fixed in place — this has the typo.

Closes #884

Related

The hook previously walked ancestor directories looking for a governing
typos config and skipped entirely when none was found, mirroring the
ruff-format/biome-format config-gate pattern. That pattern is wrong for
typos: it ships a built-in spelling dictionary and runs with zero
configuration, so gating activation on a hand-authored config made the
hook a silent no-op on most repos day one, defeating its zero-config
auto-fix purpose.

typos --write-changes now runs unconditionally on every edited file,
matching markdown-format's existing unconditional pattern. typos' own
config discovery is untouched — a governing config, when present, is
still found and honored by typos itself to widen the allowlist/excludes.

Closes #884

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01K1V3gkrfSf75isB8MiDy3o
@kyle-sexton kyle-sexton added the automated Opened by automation. label Jul 21, 2026
@claude

claude Bot commented Jul 21, 2026

Copy link
Copy Markdown

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


PR Review

  • Gather context
  • Read changed files
  • Analyze hook script logic
  • Analyze test coverage
  • Analyze docs/README changes
  • Post review findings

Summary: The change is correct and well-scoped. Removing the 32-line CONFIG_FOUND walk-up loop and its single gate check is the right fix — the walk-up was pure activation logic that the hook implemented itself; typos already handles config discovery natively, and the tool's zero-config built-in dictionary makes the gate actively harmful on unconfigured repos. The diff is clean, tests are comprehensive, documentation is accurate, and the minor version bump is justified.


Hook script (typos-format.sh)

No correctness issues. The removal is surgical: CONFIG_FOUND was only ever consumed by the gate, never passed to typos itself, so deleting the walk-up loop + [[ -n "$CONFIG_FOUND" ]] || emit_skipped leaves all downstream logic intact. The emit_skipped function now only fires on the missing-binary path, which is correct.

The skip notice wording change is accurate:

  • Before: "a typos config governs this repo but no 'typos' binary was found on PATH"
  • After: "no 'typos' binary was found on PATH"

The "governs this repo" qualifier no longer applies since activation is unconditional.

--force-exclude with no config is a no-op, not a hazard — typos simply has no excludes to honor, so it runs against its built-in dictionary. No regression there.


Tests (typos-format.test.sh)

Tests are thorough and well-structured. A few observations:

Missing-binary test still uses a config-bearing repoREPO_NT gets a _typos.toml written to it (line 333), even though the PR text says the notice fires "regardless of whether a typos config exists." The logic is provably config-independent (binary check precedes any config logic, and CONFIG_FOUND is gone), so it's low risk, but a no-config variant of this case would close the gap completely if thoroughness is desired.

Case 1d (config precedence) tests typos itself, not the hook — the case is correctly labeled as "typos' NATIVE config discovery (Config::from_dir), not anything the hook implements," so this is a deliberate regression guard. The assertion relies on fooone being absent from typos' built-in dictionary (so it's left alone when _typos.toml is suppressed by typos.toml). fooone is a sufficiently synthetic word that this should be stable, but it could theoretically collide with a future dictionary entry. No action required; just worth flagging.

Case 1c (pyproject.toml with [tool.typos]) — confirms that a [tool.typos] section doesn't accidentally block the unconditional run. Good coverage.

Telemetry / no-config test at line 303–315 is the correct flip: previously the gate caused emit_skipped (status "skipped"); now the hook runs and exits cleanly (status "ok"). The assertion jq -r '.status' == "ok" confirms the new path end-to-end.


SKILL.md (setup check probe 4)

Minor doc inconsistency. Probe 4 includes the parenthetical:

(read the hook for the exact names and the section test — its test is the authority)

The config file names are still present in the hook's header comment (lines 16–21 of typos-format.sh), so reading the hook still surfaces them. However, the "section test" — the grep -qE '^[[:space:]]*\[(workspace|package)\.metadata\.typos regex — no longer exists in the hook (it was part of the removed walk-up loop). A model following this instruction would read the hook comment and find the names but not a "section test."

Since probe 4 is now informational-only (not a FAIL gate), this is low priority, but the parenthetical could be updated to reflect that the hook's header comment is the reference, not a grep-pattern test that no longer exists.

Fix this →


Version bump

0.1.0 → 0.2.0 (minor) is correct. Per semver and the repo's own convention, a behavior change that actively rewrites files for existing installs that previously got a silent no-op is a consumer-visible surprise. The CHANGELOG entry is clear about the blast radius.


Verdict

Approve. The core logic change is correct, the test coverage is solid, and the documentation accurately reflects the new behavior. The SKILL.md parenthetical is a polish nit, not a blocker.

@claude

claude Bot commented Jul 21, 2026

Copy link
Copy Markdown

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


Security Review — PR #901

  • Gather PR diff and changed files
  • Audit typos-format.sh hook for injection/unsafe shell patterns
  • Audit typos-format.test.sh for unsafe test patterns
  • Audit plugin.json, README.md, SKILL.md, CHANGELOG.md
  • Post findings

Findings

No CRITICAL or IMPORTANT security issues found.

Reviewed surfaces: command injection, path traversal, unsafe variable handling, JSON injection, supply-chain risk, secrets/credential exposure, and dangerous sink execution patterns. Details below.


Hook invocation — typos-format.sh line 169

OUTPUT=$(cd "$RUN_DIR" && "$TYPOS_BIN" --write-changes --force-exclude --format json "$TYPOS_ARG" 2>&1)

$TYPOS_BIN comes from command -v typos (PATH resolution only — no download). $TYPOS_ARG is always double-quoted, preventing word-splitting. It resolves to either the absolute $FILE (always starts with / on POSIX, drive letter on Windows — never --) or a relative path produced by stripping the common repo-root prefix from an absolute path (also can never start with --). No -- separator is needed here in practice. $RUN_DIR is canonicalized through cd … && pwd before use. No command injection surface.

File path input — hook::read_file_path

The path comes from Claude Code's JSON envelope, parsed via jq -r '(.tool_input.file_path // empty)' (proper jq flag injection prevention), then validated with [[ -f "$file" ]] (must exist) and optionally bounded to CLAUDE_PROJECT_DIR via a symlink-resolved prefix check. No untrusted path ever reaches a shell eval or unquoted expansion. No path traversal.

Output from typos fed back into context

while IFS= read -r line; do
  [[ -n "$line" ]] || continue
  hook::ctx_append "  $line"
done <<<"$OUTPUT"

$OUTPUT (typos stdout+stderr) is appended to _HOOK_CTX_BUFFER, which is then flushed via hook::emit_additional_context using jq -n --arg ctx "$ctx". The --arg flag means jq handles all JSON escaping — a typos output line containing "},"systemMessage":"injected" is inert. No JSON injection.

Telemetry sink execution — hook-utils.sh line 455

printf '%s\n' "$envelope" | ("$sink" >/dev/null 2>&1) &

$HOOK_TELEMETRY_SINK is an executable path configured by the consumer. Execution is intentional by design; stdout/stderr are both redirected to /dev/null so the sink cannot write to the hook's additionalContext channel (fd1). The sink path is resolved relative to $CLAUDE_PROJECT_DIR when not absolute (not user-controlled per-edit input). By design; no new risk introduced by this PR.

Supply-chain

$TYPOS_BIN is resolved from PATH only and never downloaded at hook time. No new remote sources added. plugin.json introduces no new MCP servers or remote dependencies. No supply-chain risk.

Behavioral security note (informational)

The primary change — removing the config gate — means typos --write-changes now runs unconditionally on every edited file in every repository where the plugin is installed. --force-exclude is passed, so typos' own [files] extend-exclude config is honored. The consequence: repos with intentional-misspelling fixtures (e.g. test vectors for spell-checker unit tests) that lack a _typos.toml extend-words entry will have those fixtures silently rewritten. This is a behavioral surprise, not a security vulnerability. It is adequately addressed by the version bump to 0.2.0 (consumers see the version change on upgrade) and the CHANGELOG entry.


Summary: The hook code is well-structured — all subprocess calls use properly-quoted variables, JSON construction uses jq --arg throughout, file path input is validated before use, and the binary is PATH-resolved only. The gate removal does not introduce any new injection, traversal, or credential-exposure surfaces.

@kyle-sexton

Copy link
Copy Markdown
Contributor Author

Closing as a duplicate of #900 — both PRs independently implemented the same fix for #884 (a genuine race: two workers ended up on the same task). #900 landed first with a fully clean review pass (2 minor non-blocking nits, no correctness/security issues) and zero pending checks. Not merging this one.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

automated Opened by automation.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

typos-format: opt-in config-gate makes the hook a silent no-op on zero-config repos, defeating the plugin's own auto-fix purpose

1 participant