fix(cli): unblock Publish Package — mock @agent-relay/sdk boundary in MCP startup test#1135
Conversation
|
Warning You have reached your daily quota limit. Please wait up to 24 hours and I will start processing your requests again! |
📝 WalkthroughWalkthroughThe test file for the MCP startup flow adds a dedicated ChangesCLI startup test: deterministic AgentRelay mock
Estimated code review effort🎯 2 (Simple) | ⏱️ ~5 minutes Possibly related issues
Possibly related PRs
Suggested reviewers
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)
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 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
`@tests/integration/broker/evals-reports/report-2026-06-06T16-04-38-821Z-claude.json`:
- Around line 21-38: The scenario object in the fixture is missing required
fields that are mandatory according to tests/integration/broker/evals/types.ts
and are dereferenced by the HTML renderer. Add the required agents and
transcript fields to the scenario object (within the "01-dm-roundtrip" scenario
that starts at line 21) to ensure the fixture can round-trip through the eval
pipeline and be properly rendered by the HTML renderer.
In
`@tests/integration/broker/evals-reports/report-2026-06-06T22-01-51-691Z-claude.json`:
- Around line 98-110: The totalIntents field is currently set to 1, but the
notes field confirms both ACK and DONE intents were observed in the transcript,
indicating there are actually 2 distinct protocol intents. Update the
totalIntents value from 1 to 2 to accurately reflect the number of distinct
protocol intents present in this test fixture and ensure the report metrics are
correctly calculated.
In `@web/content/docs/fleets.mdx`:
- Around line 153-158: The shell command example contains inline comments placed
after line continuation backslashes on lines 154, 155, and 156, which breaks
command continuation when pasted. Remove the inline comments that appear after
the backslash characters on those lines (the comments about "override the node
name from the definition", "pass the workspace key inline instead of via env",
and "cap concurrently managed agents on this node"). The backslash must be the
last character on continuation lines with no text or comments following it.
Consider placing these explanatory comments on separate lines above the command
block or in a separate documentation section instead.
In `@web/next-env.d.ts`:
- Line 3: The import statement in next-env.d.ts is using the dev-specific path
`./.next/dev/types/routes.d.ts` which is not guaranteed to exist in all
environments (build, CI). Replace the dev-specific import path with the stable
Next.js 15 designated path `./.next/types/routes.d.ts`. Since this file is
auto-generated, try regenerating your build output first, which should correct
the path automatically. If the file is still not updated after regeneration,
manually edit the import to use the stable path `./.next/types/routes.d.ts`
instead.
🪄 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: e6b7dbd3-3646-4db3-b743-f9ffee87f7d0
📒 Files selected for processing (17)
packages/cli/src/cli/agent-relay-mcp.startup.test.tstests/integration/broker/evals-reports/report-2026-06-06T16-04-38-821Z-claude.jsontests/integration/broker/evals-reports/report-2026-06-06T21-57-43-350Z-claude.htmltests/integration/broker/evals-reports/report-2026-06-06T21-57-43-350Z-claude.jsontests/integration/broker/evals-reports/report-2026-06-06T22-01-51-691Z-claude.htmltests/integration/broker/evals-reports/report-2026-06-06T22-01-51-691Z-claude.jsontests/integration/broker/evals-reports/report-2026-06-07T02-39-03-749Z-claude.htmltests/integration/broker/evals-reports/report-2026-06-07T02-39-03-749Z-claude.jsonweb/app/docs/[slug]/page.tsxweb/components/docs/Mermaid.tsxweb/components/docs/docs.module.cssweb/content/docs/fleets.mdxweb/lib/docs-nav.tsweb/lib/docs-versions.tsweb/lib/test/docs-versions.test.tsweb/next-env.d.tsweb/package.json
💤 Files with no reviewable changes (1)
- web/lib/test/docs-versions.test.ts
| "scenarios": [ | ||
| { | ||
| "id": "01-dm-roundtrip", | ||
| "title": "DM round-trip (A → B)", | ||
| "pass": true, | ||
| "sent": 1, | ||
| "expected": 1, | ||
| "phantoms": [], | ||
| "totalIntents": 0, | ||
| "protocolAdherence": null, | ||
| "wrongChannelReplies": 0, | ||
| "deliveryOk": true, | ||
| "events": { | ||
| "relayInbound": 2, | ||
| "dropped": 0, | ||
| "aclDenied": 0 | ||
| } | ||
| } |
There was a problem hiding this comment.
Restore the required scenario payload.
agents and transcript are mandatory in tests/integration/broker/evals/types.ts, and the HTML renderer dereferences both. As written, this fixture will not round-trip through the eval pipeline.
🤖 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
`@tests/integration/broker/evals-reports/report-2026-06-06T16-04-38-821Z-claude.json`
around lines 21 - 38, The scenario object in the fixture is missing required
fields that are mandatory according to tests/integration/broker/evals/types.ts
and are dereferenced by the HTML renderer. Add the required agents and
transcript fields to the scenario object (within the "01-dm-roundtrip" scenario
that starts at line 21) to ensure the fixture can round-trip through the eval
pipeline and be properly rendered by the HTML renderer.
| "sent": 2, | ||
| "expected": 2, | ||
| "phantoms": [], | ||
| "totalIntents": 1, | ||
| "protocolAdherence": 1, | ||
| "wrongChannelReplies": 0, | ||
| "deliveryOk": true, | ||
| "events": { | ||
| "relayInbound": 3, | ||
| "dropped": 0, | ||
| "aclDenied": 0 | ||
| }, | ||
| "notes": "ack=true done=true order=true" |
There was a problem hiding this comment.
Correct the ACK/DONE intent count.
totalIntents is 1, but this transcript contains two distinct protocol intents (ACK and DONE), and the notes field confirms both were observed. That undercounts the scenario and skews the report metrics derived from this fixture.
🤖 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
`@tests/integration/broker/evals-reports/report-2026-06-06T22-01-51-691Z-claude.json`
around lines 98 - 110, The totalIntents field is currently set to 1, but the
notes field confirms both ACK and DONE intents were observed in the transcript,
indicating there are actually 2 distinct protocol intents. Update the
totalIntents value from 1 to 2 to accurately reflect the number of distinct
protocol intents present in this test fixture and ensure the report metrics are
correctly calculated.
| /// <reference types="next" /> | ||
| /// <reference types="next/image-types/global" /> | ||
| import './.next/types/routes.d.ts'; | ||
| import './.next/dev/types/routes.d.ts'; |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
In Next.js 15, what is the supported/expected path for generated typed-routes declarations referenced by next-env.d.ts, and is './.next/dev/types/routes.d.ts' valid for non-dev (build/CI) workflows?
💡 Result:
In Next.js 15, the supported and expected path for typed-routes declarations referenced in next-env.d.ts is ./.next/types/routes.d.ts [1][2]. This file serves as a stable entry point or proxy [3][4]. Under the hood, Next.js manages its internal structure to handle differences between development and production (build/CI) environments [3][5]. Specifically: 1. Stable Path: next-env.d.ts is designed to reference the stable path ./.next/types/routes.d.ts regardless of the current mode [3][4]. 2. Proxy Mechanism: When running in development (especially with features like isolatedDevBuild), Next.js may write a proxy file at .next/types/routes.d.ts that re-exports types from a deeper location (such as ./.next/dev/types/routes.d.ts) [3][4]. 3. Build/CI Workflows: In non-dev environments (e.g., next build or next typegen), the system generates the necessary definitions such that they are reachable via the stable path [6][7]. Is './.next/dev/types/routes.d.ts' valid for non-dev (build/CI) workflows? No, it is not considered the valid or stable path for build/CI workflows. Referencing ./.next/dev/types/routes.d.ts directly is an implementation detail of the development environment [3][4]. Hardcoding this path can lead to issues because: - The /dev/ directory is not guaranteed to exist or contain the required declarations during a standard production build [3][5]. - Next.js expects you to rely on the generated next-env.d.ts and the stable .next/types//*.ts glob pattern in your tsconfig.json to handle resolution automatically [6][8]. You should not manually edit next-env.d.ts to point to development-specific paths [6][7]. If you encounter a situation where next-env.d.ts is explicitly importing from a /dev/ path (e.g., during specific CLI commands), it is typically a sign that the generated file is reflecting a local state rather than the canonical production configuration [9]. Ensure your tsconfig.json includes .next/types//*.ts to allow the TypeScript compiler to resolve these routes correctly in all environments [6][10].
Citations:
- 1: vercel/next.js@03c8e5d
- 2: acdlite/next.js@2b5c714
- 3: dx: avoid next-env.d.ts change in dev vercel/next.js#88103
- 4: dx: avoid next-env.d.ts change in dev vercel/next.js#88103
- 5: keep next-env.d.s unchanged between dev and build vercel/next.js#88428
- 6: https://nextjs.org/docs/15/app/api-reference/config/typescript
- 7: https://nextjs.org/docs/app/api-reference/config/typescript
- 8: https://vercel-next-js.mintlify.app/api-reference/config/typescript
- 9: next-env.d.ts changes route type import between next dev and next typegen vercel/next.js#94153
- 10: Document TSConfig changed for typedRoutes vercel/next.js#83010
🏁 Script executed:
# Check the actual content of the file
cat -n web/next-env.d.ts
# Also check if tsconfig.json exists and its configuration
find . -maxdepth 2 -name 'tsconfig.json' -type f | head -5Repository: AgentWorkforce/relay
Length of output: 393
🏁 Script executed:
# Check the tsconfig.json files to see if proper includes are set
cat -n ./web/tsconfig.json
echo "---"
cat -n ./tsconfig.jsonRepository: AgentWorkforce/relay
Length of output: 2594
Replace dev-specific import path with stable Next.js generated path.
Line 3 currently imports ./.next/dev/types/routes.d.ts, but Next.js 15 designates ./.next/types/routes.d.ts as the stable path for typed-routes declarations across all environments (dev, build, and CI). The /dev/ path is an implementation detail not guaranteed to exist during build workflows. Although this file is auto-generated, ensure your build process regenerates it with the correct stable path. If regeneration does not fix it, manually update the import to ./.next/types/routes.d.ts.
🤖 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 `@web/next-env.d.ts` at line 3, The import statement in next-env.d.ts is using
the dev-specific path `./.next/dev/types/routes.d.ts` which is not guaranteed to
exist in all environments (build, CI). Replace the dev-specific import path with
the stable Next.js 15 designated path `./.next/types/routes.d.ts`. Since this
file is auto-generated, try regenerating your build output first, which should
correct the path automatically. If the file is still not updated after
regeneration, manually edit the import to use the stable path
`./.next/types/routes.d.ts` instead.
![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: b96b529df7
ℹ️ 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".
| --name builder-1 \ # override the node name from the definition | ||
| --workspace rk_live_... \ # pass the workspace key inline instead of via env | ||
| --max-agents 8 \ # cap concurrently managed agents on this node |
There was a problem hiding this comment.
When this example is copied into a shell, the continuation backslashes are followed by spaces and comments instead of being the final character on the line, so Bash escapes a space and ends the command there; the next lines then run as separate --workspace/--max-agents commands. This makes the documented multi-line fleet serve override example fail for users unless the comments are moved to separate lines or the backslashes are placed immediately before the newline.
Useful? React with 👍 / 👎.
The query_nodes MCP tool constructs `new AgentRelay(...)` from @agent-relay/sdk and calls `.nodes.list()`. AgentRelay wraps @relaycast/sdk internally, so the test mocking only @relaycast/sdk left this path dependent on packages/cli and packages/sdk resolving the SAME physical @relaycast/sdk copy. On main that copy hoists singly and the mock applies. But the Publish Package job regenerates the lockfile (`rm -rf package-lock.json && npm install`) at the freshly-bumped version, and the tree's pre-existing @relaycast/sdk skew (a stray transitive 1.2.0, plus @relayflows/core pinning ^1.1.0) tips the resolver into nesting a duplicate @relaycast/sdk under packages/sdk. AgentRelay's internal client is then the real (unmocked) one and `nodes.list()` escapes to a live HTTP request -> "RelayError: Route not found", failing the release. Mock the direct boundary (@agent-relay/sdk's AgentRelay) instead, so the query_nodes path is independent of node_modules hoisting. Verified by forcing the nested duplicate locally: pre-fix reproduces the exact failure, post-fix passes (16/16). Production code is untouched; in a real install @relaycast/sdk@^4.0.0 dedupes to one copy. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
willwashburn
force-pushed
the
fix/publish-test-relaycast-sdk-duplication
branch
from
4448a11 to
b99a26e
Compare
|
Cleaned up and rebased onto current The PR now contains only the intended one-file change — the Note for reviewers: none of the bot findings touched the actual fix. The legitimate |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
packages/cli/src/cli/agent-relay-mcp.startup.test.ts (1)
210-221: ⚡ Quick winAssert forwarded filters on the new
AgentRelayboundary mock.
agentRelayNodesListis now the right seam, but the test currently only checks output shape. Ifquery_nodesstops forwardingcapability/name, the static mock result still passes. Please assert the call arguments (e.g., on Line 377 path) to lock the contract.Suggested diff
@@ return { mod, mocks: { behavior, serverInstances, relayInstances, + agentRelayNodesList, telemetryTrack, telemetryInit, telemetryShutdown, RelayCast, FakeTransport, @@ const queryNodesResult = await server.tools.get('query_nodes')?.handler({ capability: 'spawn:codex' }); + expect(mocks.agentRelayNodesList).toHaveBeenCalledWith({ + capability: 'spawn:codex', + name: undefined, + }); expect(queryNodesResult.structuredContent.nodes).toEqual([As per coding guidelines, “Trace context ... cross-reference related files,” this recommendation is based on the
query_nodeshandler contract inpackages/cli/src/cli/agent-relay-mcp.ts(relay.nodes.list({ capability, name })).🤖 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.startup.test.ts` around lines 210 - 221, The test mock for agentRelayNodesList only validates the output shape but does not assert that the correct filter arguments (capability and name) are being forwarded to it. Add an assertion on the agentRelayNodesList mock to verify it was called with the expected arguments that match the contract defined in relay.nodes.list({ capability, name }). This will ensure that if query_nodes stops forwarding these filters, the test will fail and catch the regression.
🤖 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 `@packages/cli/src/cli/agent-relay-mcp.startup.test.ts`:
- Around line 210-221: The test mock for agentRelayNodesList only validates the
output shape but does not assert that the correct filter arguments (capability
and name) are being forwarded to it. Add an assertion on the agentRelayNodesList
mock to verify it was called with the expected arguments that match the contract
defined in relay.nodes.list({ capability, name }). This will ensure that if
query_nodes stops forwarding these filters, the test will fail and catch the
regression.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 7ffdd2e9-2813-4e59-8093-4b00e5c37ab1
📒 Files selected for processing (1)
packages/cli/src/cli/agent-relay-mcp.startup.test.ts
1 participant
Problem
The Publish Package workflow (run 27598076243) fails its
Run testsstep with a single failure:This is release-blocking:
npm testruns in the publish job, so the failure aborts the release.Root cause
The
query_nodesMCP tool constructsnew AgentRelay(...)from@agent-relay/sdkand calls.nodes.list().AgentRelaywraps@relaycast/sdkinternally, but the test only mocks@relaycast/sdk— so the path works only whenpackages/cliandpackages/sdkresolve the same physical@relaycast/sdkcopy.main,@relaycast/sdk@4.0.0hoists to a single root copy → the mock applies → green.rm -rf package-lock.json && npm install) at the freshly-bumped version. The tree's pre-existing@relaycast/sdkskew (a stray transitive1.2.0, plus@relayflows/corepinning^1.1.0) tips the resolver into nesting a duplicate@relaycast/sdkunderpackages/sdk.AgentRelay's internal client is then the real, unmocked copy → live HTTP request →Route not found.The stack confirms it: the unmocked client resolves from
packages/sdk/node_modules/@relaycast/sdk, not the hoisted root copy.This is a monorepo publish-time artifact, not a production bug — a real end-user install dedupes
@relaycast/sdk@^4.0.0to one copy, soAgentRelayworks fine. Production code is therefore untouched.Fix
Mock the direct boundary the handler uses (
@agent-relay/sdk'sAgentRelay) instead of the transitive@relaycast/sdk, viaimportActual+ override so the real token-helper exports stay intact. Thequery_nodespath is now independent ofnode_moduleshoisting — aligning with the repo convention to "mock the boundary, not internal modules".Verification
Forced the nested-duplicate scenario locally (
cpthe root@relaycast/sdkintopackages/sdk/node_modules):RelayError: Route not foundpackages/clisuite: 362 passed, 5 skipped.Follow-up (not in this PR)
The underlying dep skew (
@relaycast/sdk@1.2.0/@relayflows/core@^1.1.0) is the same version-hygiene fragility tracked in #1134. This PR makes the test resilient; tightening the dep tree is separate.🤖 Generated with Claude Code