210 lines
10 KiB
TypeScript
210 lines
10 KiB
TypeScript
/**
|
|
* checks/defensive-guards.ts — "redundant defensive guarding" check.
|
|
*
|
|
* Detects defensive code that guards invariants the type system or an
|
|
* upstream validation already guarantees, and removes the redundant guards
|
|
* while preserving guards that protect genuine external boundaries (user
|
|
* input, IO, parsing, untrusted data). The rubric encodes the engineering rule:
|
|
* no compatibility layers or fallbacks meant to be "replaced later" — remove
|
|
* them outright rather than layering over them.
|
|
*
|
|
* Flagged smells (non-exhaustive):
|
|
* - redundant-null-check — null/undefined check on a value whose declared
|
|
* type is already non-nullable.
|
|
* - swallowing-try-catch — try/catch that silently discards the error
|
|
* (empty catch, catch that only logs, or catch returning a fallback that
|
|
* hides the failure).
|
|
* - rethrow-only-try-catch — try/catch whose body only rethrows the exact
|
|
* error, adding nothing.
|
|
* - error-masking-fallback — `return defaultValue` / `|| fallback` in a
|
|
* catch that masks a real failure with a plausible-but-wrong value.
|
|
* - defensive-guard-on-validated-input — re-checking input that a caller or
|
|
* parser already validated (e.g. asserting a parsed enum is in range).
|
|
* - compatibility-fallback — a fallback branch kept "for now" / "to be
|
|
* replaced later" (engineering rule: remove, don't layer).
|
|
*
|
|
* Kept (legitimate boundary guards):
|
|
* - untrusted input (HTTP params, CLI args, env vars, files on disk).
|
|
* - IO (network, filesystem, subprocess) where failures are expected.
|
|
* - parsing (`JSON.parse`, `parseInt`, `Date.parse`, schema decoders).
|
|
*
|
|
* Lifecycle:
|
|
* gate (need source files) → recon (shared) → scan sub-agent writes
|
|
* `<cwd>/.pygienium/checks/defensive-guards/findings.md` separating redundant
|
|
* guards from boundary guards → [with --fix] fix sub-agent removes redundant
|
|
* guards, preserves boundary guards, and writes `changes.md` distinguishing
|
|
* removed vs kept-with-reason.
|
|
*
|
|
* Registering this file is the ONLY wiring needed: `index.ts` auto-discovers
|
|
* `src/checks/*.ts`, so dropping this file exposes `/pygienium-defensive-guards`.
|
|
*
|
|
* @module pygienium/checks/defensive-guards
|
|
*/
|
|
|
|
import { join } from "node:path";
|
|
import {
|
|
registerCheck,
|
|
type CheckDefinition,
|
|
type CheckScope,
|
|
} from "./registry.js";
|
|
import { hasScopeSources, scopeRulesMarkdown } from "./scope.js";
|
|
|
|
/** Output directory for this check's persistent reports. */
|
|
export function defensiveGuardsOutputDir(cwd: string): string {
|
|
return join(cwd, ".pygienium", "checks", "defensive-guards");
|
|
}
|
|
|
|
/** `findings.md` path for this check. */
|
|
export function findingsPath(cwd: string): string {
|
|
return join(defensiveGuardsOutputDir(cwd), "findings.md");
|
|
}
|
|
|
|
/** `changes.md` path for this check. */
|
|
export function changesPath(cwd: string): string {
|
|
return join(defensiveGuardsOutputDir(cwd), "changes.md");
|
|
}
|
|
|
|
/**
|
|
* Gate: skip when the cwd has no inspectable source files at all — a workspace
|
|
* with zero source files gives the scanner nothing to analyse.
|
|
*/
|
|
function defensiveGuardsGate(cwd: string): string | undefined {
|
|
try {
|
|
if (hasScopeSources(cwd)) return undefined;
|
|
} catch {
|
|
// unreadable cwd → let the agent decide; don't block.
|
|
return undefined;
|
|
}
|
|
return "no source files found to inspect";
|
|
}
|
|
|
|
/**
|
|
* Verify hook: confirms the check actually produced its artifacts (mirrors
|
|
* {@link commentsVerify} / {@link todosVerify}). After scan `findings.md` must
|
|
* exist; after `--fix` `changes.md` must exist too. Catches a sub-agent that
|
|
* returns ok with no output — which would otherwise be a false `complete`.
|
|
*/
|
|
async function defensiveGuardsVerify(
|
|
scope: CheckScope,
|
|
): Promise<string | undefined> {
|
|
const { stat } = await import("node:fs/promises");
|
|
const f = findingsPath(scope.cwd);
|
|
try {
|
|
await stat(f);
|
|
} catch {
|
|
return `defensive-guards verify: expected findings.md at ${f} after scan, none found.`;
|
|
}
|
|
if (scope.fix) {
|
|
const c = changesPath(scope.cwd);
|
|
try {
|
|
await stat(c);
|
|
} catch {
|
|
return `defensive-guards verify: expected changes.md at ${c} after --fix, none found.`;
|
|
}
|
|
}
|
|
return undefined;
|
|
}
|
|
|
|
/**
|
|
* Build the scan task. The defensive-guards scanner agent inspects the target,
|
|
* classifies each guard as redundant or a legitimate boundary guard against the
|
|
* rubric, and writes a structured findings report to `findings.md`. The output
|
|
* path is passed into the task so both the real agent (which uses its `write`
|
|
* tool) and the deterministic fake runner (which understands `!write <path>
|
|
* <text>`) persist the report to the same location.
|
|
*
|
|
* The `!write`/`!echo` lines are the deterministic fallback the fake runner
|
|
* executes for tests/smoke runs; a real model-driven agent receives the whole
|
|
* prompt and writes a real analysis.
|
|
*/
|
|
function buildDefensiveGuardsScanTask(cwd: string, scope: CheckScope): string {
|
|
const findings = findingsPath(cwd);
|
|
const target = scope.target;
|
|
return [
|
|
`Inspect the target "${target}" (cwd: ${cwd}) for redundant defensive guarding.`,
|
|
`Classify every guard (null check, try/catch, fallback) against your rubric as`,
|
|
`either REDUNDANT (remove) or BOUNDARY (keep). Boundary guards protect real`,
|
|
`external boundaries: untrusted input, IO, and parsing. Redundant guards protect`,
|
|
`invariants the type system or upstream validation already guarantees.`,
|
|
`Write your full findings report to: ${findings}.`,
|
|
`findings.md must separate redundant guards from legitimate boundary guards,`,
|
|
`listing each with: kind, evidence, disposition (remove | keep-boundary), and`,
|
|
`reason.`,
|
|
`Then emit a one-line summary referencing the findings path.`,
|
|
``,
|
|
scopeRulesMarkdown(),
|
|
``,
|
|
`# Deterministic fallback (executed by the fake runner in tests):`,
|
|
`!write ${findings} # Defensive-guards findings | summary: 2 redundant guard(s) flagged, 1 boundary guard kept | ## 1. ${target}/noise.ts:2 | kind: redundant-null-check | evidence: \`if (name === null)\` on \`name\` whose declared type is \`string\` (non-nullable) | disposition: remove | reason: type system already guarantees non-null | ## 2. ${target}/noise.ts:7 | kind: swallowing-try-catch | evidence: try/catch around doThing() discards the error silently (empty catch body) | disposition: remove | reason: masks bugs; no error mapping or recovery logic | ## 3. ${target}/boundary.ts:2 | kind: parsing-guard | evidence: try/catch around JSON.parse(input) | disposition: keep-boundary | reason: protects an external parsing boundary (JSON.parse of untrusted input)`,
|
|
`!echo defensive-guards: 2 redundant, 1 boundary kept — see ${findings}`,
|
|
].join("\n");
|
|
}
|
|
|
|
/**
|
|
* Build the fix task. The fixer consumes the scan findings and removes ONLY
|
|
* redundant guards — those whose protected invariant is already guaranteed by
|
|
* the type system or upstream validation. Boundary guards (IO, parsing,
|
|
* untrusted input) are preserved untouched. Every action — removed or kept —
|
|
* is recorded in `changes.md`, distinguishing removed (auto) from kept with a
|
|
* reason (boundary).
|
|
*
|
|
* The fixer rewrites the affected source files with the redundant guards
|
|
* excised; compatibility fallbacks are removed outright (engineering rule:
|
|
* remove, don't layer), never left behind as a transitional shim.
|
|
*/
|
|
function buildDefensiveGuardsFixTask(
|
|
cwd: string,
|
|
scope: CheckScope,
|
|
findings: string,
|
|
): string {
|
|
const changes = changesPath(cwd);
|
|
const findingsFile = findingsPath(cwd);
|
|
const target = scope.target;
|
|
return [
|
|
`Remove redundant defensive guards found in the scan.`,
|
|
`cwd: ${cwd} target: ${target}`,
|
|
`Findings report (also persisted at ${findingsFile}):`,
|
|
`---`,
|
|
findings,
|
|
`---`,
|
|
``,
|
|
`Rules:`,
|
|
`- Remove ONLY redundant guards: null/undefined checks on non-nullable types,`,
|
|
` try/catch that only rethrows or swallows, fallback values that hide errors,`,
|
|
` defensive guards on already-validated input, and compatibility fallbacks.`,
|
|
`- PRESERVE boundary guards: anything protecting untrusted input, IO, or parsing`,
|
|
` (e.g. JSON.parse, network, filesystem, subprocess errors). Do not touch them.`,
|
|
`- No compatibility layers: remove fallbacks outright — never leave a shim meant`,
|
|
` to be "replaced later".`,
|
|
`- Apply the smallest diff that removes the guard without changing behaviour for`,
|
|
` the happy path. Preserve tests and existing conventions.`,
|
|
`- Write changes.md to ${changes} distinguishing removed (auto) from kept`,
|
|
` (boundary — with reason) for every finding.`,
|
|
``,
|
|
`# Deterministic removal (executed by the fake runner in tests):`,
|
|
`# Redundant null check + swallowing try/catch removed from noise.ts (auto).`,
|
|
`# JSON.parse boundary guard in boundary.ts preserved (boundary).`,
|
|
`!write ${target}/noise.ts // Cleaned by pygienium-defensive-guards: removed redundant null check on non-nullable \`name\` and the swallowing try/catch around doThing(). export function greet(name: string) { return \`hello \${name}\`; } export function swallow() { doThing(); } function doThing() {}`,
|
|
`!write ${changes} # Defensive-guards changes | summary: 2 removed, 1 kept (boundary) | ## Removed (auto) | 1. ${target}/noise.ts:2 — redundant-null-check — removed \`if (name === null) return ""\`; type system guarantees non-null | 2. ${target}/noise.ts:7 — swallowing-try-catch — removed the try/catch around doThing(); the error is no longer silently swallowed | ## Kept (boundary — with reason) | 1. ${target}/boundary.ts:2 — parsing-guard — kept: try/catch around JSON.parse protects an external parsing boundary (untrusted input)`,
|
|
`!echo defensive-guards: 2 removed, 1 kept (boundary) — see ${changes}`,
|
|
].join("\n");
|
|
}
|
|
|
|
/** The check definition; registers itself on import. */
|
|
const defensiveGuardsCheck: CheckDefinition = {
|
|
name: "defensive-guards",
|
|
label: "Defensive guards",
|
|
description:
|
|
"Remove redundant defensive guarding (null checks on non-nullable types, swallowing try/catch, masking fallbacks) while keeping boundary guards (IO, parsing, untrusted input).",
|
|
agentName: "defensive-guards",
|
|
phaseId: "analysis",
|
|
buildScanTask: buildDefensiveGuardsScanTask,
|
|
buildFixTask: buildDefensiveGuardsFixTask,
|
|
gate: defensiveGuardsGate,
|
|
verify: defensiveGuardsVerify,
|
|
};
|
|
|
|
registerCheck(defensiveGuardsCheck);
|
|
|
|
export { defensiveGuardsCheck };
|