LCM Teams v1 — per-principal authorization - #215
Open
100yenadmin wants to merge 51 commits into
Open
100yenadmin wants to merge 51 commits into
100yenadmin wants to merge 51 commits into
Conversation
added 30 commits
August 5, 2026 01:59
Freeze the consumer protocol against stephenschoettler#473's four named methods, extend the authority-path inventory beyond public tool handlers, and add the non-widening derived-scope vectors. - Protocol: authorize_operation / resolve_authorized_targets / authorize_stored_scope / audit_decision, so stephenschoettler#473 implements against fixed names. internal_reason is optional because the seam audits allow and deny alike. - Inventory: 15 -> 38 entries across 19 modules. The 23 new entries are non-tool paths that can bypass public tool handlers: writes, compaction, rollups, externalization, maintenance/reset/bootstrap/diagnostics, import, retrieval and vector/DAG expansion, session lifecycle, host callbacks. Every entry point is asserted to exist in its module's source, so the inventory cannot drift into fiction. There is no separate OS-cron entry point in this repo; the only cron-like path is engine.py's rollup maintenance scheduler, recorded as such. - Derived scope: summary, chunk, vector and rollup vectors, one subset and one widening attempt each, binding that a derived artifact cannot outgrow its source. - Ordering: freeze stephenschoettler#473's six steps, notably target resolution BEFORE counting and ranking. Counting or ranking unresolved candidates discloses existence and count for targets the principal may not be entitled to. - Denials: the public projection now emits a fixed key set. Blurring the reason is not enough on its own -- a detail map that varies by reason re-identifies what the reason field hid, and four reasons share the TARGET_NOT_FOUND_OR_FORBIDDEN bucket. - Decision and PublicDecision are frozen but held a mappingproxy, so hash() raised despite frozen=True advertising otherwise. Both now hash explicitly. Still additive and inert: no existing file is modified, nothing imports the package, stdlib only. Full suite: 35 failed, 2834 passed -- the failure set is identical to the baseline measured on pristine main @ 6b7dbb1 before any change.
Cross-model review found the previous fix incomplete. Emitting a fixed key set stopped the KEY SHAPE from varying by reason, but not the VALUES: SCOPE_FORBIDDEN carries a real context_id while TARGET_NOT_FOUND_OR_FORBIDDEN carries none, so an echoed context_id was a real ID for one and None for the other. Both blur to the same public reason, so the bucket member was still recoverable -- the same defect one level down. The public projection now carries no detail at all. Echoing anything requires proving it cannot co-vary with the reason, and that argument was got wrong twice; emitting nothing needs no argument. Also: - Normalize detail values to exact primitives. isinstance admits subclasses, and a subclass can set __hash__ = None or redefine it, making an otherwise ordinary Decision unhashable or making two equal Decisions hash differently. - Sanitize PublicDecision.detail in __post_init__. The type is exported, so a caller can build one from a mutable dict; without this its hash changes after construction. - The leak test now gives each reason DIFFERENT detail, mirroring what validate() really produces. Passing identical detail to every reason manufactures the uniformity the test is supposed to be checking, which is why the earlier version missed this. It also freezes the five members of the TARGET_NOT_FOUND_OR_FORBIDDEN bucket and asserts they project to literally equal objects. - Scope the ordering test to stephenschoettler#473 steps 2-6 and name it accordingly. Step 1 is validate(), a module function rather than a consumer method; it is now asserted as a precondition instead of being implied by the test's title. Full suite: 35 failed, 2835 passed -- failure set identical to the baseline.
Slice 1 of the stephenschoettler#473 seam. Adds access_policy/ implementing the consumer protocol frozen in stephenschoettler#482: a permissive TrustedOwnerPolicy, a default-deny FailClosedPolicy, and one resolve_policy resolution point. No call sites -- nothing outside the package imports it, so behaviour is unchanged. The 2x2 that resolve_policy encodes is the point of the slice, and the cell that matters is (carrier present, Teams off) -> TrustedOwnerPolicy: merely carrying a context must never enable Teams. Enabling is explicit configuration and nothing else. FailClosedPolicy's disclosure primitives RAISE rather than return a denying Decision. The protocol declares int/Sequence returns for those, and a Decision is truthy, so returning one meant `if policy.select_collection(...)` would proceed as though a real collection had come back -- a fail-closed policy failing open at the call site. isinstance() cannot catch this, because a runtime_checkable Protocol checks that methods exist, not what they return. Note that a VALID Teams context still resolves to the permissive policy. That is a placeholder, not enforcement: the policy that actually scopes a principal is the Teams adapter in stephenschoettler#483. Documented at the branch so it is not misread as Teams being enforced. Full suite: 35 failed, 2847 passed -- failure set identical to baseline.
Slice 2 of the stephenschoettler#473 seam. Hooks the three retrieval arms in retrieval_core.py -- run_knn, hydrate_chunk_hits, hydrate_semantic_nodes -- through the policy layer added in slice 1. Authorization bites at run_knn, before the KNN query is issued and before any scan bound is applied. That placement is deliberate: hydrate_* receives rows that are ALREADY ranked, so a hook there would satisfy "authorize before hydrate" while still disclosing existence and count for targets the principal may not be entitled to. stephenschoettler#473 requires target selection before ranking and limits in every retrieval arm. The hydrators additionally re-authorize stored scope before content disclosure, which is a separate requirement. Policies resolve through one documented seam, policy_for_engine(), with the engine attribute names as module constants. Hooks must not read the engine directly: a cascade of getattr fallbacks silently yields a permissive policy when an attribute is renamed, and nothing fails to signal it -- the unscoped fallback stephenschoettler#473 forbids. Note the asymmetry: an engine with no Teams wiring gets the permissive policy (correct default-off), but an engine with Teams enabled and no context accessor fails CLOSED rather than falling back. Tests cover default-off passthrough, ordering against the real run_knn call path, fail-closed disclosing nothing, connection closure on denial, stored scope preceding hydration, the enabled-but-unwired case, and an ast check that no arm resolves a policy outside the seam. Reintroducing the getattr cascade fails four of them. Full suite: 35 failed, 2854 passed -- failure set identical to baseline.
Slice 3 of the stephenschoettler#473 seam. Guards the write surfaces named in the authority inventory: durable message writes, ingest externalization, compaction, and rollup construction. Where the hook sits was decided by what actually carries a context. Only CompactionMixin has one -- LCMEngine mixes it in, so self IS the engine. MessageStore and RollupStore are constructed from a db_path alone, and build_day and externalize_ingest_payload are standalone functions, so those are guarded at their engine-side call sites rather than by adding an engine parameter to each. That keeps this diff additions-only: no existing signature, statement or branch changes. build_day's `scope` parameter is deliberately untouched. It is the rollup/DAG storage partition key used as a SQL predicate, not an access scope; wiring it to the access context would conflate a storage namespace with an authorization boundary and would enforce nothing while looking correct. The access scope is derived from the carrier context instead. Writes are the irreversible direction, so a denial must leave nothing behind: tests assert the store, sidecar, ledger and transaction are all clean after a refusal. stephenschoettler#473's non-widening rule is enforced for derived state -- a rollup or summary cannot end up with broader scope than its sources -- reusing the derivation fixtures shipped with stephenschoettler#482 rather than inventing vectors. Policy resolution goes through the documented seam only. An ast test asserts no write arm reads engine attributes directly, mirroring the read path's guard, because a getattr fallback chain silently yields a permissive policy when a name changes and nothing fails to signal it. Full suite: 35 failed, 2860 passed -- failure set identical to baseline.
Slice 4 of the stephenschoettler#473 seam. Guards session lifecycle, engine callbacks, auxiliary session state, reset, backups, the import script, and the consumers of the active-engine resolver. The rollup scheduler needed a different shape from every other hook, and it is the part worth reviewing carefully. _RollupMaintenanceScheduler is one process-wide worker shared by every engine, draining closures on a long-lived background thread. ContextVars do not reach it -- the retrieval hooks work partly because copy_context() carries scope into a freshly spawned worker, and a pre-existing thread inherits nothing -- and jobs from different principals share that single worker, so any ambient authorization state on it is cross-contamination rather than isolation. So the decision is captured at ENQUEUE time on the caller's thread and carried inside the job closure. _schedule_rollup_maintenance resolves the policy, context, decision and narrowing before scheduler.schedule; maintain() consumes the captured decision and never resolves a policy itself. A denied principal's job is not enqueued at all. The test that matters enqueues as A, switches to B, enqueues as B, then runs A's job and asserts A still ran under A's scope; making maintain() re-resolve at run time fails it. on_session_start authorizes before the hermes_home storage rebind rather than after, since rebinding storage for an unauthorized principal and then denying is too late to matter. Two inventory entries are recorded as needing no hook, with reasons rather than silence: doctor_guidance_for_checks is pure over caller-supplied check dicts and opens nothing, and inspect_lcm_schema_health reports only schema shape -- database path and sqlite_master table names -- with no row content. Adding hooks there would be ceremony that makes the real hooks harder to review. Against upstream main the whole seam remains additions-only: 347 insertions, zero deletions. Full suite: 35 failed, 2866 passed -- failure set identical to baseline.
Slice 5 of the stephenschoettler#473 seam, and the one that makes the inventory load-bearing rather than descriptive. stephenschoettler#473 asks that the inventory be enumerated "so new memory paths cannot silently omit the hook". That was not true until now: stephenschoettler#482 shipped 38 entries, slices 2-4 hooked those paths, and nothing connected the two. A contributor could add an lcm_* handler, satisfy stephenschoettler#482's inventory/manifest set-equality, ship it with no hook, and the suite stayed green. Both directions are now asserted structurally, via ast rather than by grepping for text a comment could satisfy: - inventory -> hook: each entry's declared sites must exist as real policy_for_engine call sites. A deleted hook fails naming the entry and site. - hook -> inventory: every policy_for_engine site must be claimed by an entry, so a guarded path nobody classified is caught too. That direction matters less for safety and more for honesty -- the code would be protected while the inventory misdescribed the system. - tools.py lcm_* handlers are discovered from source and compared to the inventory directly. This is the case stephenschoettler#482 could not see: its set-equality compares the inventory to plugin.yaml, so a handler added to source but never registered slipped past both. Hook-free entries are now structured data rather than prose. Each entry carries hook.required with its sites, or required=false with a non-empty reason, and the two exempt entries move to authority_requirement: none_required so the record no longer claims admin authority for a path it also says needs no hook. An exclusion list inside the test was the alternative and is worse: it drifts, it is invisible to anyone reading the inventory, and it lets a future omission hide behind already being listed. Verified by mutation rather than assumed: deleting a hook, adding an unclassified lcm_* handler, and blanking an exemption reason each fail, and each names what went wrong. Full suite: 35 failed, 2869 passed -- failure set identical to baseline.
A two-principal isolation smoke found the seam did not isolate, for a reason no unit test could see: handle_tool_call authorized self._session_id -- the caller's OWN session -- never the target in args, and never called resolve_authorized_targets at all. tools.py had no policy calls. So principal B invoking lcm_load_session(session_id=<A's session>) had the policy asked "may B read in B's session?", which is correctly allowed, and the handler then loaded A's session. The smoke observed B reading A's messages, row count, sidecar content and rollup content. No policy could have prevented this, including the one stephenschoettler#483 will supply: it was being asked the wrong question. handle_tool_call now derives the target from args via an explicit per-tool binding map, keeps the caller's identity alongside it so a policy sees both who and what, and applies the resolved target back onto the handler arguments so a body cannot proceed with a wider target than was authorized. Tools that genuinely act only on the caller's own scope are recorded target-free with a reason rather than omitted, and every binding was derived from the handler bodies rather than guessed. The completeness test could not see this either, which was the deeper problem: all fifteen tool entries declared the same dispatch site, so it proved a hook existed on the path rather than that the hook authorized the target. It now requires each tool to declare its target binding and asserts the map matches the lcm_* handlers discovered from source in both directions, so a new tool without a binding fails. The smoke is kept and made precise rather than green. Legs that fail only because a valid Teams context still resolves the permissive placeholder are xfail(strict=True) naming stephenschoettler#483, so they will flip to unexpected-pass and force a revisit when it lands. The positive control and the default-off control remain hard assertions: A must still reach A's own data, since a suite where everything is denied would prove nothing. Full suite: 35 failed, 2873 passed -- failure set identical to baseline.
Found by running the build against a clone of a real production store rather
than fixtures. The seam imported its own packages absolutely --
importlib.import_module("access_policy") in eight modules, and a bare
"from access_context import ..." -- while every other local module in this
plugin uses a relative import, and stock main reserves import_module for the
HOST (agent.context_engine) only.
That had two consequences, neither visible to the existing tests:
Loaded as the hermes_lcm package, which is how the plugin actually loads, the
absolute form produced a SECOND copy of the package. access_policy.errors
.AuthorizationRequiredError and hermes_lcm.access_policy.errors
.AuthorizationRequiredError were different class objects, so a caller writing
`except AuthorizationRequiredError` around a tool call would not catch what
the engine raised: an authorization denial would escape as an unhandled
exception. Confirmed by observing exactly that while driving the real store.
Worse, package import failed outright when the plugin directory was not on
sys.path -- ModuleNotFoundError: No module named 'access_policy' -- where
stock main imports cleanly. That is an import regression this branch
introduced.
Everything now imports relatively, matching the plugin's convention.
__init__.py resolves the policy helpers lazily instead, because it is loaded
both as a package and directly; a module-level relative import there breaks
the direct path, which is the same reason get_recall_policy already defers
its import. The one standalone script keeps an absolute import, since it
cannot use a relative one, but now uses the package-qualified name like every
other plugin import it makes.
The completeness test previously asserted the seam was imported "through
importlib"; it now requires a relative or package-qualified import and
rejects the bare-absolute form, so this cannot come back.
Also removes test_access_policy_imports_are_inert_outside_package_and_its_test.
Written in F1 when the package genuinely was inert, its premise died when F2
wired the package in -- and it did not fail then, because it scanned for
import statements while the hook modules pulled the package in through an
importlib call. It had been vacuous ever since.
Verified against a clone of a real 24,765-message production store: the read
battery is byte-identical between stock main and this branch with Teams off,
the bootstrap ladder applies identically and idempotently, and a two-principal
composition smoke denies the second principal on all five real sessions while
the owner still reads all five.
Full suite: 35 failed, 2872 passed -- failure set identical to baseline.
ruff flagged twelve unused imports, all fallout from the previous commit:
replacing importlib.import_module("access_policy") with a relative import
left `importlib` unused in eight modules, and rewriting the test imports to
the package-qualified form left four unused names behind.
No behaviour change. CI runs ruff 0.15.13; this is what it flagged.
stephenschoettler#473 binds that derived summaries, rollups, chunks and vectors cannot have broader scope than their sources. The seam implements that check and the contract ships the primitive and fixtures -- but nothing stored carried a scope, so the check compared the caller's context to itself and could never deny. No policy, however well written, can enforce non-widening on data that does not record who owns it. Adds a nullable access_scope column to the scope-bearing tables, using the schema ladder's existing add_column_if_missing, which already tolerates two processes racing the same ALTER. NULL means not stamped, which is what keeps this additive: an unmigrated store, or one that never enables Teams, reads exactly as it does today. Existing data is stamped when Teams is set up -- not at plain schema migration, so a deployment that never opts in is never touched. Attribution is the pre-Teams owner, resolved through session_id, which every scope-bearing leaf row already carries. The backfill is idempotent (it only visits unstamped rows) and resumable (it commits per batch). The verification instrument is part of this change rather than a follow-up, because a check that cannot fail is worse than no check. It reports stamped and unstamped counts per table as numbers rather than a verdict, enumerates the writers from source and fails naming any that insert without populating the column, and distinguishes "verified" from "nothing to verify" -- an empty store reports the latter, since "all rows stamped" is trivially true when there are no rows and reporting success there would be misleading. The column is called access_scope, deliberately. lcm_rollups, lcm_rollup_invalidations and lcm_rollup_state already have a scope column which is the rollup partition key: NOT NULL, always populated, and used as a SQL predicate. Naming this one scope collided with it, and because the partition key is never NULL, every rollup reported as stamped while carrying no access scope at all -- the backfill could not reach them and the instrument reported success, blind precisely where non-widening matters most, since rollups are the derived artifacts the rule is about. Those tables now carry a separate access_scope; their partition key is untouched and keeps its meaning. Not included: the Teams policy itself, role and catalog logic, and scope-partitioned rollup construction. That last one is a real behavioural change and is named as such in the design rather than smuggled in here. Full suite: 35 failed, 2,880 passed -- failure set identical to baseline.
The scope verification added in the previous commit ran the writer enumeration on every lcm_doctor call, and that enumeration rglobs and parses the whole source tree with ast. Measured at 355ms per call; the test suite invokes doctor often enough that it took the suite from 48s to 174s. Split by what each half actually describes. The counts and the non-vacuity sentinel describe THIS database -- cheap SQL over the open connection -- and stay in doctor unchanged, including the not-enabled semantics and the distinction between verified and nothing-to-verify. The writer guard describes the SOURCE TREE, which cannot change between doctor runs on a deployed install, so it moves into the test suite beside the authority inventory check. That is better rather than merely faster. As a test it fails the build the moment a writer inserts into a scope-bearing table without populating access_scope; as a doctor check it only ever warned whoever happened to run doctor. The guard is unchanged otherwise: it still discovers writers from source and names violations. Also keeps the scope_v1 marker from the same investigation, which short-circuits the eleven-table PRAGMA sweep once the columns are materialized. It was a real inefficiency, though it was not the one that cost the time. Suite: 174s before, 49s after -- parity with the pre-scope baseline. Failure set still identical to baseline.
The scope work passed engine._access_scope_for_storage_session directly into MessageStore and SummaryDAG. A bound method holds a strong reference to its instance, so that created a cycle -- engine -> store -> provider -> engine -- and neither store could be freed by refcounting. Their sqlite connections stayed open until a cyclic GC pass. CI caught it on Linux: five engines constructed and deleted leaked 21 file descriptors. The test is Linux-only, reading /proc/self/fd, so it skips on macOS and local runs never saw it. Measured rather than inferred. Before the scope work: 15 connections opened, 15 closed. After it: 15 opened, 12 closed. With gc disabled, which is what the test simulates by deleting without collecting: all 15 leaked, and an explicit gc.collect() closed all 15 -- the signature of a reference cycle rather than a missing close. Both stores now receive a weak-reference provider. Behaviour is identical while the engine is alive; once it is gone the provider returns None, which is the same path a Teams-off store already takes. With gc disabled: 15 opened, 15 closed, nothing leaked. Suite 47.7s, failure set identical to baseline.
All five confirmed at source before changing anything, each with a regression test that was mutation-checked -- broken deliberately, observed failing, restored. Tool dispatch authorized every lcm_* call as a read, while the inventory classifies lcm_compute and lcm_compile_evidence as write_scoped and the latter can persist a query view. A read-only principal passed a read gate and reached a write path. The operation is now derived from each tool's declared authority rather than hardcoded. clone_for_agent copied model, base url and api key but none of the Teams wiring, so a host that configured the prototype and then cloned per agent got a clone that read as Teams-off and resolved to the permissive policy -- enforcement silently lost on the runtime that actually serves requests. The clone now carries the wiring, keyed off the module constants so a rename cannot desynchronise them. Recall was classified target-free on the assumption that every arm re-authorizes its own corpus, and neither did: the FTS arm hardcoded session_scope='all' and the chunk arm reached run_chunk_knn with no gate before ranking. Both arms are now authorized before they query, which is what the classification always claimed. Database backup and rotation authorized as ordinary writes despite being owner-only in the inventory, and the import script wrote into a stamped Teams database without requiring a carrier, creating rows nobody owns. Suite 48.65s, failure set identical to baseline, ruff clean.
All thirteen confirmed at source first, each with a regression test that was mutation-checked -- broken deliberately, observed failing, restored. Delegation and narrowing were the substantive group, because everything downstream trusts what "narrower" means. A narrowing token restricting operations was documented but never enforced, so a context granted read and write and narrowed to read alone still passed a write check. A child could inherit a default write collection sitting outside the allowlist it had just been narrowed to. Collection tokens were not checked against the child's own bounds. And a subset proof compared declared grants rather than effective operations, while requiring the immediate parent -- so a grandchild that was narrower than its ancestor in every dimension was still rejected. Delegation is now transitive through the recorded chain. Rows created after Teams setup were being left unowned. The import script wrote messages and summary nodes with no owner into an already-stamped database, and a rollup rebuild seeded partitions the same way. NULL is reserved for pre-Teams rows that setup backfills, so new unowned rows put the database in a state setup could never repair. Both now carry the authorized owner at creation. The public carrier protocol advertised get_access_context() while the resolver only ever read get_lcm_access_context, so a host implementing the documented interface would have failed closed on every path. The resolver's name is canonical and the protocol now matches it. A malformed carrier -- a decoded dict rather than a context -- reached validate() and raised AttributeError instead of failing closed; it now returns context_invalid. Finally, a policy narrowing a target by omitting a key left the caller's wider value in place, hydration authorization never received the stored scope the row-level column exists to provide, and rollup rebuild authorized without binding to its partition. Suite 50.3s, failure set identical to baseline, ruff clean.
Six findings from the second automated review round. The main one: every AuthorizationRequiredError carried the INTERNAL denial reason rather than the public projection. The projection exists precisely so a caller cannot distinguish "you may not see this" from "this does not exist" -- raising the internal reason handed that distinction straight back through the exception, undoing the guard at roughly twenty call sites. All of them now raise decision.public().denial_reason. The rest: - Auxiliary-session, reset-state and session-reset paths asked for "write" authority when the operation they perform is owner-only. They now request owner_only and say so in expected_scope. - The slash-command doctor surface collected scope diagnostics with no admin gate, while the governed lcm_doctor tool had one. Added _authorize_doctor_command plus its inventory entry, so the two front doors carry the same authority requirement. - Target-free tool calls bound the target to the engine's own session rather than the foreground one, so a background caller authorized against the wrong scope. - The rollup scheduler ignored the partition key the policy resolved and kept using the requested scope. Two write-path tests asserted on "scope_mismatch"; the public projection reports target_not_found_or_forbidden, which is the point. Updated rather than loosened. tests/test_r2_authorization_findings.py covers all six. The leak test feeds two genuinely different internal reasons and asserts one public message -- an earlier version of this test passed identical detail to every reason and so manufactured the uniformity it was checking.
Third automated-review round. Two of these are defects in fixes from the
previous round, which is worth saying plainly.
Narrowing unioned instead of replacing. child_narrowing started as a copy
of every parent token, so narrowing operation:{read,write} down to read
left operation:write in place and the "read-only" child still passed a
write check. Each dimension the caller actually narrows -- named through
the typed argument or as an explicit token -- is now cleared first.
Cloning copied a BOUND carrier method. Where get_lcm_access_context is a
class-defined method, getattr(self, name) returns a method bound to the
prototype, so every agent clone read the prototype's context and could
authorize as the wrong principal. Only per-instance wiring is copied now,
and a callable bound to the prototype is rebound to the clone; a
class-defined accessor is left for the descriptor protocol.
The rest:
- A single revoked context id passed as a bare string satisfies the
declared Iterable[str], and frozenset() shredded it into characters --
so the revoked context matched nothing and was ALLOWED. Normalised
through the same helper required_scope already used.
- The lifetime stage checked only the far end of the window, so a
future-dated envelope was accepted before its validity began. Reusing
CONTEXT_EXPIRED rather than adding a reason: the taxonomy is frozen by
stephenschoettler#482 and the public projection does not distinguish the two ends.
- The recall FTS arm hard-coded session_scope="all" and only ADDED keys
the policy returned, so a policy narrowing the corpus to one session
still searched every session. Omitted keys are now removed, which
degrades to the tool's own "current" default -- the narrowest it has.
- on_session_start authorized only the destination session, while a
compression rollover reads the caller-supplied old_session_id's DAG and
can reassign its summary nodes. The source is now in the scope the
policy sees, before any rollover lookup.
- Caller-supplied pre-answer baseline_refs skipped authorization
entirely; the generated path goes through the gated lcm_recall, but a
supplied ref naming another principal's store_id had its exact span
read and injected into model context. Each ref is authorized now, with
its own inventory entry so the completeness test binds it.
- resolve_authorized_targets was declared -> Sequence[Any] while every
production caller reads it with .get, so a policy honouring the
annotation would crash every retrieval it allowed. Declared TargetScope,
and FailClosedPolicy returns an empty mapping instead of ().
Verified locally that repeated narrowing drops the superseded token, that
a bare-string revocation now denies identically to a set, that a
pre-issuance context is refused, and that the inventory reconciles in
both directions (41 entries, no unclassified hook sites).
Not fixed here: revalidating a queued rollup's authority at execution
time. That needs the catalog revisions threaded into validate(), which is
its own piece of work -- filed rather than half-done.
My own regression from the previous commit, caught by CI on all four
Python versions.
Callers pass baseline_refs as MAPPINGS -- {"exact_ref": "lcm:12:0-34",
"quote": ...} -- not as bare strings. The new gate matched the exact-ref
pattern against str(ref), which never matches a dict, so every supplied
ref was dropped and three pre-existing pre_llm_call tests changed
behaviour.
Two errors, not one. The shape was wrong, and non-matching items were
silently discarded rather than passed through. A ref that is not an
exact store span is not a disclosure this gate governs, so dropping it
does not close a hole -- it just empties the payload for callers whose
refs were never store references.
Now: the exact ref is read from "exact_ref" when the item is a mapping
and from the item itself otherwise; anything that is not an exact store
span passes through untouched; and authorized items are returned in
their ORIGINAL shape, since downstream reads the quote alongside the ref.
Covered by two tests over both shapes plus the pass-through case.
The gate itself was correct after the previous commit, but it lived in
__init__.py, and tests/conftest.py registers this package WITHOUT
executing __init__.py ("Don't exec the module" -- it tries to register
with ctx). So nothing defined at that level is reachable as an attribute
of the imported package, and both new tests failed on setattr.
An authorization gate that cannot be tested is the shape of defect this
branch keeps finding, so moving it beat working around it. It now lives
in preanswer_evidence next to _source_window -- the function that
actually reads the spans -- which is where it belonged anyway. The
inventory entry and hook site moved with it.
Verified by loading the package the way conftest does (registered but
unexecuted) and exercising all three paths: mappings and bare strings
survive when allowed, both are dropped when denied, and items that are
not exact store spans pass through untouched.
Phase 1 of the revised plan, and the precondition everything else in the chain depends on. Closes the widest hole on this branch. "Teams is enabled" lived only as an attribute on the engine object. Nothing wrote it down. The usual framing is "the flag is lost on restart", but the real failure needs no restart at all: enable_teams stamps historical rows in COMMITTED batches and calls mark_teams_enabled only after the whole backfill succeeds, so an enable that dies partway -- one unresolvable owner is enough -- leaves per-owner stamps behind with no recorded decision. Every later read treated that as "Teams is off" and handed a fully permissive policy real scoped data, with nothing in the logs and a green doctor. The decision is now recorded in the existing metadata table, under a key deliberately distinct from scope_v1 -- scope_v1 means "the access_scope columns exist", which ordinary bootstrap writes whether or not anyone enabled anything, so sharing that key would make every migrated store look enabled. It is written BEFORE the in-process flag, so a crash between the two lands on "enabled" rather than back in the hole. resolve_startup_teams_state reads it in _bind_storage, before any path can consult the flag, and distinguishes four states: never-enabled, enabled, disabled, and stamped-without-marker. That last one reports ENABLED, which is what makes it safe -- with no context accessor wired the seam already resolves enabled-but-unwired to FailClosedPolicy rather than to the permissive default, so the store refuses work until an operator finishes the enable or explicitly disables. No new mechanism was needed for that; the asymmetry was already there. disable_teams did not exist anywhere. It records the decision and retains every stamp: unstamping would destroy the attribution a later re-enable depends on and would be the only genuinely irreversible operation in this feature. That is also why "disabled" is a durable false rather than a cleared marker -- an operator turning it off and an enable dying partway leave identical stamps on disk and must resolve in opposite directions. Verified the whole state machine against a real database, including the negative control: a store with no stamps and no marker still resolves to TrustedOwnerPolicy, so default-off is not dragged into fail-closed. Still open for this phase: doctor is tri-state in the data but still reports pass for a stamped-without-marker store (scope_storage.py:564 returns not-enabled and tools.py:6616 maps it to pass). Next commit.
Completes Phase 1. The durable marker made the dangerous state detectable; this makes the doctor actually detect it. verify_scope_storage took teams_enabled from the CALLER, and after an aborted enable the caller's belief is wrong in precisely the dangerous direction. The status fell through to "not-enabled" with the message "NULL access_scope values remain legacy-compatible" -- on a store full of real per-owner stamps -- and tools.py mapped everything that was not fail or nothing-to-verify to pass. So the one check an operator would run to find this reported green. It now asks the database rather than the caller, and distinguishes the two states that leave identical stamps on disk: an explicit disable passes and says the stamps are retained, while stamps with no recorded decision report stamped-without-marker, which the doctor maps to fail. The test mirrors the tools.py mapping rather than asserting on the raw status, because the status string being right is not the property that matters -- an else-pass mapping is what made this invisible.
Caught by CI: test_lcm_doctor_tool_flags_header_only_database_schema asserted a header-only database has no tables and got ['metadata']. read_persisted_teams_enabled called ensure_metadata_table, and that read runs on the doctor path and on every storage bind — so merely asking whether Teams was enabled materialised the table, and the doctor then reported a schema shape it had just produced itself. Exactly the class of defect this branch keeps finding: a check that changes the thing it is checking. It now probes sqlite_master and returns None when the table is absent. The write path still creates it, which is where creating it belongs.
Phase 2. Phase 1 made a half-done enable detectable and fail closed; this stops it happening. preflight_teams_scope resolves every owner an enable would need and writes NOTHING -- not a row, not the access_scope columns, not the metadata table. It reports the sessions it cannot attribute, so the operator learns that while the store is still untouched rather than halfway through a backfill that commits per batch. There is a test on exactly that: a preflight that migrates the store it is inspecting is not a preflight, and this is the second time on this branch a read path was caught creating schema. It also handles the un-migrated case properly: with no access_scope column, EVERY row needs attribution, so the NULL predicate is dropped rather than silently reporting zero work. Nothing could answer for an unattributable session before -- the host resolver raised and that was the end of it, so one orphan session made an enable impossible. compose_scope_resolver adds an operator override map and a fallback owner, in that precedence: an explicit override wins over the host resolver (an operator correcting a bad attribution must not be overruled), and the fallback only answers what nothing else did. Both are operator input, not a guess made here. Per-table failure isolation. The loop had no exception handling, so the first unattributable session aborted the run and every LATER table was never STARTED -- not truncated, never started -- and the operator got one error with no way to tell which tables had been attempted. Each table now runs isolated and the result carries attempted/failures/ complete. This is also the ratified converge rule the desired-state contract already states: one failure must not abort the others. enable_teams refuses to record success when any table failed. Marking enabled there would put an enforcing policy over a partly-stamped store; leaving the marker unset leaves it stamped-without-marker, which Phase 1 fails closed on until the operator supplies the missing owners and re-runs. Verified the resume: the backfill is idempotent, so the second run stamps only what the first could not.
CI caught a real conflict with the pre-existing resume test, and the test was right. Swallowing the failure and returning complete=False meant a caller that ignored the return value would proceed as though the enable had worked -- which is a worse failure than the one I was fixing. Both properties now hold. Every table is attempted, so the report names what succeeded as well as what did not, and THEN ScopeBackfillIncompleteError is raised carrying that report. Isolation was always about not cancelling the tables after a failure; it was never about making the failure optional. That also removes the branch in enable_teams: the exception propagates out of setup_teams_scope, so control never reaches persist_teams_enabled and the marker stays unset -- leaving the store stamped-without-marker, which Phase 1 fails closed on until the operator supplies the missing owners and re-runs. Updated the pre-existing test to the stronger contract rather than relaxing it: it still asserts the interruption stops the first run and the committed batch survives, and now also asserts summary_nodes was attempted anyway and the report names the original error.
My added assertion expected summary_nodes in the attempted list, but that test builds a MessageStore, where messages is the only scope-bearing table present. The cross-table isolation property is pinned in tests/test_teams_preflight.py, which creates two tables precisely so the 'later tables still run' claim is actually observable.
Phase 3, and #206 was blocked on it: validate()'s revocation, ownership and lease stages compare a context against CURRENT state, and until something owns that state, wiring them would have produced yet another check that verifies nothing. New teams/ package: tenants, principals, collections, memberships, the three revisions AccessContextV1 already consumes, and an audit table. Provider-neutral by construction -- nothing here knows about ElectricSheep, gbrain or pipedream, and a host maps its own identity model on from its own repository. The catalog OWNS the revisions. That is the ratified narrow-shim carrier decision made concrete: the host authenticates a principal, tenant and session per turn and never sends a revision, so a context arriving with its own revision numbers proves only that someone wrote them into it. Nothing here may assume the host will supply them. Created only on enable, and after the backfill -- a store whose stamping failed does not acquire catalog tables it never gets to use. A store that never enables Teams ends up with no lcm_teams table at all, which is what keeps a single-user install untouched. Added lcm_teams to _KNOWN_FEATURE_TABLE_PREFIXES. Without it, classify_version_mismatch reads the catalog as tables no known build owns, concludes a newer build owns the database, and the repair path refuses to run -- so enabling Teams would quietly cost you doctor-repair. teams_catalog_exists and verify_teams_catalog read sqlite_master and create nothing, with a test on it. That is now the third read path on this branch that had to be stopped from materialising what it reports on, so it is worth stating as a habit rather than a fix. bump_revision checks the field name against the known set instead of interpolating it -- it reaches an f-string. Revoking is a revision bump, so this is the operation that makes an issued context stale, and the test asserts the other two counters do not move with it. Verified the package imports under the tests/conftest.py regime, which registers top-level modules by glob and would not have registered a package subdirectory if the __path__ were not set.
Phase 3a, unblocked by the catalog. resolve_policy called validate() with only `now`. Every comparison in the NOT_REVOKED stage is guarded by "is not None", so with nothing supplied they all short-circuited and a revoked context validated exactly like a current one. The stage had tests and they passed, because they called validate() directly with the arguments production never supplied -- the same shape as the doctor that reported pass on the dangerous state, and as the completeness test that cannot see a missing hook. policy_for_engine now reads the tenant's current policy/membership/ revocation counters from the catalog and passes them through. Bumping the revocation epoch invalidates every context issued before it, which is the operation an operator actually performs to cut someone off. Teams on, valid context, no catalog now FAILS CLOSED rather than falling through. Defaulting the revisions to zero would have been worse than useless: zero is a real revision value, so a context minted at zero would have validated against a catalog nobody could read. One test deliberately pins the DEFECT -- the same stale context passing without revisions and failing with them -- so a regression back to the inert state is visible instead of silent. Named, not left to be discovered: OWNERSHIP_CURRENT's generation check and LEASE_CURRENT are still inert, because the catalog does not yet track ownership generations or leases. SCOPE_PERMITTED and TARGET_RESOLUTION are per-OPERATION rather than per-context and belong in authorize_operation, which is the TeamsPolicy phase. Revocation is real now; the rest of enforcement is not, and the docstring says so rather than letting "we fed validate()" read as "Teams is enforced".
The positive control caught this, which is what it is for: my first version denied every operation in the isolation smoke, and a run where everything is denied proves nothing about isolation. Two absences look alike and are not alike. A store IS bound but has no catalog: that is an inconsistent Teams store which cannot say whether the context was revoked, and it fails closed. No store at all: scripts/import_lossless_claw passes a bare context carrier -- its `engine` parameter is literally typed `object | None` and exists only to hold a context -- so there is no catalog for it to be inconsistent with, and requiring one would break that entry point's contract. Revocation is NOT enforced on that path. Stated in the code, in a test name, and here, because an unenforced path that nobody wrote down is the thing this branch keeps finding. It gets a real authenticated surface in the connector phase. Also updated the isolation smoke's engine builder to create a catalog when it enables Teams. A real Teams store always has one -- enable_teams creates it -- so the fixture was modelling a store that cannot exist, and it predates the catalog. The gate's meaning is unchanged: the positive control still has to pass, and the isolation legs still have to hold.
The positive control went red a second time, one layer deeper: the catalog existed but sat at 0/0/0 while the smoke's contexts are minted at 1/1/1, so every operation read as revoked. Correct behaviour from the new check, wrong fixture. Added set_revisions. Provisioning genuinely needs it -- a tenant is created at whatever revisions its control plane has already issued contexts against, and bumping from zero would only reach those numbers by accident -- so this is the connector's API arriving early rather than a test affordance. The smoke now seeds the catalog from the context's own revision fields, which is what provisioning does. Verified both directions on a smoke-shaped context: it resolves permissive when the catalog agrees, and still fails closed the moment the epoch is bumped. A fixture that only proved the first half would have re-armed exactly the defect this phase closes.
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
These are what the catalog promised and did not have, and their absence is the
reason `TeamsPolicy` decides from the CONTEXT rather than the catalog: with no
way to ask "which collections may this principal read", a shared collection
could not be modelled at all, so the policy could only ever answer the private
case correctly.
`authorized_collections(principal, grant=...)` is the question the policy needs
to ask. It now has an answer, and a shared collection is expressible:
alice -> ('alice-own', 'company')
bob -> ('company',) # in the shared one, NOT in alice's own
## The subtlety that makes suspension work
Suspension is deliberately NON-destructive -- membership rows survive, exactly
as `disable_teams` keeps access_scope stamps, because attribution is what a
later re-provision and every audit answer depend on. So
`authorized_collections` checks STATUS FIRST. Reading memberships directly
would let a suspended principal keep every grant it had, with the suspension
recorded and completely inert -- the same shape as every other defect this
branch has had to fix.
Suspension and revocation both bump the revocation epoch, so a context issued
before them stops validating immediately rather than running to its natural
lease expiry. stephenschoettler#498 requires revocation to block the NEXT operation, not
eventually.
## Recorded rather than worked around
`principals.archive` cannot be honoured yet. The ratified contract is
"disable-then-archive, never destructive delete", but `status` is
CHECK-constrained to active/suspended, so archive needs a schema migration
rather than an accessor. A test pins the gap so it is not quietly forgotten or
silently mapped onto suspend, which would make "archived" and "suspended"
indistinguishable in the audit trail.
The 4b gate caught all three new tables as unclassified mutation targets --
they had no accessors before, so they had never been written to. Fourth time
it has done that. Classified as control-plane infra: they hold identity and
grant metadata, never memory content.
Suite 20 failed / 3300 passed (unchanged failure set). Battery 21/21,
default-off A/B 22/22, ruff clean.
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
The canonical deployment model is ONE gateway per customer serving MANY
profiles, and es.12 literalizes it. In that shape `os.environ["HERMES_HOME"]` is
whatever the process BOOTED with and never changes, while `get_hermes_home()`
follows the context-local override `gateway/run.py::_profile_runtime_scope`
installs per routed turn.
`_hermes_config_path()` read the env. So every routed profile loaded the BOOT
profile's LCM settings: compaction thresholds, `ignore_session_patterns`,
`assertions_enabled` -- and `database_path`, which decides which `lcm.db` this
plugin opens at all.
This is the same seam `hermes_cli/plugins.py::_plugin_manager_scope_key` keys
its per-profile PluginManager on. es.12 fixed the host side; config, skills,
memory and plugins all already follow the contextvar. LCM's own config was the
one that did not, and es.12 could not fix it from outside.
The env stays as the fallback so the plugin still works standalone, outside a
gateway, where `hermes_cli` may not be importable. A blank override falls back
too -- `Path("")` is `.`, and resolving a profile home to the cwd would be worse
than the bug.
## And the pinned-database_path branch no longer claims a rebind it did not do
`_rebind_storage_for_home` relabels the home and KEEPS the file when an explicit
`database_path` is set. That is correct for a single-profile deployment -- an
operator override outranks the profile -- and an isolation hazard under a
multiplexed gateway, where it collapses every routed profile onto one store.
It logged that at INFO as "LCM rebound Hermes home", which is precisely the one
thing it is not. An operator grepping for a mis-binding would have read that
line as confirmation the switch happened. It now warns, says the stores were NOT
switched, names the pinned path, and says what to unset.
Verified `database_path` is unset on every profile on the live customer box, so
the collapse case was latent rather than live. The per-profile settings were
wrong regardless.
Suite 20 failed / 3305 passed (unchanged failure set, +5 tests). Battery 21/21,
default-off A/B 22/22, ruff clean.
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
Owner
Author
Status — 2026-08-07. Deliberately NOT merging today.Teams is parked (G7, 08-12 checkpoint) and is not on the GA critical path. Merging a 97-commit It cannot reach a customer anywayDeployment is pin-gated: Branch topology (so nobody has to work it out again)
One claim correctedSay "behaviourally inert when off", not "byte-identical". Multiplexing: the bleed is fixed upstream of usUnder es.11 a real, no-race cross-profile bleed exists — process-global Gates, all green
|
…m-teams-v1 main just caught up to upstream (#220) after never having done so. Pulling it back in immediately so integration debt does not re-accumulate -- the last time this branch let it build, the merge was 52 hunks through the retrieval core and hid a leak in code that never conflicted. Expected to be near-trivial: the branch already contained both parents' content, and #220's own resolutions were taken FROM this branch.
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 96
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (5)
rollup_store.py (1)
505-527: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winCache the partition lookup instead of querying once per expanded target.
_expand_stale_targetsproduces up to three rows per requested day, all with the samescope. Both loops call_access_scope_for_partition(scope)for every row, so a day rebuild issues three identicalsummary_nodes/messagesscans. A batch of N days issues 3N.Resolve each distinct scope once before the loop.
⚡ Proposed change (`upsert_stale_many`)
affected = 0 + resolved: dict[str, str | None] = {} with self._write_transaction(): for period_kind, period_start, scope in rows: - access_scope = ( - str(authorized_access_scope) - if authorized_access_scope is not None - else self._access_scope_for_partition(scope) - ) + if authorized_access_scope is not None: + access_scope = str(authorized_access_scope) + else: + if scope not in resolved: + resolved[scope] = self._access_scope_for_partition(scope) + access_scope = resolved[scope]Also applies to: 569-596
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@rollup_store.py` around lines 505 - 527, Cache access scopes by distinct target scope before iterating in upsert_stale_many and the corresponding stale-target loop around _expand_stale_targets, resolving each scope with _access_scope_for_partition only once. Reuse the cached value for every expanded target while preserving the existing insert and update behavior.vector_store.py (1)
2980-3036: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winAdd Teams-scoped recall coverage tests.
Teams
lcm_recalluses the exact scan. An uncapped scan reportsfull; a configured cap or budget reportsbounded, notfull_approx. Add contract tests for both summary and chunk arms.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@vector_store.py` around lines 2980 - 3036, Extend the recall coverage contract tests for Teams-scoped lcm_recall to verify both summary and chunk arms use the exact scan: uncapped scans return coverage “full”, while configured caps or budgets return “bounded” and never “full_approx”.store.py (1)
1133-1140: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winAuthorize the finite-enumeration scan before reading rows.
getandget_batchare gated by their public retrieval callers. However,_finite_enumerationcallsscan_evidence_rowsdirectly atrequirements_compiler.py:1816. The scan reads the whole corpus withoutaccess_scopeand is reached fromcompile_preanswer_evidence. Pass the resolved scope to this path and apply it in the scan query before returning counts or evidence.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@store.py` around lines 1133 - 1140, Update the finite-enumeration flow reached by compile_preanswer_evidence so the resolved access_scope is passed into _finite_enumeration and then to scan_evidence_rows. Apply that scope in the scan query before producing counts or evidence, ensuring the full-corpus scan cannot read rows outside the caller’s authorization.tools.py (1)
436-459: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winThis path resolves a scope but never authorizes and never audits.
Every other read site added by this change follows the same order:
authorize_operation,audit_decision, raise on denial, thenresolve_authorized_targets. This site callsresolve_authorized_targetsalone.Two consequences:
- No audit record. The disclosure this block exists to scope produces no entry, unlike the chunk, summary, recall, and baseline-ref paths.
- No deny path. If the policy returns a mapping without
access_scope,_state_access_scopeisNoneandquery_assertion_stateruns unscoped. The comment above states the tool-boundary gate "has nothing to attach an owner to and allows", so nothing else stops it. The only thing standing between this and the described leak is that a resolver never omits the key.Add the
authorize_operation+audit_decisionpair, and deny when Teams is on and the policy returns noaccess_scope.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tools.py` around lines 436 - 459, Update the assertion-state read path around policy_for_engine and query_assertion_state to call authorize_operation, record the decision with audit_decision, and raise on denial before resolving targets. When Teams is enabled, explicitly deny if the resolved policy mapping lacks access_scope instead of passing None to query_assertion_state; preserve the authorized access_scope for allowed requests.engine.py (1)
1966-1986: 🔒 Security & Privacy | 🟠 Major | 🏗️ Heavy liftRe-validate authorization before rollup execution
captured_decisionis a caller-thread snapshot. The worker checks onlycaptured_decision.allowed, so it can write rollups after the catalog’smembership_revisionorrevocation_epochchanges.Add a worker-safe current-revision check and reject stale jobs. Do not resolve policy from worker-thread context.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@engine.py` around lines 1966 - 1986, The maintain worker currently trusts the caller-thread captured_decision without checking whether authorization state changed. In maintain, add a worker-safe current membership_revision and revocation_epoch validation against the captured decision, rejecting stale jobs before any rollup writes while preserving the existing denial handling; do not resolve policy or access worker-thread context to recompute authorization.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.github/workflows/ci.yml:
- Around line 5-18: Add workflow-level concurrency configuration to the CI
workflow, using the head branch or equivalent branch identifier as the group key
so push and pull_request runs for the same branch share a group while different
branches remain independent. Enable cancellation of in-progress runs, and verify
that a teams/** push with an open pull request leaves only one active run for
that branch.
In `@access_context/denials.py`:
- Around line 155-158: Update PublicDecision.__post_init__ to enforce the same
allowed/reason invariant as Decision: require a denial reason when allowed is
false and reject one when allowed is true. Normalize any provided denial_reason
to the DenialReason enum before storing it, while preserving the existing detail
sanitization.
In `@access_context/fixtures.py`:
- Around line 27-41: Update the fixture_root resolution logic around the
explicit root handling: when root is provided, resolve the requested path
(including FIXTURE_ROOT_NAME as currently required), return it only if it is a
directory, and otherwise raise FixtureCorpusNotFound immediately. Keep
package-relative candidate fallback limited to the case where root is None.
- Around line 75-76: Update the description validation around
payload["description"] to reject any embedded newline or carriage-return
characters, in addition to non-string and blank values. Preserve the existing
FixtureFormatError and one-line validation behavior for invalid descriptions.
In `@access_context/inventory.py`:
- Around line 24-50: Extend InventoryEntry with modeled hook and target_binding
fields, including validation for hook shape, required/site constraints, and
target argument references. Update from_mapping to require and construct hook
metadata plus target_binding when present, then ensure validate() enforces these
structures so consumers use the parsed fields instead of raw JSON.
- Around line 85-103: Update parse_manifest_tools to record the indentation of
provides_tools:, stop when a non-indented sibling key is encountered, and accept
list entries only at the expected child indentation. Parse only scalar item
values, ignoring nested lists and mapping entries such as “- name: lcm_grep”.
In `@access_context/model.py`:
- Around line 388-394: Update is_subset_of to compare narrowing permissions per
dimension rather than requiring the flat parent.narrowing token set to be a
subset of child.narrowing. Account for narrow() replacing tokens within the same
dimension while preserving the subset guarantee across unchanged dimensions,
including repeated narrowing through derive_child.
- Around line 210-212: Update the expiry validation in the method containing
child_expiry so the upper-bound check retains “expiry would outlive the parent,”
while child_expiry < self.issued_at raises a distinct message stating that the
expiry precedes issuance. Keep both boundary checks enforced.
In `@access_context/validation.py`:
- Around line 299-305: Update the child identifier generation in the narrowing
function around child_context_id and child_request_id so repeated derivations
from the same parent cannot reuse IDs. Preserve caller-provided IDs only when
they are validated as unique; otherwise generate collision-resistant identifiers
for each derivation instead of relying on len(chain) + 1.
In `@access_policy/fail_closed.py`:
- Around line 32-41: Update resolve_authorized_targets to authorize the
lcm_query_state operation before resolving or returning the target scope. Use
authorize_operation with the provided context and operation, and reject or
propagate a denial before returning {}, ensuring query_assertion_state cannot
produce unscoped results when access is denied.
In `@access_policy/resolution.py`:
- Line 34: Update the return annotations for both policy_for_engine and
resolve_policy to include TeamsPolicy alongside TrustedOwnerPolicy and
FailClosedPolicy, matching the policies those functions can return.
- Around line 71-101: Update _session_owner_for_engine so each table detects
whether a session has more than one distinct non-null access_scope, rather than
selecting an arbitrary row. When conflicting scopes exist, return an explicit
denied/ambiguous-owner result that TeamsPolicy cannot treat as unresolved and
allow; otherwise preserve the existing single-scope owner resolution and None
behavior for sessions without stamped rows.
In `@access_policy/teams_policy.py`:
- Around line 1-12: Update the module docstring in teams_policy.py to describe
this as the active enforcing TeamsPolicy used for valid Teams contexts, removing
the DRAFT and unfinished-policy claims. Retain a concise statement of the
remaining catalog-backed gaps, including membership, shared collections, and
delegation resolution.
- Around line 89-95: The owner-resolution flow must distinguish an unclaimed
session from a resolver failure and deny access on failures. Update
_target_owner in access_policy/teams_policy.py and the corresponding resolution
logic in access_policy/resolution.py so exceptions propagate as an error state
to the owner loop at Lines 156-158, which must deny rather than treat the
session as unclaimed; add a test in tests/test_teams_target_session.py covering
a session_owner callable that raises sqlite3.OperationalError and asserting
denial.
- Around line 151-152: In the policy evaluation loop containing the target and
effective checks, remove the unreachable `effective is None` condition and
continue skipping only when `target` is absent. Preserve the existing handling
of empty principals and all subsequent effective-value processing.
In `@assertion_store.py`:
- Around line 793-810: Propagate the resolved access_scope through the
assertion-grounding flow: update ground_evidence and reasoning._ground_one to
pass it into query_assertions and query_assertion_state, ensuring
assertion_id-based lcm_compute loads only same-principal assertions. Add a
regression test proving a foreign assertion cannot be grounded or used in the
computed response.
In `@command.py`:
- Around line 2772-2779: Update the inline comment above
_authorize_apply_mutation in rebuild_assertions to remove the claim that a
backup occurs before row changes. Keep only the accurate authorization-order
guidance, consistent with the helper’s documented no-backup behavior.
- Around line 5195-5198: Move the _authorize_doctor_command(engine) call to the
start of the doctor branch in handle_lcm_command, before dispatching any
subcommand. Remove the existing authorization from only the bare doctor path,
ensuring doctor clean, repair, source, retention, and repair schema-stamp all
pass through the same gate while preserving their existing dispatch behavior.
- Around line 2517-2544: Make the narrowing checks fail closed in the
authorization flow around policy.resolve_authorized_targets: reject any
authorized_scope result that is not a dict, and reject any source_scope or
derived_scope value that is not an AccessContextV1, using the existing
SCOPE_MISMATCH denial, audit, and AuthorizationRequiredError path. Preserve the
partition comparison and is_subset_of validation for valid values, ensuring
unverifiable policy results cannot authorize the write.
In `@compaction.py`:
- Around line 415-429: Fix the scope-containment validation so mixed-type
resolved scopes fail closed: in compaction.py lines 415-429, deny with
Decision.DenyReason.SCOPE_MISMATCH when either non-None scope is not an
AccessContextV1, while retaining is_subset_of validation for two valid contexts;
apply the same change to the rollup source_scope/derived_scope handling in
engine.py lines 1948-1965 before captured_decision is assigned.
In `@dag.py`:
- Around line 942-946: Correct the positional mapping in the row-construction
code around search_rank and access_scope so pre-scope FTS rows with 13 values
store row[12] as search_rank rather than access_scope, while preserving the
current-shape mapping. Alternatively, remove the inaccurate pre-scope claim and
ensure the indexing logic only supports the migrated row shape.
- Around line 160-177: Update SummaryDAG.search and _search_like to require
matching access_scope alongside session_id, and pass the current scope from both
lexical callers in tools.py. Add the same scope predicate to _get_session_node
before returning or expanding a node, ensuring lexical results cannot cross
access boundaries.
In `@db_bootstrap.py`:
- Around line 1494-1496: Update both lazy initializer call sites in
db_bootstrap.py: at lines 1494-1496, pass tables=("lcm_embedding_meta",
"lcm_embedding_vectors", "lcm_embedding_binary") to ensure_scope_columns; at
lines 941-943, pass tables=("lcm_rollups", "lcm_rollup_invalidations",
"lcm_rollup_state"). Keep the existing explicit add_column_if_missing calls and
chunk-path behavior unchanged.
- Around line 941-943: Update the ensure_scope_columns call in the bootstrap
migration to pass the rollup table family explicitly, matching the
targeted-repair invocation used by the chunk flow. This must bypass the scope_v1
early-return path so missing access_scope columns are repaired after lazy
rollup-table creation.
In `@docs/access-context-v1.md`:
- Around line 43-57: The runtime-status paragraph in “Standard single-user
compatibility” incorrectly claims there are no runtime call sites and that the
contract is default-off. Replace it with the actual disabled-mode behavior:
authorization remains inert when Teams is disabled, while schema changes and the
contract may still be present; preserve the existing carrier matrix and
enforcement-mode descriptions.
In `@engine.py`:
- Around line 4096-4164: Update _stored_access_scopes_for_targets so that, when
storage_teams_enabled(self) is true, a missing connection or any
sqlite3.OperationalError raises or otherwise propagates a denial instead of
returning an empty owner tuple. Preserve () only for Teams-disabled storage or
targets that resolve without an owner, and ensure callers such as the
target_access_scopes handling cannot authorize when ownership resolution fails.
- Around line 893-974: Add the same authorization gate used by protected
mutating methods to enable_teams, disable_teams, and the setup_teams alias,
requiring owner_only or admin access before any persistence, catalog, backfill,
or in-process enforcement state mutation. Reuse the existing authorization
mechanism and preserve current behavior after authorization succeeds.
- Around line 2836-2839: Update on_session_start so authorization is evaluated
against the storage selected by hermes_home: perform _rebind_storage_for_home
before resolving policy_for_engine(self), or explicitly authorize the target
home before switching. Ensure subsequent lifecycle-row and DAG mutations use the
policy derived from the newly bound store, while preserving the existing
boundary_reason and old_session_id handling.
- Around line 844-855: Update the connection-none branch in the Teams state
rebinding logic to clear any existing TEAMS_ENABLED_ATTR marker and set
_teams_state_reason to the appropriate no-connection reason before returning.
Preserve the existing resolve_startup_teams_state and mark_teams_enabled
behavior when a connection is available.
- Around line 4079-4094: Update _store_id_from_exact_ref to use
_EXACT_REF_RE.fullmatch instead of match, keeping the existing value
normalization and store_id extraction unchanged so malformed trailing content is
rejected consistently with authorize_supplied_baseline_refs.
- Around line 426-437: Update the LCM_TOOL_AUTHORITY_OPERATIONS comprehension to
handle unknown entry.authority_requirement values without raising KeyError
during import, mapping them to the most restrictive authority operation instead.
Preserve the existing mappings for all recognized requirements and apply the
fallback only when the lookup key is unknown.
- Around line 4200-4201: Update TeamsPolicy.authorize_operation to reject an
operation of "none" before evaluating scope restrictions, while preserving
normal authorization for recognized operations. Alternatively, validate the
operation against the supported allowlist and deny unrecognized values so
none_required tools cannot bypass per-principal authorization.
In `@maintenance.py`:
- Around line 47-58: Extract the repeated owner-only authorization and audit
logic into a shared helper, parameterized by the operation value used in
expected_scope, then call it from both functions. Build expected_scope with
required_scope included in the dict literal, while preserving the existing
denial behavior and public audit data.
In `@preanswer_evidence.py`:
- Around line 218-228: Update the non-matching branch in the refs authorization
loop to recognize mappings containing store_id without exact_ref as store-row
references. Apply the same preanswer_baseline_ref authorization scope to that
shape, or exclude it from the authorized output, while preserving passthrough
behavior for unrelated strings and mappings.
In `@reset_state.py`:
- Around line 55-70: The owner-only authorization gate in
_reset_session_scoped_runtime_state must not independently deny during storage
rebinding and abort on_session_start. Resolve the rebind authorization in
_rebind_storage_for_home, preserving the already-established authorization
context when it invokes _reset_profile_runtime_state and this reset path; keep
normal reset-state authorization enforced outside the rebind flow.
In `@retrieval_core.py`:
- Around line 274-281: Guard the return value of resolve_authorized_targets in
both retrieval_core.py:274-281 (run_knn) and retrieval_core.py:347-354
(run_chunk_knn) with isinstance(..., Mapping) before calling .get; raise
AuthorizationRequiredError when it is not a mapping, preserving the existing
authorized-scope extraction for valid mappings.
- Around line 626-641: Update the authorization loop around
authorize_stored_scope so denied nodes are skipped and processing continues with
the remaining ranked nodes instead of raising AuthorizationRequiredError.
Preserve policy.audit_decision for every authorization decision, and retain
authorized nodes so partially authorized results can proceed consistently with
preanswer_evidence.authorize_supplied_baseline_refs.
- Around line 587-615: Update the summary scope-loading logic around summary_ids
and scopes_by_id to cover every summary entry in ranked_rows, rather than
stopping at knn_limit. Remove the early collection cap or add an equivalent lazy
lookup for summary IDs missing from scopes_by_id, while preserving batching and
existing authorization behavior.
- Around line 485-506: Update the loop around authorize_stored_scope so entries
with row is None are skipped before authorization and auditing. Only call
policy.authorize_stored_scope for chunk IDs that have a corresponding row,
preserving authorization behavior for resolved chunks and allowing unresolved
IDs to follow the existing hydration skip behavior.
- Around line 450-484: Restrict the fallback in the OperationalError handler
around the lcm_chunk_meta scan to only missing-column errors for m.access_scope.
Inspect the exception message or equivalent SQLite error detail before retrying
the query without that column; otherwise re-raise the original error so the
existing progress-handler timeout is converted by the surrounding function into
TimeoutError.
In `@rollup_store.py`:
- Around line 120-136: Update _access_scope_for_partition to query distinct
non-NULL access_scope values across summary_nodes and messages, returning the
scope only when exactly one unique value exists; return None when no value or
multiple values are found, rather than selecting an arbitrary row.
In `@scope_storage.py`:
- Line 89: Update the _TEAMS_ENABLED_ATTRIBUTE constant to use the single
literal "lcm_teams_enabled" instead of concatenating strings, and add two blank
lines after the exception class body to preserve standard module formatting.
- Around line 554-560: Move the owner_for_session None validation before the
ensure_scope_columns(conn) call in the surrounding scope-storage function, while
retaining the existing batch_size validation and error messages. Ensure invalid
arguments raise before any schema mutation occurs.
- Around line 32-58: Update _ACCESS_SCOPE_TABLES to derive directly from the
exported SCOPE_BEARING_TABLES tuple instead of duplicating its table names,
preserving the existing exported symbol and access-scope behavior.
- Around line 306-311: Update the rename branch in ensure_scope_columns to
handle a concurrent process that already renamed scope: catch the expected
SQLite operational error from RENAME COLUMN, re-read the table’s columns, and
continue only once ACCESS_SCOPE_COLUMN is present. Keep the existing columns and
existing-table tracking updates consistent with the refreshed schema state.
- Around line 454-476: Update _backfill_session_table, _backfill_rollup_table,
and _backfill_joined_table to capture each executemany UPDATE’s affected-row
count and return when a batch updates zero rows. Keep the existing batch
selection, resolution, commit, and updated-count behavior for batches that make
progress, while preventing repeated selection of stalled rows.
In `@scripts/import_lossless_claw.py`:
- Around line 1360-1381: Extract the repeated authorization setup from
import_lossless_claw and import_jsonl_sessions into a shared helper that
performs policy/context lookup, teams_enabled and access_scope calculation,
expected-scope construction, and _authorize_import. Pass each entry point’s kind
and source-specific fields to the helper, preserving target_path and apply
handling and eliminating both duplicated preambles, including the NULL-stamp
gap.
- Around line 231-242: Update both import entry points that resolve access_scope
(around _target_has_stamped_scope and _access_scope_from_context) so a stamped
target rejects apply whenever the resolved scope is None, regardless of
teams_enabled. Stop suppressing scope resolution solely because Teams is
disabled, and preserve the existing CONTEXT_MISSING/CONTEXT_INVALID
authorization behavior while preventing NULL scopes from being written.
In `@teams/catalog.py`:
- Around line 357-400: Tenant scoping is not enforced across catalog writes,
reads, and connector authentication. In teams/catalog.py lines 357-400, scope
provision_principal and suspend_principal by (principal_id, tenant_id) and
derive revision bumps from the stored tenant; lines 431-471, add tenant_id to
lcm_teams_memberships, reject cross-tenant grant_membership, and tenant-scope
revoke_membership; lines 492-508, join authorized_collections through
lcm_teams_collections using the principal tenant. In teams/connector.py lines
174-192, update CredentialCheck/_authenticate or execute so authentication
validates the credential against request.tenant_id rather than trusting the
caller-provided tenant.
- Around line 215-232: Remove the conn.commit() call from record_audit_event so
appending an audit row does not commit caller-owned transactions. Update
TeamsConnector.execute’s sqlite3.Error failure path to roll back the failed
operation before invoking _audit, then commit only the standalone audit record
while preserving the existing error propagation.
- Around line 442-447: Update grant_membership to reject malformed or unknown
grant values before persisting them: validate each normalized grant against the
established closed grant vocabulary and reject any value containing a comma.
Preserve canonical sorting and storage for valid grants, ensuring
read_memberships cannot decode one input grant into multiple permissions.
- Around line 304-316: Update the revision bump function around the validated
field allowlist and UPDATE statement to use UPDATE ... RETURNING {field},
capture the value from that statement, and return it after commit instead of
calling read_revisions(). Ensure supported SQLite versions provide RETURNING, or
add the required minimum-version enforcement/compatible fallback, and keep the
allowlist synchronized with CatalogRevisions.
In `@teams/connector.py`:
- Around line 110-115: Update ConnectorRequest dispatch in execute so capability
handlers receive a request without the credential field, while authentication
can still use the original credential beforehand. Ensure the sanitized request
preserves the remaining request data and is the object passed to the handler and
subsequent result handling.
- Around line 196-205: Update _prior so sqlite3.Error is surfaced as an
UNAVAILABLE failure instead of returning None, ensuring ledger read errors fail
closed and prevent dispatching the effect. Update execute to audit this refusal
while preserving normal “request not found” handling and allowing the control
plane to retry.
- Around line 307-317: Update execute and the analogous replay path around line
292 to catch unexpected handler or result-decoding exceptions after the existing
ConnectorError and sqlite3.Error handlers. Audit the request as failed with an
appropriate failure classification, convert the unexpected exception to
ConnectorError, and ensure the request is recorded in the ledger so retries with
the same request_id do not re-run the effect.
- Around line 231-245: Update TeamsConnector._audit to pass the public denial
projection to catalog.record_audit_event instead of failure.value, reusing the
existing public_denial_reason mapping. Because that helper is currently defined
after TeamsConnector, move public_denial_reason and its _PUBLIC_DENIAL mapping
above the class or resolve it lazily inside _audit, while preserving None for
non-denied requests.
In `@tests/fixtures/access_context_v1/delegation/redelegation-chain-3-deep.json`:
- Line 1: The fixture’s expected chain, narrowing, and depth values are unused
by the validation test. Update tests/test_access_context_v1_validation.py around
derive_child to load and assert the fixture’s expected fields, or remove those
unused keys from the fixture; ensure the chosen approach prevents the declared
child IDs and accumulated narrowing from drifting without test coverage.
In `@tests/fixtures/access_context_v1/delegation/widen-collections.json`:
- Line 1: Update the candidate object in the widen-collections fixture so
default_write_collection_id remains "collection-a", matching the parent while
the candidate narrowing still adds "collection-b". Keep the collection allowlist
widening unchanged, isolating that boundary for the is_subset_of check.
In `@tests/fixtures/access_context_v1/positive/delegated-child.json`:
- Line 1: Update the positive delegated-child fixture’s expiry contract so
normal validation remains valid after August 7, 2026: either replace expires_at
with a durable future timestamp or define an explicit fixed validation time for
this fixture. Preserve the existing context fields and positive-fixture
semantics.
In `@tests/fixtures/access_context_v1/positive/human.json`:
- Line 1: Update the positive access-context fixtures, including the human
fixture’s expires_at value, so their validity does not depend on a fixed date in
the past; use the repository’s established dynamic or sufficiently future-dated
expiry convention while preserving all required AccessContextV1 fields and exact
key names.
In `@tests/fixtures/access_context_v1/positive/service.json`:
- Line 1: Update the service fixture’s expires_at value to use the repository’s
established non-expiring or dynamically generated expiry convention, while
preserving actor_type "service" and the existing field set. Verify validation
accepts authenticated_transport "mTLS" using the expected case handling or
canonical allowlist value.
In `@tests/fixtures/access_context_v1/README.md`:
- Around line 1-2: Add a blank line immediately after the “AccessContextV1
shared corpus” Markdown heading in README.md, preserving the existing text and
formatting below it.
In `@tests/fixtures/access_context_v1/revocation/lease-generation-change.json`:
- Line 1: Update the expires_at value in the lease-generation-change fixture to
a timestamp safely beyond the validation test’s clock, so validation reaches
lease staleness and preserves the expected lease_stale denial for
current_lease_generation 5.
In `@tests/fixtures/access_context_v1/revocation/membership-revision-bump.json`:
- Line 1: The fixture’s expiration timestamp is coupled to a fixed date and
should use the consolidated revocation-fixture shelf-life convention. Update the
expiration handling in the membership-revision-bump fixture while preserving
membership_revision 4, current_membership_revision 5, and the context_revoked
expectation.
In
`@tests/fixtures/access_context_v1/revocation/ownership-generation-change.json`:
- Line 1: Update the expires_at value in the ownership-generation revocation
fixture to use the consolidated test expiry timestamp, while preserving the
ownership_generation mismatch and expected ownership_changed denial.
In `@tests/fixtures/access_context_v1/revocation/revocation-epoch-bump.json`:
- Line 1: Keep the revocation vector and expected context_revoked behavior
unchanged in this fixture, but update the context.expires_at value to use the
shared stable test expiry defined by the consolidated expiry fix, avoiding
dependence on the current clock.
In `@tests/test_access_context_v1_fixtures.py`:
- Line 35: Update the imports in the test fixture to import AccessContextV1 and
is_subset_of from the public hermes_lcm.access_context module, matching the
existing package import and avoiding the standalone access_context namespace.
In `@tests/test_access_context_v1_model.py`:
- Line 22: Anchor all fixture paths to the test file location instead of the
process working directory. Update _human() in
tests/test_access_context_v1_model.py#L22-L22 and the fixture references in
tests/test_access_context_v1_validation.py#L59-L59, `#L92-L92`, `#L115-L115`,
`#L137-L137`, `#L283-L284`, and `#L366-L366` to use a shared helper rooted at
Path(__file__).parent / "fixtures" / "access_context_v1".
In `@tests/test_access_context_v1_validation.py`:
- Around line 265-266: Update the mixed-key hashing test around
PublicDecision.__hash__ to use a non-string object whose str() value is an
allowlisted detail key, alongside the existing string key. Ensure the resulting
details retain both keys so sorting must compare mixed key types, and keep the
hash invocation as the assertion target.
- Around line 292-301: Remove the unused run_pair_39 function and its misleading
Python 3.9 comment. Keep the asyncio.to_thread-based run_pair implementation and
update the assertion to call asyncio.run(run_pair()), preserving the existing
expected context IDs.
- Around line 304-306: Update
test_validation_module_has_no_mutable_context_store to import the validation
module through its qualified package path, hermes_lcm.access_context.validation,
so the test inspects the same module object used by the application.
In `@tests/test_lcm_authorization_completeness.py`:
- Around line 319-332: Harden _source_files by excluding non-.venv
virtual-environment directories such as venv, env, and .tox, or otherwise
restrict discovery to the repository’s package tree. Preserve the existing
exclusions and ensure installed third-party modules are not included in the
returned Python source files.
In `@tests/test_lcm_authorization_read_path.py`:
- Around line 468-488: Harden the AST guard in the arm inspection loop so every
getattr call with a non-constant second argument is recorded as a forbidden
direct engine read, causing the existing direct_engine_reads assertion to fail.
Preserve the current forbidden-attribute handling for constant arguments and use
the visible getattr detection logic rather than adding a separate runtime check.
In `@tests/test_lcm_authorization_sessions.py`:
- Around line 155-174: Add a positive control to
test_rollup_denial_is_not_enqueued using the same carrier and scheduler: switch
policy_for_engine to a permissive policy, invoke _schedule_rollup_maintenance,
and assert the job is enqueued. Restore the denying policy and clear scheduled
before retaining the existing denial assertion, ensuring the test proves the
setup works rather than only relying on exception suppression.
In `@tests/test_lcm_authorization_write_path.py`:
- Around line 146-172: Move the `_engine(tmp_path, rollups=True)` construction
inside the existing try/finally in
`test_non_widening_rollup_scope_refuses_fixture_widening`, before the
monkeypatch calls, so any setup failure is still followed by
`engine.shutdown()`.
- Around line 235-249: Update
test_write_arms_resolve_only_through_documented_policy_seam to inspect the
parsed AST rather than raw source text when forbidding lcm_teams_enabled and
get_lcm_access_context. Reuse the AST-walk pattern from
test_lcm_authorization_read_path, checking attribute reads and getattr calls
while retaining the policy_for_engine assertion; eliminate the duplicate
read_text call and preserve useful node-based failure locations.
In `@tests/test_profile_scoped_config.py`:
- Around line 82-97: Update the test test_a_raising_get_hermes_home_falls_back
and the corresponding _hermes_config_path behavior so an available
get_hermes_home() that raises propagates the error instead of falling back to
HERMES_HOME. Retain the environment fallback only when hermes_cli.config cannot
be imported or is unavailable.
In `@tests/test_r3_authorization_findings.py`:
- Around line 140-156: The two tests in tests/test_r3_authorization_findings.py
at lines 140-156 and 211-241 must shut down every real LCMEngine they create.
Wrap each test body after engine construction in try/finally and call
engine.shutdown() in finally, or introduce a module-level fixture that yields
the engine and performs the same cleanup for both sites; preserve the existing
test behavior and ensure parametrized cases each release their engine.
In `@tests/test_rollup_store.py`:
- Around line 424-434: Strengthen
test_upsert_stale_many_carries_authorized_scope_for_empty_partition by querying
period_kind alongside access_scope and asserting the returned period kinds are
exactly day, week, and month, while retaining the authorized-scope assertion.
In `@tests/test_scope_storage.py`:
- Around line 40-66: Add focused tests in the existing teams preflight test
suite for setup/backfill validation when owner_for_session is None and when
batch_size is less than 1. Assert each invalid input is rejected with the
expected validation error, while preserving the existing overrides and
fallback_owner coverage.
In `@tests/test_teams_apply_mode_gates.py`:
- Around line 110-118: The test must verify runtime ordering rather than relying
on source-text positions. Add a parametrized behavioral test for
_embedding_backfill_summary_text and _chunk_backfill_text that configures a
denying policy, replaces VectorStore with a failing/recording constructor,
invokes each handler in apply mode using its actual signature, and asserts
AuthorizationRequiredError is raised without constructing the writable store;
replace or supplement the brittle source.index check so missing implementation
details produce clear test failures.
In `@tests/test_teams_assertion_scope.py`:
- Around line 127-136: Add a behavioral isolation test alongside
test_relations_require_every_endpoint_to_belong_to_the_principal: seed a
cross-scope relation between acorn and carus and a relation whose endpoints are
both in acorn, then assert query_relations(access_scope="acorn") excludes the
cross-scope relation while returning the internal one. Retain the existing
source-structure check only as a secondary guard, if desired.
In `@tests/test_teams_host_surface.py`:
- Around line 154-162: Document why clone_for_agent is excluded from
test_host_only_methods_are_not_reachable_from_a_tool_or_command, either in that
docstring or the HOST_ONLY definition comment. State the specific safety
argument and preserve the existing parametrization and test behavior.
- Around line 107-127: Update
test_the_classification_does_not_drift_from_reality to collect the callable
method names defined on LCMEngine while traversing its AST, then assert every
name in GATED exists in that collected set before checking seam usage. Preserve
the existing policy_for_engine assertion for present gated methods.
In `@tests/test_teams_lifecycle.py`:
- Around line 158-182: Remove the duplicated _doctor_status mapping from the
tests and reuse the production status-mapping helper used by lcm_doctor,
exporting that helper from its owning module if necessary. Update
test_doctor_is_not_green_on_an_aborted_enable to evaluate the result through the
shared production mapping so changes such as stamped-without-marker no longer
mapping to fail are reflected by the tests.
In `@tests/test_teams_owner_predicate.py`:
- Around line 72-87: Strengthen the vector-helper tests around
_bounded_candidate_ids and _bounded_chunk_candidate_ids with execution-level
isolation coverage: seed rows belonging to two principals, invoke each helper
for one principal with a limit below the total row count, and assert every
returned ID belongs only to that principal. Retain or replace the fragile
inspect.getsource ordering checks as appropriate, but ensure the tests validate
runtime behavior rather than only source-text ordering.
- Around line 57-69: Update test_narrowing_does_not_ride_on_the_collection_id to
assert that resolved["source"] exactly equals the input stored-row source value
"openclaw-lcm:agent:acorn:uuid", while retaining the existing access_scope
assertion.
In `@tests/test_teams_preflight.py`:
- Around line 70-83: Strengthen
test_preflight_writes_nothing_and_creates_nothing to snapshot the complete
SQLite schema and relevant data before calling preflight_teams_scope, then
assert they are unchanged afterward. Include metadata contents, summary_nodes
contents, and all schema objects in addition to the existing messages checks,
preserving the contract that preflight creates no scope columns, metadata table,
or other database objects.
In `@tests/test_teams_real_policy_gates.py`:
- Around line 98-102: The gate drift guard in
tests/test_teams_real_policy_gates.py:98-102 must detect required_scope values
assigned through subscripts, such as expected_scope["required_scope"] =
operation, and its AST walk around lines 161-169 must inspect inline dict
arguments passed to authorize_operation. Replace the hardcoded function list and
kind regex in tests/test_teams_real_policy_denies.py:76-101 with an AST walk
covering the entire command module, while preserving the existing policy-drift
detection behavior.
In `@tests/test_teams_revocation.py`:
- Around line 164-167: Strengthen test_teams_off_never_consults_the_catalog by
seeding the fixture store with catalog data and advancing its epoch before
calling policy_for_engine. Keep teams_enabled=False, then assert
TrustedOwnerPolicy so the test distinguishes the Teams-off early return from
catalog consultation, which would otherwise yield FailClosedPolicy.
In `@tests/test_teams_target_session.py`:
- Around line 116-127: Extend test_every_target_key_resolves_by_owner with a
resolver case that raises an exception for the target key, then assert the write
is denied rather than allowed. Ensure the test exercises
TeamsPolicy._target_owner’s exception path and pins secure fail-closed behavior
for all parametrized target keys.
In `@tests/test_teams_target_shapes.py`:
- Around line 84-89: Update the rationale and classification logic for the
lcm_query_state entries in known_unresolvable: acknowledge that
assertion_store.query_assertions/query_relations and
assertion_state.query_assertion_state now apply access_scope through the owning
source message, while preserving subject_key and scope_key as unresolved because
they do not identify an owner-stamped row. Ensure the structural gate records
this mechanism rather than marking the allow as UNJUSTIFIED.
In `@tests/test_two_principal_isolation_smoke.py`:
- Line 43: Update PERMISSIVE in the isolation smoke test to describe the
observed leak without referencing the obsolete access_policy/resolution.py
location or permissive placeholder. Remove the stale “until `#483`” placeholder
comments from _TargetBoundaryPolicy and the pre-existing Teams-policy legs near
_b_leaks, leaving the surrounding test behavior unchanged.
- Around line 697-739: Update test_store_wide_backup_is_admin_only_under_teams
to assert the specific AuthorizationRequiredError type in both Teams-enabled
backup_database calls instead of pytest.raises(Exception). Remove the
substring-based "authorize" assertion while preserving the checks for both
principals and the Teams-disabled success path.
In `@tools.py`:
- Around line 4822-4850: Capture access_scope from the top-level resolved
mapping before authorized_scope is replaced with target_scope, matching the
convention used by engine.py. Apply that captured owner predicate to fts_args
after the target-scope normalization, while preserving the existing
target-dimension omission handling.
- Around line 7321-7334: Update the status mapping in the scope_storage check to
explicitly enumerate the known passing statuses instead of using a catch-all
else "pass". Preserve the existing fail mappings for "fail" and
"stamped-without-marker", retain "nothing-to-verify" as "warn", and map any
unrecognized status to "warn".
---
Outside diff comments:
In `@engine.py`:
- Around line 1966-1986: The maintain worker currently trusts the caller-thread
captured_decision without checking whether authorization state changed. In
maintain, add a worker-safe current membership_revision and revocation_epoch
validation against the captured decision, rejecting stale jobs before any rollup
writes while preserving the existing denial handling; do not resolve policy or
access worker-thread context to recompute authorization.
In `@rollup_store.py`:
- Around line 505-527: Cache access scopes by distinct target scope before
iterating in upsert_stale_many and the corresponding stale-target loop around
_expand_stale_targets, resolving each scope with _access_scope_for_partition
only once. Reuse the cached value for every expanded target while preserving the
existing insert and update behavior.
In `@store.py`:
- Around line 1133-1140: Update the finite-enumeration flow reached by
compile_preanswer_evidence so the resolved access_scope is passed into
_finite_enumeration and then to scan_evidence_rows. Apply that scope in the scan
query before producing counts or evidence, ensuring the full-corpus scan cannot
read rows outside the caller’s authorization.
In `@tools.py`:
- Around line 436-459: Update the assertion-state read path around
policy_for_engine and query_assertion_state to call authorize_operation, record
the decision with audit_decision, and raise on denial before resolving targets.
When Teams is enabled, explicitly deny if the resolved policy mapping lacks
access_scope instead of passing None to query_assertion_state; preserve the
authorized access_scope for allowed requests.
In `@vector_store.py`:
- Around line 2980-3036: Extend the recall coverage contract tests for
Teams-scoped lcm_recall to verify both summary and chunk arms use the exact
scan: uncapped scans return coverage “full”, while configured caps or budgets
return “bounded” and never “full_approx”.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 30623044-1126-4550-a5a0-95104ff44f79
📒 Files selected for processing (122)
.github/workflows/ci.yml__init__.pyaccess_context/__init__.pyaccess_context/denials.pyaccess_context/fixtures.pyaccess_context/inventory.jsonaccess_context/inventory.pyaccess_context/model.pyaccess_context/protocols.pyaccess_context/validation.pyaccess_policy/__init__.pyaccess_policy/errors.pyaccess_policy/fail_closed.pyaccess_policy/resolution.pyaccess_policy/teams_policy.pyaccess_policy/trusted_owner.pyassertion_state.pyassertion_store.pyaux_session.pycommand.pycompaction.pyconfig.pydag.pydb_bootstrap.pydocs/access-context-v1.mdengine.pymaintenance.pypreanswer_evidence.pyreset_state.pyretrieval_core.pyrollup_store.pyscope_storage.pyscripts/import_lossless_claw.pystore.pyteams/__init__.pyteams/catalog.pyteams/connector.pytests/data/lcm_mutation_targets.jsontests/fixtures/access_context_v1/README.mdtests/fixtures/access_context_v1/delegation/redelegation-chain-3-deep.jsontests/fixtures/access_context_v1/delegation/subset-proof.jsontests/fixtures/access_context_v1/delegation/widen-audience.jsontests/fixtures/access_context_v1/delegation/widen-collections.jsontests/fixtures/access_context_v1/delegation/widen-expiry.jsontests/fixtures/access_context_v1/delegation/widen-lease-generation.jsontests/fixtures/access_context_v1/delegation/widen-membership-revision.jsontests/fixtures/access_context_v1/delegation/widen-operations.jsontests/fixtures/access_context_v1/delegation/widen-ownership-generation.jsontests/fixtures/access_context_v1/delegation/widen-policy-revision.jsontests/fixtures/access_context_v1/delegation/widen-profile.jsontests/fixtures/access_context_v1/delegation/widen-revocation-epoch.jsontests/fixtures/access_context_v1/delegation/widen-session.jsontests/fixtures/access_context_v1/derivation/chunk-negative.jsontests/fixtures/access_context_v1/derivation/chunk-positive.jsontests/fixtures/access_context_v1/derivation/rollup-negative.jsontests/fixtures/access_context_v1/derivation/rollup-positive.jsontests/fixtures/access_context_v1/derivation/summary-negative.jsontests/fixtures/access_context_v1/derivation/summary-positive.jsontests/fixtures/access_context_v1/derivation/vector-negative.jsontests/fixtures/access_context_v1/derivation/vector-positive.jsontests/fixtures/access_context_v1/negative/context-expired.jsontests/fixtures/access_context_v1/negative/context-invalid.jsontests/fixtures/access_context_v1/negative/context-missing.jsontests/fixtures/access_context_v1/negative/context-revoked.jsontests/fixtures/access_context_v1/negative/context-unsupported-version.jsontests/fixtures/access_context_v1/negative/expired-and-out-of-scope.jsontests/fixtures/access_context_v1/negative/lease-stale.jsontests/fixtures/access_context_v1/negative/owner-only-principal-mismatch.jsontests/fixtures/access_context_v1/negative/ownership-changed.jsontests/fixtures/access_context_v1/negative/replay-context.jsontests/fixtures/access_context_v1/negative/replay-cursor.jsontests/fixtures/access_context_v1/negative/replay-lease.jsontests/fixtures/access_context_v1/negative/replay-principal.jsontests/fixtures/access_context_v1/negative/replay-reference.jsontests/fixtures/access_context_v1/negative/replay-request.jsontests/fixtures/access_context_v1/negative/replay-review-handle.jsontests/fixtures/access_context_v1/negative/scope-forbidden.jsontests/fixtures/access_context_v1/negative/scope-mismatch.jsontests/fixtures/access_context_v1/negative/target-not-found-or-forbidden.jsontests/fixtures/access_context_v1/positive/agent.jsontests/fixtures/access_context_v1/positive/delegated-child.jsontests/fixtures/access_context_v1/positive/human.jsontests/fixtures/access_context_v1/positive/narrowed.jsontests/fixtures/access_context_v1/positive/service.jsontests/fixtures/access_context_v1/revocation/lease-generation-change.jsontests/fixtures/access_context_v1/revocation/membership-revision-bump.jsontests/fixtures/access_context_v1/revocation/ownership-generation-change.jsontests/fixtures/access_context_v1/revocation/revocation-epoch-bump.jsontests/test_access_context_v1_fixtures.pytests/test_access_context_v1_inventory.pytests/test_access_context_v1_model.pytests/test_access_context_v1_validation.pytests/test_chunk_schema.pytests/test_import_lossless_claw.pytests/test_lcm_authorization_completeness.pytests/test_lcm_authorization_policy.pytests/test_lcm_authorization_read_path.pytests/test_lcm_authorization_sessions.pytests/test_lcm_authorization_write_path.pytests/test_profile_scoped_config.pytests/test_r2_authorization_findings.pytests/test_r3_authorization_findings.pytests/test_rollup_store.pytests/test_scope_storage.pytests/test_teams_apply_mode_gates.pytests/test_teams_assertion_scope.pytests/test_teams_audit.pytests/test_teams_catalog.pytests/test_teams_catalog_accessors.pytests/test_teams_connector.pytests/test_teams_host_surface.pytests/test_teams_lifecycle.pytests/test_teams_owner_predicate.pytests/test_teams_preflight.pytests/test_teams_real_policy_denies.pytests/test_teams_real_policy_gates.pytests/test_teams_revocation.pytests/test_teams_target_session.pytests/test_teams_target_shapes.pytests/test_two_principal_isolation_smoke.pytools.pyvector_store.py
📜 Review details
⏰ Context from checks skipped due to timeout. (8)
- GitHub Check: test (3.13)
- GitHub Check: test (3.12)
- GitHub Check: test (3.14)
- GitHub Check: test (3.11)
- GitHub Check: test (3.14)
- GitHub Check: test (3.13)
- GitHub Check: test (3.11)
- GitHub Check: test (3.12)
🧰 Additional context used
🪛 ast-grep (0.45.0)
tests/test_access_context_v1_inventory.py
[warning] 56-56: Regex pattern passed to re is built from a non-literal (variable, call, concatenation, or f-string) value. If that value is attacker-controlled it can introduce a malicious pattern with catastrophic backtracking (ReDoS). Use a hardcoded literal pattern, or validate/escape untrusted input with re.escape() and bound the regex complexity before compiling.
Context: re.search(pattern, source, re.MULTILINE)
Note: [CWE-1333] Inefficient Regular Expression Complexity.
(redos-non-literal-regex-python)
tests/test_r2_authorization_findings.py
[info] 85-85: use jsonify instead of json.dumps for JSON output
Context: json.dumps(args)
Note: [CWE-116] Improper Encoding or Escaping of Output.
(use-jsonify)
tests/test_two_principal_isolation_smoke.py
[info] 432-432: use jsonify instead of json.dumps for JSON output
Context: json.dumps(recent)
Note: [CWE-116] Improper Encoding or Escaping of Output.
(use-jsonify)
[info] 589-589: use jsonify instead of json.dumps for JSON output
Context: json.dumps(recent)
Note: [CWE-116] Improper Encoding or Escaping of Output.
(use-jsonify)
teams/connector.py
[info] 123-123: use jsonify instead of json.dumps for JSON output
Context: json.dumps(dict(self.payload), sort_keys=True, separators=(",", ":"))
Note: [CWE-116] Improper Encoding or Escaping of Output.
(use-jsonify)
[info] 219-219: use jsonify instead of json.dumps for JSON output
Context: json.dumps(dict(data), sort_keys=True)
Note: [CWE-116] Improper Encoding or Escaping of Output.
(use-jsonify)
engine.py
[info] 4278-4278: use jsonify instead of json.dumps for JSON output
Context: json.dumps({"error": f"Unknown LCM tool: {name}"})
Note: [CWE-116] Improper Encoding or Escaping of Output.
(use-jsonify)
🪛 LanguageTool
tests/fixtures/access_context_v1/README.md
[style] ~3-~3: Since ownership is already implied, this phrasing may be redundant.
Context: ...mer can parse the context object with its own JSON tooling; importing the `access_con...
(PRP_OWN)
🪛 markdownlint-cli2 (0.23.2)
tests/fixtures/access_context_v1/README.md
[warning] 1-1: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
docs/access-context-v1.md
[warning] 1-1: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
🪛 OpenGrep (1.26.0)
retrieval_core.py
[ERROR] 451-461: SQL query built via f-string passed to execute()/executemany(). Use parameterized queries with placeholders instead.
(coderabbit.sql-injection.python-fstring-execute)
[ERROR] 473-483: SQL query built via f-string passed to execute()/executemany(). Use parameterized queries with placeholders instead.
(coderabbit.sql-injection.python-fstring-execute)
teams/catalog.py
[ERROR] 132-135: SQL query built via f-string passed to execute()/executemany(). Use parameterized queries with placeholders instead.
(coderabbit.sql-injection.python-fstring-execute)
[ERROR] 311-314: SQL query built via f-string passed to execute()/executemany(). Use parameterized queries with placeholders instead.
(coderabbit.sql-injection.python-fstring-execute)
scripts/import_lossless_claw.py
[ERROR] 199-199: SQL query built via f-string passed to execute()/executemany(). Use parameterized queries with placeholders instead.
(coderabbit.sql-injection.python-fstring-execute)
[ERROR] 203-205: SQL query built via f-string passed to execute()/executemany(). Use parameterized queries with placeholders instead.
(coderabbit.sql-injection.python-fstring-execute)
scope_storage.py
[ERROR] 151-153: SQL query built via f-string passed to execute()/executemany(). Use parameterized queries with placeholders instead.
(coderabbit.sql-injection.python-fstring-execute)
[ERROR] 221-221: SQL query built via f-string passed to execute()/executemany(). Use parameterized queries with placeholders instead.
(coderabbit.sql-injection.python-fstring-execute)
[ERROR] 232-245: SQL query built via f-string passed to execute()/executemany(). Use parameterized queries with placeholders instead.
(coderabbit.sql-injection.python-fstring-execute)
[ERROR] 307-307: SQL query built via f-string passed to execute()/executemany(). Use parameterized queries with placeholders instead.
(coderabbit.sql-injection.python-fstring-execute)
[ERROR] 417-419: SQL query built via f-string passed to execute()/executemany(). Use parameterized queries with placeholders instead.
(coderabbit.sql-injection.python-fstring-execute)
[ERROR] 490-499: SQL query built via f-string passed to execute()/executemany(). Use parameterized queries with placeholders instead.
(coderabbit.sql-injection.python-fstring-execute)
[ERROR] 834-834: SQL query built via f-string passed to execute()/executemany(). Use parameterized queries with placeholders instead.
(coderabbit.sql-injection.python-fstring-execute)
engine.py
[ERROR] 4117-4120: SQL query built via f-string passed to execute()/executemany(). Use parameterized queries with placeholders instead.
(coderabbit.sql-injection.python-fstring-execute)
[ERROR] 4132-4135: SQL query built via f-string passed to execute()/executemany(). Use parameterized queries with placeholders instead.
(coderabbit.sql-injection.python-fstring-execute)
🪛 zizmor (1.29.0)
.github/workflows/ci.yml
[warning] 3-18: insufficient job-level concurrency limits (concurrency-limits): workflow is missing concurrency setting
(concurrency-limits)
Comment on lines
+5
to
+18
| # Long-lived feature branches need CI too. Until this was added, a branch | ||
| # with no PR against main matched NEITHER trigger, and `teams/lcm-teams-v1` | ||
| # accumulated 85 commits with zero CI runs -- which was misread for days as | ||
| # "Actions is degraded" when in fact no job had ever been queued. | ||
| # | ||
| # A PR is not a sufficient substitute: for `pull_request` events GitHub | ||
| # builds `refs/pull/N/merge`, so a CONFLICTING PR produces no run at all -- | ||
| # silently giving zero coverage exactly when a branch has drifted furthest | ||
| # and needs it most. Push events have no such dependency. | ||
| branches: [main, 'teams/**'] | ||
| pull_request: | ||
| branches: [main] | ||
| # Any branch, on demand. | ||
| workflow_dispatch: |
There was a problem hiding this comment.
🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick win
Add a workflow-level concurrency group.
A teams/** push and its pull request can queue overlapping CI runs. Cancel stale runs for the same head branch. Keep runs for different branches independent.
Proposed change
+concurrency:
+ group: ci-${{ github.workflow }}-${{ github.event.pull_request.head.ref || github.ref_name }}
+ cancel-in-progress: true
+
on:Before merge, verify that a push to a teams/** branch with an open pull request leaves one active CI run for that branch.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| # Long-lived feature branches need CI too. Until this was added, a branch | |
| # with no PR against main matched NEITHER trigger, and `teams/lcm-teams-v1` | |
| # accumulated 85 commits with zero CI runs -- which was misread for days as | |
| # "Actions is degraded" when in fact no job had ever been queued. | |
| # | |
| # A PR is not a sufficient substitute: for `pull_request` events GitHub | |
| # builds `refs/pull/N/merge`, so a CONFLICTING PR produces no run at all -- | |
| # silently giving zero coverage exactly when a branch has drifted furthest | |
| # and needs it most. Push events have no such dependency. | |
| branches: [main, 'teams/**'] | |
| pull_request: | |
| branches: [main] | |
| # Any branch, on demand. | |
| workflow_dispatch: | |
| concurrency: | |
| group: ci-${{ github.workflow }}-${{ github.event.pull_request.head.ref || github.ref_name }} | |
| cancel-in-progress: true | |
| on: | |
| # Long-lived feature branches need CI too. Until this was added, a branch | |
| # with no PR against main matched NEITHER trigger, and `teams/lcm-teams-v1` | |
| # accumulated 85 commits with zero CI runs -- which was misread for days as | |
| # "Actions is degraded" when in fact no job had ever been queued. | |
| # | |
| # A PR is not a sufficient substitute: for `pull_request` events GitHub | |
| # builds `refs/pull/N/merge`, so a CONFLICTING PR produces no run at all -- | |
| # silently giving zero coverage exactly when a branch has drifted furthest | |
| # and needs it most. Push events have no such dependency. | |
| branches: [main, 'teams/**'] | |
| pull_request: | |
| branches: [main] | |
| # Any branch, on demand. | |
| workflow_dispatch: |
🧰 Tools
🪛 zizmor (1.29.0)
[warning] 3-18: insufficient job-level concurrency limits (concurrency-limits): workflow is missing concurrency setting
(concurrency-limits)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In @.github/workflows/ci.yml around lines 5 - 18, Add workflow-level concurrency
configuration to the CI workflow, using the head branch or equivalent branch
identifier as the group key so push and pull_request runs for the same branch
share a group while different branches remain independent. Enable cancellation
of in-progress runs, and verify that a teams/** push with an open pull request
leaves only one active run for that branch.
Source: Linters/SAST tools
Comment on lines
+155
to
+158
| def __post_init__(self) -> None: | ||
| # Sanitize on this type too. Without it a caller-built PublicDecision | ||
| # can hold a mutable dict, so its hash would change after construction. | ||
| object.__setattr__(self, "detail", _safe_detail(self.detail)) |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
Mirror the Decision invariant check in PublicDecision.__post_init__.
PublicDecision is exported and constructible directly. Decision rejects allowed=True with a reason and allowed=False without one. PublicDecision accepts both. Consumers such as command.py raise AuthorizationRequiredError(..., decision.public().denial_reason), so a hand-built PublicDecision(False, None) yields an empty reason.
Also normalize denial_reason to DenialReason so a raw string does not compare unequal to the enum in the projection tests.
♻️ Proposed fix
def __post_init__(self) -> None:
# Sanitize on this type too. Without it a caller-built PublicDecision
# can hold a mutable dict, so its hash would change after construction.
+ reason = self.denial_reason
+ if reason is not None:
+ reason = DenialReason(reason)
+ if self.allowed and reason is not None:
+ raise ValueError("an allowed decision cannot carry a denial reason")
+ if not self.allowed and reason is None:
+ raise ValueError("a denied decision requires a denial reason")
+ object.__setattr__(self, "denial_reason", reason)
object.__setattr__(self, "detail", _safe_detail(self.detail))📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| def __post_init__(self) -> None: | |
| # Sanitize on this type too. Without it a caller-built PublicDecision | |
| # can hold a mutable dict, so its hash would change after construction. | |
| object.__setattr__(self, "detail", _safe_detail(self.detail)) | |
| def __post_init__(self) -> None: | |
| # Sanitize on this type too. Without it a caller-built PublicDecision | |
| # can hold a mutable dict, so its hash would change after construction. | |
| reason = self.denial_reason | |
| if reason is not None: | |
| reason = DenialReason(reason) | |
| if self.allowed and reason is not None: | |
| raise ValueError("an allowed decision cannot carry a denial reason") | |
| if not self.allowed and reason is None: | |
| raise ValueError("a denied decision requires a denial reason") | |
| object.__setattr__(self, "denial_reason", reason) | |
| object.__setattr__(self, "detail", _safe_detail(self.detail)) |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@access_context/denials.py` around lines 155 - 158, Update
PublicDecision.__post_init__ to enforce the same allowed/reason invariant as
Decision: require a denial reason when allowed is false and reject one when
allowed is true. Normalize any provided denial_reason to the DenialReason enum
before storing it, while preserving the existing detail sanitization.
Comment on lines
+27
to
+41
| if root is not None: | ||
| candidate = Path(root) | ||
| if candidate.name != FIXTURE_ROOT_NAME: | ||
| candidate = candidate / FIXTURE_ROOT_NAME | ||
| if candidate.is_dir(): | ||
| return candidate | ||
| package_root = Path(__file__).resolve().parent | ||
| candidates = ( | ||
| package_root.parent / "tests" / "fixtures" / FIXTURE_ROOT_NAME, | ||
| package_root / "tests" / "fixtures" / FIXTURE_ROOT_NAME, | ||
| package_root.parent.parent / "tests" / "fixtures" / FIXTURE_ROOT_NAME, | ||
| ) | ||
| for candidate in candidates: | ||
| if candidate.is_dir(): | ||
| return candidate |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Fail when an explicit fixture root is invalid.
When root is provided but does not resolve to a directory, fixture_root silently searches package-relative paths. A typo can load a different fixture corpus and make authorization tests validate the wrong data. Raise FixtureCorpusNotFound for the explicit path instead of falling back.
Proposed fix
if root is not None:
candidate = Path(root)
if candidate.name != FIXTURE_ROOT_NAME:
candidate = candidate / FIXTURE_ROOT_NAME
- if candidate.is_dir():
- return candidate
+ if not candidate.is_dir():
+ raise FixtureCorpusNotFound(
+ f"AccessContextV1 fixture corpus not found at explicit root: {candidate}"
+ )
+ return candidate📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| if root is not None: | |
| candidate = Path(root) | |
| if candidate.name != FIXTURE_ROOT_NAME: | |
| candidate = candidate / FIXTURE_ROOT_NAME | |
| if candidate.is_dir(): | |
| return candidate | |
| package_root = Path(__file__).resolve().parent | |
| candidates = ( | |
| package_root.parent / "tests" / "fixtures" / FIXTURE_ROOT_NAME, | |
| package_root / "tests" / "fixtures" / FIXTURE_ROOT_NAME, | |
| package_root.parent.parent / "tests" / "fixtures" / FIXTURE_ROOT_NAME, | |
| ) | |
| for candidate in candidates: | |
| if candidate.is_dir(): | |
| return candidate | |
| if root is not None: | |
| candidate = Path(root) | |
| if candidate.name != FIXTURE_ROOT_NAME: | |
| candidate = candidate / FIXTURE_ROOT_NAME | |
| if not candidate.is_dir(): | |
| raise FixtureCorpusNotFound( | |
| f"AccessContextV1 fixture corpus not found at explicit root: {candidate}" | |
| ) | |
| return candidate | |
| package_root = Path(__file__).resolve().parent | |
| candidates = ( | |
| package_root.parent / "tests" / "fixtures" / FIXTURE_ROOT_NAME, | |
| package_root / "tests" / "fixtures" / FIXTURE_ROOT_NAME, | |
| package_root.parent.parent / "tests" / "fixtures" / FIXTURE_ROOT_NAME, | |
| ) | |
| for candidate in candidates: | |
| if candidate.is_dir(): | |
| return candidate |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@access_context/fixtures.py` around lines 27 - 41, Update the fixture_root
resolution logic around the explicit root handling: when root is provided,
resolve the requested path (including FIXTURE_ROOT_NAME as currently required),
return it only if it is a directory, and otherwise raise FixtureCorpusNotFound
immediately. Keep package-relative candidate fallback limited to the case where
root is None.
Comment on lines
+75
to
+76
| if not isinstance(payload["description"], str) or not payload["description"].strip(): | ||
| raise FixtureFormatError(f"fixture description must be one line: {fixture_path}") |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Reject descriptions that contain line breaks.
description.strip() only rejects empty text. It accepts embedded \n and \r, although the validation error requires a one-line description. Reject line-break characters before returning the payload.
Proposed fix
- if not isinstance(payload["description"], str) or not payload["description"].strip():
+ description = payload["description"]
+ if (
+ not isinstance(description, str)
+ or not description.strip()
+ or "\n" in description
+ or "\r" in description
+ ):
raise FixtureFormatError(f"fixture description must be one line: {fixture_path}")📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| if not isinstance(payload["description"], str) or not payload["description"].strip(): | |
| raise FixtureFormatError(f"fixture description must be one line: {fixture_path}") | |
| description = payload["description"] | |
| if ( | |
| not isinstance(description, str) | |
| or not description.strip() | |
| or "\n" in description | |
| or "\r" in description | |
| ): | |
| raise FixtureFormatError(f"fixture description must be one line: {fixture_path}") |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@access_context/fixtures.py` around lines 75 - 76, Update the description
validation around payload["description"] to reject any embedded newline or
carriage-return characters, in addition to non-string and blank values. Preserve
the existing FixtureFormatError and one-line validation behavior for invalid
descriptions.
Comment on lines
+24
to
+50
| @dataclass(frozen=True) | ||
| class InventoryEntry: | ||
| id: str | ||
| category: str | ||
| module: str | ||
| entry_point: str | ||
| authority_requirement: str | ||
| discloses: tuple[str, ...] | ||
| notes: str | ||
|
|
||
| @classmethod | ||
| def from_mapping(cls, payload: Mapping[str, Any]) -> "InventoryEntry": | ||
| required = ("id", "category", "module", "entry_point", "authority_requirement", "discloses", "notes") | ||
| missing = [name for name in required if name not in payload] | ||
| if missing: | ||
| raise InventoryError(f"inventory entry missing fields: {', '.join(missing)}") | ||
| entry = cls( | ||
| id=str(payload["id"]), | ||
| category=str(payload["category"]), | ||
| module=str(payload["module"]), | ||
| entry_point=str(payload["entry_point"]), | ||
| authority_requirement=str(payload["authority_requirement"]), | ||
| discloses=tuple(str(item) for item in payload["discloses"]), | ||
| notes=str(payload["notes"]), | ||
| ) | ||
| entry.validate() | ||
| return entry |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
InventoryEntry silently discards hook and target_binding.
Every entry in access_context/inventory.json carries a hook object, and most tool entries carry target_binding. from_mapping copies only the seven fields listed on Line 36, so both are dropped. Two consequences:
- Nothing validates the shape of this metadata. A misspelled key (
requiresinstead ofrequired), ahook.required: trueentry with an emptysiteslist, or atarget_bindingnaming an argument that no longer exists all load without error. - Any structural gate/target check must re-parse the raw JSON, which duplicates the loader and lets the two views drift.
This is authorization metadata. Silent acceptance of a malformed hook declaration is the failure mode you least want here. Model the fields and validate them in validate().
♻️ Model and validate the hook declaration
+@dataclass(frozen=True)
+class HookRequirement:
+ required: bool
+ sites: tuple[str, ...] = ()
+ reason: str = ""
+
+ `@classmethod`
+ def from_mapping(cls, entry_id: str, payload: Mapping[str, Any]) -> "HookRequirement":
+ unknown = set(payload) - {"required", "sites", "reason"}
+ if unknown:
+ raise InventoryError(f"unknown hook keys in {entry_id}: {', '.join(sorted(unknown))}")
+ required = payload.get("required")
+ if not isinstance(required, bool):
+ raise InventoryError(f"hook.required must be a bool: {entry_id}")
+ sites = tuple(str(item) for item in payload.get("sites", ()))
+ reason = str(payload.get("reason", ""))
+ if required and not sites:
+ raise InventoryError(f"hook.required entries must name sites: {entry_id}")
+ if not required and not reason:
+ raise InventoryError(f"hook exemptions must state a reason: {entry_id}")
+ return cls(required=required, sites=sites, reason=reason)
+
+
`@dataclass`(frozen=True)
class InventoryEntry:
id: str
category: str
module: str
entry_point: str
authority_requirement: str
discloses: tuple[str, ...]
notes: str
+ hook: HookRequirementThen build it in from_mapping and add "hook" to required.
Run the following script to check how the structural gate tests read this metadata today:
#!/bin/bash
# Description: Find consumers of the inventory `hook` and `target_binding` metadata.
set -euo pipefail
rg -n --type=py -C4 '\b(target_binding|target_free|"hook"|\[.hook.\])' -g '!**/inventory.json'
rg -n --type=py -C4 '\bload_inventory\s*\(' 🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@access_context/inventory.py` around lines 24 - 50, Extend InventoryEntry with
modeled hook and target_binding fields, including validation for hook shape,
required/site constraints, and target argument references. Update from_mapping
to require and construct hook metadata plus target_binding when present, then
ensure validate() enforces these structures so consumers use the parsed fields
instead of raw JSON.
Comment on lines
+154
to
+162
| @pytest.mark.parametrize("method", sorted(HOST_ONLY - {"clone_for_agent"})) | ||
| def test_host_only_methods_are_not_reachable_from_a_tool_or_command(method: str) -> None: | ||
| """The safety argument for HOST_ONLY is unreachability, so test THAT. | ||
|
|
||
| `disable_teams` turns isolation off for every principal in the store. It is | ||
| ungated, and that is only acceptable while nothing the model can drive can | ||
| call it. If a tool or slash command ever wires one of these up, this fails | ||
| and the gate becomes mandatory. | ||
| """ |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
Record why clone_for_agent is excluded.
The parametrization removes clone_for_agent from the unreachability check without a stated reason. The file requires a written safety argument for every method. Add the argument in the docstring or in the HOST_ONLY comment.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tests/test_teams_host_surface.py` around lines 154 - 162, Document why
clone_for_agent is excluded from
test_host_only_methods_are_not_reachable_from_a_tool_or_command, either in that
docstring or the HOST_ONLY definition comment. State the specific safety
argument and preserve the existing parametrization and test behavior.
Comment on lines
+158
to
+182
| def _doctor_status(result: dict) -> str: | ||
| """Mirror of the mapping in tools.py's lcm_doctor scope_storage check.""" | ||
| status = str(result.get("status")) | ||
| if status in {"fail", "stamped-without-marker"}: | ||
| return "fail" | ||
| if status == "nothing-to-verify": | ||
| return "warn" | ||
| return "pass" | ||
|
|
||
|
|
||
| def test_doctor_is_not_green_on_an_aborted_enable(store: sqlite3.Connection) -> None: | ||
| """The check you would run to detect the bypass used to report pass. | ||
|
|
||
| verify_scope_storage takes teams_enabled from the CALLER, and after an | ||
| aborted enable that belief is wrong in the dangerous direction, so the | ||
| status fell through to not-enabled with a reassuring "legacy-compatible" | ||
| message on a store full of real per-owner stamps. | ||
| """ | ||
| _stamp(store) | ||
|
|
||
| result = scope_storage.verify_scope_storage(store, teams_enabled=False) | ||
|
|
||
| assert result["status"] == "stamped-without-marker" | ||
| assert _doctor_status(result) == "fail" | ||
| assert "aborted partway" in str(result["message"]) |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
The duplicated doctor mapping can drift from tools.py.
_doctor_status re-implements the status mapping used by lcm_doctor. If the mapping in tools.py changes, for example if stamped-without-marker stops mapping to fail, these tests still pass while the real doctor reports green on an aborted enable. Import the production mapping, or export it as a shared helper.
#!/bin/bash
# Locate the doctor scope_storage status mapping in production code.
rg -n -C6 'stamped-without-marker|nothing-to-verify' tools.py command.py scope_storage.py🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tests/test_teams_lifecycle.py` around lines 158 - 182, Remove the duplicated
_doctor_status mapping from the tests and reuse the production status-mapping
helper used by lcm_doctor, exporting that helper from its owning module if
necessary. Update test_doctor_is_not_green_on_an_aborted_enable to evaluate the
result through the shared production mapping so changes such as
stamped-without-marker no longer mapping to fail are reflected by the tests.
Comment on lines
+57
to
+69
| def test_narrowing_does_not_ride_on_the_collection_id() -> None: | ||
| """The defect this replaces, pinned so a regression is visible. | ||
|
|
||
| `source` is a property of the STORED ROW; the context's collection id is a | ||
| Teams concept. Overwriting one with the other filters real corpora down to | ||
| nothing, which reads as perfect isolation and is total breakage. | ||
| """ | ||
| resolved = TeamsPolicy(_context()).resolve_authorized_targets( | ||
| None, "read", {"session_scope": "all", "source": "openclaw-lcm:agent:acorn:uuid"} | ||
| ) | ||
|
|
||
| assert resolved.get("source") != "collection-acorn" | ||
| assert resolved["access_scope"] == "acorn" |
There was a problem hiding this comment.
🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick win
Assert that source is preserved, not merely that it changed.
Line 68 only rejects the value "collection-acorn". The regression this test targets is the policy overwriting a stored-row property with a Teams identifier. A policy that sets source to None, or to any other wrong value, still passes. Assert the exact passthrough value.
🧪 Proposed fix
- assert resolved.get("source") != "collection-acorn"
+ assert resolved["source"] == "openclaw-lcm:agent:acorn:uuid"
assert resolved["access_scope"] == "acorn"📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| def test_narrowing_does_not_ride_on_the_collection_id() -> None: | |
| """The defect this replaces, pinned so a regression is visible. | |
| `source` is a property of the STORED ROW; the context's collection id is a | |
| Teams concept. Overwriting one with the other filters real corpora down to | |
| nothing, which reads as perfect isolation and is total breakage. | |
| """ | |
| resolved = TeamsPolicy(_context()).resolve_authorized_targets( | |
| None, "read", {"session_scope": "all", "source": "openclaw-lcm:agent:acorn:uuid"} | |
| ) | |
| assert resolved.get("source") != "collection-acorn" | |
| assert resolved["access_scope"] == "acorn" | |
| def test_narrowing_does_not_ride_on_the_collection_id() -> None: | |
| """The defect this replaces, pinned so a regression is visible. | |
| `source` is a property of the STORED ROW; the context's collection id is a | |
| Teams concept. Overwriting one with the other filters real corpora down to | |
| nothing, which reads as perfect isolation and is total breakage. | |
| """ | |
| resolved = TeamsPolicy(_context()).resolve_authorized_targets( | |
| None, "read", {"session_scope": "all", "source": "openclaw-lcm:agent:acorn:uuid"} | |
| ) | |
| assert resolved["source"] == "openclaw-lcm:agent:acorn:uuid" | |
| assert resolved["access_scope"] == "acorn" |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tests/test_teams_owner_predicate.py` around lines 57 - 69, Update
test_narrowing_does_not_ride_on_the_collection_id to assert that
resolved["source"] exactly equals the input stored-row source value
"openclaw-lcm:agent:acorn:uuid", while retaining the existing access_scope
assertion.
Comment on lines
+72
to
+87
| def test_the_predicate_survives_the_omission_rule() -> None: | ||
| """The recall arm REMOVES keys the policy omits; the owner key is added. | ||
|
|
||
| Those two rules run over the same mapping, so the owner predicate has to be | ||
| applied after the removal loop or it would be stripped as "not authorized". | ||
| """ | ||
| from hermes_lcm import tools as tools_module | ||
| import inspect | ||
|
|
||
| source = inspect.getsource(tools_module._lcm_recall_fts_arm) | ||
| removal = source.index('fts_args.pop(key, None)') | ||
| addition = source.index('fts_args["access_scope"]') | ||
| assert addition > removal, ( | ||
| "the owner predicate must be applied AFTER the omission-removal loop, " | ||
| "or the loop strips it" | ||
| ) |
There was a problem hiding this comment.
🔒 Security & Privacy | 🔵 Trivial | 🏗️ Heavy lift
The source-text ordering assertions are weak proxies for the isolation property.
Three tests compare str.index offsets in inspect.getsource output:
str.indexraisesValueErrorwhen the substring is absent. The failure surfaces as an error, not as the written assertion message.- Only the FIRST occurrence is compared. If
_bounded_candidate_idsor_bounded_chunk_candidate_idscontains a second SQL branch, an ordering violation in that branch stays invisible. - Any formatting change, for example
access_scope=?without spaces or a parameterized column name, silently breaks the check.
Add an execution-level assertion for at least the vector helpers: seed rows for two principals, call the helper with one principal and a limit smaller than the total row count, then assert that no foreign-scope id is returned. That is the property; the text order is only evidence for it.
Also applies to: 109-141
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tests/test_teams_owner_predicate.py` around lines 72 - 87, Strengthen the
vector-helper tests around _bounded_candidate_ids and
_bounded_chunk_candidate_ids with execution-level isolation coverage: seed rows
belonging to two principals, invoke each helper for one principal with a limit
below the total row count, and assert every returned ID belongs only to that
principal. Retain or replace the fragile inspect.getsource ordering checks as
appropriate, but ensure the tests validate runtime behavior rather than only
source-text ordering.
Comment on lines
+70
to
+83
| def test_preflight_writes_nothing_and_creates_nothing( | ||
| store: sqlite3.Connection, | ||
| ) -> None: | ||
| """A preflight that migrates the store it inspects is not a preflight.""" | ||
| _message(store, "orphan-1") | ||
|
|
||
| preflight_teams_scope(store, _resolver) | ||
|
|
||
| columns = [row[1] for row in store.execute("PRAGMA table_info(messages)")] | ||
| assert ACCESS_SCOPE_COLUMN not in columns | ||
| stamped = store.execute( | ||
| "SELECT COUNT(*) FROM messages WHERE session_id IS NOT NULL" | ||
| ).fetchone()[0] | ||
| assert stamped == 1 # rows untouched, nothing added or removed |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
Tighten the "writes nothing and creates nothing" assertions.
The docstring claims a full non-mutation guarantee, but the test checks only the messages table: no access_scope column and one row with a non-null session_id. It does not verify that metadata gained no rows, that summary_nodes is untouched, or that no new schema object appeared. The upstream contract in scope_storage.py states that preflight creates neither the scope columns nor the metadata table, so capture the whole schema and the metadata contents.
🧪 Proposed fix
_message(store, "orphan-1")
+ before = store.execute(
+ "SELECT type, name, sql FROM sqlite_master ORDER BY name"
+ ).fetchall()
preflight_teams_scope(store, _resolver)
+ after = store.execute(
+ "SELECT type, name, sql FROM sqlite_master ORDER BY name"
+ ).fetchall()
+ assert after == before
+ assert store.execute("SELECT COUNT(*) FROM metadata").fetchone()[0] == 0
columns = [row[1] for row in store.execute("PRAGMA table_info(messages)")]
assert ACCESS_SCOPE_COLUMN not in columns🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tests/test_teams_preflight.py` around lines 70 - 83, Strengthen
test_preflight_writes_nothing_and_creates_nothing to snapshot the complete
SQLite schema and relevant data before calling preflight_teams_scope, then
assert they are unchanged afterward. Include metadata contents, summary_nodes
contents, and all schema objects in addition to the existing messages checks,
preserving the contract that preflight creates no scope columns, metadata table,
or other database objects.
Comment on lines
+32
to
+41
| def resolve_authorized_targets( | ||
| self, | ||
| context: AccessContextV1 | None, | ||
| operation: str, | ||
| requested_narrowing: TargetScope, | ||
| ) -> TargetScope: | ||
| # An empty SCOPE, not an empty sequence: callers read this with .get, | ||
| # and the protocol declares a mapping. Both are falsy, so the practical | ||
| # behaviour is unchanged -- this keeps the type honest. | ||
| return {} |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# List every resolve_authorized_targets call site with surrounding context,
# so the preceding authorize_operation gate can be confirmed.
rg -n -B 15 -A 5 --type=py 'resolve_authorized_targets\s*\(' -g '!**/tests/**'Repository: 100yenadmin/hermes-lcm
Length of output: 160
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- tracked access_policy files ---'
git ls-files 'access_policy/**' '*.py' | sed -n '1,200p'
printf '%s\n' '--- exact symbol references ---'
rg -n --hidden --glob '*.py' --glob '!**/.git/**' \
'resolve_authorized_targets|authorize_operation|authorize_stored_scope' .
printf '%s\n' '--- candidate policy files ---'
fd -t f -i 'fail_closed|teams_policy|access_policy' . | sed -n '1,200p'Repository: 100yenadmin/hermes-lcm
Length of output: 25007
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- fail_closed.py ---'
cat -n access_policy/fail_closed.py
printf '%s\n' '--- access_context/protocols.py ---'
cat -n access_context/protocols.py
printf '%s\n' '--- compaction.py 390-435 ---'
sed -n '390,435p' compaction.py
printf '%s\n' '--- retrieval_core.py 245-365 ---'
sed -n '245,365p' retrieval_core.py
printf '%s\n' '--- retrieval_core.py 470-515 and 610-650 ---'
sed -n '470,515p;610,650p' retrieval_core.py
printf '%s\n' '--- tools.py 420-465, 930-970, 4795-4835 ---'
sed -n '420,465p;930,970p;4795,4835p' tools.py
printf '%s\n' '--- engine.py 1915-1970 and 4190-4225 ---'
sed -n '1915,1970p;4190,4225p' engine.py
printf '%s\n' '--- command.py 2495-2550 ---'
sed -n '2495,2550p' command.pyRepository: 100yenadmin/hermes-lcm
Length of output: 30870
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- tools.py assertion-state handler ---'
sed -n '330,475p' tools.py
printf '%s\n' '--- focused read-path tests ---'
sed -n '1,230p' tests/test_lcm_authorization_read_path.py
sed -n '1,135p' tests/test_r3_authorization_findings.py
printf '%s\n' '--- policy resolution and assertion query definitions ---'
rg -n -A 35 -B 15 \
'def policy_for_engine|def policy_access_context|def query_assertion_state|def _shape_assertion_state_row' \
access_policy tools.py *.pyRepository: 100yenadmin/hermes-lcm
Length of output: 31389
🏁 Script executed:
#!/bin/bash
set -eu
python3 - <<'PY'
import ast
from pathlib import Path
root = Path(".")
for path in sorted(root.rglob("*.py")):
if any(part in {"tests", ".git", "build", "dist", "node_modules"} for part in path.parts):
continue
try:
tree = ast.parse(path.read_text(encoding="utf-8"))
except (OSError, SyntaxError):
continue
class Visitor(ast.NodeVisitor):
def __init__(self):
self.scope = []
def visit_FunctionDef(self, node):
self._function(node)
def visit_AsyncFunctionDef(self, node):
self._function(node)
def _function(self, node):
calls = []
for child in ast.walk(node):
if isinstance(child, ast.Call) and isinstance(child.func, ast.Attribute):
if child.func.attr in {"authorize_operation", "resolve_authorized_targets"}:
calls.append((child.lineno, child.func.attr))
resolves = [line for line, name in calls if name == "resolve_authorized_targets"]
if resolves:
gates = [line for line, name in calls if name == "authorize_operation"]
print(f"{path}:{node.lineno} {node.name}: "
f"resolve={resolves} authorize_operation={gates} "
f"ordered={all(any(g < r for g in gates) for r in resolves)}")
# Do not report nested functions as part of the outer function.
for child in node.body:
if isinstance(child, (ast.FunctionDef, ast.AsyncFunctionDef, ast.ClassDef)):
continue
self.visit(child)
Visitor().visit(tree)
PY
printf '%s\n' '--- assertion-state tests and handler references ---'
rg -n -A 12 -B 12 \
'lcm_query_state|query_assertion_state|assertion state|source_quote|source_ref' \
tests --glob '*.py' | sed -n '1,260p'Repository: 100yenadmin/hermes-lcm
Length of output: 19758
Gate lcm_query_state before resolving targets. tools.py:443 calls resolve_authorized_targets without authorize_operation. With FailClosedPolicy, {} leaves access_scope unset, so query_assertion_state can return unscoped assertions with foreign source quotes, session IDs, and store IDs. Add the read authorization gate and reject denial before resolving targets.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@access_policy/fail_closed.py` around lines 32 - 41, Update
resolve_authorized_targets to authorize the lcm_query_state operation before
resolving or returning the target scope. Use authorize_operation with the
provided context and operation, and reject or propagate a denial before
returning {}, ensuring query_assertion_state cannot produce unscoped results
when access is denied.
| return accessor() if callable(accessor) else None | ||
|
|
||
|
|
||
| def policy_for_engine(engine: object) -> "TrustedOwnerPolicy | FailClosedPolicy": |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
Update the stale return annotations to include TeamsPolicy.
policy_for_engine returns whatever resolve_policy returns, and resolve_policy now returns TeamsPolicy (Line 213). Both signatures still declare TrustedOwnerPolicy | FailClosedPolicy (Line 34 and Line 172). Static type checkers will reject correct downstream code that reads TeamsPolicy behaviour.
♻️ Proposed annotation fix
-def policy_for_engine(engine: object) -> "TrustedOwnerPolicy | FailClosedPolicy":
+def policy_for_engine(engine: object) -> "TrustedOwnerPolicy | TeamsPolicy | FailClosedPolicy":Apply the same change at Line 172:
-) -> TrustedOwnerPolicy | FailClosedPolicy:
+) -> TrustedOwnerPolicy | TeamsPolicy | FailClosedPolicy:📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| def policy_for_engine(engine: object) -> "TrustedOwnerPolicy | FailClosedPolicy": | |
| def policy_for_engine(engine: object) -> "TrustedOwnerPolicy | TeamsPolicy | FailClosedPolicy": |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@access_policy/resolution.py` at line 34, Update the return annotations for
both policy_for_engine and resolve_policy to include TeamsPolicy alongside
TrustedOwnerPolicy and FailClosedPolicy, matching the policies those functions
can return.
Comment on lines
+71
to
+101
| def _session_owner_for_engine(engine: object): | ||
| """Bind a target-session owner resolver to this engine's store, or None. | ||
|
|
||
| Answers "who owns this session" from the stamps already on disk, which is | ||
| authoritative: the same value the write path assigns. A session with no | ||
| stamped rows resolves to None and is treated as unclaimed by the policy. | ||
|
|
||
| A callable rather than a connection, for the same reason as the audit sink: | ||
| the policy keeps no database handle and stays testable with a plain dict. | ||
| """ | ||
|
|
||
| store = getattr(engine, "_store", None) | ||
| connection = getattr(store, "connection", None) | ||
| if connection is None: | ||
| return None | ||
|
|
||
| def resolve(session_id: str) -> str | None: | ||
| for table in ("messages", "summary_nodes"): | ||
| try: | ||
| row = connection.execute( | ||
| f'SELECT access_scope FROM "{table}" ' | ||
| "WHERE session_id = ? AND access_scope IS NOT NULL LIMIT 1", | ||
| (session_id,), | ||
| ).fetchone() | ||
| except sqlite3.OperationalError: | ||
| continue | ||
| if row is not None and row[0] is not None: | ||
| return str(row[0]) | ||
| return None | ||
|
|
||
| return resolve |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Find where access_scope is assigned on the write path and check whether a
# single session_id can receive rows stamped with different scopes.
rg -n -C 6 --type=py '_access_scope_for_storage_session|access_scope' -g '!**/tests/**' \
| rg -n -C 6 'INSERT|append|stamp|def _access_scope'Repository: 100yenadmin/hermes-lcm
Length of output: 160
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- candidate files ---'
fd -t f -e py | rg '(^|/)(access_policy|.*policy.*|.*store.*|.*engine.*)' | head -200
printf '%s\n' '--- access_scope references ---'
rg -n -C 5 --type=py 'access_scope|_access_scope_for_storage_session' . -g '!**/tests/**' -g '!**/.venv/**' | head -400
printf '%s\n' '--- relevant policy symbols ---'
rg -n -C 8 --type=py 'authorize_operation|_session_owner_for_engine|TeamsPolicy' . -g '!**/tests/**' | head -300Repository: 100yenadmin/hermes-lcm
Length of output: 42819
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- all access_scope writes ---'
rg -n -C 12 --type=py \
'access_scope\s*=|access_scope[),]|access_scope\)|access_scope\s*[,)]|ACCESS_SCOPE_COLUMN' . \
-g '!**/tests/**' -g '!**/.venv/**' | head -800
printf '%s\n' '--- SQL statements touching scope-bearing tables ---'
rg -n -C 10 --type=py \
'INSERT INTO (messages|summary_nodes)|UPDATE (messages|summary_nodes)|INSERT INTO .*messages|INSERT INTO .*summary_nodes|summary_nodes.*access_scope|messages.*access_scope' . \
-g '!**/tests/**' -g '!**/.venv/**' | head -800
printf '%s\n' '--- scope resolver and authorization implementation ---'
cat -n access_policy/resolution.py | sed -n '1,180p'
cat -n access_policy/teams_policy.py | sed -n '1,260p'Repository: 100yenadmin/hermes-lcm
Length of output: 50378
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- session scope normalization ---'
rg -n -C 25 --type=py \
'def _access_scope_for_session|def _access_scope_for_partition|_access_scope_for_session\(' \
store.py rollup_store.py vector_store.py
printf '%s\n' '--- summary node writes ---'
rg -n -C 18 --type=py \
'INSERT INTO summary_nodes|UPDATE summary_nodes|summary_nodes\s*\(' \
. -g '!**/tests/**' -g '!**/.venv/**' | head -500
printf '%s\n' '--- session reassignment and scope update writes ---'
rg -n -C 15 --type=py \
'reassign_session_messages|SET session_id|SET access_scope|access_scope.*session_id|session_id.*access_scope' \
store.py rollup_store.py vector_store.py db_bootstrap.py scope_storage.pyRepository: 100yenadmin/hermes-lcm
Length of output: 50378
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- session reassignment call sites ---'
rg -n -C 12 --type=py \
'reassign_session_messages|reassign_session_nodes' . \
-g '!**/tests/**' -g '!**/.venv/**'
printf '%s\n' '--- explicit scope write call sites ---'
rg -n -C 10 --type=py \
'\.append\(|\.append_batch\(|add_node\(' . \
-g '!**/tests/**' -g '!**/.venv/**' \
| rg -n -C 5 'access_scope|append|append_batch|add_node' | head -500
printf '%s\n' '--- policy owner handling ---'
cat -n access_policy/resolution.py | sed -n '55,125p'
cat -n access_policy/teams_policy.py | sed -n '1,230p'Repository: 100yenadmin/hermes-lcm
Length of output: 50378
🏁 Script executed:
#!/bin/bash
set -eu
python3 - <<'PY'
import sqlite3
from pathlib import Path
def resolve(conn, session_id):
for table in ("messages", "summary_nodes"):
row = conn.execute(
f'SELECT access_scope FROM "{table}" '
"WHERE session_id = ? AND access_scope IS NOT NULL LIMIT 1",
(session_id,),
).fetchone()
if row is not None:
return str(row[0])
return None
def result(scopes):
conn = sqlite3.connect(":memory:")
conn.execute("CREATE TABLE messages (session_id TEXT, access_scope TEXT)")
conn.executemany(
"INSERT INTO messages(session_id, access_scope) VALUES (?, ?)",
[("s", scope) for scope in scopes],
)
return resolve(conn, "s")
print("resolver(['principal-a', 'principal-b']) =", result(["principal-a", "principal-b"]))
print("resolver(['principal-b', 'principal-a']) =", result(["principal-b", "principal-a"]))
store = Path("store.py").read_text()
dag = Path("dag.py").read_text()
print("explicit message scope bypasses provider =", "if explicit is not None:\n return explicit" in store)
print("explicit summary scope bypasses provider =", "if explicit is not None:\n return explicit" in dag)
print("message reassignment preserves access_scope =", "UPDATE messages SET session_id = ? WHERE session_id = ?" in store)
print("summary reassignment preserves access_scope =", "UPDATE summary_nodes SET session_id = ? WHERE session_id = ?" in dag)
PYRepository: 100yenadmin/hermes-lcm
Length of output: 468
Fail closed when a session has multiple access_scope values.
store.py and dag.py preserve access_scope during session reassignment and accept explicit scopes, so one session can contain different scopes. _session_owner_for_engine then selects an arbitrary row. Do not return None; TeamsPolicy allows unresolved owners. Add an explicit ambiguous-owner denial or enforce the invariant transactionally.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@access_policy/resolution.py` around lines 71 - 101, Update
_session_owner_for_engine so each table detects whether a session has more than
one distinct non-null access_scope, rather than selecting an arbitrary row. When
conflicting scopes exist, return an explicit denied/ambiguous-owner result that
TeamsPolicy cannot treat as unresolved and allow; otherwise preserve the
existing single-scope owner resolution and None behavior for sessions without
stamped rows.
Comment on lines
+1
to
+12
| """DRAFT per-principal policy — the experiment the plan calls for first. | ||
|
|
||
| Purpose of this draft is to answer ONE question: do the two levers that already | ||
| exist -- per-row ``authorize_stored_scope`` and a ``resolve_authorized_targets`` | ||
| that overrides rather than passes through -- cover every leak probe in the | ||
| isolation smoke, or does some read path need an ``access_scope`` predicate added | ||
| to its query? | ||
|
|
||
| Not the finished policy. Membership, shared collections and delegation all | ||
| resolve from the catalog in the real one; this draft decides from the context | ||
| alone, which is enough to find out which probes still leak. | ||
| """ |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
Update the module docstring. It contradicts the shipped behaviour.
The docstring calls this a DRAFT that "decides from the context alone" and is "Not the finished policy". access_policy/__init__.py Lines 3-5 state the opposite: a valid Teams context now resolves to TeamsPolicy, and this is the enforcing path. tests/test_teams_real_policy_gates.py asserts real denials against this class.
A stale "DRAFT" header on the only enforcing authorization policy misleads the next reader about how much the class is trusted. Restate the current scope and the remaining catalog-backed gaps instead.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@access_policy/teams_policy.py` around lines 1 - 12, Update the module
docstring in teams_policy.py to describe this as the active enforcing
TeamsPolicy used for valid Teams contexts, removing the DRAFT and
unfinished-policy claims. Retain a concise statement of the remaining
catalog-backed gaps, including membership, shared collections, and delegation
resolution.
Comment on lines
+89
to
+95
| if self._session_owner is None: | ||
| return None | ||
| try: | ||
| owner = self._session_owner(session_id) | ||
| except Exception: # noqa: BLE001 - an unreadable owner is not a claim | ||
| return None | ||
| return str(owner) if owner else None |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
A swallowed owner-resolver exception grants cross-principal access, and no test observes it. _target_owner converts any resolver failure into None, which the owner loop treats as an unclaimed session and allows. The same conversion happens one layer down in access_policy/resolution.py Lines 95-96.
access_policy/teams_policy.py#L89-L95: distinguish "no owner row" from "the store could not answer", and deny on the second in the loop at Lines 156-158.tests/test_teams_target_session.py#L116-L127: add a case whosesession_ownercallable raisessqlite3.OperationalErrorand assert the decision is denied.
📍 Affects 2 files
access_policy/teams_policy.py#L89-L95(this comment)tests/test_teams_target_session.py#L116-L127
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@access_policy/teams_policy.py` around lines 89 - 95, The owner-resolution
flow must distinguish an unclaimed session from a resolver failure and deny
access on failures. Update _target_owner in access_policy/teams_policy.py and
the corresponding resolution logic in access_policy/resolution.py so exceptions
propagate as an error state to the owner loop at Lines 156-158, which must deny
rather than treat the session as unclaimed; add a test in
tests/test_teams_target_session.py covering a session_owner callable that raises
sqlite3.OperationalError and asserting denial.
Comment on lines
+164
to
+167
| def test_teams_off_never_consults_the_catalog(store: sqlite3.Connection) -> None: | ||
| """The negative control: default-off must be untouched by all of this.""" | ||
| policy = policy_for_engine(_Engine(store, None, teams_enabled=False)) | ||
| assert isinstance(policy, TrustedOwnerPolicy) |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
This negative control does not prove what its name claims.
The fixture store has no catalog and the context is None. policy_for_engine returns at Line 51 of access_policy/resolution.py before _catalog_revisions runs, for both reasons at once. The test passes even if catalog consultation moved ahead of the Teams-off check.
Seed a catalog and bump the epoch so a consulted catalog would produce FailClosedPolicy. Then TrustedOwnerPolicy is real evidence.
💚 Proposed strengthening
def test_teams_off_never_consults_the_catalog(store: sqlite3.Connection) -> None:
"""The negative control: default-off must be untouched by all of this."""
- policy = policy_for_engine(_Engine(store, None, teams_enabled=False))
+ catalog.ensure_teams_catalog(store)
+ catalog.bump_revision(store, "tenant-1", "revocation_epoch")
+ # A consulted catalog would read this context as revoked.
+ policy = policy_for_engine(_Engine(store, _context(), teams_enabled=False))
assert isinstance(policy, TrustedOwnerPolicy)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| def test_teams_off_never_consults_the_catalog(store: sqlite3.Connection) -> None: | |
| """The negative control: default-off must be untouched by all of this.""" | |
| policy = policy_for_engine(_Engine(store, None, teams_enabled=False)) | |
| assert isinstance(policy, TrustedOwnerPolicy) | |
| def test_teams_off_never_consults_the_catalog(store: sqlite3.Connection) -> None: | |
| """The negative control: default-off must be untouched by all of this.""" | |
| catalog.ensure_teams_catalog(store) | |
| catalog.bump_revision(store, "tenant-1", "revocation_epoch") | |
| # A consulted catalog would read this context as revoked. | |
| policy = policy_for_engine(_Engine(store, _context(), teams_enabled=False)) | |
| assert isinstance(policy, TrustedOwnerPolicy) |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tests/test_teams_revocation.py` around lines 164 - 167, Strengthen
test_teams_off_never_consults_the_catalog by seeding the fixture store with
catalog data and advancing its epoch before calling policy_for_engine. Keep
teams_enabled=False, then assert TrustedOwnerPolicy so the test distinguishes
the Teams-off early return from catalog consultation, which would otherwise
yield FailClosedPolicy.
Comment on lines
+116
to
+127
| @pytest.mark.parametrize("key", ["session_id", "source_session_id", "partition_key"]) | ||
| def test_every_target_key_resolves_by_owner(key: str) -> None: | ||
| """The three keys that carry a target session across different paths.""" | ||
| allowed = TeamsPolicy( | ||
| _context(), session_owner=_owner_map({"other": "acorn"}) | ||
| ).authorize_operation(None, "write", {key: "other"}) | ||
| denied = TeamsPolicy( | ||
| _context(), session_owner=_owner_map({"other": "carus"}) | ||
| ).authorize_operation(None, "write", {key: "other"}) | ||
|
|
||
| assert allowed.allowed, f"{key}: same-owner target should be allowed" | ||
| assert not denied.allowed, f"{key}: cross-owner target must be denied" |
There was a problem hiding this comment.
🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick win
Add a case for a resolver that raises.
Every case here supplies a resolver that returns cleanly. TeamsPolicy._target_owner (Lines 91-94 of access_policy/teams_policy.py) swallows all exceptions and returns None, which the loop treats as unclaimed and allows. No test pins that behaviour, so the fail-open path is unobserved. See the root-cause comment on access_policy/teams_policy.py.
💚 Proposed test
+def test_an_unreadable_owner_is_not_treated_as_unclaimed() -> None:
+ """A store that cannot answer must not grant another principal's session."""
+ def raising(session_id: str) -> str | None:
+ raise sqlite3.OperationalError("no such table: messages")
+
+ decision = TeamsPolicy(_context(), session_owner=raising).authorize_operation(
+ None, "write", {"kind": "session_end", "session_id": "unknown-to-us"}
+ )
+
+ assert not decision.allowed🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tests/test_teams_target_session.py` around lines 116 - 127, Extend
test_every_target_key_resolves_by_owner with a resolver case that raises an
exception for the target key, then assert the write is denied rather than
allowed. Ensure the test exercises TeamsPolicy._target_owner’s exception path
and pins secure fail-closed behavior for all parametrized target keys.
Comment on lines
+84
to
+89
| # lcm_query_state addresses assertions by subject, and the assertion | ||
| # tables carry no access_scope column at all -- so there is nothing to | ||
| # resolve here yet. Tracked separately; this entry records that the | ||
| # allow is currently UNJUSTIFIED rather than reasoned. | ||
| "subject_key", | ||
| "scope_key", |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Stale rationale: lcm_query_state is scoped in this PR.
The comment states the assertion tables carry no access_scope column, so "there is nothing to resolve here yet" and the allow is "UNJUSTIFIED". That is no longer accurate for this branch. assertion_store.query_assertions and query_relations now filter on the owning source message via m.access_scope, and assertion_state.query_assertion_state forwards access_scope from tools.py.
The entries can stay in known_unresolvable, because subject_key and scope_key still do not address a row that bears an owner stamp. Update the rule that decides them so the structural gate records the real mechanism.
📝 Proposed rationale update
- # lcm_query_state addresses assertions by subject, and the assertion
- # tables carry no access_scope column at all -- so there is nothing to
- # resolve here yet. Tracked separately; this entry records that the
- # allow is currently UNJUSTIFIED rather than reasoned.
- "subject_key",
- "scope_key",
+ # lcm_query_state addresses assertions by subject, not by row id, so
+ # there is no stamp to resolve on these keys. The owner predicate is
+ # applied downstream instead: query_assertions/query_relations filter
+ # on the owning source message's access_scope.
+ "subject_key",
+ "scope_key",📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| # lcm_query_state addresses assertions by subject, and the assertion | |
| # tables carry no access_scope column at all -- so there is nothing to | |
| # resolve here yet. Tracked separately; this entry records that the | |
| # allow is currently UNJUSTIFIED rather than reasoned. | |
| "subject_key", | |
| "scope_key", | |
| # lcm_query_state addresses assertions by subject, not by row id, so | |
| # there is no stamp to resolve on these keys. The owner predicate is | |
| # applied downstream instead: query_assertions/query_relations filter | |
| # on the owning source message's access_scope. | |
| "subject_key", | |
| "scope_key", |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tests/test_teams_target_shapes.py` around lines 84 - 89, Update the rationale
and classification logic for the lcm_query_state entries in known_unresolvable:
acknowledge that assertion_store.query_assertions/query_relations and
assertion_state.query_assertion_state now apply access_scope through the owning
source message, while preserving subject_key and scope_key as unresolved because
they do not identify an owner-stamped row. Ensure the structural gate records
this mechanism rather than marking the allow as UNJUSTIFIED.
| SESSION_A = "session-a" | ||
| SESSION_B = "session-b" | ||
| CONVERSATION_A = "conversation-a" | ||
| PERMISSIVE = "hook present but resolves permissive at access_policy/resolution.py:79" |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Remove the stale placeholder markers. They now print wrong diagnostics.
- Line 43:
PERMISSIVEclaims the seam "resolves permissive at access_policy/resolution.py:79". Line 79 of the currentaccess_policy/resolution.pyis inside_session_owner_for_engine. The permissive placeholder is gone, as the module docstring at Lines 3-5 states. Every leak message in_b_leaksappends this text, so a real failure report points a reader at unrelated code. - Lines 232-238:
_TargetBoundaryPolicystill says "The real Teams policy is intentionally permissive until#483". - Lines 550-552: the comment still calls these "pre-existing Teams-policy placeholder legs until
#483".
Replace the constant with a description of the observed leak, and delete the two stale comments.
♻️ Proposed fix
-PERMISSIVE = "hook present but resolves permissive at access_policy/resolution.py:79"
+PERMISSIVE = "TeamsPolicy did not scope this read path to the acting principal"Also applies to: 231-238, 550-552
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tests/test_two_principal_isolation_smoke.py` at line 43, Update PERMISSIVE in
the isolation smoke test to describe the observed leak without referencing the
obsolete access_policy/resolution.py location or permissive placeholder. Remove
the stale “until `#483`” placeholder comments from _TargetBoundaryPolicy and the
pre-existing Teams-policy legs near _b_leaks, leaving the surrounding test
behavior unchanged.
Comment on lines
+697
to
+739
| def test_store_wide_backup_is_admin_only_under_teams( | ||
| tmp_path: Path, monkeypatch: pytest.MonkeyPatch | ||
| ) -> None: | ||
| """A whole-store backup is not any principal's to take. | ||
|
|
||
| It copies every principal's memory into one file, so under Teams it is an | ||
| administrative capability -- #497 gives the connector the `backup.*` family | ||
| and authenticates it separately. The positive control used to assert | ||
| principal A could do it, which encoded pre-Teams semantics: A was not | ||
| "the owner", A was merely the one who asked. | ||
|
|
||
| Asserting BOTH principals are refused is a stronger claim than the leg it | ||
| replaces, which only checked that B was not. And the Teams-off leg is what | ||
| keeps it honest -- without it this would pass just as well if backup were | ||
| broken outright rather than scoped. | ||
| """ | ||
| db_path = tmp_path / "backup-admin.db" | ||
| context_a = _context( | ||
| principal_id="A", profile_id="profile-a", session_id=SESSION_A, collection=COLLECTION_A | ||
| ) | ||
| context_b = _context( | ||
| principal_id="B", profile_id="profile-b", session_id=SESSION_B, collection=COLLECTION_B | ||
| ) | ||
| engine_a = _engine(db_path, context_a, teams_enabled=True) | ||
| engine_b = _engine(db_path, context_b, teams_enabled=True) | ||
| try: | ||
| for name, engine in (("A", engine_a), ("B", engine_b)): | ||
| with pytest.raises(Exception) as excinfo: | ||
| maintenance_module.backup_database(engine) | ||
| assert "authorize" in str(excinfo.value), ( | ||
| f"principal {name} was refused, but not by the authorization seam" | ||
| ) | ||
| finally: | ||
| engine_b.shutdown() | ||
| engine_a.shutdown() | ||
|
|
||
| # Teams OFF: unchanged. Backup is restricted BY Teams, not broken by it. | ||
| off_path = tmp_path / "backup-teams-off.db" | ||
| engine_off = _engine(off_path, context_a, teams_enabled=False) | ||
| try: | ||
| assert maintenance_module.backup_database(engine_off).get("ok") | ||
| finally: | ||
| engine_off.shutdown() |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
Tighten the exception assertion in the backup test.
pytest.raises(Exception) plus a substring check on "authorize" accepts any exception whose text happens to contain that word, including an AttributeError from a refactor. The seam raises AuthorizationRequiredError. Assert that type directly.
💚 Proposed fix
+ from hermes_lcm.access_policy import AuthorizationRequiredError
+
try:
for name, engine in (("A", engine_a), ("B", engine_b)):
- with pytest.raises(Exception) as excinfo:
+ with pytest.raises(AuthorizationRequiredError):
maintenance_module.backup_database(engine)
- assert "authorize" in str(excinfo.value), (
- f"principal {name} was refused, but not by the authorization seam"
- )📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| def test_store_wide_backup_is_admin_only_under_teams( | |
| tmp_path: Path, monkeypatch: pytest.MonkeyPatch | |
| ) -> None: | |
| """A whole-store backup is not any principal's to take. | |
| It copies every principal's memory into one file, so under Teams it is an | |
| administrative capability -- #497 gives the connector the `backup.*` family | |
| and authenticates it separately. The positive control used to assert | |
| principal A could do it, which encoded pre-Teams semantics: A was not | |
| "the owner", A was merely the one who asked. | |
| Asserting BOTH principals are refused is a stronger claim than the leg it | |
| replaces, which only checked that B was not. And the Teams-off leg is what | |
| keeps it honest -- without it this would pass just as well if backup were | |
| broken outright rather than scoped. | |
| """ | |
| db_path = tmp_path / "backup-admin.db" | |
| context_a = _context( | |
| principal_id="A", profile_id="profile-a", session_id=SESSION_A, collection=COLLECTION_A | |
| ) | |
| context_b = _context( | |
| principal_id="B", profile_id="profile-b", session_id=SESSION_B, collection=COLLECTION_B | |
| ) | |
| engine_a = _engine(db_path, context_a, teams_enabled=True) | |
| engine_b = _engine(db_path, context_b, teams_enabled=True) | |
| try: | |
| for name, engine in (("A", engine_a), ("B", engine_b)): | |
| with pytest.raises(Exception) as excinfo: | |
| maintenance_module.backup_database(engine) | |
| assert "authorize" in str(excinfo.value), ( | |
| f"principal {name} was refused, but not by the authorization seam" | |
| ) | |
| finally: | |
| engine_b.shutdown() | |
| engine_a.shutdown() | |
| # Teams OFF: unchanged. Backup is restricted BY Teams, not broken by it. | |
| off_path = tmp_path / "backup-teams-off.db" | |
| engine_off = _engine(off_path, context_a, teams_enabled=False) | |
| try: | |
| assert maintenance_module.backup_database(engine_off).get("ok") | |
| finally: | |
| engine_off.shutdown() | |
| def test_store_wide_backup_is_admin_only_under_teams( | |
| tmp_path: Path, monkeypatch: pytest.MonkeyPatch | |
| ) -> None: | |
| """A whole-store backup is not any principal's to take. | |
| It copies every principal's memory into one file, so under Teams it is an | |
| administrative capability -- `#497` gives the connector the `backup.*` family | |
| and authenticates it separately. The positive control used to assert | |
| principal A could do it, which encoded pre-Teams semantics: A was not | |
| "the owner", A was merely the one who asked. | |
| Asserting BOTH principals are refused is a stronger claim than the leg it | |
| replaces, which only checked that B was not. And the Teams-off leg is what | |
| keeps it honest -- without it this would pass just as well if backup were | |
| broken outright rather than scoped. | |
| """ | |
| db_path = tmp_path / "backup-admin.db" | |
| context_a = _context( | |
| principal_id="A", profile_id="profile-a", session_id=SESSION_A, collection=COLLECTION_A | |
| ) | |
| context_b = _context( | |
| principal_id="B", profile_id="profile-b", session_id=SESSION_B, collection=COLLECTION_B | |
| ) | |
| engine_a = _engine(db_path, context_a, teams_enabled=True) | |
| engine_b = _engine(db_path, context_b, teams_enabled=True) | |
| from hermes_lcm.access_policy import AuthorizationRequiredError | |
| try: | |
| for name, engine in (("A", engine_a), ("B", engine_b)): | |
| with pytest.raises(AuthorizationRequiredError): | |
| maintenance_module.backup_database(engine) | |
| finally: | |
| engine_b.shutdown() | |
| engine_a.shutdown() | |
| # Teams OFF: unchanged. Backup is restricted BY Teams, not broken by it. | |
| off_path = tmp_path / "backup-teams-off.db" | |
| engine_off = _engine(off_path, context_a, teams_enabled=False) | |
| try: | |
| assert maintenance_module.backup_database(engine_off).get("ok") | |
| finally: | |
| engine_off.shutdown() |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tests/test_two_principal_isolation_smoke.py` around lines 697 - 739, Update
test_store_wide_backup_is_admin_only_under_teams to assert the specific
AuthorizationRequiredError type in both Teams-enabled backup_database calls
instead of pytest.raises(Exception). Remove the substring-based "authorize"
assertion while preserving the checks for both principals and the Teams-disabled
success path.
Comment on lines
+415
to
+429
| if isinstance(authorized_scope, dict): | ||
| source_scope = authorized_scope.get("source_scope", access_context) | ||
| derived_scope = authorized_scope.get("derived_scope", access_context) | ||
| if ( | ||
| isinstance(source_scope, AccessContextV1) | ||
| and isinstance(derived_scope, AccessContextV1) | ||
| and not is_subset_of(derived_scope, source_scope) | ||
| ): | ||
| mismatch = Decision.deny(DenialReason.SCOPE_MISMATCH) | ||
| policy.audit_decision( | ||
| access_context, "write", mismatch.denial_reason, mismatch.public() | ||
| ) | ||
| raise AuthorizationRequiredError( | ||
| "authorize_operation", mismatch.public().denial_reason | ||
| ) |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Scope-containment check fails open on a mixed-type resolver return. Both sites gate is_subset_of behind isinstance(source, AccessContextV1) and isinstance(derived, AccessContextV1). When exactly one side is an AccessContextV1 and the other is any other type, the conjunction is false, the containment check is skipped, and the write proceeds. A scope-carrying value of the wrong type is precisely what the check should reject, so the malformed case is the one that passes.
compaction.py#L415-L429: deny withDenialReason.SCOPE_MISMATCHwhen either resolved scope is non-Nonebut not anAccessContextV1, instead of falling through to the compaction write.engine.py#L1948-L1965: apply the same inversion to the rollupsource_scope/derived_scopepair beforecaptured_decisionis taken.
📍 Affects 2 files
compaction.py#L415-L429(this comment)engine.py#L1948-L1965
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@compaction.py` around lines 415 - 429, Fix the scope-containment validation
so mixed-type resolved scopes fail closed: in compaction.py lines 415-429, deny
with Decision.DenyReason.SCOPE_MISMATCH when either non-None scope is not an
AccessContextV1, while retaining is_subset_of validation for two valid contexts;
apply the same change to the rollup source_scope/derived_scope handling in
engine.py lines 1948-1965 before captured_decision is assigned.
Comment on lines
+426
to
+437
| _AUTHORITY_OPERATION_BY_REQUIREMENT = { | ||
| "read_scoped": "read", | ||
| "write_scoped": "write", | ||
| "owner_only": "owner_only", | ||
| "admin_only": "admin", | ||
| "none_required": "none", | ||
| } | ||
| LCM_TOOL_AUTHORITY_OPERATIONS = { | ||
| entry.entry_point: _AUTHORITY_OPERATION_BY_REQUIREMENT[entry.authority_requirement] | ||
| for entry in load_inventory() | ||
| if entry.module == "tools.py" and entry.entry_point.startswith("lcm_") | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
An unknown authority_requirement crashes module import.
_AUTHORITY_OPERATION_BY_REQUIREMENT[entry.authority_requirement] is a direct subscript inside a dict comprehension that runs at module import. load_inventory() reads access_context/inventory.json. If any tools.py entry carries a requirement string outside the five known keys, the comprehension raises KeyError and hermes_lcm.engine becomes unimportable. The whole plugin fails to load because of one data-file value.
Fail closed on the unknown value instead of crashing: map it to the most restrictive operation.
🛡️ Proposed fix
LCM_TOOL_AUTHORITY_OPERATIONS = {
- entry.entry_point: _AUTHORITY_OPERATION_BY_REQUIREMENT[entry.authority_requirement]
+ entry.entry_point: _AUTHORITY_OPERATION_BY_REQUIREMENT.get(
+ entry.authority_requirement, "owner_only"
+ )
for entry in load_inventory()
if entry.module == "tools.py" and entry.entry_point.startswith("lcm_")
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| _AUTHORITY_OPERATION_BY_REQUIREMENT = { | |
| "read_scoped": "read", | |
| "write_scoped": "write", | |
| "owner_only": "owner_only", | |
| "admin_only": "admin", | |
| "none_required": "none", | |
| } | |
| LCM_TOOL_AUTHORITY_OPERATIONS = { | |
| entry.entry_point: _AUTHORITY_OPERATION_BY_REQUIREMENT[entry.authority_requirement] | |
| for entry in load_inventory() | |
| if entry.module == "tools.py" and entry.entry_point.startswith("lcm_") | |
| } | |
| _AUTHORITY_OPERATION_BY_REQUIREMENT = { | |
| "read_scoped": "read", | |
| "write_scoped": "write", | |
| "owner_only": "owner_only", | |
| "admin_only": "admin", | |
| "none_required": "none", | |
| } | |
| LCM_TOOL_AUTHORITY_OPERATIONS = { | |
| entry.entry_point: _AUTHORITY_OPERATION_BY_REQUIREMENT.get( | |
| entry.authority_requirement, "owner_only" | |
| ) | |
| for entry in load_inventory() | |
| if entry.module == "tools.py" and entry.entry_point.startswith("lcm_") | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@engine.py` around lines 426 - 437, Update the LCM_TOOL_AUTHORITY_OPERATIONS
comprehension to handle unknown entry.authority_requirement values without
raising KeyError during import, mapping them to the most restrictive authority
operation instead. Preserve the existing mappings for all recognized
requirements and apply the fallback only when the lookup key is unknown.
Comment on lines
+844
to
+855
| connection = getattr(self._store, "connection", None) | ||
| if connection is None: | ||
| return | ||
| enabled, reason = resolve_startup_teams_state(connection) | ||
| self._teams_state_reason = reason | ||
| if enabled: | ||
| mark_teams_enabled(self) | ||
| else: | ||
| # Explicitly cleared rather than left alone: rebinding to a | ||
| # different store must not inherit the previous store's answer. | ||
| if hasattr(self, _access_policy.TEAMS_ENABLED_ATTR): | ||
| delattr(self, _access_policy.TEAMS_ENABLED_ATTR) |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
The connection is None early return inherits the previous store's Teams answer.
The docstring states the marker is cleared explicitly so a rebind cannot inherit the previous store's decision. Lines 845-846 return before that clear. If the bound store has no connection, the in-memory TEAMS_ENABLED_ATTR from the previously bound store survives, and _teams_state_reason keeps its stale value. The result is a permissive-or-stale Teams decision over a store that was never consulted, which is the state this method exists to prevent.
Clear the marker and record the reason before returning.
🔒 Proposed fix
connection = getattr(self._store, "connection", None)
if connection is None:
+ # A store we cannot read is not a store that enabled Teams.
+ self._teams_state_reason = "unreadable-store"
+ if hasattr(self, _access_policy.TEAMS_ENABLED_ATTR):
+ delattr(self, _access_policy.TEAMS_ENABLED_ATTR)
return📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| connection = getattr(self._store, "connection", None) | |
| if connection is None: | |
| return | |
| enabled, reason = resolve_startup_teams_state(connection) | |
| self._teams_state_reason = reason | |
| if enabled: | |
| mark_teams_enabled(self) | |
| else: | |
| # Explicitly cleared rather than left alone: rebinding to a | |
| # different store must not inherit the previous store's answer. | |
| if hasattr(self, _access_policy.TEAMS_ENABLED_ATTR): | |
| delattr(self, _access_policy.TEAMS_ENABLED_ATTR) | |
| connection = getattr(self._store, "connection", None) | |
| if connection is None: | |
| # A store we cannot read is not a store that enabled Teams. | |
| self._teams_state_reason = "unreadable-store" | |
| if hasattr(self, _access_policy.TEAMS_ENABLED_ATTR): | |
| delattr(self, _access_policy.TEAMS_ENABLED_ATTR) | |
| return | |
| enabled, reason = resolve_startup_teams_state(connection) | |
| self._teams_state_reason = reason | |
| if enabled: | |
| mark_teams_enabled(self) | |
| else: | |
| # Explicitly cleared rather than left alone: rebinding to a | |
| # different store must not inherit the previous store's answer. | |
| if hasattr(self, _access_policy.TEAMS_ENABLED_ATTR): | |
| delattr(self, _access_policy.TEAMS_ENABLED_ATTR) |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@engine.py` around lines 844 - 855, Update the connection-none branch in the
Teams state rebinding logic to clear any existing TEAMS_ENABLED_ATTR marker and
set _teams_state_reason to the appropriate no-connection reason before
returning. Preserve the existing resolve_startup_teams_state and
mark_teams_enabled behavior when a connection is available.
Comment on lines
+893
to
+974
| def enable_teams( | ||
| self, | ||
| owner_for_session: Callable[[str], str | None] | None = None, | ||
| *, | ||
| batch_size: int = 256, | ||
| overrides: Mapping[str, str] | None = None, | ||
| fallback_owner: str | None = None, | ||
| ) -> dict[str, object]: | ||
| """Set up Teams scope storage and stamp all historical LCM rows. | ||
|
|
||
| This is intentionally explicit: ordinary schema startup never calls | ||
| the backfill, so a store that never enables Teams remains unstamped. | ||
| The flag is published only after the setup transaction has completed. | ||
| """ | ||
|
|
||
| result = setup_teams_scope( | ||
| self._store.connection, | ||
| owner_for_session or self._preteams_owner_for_session, | ||
| batch_size=batch_size, | ||
| overrides=overrides, | ||
| fallback_owner=fallback_owner, | ||
| ) | ||
| # After the backfill, so a store whose stamping failed does not acquire | ||
| # catalog tables it never gets to use. | ||
| result["catalog"] = ensure_teams_catalog(self._store.connection) | ||
| # An incomplete backfill raises ScopeBackfillIncompleteError out of | ||
| # setup_teams_scope, so control never reaches the lines below and the | ||
| # marker stays unset -- leaving the store stamped-without-marker, which | ||
| # fails closed until the operator supplies the missing owners and | ||
| # re-runs. The backfill is idempotent, so re-running resumes. | ||
| # | ||
| # Durable BEFORE in-process, so a crash between the two lands on | ||
| # "enabled" rather than on stamps with no recorded decision. | ||
| persist_teams_enabled(self._store.connection, True) | ||
| mark_teams_enabled(self) | ||
| self._teams_state_reason = "enabled" | ||
| return result | ||
|
|
||
| def preflight_teams( | ||
| self, | ||
| owner_for_session: Callable[[str], str | None] | None = None, | ||
| *, | ||
| overrides: Mapping[str, str] | None = None, | ||
| fallback_owner: str | None = None, | ||
| ) -> dict[str, object]: | ||
| """Report every owner an enable would need, without writing anything. | ||
|
|
||
| Run this before :meth:`enable_teams` on any store that matters. It | ||
| answers the one question that can strand an enable half-done -- which | ||
| sessions cannot be attributed -- while the store is still untouched. | ||
| """ | ||
|
|
||
| return preflight_teams_scope( | ||
| self._store.connection, | ||
| owner_for_session or self._preteams_owner_for_session, | ||
| overrides=overrides, | ||
| fallback_owner=fallback_owner, | ||
| ) | ||
|
|
||
| def disable_teams(self) -> dict[str, object]: | ||
| """Record that Teams is off, without unstamping anything. | ||
|
|
||
| Additive-only by design: the access_scope values stay exactly as they | ||
| are. Stripping them would destroy the attribution a later re-enable | ||
| depends on, and would be the one genuinely irreversible operation in | ||
| this feature. Disable is a decision, not a migration. | ||
|
|
||
| Because the stamps remain, the durable ``false`` matters -- it is what | ||
| distinguishes "an operator turned this off" from "an enable died | ||
| partway", which :func:`resolve_startup_teams_state` must treat very | ||
| differently. | ||
| """ | ||
|
|
||
| persist_teams_enabled(self._store.connection, False) | ||
| if hasattr(self, _access_policy.TEAMS_ENABLED_ATTR): | ||
| delattr(self, _access_policy.TEAMS_ENABLED_ATTR) | ||
| self._teams_state_reason = "disabled" | ||
| return {"teams_enabled": False, "stamps_retained": True} | ||
|
|
||
| # Host adapters may use either verb; both routes share the same idempotent | ||
| # setup/backfill implementation and do not alter the default-off path. | ||
| setup_teams = enable_teams |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
enable_teams and disable_teams mutate the enforcement state with no authorization gate.
Every other mutating surface in this change resolves a policy first: on_session_start, on_session_end, on_session_reset, _ingest_messages, handle_tool_call, _compress_impl, _schedule_rollup_maintenance. These three public methods do not.
disable_teams() is the sharp edge. It persists false and deletes TEAMS_ENABLED_ATTR. After that call storage_teams_enabled(self) is false, so _stored_access_scopes_for_targets returns () and _access_scope_for_storage_session returns None. Every per-principal predicate added by this PR goes inert while the stamps stay in the database. Any caller holding the engine object turns the whole feature off.
No lcm_* tool maps to these methods, so this is a posture gap rather than a proven exploit path. Gate them on owner_only (or admin) so the enforcement switch is at least as protected as a session reset.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@engine.py` around lines 893 - 974, Add the same authorization gate used by
protected mutating methods to enable_teams, disable_teams, and the setup_teams
alias, requiring owner_only or admin access before any persistence, catalog,
backfill, or in-process enforcement state mutation. Reuse the existing
authorization mechanism and preserve current behavior after authorization
succeeds.
Comment on lines
2836
to
+2839
| if "hermes_home" in kwargs: | ||
| self._rebind_storage_for_home(str(kwargs.get("hermes_home") or "")) | ||
|
|
||
| boundary_reason = str(kwargs.get("boundary_reason") or "") | ||
| old_session_id = str(kwargs.get("old_session_id") or "") | ||
| # boundary_reason / old_session_id are read above, before authorization. |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | 🏗️ Heavy lift
Authorization is resolved against the old store, then the store is switched.
Line 2808 resolves the policy from the currently bound store. _restore_persisted_teams_state derives the Teams marker from that store's connection, so policy_for_engine(self) reflects store A. Line 2837 then calls _rebind_storage_for_home, which runs _bind_storage and re-derives the Teams marker from store B. The rest of on_session_start mutates store B's lifecycle rows and DAG nodes under a decision that store A's policy granted.
If store A has Teams off and store B has Teams on, the gate is a permissive policy authorizing writes into a scoped store.
Re-authorize after the rebind, or authorize the hermes_home switch as its own target before applying it.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@engine.py` around lines 2836 - 2839, Update on_session_start so authorization
is evaluated against the storage selected by hermes_home: perform
_rebind_storage_for_home before resolving policy_for_engine(self), or explicitly
authorize the target home before switching. Ensure subsequent lifecycle-row and
DAG mutations use the policy derived from the newly bound store, while
preserving the existing boundary_reason and old_session_id handling.
Comment on lines
+485
to
+506
| for chunk_id in ordered_ids: | ||
| row = rows_by_id.get(chunk_id) | ||
| try: | ||
| stored_scope = row["access_scope"] if row is not None else None | ||
| except (IndexError, KeyError): | ||
| stored_scope = None | ||
| decision = policy.authorize_stored_scope( | ||
| access_context, | ||
| "read", | ||
| { | ||
| "target_id": chunk_id, | ||
| "kind": "chunk", | ||
| "access_scope": stored_scope, | ||
| }, | ||
| ) | ||
| policy.audit_decision( | ||
| access_context, "read", decision.denial_reason, decision.public() | ||
| ) | ||
| if not decision.allowed: | ||
| raise AuthorizationRequiredError( | ||
| "authorize_stored_scope", decision.public().denial_reason | ||
| ) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
An unresolved chunk id is authorized as an ownership claim.
The loop runs over ordered_ids, not over rows_by_id. When row is None, stored_scope becomes None and the chunk is still submitted to authorize_stored_scope. A policy that denies unowned targets then raises AuthorizationRequiredError for a chunk that does not exist.
engine.py Lines 4101-4103 state the opposite rule for the same class of lookup: "an absent row is not an ownership claim, and reporting one would turn a miss into a denial that leaks existence." The two sites disagree. The hydration loop at Line 508 already skips row is None, so skipping it here changes no delivered result.
🐛 Proposed fix
for chunk_id in ordered_ids:
row = rows_by_id.get(chunk_id)
+ if row is None:
+ # An absent row is not an ownership claim; the hydration loop
+ # below already drops it.
+ continue
try:📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| for chunk_id in ordered_ids: | |
| row = rows_by_id.get(chunk_id) | |
| try: | |
| stored_scope = row["access_scope"] if row is not None else None | |
| except (IndexError, KeyError): | |
| stored_scope = None | |
| decision = policy.authorize_stored_scope( | |
| access_context, | |
| "read", | |
| { | |
| "target_id": chunk_id, | |
| "kind": "chunk", | |
| "access_scope": stored_scope, | |
| }, | |
| ) | |
| policy.audit_decision( | |
| access_context, "read", decision.denial_reason, decision.public() | |
| ) | |
| if not decision.allowed: | |
| raise AuthorizationRequiredError( | |
| "authorize_stored_scope", decision.public().denial_reason | |
| ) | |
| for chunk_id in ordered_ids: | |
| row = rows_by_id.get(chunk_id) | |
| if row is None: | |
| # An absent row is not an ownership claim; the hydration loop | |
| # below already drops it. | |
| continue | |
| try: | |
| stored_scope = row["access_scope"] if row is not None else None | |
| except (IndexError, KeyError): | |
| stored_scope = None | |
| decision = policy.authorize_stored_scope( | |
| access_context, | |
| "read", | |
| { | |
| "target_id": chunk_id, | |
| "kind": "chunk", | |
| "access_scope": stored_scope, | |
| }, | |
| ) | |
| policy.audit_decision( | |
| access_context, "read", decision.denial_reason, decision.public() | |
| ) | |
| if not decision.allowed: | |
| raise AuthorizationRequiredError( | |
| "authorize_stored_scope", decision.public().denial_reason | |
| ) |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@retrieval_core.py` around lines 485 - 506, Update the loop around
authorize_stored_scope so entries with row is None are skipped before
authorization and auditing. Only call policy.authorize_stored_scope for chunk
IDs that have a corresponding row, preserving authorization behavior for
resolved chunks and allowing unresolved IDs to follow the existing hydration
skip behavior.
Comment on lines
+587
to
+615
| summary_ids: list[int] = [] | ||
| for embedded_id, _score, kind in ranked_rows: | ||
| if kind != "summary": | ||
| continue | ||
| try: | ||
| summary_ids.append(int(embedded_id)) | ||
| except (TypeError, ValueError): | ||
| continue | ||
| if len(summary_ids) >= knn_limit: | ||
| break | ||
| scopes_by_id: dict[int, object] = {} | ||
| batch_size = 500 | ||
| for start in range(0, len(summary_ids), batch_size): | ||
| require_remaining("summary scope lookup") | ||
| batch = summary_ids[start:start + batch_size] | ||
| placeholders = ",".join("?" for _ in batch) | ||
| try: | ||
| for row in conn.execute( | ||
| f"SELECT node_id, access_scope FROM summary_nodes " | ||
| f"WHERE node_id IN ({placeholders})", | ||
| batch, | ||
| ): | ||
| try: | ||
| stored_scope = row["access_scope"] | ||
| except (IndexError, KeyError): | ||
| stored_scope = None | ||
| scopes_by_id[int(row["node_id"])] = stored_scope | ||
| except sqlite3.OperationalError: | ||
| pass |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Scopes are fetched for the first knn_limit summaries, but more than that can be authorized.
Lines 595-596 stop collecting summary_ids at knn_limit. The authorization loop at Line 617 iterates every entry in ranked_rows, and the hydrated cap at Line 646 only stops the walk once a node has actually been resolved.
If any of the first knn_limit summaries yields read_dag.get_node(...) is None, the walk continues to a summary whose id was never included in the scope query. scopes_by_id.get(node_id) returns None for it, so the policy is told the node is unowned when in fact its stored scope was simply never read.
Collect summary_ids from all summary rows in ranked_rows, or fetch the scope lazily for any node id absent from scopes_by_id.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@retrieval_core.py` around lines 587 - 615, Update the summary scope-loading
logic around summary_ids and scopes_by_id to cover every summary entry in
ranked_rows, rather than stopping at knn_limit. Remove the early collection cap
or add an equivalent lazy lookup for summary IDs missing from scopes_by_id,
while preserving batching and existing authorization behavior.
Comment on lines
+626
to
+641
| decision = policy.authorize_stored_scope( | ||
| access_context, | ||
| "read", | ||
| { | ||
| "target_id": str(embedded_id), | ||
| "kind": "summary", | ||
| "access_scope": scopes_by_id.get(node_id), | ||
| }, | ||
| ) | ||
| policy.audit_decision( | ||
| access_context, "read", decision.denial_reason, decision.public() | ||
| ) | ||
| if not decision.allowed: | ||
| raise AuthorizationRequiredError( | ||
| "authorize_stored_scope", decision.public().denial_reason | ||
| ) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick win
Deny-aborts-all here, deny-drops-one in preanswer_evidence.
A single denied node raises and discards every already-authorized node in the ranked list. preanswer_evidence.authorize_supplied_baseline_refs takes the opposite approach for the same situation and documents why: "Unauthorized refs are dropped rather than raised on, so a partly-authorized payload still answers from the part the caller may see."
In a shared store the ranked list routinely mixes principals, so this path degrades to zero results whenever any foreign node ranks. tools.py then catches the error and falls back to FTS, so the user sees a quality loss with no explanation.
Skip denied nodes and continue, matching the baseline-ref behavior. Keep the audit call on every decision either way.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@retrieval_core.py` around lines 626 - 641, Update the authorization loop
around authorize_stored_scope so denied nodes are skipped and processing
continues with the remaining ranked nodes instead of raising
AuthorizationRequiredError. Preserve policy.audit_decision for every
authorization decision, and retain authorized nodes so partially authorized
results can proceed consistently with
preanswer_evidence.authorize_supplied_baseline_refs.
Comment on lines
+4822
to
+4850
| authorized_scope = policy.resolve_authorized_targets( | ||
| access_context, "read", expected_scope | ||
| ) | ||
| if isinstance(authorized_scope, Mapping): | ||
| authorized_scope = authorized_scope.get("target_scope", authorized_scope) | ||
| fts_args = { | ||
| "query": query, | ||
| "mode": "recall", | ||
| "session_scope": "all", | ||
| "limit": candidate_limit, | ||
| } | ||
| if isinstance(authorized_scope, Mapping): | ||
| # The resolved mapping is authoritative for the target dimension: a key | ||
| # the policy OMITS is a target it did not authorize, so the permissive | ||
| # default is REMOVED rather than left standing. Keeping the hard-coded | ||
| # session_scope="all" meant a policy narrowing the corpus to a single | ||
| # session -- or authorizing nothing at all -- still searched every | ||
| # session. Dropping the key degrades to this tool's own "current" | ||
| # default, which is the narrowest scope it offers. | ||
| for key in ("session_scope", "session_id", "source", "conversation_id"): | ||
| if key in authorized_scope: | ||
| fts_args[key] = authorized_scope[key] | ||
| else: | ||
| fts_args.pop(key, None) | ||
| # The owner predicate is ADDED, never removed by the loop above: a | ||
| # policy that scopes to a principal must be able to say so in a term | ||
| # the stored rows actually carry. | ||
| if authorized_scope.get("access_scope"): | ||
| fts_args["access_scope"] = authorized_scope["access_scope"] |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
access_scope is read from the unwrapped mapping here, but from the top level in engine.py.
Line 4826 replaces authorized_scope with its nested target_scope when that key is present. Line 4849 then reads access_scope from that nested mapping.
engine.py Lines 4236-4238 does the opposite, and says so explicitly: "Taken from the top-level resolved mapping, not from target_scope."
A policy that follows the engine.py convention — targets nested under target_scope, owner predicate at the top level — loses its predicate at this site. The omission-removal loop then also drops session_scope, so fts_args carries neither the owner predicate nor the hard-coded "all". The arm falls back to this tool's current default with no principal scoping.
Read access_scope from the top-level mapping before the unwrap, matching engine.py.
🔒 Proposed fix
authorized_scope = policy.resolve_authorized_targets(
access_context, "read", expected_scope
)
+ resolved_access_scope = (
+ authorized_scope.get("access_scope")
+ if isinstance(authorized_scope, Mapping)
+ else None
+ )
if isinstance(authorized_scope, Mapping):
authorized_scope = authorized_scope.get("target_scope", authorized_scope)
@@
- if authorized_scope.get("access_scope"):
- fts_args["access_scope"] = authorized_scope["access_scope"]
+ owner = resolved_access_scope or authorized_scope.get("access_scope")
+ if owner:
+ fts_args["access_scope"] = owner📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| authorized_scope = policy.resolve_authorized_targets( | |
| access_context, "read", expected_scope | |
| ) | |
| if isinstance(authorized_scope, Mapping): | |
| authorized_scope = authorized_scope.get("target_scope", authorized_scope) | |
| fts_args = { | |
| "query": query, | |
| "mode": "recall", | |
| "session_scope": "all", | |
| "limit": candidate_limit, | |
| } | |
| if isinstance(authorized_scope, Mapping): | |
| # The resolved mapping is authoritative for the target dimension: a key | |
| # the policy OMITS is a target it did not authorize, so the permissive | |
| # default is REMOVED rather than left standing. Keeping the hard-coded | |
| # session_scope="all" meant a policy narrowing the corpus to a single | |
| # session -- or authorizing nothing at all -- still searched every | |
| # session. Dropping the key degrades to this tool's own "current" | |
| # default, which is the narrowest scope it offers. | |
| for key in ("session_scope", "session_id", "source", "conversation_id"): | |
| if key in authorized_scope: | |
| fts_args[key] = authorized_scope[key] | |
| else: | |
| fts_args.pop(key, None) | |
| # The owner predicate is ADDED, never removed by the loop above: a | |
| # policy that scopes to a principal must be able to say so in a term | |
| # the stored rows actually carry. | |
| if authorized_scope.get("access_scope"): | |
| fts_args["access_scope"] = authorized_scope["access_scope"] | |
| authorized_scope = policy.resolve_authorized_targets( | |
| access_context, "read", expected_scope | |
| ) | |
| resolved_access_scope = ( | |
| authorized_scope.get("access_scope") | |
| if isinstance(authorized_scope, Mapping) | |
| else None | |
| ) | |
| if isinstance(authorized_scope, Mapping): | |
| authorized_scope = authorized_scope.get("target_scope", authorized_scope) | |
| fts_args = { | |
| "query": query, | |
| "mode": "recall", | |
| "session_scope": "all", | |
| "limit": candidate_limit, | |
| } | |
| if isinstance(authorized_scope, Mapping): | |
| # The resolved mapping is authoritative for the target dimension: a key | |
| # the policy OMITS is a target it did not authorize, so the permissive | |
| # default is REMOVED rather than left standing. Keeping the hard-coded | |
| # session_scope="all" meant a policy narrowing the corpus to a single | |
| # session -- or authorizing nothing at all -- still searched every | |
| # session. Dropping the key degrades to this tool's own "current" | |
| # default, which is the narrowest scope it offers. | |
| for key in ("session_scope", "session_id", "source", "conversation_id"): | |
| if key in authorized_scope: | |
| fts_args[key] = authorized_scope[key] | |
| else: | |
| fts_args.pop(key, None) | |
| # The owner predicate is ADDED, never removed by the loop above: a | |
| # policy that scopes to a principal must be able to say so in a term | |
| # the stored rows actually carry. | |
| owner = resolved_access_scope or authorized_scope.get("access_scope") | |
| if owner: | |
| fts_args["access_scope"] = owner |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tools.py` around lines 4822 - 4850, Capture access_scope from the top-level
resolved mapping before authorized_scope is replaced with target_scope, matching
the convention used by engine.py. Apply that captured owner predicate to
fts_args after the target-scope normalization, while preserving the existing
target-dimension omission handling.
Comment on lines
+7321
to
+7334
| scope_status = str(scope_storage.get("status")) | ||
| checks.append({ | ||
| "check": "scope_storage", | ||
| "status": ( | ||
| # stamped-without-marker is a FAILURE, not a variety of | ||
| # not-enabled: real per-owner stamps with no recorded decision. | ||
| # The previous mapping was an else-pass, so this state -- the | ||
| # one worth running a doctor for -- reported green. | ||
| "fail" if scope_status in {"fail", "stamped-without-marker"} | ||
| else "warn" if scope_status == "nothing-to-verify" | ||
| else "pass" | ||
| ), | ||
| "detail": scope_storage, | ||
| }) |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
An unknown status still reports green.
The comment correctly identifies the previous else-pass as the bug that made stamped-without-marker report healthy. The replacement keeps an else "pass" tail. Any status string verify_scope_storage gains later — a new partial or degraded state — lands in that tail and reports pass, reproducing the same class of failure the comment describes.
Enumerate the passing statuses instead, and map anything unrecognized to warn.
♻️ Proposed refactor
"status": (
"fail" if scope_status in {"fail", "stamped-without-marker"}
- else "warn" if scope_status == "nothing-to-verify"
- else "pass"
+ else "pass" if scope_status in {"ok", "not-enabled", "verified"}
+ # An unrecognized status is not evidence of health.
+ else "warn"
),📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| scope_status = str(scope_storage.get("status")) | |
| checks.append({ | |
| "check": "scope_storage", | |
| "status": ( | |
| # stamped-without-marker is a FAILURE, not a variety of | |
| # not-enabled: real per-owner stamps with no recorded decision. | |
| # The previous mapping was an else-pass, so this state -- the | |
| # one worth running a doctor for -- reported green. | |
| "fail" if scope_status in {"fail", "stamped-without-marker"} | |
| else "warn" if scope_status == "nothing-to-verify" | |
| else "pass" | |
| ), | |
| "detail": scope_storage, | |
| }) | |
| scope_status = str(scope_storage.get("status")) | |
| checks.append({ | |
| "check": "scope_storage", | |
| "status": ( | |
| "fail" if scope_status in {"fail", "stamped-without-marker"} | |
| else "pass" if scope_status in {"ok", "not-enabled", "verified"} | |
| # An unrecognized status is not evidence of health. | |
| else "warn" | |
| ), | |
| "detail": scope_storage, | |
| }) |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tools.py` around lines 7321 - 7334, Update the status mapping in the
scope_storage check to explicitly enumerate the known passing statuses instead
of using a catch-all else "pass". Preserve the existing fail mappings for "fail"
and "stamped-without-marker", retain "nothing-to-verify" as "warn", and map any
unrecognized status to "warn".
evaOS review status: completedPR: #215 - LCM Teams v1 — per-principal authorization evaOS review completed for this PR head. Automation note: agents should wait for this comment to reach PR URL: #215 Review URL: #215 (review) |
![evaos-code-review-bot[bot]](https://avatars.githubusercontent.com/in/4184532?s=60&v=4){kind=small}
There was a problem hiding this comment.
Walkthrough
PR: #215 - LCM Teams v1 — per-principal authorization
Head: eb7b22a4afadc5130bb7e5c13b55122a82ee8df3 into main. Review event: COMMENT.
Provider: Codex CLI (existing OAuth session) (codex-cli-oauth, codex-cli, model gpt-5.6-luna).
Estimated review effort: 5/5 (~70 min)
Changed Files
| File | Status | Churn | Purpose | Risk |
|---|---|---|---|---|
.github/workflows/ci.yml |
modified | +12/-1 | Changed file | Low |
__init__.py |
modified | +64/-1 | Changed file | Low |
access_context/__init__.py |
added | +49/-0 | Changed file | Low |
access_context/denials.py |
added | +190/-0 | Changed file | Low |
access_context/fixtures.py |
added | +89/-0 | Changed file | Low |
access_context/inventory.json |
added | +44/-0 | Changed file | Low |
access_context/inventory.py |
added | +128/-0 | Changed file | Low |
access_context/model.py |
added | +404/-0 | Changed file | Elevated: validated P1 finding |
access_context/protocols.py |
added | +83/-0 | Changed file | Low |
access_context/validation.py |
added | +308/-0 | Changed file | Elevated: large change |
access_policy/__init__.py |
added | +30/-0 | Changed file | Low |
access_policy/errors.py |
added | +26/-0 | Changed file | Low |
access_policy/fail_closed.py |
added | +83/-0 | Changed file | Low |
access_policy/resolution.py |
added | +217/-0 | Changed file | Elevated: large change |
access_policy/teams_policy.py |
added | +277/-0 | Changed file | Elevated: large change |
access_policy/trusted_owner.py |
added | +67/-0 | Changed file | Low |
assertion_state.py |
modified | +12/-2 | Changed file | Low |
assertion_store.py |
modified | +37/-1 | Changed file | Low |
aux_session.py |
modified | +62/-0 | Changed file | Low |
command.py |
modified | +192/-1 | Changed file | Low |
compaction.py |
modified | +47/-0 | Changed file | Low |
config.py |
modified | +34/-1 | Configuration | Low |
dag.py |
modified | +35/-6 | Changed file | Low |
db_bootstrap.py |
modified | +86/-12 | Changed file | Low |
docs/access-context-v1.md |
added | +57/-0 | Documentation | Low |
97 additional changed files omitted from this walkthrough.
Review Signal
Validated inline findings: 2 (P0: 0, P1: 1, P2: 1, P3: 0).
Dropped findings before posting: 0. High-severity findings: 1.
Risk Taxonomy
- Auth: 2
Validation and Proof
1 required validation/proof recommendation(s) selected from changed files.
- required: CI/release smoke proof - CI, release, launchd, or package metadata changed. Proof: green GitHub check; release-status; coverage-audit; rollback note.
Proof status: missing - 1 required validation/proof recommendation(s) missing from PR metadata.
Profile validation hints: Prefer correctness, security, data-loss, release, and regression findings over style-only feedback.
Profile proof expectations: Look for focused validation, rollback notes, and evidence appropriate to the changed surface.
Related Context
Related issues/PRs: stephenschoettler#482, stephenschoettler#473, #219.
Suggested labels: bug, docs, tests.
Suggested reviewers: none from current metadata.
Review Settings Preview
- Profile: assertive
- Enabled sections: Review summary (inline_review); Walkthrough (inline_review); Changed-files table (walkthrough); Effort estimate (walkthrough); Related issues/PRs (walkthrough); Review status comment (sticky_status)
- Path instructions: none
- Label suggestions: none
- Reviewer suggestions: none
- Suggestion behavior: suggestions only; labels and reviewers are not auto-applied.
- Roadmap-only settings: auto-apply labels; auto-request reviewers; required status checks
Pre-merge checklist
- Inline comments target current RIGHT-side diff lines.
- No secret-like content survived into posted inline comments.
- REQUEST_CHANGES is only used when eligible P0/P1 findings survive validation.
- Required behavior proof is present or not applicable.
- Labels and reviewers are suggestions only; the bot did not auto-apply them.
| if requested_operations is not None: | ||
| child_narrowing.update(f"operation:{item}" for item in child_grants) | ||
| if requested_collections is not None: | ||
| child_narrowing.update(f"collection:{item}" for item in child_collections) |
There was a problem hiding this comment.
P1: Empty collection narrowing clears the restriction
An explicit collections=[] is treated as a dimension to replace, but this update adds no replacement tokens after the parent collection:* tokens were removed. The returned context therefore has an empty collection_allowlist, which represents unrestricted scope, so narrowing a parent restricted to collection A can produce a child able to resolve collection B. Preserve the parent restriction or encode an explicit empty allowlist, and add a regression test.
Category: Auth
Why this matters: A delegated context can silently widen its collection authority at the authorization boundary.
| if child.context_id != parent.context_id: | ||
| if parent.context_id not in child.delegation_chain: | ||
| return False | ||
| if not parent.narrowing <= child.narrowing: |
There was a problem hiding this comment.
P2: Subset proof rejects valid nested narrowing
is_subset_of requires every raw parent narrowing token to remain in the child. However, narrow() deliberately removes all tokens in a dimension being narrowed before adding the child tokens. A valid redelegation from operation {read, write} to {read} (and similarly for collections or audiences) therefore returns false even though the effective authority is a subset. Compare effective restrictions rather than raw token-set inclusion, and cover a multi-level delegation chain.
Category: Auth
Why this matters: Consumers using this proof will reject legitimate nested Teams delegations, breaking the core authorization contract.
This was referenced Aug 12, 2026
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.
LCM Teams v1 — per-principal authorization
Makes
hermes_lcmenforce per-principal isolation: two principals share one store and neither can readthe other's memory. Built on the authorization seam from stephenschoettler#482/stephenschoettler#473.
Verified on real customer memory, not fixtures
Two real agent stores pulled read-only (consistent snapshot via the SQLite backup API — the stores are
WAL, so
cpcan tear), merged into one:Both halves are asserted every time. A policy that denies everything passes the first pair and fails the
second — and isolation-by-breaking-retrieval already happened once here, when narrowing on a collection id
returned an empty corpus to every principal. Only the positive control caught it.
Recall is scoped rather than truncated: 47 rows match
"Pipedream"across both principals, acorn getsexactly its 20 and carus exactly its 27, at
limit=200where the cap cannot bite. 20 + 27 = 47.Default-off is provably a no-op
22 of 22 query shapes byte-identical to stock
origin/main, including the ones that route around thenormal FTS path — CJK, emoji, bare FTS operators, a quoted phrase, the empty query — at two limits. This is
the property that matters for deployment: the build can ship with Teams switched off without perturbing
existing single-principal agents.
What it does
metadata, read before any path consults it; an aborted enable leaves the store fail-closed, never stamps-plus-permissiveteams/owns the three revisions; the host authenticates a principal and never sends a revisionTeamsPolicyaccess_scopein the FTS, LIKE-fallback, vector and chunk corpora, and in derived memorySeven cross-principal leaks found and closed
Found by adversarial audit and by CI, each reproduced on real data before and after:
lcm_grepreturned 27 of another principal's rows with verbatim contentpolicy_for_enginein their call chainASCII query was scoped
/lcm doctor cleanenumerated every principal's sessions, counts and tokens (the gate asked for anauthority the policy never read)
lcm_evidence_packreturned another principal'smessage content byte-for-byte
knn_chunks' binary prescreen ignoredaccess_scopewhile its summary twin guarded itlcm_query_statedisclosed assertions derived from every principal's messages, quote verbatimThe pattern behind them, and what now prevents it
Every one shipped with a green test beside it, because the tests injected a policy that denies and
asserted the handler propagated the denial. That proves the plumbing and says nothing about whether the
real policy ever produces a denial.
authorize_operationreads six scope keys and falls through toallow()for everything else, while gatesites across the package name 33. So a gate can name a key with a typo, a synonym, or a concept the policy
was never taught, and still run, still audit, and still permit everyone.
Three structural tests now make that class fail CI instead:
required_scopea gate names must be one the policy acts onEach caught real defects immediately. The target test found four more leaking targets than the audit
reported. The key test caught a regression I introduced myself, minutes after writing it.
Phase 4b replaces a completeness test that was structurally blind — it reconciled the inventory against
sites that already called the seam, so a handler that never called it was invisible. The replacement is
proven by three mutation tests, each demonstrated failing.
Known limits — please read before trusting this
profiles per principal are unmodelled; the catalog has no principals/collections/memberships accessors.
OWNERSHIP_CURRENTandLEASE_CURRENTremain inert.access_scopehas no index; the largest real store holds 51,502 messages. Fine for FTS, but the vectorcorpus does
ORDER BY … LIMITwhere an unindexed predicate can widen the scan.Suite
20 failed / 3252 passed locally. The 20 are pre-existing environment artifacts on this machine (macOS-only,
path-containment, host stub); CI's 3.11, 3.12 and 3.13 jobs pass, which is the measurement that
distinguishes them from real defects.
Summary by CodeRabbit
New Features
Bug Fixes
Documentation