diff --git a/README.md b/README.md index b1719c4..ea10b3e 100644 --- a/README.md +++ b/README.md @@ -135,6 +135,7 @@ Current checks focus on patterns that often show up in unreviewed generated code - [error-obscuring catch blocks](src/rules/error-obscuring/README.md) (default-return or generic replacement error) - [empty catch blocks](src/rules/empty-catch/README.md) - [promise `.catch()` default fallbacks](src/rules/promise-default-fallbacks/README.md) +- [stringified unknown errors](src/rules/stringified-unknown-errors/README.md) - [async wrapper / `return await` noise](src/rules/async-noise/README.md) - [pass-through wrappers](src/rules/pass-through-wrappers/README.md) - [barrel density](src/rules/barrel-density/README.md) diff --git a/src/default-registry.ts b/src/default-registry.ts index c740e45..a68ccd4 100644 --- a/src/default-registry.ts +++ b/src/default-registry.ts @@ -18,6 +18,7 @@ import { emptyCatchRule } from "./rules/empty-catch"; import { errorObscuringRule } from "./rules/error-obscuring"; import { errorSwallowingRule } from "./rules/error-swallowing"; import { promiseDefaultFallbacksRule } from "./rules/promise-default-fallbacks"; +import { stringifiedUnknownErrorsRule } from "./rules/stringified-unknown-errors"; import { barrelDensityRule } from "./rules/barrel-density"; import { directoryFanoutHotspotRule } from "./rules/directory-fanout-hotspot"; import { duplicateFunctionSignaturesRule } from "./rules/duplicate-function-signatures"; @@ -45,6 +46,7 @@ export function createDefaultRegistry(): Registry { registry.registerRule(errorObscuringRule); registry.registerRule(emptyCatchRule); registry.registerRule(promiseDefaultFallbacksRule); + registry.registerRule(stringifiedUnknownErrorsRule); registry.registerRule(barrelDensityRule); registry.registerRule(passThroughWrappersRule); registry.registerRule(duplicateFunctionSignaturesRule); diff --git a/src/rules/stringified-unknown-errors/README.md b/src/rules/stringified-unknown-errors/README.md new file mode 100644 index 0000000..a082b51 --- /dev/null +++ b/src/rules/stringified-unknown-errors/README.md @@ -0,0 +1,59 @@ +# defensive.stringified-unknown-errors + +Flags code that flattens unknown caught values into generic strings like `error.message` or `String(error)`. + +- **Family:** `defensive` +- **Severity:** `strong` +- **Scope:** `file` +- **Requires:** `file.ast` + +## How it works + +The rule looks for conditional expressions like: + +- `error instanceof Error ? error.message : String(error)` +- `err instanceof Error ? err.message : String(err)` +- equivalent forms assigned into `message` / `error` properties or returned directly + +This pattern is common in generated wrapper code that wants a quick printable error value. It is sometimes reasonable, but repeated use often flattens richer failure objects into plain strings and makes downstream diagnostics more generic. + +To avoid obvious vendored noise, the rule skips very large bundled/generated files over `5000` logical lines. + +## Flagged examples + +```ts +catch (error) { + return { success: false, error: error instanceof Error ? error.message : String(error) }; +} + +setError(err instanceof Error ? err.message : String(err)); + +const message = error instanceof Error ? error.message : String(error); +``` + +## Usually ignored + +```ts +catch (error) { + throw error; +} + +catch (error) { + logger.error({ error }); + return { success: false, error }; +} +``` + +## Scoring + +Each unknown-error stringification site adds `2` points. +The file total is capped at `8`. + +## Benchmark signal + +Full pinned benchmark against the exact `known-ai-vs-solid-oss` cohort: + +- Signal score: **0.88 / 1.00** +- Best separating metric: **findings / file (0.88)** +- Hit rate: **7/9 AI repos** vs **1/9 mature OSS repos** +- Full results: [experimental rule report](../../../reports/autoresearch-candidate-rule.md#defensivestringified-unknown-errors) diff --git a/src/rules/stringified-unknown-errors/index.ts b/src/rules/stringified-unknown-errors/index.ts new file mode 100644 index 0000000..41fe048 --- /dev/null +++ b/src/rules/stringified-unknown-errors/index.ts @@ -0,0 +1,169 @@ +import * as ts from "typescript"; +import type { RulePlugin } from "../../core/types"; +import { getLineNumber, unwrapExpression, walk } from "../../facts/ts-helpers"; +import { delta } from "../../rule-delta"; + +const MAX_LOGICAL_LINES = 5000; + +type MatchKind = + | "stringified-unknown-error" + | "returned-stringified-unknown-error" + | "property-stringified-unknown-error"; + +type StringifiedUnknownErrorMatch = { + line: number; + kind: MatchKind; +}; + +function isErrorIdentifier(expression: ts.Expression, allowedNames: Set): boolean { + const unwrapped = unwrapExpression(expression); + return ts.isIdentifier(unwrapped) && allowedNames.has(unwrapped.text); +} + +function isStringCallOfError(expression: ts.Expression, allowedNames: Set): boolean { + const unwrapped = unwrapExpression(expression); + return ( + ts.isCallExpression(unwrapped) && + ts.isIdentifier(unwrapped.expression) && + unwrapped.expression.text === "String" && + unwrapped.arguments.length === 1 && + isErrorIdentifier(unwrapped.arguments[0]!, allowedNames) + ); +} + +function isErrorMessageAccess(expression: ts.Expression, allowedNames: Set): boolean { + const unwrapped = unwrapExpression(expression); + return ( + ts.isPropertyAccessExpression(unwrapped) && + unwrapped.name.text === "message" && + isErrorIdentifier(unwrapped.expression, allowedNames) + ); +} + +function isErrorInstanceofCheck(expression: ts.Expression, allowedNames: Set): boolean { + const unwrapped = unwrapExpression(expression); + return ( + ts.isBinaryExpression(unwrapped) && + unwrapped.operatorToken.kind === ts.SyntaxKind.InstanceOfKeyword && + isErrorIdentifier(unwrapped.left, allowedNames) && + ts.isIdentifier(unwrapExpression(unwrapped.right)) && + unwrapExpression(unwrapped.right).text === "Error" + ); +} + +function summarizeConditional( + node: ts.ConditionalExpression, + sourceFile: ts.SourceFile, +): StringifiedUnknownErrorMatch | null { + const allowedNames = new Set(); + + const condition = unwrapExpression(node.condition); + if ( + ts.isBinaryExpression(condition) && + condition.operatorToken.kind === ts.SyntaxKind.BarBarToken + ) { + for (const side of [condition.left, condition.right]) { + const unwrapped = unwrapExpression(side); + if ( + ts.isBinaryExpression(unwrapped) && + unwrapped.operatorToken.kind === ts.SyntaxKind.InstanceOfKeyword + ) { + const left = unwrapExpression(unwrapped.left); + if (ts.isIdentifier(left)) { + allowedNames.add(left.text); + } + } + } + } else if (isErrorInstanceofCheck(condition, new Set(["error", "err", "e", "cause"]))) { + const left = unwrapExpression((condition as ts.BinaryExpression).left) as ts.Identifier; + allowedNames.add(left.text); + } + + if (allowedNames.size === 0) { + return null; + } + + const whenTrue = unwrapExpression(node.whenTrue); + const whenFalse = unwrapExpression(node.whenFalse); + const isEitherDirection = + (isErrorMessageAccess(whenTrue, allowedNames) && + isStringCallOfError(whenFalse, allowedNames)) || + (isErrorMessageAccess(whenFalse, allowedNames) && isStringCallOfError(whenTrue, allowedNames)); + + if (!isEitherDirection) { + return null; + } + + let kind: MatchKind = "stringified-unknown-error"; + if (ts.isReturnStatement(node.parent)) { + kind = "returned-stringified-unknown-error"; + } else if (ts.isPropertyAssignment(node.parent)) { + kind = "property-stringified-unknown-error"; + } + + return { + line: getLineNumber(sourceFile, node.getStart(sourceFile)), + kind, + }; +} + +function findStringifiedUnknownErrors(sourceFile: ts.SourceFile): StringifiedUnknownErrorMatch[] { + const matches: StringifiedUnknownErrorMatch[] = []; + + walk(sourceFile, (node) => { + if (!ts.isConditionalExpression(node)) { + return; + } + + const match = summarizeConditional(node, sourceFile); + if (match) { + matches.push(match); + } + }); + + return matches; +} + +export const stringifiedUnknownErrorsRule: RulePlugin = { + id: "defensive.stringified-unknown-errors", + family: "defensive", + severity: "strong", + scope: "file", + requires: ["file.ast"], + delta: delta.byLocations(), + supports(context) { + return context.scope === "file" && Boolean(context.file); + }, + evaluate(context) { + if (context.file!.logicalLineCount > MAX_LOGICAL_LINES) { + return []; + } + + const sourceFile = context.runtime.store.getFileFact( + context.file!.path, + "file.ast", + ); + if (!sourceFile) { + return []; + } + + const matches = findStringifiedUnknownErrors(sourceFile); + if (matches.length === 0) { + return []; + } + + return [ + { + ruleId: "defensive.stringified-unknown-errors", + family: "defensive", + severity: "strong", + scope: "file", + path: context.file!.path, + message: `Found ${matches.length} unknown-error normalization${matches.length === 1 ? "" : "s"} that collapse caught values into generic strings`, + evidence: matches.map((match) => `line ${match.line}: ${match.kind}`), + score: Math.min(8, matches.length * 2), + locations: matches.map((match) => ({ path: context.file!.path, line: match.line })), + }, + ]; + }, +}; diff --git a/tests/heuristics.test.ts b/tests/heuristics.test.ts index 7bf4e0c..df126b7 100644 --- a/tests/heuristics.test.ts +++ b/tests/heuristics.test.ts @@ -44,6 +44,11 @@ describe("heuristic rule pack", () => { " }", "}", "", + "export function getErrorMessage(error: unknown) {", + " const message = error instanceof Error ? error.message : String(error);", + " return { message };", + "}", + "", ].join("\n"), "src/service.ts": [ "function getData(id: string) {", @@ -80,6 +85,7 @@ describe("heuristic rule pack", () => { expect(ruleIds.has("comments.placeholder-comments")).toBe(true); expect(ruleIds.has("defensive.error-obscuring")).toBe(true); expect(ruleIds.has("defensive.promise-default-fallbacks")).toBe(true); + expect(ruleIds.has("defensive.stringified-unknown-errors")).toBe(true); expect(ruleIds.has("defensive.async-noise")).toBe(true); expect(ruleIds.has("structure.pass-through-wrappers")).toBe(true); expect(ruleIds.has("structure.barrel-density")).toBe(true); diff --git a/tests/stringified-unknown-errors.test.ts b/tests/stringified-unknown-errors.test.ts new file mode 100644 index 0000000..a45bad3 --- /dev/null +++ b/tests/stringified-unknown-errors.test.ts @@ -0,0 +1,106 @@ +import { afterEach, describe, expect, test } from "bun:test"; +import { mkdtemp, mkdir, rm, writeFile } from "node:fs/promises"; +import os from "node:os"; +import path from "node:path"; +import { DEFAULT_CONFIG } from "../src/config"; +import { analyzeRepository } from "../src/core/engine"; +import { Registry } from "../src/core/registry"; +import { createDefaultRegistry } from "../src/default-registry"; +import { stringifiedUnknownErrorsRule } from "../src/rules/stringified-unknown-errors"; + +const tempDirs: string[] = []; + +afterEach(async () => { + await Promise.all(tempDirs.splice(0).map((dir) => rm(dir, { recursive: true, force: true }))); +}); + +async function writeRepoFiles(rootDir: string, files: Record): Promise { + for (const [relativePath, content] of Object.entries(files)) { + const absolutePath = path.join(rootDir, relativePath); + await mkdir(path.dirname(absolutePath), { recursive: true }); + await writeFile(absolutePath, content); + } +} + +async function createTempRepo(files: Record): Promise { + const rootDir = await mkdtemp(path.join(os.tmpdir(), "slop-scan-stringified-errors-")); + tempDirs.push(rootDir); + await writeRepoFiles(rootDir, files); + return rootDir; +} + +function createCandidateRegistry(): Registry { + const baseRegistry = createDefaultRegistry(); + const registry = new Registry(); + + for (const language of baseRegistry.getLanguages()) { + registry.registerLanguage(language); + } + + for (const provider of baseRegistry.getFactProviders()) { + registry.registerFactProvider(provider); + } + + registry.registerRule(stringifiedUnknownErrorsRule); + return registry; +} + +describe("stringified-unknown-errors rule", () => { + test("flags unknown errors collapsed into generic strings", async () => { + const rootDir = await createTempRepo({ + "src/slop.ts": [ + "export function getMessage(error: unknown) {", + " return error instanceof Error ? error.message : String(error);", + "}", + "", + "export function toResult(err: unknown) {", + " return { ok: false, error: err instanceof Error ? err.message : String(err) };", + "}", + "", + "export function updateState(error: unknown) {", + " const next = error instanceof Error ? error.message : String(error);", + " return { message: next };", + "}", + ].join("\n"), + "src/legit.ts": [ + "export function rethrow(error: unknown) {", + " throw error;", + "}", + "", + "export function preserve(error: unknown) {", + " return { error };", + "}", + ].join("\n"), + }); + + const result = await analyzeRepository(rootDir, DEFAULT_CONFIG, createCandidateRegistry()); + const finding = result.findings.find( + (nextFinding) => nextFinding.ruleId === "defensive.stringified-unknown-errors", + ); + + expect(finding).toBeDefined(); + expect(finding?.path).toBe("src/slop.ts"); + expect(finding?.evidence).toEqual([ + "line 2: returned-stringified-unknown-error", + "line 6: property-stringified-unknown-error", + "line 10: stringified-unknown-error", + ]); + expect(result.findings).toHaveLength(1); + }); + + test("ignores giant bundled files that would otherwise create vendor noise", async () => { + const hugeFile = [ + ...Array.from({ length: 5001 }, (_, index) => `export const filler${index} = ${index};`), + "export const message = error instanceof Error ? error.message : String(error);", + "", + ].join("\n"); + + const rootDir = await createTempRepo({ + "src/bundle.ts": hugeFile, + }); + + const result = await analyzeRepository(rootDir, DEFAULT_CONFIG, createCandidateRegistry()); + + expect(result.findings).toHaveLength(0); + }); +});