fix(factory): make the relay workspace identity configurable - #311
Conversation
The relay fleet backend registers a workspace agent for the factory itself, defaulting to `factory`. `createFleet` never passed an `agentName`, and no config key existed to supply one, so every relay deployment necessarily registered the same identity. A host that cannot prove it still owns that name after a restart re-registers, collides with its own prior registration (`Agent "factory" already exists in this workspace`), and the fleet control plane never initialises -- no agent is placed and nothing is dispatched. Add `relay.agentName` to the config schema and thread it through `createFleet` into `RelayFleetClient`. The key is optional and is deliberately not defaulted in the schema: `RelayFleetClient` keeps owning the `factory` fallback, so a config that omits it resolves to exactly the identity it resolves to today. An empty or whitespace-only value is rejected at config load rather than silently coerced to the default -- a silently-defaulted identity is how this stayed invisible. `RelayFleetClient` gains a read-only `agentName` accessor so a caller can confirm which identity a configuration resolved to. Registration is unchanged. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Session-Id: f425b9b8-04fe-42c6-8e24-808c8664dd8c
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (7)
Included review availability: Your plan provides up to 1 included review per hour; 0 remain after this review. 📝 WalkthroughWalkthroughThe factory schema now supports an optional trimmed relay agent name. Fleet construction forwards this value through CLI and ChangesRelay agent identity
Estimated code review effort: 3 (Moderate) | ~20 minutes Merge Risk: ⚪ Minimal · up to This change makes the relay workspace identity configurable, rejects invalid values, and preserves the existing Sequence Diagram(s)sequenceDiagram
participant FactoryConfigSchema
participant buildFleet
participant createFleet
participant RelayFleetClient
FactoryConfigSchema->>buildFleet: parse relay.agentName
buildFleet->>createFleet: pass relayAgentName
createFleet->>RelayFleetClient: construct with agentName
RelayFleetClient-->>createFleet: expose resolved agentName
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 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: 43308018a9
ℹ️ 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".
| liveSubscription: liveSubscriptionSchema, | ||
| dispatch: dispatchSchema, | ||
| fleetHealth: fleetHealthSchema, | ||
| relay: relaySchema, |
There was a problem hiding this comment.
In split-config/cloud deployments, WorkspaceConfig is the durable configuration shared by the workspace, while NodeConfig is the per-host half. Defining relay.agentName only on WorkspaceConfigObjectSchema therefore gives every Factory deployment in that workspace the same identity, and NodeConfigSchema strips the key if a deployment tries to set its own value there. With two deployments, the second still encounters the registration collision described above and its fleet control plane cannot initialize; this identity needs to be accepted as node/deployment-local configuration and merged from that half.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 9373e35 — but the diagnosis needed one correction, so recording what I verified.
Half right. I probed loadFactoryConfig directly rather than reasoning from the schema. combineSplitConfigInput merges the two raw halves before FactoryConfigSchema.parse runs, so a nodeConfig half setting relay.agentName already reached factoryConfig.relay.agentName and already took precedence over the workspace half. The stated consequence — "the second deployment still encounters the registration collision" — was therefore not reachable: a per-deployment identity was settable from the node half as written.
The real gap, which is worth fixing. NodeConfigSchema does strip the key, so the node-half view returned by loadFactoryConfig carried no relay, while workspaceConfig did. A per-host identity was reflected back as workspace-shared configuration, and anything round-tripping the split halves would migrate one deployment's identity onto every other deployment in the workspace — the collision this setting exists to prevent, arriving by a different route.
So relay is now declared on both halves, mirroring how preview already is: workspace half = shared default, node half = per-host override. The two relay objects are merged rather than replaced, so a node half pinning only agentName cannot drop other workspace-half settings.
Covered by three new tests in src/config/schema.test.ts. Only one of them fails against the previous commit (TypeError on loaded.nodeConfig.relay) — the other two passed already, which is precisely the evidence that the runtime path was not broken. Full suite green apart from one pre-existing flake reproduced on unmodified 2e52791.
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
No issues found across 7 files
You’re at about 94% of the monthly reviewed-line limit. You may want to disable incremental reviews to conserve quota. Reviews will continue until that limit is exceeded. If you need help avoiding interruptions, please contact contact@cubic.dev.
Review flagged that `relay.agentName` lived only on the workspace half. The runtime config was already correct -- `combineSplitConfigInput` merges the raw halves before parsing, so a node-half value did reach `factoryConfig` and did take precedence -- but the node-half *view* dropped the key, so a per-host identity was reflected back as workspace-shared configuration. Anything round-tripping the split halves would migrate one deployment's identity onto every other deployment in the workspace: exactly the collision this setting exists to prevent. Declare `relay` on `NodeConfigObjectSchema` and carry it in the node-half projection, mirroring how `preview` is declared on both halves. The two halves' `relay` objects are now merged rather than replaced, so a node half pinning only `agentName` cannot drop other workspace-half settings. Precedence is unchanged and now covered: node half wins, workspace half supplies a shared default, and omitting both still resolves to `factory`. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Session-Id: f425b9b8-04fe-42c6-8e24-808c8664dd8c
There was a problem hiding this comment.
All reported issues were addressed across 2 files (changes from recent commits).
You’re at about 94% of the monthly reviewed-line limit. You may want to disable incremental reviews to conserve quota. Reviews will continue until that limit is exceeded. If you need help avoiding interruptions, please contact contact@cubic.dev.
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
Review caught a real hole in the split-config merge. `{...workspaceRelay,
...nodeRelay}` discards a workspace-half `agentName` whenever the node
half overrides it, so an invalid shared value never reached `relaySchema`
and a broken workspace config loaded clean on every host that happened to
set its own identity. That is the silent acceptance this key exists to
prevent, arriving one level up from the value itself.
Validate each half before the merge, mirroring `validateClonePathSyntax`,
which already validates both halves before node-local values take
precedence. The node half was in fact already rejected by the final parse;
it now fails with a message naming the offending half instead of a raw
issue list.
Also pins what the workspace-shaped view reports after a node override.
`normalizeLoadedConfig` projects both views from the *merged* config, so
that view carries the effective identity -- as it already does for
preview, cloneRoot, and clonePaths. Nothing serializes `workspaceConfig`
back to a shared file today, so this documents existing semantics rather
than changing them, and fails loudly if the projection moves under a
caller who starts to.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Session-Id: f425b9b8-04fe-42c6-8e24-808c8664dd8c
1 participant
Problem
The
relayfleet backend registers a workspace agent for the factory itself — not one it spawns.RelayFleetClientresolves that identity asoptions.agentName ?? DEFAULT_AGENT_NAME('factory'), butcreateFleetconstructed it with only{ workspaceKey, env, log }. Nothing ever passedagentName, and no config key existed to supply one, so every relay deployment necessarily registered the same identity.A host that cannot prove it still owns that name after a restart re-registers and collides with its own prior registration:
The fleet control plane then never initialises, the breaker opens, no agent is placed, and nothing is dispatched.
Change
Config plumbing only — three pieces:
relay.agentNamein the config schema (src/config/schema.ts). Optional, and deliberately not.default(...):RelayFleetClientkeeps owning the'factory'fallback, so the default lives in exactly one place and cannot drift.DEFAULT_AGENT_NAMEis untouched. Declared on both config halves, mirroringpreview: the workspace half carries a shared default, the node half overrides it per host. An identity that could only live on the shared half would be handed to every deployment in the workspace — the collision this exists to prevent.createFleet(src/fleet/create-fleet.ts,src/cli/fleet.ts) asrelayAgentNameinto theRelayFleetClientconstructor. This is the one-line gap..trim().min(1)means an empty or whitespace-only value fails validation instead of silently becomingfactory. A silently-defaulted identity is precisely how an unconfigurable identity stayed invisible. Therelayobject is.strict(), soagentname:is a load error rather than an ignored typo.RelayFleetClientgains a read-onlyagentNameaccessor so a caller can confirm which identity a configuration actually resolved to. Registration logic is unchanged — it still reads the private#agentNameexactly as before.Not in scope: recovery-credential enrolment so an ephemeral host self-recovers its identity across restarts. That is a separate, larger change.
Compatibility
A config that omits
relay.agentNameproducesundefined, whichRelayFleetClientresolves tofactory— byte-for-byte the identity every existing deployment registers today. No deployment changes identity on upgrade.Tests — the required pair, each stated fail-before / pass-after
Verified by running the new tests against unmodified
2e52791in a separate worktree, then against this branch.MUST-FIRE — an explicit name reaches the client.
src/cli/fleet.test.ts› forwards a configured relay agent name to fleet construction — the sharpest one: the CLI loads a config pinningrelay.agentName: 'factory-cloud'and asserts fleet construction receives it.expected undefined to be 'factory-cloud'. That is the outage in one assertion: the operator pinned a name and construction still got nothing.src/fleet/create-fleet.test.ts› registers under the agent name the config supplies — parses a real config and assertsRelayFleetClient.agentName === 'factory-cloud'.TypeError: Cannot read properties of undefined (reading 'agentName'), becauseconfig.relaydoes not exist.MUST-NOT-FIRE — omitting it still yields
factory.src/fleet/create-fleet.test.ts› keeps the built-infactoryidentity when the config omits an agent name, andsrc/cli/fleet.test.ts› leaves the relay agent name unset when the config omits it.create-fleetcase fails with the sameTypeError(the field does not exist yet); the CLI case passes trivially, sinceundefinedis what the old code already forwarded.Validation —
src/config/schema.test.ts› rejects empty / whitespace-only / tabrelay.agentNameat config load instead of defaulting it, asserted by issue path and code (relay.agentName,too_small) rather than message text.expected true to be false— base silently acceptsagentName: '', which is the exact silent-default failure mode.Split configs (added after review)
Three further tests in
src/config/schema.test.tscover the case where a cloud deployment and another deployment share one workspace: the node half pins the identity and is carried back onnodeConfig; the node half overrides a workspace-half default; the workspace half alone still applies.Worth recording precisely, since the review's stated failure mode was not reachable:
combineSplitConfigInputmerges the two raw halves beforeFactoryConfigSchema.parse, so a node-halfrelay.agentNamealready reached the runtime config and already won. Two of the three tests therefore passed before the follow-up commit. The genuine gap was thatNodeConfigSchemastripped the key, so the node-half view dropped it and a per-host identity was reflected back as workspace-shared config — that one assertion fails before (TypeErroronloaded.nodeConfig.relay) and passes after.Split-config validation (added after review)
combineSplitConfigInputmerges the two halves'relayobjects, which meant a workspace-halfagentNamewas discarded unvalidated whenever the node half overrode it — a broken shared config would load clean on every host that set its own identity. Each half is now validated before the merge, mirroringvalidateClonePathSyntaxdirectly above it, and the error names the offending half.Verified against the prior head: the workspace-half case did not throw at all there; the node-half case already threw via the final parse and only gains a clearer message.
A further test pins what the workspace-shaped view reports after a node override.
normalizeLoadedConfigprojects both views from the merged config, so that view carries the effective identity — as it already does forpreview,cloneRoot, andclonePaths. Nothing serializesworkspaceConfigback to a shared file today, so this documents existing semantics rather than changing them.Verification
npm run build— exit 0.npm run featuremap:check— exit 0.npm test(full suite, final head) — 1955 passed, 1 failed, a pre-existing flake. CI'spackagejob hit the same one on the first run and passed on a re-run of the identical SHA.fleet CLI runtime > keeps relay dispatch ownership…—expected 3 to be +0. Ransrc/cli/fleet.test.ts3× on this branch (2 fail) and 3× on pristine2e52791(1 fail). Structurally it cannot be this change: that test injectsdeps.fleet, sobuildFleetreturns before it ever readsloaded.config.relay.GitAgentWorktreeManager > discovers every run…—Test timed out in 5000ms; passes when its file is run alone. Load-only, unrelated to this change.All exit codes captured from
$?, not from log text.