Skip to content

Commit 7935582

Browse files
dmealingclaude
andcommitted
fix(codegen): a retired stub told the reader to revive the capability
Two review findings on the requirement-test renderer and its tests. THE SKIPPED BODY SAID THE OPPOSITE OF WHAT RETIRED MEANS. After `retired` joined the skip set it shared a body with `planned`: "Intended, not built. Write the assertion when this becomes live." For a retired entry that instructs the reader to revive a capability that was deliberately removed — inverting the one guardrail 0.24.2 restored `retired` for. The two skipped statuses mean opposite things and now get opposite bodies; the repo's own harness generator already branched here, so the correct wording existed one file over. THE STATUS TEST ONLY PROVED THE CODE AGREED WITH ITSELF. Deriving SKIPS and FAILS from the same two constants the implementation derives SKIPPED_STATUSES from removed the independent signal: move `retired` into the live-nodes list by mistake and both sides follow, every assertion follows, and every retired entry emits a permanently red stub with the suite still green — the regression the block exists for. The expectations are literal again, with an exhaustiveness check underneath asserting the two lists are exactly the loader's enum and agree with what it means. That keeps the independence without reintroducing the staleness that started this: the lists previously named `abandoned` and `superseded` for a release after the loader stopped accepting them. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01NynrRND6ZUwGvq3ZUTCfxG
1 parent 70e35c6 commit 7935582

3 files changed

Lines changed: 37 additions & 3 deletions

File tree

‎server/typescript/packages/codegen-ts/src/templates/requirement-test.ts‎

Lines changed: 11 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -13,6 +13,7 @@
1313
import {
1414
REQUIREMENT_STATUSES,
1515
REQUIREMENT_STATUSES_REQUIRING_LIVE_NODES,
16+
REQUIREMENT_STATUS_RETIRED,
1617
} from "@metaobjectsdev/metadata";
1718
import { GENERATED_HEADER } from "../constants.js";
1819
import type { RequirementView, ResolvedClaim } from "../requirement-walk.js";
@@ -117,8 +118,17 @@ export function renderRequirementTest(a: RequirementTestArgs): string {
117118
// A `live` or `partial` stub asserts FAILURE until someone writes the real
118119
// assertion over it. `expect.unreachable` names the requirement in the failure
119120
// message, so a red run says which claim is unproven rather than just "failed".
121+
// The two skipped statuses mean OPPOSITE things, so they must not share a body.
122+
// `planned` is intended-not-built and the instruction is to write the assertion when
123+
// it becomes live. `retired` is built-then-deliberately-removed, and telling a reader
124+
// to write it "when this becomes live" instructs them to revive the capability —
125+
// inverting the one guardrail 0.24.2 restored `retired` for. The repo's own harness
126+
// generator already branches here; the shipped renderer now does too.
120127
const body = skipped
121-
? ` // Intended, not built. Write the assertion when this becomes live.`
128+
? a.view.status === REQUIREMENT_STATUS_RETIRED
129+
? ` // Retired: this capability was deliberately removed and must not be rebuilt.\n` +
130+
` // If you assert anything here, assert that it STAYS removed.`
131+
: ` // Intended, not built. Write the assertion when this becomes live.`
122132
: ` expect.unreachable(\n` +
123133
` "unimplemented requirement stub: ${testName} — " +\n` +
124134
` "replace this with an assertion that fails when: ${forStringLiteral(a.counterexample)}",\n` +

‎server/typescript/packages/codegen-ts/test/requirement-test-escaping.test.ts‎

Lines changed: 21 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -13,6 +13,7 @@ import { describe, test, expect } from "bun:test";
1313
import {
1414
REQUIREMENT_STATUSES,
1515
REQUIREMENT_STATUSES_REQUIRING_LIVE_NODES,
16+
type RequirementStatus,
1617
} from "@metaobjectsdev/metadata";
1718
import { mkdtempSync, writeFileSync, rmSync } from "node:fs";
1819
import { tmpdir } from "node:os";
@@ -156,8 +157,26 @@ describe("a retired requirement does not redden the suite forever", () => {
156157
// replaced them — passing throughout, and pinning the defect instead of catching it.
157158
// The split under test is the semantic one: does this status claim the capability
158159
// works now, or not.
159-
const SKIPS = REQUIREMENT_STATUSES.filter((s) => !REQUIREMENT_STATUSES_REQUIRING_LIVE_NODES.includes(s));
160-
const FAILS = REQUIREMENT_STATUSES.filter((s) => REQUIREMENT_STATUSES_REQUIRING_LIVE_NODES.includes(s));
160+
// LITERAL, not derived. The implementation computes its skip set as
161+
// `REQUIREMENT_STATUSES minus REQUIREMENT_STATUSES_REQUIRING_LIVE_NODES`; a test that
162+
// computes the identical expression proves only that the code agrees with itself.
163+
// Move `retired` into the live-nodes list by mistake and both would follow, every
164+
// assertion below would follow, and every retired entry would emit a permanently red
165+
// stub with the suite still green — the exact regression this block exists for.
166+
//
167+
// Written out, the expectations are independent. The exhaustiveness check underneath
168+
// is what stops them going stale, which is the failure this file had before: it named
169+
// `abandoned` and `superseded` for a release after the loader stopped accepting them.
170+
const SKIPS: readonly RequirementStatus[] = ["planned", "retired"];
171+
const FAILS: readonly RequirementStatus[] = ["live", "partial"];
172+
173+
test("the two lists together are exactly the loader's status enum", () => {
174+
expect([...SKIPS, ...FAILS].sort()).toEqual([...REQUIREMENT_STATUSES].sort());
175+
// And the split agrees with what the enum MEANS, checked once rather than assumed
176+
// for every case below.
177+
for (const s of FAILS) expect(REQUIREMENT_STATUSES_REQUIRING_LIVE_NODES).toContain(s);
178+
for (const s of SKIPS) expect(REQUIREMENT_STATUSES_REQUIRING_LIVE_NODES).not.toContain(s);
179+
});
161180

162181
for (const status of SKIPS) {
163182
test(`${status} emits a skipped test`, () => {

‎server/typescript/packages/codegen-ts/test/requirement-test-render.test.ts‎

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -51,6 +51,11 @@ describe("renderRequirementTest", () => {
5151
});
5252
expect(src).toContain("test.skip");
5353
expect(src).not.toContain("expect.unreachable");
54+
// And it must not read as an instruction to REVIVE the capability. `planned` and
55+
// `retired` are both skipped and mean opposite things, so sharing a body told a
56+
// reader of a retired stub to write the assertion "when this becomes live".
57+
expect(src).toContain("must not be rebuilt");
58+
expect(src).not.toContain("becomes live");
5459
});
5560

5661
test("a PARTIAL stub fails like live, and carries its disposition and tracking", () => {

0 commit comments

Comments
 (0)