refactor: re-inline thin helpers and extract saved-kata CRUD#390
refactor: re-inline thin helpers and extract saved-kata CRUD#390
Conversation
|
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 (10)
📝 WalkthroughWalkthroughRemoved several small predicate helpers across execution and session modules, inlined their checks, added a new saved-kata persistence module and re-exported its CRUD functions from Changes
Sequence Diagram(s)sequenceDiagram
participant CLI as "CLI/execute command"
participant Store as "saved-kata-store"
participant FS as "File System"
participant Schema as "SavedKataSchema (validator)"
CLI->>Store: saveSavedKata(kataDir, name, stages, flavorHints)
Store->>FS: ensureDirExists(kataDir/katas)
Store->>Schema: validate({ name, stages, flavorHints })
Schema-->>Store: validated object / throws
Store->>FS: writeFile(`${name}.json`, validated JSON)
FS-->>Store: write success / error
Store-->>CLI: return / throw descriptive error
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 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 |
Re-inline 9 single-expression wrapper helpers that added cognitive overhead without domain value: hasObservations, shouldSyncOutcomes, hasFailedCaptures, hasMethod (cooldown-session), hasNoGapsToBridge, hasBridgedGaps, hasBlockedGaps, hasPipelineLearnings (execute), and remove the dead identity function mapBridgeRunStatus (session-bridge). Move listSavedKatas, loadSavedKata, saveSavedKata, deleteSavedKata to a dedicated saved-kata-store.ts module, separating persistence from CLI wiring. Re-exports from execute.ts preserve backward compatibility. Keep isSyncableBet (dual-condition domain filter) and all genuinely valuable extractions untouched. Mutation score: 90.76% (above 70% threshold). Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Address review feedback: - Inline canTransition() private method — call canTransitionCycleState directly at the single call site - Remove dead sumTokenTotals export (never imported by production code) 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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
src/features/execute/workflow-runner.ts (1)
360-363:⚠️ Potential issue | 🟠 MajorRecency sort is not globally correct across stage categories.
Line 360-363 sorts by full filename, so
stageCategoryprefix dominates ordering. That can place older artifacts ahead of newer ones when categories differ.Proposed fix
+ const extractTimestamp = (file: string): string => + file.match(/(\d{4}-\d{2}-\d{2}T\d{2}-\d{2}-\d{2}-\d{3}Z)\.json$/)?.[1] ?? ''; + const files = readdirSync(artifactsDir) .filter(isJsonFile) - .sort() - .reverse(); + .sort((a, b) => extractTimestamp(b).localeCompare(extractTimestamp(a)));🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/features/execute/workflow-runner.ts` around lines 360 - 363, The current files list is sorted lexicographically (using .sort().reverse()) so stageCategory prefixes skew recency ordering; change the sort to parse a timestamp from each filename (or use file mtime as fallback) and sort by that numeric/Date value in descending order. Locate the code constructing files via readdirSync(...).filter(isJsonFile).sort().reverse() and replace the simple string sort with a comparator that extracts the timestamp portion (via a regex matching your filename pattern) or reads fs.statSync(file).mtime when timestamp is absent, then sort by that timestamp descending so newest artifacts across all stageCategory prefixes appear first.src/features/cycle-management/cooldown-session.ts (1)
1224-1236: 🧹 Nitpick | 🔵 TrivialDefensive
checkExpiryguard is functional but verbose.The runtime method check via
typeof (this.deps.knowledgeStore as unknown as Record<string, unknown>)?.['checkExpiry'] !== 'function'is a valid defensive pattern for handling partial interface implementations (e.g., test mocks). However, the double type assertion is somewhat convoluted.A slightly cleaner alternative that achieves the same runtime check:
if (typeof (this.deps.knowledgeStore as { checkExpiry?: unknown }).checkExpiry !== 'function') return;This is a minor readability suggestion; the current code is functionally correct and the Stryker comment appropriately marks it as a guard.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/features/cycle-management/cooldown-session.ts` around lines 1224 - 1236, Replace the verbose runtime guard in runExpiryCheck with a simpler typed assertion: check typeof (this.deps.knowledgeStore as { checkExpiry?: unknown }).checkExpiry !== 'function' before returning; leave the rest of the method (calling this.deps.knowledgeStore.checkExpiry(), iterating over buildExpiryCheckMessages(result), and the catch that logs via logger.warn) unchanged so behavior and Stryker comments are preserved.src/features/cycle-management/cooldown-session.unit.test.ts (1)
1309-1339: 🧹 Nitpick | 🔵 TrivialTest names reference removed helper
hasObservations.Two tests reference
hasObservationsin their names, but this helper was removed and the logic is now inlined asrunObs.length > 0:
- Line 1309:
'collectSynthesisObservations uses hasObservations to filter empty observation lists'- Line 1679:
'hasObservations filters out runs with empty observations from synthesis input'Consider renaming these tests to describe the actual behavior without referencing the removed helper:
📝 Suggested test name updates
- it('collectSynthesisObservations uses hasObservations to filter empty observation lists', async () => { + it('collectSynthesisObservations excludes runs with empty observation arrays', async () => {- it('hasObservations filters out runs with empty observations from synthesis input', async () => { + it('collectSynthesisObservations excludes empty observation arrays from synthesis input', async () => {Additionally, these two tests cover nearly identical scenarios (runs without observations are filtered from synthesis input). Consider consolidating or differentiating them more clearly.
Also applies to: 1679-1732
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/features/cycle-management/cooldown-session.unit.test.ts` around lines 1309 - 1339, Two unit tests still mention the removed helper hasObservations in their test titles even though the logic is now inlined (runObs.length > 0); update the test names to describe the behavior instead (e.g., rename "collectSynthesisObservations uses hasObservations to filter empty observation lists" and "hasObservations filters out runs with empty observations from synthesis input" to something like "filters out runs with no observations from synthesis input" or similar), and optionally consolidate the two specs into one test that asserts CooldownSession.prepare (and/or collectSynthesisObservations) excludes runs with empty observations by checking the synthesis input observations length; locate the tests by their current names and update the descriptions and/or merge the duplicated assertions so they cover the scenario once.src/infrastructure/execution/session-bridge.helpers.test.ts (1)
1-13: 🛠️ Refactor suggestion | 🟠 MajorUse import alias instead of relative internal path.
This touched import block still uses a relative internal import; switch it to the configured alias.
Suggested fix
-} from './session-bridge.helpers.js'; +} from '@infra/execution/session-bridge.helpers.js';As per coding guidelines, "Use path aliases for imports:
@domain/,@infra/,@features/,@shared/,@cli/* instead of relative paths".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/infrastructure/execution/session-bridge.helpers.test.ts` around lines 1 - 13, The import block at the top of this test uses a relative path to session-bridge.helpers.js; replace that relative internal path with the configured path alias (e.g., `@infra/`... or the project's alias that maps to src/infrastructure) so the import for functions like canTransitionCycleState, computeBudgetPercent, countJsonlContent, extractHistoryTokenTotal, findEarliestTimestamp, hasBridgeRunMetadataChanged, isJsonFile, matchesCycleRef, resolveAgentId, and sumTokenTotals uses the alias form instead of a relative path; update the import statement accordingly to use the project's alias 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/saved-kata-store.ts`:
- Line 4: Import ZodError from 'zod/v4' and replace the fragile runtime check
that uses error.constructor.name === 'ZodError' with a proper instanceof check
(e.g., error instanceof ZodError) in the try/catch block where you validate
using SavedKataSchema (the validation/parse area referencing SavedKataSchema and
FlavorHint); update the top imports to include ZodError and change the
conditional to use instanceof ZodError so Zod validation errors are identified
reliably.
In `@src/infrastructure/execution/session-bridge.unit.test.ts`:
- Around line 1276-1285: The injected duplicate bridge-run JSON is missing the
required betName field so BridgeRunMetaSchema.parse will reject it and dedup
logic won't run; update the writeFileSync call that writes the secondRunId file
(the object passed to JSON.stringify) to include betName (e.g. set betName:
betName or the same string used for the original run) so the record conforms to
BridgeRunMetaSchema and the dedup path is actually exercised; locate the
writeFileSync usage with bridgeRunsDir and secondRunId in the test and add the
betName property to the object.
- Around line 1201-1212: The test named "loadCycle ignores non-json files in
cycles directory" never calls loadCycle directly (it calls
SessionExecutionBridge.prepare which goes through findCycleForBet), so update
the test to invoke SessionExecutionBridge.loadCycle directly (e.g., (bridge as
any).loadCycle(...) or otherwise access the private method) after creating the
non-JSON file; assert loadCycle returns the expected cycle or undefined/does not
throw and that the non-json file was ignored. Reference SessionExecutionBridge,
loadCycle, prepare, and findCycleForBet to locate and change the test.
- Around line 1230-1232: The test is passing a boolean to completeCycle but the
method signature expects a Record<string, AgentCompletionResult>; update the
call to bridge.completeCycle(cycle.id, ...) to pass a properly typed result map
(e.g. an empty object {} or a map like { "<runId>": { status: "...", output: ...
} } that matches the AgentCompletionResult shape) instead of true, ensuring the
constructed entries match the AgentCompletionResult interface used by
completeCycle and keeping the rest of the assertion (completionResult)
unchanged.
---
Outside diff comments:
In `@src/features/cycle-management/cooldown-session.ts`:
- Around line 1224-1236: Replace the verbose runtime guard in runExpiryCheck
with a simpler typed assertion: check typeof (this.deps.knowledgeStore as {
checkExpiry?: unknown }).checkExpiry !== 'function' before returning; leave the
rest of the method (calling this.deps.knowledgeStore.checkExpiry(), iterating
over buildExpiryCheckMessages(result), and the catch that logs via logger.warn)
unchanged so behavior and Stryker comments are preserved.
In `@src/features/cycle-management/cooldown-session.unit.test.ts`:
- Around line 1309-1339: Two unit tests still mention the removed helper
hasObservations in their test titles even though the logic is now inlined
(runObs.length > 0); update the test names to describe the behavior instead
(e.g., rename "collectSynthesisObservations uses hasObservations to filter empty
observation lists" and "hasObservations filters out runs with empty observations
from synthesis input" to something like "filters out runs with no observations
from synthesis input" or similar), and optionally consolidate the two specs into
one test that asserts CooldownSession.prepare (and/or
collectSynthesisObservations) excludes runs with empty observations by checking
the synthesis input observations length; locate the tests by their current names
and update the descriptions and/or merge the duplicated assertions so they cover
the scenario once.
In `@src/features/execute/workflow-runner.ts`:
- Around line 360-363: The current files list is sorted lexicographically (using
.sort().reverse()) so stageCategory prefixes skew recency ordering; change the
sort to parse a timestamp from each filename (or use file mtime as fallback) and
sort by that numeric/Date value in descending order. Locate the code
constructing files via readdirSync(...).filter(isJsonFile).sort().reverse() and
replace the simple string sort with a comparator that extracts the timestamp
portion (via a regex matching your filename pattern) or reads
fs.statSync(file).mtime when timestamp is absent, then sort by that timestamp
descending so newest artifacts across all stageCategory prefixes appear first.
In `@src/infrastructure/execution/session-bridge.helpers.test.ts`:
- Around line 1-13: The import block at the top of this test uses a relative
path to session-bridge.helpers.js; replace that relative internal path with the
configured path alias (e.g., `@infra/`... or the project's alias that maps to
src/infrastructure) so the import for functions like canTransitionCycleState,
computeBudgetPercent, countJsonlContent, extractHistoryTokenTotal,
findEarliestTimestamp, hasBridgeRunMetadataChanged, isJsonFile, matchesCycleRef,
resolveAgentId, and sumTokenTotals uses the alias form instead of a relative
path; update the import statement accordingly to use the project's alias
convention.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 2e4ca438-b1bb-4396-a79b-17065abbfcfd
📒 Files selected for processing (13)
src/cli/commands/execute.helpers.test.tssrc/cli/commands/execute.helpers.tssrc/cli/commands/execute.tssrc/cli/commands/saved-kata-store.tssrc/features/cycle-management/cooldown-session.helpers.test.tssrc/features/cycle-management/cooldown-session.helpers.tssrc/features/cycle-management/cooldown-session.tssrc/features/cycle-management/cooldown-session.unit.test.tssrc/features/execute/workflow-runner.tssrc/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
💤 Files with no reviewable changes (5)
- src/features/cycle-management/cooldown-session.helpers.ts
- src/cli/commands/execute.helpers.test.ts
- src/infrastructure/execution/session-bridge.helpers.ts
- src/cli/commands/execute.helpers.ts
- src/features/cycle-management/cooldown-session.helpers.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.test.tssrc/cli/commands/saved-kata-store.tssrc/infrastructure/execution/session-bridge.unit.test.tssrc/infrastructure/execution/session-bridge.tssrc/features/execute/workflow-runner.tssrc/features/cycle-management/cooldown-session.unit.test.tssrc/cli/commands/execute.tssrc/features/cycle-management/cooldown-session.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.tssrc/features/cycle-management/cooldown-session.unit.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: 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.test.tssrc/infrastructure/execution/session-bridge.unit.test.tssrc/features/execute/workflow-runner.tssrc/features/cycle-management/cooldown-session.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/cli/commands/saved-kata-store.tssrc/features/execute/workflow-runner.tssrc/cli/commands/execute.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/infrastructure/execution/session-bridge.unit.test.tssrc/features/cycle-management/cooldown-session.unit.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: Changed unit and integration tests must pass locally before submitting
Applied to files:
src/infrastructure/execution/session-bridge.unit.test.tssrc/features/cycle-management/cooldown-session.unit.test.ts
🔇 Additional comments (10)
src/features/execute/workflow-runner.ts (1)
84-89: Non-functional hardening changes are clean and safe.Line 84-89 (
learnings: []), Line 142-143 / Line 222-223 (join(',')), and the Stryker/logging guard additions preserve runtime behavior while making intent explicit.Also applies to: 142-143, 222-223, 237-238, 296-300, 326-330, 374-380
src/features/cycle-management/cooldown-session.ts (3)
803-824: LGTM!The inlined
runObs.length > 0guard beforeobservations.push(...runObs)is a reasonable optimization. The Stryker comment correctly notes this is equivalent to a no-op when the array is empty.
924-946: LGTM!The
toSync.length > 0guard is a sensible optimization that avoids callingrecordBetOutcomeswith an empty array. TheisSyncableBethelper is appropriately retained as a non-trivial domain predicate.
392-396: LGTM — Stryker disable comments are well-justified.The Stryker disable annotations consistently mark:
- Catch blocks with non-critical error logging
- Presentation-only text in warning/debug messages
- Equivalent mutants where the guard is functionally a no-op
These annotations improve mutation testing signal-to-noise ratio without masking real defects.
Also applies to: 423-427, 456-457, 518-522, 551-555, 830-834, 848-852, 908-911, 1112-1115, 1197-1200, 1214-1217, 1232-1235, 1272-1276, 1293-1296, 1344-1350
src/features/cycle-management/cooldown-session.unit.test.ts (3)
1995-2036: LGTM — Good defensive guard coverage.This test properly validates that
runExpiryCheckgracefully handles aknowledgeStoreimplementation lacking thecheckExpirymethod, which exercises the defensive guard added at line 1227 ofcooldown-session.ts.
1775-1815: LGTM — Effective mutation testing coverage.This test explicitly verifies the exact arguments passed to hierarchical promotion methods (
'cooldown-retrospective','cooldown'), which kills specific StringLiteral and ObjectLiteral mutants identified by Stryker.
1617-1677: LGTM — Comprehensive guard coverage.The new tests thoroughly exercise the inlined guard behaviors:
collectSynthesisObservationsreturns empty whenrunsDiris not configuredcheckIncompleteRunsreturns empty when neitherrunsDirnorbridgeRunsDiris set- Capture failure logging when
knowledgeStore.capturethrowsdojoSessionBuilder.buildreceives{ title }optionslistJsonFilesfilters non-JSON files from bridge-runs directory- Diary and session write guards based on
dojoDir/dojoSessionBuilderpresenceProper use of cleanup in
finallyblocks ensures no test pollution.Also applies to: 1734-1773, 1817-1881, 1883-1930, 1932-1993
src/infrastructure/execution/session-bridge.ts (1)
92-93: Comment-only mutation annotations look safe.These edits are Stryker pragmas/comments around existing logic and do not change runtime behavior.
Also applies to: 211-212, 226-227, 290-294, 319-320, 426-427, 439-440, 457-458, 482-483, 532-533, 634-635, 793-794, 822-823, 831-832, 857-858, 918-919
src/cli/commands/execute.ts (2)
676-677: Good simplification of emptiness guards.At Line 676, Line 682, Line 689, and Line 757, explicit
.lengthchecks are clear and preserve behavior.Also applies to: 682-690, 757-757
39-39: Nice boundary cleanup with backward-compatible re-export.At Line 39 and Line 818, importing for local use while re-exporting from
saved-kata-storekeeps compatibility and improves separation of concerns.Also applies to: 818-819
| import { join } from 'node:path'; | ||
| import { existsSync, mkdirSync, readdirSync, readFileSync, writeFileSync, unlinkSync } from 'node:fs'; | ||
| import type { StageCategory } from '@domain/types/stage.js'; | ||
| import { SavedKataSchema, type FlavorHint } from '@domain/types/saved-kata.js'; |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
In Zod v4, is ZodErrorexported from thezod/v4entrypoint and isinstanceof ZodError the recommended runtime check?
💡 Result:
Yes.
ZodErroris exported from thezod/v4entrypoint:zod/v4re-exports Zod 4 “classic” (./classic/index.js), which re-exports./errors.js, and that module exportsZodError(the constructor) as a public export. [1] [2] [3]- Recommended runtime check: Zod’s own docs/examples use
err instanceof z.ZodErroras the way to detect validation errors at runtime. [4] Also, in v4 specifically, the core error type ($ZodError) does not extend the built-inError, soinstanceof Erroris not a reliable check for Zod validation errors. [5]
Sources:
[1] https://github.com/colinhacks/zod/blob/main/packages/zod/src/v4/index.ts
[2] https://github.com/colinhacks/zod/blob/main/packages/zod/src/v4/classic/external.ts
[3] https://github.com/colinhacks/zod/blob/main/packages/zod/src/v4/classic/errors.ts
[4] https://github.com/colinhacks/zod (README “Handling errors” example)
[5] https://zod.dev/packages/core (Errors section)
Import ZodError from zod/v4 and use instanceof for error type checking.
At line 23, replace the brittle constructor.name === 'ZodError' check with instanceof ZodError. This is the recommended runtime pattern in Zod v4, where ZodError is properly exported and type-safe, whereas the string comparison is fragile and can misclassify errors.
Proposed fix
import { SavedKataSchema, type FlavorHint } from '@domain/types/saved-kata.js';
+import { ZodError } from 'zod/v4';
@@
- if (e instanceof SyntaxError || (e instanceof Error && e.constructor.name === 'ZodError')) {
+ if (e instanceof SyntaxError || e instanceof ZodError) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/cli/commands/saved-kata-store.ts` at line 4, Import ZodError from
'zod/v4' and replace the fragile runtime check that uses error.constructor.name
=== 'ZodError' with a proper instanceof check (e.g., error instanceof ZodError)
in the try/catch block where you validate using SavedKataSchema (the
validation/parse area referencing SavedKataSchema and FlavorHint); update the
top imports to include ZodError and change the conditional to use instanceof
ZodError so Zod validation errors are identified reliably.
| export function listSavedKatas(kataDir: string): Array<{ name: string; stages: StageCategory[]; description?: string }> { | ||
| const dir = katasDir(kataDir); | ||
| if (!existsSync(dir)) return []; | ||
| return readdirSync(dir) | ||
| .filter(isJsonFile) | ||
| .map((f) => { | ||
| try { | ||
| const raw = JSON.parse(readFileSync(join(dir, f), 'utf-8')); | ||
| return SavedKataSchema.parse(raw); | ||
| } catch (e) { | ||
| if (e instanceof SyntaxError || (e instanceof Error && e.constructor.name === 'ZodError')) { | ||
| console.error(`Warning: skipping invalid kata file "${f}": ${e.message}`); | ||
| return null; | ||
| } | ||
| throw e; | ||
| } | ||
| }) | ||
| .filter((k): k is NonNullable<typeof k> => k !== null); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Prefer JsonStore typed read/write over raw fs+JSON parsing in this persistence module.
This new store re-implements JSON persistence behavior with manual readFileSync/writeFileSync + JSON.parse. Please align saved-kata CRUD with JsonStore to keep typed IO, error handling, and persistence behavior consistent across CLI persistence code.
Based on learnings: "Store persistent data as JSON files in .kata/ directory using JsonStore with typed read/write against Zod schemas".
Also applies to: 33-64
| it('loadCycle ignores non-json files in cycles directory', () => { | ||
| const cycle = createCycle(kataDir, { state: 'active' }); | ||
| const betId = cycle.bets[0]!.id; | ||
|
|
||
| // Add non-json files | ||
| const cyclesDir = join(kataDir, 'cycles'); | ||
| writeFileSync(join(cyclesDir, 'notes.txt'), 'Not a cycle'); | ||
|
|
||
| const bridge = new SessionExecutionBridge(kataDir); | ||
| const prepared = bridge.prepare(betId); | ||
| expect(prepared.runId).toBeDefined(); | ||
| }); |
There was a problem hiding this comment.
This test does not exercise loadCycle despite its name.
The body calls bridge.prepare(betId), which routes through findCycleForBet, so it won’t catch regressions in loadCycle.
Suggested fix
- it('loadCycle ignores non-json files in cycles directory', () => {
+ it('loadCycle ignores non-json files in cycles directory', () => {
const cycle = createCycle(kataDir, { state: 'active' });
- const betId = cycle.bets[0]!.id;
// Add non-json files
const cyclesDir = join(kataDir, 'cycles');
writeFileSync(join(cyclesDir, 'notes.txt'), 'Not a cycle');
const bridge = new SessionExecutionBridge(kataDir);
- const prepared = bridge.prepare(betId);
- expect(prepared.runId).toBeDefined();
+ const loadCycle = (bridge as unknown as {
+ loadCycle: (cycleId: string) => ReturnType<typeof CycleSchema.parse>;
+ }).loadCycle.bind(bridge);
+ expect(loadCycle(cycle.id).id).toBe(cycle.id);
});🤖 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 1201 -
1212, The test named "loadCycle ignores non-json files in cycles directory"
never calls loadCycle directly (it calls SessionExecutionBridge.prepare which
goes through findCycleForBet), so update the test to invoke
SessionExecutionBridge.loadCycle directly (e.g., (bridge as any).loadCycle(...)
or otherwise access the private method) after creating the non-JSON file; assert
loadCycle returns the expected cycle or undefined/does not throw and that the
non-json file was ignored. Reference SessionExecutionBridge, loadCycle, prepare,
and findCycleForBet to locate and change the test.
| // completeCycle reads all bridge runs — the invalid one should be filtered (null → filtered) | ||
| const completionResult = bridge.completeCycle(cycle.id, true); | ||
| expect(completionResult).toBeDefined(); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify method signature and mismatched call-site.
rg -nP --type=ts 'completeCycle\s*\(\s*cycleId:\s*string,\s*results:\s*Record<string,\s*AgentCompletionResult>' src/infrastructure/execution/session-bridge.ts
rg -nP --type=ts 'completeCycle\s*\(\s*cycle\.id\s*,\s*true\s*\)' src/infrastructure/execution/session-bridge.unit.test.tsRepository: cmbays/kata
Length of output: 229
Fix invalid completeCycle argument type (boolean passed where result map is required).
completeCycle expects Record<string, AgentCompletionResult>, but line 1231 passes true. This breaks the method contract and fails TypeScript type checking.
Suggested fix
- const completionResult = bridge.completeCycle(cycle.id, true);
+ const completionResult = bridge.completeCycle(cycle.id, {});📝 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.
| // completeCycle reads all bridge runs — the invalid one should be filtered (null → filtered) | |
| const completionResult = bridge.completeCycle(cycle.id, true); | |
| expect(completionResult).toBeDefined(); | |
| // completeCycle reads all bridge runs — the invalid one should be filtered (null → filtered) | |
| const completionResult = bridge.completeCycle(cycle.id, {}); | |
| expect(completionResult).toBeDefined(); |
🤖 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 1230 -
1232, The test is passing a boolean to completeCycle but the method signature
expects a Record<string, AgentCompletionResult>; update the call to
bridge.completeCycle(cycle.id, ...) to pass a properly typed result map (e.g. an
empty object {} or a map like { "<runId>": { status: "...", output: ... } } that
matches the AgentCompletionResult shape) instead of true, ensuring the
constructed entries match the AgentCompletionResult interface used by
completeCycle and keeping the rest of the assertion (completionResult)
unchanged.
| writeFileSync(join(bridgeRunsDir, `${secondRunId}.json`), JSON.stringify({ | ||
| runId: secondRunId, | ||
| betId, | ||
| cycleId: cycle.id, | ||
| stages: ['build'], | ||
| isolation: 'shared', | ||
| startedAt: new Date().toISOString(), | ||
| cycleName: 'Test Cycle', | ||
| status: 'in-progress', | ||
| }, null, 2)); |
There was a problem hiding this comment.
Dedup fixture is schema-incomplete, so the dedup path may not actually be tested.
The injected duplicate bridge-run omits required betName, so BridgeRunMetaSchema.parse(...) will reject it and filter it out before dedup logic runs.
Suggested fix
writeFileSync(join(bridgeRunsDir, `${secondRunId}.json`), JSON.stringify({
runId: secondRunId,
betId,
+ betName: 'Dedup bet',
cycleId: cycle.id,
stages: ['build'],
isolation: 'shared',
startedAt: new Date().toISOString(),
cycleName: 'Test Cycle',
status: 'in-progress',
}, null, 2));📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| writeFileSync(join(bridgeRunsDir, `${secondRunId}.json`), JSON.stringify({ | |
| runId: secondRunId, | |
| betId, | |
| cycleId: cycle.id, | |
| stages: ['build'], | |
| isolation: 'shared', | |
| startedAt: new Date().toISOString(), | |
| cycleName: 'Test Cycle', | |
| status: 'in-progress', | |
| }, null, 2)); | |
| writeFileSync(join(bridgeRunsDir, `${secondRunId}.json`), JSON.stringify({ | |
| runId: secondRunId, | |
| betId, | |
| betName: 'Dedup bet', | |
| cycleId: cycle.id, | |
| stages: ['build'], | |
| isolation: 'shared', | |
| startedAt: new Date().toISOString(), | |
| cycleName: 'Test Cycle', | |
| status: 'in-progress', | |
| }, null, 2)); |
🤖 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 1276 -
1285, The injected duplicate bridge-run JSON is missing the required betName
field so BridgeRunMetaSchema.parse will reject it and dedup logic won't run;
update the writeFileSync call that writes the secondRunId file (the object
passed to JSON.stringify) to include betName (e.g. set betName: betName or the
same string used for the original run) so the record conforms to
BridgeRunMetaSchema and the dedup path is actually exercised; locate the
writeFileSync usage with bridgeRunsDir and secondRunId in the test and add the
betName property to the object.
cmbays
force-pushed
the
worktree-rosy-twirling-petal
branch
from
4a9a1ab to
c088ae6
Compare
cmbays
dismissed
coderabbitai[bot]’s stale review
Fixed: removed canTransition wrapper and dead sumTokenTotals per review
Inline shouldRecordBetOutcomes, shouldWriteDojoDiary, and shouldWriteDojoSession — same trivial-wrapper pattern as the 8 helpers removed in PR #390. Simplify checkExpiry type guard. Fix test names referencing removed hasObservations helper. Create colocated saved-kata-store.test.ts and remove duplicated tests from execute.test.ts. Fix import alias in session-bridge helpers test. Mutation score holds at 90.92%. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
#391) Inline shouldRecordBetOutcomes, shouldWriteDojoDiary, and shouldWriteDojoSession — same trivial-wrapper pattern as the 8 helpers removed in PR #390. Simplify checkExpiry type guard. Fix test names referencing removed hasObservations helper. Create colocated saved-kata-store.test.ts and remove duplicated tests from execute.test.ts. Fix import alias in session-bridge helpers test. Mutation score holds at 90.92%. Co-authored-by: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
1 participant
Summary
hasObservations(x)→x.length > 0) and added cognitive overhead without domain value. Removed from:cooldown-session.helpers.ts(4),execute.helpers.ts(4),session-bridge.helpers.ts(1 dead identity fn).listSavedKatas,loadSavedKata,saveSavedKata,deleteSavedKata) fromexecute.tsintosaved-kata-store.ts, separating persistence from CLI wiring. Re-exports preserve backward compatibility.isSyncableBet(dual-condition domain filter),isSynthesisPendingFile, all domain-logic helpers, all formatting/mapping helpers.Net: -172 lines, 0 test regressions, mutation score 90.76% (above 70% break threshold).
Test plan
npm run typecheck— passesnpm run lint— passesnpm test(unit + integration) — 3346 + 299 tests passnpm run test:mutation— 90.76% score (above 70% threshold)🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Refactor
Bug Fixes / Reliability
Tests