fix(gateway): canonical routing identity — claimant generation proof pending - #89252
fix(gateway): canonical routing identity — claimant generation proof pending#89252andrexibiza wants to merge 11 commits into
Conversation
|
Applied the #88715 exact-blob installer onto current main (
Full CI matrix and a migration drill against real multiplexed stores are still required before changing the topology keyword (kept |
andrexibiza
left a comment
There was a problem hiding this comment.
Reviewed exact head 86a3355bcea8a0fe71bd4f063ecb72dc0b04da29 against base 8911e2e0edf750b104edbdc106d63d6cdac88524, the #88715 closure contract, merged #88734, and overlapping ingress work in #88437.
The direction is right: one explicit RoutingIdentity with separate transport/runtime/persistence ownership is the right class-level seam, and selecting the per-profile physical-store model is now consistent with merged #88734. The trusted carrier staying out of the wire representation is also the right security boundary. I would not merge this head yet, though, because exact-head CI exposed a missing contract on both sides of the seam.
Blocker 1 — the runner proof gate currently drops legitimate internal/control paths instead of requiring their producers to carry identity
resolve_identity_for_runner_source() deliberately rejects multiplexed sources when there is neither a live credential owner nor a trusted carried identity:
if multiplexed and owner is None and not has_trusted_carrier:
raise RoutingIdentityRejected(
"multiplexed source has no trusted transport binding"
)and routing_identity_entrypoint() catches that and returns None.
Fail-closed is the correct policy. The regression is that several existing legitimate paths reach the runner directly without first acquiring that proof, so the new choke point silently deletes their semantics:
- Python slice 3:
test_busy_session_ack.py::test_telegram_grace_followups_respect_queue_fifo— event is dropped,_pending_messages[sk]never exists; CI logs the exactmultiplexed source has no trusted transport bindingrejection. - Python slice 4: all three
test_multiplex_busy_input_mode.pyfailures — queue, steer, and restart-drain behavior are skipped for the same reason. - Python slice 7:
test_queue_consumption.py::test_rapid_text_followups_are_queued_in_fifo_order— same missing queued state.
That is not just fixture churn. #88715 explicitly includes control commands, held/replayed input, restart/recovery, callbacks, and alternate ingress in the class-closure matrix. The invariant should remain “no stateful use without canonical identity”, but every legitimate runner-direct producer must establish/restore that identity before crossing this boundary. Please fix those producer/redispatch/restart paths to attach a trusted RoutingIdentity or resolve a specific registered transport binding, then keep this rejection intact. Weakening the gate or guessing default would reopen the original defect class; merely teaching tests to inject arbitrary identity would hide it.
Blocker 2 — outbound notice delivery currently loses the transport binding
Python slice 1 fails test_notice_rendering.py::TestDeliverNoticeLine::test_public_delivery_sends_rendered_line: _deliver_platform_notice(source, line) reaches no adapter and adapter.send is awaited zero times.
That sits directly on the other half of #88715. Canonical runtime/session ownership is insufficient if status/progress/notice/final-response delivery cannot resolve the receiving credential owner. The authz_mixin.py changes are conceptually correct in preferring carried transport ownership over source.profile; the missing piece is ensuring sources reaching this delivery path actually have trusted transport provenance, or proving every production caller establishes it before delivery and updating the fixture to model that real contract. Please do not add a fallback from missing transport identity to runtime/default ownership — that would turn a visible drop into the exact wrong-bot guess this architecture is intended to eliminate.
CI / closure state
Docker is green, but the exact-head CI run is red in four Python slices. The focused 82-test suite therefore did not cover the full ingress/egress surface changed by this PR. The current Refs #88715 topology is correct and should remain that way until the full acceptance matrix and migration drill are complete.
Interlocks / provenance
- #88734 already selected per-profile physical
state.dbas repository truth. Its core fix and analysis came from #88632 by @jackulau and that provenance should remain explicit as this PR builds the identity layer on top of it. - #88437 by @69k4xmdfm2-blip is overlapping ingress/session-key work, not disposable duplication. This centralized seam is the stronger class-level home, but its two-bot / same-human collision behavior should survive as a composed regression witness and contributor credit should be preserved if #89252 supersedes that implementation.
Re-review gate for me: the four currently failing Python slices are green; legitimate runner-direct/restart/control paths establish identity rather than being dropped; notice/status/progress/final delivery demonstrably preserves transport ownership; and the hash-pinned repair dry-run/apply verification plus the #88715 composed matrix have receipts. I did not find a reason in this pass to reject the three-axis identity model, the per-profile physical-store choice, or the repair tool's fail-closed plan/apply shape.
0ee9ddf to
81dc144
Compare
|
Reconciled the routing-identity ingress with the existing gateway test surface — the full CI matrix is now green (32 success / 0 failure, mergeState CLEAN). The fail-closed gate on unbound multiplex sources is the intended security contract (the handoff's own contract tests enforce it), so the fix preserves it while making the existing tests represent proven adapter-routed sources:
Verified locally (109 tests across routing-identity, multiplex contract, profile resolution, adapter registry, session-db scope, quick-commands, busy-ack, queue, notice, repair) and the full CI run is green. Refs #88715 — full acceptance-matrix proof (migration drill + alternate-boundary matrix) still pending before changing the topology keyword. |
|
Status after commit 21260c3 (PR #89860, salvage of #88437) landed on main: the "stamp secondary ownership before adapter-local batching / session keying" half of this PR is now superseded — set_owner_profile + _session_key_profile deliver that behavior across all 8 adapters. The routing-identity architecture half (gateway/routing_identity.py, transport-profile authz resolution in authz_mixin, the session-store repair tool) is NOT on main and still stands on its own merits. If you'd like to carry this forward, a rebase is needed — the base.py handle_message/build_source hunks will conflict with the landed fix. Leaving open for review of the surviving half. |
andrexibiza
left a comment
There was a problem hiding this comment.
Re-reviewing exact head 1d9d65d16612d16a81775d82ee5e3bd63a11158e. This is materially new from the prior 86a3355 review: the branch added two follow-up commits, exact-head CI is now green, and — more importantly — main has since landed #89860 (salvage of #88437 by @69k4xmdfm2-blip), which changes the composition boundary for this PR.
The previous CI-visible runner/notice failures are cleared at this head. I also rechecked the surviving architecture: the RoutingIdentity split between transport/runtime/persistence remains the right class-level model; the default agent:main compatibility mapping is preserved; SessionEntry persists the trusted local carrier rather than leaking transport ownership onto the wire; the per-profile physical-store invariant is aligned with merged #88734 / @jackulau; and the repair utility is meaningfully fail-closed (dry-run first, byte-pinned plan hash, destination verify before source delete, backups, duplicate/schema checks, and an origin_session_id witness for durable async delegation rows).
I would still not merge the current head. Two blockers remain, one introduced by repository evolution and one in the surviving identity layer itself.
1. Restack on #89860 and use its owner-profile seam as the single transport-ownership authority
#89860 is now on main and supersedes the adapter-local session-key/batching half of this branch. It established BasePlatformAdapter.set_owner_profile() / _owner_profile plus _session_key_profile() across the eight adapter keying sites, and GatewayRunner._configure_profile_adapter() now calls that seam before busy/session-key work.
This branch predates that merge and independently introduces _multiplex_profile_name as the credential-owner carrier. routing_identity.py::_credential_owner_for_adapter() reads _multiplex_profile_name, while current main's adapter/session-key machinery reads _owner_profile. A straight merge would therefore leave two private owner-profile authorities whose equality is only incidental to two nearby writes in _configure_profile_adapter().
That is exactly the class #88715 is trying to eliminate: ownership should be established once and consumed, not independently re-stamped for different stateful consumers. The rebase should delete the superseded batching/keying implementation rather than recreate it, preserve @69k4xmdfm2-blip's #88437/#89860 credit, and make RoutingIdentity consume the already-landed owner binding (preferably through one explicit accessor rather than reaching into a second private attribute).
Required composition witness: on current main, prove both directions through the same owner seam:
- secondary-owned credential routed to
default: transport owner remains secondary, runtime/persistence are default, key namespace ismain; - shared/default credential routed to a named profile: transport owner remains default, runtime/persistence/key namespace are named.
Also retain the all-eight-adapter batching/keying matrix from #89860 after the restack. This PR should supersede only the architectural remainder of #88437, not silently fork its now-merged ownership primitive.
2. The advertised duplicate-credential fail-closed rule is not wired into production resolution
canonicalize_routing_identity() has the right policy hook:
credential_claimants: Optional[Iterable[object]] = None
...
if claimants is not None and len(claimants) > 1:
raise RoutingIdentityConflict(...)and test_duplicate_credential_claimants_reject_even_with_route proves that helper in isolation. But no production resolution path supplies credential_claimants. identity_from_source() does not accept it; resolve_identity_for_runner_source() resolves one owner from a concrete adapter/source and calls identity_from_source() without claimant-set evidence; adapter build_source() likewise canonicalizes from a single owner. So the guard is currently dead policy: the canonical ingress accepts an ambiguous credential topology because it never asks whether another served profile claims the same live credential.
That matters here because the module docstring and API explicitly treat transport ownership as an independent authority and say two adapters cannot safely poll/send through one credential concurrently. A unit-only parameter does not close that ambiguity at the stateful boundary.
Please either:
- plumb an authoritative claimant set/fingerprint from the adapter registry/configuration into canonicalization and reject >1 distinct profile owners before adapter-local state is touched; or
- narrow the PR's claimed invariant and remove the unused policy until the registry can actually prove it.
For the first option, the acceptance test needs to exercise the real adapter/runner registration path with two served profiles claiming the same credential — not call canonicalize_routing_identity() directly — and prove the event is rejected before batching/busy/session-key state.
Current CI / topology
Exact-head CI 32162277939 is green, and Docker is green. That clears the prior review's concrete failing-slice evidence, but it predates the semantic restack onto current main (1a19fedb5ae5d8f875dc2c14d302c251f8a7f542). Re-run the full exact-head matrix after composing #89860.
Interlocks/provenance:
- #88715 / @andrexibiza — architecture/closure issue; this PR should remain
Refs, as the body already says, until the alternate-boundary and migration matrix is complete. - #88734, salvaging #88632 / @jackulau — merged per-profile
state.dbinvariant. This PR correctly builds on that storage decision rather than re-originating it. - #89860, salvaging #88437 / @69k4xmdfm2-blip — now-merged adapter owner/session-key implementation. Its behavior and credit must be preserved; the overlapping branch code is superseded, while the three-axis
RoutingIdentity, authz consumption, durable carrier, and repair tooling remain complementary/superseding architecture. - The earlier exact-head review at
86a3355is not being repeated: its CI-visible failures are resolved. This review is about the repaired head plus the new mainline composition and the still-unwired ambiguity invariant.
Re-review gate: rebase/restack onto current main with one owner-profile source of truth; add real duplicate-credential ambiguity plumbing or explicitly descope it; preserve #89860's eight-adapter behavior/credit; then attach a fresh exact-head matrix and migration-drill receipt.
…ying (NousResearch#88715) Introduce a canonical three-axis routing identity (credential owner, runtime profile, persistence profile) so SessionSource.profile is no longer overloaded. - resolve profile routes before topic recovery, session keys, busy/clarify/ control paths, or callback state; - preserve the default profile's historical agent:main namespace; - stamp secondary credential ownership before adapter-local batching (adapter._multiplex_profile_name / build_source canonicalization); - persist trusted transport ownership in the local routing index (SessionEntry.routing_identity), not on the relay wire; - use the durable transport owner for restored outbound delivery (authz _authorization_adapter / _adapter_profile_for_source); - scope the whole ingress handler to the selected runtime/profile home (routing_identity_entrypoint / routing_identity_adapter_entrypoint); - build_session_key honors source.profile when profile= is omitted (does not erase a stamped runtime owner); - keep the per-profile physical store invariant from NousResearch#88734. Adds tools/repair_multiplex_session_stores.py: dry-run-first, hash-pinned, backup-backed repair for sessions already written to the wrong profile store. Applied via the exact-blob installer; 82 focused tests pass, ruff clean. Refs NousResearch#88715 Refs NousResearch#88734
…y test surface (NousResearch#88715) The fail-closed routing-identity gate is the intended security contract (claimed-but-unbound multiplex sources are dropped). The existing busy/queue/ multiplex/notice/quick-command tests construct sources without the trusted transport binding the adapter's build_source stamps in real operation, so they were dropped. Preserve the security contract and make the tests represent proven sources: - routing_identity.normalize_profile: treat non-string values (MagicMock auto-attributes) as absent, not as fabricated profile names (fixes the quick-commands MagicMock 'conflict'). - authz_mixin _authorization_adapter / _adapter_profile_for_source: only accept a real non-empty string transport profile, so a MagicMock _transport_profile no longer resolves to a phantom adapter (fixes queue/notice delivery). - test_busy_session_ack: runner is non-multiplex (multiplex_profiles=False) and the source carries the default-profile trusted binding. - test_multiplex_busy_input_mode: the _event source carries the trusted transport/persistence binding for its profile. Verified: 109 tests pass across routing-identity, multiplex contract, profile resolution, adapter registry, session-db scope, quick-commands, busy-ack, queue, notice, and repair suites; ruff clean.
Narrow the multiplex busy-input-mode test helper so it stamps the trusted transport/persistence binding only on sources with an explicit profile (those handed to the runner's fail-closed ingress). A profile=None source stays unbound so the adapter can stamp its own runtime profile, preserving the 'stamps profile before resolving mode' contract.
…Research#88715) The superseded base.py ownership-stamping (canonicalize_routing_identity + attach_identity_to_source inside BasePlatformAdapter.build_source, plus the routing_identity_adapter_entrypoint decorator on handle_message) now lives on main via set_owner_profile/_session_key_profile (NousResearch#89860). Drop that half from base.py so it is byte-identical to main; the routing-identity architecture survives on the runner ingress (routing_identity_entrypoint in gateway/run.py), authz_mixin transport-profile resolution, session.py persistence, turn_context, and the repair tool. Keep the routing_identity_adapter_entrypoint decorator defined in gateway/routing_identity.py (still covered by test_routing_identity.py). Tests updated to current-main behavior (no coverage dropped): - test_multiplex_routing_identity_contract: explicit attach_identity_to_source after ownership resolution (build_source no longer stamps it); rename the adapter-entrypoint test to exercise the surviving runner entrypoint with the source's transport-adapter ref so credential ownership resolves. - test_multiplex_busy_input_mode: pin the fixture gateway config to an empty platform set inside _start_one_profile_adapters (current main fail-fasts secondary profiles with a port-binding platform), matching the test_multiplex_adapter_registry.py seam. Targeted suite: 124 passed; ruff + py_compile + git diff --check clean.
1d9d65d to
96e6629
Compare
|
Rebased the surviving routing-identity half onto current main ( What changed in the rebase
Tests (no coverage dropped, updated to current-main behavior)
Head: |
|
CI fix: the only failing check was |
Restack audit — current head is green but not yet semantically restackedVerified exact head 1. The branch still duplicates main's adapter-owner seamCurrent main already gives adapter._multiplex_profile_name = profile_nameand Required restack:
2. Duplicate-credential rejection is not wired into production ingress
Required restack:
3. Unrelated image-generation policy is still mixed into the identity PRThe current diff changes Re-verification gateThe PR body still reports verification against I attempted to create a clean repair/restack ref on both the fork and upstream from the current base. GitHub rejected both writes with HTTP 403 |
…er seam Semantic restack per review; current head was green but carried three stale constructs that duplicated or over-reached main. 1. Drop the duplicate adapter-owner field. main (NousResearch#89860) already gives BasePlatformAdapter the canonical set_owner_profile/_owner_profile seam and _configure_profile_adapter calls it; the branch's ad-hoc adapter._multiplex_profile_name = profile_name was a second owner source. Removed the write in run.py and made _credential_owner_for_adapter read adapter._owner_profile, keeping the adapter-registry identity check as corroboration, not a second owner source. The contract test now stamps via set_owner_profile. 2. Remove the dead credential_claimants fail-closed parameter. canonicalize_routing_identity rejected >1 claimant but production never computed or passed it (only the leaf unit test exercised it). Duplicate-credential enforcement is a startup invariant already enforced by _configure_profile_adapter's claimed map via _adapter_credential_fingerprint. Removed the unwired parameter and its leaf test; documented the startup guarantee in the docstring. 3. Drop the unrelated image-generation upscale policy change from this branch so the routing change can be reviewed and reverted independently. Focused identity/adapter/session/repair suites: 123 passed.
f4f77b2 to
eaf71c8
Compare
|
All three gaps are closed on the restacked head Gap 1 — duplicate adapter-owner seam: fixed. main already calls Gap 2 — dead Gap 3 — unrelated image-generation change: dropped. The Re-verification: rebased on current main |
|
Thanks — carrying the surviving half forward as suggested. Done and verified:
Head |
|
Exact-head follow-up at The #89860 overlap is correctly gone: this head consumes One architecture gate from the requested restack is still not satisfied: claimant ambiguity is only enforced at startup for duplicate live credentials, not at the durable/restored identity resolution boundary. The removed Current production resolution has two proof modes:
The missing witness is the transition between persisted identity and the current live registry when ownership can no longer be proven unique (credential rotation, stale repair input, source recreation, multiple valid claimant records). The contract should be explicit at the consumer that binds persisted/restored identity back to current runtime ownership:
Do not weaken Keep |
andrexibiza
left a comment
There was a problem hiding this comment.
Re-review of exact head eaf71c8ada68ef7416b95b43816c31f3f6766a16 after the #89860 restack.
The overlap blocker is closed correctly. This head consumes set_owner_profile() / _owner_profile from merged #89860, drops the duplicate adapter-key implementation, retains @69k4xmdfm2-blip’s ownership/session-key behavior and credit, removes the unrelated image-default change, and keeps the complementary three-axis RoutingIdentity, authorization consumption, durable carrier, per-profile persistence model, and repair utility. Docker and Nix are green; every ordinary-CI job exposed by the connector is green, including all exposed Python slices, e2e, attribution, OSV, ruff/ty, blocking ruff, Windows footguns, Windows-only, and macOS-only. The aggregate remains red outside those exposed jobs.
One architecture blocker remains: duplicate-credential claimant ambiguity was deleted rather than wired into production identity resolution.
The branch removed the unwired credential_claimants parameter and now treats duplicate credentials as startup-only policy in _start_one_profile_adapters(). That does close the dead helper-test mismatch, but it does not satisfy the durable routing contract introduced by this PR:
- startup records the first claim in a transient
claimedmap and marks a later profileduplicate_credentialfatal; _served_profiles_for_runner()can still include that rejected profile throughprofiles_to_serve();- a restored source’s
_transport_profile/routing_identity.transport_profileis accepted by_credential_owner_for_source()before resolving a concrete registered adapter or current credential claim; - the persisted carrier contains only profile names, not a claim ID/fingerprint generation that can prove the same credential owner still exists after restart, reconnect, config rotation, or duplicate-claim rejection.
So the durable carrier can currently self-certify a served-but-unbound transport profile, or bind to a different credential later installed under the same profile name. The failure may surface downstream as a dropped authorization/delivery adapter, but by then canonical identity has already been admitted and stateful runtime/session work may have occurred. That violates #88715’s explicit rule for restored/synthetic sources: fail closed unless an actual transport binding can be resolved; never guess another bot.
Required production contract
Make the adapter/credential registry publish a non-secret claim identity with at least:
platform + credential-claim-id + owner-profile + adapter-generation
and a verdict:
unique | ambiguous | unavailable
Then make resolve_identity_for_runner_source() consume that registry verdict. A carried transport_profile may select the expected owner, but it may not prove current ownership by itself.
- Multiple profiles claiming one credential: mark the claim ambiguous and reject before adapter-local/runner stateful use; do not turn registration order into authority.
- No live/current claim for a restored carrier: reject before session lookup/runtime work.
- Credential or adapter generation changed under the same profile: stale durable bindings reject or reconcile explicitly; profile-name equality is insufficient.
- Unique live claim: attach the current claim/generation to the canonical identity and persist the non-secret binding needed for restart recovery.
Required witnesses
- Two served profiles configure the same credential through the real startup path; a restored/internal source naming either claimant is rejected before batching, busy state, session lookup, authorization, or delivery.
- A persisted source from adapter generation N is replayed after the profile rotates to another credential/generation N+1; it does not deliver through N+1.
- A unique claim survives restart/reconnect and restores the same transport owner.
- Shared-credential routing remains valid when there is intentionally one live credential owner serving multiple runtime profiles; runtime routing must not be mistaken for an additional credential claim.
Keep Refs #88715 / Refs #88734. The semantic restack and exposed CI are strong, but the migration drill and the production claimant/generation proof remain real class-closure gates.
Exact current-base CI receipt — architecture blocker unchangedHead Current merge candidate:
The immediately preceding refresh run Executable verification is closed. The formal review’s durable claimant/generation finding remains unresolved: green CI does not prove a persisted transport profile is bound to a unique current credential claim across restart, reconnect, rotation, or duplicate rejection. Keep this PR WIP / |
Architecture interlock — #90145 is the canonical generation-authority contractThis PR already has the correct merge gate; this comment records the repository-level reason so it does not get weakened during later review/rebase. The durable That is the routing instance of #90145:
Keep the existing claimant/generation blocker as a merge gate. Do not replace it with uniqueness-at-startup, profile-name equality, endpoint equality, or “only one current candidate” inference. The critical ABA witness remains: Composition:
No new abstraction is required here; the existing |
Install temporary exact-branch authority to recover, validate, and publish the claimant-generation implementation.
Current disposition
This branch has been semantically restacked on merged #89860 and preserves its contributor provenance. The duplicate adapter-owner/session-key implementation is gone; the surviving architecture consumes
set_owner_profile()/_owner_profileas the one live adapter ownership seam.One architecture blocker remains before merge: durable/restored transport ownership is still admitted by profile name without a current credential-claim/generation proof. Startup duplicate detection is necessary but insufficient for the durable routing model introduced here.
Required completion:
platform + credential-claim-id + owner-profile + adapter-generationwithunique | ambiguous | unavailableverdict;resolve_identity_for_runner_source()consume that current registry verdict—carriedtransport_profilemay select the expected owner but cannot prove it;Keep
Refs #88715/Refs #88734until those gates are satisfied.Summary
Introduce a canonical multiplex routing identity so credential ownership, runtime ownership, and physical session ownership are no longer conflated in
SessionSource.profile.agent:mainnamespace;Composition with #89860
Merged #89860, salvaging #88437 by @69k4xmdfm2-blip, owns adapter-local owner stamping and the eight-adapter session-key/batching behavior. This branch no longer carries the duplicate
_multiplex_profile_nameimplementation. It consumes the landedset_owner_profile()seam and retains the complementary architecture:RoutingIdentity;#89860's behavior and authorship remain authoritative; this PR does not silently re-originate them.
Storage decision
One physical
state.dbper active profile. This composes with merged #88734 and does not adopt the closed shared-store reader model. The storage decision preserves #88632/@jackulau provenance through #88734.Safety
The repair command is read-only by default. Apply requires
--yes,--gateway-stopped, and the exactplan_sha256emitted by the reviewed dry run. Destination copies are committed and verified before source rows are deleted; every touched DB is backed up with SQLite's backup API.The routing proof gate remains fail-closed. Do not repair the claimant blocker by guessing
default, treating runtime ownership as transport ownership, or allowing a persisted profile name to self-certify a credential.Exact current-base verification
Head:
eaf71c8ada68ef7416b95b43816c31f3f6766a16.Current base at dispatch:
87e32b6b30f3e5113e26ac7468319d000a3affac.Current merge candidate:
825d9c1479a56d1bc7c6aae07b524b099eaefa5d.32286897668: success;32286896514: success;32286896634: success;py_compile, attribution audit, andgit diff --check: clean.The prior refresh run
32285484604checked out stale merge ref991f63cf...on base13ce0c5c...and failed only the then-existing image-upscale invariant. That run is superseded by the successful current-base matrix above.Executable verification is now closed. The claimant/generation proof and real migration drill remain architecture gates; green CI does not waive them.
Required claimant/generation witnesses
Interlocks and credit