fix(runtime): surface agent_call middleware bypass on sync agent methods - #115
Open
Atharva-Kanherkar wants to merge 2 commits into
Open
Conversation
`agent_call` middleware is documented as wrapping the "entire agent method
execution" and as the auth / rate-limiting layer, but the sync method wrapper
skips it — middleware is async and cannot wrap a sync calling convention.
A guard registered via `intercept("agent_call", ...)` therefore appears
installed while a synchronous capability runs unchecked, including when
generated CodeAct Python calls it. The failure is silent, and `def` vs
`async def` is not a keyword anyone reads as a policy boundary.
This does not change the execution model. It makes the gap visible and
corrects the documentation that promised coverage sync methods never had:
- Emit a RuntimeWarning, once per wrapped method, when a sync agent method
runs while agent_call middleware is registered. Calls originating inside a
CodeAct cell route to the logger instead: cells redirect sys.stderr into a
capture buffer, so a warning there would be invisible to the developer and
would be fed back to the model as cell output.
- Correct the AgentCallContext and intercept() docstrings.
- Add regression tests covering the sync/async asymmetry, the delivery
channel in both call paths, and the once-per-method dedup.
Refs NVIDIA-NeMo#114
Signed-off-by: Atharva-Kanherkar <142440039+Atharva-Kanherkar@users.noreply.github.com>
Adversarial review of the previous commit found the warning did not actually reach developers, and covered only one of the several ways a method can fall outside agent_call middleware. Delivery — the warning was routed to `logger.warning` when it originated inside a CodeAct cell, on the reasoning that cells redirect sys.stderr into a capture buffer fed back to the model. But `nooa` attaches a NullHandler to its root logger, so that handler consumed the record instead of letting it fall through to logging.lastResort: with no logging configuration, nothing was emitted on either stream. The test used caplog, which attaches its own handler and hid it. Now always `warnings.warn`, clearing the stderr buffer contextvar for the duration of the call so ContextVarStream falls through to the real stream. The warning reaches the developer, the cell output the model reads stays clean, and warning filters apply uniformly. Coverage — the diagnostic lived in the sync wrapper, so it could only fire for methods that wrapper is built for. @no_trace methods (sync and async), staticmethod/classmethod, and methods inherited from non-Agent bases are never wrapped at all, and all bypass middleware silently. Note this includes async methods: @no_trace async is uninstrumented and therefore also unguarded. Added a class scan that runs from the async wrapper at the entry point, keyed on the attribute rather than on how it was declared: covered iff the metaclass wrapped it and the result is a coroutine function. This reports every uncovered method proactively, before the model can call one, and reports nooa-internal methods to nobody. The per-call sync warning remains for code that calls a sync capability without entering an async method at all. Error promotion — the emit path was inside a blanket `except Exception`, so `-W error` was swallowed, and the delivered flag was set before the emit, so a warning that raised was never retried. The flag is now set after a successful emit and only diagnostic construction is guarded. Docs — corrected the remaining whole-method coverage claims in the middleware module docstring, the middleware-hooks skill, and the NeMo Relay example. Refs NVIDIA-NeMo#114 Signed-off-by: Atharva-Kanherkar <142440039+Atharva-Kanherkar@users.noreply.github.com>
Atharva-Kanherkar
marked this pull request as ready for review
August 8, 2026 09:29
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
refs #114
agent_callmiddleware is documented as wrapping the entire agent method, but it only wraps traced async methods. sync methods,@no_tracemethods (sync or async), staticmethod/classmethod, and methods inherited from non-Agent bases all run outside it, so a registered guard silently does not apply.no behaviour change. this warns instead of staying silent, and fixes the docs that promised full coverage.
real
gpt-5-minirun of the reproducer from #114, on this branch:same result as the issue, but the gap is now reported at
await agent.perform(), before the model reachescharge_card.changes:
middleware.py,event_manager.py, the middleware skill, and the relay example-W error6263 passed, 152 skipped.
tests/test_mcpexcluded, optional dep not installed here. ruff and format clean.open question: should this raise instead of warn? and
@no_trace asyncbypassing middleware may be worth its own issue, since one decorator is controlling both tracing and policy.