diff --git a/CHANGELOG.md b/CHANGELOG.md index e4b51e1..3ca3621 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -6,6 +6,16 @@ adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.html). ## [Unreleased] +### Added + +- Optional `filter` front-matter field: a second-stage applicability command run + after a rule's globs match, with the matched paths appended as arguments. + grep-style exit codes (`0` applies, `1` skips, anything else / timeout / missing + command fails open and applies). Disable with `--no-filters` / `runFilters: +false`; bound with `--filter-timeout` / `filterTimeoutMs` (default 10000 ms). + Filter errors surface in the new `ReviewResult.warnings`. New exports: + `makeFilterExecutor`, `FilterResult`, `FilterExecutor`, `DiscoverOptions`. + ## [0.1.0] - 2026-06-26 ### Added diff --git a/Dockerfile b/Dockerfile index c32bb20..5033750 100644 --- a/Dockerfile +++ b/Dockerfile @@ -27,9 +27,12 @@ COPY tsconfig.json tsconfig.build.json ./ COPY src ./src RUN yarn build -# Scripts, sample rules, and entrypoint. +# Scripts, sample rules, unit tests, and entrypoint. The `test/` dir is needed +# for the `test` command (vitest); vitest is a devDependency already installed +# above by `yarn install --frozen-lockfile`. COPY scripts ./scripts COPY examples ./examples +COPY test ./test COPY docker/entrypoint.sh /usr/local/bin/entrypoint.sh RUN chmod +x /usr/local/bin/entrypoint.sh scripts/*.sh diff --git a/README.md b/README.md index 9ea86c4..67628df 100644 --- a/README.md +++ b/README.md @@ -53,11 +53,47 @@ globs: Use the project logger instead of `console.log` in non-test source files. ``` -| Field | Description | -| ------------- | ----------------------------------------------------------------------------- | -| `description` | Display name (falls back to the filename) | -| `globs` | Inline list or YAML list; `!` negates. A rule with no globs is never applied. | -| `reviewSkip` | If `true`, the rule is parsed but excluded from review | +| Field | Description | +| ------------- | ---------------------------------------------------------------------------------------------------------------------------------------------- | +| `description` | Display name (falls back to the filename) | +| `globs` | Inline list or YAML list; `!` negates. A rule with no globs is never applied. | +| `reviewSkip` | If `true`, the rule is parsed but excluded from review | +| `filter` | Optional command run after a glob match to decide if the rule applies. Matched paths are appended as args. See below. Absent ⇒ no extra check. | + +### Filtering beyond globs + +Globs match file _paths_. A `filter` command lets a rule also depend on file +_content_ or relationships between changes. After a rule's globs select the +changed files, its `filter` runs once with those paths appended as arguments, +and decides applicability by exit code: + +| Exit code | Meaning | +| ---------------------------------------------- | --------------------------------------------------- | +| `0` | Filter passed — the rule applies | +| `1` | Clean rejection — the rule is skipped | +| anything else, a missing command, or a timeout | Error — fail-open: the rule applies (and is warned) | + +```markdown +--- +description: No raw SQL in repositories +globs: + - 'src/repositories/**/*.ts' +filter: "grep -ilq 'select \\|insert \\|update '" +--- + +Use the query builder, not raw SQL strings, in repository classes. +``` + +Here `grep` exits `0` if any matched file contains a SQL keyword (rule applies), +`1` if none do (skipped). The command can be inline (with flags) or a script; +see [`examples/filters/`](./examples/filters/) and [`examples/rules/no-raw-sql.md`](./examples/rules/no-raw-sql.md). + +> **Filter commands execute with your privileges.** Treat the rules directory as +> trusted code, like a git hook. When reviewing an untrusted diff (e.g. a fork PR +> that could edit a `filter`), pass `--no-filters`. Note also that a filter reads +> files from the working tree — for `--diff ` reviews those may differ from +> the diffed revision, so content filters should query git (`git grep `) +> rather than read the tree. ## CLI @@ -78,12 +114,18 @@ agent-rules --working-tree --transport codex # Force an explicit transport command (any stdin->stdout program) agent-rules --working-tree --exec "claude -p --output-format json" + +# Review an untrusted diff without executing any rule `filter` commands +agent-rules --diff origin/main...HEAD --no-filters ``` Diff sources (exactly one): `--working-tree`, `--staged`, `--diff `. Run `agent-rules --help` for all options. Exit codes: `0` clean, `1` blocking findings, `2` error. +Rule `filter` commands run by default (including under `--list`); `--no-filters` +disables them and `--filter-timeout ` bounds each one (default `10000`). + ## Library ```ts diff --git a/docker/README.md b/docker/README.md index 1929989..68aa625 100644 --- a/docker/README.md +++ b/docker/README.md @@ -15,14 +15,20 @@ docker build -t agent-rules . # or: yarn docker:build ``` -## Hermetic smoke test (no credentials) +## Hermetic tests (no credentials) -The default command runs `scripts/smoke.sh` — a fake `--exec` transport, so it -needs no agent login and is safe anywhere (also what CI runs). +These need no agent login and are safe to run anywhere (also what CI runs). ```sh +# End-to-end smoke test (fake --exec transport) — the default command docker run --rm agent-rules # or: yarn docker:smoke + +# Unit test suite (vitest) +docker run --rm agent-rules test + +# Both: unit tests + smoke +docker run --rm agent-rules check ``` ## Live review against a real agent @@ -65,6 +71,8 @@ docker run --rm \ ## Commands -The entrypoint accepts: `smoke` (default), `verify [args]`, `demo [args]`, -`cli [args]`, or any other command to exec directly. `verify`/`demo` forward -extra args to the CLI, e.g. `--transport codex` or `--output json`. +The entrypoint accepts: `smoke` (default), `test`, `check`, `verify [args]`, +`demo [args]`, `cli [args]`, or any other command to exec directly. `smoke`, +`test`, and `check` are hermetic (no credentials); `verify`/`demo` need a real +agent and forward extra args to the CLI, e.g. `--transport codex` or +`--output json`. diff --git a/docker/entrypoint.sh b/docker/entrypoint.sh index 29270a6..ecec1b3 100644 --- a/docker/entrypoint.sh +++ b/docker/entrypoint.sh @@ -3,6 +3,8 @@ # Container entrypoint. Dispatches to the project scripts / CLI. # # smoke hermetic end-to-end test (fake transport, no creds) [default] +# test unit test suite (vitest), no creds +# check unit tests + smoke (full hermetic test pass), no creds # verify [args] live review against a real agent (needs creds); # extra args pass through, e.g. `verify --transport codex` # demo [args] review the bundled examples/ sample against a real agent @@ -15,6 +17,13 @@ case "$cmd" in smoke) exec bash scripts/smoke.sh ;; + test) + exec yarn test + ;; + check) + yarn test + exec bash scripts/smoke.sh + ;; verify) shift exec bash scripts/verify-transport.sh "$@" diff --git a/docs/agent-rules-requirements.md b/docs/agent-rules-requirements.md index 0e93782..774c493 100644 --- a/docs/agent-rules-requirements.md +++ b/docs/agent-rules-requirements.md @@ -33,7 +33,15 @@ yarn add @casa/agent-rules ```typescript // Types -export type { AgentRule, Finding, ReviewResult, RunOptions, LLMAdapter }; +export type { + AgentRule, + Finding, + ReviewResult, + RunOptions, + LLMAdapter, + FilterResult, + FilterExecutor, +}; // Rule file utilities export { collectRuleFiles, parseRuleFile }; @@ -47,6 +55,9 @@ export { extractChangedFiles, extractDiffSections, buildDiffLineMap }; // Core filtering export { deduplicateFindings, filterFindingsToDiff, prioritizeFindings }; +// Filter-command execution (default applicability executor) +export { makeFilterExecutor }; + // High-level runner export { runReview }; @@ -68,7 +79,8 @@ agent-rules/ │ ├── diff.ts ← getDiff, extractChangedFiles, extractDiffSections, buildDiffLineMap │ ├── prompt.ts ← buildReviewPrompt │ ├── filter.ts ← deduplicateFindings, filterFindingsToDiff, prioritizeFindings -│ ├── runner.ts ← runReview +│ ├── filter-exec.ts ← makeFilterExecutor (default `filter`-command runner) +│ ├── runner.ts ← runReview, discoverApplicableRules │ └── cli.ts ← CLI entrypoint (bin) ├── package.json └── tsconfig.json @@ -180,6 +192,18 @@ interface RunOptions { * comparing against minSuggestionImpact. Default: 2. */ testFileImpactDiscount?: number; + + /** When false, rule `filter` commands are ignored (treated as absent). Default: true. */ + runFilters?: boolean; + + /** Per-filter subprocess timeout in ms. Default: 10000. */ + filterTimeoutMs?: number; + + /** Injectable filter executor (for tests / sandboxing). Default: built-in subprocess runner. */ + filterExecutor?: FilterExecutor; + + /** Working directory in which `filter` commands run. Default: process.cwd(). */ + cwd?: string; } ``` @@ -260,11 +284,12 @@ const UserSchema = z.object({ ### Front-matter fields -| Field | Type | Required | Default | Description | -| ------------- | ------------------ | -------- | ------------------------- | --------------------------------------------------------------------------------------------------------- | -| `description` | string | no | filename (sans extension) | Human-readable name used in findings output | -| `globs` | string or string[] | no | — | Glob patterns controlling which changed files trigger this rule. A rule with no `globs` is never applied. | -| `reviewSkip` | boolean | no | `false` | If `true`, the rule is parsed but excluded from review | +| Field | Type | Required | Default | Description | +| ------------- | ------------------ | -------- | ------------------------- | ---------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | +| `description` | string | no | filename (sans extension) | Human-readable name used in findings output | +| `globs` | string or string[] | no | — | Glob patterns controlling which changed files trigger this rule. A rule with no `globs` is never applied. | +| `reviewSkip` | boolean | no | `false` | If `true`, the rule is parsed but excluded from review | +| `filter` | string | no | — | A command run after a glob match to decide whether the rule applies. Matched paths are appended as arguments. See "Filter command". Empty or absent ⇒ no second-stage check. | ### Glob format @@ -342,11 +367,16 @@ output: applicableRules (ordered list of AgentRule) 2. For each collected file: a. Read contents. - b. Parse front-matter → { description, globs, reviewSkip }. + b. Parse front-matter → { description, globs, reviewSkip, filter }. c. If reviewSkip === true → discard, continue. d. If globs is empty → discard, continue. - e. For each path in changedFiles: - If matchGlobs(path, globs) → add rule to applicableRules, break. + e. matched = changedFiles where matchGlobs(path, globs). + If matched is empty → discard, continue. + f. If filter is set and filters are enabled, run it against `matched`: + pass → add rule to applicableRules. + reject → discard (recorded as "filtered"), continue. + error → add rule to applicableRules (fail-open), record a warning. + Otherwise add rule to applicableRules. 3. Return applicableRules. ``` @@ -359,8 +389,12 @@ interface AgentRule { content: string; // Markdown body after the closing --- globs: string[]; // parsed glob patterns reviewSkip?: boolean; + filter?: string; // optional second-stage applicability command } +type FilterResult = 'pass' | 'reject' | 'error'; +type FilterExecutor = (command: string, paths: string[]) => Promise; + async function collectRuleFiles(dir: string): Promise; function parseRuleFile(filename: string, raw: string): AgentRule | null; function matchGlobs(filePath: string, globs: string[]): boolean; @@ -426,6 +460,66 @@ function matchGlobs(filePath: string, globs: string[]): boolean { --- +## Filter command + +Globs decide applicability by file _path_. The optional `filter` front-matter +field adds a second stage that can depend on file _content_ or on relationships +between the changed files. It runs **only** for a rule that already passed the +glob stage (i.e. at least one changed file matched), **once per rule**, with the +glob-matched paths appended as command-line arguments. + +### Input contract + +- **Arguments:** the command string (tokenised with simple `'…'`/`"…"` quoting), + followed by the matched changed-file paths in their diff-relative form (no + leading `a/`/`b/`). Example: `filter: "rg -q TODO"` over matched files `a.ts` + and `b.ts` runs `rg -q TODO a.ts b.ts`. +- **stdin:** empty (closed immediately). +- **cwd:** the review working directory (`RunOptions.cwd`, default `process.cwd()`). +- **env:** inherits the parent environment plus `AGENT_RULES_SUBPROCESS=1`, so a + filter that re-invokes `agent-rules` trips the recursion guard. +- **stdout/stderr:** ignored; only the exit code is consulted. + +### Exit-code semantics (grep-style) + +| Outcome | `FilterResult` | Effect on the rule | +| ----------------------------------------------- | -------------- | ------------------------------------------------------------- | +| exit `0` | `pass` | Rule applies | +| exit `1` | `reject` | Rule skipped (recorded as `" (filtered)"`) | +| exit ≥ `2` | `error` | Fail-open: rule applies; recorded in `warnings` | +| spawn failure (command not found, not runnable) | `error` | Fail-open: rule applies; recorded in `warnings` | +| timeout (`filterTimeoutMs` exceeded) | `error` | Fail-open: rule applies; recorded in `warnings`; child killed | + +The fail-open behaviour is deliberate: a broken or missing filter must never +silently suppress a rule. A rule that errored is **applied** and surfaced in +`ReviewResult.warnings`, which is distinct from `skipped`. + +Filters run subject to the same `concurrency` cap as the rest of the review. The +executor is injectable via `RunOptions.filterExecutor` (the default spawns a +subprocess), which keeps discovery testable without real processes. + +### Caveat: working tree vs. diffed revision + +The filter receives _paths_ and runs against files on disk. For `--working-tree` +/ `--staged` this is the content under review. For `--diff ` the on-disk +files may differ from the diffed blobs; a filter that needs the exact reviewed +content should query git (e.g. `git grep `) rather than read the working +tree. + +### Trust model + +`filter` executes arbitrary commands with the invoking user's privileges — the +same trust level the rules directory already carries (rule bodies are agent +instructions; repos already run git hooks and npm scripts). The sharper risk is +CI reviewing an **untrusted** diff (e.g. a fork PR) that can also add or edit a +`filter`. Mitigation: pass `--no-filters` (`runFilters: false`) to disable all +filter execution when the rule set itself is part of the untrusted change, and +keep the rules directory under the same ownership controls (e.g. `CODEOWNERS`) as +the rest of the trusted codebase. `--list` executes filters by default so its +output is accurate; `--no-filters` opts out there too. + +--- + ## Diff scoping Before invoking the reviewing agent, the full diff is narrowed to only the sections relevant to the current rule. This reduces context size and prevents the agent from commenting on files it has no mandate to review. @@ -593,7 +687,8 @@ The caller receives a `ReviewResult` and is responsible for deciding how to pres interface ReviewResult { findings: Finding[]; // filtered, deduplicated, ready to surface ruleCount: number; // number of rules that were evaluated - skipped: string[]; // rule names skipped (no matching files, reviewSkip, etc.) + skipped: string[]; // rule names skipped (no matching files, reviewSkip, filtered, etc.) + warnings: string[]; // non-fatal notices, e.g. a filter that errored and was applied fail-open } ``` @@ -684,18 +779,20 @@ Exactly one diff source flag must be provided: ### Other flags -| Flag | Default | Description | -| ------------------------------ | -------------- | ------------------------------------------------------------------------- | -| `--rules ` | `.agent/rules` | Path to the rules directory | -| `--concurrency ` | `3` | Max rules evaluated in parallel | -| `--min-impact ` | `7` | Minimum impact score for suggestions to be included | -| `--ticket-context ` | — | Optional context string included in every rule prompt | -| `--ticket-context-file ` | — | Read ticket context from a file instead of inline | -| `--output ` | `text` | Output format: `text` or `json` | -| `--list` | — | Discover and print the rules applicable to the diff, then exit (no model) | -| `--exec ` | — | Override the model transport with an explicit command (see below) | -| `--transport ` | — | Pin which installed agent profile to use (bypasses context/PATH ordering) | -| `--model ` | tool default | Model passed to the resolved agent executable | +| Flag | Default | Description | +| ------------------------------ | -------------- | ---------------------------------------------------------------------------- | +| `--rules ` | `.agent/rules` | Path to the rules directory | +| `--concurrency ` | `3` | Max rules evaluated in parallel | +| `--min-impact ` | `7` | Minimum impact score for suggestions to be included | +| `--ticket-context ` | — | Optional context string included in every rule prompt | +| `--ticket-context-file ` | — | Read ticket context from a file instead of inline | +| `--output ` | `text` | Output format: `text` or `json` | +| `--list` | — | Discover and print the rules applicable to the diff, then exit (no model) | +| `--no-filters` | filters on | Ignore all rule `filter` commands (treat as absent); use for untrusted diffs | +| `--filter-timeout ` | `10000` | Per-filter subprocess timeout | +| `--exec ` | — | Override the model transport with an explicit command (see below) | +| `--transport ` | — | Pin which installed agent profile to use (bypasses context/PATH ordering) | +| `--model ` | tool default | Model passed to the resolved agent executable | ### Model transport (CLI) @@ -869,3 +966,10 @@ const result = await runReview({ diff, rulesDir: '.agent/rules', llm: myAdapter | R29 | The CLI must ship built-in invocation profiles for recognised tools (`claude`, `codex`) that force a clean, tool-free completion; `--exec` must bypass profiles and run a raw stdin→stdout command. | | R30 | `runReview` must own no timeout/retry policy (resilience is the `LLMAdapter`'s responsibility) but must isolate per-rule failures: a rejected `run` drops that rule into `ReviewResult.skipped` without aborting the review. | | R31 | The CLI must provide a `--list` mode that discovers and prints the rules applicable to the diff without invoking a transport, so editor/agent integrations (slash commands) can fetch rules without spawning a nested agent. | +| R32 | A rule may declare an optional `filter` front-matter field: a single command string. Absent or empty (after stripping quotes and whitespace) ⇒ no second-stage check. | +| R33 | The filter runs only after a rule's globs match at least one changed file, once per rule, with the matched paths appended as command arguments and stdin empty. | +| R34 | Filter results follow grep-style exit codes: `0` ⇒ applies; `1` ⇒ skipped (recorded in `skipped`); any other exit, spawn failure, or timeout ⇒ fail-open (applies). | +| R35 | Filter errors must never abort the review and must be surfaced as non-fatal `warnings`, distinct from `skipped`. | +| R36 | Filter execution must be disableable via `runFilters: false` / `--no-filters`, and bounded by a configurable timeout (`filterTimeoutMs` / `--filter-timeout`, default 10000 ms). | +| R37 | The filter executor must be injectable (`RunOptions.filterExecutor`) so discovery is testable without real subprocesses; the default spawns with the review `cwd` and the `AGENT_RULES_SUBPROCESS` marker set. | +| R38 | `--list` must reflect the post-filter applicable set and honour `--no-filters`; the filter feature must be documented as executing repo-defined commands (trust model). | diff --git a/examples/filters/touches-public-api.sh b/examples/filters/touches-public-api.sh new file mode 100755 index 0000000..cdd2e4d --- /dev/null +++ b/examples/filters/touches-public-api.sh @@ -0,0 +1,31 @@ +#!/usr/bin/env bash +# +# Example agent-rules `filter` command. +# +# A rule's `filter` runs after its globs match, receiving the matched file paths +# as arguments. It decides whether the rule applies via its exit code: +# +# exit 0 -> rule applies +# exit 1 -> rule is skipped (clean rejection) +# exit >1 -> error; agent-rules treats it as "no filter" and applies the rule +# (fail-open). Spawn failures and timeouts are treated the same way. +# +# This filter applies a rule only when one of the changed files looks like part +# of the public API surface — a barrel/index module or a file under a `public/` +# directory, or a file that re-exports with `export * from`. +# +# Usage in a rule's front-matter: +# filter: '.agent/filters/touches-public-api.sh' + +set -euo pipefail + +for f in "$@"; do + case "$f" in + */index.ts | */public/*) exit 0 ;; + esac + if grep -Eq '^export \* from' "$f" 2>/dev/null; then + exit 0 + fi +done + +exit 1 diff --git a/examples/rules/no-raw-sql.md b/examples/rules/no-raw-sql.md new file mode 100644 index 0000000..a9a480f --- /dev/null +++ b/examples/rules/no-raw-sql.md @@ -0,0 +1,29 @@ +--- +description: No raw SQL in repositories +globs: + - 'src/repositories/**/*.ts' +filter: "grep -ilq 'select \\|insert \\|update \\|delete '" +--- + +# No raw SQL in repositories + +Repository classes must build queries through the query builder, not by +embedding raw SQL strings. Raw SQL bypasses parameter binding and the schema +types, and is the usual source of injection bugs. + +The `filter` above is a second-stage applicability check: after the globs select +the changed repository files, it runs `grep` over them and the rule only applies +when at least one matched file actually contains a SQL keyword. If none do, the +rule is skipped without calling the model — globs match the _files_, the filter +matches the _content_. + +## What to flag + +- A template literal or string concatenation containing `SELECT`/`INSERT`/ + `UPDATE`/`DELETE` passed to a `query`/`raw`/`execute` call. +- User-derived values interpolated into a SQL string. + +## What NOT to flag + +- Query-builder calls (`.select(...)`, `.where(...)`, etc.). +- SQL inside migration files or fixtures (those live outside `src/repositories/`). diff --git a/scripts/smoke.sh b/scripts/smoke.sh index 95484bd..864df39 100755 --- a/scripts/smoke.sh +++ b/scripts/smoke.sh @@ -80,6 +80,18 @@ listcode=$(cd "$REPO" && env -u CLAUDE_CODE_EXECPATH PATH="$BIN" "$(command -v n check "--list exit code is 0" "0" "$listcode" check "--list returns the rule" "1" "$(node -e 'let s="";process.stdin.on("data",d=>s+=d).on("end",()=>console.log(JSON.parse(s).rules.length))' <<<"$out")" +echo "6. filter commands gate applicability (and --no-filters disables them)" +RULES2="$WORK/rules-filter" +mkdir -p "$RULES2" +# Two rules that both match the change; one filter passes (exit 0), one rejects (exit 1). +printf -- '---\ndescription: Pass\nglobs: "*.ts"\nfilter: "true"\n---\nx\n' >"$RULES2/pass.md" +printf -- '---\ndescription: Reject\nglobs: "*.ts"\nfilter: "false"\n---\nx\n' >"$RULES2/reject.md" +count() { node -e 'let s="";process.stdin.on("data",d=>s+=d).on("end",()=>console.log(JSON.parse(s).rules.length))'; } +out="$(cd "$REPO" && node "$CLI" --working-tree --rules "$RULES2" --list --output json)" +check "filter on: only the passing rule applies" "1" "$(count <<<"$out")" +out="$(cd "$REPO" && node "$CLI" --working-tree --rules "$RULES2" --list --no-filters --output json)" +check "--no-filters: both rules apply" "2" "$(count <<<"$out")" + echo echo "smoke: $PASS passed, $FAIL failed" [[ "$FAIL" -eq 0 ]] diff --git a/src/cli.ts b/src/cli.ts index 08fb23d..df5494f 100644 --- a/src/cli.ts +++ b/src/cli.ts @@ -27,6 +27,9 @@ Options: --output Output format (default: text) --list List the rules that apply to the diff and exit (no model call; for editor/agent integrations) + --no-filters Ignore rule \`filter\` commands (treat as absent). + Use when reviewing untrusted changes. + --filter-timeout Per-filter subprocess timeout (default: 10000) --exec Override transport (any stdin->stdout command) --transport Pin which installed agent CLI to use --model Model passed to the resolved agent CLI @@ -48,6 +51,8 @@ async function main(): Promise { 'ticket-context-file': { type: 'string' }, output: { type: 'string', default: 'text' }, list: { type: 'boolean' }, + 'no-filters': { type: 'boolean' }, + 'filter-timeout': { type: 'string' }, exec: { type: 'string' }, transport: { type: 'string' }, model: { type: 'string' }, @@ -86,14 +91,23 @@ async function main(): Promise { return 0; } + const runFilters = values['no-filters'] ? false : undefined; + const filterTimeoutMs = parseIntOption(values['filter-timeout'], 'filter-timeout'); + // --list: discover applicable rules and exit. No model call, so this is safe to - // run from inside an agent session (editor/slash-command integrations). + // run from inside an agent session (editor/slash-command integrations). Note + // that filter commands DO run here unless --no-filters is given. if (values.list) { const changed = extractChangedFiles(diff); - const { rules } = await discoverApplicableRules(values.rules ?? '.agent/rules', changed); + const { rules, warnings } = await discoverApplicableRules( + values.rules ?? '.agent/rules', + changed, + { runFilters, filterTimeoutMs }, + ); + for (const w of warnings) process.stderr.write(`warning: ${w}\n`); if (values.output === 'json') { const payload = rules.map((r) => ({ name: r.name, globs: r.globs, content: r.content })); - process.stdout.write(JSON.stringify({ rules: payload }, null, 2) + '\n'); + process.stdout.write(JSON.stringify({ rules: payload, warnings }, null, 2) + '\n'); } else { process.stdout.write(formatRuleList(rules)); } @@ -117,8 +131,11 @@ async function main(): Promise { llm: adapter, concurrency: parseIntOption(values.concurrency, 'concurrency'), minSuggestionImpact: parseIntOption(values['min-impact'], 'min-impact'), + runFilters, + filterTimeoutMs, }); + for (const w of result.warnings) process.stderr.write(`warning: ${w}\n`); if (values.output === 'json') { process.stdout.write(JSON.stringify(result, null, 2) + '\n'); } else { diff --git a/src/filter-exec.ts b/src/filter-exec.ts new file mode 100644 index 0000000..066802d --- /dev/null +++ b/src/filter-exec.ts @@ -0,0 +1,81 @@ +import { spawn } from 'node:child_process'; + +import type { FilterExecutor, FilterResult } from './types.js'; + +const DEFAULT_FILTER_TIMEOUT_MS = 10_000; + +export interface FilterExecOptions { + /** Per-filter subprocess timeout in ms. Default: 10000. */ + timeoutMs?: number; + /** Working directory the command runs in. Default: `process.cwd()`. */ + cwd?: string; + /** Environment for the child (defaults to `process.env`). */ + env?: NodeJS.ProcessEnv; +} + +/** + * Build the default {@link FilterExecutor}: it tokenises the `filter` command, + * appends the matched paths as arguments, and spawns it. The decision is taken + * entirely from the exit code (grep-style): + * + * - `0` ⇒ `'pass'` (rule applies) + * - `1` ⇒ `'reject'` (rule skipped) + * - anything else, ⇒ `'error'` (fail-open — caller applies the rule) + * timeout, or a + * spawn failure + * + * stdout/stderr are ignored and stdin is closed; only the exit status matters. + */ +export function makeFilterExecutor(options: FilterExecOptions = {}): FilterExecutor { + const timeoutMs = options.timeoutMs ?? DEFAULT_FILTER_TIMEOUT_MS; + const cwd = options.cwd ?? process.cwd(); + const baseEnv = options.env ?? process.env; + + return (command, paths) => + new Promise((resolve) => { + const [cmd, ...cmdArgs] = tokenize(command); + if (!cmd) { + resolve('error'); + return; + } + + // Mark the child so a filter that re-invokes agent-rules is caught by the + // recursion guard in cli.ts. + const child = spawn(cmd, [...cmdArgs, ...paths], { + stdio: ['ignore', 'ignore', 'ignore'], + cwd, + env: { ...baseEnv, AGENT_RULES_SUBPROCESS: '1' }, + }); + + let settled = false; + const finish = (result: FilterResult): void => { + if (settled) return; + settled = true; + clearTimeout(timer); + resolve(result); + }; + + const timer = setTimeout(() => { + child.kill('SIGKILL'); + finish('error'); + }, timeoutMs); + + child.on('error', () => finish('error')); + child.on('close', (code) => { + if (code === 0) finish('pass'); + else if (code === 1) finish('reject'); + else finish('error'); + }); + }); +} + +/** Minimal shell-like tokenizer (handles simple single/double quotes). */ +function tokenize(input: string): string[] { + const tokens: string[] = []; + const re = /"([^"]*)"|'([^']*)'|(\S+)/g; + let m: RegExpExecArray | null; + while ((m = re.exec(input)) !== null) { + tokens.push(m[1] ?? m[2] ?? m[3] ?? ''); + } + return tokens; +} diff --git a/src/index.ts b/src/index.ts index 612c2df..909c3ca 100644 --- a/src/index.ts +++ b/src/index.ts @@ -6,6 +6,8 @@ export type { RunOptions, LLMAdapter, DiffSource, + FilterResult, + FilterExecutor, } from './types.js'; export { collectRuleFiles, parseRuleFile, loadRules } from './rule.js'; @@ -19,5 +21,7 @@ export { isTestFile, } from './filter.js'; export { FindingSchema, extractJsonArray, parseFindings } from './parse.js'; +export { makeFilterExecutor } from './filter-exec.js'; +export type { FilterExecOptions } from './filter-exec.js'; export { runReview, discoverApplicableRules } from './runner.js'; -export type { DiscoveryResult } from './runner.js'; +export type { DiscoveryResult, DiscoverOptions } from './runner.js'; diff --git a/src/rule.ts b/src/rule.ts index 664d7a0..942bb33 100644 --- a/src/rule.ts +++ b/src/rule.ts @@ -40,6 +40,7 @@ export function parseRuleFile(filename: string, raw: string): AgentRule { const globs: string[] = []; let description = ''; let reviewSkip = false; + let filter = ''; let inGlobsList = false; for (const line of lines.slice(1, closing)) { @@ -78,6 +79,12 @@ export function parseRuleFile(filename: string, raw: string): AgentRule { const skip = /^reviewskip:\s*(.+)$/i.exec(trimmed); if (skip) { reviewSkip = skip[1]!.trim().toLowerCase() === 'true'; + continue; + } + + const filterMatch = /^filter:\s*(.+)$/i.exec(trimmed); + if (filterMatch) { + filter = stripQuotes(filterMatch[1]!.trim()).trim(); } } @@ -86,7 +93,9 @@ export function parseRuleFile(filename: string, raw: string): AgentRule { .join('\n') .trim(); - return { name: description || fallbackName, content, globs, reviewSkip }; + const rule: AgentRule = { name: description || fallbackName, content, globs, reviewSkip }; + if (filter) rule.filter = filter; + return rule; } /** Read and parse every rule file under `dir`. */ diff --git a/src/runner.ts b/src/runner.ts index 2daf28a..f65c5a4 100644 --- a/src/runner.ts +++ b/src/runner.ts @@ -1,31 +1,59 @@ import { buildDiffLineMap, extractChangedFiles, extractDiffSections } from './diff.js'; +import { makeFilterExecutor } from './filter-exec.js'; import { deduplicateFindings, filterFindingsToDiff, prioritizeFindings } from './filter.js'; import { matchGlobs } from './glob.js'; import { parseFindings } from './parse.js'; import { buildReviewPrompt } from './prompt.js'; import { loadRules } from './rule.js'; -import type { AgentRule, Finding, ReviewResult, RunOptions } from './types.js'; +import type { AgentRule, Finding, FilterExecutor, ReviewResult, RunOptions } from './types.js'; const DEFAULT_CONCURRENCY = 3; export interface DiscoveryResult { rules: AgentRule[]; skipped: string[]; + warnings: string[]; +} + +/** Filter-stage options for {@link discoverApplicableRules}. */ +export interface DiscoverOptions { + /** When `false`, `filter` commands are ignored (treated as absent). Default: `true`. */ + runFilters?: boolean; + /** Per-filter subprocess timeout in ms. Default: 10000. */ + filterTimeoutMs?: number; + /** Injectable filter executor. Defaults to the built-in subprocess runner. */ + filterExecutor?: FilterExecutor; + /** Working directory in which `filter` commands run. Default: `process.cwd()`. */ + cwd?: string; + /** Max filter commands to run concurrently. Default: 3. */ + concurrency?: number; } /** - * Discover the rules under `rulesDir` that apply to `changedFiles`. Rules are - * dropped (and recorded in `skipped`) when they set `reviewSkip`, declare no - * globs, or match none of the changed files. + * Discover the rules under `rulesDir` that apply to `changedFiles`. + * + * Rules are dropped (and recorded in `skipped`) when they set `reviewSkip`, + * declare no globs, or match none of the changed files. A rule that survives the + * glob stage and declares a `filter` command then runs that command against its + * matched paths: `reject` skips the rule (`" (filtered)"`), `error` is + * fail-open (the rule applies, with a note in `warnings`), and `pass` (or no + * filter) applies it. */ export async function discoverApplicableRules( rulesDir: string, changedFiles: string[], + options: DiscoverOptions = {}, ): Promise { const all = await loadRules(rulesDir); const rules: AgentRule[] = []; const skipped: string[] = []; + const warnings: string[] = []; + + const runFilters = options.runFilters ?? true; + const concurrency = options.concurrency ?? DEFAULT_CONCURRENCY; + // Glob stage: collect the candidates that survive, with their matched paths. + const candidates: { rule: AgentRule; matched: string[] }[] = []; for (const rule of all) { if (rule.reviewSkip) { skipped.push(`${rule.name} (reviewSkip)`); @@ -35,14 +63,44 @@ export async function discoverApplicableRules( skipped.push(`${rule.name} (no globs)`); continue; } - if (!changedFiles.some((f) => matchGlobs(f, rule.globs))) { + const matched = changedFiles.filter((f) => matchGlobs(f, rule.globs)); + if (matched.length === 0) { skipped.push(`${rule.name} (no matching files)`); continue; } + candidates.push({ rule, matched }); + } + + // Filter stage: only candidates with a `filter` (and filters enabled) spawn a + // command. Run them concurrently; everything else passes straight through. + const executor = + options.filterExecutor ?? + makeFilterExecutor({ timeoutMs: options.filterTimeoutMs, cwd: options.cwd }); + const decisions = new Map(); + const needFilter = runFilters ? candidates.filter((c) => c.rule.filter) : []; + await mapPool(needFilter, concurrency, async ({ rule, matched }) => { + let result; + try { + result = await executor(rule.filter!, matched); + } catch { + result = 'error' as const; + } + if (result === 'reject' || result === 'error') decisions.set(rule, result); + }); + + for (const { rule } of candidates) { + const decision = decisions.get(rule); + if (decision === 'reject') { + skipped.push(`${rule.name} (filtered)`); + continue; + } + if (decision === 'error') { + warnings.push(`${rule.name} (filter error; applied anyway)`); + } rules.push(rule); } - return { rules, skipped }; + return { rules, skipped, warnings }; } /** @@ -62,10 +120,20 @@ export async function runReview(options: RunOptions): Promise { concurrency = DEFAULT_CONCURRENCY, minSuggestionImpact, testFileImpactDiscount, + runFilters, + filterTimeoutMs, + filterExecutor, + cwd, } = options; const changedFiles = extractChangedFiles(diff); - const { rules, skipped } = await discoverApplicableRules(rulesDir, changedFiles); + const { rules, skipped, warnings } = await discoverApplicableRules(rulesDir, changedFiles, { + runFilters, + filterTimeoutMs, + filterExecutor, + cwd, + concurrency, + }); const validLines = buildDiffLineMap(diff); // Pair each rule with its scoped diff; drop rules with no relevant sections. @@ -96,7 +164,7 @@ export async function runReview(options: RunOptions): Promise { testFileImpactDiscount, }); - return { findings, ruleCount: queue.length, skipped }; + return { findings, ruleCount: queue.length, skipped, warnings }; } /** Run `fn` over `items` with at most `limit` concurrent executions. */ diff --git a/src/types.ts b/src/types.ts index e584ef1..173ba56 100644 --- a/src/types.ts +++ b/src/types.ts @@ -8,8 +8,25 @@ export interface AgentRule { globs: string[]; /** When `true`, the rule is parsed but excluded from review. */ reviewSkip?: boolean; + /** + * Optional second-stage applicability command, run after a glob match. The + * glob-matched changed file paths are appended as arguments. Exit `0` ⇒ the + * rule applies, `1` ⇒ it is skipped, any other outcome (incl. spawn failure + * or timeout) ⇒ fail-open (applies). Absent ⇒ no second-stage check. + */ + filter?: string; } +/** Outcome of running a rule's {@link AgentRule.filter} command. */ +export type FilterResult = 'pass' | 'reject' | 'error'; + +/** + * Runs a rule's `filter` command against the matched paths and reports whether + * the rule applies. The default implementation spawns a subprocess; callers may + * inject their own (e.g. for tests or sandboxing). + */ +export type FilterExecutor = (command: string, paths: string[]) => Promise; + export type Severity = 'blocking' | 'suggestion' | 'nitpick' | 'ignored'; /** A single reviewer finding, anchored to a line in the diff. */ @@ -35,6 +52,12 @@ export interface ReviewResult { ruleCount: number; /** Rule names skipped, with a reason, e.g. "no-secrets (reviewSkip)". */ skipped: string[]; + /** + * Non-fatal notices, e.g. a `filter` command that errored and was treated as + * fail-open (" (filter error; applied anyway)"). A rule listed here was + * still applied — unlike {@link ReviewResult.skipped}. + */ + warnings: string[]; } /** @@ -71,6 +94,21 @@ export interface RunOptions { * against {@link RunOptions.minSuggestionImpact}. Default: 2. */ testFileImpactDiscount?: number; + /** + * When `false`, rule `filter` commands are ignored (treated as absent). + * Default: `true`. + */ + runFilters?: boolean; + /** Per-filter subprocess timeout in ms. Default: 10000. */ + filterTimeoutMs?: number; + /** + * Injectable filter executor (for tests or custom sandboxing). Defaults to + * the built-in subprocess runner. Receives the command string and the + * glob-matched paths. + */ + filterExecutor?: FilterExecutor; + /** Working directory in which `filter` commands run. Default: `process.cwd()`. */ + cwd?: string; } /** Where {@link getDiff} should source the diff from. */ diff --git a/test/filter-exec.test.ts b/test/filter-exec.test.ts new file mode 100644 index 0000000..24b89b7 --- /dev/null +++ b/test/filter-exec.test.ts @@ -0,0 +1,55 @@ +import { mkdtemp, rm, writeFile } from 'node:fs/promises'; +import { tmpdir } from 'node:os'; +import path from 'node:path'; + +import { afterAll, beforeAll, describe, expect, it } from 'vitest'; + +import { makeFilterExecutor } from '../src/filter-exec.js'; + +describe('makeFilterExecutor', () => { + const run = makeFilterExecutor({ timeoutMs: 2000 }); + + it('maps exit 0 to pass', async () => { + expect(await run('sh -c "exit 0"', [])).toBe('pass'); + }); + + it('maps exit 1 to reject', async () => { + expect(await run('sh -c "exit 1"', [])).toBe('reject'); + }); + + it('maps any other exit code to error (fail-open)', async () => { + expect(await run('sh -c "exit 2"', [])).toBe('error'); + }); + + it('treats a missing command as error', async () => { + expect(await run('this-command-does-not-exist-xyz', [])).toBe('error'); + }); + + it('treats a timeout as error', async () => { + const slow = makeFilterExecutor({ timeoutMs: 50 }); + expect(await slow('sh -c "sleep 5"', [])).toBe('error'); + }); + + describe('with matched paths passed as arguments', () => { + let dir: string; + let file: string; + + beforeAll(async () => { + dir = await mkdtemp(path.join(tmpdir(), 'agent-rules-fexec-')); + file = path.join(dir, 'src.ts'); + await writeFile(file, 'const x = 1; // needle\n', 'utf8'); + }); + + afterAll(async () => { + await rm(dir, { recursive: true, force: true }); + }); + + it('passes when grep finds the pattern in a supplied file', async () => { + expect(await run('grep -q needle', [file])).toBe('pass'); + }); + + it('rejects when grep does not find the pattern', async () => { + expect(await run('grep -q haystack', [file])).toBe('reject'); + }); + }); +}); diff --git a/test/rule.test.ts b/test/rule.test.ts index 7d6005a..15386f8 100644 --- a/test/rule.test.ts +++ b/test/rule.test.ts @@ -46,4 +46,20 @@ describe('parseRuleFile', () => { expect(rule.name).toBe('plain'); expect(rule.content).toBe('# Just markdown'); }); + + it('parses a quoted filter command', () => { + const raw = ['---', 'globs: "*.ts"', 'filter: "grep -ilq TODO"', '---', 'body'].join('\n'); + expect(parseRuleFile('x.md', raw).filter).toBe('grep -ilq TODO'); + }); + + it('leaves filter unset when absent or empty', () => { + const noFilter = parseRuleFile('a.md', ['---', 'globs: "*.ts"', '---', 'body'].join('\n')); + expect(noFilter.filter).toBeUndefined(); + + const emptyFilter = parseRuleFile( + 'b.md', + ['---', 'globs: "*.ts"', 'filter: " "', '---', 'body'].join('\n'), + ); + expect(emptyFilter.filter).toBeUndefined(); + }); }); diff --git a/test/runner.test.ts b/test/runner.test.ts index ec1b241..1132df9 100644 --- a/test/runner.test.ts +++ b/test/runner.test.ts @@ -4,8 +4,8 @@ import path from 'node:path'; import { afterAll, beforeAll, describe, expect, it } from 'vitest'; -import { runReview } from '../src/runner.js'; -import type { LLMAdapter } from '../src/types.js'; +import { discoverApplicableRules, runReview } from '../src/runner.js'; +import type { FilterExecutor, LLMAdapter } from '../src/types.js'; const DIFF = [ 'diff --git a/src/foo.ts b/src/foo.ts', @@ -102,3 +102,79 @@ describe('runReview', () => { expect(result.findings).toHaveLength(0); }); }); + +describe('discoverApplicableRules (filter stage)', () => { + let dir: string; + const changed = ['src/foo.ts']; + + beforeAll(async () => { + dir = await mkdtemp(path.join(tmpdir(), 'agent-rules-filter-')); + // A rule that matches the change AND declares a filter command. + await writeFile( + path.join(dir, 'filtered.md'), + '---\ndescription: Rule F\nglobs: "src/**/*.ts"\nfilter: "check"\n---\nCheck F.', + 'utf8', + ); + // A plain rule (matches, no filter) — proves the executor is only called for + // rules that actually declare a filter. + await writeFile( + path.join(dir, 'plain.md'), + '---\ndescription: Rule P\nglobs: "src/**/*.ts"\n---\nCheck P.', + 'utf8', + ); + }); + + afterAll(async () => { + await rm(dir, { recursive: true, force: true }); + }); + + it('applies a rule when its filter passes, calling the executor with matched paths only', async () => { + const calls: { command: string; paths: string[] }[] = []; + const filterExecutor: FilterExecutor = (command, paths) => { + calls.push({ command, paths }); + return Promise.resolve('pass'); + }; + + const { rules, warnings } = await discoverApplicableRules(dir, changed, { filterExecutor }); + + expect(rules.map((r) => r.name).sort()).toEqual(['Rule F', 'Rule P']); + expect(calls).toEqual([{ command: 'check', paths: ['src/foo.ts'] }]); // only the filtered rule + expect(warnings).toEqual([]); + }); + + it('skips a rule when its filter rejects (exit 1)', async () => { + const { rules, skipped } = await discoverApplicableRules(dir, changed, { + filterExecutor: () => Promise.resolve('reject'), + }); + expect(rules.map((r) => r.name)).not.toContain('Rule F'); + expect(skipped).toEqual(expect.arrayContaining(['Rule F (filtered)'])); + }); + + it('fails open and warns when the filter errors', async () => { + const { rules, warnings } = await discoverApplicableRules(dir, changed, { + filterExecutor: () => Promise.resolve('error'), + }); + expect(rules.map((r) => r.name)).toContain('Rule F'); + expect(warnings).toEqual(expect.arrayContaining(['Rule F (filter error; applied anyway)'])); + }); + + it('fails open when the executor throws', async () => { + const { rules } = await discoverApplicableRules(dir, changed, { + filterExecutor: () => Promise.reject(new Error('boom')), + }); + expect(rules.map((r) => r.name)).toContain('Rule F'); + }); + + it('ignores filters entirely when runFilters is false', async () => { + let called = false; + const { rules } = await discoverApplicableRules(dir, changed, { + runFilters: false, + filterExecutor: () => { + called = true; + return Promise.resolve('reject'); + }, + }); + expect(called).toBe(false); + expect(rules.map((r) => r.name)).toContain('Rule F'); + }); +});