fix(e2e): make the MCP registration test-owned, and name the working directory - #510
Conversation
…directory Both from CodeRabbit on tinyhumansai#484, both real. A fixed server name plus a tolerated 409 let this spec adopt a leftover registration from an earlier run — pointing anywhere at all — pass against it, and then delete a server it never created. The name is unique per run now and 409 is a failure. The cleanup is checked too, but only when the body got that far: an assertion thrown out of a `finally` replaces the real failure with a complaint about cleanup. The README's live-lane recipe ran `cargo build` from the repository root and `npm run e2e:live` from wherever you happened to be.
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
There was a problem hiding this comment.
Your trial has ended. Reactivate Greptile to resume code reviews.
|
@coderabbitai review |
|
|
Warning Review limit reachedYou’ve reached a temporary PR review limit under our Fair Usage Limits Policy. Next review available in: 59 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThe live E2E command now targets the frontend directory. The MCP agent test uses unique server names, requires successful registration, passes the generated name to tool calls, and conditionally validates cleanup. ChangesLive E2E MCP testing
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
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: 3
🤖 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 `@frontend/README.md`:
- Around line 208-210: Update the live E2E command in the README to run from the
repository root without changing directories, while preserving the frontend
package context through the npm invocation. Remove the standalone “cd frontend”
step and keep the existing live E2E behavior unchanged.
In `@frontend/test/e2e/mcp-agent.spec.ts`:
- Around line 86-91: Update the test setup around the server and marker
generation to import and call node:crypto.randomUUID() once per attempt, then
reuse that UUID in both server and marker values. Remove reliance on Date.now()
and preserve the existing per-run uniqueness and cleanup behavior.
- Around line 149-161: Update the cleanup in the finally block around
page.request.delete for server so transport rejections are caught; rethrow the
cleanup error only when bodyPassed is true, and otherwise record it without
replacing the original body failure. Preserve the existing removed.ok assertion
for successful delete responses.
🪄 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
Run ID: 1c426b69-590f-4e57-90b6-02b8a4e521b9
📒 Files selected for processing (2)
frontend/README.mdfrontend/test/e2e/mcp-agent.spec.ts
…after Two more from CodeRabbit on tinyhumansai#510. The first is a hole in the reasoning of the commit it reviews. I guarded the cleanup ASSERTION behind `bodyPassed` so it could not replace a real failure, and left the `page.request.delete` that precedes it unguarded — and that call REJECTS on a transport failure rather than returning a response to inspect. So the exact substitution the guard exists to prevent was still reachable, by the other of the two paths. Both are inside the guard now. The second: run both commands from the repository root. The recipe said `cd frontend`, which contradicts this repo's own rule, so `npm --prefix frontend` instead.
There was a problem hiding this comment.
Your trial has ended. Reactivate Greptile to resume code reviews.
|
Two of the three fixed in 591bcc0; declining the third with a reason. Preserve the original body failure when cleanup transport fails (Major) — fixed, and a good catch. It is a hole in the reasoning of the commit it reviews: I guarded the cleanup assertion behind Run from the repository root (Minor) — fixed.
Happy to switch if either of the first two stops being true. @coderabbitai review |
🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
git show --stat --oneline 591bcc0
git diff 591bcc0^ 591bcc0 -- frontend/README.md frontend/test/e2e/mcp-agent.spec.ts
printf '\n--- Candidate files ---\n'
fd -i 'mcp.spec.ts' .
fd -i 'playwright*.{ts,js,mjs,cjs}' . || true
fd -i 'host.sh' .
printf '\n--- mcp-agent test outline ---\n'
ast-grep outline frontend/test/e2e/mcp-agent.spec.ts --items all || true
printf '\n--- Relevant implementation and configuration references ---\n'
rg -n -C 4 --glob '!*node_modules*' \
'(workers\s*:|fullyParallel\s*:|pw-mcp-\$\{Date\.now\(\)\}|target/e2e|data root|dataRoot|bind|page\.request\.delete|bodyPassed)' \
frontend .github scripts . 2>/dev/null || trueLength of output: 191593 🏁 Script executed: #!/bin/bash
set -euo pipefail
printf '%s\n' '--- MCP agent test ---'
sed -n '80,185p' frontend/test/e2e/mcp-agent.spec.ts
printf '%s\n' '--- Playwright host selection ---'
sed -n '1,170p' frontend/playwright.config.ts
printf '%s\n' '--- Adjacent MCP test naming and cleanup ---'
sed -n '55,125p' frontend/test/e2e/mcp.spec.ts
printf '%s\n' '--- External-host documentation and references ---'
rg -n -C 3 --glob 'frontend/**/*.{ts,md,sh}' \
'(PW_BASE_URL|managesHost|reuseExistingServer|PW_HOST_DATA_DIR)' frontendLength of output: 28043
I accept the
Use 🐇🔍
|
|
CI note, and I am going to stop re-running it. The four other jobs are green on this commit;
An empty For comparison, the same lane on the same spec content ran fine twice earlier today: run 31113374341 (8m15s, 90 passed / 6 skipped, This lane is last in the dependency chain ( |
Checkpoint of work in progress, touching frontend/test/e2e/mcp-agent.spec.ts. Auto-committed-on: dragonfly Co-authored-by: Medulla <medulla@tinyhumans.ai>
Checkpoint of work in progress, touching frontend/test/e2e/mcp-agent.spec.ts. Auto-committed-on: dragonfly Co-authored-by: Medulla <medulla@tinyhumans.ai>
Checkpoint of work in progress, touching frontend/test/e2e/mcp-agent.spec.ts. Auto-committed-on: dragonfly Co-authored-by: Medulla <medulla@tinyhumans.ai>
2 participants
Summary
Follow-up to #484 (issue #467), which merged at 83bd8ba while these two fixes
were still in flight. Both come from CodeRabbit's review of that PR, and both
are real; neither reached
main.The MCP registration in
mcp-agent.spec.tswas not test-owned (CodeRabbit,Major). The server name was fixed and a
409 already existswas accepted assuccessful setup, so a leftover registration from an earlier run — pointing at
any endpoint at all — would be adopted, passed against, and then deleted by a
test that never created it. The spec would go green having validated nothing
about
MCP_SERVER. The name is unique per run now and a409is a setupfailure.
Cleanup is checked as well, but only when the body got that far: an
assertion thrown out of a
finallyreplaces the real failure with a complaintabout cleanup, which is the harder of the two to debug. That is a deliberate
narrowing of the suggested fix, and the reason is in the comment.
The README's live-lane recipe did not say where to stand (CodeRabbit,
Minor). It runs
cargo buildfrom the repository root andnpm run e2e:livefrom
frontend/; only CI knew that, throughworking-directory: frontend.API Or Behavior Changes
None. Test-fixture ownership and one documentation line.
Tests
cargo fmt --all -- --check— N/A - full local build matrix is prohibited on this machine; verified in CI. (No Rust in this diff.)cargo clippy --all-targets -- -D warnings— N/A - full local build matrix is prohibited on this machine; verified in CI.cargo build --all-targets— N/A - full local build matrix is prohibited on this machine; verified in CI.cargo test— N/A - full local build matrix is prohibited on this machine; verified in CI.npm run typecheck:e2eclean. The spec itself runs inConsole E2E (live brain), which this PR's own CI run exercises — that lane is where the changeis observable, and it is green on
mainas of #484.No new test accompanies this: the change is a test's correctness, and the
property it fixes (a stale registration cannot be adopted) is asserted by the
spec's own
expect(added.ok())on every run.Documentation
frontend/README.md— the live-lane recipe now names the working directory.Summary by CodeRabbit
Documentation
Tests