refactor(cli): modularize command handlers and audit internals#13
Merged
WellDunDun merged 9 commits intomasterfrom Feb 23, 2026
Merged
refactor(cli): modularize command handlers and audit internals#13WellDunDun merged 9 commits intomasterfrom
WellDunDun merged 9 commits intomasterfrom
Conversation
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
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:
📝 WalkthroughWalkthroughRefactors the CLI into a thin router delegating to command handlers; adds audit runtime context and scoring, automation-pack detection/scaffolding (auto, agent-factory), templates, detection/filesystem utilities, types, expanded tests, docs updates, and a new --pack init flag. Changes
Sequence Diagram(s)sequenceDiagram
participant CLI as CLI Router
participant Init as Init Command
participant Pack as Automation Pack Resolver
participant Template as Templates
participant FS as Filesystem
CLI->>Init: runInitCommand(path, name, force, pack)
Init->>Pack: resolveAutomationPack(path, requested)
Pack->>FS: detectCliProject(path)
Pack->>FS: hasAgentFactoryPack(path)
Pack-->>Init: PackResolution(reason, selected)
Init->>Template: generate base templates (AGENTS.md, ARCHITECTURE.md, risk)
Init->>Pack: scaffoldAutomationPack(path, selected, force)
Pack->>Template: getAgentFactoryPackFiles()
Pack->>FS: write created files
Pack-->>Init: createdFiles[]
Init-->>CLI: JSON summary (created items, next steps)
sequenceDiagram
participant CLI as CLI Router
participant Audit as Audit Command
participant Context as Audit Context Builder
participant Detection as Detection Utils
participant FS as Filesystem
participant Scoring as Scoring Engine
CLI->>Audit: runAuditCommand(targetPath)
Audit->>Context: buildAuditRuntimeContext(targetDir)
Context->>Detection: detectCliDiagnosabilitySignals(targetDir)
Context->>Detection: detectMonorepoWorkspaces(pkgJsonPath)
Context->>FS: findFiles(targetDir, docPattern)
Context-->>Audit: AuditRuntimeContext
Audit->>Scoring: applyAuditScoring(result, context)
Scoring->>Scoring: compute facet scores & findings
Scoring->>Scoring: resolveMaturityLevel(totalScore)
Scoring-->>Audit: populated result (total_score, maturity_level, recommendations)
Audit-->>CLI: JSON AuditResult
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
🚥 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🧪 Generate unit tests (beta)
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: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
cli/reins/src/index.ts (1)
8-46:⚠️ Potential issue | 🟠 MajorHelp output breaks deterministic JSON contract.
printHelp()emits plain text, which conflicts with the deterministic JSON output requirement. Consider emitting a JSON help payload (or making help JSON-only) so agents can parse it reliably.💡 Minimal JSON output adjustment
- console.log(help); + console.log(JSON.stringify({ command: "help", help }, null, 2));As per coding guidelines, "All commands must output deterministic JSON".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cli/reins/src/index.ts` around lines 8 - 46, printHelp currently prints plain text which violates the deterministic JSON output rule; modify the printHelp function to emit a single JSON payload instead (e.g., a plain object with keys like "name", "usage", "commands", "options", "examples", "maturityLevels") and output it via console.log(JSON.stringify(helpPayload)) so consumers can parse it reliably; ensure fields are consistently ordered/stable (explicitly construct the object with fixed keys) and remove or replace the plain-text console.log(help) call so only deterministic JSON is emitted.
♻️ Duplicate comments (2)
README.md (2)
291-294: Project structure reflects modular CLI layout.If the CLI must remain single-file, this structure section will need to be revised accordingly (see index.ts comment). As per coding guidelines, "All CLI logic must live in a single file at
cli/reins/src/index.ts".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@README.md` around lines 291 - 294, The README's "Project structure" section lists a modular layout that contradicts the coding guideline requiring all CLI logic to live in a single file (cli/reins/src/index.ts); update this section to reflect the single-file CLI design by removing or replacing references to src/lib/commands, src/lib/audit, and other multi-file directories and instead document that CLI entrypoint and handlers reside in cli/reins/src/index.ts (you can keep package.json and any top-level files listed).
15-29: Docs now describe modular command handlers.If we must keep all CLI logic inside
cli/reins/src/index.ts, this section should be updated to match that constraint (see index.ts comment). As per coding guidelines, "All CLI logic must live in a single file atcli/reins/src/index.ts".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@README.md` around lines 15 - 29, Update the README's "CLI" row to reflect the single-file CLI rule: replace the current "CLI" source-of-truth value (`cli/reins/src/lib/commands/` (+ routed via `cli/reins/src/index.ts`) ) with the enforced single file `cli/reins/src/index.ts`, and remove or reword any language implying modular command handlers so it matches the index.ts comment and the coding guideline "All CLI logic must live in a single file at cli/reins/src/index.ts".
🧹 Nitpick comments (3)
ARCHITECTURE.md (1)
15-15: Consider adding language specifiers to fenced code blocks.Static analysis flagged missing language specifiers on the directory tree (line 15) and flow diagram (line 40) code blocks. Adding
textorplaintextas the language identifier would satisfy the linter.Proposed fix
-``` +```text cli/reins/src/-``` +```text lib/helpers + lib/commands → index.ts router → CLI outputAlso applies to: 40-40
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ARCHITECTURE.md` at line 15, Add missing fenced-code language specifiers to the two code blocks in ARCHITECTURE.md: the directory tree block containing "cli/reins/src/" and the flow-diagram block containing "lib/helpers + lib/commands → index.ts router → CLI output"; update each triple-backtick fence to include a language like text or plaintext (e.g., ```text) so the linter stops flagging them.cli/reins/src/lib/templates.ts (1)
1-6: Avoid duplicatingAutomationPacktype definitions.
AutomationPackalready exists incli/reins/src/lib/automation-pack.ts; defining it again here risks drift. Prefer re-exporting the shared type instead of duplicating it.♻️ Suggested change
-export type AutomationPack = "none" | "auto" | "agent-factory"; +export type { AutomationPack } from "./automation-pack";🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cli/reins/src/lib/templates.ts` around lines 1 - 6, Remove the duplicate AutomationPack type declaration in templates.ts and re-export the shared type from the existing module instead: replace the local "export type AutomationPack = ..." with a re-export such as "export { AutomationPack } from './automation-pack';" (or "export type { AutomationPack } from './automation-pack';" if you prefer type-only export), leaving AgentFactoryPackFile intact and updating any local imports/usages in this file to reference the re-exported AutomationPack.cli/reins/src/lib/audit/scoring.ts (1)
265-274: Consider adding defensive error handling.This function performs
JSON.parsewithout its own try-catch, relying on the caller (scoreDependencyFootprint) to handle exceptions. While this works for the current single call site, adding local error handling would make the function more robust if it's ever called directly.🛡️ Optional: Add defensive error handling
function scoreSinglePackageDependencyFootprint(result: AuditResult, ctx: AuditRuntimeContext): void { - const pkg = JSON.parse(readFileSync(ctx.pkgJsonPath, "utf-8")); - const depCount = Object.keys(pkg.dependencies || {}).length; - if (depCount < 20) { - result.scores.agent_legibility.score++; - result.scores.agent_legibility.findings.push(`Lean dependency set (${depCount} dependencies)`); - } else { - result.scores.agent_legibility.findings.push(`Heavy dependency set (${depCount} dependencies) — consider trimming`); + try { + const pkg = JSON.parse(readFileSync(ctx.pkgJsonPath, "utf-8")); + const depCount = Object.keys(pkg.dependencies || {}).length; + if (depCount < 20) { + result.scores.agent_legibility.score++; + result.scores.agent_legibility.findings.push(`Lean dependency set (${depCount} dependencies)`); + } else { + result.scores.agent_legibility.findings.push(`Heavy dependency set (${depCount} dependencies) — consider trimming`); + } + } catch { + // ignore parse failures } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cli/reins/src/lib/audit/scoring.ts` around lines 265 - 274, The function scoreSinglePackageDependencyFootprint currently calls readFileSync + JSON.parse without local error handling; wrap the file read/parse in a try/catch inside scoreSinglePackageDependencyFootprint and handle parse/io errors locally (e.g., add a descriptive finding to result.scores.agent_legibility.findings and avoid throwing) so the function is robust if called directly; ensure you reference the same result and ctx parameters and preserve existing behavior for the depCount path while returning early on error.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@cli/reins/src/index.ts`:
- Around line 3-6: The CLI currently imports command implementations (runAudit,
runAuditCommand, runDoctor, runEvolve, runInit as runInitCommand) from
cli/reins/src/lib/commands/* which violates the single-file CLI rule; revert
this by moving the command logic back into cli/reins/src/index.ts: copy the
implementations for runAudit/runAuditCommand, runDoctor, runEvolve, and runInit
from their modules into index.ts, remove those imports, and ensure index.ts
exports/uses the local functions (or alternatively, if you intend to allow
modularization, update the project guideline instead—but do one or the other so
the code and guideline stay consistent).
In `@cli/reins/src/lib/commands/evolve.ts`:
- Around line 219-252: The current applyEvolveScaffolding flow only triggers
runInit when AGENTS.md is missing, causing base docs to be skipped if AGENTS.md
exists but other base files are absent; update the needsInitScaffold detection
in applyEvolveScaffolding (or extract a helper) to check for the presence of
required base artifacts (e.g., docs/exec-plans and other base docs referenced by
EvolutionPath.steps) in addition to AGENTS.md, and if any are missing call
runInit({ path: targetPath, name: "", force: false, pack: initPack }) so init
runs without overwriting AGENTS.md; alternatively implement a base-scaffold
helper that returns whether missing files exist and invoke runInit when it
reports gaps, keeping the existing scaffoldAutomationPack("agent-factory")
behavior intact.
- Around line 263-282: The function buildPackRecommendationOutput returns a
misleading reason when a pack is not recommended due to maturity gating: update
it so that when packState.shouldRecommendFactoryPack is false and
packState.hasFactoryPack is false you override
packState.packRecommendation.reason with a clear suppression message (e.g.
"Agent-factory recommendation suppressed by maturity level") instead of the
active-recommendation text; keep existing behavior for the hasFactoryPack branch
and when shouldRecommendFactoryPack is true, and reference the function name
buildPackRecommendationOutput and the flags
packState.shouldRecommendFactoryPack, packState.hasFactoryPack, and
packState.packRecommendation.reason to locate where to apply the change.
In `@cli/reins/src/lib/templates.ts`:
- Around line 244-276: The high-risk path strings in riskPolicyTemplate (when
pack === "agent-factory") include trailing slashes which cause the matching
logic (file.startsWith(pattern + '/')) to produce double slashes and miss
matches; remove trailing slashes from entries in "riskTierRules.high" (e.g.,
change "src/security/" -> "src/security", "src/auth/" -> "src/auth",
".github/workflows/" -> ".github/workflows", etc.) and likewise strip trailing
slashes from patterns in "docsDriftRules.watchPaths" so the gate's
startsWith(pattern + '/') comparisons work correctly.
---
Outside diff comments:
In `@cli/reins/src/index.ts`:
- Around line 8-46: printHelp currently prints plain text which violates the
deterministic JSON output rule; modify the printHelp function to emit a single
JSON payload instead (e.g., a plain object with keys like "name", "usage",
"commands", "options", "examples", "maturityLevels") and output it via
console.log(JSON.stringify(helpPayload)) so consumers can parse it reliably;
ensure fields are consistently ordered/stable (explicitly construct the object
with fixed keys) and remove or replace the plain-text console.log(help) call so
only deterministic JSON is emitted.
---
Duplicate comments:
In `@README.md`:
- Around line 291-294: The README's "Project structure" section lists a modular
layout that contradicts the coding guideline requiring all CLI logic to live in
a single file (cli/reins/src/index.ts); update this section to reflect the
single-file CLI design by removing or replacing references to src/lib/commands,
src/lib/audit, and other multi-file directories and instead document that CLI
entrypoint and handlers reside in cli/reins/src/index.ts (you can keep
package.json and any top-level files listed).
- Around line 15-29: Update the README's "CLI" row to reflect the single-file
CLI rule: replace the current "CLI" source-of-truth value
(`cli/reins/src/lib/commands/` (+ routed via `cli/reins/src/index.ts`) ) with
the enforced single file `cli/reins/src/index.ts`, and remove or reword any
language implying modular command handlers so it matches the index.ts comment
and the coding guideline "All CLI logic must live in a single file at
cli/reins/src/index.ts".
---
Nitpick comments:
In `@ARCHITECTURE.md`:
- Line 15: Add missing fenced-code language specifiers to the two code blocks in
ARCHITECTURE.md: the directory tree block containing "cli/reins/src/" and the
flow-diagram block containing "lib/helpers + lib/commands → index.ts router →
CLI output"; update each triple-backtick fence to include a language like text
or plaintext (e.g., ```text) so the linter stops flagging them.
In `@cli/reins/src/lib/audit/scoring.ts`:
- Around line 265-274: The function scoreSinglePackageDependencyFootprint
currently calls readFileSync + JSON.parse without local error handling; wrap the
file read/parse in a try/catch inside scoreSinglePackageDependencyFootprint and
handle parse/io errors locally (e.g., add a descriptive finding to
result.scores.agent_legibility.findings and avoid throwing) so the function is
robust if called directly; ensure you reference the same result and ctx
parameters and preserve existing behavior for the depCount path while returning
early on error.
In `@cli/reins/src/lib/templates.ts`:
- Around line 1-6: Remove the duplicate AutomationPack type declaration in
templates.ts and re-export the shared type from the existing module instead:
replace the local "export type AutomationPack = ..." with a re-export such as
"export { AutomationPack } from './automation-pack';" (or "export type {
AutomationPack } from './automation-pack';" if you prefer type-only export),
leaving AgentFactoryPackFile intact and updating any local imports/usages in
this file to reference the re-exported AutomationPack.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (28)
AGENTS.mdARCHITECTURE.mdREADME.mdcli/reins/README.mdcli/reins/package.jsoncli/reins/src/index.test.tscli/reins/src/index.tscli/reins/src/lib/audit/context.tscli/reins/src/lib/audit/scoring.tscli/reins/src/lib/automation-pack.tscli/reins/src/lib/commands/audit.tscli/reins/src/lib/commands/doctor.tscli/reins/src/lib/commands/evolve.tscli/reins/src/lib/commands/init.tscli/reins/src/lib/detection.tscli/reins/src/lib/filesystem.tscli/reins/src/lib/scoring-utils.tscli/reins/src/lib/templates.tscli/reins/src/lib/types.tsdocs/design-docs/ecosystem-positioning.mddocs/design-docs/skill-evals-and-shell-boundaries.mddocs/exec-plans/tech-debt-tracker.mdpackage.jsonskill/Reins/SKILL.mdskill/Reins/Workflows/Audit.mdskill/Reins/Workflows/Doctor.mdskill/Reins/Workflows/Evolve.mdskill/Reins/Workflows/Scaffold.md
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@cli/reins/src/lib/commands/evolve.ts`:
- Around line 238-272: The gating on step.action (needsInitScaffold) can prevent
scaffolding when base artifacts are missing; update applyEvolveScaffolding so it
triggers runInit whenever hasMissingBaseScaffold(targetDir) is true (i.e.,
remove or bypass the action-string check), using the same initPack selection and
runInit call currently present (refer to needsInitScaffold, missingBaseScaffold,
initPack, and runInit in applyEvolveScaffolding), and keep the existing
applied.push messages and early return behavior unchanged.
In `@cli/reins/src/lib/commands/init.ts`:
- Around line 144-172: The CLI command handler implemented in runInit should not
live in this module; either move the entire CLI logic (the body of runInit that
calls buildInitContext, ensureInitTarget, resolveAutomationPack,
createBaseDirectories, createBaseFiles, scaffoldAutomationPack and console.log
of buildNextSteps output) into the central CLI entry (cli/reins/src/index.ts)
and invoke it there, or refactor this file so runInit becomes a pure library
function that returns the result object (project, target, selected pack, reason,
created, next_steps) and remove any direct console/logging; then update
cli/reins/src/index.ts to import and perform the actual CLI I/O and printing.
Ensure only index.ts contains the CLI side-effects (console.log/process exit)
and this file exports non-CLI helpers if needed.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
CLAUDE.mdcli/reins/src/index.test.tscli/reins/src/lib/commands/evolve.tscli/reins/src/lib/commands/init.tscli/reins/src/lib/templates.tscli/reins/src/lib/types.ts
✅ Files skipped from review due to trivial changes (1)
- CLAUDE.md
🚧 Files skipped from review as they are similar to previous changes (1)
- cli/reins/src/lib/types.ts
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
cli/reins/src/lib/commands/evolve.ts (1)
346-346: Optional: renamepathtoevolutionPathto reduce semantic ambiguity.
pathis a common alias for filesystem paths elsewhere in the file (e.g.,targetPath). Renaming theEvolutionPathlocal here makes the intent unambiguous with minimal churn.♻️ Proposed rename
- const path = EVOLUTION_PATHS[currentKey]; - const packState = buildPackState(targetDir, currentKey); - const steps = buildEvolutionSteps(path, packState.shouldRecommendFactoryPack); + const evolutionPath = EVOLUTION_PATHS[currentKey]; + const packState = buildPackState(targetDir, currentKey); + const steps = buildEvolutionSteps(evolutionPath, packState.shouldRecommendFactoryPack); const applied = applyEvolveScaffolding( ... ); console.log( JSON.stringify( { ... - next_level: path.to, - goal: path.goal, + next_level: evolutionPath.to, + goal: evolutionPath.goal, steps, - success_criteria: path.success_criteria, + success_criteria: evolutionPath.success_criteria,🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cli/reins/src/lib/commands/evolve.ts` at line 346, Rename the local variable currently declared as "path" when assigned from EVOLUTION_PATHS[currentKey] to "evolutionPath" to avoid ambiguity with other filesystem "path" variables (e.g., targetPath); update every use of that identifier in the evolve.ts scope (references to path, including any passed to functions or returned) to evolutionPath and ensure imports/types are unaffected.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@cli/reins/src/lib/commands/evolve.ts`:
- Around line 238-268: applyEvolveScaffolding currently calls runInit and
scaffoldAutomationPack without protection, so any thrown error will bypass the
JSON audit error contract; wrap the runInit call and the scaffoldAutomationPack
call in try/catch blocks inside applyEvolveScaffolding and on catch invoke the
same audit-style exit used elsewhere (e.g., call getAuditResultOrExit with a
structured error object containing a concise message and error details, then
exit with nonzero) so failures produce the consistent JSON error + exit(1)
instead of an unstructured stack trace.
---
Nitpick comments:
In `@cli/reins/src/lib/commands/evolve.ts`:
- Line 346: Rename the local variable currently declared as "path" when assigned
from EVOLUTION_PATHS[currentKey] to "evolutionPath" to avoid ambiguity with
other filesystem "path" variables (e.g., targetPath); update every use of that
identifier in the evolve.ts scope (references to path, including any passed to
functions or returned) to evolutionPath and ensure imports/types are unaffected.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
cli/reins/src/lib/commands/evolve.ts (1)
181-187: Score-to-level boundaries look intentional but are undocumented.The thresholds (≤4 → L0, ≤8 → L1, ≤13 → L2, ≤16 → L3, ≥17 → L4) determine the entire evolve path. Consider adding a brief inline comment or a named constant mapping explaining the rationale — especially the asymmetric gap sizes (5, 5, 4, 2) — so future maintainers don't accidentally shift them.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cli/reins/src/lib/commands/evolve.ts` around lines 181 - 187, The score-to-level thresholds in resolveCurrentLevelKey (mapping totalScore ≤4→"L0", ≤8→"L1", ≤13→"L2", ≤16→"L3", else "L4") need an explanatory note and/or a named constant mapping to document the rationale for the asymmetric gaps; update the function by extracting these numeric boundaries into a clearly named constant or an ordered array (e.g., LEVEL_THRESHOLDS or SCORE_TO_LEVEL_MAP) and add a concise inline comment describing why each breakpoint exists (or linking to design docs), then use that constant in resolveCurrentLevelKey so future maintainers can see the intent without guessing.cli/reins/src/index.test.ts (2)
426-465: Console.log mock captures only the last call and accepts a single argument.The mock
(value?: unknown) => { output = String(value ?? ""); }overwritesoutputon every call and ignores extra arguments. This works today becauserunEvolveemits exactly oneconsole.log(JSON.stringify(...))call on the non-L4/non-error path withrunApply: false. If the implementation ever adds diagnostic logging, this test will silently capture only the last message.A minor hardening option:
💡 Suggested improvement
- let output = ""; - const originalLog = console.log; - console.log = (value?: unknown) => { - output = String(value ?? ""); - }; + const logs: string[] = []; + const originalLog = console.log; + console.log = (...args: unknown[]) => { + logs.push(args.map(String).join(" ")); + }; ... - const result = JSON.parse(output); + const result = JSON.parse(logs[logs.length - 1]);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cli/reins/src/index.test.ts` around lines 426 - 465, The console.log mock in this test overwrites output on each call and only accepts a single argument, making the test brittle if runEvolve emits diagnostic logs; modify the mock used around runEvolve to capture all console.log calls (e.g., push each call's joined args into an array) and then locate the JSON.stringify call (or the first parseable JSON entry) to JSON.parse for assertions—update the variables `output`/`originalLog` and the mock replacement of console.log used in this test ("suppresses pack recommendation reason when maturity gating defers agent-factory") so it accumulates calls rather than overwriting them before parsing.
398-412: Fragile JSON extraction from mixed stdout.Line 405 uses
stdout.lastIndexOf('{\n "command": "evolve"')to locate the evolve output amidst potential earlier init output. This couples the test to the exactJSON.stringify(…, null, 2)indentation. If the output format changes (compact JSON, different indent), this test silently breaks.Consider having the CLI separate init output from evolve output more cleanly (e.g., only emit the final evolve JSON on stdout and route init output to stderr), or at least extract the magic string to a constant.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cli/reins/src/index.test.ts` around lines 398 - 412, The test's JSON extraction is fragile because evolveJsonStart uses a hardcoded pretty-printed prefix; update the test around runCli/stdout/evolveJsonStart to robustly locate the evolve JSON by using a regex that finds the JSON object containing the "command":"evolve" key (eg. search for /{[\s\S]*?"command"\s*:\s*"evolve"[\s\S]*?}/) or by finding the index of the "command":"evolve" substring then scanning backward to the preceding '{' and forward to the matching '}' before calling JSON.parse; alternatively extract that magic prefix into a named constant used in the search so formatting changes don't break the test.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@cli/reins/src/index.test.ts`:
- Around line 5-6: The unit-level test(s) that import runEvolve and the
AuditResult type should be moved out of index.test.ts and into a co-located test
next to the evolve implementation (create
cli/reins/src/lib/commands/evolve.test.ts), so relocate the mock-based test that
directly calls runEvolve (and any uses of AuditResult) into that new file; leave
only integration tests that exercise the CLI via runCli in index.test.ts. Update
imports in the moved test to reference local ./evolve (or the correct relative
path to evolve.ts) and ensure mocks are set up there, and remove the direct
runEvolve/AuditResult imports from index.test.ts.
---
Nitpick comments:
In `@cli/reins/src/index.test.ts`:
- Around line 426-465: The console.log mock in this test overwrites output on
each call and only accepts a single argument, making the test brittle if
runEvolve emits diagnostic logs; modify the mock used around runEvolve to
capture all console.log calls (e.g., push each call's joined args into an array)
and then locate the JSON.stringify call (or the first parseable JSON entry) to
JSON.parse for assertions—update the variables `output`/`originalLog` and the
mock replacement of console.log used in this test ("suppresses pack
recommendation reason when maturity gating defers agent-factory") so it
accumulates calls rather than overwriting them before parsing.
- Around line 398-412: The test's JSON extraction is fragile because
evolveJsonStart uses a hardcoded pretty-printed prefix; update the test around
runCli/stdout/evolveJsonStart to robustly locate the evolve JSON by using a
regex that finds the JSON object containing the "command":"evolve" key (eg.
search for /{[\s\S]*?"command"\s*:\s*"evolve"[\s\S]*?}/) or by finding the index
of the "command":"evolve" substring then scanning backward to the preceding '{'
and forward to the matching '}' before calling JSON.parse; alternatively extract
that magic prefix into a named constant used in the search so formatting changes
don't break the test.
In `@cli/reins/src/lib/commands/evolve.ts`:
- Around line 181-187: The score-to-level thresholds in resolveCurrentLevelKey
(mapping totalScore ≤4→"L0", ≤8→"L1", ≤13→"L2", ≤16→"L3", else "L4") need an
explanatory note and/or a named constant mapping to document the rationale for
the asymmetric gaps; update the function by extracting these numeric boundaries
into a clearly named constant or an ordered array (e.g., LEVEL_THRESHOLDS or
SCORE_TO_LEVEL_MAP) and add a concise inline comment describing why each
breakpoint exists (or linking to design docs), then use that constant in
resolveCurrentLevelKey so future maintainers can see the intent without
guessing.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
docs/design-docs/skill-evals-and-shell-boundaries.md (1)
72-72: Consider extending the error shape beyond a bare string to support programmatic discrimination.The
{ "error": "..." }contract is the minimum viable shape. Adding an optionalcodeortypefield would let parsers branch on error class without string-matching the message, which trade-off item 3 (line 90) itself calls fragile.💡 Suggested richer shape
-3. Keep message shape stable (`{ "error": "..." }`) so skill parsers can rely on it. +3. Keep message shape stable (`{ "error": "...", "code": "<ERROR_CODE>" }`) so + skill parsers can branch on error class without string-matching message text.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/design-docs/skill-evals-and-shell-boundaries.md` at line 72, Update the error contract described as { "error": "..." } to a richer, backwards-compatible shape so skill parsers can programmatically discriminate errors; specifically, modify the schema and any producing functions to optionally include fields like "code" (string or enum) and/or "type" (string) alongside "error", and ensure consumers (skill parsers and serializers) read these optional fields when present while still accepting the original { "error": "..." } form; locate references to the contract by searching for the literal "{ \"error\": \"...\" }", usages in skill parser code, and any serialization helpers to implement the change.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/design-docs/skill-evals-and-shell-boundaries.md`:
- Around line 70-72: Update the document to explicitly state the parser-side
stderr contract: add a fourth bullet after items 1–3 saying skill parsers must
capture and parse both stdout and stderr (stderr will carry structured JSON
error objects with the stable shape `{ "error": "..." }`) so recoverable
failures are not missed; reference the existing terminology ("stderr JSON
objects", "message shape", and "skill parsers") and give a short directive like
"capture both streams / parse stderr for structured failures" so consumers know
to read stderr separately from stdout.
---
Nitpick comments:
In `@docs/design-docs/skill-evals-and-shell-boundaries.md`:
- Line 72: Update the error contract described as { "error": "..." } to a
richer, backwards-compatible shape so skill parsers can programmatically
discriminate errors; specifically, modify the schema and any producing functions
to optionally include fields like "code" (string or enum) and/or "type" (string)
alongside "error", and ensure consumers (skill parsers and serializers) read
these optional fields when present while still accepting the original { "error":
"..." } form; locate references to the contract by searching for the literal "{
\"error\": \"...\" }", usages in skill parser code, and any serialization
helpers to implement the change.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
docs/design-docs/index.mddocs/design-docs/skill-evals-and-shell-boundaries.md
✅ Files skipped from review due to trivial changes (1)
- docs/design-docs/index.md
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
Refactors the Reins CLI from a large single implementation into modular command and audit layers while preserving deterministic JSON behavior and score semantics. Updates docs and skill workflow references to match the new architecture and execution paths.
Changes
cli/reins/src/lib/*, including command handlers (init,audit,doctor,evolve), shared types, and utility modules.cli/reins/src/lib/audit/context.ts,cli/reins/src/lib/audit/scoring.ts) and keptcli/reins/src/index.tsas a thin router.docs/exec-plans/tech-debt-tracker.mdto reflect completed CLI modularization.Testing
bun testpassesbun src/index.ts audit ../..score maintained or improvedAudit Impact
Audit score maintained at 18/18 (
L4: Self-Correcting) before and after modularization.Summary by CodeRabbit
New Features
Documentation
Tests
Chores