Skip to content

fix(mcp): surface prepare confirmation hash to UI apps - #72935

Merged
charlesvien merged 5 commits into
masterfrom
fix/loops-review-confirmation-hash
Jul 22, 2026
Merged

fix(mcp): surface prepare confirmation hash to UI apps#72935
charlesvien merged 5 commits into
masterfrom
fix/loops-review-confirmation-hash

Conversation

@charlesvien

@charlesvien charlesvien commented Jul 22, 2026

Copy link
Copy Markdown
Member

Problem

The loops-review MCP app's Create button always fails with "Failed to create the loop." The card reads confirmation_hash from structuredContent on the loops-create-prepare result, but buildToolResultPayload only attaches structuredContent to tools that have a UI resource, which -prepare tools never do. The hash only rides in the TOON text toward the model, so the card can never complete the prepare/execute flow.

Review also surfaced a confirmation-integrity gap in the card itself: it forwarded the agent-assembled config verbatim to loops-create-prepare, so fields the review UI never rendered (connector scopes, MCP installations, sandbox environment) would be persisted by a click that never showed them.

Changes

  • buildToolResultPayload now carries confirmed-action prepare results on _meta under APP_DATA_META_KEY, the host/app-only channel UI apps already hydrate from. Model-facing text and structuredContent behavior are unchanged, so no token cost for agents.
  • Added an isPrepareConfirmedActionResult shape guard beside the result type in confirmed-action-runtime.ts.
  • The loops-review card reads the hash from _meta first with a structuredContent fallback, and its missing-hash error now says what went wrong.
  • The card builds the prepare arguments from an explicit allowlist of the fields the review UI renders, and LoopReviewView now renders the capability-bearing fields that were missing: PostHog access scope, connectors, sandbox environment, description, paused state and overlap policy. What is shown, sent and created is now the same set.

How did you test this code?

Added unit tests in build-tool-result.test.ts: the prepare-shaped result lands on _meta[APP_DATA_META_KEY] (the regression no existing test caught: a UI app driving a confirmed action had no machine-readable hash), plus guard false-positive cases for non-prepare shapes. Ran vitest run tests/unit/build-tool-result.test.ts tests/unit/confirmed-action-runtime.test.ts (32 passed), pnpm typecheck and pnpm lint in services/mcp. Not manually tested against a deployed server; the failing flow was diagnosed from PostHog Code desktop logs where prepare POSTs returned 200 but the card showed the generic no-hash error. The new LoopReviewView rows were not visually rendered.

@charlesvien charlesvien self-assigned this Jul 22, 2026
@github-actions

github-actions Bot commented Jul 22, 2026

Copy link
Copy Markdown
Contributor

🤖 CI report

Bundle size — no change

Uncompressed size of every built .js bundle, compared against the base branch.

Total: 64.63 MiB · no change

No file changed by more than 1000 B.

Posted automatically by build-bundle-size-report · uncompressed bytes from dist-report

Eager graph — within budget

How much code each root ships on the eager path — downloaded and parsed before the surface is interactive. Measured from the esbuild output chunks (post-tree-shake, static imports only); lazy import() / React.lazy chunks are not counted.

Root Eager (shipped) Δ vs base Budget
entry (logged-out pages, app bootstrap)
src/index.tsx
1.24 MiB · 22 files no change ███░░░░░░░ 27.5% of 4.51 MiB
authenticated shell (every logged-in page)
src/scenes/AuthenticatedShell.tsx
8.21 MiB · 3,001 files no change ████████░░ 84.5% of 9.71 MiB

🟢 node_modules/monaco-editor/ stays out of src/index.tsx
🟢 src/lib/components/ActivityLog/describers stays out of src/index.tsx
🟢 [object Object] stays out of src/index.tsx
🟢 [object Object] stays out of src/index.tsx
🟢 node_modules/monaco-editor/ stays out of src/scenes/AuthenticatedShell.tsx
🟢 src/lib/components/ActivityLog/describers stays out of src/scenes/AuthenticatedShell.tsx
🟢 [object Object] stays out of src/scenes/AuthenticatedShell.tsx
🟢 [object Object] stays out of src/scenes/AuthenticatedShell.tsx

Largest files eagerly shipped from src/index.tsx
Size File
126.8 KiB ../node_modules/.pnpm/react-dom@18.3.1_react@18.3.1/node_modules/react-dom/cjs/react-dom.production.min.js
24.6 KiB ../node_modules/.pnpm/buffer@6.0.3/node_modules/buffer/index.js
6.3 KiB ../node_modules/.pnpm/react@18.3.1/node_modules/react/cjs/react.production.min.js
4.5 KiB ../node_modules/.pnpm/@jspm+core@2.1.0/node_modules/@jspm/core/nodelibs/browser/process.js
3.9 KiB ../node_modules/.pnpm/scheduler@0.23.2/node_modules/scheduler/cjs/scheduler.production.min.js
1.4 KiB ../node_modules/.pnpm/base64-js@1.5.1/node_modules/base64-js/index.js
1.3 KiB src/RootErrorBoundary.tsx
912 B ../node_modules/.pnpm/ieee754@1.2.1/node_modules/ieee754/index.js
789 B src/scenes/ChunkLoadErrorBoundary.tsx
762 B src/index.tsx
Largest files eagerly shipped from src/scenes/AuthenticatedShell.tsx
Size File
281.3 KiB ../node_modules/.pnpm/posthog-js@1.405.2/node_modules/posthog-js/dist/rrweb.js
267.7 KiB ../node_modules/.pnpm/@posthog+icons@0.38.0_react-dom@18.3.1_react@18.3.1__react@18.3.1/node_modules/@posthog/icons/dist/posthog-icons.es.js
236.0 KiB src/taxonomy/core-filter-definitions-by-group.json
224.4 KiB ../node_modules/.pnpm/posthog-js@1.405.2/node_modules/posthog-js/dist/module.js
167.1 KiB src/queries/validators.js
154.3 KiB ../node_modules/.pnpm/re2js@0.4.1/node_modules/re2js/build/index.esm.js
126.8 KiB ../node_modules/.pnpm/react-dom@18.3.1_react@18.3.1/node_modules/react-dom/cjs/react-dom.production.min.js
106.2 KiB src/lib/api.ts
94.0 KiB ../packages/quill/packages/quill/dist/index.js
93.3 KiB ../node_modules/.pnpm/prosemirror-view@1.40.1/node_modules/prosemirror-view/dist/index.js

Posted automatically by check-eager-graph · sizes are eager output bytes (shipped, post-tree-shake) from the esbuild metafile · part of #32479

Toolbar bundle — eager 2.18 MiB within budget

What the toolbar ships to customer pages, measured from the esbuild output (minified, post-tree-shake). The eager set is the entry plus everything statically imported from it — fetched before any feature runs; deferred chunks load lazily. The eager guardrail is 5.72 MiB. Each output file must also stay below 10 MB, where CloudFront stops compressing it. The module boundary is enforced separately by check-toolbar-graph.

Metric Size Δ vs base Budget
Eager (shipped)
entry + static imports
2.18 MiB · 17 files no change ████░░░░░░ 38.1% of 5.72 MiB
Deferred (lazy) 2.07 MiB · 33 files no change n/a — loads on demand
Loader dist/toolbar.js 1.1 KiB no change █░░░░░░░░░ 5.8% of 19.5 KiB
Largest eagerly-shipped chunks
Size File
713.8 KiB dist/toolbar/toolbar-app-HY7HJI4V.css
543.2 KiB dist/toolbar/chunk-chunk-Q7RHKXX5.js
484.2 KiB dist/toolbar/chunk-chunk-L423YPH4.js
133.7 KiB dist/toolbar/chunk-chunk-5MHL6INX.js
131.8 KiB dist/toolbar/chunk-chunk-T5KY5WYR.js
71.0 KiB dist/toolbar/toolbar-app-XBDTUN5X.js
69.0 KiB dist/toolbar/chunk-chunk-27JL52RE.js
35.6 KiB dist/toolbar/chunk-chunk-EGOJWYOP.js
20.9 KiB dist/toolbar/chunk-chunk-Y4N3PHT4.js
12.2 KiB dist/toolbar/chunk-chunk-PIK3PADE.js

Posted automatically by check-toolbar-size · sizes are toolbar output bytes (shipped, post-tree-shake) from the esbuild metafile

Dist folder size — no change

Total size of the built frontend/dist folder (all assets), compared against the base branch.

Total: 1357.43 MiB · no change

ℹ️ MCP UI apps size — 32 app(s), 17064.9 KB JS

Built size of each MCP UI app (main.js + styles.css).

App JS CSS
debug 599.5 KB 187.7 KB
action 457.8 KB 187.7 KB
action-list 564.3 KB 187.7 KB
cohort 456.8 KB 187.7 KB
cohort-list 563.3 KB 187.7 KB
email-template 456.6 KB 187.7 KB
error-details 472.4 KB 187.7 KB
error-issue 457.5 KB 187.7 KB
error-issue-list 564.2 KB 187.7 KB
experiment 561.5 KB 187.7 KB
experiment-list 565.1 KB 187.7 KB
experiment-results 563.2 KB 187.7 KB
feature-flag 567.1 KB 187.7 KB
feature-flag-list 570.9 KB 187.7 KB
feature-flag-testing 461.0 KB 187.7 KB
insight-actors 562.1 KB 187.7 KB
invite-email-preview 456.0 KB 187.7 KB
llm-costs 559.5 KB 187.7 KB
session-recording 458.6 KB 187.7 KB
session-summary 463.9 KB 187.7 KB
survey 458.4 KB 187.7 KB
survey-global-stats 562.2 KB 187.7 KB
survey-list 565.0 KB 187.7 KB
survey-stats 562.2 KB 187.7 KB
trace-span 457.2 KB 187.7 KB
trace-span-list 564.2 KB 187.7 KB
workflow 457.1 KB 187.7 KB
workflow-list 563.7 KB 187.7 KB
loops-review 460.6 KB 187.7 KB
query-results 745.5 KB 187.7 KB
render-ui 826.2 KB 187.7 KB
visual-review-snapshots 461.6 KB 187.7 KB

@charlesvien
charlesvien marked this pull request as ready for review July 22, 2026 16:11
@charlesvien charlesvien added the stamphog Request AI approval (no full review) label Jul 22, 2026
Comment thread services/mcp/src/ui-apps/apps/loops-review.tsx Outdated
@veria-ai

veria-ai Bot commented Jul 22, 2026

Copy link
Copy Markdown
Contributor

PR overview

All previously flagged issues have been addressed. No open security concerns remain on this pull request.

Security review

No open security issues remain on this pull request.

Fixed/addressed: 1 · PR risk: 0/10

@greptile-apps

greptile-apps Bot commented Jul 22, 2026

Copy link
Copy Markdown
Contributor

Reviews (1): Last reviewed commit: "carry prepare confirmation hash on resul..." | Re-trigger Greptile

@stamphog

stamphog Bot commented Jul 22, 2026

Copy link
Copy Markdown

@stamphog stamphog Bot removed the stamphog Request AI approval (no full review) label Jul 22, 2026
@charlesvien charlesvien added the stamphog Request AI approval (no full review) label Jul 22, 2026
stamphog[bot]
stamphog Bot previously approved these changes Jul 22, 2026

@stamphog stamphog Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixes a broken MCP confirmation flow and closes the security gap the reviewer flagged (hidden capability fields could be persisted unreviewed) by both allowlisting the prepare payload and rendering the previously-hidden fields; the diff matches the description, has new tests, and the author (STRONG familiarity, on the owning team) plus the agent reviewer's follow-up confirm the fix.

  • Author wrote 100% of the modified lines and has 1 merged PRs in these paths (familiarity STRONG).
  • 👍 on the PR from greptile-apps[bot], hex-security-app[bot].
Gate mechanics and policy version
Gate Result
prerequisites all clear
deny-list no deny categories matched
size 102L, 4F substantive, 144L/5F incl. docs/generated/snapshots — within ceiling
tier T1-agent / T1c-medium (144L, 5F, two-areas, fix)
stamphog 2.0.0b3 .stamphog/policy.yml @ b02be94 · reviewed head 79b66c7

@charlesvien
charlesvien enabled auto-merge (squash) July 22, 2026 16:25
@charlesvien
charlesvien force-pushed the fix/loops-review-confirmation-hash branch from 79b66c7 to 307ac99 Compare July 22, 2026 16:56
Comment thread services/mcp/src/ui-apps/apps/loops-review.tsx Outdated
@stamphog
stamphog Bot dismissed their stale review July 22, 2026 17:04

New commits pushed (delta classified non_trivial_delta) — stamphog approval dismissed; re-review running automatically.

stamphog[bot]
stamphog Bot previously approved these changes Jul 22, 2026

@stamphog stamphog Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Both bot-flagged security concerns (unreviewed fields forwarded to the confirmed-action prepare call, and inconsistent auto-fix display logic) are directly fixed in this diff via an explicit field allowlist plus matching UI rows and a regression test; author is on the owning team with STRONG familiarity on these exact lines.

  • Author wrote 100% of the modified lines and has 1 merged PRs in these paths (familiarity STRONG).
  • 👍 on the PR from greptile-apps[bot], hex-security-app[bot].
Gate mechanics and policy version
Gate Result
prerequisites all clear
deny-list no deny categories matched
size 114L, 4F substantive, 176L/6F incl. docs/generated/snapshots — within ceiling
tier T1-agent / T1c-medium (176L, 6F, two-areas, fix)
stamphog 2.0.0b3 .stamphog/policy.yml @ 73ecfc9 · reviewed head e9041dd

@posthog

posthog Bot commented Jul 22, 2026

Copy link
Copy Markdown
Contributor

👋 Visual changes detected for this PR.

Review and approve in PostHog Visual Review

If these changes are unexpected, they may be caused by a flaky test or a broken snapshot on master. Don't approve — rerun the job or wait for a fix.

@charlesvien
charlesvien disabled auto-merge July 22, 2026 17:17
@charlesvien
charlesvien enabled auto-merge (squash) July 22, 2026 17:26
@stamphog
stamphog Bot dismissed their stale review July 22, 2026 17:26

New commits pushed (delta classified non_trivial_delta) — stamphog approval dismissed; re-review running automatically.

@stamphog stamphog Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Touches a security-sensitive confirmed-action MCP flow, but both agent security reviewers' concerns (hidden fields bypassing confirmation, auto-fix mislabeling) are directly and verifiably fixed in this diff — explicit field allowlist plus matching UI rows, and a fix_review_comments/watch_ci split matching the backend's OR guard — with new tests, and veria-ai's follow-up comment confirms no open concerns remain. Author is on the owning team with STRONG familiarity, which supplies the required independent assurance for this risky territory.

  • Author wrote 100% of the modified lines and has 1 merged PRs in these paths (familiarity STRONG).
  • 👍 on the PR from greptile-apps[bot], hex-security-app[bot].
Gate mechanics and policy version
Gate Result
prerequisites all clear
deny-list no deny categories matched
size 141L, 6F substantive, 303L/9F incl. docs/generated/snapshots — within ceiling
tier T1-agent / T1d-complex (303L, 9F, two-areas, fix)
stamphog 2.0.0b3 .stamphog/policy.yml @ ff0655c · reviewed head 3d2474e

@charlesvien
charlesvien merged commit 8ccafb6 into master Jul 22, 2026
210 checks passed
@charlesvien
charlesvien deleted the fix/loops-review-confirmation-hash branch July 22, 2026 17:42
@deployment-status-posthog

deployment-status-posthog Bot commented Jul 22, 2026

Copy link
Copy Markdown

Deploy status

Environment Status Deployed At Workflow
dev ✅ Deployed 2026-07-22 19:44 UTC Run
prod-us ✅ Deployed 2026-07-22 19:59 UTC Run
prod-eu ✅ Deployed 2026-07-22 19:59 UTC Run

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

stamphog Request AI approval (no full review)

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant