Skip to content

Conversation

@ghostwriternr
Copy link
Member

The hibernated OAuth callback test was timing out because mock objects lacked url property and establishConnection method. This caused background connection attempts to make real HTTP requests.

Changes:

  • Add missing properties to test mocks
  • Extract shared helper for mock creation

Tests timed out because mocks lacked url property and
establishConnection method, causing real HTTP requests.
@changeset-bot
Copy link

changeset-bot bot commented Nov 5, 2025

⚠️ No Changeset found

Latest commit: 12e2302

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

@claude
Copy link

claude bot commented Nov 5, 2025

Claude Code Review

Summary: Good fix for the flaky test. The refactoring improves code reusability and the mock is more complete.

Issues Found:

  1. Potential state inconsistency in completeAuthorization mock (packages/agents/src/tests/worker.ts:224-226)

    The mock completeAuthorization updates the connection state, but it's accessing this.mcp.mcpConnections[serverId] which may not be the same reference as the mock object being created. This works when the mock is already assigned to this.mcp.mcpConnections[serverId], but creates a temporal coupling.

    Consider using this.connectionState instead:

    completeAuthorization: async (_code: string) => {
      this.connectionState = "ready";
    },

    Same issue at line 228 for establishConnection.

  2. Redundant state update in setupMockOAuthState (packages/agents/src/tests/worker.ts:268-274)

    When createConnection is false and a connection already exists, you're manually setting completeAuthorization and establishConnection on the existing connection. However, createMockMcpConnection already provides these methods. This code path seems to handle a case where an old mock without these methods exists, but it's unclear if this is still needed.

    Consider documenting why this fallback is needed or removing it if all mocks now use createMockMcpConnection.

Minor observations:

  • The helper extraction is good - makes the mock creation consistent across test cases
  • All required properties are now present (url, establishConnection, resourceTemplates, serverCapabilities, lastConnectedTransport)

The core fix addresses the timeout issue effectively.

@pkg-pr-new
Copy link

pkg-pr-new bot commented Nov 5, 2025

Open in StackBlitz

npm i https://pkg.pr.new/cloudflare/agents@634

commit: 12e2302

@ghostwriternr ghostwriternr merged commit 2b6cba4 into main Nov 5, 2025
5 of 6 checks passed
@ghostwriternr ghostwriternr deleted the fix/oauth-test-flakiness branch November 5, 2025 12:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants