Repository navigation
Collapse thread catalog into one coordinator operation - #69
Conversation
Replace the four parallel getter/setter pairs and the threads/catalog forwarding module with a field-parameterized catalog op, and hold the per-thread lock on setters so catalog changes cannot race bind/close. Co-authored-by: Cursor <cursoragent@cursor.com>
Co-authored-by: Cursor <cursoragent@cursor.com>
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Warning Review limit reached
Next review available in: 42 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (4)
📝 WalkthroughWalkthroughCatalog reads and writes now use field-parameterized APIs across agent runtime, thread coordination, and controller handlers. Catalog writes acquire the per-thread lock, and coordinator tests cover binding, errors, field operations, and concurrency ordering. ChangesThread catalog consolidation
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Controller
participant ThreadCoordinator
participant AgentRuntime
Controller->>ThreadCoordinator: catalog(field, operation)
ThreadCoordinator->>AgentRuntime: getCatalogField or setCatalogField
AgentRuntime-->>ThreadCoordinator: catalog values or completion
ThreadCoordinator-->>Controller: Result response
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ 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 |
soorya-u
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
apps/cli/src/core/threads/coordinator.catalog.test.ts (2)
174-180: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAssert the specific unbound error.
Checking only
isErr()allows unrelated repository or runtime failures to satisfy this test. Assert the expected coordinator error tag as done in the project-mismatch test.Proposed assertion
expect(result.isErr()).toBe(true); + if (result.isErr()) { + expect(result.error._tag).toBe("coordinator.not_found"); + }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/cli/src/core/threads/coordinator.catalog.test.ts` around lines 174 - 180, Update the “catalog get surfaces unbound errors” test to assert the specific expected coordinator error tag, matching the assertion pattern used by the project-mismatch test, rather than only checking result.isErr(). Keep the existing catalog invocation and verify the unbound error identity explicitly.
182-238: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMake the lock assertion deterministic.
Line 226 does not prove
bindAgentis blocked: an unlocked bind can require several awaits, while the set gate is released after only one microtask, still producing the expected final order.Gate
bindAgentinside a mocked dependency, wait until it has entered, then start the catalog setter and verifysetModelcannot enter until the bind gate is released.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/cli/src/core/threads/coordinator.catalog.test.ts` around lines 182 - 238, Make the mutual-exclusion test deterministic by mocking a dependency used by bindAgent, adding a bind-entry signal and release gate, and awaiting bindAgent’s entry before starting the catalog set operation. Assert the setter’s setModel mock has not entered while bindAgent is gated, then release bindAgent and verify both operations succeed in bind-before-set order; update the existing setModel gating accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@apps/cli/src/core/threads/coordinator.catalog.test.ts`:
- Around line 174-180: Update the “catalog get surfaces unbound errors” test to
assert the specific expected coordinator error tag, matching the assertion
pattern used by the project-mismatch test, rather than only checking
result.isErr(). Keep the existing catalog invocation and verify the unbound
error identity explicitly.
- Around line 182-238: Make the mutual-exclusion test deterministic by mocking a
dependency used by bindAgent, adding a bind-entry signal and release gate, and
awaiting bindAgent’s entry before starting the catalog set operation. Assert the
setter’s setModel mock has not entered while bindAgent is gated, then release
bindAgent and verify both operations succeed in bind-before-set order; update
the existing setModel gating accordingly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 213538d7-8b67-40c9-96e0-ed1c0995929c
📒 Files selected for processing (15)
apps/cli/__tests__/integration/draft-session-lifecycle.test.tsapps/cli/src/core/agents/catalog-ops.tsapps/cli/src/core/agents/runtime.test.tsapps/cli/src/core/agents/runtime.tsapps/cli/src/core/threads/bind.tsapps/cli/src/core/threads/binding.test.tsapps/cli/src/core/threads/catalog.test.tsapps/cli/src/core/threads/catalog.tsapps/cli/src/core/threads/coordinator.binding.test.tsapps/cli/src/core/threads/coordinator.catalog.test.tsapps/cli/src/core/threads/coordinator.tsapps/cli/src/handlers/controller/catalog/effort.tsapps/cli/src/handlers/controller/catalog/mode.tsapps/cli/src/handlers/controller/catalog/model.tsapps/cli/src/handlers/controller/catalog/persona.ts
💤 Files with no reviewable changes (3)
- apps/cli/src/core/threads/catalog.test.ts
- apps/cli/src/core/threads/binding.test.ts
- apps/cli/src/core/threads/catalog.ts
Keep the field-parameterized catalog API while preserving getDraftCatalog and probeCatalog from the draft-catalog probe work. Co-authored-by: Cursor <cursoragent@cursor.com>
Co-authored-by: Cursor <cursoragent@cursor.com>
This branch was successfully deployed
1 participant
Summary
ThreadCoordinator.catalogoperation and deletes thethreads/catalogforwarding modulepanicfrombetter-resultCloses #65
Test plan
cd apps/cli && bun run check:typescd apps/cli && bun test src __tests__/integration