Skip to content

Make one rule govern every failure in the function-invocation seam - #147

Merged
shibayan merged 4 commits into
masterfrom
fix/function-middleware-error-contract
Sep 1, 2026
Merged

Make one rule govern every failure in the function-invocation seam#147
shibayan merged 4 commits into
masterfrom
fix/function-middleware-error-contract

Conversation

@shibayan

@shibayan shibayan commented Sep 1, 2026

Copy link
Copy Markdown
Member

What this changes

Closes #107, both halves.

Where a failure surfaces. A tool body's exception was written to ctx.error and next()
resolved, so a middleware wrapping the call never saw it — while an exception from an inner
middleware
propagated normally. The same try { await next() } catch behaved differently
depending on which layer threw. The repo's own examples/03-extensibility/01-middleware.ts timing
middleware demonstrated the consequence: it silently skipped exactly the calls worth timing.

Whether a failure is fatal. A middleware exception failed the whole run; a tool exception did
not. Both now become that call's function_result and the loop continues, still counting against
maxConsecutiveErrors (default 3, the same number Go uses), so a layer that keeps failing still
ends the run.

MiddlewareFailed says a failure must end the run instead: never turned into a result, cancels the
rest of a concurrent batch, reaches the caller. A tool body may throw it too.

A correction to how this issue was framed

The issue's second half is written as "are tool exceptions fatal", and I carried that framing into
the design notes. Pinning the current behaviour first showed it was wrong: tool bodies were
already fail-open
(runTool catches and returns an exception result — its own comment says "never
a failed run"). Only middleware exceptions were fatal. That narrowed the change and made it a
unification rather than a reversal of the tool contract.

It also dissolved the security argument I had for keeping the old behaviour. That argument was that
toolApprovalMiddleware's rule.when predicate throwing should abort the run. But the predicate is
evaluated before next(), so reporting its failure to the model does not let the tool run — the
call is still stopped. What changes is only whether the run dies immediately or after the error
budget. There is a test for that.

Parity

  • Reference checked, unwinding: .NET AgentHooksFunctionMiddleware wraps await next(context, …)
    in try/catch and catches the invocation's exception; Python runs the tool body as
    final_handler inside middleware_pipeline.execute and catches outside it (_tools.py:1607).
    Go has no function-middleware seam.
  • Reference checked, fail-open: .NET's own comment calls the loop "fail open" — it "converts thrown
    exceptions into tool errors and keeps running", with context.Terminate as the only loud escape.
    Python: except Exception → _function_execution_error_result, re-raising only MiddlewareFailure,
    MiddlewareTermination and UserInputRequiredException (_tools.py:1635). Go: fail-open with
    MaximumConsecutiveErrorsPerRequest, default 3.
  • MiddlewareFailed is Python's MiddlewareFailure; the name follows this codebase's existing
    MiddlewareTerminated. A tool body may raise it because Python's re-raise sits outside the whole
    pipeline, tool body included.
  • Cooperative cancellation reaches exactly as far as Python's, including the limit: a sibling that
    ignores its signal runs to completion and may still have its effects; the result is discarded
    either way.
  • TypeScript-specific addition: ctx.error stays set alongside the throw, so a middleware can
    observe without catching. The throw is the channel — clearing ctx.error does not clear the
    failure.
  • Wire format affected: no
  • Public API affected: yes (MiddlewareFailed added)
  • Breaking change: yes

Notes

Not in scope, filing separately: settlement of dangling calls when the service owns the transcript,
which MiddlewareFailed and the iteration limit can both produce. Python implements it
(_tools.py:3096-3130) and it is what keeps Anthropic from rejecting the next request — an unanswered
tool_use is a measured 400 there.

Checklist

  • pnpm check passes (lint, typecheck, build, test)
  • Behaviour changes are covered by a test that fails without the change
  • Public API changes are reflected in the package README and CHANGELOG.md

A thrown error behaved differently depending on which layer threw it. A tool
body's exception was written to `ctx.error` and `next()` resolved, so a
middleware wrapping the call could not see it; an inner middleware's
exception propagated normally. Same syntax, opposite outcome. And a middleware
exception failed the whole run while a tool exception became a result the
model reads.

Both now follow one rule, the one .NET and Python already use: the failure
travels out through the middleware around it, and whatever nothing recovered
becomes this call's `function_result`. The round still counts against
`maxConsecutiveErrors`, so a layer that keeps failing still ends the run.

`MiddlewareFailed` is the way to say a failure must end the run instead. It is
never turned into a result, it cancels the rest of a concurrent batch, and it
reaches the caller. A tool body may throw it too, matching where Python places
its own re-raise.

`ctx.error` stays readable for a middleware that only wants to observe, but
the throw is the channel: clearing it no longer clears the failure.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Copilot AI lite review requested due to automatic review settings September 1, 2026 05:20
@shibayan shibayan added bug Usage: [PRs], Target: bug fixes and regressions; issues use the Bug issue type public-api-change Usage: [PRs], Target: changes to exported public APIs that require API review breaking change Usage: [PRs], Target: changes that are not backward compatible labels Sep 1, 2026
@github-actions github-actions Bot added documentation Usage: [Issues, PRs], Target: documentation changes core Usage: [Issues, PRs], Target: packages/core labels Sep 1, 2026

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Changes recommended

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Pull request overview

This PR standardizes the error contract at the function-invocation middleware seam so that failures from either tool bodies or function middleware propagate consistently through await next(), and introduces a dedicated fatal error (MiddlewareFailed) to abort an entire run when needed.

Changes:

  • Make tool-body failures unwind through the function middleware onion (so try/catch and try/finally around await next() behave consistently).
  • Treat ordinary function-middleware exceptions as recoverable tool-call failures reported via function_result (counting toward maxConsecutiveErrors), while adding MiddlewareFailed as an explicit fatal escape hatch.
  • Add/adjust tests and update example + changelog guidance for middleware authors.
File summaries
File Description
packages/core/src/middleware/middleware.ts Updates function middleware JSDoc to reflect new failure propagation and recovery patterns.
packages/core/src/middleware/middleware.test.ts Adjusts recovery test to use try/catch around next() under the new contract.
packages/core/src/index.ts Exports MiddlewareFailed on the core public surface.
packages/core/src/errors.ts Adds the new MiddlewareFailed error type and documentation.
packages/core/src/client/function-middleware-errors.test.ts New test suite pinning the unified seam contract and MiddlewareFailed behavior (including concurrency cancellation signaling).
packages/core/src/client/function-execution.ts Implements unified exception boundary behavior and abort signaling for concurrent batches on MiddlewareFailed.
examples/03-extensibility/01-middleware.ts Updates timing middleware example to use finally so failures are timed/logged too.
CHANGELOG.md Documents the breaking changes and migration guidance for middleware authors.
Review details
  • Files reviewed: 8/8 changed files
  • Comments generated: 2
  • Review effort level: Lite

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Comment thread packages/core/src/errors.ts Outdated
Comment thread packages/core/src/client/function-execution.ts
A middleware that threw after `await next()` had already succeeded left
`ctx.result` set, and the seam read that as a completed call: the failure was
reported nowhere. The call as a whole did not succeed, so the result is
cleared alongside recording the error.

Recovery is unaffected — a middleware that recovers catches the failure
itself, so the chain resolves and never reaches that branch.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Copilot AI review requested due to automatic review settings September 1, 2026 05:32
@github-actions github-actions Bot removed the public-api-change Usage: [PRs], Target: changes to exported public APIs that require API review label Sep 1, 2026

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Changes recommended

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review details

Suppressed comments (1)

Previously missed (1) — in code that hasn't changed since the last review.

CHANGELOG.md:26

  • The PR description/checklist says the public API change (adding MiddlewareFailed) is reflected in the package README, but there are no MiddlewareFailed references in any README.md in this repo. If the intent is to document the new fatal-error contract/migration for middleware authors, please either update the relevant README(s) (root and/or packages/core/README.md) or adjust the PR description so it accurately reflects what shipped.
- **[BREAKING] `@polymind-inc/agent-framework-core`** — an exception from a function middleware is
  reported to the model as that call's `function_result` and the loop continues, instead of failing
  the run. Tool bodies already behaved this way; middleware did not, and all three reference
  implementations treat both the same. The round still counts against `maxConsecutiveErrors`
  (default 3), so a layer that keeps failing still ends the run — this is not a licence to fail
  forever. **This reversal is silent**: nothing in the type system or the linter will point at
  middleware that relied on throwing to abort. Throw the new `MiddlewareFailed` where that was the
  intent — it is never turned into a result, it cancels the rest of the concurrent batch, and it
  • Files reviewed: 8/8 changed files
  • Comments generated: 1
  • Review effort level: Lite

Comment thread packages/core/src/middleware/middleware.ts Outdated
… README

`ctx.error` is set only for a tool-body failure. A middleware's throw unwinds
without passing through the tool seam, so it is still unset while an outer
`catch` or `finally` runs — pinned with a test rather than asserted.

The core README carried nothing about the middleware error contract, which
the PR checklist claimed it did.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Copilot AI review requested due to automatic review settings September 1, 2026 05:40

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Changes recommended

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review details
  • Files reviewed: 9/9 changed files
  • Comments generated: 1
  • Review effort level: Lite

Comment thread packages/core/src/client/function-execution.ts
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Copilot AI review requested due to automatic review settings September 1, 2026 05:50

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔵 Needs a closer look

Review details
  • Files reviewed: 9/9 changed files
  • Comments generated: 0 new
  • Review effort level: Lite

@shibayan
shibayan merged commit 3330a20 into master Sep 1, 2026
9 checks passed
@shibayan
shibayan deleted the fix/function-middleware-error-contract branch September 1, 2026 06:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

breaking change Usage: [PRs], Target: changes that are not backward compatible bug Usage: [PRs], Target: bug fixes and regressions; issues use the Bug issue type core Usage: [Issues, PRs], Target: packages/core documentation Usage: [Issues, PRs], Target: documentation changes

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Define the function middleware error contract: propagation, recoverable tool errors, and fatal failures

2 participants