Skip to content

fix(tools): run middleware layers exactly once and honour pre-delegation vetoes [K-01] - #4

Merged
haydarkadioglu merged 1 commit into
mainfrom
feat/K-01-middleware-exactly-once
Sep 30, 2026
Merged

haydarkadioglu merged 1 commit into
mainfrom
feat/K-01-middleware-exactly-once

Conversation

@haydarkadioglu

Copy link
Copy Markdown
Owner

Summary

The tool middleware pipeline (tools/middleware.py) now calls the tool handler exactly once per tool call, and a middleware that rejects a call before delegating is treated as a veto instead of being swallowed.

Why

MiddlewareChain.execute() wrapped each layer in try/except Exception and, on error, fell through to call_at(index + 1, ...) — i.e. it re-ran the rest of the chain, including the terminal handler:

# tools/middleware.py:43-53 (before)
try:
    return middleware(agent, name, current_args, next_call)
except Exception as e:
    logger.warning("Middleware error in %s: %s", ...)
    # Fallback to the rest of the chain with current arguments
    return call_at(index + 1, current_args)

Because a middleware sits around next_call, the handler's own exception bubbles back up into every enclosing layer's except, and each one replays the downstream chain: 2 ** len(middlewares) handler invocations. With the default 4-middleware chain (coerce_arguments_middleware, ssrf_guard_middleware, logging_middleware, special_tools_middleware, core.py:588-589) a single raising tool runs 16 times — side effects (messages sent, files written, commands run, money spent) happen 16 times, and logging_middleware records 15 extra "failed" rows in working memory.

Measured on unfixed code with the default chain:

handler raises      : (16, 'RuntimeError: handler blew up')   <- 16 handler invocations

The same fallback also inverted the guards: ssrf_guard_middleware raises ValueError("SSRF violation: ...") before delegating, which the fallback logged and then continued past, so the blocked call executed anyway:

middleware vetoes   : (1, "returned 'handler ran'")           <- handler ran despite the veto

What changed

  • tools/middleware.py — call_at now tracks whether the layer delegated via next_call:
    • delegated and something raised → the exception propagates as-is, unchanged, and the chain is never replayed;
    • raised before delegating → the layer's own failure is logged once (Middleware <name> vetoed tool <tool>: ...) and propagates to the caller. Nothing downstream runs, so ssrf_guard_middleware and coerce_arguments_middleware actually gate the call.
  • core.py is untouched: Agent._execute_tool already catches the exception and returns Tool error (<name>): <reason> to the model, so a veto now surfaces as a tool error instead of a silent execution.
  • tests/test_tool_middleware.py — 11 new tests (parametrised chain depths included).

Provenance

Mechanism and edge cases reviewed against hermes:agent/tool_executor.py (upstream middleware runner: at-most-once layer execution, veto-vs-handler-failure distinction). Re-implemented in koza's own style — koza names, koza log messages, no upstream import, no foreign identifiers.

Verification

$ python -m pytest tests -q -p no:cacheprovider
19 passed, 3 warnings in 19.81s

(8 pre-existing tests + 11 new; all green, none skipped.)

Before/after probe, default 4-middleware chain, handler raising:

before: handler raises = 16 invocations | veto = handler still ran
after : handler raises = 1  invocation  | veto = ValueError propagates, handler never ran

Real Agent._execute_tool path (probe script, stub agent + real DEFAULT_MIDDLEWARES and real ALL_HANDLERS entry whose handler raises):

handler invocations: 1
returned to caller  : Tool error (k01_probe_tool): exploded inside handler
OK: one execution, error surfaced to the model

ruff check tools/middleware.py tests/test_tool_middleware.py: 10 findings, down from 11 on main — all pre-existing style rules (UP035/RUF013/UP006/PERF102/SIM102/PIE810/S110/BLE001), none introduced. Repo-wide ruff check . reports 1997 pre-existing findings and is unchanged by this PR.

Risk / rollback

Behaviour change: a middleware failure that previously let the tool run now surfaces as Tool error (...) (fail-closed). For the shipped middlewares this is the intended semantics — is_safe_url() in skills/web.py already catches its own exceptions and returns False, so the SSRF guard only raises on a genuine violation; coerce_tool_args() is defensive and returns args unchanged when it cannot coerce; logging_middleware fails after the handler has run and only re-raises. Error-handling middleware that intentionally catches a downstream failure and returns a fallback (a supported pattern, covered by a test) is unaffected. Rollback: revert this single commit — no config, schema, storage or interface change.

Note for the owner: tests/ is listed in .gitignore (line 63) while tests/test_custom_base_url.py and tests/test_telegram_auto_delegation.py are tracked, so this test file had to be added with git add -f. Worth removing that ignore line separately (K-19 / K-52 territory) so future tests are not silently dropped.

Roadmap: K-01

@haydarkadioglu
haydarkadioglu merged commit c50f0a7 into main Sep 30, 2026
0 of 2 checks passed
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.

1 participant