Skip to content

docs: define shared CLI and MCP command architecture - #4847

Merged
mnriem merged 19 commits into
github:mainfrom
mnriem:mnriem-mcp-command-architecture
Oct 6, 2026
Merged

mnriem merged 19 commits into
github:mainfrom
mnriem:mnriem-mcp-command-architecture

Conversation

@mnriem

@mnriem mnriem commented Oct 6, 2026 •

Copy link
Copy Markdown
Collaborator

Description

Defines the target command architecture across three authoritative design documents:

  • design/shared.md owns the lean application layer: logical operations, typed requests and outcomes, semantic validation, orchestration, side effects, operation metadata, project-path handling, compatibility, testing, and review rules.
  • design/cli.md owns Typer registration, argument and prompt mapping, CLI-private phases, human/JSON reporting, exit codes, and mirrored CLI tests.
  • design/mcp.md owns first-class MCP tool identity, typed protocol schemas, explicit inventory, standard annotations, non-interactive behavior, protocol mapping, the local stdio server boundary, and MCP-specific tests.

The core invariant is that CLI and MCP are peer delivery adapters over one shared operation. Neither adapter invokes, parses, or reimplements the other. Every CLI leaf receives an explicit MCP inventory disposition; eligible leaves become discoverable, typed MCP tools, while unavailable or excluded leaves remain reviewable. Command-specific contracts stay in the relevant command/domain hierarchy rather than a central service locator or universal execution engine.

The design intentionally does not introduce a universal invocation context, filesystem abstraction, application-resource provider, command runtime, internal MCP policy engine, remote transport abstraction, custom MCP metadata contract, or general command-cancellation contract. The MCP server is local stdio because its operations act on the local project, Specify installation, filesystem, and host tools. Project and target directories are explicit operation inputs; existing Python/domain helpers continue to read distribution metadata and bundled assets. CLI and MCP run with their process user's ordinary permissions. Exact capability and network declarations remain hierarchy-owned architecture metadata for review and parity tests, while MCP adapters project conservative standard ToolAnnotations. Semantic consent such as force or external-source trust remains part of the shared typed request. Deployments requiring confinement sandbox the local MCP server process.

This is normative target architecture documentation, not an implementation or delivery plan. It changes no implementation code, dependencies, or lockfiles. specify mcp is excluded from tool exposure because it starts the local stdio server, but it remains represented in the hierarchy-owned inventory.

AGENTS.md directs future CLI and MCP command changes to the shared architecture and the applicable adapter design.

Testing

  • npx --yes markdownlint-cli2 AGENTS.md design/shared.md design/cli.md design/mcp.md — passed with 0 issues.

  • git diff --check — passed.

  • uv run specify --help — passed.

  • Local Markdown file-and-anchor validation across AGENTS.md, design/shared.md, design/cli.md, and design/mcp.md — passed with no missing files or anchors.

  • Upstream CI passed on commit ea4c338bb72399ad6965e7da8d6403d9d11c5246: analysis, CodeQL, dependency audit, Markdown lint, Ruff, ShellCheck, version-bump, and the full pytest matrix on macOS, Ubuntu, and Windows with Python 3.13 and 3.14.

  • Full pytest was not run locally because this PR changes documentation only and adds no executable behavior; the complete upstream matrix passed.

  • Tested locally with uv run specify --help

  • Ran existing tests with uv sync && uv run pytest (equivalent full upstream pytest matrix passed)

  • Tested with a sample project (if applicable)

AI Disclosure

  • I did not use AI assistance for this contribution
  • I did use AI assistance (fill in the disclosure below)

AI disclosure: GitHub Copilot using GPT-5.6 Sol in autonomous mode (reasoning-effort setting not surfaced) authored and revised the architecture documentation and PR text. No implementation code was generated or changed.

Define the target peer-adapter architecture, typed command contracts, explicit inventory, access policy, transport separation, testing expectations, and incremental migration from the experimental version-only server.

Assisted-by: GitHub Copilot (model: GPT-5.6 Sol, autonomous)

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Copilot AI balanced review requested due to automatic review settings October 6, 2026 12:56

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Copilot review overview

🟡 Changes recommended

The access-policy model does not safely represent operations requiring multiple capabilities and misclassifies check as read-only.

Review effort: Balanced
Findings: 2 High severity

Open (2)
What changed in this PR

Defines the target architecture for exposing Specify operations through MCP.

Changes:

  • Documents shared operations, typed MCP tools, registration, policy, transport, and testing.
  • Records the transitional command inventory and migration plan.
  • Adds contributor and documentation cross-references.
File Description
design/​mcp.md Defines the MCP command architecture.
AGENTS.md Directs contributors to the new design.
docs/​reference/​mcp.md Links current MCP behavior to the target architecture.
docs/​guides/​agentic-sdlc.md Adds MCP to the design-document index.

💡 Configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Comment thread design/mcp.md Outdated
Comment thread design/mcp.md Outdated
Replace the single highest side-effect class with cumulative capability requirements, classify check as requiring execution, and document request-specific capability evaluation.

Assisted-by: GitHub Copilot (model: GPT-5.6 Sol, autonomous)

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Copilot AI balanced review requested due to automatic review settings October 6, 2026 13:11
@mnriem

mnriem commented Oct 6, 2026

Copy link
Copy Markdown
Collaborator Author

Addressed both review findings in commit 3b98377d:

  • Replaced the single highest side-effect class with cumulative capability requirements. Inventory declares the conservative union, request-specific evaluation may narrow it only from validated inputs, and policy must authorize every required capability.
  • Classified check as requiring {local-read, execution} because it launches installed host binaries. The examples also show workflow.run requiring project-write plus execution and init declaring conditional execution.

Validation passed: npx --yes markdownlint-cli2 design/mcp.md and git diff --check.

AI disclosure: GitHub Copilot using GPT-5.6 Sol in autonomous mode (reasoning-effort setting not surfaced) authored the documentation changes and this review-round summary.

Remove the unrequested guide and reference cross-links while retaining the authoritative design document and contributor guidance.

Assisted-by: GitHub Copilot (model: GPT-5.6 Sol, autonomous)

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
@mnriem

mnriem commented Oct 6, 2026

Copy link
Copy Markdown
Collaborator Author

User-directed scope correction in commit f02468d1: removed the unrequested docs/reference/mcp.md and docs/guides/agentic-sdlc.md cross-links. The PR now contains only the authoritative design/mcp.md document and the MCP contributor pointer in AGENTS.md.

Validation passed: npx --yes markdownlint-cli2 design/mcp.md and git diff --check.

AI disclosure: GitHub Copilot using GPT-5.6 Sol in autonomous mode (reasoning-effort setting not surfaced) authored the documentation correction and this PR comment.

Remove current-state and adoption framing so the design document describes only the required architecture and conformance rules.

Assisted-by: GitHub Copilot (model: GPT-5.6 Sol, autonomous)

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
@mnriem

mnriem commented Oct 6, 2026

Copy link
Copy Markdown
Collaborator Author

User-directed design correction in commit 1b015393: removed current-state, transitional, migration, and adoption framing. design/mcp.md now contains only the normative MCP architecture, conformance rules, testing model, and representative layouts.

Validation passed: npx --yes markdownlint-cli2 design/mcp.md, git diff --check, and a content check that rejects transitional/adoption framing.

AI disclosure: GitHub Copilot using GPT-5.6 Sol in autonomous mode (reasoning-effort setting not surfaced) authored the documentation correction and this PR comment.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Copilot review overview

🟡 Changes recommended

Capability authorization ordering has a security gap, and promised documentation cross-references are absent.

Review effort: Balanced
Findings: 1 High severity · 1 Low severity

Open (2)
Resolved since last review (2)

Comment thread design/mcp.md Outdated
Comment thread AGENTS.md Outdated
Copilot AI balanced review requested due to automatic review settings October 6, 2026 13:14

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Copilot review overview

🟡 Changes recommended

The architecture has conflicting ownership and availability contracts and reuses an existing module for an unrelated operation.

Review effort: Balanced
Findings: 1 High severity · 2 Low severity

Open (3)
Previously missed (3)

In code that hasn't changed since last review

Low severity Avoid reusing self-update module for root version operation

design/​mcp.md:247

_version.py is already the version-checking/self-upgrade domain and compatibility surface (src/specify_cli/_version.py:1-9), is imported by both self adapters, and even depends on Typer/console. Reusing it for the root version operation would mix unrelated command domains and contradict the Typer-free shared-operation boundary. Use _operation_version.py consistently here and in the representative layout, or explicitly define how the existing self-update module will be split first.

Low severity Keep exit-code mapping out of shared operation errors

design/​mcp.md:419

exit_code makes the shared operation error own a CLI-only presentation concern, contradicting the earlier rule that command_<name>.py owns exit codes. MCP has no exit-code concept. Keep operation errors transport-neutral and let each CLI adapter map stable operation error codes to its established exit codes.

Low severity Define a mechanism for exposing the CLI contract version

design/​mcp.md:458

No CLI contract-version exposure mechanism is defined: the preceding text places the version in inventory and MCP tool metadata and explicitly forbids injecting it into established CLI JSON, so requiring the CLI adapter to “advertise” it cannot be implemented or tested. State that both adapters conform to the inventory-declared version, or define an actual CLI metadata channel.

Comment thread design/mcp.md Outdated
Authorize capabilities before stateful validation, separate static disposition from runtime policy state, keep version in its own operation module, and make errors and contract versions transport-neutral.

Assisted-by: GitHub Copilot (model: GPT-5.6 Sol, autonomous)

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Copilot AI balanced review requested due to automatic review settings October 6, 2026 13:21
@mnriem

mnriem commented Oct 6, 2026

Copy link
Copy Markdown
Collaborator Author

Addressed review 5428957577 in commit 2fe781c4:

  • Added a capability-free validation and authorization sequence so no state, network, environment, or process capability is exercised before policy approval.
  • Separated static inventory disposition from request-derived runtime effective_state, including the default policy-disabled state for self.upgrade.
  • Kept the user-directed PR scope and corrected description: only design/mcp.md and AGENTS.md are changed.
  • Replaced the unrelated existing _version.py self-update module with a dedicated _operation_version.py in the architecture.
  • Removed CLI exit_code from shared operation errors; CLI adapters own error-code-to-exit-code mapping.
  • Defined the inventory declaration as the contract-version source of truth: MCP exposes it as metadata, while CLI conformance tests bind existing machine output to it without adding a JSON field.

Validation passed: npx --yes markdownlint-cli2 design/mcp.md and git diff --check.

AI disclosure: GitHub Copilot using GPT-5.6 Sol in autonomous mode (reasoning-effort setting not surfaced) authored the documentation changes and this review-round summary.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Copilot review overview

🟡 Changes recommended

Conflicting orchestration ownership and pre-authorization project resolution must be clarified.

Review effort: Balanced
Findings: 1 High severity

Open (1)
Resolved since last review (3)
Previously missed (1)

In code that hasn't changed since last review

Low severity Clarify ownership between CLI commands and shared operations

design/​mcp.md:69

This ownership rule conflicts with the other authoritative architecture: design/cli.md:48-54 says command_<name>.py owns command-specific orchestration, while this document moves orchestration into the shared operation. Every MCP-eligible CLI leaf is subject to both instructions, so contributors cannot satisfy both as written. Update the CLI design (and cross-link the exception) so the shared-operation boundary is unambiguous.

Comment thread design/mcp.md Outdated
Define the common application layer beneath CLI and MCP adapters and narrow each adapter design to its own delivery concerns.

Assisted-by: GitHub Copilot (model: GPT-5.6 Sol, autonomous)

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Copilot AI balanced review requested due to automatic review settings October 6, 2026 13:29
@mnriem mnriem changed the title docs: define MCP command architecture docs: define shared CLI and MCP command architecture Oct 6, 2026
@mnriem

mnriem commented Oct 6, 2026

Copy link
Copy Markdown
Collaborator Author

Implemented the shared application-layer separation in commit 055efbe0e7388ee25ae757bd70ef70fd98894d22.

  • Added design/shared.md as the authoritative source for logical operation identity, typed contracts, validation and authorization lifecycle, invocation context, capabilities, shared testing, anti-patterns, and code-review checks.
  • Narrowed design/cli.md to Typer registration, CLI mapping and presentation, CLI-private phases, and mirrored CLI tests.
  • Narrowed design/mcp.md to tool identity and inventory, MCP policy, non-interactive behavior, protocol projection, transport, and MCP-specific tests.
  • Updated AGENTS.md so CLI and MCP changes require reading the shared design plus the applicable adapter design.

Validation passed: npx --yes markdownlint-cli2 AGENTS.md design/shared.md design/cli.md design/mcp.md (0 issues), git diff --check, uv run specify --help, and local validation of all 18 inter-document file/anchor links.

AI disclosure: Posted on behalf of @mnriem by GitHub Copilot using GPT-5.6 Sol in autonomous mode (reasoning-effort setting not surfaced). Copilot authored the documentation changes and this review-round summary; no implementation code was generated or changed.

Separate the I/O-free pre-authorization context from the canonical authorized operation context and require a post-resolution allowed-root check.

Assisted-by: GitHub Copilot (model: GPT-5.6 Sol, autonomous)

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
@mnriem

mnriem commented Oct 6, 2026

Copy link
Copy Markdown
Collaborator Author

Addressed the project-resolution authorization review in commit db1317868c7ead29ac98f8e4cdcf017e57233025.

The architecture now distinguishes an I/O-free PreAuthorizationContext from the post-authorization AuthorizedOperationContext. The policy layer performs only a preliminary check on the unresolved requested path; canonical project or target resolution requires authorized local-read, followed by a second allowed-root containment check that rejects symlink escapes. The MCP invocation rules, representative artifact list flow, operation tests, and review checklist now enforce this lifecycle.

Validation passed: npx --yes markdownlint-cli2 design/shared.md design/mcp.md (0 issues), git diff --check, and local validation of all 18 inter-document file/anchor links.

AI disclosure: Posted on behalf of @mnriem by GitHub Copilot using GPT-5.6 Sol in autonomous mode (reasoning-effort setting not surfaced). Copilot authored the documentation fix and this review-round summary; no implementation code was generated or changed.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Copilot review overview

🟡 Changes recommended

The CLI design contains conflicting and stale phase-naming guidance.

Review effort: Balanced
Findings: 2 Low severity

Open (2)
Resolved since last review (1)

Comment thread design/cli.md
Comment thread design/cli.md
Copilot AI balanced review requested due to automatic review settings October 6, 2026 13:33
@mnriem

mnriem commented Oct 6, 2026

Copy link
Copy Markdown
Collaborator Author

Addressed review 5432832738 in 531e0fc5dab8c0a39e45a0836e0aed31e8416fa2 after tracing the flagged fields to the earlier policy/runtime design.\n\n- Removed project_scope because typed requests and explicit project/target path rules already own that behavior.\n- Removed default_timeout because the lean design uses stdio protocol deadlines and focused operation/subprocess/network timeouts rather than a universal descriptor timeout.\n- Defined the conservative mapping to standard MCP ToolAnnotations, while keeping exact capability/network declarations as hierarchy-owned architecture metadata rather than inventing a custom _meta contract or mandatory inventory tool.\n- Removed speculative HTTP/SSE support and transport abstractions. The architecture now specifies a local stdio server only because operations act directly on local projects, installation state, files, and host tools.\n\nLocal validation passed: Markdown lint (0 issues), git diff --check, uv run specify --help, and Markdown link/anchor validation.\n\nAI disclosure: This comment and the referenced documentation revision were authored by GitHub Copilot using GPT-5.6 Sol in autonomous mode (reasoning-effort setting not surfaced). No implementation code was generated or changed.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Copilot review overview

🟡 Changes recommended

Contract-version exposure is undefined, and the CLI checklist contradicts its CLI-only exception.

Review effort: Balanced
Findings: 2 Low severity

Open (2)
Previously missed (1)

In code that hasn't changed since last review

Low severity Scope shared-operation requirements to multi-adapter commands

design/​cli.md:417

This checklist is unqualified, but lines 7 and 67-69 explicitly allow a CLI-only command to keep small behavior in its command module until another adapter needs it. Requiring every new or refactored command to map to a shared operation contradicts that lean-boundary exception. Scope these items to leaves with another delivery adapter (or remove the earlier exception).

Comment thread design/mcp.md Outdated
Comment thread design/shared.md Outdated
Keep operation contract versions internal to hierarchy inventories and tests, and scope shared-operation checklist requirements to multi-adapter commands.

Assisted-by: GitHub Copilot (model: GPT-5.6 Sol, autonomous)

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Copilot AI balanced review requested due to automatic review settings October 6, 2026 19:28
@mnriem

mnriem commented Oct 6, 2026

Copy link
Copy Markdown
Collaborator Author

Addressed the follow-up review in 2369c5554abc97bc73e83e0ef9ce3c3336dfc7c5 without adding new protocol machinery.\n\n- contract_version is now explicitly hierarchy-owned source/inventory metadata for compatibility and adapter contract tests, not a client-visible MCP field.\n- The CLI review checklist now scopes shared-operation requirements to multi-adapter commands, preserving the documented exception for small CLI-only behavior.\n\nLocal validation passed: Markdown lint (0 issues), git diff --check, and uv run specify --help.\n\nAI disclosure: This comment and the referenced documentation revision were authored by GitHub Copilot using GPT-5.6 Sol in autonomous mode (reasoning-effort setting not surfaced). No implementation code was generated or changed.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Copilot review overview

🔵 Needs a closer look

The documented deadline and cancellation ownership conflicts with MCP stdio protocol semantics.

Review effort: Balanced
Findings: None

Resolved since last review (2)
Previously missed (2)

In code that hasn't changed since last review

Low severity Cancellation handling may send responses after client cancellation

design/​mcp.md:435

This conflicts with MCP cancellation semantics: request deadlines are tracked by the client/host, and after a stdio notifications/cancelled notification the server should stop work and send no response. Requiring a structured cancellation error can produce a response after the caller has cancelled and stopped waiting. Separate client-owned protocol cancellation from server-enforced operation timeouts.

Low severity Adapter incorrectly owns MCP request deadlines

design/​shared.md:345

MCP request deadlines belong to the request sender, not the server adapter. On stdio, a timed-out client sends notifications/cancelled; the server handles that signal and should stop without responding. Assigning “protocol deadlines” to the adapter gives future implementations the wrong lifecycle contract. Limit this responsibility to handling cancellation notifications and server-side output limits.

Separate client-owned stdio cancellation from server-enforced operation timeouts and document the no-response cancellation behavior.

Assisted-by: GitHub Copilot (model: GPT-5.6 Sol, autonomous)

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Copilot AI balanced review requested due to automatic review settings October 6, 2026 19:42
@mnriem

mnriem commented Oct 6, 2026

Copy link
Copy Markdown
Collaborator Author

Addressed the cancellation/deadline lifecycle findings in 9581d3cfec753619e269d0f8a8d0386f6dbc6d81. The design now distinguishes client-owned MCP request deadlines from server-owned operation timeouts: notifications/cancelled stops cooperative work and produces no response, while a server-enforced subprocess/network/operation timeout on an active request returns a structured timeout error. No universal invocation context was introduced.\n\nLocal validation passed: Markdown lint (0 issues), git diff --check, and uv run specify --help.\n\nAI disclosure: This comment and the referenced documentation revision were authored by GitHub Copilot using GPT-5.6 Sol in autonomous mode (reasoning-effort setting not surfaced). No implementation code was generated or changed.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Copilot review overview

🔵 Needs a closer look

MCP annotation and cancellation rules contain unresolved contract ambiguities.

Review effort: Balanced
Findings: None

Previously missed (1)

In code that hasn't changed since last review

Low severity Clarify destructiveHint behavior for read-only tools

design/​mcp.md:376

This mapping does not define destructiveHint for read-only tools: the first sentence implies it is not true, while the second permits false only for additive updates. Tools such as version and artifact.list perform no update at all, so implementers and the promised exact-annotation tests cannot tell whether to emit or omit the field. Define the read-only case explicitly.

This issue also appears on line 436 of the same file.

Specify that destructiveHint is omitted for read-only tools and is otherwise set from destructive versus additive behavior.

Assisted-by: GitHub Copilot (model: GPT-5.6 Sol, autonomous)

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Copilot AI balanced review requested due to automatic review settings October 6, 2026 19:56
@mnriem

mnriem commented Oct 6, 2026

Copy link
Copy Markdown
Collaborator Author

Clarified the final annotation edge case in 8571b4bdad6cca03628b11fe474fcc77a86bdd9f: destructiveHint is omitted when readOnlyHint is true; for non-read-only tools it is true for destructive updates and false only for additive updates.\n\nLocal validation passed: npx --yes markdownlint-cli2 design/mcp.md and git diff --check.\n\nAI disclosure: This comment and the referenced documentation revision were authored by GitHub Copilot using GPT-5.6 Sol in autonomous mode (reasoning-effort setting not surfaced). No implementation code was generated or changed.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Copilot review overview

🔵 Needs a closer look

The CLI review checklist does not enforce the required MCP inventory disposition for every CLI leaf.

Review effort: Balanced
Findings: None

Previously missed (1)

In code that hasn't changed since last review

Low severity Require MCP inventory records for every new CLI leaf

design/​cli.md:416

This checklist can approve a newly added CLI leaf without recording the MCP disposition required by design/mcp.md:241-243: the next item applies only when another adapter already exposes the operation, but excluded and unavailable leaves also need inventory records. Add an unconditional inventory check so contributors following this CLI design cannot silently omit new leaves.

Make every CLI leaf record an MCP disposition even when the operation remains unavailable or excluded.

Assisted-by: GitHub Copilot (model: GPT-5.6 Sol, autonomous)

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Copilot AI balanced review requested due to automatic review settings October 6, 2026 20:08
@mnriem

mnriem commented Oct 6, 2026

Copy link
Copy Markdown
Collaborator Author

Addressed review 5433931725 in f25413f9c0c7d752a439bd3a184ea22d2dbe69d7. The CLI checklist now unconditionally requires exactly one hierarchy-owned MCP inventory disposition for every CLI leaf, including leaves that remain unavailable or are explicitly excluded. This preserves the lean CLI-only implementation exception while preventing silent inventory omissions.\n\nLocal validation passed: npx --yes markdownlint-cli2 design/cli.md and git diff --check.\n\nAI disclosure: This comment and the referenced documentation revision were authored by GitHub Copilot using GPT-5.6 Sol in autonomous mode (reasoning-effort setting not surfaced). No implementation code was generated or changed.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Copilot review overview

🔵 Needs a closer look

The cancellation contract promises stopping non-cooperative work despite explicitly making cancellation support optional.

Review effort: Balanced
Findings: None

Previously missed (1)

In code that hasn't changed since last review

Low severity Clarify stop guarantees for non-cooperative operations

design/​mcp.md:438

This unconditional stop guarantee conflicts with lines 429–430 and the protocol-test requirement at line 508, which limit cancellation to cooperative operations. Non-cooperative work (especially an already-running subprocess or mutation) can continue after the adapter suppresses its response, so implementations cannot satisfy this rule as written. Either require every exposed operation to be cancellable, or scope the stop guarantee to cooperative work and document that other work remains bounded by its own completion/timeout.

Keep the local stdio architecture limited to explicit operation timeouts and bounded output rather than promising command-level cancellation.

Assisted-by: GitHub Copilot (model: GPT-5.6 Sol, autonomous)

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Copilot AI balanced review requested due to automatic review settings October 6, 2026 20:15
@mnriem

mnriem commented Oct 6, 2026

Copy link
Copy Markdown
Collaborator Author

Addressed review 5434051868 in ea4c338bb72399ad6965e7da8d6403d9d11c5246 by removing cancellation from the command architecture entirely.\n\nThis was overreach: MCP can carry cancellation notifications over stdio, but the current local server has no command-level cancellation contract, and filesystem mutations or running subprocesses cannot be assumed to stop cooperatively. The design now promises only explicit operation/subprocess/network timeouts, non-interactive stdin behavior, and bounded output.\n\nLocal validation passed: Markdown lint (0 issues), git diff --check, and uv run specify --help.\n\nAI disclosure: This comment and the referenced documentation revision were authored by GitHub Copilot using GPT-5.6 Sol in autonomous mode (reasoning-effort setting not surfaced). No implementation code was generated or changed.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Copilot review overview

🟢 Approval recommended

The documentation is internally consistent, complete, and introduces no executable behavior requiring tests.

Review effort: Balanced
Findings: None

@mnriem
mnriem merged commit 4e1b814 into github:main Oct 6, 2026
15 checks passed
@mnriem
mnriem deleted the mnriem-mcp-command-architecture branch October 6, 2026 20:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants