feat: implement security-level-aware globals and enhance sandboxing for defense-in-depth #24
Conversation
…or defense-in-depth
📝 WalkthroughWalkthroughIntroduces a security-level model (STRICT, SECURE, STANDARD, PERMISSIVE) that centralizes allowed globals and sandbox behavior across ast-guard and enclave-vm; propagates the selected level into worker serialization, sandbox bootstrap, validators, docs, and tests. Changes
Sequence Diagram(s)sequenceDiagram
participant User as User Code
participant Enclave as Enclave (main)
participant Validator as AST Validator
participant WPA as Worker Pool Adapter
participant WScript as Worker Script (worker)
participant Sandbox as Sandbox Runtime
User->>Enclave: createEnclave({ securityLevel })
activate Enclave
Enclave->>Validator: init(securityLevel)
Validator->>Validator: allowed = getAgentScriptGlobals(securityLevel)
Enclave->>WPA: startWorker(serializedConfig{..., securityLevel})
deactivate Enclave
WPA->>WScript: launch(serializedConfig)
activate WScript
WScript->>WScript: allowed = getAllowedGlobalsForSecurityLevel(securityLevel)
WScript->>WScript: for each name in allowed: value = getGlobalValue(name)
WScript->>Sandbox: populateSandbox(with permitted globals)
deactivate WScript
User->>Sandbox: execute(userCode)
activate Sandbox
Sandbox->>Validator: validate AST against allowed globals
Validator-->>Sandbox: allow / block
Sandbox-->>User: result or error
deactivate Sandbox
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 📜 Recent review detailsConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ 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). (2)
🔇 Additional comments (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: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
libs/enclave-vm/src/double-vm/double-vm-wrapper.ts (1)
206-217: Clarify: host_vm_module is nullified, not deleted.The property is not actually deleted after use. Instead,
parent-vm-bootstrap.ts(lines 1073-1079) setsvm.createContextandvm.Scripttonullbefore user code runs. The comment in the code explicitly states: "We null out the vm object's methods since we can't delete globals in strict mode."Making
__host_vm_module__configurable=true remains a justified defense-in-depth decision because:
- The AST validator blocks access to
__host_vm_module__as an unknown global (first layer)- If AST validation is bypassed, the vm methods are nullified before user code executes (second layer)
- Even if malicious code deletes or redefines
__host_vm_module__, it runs after the inner VM is already createdHowever, the test comment (line 1468) claiming deletion occurs is inaccurate and should be corrected to reflect that nullification, not deletion, is the security mechanism.
libs/enclave-vm/src/__tests__/enclave.io-flood.spec.ts (1)
302-322: Test ATK-IOFL-13 uses STANDARD but calls console.log.This test will fail at AST validation because
consoleis not allowed in STANDARD mode. The test expects to check circular reference handling, but the code won't reach the console call.Either:
- Change to
securityLevel: 'PERMISSIVE'to actually test console circular reference behavior, or- Remove the console call and test circular references through a different mechanism (e.g., return value)
🔧 Proposed fix
it('ATK-IOFL-13: should handle circular references gracefully', async () => { const enclave = new Enclave({ - securityLevel: 'STANDARD', + securityLevel: 'PERMISSIVE', maxConsoleOutputBytes: 1024, });
🤖 Fix all issues with AI agents
In @package.json:
- Around line 24-30: Upgrade to Nx 22 requires running the official migration
and updating code to the new plugin/config APIs: run npx nx@latest migrate
latest and then nx migrate --run-migrations, remove any NX_DISABLE_DB usage and
enable DB-backed caching, add the required conventionalCommitsConfig to your
release config, update any custom plugin code using CreateNodes V1 to
createNodesV2, adjust TypeScript plugin usage around useLegacyTypescriptPlugin
(explicitly set it if you rely on legacy behavior), remove/replace uses of
deleted generator options (deleteOutputPath, sassImplementation, simpleName),
stop using experimental JS inlining options for @nx/js:tsc and @nx/js:swc, and
re-test the full suite and CI while reviewing self-hosted cache mitigations for
the CREEP advisory if you use bucket-based caches.
🧹 Nitpick comments (4)
libs/enclave-vm/src/adapters/worker-pool/protocol.ts (1)
48-72: Consider adding Zod validation schema for SerializedConfig.While
ExecuteMessage(which containsSerializedConfig) has a validation schema,SerializedConfigitself lacks one. Adding a dedicated schema would provide defense-in-depth validation and make it easier to validate the config independently.Example schema structure
export const serializedConfigSchema = z.object({ timeout: z.number().nonnegative(), maxIterations: z.number().nonnegative().int(), maxToolCalls: z.number().nonnegative().int(), maxConsoleOutputBytes: z.number().nonnegative(), maxConsoleCalls: z.number().nonnegative(), sanitizeStackTraces: z.boolean(), maxSanitizeDepth: z.number().nonnegative().int(), maxSanitizeProperties: z.number().nonnegative().int(), globals: z.record(z.string(), z.unknown()).optional(), securityLevel: z.enum(['STRICT', 'SECURE', 'STANDARD', 'PERMISSIVE']), }).strict();libs/enclave-vm/src/adapters/worker-pool/worker-script.ts (1)
357-413: Extract shared security-level globals or add verification test to prevent drift.The globals lists in
getAllowedGlobalsForSecurityLevelcorrectly mirrorast-guard'sgetAgentScriptGlobalsfor defense-in-depth. However, maintaining identical lists in two separate files risks drift if one is updated without the other.Current verification shows they match exactly across all four security levels (STRICT, SECURE, STANDARD, PERMISSIVE), but there is no automated check to catch future divergence. Consider:
- Extracting shared globals definitions to a common package, or
- Adding a test that verifies
getAllowedGlobalsForSecurityLeveloutput matchesgetAgentScriptGlobalsfor each security levelpackage.json (1)
1-68: Consider separating dependency updates from feature implementation.The PR title indicates "feat: implement security-level-aware globals and enhance sandboxing" but this file only contains dependency upgrades (Nx 21→22, ESLint, Jest, TypeScript tooling). The branch name "update-deps" more accurately reflects these changes.
Mixing infrastructure updates with feature implementation in a single PR can:
- Complicate code review and testing
- Make it harder to identify the root cause if issues arise
- Complicate rollback scenarios if either the feature or dependencies need to be reverted
Consider splitting this PR into two:
- A separate PR for dependency updates with thorough testing
- The current PR focused solely on the security-level implementation
This separation provides:
- Clearer change history and easier bisecting
- Independent testing and validation of dependency compatibility
- Ability to merge/revert changes independently based on stability
libs/enclave-vm/src/double-vm/parent-vm-bootstrap.ts (1)
863-895: Consider freezing RegExp.prototype for consistency.While reviewing the prototype freezing logic, I noticed that
RegExp.prototypeis not frozen in either the parent VM (lines 863-875) or inner VM (lines 880-892), even thoughRegExpis included in the inner intrinsics and safe globals.For consistency with other constructors and defense-in-depth against prototype pollution, consider freezing
RegExp.prototypein both contexts.🔒 Proposed enhancement to freeze RegExp.prototype
For parent VM (add after line 869):
Object.freeze(Date.prototype); +Object.freeze(RegExp.prototype); Object.freeze(Error.prototype);For inner VM (add after line 886):
'Object.freeze(Date.prototype);' + +'Object.freeze(RegExp.prototype);' + 'Object.freeze(Error.prototype);' +
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (21)
.verdaccio/config.ymllibs/ast-guard/README.mdlibs/ast-guard/docs/AGENTSCRIPT.mdlibs/ast-guard/src/agentscript-transformer.tslibs/ast-guard/src/index.tslibs/ast-guard/src/presets/agentscript.preset.tslibs/ast-guard/src/presets/index.tslibs/enclave-vm/README.mdlibs/enclave-vm/src/__tests__/double-vm.security.spec.tslibs/enclave-vm/src/__tests__/enclave.io-flood.spec.tslibs/enclave-vm/src/__tests__/enclave.security.spec.tslibs/enclave-vm/src/__tests__/literal-constructor-escape.spec.tslibs/enclave-vm/src/__tests__/vm-adapter.spec.tslibs/enclave-vm/src/__tests__/worker-pool-adapter.spec.tslibs/enclave-vm/src/adapters/worker-pool/protocol.tslibs/enclave-vm/src/adapters/worker-pool/worker-pool-adapter.tslibs/enclave-vm/src/adapters/worker-pool/worker-script.tslibs/enclave-vm/src/double-vm/double-vm-wrapper.tslibs/enclave-vm/src/double-vm/parent-vm-bootstrap.tslibs/enclave-vm/src/enclave.tspackage.json
💤 Files with no reviewable changes (1)
- .verdaccio/config.yml
🧰 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/enclave-vm/src/__tests__/literal-constructor-escape.spec.tslibs/enclave-vm/src/__tests__/vm-adapter.spec.tslibs/enclave-vm/src/double-vm/parent-vm-bootstrap.tslibs/enclave-vm/src/__tests__/double-vm.security.spec.tslibs/ast-guard/docs/AGENTSCRIPT.mdlibs/ast-guard/src/agentscript-transformer.tslibs/enclave-vm/src/enclave.tslibs/ast-guard/src/index.tslibs/enclave-vm/src/__tests__/enclave.security.spec.tslibs/ast-guard/README.mdlibs/enclave-vm/src/__tests__/enclave.io-flood.spec.tslibs/enclave-vm/src/adapters/worker-pool/protocol.tslibs/enclave-vm/src/__tests__/worker-pool-adapter.spec.tslibs/ast-guard/src/presets/agentscript.preset.tslibs/enclave-vm/src/adapters/worker-pool/worker-pool-adapter.tslibs/enclave-vm/README.mdlibs/ast-guard/src/presets/index.tslibs/enclave-vm/src/adapters/worker-pool/worker-script.tslibs/enclave-vm/src/double-vm/double-vm-wrapper.ts
🧬 Code graph analysis (3)
libs/enclave-vm/src/__tests__/double-vm.security.spec.ts (1)
libs/enclave-vm/src/enclave.ts (1)
Enclave(158-726)
libs/enclave-vm/src/enclave.ts (1)
libs/ast-guard/src/presets/agentscript.preset.ts (2)
getAgentScriptGlobals(118-130)createAgentScriptPreset(304-555)
libs/enclave-vm/src/__tests__/enclave.security.spec.ts (1)
libs/enclave-vm/src/enclave.ts (1)
Enclave(158-726)
🔇 Additional comments (35)
libs/enclave-vm/src/__tests__/literal-constructor-escape.spec.ts (3)
248-277: Well-structured memory exhaustion tests for Single VM mode.The test coverage is thorough with symmetric positive and negative cases. The explicit
doubleVm: { enabled: false }configuration clearly indicates the testing target.
279-341: The Double VM Mode tests correctly rely on defaults; explicit configuration would obscure the default behavior.Double VM is indeed enabled by default (
DEFAULT_DOUBLE_VM_CONFIG.enabled: trueinlibs/enclave-vm/src/types.ts), and the test asymmetry is intentional. The Single VM tests explicitly disable double VM with{ enabled: false }to test non-default behavior, while the Double VM tests correctly rely on the implicit default. Making the Double VM tests explicit withdoubleVm: { enabled: true }would contradict standard testing practices and obscure which behavior is the default. The explanatory comment at lines 280–282 about memory protection via inner context intrinsics is helpful and accurate.
347-356: No action needed—test is correctly designed.This test intentionally uses a very short 100ms timeout to verify the timeout mechanism works under CPU-intensive BigInt operations. The 100ms timeout is not a potential point of failure; it's the mechanism being tested. The platform-dependent behavior is already acknowledged in the code comment, and the short timeout is specifically designed to trigger reliably across platforms. A complementary positive test case confirms normal BigInt operations work without timeout.
libs/enclave-vm/README.md (1)
109-114: LGTM! Excellent security documentation.The security levels table and per-level globals documentation are clear, comprehensive, and well-structured. The defense-in-depth approach is well-explained with both allowed and blocked globals explicitly documented.
Also applies to: 127-177
libs/enclave-vm/src/double-vm/double-vm-wrapper.ts (1)
61-64: LGTM! Constructor formatting improved.The multi-line parameter format enhances readability.
libs/ast-guard/docs/AGENTSCRIPT.md (1)
36-78: LGTM! Comprehensive security model documentation.The security levels, allowed globals table, blocked globals, and blocked language features sections are well-structured and provide clear guidance. The per-level approach is clearly explained and aligns well with the defense-in-depth model.
Also applies to: 99-128
libs/ast-guard/src/agentscript-transformer.ts (1)
165-169: LGTM! Safe URI functions added to whitelist.Adding
encodeURI,decodeURI,encodeURIComponent, anddecodeURIComponentto the whitelist is appropriate. These are standard JavaScript built-in functions for safe string manipulation and URL encoding/decoding. Per the documentation, they are available in SECURE, STANDARD, and PERMISSIVE security levels.libs/enclave-vm/src/adapters/worker-pool/protocol.ts (1)
67-71: All construction sites have been updated. ThesecurityLevelfield is required in theSerializedConfiginterface and is properly included in the only construction site found (worker-pool-adapter.ts:344).libs/enclave-vm/src/adapters/worker-pool/worker-pool-adapter.ts (1)
343-357: LGTM: Security level correctly serialized for worker sandbox.The addition of
securityLevelto the serialized configuration is well-documented and aligns with the defense-in-depth model. The field correctly passes through from the adapter's constructor parameter to the worker sandbox configuration.libs/enclave-vm/src/__tests__/vm-adapter.spec.ts (1)
248-327: LGTM: Console tests correctly updated for PERMISSIVE security level.All console rate limiting tests now properly specify
securityLevel: 'PERMISSIVE', which correctly aligns with the new security model where console access requires the PERMISSIVE level. The clarifying comment on line 249 helps future maintainers understand this requirement.libs/enclave-vm/src/__tests__/enclave.security.spec.ts (2)
1177-1195: LGTM: Console test correctly uses PERMISSIVE security level.The test now properly configures
securityLevel: 'PERMISSIVE'for console access, with a clear comment explaining the requirement. This aligns with the security-level aware globals model.
1197-1218: LGTM: Console rate limit test correctly configured.The test properly uses
securityLevel: 'PERMISSIVE'to enable console access before verifying rate limiting enforcement. The comment clarifies the security level requirement.libs/enclave-vm/src/__tests__/double-vm.security.spec.ts (1)
1462-1570: Excellent: Comprehensive VM escape prevention test coverage.This new test suite provides thorough coverage of defense-in-depth mechanisms against VM escape attacks:
- Host identifier blocking: Tests verify AST validation blocks access to
__host_vm_module__,__host_callTool__,__host_stats__, and__host_config__as unknown globals- Constructor chain escapes: Tests verify obfuscated constructor access attempts are blocked
- VM module blocking: Tests verify
vmandScriptglobals are blocked at AST level- Whitelist verification: Tests confirm allowed globals (Array, String, Math, JSON, callTool) work as expected
The tests correctly demonstrate that the AST validator provides the first layer of defense, with clear comments explaining the defense-in-depth model.
libs/ast-guard/src/index.ts (1)
93-102: LGTM: Public API expanded with security-level aware globals.The new exports appropriately expose the security-level aware globals infrastructure:
- Per-level global constants (STRICT, SECURE, STANDARD, PERMISSIVE)
- Helper function
getAgentScriptGlobalsfor dynamic resolutionSecurityLeveltype for type-safe configurationThis is an additive, non-breaking change to the public API. Based on the coding guidelines context, the corresponding documentation updates in libs/ast-guard/README.md and docs/AGENTSCRIPT.md properly document these new exports.
libs/enclave-vm/src/enclave.ts (2)
21-21: LGTM! Import aligns with new ast-guard exports.The new
getAgentScriptGlobalsimport enables dynamic, security-level-aware global whitelisting.
631-655: Well-structured security-level-aware global derivation.The layered approach correctly combines:
- Base globals from
getAgentScriptGlobals(securityLevel)- Enclave-specific runtime globals (
parallel,__safe_*)- Custom user-provided globals
This ensures AST validation aligns with runtime sandbox restrictions for defense-in-depth.
libs/enclave-vm/src/adapters/worker-pool/worker-script.ts (3)
286-355: Comprehensive global value mapping for sandbox injection.The switch statement correctly handles:
- Runtime-injected safe functions (
__safe_callTool,__safe_forOf, etc.)- Core built-ins (
Math,JSON,Object, etc.)- Standard globals and utility functions
Minor note:
Booleanis exposed by ast-guard's globals but not mapped here - this is fine since it's not in the worker's allowed list, but worth tracking ifBooleanis added to future security levels.
415-447: LGTM! Security-level-aware sandbox creation.The implementation correctly:
- Uses a null-prototype object to prevent prototype pollution
- Only exposes globals that match the security level
- Handles the
'undefined'edge case properly (line 431)- Sanitizes custom globals before injection
643-671: Good enhancement to handle diverse error types.The changes properly handle:
- Thrown strings from iteration limit checks in transformed loops
- Error code preservation for structured error handling
- Configurable stack trace sanitization
libs/ast-guard/src/presets/index.ts (1)
21-32: LGTM! Clean API surface expansion.The re-exports correctly expose the new security-level-aware globals and types for public consumption. The
AGENTSCRIPT_BASE_GLOBALSalias maintains backward compatibility.libs/enclave-vm/src/__tests__/enclave.io-flood.spec.ts (1)
35-120: LGTM! Tests correctly updated for PERMISSIVE-only console.The I/O flood tests properly use
securityLevel: 'PERMISSIVE'since console is now restricted to that level. The tests maintain coverage for output size and call count limits.libs/ast-guard/README.md (2)
381-403: Excellent documentation of the security-level globals model.The new section clearly documents:
- Per-level global availability in an easy-to-scan table
- The progression from STRICT (minimal) to PERMISSIVE (includes console)
- The comprehensive list of always-blocked globals
This aligns well with the coding guidelines requirement for matching docs updates when public APIs change.
423-434: LGTM! Options table correctly documents securityLevel.The updated table accurately reflects the new
securityLeveloption with its default value of'STANDARD'and the precedence behavior ofallowedGlobals.libs/enclave-vm/src/__tests__/worker-pool-adapter.spec.ts (3)
280-323: LGTM! Iteration limit tests with clear explanations.The comments correctly explain:
- Loop transformation behavior (for loops →
__safe_for)- Why PERMISSIVE mode may not transform loops
- That
for-ofloops track iteration count via__safe_forOfThe test assertions are appropriate for the security model.
404-456: Good security test updates - AST validation provides stronger guarantees.The tests correctly verify that dangerous globals (
parentPort,process,require) are blocked at AST validation rather than just returningundefinedat runtime. This is the intended defense-in-depth behavior.
474-578: Comprehensive security level test coverage.The new test suite effectively validates:
- Console access restricted to PERMISSIVE only
- Utility functions (
parseInt,encodeURI,isFinite) available in STANDARD/SECURE but blocked in STRICT- Defense-in-depth behavior where AST validation and sandbox both enforce the same restrictions
The tests properly clean up resources with
enclave.dispose().libs/ast-guard/src/presets/agentscript.preset.ts (3)
21-130: Well-designed security-level-aware globals architecture.The implementation provides:
- Clear layered progression (STRICT → SECURE → STANDARD → PERMISSIVE)
- Immutable constant exports with
as constfor type safety- Sensible default fallback to STRICT for unknown security levels
- Helpful JSDoc documentation explaining each level
The
AGENTSCRIPT_BASE_GLOBALSlegacy alias maintains backward compatibility.
135-153: LGTM! Clear interface extension with documented precedence.The
securityLeveloption is well-documented with:
- Default value clearly stated (
'STANDARD')- Explicit precedence rules (explicit
allowedGlobalsoverridessecurityLevel)- Flexible typing (
SecurityLevel | string) for extensibility
304-335: LGTM! Correct globals derivation with clear priority.The implementation correctly handles:
- Explicit
allowedGlobals(highest priority)- Security-level-derived globals via
getAgentScriptGlobals- Default to
'STANDARD'levelThe spread on line 330 is necessary to satisfy
UnknownGlobalRule's mutable array requirement while preserving thereadonlynature of the source constants.package.json (3)
37-38: Both package versions are valid and secure. ESLint 9.39.2 and eslint-config-prettier 10.1.8 are the latest available versions, neither is deprecated, and no security vulnerabilities have been detected. Dependency requirements are satisfied.
40-43: No issues found. All Jest packages (jest@30.2.0, jest-environment-node@30.2.0, jest-util@30.2.0) and jsonc-eslint-parser@2.4.2 are valid versions on NPM with no known security vulnerabilities.
45-51: All TypeScript tooling packages are valid and secure.All specified package versions exist on the NPM registry and have no reported direct security advisories:
- nx@22.3.3, prettier@3.7.4, ts-jest@29.4.6, ts-node@10.9.2, tslib@2.8.1, typescript@5.9.3, and typescript-eslint@8.52.0 are confirmed stable releases with no active CVEs.
libs/enclave-vm/src/double-vm/parent-vm-bootstrap.ts (3)
972-994: Excellent implementation of inner intrinsics retrieval.This change correctly addresses a critical security gap where using parent VM constructors would bypass memory-safe prototype patches. By retrieving the inner context's intrinsic constructors after memory patches have been applied, you ensure that
new Array(),new String(), etc. in user code will use the patched prototypes with memory limits enforced.The timing is correct: this happens after memory patching (lines 788-847) and prototype freezing (lines 877-895), ensuring the retrieved constructors reference the hardened prototypes.
997-1011: LGTM: Safe globals correctly use inner intrinsics.This change ensures that all constructor-based object creation in user code (e.g.,
new Array(),new String()) uses the inner VM's constructors with patched prototypes, maintaining the memory-safety guarantees established during the patching phase.The decision to continue using
SafeObjectfor theObjectconstructor is correct, as it provides a custom wrapper that blocks dangerous static methods (defineProperty, etc.) while still allowing safe operations.
1072-1079: Strong defense-in-depth hardening by disabling vm module.This security measure prevents exploitation of the vm module even if user code finds a way to escape from the inner VM to the parent VM. Nulling
vm.createContextandvm.Scriptafter preparing the user code script (line 1071) but before execution (line 1082) is the correct timing.The try-catch wrapper appropriately handles the edge case where these properties might be non-writable/non-configurable, which would indicate they're already protected through other means.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In @.husky/pre-commit:
- Around line 3-9: The pre-commit hook currently masks errors and mishandles
filenames: stop suppressing failures and make the changed-file pipeline
null-safe. First, verify the main branch exists (e.g., git show-ref --quiet
refs/heads/main) and abort with a clear message if not; then produce
null-terminated paths from git diff (use --name-only -z) into CHANGED_FILES, use
grep -z with an appropriate exit-code check to detect matches without swallowing
errors, and pass the null-terminated list to xargs -0 -r npx prettier --check so
filenames with spaces/special chars are handled and the formatter is only
skipped intentionally with a clear log. Ensure you remove 2>/dev/null and ||
true so real errors propagate and add explicit messages when checks are skipped.
In @package.json:
- Around line 41-55: The upgraded dev deps in package.json introduce breaking
changes: migrate ESLint to the new flat config or restore legacy behavior
(create eslint.config.js or update the "lint" npm script to pass a specific
config and adjust any .eslintrc usage), update or rename Jest CLI flags and
validate jest.config.js (replace --testPathPattern usages with
--testPathPatterns and run snapshot updates), remove or replace deprecated rules
from typescript-eslint config (e.g., remove prefer-ts-expect-error,
no-var-requires, no-throw-literal from your ESLint rules), and update the
package.json "engines" (or CI node version) to a Node.js version compatible with
typescript-eslint v8 (>=18.18.0) and Jest v30; run CI locally to ensure tests
and linting pass and consult each tool’s migration guide for any further config
tweaks.
🧹 Nitpick comments (2)
libs/ast-guard/src/errors.ts (1)
5-37: Apply consistent formatting to all error constructors.While the multiline formatting with trailing commas is a good practice, only three of the six error constructors in this file have been reformatted. Consider applying the same formatting style to
ConfigurationError,RuleNotFoundError, andInvalidSourceErrorfor consistency.♻️ Proposed formatting for remaining constructors
export class ConfigurationError extends AstGuardError { - constructor(message: string) { + constructor( + message: string, + ) { super(message, 'CONFIGURATION_ERROR'); this.name = 'ConfigurationError'; Object.setPrototypeOf(this, ConfigurationError.prototype); } }export class RuleNotFoundError extends AstGuardError { - constructor(public readonly ruleName: string) { + constructor( + public readonly ruleName: string, + ) { super(`Rule not found: ${ruleName}`, 'RULE_NOT_FOUND'); this.name = 'RuleNotFoundError'; Object.setPrototypeOf(this, RuleNotFoundError.prototype); } }export class InvalidSourceError extends AstGuardError { - constructor(message: string) { + constructor( + message: string, + ) { super(message, 'INVALID_SOURCE'); this.name = 'InvalidSourceError'; Object.setPrototypeOf(this, InvalidSourceError.prototype); } }package.json (1)
12-13: Consider safer xargs handling for filenames with special characters.The format scripts use
xargswithout null-terminator handling, which could fail on filenames containing spaces or special characters. While most projects avoid such filenames, adding-print0and-0flags provides more robust handling.♻️ Suggested improvement
- "format": "git diff --name-only --diff-filter=ACMR main... | grep -E '\\.(js|jsx|ts|tsx|json|css|scss|md|html|yml|yaml)$' | xargs prettier --write", - "format:check": "git diff --name-only --diff-filter=ACMR main... | grep -E '\\.(js|jsx|ts|tsx|json|css|scss|md|html|yml|yaml)$' | xargs prettier --check", + "format": "git diff --name-only --diff-filter=ACMR -z main... | grep -zE '\\.(js|jsx|ts|tsx|json|css|scss|md|html|yml|yaml)$' | xargs -0 prettier --write", + "format:check": "git diff --name-only --diff-filter=ACMR -z main... | grep -zE '\\.(js|jsx|ts|tsx|json|css|scss|md|html|yml|yaml)$' | xargs -0 prettier --check",Note: This same pattern is duplicated in
.husky/pre-commit(line 4), so the same improvement should be applied there as well.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (17)
.github/RELEASE_WORKFLOW.md.github/codex/prompts/analyze-release.md.github/codex/prompts/analyze-version-bump.md.github/codex/prompts/update-docs.md.husky/pre-commitSECURITY.mdlibs/ast-guard/CHANGELOG.mdlibs/ast-guard/src/errors.tslibs/ast-guard/src/rules/no-computed-destructuring.rule.tslibs/ast-guard/src/rules/no-global-access.rule.tslibs/ast-guard/src/validator.tslibs/enclave-vm/SECURITY-AUDIT.mdlibs/enclave-vm/src/scoring/feature-extractor.tslibs/enclave-vm/src/sidecar/reference-resolver.tslibs/vectoriadb/src/errors.tspackage.jsonscripts/nx-release.mjs
💤 Files with no reviewable changes (6)
- .github/codex/prompts/analyze-release.md
- libs/ast-guard/CHANGELOG.md
- libs/enclave-vm/SECURITY-AUDIT.md
- .github/codex/prompts/update-docs.md
- .github/codex/prompts/analyze-version-bump.md
- .github/RELEASE_WORKFLOW.md
✅ Files skipped from review due to trivial changes (6)
- libs/ast-guard/src/rules/no-global-access.rule.ts
- libs/ast-guard/src/rules/no-computed-destructuring.rule.ts
- libs/enclave-vm/src/sidecar/reference-resolver.ts
- SECURITY.md
- libs/enclave-vm/src/scoring/feature-extractor.ts
- libs/vectoriadb/src/errors.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/errors.tslibs/ast-guard/src/validator.ts
🧬 Code graph analysis (1)
libs/ast-guard/src/validator.ts (1)
libs/ast-guard/src/index.ts (1)
ValidationSeverity(33-33)
⏰ 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). (3)
- GitHub Check: Performance
- GitHub Check: Test
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (2)
scripts/nx-release.mjs (1)
107-109: LGTM! Formatting improvement enhances readability.The multi-line formatting of the warning message improves code readability without affecting functionality.
libs/ast-guard/src/validator.ts (1)
114-119: LGTM! Formatting improvement enhances readability.The re-indentation of the nested ternary expression improves code readability without altering the mapping logic.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In @.husky/pre-commit:
- Line 22: The pre-commit hook uses `echo "$CHANGED_FILES"` which can mangle
null-terminated lists; update the pipeline in the `.husky/pre-commit` hook so
the null bytes are preserved by replacing `echo "$CHANGED_FILES"` with `printf
'%s' "$CHANGED_FILES"` or, better, avoid the variable entirely and pipe `git
diff --name-only -z --diff-filter=ACMR main...` directly into the existing `grep
-zE '...` and `xargs -0 -r npx prettier --check` sequence so `grep -zE` and
`xargs -0` see true null-terminated input.
In @package.json:
- Around line 12-13: The package.json "format" and "format:check" scripts assume
a "main" branch exists and will fail with confusing git errors; update those two
scripts ("format" and "format:check") to first check for the existence of the
main branch and fall back to a safe alternative (for example use the existing
"format:all" behavior or use HEAD-based diff) so the command succeeds on repos
without main, or document this requirement in README; adjust the script commands
to perform that branch-existence check and fallback logic for both "format" and
"format:check".
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
.husky/pre-commitpackage.json
⏰ 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: Test
🔇 Additional comments (3)
.husky/pre-commit (2)
5-11: Good handling of missingmainbranch.The explicit check for
mainbranch existence with clear user messaging addresses the previous concern about silent error masking. The fallback tolint-stagedis appropriate.
23-27: LGTM on exit code handling.Capturing and propagating the prettier exit code ensures CI fails appropriately when formatting issues are detected.
package.json (1)
5-7: Engine constraint is compatible with upgraded dependencies.The
node >= 22.0.0requirement satisfies Jest 30 and typescript-eslint 8 minimums.
…r unsupported branches
Summary by CodeRabbit
New Features
Documentation
Tests
Chores
✏️ Tip: You can customize this high-level summary in your review settings.