fix(middleware): complete transactional request transform contract - #119
fix(middleware): complete transactional request transform contract#119Skywind5487 wants to merge 2 commits into
Conversation
૮ >ﻌ< ა ci reviewran on 0517125 — docs(middleware): document atomic LLM request transforms
|
Validation update — Aug 19 current-main rebuildThe Aug 17 Topology
Correctness blockerThe shallow-copy fallback is gone. The generic Final-head CIHead
Focused regression evidence from slice 4/12:
That file includes the non-deepcopyable-provider-object regression and nested mutation-before-raise rollback regression, so the two correctness boundaries are exercised on the final head rather than inferred from aggregate CI status. Ready for re-review; not merged. |
768f898 to
243352e
Compare
ac6b010 to
243352e
Compare
Skywind5487
left a comment
There was a problem hiding this comment.
HOLD — two blocking contract regressions found. Full two-axis report is on #117.
-
apply_llm_request_middleware()switched from discovery-awarehas_middleware()/invoke_middleware()to_get_middleware_callbacks()→get_plugin_manager()._middleware. The plugin delivery layer explicitly lazy-discovers on first middleware delivery; the raw registry lookup does not. A configuredllm_requestplugin can therefore look “missing” in a fresh process and the request will be dispatched unchanged. Please preserve a discovery-aware gate/snapshot and regression-test first-use delivery without an explicitdiscover_and_load(). -
The
_safe_copy()change is outside this LLM-only merge unit and changes tool-request behavior.apply_tool_request_middleware()shares that helper, so non-deepcopyable tool args that previously degraded to a shallow dict copy can now raise. The LLM transaction already uses directdeepcopy()locally, so the smallest safe fix is to leave_safe_copy()at base semantics and keep strict isolation insideapply_llm_request_middleware()only.
CI is green and the actual transaction semantics (ordered commits, stable original snapshot, rollback after callback mutation/failure, no-listener identity, common dispatch seam) otherwise look solid.
Apply registered llm_request middleware transactionally: callbacks run in registration order on isolated deep copies of the complete provider request, and a replacement commits only after it can be deep-copied. If the original request cannot be isolated, middleware is skipped and the original request is returned unchanged. Strict copying is local to this path; the shared _safe_copy() helper keeps its tolerant deepcopy-with-shallow-fallback behavior for tool-request middleware. Code, contract regression tests, and documentation travel together: atomic compose/replace semantics, stable original_request, discarded candidate mutation on callback failure, fail-open on uncopyable original/replacement, and transform-before-common-dispatch ordering.
llm_request delivery now triggers first-use plugin discovery before snapshotting callbacks, so a configured plugin in a fresh process is never mistaken for no middleware. This seam must not depend on which surface imported Hermes first (dashboards, TUI slash workers, query mode, cron). Adds a fresh-process regression that calls apply_llm_request_middleware() without an explicit prior discovery and proves the configured plugin runs.
0517125 to
a9739e3
Compare
Closes #117.
Completion claim
The outbound LLM request transformation extension point is complete as one current-main merge unit. This PR uses the merged
llm_requestmiddleware seam and makes its replacement chain genuinely transactional: no shallow-copy fallback can expose shared nested request state to a failing callback.Current-main / prior-art preflight
Pinned during the Aug 19 rebuild:
Skywind5487/hermes-agent:main:243352e7b8bddc9f33eba1b6506810f8dd88beaaNousResearch/hermes-agent:main:5dd15872a6878a19b9b5478b6968b38f48dd311fPrior-art classification:
llm_request/llm_executionmiddleware and the common request/dispatch seam.transform_api_requesthook and closed unmerged; its issue was later closed in favor of middleware.This fork PR implements only the LLM request-transform contract required by #117. It does not pull NousResearch#82092's tool/Relay-specific changes into this merge unit.
What changes
Atomic
llm_requestchainapply_llm_request_middleware()keeps the transaction boundary inline and explicit:{"request": {...}}replacement that can itself be deep-copied.There is deliberately no shallow-copy fallback. A shallow dict copy shares nested lists/dicts and cannot uphold rollback.
The transaction stays inline instead of introducing
_run_request_chain(kind, payload_key, original_key, ...); there is only one maintained LLM request transaction today. The shared_safe_copy()helper keeps its tolerant deepcopy-with-shallow-fallback behavior for tool-request middleware — this PR does not change tool-request semantics.Documented boundary
The existing canonical
docs/middleware/README.mdnow documents the atomic composition/failure semantics and the lazy-discovery delivery guarantee. Its existing execution-order section pins the maintained boundary:llm_requestpre_api_requestllm_executionFocused regression tests
Tests cover only the contracts that can regress here:
original_request;llm_requestplugin runs on first delivery even before discovery has run (callsapply_llm_request_middleware()without an explicitdiscover_and_load());PR topology
This PR was rebuilt from current fork
main, not the old Phase-2devlineage.243352e7b8bddc9f33eba1b6506810f8dd88beaafork/request-transform13c3f787atransactionalllm_requestrequest transform;a9739e3a2preserve lazy plugin discovery forllm_requestmain: ahead 2 / behind 0hermes_cli/middleware.py,tests/hermes_cli/test_request_transform_contract.py,tests/hermes_cli/test_plugins.py,docs/middleware/README.mdAcceptance mapping