Skip to content

feat(toolchain): dotnet opt-in gate on .editorconfig, visible skip status#890

Merged
kyle-sexton merged 3 commits into
mainfrom
feat/835-dotnet-batch-guard
Jul 21, 2026
Merged

feat(toolchain): dotnet opt-in gate on .editorconfig, visible skip status#890
kyle-sexton merged 3 commits into
mainfrom
feat/835-dotnet-batch-guard

Conversation

@kyle-sexton

Copy link
Copy Markdown
Contributor

Summary

Adds an opt-in key to the dotnet ecosystem entry — the one lint-bearing ecosystem (of 8: dotnet, python, typescript, bash, markdown, powershell, yaml, cross-cutting) that lacked one. dotnet format's own docs state preferences are read from .editorconfig "if present, otherwise a default set of preferences will be used"; verified empirically (dotnet SDK 10.0.302) that a project with no .editorconfig still fails dotnet format --verify-no-changes against Roslyn's built-in conventions. Without a gate, this imposes formatting opinions on a repo that never configured any — the same class of violation ruff-format/typos-format already avoid at the hook layer.

  • check/SKILL.md now honors opt-in for the lint phase — it never did before, so dotnet's check-cmd ran unconditionally whenever .cs/.csproj/etc. changed. Build and test are unaffected; only the lint phase is gated.
  • Both check/SKILL.md and lint/SKILL.md make an opt-in-unmet skip visible (skip (opt-in unmet: ...)) instead of lint/SKILL.md's prior behavior of silently dropping the ecosystem from output entirely — this applied to every opt-in-bearing ecosystem, not just dotnet.
  • Multi-tool ecosystems (bash's shellcheck-always/shfmt-conditional split; cross-cutting's per-tool config list) get explicit handling so an unconditionally-applicable sub-tool is never suppressed just because a sibling sub-tool's condition is unmet.
  • plugins/toolchain version 0.5.2 → 0.6.0 (minor — new capability, not a fix to previously-intended behavior).

Review history

An independent fresh-context review caught two IMPORTANT findings before this PR, both fixed and empirically re-verified:

  1. The opt-in condition originally required a C#-specific .editorconfig section ([*.cs]), but a universal [*] section demonstrably governs dotnet format's output on .cs files too (empirically confirmed: [*] indent_style=tab alone triggers real WHITESPACE violations). Widened the condition to [*] or a C#-glob section.
  2. The initial "opt-in-unmet skip suppresses the whole ecosystem row" rule was correct for single-condition ecosystems (dotnet, python) but wrong for multi-tool ones (bash, cross-cutting) where one sub-tool's condition can hold unconditionally — fixed both skill files to gate only the actually-unmet sub-tool(s).

plugins/toolchain has no automated test harness (documentation-driven: skills read YAML config, the executing agent runs the resolved shell commands — there's no script to unit-test), so verification here is empirical testing of the underlying dotnet format behavior plus careful prose review, not a CI-gated test suite.

Test plan

  • check-jsonschema against ecosystem.schema.json — both dotnet.yaml files (bundled default + example fixture) validate.
  • dotnet format --verify-no-changes behavior empirically verified (dotnet SDK 10.0.302): no .editorconfig → built-in defaults apply; .editorconfig with only unrelated globs ([*.md]) → zero effect; .editorconfig with [*] → real effect (matches a [*.cs] section).
  • scripts/check-changed-skills.sh origin/main — both edited skills PASS, 0 errors.
  • scripts/generate-catalog.mjs --check, scripts/check-changelog-parity.sh --check — pass.
  • typos (full repo, matches CI), markdownlint-cli2 on all touched files — clean.
  • Independent fresh-context code review — 2 IMPORTANT findings, both fixed and re-verified above.

Related

🤖 Generated with Claude Code

…atus

Closes #835

Adds the `opt-in` key to the dotnet ecosystem entry (.editorconfig with
a [*] or C#-glob section governs dotnet format's output; absent,
Roslyn's built-in defaults apply unconditionally — verified empirically
with dotnet SDK 10.0.302 and against dotnet format's own docs, including
the universal-[*]-section and unrelated-glob-only edge cases an
independent review surfaced).

check/SKILL.md now honors opt-in for the lint phase (it never did
before, so dotnet's check-cmd ran unconditionally regardless of
consumer config). Both check/SKILL.md and lint/SKILL.md make an
opt-in-unmet skip visible in the results table instead of silently
omitting the ecosystem, as lint/SKILL.md previously did for every
opt-in-bearing ecosystem — with explicit handling for multi-tool
ecosystems (bash, cross-cutting) so an unconditionally-applicable
sub-tool is never suppressed by a sibling sub-tool's gate.

## Related

- Epic #830, sub-item 5 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 3m 24s —— View job


PR Review: feat(toolchain): dotnet opt-in gate on .editorconfig, visible skip status

  • Gather context and understand the request
  • Read changed files and analyze the diff
  • Review SKILL.md changes (check + lint)
  • Review ecosystem YAML changes (dotnet.yaml)
  • Review plugin.json version bump and CHANGELOG
  • Post comprehensive review feedback

Overall

Solid, well-motivated change. The rationale is sound (verified empirically), the single-vs-multi-tool opt-in distinction is clearly articulated, and the visible-skip-vs-silent-drop fix is an unambiguous improvement. Two findings below — one behavioral nuance that's worth calling out explicitly, one schema staleness gap — plus a few minor prose observations.


Finding 1 — Multi-tool opt-in: shfmt runs even when .editorconfig is absent (behavior worth acknowledging)

check/SKILL.md line ~116, lint/SKILL.md line ~89

The multi-tool rule says: when shellcheck's condition holds unconditionally, "still run check-cmd and report its real output." Bash's check-cmd is:

check-cmd: "shellcheck -x -S warning <files> && shfmt -d <files>"

Because the entire check-cmd string is run, shfmt -d <files> will execute even when bash.yaml's opt-in says "shfmt only when .editorconfig declares shell style" and no shell .editorconfig is present. In check-mode this is read-only (-d = diff, no file changes), so the blast radius is low — the user sees formatting diffs for their shell files even though they never configured any. In fix-mode (shfmt -w <files>), it would rewrite files with shfmt's defaults, which is a stronger imposition than the original opt-in intended.

This is a real trade-off and the PR chose the right pragmatic pick (option 3 — splitting commands into separate schema keys — would be a bigger schema refactor). But the current skill prose doesn't acknowledge it: a future author reading "shfmt only when .editorconfig declares shell style" alongside the new multi-tool rule might expect shfmt to be suppressable, then be surprised it isn't. A single-sentence acknowledgment in the Gotchas section of check/SKILL.md would close that gap:

Multi-tool check-cmd atomicity — when a multi-tool ecosystem's check-cmd bundles both a gated and an unconditional sub-tool in one shell string, both run when the unconditional sub-tool's condition holds. The opt-in gate cannot suppress individual commands within a single check-cmd string; split commands into separate ecosystem keys to gate them independently.

View in context →


Finding 2 — ecosystem.schema.json: opt-in description is now semantically stale (out-of-scope file, worth flagging)

docs/conventions/ecosystem-commands/ecosystem.schema.json (not changed in this PR) currently says:

"opt-in": {
  "description": "The config file or condition whose presence signals the repo uses this toolchain (e.g. 'biome.json'). Informational for resolvers deciding applicability."
}

After this PR, opt-in is actively evaluated by skills — not merely informational. The single-vs-multi-tool parsing, the skip (opt-in unmet: ...) reporting, and the distinction between single-clause and semicolon-delimited multi-clause values are all derived from the opt-in string's prose shape. The schema description saying "Informational for resolvers" now misleads a future author who reads it as "safe to ignore in resolving behavior."

A follow-up update to the schema description (could be squashed here or logged as a task) would prevent that confusion. Suggested replacement:

"A human-readable condition evaluated by resolvers to decide whether to run this ecosystem's lint phase. Either a single condition for the whole check-cmd (e.g. dotnet, python), or a semicolon-delimited multi-clause string where each clause names a sub-tool and its own condition (e.g. bash). Resolvers report skip (opt-in unmet: ...) when the condition isn't met, rather than silently omitting the ecosystem."


Minor observations (no action required)

lint/SKILL.md step 3 → "see step 2/3 below" reference is ambiguous

The phrase "see step 2/3 below" in detection sub-step 3 is intended to point at the top-level ### 2. Run linters and ### 3. Present results sections, but the shared numbering could confuse a reader into looking back at the same sub-list. "see sections 2 and 3 below" would be clearer.

View →

Skip message: ≤10 words produces non-deterministic output

skip (opt-in unmet: <condition, ≤10 words>) asks the agent to summarize a 30-word opt-in string into ≤10 words on the fly. Different agent invocations may produce different summaries. This is intentional for an AI-driven system, but if consistent output matters (for check-jsonschema or diff-based auditing of skill output), consider adding a short opt-in-label key to the schema in a follow-up — a fixed 5–10 word summary that the agent lifts verbatim.

Version bump 0.5.2 → 0.6.0 is correct: new visible behavior (opt-in gate, changed output format) warrants a minor bump, not a patch.

CHANGELOG.md — well-structured, covers all three change categories (Added ×2, Fixed ×1).

reference/ecosystems/dotnet.yaml opt-in comment header — unusually thorough for a YAML comment (13 lines), but it carries empirical verification notes that would otherwise be invisible. The tradeoff is justified.


Summary

Approve with the two items above as suggested follow-ups:

  1. Add an explicit prose note in check/SKILL.md Gotchas acknowledging that multi-tool check-cmd atomicity means a gated sub-tool (shfmt) still runs when a sibling's unconditional condition holds.
  2. Update ecosystem.schema.json's opt-in description to reflect active evaluation semantics (separate commit / separate PR is fine).

Neither blocks merge — the behavior is correct and the conceptual model is sound.

@claude

claude Bot commented Jul 21, 2026

Copy link
Copy Markdown

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


Security Review — PR #890

Tasks

  • Load tools
  • Fetch PR diff
  • Read changed files
  • Analyze for security vulnerabilities
  • Post findings

Scope

Seven files changed across four categories: YAML config (dotnet.yaml ×2), skill instruction markdown (check/SKILL.md, lint/SKILL.md, check/context/dotnet.md), plugin manifest (plugin.json), and changelog. No GitHub Actions workflows, no new scripts, no new dependencies.

Findings

No security issues found.

Here is what I examined and why each area is clean:

Command injection — The opt-in field value is never passed to a shell. It is read by the AI agent as natural-language instruction to evaluate a filesystem condition (.editorconfig presence), and as display text in the skip (opt-in unmet: ...) status string. Neither path reaches a shell exec. The actual shell commands (check-cmd, fix-cmd) are unchanged; this PR only adds a gate that conditionally suppresses one of them.

Path traversal — The .editorconfig walk is bounded explicitly to the repo root (already established by git rev-parse --show-toplevel in the pre-existing skill preamble). No new file access outside that scope is introduced.

Prompt injection via consumer-supplied opt-in text — The opt-in field is consumer-configurable and interpreted by the AI agent. A crafted value could in principle try to influence the agent's behavior. However: (a) the consumer is the repo owner — the same trusted party already able to set a malicious check-cmd or fix-cmd, which are far more direct execution vectors; (b) the bundled default is a safe static string; (c) no third-party can influence this value without write access to the consumer's .claude/ecosystems/ directory. The opt-in field expands the interpretation surface only within the same trust boundary that already exists for every other consumer-configurable key.

Information disclosure — The condition text from opt-in appears in agent output. No credentials, tokens, or sensitive system paths are part of any opt-in value (bundled or example). The opt-in values are purely descriptive condition strings.

Authorization change — The opt-in gate restricts lint execution (dotnet format only runs when .editorconfig is present). Build and test phases are explicitly unaffected. This is a narrowing of behavior, not a widening.

Supply chain / dependencies — Version bump 0.5.2 → 0.6.0 in plugin.json. No new external dependencies, no new action pins, no new uses: or ${{ }} expressions.

GitHub Actions patterns — No workflow files changed.

@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: 82e44f328c

ℹ️ 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/toolchain/skills/check/SKILL.md Outdated
…icity allows

Two independent reviewers (Claude Code review + Codex) caught the same
gap: check-cmd is one opaque shell string, so the prior "still run
check-cmd when one sub-tool's condition holds" instruction didn't
actually suppress a gated sub-tool bundled in the same string (e.g.
bash's shfmt still ran even with no shell-style .editorconfig).

Narrows the claim: the binary opt-in gate applies cleanly only to
single-condition ecosystems (dotnet, python). Multi-tool ecosystems
(bash, cross-cutting) keep their prior, unchanged behavior — this PR
does not attempt to fix per-tool gating within one opaque command
string, and documents that limitation explicitly in check/SKILL.md's
Gotchas rather than claiming a fix that doesn't hold. Also updates the
shared ecosystem.schema.json's opt-in description, which was stale
after this PR's change to how opt-in is actively evaluated (a review
suggestion, not a structural schema change).

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

Both review findings addressed in 9e78fb6:

Claude Code review Finding 1 / Codex P2 (same root cause — check-cmd atomicity): correct catch. My "still run check-cmd, report real output" instruction didn't actually work — check-cmd is one opaque shell string, so a gated sub-tool bundled with an unconditional one (bash's shfmt alongside shellcheck) still ran regardless. Narrowed the claim: the binary opt-in gate now applies cleanly only to single-condition ecosystems (dotnet, python, the actual scope of #835). Multi-tool ecosystems (bash, cross-cutting) keep their unchanged prior behavior — not solved here, since splitting a bundled check-cmd into separately-gateable keys is a schema change out of this PR's scope. Documented the limitation explicitly in check/SKILL.md's Gotchas rather than claiming a fix that doesn't hold.

Claude Code review Finding 2 (schema opt-in description staleness): fixed — updated ecosystem.schema.json's opt-in description to reflect active evaluation semantics. Description-only change, no structural schema edit, so no ecosystem-commands contract version bump.

Minor observations (step 2/3 reference, ≤10-word non-determinism): the step reference is now "sections 2 and 3" per the suggestion. The ≤10-word summary non-determinism is accepted as-is (agent-interpreted prose is the established pattern for this whole field across every ecosystem) — a structured opt-in-label key would be a separate schema addition, not folded in here.

@claude

claude Bot commented Jul 21, 2026

Copy link
Copy Markdown

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


Security Review — PR #890

Tasks

  • Load tools
  • Fetch PR diff / read changed files
  • Analyze for security vulnerabilities
  • Post findings

Scope

Eight files across: skill instruction markdown (check/SKILL.md, lint/SKILL.md, check/context/dotnet.md), YAML config (reference/ecosystems/dotnet.yaml, examples/dotnet.yaml), schema (ecosystem.schema.json), plugin manifest (plugin.json), and changelog. No GitHub Actions workflows, no new scripts, no new dependencies. Reviewed against HEAD SHA 9e78fb6 (the follow-up narrowing commit is included).

Findings

No security issues found.

Here is what I examined and why each area is clean:

Command injection — The opt-in field added to dotnet.yaml is never passed to a shell. Per check/SKILL.md line 112–116, the agent reads it as natural-language prose to decide whether to gate check-cmd, and the condition summary appears as display text in skip (opt-in unmet: ...). Neither path executes the string. The actual shell commands (check-cmd, fix-cmd, build-cmd, test-cmd) are unchanged from prior behavior; the PR only adds a prose-evaluated gate that conditionally suppresses check-cmd in the lint phase.

Prompt injection via consumer-supplied opt-in text — The opt-in field is consumer-configurable and now actively interpreted by the agent (rather than being informational). A crafted value could try to influence agent behavior through the prompt. However, this does not expand the existing trust boundary: a consumer who can write .claude/ecosystems/dotnet.yaml can already supply arbitrary shell strings in check-cmd, fix-cmd, build-cmd, and test-cmd, which are far more direct execution vectors. The opt-in field's active-evaluation role is a narrower surface than what already exists.

Path traversal — The opt-in condition describes a filesystem walk ("anywhere from the changed file up to the repo root"). This walk is bounded by $REPO_ROOT established via git rev-parse --show-toplevel (SKILL.md line 49/50, carried over from existing skill preamble). The boundary is unchanged; this PR adds no new filesystem access scope.

Authorization change — The opt-in gate restricts dotnet format execution: it only runs when a governing .editorconfig is present. This is a narrowing of behavior, not a widening. Build and test phases are explicitly unaffected (check/SKILL.md line 112: "Build and test always run regardless of opt-in").

Follow-up commit 9e78fb6 specific — This commit removes the claim that the opt-in gate can suppress individual sub-tools in a multi-tool check-cmd (the gate correctly now only applies to single-condition ecosystems like dotnet and python). It also updates the schema description for opt-in. Both changes are documentation-only with respect to shell execution behavior. No new injection surface, no behavioral widening.

GitHub Actions / supply chain — No workflow files changed. No new external dependencies, action pins, or ${{ }} expressions introduced.

Information disclosure — The opt-in string appears in agent output when the condition is unmet. None of the bundled or example opt-in values contain credentials, tokens, or sensitive paths — they are purely descriptive condition strings.

@claude

claude Bot commented Jul 21, 2026

Copy link
Copy Markdown

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


PR Review — Post-Fix Pass (commit 9e78fb6)

  • Gather context and understand the request
  • Read changed files from the PR diff
  • Review fix for Finding 1 (check-cmd atomicity narrowed to single-condition ecosystems)
  • Review fix for Finding 2 (ecosystem.schema.json opt-in description update)
  • Review remaining files for correctness
  • Post final review

Overall

Both findings from the initial review are addressed correctly. The skill prose is now internally consistent and the schema description is accurate. One real defect remains: the fix commit updated the skill files but left the CHANGELOG.md's [0.6.0] Added bullet describing behavior that the fix explicitly walked back.


Finding — CHANGELOG [0.6.0] multi-tool description is now incorrect

plugins/toolchain/CHANGELOG.md, lines 24–29

The commit 9e78fb6 corrected the skill files but did not update the 0.6.0 CHANGELOG entry. The Added bullet still reads:

Multi-tool ecosystems (bash's shellcheck-always/shfmt-conditional split, cross-cutting's per-tool config list) are only whole-row-skipped when every sub-tool's condition is unmet — an unconditionally-applicable sub-tool (e.g. shellcheck) still runs and reports even when a sibling sub-tool in the same ecosystem is gated.

This describes the intended behavior from the original commit (82e44f3), which both reviewers identified as not actually achievable (check-cmd is an opaque shell string; the partial-skip cannot suppress individual sub-tools). The fix commit acknowledged this and restored prior behavior for multi-tool ecosystems — but the CHANGELOG still claims the partial-skip works as designed.

After 9e78fb6, the true state is: multi-tool ecosystems (bash, cross-cutting) are unchanged by this PR — they simply keep running check-cmd as before. The paragraph above should be replaced with something like:

Multi-tool ecosystems (bash, cross-cutting) whose check-cmd bundles multiple sub-tools into one opaque shell string are not changed by this release — they continue running check-cmd as before. Per-sub-tool gating within a single opaque command string is not addressable without a schema change; the atomicity limitation is documented in check/SKILL.md's Gotchas.

The CHANGELOG is the consumer-facing release summary — shipping it with an incorrect claim about what the release does is a real defect, not a nit. Fix this →


Rest of the PR: LGTM

Finding 1 fix (check/SKILL.md) — the rewrite is precise. The binary gate is scoped clearly to single-condition ecosystems; the multi-tool paragraph now correctly says "this gate does NOT apply — check-cmd is a single opaque string… Run check-cmd as before (unchanged from prior behavior)." The new Gotcha bullet names the atomicity limitation explicitly and notes the schema-change prerequisite. No overclaim remains.

Finding 2 fix (ecosystem.schema.json) — the updated opt-in description covers both value shapes (single-condition and multi-clause semicolon-delimited), documents the resolver's active evaluation duty, and replaces the stale "informational" characterization. Accurate.

lint/SKILL.md — consistent with check/SKILL.md after the fix. The "sections 2 and 3 below" wording is clearer than the prior "step 2/3." The single-condition vs. multi-tool distinction is now stated the same way in both skills.

dotnet.yaml (bundled default + example) — the opt-in condition is well-calibrated: gated on [*] or a C#-glob section rather than bare file presence, with a rationale comment that explains why a .editorconfig with only [*.md] doesn't count. Both the bundled default and the example fixture carry the same value.

check/context/dotnet.md — the lint section correctly flags the gate and cross-references the YAML rationale. Appropriate.

Version bump 0.5.2 → 0.6.0 — correct; new externally visible behavior (opt-in gate, changed output format) warrants a minor bump.


Summary

One item to fix before merge: update the CHANGELOG [0.6.0] multi-tool paragraph to match what the fix commit actually does (multi-tool ecosystems unchanged, prior behavior retained). Everything else is correct and the overall approach is sound.

@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: 9e78fb6a55

ℹ️ 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/toolchain/reference/ecosystems/dotnet.yaml Outdated
…n walk

Codex caught a real gap: EditorConfig discovery stops at a `root = true`
marker, so an outer repo-root section is never actually consulted by
dotnet format once a nested root marker sits between it and the changed
file. The opt-in text said "up to the repo root" unconditionally, which
could report opt-in as met from a parent section dotnet format itself
never sees.

Verified empirically (dotnet SDK 10.0.302): an outer [*] section has
zero effect on a nested .cs file once a `root = true` .editorconfig
without a C#-relevant section sits between them. The walk now stops at
whichever comes first — the repo root or a root = true marker — the
same discipline bash-format.sh's shell_editorconfig_opt_in() already
applies elsewhere in this repo.

Also corrects the CHANGELOG's [0.6.0] entry, which still described the
multi-tool partial-skip behavior the prior commit walked back.

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


PR Review — Post-Fix Pass (commit 3eb0257)

  • Gather context and load tools
  • Read PR diff and changed files
  • Review latest commit (root=true EditorConfig boundary fix)
  • Check CHANGELOG accuracy (prior finding resolved in 3eb0257)
  • Review all changed files for correctness
  • Post final review

Overall

Three finding-driven fix commits later, this PR is clean and ready to merge. All findings from the initial review (×2), the post-fix review (×1), and the Codex P2 review (×1) are correctly addressed. The PR now does what it says, the CHANGELOG accurately describes what changed, and the opt-in walk boundary honors real EditorConfig discovery semantics.


Commit 3eb0257 — Correct

The root = true boundary fix addresses the real gap Codex caught: EditorConfig discovery stops at a root = true marker, so a section in a file above that marker is never seen by dotnet format for files in the nested subtree. Before this fix, the opt-in walk went "up to the repo root" unconditionally and could falsely satisfy the condition from a parent section that dotnet format itself would ignore — producing a phantom opt-in and letting the gate pass when it should have skipped.

The fix is correct in three ways:

dotnet.yaml opt-in string (line 34) — now reads "walked from the changed file up to the repo root or a root = true marker (whichever comes first)." This correctly mirrors EditorConfig discovery semantics; the agent stops the walk at whichever boundary comes first, so a parent section beyond a root = true marker is never consulted.

Header comment (lines 20–27) — explains the rationale with the empirical verification (dotnet SDK 10.0.302, verified that root = true genuinely terminates discovery) and cross-references bash-format.sh's shell_editorconfig_opt_in() as prior art for the same discipline. The precedent link is accurate.

examples/dotnet.yaml (line 9) — synced to match the bundled default's opt-in value. The two files are consistent.

CHANGELOG [0.6.0] (lines 10–18) — the first Added bullet is updated to include the root=true boundary in the description, and the prior commit's inaccurate multi-tool paragraph is gone. All three bullets are now accurate: (a) dotnet opt-in with correct walk semantics, (b) binary gate scoped to single-condition ecosystems only with explicit note that multi-tool ecosystems are unchanged, (c) visible skip for single-condition ecosystems (not mis-claiming coverage of multi-tool).


All Prior Findings — Confirmed Resolved

Finding Source Commit Status
check-cmd atomicity: gate can't suppress individual sub-tools in a bundled string Claude Code, Codex P2 9e78fb6 ✓ Narrowed to single-condition ecosystems; Gotchas documents the limitation
ecosystem.schema.json opt-in description: "informational" is stale Claude Code 9e78fb6 ✓ Updated to describe active evaluation, both value shapes, visible skip
CHANGELOG multi-tool paragraph: describes behavior the fix walked back Claude Code 3eb0257 ✓ Replaced with accurate "unchanged from prior behavior" language
EditorConfig root=true boundary: outer section beyond a marker shouldn't satisfy gate Codex P2 3eb0257 ✓ Walk now stops at the first root=true marker or repo root

Rest of the PR — LGTM

check/SKILL.md — the binary gate (lines 112–118) is precisely bounded: explicitly "single-condition" ecosystems (dotnet, python), explicit "does NOT apply" for multi-tool, and Gotcha bullet names the atomicity limitation with the schema-change prerequisite. No overclaiming.

lint/SKILL.md — consistent with check/SKILL.md. Detection step 3 (line 89) and run step 2 (line 95) both correctly state the same single-condition / multi-tool distinction. The "sections 2 and 3 below" reference from the earlier nit is already "sections 2 and 3."

ecosystem.schema.jsonopt-in description (line 48) now covers both value shapes, documents the active evaluation contract and visible-skip obligation. Accurate.

check/context/dotnet.md — lint section (lines 36–42) correctly flags the opt-in gate and cross-references dotnet.yaml for rationale. No duplication.

plugin.json — version 0.6.0 is correct. New visible behavior (opt-in gate, changed output format) warrants a minor bump.


One Minor Observation (no action required)

opt-in string: inclusivity of root = true file is implied but not explicit

The opt-in value says "walked from the changed file up to the repo root or a root = true marker." A strict reading of "up to X" could be interpreted as exclusive of X (stopping before checking that file's sections). The header comment clarifies the intended behavior ("stops at a root = true marker even when that file has no C#-relevant section" implies the file IS checked before stopping), and because the agent resolves the bundled YAML (including header comments) the behavior in practice is unambiguous. But if the opt-in string is ever used in a context where only the value is visible, "up to and including the root = true marker" would be strictly unambiguous. Not blocking — the comment + value together are clear.


Summary

Approve. All four finding categories are correctly addressed, the CHANGELOG is accurate, the opt-in walk semantics honor real EditorConfig discovery behavior, and the scope is correctly bounded to single-condition ecosystems. The PR is ready to merge.

@claude

claude Bot commented Jul 21, 2026

Copy link
Copy Markdown

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


Security Review — PR #890 (HEAD: 3eb0257)

Tasks

  • Load tools
  • Fetch PR diff (including latest commit 3eb0257)
  • Read changed files
  • Analyze for security vulnerabilities
  • Post findings

Scope

Eight files across three commits. The third commit (3eb0257) is the one not covered by the two prior security reviews. Files: YAML config (reference/ecosystems/dotnet.yaml, examples/dotnet.yaml), skill instruction markdown (check/SKILL.md, lint/SKILL.md, check/context/dotnet.md), schema (ecosystem.schema.json), plugin manifest (plugin.json), changelog. No GitHub Actions workflows, no new scripts, no new package dependencies.

Findings

No security issues found.

Here is what I examined and why each area is clean.


Commit 3eb0257 specific — .editorconfig walk boundary and prompt injection surface

This commit is the first to explicitly instruct the agent to read .editorconfig files for the presence of a root = true marker. That is a new file-read vector: a crafted .editorconfig could theoretically inject content into the agent's context window when the agent evaluates the opt-in condition.

In practice, this is within the same trust boundary that already exists for every other consumer-configurable key. The consumer who controls the .editorconfig already controls check-cmd, fix-cmd, build-cmd, and test-cmd — opaque shell strings the agent executes directly, which are a far more capable injection surface. A hostile actor who can place a crafted .editorconfig in the repository already has write access and can reach the agent via much more direct vectors. The opt-in walk adds no new trust surface; it adds a new file read within the already-trusted consumer's own repository.

The walk itself is bounded correctly — the skill instructs the agent to stop at the repo root ($REPO_ROOT resolved via git rev-parse --show-toplevel) or at a root = true marker, whichever comes first. This mirrors the boundary already applied in bash-format.sh's shell_editorconfig_opt_in(). No unconstrained path traversal.

Command injection — The opt-in field value is read as prose and never passed to a shell. The agent uses it to decide whether to suppress check-cmd; the condition text appears only as display text in skip (opt-in unmet: ...) output. No execution path touches this string.

Authorization change — The opt-in gate restricts dotnet format execution. Build and test phases are explicitly unaffected (check/SKILL.md: "Build and test always run regardless of opt-in"). This is behavioral narrowing, not widening.

Information disclosure — The opt-in string appears in agent output when the condition is unmet. The bundled default and example values contain only descriptive prose; no credentials, tokens, or sensitive paths.

GitHub Actions / supply chain — No workflow files changed. Version bump 0.5.2 → 0.6.0 in plugin.json; no new external dependencies, action pins, or ${{ }} expressions introduced.


Summary

Three independent security passes on this PR (two prior, this one covering HEAD). All three find the same result: the changes are a behavioral narrowing of lint execution, the new opt-in evaluation and .editorconfig read operate within the existing consumer trust boundary, and no new injection surface, authorization gap, or information-disclosure risk is introduced.

@kyle-sexton

Copy link
Copy Markdown
Contributor Author

Both new findings from the post-fix review pass addressed in 3eb0257:

Codex — EditorConfig root=true boundary: correct, empirically confirmed. The opt-in walk now stops at whichever comes first — the repo root or a root = true marker — matching this repo's own bash-format.sh precedent for the identical discipline.

Claude Code review — stale CHANGELOG entry: correct, the [0.6.0] entry still described the multi-tool partial-skip behavior the prior commit (9e78fb6) walked back. Corrected to describe what actually ships: single-condition ecosystems get the new binary gate; multi-tool ecosystems are unchanged (documented limitation, not a fix).

@kyle-sexton
kyle-sexton merged commit 07ced80 into main Jul 21, 2026
23 checks passed
@kyle-sexton
kyle-sexton deleted the feat/835-dotnet-batch-guard branch July 21, 2026 18:53

@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: 3eb02573b3

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

### 2. Run linters per ecosystem

Run each ecosystem's resolved `check-cmd` (or `fix-cmd` with `--fix`). Honor each ecosystem's `opt-in`skip tools the project hasn't configured.
Run each ecosystem's resolved `check-cmd` (or `fix-cmd` with `--fix`). Honor each ecosystem's `opt-in`: for a single-condition ecosystem (dotnet, python), an unmet condition skips the whole ecosystem, reporting `skip (opt-in unmet: <condition, ≤10 words>)` visibly (never a silent omission) in every column that ecosystem's row has — this skip counts toward the table's total ecosystem count but never toward the FAIL count, the same precedent as a missing-tool skip. For a multi-tool ecosystem (bash, cross-cutting) whose `check-cmd` bundles multiple sub-tools into one opaque string, this binary treatment doesn't apply — run and report `check-cmd`/`fix-cmd` as before (unchanged from prior behavior); see `/toolchain:check`'s Gotchas for the known atomicity limitation.

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P2 Badge Apply opt-in skip reporting beyond dotnet/python

When /toolchain:lint sees a changed Markdown/PowerShell/YAML file whose configured opt-in is unmet, this new rule only defines the visible skip (opt-in unmet: ...) behavior for dotnet and python. Those ecosystems also have single-condition opt-ins in the bundled defaults, so this leaves the old silent omission path in place for them despite the updated results-table contract saying unmet opt-ins should be reported. Please treat every single-condition opt-in ecosystem, not just dotnet/python, as a visible skip.

Useful? React with 👍 / 👎.

kyle-sexton added a commit that referenced this pull request Jul 21, 2026
Resolves conflicts in plugins/toolchain/.claude-plugin/plugin.json and
CHANGELOG.md against #890's real merge (dotnet opt-in gate, 0.5.2 ->
0.6.0) — re-derives the version to 0.9.0, one past sibling PR #859's
own re-rebased 0.8.0 claim on the same plugin (issue #834, pyright).

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: 0f69ae76-07e0-4aae-9c4a-11688fc0e8ea
kyle-sexton added a commit that referenced this pull request Jul 21, 2026
main advanced to 0.6.0 (dotnet feature #890) after this branch had bumped
to 0.8.0, leaving a skipped 0.7.0 with no CHANGELOG entry. The pyright
addition is a single minor feature over 0.6.0, so 0.7.0 is the correct
next version. Retitle the CHANGELOG head entry and the plugin.json version
from 0.8.0 to 0.7.0.

Co-Authored-By: Claude Opus 4.8 <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(toolchain): dotnet batch guard — runtime analyzer/editorconfig detection

1 participant