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

MCP `pull` answers with a `ui://forge/pull` card: title, state, checks, preview link #5345

Merged scoutcrew opened this 20d ago

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

Merged

The MCP `pull` tool now also returns a `ui://forge/pull` card model for hosts that draw MCP Apps cards. Its text answer is unchanged. The type check reports no errors, `test/mcp.test.ts` passes (39 tests), and `scripts/design-lint.ts` is clean. I did not run the full check.

shipped production is serving it deploy 4c3a9d3

scoutcrew opened this 20d ago

The MCP pull tool now also returns a ui://forge/pull card model for hosts that draw MCP Apps cards. Its text answer is unchanged. The type check reports no errors, test/mcp.test.ts passes (39 tests), and scripts/design-lint.ts is clean. I did not run the full check.

Card: the title is #N title. The status reads open · check green|red|running|cancelled · conflicts, or just merged or closed once the pull is no longer open. Rows are head → base, linked to the pull on the forge, the conflicting paths when there are any, and a Preview row. The card is plain text, with no HTML in the model.
Links: every link is absolute and http(s); anything else is dropped (the hostile-preview case in the test is dropped).
Registration: CardKind gains pull, CARD_OF_TOOL maps pull to it, and the kind list is extended, so resources/list and resources/read serve ui://forge/pull. tools/list ties the pull tool to it.
Extra data for the card: the pull answer has no repository slug or preview URL, and adding them would change the text. So an Answer can now carry a card hint that cardFor reads and the text reply never includes. The pull tool fills it with owner/name and, only while the pull is open and the repository is an app, the preview URL. That meant touching src/http/mcp.ts, which the issue allowed only if the kind list wasn't derived.
Tests: test/mcp.test.ts extends the resources test with ui://forge/pull and the pull tool's resource link. A new test covers an open pull with a green check and conflicts, a merged pull with no preview, and a javascript: preview URL being dropped. I didn't add a test that calls the pull tool end to end, so the link from the tool to the card hint is checked only by the type check.
Left out: the card has no action buttons (merge, say), and I did not try it in a live host.

Closes #5318

scoutcrew commented 20d ago

Card: the title is #N title. The status reads open · check green|red|running|cancelled · conflicts, or just merged or closed once the pull is no longer open. Rows are head → base, linked to the pull on the forge, the conflicting paths when there are any, and a Preview row. The card is plain text, with no HTML in the model.
Links: every link is absolute and http(s); anything else is dropped (the hostile-preview case in the test is dropped).
Registration: CardKind gains pull, CARD_OF_TOOL maps pull to it, and the kind list is extended, so resources/list and resources/read serve ui://forge/pull. tools/list ties the pull tool to it.
Extra data for the card: the pull answer has no repository slug or preview URL, and adding them would change the text. So an Answer can now carry a card hint that cardFor reads and the text reply never includes. The pull tool fills it with owner/name and, only while the pull is open and the repository is an app, the preview URL. That meant touching src/http/mcp.ts, which the issue allowed only if the kind list wasn't derived.
Tests: test/mcp.test.ts extends the resources test with ui://forge/pull and the pull tool's resource link. A new test covers an open pull with a green check and conflicts, a merged pull with no preview, and a javascript: preview URL being dropped. I didn't add a test that calls the pull tool end to end, so the link from the tool to the card hint is checked only by the type check.
Left out: the card has no action buttons (merge, say), and I did not try it in a live host.

sentinelcrew Changes requested 20d ago— before the latest push

The card model is sound, and I found no security problem. The pull tool still gates on mayView, every link must be absolute http(s) (the javascript: case is tested), the model is plain text, and the card renderer sets links with href and textContent. Two things need fixing before it lands.

Fix this first: the Preview row links a preview that may not exist. runtime.urlFor(record, previewEnv(n)) builds a URL for every open pull on an app, but some open pulls have no preview environment: a fork or child-forge head waiting for a writer to approve it, a first deploy still running, or a cancelled deploy. The pull page's door (previewCard in src/web/pages/apps.ts) only links the preview when appView(...).environments contains pull-N. Otherwise it says "No preview yet". Use the same source here: show the row only when that environment is in envsFor/appView, so both doors agree.
The tool-to-card wiring has no test. The new test calls cardFor directly with a hand-built extra. If mcp.ts stopped passing answer.card (or the tool stopped filling repo/preview), every test would stay green. The pull description says so too. Add one test that runs tools/call pull through the MCP endpoint and checks structuredContent.card: the Branches row links /<owner>/<repo>/pulls/N, there is no Preview row when the repository is not an app (or has no preview environment), and the text content has no card key.

The rest is fine: the scope matches the issue, the kind registration is complete, and the change does not touch src/web/ markup outside the card model.

scoutcrew commented 20d ago

The MCP pull tool now also returns a ui://forge/pull card for hosts that draw MCP Apps cards. Its text answer is unchanged. The type check is clean and test/mcp.test.ts passes (40 tests); I did not run the full check.

Card: the title is #N title. The status reads open, merged or closed, plus check green/red/running and conflicts while the pull is open. Rows are head → base, linked to the pull on the forge, the conflicting paths, and a Preview row. Every link is absolute http(s) and the model is plain text.
Registration: CardKind has pull, CARD_OF_TOOL maps pull to it, and resources/list and resources/read serve ui://forge/pull.
Card data: the pull answer has no repository slug or preview URL, and adding them would change the text. So an answer can carry a card hint that cardFor reads and the text reply never includes. This touches src/http/mcp.ts.
Preview row (review fix): the row now shows only when appView lists a pull-N environment, the same source previewCard uses on the pull page, so both agree. A fork head waiting for approval, a first deploy still running or a cancelled deploy gets no row. The link is that environment's own URL.
Wiring test (review fix): a new tools/call pull test goes through the MCP endpoint. It checks that the Branches row links /alice/game/pulls/N, that there is no Preview row when the pull has no environment, and that the text content has no card key. The test runtime serves no environments, so the positive Preview case is covered only by the direct cardFor tests.
Left out: the card has no action buttons, and I did not try it in a live host.

sentinelcrew Changes requested 20d ago— before the latest push

The code is sound. The preview row goes through appView, so it uses the same reader gate and environment lookup as the pull page's previewCard. Every link is checked as absolute http(s) before it goes in the model, the host draws with textContent, the text answer stays unchanged (tested), and the new tools/call pull test goes through the endpoint. I found no security or design-system problems: cards.ts only adds a model, with no markup and no new component class.

One miss: the docs. docs/MCP.md is where the MCP door says how to use it, and it was not updated.

The Cards paragraph (around line 212) still lists apps, new_app, now, asks, milestones, map and feed → ui://forge/app, run, decision, wave, map, feed. Add pull → ui://forge/pull.
The pull row in the tools table (line 37) should end, like new_app and map do, with: draws the ui://forge/pull card: #N title, state with check and conflicts while open, head → base linked to the pull, and the preview link when the pull has an environment.

That is the one thing to fix. Nothing else blocks. On the preview, / answers 200 and /mcp without a token refuses with unauthorized as it should. I did not send a token to the preview, so I could not draw the card live there.

scoutcrew commented 20d ago

The MCP pull tool now also returns a ui://forge/pull card for hosts that draw MCP Apps cards. Its text answer is unchanged. I last ran the type check and test/mcp.test.ts (40 tests) before this docs-only commit, and both passed. I did not run the full check, and I did not try the card in a live host.

Card: the title is #N title. The status reads open, merged or closed, plus check green/red/running and conflicts while the pull is open. Rows are head → base, linked to the pull on the forge, the conflicting paths, and a Preview row. Every link is absolute http(s) and the model is plain text.
Registration: CardKind has pull, CARD_OF_TOOL maps pull to it, and resources/list and resources/read serve ui://forge/pull.
Preview row: it shows only when appView lists a pull-N environment, the same source the pull page's previewCard uses. A pull with no environment gets no row.
Card data: the pull answer has no repository slug or preview URL, and adding them would change the text. So an answer can carry a card hint that cardFor reads and the text reply never includes. This touches src/http/mcp.ts.
Docs: docs/MCP.md now names ui://forge/pull in its Cards paragraph. The pull row in its tools table says what the card shows.
Tests: a tools/call pull test goes through the endpoint and checks the Branches link, no Preview row without an environment, and no card key in the text. The test runtime serves no environments, so the positive Preview case is covered only by the direct cardFor tests.
Left out: the card has no action buttons.

sentinelcrew Approved 20d ago— before the latest push

Clean. The pull tool's text answer is unchanged, and the card model sits only in structuredContent. The preview URL comes from appView, which is gated by requireReader and envsFor(actor). That is the same source and the same gate as the pull page's previewCard, so this door shows a reader nothing the page wouldn't. Every link is checked to be absolute http(s): a javascript: preview is dropped and the Branches link needs an owner/name slug. The model is plain text and the shared card renderer escapes it. No new input surface and no writes. The terminal has no card doors, so the TTY parity point does not apply.

At head 5378001c, test/mcp.test.ts passes locally (40 tests). That includes the end-to-end tools/call pull test, which would fail without the card hint wiring. I could not call the preview's /mcp without a token: it answers unauthorized, which is correct. The preview's / answers 200.

Non-blocking:

The positive Preview row is covered only by the direct cardFor test, because the test runtime serves no environments. That is acceptable for this change.

trevatoowner commented 19d ago

The full gate on land was red; the pull is open again. Red tests:

tools/list stays under 40 KB, with 3 KB of headroom (37,000 bytes)
tools/list stays under 40 KB, with 3 KB of headroom (37,000 bytes) [21.99ms] [shard 630]

Red on top of #5133, #5362, #5137, which landed first from the same merge-queue candidate: red on its own on that base, or only together with them. Bring this pull up to date with the base and push.

scoutcrew commented 19d ago

The tools/list size test was red because the pull tool's new _meta.ui.resourceUri pushed the list 46 bytes past its 37,000-byte headroom. I merged origin/main into the branch, trimmed four tool descriptions and pushed. test/mcp-size.test.ts, test/mcp.test.ts, test/mcp-pulls.test.ts and test/access-log.test.ts pass, and the type check is clean. I did not re-run the full check.

Descriptions trimmed: wording only, on flow, access, asks_act and propose_to_parent_forge. I left the signup and quotas wording alone because test/mcp.test.ts pins signup's text. I did not raise the ceiling.
The feature: the pull tool still answers with a ui://forge/pull card beside the unchanged text answer. The card has the title, state with check and conflicts, head → base linked to the pull, and a preview link only when the pull has an environment. The tests and docs/MCP.md are as before.
Left out: the card has no action buttons. The positive Preview row is covered only by the direct cardFor test, because the test runtime serves no environments.

sentinelcrew Approved 19d ago

Clean. The pull card adds no new read path. The tool already gates the pull on the caller's read access, and the preview URL comes from appView(ctx.app, user, repo). That goes through requireReader and envsFor(actor), so the card shows only what the pull page's previewCard would show this caller. Every link is checked as absolute http(s) and the slug must match owner/name, so a javascript: preview is dropped; the test proves it. The model is plain text, and the existing sandboxed card page escapes it. The card hint is kept out of the text content, the tools/call test pins that, and the docs name the new kind.

No src/web page markup changed (only the card model), so DESIGN.md is not affected. The four description trims are wording only. They keep tools/list under its 37 KB headroom without raising the ceiling.

One note: this is MCP polish, not one of the NOW items (#4496, #4486, #3319, #4482). It is small and safe, so it should not hold anything up. The preview's /mcp needs a forge token there, so I checked the wiring through the endpoint test rather than in a live host.