feat(extensibility): migrate n8n hook execution to per-workflow MCP tool invocation

[rishikunnath2747]

Disclaimer: Do not include SAP-internal or customer-specific information in this PR (e.g. internal system URLs, customer names, tenant IDs, or confidential configurations). This is a public repository.

Description

Migrates n8n hook execution in the extensibility module from a workflow-ID-based polling model (using instance mcp) to a per-workflow MCP tool invocation model.

What changed:

• N8nWorkflowConfig dataclass: workflow_id field removed; replaced with ord_id, card_ord_id, tool_name, and global_tenant_id
• UMS GraphQL fragment updated to fetch the four new fields instead of workflowId
• _build_hook() now skips hooks missing either toolName or cardOrdId (previously guarded on workflowId)
• _discover_n8n_tools() now filters discovered MCP tools by both tool_name and card_ord_id (each hook is its own MCP server)
• _execute_workflow_via_agw() rewritten: single call_mcp_tool() call; response is expected to be an A2A Message returned directly (no polling, no execution-tree traversal)
• _poll_hook_execution() deleted — polling is no longer part of the AGW execution path
• call_hook() (legacy direct-HTTP path) is unchanged
• local_extensibility_example.json and user-guide.md updated to reflect the new schema
• emit_extensions_summary_span now includes joule_studio_gsid sourced from ext_impl.joule_studio_gsid, propagating the Joule Studio global solution ID to the aggregate summary span

How to Test

1. Deploy an extension with pre/post hook using CAD
2. Call the agent, it should incorporate the pre/post hook response

Type of Change

• New feature (non-breaking change that adds functionality)

Checklist

• I have read the Contributing Guidelines
• I have verified that my changes solve the issue
• I have added/updated automated tests to cover my changes
• All tests pass locally
• I have verified that my code follows the Code Guidelines
• I have updated documentation (if applicable)
• I have added type hints for all public APIs
• My code does not contain sensitive information (credentials, tokens, etc.)
• I have followed Conventional Commits for commit messages

[rishikunnath2747]

/review-pr

[rishikunnath2747]

PR Review: #324: feat(extensibility): migrate n8n hook execution to per-workflow MCP tool invocation

Author: rishikunnath2747 Branch: n8n_change → main
Verdict: ❌ Blocked
Summary: The PR delivers a clean architectural migration from workflow-ID polling to per-hook MCP tool invocation, but ships two blockers: a breaking dataclass field removal that is not declared as such (and leaves call_hook() broken at runtime, not just deprecated), and a modified user-guide.md that is missing the required ## Multi-tenancy section.

---

A: Process & Compliance

#	Criterion	Status	Finding
A1	PR template complete	⚠️	"Related Issue" section is missing from the body; all 9 checklist items ticked, How to Test steps present
A2	Conventional Commits	✅	feat(extensibility): … matches the required format
A3	Issue linked	⚠️	No Closes #N / Fixes #N / Resolves #N in the PR body
A4	AI-generated code disclosure	➖	Iterative commit history (reverts, small fixes) is consistent with human-authored code

B: Security & Sensitive Data

#	Criterion	Status	Finding
B1	No sensitive data in code	✅	Example values in local_extensibility_example.json are clearly synthetic ("tenant-example-001")
B2	No sensitive data in PR body	✅	No internal URLs, tenant IDs, or customer names

C: Code Quality

#	Criterion	Status	Finding
C1	CI checks passing	✅	All required jobs pass (Code Quality, Tests & Coverage, Build & Package, Commit Validation, Check Version Bump, Analyze)
C2	Version bump	➖	No prohibited version bump detected; Check Version Bump CI job passed
C3	Type hints	⚠️	call_hook_agw and _execute_workflow_via_agw both declare message: Optional[Any] — should be Optional[Message]. Both also use bare Optional[dict] without type params — should be Optional[dict[str, Any]] (client.py:437, 482, 488)
C4	No hardcoded values	✅	All magic strings are module-level constants
C5	Import organization	✅	All imports at module level; TYPE_CHECKING block used correctly
C6	Naming conventions	✅	No new enums or naming violations introduced in this PR
C7	No unused code	⚠️	_EXECUTE_WORKFLOW_TOOL_NAME and _GET_EXECUTION_TOOL_NAME are imported in tests/extensibility/unit/test_client.py:11-12 but never used in the test file (the call_hook tests that used them were deleted). Ruff skips tests/ per the pyproject.toml config, so this was not caught by CI.
C8	No unjustified new dependencies	✅	Only change is narrowing the a2a-sdk upper bound (<2 → <1.0) — no new deps
C9	Proto code freshness	➖	No proto files changed

D: API & Design

#	Criterion	Status	Finding
D1	API future-proofing	✅	New fields use the existing N8nWorkflowConfig dataclass pattern; no loose scalar params added to public API
D2	Public API hygiene	✅	Internal helpers prefixed _; __all__ not widened unnecessarily
D3	Breaking changes marked	❌	N8nWorkflowConfig.workflow_id was a public dataclass field and has been removed. This is a breaking change: any caller that constructs N8nWorkflowConfig(workflow_id=…) or reads hook.n8n_workflow_config.workflow_id will get AttributeError at runtime. The PR is typed as "New feature (non-breaking)" with no Breaking Changes checkbox or migration guide. Compounding this, call_hook() still accesses the removed field via # ty: ignore[unresolved-attribute] at client.py:289,340, meaning callers receive a DeprecationWarning followed immediately by an AttributeError — the method is broken, not just deprecated. The docstring at client.py:164 and user-guide.md:164 also still reference workflow_id as if it exists.
D4	Pagination & tenant filtering	➖	Not applicable — this PR does not add new list/query operations
D5	Telemetry instrumentation	⚠️	call_hook_agw() is decorated with @record_metrics(Module.EXTENSIBILITY, Operation.EXTENSIBILITY_CALL_HOOK) — the same operation as the deprecated call_hook() (client.py:477-480). The two code paths are architecturally distinct (direct HTTP vs. AGW MCP); sharing the same operation name makes telemetry dashboards unable to distinguish them. Consider adding Operation.EXTENSIBILITY_CALL_HOOK_AGW to core/telemetry/operation.py.
D6	Multi-tenancy support	⚠️	call_hook_agw() accepts tenant_subdomain and user_token, and get_extension_capability_implementation() accepts tenant; tenant routing is present. No explicit decision documented in the user guide (see E2a).

E: Tests & Documentation

#	Criterion	Status	Finding
E1	Tests added/updated	✅	7 new TestCallHookAgw test cases cover tool-not-found, wrong-server isolation, success path, duplicate-tool disambiguation, AGW network error, malformed A2A response, and message forwarding. Model tests updated for the new N8nWorkflowConfig fields.
E2	Documentation quality	⚠️	user-guide.md:164 still reads "The workflow ID is taken from hook.n8n_workflow_config.workflow_id (legacy direct-HTTP path)." — that field no longer exists. The call_hook() API reference in the guide is inconsistent with the current code.
E2a	Multi-tenancy documentation	❌	user-guide.md was modified by this PR but contains no ## Multi-tenancy section with the required four fields (Supported / Authentication / How to use / Further reading). The extensibility module does handle tenant routing (tenant param, tenant_subdomain param), so this section is load-bearing, not boilerplate.
E3	Module structure compliance	➖	No new module — existing module modified

---

❌ Blocking Issues

• [D3] N8nWorkflowConfig.workflow_id removed without marking the PR as a breaking change. Add - [x] Breaking change to the Type of Change section and include a migration note (e.g., "Replace workflow_id with ord_id, card_ord_id, tool_name, global_tenant_id"). Additionally, fix call_hook() so it fails gracefully: replace the # ty: ignore references at client.py:289,340 with a raise NotImplementedError("call_hook() is deprecated and non-functional; use call_hook_agw() instead") early in the method body, and update the docstring and user-guide.md:164 to remove the workflow_id reference.

• [E2a] user-guide.md was modified by this PR but is missing the required ## Multi-tenancy section. The section must include: Supported (Yes), Authentication (XSUAA/IAS via AGW — describe the tenant_subdomain and user_token mechanism), How to use (pass tenant_subdomain to call_hook_agw() and tenant to get_extension_capability_implementation()), Further reading (link or —).

⚠️ Non-Blocking Suggestions

• [A3] Add Closes #<issue> or Fixes #<issue> to link this PR to its tracking issue.

• [C3] Tighten the type annotations: message: Optional[Any] → Optional[Message] in call_hook_agw (client.py:482) and _execute_workflow_via_agw (client.py:437); headers: Optional[dict] → Optional[dict[str, Any]] in both signatures (client.py:438, 488).

• [C7] Remove the unused imports _EXECUTE_WORKFLOW_TOOL_NAME and _GET_EXECUTION_TOOL_NAME from tests/extensibility/unit/test_client.py:11-12 — the call_hook tests that referenced them were deleted in this PR.

• [D5] Add EXTENSIBILITY_CALL_HOOK_AGW to core/telemetry/operation.py and update call_hook_agw()'s @record_metrics decorator to use it. This lets telemetry distinguish AGW-based hook calls from the legacy direct-HTTP path.

• [E2] Update user-guide.md:164 to remove the reference to hook.n8n_workflow_config.workflow_id; that field was deleted in this PR. The line should either be removed or updated to describe the new tool_name/card_ord_id lookup.

✅ Things Done Well

• _discover_n8n_tools() filters by both tool_name and card_ord_id, correctly isolating the hook's MCP server from other servers that might expose an identically-named tool.
• _build_hook() in _ums_transport.py guards against missing toolName or cardOrdId before constructing the hook, preventing silent half-initialized objects.
• Strong new test coverage for call_hook_agw(): the duplicate-tool-disambiguation test (test_picks_correct_tool_among_duplicates) in particular guards a subtle correctness property.
• @record_metrics(..., deprecated=True) on call_hook() is a nice use of the decorator's built-in deprecation support.
• Narrowing the a2a-sdk upper bound to <1.0 is a correct defensive measure against a major-version API break.

[rishikunnath2747]

PR Review Update

Both blockers from the previous review are resolved:

• D3 — withdrawn. call_hook() has been non-functional for ~2 months, so removing workflow_id does not introduce new breakage. Marking the method as deprecated and providing call_hook_agw() as the replacement is the right approach.
• E2a — resolved. ## Multi-tenancy section added to user-guide.md.

Revised verdict: ⚠️ Needs Minor Work

Remaining non-blocking suggestions:

• [A3] Add Closes #<issue> / Fixes #<issue> to link to the tracking issue.
• [C3] call_hook_agw and _execute_workflow_via_agw: message: Optional[Any] → Optional[Message]; headers: Optional[dict] → Optional[dict[str, Any]] (client.py:437, 482, 488).
• [C7] Remove unused imports _EXECUTE_WORKFLOW_TOOL_NAME and _GET_EXECUTION_TOOL_NAME from tests/extensibility/unit/test_client.py:11-12 (ruff skips tests/ so these were not caught by CI).
• [D5] call_hook_agw() shares Operation.EXTENSIBILITY_CALL_HOOK with the deprecated call_hook(). Consider adding EXTENSIBILITY_CALL_HOOK_AGW to make telemetry distinguishable.
• [E2] user-guide.md:164 still references hook.n8n_workflow_config.workflow_id — that field was removed in this PR.