feat: add ast-guard lib an AST validator #71
Conversation
Add comprehensive AST-based JavaScript validation library with: Features: - 7 validation rules (DisallowedIdentifier, NoEval, NoAsync, ForbiddenLoop, etc.) - 4-tier security preset system (STRICT > SECURE > STANDARD > PERMISSIVE) - Custom error classes (RuleConfigurationError, ConfigurationError, etc.) - Extensible rule plugin architecture - TypeScript with strict mode Testing & Quality: - 188 tests with 95.11% code coverage - Advanced security testing with 24 attack vector scenarios - Comprehensive error handling tests Documentation: - Complete API documentation with examples - SECURITY-AUDIT.md documenting 67 blocked attack vectors - Known limitations and defense-in-depth strategies - CLAUDE.md for monorepo development guidelines Security Model: - STRICT preset: Bank-grade security (90+ dangerous identifiers blocked) - SECURE preset: High security with flexibility - STANDARD preset: Balanced security for general use - PERMISSIVE preset: Minimal restrictions 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
WalkthroughAdds a new ast-guard library under libs/ast-guard: validator, interfaces, errors, seven built-in validation rules, preset factories (strict/secure/standard/permissive), extensive tests, ESLint/Jest/TS/Nx configs, package metadata, changelog, security audit, and documentation. No changes to other runtime projects. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant JSAstValidator
participant Acorn
participant Rule as ValidationRule
participant Result
User->>JSAstValidator: validate(source, config)
JSAstValidator->>Acorn: parse(source)
Acorn-->>JSAstValidator: AST or parseError
JSAstValidator->>JSAstValidator: getEnabledRules(config)
loop each enabled Rule
JSAstValidator->>Rule: validate(context)
alt rule reports issue
Rule->>JSAstValidator: report(issue)
JSAstValidator->>Result: collect issue
alt stopOnFirstError or maxIssues reached
JSAstValidator->>JSAstValidator: short-circuit (StopValidationError)
end
else
Rule-->>JSAstValidator: no issues
end
end
JSAstValidator->>Result: assemble ValidationResult & stats
Result-->>User: return result
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes
Possibly related PRs
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 13
🧹 Nitpick comments (19)
libs/ast-guard/README.md (1)
13-20: Clarify “Zero Dependencies” wording for accuracyThe bullet says “Zero Dependencies” but immediately lists
acornandacorn-walkas dependencies. This can confuse users scanning for transitive footprint.Consider rephrasing to something like:
Minimal Dependencies: Only
acornandacorn-walkas runtime dependenciesto avoid contradicting the heading.
libs/ast-guard/PENETRATION-TEST-SUMMARY.md (1)
226-242: Add a language identifier to this fenced code blockmarkdownlint (MD040) flags the defense‑in‑depth “Layer 1/2/3/4” block because it uses a bare triple‑backtick fence without a language.
Consider adding a language hint, e.g.:
```text Layer 1: AST Guard (Static Analysis) ...to satisfy linting and improve rendering consistency. </blockquote></details> <details> <summary>libs/ast-guard/SECURITY-AUDIT.md (1)</summary><blockquote> `463-488`: **Add language to fenced “Recommended Security Stack” block** The ASCII‑diagram block under “Recommended Security Stack (Defense in Depth)” uses a bare triple‑backtick fence, which triggers MD040. You can mark it as plain text: ```markdown ```text ┌─────────────────────────────────────┐ │ 1. AST Guard (STRICT preset) │ ...to keep markdownlint happy and make the intent clearer in rendered views. </blockquote></details> <details> <summary>libs/ast-guard/src/rules/required-function-call.rule.ts (1)</summary><blockquote> `25-105`: **Rule implementation looks solid; consider tightening MemberExpression handling** Core behavior (constructor guard, counting logic, min/max reporting, and error shapes) looks correct and consistent with the rest of the framework. Two edge cases you may want to refine: - In the `MemberExpression` branch you currently count any call where `callee.property.type === 'Identifier'`. That will include `obj[callTool]()` as a call to `"callTool"` but not `obj["callTool"]()`. If you only want plain method syntax (`obj.callTool()`), you could restrict to non‑computed properties; if you also want `obj["callTool"]()` to count, you’d need a separate `Literal` branch. ```diff - // Handle member calls: obj.funcName() - if (callee.type === 'MemberExpression' && callee.property.type === 'Identifier') { - const name = callee.property.name; + // Handle member calls: obj.funcName() + if ( + callee.type === 'MemberExpression' && + !callee.computed && + callee.property.type === 'Identifier' + ) { + const name = callee.property.name; if (callCounts.has(name)) { callCounts.set(name, callCounts.get(name)! + 1); } }
- If you consider configurations with
minCalls > maxCallsinvalid, you might also add a small sanity check in the constructor to fail fast instead of allowing contradictory thresholds.These are non‑blocking; current behavior is coherent as long as the above semantics are intentional.
libs/ast-guard/package.json (1)
1-48: Verify thatmain/types/exportsmatch the actual published layoutThis manifest looks generally good, but a couple of details are worth double‑checking before publishing:
main/types/exportsall assume./dist/src/index.{js,d.ts}relative to the package root. Make sure the published package root and your build output actually produce that structure (e.g., if you publish fromdist/libs/ast-guard, you should confirm thatdist/libs/ast-guard/dist/src/index.jsreally exists).The
developmentcondition currently points to./src/index.ts. If any consumer tooling enables thedevelopmentcondition but does not understand TypeScript innode_modules, this can fail at runtime/bundle time. If you don’t explicitly need to ship TS source to consumers, consider pointingdevelopmentat the built JS (same asimport/default) instead.These are packaging/interop concerns rather than functional bugs, but worth validating now to avoid broken entry points after publish.
libs/ast-guard/tsconfig.lib.json (1)
1-12: Lib tsconfig looks solid; decorator flags are optional.The config matches typical Nx-style library settings (shared base, dist/out-tsc, declarations, test files excluded). If this lib never uses decorators, you could drop
emitDecoratorMetadataandexperimentalDecoratorsto keep compiler options minimal, but it’s fine as-is.libs/ast-guard/src/rules/index.ts (1)
1-11: Rule barrel cleanly defines the built-in rule surface; align docs accordingly.The exports line up with the rules used by presets and tests, which is good for a single public entry point. Since this is part of the SDK API, just make sure the docs under
docs/draft/docs/**enumerate these rule names so consumers know what’s available.libs/ast-guard/src/__tests__/presets.spec.ts (1)
209-220: Inline comment about SECURE preset’s disallowed identifiers is slightly misleading.The comment says “constructor and proto are not in the disallowed list for secure”, but from
secure.preset.tsonlyconstructoris allowed—__proto__is actually disallowed there. The test itself only checksconstructor, so it’s correct; updating the comment to mention justconstructorwould avoid confusion:- // constructor and __proto__ are not in the disallowed list for secure + // constructor is not in the disallowed list for securelibs/ast-guard/src/rules/no-eval.rule.ts (1)
18-52: Broaden detection to member-access and indirect eval patternsThe current logic correctly flags direct
eval(...),new Function(...), andsetTimeout/setIntervalwith a string literal first argument, which is a solid baseline. However, it will miss several common dynamic-code patterns:
- Member access:
window.eval(...),globalThis.setTimeout('...'), orthis.setInterval('...')(callee is aMemberExpression, not anIdentifier).- Indirect eval:
(0, eval)('code')where the callee is aSequenceExpression.- Non-
Literalstring code, e.g. simple template literals that still represent static strings.If you want this rule to act as a true “no dynamic execution” guard, consider:
- Treating
MemberExpressioncallees with a property namedeval,setTimeout, orsetIntervalas violations.- Optionally handling obvious static
TemplateLiteralcases as equivalent to stringLiterals.- Adding tests in
__tests__that lock in these behaviors.This would materially improve security coverage without changing the public API surface.
Also applies to: 55-70
libs/ast-guard/src/rules/disallowed-identifier.rule.ts (1)
31-42: Clarify identifier contexts you intend to block (and optionally precompute the set)Functionally this works, but a couple of behavioral nuances are worth calling out:
- Matching on every
Identifiernode means you’ll flag cases like object keys, property accesses, and parameter names, e.g.{ eval: 1 },obj.eval, orfunction eval() {}. If the intent is specifically to block usage of certain globals (likeprocessorrequire), you may want to narrow matches to particular contexts (e.g., identifier references, not property keys/params) and add tests to lock that behavior in.- The disallowed list never changes per rule instance, so you could precompute the
Setonce in the constructor and reuse it invalidateto avoid reallocation on every run. This is a micro-optimization but easy to apply.If the current behavior is intentional (block any appearance of those identifiers), then just documenting that explicitly in the rule’s docs would be enough.
Also applies to: 44-60
libs/ast-guard/eslint.config.mjs (1)
1-37: ESLint flat config is reasonable; consider simplifying the JSON parser importExtending the root
eslint.config.mjsand adding TS/JSON overrides plus ignore patterns looks good.For the JSON parser, you currently use:
languageOptions: { parser: await import('jsonc-eslint-parser'), },This relies on top‑level
awaitand on ESLint accepting the dynamic import namespace as a parser object. To reduce surprises, you might prefer one of:
A static ESM import:
import jsoncParser from 'jsonc-eslint-parser'; // ... languageOptions: { parser: jsoncParser, },Or, if you keep the dynamic import, explicitly use
.defaultto make the intent clear:languageOptions: { parser: (await import('jsonc-eslint-parser')).default, },Also just confirm your Node/ESLint toolchain is configured to support top‑level
awaitin flat configs.libs/ast-guard/src/__tests__/advanced-attack-vectors.spec.ts (1)
481-507: Minor comment mismatch in multi‑vector attack testIn the “Comprehensive Multi‑Vector Attack” test, the comment says:
// BLOCKED: Multiple identifiers caught ('e', 'constructor', 'process', 'Function')Under the strict preset,
constructor,process, andFunctionare indeed in the disallowed list, but plain'e'is not. The test only assertsissues.length > 0, so behavior is fine; the comment is just slightly misleading.You may want to update the comment to only list identifiers that are actually disallowed by default (or explicitly configured) to avoid confusion for future readers.
libs/ast-guard/src/presets/strict.preset.ts (1)
58-175: Factor out shared disallowed identifier sets between presetsThe strict preset’s
disallowedIdentifiersarray is long and largely overlaps with the secure preset’s list. Maintaining two separate hard‑coded lists will be error‑prone when you inevitably adjust what counts as “dangerous”.Consider extracting shared constants or small helpers, e.g.:
BASE_DANGEROUS_IDENTIFIERSSTRICT_ONLY_IDENTIFIERSSECURE_ONLY_IDENTIFIERSand compose the preset lists from those, possibly adding
options.additionalDisallowedIdentifierson top. This keeps behavior between presets intentionally different while making changes auditable and less likely to drift.libs/ast-guard/src/rules/no-async.rule.ts (1)
7-14: NoAsyncRule logic is correct; consider refining message customizationThe traversal and allow flags behave as expected (covering async declarations/expressions/arrows and await), and align with the tests.
One minor ergonomic point: a single
messageoption is reused for all violation types (NO_ASYNCandNO_AWAIT). If consumers ever want different wording for async functions vsawait, you might later extend the options to support more granular messages (e.g.,asyncMessage,awaitMessage) while keeping the currentmessageas a default fallback.Also applies to: 21-26, 29-93
libs/ast-guard/src/presets/index.ts (1)
13-30: Preset factory and Presets helper provide a clean public API
createPresetcleanly switches onPresetLeveland fails fast withConfigurationErrorfor unknown values, and thePresetsobject offers a convenient, typed entry point for consumers. The extra re-exports plus local imports for the same factories are slightly redundant but not problematic.Also applies to: 31-65, 86-97
libs/ast-guard/src/rules/unreachable-code.rule.ts (1)
4-11: Doc comment overstates current detection scopeThe rule comment mentions “code after infinite loops” and “dead branches in conditionals”, but the implementation only handles:
- Statements following terminal statements (return/throw/break/continue) in blocks.
- Simple
ifconstructs where both branches are terminal.There is no current detection for infinite loops or general dead branches. Consider tightening the description to match implemented behavior, or extending the implementation to actually cover those cases.
libs/ast-guard/src/__tests__/advanced-security.spec.ts (2)
12-52: Align test names with current expectations (or mark as TODO)Several specs under “Property Access Obfuscation Attacks” are named as if the guard already blocks the attacks (e.g., “should block computed property access to constructor”), but the assertions explicitly expect
result.validto betrueand comments explain this as a current limitation.That can be misleading to someone scanning test names only. Consider either:
- Renaming these tests to reflect that they document known gaps (e.g.,
it('documents current limitation for computed property access to constructor', ...)), or- Adding a clear
TODOmarker in the titles so it’s obvious these are aspirational.
680-682: Avoidconsole.login tests to keep CI output cleanThe “Comprehensive Bank-Level Lockdown Test” logs validation issues on failure. This can clutter CI output and hide more relevant Jest diagnostics. It’s usually better to rely on Jest’s assertion messages, or only log conditionally when debugging locally (e.g., behind an env flag).
libs/ast-guard/src/rules/call-argument-validation.rule.ts (1)
176-200: TightengetArgumentTypeto better match supportedexpectedTypes
getArgumentTypehas a few inconsistencies with the advertisedexpectedTypes:
- For
Literalnodes:
Array.isArray(node.value)will never be true for acorn ASTs; arrays are represented asArrayExpression, so this branch is effectively dead.- BigInt literals will produce
typeof node.value === 'bigint', but'bigint'is not in the allowedexpectedTypesunion, so any BigInt argument will always be reported as a type mismatch.- Only
nullliterals are classified as'literal'; other primitive literals ('foo',42,true) are reported as'string','number','boolean'respectively, soexpectedTypes: ['literal']doesn’t behave as a general “any literal” matcher.- Node kinds like
TemplateLiteral(for string-like arguments) are reported as'unknown', which may be surprising if a caller expects template strings to satisfy'string'.To make behavior more predictable, consider:
- Removing the
Array.isArray(node.value)branch and relying onArrayExpressionfor'array'.- Deciding what
expectedTypes: 'literal'should mean (e.g., “any literal”, or onlynull), and aligninggetArgumentTypeaccordingly.- Mapping
TemplateLiteral(and possibly tagged templates) to'string', and normalizing BigInt literals either to'literal'or to a dedicated'bigint'type that can be configured.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (38)
CLAUDE.md(1 hunks)libs/ast-guard/CHANGELOG.md(1 hunks)libs/ast-guard/PENETRATION-TEST-SUMMARY.md(1 hunks)libs/ast-guard/README.md(1 hunks)libs/ast-guard/SECURITY-AUDIT.md(1 hunks)libs/ast-guard/eslint.config.mjs(1 hunks)libs/ast-guard/jest.config.ts(1 hunks)libs/ast-guard/jest.setup.ts(1 hunks)libs/ast-guard/package.json(1 hunks)libs/ast-guard/project.json(1 hunks)libs/ast-guard/src/__tests__/advanced-attack-vectors.spec.ts(1 hunks)libs/ast-guard/src/__tests__/advanced-security.spec.ts(1 hunks)libs/ast-guard/src/__tests__/errors.spec.ts(1 hunks)libs/ast-guard/src/__tests__/presets.spec.ts(1 hunks)libs/ast-guard/src/__tests__/rules.spec.ts(1 hunks)libs/ast-guard/src/__tests__/security.spec.ts(1 hunks)libs/ast-guard/src/__tests__/validator.spec.ts(1 hunks)libs/ast-guard/src/errors.ts(1 hunks)libs/ast-guard/src/index.ts(1 hunks)libs/ast-guard/src/interfaces.ts(1 hunks)libs/ast-guard/src/presets/index.ts(1 hunks)libs/ast-guard/src/presets/permissive.preset.ts(1 hunks)libs/ast-guard/src/presets/secure.preset.ts(1 hunks)libs/ast-guard/src/presets/standard.preset.ts(1 hunks)libs/ast-guard/src/presets/strict.preset.ts(1 hunks)libs/ast-guard/src/presets/types.ts(1 hunks)libs/ast-guard/src/rules/call-argument-validation.rule.ts(1 hunks)libs/ast-guard/src/rules/disallowed-identifier.rule.ts(1 hunks)libs/ast-guard/src/rules/forbidden-loop.rule.ts(1 hunks)libs/ast-guard/src/rules/index.ts(1 hunks)libs/ast-guard/src/rules/no-async.rule.ts(1 hunks)libs/ast-guard/src/rules/no-eval.rule.ts(1 hunks)libs/ast-guard/src/rules/required-function-call.rule.ts(1 hunks)libs/ast-guard/src/rules/unreachable-code.rule.ts(1 hunks)libs/ast-guard/src/validator.ts(1 hunks)libs/ast-guard/tsconfig.json(1 hunks)libs/ast-guard/tsconfig.lib.json(1 hunks)libs/ast-guard/tsconfig.spec.json(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
libs/**
⚙️ CodeRabbit configuration file
libs/**: Contains publishable SDK libraries. Review for API correctness, breaking changes, and consistency with docs. When public APIs change, ensure there is a matching docs/draft/docs/** update (not direct edits under docs/docs/**).
Files:
libs/ast-guard/README.mdlibs/ast-guard/package.jsonlibs/ast-guard/src/rules/no-eval.rule.tslibs/ast-guard/CHANGELOG.mdlibs/ast-guard/src/rules/forbidden-loop.rule.tslibs/ast-guard/jest.config.tslibs/ast-guard/src/rules/disallowed-identifier.rule.tslibs/ast-guard/src/rules/required-function-call.rule.tslibs/ast-guard/src/presets/strict.preset.tslibs/ast-guard/src/__tests__/advanced-security.spec.tslibs/ast-guard/src/__tests__/rules.spec.tslibs/ast-guard/src/index.tslibs/ast-guard/tsconfig.lib.jsonlibs/ast-guard/src/presets/secure.preset.tslibs/ast-guard/src/rules/call-argument-validation.rule.tslibs/ast-guard/src/presets/types.tslibs/ast-guard/src/presets/permissive.preset.tslibs/ast-guard/src/errors.tslibs/ast-guard/src/rules/no-async.rule.tslibs/ast-guard/src/__tests__/errors.spec.tslibs/ast-guard/src/presets/index.tslibs/ast-guard/jest.setup.tslibs/ast-guard/src/interfaces.tslibs/ast-guard/src/rules/unreachable-code.rule.tslibs/ast-guard/src/validator.tslibs/ast-guard/src/__tests__/advanced-attack-vectors.spec.tslibs/ast-guard/src/presets/standard.preset.tslibs/ast-guard/project.jsonlibs/ast-guard/PENETRATION-TEST-SUMMARY.mdlibs/ast-guard/src/rules/index.tslibs/ast-guard/tsconfig.jsonlibs/ast-guard/tsconfig.spec.jsonlibs/ast-guard/src/__tests__/security.spec.tslibs/ast-guard/src/__tests__/validator.spec.tslibs/ast-guard/eslint.config.mjslibs/ast-guard/src/__tests__/presets.spec.tslibs/ast-guard/SECURITY-AUDIT.md
🧬 Code graph analysis (14)
libs/ast-guard/src/rules/no-eval.rule.ts (1)
libs/ast-guard/src/interfaces.ts (2)
ValidationRule(62-77)ValidationContext(44-57)
libs/ast-guard/src/rules/forbidden-loop.rule.ts (2)
libs/ast-guard/src/interfaces.ts (2)
ValidationRule(62-77)ValidationContext(44-57)scripts/strip-dist-from-pkg.js (1)
walk(22-37)
libs/ast-guard/src/rules/disallowed-identifier.rule.ts (2)
libs/ast-guard/src/interfaces.ts (2)
ValidationRule(62-77)ValidationContext(44-57)libs/ast-guard/src/errors.ts (1)
RuleConfigurationError(26-32)
libs/ast-guard/src/presets/secure.preset.ts (9)
libs/ast-guard/src/index.ts (8)
createSecurePreset(62-62)PresetOptions(67-67)ValidationRule(12-12)NoEvalRule(41-41)ForbiddenLoopRule(37-37)NoAsyncRule(42-42)UnreachableCodeRule(39-39)CallArgumentValidationRule(40-40)libs/ast-guard/src/presets/index.ts (2)
createSecurePreset(18-18)PresetOptions(14-14)libs/ast-guard/src/presets/types.ts (1)
PresetOptions(6-50)libs/ast-guard/src/interfaces.ts (1)
ValidationRule(62-77)libs/ast-guard/src/rules/no-eval.rule.ts (1)
NoEvalRule(12-74)libs/ast-guard/src/rules/forbidden-loop.rule.ts (1)
ForbiddenLoopRule(28-84)libs/ast-guard/src/rules/no-async.rule.ts (1)
NoAsyncRule(21-94)libs/ast-guard/src/rules/unreachable-code.rule.ts (1)
UnreachableCodeRule(12-124)libs/ast-guard/src/rules/call-argument-validation.rule.ts (1)
CallArgumentValidationRule(40-201)
libs/ast-guard/src/presets/types.ts (1)
libs/ast-guard/src/rules/call-argument-validation.rule.ts (1)
FunctionArgumentConfig(13-24)
libs/ast-guard/src/errors.ts (1)
libs/ast-guard/src/index.ts (6)
AstGuardError(26-26)ParseError(27-27)RuleConfigurationError(28-28)ConfigurationError(29-29)RuleNotFoundError(30-30)InvalidSourceError(31-31)
libs/ast-guard/src/__tests__/errors.spec.ts (2)
libs/ast-guard/src/errors.ts (6)
AstGuardError(4-10)ParseError(15-21)RuleConfigurationError(26-32)ConfigurationError(37-43)RuleNotFoundError(48-54)InvalidSourceError(59-65)libs/ast-guard/src/index.ts (6)
AstGuardError(26-26)ParseError(27-27)RuleConfigurationError(28-28)ConfigurationError(29-29)RuleNotFoundError(30-30)InvalidSourceError(31-31)
libs/ast-guard/src/presets/index.ts (7)
libs/ast-guard/src/presets/types.ts (1)
PresetOptions(6-50)libs/ast-guard/src/interfaces.ts (1)
ValidationRule(62-77)libs/ast-guard/src/presets/strict.preset.ts (1)
createStrictPreset(52-220)libs/ast-guard/src/presets/secure.preset.ts (1)
createSecurePreset(47-148)libs/ast-guard/src/presets/standard.preset.ts (1)
createStandardPreset(48-107)libs/ast-guard/src/presets/permissive.preset.ts (1)
createPermissivePreset(45-88)libs/ast-guard/src/errors.ts (1)
ConfigurationError(37-43)
libs/ast-guard/src/rules/unreachable-code.rule.ts (2)
libs/ast-guard/src/index.ts (4)
UnreachableCodeRule(39-39)ValidationRule(12-12)ValidationSeverity(22-22)ValidationContext(15-15)libs/ast-guard/src/interfaces.ts (2)
ValidationRule(62-77)ValidationContext(44-57)
libs/ast-guard/src/validator.ts (2)
libs/ast-guard/src/interfaces.ts (6)
ValidationRule(62-77)ValidationConfig(94-103)ValidationResult(108-117)ValidationIssue(28-39)ValidationContext(44-57)ValidationStats(122-135)libs/ast-guard/src/errors.ts (3)
ConfigurationError(37-43)InvalidSourceError(59-65)ParseError(15-21)
libs/ast-guard/src/__tests__/advanced-attack-vectors.spec.ts (2)
libs/ast-guard/src/validator.ts (1)
JSAstValidator(16-229)libs/ast-guard/src/presets/index.ts (1)
Presets(86-97)
libs/ast-guard/src/presets/standard.preset.ts (9)
libs/ast-guard/src/presets/types.ts (1)
PresetOptions(6-50)libs/ast-guard/src/interfaces.ts (1)
ValidationRule(62-77)libs/ast-guard/src/rules/no-eval.rule.ts (1)
NoEvalRule(12-74)libs/ast-guard/src/rules/disallowed-identifier.rule.ts (1)
DisallowedIdentifierRule(25-62)libs/ast-guard/src/rules/forbidden-loop.rule.ts (1)
ForbiddenLoopRule(28-84)libs/ast-guard/src/rules/no-async.rule.ts (1)
NoAsyncRule(21-94)libs/ast-guard/src/rules/unreachable-code.rule.ts (1)
UnreachableCodeRule(12-124)libs/ast-guard/src/rules/required-function-call.rule.ts (1)
RequiredFunctionCallRule(25-106)libs/ast-guard/src/rules/call-argument-validation.rule.ts (1)
CallArgumentValidationRule(40-201)
libs/ast-guard/src/__tests__/security.spec.ts (8)
libs/ast-guard/src/index.ts (7)
JSAstValidator(8-8)NoEvalRule(41-41)DisallowedIdentifierRule(36-36)ForbiddenLoopRule(37-37)RequiredFunctionCallRule(38-38)CallArgumentValidationRule(40-40)UnreachableCodeRule(39-39)libs/ast-guard/src/validator.ts (1)
JSAstValidator(16-229)libs/ast-guard/src/rules/no-eval.rule.ts (1)
NoEvalRule(12-74)libs/ast-guard/src/rules/disallowed-identifier.rule.ts (1)
DisallowedIdentifierRule(25-62)libs/ast-guard/src/rules/forbidden-loop.rule.ts (1)
ForbiddenLoopRule(28-84)libs/ast-guard/src/rules/required-function-call.rule.ts (1)
RequiredFunctionCallRule(25-106)libs/ast-guard/src/rules/call-argument-validation.rule.ts (1)
CallArgumentValidationRule(40-201)libs/ast-guard/src/rules/unreachable-code.rule.ts (1)
UnreachableCodeRule(12-124)
libs/ast-guard/src/__tests__/presets.spec.ts (7)
libs/ast-guard/src/index.ts (8)
createStrictPreset(61-61)JSAstValidator(8-8)createSecurePreset(62-62)createStandardPreset(63-63)createPermissivePreset(64-64)createPreset(60-60)PresetLevel(58-58)Presets(59-59)libs/ast-guard/src/presets/index.ts (7)
createStrictPreset(17-17)createSecurePreset(18-18)createStandardPreset(19-19)createPermissivePreset(20-20)createPreset(52-65)PresetLevel(14-14)Presets(86-97)libs/ast-guard/src/presets/strict.preset.ts (1)
createStrictPreset(52-220)libs/ast-guard/src/validator.ts (1)
JSAstValidator(16-229)libs/ast-guard/src/presets/secure.preset.ts (1)
createSecurePreset(47-148)libs/ast-guard/src/presets/standard.preset.ts (1)
createStandardPreset(48-107)libs/ast-guard/src/presets/permissive.preset.ts (1)
createPermissivePreset(45-88)
🪛 LanguageTool
CLAUDE.md
[style] ~186-~186: For a more polite and respectful tone, consider rephrasing with ‘please’.
Context: ...rameters 5. Test Utility Functions: Don't forget small helper functions ### Coverage-Fo...
(DONT_FORGET)
libs/ast-guard/SECURITY-AUDIT.md
[grammar] ~38-~38: Use a hyphen to join words.
Context: ... - Use Case: Internal scripts, high trust environments - Blocks: eval() ...
(QB_NEW_EN_HYPHEN)
[grammar] ~173-~173: Use a hyphen to join words.
Context: ...rty is blocked, preventing getter/setter based side effects. --- #### 9. **Bina...
(QB_NEW_EN_HYPHEN)
🪛 markdownlint-cli2 (0.18.1)
libs/ast-guard/PENETRATION-TEST-SUMMARY.md
228-228: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
libs/ast-guard/SECURITY-AUDIT.md
463-463: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🔇 Additional comments (21)
libs/ast-guard/src/errors.ts (1)
1-65: Error hierarchy implementation looks solidThe custom error classes (
AstGuardErrorand its subclasses) correctly:
- Extend
Errorwith a stablecodefield and additional context (line/column, ruleName).- Set
nameand useObject.setPrototypeOfto preserveinstanceofbehavior across inheritance.This provides a clean, type‑safe error surface for consumers and internal code.
CLAUDE.md (2)
136-168: <!-- [scratchpad_start]
[task verify_review_comment complete 3/3]
[observations]
- README.md is for FrontMCP (MCP server framework), NOT ast-guard
- No "validate", "astguard", or custom rule patterns found in README.md
- README.md contains Tool implementations with
async execute()methods (for Tools, not ValidationRules)- CLAUDE.md is ast-guard documentation, but root README is FrontMCP documentation
- ValidationRule.validate() actual signature:
validate(context: ValidationContext): void | Promise<void>(allows sync OR async)- CLAUDE.md example shows
async execute()- this is WRONG on both counts:
- Method name should be
validate, notexecute- Should allow sync OR async, not strictly async Promise
- No AstGuard class or alias exists in codebase - only JSAstValidator
- Anti-pattern comment about "export { JSAstValidator as AstGuard } // WRONG" is misleading since this export doesn't exist
[analysis]
- Review comment is based on false premise: README.md does not contain ValidationRule examples or AstGuard imports
- CLAUDE.md examples have TWO errors: wrong method name (execute vs validate) and wrong return type (Promise vs void | Promise)
- The concern about syncing with README is invalid - README is unrelated FrontMCP docs
- Only real actionable issue: CLAUDE.md code pattern is incorrect
[done]
- Verification complete: review comment is INCORRECT regarding README.md content
- Code pattern in CLAUDE.md is factually wrong about method signature
</scratchpad_end] -->Update CLAUDE.md to use correct ValidationRule contract signature
The ValidationRule interface uses
validate()method, notexecute(). The method signature also supports both sync and async operations—not async-only.Correct the code pattern at lines 136–168 from:
async execute(context: ValidationContext): Promise<void>To:
validate(context: ValidationContext): void | Promise<void>The claim about README.md containing ValidationRule examples or AstGuard imports is unsupported; the repository's README.md covers FrontMCP (an MCP server framework), not ast-guard validation rules. Remove the references to reconciling with README examples unless documentation for ast-guard exists at a different location (e.g.,
docs/subdirectory).Likely an incorrect or invalid review comment.
115-135: CLAUDE.md preset section is correct; README.md and CHANGELOG.md require fixesVerification confirms the CLAUDE.md section (lines 115–135) is accurate and matches the actual implementation. The real inconsistency is elsewhere:
- CLAUDE.md (lines 115–135): ✓ Correctly documents
PresetLevel{STRICT, SECURE, STANDARD, PERMISSIVE} andPresets.strict/secure/standard/permissive- Implementation (
src/presets/): ✓ Confirms STRICT, SECURE, STANDARD, PERMISSIVE with corresponding factory functions- SECURITY-AUDIT.md: ✓ Correctly uses STRICT, SECURE, STANDARD, PERMISSIVE
- README.md & CHANGELOG.md: ✗ Document LOCKDOWN, SECURED, BALANCED, NAIVE, NO_BUILTIN with
Presets.lockdown(),Presets.secured(), etc.—these presets do not exist in the codeThe problem is not CLAUDE.md; it requires no changes. Instead, libs/ast-guard/README.md and libs/ast-guard/CHANGELOG.md must be rewritten to document the actual preset API (STRICT, SECURE, STANDARD, PERMISSIVE) and correct the function names (
.strict(),.secure(),.standard(),.permissive()).Likely an incorrect or invalid review comment.
libs/ast-guard/jest.setup.ts (1)
1-2: Jest setup placeholder is fineA minimal commented stub is OK for now; you can flesh this out once you need shared Jest globals or custom matchers.
libs/ast-guard/tsconfig.json (1)
1-28: TS config structure looks appropriate for a strict libraryExtending the base config, using project references (lib/spec), and enabling strict/null/return/switch checks is a solid setup for this library; no issues from what’s visible here.
libs/ast-guard/tsconfig.spec.json (1)
1-9: Test tsconfig aligns with Jest usageSpec config (CommonJS module, Jest/Node types, and test file globs) is consistent with a standard Jest+TS setup; looks good as-is.
libs/ast-guard/jest.config.ts (1)
1-25: Jest config is well-scoped and consistent.Preset, SWC transform, testMatch, and coverage settings all look appropriate for a Node-focused TS library; no issues from a testing/config standpoint.
libs/ast-guard/src/__tests__/validator.spec.ts (1)
6-62: Rule registration and lookup tests are comprehensive.Constructor,
registerRule/unregisterRule, duplicate detection, andgetRule/getRulesbehaviors are all exercised cleanly, which should keep the rule registry semantics stable.libs/ast-guard/src/__tests__/presets.spec.ts (2)
12-140: Preset tests for STRICT behavior are very thorough.You exercise eval blocking, dangerous identifiers, all loop variants, async/await defaults, required function calls, argument validation, and custom disallowed identifiers, which gives high confidence that
createStrictPresetand its options are wired correctly.
223-582: Preset, factory, and “real-world” tests give excellent end-to-end coverage.STANDARD and PERMISSIVE presets,
createPreset, thePresetshelper, and the real-world scenarios collectively validate loop policies, async policies, identifier lists, required-call semantics, argument validation, and customization hooks. This is a strong safety net for the preset system and its public API.libs/ast-guard/src/presets/permissive.preset.ts (1)
45-88: Preset behavior looks consistent; double‑check severity and docs alignmentThe preset wiring is coherent:
NoEvalRule+ optionalDisallowedIdentifierRule/RequiredFunctionCallRule/CallArgumentValidationRule, with loops and async intentionally left unrestricted.Two things worth verifying:
- The comment says “Unreachable code (as warning)” but the code relies on
new UnreachableCodeRule()defaults. If that rule’sdefaultSeverityis notWARNING, either update its default or adjust the comment to avoid misleading users.- Since
createPermissivePresetbecomes part of the public presets API, ensure the new PERMISSIVE level and its behavior are documented underdocs/draft/docs/**in line with the other presets.libs/ast-guard/src/index.ts (1)
7-67: Public API barrel looks good; treat this surface as stable going forwardThe exports are well-structured and give consumers a clear, centralized entry point (validator, interfaces, errors, built‑in rules, rule option types, and presets including
createPermissivePreset).Given how much is now publicly exported from here, future changes to these names/locations will be breaking. It’s worth:
- Treating this file as the canonical API contract for semver.
- Ensuring corresponding documentation clearly reflects all exported rules, option types, and preset factories.
libs/ast-guard/src/__tests__/errors.spec.ts (1)
1-159: Error hierarchy tests are comprehensive and aligned with implementationsThe suite thoroughly exercises construction, codes, names, and prototype/
instanceofbehavior for all error types and the shared base, matching the implementations inerrors.ts. No changes needed here.libs/ast-guard/src/presets/secure.preset.ts (1)
47-147: Secure preset composition matches documented behaviorThe rule set and options handling line up with the JSDoc:
NoEvalRule+ a strong (but slightly relaxed vs strict) disallowed identifier list, bounded‑loop defaults viaForbiddenLoopRule, async‑functions blocked butawaitallowed by default viaNoAsyncRule, plus optional required‑call and argument‑validation rules. Use of??for options correctly preserves explicitfalsevalues. No functional issues from what’s visible here.libs/ast-guard/src/__tests__/advanced-attack-vectors.spec.ts (1)
52-551: Advanced attack vector coverage is strong and well‑documentedAside from the Unicode‑escape case above, the rest of the suite provides a solid set of edge‑case scenarios: property access chains, destructuring, tagged templates, class constructors,
with, comma operator, logical assignment, generators/async generators, labels, and known static‑analysis limitations are all clearly encoded with expectations that match the described behavior. This is valuable documentation as much as it is regression coverage.libs/ast-guard/src/presets/strict.preset.ts (1)
52-219: Strict preset semantics match the “bank‑grade” description
createStrictPresetwires up the rules in a deterministic order, with defaults that align with the docs:
NoEvalRuleand a very aggressiveDisallowedIdentifierRulelist.- All loop types and async/await blocked by default, overridable via
allowedLoops/allowAsync.- Optional required‑function and argument‑validation enforcement.
- Unreachable code detection always on.
Use of
??ensures explicitly providingfalsein options is respected. From the visible code, this preset matches the intended “maximum security” contract.libs/ast-guard/project.json (1)
1-33: ---No issues found—configuration is correct and consistent
Verification confirms:
scripts/strip-dist-from-pkg.jsexists at the expected path and will execute correctly.- The
publishtarget inlibs/ast-guard/project.jsonfollows the exact same pattern as all other publishable libraries in the monorepo (sdk, cli, adapters, plugins, json-schema-to-zod-v3, vectoriadb, mcp-from-openapi), indicating this is an intentional, standardized approach to publishing.The build target chain (
build-tsc→build→publish) is sound and ready for CI.libs/ast-guard/src/rules/forbidden-loop.rule.ts (1)
7-83: ForbiddenLoopRule implementation matches intended configurable behaviorConfig surface and implementation look solid: per-loop allow flags, sensible default message, and targeted handlers for each loop node type. This aligns with how presets and tests are using the rule.
libs/ast-guard/src/__tests__/rules.spec.ts (1)
1-497: Rule tests are comprehensive and consistent with rule semanticsThe suite thoroughly exercises happy paths and error conditions for each rule (including config errors), and matches the current implementations’ codes/messages. This should give strong confidence in rule behavior.
libs/ast-guard/src/presets/types.ts (1)
1-73: PresetOptions and PresetLevel are well-structured and match preset usageThe configuration surface here lines up cleanly with how the strict/secure/standard/permissive presets consume these options, and the enum/docs clearly express the intended security levels.
libs/ast-guard/src/interfaces.ts (1)
1-135: Core validation interfaces look coherent and aligned with usageThe definitions for
ValidationSeverity,ValidationIssue,ValidationContext,ValidationRule,RuleConfig,ValidationConfig,ValidationResult, andValidationStatsare consistent with how they’re used in the rules andJSAstValidator. No structural issues spotted here.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
libs/ast-guard/src/validator.ts (1)
165-190: PopulaterulesExecutedin stats
rulesExecutedis still hardcoded to0, so the metric is useless (same concern noted earlier). Please thread the actual number of executed rules into this function—e.g., pass a counter fromvalidateand assign it here—instead of leaving this field at zero.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
libs/ast-guard/CHANGELOG.md(1 hunks)libs/ast-guard/src/__tests__/advanced-attack-vectors.spec.ts(1 hunks)libs/ast-guard/src/__tests__/security.spec.ts(1 hunks)libs/ast-guard/src/validator.ts(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- libs/ast-guard/CHANGELOG.md
🧰 Additional context used
📓 Path-based instructions (1)
libs/**
⚙️ CodeRabbit configuration file
libs/**: Contains publishable SDK libraries. Review for API correctness, breaking changes, and consistency with docs. When public APIs change, ensure there is a matching docs/draft/docs/** update (not direct edits under docs/docs/**).
Files:
libs/ast-guard/src/validator.tslibs/ast-guard/src/__tests__/advanced-attack-vectors.spec.tslibs/ast-guard/src/__tests__/security.spec.ts
🧬 Code graph analysis (2)
libs/ast-guard/src/validator.ts (2)
libs/ast-guard/src/interfaces.ts (6)
ValidationRule(62-77)ValidationConfig(94-103)ValidationResult(108-117)ValidationIssue(28-39)ValidationContext(44-57)ValidationStats(122-135)libs/ast-guard/src/errors.ts (3)
ConfigurationError(37-43)InvalidSourceError(59-65)ParseError(15-21)
libs/ast-guard/src/__tests__/advanced-attack-vectors.spec.ts (3)
libs/ast-guard/src/validator.ts (1)
JSAstValidator(16-225)libs/ast-guard/src/index.ts (2)
JSAstValidator(8-8)Presets(59-59)libs/ast-guard/src/presets/index.ts (1)
Presets(86-97)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
libs/ast-guard/src/validator.ts (1)
197-223: Severity-only rule overrides are still treated as “disable rule”When
config.rules[ruleName]is an object without an explicitenabledflag (e.g.{ severity: WARNING }), this branch:// If rule config is object if (ruleConfig.enabled) { enabled.push(rule); }treats
enabled: undefinedas falsy and drops the rule, so severity-only overrides silently disable it. For alibs/**SDK, this breaks the public configuration contract. As per coding guidelines, rule configuration must actually affect which rules run and how.You should only disable the rule when
enabled === false, enable whenenabled === true, and otherwise fall back torule.enabledByDefault. For example:private getEnabledRules(config: ValidationConfig): ValidationRule[] { const enabled: ValidationRule[] = []; for (const rule of this.rules.values()) { const ruleConfig = config.rules?.[rule.name]; // If rule is not configured, use its default enabled state if (ruleConfig === undefined) { if (rule.enabledByDefault) { enabled.push(rule); } continue; } // If rule config is boolean if (typeof ruleConfig === 'boolean') { if (ruleConfig) { enabled.push(rule); } continue; } - // If rule config is object - if (ruleConfig.enabled) { - enabled.push(rule); - } + // If rule config is object: respect explicit enabled flag, else fall back to default + const explicitlyEnabled = + typeof ruleConfig.enabled === 'boolean' ? ruleConfig.enabled : undefined; + const shouldEnable = explicitlyEnabled ?? rule.enabledByDefault; + if (shouldEnable) { + enabled.push(rule); + } } return enabled; }This preserves backward-compatible defaults while making severity-only overrides work as intended.
As per coding guidelines, this should be fixed before publishing the library.
🧹 Nitpick comments (3)
CLAUDE.md (2)
48-59: Barrel exports example is clear but should reference actual file location.The barrel exports documentation provides good guidance on the correct pattern. Consider adding a comment clarifying that developers should reference
libs/ast-guard/src/index.ts(or the actual path) as the implementation example.
21-21: Add hyperlink to SECURITY-AUDIT.md for discoverability.The documentation mentions SECURITY-AUDIT.md multiple times but doesn't provide a direct link. This would improve navigation for developers reviewing security considerations.
-#### ast-guard - -- **Type**: Helper library (independent, publishable) -- **Purpose**: Bank-grade JavaScript validation using AST analysis -- **Security Model**: Four-tier preset system (STRICT > SECURE > STANDARD > PERMISSIVE) -- **Test Count**: 188 tests with 95.11% coverage -- **Key Rules**: DisallowedIdentifier, NoEval, NoAsync, CallArgumentValidation, etc. -- **Documentation**: SECURITY-AUDIT.md documents all 67 blocked attack vectors +#### ast-guard + +- **Type**: Helper library (independent, publishable) +- **Purpose**: Bank-grade JavaScript validation using AST analysis +- **Security Model**: Four-tier preset system (STRICT > SECURE > STANDARD > PERMISSIVE) +- **Test Count**: 188 tests with 95.11% coverage +- **Key Rules**: DisallowedIdentifier, NoEval, NoAsync, CallArgumentValidation, etc. +- **Documentation**: [SECURITY-AUDIT.md](../libs/ast-guard/SECURITY-AUDIT.md) documents all 67 blocked attack vectorsAlso applies to: 87-95
libs/ast-guard/src/presets/strict.preset.ts (1)
52-219: Strict preset wiring is consistent and highly restrictive by designRule composition and option handling look correct: eval and a broad set of dangerous/global identifiers are blocked, all loops and async are denied by default but can be selectively relaxed via
allowedLoops/allowAsync, and optional required-call and argument-validation rules are only added when configured. The resulting behavior matches the “bank-grade” description; just be aware that the disallowed identifier list is very aggressive (e.g., standard constructors and browser APIs) and should be clearly documented for consumers.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
CLAUDE.md(1 hunks)libs/ast-guard/CHANGELOG.md(1 hunks)libs/ast-guard/README.md(1 hunks)libs/ast-guard/src/interfaces.ts(1 hunks)libs/ast-guard/src/presets/permissive.preset.ts(1 hunks)libs/ast-guard/src/presets/secure.preset.ts(1 hunks)libs/ast-guard/src/presets/standard.preset.ts(1 hunks)libs/ast-guard/src/presets/strict.preset.ts(1 hunks)libs/ast-guard/src/rules/unreachable-code.rule.ts(1 hunks)libs/ast-guard/src/validator.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (5)
- libs/ast-guard/README.md
- libs/ast-guard/src/presets/secure.preset.ts
- libs/ast-guard/CHANGELOG.md
- libs/ast-guard/src/interfaces.ts
- libs/ast-guard/src/presets/standard.preset.ts
🧰 Additional context used
📓 Path-based instructions (1)
libs/**
⚙️ CodeRabbit configuration file
libs/**: Contains publishable SDK libraries. Review for API correctness, breaking changes, and consistency with docs. When public APIs change, ensure there is a matching docs/draft/docs/** update (not direct edits under docs/docs/**).
Files:
libs/ast-guard/src/presets/strict.preset.tslibs/ast-guard/src/rules/unreachable-code.rule.tslibs/ast-guard/src/presets/permissive.preset.tslibs/ast-guard/src/validator.ts
🧠 Learnings (1)
📚 Learning: 2025-11-16T22:47:05.139Z
Learnt from: frontegg-david
Repo: agentfront/frontmcp PR: 50
File: docs/docs/v/0.1/getting-started/welcome.mdx:2-3
Timestamp: 2025-11-16T22:47:05.139Z
Learning: In the agentfront/frontmcp repository, the docs/docs/v/** directory governance rule prohibiting manual edits applies to already-frozen releases, but does not preclude initial setup and bootstrap commits that establish the versioned docs structure.
Applied to files:
CLAUDE.md
🧬 Code graph analysis (1)
libs/ast-guard/src/validator.ts (3)
libs/ast-guard/src/index.ts (11)
JSAstValidator(8-8)ValidationRule(12-12)ConfigurationError(29-29)ValidationConfig(13-13)ValidationResult(14-14)InvalidSourceError(31-31)ValidationIssue(16-16)ParseError(27-27)ValidationSeverity(22-22)ValidationContext(15-15)ValidationStats(17-17)libs/ast-guard/src/interfaces.ts (6)
ValidationRule(62-77)ValidationConfig(94-103)ValidationResult(108-119)ValidationIssue(28-39)ValidationContext(44-57)ValidationStats(124-137)libs/ast-guard/src/errors.ts (3)
ConfigurationError(37-43)InvalidSourceError(59-65)ParseError(15-21)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build
🔇 Additional comments (3)
CLAUDE.md (1)
126-140: Anti-patterns and do's sections are well-structured and actionable.The guidance here is practical and specific, making it clear what behaviors are discouraged and encouraged. This supports consistent code quality across the monorepo.
libs/ast-guard/src/validator.ts (1)
116-145: Severity resolution and early-stop behavior look correctThe per-rule
reportclosure now correctly applies severity precedence (issue.severity→ config override → rule default →ERROR) and enforcesstopOnFirstError/maxIssuesafter pushing the issue. This aligns the runtime behavior with the configuration model and previous review feedback.libs/ast-guard/src/presets/permissive.preset.ts (1)
45-88: Permissive preset composition matches documented behaviorThe preset wiring is coherent: always blocking
eval, warning on unreachable code, and only adding identifier / required-call / argument rules when corresponding options are non-empty. Omitting loop/async restrictions aligns with the “permissive” description.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
libs/ast-guard/src/rules/unreachable-code.rule.ts (1)
54-80: LGTM: Terminal statement detection is correctly implemented.The logic properly handles various terminal statement types:
- Return/throw are always terminal
- Break/continue correctly identify unreachable code within their block
- IfStatement requires both branches to be terminal
- BlockStatement's
some()check is appropriate (any terminal statement means the block terminates from the parent's perspective)Optional: Consider adding stronger TypeScript types to replace
any:import type { Node } from 'acorn'; // Then use Node type instead of any where applicable private isTerminalStatement(node: Node): boolean { // ... }This would improve type safety and IDE support, though acorn's type definitions may require type assertions for specific node types.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
libs/ast-guard/src/rules/unreachable-code.rule.ts(1 hunks)libs/ast-guard/src/validator.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- libs/ast-guard/src/validator.ts
🧰 Additional context used
📓 Path-based instructions (1)
libs/**
⚙️ CodeRabbit configuration file
libs/**: Contains publishable SDK libraries. Review for API correctness, breaking changes, and consistency with docs. When public APIs change, ensure there is a matching docs/draft/docs/** update (not direct edits under docs/docs/**).
Files:
libs/ast-guard/src/rules/unreachable-code.rule.ts
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build
🔇 Additional comments (2)
libs/ast-guard/src/rules/unreachable-code.rule.ts (2)
12-16: LGTM: Well-defined rule metadata.The rule identity and defaults are appropriate. WARNING severity is a good default for unreachable code, and enabling by default helps catch common issues.
18-49: LGTM: Core detection logic is sound.The validation method correctly identifies terminal statements and reports subsequent unreachable code. Both past review concerns have been addressed:
- No duplicate reporting (removed extra ReturnStatement/ThrowStatement visitors)
- Config-based severity override now works (no explicit severity in report call)
Summary by CodeRabbit
New Features
Documentation
Tests
Chores
✏️ Tip: You can customize this high-level summary in your review settings.