From b4c6387eee9389fdfb87960c27c8f507abb9179c Mon Sep 17 00:00:00 2001 From: Daniel Petty Date: Thu, 5 Nov 2026 15:25:41 +0000 Subject: [PATCH 1/4] fix(mcp): a read-only tool has no write option The exposure table marked pr check-readiness, roadmap refine and tf plan as read-only. Each has an option that writes to GitHub or to a file. Mark the three tools not read-only. Drop the sarif option from the scan injection tool, which stays read-only. A new test fails on a read-only tool with an option whose description holds resolve, write, delete or create. Refs #3235 --- docs/MCP_TOOLS.md | 13 +- .../.openspec.yaml | 2 + .../design.md | 19 ++ .../proposal.md | 44 +++++ .../specs/mcp-role-server/spec.md | 46 +++++ .../tasks.md | 27 +++ src/devops_cli/ai/mcp/generator.py | 15 ++ src/devops_cli/config/constants.py | 12 +- .../mcp/test_mcp_read_only_write_options.py | 162 ++++++++++++++++++ 9 files changed, 330 insertions(+), 10 deletions(-) create mode 100644 openspec/changes/3235-read-only-mcp-tools-have-no-write-option/.openspec.yaml create mode 100644 openspec/changes/3235-read-only-mcp-tools-have-no-write-option/design.md create mode 100644 openspec/changes/3235-read-only-mcp-tools-have-no-write-option/proposal.md create mode 100644 openspec/changes/3235-read-only-mcp-tools-have-no-write-option/specs/mcp-role-server/spec.md create mode 100644 openspec/changes/3235-read-only-mcp-tools-have-no-write-option/tasks.md create mode 100644 tests/ai/mcp/test_mcp_read_only_write_options.py diff --git a/docs/MCP_TOOLS.md b/docs/MCP_TOOLS.md index bd416f53d..8354631ef 100644 --- a/docs/MCP_TOOLS.md +++ b/docs/MCP_TOOLS.md @@ -89,7 +89,7 @@ The `devops-cli` FastMCP server exposes DevOps automation and AI review capabili | [`k8s_teardown_stack`](#k8s-teardown-stack) | `false` | Uninstall the k8s infrastructure / LLM stack and delete namespaces. | | [`k8s_tempo_info`](#k8s-tempo-info) | `true` | Retrieve Tempo distributed tracing HTTP API URL and OTLP trace endpoints. | | [`k8s_validate`](#k8s-validate) | `true` | Validate Kubernetes YAML manifests against OpenAPI schemas using Kubeconform. | -| [`pr_check_readiness`](#pr-check-readiness) | `true` | Validate PR merge readiness: conflicts, draft state, checks, review threads, grounding and draft readiness. Grounding applies where the base holds openspec/changes/. The PR targets the default branch. It closes exactly one issue, an open issue of its own repository that holds a priority/* label: a roadmap item. GitHub closes that issue when the PR merges. The PR adds openspec/changes/archive/\-\-\/, which uv run devops openspec archive writes for that issue's change. Only that archive writes openspec/specs/ and openspec/changes/archive/. An implement pull request must have every task group ticked. | +| [`pr_check_readiness`](#pr-check-readiness) | `false` | Validate PR merge readiness: conflicts, draft state, checks, review threads, grounding and draft readiness. Grounding applies where the base holds openspec/changes/. The PR targets the default branch. It closes exactly one issue, an open issue of its own repository that holds a priority/* label: a roadmap item. GitHub closes that issue when the PR merges. The PR adds openspec/changes/archive/\-\-\/, which uv run devops openspec archive writes for that issue's change. Only that archive writes openspec/specs/ and openspec/changes/archive/. An implement pull request must have every task group ticked. | | [`pr_checks`](#pr-checks) | `true` | Check remote CI quality gate status on a pull request. | | [`pr_close`](#pr-close) | `false` | Close a pull request. | | [`pr_describe`](#pr-describe) | `true` | Print the title and the body of the pull request that delivers an issue. The first line is the title, then a blank line, then the body. | @@ -112,7 +112,7 @@ The `devops-cli` FastMCP server exposes DevOps automation and AI review capabili | [`review_pr`](#review-pr) | `false` | Review a GitHub pull request. | | [`review_stats`](#review-stats) | `true` | Compute and display review accuracy statistics across saved sessions. | | [`roadmap_intake`](#roadmap-intake) | `false` | Turn each open issue with no priority label into a roadmap item, or file a new candidate: intake sets the labels, adds a reason comment and names a possible duplicate for a person to close. It writes nothing without --confirm. Details: docs/roadmap/intake.md. | -| [`roadmap_refine`](#roadmap-refine) | `true` | Refine roadmap items to Ready: investigate open questions with tools that read the code and the documentation, then write the design, tasks and acceptance criteria to GitHub. Without --confirm, refine prints its plan only. Details: docs/roadmap/refine.md. | +| [`roadmap_refine`](#roadmap-refine) | `false` | Refine roadmap items to Ready: investigate open questions with tools that read the code and the documentation, then write the design, tasks and acceptance criteria to GitHub. Without --confirm, refine prints its plan only. Details: docs/roadmap/refine.md. | | [`roadmap_run`](#roadmap-run) | `true` | Run the roadmap jobs that are due, in order: metrics, then intake, refine and the alternatives check, and record each one's last success. Intake is due on an issue reopened, and decides at most 5 candidates a run and keeps a record of those it left beside the schedule; refine is due every 31 minutes, on a first run, and after an intake that placed a critical fix or a P0 item; the alternatives check is due every 719 minutes and on a first run, and checks at most roadmap.alternatives.first_pass_limit items while an item has no check, and roadmap.alternatives.limit items after. Without --confirm, or with --dry-run, prints the due list and runs nothing. | | [`sandbox_deploy`](#sandbox-deploy) | `false` | Deploy an isolated background container sandbox with security containment. | | [`sandbox_exec`](#sandbox-exec) | `false` | Execute a command inside an active sandbox container. | @@ -140,7 +140,7 @@ The `devops-cli` FastMCP server exposes DevOps automation and AI review capabili | [`tf_cost_breakdown`](#tf-cost-breakdown) | `true` | Estimate monthly and hourly cloud infrastructure costs using Infracost. | | [`tf_notify_plan`](#tf-notify-plan) | `true` | Format and post structured, collapsible OpenTofu/Terraform plan diffs to PR comments. | | [`tf_output`](#tf-output) | `true` | Read an output variable from the OpenTofu state. | -| [`tf_plan`](#tf-plan) | `true` | Generate and show an OpenTofu execution plan. | +| [`tf_plan`](#tf-plan) | `false` | Generate and show an OpenTofu execution plan. | | [`tls_inspect`](#tls-inspect) | `true` | Inspect and display metadata of an X.509 certificate. | | [`valkey_flush`](#valkey-flush) | `false` | Flush and purge keys from current or all databases. | | [`valkey_get`](#valkey-get) | `true` | Retrieve string value stored at key. | @@ -1443,7 +1443,7 @@ adds openspec/changes/archive/\-\-\/, which uv run devops writes for that issue's change. Only that archive writes openspec/specs/ and openspec/changes/archive/. An implement pull request must have every task group ticked. -**Annotations:** `readOnlyHint: true` +**Annotations:** `readOnlyHint: false` **Parameters:** @@ -1884,7 +1884,7 @@ Turn each open issue with no priority label into a roadmap item, or file a new c Refine roadmap items to Ready: investigate open questions with tools that read the code and the documentation, then write the design, tasks and acceptance criteria to GitHub. Without --confirm, refine prints its plan only. Details: docs/roadmap/refine.md. -**Annotations:** `readOnlyHint: true` +**Annotations:** `readOnlyHint: false` **Parameters:** @@ -2057,7 +2057,6 @@ Scan target files for prompt-injection smuggling vectors and canary tokens. | Parameter | Type | Required | Default | Description | |---|---|---|---|---| | `target` | `string` | No | `.` | Target file or directory to scan for prompt-injection smuggling vectors. | -| `sarif` | `string` | No | - | Write findings to this path as a SARIF 2.1.0 document. | | `min_severity` | `string` | No | - | Drop findings below this severity (CRITICAL|HIGH|MEDIUM|LOW|INFO). | | `suppress` | `string` | No | - | Suppression policy file; inherited policies are resolved via 'extends'. | | `show_suppressed` | `boolean` | No | `False` | List findings hidden by the suppression policy and the rule that hid them. | @@ -2306,7 +2305,7 @@ Read an output variable from the OpenTofu state. Generate and show an OpenTofu execution plan. -**Annotations:** `readOnlyHint: true` +**Annotations:** `readOnlyHint: false` **Parameters:** diff --git a/openspec/changes/3235-read-only-mcp-tools-have-no-write-option/.openspec.yaml b/openspec/changes/3235-read-only-mcp-tools-have-no-write-option/.openspec.yaml new file mode 100644 index 000000000..1491b921e --- /dev/null +++ b/openspec/changes/3235-read-only-mcp-tools-have-no-write-option/.openspec.yaml @@ -0,0 +1,2 @@ +schema: spec-driven +created: 2026-11-05 diff --git a/openspec/changes/3235-read-only-mcp-tools-have-no-write-option/design.md b/openspec/changes/3235-read-only-mcp-tools-have-no-write-option/design.md new file mode 100644 index 000000000..c57891625 --- /dev/null +++ b/openspec/changes/3235-read-only-mcp-tools-have-no-write-option/design.md @@ -0,0 +1,19 @@ +# Design + +## Decisions + +- **The test scans the description of each option in the published schema of each read-only tool, for `resolve`, `write`, `delete` and `create` in the base, `-s` and `-ing` forms.**: The issue names these verbs and names the help text as the signal. The generator copies the help text into the description of the published parameter, so the schema is what a model reads. The test names the tool and the option. +- **A past form of a verb does not match.**: `resolved` in `sandbox_network_policy.public_whitelist`, `sandbox_network_policy.local_whitelist` and `scan_injection.suppress`, and `wrote` in `roadmap_refine.item`, describe a result and write nothing. A match on them would need an exempt list, and an exempt list hides the error that the test must show. +- **`pr check-readiness`, `roadmap refine` and `tf plan` are marked not read-only, and each command keeps its option.**: `--auto-resolve` and `--confirm` write to GitHub. `--out` writes the plan file that `tf_apply` takes as `plan_file`, so the option has a use for a model. The flag then says what the tool can do. No job role table names these tools, so no role loses a tool. +- **`scan_injection` stays read-only and its schema drops `sarif`.**: The command prints its findings, and a SARIF file is for a CI upload that a person starts. The dry-run class of the command is `no-external-request`, and `test_mcp_exposure_table_no_external_request_read_only` fails when the tool is not read-only. The restriction follows the rule of `ai_ast_graph` and `scan_sbom`, which leave out a file option that the model does not need. +- **The verb pattern is a private constant of the test.**: Only the test reads it, so CONTRIBUTING.md section 5 puts it in that module. +- **The test builds a one-tool server for each case of the rule, with the signature that the generator makes.**: The rule then runs on the same kind of object that the repository check reads, and no double replaces a tool. + +## Rejected alternatives + +- **Mark only `pr check-readiness` and add no general check.**: The scan finds three more tools of the same kind. Acceptance criterion 1 of the issue says that no tool with a write option is read-only. +- **Remove `auto_resolve` from the published schema of `pr_check_readiness`.**: `_PATH_PARAM_RESTRICTIONS` takes an `allowed` list only. The entry must list the seven other options, and it drops each option that the command adds later. The option is also useful for a model that has the write right. +- **Scan option names, not descriptions.**: `--confirm`, `--out` and `--sarif` have names with no write verb, and the check would miss three of the four tools. +- **Widen the verbs to `post`, `execute`, `apply`, `restore` and `close`.**: These words find `roadmap_run.confirm` (`Execute`), `tf_notify_plan.pr_number` (`post`) and `ai_gateway_failover.reset` (`Restore`). They also match words with no write: `sandbox_status.all_instances` (`Apply operation across all registered sandbox instances`), `ci.check` (`applying`), `gh_issues_list.state` (`closed`) and `pr_describe.issue` (`closes`). A word list cannot tell the two groups apart. A person must read each flag. The issue names four verbs, so this change keeps them. +- **Mark `scan_injection` not read-only.**: The dry-run ratchet test fails with that flag (see Feasibility in the proposal). +- **Keep the old flag and rely on the description of the tool.**: A role server reads the flag and not the text, so the text does not stop a write. diff --git a/openspec/changes/3235-read-only-mcp-tools-have-no-write-option/proposal.md b/openspec/changes/3235-read-only-mcp-tools-have-no-write-option/proposal.md new file mode 100644 index 000000000..95f168fa2 --- /dev/null +++ b/openspec/changes/3235-read-only-mcp-tools-have-no-write-option/proposal.md @@ -0,0 +1,44 @@ +# Proposal + +## Why + +Issue #3235 reports that `CONST_MCP_EXPOSURE_TABLE` marks `pr_check_readiness` as read-only. The command has the option `--auto-resolve`, which resolves review threads on GitHub. A role server and a client choose tools by the read-only flag. So a model can get a write that the flag says it cannot make. No test finds this kind of error. + +The check that the issue asks for finds three more tools of the same kind. `roadmap_refine` has `--confirm`, which writes the proposed designs to GitHub. `tf_plan` has `--out`, which writes the plan to a file. `scan_injection` has `--sarif`, which writes the findings to a file. All three are marked read-only. + +## What Changes + +- The `pr check-readiness`, `roadmap refine` and `tf plan` rows of `CONST_MCP_EXPOSURE_TABLE` in `src/devops_cli/config/constants.py` are not read-only. Each command keeps its option for people at the command line. +- `_PATH_PARAM_RESTRICTIONS` in `src/devops_cli/ai/mcp/generator.py` gets an entry for `("scan", "injection")` that leaves out `sarif`. The command prints its findings, so the model needs no file. The tool stays read-only, because the dry-run class of the command is `no-external-request`, and that class requires a read-only tool. +- A new test, `tests/ai/mcp/test_mcp_read_only_write_options.py`, reads the published schema of every tool. It fails on a read-only tool that has an option whose description holds `resolve`, `write`, `delete` or `create` in its base, `-s` or `-ing` form, and it names the tool and the option. A past form such as `resolved` does not count. +- `docs/MCP_TOOLS.md` is regenerated: four flags change and the `sarif` row of `scan_injection` goes. +- The change alters the flag and the schema that an MCP client reads, so it carries a spec delta and `skip_specs` is false. +- The change fits one pull request. + +## Capabilities + +### New Capabilities + +None. + +### Modified Capabilities + +- `mcp-role-server`: A read-only tool publishes no option that writes, A tool that keeps a writing option is not read-only, The injection scan tool publishes no SARIF option + +## Impact + +- Code: `src/devops_cli/config/constants.py`, `src/devops_cli/ai/mcp/generator.py` +- Documents: `docs/MCP_TOOLS.md` +- Tests: `tests/ai/mcp/test_mcp_read_only_write_options.py` (new) + +## Feasibility + +- Command output at `origin/main` commit 66a8a63e. A script that lists the published tools of `devops_cli.ai.mcp.server.mcp` shows `pr_check_readiness` with `readOnlyHint` true and the parameters `number`, `allow_draft`, `allow_pending_checks`, `allow_blocked_state`, `auto_resolve`, `allow_replied_threads`, `repo` and `dry_run`. The description of `auto_resolve` is "Automatically resolve review discussion threads that have received a reply from someone other than the thread opener." So the premise of the issue holds. The stage comment of 2026-11-02 on the issue says that the premise did not hold, but it gives no evidence. +- Command output, `uv run devops pr check-readiness --help`, lists `--auto-resolve`. The row in `src/devops_cli/config/constants.py` was `(("pr", "check-readiness"), 60.0, True, False)`. +- The same script, with the four verbs of the issue in the base, `-s` and `-ing` forms, finds four read-only options: `pr_check_readiness.auto_resolve`, `roadmap_refine.confirm`, `scan_injection.sarif` and `tf_plan.out`. With every form of the verbs, it also finds `roadmap_refine.item` (`wrote`), `sandbox_network_policy.public_whitelist` and `.local_whitelist` (`resolved`) and `scan_injection.suppress` (`resolved`). Each of these four describes a result, not a write. So the rule leaves out the past forms. +- When the `scan injection` row was set to not read-only, `tests/invariants/test_dry_run_class_ratchet.py::test_mcp_exposure_table_no_external_request_read_only` failed with "MCP tool for 'scan injection' has read_only=False but command is 'no-external-request'". The command prints its findings without `--sarif`, so the tool drops that option. +- `src/devops_cli/ai/mcp/generator.py` builds the description of a parameter from the help text of the Click option (`_build_parameter_field`), so the schema carries the same words as the help. + +## Existing Tool Considered + +No open, maintained tool covers the need. The alternatives check of 2026-11-05 on the issue found none: MCP security scanners validate annotation fields against each other or scan a running server, and none reads `CONST_MCP_EXPOSURE_TABLE` or the option help text of a CLI-derived tool. The change builds one repository-specific test. The written cost is about 30,000 tokens to build it, about 3,000 tokens per year to keep and fix it, and about two hours elapsed. The change deletes no internal code. diff --git a/openspec/changes/3235-read-only-mcp-tools-have-no-write-option/specs/mcp-role-server/spec.md b/openspec/changes/3235-read-only-mcp-tools-have-no-write-option/specs/mcp-role-server/spec.md new file mode 100644 index 000000000..fe0459a0e --- /dev/null +++ b/openspec/changes/3235-read-only-mcp-tools-have-no-write-option/specs/mcp-role-server/spec.md @@ -0,0 +1,46 @@ +## ADDED Requirements + +### Requirement: A read-only tool publishes no option that writes +A tool that the exposure table marks read-only SHALL publish no option whose description holds `resolve`, `write`, `delete` or `create` as a whole word, in its base, `-s` or `-ing` form and in any letter case. A past form such as `resolved` SHALL NOT count. + +#### Scenario: A read-only tool with a writing option +- **WHEN** A test reads the published schema of a read-only tool, and one option description is `Write findings to this path.` +- **THEN** The test reports the tool and the option. + +#### Scenario: A tool that is not read-only +- **WHEN** A test reads the published schema of a tool that is not read-only, and one option description is `Automatically resolve review threads.` +- **THEN** The test reports nothing. + +#### Scenario: A past form of a write verb +- **WHEN** A test reads the published schema of a read-only tool, and one option description is `Inherited policies are resolved via 'extends'.` +- **THEN** The test reports nothing. + +#### Scenario: The published tools +- **WHEN** A test reads the published schema of every tool that the server registers +- **THEN** No read-only tool has an option that the rule reports. + +### Requirement: A tool that keeps a writing option is not read-only +The tools `pr_check_readiness`, `roadmap_refine` and `tf_plan` SHALL be marked not read-only, and SHALL publish the option `auto_resolve`, `confirm` and `out` respectively. The command `devops pr check-readiness` SHALL keep the option `--auto-resolve`. + +#### Scenario: The readiness tool +- **WHEN** A test reads the published tool `pr_check_readiness` +- **THEN** Its read-only flag is false, and its schema holds `auto_resolve`. + +#### Scenario: The refine and plan tools +- **WHEN** A test reads the published tools `roadmap_refine` and `tf_plan` +- **THEN** Both read-only flags are false, and the schemas hold `confirm` and `out`. + +#### Scenario: The readiness command +- **WHEN** A test reads the options of `devops pr check-readiness` +- **THEN** The options include `--auto-resolve`. + +### Requirement: The injection scan tool publishes no SARIF option +The tool `scan_injection` SHALL be marked read-only and SHALL publish no `sarif` option. The command `devops scan injection` SHALL keep the option `--sarif`. + +#### Scenario: The published injection scan tool +- **WHEN** A test reads the published tool `scan_injection` +- **THEN** Its read-only flag is true, and its schema holds no `sarif` property. + +#### Scenario: The injection scan command +- **WHEN** A test reads the options of `devops scan injection` +- **THEN** The options include `--sarif`. diff --git a/openspec/changes/3235-read-only-mcp-tools-have-no-write-option/tasks.md b/openspec/changes/3235-read-only-mcp-tools-have-no-write-option/tasks.md new file mode 100644 index 000000000..d7e944d25 --- /dev/null +++ b/openspec/changes/3235-read-only-mcp-tools-have-no-write-option/tasks.md @@ -0,0 +1,27 @@ +# Tasks + +## 1. Report a write option of a read-only tool and fix the four tools + +Reads: `src/devops_cli/config/constants.py`, `src/devops_cli/ai/mcp/generator.py`, `tests/ai/mcp/test_path_confinement.py`, `tests/ai/mcp/test_mcp_tool_secrets.py`. Changes: `tests/ai/mcp/test_mcp_read_only_write_options.py` (new), `src/devops_cli/config/constants.py`, `src/devops_cli/ai/mcp/generator.py`. + +- [x] 1.1 Write `tests/ai/mcp/test_mcp_read_only_write_options.py` with the cases of the rule on a one-tool server (a read-only tool with a writing option, each verb in each form, a tool that is not read-only, a past form, a word that only holds a verb), the check on the published schema of every tool, the flag and option of `pr_check_readiness`, `roadmap_refine` and `tf_plan`, and the schema of `scan_injection` + - Verification: `uv run devops ci test tests/ai/mcp/test_mcp_read_only_write_options.py` fails on the base +- [x] 1.2 Set the `pr check-readiness`, `roadmap refine` and `tf plan` rows of `CONST_MCP_EXPOSURE_TABLE` to not read-only, with a comment that names the writing option. Add the `("scan", "injection")` entry to `_PATH_PARAM_RESTRICTIONS` with every option but `sarif` + - Verification: `uv run devops ci test tests/ai/mcp/test_mcp_read_only_write_options.py tests/invariants/test_dry_run_class_ratchet.py tests/ai/mcp/test_mcp.py tests/ai/mcp/test_fastmcp_contracts.py` passes + +## 2. Regenerate the MCP tool reference + +Reads: `docs/MCP_TOOLS.md`. Changes: `docs/MCP_TOOLS.md`. + +- [x] 2.1 Run `uv run devops docs generate --sync-readme`. The four flags change and the `sarif` row of `scan_injection` goes + - Verification: `uv run devops ci docs` passes + +## Criteria + +- A read-only tool publishes no option that writes: group 1 +- A tool that keeps a writing option is not read-only: group 1 +- The injection scan tool publishes no SARIF option: group 1 + +## Workflow follow-up + +- Pending a person: review the diff in tuicr (tuicr -r ..HEAD). diff --git a/src/devops_cli/ai/mcp/generator.py b/src/devops_cli/ai/mcp/generator.py index 47cd4cedb..b57368b25 100644 --- a/src/devops_cli/ai/mcp/generator.py +++ b/src/devops_cli/ai/mcp/generator.py @@ -86,6 +86,21 @@ "allowed": frozenset({"target", "format_type"}), "required": frozenset(), }, + # No `sarif`: the command prints its findings, so the model needs no file (#3235). + ("scan", "injection"): { + "allowed": frozenset( + { + "target", + "min_severity", + "suppress", + "show_suppressed", + "fail_on", + "output_format", + "json_output", + } + ), + "required": frozenset(), + }, # No `output`: the command prints the stream, so the model needs no file. ("k8s", "security-stream"): { "allowed": frozenset( diff --git a/src/devops_cli/config/constants.py b/src/devops_cli/config/constants.py index b03a5437c..ad05732ae 100644 --- a/src/devops_cli/config/constants.py +++ b/src/devops_cli/config/constants.py @@ -265,6 +265,9 @@ CONST_MCP_EAGER_DOMAINS | CONST_MCP_LAZY_DOMAINS | {"github", "secrets", "benchmarks"} ) # Command exposure table: (command_path, timeout_seconds, read_only, format_json) +# A command that has an option to write is not read-only, because a role server and a client choose +# tools by that flag. `tests/ai/mcp/test_mcp_read_only_write_options.py` finds such an option in +# the published schema (#3235). CONST_MCP_EXPOSURE_TABLE: Final[tuple[tuple[tuple[str, ...], float, bool, bool], ...]] = ( (("ai", "ast", "graph"), 60.0, True, False), (("ai", "ast", "parse"), 60.0, True, True), @@ -348,7 +351,8 @@ (("k8s", "status"), 60.0, True, False), (("k8s", "teardown-stack"), 180.0, False, False), (("k8s", "validate"), 60.0, True, False), - (("pr", "check-readiness"), 60.0, True, False), + # `--auto-resolve` resolves review threads on GitHub (#3235). + (("pr", "check-readiness"), 60.0, False, False), (("pr", "checks"), 60.0, True, False), (("pr", "close"), 60.0, False, False), (("pr", "describe"), 60.0, True, False), @@ -371,7 +375,8 @@ (("review", "pr"), 300.0, False, False), (("review", "stats"), 60.0, True, False), (("roadmap", "intake"), 300.0, False, False), - (("roadmap", "refine"), 300.0, True, False), + # `--confirm` writes the proposed designs to GitHub (#3235). + (("roadmap", "refine"), 300.0, False, False), (("roadmap", "run"), 300.0, True, False), (("sandbox", "deploy"), 300.0, False, False), (("sandbox", "exec"), 300.0, False, False), @@ -396,7 +401,8 @@ (("tf", "cost", "breakdown"), 300.0, True, True), (("tf", "notify-plan"), 60.0, True, False), (("tf", "output"), 30.0, True, False), - (("tf", "plan"), 300.0, True, False), + # `--out` writes the plan to a file (#3235). + (("tf", "plan"), 300.0, False, False), (("tls", "inspect"), 30.0, True, False), (("valkey", "flush"), 30.0, False, False), (("valkey", "get"), 30.0, True, False), diff --git a/tests/ai/mcp/test_mcp_read_only_write_options.py b/tests/ai/mcp/test_mcp_read_only_write_options.py new file mode 100644 index 000000000..50fbe29ac --- /dev/null +++ b/tests/ai/mcp/test_mcp_read_only_write_options.py @@ -0,0 +1,162 @@ +"""A read-only MCP tool publishes no option that writes (#3235). + +A role server and a client choose tools by the `readOnlyHint` of the exposure table. A tool that +the table marks read-only but that has an option to resolve, write, delete or create would give +a model a write that the flag says it cannot make. The help text of an option is the signal: the +generator copies it into the description of the published parameter. +""" + +from __future__ import annotations + +import asyncio +import inspect +import re +from collections.abc import Callable, Sequence +from typing import Annotated + +import pytest +from fastmcp import FastMCP +from fastmcp.tools import Tool +from pydantic import Field + +from devops_cli.ai.mcp.generator import resolve_click_command +from devops_cli.ai.mcp.server import mcp + +# The write verbs of the issue, in the forms that say what an option does: the base form, the +# `-s` form and the `-ing` form. A past form ("resolved via 'extends'") describes a result, not a +# write, so it does not match. +_WRITE_STEMS = ("resolv", "writ", "delet", "creat") +_WRITE_WORD = re.compile(r"\b(?:" + "|".join(_WRITE_STEMS) + r")(?:e|es|ing)\b", re.IGNORECASE) + + +def _read_only(tool: Tool) -> bool | None: + return tool.annotations.read_only_hint if tool.annotations else None + + +def _write_options(tools: Sequence[Tool]) -> list[str]: + """Every `tool.option` of a read-only tool whose description holds a write verb.""" + found: list[str] = [] + for tool in tools: + if _read_only(tool) is not True: + continue + for option, schema in (tool.parameters or {}).get("properties", {}).items(): + if _WRITE_WORD.search(str(schema.get("description", ""))): + found.append(f"{tool.name}.{option}") + return sorted(found) + + +def _tool_with_option(description: str) -> Callable[..., str]: + """A tool whose one option has `description`, signed as the generator signs its tools.""" + + def tool_fn(*, option: bool = False) -> str: + return "ok" + + annotation = Annotated[bool, Field(description=description)] + parameter = inspect.Parameter( + "option", inspect.Parameter.KEYWORD_ONLY, default=False, annotation=annotation + ) + setattr(tool_fn, "__signature__", inspect.Signature([parameter], return_annotation=str)) + setattr(tool_fn, "__annotations__", {"option": annotation, "return": str}) + return tool_fn + + +def _catalogue(*, read_only: bool, description: str) -> Sequence[Tool]: + """The tools of a server that holds one tool with one option.""" + server = FastMCP("write-option-test") + server.tool(name="probe", annotations={"readOnlyHint": read_only})( + _tool_with_option(description) + ) + return asyncio.run(server._list_tools()) + + +def _published(name: str) -> Tool: + tool = asyncio.run(mcp.get_tool(name)) + assert tool is not None, f"The server publishes no tool named {name}." + return tool + + +def _flags(path: tuple[str, ...]) -> set[str]: + return {opt for param in resolve_click_command(path).params for opt in param.opts} + + +def test_a_read_only_tool_with_an_option_that_resolves_is_reported() -> None: + tools = _catalogue(read_only=True, description="Automatically resolve review threads.") + + assert _write_options(tools) == ["probe.option"] + + +def test_each_write_verb_in_each_form_is_reported() -> None: + forms = [ + f"{prefix} {stem}{suffix} the thing." + for stem in _WRITE_STEMS + for suffix in ("e", "es", "ing") + for prefix in ("It", "Skip") + ] + + reported = [ + text + for text in forms + if _write_options(_catalogue(read_only=True, description=text)) == ["probe.option"] + ] + + assert reported == forms + + +def test_a_write_verb_in_the_case_of_a_title_is_reported() -> None: + tools = _catalogue(read_only=True, description="Write findings to this path.") + + assert _write_options(tools) == ["probe.option"] + + +def test_a_tool_that_is_not_read_only_may_have_a_write_option() -> None: + tools = _catalogue(read_only=False, description="Automatically resolve review threads.") + + assert _write_options(tools) == [] + + +def test_a_past_form_of_a_write_verb_is_not_reported() -> None: + tools = _catalogue(read_only=True, description="Inherited policies are resolved via 'extends'.") + + assert _write_options(tools) == [] + + +def test_a_word_that_only_holds_a_write_verb_is_not_reported() -> None: + tools = _catalogue(read_only=True, description="Filter by the creator or the rewriter.") + + assert _write_options(tools) == [] + + +def test_no_read_only_tool_publishes_a_write_option() -> None: + """The check runs on the published schema of every tool that the server registers.""" + tools = asyncio.run(mcp._list_tools()) + + assert _write_options(tools) == [] + + +@pytest.mark.parametrize( + ("tool_name", "option"), + [ + ("pr_check_readiness", "auto_resolve"), + ("roadmap_refine", "confirm"), + ("tf_plan", "out"), + ], +) +def test_a_tool_that_keeps_a_writing_option_is_not_read_only(tool_name: str, option: str) -> None: + """The tool keeps the option that writes, so its flag says that it writes.""" + tool = _published(tool_name) + + assert _read_only(tool) is False + assert option in tool.parameters["properties"] + + +def test_the_readiness_command_keeps_its_auto_resolve_option() -> None: + assert "--auto-resolve" in _flags(("pr", "check-readiness")) + + +def test_the_injection_scan_stays_read_only_and_publishes_no_sarif_option() -> None: + """The scan prints its findings, so its tool leaves out the option that writes a file.""" + tool = _published("scan_injection") + + assert _read_only(tool) is True + assert "sarif" not in tool.parameters["properties"] + assert "--sarif" in _flags(("scan", "injection")) From 672895d1f65d3e127fec997507d5cb39a104d0b4 Mon Sep 17 00:00:00 2001 From: Daniel Petty Date: Thu, 5 Nov 2026 15:54:24 +0000 Subject: [PATCH 2/4] fix(mcp): mark roadmap run and ci not read-only, drop tf plan out The review of #3235 found two more tools that write: roadmap run with --confirm and ci with --fix. The tf plan tool published --out, a path that the command writes and nothing confines. Its schema now leaves out that option and the tool stays read-only. The change folder names the follow-up issues. --- docs/MCP_TOOLS.md | 13 ++++--- .../design.md | 24 ++++++++----- .../proposal.md | 34 ++++++++++++------ .../specs/mcp-role-server/spec.md | 26 +++++++------- .../tasks.md | 13 ++++--- src/devops_cli/ai/mcp/generator.py | 5 +++ src/devops_cli/config/constants.py | 16 +++++---- .../mcp/test_mcp_read_only_write_options.py | 36 ++++++++++++------- 8 files changed, 103 insertions(+), 64 deletions(-) diff --git a/docs/MCP_TOOLS.md b/docs/MCP_TOOLS.md index 8354631ef..8d5a6f7a3 100644 --- a/docs/MCP_TOOLS.md +++ b/docs/MCP_TOOLS.md @@ -37,7 +37,7 @@ The `devops-cli` FastMCP server exposes DevOps automation and AI review capabili | [`argo_fleet_sync`](#argo-fleet-sync) | `false` | Synchronize an application across a fleet of Kubernetes clusters with bounded concurrency. | | [`argo_rollouts_analyze`](#argo-rollouts-analyze) | `false` | Evaluate metric rollback gates and trigger automated rollback on threshold violation. | | [`branches_list`](#branches-list) | `true` | List branches across all repos. | -| [`ci`](#ci) | `true` | Run tests, linting, formatting, and type-checks. A run that selects test, security or typecheck first waits for a free gate slot, so the gates of parallel agents take turns. ci.gate_slots sets how many run at once. | +| [`ci`](#ci) | `false` | Run tests, linting, formatting, and type-checks. A run that selects test, security or typecheck first waits for a free gate slot, so the gates of parallel agents take turns. ci.gate_slots sets how many run at once. | | [`config_audit_keys`](#config-audit-keys) | `true` | Audit OS Keyring token health, backend status, and zero-plaintext secret compliance. The audit scans each config file that the settings merge: the global config file and the project config file. It also scans config.yaml and .devops/config.yaml in the current directory. Exits 0 only when the audit is compliant. Exits 1 when a config file holds a plaintext secret, or when a config file cannot be read or parsed, reporting it unaudited. The full report prints before the exit. The alert of a leak names two commands. `devops config set ` stores a credential in the keyring. Add `--from-file ` for a multi-line credential, such as a PEM key. `DEVOPS_CLI_CONFIG= devops config unset ` removes the value from the file of the leak. | | [`config_output`](#config-output) | `true` | Output environment variables available for devops-cli configuration. | | [`config_show`](#config-show) | `true` | Print every setting that a variable sets and every credential. A credential shows as set or not set, never its value. | @@ -113,7 +113,7 @@ The `devops-cli` FastMCP server exposes DevOps automation and AI review capabili | [`review_stats`](#review-stats) | `true` | Compute and display review accuracy statistics across saved sessions. | | [`roadmap_intake`](#roadmap-intake) | `false` | Turn each open issue with no priority label into a roadmap item, or file a new candidate: intake sets the labels, adds a reason comment and names a possible duplicate for a person to close. It writes nothing without --confirm. Details: docs/roadmap/intake.md. | | [`roadmap_refine`](#roadmap-refine) | `false` | Refine roadmap items to Ready: investigate open questions with tools that read the code and the documentation, then write the design, tasks and acceptance criteria to GitHub. Without --confirm, refine prints its plan only. Details: docs/roadmap/refine.md. | -| [`roadmap_run`](#roadmap-run) | `true` | Run the roadmap jobs that are due, in order: metrics, then intake, refine and the alternatives check, and record each one's last success. Intake is due on an issue reopened, and decides at most 5 candidates a run and keeps a record of those it left beside the schedule; refine is due every 31 minutes, on a first run, and after an intake that placed a critical fix or a P0 item; the alternatives check is due every 719 minutes and on a first run, and checks at most roadmap.alternatives.first_pass_limit items while an item has no check, and roadmap.alternatives.limit items after. Without --confirm, or with --dry-run, prints the due list and runs nothing. | +| [`roadmap_run`](#roadmap-run) | `false` | Run the roadmap jobs that are due, in order: metrics, then intake, refine and the alternatives check, and record each one's last success. Intake is due on an issue reopened, and decides at most 5 candidates a run and keeps a record of those it left beside the schedule; refine is due every 31 minutes, on a first run, and after an intake that placed a critical fix or a P0 item; the alternatives check is due every 719 minutes and on a first run, and checks at most roadmap.alternatives.first_pass_limit items while an item has no check, and roadmap.alternatives.limit items after. Without --confirm, or with --dry-run, prints the due list and runs nothing. | | [`sandbox_deploy`](#sandbox-deploy) | `false` | Deploy an isolated background container sandbox with security containment. | | [`sandbox_exec`](#sandbox-exec) | `false` | Execute a command inside an active sandbox container. | | [`sandbox_network_policy`](#sandbox-network-policy) | `true` | Generate declarative Kubernetes NetworkPolicy YAML for workload sandbox isolation. | @@ -140,7 +140,7 @@ The `devops-cli` FastMCP server exposes DevOps automation and AI review capabili | [`tf_cost_breakdown`](#tf-cost-breakdown) | `true` | Estimate monthly and hourly cloud infrastructure costs using Infracost. | | [`tf_notify_plan`](#tf-notify-plan) | `true` | Format and post structured, collapsible OpenTofu/Terraform plan diffs to PR comments. | | [`tf_output`](#tf-output) | `true` | Read an output variable from the OpenTofu state. | -| [`tf_plan`](#tf-plan) | `false` | Generate and show an OpenTofu execution plan. | +| [`tf_plan`](#tf-plan) | `true` | Generate and show an OpenTofu execution plan. | | [`tls_inspect`](#tls-inspect) | `true` | Inspect and display metadata of an X.509 certificate. | | [`valkey_flush`](#valkey-flush) | `false` | Flush and purge keys from current or all databases. | | [`valkey_get`](#valkey-get) | `true` | Retrieve string value stored at key. | @@ -640,7 +640,7 @@ List branches across all repos. Run tests, linting, formatting, and type-checks. A run that selects test, security or typecheck first waits for a free gate slot, so the gates of parallel agents take turns. ci.gate_slots sets how many run at once. -**Annotations:** `readOnlyHint: true` +**Annotations:** `readOnlyHint: false` **Parameters:** @@ -1901,7 +1901,7 @@ Refine roadmap items to Ready: investigate open questions with tools that read t Run the roadmap jobs that are due, in order: metrics, then intake, refine and the alternatives check, and record each one's last success. Intake is due on an issue reopened, and decides at most 5 candidates a run and keeps a record of those it left beside the schedule; refine is due every 31 minutes, on a first run, and after an intake that placed a critical fix or a P0 item; the alternatives check is due every 719 minutes and on a first run, and checks at most roadmap.alternatives.first_pass_limit items while an item has no check, and roadmap.alternatives.limit items after. Without --confirm, or with --dry-run, prints the due list and runs nothing. -**Annotations:** `readOnlyHint: true` +**Annotations:** `readOnlyHint: false` **Parameters:** @@ -2305,7 +2305,7 @@ Read an output variable from the OpenTofu state. Generate and show an OpenTofu execution plan. -**Annotations:** `readOnlyHint: false` +**Annotations:** `readOnlyHint: true` **Parameters:** @@ -2313,7 +2313,6 @@ Generate and show an OpenTofu execution plan. |---|---|---|---|---| | `directory` | `string` | No | `.` | Target directory containing OpenTofu configuration. | | `var_file` | `string` | No | - | Path to variable definitions file. | -| `out` | `string` | No | - | Write generated plan to file. | | `destroy` | `boolean` | No | `False` | Generate a plan to destroy all resources. | | `dry_run` | `boolean` | No | `False` | Preview execution plan without mutating external state. | diff --git a/openspec/changes/3235-read-only-mcp-tools-have-no-write-option/design.md b/openspec/changes/3235-read-only-mcp-tools-have-no-write-option/design.md index c57891625..37c1bb59c 100644 --- a/openspec/changes/3235-read-only-mcp-tools-have-no-write-option/design.md +++ b/openspec/changes/3235-read-only-mcp-tools-have-no-write-option/design.md @@ -2,18 +2,26 @@ ## Decisions -- **The test scans the description of each option in the published schema of each read-only tool, for `resolve`, `write`, `delete` and `create` in the base, `-s` and `-ing` forms.**: The issue names these verbs and names the help text as the signal. The generator copies the help text into the description of the published parameter, so the schema is what a model reads. The test names the tool and the option. -- **A past form of a verb does not match.**: `resolved` in `sandbox_network_policy.public_whitelist`, `sandbox_network_policy.local_whitelist` and `scan_injection.suppress`, and `wrote` in `roadmap_refine.item`, describe a result and write nothing. A match on them would need an exempt list, and an exempt list hides the error that the test must show. -- **`pr check-readiness`, `roadmap refine` and `tf plan` are marked not read-only, and each command keeps its option.**: `--auto-resolve` and `--confirm` write to GitHub. `--out` writes the plan file that `tf_apply` takes as `plan_file`, so the option has a use for a model. The flag then says what the tool can do. No job role table names these tools, so no role loses a tool. -- **`scan_injection` stays read-only and its schema drops `sarif`.**: The command prints its findings, and a SARIF file is for a CI upload that a person starts. The dry-run class of the command is `no-external-request`, and `test_mcp_exposure_table_no_external_request_read_only` fails when the tool is not read-only. The restriction follows the rule of `ai_ast_graph` and `scan_sbom`, which leave out a file option that the model does not need. +- **The test scans the description of each option in the published schema of each read-only tool.**: It looks for `resolve`, `write`, `delete` and `create` in the base, `-s` and `-ing` forms. The issue names these verbs and names the help text as the signal. The generator copies the help text into the description of the published parameter. So the schema is what a model reads. The test names the tool and the option. +- **A past form of a verb does not match.**: `resolved` in `sandbox_network_policy.public_whitelist`, `sandbox_network_policy.local_whitelist` and `scan_injection.suppress` describes a result. `wrote` in `roadmap_refine.item` does the same. A match on them would need an exempt list. An exempt list hides the error that the test must show. +- **`pr check-readiness`, `roadmap refine`, `roadmap run` and `ci` are marked not read-only. Each command keeps its option.**: `--auto-resolve` and `--confirm` write to GitHub. `--fix` rewrites files, and it is on by default. The flag then says what the tool can do. No job role table names these tools, so no role loses a tool. The rule finds the first two. A person finds the other two by reading the options, so the table test names them. +- **`scan_injection` and `tf_plan` stay read-only. Their schemas drop `sarif` and `out`.**: Each command prints its result, so the model needs no file. SECURITY.md rule 11 says that a tool publishes no option for a path that the command writes, unless a path helper confines it. Nothing confines `sarif` or `out`. The dry-run class of `scan injection` is `no-external-request`, and the ratchet test requires a read-only tool for that class. The restriction follows the rule of `ai_ast_graph` and `scan_sbom`. - **The verb pattern is a private constant of the test.**: Only the test reads it, so CONTRIBUTING.md section 5 puts it in that module. -- **The test builds a one-tool server for each case of the rule, with the signature that the generator makes.**: The rule then runs on the same kind of object that the repository check reads, and no double replaces a tool. +- **The test builds a one-tool server for each case of the rule, with the signature that the generator makes.**: The rule then runs on the same kind of object that the repository check reads. No double replaces a tool. + +## Out of scope + +- **Tools that write local state.**: `ai_quiesce`, `ai_resume`, `ai_failover`, `ai_gateway_failover` and `review_export_feedback` write local state or a file. Dropping an option does not make them read-only, because the whole command writes. A flag that is not read-only fails the requirement `mcp-read-only-hint-agrees-with-no-external-request` and its ratchet test. Issue #3642 decides what the class means for the flag. +- **The words of `tf_notify_plan`.**: The command prints the comment and posts nothing, so the flag is right. Issue #3644 fixes the help text. +- **Paths with no confinement decision.**: `scan_injection.suppress` reads a path and `review_export_feedback.output` writes a path. Both stay as before. Issue #3480 gives each a decision. ## Rejected alternatives -- **Mark only `pr check-readiness` and add no general check.**: The scan finds three more tools of the same kind. Acceptance criterion 1 of the issue says that no tool with a write option is read-only. +- **Keep `tf_plan` not read-only and publish `out`.**: The first version did so. The tool then publishes a path that the command writes, and nothing confines it. SECURITY.md rule 11 and the requirement "Write path confinement" of `mcp-tool-path-confinement` forbid that. +- **Mark only `pr check-readiness` and add no general check.**: The scan finds more tools of the same kind. Acceptance criterion 1 of the issue says that no tool with a write option is read-only. - **Remove `auto_resolve` from the published schema of `pr_check_readiness`.**: `_PATH_PARAM_RESTRICTIONS` takes an `allowed` list only. The entry must list the seven other options, and it drops each option that the command adds later. The option is also useful for a model that has the write right. -- **Scan option names, not descriptions.**: `--confirm`, `--out` and `--sarif` have names with no write verb, and the check would miss three of the four tools. -- **Widen the verbs to `post`, `execute`, `apply`, `restore` and `close`.**: These words find `roadmap_run.confirm` (`Execute`), `tf_notify_plan.pr_number` (`post`) and `ai_gateway_failover.reset` (`Restore`). They also match words with no write: `sandbox_status.all_instances` (`Apply operation across all registered sandbox instances`), `ci.check` (`applying`), `gh_issues_list.state` (`closed`) and `pr_describe.issue` (`closes`). A word list cannot tell the two groups apart. A person must read each flag. The issue names four verbs, so this change keeps them. +- **Scan option names, not descriptions.**: `--confirm`, `--out`, `--fix` and `--sarif` have names with no write verb. The check would miss four of the tools. +- **Widen the verbs to `post`, `execute`, `apply`, `restore` and `close`.**: These words find `roadmap_run.confirm` (`Execute`), `tf_notify_plan.pr_number` (`post`) and `ai_gateway_failover.reset` (`Restore`). The first is a write. The second is not, because the command posts nothing. The words also match text with no write: `sandbox_status.all_instances` (`Apply operation across all registered sandbox instances`), `ci.check` (`applying`), `gh_issues_list.state` (`closed`) and `pr_describe.issue` (`closes`). A word list cannot tell the groups apart. A person must read each flag. So the test keeps the four verbs of the issue, and the table holds the result of that reading. - **Mark `scan_injection` not read-only.**: The dry-run ratchet test fails with that flag (see Feasibility in the proposal). +- **Mark the five local writers not read-only.**: The requirement `mcp-read-only-hint-agrees-with-no-external-request` and its ratchet test fail. A change to them needs a decision of its own, which issue #3642 holds. - **Keep the old flag and rely on the description of the tool.**: A role server reads the flag and not the text, so the text does not stop a write. diff --git a/openspec/changes/3235-read-only-mcp-tools-have-no-write-option/proposal.md b/openspec/changes/3235-read-only-mcp-tools-have-no-write-option/proposal.md index 95f168fa2..338ca25ed 100644 --- a/openspec/changes/3235-read-only-mcp-tools-have-no-write-option/proposal.md +++ b/openspec/changes/3235-read-only-mcp-tools-have-no-write-option/proposal.md @@ -4,17 +4,25 @@ Issue #3235 reports that `CONST_MCP_EXPOSURE_TABLE` marks `pr_check_readiness` as read-only. The command has the option `--auto-resolve`, which resolves review threads on GitHub. A role server and a client choose tools by the read-only flag. So a model can get a write that the flag says it cannot make. No test finds this kind of error. -The check that the issue asks for finds three more tools of the same kind. `roadmap_refine` has `--confirm`, which writes the proposed designs to GitHub. `tf_plan` has `--out`, which writes the plan to a file. `scan_injection` has `--sarif`, which writes the findings to a file. All three are marked read-only. +The check that the issue asks for finds three more tools of the same kind. `roadmap_refine` has `--confirm`, which writes the proposed designs to GitHub. `scan_injection` has `--sarif`, which writes the findings to a file. `tf_plan` has `--out`, which writes the plan to a file. All three are marked read-only. + +A review of the first version finds two more. `roadmap_run` has `--confirm`, which runs the same refine and intake jobs. `ci` has `--fix`, which is on by default and rewrites files. Both are marked read-only. ## What Changes -- The `pr check-readiness`, `roadmap refine` and `tf plan` rows of `CONST_MCP_EXPOSURE_TABLE` in `src/devops_cli/config/constants.py` are not read-only. Each command keeps its option for people at the command line. -- `_PATH_PARAM_RESTRICTIONS` in `src/devops_cli/ai/mcp/generator.py` gets an entry for `("scan", "injection")` that leaves out `sarif`. The command prints its findings, so the model needs no file. The tool stays read-only, because the dry-run class of the command is `no-external-request`, and that class requires a read-only tool. -- A new test, `tests/ai/mcp/test_mcp_read_only_write_options.py`, reads the published schema of every tool. It fails on a read-only tool that has an option whose description holds `resolve`, `write`, `delete` or `create` in its base, `-s` or `-ing` form, and it names the tool and the option. A past form such as `resolved` does not count. -- `docs/MCP_TOOLS.md` is regenerated: four flags change and the `sarif` row of `scan_injection` goes. +- The `pr check-readiness`, `roadmap refine`, `roadmap run` and `ci` rows of `CONST_MCP_EXPOSURE_TABLE` in `src/devops_cli/config/constants.py` are not read-only. Each command keeps its option for people at the command line. +- `_PATH_PARAM_RESTRICTIONS` in `src/devops_cli/ai/mcp/generator.py` gets an entry for `("scan", "injection")` that leaves out `sarif`. It gets an entry for `("tf", "plan")` that leaves out `out`. Each command prints its result, so the model needs no file. SECURITY.md rule 11 forbids a tool option for a path that the command writes. Both tools stay read-only. +- A new test, `tests/ai/mcp/test_mcp_read_only_write_options.py`, reads the published schema of every tool. It fails on a read-only tool that has an option whose description holds `resolve`, `write`, `delete` or `create`. The word is in its base, `-s` or `-ing` form. The test names the tool and the option. A past form such as `resolved` does not count. +- `docs/MCP_TOOLS.md` is regenerated. Four flags change. The `sarif` row of `scan_injection` and the `out` row of `tf_plan` go. - The change alters the flag and the schema that an MCP client reads, so it carries a spec delta and `skip_specs` is false. - The change fits one pull request. +Acceptance criterion 1 of the issue is met for the tools that the rule finds, and for `roadmap_run` and `ci`. Other tools stay open, and each has its own issue: + +- `ai_quiesce`, `ai_resume`, `ai_failover`, `ai_gateway_failover` and `review_export_feedback` write local state or a file. Their commands have the class `no-external-request`. The requirement `mcp-read-only-hint-agrees-with-no-external-request` forces their flag to true. Issue #3642 owns that decision. +- `tf_notify_plan` posts nothing, so its flag is right. Its help says that it posts. Issue #3644 owns the text. +- `scan_injection.suppress` and `review_export_feedback.output` publish a path with no confinement decision. Issue #3480 owns both. + ## Capabilities ### New Capabilities @@ -23,7 +31,7 @@ None. ### Modified Capabilities -- `mcp-role-server`: A read-only tool publishes no option that writes, A tool that keeps a writing option is not read-only, The injection scan tool publishes no SARIF option +- `mcp-role-server`: A read-only tool publishes no option that writes, A tool that keeps a writing option is not read-only, A read-only tool publishes no option for a file that its command writes ## Impact @@ -33,12 +41,16 @@ None. ## Feasibility -- Command output at `origin/main` commit 66a8a63e. A script that lists the published tools of `devops_cli.ai.mcp.server.mcp` shows `pr_check_readiness` with `readOnlyHint` true and the parameters `number`, `allow_draft`, `allow_pending_checks`, `allow_blocked_state`, `auto_resolve`, `allow_replied_threads`, `repo` and `dry_run`. The description of `auto_resolve` is "Automatically resolve review discussion threads that have received a reply from someone other than the thread opener." So the premise of the issue holds. The stage comment of 2026-11-02 on the issue says that the premise did not hold, but it gives no evidence. +- Command output at `origin/main` commit 66a8a63e. A script that lists the published tools of `devops_cli.ai.mcp.server.mcp` shows `pr_check_readiness` with `readOnlyHint` true. Its parameters include `auto_resolve`. The description of `auto_resolve` is "Automatically resolve review discussion threads that have received a reply from someone other than the thread opener." So the premise of the issue holds. The stage comment of 2026-11-02 on the issue says that the premise did not hold, but it gives no evidence. - Command output, `uv run devops pr check-readiness --help`, lists `--auto-resolve`. The row in `src/devops_cli/config/constants.py` was `(("pr", "check-readiness"), 60.0, True, False)`. -- The same script, with the four verbs of the issue in the base, `-s` and `-ing` forms, finds four read-only options: `pr_check_readiness.auto_resolve`, `roadmap_refine.confirm`, `scan_injection.sarif` and `tf_plan.out`. With every form of the verbs, it also finds `roadmap_refine.item` (`wrote`), `sandbox_network_policy.public_whitelist` and `.local_whitelist` (`resolved`) and `scan_injection.suppress` (`resolved`). Each of these four describes a result, not a write. So the rule leaves out the past forms. -- When the `scan injection` row was set to not read-only, `tests/invariants/test_dry_run_class_ratchet.py::test_mcp_exposure_table_no_external_request_read_only` failed with "MCP tool for 'scan injection' has read_only=False but command is 'no-external-request'". The command prints its findings without `--sarif`, so the tool drops that option. -- `src/devops_cli/ai/mcp/generator.py` builds the description of a parameter from the help text of the Click option (`_build_parameter_field`), so the schema carries the same words as the help. +- The same script, with the four verbs of the issue in the base, `-s` and `-ing` forms, finds four read-only options. They are `pr_check_readiness.auto_resolve`, `roadmap_refine.confirm`, `scan_injection.sarif` and `tf_plan.out`. +- With every form of the verbs, the script also finds `roadmap_refine.item` (`wrote`), `sandbox_network_policy.public_whitelist` and `.local_whitelist` (`resolved`) and `scan_injection.suppress` (`resolved`). Each of these describes a result and not a write. So the rule leaves out the past forms. +- A read of the commands shows two more true writes. `devops ci` has `--fix/--no-fix` with the default true in `src/devops_cli/commands/ci.py`. `devops roadmap run --confirm` runs the intake and refine jobs. Both commands have the dry-run class `request-plan`, so the ratchet test accepts a flag that is not read-only. +- `devops tf notify-plan` in `src/devops_cli/commands/tf.py` sends no request. It puts the PR number in its payload and prints the comment. So `tf_notify_plan` is read-only, although its help says that it posts. +- When the `scan injection` row was set to not read-only, `tests/invariants/test_dry_run_class_ratchet.py::test_mcp_exposure_table_no_external_request_read_only` failed. The message was "MCP tool for 'scan injection' has read_only=False but command is 'no-external-request'". The command prints its findings without `--sarif`, so the tool drops that option. +- The generated tool `tf_plan` passed an absolute path outside the work tree to `--out` in the command line that it built. Nothing confined it. `PATH_CONFINEMENTS['tf_plan']` holds only `directory` and `var_file`. So the tool drops `out`. +- `src/devops_cli/ai/mcp/generator.py` builds the description of a parameter from the help text of the Click option (`_build_parameter_field`). So the schema carries the same words as the help. ## Existing Tool Considered -No open, maintained tool covers the need. The alternatives check of 2026-11-05 on the issue found none: MCP security scanners validate annotation fields against each other or scan a running server, and none reads `CONST_MCP_EXPOSURE_TABLE` or the option help text of a CLI-derived tool. The change builds one repository-specific test. The written cost is about 30,000 tokens to build it, about 3,000 tokens per year to keep and fix it, and about two hours elapsed. The change deletes no internal code. +No open, maintained tool covers the need. The alternatives check of 2026-11-05 on the issue found none. MCP security scanners validate annotation fields against each other. Other scanners read a running server. None reads `CONST_MCP_EXPOSURE_TABLE` or the option help text of a CLI-derived tool. The change builds one repository-specific test. The written cost is about 30,000 tokens to build it. It is about 3,000 tokens per year to keep and fix it. The elapsed time is about two hours. The change deletes no internal code. diff --git a/openspec/changes/3235-read-only-mcp-tools-have-no-write-option/specs/mcp-role-server/spec.md b/openspec/changes/3235-read-only-mcp-tools-have-no-write-option/specs/mcp-role-server/spec.md index fe0459a0e..6abcf121d 100644 --- a/openspec/changes/3235-read-only-mcp-tools-have-no-write-option/specs/mcp-role-server/spec.md +++ b/openspec/changes/3235-read-only-mcp-tools-have-no-write-option/specs/mcp-role-server/spec.md @@ -1,7 +1,7 @@ ## ADDED Requirements ### Requirement: A read-only tool publishes no option that writes -A tool that the exposure table marks read-only SHALL publish no option whose description holds `resolve`, `write`, `delete` or `create` as a whole word, in its base, `-s` or `-ing` form and in any letter case. A past form such as `resolved` SHALL NOT count. +A tool that the exposure table marks read-only SHALL publish no option whose description holds `resolve`, `write`, `delete` or `create`. The word SHALL count as a whole word in its base, `-s` or `-ing` form and in any letter case. A past form such as `resolved` SHALL NOT count. #### Scenario: A read-only tool with a writing option - **WHEN** A test reads the published schema of a read-only tool, and one option description is `Write findings to this path.` @@ -20,27 +20,27 @@ A tool that the exposure table marks read-only SHALL publish no option whose des - **THEN** No read-only tool has an option that the rule reports. ### Requirement: A tool that keeps a writing option is not read-only -The tools `pr_check_readiness`, `roadmap_refine` and `tf_plan` SHALL be marked not read-only, and SHALL publish the option `auto_resolve`, `confirm` and `out` respectively. The command `devops pr check-readiness` SHALL keep the option `--auto-resolve`. +The tools `pr_check_readiness`, `roadmap_refine`, `roadmap_run` and `ci` SHALL be marked not read-only. They SHALL publish the options `auto_resolve`, `confirm`, `confirm` and `fix` respectively. The command `devops pr check-readiness` SHALL keep the option `--auto-resolve`. #### Scenario: The readiness tool - **WHEN** A test reads the published tool `pr_check_readiness` - **THEN** Its read-only flag is false, and its schema holds `auto_resolve`. -#### Scenario: The refine and plan tools -- **WHEN** A test reads the published tools `roadmap_refine` and `tf_plan` -- **THEN** Both read-only flags are false, and the schemas hold `confirm` and `out`. +#### Scenario: The refine, run and ci tools +- **WHEN** A test reads the published tools `roadmap_refine`, `roadmap_run` and `ci` +- **THEN** All three read-only flags are false, and the schemas hold `confirm`, `confirm` and `fix`. #### Scenario: The readiness command - **WHEN** A test reads the options of `devops pr check-readiness` - **THEN** The options include `--auto-resolve`. -### Requirement: The injection scan tool publishes no SARIF option -The tool `scan_injection` SHALL be marked read-only and SHALL publish no `sarif` option. The command `devops scan injection` SHALL keep the option `--sarif`. +### Requirement: A read-only tool publishes no option for a file that its command writes +The tools `scan_injection` and `tf_plan` SHALL be marked read-only. They SHALL publish no `sarif` option and no `out` option. The commands `devops scan injection` and `devops tf plan` SHALL keep the options `--sarif` and `--out`. -#### Scenario: The published injection scan tool -- **WHEN** A test reads the published tool `scan_injection` -- **THEN** Its read-only flag is true, and its schema holds no `sarif` property. +#### Scenario: The published injection scan and plan tools +- **WHEN** A test reads the published tools `scan_injection` and `tf_plan` +- **THEN** Both read-only flags are true, and the schemas hold no `sarif` property and no `out` property. -#### Scenario: The injection scan command -- **WHEN** A test reads the options of `devops scan injection` -- **THEN** The options include `--sarif`. +#### Scenario: The injection scan and plan commands +- **WHEN** A test reads the options of `devops scan injection` and `devops tf plan` +- **THEN** The options include `--sarif` and `--out`. diff --git a/openspec/changes/3235-read-only-mcp-tools-have-no-write-option/tasks.md b/openspec/changes/3235-read-only-mcp-tools-have-no-write-option/tasks.md index d7e944d25..8f7ee7976 100644 --- a/openspec/changes/3235-read-only-mcp-tools-have-no-write-option/tasks.md +++ b/openspec/changes/3235-read-only-mcp-tools-have-no-write-option/tasks.md @@ -1,27 +1,30 @@ # Tasks -## 1. Report a write option of a read-only tool and fix the four tools +## 1. Report a write option of a read-only tool and fix the tools Reads: `src/devops_cli/config/constants.py`, `src/devops_cli/ai/mcp/generator.py`, `tests/ai/mcp/test_path_confinement.py`, `tests/ai/mcp/test_mcp_tool_secrets.py`. Changes: `tests/ai/mcp/test_mcp_read_only_write_options.py` (new), `src/devops_cli/config/constants.py`, `src/devops_cli/ai/mcp/generator.py`. -- [x] 1.1 Write `tests/ai/mcp/test_mcp_read_only_write_options.py` with the cases of the rule on a one-tool server (a read-only tool with a writing option, each verb in each form, a tool that is not read-only, a past form, a word that only holds a verb), the check on the published schema of every tool, the flag and option of `pr_check_readiness`, `roadmap_refine` and `tf_plan`, and the schema of `scan_injection` +- [x] 1.1 Write `tests/ai/mcp/test_mcp_read_only_write_options.py`. The cases of the rule run on a one-tool server. They cover a read-only tool with a writing option and each verb in each form. They cover a tool that is not read-only, a past form, and a word that only holds a verb. One test checks the published schema of every tool. One test checks the flag and the option of `pr_check_readiness`, `roadmap_refine`, `roadmap_run` and `ci`. One test checks that `scan_injection` and `tf_plan` stay read-only and publish no file option. - Verification: `uv run devops ci test tests/ai/mcp/test_mcp_read_only_write_options.py` fails on the base -- [x] 1.2 Set the `pr check-readiness`, `roadmap refine` and `tf plan` rows of `CONST_MCP_EXPOSURE_TABLE` to not read-only, with a comment that names the writing option. Add the `("scan", "injection")` entry to `_PATH_PARAM_RESTRICTIONS` with every option but `sarif` +- [x] 1.2 Set the `pr check-readiness`, `roadmap refine`, `roadmap run` and `ci` rows of `CONST_MCP_EXPOSURE_TABLE` to not read-only. Write a comment that names the writing option. Add the `("scan", "injection")` and `("tf", "plan")` entries to `_PATH_PARAM_RESTRICTIONS`. Each entry lists every option but the file option. - Verification: `uv run devops ci test tests/ai/mcp/test_mcp_read_only_write_options.py tests/invariants/test_dry_run_class_ratchet.py tests/ai/mcp/test_mcp.py tests/ai/mcp/test_fastmcp_contracts.py` passes ## 2. Regenerate the MCP tool reference Reads: `docs/MCP_TOOLS.md`. Changes: `docs/MCP_TOOLS.md`. -- [x] 2.1 Run `uv run devops docs generate --sync-readme`. The four flags change and the `sarif` row of `scan_injection` goes +- [x] 2.1 Run `uv run devops docs generate --sync-readme`. Four flags change. The `sarif` row of `scan_injection` and the `out` row of `tf_plan` go. - Verification: `uv run devops ci docs` passes ## Criteria - A read-only tool publishes no option that writes: group 1 - A tool that keeps a writing option is not read-only: group 1 -- The injection scan tool publishes no SARIF option: group 1 +- A read-only tool publishes no option for a file that its command writes: group 1 ## Workflow follow-up +- Issue #3642 owns the tools that write local state and that the class `no-external-request` keeps read-only. They are `ai_quiesce`, `ai_resume`, `ai_failover`, `ai_gateway_failover` and `review_export_feedback`. +- Issue #3644 owns the help text of `tf_notify_plan`, which says that the command posts a comment. +- Issue #3480 owns the path decision for `scan_injection.suppress` and `review_export_feedback.output`. - Pending a person: review the diff in tuicr (tuicr -r ..HEAD). diff --git a/src/devops_cli/ai/mcp/generator.py b/src/devops_cli/ai/mcp/generator.py index b57368b25..82ad1c0aa 100644 --- a/src/devops_cli/ai/mcp/generator.py +++ b/src/devops_cli/ai/mcp/generator.py @@ -101,6 +101,11 @@ ), "required": frozenset(), }, + # No `out`: a path that the model gives would make the command write a file (#3235). + ("tf", "plan"): { + "allowed": frozenset({"directory", "var_file", "destroy", "dry_run"}), + "required": frozenset(), + }, # No `output`: the command prints the stream, so the model needs no file. ("k8s", "security-stream"): { "allowed": frozenset( diff --git a/src/devops_cli/config/constants.py b/src/devops_cli/config/constants.py index ad05732ae..e3584129e 100644 --- a/src/devops_cli/config/constants.py +++ b/src/devops_cli/config/constants.py @@ -265,9 +265,10 @@ CONST_MCP_EAGER_DOMAINS | CONST_MCP_LAZY_DOMAINS | {"github", "secrets", "benchmarks"} ) # Command exposure table: (command_path, timeout_seconds, read_only, format_json) -# A command that has an option to write is not read-only, because a role server and a client choose -# tools by that flag. `tests/ai/mcp/test_mcp_read_only_write_options.py` finds such an option in -# the published schema (#3235). +# A role server and a client choose tools by the read-only flag. A tool with an option that writes +# to GitHub or to a file is not read-only, or its schema leaves that option out. +# `tests/ai/mcp/test_mcp_read_only_write_options.py` finds such an option by the verbs resolve, +# write, delete and create in its description (#3235). CONST_MCP_EXPOSURE_TABLE: Final[tuple[tuple[tuple[str, ...], float, bool, bool], ...]] = ( (("ai", "ast", "graph"), 60.0, True, False), (("ai", "ast", "parse"), 60.0, True, True), @@ -300,7 +301,8 @@ (("argo", "fleet", "sync"), 300.0, False, True), (("argo", "rollouts", "analyze"), 300.0, False, True), (("branches", "list"), 60.0, True, False), - (("ci",), 360.0, True, False), + # `--fix` is on by default and rewrites files (#3235). + (("ci",), 360.0, False, False), (("config", "audit-keys"), 30.0, True, False), (("config", "output"), 30.0, True, True), (("config", "show"), 30.0, True, False), @@ -377,7 +379,8 @@ (("roadmap", "intake"), 300.0, False, False), # `--confirm` writes the proposed designs to GitHub (#3235). (("roadmap", "refine"), 300.0, False, False), - (("roadmap", "run"), 300.0, True, False), + # `--confirm` runs the jobs that write to GitHub (#3235). + (("roadmap", "run"), 300.0, False, False), (("sandbox", "deploy"), 300.0, False, False), (("sandbox", "exec"), 300.0, False, False), (("sandbox", "network-policy"), 300.0, True, False), @@ -401,8 +404,7 @@ (("tf", "cost", "breakdown"), 300.0, True, True), (("tf", "notify-plan"), 60.0, True, False), (("tf", "output"), 30.0, True, False), - # `--out` writes the plan to a file (#3235). - (("tf", "plan"), 300.0, False, False), + (("tf", "plan"), 300.0, True, False), (("tls", "inspect"), 30.0, True, False), (("valkey", "flush"), 30.0, False, False), (("valkey", "get"), 30.0, True, False), diff --git a/tests/ai/mcp/test_mcp_read_only_write_options.py b/tests/ai/mcp/test_mcp_read_only_write_options.py index 50fbe29ac..f5883470b 100644 --- a/tests/ai/mcp/test_mcp_read_only_write_options.py +++ b/tests/ai/mcp/test_mcp_read_only_write_options.py @@ -1,9 +1,9 @@ """A read-only MCP tool publishes no option that writes (#3235). -A role server and a client choose tools by the `readOnlyHint` of the exposure table. A tool that -the table marks read-only but that has an option to resolve, write, delete or create would give -a model a write that the flag says it cannot make. The help text of an option is the signal: the -generator copies it into the description of the published parameter. +A role server and a client choose tools by the `readOnlyHint` of the exposure table. A read-only +tool must not give a model a write. The help text of an option is the signal. The generator copies +it into the description of the published parameter. The rule finds an option whose description +holds the verb resolve, write, delete or create. A person reads each other option. """ from __future__ import annotations @@ -22,9 +22,9 @@ from devops_cli.ai.mcp.generator import resolve_click_command from devops_cli.ai.mcp.server import mcp -# The write verbs of the issue, in the forms that say what an option does: the base form, the -# `-s` form and the `-ing` form. A past form ("resolved via 'extends'") describes a result, not a -# write, so it does not match. +# The write verbs of the issue, in the forms that say what an option does. The forms are the base +# form, the `-s` form and the `-ing` form. A past form ("resolved via 'extends'") describes a +# result and not a write, so it does not match. _WRITE_STEMS = ("resolv", "writ", "delet", "creat") _WRITE_WORD = re.compile(r"\b(?:" + "|".join(_WRITE_STEMS) + r")(?:e|es|ing)\b", re.IGNORECASE) @@ -138,7 +138,8 @@ def test_no_read_only_tool_publishes_a_write_option() -> None: [ ("pr_check_readiness", "auto_resolve"), ("roadmap_refine", "confirm"), - ("tf_plan", "out"), + ("roadmap_run", "confirm"), + ("ci", "fix"), ], ) def test_a_tool_that_keeps_a_writing_option_is_not_read_only(tool_name: str, option: str) -> None: @@ -153,10 +154,19 @@ def test_the_readiness_command_keeps_its_auto_resolve_option() -> None: assert "--auto-resolve" in _flags(("pr", "check-readiness")) -def test_the_injection_scan_stays_read_only_and_publishes_no_sarif_option() -> None: - """The scan prints its findings, so its tool leaves out the option that writes a file.""" - tool = _published("scan_injection") +@pytest.mark.parametrize( + ("tool_name", "path", "option", "flag"), + [ + ("scan_injection", ("scan", "injection"), "sarif", "--sarif"), + ("tf_plan", ("tf", "plan"), "out", "--out"), + ], +) +def test_a_read_only_tool_leaves_out_an_option_that_writes_a_file( + tool_name: str, path: tuple[str, ...], option: str, flag: str +) -> None: + """The command keeps the option for a person. Its tool takes no path that the command writes.""" + tool = _published(tool_name) assert _read_only(tool) is True - assert "sarif" not in tool.parameters["properties"] - assert "--sarif" in _flags(("scan", "injection")) + assert option not in tool.parameters["properties"] + assert flag in _flags(path) From a753b2a9b9ad7a3acfebd0372f7c80ce3f5dccfd Mon Sep 17 00:00:00 2001 From: Daniel Petty Date: Thu, 5 Nov 2026 16:14:08 +0000 Subject: [PATCH 3/4] docs(openspec): state the change as merged --- .../3235-read-only-mcp-tools-have-no-write-option/design.md | 2 +- .../3235-read-only-mcp-tools-have-no-write-option/proposal.md | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/openspec/changes/3235-read-only-mcp-tools-have-no-write-option/design.md b/openspec/changes/3235-read-only-mcp-tools-have-no-write-option/design.md index 37c1bb59c..9ad0c05ca 100644 --- a/openspec/changes/3235-read-only-mcp-tools-have-no-write-option/design.md +++ b/openspec/changes/3235-read-only-mcp-tools-have-no-write-option/design.md @@ -17,7 +17,7 @@ ## Rejected alternatives -- **Keep `tf_plan` not read-only and publish `out`.**: The first version did so. The tool then publishes a path that the command writes, and nothing confines it. SECURITY.md rule 11 and the requirement "Write path confinement" of `mcp-tool-path-confinement` forbid that. +- **Keep `tf_plan` not read-only and publish `out`.**: The tool then publishes a path that the command writes, and nothing confines it. SECURITY.md rule 11 and the requirement "Write path confinement" of `mcp-tool-path-confinement` forbid that. - **Mark only `pr check-readiness` and add no general check.**: The scan finds more tools of the same kind. Acceptance criterion 1 of the issue says that no tool with a write option is read-only. - **Remove `auto_resolve` from the published schema of `pr_check_readiness`.**: `_PATH_PARAM_RESTRICTIONS` takes an `allowed` list only. The entry must list the seven other options, and it drops each option that the command adds later. The option is also useful for a model that has the write right. - **Scan option names, not descriptions.**: `--confirm`, `--out`, `--fix` and `--sarif` have names with no write verb. The check would miss four of the tools. diff --git a/openspec/changes/3235-read-only-mcp-tools-have-no-write-option/proposal.md b/openspec/changes/3235-read-only-mcp-tools-have-no-write-option/proposal.md index 338ca25ed..22a37bd2f 100644 --- a/openspec/changes/3235-read-only-mcp-tools-have-no-write-option/proposal.md +++ b/openspec/changes/3235-read-only-mcp-tools-have-no-write-option/proposal.md @@ -6,7 +6,7 @@ Issue #3235 reports that `CONST_MCP_EXPOSURE_TABLE` marks `pr_check_readiness` a The check that the issue asks for finds three more tools of the same kind. `roadmap_refine` has `--confirm`, which writes the proposed designs to GitHub. `scan_injection` has `--sarif`, which writes the findings to a file. `tf_plan` has `--out`, which writes the plan to a file. All three are marked read-only. -A review of the first version finds two more. `roadmap_run` has `--confirm`, which runs the same refine and intake jobs. `ci` has `--fix`, which is on by default and rewrites files. Both are marked read-only. +A read of the other options finds two more. `roadmap_run` has `--confirm`, which runs the same refine and intake jobs. `ci` has `--fix`, which is on by default and rewrites files. Both are marked read-only. ## What Changes From 0204ec21bcd2f05fb7fe98db9dbd8529ae49c254 Mon Sep 17 00:00:00 2001 From: Daniel Petty Date: Thu, 5 Nov 2026 16:16:54 +0000 Subject: [PATCH 4/4] chore(openspec): archive 3235-read-only-mcp-tools-have-no-write-option --- .../.openspec.yaml | 0 .../design.md | 0 .../proposal.md | 0 .../specs/mcp-role-server/spec.md | 0 .../tasks.md | 0 openspec/specs/mcp-role-server/spec.md | 45 +++++++++++++++++++ 6 files changed, 45 insertions(+) rename openspec/changes/{3235-read-only-mcp-tools-have-no-write-option => archive/2026-11-05-3235-read-only-mcp-tools-have-no-write-option}/.openspec.yaml (100%) rename openspec/changes/{3235-read-only-mcp-tools-have-no-write-option => archive/2026-11-05-3235-read-only-mcp-tools-have-no-write-option}/design.md (100%) rename openspec/changes/{3235-read-only-mcp-tools-have-no-write-option => archive/2026-11-05-3235-read-only-mcp-tools-have-no-write-option}/proposal.md (100%) rename openspec/changes/{3235-read-only-mcp-tools-have-no-write-option => archive/2026-11-05-3235-read-only-mcp-tools-have-no-write-option}/specs/mcp-role-server/spec.md (100%) rename openspec/changes/{3235-read-only-mcp-tools-have-no-write-option => archive/2026-11-05-3235-read-only-mcp-tools-have-no-write-option}/tasks.md (100%) diff --git a/openspec/changes/3235-read-only-mcp-tools-have-no-write-option/.openspec.yaml b/openspec/changes/archive/2026-11-05-3235-read-only-mcp-tools-have-no-write-option/.openspec.yaml similarity index 100% rename from openspec/changes/3235-read-only-mcp-tools-have-no-write-option/.openspec.yaml rename to openspec/changes/archive/2026-11-05-3235-read-only-mcp-tools-have-no-write-option/.openspec.yaml diff --git a/openspec/changes/3235-read-only-mcp-tools-have-no-write-option/design.md b/openspec/changes/archive/2026-11-05-3235-read-only-mcp-tools-have-no-write-option/design.md similarity index 100% rename from openspec/changes/3235-read-only-mcp-tools-have-no-write-option/design.md rename to openspec/changes/archive/2026-11-05-3235-read-only-mcp-tools-have-no-write-option/design.md diff --git a/openspec/changes/3235-read-only-mcp-tools-have-no-write-option/proposal.md b/openspec/changes/archive/2026-11-05-3235-read-only-mcp-tools-have-no-write-option/proposal.md similarity index 100% rename from openspec/changes/3235-read-only-mcp-tools-have-no-write-option/proposal.md rename to openspec/changes/archive/2026-11-05-3235-read-only-mcp-tools-have-no-write-option/proposal.md diff --git a/openspec/changes/3235-read-only-mcp-tools-have-no-write-option/specs/mcp-role-server/spec.md b/openspec/changes/archive/2026-11-05-3235-read-only-mcp-tools-have-no-write-option/specs/mcp-role-server/spec.md similarity index 100% rename from openspec/changes/3235-read-only-mcp-tools-have-no-write-option/specs/mcp-role-server/spec.md rename to openspec/changes/archive/2026-11-05-3235-read-only-mcp-tools-have-no-write-option/specs/mcp-role-server/spec.md diff --git a/openspec/changes/3235-read-only-mcp-tools-have-no-write-option/tasks.md b/openspec/changes/archive/2026-11-05-3235-read-only-mcp-tools-have-no-write-option/tasks.md similarity index 100% rename from openspec/changes/3235-read-only-mcp-tools-have-no-write-option/tasks.md rename to openspec/changes/archive/2026-11-05-3235-read-only-mcp-tools-have-no-write-option/tasks.md diff --git a/openspec/specs/mcp-role-server/spec.md b/openspec/specs/mcp-role-server/spec.md index bd4eca248..5b8d0242e 100644 --- a/openspec/specs/mcp-role-server/spec.md +++ b/openspec/specs/mcp-role-server/spec.md @@ -66,3 +66,48 @@ The server SHALL log the role and the tool name of each call, including a call t #### Scenario: A call is logged - **WHEN** a client calls `pr_diff` on `/review/mcp` - **THEN** the log holds `MCP tool call: role=review tool=pr_diff` and no argument + +### Requirement: A read-only tool publishes no option that writes +A tool that the exposure table marks read-only SHALL publish no option whose description holds `resolve`, `write`, `delete` or `create`. The word SHALL count as a whole word in its base, `-s` or `-ing` form and in any letter case. A past form such as `resolved` SHALL NOT count. + +#### Scenario: A read-only tool with a writing option +- **WHEN** A test reads the published schema of a read-only tool, and one option description is `Write findings to this path.` +- **THEN** The test reports the tool and the option. + +#### Scenario: A tool that is not read-only +- **WHEN** A test reads the published schema of a tool that is not read-only, and one option description is `Automatically resolve review threads.` +- **THEN** The test reports nothing. + +#### Scenario: A past form of a write verb +- **WHEN** A test reads the published schema of a read-only tool, and one option description is `Inherited policies are resolved via 'extends'.` +- **THEN** The test reports nothing. + +#### Scenario: The published tools +- **WHEN** A test reads the published schema of every tool that the server registers +- **THEN** No read-only tool has an option that the rule reports. + +### Requirement: A tool that keeps a writing option is not read-only +The tools `pr_check_readiness`, `roadmap_refine`, `roadmap_run` and `ci` SHALL be marked not read-only. They SHALL publish the options `auto_resolve`, `confirm`, `confirm` and `fix` respectively. The command `devops pr check-readiness` SHALL keep the option `--auto-resolve`. + +#### Scenario: The readiness tool +- **WHEN** A test reads the published tool `pr_check_readiness` +- **THEN** Its read-only flag is false, and its schema holds `auto_resolve`. + +#### Scenario: The refine, run and ci tools +- **WHEN** A test reads the published tools `roadmap_refine`, `roadmap_run` and `ci` +- **THEN** All three read-only flags are false, and the schemas hold `confirm`, `confirm` and `fix`. + +#### Scenario: The readiness command +- **WHEN** A test reads the options of `devops pr check-readiness` +- **THEN** The options include `--auto-resolve`. + +### Requirement: A read-only tool publishes no option for a file that its command writes +The tools `scan_injection` and `tf_plan` SHALL be marked read-only. They SHALL publish no `sarif` option and no `out` option. The commands `devops scan injection` and `devops tf plan` SHALL keep the options `--sarif` and `--out`. + +#### Scenario: The published injection scan and plan tools +- **WHEN** A test reads the published tools `scan_injection` and `tf_plan` +- **THEN** Both read-only flags are true, and the schemas hold no `sarif` property and no `out` property. + +#### Scenario: The injection scan and plan commands +- **WHEN** A test reads the options of `devops scan injection` and `devops tf plan` +- **THEN** The options include `--sarif` and `--out`.