chore: address Copilot review round 15
- The CI wiring guard matched a substring, and last round's justification for that was wrong: it accepts a longer script name (`npm run verify:skills:cli:disabled`) and a mere mention (`echo npm run verify:skills:cli`), either of which lets the real validator be removed while the guard stays green. `runsCommand` now splits the script on the separators that begin a new command and requires a segment to equal the invocation exactly, with both counterexamples as regression cases. - `local-dev`'s own description and its AGENTS.md index row both still promised "where a new dependency must be declared" — the rules round 13 moved out of it entirely. Both now describe what the skill actually carries: the reasoning behind those rules. Recorded listing share updated to 1,199/4,000. Signed-off-by: cliffhall <cliff@futurescale.com>
This commit is contained in:
parent
0798ecf5fa
commit
3905ceef06
4 changed files with 71 additions and 8 deletions
|
|
@ -1,6 +1,6 @@
|
|||
---
|
||||
name: local-dev
|
||||
description: Getting the Inspector running locally and adding dependencies to it. Which install to run and which script starts each client (web, cli, tui, launcher); why v2 is not an npm workspace; how the @inspector/core alias works; and where a new dependency must be declared — root vs client, dependencies vs devDependencies, and the bundler `external` lists a root package must also appear in.
|
||||
description: Getting the Inspector running locally, and the reasoning behind its dependency rules. Which install to run and which script starts each client (web, cli, tui, launcher); why v2 is not an npm workspace; how the @inspector/core alias works; and why the placement rules in AGENTS.md are what they are — what each one is defending against, what it looked like when it was violated, and how to tell you have hit one.
|
||||
disable-model-invocation: false
|
||||
---
|
||||
|
||||
|
|
|
|||
|
|
@ -16,7 +16,7 @@ users invoke them by name.
|
|||
|
||||
| Skill | Covers | How it loads |
|
||||
| --- | --- | --- |
|
||||
| [`local-dev`](.claude/skills/local-dev/SKILL.md) | Install and run each client; the `@inspector/core` alias; **where a new dependency is declared** (root vs client, `dependencies` vs `devDependencies`, the bundler `external` lists, `overrides`) | Model-invoked |
|
||||
| [`local-dev`](.claude/skills/local-dev/SKILL.md) | Install and run each client; the `@inspector/core` alias; and the **reasoning** behind Dependency placement below — what each rule defends against and how to tell you have hit one (the rules themselves stay here) | Model-invoked |
|
||||
| [`project-structure`](.claude/skills/project-structure/SKILL.md) | Which client owns which surface, what is in `core/`, where a new file belongs | Model-invoked (not user-invocable) |
|
||||
| [`testing`](.claude/skills/testing/SKILL.md) | Where a test file goes, which command runs it, the tiers, clearing the coverage gate, `renderWithMantine` | Model-invoked |
|
||||
| [`issue-create`](.claude/skills/issue-create/SKILL.md) | The five-step create flow: version label, type label, milestone, board card, Status + Priority | `/issue-create` |
|
||||
|
|
@ -201,7 +201,7 @@ that from happening:
|
|||
overflows, and drops the least-invoked entries **first** — which are exactly the
|
||||
model-invoked skills that must fire on their own. `verify:skills` prints the
|
||||
current cost against the budget recorded in `scripts/lib/skill-manifest.mjs`
|
||||
(1,173/4,000 characters as of this writing) and fails when it is exceeded. Raise
|
||||
(1,199/4,000 characters as of this writing) and fails when it is exceeded. Raise
|
||||
the budget deliberately, or tighten a description; each entry is capped at 1,536
|
||||
characters regardless, so **put the key use case first**.
|
||||
|
||||
|
|
|
|||
|
|
@ -24,7 +24,11 @@ import { tmpdir } from "node:os";
|
|||
import path from "node:path";
|
||||
import { fileURLToPath } from "node:url";
|
||||
|
||||
import { checkWiring, ciRunsUnconditionally } from "./verify-skills.mjs";
|
||||
import {
|
||||
checkWiring,
|
||||
ciRunsUnconditionally,
|
||||
runsCommand,
|
||||
} from "./verify-skills.mjs";
|
||||
|
||||
const SCRIPT = path.join(
|
||||
path.dirname(fileURLToPath(import.meta.url)),
|
||||
|
|
@ -342,3 +346,41 @@ test("the repository's workflow runs the validator unconditionally", () => {
|
|||
true,
|
||||
);
|
||||
});
|
||||
|
||||
test("runsCommand matches an exact invocation, not a substring", () => {
|
||||
const C = "npm run verify:skills:cli";
|
||||
for (const script of [
|
||||
C,
|
||||
`npm run validate && ${C}`,
|
||||
`npm run validate\n${C}\n`,
|
||||
`${C} ; npm run something`,
|
||||
]) {
|
||||
assert.equal(runsCommand(script, C), true, script);
|
||||
}
|
||||
});
|
||||
|
||||
test("runsCommand rejects a longer script name or a mere mention", () => {
|
||||
// Both would let the real validator be removed while the wiring guard stayed
|
||||
// green — the counterexamples to the substring test this replaced.
|
||||
const C = "npm run verify:skills:cli";
|
||||
for (const script of [
|
||||
"npm run verify:skills:cli:disabled",
|
||||
"echo npm run verify:skills:cli",
|
||||
"npm run verify:skills",
|
||||
"npm run verify:skills:cli --silent",
|
||||
"",
|
||||
]) {
|
||||
assert.equal(runsCommand(script, C), false, script);
|
||||
}
|
||||
});
|
||||
|
||||
test("ciRunsUnconditionally inherits the exact-invocation rule", () => {
|
||||
const C = "npm run verify:skills:cli";
|
||||
assert.equal(
|
||||
ciRunsUnconditionally(
|
||||
"jobs:\n build:\n steps:\n - run: npm run verify:skills:cli:disabled\n",
|
||||
C,
|
||||
),
|
||||
false,
|
||||
);
|
||||
});
|
||||
|
|
|
|||
|
|
@ -60,6 +60,30 @@ export function skillDirs(
|
|||
.sort();
|
||||
}
|
||||
|
||||
/**
|
||||
* Whether a shell script invokes exactly `command`.
|
||||
*
|
||||
* A substring test is not enough, and my own justification for one was wrong:
|
||||
* it accepts a LONGER script name (`npm run verify:skills:cli:disabled`) and a
|
||||
* script merely named by another command (`echo npm run verify:skills:cli`),
|
||||
* either of which would let the real validator be removed while this guard
|
||||
* stayed green (Copilot).
|
||||
*
|
||||
* So the script is split on the separators that start a new command, and a
|
||||
* segment must equal the invocation exactly. That is precise for the commands
|
||||
* this guard checks, which take no arguments; a command that did would need
|
||||
* prefix matching with an argument boundary instead.
|
||||
*
|
||||
* @param {string} script A step's `run:` block.
|
||||
* @param {string} command e.g. `npm run verify:skills:cli`.
|
||||
*/
|
||||
export function runsCommand(script, command) {
|
||||
return script
|
||||
.split(/[\n;|&]+/)
|
||||
.map((segment) => segment.trim())
|
||||
.some((segment) => segment === command);
|
||||
}
|
||||
|
||||
/**
|
||||
* Whether the workflow has a step that runs `command` on every push.
|
||||
*
|
||||
|
|
@ -90,10 +114,7 @@ export function ciRunsUnconditionally(workflowText, command) {
|
|||
for (const step of steps) {
|
||||
if (step === null || typeof step !== "object") continue;
|
||||
if ("if" in step) continue;
|
||||
// A plain substring is enough and cannot over-match: the command is a
|
||||
// full `npm run <script>` invocation, so `verify:skills` does not match
|
||||
// a search for `verify:skills:cli`.
|
||||
if (typeof step.run === "string" && step.run.includes(command))
|
||||
if (typeof step.run === "string" && runsCommand(step.run, command))
|
||||
return true;
|
||||
}
|
||||
}
|
||||
|
|
|
|||
Loading…
Reference in a new issue