feat(factory-sdk): add internal fleet client#232
Conversation
|
Warning You have reached your daily quota limit. Please wait up to 24 hours and I will start processing your requests again! |
|
Warning Review limit reached
More reviews will be available in 14 minutes and 3 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more credits in the billing tab to continue. ⌛ How to resolve this issue?After more reviews become available, 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 include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (9)
📝 WalkthroughWalkthroughThis PR introduces task instruction templates for agent coordination and a complete InternalFleetClient implementation. The templates render PR task text with repository, issue, and merge-policy details. The fleet client wraps a harness driver, manages PTY spawning/resuming/releasing, handles async broker message delivery with injection timeouts, and deduplicates broker events. A factory selects between internal and relay backends. Public exports are reorganized to reflect the new modules. ChangesTask Templates and Fleet Client System
Estimated Code Review Effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
Fixed one validated issue in the PR: legacy Addressed comments
Advisory Notes
Local verification run:
|
|
Reviewed PR #232 and made one validated fix in the PR scope: legacy Addressed comments
Advisory Notes
Local verification passed:
I am not printing |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
packages/factory-sdk/src/fleet/create-fleet.test.ts (1)
23-38: ⚡ Quick winConsider adding test coverage for undefined
harnessClientscenario.All current tests explicitly provide
harnessClientin deps. Consider adding a test case that verifies the behavior whencreateFleet()is called with no arguments (or withdeps.harnessClientundefined) to ensure the internal client handles this case gracefully.🧪 Suggested test case
+ it('handles undefined harnessClient gracefully', () => { + const fleet = createFleet() + expect(fleet).toBeInstanceOf(InternalFleetClient) + // Add assertions to verify that basic operations don't throw + })🤖 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 `@packages/factory-sdk/src/fleet/create-fleet.test.ts` around lines 23 - 38, Add a test in create-fleet.test.ts that calls createFleet with no args (or with deps where harnessClient is undefined) to assert it still returns an InternalFleetClient and behaves as expected; specifically, create a new it(...) case that calls createFleet() (or createFleet(undefined, {})) and expects the result toBeInstanceOf(InternalFleetClient) and any relevant method calls (e.g., roster()) to either succeed or reject in the documented/expected way to prove the internal client handles undefined harnessClient gracefully. Make sure to reference createFleet and InternalFleetClient in the test so it verifies the undefined harnessClient scenario.packages/factory-sdk/src/fleet/internal-fleet-client.ts (2)
336-343: 💤 Low valueEvent identity key is redundant when stable ID exists.
When
stable(event_id or delivery_id) is present, the key still includesJSON.stringify(event). This means two events with the same stable ID but different content (e.g., different timestamps or metadata) won't deduplicate. If stable IDs are meant to be authoritative for identity, consider using only the stable ID:♻️ Suggested simplification
function eventIdentity(event: BrokerEvent): EventIdentity { const record = event as BrokerEvent & { event_id?: string; delivery_id?: string } const stable = record.event_id ?? record.delivery_id return { - key: `${event.kind}:${stable ?? ''}:${JSON.stringify(event)}`, + key: stable ? `${event.kind}:${stable}` : `${event.kind}::${JSON.stringify(event)}`, hasStableId: Boolean(stable), } }🤖 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 `@packages/factory-sdk/src/fleet/internal-fleet-client.ts` around lines 336 - 343, The eventIdentity function currently builds a key that appends JSON.stringify(event) even when a stable id (event_id or delivery_id) exists, preventing deduplication by stable id; modify eventIdentity (function name: eventIdentity, types: BrokerEvent and the record with event_id/delivery_id) so that if stable is present the key uses only `${event.kind}:${stable}` (or similar stable-only form) and when stable is absent fall back to the current `${event.kind}:${''}:${JSON.stringify(event)}` approach; keep hasStableId = Boolean(stable) unchanged.
168-182: 💤 Low valueEvent subscription may process the same event twice when both paths are active.
Both
onEventandaddListener('deliveryUpdate')route events to#handleBrokerEvent. If the driver client implements both, delivery-related events (e.g.,delivery_injected,delivery_failed) will be processed twice. The deduplication logic in#resolveInjectedand#rememberEventprevents incorrect behavior, but this creates unnecessary overhead.Consider guarding against double-registration or preferring one subscription path over the other:
♻️ Suggested approach
`#ensureEventSubscription`(): void { if (this.#subscribed) { return } this.#subscribed = true - this.#client.onEvent?.((event) => this.#handleBrokerEvent(event)) - this.#client.addListener?.('deliveryUpdate', (event) => this.#handleBrokerEvent(event)) + // Prefer onEvent if available; fall back to addListener for delivery updates + if (this.#client.onEvent) { + this.#client.onEvent((event) => this.#handleBrokerEvent(event)) + } else { + this.#client.addListener?.('deliveryUpdate', (event) => this.#handleBrokerEvent(event)) + } this.#client.addListener?.('agentExited', (agent) =>🤖 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 `@packages/factory-sdk/src/fleet/internal-fleet-client.ts` around lines 168 - 182, The current `#ensureEventSubscription` registers two handlers that both call `#handleBrokerEvent` when the driver supports both this.#client.onEvent and this.#client.addListener('deliveryUpdate'), causing duplicate processing; change `#ensureEventSubscription` to prefer one subscription path and avoid double-registration — e.g., if this.#client.onEvent is present use only that, otherwise fall back to this.#client.addListener('deliveryUpdate'), or add a guard boolean to ensure only one of the two registrations is performed; update references in `#ensureEventSubscription` (and related registration lines) so only a single route to `#handleBrokerEvent` is created while keeping existing deduplication in `#resolveInjected` and `#rememberEvent`.
🤖 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.
Inline comments:
In `@packages/factory-sdk/src/fleet/internal-fleet-client.ts`:
- Around line 315-319: The error message in function assertSelfNode currently
reads "...only supports node 'self' tonight"; update that string to use
"currently" instead of "tonight" so the thrown Error becomes
`InternalFleetClient only supports node 'self' currently; received ${node}`
(keep the same function assertSelfNode and the SpawnInput['node'] check/throw
logic).
---
Nitpick comments:
In `@packages/factory-sdk/src/fleet/create-fleet.test.ts`:
- Around line 23-38: Add a test in create-fleet.test.ts that calls createFleet
with no args (or with deps where harnessClient is undefined) to assert it still
returns an InternalFleetClient and behaves as expected; specifically, create a
new it(...) case that calls createFleet() (or createFleet(undefined, {})) and
expects the result toBeInstanceOf(InternalFleetClient) and any relevant method
calls (e.g., roster()) to either succeed or reject in the documented/expected
way to prove the internal client handles undefined harnessClient gracefully.
Make sure to reference createFleet and InternalFleetClient in the test so it
verifies the undefined harnessClient scenario.
In `@packages/factory-sdk/src/fleet/internal-fleet-client.ts`:
- Around line 336-343: The eventIdentity function currently builds a key that
appends JSON.stringify(event) even when a stable id (event_id or delivery_id)
exists, preventing deduplication by stable id; modify eventIdentity (function
name: eventIdentity, types: BrokerEvent and the record with
event_id/delivery_id) so that if stable is present the key uses only
`${event.kind}:${stable}` (or similar stable-only form) and when stable is
absent fall back to the current `${event.kind}:${''}:${JSON.stringify(event)}`
approach; keep hasStableId = Boolean(stable) unchanged.
- Around line 168-182: The current `#ensureEventSubscription` registers two
handlers that both call `#handleBrokerEvent` when the driver supports both
this.#client.onEvent and this.#client.addListener('deliveryUpdate'), causing
duplicate processing; change `#ensureEventSubscription` to prefer one subscription
path and avoid double-registration — e.g., if this.#client.onEvent is present
use only that, otherwise fall back to
this.#client.addListener('deliveryUpdate'), or add a guard boolean to ensure
only one of the two registrations is performed; update references in
`#ensureEventSubscription` (and related registration lines) so only a single route
to `#handleBrokerEvent` is created while keeping existing deduplication in
`#resolveInjected` and `#rememberEvent`.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 5191d6fb-9641-4f10-9675-0ddc378f6402
📒 Files selected for processing (9)
packages/factory-sdk/src/dispatch/templates.test.tspackages/factory-sdk/src/dispatch/templates.tspackages/factory-sdk/src/fleet/create-fleet.test.tspackages/factory-sdk/src/fleet/create-fleet.tspackages/factory-sdk/src/fleet/internal-fleet-client.test.tspackages/factory-sdk/src/fleet/internal-fleet-client.tspackages/factory-sdk/src/index.tspackages/factory-sdk/src/ports/fleet.tspackages/factory-sdk/src/ports/index.ts
💤 Files with no reviewable changes (2)
- packages/factory-sdk/src/ports/index.ts
- packages/factory-sdk/src/ports/fleet.ts
| function assertSelfNode(node: SpawnInput['node']): void { | ||
| if (node && node !== 'self') { | ||
| throw new Error(`InternalFleetClient only supports node 'self' tonight; received ${node}`) | ||
| } | ||
| } |
There was a problem hiding this comment.
Typo in error message: "tonight" should be "currently".
The error message contains what appears to be a leftover debug phrase.
✏️ Proposed fix
function assertSelfNode(node: SpawnInput['node']): void {
if (node && node !== 'self') {
- throw new Error(`InternalFleetClient only supports node 'self' tonight; received ${node}`)
+ throw new Error(`InternalFleetClient only supports node 'self'; received ${node}`)
}
}🤖 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 `@packages/factory-sdk/src/fleet/internal-fleet-client.ts` around lines 315 -
319, The error message in function assertSelfNode currently reads "...only
supports node 'self' tonight"; update that string to use "currently" instead of
"tonight" so the thrown Error becomes `InternalFleetClient only supports node
'self' currently; received ${node}` (keep the same function assertSelfNode and
the SpawnInput['node'] check/throw logic).
kjgbot
force-pushed
the
factory-sdk/w5-fleet
branch
from
c44b5ab to
806b022
Compare
1 participant
Summary
InternalFleetClientover@agent-relay/harness-driverwith the W5 mapping table: capability to CLI,cwd,sessionReftocontinueFrom,restartPolicy, release, roster, messaging, delivery failure, and agent exit callbacks.createFleet({ backend })selection with defaultinternaland the existingRelayFleetClientseam forrelay.Verification
npx vitest run packages/factory-sdknpx tsc --noEmit -p tsconfig.node.jsonV0 logic checks only; no live V1 surface verification claimed.