Skip to content

Commit e6a660b

Browse files
committed
fix(cli): blame a mismatched fixture id only when it hid every fixture
`sg-fixture-id-matches-rule` was attributed from `fixturesExcludedById` alone. That flag is rule-wide — true if ANY discovered test file was skipped for carrying another rule's `id:` — while the shortfall it was gating is a per-coverage-state fact summed over every file the rule owns. A rule-wide boolean cannot decide a per-coverage-state question, and the two come apart as soon as a rule ships more than one test file. The reachable case: `<id>-a-test.yml` with the right id but only a `valid:` bucket, beside `<id>-b-test.yml` with a wrong id, also `valid:`-only. Combined coverage is `valid-only`, so the shortfall is "has only valid: fixtures — half a claim", and the exclusion flag attached that message to the id constraint. The author would have been sent to fix an id in a file they may not have been thinking about, and fixing it would not have moved the shortfall at all: neither file has an `invalid:` case. The real defect — that nothing shows the rule fires — goes unmentioned behind an unrelated diagnosis. The gate now also requires `fixtures === "none"`, which is the state the constraint's own rationale describes: an exclusion making a rule read as having shipped no fixtures. Both halves are needed, and each rules out a different wrong answer — without the flag, a rule with no cases is told to hunt for a mismatched id; without the coverage check, a rule with a partial-but-counted fixture is told the same thing.
1 parent 71f896e commit e6a660b

4 files changed

Lines changed: 67 additions & 8 deletions

File tree

‎openspec/changes/reference-fixture-cases/design.md‎

Lines changed: 6 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -192,6 +192,9 @@ honestly. It surfaces as the same "has no fixtures" shortfall a rule with no
192192
cases at all produces, and those two want different advice: one author has
193193
written nothing, the other has a file sitting right there that is silently not
194194
counted. `fixtureCoverage` already skips a test file whose own `id:` names
195-
another rule, so it now reports that it did, and only that case is attributed.
196-
Without it the constraint could only have been attributed by guessing, which
197-
this design rejects everywhere else.
195+
another rule, so it now reports that it did. That report is rule-wide, so it is
196+
required alongside a coverage of `none` rather than on its own: an excluded file
197+
sitting beside a counted one leaves a `valid-only` shortfall that fixing the id
198+
would not repair, and naming the constraint there would send the author to an
199+
unrelated mismatch. Without both, the constraint could only have been attributed
200+
by guessing, which this design rejects everywhere else.

‎packages/cli/src/rules/inspect.ts‎

Lines changed: 22 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -356,11 +356,28 @@ export async function testOneRule(
356356
":"
357357
);
358358
if (shortfall !== undefined) {
359-
// Attributed only when a fixture file was actually excluded by its own
360-
// `id:`. The same shortfall is produced by a rule that simply has no
361-
// cases, and calling that `sg-fixture-id-matches-rule` would send an
362-
// author looking for a mismatched id in a file they never wrote.
363-
if (result.tests.fixturesExcludedById === true) {
359+
// Both halves are required, because each rules out a different way of
360+
// being wrong about the same shortfall.
361+
//
362+
// `fixturesExcludedById` says a file the rule owns by NAME was skipped
363+
// for carrying another rule's `id:`. Without it, a rule that simply has
364+
// no cases would be told to look for a mismatched id in a file it never
365+
// wrote.
366+
//
367+
// `fixtures === "none"` says the shortfall is the one this constraint
368+
// describes: exclusion making a rule read as having shipped NOTHING. The
369+
// flag alone cannot say that, because it is rule-wide while coverage is
370+
// the sum over every discovered file. A rule shipping `<id>-a-test.yml`
371+
// with the right id but only a `valid:` bucket, beside an excluded
372+
// `<id>-b-test.yml` that is also `valid:`-only, lands on `valid-only`
373+
// with the flag set — and fixing the id would not fix that shortfall,
374+
// since neither file has an `invalid:` case. Attributing it would send
375+
// the author to an unrelated id mismatch and leave the missing half of
376+
// the claim unmentioned.
377+
if (
378+
result.tests.fixturesExcludedById === true &&
379+
result.tests.fixtures === "none"
380+
) {
364381
violate(
365382
{ errors, violations },
366383
"sg-fixture-id-matches-rule",

‎packages/cli/src/rules/verify.ts‎

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -105,6 +105,12 @@ export interface TestLayerResult extends LayerResult {
105105
* different advice, and only this can tell them apart — so the attribution
106106
* `test` reports is decided here rather than guessed from a shortfall
107107
* message that both states produce.
108+
*
109+
* Necessary for that attribution but not sufficient for it. This is
110+
* rule-wide, while `fixtures` is the sum over every discovered file, so an
111+
* excluded file can sit beside a counted one and leave coverage at
112+
* `valid-only` — a shortfall fixing the id would not repair. `test` requires
113+
* `fixtures === "none"` alongside this before naming the constraint.
108114
*/
109115
fixturesExcludedById?: boolean;
110116
}

‎packages/cli/test/constraints.test.ts‎

Lines changed: 33 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -293,6 +293,39 @@ describe("every documented constraint is a check that still fires", () => {
293293
).not.toContain("sg-fixture-id-matches-rule");
294294
});
295295

296+
it("does not blame an id mismatch for a shortfall fixing the id would not fix", async () => {
297+
// `fixturesExcludedById` is rule-wide, but coverage is the sum over every
298+
// discovered file, so the two can disagree. Here one file carries the right
299+
// id and one carries the wrong one, and NEITHER has an `invalid:` bucket:
300+
// combined coverage is `valid-only`, not `none`. Correcting the id would
301+
// leave that shortfall exactly where it is, so attributing it to
302+
// `sg-fixture-id-matches-rule` would send the author to an unrelated
303+
// mismatch while the missing half of the claim goes unmentioned. This test
304+
// catches a gate that reads only the flag.
305+
const ruleId = "probe-rule";
306+
const directory = join(project, ".taskless/rules/sg", ruleId);
307+
await mkdir(join(directory, ".tests"), { recursive: true });
308+
await writeFile(join(directory, `${ruleId}.yml`), stringify(VALID_RULE));
309+
// Both filenames match the `<ruleId>-*-test.yml` discovery pattern, so both
310+
// are found; only the second is excluded, and only by its `id:`.
311+
await writeFile(
312+
join(directory, ".tests", `${ruleId}-a-test.yml`),
313+
"id: probe-rule\nvalid:\n - const a = 1;\n"
314+
);
315+
await writeFile(
316+
join(directory, ".tests", `${ruleId}-b-test.yml`),
317+
"id: some-other-rule\nvalid:\n - const b = 2;\n"
318+
);
319+
320+
const result = await testOneRule(project, { engine: "sg", ruleId });
321+
expect(result.ok, result.errors.join("; ")).toBe(false);
322+
expect(result.errors.join("\n")).toMatch(/only valid: fixtures/);
323+
expect(result.errors.join("\n")).toMatch(/half a claim/);
324+
expect(
325+
result.violations.map((violation) => violation.constraintId)
326+
).not.toContain("sg-fixture-id-matches-rule");
327+
});
328+
296329
it("says which command enforces each, since that decides eval order", () => {
297330
for (const constraint of RULE_CONSTRAINTS) {
298331
expect(["verify", "test"]).toContain(constraint.enforcedBy);

0 commit comments

Comments
 (0)