fix(toolchain): python.yaml pyright skip granularity + config-glob + opt-in wording gaps - #1025
Conversation
…opt-in wording gaps Three PR #859 review follow-ups on the bundled python ecosystem default, batched via the #657 triage sweep: - Site 1: document pyright as a hard prerequisite of the python default once ruff config opts the ecosystem in (context/python.md). Tool presence is evaluated per ecosystem, not per tool, so a ruff-configured project with pyright absent reports a whole-ecosystem missing-tool skip that drops ruff coverage — the doc now says to install pyright alongside ruff. Chose the documented-prerequisite option over per-tool skip granularity per the issue's defaulted decision (smaller, reversible; the larger cross-ecosystem skip-mechanism change is not warranted here). - Site 2: add pyrightconfig.json to the python detection globs so a standalone pyright-config-only change is auto-detected by /toolchain:check and /toolchain:lint local runs. - Site 3: reduce the opt-in value to its single ruff-config gating condition. The trailing semicolon-separated pyright-mode clause structurally resembled the multi-clause per-tool opt-in form (per the ecosystem-commands contract), risking a misread of python's single-condition gate as multi-condition. Removed rather than reworded: opt-in is a condition field, not a mode note, and the descriptive pyright-mode text already lives in context/python.md gotchas (SSOT). Version 0.9.0 -> 0.9.1 + CHANGELOG. Verification: check-jsonschema (ecosystem.schema.json) ok; markdownlint 0 errors; typos clean; plugin-manifest schema ok. Closes #1017. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com>
|
Claude finished @kyle-sexton's task in 2m 21s —— View job PR Review
Verdict: Approve with one minor omission noted. All three fixes are correct, internally consistent, and well-scoped. No security concerns. One small gap and two editorial observations below. Site 2 —
|
|
Claude finished @kyle-sexton's task in 1m 26s —— View job Security Review — PR #1025
No security issues found. The four changed files are purely documentation and static configuration — no executable code paths, no injection surfaces, no credential handling, no authorization changes, and no supply chain exposure.
Rationale for each category checked:
|
|
🔒 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. |
|
Claude finished @kyle-sexton's task in 1m 21s —— View job PR Review
Verdict: Approve with one minor omission still outstanding. All three fixes are correct and internally consistent. The gap flagged in the previous review cycle (2026-07-22) has not yet been resolved. Outstanding gap —
|
|
Claude finished @kyle-sexton's task in 1m 2s —— View job Security Review — PR #1025
No security issues found. All four changed files are documentation and static configuration — no executable code paths, no injection surfaces, no credential handling, no authorization changes, and no supply-chain exposure.
Rationale per category checked:
|
|
Claude finished @kyle-sexton's task in 1m 28s —— View job PR Review
Verdict: Approve with one minor omission still outstanding (third cycle). All three fixes are correct and internally consistent. The gap flagged in both previous review cycles (2026-07-22 and 2026-07-25) remains unaddressed. Outstanding gap —
|
|
Claude finished @kyle-sexton's task in 1m 10s —— View job Security Review — PR #1025
No security issues found. All four changed files are documentation and static configuration — no executable code paths, no injection surfaces, no credential handling, no authorization changes, and no supply-chain exposure.
Rationale per category checked:
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d6099c72ab
ℹ️ 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".
… skip the runner never performs (#2092) ## Summary Discharges review thread `PRRT_kwDOTCGFQM6TyjYS` (PR #1025) on `plugins/toolchain/skills/check/context/python.md:37`. The doc claimed that a ruff-configured project with pyright absent reports a whole-ecosystem missing-tool `skip`, "dropping ruff coverage too". That is not what the runner does. The preflight probes the tool the ecosystem's commands are invoked through — `uv` — and pyright is never probed anywhere in the plugin. With `uv` installed, nothing skips: the two ruff commands run and pass, `uv run pyright` then exits 2, and the Lint cell reports **`FAIL`**. ### Which discharge branch, and why The reviewer offered two: *add a pyright availability probe*, or *document the actual failure behavior*. **This PR documents the actual behavior.** A probe is a contract change, not a bug fix — `SKILL.md:120` explicitly declines partial skips for opaque compound commands ("`check-cmd` is a single opaque string … with no way to run one sub-tool's portion without the other … do not attempt a partial skip"). Adding a per-sub-tool probe would contradict that stated rule and would want its own decision and its own schema work; `SKILL.md:184` already records it as a known limitation with "would need a schema change; not addressed here". Correcting the false claim is the smaller, safer discharge and is the one that removes the actual defect — a doc asserting an outcome the runner never produces. `SKILL.md`'s two gotcha bullets are in scope rather than creep: the "Missing tools → `skip`" bullet is the cross-ecosystem rule `python.md` was mis-inferring from. Correcting only `python.md` would leave the same wrong inference available to the next reader and to every other multi-tool ecosystem. ## Verification **Structural claims, read from the repository:** | Claim | Evidence | | --- | --- | | ruff precedes pyright, chained with `&&` | `reference/ecosystems/python.yaml:8` — `uv run ruff check . --no-fix && uv run ruff format . --check && uv run pyright` | | the preflight probes one tool per ecosystem | `skills/check/SKILL.md:114` — "verify **the tool** is on `PATH`" | | no pyright probe exists | `grep -rn pyright plugins/toolchain/` returns only the yaml command/globs/install-hint and the two doc pages — no probe | | partial skips are declined for opaque compound commands | `skills/check/SKILL.md:120` | **Runtime claim, executed** — disposable temp project, `PATH` scrubbed of the directory holding this machine's `pyright`, `uv` left reachable: ```text $ command -v pyright # under scrubbed PATH pyright NOT on scrubbed PATH $ uv run --no-project pyright error: Failed to spawn: `pyright` Caused by: program not found exit=2 ``` Exit 2 and the exact message the doc now quotes. Nothing on this machine was mutated: the project was a `mktemp -d`, and no installed-plugin state was touched. **Pre-fix control.** The defect here is a doc asserting an outcome the runner does not produce, so the control is the two texts against the established behavior: `origin/main`'s `python.md:37` says *"the whole ecosystem reports a missing-tool `skip` (dropping ruff coverage too)"*, which the `&&` ordering and the uv-only probe both contradict; the branch's text says ruff runs and the ecosystem reports `FAIL`, which the reproduction above confirms. Same runner, two docs, only one of them true. **Gates**, from the worktree root against `origin/main`: `check-changed-skills.sh` (PASS), `check-changelog-parity.sh` (`--check-bump`, `--check-order`), `check-skill-portability.sh` (no unexcused coupling tokens), `markdownlint-cli2` over `plugins/toolchain/**/*.md` (13 files, 0 errors). `SKILL.md` is 185 lines and `python.md` 53 — both well under the 500-line cap. Docs-only; no runtime behavior change. `toolchain` 0.11.1 → 0.11.2. ## Follow-up commit — two review threads, both real `24d54388` addresses both threads filed against the first pass. Neither was noise. **`PRRT_kwDOTCGFQM6XpW6P` — the normative rules disagreed with the new text.** `skills/lint/SKILL.md` said "verify **tools** on `PATH`" and listed missing tools as a `skip` edge case, and `skills/check/SKILL.md:114` said never to report `FAIL` for a missing tool. These markdown skills *are* the executable instructions, so a rule worded three ways is a real classification fork: an agent reading `lint/SKILL.md` could probe pyright and report `skip` for exactly the environment `python.md` now documents as `FAIL`. All three normative statements are now scoped explicitly to the tool an ecosystem's commands are invoked through, so an identical environment cannot be classified two ways depending on which file was read. Correcting `python.md` alone would have left the fork open — the thread was right that the doc change needed the rule change beside it. **`PRRT_kwDOTCGFQM6XpX1E` — `bash.md` lacked the parallel note.** The atomicity gotcha names bash's `shellcheck … && shfmt -d <files>` as the paired example of this exact shape, but only python documented the consequence. A project with ShellCheck present, shfmt absent, and the `.editorconfig` opt-in met hits the identical "nothing skips, spawn error, Lint `FAIL`" outcome. Since this PR's own argument for touching `SKILL.md` was that a `python.md`-only fix leaves the wrong inference available to every other multi-tool ecosystem, leaving bash undocumented would have contradicted it. `context/bash.md` now carries the matching gotcha rather than deferring it to a fast-follow. Re-run after the follow-up: `check-changed-skills.sh` (4 skills checked, 0 failed), `check-skill-portability.sh` (7 skill files, no unexcused coupling tokens), `markdownlint-cli2` (13 files, 0 errors). `check/SKILL.md` 185 lines, `lint/SKILL.md` 161, `bash.md` 43. ## Related N/A No linked issue --------- Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
Summary
Three mature, well-scoped follow-up findings on the bundled python ecosystem default
(
plugins/toolchain/reference/ecosystems/python.yamland its owning check-skill context doc),deferred from PR #859's review threads and batched here via the #657 triage sweep. All three are
scoped to the python ecosystem surface; no cross-ecosystem mechanism changes.
Fix
Site 1 (P1) — pyright skip granularity.
uv run pyrightis the firstcheck-cmdtool whosepresence is not implied by the ruff-config opt-in gate, and tool presence is evaluated per
ecosystem, not per tool (per the check skill's tool-presence rule). So a ruff-configured project
with pyright absent reports a whole-ecosystem missing-tool
skipthat also drops ruff coverage,rather than skipping pyright alone.
Chose the issue's defaulted decision — document pyright as a hard prerequisite of the python
default once ruff opts the ecosystem in (
skills/check/context/python.md, Type-check section) —over a per-tool skip-granularity code change. Rationale: per-tool skip is a larger cross-ecosystem
mechanism change (every
check-cmdtool would need independent-skip semantics); the documentedprerequisite is the smaller, reversible fix that resolves the reported fail-closed surprise without
redesigning the skip model. A maintainer wanting the larger mechanism can veto and redirect.
Site 2 (P2) — detection globs. Added
pyrightconfig.jsonto the pythonglobsso a standalonepyright-config-only change is auto-detected as python-affecting by
/toolchain:checkand/toolchain:lintlocal runs (CI already enforces its own check). The[tool.pyright]table isalready covered by the existing
pyproject.tomlglob.Site 3 (P2) — opt-in wording. Reduced the
opt-invalue to its single ruff-config gatingcondition. The trailing semicolon-separated pyright-mode clause structurally resembled the
multi-clause per-tool
opt-inform (per the ecosystem-commands contract, whose schema definesopt-inas either a single whole-check-cmdcondition or a per-tool multi-clause value), riskinga misread of python's single-condition gate as multi-condition. Removed rather than reworded:
opt-inis a condition field, not a mode note, and the descriptive pyright-mode text already livesverbatim in
skills/check/context/python.mdgotchas (single source of truth — no duplication). Thereduced value is now identical to the contract's own clean example
(
docs/conventions/ecosystem-commands/examples/python.yaml), confirming the intended single-conditionform.
Includes the per-plugin version bump (
0.9.0→0.9.1, patch — scoped bugfix/hardening) and amatching top-inserted
CHANGELOG.mdentry.Verification
Ran against the changed files in the worktree:
(
/toolchain:checkand/toolchain:lintare model-driven skills, not runnable scripts; theecosystem YAML is validated against its published
ecosystem.schema.jsoncontract as the concretestatic gate. No CI lane enforces the ecosystem schema — it is a reference contract — so the schema
run above is the authoritative structural check for the YAML edit.)
Related
🤖 Generated with Claude Code
Work-class: C3 (bug-fix-shaped) — attended triage 2026-07-23, operator-ratified. 🤖