Skip to content

docs: MCP App review recipe + README updates (#1578)#1674

Closed
cliffhall wants to merge 2 commits into
v2/mainfrom
v2/1578-docs
Closed

docs: MCP App review recipe + README updates (#1578)#1674
cliffhall wants to merge 2 commits into
v2/mainfrom
v2/1578-docs

Conversation

@cliffhall

Copy link
Copy Markdown
Member

Closes #1578

Summary

Adds docs/mcp-app-review.md — the CLI-first → one-shot-web recipe for automated MCP App review: --app-info probe → deep-link navigate → rendered widget, plus §5 OAuth-gated servers (stored-auth handoff), §6 SSH port-forward (both ports), §7 HTTP proxy.

Adapted from PR #1510's reference doc to only document what actually merged and to match the current implementation:

The per-client READMEs were already updated in their own Wave 4 PRs (web deep-link scheme + data-* contract in #1576/#1577; CLI handoff format in #1666; CLI refresh in #1665). Proxy/undici is already documented in the CLI README.

Docs-only PR — no code, no tests. Part of the #1579 decomposition (Wave 4, lands last). Per AGENTS.md the Closes line won't auto-fire on v2/main; the issue is closed and its board card moved on the Wave 4 rollup merge.

🤖 Generated with Claude Code

Closes #1578

Adds docs/mcp-app-review.md — the CLI-first → one-shot-web recipe for automated
MCP App review: `--app-info` probe → deep-link navigate → rendered widget, plus
OAuth-gated servers (stored-auth handoff, now honoring refresh_token per #1665),
SSH port-forward (both ports), and HTTP proxy support.

Adapted from PR #1510's reference doc to only document what actually merged and
to match the current implementation:
- Correct driver contract: `data-testid="connection-status"` + `data-status`
  /`data-error-message`/`data-deeplink`, `data-testid="open-app"`, and the
  `data-app-status="ready"` app lifecycle (not the reference's stale selectors).
- The zero-click `autoOpen` path (#1577) alongside the click-to-open variant.
- `--use-stored-auth` now refreshes a stored refresh_token (#1665), so the
  "token is not refreshed" caveat is replaced.

The per-client READMEs (web deep-link scheme + data-* contract, CLI handoff +
refresh + proxy) were updated in their own Wave 4 PRs; this adds the root README
pointer to docs/. Proxy/undici is already documented in the CLI README.

Part of the #1579 decomposition (Wave 4, docs — lands last).

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01BrihGWcrM9JGRyu41nzZYw
@cliffhall cliffhall added the v2 Issues and PRs for v2 label Jul 12, 2026
@cliffhall

Copy link
Copy Markdown
Member Author

@claude review

@claude

claude Bot commented Jul 12, 2026

Copy link
Copy Markdown
Contributor

Claude finished @cliffhall's task in 2m 50s —— View job


Review: docs PR #1674

  • Read the changed files (docs/mcp-app-review.md, README.md)
  • Verify documented CLI flags / exit codes against the implementation
  • Verify the data-testid / data-* contract & deep-link params against the web implementation
  • Verify cross-references (docs/mcp-server-configuration.md, README anchors)
  • Post review

This is a clear, well-organized recipe — the CLI-first → one-shot-web narrative is genuinely useful and the CLI sections check out precisely. My concern is that several sections document functionality that is not yet on v2/main (the current branch HEAD is a30a5760, on top of Wave 3 a029e42a; Wave 2 #1576/#1577 and the CLI refresh #1665 are not present). The PR body says this "lands last" and its dependencies merged in their own PRs — but mechanically they haven't merged into v2/main yet, so as written this doc describes behavior the shipped code doesn't have.

⚠️ Documents unmerged behavior

1. §4 deep-link auto-connect params + connection-status contract — not in the codebase.
The params autoConnect, openApp, appArgs, autoOpen, and the serverUrl deep-link auto-connect (lines 71–130) are not found anywhere in clients/web/src. Likewise data-testid="connection-status" with data-status / data-deeplink / data-error-message (lines 104–130) don't exist. This is the #1576/#1577 (Wave 2) work — the CLI source even carries a TODO waiting on it (cli.ts:548: "interim format pending the web deep-link auto-connect work (#1576); reconcile … once it lands").
What does exist and is correctly documented: data-testid="apps-form", data-testid="open-app", and data-app-status="ready" (AppsScreen/AppDetailPanel).

2. §5 --use-stored-auth refresh grant — not implemented.
Lines 146–151 state it "runs the OAuth refresh grant and injects the fresh access token, persisting the rotation … A failed refresh (revoked token) exits 3." The current implementation (cli.ts:935–967) injects only the stored access token blindly — the comment there says exactly that and lists the refresh as a "Follow-up" (#1665, unmerged):

only the access token is injected, blindly; a stale one surfaces as HTTP 401 → exit 3 … Follow-up: when the stored tokens.refresh_token is present … mint a fresh one instead of injecting the stale token.

🔗 Broken links

3. README.md:47 links to docs/mcp-server-configuration.md, which does not exist. Per specification/v2_catalog_launch_config.md (gap G6) that v1.5 doc's port to v2 is still pending. The CLI/TUI READMEs already carry this dangling link, but this PR adds a new one from the root README. Consider gating this pointer on the config doc actually landing (or add a stub). Fix this →

4. §4 anchor clients/web/README.md#deep-link-auto-connect (line 73) doesn't resolve. The web README section is ## MCP Apps screen automation contract (anchor #mcp-apps-screen-automation-contract), and it does not document deep-link auto-connect or connection-status. Fix this →

Minor

5. §"Local fixture" (lines 201–203) renders oddly. The parenthetical got split by a blank line, so - resource preset … starts a bulleted list mid-sentence:

…tool (preset mcp_app_demo

- resource preset mcp_app_demo_widget) whose widget…

Collapse it to a single line, e.g. (presets mcp_app_demo+mcp_app_demo_widget).

✅ Verified accurate

  • Exit-code map 0–5, no_app=2, auth_required=3 (error-handler.ts)
  • --app-info probe, tools/list --app-info NDJSON, --format json, --tool-args-json, --tool-name, --wait-for-auth, --print-handoff, --use-stored-auth all exist
  • App-info JSON shape (resourceUri/csp/permissions), resource-side CSP read (collectAppInfo)
  • Loopback env vars, oauth.json at ~/.mcp-inspector/storage/, URL normalization via new URL().href
  • Proxy support: lazy EnvHttpProxyAgent, HTTPS_PROXY/HTTP_PROXY/NO_PROXY (core/mcp/node/transport.ts)
  • mcp_app_demo / mcp_app_demo_widget fixture presets exist

Bottom line: if this doc is merged only as part of the Wave 4 rollup that also lands #1576/#1577/#1665, findings #1 and #2 resolve themselves — but the rollup must include them, or the doc will describe features v2/main doesn't have. #3 and #4 (broken links) should be fixed regardless.

- Root README no longer links to docs/mcp-server-configuration.md (not yet
  ported to v2); keep only the mcp-app-review.md pointer (review #3).
- Collapse the Local-fixture parenthetical onto one line so prettier no longer
  renders '- resource preset …' as a mid-sentence bullet (review #5).

Findings #1/#2 (documents unmerged #1576/#1577/#1665 behavior) and #4 (the
web-README #deep-link-auto-connect anchor) resolve in the Wave 4 rollup, which
lands this doc together with those code PRs — the reviewer noted the same.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01BrihGWcrM9JGRyu41nzZYw
@cliffhall

Copy link
Copy Markdown
Member Author

Thanks — fixed the two link/rendering issues in e6278aea: dropped the new root-README link to docs/mcp-server-configuration.md (not yet ported to v2), and collapsed the Local-fixture parenthetical so prettier no longer renders it as a mid-sentence bullet. Findings #1/#2 (documents #1576/#1577/#1665 behavior) and #4 (the web-README #deep-link-auto-connect anchor) resolve in the Wave 4 rollup, which lands this doc together with those code PRs — as you noted.

@cliffhall

Copy link
Copy Markdown
Member Author

Superseded by the Wave 4 rollup #1676, which contains this change (reviewed to LGTM here) merged base→tip. Closing in favor of the rollup.

@cliffhall cliffhall closed this Jul 12, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

v2 Issues and PRs for v2

Projects

None yet

Development

Successfully merging this pull request may close these issues.

docs: MCP App review recipe + README updates for new CLI flags, deep-links, and proxy support

1 participant