refactor: extract BridgeRunSyncer from CooldownSession (Wave 1A)#396
refactor: extract BridgeRunSyncer from CooldownSession (Wave 1A)#396
Conversation
Extract bridge-run filesystem scanning into a dedicated BridgeRunSyncer class with constructor DI, reducing CooldownSession from 1,359 to 1,225 lines. Full quality loop: 19 BDD scenarios, 27 unit tests, CRAP < 8, ArchUnit pass, 87% mutation score. Error discrimination in catch blocks (ENOENT/SyntaxError/JsonStoreError = silent, others = logger.warn). Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
|
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 (3)
📝 WalkthroughWalkthroughIntroduces BridgeRunSyncer, extracting filesystem-based bridge-run scanning and bet-outcome reconciliation from CooldownSession into a dedicated class with unit tests, acceptance steps, and a Gherkin feature; CooldownSession now delegates bridge-run responsibilities to the syncer. Changes
Sequence DiagramsequenceDiagram
participant CS as CooldownSession
participant BRS as BridgeRunSyncer
participant FS as FileSystem
participant CM as CycleManager
CS->>BRS: syncOutcomes(cycleId)
BRS->>FS: list/read bridgeRunsDir/*.json
FS-->>BRS: bridge-run metadata (per runId)
BRS->>CM: updateBetOutcomes([BetOutcomeRecord])
CM-->>BRS: {unmatchedBetIds}
BRS-->>CS: [BetOutcomeRecord...]
CS->>BRS: checkIncomplete(cycleId)
BRS->>FS: read bridge-run/<runId>.json (if present)
FS-->>BRS: {status}
alt bridge-run missing
BRS->>FS: read runsDir/<runId>/run.json
FS-->>BRS: {status}
end
BRS-->>CS: [IncompleteRunInfo...]
CS->>BRS: loadBridgeRunIdsByBetId(cycleId)
BRS->>FS: scan bridgeRunsDir/*.json
FS-->>BRS: metadata files
BRS-->>CS: Map<betId, runId>
Estimated code review effort🎯 4 (Complex) | ⏱️ ~65 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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 CodeRabbit can enforce grammar and style rules using `languagetool`.Configure the |
… threshold readRun wraps all errors as JsonStoreError, so ENOENT and SyntaxError checks were redundant. Simplified to a single instanceof check, reducing cyclomatic complexity from 9 to 7 and CRAP score from 10.3 to 7.8. 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/features/cycle-management/bridge-run-syncer.steps.ts`:
- Around line 413-420: The Then step "no incomplete runs are reported" currently
treats world.incompleteRuns being undefined as a no-op; update the step so it
fails if checkIncomplete() was never executed by asserting that
world.incompleteRuns is defined and then asserting its length is 0 (keep the
existing expect(world.lastError).toBeUndefined() check). Locate the Then step
definition in bridge-run-syncer.steps.ts (the function for the string 'no
incomplete runs are reported' and the BridgeRunSyncerWorld type) and replace the
conditional guard with a strict expect(world.incompleteRuns).toBeDefined()
followed by expect(world.incompleteRuns).toHaveLength(0).
In `@src/features/cycle-management/bridge-run-syncer.ts`:
- Around line 177-193: The catch block in readIncompleteRunFileStatus is
bloating the method and pushing CRAP over the threshold; extract the condition
that classifies expected errors into a small helper (e.g.,
isExpectedFileReadError(err) or isExpectedRunFileError(err)) that returns true
for ENOENT, SyntaxError, and JsonStoreError, then replace the inline condition
in readIncompleteRunFileStatus with a call to that helper and only call
logger.warn when the helper returns false; reuse the new helper in the other
file-reading path that performs similar error handling to keep logic consistent
and reduce complexity.
- Around line 115-120: The method recordBetOutcomes currently forwards an empty
outcomes array to this.deps.cycleManager.updateBetOutcomes, which causes the
cycle to be touched even when no data changed; fix it by treating an empty
outcomes list as a no-op: at the start of recordBetOutcomes (before calling
updateBetOutcomes) check if outcomes is empty (length === 0) and simply return
without calling this.deps.cycleManager.updateBetOutcomes, performing any
logging, or mutating the cycle; reference the recordBetOutcomes method, the
BetOutcomeRecord parameter, and the cycleManager.updateBetOutcomes call when
making this change.
- Around line 7-14: The bridge-run-syncer currently imports BetOutcomeRecord and
IncompleteRunInfo types from cooldown-session.ts which keeps the new syncer
coupled to that module; create a small shared types file under the feature
(e.g., add a new file exporting BetOutcomeRecord and IncompleteRunInfo in
`@features/cycle-management/types` or similar) and update bridge-run-syncer.ts to
import those types via the path alias (e.g., import type { BetOutcomeRecord,
IncompleteRunInfo } from '@features/cycle-management/types') and remove the
direct import from cooldown-session.js, ensuring all references
(collectBridgeRunIds, isJsonFile, isSyncableBet,
mapBridgeRunStatusToIncompleteStatus, mapBridgeRunStatusToSyncedOutcome) remain
unchanged.
In `@src/features/cycle-management/cooldown-session.ts`:
- Around line 575-576: The call to this.bridgeRunSyncer.checkIncomplete(cycleId)
can return bridge-run-only results but the public return object currently clears
incompleteRuns when this.deps.runsDir is not set; update the code paths around
this.bridgeRunSyncer.checkIncomplete and the places that assign/return
incompleteRuns (including where this.warnOnIncompleteRuns is called) so that if
runsDir is absent you still propagate the bridge-run-only result (e.g., prefer
the value from the bridgeRunSyncer result when this.deps.runsDir is falsy
instead of setting incompleteRuns to undefined). Ensure all similar sites
referenced (the occurrences near the current snippet and the ones around the
other locations noted) follow the same logic so warnings can be emitted while
the incompleteRuns value is preserved in the public return.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 165d4bc5-5687-43c0-8bac-db875814ea19
📒 Files selected for processing (8)
.gitignoresrc/acceptance/setup.tssrc/features/cycle-management/bridge-run-syncer.featuresrc/features/cycle-management/bridge-run-syncer.steps.tssrc/features/cycle-management/bridge-run-syncer.test.tssrc/features/cycle-management/bridge-run-syncer.tssrc/features/cycle-management/cooldown-session.tsstryker.config.mjs
📜 Review details
🧰 Additional context used
📓 Path-based instructions (5)
src/acceptance/**/*.ts
📄 CodeRabbit inference engine (AGENTS.md)
Register step files for
.featurefiles throughsrc/acceptance/setup.ts
Files:
src/acceptance/setup.ts
src/**/*.{ts,js}
📄 CodeRabbit inference engine (CLAUDE.md)
Use ESM-only imports with
.jsextensions on all internal imports. Project must have"type": "module"in package.json.
Files:
src/acceptance/setup.tssrc/features/cycle-management/cooldown-session.tssrc/features/cycle-management/bridge-run-syncer.steps.tssrc/features/cycle-management/bridge-run-syncer.test.tssrc/features/cycle-management/bridge-run-syncer.ts
src/**/*.ts
📄 CodeRabbit inference engine (CLAUDE.md)
src/**/*.ts: Use path aliases for imports:@domain/*→src/domain/*,@infra/*→src/infrastructure/*,@features/*→src/features/*,@shared/*→src/shared/*,@cli/*→src/cli/*. Aliases must be defined in tsconfig.json and vitest.config.ts.
Dependency direction must be strictly enforced: domain → infrastructure → features → shared → cli. No reverse imports allowed.
Coverage thresholds must maintain 80% statements/functions/lines and 75% branches. Coverage excludes test files andsrc/cli/index.ts.
Files:
src/acceptance/setup.tssrc/features/cycle-management/cooldown-session.tssrc/features/cycle-management/bridge-run-syncer.steps.tssrc/features/cycle-management/bridge-run-syncer.test.tssrc/features/cycle-management/bridge-run-syncer.ts
**/*.feature
📄 CodeRabbit inference engine (AGENTS.md)
**/*.feature: Co-locate.featurefiles with the feature they specify
For new behavior or behavior clarification, acceptance scenarios must exist and fail before implementation
Files:
src/features/cycle-management/bridge-run-syncer.feature
src/**/*.test.ts
📄 CodeRabbit inference engine (CLAUDE.md)
Test files must be colocated next to source files as
*.test.tswith Vitest globals enabled (no need to import describe/it/expect).
Files:
src/features/cycle-management/bridge-run-syncer.test.ts
🧠 Learnings (12)
📓 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/acceptance/**/*.ts : Register step files for `.feature` files through `src/acceptance/setup.ts`
Applied to files:
src/acceptance/setup.tsstryker.config.mjssrc/features/cycle-management/bridge-run-syncer.steps.tssrc/features/cycle-management/bridge-run-syncer.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 acceptance tests using `npm run test:acceptance`
Applied to files:
src/acceptance/setup.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 focused mutation testing on changed behavior-heavy modules
Applied to files:
stryker.config.mjs
📚 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: Focused mutation testing must be run for changed business logic or orchestration seams with meaningful survivors explicitly called out
Applied to files:
stryker.config.mjs
📚 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:
stryker.config.mjssrc/features/cycle-management/bridge-run-syncer.test.ts
📚 Learning: 2026-03-21T22:25:46.055Z
Learnt from: CR
Repo: cmbays/kata PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-21T22:25:46.055Z
Learning: Applies to src/**/*.ts : Dependency direction must be strictly enforced: domain → infrastructure → features → shared → cli. No reverse imports allowed.
Applied to files:
stryker.config.mjs
📚 Learning: 2026-03-21T22:25:46.055Z
Learnt from: CR
Repo: cmbays/kata PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-21T22:25:46.055Z
Learning: Applies to src/{cli,domain/types}/index.ts : Two entrypoints must be defined via tsup: `cli/index` (bin) and `domain/types/index` (library exports).
Applied to files:
stryker.config.mjs
📚 Learning: 2026-03-21T22:25:46.055Z
Learnt from: CR
Repo: cmbays/kata PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-21T22:25:46.055Z
Learning: Applies to src/**/*.ts : Use path aliases for imports: `domain/*` → `src/domain/*`, `infra/*` → `src/infrastructure/*`, `features/*` → `src/features/*`, `shared/*` → `src/shared/*`, `cli/*` → `src/cli/*`. Aliases must be defined in tsconfig.json and vitest.config.ts.
Applied to files:
stryker.config.mjs
📚 Learning: 2026-03-21T22:25:46.055Z
Learnt from: CR
Repo: cmbays/kata PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-21T22:25:46.055Z
Learning: Applies to src/**/*.ts : Coverage thresholds must maintain 80% statements/functions/lines and 75% branches. Coverage excludes test files and `src/cli/index.ts`.
Applied to files:
stryker.config.mjs
📚 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:
stryker.config.mjssrc/features/cycle-management/bridge-run-syncer.test.ts
📚 Learning: 2026-03-21T22:25:46.055Z
Learnt from: CR
Repo: cmbays/kata PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-21T22:25:46.055Z
Learning: Applies to src/**/*.test.ts : Test files must be colocated next to source files as `*.test.ts` with Vitest globals enabled (no need to import describe/it/expect).
Applied to files:
src/features/cycle-management/bridge-run-syncer.test.ts
🪛 GitHub Actions: CI
src/features/cycle-management/bridge-run-syncer.ts
[error] 1-1: crap4ts FAIL: Function BridgeRunSyncer.readIncompleteRunFileStatus has CRAP 10.3 (above threshold 8).
🔇 Additional comments (2)
stryker.config.mjs (1)
13-18: LGTM: the extracted syncer is included in the focused mutation target list.This keeps the new behavior-heavy seam inside the Stryker gate instead of letting the refactor dilute mutation coverage.
Based on learnings, "Focused mutation testing must be run for changed business logic or orchestration seams with meaningful survivors explicitly called out".
src/acceptance/setup.ts (1)
3-3: LGTM: the new step file is registered in the shared acceptance bootstrap.That keeps the colocated feature discoverable through the standard
src/acceptance/setup.tsentrypoint.As per coding guidelines, "Register step files for
.featurefiles throughsrc/acceptance/setup.ts".
| import type { BetOutcomeRecord, IncompleteRunInfo } from './cooldown-session.js'; | ||
| import { | ||
| collectBridgeRunIds, | ||
| isJsonFile, | ||
| isSyncableBet, | ||
| mapBridgeRunStatusToIncompleteStatus, | ||
| mapBridgeRunStatusToSyncedOutcome, | ||
| } from './cooldown-session.helpers.js'; |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Decouple the extracted syncer from cooldown-session.ts instead of importing its types back in.
Pulling BetOutcomeRecord and IncompleteRunInfo from cooldown-session.ts keeps this seam tied to the monolith it is supposed to slim down, and these new relative internal imports also miss the repo alias rule. A small shared types module under @features/cycle-management/* would clean this up.
As per coding guidelines, "Use path aliases for imports: @domain/* → src/domain/*, @infra/* → src/infrastructure/*, @features/* → src/features/*, @shared/* → src/shared/*, @cli/* → src/cli/*."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/features/cycle-management/bridge-run-syncer.ts` around lines 7 - 14, The
bridge-run-syncer currently imports BetOutcomeRecord and IncompleteRunInfo types
from cooldown-session.ts which keeps the new syncer coupled to that module;
create a small shared types file under the feature (e.g., add a new file
exporting BetOutcomeRecord and IncompleteRunInfo in
`@features/cycle-management/types` or similar) and update bridge-run-syncer.ts to
import those types via the path alias (e.g., import type { BetOutcomeRecord,
IncompleteRunInfo } from '@features/cycle-management/types') and remove the
direct import from cooldown-session.js, ensuring all references
(collectBridgeRunIds, isJsonFile, isSyncableBet,
mapBridgeRunStatusToIncompleteStatus, mapBridgeRunStatusToSyncedOutcome) remain
unchanged.
There was a problem hiding this comment.
Tracked as deferred review finding #4 — moving shared types to cooldown-types.ts is planned for Wave 3A when all 5 extractors are done and the final type boundaries are clear.
There was a problem hiding this comment.
@cmbays, that makes good sense — locking in the shared type boundary after all five extractors exist will avoid a premature abstraction that might need reshaping with each subsequent wave. Waiting for Wave 3A keeps the intermediate extractors simple and the final cooldown-types.ts module well-defined.
Would you like me to open a GitHub issue to track this deferred finding (#4 — move BetOutcomeRecord / IncompleteRunInfo to a shared cooldown-types.ts and update imports via @features/cycle-management/* alias) so it doesn't get lost before Wave 3A?
🧠 Learnings used
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
| const incompleteRuns = this.bridgeRunSyncer.checkIncomplete(cycleId); | ||
| this.warnOnIncompleteRuns(incompleteRuns, force); |
There was a problem hiding this comment.
Expose bridge-run-only incomplete results in the public return values.
BridgeRunSyncer.checkIncomplete() now works with bridgeRunsDir alone, but both result objects still gate incompleteRuns on this.deps.runsDir. In that configuration callers can hit the warning path and still get incompleteRuns: undefined, which drops the state you just computed.
💡 Suggested fix
- incompleteRuns: this.deps.runsDir ? incompleteRuns : undefined,
+ incompleteRuns: this.deps.runsDir || this.deps.bridgeRunsDir ? incompleteRuns : undefined,
...
- incompleteRuns: this.deps.runsDir ? incompleteRuns : undefined,
+ incompleteRuns: this.deps.runsDir || this.deps.bridgeRunsDir ? incompleteRuns : undefined,Also applies to: 610-610, 631-632, 655-655
🤖 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 575 - 576,
The call to this.bridgeRunSyncer.checkIncomplete(cycleId) can return
bridge-run-only results but the public return object currently clears
incompleteRuns when this.deps.runsDir is not set; update the code paths around
this.bridgeRunSyncer.checkIncomplete and the places that assign/return
incompleteRuns (including where this.warnOnIncompleteRuns is called) so that if
runsDir is absent you still propagate the bridge-run-only result (e.g., prefer
the value from the bridgeRunSyncer result when this.deps.runsDir is falsy
instead of setting incompleteRuns to undefined). Ensure all similar sites
referenced (the occurrences near the current snippet and the ones around the
other locations noted) follow the same logic so warnings can be emitted while
the incompleteRuns value is preserved in the public return.
There was a problem hiding this comment.
Pre-existing behavior in CooldownSession, not introduced by this extraction. The incompleteRuns gate predates BridgeRunSyncer — tracking separately.
There was a problem hiding this comment.
@cmbays, understood — the runsDir gate on incompleteRuns was already present before this extraction, so it's not a regression introduced here. Thanks for the context.
Would you like me to open a follow-up GitHub issue to track the incompleteRuns exposure gap (i.e., callers can hit the warning path via bridgeRunsDir-only config but still receive incompleteRuns: undefined)? That would keep it from falling through the cracks while staying out of this PR's scope.
🧠 Learnings used
Learnt from: CR
Repo: cmbays/kata PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-21T22:25:46.055Z
Learning: Applies to src/**/*SessionExecutionBridge* : SessionExecutionBridge is NOT an IExecutionAdapter — it must split lifecycle into prepare/complete phases instead.
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
…utcomes guard - Make "no incomplete runs are reported" BDD step strict (require checkIncomplete was actually called) and add the missing When step to the safety scenario - Add early return in recordBetOutcomes for empty outcomes to avoid touching cycle Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
src/features/cycle-management/bridge-run-syncer.ts (2)
115-120:⚠️ Potential issue | 🟠 MajorTreat an empty outcome batch as a no-op.
CycleManager.updateBetOutcomes()insrc/domain/services/cycle-manager.ts, Lines 112-122, still falls through totouch(cycle)whenoutcomes.length === 0, so this public wrapper mutatesupdatedAteven though nothing changed. Guard[]before calling it.💡 Suggested fix
recordBetOutcomes(cycleId: string, outcomes: BetOutcomeRecord[]): void { + if (outcomes.length === 0) return; const { unmatchedBetIds } = this.deps.cycleManager.updateBetOutcomes(cycleId, outcomes); if (unmatchedBetIds.length > 0) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/features/cycle-management/bridge-run-syncer.ts` around lines 115 - 120, The public method recordBetOutcomes should treat an empty outcomes array as a no-op: before calling this.deps.cycleManager.updateBetOutcomes(cycleId, outcomes) check if outcomes.length === 0 and return immediately so updateBetOutcomes (and its internal touch(cycle)) is not invoked; this prevents updating updatedAt when there are no outcomes and preserves the existing behavior for non-empty batches (including logging unmatchedBetIds).
7-14: 🛠️ Refactor suggestion | 🟠 MajorStop importing feature internals through
cooldown-session.ts.This seam still depends on the monolith for
BetOutcomeRecord/IncompleteRunInfo, and both feature-local imports are relative instead of@features/.... Move the shared types/helpers behind a small@features/cycle-management/...module so the extraction can stand on its own. As per coding guidelines, "Use path aliases for imports:@domain/*→src/domain/*,@infra/*→src/infrastructure/*,@features/*→src/features/*,@shared/*→src/shared/*,@cli/*→src/cli/*."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/features/cycle-management/bridge-run-syncer.ts` around lines 7 - 14, Replace the internal relative imports from cooldown-session.js in bridge-run-syncer.ts with a feature-scoped alias: create a new module under `@features/cycle-management` (e.g., an index or helpers file) that re-exports the shared types and functions (BetOutcomeRecord, IncompleteRunInfo, collectBridgeRunIds, isJsonFile, isSyncableBet, mapBridgeRunStatusToIncompleteStatus, mapBridgeRunStatusToSyncedOutcome), update bridge-run-syncer.ts to import those symbols from the new `@features/cycle-management/`... path, and remove the direct dependency on cooldown-session.js so the feature is self-contained and follows the project's path-alias guidelines.
🤖 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/features/cycle-management/bridge-run-syncer.ts`:
- Around line 167-174: In readIncompleteBridgeRunStatus, don't treat an
unreadable or unparsable bridge-run meta as a terminal null; call
readBridgeRunMeta(bridgeRunPath) once, if it returns undefined propagate
undefined (so resolveIncompleteRunStatus can fall back to read()), otherwise
take its .status, map it with mapBridgeRunStatusToIncompleteStatus and return
incompleteStatus ?? null; this preserves null only for explicit terminal
statuses and undefined for unreadable metadata.
---
Duplicate comments:
In `@src/features/cycle-management/bridge-run-syncer.ts`:
- Around line 115-120: The public method recordBetOutcomes should treat an empty
outcomes array as a no-op: before calling
this.deps.cycleManager.updateBetOutcomes(cycleId, outcomes) check if
outcomes.length === 0 and return immediately so updateBetOutcomes (and its
internal touch(cycle)) is not invoked; this prevents updating updatedAt when
there are no outcomes and preserves the existing behavior for non-empty batches
(including logging unmatchedBetIds).
- Around line 7-14: Replace the internal relative imports from
cooldown-session.js in bridge-run-syncer.ts with a feature-scoped alias: create
a new module under `@features/cycle-management` (e.g., an index or helpers file)
that re-exports the shared types and functions (BetOutcomeRecord,
IncompleteRunInfo, collectBridgeRunIds, isJsonFile, isSyncableBet,
mapBridgeRunStatusToIncompleteStatus, mapBridgeRunStatusToSyncedOutcome), update
bridge-run-syncer.ts to import those symbols from the new
`@features/cycle-management/`... path, and remove the direct dependency on
cooldown-session.js so the feature is self-contained and follows the project's
path-alias guidelines.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 1701b670-3852-44aa-ad69-a72a5e6a72e5
📒 Files selected for processing (1)
src/features/cycle-management/bridge-run-syncer.ts
📜 Review details
🧰 Additional context used
📓 Path-based instructions (2)
src/**/*.{ts,js}
📄 CodeRabbit inference engine (CLAUDE.md)
Use ESM-only imports with
.jsextensions on all internal imports. Project must have"type": "module"in package.json.
Files:
src/features/cycle-management/bridge-run-syncer.ts
src/**/*.ts
📄 CodeRabbit inference engine (CLAUDE.md)
src/**/*.ts: Use path aliases for imports:@domain/*→src/domain/*,@infra/*→src/infrastructure/*,@features/*→src/features/*,@shared/*→src/shared/*,@cli/*→src/cli/*. Aliases must be defined in tsconfig.json and vitest.config.ts.
Dependency direction must be strictly enforced: domain → infrastructure → features → shared → cli. No reverse imports allowed.
Coverage thresholds must maintain 80% statements/functions/lines and 75% branches. Coverage excludes test files andsrc/cli/index.ts.
Files:
src/features/cycle-management/bridge-run-syncer.ts
🧠 Learnings (4)
📓 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/features/cycle-management/bridge-run-syncer.ts
📚 Learning: 2026-03-21T22:25:46.055Z
Learnt from: CR
Repo: cmbays/kata PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-21T22:25:46.055Z
Learning: Applies to src/**/*.ts : Use path aliases for imports: `domain/*` → `src/domain/*`, `infra/*` → `src/infrastructure/*`, `features/*` → `src/features/*`, `shared/*` → `src/shared/*`, `cli/*` → `src/cli/*`. Aliases must be defined in tsconfig.json and vitest.config.ts.
Applied to files:
src/features/cycle-management/bridge-run-syncer.ts
📚 Learning: 2026-03-21T22:25:46.055Z
Learnt from: CR
Repo: cmbays/kata PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-21T22:25:46.055Z
Learning: Applies to src/**/*.ts : Dependency direction must be strictly enforced: domain → infrastructure → features → shared → cli. No reverse imports allowed.
Applied to files:
src/features/cycle-management/bridge-run-syncer.ts
🔇 Additional comments (1)
src/features/cycle-management/bridge-run-syncer.ts (1)
177-193: Nice simplification of the run-file catch path.Collapsing the expected read/parse failures to
JsonStoreErrormatches the providedreadRun()contract and keeps this method smaller without changing the fallback behavior.
| private readIncompleteBridgeRunStatus(runId: string): IncompleteRunInfo['status'] | null | undefined { | ||
| if (!this.deps.bridgeRunsDir) return undefined; | ||
|
|
||
| const bridgeRunPath = join(this.deps.bridgeRunsDir, `${runId}.json`); | ||
| if (!existsSync(bridgeRunPath)) return undefined; | ||
| const status = this.readBridgeRunMeta(bridgeRunPath)?.status; | ||
| const incompleteStatus = mapBridgeRunStatusToIncompleteStatus(status); | ||
| return incompleteStatus ?? null; |
There was a problem hiding this comment.
Unreadable bridge-run metadata currently suppresses the run.json fallback.
existsSync() makes an existing bridge-run file look authoritative before it is parsed. If readBridgeRunMeta() then returns undefined for corrupt JSON or another swallowed read failure, this method still returns null, so resolveIncompleteRunStatus() never consults readRun(). That can hide genuinely pending/running runs. Return undefined on unreadable/invalid bridge metadata and reserve null for explicit terminal statuses.
💡 Suggested fix
private readIncompleteBridgeRunStatus(runId: string): IncompleteRunInfo['status'] | null | undefined {
if (!this.deps.bridgeRunsDir) return undefined;
const bridgeRunPath = join(this.deps.bridgeRunsDir, `${runId}.json`);
if (!existsSync(bridgeRunPath)) return undefined;
- const status = this.readBridgeRunMeta(bridgeRunPath)?.status;
- const incompleteStatus = mapBridgeRunStatusToIncompleteStatus(status);
- return incompleteStatus ?? null;
+ const meta = this.readBridgeRunMeta(bridgeRunPath);
+ if (!meta) return undefined;
+
+ const incompleteStatus = mapBridgeRunStatusToIncompleteStatus(meta.status);
+ if (incompleteStatus !== undefined) return incompleteStatus;
+
+ return meta.status === 'complete' || meta.status === 'failed' ? null : undefined;
}📝 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.
| private readIncompleteBridgeRunStatus(runId: string): IncompleteRunInfo['status'] | null | undefined { | |
| if (!this.deps.bridgeRunsDir) return undefined; | |
| const bridgeRunPath = join(this.deps.bridgeRunsDir, `${runId}.json`); | |
| if (!existsSync(bridgeRunPath)) return undefined; | |
| const status = this.readBridgeRunMeta(bridgeRunPath)?.status; | |
| const incompleteStatus = mapBridgeRunStatusToIncompleteStatus(status); | |
| return incompleteStatus ?? null; | |
| private readIncompleteBridgeRunStatus(runId: string): IncompleteRunInfo['status'] | null | undefined { | |
| if (!this.deps.bridgeRunsDir) return undefined; | |
| const bridgeRunPath = join(this.deps.bridgeRunsDir, `${runId}.json`); | |
| if (!existsSync(bridgeRunPath)) return undefined; | |
| const meta = this.readBridgeRunMeta(bridgeRunPath); | |
| if (!meta) return undefined; | |
| const incompleteStatus = mapBridgeRunStatusToIncompleteStatus(meta.status); | |
| if (incompleteStatus !== undefined) return incompleteStatus; | |
| return meta.status === 'complete' || meta.status === 'failed' ? null : undefined; | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/features/cycle-management/bridge-run-syncer.ts` around lines 167 - 174,
In readIncompleteBridgeRunStatus, don't treat an unreadable or unparsable
bridge-run meta as a terminal null; call readBridgeRunMeta(bridgeRunPath) once,
if it returns undefined propagate undefined (so resolveIncompleteRunStatus can
fall back to read()), otherwise take its .status, map it with
mapBridgeRunStatusToIncompleteStatus and return incompleteStatus ?? null; this
preserves null only for explicit terminal statuses and undefined for unreadable
metadata.
1 participant
Summary
BridgeRunSyncerclass (183 lines), reducing CooldownSession to 1,225 lineslogger.warnBridgeRunSyncerDepsinterface withPickfor collaborators) validated for remaining 4 extractionsCloses #375 partially — Wave 1A only (BridgeRunSyncer). Remaining waves: 1B (CooldownBeltComputer), 2A (CooldownFollowUpRunner), 2B (CooldownDiaryWriter), 2C (CooldownSynthesisManager), 3A (final wiring).
Related: #395 (formalize BridgeRunMeta schema — deferred to follow-up)
Test plan
bridge-run-syncer.test.ts— all passingbridge-run-syncer.feature— all passing🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Refactor
Tests
Chores