diff --git a/server/eslint-rules/fixtures/file.ts b/server/eslint-rules/fixtures/file.ts new file mode 100644 index 000000000..cb0ff5c3b --- /dev/null +++ b/server/eslint-rules/fixtures/file.ts @@ -0,0 +1 @@ +export {}; diff --git a/server/eslint-rules/fixtures/strict.ts b/server/eslint-rules/fixtures/strict.ts new file mode 100644 index 000000000..cb0ff5c3b --- /dev/null +++ b/server/eslint-rules/fixtures/strict.ts @@ -0,0 +1 @@ +export {}; diff --git a/server/eslint-rules/fixtures/tsconfig.json b/server/eslint-rules/fixtures/tsconfig.json new file mode 100644 index 000000000..023f92807 --- /dev/null +++ b/server/eslint-rules/fixtures/tsconfig.json @@ -0,0 +1,16 @@ +{ + // Fixture project for the RuleTester in tests/unit/eslint-rules/. It exists so + // typescript-eslint can build a real program for the virtual `file.ts` the + // tester feeds it. Kept outside src/ and tests/ on purpose: files here must not + // reach `tsc --noEmit`, `tsc -p tsconfig.tests.json` or the type-aware lint + // blocks, since the invalid cases are deliberately broken code. + "compilerOptions": { + "target": "ES2022", + "module": "commonjs", + "lib": ["ES2022"], + "strict": false, + "noEmit": true, + "skipLibCheck": true + }, + "include": ["file.ts"] +} diff --git a/server/eslint-rules/fixtures/tsconfig.strict.json b/server/eslint-rules/fixtures/tsconfig.strict.json new file mode 100644 index 000000000..48ce17e3c --- /dev/null +++ b/server/eslint-rules/fixtures/tsconfig.strict.json @@ -0,0 +1,15 @@ +{ + // Second fixture project, identical but with strictNullChecks ON. The server + // compiles with `strict: false`, where `Promise | null` collapses to + // `Promise` and the rule's nullable exemptions are unreachable — this one + // exists so those branches (`p ?? x`, `p === null`) are actually exercised. + "compilerOptions": { + "target": "ES2022", + "module": "commonjs", + "lib": ["ES2022"], + "strict": true, + "noEmit": true, + "skipLibCheck": true + }, + "include": ["strict.ts"] +} diff --git a/server/eslint-rules/no-promise-as-value.mjs b/server/eslint-rules/no-promise-as-value.mjs new file mode 100644 index 000000000..39ede4576 --- /dev/null +++ b/server/eslint-rules/no-promise-as-value.mjs @@ -0,0 +1,241 @@ +/** + * trek/no-promise-as-value — the blind spot the compiler cannot see. + * + * Phase 1 of the ORM migration turns hundreds of DB-touching methods `async`. + * TypeScript catches a missed `await` only when the promise is used in a way it + * can type-check: `promise.foo` (TS2339), a floating statement + * (no-floating-promises), a condition (no-misused-promises). It catches NOTHING + * when the promise is used as a plain value, because this project compiles with + * `strict: false` / `noImplicitAny: false`: + * + * settings['metric'] // element access on a promise -> `any` + * if (svc.get(id) === 'x') // always false, no error + * `${svc.count(id)}` // "[object Promise]" + * Object.keys(svc.all()) // [] + * JSON.stringify(svc.row(id)) // "{}" + * for (const r of svc.rows()) // not iterable, at runtime only + * + * This rule reports those shapes. It is DELIBERATELY CONSERVATIVE and has no + * fixer: it fires only when the resolved type is the global `Promise`/ + * `PromiseLike` itself (never a merely-thenable library type such as MikroORM's + * QueryBuilder or supertest's Test, both of which are chained on purpose), only + * when every non-nullish member of a union is one, and never inside + * `Promise.all`/`allSettled`/`race`/`any`, on `.then`/`.catch`/`.finally`, on + * `expect(p).resolves`/`.rejects`, or in an existence/identity check + * (`p === undefined`, `p === null`, `p ?? q`, `a === b` between two promises) — + * see isExistenceOrIdentityCheck below. A finding is a bug: add the `await`. + */ +import ts from 'typescript'; + +const NULLISH_FLAGS = ts.TypeFlags.Null | ts.TypeFlags.Undefined | ts.TypeFlags.Void; +const PASSTHROUGH_PROPS = new Set(['then', 'catch', 'finally', 'resolves', 'rejects']); +const COMBINATORS = new Set(['all', 'allSettled', 'race', 'any']); +const EQUALITY_OPERATORS = new Set(['==', '!=', '===', '!==']); +// Callbacks whose result is consumed as a value, where a promise is always a bug +// (every promise is truthy, so the predicate degenerates). `.map`/`.reduce` are +// excluded on purpose: returning promises there feeds Promise.all legitimately. +const PREDICATE_METHODS = new Set([ + 'filter', + 'find', + 'findLast', + 'findIndex', + 'findLastIndex', + 'some', + 'every', + 'sort', +]); +const OBJECT_CONSUMERS = new Set(['keys', 'values', 'entries', 'assign', 'fromEntries']); + +/** `String(p)` / `Number(p)` / `Object.keys(p)` / `JSON.stringify(p)` / `Array.isArray(p)`. */ +function isValueConsumerCall(callee) { + if (callee.type === 'Identifier') return callee.name === 'String' || callee.name === 'Number'; + if (callee.type !== 'MemberExpression' || callee.computed) return false; + const { object, property } = callee; + if (object.type !== 'Identifier' || property.type !== 'Identifier') return false; + if (object.name === 'Object') return OBJECT_CONSUMERS.has(property.name); + if (object.name === 'JSON') return property.name === 'stringify'; + if (object.name === 'Array') return property.name === 'isArray'; + return false; +} + +function isCombinatorCall(node) { + const callee = node.callee; + return ( + callee.type === 'MemberExpression' && + !callee.computed && + callee.object.type === 'Identifier' && + callee.object.name === 'Promise' && + callee.property.type === 'Identifier' && + COMBINATORS.has(callee.property.name) + ); +} + +/** Inside `Promise.all([...])` a promise IS the value; stop at the function boundary. */ +function isInsideCombinator(node) { + for (let cur = node.parent; cur; cur = cur.parent) { + if (cur.type === 'CallExpression' && isCombinatorCall(cur)) return true; + if (cur.type.includes('Function')) return false; + } + return false; +} + +function isNullLiteral(node) { + return (node.type === 'Literal' && node.value === null) || (node.type === 'Identifier' && node.name === 'undefined'); +} + +export default { + meta: { + type: 'problem', + docs: { + description: + 'Disallow using a Promise as a plain value (element access, comparison, interpolation, spread, iteration) — the missing-await shapes the compiler cannot see with strict mode off.', + }, + schema: [], + messages: { + promiseAsValue: 'Promise used as a value (missing await?): {{ text }}', + }, + }, + + create(context) { + const services = context.sourceCode.parserServices; + if (!services?.program || !services.esTreeNodeToTSNodeMap) return {}; + const checker = services.program.getTypeChecker(); + const reported = new WeakSet(); + + const isNullish = (type) => (type.flags & NULLISH_FLAGS) !== 0; + + function isPromiseType(type, tsNode) { + if (!type) return false; + if (type.isUnion()) { + const parts = type.types.filter((t) => !isNullish(t)); + return parts.length > 0 && parts.every((t) => isPromiseType(t, tsNode)); + } + const symbol = type.getSymbol() ?? type.aliasSymbol; + const name = symbol?.getName(); + if (name !== 'Promise' && name !== 'PromiseLike') return false; + const then = type.getProperty('then'); + if (!then) return false; + return checker.getTypeOfSymbolAtLocation(then, tsNode).getCallSignatures().length > 0; + } + + function typeOf(node) { + if (!node || node.type === 'AwaitExpression' || node.type === 'PrivateIdentifier') return null; + const tsNode = services.esTreeNodeToTSNodeMap.get(node); + return tsNode ? { type: checker.getTypeAtLocation(tsNode), tsNode } : null; + } + + function isPromise(node) { + const resolved = typeOf(node); + return !!resolved && isPromiseType(resolved.type, resolved.tsNode); + } + + function report(node) { + if (!node || reported.has(node) || isInsideCombinator(node)) return; + reported.add(node); + const raw = context.sourceCode.getText(node).replace(/\s+/g, ' '); + const text = raw.length > 60 ? `${raw.slice(0, 57)}...` : raw; + context.report({ node, messageId: 'promiseAsValue', data: { text } }); + } + + function check(node) { + if (isPromise(node)) report(node); + } + + /** + * `p === undefined` / `p === null` is how this codebase asks whether an + * in-flight promise is already parked in a map — a truthiness test there is + * what no-misused-promises rejects, so the explicit comparison IS the + * correct idiom (weather, collab, maps, exchange-rates, idempotency, the + * plugin drain, atlas). `chains.get(userId) === next` likewise asks whether + * the promise it stored is still the current one. Neither can be told apart + * from a missing await by type alone — with strictNullChecks off `Map.get()` + * is not even typed as nullable — so equality against nullish, and equality + * between two promises, are both exempt. `p === 'x'` still reports. + */ + function isExistenceOrIdentityCheck(node) { + if (isNullLiteral(node.left) || isNullLiteral(node.right)) return true; + return isPromise(node.left) && isPromise(node.right); + } + + return { + MemberExpression(node) { + const key = + node.property.type === 'Identifier' && !node.computed + ? node.property.name + : node.property.type === 'Literal' + ? node.property.value + : undefined; + if (typeof key === 'string' && PASSTHROUGH_PROPS.has(key)) return; + check(node.object); + }, + + BinaryExpression(node) { + // `p instanceof Promise` is the legitimate way to ask. + if (node.operator === 'instanceof') return; + if (EQUALITY_OPERATORS.has(node.operator) && isExistenceOrIdentityCheck(node)) return; + check(node.left); + check(node.right); + }, + + LogicalExpression(node) { + // `p ?? x` is the same existence check as `p === undefined`, and with + // strictNullChecks off the type cannot say whether it is a real one. + if (node.operator === '??') return; + // Only the left operand of `&&`/`||`: it is the one evaluated as a test, + // where a promise is unconditionally truthy. The right operand IS the + // expression's value and gets caught at whatever consumes it. + check(node.left); + }, + + TemplateLiteral(node) { + node.expressions.forEach(check); + }, + + SpreadElement(node) { + check(node.argument); + }, + + UnaryExpression(node) { + if (node.operator === 'typeof') check(node.argument); + }, + + ForOfStatement(node) { + if (!node.await) check(node.right); + }, + + ForInStatement(node) { + check(node.right); + }, + + CallExpression(node) { + if (isValueConsumerCall(node.callee)) { + node.arguments.forEach((arg) => { + if (arg.type !== 'SpreadElement') check(arg); + }); + return; + } + const callee = node.callee; + if ( + callee.type !== 'MemberExpression' || + callee.computed || + callee.property.type !== 'Identifier' || + !PREDICATE_METHODS.has(callee.property.name) + ) { + return; + } + for (const arg of node.arguments) { + if (arg.type !== 'ArrowFunctionExpression' && arg.type !== 'FunctionExpression') continue; + const resolved = typeOf(arg); + if (!resolved) continue; + const signatures = resolved.type.getCallSignatures(); + if ( + signatures.length > 0 && + signatures.every((sig) => isPromiseType(checker.getReturnTypeOfSignature(sig), resolved.tsNode)) + ) { + report(arg.body.type === 'BlockStatement' ? arg : arg.body); + } + } + }, + }; + }, +}; diff --git a/server/eslint.config.mjs b/server/eslint.config.mjs index cecebbce9..0a69623b4 100644 --- a/server/eslint.config.mjs +++ b/server/eslint.config.mjs @@ -4,6 +4,11 @@ import gitignore from 'eslint-config-flat-gitignore'; import eslintConfigPrettier from 'eslint-config-prettier'; import tseslint from 'typescript-eslint'; +import noPromiseAsValue from './eslint-rules/no-promise-as-value.mjs'; + +// Local rules live in eslint-rules/ and are exposed under the `trek/` prefix. +const trek = { rules: { 'no-promise-as-value': noPromiseAsValue } }; + export default tseslint.config( gitignore({ strict: false }), { @@ -56,7 +61,14 @@ export default tseslint.config( }, { files: ['src/**/*.ts', 'tests/**/*.ts'], + plugins: { trek }, rules: { + // trek/no-promise-as-value closes the hole the two rules below leave open: + // with `strict: false`, a promise used as a plain VALUE (element access, + // comparison, interpolation, spread, iteration, Object.keys/JSON.stringify) + // type-checks clean and silently produces `any`, `false` or `[object + // Promise]`. See eslint-rules/no-promise-as-value.mjs. + 'trek/no-promise-as-value': 'error', // --- The promise safety net for the Phase 1 async sweep (ORM migration) --- // Turning a DB-touching method `async` makes every unawaited call site a // silently reordered write, and every `if (svc.exists(id))` permanently diff --git a/server/tests/unit/eslint-rules/no-promise-as-value.test.ts b/server/tests/unit/eslint-rules/no-promise-as-value.test.ts new file mode 100644 index 000000000..541b51641 --- /dev/null +++ b/server/tests/unit/eslint-rules/no-promise-as-value.test.ts @@ -0,0 +1,134 @@ +import noPromiseAsValue from '../../../eslint-rules/no-promise-as-value.mjs'; +import tsParser from '@typescript-eslint/parser'; + +import { RuleTester, type Rule } from 'eslint'; +import path from 'path'; + +// The rule only exists because it is TYPE-aware, so the tester runs against a +// real TypeScript program: eslint-rules/fixtures/ holds a tsconfig whose single +// file is the one the tester feeds code into. It lives outside src/ and tests/ +// so the deliberately-broken snippets below never reach tsc or the lint gates. +const fixtureRoot = path.resolve(__dirname, '../../../eslint-rules/fixtures'); + +const ruleTester = new RuleTester({ + languageOptions: { + parser: tsParser, + parserOptions: { + project: './tsconfig.json', + tsconfigRootDir: fixtureRoot, + }, + }, +}); + +const PRELUDE = ` +declare function getSettings(id: number): Promise>; +declare function getName(id: number): Promise; +declare function getCount(): Promise; +declare function getRows(): Promise; +declare function maybeSettings(id: number): Promise> | null; +declare function syncName(id: number): string; +declare const rows: { id: number; active: boolean }[]; +declare const expect: (value: unknown) => { resolves: { toBe(value: unknown): void } }; +`; + +/** Every case shares the declarations above; offsets never matter, only counts. */ +const withPrelude = (code: string) => `${PRELUDE}${code}\n`; + +const valid = [ + // Awaited first — the shape every finding is fixed into. + `const s = await getSettings(1); const v = s['metric']; void v;`, + `const hit = (await getName(1)) === 'x'; void hit;`, + `const msg = \`\${await getCount()}\`; void msg;`, + `const all = [...(await getRows())]; void all;`, + `for (const r of await getRows()) { void r; }`, + `const keys = Object.keys(await getSettings(1)); void keys;`, + // Promise-shaped uses that are correct by construction. + `const n = getName(1).then((x) => x.length); void n;`, + `const both = await Promise.all([getName(1), getCount()]); void both;`, + `const joined = \`\${(await Promise.all([getName(1)]))[0]}\`; void joined;`, + `expect(getName(1)).resolves.toBe('x');`, + `const isP = getName(1) instanceof Promise; void isP;`, + // Existence / identity checks on a parked promise: the in-flight-map idiom the + // server uses everywhere, and the one a truthiness test would get wrong. + `const inflight = new Map>(); const hit = inflight.get('k'); if (hit !== undefined) { void hit; }`, + `let building: Promise = getName(1); if (building === null) { void 0; }`, + `const chains = new Map>(); const next = Promise.resolve(); if (chains.get(1) === next) { void 0; }`, + `const chains = new Map>(); const prev = chains.get(1) ?? Promise.resolve(); void prev;`, + `void getName(1);`, + // Non-promise values, and the array callbacks a promise legitimately flows through. + `const same = syncName(1) === 'x'; void same;`, + `const active = rows.filter((r) => r.active); void active;`, + `const pending = rows.map((r) => getName(r.id)); void pending;`, +].map((code) => ({ code: withPrelude(code), filename: path.join(fixtureRoot, 'file.ts') })); + +const invalid = [ + // Element access: compiles to `any` with strict mode off — the live miss Task 2 found. + `const v = getSettings(1)['metric']; void v;`, + // Property access on a promise-typed object, bracket-free. + `const v = getSettings(1).metric; void v;`, + // Comparisons: always false, no error. + `if (getName(1) === 'x') { void 0; }`, + `const n = getCount() + 1; void n;`, + `const has = 'metric' in getSettings(1); void has;`, + // Interpolation: "[object Promise]". + `const msg = \`\${getCount()}\`; void msg;`, + // Spread: neither iterable nor enumerable at runtime. + `const a = [...getRows()]; void a;`, + `const o = { ...getSettings(1) }; void o;`, + // Plain-value consumers. + `Object.keys(getSettings(1));`, + `JSON.stringify(getSettings(1));`, + `Array.isArray(getRows());`, + `const t = typeof getName(1); void t;`, + // Iteration. + `for (const r of getRows()) { void r; }`, + `for (const k in getSettings(1)) { void k; }`, + // Logical left operand: always truthy, so the fallback is dead code. + `const v = getName(1) || 'x'; void v;`, + // A predicate callback that returns a promise: every element passes. + `const active = rows.filter((r) => getName(r.id)); void active;`, +].map((code) => ({ + code: withPrelude(code), + filename: path.join(fixtureRoot, 'file.ts'), + errors: [{ messageId: 'promiseAsValue' }], +})); + +ruleTester.run('trek/no-promise-as-value', noPromiseAsValue as unknown as Rule.RuleModule, { + valid, + invalid, +}); + +// Under strictNullChecks a promise-returning call can genuinely be nullable, and +// `p ?? x` / `p === null` are then real checks rather than missing awaits. The +// server compiles with `strict: false`, where that union collapses and these +// branches are unreachable — so they get their own project to be tested in. +const strictTester = new RuleTester({ + languageOptions: { + parser: tsParser, + parserOptions: { project: './tsconfig.strict.json', tsconfigRootDir: fixtureRoot }, + }, +}); + +const strictFile = path.join(fixtureRoot, 'strict.ts'); + +strictTester.run('trek/no-promise-as-value (strictNullChecks)', noPromiseAsValue as unknown as Rule.RuleModule, { + valid: [ + { code: withPrelude(`const f = maybeSettings(1) ?? null; void f;`), filename: strictFile }, + { code: withPrelude(`const missing = maybeSettings(1) === null; void missing;`), filename: strictFile }, + ], + invalid: [ + { + // Nullable or not, indexing it is still a missing await. + code: withPrelude(`const v = maybeSettings(1)!['metric']; void v;`), + filename: strictFile, + errors: [{ messageId: 'promiseAsValue' }], + }, + { + // A union of `Promise | null` is still a promise once the nullish + // members are stripped — interpolating it is "[object Promise]". + code: withPrelude(`const msg = \`\${maybeSettings(1)}\`; void msg;`), + filename: strictFile, + errors: [{ messageId: 'promiseAsValue' }], + }, + ], +}); diff --git a/server/tests/unit/plugins/plugin-instance-settings.test.ts b/server/tests/unit/plugins/plugin-instance-settings.test.ts index 44c0eb518..973f0a6f4 100644 --- a/server/tests/unit/plugins/plugin-instance-settings.test.ts +++ b/server/tests/unit/plugins/plugin-instance-settings.test.ts @@ -362,8 +362,8 @@ describe('instance-scope actions (admin)', () => { .run(pluginId, key, key, scope); } const adminReq = { user: { id: 42 } } as unknown as Request; - function controller(invoke = vi.fn(async () => ({ ok: true, message: 'pong' }))) { - const rt = createPluginRuntime(new DatabaseService(dbConn)); + async function controller(invoke = vi.fn(async () => ({ ok: true, message: 'pong' }))) { + const rt = await createPluginRuntime(new DatabaseService(dbConn)); // isActive normally reflects the supervisor's live child map, which nothing here // spawns — so it's stubbed to read the same DB status the test itself flips, // mirroring what an actually-activated plugin would report. @@ -375,25 +375,25 @@ describe('instance-scope actions (admin)', () => { beforeEach(() => { testDb.prepare('DELETE FROM plugin_actions').run(); }); - it('ACT-ADM-001 — GET config lists the instance actions and none of the user ones', () => { + it('ACT-ADM-001 — GET config lists the instance actions and none of the user ones', async () => { install('p'); declareAction('p', 'purge', 'instance'); declareAction('p', 'testConnection', 'user'); - const { c } = controller(); + const { c } = await controller(); expect(c.getConfig('p').actions).toEqual([{ key: 'purge', label: 'purge', hint: undefined, danger: false, scope: 'instance' }]); }); it('ACT-ADM-002 — POST runs the action as the clicking admin in the instance scope', async () => { install('p'); testDb.prepare("UPDATE plugins SET status = 'active', enabled = 1 WHERE id = 'p'").run(); - const { c, invoke } = controller(); + const { c, invoke } = await controller(); expect(await c.runAction('p', 'purge', adminReq)).toEqual({ ok: true, message: 'pong' }); expect(invoke).toHaveBeenCalledWith('p', 'purge', 42, 'instance'); }); it('ACT-ADM-003 — an inactive plugin answers 404 like the user route', async () => { install('p'); - const { c, invoke } = controller(); + const { c, invoke } = await controller(); await expect(c.runAction('p', 'purge', adminReq)).rejects.toMatchObject({ status: 404, response: { error: 'Plugin is not active' } }); expect(invoke).not.toHaveBeenCalled(); }); @@ -401,7 +401,7 @@ describe('instance-scope actions (admin)', () => { it('ACT-ADM-004 — a refused key is a failed RESULT, not a server error', async () => { install('p'); testDb.prepare("UPDATE plugins SET status = 'active', enabled = 1 WHERE id = 'p'").run(); - const { c } = controller(vi.fn(async () => { throw new Error('plugin p did not declare action "x" in scope instance'); })); + const { c } = await controller(vi.fn(async () => { throw new Error('plugin p did not declare action "x" in scope instance'); })); expect(await c.runAction('p', 'x', adminReq)).toEqual({ ok: false, message: 'plugin p did not declare action "x" in scope instance' }); }); });