diff --git a/docs/MCP_TOOLS.md b/docs/MCP_TOOLS.md index bd416f53d..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. | @@ -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,8 +112,8 @@ 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_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_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) | `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. | @@ -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:** @@ -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:** @@ -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:** @@ -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. | @@ -2314,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/archive/2026-11-05-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 new file mode 100644 index 000000000..1491b921e --- /dev/null +++ b/openspec/changes/archive/2026-11-05-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/archive/2026-11-05-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 new file mode 100644 index 000000000..9ad0c05ca --- /dev/null +++ b/openspec/changes/archive/2026-11-05-3235-read-only-mcp-tools-have-no-write-option/design.md @@ -0,0 +1,27 @@ +# Design + +## Decisions + +- **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. 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 + +- **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. +- **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/archive/2026-11-05-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 new file mode 100644 index 000000000..22a37bd2f --- /dev/null +++ b/openspec/changes/archive/2026-11-05-3235-read-only-mcp-tools-have-no-write-option/proposal.md @@ -0,0 +1,56 @@ +# 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. `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 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 + +- 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 + +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, A read-only tool publishes no option for a file that its command writes + +## 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. 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. 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. 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/archive/2026-11-05-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 new file mode 100644 index 000000000..6abcf121d --- /dev/null +++ b/openspec/changes/archive/2026-11-05-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`. 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`. diff --git a/openspec/changes/archive/2026-11-05-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 new file mode 100644 index 000000000..8f7ee7976 --- /dev/null +++ b/openspec/changes/archive/2026-11-05-3235-read-only-mcp-tools-have-no-write-option/tasks.md @@ -0,0 +1,30 @@ +# Tasks + +## 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`. 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`, `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`. 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 +- 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/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`. diff --git a/src/devops_cli/ai/mcp/generator.py b/src/devops_cli/ai/mcp/generator.py index 47cd4cedb..82ad1c0aa 100644 --- a/src/devops_cli/ai/mcp/generator.py +++ b/src/devops_cli/ai/mcp/generator.py @@ -86,6 +86,26 @@ "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 `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 b03a5437c..e3584129e 100644 --- a/src/devops_cli/config/constants.py +++ b/src/devops_cli/config/constants.py @@ -265,6 +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 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), @@ -297,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), @@ -348,7 +353,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,8 +377,10 @@ (("review", "pr"), 300.0, False, False), (("review", "stats"), 60.0, True, False), (("roadmap", "intake"), 300.0, False, False), - (("roadmap", "refine"), 300.0, True, False), - (("roadmap", "run"), 300.0, True, False), + # `--confirm` writes the proposed designs to GitHub (#3235). + (("roadmap", "refine"), 300.0, False, 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), 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..f5883470b --- /dev/null +++ b/tests/ai/mcp/test_mcp_read_only_write_options.py @@ -0,0 +1,172 @@ +"""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 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 + +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 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) + + +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"), + ("roadmap_run", "confirm"), + ("ci", "fix"), + ], +) +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")) + + +@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 option not in tool.parameters["properties"] + assert flag in _flags(path)