refactor: bounded extraction for mutation coverage hardening#384
refactor: bounded extraction for mutation coverage hardening#384
Conversation
Extract `resolveRegistryDir` and `computeAverageConfidence` as exported pure functions from the monolithic `compute()` method. Add 12 targeted unit tests covering: - agentDir vs katakaDir resolution (precedence, fallback, throw) - average confidence arithmetic (empty, single, multi, boundary) - load() throw path when no registry dir configured Addresses #375 Phase 3 — bounded extraction for mutation coverage. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Remove two dead-code guards that masked 6 surviving mutants:
- existsSync before mkdirSync({recursive:true}) is idempotent
- existsSync before JsonStore.read in load() has error handler fallback
Eliminates all 6 survivors by removing untestable redundant branches.
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…m cooldown-session Extract isJsonFile, isSynthesisPendingFile, hasFailedCaptures as pure helpers with direct unit tests. Remove 3 redundant existsSync guards in bridge-run loading where readBridgeRunMeta already handles missing files via its own error handler. Addresses #375 Phase 3 — cooldown-session bounded extraction. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…Status from execute.ts Move three pure business-logic functions out of CLI command wiring into execute.helpers.ts with direct unit tests. Eliminates 4+ LogicalOperator and ConditionalExpression mutation survivors in execute.ts. Addresses #375 Phase 2 — execute helper extraction. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…ty aggregator Extract run-directory listing and latest-run comparison as pure exported functions. Remove redundant existsSync guard. Add tests for directory filtering (files vs dirs) and multi-run latest tracking. Addresses #375 Phase 3 — observability aggregator extraction. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
This guard is semantically meaningful — it distinguishes "file missing" (undefined, triggers run.json fallback) from "file found but run not in-progress" (null, no fallback). Removing it broke the checkIncompleteRuns fallthrough to run.json for runs without bridge metadata. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…m aggregator Reduce cyclomatic complexity of computeStats by extracting agent attribution check and observation counting as pure functions. Adds direct unit tests for both. Targets CRAP strict threshold for computeStats (CC 9 -> lower effective complexity). Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (6)
📝 WalkthroughWalkthroughAdds numerous small, pure helper functions and corresponding tests across CLI, cycle-management, kata-agent, and session-bridge modules; existing inline predicates/logic are replaced with these helpers and call sites updated accordingly. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 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🧪 Generate unit tests (beta)
📝 Coding Plan
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 Tip You can disable sequence diagrams in the walkthrough.Disable the |
Create session-bridge.helpers.ts with canTransitionCycleState, hasBridgeRunMetadataChanged, and isJsonFile. Wire into session-bridge.ts replacing inline logic. Add 13 unit tests covering state transitions, metadata change detection, and file filtering. Addresses #375 Phase 3 — session-bridge bounded extraction. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Replace prototype-hacking access to private canTransitionCycleState with direct import from session-bridge.helpers.ts. 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}
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 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/execute.helpers.test.ts`:
- Around line 428-445: Add a focused test case to assert the precedence behavior
of resolveJsonFlag when local is explicitly false but global is true: call
resolveJsonFlag(false, true) and expect true; this ensures the function returns
true for the global flag even when local is false and prevents a boolean-mutant
survivor. Locate the resolveJsonFlag tests in execute.helpers.test.ts (the
existing describe('resolveJsonFlag') block) and add the new it(...) assertion
alongside the other cases to cover the local=false, global=true edge case.
In `@src/features/kata-agent/kata-agent-confidence-calculator.test.ts`:
- Around line 5-9: The import in the test uses a relative path for
KataAgentConfidenceCalculator, resolveRegistryDir, and computeAverageConfidence;
replace the relative import ("./kata-agent-confidence-calculator.js") with the
project path alias (e.g. `@features/kata-agent/kata-agent-confidence-calculator`)
so the named imports remain identical and consistent with the project's alias
convention.
In `@src/features/kata-agent/kata-agent-confidence-calculator.ts`:
- Line 9: Replace the relative import for the KataAgentObservabilityAggregator
with the project path alias: update the import of
KataAgentObservabilityAggregator in kata-agent-confidence-calculator.ts to use
'@features/kata-agent/kata-agent-observability-aggregator.js' (keeping the same
exported symbol name) so the file follows the project's import conventions.
In `@src/features/kata-agent/kata-agent-observability-aggregator.test.ts`:
- Around line 5-11: Replace the relative import of
KataAgentObservabilityAggregator, countObservationsByType, isAttributedToAgent,
isNewerRun, and listRunDirectoryIds from
'./kata-agent-observability-aggregator.js' with the project path alias for that
feature (e.g. the alias that maps to the features/kata-agent module) and omit
the explicit .js extension; update the import statement in
kata-agent-observability-aggregator.test.ts to use the path alias (so the same
named exports are imported via the alias), ensuring the alias is the one
configured in your tsconfig/paths or bundler.
In `@src/features/kata-agent/kata-agent-observability-aggregator.ts`:
- Around line 52-62: The function countObservationsByType manually increments
count inside the loop even though it always equals observations.length; change
the implementation to compute count = observations.length (keep the existing
for-loop to build byType) or remove the manual increment and set count after the
loop, updating the function return accordingly to avoid unnecessary mutation of
the count variable in countObservationsByType while preserving byType
aggregation.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 0dac2d9f-5520-416b-96d2-96be23d25414
📒 Files selected for processing (10)
src/cli/commands/execute.helpers.test.tssrc/cli/commands/execute.helpers.tssrc/cli/commands/execute.tssrc/features/cycle-management/cooldown-session.helpers.test.tssrc/features/cycle-management/cooldown-session.helpers.tssrc/features/cycle-management/cooldown-session.tssrc/features/kata-agent/kata-agent-confidence-calculator.test.tssrc/features/kata-agent/kata-agent-confidence-calculator.tssrc/features/kata-agent/kata-agent-observability-aggregator.test.tssrc/features/kata-agent/kata-agent-observability-aggregator.ts
📜 Review details
🧰 Additional context used
📓 Path-based instructions (2)
src/**/*.ts
📄 CodeRabbit inference engine (CLAUDE.md)
src/**/*.ts: All types must be defined as Zod schemas usingzod/v4import path, not separate interface definitions
Use ESM-only format with.jsextensions on all internal imports
Use path aliases for imports:@domain/,@infra/,@features/,@shared/,@cli/* instead of relative paths
Files:
src/features/kata-agent/kata-agent-confidence-calculator.test.tssrc/features/kata-agent/kata-agent-confidence-calculator.tssrc/features/cycle-management/cooldown-session.helpers.tssrc/features/cycle-management/cooldown-session.tssrc/features/kata-agent/kata-agent-observability-aggregator.test.tssrc/features/kata-agent/kata-agent-observability-aggregator.tssrc/cli/commands/execute.helpers.test.tssrc/features/cycle-management/cooldown-session.helpers.test.tssrc/cli/commands/execute.tssrc/cli/commands/execute.helpers.ts
src/**/*.test.ts
📄 CodeRabbit inference engine (CLAUDE.md)
Colocate test files with implementation files using
*.test.tsnaming convention
Files:
src/features/kata-agent/kata-agent-confidence-calculator.test.tssrc/features/kata-agent/kata-agent-observability-aggregator.test.tssrc/cli/commands/execute.helpers.test.tssrc/features/cycle-management/cooldown-session.helpers.test.ts
🧠 Learnings (5)
📓 Common learnings
Learnt from: CR
Repo: cmbays/kata PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-03-15T01:51:21.342Z
Learning: Focused mutation testing must be run for changed business logic or orchestration seams with meaningful survivors explicitly called out
Learnt from: CR
Repo: cmbays/kata PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-03-15T01:51:21.342Z
Learning: Run focused mutation testing on changed behavior-heavy modules
📚 Learning: 2026-03-15T01:51:21.342Z
Learnt from: CR
Repo: cmbays/kata PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-03-15T01:51:21.342Z
Learning: Applies to src/**/{cli,orchestrat}*.ts : Prefer extracting pure helpers from CLI/orchestration files before adding more direct tests to large side-effect-heavy modules
Applied to files:
src/features/kata-agent/kata-agent-confidence-calculator.test.tssrc/features/kata-agent/kata-agent-observability-aggregator.test.tssrc/features/kata-agent/kata-agent-observability-aggregator.tssrc/cli/commands/execute.helpers.test.tssrc/features/cycle-management/cooldown-session.helpers.test.tssrc/cli/commands/execute.tssrc/cli/commands/execute.helpers.ts
📚 Learning: 2026-03-15T01:51:21.342Z
Learnt from: CR
Repo: cmbays/kata PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-03-15T01:51:21.342Z
Learning: Applies to src/**/*.command.test.ts : Keep command-level tests focused on wiring and delegation; put parsing/formatting/selection behavior into direct unit tests
Applied to files:
src/cli/commands/execute.helpers.test.tssrc/features/cycle-management/cooldown-session.helpers.test.ts
📚 Learning: 2026-03-15T01:51:09.797Z
Learnt from: CR
Repo: cmbays/kata PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-15T01:51:09.797Z
Learning: Applies to src/**/*.test.ts : Colocate test files with implementation files using `*.test.ts` naming convention
Applied to files:
src/features/cycle-management/cooldown-session.helpers.test.ts
📚 Learning: 2026-03-15T01:51:21.342Z
Learnt from: CR
Repo: cmbays/kata PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-03-15T01:51:21.342Z
Learning: Run `npm run test:coverage:unit && npm run test:crap` after driving implementation with tests
Applied to files:
src/features/cycle-management/cooldown-session.helpers.test.ts
🔇 Additional comments (30)
src/features/cycle-management/cooldown-session.helpers.test.ts (1)
380-416: LGTM!The new tests for
isJsonFile,isSynthesisPendingFile, andhasFailedCapturesprovide good coverage of typical inputs and edge cases (empty strings, wrong prefixes/suffixes, boundary values). Test file is properly colocated with implementation per coding guidelines.src/features/cycle-management/cooldown-session.helpers.ts (1)
296-306: LGTM!The three new pure helper functions are well-scoped and improve testability:
isJsonFile— simple suffix checkisSynthesisPendingFile— combined prefix/suffix checkhasFailedCaptures— numeric predicateThese extractions align with the learning to "prefer extracting pure helpers from CLI/orchestration files before adding more direct tests to large side-effect-heavy modules."
src/features/cycle-management/cooldown-session.ts (4)
56-58: LGTM!New helper imports are correctly added and follow the ESM
.jsextension convention.
843-851: LGTM!The refactor to use
isSynthesisPendingFileas the filter predicate is clean and maintains the same behavior.
888-894: LGTM!Clean refactor using
isJsonFileas the filter predicate.
990-997: LGTM!The refactor to use
hasFailedCaptures(attempts.failed)correctly replaces the inline comparison while maintaining identical semantics.src/features/kata-agent/kata-agent-observability-aggregator.ts (6)
1-6: LGTM!Imports correctly use path aliases (
@domain/*,@infra/*) with.jsextensions as required by the coding guidelines.
11-35: LGTM!The interface is well-documented with JSDoc comments explaining each field's purpose. Since this is an internally-constructed stats object (not parsed from external input), using a TypeScript interface here is reasonable.
41-43: LGTM!Clean extraction of the recency comparison logic. Lexicographic comparison is valid for ISO 8601 date strings, and the strict inequality correctly handles equal timestamps.
45-50: LGTM!Clear attribution logic with proper fallback from
agentIdto legacykatakaId. The nullish coalescing correctly handles the migration path between naming conventions.
64-72: LGTM!Efficient implementation using
withFileTypesfor single-syscall directory listing. Error handling gracefully returns an empty array for missing directories, eliminating untestable branches in the caller.
108-158: LGTM!Clean integration of the extracted helpers into
computeStats. The helper calls are correctly placed:
listRunDirectoryIdsfor directory discoveryisAttributedToAgentfor both run and observation filteringisNewerRunfor tracking the most recent runcountObservationsByTypefor aggregating observation countsThe
lastActiveAtpropagation at line 132 correctly captures the timestamp when a newer run is found.src/features/kata-agent/kata-agent-observability-aggregator.test.ts (5)
21-53: LGTM!Test helpers are well-structured with proper isolation using
randomUUID()for unique directories. The factory functions provide sensible defaults with override capability.
59-219: LGTM!Comprehensive test coverage for
computeStats:
- Edge cases: empty/nonexistent directories, non-matching attribution
- Core functionality: run-level and stage-level observations, observation filtering
- Tracking:
lastRunId,lastRunCycleId,lastActiveAtselection- Aggregation: multiple runs with proper count accumulation
221-315: LGTM!Good coverage of
KnowledgeStoreintegration including the important edge case of archived learnings being excluded.
317-334: Minor overlap with existing test at lines 171-186.Both test
lastRunId/lastActiveAttracking with similar scenarios. This is acceptable for mutation coverage hardening, but could be consolidated in future cleanup.
341-420: LGTM!Excellent unit test coverage for the pure helpers:
isNewerRun: All boundary cases (undefined, later, earlier, equal timestamps)isAttributedToAgent: All attribution scenarios including preference behaviorcountObservationsByType: Empty, multiple types, single observationlistRunDirectoryIds: Directory filtering and nonexistent path handlingThis aligns well with the PR objective of improving mutation coverage through extracted pure helpers. Based on learnings, "Prefer extracting pure helpers from CLI/orchestration files before adding more direct tests to large side-effect-heavy modules."
src/features/kata-agent/kata-agent-confidence-calculator.ts (4)
15-26: LGTM!The
resolveRegistryDirhelper is clean with clear fallback semantics and an informative error message. The relevant code snippets confirm that callers (CooldownSession, agent.ts, cycle.ts) properly provideagentDirbefore invoking methods that use this helper.
28-35: LGTM!The
computeAverageConfidencehelper correctly handles the empty array edge case and usesReadonlyArrayto communicate immutability intent. The arithmetic is straightforward and testable.
80-83: LGTM!Good refactoring:
mkdirSyncwith{ recursive: true }is idempotent and eliminates the untestableexistsSyncbranch while preserving correctness. Using the extracted helpers improves testability without changing behavior.
91-93: LGTM!Consistent use of
resolveRegistryDirinload()ensures the same resolution logic applies to both compute and load paths.src/features/kata-agent/kata-agent-confidence-calculator.test.ts (4)
76-94: LGTM!Excellent branch coverage for
resolveRegistryDir: all four combinations ofagentDir/katakaDirpresence are tested, including the error case with exact message verification.
96-119: LGTM!Solid test coverage for
computeAverageConfidencewith appropriate edge cases (empty, single, multiple, all-zero, all-one). UsingtoBeCloseTofor floating-point comparison is the right approach. Based on learnings, this aligns with the goal of focused mutation testing on changed business logic.
234-276: LGTM!These tests comprehensively verify the
agentDirvskatakaDirresolution behavior:
- Priority given to
agentDirwhen both are providedagentDirworks standalone without legacykatakaDir- Error thrown when neither is provided
The negative assertion on line 252 (
expect(existsSync(...katakaDir...)).toBe(false)) is a good practice to confirm priority behavior.
316-348: LGTM!Good coverage for
load()with the new directory resolution. The test on lines 325-348 properly verifies that when both directories are provided,load()reads fromagentDir(wherecompute()wrote the file).src/cli/commands/execute.helpers.test.ts (2)
3-3: Import wiring is clean and guideline-compliant.The new helper imports are correctly aliased and use ESM
.jsextension consistently.As per coding guidelines: "Use ESM-only format with
.jsextensions on all internal imports" and "Use path aliases ... instead of relative paths".Also applies to: 14-16
447-478: Helper test coverage is solid for status symbol and completion status branches.Good extraction-level tests with explicit branch assertions and fallback behavior checks.
Based on learnings: "Prefer extracting pure helpers from CLI/orchestration files before adding more direct tests to large side-effect-heavy modules".
src/cli/commands/execute.helpers.ts (1)
313-326: Extracted helper implementations look correct and intentionally minimal.These pure helpers are deterministic and align with their new unit tests and call sites.
src/cli/commands/execute.ts (2)
24-39: JSON output flag resolution is now consistently centralized.Good replacement of duplicated boolean checks with a single helper across command paths.
Also applies to: 108-108, 195-195, 251-251, 273-273
149-150: Status/symbol helper adoption is coherent across text and JSON outputs.Nice consolidation—this reduces drift between CLI display and machine-readable status fields.
Based on learnings: "Prefer extracting pure helpers from CLI/orchestration files before adding more direct tests to large side-effect-heavy modules".
Also applies to: 232-233, 239-239
| import { | ||
| KataAgentConfidenceCalculator, | ||
| resolveRegistryDir, | ||
| computeAverageConfidence, | ||
| } from './kata-agent-confidence-calculator.js'; |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider using path alias for consistency.
The import uses a relative path while other imports in this file use path aliases. For consistency with project conventions, consider using the path alias.
Suggested fix
import {
KataAgentConfidenceCalculator,
resolveRegistryDir,
computeAverageConfidence,
-} from './kata-agent-confidence-calculator.js';
+} from '@features/kata-agent/kata-agent-confidence-calculator.js';As per coding guidelines: "Use path aliases for imports: @domain/, @infra/, @features/, @shared/, @cli/* instead of relative paths"
📝 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.
| import { | |
| KataAgentConfidenceCalculator, | |
| resolveRegistryDir, | |
| computeAverageConfidence, | |
| } from './kata-agent-confidence-calculator.js'; | |
| import { | |
| KataAgentConfidenceCalculator, | |
| resolveRegistryDir, | |
| computeAverageConfidence, | |
| } from '@features/kata-agent/kata-agent-confidence-calculator.js'; |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/features/kata-agent/kata-agent-confidence-calculator.test.ts` around
lines 5 - 9, The import in the test uses a relative path for
KataAgentConfidenceCalculator, resolveRegistryDir, and computeAverageConfidence;
replace the relative import ("./kata-agent-confidence-calculator.js") with the
project path alias (e.g. `@features/kata-agent/kata-agent-confidence-calculator`)
so the named imports remain identical and consistent with the project's alias
convention.
| @@ -8,6 +8,32 @@ import { | |||
| } from '@domain/types/kata-agent-confidence.js'; | |||
| import { KataAgentObservabilityAggregator } from './kata-agent-observability-aggregator.js'; | |||
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Use path alias instead of relative import.
The import should use @features/kata-agent/kata-agent-observability-aggregator.js to comply with the project's import conventions. Other imports in this file correctly use path aliases.
Suggested fix
-import { KataAgentObservabilityAggregator } from './kata-agent-observability-aggregator.js';
+import { KataAgentObservabilityAggregator } from '@features/kata-agent/kata-agent-observability-aggregator.js';As per coding guidelines: "Use path aliases for imports: @domain/, @infra/, @features/, @shared/, @cli/* instead of relative paths"
📝 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.
| import { KataAgentObservabilityAggregator } from './kata-agent-observability-aggregator.js'; | |
| import { KataAgentObservabilityAggregator } from '@features/kata-agent/kata-agent-observability-aggregator.js'; |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/features/kata-agent/kata-agent-confidence-calculator.ts` at line 9,
Replace the relative import for the KataAgentObservabilityAggregator with the
project path alias: update the import of KataAgentObservabilityAggregator in
kata-agent-confidence-calculator.ts to use
'@features/kata-agent/kata-agent-observability-aggregator.js' (keeping the same
exported symbol name) so the file follows the project's import conventions.
| import { | ||
| KataAgentObservabilityAggregator, | ||
| countObservationsByType, | ||
| isAttributedToAgent, | ||
| isNewerRun, | ||
| listRunDirectoryIds, | ||
| } from './kata-agent-observability-aggregator.js'; |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Use path alias instead of relative import.
As per coding guidelines, use path aliases instead of relative paths for internal imports.
♻️ Proposed fix
import {
KataAgentObservabilityAggregator,
countObservationsByType,
isAttributedToAgent,
isNewerRun,
listRunDirectoryIds,
-} from './kata-agent-observability-aggregator.js';
+} from '@features/kata-agent/kata-agent-observability-aggregator.js';📝 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.
| import { | |
| KataAgentObservabilityAggregator, | |
| countObservationsByType, | |
| isAttributedToAgent, | |
| isNewerRun, | |
| listRunDirectoryIds, | |
| } from './kata-agent-observability-aggregator.js'; | |
| import { | |
| KataAgentObservabilityAggregator, | |
| countObservationsByType, | |
| isAttributedToAgent, | |
| isNewerRun, | |
| listRunDirectoryIds, | |
| } from '@features/kata-agent/kata-agent-observability-aggregator.js'; |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/features/kata-agent/kata-agent-observability-aggregator.test.ts` around
lines 5 - 11, Replace the relative import of KataAgentObservabilityAggregator,
countObservationsByType, isAttributedToAgent, isNewerRun, and
listRunDirectoryIds from './kata-agent-observability-aggregator.js' with the
project path alias for that feature (e.g. the alias that maps to the
features/kata-agent module) and omit the explicit .js extension; update the
import statement in kata-agent-observability-aggregator.test.ts to use the path
alias (so the same named exports are imported via the alias), ensuring the alias
is the one configured in your tsconfig/paths or bundler.
| export function countObservationsByType( | ||
| observations: ReadonlyArray<{ type: string }>, | ||
| ): { count: number; byType: Record<string, number> } { | ||
| const byType: Record<string, number> = {}; | ||
| let count = 0; | ||
| for (const obs of observations) { | ||
| count++; | ||
| byType[obs.type] = (byType[obs.type] ?? 0) + 1; | ||
| } | ||
| return { count, byType }; | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Minor simplification opportunity.
The count variable always equals observations.length, so the manual increment could be replaced. However, the explicit loop approach may be intentional for mutation coverage visibility.
♻️ Optional simplification
export function countObservationsByType(
observations: ReadonlyArray<{ type: string }>,
): { count: number; byType: Record<string, number> } {
const byType: Record<string, number> = {};
- let count = 0;
for (const obs of observations) {
- count++;
byType[obs.type] = (byType[obs.type] ?? 0) + 1;
}
- return { count, byType };
+ return { count: observations.length, byType };
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/features/kata-agent/kata-agent-observability-aggregator.ts` around lines
52 - 62, The function countObservationsByType manually increments count inside
the loop even though it always equals observations.length; change the
implementation to compute count = observations.length (keep the existing
for-loop to build byType) or remove the manual increment and set count after the
loop, updating the function return accordingly to avoid unnecessary mutation of
the count variable in countObservationsByType while preserving byType
aggregation.
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/infrastructure/execution/session-bridge.unit.test.ts (1)
191-198: 🧹 Nitpick | 🔵 TrivialConsider removing duplicate test coverage.
This test case duplicates assertions that are already comprehensively covered in
session-bridge.helpers.test.ts(lines 8-37). Since the helper is now a standalone export tested in its own file, this test adds no additional coverage value.If the intent is to verify integration with
SessionExecutionBridge, the test at lines 175-189 already covers that path by exercisingupdateCycleStatewhich delegates to the helper.🧹 Proposed removal
- it('only allows adjacent forward cycle state transitions', () => { - expect(canTransitionCycleState('planning', 'active')).toBe(true); - expect(canTransitionCycleState('active', 'cooldown')).toBe(true); - expect(canTransitionCycleState('cooldown', 'complete')).toBe(true); - expect(canTransitionCycleState('planning', 'complete')).toBe(false); - expect(canTransitionCycleState('active', 'complete')).toBe(false); - expect(canTransitionCycleState('complete', 'planning')).toBe(false); - });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/infrastructure/execution/session-bridge.unit.test.ts` around lines 191 - 198, This test duplicates coverage for the canTransitionCycleState helper already tested in session-bridge.helpers.test.ts; remove the redundant "only allows adjacent forward cycle state transitions" test block from session-bridge.unit.test.ts so coverage stays focused on SessionExecutionBridge integration tests (keep the existing updateCycleState integration test that exercises the helper indirectly); locate the duplicate by the test description and references to canTransitionCycleState and delete that it(...) block.src/infrastructure/execution/session-bridge.ts (1)
850-851: 🛠️ Refactor suggestion | 🟠 MajorInconsistent: should use
isJsonFilehelper.
listBridgeRunsForCyclestill uses inlinef.endsWith('.json')whilefindCycleForBet(line 428) andloadCycle(line 445) use the extractedisJsonFilehelper. For consistency and to fully realize the refactor goal, use the helper here as well.♻️ Proposed fix
return readdirSync(dir) - .filter((f) => f.endsWith('.json')) + .filter(isJsonFile) .map((f) => {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/infrastructure/execution/session-bridge.ts` around lines 850 - 851, The listBridgeRunsForCycle implementation is using an inline check readdirSync(dir).filter((f) => f.endsWith('.json')) instead of the existing isJsonFile helper; update listBridgeRunsForCycle to call isJsonFile(f) for the filter, and ensure isJsonFile is in scope (import or reference the helper where listBridgeRunsForCycle is defined) so the behavior matches findCycleForBet and loadCycle.
🤖 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/infrastructure/execution/session-bridge.helpers.ts`:
- Around line 16-24: The function hasBridgeRunMetadataChanged only compares
betName and cycleName but its name implies all metadata; either rename the
function to hasBridgeRunNamesChanged and update all call sites (and any exported
typings) accordingly, or keep the name and update the JSDoc comment to
explicitly state it only checks betName and cycleName; locate the symbol
hasBridgeRunMetadataChanged in this file and across imports/exports to rename or
document consistently.
---
Outside diff comments:
In `@src/infrastructure/execution/session-bridge.ts`:
- Around line 850-851: The listBridgeRunsForCycle implementation is using an
inline check readdirSync(dir).filter((f) => f.endsWith('.json')) instead of the
existing isJsonFile helper; update listBridgeRunsForCycle to call isJsonFile(f)
for the filter, and ensure isJsonFile is in scope (import or reference the
helper where listBridgeRunsForCycle is defined) so the behavior matches
findCycleForBet and loadCycle.
In `@src/infrastructure/execution/session-bridge.unit.test.ts`:
- Around line 191-198: This test duplicates coverage for the
canTransitionCycleState helper already tested in session-bridge.helpers.test.ts;
remove the redundant "only allows adjacent forward cycle state transitions" test
block from session-bridge.unit.test.ts so coverage stays focused on
SessionExecutionBridge integration tests (keep the existing updateCycleState
integration test that exercises the helper indirectly); locate the duplicate by
the test description and references to canTransitionCycleState and delete that
it(...) block.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: bf7a445f-38aa-436c-a9b4-43805a7b8782
📒 Files selected for processing (4)
src/infrastructure/execution/session-bridge.helpers.test.tssrc/infrastructure/execution/session-bridge.helpers.tssrc/infrastructure/execution/session-bridge.tssrc/infrastructure/execution/session-bridge.unit.test.ts
📜 Review details
🧰 Additional context used
📓 Path-based instructions (2)
src/**/*.ts
📄 CodeRabbit inference engine (CLAUDE.md)
src/**/*.ts: All types must be defined as Zod schemas usingzod/v4import path, not separate interface definitions
Use ESM-only format with.jsextensions on all internal imports
Use path aliases for imports:@domain/,@infra/,@features/,@shared/,@cli/* instead of relative paths
Files:
src/infrastructure/execution/session-bridge.helpers.tssrc/infrastructure/execution/session-bridge.tssrc/infrastructure/execution/session-bridge.helpers.test.tssrc/infrastructure/execution/session-bridge.unit.test.ts
src/**/*.test.ts
📄 CodeRabbit inference engine (CLAUDE.md)
Colocate test files with implementation files using
*.test.tsnaming convention
Files:
src/infrastructure/execution/session-bridge.helpers.test.tssrc/infrastructure/execution/session-bridge.unit.test.ts
🧠 Learnings (3)
📓 Common learnings
Learnt from: CR
Repo: cmbays/kata PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-03-15T01:51:21.342Z
Learning: Applies to src/**/{cli,orchestrat}*.ts : Prefer extracting pure helpers from CLI/orchestration files before adding more direct tests to large side-effect-heavy modules
📚 Learning: 2026-03-15T01:51:21.342Z
Learnt from: CR
Repo: cmbays/kata PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-03-15T01:51:21.342Z
Learning: Applies to src/**/{cli,orchestrat}*.ts : Prefer extracting pure helpers from CLI/orchestration files before adding more direct tests to large side-effect-heavy modules
Applied to files:
src/infrastructure/execution/session-bridge.helpers.tssrc/infrastructure/execution/session-bridge.helpers.test.tssrc/infrastructure/execution/session-bridge.unit.test.ts
📚 Learning: 2026-03-15T01:51:09.797Z
Learnt from: CR
Repo: cmbays/kata PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-15T01:51:09.797Z
Learning: Store persistent data as JSON files in `.kata/` directory using JsonStore with typed read/write against Zod schemas
Applied to files:
src/infrastructure/execution/session-bridge.ts
🔇 Additional comments (8)
src/infrastructure/execution/session-bridge.helpers.test.ts (1)
1-82: LGTM! Comprehensive test coverage for the extracted helpers.The test suite covers valid forward transitions, skip transitions, backward transitions, same-state rejections, metadata change detection, and JSON file filtering edge cases. This aligns well with the PR goal of hardening mutation coverage.
src/infrastructure/execution/session-bridge.unit.test.ts (1)
11-11: LGTM!The import follows ESM conventions with the
.jsextension.src/infrastructure/execution/session-bridge.helpers.ts (2)
1-14: LGTM! Clean implementation of cycle state transitions.The transition map correctly encodes the linear state machine
planning → active → cooldown → complete. UsingPartial<Record<CycleState, CycleState>>is appropriate sincecompletehas no valid forward transition. The function safely returnsfalsefor any unmapped state.
26-31: LGTM!Simple and effective filename filter.
src/infrastructure/execution/session-bridge.ts (4)
21-25: LGTM!The helper imports follow ESM conventions and use the appropriate relative path with
.jsextension.
428-428: LGTM!Good use of the extracted
isJsonFilehelper for JSON file filtering.
539-542: LGTM!The private wrapper provides a clean delegation point while maintaining encapsulation. The comment clarifies the testability rationale.
651-651: LGTM!Correct usage of
hasBridgeRunMetadataChangedfor detecting name changes that require metadata rewrites.
| /** | ||
| * Detect whether bridge-run metadata has changed vs its refreshed values. | ||
| */ | ||
| export function hasBridgeRunMetadataChanged( | ||
| current: { betName?: string; cycleName?: string }, | ||
| refreshed: { betName?: string; cycleName?: string }, | ||
| ): boolean { | ||
| return refreshed.betName !== current.betName || refreshed.cycleName !== current.cycleName; | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Function name may be overly broad for its scope.
The function only compares betName and cycleName, but the name hasBridgeRunMetadataChanged suggests it checks all metadata fields. Per the BridgeRunMeta schema (see session-bridge.ts:38-64), metadata includes runId, status, completedAt, tokenUsage, agentId, etc.
Consider renaming to hasBridgeRunNamesChanged or documenting in the JSDoc that this specifically detects name changes (not all metadata changes).
📝 Suggested documentation improvement
/**
- * Detect whether bridge-run metadata has changed vs its refreshed values.
+ * Detect whether bridge-run name fields (betName, cycleName) have changed.
+ * Used by refreshPreparedRunMeta to trigger metadata rewrites on rename.
*/
export function hasBridgeRunMetadataChanged(🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/infrastructure/execution/session-bridge.helpers.ts` around lines 16 - 24,
The function hasBridgeRunMetadataChanged only compares betName and cycleName but
its name implies all metadata; either rename the function to
hasBridgeRunNamesChanged and update all call sites (and any exported typings)
accordingly, or keep the name and update the JSDoc comment to explicitly state
it only checks betName and cycleName; locate the symbol
hasBridgeRunMetadataChanged in this file and across imports/exports to rename or
document consistently.
Address CodeRabbit review feedback: - Move isJsonFile to shared file-filters module (single source of truth) - Re-export from session-bridge.helpers and cooldown-session.helpers - Add explicit local=false,global=true test for resolveJsonFlag Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
cmbays
dismissed stale reviews from coderabbitai[bot] and coderabbitai[bot]
Addressed feedback: consolidated isJsonFile, added test
1 participant
Extract pure helpers from 4 hotspot files. Observability aggregator reaches 100% mutation score. Overall 70.65 to 71.23. Addresses #375.
Summary by CodeRabbit
New Features
Tests
Refactor