fix(testing): guard unguarded dotnet cross-plugin ref, frame .NET enrichment lists as stack-specific - #491
Conversation
…ichment lists as stack-specific The `testing` plugin bills as ecosystem-agnostic but handed non-.NET consumers .NET-only enrichment as the universal path, and carried one bare unguarded cross-plugin reference (a defect per PLUGIN-PHILOSOPHY). - write (per-cycle checklist): remove the unguarded inline `dotnet-test:*` parenthetical; move its detection-layer skills under write's now-guarded marketplace list. - Add the `(invoke only when installed)` guard suffix to write's marketplace heading (the only one of six missing it). - Reframe the all-.NET lists (write, organize, investigate, loop) as ".NET-ecosystem plugin skills — applicable when your stack is .NET"; plan (mixed) and run-e2e (no .NET) get accurate stack-qualifier lead-ins instead of a blanket .NET label. - Bump testing 0.2.0 -> 0.2.1 + CHANGELOG. Closes #430. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_011V31qpAHP3jfs76B9d5Rfo
|
Claude finished @kyle-sexton's task in 2m 3s —— View job PR Review: fix(testing): guard unguarded dotnet cross-plugin ref, frame .NET enrichment lists as stack-specific
This PR addresses the exact defect class defined in What's correctUnguarded inline reference removed correctly. The parenthetical Guard heading added to Ecosystem-specific lead-ins are accurate and differentiated. The PR correctly uses three distinct lead-in variants:
Moved skills maintain their connection to the checklist. The Version bump and CHANGELOG are correct. No hard dependencies introduced. Verified: One finding worth noting (pre-existing, not introduced here)Seam-phrasing fallbacks absent — pre-existing gap across the fleet.
This gap pre-exists this PR and appears consistently across the whole Out-of-scope observation (not a blocker)
SummaryThe PR is correct, complete within its stated scope, and clean. All six marketplace sections now carry the Approachable for merge. |
…ls (#531) (#609) ## Summary Skills declared ecosystem/forge/tracker-agnostic ship bare hardcoded stack/forge/branch/tracker defaults because agnosticism was asserted in prose and never enforced mechanically — the dominant review-churn class, re-caught by the external reviewer PR after PR (branch/remote hardcodes, seam-conditional eval, .NET-flavored examples). This adds a fail-closed CI lane that mechanically detects that coupling instead of paying for it at review time again and again. Detection only — this PR builds the gate. Fixing the existing violations stays with the member issues below. ## Fix A new `portability-lint` lane (`.github/workflows/ci.yml`) wired into the required `ci-status` aggregate, plus a repo-local detector: - **`scripts/check-skill-portability.sh`** — scans skill files for coupling tokens. On a PR it scans only the skill files the change **touches** (mirroring the `skill-quality-gate` changed-diff pattern), so enabling a token class prevents **new** coupling without red-lining pre-existing violations — main's push event scans nothing (self-test is the push path), and existing hits wait for their owning follow-up fix or the file's next edit. - **`scripts/skill-portability-tokens.txt`** — the token list as external, extensible **data** (not logic buried in bash), so a reviewer re-catch is a one-line data edit. Seeded with **one active class** — the branch/default-branch hardcode (`origin/main` / `origin/master`) — per the ratified one-token-class-at- a-time rollout; further classes (`dotnet`/`Clean Arch`, `raw.githubusercontent`, bare `gh` tracker calls) are staged as commented entries with enable-triggers. - **Never-skip shape**: the job is unconditional; only the PR-diff step is event-gated, and a self-test step runs first so a broken detector can never mask a real violation behind a green gate. **Design decisions** (resolved per the ratified plan, not invented): - *How a skill declares agnosticism scope* — **no new frontmatter field**. A skill is agnostic by default (the Design boundary already binds every plugin), so the gated set is the files a change touches. A hit is excused three ways, all reviewer-visible comments (reusing the silent-skip gate's annotated-exemption shape): an auto-recognized detection-first / presence-gated use; a per-site `portability-ok: <reason>`; or a whole-file `portability-scope: <reason>` declaring an inherent narrower boundary (the forge-locked-under-a-neutral-name case). This distinguishes **guarded** refs (fine) from **bare** ones, as the guarded-forward-ref disposition requires. - `docs/PLUGIN-PHILOSOPHY.md` gains one doctrine sentence extending the existing declared-narrower-boundary allowance from OS platform to the forge/ecosystem/tracker axis. No `plugins/<name>/` directory is touched, so no version bump / CHANGELOG entry is needed (matching the CI-gate precedent). ## Verification - **Self-test suite** (`scripts/check-skill-portability.test.sh`, runs in CI): `PASS=12 FAIL=0` — covers bare-token FAIL with file:line, detection-ladder pass, same-line and comment-block-above `portability-ok`, annotation non-leak past intervening code, whole-file `portability-scope`, staged tokens staying inactive under the shipped list, fail-closed exit 2 on missing token list / invalid base ref, the vendor/evals/`*.test.sh` exclusion set, and empty scope. - **Catches a real violation, passes a real legit use** (the bare-vs-guarded discrimination that drives this class), on live corpus files: ``` $ scripts/check-skill-portability.sh --paths plugins/work-items/skills/track/actions/start.md COUPLING: plugins/work-items/skills/track/actions/start.md:61: origin/(main|master) -> ... git checkout -b <type>/<N>-<slug> origin/main ... COUPLING: plugins/work-items/skills/track/actions/start.md:65: origin/(main|master) -> ... git checkout -b <type>/<N>-<slug> origin/main ... (exit 1) $ scripts/check-skill-portability.sh --paths plugins/review/skills/fanout/SKILL.md No unexcused coupling tokens in 1 skill file(s). # detection-first origin/HEAD ladder → guarded (exit 0) ``` - **Calibrated against the full corpus** (`--all`): the active branch class flags exactly 3 genuine bare hardcodes in 2 files (owned by member issues; changed-file scoping keeps them off main) and correctly passes the detection-ladder uses in `review/*` and excludes evals/test fixtures. - **Changed-file CI path proven** against a real base ref (16 changed skills gated, all clean). Local checks green: `shellcheck` (repo `.shellcheckrc`), `shfmt`, `actionlint`, comment-residue detector (T1=T2=T3=0), `typos`. Closes #620 Part of #531 (umbrella stays open — stages remain; child #620 carries this PR's shipped scope per the #603 pattern). ## Related - Member coupling instances (this is prevention; it does not block them): #404 #405 #406 #408 #410 #412 #415 #416 #418 #421 #422 #423 #428 #429 #432 #438 #439 #441 #442 - #445 — sibling CI-gate backlog (mechanical conformance only; explicitly not coupling) - #412 — guarded-forward-ref disposition the gate honors (guarded ≠ bare) - #441 — declared narrower-scope exemption the `portability-scope` mechanism serves - #467 — the branch/remote review-churn the seed active class targets - #453 — seam-conditional eval re-catch of this class - #491 #492 — .NET-flavored-example hand-fixes a future staged class would obviate - #611 — post-green review finding (annotation-carry pending_annot doesn't distinguish inline vs block HTML comments), deferred out of this PR's scope 🤖 Generated with [Claude Code](https://claude.com/claude-code) https://claude.ai/code/session_01KyLj6oaFVTE6xFuoYCC2KC --------- Co-authored-by: Claude Sonnet 5 <noreply@anthropic.com>
…efs, keep tool names (#405) Rework per the control-tower ruling relay (2026-07-20) on this PR: the ratified #412 disposition governing #405 requires KEEPING the dotnet-* tool names and the presence gate, presence-gating the forward references, and adding stack-qualified framing — not genericizing the references away. The prior commit on this branch genericized the three mode-context marketplace sections into ecosystem-neutral capability descriptions, which is the OPPOSITE of the disposition. Restore each context file's `## Marketplace plugin skills (invoke only when installed)` heading and its exact dotnet-* bullets, and add a lead-in that frames those skills as .NET-ecosystem forward references — invoked only when your stack is .NET and the plugin is installed — with an explicit fallback to the project's own tooling so the generic path stays first-class. This matches the conforming testing (#491) and verification (#526) pattern. No reference removal; every reference stays optional and installed-gated. CHANGELOG 0.7.3 entry rewritten to describe the stack-qualification rework; version bump 0.7.2 -> 0.7.3 (from current origin/main) unchanged. Sibling PR #583 (toolchain, issue #412) is reworked in parallel under the same ruling. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…ard references (#412) Reverses this branch's earlier genericized approach, which implemented the OPPOSITE of the ratified #412 disposition. Per the maintainer ruling (relayed 2026-07-20): the `dotnet-msbuild:*` build-diagnostics skills in `plugins/toolchain/skills/check/context/dotnet.md` are optional FORWARD references to the maintainer's planned `dotnet-*` plugin family — keep the tool names, presence-gate them, add stack framing, and never remove a reference. Restore every `dotnet-msbuild:*` skill name and the `## Marketplace plugin skills for build diagnostics (invoke only when installed)` heading, and add a lead-in that frames the list as .NET-ecosystem skills applicable when the stack is .NET and as forward references to the planned family — invoked only when the plugin is installed, otherwise falling back to the section's own prose remediation and binlog gotcha (the generic path stays first-class). This matches the presence-gated forward-reference convention the merged `testing` (#491) and `verification` (#526) siblings adopted. Framing only: no skill reference is removed, renamed, or genericized, and no command string is altered. The CHANGELOG entry is rewritten to a `### Changed` note describing the reframing; version stays at the 0.4.3 bump. Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
… as .NET forward references (#412) (#583) ## Summary `plugins/toolchain/skills/check/context/dotnet.md` lists `dotnet-msbuild:*` build-diagnostics plugin skills (`build-perf-diagnostics`, `binlog-failure-analysis`, `msbuild-antipatterns`, and others) under a `## Marketplace plugin skills for build diagnostics (invoke only when installed)` presence gate. No `dotnet-*` plugin ships in this marketplace today — but per the **ratified #412 disposition** (maintainer answer 2026-07-19, control-tower relay 2026-07-20) these are **optional FORWARD references** to the maintainer's planned `dotnet-*` plugin family (`.NET` is the primary application stack), not stale references to remove. ## Fix Add a lead-in to the presence-gated list that frames it as .NET-ecosystem skills — applicable when the stack is .NET — and as forward references to the planned `dotnet-*` family: invoke each only when its plugin is installed, otherwise fall back to the section's own prose remediation and binlog gotcha (the generic path stays first-class). **Framing only** — the ratified disposition is *keep tool names + stack framing, presence-gate forward references, no reference removal*: - Every `dotnet-msbuild:*` skill name is kept verbatim. - Nothing is removed, renamed, or genericized. - No command string is altered (`dotnet build /bl:{}` unchanged). This matches the presence-gated forward-reference convention the merged siblings **#491 (`testing`)** and **#526 (`verification`, issue #422)** adopted — each added a ".NET-ecosystem plugin skills — applicable when your stack is .NET" framing line over the same `dotnet-*`/`dotnet-diag:*` lists. Bumps `toolchain` 0.4.2 → 0.4.3 (patch) with a matching `### Changed` CHANGELOG entry. ### Rework note An earlier revision of this branch took the OPPOSITE approach — it genericized the `dotnet-msbuild:*` names into ecosystem-neutral capability descriptions, reading #412's original "replace with capability descriptions" fix-direction literally. The control-tower ruling relayed on this PR (2026-07-20) identified that as the inverse of the ratified disposition and required rework to the #491/#526 keep-tool-names pattern. This revision implements that rework. Sibling PR #580 (issue #405, `implementation`) is governed by the same ruling. ## Verification Cumulative branch diff vs `main` is exactly: the framing lead-in sentence, the version bump, and the CHANGELOG entry — no other change to `dotnet.md`. Validators run on the merged tree (branch rebased current via a `main` merge), all clean, nothing suppressed: ``` markdownlint-cli2 (dotnet.md, CHANGELOG.md) 0 error(s) typos (3 touched files) exit 0 editorconfig-checker (3 touched files) exit 0 check-skill-portability.sh origin/main No unexcused coupling tokens in 1 skill file(s) validate-plugins.sh All plugin manifests and the catalog validated validate-plugin-contracts.mjs 33 setup skills and 1758 plugin files checked ``` Closes #412 ## Related - Closes #412 (source issue — `dotnet-msbuild:*` forward references in `toolchain`'s `dotnet.md`). - #405 (`implementation`) / PR #580 — sibling finding, same pattern, same ruling, fixed in parallel. - #491 (`testing`) and #526 (`verification`, issue #422) — merged siblings whose keep-tool-names + stack-framing pattern this PR now conforms to. 🤖 Generated with [Claude Code](https://claude.com/claude-code) --------- Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…or refs ecosystem-neutral (#580) ## Summary Reworked per the ratified #412 disposition (which governs #405): the implementation plugin's mode-context files KEEP their `dotnet-*` marketplace-skill names and the `## Marketplace plugin skills (invoke only when installed)` presence gate, and gain a stack-qualified lead-in with a generic-path fallback — the #491/#526 conforming pattern. An earlier revision of this branch genericized/renamed those references; that approach was rejected by the maintainer ruling and has been fully reverted (commit 08f8e2c). ## Fix - feature.md / bugfix.md / refactor.md: all six `dotnet-*` skill references retained verbatim under the intact presence-gated heading; a .NET-ecosystem forward-reference lead-in added ("stack-qualified, invoke only when installed, otherwise the generic path applies"). No reference removal, no renames, no command-string changes. - CHANGELOG [0.7.3]: describes the stack-qualification accurately ("retain their dotnet-* names… No reference removal"). - plugin.json 0.7.2 → 0.7.3 (patch, from current main). ## Verification - CI green (all checks incl. portability-lint); the review bot independently re-reviewed the reworked diff and confirmed conformance to the #491/#526 pattern and the #412 disposition. - Zero review threads; `git diff origin/main` on the three context files shows only heading-preserved + lead-in-added. Closes #405 ## Related - #412 (the governing maintainer disposition; sibling PR #583 applies the same ruling to toolchain/dotnet.md) - #491 / #526 (merged precedent pattern for presence-gated stack-qualified references) --------- Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…s (stage 3) (#1790) Stage 3 of #531's portability-lint lane: the **stack/ecosystem opinion class** — baked stack defaults in skills that claim to work in any repository. The operator ratification recorded on this issue fixed the shape: staged per-token rollout, no bulk activation, and per-token activation only after (1) a POSIX-safe rewrite, (2) a green `--all` audit, and (3) a class-scoped guard marker. This lands all three, plus the fixtures the acceptance criteria ask for. ## The staged patterns were inert, not merely dormant `\bdotnet\b` and `\bClean Arch\b` could not have worked. POSIX ERE defines no word-boundary escape, and on gawk as well as mawk/nawk a backslash-b matches a literal **backspace byte** — so those patterns matched nothing. Uncommenting either would have turned the class green while missing every violation, which is the worst outcome a gate has. Both are **replaced** with POSIX-safe spellings rather than uncommented, and a fixture now fails if a backslash-b ever reaches an ACTIVE token again. ## One token activates, on measured evidence **`Clean Arch(itecture)?` is now ACTIVE.** It measures **0 hits / 0 files** through the gate, and a raw grep finds the phrase nowhere in the scanned corpus. It carries no migration debt whatsoever — the one member of this class that can be locked in *before* it arrives, which is exactly the prevention #531 exists for ("prevention instead of hand-fixes plus indefinite recurrence"). A skill whose subject genuinely *is* that architecture declares `portability-scope`, and a fixture pins that escape so the active token cannot strand it. ## The ecosystem literals stay staged, with real numbers Residue measured through the gate **itself** at `c7b0af78`, so guards, per-site annotations, and whole-file scope declarations are already applied to the counts: | Pattern | Hits | Files | |---|---|---| | `[Dd]otnet` | 107 | 40 | | `(^\|[^a-zA-Z0-9_])\.NET([^a-zA-Z0-9_]\|$)` | 63 | 37 | | `\.csproj` | 26 | 17 | | `(^\|[^a-zA-Z0-9_])C#([^a-zA-Z0-9_]\|$)` | 19 | 15 | | `[Bb]lazor` | 13 | 5 | | `\.sln([^a-zA-Z0-9_]\|$)` | 7 | 7 | | `\.razor` | 1 | 1 | | `\*\*/\*\.cs([^a-zA-Z0-9_]\|$)` | 1 | 1 | Their shared blocker is now recorded in the token file rather than left implicit: these are not stray hardcodes, they are skills that legitimately need to **name** an ecosystem and have no seam to read one from. The fix is the `userConfig` seam work on #405 / #406 / #421 / #428 — the lint is the forcing function, not the fix — or a `portability-scope` declaration on genuinely ecosystem-locked files. ## The class-scoped guard The class is excused by **consumer-seam evidence** co-located on the line — never by the branch class's evidence: 1. a `${user_config.*}` read (the seam this class exists to force), anchored to the read syntax so prose *about* a seam does not count; or 2. a neutral `<placeholder>` **and** an explicit illustration lead (`e.g.`, `for example`, `such as`) — the portable idiom already used across this corpus, where the ecosystem token illustrates the placeholder rather than being the value the skill ships. Both halves of (2) are required. A placeholder alone would excuse `run dotnet build in <repo-root>`, which ships the stack and parameterizes only the path; an illustration lead alone would excuse `e.g. run dotnet build`, the bare hardcode wearing a hedge — precisely the generic hedge the token file's staged-class preamble forbids as a guard. Deliberately **not** a marker: "another ecosystem is named on the same line". That multi-ecosystem enumeration shape is the dominant residual in the corpus and reads as correct, but a co-occurrence test cannot distinguish it from a line that hardcodes one stack and merely mentions another. It is recorded in the token file as the shape the next activator must handle, with `portability-ok` as its escape. ## A bug the fixtures caught, worth calling out Class membership is tested with `index()` on literal fragments, **never** a regex match against the pattern text. My first revision used `p ~ /[Dd]otnet/` — copying the branch class's shape — which asks for a `D`-or-`d` followed by `otnet` and therefore does **not** match the pattern *string* `[Dd]otnet`, whose `d` is followed by `]otnet`. It silently returned 0 for every member, leaving the whole class unguarded while every flagging test still passed. It surfaced only because the honored-case fixtures were written, which is why they are part of the deliverable rather than a nice-to-have. `index()` removes the trap instead of re-solving the escaping per pattern. ## Acceptance criteria - **Same lane discipline as Stages 1–2** — patterns are data in `skill-portability-tokens.txt`, staged entries carry measured residue and a named blocker, `assert_staged` pins every staged pattern verbatim so an edit that stops catching its own defect fails loudly. - **Fixtures proving the class is flagged where undeclared and honored where a `userConfig` seam declares it** — both directions, plus prose-about-a-seam failing to stand in for a seam, each half of the placeholder guard failing alone, the per-site and whole-file escapes, and class scoping proven **both** ways (branch-resolution evidence does not excuse an ecosystem hardcode; a seam read does not excuse a branch default). ## Verification - `bash scripts/check-skill-portability.test.sh` — **PASS=65 FAIL=0** (was 44; 21 added) - `bash scripts/check-skill-portability.sh --all` — the only hit repo-wide is one pre-existing branch-class line in `plugins/playbooks/skills/boris/reference/foundations.md`; the newly active Clean-Architecture token contributes **0**. CI is changed-file scoped, so this never red-lines `main`. - `bash scripts/check-shell-portability.sh --paths <the two changed scripts>` — clean. The `\b` assertion is written as the bracket expression `[\]b` so it does not trip the sibling gate that agrees with it. - `bash scripts/check-shell-portability.test.sh` — PASS=215 FAIL=0 (sibling gate unaffected) - `shellcheck` on both changed scripts — clean - `scripts/check-changelog-parity.sh --check-bump origin/main`, `scripts/check-changed-skills.sh` — pass. No plugin version bump: the diff is repo tooling under `scripts/`, not a plugin. - `typos scripts/` exit 0 ## Related - Fixes #714 - #531 — the umbrella lane this is Stage 3 of; Option A (declaration-first, one token-class at a time) was ratified there - #713 — Stage 2, whose declaration reader this class's `portability-scope` exemption rides on - #405, #406, #421, #428 — the `userConfig` seam members this class is the forcing function for; advanced, not retired, by this change - #491, #492 — the .NET-flavored hand-fixes that named this class 🤖 Generated with [Claude Code](https://claude.com/claude-code) <https://claude.ai/code/session_01RhS3T7ShwJgKTrvk2Mvd3C> --------- Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
Summary
The
testingplugin bills as ecosystem-agnostic (seeplugin.json), but its marketplace-skill enrichment handed non-.NET consumers a .NET-only path as the universal one, and it carried one bare unguarded cross-plugin reference — the exact "defect"docs/PLUGIN-PHILOSOPHY.mdnames (every cross-plugin reference must be declared or presence-gated). None of the referenceddotnet-*plugins exist in this marketplace (same pattern as siblings #405/#412/#422). This PR guards the stray reference and reframes the enrichment lists as stack-specific, keeping every reference optional and installed-gated — no hard dependencies added.Fix
Guard adopted verbatim across all six lists:
## Marketplace plugin skills (invoke only when installed).skills/write/context/write.md:78— removed the unguarded inline parenthetical(detection layer: dotnet-test:test-anti-patterns + dotnet-test:assertion-quality plugins)from the per-cycle checklist item. Its two detection-layer skills were moved underwrite's marketplace list (orchestrator preferred moving over dropping).skills/write/context/write.md:138— added the(invoke only when installed)suffix to this heading; it was the only one of the six missing it. Lead-in:These are .NET-ecosystem plugin skills — applicable when your stack is .NET:skills/write/context/organize.md:61(all .NET) — lead-in:These are .NET-ecosystem plugin skills — applicable when your stack is .NET:skills/diagnose/context/investigate.md:55(all .NET) — lead-in:These are .NET-ecosystem plugin skills — applicable when your stack is .NET:skills/diagnose/context/loop.md:110(all .NET) — lead-in:These are .NET-ecosystem plugin skills — applicable when your stack is .NET:skills/plan/SKILL.md:105(mixed:dotnet-test:*+document-skills:webapp-testing) — lead-in:These enrichment skills are ecosystem-specific — the \dotnet-test` skill applies when your stack is .NET; `document-skills:webapp-testing` is stack-agnostic:`skills/run-e2e/context/e2e.md:134(no .NET:cloudflare:web-perf+document-skills:webapp-testing) — lead-in:These enrichment skills are tool-specific — invoke each only when it matches your stack and tooling:(deliberately not a .NET label, since neither entry is .NET).testing0.2.0 → 0.2.1(plugin.json) with a matching## [0.2.1] / ### ChangedCHANGELOG entry, mirroring the 0.1.1/0.1.2 content-fix precedent.The four all-.NET lists share one byte-identical lead-in;
planandrun-e2euse accurate variants because a blanket ".NET" tag would be false there. The consistent through-line is the identical guarded heading + one stack-qualifier lead-in per list.Verification
Ran on the changed markdown, all green, nothing suppressed:
markdownlint-cli2(repo-pinned v0.23.0,.markdownlint-cli2.jsonc) —Linting: 12 file(s) / Summary: 0 error(s).editorconfig-checker(ec) on all 8 changed files — exit 0, no findings.typosonplugins/testing/— exit 0, clean.scripts/validate-plugins.shandscripts/validate-plugin-contracts.mjs— both pass (manifest + catalog + 1586 plugin files). None of the changed files appear inscripts/cross-plugin-source-registry.txt, so no upstream sync is involved.Related
testing).dotnet-*/cloudflare:*unguarded/ecosystem-coupled pattern, tracked separately: implementation: .NET-only marketplace-skill references in every mode context (origin coupling, low severity) #405 (implementation), toolchain: dotnet.md references dotnet-msbuild:* plugin skills that do not exist in this marketplace #412 (toolchain), verification: bare unguarded cross-plugin references to dotnet-*/cloudflare plugins in measure contexts #422 (verification).🤖 Generated with Claude Code