[AAASM-5743] 🐛 (core): Tell a caller-supplied client the truth about its own checks - #371
Open
Chisanan232 wants to merge 3 commits into
Open
[AAASM-5743] 🐛 (core): Tell a caller-supplied client the truth about its own checks#371Chisanan232 wants to merge 3 commits into
Chisanan232 wants to merge 3 commits into
Conversation
A caller who supplies a real enforcing client and honestly declares `auditSink: "discarded"` was told at every startup that their policy checks route through the allow-all no-op and that a DENY cannot block a tool. Measured false for such a client: its `check` returned a genuine deny. `resolveAuditSink` collapses shipped-and-discarding with caller-supplied-and-declared-discarding into one value, and the warning was told only the mode — so it could not tell whose client it was describing. Provenance is now passed in, following the rule AAASM-5681 established twice: a claim that varies by a branch is expressed by that branch, not by wording near it. For a caller-supplied client the SDK now makes no claim about enforcement at all, which is the honest position — it did not build that client. AAASM-5743.
Forcing callerSupplied false reddens only the caller-supplied case; forcing it true reddens only the shipped ones, including the pre-existing AAASM-5681 per-mode assertion. Neither passes on the other coverage, which is what AAASM-5681 N9/N10 established for the mode branch and AAASM-5743 AC2 asks for here. The caller case drives its own `check` and observes the deny before judging the wording — a check that is never called cannot falsify a claim about what check does.
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
AAASM-5681 gave the enforcement clause a CHECK_CAPABLE_MODE arm. Coverage measurement put it at zero executions, and the reason is structural, not a missing test: reaching it needs `discarded` in napi-inprocess, and `createNativeClient` THROWS on an unloadable binding in that mode rather than falling back to the stub — the stub is only ever built for grpc-sidecar. So the client always has canRegister: true and declares "forwarded". The arm therefore held a claim-bearing sentence that no run could emit. It is removed, and the invariant that makes it unreachable is pinned instead: if a future change lets napi-inprocess fall back to a stub, that test reddens and the arm has to come back with it. Found by codecov flagging the patch, then measured rather than assumed (AAASM-5743).
|
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.
Summary
A caller who did exactly the right thing — supplied a real enforcing gateway client, and declared honestly that it drops audit — was told at every startup that their policy checks route through the allow-all no-op and that a DENY cannot block a tool call.
Measured false for such a client: its
checkreturns{denied: true, pending: false, reason: "MY OWN POLICY DENIES"}. Precisely inverted, for the most sophisticated adopter.Root cause
resolveAuditSinkcollapses two distinct states into one value:warnAuditEventsDiscardedreceived onlymode, so it could not tell whose client it was describing — and two of its sentences differ between the two.This is AAASM-5681's shape in a third direction. That ticket fixed the mode branch (twice — the first fix inverted the defect into
napi-inprocessbefore N9/N10 pinned it both ways). This is the provenance branch, which nobody had looked at.Fix
Provenance is passed in rather than re-derived, following the rule AAASM-5681 established the hard way: a claim that varies by a branch has to be expressed by that branch, not by wording placed near it.
For a caller-supplied client the SDK now makes no claim about enforcement at all — it did not build that client and does not know what its
checkdoes. The warning still fires, because the caller declared the events are dropped and that part is true; what changed is whose client it describes and what it refuses to assert.Acceptance criteria
AC1 — no statement false for a caller-supplied client. Both false sentences are asserted absent; the honest one is asserted present.
AC2 — pinned in both directions, neither passing on the other's coverage. Two mutations at the call site:
callerSuppliedforced false (fix reverted)a caller's client declaring "discarded" is told nothing that is false for itcallerSuppliedforced true (fix inverted)in auto, DOES say policy checks route through the allow-all no-op client(pre-existing, AAASM-5681) and the newa SHIPPED discarding client still gets the shipped wordingDisjoint. That is the N9/N10 property this AC asks for.
The caller-supplied case also drives its own
checkand observes the deny before judging the wording — acheckthat is never called cannot falsify a claim about whatcheckdoes.AC3 — the existing negative control still holds. No declaration still yields
"caller-supplied"and no warning.AC4 — the four per-mode assertions from AAASM-5681 pass unchanged. The
in autoone is load-bearing in the table above: it is one of the tests that catches the inverted mutation.Testing
tests/audit-sink-disposition.test.ts17/17. Full suite 744 passed, 2 skipped.pnpm lintclean,pnpm typecheckclean.Type of Change
Breaking Changes
warnAuditEventsDiscardedis module-private; the warning text changes for one previously-wrong case.Related Issues