[poshinchen]

Replica of https://github.com/strands-agents/harness-sdk/pull/5043 (backticked: no cross-reference). Below: the v4-reason optimized review and a sanitized copy of live Strandly's actual comments.

[poshinchen]

Strandly v4-reason (optimized) review — run af1746, Opus 4.8

Assessment: comment

Assessment: Comment

Both fixes are well-targeted and thoroughly tested, with the plain-path connect signal forwarding being a clean, correct change. My main concern is the latching semantics of the new metadata-fetch guard.

Review Categories

• Correctness: The _serverToolsListed flag is latched before listTools resolves, so an aborted/timed-out fetch permanently poisons the once-per-client guard — a later retry with a fresh signal silently falls back to a plain tools/call and the exact taskSupport: "required" failure this PR targets can recur. Genuine failures intentionally latching is fine; abort/timeout is the edge worth revisiting.
• Maintainability: _ensureServerToolDefinitions(operation!) depends on an implicit completeLegacyTask ⇒ operation present invariant; worth documenting or tightening.
• Testing: Regression coverage is strong across all four new scenarios and matches the described behavior.

Nice work isolating the once-per-client fetch so servers without legacy tasks pay no round-trip.

[poshinchen]

Important
Issue: _serverToolsListed is set to true before listTools resolves. On an aborted/timed-out fetch, the catch re-throws (good), but the flag stays true and _serverToolDefinitions was never populated. A subsequent callTool with a fresh signal then skips the fetch (if (this._serverToolsListed) return), sends a plain tools/call with no task: {}, and a taskSupport: "required" tool fails with the confusing server error this PR set out to prevent. A genuine listing failure logging a warning and falling back is intentional per the description, but an abort/timeout permanently poisons the once-per-client flag so retries can never recover.
Suggestion: Only latch the flag on outcomes you intend to be permanent — e.g. set _serverToolsListed = true after a successful fetch and inside the non-abort branch of catch, leaving it false when the fetch is aborted/times out so a retry can re-attempt.

[poshinchen]

Suggestion
Issue: _ensureServerToolDefinitions(operation!) relies on the non-null assertion holding because the gating condition includes completeLegacyTask, which today is only true on the task path where operation is always supplied. This invariant is implicit and could silently break if a future caller sets completeLegacyTask without an operation.
Suggestion: Consider making operation a required parameter when completeLegacyTask is set, or add a brief comment at this call site documenting why operation is guaranteed present here, so the ! is self-explanatory.

[poshinchen]

Live Strandly's actual comments on the original PR 5043 (sanitized copy: mentions escaped, links backticked)

Review (COMMENTED):

Both fixes do what they claim, and the tests earn their keep — I re-ran each new test against main and both fail there (the abort test times out; the legacy test rejects with Invalid result for tools/call), so each one pins its fix. One 🟡 inline: the metadata fetch isn't once-per-client when the listing is empty or fails, and it runs even on legacy servers that never advertised tasks.

Separately, with measurements: the abort and deadline cases behind the two open bot findings are already handled by the operation signal — no plain tools/call is ever resurrected. Details below; you may be able to answer both with a comment instead of code.

✅ What I verified

• Head e72a3613, base main afc3e059b (one unrelated commit ahead of the PR's parent; nothing under src/mcp).
• npx vitest run --project unit-node src/mcp src/tools → 337 passed, no type errors.
• Both new tests fail without the change (client.ts reverted to main): abort test → Test timed out in 5000ms (×3 retries); legacy test → promise rejected "SdkError: Invalid result for tools/call…".
• Probes + logs are attached as review artifacts: repro-metadata-refetch.probe.ts / .log, repro-connect-wedge.test.ts / .log, plus the suite and both base-branch failure logs.

On the two open bot findings (swallowed abort / full requestTimeoutMs) — evidence they're cosmetic

The fetch only runs on the legacy complete path, where options.signal === operation.signal (client.ts:707). So after the catch swallows an abort, the aborted signal still reaches this._client.callTool at client.ts:882 and the SDK rejects before anything goes on the wire:

P5 REJECTED: Error: caller aborted mid-listing                  # caller aborts mid-listing
P5 tools/call on the wire = 0
P6 REJECTED: SdkError: MCP task did not complete within 80ms    # pollTimeout 80ms, tools/list hangs
P6 tools/call on the wire = 0

Both the caller's own reason and the deadline surface correctly, and P6 shows the operation signal already bounds the listing by the operation deadline (it fired at 80 ms with requestTimeout: 500), which is what the remainingTime(...) suggestion would buy. The only real damage is the misleading failed to list tools for legacy task support detection warning on a user cancel — an if (!signal?.aborted) around the logger.warn covers that if you want it.

Questions (non-blocking)

• Is skipping the tasks capability check at client.ts:854 deliberate — e.g. to populate outputSchema for a legacy server regardless? If so, the comment at :852 is the place to say it, because _supportsLegacyTask can never be true without the capability.
• On an empty or failing tools/list, do you want the client to keep retrying per call (current behaviour) or to give up for the connection's lifetime? The inline suggestion assumes the latter.

Review (CHANGES_REQUESTED):

Changes requested — one 🔴. Both fixes do what they claim and the new tests are real regression tests (both fail on main). But forwarding the signal into connect() on the default path exposes a latent wedge: a user cancel during a stalled handshake, followed by a retry, leaves the client permanently failing with StreamableHTTPClientTransport already started! (repro'd against a real Streamable HTTP server; connect(true) doesn't recover). The fix is ~5 lines in connect() — inline. One 🟡 on gating the tools/list fetch on the server's tasks capability.

✅ Verified · Questions · Reading order · Appendix

Verified at e72a361

• npx vitest run --project unit-node src/mcp/__tests__/client.test.ts src/mcp/__tests__/task-client.test.ts → 142 passed, no type errors.
• Both new tests copied onto origin/main fail (stops waiting for the connection… times out; fetches task metadata… fails) — they cover the fixes.
• Abort during the tools/list fetch: caller gets their own abort reason and no tools/call goes on the wire (the SDK rejects an already-aborted signal before sending). So the catch already flagged by github-actions is a misleading warn-log for aborts; the real gap is a requestTimeoutMs timeout, after which the plain call does proceed.
• Connect wedge repro: HTTP server with initialize delayed 400 ms; callTool with a signal, abort at 100 ms, call again → 2nd and 3rd calls reject with already started, state stays disconnected. Same on main via tasksConfig; new on the default path with this PR. With the suggested connect() change the 2nd call waits for the in-flight attempt and succeeds, and the 142 tests stay green. Logs and repro files are in the review artifacts.

Questions (non-blocking)

• Without tasksConfig, callTool(tool, args, { timeoutMs }) with no signal still waits forever on a hung connect (the task path bounds connect by the operation deadline). Pre-existing — intentional, or worth bounding while you're here?

Reading order

1. client.ts:846-847 (signal forwarding) together with connect() at client.ts:478-504 — the 🔴.
2. client.ts:852-856 + client.ts:1117-1128 — the metadata fetch.
3. The two tests.

Appendix — non-blocking (5)

• ⚪ Concurrent first calls on an empty cache each issue tools/list (3 for 3 concurrent calls). Memoizing the in-flight promise would fix it; only matters for parallel tool use on a fresh client.
• ⚪ A server that returns tools: [], or whose tools/list hangs, keeps the cache empty, so the fetch (and its requestTimeoutMs latency) repeats on every call. Contrived.
• ⚪ Populating the SDK's tool cache via listTools turns on SDK output-schema validation for later callTools: a tool with outputSchema whose server omits structuredContent now rejects on this path (did not return structured content). Same as after a normal listTools(), so arguably correct — just a behaviour change for hand-constructed too

Review (CHANGES_REQUESTED):

Still changes requested — the 🔴 connect wedge is unaddressed, and the new _serverToolsListed flag trades the refetch bug for a worse one.

Status of previous findings at 7ef2966:

• 🔴 abort-during-connect wedge (client.ts:850 thread) — still open; connect() is unchanged. Repro and 5-line fix are in that thread.
• 🟡 capability gate — fixed, verified by skips the metadata fetch when the server does not advertise legacy tasks.
• 🟡 once-per-client listing — fixed, but introduced a regression: the flag is set before the listing settles, so an abort during the first fetch permanently disables task detection on that client, and a concurrent second first-call (the Agent's default executor) goes plain. Both repro'd; details and fix in reply to mkmeral's question at client.ts:1126.
• The three github-actions items — addressed as described.

✅ Verified · Reading order

• npx vitest run --project unit-node src/mcp/__tests__/client.test.ts src/mcp/__tests__/task-client.test.ts at 7ef2966 → 145 passed, no type errors.
• git diff e72a361 7ef2966 -- strands-ts/src/mcp/client.ts touches no line of connect(); the 🔴 repro conditions are identical.
• Repro ZZ2 (abort during hung tools/list, then a second call): tools/list count stays 1, second call's task param is null.
• Repro ZZ1 (Promise.all of two first calls, 50 ms tools/list): task params [null, {}].
• Moving this._serverToolsListed = true after the try/catch: both repros emit task: {}, 186 tests pass. Logs + repro file uploaded as review artifacts.

Reading order: client.ts:1124-1138 (the flag), then the three new tests at task-client.test.ts:287-330.

Inline at strands-ts/src/mcp/client.ts:1118:

🟡 tools/list is re-fetched on every call when the listing is empty or fails — and it runs even when the server never advertised tasks.

size > 0 stands in for "already listed", so a server that answers { tools: [] } or errors on tools/list never populates the cache and every callTool pays another listing (plus another warning). And the gate at client.ts:854 checks only the protocol version, so a legacy client also pays one needless tools/list against a server with no tasks capability, where _supportsLegacyTask can never be true. That doesn't match the description's "fetches tools/list once" / "servers without tools/list are unaffected".

Measured on e72a3613 (probe + log in the review artifacts):

P1 tools/list count after 3 calls = 3    # server returns { tools: [] }
P2 tools/list count after 2 calls = 2    # tools/list errors
P3 tools/list count (legacy server, no tasks capability) = 1

reach: new McpClient({ url, tasksConfig }) + a hand-constructed tool on a 2025-11-25 server — the scenario this PR adds.

Suggestion: track "listed" separately from "non-empty" (new private _serverToolsListed = false, also set in listTools()), and add the capability check to the :854 gate. That also pins the guard the bot asked for a test on.

    if (this._serverToolsListed) return

Inline at strands-ts/src/mcp/client.ts:850:

🔴 Forwarding the signal here exposes a wedge: a per-call abort during a stalled handshake clears _connectionPromise while _connect() is still running, and the next call starts a second connect on the same transport → StreamableHTTPClientTransport already started!, which connect(true) can't recover from (strands-agents#4095).

connect() clears the slot in a finally on the raced await (client.ts:496-503), so it fires when the caller aborts, not when the attempt settles. Until now only tasksConfig clients forwarded a signal into connect(); with this line every Agent MCP tool call does (mcp-tool.ts:62-63 passes toolContext.cancelSignal), so cancel-during-connect followed by a retry — the scenario this fix targets — wedges the client. Repro against a real Streamable HTTP server (initialize delayed 400 ms, abort at 100 ms, call again): 2nd and 3rd calls both reject with already started, state stays disconnected. Tracked in strands-agents#5045.

Suggestion (verified: repro passes, 142 mcp tests stay green) — tie the cleanup to the connection's own settlement, in connect():

const connectionPromise = this._connect(reconnect)
this._connectionPromise = connectionPromise
void connectionPromise.catch(() => undefined).finally(() => {
  if (this._connectionPromise === connectionPromise) this._connectionPromise = undefined
})
await (signal ? raceWithAbort(connectionPromise, signal) : connectionPromise)
this._assertConnectionCurrent(generation)

Plus a test where the creating caller aborts and a second caller then calls, asserting sdkClientMock.connect ran once — client.test.ts:257 only covers a waiting caller, which takes the other branch.

Inline at strands-ts/src/mcp/client.ts:854:

🟡 Gated on protocol version only, so a tasksConfig client calling an unlisted tool on an ordinary 2025-11-25 server (no tasks capability — most servers today) pays a tools/list round-trip that can't change the outcome: _supportsLegacyTask (client.ts:900-902) returns false regardless.

If that server also has no tools/list handler, the failed fetch and warn log repeat on every call because the cache stays empty (probe: 3 fetches for 3 calls), which contradicts "servers without tools/list are unaffected". The fetch also has a side effect — the SDK caches output validators from listTools, so a tool declaring outputSchema whose server omits structuredContent now rejects on this path.

Suggestion: reuse the capability check _supportsLegacyTask already makes.

    if (
      completeLegacyTask &&
      this._client.getNegotiatedProtocolVersion() === '2025-11-25' &&
      this._client.getServerCapabilities()?.tasks?.requests?.tools?.call !== undefined
    ) {

Inline at strands-ts/src/mcp/client.ts:1126:

🟡 Setting the flag before the listing settles re-introduces the bug this PR fixes, in two reachable ways (repro'd at 7ef2966, files in the review artifacts):

1. Abort during the first fetch — the catch rethrows but the flag stays true, so every later call on that client skips the fetch and sends a plain tools/call for a taskSupport: "required" tool. One user cancel during the first call permanently disables task detection. Repro: tools/list hangs → abort → unhang → second callTool → tools/list count 1, task param null.
2. Concurrent first calls — Agent runs same-turn tool calls in parallel by default (agent.ts:143). The second call sees the flag already set, skips the fetch, and goes plain while the first is still listing. Repro with a 50 ms tools/list: Promise.all([callTool, callTool]) → task params [null, {}].

Suggestion: set the flag only once the listing has settled (success or non-abort failure). Concurrent first callers then each fetch once, which is wasteful but correct — memoizing the in-flight promise would also dedupe that, if you care.

  private async _ensureServerToolDefinitions(operation: TaskOperation): Promise<void> {
    if (this._serverToolsListed) return
    try {
      …
    } catch (error) {
      if (operation.signal.aborted) throw error
      logger.warn(…)
    }
    this._serverToolsListed = true
  }

Verified: with that change both repros produce task-augmented calls and the full client.test.ts + task-client.test.ts suites pass (186 incl. my repros; the new lists tools once even when the listing is empty test still holds).