Add audit export, policy enforcement, and preflight config #12
Conversation
📝 WalkthroughWalkthroughThe changes add security auditing capabilities and configurable preflight checks to the Ignite system. A new Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
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[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
docs/api.md (2)
97-103: Clarify whether--audit-outputrequires--auditThe option description doesn’t specify behavior when
--auditis omitted. Please clarify whether it implies--auditor is ignored/errored without it.✏️ Suggested wording
-| `--audit-output <file>` | - | Write security audit to a JSON file | +| `--audit-output <file>` | - | Write security audit to a JSON file (requires `--audit`) |
125-132: Audit sandboxing is policy‑driven—document defaultsWith policy-based enforcement, network/filesystem/process rules can be overridden by
ignite.policy.yaml. The bullet list should be framed as defaults.✏️ Suggested wording
-When running with `--audit`, the service runs in a hardened sandbox: +When running with `--audit`, the service runs in a hardened sandbox by default (policy overrides may apply): -- Network completely disabled (`--network=none`) -- Read-only root filesystem (`--read-only`) -- Writable `/tmp` only (`--tmpfs /tmp`) -- All Linux capabilities dropped (`--cap-drop=ALL`) -- No privilege escalation (`--security-opt=no-new-privileges`) +- Network disabled (`--network=none`) +- Read-only root filesystem (`--read-only`) +- Writable `/tmp` only (`--tmpfs /tmp`) +- All Linux capabilities dropped (`--cap-drop=ALL`) +- No privilege escalation (`--security-opt=no-new-privileges`)
🤖 Fix all issues with AI agents
In `@packages/cli/src/commands/run.ts`:
- Around line 62-66: Wrap the audit file write in a try/catch to handle
filesystem errors when calling writeFile with the resolved outputPath (used when
options.auditOutput && audit), so failures (permission, disk, invalid path) are
caught; on error log a clear message via logger.error including the outputPath
and error details and avoid letting the unhandled rejection propagate (either
return a non-fatal error status or rethrow after logging depending on command
semantics). Ensure the success log (logger.success(`Audit saved to
${outputPath}`)) remains inside the try block and reference the existing symbols
options.auditOutput, audit, outputPath, writeFile, and logger in your changes.
In `@packages/core/src/preflight/analyze-memory.ts`:
- Around line 55-70: Add a validation in the load-service preflight parsing to
ensure preflight.dependencies.warnCount > preflight.dependencies.infoCount:
after extracting dependencyConfig and the numeric warnCount and infoCount (the
same way memory/image/timeout validations do), check both are numbers and push
an error message via errors.push('preflight.dependencies.warnCount must be
greater than preflight.dependencies.infoCount') when warnCount <= infoCount;
reference the dependencyConfig/warnCount/infoCount variables and the errors
array used in load-service.ts so the preflight tiering logic remains consistent
with other sections.
In `@packages/core/src/service/load-service.ts`:
- Around line 139-142: Add a cross-field validation after the existing call to
validatePreflightSection for pf['dependencies']: when both
pf['dependencies'].infoCount and pf['dependencies'].warnCount are defined, check
that infoCount < warnCount and push an appropriate error into the errors array
(use the same error key/namespace 'preflight.dependencies' or
'preflight.dependencies.infoCount' to match existing validations). Modify the
block around validatePreflightSection(pf['dependencies'],
'preflight.dependencies', errors, { ... }) to perform this additional check and
append a descriptive error if the ordering is violated.
🧹 Nitpick comments (2)
packages/core/src/preflight/analyze-image.ts (1)
7-10: Consider reusing the type from@ignite/shared.The
ImagePreflightConfiginterface duplicates the shape ofPreflightConfig['image']from@ignite/shared. Consider importing and reusing the shared type to maintain a single source of truth and avoid drift between definitions.♻️ Suggested refactor
-export interface ImagePreflightConfig { - warnMb?: number; - failMb?: number; -} +import type { PreflightConfig } from '@ignite/shared'; + +export type ImagePreflightConfig = PreflightConfig['image'];docs/api.md (1)
575-590: AddskipBuildto request example for parityThe table documents
skipBuild, but the example JSON omits it. Adding it would reduce ambiguity.✏️ Example update
{ "input": { "data": [1, 2, 3], "operation": "sum" }, "skipPreflight": false, + "skipBuild": false, "audit": true }
| if (options.auditOutput && audit) { | ||
| const outputPath = resolve(process.cwd(), options.auditOutput); | ||
| await writeFile(outputPath, JSON.stringify(audit, null, 2)); | ||
| logger.success(`Audit saved to ${outputPath}`); | ||
| } |
There was a problem hiding this comment.
Add error handling for audit file write operation.
The writeFile call can fail (e.g., permission denied, disk full, invalid path) but errors aren't caught. This could cause the command to fail with an unhandled rejection after successful execution.
🛡️ Proposed fix
if (options.auditOutput && audit) {
const outputPath = resolve(process.cwd(), options.auditOutput);
- await writeFile(outputPath, JSON.stringify(audit, null, 2));
- logger.success(`Audit saved to ${outputPath}`);
+ try {
+ await writeFile(outputPath, JSON.stringify(audit, null, 2));
+ logger.success(`Audit saved to ${outputPath}`);
+ } catch (writeErr) {
+ logger.warn(`Failed to save audit to ${outputPath}: ${(writeErr as Error).message}`);
+ }
}📝 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.
| if (options.auditOutput && audit) { | |
| const outputPath = resolve(process.cwd(), options.auditOutput); | |
| await writeFile(outputPath, JSON.stringify(audit, null, 2)); | |
| logger.success(`Audit saved to ${outputPath}`); | |
| } | |
| if (options.auditOutput && audit) { | |
| const outputPath = resolve(process.cwd(), options.auditOutput); | |
| try { | |
| await writeFile(outputPath, JSON.stringify(audit, null, 2)); | |
| logger.success(`Audit saved to ${outputPath}`); | |
| } catch (writeErr) { | |
| logger.warn(`Failed to save audit to ${outputPath}: ${(writeErr as Error).message}`); | |
| } | |
| } |
🤖 Prompt for AI Agents
In `@packages/cli/src/commands/run.ts` around lines 62 - 66, Wrap the audit file
write in a try/catch to handle filesystem errors when calling writeFile with the
resolved outputPath (used when options.auditOutput && audit), so failures
(permission, disk, invalid path) are caught; on error log a clear message via
logger.error including the outputPath and error details and avoid letting the
unhandled rejection propagate (either return a non-fatal error status or rethrow
after logging depending on command semantics). Ensure the success log
(logger.success(`Audit saved to ${outputPath}`)) remains inside the try block
and reference the existing symbols options.auditOutput, audit, outputPath,
writeFile, and logger in your changes.
| const dependencyConfig = service.config.preflight?.dependencies; | ||
| const warnCount = dependencyConfig?.warnCount ?? 100; | ||
| const infoCount = dependencyConfig?.infoCount ?? 50; | ||
|
|
||
| if (depCount > 100) { | ||
| if (depCount > warnCount) { | ||
| return { | ||
| name: 'dependency-count', | ||
| status: 'warn', | ||
| message: `High dependency count (${depCount}). node_modules size: ${nodeModulesSizeMb}MB. Consider reducing dependencies for faster cold starts.`, | ||
| value: depCount, | ||
| threshold: 100, | ||
| threshold: warnCount, | ||
| }; | ||
| } | ||
|
|
||
| if (depCount > 50) { | ||
| if (depCount > infoCount) { | ||
| return { |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's examine the file mentioned in the review
cat -n packages/core/src/preflight/analyze-memory.ts | head -100Repository: dev-dami/ignite
Length of output: 3754
🏁 Script executed:
# Search for where preflight dependencies config is defined/validated
rg -n "preflight.*dependencies|warnCount|infoCount" -g '*.ts' -g '*.json' --max-count=50Repository: dev-dami/ignite
Length of output: 1169
🏁 Script executed:
# Look for config schema or type definitions related to preflight
rg -n "type.*preflight|interface.*preflight" -g '*.ts' -A 10 --max-count=30Repository: dev-dami/ignite
Length of output: 1729
🏁 Script executed:
# Find the validatePreflightSection function to see what validation is actually done
rg -n "validatePreflightSection" -g '*.ts' -A 30 | head -80Repository: dev-dami/ignite
Length of output: 6407
🏁 Script executed:
# Also check the type definitions in shared to see if there's any constraint
cat -n packages/shared/src/types.ts | grep -A 5 -B 5 "warnCount\|infoCount"Repository: dev-dami/ignite
Length of output: 356
Add validation for warnCount > infoCount to dependencies config
The other preflight sections (memory, image, timeout) validate ordering constraints—warnRatio must be less than failRatio, warnMb must be less than failMb, minMs must be less than maxMs. The dependencies validation (lines 139-142 in load-service.ts) currently only checks that both values are positive and doesn't enforce warnCount > infoCount. If a user provides infoCount ≥ warnCount, the tiering logic breaks: a dependency count above infoCount will incorrectly trigger a 'warn' status instead of 'pass'.
Add a validation check in load-service.ts after line 142 (following the pattern used for memory, image, and timeout configs):
const dependencyConfig = pf['dependencies'] as Record<string, unknown> | undefined;
const warnCount = dependencyConfig?.['warnCount'];
const infoCount = dependencyConfig?.['infoCount'];
if (typeof warnCount === 'number' && typeof infoCount === 'number') {
if (warnCount <= infoCount) {
errors.push('preflight.dependencies.warnCount must be greater than preflight.dependencies.infoCount');
}
}🤖 Prompt for AI Agents
In `@packages/core/src/preflight/analyze-memory.ts` around lines 55 - 70, Add a
validation in the load-service preflight parsing to ensure
preflight.dependencies.warnCount > preflight.dependencies.infoCount: after
extracting dependencyConfig and the numeric warnCount and infoCount (the same
way memory/image/timeout validations do), check both are numbers and push an
error message via errors.push('preflight.dependencies.warnCount must be greater
than preflight.dependencies.infoCount') when warnCount <= infoCount; reference
the dependencyConfig/warnCount/infoCount variables and the errors array used in
load-service.ts so the preflight tiering logic remains consistent with other
sections.
| validatePreflightSection(pf['dependencies'], 'preflight.dependencies', errors, { | ||
| warnCount: 'positive', | ||
| infoCount: 'positive', | ||
| }); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# First, let's examine the file around the mentioned lines
wc -l packages/core/src/service/load-service.tsRepository: dev-dami/ignite
Length of output: 104
🏁 Script executed:
#!/bin/bash
# View the relevant sections of the file
sed -n '116,210p' packages/core/src/service/load-service.tsRepository: dev-dami/ignite
Length of output: 3219
🏁 Script executed:
#!/bin/bash
# Search for how infoCount and warnCount are used across the codebase
rg -n "infoCount|warnCount" --type=ts -C 2Repository: dev-dami/ignite
Length of output: 2979
Add cross-field validation for dependencies thresholds.
The preflight.dependencies section allows both warnCount and infoCount but lacks validation for their logical ordering. The actual usage in analyze-memory.ts (lines 56-75) shows the expected relationship: infoCount triggers informational checks, warnCount triggers warnings. This requires infoCount < warnCount semantically—equivalent to the ordering validations applied to other preflight sections (e.g., warnRatio < failRatio for memory, warnMb < failMb for image).
Add a cross-field check ensuring infoCount < warnCount when both are defined, similar to the validations for other sections.
🤖 Prompt for AI Agents
In `@packages/core/src/service/load-service.ts` around lines 139 - 142, Add a
cross-field validation after the existing call to validatePreflightSection for
pf['dependencies']: when both pf['dependencies'].infoCount and
pf['dependencies'].warnCount are defined, check that infoCount < warnCount and
push an appropriate error into the errors array (use the same error
key/namespace 'preflight.dependencies' or 'preflight.dependencies.infoCount' to
match existing validations). Modify the block around
validatePreflightSection(pf['dependencies'], 'preflight.dependencies', errors, {
... }) to perform this additional check and append a descriptive error if the
ordering is violated.
Resolve conflicts: - packages/cli/src/commands/run.ts: Keep resolve() for absolute path handling - packages/core/src/service/load-service.ts: Keep cross-field validation
Summary
Testing
Summary by CodeRabbit
New Features
--audit-outputCLI option to write security audit results to a JSON fileDocumentation
Chores
✏️ Tip: You can customize this high-level summary in your review settings.