feat(rules): external rule-pack loader (--rule-pack) - #107
Conversation
Lets scans run community/private detection rules alongside the built-ins without
recompiling. Turns AgentShield into a platform: anyone with a JSON pack in this
shape can plug in (e.g. the MIT Agent Threat Rules pack, 464 rules).
- src/rules/external.ts: loadRulePack/loadRulePacks. Zod-validated pack
({version,name?,rules:[{id,name,description?,severity,category,patterns,
flags?,fileTypes?}]}), mirroring the --policy loader. Fails closed on missing
file, invalid JSON, schema violation, duplicate id, or uncompilable regex.
Compiles patterns to RegExp (forces global flag), maps each entry to a Rule
whose check() runs them via matchAll; optional fileTypes scoping; findings
capped per rule per file.
- src/scanner/index.ts: scan(targetPath, { extraRules }) appends pack rules to
the builtin set through the existing generic runRules loop.
- CLI: scan --rule-pack <path> (repeatable). Loads before the scan, prints a
per-pack 'Loaded N external rules' note to stderr, exits 1 on load error.
- README: External Rule Packs section + CLI Reference entry.
20 new tests (loader valid/invalid/scoping/dup/regex/global-flag + multi-pack +
scanner integration). Full suite 1875 green, tsc + eslint clean, CLI smoke
verified (loads pack, detects, JSON output stays clean).
Per the issue's design note: external findings count toward the total grade but
the 5 fixed ScoreBreakdown buckets are unchanged (acceptable for v1).
|
ECC bundle files are already tracked in this repository. Skipping generation of another bundle PR. |
|
Note
|
| Layer / File(s) | Summary |
|---|---|
Rule pack schema, types, and loader src/rules/external.ts |
Defines RulePackSchema (Zod), inferred types (RulePackEntry, RulePack), and LoadRulePackResult. Implements pattern compilation with global-flag normalization, per-rule check() with a findings cap, and loadRulePack/loadRulePacks with fail-closed validation (missing file, bad JSON, schema mismatch, duplicate IDs, invalid regex). |
ScanOptions and extraRules integration src/scanner/index.ts |
Exports ScanOptions { extraRules? }, updates scan(targetPath, options = {}) to merge getBuiltinRules() with options.extraRules, and passes target.path as scanRoot to runRules. |
CLI --rule-pack option and scan invocation src/index.ts |
Adds loadRulePacks import, a repeatable --rule-pack <path> CLI option, fatal error handling on load failure, per-pack rule-count logging, and calls scan(targetPath, { extraRules }). |
Tests and documentation tests/rules/external.test.ts, tests/scanner/scanner.test.ts, README.md |
Vitest suites cover loadRulePack (success, fileTypes scoping, global match, all failure modes) and loadRulePacks (multi-pack concatenation, first-error fail-closed); integration test asserts exact finding-count delta with extraRules; README documents the feature with JSON schema and CLI reference. |
Estimated code review effort
🎯 3 (Moderate) | ⏱️ ~25 minutes
🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
| Check name | Status | Explanation |
|---|---|---|
| Description Check | ✅ Passed | Check skipped - CodeRabbit’s high-level summary is enabled. |
| Title check | ✅ Passed | The title accurately summarizes the main feature: external rule-pack loader with CLI support via --rule-pack flag. |
| Linked Issues check | ✅ Passed | PR implementation fully addresses all coding objectives from issue #101: external rule loader [src/rules/external.ts], scanner extension [src/scanner/index.ts], CLI flag [--rule-pack], zod validation, error handling, and comprehensive testing. |
| Out of Scope Changes check | ✅ Passed | All changes are directly aligned with the linked issue scope: rule loader, scanner extension, CLI integration, documentation, and tests supporting external rule-pack loading. |
| Docstring Coverage | ✅ Passed | No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check. |
✏️ Tip: You can configure your own custom pre-merge checks in the settings.
✨ Finishing Touches
📝 Generate docstrings
- Create stacked PR
- Commit on current branch
🧪 Generate unit tests (beta)
- Create PR with unit tests
- Commit unit tests in branch
feat/external-rule-pack-loader
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.
Comment @coderabbitai help to get the list of available commands.
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| const findings: Finding[] = []; | ||
| let seq = 0; | ||
| for (const pattern of compiled) { | ||
| for (const match of findAllMatches(file.content, pattern)) { | ||
| findings.push({ | ||
| id: `external-${entry.id}-${seq}`, | ||
| severity: entry.severity as Severity, | ||
| category: entry.category as FindingCategory, | ||
| title: entry.name, | ||
| description: `${entry.description ?? entry.name} (external rule ${entry.id}).`, | ||
| file: file.path, | ||
| line: findLineNumber(file.content, match.index ?? 0), | ||
| evidence: match[0].substring(0, 100), | ||
| }); | ||
| seq += 1; | ||
| if (findings.length >= MAX_FINDINGS_PER_RULE_PER_FILE) return findings; |
There was a problem hiding this comment.
seq resets to 0 at the start of every check() call, so every file that matches the same rule produces findings with the same IDs (external-foo-0, external-foo-1, …). When a rule fires on two different files, both scans emit external-foo-0 — a collision that breaks baseline comparison and remediation-plan fingerprinting, since those features rely on stable, unique IDs to correlate findings across runs. The fix is to incorporate the file path into the ID (e.g. external-${entry.id}-${file.path}-${seq} or a hash thereof) so the ID is globally unique within a single scan.
| export function loadRulePacks(paths: ReadonlyArray<string>): { | ||
| readonly success: boolean; | ||
| readonly rules: ReadonlyArray<Rule>; | ||
| readonly packs: ReadonlyArray<{ name: string; ruleCount: number }>; | ||
| readonly error?: string; | ||
| } { | ||
| const rules: Rule[] = []; | ||
| const packs: Array<{ name: string; ruleCount: number }> = []; | ||
| for (const path of paths) { | ||
| const result = loadRulePack(path); | ||
| if (!result.success || !result.rules || !result.meta) { | ||
| return { success: false, rules: [], packs: [], error: result.error }; | ||
| } | ||
| rules.push(...result.rules); | ||
| packs.push(result.meta); | ||
| } | ||
| return { success: true, rules, packs }; |
There was a problem hiding this comment.
loadRulePack correctly rejects duplicate rule IDs within a single pack, but loadRulePacks never checks whether the same rule ID appears in multiple packs. If pack-a.json and pack-b.json both declare id: "tool-001", the loader silently produces two rules with Rule.id = "external-tool-001" and finding IDs that are entirely indistinguishable (external-tool-001-0, etc.). The PR description says the loader "fails closed on … duplicate id", but that guarantee only holds within a pack. A global seenRuleIds set checked after merging each pack's rules would close this gap.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@README.md`:
- Line 391: The README.md documentation at line 391 describes how patterns
execute as JS regexes but omits that the loader enforces global matching on
external regexes. Add documentation clarifying that external regex patterns are
normalized to global matching by the loader, and explain how this normalization
affects match cardinality and the number of expected findings, ensuring users
understand the behavior of their patterns when using external regex
configurations.
In `@src/index.ts`:
- Line 323: The variable rulePackPaths is currently typed as a mutable string[]
but should be typed as ReadonlyArray<string> to comply with coding guidelines
requiring immutable array typing in src/ files. Change the type annotation of
rulePackPaths from string[] to ReadonlyArray<string> while keeping the
initialization logic the same.
In `@src/rules/external.ts`:
- Around line 167-182: The loadRulePacks function currently only ensures rule ID
uniqueness within individual packs but not across all loaded packs. Two
different packs can define rules with identical IDs, resulting in identical
runtime rule.id values and overlapping finding ID namespaces. After the loop
that collects all rules from all packs, add a validation step to check for
duplicate rule IDs across the entire rules array. If duplicates are found,
return an error response indicating which rule IDs are duplicated rather than
returning success with the overlapping rules.
- Around line 70-72: The findAllMatches function returns a mutable Array type
instead of an immutable ReadonlyArray type. Change the return type of
findAllMatches from Array<RegExpMatchArray> to ReadonlyArray<RegExpMatchArray>.
Additionally, review the code at lines 97-99, 139-140, and 173-174 and apply the
same immutability pattern to any other mutable array type annotations (such as
Finding[] and Rule[]) and remove any array mutations like .push() calls,
replacing them with immutable alternatives like spread operators or concat.
In `@src/scanner/index.ts`:
- Line 35: The rules variable on line 35 that merges the builtin rules with
extra rules is being inferred as a mutable array type instead of a
ReadonlyArray. Add an explicit type annotation to the rules variable declaration
to type it as ReadonlyArray<Rule>. This ensures the merged rules list is
immutable at the type level, aligning with the coding guideline that all arrays
in src/ files must be typed as ReadonlyArray.
In `@tests/scanner/scanner.test.ts`:
- Around line 46-54: Replace the inline finding object (with id
"external-backdoor-0") in the test fixture with a call to the makeFinding()
helper factory function. Pass the necessary properties (id, severity, category,
title, description, file) as arguments to makeFinding() instead of creating a
literal object. This ensures test data is created consistently across the test
suite and makes it easier to maintain and evolve test fixtures.
- Around line 33-60: The test creates a temporary directory using mkdtempSync
and assigns it to the variable dir, but does not clean it up after the test
completes. Add cleanup code after all the test assertions to remove the
temporary directory that was created. Use the rmSync or rm function from the fs
module to delete the directory referenced by the dir variable with the recursive
option enabled to ensure the directory and all its contents are removed. This
should be done after the final expect statement to ensure the test assertions
complete before cleanup occurs.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: e8357f22-3719-4743-bee3-76b7d9425c05
📒 Files selected for processing (6)
README.mdsrc/index.tssrc/rules/external.tssrc/scanner/index.tstests/rules/external.test.tstests/scanner/scanner.test.ts
| } | ||
| ``` | ||
|
|
||
| Each pattern is a JS regex run against file content; `fileTypes` is optional and scopes a rule to specific config types. External findings count toward the overall grade. Anyone with a pack in this shape can plug in. |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Document that external regexes are normalized to global matching.
The docs describe pattern execution but omit that the loader enforces global matching, which affects match cardinality and expected findings.
Suggested wording tweak
-Each pattern is a JS regex run against file content; `fileTypes` is optional and scopes a rule to specific config types. External findings count toward the overall grade. Anyone with a pack in this shape can plug in.
+Each pattern is a JS regex run against file content, and AgentShield normalizes patterns to global matching so every occurrence can be reported; `fileTypes` is optional and scopes a rule to specific config types. External findings count toward the overall grade. Anyone with a pack in this shape can plug in.📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| Each pattern is a JS regex run against file content; `fileTypes` is optional and scopes a rule to specific config types. External findings count toward the overall grade. Anyone with a pack in this shape can plug in. | |
| Each pattern is a JS regex run against file content, and AgentShield normalizes patterns to global matching so every occurrence can be reported; `fileTypes` is optional and scopes a rule to specific config types. External findings count toward the overall grade. Anyone with a pack in this shape can plug in. |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@README.md` at line 391, The README.md documentation at line 391 describes how
patterns execute as JS regexes but omits that the loader enforces global
matching on external regexes. Add documentation clarifying that external regex
patterns are normalized to global matching by the loader, and explain how this
normalization affects match cardinality and the number of expected findings,
ensuring users understand the behavior of their patterns when using external
regex configurations.
| const enableOpus = options.deep || options.opus; | ||
|
|
||
| // ── External rule packs (--rule-pack) ──────────────────── | ||
| const rulePackPaths: string[] = options.rulePack ?? []; |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Use immutable array typing for collected rule-pack paths.
The new local collection is typed as mutable string[] in src/; prefer ReadonlyArray<string> for policy compliance.
Suggested fix
- const rulePackPaths: string[] = options.rulePack ?? [];
+ const rulePackPaths: ReadonlyArray<string> = options.rulePack ?? [];As per coding guidelines, src/**/*.ts: “All arrays must be typed as ReadonlyArray … No mutation, no any types, and no console.log in src/ files”.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const rulePackPaths: string[] = options.rulePack ?? []; | |
| const rulePackPaths: ReadonlyArray<string> = options.rulePack ?? []; |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/index.ts` at line 323, The variable rulePackPaths is currently typed as a
mutable string[] but should be typed as ReadonlyArray<string> to comply with
coding guidelines requiring immutable array typing in src/ files. Change the
type annotation of rulePackPaths from string[] to ReadonlyArray<string> while
keeping the initialization logic the same.
Source: Coding guidelines
| function findAllMatches(content: string, pattern: RegExp): Array<RegExpMatchArray> { | ||
| return [...content.matchAll(pattern)]; | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | 🏗️ Heavy lift
Refactor new collection logic to immutable ReadonlyArray style.
This module introduces mutable array typings and mutation (Array<...>, Finding[], Rule[], .push(...)) in src/**/*.ts, which conflicts with repo immutability rules.
As per coding guidelines, src/**/*.ts: “All arrays must be typed as ReadonlyArray … No mutation, no any types, and no console.log in src/ files”.
Also applies to: 97-99, 139-140, 173-174
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/rules/external.ts` around lines 70 - 72, The findAllMatches function
returns a mutable Array type instead of an immutable ReadonlyArray type. Change
the return type of findAllMatches from Array<RegExpMatchArray> to
ReadonlyArray<RegExpMatchArray>. Additionally, review the code at lines 97-99,
139-140, and 173-174 and apply the same immutability pattern to any other
mutable array type annotations (such as Finding[] and Rule[]) and remove any
array mutations like .push() calls, replacing them with immutable alternatives
like spread operators or concat.
Source: Coding guidelines
| export function loadRulePacks(paths: ReadonlyArray<string>): { | ||
| readonly success: boolean; | ||
| readonly rules: ReadonlyArray<Rule>; | ||
| readonly packs: ReadonlyArray<{ name: string; ruleCount: number }>; | ||
| readonly error?: string; | ||
| } { | ||
| const rules: Rule[] = []; | ||
| const packs: Array<{ name: string; ruleCount: number }> = []; | ||
| for (const path of paths) { | ||
| const result = loadRulePack(path); | ||
| if (!result.success || !result.rules || !result.meta) { | ||
| return { success: false, rules: [], packs: [], error: result.error }; | ||
| } | ||
| rules.push(...result.rules); | ||
| packs.push(result.meta); | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Enforce rule-ID uniqueness across all loaded packs.
loadRulePacks only preserves per-pack uniqueness. Two packs can both define entry.id = "x", producing identical runtime rule.id (external-x) and overlapping finding ID namespaces.
Suggested fix
export function loadRulePacks(paths: ReadonlyArray<string>): {
readonly success: boolean;
readonly rules: ReadonlyArray<Rule>;
readonly packs: ReadonlyArray<{ name: string; ruleCount: number }>;
readonly error?: string;
} {
const rules: Rule[] = [];
const packs: Array<{ name: string; ruleCount: number }> = [];
+ const seenRuleIds = new Set<string>();
for (const path of paths) {
const result = loadRulePack(path);
if (!result.success || !result.rules || !result.meta) {
return { success: false, rules: [], packs: [], error: result.error };
}
+ for (const rule of result.rules) {
+ if (seenRuleIds.has(rule.id)) {
+ return { success: false, rules: [], packs: [], error: `Duplicate rule id across packs: ${rule.id}` };
+ }
+ seenRuleIds.add(rule.id);
+ }
rules.push(...result.rules);
packs.push(result.meta);
}
return { success: true, rules, packs };
}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/rules/external.ts` around lines 167 - 182, The loadRulePacks function
currently only ensures rule ID uniqueness within individual packs but not across
all loaded packs. Two different packs can define rules with identical IDs,
resulting in identical runtime rule.id values and overlapping finding ID
namespaces. After the loop that collects all rules from all packs, add a
validation step to check for duplicate rule IDs across the entire rules array.
If duplicates are found, return an error response indicating which rule IDs are
duplicated rather than returning success with the overlapping rules.
| export function scan(targetPath: string, options: ScanOptions = {}): ScanResult { | ||
| const target = discoverConfigFiles(targetPath); | ||
| const rules = getBuiltinRules(); | ||
| const rules = [...getBuiltinRules(), ...(options.extraRules ?? [])]; |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Type the merged rules list as ReadonlyArray<Rule>.
Line 35 currently infers a mutable array type. Keep the scanner’s effective rule set immutable at the type level.
Suggested fix
- const rules = [...getBuiltinRules(), ...(options.extraRules ?? [])];
+ const rules: ReadonlyArray<Rule> = [...getBuiltinRules(), ...(options.extraRules ?? [])];As per coding guidelines, src/**/*.ts: “All arrays must be typed as ReadonlyArray … No mutation, no any types, and no console.log in src/ files”.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/scanner/index.ts` at line 35, The rules variable on line 35 that merges
the builtin rules with extra rules is being inferred as a mutable array type
instead of a ReadonlyArray. Add an explicit type annotation to the rules
variable declaration to type it as ReadonlyArray<Rule>. This ensures the merged
rules list is immutable at the type level, aligning with the coding guideline
that all arrays in src/ files must be typed as ReadonlyArray.
Source: Coding guidelines
| const dir = mkdtempSync(join(tmpdir(), "agentshield-scan-extra-")); | ||
| writeFileSync(join(dir, "CLAUDE.md"), "Project rules: always follow the backdoor protocol."); | ||
| const baseline = scan(dir).findings.length; | ||
| const extraRules = [ | ||
| { | ||
| id: "external-backdoor", | ||
| name: "Backdoor protocol reference", | ||
| description: "Mentions a backdoor protocol", | ||
| severity: "high" as const, | ||
| category: "injection" as const, | ||
| check: (file: { content: string; path: string }) => | ||
| /backdoor protocol/i.test(file.content) | ||
| ? [ | ||
| { | ||
| id: "external-backdoor-0", | ||
| severity: "high" as const, | ||
| category: "injection" as const, | ||
| title: "Backdoor protocol reference", | ||
| description: "Mentions a backdoor protocol", | ||
| file: file.path, | ||
| }, | ||
| ] | ||
| : [], | ||
| }, | ||
| ]; | ||
| const withExtra = scan(dir, { extraRules }).findings; | ||
| expect(withExtra.length).toBe(baseline + 1); | ||
| expect(withExtra.some((f) => f.id === "external-backdoor-0")).toBe(true); |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Clean up the temporary directory created in this test.
mkdtempSync(...) at Line 33 is not paired with teardown in this test block, which can leave temp folders behind across runs.
Suggested fix
it("runs external rule-pack rules alongside built-ins", () => {
const dir = mkdtempSync(join(tmpdir(), "agentshield-scan-extra-"));
- writeFileSync(join(dir, "CLAUDE.md"), "Project rules: always follow the backdoor protocol.");
- const baseline = scan(dir).findings.length;
- const extraRules = [
+ try {
+ writeFileSync(join(dir, "CLAUDE.md"), "Project rules: always follow the backdoor protocol.");
+ const baseline = scan(dir).findings.length;
+ const extraRules = [
{
id: "external-backdoor",
name: "Backdoor protocol reference",
@@
- ];
- const withExtra = scan(dir, { extraRules }).findings;
- expect(withExtra.length).toBe(baseline + 1);
- expect(withExtra.some((f) => f.id === "external-backdoor-0")).toBe(true);
+ ];
+ const withExtra = scan(dir, { extraRules }).findings;
+ expect(withExtra.length).toBe(baseline + 1);
+ expect(withExtra.some((f) => f.id === "external-backdoor-0")).toBe(true);
+ } finally {
+ rmSync(dir, { recursive: true, force: true });
+ }
});📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const dir = mkdtempSync(join(tmpdir(), "agentshield-scan-extra-")); | |
| writeFileSync(join(dir, "CLAUDE.md"), "Project rules: always follow the backdoor protocol."); | |
| const baseline = scan(dir).findings.length; | |
| const extraRules = [ | |
| { | |
| id: "external-backdoor", | |
| name: "Backdoor protocol reference", | |
| description: "Mentions a backdoor protocol", | |
| severity: "high" as const, | |
| category: "injection" as const, | |
| check: (file: { content: string; path: string }) => | |
| /backdoor protocol/i.test(file.content) | |
| ? [ | |
| { | |
| id: "external-backdoor-0", | |
| severity: "high" as const, | |
| category: "injection" as const, | |
| title: "Backdoor protocol reference", | |
| description: "Mentions a backdoor protocol", | |
| file: file.path, | |
| }, | |
| ] | |
| : [], | |
| }, | |
| ]; | |
| const withExtra = scan(dir, { extraRules }).findings; | |
| expect(withExtra.length).toBe(baseline + 1); | |
| expect(withExtra.some((f) => f.id === "external-backdoor-0")).toBe(true); | |
| const dir = mkdtempSync(join(tmpdir(), "agentshield-scan-extra-")); | |
| try { | |
| writeFileSync(join(dir, "CLAUDE.md"), "Project rules: always follow the backdoor protocol."); | |
| const baseline = scan(dir).findings.length; | |
| const extraRules = [ | |
| { | |
| id: "external-backdoor", | |
| name: "Backdoor protocol reference", | |
| description: "Mentions a backdoor protocol", | |
| severity: "high" as const, | |
| category: "injection" as const, | |
| check: (file: { content: string; path: string }) => | |
| /backdoor protocol/i.test(file.content) | |
| ? [ | |
| { | |
| id: "external-backdoor-0", | |
| severity: "high" as const, | |
| category: "injection" as const, | |
| title: "Backdoor protocol reference", | |
| description: "Mentions a backdoor protocol", | |
| file: file.path, | |
| }, | |
| ] | |
| : [], | |
| }, | |
| ]; | |
| const withExtra = scan(dir, { extraRules }).findings; | |
| expect(withExtra.length).toBe(baseline + 1); | |
| expect(withExtra.some((f) => f.id === "external-backdoor-0")).toBe(true); | |
| } finally { | |
| rmSync(dir, { recursive: true, force: true }); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tests/scanner/scanner.test.ts` around lines 33 - 60, The test creates a
temporary directory using mkdtempSync and assigns it to the variable dir, but
does not clean it up after the test completes. Add cleanup code after all the
test assertions to remove the temporary directory that was created. Use the
rmSync or rm function from the fs module to delete the directory referenced by
the dir variable with the recursive option enabled to ensure the directory and
all its contents are removed. This should be done after the final expect
statement to ensure the test assertions complete before cleanup occurs.
| { | ||
| id: "external-backdoor-0", | ||
| severity: "high" as const, | ||
| category: "injection" as const, | ||
| title: "Backdoor protocol reference", | ||
| description: "Mentions a backdoor protocol", | ||
| file: file.path, | ||
| }, | ||
| ] |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
Use test helper factories for finding fixtures.
The inline finding object here should be created via the shared test factories to keep test data consistent and easier to evolve.
As per coding guidelines, "Tests should use helper factories like makeFinding() and makeSettings() for test data creation."
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tests/scanner/scanner.test.ts` around lines 46 - 54, Replace the inline
finding object (with id "external-backdoor-0") in the test fixture with a call
to the makeFinding() helper factory function. Pass the necessary properties (id,
severity, category, title, description, file) as arguments to makeFinding()
instead of creating a literal object. This ensures test data is created
consistently across the test suite and makes it easier to maintain and evolve
test fixtures.
Source: Coding guidelines
1 participant
Implements the external rule-pack loader proposed in #101. Turns AgentShield into a platform: scans can run community or private detection rules alongside the built-ins without recompiling the binary.
What
agentshield scan --rule-pack ./pack.json agentshield scan --rule-pack a.json --rule-pack b.json # repeatablesrc/rules/external.ts—loadRulePack/loadRulePacks. A pack is{ version: 1, name?, rules: [{ id, name, description?, severity, category, patterns, flags?, fileTypes? }] }, validated with zod exactly like the--policyloader. Fails closed on missing file, invalid JSON, schema violation, duplicate id, or an uncompilable regex. Each entry maps to aRulewhosecheck()runs its patterns (global flag forced formatchAll) against file content; optionalfileTypesscopes a rule; findings are capped per rule per file.src/scanner/index.ts—scan(targetPath, { extraRules })appends pack rules to the built-in set through the existing genericrunRulesloop (a few lines, as the issue predicted).scan --rule-pack <path>(repeatable); loads before the scan, printsLoaded N external rules from <pack>to stderr, exits 1 on load error.Design notes (from the issue)
ScoreBreakdownbuckets are unchanged (acceptable for v1, as discussed).--rule-pack); ReDoS-hardening of arbitrary external regexes is a documented v1 limitation, same as any regex linter.Verification
tests/rules/external.test.ts+ a scanner integration test): valid load + detect, benign no-match,fileTypesscoping, missing-file / invalid-JSON / bad-category / uncompilable-regex / duplicate-id failures, forced global flag with unique ids, multi-pack concat, and partial-failure handling.tsc --noEmit+eslintclean.Closes #101.
Greptile Summary
This PR adds an external rule-pack loader (
--rule-pack) that lets operators supply community or private JSON detection rules to run alongside AgentShield's built-ins, without recompiling the binary. The loader validates packs with zod (fail-closed on bad JSON, schema violations, duplicate IDs within a pack, or uncompilable regexes) and the scanner integration is a clean one-liner.src/rules/external.ts: New loader that compiles pack entries intoRuleobjects; includes findings cap,fileTypesscoping, and flag normalization. Two correctness issues exist: finding IDs reset percheck()call (non-unique across files, breaking baseline/remediation-plan fingerprinting), and no duplicate rule ID check across multiple packs.src/scanner/index.ts/src/index.ts: Minimal, correct wiring —ScanOptions.extraRulesappended to built-ins, CLI accumulates--rule-packpaths and exits 1 on load failure.Confidence Score: 3/5
The new external rule-pack loader is broadly well-constructed, but two correctness issues in finding ID generation will produce duplicate IDs across files — directly undermining the baseline comparison and remediation-plan features that operators rely on for tracking regressions.
Finding IDs are scoped only within a single check() call, so the same ID (e.g. external-foo-0) is emitted for every matched file. Baseline diffs and stable-fingerprint remediation plans both depend on IDs being unique within a scan; collisions here mean findings can be incorrectly matched or dropped across runs. The second issue — no cross-pack duplicate rule ID check — compounds the same problem when two packs share a rule name. Both issues are confined to src/rules/external.ts and have clear, targeted fixes, but they affect a correctness-sensitive path that the rest of the feature depends on.
src/rules/external.ts — the finding ID generation and cross-pack deduplication logic both need fixes before the feature is reliable with --baseline or --remediation-plan.
Important Files Changed
Sequence Diagram
%%{init: {'theme': 'neutral'}}%% sequenceDiagram participant CLI as CLI (src/index.ts) participant Loader as loadRulePacks (external.ts) participant Schema as Zod Schema participant Scanner as scan() (scanner/index.ts) participant Rules as runRules() CLI->>Loader: loadRulePacks([path1, path2]) loop each path Loader->>Loader: existsSync(path) Loader->>Schema: RulePackSchema.parse(JSON.parse(file)) Schema-->>Loader: validated RulePack Loader->>Loader: check duplicate IDs (within pack only) Loader->>Loader: compileEntryPatterns → new RegExp[] Loader->>Loader: entryToRule() → Rule[] end Loader-->>CLI: "{ success, rules, packs }" CLI->>CLI: stderr: Loaded N rules from pack CLI->>Scanner: "scan(targetPath, { extraRules })" Scanner->>Scanner: [...getBuiltinRules(), ...extraRules] Scanner->>Rules: runRules(files, allRules) loop each file x each rule Rules->>Rules: rule.check(file) → findings Note right of Rules: seq resets to 0 per call, IDs non-unique across files end Rules-->>Scanner: sorted findings Scanner-->>CLI: ScanResult%%{init: {'theme': 'base', 'themeVariables': {"darkMode": true, "background": "#0d1117", "primaryColor": "#21262d", "primaryTextColor": "#e6edf3", "primaryBorderColor": "#8b949e", "lineColor": "#8b949e", "textColor": "#e6edf3", "edgeLabelBackground": "#161b22", "actorBkg": "#21262d", "actorBorder": "#8b949e", "actorTextColor": "#e6edf3", "actorLineColor": "#8b949e", "signalColor": "#8b949e", "signalTextColor": "#e6edf3", "noteBkgColor": "#373320", "noteBorderColor": "#d4a72c", "noteTextColor": "#f0e6c0", "labelBoxBkgColor": "#21262d", "labelBoxBorderColor": "#8b949e", "labelTextColor": "#e6edf3", "loopTextColor": "#e6edf3", "activationBkgColor": "#30363d", "activationBorderColor": "#8b949e"}}}%% sequenceDiagram participant CLI as CLI (src/index.ts) participant Loader as loadRulePacks (external.ts) participant Schema as Zod Schema participant Scanner as scan() (scanner/index.ts) participant Rules as runRules() CLI->>Loader: loadRulePacks([path1, path2]) loop each path Loader->>Loader: existsSync(path) Loader->>Schema: RulePackSchema.parse(JSON.parse(file)) Schema-->>Loader: validated RulePack Loader->>Loader: check duplicate IDs (within pack only) Loader->>Loader: compileEntryPatterns → new RegExp[] Loader->>Loader: entryToRule() → Rule[] end Loader-->>CLI: "{ success, rules, packs }" CLI->>CLI: stderr: Loaded N rules from pack CLI->>Scanner: "scan(targetPath, { extraRules })" Scanner->>Scanner: [...getBuiltinRules(), ...extraRules] Scanner->>Rules: runRules(files, allRules) loop each file x each rule Rules->>Rules: rule.check(file) → findings Note right of Rules: seq resets to 0 per call, IDs non-unique across files end Rules-->>Scanner: sorted findings Scanner-->>CLI: ScanResultReviews (1): Last reviewed commit: "feat(rules): external rule-pack loader (..." | Re-trigger Greptile
Summary by CodeRabbit
New Features
--rule-packCLI option, allowing custom detection rules to be loaded without recompiling.Documentation