diff --git a/.changeset/citation-aware-claim-tie-4607.md b/.changeset/citation-aware-claim-tie-4607.md new file mode 100644 index 000000000..45bc656e4 --- /dev/null +++ b/.changeset/citation-aware-claim-tie-4607.md @@ -0,0 +1,29 @@ +--- +--- + +Tooling only — a CI guard and its own test suite, no package source, so no release. + +`check:spec-symbols` rule 2 flags an exported declaration whose doc comment claims +`@objectstack/spec` alignment while the declaration references nothing spec-bound. +That tie test was symbol-AGNOSTIC: it asked whether the declaration referenced ANY +spec-bound identifier, so a claim about symbol X passed on an incidental reference +to an unrelated symbol Y. + +`FeedItem` (packages/types/src/views.ts) was the live specimen (objectui#4607). It +cited `FeedItemSchema`, removed from `@objectstack/spec/data` in the 16.0.0 major, +and never appeared in a single gate run — purely because one member is typed +`FeedItemType`, the one feed symbol that removal kept. It sat four lines from a +section banner making the same claim, in the same file as four declarations that +WERE flagged, and it was found by reading the file rather than by any run. + +The tie is now judged against the symbols the claim CITES: when a claim names +symbols and the installed spec exports none of them, the declaration is flagged +whatever else it references. A claim naming at least one live symbol stays governed +by the tie test unchanged — a claim-vs-tie mismatch among LIVE symbols is a +documented non-goal, since it needs a name-relatedness allowance for the legitimate +`type: FeedItemType` shape. + +Measured repo-wide with the sharpened rule: the hidden population is zero. Of the +three declarations that carry a claim and pass the tie test, two cite only live +symbols and one is mixed, so no verdict in the tree changes and no ledger entry +moves (CLAIM_ALLOW 2, CLAIM_DEBT 18 in 5 packages, before and after). diff --git a/scripts/__tests__/check-spec-symbol-derivation.test.ts b/scripts/__tests__/check-spec-symbol-derivation.test.ts index 5a78bcbc2..0adf82a4c 100644 --- a/scripts/__tests__/check-spec-symbol-derivation.test.ts +++ b/scripts/__tests__/check-spec-symbol-derivation.test.ts @@ -53,10 +53,29 @@ function withFixture(files: Record, run: (paths: Record>([ ['NavigationConfig', new Set(['@objectstack/spec/ui'])], ['NavigationConfigSchema', new Set(['@objectstack/spec/ui'])], + ['ListView', new Set(['@objectstack/spec/ui'])], + // Kept when the 16.0.0 major removed the rest of the feed surface — the live + // half of the objectui#4607 specimen. + ['FeedItemType', new Set(['@objectstack/spec/data'])], + ['FeedFilterMode', new Set(['@objectstack/spec/data'])], ]); const scan = (file: string) => scanFileForClaims(file, SPEC_NAMES); @@ -309,6 +328,215 @@ export type { InternalNavigationConfig }; }); }); +// ── The tie is judged against the symbols the claim CITES (objectui#4607) ──── + +/** + * The tie test above is symbol-AGNOSTIC: it asks whether the declaration + * references ANY spec-bound identifier. So a claim about symbol X passed on an + * incidental reference to unrelated symbol Y — and the more spec-integrated a + * declaration was, the weaker the check on its prose became. + * + * `FeedItem` (packages/types/src/views.ts) was the live specimen: it cited + * `FeedItemSchema`, removed from `@objectstack/spec/data` in the 16.0.0 major, + * and never appeared in a single gate run because one member is typed + * `FeedItemType` — the one feed symbol the removal kept. Measured on + * origin/main@92876f097 before this change, the scanner returned `0 findings` + * for the fixture below; it returns the finding asserted here after it. + */ +describe('a claim citing only symbols the spec does not export is flagged despite a live tie', () => { + /** The objectui#4607 specimen: dangling citation, live tie to a DIFFERENT symbol. */ + const FEED_ITEM_SPECIMEN = ` +import type { FeedItemType } from '@objectstack/spec/data'; + +/** + * FeedItem — A single item in the unified activity feed. + * Aligned with @objectstack/spec FeedItemSchema. + */ +export interface FeedItem { + id: string; + type: FeedItemType; + body?: string; + createdAt: string; +} +`; + + it('(a) flags the specimen, and names the symbol the spec has dropped', () => { + withFixture({ 'views.ts': FEED_ITEM_SPECIMEN }, ({ 'views.ts': file }) => { + const found = scan(file); + expect(found).toHaveLength(1); + expect(found[0].name).toBe('FeedItem'); + expect(found[0].phrase.toLowerCase()).toBe('aligned with'); + expect(found[0].dangling).toEqual(['FeedItemSchema']); + }); + }); + + it('(a) the tie itself is real — only the CITATION differs from a green declaration', () => { + // The discrimination proof's other half, and the reason this rule is not + // just "flag anything with a retired name in the comment": the fixture is + // byte-identical to the one above except that the claim cites the symbol the + // declaration is actually tied to. Same import, same member, same claim + // phrase — green. + withFixture( + { 'views.ts': FEED_ITEM_SPECIMEN.replace('FeedItemSchema', 'FeedItemType') }, + ({ 'views.ts': file }) => expect(scan(file)).toEqual([]) + ); + }); + + it('(c) a claim citing a LIVE symbol the declaration is tied to stays green', () => { + withFixture( + { + 'tied.ts': ` +import type { NavigationConfig } from '@objectstack/spec/ui'; + +/** Navigation node. Aligned with @objectstack/spec NavigationConfig. */ +export interface TiedNavigationNode { + navigation?: NavigationConfig; + columns?: string[]; +} +`, + }, + ({ 'tied.ts': file }) => expect(scan(file)).toEqual([]) + ); + }); + + it('(d) a claim citing a LIVE symbol while tied to a DIFFERENT live one stays green', () => { + // The KNOWN NON-GOAL recorded on objectui#4607. This is a claim-vs-tie + // MISMATCH, not a dangling citation: both symbols exist, so the claim points + // at something real and the reader can check it. Judging these needs a + // name-relatedness allowance for the legitimate `type: FeedItemType` shape, + // where citation and tie are genuinely different-but-related symbols — a + // different instrument, deliberately not built here. + withFixture( + { + 'mismatch.ts': ` +import type { NavigationConfig } from '@objectstack/spec/ui'; + +/** List view node. Aligned with @objectstack/spec ListView. */ +export interface MismatchedListViewNode { + navigation?: NavigationConfig; + columns?: string[]; +} +`, + }, + ({ 'mismatch.ts': file }) => expect(scan(file)).toEqual([]) + ); + }); + + it('a claim naming no symbol at all is still governed by the tie test', () => { + withFixture( + { + 'unnamed.ts': ` +import type { NavigationConfig } from '@objectstack/spec/ui'; + +/** Navigation node, aligned with @objectstack/spec. */ +export interface UnnamedClaimNode { + navigation?: NavigationConfig; +} +`, + }, + ({ 'unnamed.ts': file }) => expect(scan(file)).toEqual([]) + ); + }); + + it('with no spec export set to check against, the rule stays out of the way', () => { + // "Dangling" is a statement about the installed spec. Given no export set, + // every citation would read as dangling and the rule would flag every claim + // in the repo at once — a verdict manufactured from ignorance of the spec. + withFixture({ 'views.ts': FEED_ITEM_SPECIMEN }, ({ 'views.ts': file }) => { + expect(scanFileForClaims(file, new Map())).toEqual([]); + }); + }); +}); + +// ── The retirement-record idiom must never re-trigger the gate (#4597/#4606) ─ + +describe('(b) an honest provenance note is not a claim', () => { + /** + * PR #4606 rewrote eight comments that cited retired spec symbols so they + * RECORD the retirement instead of vouching for the symbol. That idiom names + * the dead symbol on purpose — it is the provenance a reader needs — so a rule + * that turned on it would punish exactly the fix it is meant to produce. What + * makes these green is that they claim nothing: no alignment phrase sits next + * to a `@objectstack/spec` mention, so there is no claim to have anything + * behind. + */ + it('the @object-ui/i18n idiom — "authored against the protocol\'s X, retired in …"', () => { + withFixture( + { + 'spec-formatters.ts': ` +/** + * Plural forms for a single translation key, in CLDR categories. + * + * Local shape — authored against the protocol's \`PluralRuleSchema\`, retired in + * 17.0.0-rc.6 (see the module doc), so there is nothing upstream to derive from. + */ +export interface SpecPluralRule { + key: string; + zero?: string; + one?: string; + other: string; +} +`, + }, + ({ 'spec-formatters.ts': file }) => expect(scan(file)).toEqual([]) + ); + }); + + it('the @object-ui/types idiom — "its cited X went with the 16.0.0 feed removal"', () => { + withFixture( + { + 'views.ts': ` +import type { FeedItemType } from '@objectstack/spec/data'; + +/** + * FeedItem — A single item in the unified activity feed. + * + * Local shape; its cited \`FeedItemSchema\` went with the 16.0.0 feed removal + * (see the section banner). Only \`type\` is still protocol-bound, through the + * \`FeedItemType\` import above. + */ +export interface FeedItem { + id: string; + type: FeedItemType; + createdAt: string; +} +`, + }, + ({ 'views.ts': file }) => expect(scan(file)).toEqual([]) + ); + }); + + /** + * The fixtures above are a copy of the idiom; these two are the REAL FILES. + * A copy can drift from what shipped, and the guarantee #4607 owes #4606 is + * about the tree, not about a paraphrase of it: the sharpened rule must be + * green on the very comments that card wrote. + * + * If one of these ever fails, read it as a defect in the RULE first. The + * rewordings are the honest retirement record the guard exists to produce, so + * a rule that flags them has turned on its own remedy. + */ + const REWORDED_BY_4606 = [ + '../../packages/types/src/views.ts', + '../../packages/i18n/src/utils/spec-formatters.ts', + ]; + + it.each(REWORDED_BY_4606)('stays green on the real tree: %s', (rel) => { + expect(scan(path.join(here, rel))).toEqual([]); + }); + + it('and those files still carry the provenance the pin is about', () => { + // Deleting the comments outright would satisfy the pin above while throwing + // away the record. Pin the idiom's load-bearing phrases too. + const views = fs.readFileSync(path.join(here, REWORDED_BY_4606[0]), 'utf8'); + const i18n = fs.readFileSync(path.join(here, REWORDED_BY_4606[1]), 'utf8'); + expect(views).toContain('went with the 16.0.0 feed removal'); + expect(views).toContain('`FeedItemType` and `FeedFilterMode` were deliberately KEPT'); + expect(i18n).toContain('authored against the protocol'); + expect(i18n).toContain('retired in'); + }); +}); + // ── The claim detector itself ──────────────────────────────────────────────── describe('findClaim', () => { @@ -351,6 +579,47 @@ describe('findClaim', () => { it('every documented pattern is case-insensitive', () => { for (const pattern of CLAIM_PATTERNS) expect(pattern.flags, String(pattern)).toContain('i'); }); + + it("takes cited symbols from the mention's own sentence, not the next one", () => { + // Live case: `ActionDef` (packages/core/src/actions/ActionRunner.ts) reads + // "…mirroring `@objectstack/spec`'s `ActionSchema`. Open key set on a data + // bag is correct". `Open` opens the NEXT sentence and is prose, not a + // citation. This was harmless while `symbols` only decorated the failure + // message; since objectui#4607 it decides whether the tie test applies, so a + // scraped prose word could make a green declaration read as citing nothing + // but symbols the spec does not export. + const claim = findClaim( + "/** A declared metadata contract mirroring `@objectstack/spec`'s `ActionSchema`. Open key set on a data bag is correct. */" + ); + expect(claim).not.toBeNull(); + expect(claim!.symbols).toEqual(['ActionSchema']); + }); + + it('still reads a symbol followed by a dotted member path', () => { + // The truncation must not fire on `ListView.navigation` — the `.` there is a + // member separator, not a sentence end, which is why the test is + // "terminator followed by whitespace or end", the same shape the + // claim/mention sentence test uses. + expect(findClaim('/** Aligned with @objectstack/spec ListView.navigation. */')!.symbols).toEqual(['ListView']); + }); + + it('KNOWN LIMITATION: a sentence that never terminates still donates prose words', () => { + // `PageNodeSchema` (packages/types/src/layout.ts) is the live instance: the + // claim line ends without punctuation and the next line continues "This is + // the SDUI NODE, not the authored page DOCUMENT", so `normalizeDoc` joins + // them into ONE sentence and the window scrapes three prose words. + // + // Pinned rather than fixed, and it costs nothing today: the claim also cites + // `PageSchema`, which the spec DOES export, so the declaration is governed + // by the tie test exactly as before. It would only matter for a comment that + // cites no real symbol at all AND has an incidental live tie — measured at + // zero instances repo-wide (objectui#4607). Tightening it further means + // deciding what a citation LOOKS like, which is a different instrument. + const claim = findClaim( + '/**\n * Aligned with @objectstack/spec PageSchema\n *\n * This is the SDUI NODE, not the authored page DOCUMENT\n */' + ); + expect(claim!.symbols).toEqual(['PageSchema', 'This', 'SDUI', 'NODE']); + }); }); describe('normalizeDoc', () => { diff --git a/scripts/check-spec-symbol-derivation.mjs b/scripts/check-spec-symbol-derivation.mjs index f1e2adb53..9233b59fa 100644 --- a/scripts/check-spec-symbol-derivation.mjs +++ b/scripts/check-spec-symbol-derivation.mjs @@ -99,6 +99,44 @@ * so "Aligned with @objectstack/spec ReactionSchema" on a `` * is a statement about what it draws. * + * ── The tie is judged against the symbols the claim CITES (objectui#4607) ──── + * The tie test above asks "does this declaration reference ANY spec-bound + * identifier". That question is symbol-AGNOSTIC, so a claim about symbol X + * passed on an incidental reference to an unrelated symbol Y. + * + * The measured specimen: `FeedItem` (packages/types/src/views.ts) carried + * "Aligned with @objectstack/spec FeedItemSchema" while `FeedItemSchema` had + * been removed from `@objectstack/spec/data` in the 16.0.0 major — and it never + * appeared in a single gate run, purely because one member is typed + * `FeedItemType`, the one feed symbol that survived the removal. It sat four + * lines from a section banner making the same claim, in the same file as four + * declarations that WERE flagged, and it was found by reading the file rather + * than by any run of this script. + * + * Note the asymmetry, which is what made it expensive: the more spec-integrated + * a declaration is, the weaker the check on its prose became. A fully + * hand-written fork got its claim scrutinised; one importing a single live spec + * type for one member did not. + * + * Hence the fourth precision rule, deliberately narrow: + * + * When the claim NAMES symbols and EVERY one of them is absent from the + * installed spec's export set, the declaration is flagged whatever else it + * references. A tie to a DIFFERENT symbol is not something behind THIS claim. + * + * Two limits on it, both deliberate: + * + * - A claim naming at least one LIVE symbol stays governed by the tie test + * above, unchanged. A claim citing live `A` while tied to live `B` is a + * claim-vs-tie MISMATCH, and judging those is a KNOWN NON-GOAL of this rule + * (objectui#4607). It would need a name-relatedness allowance for the + * legitimate `type: FeedItemType` shape — where the citation and the tie are + * genuinely different-but-related symbols — and that allowance is a + * different instrument, not a tightening of this one. + * - Given no spec export set to check against, nothing can be known to dangle + * and this rule stays out of the way entirely. A verdict fabricated from + * ignorance of the spec would flag every citation in the repo at once. + * * `SpecAuthoredInput` (@object-ui/react) counts as derivation evidence by name: * its entire purpose is to bind a local type to a spec schema's authoring input. * @@ -685,7 +723,12 @@ const SPEC_MENTION = "@objectstack/spec"; * Does this doc comment CLAIM alignment with `@objectstack/spec`? * Returns `{ phrase, distance, symbols }` or null. `symbols` are the capitalised * identifiers named right after a spec mention — the symbols the claim points - * at, used only to sharpen the failure message. + * at. + * + * `symbols` is load-bearing, not decoration: since objectui#4607 it decides + * whether the tie test applies at all (a claim every one of whose cited symbols + * is absent from the spec is flagged regardless of an incidental tie), as well + * as sharpening the failure message it always did. */ export function findClaim(docText) { const text = normalizeDoc(docText); @@ -713,7 +756,18 @@ export function findClaim(docText) { for (const m of mentions) { // `@objectstack/spec/ui ReactionSchema` — take the identifiers the claim // names just after the mention (its subpath included, then skipped). - const tail = text.slice(m + SPEC_MENTION.length, m + SPEC_MENTION.length + 48); + let tail = text.slice(m + SPEC_MENTION.length, m + SPEC_MENTION.length + 48); + // Stop at the end of the SENTENCE, the same discipline the claim/mention + // pairing above applies. Without it the window scrapes the capitalised + // opening words of the NEXT sentence and reports them as cited symbols: + // `ActionDef` (packages/core/src/actions/ActionRunner.ts) reads + // "…mirroring `@objectstack/spec`'s `ActionSchema`. Open key set on a + // data bag is correct", and `Open` is prose, not a citation. Harmless + // while `symbols` only decorated a message; since objectui#4607 it + // decides whether the tie test applies, and a claim whose only "cited + // symbols" are prose words would read as citing nothing but dangling. + const sentenceEnd = tail.search(/[.;!?](?:\s|$)/); + if (sentenceEnd !== -1) tail = tail.slice(0, sentenceEnd); for (const s of tail.matchAll(/[`'"\s(]([A-Z][A-Za-z0-9_]{2,})\b/g)) symbols.push(s[1]); } return { phrase: hit[0], distance, symbols: [...new Set(symbols)], text }; @@ -777,11 +831,23 @@ export function scanFileForClaims(file, specNames = new Map()) { const claim = findClaim(attachedDoc(stmt, text)); if (!claim) continue; + + const dangling = claim.symbols.filter((s) => !specNames.has(s)); + // The tie is judged against the symbols the claim CITES (objectui#4607) — + // see the header's fourth precision rule. When the claim names symbols and + // the spec exports NONE of them, no reference elsewhere in the declaration + // can be evidence for THIS claim, so the tie test below is not consulted. + // Guarded on a non-empty `specNames`: with no export set to check against, + // "dangling" is unknowable and the rule must not manufacture a verdict from + // ignorance of the spec. + const citesOnlyDanglingSymbols = + specNames.size > 0 && claim.symbols.length > 0 && dangling.length === claim.symbols.length; + // `skipLiterals: false` on purpose — rule 1 asks "is this THE spec's symbol", // where only a structural position counts. Rule 2 asks the weaker question // "does this declaration have ANY compile-time tie to what it claims", and a // spec type used on a member is a tie a spec change can still break. - if (referencesSpec(stmt, specBindings, false, nameNode)) continue; + if (!citesOnlyDanglingSymbols && referencesSpec(stmt, specBindings, false, nameNode)) continue; const { line } = sf.getLineAndCharacterOfPosition(stmt.getStart(sf)); findings.push({ @@ -789,7 +855,7 @@ export function scanFileForClaims(file, specNames = new Map()) { file, line: line + 1, phrase: claim.phrase, - dangling: claim.symbols.filter((s) => !specNames.has(s)), + dangling, }); } return findings;