Make a Policy say what it grants, instead of multiplying two lists - #762
Merged
Conversation
Policy held permitted_principal_ids and permitted_commands as two independent sets and evaluate checked membership in each separately, which grants every listed principal every listed command: the full N x M cross-product. The backdoor rulebook deployed at 2-BM yesterday says InHouseCautionDrafter may TruncateRun, though its code only ever calls AppendInferences. Nothing exercises that authority and the surface gate still bounds who can reach the rulebook at all, but a rulebook that overstates what it permits is the one artifact that must not. At full fleet the gap is roughly 20 principals x 35 commands = 700 nominal grants against about 48 real ones. It also blocks the watcher work: adding the run supervisor's AbortRun to a shared rulebook would hand AbortRun to the read-only status page feed too. Policy now holds `grants`, a frozenset of (principal, command) pairs. permitted_principal_ids and permitted_commands survive as derived properties, so every existing reader keeps working and neither can disagree with grants because both are computed from it. This is a no-op for every policy that exists. A PolicyDefined written before pairs carries the two lists and no grants key; from_stored cross-products them at the deserialization boundary, the same place a legacy event's missing surface_id already gets its default. Moving the multiplication from the check to the fold changes where it happens, never what it yields. Four tests pin that, one of them by exhaustive evaluation rather than by rebuilding the same cross-product the implementation builds, so it stays honest if the fold is rewritten. Two things the work turned up that review would not have: Reverting evaluate to the union check left all 682 trust tests green. Every existing case used one principal or a shared command list, where both designs agree; only a policy whose principals have DIFFERENT command sets can tell them apart. Three such tests now exist and the same mutation turns them red. Reading permitted_principal_ids first put an O(len(grants)) rebuild on every authorization. The authz-latency benchmark hung; a 64-principal policy was rebuilding a set from 2048 tuples per decision. evaluate now settles the permitted case with one hash lookup and pays the scan only when choosing which refusal to report. PolicyDefined.grants is overridden to drop:opaque in the export table. `tuple[tuple[UUID, str], ...]` is a collection of pairs, but the generator erases the outer collection and emits the positional-record rule, which _redact_tier1 then zips against the list of pairs: it raises for any length except two and mis-redacts at two. PartitionRule.partition_parameters has the identical latent defect and is why this is a limitation rather than a one-off. Fixing the generator and redactor changes what published records disclose for a second, unrelated aggregate and deserves its own reviewed diff. Dropping the field whole is fail-closed meanwhile, and a narrowing: an exported policy record now discloses nothing about its grants rather than pseudonymised principals with no commands beside them. POST /policies and the MCP tool accept either shape, exactly one required, refusing both-or-neither rather than picking a winner: the pair form grants far more than an equivalent-looking mapping, and an agent asking for one and receiving the other is the exact over-grant this removes. DefinePolicy.from_cross_product is the named constructor for callers that do mean everyone-gets-everything. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
An independent review was asked to refute the previous commit's claims rather than confirm them. It found one factual error and three weaknesses; this closes all four. The error was mine, in the commit message: "Three such tests now exist and the same mutation turns them red." Two do, not three. The third asserts an Allow, and a union check permits a superset of what a pair check permits, so no Allow-path test can ever distinguish them. Verified by reverting evaluate to the original two-check structure, which is a more faithful revert than the single-line one used before. The test is still worth keeping (a narrowing that denied what the policy permits would be its own bug) but it is not evidence of precision, and the PR description states two. Legacy folds no longer repeat a pair. A pre-pairs payload may repeat an entry in either list, since the old state folded both to frozensets and nothing upstream had reason to prevent it, and the cross-product then emitted the pair once per repetition. Folding to Policy collapsed that, so it was invisible; anything reading the EVENT first, an audit trail or a grant count, would have double-counted. The "exactly one grant shape" rule now lives in one place. It was duplicated between the REST route and the MCP tool, and each surface's contract tests exercise only its own copy, so the two could drift into disagreeing about a security-relevant question with nothing red. Only the RULE is shared: the route still raises inside a Pydantic validator, because that is what renders a 422 instead of a 500. Its unit tests pin the distinction the duplication kept obscuring, that ABSENT and EMPTY differ, and a mutation to truthiness (which would wave through an empty mapping supplied alongside empty lists) turns one of them red. make_policy_event now raises when handed both shapes instead of quietly preferring grants, matching what the two production surfaces do. A fixture that ignores half its arguments lets a test believe it seeded a policy it did not, which is worse in a helper than in production precisely because the fixture is the part being trusted. Co-Authored-By: Claude Sonnet 5 (1M context) <noreply@anthropic.com> Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Coverage reportClick to see where and how coverage changed
This report was generated by python-coverage-comment-action |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
xmap
added a commit
that referenced
this pull request
Sep 1, 2026
…tus page a name (#764) Groundwork for moving CORA's background runtimes onto the in-process door. Neither half changes what the running application does. ## The grant table `cora.api.in_process_grants.IN_PROCESS_GRANTS` names, per principal, the exact commands that principal issues through the back door: 20 principals, 28 distinct commands, 44 grant pairs. The cross-product a pre-#762 rulebook would have granted is 560, so 92% of it was fiction. It lives in the composition root, not in `cora.trust`, because `tach.toml` restricts Trust to infrastructure, shared, and its own aggregates: a table there importing agent-id constants from `cora.agent` does not build. The composition root is also where it belongs, since which agents a deployment runs is wiring rather than domain. Inert by construction, which is the load-bearing property. Nothing in `src/` imports it; only the fitness test (which AST-parses rather than imports) and `tools/gen_policy_grants.py`, which emits the `POST /policies` body an operator pipes into the API. If the running app read this table and defined a Policy from it, code would be granting itself its own authority, and a merged commit would be the only thing between an edit here and live authority. A human still posts it; nothing arms itself. The fitness test extends #750's registry, so every command name here must appear in the real wire surface. That closes the gap #750 opened but could not fill: it guards hand-typed command lists in the repo, while the two rulebooks actually armed at 2-BM were typed into a curl body and live only as events in a database CI cannot see. Generating the body from a CI-checked table is what turns "these names were correct on the day I checked" into a property. ## StatusPublisher `_status_push.py` acts as `SYSTEM_PRINCIPAL_ID`, which is also the fallback identity an unauthenticated HTTP request receives. Safe, since the back door demands a surface no HTTP request can claim, but the verdict log would read "nobody in particular" about a thousand times an hour. It now has a seeded Agent of its own, following `seed_run_witness.py`'s shape. `_status_push.py` itself is untouched: it still issues every read as SYSTEM until the call-site sweep. This commit creates the identity that sweep will use. ## Notes The eight synthetic names (`CampaignWatcherTick` and siblings) turned out NOT to need the allow-list the brief anticipated. They are `command_name=` values on `to_new_event(...)` envelopes, audit labels on a Decision record, and never reach `authorize()`. Verified at every site before omitting them. Two principals in the table, RunDebriefer and CautionDrafter, are overridable by env var and are overridden at 2-BM to deployment-specific Agent ids. The table names the source-code constants; a deployment that overrides them must grant the ids it actually runs. Co-authored-by: xmap <16776958+xmap@users.noreply.github.com> Co-authored-by: Claude Sonnet 5 (1M context) <noreply@anthropic.com>
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
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.
Part 1 of the authorization work planned after #761. It is a prerequisite for migrating the background watchers onto the in-process door, and it stands alone.
The problem
Policyheldpermitted_principal_idsandpermitted_commandsas two independent sets, andevaluatechecked membership in each separately. That grants every listed principal every listed command: the full N x M cross-product.Already live: the backdoor rulebook deployed at 2-BM on 2026-09-01 says
InHouseCautionDraftermayTruncateRun, though its code only ever callsAppendInferences. Not exploitable, since the surface gate still bounds who can reach that rulebook at all, but the rulebook grants strictly more than the code it governs. At full fleet the gap is roughly 20 principals x 35 commands = 700 nominal grants against about 48 real ones.It also blocked the watcher migration: adding the run supervisor's
AbortRunto a shared rulebook would have handedAbortRunto the read-only status page feed.What ships
Policy.grants, a frozenset of(principal_id, command_name)pairs.permitted_principal_idsandpermitted_commandssurvive as derived properties, so every existing reader keeps working and neither can disagree withgrantsbecause both are computed from it.This is a no-op for every policy that already exists. A
PolicyDefinedwritten before pairs carries the two lists and nograntskey;_grants_from_payloadcross-products them at the deserialization boundary, the same place a legacy event's missingsurface_idalready gets its default. Moving the multiplication from the check to the fold changes where it happens, never what it yields. Six tests pin that, one by exhaustive evaluation rather than by rebuilding the same cross-product the implementation builds, so it stays honest if the fold is ever rewritten.POST /policiesand thedefine_policyMCP tool accept either an exactgrantsmapping or the two cross-producted lists, exactly one required. Both refused rather than resolved: the pair form grants materially more than an equivalent-looking mapping, and an agent asking for one and receiving the other is the exact over-grant this removes.DefinePolicy.from_cross_productis the named constructor for callers who do mean everyone-gets-everything.Two things the work found that review would not have
The precision was untested. Reverting
evaluateto the union check left all 682 trust tests green. Every existing case used one principal or a shared command list, where both designs agree; only a policy whose principals have different command sets can tell them apart. Two tests now catch that mutation. (A third split-policy test asserts an Allow and cannot catch it, since a union check permits a superset of what a pair check permits. It is kept because a narrowing that denied what a policy permits would be its own bug, but it is not evidence of precision.)It was a real performance regression. The authz-latency benchmark hung:
evaluateread the derivedpermitted_principal_idson every authorization, rebuilding a set from 2,048 tuples per decision on a 64-principal policy.evaluatenow settles the permitted case with one hash lookup and pays the scan only when choosing which refusal to report, so the happy path is faster than before. That benchmark passes in 10.8s.A pre-existing export bug, surfaced but deliberately not fixed
PolicyDefined.grantsis force-overridden todrop:opaquein the disposition generator.tuple[tuple[UUID, str], ...]is a collection of pairs, but_classifyerases the outer collection and emits the single-positional-record rule;_redact_tier1then zips two dispositions against a list of N pairs, which raises for any N except 2 and silently mis-redacts at 2 (independently reproduced during review, including theOMITTEDsentinel leaking into the output list).PartitionRule.partition_parametershas the identical shape and the identical latent defect. Fixing the generator and redactor changes what published records disclose for a second, unrelated aggregate, and deserves its own reviewed diff rather than riding inside a Trust change.The cost is admitted, not glossed: an exported policy record now discloses nothing about its grants, where before it showed pseudonymised principals with no commands beside them. Fail-closed, and a narrowing.
Process
Designed and implemented by Opus, then handed to a Sonnet subagent with instructions to refute the claims above rather than confirm them. It found one factual error (I had written "three tests turn red" where two do) and three weaknesses, all closed in the second commit: legacy folds no longer emit duplicate pairs; the "exactly one grant shape" rule now lives in one shared place instead of being duplicated across the two surfaces that could drift; and the test helper raises on both-shapes like the production surfaces do. The review also independently reproduced the redaction bug above, confirming it is real rather than a convenient pretext for the override.
Verification
52,581 unit + architecture + contract, 1,367 integration, all green.
ruff,pyright,tach,make docs-buildclean. OpenAPI and the disposition table regenerated by their own generators, not hand-edited.Mutation-verified: the exact pair check, the legacy cross-product fold, both route-validator branches, and the shared shape rule's ABSENT-vs-EMPTY distinction each turn exactly their paired tests red.
Co-Authored-By: Claude Opus 5 (1M context) noreply@anthropic.com