[AAASM-5731] 🐛 (core): Declare and test the non-resolving hook-layer audit path - #315
Merged
Merged
Conversation
The adapters' audit hook is duck-typed and returns None, so a handler that retains the record, one that drops it, and one on which the hook never resolves are indistinguishable at the call site. Give a handler a way to say which it is. The vocabulary separates "absent" from "discarded" because they are different failures with different remedies: a discarded record was built and handed over, whereas an absent hook means nothing is attempted at all — which is why this SDK's gap covers the allowed path and not only the denied one. It carries no "recorded" value: the SDK can only speak for handlers it built, so the honest answer for anything else is the absence of a claim, and an unrecognised value degrades to that rather than being trusted through. Refs AAASM-5731
Measured against the native and HTTP boundaries: on RuntimeQueryInterceptor, getattr for record_result and on_tool_end both return None — __getattr__ delegates to a GatewayClient that has neither — so the adapters' guard finds nothing to call and no record is emitted for a governed call, allowed or denied. The fail-closed interceptor delegates the same way, so a call denied there produces nothing either. Positive controls on the same objects resolve normally (check_tool_start, report_edge), so this is specific to the audit hooks rather than broken delegation. Refs AAASM-5731
Under observe / disabled the bare GatewayClient is handed to the adapters as the governance interceptor unchanged, so its surface is also the audit surface — and it exposes neither record_result nor on_tool_end. report_edge is topology metadata, not a tool-call record, and does not close the gap. Refs AAASM-5731
…nd drops it This handler sits on the other side of the split from the interceptors it wraps: on_tool_end is defined here, so the adapters' audit-hook lookup does resolve and the record IS handed over. It is then forwarded to the interceptor's own on_tool_end, which does not exist on anything this SDK ships — so the record stops here. Accepted and dropped is "discarded", not "absent". Computed rather than declared, so a caller-supplied interceptor that really records is reported as such: this SDK claims nothing about a handler it did not build, in either direction. Refs AAASM-5731
Before this there was no signal at all that governed tool calls produce no audit evidence, so a caller had to read the interceptor to find out. Warn once per init_assembly on stderr — where logging configuration cannot silence it, as with the unregistered-agent warning — and expose the value on the returned context as the programmatic counterpart. _register_adapters now returns the disposition alongside the adapters rather than the caller re-deriving it, because building a second interceptor to ask would re-emit the one-time native-missing warning. Its stubs across five test modules are updated for the new return shape in the same commit, so no revision in between is left broken. The warning branches on the disposition: "absent" and "discarded" fail differently and a single sentence would be wrong in one direction. It does not fail init — the proxy / eBPF layers are unaffected, so refusing to start over an evidence gap would trade a truthfulness fix for an availability regression. Refs AAASM-5731
Three things are pinned separately, because any one alone passes while the defect is present: every handler the factory can return declares a disposition; the declaration matches behaviour in both directions; and init_assembly surfaces it on the default path. Handlers are enumerated by sweeping build_governance_interceptor's own branches rather than by naming classes, because a name list is not a gate — a fourth branch returning a fourth undeclared handler would pass by omission. The LangChain handler is included because _register_adapters substitutes it for the interceptor, making it equally a shipped audit surface. Both directions are pinned: "absent" asserts the hook does not resolve (which is why the gap covers the allowed path), "discarded" asserts it does resolve and still reaches nothing, and a handler that genuinely records must be reported as caller-supplied. Every absence assertion is paired with a positive control on the same boundary and with a forwarding control, since otherwise it is indistinguishable from a probe that never ran or one that cannot see a record. Refs AAASM-5731
The docstring called the shipped path Unmeasured, but ADR 0033 section 6 reserves that for an action no control inspected, where nothing is known. Here exactly where the record stops has been measured against the native and HTTP boundaries, so what is missing is a decided-but-unbuilt sink — which is Planned, with AAASM-5731 as the reference. It is certainly not Observed. Refs AAASM-5731
The README, the docs home and the site description all asserted that "every tool call, prompt, and policy decision is emitted to the gateway". The policy check is; the record is not, on any path. The architecture page also described step 5 as "the side-channel that streams audit events to the gateway" and step 8 as flushing them. Read against the code, all three branches of _start_network_layer return a no-op shutdown and start nothing, so there is no such side-channel and nothing to flush. The two example pages narrate audit output that the demos' own handlers produce; they now say so, since a caller-supplied handler is exactly the branch on which a record does survive. Refs AAASM-5731
…rfaces Reconciled against an exhaustive inventory of the repo rather than the targeted search the first docs commit was based on: 19 claims that the SDK layer produces audit evidence, of which that commit reached 9. The remaining twelve, in blast-radius order: the .claude/CLAUDE.md three-layer model, which re-seeds the claim into every future agent task on this repo; the module docstring of tool_governance.py, which asserted "either way the outcome is recorded" 170 lines above the docstring that states neither hook resolves — a reader who stops at the module docstring gets exactly the wrong model; a sample comment in framework-support.md that a user copies into their own code; the architecture diagram edge labelling the SDK→gateway hop "allow/deny + audit"; the native-shim section, where send_event exists in the shim and nothing in agent_assembly calls it; the adapter-author contract table, which documented the audit hook as a live route; three adapter/example docstrings; and the docs-home lede presenting "recorded" as a third outcome alongside allowed and denied. Nothing new is invented. The repo already contained the correct framing in _record_async_tool_result's docstring, in one example page's "a real gateway would persist server-side", and throughout the observe-mode family's "the gateway records" — this propagates that wording outward to the surfaces users read, which is the only place it had failed to reach. Refs AAASM-5731
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
RuntimeQueryInterceptor owns no audit hook — __getattr__ hands both names straight to the wrapped client — so its disposition is the client's, not a constant. As a fixed class attribute it produced a false 'absent': review of #315 measured a caller-supplied client whose record_result resolves still reporting 'absent', contradicting the very hook the adapters would have called, with the LangChain handler on top compounding it to 'discarded'. Both interceptors now compute it, as AssemblyCallbackHandler already did. The two branches are the honest ones: no hook resolves on the delegate, so nothing can be attempted through the interceptor either (absent); a hook resolves, so it came from the caller and this SDK makes no claim (caller-supplied). The old error under-claimed rather than over-claimed, which is why this is a correctness fix and not a severity one — it never reported retention where there was none, and the new resolver cannot either. AUDIT_HOOK_NAMES moves into audit_sink.py so the interceptors, the adapters and the tests cannot drift on what counts as the audit hook — a drift that would make every 'absent' declaration unfalsifiable. Refs AAASM-5731
…t one The gate read audit_sink with getattr, which every interceptor answers through __getattr__. Review of #315 measured the hole: deleting the declaration from RuntimeQueryInterceptor left GatewayClient answering for it and all eight tests passed, so the interceptor was never required to speak for itself — and a fourth interceptor that delegates would inherit 'absent' silently even when its own hook resolves. The gate now walks the handler's own MRO, which instance __getattr__ cannot satisfy. Re-measured with the reviewer's mutation: it fails and names the two affected branches. The probe carries its own control, because the gate's verdict now rests on it: a delegating object that answers the same value must be rejected while a declaring one is accepted. Without it, a probe stuck at True would make the gate pass for everything. Adds the regression test for the false 'absent', with a control that the shipped client still reads 'absent' — so the fix cannot pass by simply ceasing to report the real gap. Refs AAASM-5731
Inventory item 16. The adapter-author contract table was corrected on the record_result / on_tool_end row but not the `record` row one line below, so it still documented a live audit route. Measured: `record` resolves to None on all three shipped interceptors, and crewai/patch.py:429,445 looks it up for task start and complete — so those events are not recorded either. The row was missed because the fix was applied to the item rather than to the list it belongs to; the whole table is now consistent. Also the last residual Unmeasured. Every remaining occurrence in the repo is now part of an explicit "Planned, not Unmeasured, because where the record stops has been measured" statement. Refs AAASM-5731
|
8 tasks
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
1 participant
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.
Description
The audit hook the framework adapters call does not resolve on any interceptor this SDK ships, so a governed tool call produces no audit record — for allowed calls as much as denied ones. This PR does not build a sink. It makes the absence declared, surfaced and tested, and corrects the 19 documentation surfaces that said otherwise.
This is not the same mechanism as the Go and Node SDKs, and the difference is load-bearing. There the hook exists and its implementation drops the payload. Here the hook is absent, so the
getattrguard in_record_async_tool_resultfinds nothing and the record is never even attempted. "The sink discards" would be false in the other direction.The measured absence
record_resulton_tool_endRuntimeQueryInterceptorNoneNonecheck_tool_start✅ ·report_edge✅_FailClosedInterceptorNoneNonecheck_tool_start✅ ·report_edge✅GatewayClient(observe / disabled)NoneNonereport_edge✅The controls are on the same objects, so a blanket
getattrfailure cannot masquerade as the finding.Diagnosis — not registration, not import, not configuration. A surface gap.
RuntimeQueryInterceptor.__getattr__delegates every non-check_tool_startattribute toGatewayClient, whose surface has norecord_resultand noon_tool_end. There is no second route either: the native shim exposesRuntimeClient.send_eventand nothing inagent_assembly/calls it — the symbol is re-exported and never used.End to end through the SDK's own
run_governed_async_tool, with stubs at the native and HTTP boundaries:The forwarding control is what makes the zeros admissible in the second direction: without a handler whose record path genuinely resolves, "reached nothing" is consistent with a probe that cannot observe a record at all.
One object sits on the other side of the split
The LangChain
AssemblyCallbackHandlerdefineson_tool_end, so the lookup resolves and the record is handed over — and is then forwarded to an interceptor that has none. Accepted and dropped, which isdiscarded, notabsent. Its declaration is therefore computed, not fixed, and a caller-supplied interceptor that really records is reported as such.Claim vocabulary: Planned, not
UnmeasuredADR 0033 §6 reserves
Unmeasuredfor "no control inspected this action or payload; nothing is known about it." Exactly where the record stops has been measured, soUnmeasuredis wrong — the pre-existing wording intool_governance.pyis corrected here. Planned is the fit: "Decided but not implemented — a ticket reference; no capability claim."AuditSinkDispositionis three-valued, not Node's two, because the mechanisms differ and so do the remedies — wiring a sink is sufficient fordiscardedand is not sufficient forabsent, where nothing constructs the event:absentdiscardedAssemblyCallbackHandler;go-sdk/node-sdkclientscaller-suppliedAn unrecognised value degrades to
caller-supplied: a typo must fall back to "this SDK makes no claim", never to a claim it cannot stand behind.Type of Change
Breaking Changes
audit_sinkis a duck-typed class attribute, matching how the audit hook itself is discovered — the adapters accept any object as acallback_handler, so a base class or a Protocol registration would break every caller-supplied handler that works today.init_assemblygains onesys.stderr.writewarning. It does not fail init: a caller may not need SDK-side audit, and the proxy / eBPF layers are unaffected, so refusing to start over an evidence gap would trade a truthfulness fix for an availability regression. Written straight tosys.stderrfor the same reason as_warn_agent_unregistered—loggingconfiguration cannot silence it — and once perinit_assemblyrather than per call, so it cannot become steady-state noise.One internal signature change:
_register_adaptersnow returns(adapters, disposition). The disposition is returned rather than re-derived by the caller because building a second interceptor to ask would re-emit the one-time native-missing warning. Its stubs across five test modules are updated in the same commit, so no revision in between is left broken._register_adaptersis private; no public API changes.Related Issues
go-sdk, same ticket — same defect, discarding sink rather than absent hook.node-sdkinstance this was measured out of.Testing
pytest test/— 1222 passed / 16 skipped, exit 0 ·ruff check .0 ·ruff format --checkclean on every file touched ·mypy agent_assembly— same 4 errors as unmodifiedmain(native extension not built locally), no new ones ·pre-commitgreen on every file in this PR · all 12 commits pass the unit suite individually (bisectable).test/unit/core/test_audit_sink_disposition.pypins three things separately, because any one alone passes while the defect is present:build_governance_interceptorcan return, plus the LangChain handler that replaces it, declares a disposition;absentmust resolve no hook,discardedmust resolve one and still reach nothing, and a handler that genuinely records must be reportedcaller-supplied;init_assemblysurfaces it on the default path.Handlers are enumerated by sweeping the factory's own branches, not by naming classes — a name list is not a gate, and a fourth branch returning a fourth undeclared handler would pass by omission. The sweep carries its own positive control: it must find ≥3 distinct handler types, or an all-pass result would only mean the sweep collapsed.
Mutation evidence
Six mutations, run serially, each in one foreground command with a
trapand a SHA-256-verified restore (all six restored clean).GatewayClientstops declaringabsentsend_eventsend_event:('AUDIT-PROBE-AAASM-5731-RESULT',)"init_assemblystops warning_record_async_tool_resultreturns earlyabsentassert 'absent' == 'discarded'RuntimeQueryInterceptorstops declaring;__getattr__answersassert 'absent' == 'caller-supplied'Documentation
Reconciled against an exhaustive inventory of the repo —
README.md,CONTRIBUTING.md,SECURITY.md,mkdocs.yml,.claude/**,docs/**,quickstart_snippets/**,design/**,examples/**,validation/**,verification-reports/**, and every docstring underagent_assembly/**/*.py— not a targeted search.The count, corrected. The inventory holds 23 locations — the table's 19 plus four under "Plus" — not 19. I earlier wrote "19 of 19, none deferred", which understated the work and overstated completeness. The accurate figure is 22 corrected, 1 deliberately excluded:
README.md:321describes the examples repo's sample catalogue rather than this SDK's behaviour.Review refinements (three, two of them tests that could not fail)
1. The declaration gate was satisfied by delegation. It read
audit_sinkwithgetattr, which every interceptor answers through__getattr__. Review measured it: removing the declaration fromRuntimeQueryInterceptorleftGatewayClientanswering for it and all 8 tests passed — the interceptor was never required to speak for itself, and a fourth interceptor that delegates would inheritabsentsilently even when its own hook resolves. The gate now walks the handler's own MRO, which instance__getattr__cannot satisfy, and carries its own control (a delegating object answering the same value must be rejected while a declaring one is accepted) because the gate's verdict now rests on that probe.2. A false
absentwas reachable.RuntimeQueryInterceptor.audit_sinkwas a fixed class attribute rather than computed from what it wraps — unlikeAssemblyCallbackHandler, which computes. Measured: a caller-supplied client whoserecord_resultresolves still reported'absent', with the handler on top compounding it to'discarded'. Both interceptors now compute it. The old error under-claimed rather than over-claimed — it never reported retention where there was none, which is why this is a correctness fix rather than a severity one, and the new resolver cannot produce the dangerous direction either.AUDIT_HOOK_NAMESmoved intoaudit_sink.pyso the interceptors, adapters and tests cannot drift on what counts as the audit hook.3. Inventory item 16 was unaddressed.
authoring-adapters.mdwas corrected on therecord_result/on_tool_endrow but not therecordrow one line below, which still documented a live audit route. Measured before correcting:recordresolves toNoneon all three shipped interceptors, andcrewai/patch.py:429,445looks it up for task start and complete. The row was missed because the fix was applied to the item rather than to the list it belongs to — the "list is the unit, not the item" pattern, and the whole table is now consistent.The four worth naming:
mkdocs.yml:7— the<meta name="description">on every published docs page and the search-result snippet. Highest-visibility single location in the repo..claude/CLAUDE.md:35— re-seeded the false claim into every future agent task on this repo. It now carries an explicit "do not describe this layer as producing an audit trail."docs/examples/framework-support.md:27— "Every tool call now passes through the policy gate and is audited", inside copy-paste sample code, so it became the user's own comment.agent_assembly/adapters/_shared/tool_governance.py:8— the module docstring asserted "Either way the outcome is recorded through the audit hook" 170 lines above the docstring in the same file stating that neither hook resolves. A reader who stops at the module docstring — the normal thing to do — got exactly the wrong model, and"either way"made the false claim maximally broad.Also corrected, and worth its own mention:
docs/concepts/architecture.mddocumented a network layer that "streams audit events to the gateway" (step 5) and "flushes in-flight audit events" (step 8) when all three branches of_start_network_layerreturn _noop_shutdownand start nothing. The diagram edge labelled the SDK→gateway hopallow/deny + audit; the audit half is now gone.Nothing new was invented. The repo already contained the correct framing in three places —
_record_async_tool_result's docstring, one example page's "a real gateway would persist server-side", and the wholeobserve-mode family's consistent "the gateway records". The defect was never a misunderstanding: the accurate wording simply never propagated outward from one docstring to the surfaces users read. This propagates it, using that wording's own conventions — "offer" rather than "record", naming the specific interceptors, and stating the allowed/denied symmetry every time.Deliberately not changed:
CONTRIBUTING.md:162andSECURITY.md:59,63(pip-audit/ SBOM — supply-chain, not governance-audit) andREADME.md:321(describes the examples repo's sample catalogue, not this SDK's behaviour).design/andvalidation/contain no prose at all — PNGs only, measured rather than assumed.One thing the rewrite retains rather than fixes:
framework-support.md:27still opens "Every tool call" — a banned absolute under fd-7 — as the pre-existing policy-gate half of the sentence. Net across this PR is 4 such absolutes removed and 1 retained, and the fd-7 gate is unautomated (AAASM-5536), so this is a note rather than a claim of compliance.Out of scope: the capability manifest
AAASM-5681's manifest criterion is out of both PRs, because the manifest is not in either repo — it is
verification-reports/AAASM-5527-capability-coverage-matrix.yamlin theagent-assemblymonorepo, which would be a third repo and a third PR. Naming the specific rows so it can be filed rather than lost:S4— "LangChain tool call via the callback handler",coverage: observed,language: node. The row AAASM-5681 named; node-only, so it is that ticket's territory, not this one's.S3— "Graph / workflow node execution",coverage: observed,language: [python, node]. Itsinterception_componentsays "lineage only" and its evidence is "lineage tests only", so itsobservedrests on topology-edge emission viareport_edge→ HTTP/topology/edges— not on the tool-call audit record this ticket measured.report_edgeresolves on all three shipped interceptors (it is one of my positive controls). I did not measure the LangGraph lineage path end to end, so I am not impeaching S3 — this is a lead, not a finding.No other row in that manifest asserts
observedfor an SDK hook-layer path (5observedrows total; the other three arehost_action,devtool_launchandplatform).Checklist
One pre-existing repo defect, deliberately untouched
pre-commit run --all-filesis red onremote/mainindependently of this work:scripts/check_contact_metadata.pyis notruff formatclean. I reverted the hook's reformat rather than absorbing an unrelated fix into a truthfulness PR. It does not block this PR — measured: no workflow in.github/workflows/runspre-commitor aruffgate (rg "pre-commit|precommit" .github/returns only aCODEOWNERSline and anechointype-check.yml; CI is tests, type-check, CodeQL and consistency checks). Filing separately.🤖 Generated with Claude Code
https://claude.ai/code/session_01XWLmA8FgULT9e6ntdCo1H2