🔴 Blocking

1. db.commit() + db.close() on the caller's session — breaks the caller's transaction

In preview_tool_invocation, near the end of the method:
db.commit()
db.close()
return ToolPreviewResponse(...)

Unlike invoke_tool, which closes the session before making HTTP calls (documented reason: "release DB connection back to pool BEFORE making HTTP calls, prevents idle-in-transaction"), preview does no I/O after the DB close. Everything after _resolve_tool_for_invocation returns is pure in-memory work (schema validation, annotation parsing, plugin hook dispatch).

Closing the caller's session here is actively harmful:

• _resolve_tool_for_invocation already commits/closes the session internally (see line 5352–5353). Calling db.commit() + db.close() a second time on an already-closed session is a no-op in SQLAlchemy today, but it hides latent correctness risk.
• More concretely: when the future route handler (PR 4 in this series) wraps multiple calls with the same session — or any middleware that owns the db lifetime does work after this returns — closing the session here will break it. Per the design principle in AGENTS.md, get_db() in the route owns the session lifecycle; service methods should not call db.close().
• The pattern in invoke_tool at line 5352 has a clear rationale: close before potentially-blocking HTTP calls. Preview has no such calls. The db.commit() + db.close() should be removed from preview_tool_invocation.

⚠️ Functionally-impacting

2. Plugin hooks use invoke_hook_for_plugin — confirm this exists in the cpex public API

The live path everywhere (lines 4530, 5530, 6450, 6549, etc.) calls plugin_manager.invoke_hook(hook_type, ...) — the aggregate all-plugins invocation. The PR introduces plugin_manager.invoke_ hook_for_plugin(name=..., hook_type=..., ...) — a per-plugin variant that doesn't appear anywhere else in the codebase.

If invoke_hook_for_plugin is a real cpex method, that's fine. But its absence from every other call site is a yellow flag. If the method name or signature is wrong, the preview_safe-hooks branch will silently catch the resulting PluginError and fold it into a warning — making the failure invisible in tests (since all tests mock the plugin manager anyway). Worth confirming invoke_hook_for_plugin is in the cpex public API before merge.

3. _get_hook_refs doesn't filter DISABLED plugins — disabled plugins appear in pre_hooks_run

The live path's _check_for_retry_policy_config (lines 4668–4673) filters out DISABLED plugins and applies condition matching before processing hook refs. _get_hook_refs returns all refs raw, including disabled ones — so a preview_safe-tagged but disabled plugin would still appear in pre_hooks_run, reporting a run that didn't and shouldn't happen. The same PluginMode.DISABLED filter should be applied here.

💡 Suggestions

4. resolved_arguments mirrors input unchanged — worth a comment

resolved_arguments in the response is always == arguments (the input). The live path's pre-invoke hook can modify args before dispatch (lines 5548–5551). For now, preview_tool_invocation doesn't apply the hook's modified payload back to resolved_arguments. That's a reasonable v1 choice, but a comment stating "hook-modified args are not reflected here; resolved_arguments is the original input" would prevent confusion once preview_safe hooks get real users.

5. Minor: docstring cross-reference in ToolPreviewRequest

ToolPreviewRequest.arguments says "Not executed against the tool; see ToolPreviewResponse" but ToolPreviewResponse has no reciprocal pointer. Fine as-is, just noting it.

🔴 Confirmed blocking — plugin_context_id divergence (tool_service.py, preview_tool_invocation)

Verified directly against invoke_tool (mcpgateway/services/tool_service.py:5373-5382):

_binding_tool_name = tool_payload.get("name") or name
plugin_context_id = make_context_id(str(_tool_team_id), _binding_tool_name) if _tool_team_id else server_id

vs. preview's:

plugin_context_id = server_id or (str(_tool_team_id) if _tool_team_id else None)

Two independent divergences, not one:

1. Priority is inverted. invoke_tool prefers the team-scoped context (_tool_team_id wins over server_id); preview prefers server_id over team. So a team-scoped tool invoked through a virtual server gets a completely different resolution path in preview than live.
2. Format is wrong even when team_id wins. make_context_id produces "{team_id}:{tool_name}" (mcpgateway/plugins/gateway_plugin_manager.py:445-447); preview just uses the bare team_id string. Since _get_plugin_manager/get_plugin_manager key lookups off this exact string (base_service.py:284-293), team-scoped ToolPluginBindings never match in preview — plugin_manager resolves to a different manager than the live path (likely the wrong one or None), and all_refs is empty for essentially all team-scoped tools. The entire preview-hook feature silently no-ops for that case.

Fix is as the other review suggested: import make_context_id and copy the exact expression from invoke_tool verbatim, including the _binding_tool_name derivation (using tool_payload.get("name"), not just name, matters per the comment at tool_service.py:5376-5379 about ambiguous original_name).

Also agree a test with a team-scoped tool asserting the exact plugin_context_id passed to _get_plugin_manager is missing — every current test mocks _get_plugin_manager directly, so this regressed silently.

🟡 New finding — GlobalContext built for preview hooks is missing fields invoke_tool sets

invoke_tool's pre-invoke GlobalContext (tool_service.py:5463) is:

GlobalContext(request_id=request_id, server_id=context_server_id, tenant_id=payload_tenant_id, user=app_user_email, content_type=content_type)

where context_server_id = tool_gateway_id if ... else "unknown".

Preview's version (tool_service.py, new code) is:

GlobalContext(request_id=get_correlation_id() or uuid.uuid4().hex, tenant_id=_extract_tenant_id_from_payload(_tool_team_id), user=user_email)

server_id and content_type are simply omitted (default to None/unset). Any preview_safe -tagged plugin whose conditions or hook logic branches on context.server_id (a documented plugin filter dimension) will behave differently in preview than live — e.g. a plugin scoped to a specific gateway via conditions could be silently skipped or silently included incorrectly in preview when it wouldn't be live, or vice versa. Lower severity than the context_id bug (it only affects plugins that actually opt into preview_safe and use server-scoped conditions, and none ship today), but same root cause: this hook-invocation path was built by hand instead of factored out of invoke_tool, so it drifts. Worth at least setting server_id=context_server_id (computed the same way) for consistency, even if content_type genuinely doesn't apply to a preview (no request body content-type to speak of).

🔴 Blocking

1. Blocking plugin violations are silently swallowed — preview can report "clean" for a call that would actually be denied live

preview_tool_invocation (tool_service.py:7192-7199) calls plugin_manager.invoke_hook_for_plugin(..., violations_as_exceptions=False) and only handles the result via except PluginViolationError. But under violations_as_exceptions=False, cpex's execute_plugin (cpex/framework/manager.py:1214-1248) never raises on a blocking violation for SEQUENTIAL/CONCURRENT-mode plugins — it just returns PluginResult(continue_processing=False, violation=...) as a normal value. That return is discarded entirely here: the except never fires, the plugin name still gets appended to pre_hooks_run as if it ran cleanly, and no preview_hook_violation warning is ever added.

A preview_safe plugin that would actually block the call in production previews as clean — a false negative on the exact question this endpoint exists to answer. The existing unit test (test_hook_violation_folds_into_warning_not_raised) only passes because it mocks invoke_hook_for_plugin with side_effect=PluginViolationError(...), which doesn't reproduce real cpex behavior under violations_as_exceptions=False — so CI won't catch this.

Fix: check result.continue_processing / result.violation on the returned PluginResult, don't rely on an exception that can't be raised in this mode.

2. preview_safe hooks run regardless of their configured conditions

_get_hook_refs (tool_service.py:7061-7086) filters only PluginMode.DISABLED. The dispatch loop then calls invoke_hook_for_plugin → execute_plugin directly, which never evaluates conditions. Confirmed in cpex: invoke_hook_for_plugin (manager.py:1902-1922) never routes through _group_by_mode (manager.py:555-597), which is the only place payload_matches(ref.plugin_ref.conditions, ...) is checked before dispatch on the live path. This pattern is already implemented correctly nearby in this same file for TOOL_POST_INVOKE (tool_service.py:4747-4754) — the new preview code omits the conditions check its own sibling code has.

A preview_safe plugin scoped via conditions to a specific tool/tenant/server that would never fire for this tool in production still runs during preview — either adding a spurious violation, or reporting a clean run for a hook live traffic would never invoke.

⚠️ Functionally-impacting

3. Runtime-disabled plugins aren't excluded from preview

Same root cause as 2.: cpex's live dispatch (_group_by_mode, manager.py:589) also skips plugins the executor has auto-disabled at runtime after repeated errors (self._runtime_disabled). invoke_hook_for_plugin bypasses this check entirely, so a preview_safe plugin that's currently runtime-disabled (and thus skipped by real invoke_tool) still gets invoked — and can still warn/error — during preview.

4. Preview's hook payload omits headers, unlike every live call site

All 4 live construction sites of ToolPreInvokePayload (tool_service.py:4615, 5621, 6561, 6660) pass headers=pre_invoke_headers. Preview builds it with ToolPreInvokePayload(name=name, args=arguments) — no headers at all, and preview_tool_invocation's signature doesn't even accept a headers param. A preview_safe plugin that inspects or conditions on payload.headers will see empty/None regardless of what a real request would send, so preview can report "clean" for a hook that would behave differently — or be blocked — once real headers are present live.

5. hook_not_previewed warning overclaims what live invocation will do

Because skipped_refs (tool_service.py:7201-7208) is built from the same conditions-unfiltered all_refs as 2., every non-preview_safe hook gets "live invocation will run this hook but preview did not" — even when that hook's own conditions mean live invocation would also skip it. Direct corollary of 2.; fixing 2. should feed into this list too.

💡 Suggestions

6. _get_hook_refs duplicates existing logic in this file

tool_service.py:7061-7086 vs. 4747-4755: the private-registry reach-through plus DISABLED filter is duplicated (and, per 2., drifted — the original applies a conditions filter this one doesn't). Worth factoring into one shared helper so the two can't diverge again.

7. Plugin-context-id derivation is now copy-pasted a third time

tool_service.py:7174-7178 vs. 4407-4422 and 5375-5382. Now fixed to match invoke_tool's logic exactly, but the comment at 7175-7176 admitting it "must match exactly" the other copies is itself a sign this should be a shared helper — this is precisely the class of bug the prior commit in this PR had to patch once already.

8. _get_hook_refs reaches into private cpex internals as a workaround

tool_service.py:7061-7086 reaches through plugin_manager._registry (private, no public enumeration API) with a broad except Exception that degrades to "no hooks known" on any failure — acknowledged in the docstring. A future cpex refactor would silently make preview stop reporting real hooks with only a debug log line, no test/alert catching it. Longer-term this probably wants a real dry-run/preview primitive exposed by cpex's PluginManager itself rather than reimplementing (and under-implementing, per 2./3.) its dispatch-eligibility logic from outside.

Recommendation: Request Changes. 1-4 go directly against the feature's own contract (report what a live invocation would do, without running it) and aren't caught by the existing tests, which mock around cpex's real violations_as_exceptions=False / conditions / runtime-disabled semantics rather than reproducing them. 5-8 are worth raising but shouldn't block on their own.

The issues were addressed! CI is green!

LGTM 🚀