diff --git a/.changeset/service-knowledge-test-tsc-program.md b/.changeset/service-knowledge-test-tsc-program.md new file mode 100644 index 0000000000..413e4e417f --- /dev/null +++ b/.changeset/service-knowledge-test-tsc-program.md @@ -0,0 +1,86 @@ +--- +"@objectstack/service-knowledge": patch +--- + +fix(service-knowledge): put the test layer in front of tsc, and repair the four defects it was hiding (#15049) + +`packages/services/service-knowledge` had **no `typecheck` script at all** — +its scripts were `build` and `test` — so no tsc program anywhere read this +package. Turbo/CI typecheck lanes skipped it silently, because a zero-matching +filter run exits 0. `tsup` transpiles with esbuild and `vitest` runs through +esbuild type-**stripping**; neither type-checks. The package's own +`tsconfig.json` does include the tests and always did, so the program that +would have read them already existed and was simply never invoked — the same +shape `@objectstack/service-cluster` (#14181) reached the ledger by. + +**Measured before any repair**, dependency closure built first: `tsc --noEmit` +against the existing `tsconfig.json` (undivided, BUILD/NodeNext semantics) +read **10** raw errors, matching the `DEBT` entry this PR deletes exactly. The +new sibling `tsconfig.test.json` (module semantics only — `esnext` / `bundler` +/ `lib: ES2022`, matching how vitest actually executes these files; strictness +untouched) read **4** under the correct split — not the ledger's own 3-code-tier +guess. Fixing the 3 TS2835 (three relative test imports missing `.js`, required +by `moduleResolution: NodeNext`) removed the noise cascade (4 TS7006, every +`(h) => h.documentId)` callback over a `KnowledgeService` search result that +had degraded to `any`) and, in doing so, re-enabled a TypeScript excess-property +check the cascade had been suppressing — uncovering a 4th real error the +undivided reading had masked entirely. + +**The four code-tier defects, all in the test file, all in the test file's own +typing — never in `src/`:** + +1. `roles: ['member']` in one `ExecutionContext` object literal (TS2353 once + the excess-property check could see it) — a field the spec renamed to + `positions` (`execution-context.zod.ts`: *"Position names held by the + user … Formerly `roles`"*), that no check had ever read against the + renamed type. Every other `executionContext` literal in this file already + used `positions`; this one was simply never checked before. Fixed by + renaming it — `ExecutionContext` itself is untouched and correct. +2. `buildSetup`'s `vi.fn()` stub for `IDataEngine.find` typed its second + parameter as `{ context: { isSystem?: boolean } }`, omitting the `where` + field the real call site (`knowledge-service.ts`'s RLS re-check) actually + passes. `expect(opts.where).toEqual(...)` then read a property TypeScript + correctly said did not exist (TS2339). Fixed by widening the mock's + parameter type to match the call it stubs (`where` and `fields` added), + not by loosening the assertion. +3 & 4. Two more `vi.fn()` mocks (`upsertSpy`/`deleteSpy`/`searchSpy` in + `makeAdapter`, and a `find` mock in the reindex test) had **no** parameter + type at all, so TypeScript inferred a zero-argument implementation and + `.mock.calls[N]` was typed as an array of **empty tuples**. Indexing past + that boundary (`.mock.calls[0][1]`) is a genuine tuple-length error + (TS2493), and casting the resulting `undefined` onward compounded into + TS2352. Fixed the reindex-test `find` mock by typing its parameters to + match the real `reindexSource` call site (`where`, `limit`, `context`); + the `makeAdapter` stubs were reached only through the 3 TS2835 (below) and + needed no change of their own once those were fixed. + +**The three TS2835 are repaired directly** (`.js` added to three relative +specifiers in the test files), not routed around by excluding tests from +`tsconfig.json` — which stays exactly as it is, per the family's own rule +(AGENTS.md: never add such an exclusion). Because this package's build config +already includes the tests, its `typecheck` script's own `tsc --noEmit tsconfig.json` +step reads these same files under NodeNext regardless of the new sibling +config, so they needed fixing either way — unlike `service-cluster`, whose test +files already carried the extension and needed no import repair. + +Wired by the #14062 / #5286 route: `tsconfig.test.json` named by a new +`typecheck` script through the shared `check:test-typecheck` gate. No +`test-typecheck-debt.json` is added — its **absence is the zero**: the gate +reads a missing ledger as no entries, under which any error in any file here +is immediately red. After the repair, **both** readings (`tsconfig.json` and +`tsconfig.test.json`) are 0. + +The package's `DEBT` entry in `scripts/check-type-check-coverage.mjs` +(`errors: 10`) is **deleted**, not lowered — the graduation the ratchet's own +invariant requires. `scripts/check-type-source-resolution.mjs` gains a +registry entry for the three workspace deps (`core`, `objectql`, `spec`) now +reached only through the new `tsconfig.test.json` program (the #11490 +onboarding-limb re-baseline, same route `service-cluster` took): `paths` was +measured and rejected — redirecting those three deps to source takes this +package's test layer from 0 errors to 487, all TS6059, all in another +package's source. + +No runtime code changes: `src/**` (excluding tests) is byte-identical, so no +shipped behaviour moves. The `patch` level reflects the published +`package.json` gaining `typecheck` / `check:test-typecheck` scripts and a +`tsx` devDependency. diff --git a/packages/services/service-knowledge/package.json b/packages/services/service-knowledge/package.json index 65597aa943..3caf1cfece 100644 --- a/packages/services/service-knowledge/package.json +++ b/packages/services/service-knowledge/package.json @@ -20,7 +20,9 @@ }, "scripts": { "build": "tsup --config ../../../tsup.config.ts && node ../../../scripts/check-dts-emitted.mjs", - "test": "vitest run" + "test": "vitest run", + "typecheck": "tsc --noEmit && pnpm check:test-typecheck", + "check:test-typecheck": "tsx ../../../scripts/check-test-typecheck.mts --self-test && tsx ../../../scripts/check-test-typecheck.mts --package packages/services/service-knowledge --project tsconfig.test.json" }, "dependencies": { "@objectstack/core": "workspace:*", @@ -29,6 +31,7 @@ "devDependencies": { "@objectstack/objectql": "workspace:*", "@types/node": "^26.2.0", + "tsx": "^4.23.12", "typescript": "^6.0.3", "vitest": "^4.1.10" }, diff --git a/packages/services/service-knowledge/src/__tests__/event-sync-data-events.test.ts b/packages/services/service-knowledge/src/__tests__/event-sync-data-events.test.ts index 757c0d6e6b..26205cc5ea 100644 --- a/packages/services/service-knowledge/src/__tests__/event-sync-data-events.test.ts +++ b/packages/services/service-knowledge/src/__tests__/event-sync-data-events.test.ts @@ -12,8 +12,8 @@ import { describe, it, expect, vi } from 'vitest'; import type { RealtimeEventHandler, RealtimeEventPayload } from '@objectstack/spec/contracts'; -import { KnowledgeServicePlugin } from '../knowledge-service-plugin'; -import type { KnowledgeService } from '../knowledge-service'; +import { KnowledgeServicePlugin } from '../knowledge-service-plugin.js'; +import type { KnowledgeService } from '../knowledge-service.js'; function makeCtx() { let readyHook: (() => Promise) | undefined; diff --git a/packages/services/service-knowledge/src/__tests__/knowledge-service.test.ts b/packages/services/service-knowledge/src/__tests__/knowledge-service.test.ts index 84f54f8722..4cb4f0ec04 100644 --- a/packages/services/service-knowledge/src/__tests__/knowledge-service.test.ts +++ b/packages/services/service-knowledge/src/__tests__/knowledge-service.test.ts @@ -1,7 +1,7 @@ // Copyright (c) 2025 ObjectStack. Licensed under the Apache-2.0 license. import { describe, it, expect, vi } from 'vitest'; -import { KnowledgeService, documentIdFor, recordToDocument } from '../knowledge-service'; +import { KnowledgeService, documentIdFor, recordToDocument } from '../knowledge-service.js'; import type { IDataEngine, IKnowledgeAdapter, @@ -87,7 +87,10 @@ describe('KnowledgeService — adapter & source registry', () => { describe('KnowledgeService — permission-aware search', () => { function buildSetup(hits: KnowledgeHit[]) { const adapter = makeAdapter('memory', hits); - const findSpy = vi.fn(async (_obj: string, opts: { context: { isSystem?: boolean } }) => { + const findSpy = vi.fn(async ( + _obj: string, + opts: { where?: Record; fields?: string[]; context: { isSystem?: boolean } }, + ) => { if (opts.context?.isSystem) return [{ id: 'rec_1' }, { id: 'rec_2' }]; return [{ id: 'rec_1' }]; }); @@ -140,7 +143,7 @@ describe('KnowledgeService — permission-aware search', () => { ]; const { svc, findSpy } = buildSetup(hits); const out = await svc.search('q', { - executionContext: { userId: 'u1', roles: ['member'], permissions: [], isSystem: false }, + executionContext: { userId: 'u1', positions: ['member'], permissions: [], isSystem: false }, }); expect(out.map((h) => h.documentId)).toEqual(['d1']); expect(findSpy).toHaveBeenCalledOnce(); @@ -256,7 +259,10 @@ describe('KnowledgeService — event sync', () => { describe('KnowledgeService — reindex', () => { it('object source: walks IDataEngine with isSystem context and pushes docs', async () => { - const find = vi.fn(async () => [ + const find = vi.fn(async ( + _obj: string, + _opts: { where?: unknown; limit?: number; context: { isSystem?: boolean } }, + ) => [ { id: 'r1', title: 'T1', notes: 'N1', status: 'open' }, { id: 'r2', title: 'T2', notes: 'N2', status: 'done' }, ]); @@ -268,7 +274,7 @@ describe('KnowledgeService — reindex', () => { expect(res.ok).toBe(true); expect(res.indexed).toBe(2); expect(res.discovered).toBe(2); - expect((find.mock.calls[0][1] as { context: { isSystem?: boolean } }).context.isSystem).toBe(true); + expect(find.mock.calls[0][1].context.isSystem).toBe(true); expect(adapter.upsertSpy).toHaveBeenCalledOnce(); }); diff --git a/packages/services/service-knowledge/tsconfig.test.json b/packages/services/service-knowledge/tsconfig.test.json new file mode 100644 index 0000000000..d809613322 --- /dev/null +++ b/packages/services/service-knowledge/tsconfig.test.json @@ -0,0 +1,98 @@ +// The TEST-layer type-check program (#15049 — the `packages/services/**` +// instance of the class #14062 settled for `packages/plugins/**`, itself +// adopting the mechanism #5286 set for `packages/spec`, #5449 generalised, +// #12542 carried to `packages/rest`, #13176 to `packages/plugins/ +// plugin-security`, and #14181 / PR #15032 to `packages/services/ +// service-cluster` — the worked example this file is copied from). +// `tsconfig.json` beside this one stays exactly as it is: it is the BUILD +// config. This sibling puts the test layer in front of tsc under the module +// semantics vitest really executes it with, and `package.json`'s `typecheck` +// script NAMES it (via `check:test-typecheck --project`), because a config no +// script invokes is exactly the phantom this whole change is about. +// +// ⚠️ WHY `service-knowledge`, LIKE `service-cluster` AND UNLIKE `plugin-auth` / +// `plugin-sharing` / `core`: this package's `tsconfig.json` does NOT exclude +// tests (`include: ["src"]`, no `**/*.test.ts` exclusion) and never did, so the +// program that would have read them already existed and was simply never +// invoked -- no `typecheck` script named it. `tsup` type-strips, `vitest` +// type-strips; neither ran tsc. +// +// What differs from the build config, and what deliberately does NOT: +// - MODULE SEMANTICS ONLY, plus `lib`. The tests are written and executed as +// ESM by vitest (esbuild/vite). Matching that is FIDELITY, not laxity: it is +// the same subtraction `packages/spec`, `packages/rest`, the +// `packages/plugins/**` family and `service-cluster` each made. +// - ⛔ STRICTNESS IS UNTOUCHED. `strict`, `noUnusedLocals`, +// `noUnusedParameters`, `noImplicitReturns`, `noFallthroughCasesInSwitch`, +// `rootDir`, `paths` and `types` are all INHERITED from `tsconfig.json` +// (and through it the root config), and none of them is re-declared here. +// ⚠️ A child that declared its own `paths` would REPLACE the parent map +// rather than merge into it, silently sending a source-resolved specifier +// back to `dist/` — a BUILD ARTIFACT — so this file declares none. +// Nothing here may loosen a type rule; if a test does not compile, that is +// the finding. +// - `lib: ["ES2022"]`, for the same reason `packages/rest` states: the root +// config's `lib` is ES2020 and vitest runs on a Node that has es2022 +// builtins, so the gap is reported as TS2550 about the CHECK. No `DOM`: +// nothing in this layer touches a browser global. +// +// MEASURED, workspace closure built first (`tsc --noEmit --pretty false +// --listFiles -p `, and the same command without `--listFiles`), on a +// checkout at merge-base 2cc4610304, BEFORE any repair: +// +// files in this program (test config) 409 +// files in tsconfig.json (build config) 441 +// own `src/**/*.test.ts` in the program 4 +// errors under BUILD semantics (tsconfig.json, undivided) 10 +// errors under THIS config (the split) 4 +// +// The 10 undivided matched the DEBT entry this PR deletes exactly: 3 TS2835 +// (config-tier -- three relative test imports missing `.js`, required by +// `moduleResolution: NodeNext`) + 4 TS7006 (noise -- `KnowledgeService` +// resolving to `any` through the unresolved imports cascades into every +// `(h) => h.documentId)` callback over its return value) + 3 code-tier the +// ledger's note itemised (TS2339/TS2352/TS2493). +// +// The split did NOT confirm the ledger's 3-code-tier guess -- it found 4, the +// same "a tier split read off an unrepaired config is a guess about what is +// UNDER it" lesson this file's sibling ledger note states for `metadata` and +// `service-storage`: stripping the config-tier noise re-enabled an EXCESS +// PROPERTY CHECK that the `any`-typed parameter had been suppressing, and it +// caught a real one -- `roles: ['member']` in one `ExecutionContext` literal, +// stale since the field was renamed to `positions` (`execution-context.zod.ts` +// docs it: "Formerly `roles`"). The other 3 were exactly the ledger's guess: +// two `vi.fn()` mocks stubbing `IDataEngine.find` typed with fewer parameters +// than the real call site they stand in for, so `.mock.calls[N]` indexed past +// a TS-inferred EMPTY tuple (TS2493, plus the `as` cast off it reading TS2352) +// and a third mock's param type omitted the `where` field the real call +// passes (TS2339). All 4 are fixed in the test file, matching each mock's type +// to the call site it stubs -- never widening the mock, never touching +// `ExecutionContext` (which is correct; the test's stale field name was not). +// +// The three TS2835 are ALSO repaired directly (`.js` added to the three +// relative specifiers), because `tsconfig.json` genuinely DOES include the +// tests -- same as `service-cluster` -- so the package's `typecheck` script's +// bare `tsc --noEmit` step reads these same files under NodeNext and needs +// them resolvable regardless of this sibling config; unlike `service-cluster`, +// this package's test files had NOT already been written with the extension. +// After both repairs, BOTH readings are 0 -- see the PR body / changeset for +// the fix-by-fix account. +// +// There is NO `test-typecheck-debt.json` beside this config, and its ABSENCE is +// the zero: `check:test-typecheck` reads a missing ledger as `{ entries: {} }`, +// under which ANY error in ANY file here is red immediately, with no entry to be +// added to. If this package ever acquires residue that cannot be fixed in the +// PR that causes it, THAT is when a ledger and a `gen:test-typecheck-debt` +// script are owed — and adding one is maintainer-only (#5286), exactly as the +// gate says when it refuses. +{ + "extends": "./tsconfig.json", + "compilerOptions": { + "noEmit": true, + "module": "esnext", + "moduleResolution": "bundler", + "lib": ["ES2022"] + }, + "include": ["src/**/*"], + "exclude": ["node_modules", "dist"] +} diff --git a/pnpm-lock.yaml b/pnpm-lock.yaml index 29dc9584df..a615c9086e 100644 --- a/pnpm-lock.yaml +++ b/pnpm-lock.yaml @@ -2526,6 +2526,9 @@ importers: '@types/node': specifier: ^26.2.0 version: 26.2.0 + tsx: + specifier: ^4.23.12 + version: 4.23.12 typescript: specifier: ^6.0.3 version: 6.0.3 diff --git a/scripts/check-type-check-coverage.mjs b/scripts/check-type-check-coverage.mjs index 46e198d986..420de3c9ec 100644 --- a/scripts/check-type-check-coverage.mjs +++ b/scripts/check-type-check-coverage.mjs @@ -673,6 +673,27 @@ const ROOT_PROGRAM_COUPLED_SCRIPT = 'scripts/check-test-typecheck.mts'; // ran even though that config DOES include the tests. Repaired by the #5286 // route -- a `tsconfig.test.json` over the test layer, named by a new `typecheck` // script -- so the entry is deleted rather than lowered. +// +// `@objectstack/service-knowledge` GRADUATED from this ledger too (#15049, +// PR #15032's sibling for `packages/services/**`; entry: 10 raw, repaired to +// 0 under BOTH the build config and the new `tsconfig.test.json` split). This +// one is worth a line because the split did NOT confirm this entry's own +// 3-code-tier guess -- it found 4, the same "a tier split read off an +// unrepaired config is a guess about what is UNDER it" lesson the paragraph +// above states for `metadata` and `service-storage`. Fixing the 3 TS2835 (the +// config-tier third, and the noise: the unresolved imports made +// `KnowledgeService` `any`, which suppressed the TypeScript excess-property +// check on an `ExecutionContext` literal) uncovered a 4th real error the +// undivided reading had masked: `roles: ['member']`, a field the spec renamed +// to `positions` (`execution-context.zod.ts`: "Formerly `roles`") that no +// check had ever read with the renamed type. The other 3 code-tier errors +// were exactly this entry's guess: two `vi.fn()` mocks stubbing +// `IDataEngine.find` typed with fewer parameters than the call site they +// stand in for, so `.mock.calls[N]` indexed past a TS-inferred EMPTY tuple +// (TS2493/TS2352), and a third mock's parameter type omitted the `where` +// field the real call passes (TS2339). All 4 are fixed in the test file, +// matching each mock's type to the call site it stubs; `ExecutionContext` +// itself was not touched (it was correct -- the test's field name was stale). const DEBT = { '@objectstack/cloud-connection': { errors: 13, @@ -700,12 +721,6 @@ const DEBT = { + 'by acquiring a second file, then 5 -> 3 by graduating the first -- so re-read what the pile is ' + 'made of before sizing it, never just the number.', }, - '@objectstack/service-knowledge': { - errors: 10, - note: 'code-tier 3 (TS2339/TS2352/TS2493); config-tier 3 (TS2835); noise 4 (TS7006). Re-measured 10 at ' - + '5ab08428, up from 8; code-tier is unchanged at 3, so the +2 is config-tier/noise. 8 of the 10 are ' - + 'in __tests__/knowledge-service.test.ts.', - }, '@objectstack/service-storage': { errors: 51, note: 'code-tier 8 (TS2339 x4, TS2347 x4); config-tier 26 (TS2835 x23, TS2550 x3); noise 17 ' diff --git a/scripts/check-type-source-resolution.mjs b/scripts/check-type-source-resolution.mjs index 246a684eed..3f3b214488 100644 --- a/scripts/check-type-source-resolution.mjs +++ b/scripts/check-type-source-resolution.mjs @@ -694,6 +694,50 @@ const KNOWN_DIST_RESOLVED_TYPE_IMPORTS = { // // so +1 program, +1 pair, +1 package -- this entry and nothing else. '@objectstack/service-i18n': ['@objectstack/spec'], + // #15049 re-baseline (the onboarding limb above): a NEW entry, reached ONLY + // through `tsconfig.test.json` -- a program this card ADDED, exactly the + // #14181 shape one package over (`service-cluster`, directly above): this + // package's `typecheck` script was ABSENT before this card, its build config + // (`tsconfig.json`) does NOT exclude tests and never did, so it ran ONE + // counted program (the build config) with ZERO dist-resolved deps, and there + // is no pre-existing program a dep could be laundered through. All three + // deps here are annotated `via tsconfig.test.json` by this gate's own + // failure text. + // + // Provenance measured by varying only what the `typecheck` script NAMES, + // same checkout (`--list`, totals as printed): + // + // no `typecheck` script (origin/main) absent 119 programs / 290 pairs + // names `tsconfig.json` only absent 119 programs / 290 pairs + // names `tsconfig.test.json` only PRESENT 120 programs / 293 pairs + // names both (this card) PRESENT 120 programs / 293 pairs + // + // Row 2 is the load-bearing one, same as `service-cluster`'s: the BUILD + // program carries no dist-resolved workspace type import at all, so the + // exposure is not merely first SEEN through the onboarded program, it is + // only REACHABLE through it. + // + // Numbers, `--list` before/after on the same checkout (before at the + // `service-cluster` merge, 2cc4610304; after with this card applied): + // + // before 58 of 78 packages, 119 programs, 290 pairs, 20 clean + // after 59 of 78 packages, 120 programs, 293 pairs, 19 clean + // + // so +1 package, +1 program, +3 pairs -- this entry and nothing else. + // + // Why the entry and not `paths`: MEASURED, not argued -- redirecting these + // three deps to source takes this package's test layer from 0 errors to + // 487, ALL of them TS6059 (`not under rootDir`) and every one of them in + // ANOTHER package's source (`packages/spec/src/**`, `packages/core/src/**`, + // `packages/objectql/src/**`) -- billed to a package that cannot pay them + // down. Same shape as `service-cluster`'s own 0 -> 435 (below) and #12570's + // +5 for `rest`, at a larger scale because this package's test layer pulls + // three workspace deps rather than two. The #5286 route this card took + // makes its OWN test files compile clean; `paths` would immediately re-bury + // that result under other packages' diagnostics. + '@objectstack/service-knowledge': [ + '@objectstack/core', '@objectstack/objectql', '@objectstack/spec', + ], // #11490 re-baseline: NEW entries — reached only through `tsconfig.scripts.json`. '@objectstack/service-messaging': ['@objectstack/spec'], '@objectstack/service-realtime': ['@objectstack/spec'],