fix(toolchain): execute resolved ecosystem gates array in check/lint (#926)#1020
fix(toolchain): execute resolved ecosystem gates array in check/lint (#926)#1020kyle-sexton wants to merge 4 commits into
gates array in check/lint (#926)#1020Conversation
The ecosystem-commands `gates` array (name/cmd/trigger-globs/remediation) was resolved as part of each ecosystem's command surface but never invoked by any workflow step, leaving bundled and consumer-declared gates inert. check/SKILL.md §2 now iterates each affected ecosystem's resolved `gates` after build → test → lint: a gate fires on a `trigger-globs` match (full changed-files set) or unconditionally when omitted, runs independent of the build/test/lint short-circuit, reports `skip` on a missing tool, and on failure surfaces `remediation` and flips Overall to FAIL. Replaces the stale prose gate step (which claimed the plugin ships none of its own) with the structured-array channel that setup/SKILL.md already routes gates into. lint/SKILL.md is unchanged — gates are CI-parity checks beyond lint's fast path. Bumps plugin 0.9.0 → 0.10.0 with a matching CHANGELOG Added entry. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
|
Claude finished @kyle-sexton's task in 1m 50s —— View job PR Review: fix(toolchain): execute resolved ecosystem
|
|
Claude finished @kyle-sexton's task in 2m 22s —— View job Security Review — PR #1020
No security issues found. This PR is a documentation-layer change (a SKILL.md instruction file, a version bump, and a changelog entry) with no executable code. What was reviewed
Analysis
Consumer-supplied gate commands — A consumer's Bundled
Fail-safe behaviors preserved — Missing tool → No GitHub Actions workflows touched. No credentials, tokens, or secrets involved. |
|
Claude finished @kyle-sexton's task in 3m 22s —— View job PR Review: fix(toolchain): execute resolved ecosystem
|
|
Claude finished @kyle-sexton's task in 1m 26s —— View job Security Review — PR #1020
No security issues found. This PR is a documentation-layer change (a SKILL.md instruction file, a version bump, and a changelog entry) with no executable code. The security model is unchanged from the existing What was reviewed
Analysis
The new step instructs the AI to execute
Gates substitute Bundled
Pattern matching filenames for gate fire decisions is safe; the filenames are used for comparison only, not passed to a shell or eval'd. Gate independence from build/test/lint short-circuit (L124) Gates that were previously inert now execute after a failed build/test/lint phase. This is a behavioral change but not a security regression — the gate commands are still consumer/bundled YAML content subject to the same trust rules. There is no privilege escalation from executing in the presence of a prior failure.
No GitHub Actions workflows touched. No |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: fb0afb922d
ℹ️ 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".
|
|
||
| - **Fire condition** — if `trigger-globs` is present, run the gate only when ≥1 changed file matches, matched against the **full** changed-files set (not the ecosystem-scoped subset — a gate's trigger files need not classify into the ecosystem's own `globs`). If `trigger-globs` is omitted, run whenever the ecosystem runs. A present `trigger-globs` with no matching changed file (e.g. `check all` on a clean tree) → the gate does not fire. | ||
| - **Independent of the build/test/lint short-circuit** — a fired gate runs even when this ecosystem's build, test, or lint already failed and stopped (line above). Gates mirror CI checks that are independent of build success (a lockfile or `go mod tidy` gate is meaningful whether or not the build compiled), so a failed earlier phase never suppresses them. | ||
| - **Run** `gate.cmd` (an opaque shell string — substitute the same placeholders as other commands: `<files>`, resolved anchor, etc.) from `$REPO_ROOT` with absolute paths. |
There was a problem hiding this comment.
In Go monorepos, project-discovery: ["go.mod"] deliberately runs build/test/lint from every module root, but this instruction runs go mod tidy -diff only from $REPO_ROOT. If the only module is nested, the gate exits with “go.mod file not found” and falsely fails every matching change; if a root module also exists, drift in nested modules is never checked. Run project-discovery-backed gates once per resolved <project-dir> instead.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
| # | Finding | Classification | Evidence | Reacted |
|---|---|---|---|---|
| 1 | Execute module gates from each discovered Go module root (Go monorepos) | VALID — fixed | Genuine defect. go.yaml sets project-discovery: ["go.mod"] precisely because go build/test/list ./... silently skip nested-module packages (documented in go.yaml L6-11), so build/test/lint already walk each module root — but the gate step ran gate.cmd only from $REPO_ROOT. go mod tidy -diff is inherently per-module: a nested-only monorepo falsely fails with go.mod file not found, and a root+nested repo never checks nested-module drift. |
👍 |
Fixed in c8862de: the gate now executes from the same location the ecosystem's own build/test/lint use — once per resolved <project-dir> for a project-discovery ecosystem, from the anchor's directory otherwise, $REPO_ROOT only when neither is defined. The fire condition stays repo-wide (trigger-globs vs the full changed-files set decides whether the gate runs); only the execution location becomes per-project.
|
🔒 merge-drive lane claiming this PR for on-branch fix work this cycle (amendment-round: 16, worker tier). Will fix clear branch-owned CI/bot findings, defer non-DEFER-FORBIDDEN items with reply+resolve, push, then re-check the merge gate. |
…REPO_ROOT The gate-execution step ran `gate.cmd` from `$REPO_ROOT` regardless of the ecosystem's scoping model. For a `project-discovery` ecosystem this broke the bundled `go.yaml` `go-mod-tidy-drift` gate in Go monorepos: `go mod tidy -diff` is inherently per-module, so a repo-root-only run falsely fails when the sole module is nested (`go.mod file not found`) and never checks drift in nested modules when a root module also exists — the exact monorepo case go.yaml's `project-discovery: ["go.mod"]` walk exists to cover for build/test/lint. Gate execution now mirrors the ecosystem's own build/test/lint location: once per resolved `<project-dir>` for a `project-discovery` ecosystem, from the `anchor`'s directory otherwise, `$REPO_ROOT` only when neither is defined. The fire condition stays repo-wide; only the execution location is per-project, and a gate `cmd` using `<files>` gets that project's scoped subset. Also completes the report scaffold with a gate-failure row and clarifies the PASS-case gate-count format. Addresses codex P2 (Go-monorepo gate roots) and claude[bot] review Nits 1-3.
|
Claude finished @kyle-sexton's task in 1m 54s —— View job Security Review — PR #1020
No security issues found. This PR is a documentation-layer change that wires a previously-inert feature ( What was reviewed
Referenced for trust-model verification: Analysis
L125 instructs the AI to execute Bundled The only bundled gate that fires under this new step ( Per-project execution location (HEAD commit c8862de) — gates now run from each The latest commit changes gate execution location from
L123: filenames from git status are matched against globs for fire decisions, not passed to a shell or eval'd. No injection surface. Gates running after a build failure — behavioral change, not a security regression Gates now execute even when the ecosystem's build/test/lint already failed (L124). A consumer-controlled SUGGESTION —
|
|
Claude finished @kyle-sexton's task in 2m 0s —— View job PR Review: fix(toolchain): execute resolved ecosystem
|
| Nit | Status |
|---|---|
| Nit 1 — report scaffold lacked gate-failure example | Resolved — L143–145 now shows a Gates: summary line and gate-failure Overall in the fenced code block |
Nit 2 — <files> expansion inside gate.cmd was ambiguous |
Resolved — L125 now explicitly states <files> expands to the project's scoped changed-files subset in gate context, same as ecosystem commands |
| Nit 3 — PASS-case gate-count format unspecified | Resolved — L150 now provides both the FAIL example and an explicit PASS example: Overall: PASS (2 of 2 ecosystems passed, 1 gate passed) |
Nit 4 — per-gate install-hint gap (pre-existing schema gap) |
Still open; schema has no per-gate install-hint; out of scope for this PR |
Codex P2 — gates only ran from $REPO_ROOT, not per module root |
Resolved in c8862de |
Remaining observation (low priority, not blocking)
Per-project gate run reporting format is underspecified. When project-discovery results in N project roots and a gate fires, it runs N times (once per <project-dir>). The fire condition (trigger-globs vs the full changed-files set) is correctly specified as a single repo-wide decision. But the report spec at L143 shows Gates: go-mod-tidy-drift — FAIL (...) as a single line — it doesn't say whether a gate that runs 3 times (e.g. 3 Go module roots) should appear as 3 separate per-project lines or one aggregated line. If ./cmd passes and ./internal fails, an aggregated FAIL is the right verdict, but a reader would lose the per-project detail. This is a documentation completeness gap that would be noticeable in a real monorepo failure. A follow-on can address it (e.g. go-mod-tidy-drift [./internal] — FAIL (...)), but it doesn't block this PR — the fix is correct and the FAIL verdict propagates either way.
plugins/toolchain/.claude-plugin/plugin.json
Version bump 0.9.0 → 0.10.0 is correct — bundled gates were previously inert (no behavioral effect); making them execute and adding per-project-root execution is new backward-compatible behavior, warranting a minor-version bump under semver.
plugins/toolchain/CHANGELOG.md
The [0.10.0] / ### Added entry is accurate and complete. It mentions: gate array execution, the per-project-root fix and the failure modes it prevents, both the bundled (go-mod-tidy-drift) and consumer-declared gate shapes, and that lint is unchanged. Well-scoped.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c8862de21c
ℹ️ 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".
| **CI-parity gates (resolved `gates` array).** After an affected ecosystem's build → test → lint, iterate its resolved `gates` array (§1.5 — bundled default or consumer file, per the ladder). Gates cover the CI-parity checks plain build / test / lint don't catch: lockfile drift, generated-artifact freshness, schema regeneration. For each gate: | ||
|
|
||
| - **Fire condition** — if `trigger-globs` is present, run the gate only when ≥1 changed file matches, matched against the **full** changed-files set (not the ecosystem-scoped subset — a gate's trigger files need not classify into the ecosystem's own `globs`). If `trigger-globs` is omitted, run whenever the ecosystem runs. A present `trigger-globs` with no matching changed file (e.g. `check all` on a clean tree) → the gate does not fire. |
There was a problem hiding this comment.
With automatic targeting, an ecosystem is selected only when a changed file matches its globs; consequently, a consumer gate such as a Go buf/protobuf freshness check with trigger-globs: ["*.proto"] is never reached when only a .proto file changes, because Go's normal globs do not include that file. This contradicts the stated support for gate trigger files that do not classify into the ecosystem and leaves those CI-parity gates inert unless the user explicitly runs /toolchain:check go or all. Include ecosystems whose gates match the full changed-file set before iterating their gates.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
| # | Finding | Classification | Evidence | Reacted |
|---|---|---|---|---|
| 1 | Select ecosystems when a gate trigger-globs matches, even if the ecosystem's own globs don't classify any changed file |
VALID (defer — author design decision) | Real gap. §1 Ecosystem detection selects the affected set by matching git status against each ecosystem's own globs; the gate loop lives inside "For each affected ecosystem" (§2 CI-parity gates). So an ecosystem whose only relevance to a change is a gate trigger-globs match — not its own globs — is never selected under auto-targeting, and its gates never evaluate. That contradicts the new §2 claim that a gate's trigger files "need not classify into the ecosystem's own globs." Scope is narrow (P2-consistent): the only bundled gate, go.yaml's go-mod-tidy-drift, has trigger-globs: [go.mod, go.sum, *.go] — a strict subset of go's own globs: [*.go, go.mod, go.sum] — so no shipped gate exercises the cross-ecosystem path; the gap bites only consumer-declared cross-ecosystem gates (e.g. a Go buf/protobuf gate with trigger-globs: ["*.proto"]), and even then check all / check go still fire it. |
👍 |
Deferring rather than fixing on-branch because the remediation is a genuine design fork the author should own, not a mechanical edit — the two directions point opposite ways:
- (a) Expand auto-selection (this comment's suggestion): pull an ecosystem into the affected set when any of its resolved gates'
trigger-globsmatch the full changed-file set. This then raises "do you also run that ecosystem's build/test/lint, or only its gates?" — likely a new gate-only sweep concept the workflow doesn't currently model — and it requires resolvinggates(§1.5, ladder) during detection (§1), before ladder resolution currently runs. - (b) Narrow the claim: qualify the "need not classify into the ecosystem's own
globs" wording to state that, under auto-targeting, a cross-ecosystem gate fires only viacheck all/check <ecosystem>.
Add-capability vs. document-reduced-behavior is the maintainer's call, so surfacing for a human decision rather than picking one autonomously. Branch was freshened with main in the interim (merge 41fa7c5) — no toolchain overlap; this finding is unaffected by that merge.
|
Classifying the four 🟡 Nit items from the PR review (verdict was APPROVE — no blocking issues):
Fix commit: c8862de. Nits 1-3 fixed on-branch; Nit 4 deferred as a schema follow-on. Branch was also freshened with |
…m-gates-execution
|
Claude finished @kyle-sexton's task in 4m 38s —— View job PR Review: fix(toolchain): execute resolved ecosystem
|
| Finding | Status |
|---|---|
| Nit 1 — report scaffold lacked gate-failure example | ✅ Resolved — Gates: row + gate-failure Overall now in fenced block |
Nit 2 — <files> in gate cmd was ambiguous |
✅ Resolved — <files> explicitly expands to project-scoped subset in gate context |
| Nit 3 — PASS-case gate-count format unspecified | ✅ Resolved — PASS example added at L150 |
Nit 4 — per-gate install-hint schema gap |
🔵 Deferred (pre-existing schema gap, out of scope) |
Prior Codex P2 — gates ran from $REPO_ROOT only |
✅ Resolved in c8862de — gates now run per-project-root |
🔴 Blocking: New Codex P2 — gate-trigger ecosystems never reached in automatic detection
Finding from commit c8862de review:
With automatic targeting, an ecosystem is selected only when a changed file matches its
globs; a gate withtrigger-globs: ["*.proto"]on the Go ecosystem is never reached when only a.protofile changes, because Go's normalglobsdon't include that file. Include ecosystems whose gates match the full changed-file set before iterating their gates.
Verdict: VALID. Fix required.
The spec contains a self-contradiction:
- §1 (ecosystem detection) selects ecosystems where ≥1 changed file matches the ecosystem's own
globs. - §2 gate fire condition (L123) says: "matched against the full changed-files set (not the ecosystem-scoped subset — a gate's trigger files need not classify into the ecosystem's own
globs)".
The §2 clause is unreachable: if the ecosystem's globs didn't match, §1 never includes the ecosystem, so §2 is never entered. A gate with trigger-globs: ["*.proto"] on the Go ecosystem is permanently inert in automatic mode unless the user explicitly runs /toolchain:check go.
Proposed fix — add a gate-trigger inclusion pass in §1, immediately after the current first sentence:
**Gate-trigger inclusion (automatic detection only).** After the `globs` pass, also
resolve each non-selected covered ecosystem's `gates` array and include it if at least
one gate's `trigger-globs` matches ≥1 file in the full changed-files set. A
gate-only-included ecosystem skips build/test/lint entirely — all three columns show
`—` in the report table, because its `globs` matched nothing and there is nothing to
build, test, or lint; only its gates run, under the same fire-condition, tool-presence,
and outcome rules in §2. (This is what makes the gate fire-condition clause "a gate's
trigger files need not classify into the ecosystem's own `globs`" reachable — without
this pass, an ecosystem whose `globs` did not match would never be reached and its
gates would be inert.)
Also add a bullet to the [0.10.0] CHANGELOG entry (under the existing ### Added bullet):
- **Gate-trigger ecosystem inclusion.** Automatic ecosystem detection now includes any
covered ecosystem whose resolved `gates` array has a gate with `trigger-globs`
matching ≥1 changed file, even when the ecosystem's own `globs` matched nothing.
Without this, a gate declaring `trigger-globs` outside the ecosystem's own file
patterns (e.g. a Go buf/proto freshness gate on `*.proto`) was inert in automatic
mode — the ecosystem was never selected and its gates were never reached. A
gate-only-included ecosystem skips build/test/lint (all `—`); only its gates run.
plugins/toolchain/.claude-plugin/plugin.json
Version bump 0.9.0 → 0.10.0 is correct. The gate-trigger inclusion fix (if applied) remains a minor-version addition within the 0.10.0 release.
plugins/toolchain/CHANGELOG.md
Accurate, well-scoped. The gate-trigger inclusion bullet above should be added to complete the entry.
I prepared a commit with this exact fix locally, but push permissions are restricted to the PR author for this branch. The "Fix this →" link above will apply the change directly.
|
Claude finished @kyle-sexton's task in 2m 40s —— View job Security Review — PR #1020
No blocking security issues found. This PR is a documentation-layer change — a SKILL.md instruction file, a version bump, and a changelog entry. One SUGGESTION below regarding a design gap that this PR activates for the first time. What was reviewed
Referenced for trust-model verification: Analysis
L125 instructs the AI to execute Per-project-dir execution (introduced in The latest commit changed gate execution from Bundled The only bundled gate that fires (
L123: filenames are matched against globs to decide whether a gate fires — they are used for comparison only, not passed to a shell or eval'd. Gates running after a build failure — behavioral change, not a regression L124: gates now execute even when a prior build/test/lint phase failed. The consumer already controlled No GitHub Actions workflows touched. No credentials, tokens, or secrets in any changed file. SUGGESTION — Ecosystem selection gap silently bypasses declared gates on non-ecosystem-classified filesConfidence: High — confirmed by reading the spec and The gate-execution step in L121 iterates Before this PR all gates were inert anyway; this PR is the first time gates actually execute, which makes this silent bypass newly exploitable. A consumer who declares a security-critical gate (e.g. a schema regeneration or auth-proto freshness check) for a file type that doesn't classify into the parent ecosystem's This is a pre-existing architectural gap in the design (the spec correctly notes "a gate's trigger files need not classify into the ecosystem's own |
|
Classifying the one open item from this review (verdict was APPROVE; nits 1–3 and the Codex module-root P2 are marked resolved in the review's own status table, and were classified earlier on this PR):
|
|
Classifying the one SUGGESTION from this security review (verdict: No security issues found):
|
kyle-sexton
added
the
agent-ready
1 participant
Summary
docs/conventions/ecosystem-commands/ecosystem.schema.jsondefines a structuredgatesarray (name/cmd/trigger-globs/remediation) that any ecosystem config — bundled default or consumer-tracked — can declare, and bothplugins/toolchain/reference/ecosystems/go.yaml(go-mod-tidy-drift) anddocs/conventions/ecosystem-commands/examples/dotnet.yaml(nuget-lockfile-drift) ship one. The array was resolved as part of each ecosystem's command surface (check/SKILL.md§1.5) but never invoked by any workflow step — so every declared gate was inert. The only gate-execution mechanismcheck/SKILL.mddocumented was a prose, consumer-CLAUDE.md-only step whose closing sentence ("this plugin ships none of its own") was itself stale oncego.yamlshipped a bundled gate.Fix
Scoped to
plugins/toolchain/skills/check/SKILL.md, precise and additive (no restructuring of unrelated parts)::121. After an affected ecosystem's build → test → lint, iterate its resolvedgatesarray (§1.5 ladder — bundled default or consumer file). Per gate:trigger-globspresent → run only when ≥1 changed file matches, against the full changed-files set (a gate's trigger files need not classify into the ecosystem's ownglobs); omitted → run whenever the ecosystem runs; present-but-no-match (e.g.check allon a clean tree) → does not fire.go mod tidygate is meaningful regardless of build success).skipwith the ecosystem'sinstall-hint, neverFAIL.pass/FAILby name; onFAILsurfacegate.remediation; a fired-and-failed gate counts toward the run's FAIL verdict.FAILand is counted even when every ecosystem's build/test/lint cell passed (e.g.Overall: FAIL (0 of 2 ecosystems failed, 1 gate failed)).go.yaml'sgo-mod-tidy-drift; consumer-declared in.claude/ecosystems/<ecosystem>.yaml).:119("CI-parity gates … run independent ofcheck-cmd") and the report scaffold (:140).lint/SKILL.mddeliberately unchanged (the one maintainer-vetoable fork, per the planning comment): every schema-described gate and both shipped examples are CI-parity checks beyond build/test/lint (lockfile drift, module drift, generated-artifact freshness) — none is a lint/format op. They belong with the heaviercheckstep and/verification:confirm(the sanctioned pre-commit gate); adding them to/toolchain:lintwould break its "fast" contract. Veto before merge if a maintainer wants gates on the lint path.Version bump (rule 6d): fresh
origin/mainhadplugins/toolchainat0.9.0(claimed by the lychee PR #867); this is a new executable behavior, so bumped to0.10.0with a matching## [0.10.0]→### AddedCHANGELOG entry. (The planning comment's0.8.0 → 0.9.0predated #867 landing on main.)Verification
No skill-behavior test harness exists (
find plugins/toolchain -iname '*test*'and a repo-level search fortoolchain:check/gatestests both empty), so verification is static gate + hand-trace:/skill-quality:checkon the editedcheckskill: PASS — 0 errors, markdownlint clean, 165/500 lines, all base-ref trigger phrases preserved (frontmatterdescriptionuntouched). The lone WARN (description lacksUse when:phrasing) is pre-existing and out of scope.go.yaml, a real rung-4 default that runs for this repo): a change set includinggo.sum(or any*.go) makesgoaffected; §1.5 resolvesgo.yaml; the new §2 step reaches itsgatesand matchesgo-mod-tidy-drift'strigger-globs: ["go.mod","go.sum","*.go"]against the full changed-files set → gate fires → runsgo mod tidy -difffrom$REPO_ROOT. Drift →FAIL+ remediation + Overall FAIL; clean →pass; missing Go toolchain →skipwithinstall-hint.examples/dotnet.yaml, an example fixture underdocs/, not a bundled default): for a consumer whose tracked.claude/ecosystems/dotnet.yamlcarries thenuget-lockfile-driftgate, a changed*.csprojmatchestrigger-globs: ["*.csproj","Directory.Packages.props","packages.lock.json"]→ gate fires → runsdotnet restore --locked-mode. Same pass/FAIL/skip semantics.Both gate entries are invoked by the new step given a matching changed-files set; neither required a per-ecosystem change.
Closes #926
Related
2026-07-22; addedgo.yaml'sgo-mod-tidy-driftgate whose Codex review surfaced this wiring gap. This change is the cross-cutting follow-up that makes that gate (and every other declared gate) actually run — additive, nogo.yamlchange.gatesschema array into an actual execution step #926 (2026-07-22T03:10:48Z) — the primary implementation plan followed here; the only deviation is the version target (0.10.0, re-derived from freshorigin/mainper rule 6d, since0.9.0was taken by feat(toolchain): add lychee-offline to cross-cutting ecosystem default #867 after the plan was written).🤖 Generated with Claude Code
https://claude.ai/code/session_019xSPACJvbsqGsjYCP91DoX
Work-class: C3 (bug-fix-shaped) — attended triage 2026-07-23, operator-ratified. 🤖