Skip to content

feat(typos-format): add per-file typos autofix hook plugin - #872

Merged
kyle-sexton merged 5 commits into
mainfrom
feat/831-typos-hook-plugin
Jul 21, 2026
Merged

feat(typos-format): add per-file typos autofix hook plugin#872
kyle-sexton merged 5 commits into
mainfrom
feat/831-typos-hook-plugin

Conversation

@kyle-sexton

@kyle-sexton kyle-sexton commented Jul 21, 2026

Copy link
Copy Markdown
Contributor

Summary

New hook plugin plugins/typos-format/: runs typos --write-changes on every Write/Edit, gated on a consumer typos config (typos.toml/_typos.toml/.typos.toml/Cargo.toml/pyproject.toml) found by an ancestor walk-up, mirroring the shipped ruff-format/markdown-format plugin pattern. Residual (unfixable) findings surface via additionalContext with remediation guidance pointing at extend-words/extend-identifiers/extend-ignore-re allowlist entries. Advisory only — never blocks the edit.

  • Hook-precision + hook-telemetry convention conformance (schema + registry row added).
  • /typos-format:setup check|apply skill.
  • 44 black-box contract-test assertions (typos-format.test.sh), including a --force-exclude regression case added after independent review caught the flag was missing (verified empirically: without it, the hook silently overrode a consumer's own typos excludes).

Test plan

  • typos-format.test.sh — 44/44 pass against real typos-cli 1.44.0.
  • Full fleet scripts/run-plugin-tests.sh — all plugin contract tests pass.
  • scripts/validate-plugins.sh (manifest schema, catalog sync, no-npx contract).
  • scripts/sync-hook-utils.sh --check, check-silent-skips.sh, check-changelog-parity.sh --check, check-cross-plugin-source-drift.sh --check, check-skill-leaf-names.sh --check, check-changed-skills.sh origin/main.
  • typos/shellcheck/markdownlint-cli2 self-check on all new files.
  • Two independent fresh-context reviews (code review + security review via CI): one CRITICAL finding (missing --force-exclude) fixed + regression-tested; two SUGGESTION-level security notes triaged (one deferred as a fleet-wide pre-existing test-harness pattern, one out-of-scope for the shared hook-utils.sh library); two minor code-review notes fixed (test assertion strength, marketplace relevance block).

Review follow-ups filed

Related

Implementation plan (docs/topics/typos-format-hook/PLAN.md, pruned from the tree after merge)

typos-format hook plugin

Brief

Issue #831 (epic #830, sub-item 1 of
docs/topics/lint-static-analysis-gaps/PLAN.md, merged via PR #829). New hook
plugin plugins/typos-format/: per-file typos -w autofix on Write/Edit,
mirroring the existing plugins/markdown-format/ and plugins/ruff-format/
patterns. Opt-in via consumer typos config ancestor walk-up (no config = no-op,
never impose typos' defaults on a repo that hasn't adopted it — same posture as
ruff-format). Advisory only, hook-precision + hook-telemetry conventions,
setup skill (check/apply).

Scope: Tier C (no design) — pure pattern replication of two already-shipped,
gate-passing plugins; no new types, no new architecture.

Plan

Phase 1: Hook plugin skeleton

Files (new): .claude-plugin/plugin.json, hooks/hooks.json,
hooks/hook-utils.sh (synced copy of lib/hook-utils.sh),
hooks/typos-format.sh, .claude-plugin/marketplace.json entry.

Hook control flow (modeled on ruff-format.sh): kill-switch check, buffered
stdin read, jq gate, file-path extraction, repo-root resolution, ancestor
config walk-up (typos.toml > _typos.toml > .typos.toml > Cargo.toml
metadata > pyproject.toml [tool.typos], precedence per crate-ci/typos'
own docs and empirical verification), typos binary resolution from PATH,
typos --write-changes --force-exclude --format json fix pass, exit-code
branching (0 = clean/fixed, 2 = residual findings, other = tool break),
residual-finding parsing with remediation guidance pointing at
extend-words/extend-identifiers/extend-ignore-re, and hook-telemetry
envelope emission.

Hook-precision conformance: the convention is fleet-wide by intent (its
owner doc states "every plugin hook follows it") though CI-audited
enforcement is currently guardrails-only; this plugin opts in voluntarily.

Known, documented, out-of-scope risk: concurrent same-file PostToolUse
hook writes (no extension filter + no locking) — tracked in #875.

Phase 2: Setup skill + docs

skills/setup/SKILL.md (check/apply contract; apply is guidance-only
since typos has no per-repo dependency-manager install path, unlike Ruff or
markdownlint-cli2), README.md, CHANGELOG.md (starts at 0.1.0).

Phase 3: Contract test

hooks/typos-format.test.sh — black-box Bash contract test mirroring
ruff-format.test.sh's fixture-corpus shape: opt-in gate (present/absent
config, pyproject with/without [tool.typos], typos.toml/_typos.toml
precedence), fix + residual-finding reporting, config-exclude respect
(--force-exclude), kill switch, telemetry envelope shape, missing-binary
and missing-jq visibility notices.

Phase 4: Local CI-equivalent verification

Ran every gate the new directory trips in plugin-gate/hygiene locally
before opening the PR (hook-utils sync, plugin manifest validation, catalog
sync, changelog parity, silent-skip gate, cross-plugin source drift, skill
leaf names, skill quality gate, full fleet test suite, and a typos/
shellcheck/markdownlint-cli2 self-check on the new files).

Blast radius

LOW — new, additive plugin directory; zero changes to existing plugin
behavior beyond the synced hook-utils.sh copy (mechanical, CI-gated) and
two purely-additive doc/registry rows.

Open questions (resolved during implementation)

  1. Binary-file handling — empirically not an issue; typos handles arbitrary
    file content without a hard error.
  2. Setup apply scope — resolved to guidance-only (no auto-install),
    confirmed against crate-ci/typos' own install docs (no universal
    per-repo dependency-manager path exists for a standalone Rust binary).

🤖 Generated with Claude Code

Closes #831

New hook plugin mirroring the ruff-format/markdown-format pattern:
PostToolUse autofix via `typos --write-changes`, gated on a consumer
typos config found by ancestor walk-up (typos.toml > _typos.toml >
.typos.toml > Cargo.toml > pyproject.toml, matching typos' own
documented precedence), residual findings surfaced advisory-only with
extend-words/extend-identifiers/extend-ignore-re remediation guidance,
hook-telemetry envelope conformance, and a check/apply setup skill.

## Related

- Epic #830, sub-item 1 of docs/topics/lint-static-analysis-gaps/PLAN.md (#829)

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

claude Bot commented Jul 21, 2026

Copy link
Copy Markdown

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


PR Review: feat(typos-format): add per-file typos autofix hook plugin

Tasks:

  • Gather context and understand request
  • Read plugin manifest and hooks configuration
  • Review main hook script (typos-format.sh)
  • Review hook-utils.sh
  • Review test suite (typos-format.test.sh)
  • Review skill, README, and supporting files
  • Check marketplace.json and registry changes
  • Synthesize findings and post review

Overall Assessment

Approve with minor notes. The implementation is correct, secure, and closely follows the established ruff-format/markdown-format patterns. The 44-assertion black-box test suite is comprehensive, the --force-exclude regression case is properly covered, and the hook's advisory-only posture is correctly maintained throughout. Two minor items and one informational note follow.


Finding 1 — Minor: Case 4b doesn't fully enforce "only unfixable reported"

File: plugins/typos-format/hooks/typos-format.test.sh lines 203–215

The test description says "mixed fixable+unfixable -> only unfixable reported" but the assertion only checks that "disallowme" appears in additionalContext. It does not verify that "teh" (the fixed typo) is absent from the context:

if printf '%s' "$OUT" | jq -r '.hookSpecificOutput.additionalContext' 2>/dev/null | grep -q 'disallowme'; then
  ok "mixed fixable+unfixable -> only unfixable reported"

The hook's RC=0 branch (all-fixed path) returns before parsing any output, so "teh" cannot leak in that scenario. For the RC=2 branch, the hook only looks at select(.type == "typo") JSON output from typos after --write-changes. That behavior — that typos omits fixed findings from its JSON output when --write-changes completes them — is cited as "verified against typos-cli 1.44.0" in the hook header, but the test doesn't enforce the absence property explicitly.

The practical risk is low given the empirical verification, but a stronger assertion would be:

CTX=$(printf '%s' "$OUT" | jq -r '.hookSpecificOutput.additionalContext' 2>/dev/null)
if printf '%s' "$CTX" | grep -q 'disallowme' && ! printf '%s' "$CTX" | grep -q '"teh"'; then
  ok "mixed fixable+unfixable -> only unfixable reported (fixed 'teh' absent)"

Fix this →


Finding 2 — Minor: Marketplace entry has no relevance signals

File: .claude-plugin/marketplace.json lines 61–65

{
  "name": "typos-format",
  "source": "./plugins/typos-format",
  "category": "development",
  "tags": ["typos", "spelling", "formatter", "linter", "hook"]
}

Every other hook-based formatter plugin has a relevance block — ruff-format signals on ["**/*.py"] and cli: ["ruff"], biome-format on **/biome.json and a manifest dep, etc. typos-format is intentionally language-agnostic (no extension filter), but the typos binary and the three canonical config filenames (typos.toml, _typos.toml, .typos.toml) are reliable, unambiguous discovery signals:

"relevance": {
  "topic": "spelling/typo fixing",
  "signals": {
    "filesRead": ["**/typos.toml", "**/_typos.toml", "**/.typos.toml"],
    "cli": ["typos"]
  }
}

Without this the plugin requires a user to know its name. Given that the plugin's own opt-in gate is a typos config, surfacing the plugin when that config is present is exactly the right trigger.

Fix this →


Finding 3 — Info: PLAN.md committed to main tree

File: docs/topics/typos-format-hook/PLAN.md

290 lines of implementation-phase planning scaffolding. Similar files exist for other topics in this repo (docs/topics/lint-static-analysis-gaps/PLAN.md is referenced in the PR body), so this follows existing project convention. Not blocking — noting it for completeness and in case the team wants to revisit whether in-flight plans should be committed.


Code quality notes (non-blocking)

Hook logic is clean and correct. A few specific verifications:

  • Config precedence walk (typos-format.sh:140–157): The for name in typos.toml _typos.toml .typos.toml; do ... && break; done inner loop correctly breaks the for loop (not the while), and the outer [[ -n "$CONFIG_FOUND" ]] && break then exits the walk. Precedence mirrors typos' own Config::from_dir order.

  • Cargo.toml grep (typos-format.sh:144–146): '^[[:space:]]*\[(workspace|package)\.metadata\.typos(\]|[.]) correctly anchors to line start so it can't be tricked by a value string like name = "[workspace.metadata.typos]".

  • RC=0/2/other handling (typos-format.sh:198–249): Mapping typos' exit codes (0 = clean/fully-fixed, 2 = residual findings, other = tool error) to ok/ok/skipped telemetry status is consistent with how ruff-format handles Ruff's exit codes.

  • No command injection: $TYPOS_ARG is passed as a positional argument inside a quoted array via cd "$RUN_DIR" && "$TYPOS_BIN" --write-changes --force-exclude --format json "$TYPOS_ARG". JSON field values from typos output are extracted with jq -r (not eval), and path values are JSON-escaped before emission via hook::json_escape. Clean throughout.

  • --force-exclude regression (typos-format.sh:195 + test Case 5): Flag is present and the exclusion case is properly exercised.

  • Known race with parallel hooks: Documented in both the hook header and README.md. Not addressable at the hook level today.


@claude

claude Bot commented Jul 21, 2026

Copy link
Copy Markdown

Claude finished @kyle-sexton's task in 5m 48s —— View job


Security Review — PR #872 feat(typos-format): add per-file typos autofix hook plugin

  • Gather context — reading changed files
  • Review shell scripts for injection surfaces and unsafe patterns
  • Review hook-utils.sh for command/path injection
  • Review typos-format.sh for unsafe variable handling
  • Review plugin manifest and hooks config
  • Synthesize and post findings

Findings

No CRITICAL or IMPORTANT vulnerabilities found. Two SUGGESTION-level findings below.


SUGGESTION — Confidence: HIGH

Test script constructs JSON with printf %s without escaping the file path

plugins/typos-format/hooks/typos-format.test.shrun_hook, L97–100 and run_hook_env, L108–111:

printf '{"tool_input":{"file_path":"%s"},"tool_name":"Write"}' "$file_path" |

%s does not JSON-escape its argument. A path containing ", \, or a newline would produce malformed JSON, which the hook would silently discard (jq parse failure → hook::buffer_stdin returns empty → exit 0). The test would then misread the skip as a pass. In practice all test paths come from mktemp and fixed string suffixes, so no test path ever carries these characters — but this is a latent fragility rather than a defence.

The production hook is unaffected: it reads file paths from stdin JSON via jq -r, which handles all escaping correctly.


SUGGESTION — Confidence: MEDIUM

Relative HOOK_TELEMETRY_SINK resolved without containment check

plugins/typos-format/hooks/hook-utils.sh L438–446 (telemetry sink resolution):

local sink="$HOOK_TELEMETRY_SINK"
case "$sink" in
/* | [A-Za-z]:[/\\]*) ;;     # absolute — used as-is
*)
  local root="${repo_root:-${CLAUDE_PROJECT_DIR:-}}"
  [[ -n "$root" ]] || return 0
  sink="${root%/}/$sink"      # relative — joined onto repo root
  ;;
esacprintf '%s\n' "$envelope" | ("$sink" >/dev/null 2>&1) &

A relative HOOK_TELEMETRY_SINK value such as ../../outside-repo/sink is joined onto the repo root with no ..-normalization or containment check, so the resolved executable path can fall outside the repository. The code acknowledges the portability motivation for the relative form (L385–387), and since HOOK_TELEMETRY_SINK is exclusively consumer-set configuration (in settings.json), an escape from the repo root is already under the consumer's full control — they could equally set an absolute path anywhere. Severity is therefore low, but the lack of a guard is worth documenting as a trust boundary for any future consumer-facing docs that describe what relative values mean.


What looks good

  • No shell-word-splitting injection. All variables passed to external commands ($TYPOS_BIN, $TYPOS_ARG, $RUN_DIR, $FILE, $REPO_ROOT, $sink) are consistently double-quoted. set -u catches unbound variable dereferences early.
  • JSON output is safe regardless of file content. The residual-findings path extracts typo words with jq -r and emits them into additionalContext via hook::emit_additional_context, which uses jq -n --arg, so jq handles all JSON-string escaping natively. Skip-notice messages (the only path through hook::emit_channels, which uses hook::json_escape) are entirely author-controlled strings, not derived from file content.
  • File path validation chain is correct. hook::read_file_path checks existence with -f and validates project membership by comparing canonicalized (symlink-resolved via hook::physical_path) paths against CLAUDE_PROJECT_DIR, anchored on a path-segment boundary to prevent sibling-repo admittance.
  • Session ID sanitized before file-system use. hook::notice_once strips session ID to [A-Za-z0-9_-] before constructing the marker path under CLAUDE_PLUGIN_DATA.
  • No command injection through config file content. The Cargo.toml / pyproject.toml opt-in check uses grep -qE with a hardcoded pattern; no config-file content is ever interpolated into a shell command or executed.
  • typos binary resolved from $PATH only. No download, no per-repo script execution, no eval.
  • hook-utils.sh hook::ansi_c_decode format-string injection prevented. The function pre-escapes %%% before using the body as a printf format string, preventing injected format specifiers.
  • No GitHub Actions files changed. No pull_request_target, workflow permissions: widening, or github context injection surfaces are present.

- Strengthen the mixed fixable+unfixable test assertion to also verify
  the fixed typo is absent from the residual advisory context.
- Add a marketplace relevance block (typos config filenames + typos
  CLI signal), matching sibling formatter-plugin entries.
- Prune docs/topics/typos-format-hook/PLAN.md now that the plan is
  captured in the PR description (plan close-out).

Review findings from PR #872 (Claude Code review + security review).

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

claude Bot commented Jul 21, 2026

Copy link
Copy Markdown

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


PR Review — fix(typos-format): address review findings, prune topic plan

Tasks:

  • Gather context and understand the request
  • Diff the latest commit against the prior one to see what changed
  • Verify test assertion strength (Finding 1 from prior review)
  • Verify marketplace relevance block (Finding 2 from prior review)
  • Verify PLAN.md was pruned (Finding 3 from prior review / PR body)
  • Synthesize findings and post final review

Summary

The fix(typos-format) follow-up commit correctly addresses all three findings raised in the prior code review. No new issues introduced.


Finding 1 — Resolved: Mixed fixable+unfixable test assertion strengthened

typos-format.test.sh L211–215

The fix matches the suggested approach exactly: the context is captured into CTX_MIXED, and both properties are asserted:

CTX_MIXED=$(printf '%s' "$OUT" | jq -r '.hookSpecificOutput.additionalContext' 2>/dev/null)
if printf '%s' "$CTX_MIXED" | grep -q 'disallowme' && ! printf '%s' "$CTX_MIXED" | grep -q '"teh"'; then
  ok "mixed fixable+unfixable -> only unfixable reported (fixed 'teh' absent)"

The negative assertion (! grep -q '"teh"') now enforces the claim that fixed typos are not leaked into residual advisory output. The failure message also improved to include the actual context value. ✅


Finding 2 — Resolved: relevance block added to marketplace entry

.claude-plugin/marketplace.json L64–71

"relevance": {
  "topic": "Spelling / typo fixing",
  "signals": {
    "filesRead": ["**/typos.toml", "**/_typos.toml", "**/.typos.toml"],
    "cli": ["typos"]
  }
}

Signal selection is well-judged: only typos-specific config filenames are listed (not the general-purpose Cargo.toml or pyproject.toml that the opt-in gate also accepts), and cli: ["typos"] adds the binary-presence signal. The structure is consistent with sibling formatter entries. ✅


Finding 3 — Resolved: docs/topics/typos-format-hook/PLAN.md removed

The docs/topics/typos-format-hook/ directory has been deleted entirely (290 lines pruned). Plan content is captured in the PR description for the historical record. ✅


No new issues

The three changes are minimal, targeted, and correct. The security review's two SUGGESTION-level items (test-harness JSON path escaping, relative HOOK_TELEMETRY_SINK containment) remain noted as pre-existing fleet patterns; nothing in this follow-up commit introduces or worsens them.

Ready to merge.

@kyle-sexton

Copy link
Copy Markdown
Contributor Author

Addressed in 1df48cc:

Finding 1 (Case 4b assertion) — CORRECT. Strengthened to also assert the fixed typo is absent from the residual context.

Finding 2 (marketplace relevance) — CORRECT. Added, matching ruff-format's pattern (typos config filenames + typos CLI signal).

Finding 3 (PLAN.md committed) — pruned; the plan is now captured in full in the PR description.

Security suggestion 1 (test JSON escaping) — confirmed and correct, but deferred: plugins/ruff-format/hooks/ruff-format.test.sh and plugins/markdown-format/hooks/markdown-format.test.sh use the identical unescaped printf '%s' pattern for every test fixture — this is a fleet-wide, pre-existing test-harness convention, not something introduced by this PR. Fixing it in one plugin's test file alone would diverge from the shared pattern without closing the gap elsewhere. Filed nothing new here; flagging for a future fleet-wide test-harness hygiene pass if one is warranted.

Security suggestion 2 (relative telemetry sink containment) — confirmed and correct, but out of scope: the code lives in lib/hook-utils.sh (the shared source), synced byte-for-byte into every plugin's hooks/hook-utils.sh copy via scripts/sync-hook-utils.sh. Per this repo's own convention, a plugin's copy is never hand-edited — a fix belongs in the shared source and would affect every hook that emits telemetry, not just typos-format.

Two follow-up issues also filed from findings surfaced during review: #874 (ruff-format missing its own telemetry schema/registry row) and #875 (fleet-wide concurrent-hook-write race, documented as a known limitation in this plugin).

@claude

claude Bot commented Jul 21, 2026

Copy link
Copy Markdown

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


Security Review — PR #872 feat(typos-format): add per-file typos autofix hook plugin

Tasks:

  • Load tools and initialize
  • Fetch PR diff / read changed files
  • Read main hook script (typos-format.sh)
  • Read hook-utils.sh (injection surfaces, telemetry sink)
  • Read test script for test-harness security patterns
  • Read plugin manifest, hooks.json, marketplace.json
  • Synthesize and post findings

No CRITICAL or IMPORTANT vulnerabilities found. The two SUGGESTION-level findings from the previous CI security review remain valid and are reproduced below with updated confidence assessments after independent re-verification. No new findings.


SUGGESTION — Confidence: HIGH

Test harness constructs hook input JSON with printf %s and an unescaped file path

plugins/typos-format/hooks/typos-format.test.sh L98 and L109:

printf '{"tool_input":{"file_path":"%s"},"tool_name":"Write"}' "$file_path" |

A path containing ", \, or a newline produces malformed JSON. The hook's hook::buffer_stdin would then parse-fail and exit 0, which the test framework would misread as a pass. In practice, all test paths come from mktemp with fixed suffixes, so no test path ever carries these bytes — the risk is latent fragility only. The production hook is unaffected: it reads file paths from Claude Code's stdin JSON via jq -r, which handles escaping correctly. This is a pre-existing fleet-wide pattern also present in ruff-format.test.sh and markdown-format.test.sh.


SUGGESTION — Confidence: MEDIUM

Relative HOOK_TELEMETRY_SINK is joined to the repo root without a path-containment check

plugins/typos-format/hooks/hook-utils.sh L438–446:

case "$sink" in
/* | [A-Za-z]:[/\\]*) ;;     # absolute — used as-is
*)
  local root="${repo_root:-${CLAUDE_PROJECT_DIR:-}}"
  [[ -n "$root" ]] || return 0
  sink="${root%/}/$sink"      # relative — no .. normalization or containment check
  ;;
esac
printf '%s\n' "$envelope" | ("$sink" >/dev/null 2>&1) &

A relative value of ../../outside-repo/sink resolves an executable path outside the repository. Since HOOK_TELEMETRY_SINK is exclusively consumer-set (in settings.json), this is already under the consumer's full control — they could equally provide an absolute path to any executable. Severity is low and documented as a trust boundary in the PR. The code lives in the shared source (lib/hook-utils.sh) and any fix would need to land there and propagate via sync-hook-utils.sh, not in this plugin's copy.


What looks correct

  • No command injection in the main hook (typos-format.sh:195): $TYPOS_BIN, $RUN_DIR, and $TYPOS_ARG are all double-quoted in the invocation; TYPOS_BIN is resolved from PATH via command -v (no download, no per-repo binary resolution). No eval anywhere.
  • typos output safely embedded in JSON (typos-format.sh:211–223): Typo words and corrections extracted with jq -r .typo / jq -r .corrections[0] flow through hook::ctx_appendhook::emit_additional_contextjq -n --arg ctx. JQ handles all JSON string escaping; no hand-built interpolation of file content into JSON.
  • Tool-break stderr also safe (typos-format.sh:241–246): Raw typos stderr captured via 2>&1 and echoed line-by-line into the context buffer. The buffer ultimately reaches jq -n --arg ctx, so arbitrary typos error output cannot inject JSON.
  • File path validation chain is correct (hook-utils.sh:193–211): hook::read_file_path validates existence (-f) and project membership by comparing canonicalized (symlink-resolved via hook::physical_path) paths against CLAUDE_PROJECT_DIR, anchored on a path-segment boundary.
  • Session ID sanitized before filesystem use (hook-utils.sh:98): session="${session//[^A-Za-z0-9_-]/-}" strips all non-alphanumeric-safe characters before using the session ID to construct a marker file path under CLAUDE_PLUGIN_DATA.
  • hook::json_escape coverage is complete (hook-utils.sh:37–50): Escapes \, ", \n, \r, \t, then tr -d drops remaining C0 bytes — covers the full JSON string requirement. hook::ansi_c_decode pre-escapes %%% before using the body as a printf format string, preventing format-specifier injection.
  • No GitHub Actions files changed. No pull_request_target, workflow_run, permissions: widening, or github context injection surfaces present.
  • No supply-chain risk: No new dependency pins, no action version changes, no external fetch or eval in the hook.

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 1df48cc3a8

ℹ️ 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".

Comment thread plugins/typos-format/hooks/typos-format.sh
The Case 4b strengthening in the prior commit introduced two more
literal "teh" occurrences the repo's own typos gate correctly caught
(hygiene CI, not local: the earlier local self-check ran before this
edit). Route through a suppressed variable instead of embedding the
literal twice.

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

claude Bot commented Jul 21, 2026

Copy link
Copy Markdown

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


Tasks:

  • Gather context and load tools
  • Review latest commit (c9dff9b) diff
  • Evaluate Codex P2 finding on inline TOML configs
  • Synthesize and post final review

Review — fix(typos-format): suppress typos gate hit from own test literal

No issues found. Ready to merge.


Latest commit (c9dff9b) — Correct

The Case 4b strengthening in 1df48cc embedded the literal teh twice directly in the test source, causing the repo's own typos CI gate to flag it. The fix routes it through a suppressed variable:

FIXED_TYPO='teh' # spellchecker:disable-line
if! printf '%s' "$CTX_MIXED" | grep -qF "\"$FIXED_TYPO\""; then
  ok "mixed fixable+unfixable -> only unfixable reported (fixed typo absent)"

This is the right approach: one variable assignment, one suppression comment, and the test assertion semantics are preserved exactly. The switch from -q to -qF is a minor correctness improvement — the pattern has no regex metacharacters but -F makes the intent explicit.

The approach is consistent with plugins/typos-format/hooks/typos-format.test.sh:227 on the very next case (# spellchecker:disable-line inline), so suppression style is now uniform.


Codex P2 finding — Not a merge blocker

The Codex comment observes that the opt-in gate grep does not recognize the TOML inline-table form:

[tool]
typos = { extend-words = { ... } }

This is technically correct: the pyproject.toml grep at typos-format.sh:148–150 only matches [tool.typos]-style section headers, not dotted-key inline tables under [tool]. The Cargo.toml check has the same structural limit.

Two reasons not to fix this now:

  1. Severity is low. The consequence of missing the form is the hook silently skips — no incorrect behavior, no data loss, same outcome as having no config. This is a discovery gap, not a correctness defect.
  2. The fix is fragile. A multi-pass grep (grep [tool] followed by grep typos =) would produce false positives whenever a pyproject.toml has an unrelated typos = key inside any other section. Reliable detection would require a proper TOML parser, which is out of scope for a shell hook.

The inline-table form for a full section is also unusual enough in real pyproject.toml files that the practical impact is very low. This belongs in a follow-up issue (same class as the concurrent-hook-write race in #875) if the team wants to track it, but it is not a reason to hold this PR.

@claude

claude Bot commented Jul 21, 2026

Copy link
Copy Markdown

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


Security Review — PR #872 feat(typos-format): add per-file typos autofix hook plugin

Tasks:

  • Load tools and initialize
  • Fetch PR diff and read changed files
  • Audit typos-format.sh (main hook) for injection surfaces
  • Audit hook-utils.sh for unsafe patterns
  • Audit test script, hooks.json, marketplace.json, plugin.json
  • Audit latest fix commit (c9dff9b — suppress typos gate hit from own test literal)
  • Synthesize and post findings

No CRITICAL or IMPORTANT vulnerabilities found. Two SUGGESTION-level findings remain — both pre-existing fleet-wide patterns, not introduced by this PR. One informational note.


SUGGESTION — Confidence: HIGH

Test harness constructs hook input JSON with printf %s and an unescaped file path

plugins/typos-format/hooks/typos-format.test.sh L98 and L109:

printf '{"tool_input":{"file_path":"%s"},"tool_name":"Write"}' "$file_path" |

A path containing ", \, or a newline produces malformed JSON. The hook's hook::buffer_stdin would parse-fail and exit 0, which the test framework would misread as a pass. In practice all test paths come from mktemp with fixed suffixes, so no test path ever carries these bytes — the risk is latent fragility only. The production hook is unaffected: it reads file paths from Claude Code's stdin JSON via jq -r, which handles escaping correctly. This is a pre-existing fleet-wide pattern also present in ruff-format.test.sh and markdown-format.test.sh. The latest commit (c9dff9b) only modifies lines 211–215 and does not affect this surface.


SUGGESTION — Confidence: MEDIUM

Relative HOOK_TELEMETRY_SINK joined to repo root without a path-containment check

plugins/typos-format/hooks/hook-utils.sh L438–446:

case "$sink" in
/* | [A-Za-z]:[/\\]*) ;;     # absolute — used as-is
*)
  local root="${repo_root:-${CLAUDE_PROJECT_DIR:-}}"
  [[ -n "$root" ]] || return 0
  sink="${root%/}/$sink"      # relative — no .. normalization or containment check
  ;;
esac
printf '%s\n' "$envelope" | ("$sink" >/dev/null 2>&1) &

A relative value of ../../outside-repo/sink resolves an executable path outside the repository. Since HOOK_TELEMETRY_SINK is set exclusively by the consumer in settings.json, this is already under the consumer's full control — they could equally provide an absolute path to any executable. The code lives in the shared source (lib/hook-utils.sh); any fix must land there and propagate via scripts/sync-hook-utils.sh, not in this plugin's copy.


What looks correct

  • No command injection in the main hook (typos-format.sh:195): $TYPOS_BIN, $RUN_DIR, and $TYPOS_ARG are all double-quoted; TYPOS_BIN is resolved from PATH via command -v (no download, no per-repo binary execution, no eval anywhere).

  • File-derived content safely embedded in JSON (typos-format.sh:213–222): Typo words extracted with jq -r '.typo // empty' flow through hook::ctx_appendhook::ctx_flushhook::emit_additional_contextjq -n --arg ctx. JQ handles all JSON string escaping natively; no hand-built interpolation of file content into JSON anywhere in the hot path.

  • Tool-break stderr also safe (typos-format.sh:242–245): Raw typos stderr captured via 2>&1 and echoed line-by-line into the context buffer. The buffer reaches jq -n --arg ctx, so arbitrary typos error output cannot inject into the JSON response.

  • File path validation chain is correct (hook-utils.sh:193–211): hook::read_file_path validates existence (-f) and project membership by comparing canonicalized (symlink-resolved via hook::physical_path) paths against CLAUDE_PROJECT_DIR, anchored on a path-segment boundary so /c/repo-backup/… cannot be admitted by sharing a prefix with /c/repo.

  • Session ID sanitized before filesystem use (hook-utils.sh:98): session="${session//[^A-Za-z0-9_-]/-}" strips all non-alphanumeric-safe characters before constructing the marker file path under CLAUDE_PLUGIN_DATA.

  • hook::json_escape coverage is complete (hook-utils.sh:37–50): Escapes \, ", \n, \r, \t, then tr -d drops remaining C0 bytes. hook::ansi_c_decode pre-escapes %%% before using the body as a printf format string, blocking format-specifier injection.

  • Cargo.toml grep pattern anchored correctly (typos-format.sh:145): '^[[:space:]]*\[(workspace|package)\.metadata\.typos(\]|[.])' anchors to line start via ^, so a config value string containing [workspace.metadata.typos] cannot trigger a false opt-in.

  • Latest commit (c9dff9b) is minimal and clean: Introduces FIXED_TYPO='teh' as an indirection variable and switches to grep -qF "\"$FIXED_TYPO\"". No new execution surfaces or variable-expansion paths introduced.

  • No GitHub Actions files changed. No pull_request_target, workflow_run, permissions: widening, or github context injection surfaces present. No supply-chain risk from loosened action or dependency pins.


Ready to merge from a security perspective.

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: c9dff9b71c

ℹ️ 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".

Comment thread plugins/typos-format/hooks/typos-format.sh
…fig regression test

Codex review on #872 raised a config-resolution concern (running from
repo root vs. the discovered config's own directory). Verified
empirically: typos resolves config relative to the target path passed
on the command line, not the process CWD, so this hook's existing
behavior is correct. Clarifies the header comment's imprecise
"CWD-anchored" wording and locks the scenario in with a permanent
regression test (nested config, no config at repo root).

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

claude Bot commented Jul 21, 2026

Copy link
Copy Markdown

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


PR Review — docs(typos-format): clarify config-resolution comment, add nested-config regression test

Tasks:

  • Gather context and load tools
  • Review latest commit (4626e1e) diff
  • Evaluate Codex P2 finding response (nested config resolution)
  • Review new regression test (Case 3b)
  • Synthesize and post final review

Summary

Approve. The commit correctly addresses the Codex P2 concern and adds a permanent regression test locking in the empirically-verified behavior. One minor stale-comment nit, noted below but not blocking.


Codex P2 — Correctly refuted and documented

The Codex finding claimed the hook should cd to the discovered config directory or pass --config explicitly because typos' config discovery is "CWD-anchored." Kyle's empirical verification — and this regression test — prove that is incorrect: typos resolves config relative to the target path passed on the command line, not the process CWD. Running from the repo root with a subdirectory-relative path still discovers and honors a config nested at any depth.

The comment update in typos-format.sh:175–181 is accurate and materially more precise than the old "CWD-anchored" label.


Case 3b — Correct regression test

typos-format.test.sh:181–196

The test correctly:

  • Creates a repo with no config at the root (NO_CONFIG)
  • Places a _typos.toml only at packages/pkg/
  • Writes a file with the custom pkgword typo there
  • Invokes the hook with the absolute file path (the hook will cd to the repo root and pass the relative path to typos)
  • Asserts that correctword appears in the file — proving typos found and applied the nested config despite running from a parent directory

The [default.extend-words]\npkgword = "correctword"\n fixture is the right shape: typos treats the key as an incorrect form and replaces it with the value under --write-changes. The assertion is correct.


Minor nit — Test file header still carries the old label

typos-format.test.sh:13

# The hook is invoked from an UNRELATED cwd so any reliance on the caller's
# working directory would surface (typos' config discovery is CWD-anchored;
# the hook cd's to the repo root before running).

The parenthetical still says typos' config discovery is CWD-anchored, which is now directly contradicted by the correction in typos-format.sh and the newly-added Case 3b. The commit message says it "Clarifies the header comment's imprecise 'CWD-anchored' wording" but only updated the hook file, not this test-file header. Not blocking — the behavior is correct and the regression test proves it — but the stale phrasing undermines the clarification's completeness.

Fix this →


What looks correct

  • Behavioral change: none. This is a docs-only commit plus a test addition. No execution path changed.
  • New test is self-contained: uses $WORK-prefixed temp dir, proper new_typos_repo NO_CONFIG baseline, and is placed correctly between Case 3 and Case 4 in execution order.
  • No spellchecker false positives: pkgword and correctword are not in typos' built-in dictionary, so the test fixture won't accidentally fire on unrelated CI checks.

Ready to merge.

@claude

claude Bot commented Jul 21, 2026

Copy link
Copy Markdown

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


Security Review — PR #872 feat(typos-format): add per-file typos autofix hook plugin

Tasks:

  • Load tools and initialize
  • Fetch PR diff and read changed files (latest commit: 4626e1e)
  • Audit typos-format.sh — injection surfaces, variable handling
  • Audit hook-utils.sh — unsafe patterns, sink resolution
  • Audit test script, hooks.json, marketplace.json, plugin.json
  • Audit latest commit (nested-config regression test + docs clarification)
  • Synthesize and post findings

No CRITICAL or IMPORTANT vulnerabilities found. Two SUGGESTION-level findings remain — both pre-existing fleet-wide patterns, not introduced by this PR. The latest commit (4626e1e) is a comment clarification and regression test addition; it introduces no new execution surfaces.


SUGGESTION — Confidence: HIGH

Test harness constructs hook input JSON with printf %s and an unescaped file path

plugins/typos-format/hooks/typos-format.test.sh L98 and L109:

printf '{"tool_input":{"file_path":"%s"},"tool_name":"Write"}' "$file_path" |

A path containing ", \, or a newline produces malformed JSON. The hook's hook::buffer_stdin would parse-fail and exit 0, which the test framework would misread as a pass. In practice all test paths come from mktemp with fixed suffixes, so no test path ever carries these bytes — the risk is latent fragility only. The production hook is unaffected: it reads file paths from Claude Code's stdin JSON via jq -r, which handles escaping correctly. This is a pre-existing fleet-wide pattern also present in ruff-format.test.sh and markdown-format.test.sh. The latest commit adds a new Case 3b test (L188–195) using run_hook with the same pattern — no worsening introduced.


SUGGESTION — Confidence: MEDIUM

Relative HOOK_TELEMETRY_SINK joined to repo root without a path-containment check

plugins/typos-format/hooks/hook-utils.sh L438–446:

case "$sink" in
/* | [A-Za-z]:[/\\]*) ;;     # absolute — used as-is
*)
  local root="${repo_root:-${CLAUDE_PROJECT_DIR:-}}"
  [[ -n "$root" ]] || return 0
  sink="${root%/}/$sink"      # relative — no .. normalization or containment check
  ;;
esac
printf '%s\n' "$envelope" | ("$sink" >/dev/null 2>&1) &

A relative value of ../../outside-repo/sink resolves an executable path outside the repository. Since HOOK_TELEMETRY_SINK is set exclusively by the consumer in settings.json, this is already under the consumer's full control — they could equally provide an absolute path to any executable. The code lives in the shared source (lib/hook-utils.sh); any fix must land there and propagate via scripts/sync-hook-utils.sh, not in this plugin's copy.


What looks correct

  • No command injection in the main hook (typos-format.sh:199): $TYPOS_BIN, $RUN_DIR, and $TYPOS_ARG are all double-quoted; TYPOS_BIN is resolved from PATH via command -v (no download, no per-repo binary execution, no eval anywhere).

  • File-derived content safely embedded in JSON (typos-format.sh:213–228): Typo words extracted with jq -r '.typo // empty' flow through hook::ctx_appendhook::ctx_flushhook::emit_additional_contextjq -n --arg ctx. JQ handles all JSON string escaping natively. build_data_json() (typos-format.sh:109–116) likewise uses jq -n --arg/--argjson throughout — no hand-built JSON interpolation of untrusted values.

  • Tool-break stderr also safe (typos-format.sh:244–249): Raw typos stderr captured via 2>&1 and echoed line-by-line into the context buffer. The buffer ultimately reaches jq -n --arg ctx, so arbitrary typos error output cannot inject into the JSON response.

  • File path validation chain is correct (hook-utils.sh:193–211): hook::read_file_path validates existence (-f) and project membership by comparing canonicalized (symlink-resolved via hook::physical_path) paths against CLAUDE_PROJECT_DIR, anchored on a path-segment boundary so /c/repo-backup/… cannot be admitted by sharing a prefix with /c/repo.

  • Session ID sanitized before filesystem use (hook-utils.sh:98): session="${session//[^A-Za-z0-9_-]/-}" strips all non-alphanumeric-safe characters before constructing the marker file path under CLAUDE_PLUGIN_DATA.

  • hook::json_escape coverage is complete (hook-utils.sh:37–50): Escapes \, ", \n, \r, \t, then tr -d drops remaining C0 bytes. hook::ansi_c_decode pre-escapes %%% before using the body as a printf format string, blocking format-specifier injection.

  • Cargo.toml grep pattern anchored correctly (typos-format.sh:145): '^[[:space:]]*\[(workspace|package)\.metadata\.typos(\]|[.])' anchors to line start via ^, so a config value string containing [workspace.metadata.typos] cannot trigger a false opt-in.

  • Latest commit (4626e1e) is minimal and clean: A 5-line comment rewrite in typos-format.sh and a 16-line regression test in the test file. No new execution surfaces, variable-expansion paths, or injection vectors introduced.

  • No GitHub Actions files changed. No pull_request_target, workflow_run, permissions: widening, or github context injection surfaces present. No supply-chain risk from loosened action or dependency pins.


Ready to merge from a security perspective.

Missed updating this copy in the prior commit — it still contradicted
the corrected typos-format.sh comment.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_019KVQkSXBKY8pcCSNKUnP3g
@kyle-sexton

Copy link
Copy Markdown
Contributor Author

Fixed the stale wording in typos-format.test.sh's own header comment (552fd7f) — it still said "CWD-anchored" after the prior commit only updated typos-format.sh. Both now consistently describe target-path-anchored resolution, matching the Case 3b regression test.

@claude

claude Bot commented Jul 21, 2026

Copy link
Copy Markdown

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


Tasks

  • Gather context and understand request
  • Review latest commit (552fd7f) — stale CWD-anchored wording fix
  • Synthesize and post final review

Review — docs(typos-format): fix stale CWD-anchored wording in test file header

Approved. Ready to merge.

The latest commit (552fd7f) resolves the last open nit from the 4626e1e review: the test file header still described typos' config discovery as "CWD-anchored" after the prior commit had already corrected that framing in typos-format.sh. Both files now consistently describe the actual, empirically-verified behavior.

Updated text (typos-format.test.sh:12–15):

-# working directory would surface (typos' config discovery is CWD-anchored;
-# the hook cd's to the repo root before running).
+# own working directory would surface (typos resolves config relative to the
+# target path passed on the command line, not the process CWD — Case 3b below
+# locks this in — so the hook's own cd to the repo root does not change which
+# config governs).

This is consistent with the correction already in typos-format.sh:175–181 and anchored by the Case 3b regression test added in 4626e1e. No execution path changed; this is documentation only.


Summary of all review rounds

Commit Finding Status
ed15527 Case 4b assertion didn't verify absence of fixed typo Fixed in 1df48cc
ed15527 Marketplace typos-format entry missing relevance block Fixed in 1df48cc
ed15527 docs/topics/typos-format-hook/PLAN.md committed to main tree Pruned in 1df48cc
1df48cc Embedded literal teh triggered repo's own typos CI gate Fixed in c9dff9b
c9dff9b Codex P2: Inline TOML config form not recognized Deferred — pre-existing fleet pattern; filed #876
c9dff9b Codex P2: cd to config dir / pass --config explicitly Refuted empirically; regression-tested in 4626e1e
4626e1e Test file header still said "CWD-anchored" Fixed in 552fd7f
All Security SUGGESTION: test harness unescaped printf %s path Pre-existing fleet pattern — deferred
All Security SUGGESTION: relative HOOK_TELEMETRY_SINK containment Shared lib concern — deferred to lib/hook-utils.sh

All code-review findings are resolved. Both remaining security suggestions are pre-existing fleet-wide patterns not introduced by this PR, correctly deferred. No blocking issues remain.

@claude

claude Bot commented Jul 21, 2026

Copy link
Copy Markdown

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


Tasks:

  • Gather context and load tools
  • Fetch PR diff — read changed files (latest commit: 552fd7f)
  • Audit for security issues
  • Post findings

Security Review — PR #872 feat(typos-format): add per-file typos autofix hook plugin

No CRITICAL or IMPORTANT vulnerabilities found. The two SUGGESTION-level findings from prior CI security review passes remain valid and are reproduced below after independent verification. The latest commit (552fd7f) is a 6-line comment rewrite in the test file header — no code, no execution surfaces changed.


SUGGESTION — Confidence: HIGH

Test harness constructs hook input JSON with printf %s and an unescaped file path

plugins/typos-format/hooks/typos-format.test.sh L98 and L109:

printf '{"tool_input":{"file_path":"%s"},"tool_name":"Write"}' "$file_path" |

A path containing ", \, or a newline produces malformed JSON. The hook's hook::buffer_stdin would then parse-fail and exit 0, which the test framework would misread as a pass. In practice all test paths come from mktemp with fixed suffixes, so no test path ever carries these bytes — the risk is latent fragility only. The production hook is unaffected: it reads file paths from Claude Code's stdin JSON via jq -r, which handles escaping correctly. This is a pre-existing fleet-wide pattern also present in ruff-format.test.sh and markdown-format.test.sh. No changes in the latest commit affect this surface.


SUGGESTION — Confidence: MEDIUM

Relative HOOK_TELEMETRY_SINK joined to repo root without a path-containment check

plugins/typos-format/hooks/hook-utils.sh L438–446:

case "$sink" in
/* | [A-Za-z]:[/\\]*) ;;     # absolute — used as-is
*)
  local root="${repo_root:-${CLAUDE_PROJECT_DIR:-}}"
  [[ -n "$root" ]] || return 0
  sink="${root%/}/$sink"      # relative — no .. normalization or containment check
  ;;
esac
printf '%s\n' "$envelope" | ("$sink" >/dev/null 2>&1) &

A relative value of ../../outside-repo/sink resolves an executable path outside the repository. Since HOOK_TELEMETRY_SINK is set exclusively by the consumer in settings.json, this is already under the consumer's full control — they could equally provide an absolute path to any executable. Severity is low. The code lives in the shared source (lib/hook-utils.sh); any fix must land there and propagate via scripts/sync-hook-utils.sh, not in this plugin's copy.


What looks correct

  • No command injection in the main hook (typos-format.sh:199): $TYPOS_BIN, $RUN_DIR, and $TYPOS_ARG are all double-quoted in the invocation; TYPOS_BIN is resolved from PATH via command -v — no download, no per-repo binary execution, no eval anywhere.

  • File-derived content safely embedded in JSON (typos-format.sh:213–228): Typo words extracted with jq -r '.typo // empty' flow through hook::ctx_appendhook::ctx_flushhook::emit_additional_contextjq -n --arg ctx. JQ handles all JSON string escaping natively; no hand-built interpolation of file content into JSON anywhere in the hot path.

  • Tool-break stderr also safe (typos-format.sh:244–249): Raw typos stderr captured via 2>&1 and echoed line-by-line into the context buffer; the buffer ultimately reaches jq -n --arg ctx, so arbitrary tool error output cannot inject into the JSON response.

  • File path validation chain is correct (hook-utils.sh:193–211): hook::read_file_path validates existence (-f) and project membership by comparing canonicalized (symlink-resolved via hook::physical_path) paths against CLAUDE_PROJECT_DIR, anchored on a path-segment boundary so /c/repo-backup/… cannot be admitted by sharing a prefix with /c/repo.

  • Session ID sanitized before filesystem use (hook-utils.sh:98): session="${session//[^A-Za-z0-9_-]/-}" strips all non-alphanumeric-safe characters before constructing the marker file path under CLAUDE_PLUGIN_DATA.

  • hook::json_escape coverage is complete (hook-utils.sh:37–50): Escapes \, ", \n, \r, \t, then tr -d drops remaining C0 bytes. hook::ansi_c_decode pre-escapes %%% before using the body as a printf format string, blocking format-specifier injection.

  • Cargo.toml grep pattern anchored correctly (typos-format.sh:145): '^[[:space:]]*\[(workspace|package)\.metadata\.typos(\]|[.])' anchors to line start via ^, so a config value string containing [workspace.metadata.typos] cannot trigger a false opt-in.

  • Latest commit (552fd7f) is a no-op from a security standpoint: Updates a 6-line comment in typos-format.test.sh from "CWD-anchored" to "target-path-anchored" wording. No execution paths, variable expansions, or injection surfaces were touched.

  • No GitHub Actions files changed. No pull_request_target, workflow_run, permissions: widening, or github context injection surfaces present. No supply-chain risk from loosened action or dependency pins.


Ready to merge from a security perspective.

@kyle-sexton
kyle-sexton merged commit c0a9a69 into main Jul 21, 2026
23 checks passed
@kyle-sexton
kyle-sexton deleted the feat/831-typos-hook-plugin branch July 21, 2026 17:35
@kyle-sexton

Copy link
Copy Markdown
Contributor Author

Filed independently while comparing this merged plugin against a parallel implementation (#880, now superseded/closing as duplicate of this one). One material behavioral gap worth flagging even though this already merged, since it affects real users of the plugin:

This hook only runs when the consuming repo already has a typos.toml/_typos.toml/.typos.toml/Cargo.toml([*.metadata.typos])/pyproject.toml([tool.typos]) file. Without one, typos-format.sh exits silently via emit_skipped and never invokes typos at all — confirmed by reading the merged hook script and its own test suite (Case 1: opt-in gate OFF (no typos config) -> file left untouched).

That's the ruff-format/biome-format opt-in-gate pattern, but typos isn't shaped like Ruff or Biome:

  • typos ships a built-in spelling dictionary and runs standalone with zero configuration — its own README frames it as something you run unconditionally ("fast enough to run on monorepos... low false positives so you can run on PRs"), not something a repo must first adopt a config for.
  • Verified empirically against a real typos-cli 1.44.0 binary: typos <file> catches and fixes real typos (e.g. recievereceive) in a directory tree with no _typos.toml anywhere above it. A _typos.toml only widens the allowlist (extend-words/extend-identifiers/extend-ignore-re) and excludes ([files] extend-exclude) — it is not an activation switch.
  • The originating brief (docs/topics/lint-static-analysis-gaps/PLAN.md, brief item 1) describes "consumer _typos.toml ancestor walk-up" in the context of "false-positive remediation via consumer allowlist entries" — i.e., config narrows false positives, it doesn't gate whether the hook does anything.
  • The dispatch instructions for this plugin explicitly said to match markdown-format's pattern (zero-config, ships none of its own rules, always active using the tool's own built-in defaults) rather than ruff-format's (opt-in required) — markdown-format never gates on a .markdownlint-cli2.jsonc existing; it always runs markdownlint-cli2 --fix, config or not.

Net effect: for any consumer repo that hasn't already hand-authored a typos config (i.e. most repos on day one), installing this plugin today does nothing — no auto-fix, no advisory, completely inert — until they write a _typos.toml first. That inverts the "auto-fix on edit, zero-config" value proposition the epic's PLAN.md brief called for.

Suggested fix (not filing a new issue per the review-deferral note — flagging here since the PR is merged): drop the opt-in walk/gate entirely; always run typos --force-exclude -w --format json <file> unconditionally (typos' own file-anchored config discovery already picks up an optional _typos.toml if one exists, for allowlist/exclude purposes only). Happy to open a follow-up PR if that's preferred over amending this one directly.

kyle-sexton added a commit that referenced this pull request Jul 21, 2026
## Summary

`plugins/ruff-format/hooks/ruff-format.sh` emits telemetry via
`hook::emit_telemetry("ruff-format", ...)` but never shipped the
hook-telemetry convention's per-hook data schema or Implementers table
row, unlike the `markdown-format` and `typos-format` producers. Consumer
sinks discovering the `ruff-format` `hook` value had no published `data`
schema to validate against.

## Fix

- Added `docs/conventions/hook-telemetry/data/ruff-format.schema.json`,
mirroring `markdown-format.schema.json`'s shape (`tool`, `file`,
`findings: string[]`) — `ruff-format.sh` builds `data.findings` as an
array of concise diagnostic lines from `ruff check --no-fix
--output-format concise`, the same shape as markdown-format's lint-line
findings (not typos-format's structured `{typo, corrections}` objects).
- Added the `ruff-format` row to the Implementers table in
`docs/conventions/hook-telemetry/README.md`.
- Bumped `plugins/ruff-format/.claude-plugin/plugin.json` to `0.4.3`
(patch — conformance fix, no hook behavior change) with a matching
`CHANGELOG.md` entry.

Followed the same pattern as `typos-format`'s schema+registry-row
addition (#872): no `hook-telemetry/CHANGELOG.md` entry, since per-hook
`data` schemas are not separately version-stamped (README "Versioning").

## Verification

- `jq . docs/conventions/hook-telemetry/data/ruff-format.schema.json`
and `jq . plugins/ruff-format/.claude-plugin/plugin.json` — both valid
JSON.
- Re-fetched `origin/main` immediately before opening this PR and
confirmed `plugins/ruff-format/.claude-plugin/plugin.json` was still at
`0.4.2` (no collision with the version bump).
- Searched `scripts/run-plugin-tests.sh` and
`scripts/aggregate-hygiene-results.sh` for an automated
Implementers-table/schema-file consistency check — none exists; this gap
is exactly what issue #874 is about, so verification here is manual:
diffed against `markdown-format.schema.json` and
`typos-format.schema.json` field-by-field, and cross-checked
`data.findings`'s shape against the actual `jq` construction in
`ruff-format.sh`'s `build_data_json`.
- `gh pr list --state open` showed no other open PR touching
`plugins/ruff-format/` or `docs/conventions/hook-telemetry/` — no
collision risk.

## Related

- Reference templates: `markdown-format` plugin's
`data/markdown-format.schema.json` + registry row, and `typos-format`
plugin's `data/typos-format.schema.json` + registry row (#872).
- Epic: #830 (`docs/topics/lint-static-analysis-gaps/PLAN.md`).

Closes #874

Co-authored-by: Claude Sonnet 5 <noreply@anthropic.com>
kyle-sexton added a commit that referenced this pull request Jul 21, 2026
## 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
— `recieve` → `receive`.

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

Closes #884

## Related

- #872 — the merged PR that shipped the opt-in gate this PR removes.
- #876 — covered a TOML inline-table detection gap in both
`typos-format`'s and `ruff-format`'s opt-in-gate grep. #896 already
closed it for the `ruff-format` half (documented as a known limitation
there). The `typos-format` half is now moot: this PR deletes the opt-in
gate and its grep entirely, so there is no gate left for that detection
gap to affect.

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

Co-authored-by: Claude Sonnet 5 <noreply@anthropic.com>
kyle-sexton added a commit that referenced this pull request Jul 22, 2026
…#910)

## Summary

Two deliverables per issue #832's "Go coverage, both lanes":

1. **`plugins/toolchain/reference/ecosystems/go.yaml`** batch ecosystem
default (+ matching
`docs/conventions/ecosystem-commands/examples/go.yaml` fixture) — `go
build ./...`/`go test ./...` unconditional; `golangci-lint run [--fix]
./...` gated behind an `opt-in` key
(`.golangci.yml`/`.golangci.yaml`/`.golangci.toml`/`.golangci.json`
presence); `project-discovery: ["go.mod"]` for nested-module coverage; a
`go-mod-tidy-drift` gate via `go mod tidy -diff`.
2. **New hook plugin `plugins/go-format/`** — runs `goimports -w` on
every `.go` `Write`/`Edit`, **unconditionally** (no consumer-config
opt-in gate — the one deliberate shape difference from sibling
`ruff-format`/`typos-format`), skipping files carrying Go's `// Code
generated ... DO NOT EDIT.` marker.

## Design decisions

Issue #832 calls for an "implementation-time field survey" to pick the
per-file formatter (gofmt/goimports/gofumpt/golangci-lint fmt) against
three criteria: official/authoritative, maintained, feature-fit.

- **`goimports`** picked: its own docs state it "formats your code in
the same style as gofmt so it can be used as a replacement for your
editor's gofmt-on-save hook" — a direct official statement of intent for
this exact per-file-hook scenario. Actively maintained (golang/tools,
v0.48.0). Handles import add/remove, which bare `gofmt` never does —
necessary since an LLM edit changing symbol usage can leave a broken
import list.
- **`gofumpt`** rejected as the unconditional default: third-party
opinionated superset ("a stricter gofmt"), same risk class as why
`ruff-format`/`dotnet format` gate behind consumer config. No toggle
added in v1 (no consumer has asked; can be added later if requested).
- **`golangci-lint fmt`** rejected for the per-file hook: with no config
present it has zero formatters enabled by default (does nothing) —
corroborates the issue's own "golangci-lint is batch-only by design"
framing, for the correct reason (verified via the tool's own behavior,
not just restated from the brief).
- **`go-format` runs unconditionally** — `goimports` has no meaningful
config-divergence axis when left unconfigured (unlike
ruff/dotnet-format, whose underlying tools genuinely diverge by config),
so gating it would be inert ceremony.
- **`golangci-lint`'s lint step IS gated** — empirically verified
golangci-lint v2 with no config file still applies its own fixed
"standard" linter preset unconditionally, the same
imposed-unconfigured-opinion risk PR #890 (issue #835) fixed for `dotnet
format`. Mirrors the existing `python.yaml`/`dotnet.yaml` `opt-in`
pattern.

Full rationale, stress-test findings, and empirical verification notes:
`docs/topics/832-go-ecosystem/PLAN.md` (pasted below).

## Review history

Two independent fresh-context passes ran before this PR, findings folded
in:

**Plan stress-test** (before implementation) — one HIGH finding: the
"goimports has no config-divergence axis" premise missed that
`goimports` has zero awareness of Go's generated-file convention, and
would silently rewrite generated files. Fixed by adding a marker-skip
guard to the hook before implementation began. Also found a MEDIUM
multi-module gap (`./...` doesn't cross a nested `go.mod` boundary —
fixed by adding `project-discovery: ["go.mod"]`) and corrected two
citation errors (a misattributed `golangci-lint fmt` quote; an
inaccurate "every ecosystem has a matching example fixture" claim).

**Independent code review** (after implementation, before PR) — one
CRITICAL finding: the generated-file guard checked only the file's
*first non-blank line*, missing the common real-world shape where a
copyright/license header (`addlicense`/`goheader`-style tooling)
precedes the marker by a few `//` comment lines — reviewer empirically
reproduced the miss against a real generated-file shape and confirmed
the hook silently rewrote it. Fixed: the guard now scans the file's full
leading comment/blank-line run per Go's own stated convention ("before
the first non-comment, non-blank text in the file"), and two related
defeat vectors caught by the same review (trailing CRLF, leading UTF-8
BOM) are also fixed. Five new regression cases added. Two
SUGGESTION-level findings (an unnecessary `mktemp` for stderr capture;
an inaccurate PLAN.md claim about a nonexistent schema-check script)
were also fixed.

## Test plan

- [x] `plugins/go-format/hooks/go-format.test.sh` — 41/41 passing
(FAKEBIN-pattern contract test, real `goimports` v0.48.0 binary),
including 5 generated-file-guard regression cases (bare marker,
license-header preamble, CRLF, BOM, marker-after-leading-block negative
case).
- [x] `check-jsonschema` against `ecosystem.schema.json` — both
`go.yaml` files (bundled default + example fixture) validate; against
the plugin manifest schema for
`plugins/go-format/.claude-plugin/plugin.json`.
- [x] `scripts/check-changed-skills.sh` / direct `check-skill.sh`
invocation — `go-format:setup`, `toolchain:check`, `toolchain:lint` all
PASS.
- [x] `scripts/sync-hook-utils.sh --check`,
`scripts/check-cross-plugin-source-drift.sh`,
`scripts/check-silent-skips.sh`, `scripts/check-changelog-parity.sh
--check`, `node scripts/validate-plugin-contracts.mjs`, `bash
scripts/validate-plugins.sh` (strict catalog validation) — all pass.
- [x] `typos`, `markdownlint-cli2`, `shellcheck` on all new/changed
files — clean.
- [x] `node scripts/generate-catalog.mjs --check` — in sync.
- [x] Manually verified `go mod tidy -diff` and golangci-lint's
unconfigured-defaults behavior against real installed toolchains (Go
1.26.5, golangci-lint v2) — see PLAN.md.
- [x] Plan stress-test + independent code review — findings above, both
fixed and re-verified.

## Related

- Closes #832 (epic #830, sub-item 2 of
`docs/topics/lint-static-analysis-gaps/PLAN.md`, contract landed via PR
#829).

<details>
<summary>PLAN.md (implementation plan, decisions, and review
history)</summary>

## Brief

Issue #832 (epic #830, sub-item 2 of
`docs/topics/lint-static-analysis-gaps/PLAN.md` lines 26-30). Two
deliverables, one PR:

1. New `plugins/toolchain/reference/ecosystems/go.yaml` batch ecosystem
entry + matching
   `docs/conventions/ecosystem-commands/examples/go.yaml` fixture.
2. New hook plugin `plugins/go-format/` — per-file `goimports` autofix
on Write/Edit.

Scope boundaries: batch additions are rung-4 defaults only (consumer
`.claude/ecosystems/*.yaml`
override ladder unchanged). `govulncheck` is explicitly "optional" per
the brief — NOT shipped as a
default; documented as a consumer-addable local gate only. No `gofumpt`
toggle in v1 (YAGNI — no
consumer has asked).

Success criteria: both new surfaces pass the plugin contract gate +
fleet conformance audit
(acceptance criteria line 61 of the epic Brief); CI/local parity
restored for the Go toolchain gap
identified 2026-07-21 (line 66-67).

## Open Decisions (resolved this session, recorded here for the approval
gate)

1. **Formatter pick for the per-file hook: `goimports`.** Field survey
against the Brief's own
criteria (official/authoritative, maintained, feature-fit), researched
fresh this session:
- `gofmt` — non-configurable by design (go.dev/blog/gofmt,
go.dev/doc/effective_go), but doesn't
manage imports; an LLM edit that adds/removes symbol usage leaves a
broken file.
- `goimports` (golang.org/x/tools, v0.48.0, 2026-07-09; ~7,983 commits,
last updated
2026-07-20) — its own docs state it "formats your code in the same style
as gofmt so it can be
used as a replacement for your editor's gofmt-on-save hook." Direct
official statement of
     intent for exactly this scenario. **Picked.**
- `gofumpt` (mvdan/gofumpt v0.10.0, 2026-05-04) — self-branded "a
stricter gofmt," third-party
opinionated superset. Rejected as the unconditional default (same class
of risk as why
ruff-format/dotnet-format gate behind consumer config); not adding a
toggle in v1.
- `golangci-lint fmt` — rejected for the per-file hook. **Stress-test
correction:** the plan's
original citation was imprecise — `golangci-lint fmt --stdin` IS a
genuine single-file,
non-package-scoped invocation (empirically confirmed: no `go/packages`
loading, formats piped
stdin instantly). The "directories are NOT analyzed recursively... files
must come from the
same package" quote governs `golangci-lint run` (linters), not `fmt`
(formatters) — don't
misattribute it. The correct, empirically-confirmed rejection reason:
with no config file
present, `golangci-lint fmt` has **zero formatters enabled by default**
(unlike `run`'s fixed
"standard" 5-linter preset) — i.e. it silently does nothing, the
opposite of a usable default.
Conclusion (rejected for the hook) is unchanged; only the stated reason
is corrected.
Independently corroborates the Brief's own line 30 ("golangci-lint is
batch-only by design")
     for the `run`/lint half of the tool.
- **Consequence: `go-format` runs `goimports` unconditionally — no
consumer-config opt-in gate.**
This is the one deliberate shape difference from
ruff-format/typos-format/dotnet-format's
opt-in-gated pattern. Rationale: goimports has no meaningful
config-divergence axis when left
     unconfigured, so gating it would be inert ceremony. Qualifies for
`docs/PLUGIN-PHILOSOPHY.md` lines 143-147 lane-1 treatment
(non-conflicting good-practice
     default) where gofumpt/golangci-lint-fmt would not.
- **Stress-test correction (HIGH finding, folded in):** the "no
config-divergence axis" claim
was incomplete — empirically confirmed goimports rewrites files carrying
the canonical
`// Code generated ... DO NOT EDIT.` marker with zero awareness of that
convention, while
golangci-lint's own linters/formatters default to
`issues.exclude-generated: strict` in v2.
Generated Go files (protobuf, mockgen, sqlc, stringer, wire output) are
common; an
unconditional hook would silently rewrite them on any edit. **Fix, not a
re-open of the
unconditional-default decision:** `go-format.sh` adds a generated-file
marker guard — skip
(emit_skipped) when the marker appears anywhere in the file's leading
comment/blank-line run (scanning stops at the first line that is neither
blank nor a `//`
comment), matching Go's own stated convention ("before the first
non-comment, non-blank text
in the file"), not just the first non-blank line. **Independent-review
correction (CRITICAL,
folded in post-stress-test):** the original implementation checked ONLY
the first non-blank
line, on the premise that a preamble before the marker is "uncommon" —
that premise was
false; a license/copyright header (common `addlicense`/`goheader`
tooling output) routinely
precedes the marker by several `//` lines, and this shape is empirically
present in real Go
stdlib-adjacent generated files. Fixed by scanning the full leading
comment/blank block
instead of only line one; also fixed two related defeat vectors caught
by the same review
(trailing CRLF `\r` not stripped before the `$` anchor; a leading UTF-8
BOM defeating the `^`
anchor) and added five regression test cases (license-header preamble,
CRLF, BOM, and a
negative case confirming a marker appearing AFTER the leading block does
NOT suppress a real
edit). This is precision-scoping (same category as `--force-exclude`
giving Ruff per-file skip
     precision), not a consumer-config
walk-up, so it does not undermine Open Decision 1's "unconditional"
framing.

2. **`golangci-lint` lint step gated behind an `opt-in` key** requiring
`.golangci.yml`/`.golangci.yaml`/`.golangci.toml`/`.golangci.json`
present (ancestor walk to
repo root, mirroring `dotnet.yaml`'s `root = true` boundary if an
analogous concept exists for
golangci-lint — verified during Phase 1 to NOT exist, so ceiling at repo
root only). Verified
via WebFetch/WebSearch this session
(golangci-lint.run/docs/configuration,
github.com/golangci/golangci-lint discussions, 2026): golangci-lint v2
with **no config file
present still runs its own fixed "standard" linter preset**
(`linters.default: standard`)
rather than erroring or running nothing — the same
imposed-unconfigured-opinion risk class
PR #890 (issue #835) just fixed for `dotnet format`'s unconfigured
Roslyn defaults, and
consistent with why `python.yaml` already gates ruff behind
`[tool.ruff]`/`ruff.toml` presence.
Mirrors `python.yaml:10` and `dotnet.yaml:34`'s `opt-in` key shape
exactly.

3. **Single PR, closes #832.** Both deliverables are one Brief line ("Go
coverage, both lanes"),
share the same formatter research, and match the one-issue-one-PR
precedent from #831/#835.

## Plan

### Phase 1: `go.yaml` ecosystem batch entry [DONE]

Files:

- `plugins/toolchain/reference/ecosystems/go.yaml` (new) — flat
top-level keys, bundled-fallback
  disclaimer header (mirrors `dotnet.yaml:1-27` style):
  - `globs: ["*.go", "go.mod", "go.sum"]`
- `project-discovery: ["go.mod"]` — **added per stress-test MEDIUM
finding**: empirically
confirmed `go build ./...`/`go test ./...`/`go list ./...` run from a
repo root silently skip
a nested module's packages (a nested `go.mod` bounds `./...` expansion;
a root `go.work` file
does NOT cross module boundaries for this either). Without
`project-discovery`, a monorepo with
a nested Go module gets silent incomplete build/test/lint, directly
undercutting this issue's
own "CI/local parity" success criterion. Mirrors
`python.yaml`/`typescript.yaml`'s existing
    `project-discovery` handling for the same class of problem.
  - `build-cmd: "go build ./..."`
  - `test-cmd: "go test ./..."`
  - `check-cmd: "golangci-lint run ./..."`
  - `fix-cmd: "golangci-lint run --fix ./..."`
  - `opt-in`: text per Open Decision 2 above.
- `install-hint`: **resolved per stress-test finding** — do NOT bake in
a pinned
`curl|sh -s -- -b ... vX.Y.Z` one-liner (upstream's own install docs
explicitly state
`go install`/`go get` "aren't guaranteed to work" for golangci-lint, and
a version-pinned
    curl\|sh command drifts immediately). Use a durable pointer instead:
`"Install golangci-lint: https://golangci-lint.run/docs/welcome/install/
| Go toolchain:
    https://go.dev/dl/"` — same durable-pointer style as `dotnet.yaml`'s
    `"Install .NET SDK from https://dot.net"`.
- `gates`: one entry, `go-mod-tidy-drift`, `trigger-globs: ["go.mod",
"go.sum"]`, `cmd:
"go mod tidy -diff"` — **confirmed live** via `go help mod tidy` (Go
1.26.5 installed this
session): "-diff causes tidy not to modify go.mod or go.sum but instead
print the necessary
changes as a unified diff. It exits with a non-zero code if the diff is
not empty." Introduced
Go 1.23 (2024) — note this as an implicit minimum-Go-version
prerequisite in `context/go.md`.
`remediation: "Run go mod tidy and commit the updated go.mod/go.sum."`
- `notes`: mention `govulncheck` as an available consumer-addable local
gate (not shipped by
default per Open Decision brief-wording), pointing at
`.claude/ecosystems/go.local.yaml`.
- `docs/conventions/ecosystem-commands/examples/go.yaml` (new) — richer
worked-example fixture
mirroring `examples/dotnet.yaml`'s structure (same keys as above plus
the `gates` block spelled
out). **Correction (stress-test finding):** only 3 of the 8 existing
bundled ecosystem yamls
(`bash`, `python`, `dotnet`) actually have a matching example fixture —
`markdown`, `powershell`,
`typescript`, `cross-cutting`, `yaml` do not, and no CI gate enforces
1:1 coverage. Add
`examples/go.yaml` anyway (it's good practice and dotnet/python both
have one), but don't claim
in the PR body that this closes a universal-coverage gap — it doesn't
exist as a gap.
- `plugins/toolchain/skills/check/context/go.md` (new) — Go-specific
gotchas: `go test ./...`
module-root requirement (must run from the module root or a path
containing `go.mod`;
`project-discovery` above handles nested-module walking), GOFLAGS
interactions,
golangci-lint's default-"standard"-preset caveat tied to the opt-in
gate, AND (stress-test
MEDIUM finding) golangci-lint's own config discovery falls back to the
user's **home directory**
with no `root = true`-equivalent stop marker when no repo-level config
is found — a stray
`~/.golangci.yml` on a developer's machine makes local runs diverge from
a clean CI container;
document this as a known local/CI divergence source. Mirrors the
existing
  `context/dotnet.md`/`context/python.md` shape.
- `plugins/toolchain/skills/check/SKILL.md` — add `go` to the
covered-ecosystems list (currently:
dotnet, python, typescript, bash, powershell, markdown) and its alias
table if one applies (no
  common alias needed — "go" is already short).
- `plugins/toolchain/skills/lint/SKILL.md` — same covered-ecosystems
list addition if `lint` also
enumerates them explicitly (verify during implementation; python's
opt-in-gated lint precedent
from #835 touched both `check/SKILL.md` and `lint/SKILL.md`, so `go`
likely needs the same
  two-file touch).
- `plugins/toolchain/.claude-plugin/plugin.json` — version bump (minor:
new ecosystem capability).
**Verify current version live** (`git show
origin/main:plugins/toolchain/.claude-plugin/plugin.json`
at rebase time — do not assume 0.6.0 is still current, #833/#834 may
have already bumped it).
- `plugins/toolchain/CHANGELOG.md` — `[Unreleased]`/new version entry
under Added.

**Sanity Check:** `check-jsonschema --schemafile
docs/conventions/ecosystem-commands/ecosystem.schema.json
<file>` passes for both `plugins/toolchain/reference/ecosystems/go.yaml`
and
`docs/conventions/ecosystem-commands/examples/go.yaml`.
**Independent-review correction:** no CI
job or repo script actually validates
`reference/ecosystems/*.yaml`/`examples/*.yaml` against this
schema today (confirmed by grepping `.github/workflows/ci.yml` — its
four `check-jsonschema` steps
cover only marketplace/plugin manifests, dependabot, and workflow
files); the check above is a
manual `check-jsonschema` CLI run, not an existing repo script being
reused. Both files validated
clean this way. This is a pre-existing gap in the repo's own CI
coverage, out of scope for this
issue — noted here rather than silently left as an inaccurate claim.

### Phase 2: `plugins/go-format/` hook plugin [DONE]

Files (full new plugin directory, mirroring `plugins/typos-format/`
structure):

- `.claude-plugin/plugin.json` — `userConfig.go_format_enabled`
(boolean, default `true`), version
  `0.1.0`, keywords `["go","golang","goimports","formatter","hook"]`.
- `hooks/hooks.json` — `PostToolUse`, matcher `Write|Edit`, command
  `"${CLAUDE_PLUGIN_ROOT}"/hooks/go-format.sh`, timeout 15.
- `hooks/hook-utils.sh` — initial copy via `scripts/sync-hook-utils.sh`
(never hand-copy).
- `hooks/go-format.sh` — control flow mirrors `ruff-format.sh`'s shape:
- Extension pre-filter on `*.go` (jq-free, before requiring jq) — like
ruff-format, unlike
    typos-format's no-filter shape.
- `hook::check_enabled "GO_FORMAT"`, `hook::buffer_stdin`,
`hook::require_jq`,
    `hook::read_file_path`, `hook::repo_root`.
- **No ancestor consumer-config walk-up** (goimports is unconditional
per Open Decision 1) —
document this simplification explicitly in a short header comment
referencing this PLAN's
rationale, not just silently omitting the walk-up
ruff-format/typos-format both have.
- Binary resolution: PATH-only (`command -v goimports`) — no
`.venv`-style walk (Go has no
per-project virtualenv concept; mirrors typos-format's PATH-only
resolution).
- Invocation: **resolved via empirical stress-test verification (real
goimports v0.48.0
binary):** `goimports -w -l "$FILE"` in one pass — `-w` writes the fix,
`-l` (list-only)
combined with `-w` still lists the filename if changes were needed,
giving a single-pass
fix+detect (simpler than ruff-format's two-pass shape). Exit-code
semantics confirmed: **`-l`
ALWAYS exits 0**, even when it lists a file needing changes — there is
no exit-1-style
"findings" signal like ruff/typos have. Detect "changes were made" from
**non-empty stdout**
(the listed filename), not from exit code. Non-zero exit (confirmed:
exit 2, parseable message
on **stderr**) occurs only on a genuine parse/syntax error — surface
that as a finding-text
message (mirrors how ruff-format surfaces a mid-edit syntax error as a
finding, not a
tool-break), matching the doctrine even though the underlying signal
shape differs from ruff.
Skip entirely (before invoking goimports) when the file matches the
generated-file marker guard
    from Open Decision 1's stress-test correction above.
- Telemetry: `hook::emit_telemetry "go-format" "PostToolUse" <status>
"$start" "$data_json"
"$REPO_ROOT"`; `status` semantics per the shared doctrine (`ok` = ran to
judgment, `skipped` =
    tool broke/missing prerequisite).
  - Always exits 0 (advisory only).
- `hooks/go-format.test.sh` — FAKEBIN-pattern contract test mirroring
`ruff-format.test.sh`/`typos-format.test.sh`: gate-off, non-`.go`
extension skip, clean file,
import-added-by-edit auto-added, import-removed-by-edit auto-removed,
**generated-file marker
skip** (new case per the Open Decision 1 stress-test correction),
missing-binary dim-9 visibility
(once-per-session), missing-jq dim-9 visibility, kill-switch, telemetry
envelope shape assertions

(`schema_version`/`timestamp`/`hook`/`hook_event`/`status`/`duration_ms`/`data`),
a
syntax-error-mid-edit case surfaced as a finding (per the confirmed `-l`
exit-2/stderr behavior
  above, not a tool-break).
- `skills/setup/SKILL.md` — `check`/`apply`, `disable-model-invocation:
true`. `check` probes Bash,
jq, `goimports` on PATH, the `go_format_enabled` toggle, hook
registration. `apply`: **resolved
per stress-test finding — guidance-only, no write path**, matching
`typos-format`'s pattern
(`plugins/typos-format/skills/setup/SKILL.md:57-64`), not
`ruff-format`'s `.venv`-install path.
`go install golang.org/x/tools/cmd/goimports@latest` writes to the
machine-global
`$GOBIN`/`$GOPATH/bin` (not project-scoped) and `@latest` is not
idempotent-pinned (silently
drifts over time) — structurally the same "no per-repo
dependency-manager, machine-level binary"
case as typos-format, not ruff-format's project-`.venv` case. Document
the goimports install
pointer (`go install golang.org/x/tools/cmd/goimports@latest` — a plain,
uncaveated `go install`,
unlike golangci-lint's own install docs which explicitly warn `go
install`/`go get` "aren't
guaranteed to work" for that tool specifically — don't let that caveat
bleed across into this
  SKILL.md's goimports guidance).
- `README.md` — mirror typos-format's structure; explicitly document the
"no config-gate,
unconditional default" design choice (the one plugin in the family
without an opt-in section) —
  say so plainly, don't silently omit the section.
- `CHANGELOG.md` — `[0.1.0]` initial release entry, telemetry-conformant
from day one (matches
  typos-format's precedent, not ruff-format's later-follow-up pattern).
- `docs/conventions/hook-telemetry/data/go-format.schema.json` (new) —
findings shape decided from
Phase 2's live verification of goimports' actual diagnostic output
(flat-string per ruff-format's
shape unless goimports emits something structured — verify, don't
assume).
- `docs/conventions/hook-telemetry/README.md` — add Implementers table
row for `go-format`.
- `.claude-plugin/marketplace.json` — new entry: `category:
"development"`,
  `tags: ["go","golang","goimports","formatter","hook"]`,
`relevance: {topic: "Go", signals: {filesRead: ["**/*.go"], cli:
["goimports"]}}` (mirrors
  ruff-format/typos-format's relevance-block shape exactly).
- `README.md` (repo root) — regenerate catalog block via `node
scripts/generate-catalog.mjs`.

**Sanity Check:** `plugins/go-format/hooks/go-format.test.sh` passes
100% when run directly
(`bash plugins/go-format/hooks/go-format.test.sh`), and
`scripts/sync-hook-utils.sh --check`
reports no drift for the new copy.

### Phase 3: Cross-cutting verification + PR [DOING]

- Rebase onto latest `origin/main` (expect a benign conflict on
`plugins/toolchain/.claude-plugin/plugin.json` +
`plugins/toolchain/CHANGELOG.md` against
#833/#834's already-merged or still-in-flight changes — resolve by
reapplying this lane's version
  bump on top of theirs, not by discarding either).
- Run the full local CI-equivalent gate set: hygiene
(schema/markdownlint/typos/shellcheck/exec-bit),
hook-utils-sync, cross-plugin-source-drift, silent-skip-gate,
changelog-parity-gate,
skill-quality-gate + portability-lint, plugin-gate
(`scripts/validate-plugin-contracts.mjs` +
  both new/changed test suites), `scripts/generate-catalog.mjs --check`.
- Mandatory fresh-context independent code review (per this session's
established discipline).
- `gh pr create` — closes #832, body includes the two locked Open
Decisions above (formatter pick +
opt-in-gate rationale) so reviewers see the reasoning, not just the
diff.
- Monitor CI, process every review thread to resolution (GraphQL
`resolveReviewThread` for
  bot-authored addressed threads — this repo's ruleset requires
`required_review_thread_resolution`), merge, remove worktree, delete
branch, confirm issue
  auto-closed.
- `/planning:plan close-out`: paste this PLAN.md into the PR body
`<details>` block, prune
  `docs/topics/832-go-ecosystem/` before merge.

**Sanity Check:** `gh pr view <N> --json state -q .state` returns
`MERGED`; `gh issue view 832
--json state -q .state` returns `CLOSED`.

## Review history

An independent fresh-context code review ran before PR creation and
found one CRITICAL and two
lower-severity items, all addressed and re-verified before opening the
PR:

1. **CRITICAL — generated-file guard only checked the first non-blank
line, missing the common
license-header-then-marker layout** (and two related defeat vectors:
CRLF, UTF-8 BOM).
Reviewer empirically reproduced the miss against a real generated-file
shape (a copyright
header plus a `stringer`-style marker) and confirmed the hook silently
rewrote it. Fixed: the guard now
scans the file's full leading comment/blank-line run (stopping at the
first non-comment,
non-blank line) per Go's own stated convention, strips a trailing CRLF
and leading BOM per
line before matching. Five new regression cases added (license-header
preamble, CRLF, BOM,
and a negative case proving a marker appearing after the leading block
does NOT suppress a
   real edit) — see Open Decision 1 above for the full before/after.
2. **SUGGESTION — stderr capture used an unnecessary `mktemp` file**,
inconsistent with every
other hook in the repo's simpler command-substitution idiom. Simplified
to match.
3. **SUGGESTION — this PLAN's own Phase 1 Sanity Check claimed an
existing repo script validates
ecosystem yaml against `ecosystem.schema.json`; no such CI job or script
exists.** Corrected the
claim (see Phase 1's Sanity Check above) — the schema validation itself
was and remains correct
(verified manually via `check-jsonschema`), only the "reuse an existing
script" framing was
wrong. This is a pre-existing repo-wide CI-coverage gap, out of scope
here.

**On-PR review round** (`claude-review`, `security-review`, and Codex,
both pushes) surfaced eight
more findings. Six confirmed and fixed, two evaluated and consciously
left as-is:

1. **CRITICAL-equivalent (Codex P2, independently confirmed via `go help
generate` live) —
the generated-file guard still missed a `/* ... */` block-comment
preamble.** The authoritative
convention text ("This line must appear before the first non-comment,
non-blank text in the
file") does not restrict "comment" to `//` style; the guard only treated
`//` lines as
comment-continuation, so a block-comment license header (e.g. `/*
Copyright ... */` before
`// Code generated`) caused premature loop termination — the same
failure class as the
pre-PR CRITICAL finding, just a different comment syntax. **Not accepted
as "zero practical
risk"** (one review pass argued this) — fixed properly: the guard now
tracks open `/* */`
blocks and continues scanning through them. New regression case (5f)
added.
2. **Real correctness gap (Codex P2) — `goimports` ran with no `-local`
grouping prefix,** so a
repo already formatting with `-local` (a common Go convention wired into
CI/editor config)
would have every edit re-collapse its local-import grouping back into
the third-party group —
empirically confirmed this materially changes output (verified with a
real third-party import
present). This directly undercut Open Decision 1's "no config-divergence
axis" premise a
second time. Fixed: the hook now derives `-local` from the edited file's
own module path
(`go list -m`, walks to the nearest `go.mod` via Go's own resolution)
when a `go` toolchain is
present — zero new consumer-config surface, covers the single most
common `-local` use case
(self-grouping), gracefully degrades to goimports' plain default when
`go` is absent or the
   file isn't in a resolvable module. New regression case (4b) added.
3. **Security SUGGESTION (both security-review passes) — missing `--`
end-of-flags separator
before `$FILE`.** Low-confidence defense-in-depth; fixed alongside the
`-local` change.
4. **Codex P2 — `go-mod-tidy-drift` gate only triggered on
`go.mod`/`go.sum`, missing source-only
tidy drift** (e.g. removing the last usage of a dependency leaves
`go.mod` over-declared while
`go build`/`go test` still pass). One review pass called this an
acceptable tradeoff citing the
`python.yaml` precedent; **not accepted** — the epic's own stated
success criterion is
"CI/local parity," and leaving this gap directly contradicts it. Fixed:
`trigger-globs` widened
to include `*.go`. Remediation text also now notes the Go 1.23+ floor
for `go mod tidy -diff`
   (a separate LOW finding from the first review pass).
5. **Codex P2 — `/toolchain:lint`'s "Per-project walking" list
enumerated python/typescript only,
omitting `go`** despite `go.yaml` declaring `project-discovery:
["go.mod"]` — a monorepo with a
nested Go module would have `/toolchain:lint go` run `golangci-lint run
./...` from the wrong
   root. Fixed: added a `go` bullet.
6. **LOW — README described the generated-file guard as checking only
the "first non-blank line,"**
stale after the pre-PR CRITICAL fix. Corrected to describe the actual
leading-block scan.
7. **COSMETIC, evaluated and left as-is — BOM-stripping runs on every
loop line, not just the
first.** A UTF-8 BOM can only appear at byte 0, so this is a harmless
no-op after line one, not
a bug. Left unchanged (the fix would add branching complexity for zero
behavioral gain).
8. **COSMETIC — key ordering differed between the bundled `go.yaml`
(`gates` before
`install-hint`) and the example fixture (`install-hint` before
`gates`).** Aligned the bundled
file to the example's ordering (which matches the `dotnet.yaml` example
precedent).

## Blast radius

**MEDIUM.** New plugin + new ecosystem entry, but both are close
pattern-replications of two
already-merged, already-reviewed precedents (typos-format PR #872,
dotnet opt-in gate PR #890) in
this same epic. The two genuine judgment calls (goimports-unconditional,
golangci-lint-opt-in) are
each backed by fresh primary-source research and a direct analogy to an
already-accepted precedent
in this repo — not novel territory. No cross-module architectural
change, no data-model change, no
multi-tenant concern. Several implementation-time facts (exact goimports
flags, exact golangci-lint
install command, `go mod tidy` dry-run flag syntax) are flagged as
**verify-live, don't assume** —
this is where a stress-test/implementation-time slip is most likely, not
in the two locked design
decisions.

## Stress-test summary

Blast radius MEDIUM — no CRITICAL/HIGH triggers (no cross-module
integration, no data-model change,
no multi-tenant posture) per `context/stress-test-triggers.md`'s
criteria, but the two load-bearing
judgment calls (Open Decisions 1 and 2) warrant the mandatory Step 3
fresh-context plan-reviewer
pass before implementation, specifically pressure-testing: (a) is
"goimports unconditional, no
opt-in" actually safe, or does goimports have a config-divergence axis
this session's research
missed; (b) is the golangci-lint opt-in gate correctly scoped (does it
match how `check-cmd`
composes with `fix-cmd` the same way python/dotnet's gates do, and is
the ancestor-walk boundary
choice — repo-root-only, no `root = true`-equivalent — actually correct
for golangci-lint's own
config discovery, which may differ from EditorConfig's semantics).

*(Dispatched separately as the mandatory Step 3 fresh-context
plan-reviewer sub-agent — findings
folded in before implementation begins.)*

## Execution shape

Sequential — Phase 1 (ecosystem yaml) and Phase 2 (hook plugin) touch
disjoint files and share no
data dependency (Phase 2 doesn't consume Phase 1's output), so they are
parallel-safe by the
file-overlap test, but the combined scope is small enough (~14 files
total) that the coordination
overhead of a two-agent split isn't worth it for a MEDIUM-blast-radius,
largely-mechanical
replication lane. Single main-session sequential execution, Phase 1 then
Phase 2, then Phase 3
cross-cutting verification gates both. Phase 3 is fully
sequential-dependent on both.

## Open questions

None blocking — all flagged "verify live during implementation" items
are execution-time fact
lookups (exact CLI flags/install commands), not design decisions
requiring further user input.

## Handoff to implementation

### User-approval gates

None beyond initial plan approval — no `[FALLBACK]` tags, no
scope-expansion proposals anticipated.
If live verification during Phase 1/2 surfaces that `goimports` or
`golangci-lint` behave
materially differently than researched (e.g., goimports turns out to
have a real config-divergence
axis), STOP and re-open Open Decision 1 rather than silently proceeding.

### Execution shape (`[EXEC-SHAPE]` tagged)

- `[EXEC-SHAPE]` Single PR bundling both deliverables (Open Decision 3).
- `[EXEC-SHAPE]` Sequential single-main-session execution (Execution
shape section above).
- `[EXEC-SHAPE]` `go-format` hook plugin structural simplification (no
ancestor config walk-up) —
  Open Decision 1's consequence.

### Mechanical work

Commit boundaries: one commit per phase is reasonable (Phase 1, Phase 2,
then fixup commits for
review findings) but not mandated — squash-merge means the final PR
history is one commit anyway.
Verification checkpoints: run the full local gate set after each phase,
not just once at the end,
to catch cross-phase interactions (e.g., `plugin.json`/`CHANGELOG.md`
version-bump conflicts
between the toolchain-plugin edit in Phase 1 and any repo-root catalog
regen in Phase 2) early.
Sequential fallback: N/A (already sequential).

</details>

🤖 Generated with [Claude Code](https://claude.com/claude-code)

---------

Co-authored-by: Claude Sonnet 5 <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

feat(typos-format): per-file typos autofix hook plugin

1 participant