Conversation
1. appendDiscoveryEvidence warns on non-standard categories (CategorySchema validation as safety net for converter typos) 2. calibrate-collect-gap-evidence CLI command: Reads gaps.json, extracts actionable + uncovered gaps, appends to data/discovery-evidence.json as source: "gap-analysis" entries. Fills the gap-analysis → discovery evidence path that was missing. 3. calibrate-loop.md updated to run gap evidence collection after Step 3 Tests: collectGapEvidence (4 new) Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
All new internal CLI commands now validate string arguments with Zod before processing (z.string().trim().min(1)): - calibrate-gather-evidence, calibrate-finalize-debate (calibrate-debate.ts) - discovery-filter-evidence, rule-apply-decision, calibrate-collect-gap-evidence (rule-discovery.ts) - calibrate-enrich-evidence, calibrate-prune-evidence (fixture-management.ts) Invalid args → console.log + early return (internal CLI convention). Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds Zod validation for internal CLI runDir/keyword args, warns on non-standard discovery-evidence categories during append, and introduces a new CLI command that reads Changes
Sequence DiagramsequenceDiagram
participant CLI as CLI Handler
participant Zod as Zod Validator
participant FS as File System
participant Parser as JSON Parser
participant Filter as Gap Filter
participant Appender as appendDiscoveryEvidence
CLI->>Zod: safeParse(runDir / keyword)
alt Invalid
Zod-->>CLI: Parse Error
CLI->>CLI: Log + return
else Valid
Zod-->>CLI: Validated args
CLI->>FS: read `gaps.json` from runDir
alt Missing or unreadable
FS-->>CLI: not found / error
CLI->>CLI: return []
else Read file
FS-->>Parser: file contents
Parser->>Parser: parse JSON (safe)
alt JSON invalid
Parser-->>CLI: parse error
CLI->>CLI: return []
else JSON valid
Parser-->>Filter: gaps[]
Filter->>Filter: select actionable && coveredByRule == null
Filter-->>Appender: actionable uncovered gaps
Appender->>Appender: map -> DiscoveryEvidenceEntry[] (source: "gap-analysis", impact: "medium", timestamp)
Appender->>Zod: validate entry.category with CategorySchema
alt Category invalid
Zod-->>Appender: invalid
Appender-->>CLI: console.warn (non-standard category)
end
Appender->>FS: append entries to discovery-evidence.json
CLI->>CLI: log appended count
end
end
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
…ategory - Test: category warning fires on non-standard, does not fire on standard (2 new) - coveredByRule: truthy check instead of null/undefined (handles empty string) - Legacy migration test: "color" → "pixel-critical" (avoids CI warning noise) Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
User corrections: - coveredByRule: truthy → nullish check (!=null), empty string counts as "marked covered" — more precise than truthy which would also skip 0 - Empty string coveredByRule test added - Existing test categories updated to standard values Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/cli/commands/internal/fixture-management.ts`:
- Around line 169-175: The code in registerEvidencePrune calls
resolve(parsed.data) multiple times; assign the resolved path to a local
variable (e.g., resolvedDir) after RunDirArgSchema.safeParse succeeds, use that
variable for the existsSync check and for parseDebateResult to avoid duplicate
resolution and match the pattern used in registerEvidenceEnrich (use parsed,
resolve, existsSync, and parseDebateResult to locate the change).
🪄 Autofix (Beta)
✅ Autofix completed
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: e13ee687-bf18-4f83-b159-a8d93df92230
📒 Files selected for processing (8)
.claude/commands/calibrate-loop.mdsrc/agents/evidence-collector.test.tssrc/agents/evidence-collector.tssrc/cli/commands/internal/calibrate-debate.tssrc/cli/commands/internal/fixture-management.tssrc/cli/commands/internal/rule-discovery.test.tssrc/cli/commands/internal/rule-discovery.tssrc/cli/index.ts
Contributor
|
Note Autofix is a beta feature. Expect some limitations and changes as we gather feedback and continue to improve it. Fixes Applied SuccessfullyFixed 1 file(s) based on 1 unresolved review comment. Files modified:
Commit: The changes have been pushed to the Time taken: |
Fixed 1 file(s) based on 1 unresolved review comment. Co-authored-by: CodeRabbit <noreply@coderabbit.ai>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/cli/commands/internal/fixture-management.ts`:
- Line 6: Rename the constant RunDirArgSchema to SCREAMING_SNAKE_CASE
(RUN_DIR_ARG_SCHEMA) and update all local references and any imports/exports
that use RunDirArgSchema; locate the definition of RunDirArgSchema
(z.string().trim().min(1, "runDir is required")) and rename the identifier to
RUN_DIR_ARG_SCHEMA, adjust any named export or import statements that reference
it, and run a quick project-wide search to replace remaining uses to avoid
breaking references.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 8eec8d15-c583-47fd-8dcc-2f27d733eb37
📒 Files selected for processing (1)
src/cli/commands/internal/fixture-management.ts
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
src/cli/commands/internal/rule-discovery.ts (1)
139-149: 🛠️ Refactor suggestion | 🟠 MajorRename the schema constants to SCREAMING_SNAKE_CASE.
GapSchemaandGapsFileSchemaare module-level constants, so they should follow the same naming rule asRUN_DIR_ARG_SCHEMAandKEYWORD_ARG_SCHEMA.♻️ Suggested rename
-const GapSchema = z.object({ +const GAP_SCHEMA = z.object({ category: z.string(), description: z.string(), actionable: z.boolean(), coveredByRule: z.unknown().optional(), }).passthrough(); -const GapsFileSchema = z.object({ +const GAPS_FILE_SCHEMA = z.object({ fixture: z.string().optional(), - gaps: z.array(GapSchema), + gaps: z.array(GAP_SCHEMA), }).passthrough(); ... - const parsed = GapsFileSchema.safeParse(raw); + const parsed = GAPS_FILE_SCHEMA.safeParse(raw);As per coding guidelines, "Constants must use SCREAMING_SNAKE_CASE (e.g.,
MY_CONSTANT, notmyConstant)".Also applies to: 165-165
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/cli/commands/internal/rule-discovery.ts` around lines 139 - 149, The module-level schema constants GapSchema and GapsFileSchema should be renamed to SCREAMING_SNAKE_CASE (GAP_SCHEMA and GAPS_FILE_SCHEMA); update the const declarations for GapSchema and GapsFileSchema to GAP_SCHEMA and GAPS_FILE_SCHEMA respectively and replace every use/reference of GapSchema and GapsFileSchema in this file (including the related usage around the other occurrence noted near line ~165) so the code compiles and follows the constant naming convention.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/cli/commands/internal/calibrate-debate.ts`:
- Around line 109-114: The validation currently only checks existence via
RUN_DIR_ARG_SCHEMA.safeParse and existsSync(dir) which allows existing files to
pass; update both handlers (calibrate-gather-evidence and
calibrate-finalize-debate) to reject non-directories by adding an is-directory
guard after resolve(parsed.data) — e.g., call fs.statSync(dir) or
fs.lstatSync(dir) and check .isDirectory() and if false log the same "Run
directory not found" (or similar) and return; keep the existing parsed/dir
variables (RUN_DIR_ARG_SCHEMA, parsed, dir) so the new check sits immediately
after the existsSync check to prevent later ENOTDIR errors and misleading
messages.
In `@src/cli/commands/internal/rule-discovery.ts`:
- Around line 210-220: The catch block around
collectGapEvidence/appendDiscoveryEvidence currently swallows persistence errors
and exits 0; change it so filesystem failures from appendDiscoveryEvidence
signal command failure: in the catch for the try that calls
appendDiscoveryEvidence(entries) keep the console.log of the error but then
either rethrow the error or call process.exit(1) so the process returns a
non-zero exit code; locate the try/catch that wraps collectGapEvidence and
appendDiscoveryEvidence in rule-discovery.ts and adjust the catch to emit the
same message and then fail the command (process.exit(1) or throw) instead of
returning successfully.
- Around line 143-145: The schema currently allows coveredByRule to be missing
because it uses z.unknown().optional(), so gaps with a missing field are treated
as uncovered; change the property definition(s) for coveredByRule in the Zod
schemas in rule-discovery.ts from z.unknown().optional() to explicitly require
null (e.g., z.null()) so only records with coveredByRule === null pass, and
update both occurrences where coveredByRule is defined (the schema around the
actionable selection and the other schema at the later block).
---
Duplicate comments:
In `@src/cli/commands/internal/rule-discovery.ts`:
- Around line 139-149: The module-level schema constants GapSchema and
GapsFileSchema should be renamed to SCREAMING_SNAKE_CASE (GAP_SCHEMA and
GAPS_FILE_SCHEMA); update the const declarations for GapSchema and
GapsFileSchema to GAP_SCHEMA and GAPS_FILE_SCHEMA respectively and replace every
use/reference of GapSchema and GapsFileSchema in this file (including the
related usage around the other occurrence noted near line ~165) so the code
compiles and follows the constant naming convention.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 893192c1-e0e3-4d1f-aa32-7efd4a99e7de
📒 Files selected for processing (3)
src/cli/commands/internal/calibrate-debate.tssrc/cli/commands/internal/fixture-management.tssrc/cli/commands/internal/rule-discovery.ts
Comment on lines
+143
to
+145
| coveredByRule: z.unknown().optional(), | ||
| }).passthrough(); | ||
|
|
Contributor
There was a problem hiding this comment.
Require an explicit coveredByRule: null before importing a gap.
The selection contract here is “actionable and coveredByRule === null”. With coveredByRule optional plus the != null check, gaps where the field is simply missing are treated as uncovered and get appended anyway. That turns older or malformed gaps.json into false-positive discovery evidence.
♻️ Suggested contract tightening
const GapSchema = z.object({
category: z.string(),
description: z.string(),
actionable: z.boolean(),
- coveredByRule: z.unknown().optional(),
+ coveredByRule: z.unknown().nullable(),
}).passthrough();
...
- if (gap.coveredByRule != null) continue;
+ if (gap.coveredByRule !== null) continue;Also applies to: 171-175
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/cli/commands/internal/rule-discovery.ts` around lines 143 - 145, The
schema currently allows coveredByRule to be missing because it uses
z.unknown().optional(), so gaps with a missing field are treated as uncovered;
change the property definition(s) for coveredByRule in the Zod schemas in
rule-discovery.ts from z.unknown().optional() to explicitly require null (e.g.,
z.null()) so only records with coveredByRule === null pass, and update both
occurrences where coveredByRule is defined (the schema around the actionable
selection and the other schema at the later block).
Comment on lines
+210
to
+220
| try { | ||
| const entries = collectGapEvidence(dir, fixture); | ||
| if (entries.length === 0) { | ||
| console.log("No uncovered actionable gaps found"); | ||
| return; | ||
| } | ||
|
|
||
| appendDiscoveryEvidence(entries); | ||
| console.log(`Collected ${entries.length} gap evidence entries for fixture "${fixture}"`); | ||
| } catch (err) { | ||
| console.log(`Failed to collect gap evidence: ${err instanceof Error ? err.message : String(err)}`); |
Contributor
There was a problem hiding this comment.
Mark append failures as command failures.
appendDiscoveryEvidence() can still throw on filesystem errors, but this catch only prints a message and exits successfully. That makes the calibration loop continue without the new evidence even though the command’s only side effect failed. Keep the stdout message if needed, but set a non-zero exit code here.
♻️ Suggested failure signaling
} catch (err) {
console.log(`Failed to collect gap evidence: ${err instanceof Error ? err.message : String(err)}`);
+ process.exitCode = 1;
}Based on learnings, the stdout-and-exit-0 convention in src/cli/commands/internal/ is for expected cases like missing run directories and parseDebateResult returning null, not unexpected persistence failures.
📝 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
| try { | |
| const entries = collectGapEvidence(dir, fixture); | |
| if (entries.length === 0) { | |
| console.log("No uncovered actionable gaps found"); | |
| return; | |
| } | |
| appendDiscoveryEvidence(entries); | |
| console.log(`Collected ${entries.length} gap evidence entries for fixture "${fixture}"`); | |
| } catch (err) { | |
| console.log(`Failed to collect gap evidence: ${err instanceof Error ? err.message : String(err)}`); | |
| try { | |
| const entries = collectGapEvidence(dir, fixture); | |
| if (entries.length === 0) { | |
| console.log("No uncovered actionable gaps found"); | |
| return; | |
| } | |
| appendDiscoveryEvidence(entries); | |
| console.log(`Collected ${entries.length} gap evidence entries for fixture "${fixture}"`); | |
| } catch (err) { | |
| console.log(`Failed to collect gap evidence: ${err instanceof Error ? err.message : String(err)}`); | |
| process.exitCode = 1; | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/cli/commands/internal/rule-discovery.ts` around lines 210 - 220, The
catch block around collectGapEvidence/appendDiscoveryEvidence currently swallows
persistence errors and exits 0; change it so filesystem failures from
appendDiscoveryEvidence signal command failure: in the catch for the try that
calls appendDiscoveryEvidence(entries) keep the console.log of the error but
then either rethrow the error or call process.exit(1) so the process returns a
non-zero exit code; locate the try/catch that wraps collectGapEvidence and
appendDiscoveryEvidence in rule-discovery.ts and adjust the catch to emit the
same message and then fail the command (process.exit(1) or throw) instead of
returning successfully.
- calibrate-gather-evidence, calibrate-finalize-debate: add isDirectory() (was: existsSync only, file path would cause ENOTDIR) - GapSchema.coveredByRule: .default(null) instead of .optional() (missing field now defaults to null → treated as uncovered, not silently skipped) Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Contributor
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 (1)
src/cli/commands/internal/rule-discovery.ts (1)
45-59:⚠️ Potential issue | 🟡 MinorValidate
--run-dirbefore branching on it.
keywordis now trimmed/validated, butoptions.runDirstill bypasses Zod entirely. That means--run-dir ""is treated as “option absent” instead of invalid input, and whitespace-padded paths are never trimmed before the directory check.♻️ Proposed fix
.option("--run-dir <path>", "Write filtered evidence to run directory") .action((keyword: string, options: { runDir?: string }) => { const kParsed = KEYWORD_ARG_SCHEMA.safeParse(keyword); if (!kParsed.success) { console.log(`Invalid keyword: ${kParsed.error.issues[0]?.message}`); return; } try { const filtered = filterDiscoveryEvidence(kParsed.data); - if (options.runDir) { - const dir = resolve(options.runDir); + if (options.runDir !== undefined) { + const runDirParsed = RUN_DIR_ARG_SCHEMA.safeParse(options.runDir); + if (!runDirParsed.success) { console.log(`Invalid runDir: ${runDirParsed.error.issues[0]?.message}`); return; } + const dir = resolve(runDirParsed.data); if (!existsSync(dir) || !statSync(dir).isDirectory()) { console.log(`Run directory not found or is not a directory: ${options.runDir}`); return; }As per coding guidelines,
src/**/*.ts: Use Zod schemas for external input validation (schemas in contracts/ directory).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/cli/commands/internal/rule-discovery.ts` around lines 45 - 59, Validate the --run-dir option with a Zod schema instead of treating falsy/whitespace as absent: add or import a RUN_DIR_ARG_SCHEMA (in contracts/) and call RUN_DIR_ARG_SCHEMA.safeParse(options.runDir) inside the .action callback (near where KEYWORD_ARG_SCHEMA is used); if parsing fails log the validation error and return, otherwise use the parsed/trimmed value (not the raw options.runDir) for resolve(), existence and isDirectory checks and for building outPath before calling writeFileSync and the final console.log; keep filterDiscoveryEvidence and the rest of the flow unchanged, only replace direct uses of options.runDir with the validated value.
♻️ Duplicate comments (1)
src/cli/commands/internal/rule-discovery.ts (1)
210-220:⚠️ Potential issue | 🟠 MajorSurface append failures as command failures.
By the time execution reaches this
catch, the expected exit-0 cases have already been handled (gaps.jsonmissing/invalid or no uncovered gaps). IfappendDiscoveryEvidence()throws here, the command's only side effect failed, but the process still returns success and the calibration loop can continue without the new evidence.♻️ Proposed fix
} catch (err) { console.log(`Failed to collect gap evidence: ${err instanceof Error ? err.message : String(err)}`); + process.exitCode = 1; }Based on learnings, the stdout-and-exit-0 convention in
src/cli/commands/internal/applies to expected cases like missing run directories andparseDebateResultreturningnull, not unexpected persistence failures.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/cli/commands/internal/rule-discovery.ts` around lines 210 - 220, The catch block swallowing persistence errors should be changed to surface failures as command failures: when appendDiscoveryEvidence(entries) throws inside the try in function handling the rule-discovery command, log the error (use console.error or processLogger.error) with the full error message and stack and then terminate with a non-zero exit (e.g., throw the error or call process.exit(1)) so the process does not return success; update the catch that currently logs `Failed to collect gap evidence` to rethrow or exit non‑zero to ensure appendDiscoveryEvidence failures fail the command.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/cli/commands/internal/calibrate-debate.ts`:
- Around line 109-113: The statSync call can throw and currently can abort the
CLI instead of performing the intended console.log + return flow; wrap the
directory check in a try/catch around the statSync (after resolving parsed.data
and verifying existsSync) so any thrown error results in the same
console.log(`Run directory not found or is not a directory: ${runDir}`) and an
early return; update the check around the dir variable used after
RUN_DIR_ARG_SCHEMA.safeParse (and apply the same try/catch pattern to the
similar check later in the file around the lines that validate run directories
at the second block) to ensure exception-safe directory validation.
---
Outside diff comments:
In `@src/cli/commands/internal/rule-discovery.ts`:
- Around line 45-59: Validate the --run-dir option with a Zod schema instead of
treating falsy/whitespace as absent: add or import a RUN_DIR_ARG_SCHEMA (in
contracts/) and call RUN_DIR_ARG_SCHEMA.safeParse(options.runDir) inside the
.action callback (near where KEYWORD_ARG_SCHEMA is used); if parsing fails log
the validation error and return, otherwise use the parsed/trimmed value (not the
raw options.runDir) for resolve(), existence and isDirectory checks and for
building outPath before calling writeFileSync and the final console.log; keep
filterDiscoveryEvidence and the rest of the flow unchanged, only replace direct
uses of options.runDir with the validated value.
---
Duplicate comments:
In `@src/cli/commands/internal/rule-discovery.ts`:
- Around line 210-220: The catch block swallowing persistence errors should be
changed to surface failures as command failures: when
appendDiscoveryEvidence(entries) throws inside the try in function handling the
rule-discovery command, log the error (use console.error or processLogger.error)
with the full error message and stack and then terminate with a non-zero exit
(e.g., throw the error or call process.exit(1)) so the process does not return
success; update the catch that currently logs `Failed to collect gap evidence`
to rethrow or exit non‑zero to ensure appendDiscoveryEvidence failures fail the
command.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 91478502-1e72-404e-9804-19975aa78192
📒 Files selected for processing (2)
src/cli/commands/internal/calibrate-debate.tssrc/cli/commands/internal/rule-discovery.ts
Comment on lines
+109
to
+113
| const parsed = RUN_DIR_ARG_SCHEMA.safeParse(runDir); | ||
| if (!parsed.success) { console.log(`Invalid runDir: ${parsed.error.issues[0]?.message}`); return; } | ||
| const dir = resolve(parsed.data); | ||
| if (!existsSync(dir) || !statSync(dir).isDirectory()) { | ||
| console.log(`Run directory not found or is not a directory: ${runDir}`); |
Contributor
There was a problem hiding this comment.
Guard statSync exceptions to preserve stdout-and-return contract.
statSync(dir) can throw (e.g., ENOENT race/EACCES), which would abort the command instead of following the expected console.log + early return flow.
♻️ Suggested fix (shared validator + exception-safe dir check)
+function resolveValidatedRunDir(runDir: string): string | null {
+ const parsed = RUN_DIR_ARG_SCHEMA.safeParse(runDir);
+ if (!parsed.success) {
+ console.log(`Invalid runDir: ${parsed.error.issues[0]?.message}`);
+ return null;
+ }
+
+ const dir = resolve(parsed.data);
+ try {
+ if (!statSync(dir).isDirectory()) {
+ console.log(`Run directory not found or is not a directory: ${runDir}`);
+ return null;
+ }
+ } catch {
+ console.log(`Run directory not found or is not a directory: ${runDir}`);
+ return null;
+ }
+
+ return dir;
+}
+
export function registerGatherEvidence(cli: CAC): void {
cli
@@
)
.action((runDir: string) => {
- const parsed = RUN_DIR_ARG_SCHEMA.safeParse(runDir);
- if (!parsed.success) { console.log(`Invalid runDir: ${parsed.error.issues[0]?.message}`); return; }
- const dir = resolve(parsed.data);
- if (!existsSync(dir) || !statSync(dir).isDirectory()) {
- console.log(`Run directory not found or is not a directory: ${runDir}`);
- return;
- }
+ const dir = resolveValidatedRunDir(runDir);
+ if (!dir) return;
@@
export function registerFinalizeDebate(cli: CAC): void {
cli
@@
)
.action((runDir: string) => {
- const parsed = RUN_DIR_ARG_SCHEMA.safeParse(runDir);
- if (!parsed.success) { console.log(`Invalid runDir: ${parsed.error.issues[0]?.message}`); return; }
- const dir = resolve(parsed.data);
- if (!existsSync(dir) || !statSync(dir).isDirectory()) {
- console.log(`Run directory not found or is not a directory: ${runDir}`);
- return;
- }
+ const dir = resolveValidatedRunDir(runDir);
+ if (!dir) return;Based on learnings, internal calibration CLI commands must treat missing/invalid run directories as expected outcomes via console.log + return (exit code 0).
Also applies to: 142-146
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/cli/commands/internal/calibrate-debate.ts` around lines 109 - 113, The
statSync call can throw and currently can abort the CLI instead of performing
the intended console.log + return flow; wrap the directory check in a try/catch
around the statSync (after resolving parsed.data and verifying existsSync) so
any thrown error results in the same console.log(`Run directory not found or is
not a directory: ${runDir}`) and an early return; update the check around the
dir variable used after RUN_DIR_ARG_SCHEMA.safeParse (and apply the same
try/catch pattern to the similar check later in the file around the lines that
validate run directories at the second block) to ensure exception-safe directory
validation.
Shared cli-helpers.ts with resolveRunDir() — validates arg, resolves path, checks isDirectory, catches statSync exceptions. All internal commands now use this single function instead of duplicating the pattern. Removes RunDirArgSchema duplication across 3 files. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
src/cli/commands/internal/rule-discovery.ts (2)
195-206:⚠️ Potential issue | 🟠 MajorSignal collection failures as command failures.
If
appendDiscoveryEvidence()throws here, the command only logs and exits successfully even though its only side effect failed. Setprocess.exitCode = 1after the log so the calibration loop can detect the failure.Based on learnings, internal calibration CLI commands should only "log the relevant message to stdout with `console.log` and complete with exit code `0`" for expected cases like missing run directories or `parseDebateResult` returning `null`.🛡️ Proposed fix
} catch (err) { console.log(`Failed to collect gap evidence: ${err instanceof Error ? err.message : String(err)}`); + process.exitCode = 1; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/cli/commands/internal/rule-discovery.ts` around lines 195 - 206, The catch block around collectGapEvidence/appendDiscoveryEvidence currently only logs errors so failures from appendDiscoveryEvidence are treated as success; modify the catch in the try/catch that wraps collectGapEvidence and appendDiscoveryEvidence so that after logging the error you set process.exitCode = 1 to signal command failure (keep console.log unchanged for expected cases like missing directories or parseDebateResult null); reference the block handling entries = collectGapEvidence(dir, fixture) and the call to appendDiscoveryEvidence(entries) and ensure the change is limited to adding process.exitCode = 1 in that catch branch.
133-165:⚠️ Potential issue | 🟠 MajorRequire an explicit
coveredByRule: nullbefore importing a gap.Line 133 turns a missing
coveredByRuleintonull, so Line 165 imports legacy or malformed gaps as uncovered. That violates thecoveredByRule === nullcontract and can create false-positive discovery evidence.🐛 Proposed fix
const GapSchema = z.object({ category: z.string(), description: z.string(), actionable: z.boolean(), - coveredByRule: z.unknown().default(null), + coveredByRule: z.unknown().nullable().optional(), }).passthrough(); @@ - if (gap.coveredByRule != null) continue; + if (gap.coveredByRule !== null) continue;In Zod 4, does `z.object({ coveredByRule: z.unknown().default(null) }).safeParse({})` return `{ coveredByRule: null }`, and what is the recommended way to distinguish a missing field from an explicit `null` value?🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/cli/commands/internal/rule-discovery.ts` around lines 133 - 165, The schema currently sets coveredByRule to default(null) so a missing field is treated as explicit null; update the Zod schema (GapSchema/GapsFileSchema used by collectGapEvidence) to remove the default and make coveredByRule optional (e.g., z.unknown().optional()) so absence stays absent, and then change the loop in collectGapEvidence to only import gaps when the property is present and explicitly null — e.g., use Object.prototype.hasOwnProperty.call(gap, "coveredByRule") to check presence and require gap.coveredByRule === null before treating the gap as uncovered.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/cli/commands/internal/rule-discovery.ts`:
- Around line 129-139: Rename the zod schema constants from GapSchema and
GapsFileSchema to SCREAMING_SNAKE_CASE (e.g., GAP_SCHEMA and GAPS_FILE_SCHEMA);
update the declaration identifiers and every reference/usage (including the
other occurrences around the second set of schema definitions) to the new names,
and adjust any exports/imports accordingly so the rest of the module compiles
with the new constant identifiers.
---
Duplicate comments:
In `@src/cli/commands/internal/rule-discovery.ts`:
- Around line 195-206: The catch block around
collectGapEvidence/appendDiscoveryEvidence currently only logs errors so
failures from appendDiscoveryEvidence are treated as success; modify the catch
in the try/catch that wraps collectGapEvidence and appendDiscoveryEvidence so
that after logging the error you set process.exitCode = 1 to signal command
failure (keep console.log unchanged for expected cases like missing directories
or parseDebateResult null); reference the block handling entries =
collectGapEvidence(dir, fixture) and the call to
appendDiscoveryEvidence(entries) and ensure the change is limited to adding
process.exitCode = 1 in that catch branch.
- Around line 133-165: The schema currently sets coveredByRule to default(null)
so a missing field is treated as explicit null; update the Zod schema
(GapSchema/GapsFileSchema used by collectGapEvidence) to remove the default and
make coveredByRule optional (e.g., z.unknown().optional()) so absence stays
absent, and then change the loop in collectGapEvidence to only import gaps when
the property is present and explicitly null — e.g., use
Object.prototype.hasOwnProperty.call(gap, "coveredByRule") to check presence and
require gap.coveredByRule === null before treating the gap as uncovered.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 15e377ad-9a15-4272-9734-be7d8dc71f2a
📒 Files selected for processing (4)
src/cli/commands/internal/calibrate-debate.tssrc/cli/commands/internal/cli-helpers.tssrc/cli/commands/internal/fixture-management.tssrc/cli/commands/internal/rule-discovery.ts
Comment on lines
+129
to
+139
| const GapSchema = z.object({ | ||
| category: z.string(), | ||
| description: z.string(), | ||
| actionable: z.boolean(), | ||
| coveredByRule: z.unknown().default(null), | ||
| }).passthrough(); | ||
|
|
||
| const GapsFileSchema = z.object({ | ||
| fixture: z.string().optional(), | ||
| gaps: z.array(GapSchema), | ||
| }).passthrough(); |
Contributor
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Rename the schema constants to SCREAMING_SNAKE_CASE.
GapSchema and GapsFileSchema are values, so they should follow the repo's constant naming rule.
♻️ Proposed rename
-const GapSchema = z.object({
+const GAP_SCHEMA = z.object({
category: z.string(),
description: z.string(),
actionable: z.boolean(),
coveredByRule: z.unknown().default(null),
}).passthrough();
-const GapsFileSchema = z.object({
+const GAPS_FILE_SCHEMA = z.object({
fixture: z.string().optional(),
- gaps: z.array(GapSchema),
+ gaps: z.array(GAP_SCHEMA),
}).passthrough();
@@
- const parsed = GapsFileSchema.safeParse(raw);
+ const parsed = GAPS_FILE_SCHEMA.safeParse(raw);Also applies to: 155-156
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/cli/commands/internal/rule-discovery.ts` around lines 129 - 139, Rename
the zod schema constants from GapSchema and GapsFileSchema to
SCREAMING_SNAKE_CASE (e.g., GAP_SCHEMA and GAPS_FILE_SCHEMA); update the
declaration identifiers and every reference/usage (including the other
occurrences around the second set of schema definitions) to the new names, and
adjust any exports/imports accordingly so the rest of the module compiles with
the new constant identifiers.
This was referenced Mar 30, 2026
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
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
Two issues combined:
#174 — Discovery evidence category validation + gap-analysis collection
Category validation:
appendDiscoveryEvidencenow warns when entries have non-standard categories (not inCategorySchema). Safety net for converter typos/old category labels — the converter prompt was fixed in #173, this is the code-side guard.Gap evidence collection: New CLI
calibrate-collect-gap-evidence <runDir>extracts actionable uncovered gaps fromgaps.jsonand appends todata/discovery-evidence.jsonassource: "gap-analysis"entries. Previously only the evaluation path (source: "evaluation") fed discovery evidence — gap-analysis was a dead end.calibrate-loop.mdupdated to run this after Step 3 (Gap Analysis).#175 — Zod argument validation for internal CLI commands
All 7 new internal commands now validate args with
z.string().trim().min(1):calibrate-gather-evidence,calibrate-finalize-debatecalibrate-enrich-evidence,calibrate-prune-evidencediscovery-filter-evidence,rule-apply-decision,calibrate-collect-gap-evidenceTest plan
Closes #174, closes #175
🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Bug Fixes
Tests