Skip to content

fix(mcp): ExecuteScript decides "this target cannot run a script" itself (#2918) - #2979

Merged
github-actions[bot] merged 1 commit into
mainfrom
fix/2918-executescript-fail-fast
Sep 1, 2026
Merged

github-actions[bot] merged 1 commit into
mainfrom
fix/2918-executescript-fail-fast

Conversation

@rbuergi

@rbuergi rbuergi commented Sep 1, 2026

Copy link
Copy Markdown
Contributor

The measurement first

ExecuteScript posts an ExecuteScriptRequest at the target path and waits for the owning hub to
answer. I drove the two failing shapes against a real monolith mesh before changing anything:

target answer on main elapsed
absent path DeliveryFailureException: No node found at '…'. Closest ancestor is '…' 0 ms
existing non-Code node DeliveryFailureException: No handler found for message type ExecuteScriptRequest 70 ms

So both routers do NACK, and in-process the NACK is immediate. The 60 s the incident reports is
not the tool's own budget (timeoutSeconds, default 120) — it is the hub's RequestTimeout,
which fires first and produces the TimeoutException in the log. The incident is therefore a NACK
that did not come back, not a tool that forgot to time out.

What this PR fixes

A reply that never returns is a transport defect and is tracked as one. What is fixed here is that
the tool needed the reply at all for two conditions it can establish itself. It now reads the
target before dispatching — the same bounded FetchNode read every other path-taking tool on this
surface uses (Get / Patch / Update / Delete), bounded again by the caller's own timeoutSeconds so
the pre-flight can never cost more than the dispatch it replaces — and refuses locally:

errorType condition
NodeNotFound no readable node at that path (a denied read reports the same, deliberately — saying "denied" would disclose that a gated node exists there)
NotExecutable the node carries no CodeConfiguration, or one with IsExecutable = false

Both are the verdicts CodeNodeType.HandleExecuteScript already reaches on the owning hub, so the
answer does not change — only who says it, and how fast. errorType is a stable token instead of
an exception class name, because there is no exception and inventing one would be a lie.

🚨 The refusal is one-sided, on purpose

NodeReadOutcome.IsUnavailable — a read that reached no verdict — never refuses. The dispatch
proceeds exactly as before and the owning hub gets the last word. A check that fired on "I could not
find out" would turn a momentary blip into a hard error on a script that is perfectly fine, which is
the more expensive mistake by far. That asymmetry is what makes the pre-flight safe to add: it can
turn a guaranteed failure into a fast one, never a would-be success into an error.

The content is read with ContentAs<CodeConfiguration>, not a CLR cast: the node has crossed a hub
boundary and its content can legitimately arrive as a raw JsonElement, which a cast would read as
"nothing to execute" and refuse a node that runs fine.

Verification

ExecuteScriptPreflightTest (2 facts) drives the tool exactly as MCP does against a real
MonolithMeshTestBase mesh and asserts both the elapsed wall clock (well under the dispatch
budget — the non-regression half) and the actionable errorType (the discriminator).

  • RED on main: both fail — Expected "NodeNotFound" … but found "MeshWeaver.Messaging.DeliveryFailureException" / Expected "NotExecutable" … but found "MeshWeaver.Messaging.DeliveryFailureException".
  • GREEN with the fix: Passed! - Failed: 0, Passed: 2 (861 ms for the pair, mesh construction included).

Builds, one project per invocation, -c Release -warnaserror, all 0 Warning(s) / 0 Error(s):
MeshWeaver.Mesh.Operations, Memex.Portal.Shared.Test, MeshWeaver.Documentation.Test,
tools/MeshWeaver.PluginTester (the other dependent). WhatsNewEntryIntegrityTest +
DocumentationLinkIntegrityTest green.

Work product conserved

Doc/AI/ExecuteScript had drifted from the code and is corrected in the same change set: the reply
is the dispatch acknowledgement (Dispatched + activityPath), never the run's result; the
errorType table is documented with the one-sided rule; and timeoutSeconds above 60 s is recorded
as inert, because the hub's own request timeout fires first — the finding that explains why the
incident says 60 s and not 120 s.

Closes #2918

🤖 Generated with Claude Code

…elf, instead of waiting for a reply that may never come (#2918)

`ExecuteScript` posts an `ExecuteScriptRequest` at the target path and waits for
the owning hub to answer. Both routers do NACK a target that cannot answer — an
absent path gets `ErrorType.NotFound`, a hub with no `ExecuteScriptRequest`
handler answers "No handler found for message type ExecuteScriptRequest" — and
in-process both arrive in milliseconds (measured: 0 ms / 70 ms in a monolith
mesh). Production 2026-08-31 (fingerprint `6094e6b21967f47b`) shows what happens
when one does NOT come back: the caller sat on the hub's 60 s `RequestTimeout`
and reported a `TimeoutException` whose own text could not say which of
"undeliverable" and "delivered but unanswered" had occurred.

A reply that never returns is a transport defect and is tracked as one. What is
fixed here is that the tool needed the reply at all for two conditions it can
establish itself. It now reads the target first — the same bounded `FetchNode`
read every other path-taking tool on this surface uses, bounded again by the
caller's own `timeoutSeconds` — and refuses locally:

  * `NodeNotFound`  — no readable node at that path
  * `NotExecutable` — no `CodeConfiguration`, or `IsExecutable = false`

Both are the verdicts `CodeNodeType.HandleExecuteScript` reaches on the owning
hub, so the answer does not change; only who says it, and how fast. `errorType`
is a stable token rather than an exception class name, because there is no
exception and inventing one would be a lie.

🚨 The refusal is ONE-SIDED. `NodeReadOutcome.IsUnavailable` — a read that
reached no verdict — never refuses: the dispatch proceeds exactly as before and
the owning hub gets the last word. A check that fired on "I could not find out"
would turn a momentary blip into a hard error on a script that is fine, which is
the more expensive mistake. That asymmetry is what makes the pre-flight safe: it
can turn a guaranteed failure into a fast one, never a would-be success into an
error. The content is read with `ContentAs<CodeConfiguration>`, not a cast — the
node has crossed a hub boundary and can legitimately arrive as a `JsonElement`.

Also corrects `Doc/AI/ExecuteScript`, whose documented response shape had drifted
away from the code: the reply is the DISPATCH acknowledgement (`Dispatched` +
`activityPath`), never the run's result, and `timeoutSeconds` above 60 s is inert
because the hub's own request timeout fires first.

Closes #2918

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Copilot AI lite review requested due to automatic review settings September 1, 2026 14:31
@github-actions
github-actions Bot enabled auto-merge September 1, 2026 14:31
@github-actions

github-actions Bot commented Sep 1, 2026

Copy link
Copy Markdown
Contributor

Test Results (shard 3)

578 tests   578 ✅  52s ⏱️
  2 suites    0 💤
  2 files      0 ❌

Results for commit 0967f18.

@github-actions

github-actions Bot commented Sep 1, 2026

Copy link
Copy Markdown
Contributor

Test Results (shard 0)

216 tests   216 ✅  2m 21s ⏱️
  1 suites    0 💤
  1 files      0 ❌

Results for commit 0967f18.

@github-actions

github-actions Bot commented Sep 1, 2026

Copy link
Copy Markdown
Contributor

Test Results (shard 5)

1 128 tests   936 ✅  1m 4s ⏱️
    3 suites  192 💤
    3 files      0 ❌

Results for commit 0967f18.

@github-actions

github-actions Bot commented Sep 1, 2026

Copy link
Copy Markdown
Contributor

Test Results (shard 2)

507 tests   507 ✅  1m 31s ⏱️
  3 suites    0 💤
  3 files      0 ❌

Results for commit 0967f18.

@github-actions

github-actions Bot commented Sep 1, 2026

Copy link
Copy Markdown
Contributor

Test Results (shard 1)

419 tests   419 ✅  1m 27s ⏱️
  1 suites    0 💤
  1 files      0 ❌

Results for commit 0967f18.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR addresses #2918 by making the ExecuteScript MeshOperations/MCP tool fail fast for targets that are definitely unrunnable (absent path or non-executable node) via a bounded pre-flight node read, avoiding long request/response timeouts when no owning hub reply will arrive.

Changes:

  • Added a pre-flight FetchNode read in MeshOperations.ExecuteScript to locally reject NodeNotFound / NotExecutable cases before dispatch.
  • Added ExecuteScriptPreflightTest to pin both the returned discriminator (errorType) and the elapsed time (non-regression vs. “wait out the dispatch budget”).
  • Updated documentation (AI tool docs + What’s New entry) to describe the dispatch-ack contract and the new fast-fail behavior.

Reviewed changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.

File Description
test/Memex.Portal.Shared.Test/ExecuteScriptPreflightTest.cs Adds end-to-end preflight tests asserting fast, actionable errors instead of budget-length waits.
src/MeshWeaver.Mesh.Operations/MeshOperations.cs Implements pre-flight target validation and refactors dispatch logic into a helper.
src/MeshWeaver.Documentation/Data/WhatsNew/2026-09-01-running-a-script-on-the-wrong-node-says-so-immediately.md Adds a user-facing release note for the new fail-fast behavior.
src/MeshWeaver.Documentation/Data/AI/ExecuteScript.md Updates the tool contract docs (response envelope, errorType, timeout notes).
Suppressed comments (1)

src/MeshWeaver.Documentation/Data/AI/ExecuteScript.md:143

  • The errorType table says an exception type name covers cases where the owning hub refused the dispatch, but the code path for a hub refusal does not throw an exception. If you add a stable DispatchRefused token (or similar) for that branch, update this table so it matches the actual envelopes emitted.
| `errorType` | Meaning | What to do |
|---|---|---|
| `NodeNotFound` | There is no readable node at that path. | Check the path. A denied read reports the same way on purpose — saying "denied" would disclose that a gated node exists there. |
| `NotExecutable` | The node exists but carries no `CodeConfiguration`, or carries one with `isExecutable: false`. | Point at a `Code` node, or set `isExecutable: true` on it. |
| *(an exception type name)* | The dispatch itself failed — undeliverable, refused by the owning hub, or the budget elapsed. | `message` and `detail` carry the type and stack; the owning hub has logged the full cause. |

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Comment thread src/MeshWeaver.Mesh.Operations/MeshOperations.cs
Comment thread src/MeshWeaver.Documentation/Data/AI/ExecuteScript.md
@github-actions

github-actions Bot commented Sep 1, 2026

Copy link
Copy Markdown
Contributor

Test Results (shard 4)

1 640 tests   1 640 ✅  1m 44s ⏱️
    3 suites      0 💤
    3 files        0 ❌

Results for commit 0967f18.

@github-actions

github-actions Bot commented Sep 1, 2026

Copy link
Copy Markdown
Contributor

Test Results

4 488 tests   4 296 ✅  9m 1s ⏱️
   13 suites    192 💤
   13 files        0 ❌

Results for commit 0967f18.

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.

ExecuteScript hangs for the full 60s and logs a dispatch failure when the target node's hub is not found, instead of failing fast

2 participants