nlq: close the declared-unenforced gap inside the declared-unenforced guard (format) - #37
nlq: close the declared-unenforced gap inside the declared-unenforced guard (format)#37mdheller wants to merge 3 commits into
Conversation
|
@copilot review |
There was a problem hiding this comment.
Pull request overview
Note
Copilot couldn't run its full agentic review because it didn't start before the timeout. Make sure your repository has a runner available, or add a copilot-code-review.yml file specifying one with the runs-on attribute. See the docs for more details.
Closes the “declared-but-unenforced” validation gap for format in the shared JSON-Schema subset validator by making format support depend on implemented checks (not just the keyword’s presence), and by centralizing format enforcement in nlq.ts.
Changes:
- Introduces
FORMAT_CHECKS+matchesFormatand gatesformatdeclarations by value inassertSupportedKeywords. - Implements
date-timein the shared validator and removes the duplicated call-site regex invendor-graph.ts. - Adds tests asserting the implemented formats list and that each supported format both accepts and rejects a representative value.
Reviewed changes
Copilot reviewed 5 out of 9 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| ts/src/nlq.ts | Centralizes format checks, updates the import-time guard, and enforces formats through a single map. |
| ts/src/vendor-graph.ts | Replaces the private requestedAt regex with matchesFormat('date-time', ...) and keeps a loud failure. |
| ts/src/vendor-graph.test.ts | Adds coverage proving the contract-declared date-time is enforced by the shared validator and still throws at the call site. |
| ts/src/nlq.test.ts | Adds non-tautological tests for implemented formats, including counter-examples and loud failures for unimplemented formats. |
| ontology/effect-request/PROVENANCE.md | Updates documentation to reflect the new “format admitted by value” behavior and removal of the private regex. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| function isIsoDateTime(s: string): boolean { | ||
| return /^\d{4}-\d{2}-\d{2}T\d{2}:\d{2}:\d{2}(\.\d+)?(Z|[+-]\d{2}:\d{2})$/.test(s) | ||
| } |
There was a problem hiding this comment.
Accepted and fixed in d773c1d — this was a real gap, and a pointed one given where it sat.
2026-99-99T99:99:99Z cleared the old check: correct punctuation, no time. A format check that admits values no clock can produce is shape-matching wearing enforcement's clothes, which is the exact failure this PR exists to close — so the gap was inside the guard against that gap.
The check now validates component ranges and the day against the real length of that month in that year, so 2026-02-30 and 2026-04-31 are rejected. Month length is computed rather than asked of Date, because Date.UTC(year, …) maps a two-digit year onto 19xx and would answer for 1950 when asked about 0050.
The new test is red against the old regex and green against the new one — 12 impossible instants that previously passed, plus the cases that must keep passing: the RFC 3339 leap second (23:59:60Z), a genuine leap day (2024-02-29), the divisible-by-400 case (2000-02-29), and 1900-02-29 rejected at the other end.
One correction to the wording: these were false positives (invalid timestamps passing), not false negatives.
| /** | ||
| * `format: "date-time"` — an RFC-3339 instant: date, `T`, time, and an EXPLICIT offset (`Z` or | ||
| * ±HH:MM). A bare date (`2026-07-29`) names a day, not an instant, and does not pass. | ||
| */ |
There was a problem hiding this comment.
Accepted and fixed in d773c1d (this and the duplicate on line 104).
Resolved toward the user-visible string rather than away from it: the docstring now reads "an ISO-8601 date-time in its RFC-3339 profile", naming RFC 3339 as the profile that makes the offset mandatory. That keeps the violation message, the thrown error in vendor-graph.ts, and the tests all saying one thing, instead of renaming four call sites to fix a comment.
| const FORMAT_CHECKS: ReadonlyMap<string, { check: (s: string) => boolean; expected: string }> = new Map([ | ||
| ['date-time', { check: isIsoDateTime, expected: 'an ISO-8601 date-time' }], |
There was a problem hiding this comment.
Accepted and fixed in d773c1d (this and the duplicate on line 104).
Resolved toward the user-visible string rather than away from it: the docstring now reads "an ISO-8601 date-time in its RFC-3339 profile", naming RFC 3339 as the profile that makes the offset mandatory. That keeps the violation message, the thrown error in vendor-graph.ts, and the tests all saying one thing, instead of renaming four call sites to fix a comment.
| /** Every `format` the schema declares, at any depth — walked here, not asked of the validator. */ | ||
| function declaredFormats(node: unknown, into = new Set<string>()): Set<string> { | ||
| if (Array.isArray(node)) { for (const v of node) declaredFormats(v, into); return into } | ||
| if (node === null || typeof node !== 'object') return into | ||
| for (const [k, v] of Object.entries(node as Record<string, unknown>)) { | ||
| if (k === 'format' && typeof v === 'string') into.add(v) | ||
| else declaredFormats(v, into) | ||
| } | ||
| return into | ||
| } |
There was a problem hiding this comment.
Rejected, though it is a fair thing to raise.
Both copies are ~8-line pure helpers local to their own test file, and the duplication is doing something: each suite walks the schema independently of the validator it is testing. A shared helper would mean one walker's blind spot becomes both suites' blind spot simultaneously — and these particular tests exist to detect drift, so a single point of failure underneath them is the thing worth avoiding, not the duplication above them.
The drift risk is also bounded in a way the comment assumes it is not: if the two ever disagree, the assertions they feed (declaredFormats(SEMANTIC_ACTION_SCHEMA) is exactly ['uri'], and the EffectRequest one is exactly ['date-time']) are pinned to literal expected sets, so a walker that started skipping a branch would go red rather than quietly agree with itself.
Happy to extract it if a third consumer appears — at that point the balance changes.
ReviewThe fix is correct and the design is sound. A few observations, from most to least significant:
|
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 5 out of 9 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (3)
ts/src/nlq.ts:112
- Although typed as
ReadonlySet<string>, this exports a mutableSetinstance at runtime (callers can still.add()/.delete()via JS or casts), which can silently break the invariant thatIMPLEMENTED_FORMATSmirrorsFORMAT_CHECKS. Consider exporting a function that returns a fresh snapshot (e.g.,implementedFormats(): readonly string[]/new Set(...)) or exposing only predicates likeisImplementedFormat(name)to prevent external mutation.
export const IMPLEMENTED_FORMATS: ReadonlySet<string> = new Set(FORMAT_CHECKS.keys())
ts/src/vendor-graph.test.ts:376
- This
declaredFormats()helper is duplicated ints/src/nlq.test.ts. To avoid the two copies drifting (especially since these tests are intended to detect drift), consider extracting it into a shared test utility (or reusing an existing one if the repo has a test helpers module) and importing it from both test files.
/** Every `format` the schema declares, at any depth — walked here, not asked of the validator. */
function declaredFormats(node: unknown, into = new Set<string>()): Set<string> {
if (Array.isArray(node)) { for (const v of node) declaredFormats(v, into); return into }
if (node === null || typeof node !== 'object') return into
for (const [k, v] of Object.entries(node as Record<string, unknown>)) {
if (k === 'format' && typeof v === 'string') into.add(v)
else declaredFormats(v, into)
}
return into
}
ontology/effect-request/PROVENANCE.md:63
- The PR description says it 'Touches
nlq.ts/vendor-graph.tsonly', but this PR also changes test files (ts/src/nlq.test.ts,ts/src/vendor-graph.test.ts) and documentation (ontology/effect-request/PROVENANCE.md). Please update the description to reflect the full set of touched files (or clarify that the statement meant production code only).
## Regeneration
…red-unenforced gaps
`assertSupportedKeywords` is the bar every vendored sourceos-spec contract clears at
import, and its stated contract is that a schema using an unimplemented keyword fails
LOUDLY rather than silently validating less than it says. It did not hold for `format`.
`SUPPORTED_KEYWORDS` listed `format`, so a schema declaring ANY format cleared the bar —
while `validateAgainst` enforced exactly one, `uri`. A contract declaring `date-time`,
`email` or `uuid` therefore passed the loud-failure bar and was then validated for
everything except the constraint it actually declared. Exactly one such declaration exists
on main: `requestedAt` in `EffectRequest.json` — enforced only by a private regex at the
`proposeRevendor` call site, and by nothing at all for any other consumer of the contract.
`format` is now admitted by VALUE, not by name:
* `FORMAT_CHECKS` is one map from format name to the check that implements it. It is the
single source of truth for both halves of the guard — `validateAgainst` enforces
through it, `assertSupportedKeywords` admits a declared format only if it has a key
there. Adding a name to the accept-list without writing the check is not expressible,
because the accept-list IS the set of checks.
* `date-time` is implemented rather than allowlisted: the ISO-8601 rule moves out of
`vendor-graph.ts` into `FORMAT_CHECKS`, so it now holds for EVERY contract.
* `format` is dropped from `SUPPORTED_KEYWORDS`, which is fail-safe: delete the explicit
branch and `format` becomes an unknown keyword, which throws.
* A Map, not an object literal — `'constructor' in {}` is true, and a prototype key must
never read as an implemented format.
`vendor-graph.ts` loses its duplicate `ISO_DATE_TIME` regex and the comment documenting the
limitation. The call site keeps a loud throw, now sourced from the one implementation via
`matchesFormat`, because `analyzeVendorFreshness` seals whatever proposals it is handed
without reading `contractViolations`: downgrading this to a violation entry would convert a
loud failure into a silent one, which is the bug class this change exists to close.
Teeth, both ways. `nlq.test.ts` states the expected formats and their counter-examples
LITERALLY rather than deriving them from `IMPLEMENTED_FORMATS` — a test written from the
implementation's own constant cannot catch a format that was accepted and never
implemented. Verified by injection: adding a stub `email: () => true` fails the drift test,
and adding it together with a matching test row still fails on the counter-example.
Copilot review finding on #37. `isIsoDateTime` was a purely structural regex, so `2026-99-99T99:99:99Z` passed: the right punctuation, and no time. A format check that admits values no clock can produce is shape-matching wearing enforcement's clothes — precisely what this PR's "declared ⇒ enforced" bar exists to prevent, so the gap sat inside the guard against exactly that gap. Component ranges are now checked, and the day against the real length of that month in that year (`2026-02-30` is rejected; `2024-02-29` is not). Month length is computed rather than asked of `Date`, because `Date.UTC(year, …)` maps a two-digit year onto 19xx and would answer for 1950 when asked about 0050. The new test is red against the old regex and green against this one — 12 impossible instants that used to pass, plus the leap-second and leap-day cases that must keep passing, and 1900-02-29 (not a leap year) at the other end. Also aligns the docstring's "RFC-3339 instant" with the violation message's "ISO-8601 date-time" (Copilot #37, two comments): one rule, named consistently, with RFC 3339 identified as the profile that makes the offset mandatory.
51f2459 to
d773c1d
Compare
Copilot review on #37, low-confidence channel — the sharpest finding on this PR, and one neither REST comment channel returned. `IMPLEMENTED_FORMATS` was `export const … : ReadonlySet<string>`. `ReadonlySet` is erased at runtime: the value is a live `Set`, and `export * from './nlq'` puts it on the package's public surface. So (IMPLEMENTED_FORMATS as Set<string>).add('email') widened the accept-list without adding a check. `validateAgainst` skips any format it has no `FORMAT_CHECKS` entry for, so `format: "email"` then cleared the import bar and was enforced by nobody — verified end to end before the fix. That is precisely the declared-unenforced gap this PR closes, reachable from outside the module: the guard against the gap had the gap. The accept-list is now asked of `FORMAT_CHECKS` on every call (`isImplementedFormat`), so there is no copy to widen, and `implementedFormats()` hands out a fresh FROZEN array — frozen because `readonly string[]` is erased exactly as `ReadonlySet` was, so only the freeze makes a mutation attempt fail rather than quietly succeed on a copy the caller then trusts. `IMPLEMENTED_FORMATS` is removed rather than deprecated: it was introduced by this same PR, so nothing outside it consumes the name.
Review disposition — including the suppressed channelCopilot's review carries the degraded-mode banner and reports 5 of 9 changed files reviewed — barely half. The diff was reviewed independently on that basis. Three further findings were in the review body's Fixed — the suppressed one first
Rejected
Noted, not actioned
The design claim, tested rather than trustedThe premise is that
The design holds. Keeping Mirror in the sibling implementationA second copy of this validator lives at |
Not merged — CI never ran, and the previous run never ran the testsHeld deliberately. Two separate reasons, both about the suite not having executed against this diff. 1. The earlier green run did not include the
|
The gap
assertSupportedKeywordsis the bar every vendored sourceos-spec contract clears at import. Its stated contract:It did not hold for
format— inside the guard whose entire job is to prevent exactly this.SUPPORTED_KEYWORDSlistedformat, so a schema declaring any format cleared the bar.validateAgainstenforced exactly one:So
format: "date-time"/"email"/"uuid"passed the loud-failure bar and was then validated for everything except the constraint it declared.A walk of the
origin/maintree (ontology/**+ts/src/*-data.ts) finds exactly two format declarations:uriinSemanticAction.json(enforced) anddate-timeonrequestedAtinEffectRequest.json— enforced only by a private regex at theproposeRevendorcall site, and by nothing at all for any other consumer of that contract.The fix
formatis now admitted by value, not by name.FORMAT_CHECKS— one map from format name to the check implementing it. Single source of truth for both halves of the guard:validateAgainstenforces through it,assertSupportedKeywordsadmits a declared format only if it has a key there. Adding a name to the accept-list without writing the check is not expressible — the accept-list IS the set of checks.date-timeis implemented, not allowlisted. The ISO-8601 rule moves out ofvendor-graph.tsintoFORMAT_CHECKS, so it holds for every contract.formatdropped fromSUPPORTED_KEYWORDS— fail-safe: delete the explicit branch andformatbecomes an unknown keyword, which throws.Map, not an object literal:'constructor' in {}is true, and a prototype key must never read as an implemented format.vendor-graph.tsloses its duplicateISO_DATE_TIMEregex and the comment documenting the limitation.ontology/effect-request/PROVENANCE.mdno longer claims a limitation that is fixed.Proof — before vs after
The same probe, run against pristine
origin/mainand against this branch.origin/mainformat: "email"at the import bar[](unchecked)date-time"2026-07-29"viavalidateAgainst[]silently valid["… is not an ISO-8601 date-time"]EffectRequest.requestedAtbad value[]declared, enforced by nobodySemanticAction.output.typeRef":ContactList"The loud error:
Both real contracts still import and validate clean:
SemanticAction.jsondeclares["uri"],EffectRequest.jsondeclares["date-time"], both now enforced.Anti-tautology: the test cannot be written from the implementation
nlq.test.tsstates the expected formats and their counter-examples literally, not derived fromIMPLEMENTED_FORMATS— a test written from the implementation's own constant cannot catch a format that was accepted and never implemented.Verified by injection, both ways:
email: () => truetoFORMAT_CHECKS, leave the test alone → drift test fails (+ 'email'in the accept-list).email: 'definitely not an email' must be rejected — a format that rejects nothing is declared, not enforced.One deliberate deviation
proposeRevendorkeeps a loud throw for a malformedrequestedAt, now sourced from the one implementation viamatchesFormatrather than a private regex.Deleting the guard entirely would let the failure fall through to
contractViolations— butanalyzeVendorFreshnessmapsproposeRevendorover stale pins and seals the results without ever reading that array. That would convert a loud failure into a silent one, which is the bug class this PR closes. The duplicated rule is gone; the escalation is kept, and the reason is stated in the code.Gates
npm test— 416 pass, 0 fail (6 new tests)npm run typecheck— cleannpm run check:kko— greennpm run check:dist— green (ts/distrebuilt and committed)Production code touched:
nlq.tsandvendor-graph.tsonly. The change also updates their tests(
nlq.test.ts,vendor-graph.test.ts) andontology/effect-request/PROVENANCE.md— the earlierwording said "only" without that qualifier (Copilot review, suppressed channel).
No overlap with #34. Rebased onto #36 so the
Teststep actually runs against this diff: theoriginal CI run was queued before #36 merged, so the suite never executed against it.
Review round 2
isIsoDateTimewas structural only —2026-99-99T99:99:99Zpassed. Now range- and calendar-checked.IMPLEMENTED_FORMATSwas an exported mutableSetbehind aReadonlySettype:.add("email")widened the accept-list without adding a check, so a declared format cleared the bar and was
enforced by nobody. Replaced with
isImplementedFormat()/implementedFormats(), which readFORMAT_CHECKSdirectly and hand out a frozen snapshot.