test(COMPASS-28): move to @stoplight/spectral-core - #2724
Conversation
Automated review —
|
| case | validateSchema |
compile |
|---|---|---|
dangling $ref |
true | THROWS |
typo'd keyword (minimun) |
true | accepts |
unknown format |
true | accepts |
required: 42 |
false | THROWS |
The generator emits $ref: "#/definitions/User" (snapshot line 61), so dangling refs are a live bug class this assertion cannot see. Add validator.compile(schema) alongside — it throws on an unresolvable $ref at any strict setting.
2. Same file — the spec that had the vacuous assertion is the only one that got no guard. assertValidJsonSchema is only ever called on valid generator output (:34, :48, :60); nothing proves it can fail. The two oas specs each received exactly this mutation check. Add one negative case (type: "strng" → expect false).
Worth being explicit about why this matters: my mutation check for the replacement used required: 42 — the one row above where validateSchema returns false. That gave false confidence that the new assertion was strong.
🟡 Important (2)
3. json-schema.spec.ts:16 — strict: false is inert. Strict-mode checks fire at compile(), never inside validateSchema; verified identical results for strict true/false across all 7 cases. It doesn't weaken anything today, but it is dead config that will silently disarm a later compile call. Drop it, or keep it with a comment naming what it suppresses.
4. openapi2.spec.ts:61 / openapi3.spec.ts:79 — the guard test overclaims. I verified: flipping oas2-schema, typed-enum and operation-success-response to false leaves the oas2 guard green (20/20 pass). It exercises 2 of 19 rules there, and neither is oas2--prefixed, so no format-specific rule is covered anywhere in that file. The name ("the enabled rules are applied") claims more than it enforces — rename it to the wiring property it actually pins.
That's the third round on this same comment in this PR, which is its own signal.
🔵 Suggestions (1)
openapi2.spec.ts, every toHaveLength(0) site — the unrecognized-format safety net documented at openapi3.spec.ts:22-26 is genuine, but it misses cross-version drift: an OpenAPI 3 document linted under the oas2 ruleset returns [], because the extended oas ruleset registers oas3 formats too. swagger: "2.0" appears only in the new fixture, never as an assertion on generator output. expect(result.swagger).toEqual("2.0") inside lint closes it.
✅ Verified sound
- Exact-code arrays are not brittle —
extends: [[oas, "off"]]lands upstream additions disabled, so they cannot add a finding. Only a rename (which makessetRulesetthrow) or a behaviour change in an already-enabled rule breaks the array, and both merit a failure. KeeptoEqual. - The guard does not duplicate
setRulesetthrowing — that validates rule names, not enablement. The reasoning in the comment checks out. - The four renames are documented at the site with the old name; the two deliberately-off rules carry a navigable COMPASS-29 link rather than a bare "deferred"; the ruleset objects sit next to the assertions that depend on them.
Recommended action
- Add
compile()and a negative case tojson-schema.spec.ts— findings 1 and 2 are the same omission and fix together. - Drop the inert
strict: false. - Rename the guard test to what it enforces, or widen it to cover a format-specific rule.
Automated review, part 2 — code-reviewerThe code-reviewer agent finished after my comment above and found something that corrects both that comment and this PR's description. I verified it directly against the unpacked 🔴 Critical — the rename history is wrong, and it is the premise for the deferral
Checked
So three of the names the old YAMLs enabled did not exist, and v5's Two things follow, and both contradict what I wrote:
The code stays as-is; the narrative needs fixing, in the two comments and the description. It matters because the COMPASS-29 owner would otherwise inherit "v6 got stricter" when the truth is "this rule was never on" — a different investigation. I have corrected COMPASS-29's description accordingly. 🟡 Important (3)
Change-narration comments — ✅ Verified sound
Recommended action
|
`@stoplight/spectral@5.9.2` is deprecated, and it was superseded by a package split rather than a version. Replaced with `spectral-core`, `spectral-parsers` and `spectral-rulesets`, all dev-only, all absent from the image. Three specs used it, not one: the OpenAPI 3, OpenAPI 2 and JSON Schema generators. Both YAML rulesets are now typed ruleset objects next to the assertions that depend on them, since v6 takes an object rather than a path and the files were test-only configuration. Four of the names the old rulesets enabled did not exist in spectral 5.9.2 — `example-value-or-externalValue`, `oas3-valid-example` and `oas2-valid-example` are absent from its `oas` ruleset — and `loadRuleset` ignored an unknown name rather than reporting it. So those lines were dead and no example validation ran at all. `oas3-valid-schema-example` did exist under that name; it was simply never switched on. The media rules are therefore new coverage rather than ports, and the schema rules stay off because they report six standing findings against the schemaprops contract, which belong to COMPASS-29 with the generator fix. Two names did change, and both are noted at the site: `operation-2xx-response` to `operation-success-response`, and `oas3-unused-components-schema` to `oas3-unused-component`. Each ruleset has a test that breaks specific rules and asserts the codes it gets back. Every other spectral assertion is `toHaveLength(0)`, which a ruleset that enables nothing satisfies just as well as a clean document; an unknown rule name is not that gap, because `setRuleset` throws on one. It is named for what it pins — that the extends and rules wiring leaves the named rules enabled — rather than for coverage it does not have: it exercises three rules, not the whole set. The JSON Schema spec's spectral call could not fail. It registered the draft-07 format and never loaded a ruleset, so `run` had no rules — zero findings for a valid schema and zero for a broken one. It now validates through ajv, and does so twice: `validateSchema` reads the shape, while `compile` under `strict` resolves the `$ref`s this generator emits and rejects a keyword ajv does not know. Both halves are pinned by a case that fails without them. `@scarf/scarf` reaches the tree through four of the seven `@stoplight/spectral-*` packages. Its postinstall reports the install to scarf.sh and it carries no functionality, so its build is denied rather than allowed. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
5b783e6 to
4735738
Compare
Automated review, round 2 —
|
| validator | schema with format: "date-time" |
|---|---|
new Ajv({ strict: true }) |
THROWS unknown format "date-time" |
new Ajv({}) |
THROWS (this is strictSchema, on by default) |
new Ajv({ strict: false }) |
compiles |
addFormats(new Ajv({ strict: true })) |
compiles |
It ships green only because no JSON Schema fixture has a date property. The first contributor to add one gets a confusing failure in an assertion that looks unrelated. ajv-formats@^3.0.1 is already a production dependency — add addFormats(validator).
🟡 Important (3)
1. Two of the three assertions are redundant, and the docstring says the opposite. compile() runs validateSchema internally (ajv's validateSchema: true default) and throws schema is invalid: …. Confirmed with {type: "nummber"}: validateSchema returns false and compile throws. So :18-19 catch nothing :20 misses — they only improve the message. My docstring "Two assertions, because they catch different things" is inaccurate. And the errors check at :19 is separately vacuous: errors is null on success, and on failure the line above already aborts.
2. strict: true — the agents split, and both are partly right. The test-analyzer mutated strict: true → false and the minimun case failed, so it is pinned. The code-reviewer noted minimun also throws under new Ajv({}), because strictSchema is on by default. My probe confirms both: {} throws, {strict: false} does not.
So the honest reading is: the test guards against someone setting strict: false; it does not show strict: true adds anything over the default. What strict: true actually adds is strictTypes/strictTuples escalation and strictRequired. Either pin it with a shape it genuinely governs (a required naming a property absent from properties), or drop it and rewrite the comment.
3. The guard test still overclaims, after being renamed once. "…leaves the named rules enabled" reads as the whole list above it. It pins 2 of 18 enabled oas2 rules and 3 of 19 oas3 rules — and a mutation flipping five other named rules off leaves all 221 tests green. Its true scope is "not every rule is off". Name it for the codes it asserts.
🔵 Suggestions (3)
openapi3.spec.ts:25-27is wrong as written.unrecognized-formatfires only for a document spectral can place nowhere (confirmed with{hello:"world"}). A wrong-version OpenAPI document is placeable and does pass vacuously. Narrow to "a document of neither OpenAPI version".- Bare
.toThrow()at:33and:45passes if any of the three internal expects throws, so it does not pin which mechanism rejected. Use.toThrow(/can't resolve reference/)and.toThrow(/unknown keyword: "minimun"/). - Two words on the
as RulesetDefinitioncast. It bridges a genuine upstream imprecision —oas.rules['duplicated-entry-in-enum'].severityis widened tostringin spectral-rulesets' emitted.d.ts(missingas const) — so it is correct and materially better thanas any. But because it covers the wholeoasobject it will also swallow a future real incompatibility. Naming the widened field tells the next reader what the cast is for. - Three positive tests are still
asyncwith nothing awaited, left over from the spectral call.
✅ Verified sound
- The corrected rename narrative checks out, independently re-verified against
@stoplight/spectral@5.9.2'soas/index.json:example-value-or-externalValue,oas2-valid-example,oas3-valid-exampleabsent;oas3-valid-schema-example,operation-2xx-response,oas3-unused-components-schemapresent. Every claim at the call sites and in the commit message holds, including "spectral 6 has only anoas3-form". compile()is load-bearing — dropping it fails both negative cases.- No
$idcollision is possible — a fresh validator per call, and the generated schemas carry no$idat all, so even a shared instance would key on the object. strict: truedoes not change what the three existing generator assertions accept — they pass because the fixtures emit onlytype/properties/required/items/allOf/$ref,requiredis always derived fromproperties, and no tupleitemsexist.- Cross-version drift is covered after all — the round-1 suggestion to assert
swagger: "2.0"is redundant: all 18 oas2 lint sites sit behind a full-document snapshot whose stored text carries the version, so drift breaks 18 snapshots first. @scarf/scarfdenial correct; COMPASS-29 is a distinct navigable follow-up; no surviving reference to the two deleted YAML rulesets in tracked files, dotfiles included.
Recommended action
- Add
addFormats(validator)— this is a real trap for the next contributor. - Collapse the three assertions to what
compile()actually needs and fix the docstring. - Decide
strict: pin it properly or drop it. - Rename the guard for the third time, to what it pins.
Ticket
COMPASS-28 — Bump
spot's NodeJS minimum from 18 to 22What
@stoplight/spectral@5.9.2is deprecated and was superseded by a package split, not a version bump. Replaced with@stoplight/spectral-core,-parsersand-rulesets— all dev-only, all absent from the image (verified). Supersedes Dependabot #1987.Three specs used spectral, not one — OpenAPI 3, OpenAPI 2, and JSON Schema. I found the second by grep and the third only when its suite failed, which is a good argument for grepping the package name before starting rather than after.
Four rules in each ruleset had been renamed upstream
A name-for-name port would have looked clean and silently changed what is checked.
setRulesetthrows on an unknown rule name, so these surfaced loudly — but only once the names were being resolved at all:operation-2xx-responseoperation-success-responseexample-value-or-externalValueoas3-examples-value-or-externalValueoas3-unused-components-schemaoas3-unused-componentoas3-valid-exampleoas3-valid-media-example+oas3-valid-schema-exampleoas2-valid-exampleoas2-valid-media-example+oas2-valid-schema-exampleexample-value-or-externalValueis dropped from the OpenAPI 2 ruleset rather than renamed: v6 only has anoas3-form, which would be skipped against an OpenAPI 2 document anyway.A real bug this surfaced — deferred to COMPASS-29
The schema half of the split
valid-examplerules is left off. Enabling it is a coverage increase rather than a port, and it reports six real findings against the schemaprops contract:The generator emits, for example:
{ "type": "number", "format": "float", "maximum": 99.95, "exclusiveMinimum": false, ... }exclusiveMinimumwith nominimum. Under OAS 3.0 that keyword is a boolean modifier onminimum, so the schema is invalid against the meta-schema — andfalseis the default, so emitting it at all is redundant. Confirmed spectral v5's single rule never checked this, so nothing regressed; the rule was simply never on.Fixing the generator changes generated output and needs its own snapshot review, so it is COMPASS-29, linked from the deferral comment at each site.
Each ruleset now has a test that proves it is doing something
Every other spectral assertion in these specs is
toHaveLength(0)— which a ruleset that enables nothing satisfies exactly as well as a clean document. With 20-odd rules per ruleset and four of them renamed in this very bump, that is a lot of silent inertness to leave unguarded.So each spec lints a document that breaks specific enabled rules and asserts the codes it gets back. Mutation-checked:
setRulesetthrows (assertExistingRule)I initially justified this test as guarding against a format mismatch, and also declared
formatson each ruleset for the same reason. Both were wrong and are gone: the rules inherited fromoascarry their ownformats, which is what decides application (a ruleset-level declaration changed nothing in an A/B/C comparison), and a document spectral cannot place reportsunrecognized-formatrather than passing silently. The comments now state only what the tests actually pin.The JSON Schema spec's spectral call could not fail
It registered the draft-07 format and never loaded a ruleset, so
runexecuted with zero rules:The assertion held regardless of the document. v6 cannot reproduce that setup (it requires a ruleset), and there is no off-the-shelf JSON Schema ruleset to port to — so rather than delete a dead assertion, it now validates against the draft-07 meta-schema via ajv, already a dependency. Mutation-checked: feeding it
required: 42fails withmust be array.Supply chain
@scarf/scarfarrives through every spectral v6 package. Its postinstall isnode ./report.js— install analytics, "like Google Analytics for your npm packages" — and it carries no functionality.strictDepBuildscaught it and its build is denied, alongsidecore-jsandunrs-resolver.@stoplight/spectral-formatswas declared and then became unused once the inertformatsdeclarations came out; removed rather than left in the manifest.How this was verified
pnpm buildpnpm testpnpm lint:checkpnpm build-docsdocker build+check-image-parity@stoplight/*in the image