Feature/audit export preflight config #11
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
📝 WalkthroughWalkthroughIntroduces configurable preflight checks and security audit features across the ignite framework. Adds Changes
Sequence DiagramsequenceDiagram
participant User
participant CLI
participant Core as Execution Core
participant Docker
participant Policy as Policy System
User->>CLI: run --audit --audit-output audit.json
CLI->>Policy: loadPolicyFile(servicePath)
Policy-->>CLI: SecurityPolicy (or DEFAULT_POLICY)
CLI->>Core: executeService({input, audit: true, policy})
Core->>Docker: dockerRun(with securityOptions from policy)
Docker-->>Core: metrics + output
Core->>Policy: parseAuditFromOutput(metrics, policy)
Policy-->>Core: SecurityAudit
Core-->>CLI: {success, audit: SecurityAudit}
CLI->>CLI: writeFile(auditOutput, JSON.stringify(audit))
CLI-->>User: Audit written to audit.json
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 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: 2
🤖 Fix all issues with AI agents
In `@packages/cli/src/commands/run.ts`:
- Around line 62-66: Replace the path construction that uses join(process.cwd(),
options.auditOutput) so absolute auditOutput paths are preserved; update the
code that writes the audit (the block using options.auditOutput, writeFile and
logger.success) to compute the output path with resolve(process.cwd(),
options.auditOutput) (or resolve(options.auditOutput)) from node:path instead of
join, then pass that resolved path to writeFile and logger.success.
In `@packages/core/src/service/load-service.ts`:
- Around line 116-146: The preflight object validation currently checks
individual numeric fields but misses cross-field ordering rules; after the calls
to validatePreflightSection for 'preflight.memory', 'preflight.image', and
'preflight.timeout' in load-service.ts, read the parsed values from
pf['memory'], pf['image'], and pf['timeout'] and add explicit comparisons that
push errors into the errors array when ordering is wrong (e.g., if
pf['memory']?.warnRatio >= pf['memory']?.failRatio then push
"preflight.memory.warnRatio must be less than preflight.memory.failRatio"; if
pf['image']?.warnMb >= pf['image']?.failMb then push "preflight.image.warnMb
must be less than preflight.image.failMb"; if pf['timeout']?.minMs >=
pf['timeout']?.maxMs then push "preflight.timeout.minMs must be less than
preflight.timeout.maxMs"). Ensure you handle missing or non-number values
gracefully (only perform comparison when both sides are numbers).
🧹 Nitpick comments (3)
packages/core/src/execution/execute.ts (1)
78-78: Minor: Redundant conditional check.The
options.audit ? securityOptions : undefinedcheck is redundant sincesecurityOptionsis only defined whenoptions.auditis true (lines 51-54). However, this defensive approach is acceptable for clarity.♻️ Optional simplification
- security: options.audit ? securityOptions : undefined, + security: securityOptions,packages/core/src/preflight/analyze-memory.ts (1)
55-84: Consider normalizing dependency thresholds to avoid inverted configs.If
warnCountis set belowinfoCount, warnings can become unreachable or overly aggressive. A small normalization avoids surprising behavior.♻️ Suggested normalization
- const warnCount = dependencyConfig?.warnCount ?? 100; - const infoCount = dependencyConfig?.infoCount ?? 50; + const warnCountRaw = dependencyConfig?.warnCount ?? 100; + const infoCountRaw = dependencyConfig?.infoCount ?? 50; + const warnCount = Math.max(warnCountRaw, infoCountRaw); + const infoCount = Math.min(infoCountRaw, warnCountRaw);packages/core/src/preflight/analyze-image.ts (1)
17-18: Normalize warn/fail thresholds to avoid inverted configs.If
failMbis set belowwarnMb, warnings become unreachable and medium-size images fail unexpectedly. Clamping keeps semantics consistent.♻️ Suggested normalization
- const warnThresholdMb = config?.warnMb ?? DEFAULT_IMAGE_SIZE_WARN_MB; - const failThresholdMb = config?.failMb ?? DEFAULT_IMAGE_SIZE_FAIL_MB; + const warnThresholdMb = config?.warnMb ?? DEFAULT_IMAGE_SIZE_WARN_MB; + const failThresholdMb = config?.failMb ?? DEFAULT_IMAGE_SIZE_FAIL_MB; + const effectiveWarnMb = Math.min(warnThresholdMb, failThresholdMb); + const effectiveFailMb = Math.max(warnThresholdMb, failThresholdMb); ... - if (sizeMb > failThresholdMb) { + if (sizeMb > effectiveFailMb) { return { name: 'image-size', status: 'fail', - message: `Image size ${sizeMb}MB exceeds ${failThresholdMb}MB limit`, + message: `Image size ${sizeMb}MB exceeds ${effectiveFailMb}MB limit`, value: sizeMb, - threshold: failThresholdMb, + threshold: effectiveFailMb, }; } ... - if (sizeMb > warnThresholdMb) { + if (sizeMb > effectiveWarnMb) { return { name: 'image-size', status: 'warn', - message: `Image size ${sizeMb}MB exceeds recommended ${warnThresholdMb}MB`, + message: `Image size ${sizeMb}MB exceeds recommended ${effectiveWarnMb}MB`, value: sizeMb, - threshold: warnThresholdMb, + threshold: effectiveWarnMb, }; } ... - threshold: warnThresholdMb, + threshold: effectiveWarnMb,Also applies to: 30-55
Comment on lines
+62
to
+66
| if (options.auditOutput && audit) { | ||
| const outputPath = join(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.
Potential issue with absolute paths in auditOutput.
Using join(process.cwd(), options.auditOutput) will produce incorrect results if auditOutput is already an absolute path (e.g., /tmp/audit.json becomes /current/dir/tmp/audit.json).
Suggested fix using resolve()
- const outputPath = join(process.cwd(), options.auditOutput);
+ const outputPath = resolve(options.auditOutput);resolve() correctly handles both relative and absolute paths - relative paths are resolved against cwd(), while absolute paths are returned as-is. Note: resolve is already imported from node:path via line 2.
📝 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.
Suggested change
| if (options.auditOutput && audit) { | |
| const outputPath = join(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(options.auditOutput); | |
| await writeFile(outputPath, JSON.stringify(audit, null, 2)); | |
| logger.success(`Audit saved to ${outputPath}`); | |
| } |
🤖 Prompt for AI Agents
In `@packages/cli/src/commands/run.ts` around lines 62 - 66, Replace the path
construction that uses join(process.cwd(), options.auditOutput) so absolute
auditOutput paths are preserved; update the code that writes the audit (the
block using options.auditOutput, writeFile and logger.success) to compute the
output path with resolve(process.cwd(), options.auditOutput) (or
resolve(options.auditOutput)) from node:path instead of join, then pass that
resolved path to writeFile and logger.success.
Comment on lines
+116
to
+146
| const preflight = c['preflight']; | ||
| if (preflight !== undefined) { | ||
| if (typeof preflight !== 'object' || preflight === null) { | ||
| errors.push('preflight must be an object'); | ||
| } else { | ||
| const pf = preflight as Record<string, unknown>; | ||
|
|
||
| validatePreflightSection(pf['memory'], 'preflight.memory', errors, { | ||
| baseMb: 'positive', | ||
| perDependencyMb: 'positive', | ||
| warnRatio: 'positive', | ||
| failRatio: 'positive', | ||
| }); | ||
|
|
||
| validatePreflightSection(pf['dependencies'], 'preflight.dependencies', errors, { | ||
| warnCount: 'positive', | ||
| infoCount: 'positive', | ||
| }); | ||
|
|
||
| validatePreflightSection(pf['image'], 'preflight.image', errors, { | ||
| warnMb: 'positive', | ||
| failMb: 'positive', | ||
| }); | ||
|
|
||
| validatePreflightSection(pf['timeout'], 'preflight.timeout', errors, { | ||
| minMs: 'positive', | ||
| maxMs: 'positive', | ||
| coldStartBufferMs: 'positive', | ||
| }); | ||
| } | ||
| } |
There was a problem hiding this comment.
Missing cross-field validation for threshold ordering.
The validation ensures each field is a positive number, but doesn't validate logical relationships between related thresholds:
preflight.memory.warnRatioshould be less thanfailRatiopreflight.image.warnMbshould be less thanfailMbpreflight.timeout.minMsshould be less thanmaxMs
Invalid configurations (e.g., warnMb: 200, failMb: 100) would pass validation but produce confusing runtime behavior where warnings trigger after failures.
Suggested cross-field validation
validatePreflightSection(pf['image'], 'preflight.image', errors, {
warnMb: 'positive',
failMb: 'positive',
});
+
+ // Cross-field validations
+ const memory = pf['memory'] as Record<string, unknown> | undefined;
+ if (memory?.['warnRatio'] !== undefined && memory?.['failRatio'] !== undefined) {
+ if ((memory['warnRatio'] as number) >= (memory['failRatio'] as number)) {
+ errors.push('preflight.memory.warnRatio must be less than failRatio');
+ }
+ }
+ const image = pf['image'] as Record<string, unknown> | undefined;
+ if (image?.['warnMb'] !== undefined && image?.['failMb'] !== undefined) {
+ if ((image['warnMb'] as number) >= (image['failMb'] as number)) {
+ errors.push('preflight.image.warnMb must be less than failMb');
+ }
+ }
+ const timeout = pf['timeout'] as Record<string, unknown> | undefined;
+ if (timeout?.['minMs'] !== undefined && timeout?.['maxMs'] !== undefined) {
+ if ((timeout['minMs'] as number) >= (timeout['maxMs'] as number)) {
+ errors.push('preflight.timeout.minMs must be less than maxMs');
+ }
+ }📝 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.
Suggested change
| const preflight = c['preflight']; | |
| if (preflight !== undefined) { | |
| if (typeof preflight !== 'object' || preflight === null) { | |
| errors.push('preflight must be an object'); | |
| } else { | |
| const pf = preflight as Record<string, unknown>; | |
| validatePreflightSection(pf['memory'], 'preflight.memory', errors, { | |
| baseMb: 'positive', | |
| perDependencyMb: 'positive', | |
| warnRatio: 'positive', | |
| failRatio: 'positive', | |
| }); | |
| validatePreflightSection(pf['dependencies'], 'preflight.dependencies', errors, { | |
| warnCount: 'positive', | |
| infoCount: 'positive', | |
| }); | |
| validatePreflightSection(pf['image'], 'preflight.image', errors, { | |
| warnMb: 'positive', | |
| failMb: 'positive', | |
| }); | |
| validatePreflightSection(pf['timeout'], 'preflight.timeout', errors, { | |
| minMs: 'positive', | |
| maxMs: 'positive', | |
| coldStartBufferMs: 'positive', | |
| }); | |
| } | |
| } | |
| const preflight = c['preflight']; | |
| if (preflight !== undefined) { | |
| if (typeof preflight !== 'object' || preflight === null) { | |
| errors.push('preflight must be an object'); | |
| } else { | |
| const pf = preflight as Record<string, unknown>; | |
| validatePreflightSection(pf['memory'], 'preflight.memory', errors, { | |
| baseMb: 'positive', | |
| perDependencyMb: 'positive', | |
| warnRatio: 'positive', | |
| failRatio: 'positive', | |
| }); | |
| validatePreflightSection(pf['dependencies'], 'preflight.dependencies', errors, { | |
| warnCount: 'positive', | |
| infoCount: 'positive', | |
| }); | |
| validatePreflightSection(pf['image'], 'preflight.image', errors, { | |
| warnMb: 'positive', | |
| failMb: 'positive', | |
| }); | |
| validatePreflightSection(pf['timeout'], 'preflight.timeout', errors, { | |
| minMs: 'positive', | |
| maxMs: 'positive', | |
| coldStartBufferMs: 'positive', | |
| }); | |
| // Cross-field validations | |
| const memory = pf['memory'] as Record<string, unknown> | undefined; | |
| if (memory?.['warnRatio'] !== undefined && memory?.['failRatio'] !== undefined) { | |
| if ((memory['warnRatio'] as number) >= (memory['failRatio'] as number)) { | |
| errors.push('preflight.memory.warnRatio must be less than failRatio'); | |
| } | |
| } | |
| const image = pf['image'] as Record<string, unknown> | undefined; | |
| if (image?.['warnMb'] !== undefined && image?.['failMb'] !== undefined) { | |
| if ((image['warnMb'] as number) >= (image['failMb'] as number)) { | |
| errors.push('preflight.image.warnMb must be less than failMb'); | |
| } | |
| } | |
| const timeout = pf['timeout'] as Record<string, unknown> | undefined; | |
| if (timeout?.['minMs'] !== undefined && timeout?.['maxMs'] !== undefined) { | |
| if ((timeout['minMs'] as number) >= (timeout['maxMs'] as number)) { | |
| errors.push('preflight.timeout.minMs must be less than maxMs'); | |
| } | |
| } | |
| } | |
| } |
🤖 Prompt for AI Agents
In `@packages/core/src/service/load-service.ts` around lines 116 - 146, The
preflight object validation currently checks individual numeric fields but
misses cross-field ordering rules; after the calls to validatePreflightSection
for 'preflight.memory', 'preflight.image', and 'preflight.timeout' in
load-service.ts, read the parsed values from pf['memory'], pf['image'], and
pf['timeout'] and add explicit comparisons that push errors into the errors
array when ordering is wrong (e.g., if pf['memory']?.warnRatio >=
pf['memory']?.failRatio then push "preflight.memory.warnRatio must be less than
preflight.memory.failRatio"; if pf['image']?.warnMb >= pf['image']?.failMb then
push "preflight.image.warnMb must be less than preflight.image.failMb"; if
pf['timeout']?.minMs >= pf['timeout']?.maxMs then push "preflight.timeout.minMs
must be less than preflight.timeout.maxMs"). Ensure you handle missing or
non-number values gracefully (only perform comparison when both sides are
numbers).
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
Release Notes
New Features
--audit-outputflag to write audit results to JSON fileDocumentation
Updates
✏️ Tip: You can customize this high-level summary in your review settings.