Skip to content

[AAASM-5752] 🐛 (core): Resolve an audit-sink disposition without raising, and pin the shipped set - #321

Merged
Chisanan232 merged 4 commits into
mainfrom
v0.0.1/AAASM-5752/fix/audit_sink_disposition
Aug 14, 2026
Merged

[AAASM-5752] 🐛 (core): Resolve an audit-sink disposition without raising, and pin the shipped set#321
Chisanan232 merged 4 commits into
mainfrom
v0.0.1/AAASM-5752/fix/audit_sink_disposition

Conversation

@Chisanan232

@Chisanan232 Chisanan232 commented Aug 14, 2026

Copy link
Copy Markdown
Contributor

Description

Two edge cases in the computed audit_sink disposition, both measured during the independent review of AAASM-5731 / PR #315. Neither was a defect in what shipped — the review recommended merge, which happened — but each weakened a property that PR otherwise establishes.

1. Resolving a disposition could raise. audit_sink became a computed property under AAASM-5731, which is the right fix for the false absent it replaced. It also introduced a failure mode the previous class attribute did not have: getattr(x, name, None) swallows AttributeError and nothing else, so a wrapped client whose __getattr__ raises anything else turned into

ConfigurationError: Failed to initialize assembly runtime: client is not connected

out of init_assembly.

Guarded at three sites, not the one the ticket names. The ticket points at resolve_audit_sink, but the reproduction actually reaches resolve_delegated_audit_sinkRuntimeQueryInterceptor.audit_sink delegates there — and that property reads runtime_can_record first, which has the same getattr shape. Fixing only the named site would have left the reproduction reproducing.

Both fall to the under-claiming answer. absent is the value init_assembly warns on, so a handler this SDK cannot read is reported as making no record rather than passing silently for one that does.

2. The gate accepted a value the shipped matrix cannot produce. test_every_shipped_governance_handler_declares_its_audit_sink accepted any of the four vocabulary values. It now asserts the set.

Correction to the ticket's premise

The ticket states the shipped matrix yields {absent, discarded}. Measured, it yields {forwarded, absent} — AAASM-5750 moved RuntimeQueryInterceptor to forwarded after the ticket was filed. The assertion pins what is actually produced.

discarded and caller-supplied are real and reachable, just not from this matrix: discarded is the LangChain handler wrapping an interceptor that records nothing, caller-supplied is an object that declares nothing. Each gets its own case with its own reason, rather than a standing waiver inside the matrix gate.

Type of Change

  • 🔧 Bug fix

Breaking Changes

  • No

Related Issues

Testing

  • Unit tests added/updated

Full suite: 1338 passed, 15 skipped.

The gate was one notch weaker — measured before and after

Same production mutation both times (RuntimeQueryInterceptor.audit_sink returning caller-supplied), running only the matrix gate so sibling tests cannot stand in for it:

gate form verdict alone
pre-5752, four-value acceptance GREEN — the defect
this PR, set assertion RED

With the whole file running, the mutation reddens 9 tests either way; defence-in-depth was never in question. What changed is that the gate now decides something by itself.

Every guard proven able to fail

Each reverted in turn, suite re-run, restored:

reverted result
resolve_delegated_audit_sink guard test_resolving_a_disposition_never_raises RED
resolve_audit_sink guard RED
runtime_can_record guard RED

And the existing controls still bind in both directions, as the ticket's AC4 requires — runtime_can_record hard-wired True reddens the never-raises assertion; hard-wired False reddens 8 tests including the matrix gate, the forwarding controls and the init-warning one.

One mutation I ran first was worthless and is recorded so it does not get repeated: rewriting the constant AUDIT_SINK_FORWARDED = "caller-supplied" moves the value the assertion compares against, so it is a tautology and passes. The mutation has to be at the property.

Checklist

  • Code follows project style guidelines
  • Self-review completed
  • Comments added for complex logic
  • Documentation updated if needed
  • All tests passing

audit_sink became a computed property under AAASM-5731, which is the right fix
for the false `absent` it replaced, but it gave the lookup a failure mode the
class attribute did not have: `getattr(x, name, None)` swallows AttributeError
and nothing else, so a wrapped client whose `__getattr__` raises surfaced as
`ConfigurationError: Failed to initialize assembly runtime: client is not
connected` out of `init_assembly`.

Guarded at all three sites a disposition is read through, not only the one the
ticket names — the reproduction reaches `resolve_delegated_audit_sink` via
`RuntimeQueryInterceptor.audit_sink`, which first reads `runtime_can_record`.

Both fall to the under-claiming answer: `absent` is the value `init_assembly`
warns on, so a handler this SDK cannot read is reported as making no record
rather than passing silently for one that does (AAASM-5752).
…arately

The gate accepted any of the four vocabulary values, including
`caller-supplied` — which the shipped matrix cannot produce and which is exactly
what `init_assembly` treats as "do not warn". Measured before/after against one
production mutation (RuntimeQueryInterceptor.audit_sink returning
caller-supplied), running the gate ALONE: the four-value form passes, the set
assertion fails.

`discarded` and `caller-supplied` are reachable — just not from this matrix — so
each gets its own case rather than a standing waiver, along with the raising
client. All three guards proven able to fail by reverting each in turn.
(AAASM-5752)
"""

class Raises:
def __getattr__(self, name: str) -> object:
A `.venv` symlink pointing at a sibling worktree's environment slipped past
`.venv/` — that pattern matches a directory only — and was committed on this
branch. CI then failed at `uv sync`:

    error: failed to create directory `.venv`: File exists (os error 17)

Measured both ways: with the old rule alone `git check-ignore .venv` does not
match the symlink; with the added rule it matches at .gitignore:12.
@codecov

codecov Bot commented Aug 14, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.

📢 Thoughts on this report? Let us know!

…e union

Review measured why the set form is not enough: it detects a NEW value but not
a WRONG ASSIGNMENT within the set. Flipping GatewayClient.audit_sink from
absent to forwarded — a false retention claim on two shipped configurations,
the defect this type exists to prevent — left the whole suite green, because
{absent, forwarded} was still the union. The per-label mapping reddens the gate
alone under that mutation.

The discarded case now drives the shipped factory instead of a 3-line stub: the
stub pinned the substitution rule inside AssemblyCallbackHandler, and passed
under the same mutation (AAASM-5752).
@Chisanan232

Copy link
Copy Markdown
Contributor Author

Review round 1 — findings addressed

Approved with three recommendations; all three acted on. Head 02dc05b.

G2 — the set assertion did not bind the assignment, and that mattered

The review measured the hole: flipping GatewayClient.audit_sink from absent to forwarded — a false retention claim on two shipped configurations, the exact defect this type exists to prevent — left the entire suite green, because {absent, forwarded} was still the union.

The gate now asserts a per-label mapping. Under the same mutation it reddens alone.

That is the second time in this PR that a form which "looks stricter" decided less than it appeared to. The four-value membership was one notch too wide; the set assertion was another. A set detects a new value; only a mapping binds which branch produces which.

G1 — the discarded case now drives shipped code

It used a 3-line stub, and the review showed that under a production mutation removing discarded from two of the four shipped configurations that produce it, the case still passed — it pinned the substitution rule inside AssemblyCallbackHandler, not that shipped code reaches the value. It now wraps the "runtime unreachable, observe" and "native missing, observe" interceptors from _shipped_handler_matrix, which is the substitution _register_adapters actually performs.

The caller-supplied case stays a stub, for the reason its own docstring gives: no shipped handler reaches it.

G3 — the end-to-end test does not bind, and I am not shipping it

The recommendation was a regression test at init_assembly, where the reported ConfigurationError surfaces. I wrote it. It passed with every guard reverted, so it went in the bin rather than into the suite.

Cause: a client that raises on attribute access degrades the connect, so init_assembly falls back to an interceptor with no runtime and reports absent for an unrelated reason. Three constructions were tried — raise on every unknown attribute, raise only on the three audit-hook names, and a composed client with real register/query_policy. All three passed under all three guard mutations.

This is consistent with what the ticket itself says: "Unreachable on the shipped path — init_assembly builds its own GatewayClient — and reachable only by calling build_governance_interceptor directly with such a client." An init_assembly-level test cannot reproduce a symptom the shipped path does not reach.

So the binding is made at the reachable path instead: test_building_an_interceptor_over_a_raising_client_reports_absent_rather_than_failing drives build_governance_interceptor directly. Reverting resolve_delegated_audit_sink's guard or runtime_can_record's guard reddens it.

The docstring records that the init_assembly version was written, measured, and dropped — so the next person does not spend the same three attempts rediscovering that it cannot fail.

Also confirmed from the review

  • The .venv symlink committed in eaa9ef0 is gone at HEAD, and .gitignore now matches a symlink named .venv as well as a directory. Measured both ways: with the old rule alone, git check-ignore .venv does not match the symlink.
  • The send_tool_outcome observation is right and is a strict improvement this PR did not claim: pre-fix, runtime_can_record sat outside that function's try, so a raising getattr propagated into the governed tool path against the function's own documented contract.
  • The two unguarded properties on the LangChain handler (_enforce, __getattr__) are outside the disposition class and are read on the governed-call path. Not touched here; worth a follow-up.

Suite

test/unit + test/integration: 1339 passed, 15 skipped. Bench-latency cases excluded — they fail identically on the unmodified base checkout on this machine.

"""

class RaisesOnEveryLookup:
def __getattr__(self, name: str) -> Any:
@sonarqubecloud

Copy link
Copy Markdown

@Chisanan232
Chisanan232 merged commit 8ed7e72 into main Aug 14, 2026
26 checks passed
@Chisanan232
Chisanan232 deleted the v0.0.1/AAASM-5752/fix/audit_sink_disposition branch August 14, 2026 09:22
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