Revalidate MCP actions before dispatch

[dcartertwo]

An MCP action may wait for approval while its connection, grant, or effective policy changes. This captures the approved connection generation and policy, then rechecks them within the bounded discovery path immediately before tools/call. A stale action is never dispatched; it is recorded using the existing rejected wire state plus an invalidation reason, and auto-approval rules are cleared so current and rolled-back backends stop the current drain.

Invalidation uses a thrown, message-preserved sentinel so older Workshops fail closed. Re-enabling a cleared rule explicitly allows future auto-approval rather than leaving a permanent historical barrier.

Stacked on #199. Verified with 316 MCP tests, 289 backend tests plus integration tests, 152 frontend tests, both MCP connector suites, and full pnpm lint.

---

[ask-bonk]

The stageAction/stage in other gatekeepers (notion, confluence, github) are unrelated — they're separate implementations with their own signatures. The MCP stage method's only caller is the facet, and McpSessionBase.callTool is the only caller of host.stageAction, both updated. Good.

I've completed my review. The change is well-structured and thoroughly tested. Let me summarize my findings.

Review Summary

This PR revalidates approved MCP actions immediately before dispatch, so a stale action (connection/grant/policy changed while awaiting approval) is never sent. The design is sound and consistent with the kernel's capability model, and the test coverage is substantial (action-store, facet, session, auto-approval, Activity).

I reviewed the full diff (HEAD~1..HEAD) and traced the key flows: the dispatched gating in ActionStore.apply(), the revalidation callback in McpFacetBase.applyAction(), the read-path generation check in McpSessionBase.callTool(), the message-preserved sentinel across the RPC boundary (createActionInvalidatedError / getActionInvalidationReason), the auto-approval barrier, and the rolling-deploy compat paths.

Findings — no blocking issues. A few observations, none actionable enough to block:

• Extra remote round-trip per tool call (by design). resolveToolForCall always calls #freshTool (a remote findTool/listTools probe), whereas the old findTool could serve a read tool from the described catalog without any network call. Reads now also do a second call for dispatch after resolveToolForCall already did one. This is the correctness/latency tradeoff the PR intends, but it's a meaningful behavioral change worth being aware of for latency-sensitive read tools.

• legacyResult: unknown inspection in overseer.ts. Reading a return-object form from a method typed Promise<void> is a deliberate rolling-deploy compat measure; it uses : unknown (not the discouraged as unknown as fake-interface pattern), so it's within AGENTS.md guidance.

• Migration safety confirmed. The constructor's dispatched IS NOT 0 / dispatched = 0 split correctly treats legacy applying rows (NULL dispatched) as outcome-unknown/non-retryable, and the ADD COLUMN guards are idempotent.

• ActionInvalidatedError recovery path is consistent: invalidated actions persist as failed/retryable=false/dispatched=false, and re-apply() re-throws the sentinel with the stored reason rather than a generic error.

LGTM!

github run

[ask-bonk]

The interface declares applyAction(action: number): Promise<void>. The new MCP facet throws (doesn't return an object) for invalidation. The overseer reads a legacyResult object form for rolling-deploy compat with the earlier return-object form — but the interface signature is still Promise<void> and the current implementation throws. This is consistent with the doc comment update. The legacyResult: unknown reading is a compat shim.

I've thoroughly traced all the key flows. The existing bonk review already exists with a detailed analysis and LGTM verdict. My independent review confirms the same conclusions:

• The dispatched gating and crash-recovery split (IS NOT 0 / = 0) are correct, with legacy NULL rows treated conservatively as outcome-unknown.
• The ActionInvalidatedError sentinel survives the RPC boundary via message prefix, and getActionInvalidationReason correctly extracts it (backend records the reason; older Workshops fail closed).
• The auto-approval barrier logic correctly keeps invalidated actions as durable manual gates and clears rules for rolled-back-backend safety.
• The read-path generation recheck prevents dispatching a read against a changed connection.
• Frontend optimistic-state removal is coherent (server now drives the rejected/invalidated state), and the Activity/ChatInterface invalidation rendering is consistent.
• Test coverage is substantial and matches the intended behaviors, including migration idempotency, barrier ordering, and cross-RPC serialization.

I found no discrete, actionable problems introduced by this change beyond the by-design observations the existing review already documented (extra remote round-trip per tool call; the compat legacyResult: unknown read). I have no additional inline findings to submit.

LGTM!

github run

[ask-bonk]

The read path throws McpCallNotDispatchedError which propagates to the agent as an error it can retry. Since a read is idempotent and never dispatched here, that's safe. The window between resolveToolForCall and the dispatch call is small, but a generation change is caught.

I've completed a thorough independent review. My review confirms the design is sound and I found no discrete, actionable problems introduced by this change beyond the by-design observations the two existing bonk reviews already documented (extra remote round-trip per tool call; the defensive errorCode-fallback path in getActionInvalidationReason). I traced:

• The dispatched gating and crash-recovery migration split (IS NOT 0 / = 0) — correct and conservative for legacy NULL rows.
• ActionInvalidatedError propagation through withClient (not caught, correctly recorded as failed/non-retryable/undispatched).
• The message-prefixed sentinel surviving the Workers RPC boundary via getActionInvalidationReason.
• The auto-approval barrier: invalidation returns "invalidated", clears rules for rolled-back-backend safety, and stops the drain in-order.
• The read-path generation recheck using a non-dispatched sentinel.
• Frontend optimistic-state removal is coherent now that the server drives the rejected/invalidated state; the awaited-turn resume correctly treats invalidation like denial.
• All callers of the changed signatures are updated; findTool remains for the describe path.

No inline findings to submit.

LGTM!

github run

[ask-bonk]

The frontend tests confirm the invalidation reason renders in history and that changing latestInvalidationId triggers an auto-approval panel refresh (via the reloadTrigger key change → useAutoApproval refresh). This closes the loop: when an action is invalidated server-side, the UI reloads the auto-approval rules to reflect that they were cleared.

I've completed a thorough independent review. Let me summarize.

Review Summary

This PR revalidates an approved MCP action immediately before dispatch, so a stale action (whose connection generation, tool scope, or effective policy changed while awaiting approval) is never sent. I reviewed the full diff (HEAD~1..HEAD, 18 files) independently and traced every key flow.

Verified correct:

• dispatched gating + crash-recovery migration (action-store.ts): the dispatched IS NOT 0 / dispatched = 0 split correctly treats legacy NULL-dispatched applying rows as outcome-unknown (non-retryable) while a proven pre-dispatch interruption stays retryable. The ADD COLUMN guards are idempotent, and stored.dispatched = true is persisted inside the call callback before callTool, so the flag genuinely proves whether the wire call was reached.
• Revalidation path (facet.ts applyAction): rechecks generation, scope, and toolPolicyFingerprint under runDiscovery/call before invoking fn(client). No re-entrant runDiscovery deadlock (the nested this.call doesn't re-enter). The #resolvingCalls dedup shares one snapshot per tool name, which is safe.
• Sentinel across RPC (gatekeeper.ts): createActionInvalidatedError message prefix + getActionInvalidationReason survive Workers RPC serialization; older Workshops fail closed by leaving the action pending. Re-apply() of an invalidated record re-throws the sentinel with the stored reason.
• Auto-approval barrier (auto-approval.ts): invalidation returns "invalidated", stops the drain in-order, and clearAutoApprovalRules removes all rules so a rolled-back backend also halts at a manual gate. The lazy-list snapshot before delete is protected by a dedicated test.
• Generation semantics (account.ts): generation advances only on reconnect, not on token refresh (#refresh), so routine refreshes don't spuriously invalidate.
• approveAction early return (overseer.ts:7779): skipping the resume/drain on invalidation matches denial semantics exactly (rejectAction also leaves the turn ended; a rejected awaited action blocks resume at line 7895).
• Frontend: optimistic-state removal is coherent now that the server drives the rejected/invalidated state; latestInvalidationId correctly forces an auto-approval-rule refresh.

By-design observations (not blocking):

• resolveToolForCall always does a fresh remote findTool/listTools probe, and a read tool then does a second remote call for dispatch — an extra round trip per tool call versus the old catalog-served read path. This is the intended correctness/latency tradeoff.

I found no discrete, actionable problems introduced by this change. The earlier bonk comments referencing a legacyResult: unknown compat read describe a prior revision of the branch; the current HEAD uses the cleaner thrown-sentinel approach and no legacyResult remains. No inline findings to submit.

LGTM!

github run

[ask-bonk]

Test coverage is thorough, including the read-path generation-change guard. My independent review confirms the design is sound and the implementation is correct.

I traced all the key flows independently:

• dispatched gating + crash-recovery migration (action-store.ts): the dispatched IS NOT 0 / dispatched = 0 split correctly treats legacy NULL-dispatched applying rows conservatively as outcome-unknown (non-retryable) while a proven pre-dispatch interruption stays retryable. ADD COLUMN guards are idempotent, and stored.dispatched = true is persisted inside the call callback immediately before callTool, so the flag genuinely proves the wire call was reached. On invalidation, dispatched stays false, so re-apply() correctly re-throws the sentinel (action-store.ts:192-197).

• Revalidation path (facet.ts:326-352): rechecks generation, scope, and toolPolicyFingerprint inside runDiscovery/call before invoking fn(client); ActionInvalidatedError is thrown before dispatched is set. The #resolvingCalls dedup shares one snapshot per tool name, and the read path's independent generation recheck at dispatch (session.ts:222-228) guards the shared-snapshot window.

• Sentinel across RPC (gatekeeper.ts): the GATEKEEPER_ACTION_INVALIDATED: message prefix survives Workers RPC serialization; getActionInvalidationReason extracts it, and older Workshops fail closed by leaving the action pending.

• Auto-approval barrier (auto-approval.ts): invalidation returns "invalidated", stops the drain in-order, and clearAutoApprovalRules materializes its list snapshot before deleting, so a rolled-back backend halts at a manual gate.

• Overseer (overseer.ts:2585-2612): invalidation records the existing rejected wire state plus invalidationReason, and the approveAction early return matches denial semantics.

• Frontend: optimistic-state removal is coherent now that the server drives the rejected/invalidated state; invalidationCount correctly forces an auto-approval-rule refresh via the composite reloadTrigger key.

All callers of the changed signatures (stage/stageAction/call/findTool→resolveToolForCall) are updated consistently. The four prior bonk reviews already documented the by-design observations (extra remote round-trip per tool call; compat sentinel read), and I found no additional discrete, actionable problems introduced by this change. No inline findings to submit.

LGTM!

github run

[github-actions]

Preview: pr198-feature-mcp-p-cc7c572a

https://pr198-feature-mcp-p-cc7c572a-router.cloudflare-os-previews.workers.dev

Dashboard · deleted when this PR closes

[devin-ai-integration]

Devin Review found 1 new potential issue.

View 5 additional findings in Devin Review.

[devin-ai-integration]

🟡 Approving a request queued before this release fails it and silently deletes every automatic-approval rule for that connection

Any request that was queued for approval before this release lacks the newly captured approval snapshot and is therefore always refused at dispatch (ActionInvalidatedError at packages/mcp-shared/src/facet.ts:336-340), which in the Workshop also erases all of that connection's automatic-approval rules.
Impact: After deploying, a user who approves an already-waiting request sees it fail with a misleading "connection changed" message and loses every auto-approval setting they had configured for that connection.

Why a pre-upgrade action always invalidates, and what invalidation costs

ActionStore.stage only started persisting policy_fingerprint/connection_generation in this PR, and StoredAction documents both as "absent on actions staged by older code" (packages/mcp-shared/src/session.ts:50-53). Rows inserted before the deploy have both columns NULL, so applyAction takes the first branch (stored.connectionGeneration === undefined || stored.policyFingerprint === undefined) and throws the "This MCP connection changed after approval was requested" error even though nothing changed.

On the Workshop side applyPendingAction treats every invalidation as a barrier: it calls clearAutoApprovalRules(this.storage, record.gatekeeperId) (packages/workshop-backend/src/overseer.ts:2675-2686), deleting every autoApproveTags row for that gatekeeper. The rules are not restored automatically; the user must re-enable each one. The same collateral applies to any transient invalidation (for example an aggregator that momentarily omits the tool from tools/list).

A cheaper fail-closed option for the legacy case would be to keep the action retryable, or at least to use a reason that does not claim the connection changed and does not trip the rule-clearing barrier.

Prompt for agents

Actions staged before this PR have NULL policy_fingerprint/connection_generation, so McpFacetBase.applyAction (packages/mcp-shared/src/facet.ts) always throws ActionInvalidatedError for them with the message "This MCP connection changed after approval was requested." In the Workshop, OverseerImpl.applyPendingAction (packages/workshop-backend/src/overseer.ts) treats any invalidation as a barrier and calls clearAutoApprovalRules(), wiping every auto-approval rule the user configured for that connection. The result is that a routine deploy turns each already-queued approval into a failure that also destroys unrelated user configuration. Consider distinguishing the legacy-snapshot case: either resolve the current policy and treat a matching tool as still valid, or fail the action with a distinct message that does not trigger the auto-approval barrier (and does not misattribute the cause to a connection change). Also consider whether clearing every rule is the right response to a single invalidation, versus only stopping the current drain.

---

Was this helpful? React with 👍 or 👎 to provide feedback.

[devin-ai-integration]

Devin Review found 1 new potential issue.

View 5 additional findings in Devin Review.

[devin-ai-integration]

🟡 Kernel and UI changes are shipped together in a single unsplit commit

The change bundles kernel edits (packages/workshop-backend/src/overseer.ts, packages/workshop-backend/src/auto-approval.ts, packages/workshop-shared/src/api.ts, packages/workshop-shared/src/gatekeeper.ts) with Workshop UI edits (packages/workshop-frontend/src/Activity.tsx, ChatInterface.tsx, useResolveAction.ts) in one commit, which REVIEW.md forbids: a kernel change must at minimum be grouped into commits that let workshop-backend/workshop-shared be reviewed apart from UI.
Impact: Reviewers cannot read the higher-bar kernel diff separately from the UI diff, which is exactly what the repository's review rules require.

Where the rule applies

REVIEW.md, "High-scrutiny areas": "A large kernel change should be split by concern into separate PRs, and at minimum grouped into commits that let workshop-backend/workshop-shared be reviewed apart from UI. Flag PRs that bundle both." git log shows a single commit (Revalidate MCP actions before dispatch) touching packages/workshop-backend/src/overseer.ts:2664-2693, packages/workshop-backend/src/auto-approval.ts:27-36, packages/workshop-shared/src/gatekeeper.ts:21-48 and the three frontend files together.

Prompt for agents

REVIEW.md requires that kernel changes (packages/workshop-backend and packages/workshop-shared) be reviewable apart from UI changes. This PR lands overseer.ts, auto-approval.ts, api.ts and gatekeeper.ts together with workshop-frontend's Activity.tsx, ChatInterface.tsx and useResolveAction.ts in a single commit. Split the history so the kernel/protocol change (invalidation error contract, ActionRecord.invalidationReason, auto-approval drain semantics) is one commit and the frontend rendering of the new state is another, or move the UI portion to a follow-up PR.

---

Was this helpful? React with 👍 or 👎 to provide feedback.