Skip to content

fix(witan-core): chain every proxy classification from the exception it located - #290

Open
blarghmatey wants to merge 1 commit into
mainfrom
proxy-exception-chaining
Open

fix(witan-core): chain every proxy classification from the exception it located#290
blarghmatey wants to merge 1 commit into
mainfrom
proxy-exception-chaining

Conversation

@blarghmatey

Copy link
Copy Markdown
Member

What are the relevant tickets?

Follow-up to #209, which fixed one of these branches and flagged the rest. Tracked in witan as tk-exception-chaining-in-reclassifying-is-inconsist-ac79d7.

Description (What does it do?)

RemoteMCPProxy._reclassifying catches one exception and classifies a different one — every classifier walks _chain precisely because anyio re-raises through an ExceptionGroup, so the fault is a member of the group rather than the group itself. Raising from exc then put the container on __cause__ and made a caller redo the walk that had just been done for them.

  • Five of the six raise sites did that. RemoteToolFailed was the exception, fixed in fix(witan): a server-side refusal on a remote target was a traceback, not a sentence #209 because its docstring published a __cause__ contract the chaining made false.
  • RemotePayloadTooLarge, RemoteWriteIndeterminate, RemoteCredentialRejected and both RemoteUnreachable raises now chain from the located exception. The group stays reachable on __context__.
  • The rule is stated once in the _reclassifying docstring instead of as a comment on the single branch that followed it. The branch count had grown from three classifiers to five (six raise sites) with each new one copying whichever neighbour was read first — the drift a stated rule exists to stop.
  • A test per branch pins which exception lands on __cause__ and which on __context__. Only the tool-failure branch had one. Three had nothing pinning the grouped case at all — transport drop, gateway-cut read, rejected credential — and the transport branch's existing test (test_the_underlying_error_is_kept_as_the_cause) uses an ungrouped connect failure, where exc and the located exception are the same object and the distinction cannot show up.

This is an observable change, not the free consistency edit it was filed as. The task reasoned that nothing read __cause__ on these types. Two tests did, and pinned the group there — test_a_413_carries_the_underlying_error_as_its_cause and test_an_indeterminate_write_carries_the_underlying_error_as_its_cause. Both now assert the located exception instead. Anything walking these exceptions by hand should read __context__ for the group.

How can this be tested?

just test-witan-core      # 568 passed, 2 skipped
just test-witan-council   # 974 passed
just test-witan-code      # 523 passed

witan-code's 4 failures (test_branches.py ×3, test_graph.py ×1) are pre-existing on main — they assume a logged-out actor and fail on any machine with a witan identity. Tracked separately as tk-four-witan-code-tests-fail-on-any-machine-with-a-298214; I verified them failing on a clean main checkout before starting.

To see the change directly, the new per-branch tests are the readable form — e.g. test_a_drop_carries_the_transport_error_as_its_cause in packages/witan-core/tests/test_remote_proxy.py.

I checked the tests are actually load-bearing rather than merely green, by reverting each classifier's chaining to from exc one at a time and re-running. Each revert fails exactly the test for that raise site and nothing else:

reverted fails
from oversized test_a_413_carries_the_underlying_error_as_its_cause
from cut_off test_an_indeterminate_write_... and test_a_gateway_cut_read_... (this classifier has two raise sites)
from rejected test_a_rejected_credential_carries_the_auth_error_as_its_cause
from dropped test_a_drop_carries_the_transport_error_as_its_cause
from refused test_a_refusal_arriving_inside_an_exception_group_is_still_classified

Additional Context

Two non-test readers of __cause__ exist and neither is affected: witan_core.elicit._pending_ask walks __cause__ or __context__, so it reaches the same exceptions either way; witan-code's test_remote_store assertion is against witan_code.remote.store's own separate RemoteToolFailed.

No new public witan_core symbol, so no server floor bump — just check-core-floor and just check-versions are clean. witan-core bumped 0.32.1 → 0.32.2.

Gotcha worth knowing if you touch these tests: _ScriptedProxy assumes an unclassified tool writes, so a read-path test needs _ReadingProxy or it silently takes the write branch and asserts nothing about the raise it meant to cover. My first version of the gateway-read test did exactly that.

…it located

`_reclassifying` catches one exception and classifies a different one: each
classifier walks `_chain` precisely because anyio re-raises through an
`ExceptionGroup`, so the fault is a member of the group rather than the group
itself. Raising `from exc` then handed a caller reading `__cause__` the
container and made them redo the walk that had just been done for them.

Five of the six raise sites did that. Only `RemoteToolFailed` did not, because
its docstring published a `__cause__` contract that the chaining made false —
which is why #209 fixed that one alone and filed the rest.

OBSERVABLE, not the free consistency edit it was filed as. The task reasoned
"nothing reads `__cause__` on either type today", but two tests did and pinned
the group there: test_a_413_carries_the_underlying_error_as_its_cause and
test_an_indeterminate_write_carries_the_underlying_error_as_its_cause. Both now
assert the located exception on `__cause__` and the group on `__context__`,
matching the tool-failure test that already worked that way. Anything walking
these exceptions by hand should read `__context__` for the group.

The rule is stated once in the `_reclassifying` docstring instead of as a
comment on the single branch that followed it. The branch count had already
grown from three classifiers to five (six raise sites — the gateway branch
raises two classes) with each new one copying whichever neighbour was read
first, which is the drift a stated rule is there to stop. Every branch now has
a test pinning which exception lands where.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_0162A7PPanL5ZWKd9BAJmAhn
Copilot AI balanced review requested due to automatic review settings August 26, 2026 11:02

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.

Pull request overview

Updates witan-core proxy error chaining so reclassified errors directly expose the located underlying exception.

Changes:

  • Chains all proxy classifications from their identified exception.
  • Adds regression coverage for every classification branch.
  • Releases witan-core 0.32.2 with changelog and lockfile updates.

Reviewed changes

Copilot reviewed 4 out of 5 changed files in this pull request and generated no comments.

Show a summary per file
File Description
uv.lock Updates the workspace package version.
packages/witan-core/witan_core/remote/proxy.py Standardizes exception chaining.
packages/witan-core/tests/test_remote_proxy.py Verifies causes and contexts for grouped errors.
packages/witan-core/pyproject.toml Bumps witan-core to 0.32.2.
packages/witan-core/CHANGELOG.md Documents the observable chaining change.

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

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