From a61b6008b4f28d03000a41fb2e9cfd04ad0e0038 Mon Sep 17 00:00:00 2001 From: Jon Langevin Date: Mon, 19 Oct 2026 19:39:04 -0400 Subject: [PATCH 1/5] chore: lock reviewed shared MCP compatibility scope --- docs/plans/2026-10-19-shared-mcp-alignment.md | 31 +++++ docs/plans/2026-10-19-shared-mcp-design.md | 123 +++++++++++++++++ docs/plans/2026-10-19-shared-mcp-review.md | 109 +++++++++++++++ docs/plans/2026-10-19-shared-mcp.md | 125 ++++++++++++++++++ .../plans/2026-10-19-shared-mcp.md.scope-lock | 1 + 5 files changed, 389 insertions(+) create mode 100644 docs/plans/2026-10-19-shared-mcp-alignment.md create mode 100644 docs/plans/2026-10-19-shared-mcp-design.md create mode 100644 docs/plans/2026-10-19-shared-mcp-review.md create mode 100644 docs/plans/2026-10-19-shared-mcp.md create mode 100644 docs/plans/2026-10-19-shared-mcp.md.scope-lock diff --git a/docs/plans/2026-10-19-shared-mcp-alignment.md b/docs/plans/2026-10-19-shared-mcp-alignment.md new file mode 100644 index 00000000..dee604cb --- /dev/null +++ b/docs/plans/2026-10-19-shared-mcp-alignment.md @@ -0,0 +1,31 @@ +# Shared MCP design-to-plan alignment + +Status: PASS, 2026-10-19. Independent adversarial review P1/P2 resolved first. + +| Design requirement | Tasks | PR | Result | +|---|---|---|---| +| R1 declared argument validation | 1, 3, 5 | 1, 2 | Covered | +| R2 array item contracts/all-or-nothing parsing | 1, 3, 5 | 1, 2 | Covered | +| R3 IsError/adapter parity, analytical negatives normal | 2, 3, 5 | 1, 2 | Covered | +| R4 truthful annotations per actual behavior | 3 | 1 | Covered | +| R5 released SDK, legacy four-version and July stdio | 4, 5, 6 | 2 | Covered | +| R6 offline schemas, safe logs, deterministic inventory, framing/EOF/cancel/private cache | 1, 2, 3, 5 | 1, 2 | Covered | +| R7 truthful inventory/reload/version documentation | 3, 6 | 1, 2 | Covered | +| Isolation, no package/installed-session/Compute/remote/auth/UI changes | all | both | Explicit exclusions | +| Independently reversible contracts then SDK migration | 3, 4, 6 | both | Covered | + +Reverse trace: Tasks 1–6 each map to the rows above; none introduces another +feature, transport, auth chain or production probe. P1 uses entered-handler +synchronization and separates built-CLI robustness from helper cancellation. +P2 forces a synthetic local full-reference file and checks exact/private/zero-TTL +wire results. Existing API success payloads are preserved with differential +cases; compile/runtime proof remains execution work. + +Manifest: exactly two rows, six headings, each task appears in its intended PR. +The installed plan-scope-check helper returned PASS with --strict. No missing, +extra, unscoped or orphaned tasks. Header/path/status normalization moves the +reviewed artifacts into the implementation checkout without changing features. + +User explicitly authorized execution and nonblocking treatment of the unrelated +benchmark harness warning. Workflow trust policy and required checks remain +unchanged. The two-PR/six-task grouping is preserved. diff --git a/docs/plans/2026-10-19-shared-mcp-design.md b/docs/plans/2026-10-19-shared-mcp-design.md new file mode 100644 index 00000000..98dac532 --- /dev/null +++ b/docs/plans/2026-10-19-shared-mcp-design.md @@ -0,0 +1,123 @@ +# Shared Workflow MCP compatibility design + +Status: local design draft for independent review; no implementation, repository writes, commits or publication performed. Execution disposition belongs to the parent under existing user authorization. +Date: 2026-10-19. +Boundary: preserve packaging PR [1083](https://github.com/GoCodeAlone/workflow/pull/1083), its scope lock, installed plugins, active sessions, CMS work, and Compute work. This is a separate shared-MCP proposal. + +## Global Design Guidance + +Sources: Workflow `AGENTS.md`, `docs/AGENT_GUIDE.md`, `docs/REPO_LAYOUT.md`; explicit delegated task. No canonical `docs/design-guidance.md` was found. These existing guides and the task supply the design constraints. + +| Guidance | Response | +|---|---| +| Preserve unrelated work; isolate release-bound changes | Future clean worktree/branches from public main; never amend PR 1083 or change its package | +| Reuse core bootstrap CLI/contracts | Keep `wfctl mcp`, existing `mcp/`, and mcp-go; no replacement server or orchestration layer | +| Update docs/tests with behavior | Shared contract, CLI-wire and documentation checks in each proposed PR | +| GOWORK=off for Go | Every future Go command uses `GOWORK=off` | +| Bounded customer value | Correct existing tools and transport interoperability; no feature expansion or Compute dependency | + +## Evidence and requirements classification + +Current inspected core matches baseline `b12aff37cfdf40d71e72c875f4c271ec23c28178`; packaging-only changes do not alter `go.mod`, `mcp/`, or `cmd/wfctl/mcp.go`. Release v0.86.2 build metadata identifies revision `92aecb70013735aac2660c60df76ac9f1a0195d2`, Go 1.26.5, mcp-go v0.54.1. Current tree uses Go 1.27.2 and the same SDK. This SDK was upgraded previously; this proposal does not claim it was never maintained. + +| ID | Kind | Verified state / required outcome | +|---|---|---| +| R1 | Protocol/correctness | Enable declared-input validation. Current `NewServer` omits the SDK opt-in; release accepts string `skip_unknown_types:"true"` and skips unknown-type checking. Reject invalid argument types as tool errors; preserve valid calls. | +| R2 | Contract accuracy | `get_config_skeleton.module_types` lacks string item schema; handler silently drops nonstrings. Release `['http.server',73]` succeeds and drops 73. Type every described string array and reject the entire malformed invocation. | +| R3 | Protocol/correctness | `InProcessServer.CallTool` strips `IsError`; engine-backed `run_workflow` returns `success:false` without `IsError`. Preserve failure semantics across wire and in-process paths. A successful validation report containing `valid:false` remains a normal successful invocation. | +| R4 | OpenAI directory policy + honest metadata | Explicit annotation booleans already exist via SDK defaults. Fix their meanings per handler; do not describe this as adding absent fields. [OpenAI policy](https://developers.openai.com/plugins/plugin-guidelines#correct-annotation) governs directory review; it is distinct from MCP's optional hints. | +| R5 | Claimed protocol-version support | Latest released spec is [2026-07-28](https://modelcontextprotocol.io/specification/2026-07-28/changelog). Existing SDK stops at 2025-11-25. Actual release modern discovery returns -32601, and modern tools/list lacks July metadata. Existing initialize clients still work; no evidence the packaging plugin is broken. | +| R6 | Security/transport evidence | Test genuine CLI framing, EOF, cancellation-notification robustness, argument rejection, no ambient secret/log leakage, offline schemas, private synthetic local-resource caching and deterministic inventories. Prove cooperative handler cancellation separately with an entered blocking fake handler and explicit barrier through the dispatcher and real SDK stdio helper. These are scoped proofs, not blanket security certification. | +| R7 | Documentation accuracy | `docs/mcp.md` lists 11 tools/3 resources and promises restart-free discovery; actual stdio inventory is 33 tools/5 resources. Startup manifest discovery needs restart; metadata inventory is not binary/runtime acceptance. | + +Primary sources: [released Workflow dependency](https://github.com/GoCodeAlone/workflow/blob/v0.86.2/go.mod), [server](https://github.com/GoCodeAlone/workflow/blob/v0.86.2/mcp/server.go), [adapter](https://github.com/GoCodeAlone/workflow/blob/v0.86.2/mcp/library.go), [tools/error contract](https://modelcontextprotocol.io/specification/2026-07-28/server/tools), [discovery contract](https://modelcontextprotocol.io/specification/2026-07-28/server/discover). + +July support requires stateless `server/discover`, protocol/capabilities per request in `_meta`, ordinary `resultType:"complete"`, version-mismatch errors, and cache fields on supported list/read results. Legacy initialization remains for older revisions. HTTP session/header changes do not justify adding HTTP to this stdio server. Optional tasks, MRTR features, authentication, UI, prompts, sampling and structured outputs are not selected. + +## Selected approach and alternatives + +Recommended: two sequential reviewable PRs. PR A fixes existing contracts on v0.54.1. PR B upgrades the existing SDK to verified released v1.3.0 and proves both protocol eras. Both preserve tool names, successful text payloads, `MCPProvider` signatures, and local stdio distribution. + +Alternatives: one combined PR mixes correctness changes with a major dependency migration and weakens rollback attribution; a new server/SDK rewrite increases maintenance without a customer requirement. Staying on v0.54.1 indefinitely preserves legacy interoperability but cannot substantiate July support. + +### SDK release/API review + +Candidate v1.3.0 is publicly released (2026-10-14), tagged and signed in GitHub's UI; its module path remains `github.com/mark3labs/mcp-go`, minimum declared Go is 1.25.5. [Release](https://github.com/mark3labs/mcp-go/releases/tag/v1.3.0), [module](https://github.com/mark3labs/mcp-go/blob/v1.3.0/go.mod). + +| Existing consumed API | Public v1.3.0 source inspection | +|---|---| +| `server.NewMCPServer(name,version,opts...)`, `AddTool`, `ListTools` | Retained; input validator remains opt-in | +| `server.ServeStdio(server,opts...)` | Retained; no transport rewrite required | +| `MCPServer.HandleMessage(ctx,json.RawMessage)` | Retained; dispatcher routes both eras and decorates modern responses | +| `mcp.Tool.InputSchema.Properties/Required`, `CallToolRequest`, `CallToolResult.IsError/Content` | Retained consumed fields; new protocol fields are additive for this use | +| Tool/property/annotation helpers and ParseString/ParseBoolean | Consumed helpers remain; `WithStringItems` exists in both SDK versions | +| Version negotiation | `version.go` lists July plus all four existing revisions; legacy negotiation is capped at 2025-11-25 | +| Cache hints | `server.WithCacheHints(0,mcp.CacheScopePrivate)` exists; modern response decorator emits cache/result metadata | + +Sources: [version.go](https://github.com/mark3labs/mcp-go/blob/v1.3.0/mcp/version.go), [server API](https://github.com/mark3labs/mcp-go/blob/v1.3.0/server/server.go), [dispatcher](https://github.com/mark3labs/mcp-go/blob/v1.3.0/server/request_handler.go), [stdio](https://github.com/mark3labs/mcp-go/blob/v1.3.0/server/stdio.go), [tool types/helpers](https://github.com/mark3labs/mcp-go/blob/v1.3.0/mcp/tools.go), [response metadata](https://github.com/mark3labs/mcp-go/blob/v1.3.0/server/response.go). + +Selection is supported by released source/API inspection, not compilation. No candidate SDK was installed or built. Future PR B must compile all actual consumers and launch both eras before claiming compatibility; unexpected API churn beyond the listed files stops that PR for review. No opportunistic dependency/toolchain upgrade. + +## Components and behavior + +- PR A: enable SDK input-schema validation in `NewServer`; audit registered schemas; retain permissive additional properties where currently declared. Add string item constraints to all arrays documented as strings, and defensive all-or-nothing skeleton parsing. Compile all built-in tool input schemas in offline contract tests; the SDK silently skips malformed schemas, so tests must fail on non-compiling definitions or nonlocal `$ref`. +- Route the in-process adapter through the same SDK `HandleMessage` dispatcher using unique per-instance request IDs and existing arguments serialized as JSON; do not call raw handlers around SDK validation. Convert protocol/tool error results to nonnil Go errors while preserving useful bounded tool feedback. Keep successful text behavior and public interfaces. Audit logs record tool/status only, never raw arguments, result bodies, or raw engine errors. Sort both in-process inventory methods by name. +- Set `IsError:true` on engine trigger failure while preserving the existing `success:false` payload. A fake EngineProvider exercises this test; do not trigger a real workflow. Validation reports with `valid:false` or compatibility reports with `compatible:false` are successful analytical results, not invocation errors. +- Cancellation proof has three distinct levels: (1) dispatcher/fake-engine test waits for explicit handler entry, then cancels and observes `ctx.Done()` without post-cancel success; (2) a test-only subprocess serves the existing conditional `run_workflow` through real SDK stdio with the same blocking fake, emits a fixed nonsecret entry barrier on its captured stderr, and the client waits for that barrier before sending a matching cancellation notification; (3) the newly built production wfctl, whose tools are short computations and have no engine, proves only cancellation-notification robustness and continued valid-request handling. No new production delay/probe tool or API. Every operation has a five-second deadline, and child processes are terminated/reaped on failure. +- PR B changes only the existing SDK pin and necessary API adaptation, retaining PR A's behavior. Explicit private/zero-TTL cache hints apply to local documentation and inventories. For local full-reference privacy proof, tests create a synthetic `DOCUMENTATION.md` containing an exact public marker, pass its path explicitly via `-documentation-file` to helper and newly built wfctl processes, and read `workflow://docs/full-reference`. Assert exact synthetic content plus `resultType:"complete"`, `ttlMs:0` and `cacheScope:"private"` on actual wire results; a public fallback cannot satisfy this check. Marker is allowed only in the requested resource result, absent from stderr/audit logs. Never read operator/repository documentation. SDK supplies July dispatch/metadata; Workflow adds assertions rather than reimplementing protocol negotiation. +- Continue treating configs, plugin manifests, documentation and tool outputs as untrusted data. No hooks, shell-execution tools, credentials, network schema dereferencing or telemetry are added. + +### Annotation decision table + +Apply explicit `readOnlyHint`, `destructiveHint`, `openWorldHint` in each registration, with an exhaustive test for the default 33 tools and conditional engine tool. Do not rely on SDK defaults. + +| Tools | readOnly | destructive | openWorld | Reason | +|---|---|---|---|---| +| All existing non-engine tools except openWorld exceptions below | true | false | false | Retrieval/computation/preview; `modernize apply:true` returns transformed YAML but does not persist it | +| `list_plugins`, `detect_infra_needs` | true | false | true | Caller can supply arbitrary filesystem directories | +| `ci_plan`, `generate_github_actions` | true | false | true | cigen loads config files/imports from temporary-path context; arbitrary local references may be examined | +| Conditional `run_workflow` | false | true | true | Engine-specific workflows can cause unknown external/destructive effects | + +`registry_search` searches a configured local catalog; it does not contact an arbitrary remote registry. Source audit must confirm every registration fits its row; discovering another external read expands the exception row explicitly, not by setting every tool openWorld. Temporary implementation files used for computation do not imply a persisted customer artifact. Annotation tests cannot serve as authorization tests. + +## Security Review + +Local process launch/OS permissions remain the access boundary; no remote endpoint or identity flow. CLI has no attached engine, so `run_workflow` remains absent. Engine-backed embedders retain responsibility for authorization. Input validation runs before handlers across both adapters. Public fixtures and temporary isolated dirs only; inherited secret values must never be read for tests or added to logs. + +Test malformed/wrong-type input with synthetic canaries, and inspect stdout/stderr/audit logs. Intentionally requested public result data remains allowed; this is not a promise to redact every arbitrary string. Registered input schemas must be self-contained or fragment-local; no HTTP/file `$ref` resolution. Do not claim local directory flags form a sandbox. Path/symlink sandbox redesign, generic rate limiting and universal output sanitization are separate security work; the tools spec lists broader security obligations, so this bounded plan must not claim full generic server security conformance or directory eligibility. + +## Infrastructure Impact + +No cloud resources, database migrations, listener, host installation, account changes or runtime deployments. Planning is read-only except the two local research files. Future tests may launch only isolated transient CLI/test processes. No Docker builds, app/browser/DB interaction, installed-plugin changes or persistent access. + +## Multi-Component Validation + +| Integration | Level | Required evidence | +|---|---|---| +| wfctl -> SDK stdio -> JSON-RPC client | runtime-integrated, future | Launch newly built CLI; actual discovery/list/call/error/EOF transcripts, cancellation-notification robustness only, and exact synthetic full-reference content/cache assertions | +| In-process provider -> SDK dispatcher -> handlers | runtime-integrated, future | Same malformed/valid cases, preserved IsError semantics and deterministic listing | +| mcp-go v1.3.0 | runtime-integrated, future | Go consumer compilation and legacy/July wire matrix | +| Optional engine provider | runtime-integrated via test double | Failure and deterministic cooperative cancellation across actual dispatcher and separate SDK-stdio helper; wait for entered-handler barrier before cancel, observe ctx.Done and no post-cancel success; no real engine actions | +| ChatGPT/Codex plugin activation / PR1083 package | deferred and unchanged | Existing host evaluation gate; shared CLI tests do not prove host activation | +| HTTP/auth/tasks/UI/remote services/Compute | deferred | Explicitly outside this plan | + +## Assumptions and self-challenge + +| ID | Assumption | Failure/fallback | +|---|---|---| +| A1 | Source-compatible v1.3.0 compiles with actual consumers | Compile/API gate precedes merge; unexpected spread stops PR B for review, no rewrite | +| A2 | SDK dispatcher preserves adapter success payload shape | Differential tests on representative tools before/after; retain public interface | +| A3 | SDK emits accurate July cache/metadata over stdio | Observe actual bytes; no support claim based on constants alone | +| A4 | Correct annotations can reflect existing behavior | Audit arbitrary-path/import tools; retain conservative engine labels | +| A5 | Legacy clients stay interoperable | Four-version matrix and independent PR B rollback | + +Top doubts: metadata accuracy is not access control; SDK validation can fail open on invalid schemas; SDK release notes are not Workflow runtime proof. The proposed tests and limited claims address these without feature expansion. + +## Rollback + +PR A: revert its additive contract/annotation changes together; rebuild and rerun legacy wire smoke. Document restored malformed-input permissiveness rather than concealing it. No migration or persistent state. +PR B: revert SDK/API/cache changes to v0.54.1 while retaining PR A; rebuild and run legacy smoke. July support is then explicitly unavailable; public docs must revert that claim with the pin. Never edit installed CLI binaries or active plugin sessions. + +## Review and execution gate + +Primary owner obtains independent adversarial review of this design and the companion plan. They remain local drafts, uncommitted and unlocked, pending the parent's execution decision. Existing human authorization remains applicable; this document adds no new human approval requirement. The parent applies review/alignment/scope-lock checkpoints before execution under that authorization. Future ADR: record the two-stage SDK decision and retained success/error contracts in an available numbered `decisions/` file; do not overwrite another workstream's allocation. diff --git a/docs/plans/2026-10-19-shared-mcp-review.md b/docs/plans/2026-10-19-shared-mcp-review.md new file mode 100644 index 00000000..d4adf71e --- /dev/null +++ b/docs/plans/2026-10-19-shared-mcp-review.md @@ -0,0 +1,109 @@ +# Shared MCP compatibility adversarial review + +Date: 2026-10-19. +Phase: design + plan; independent review before implementation. +Artifacts: `docs/plans/2026-10-19-shared-mcp-design.md`, `docs/plans/2026-10-19-shared-mcp.md`. +Current verdict: **PASS** after narrow Cycle 2 rereview; initial verdict was REQUEST CHANGES. +Scope: two PRs / six tasks; PR 1083 and all concurrent package changes remain separate. Review changes only this file; no implementation, SDK download/install, build, runtime process, commit or publication performed. + +## Original findings — Cycle 1 + +### P1 — Important: cancellation proof lacks a deterministic in-flight operation + +Location: plan Task 2.4, Task 3.3–3.5 and Task 5.6; design Multi-Component Validation / R6. + +Task 2's fake EngineProvider waiting on `ctx.Done()` can prove cancellation across the in-process dispatcher. Task 3 instead launches actual `runMCP`, which attaches no engine and exposes 33 predominantly short computations. The plan does not identify an entered, blocking operation or synchronization barrier before sending a cancellation notification. A cancel followed by a successful ordinary request can pass when the target request already completed or never entered its handler. Repeating it in July does not strengthen that proof. + +Fix within current scope: make the fake-engine dispatcher test wait for an explicit handler-entry barrier before canceling; require observed `ctx.Done()` and no post-cancel success effect. For actual stdio handler-cancellation proof, specify a separate test-helper process with a controlled blocking handler, entry barrier, real SDK transport, bounded completion and subsequent successful request. Keep this helper clearly distinguished from the unchanged 33-tool production CLI. For the newly built wfctl executable, narrow the claim to cancellation-notification robustness unless a real deterministic handler can be exercised safely. Do not add a production delay/probe tool or launch a real workflow. Preserve five-second operation deadlines and reaping. + +Status: **resolved in Cycle 2** — revised Task 2.4 requires entered-handler synchronization before context/notification cancellation; Task 3.3–3.4 and Task 5.6 distinguish the controlled 34-tool SDK-stdio helper from the 33-tool built production CLI. The helper must observe cooperative cancellation and no post-cancel success; production CLI claims only notification robustness. Deadlines/reaping remain required. + +### P2 — Important: local-resource privacy claim can be tested against public fallback content + +Location: plan Task 5.1–5.2; design PR B cache policy / A3. + +The wire plan reads “one public docs URI” but asserts that local full-reference content remains private. In the proposed empty temporary cwd/plugin layout, `workflow://docs/full-reference` can return its built-in public fallback without reading any local documentation. That passes metadata assertions without exercising the local-file boundary at issue. `mcp/server.go` supports an explicit documentation override and otherwise probes configured/local paths; this boundary needs a controlled fixture. + +Fix within current scope: write a synthetic `DOCUMENTATION.md` under the test's temporary directory, start the helper and newly built wfctl with explicit `-documentation-file `, read `workflow://docs/full-reference`, and assert the exact synthetic marker/content plus `resultType:"complete"`, explicit `ttlMs:0`, and `cacheScope:"private"` on the actual response. Use public synthetic bytes, never operator/repository documentation or secrets. Assert the marker is absent from stderr/audit logs while allowing it in the explicitly requested resource result. This fits `cmd/wfctl/mcp_stdio_test.go`; no new resource or feature is required. + +Status: **resolved in Cycle 2** — revised Task 5.1–5.2 supplies exact synthetic DOCUMENTATION.md bytes, explicit `-documentation-file` for helper and newly built CLI, exact full-reference result equality/marker, complete/private/zero-TTL assertions and marker exclusion from stderr/audit logs. Public fallback content cannot pass. + +No Critical findings. No additional findings manufactured to meet a count: every mandatory class is scanned below. + +## Verified foundations and clean outcomes + +- The official [specification entrypoint](https://modelcontextprotocol.io/specification/) currently redirects to 2026-07-28. The [July changelog](https://modelcontextprotocol.io/specification/2026-07-28/changelog) supports the plan's stateless request model, discovery, result metadata and deterministic-listing direction. Existing initialize-era operation is not thereby proven broken. +- [mcp-go v1.3.0](https://github.com/mark3labs/mcp-go/releases/tag/v1.3.0) and its [module file](https://github.com/mark3labs/mcp-go/blob/v1.3.0/go.mod) are public. The module path remains unchanged and its Go minimum is 1.25.5, below this repository's Go 1.27.2. Public API inspection supports evaluating this candidate; compilation/runtime success remains unverified. +- Local v0.54.1 `mcp.NewTool` already emits explicit default annotation booleans. Workflow has 11 ordinary registrations without a read-only override and explicitly marks `modernize` false. The proposal correctly fixes meanings rather than claiming absent wire fields. `modernize apply:true` returns transformed YAML without persisting it; local arbitrary-path/import tools warrant the explicit open-world exceptions. Metadata is not authorization. +- v0.54.1 already sorts wire `tools/list`. The unsorted maps are the Workflow in-process `ListTools`/`ListToolSchemas` methods. The plan targets those and adds a wire regression assertion rather than inventing another sort layer. +- Both inspected SDK versions use `santhosh-tekuri/jsonschema/v6` for input validation and skip validation if a schema fails compilation. The plan's inventory compile gate addresses that fail-open risk for Workflow-owned schemas. The compiler's default loader is unset; external references do not trigger network/file loading by default. See [candidate input validation](https://github.com/mark3labs/mcp-go/blob/v1.3.0/server/input_validation.go); locally inspected `jsonschema/v6@v6.0.3/compiler.go` and `loader.go` confirm no default URL loader. Do not add one. +- Candidate [response decoration](https://github.com/mark3labs/mcp-go/blob/v1.3.0/server/response.go) already defaults cache hints to private/zero TTL and decorates modern responses. Explicit Workflow policy is reasonable; it is not a fix for an observed public-cache default. [Caching semantics](https://modelcontextprotocol.io/specification/2026-07-28/server/utilities/caching) permit same-context private reuse; zero TTL marks results immediately stale. Neither field is an access-control mechanism. +- The [tools error contract](https://modelcontextprotocol.io/specification/2026-07-28/server/tools#error-handling) distinguishes malformed/unknown-tool protocol failures from actionable tool failures. A successful validation report with `valid:false` or compatibility report with `compatible:false` is analytical data. The plan preserves this distinction while repairing stripped `IsError` and engine failure flags. +- Current `docs/mcp.md` omits much of the registered surface and promises restart-free manifest discovery. The planned 33-tool/5-resource inventory and startup/restart correction is justified. Runtime binary/type loading and desktop activation remain separate claims. + +## Mandatory bug-class scan — initial cycle + +This table preserves the initial scan and finding history. Cycle 2 resolves the two findings below; it introduces no additional finding or scope change. + +| Class | Result | Specific check | +|---|---|---| +| Project-guidance conflicts | Clean | Read Workflow AGENTS/CLAUDE/agent/layout guidance; future isolation, GOWORK=off, additive tests/docs and package exclusions conform. | +| Assumptions under attack | Finding | P1/P2 expose proof assumptions; SDK source compatibility is appropriately separated from future compilation and launched-byte evidence. | +| Repo-precedent conflicts | Clean | Reuses existing `mcp/`, `runMCP`, SDK, provider interface and JSON Schema dependency; no replacement framework. | +| Artifact-class precedent | Clean | Go tests are beside source; inline public fixtures avoid new runnable root examples; ADR allocation is checked rather than presumed. | +| YAGNI violations | Clean | Remote/auth/tasks/MRTR/UI/output-schema migration and universal hardening are explicitly deferred; two PRs separate correctness from dependency migration. | +| Missing failure modes | Finding | P1: cancellation can race completion and falsely pass; other malformed/schema/tool/engine/EOF/version/cache failures are represented. | +| Security/privacy at architecture level | Finding | P2: private local-file content is not yet exercised; synthetic canaries, minimal env, no raw audit values and no ambient config are otherwise explicit. | +| Infrastructure impact | Clean | No hosting, databases, accounts, listeners, installation, Docker or persistent access; planning authorizes no source operations. | +| Multi-component validation | Finding | P1/P2 need concrete adjacent-component stimuli; real built CLI/SDK/provider boundaries are otherwise required. | +| Declared integration proof | Clean | Matrix labels future CLI/SDK/provider proof, fake-engine boundary and deferred hosts/remote integrations without equating a fake with engine runtime. | +| Contributed UI rendering proof | Clean | No contributed UI or route claims. | +| Rollback story | Clean | PR A reverts contracts; PR B independently restores v0.54.1 while retaining PR A and removing July claims. | +| Simpler alternative not considered | Clean | Combined PR, frozen SDK and server rewrite are considered; none improves the bounded two-stage proof/rollback path. | +| User-intent drift | Clean | Shared MCP maintenance remains separate from packaging PR 1083, active sessions/CMS and Compute work. | +| Existence/runtime-validity | Finding | Consumer APIs and target source files exist; P1/P2 concern stimuli missing from promised real-consumer proof, not imaginary SDK surfaces. | +| Over-/under-decomposition | Clean | Six tasks across two sequential PRs are reasonable; implementers should retain individual RED/GREEN/checkpoint actions within the listed steps. | +| Verification-class mismatch | Finding | P1/P2: current tests can prove robustness/public fallback metadata while claiming handler cancellation/local-resource privacy. | +| Auth/authz chain composition | Clean | No new auth chain; existing local OS/process boundary and engine-embedder responsibility are explicit. | +| Hidden serial dependencies | Clean | PR B follows PR A; shared server/docs/test/ADR files are sequenced, not falsely parallelized. | +| Missing rollback wiring | Clean | Each task includes rollback and Task 6 requires an isolated old-pin/candidate rebuild sequence. | +| Missing integration proof | Finding | P1/P2 require amendments; remaining actual compiled CLI and provider-dispatch tests cross the intended boundaries. | +| Missing declared integration matrix | Clean | Every declared component is runtime-integrated via future evidence, controlled test double or explicitly deferred. | +| Missing contributed UI route proof | Clean | No UI integration. | +| Infrastructure verification mismatch | Clean | No infrastructure mutation; future full checks remain resource-safe and explicit blockers if unavailable. | +| Plugin-loader runtime layout | Clean | No external plugin binary build/load is added; default inventory uses isolated temp directories. | +| Config-validation schema rules | Clean | Tool argument schemas, array items and offline compilation are selected; no new runnable app config or replacement YAML validator. | +| Identifier/naming-convention match | Clean | Checked actual SDK options, flags, tool names and July metadata/error identifiers; ordinary successful payloads/interfaces remain stable. | +| Planned-code compile-validity | Clean | No implementation code snippets require compilation now; exact SDK pin/API compilation and build-metadata gates precede future compatibility claims. | + +## Options and remaining unknowns + +1. Prefer the smallest cancellation claim: prove cooperative cancellation deterministically through the real dispatcher/fake engine; prove only notification robustness through the unchanged production CLI. Add a test-only stdio blocking consumer only if transport cancellation itself is an acceptance requirement. +2. Keep all July cache tests explicit and local: one synthetic full-reference fixture plus list/read metadata assertions is sufficient. No cache service, public resource, authentication mechanism or operator file is needed. + +Current planning blockers: **none from this review**; P1 and P2 are resolved by the revised plan/design. SDK compilation, newly built CLI behavior, rollback execution, full CI and desktop activation remain **unknown future gates**, not failures observed by this review; the plan correctly forbids claiming them early. Broader filesystem/rate-limit/output-security work remains deferred and prevents blanket security/directory-certification claims. + +## Cycle 2 — narrow rereview + +Verdict: **PASS** for design/plan acceptance. No implementation or publication permission is created by this verdict. + +| Finding | Resolution checked | Outcome | +|---|---|---| +| P1 | Design R6, Components cancellation levels and integration matrix; plan Task 2.4, Task 3.3–3.4, Task 5.6 | Resolved: entry barrier precedes cancellation; fake dispatcher and real SDK-stdio helper prove cooperative behavior; unchanged built CLI proves only notification robustness. | +| P2 | Design local full-reference proof; plan Task 5.1–5.2 | Resolved: test-created exact file bytes and explicit CLI flag force local resource retrieval; actual response must match and carry complete/private/zero-TTL metadata. | +| Scope | Scope Manifest and exclusions | Unchanged: two PRs, six tasks; helper uses existing conditional engine tool, no production probe/tool, package changes, remote endpoint or new integration. | + +No new Critical/Important finding from these corrections. The existing test helper/file/deadline boundaries make both proposed proofs concrete and falsifiable. Original findings and full initial scan are retained above; no unrelated broad rereview was performed. + +Reviewed corrected artifact SHA-256: + +- `docs/plans/2026-10-19-shared-mcp-design.md`: `6980878e8c567fce013e7722ffba4be174988c903d24e514b53c05b74de09d7b` +- `docs/plans/2026-10-19-shared-mcp.md`: `e68949e632007d71a5a9ed3a2ccf3510b083ee80df32e9e15aed38b374319d1b` + +Read-only source-status check for `mcp/`, `cmd/wfctl/`, `go.mod` and `go.sum` returned clean during this rereview. This is a planning gate only; no future test result is claimed. + +## Review evidence / writes + +Inspected local Workflow `mcp/{server,library,tools,wfctl_tools,scaffold_tools}.go`, provider interface, relevant tests/docs, CLI startup, cached mcp-go v0.54.1 and jsonschema v6.0.3 source; compared public v1.3.0 module/version/server/dispatcher/stdio/validation/response source and official July specification pages. Existing unrelated working-tree changes were observed and preserved. + +Writes: `docs/plans/2026-10-19-shared-mcp-review.md` only. No source edits, commits, installations, SDK compilation, tests, publication or active app/session interactions. diff --git a/docs/plans/2026-10-19-shared-mcp.md b/docs/plans/2026-10-19-shared-mcp.md new file mode 100644 index 00000000..dca22f54 --- /dev/null +++ b/docs/plans/2026-10-19-shared-mcp.md @@ -0,0 +1,125 @@ +# Shared Workflow MCP Compatibility Implementation Plan + +> **For the implementing agent:** REQUIRED SUB-SKILL: Use autodev:executing-plans to implement this plan task-by-task when the parent starts execution under existing user authorization after review. + +**Goal:** Correct existing MCP tool contracts and prove July 2026 plus legacy stdio interoperability without changing the assistant packaging work. +**Architecture:** Keep Workflow's current CLI/server and SDK. First correct contracts on v0.54.1; then upgrade the existing SDK to publicly verified v1.3.0 with independently reversible wire proof. +**Tech Stack:** Go 1.27.2 per current repository; mcp-go v0.54.1 then v1.3.0; existing JSON Schema dependency; standard Go testing and stdlib subprocess client. +**Base branch:** main, refreshed and inspected when execution is authorized; do not branch from or amend packaging PR 1083. +**Design:** `docs/plans/2026-10-19-shared-mcp-design.md` (local planning artifact outside repository). + +## Scope Manifest + +**PR Count:** 2 +**Tasks:** 6 +**Estimated Lines of Change:** ~600-900 (informational; no authority to grow scope to meet estimate). + +**Out of scope:** +- Changes to PR 1083, its lock, `example/workflow-agent-plugin/`, installed plugins/sessions, or active CMS/Compute work. +- New server/framework, orchestration, tools/features, HTTP hosting, credentials, auth flows, tasks, MRTR features, UI, prompts or sampling. +- Structured-output migration, filesystem sandbox redesign, arbitrary user-provided schema support, generic rate-limit platform, universal redactor, directory submission or certification. +- Updating Go/toolchain or unrelated dependencies; changing unrelated source or protected workflows. + +**PR Grouping:** + +| PR # | Title | Tasks | Branch | +|---|---|---|---| +| 1 | fix: enforce shared MCP tool contracts and metadata | Task 1, Task 2, Task 3 | fix/mcp-tool-contracts | +| 2 | fix: support July 2026 MCP while preserving legacy stdio | Task 4, Task 5, Task 6 | fix/mcp-july-compatibility | + +**Status:** Locked 2026-10-19T23:39:01Z + +PR 2 follows merged PR 1 (or is explicitly stacked for review); never silently collapse, drop or enlarge these PRs. Both begin draft. Source changes, branch creation, commits and publication remain future work pending the parent's execution decision under existing user authorization. Rebase/precedent inspection must preserve concurrent changes. Before implementation, independent adversarial review and design-to-plan alignment must pass and the owner must lock this manifest; this local draft itself is not a scope lock. No additional human approval gate is introduced. + +## Requirements and acceptance map + +| Design | Task | Acceptance | +|---|---|---| +| R1/R2 validation and arrays | 1, 3, 5 | Invalid invocation rejected before handler; valid typed input unchanged | +| R3 errors / adapter parity | 2, 3, 5 | Tool/engine errors remain errors; `valid:false` analytical report remains success | +| R4 truthful explicit annotations | 3 | Exhaustive per-name booleans match audited behavior | +| R5 SDK July and legacy | 4, 5, 6 | Real CLI passes four legacy versions and stateless July | +| R6 wire/security/determinism | 1, 2, 3, 5 | Offline schemas; no raw secret logging; clean framing/EOF/cancel/private cache | +| R7 documentation accuracy | 3, 6 | 33 tools/5 resources by live inventory, restart caveat, supported eras and limits | + +## Future execution prerequisites + +Read `AGENTS.md`, `docs/AGENT_GUIDE.md`, `docs/REPO_LAYOUT.md`, relevant `CLAUDE.md`, and changed-source history. Confirm target files exist; refresh source/API review if versions changed. Use a clean isolated checkout, `GOWORK=off`, temporary build/plugin/cwd directories, minimal process environment and public fixtures. Never use ambient application configs, secret values, installed plugin directories or shared app/database/browser sessions. Record source revision, SDK pin and executable digest with the final transcript. All commands below are future verification commands, not operations performed by this plan author. + +### Task 1: Enforce existing schemas without changing successful calls + +**Files:** Modify `mcp/server.go`, `mcp/wfctl_tools.go`, `mcp/scaffold_tools.go`; create `mcp/input_contract_test.go`; adjust existing `mcp/server_test.go` only for changed error expectations. + +1. Add failing dispatcher tests for missing required arguments, string-as-boolean, scalar/null array values and mixed `module_types` elements. Add a valid typed counterpart for each. For a malformed call to an engine-backed test handler, assert the handler's invocation counter stays zero. +2. Run `GOWORK=off go test ./mcp -run 'TestMCPInputContract|TestMCPSchemaInventory' -count=1`; expected pre-fix failure includes accepted wrong-type input / missing `items.type:string`. +3. Enable `server.WithInputSchemaValidation()` in `NewServer`. Add `mcp.WithStringItems()` to every string-array registration (including module_types, environment lists); retain currently permitted extra top-level fields rather than adding a global breaking strictness policy. Make skeleton handler reject any nonstring element instead of silently dropping it; no partial output. +4. Add offline schema inventory validation: every built-in schema compiles with the existing JSON Schema library; `$ref` is absent or fragment-local. No remote/file loader, no downloaded schemas, and no custom YAML validator. This catches the SDK's invalid-schema skip behavior for Workflow-owned registrations. +5. Rerun focused tests plus `GOWORK=off go test ./mcp -count=1`. Expected `PASS`; wrong-type calls have `IsError:true`; valid baseline text payloads remain equivalent. A YAML validation report with `valid:false` is a separate successful-report test. +6. Commit future task changes after review. Rollback: revert task commit and rerun existing legacy valid-call checks; explicitly record reintroduced malformed-input acceptance. + +### Task 2: Preserve adapter errors and make inventories deterministic + +**Files:** Modify `mcp/library.go`, `mcp/server.go`; test `mcp/library_test.go`, `mcp/server_test.go`; create `mcp/error_contract_test.go`. + +1. Add failing tests: provider calls with missing/wrong-type input return nonnil Go error; failed EngineProvider trigger produces `IsError:true` and existing `success:false`; successful `valid:false`/`compatible:false` reports return nil Go error. Preserve successful string outputs and the `MCPProvider` interface. +2. Run `GOWORK=off go test ./mcp -run 'TestMCPErrorContract|TestInProcessErrorContract|TestInProcessInventoryOrder' -count=1`; expect old error-stripping, engine IsError and ordering assertions to fail. +3. Replace raw-handler bypass in `InProcessServer.CallTool` with SDK `HandleMessage` dispatch of serialized tools/call JSON-RPC. Use monotonically unique per-instance request IDs for concurrent calls; do not introduce external sessions/network. Decode actual protocol/tool errors and retain bounded caller feedback. Keep audit logs to tool/status metadata; do not log raw args, config, payload or engine error strings. Sort `ListTools` and `ListToolSchemas` by tool name. +4. On `TriggerWorkflow` failure preserve payload but mark tool result error. Do not mark analytical negative results as invocation errors. For deterministic cooperative cancellation, a fake EngineProvider signals an explicit entered-handler barrier and then blocks on `ctx.Done()`. The test waits for that barrier before canceling the provider call context; a dispatcher test with known request ID waits for entry before sending a matching `notifications/cancelled` through the same SDK dispatcher. Require observed ctx.Done, bounded completion, no post-cancel success counter, and a subsequent successful request. Never start a real engine workflow; a cancel sent to an already completed request is not cancellation proof. +5. Run focused tests, `GOWORK=off go test ./mcp -count=1`, and `GOWORK=off go test -race ./mcp -run 'TestInProcessErrorContract|TestInProcessInventoryOrder' -count=1`. Expected `PASS`, no data race, identical sorted names across repeated calls, no canary in audit log. +6. Commit after review. Rollback: revert adapter/engine-error commit as a unit; public interface remains unchanged in both directions. + +### Task 3: Audit annotations and prove PR 1 through real CLI stdio + +**Files:** Modify registrations in `mcp/server.go`, `mcp/tools.go`, `mcp/wfctl_tools.go`, `mcp/scaffold_tools.go`; create `mcp/annotations_test.go`, `cmd/wfctl/mcp_stdio_test.go`; update `docs/mcp.md`, `docs/mcp-tools-reference.md`; create one available numbered `decisions/-shared-mcp-contracts-and-protocol.md` after checking allocation. + +1. Write failing exhaustive annotation tests keyed by each registered name. Read-only computations/previews: true/false for readOnly/destructive; modernize is read-only even when returning fixed_yaml. OpenWorld exceptions: list_plugins, detect_infra_needs, ci_plan, generate_github_actions true for arbitrary local paths/imports; bounded remaining catalogs/computations false. Conditional run_workflow: false/true/true. Assert exact booleans exist; they already exist today, so test semantic mismatches rather than presence alone. +2. Run `GOWORK=off go test ./mcp -run TestMCPAnnotations -count=1`; expect failure against SDK-default meanings. Apply the design's explicit annotation table; audit actual effects before assigning each row. Keep optional idempotence tuning outside scope. +3. Implement a Go stdio contract client in `cmd/wfctl/mcp_stdio_test.go`. Production-CLI mode exercises actual `runMCP` in a fresh subprocess of the compiled Go test binary, with isolated cwd/plugin dirs and a minimal environment. An explicit `WFCTL_MCP_TEST_BINARY` selects a newly built wfctl executable for this mode. A distinct test-only transport-helper mode always launches the Go test binary, constructs the existing server with a blocking fake EngineProvider, and serves actual SDK stdio; it never adds a production tool or substitutes for built-CLI proof. Its existing conditional run_workflow signals `WFCTL_MCP_TEST_HANDLER_ENTERED` on captured stderr after entry, then blocks on ctx.Done; parent waits for this exact barrier before sending the matching cancellation notification. Require observed cancellation, no post-cancel success, bounded completion and subsequent valid request. Keep stdout exclusively MCP JSON. Public fixture YAML lives as test strings; every response is parsed and asserted. +4. Test legacy initialize/initialized for 2024-11-05, 2025-03-26, 2025-06-18, 2025-11-25; tools/list, resources/list, valid call, wrong-type call, mixed array and malformed JSON. Default production count 33 tools/5 resources; run_workflow absent. Test no extra stdout, notification response absence, EOF exit and SIGTERM exit. For the actual/newly built production CLI assert cancellation-notification robustness and subsequent valid-request handling only, not cancellation of a short handler. Cooperative handler cancellation is proved by the separate entered-barrier transport helper (34 tools including the existing engine tool), not by a notification race. Require 5-second per-operation deadlines and always terminate/reap children. Reject raw canaries in stderr/audit logs; allow explicitly requested public fixture data and the transport helper's fixed nonsecret stderr barrier. +5. Run `GOWORK=off go test ./cmd/wfctl -run TestMCPStdioLegacy -count=1` and focused package tests. Then build an isolated executable with `GOWORK=off go build -o "$TASK_MCP_BIN/wfctl" ./cmd/wfctl`, where TASK_MCP_BIN is a fresh temporary directory; run `GOWORK=off WFCTL_MCP_TEST_BINARY="$TASK_MCP_BIN/wfctl" go test ./cmd/wfctl -run TestMCPStdioLegacy -count=1`. Expected exit 0/`PASS`, four negotiated versions and asserted tool errors. +6. Correct docs to 33 tools/5 resources, exact live argument names, manifest startup/restart behavior, negative-report versus invocation-error semantics, sanitized-input/runtime limits and full-surface visibility. Keep existing packaging links untouched. Record ADR rationale for two PRs, error preservation and dependency reuse; check filename availability before adding it. +7. Run `GOWORK=off golangci-lint run --new-from-rev=origin/main` and relevant checks; inspect doc links/rendering and diff for package/lock/secret changes. The parent may open PR 1 as draft under existing publication authorization after its execution/review checkpoints. Rollback: revert PR 1, rebuild and rerun legacy smoke; no installed binary/session mutation. + +### Task 4: Upgrade existing SDK after actual API compilation + +**Files:** Modify `go.mod`, `go.sum`, `mcp/server.go`; adjust only proven necessary consumers among `mcp/library.go`, `mcp/server_test.go`, `mcp/input_contract_test.go`, `mcp/error_contract_test.go`, `mcp/annotations_test.go`, `cmd/wfctl/mcp_stdio_test.go`. Do not change package manifests, Go directive, unrelated dependencies or generated engine code. + +1. Refresh public v1.3.0 release/tag and consumed API review described in design. Confirm module path and no need for another framework; inspect release diff before downloading dependencies. Source/API compatibility is verified at planning time; executable compatibility is not. +2. In isolated authorized checkout pin only mcp-go to v1.3.0; use `GOWORK=off go get github.com/mark3labs/mcp-go@v1.3.0`. Inspect go.mod/go.sum diff; reject unrelated version churn instead of blindly accepting tidy output. +3. Run `GOWORK=off go test ./mcp ./cmd/wfctl -run 'TestMCP|TestInProcess' -count=1` and `GOWORK=off go test ./... -run '^$'`. Expected exit 0 and no unresolved API symbols across real consumers. If changes exceed the listed API adaptation boundary, stop for design review; do not rewrite framework or silently drop PR 2. +4. Set `server.WithCacheHints(0,mcp.CacheScopePrivate)` explicitly. Keep existing capabilities; do not enable tasks/extensions/auth/UI. Preserve legacy payload semantics. Re-run all PR 1 contract tests on the new SDK. +5. Audit versions with `GOWORK=off go list -m github.com/mark3labs/mcp-go` (expect `github.com/mark3labs/mcp-go v1.3.0`); build isolated CLI and inspect `go version -m` for same pin. Launch legacy suite against that executable before continuing. +6. Commit after review. Rollback: restore v0.54.1 go.mod/go.sum and accompanying API/cache adaptations, retain PR 1 fixes, rebuild and pass legacy suite. + +### Task 5: Prove July stateless wire behavior and security boundaries + +**Files:** Extend `cmd/wfctl/mcp_stdio_test.go`; create `mcp/protocol_contract_test.go`; extend `mcp/input_contract_test.go`. Only minimal `mcp/server.go` adaptation if actual tests show SDK wiring omission; no handwritten protocol engine. + +1. Add July stdio tests using per-request `_meta` keys `io.modelcontextprotocol/protocolVersion:"2026-07-28"` and `io.modelcontextprotocol/clientCapabilities:{}`; no initialize/initialized in this client path. Create `DOCUMENTATION.md` inside the test temporary directory with exact public synthetic bytes `Workflow MCP synthetic public documentation fixture\nmarker: PUBLIC_MCP_DOCS_20261019\n`. Launch both the CLI helper and newly built production wfctl with explicit `-documentation-file ` plus isolated plugin/cwd directories; never use operator/repository documentation. Call server/discover, tools/list, resources/list, resources/templates/list, resources/read for `workflow://docs/full-reference`, valid tools/call and invalid typed call. +2. Assert discovery supportedVersions contains July and all four legacy revisions; advertised capabilities match actual tools/resources, without unimplemented extensions. Ordinary results carry resultType complete. On each actual `workflow://docs/full-reference` wire result assert the text equals the exact synthetic file bytes and contains PUBLIC_MCP_DOCS_20261019, plus explicit resultType complete, ttlMs 0 and cacheScope private. Public fallback content must fail this test. The marker may appear in this explicitly requested result and must be absent from captured stderr/audit logs. Assert the same cache fields on other cacheable results. Server identity metadata identifies wfctl-mcp and observed source version; repeated tool ordering is stable. +3. Negative wire cases: missing/wrong-shaped modern clientCapabilities -> -32602; unsupported modern date -> -32022 with supported versions; modern initialize and ping -> -32601. Repeat wrong-argument/engine-failure semantics in dispatcher tests. A malformed request is a protocol error; tool argument/business failure is IsError; analytical negatives remain normal results. +4. Verify no version/capability leakage across successive modern/legacy requests, no unsolicited logging when logLevel absent, no unknown extension advertised, and no attempt to network-resolve schemas. Run schema-inventory compilation offline for every built-in tool, including engine-attached variant. Test external/absolute refs are rejected by the Workflow-owned schema audit; do not make real outbound requests. +5. Run `GOWORK=off go test ./mcp -run 'TestMCPProtocolContract|TestMCPSchemaInventory' -count=1` and `GOWORK=off go test ./cmd/wfctl -run 'TestMCPStdioLegacy|TestMCPStdioJuly' -count=1`. Build isolated CLI and repeat with `WFCTL_MCP_TEST_BINARY` exactly as Task 3. Expected exit 0/`PASS`; assertions observe real response bytes, not SDK constants alone. +6. Reuse Task 3 EOF/frame/canary cases in both eras. In the test-only SDK-stdio helper, wait for the entered blocking fake-handler barrier before sending notifications/cancelled; assert observed ctx.Done, no post-cancel success, bounded completion and subsequent valid request. In the newly built production wfctl, assert only cancellation-notification robustness and continued service. Do not equate that CLI robustness check with deterministic handler cancellation. If SDK behavior fails, report exact contract and stop compatibility claims; a bounded upstream fix/pin needs review, not transport rewriting. Rollback: Task 4 SDK rollback plus legacy suite; no persisted data. + +### Task 6: Publish accurate compatibility evidence and finish separate PR 2 + +**Files:** Update `docs/mcp.md`, `docs/mcp-tools-reference.md` and the Task 3 decision record; no package example or PR1083 files. Final transcripts are sanitized task artifacts, not committed machine-path logs. + +1. Document supported revisions from successful launched-CLI matrix, both connection models, explicit private cache policy, current tool/resource inventory, exact argument schemas, restart requirements, input/error changes and adapter parity. Explain July upgrade need without claiming existing initialize clients are broken. Note historical SDK upgrade rather than saying SDK was never maintained. +2. Separate protocol requirements from OpenAI directory policy and optional features. State text-only results remain supported; no structured-output migration or directory/host certification. List remaining filesystem/rate-limit/output-sanitization security work without claiming complete generic security compliance. +3. Run `GOWORK=off go test ./mcp ./cmd/wfctl -count=1`, `GOWORK=off golangci-lint run --new-from-rev=origin/main`, and actual built-binary legacy/July suites. Run full repository checks required by current repo guidance/CI when authorized and resource-safe; do not substitute mocks or prior release probes for new executable evidence. +4. Review integration matrix: CLI/SDK/provider runtime evidence present, engine represented by controlled fake only, desktop activation deferred, HTTP/auth/tasks/UI/Compute unchanged. Inspect docs links/rendered Markdown, dependency diff, tracked/untracked changes, and package/lock exclusions. Any unmet required check remains an explicit blocker. +5. Independently review final changes; the parent may publish PR 2 as draft under existing source/publication authorization after its execution checkpoints. Attach created PR to its task. This plan adds no new merge/deploy/install action. Record tested source SHA, executable digest, SDK version, protocol results and limitations. +6. Prove rollback in the isolated worktree: restore PR 1 base plus v0.54.1, build another temporary CLI, pass legacy suite; restore PR 2 candidate and repeat modern smoke. Expected legacy support returns, July support is unavailable under old pin, then candidate proof passes again. Do not disturb published history or other worktrees. + +## Review and remaining gates + +Independent adversarial design/plan review passed after P1/P2 corrections; formal alignment/scope lock for future execution remains pending. This plan is deliberately uncommitted outside the checkout per explicit instructions; those instructions override skill defaults to write/commit under docs/plans. No implementation or new PR has been executed. Planning is pending the parent's execution decision under existing human authorization; do not ask for redundant user approval. Before execution, record accepted review/alignment and lock exactly this manifest. SDK compile/runtime success, full CI and host activation remain unknown until their respective checks run. + +## Execution continuation + +The user explicitly authorized execution after independent review. Planning-only +restrictions above describe the prior phase; Tasks 1–6 now run in isolated +worktrees with the same two-PR grouping and exclusions. No additional human +approval gate. PR 1083 stays separate. diff --git a/docs/plans/2026-10-19-shared-mcp.md.scope-lock b/docs/plans/2026-10-19-shared-mcp.md.scope-lock new file mode 100644 index 00000000..eddec2f3 --- /dev/null +++ b/docs/plans/2026-10-19-shared-mcp.md.scope-lock @@ -0,0 +1 @@ +8e54871b05026001c16a690eaaf981065119ae5952654aac83e7e2229ff0fbc5 From 6f58f1b5e8630601e2881cc29f13db2a64fb5798 Mon Sep 17 00:00:00 2001 From: Jon Langevin Date: Mon, 19 Oct 2026 19:43:29 -0400 Subject: [PATCH 2/5] fix(mcp): enforce tool input schemas (task_id:1) --- mcp/input_contract_test.go | 281 +++++++++++++++++++++++++++++++++++++ mcp/scaffold_tools.go | 2 + mcp/server.go | 10 +- 3 files changed, 290 insertions(+), 3 deletions(-) create mode 100644 mcp/input_contract_test.go diff --git a/mcp/input_contract_test.go b/mcp/input_contract_test.go new file mode 100644 index 00000000..a68d3906 --- /dev/null +++ b/mcp/input_contract_test.go @@ -0,0 +1,281 @@ +package mcp + +import ( + "bytes" + "context" + "encoding/json" + "fmt" + "strings" + "testing" + + "github.com/mark3labs/mcp-go/mcp" + "github.com/santhosh-tekuri/jsonschema/v6" +) + +const inputContractYAML = "modules:\n - name: web\n type: http.server\n config:\n address: \":8080\"\n" + +// Exercise Workflow's actual SDK dispatcher and serialized response, rather +// than invoking a handler around the server's input-validation options. +func dispatchInputContract(t *testing.T, srv *Server, tool string, args map[string]any) *mcp.CallToolResult { + t.Helper() + request, err := json.Marshal(map[string]any{ + "jsonrpc": "2.0", "id": 1, "method": "tools/call", + "params": map[string]any{"name": tool, "arguments": args}, + }) + if err != nil { + t.Fatal(err) + } + response := srv.MCPServer().HandleMessage(t.Context(), request) + encoded, err := json.Marshal(response) + if err != nil { + t.Fatal(err) + } + var reply struct { + Result *mcp.CallToolResult `json:"result"` + Error *mcp.JSONRPCErrorDetails `json:"error"` + } + if err := json.Unmarshal(encoded, &reply); err != nil { + t.Fatal(err) + } + if reply.Error != nil || reply.Result == nil { + t.Fatalf("expected tool result, got %s", encoded) + } + return reply.Result +} + +func TestMCPInputContract(t *testing.T) { + tests := []struct { + name string + tool string + args map[string]any + field string + }{ + {"missing required", "validate_config", map[string]any{}, "yaml_content"}, + {"wrong YAML type", "validate_config", map[string]any{"yaml_content": 73}, "yaml_content"}, + {"string strict flag", "validate_config", map[string]any{"yaml_content": inputContractYAML, "strict": "false"}, "strict"}, + {"string skip flag", "validate_config", map[string]any{"yaml_content": inputContractYAML, "skip_unknown_types": "true"}, "skip_unknown_types"}, + } + for _, array := range []struct { + tool string + field string + base map[string]any + }{ + {"get_config_skeleton", "module_types", nil}, + {"scaffold_ci", "environments", map[string]any{"description": "Public sample API"}}, + {"scaffold_environment", "environments", map[string]any{"provider": "docker"}}, + } { + for _, value := range []struct { + name string + data any + }{ + {"scalar", "staging"}, + {"null", nil}, + {"mixed", []any{"http.server", 73}}, + } { + args := map[string]any{array.field: value.data} + for key, val := range array.base { + args[key] = val + } + tests = append(tests, struct { + name string + tool string + args map[string]any + field string + }{array.tool + "/" + value.name, array.tool, args, array.field}) + } + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + result := dispatchInputContract(t, NewServer(""), tt.tool, tt.args) + text := extractText(t, result) + if !result.IsError { + t.Fatalf("schema-invalid %s accepted by %s: %s", tt.field, tt.tool, text) + } + if !strings.Contains(text, tt.field) { + t.Errorf("error must identify %s, got %q", tt.field, text) + } + }) + } +} + +func TestMCPInputContractValid(t *testing.T) { + for _, tt := range []struct { + name string + tool string + args map[string]any + want string + }{ + {"required YAML", "validate_config", map[string]any{"yaml_content": inputContractYAML}, `"valid": true`}, + {"typed booleans", "validate_config", map[string]any{"yaml_content": inputContractYAML, "strict": true, "skip_unknown_types": false}, `"valid": true`}, + {"module strings", "get_config_skeleton", map[string]any{"module_types": []string{"http.server", "http.router"}}, "type: http.router"}, + {"CI environments", "scaffold_ci", map[string]any{"description": "Public sample API", "environments": []string{"staging"}}, "staging"}, + {"provider environments", "scaffold_environment", map[string]any{"provider": "docker", "environments": []string{"staging"}}, "staging"}, + {"omitted optional array", "scaffold_environment", map[string]any{"provider": "docker"}, "production"}, + {"extra argument remains allowed", "validate_config", map[string]any{"yaml_content": inputContractYAML, "future_client_hint": "ignored"}, `"valid": true`}, + {"negative validation is a report", "validate_config", map[string]any{"yaml_content": "modules:\n - name: missing\n type: task1.unknown\n"}, `"valid": false`}, + } { + t.Run(tt.name, func(t *testing.T) { + result := dispatchInputContract(t, NewServer(""), tt.tool, tt.args) + text := extractText(t, result) + if result.IsError || !strings.Contains(text, tt.want) { + t.Fatalf("valid invocation changed: IsError=%v, expected %q in %q", result.IsError, tt.want, text) + } + }) + } +} + +// This engine is only a counter: malformed arguments must never reach it. +type inputContractEngine struct { + mockEngine + calls int +} + +func (e *inputContractEngine) TriggerWorkflow(_ context.Context, _ string, _ string, _ map[string]any) error { + e.calls++ + return nil +} + +func TestMCPInputContractBeforeEngine(t *testing.T) { + for _, tt := range []struct { + name string + args map[string]any + }{ + {"missing action", map[string]any{"workflow_type": "http"}}, + {"wrong workflow type", map[string]any{"workflow_type": 73, "action": "public_test"}}, + {"wrong data type", map[string]any{"workflow_type": "http", "action": "public_test", "data": "not an object"}}, + } { + t.Run(tt.name, func(t *testing.T) { + engine := &inputContractEngine{} + result := dispatchInputContract(t, NewServer("", WithEngine(engine)), "run_workflow", tt.args) + if !result.IsError || engine.calls != 0 { + t.Fatalf("malformed engine invocation: IsError=%v, engine calls=%d", result.IsError, engine.calls) + } + }) + } + t.Run("valid typed call", func(t *testing.T) { + engine := &inputContractEngine{} + result := dispatchInputContract(t, NewServer("", WithEngine(engine)), "run_workflow", map[string]any{ + "workflow_type": "http", "action": "public_test", "data": map[string]any{"value": "public"}, + }) + if result.IsError || engine.calls != 1 || !strings.Contains(extractText(t, result), `"success": true`) { + t.Fatalf("valid engine invocation changed: IsError=%v, engine calls=%d", result.IsError, engine.calls) + } + }) +} + +func TestMCPInputContractSkeletonAllOrNothing(t *testing.T) { + for _, bad := range []any{73, nil, map[string]any{"type": "http.router"}} { + result, err := NewServer("").handleGetConfigSkeleton(t.Context(), makeCallToolRequest(map[string]any{ + "module_types": []any{"http.server", bad}, + })) + if err != nil { + t.Fatal(err) + } + if !result.IsError || !strings.Contains(extractText(t, result), "module_types[1]") { + t.Errorf("mixed module_types must fail without partial skeleton: IsError=%v, %s", result.IsError, extractText(t, result)) + } + } +} + +// The SDK skips validation when a tool schema fails to compile. Keep every +// Workflow-owned definition compile-valid and self-contained, including tools +// registered only when an engine is attached. +func TestMCPSchemaInventory(t *testing.T) { + for _, variant := range []struct { + name string + srv *Server + }{ + {"stdio", NewServer("")}, + {"engine", NewServer("", WithEngine(&inputContractEngine{}))}, + } { + t.Run(variant.name, func(t *testing.T) { + for name, registered := range variant.srv.MCPServer().ListTools() { + t.Run(name, func(t *testing.T) { + data, err := json.Marshal(registered.Tool.InputSchema) + if len(registered.Tool.RawInputSchema) != 0 { + data = registered.Tool.RawInputSchema + } + if err != nil { + t.Fatal(err) + } + if err := compileInputContractSchema(data); err != nil { + t.Fatalf("invalid or nonlocal input schema: %v", err) + } + for field, property := range registered.Tool.InputSchema.Properties { + definition, ok := property.(map[string]any) + if !ok || definition["type"] != "array" { + continue + } + items, ok := definition["items"].(map[string]any) + if !ok || items["type"] != "string" { + t.Errorf("%s requires items.type:string", field) + } + } + }) + } + }) + } +} + +func TestMCPSchemaInventoryGuard(t *testing.T) { + for _, tt := range []struct { + name string + raw string + bad bool + }{ + {"invalid schema", `{"type":"not-a-schema-type"}`, true}, + {"HTTP ref", `{"$ref":"https://example.invalid/schema.json"}`, true}, + {"file ref", `{"properties":{"nested":{"$ref":"file:///never-read.json"}}}`, true}, + {"relative ref", `{"$ref":"other.json"}`, true}, + {"dynamic HTTP ref", `{"$dynamicRef":"https://example.invalid/schema.json"}`, true}, + {"fragment ref", `{"$defs":{"value":{"type":"string"}},"properties":{"value":{"$ref":"#/$defs/value"}}}`, false}, + } { + t.Run(tt.name, func(t *testing.T) { + if err := compileInputContractSchema([]byte(tt.raw)); (err != nil) != tt.bad { + t.Fatalf("offline schema guard error=%v, want rejection=%v", err, tt.bad) + } + }) + } +} + +func compileInputContractSchema(raw []byte) error { + doc, err := jsonschema.UnmarshalJSON(bytes.NewReader(raw)) + if err != nil { + return err + } + if err := checkInputContractRefs(doc); err != nil { + return err + } + compiler := jsonschema.NewCompiler() + // No URL loader: only this in-memory resource and its fragments can resolve. + const resource = "mem:///workflow/input-contract.json" + if err := compiler.AddResource(resource, doc); err != nil { + return err + } + _, err = compiler.Compile(resource) + return err +} + +func checkInputContractRefs(value any) error { + switch node := value.(type) { + case map[string]any: + for key, child := range node { + if key == "$ref" || key == "$dynamicRef" || key == "$recursiveRef" { + ref, ok := child.(string) + if !ok || !strings.HasPrefix(ref, "#") { + return fmt.Errorf("%s must be fragment-local", key) + } + } + if err := checkInputContractRefs(child); err != nil { + return err + } + } + case []any: + for _, child := range node { + if err := checkInputContractRefs(child); err != nil { + return err + } + } + } + return nil +} diff --git a/mcp/scaffold_tools.go b/mcp/scaffold_tools.go index d2fec9d1..38c9d147 100644 --- a/mcp/scaffold_tools.go +++ b/mcp/scaffold_tools.go @@ -32,6 +32,7 @@ func (s *Server) registerScaffoldTools() { mcp.Description("Go binary entrypoint path (default: ./cmd/server)"), ), mcp.WithArray("environments", + mcp.WithStringItems(), mcp.Description("Deployment environment names to include (e.g. ['staging', 'production'])"), ), ), @@ -48,6 +49,7 @@ func (s *Server) registerScaffoldTools() { mcp.Description("Deployment provider: docker, kubernetes, aws-ecs, gcp-cloudrun, digitalocean"), ), mcp.WithArray("environments", + mcp.WithStringItems(), mcp.Description("Environment names to generate (default: ['local', 'staging', 'production'])"), ), mcp.WithString("secrets_provider", diff --git a/mcp/server.go b/mcp/server.go index c3c27adc..2d1edaeb 100644 --- a/mcp/server.go +++ b/mcp/server.go @@ -123,6 +123,7 @@ func NewServer(pluginDir string, opts ...ServerOption) *Server { server.WithToolCapabilities(true), server.WithResourceCapabilities(false, true), server.WithInstructions(serverInstructions), + server.WithInputSchemaValidation(), ) // Load types from installed plugin manifests so that plugin-provided types @@ -256,6 +257,7 @@ func (s *Server) registerTools() { mcp.WithDescription("Generate a skeleton/template workflow YAML configuration for a given set of module types. Useful for bootstrapping new workflow configs."), mcp.WithArray("module_types", mcp.Required(), + mcp.WithStringItems(), mcp.Description("List of module type strings to include in the skeleton config (e.g., ['http.server', 'http.router', 'http.handler'])"), ), ), @@ -595,10 +597,12 @@ func (s *Server) handleGetConfigSkeleton(_ context.Context, req mcp.CallToolRequ } var moduleTypes []string - for _, t := range typesSlice { - if s, ok := t.(string); ok { - moduleTypes = append(moduleTypes, s) + for i, value := range typesSlice { + moduleType, ok := value.(string) + if !ok { + return mcp.NewToolResultError(fmt.Sprintf("module_types[%d] must be a string", i)), nil } + moduleTypes = append(moduleTypes, moduleType) } if len(moduleTypes) == 0 { From 354c025c31a56c174f547c43b2a66b37df6f03d6 Mon Sep 17 00:00:00 2001 From: Jon Langevin Date: Mon, 19 Oct 2026 19:56:11 -0400 Subject: [PATCH 3/5] fix(mcp): preserve adapter errors and deterministic inventories (task_id:2) --- mcp/error_contract_test.go | 320 +++++++++++++++++++++++++++++++++++++ mcp/library.go | 100 +++++++++--- mcp/library_test.go | 21 +++ mcp/server.go | 4 +- mcp/server_test.go | 3 + 5 files changed, 423 insertions(+), 25 deletions(-) create mode 100644 mcp/error_contract_test.go diff --git a/mcp/error_contract_test.go b/mcp/error_contract_test.go new file mode 100644 index 00000000..5a169d7d --- /dev/null +++ b/mcp/error_contract_test.go @@ -0,0 +1,320 @@ +package mcp + +import ( + "bytes" + "context" + "encoding/json" + "errors" + "fmt" + "log/slog" + "strconv" + "strings" + "sync" + "sync/atomic" + "testing" + "time" + "unicode/utf8" + + "github.com/mark3labs/mcp-go/mcp" + "github.com/mark3labs/mcp-go/server" +) + +const errorContractYAML = "modules:\n - name: absent\n type: task2.uninstalled-module\n" + +func TestMCPErrorContract(t *testing.T) { + engine := &mockEngine{triggerErr: errors.New("PUBLIC_SYNTHETIC_ENGINE_FAILURE")} + result := dispatchInputContract(t, NewServer("", WithEngine(engine)), "run_workflow", map[string]any{ + "workflow_type": "http", "action": "public_test", + }) + var payload map[string]any + if err := json.Unmarshal([]byte(extractText(t, result)), &payload); err != nil { + t.Fatal(err) + } + if !result.IsError || payload["success"] != false || payload["error"] != engine.triggerErr.Error() { + t.Fatalf("engine failure must retain its payload and IsError: IsError=%v, payload=%v", result.IsError, payload) + } +} + +func TestMCPErrorContractAnalyticalNegatives(t *testing.T) { + for _, item := range []struct{ tool, field string }{{"validate_config", "valid"}, {"compat_check", "compatible"}} { + t.Run(item.tool, func(t *testing.T) { + result := dispatchInputContract(t, NewServer(""), item.tool, map[string]any{"yaml_content": errorContractYAML}) + var payload map[string]any + if err := json.Unmarshal([]byte(extractText(t, result)), &payload); err != nil { + t.Fatal(err) + } + if result.IsError || payload[item.field] != false { + t.Fatal("analytical negatives must remain successful reports") + } + }) + } +} + +func TestInProcessErrorContract(t *testing.T) { + for _, item := range []struct { + name, tool, field string + args map[string]any + }{ + {"missing YAML", "validate_config", "yaml_content", map[string]any{}}, + {"wrong YAML type", "validate_config", "yaml_content", map[string]any{"yaml_content": 73}}, + {"wrong strict type", "validate_config", "strict", map[string]any{"yaml_content": inputContractYAML, "strict": "false"}}, + {"mixed array", "get_config_skeleton", "module_types", map[string]any{"module_types": []any{"http.server", 73}}}, + } { + t.Run(item.name, func(t *testing.T) { + _, err := NewInProcessServer().CallTool(t.Context(), item.tool, item.args) + if err == nil { + t.Fatalf("invalid invocation must return a Go error: %s", item.name) + } + if !strings.Contains(err.Error(), item.field) { + t.Errorf("bounded feedback must identify %s: %v", item.field, err) + } + }) + } +} + +func TestInProcessErrorContractSuccessParity(t *testing.T) { + provider := NewInProcessServer() + for _, item := range []struct { + tool string + args map[string]any + }{ + {"list_module_types", nil}, + {"get_config_skeleton", map[string]any{"module_types": []string{"http.server"}}}, + {"validate_config", map[string]any{"yaml_content": inputContractYAML}}, + {"validate_config", map[string]any{"yaml_content": errorContractYAML}}, + {"compat_check", map[string]any{"yaml_content": errorContractYAML}}, + } { + t.Run(item.tool+fmt.Sprint(item.args), func(t *testing.T) { + result, err := provider.CallTool(t.Context(), item.tool, item.args) + if err != nil { + t.Fatal(err) + } + text, ok := result.(string) + if !ok { + t.Fatalf("successful result must remain string, got %T", result) + } + wire := dispatchInputContract(t, NewServer(""), item.tool, item.args) + if wire.IsError || text != extractText(t, wire) { + t.Fatal("adapter must preserve successful dispatcher text, including analytical negative reports") + } + }) + } +} + +func TestInProcessErrorContractBoundedEngineFailure(t *testing.T) { + engine := &mockEngine{triggerErr: errors.New(strings.Repeat("synthetic雪", 2000))} + _, err := NewInProcessServer(WithInProcessEngine(engine)).CallTool(t.Context(), "run_workflow", map[string]any{"workflow_type": "http", "action": "public_test"}) + if err == nil { + t.Fatal("engine failure must return a Go error") + } + if len(err.Error()) > 4200 || !utf8.ValidString(err.Error()) { + t.Fatal("tool failure feedback must be bounded and valid UTF-8") + } +} + +func TestInProcessErrorContractAuditMetadataOnly(t *testing.T) { + const argsCanary = "SYNTHETIC_ARGUMENT_CANARY" + const engineCanary = "SYNTHETIC_ENGINE_ERROR_CANARY" + const handlerCanary = "SYNTHETIC_HANDLER_ERROR_CANARY" + const unknownCanary = "SYNTHETIC_UNKNOWN_TOOL_CANARY" + var log bytes.Buffer + provider := NewInProcessServer(WithInProcessEngine(&mockEngine{triggerErr: errors.New(engineCanary)}), WithInProcessAuditLog(slog.New(slog.NewJSONHandler(&log, nil)))) + _, _ = provider.CallTool(t.Context(), "run_workflow", map[string]any{"workflow_type": "http", "action": "public_test", "data": map[string]any{"secret": argsCanary}}) + _, _ = provider.CallTool(t.Context(), "validate_config", map[string]any{"yaml_content": argsCanary, "strict": true}) + _, err := provider.CallTool(t.Context(), strings.Repeat(unknownCanary, 500), nil) + if err == nil || len(err.Error()) > 4200 { + t.Error("unknown-tool feedback must be a bounded error") + } + _, err = provider.CallTool(t.Context(), "validate_config", map[string]any{"yaml_content": make(chan int)}) + if err == nil { + t.Error("non-JSON arguments must return an error") + } + // Dependency double at the existing handler seam; no new production tool. + registered := provider.server.MCPServer().ListTools()["list_module_types"] + handler := func(context.Context, mcp.CallToolRequest) (*mcp.CallToolResult, error) { + return nil, errors.New(handlerCanary) + } + provider.tools["list_module_types"] = handler + provider.server.MCPServer().AddTool(registered.Tool, handler) + _, err = provider.CallTool(t.Context(), "list_module_types", nil) + if err == nil { + t.Error("SDK protocol error must become a Go error") + } + for _, canary := range []string{argsCanary, engineCanary, handlerCanary, unknownCanary} { + if strings.Contains(log.String(), canary) { + t.Errorf("audit exposed %s", canary) + } + } + for _, line := range bytes.Split(bytes.TrimSpace(log.Bytes()), []byte("\n")) { + var entry map[string]any + if err := json.Unmarshal(line, &entry); err != nil { + t.Fatal(err) + } + for key := range entry { + if key != "time" && key != "level" && key != "msg" && key != "tool" && key != "status" { + t.Errorf("audit must contain metadata only, found %q", key) + } + } + if _, ok := entry["status"].(string); !ok { + t.Error("audit missing bounded status metadata") + } + if name, ok := entry["tool"].(string); ok { + if _, known := provider.GetToolSchema(name); !known { + t.Error("audit exposed unknown user-controlled tool name") + } + } + } +} + +func TestInProcessErrorContractRequestIDs(t *testing.T) { + provider := NewInProcessServer() + var mu sync.Mutex + var ids []string + hooks := provider.server.MCPServer().GetHooks() + if hooks == nil { + hooks = &server.Hooks{} + server.WithHooks(hooks)(provider.server.MCPServer()) + } + hooks.AddBeforeCallTool(func(_ context.Context, id any, _ *mcp.CallToolRequest) { + mu.Lock() + defer mu.Unlock() + ids = append(ids, fmt.Sprint(id)) + }) + for range 3 { + if _, err := provider.CallTool(t.Context(), "list_module_types", nil); err != nil { + t.Fatal(err) + } + } + var calls sync.WaitGroup + for range 40 { + calls.Go(func() { + if _, err := provider.CallTool(t.Context(), "list_module_types", nil); err != nil { + t.Error(err) + } + }) + } + calls.Wait() + if len(ids) != 43 { + t.Fatalf("all calls must cross SDK dispatcher, observed %d", len(ids)) + } + seen := map[string]bool{} + for _, id := range ids { + value, err := strconv.ParseUint(id, 10, 64) + if err != nil || value == 0 || value > 43 || seen[id] { + t.Fatalf("request ID must be unique and monotonic per instance: %q", id) + } + seen[id] = true + } + if ids[0] != "1" || ids[1] != "2" || ids[2] != "3" { + t.Fatalf("sequential request IDs are not monotonic: %v", ids[:3]) + } +} + +// Controlled fake engine: it cannot start a real workflow. The entered and +// cancelled barriers prove cancellation reaches a currently blocked handler. +type errorContractEngine struct { + mockEngine + entered chan struct{} + cancelled chan struct{} + successes atomic.Int32 +} + +func newErrorContractEngine() *errorContractEngine { + return &errorContractEngine{entered: make(chan struct{}), cancelled: make(chan struct{})} +} +func (e *errorContractEngine) TriggerWorkflow(ctx context.Context, _ string, action string, _ map[string]any) error { + if action == "block_until_cancelled" { + close(e.entered) + <-ctx.Done() + close(e.cancelled) + return ctx.Err() + } + e.successes.Add(1) + return nil +} +func awaitErrorContract(t *testing.T, signal <-chan struct{}, what string) { + t.Helper() + select { + case <-signal: + case <-time.After(5 * time.Second): + t.Fatalf("timed out waiting for %s", what) + } +} + +func TestInProcessErrorContractCancellation(t *testing.T) { + engine := newErrorContractEngine() + provider := NewInProcessServer(WithInProcessEngine(engine)) + ctx, cancel := context.WithTimeout(t.Context(), 5*time.Second) + defer cancel() + completed := make(chan error, 1) + go func() { + _, err := provider.CallTool(ctx, "run_workflow", map[string]any{"workflow_type": "http", "action": "block_until_cancelled"}) + completed <- err + }() + awaitErrorContract(t, engine.entered, "provider handler entry") + cancel() + awaitErrorContract(t, engine.cancelled, "provider ctx.Done") + select { + case err := <-completed: + if err == nil { + t.Error("cancelled engine invocation must return a Go error") + } + case <-time.After(5 * time.Second): + t.Fatal("cancelled provider call did not complete") + } + if engine.successes.Load() != 0 { + t.Fatal("cancelled operation recorded post-cancel success") + } + if _, err := provider.CallTool(t.Context(), "run_workflow", map[string]any{"workflow_type": "http", "action": "public_success"}); err != nil { + t.Fatal(err) + } + if engine.successes.Load() != 1 { + t.Fatal("subsequent successful request did not execute") + } +} + +func TestMCPErrorContractCancellationNotification(t *testing.T) { + engine := newErrorContractEngine() + srv := NewServer("", WithEngine(engine)) + ctx, cancel := context.WithTimeout(t.Context(), 5*time.Second) + defer cancel() + const requestID = "public-cancellation-request" + request, err := json.Marshal(map[string]any{"jsonrpc": "2.0", "id": requestID, "method": "tools/call", "params": map[string]any{"name": "run_workflow", "arguments": map[string]any{"workflow_type": "http", "action": "block_until_cancelled"}}}) + if err != nil { + t.Fatal(err) + } + completed := make(chan mcp.JSONRPCMessage, 1) + go func() { completed <- srv.MCPServer().HandleMessage(ctx, request) }() + awaitErrorContract(t, engine.entered, "dispatcher handler entry") + notification := []byte(`{"jsonrpc":"2.0","method":"notifications/cancelled","params":{"requestId":"public-cancellation-request"}}`) + if response := srv.MCPServer().HandleMessage(t.Context(), notification); response != nil { + t.Error("cancellation notification must not produce a response") + } + awaitErrorContract(t, engine.cancelled, "dispatcher ctx.Done") + if ctx.Err() != nil { + t.Fatal("matching notification must cancel the handler before the parent operation deadline") + } + select { + case response := <-completed: + data, err := json.Marshal(response) + if err != nil { + t.Fatal(err) + } + var reply struct{ Result *mcp.CallToolResult } + if err = json.Unmarshal(data, &reply); err != nil { + t.Fatal(err) + } + if reply.Result == nil || !reply.Result.IsError || !strings.Contains(extractText(t, reply.Result), `"success": false`) { + t.Fatal("cancelled engine invocation must retain error semantics") + } + case <-time.After(5 * time.Second): + t.Fatal("cancelled dispatcher call did not complete") + } + if engine.successes.Load() != 0 { + t.Fatal("cancelled operation recorded post-cancel success") + } + result := dispatchInputContract(t, srv, "run_workflow", map[string]any{"workflow_type": "http", "action": "public_success"}) + if result.IsError || engine.successes.Load() != 1 { + t.Fatal("dispatcher did not recover for a subsequent successful request") + } +} diff --git a/mcp/library.go b/mcp/library.go index f65a20c9..89bde129 100644 --- a/mcp/library.go +++ b/mcp/library.go @@ -3,8 +3,13 @@ package mcp import ( "context" "encoding/json" + "errors" "fmt" "log/slog" + "sort" + "strconv" + "sync/atomic" + "unicode/utf8" "github.com/mark3labs/mcp-go/mcp" ) @@ -62,6 +67,7 @@ type InProcessServer struct { tools map[string]ToolHandlerFunc schemas map[string]ToolSchema auditLogger *slog.Logger + requestID atomic.Uint64 } // NewInProcessServer creates an InProcessServer with all workflow tools registered. @@ -118,6 +124,7 @@ func (p *InProcessServer) ListTools() []string { for name := range p.tools { names = append(names, name) } + sort.Strings(names) return names } @@ -127,6 +134,7 @@ func (p *InProcessServer) ListToolSchemas() []ToolSchema { for _, s := range p.schemas { result = append(result, s) } + sort.Slice(result, func(i, j int) bool { return result[i].Name < result[j].Name }) return result } @@ -140,38 +148,82 @@ func (p *InProcessServer) GetToolSchema(name string) (ToolSchema, bool) { // Returns the tool result, which may be of any type, or an error if the // tool is not found or invocation fails. func (p *InProcessServer) CallTool(ctx context.Context, name string, args map[string]any) (any, error) { - handler, ok := p.tools[name] - if !ok { - return nil, fmt.Errorf("unknown tool: %s", name) + _, known := p.schemas[name] + status := "protocol_error" + defer func() { + if p.auditLogger != nil { + attributes := []any{"status", status} + if known { + attributes = append(attributes, "tool", name) + } + p.auditLogger.Info("mcp tool call", attributes...) + } + }() + + // String IDs avoid JSON number precision loss while preserving per-instance + // uniqueness across concurrent calls and SDK cancellation registration. + id := strconv.FormatUint(p.requestID.Add(1), 10) + request, err := json.Marshal(map[string]any{ + "jsonrpc": "2.0", "id": id, "method": "tools/call", + "params": map[string]any{"name": name, "arguments": args}, + }) + if err != nil { + status = "invalid_arguments" + return nil, errors.New("MCP arguments must be JSON-serializable") } - - argsJSON, err := json.Marshal(args) + // Use the same SDK dispatcher as stdio, including schema validation and + // protocol/tool error semantics, rather than calling a raw handler. + encoded, err := json.Marshal(p.server.MCPServer().HandleMessage(ctx, request)) if err != nil { - return nil, fmt.Errorf("marshal args: %w", err) + return nil, errors.New("MCP response could not be encoded") } - - var req mcp.CallToolRequest - req.Params.Name = name - req.Params.Arguments = make(map[string]any) - if err := json.Unmarshal(argsJSON, &req.Params.Arguments); err != nil { - return nil, fmt.Errorf("unmarshal args: %w", err) + var reply struct { + JSONRPC string `json:"jsonrpc"` + ID string `json:"id"` + Result *mcp.CallToolResult `json:"result"` + Error *mcp.JSONRPCErrorDetails `json:"error"` } - - result, callErr := handler(ctx, req) - if p.auditLogger != nil { - p.auditLogger.Info("mcp tool call", - "tool", name, - "error", callErr, - ) + if err = json.Unmarshal(encoded, &reply); err != nil || reply.JSONRPC != "2.0" || reply.ID != id { + return nil, errors.New("invalid MCP response") } - if callErr != nil { - return nil, callErr + if reply.Error != nil { + if !known { + status = "unknown_tool" + } + // Protocol messages can contain user names or raw handler errors. The + // error code supplies bounded feedback without echoing those details. + return nil, fmt.Errorf("MCP protocol error (%d)", reply.Error.Code) } - - for _, c := range result.Content { + if reply.Result == nil { + return nil, errors.New("missing MCP tool result") + } + if reply.Result.IsError { + status = "tool_error" + for _, content := range reply.Result.Content { + if text, ok := content.(mcp.TextContent); ok { + return nil, fmt.Errorf("MCP tool execution failed: %s", boundedToolFeedback(text.Text)) + } + } + return nil, errors.New("MCP tool execution failed") + } + status = "ok" + for _, c := range reply.Result.Content { if tc, ok := c.(mcp.TextContent); ok { return tc.Text, nil } } - return result, nil + return reply.Result, nil +} + +// Bound failure feedback only; successful tool text remains unchanged. +func boundedToolFeedback(text string) string { + const limit = 4096 + if len(text) <= limit { + return text + } + end := limit + for !utf8.ValidString(text[:end]) { + end-- + } + return text[:end] + "…" } diff --git a/mcp/library_test.go b/mcp/library_test.go index be3155b4..80041d27 100644 --- a/mcp/library_test.go +++ b/mcp/library_test.go @@ -2,6 +2,7 @@ package mcp import ( "context" + "slices" "strings" "testing" ) @@ -33,6 +34,26 @@ func TestNewInProcessServer(t *testing.T) { } } +func TestInProcessInventoryOrder(t *testing.T) { + provider := NewInProcessServer() + want := provider.ListTools() + slices.Sort(want) + for range 20 { + names := provider.ListTools() + if !slices.Equal(names, want) { + t.Errorf("tool names must be deterministically sorted") + } + schemas := provider.ListToolSchemas() + schemaNames := make([]string, len(schemas)) + for i, schema := range schemas { + schemaNames[i] = schema.Name + } + if !slices.Equal(schemaNames, want) { + t.Errorf("tool schemas must have the same sorted order") + } + } +} + func TestInProcessToolInvocation(t *testing.T) { s := NewInProcessServer() ctx := context.Background() diff --git a/mcp/server.go b/mcp/server.go index 2d1edaeb..b8fc5334 100644 --- a/mcp/server.go +++ b/mcp/server.go @@ -643,7 +643,9 @@ func (s *Server) handleRunWorkflow(ctx context.Context, req mcp.CallToolRequest) "success": false, "error": err.Error(), } - return marshalToolResult(result) + failureResult, marshalErr := marshalToolResult(result) + failureResult.IsError = true + return failureResult, marshalErr } result := map[string]any{ diff --git a/mcp/server_test.go b/mcp/server_test.go index ecd1cf1c..589f9ae5 100644 --- a/mcp/server_test.go +++ b/mcp/server_test.go @@ -940,6 +940,9 @@ func TestRunWorkflow_Error(t *testing.T) { if err != nil { t.Fatalf("unexpected error: %v", err) } + if !result.IsError { + t.Error("engine trigger failure must be a tool error") + } text := extractText(t, result) var data map[string]any From 27dca20800f5b45373eeefeb53d6ade4eb1c6ba0 Mon Sep 17 00:00:00 2001 From: Jon Langevin Date: Mon, 19 Oct 2026 20:10:48 -0400 Subject: [PATCH 4/5] fix(mcp): audit annotations and verify legacy stdio contracts --- cmd/wfctl/mcp_stdio_test.go | 522 ++++++++++++++++ .../0066-shared-mcp-contracts-and-protocol.md | 44 ++ docs/mcp-tools-reference.md | 578 +++++------------- docs/mcp.md | 304 +++++---- mcp/annotations_test.go | 81 +++ mcp/scaffold_tools.go | 7 + mcp/server.go | 25 +- mcp/tools.go | 11 +- mcp/wfctl_tools.go | 22 +- 9 files changed, 982 insertions(+), 612 deletions(-) create mode 100644 cmd/wfctl/mcp_stdio_test.go create mode 100644 decisions/0066-shared-mcp-contracts-and-protocol.md create mode 100644 mcp/annotations_test.go diff --git a/cmd/wfctl/mcp_stdio_test.go b/cmd/wfctl/mcp_stdio_test.go new file mode 100644 index 00000000..2a14073a --- /dev/null +++ b/cmd/wfctl/mcp_stdio_test.go @@ -0,0 +1,522 @@ +package main + +import ( + "bufio" + "context" + "encoding/json" + "errors" + "fmt" + "io" + "os" + "os/exec" + "path/filepath" + "runtime" + "strings" + "sync" + "syscall" + "testing" + "time" + + "github.com/GoCodeAlone/workflow/config" + workflowmcp "github.com/GoCodeAlone/workflow/mcp" +) + +const stdioDeadline = 5 * time.Second +const stdioPublicYAML = "modules:\n - name: public_web\n type: http.server\n config:\n address: ':8080'\n" +const stdioCanary = "SYNTHETIC_STDIO_PRIVATE_CANARY" +const handlerEntered = "WFCTL_MCP_TEST_HANDLER_ENTERED" +const handlerCancelled = "WFCTL_MCP_TEST_HANDLER_CANCELLED" + +// This dependency double exercises Workflow's existing optional engine tool +// through the real SDK transport; it cannot execute application workflows. +type stdioBlockingEngine struct{} + +func (*stdioBlockingEngine) BuildFromConfig(*config.WorkflowConfig) error { return nil } +func (*stdioBlockingEngine) Start(context.Context) error { return nil } +func (*stdioBlockingEngine) Stop(context.Context) error { return nil } +func (*stdioBlockingEngine) TriggerWorkflow(ctx context.Context, _ string, action string, _ map[string]any) error { + if action == "block_until_cancelled" { + fmt.Fprintln(os.Stderr, handlerEntered) + <-ctx.Done() + fmt.Fprintln(os.Stderr, handlerCancelled) + return ctx.Err() + } + return nil +} + +// A separate process prevents test-framework output from entering MCP stdout. +// Production mode calls runMCP. Transport mode is always this test executable, +// even when WFCTL_MCP_TEST_BINARY selects a real CLI for production-mode tests. +func TestMCPStdioLegacyHelperProcess(t *testing.T) { + mode := os.Getenv("WFCTL_MCP_TEST_HELPER") + if mode == "" { + return + } + var err error + switch mode { + case "transport": + err = workflowmcp.NewServer("plugins", workflowmcp.WithEngine(&stdioBlockingEngine{})).ServeStdio() + case "production": + err = runMCP([]string{"-plugin-dir", "plugins", "-documentation-file", "public-reference.md"}) + default: + os.Exit(2) + } + if err != nil { + if errors.Is(err, context.Canceled) { + fmt.Fprintln(os.Stderr, "WFCTL_MCP_TEST_CONTEXT_CANCELLED_EXIT") + } else { + fmt.Fprintln(os.Stderr, "WFCTL_MCP_TEST_SERVE_FAILED") + } + os.Exit(1) + } + os.Exit(0) +} + +type stdioReply struct { + JSONRPC string `json:"jsonrpc"` + ID json.RawMessage `json:"id"` + Result json.RawMessage `json:"result"` + Error *struct { + Code int `json:"code"` + Message string `json:"message"` + } `json:"error"` +} + +type stdioFrame struct { + reply stdioReply + err error +} + +type stdioContractClient struct { + cmd *exec.Cmd + stdin io.WriteCloser + frames chan stdioFrame + barriers chan string + done chan struct{} + readersDone chan struct{} + stopReaders chan struct{} + writers sync.WaitGroup + waitErr error // Published by closing done. + stderrMu sync.Mutex + stderr strings.Builder + stderrErr bool + next int +} + +func startStdioContract(t *testing.T, transport bool) *stdioContractClient { + t.Helper() + root := t.TempDir() + for _, dir := range []string{"plugins", "tmp"} { + if err := os.Mkdir(filepath.Join(root, dir), 0o700); err != nil { + t.Fatal(err) + } + } + if err := os.WriteFile(filepath.Join(root, "public-reference.md"), []byte("# Public synthetic reference\n"), 0o600); err != nil { + t.Fatal(err) + } + bin, err := os.Executable() + if err != nil { + t.Fatal(err) + } + mode := "production" + args := []string{"-test.run=^TestMCPStdioLegacyHelperProcess$"} + if transport { + mode = "transport" + } else if override := os.Getenv("WFCTL_MCP_TEST_BINARY"); override != "" { + bin = override + args = []string{"mcp", "-plugin-dir", "plugins", "-documentation-file", "public-reference.md"} + } + c := &stdioContractClient{frames: make(chan stdioFrame, 32), barriers: make(chan string, 8), done: make(chan struct{}), readersDone: make(chan struct{}), stopReaders: make(chan struct{})} + c.cmd = exec.Command(bin, args...) //nolint:gosec // Explicit test-owned executable, never user tool input. + c.cmd.Dir = root + // No ambient HOME, CODEX_HOME, plugin locations, credentials or app config. + c.cmd.Env = []string{"PATH=/usr/bin:/bin", "TMPDIR=" + filepath.Join(root, "tmp"), "WFCTL_NO_UPDATE_CHECK=1", "WFCTL_DIFFCACHE=disabled", "WFCTL_MCP_TEST_HELPER=" + mode} + c.stdin, err = c.cmd.StdinPipe() + if err != nil { + t.Fatal(err) + } + outR, outW, err := os.Pipe() + if err != nil { + _ = c.stdin.Close() + t.Fatal(err) + } + errR, errW, err := os.Pipe() + if err != nil { + _ = c.stdin.Close() + _ = outR.Close() + _ = outW.Close() + t.Fatal(err) + } + c.cmd.Stdout, c.cmd.Stderr = outW, errW + if err := c.cmd.Start(); err != nil { + _ = c.stdin.Close() + _ = outR.Close() + _ = outW.Close() + _ = errR.Close() + _ = errW.Close() + t.Fatal(err) + } + _ = outW.Close() + _ = errW.Close() + var readers sync.WaitGroup + readers.Add(2) + publish := func(frame stdioFrame) bool { + select { + case c.frames <- frame: + return true + case <-c.stopReaders: + return false + } + } + go func() { + defer readers.Done() + defer outR.Close() + defer close(c.frames) + scan := bufio.NewScanner(outR) + scan.Buffer(make([]byte, 4096), 4<<20) + for scan.Scan() { + var reply stdioReply + err := json.Unmarshal(scan.Bytes(), &reply) + if err != nil || reply.JSONRPC != "2.0" || len(reply.ID) == 0 { + if !publish(stdioFrame{err: fmt.Errorf("stdout contained non-response JSON-RPC framing")}) { + return + } + continue + } + if !publish(stdioFrame{reply: reply}) { + return + } + } + if scan.Err() != nil { + publish(stdioFrame{err: fmt.Errorf("stdout reader failed")}) + } + }() + go func() { + defer readers.Done() + defer errR.Close() + scan := bufio.NewScanner(errR) + scan.Buffer(make([]byte, 4096), 4<<20) + for scan.Scan() { + line := scan.Text() + c.stderrMu.Lock() + c.stderr.WriteString(line + "\n") + c.stderrMu.Unlock() + if line == handlerEntered || line == handlerCancelled { + select { + case c.barriers <- line: + case <-c.stopReaders: + return + } + } + } + if scan.Err() != nil { + c.stderrMu.Lock() + c.stderrErr = true + c.stderrMu.Unlock() + } + }() + go func() { readers.Wait(); close(c.readersDone) }() + go func() { c.waitErr = c.cmd.Wait(); close(c.done) }() + t.Cleanup(func() { + close(c.stopReaders) + _ = c.stdin.Close() + select { + case <-c.done: + default: + _ = c.cmd.Process.Kill() + } + c.writers.Wait() // Closing stdin releases any timed-out write. + select { + case <-c.done: + case <-time.After(stdioDeadline): + t.Error("child was not reaped") + } + select { + case <-c.readersDone: + case <-time.After(stdioDeadline): + t.Error("stdio readers did not finish") + } + c.stderrMu.Lock() + defer c.stderrMu.Unlock() + if strings.Contains(c.stderr.String(), stdioCanary) { + t.Error("raw synthetic canary leaked to stderr") + } + if c.stderrErr { + t.Error("stderr reader failed; canary audit incomplete") + } + }) + return c +} + +func (c *stdioContractClient) send(t *testing.T, message any) { + t.Helper() + data, err := json.Marshal(message) + if err != nil { + t.Fatal(err) + } + c.sendRaw(t, string(data)) +} + +func (c *stdioContractClient) sendRaw(t *testing.T, line string) { + t.Helper() + written := make(chan error, 1) + c.writers.Add(1) + go func() { defer c.writers.Done(); _, err := io.WriteString(c.stdin, line+"\n"); written <- err }() + select { + case err := <-written: + if err != nil { + t.Fatal(err) + } + case <-time.After(stdioDeadline): + _ = c.cmd.Process.Kill() + t.Fatal("stdio write timed out") + } +} + +func (c *stdioContractClient) receive(t *testing.T, id string) stdioReply { + t.Helper() + select { + case frame, ok := <-c.frames: + if !ok { + t.Fatal("stdout closed before response") + } + if frame.err != nil { + t.Fatal(frame.err) + } + if string(frame.reply.ID) != id { + t.Fatalf("unexpected response ID: got %s, want %s", frame.reply.ID, id) + } + return frame.reply + case <-time.After(stdioDeadline): + t.Fatal("stdio response timed out") + } + return stdioReply{} +} + +func (c *stdioContractClient) request(t *testing.T, method string, params any) stdioReply { + t.Helper() + c.next++ + c.send(t, map[string]any{"jsonrpc": "2.0", "id": c.next, "method": method, "params": params}) + return c.receive(t, fmt.Sprint(c.next)) +} + +func (c *stdioContractClient) initialize(t *testing.T, version string) { + t.Helper() + reply := c.request(t, "initialize", map[string]any{"protocolVersion": version, "capabilities": map[string]any{}, "clientInfo": map[string]any{"name": "public-contract-test", "version": "1"}}) + var result struct { + ProtocolVersion string `json:"protocolVersion"` + ServerInfo struct{ Name, Version string } `json:"serverInfo"` + } + if reply.Error != nil || json.Unmarshal(reply.Result, &result) != nil || result.ProtocolVersion != version || result.ServerInfo.Name != "wfctl-mcp" || result.ServerInfo.Version == "" { + t.Fatal("initialize failed to negotiate requested legacy version and server identity") + } + c.send(t, map[string]any{"jsonrpc": "2.0", "method": "notifications/initialized"}) +} + +func (c *stdioContractClient) call(t *testing.T, name string, args map[string]any, wantError bool) string { + t.Helper() + reply := c.request(t, "tools/call", map[string]any{"name": name, "arguments": args}) + return assertStdioTool(t, reply, wantError) +} + +func assertStdioTool(t *testing.T, reply stdioReply, wantError bool) string { + t.Helper() + var result struct { + IsError bool `json:"isError"` + Content []struct{ Type, Text string } `json:"content"` + } + if reply.Error != nil || json.Unmarshal(reply.Result, &result) != nil || result.IsError != wantError || len(result.Content) != 1 || result.Content[0].Type != "text" { + t.Fatal("unexpected tool error/content contract") + } + return result.Content[0].Text +} + +func (c *stdioContractClient) inventory(t *testing.T, engine bool) { + t.Helper() + reply := c.request(t, "tools/list", map[string]any{}) + var tools struct { + Tools []struct{ Name string } `json:"tools"` + } + if reply.Error != nil || json.Unmarshal(reply.Result, &tools) != nil { + t.Fatal("tools/list failed") + } + want := 33 + if engine { + want++ + } + if len(tools.Tools) != want { + t.Fatalf("got %d tools, want %d", len(tools.Tools), want) + } + seen := map[string]bool{} + for _, tool := range tools.Tools { + if seen[tool.Name] { + t.Fatal("duplicate tool") + } + seen[tool.Name] = true + } + if seen["run_workflow"] != engine { + t.Fatal("conditional engine tool visibility mismatch") + } + reply = c.request(t, "resources/list", map[string]any{}) + var resources struct { + Resources []struct { + URI string `json:"uri"` + } `json:"resources"` + } + if reply.Error != nil || json.Unmarshal(reply.Result, &resources) != nil || len(resources.Resources) != 5 { + t.Fatal("expected five resources") + } + wantResources := map[string]bool{"workflow://docs/overview": true, "workflow://docs/yaml-syntax": true, "workflow://docs/module-reference": true, "workflow://docs/full-reference": true, "workflow://docs/setup-guide": true} + for _, resource := range resources.Resources { + if !wantResources[resource.URI] { + t.Fatal("unknown or duplicate resource URI") + } + delete(wantResources, resource.URI) + } +} + +func (c *stdioContractClient) finish(t *testing.T, terminate bool) { + t.Helper() + if terminate { + if err := c.cmd.Process.Signal(syscall.SIGTERM); err != nil { + t.Fatal(err) + } + } else if err := c.stdin.Close(); err != nil { + t.Fatal(err) + } + select { + case <-c.done: + case <-time.After(stdioDeadline): + t.Fatal("MCP child did not exit within deadline") + } + select { + case <-c.readersDone: + case <-time.After(stdioDeadline): + t.Fatal("reader join timed out") + } + if terminate { + // v0.54.1 ServeStdio returns context.Canceled on SIGTERM; both + // runMCP's caller and the real CLI currently report this as exit 1. + var exitErr *exec.ExitError + if !errors.As(c.waitErr, &exitErr) || exitErr.ExitCode() != 1 { + t.Fatalf("expected bounded SIGTERM context-cancel exit 1, got %v", c.waitErr) + } + c.stderrMu.Lock() + cancelled := strings.Contains(c.stderr.String(), "WFCTL_MCP_TEST_CONTEXT_CANCELLED_EXIT") || strings.Contains(c.stderr.String(), context.Canceled.Error()) + c.stderrMu.Unlock() + if !cancelled { + t.Fatal("SIGTERM exit lacked context-cancellation evidence") + } + } else if c.waitErr != nil { + t.Fatalf("EOF child exit: %v", c.waitErr) + } + for frame := range c.frames { + if frame.err != nil { + t.Error(frame.err) + } else { + t.Error("unsolicited/extra stdout response (including notification reply)") + } + } +} + +func TestMCPStdioLegacy(t *testing.T) { + for _, version := range []string{"2024-11-05", "2025-03-26", "2025-06-18", "2025-11-25"} { + t.Run(version, func(t *testing.T) { + c := startStdioContract(t, false) + c.initialize(t, version) + c.inventory(t, false) + var report struct { + Valid bool `json:"valid"` + } + text := c.call(t, "validate_config", map[string]any{"yaml_content": stdioPublicYAML}, false) + if json.Unmarshal([]byte(text), &report) != nil || !report.Valid { + t.Fatal("valid public config rejected") + } + c.call(t, "validate_config", map[string]any{}, true) + c.call(t, "validate_config", map[string]any{"yaml_content": stdioCanary, "strict": "false"}, true) + c.call(t, "get_config_skeleton", map[string]any{"module_types": []any{"http.server", 73}}, true) + text = c.call(t, "get_config_skeleton", map[string]any{"module_types": []string{"http.server"}}, false) + if !strings.Contains(text, "http.server") { + t.Fatal("typed skeleton missing public type") + } + text = c.call(t, "validate_config", map[string]any{"yaml_content": "modules:\n - name: absent\n type: public.uninstalled\n"}, false) + if json.Unmarshal([]byte(text), &report) != nil || report.Valid { + t.Fatal("analytical negative must remain successful valid:false report") + } + c.sendRaw(t, "{") + if reply := c.receive(t, "null"); reply.Error == nil || reply.Error.Code != -32700 { + t.Fatal("malformed JSON must return parse error") + } + // This proves notification robustness only: production tools may finish + // before cancellation arrives. In-flight proof uses the helper below. + c.next++ + id := c.next + c.send(t, map[string]any{"jsonrpc": "2.0", "id": id, "method": "tools/call", "params": map[string]any{"name": "list_module_types", "arguments": map[string]any{}}}) + c.send(t, map[string]any{"jsonrpc": "2.0", "method": "notifications/cancelled", "params": map[string]any{"requestId": id}}) + reply := c.receive(t, fmt.Sprint(id)) + if reply.Error != nil { + if reply.Error.Code != -32800 || !strings.Contains(strings.ToLower(reply.Error.Message), "cancel") { + t.Fatal("unexpected cancellation protocol error") + } + } else { + var result struct { + IsError bool `json:"isError"` + } + if json.Unmarshal(reply.Result, &result) != nil { + t.Fatal("invalid cancellation result") + } + text := assertStdioTool(t, reply, result.IsError) + if result.IsError && !strings.Contains(strings.ToLower(text), "cancel") { + t.Fatal("unexpected cancellation tool error") + } + } + c.call(t, "list_module_types", map[string]any{}, false) + c.finish(t, false) + }) + } +} + +func TestMCPStdioLegacyCooperativeCancellation(t *testing.T) { + c := startStdioContract(t, true) + c.initialize(t, "2025-11-25") + c.inventory(t, true) + c.next++ + id := c.next + c.send(t, map[string]any{"jsonrpc": "2.0", "id": id, "method": "tools/call", "params": map[string]any{"name": "run_workflow", "arguments": map[string]any{"workflow_type": "public", "action": "block_until_cancelled", "data": map[string]any{"synthetic": stdioCanary}}}}) + for _, barrier := range []string{handlerEntered, handlerCancelled} { + select { + case actual := <-c.barriers: + if actual != barrier { + t.Fatal("unexpected handler barrier") + } + case <-time.After(stdioDeadline): + t.Fatalf("timed out waiting for %s", barrier) + } + if barrier == handlerEntered { + c.send(t, map[string]any{"jsonrpc": "2.0", "method": "notifications/cancelled", "params": map[string]any{"requestId": id}}) + } + } + text := assertStdioTool(t, c.receive(t, fmt.Sprint(id)), true) + var result struct { + Success *bool `json:"success"` + Error string `json:"error"` + } + if json.Unmarshal([]byte(text), &result) != nil || result.Success == nil || *result.Success || result.Error != context.Canceled.Error() { + t.Fatal("cancelled handler must not report success") + } + text = c.call(t, "run_workflow", map[string]any{"workflow_type": "public", "action": "public_success"}, false) + if json.Unmarshal([]byte(text), &result) != nil || result.Success == nil || !*result.Success { + t.Fatal("subsequent engine request must succeed") + } + c.finish(t, false) +} + +func TestMCPStdioLegacySIGTERM(t *testing.T) { + if runtime.GOOS == "windows" { + t.Skip("os.Process.Signal(SIGTERM) is unsupported on Windows; framing and EOF are tested separately") + } + c := startStdioContract(t, false) + c.initialize(t, "2025-11-25") + c.call(t, "list_module_types", map[string]any{}, false) + c.finish(t, true) +} diff --git a/decisions/0066-shared-mcp-contracts-and-protocol.md b/decisions/0066-shared-mcp-contracts-and-protocol.md new file mode 100644 index 00000000..0e3cf9f9 --- /dev/null +++ b/decisions/0066-shared-mcp-contracts-and-protocol.md @@ -0,0 +1,44 @@ +# 0066. Correct MCP contracts before upgrading the protocol SDK + +**Status:** Accepted +**Date:** 2026-10-20 +**Related:** [Design](../docs/plans/2026-10-19-shared-mcp-design.md), +[locked two-PR/six-task plan](../docs/plans/2026-10-19-shared-mcp.md), +[independent review](../docs/plans/2026-10-19-shared-mcp-review.md). + +## Context + +Workflow already serves local MCP through wfctl and mcp-go v0.54.1. Legacy +clients initialize successfully, but permissive tool inputs, discarded adapter +errors, engine failures lacking isError, and SDK-default effect annotations +weaken the advertised contract. Catalog validation is not runtime acceptance. +The assistant packaging work is independent and must remain unchanged. + +## Decision + +Keep two separately reviewable PRs. First enforce existing input schemas and +string arrays, preserve protocol/tool errors through the in-process adapter, +mark engine failures as errors, sort inventories, and describe actual effects +with explicit annotations. Preserve successful text outputs and analytical +negative reports. Reuse the existing SDK dispatcher and stdio transport rather +than introducing another server or protocol implementation. + +Prove the four legacy revisions through a newly built CLI. Prove cooperative +cancellation separately with an entered blocking engine-provider test double +through real SDK stdio. Current SIGTERM cancellation exits 1 and must terminate +within the test deadline. The second PR upgrades the existing SDK and separately +proves July 2026 plus legacy behavior; this PR makes no July support claim. + +## Consequences + +Contract corrections can reject previously accepted malformed calls; valid +payloads retain their behavior. Read-only previews include modernize fixes +returned as text. Path/import readers remain open-world, and engine embedders +own authorization. Tests use public fixtures, minimal environments and isolated +processes; they neither execute a real engine workflow nor certify a host. + +A combined upgrade was rejected because contract regressions and protocol +regressions need independent review and rollback. A new framework, hosted +transport, tasks/auth/UI, structured output migration and packaging changes +remain outside scope. Revert each PR independently and rebuild to roll back; +no installed session or persistent state migration is involved. diff --git a/docs/mcp-tools-reference.md b/docs/mcp-tools-reference.md index 9668f3e5..35c589b8 100644 --- a/docs/mcp-tools-reference.md +++ b/docs/mcp-tools-reference.md @@ -1,395 +1,148 @@ # MCP Tools Reference -Complete reference for all MCP tools available through `wfctl mcp` and the -in-process MCP library. Tools are organized by namespace. - -## wfctl Tools - -### Config Validation - -#### `validate_config` - -Validate a workflow YAML configuration string. - -| Parameter | Type | Required | Description | -|-----------|------|----------|-------------| -| `yaml_content` | string | yes | YAML config content to validate | -| `skip_unknown_types` | bool | no | Skip unknown module/step type errors | -| `strict` | bool | no | Fail on any warning | - -**Example:** -```json -{ - "tool": "validate_config", - "arguments": { - "yaml_content": "modules:\n - name: server\n type: http.server\n config:\n address: ':8080'\n" - } -} -``` - -**Returns:** Validation result with errors and warnings. - ---- - -#### `template_validate_config` - -Validate a workflow config that uses template expressions. - -| Parameter | Type | Required | Description | -|-----------|------|----------|-------------| -| `config` | string | yes | YAML config with template expressions | -| `context` | object | no | Template context variables for expression evaluation | - ---- - -#### `inspect_config` - -Inspect a config and get a structured summary. - -| Parameter | Type | Required | Description | -|-----------|------|----------|-------------| -| `yaml_content` | string | yes | YAML config content | - -**Returns:** Structured summary with module names/types, workflow definitions, pipeline triggers, step counts. - ---- - -#### `diff_configs` - -Compute a semantic diff between two workflow configs. - -| Parameter | Type | Required | Description | -|-----------|------|----------|-------------| -| `old_yaml` | string | yes | Base config YAML | -| `new_yaml` | string | yes | Proposed config YAML | - -**Returns:** Structured diff showing added/modified/removed modules, pipelines, and steps. - ---- - -#### `modernize` - -Detect and fix known YAML config anti-patterns. - -| Parameter | Type | Required | Description | -|-----------|------|----------|-------------| -| `config` | string | yes | YAML config to modernize | -| `dry_run` | bool | no | Report fixes without applying them | - -**Returns:** Modernized config with fix descriptions. - ---- - -#### `compat_check` - -Check config compatibility with a specific engine version. - -| Parameter | Type | Required | Description | -|-----------|------|----------|-------------| -| `config` | string | yes | YAML config to check | -| `target_version` | string | yes | Engine version to check against (e.g., `v0.5.0`) | - ---- - -### Type Discovery - -#### `list_module_types` - -List all available module types. - -| Parameter | Type | Required | Description | -|-----------|------|----------|-------------| -| `category` | string | no | Filter by category (e.g., `database`, `http`, `agent`) | -| `plugin` | string | no | Filter by plugin name | - ---- - -#### `list_step_types` - -List all pipeline step types. - -| Parameter | Type | Required | Description | -|-----------|------|----------|-------------| -| `category` | string | no | Filter by category | - ---- - -#### `list_trigger_types` - -List all trigger types. - ---- - -#### `list_workflow_types` - -List all workflow handler types. - ---- - -#### `list_plugins` - -List installed external plugins. - -| Parameter | Type | Required | Description | -|-----------|------|----------|-------------| -| `include_uninstalled` | bool | no | Include plugins from the registry that aren't installed | - ---- - -### Schema & Documentation - -#### `get_module_schema` - -Get the JSON Schema for a specific module type. - -| Parameter | Type | Required | Description | -|-----------|------|----------|-------------| -| `type` | string | yes | Module type (e.g., `http.server`, `storage.sqlite`) | - -**Example:** -```json -{"tool": "get_module_schema", "arguments": {"type": "agent.guardrails"}} -``` - ---- - -#### `get_step_schema` - -Get the JSON Schema for a specific step type. - -| Parameter | Type | Required | Description | -|-----------|------|----------|-------------| -| `type` | string | yes | Step type (e.g., `step.agent_execute`, `step.db_query`) | - ---- - -#### `get_config_skeleton` - -Generate a skeleton YAML config for given module types. - -| Parameter | Type | Required | Description | -|-----------|------|----------|-------------| -| `module_types` | array | yes | List of module types to include | -| `include_optional` | bool | no | Include optional config fields | - ---- - -#### `get_config_examples` - -Get example configs for a module or step type. - -| Parameter | Type | Required | Description | -|-----------|------|----------|-------------| -| `type` | string | yes | Module or step type | - ---- - -#### `get_module_schema` / `get_step_schema` / `get_template_functions` - -Get the template functions available in workflow expressions. - ---- - -### Code Generation & Scaffolding - -#### `generate_schema` - -Generate a JSON Schema for workflow config files. - ---- - -#### `scaffold_ci` - -Generate a `ci:` YAML section (build / test / deploy sub-sections) for a workflow config, tailored to the app type. - -| Parameter | Type | Required | Description | -|-----------|------|----------|-------------| -| `description` | string | yes | Short description of the application (e.g. 'Go API server with Postgres') | -| `yaml_content` | string | no | Existing workflow config to analyze (used to detect app type) | -| `binary_path` | string | no | Go binary entrypoint path (default: `./cmd/server`) | -| `environments` | array | no | Deployment environment names to include (e.g. `['staging', 'production']`) | - ---- - -#### `ci_plan` - -Analyze a workflow config and emit a platform-neutral `CIPlan` JSON via the config-derived cigen engine (deploy phases with per-phase scoped secrets, migrations spec, smoke test, plan-guard, warnings). Render it with `generate_github_actions` or `wfctl ci generate --from-plan`. - -| Parameter | Type | Required | Description | -|-----------|------|----------|-------------| -| `yaml_content` | string | yes | The YAML content of the workflow configuration | -| `phase_config_yaml` | string | no | YAML of a prerequisite phase config (creates a two-phase plan) | -| `wfctl_version` | string | no | wfctl version to pin in the plan (default: latest) | - ---- - -#### `generate_github_actions` - -Render CI/CD workflow YAML from a workflow config. The CI workflow (`files`/`ci_yaml`) is config-derived via the cigen engine (scoped secrets, migrations step, smoke job, plan-guard); also returns the `plan` (CIPlan) and legacy template-based `cd_yaml`/`release_yaml` (which use the `registry`/`platforms` inputs). - -| Parameter | Type | Required | Description | -|-----------|------|----------|-------------| -| `yaml_content` | string | yes | The YAML content of the workflow configuration | -| `registry` | string | no | Container registry for the legacy CD/release output (default: `ghcr.io`) | -| `platforms` | string | no | Build platforms for the legacy CD/release output (default: `linux/amd64,linux/arm64`) | -| `phase_config_yaml` | string | no | YAML of a prerequisite phase config (creates a two-phase plan) | -| `wfctl_version` | string | no | wfctl version to pin in the plan (default: latest) | - ---- - -#### `scaffold_environment` - -Generate an `environments:` YAML section (per-environment provider, env vars, secrets provider, exposure method) for a workflow config. - -| Parameter | Type | Required | Description | -|-----------|------|----------|-------------| -| `provider` | string | yes | Deployment provider: `docker`, `kubernetes`, `aws-ecs`, `gcp-cloudrun`, `digitalocean` | -| `environments` | array | no | Environment names to generate (default: `['local', 'staging', 'production']`) | -| `secrets_provider` | string | no | Secrets provider: `env`, `aws-secrets-manager`, `gcp-secret-manager`, `vault` (default: `env`) | -| `exposure` | string | no | Exposure method for the `local` environment: `tailscale`, `cloudflare`, `port-forward` (default: `port-forward`) | - ---- - -#### `scaffold_infra` - -Generate an `infra:` YAML section by analyzing the config's modules (e.g. `database.postgres` → RDS, `cache.redis` → ElastiCache). - -| Parameter | Type | Required | Description | -|-----------|------|----------|-------------| -| `yaml_content` | string | yes | Workflow YAML config content to analyze for infrastructure needs | -| `provider` | string | yes | Cloud provider for resources: `aws`, `gcp`, `azure`, `digitalocean` | -| `environment` | string | no | Environment name for resource sizing (default: `production`) | - ---- - -#### `generate_bootstrap` - -Generate a complete bootstrap config for a new workflow application. - ---- - -#### `contract_generate` - -Generate API contracts (OpenAPI spec) from a workflow config. - ---- - -### Introspection & Analysis - -#### `api_extract` - -Extract API endpoints from a workflow config. - -| Parameter | Type | Required | Description | -|-----------|------|----------|-------------| -| `config` | string | yes | YAML config | -| `format` | string | no | Output format (`openapi`, `markdown`, `table`) | - ---- - -#### `detect_project_features` - -Detect features and capabilities used in a project. - -| Parameter | Type | Required | Description | -|-----------|------|----------|-------------| -| `config` | string | yes | YAML config | - ---- - -#### `detect_infra_needs` - -Analyze a config and detect infrastructure requirements. - ---- - -#### `detect_ports` - -Detect all ports used by a workflow config. - ---- - -#### `detect_secrets` - -Detect secret references in a workflow config. - ---- - -#### `infer_pipeline_context` - -Infer the runtime context available at each step in a pipeline. - -| Parameter | Type | Required | Description | -|-----------|------|----------|-------------| -| `config` | string | yes | YAML config | -| `pipeline` | string | yes | Pipeline name to analyze | - ---- - -#### `manifest_analyze` - -Analyze a plugin manifest for compatibility. - ---- - -#### `registry_search` - -Search the plugin registry for available plugins. - -| Parameter | Type | Required | Description | -|-----------|------|----------|-------------| -| `query` | string | yes | Search query | -| `category` | string | no | Filter by category | - ---- - -#### `validate_template_expressions` - -Validate template expressions in a workflow config. - ---- - -## LSP Tools - -The Language Server Protocol (LSP) tools provide IDE-like diagnostics for -workflow YAML files. - -#### `mcp:lsp:diagnose` - -Run diagnostics on a YAML workflow config. - -| Parameter | Type | Required | Description | -|-----------|------|----------|-------------| -| `content` | string | yes | YAML content to diagnose | -| `uri` | string | no | File URI for context | - -**Returns:** List of diagnostics with severity (error, warning, hint), line/column, and message. - ---- - -#### `mcp:lsp:complete` - -Get completion suggestions at a position in a YAML file. - -| Parameter | Type | Required | Description | -|-----------|------|----------|-------------| -| `content` | string | yes | YAML content | -| `line` | int | yes | 0-indexed line number | -| `character` | int | yes | 0-indexed character position | - ---- - -#### `mcp:lsp:hover` - -Get hover documentation at a position in a YAML file. - -| Parameter | Type | Required | Description | -|-----------|------|----------|-------------| -| `content` | string | yes | YAML content | -| `line` | int | yes | 0-indexed line number | -| `character` | int | yes | 0-indexed character position | - ---- +`wfctl mcp` exposes **33 tools and 5 resources** over local stdio. The in-process +library uses the same tool registrations and SDK input validation. An embedder +that attaches an engine with `WithEngine` / `WithInProcessEngine` additionally +exposes `run_workflow` (34 tools). It is absent from the default CLI. + +The corrected validation, error and annotation contracts describe this source +change. Released `v0.86.2` predates them; a release download or `go install +...@latest` may differ until a Workflow release includes the corrections. Match +claims to the executable's version/source, not only this reference. + +Use live `tools/list` for this executable's complete schemas and descriptions; +this table lists every registered argument. `string[]` means an array containing +only strings. Optional fields may be omitted; a supplied value must have its +advertised type. Extra top-level argument fields currently remain permitted, +but undocumented names do not select alternate behavior. + +## Built-in tool inventory + +| Tool | Required arguments | Optional arguments | Purpose | +|------|--------------------|--------------------|---------| +| `list_module_types` | — | — | Available module metadata. | +| `list_step_types` | — | — | Available pipeline step metadata. | +| `list_trigger_types` | — | — | Available trigger metadata. | +| `list_workflow_types` | — | — | Available workflow-handler metadata. | +| `generate_schema` | — | — | Workflow configuration JSON Schema. | +| `validate_config` | `yaml_content: string` | `strict: boolean`, `skip_unknown_types: boolean` | Basic configuration validation report. | +| `inspect_config` | `yaml_content: string` | — | Modules, dependencies, workflows, triggers and pipeline summary. | +| `list_plugins` | — | `data_dir: string` | Installed plugin manifests; data_dir overrides the startup catalog path. | +| `get_config_skeleton` | `module_types: string[]` | — | Skeleton YAML for requested module types. | +| `run_workflow` | `workflow_type: string`, `action: string` | `data: object` | Conditional engine execution; absent from wfctl mcp. | +| `get_module_schema` | `module_type: string` | — | Module config fields and example. | +| `get_step_schema` | `step_type: string` | — | Step config keys and example. | +| `get_template_functions` | — | — | Template function catalog. | +| `validate_template_expressions` | `yaml_content: string` | — | Static template-reference diagnostics. | +| `infer_pipeline_context` | `yaml_content: string`, `pipeline_name: string` | `after_step: string`, `openapi_spec: string` | Static context inferred before/after a pipeline step. | +| `get_config_examples` | — | `name: string` | List examples, or return one by name. | +| `api_extract` | `yaml_content: string` | `title: string`, `version: string`, `include_schemas: boolean` | Extracted OpenAPI 3.0 specification. | +| `diff_configs` | `old_yaml: string`, `new_yaml: string` | `check_breaking: boolean` | Semantic change and breaking-change report. | +| `manifest_analyze` | `yaml_content: string` | `name: string` | Application deployment manifest inferred from workflow YAML. | +| `contract_generate` | `yaml_content: string` | `baseline_json: string` | API contract snapshot; optional baseline comparison. | +| `compat_check` | `yaml_content: string` | — | Module/step compatibility with this server’s catalog. | +| `template_validate_config` | `yaml_content: string` | `strict: boolean` | Module/step/trigger types, config-key hints and dependency diagnostics. | +| `generate_github_actions` | `yaml_content: string` | `registry: string`, `platforms: string`, `phase_config_yaml: string`, `wfctl_version: string` | Config-derived CI plan and GitHub Actions files, plus legacy CD/release output. | +| `ci_plan` | `yaml_content: string` | `phase_config_yaml: string`, `wfctl_version: string` | Config-derived platform-neutral CIPlan. | +| `detect_project_features` | `yaml_content: string` | — | Application feature summary. | +| `modernize` | `yaml_content: string` | `apply: boolean`, `rules: string`, `exclude_rules: string` | Findings and optional fixed_yaml returned in memory. | +| `registry_search` | — | `query: string`, `type: string`, `tier: string`, `include_private: boolean` | Search a configured local workflow-registry clone. | +| `scaffold_ci` | `description: string` | `yaml_content: string`, `binary_path: string`, `environments: string[]` | Proposed ci: section. | +| `scaffold_environment` | `provider: string` | `environments: string[]`, `secrets_provider: string`, `exposure: string` | Proposed environments: section. | +| `scaffold_infra` | `yaml_content: string`, `provider: string` | `environment: string` | Proposed infra: section. | +| `detect_secrets` | `yaml_content: string` | — | Secret candidates and environment references; not secret removal. | +| `detect_ports` | `yaml_content: string` | — | Application port inventory. | +| `generate_bootstrap` | `yaml_content: string`, `platform: string` | — | CI bootstrap YAML invoking wfctl ci run. | +| `detect_infra_needs` | `yaml_content: string` | `provider: string`, `plugins_dir: string` | Suggested infrastructure requirements; optionally reads plugin manifests. | + +## Behavior and defaults + +- `validate_config` defaults `strict:false` and `skip_unknown_types:false`. + Strict mode disallows an empty module list. The basic check permits absent + entry points and skips workflow/trigger type checks even with strict mode; + skipping unknown types also skips the module type check. It does not build or + start an engine. Use `template_validate_config` for deeper checks and compare + the module, step, trigger and workflow inventories explicitly. +- `template_validate_config` defaults `strict:false`; strict mode treats its + warnings as failure. It checks module/step/trigger types, config-key hints and + dependencies; despite its name, it does not analyze template expressions. + Use `validate_template_expressions` for heuristic static template-expression + and step-reference checks. Neither tool evaluates templates at runtime. +- `compat_check` checks the running server's catalog. It has no `target_version` + argument and does not prove that plugin binaries can load or an app can run. +- `modernize` defaults `apply:false`. `apply:true` returns `fixed_yaml` when fixes + exist; it does not save a file. `rules` and `exclude_rules` are comma-separated + rule IDs. Plugin-declared rules can be read from the startup plugin directory. +- `infer_pipeline_context` requires `pipeline_name`; omit `after_step` for initial + context. Its advertised `openapi_spec` field is currently unused by the + handler, so supplying it does not enrich the result. +- `get_config_examples` lists examples when `name` is omitted. Named examples + come from the discovered `example/` catalog; this is not an arbitrary path + reader. Catalog contents depend on the server's local installation. +- `api_extract` defaults its API `version` to `1.0.0` and `include_schemas:true`; + `title` comes from the config or `Workflow API`. There is no `format` argument. +- `diff_configs` advertises `check_breaking`, but its current handler does not + read that field; breaking-change analysis is always included. + `contract_generate` consumes `baseline_json`, not an OpenAPI document, for + optional contract comparison. +- `ci_plan` and `generate_github_actions` re-analyze the supplied config through + cigen. `phase_config_yaml` being nonempty selects a prerequisite phase's + logical path; its contents are not currently analyzed. `wfctl_version` defaults + to `latest`. The GitHub renderer returns `plan`, `files`, `ci_yaml` and legacy + CD/release fields. `registry` (`ghcr.io`) and `platforms` + (`linux/amd64,linux/arm64`) affect that legacy output. These tools do not consume + an edited CIPlan; use `wfctl ci generate --from-plan` for that CLI operation. +- `scaffold_ci` requires a description; `binary_path` defaults to `./cmd/server`. + Its advertised optional `yaml_content` is currently unused by the handler; + application features are inferred from the description. + `scaffold_environment` defaults to local/staging/production environments, + `secrets_provider:env`, and `exposure:port-forward`. `scaffold_infra` defaults + its environment to production. These return proposed YAML without deploying. +- `generate_bootstrap` accepts `platform:github-actions` or `gitlab-ci`; it + generates CI bootstrap text rather than an application or installed runner. +- `detect_infra_needs` defaults its provider to `aws`; `plugins_dir` selects a + directory whose plugin manifests can contribute requirements. +- `registry_search` reads the startup `-registry-dir` clone and defaults + `include_private:false`. `query`, `type` and `tier` are optional local filters; + no configured clone produces a tool error. It does not query a remote service. + +## Result and error contract + +Tools return MCP text content: either YAML/text or a JSON report encoded in the +text field. Required-field/type violations and handler failures have +`isError:true`. A parsed analysis reporting `valid:false` or `compatible:false` +remains a successful tool invocation: inspect its report, not only `isError`. +Malformed JSON produces a JSON-RPC parse error. The in-process adapter returns a +nonnil Go error for protocol/tool failures and preserves successful text results, +including analytical negatives. Audit logging records tool/status metadata only. + +The engine-only tool reports `success:false` with `isError:true` when its provider +fails; successful execution reports `success:true`. It can have arbitrary +external or destructive effects, so an embedder must supply authorization. + +## Metadata and input boundaries + +All 33 default tools explicitly advertise `readOnlyHint:true` and +`destructiveHint:false`: analysis and generation do not persist customer changes. +`modernize apply:true` is included. CI analysis uses temporary implementation +files that are removed after use. `openWorldHint:true` applies to `list_plugins`, +`detect_infra_needs`, `ci_plan` and `generate_github_actions`, which can inspect +caller-selected local directories or file imports. Other tools use bounded local +catalogs or in-memory computation and advertise `openWorldHint:false`. +`run_workflow` advertises false/true/true for readOnly/destructive/openWorld. +These hints are descriptive metadata, not an OS filesystem sandbox. + +Supply sanitized YAML and public/synthetic values. Results may repeat submitted +configuration, paths, manifests and detected secret candidates; no general +redaction guarantee exists. Basic string-loaded config analysis does not resolve +imports. CI analysis does use file/import context and can read local references. +Manifest discovery proves metadata availability, not executable/plugin or runtime +acceptance. See [server setup and limits](mcp.md). + +## Separate application integrations + +The integrations below do not add tools to the default `wfctl mcp` inventory. +LSP diagnostics/completion/hover are not registered by this stdio server. ## Workflow-Defined MCP Tools @@ -456,46 +209,21 @@ modules: audit_tool_calls: true ``` -## In-Process vs External MCP Server +## In-process and stdio use -| Feature | In-Process | External (stdio/HTTP) | -|---------|------------|----------------------| -| Latency | <1ms | 5-50ms | -| Setup | Zero (built-in) | Requires subprocess or HTTP server | -| Tool access | All wfctl tools | All wfctl tools | -| Security | Agent process boundary | Process isolation | -| Multi-agent | Shared instance | Per-agent instances | -| Use case | Self-improvement loops | IDE integration, Claude Desktop | - -### In-Process Usage (workflow-plugin-agent) +`NewInProcessServer` provides the Go `MCPProvider` interface with sorted inventory +methods and the same validated calls. `wfctl mcp` launches a separate local +process over stdio; it does not start an HTTP server. Neither mode introduces +an authentication or filesystem-isolation boundary by itself. ```go -// In-process MCP server — no HTTP overhead -mcp := mcp.NewInProcessServer( - mcp.WithInProcessPluginDir("/data/plugins"), +provider := mcp.NewInProcessServer( + mcp.WithInProcessPluginDir("/path/to/public/plugins"), mcp.WithInProcessAuditLog(logger), ) - -result, err := mcp.CallTool(ctx, "validate_config", map[string]any{ - "config": proposedYAML, +result, err := provider.CallTool(ctx, "validate_config", map[string]any{ + "yaml_content": proposedYAML, }) ``` -### External Usage (wfctl mcp) - -```bash -# Start MCP server for Claude Desktop or other IDE -wfctl mcp -``` - -Claude Desktop config (`~/Library/Application Support/Claude/claude_desktop_config.json`): -```json -{ - "mcpServers": { - "workflow": { - "command": "wfctl", - "args": ["mcp"] - } - } -} -``` +For CLI launch options and generic client configuration, see [MCP server](mcp.md). diff --git a/docs/mcp.md b/docs/mcp.md index 0fedfc82..f55e01a6 100644 --- a/docs/mcp.md +++ b/docs/mcp.md @@ -1,198 +1,172 @@ # MCP Server for Workflow Engine -The workflow engine includes a built-in [Model Context Protocol (MCP)](https://modelcontextprotocol.io/) server that exposes engine functionality to AI assistants and tools. +`wfctl mcp` exposes Workflow configuration tools and documentation to local +[Model Context Protocol](https://modelcontextprotocol.io/) clients over stdio. +The server is part of the CLI, so its handshake version comes from the same +build. Run `wfctl version` to identify that executable. -The MCP server is integrated into the `wfctl` CLI as the `mcp` subcommand. Use `wfctl mcp` to start it. This ensures the MCP server version always matches the CLI and engine version, and benefits from automatic updates via `wfctl update`. +The corrected input validation, error propagation and annotations below describe +this source change. Released `v0.86.2` predates these corrections; downloading a +release or using `go install ...@latest` does not guarantee this source behavior +until a Workflow release includes it. Record the actual executable version and +source revision rather than applying these claims to an older binary. -## Features +## Tools and resources -The MCP server provides: +The default CLI registers **33 tools and 5 resources**. All default tools analyze, +read catalogs or return proposed text; they do not start an application, install +plugins or deploy infrastructure. An engine-backed Go embedder can additionally +register the existing `run_workflow` tool (34 total); the default CLI has no +attached engine and does not expose it. -### Tools +See the [complete tool/argument reference](mcp-tools-reference.md) and query +`tools/list` for the running executable's entire surface. Assistant instructions +that focus on a few tools do not hide or restrict the other tools. -| Tool | Description | -|------|-------------| -| `list_module_types` | List all available module types for workflow YAML configs | -| `list_step_types` | List all pipeline step types | -| `list_trigger_types` | List all trigger types (http, schedule, event, eventbus) | -| `list_workflow_types` | List all workflow handler types | -| `generate_schema` | Generate JSON Schema for workflow config files | -| `validate_config` | Validate a workflow YAML configuration string | -| `inspect_config` | Inspect a config and get structured summary of modules, workflows, triggers | -| `list_plugins` | List installed external plugins | -| `get_config_skeleton` | Generate a skeleton YAML config for given module types | -| `modernize` | Detect and fix known YAML config anti-patterns | -| `registry_search` | Search the plugin registry for available plugins | +| Resource | Content | +|----------|---------| +| `workflow://docs/overview` | Engine and CLI overview | +| `workflow://docs/yaml-syntax` | Configuration syntax guide | +| `workflow://docs/module-reference` | Current module/step catalog | +| `workflow://docs/full-reference` | Local DOCUMENTATION.md when found, otherwise a public documentation pointer | +| `workflow://docs/setup-guide` | Assistant setup guide | -### Resources +## Install or build -| Resource | Description | -|----------|-------------| -| `workflow://docs/overview` | Engine overview documentation | -| `workflow://docs/yaml-syntax` | YAML configuration syntax guide | -| `workflow://docs/module-reference` | Dynamic module type reference | +Install a released CLI through the [Workflow releases](https://github.com/GoCodeAlone/workflow/releases) +or the repository's supported installation methods. For a Go source install: -## Installation - -Install `wfctl` (the CLI includes the MCP server): - -```bash -# Install via Go -go install github.com/GoCodeAlone/workflow/cmd/wfctl@latest - -# Or download a pre-built binary from GitHub releases -# https://github.com/GoCodeAlone/workflow/releases/latest - -# Keep wfctl up to date (and thus the MCP server too) -wfctl update +```sh +GOWORK=off go install github.com/GoCodeAlone/workflow/cmd/wfctl@latest ``` -### Building from Source +From a checkout, using the Go version required by its `go.mod`: -```bash -# Build wfctl (includes the mcp command) -go build -o wfctl ./cmd/wfctl -# Or: make build-wfctl +```sh +GOWORK=off go build -o wfctl ./cmd/wfctl +./wfctl version ``` -## Configuring AI Clients +Updating an executable does not replace an already running MCP process. Restart +the client/server process to use the new build. `wfctl update --check` checks for +CLI updates; `wfctl update` replaces the installed binary when requested. -### Claude Desktop +## Local client configuration -Add the following to your Claude Desktop configuration file: +Launch a dedicated local process with explicit directories, for example: -**macOS**: `~/Library/Application Support/Claude/claude_desktop_config.json` -**Windows**: `%APPDATA%\Claude\claude_desktop_config.json` - -```json -{ - "mcpServers": { - "workflow": { - "command": "wfctl", - "args": ["mcp", "-plugin-dir", "/path/to/data/plugins"] - } - } -} +```sh +wfctl mcp -plugin-dir /path/to/public/plugins ``` -### VS Code with GitHub Copilot - -Add to your VS Code `settings.json`: - -```json -{ - "github.copilot.chat.mcpServers": { - "workflow": { - "command": "wfctl", - "args": ["mcp", "-plugin-dir", "/path/to/data/plugins"] - } - } -} -``` - -### Cursor - -Add to your Cursor MCP configuration (`.cursor/mcp.json`): +Clients that accept an `mcpServers` configuration can use this shape; consult +your host's own current configuration format and process permissions: ```json { "mcpServers": { "workflow": { - "command": "wfctl", - "args": ["mcp", "-plugin-dir", "/path/to/data/plugins"] + "command": "/path/to/wfctl", + "args": ["mcp", "-plugin-dir", "/path/to/public/plugins"] } } } ``` -### Generic MCP Client - -The server communicates over **stdio** using JSON-RPC 2.0. Any MCP-compatible client can connect: - -```bash -wfctl mcp -plugin-dir ./data/plugins -``` - -## Usage Examples - -Once connected, the AI assistant can use the tools to: - -### List available module types -``` -Use the list_module_types tool to see what modules are available. -``` - -### Validate a configuration -``` -Validate this workflow config: -modules: - - name: webServer - type: http.server - config: - address: ":8080" -``` - -### Generate a config skeleton -``` -Generate a skeleton config with http.server, http.router, and http.handler modules. -``` - -### Inspect a configuration -``` -Inspect this config and show me the dependency graph... -``` +This is local stdio integration, not host certification. A cloud client that +cannot launch a local executable needs a separately provided transport/service; +`wfctl mcp` does not start an HTTP endpoint or a hosted remote service. -## Command Line Options +## Command-line options -``` +```text Usage: wfctl mcp [options] - -Options: - -plugin-dir string Plugin data directory (default "data/plugins") -``` - -## Keeping the MCP Server Up to Date - -Because the MCP server is now part of `wfctl`, you can use the built-in update command to keep everything in sync: - -```bash -# Check for updates -wfctl update --check - -# Install the latest version (replaces the wfctl binary in-place) -wfctl update -``` - -Set `WFCTL_NO_UPDATE_CHECK=1` to suppress automatic update notices. - -## Dynamic Updates - -The MCP server dynamically reflects the current state of the engine: - -- **Module types** are read from the schema registry, which includes both built-in types and any dynamically registered plugin types -- **Plugin list** scans the configured plugin directory at query time -- **Schema generation** uses the current module schema registry -- **Validation** uses the same validation logic as `wfctl validate` - -This means the MCP server automatically picks up new module types and plugins without requiring a restart. - -## Running Tests - -```bash -go test -v ./mcp/ -go test -v -run TestRunMCP ./cmd/wfctl/ -``` - -## Architecture - -``` -cmd/wfctl/main.go → CLI entry point; registers "mcp" command -cmd/wfctl/mcp.go → "wfctl mcp" command handler (delegates to mcp package) -mcp/server.go → MCP server setup, tool handlers, resource handlers -mcp/docs.go → Embedded documentation content -mcp/server_test.go → Unit tests -modernize/ → Shared modernize rules (used by both wfctl CLI and MCP server) -``` - -The server uses the [mcp-go](https://github.com/mark3labs/mcp-go) library for MCP protocol implementation over stdio transport. - + -plugin-dir string Plugin data directory (default "data/plugins") + -registry-dir string Local workflow-registry clone for registry_search + -documentation-file string Explicit DOCUMENTATION.md path (auto-detected when empty) +``` + +Use empty, dedicated plugin directories for a standalone public configuration +session. Plugin manifests and an optional local registry clone supply metadata; +no credentials or organization repository access are required for built-in tools. +The full-reference resource can serve local documentation, so choose the working +directory and `-documentation-file` deliberately. + +## Validation and evidence limits + +Pass `yaml_content` to `validate_config`. Its basic check allows missing entry +points, skips workflow/trigger type checks, and optionally skips module type +checks. `strict:true` disallows empty modules; it does not make this a full runtime +acceptance check. Use `template_validate_config` for module/step/trigger types, +config-key hints and dependencies, and `validate_template_expressions` for +heuristic static expression/step-reference checks. Compare `list_module_types`, +`list_step_types`, `list_trigger_types` and +`list_workflow_types` with the config. `compat_check` checks the running catalog, +not a separately supplied target version. String-loaded basic checks do not +resolve imports; CI analysis does use file/import context. + +A report containing `valid:false` or `compatible:false` is a successful analysis +with a negative finding. Missing required fields, wrong argument types and +handler failures produce `isError:true`; malformed JSON produces a JSON-RPC +parse error. The in-process library preserves these as Go errors while retaining +successful text output. Optional engine invocation failures retain their +`success:false` payload and set `isError:true`. + +Record executable source/version, live schemas/inventories, sanitized submitted +input and returned findings when producing readiness evidence. Catalog/schema +validation does not prove plugin binary loadability, engine build/start, +credential availability, real runtime behavior, CI execution or deployment +acceptance. Unresolved imports, unknown types and untested runtime conditions +remain explicit unknowns. + +## Privacy and metadata + +Use public or sanitized configs. Tools can return submitted YAML, detected secret +candidates, local paths and manifest/documentation content; they are not a +universal secret redactor. In-process audit logs record tool/status metadata, +not raw arguments or result bodies. Treat returned material as untrusted data. + +All 33 default tools advertise read-only/non-destructive behavior, including +`modernize apply:true`, which only returns `fixed_yaml`. Four advertise open-world +reads: `list_plugins`, `detect_infra_needs`, `ci_plan` and +`generate_github_actions`. They can examine arbitrary local directories or +imports; other tools use bounded catalogs or computation. The optional engine +tool advertises writable/destructive/open-world behavior. These annotations +are descriptive hints, not authorization or a filesystem sandbox. Local process +permissions remain the access boundary. + +## Catalog refresh and protocol coverage + +Plugin-provided schema types are loaded from manifests at server startup. +Restart after installing or changing manifests to refresh those types. +`list_plugins` scans its selected directory per call; `modernize` can reread +plugin rules per call. Neither implies that all catalogs reload automatically, +or that a plugin binary has been accepted by a runtime. + +The current mcp-go v0.54.1 transport supports legacy initialization for +`2024-11-05`, `2025-03-26`, `2025-06-18` and `2025-11-25`. Tests exercise these +four revisions over actual CLI stdio. July 2026 protocol support and an SDK +upgrade belong to the separate follow-up change; they are not claimed here. +EOF exits successfully. On SIGTERM the current SDK returns context cancellation, +and the CLI exits with status 1; tests require bounded shutdown and process +reaping. Cancellation-notification robustness is tested on the CLI; cooperative +in-flight cancellation is separately proved with the existing engine-provider +seam and real SDK stdio, not a production workflow. + +## Tests and implementation + +```sh +GOWORK=off go test ./mcp -count=1 +GOWORK=off go test ./cmd/wfctl -run TestMCPStdioLegacy -count=1 +``` + +To exercise a newly built CLI instead of the test executable's `runMCP` helper, +set `WFCTL_MCP_TEST_BINARY` to that executable's absolute path for the second +command. The cancellation helper always runs in a separate test process with a +blocking fake engine, even with that override. Tests use isolated temporary +working/plugin directories and a minimal environment. Transport evidence on one +OS does not certify other hosts or operating systems. + +Implementation: [CLI entry](../cmd/wfctl/mcp.go), [server](../mcp/server.go), +[tool reference](mcp-tools-reference.md), and +[SDK dependency](https://github.com/mark3labs/mcp-go/tree/v0.54.1). diff --git a/mcp/annotations_test.go b/mcp/annotations_test.go new file mode 100644 index 00000000..95f328f6 --- /dev/null +++ b/mcp/annotations_test.go @@ -0,0 +1,81 @@ +package mcp + +import ( + "encoding/json" + "testing" +) + +func TestMCPAnnotations(t *testing.T) { + // Per-name effects audit: catalog reads and in-memory previews are read-only; + // only the four path/import readers have open-ended filesystem targets. + want := map[string][3]bool{ + "api_extract": {true, false, false}, + "ci_plan": {true, false, true}, + "compat_check": {true, false, false}, + "contract_generate": {true, false, false}, + "detect_infra_needs": {true, false, true}, + "detect_ports": {true, false, false}, + "detect_project_features": {true, false, false}, + "detect_secrets": {true, false, false}, + "diff_configs": {true, false, false}, + "generate_bootstrap": {true, false, false}, + "generate_github_actions": {true, false, true}, + "generate_schema": {true, false, false}, + "get_config_examples": {true, false, false}, + "get_config_skeleton": {true, false, false}, + "get_module_schema": {true, false, false}, + "get_step_schema": {true, false, false}, + "get_template_functions": {true, false, false}, + "infer_pipeline_context": {true, false, false}, + "inspect_config": {true, false, false}, + "list_module_types": {true, false, false}, + "list_plugins": {true, false, true}, + "list_step_types": {true, false, false}, + "list_trigger_types": {true, false, false}, + "list_workflow_types": {true, false, false}, + "manifest_analyze": {true, false, false}, + "modernize": {true, false, false}, // apply:true only returns fixed_yaml. + "registry_search": {true, false, false}, // Fixed startup registry catalog. + "scaffold_ci": {true, false, false}, + "scaffold_environment": {true, false, false}, + "scaffold_infra": {true, false, false}, + "template_validate_config": {true, false, false}, + "validate_config": {true, false, false}, + "validate_template_expressions": {true, false, false}, + } + for _, engine := range []bool{false, true} { + srv := NewServer("") + if engine { + srv = NewServer("", WithEngine(&mockEngine{})) + want["run_workflow"] = [3]bool{false, true, true} + } + tools := srv.MCPServer().ListTools() + if len(tools) != len(want) { + t.Fatalf("engine=%v: got %d tools, want %d audited names", engine, len(tools), len(want)) + } + for name, expected := range want { + t.Run(name, func(t *testing.T) { + registered, ok := tools[name] + if !ok { + t.Fatal("audited tool missing") + } + data, err := json.Marshal(registered.Tool) + if err != nil { + t.Fatal(err) + } + var wire struct { + Annotations map[string]any `json:"annotations"` + } + if err := json.Unmarshal(data, &wire); err != nil { + t.Fatal(err) + } + for i, key := range []string{"readOnlyHint", "destructiveHint", "openWorldHint"} { + actual, present := wire.Annotations[key].(bool) + if !present || actual != expected[i] { + t.Errorf("engine=%v: %s=%v (boolean=%v), want %v", engine, key, wire.Annotations[key], present, expected[i]) + } + } + }) + } + } +} diff --git a/mcp/scaffold_tools.go b/mcp/scaffold_tools.go index 38c9d147..97afbfc0 100644 --- a/mcp/scaffold_tools.go +++ b/mcp/scaffold_tools.go @@ -18,6 +18,7 @@ import ( func (s *Server) registerScaffoldTools() { s.mcpServer.AddTool( mcp.NewTool("scaffold_ci", + toolBehavior(true, false, false), mcp.WithDescription("Generate a ci: YAML section for a workflow config based on the application type. "+ "Produces build (binaries/containers/assets), test (unit/integration/e2e), and deploy sub-sections "+ "with sensible defaults. Pass the existing YAML config and an app description."), @@ -41,6 +42,7 @@ func (s *Server) registerScaffoldTools() { s.mcpServer.AddTool( mcp.NewTool("scaffold_environment", + toolBehavior(true, false, false), mcp.WithDescription("Generate an environments: YAML section for a workflow config. "+ "Creates per-environment configuration blocks with provider, region, env vars, "+ "secrets provider, and exposure method (tailscale, cloudflare, port-forward)."), @@ -64,6 +66,7 @@ func (s *Server) registerScaffoldTools() { s.mcpServer.AddTool( mcp.NewTool("scaffold_infra", + toolBehavior(true, false, false), mcp.WithDescription("Generate an infra: YAML section by analyzing the workflow config's modules. "+ "Detects database, cache, and messaging module types and suggests matching cloud resources "+ "(e.g. database.postgres → RDS instance, cache.redis → ElastiCache cluster)."), @@ -84,6 +87,7 @@ func (s *Server) registerScaffoldTools() { s.mcpServer.AddTool( mcp.NewTool("detect_secrets", + toolBehavior(true, false, false), mcp.WithDescription("Scan a workflow YAML config and return a list of secret candidates: "+ "fields whose names (dsn, apiKey, token, password, signingKey, clientSecret) or values "+ "(${...} env var references) indicate secrets that should be managed externally."), @@ -97,6 +101,7 @@ func (s *Server) registerScaffoldTools() { s.mcpServer.AddTool( mcp.NewTool("detect_ports", + toolBehavior(true, false, false), mcp.WithDescription("Scan a workflow YAML config and return a list of ports used by the application. "+ "Checks http.server, grpc.server, and other network-listening module configs."), mcp.WithString("yaml_content", @@ -109,6 +114,7 @@ func (s *Server) registerScaffoldTools() { s.mcpServer.AddTool( mcp.NewTool("generate_bootstrap", + toolBehavior(true, false, false), mcp.WithDescription("Generate a CI platform bootstrap file that calls wfctl ci run. "+ "Reads the ci.deploy.environments section from the config to create per-environment "+ "deploy jobs. Supports github-actions and gitlab-ci platforms."), @@ -126,6 +132,7 @@ func (s *Server) registerScaffoldTools() { s.mcpServer.AddTool( mcp.NewTool("detect_infra_needs", + toolBehavior(true, false, true), mcp.WithDescription("Analyze a workflow YAML config's modules and suggest the infrastructure "+ "components needed to run the application in production. Returns a structured list of "+ "infrastructure requirements (databases, caches, message brokers, storage buckets) "+ diff --git a/mcp/server.go b/mcp/server.go index b8fc5334..c1a988c9 100644 --- a/mcp/server.go +++ b/mcp/server.go @@ -162,13 +162,22 @@ func (s *Server) ServeStdio() error { return server.ServeStdio(s.mcpServer) } +// toolBehavior records effects explicitly without changing the SDK's idempotence hint. +func toolBehavior(readOnly, destructive, openWorld bool) mcp.ToolOption { + return func(tool *mcp.Tool) { + mcp.WithReadOnlyHintAnnotation(readOnly)(tool) + mcp.WithDestructiveHintAnnotation(destructive)(tool) + mcp.WithOpenWorldHintAnnotation(openWorld)(tool) + } +} + // registerTools registers all workflow engine tools with the MCP server. func (s *Server) registerTools() { // list_module_types s.mcpServer.AddTool( mcp.NewTool("list_module_types", + toolBehavior(true, false, false), mcp.WithDescription("List all available workflow module types that can be used in the 'modules' section of a workflow YAML config. Returns built-in types plus types from installed plugins (loaded from plugin_dir at server startup)."), - mcp.WithReadOnlyHintAnnotation(true), ), s.handleListModuleTypes, ) @@ -176,8 +185,8 @@ func (s *Server) registerTools() { // list_step_types s.mcpServer.AddTool( mcp.NewTool("list_step_types", + toolBehavior(true, false, false), mcp.WithDescription("List all available pipeline step types that can be used in pipeline definitions. Returns built-in steps plus steps from installed plugins (loaded from plugin_dir at server startup)."), - mcp.WithReadOnlyHintAnnotation(true), ), s.handleListStepTypes, ) @@ -185,8 +194,8 @@ func (s *Server) registerTools() { // list_trigger_types s.mcpServer.AddTool( mcp.NewTool("list_trigger_types", + toolBehavior(true, false, false), mcp.WithDescription("List all available trigger types (e.g., http, schedule, event, eventbus) that can start workflow execution. Includes types from installed plugins."), - mcp.WithReadOnlyHintAnnotation(true), ), s.handleListTriggerTypes, ) @@ -194,8 +203,8 @@ func (s *Server) registerTools() { // list_workflow_types s.mcpServer.AddTool( mcp.NewTool("list_workflow_types", + toolBehavior(true, false, false), mcp.WithDescription("List all available workflow handler types (e.g., http, messaging, statemachine, scheduler, integration, event, pipeline) that define how workflows process work. Includes types from installed plugins."), - mcp.WithReadOnlyHintAnnotation(true), ), s.handleListWorkflowTypes, ) @@ -203,8 +212,8 @@ func (s *Server) registerTools() { // generate_schema s.mcpServer.AddTool( mcp.NewTool("generate_schema", + toolBehavior(true, false, false), mcp.WithDescription("Generate the JSON Schema for workflow configuration YAML files. This schema describes all valid fields, module types, and structure for authoring workflow configs."), - mcp.WithReadOnlyHintAnnotation(true), ), s.handleGenerateSchema, ) @@ -212,6 +221,7 @@ func (s *Server) registerTools() { // validate_config s.mcpServer.AddTool( mcp.NewTool("validate_config", + toolBehavior(true, false, false), mcp.WithDescription("Validate a workflow YAML configuration string. Returns validation results including any errors found. Use this to check if a YAML config is well-formed and uses valid module types."), mcp.WithString("yaml_content", mcp.Required(), @@ -230,6 +240,7 @@ func (s *Server) registerTools() { // inspect_config s.mcpServer.AddTool( mcp.NewTool("inspect_config", + toolBehavior(true, false, false), mcp.WithDescription("Inspect a workflow YAML configuration and return a structured summary of its modules, workflows, triggers, pipelines, and dependency graph."), mcp.WithString("yaml_content", mcp.Required(), @@ -242,11 +253,11 @@ func (s *Server) registerTools() { // list_plugins s.mcpServer.AddTool( mcp.NewTool("list_plugins", + toolBehavior(true, false, true), mcp.WithDescription("List installed external plugins from the plugin directory."), mcp.WithString("data_dir", mcp.Description("Plugin data directory. Defaults to 'data/plugins'"), ), - mcp.WithReadOnlyHintAnnotation(true), ), s.handleListPlugins, ) @@ -254,6 +265,7 @@ func (s *Server) registerTools() { // get_config_skeleton s.mcpServer.AddTool( mcp.NewTool("get_config_skeleton", + toolBehavior(true, false, false), mcp.WithDescription("Generate a skeleton/template workflow YAML configuration for a given set of module types. Useful for bootstrapping new workflow configs."), mcp.WithArray("module_types", mcp.Required(), @@ -268,6 +280,7 @@ func (s *Server) registerTools() { if s.engine != nil { s.mcpServer.AddTool( mcp.NewTool("run_workflow", + toolBehavior(false, true, true), mcp.WithDescription("Trigger a workflow execution on the attached engine. Requires the server to be started with an engine (WithEngine option). "+ "Provide the workflow type, action, and optional data payload."), mcp.WithString("workflow_type", diff --git a/mcp/tools.go b/mcp/tools.go index ff3d4ddb..30a83d99 100644 --- a/mcp/tools.go +++ b/mcp/tools.go @@ -21,6 +21,7 @@ func (s *Server) registerNewTools() { // get_module_schema s.mcpServer.AddTool( mcp.NewTool("get_module_schema", + toolBehavior(true, false, false), mcp.WithDescription("Return the full configuration schema for a given module type. "+ "Includes description, config fields (key, type, description, required, default, options), "+ "inputs, outputs, and an example usage snippet."), @@ -28,7 +29,6 @@ func (s *Server) registerNewTools() { mcp.Required(), mcp.Description("The module type string (e.g. 'http.server', 'messaging.broker')"), ), - mcp.WithReadOnlyHintAnnotation(true), ), s.handleGetModuleSchema, ) @@ -36,13 +36,13 @@ func (s *Server) registerNewTools() { // get_step_schema s.mcpServer.AddTool( mcp.NewTool("get_step_schema", + toolBehavior(true, false, false), mcp.WithDescription("Return the schema for a given pipeline step type. "+ "Includes description, config keys with types and descriptions, and an example usage snippet."), mcp.WithString("step_type", mcp.Required(), mcp.Description("The step type string (e.g. 'step.set', 'step.http_call')"), ), - mcp.WithReadOnlyHintAnnotation(true), ), s.handleGetStepSchema, ) @@ -50,9 +50,9 @@ func (s *Server) registerNewTools() { // get_template_functions s.mcpServer.AddTool( mcp.NewTool("get_template_functions", + toolBehavior(true, false, false), mcp.WithDescription("Return the complete list of available Go template functions for pipeline templates. "+ "Each function includes its name, signature, description, and an example usage."), - mcp.WithReadOnlyHintAnnotation(true), ), s.handleGetTemplateFunctions, ) @@ -60,6 +60,7 @@ func (s *Server) registerNewTools() { // validate_template_expressions s.mcpServer.AddTool( mcp.NewTool("validate_template_expressions", + toolBehavior(true, false, false), mcp.WithDescription("Validate template expressions ({{ ... }}) in a YAML pipeline config string. "+ "Checks for forward references, self-references, undefined step references, "+ "and hyphenated step name dot-access patterns that require index syntax."), @@ -74,6 +75,7 @@ func (s *Server) registerNewTools() { // infer_pipeline_context s.mcpServer.AddTool( mcp.NewTool("infer_pipeline_context", + toolBehavior(true, false, false), mcp.WithDescription("Infer the available template variables at a specific point in a pipeline. "+ "Analyzes preceding step outputs to tell you what .steps.* fields are available, "+ "plus trigger, body, and meta context. Useful for authoring template expressions."), @@ -91,7 +93,6 @@ func (s *Server) registerNewTools() { mcp.WithString("openapi_spec", mcp.Description("Optional OpenAPI 3.0 JSON/YAML spec to enrich request body schema context"), ), - mcp.WithReadOnlyHintAnnotation(true), ), s.handleInferPipelineContext, ) @@ -99,13 +100,13 @@ func (s *Server) registerNewTools() { // get_config_examples s.mcpServer.AddTool( mcp.NewTool("get_config_examples", + toolBehavior(true, false, false), mcp.WithDescription("List and optionally return example workflow config files from the example/ directory. "+ "When called without a name, lists all available examples. "+ "When called with a name, returns the full content of that example."), mcp.WithString("name", mcp.Description("Name of a specific example to fetch (e.g. 'api-server-config'). Omit to list all examples."), ), - mcp.WithReadOnlyHintAnnotation(true), ), s.handleGetConfigExamples, ) diff --git a/mcp/wfctl_tools.go b/mcp/wfctl_tools.go index 84e143cb..3d334bf4 100644 --- a/mcp/wfctl_tools.go +++ b/mcp/wfctl_tools.go @@ -26,6 +26,7 @@ import ( func (s *Server) registerWfctlTools() { s.mcpServer.AddTool( mcp.NewTool("api_extract", + toolBehavior(true, false, false), mcp.WithDescription("Extract an OpenAPI 3.0 specification from a workflow YAML config. "+ "Parses HTTP-triggered workflows and pipelines to produce a complete API spec "+ "with inferred request/response schemas."), @@ -42,13 +43,13 @@ func (s *Server) registerWfctlTools() { mcp.WithBoolean("include_schemas", mcp.Description("Attempt to infer request/response schemas from step types (default: true)"), ), - mcp.WithReadOnlyHintAnnotation(true), ), s.handleAPIExtract, ) s.mcpServer.AddTool( mcp.NewTool("diff_configs", + toolBehavior(true, false, false), mcp.WithDescription("Compare two workflow YAML configurations and report what changed. "+ "Detects added/removed/changed modules and pipelines, flags stateful module changes, "+ "and identifies breaking changes."), @@ -63,13 +64,13 @@ func (s *Server) registerWfctlTools() { mcp.WithBoolean("check_breaking", mcp.Description("Include breaking change detection for stateful modules (default: true)"), ), - mcp.WithReadOnlyHintAnnotation(true), ), s.handleDiffConfigs, ) s.mcpServer.AddTool( mcp.NewTool("manifest_analyze", + toolBehavior(true, false, false), mcp.WithDescription("Analyze a workflow YAML config and produce an infrastructure requirements manifest. "+ "Reports databases, services, event buses, storage, external APIs, ports, sidecars, "+ "and resource estimates (CPU, memory, disk)."), @@ -80,13 +81,13 @@ func (s *Server) registerWfctlTools() { mcp.WithString("name", mcp.Description("Override the manifest name (default: derived from config)"), ), - mcp.WithReadOnlyHintAnnotation(true), ), s.handleManifestAnalyze, ) s.mcpServer.AddTool( mcp.NewTool("contract_generate", + toolBehavior(true, false, false), mcp.WithDescription("Generate an API contract snapshot from a workflow config. "+ "Extracts endpoints, modules, step types, and event topics. "+ "Optionally compares against a baseline contract JSON to detect breaking changes."), @@ -97,26 +98,26 @@ func (s *Server) registerWfctlTools() { mcp.WithString("baseline_json", mcp.Description("Previous contract JSON for comparison (optional)"), ), - mcp.WithReadOnlyHintAnnotation(true), ), s.handleContractGenerate, ) s.mcpServer.AddTool( mcp.NewTool("compat_check", + toolBehavior(true, false, false), mcp.WithDescription("Check whether a workflow config is compatible with the current engine version. "+ "Reports which module and step types are available in the engine."), mcp.WithString("yaml_content", mcp.Required(), mcp.Description("The YAML content of the workflow configuration"), ), - mcp.WithReadOnlyHintAnnotation(true), ), s.handleCompatCheck, ) s.mcpServer.AddTool( mcp.NewTool("template_validate_config", + toolBehavior(true, false, false), mcp.WithDescription("Deep validation of a workflow config against known module types, step types, "+ "trigger types, dependencies, and template expressions. More comprehensive than validate_config."), mcp.WithString("yaml_content", @@ -126,13 +127,13 @@ func (s *Server) registerWfctlTools() { mcp.WithBoolean("strict", mcp.Description("Fail on warnings in addition to errors (default: false)"), ), - mcp.WithReadOnlyHintAnnotation(true), ), s.handleTemplateValidateConfig, ) s.mcpServer.AddTool( mcp.NewTool("generate_github_actions", + toolBehavior(true, false, true), mcp.WithDescription("Render CI/CD workflow YAML from a workflow config. The CI workflow (the 'files'/'ci_yaml' result) is config-derived via the cigen engine (analyze -> CIPlan -> render): required secrets scoped per deploy phase, a 'wfctl migrations up' step when ci.migrations is set, a health-check smoke job, plugin-install steps when plugins are used, and a plan-guard that fails on a replace/destroy - not a fixed template. Also returns the 'plan' (CIPlan) and legacy template-based 'cd_yaml'/'release_yaml' (these use the registry/platforms inputs). This tool re-analyzes the config on each call and renders GitHub Actions ONLY - it does NOT accept a pre-built/edited CIPlan. Use the ci_plan tool to preview the CIPlan; to render an edited plan or a GitLab CI workflow, use the wfctl CLI ('wfctl ci generate --from-plan' / '--platform gitlab_ci')."), mcp.WithString("yaml_content", mcp.Required(), @@ -150,13 +151,13 @@ func (s *Server) registerWfctlTools() { mcp.WithString("wfctl_version", mcp.Description("wfctl version to pin in the plan (default: latest)"), ), - mcp.WithReadOnlyHintAnnotation(true), ), s.handleGenerateGithubActions, ) s.mcpServer.AddTool( mcp.NewTool("ci_plan", + toolBehavior(true, false, true), mcp.WithDescription("Analyze a workflow YAML config and emit a platform-neutral CIPlan JSON using the config-derived cigen engine. "+ "The plan describes project name, wfctl version, deploy phases (with secrets scoped per phase), the secrets union, "+ "migrations spec, smoke test URL, plan guard, and warnings. "+ @@ -171,26 +172,26 @@ func (s *Server) registerWfctlTools() { mcp.WithString("wfctl_version", mcp.Description("wfctl version to pin in the plan (default: latest)"), ), - mcp.WithReadOnlyHintAnnotation(true), ), s.handleCIPlan, ) s.mcpServer.AddTool( mcp.NewTool("detect_project_features", + toolBehavior(true, false, false), mcp.WithDescription("Analyze a workflow config to detect what features the project uses. "+ "Reports presence of UI, auth, database, plugins, HTTP, and lists all module types."), mcp.WithString("yaml_content", mcp.Required(), mcp.Description("The YAML content of the workflow configuration"), ), - mcp.WithReadOnlyHintAnnotation(true), ), s.handleDetectProjectFeatures, ) s.mcpServer.AddTool( mcp.NewTool("modernize", + toolBehavior(true, false, false), mcp.WithDescription("Detect and optionally fix known YAML config anti-patterns. "+ "Reports issues like hyphenated step names, template syntax in conditional fields, "+ "missing db_query mode, dot-access patterns, absolute dbPaths, empty routes, "+ @@ -210,13 +211,13 @@ func (s *Server) registerWfctlTools() { mcp.WithString("exclude_rules", mcp.Description("Comma-separated list of rule IDs to skip"), ), - mcp.WithReadOnlyHintAnnotation(false), ), s.handleModernize, ) s.mcpServer.AddTool( mcp.NewTool("registry_search", + toolBehavior(true, false, false), mcp.WithDescription("Search the workflow plugin registry for available plugins. "+ "Returns plugin manifests matching the query, with capabilities, versions, and download information. "+ "Queries the local clone of the GoCodeAlone/workflow-registry repository."), @@ -232,7 +233,6 @@ func (s *Server) registerWfctlTools() { mcp.WithBoolean("include_private", mcp.Description("Include private/proprietary plugins (default: false)"), ), - mcp.WithReadOnlyHintAnnotation(true), ), s.handleRegistrySearch, ) From 38fa0d07bbe251f43994c08b0645ecc9d97d293f Mon Sep 17 00:00:00 2001 From: Jon Langevin Date: Mon, 19 Oct 2026 20:28:29 -0400 Subject: [PATCH 5/5] fix(mcp): describe current validation and compatibility arguments --- docs/mcp-tools-reference.md | 6 +++--- mcp/scaffold_tools.go | 6 +++--- mcp/tools.go | 4 ++-- mcp/wfctl_tools.go | 7 ++++--- 4 files changed, 12 insertions(+), 11 deletions(-) diff --git a/docs/mcp-tools-reference.md b/docs/mcp-tools-reference.md index 35c589b8..8b6105f2 100644 --- a/docs/mcp-tools-reference.md +++ b/docs/mcp-tools-reference.md @@ -73,9 +73,9 @@ but undocumented names do not select alternate behavior. - `modernize` defaults `apply:false`. `apply:true` returns `fixed_yaml` when fixes exist; it does not save a file. `rules` and `exclude_rules` are comma-separated rule IDs. Plugin-declared rules can be read from the startup plugin directory. -- `infer_pipeline_context` requires `pipeline_name`; omit `after_step` for initial - context. Its advertised `openapi_spec` field is currently unused by the - handler, so supplying it does not enrich the result. +- `infer_pipeline_context` requires `pipeline_name`; omitting `after_step` + includes outputs from all pipeline steps. Its advertised `openapi_spec` field + is currently unused by the handler, so supplying it does not enrich the result. - `get_config_examples` lists examples when `name` is omitted. Named examples come from the discovered `example/` catalog; this is not an arbitrary path reader. Catalog contents depend on the server's local installation. diff --git a/mcp/scaffold_tools.go b/mcp/scaffold_tools.go index 97afbfc0..5f5342dd 100644 --- a/mcp/scaffold_tools.go +++ b/mcp/scaffold_tools.go @@ -19,11 +19,11 @@ func (s *Server) registerScaffoldTools() { s.mcpServer.AddTool( mcp.NewTool("scaffold_ci", toolBehavior(true, false, false), - mcp.WithDescription("Generate a ci: YAML section for a workflow config based on the application type. "+ + mcp.WithDescription("Generate a ci: YAML section using application features inferred from the description. "+ "Produces build (binaries/containers/assets), test (unit/integration/e2e), and deploy sub-sections "+ - "with sensible defaults. Pass the existing YAML config and an app description."), + "with sensible defaults."), mcp.WithString("yaml_content", - mcp.Description("Existing workflow YAML config to analyze (optional, used to detect app type)"), + mcp.Description("Compatibility argument for existing workflow YAML, currently ignored; features are inferred from description"), ), mcp.WithString("description", mcp.Required(), diff --git a/mcp/tools.go b/mcp/tools.go index 30a83d99..01d0941b 100644 --- a/mcp/tools.go +++ b/mcp/tools.go @@ -88,10 +88,10 @@ func (s *Server) registerNewTools() { mcp.Description("The name of the pipeline to analyze"), ), mcp.WithString("after_step", - mcp.Description("Step name to analyze context after. Omit to get context at the start of the pipeline."), + mcp.Description("Step name to analyze context after. When omitted, includes outputs from all pipeline steps."), ), mcp.WithString("openapi_spec", - mcp.Description("Optional OpenAPI 3.0 JSON/YAML spec to enrich request body schema context"), + mcp.Description("Compatibility argument, currently ignored; supplying a spec does not enrich request body schema context"), ), ), s.handleInferPipelineContext, diff --git a/mcp/wfctl_tools.go b/mcp/wfctl_tools.go index 3d334bf4..72ee4f37 100644 --- a/mcp/wfctl_tools.go +++ b/mcp/wfctl_tools.go @@ -62,7 +62,7 @@ func (s *Server) registerWfctlTools() { mcp.Description("The YAML content of the new configuration"), ), mcp.WithBoolean("check_breaking", - mcp.Description("Include breaking change detection for stateful modules (default: true)"), + mcp.Description("Compatibility argument, currently ignored; breaking-change detection always runs"), ), ), s.handleDiffConfigs, @@ -118,8 +118,9 @@ func (s *Server) registerWfctlTools() { s.mcpServer.AddTool( mcp.NewTool("template_validate_config", toolBehavior(true, false, false), - mcp.WithDescription("Deep validation of a workflow config against known module types, step types, "+ - "trigger types, dependencies, and template expressions. More comprehensive than validate_config."), + mcp.WithDescription("Validate module types, pipeline step and trigger types, config-key hints, "+ + "and dependencies in a workflow config. Use validate_template_expressions for static "+ + "template-expression and step-reference checks."), mcp.WithString("yaml_content", mcp.Required(), mcp.Description("The YAML content of the workflow configuration"),