Repository navigation
fix(mcp): every ExecuteScript Error carries an errorType — and "Error" is not universally side-effect free (#2918 review) - #2983
Conversation
…"Error" does not universally mean "nothing happened" (#2918 review) Two review findings on #2979, both correct. 1. `errorType` was not actually a discriminator. #2979 introduced it as the field a caller branches on, and documented it that way — but only the pre-flight verdicts and the exception path carried it. The hub-refusal branch (`ExecuteScriptResponse.Success = false`) and the no-activity-path branch emitted `status: "Error"` with no `errorType` at all, so a caller reading that key got `undefined` on two of the four refusals. Both now carry a stable token — `DispatchRefused` and `NoActivityPath` — for the same reason the pre-flight does: the hub answered, there is no exception to name, and inventing one would be a lie. Both also prefer the hub's own `SubmissionId` when it sent one, matching the success branch. 2. The Status table claimed every `Error` meant nothing was executed. That is true of the two pre-flight verdicts and of a hub refusal (the hub creates no ActivityLog on any refusal path) — and FALSE of the other two. `NoActivityPath` means the hub ACCEPTED the submission and the run may well be under way; an undeliverable/timed-out dispatch means "no acknowledgement", which is not "not delivered". A caller that treats all four alike can run a script twice. The table now carries a side-effects column and the retry advice per condition, and `NoActivityPath`'s own message says so on the wire, not only in the docs. No new test: with the pre-flight in place, the two branches this touches are no longer reachable deterministically from the tool surface — the pre-flight now answers the conditions the owning hub used to refuse. I probed for one (an executable Code node whose ActivityParentPath names a partition that does not exist) and it did NOT refuse: the in-memory store created the ActivityLog under that namespace and the dispatch reported `Dispatched`. That test is therefore not in this commit rather than being weakened until it passed. The doc half IS covered: DocumentationLinkIntegrityTest + WhatsNewEntryIntegrityTest green, and ExecuteScriptPreflightTest's two facts still pass unchanged. Refs #2918 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
This PR tightens the ExecuteScript MCP tool’s error contract so callers can reliably branch on errorType, and updates the documentation to clarify which Error conditions may already have side effects (i.e., where a blind retry risks executing twice).
Changes:
- Ensure previously “missing-
errorType” error branches (DispatchRefused,NoActivityPath) always include a stableerrorTypetoken and prefer the hub-providedSubmissionIdwhen present. - Strengthen the
NoActivityPathwiremessageto warn callers that the script may already be running. - Update
Doc/AI/ExecuteScriptto include a side-effects + retry-safety table for eacherrorType.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| src/MeshWeaver.Mesh.Operations/MeshOperations.cs | Adds stable errorType tokens and consistent submissionId behavior for remaining Error response branches. |
| src/MeshWeaver.Documentation/Data/AI/ExecuteScript.md | Clarifies Error semantics, documents errorType exhaustiveness, and adds retry/side-effect guidance per error type. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Test Results (shard 3)578 tests 578 ✅ 45s ⏱️ Results for commit 38f228d. |
Test Results (shard 1)419 tests 419 ✅ 1m 24s ⏱️ Results for commit 38f228d. |
Test Results (shard 2)507 tests 507 ✅ 1m 32s ⏱️ Results for commit 38f228d. |
Test Results (shard 5)1 128 tests 936 ✅ 1m 4s ⏱️ Results for commit 38f228d. |
Test Results (shard 4)1 642 tests 1 642 ✅ 1m 41s ⏱️ Results for commit 38f228d. |
Test Results (shard 0)216 tests 216 ✅ 2m 38s ⏱️ Results for commit 38f228d. |
Test Results4 490 tests 4 298 ✅ 9m 7s ⏱️ Results for commit 38f228d. |
Follow-up to #2979 (merged). Two Copilot review findings, both correct.
1.
errorTypewas not actually a discriminator#2979 introduced
errorTypeas the field a caller branches on, and documented it that way — butonly the two pre-flight verdicts and the exception path carried it. The hub-refusal branch
(
ExecuteScriptResponse.Success = false) and the no-activity-path branch emittedstatus: "Error"with noerrorTypeat all, so a caller reading that key gotundefinedon two ofthe four refusals. A field present on some refusals and absent on others is not something you can
branch on.
Both now carry a stable token —
DispatchRefusedandNoActivityPath— for the same reason thepre-flight does: the hub answered, so there is no exception to name, and inventing one would be a
lie. Both also prefer the hub's own
SubmissionIdwhen it sent one, matching the success branch.2. "Error" did not universally mean "nothing happened"
The Status table said it did. That is true of the two pre-flight verdicts and of a hub refusal (the
hub creates no
ActivityLogon any refusal path) — and false of the other two:errorTypeNodeNotFound/NotExecutableDispatchRefusedNoActivityPathAn agent that treats all four alike can run a script twice. The table now carries a side-effects
column and per-condition retry advice, and
NoActivityPath's own wire message says so, not onlythe docs.
Why there is no new test
With the pre-flight in place, the two branches this touches are no longer reachable deterministically
from the tool surface — the pre-flight now answers the conditions the owning hub used to refuse.
I probed for one anyway: an executable Code node whose
ActivityParentPathnames a partition thatdoes not exist, expecting
CreateNodeof theActivityLogto fail intoFaultDispatch. It didnot refuse — the in-memory store created the activity under that namespace and the tool answered
Dispatchedwith anactivityPathin a partition that had never existed. So that test is absentfrom this PR rather than weakened until it passed. (Whether an activity should be creatable under an
arbitrary non-existent namespace is a separate question I did not chase; noted here so it is not
lost.)
The doc half is covered, and it is the half that changes what a caller does.
Verification
dotnet build … -c Release -warnaserror, one project per invocation —MeshWeaver.Mesh.Operations,MeshWeaver.Documentation.Test,Memex.Portal.Shared.Test— all0 Warning(s) / 0 Error(s).DocumentationLinkIntegrityTest+WhatsNewEntryIntegrityTestgreen (2/2).ExecuteScriptPreflightTeststill green unchanged (2/2).Refs #2918
🤖 Generated with Claude Code