Default factory CLI to real clients#266
Conversation
|
Warning You have reached your daily quota limit. Please wait up to 24 hours and I will start processing your requests again! |
📝 WalkthroughWalkthroughThe PR extends fleet CLI fixture handling with explicit opt-in detection. Configuration loading now distinguishes missing vs. explicitly-set ChangesFleet CLI fixture handling and DI improvements
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
ESLint install failed due to a network error. 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 |
|
Implemented two PR-scope fixes:
Addressed comments
Advisory Notes
Local validation run:
|
|
Reviewed PR #266 against Addressed comments
Advisory Notes
VerificationPassed locally:
I also ran the focused factory CLI test: |
|
Fixed one PR-scoped regression in fixture detection. The PR intended “fixture-less config” to mean the Addressed comments
ValidationPassed locally:
I did not run the macOS-only |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/factory-sdk/src/cli/fleet.ts (1)
285-293:⚠️ Potential issue | 🟠 MajorFix fixture opt-in behavior for
--backend relayinbuildFleet
buildFleetonly switches toFakeFleetClientwhenbackend === 'internal' && hasExplicitFixtureFiles(loaded)(fleet.ts ~287), butbuildMountswitches toFakeMountClientfor fixture files regardless of backend (fleet.ts ~312).- When
--backend relayis used,createFleetreturnsRelayFleetClient, whose methods all throwRelayFleetClient not implemented — see relay#1056(create-fleet.ts / relay-fleet-client.ts). So with fixture files you’ll get a fake mount but a non-functional relay fleet, leading to runtime failure.- Make
buildFleetignore the backend check when fixture files are present (useFakeFleetClientwheneverhasExplicitFixtureFiles), or add an explicit refusal/consistent gating forrelay + fixtureFilesso behavior is predictable.🤖 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/cli/fleet.ts` around lines 285 - 293, buildFleet currently only returns FakeFleetClient when backend === 'internal', causing a mismatch with buildMount which uses FakeMountClient for fixtures regardless of backend; update buildFleet (function buildFleet) to check hasExplicitFixtureFiles(loaded) first and return new FakeFleetClient() whenever fixtures are present (i.e., remove the backend === 'internal' gating), so that when hasExplicitFixtureFiles(loaded) is true you never call createFleet (and thus never instantiate RelayFleetClient), keeping behavior consistent with buildMount and avoiding runtime failures.
🤖 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.
Outside diff comments:
In `@packages/factory-sdk/src/cli/fleet.ts`:
- Around line 285-293: buildFleet currently only returns FakeFleetClient when
backend === 'internal', causing a mismatch with buildMount which uses
FakeMountClient for fixtures regardless of backend; update buildFleet (function
buildFleet) to check hasExplicitFixtureFiles(loaded) first and return new
FakeFleetClient() whenever fixtures are present (i.e., remove the backend ===
'internal' gating), so that when hasExplicitFixtureFiles(loaded) is true you
never call createFleet (and thus never instantiate RelayFleetClient), keeping
behavior consistent with buildMount and avoiding runtime failures.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: f85cffee-09da-4f9d-8663-26dc943709f5
📒 Files selected for processing (2)
packages/factory-sdk/src/cli/fleet.test.tspackages/factory-sdk/src/cli/fleet.ts
kjgbot
force-pushed
the
factory-sdk-v2fix10b-real-sb-impl2
branch
from
e76232a to
51545c0
Compare
1 participant
Summary
loadConfigleavefixtureFilesundefined when the operator config omits it, instead of manufacturing a truthy empty{}.fixtureFilespresent after loadConfig), making real RelayfileCloudMountClient/createFleet the factory CLI default.runFleetCli → loadConfig → buildFleet/buildMount: fixture-less configs use the real constructor seams, while explicit fixture configs still use Fake clients for harness runs.Notes
packages:external) was benign but did not fix this bug; the issue was the CLI loadConfig/buildMount/buildFleet path, not esbuild packaging.Verification
npx vitest run packages/factory-sdk/src/cli/fleet.test.ts→ 15 tests passednpx vitest run packages/factory-sdk→ 21 files, 275 tests passednpx tsc --noEmit→ passedCert Scope
This code path and the binary discovery V1 are writeback-independent and landable now:
node bin/fleet.mjs factory run-once|loop --config <live>should select real clients and enumerate live issues. The full binary reap-orphans quad remains writeback-dependent and should wait for cloud#2106 recovery.