From 873ec56f5ca04d0e7f37ef7c2c85d100ec492841 Mon Sep 17 00:00:00 2001 From: Erny Sans Date: Sat, 22 Aug 2026 18:15:22 -0500 Subject: [PATCH] build: enable strictNullChecks and noImplicitAny, lint the test tree tsconfig.json declared "strict": true and then switched two of its members back off, so the strictness the config appeared to promise was not the one in effect. Both are now on, and eslint covers test/**/*.ts alongside src/**/*.ts, with parserOptions.project listing both tsconfigs so type-aware rules resolve there. Yield: 0 errors in src/, 5 in test/ -- all the same shape, a test reading a deliberately undeclared key to assert that the parse boundary preserves unknown keys. Each is resolved by making the read explicit (`as unknown as Record`) rather than by weakening a type or silencing the check. The compiler rejected the single-step cast on two of them and named the remedy; that form is used uniformly so identical intent reads identically. Mutation-tested: switching the boundary from z.looseObject to z.object turns all five red, so they still assert what they claim. Enabling noImplicitAny alone reports two errors that both flags together do not: without strictNullChecks a `null` literal infers as `any`. The half-configuration is worse than either end, which is why these land as one change rather than staged. One consequence worth calling out, since it changes a published type. The inferred output of the Idempotency response body was `string`, while the runtime schema accepts and preserves `null` and the hand-written interface declares `string | null`. Under the old setting the union collapsed, so the schema's inferred type contradicted both the runtime and the interface beside it; it now reads `string | null` and the three agree. Every emitted .js is unchanged, and that is the only type-level difference in the entire build output. Documentation that described the old configuration is corrected in the same commit, including the rationale for the requiredKey wrapper, which existed only to work around the inference collapse. The wrapper is retained rather than removed here: it has two call sites and removing it changes the errors they report, which deserves its own change and tests rather than arriving as a side effect of a compiler flag. --- .github/instructions/security.instructions.md | 27 ++++++++++++------- .github/instructions/tests.instructions.md | 6 ++--- eslint.config.js | 6 ++++- lib/interface/schema.d.ts | 26 ++++++++++++------ lib/interface/schema.js | 26 ++++++++++++------ lib/model/Block.js | 12 +++++---- lib/model/Idempotency.d.ts | 2 +- lib/model/Idempotency.js | 13 +++++---- src/interface/schema.ts | 26 ++++++++++++------ src/model/Block.ts | 12 +++++---- src/model/Idempotency.ts | 13 +++++---- test/interface/place.test.ts | 2 +- test/interface/queue.test.ts | 2 +- test/interface/schema.test.ts | 18 ++++++++----- test/model/Account.test.ts | 2 +- test/model/Block.test.ts | 2 +- test/model/Reservation.test.ts | 2 +- tsconfig.json | 4 +-- 18 files changed, 128 insertions(+), 73 deletions(-) diff --git a/.github/instructions/security.instructions.md b/.github/instructions/security.instructions.md index f1690ab..f7fa383 100644 --- a/.github/instructions/security.instructions.md +++ b/.github/instructions/security.instructions.md @@ -60,15 +60,19 @@ signature `base_db.ts` `[x: string]: any`. ### 2.2 Tooling that does not enforce what it appears to -Three settings mean the compiler and linter are **more permissive than they look**. This is -recorded so nobody mistakes a green build for a strictness guarantee: +Three settings previously meant the compiler and linter were **more permissive than they looked**. +Two have since been corrected; the remaining one is recorded so nobody mistakes a green build for a +strictness guarantee: - `eslint.config.js` sets `@typescript-eslint/no-explicit-any: ['off']` — an explicit `any` is - **not** a lint error here. -- `tsconfig.json` sets `"strict": true` but then **overrides two of its members**: - `"noImplicitAny": false` and `"strictNullChecks": false`. `strict: true` is not the final word; - the later, narrower keys win. -- `eslint.config.js` `files` is scoped to `src/**/*.ts`, so **`test/**` is not linted**. + **not** a lint error here. Still current. +- `tsconfig.json` sets `"strict": true`, and **no longer overrides it**: `noImplicitAny` and + `strictNullChecks` are both `true`. They were previously `false`, which meant `strict: true` was + not the final word — the later, narrower keys won. Any claim about this repository written before + that change may assume the old behaviour. +- `eslint.config.js` `files` now covers **both** `src/**/*.ts` and `test/**/*.ts`, with + `parserOptions.project` listing both tsconfigs so type-aware rules resolve. `test/` was + previously unlinted. Any claim that "strict mode would have caught it" must be checked against these three lines first. @@ -166,9 +170,12 @@ becomes someone's rediscovery: - **The 8 bare `any` are left in place.** Rationale in §2.1: precise typing needs a server SDK type this package must not depend on, and narrowing a published type breaks consumers. The resolution is a runtime schema layer, not a type edit. -- **`noImplicitAny` / `strictNullChecks` are left `false`.** Flipping either is not a - documentation change — it is a compile-breaking change across every model, and it belongs in its - own reviewed unit of work with the resulting diff visible. +- **`noImplicitAny` / `strictNullChecks` are now both `true`.** Enabling them produced **0** errors + in `src/` and 5 in `test/`, all of the same shape (a test reading a deliberately-undeclared key + to assert unknown-key preservation), resolved with an explicit `unknown`-first cast rather than + by weakening a type. Note that enabling `noImplicitAny` *alone* produces two additional errors + that both flags together do not: `null` literals infer as `any` without `strictNullChecks`, so + the half-configuration is strictly worse than either end. Enable them together or not at all. - **`@typescript-eslint/no-explicit-any` is left `off`.** Turning it on would fail the build on the 8 known fields above before there is anywhere for them to go. - **Transitive advisories with no upstream fix are not suppressed.** An `overrides` entry that diff --git a/.github/instructions/tests.instructions.md b/.github/instructions/tests.instructions.md index 543e6c5..0a13e8c 100644 --- a/.github/instructions/tests.instructions.md +++ b/.github/instructions/tests.instructions.md @@ -76,9 +76,9 @@ member's value produced **3 failed, exit 1**. - **Structure.** Arrange–Act–Assert inside descriptive nested `describe()` / `it()` blocks. - **Isolation.** Zero network, zero disk I/O, zero cloud or emulator access. The suite must be safe to run anywhere, against anything. It currently is; keep it that way. -- **No lint escape hatches.** No `// eslint-disable*`. Note that `eslint.config.js` scopes `files` - to `src/**/*.ts`, so **`test/` is not currently linted** — do not read a green `npm run lint` as - a statement about test files. +- **No lint escape hatches.** No `// eslint-disable*`. `eslint.config.js` `files` covers both + `src/**/*.ts` and `test/**/*.ts`, so test files are linted with type-aware rules — + `parserOptions.project` lists both `tsconfig.json` and `tsconfig.test.json` so they resolve. --- diff --git a/eslint.config.js b/eslint.config.js index 2ce6c11..61df12f 100644 --- a/eslint.config.js +++ b/eslint.config.js @@ -42,7 +42,10 @@ export default tseslint.config( languageOptions: { sourceType: 'module', parserOptions: { - project: './tsconfig.json', + project: [ + './tsconfig.json', + './tsconfig.test.json', + ], jsDocParsingMode: 'type-info', ecmaVersion: 'latest', sourceType: 'module', @@ -57,6 +60,7 @@ export default tseslint.config( }, files: [ 'src/**/*.ts', + 'test/**/*.ts', ], rules: { 'no-restricted-syntax': [ diff --git a/lib/interface/schema.d.ts b/lib/interface/schema.d.ts index c36d98b..dfe3a1a 100644 --- a/lib/interface/schema.d.ts +++ b/lib/interface/schema.d.ts @@ -273,18 +273,18 @@ export declare const parseOrThrow: (schema: TSchema, * Wraps a schema so that the object key it validates is inferred as * **required** rather than optional. * - * ## Why this is necessary, and why it is specific to this repository + * ## Why this exists, and its current status * - * `tsconfig.json` sets `strict: true` and then overrides it with + * This wrapper was introduced to work around a type-inference collapse caused by * `strictNullChecks: false`. Under that setting `undefined` is assignable to * everything, so a type of the form `"optional" | undefined` collapses to * `"optional"` — and that is exactly how zod carries its per-schema optionality * marker for several wrapper schemas, including `z.union`, `z.nullable` and * `z.nonoptional`. Zod decides an object key's optionality by testing that - * marker, so in this repository **every required key whose schema is one of - * those wrappers is inferred as optional**. + * marker, so under that setting **every required key whose schema is one of + * those wrappers was inferred as optional**. * - * The consequence is not cosmetic: a parse helper would return a type claiming + * The consequence was not cosmetic: a parse helper would return a type claiming * a field may be absent when at runtime it never is, and every consumer would * then write a defensive `?? fallback` for a case that cannot occur — which is * how a default value gets into a code path that had no need of one. @@ -294,9 +294,19 @@ export declare const parseOrThrow: (schema: TSchema, * one preserves the runtime validation exactly — the inner schema still decides * what is accepted — while restoring the correct inferred optionality. * - * The trade-off is error granularity: a failure reports one issue at the key's - * path rather than the inner schema's per-member detail, which is why the - * message is a required argument rather than a generic default. + * **`strictNullChecks` is now enabled, so the collapse no longer occurs.** + * Verified by inferring `z.object({u: z.union([...])})` and observing that the + * required key is now correctly reported as missing (`TS2741`) where it + * previously type-checked clean. The wrapper is therefore no longer necessary + * for its original purpose, and is retained only so that removing it — which + * changes the errors reported at its two call sites — is a deliberate change + * with its own tests rather than a side effect of a compiler-flag change. + * + * The trade-off it carries is error granularity: a failure reports one issue at + * the key's path rather than the inner schema's per-member detail, which is why + * the message is a required argument rather than a generic default. That + * trade-off is now a cost without a corresponding benefit, so prefer the inner + * schema directly for new code, and see the note above before adding a call. * * @template TSchema The inner schema, which performs the actual validation. * @param {TSchema} schema - Schema describing the accepted values. diff --git a/lib/interface/schema.js b/lib/interface/schema.js index 5b9afac..ade8801 100644 --- a/lib/interface/schema.js +++ b/lib/interface/schema.js @@ -114,18 +114,18 @@ export const parseOrThrow = (schema, value, label) => { * Wraps a schema so that the object key it validates is inferred as * **required** rather than optional. * - * ## Why this is necessary, and why it is specific to this repository + * ## Why this exists, and its current status * - * `tsconfig.json` sets `strict: true` and then overrides it with + * This wrapper was introduced to work around a type-inference collapse caused by * `strictNullChecks: false`. Under that setting `undefined` is assignable to * everything, so a type of the form `"optional" | undefined` collapses to * `"optional"` — and that is exactly how zod carries its per-schema optionality * marker for several wrapper schemas, including `z.union`, `z.nullable` and * `z.nonoptional`. Zod decides an object key's optionality by testing that - * marker, so in this repository **every required key whose schema is one of - * those wrappers is inferred as optional**. + * marker, so under that setting **every required key whose schema is one of + * those wrappers was inferred as optional**. * - * The consequence is not cosmetic: a parse helper would return a type claiming + * The consequence was not cosmetic: a parse helper would return a type claiming * a field may be absent when at runtime it never is, and every consumer would * then write a defensive `?? fallback` for a case that cannot occur — which is * how a default value gets into a code path that had no need of one. @@ -135,9 +135,19 @@ export const parseOrThrow = (schema, value, label) => { * one preserves the runtime validation exactly — the inner schema still decides * what is accepted — while restoring the correct inferred optionality. * - * The trade-off is error granularity: a failure reports one issue at the key's - * path rather than the inner schema's per-member detail, which is why the - * message is a required argument rather than a generic default. + * **`strictNullChecks` is now enabled, so the collapse no longer occurs.** + * Verified by inferring `z.object({u: z.union([...])})` and observing that the + * required key is now correctly reported as missing (`TS2741`) where it + * previously type-checked clean. The wrapper is therefore no longer necessary + * for its original purpose, and is retained only so that removing it — which + * changes the errors reported at its two call sites — is a deliberate change + * with its own tests rather than a side effect of a compiler-flag change. + * + * The trade-off it carries is error granularity: a failure reports one issue at + * the key's path rather than the inner schema's per-member detail, which is why + * the message is a required argument rather than a generic default. That + * trade-off is now a cost without a corresponding benefit, so prefer the inner + * schema directly for new code, and see the note above before adding a call. * * @template TSchema The inner schema, which performs the actual validation. * @param {TSchema} schema - Schema describing the accepted values. diff --git a/lib/model/Block.js b/lib/model/Block.js index 958ac74..81dea91 100644 --- a/lib/model/Block.js +++ b/lib/model/Block.js @@ -68,11 +68,13 @@ export var Block; * enforce rather than this schema's. * * Wrapped in `requiredKey` because this key is required and its schema is a - * `z.union`, which zod infers as an optional key under this repository's - * `strictNullChecks: false` setting. Without the wrapper {@link parse} would - * return a type claiming `value` may be absent when at runtime it never is. - * The compile-time proof below is what surfaced that; see `requiredKey` for - * the full explanation. + * `z.union`, which zod inferred as an optional key under the + * `strictNullChecks: false` setting this package previously used. Without + * the wrapper {@link parse} would have returned a type claiming `value` may + * be absent when at runtime it never is. `strictNullChecks` is now enabled + * and the inference is correct without the wrapper, which is retained here + * only so that removing it is a deliberate change with its own tests rather + * than a side effect of a compiler-flag change; see `requiredKey`. */ value: requiredKey(blockValueSchema, 'Expected a string, number, object or array block value'), /** diff --git a/lib/model/Idempotency.d.ts b/lib/model/Idempotency.d.ts index cbce297..b097219 100644 --- a/lib/model/Idempotency.d.ts +++ b/lib/model/Idempotency.d.ts @@ -154,7 +154,7 @@ export declare namespace Idempotency { progress: z.ZodOptional>; response: z.ZodOptional; + body: z.ZodCustom; truncated: z.ZodBoolean; }, z.core.$loose>>; lockExpires: z.ZodOptional>>; diff --git a/lib/model/Idempotency.js b/lib/model/Idempotency.js index ff4c642..8e98b0a 100644 --- a/lib/model/Idempotency.js +++ b/lib/model/Idempotency.js @@ -52,11 +52,14 @@ export var Idempotency; /** * Schema for {@link Response}. * - * {@link Response.body} is required **and** nullable, which in this repository - * needs `requiredKey`: zod infers a required `z.nullable` key as optional - * under `strictNullChecks: false`. See that helper for why. `null` here means - * the original response genuinely had no body, which is a different claim from - * the field being absent, so the distinction has to survive. + * {@link Response.body} is required **and** nullable. It is wrapped in + * `requiredKey` because zod inferred a required `z.nullable` key as optional + * under the `strictNullChecks: false` setting this package previously used. + * That setting is now enabled and the inference is correct without the + * wrapper, which is retained pending a deliberate removal; see that helper. + * `null` here means the original response genuinely had no body, which is a + * different claim from the field being absent, so the distinction has to + * survive. */ const responseSchema = z.looseObject({ /** diff --git a/src/interface/schema.ts b/src/interface/schema.ts index 0c45c00..b01a686 100644 --- a/src/interface/schema.ts +++ b/src/interface/schema.ts @@ -340,18 +340,18 @@ export const parseOrThrow = ( * Wraps a schema so that the object key it validates is inferred as * **required** rather than optional. * - * ## Why this is necessary, and why it is specific to this repository + * ## Why this exists, and its current status * - * `tsconfig.json` sets `strict: true` and then overrides it with + * This wrapper was introduced to work around a type-inference collapse caused by * `strictNullChecks: false`. Under that setting `undefined` is assignable to * everything, so a type of the form `"optional" | undefined` collapses to * `"optional"` — and that is exactly how zod carries its per-schema optionality * marker for several wrapper schemas, including `z.union`, `z.nullable` and * `z.nonoptional`. Zod decides an object key's optionality by testing that - * marker, so in this repository **every required key whose schema is one of - * those wrappers is inferred as optional**. + * marker, so under that setting **every required key whose schema is one of + * those wrappers was inferred as optional**. * - * The consequence is not cosmetic: a parse helper would return a type claiming + * The consequence was not cosmetic: a parse helper would return a type claiming * a field may be absent when at runtime it never is, and every consumer would * then write a defensive `?? fallback` for a case that cannot occur — which is * how a default value gets into a code path that had no need of one. @@ -361,9 +361,19 @@ export const parseOrThrow = ( * one preserves the runtime validation exactly — the inner schema still decides * what is accepted — while restoring the correct inferred optionality. * - * The trade-off is error granularity: a failure reports one issue at the key's - * path rather than the inner schema's per-member detail, which is why the - * message is a required argument rather than a generic default. + * **`strictNullChecks` is now enabled, so the collapse no longer occurs.** + * Verified by inferring `z.object({u: z.union([...])})` and observing that the + * required key is now correctly reported as missing (`TS2741`) where it + * previously type-checked clean. The wrapper is therefore no longer necessary + * for its original purpose, and is retained only so that removing it — which + * changes the errors reported at its two call sites — is a deliberate change + * with its own tests rather than a side effect of a compiler-flag change. + * + * The trade-off it carries is error granularity: a failure reports one issue at + * the key's path rather than the inner schema's per-member detail, which is why + * the message is a required argument rather than a generic default. That + * trade-off is now a cost without a corresponding benefit, so prefer the inner + * schema directly for new code, and see the note above before adding a call. * * @template TSchema The inner schema, which performs the actual validation. * @param {TSchema} schema - Schema describing the accepted values. diff --git a/src/model/Block.ts b/src/model/Block.ts index 818b91c..89af95b 100644 --- a/src/model/Block.ts +++ b/src/model/Block.ts @@ -114,11 +114,13 @@ export namespace Block { * enforce rather than this schema's. * * Wrapped in `requiredKey` because this key is required and its schema is a - * `z.union`, which zod infers as an optional key under this repository's - * `strictNullChecks: false` setting. Without the wrapper {@link parse} would - * return a type claiming `value` may be absent when at runtime it never is. - * The compile-time proof below is what surfaced that; see `requiredKey` for - * the full explanation. + * `z.union`, which zod inferred as an optional key under the + * `strictNullChecks: false` setting this package previously used. Without + * the wrapper {@link parse} would have returned a type claiming `value` may + * be absent when at runtime it never is. `strictNullChecks` is now enabled + * and the inference is correct without the wrapper, which is retained here + * only so that removing it is a deliberate change with its own tests rather + * than a side effect of a compiler-flag change; see `requiredKey`. */ value: requiredKey(blockValueSchema, 'Expected a string, number, object or array block value'), /** diff --git a/src/model/Idempotency.ts b/src/model/Idempotency.ts index 9524a67..d8dc776 100644 --- a/src/model/Idempotency.ts +++ b/src/model/Idempotency.ts @@ -150,11 +150,14 @@ export namespace Idempotency { /** * Schema for {@link Response}. * - * {@link Response.body} is required **and** nullable, which in this repository - * needs `requiredKey`: zod infers a required `z.nullable` key as optional - * under `strictNullChecks: false`. See that helper for why. `null` here means - * the original response genuinely had no body, which is a different claim from - * the field being absent, so the distinction has to survive. + * {@link Response.body} is required **and** nullable. It is wrapped in + * `requiredKey` because zod inferred a required `z.nullable` key as optional + * under the `strictNullChecks: false` setting this package previously used. + * That setting is now enabled and the inference is correct without the + * wrapper, which is retained pending a deliberate removal; see that helper. + * `null` here means the original response genuinely had no body, which is a + * different claim from the field being absent, so the distinction has to + * survive. */ const responseSchema = z.looseObject({ /** diff --git a/test/interface/place.test.ts b/test/interface/place.test.ts index 903996d..087c8e3 100644 --- a/test/interface/place.test.ts +++ b/test/interface/place.test.ts @@ -459,7 +459,7 @@ describe('PlaceDataSchema', () => { describe('unknown-key policy', () => { it('should preserve an undeclared field rather than dropping it', () => { const parsed = parsePlaceData({ ...validPlace(), legacyField: 'kept' }); - expect(parsed['legacyField']).toBe('kept'); + expect((parsed as unknown as Record)['legacyField']).toBe('kept'); }); }); diff --git a/test/interface/queue.test.ts b/test/interface/queue.test.ts index 6c9f8f6..f64cac4 100644 --- a/test/interface/queue.test.ts +++ b/test/interface/queue.test.ts @@ -165,7 +165,7 @@ describe('MessageQueueSchema', () => { describe('unknown-key policy', () => { it('should preserve an undeclared field rather than dropping it', () => { const parsed = parseMessageQueue({ pending: 1, legacyCounter: 9 }); - expect(parsed['legacyCounter']).toBe(9); + expect((parsed as unknown as Record)['legacyCounter']).toBe(9); }); }); diff --git a/test/interface/schema.test.ts b/test/interface/schema.test.ts index 8bafed9..7aa467a 100644 --- a/test/interface/schema.test.ts +++ b/test/interface/schema.test.ts @@ -381,13 +381,17 @@ describe('parse plumbing', () => { /** * Package-wide null and optionality policy. * - * `tsconfig.json` sets `strictNullChecks: false`, under which `null` is - * assignable to every type. Every `| null` annotation in this package is - * therefore **unenforced by our own compiler**, while still being emitted into - * the shipped `.d.ts` and enforced in a consumer that compiles strictly. That - * asymmetry means a schema rejecting `null` where the interface promises - * `| null` — or accepting it where the interface does not — is a real defect - * that nothing in this repository's type checking can catch. + * `tsconfig.json` previously set `strictNullChecks: false`, under which `null` + * was assignable to every type, leaving every `| null` annotation in this + * package **unenforced by our own compiler** while still being emitted into the + * shipped `.d.ts` and enforced in a consumer that compiles strictly. That flag + * is now enabled, so the annotations are enforced locally too. + * + * These tests remain the primary enforcement, because the asymmetry they guard + * is not one the compiler can see: a schema rejecting `null` where the interface + * promises `| null` — or accepting it where the interface does not — is a + * runtime-versus-declaration mismatch, and no amount of type checking compares + * those two artifacts against each other. * * These tests are the enforcement. Each entry states, explicitly, which keys * accept `null` and which are required, and the assertions compare that diff --git a/test/model/Account.test.ts b/test/model/Account.test.ts index f244c30..02ddfb3 100644 --- a/test/model/Account.test.ts +++ b/test/model/Account.test.ts @@ -1027,7 +1027,7 @@ describe('Account.Schema', () => { it('should preserve an undeclared link platform rather than dropping it', () => { const parsed = Account.parse({ ...validAccount(), links: { website: 'https://example.invalid', mastodon: 'https://example.invalid/m' } }); - expect(parsed.links?.['mastodon']).toBe('https://example.invalid/m'); + expect((parsed.links as unknown as Record | undefined)?.['mastodon']).toBe('https://example.invalid/m'); }); }); diff --git a/test/model/Block.test.ts b/test/model/Block.test.ts index 36231b9..ec53e25 100644 --- a/test/model/Block.test.ts +++ b/test/model/Block.test.ts @@ -316,7 +316,7 @@ describe('Block.Schema', () => { describe('unknown-key policy', () => { it('should preserve an undeclared field rather than dropping it', () => { const parsed = Block.parse({ ...validBlock(), alt: 'kept' }); - expect(parsed['alt']).toBe('kept'); + expect((parsed as unknown as Record)['alt']).toBe('kept'); }); }); diff --git a/test/model/Reservation.test.ts b/test/model/Reservation.test.ts index 4e211e4..d4f6462 100644 --- a/test/model/Reservation.test.ts +++ b/test/model/Reservation.test.ts @@ -127,7 +127,7 @@ describe('Reservation.Schema', () => { describe('unknown-key policy', () => { it('should preserve an undeclared field rather than dropping it', () => { const parsed = Reservation.parse({...validEntry(), seat: 'A1'}); - expect(parsed['seat']).toBe('A1'); + expect((parsed as unknown as Record)['seat']).toBe('A1'); }); }); diff --git a/tsconfig.json b/tsconfig.json index 73bb0eb..171321b 100755 --- a/tsconfig.json +++ b/tsconfig.json @@ -10,14 +10,14 @@ "ES2020" ], "target": "ES2020", - "noImplicitAny": false, + "noImplicitAny": true, "importHelpers": true, "outDir": "lib", "stripInternal": true, "inlineSources": false, "inlineSourceMap": false, "forceConsistentCasingInFileNames": true, - "strictNullChecks": false, + "strictNullChecks": true, "skipLibCheck": true, "allowJs": true, "moduleResolution": "Node16",