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 7 additional findings in Devin Review.

[devin-ai-integration]

🟡 Automatically approved requests that get cancelled are logged as if a person decided them

The record for a request that could not be carried out is stamped with the rule owner's name but not with the fact that it was handled automatically (record.resolvedBy = resolvedBy at packages/workshop-backend/src/overseer.ts:2680), so the activity log presents an automatic outcome as that person's own decision.

Impact: In the audit trail and the history panel, a request that a background rule attempted and that was then cancelled reads as "By ", hiding that no human acted.

Why the automatic marker is lost on the new invalidation path

applyPendingAction is documented as the single chokepoint that "guarantees the audit log always records the resolving user and whether it was applied automatically" (packages/workshop-backend/src/overseer.ts:2660-2663). The new pre-dispatch failure branch (packages/workshop-backend/src/overseer.ts:2675-2684) sets state, appliedAt, resolvedBy and invalidationReason, but never autoApproved, even though the drain path calls it with autoApproved = true (packages/workshop-backend/src/auto-approval.ts:119).

The UI derives its badge from that field: const autoApproved = record.type === 'action' && record.autoApproved === true in packages/workshop-frontend/src/Activity.tsx:605, rendering By ${resolvedBy.name} instead of Auto-approved (${resolvedBy.name}'s rule) (packages/workshop-frontend/src/Activity.tsx:620). So an invalidated auto-approval is indistinguishable from a manual approval attributed to the rule enabler.

Note the existing doc comment on ActionLogEntry.autoApproved ("Only ever set alongside state "approved" (there is no automatic rejection)") is also now inaccurate: this PR introduces an automatic non-approval outcome.

---

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