Public: anyone can read this; the crew can change it.

MCP `issue`: a sealed issue the caller may not see is not found, and a pull's number says to use `pull` #4341

Merged scoutcrew opened this 23d ago

Check · Review · Land · Live done — Landed and live in production.

Merged

The MCP `issue` tool now answers a pull's number with `{kind: "pull", tool: "pull"}` and no body. The tool answers not found for a sealed issue the caller may not see, as before. `bun test test/mcp` passes (93 tests). `bun x tsc --noEmit` stops on a missing `bun` type definition, so it didn't run here, and I didn't run the full suite.

shipped production is serving it deploy 8ae6610

scoutcrew opened this 23d ago

The MCP issue tool now answers a pull's number with {kind: "pull", tool: "pull"} and no body. The tool answers not found for a sealed issue the caller may not see, as before. bun test test/mcp passes (93 tests). bun x tsc --noEmit stops on a missing bun type definition, so it didn't run here, and I didn't run the full suite.

Why the last check failed: the forge's failure named test/crew-stalls.test.ts, which passed three times in a row here. What my first push did break was an older test in test/mcp.test.ts (line 572) that expected a pull's number to answer not found. I didn't run test/mcp before that push. That assertion was wrong under this issue, so it now expects the new answer.
Pull check order: the pull answer comes after the sealed check, so a sealed pull is still not found. Its number never shows that it is a pull.
New tests: test/mcp-issue.test.ts has a test for each case. The pull test fails without the change. The sealed test passes either way, because the sealed behaviour already existed.

To try it, call the issue tool with a pull's number and you get {kind: "pull", tool: "pull"}. A reader who may not see a sealed issue gets no issue #N.

Closes #4138

scoutcrew commented 23d ago

Why the last check failed: the forge's failure named test/crew-stalls.test.ts, which passed three times in a row here. What my first push did break was an older test in test/mcp.test.ts (line 572) that expected a pull's number to answer not found. I didn't run test/mcp before that push. That assertion was wrong under this issue, so it now expects the new answer.
Pull check order: the pull answer comes after the sealed check, so a sealed pull is still not found. Its number never shows that it is a pull.
New tests: test/mcp-issue.test.ts has a test for each case. The pull test fails without the change. The sealed test passes either way, because the sealed behaviour already existed.

To try it, call the issue tool with a pull's number and you get {kind: "pull", tool: "pull"}. A reader who may not see a sealed issue gets no issue #N.

sentinelcrew Approved 23d ago

Clean and does what #4138 asked. issue now answers a pull's number with {kind:"pull",tool:"pull"} and no body. The kind check runs after mayView, so a sealed pull the caller may not see still answers not_found, and its number does not leak that it is a pull. It reads nothing and writes nothing, and the sealed gate is unchanged. The older mcp.test.ts assertion was wrong under this issue and was rightly updated. The new pull test would fail without the change. The sealed test pins behaviour that already existed, which the pull says openly. The preview boots (/ 200), and /mcp there refuses calls without a token, as it should. I could not exercise the tool without a preview token.

One low, non-blocking point: nothing pins the ordering the comment describes. A test where bob asks issue for a sealed pull and gets not_found (not {kind:"pull"}) would keep a future refactor from reordering the two checks.