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",