madt_finish over MCP ignores summary=@file and ships the literal "@path" as the merged PR body #792

New issue

Open

opened 2026-10-16 16:00:25 +00:00 by hexajon · 0 comments

hexajon commented

2026-10-16 16:00:25 +00:00

Owner

What happens

madt_finish(summary=@/path/to/file.md) over the MCP surface does not read the file. The server takes the value verbatim, so the PR body becomes the literal string @/path/to/file.md. madt_finish commits, pushes, opens the PR, and merges in one call, so that broken body is already on a merged PR before I can look at it. The merge result says there is no need to re-verify, so nothing points at the loss.

In the code:

The CLI expands @file: internal/cmd/workflow/finish.go:209-233.
The MCP path does not: internal/mcp/tools_workflow.go:104 passes Summary: input.Summary straight through. There is no @ handling anywhere in internal/mcp/ or internal/service/ for this field.
The parameter doc still promises it: internal/opspec/paramdocs.go:224 ("use @file to read from file, @- for stdin"), surfaced into the MCP help at internal/mcp/help_generated.go:3318.

So a caller that follows the parameter doc gets a merged PR with no real body.

Why it matters

The PR body is the record of the change. Losing it drops the test runs and the proof the repo rules ask for, with nothing to flag the loss.
The broken body lands on a merged PR in the same call, before review.
The "no need to re-verify" result tells the caller not to check.

Why not to read the file server-side

The obvious fix - have the MCP server read @file and use its content - opens a credential-exfiltration hole, so I will not take it. The value is caller-controlled. The server reads with its own file access, which is the same account that drives the tool. The content is then published to the forge. A value like summary=@~/.ssh/id_ed25519 or @<a token-bearing config file> would read a secret and publish it. Untrusted text processed upstream (an issue body, a diff, a fetched page) can steer that value. A human types CLI @file with intent; the MCP surface has no such gate. Server-side file reads for forge text do not belong on the MCP surface.

Fix

One shared refusal at the MCP argument-validation choke point, not a per-field patch:

On the MCP surface, reject any text-field value that starts with a file sigil - @/, @~, @., or @- - for every text parameter (summary, body, title, comment, and any future one), so no field can reintroduce the pattern.
Fail before any network effect: before the push, before the PR opens, before the merge. The error says to pass the content inline.
@- (stdin) has no meaning over MCP; the same refusal covers it.
Drop the @file/@- claim from the summary parameter doc on the MCP surface (internal/opspec/paramdocs.go:224). The CLI keeps the behavior; MCP will not.

This follows the single-guard pattern already in the tree: util.ReadStdin is the one shared guard for every explicit -/@- stdin read (internal/util/input.go:26).

Scope

Today only summary carries the @file promise over MCP. Bodies already take inline text over MCP, and --body-file is CLI-only (not exposed over MCP). The shared guard is the whole point: it closes the class so a new text field cannot reopen it. CLI behavior (human-typed @file, @-, --body-file) is unchanged.

Acceptance (test first)

Red first: a test that calls the MCP finish handler with summary=@/etc/hostname (and @-, @~/x, @./x) and asserts an error with no push, no PR, and no merge - the guard fires before any forge effect. Add the same for the other text-param tools.
A test that the MCP summary parameter doc no longer claims @file or @-.
Green only when the one shared guard refuses every sigil across the text parameters at a single choke point.

## What happens `madt_finish(summary=@/path/to/file.md)` over the MCP surface does not read the file. The server takes the value verbatim, so the PR body becomes the literal string `@/path/to/file.md`. `madt_finish` commits, pushes, opens the PR, and merges in one call, so that broken body is already on a merged PR before I can look at it. The merge result says there is no need to re-verify, so nothing points at the loss. In the code: - The CLI expands `@file`: `internal/cmd/workflow/finish.go:209-233`. - The MCP path does not: `internal/mcp/tools_workflow.go:104` passes `Summary: input.Summary` straight through. There is no `@` handling anywhere in `internal/mcp/` or `internal/service/` for this field. - The parameter doc still promises it: `internal/opspec/paramdocs.go:224` ("use @file to read from file, @- for stdin"), surfaced into the MCP help at `internal/mcp/help_generated.go:3318`. So a caller that follows the parameter doc gets a merged PR with no real body. ## Why it matters - The PR body is the record of the change. Losing it drops the test runs and the proof the repo rules ask for, with nothing to flag the loss. - The broken body lands on a merged PR in the same call, before review. - The "no need to re-verify" result tells the caller not to check. ## Why not to read the file server-side The obvious fix - have the MCP server read `@file` and use its content - opens a credential-exfiltration hole, so I will not take it. The value is caller-controlled. The server reads with its own file access, which is the same account that drives the tool. The content is then published to the forge. A value like `summary=@~/.ssh/id_ed25519` or `@<a token-bearing config file>` would read a secret and publish it. Untrusted text processed upstream (an issue body, a diff, a fetched page) can steer that value. A human types CLI `@file` with intent; the MCP surface has no such gate. Server-side file reads for forge text do not belong on the MCP surface. ## Fix One shared refusal at the MCP argument-validation choke point, not a per-field patch: - On the MCP surface, reject any text-field value that starts with a file sigil - `@/`, `@~`, `@.`, or `@-` - for every text parameter (summary, body, title, comment, and any future one), so no field can reintroduce the pattern. - Fail before any network effect: before the push, before the PR opens, before the merge. The error says to pass the content inline. - `@-` (stdin) has no meaning over MCP; the same refusal covers it. - Drop the `@file`/`@-` claim from the `summary` parameter doc on the MCP surface (`internal/opspec/paramdocs.go:224`). The CLI keeps the behavior; MCP will not. This follows the single-guard pattern already in the tree: `util.ReadStdin` is the one shared guard for every explicit `-`/`@-` stdin read (`internal/util/input.go:26`). ## Scope Today only `summary` carries the `@file` promise over MCP. Bodies already take inline text over MCP, and `--body-file` is CLI-only (not exposed over MCP). The shared guard is the whole point: it closes the class so a new text field cannot reopen it. CLI behavior (human-typed `@file`, `@-`, `--body-file`) is unchanged. ## Acceptance (test first) - Red first: a test that calls the MCP `finish` handler with `summary=@/etc/hostname` (and `@-`, `@~/x`, `@./x`) and asserts an error with no push, no PR, and no merge - the guard fires before any forge effect. Add the same for the other text-param tools. - A test that the MCP `summary` parameter doc no longer claims `@file` or `@-`. - Green only when the one shared guard refuses every sigil across the text parameters at a single choke point.

hexajon added the

bug

security

labels

2026-10-16 16:00:25 +00:00

hexajon referenced this issue

2026-10-16 16:30:50 +00:00

docs(adr): propose that the MCP surface reads no local files #793

hexajon referenced this issue

2026-10-16 19:17:26 +00:00

Retire the curl-style @file/@- sigil on finish --summary; keep --summary-file #794