Skip to content

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

Merged
kyle-sexton merged 1 commit into
mainfrom
fix/884-typos-format-remove-opt-in-gate
Jul 21, 2026
Merged

fix(typos-format): remove opt-in config-gate, run unconditionally#900
kyle-sexton merged 1 commit into
mainfrom
fix/884-typos-format-remove-opt-in-gate

Conversation

@kyle-sexton

Copy link
Copy Markdown
Contributor

Summary

typos-format's hook only ran typos --write-changes when the consuming repo already had a typos.toml/_typos.toml/.typos.toml/Cargo.toml ([*.metadata.typos])/pyproject.toml ([tool.typos]) file present. typos ships a built-in spelling dictionary and runs standalone with zero configuration — a repo config only widens the allowlist/exclude list, it is not an activation switch. The gate made the hook a silent no-op on exactly the zero-config repos it was meant to help, defeating the plugin's purpose.

Fix

Removed the opt-in config-gate (and its ancestor-walk grep logic) entirely. typos --write-changes --force-exclude now runs unconditionally on every edit, matching markdown-format's existing unconditional pattern. typos's own file-anchored config discovery still applies automatically when a config IS present (allowlist/exclude, precedence order) — the hook never re-implemented that discovery and still doesn't; only the activation gate is gone.

Also updated: plugin manifest description/version, README (top-level + plugin), CHANGELOG, the setup skill's check step (config presence is now reported as informational only, never a gate), and the test suite (the old "gate OFF -> file left untouched" case is replaced with "runs unconditionally, fixes a real typo with no config present anywhere").

Verification

Empirical before/after on an isolated zero-config git repo (this document has a recieve typo, no typos.toml/_typos.toml/.typos.toml/Cargo.toml/pyproject.toml anywhere in the ancestor chain):

  • Before (origin/main's gated hook): file left untouched — recieve unfixed, hook exits silently.
  • After (this branch's unconditional hook): file rewritten in place — recievereceive.

Full test suite: bash plugins/typos-format/hooks/typos-format.test.sh41/41 passing, including the new unconditional-fix case, kill-switch, residual-findings, exclude, and telemetry cases.

Closes #884

Related

Co-Authored-By: Claude Sonnet 5 noreply@anthropic.com
Claude-Session: https://claude.ai/code/session_01K1V3gkrfSf75isB8MiDy3o

typos ships a built-in spelling dictionary and needs no configuration to be
useful, so gating the hook on a consumer typos config existing made it a
silent no-op on exactly the zero-config repos it was meant to help. Run
`typos --write-changes` unconditionally on every edit, matching
`markdown-format`'s existing unconditional pattern. A consumer typos config,
when present, is still discovered and honored automatically by typos itself.

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 43s —— View job


Security Review — PR #900

  • Gather PR diff and changed files
  • Read hook script (typos-format.sh)
  • Read test script (typos-format.test.sh)
  • Read plugin manifest, README, CHANGELOG, setup skill
  • Analyze for injection surfaces, unsafe shell patterns, logic flaws
  • Report findings

No security issues found.

Reviewed all 7 changed files against: command/path/JSON injection, unsafe shell expansion, secrets/credential handling, GitHub Actions patterns, permission widening, and supply-chain risk.

What was examined

typos-format.sh (hook, the highest-risk surface)

  • $TYPOS_BIN — resolved via command -v typos (PATH-only), never from user input. Properly double-quoted at the invocation site ("$TYPOS_BIN", line 166). No change from prior version.
  • $TYPOS_ARG / $RUN_DIR — both double-quoted at invocation. $RUN_DIR's fallback changed from a pwd-canonicalized $FILE_DIR_POSIX to $(dirname "$FILE") (line 152); this is a cosmetic regression (non-canonical path on repo-root-resolve failure) but carries no exploitable consequence — cd accepts non-canonical paths, and $TYPOS_ARG is still the original $FILE, so the file operated on is unchanged.
  • typos JSON output parsing$typo / $first_correction are extracted with jq -r and passed to hook::ctx_append as argument strings; bash does not re-expand command substitutions inside a variable's value, so no secondary injection. Structured findings passed back to jq use --arg/--argjson (lines 193–194), which escape properly.
  • 2>&1 merge in typos invocation — stderr flows through the JSON-parsing loop when RC == 2; non-JSON lines are silently discarded by jq -c 'select(.type == "typo")'. Pre-existing pattern, unchanged by this PR.
  • Removed config-walk grep logic — the deleted grep -qE calls on Cargo.toml/pyproject.toml were read-only filesystem checks with no injection surface; their removal is neutral from a security standpoint.

typos-format.test.sh (tests)

  • make_sink writes caller-supplied strings verbatim into generated shell scripts. Callers pass "cat >\"$TEL\"" where $TEL is a mktemp-generated path — no untrusted input. Not a vulnerability, and confined to the test harness.
  • run_hook/run_hook_env construct JSON via printf '...%s...' "$file_path" without JSON-escaping the path. A path with embedded " or \ would produce malformed JSON. All test paths come from mktemp so this never triggers in practice. A latent test-harness issue, not a hook vulnerability.

Remaining files (plugin.json, README.md, CHANGELOG.md, setup/SKILL.md) — documentation and metadata only; no executable surface.

Behavioral note (not a vulnerability)

The hook now runs unconditionally on every file write in every repo. This is the intended change. The CLAUDE_PLUGIN_OPTION_TYPOS_FORMAT_ENABLED=false kill-switch remains intact (line 55) for consumers who want to opt out.

@claude

claude Bot commented Jul 21, 2026

Copy link
Copy Markdown

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


PR Review: fix(typos-format): remove opt-in config-gate, run unconditionally

  • Gather context and read changed files
  • Review hook script changes
  • Review test suite changes
  • Review manifest, README, and CHANGELOG
  • Review setup skill changes
  • Synthesize findings and post review

Summary

The core change is correct and well-motivated. typos genuinely ships a built-in dictionary and needs zero configuration, so the old opt-in gate was silently defeating the plugin's purpose on the repos it was most intended to help. The removal is clean (~34 lines gone), the behavioral fallback for config-governed repos is unchanged (typos still discovers and honors any config via its own file-anchored resolution), and --force-exclude continues to respect config-based exclusion lists.


Findings

1. Stale "Case 3" cross-reference in test header — MINOR

The test file's opening comment (line 14) says "Case 3 below locks this in" to document that typos resolves config relative to the target path, not the process CWD. After renumbering, that test is now Case 5 ("config nested BELOW repo root"). The test itself is correct; the comment cross-reference is stale.

typos-format.test.sh line 14

-# target path passed on the command line, not the process CWD — Case 3 below
+# target path passed on the command line, not the process CWD — Case 5 below

Fix this →


2. KNOWN RISK comment overstates the triggering condition — MINOR

typos-format.sh line 38

The comment reads "on a repo with both a typos config and (e.g.) a Ruff config". Since the hook now runs unconditionally, the concurrent-write race with sibling formatter hooks applies regardless of whether the repo has a typos config — the co-condition is just "both plugins are installed and the file is edited". The risk itself is correctly described; only the triggering condition language is slightly stale.


What's correct and well-done

  • RUN_DIR fallback (${root:-$(dirname "$FILE")}) is a clean replacement for the old FILE_DIR_POSIX variable (which existed solely to serve the now-deleted config walk and as this same fallback). The path $(dirname "$FILE") is sufficient here — typos resolves config from the target path regardless of CWD.
  • --force-exclude is kept unconditionally. Without a config this flag is a no-op, but its presence ensures that repos with a config that excludes certain paths (e.g. gen/) still see those files skipped — the exclusion test (Case 7) validates this correctly.
  • Telemetry test correctly updated: the no-config path now expects "ok" (typos ran and fixed the file) rather than "skipped", matching the hook's status semantics.
  • Missing-tool notice correctly updated to drop the stale "a typos config governs this repo but…" preamble — the binary check is now unconditional context.
  • Cases 1b/1c removed cleanly: pyproject.toml opt-in tests are now irrelevant since the gate is gone, and the precedence test (Case 2) correctly pivots to proving that typos' own resolution still applies automatically.
  • Version bump (0.1.0 → 0.2.0) is appropriate — this is a visible behavioral change for any consumer who deliberately had no typos config and expected the hook to stay silent.
  • CHANGELOG, plugin README, top-level README, and SKILL.md are all updated consistently.

Two minor fixes needed (one stale comment cross-reference, one comment-only nit). No correctness, security, or functional issues found.

@kyle-sexton

Copy link
Copy Markdown
Contributor Author

Post-green review classification (work lane):

Finding Source Classification Disposition
Everything substantive PR review + security review No correctness/security/functional issues found No action needed
Stale "Case 3" cross-reference in test header comment (should say "Case 5" after renumbering) PR review (minor) Non-blocking, comment-only Left alone
KNOWN RISK comment slightly overstates the triggering condition post-gate-removal PR review (minor) Non-blocking, comment-only, risk itself correctly described Left alone
RUN_DIR fallback changed from pwd-canonicalized to $(dirname "$FILE") Security review Cosmetic, no exploitable consequence — cd accepts non-canonical paths, target file unchanged No action needed
Test-harness JSON construction doesn't escape embedded quotes in paths Security review Latent test-harness-only issue, never triggers in practice (all paths are mktemp-generated) No action needed

No DEFER-FORBIDDEN class applies. No inline review threads to resolve (PR-level comments only). CLAUDE_PLUGIN_OPTION_TYPOS_FORMAT_ENABLED=false kill-switch confirmed intact for consumers who want to opt out entirely.

Duplicate PR #901 (an independent parallel implementation of the same fix) closed as superseded — this one landed first with a clean review and zero pending checks.

Flipping to ready, handing off to the babysit lane for ongoing watch.

@claude

claude Bot commented Jul 21, 2026

Copy link
Copy Markdown

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


PR Review: fix(typos-format): remove opt-in config-gate, run unconditionally

  • Read PR context and prior review comments
  • Get PR diff and read changed files
  • Review hook script (typos-format.sh)
  • Review test suite (typos-format.test.sh)
  • Review plugin manifest, README, CHANGELOG, setup skill
  • Check CLAUDE.md alignment
  • Post findings

Summary

The fix is correct and well-motivated. typos ships a built-in dictionary and genuinely needs zero consumer configuration to be useful — the old gate was silently defeating the plugin on exactly the repos it was intended to help. The removal is clean (~34 lines gone, no new logic added), the implementation matches the stated design, and all 7 changed files are updated consistently.


Hook Script — typos-format.sh

Core change (lines 123–140): The FILE_DIR_POSIX variable and the full config-walk block are removed cleanly. The only mechanical consequence is the RUN_DIR fallback change:

-RUN_DIR="${root:-$FILE_DIR_POSIX}"
+RUN_DIR="${root:-$(dirname "$FILE")}"

FILE_DIR_POSIX was a cd ... && pwd canonicalized path; $(dirname "$FILE") is simpler but non-canonical. This is an acceptable regression: cd "$RUN_DIR" accepts non-canonical paths, and TYPOS_ARG remains $FILE/$FILE_REL regardless — so the file actually operated on is unchanged. The root variable (still cd "$REPO_ROOT" && pwd, line 126) is used as the primary RUN_DIR and the fallback only fires when REPO_ROOT resolution fails entirely.

Kill switch (hook::check_enabled "TYPOS_FORMAT", line 55): confirmed intact.

Missing-binary notice (line 137): updated to drop the stale "a typos config governs this repo but…" preamble — now reads cleanly for the unconditional flow.

KNOWN RISK comment (line 37): still reads "on a repo with both a typos config and (e.g.) a Ruff config" — the concurrent-write race now applies regardless of whether a typos config exists, so the triggering condition is understated. The risk itself is correctly described and already flagged in the prior review as non-blocking. No action required, just noting for awareness.

Edge-case coverage: Files outside a git repo fall back to RUN_DIR = dirname "$FILE", which lets typos discover any ancestor config from the file upward — correct. All exit-code paths (RC=0, RC=2, other) are unchanged.


Test Suite — typos-format.test.sh

Case 1 (the critical new case, lines 116–130): inverted from "gate OFF → file left untouched" to "no config anywhere → hook still fixes unconditionally". Asserts both exit code and file content — correct.

Cases 1b/1c removed: The pyproject.toml opt-in cases are now irrelevant. Their removal is clean.

Renumbering (1d→2, 2→3, 3→4, 3b→5, 4→6, 4b→6b, 5→7, 6→8): All case labels and their cross-references within each case's own comment are updated correctly.

Test header cross-reference (line 14): the comment reads "Case 3 below locks this in" to prove typos resolves config from the target path, not process CWD. Case 3 is "config present + clean file → exit 0"; Case 5 ("config nested BELOW repo root") is the one that actually proves that invariant. The prior review identified this as stale (should say "Case 5 below") and the author left it as non-blocking.

Fix this →

Telemetry / no-config case (lines 291–303): asserts status: ok (not skipped) and findings: [] for a no-config repo where the typo is fully fixed. Correctly inverted from the old "status: skipped" gate-off assertion.

Missing-tool jq check (line 333): updated from contains("typos config") to contains("PATH"), matching the updated notice text. Correct.


Manifest, Docs, Setup Skill

File Change Assessment
plugin.json Version 0.1.0 → 0.2.0; description updated ✓ Appropriate — visible behavioral change
CHANGELOG.md 0.2.0 entry added ✓ Accurate and well-written
plugins/typos-format/README.md Behavior + Requirements rewritten ✓ Consistent with new behavior; config requirement correctly removed from Requirements
README.md (top-level) Plugin description updated ✓ One-liner consistent with others
skills/setup/SKILL.md Step 3 FAIL condition simplified; Step 4 changed to INFO-only ✓ Config check correctly demoted from gate to informational

CLAUDE.md Alignment

Requirement Status
Repo-agnostic (no hardcoded paths)
Configurable via userConfig typos_format_enabled kill-switch intact
Plugin-form-safe (no ../ reach-outs)
No PII / secrets
Versioned with explicit semver ✓ 0.2.0
Security-reviewed ✓ Prior review in this PR: no issues found

Verdict

Approve. The behavioral fix is correct, the implementation is minimal and clean, and the documentation and test suite are updated consistently across all 7 files. Two minor comment-only issues from prior reviews (stale "Case 3" cross-reference, KNOWN RISK trigger wording) are non-blocking and were deliberately left by the author. No correctness, security, or functional issues.

@claude

claude Bot commented Jul 21, 2026

Copy link
Copy Markdown

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


Security Review — PR #900

  • Gather PR diff and changed files
  • Read hook script (typos-format.sh)
  • Read test script (typos-format.test.sh)
  • Read plugin manifest, README, CHANGELOG, setup skill
  • Analyze for injection surfaces, unsafe shell patterns, logic flaws
  • Report findings

No security issues found.

All 7 changed files reviewed against: command/path/JSON injection, unsafe shell expansion, secrets/credential handling, permission widening, supply-chain risk, and dangerous GitHub Actions patterns.

Hook script: typos-format.sh

This is the highest-risk surface. Line references point to the current commit.

$TYPOS_BIN (line 131) — resolved via command -v typos (PATH-only, never from user input). Properly double-quoted at the invocation site (line 166). Unchanged by this PR.

$RUN_DIR fallback change (line 152) — old code used $FILE_DIR_POSIX (a pwd-canonicalized form); new code inlines $(dirname "$FILE") (non-canonicalized). When this fallback fires, $root is empty (repo root resolution failed) and $TYPOS_ARG is the absolute $FILE path regardless — the CWD has no effect on which file is operated on. Double-quoted at cd "$RUN_DIR". No security consequence.

jq parsing of typos output (lines 182–194) — $typo and $first_correction are extracted with jq -r and appended as strings; bash does not re-expand command substitutions inside variable values, so no secondary injection. Structured findings are passed back to jq via --arg/--argjson (lines 193–194), which escape properly. Pre-existing pattern, unchanged.

Deleted config-walk grep logic — the removed grep -qE calls on Cargo.toml/pyproject.toml were read-only filesystem checks. Their removal is security-neutral and eliminates approximately 20 lines of dead-code attack surface.

Test script: typos-format.test.sh

make_sink (line 55) — writes caller-supplied strings verbatim into generated shell scripts. All callers pass constant trusted command strings (cat >"$TEL"). Not exploitable.

JSON construction in run_hook/run_hook_env (lines 100, 111) — $file_path is interpolated into JSON via printf '...%s...' without JSON-escaping. A path with embedded " or \ would produce malformed JSON. All test paths come from mktemp, so this can never trigger in practice. A latent test-harness issue only, not a hook vulnerability.

Remaining files

plugin.json, README.md, CHANGELOG.md, SKILL.md — documentation and metadata only. No executable surface.

Behavioral note (not a vulnerability)

The hook now runs unconditionally on every file write in every repo. This is the intended change. The CLAUDE_PLUGIN_OPTION_TYPOS_FORMAT_ENABLED=false kill-switch remains intact (line 55) for consumers who want to opt out.

@kyle-sexton
kyle-sexton merged commit 78c0c99 into main Jul 21, 2026
26 of 27 checks passed
@kyle-sexton
kyle-sexton deleted the fix/884-typos-format-remove-opt-in-gate branch July 21, 2026 23:13
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