Conversation
andrexibiza
left a comment
There was a problem hiding this comment.
Reviewed exact head 7628f96306e049593858b034ac6a67ff179eb68d against current main@98f758ae7e8db83c2bb9214c3b35adf41df15f03 and the current multiplex/launch-profile ownership work. The core direction here is right: dynamically registered model-provider metadata should not be one process-global namespace once one process serves multiple Hermes homes, and moving auth/runtime lookups through a profile-aware accessor is the correct architectural seam.
I found two composition blockers, one of them P1. Both come from the same underlying rule: profile authority is not identical to “a HERMES_HOME override exists,” and plugin declaration ownership is not identical to “Python imported this module in this profile.”
P1 — launch-profile provider resolution is rejected after multiplex activation
Current main deliberately represents the launch profile with no HOME override. tui_gateway.launch_profile_policy.launch_profile_runtime_scope() says this explicitly: the launch home is the process home, so it installs the launch secret/terminal scopes without a HERMES_HOME override. That contract is now consumed by #113163 for standalone gateway turns after hosted/multiplex activation, and it descends from the launch-profile fixes in #111620 / #112685.
This PR's _multiplex_state() instead treats is_multiplex_active() && !get_hermes_home_override() as proof that the caller is unscoped and raises. That conflates two different states:
- a valid launch-profile activity after another profile has activated multiplexing; and
- a truly unscoped worker/thread that lost its profile authority.
The first is an ordinary supported runtime path on current main. After B activates hosting, the next A/launch turn enters launch_profile_runtime_scope(process_home), then any plugin-provider lookup (get_provider_profile, list_providers, and therefore the new get_provider_config auth/runtime path) reaches _multiplex_state() and raises before it can resolve A's providers. The launch profile can therefore lose its model-provider registry simply because another profile was served.
Please preserve the current launch-profile authority contract while still refusing case (2). The state key needs positive profile ownership that can represent both routed homes and the launch home; loosening this to raw ambient process state for every override-less multiplex call would reopen the original cross-profile failure class. A good regression is the real A→B→A shape: activate multi-profile hosting, enter launch_profile_runtime_scope(get_process_hermes_home()), prove A's plugin/auth/runtime lookup still works after B, prove B remains isolated, and separately prove a genuinely unscoped multiplex worker still refuses.
P2 — bare self-registering entry-point providers only register into the first profile
The existing provider extension contract supports both module:func hooks and a bare self-registering module. The current tests/providers/test_entry_point_discovery.py witness is important: its bare module registers by import side effect via register_provider(...) and returns a non-callable object; it does not export PROFILE or PROFILES.
On this head, profile A's first ep.load() executes that import side effect under _REGISTRATION_TARGET=A, so A gets the provider. When profile B discovers the same entry point, Python returns the already-cached module and the module-level registration side effect does not run again. The new fallback only replays loaded.PROFILE / loaded.PROFILES, which the documented existing contract never required, so candidates is empty and B silently loses the provider. The new test happens to add a PROFILE attribute, which proves a new export convention rather than preserving the old self-registration convention.
Please keep the existing third-party contract. One viable shape is to capture the declarations/registrations produced by the first bare-module load and replay those declarations into later profile-owned registries; another is any equivalent mechanism that does not depend on an undocumented new export. The regression should use the old contract exactly: one cached bare module whose only behavior is module-level register_provider(...), no PROFILE/PROFILES, then A→B→A and prove the provider exists in each profile-owned namespace without cross-profile mutation.
Interlocks / provenance
This is complementary to the landed profile-runtime work, not a duplicate of it. #111620 established fail-closed multi-profile secret/runtime scoping; #112685 hardened launch-profile/routed-profile credential ownership; #113163 (with the #112884 salvage provenance retained there) made the launch profile explicitly re-enter its own runtime scope after hosted activation. This provider-registry change should compose on top of those ownership semantics rather than redefine “profile-scoped” as “has a HOME override.” The bare-entrypoint finding is compatibility with the provider plugin API already exercised by test_entry_point_discovery.py, so existing plugin authors should not need to adopt a new PROFILE export just to survive multiplexing.
Acceptance state
Current main and this head have actual merge base fb56a7e06dde62e9f645ff744c82cb47b60c469e; the branch is 1 ahead / 641 behind. GitHub currently reports the PR mergeable, but the semantic launch-profile contract above landed in that intervening history, so current-main composition needs to be proven after synchronization.
There is one surviving implementation commit, so exact-head and every-surviving-commit acceptance are the same object. Exact-head CI 35076386942, Docker 35076386575, and Nix 35076386516 all concluded action_required; the CI run created zero jobs. The focused local 9 passed + git diff --check receipt is useful, but there is no hosted acceptance verdict on this exact candidate yet.
This is a worthwhile isolation fix. Once the launch owner and the original entry-point contract are both represented explicitly, the overall registry split looks much closer to the right long-term shape.
| if not is_multiplex_active(): | ||
| return None | ||
| from hermes_constants import get_hermes_home, get_hermes_home_override, hermes_home_key | ||
| if not get_hermes_home_override(): |
There was a problem hiding this comment.
P1 — this rejects a valid launch-profile scope on current main. After multi-profile hosting is activated, the launch profile intentionally runs with is_multiplex_active() == True and no HERMES_HOME override: launch_profile_runtime_scope() binds its frozen launch secret/terminal authority while the process home remains the launch home. #113163 now uses that path for standalone turns after hosted activation. So a later launch-profile get_provider_profile() / list_providers() call reaches this branch and raises even though the caller is correctly scoped. Please distinguish the canonical launch-profile authority from a genuinely unscoped worker; do not make “override present” the proof of profile ownership. Add a real A→B→A launch-scope regression while keeping the truly-unscoped multiplex refusal.
| ) | ||
| else: | ||
| candidates = [] | ||
| profile = getattr(loaded, "PROFILE", None) |
There was a problem hiding this comment.
P2 — this changes the existing bare-entrypoint contract. Hermes already supports a bare self-registering module whose only effect is calling register_provider(...) at import time; the existing entrypoint test does not export PROFILE/PROFILES. A's first ep.load() runs that side effect under target A, but B gets Python's cached module, so the side effect does not rerun and candidates is empty. The new test avoids the failure by inventing a PROFILE export that old plugins were never required to provide. Please preserve the existing contract (for example, capture/replay the declarations registered by the first bare-module load) and regress with a real cached self-registering module that has no exported profile symbol.
|
Thanks @ggro4444 — you found the real defect here: one global provider registry discovered once per process, so a multi-profile gateway / Desktop backend never saw the plugins of any profile but the first-discovered one. This landed in a slimmer shape in #118582 (main SHA |
Summary
Verification
fb56a7e06dde62e9f645ff744c82cb47b60c469e7628f96306e049593858b034ac6a67ff179eb68dgit diff --check: PASS.env,auth.json, or credential storesRisk
This changes provider registry ownership, so the tests cover profile switching, registry restoration, runtime-provider resolution, and fail-closed secret scoping. The full repository suite remains CI-owned and is not claimed here.