Skip to content

fix(server): centralize pre-validation hook composition#882

Closed
sangilish wants to merge 1 commit into
adcontextprotocol:mainfrom
sangilish:feat/pre-validation-hooks-859
Closed

fix(server): centralize pre-validation hook composition#882
sangilish wants to merge 1 commit into
adcontextprotocol:mainfrom
sangilish:feat/pre-validation-hooks-859

Conversation

@sangilish
Copy link
Copy Markdown
Contributor

@sangilish sangilish commented May 27, 2026

Summary

Refs #859.

This is a focused follow-up to the hook-composition work that already landed in #860. Current main already supports ordered pre-validation hook chains, so this PR avoids reimplementing that behavior and only tightens the remaining pieces from the #859 brief and #860 review notes:

  • Moves shared pre-validation hook types, composition, flattening, and application into adcp.server._hooks.
  • Keeps adcp.server.spec_compat backward-compatible by re-exporting the public hook types/helper.
  • Makes hook-chain failures surface the failing chain index and callable name in the INVALID_REQUEST message.
  • Replaces mock-only MCP forwarding coverage with a real MCPToolSet.call_tool(...) behavior test.
  • Adds real A2A executor coverage for ordered hook chains.

Review note

I opened this as a draft because #860 already shipped most of #859. The main question is whether this small cleanup/diagnostic follow-up is still useful, or whether #859 should be closed as already covered by #860.

Current CI note

The draft currently has red storyboard checks against the moving adcp-3.1 npm dist-tag. The failing PR run installed @adcp/sdk@8.1.0-beta.13; the latest passing main CI run I checked used @adcp/sdk@8.1.0-beta.12. I also reproduced the same classes of storyboard failure from a detached current-main worktree with ADCP_SDK_VERSION=adcp-3.1, including the reference seller 3.1 storyboard and proposal-finalize recovery expectation.

I am therefore keeping this as draft rather than marking it ready while that runner/schema drift is active.

Testing

  • uv run pytest tests/test_pre_validation_hooks.py tests/test_a2a_server.py -q
  • uv run pytest tests/test_pre_validation_hooks.py tests/test_a2a_server.py tests/test_spec_compat_hooks.py tests/test_spec_compat_hooks_deprecation.py tests/test_unknown_field_policy_server.py tests/test_schema_validation_server.py tests/test_serve_validation_passthrough.py tests/test_testing_decisioning.py tests/test_public_api.py -q
  • uv run ruff check src/adcp/server/_hooks.py src/adcp/server/spec_compat.py src/adcp/server/mcp_tools.py src/adcp/server/a2a_server.py src/adcp/server/serve.py src/adcp/server/__init__.py tests/test_pre_validation_hooks.py tests/test_a2a_server.py
  • uv run mypy src/adcp/server
  • uv run mypy src/adcp
  • uv run pytest -q
  • git diff --check

Full uv run ruff check still fails on unrelated existing test lint drift outside this PR's touched files, including long lines and unused imports in existing tests.

@bokelley
Copy link
Copy Markdown
Contributor

bokelley commented Jun 6, 2026

Decision: this is worth landing.

#860 already shipped the core of #859PreValidationHook / PreValidationHookChain / compose_pre_validation_hooks are live and threaded through MCP + A2A + serve(). So this PR is the diagnostics + test-quality follow-up, and that part earns its place:

  • named-chain error attribution (the failing chain index + callable surfaced in INVALID_REQUEST messages, instead of the generic "pre_validation_hook raised ..."),
  • the swap from mock/spy MCPToolSet tests to a real call_tool() behavior test, plus the A2A ordered-chain executor test and chain-failure-attribution tests,
  • consolidating the hook types/composition into adcp/server/_hooks.py as a single source of truth (spec_compat.py re-exports).

To land:

  1. Rebase onto current main (the PR is CONFLICTING/DIRTY).
  2. Re-run CI on the current @adcp/sdk adcp-3.1 dist-tag — the red storyboard checks looked like dist-tag drift (the failing run used @adcp/sdk@8.1.0-beta.13), reproducible on a clean main worktree; confirm they're unrelated to this diff.
  3. Since the symbols move from spec_compat.py to _hooks.py, confirm tests/test_import_layering.py and test_public_api.py stay green.

@sangilish — can you rebase? Happy to take it over and push the rebase if you'd prefer. Keeping #859 open as the tracker until this lands.

@bokelley
Copy link
Copy Markdown
Contributor

bokelley commented Jun 6, 2026

Rebased onto current main and pushed to claude/issue-859-pre-validation-hooks-rebase for review/fast-forward.

Conflict resolution summary:

  • src/adcp/server/serve.py: The PR's base had StreamableHTTPSessionManager imported at the top-level; current main replaced that with ADCPStreamableHTTPSessionManager from adcp.server.mcp_sessions. Resolution: dropped the stale StreamableHTTPSessionManager line, kept the from adcp.server._hooks import PreValidationHooks addition.
  • tests/test_a2a_server.py: Both main (test_execute_skips_scalar_datapart_and_uses_text_fallback) and this PR (test_pre_validation_hook_chain_runs_through_a2a_executor) inserted tests at the same point after test_execute_with_datapart. Resolution: both tests included in order.

Checks on the rebased branch (Python 3.11):

  • pytest tests/test_pre_validation_hooks.py tests/test_a2a_server.py tests/test_spec_compat_hooks.py tests/test_spec_compat_hooks_deprecation.py tests/test_import_layering.py tests/test_public_api.py128 passed
  • ruff check on touched files → all checks passed
  • mypy src/adcp/serverno issues found in 31 source files

The storyboard CI failures match the dist-tag drift @sangilish described and are reproducible from a clean main worktree — unrelated to this diff.

@bokelley — the rebased branch is ready. You can fast-forward feat/pre-validation-hooks-859 from it, or redirect the PR to merge from claude/issue-859-pre-validation-hooks-rebase directly.


Generated by Claude Code

@bokelley
Copy link
Copy Markdown
Contributor

bokelley commented Jun 7, 2026

Superseded by #938, which re-applies this PR's diagnostics — the named-chain error attribution (pre_validation_hook[i] ...) and the real-path MCP/A2A tests, consolidated into a new adcp/server/_hooks.py — freshly on current main (#930/#933 had moved the server files this branch was based on, so a rebase wasn't clean). You're credited as co-author on #938, @sangilish. The core hook composition shipped earlier in #860, and #938 closes #859. Thanks for the original work.

@bokelley bokelley closed this Jun 7, 2026
bokelley added a commit that referenced this pull request Jun 7, 2026
…ures (#938)

Move the pre-validation hook types, compose_pre_validation_hooks, and the
_flatten/_apply helpers into a single source-of-truth module
adcp.server._hooks. spec_compat.py re-imports and re-exports the public
names with __all__ so existing imports keep working; __init__.py,
a2a_server.py, mcp_tools.py, and serve.py now import from _hooks.

Add PreValidationHookError so INVALID_REQUEST messages name the failing
chain index and callable (e.g. "pre_validation_hook[1] second_hook raised
RuntimeError: ...") instead of a generic "pre_validation_hook raised ...".
The dispatcher catches it on the shared create_tool_caller path, so both
MCP and A2A surface the attributed message.

Replace the mock/spy MCPToolSet test with a real call_tool() behavior test,
add an A2A ordered-chain executor test, and add MCP + A2A chain-failure
attribution tests asserting the index and callable appear in the error.

Closes #859
Supersedes #882

Co-authored-by: sangilish <sangilish@users.noreply.github.com>
Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants