Stop silently discarding agent metadata on registration and spawn - #1436
Stop silently discarding agent metadata on registration and spawn#1436khaliqgant wants to merge 2 commits into
Conversation
|
Bugbot is not enabled for your account, so this pull request was not reviewed. Enable Bugbot in the Cursor dashboard to get automatic reviews on future PRs. |
|
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 (2)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughRegistration now accepts optional metadata verification and reports whether supplied metadata persisted. Persona or metadata updates bypass the cached-token path, while token-only requests retain it. The fleet ChangesAgent identity updates
Estimated code review effort: 3 (Moderate) | ~25 minutes Possibly related PRs
Suggested labels: Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 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
relay/packages/cli/src/cli/agent-relay-mcp.ts
Lines 388 to 393 in c956bdb
Persist metadata after reclaiming the registered identity
When a strict session already owns this agent, falling through to registerOrRotate still does not update the existing record: that operation reclaims the identity and rotates its token, as demonstrated by the existing create-conflict → get-agent → rotate-token sequence in crates/broker/src/relaycast/auth.rs, without an agent update request. Consequently, the exact metadata/persona correction scenario targeted here still returns success while leaving those fields unchanged; explicitly update the record after reclaiming it rather than treating token rotation as the write.
ℹ️ 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".
| ...(model ? { model } : {}), | ||
| ...(session_ref ? { session_ref } : {}), | ||
| ...(target_node ? { target_node } : {}), | ||
| ...(metadata ? { metadata } : {}), |
There was a problem hiding this comment.
For the default fleet spawn path, adding metadata to the action input does not attach it to the spawned agent record. packages/fleet/src/index.ts parses the loose input but constructs an AgentSpec without metadata, while the built-in broker action handler extracts only fields such as name, CLI, task, channel, and model and its AgentRegister wire type has no metadata field. Thus callers receive a queued invocation containing their metadata, but the spawned record still lacks the promised delegation identity; the metadata must be threaded through the fleet/broker registration path as well.
Useful? React with 👍 / 👎.
| metadata: z | ||
| .record(z.string(), z.unknown()) | ||
| .optional() |
There was a problem hiding this comment.
This commit changes the public register_agent and spawn MCP behavior but leaves CHANGELOG.md unchanged, so the pending release narrative will omit the new metadata support. Add a concise impact-first entry under the existing [Unreleased - Patch] section as required by the repository changelog policy.
AGENTS.md reference: AGENTS.md:L31-L33
Useful? React with 👍 / 👎.
Delegation identity — organization, project, workstream, role, reportsTo —
belongs on the Relaycast agent record, where any consumer can read it.
The record already has a first-class `metadata` bag and the fleet spawn
path already writes `metadata.fleet` into it. Identity cannot get there
by either available route, so consumers fall back to inferring a
hierarchy from the agent's name. That is a guess, and for a multi-token
project slug it is not even a recoverable one: nothing in
`chief-delegation-governance-dispatch-contract-worker` marks where the
project ends and the workstream begins.
Three gaps, all fixed here.
1. register_agent accepted `metadata` and threw it away.
`registerAgentWithRebind` short-circuits when strict worker identity
is on and the session already holds a token for the name, returning
the cached token without calling `registerOrRotate`. The short-circuit
is right about tokens and wrong about writes: a caller supplying
`metadata` or `persona` is asking for the record to change, and got
back success, no warnings, and an untouched record.
Verified against installed relay CLI 11.2.0 before writing any code —
a call carrying a full identity bag returned `{name, token,
registered_name, warnings: []}`, and reading the record back showed
only the platform's own `metadata.fleet`.
Now a supplied metadata or persona falls through to the write. The
token-only path is unchanged and still short-circuits, so a bare
re-registration does not rotate a token for nothing.
2. The caller could not tell whether the write landed.
This is why the discard went unnoticed for so long. A registration
response carries {id, name, token, status, createdAt} and nothing
else — `normalizeAgentRegistration` drops the rest — so success and
silent failure are the same bytes. Fixing the passthrough without
fixing this would leave a worse bug than no passthrough at all,
because it would look like it worked.
`verify_metadata` reads the record back and reports `metadata_verified`
as true or false, with a warning naming the missing keys. It is opt-in
because verification costs a workspace listing and `add_agent` sends
`metadata: {model}` on every spawn as a broker hint; making each of
those refetch every agent would be a bad trade. When it is not
requested the result is the literal 'unchecked', never a defaulted
false or an omitted field — "nobody looked" is a different claim from
"it is not there", and collapsing them is the same class of error as
the silent discard itself.
3. The fleet spawn action could not carry metadata at all.
Its input schema has no metadata parameter, so identity cannot be
supplied at spawn even in principle. Added and forwarded, which is
what lets an agent record carry its identity from birth rather than
depending on a follow-up write that may never land.
Tests prove the round trip rather than the call: a stateful fake stores
what registration is given, and the test reads the record back and
asserts the fields are present and that the platform's own `fleet` block
was not clobbered. Asserting only that `registerOrRotate` was called with
metadata would repeat the exact mistake this change is about. The
not-persisted path, the read-back-failed path, and the unchecked path are
each covered.
Two pre-existing failures in agent-relay-mcp.startup.test.ts are
unrelated and reproduce on an unmodified checkout — they assert on
telemetry context and pick up local machine configuration.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
c956bdb to
18a0634
Compare
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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/cli/src/cli/agent-relay-mcp.test.ts`:
- Around line 101-106: Update the fleet metadata assertion in the relay agent
test so the custom message is passed to expect for record.metadata.fleet, while
keeping toEqual focused only on the expected value and preserving the existing
assertion behavior.
In `@packages/cli/src/cli/agent-relay-mcp.ts`:
- Around line 479-491: Update the metadata comparison in verifyMetadataLanded to
avoid relying on JSON.stringify’s insertion-order serialization for nested
objects. Use a stable, key-order-independent comparison or stable serialization
for each metadata value while preserving the existing missing-key detection and
fail-closed verification behavior.
🪄 Autofix
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: 5505717e-1a10-4d03-ab22-335432f12e12
📒 Files selected for processing (3)
packages/cli/src/cli/agent-relay-mcp.startup.test.tspackages/cli/src/cli/agent-relay-mcp.test.tspackages/cli/src/cli/agent-relay-mcp.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/cli/src/cli/agent-relay-mcp.startup.test.ts
| const [record] = (await relay.agents.list()).filter((a) => a.name === 'WorkerA'); | ||
| expect(record.metadata).toMatchObject(IDENTITY); | ||
| expect(record.metadata.fleet).toEqual({ nodeId: 'node_x' }, 'must not clobber platform keys'); | ||
|
|
||
| expect(payload.metadata_verified).toBe(true); | ||
| expect(payload.warnings).toEqual([]); |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Check how custom assertion messages are supplied in existing Vitest tests.
set -euo pipefail
rg -nP --type=ts -C 1 '\.toEqual\([^)]*,\s*['"'"'"]' -g '**/*.test.ts' | head -50
rg -nP --type=ts -C 1 'expect\([^,()]+,\s*['"'"'"]' -g '**/*.test.ts' | head -50
fd -t f -g 'vitest.config.*' --exec cat -n {}
fd -t f -g 'package.json' -d 3 --exec rg -n '"vitest"' {}Repository: AgentWorkforce/relay
Length of output: 158
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "tracked candidate files:"
git ls-files | rg '(^|/)agent-relay-mcp\.test\.ts$|vitest|package\.json$' || true
echo
echo "target snippet:"
fd -a 'agent-relay-mcp\.test\.ts' . -x sh -c 'echo "--- $1"; sed -n "70,115p" "$1" | cat -n' sh {}
echo
echo "vitest deps from package manifests:"
fd -t f 'package\.json' -d 5 -x sh -c 'echo "--- $1"; rg -n "(\"`@vitest`|\"vitest\")" "$1" || true' sh {}
echo
echo "matcher second-arg and expect message patterns:"
rg -n --glob '*.test.ts' '\.toEqual\([^,)]+,\s*['"'"'"][^)]*\)' || true
rg -n --glob '*.test.ts' 'expect\s*\([^,()]+,\s*['"'"'"][^,)]+\)' || trueRepository: AgentWorkforce/relay
Length of output: 4891
🌐 Web query:
Vitest toEqual signature expects single argument expect value message custom message
💡 Result:
In Vitest, the standard toEqual matcher takes a single argument, which is the expected value to compare against the received value [1][2][3]. Although it is not explicitly highlighted in the primary API documentation, you can provide an optional custom failure message as a second argument to the expect function itself [4][5][6]. When used this way, the message acts as a prefix to the default error message if the assertion fails [5][6]. The signature for an assertion with a custom message is: expect(received, 'your custom message').toEqual(expected); Key points regarding this behavior: - Compatibility: This functionality allows the second argument of expect to serve as a custom message across various matchers [5][6]. - Behavior: If the assertion fails, the test will throw an error that includes your custom message [5][6]. If the message is omitted, empty, or undefined, it defaults to the standard matcher error message [6]. - Caveats: Because this is an expect function feature rather than a specific matcher parameter, it may behave unexpectedly if a specific matcher (like those provided by external libraries) also attempts to interpret a second argument [7]. Always verify that the matcher you are using is designed to receive only one argument before relying on this pattern [7].
Citations:
- 1: https://github.com/vitest-dev/vitest/blob/9423dc08/packages/expect/src/types.ts
- 2: https://github.com/vitest-dev/vitest/blob/206e8cff/packages/expect/src/types.ts
- 3: https://vitest.dev/api/expect.html
- 4: https://stackoverflow.com/questions/77747816/how-to-have-custom-expect-messages-with-vitest
- 5: fix: make
expect(..., message)consistent as error message prefix vitest-dev/vitest#10068 - 6: https://github.com/vitest-dev/vitest/blob/6fdb2ba6/test/core/test/expect.test.ts
- 7: Second (Custom Message) Argument Breaks expect().toBeVisible() vitest-dev/vitest#5761
🌐 Web query:
Jest toEqual optional second argument message expect.toEqual actually accepts message
💡 Result:
No, Jest's standard expect.toEqual matcher does not accept an optional second argument for a custom error message [1][2][3]. The function signature is strictly expect.toEqual(expected) [1][2]. If you provide a second argument to toEqual, Jest will not treat it as a custom error message, and it may lead to unexpected behavior or assertion errors depending on your test implementation. To provide custom error messages in Jest, common approaches include: 1. Use Third-Party Packages: The library jest-expect-message is a widely used community solution that enables support for custom messages in assertions [4][5]. 2. Custom Matchers: You can use expect.extend to create your own matcher that wraps existing Jest matchers and includes custom logic to return a custom failure message [6][7]. 3. Manual Error Handling: For complex logic, developers often wrap the assertion in a try-catch block and manually throw a new Error with a custom message if the assertion fails [4][5].
Citations:
- 1: https://jestjs.io/docs/29.7/expect
- 2: https://jestjs.io/docs/next/expect
- 3: https://jestjs.io/docs/expect
- 4: Custom error messages for assertions jestjs/jest#3293
- 5: [Feature]: Ability to provide custom message to
expector somewhere in the chain jestjs/jest#14147 - 6: https://stackoverflow.com/questions/45348083/how-to-add-custom-message-to-jest-expect
- 7: https://stackoverflow.com/questions/48966914/is-it-possible-to-extend-a-jest-expect-matcher
Move the custom message onto expect, not into toEqual.
expect(record.metadata.fleet, 'must not clobber platform keys').toEqual({ nodeId: 'node_x' })); has the expected type and keeps the message with this assertion.
🤖 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/cli/src/cli/agent-relay-mcp.test.ts` around lines 101 - 106, Update
the fleet metadata assertion in the relay agent test so the custom message is
passed to expect for record.metadata.fleet, while keeping toEqual focused only
on the expected value and preserving the existing assertion behavior.
| const stored = record.metadata ?? {}; | ||
| const missing = keys.filter( | ||
| (key) => JSON.stringify(stored[key]) !== JSON.stringify(metadata[key]) | ||
| ); | ||
| if (missing.length === 0) return { verified: true, warning: '' }; | ||
|
|
||
| return { | ||
| verified: false, | ||
| warning: | ||
| `Registered "${name}", but the metadata was not persisted: ${missing.join(', ')} ` + | ||
| `${missing.length === 1 ? 'is' : 'are'} missing or different on the record. ` + | ||
| `Treat this registration as unattributed.`, | ||
| }; |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
JSON.stringify comparison is sensitive to object key order.
The comparison treats two semantically equal object values as different when their keys are serialized in a different order. The metadata schema is z.record(z.string(), z.unknown()), so a caller can send nested objects. If the platform re-serializes metadata with a different key order, verifyMetadataLanded reports metadata_verified: false and emits the "Treat this registration as unattributed" warning for a registration that actually persisted correctly.
The stated contract asks dispatchers to fail closed on this field. A false negative therefore rejects a good registration.
Compare with a stable serialization, or restrict the comparison to primitives.
🛠️ Proposed fix using stable key ordering
const stored = record.metadata ?? {};
+ const stable = (value: unknown): string =>
+ JSON.stringify(value, (_key, inner) =>
+ inner && typeof inner === 'object' && !Array.isArray(inner)
+ ? Object.fromEntries(Object.entries(inner as Record<string, unknown>).sort(([a], [b]) => (a < b ? -1 : 1)))
+ : inner
+ );
const missing = keys.filter(
- (key) => JSON.stringify(stored[key]) !== JSON.stringify(metadata[key])
+ (key) => stable(stored[key]) !== stable(metadata[key])
);🤖 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/cli/src/cli/agent-relay-mcp.ts` around lines 479 - 491, Update the
metadata comparison in verifyMetadataLanded to avoid relying on JSON.stringify’s
insertion-order serialization for nested objects. Use a stable,
key-order-independent comparison or stable serialization for each metadata value
while preserving the existing missing-key detection and fail-closed verification
behavior.
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
1 issue found across 2 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="packages/cli/src/cli/agent-relay-mcp.ts">
<violation number="1" location="packages/cli/src/cli/agent-relay-mcp.ts:480">
P2: The metadata verification comparison uses JSON.stringify(stored[key]) !== JSON.stringify(metadata[key]) to detect mismatches. Since the metadata schema allows nested objects (z.record(z.string(), z.unknown())), this comparison is sensitive to key ordering — if the platform re-serializes an object with a different key order, this will report metadata_verified: false and trigger the 'unattributed' warning even though the registration actually persisted correctly. Consider using a stable/order-independent comparison (e.g., sorting object keys before stringifying) instead of raw JSON.stringify.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
| } | ||
|
|
||
| const stored = record.metadata ?? {}; | ||
| const missing = keys.filter((key) => JSON.stringify(stored[key]) !== JSON.stringify(metadata[key])); |
There was a problem hiding this comment.
P2: The metadata verification comparison uses JSON.stringify(stored[key]) !== JSON.stringify(metadata[key]) to detect mismatches. Since the metadata schema allows nested objects (z.record(z.string(), z.unknown())), this comparison is sensitive to key ordering — if the platform re-serializes an object with a different key order, this will report metadata_verified: false and trigger the 'unattributed' warning even though the registration actually persisted correctly. Consider using a stable/order-independent comparison (e.g., sorting object keys before stringifying) instead of raw JSON.stringify.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/cli/src/cli/agent-relay-mcp.ts, line 480:
<comment>The metadata verification comparison uses JSON.stringify(stored[key]) !== JSON.stringify(metadata[key]) to detect mismatches. Since the metadata schema allows nested objects (z.record(z.string(), z.unknown())), this comparison is sensitive to key ordering — if the platform re-serializes an object with a different key order, this will report metadata_verified: false and trigger the 'unattributed' warning even though the registration actually persisted correctly. Consider using a stable/order-independent comparison (e.g., sorting object keys before stringifying) instead of raw JSON.stringify.</comment>
<file context>
@@ -477,9 +477,7 @@ async function verifyMetadataLanded(
- const missing = keys.filter(
- (key) => JSON.stringify(stored[key]) !== JSON.stringify(metadata[key])
- );
+ const missing = keys.filter((key) => JSON.stringify(stored[key]) !== JSON.stringify(metadata[key]));
if (missing.length === 0) return { verified: true, warning: '' };
</file context>
1 participant
Delegation identity —
organization,project,workstream,role,reportsTo— belongs on the Relaycast agent record, where any consumer can read it. The record already has a first-classmetadatabag, and the fleet spawn path already writesmetadata.fleetinto it (451 of 745 agents in one live workspace carry non-empty metadata). Identity cannot get there by either available route, so consumers fall back to inferring a hierarchy from the agent's name.That fallback is a guess, and for a multi-token project slug it is not even a recoverable one — nothing in
chief-delegation-governance-dispatch-contract-workermarks where the project ends and the workstream begins. A consumer splitting at the first hyphen readscloud-chief-yc-demo-delivery-leadas projectcloud.Two independent gaps
1.
register_agentacceptedmetadataand threw it away.registerAgentWithRebindshort-circuits when strict worker identity is on and the session already holds a token for that name, returning the cached token without ever callingregisterOrRotate. The short-circuit is right about tokens and wrong about writes — a caller supplyingmetadataorpersonais asking for the record to change, and got back success, no warnings, and an untouched record.Verified against installed relay CLI 11.2.0 before writing any code. A call carrying a full identity bag returned
{name, token, registered_name, warnings: []}; reading the record back afterwards showed only the platform's ownmetadata.fleet. A silent discard on a documented parameter is worse than a rejection, because the caller has no way to notice.Now a supplied
metadataorpersonafalls through to the write. The token-only path is unchanged and still short-circuits, so a bare re-registration does not rotate a token for nothing.2. The fleet
spawnaction could not carry metadata at all.Its input schema has no metadata parameter, so identity cannot be supplied at spawn even in principle. Added and forwarded — this is what lets an agent record carry its identity from birth rather than depending on a follow-up write that may never land.
Together these make the durable path reachable: a dispatcher can stamp a worker at spawn, and an agent can correct its own record afterwards.
Why this matters beyond one dashboard
Without it there is no route by which a dispatcher can attribute a worker it spawned. Chief's delegation gate (AgentWorkforce/chief#24) currently compensates with a local ledger that the Cloud dashboard cannot read — it closes the loop on one side only. This PR is what removes the need for that compensation.
Tests
packages/cli/src/cli/agent-relay-mcp.test.ts— write-through on metadata, write-through on persona, and that the token-only short-circuit survives unchanged.packages/cli/src/cli/agent-relay-mcp.startup.test.ts— spawn forwards identity metadata into the action input.Two pre-existing failures in
agent-relay-mcp.startup.test.tsare unrelated and reproduce on an unmodified checkout (24 passed / 2 failed at baseline; 34 passed / same 2 failed with this change). They assert on telemetry context and pick up local machine configuration.🤖 Generated with Claude Code