Finish cycle activation naming quick wins#394
Conversation
|
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 (2)
📝 WalkthroughWalkthroughAdds activation-time cycle naming (optional --name, LLM/heuristic suggestion, and optional prompt), normalizes/validates names across CLI, bridge, and domain layers, updates session-bridge to require persisted names for activation, expands tests/features, and refactors/docs to a sensei/delegated-worker orchestration model. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant CLI as CLI Handler<br/>(cycle/execute)
participant Resolver as CycleActivationName<br/>Resolver
participant Suggester as CycleNameSuggester
participant LLM as Claude LLM
participant Prompt as User Prompt
participant Bridge as SessionExecution<br/>Bridge
participant Manager as CycleManager
User->>CLI: cycle start <id> [--name]
CLI->>CLI: Determine prompting (TTY & !JSON)
alt Name explicitly provided
CLI->>Resolver: resolveCycleActivationName(cycle, providedName)
Resolver->>Resolver: Validate & trim
Resolver-->>CLI: ResolvedCycleActivationName
else No name & prompt enabled
CLI->>Resolver: resolveCycleActivationName(cycle, undefined, promptFn)
Resolver->>Suggester: suggest(cycle)
Suggester->>LLM: invoke Claude prompt
LLM-->>Suggester: raw response
Suggester->>Suggester: parse/sanitize or heuristic fallback
Suggester-->>Resolver: CycleNameSuggestion
Resolver->>Prompt: promptForName(suggestion)
Prompt->>User: ask for name
User-->>Prompt: enters name
Prompt-->>Resolver: validated trimmed name
Resolver-->>CLI: ResolvedCycleActivationName (prompted)
else No name & no prompt
CLI->>Resolver: resolveCycleActivationName(cycle)
Resolver->>Suggester: suggest(cycle)
Suggester->>Suggester: heuristic fallback
Suggester-->>Resolver: CycleNameSuggestion
Resolver-->>CLI: ResolvedCycleActivationName
end
CLI->>Bridge: prepareCycle(cycleId, agentId, activationName)
Bridge->>Manager: transitionState(cycleId, 'active', activationName)
Manager->>Manager: Validate non-empty name & persist
Manager-->>Bridge: updated cycle
Bridge-->>CLI: prepared runs
CLI-->>User: "Cycle '<name>' started!"
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 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)
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 |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: eaee51150b
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| const firstContentLine = raw | ||
| .split('\n') | ||
| .map((line) => line.trim()) | ||
| .find(Boolean); |
There was a problem hiding this comment.
parseSuggestedCycleName currently takes the first non-empty line verbatim before cleanup, so if Claude prepends text like Sure, here's a name: or a code-fence marker, that line is accepted as the cycle name and the real suggestion on later lines is ignored. This can activate cycles with unusable names and break name-based UX, even when Claude returns a valid name further down. Parse should iterate lines and select the first line that remains a plausible name after cleanup, rather than the first non-empty raw line.
Useful? React with 👍 / 👎.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 12
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/domain/services/cycle-manager.test.ts (1)
1-1: 🧹 Nitpick | 🔵 TrivialRemove unnecessary vitest imports per coding guidelines.
Same issue as other test files — Vitest globals are enabled and should be used directly. As per coding guidelines: "Vitest globals are enabled, so do not import
describe,it, orexpect— use them directly."🔧 Proposed fix
-import { describe, it, expect, beforeEach, afterEach } from 'vitest';🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/domain/services/cycle-manager.test.ts` at line 1, The test file imports Vitest globals unnecessarily; remove the import statement that brings in describe, it, expect, beforeEach, afterEach and rely on Vitest's global functions directly (i.e., delete the import line in cycle-manager.test.ts so tests use describe/it/expect/beforeEach/afterEach as globals).src/infrastructure/execution/session-bridge.unit.test.ts (1)
5-5: 🧹 Nitpick | 🔵 TrivialRemove unnecessary vitest imports per coding guidelines.
The coding guidelines state that Vitest globals are enabled, so
describe,it,expect, andvishould be used directly without importing them. As per coding guidelines: "Vitest globals are enabled, so do not importdescribe,it, orexpect— use them directly."🔧 Proposed fix
-import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest';🤖 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` at line 5, The import line currently pulls Vitest globals; update it to only import lifecycle helpers by removing describe, it, expect, and vi from the import statement so you only import beforeEach and afterEach from 'vitest' and rely on the enabled Vitest globals (describe, it, expect, vi) used directly in the tests (look for the import line at the top of session-bridge.unit.test.ts to modify).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.claude/rules/cli-and-cycle-commands.md:
- Line 10: Update the rule text for `cycle new` in
.claude/rules/cli-and-cycle-commands.md so it matches the activation policy:
state that providing a name is optional during creation/planning but a non-empty
persisted name is required before activation/launch. Change the sentence that
currently reads "`cycle new` requires a non-empty cycle name" to explicitly
allow unnamed planning and assert the requirement at activation/launch, and
ensure any surrounding guidance references the "activation" requirement rather
than creation.
In @.claude/rules/execution-bridge-and-cycles.md:
- Around line 1-9: The file's frontmatter lacks a top-level heading which
triggers markdownlint MD041; immediately after the closing frontmatter delimiter
(the --- block shown at the top), add a single H1 line (a top-level heading)
that summarizes the document (e.g., an "Execution bridge and cycles" title) so
the file has a proper first-level heading following the frontmatter.
In @.claude/rules/testing-and-quality.md:
- Around line 1-11: Add a top-level H1 heading immediately after the YAML
frontmatter in testing-and-quality.md to satisfy MD041; locate the frontmatter
block (the leading lines between the initial --- and the closing ---) and insert
a Markdown H1 (for example, "# Testing and Quality") on the first line after
that closing --- so the file begins with frontmatter followed by a top-level
heading.
In @.gitignore:
- Line 39: Remove the negation entry that un-ignores the local settings file
(the line '!.claude/settings.local.json') from .gitignore, ensure the file
pattern for .claude/settings.local.json is ignored (add it if missing), and if
that file is already tracked remove it from the repository index so it stops
being committed and then commit the updated .gitignore; this prevents local
secrets from being tracked.
In `@CLAUDE.md`:
- Line 122: The document uses inconsistent spelling for the same
term—"Colocated" (line 78) vs "co-located" (line 122); pick one canonical
spelling (e.g., "co-located" or "colocated") and update all occurrences to that
form so the CLAUDE.md file is consistent; search for "Colocated", "colocated",
and "co-located" and replace them uniformly (ensure casing matches surrounding
headings or sentence case where used).
In `@docs-site/architecture/sensei-orchestration.md`:
- Line 48: Replace the phrase "Spawn teammates or sub-agents to execute the
selected ryu." with "Spawn delegated worker agents to execute the selected ryu."
to use a single term ("delegated worker agents") consistently; search for other
occurrences of "teammates" or "sub-agents" in this document and standardize them
to "delegated worker agents" so the language matches the centralized model
described elsewhere in this file.
In `@src/cli/cycle-name-prompt.ts`:
- Around line 7-14: The promptForCycleActivationName function currently
validates using value.trim() but returns the raw input, causing inconsistency;
update promptForCycleActivationName so the string returned from input(...) is
trimmed before being returned (e.g., await input(...) then return
result.trim()), keeping the existing default (suggestion.name) and validation
logic intact; reference promptForCycleActivationName, the input prompt call, and
related resolveCycleActivationName/normalizeCycleName for context.
In `@src/cli/integration.test.ts`:
- Line 1: The test imports Vitest globals unnecessarily; remove the imported
symbols describe, it, expect, beforeEach, and afterEach and only import vi from
'vitest' so the suite uses the enabled global functions while still importing
the vi mock helper. Update the import line to reference only vi and ensure any
usages of describe/it/expect/beforeEach/afterEach remain as globals in the file
(no other changes to test code or symbol names like vi).
In `@src/features/cycle-management/cycle-activation-name-resolver.ts`:
- Around line 56-59: The function normalizeCycleName in
cycle-activation-name-resolver.ts duplicates logic found in the session bridge
module; extract this logic into a shared utility function (e.g., export a
normalizeCycleName helper from a new or existing utils module) and import/use
that helper from both cycle-activation-name-resolver and the session bridge so
both call the single shared normalizeCycleName implementation; update imports
and remove the local duplicate function in cycle-activation-name-resolver (and
keep only the shared export in the utils module).
In `@src/features/cycle-management/cycle-name-suggester.ts`:
- Around line 142-148: The defaultInvokeClaude function currently calls
execFileSync('claude', ['--print'], ...) without a timeout, risking indefinite
blocking; modify defaultInvokeClaude to pass a timeout option (e.g., timeout:
30000) into execFileSync's options so the child process is killed after a
reasonable wait, and ensure the existing try/catch fallback still handles the
thrown Error (including TimeoutError) gracefully; reference the
defaultInvokeClaude function and the execFileSync call when making the change.
In `@src/infrastructure/execution/session-bridge.feature`:
- Line 38: Update the Gherkin step text that currently reads "And the prepared
cycle includes 1 runs" to use the correct singular form "And the prepared cycle
includes 1 run"; locate the step in the session-bridge.feature scenario where
the step string "And the prepared cycle includes 1 runs" appears and replace it
with "And the prepared cycle includes 1 run".
In `@src/infrastructure/execution/session-bridge.ts`:
- Around line 46-49: The normalizeCycleName function is duplicated; extract it
into a single shared domain service module (e.g., domain cycle-helpers) as an
exported function normalizeCycleName(name: string | undefined): string |
undefined, then replace the local implementations by importing and using that
exported function in the files that currently define it (the cycle-manager,
session-bridge, and cycle-activation-name-resolver modules) to remove
duplication and ensure a single normalization implementation.
---
Outside diff comments:
In `@src/domain/services/cycle-manager.test.ts`:
- Line 1: The test file imports Vitest globals unnecessarily; remove the import
statement that brings in describe, it, expect, beforeEach, afterEach and rely on
Vitest's global functions directly (i.e., delete the import line in
cycle-manager.test.ts so tests use describe/it/expect/beforeEach/afterEach as
globals).
In `@src/infrastructure/execution/session-bridge.unit.test.ts`:
- Line 5: The import line currently pulls Vitest globals; update it to only
import lifecycle helpers by removing describe, it, expect, and vi from the
import statement so you only import beforeEach and afterEach from 'vitest' and
rely on the enabled Vitest globals (describe, it, expect, vi) used directly in
the tests (look for the import line at the top of session-bridge.unit.test.ts to
modify).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 45940e2c-499d-4bd8-a39b-87f72826c154
📒 Files selected for processing (47)
.claude/rules/cli-and-cycle-commands.md.claude/rules/docs-and-agent-context.md.claude/rules/execution-bridge-and-cycles.md.claude/rules/testing-and-quality.md.dependency-cruiser.cjs.gitignoreCLAUDE.mddocs-site/agents/context-flow.mddocs-site/agents/kata-sensei.mddocs-site/agents/orchestration.mddocs-site/architecture/dojo.mddocs-site/architecture/kataka.mddocs-site/architecture/meta-learning.mddocs-site/architecture/sensei-orchestration.mddocs-site/getting-started/project-setup.mddocs-site/guides/file-structure.mddocs-site/guides/skill-overview.mddocs-site/guides/system-guide.mddocs-site/methodology/reference/app-flow-standard.mddocs-site/product-design/design-vision.mddocs-site/product-design/interaction-design.mddocs-site/product-design/product-design.mddocs-site/product-design/user-journeys.mdscripts/run-crap-diff.mjssrc/acceptance/setup.tssrc/cli/commands/cycle.test.tssrc/cli/commands/cycle.tssrc/cli/commands/execute.test.tssrc/cli/commands/execute.tssrc/cli/cycle-name-prompt.tssrc/cli/integration.test.tssrc/domain/services/cycle-manager.featuresrc/domain/services/cycle-manager.steps.tssrc/domain/services/cycle-manager.test.tssrc/domain/services/cycle-manager.tssrc/domain/types/cycle.tssrc/features/cycle-management/cycle-activation-name-resolver.featuresrc/features/cycle-management/cycle-activation-name-resolver.steps.tssrc/features/cycle-management/cycle-activation-name-resolver.test.tssrc/features/cycle-management/cycle-activation-name-resolver.tssrc/features/cycle-management/cycle-name-suggester.test.tssrc/features/cycle-management/cycle-name-suggester.tssrc/infrastructure/execution/session-bridge.featuresrc/infrastructure/execution/session-bridge.steps.tssrc/infrastructure/execution/session-bridge.test.tssrc/infrastructure/execution/session-bridge.tssrc/infrastructure/execution/session-bridge.unit.test.ts
📜 Review details
🧰 Additional context used
📓 Path-based instructions (10)
src/acceptance/**/*.ts
📄 CodeRabbit inference engine (AGENTS.md)
Register step files for
.featurefiles throughsrc/acceptance/setup.ts
Files:
src/acceptance/setup.ts
**/*.ts
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.ts: All types must be defined as Zod schemas (usingzod/v4import path), not as separate interface definitions. This is a non-negotiable schema-first approach.
Use path aliases for internal imports:@domain/*→src/domain/*,@infra/*→src/infrastructure/*,@features/*→src/features/*,@shared/*→src/shared/*,@cli/*→src/cli/*
Use.jsfile extensions on all internal imports in ESM module context
Respect strict dependency direction: domain → infrastructure → features → shared → cli. No reverse imports allowed.
Files:
src/acceptance/setup.tssrc/domain/types/cycle.tssrc/domain/services/cycle-manager.steps.tssrc/infrastructure/execution/session-bridge.tssrc/cli/integration.test.tssrc/cli/cycle-name-prompt.tssrc/infrastructure/execution/session-bridge.test.tssrc/domain/services/cycle-manager.test.tssrc/features/cycle-management/cycle-activation-name-resolver.test.tssrc/features/cycle-management/cycle-name-suggester.test.tssrc/features/cycle-management/cycle-activation-name-resolver.steps.tssrc/domain/services/cycle-manager.tssrc/infrastructure/execution/session-bridge.unit.test.tssrc/cli/commands/cycle.test.tssrc/cli/commands/cycle.tssrc/infrastructure/execution/session-bridge.steps.tssrc/cli/commands/execute.tssrc/cli/commands/execute.test.tssrc/features/cycle-management/cycle-name-suggester.tssrc/features/cycle-management/cycle-activation-name-resolver.ts
src/**/*.ts
📄 CodeRabbit inference engine (CLAUDE.md)
Colocate test files as
*.test.tsnext to the corresponding*.tssource file
Files:
src/acceptance/setup.tssrc/domain/types/cycle.tssrc/domain/services/cycle-manager.steps.tssrc/infrastructure/execution/session-bridge.tssrc/cli/integration.test.tssrc/cli/cycle-name-prompt.tssrc/infrastructure/execution/session-bridge.test.tssrc/domain/services/cycle-manager.test.tssrc/features/cycle-management/cycle-activation-name-resolver.test.tssrc/features/cycle-management/cycle-name-suggester.test.tssrc/features/cycle-management/cycle-activation-name-resolver.steps.tssrc/domain/services/cycle-manager.tssrc/infrastructure/execution/session-bridge.unit.test.tssrc/cli/commands/cycle.test.tssrc/cli/commands/cycle.tssrc/infrastructure/execution/session-bridge.steps.tssrc/cli/commands/execute.tssrc/cli/commands/execute.test.tssrc/features/cycle-management/cycle-name-suggester.tssrc/features/cycle-management/cycle-activation-name-resolver.ts
src/domain/types/**/*.ts
📄 CodeRabbit inference engine (CLAUDE.md)
Define all domain types as Zod schemas in the
domain/types/directory, which serves as the source of truth for all types
Files:
src/domain/types/cycle.ts
**/*.md
📄 CodeRabbit inference engine (AGENTS.md)
Keep public/private boundaries intact; public repo docs should not point at private
opspaths directly
Files:
CLAUDE.mddocs-site/product-design/design-vision.mddocs-site/guides/file-structure.mddocs-site/guides/system-guide.mddocs-site/agents/kata-sensei.mddocs-site/methodology/reference/app-flow-standard.mddocs-site/architecture/dojo.mddocs-site/product-design/user-journeys.mddocs-site/getting-started/project-setup.mddocs-site/product-design/product-design.mddocs-site/architecture/meta-learning.mddocs-site/architecture/kataka.mddocs-site/guides/skill-overview.mddocs-site/architecture/sensei-orchestration.mddocs-site/agents/orchestration.mddocs-site/product-design/interaction-design.mddocs-site/agents/context-flow.md
**/*.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/infrastructure/execution/session-bridge.featuresrc/domain/services/cycle-manager.featuresrc/features/cycle-management/cycle-activation-name-resolver.feature
src/domain/services/**/*.ts
📄 CodeRabbit inference engine (CLAUDE.md)
Use
domain/services/for pipeline composition, manifest building, and cycle management
Files:
src/domain/services/cycle-manager.steps.tssrc/domain/services/cycle-manager.test.tssrc/domain/services/cycle-manager.ts
src/infrastructure/**/*.ts
📄 CodeRabbit inference engine (CLAUDE.md)
Place persistence implementations (JsonStore), registries, adapters, and tracking logic in the
infrastructure/directory
Files:
src/infrastructure/execution/session-bridge.tssrc/infrastructure/execution/session-bridge.test.tssrc/infrastructure/execution/session-bridge.unit.test.tssrc/infrastructure/execution/session-bridge.steps.ts
src/cli/**/*.ts
📄 CodeRabbit inference engine (CLAUDE.md)
Use
cli/directory as a thin wrapper using Commander.js, not for complex application logic
Files:
src/cli/integration.test.tssrc/cli/cycle-name-prompt.tssrc/cli/commands/cycle.test.tssrc/cli/commands/cycle.tssrc/cli/commands/execute.tssrc/cli/commands/execute.test.ts
**/*.test.ts
📄 CodeRabbit inference engine (CLAUDE.md)
Vitest globals are enabled, so do not import
describe,it, orexpect— use them directly
Files:
src/cli/integration.test.tssrc/infrastructure/execution/session-bridge.test.tssrc/domain/services/cycle-manager.test.tssrc/features/cycle-management/cycle-activation-name-resolver.test.tssrc/features/cycle-management/cycle-name-suggester.test.tssrc/infrastructure/execution/session-bridge.unit.test.tssrc/cli/commands/cycle.test.tssrc/cli/commands/execute.test.ts
🧠 Learnings (23)
📚 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.ts.claude/rules/testing-and-quality.mdsrc/features/cycle-management/cycle-activation-name-resolver.steps.ts
📚 Learning: 2026-03-20T03:28:00.814Z
Learnt from: CR
Repo: cmbays/kata PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-20T03:28:00.814Z
Learning: Applies to src/domain/services/**/*.ts : Use `domain/services/` for pipeline composition, manifest building, and cycle management
Applied to files:
src/acceptance/setup.ts.claude/rules/cli-and-cycle-commands.md.claude/rules/execution-bridge-and-cycles.mdsrc/domain/services/cycle-manager.test.tssrc/domain/services/cycle-manager.tssrc/features/cycle-management/cycle-activation-name-resolver.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: Read `MEMORY.md` before any cross-repo or governance-heavy task
Applied to files:
CLAUDE.md.claude/rules/docs-and-agent-context.md
📚 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 **/*.md : Keep public/private boundaries intact; public repo docs should not point at private `ops` paths directly
Applied to files:
CLAUDE.md.claude/rules/docs-and-agent-context.md
📚 Learning: 2026-03-20T03:28:00.814Z
Learnt from: CR
Repo: cmbays/kata PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-20T03:28:00.814Z
Learning: Applies to **/*.ts : Respect strict dependency direction: domain → infrastructure → features → shared → cli. No reverse imports allowed.
Applied to files:
CLAUDE.md.claude/rules/testing-and-quality.md.claude/rules/cli-and-cycle-commands.md.claude/rules/execution-bridge-and-cycles.md.dependency-cruiser.cjs
📚 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: Follow org testing standards referenced from `MEMORY.md`
Applied to files:
CLAUDE.md
📚 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 **/*.feature : For new behavior or behavior clarification, acceptance scenarios must exist and fail before implementation
Applied to files:
src/infrastructure/execution/session-bridge.feature.claude/rules/testing-and-quality.mdsrc/domain/services/cycle-manager.featuresrc/features/cycle-management/cycle-activation-name-resolver.feature
📚 Learning: 2026-03-20T03:28:00.814Z
Learnt from: CR
Repo: cmbays/kata PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-20T03:28:00.814Z
Learning: SessionExecutionBridge is NOT an IExecutionAdapter — it splits lifecycle into prepare/complete phases
Applied to files:
src/infrastructure/execution/session-bridge.featuresrc/infrastructure/execution/session-bridge.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:
.claude/rules/testing-and-quality.md.claude/rules/cli-and-cycle-commands.mdsrc/cli/integration.test.ts.claude/rules/execution-bridge-and-cycles.mdsrc/domain/services/cycle-manager.test.tssrc/features/cycle-management/cycle-name-suggester.test.tssrc/cli/commands/cycle.test.tssrc/cli/commands/execute.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: Drive implementation with unit/integration tests until acceptance scenarios pass as part of the quality loop
Applied to files:
.claude/rules/testing-and-quality.md
📚 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:
.claude/rules/testing-and-quality.md.claude/rules/cli-and-cycle-commands.mdsrc/cli/integration.test.ts.claude/rules/execution-bridge-and-cycles.mdsrc/domain/services/cycle-manager.test.tssrc/features/cycle-management/cycle-name-suggester.test.tssrc/cli/commands/execute.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 focused mutation testing on changed behavior-heavy modules
Applied to files:
.claude/rules/testing-and-quality.md
📚 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:
.claude/rules/testing-and-quality.md
📚 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:
.claude/rules/testing-and-quality.mdscripts/run-crap-diff.mjs
📚 Learning: 2026-03-20T03:28:00.814Z
Learnt from: CR
Repo: cmbays/kata PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-20T03:28:00.814Z
Learning: Applies to **/*.test.ts : Vitest globals are enabled, so do not import `describe`, `it`, or `expect` — use them directly
Applied to files:
.claude/rules/testing-and-quality.md.claude/rules/cli-and-cycle-commands.mdsrc/cli/integration.test.ts
📚 Learning: 2026-03-20T03:28:00.814Z
Learnt from: CR
Repo: cmbays/kata PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-20T03:28:00.814Z
Learning: Applies to src/cli/**/*.ts : Use `cli/` directory as a thin wrapper using Commander.js, not for complex application logic
Applied to files:
scripts/run-crap-diff.mjs.claude/rules/cli-and-cycle-commands.md
📚 Learning: 2026-03-20T03:28:00.814Z
Learnt from: CR
Repo: cmbays/kata PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-20T03:28:00.814Z
Learning: Applies to src/shared/constants/**/*.ts : Define KATA_DIRS path constants in the `shared/constants/` directory
Applied to files:
.claude/rules/cli-and-cycle-commands.mddocs-site/architecture/kataka.mdsrc/infrastructure/execution/session-bridge.steps.tssrc/cli/commands/execute.tssrc/cli/commands/execute.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 integration tests using `npm run test:integration`
Applied to files:
src/cli/integration.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 unit tests using `npm run test:unit`
Applied to files:
src/cli/integration.test.ts
📚 Learning: 2026-03-20T03:28:00.814Z
Learnt from: CR
Repo: cmbays/kata PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-20T03:28:00.814Z
Learning: Applies to src/infrastructure/**/*.ts : Place persistence implementations (JsonStore), registries, adapters, and tracking logic in the `infrastructure/` directory
Applied to files:
.claude/rules/execution-bridge-and-cycles.mdsrc/cli/commands/cycle.ts
📚 Learning: 2026-03-20T03:28:00.814Z
Learnt from: CR
Repo: cmbays/kata PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-20T03:28:00.814Z
Learning: Applies to src/domain/ports/**/*.ts : Define interfaces (IPersistence, IStageRegistry, IRefResolver) in the `domain/ports/` directory
Applied to files:
.claude/rules/execution-bridge-and-cycles.md
📚 Learning: 2026-03-20T03:28:00.814Z
Learnt from: CR
Repo: cmbays/kata PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-20T03:28:00.814Z
Learning: Applies to **/*.ts : Use path aliases for internal imports: `domain/*` → `src/domain/*`, `infra/*` → `src/infrastructure/*`, `features/*` → `src/features/*`, `shared/*` → `src/shared/*`, `cli/*` → `src/cli/*`
Applied to files:
.claude/rules/execution-bridge-and-cycles.md
📚 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.ts
🪛 LanguageTool
CLAUDE.md
[uncategorized] ~122-~122: Do not mix variants of the same word (‘co-locate’ and ‘colocate’) within a single text.
Context: ...to forget: worktrees for tracked edits, co-located .feature files, quickpickle, and RE...
(EN_WORD_COHERENCY)
docs-site/architecture/sensei-orchestration.md
[style] ~93-~93: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...h bet or which stage should run next. - It can dispatch work for different bets as...
(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
[style] ~94-~94: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ... is the right orchestration strategy. - It can also serialize bets when coordinati...
(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
[style] ~125-~125: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...cursive agent-management assumptions. - It allows parallelism where Claude Code su...
(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
[style] ~126-~126: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...lelism where Claude Code supports it. - It keeps orchestration decisions visible i...
(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
[style] ~127-~127: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...of burying them inside a worker agent. It also keeps failure handling simpler: wh...
(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
🪛 markdownlint-cli2 (0.21.0)
.claude/rules/docs-and-agent-context.md
[warning] 12-12: First line in a file should be a top-level heading
(MD041, first-line-heading, first-line-h1)
.claude/rules/testing-and-quality.md
[warning] 8-8: First line in a file should be a top-level heading
(MD041, first-line-heading, first-line-h1)
.claude/rules/execution-bridge-and-cycles.md
[warning] 10-10: First line in a file should be a top-level heading
(MD041, first-line-heading, first-line-h1)
docs-site/agents/context-flow.md
[warning] 9-9: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🔇 Additional comments (84)
scripts/run-crap-diff.mjs (2)
11-27:findUptraversal logic looks correct and robust.This upward search safely terminates at filesystem root and cleanly returns
nullwhen unresolved. Good reliability improvement.
86-95: Runner resolution fallback flow is solid.Using
findUpfor both.bin/crap4tsand source CLI fallback is a good resilience improvement across varied local/worktree layouts..dependency-cruiser.cjs (2)
4-21: LGTM — robust upward traversal for worktree/monorepo compatibility.The helper correctly walks up the directory tree, terminates at the filesystem root when
parentDir === currentDir, and throws a clear error if the config isn't found. This approach aligns with the PR goal of resolving local tool installations reliably in worktrees.
1-2: Minor observation:node:protocol usage is fine.The
node:fsandnode:pathimports use the modern Node.js protocol syntax, which is supported in Node.js v16+ and improves clarity that these are core modules. No issues here.docs-site/guides/file-structure.md (2)
21-21: LGTM — architecturally consistent.The attribution clarifies that synthesis.md is written directly by the sensei/top-level orchestrator rather than delegated to spawned agents. This aligns with the architectural constraint that execution is one layer deep under sensei.
138-138: LGTM — consistent with line 21.The description correctly states that synthesis is written directly by sensei, maintaining consistency with the file-tree comment at line 21.
docs-site/product-design/design-vision.md (2)
5-5: LGTM — link target updated to match guide relocation.The link correctly points to the relocated system guide at
../guides/system-guide.md.
104-104: LGTM — link target updated to match architecture doc naming.The link correctly points to
../architecture/meta-learning.md(the file is in the same architecture/ directory, so one level up then into architecture/).docs-site/guides/system-guide.md (4)
6-14: LGTM — companion document links reorganized consistently.All links updated to reflect the new documentation structure with product-design/, architecture/, and getting-started/ subdirectories.
109-109: LGTM — architecture link updated consistently.Points to
../architecture/sensei-orchestration.mdmatching the new structure.
145-145: LGTM — getting-started link updated consistently.Points to
../getting-started/project-setup.mdmatching the new structure.
151-151: LGTM — architecture reference updated consistently.Points to
../architecture/meta-learning.mdmatching the new structure.docs-site/product-design/user-journeys.md (1)
6-8: LGTM — companion links updated consistently.Links correctly reflect the documentation reorganization: same-directory references use simple filenames, while the system guide uses the new
../guides/path.docs-site/architecture/dojo.md (2)
6-7: LGTM — companion links updated to new structure.System guide moved to
../guides/and meta-learning doc renamed (dropped-architecturesuffix).
297-297: LGTM — closing reference updated consistently.Matches the companion link update at line 6.
docs-site/architecture/meta-learning.md (2)
6-8: LGTM — companion links updated to new documentation structure.All paths correctly reflect the reorganization: guides/, product-design/, and same-directory references.
490-490: LGTM — closing reference updated consistently.Matches the companion link update at line 6.
docs-site/product-design/product-design.md (2)
6-9: LGTM — companion links updated consistently.Same-directory references simplified to filenames; system guide correctly uses
../guides/path.
80-80: LGTM — lexicon reference link updated with correct anchor.The link correctly points to the relocated guide with the section anchor preserved.
docs-site/methodology/reference/app-flow-standard.md (1)
355-357: The referenced files do not exist in the repository. The current state of plain backticked filenames is appropriate. Adding hyperlinks as suggested would create broken references sinceFRONTEND_GUIDELINES.md,PLAYGROUND_AUDIT_PROTOCOL.md, andWORKFLOW_STAGES.mddo not exist in this codebase. If these are intended to be reference documents, they should either be created in the repository or removed from the documentation.> Likely an incorrect or invalid review comment.docs-site/agents/kata-sensei.md (1)
194-194: Good centralization update.This clarifies the delegator model and removes ambiguity about nested worker spawning.
docs-site/getting-started/project-setup.md (1)
6-8: Link path updates look correct.The companion/reference links are consistent and remain within public docs boundaries.
Also applies to: 139-139
docs-site/architecture/kataka.md (1)
7-10: Companion and lexicon references are aligned.These path/anchor changes are coherent with the docs-site structure and read cleanly.
Also applies to: 31-31, 58-58
docs-site/guides/skill-overview.md (1)
108-131: Delegated-worker rewrite is clear and consistent.The responsibility split between sensei and workers is now explicit and operationally actionable.
Also applies to: 143-143
docs-site/product-design/interaction-design.md (1)
6-9: Nice terminology and link alignment pass.The orchestration narrative now consistently reflects the sensei/delegated-worker execution model.
Also applies to: 42-42, 85-85, 109-109, 165-167, 175-186, 416-416
docs-site/agents/orchestration.md (1)
11-21: Strong consistency update across the orchestration contract.The revised hierarchy/rules clearly keep
kata step nextand gate control at sensei level while constraining worker scope.Also applies to: 27-39, 203-204, 240-240, 438-443
docs-site/agents/context-flow.md (1)
11-13: Context-tier split is well defined.This cleanly documents minimal worker context and keeps orchestration authority with sensei.
Also applies to: 15-16, 41-63, 68-69, 94-95, 98-141
src/acceptance/setup.ts (1)
3-3: Good acceptance wiring for new step definitions.Line 3 correctly registers the new cycle activation resolver steps in the shared setup entrypoint.
As per coding guidelines: "Register step files for
.featurefiles throughsrc/acceptance/setup.ts."src/domain/types/cycle.ts (1)
52-53: Clarifying comment matches current domain model behavior.The added note is accurate and keeps
CycleSchemasemantics explicit without changing validation behavior.src/domain/services/cycle-manager.steps.ts (2)
55-59: State-walk helper now correctly applies names only at activation boundary.This keeps setup behavior aligned with the “name required before/at activation” rule.
211-217: Good targeted assertion for the unnamed-cycle activation rejection path.The new Then step cleanly captures the expected domain error condition for this scenario.
src/domain/services/cycle-manager.feature (2)
3-6: LGTM!The scenario correctly validates that a named planning cycle can transition to active. The
named "Launch Cycle"step aligns with the domain requirement that planning→active transitions require a non-empty cycle name.
8-11: LGTM!Good addition of the negative test case. This scenario ensures the domain rejects activation of unnamed planning cycles, which is the core behavior being enforced by this PR.
src/infrastructure/execution/session-bridge.test.ts (2)
989-998: LGTM!The test correctly validates the new rejection behavior when preparing a planning cycle without a name. The setup properly mutates the cycle JSON to remove the name field, and the assertion matches the error message from
prepareCycle().
1116-1116: LGTM!Good addition of whitespace in the name parameter to validate that
normalizeCycleName()trims correctly. The assertion on line 1121 confirms the trimmed result.src/cli/integration.test.ts (1)
136-157: LGTM!The test correctly validates the new behavior where
cycle new --skip-promptswithout a-nflag creates an unnamed draft cycle in planning state. The assertions properly verify:
- CLI output contains "Cycle created"
- Exactly one cycle file exists
- Cycle JSON has
name: undefinedandstate: 'planning'This aligns with the PR objective of keeping planning cycles unnamed until activation.
src/infrastructure/execution/session-bridge.ts (1)
188-201: LGTM!The validation logic correctly:
- Normalizes both the provided name and existing cycle name
- Throws a clear, actionable error when no name is resolved
- Passes the resolved name to
transitionStatefor domain-level enforcementThis aligns with the domain-level requirement in
cycle-manager.tsthat planning→active transitions require a non-empty cycle name.CLAUDE.md (1)
41-42: LGTM!The new guidance for scoped rules maintenance and compaction instructions is clear, actionable, and aligns with the newly added
.claude/rules/files. The separation of concerns between preserving durable conventions and discarding ephemeral details is well-articulated.Also applies to: 112-128
.claude/rules/docs-and-agent-context.md (1)
1-15: LGTM!The scoped rule file follows the correct frontmatter format for Claude's rules and provides clear, actionable guidance. The rules align with the retrieved learnings about keeping public/private boundaries intact. The MD041 warning about first-line heading is a false positive—YAML frontmatter is expected for these scoped rule files.
src/cli/cycle-name-prompt.ts (1)
3-5: LGTM!The guard correctly prevents prompting in JSON mode or non-interactive environments by checking both stdin and stdout TTY status.
src/features/cycle-management/cycle-activation-name-resolver.feature (1)
1-23: LGTM!The feature file provides comprehensive acceptance coverage for the activation name resolution logic:
- Scenario 1 validates explicit name takes precedence
- Scenario 2 validates LLM suggestion fallback when no prompt available
- Scenario 3 validates user-prompted override flow
The scenarios correctly test the
sourcefield tracking, which is valuable for understanding how the name was resolved. The file is properly co-located with its implementation.src/infrastructure/execution/session-bridge.unit.test.ts (2)
438-464: LGTM — Well-structured warning verification.The test properly spies on
logger.warn, verifies the warning is emitted exactly once with the expected message content, and correctly restores the spy in afinallyblock. The assertion aligns with the implementation incompletePendingCycleRuns(context snippet 1) which logs one warning per missing completion result.
600-617: LGTM — Correct null-filtering test setup.The spy correctly returns
nullfor a specific run ID while delegating to the original implementation for others. This validates the null-filtering behavior incollectCycleCompletionTotals(context snippet 2). Thetry/finallyensures proper cleanup.src/domain/services/cycle-manager.test.ts (3)
61-74: LGTM — Comprehensive name normalization tests.These tests correctly verify that the cycle manager trims whitespace from provided names and normalizes blank/whitespace-only names to
undefined. This aligns with thenormalizeCycleNamehelper in the implementation.
371-376: LGTM — Critical activation name enforcement test.This test validates the guard that prevents transitioning from
planningtoactivewithout a usable cycle name, matching therequireCycleNameForActivationimplementation.
646-653: LGTM — Validates activation rejection for unnamed immediate starts.This test ensures that
startCyclewithout a name throws when the cycle would immediately transition to active (all bets have kata assignments), enforcing the domain invariant.src/features/cycle-management/cycle-activation-name-resolver.test.ts (1)
1-72: LGTM — Comprehensive resolver test coverage.This new test file properly covers all control-flow branches of
resolveCycleActivationName:
- Explicit
providedNameoverride with trimming- Reusing existing
cycle.name- Fallback to suggester when unnamed
- Prompt callback editing the suggested name
- Rejection of whitespace-only explicit names
The
makeCyclehelper creates validCyclefixtures with appropriate defaults.src/cli/commands/cycle.test.ts (3)
112-137: LGTM — Tests unnamed draft and whitespace normalization.These tests correctly verify:
--skip-promptswithout--namecreates an unnamed draft cycle- Whitespace-only
--namenormalizes toundefinedat creation timeThis matches the normalization behavior in
cycle.tsline 69.
352-376: LGTM — Auto-suggest integration test with stubbed Claude.The test properly uses
withStubbedClaudeOutputto mock the Claude CLI response, then verifies both the JSON output and persisted cycle name match the suggested value.
908-918: LGTM — Validates whitespace-only name rejection at launch.This test ensures the CLI properly rejects whitespace-only
--namevalues during staged launch, settingprocess.exitCode = 1and emitting the expected error message.src/features/cycle-management/cycle-name-suggester.test.ts (1)
1-179: LGTM — Thorough test coverage for cycle name suggestion utilities.This new test file provides excellent coverage of:
CycleNameSuggesterclass: LLM success, empty response fallback, error fallbackbuildCycleNameSuggestionPrompt: proper prompt structure with bet descriptionsparseSuggestedCycleName: sanitization of various LLM output formatsbuildHeuristicCycleName: single/multiple bets, truncation, issue ref strippingThe
makeCyclehelper creates appropriate test fixtures with overridable fields.src/features/cycle-management/cycle-activation-name-resolver.steps.ts (2)
30-103: LGTM — Well-structured BDD step definitions.The Given/When/Then steps properly exercise
resolveCycleActivationNameacross all control-flow variants:
providedNameoverride → sourceprovided- No prompt available → suggester fallback with source
llm- Prompt override → source
promptedwith preservedsuggestedNameThe world state management correctly stores cycle, suggestion, and resolved result.
3-3: 🧹 Nitpick | 🔵 TrivialRemove unnecessary vitest import per coding guidelines.
The
expectfunction is available globally when Vitest globals are enabled. As per coding guidelines: "Vitest globals are enabled, so do not importdescribe,it, orexpect— use them directly."🔧 Proposed fix
-import { expect } from 'vitest';⛔ Skipped due to learnings
Learnt from: CR Repo: cmbays/kata PR: 0 File: CLAUDE.md:0-0 Timestamp: 2026-03-20T03:28:00.814Z Learning: Applies to **/*.test.ts : Vitest globals are enabled, so do not import `describe`, `it`, or `expect` — use them directlyLearnt 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`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`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 testsLearnt 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 modulessrc/domain/services/cycle-manager.ts (2)
13-26: LGTM — Clean centralized name normalization and activation guard.The
normalizeCycleNamehelper properly trims whitespace and returnsundefinedfor empty strings. TherequireCycleNameForActivationguard correctly enforces thatplanning → activetransitions require a usable name (either provided or from the existing cycle), throwing a descriptiveKataErrorwhen neither is available.
233-262: LGTM — Well-factored private helpers for transition logic.The refactoring into
renameCycleIfNeeded,assertTransitionAllowed,applyTransitionName, andrequiresActivationNameimproves readability and testability. TheapplyTransitionNamemethod correctly routes torequireCycleNameForActivationfor activation scenarios while allowing optional naming for other transitions.src/cli/commands/cycle.ts (3)
397-404: LGTM — Proper activation name resolution for cycle start.The integration correctly:
- Calls
resolveCycleActivationNamewith the draft cycle and optional--name- Conditionally provides
promptForCycleActivationNamebased onshouldPromptForCycleName(Boolean(ctx.globalOpts.json))- Passes the resolved
activationName.nametobridge.prepareCycleThis ensures the cycle is named before activation per the new domain invariant.
749-778: LGTM — Consistent activation naming for staged launch.The staged launch implementation mirrors the cycle start pattern:
- Resolves activation name with optional
--nameoverride- Conditionally enables prompting based on JSON mode
- Passes resolved name to
bridge.prepareCycleThe flow correctly validates agent ID before launching and provides clear error messages.
432-435: LGTM — Enhanced output includes cycle name.Both JSON and text output now include the resolved/persisted cycle name, improving observability and UX consistency.
src/infrastructure/execution/session-bridge.steps.ts (6)
28-28: LGTM!Adding an optional
errorproperty to capture exceptions during step execution is a clean pattern for negative test scenarios.
98-119: LGTM!The
createUnnamedPlanningCyclehelper correctly creates a cycle without anameproperty and setsstate: 'planning', aligning with the domain requirement that planning cycles remain unnamed until activation.
200-210: LGTM!The step definition correctly initializes the test world with an unnamed planning cycle using the new helper.
284-306: LGTM!The new step definitions correctly exercise the
prepareCyclemethod with the newnameparameter. Passingundefinedas the second argument (agentId) is safe per the method signatureprepareCycle(cycleId: string, agentId?: string, name?: string).
420-428: LGTM!The assertion step correctly validates that the persisted cycle name matches the expected value after preparation.
469-472: LGTM!The error assertion correctly validates that the cycle preparation was rejected due to a missing name, using the expected error message substring.
src/cli/commands/execute.test.ts (6)
139-153: LGTM!The
withStubbedClaudeOutputhelper is well-designed with proper PATH restoration in afinallyblock. The platform-aware path separator handling ensures cross-platform compatibility.
329-349: LGTM!Good coverage for the auto-suggestion flow when preparing an unnamed cycle without
--name. The test correctly verifies both the console output and the persisted cycle name.
351-371: LGTM!The test correctly validates that
--nameaccepts and trims whitespace from the provided name, and persists the trimmed value.
373-393: LGTM!Good negative test case ensuring whitespace-only
--namevalues are properly rejected with an informative error message.
395-418: LGTM!This edge case correctly tests that a whitespace-only stored cycle name is treated as unnamed and auto-recovered via the suggester.
420-431: LGTM!Good test for the error path when the cycle reference cannot be resolved.
src/cli/commands/execute.ts (7)
22-24: LGTM!The new imports for cycle activation name resolution and prompting are correctly structured with proper path aliases and
.jsextensions.Also applies to: 100-100, 109-109
133-142: LGTM!The integration with
resolveCycleActivationNamecorrectly passes the cycle, provided name from--name, and conditionally supplies the prompt callback based on whether JSON output is requested. This ensures non-interactive behavior when--jsonis set.
540-550: LGTM!The refactored
runCategoriesfunction is cleaner with clear early returns and delegation to helper functions.
552-585: LGTM!The
ExecutionSetuptype andprepareExecutionRunfunction cleanly encapsulate the setup logic with proper early returns for failure cases.
587-615: LGTM!The
runExecutionModefunction cleanly delegates to single-category or pipeline mode based on the number of categories.
617-665: LGTM!The extracted helper functions (
parseExecutionCategories,resolveExecutionBet,markExecutionProjectState,validateExecutionAgent) improve modularity and testability while maintaining the same error handling behavior.
667-680: LGTM!The
finalizeSavedKatafunction cleanly encapsulates the kata saving logic with proper conditional output based on JSON mode.src/features/cycle-management/cycle-name-suggester.ts (5)
1-35: LGTM!The
CycleNameSuggesterclass is well-designed with dependency injection for testability. The fallback from LLM to heuristic naming on any error is appropriate for this use case.
37-66: LGTM!The prompt construction is clear and well-constrained, providing the LLM with explicit formatting requirements that should yield consistent, parseable output.
68-84: LGTM!The parsing logic robustly handles various LLM output quirks (bullets, numbering, quotes, common prefixes) before normalizing the result.
86-102: LGTM!The heuristic naming strategy is sensible: use up to two bet descriptions, truncated and title-cased, with a "+ More" suffix when additional bets exist.
104-140: LGTM!The text normalization helpers are well-implemented. The
summarizeBetDescriptionfunction properly handles issue references, tokenization, truncation, and casing. ThetoTitleCasefunction correctly preserves short uppercase acronyms.src/features/cycle-management/cycle-activation-name-resolver.ts (2)
1-19: LGTM!The interface definitions are well-structured with clear separation between input, dependencies, and output types. The
sourcediscriminator provides good traceability for debugging and testing.
20-54: LGTM!The resolution logic correctly implements the priority chain (provided → existing → suggested/prompted) with proper error handling for empty names and accurate source tracking.
src/features/cycle-management/cycle-activation-name-resolver.ts
Outdated
Show resolved
Hide resolved
| function normalizeCycleName(name: string | undefined): string | undefined { | ||
| const trimmed = name?.trim(); | ||
| return trimmed ? trimmed : undefined; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Find all definitions of normalizeCycleName across the codebase
rg -n 'function normalizeCycleName' --type=tsRepository: cmbays/kata
Length of output: 439
🏁 Script executed:
rg -A 3 'function normalizeCycleName' --type=ts -B 1Repository: cmbays/kata
Length of output: 1253
Extract normalizeCycleName to a shared domain service module.
This function is defined identically in three locations:
src/domain/services/cycle-manager.ts(line 13)src/infrastructure/execution/session-bridge.ts(line 46)src/features/cycle-management/cycle-activation-name-resolver.ts(line 56)
Extract it to @domain/services/cycle-helpers.ts (or similar) to eliminate duplication and ensure consistent normalization behavior across all layers.
🤖 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 46 - 49, The
normalizeCycleName function is duplicated; extract it into a single shared
domain service module (e.g., domain cycle-helpers) as an exported function
normalizeCycleName(name: string | undefined): string | undefined, then replace
the local implementations by importing and using that exported function in the
files that currently define it (the cycle-manager, session-bridge, and
cycle-activation-name-resolver modules) to remove duplication and ensure a
single normalization implementation.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs-site/architecture/sensei-orchestration.md`:
- Around line 91-95: The consecutive bullets in the "sensei-orchestration"
section repetitively start with "It …", so update the four bullets (starting
with "It can manage the overall flow across multiple bets in the same cycle.",
"It can decide which bet or which stage should run next.", "It can dispatch work
for different bets as separate agent tasks when that is the right orchestration
strategy.", and "It can also serialize bets when coordination cost or dependency
risk is too high.") to remove the repeated "It can" opening—rewrite them to
start with concise verbs or noun phrases (e.g., "Manage the overall flow across
multiple bets in the same cycle.", "Decide which bet or stage should run next.",
"Dispatch work for different bets as separate agent tasks when appropriate.",
"Serialize bets when coordination cost or dependency risk is too high.") to
improve readability and consistency.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: bd4306b3-4c5b-4e88-9338-b446e9f622a4
📒 Files selected for processing (19)
.claude/rules/cli-and-cycle-commands.md.claude/rules/docs-and-agent-context.md.claude/rules/execution-bridge-and-cycles.md.claude/rules/testing-and-quality.md.claude/settings.local.json.gitignoreCLAUDE.mddocs-site/architecture/sensei-orchestration.mdsrc/cli/cycle-name-prompt.tssrc/cli/integration.test.tssrc/domain/services/cycle-manager.tssrc/domain/services/cycle-name.tssrc/features/cycle-management/cycle-activation-name-resolver.test.tssrc/features/cycle-management/cycle-activation-name-resolver.tssrc/features/cycle-management/cycle-name-suggester.test.tssrc/features/cycle-management/cycle-name-suggester.tssrc/infrastructure/execution/session-bridge.featuresrc/infrastructure/execution/session-bridge.steps.tssrc/infrastructure/execution/session-bridge.ts
💤 Files with no reviewable changes (1)
- .claude/settings.local.json
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Lint, Tests, Quality Loop & Build
🧰 Additional context used
📓 Path-based instructions (8)
**/*.ts
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.ts: All types must be defined as Zod schemas (usingzod/v4import path), not as separate interface definitions. This is a non-negotiable schema-first approach.
Use path aliases for internal imports:@domain/*→src/domain/*,@infra/*→src/infrastructure/*,@features/*→src/features/*,@shared/*→src/shared/*,@cli/*→src/cli/*
Use.jsfile extensions on all internal imports in ESM module context
Respect strict dependency direction: domain → infrastructure → features → shared → cli. No reverse imports allowed.
Files:
src/domain/services/cycle-name.tssrc/infrastructure/execution/session-bridge.tssrc/cli/cycle-name-prompt.tssrc/domain/services/cycle-manager.tssrc/infrastructure/execution/session-bridge.steps.tssrc/features/cycle-management/cycle-name-suggester.test.tssrc/features/cycle-management/cycle-activation-name-resolver.test.tssrc/cli/integration.test.tssrc/features/cycle-management/cycle-name-suggester.tssrc/features/cycle-management/cycle-activation-name-resolver.ts
src/**/*.ts
📄 CodeRabbit inference engine (CLAUDE.md)
Colocate test files as
*.test.tsnext to the corresponding*.tssource file
Files:
src/domain/services/cycle-name.tssrc/infrastructure/execution/session-bridge.tssrc/cli/cycle-name-prompt.tssrc/domain/services/cycle-manager.tssrc/infrastructure/execution/session-bridge.steps.tssrc/features/cycle-management/cycle-name-suggester.test.tssrc/features/cycle-management/cycle-activation-name-resolver.test.tssrc/cli/integration.test.tssrc/features/cycle-management/cycle-name-suggester.tssrc/features/cycle-management/cycle-activation-name-resolver.ts
src/domain/services/**/*.ts
📄 CodeRabbit inference engine (CLAUDE.md)
Use
domain/services/for pipeline composition, manifest building, and cycle management
Files:
src/domain/services/cycle-name.tssrc/domain/services/cycle-manager.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/infrastructure/execution/session-bridge.feature
**/*.md
📄 CodeRabbit inference engine (AGENTS.md)
Keep public/private boundaries intact; public repo docs should not point at private
opspaths directly
Files:
CLAUDE.mddocs-site/architecture/sensei-orchestration.md
src/infrastructure/**/*.ts
📄 CodeRabbit inference engine (CLAUDE.md)
Place persistence implementations (JsonStore), registries, adapters, and tracking logic in the
infrastructure/directory
Files:
src/infrastructure/execution/session-bridge.tssrc/infrastructure/execution/session-bridge.steps.ts
src/cli/**/*.ts
📄 CodeRabbit inference engine (CLAUDE.md)
Use
cli/directory as a thin wrapper using Commander.js, not for complex application logic
Files:
src/cli/cycle-name-prompt.tssrc/cli/integration.test.ts
**/*.test.ts
📄 CodeRabbit inference engine (CLAUDE.md)
Vitest globals are enabled, so do not import
describe,it, orexpect— use them directly
Files:
src/features/cycle-management/cycle-name-suggester.test.tssrc/features/cycle-management/cycle-activation-name-resolver.test.tssrc/cli/integration.test.ts
🧠 Learnings (22)
📚 Learning: 2026-03-20T03:28:00.814Z
Learnt from: CR
Repo: cmbays/kata PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-20T03:28:00.814Z
Learning: Applies to src/domain/services/**/*.ts : Use `domain/services/` for pipeline composition, manifest building, and cycle management
Applied to files:
src/domain/services/cycle-name.tssrc/infrastructure/execution/session-bridge.ts.claude/rules/cli-and-cycle-commands.md.claude/rules/execution-bridge-and-cycles.mdsrc/domain/services/cycle-manager.tssrc/features/cycle-management/cycle-activation-name-resolver.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 **/*.feature : For new behavior or behavior clarification, acceptance scenarios must exist and fail before implementation
Applied to files:
src/infrastructure/execution/session-bridge.feature.claude/rules/testing-and-quality.md
📚 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 **/*.md : Keep public/private boundaries intact; public repo docs should not point at private `ops` paths directly
Applied to files:
.claude/rules/docs-and-agent-context.mdCLAUDE.md
📚 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: Read `MEMORY.md` before any cross-repo or governance-heavy task
Applied to files:
.claude/rules/docs-and-agent-context.mdCLAUDE.md
📚 Learning: 2026-03-20T03:28:00.814Z
Learnt from: CR
Repo: cmbays/kata PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-20T03:28:00.814Z
Learning: Applies to **/*.ts : Respect strict dependency direction: domain → infrastructure → features → shared → cli. No reverse imports allowed.
Applied to files:
CLAUDE.md.claude/rules/cli-and-cycle-commands.md.claude/rules/execution-bridge-and-cycles.md
📚 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: Follow org testing standards referenced from `MEMORY.md`
Applied to files:
CLAUDE.md.claude/rules/testing-and-quality.md
📚 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.ts.claude/rules/testing-and-quality.md.claude/rules/cli-and-cycle-commands.md.claude/rules/execution-bridge-and-cycles.mdsrc/features/cycle-management/cycle-name-suggester.test.tssrc/features/cycle-management/cycle-activation-name-resolver.test.tssrc/cli/integration.test.tssrc/features/cycle-management/cycle-activation-name-resolver.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:
.claude/rules/testing-and-quality.md.claude/rules/cli-and-cycle-commands.md.claude/rules/execution-bridge-and-cycles.mdsrc/features/cycle-management/cycle-name-suggester.test.tssrc/cli/integration.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: Drive implementation with unit/integration tests until acceptance scenarios pass as part of the quality loop
Applied to files:
.claude/rules/testing-and-quality.md
📚 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:
.claude/rules/testing-and-quality.md
📚 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:
.claude/rules/testing-and-quality.md
📚 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:
.claude/rules/testing-and-quality.md
📚 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:
.claude/rules/testing-and-quality.md
📚 Learning: 2026-03-20T03:28:00.814Z
Learnt from: CR
Repo: cmbays/kata PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-20T03:28:00.814Z
Learning: Applies to **/*.test.ts : Vitest globals are enabled, so do not import `describe`, `it`, or `expect` — use them directly
Applied to files:
.claude/rules/testing-and-quality.mdsrc/cli/integration.test.ts
📚 Learning: 2026-03-20T03:28:00.814Z
Learnt from: CR
Repo: cmbays/kata PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-20T03:28:00.814Z
Learning: Applies to src/cli/**/*.ts : Use `cli/` directory as a thin wrapper using Commander.js, not for complex application logic
Applied to files:
.claude/rules/cli-and-cycle-commands.md
📚 Learning: 2026-03-20T03:28:00.814Z
Learnt from: CR
Repo: cmbays/kata PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-20T03:28:00.814Z
Learning: Applies to src/shared/constants/**/*.ts : Define KATA_DIRS path constants in the `shared/constants/` directory
Applied to files:
.claude/rules/cli-and-cycle-commands.mdsrc/infrastructure/execution/session-bridge.steps.tssrc/cli/integration.test.ts
📚 Learning: 2026-03-20T03:28:00.814Z
Learnt from: CR
Repo: cmbays/kata PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-20T03:28:00.814Z
Learning: Applies to src/infrastructure/**/*.ts : Place persistence implementations (JsonStore), registries, adapters, and tracking logic in the `infrastructure/` directory
Applied to files:
.claude/rules/execution-bridge-and-cycles.md
📚 Learning: 2026-03-20T03:28:00.814Z
Learnt from: CR
Repo: cmbays/kata PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-20T03:28:00.814Z
Learning: Applies to src/domain/ports/**/*.ts : Define interfaces (IPersistence, IStageRegistry, IRefResolver) in the `domain/ports/` directory
Applied to files:
.claude/rules/execution-bridge-and-cycles.md
📚 Learning: 2026-03-20T03:28:00.814Z
Learnt from: CR
Repo: cmbays/kata PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-20T03:28:00.814Z
Learning: Applies to **/*.ts : Use path aliases for internal imports: `domain/*` → `src/domain/*`, `infra/*` → `src/infrastructure/*`, `features/*` → `src/features/*`, `shared/*` → `src/shared/*`, `cli/*` → `src/cli/*`
Applied to files:
.claude/rules/execution-bridge-and-cycles.md
📚 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 integration tests using `npm run test:integration`
Applied to files:
src/cli/integration.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/cli/integration.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 unit tests using `npm run test:unit`
Applied to files:
src/cli/integration.test.ts
🪛 LanguageTool
docs-site/architecture/sensei-orchestration.md
[style] ~93-~93: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...h bet or which stage should run next. - It can dispatch work for different bets as...
(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
[style] ~94-~94: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ... is the right orchestration strategy. - It can also serialize bets when coordinati...
(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
[style] ~125-~125: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...cursive agent-management assumptions. - It allows parallelism where Claude Code su...
(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
[style] ~126-~126: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...lelism where Claude Code supports it. - It keeps orchestration decisions visible i...
(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
[style] ~127-~127: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...of burying them inside a worker agent. It also keeps failure handling simpler: wh...
(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
🔇 Additional comments (31)
docs-site/architecture/sensei-orchestration.md (1)
5-7: Good boundary-safe links and terminology alignment.Line 6–7 keep links within public docs paths, and Line 48 uses the standardized “delegated worker agents” wording consistently with the centralized model.
As per coding guidelines, "Keep public/private boundaries intact; public repo docs should not point at private
opspaths directly."Also applies to: 48-48
src/cli/integration.test.ts (2)
1-2: LGTM!The imports are now correctly structured: only
viis imported from Vitest (using globals fordescribe,it,expect, etc.), andreaddirSyncis properly added to support the new test case.
136-157: LGTM!This integration test correctly validates the PR objective of allowing unnamed cycles during the planning phase. The test:
- Creates a cycle without the
--nameflag- Verifies the cycle file is persisted
- Confirms
nameisundefinedandstateis'planning'The assertions align with the domain behavior where
normalizeCycleName(undefined)returnsundefinedand the default cycle state is'planning'.The non-null assertion at line 154 (
cycleFiles[0]!) is safe given the preceding length check at line 152..gitignore (1)
32-40: Good.claudeignore hardening.This correctly keeps scoped shared config/rules trackable while ensuring
settings.local.jsonstays ignored.src/domain/services/cycle-name.ts (1)
1-4: Normalization helper is clean and correct.Trim + empty-to-
undefinedbehavior is precise and reusable for activation-name enforcement paths.src/infrastructure/execution/session-bridge.feature (1)
28-39: Acceptance coverage for activation-time naming is solid.These scenarios clearly lock in both the rejection path (unnamed cycle) and successful activation with an explicit name.
src/infrastructure/execution/session-bridge.ts (1)
184-197: Activation guard is implemented correctly.Requiring a resolved non-empty name before
transitionState(..., 'active', ...)removes ID-fallback ambiguity and enforces the intended domain rule..claude/rules/docs-and-agent-context.md (1)
14-17: Docs boundary guidance is clear and aligned.The rules keep public docs decoupled from private ops paths and clarify current-vs-target behavior expectations.
CLAUDE.md (1)
41-42: Root guidance updates are coherent and practical.The scoped-rule lifecycle and compact-instruction sections are consistent with the repo’s current operating model.
Also applies to: 112-128
.claude/rules/cli-and-cycle-commands.md (1)
10-14: CLI rule set matches the implemented activation policy.Especially good to see
cycle newplanning-name optionality and activation-time non-empty-name requirement stated explicitly.src/cli/cycle-name-prompt.ts (1)
3-5: Prompt gating and normalization are correct.TTY/JSON gating is appropriate, and returning
value.trim()keeps prompt output aligned with validation and downstream resolver semantics.Also applies to: 7-16
.claude/rules/testing-and-quality.md (1)
1-19: LGTM!The rule file is well-structured with proper frontmatter scoping and a top-level heading that addresses the previous MD041 concern. The guidance aligns with the project's testing philosophy—particularly the emphasis on extracting pure helpers before adding side-effect-heavy tests and keeping mutation ignore scope narrow.
src/domain/services/cycle-manager.ts (3)
14-22: LGTM!The
requireCycleNameForActivationhelper cleanly encapsulates the activation-time naming guard with correct fallback precedence (provided name → existing cycle name → error). The error message is descriptive and actionable.
170-182: Well-refactored state transition logic.The separation into
renameCycleIfNeeded,assertTransitionAllowed, andapplyTransitionNamefollows the principle of extracting deterministic helpers from orchestration code. The same-state rename check at line 175 correctly avoids unnecessary writes when the name hasn't changed.
245-258: Clear activation-name enforcement.The
applyTransitionNameandrequiresActivationNamehelpers correctly enforce thatplanning → activetransitions require a resolved name while allowing optional renaming for other transitions.src/features/cycle-management/cycle-activation-name-resolver.test.ts (2)
5-18: Clean test fixture helper.The
makeCyclefactory provides sensible defaults with easy overrides, supporting deterministic test setup. UsingrandomUUID()for cycle IDs ensures uniqueness across test runs.
20-93: Comprehensive test coverage for resolution precedence.The test suite thoroughly validates:
- Priority order (provided → existing → suggested)
- Source attribution for each path
- Prompt editing and source preservation when accepting suggestions unchanged
- Validation errors for whitespace-only inputs
This aligns with the learning to put parsing/formatting/selection behavior into direct unit tests rather than command-level tests.
.claude/rules/execution-bridge-and-cycles.md (1)
1-16: LGTM!The rule file establishes clear architectural guardrails for the execution bridge layer. The H1 heading addresses the previous MD041 concern, and the content effectively codifies the dependency direction and helper extraction principles from the project learnings.
src/infrastructure/execution/session-bridge.steps.ts (3)
98-119: LGTM!The
createUnnamedPlanningCyclehelper correctly creates a cycle without anameproperty, allowing Zod's schema parsing to handle the optional field. This supports testing the activation-name enforcement path.
296-306: Good error-capture pattern for negative testing.The try/catch with
world.errorstorage enables clean assertion of error scenarios without failing the step prematurely.
469-472: Clear error assertion for unnamed cycle rejection.The assertion verifies both that an error occurred and that the message contains the expected constraint text, providing good test observability.
src/features/cycle-management/cycle-name-suggester.test.ts (3)
25-63: Thorough suggester control-flow coverage.Tests validate the three key paths: successful LLM parsing, empty LLM output fallback, and LLM exception fallback. This ensures robust behavior regardless of Claude availability.
109-133: Good parser normalization coverage.Tests cover the key parsing edge cases: labels/bullets/quotes, internal whitespace, preamble lines, and code fences. The
undefinedreturn for empty input ensures clean fallback handling.
135-186: Comprehensive heuristic name generation tests.Coverage includes single/multiple bets, the
+ Moresuffix logic, empty-bets fallback, issue-ref stripping, and truncation. This aligns with the learning to put formatting/selection behavior into direct unit tests.src/features/cycle-management/cycle-activation-name-resolver.ts (3)
1-3: Good centralization of normalization logic.Using
normalizeCycleNamefrom@domain/services/cycle-name.jsaddresses the previous duplication concern and ensures consistent name handling across the codebase.
21-39: Clear resolution precedence with clean helper delegation.The main function's structure makes the priority order immediately apparent: provided → existing → suggested (with optional prompt). Extracting each path into named helpers improves readability.
63-77: Smart source attribution for prompted names.The logic at lines 72-74 correctly preserves the suggester's source (
'llm'or'heuristic') when the user accepts the suggestion unchanged, providing accurate attribution for analytics or logging.src/features/cycle-management/cycle-name-suggester.ts (4)
13-35: Well-structured suggester with graceful fallback.The try/catch pattern in
suggest()ensures the function always returns a valid suggestion, falling back to heuristic naming when Claude is unavailable or returns unusable output. The comment at line 30 documents this intentional behavior.
68-77: Robust multi-line parsing strategy.Iterating through lines and checking each with
isLikelySuggestedCycleNamehandles various LLM output formats (preamble, code fences, etc.) gracefully.
97-114: Thorough bet description sanitization.The
summarizeBetDescriptionfunction correctly:
- Strips common issue-linking patterns (
closes#123, `fixes `#123)- Removes quotes that might wrap descriptions
- Limits token count and applies truncation with ellipsis
- Falls back to
'Planned Cycle'if normalization yields nothing
159-166: Timeout addresses previous blocking concern.The 30-second timeout on
execFileSyncprevents indefinite hangs when theclaudeCLI is unresponsive, and the existing try/catch insuggest()handles the resulting error gracefully.
| - It can manage the overall flow across multiple bets in the same cycle. | ||
| - It can decide which bet or which stage should run next. | ||
| - It can dispatch work for different bets as separate agent tasks when that is the right orchestration strategy. | ||
| - It can also serialize bets when coordination cost or dependency risk is too high. | ||
|
|
There was a problem hiding this comment.
Reduce repeated bullet openings to improve readability.
Several consecutive bullets start with “It …”, which makes this section feel repetitive. A small wording pass would read cleaner.
✍️ Suggested wording tweak
- - It can manage the overall flow across multiple bets in the same cycle.
- - It can decide which bet or which stage should run next.
- - It can dispatch work for different bets as separate agent tasks when that is the right orchestration strategy.
- - It can also serialize bets when coordination cost or dependency risk is too high.
+ - Sensei can manage the overall flow across multiple bets in the same cycle.
+ - It decides which bet or stage should run next.
+ - Work for different bets can be dispatched as separate agent tasks when that is the right orchestration strategy.
+ - Bets can also be serialized when coordination cost or dependency risk is too high.Also applies to: 123-127
🧰 Tools
🪛 LanguageTool
[style] ~93-~93: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...h bet or which stage should run next. - It can dispatch work for different bets as...
(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
[style] ~94-~94: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ... is the right orchestration strategy. - It can also serialize bets when coordinati...
(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@docs-site/architecture/sensei-orchestration.md` around lines 91 - 95, The
consecutive bullets in the "sensei-orchestration" section repetitively start
with "It …", so update the four bullets (starting with "It can manage the
overall flow across multiple bets in the same cycle.", "It can decide which bet
or which stage should run next.", "It can dispatch work for different bets as
separate agent tasks when that is the right orchestration strategy.", and "It
can also serialize bets when coordination cost or dependency risk is too high.")
to remove the repeated "It can" opening—rewrite them to start with concise verbs
or noun phrases (e.g., "Manage the overall flow across multiple bets in the same
cycle.", "Decide which bet or stage should run next.", "Dispatch work for
different bets as separate agent tasks when appropriate.", "Serialize bets when
coordination cost or dependency risk is too high.") to improve readability and
consistency.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/domain/services/cycle-manager.test.ts`:
- Around line 598-608: Add a test to guard against activating an empty, unnamed
cycle: when using CycleManager methods (create, startCycle) attempt to start a
newly created cycle without bets and without providing a name and assert it
fails (state is not 'active' and startCycle returns an error or contains
betsWithoutKata indicating rejection). Specifically, add a new it() similar to
the existing "transitions cycle to active..." test but call
manager.create(makeBudget()) then immediately call manager.startCycle(cycle.id,
'') or undefined and assert the cycle remains non-active and that the API
reports the empty-cycle rejection; apply the same additional test pattern
mirrored for the other case noted around the 640-652 region to cover both
branches.
In `@src/infrastructure/execution/session-bridge.unit.test.ts`:
- Around line 438-464: The test is making two separate toHaveBeenCalledWith
assertions on the same spy (warnSpy) for messages about run completion;
consolidate them into a single assertion that matches both substrings using a
regex so the expectation is atomic and clearer. Update the assertions around
warnSpy (used in the try/finally) to one
expect(warnSpy).toHaveBeenCalledWith(expect.stringMatching(/No completion result
provided for run ".*"[\s\S]*defaulting to success/)) or similar regex that
includes both "No completion result provided for run" and "defaulting to
success", leaving the rest of the assertions for bridge.completeCycle,
firstRun/secondRun, and CycleSchema unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 3b51588e-8b0b-4957-b1cd-1b41e0d02eed
📒 Files selected for processing (2)
src/domain/services/cycle-manager.test.tssrc/infrastructure/execution/session-bridge.unit.test.ts
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Lint, Tests, Quality Loop & Build
🧰 Additional context used
📓 Path-based instructions (5)
**/*.ts
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.ts: All types must be defined as Zod schemas (usingzod/v4import path), not as separate interface definitions. This is a non-negotiable schema-first approach.
Use path aliases for internal imports:@domain/*→src/domain/*,@infra/*→src/infrastructure/*,@features/*→src/features/*,@shared/*→src/shared/*,@cli/*→src/cli/*
Use.jsfile extensions on all internal imports in ESM module context
Respect strict dependency direction: domain → infrastructure → features → shared → cli. No reverse imports allowed.
Files:
src/domain/services/cycle-manager.test.tssrc/infrastructure/execution/session-bridge.unit.test.ts
src/**/*.ts
📄 CodeRabbit inference engine (CLAUDE.md)
Colocate test files as
*.test.tsnext to the corresponding*.tssource file
Files:
src/domain/services/cycle-manager.test.tssrc/infrastructure/execution/session-bridge.unit.test.ts
src/domain/services/**/*.ts
📄 CodeRabbit inference engine (CLAUDE.md)
Use
domain/services/for pipeline composition, manifest building, and cycle management
Files:
src/domain/services/cycle-manager.test.ts
**/*.test.ts
📄 CodeRabbit inference engine (CLAUDE.md)
Vitest globals are enabled, so do not import
describe,it, orexpect— use them directly
Files:
src/domain/services/cycle-manager.test.tssrc/infrastructure/execution/session-bridge.unit.test.ts
src/infrastructure/**/*.ts
📄 CodeRabbit inference engine (CLAUDE.md)
Place persistence implementations (JsonStore), registries, adapters, and tracking logic in the
infrastructure/directory
Files:
src/infrastructure/execution/session-bridge.unit.test.ts
🧠 Learnings (6)
📚 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/domain/services/cycle-manager.test.ts
📚 Learning: 2026-03-20T03:28:00.814Z
Learnt from: CR
Repo: cmbays/kata PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-20T03:28:00.814Z
Learning: Applies to src/domain/services/**/*.ts : Use `domain/services/` for pipeline composition, manifest building, and cycle management
Applied to files:
src/domain/services/cycle-manager.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: 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/domain/services/cycle-manager.test.tssrc/infrastructure/execution/session-bridge.unit.test.ts
📚 Learning: 2026-03-20T03:28:00.814Z
Learnt from: CR
Repo: cmbays/kata PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-20T03:28:00.814Z
Learning: Applies to **/*.test.ts : Vitest globals are enabled, so do not import `describe`, `it`, or `expect` — use them directly
Applied to files:
src/domain/services/cycle-manager.test.tssrc/infrastructure/execution/session-bridge.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.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 unit tests using `npm run test:unit`
Applied to files:
src/infrastructure/execution/session-bridge.unit.test.ts
🔇 Additional comments (5)
src/domain/services/cycle-manager.test.ts (2)
60-63: Good normalization coverage for create-time naming.These cases cleanly lock in trim + blank-to-
undefinedbehavior and reduce regressions around draft-cycle naming.Also applies to: 70-73
327-332: Activation gate tests are solid and aligned with the new rule.This set validates all key paths: existing name carry-forward, supplied-name normalization, and hard failure when activation has no valid name.
Also applies to: 365-375
src/infrastructure/execution/session-bridge.unit.test.ts (3)
5-5: LGTM!The import correctly excludes
describe,it,expect, andvisince Vitest globals are enabled. Explicitly importingbeforeEachandafterEachis valid and works alongside the globals configuration.
191-206: LGTM!Good test coverage for cycle name normalization. The input
' Renamed Cycle 'with leading/trailing whitespace properly validates that the domain/infrastructure layer trims and normalizes names before persisting.
600-617: LGTM!The spy implementation correctly delegates to the original function for most runs while returning
nullfor the specific test case. Thetry/finallypattern ensures proper cleanup. The assertioncompletedBets === 1is correct: one run is completed and readable, while the other returnsnulland is filtered out per the implementation incollectCycleCompletionTotals.
cmbays
dismissed stale reviews from coderabbitai[bot] and coderabbitai[bot]
Superseded by later commits; all actionable feedback has been addressed.
1 participant
Summary
Testing
Review Notes
Closes #371
Closes #373
Closes #379
Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Chores