feat(plugins): support root-owned discovery - #12
Conversation
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 472b17ac8a
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
|
||
| # 2. User plugins (~/.hermes/plugins/) | ||
| user_dir = get_hermes_home() / "plugins" | ||
| user_dir = get_user_plugins_dir() |
There was a problem hiding this comment.
When the gateway is started through its documented python -m gateway.run entry point, hermes_cli.main is never imported, and no code in that path calls configure_user_plugins_root(). Consequently this lookup treats managed mode as disabled and scans <HERMES_HOME>/plugins, allowing the gateway to load a profile-local plugin instead of the administrator-published tree. Resolve plugins.user_root in a shared loader or explicitly initialize it in every supported entry point.
AGENTS.md reference: AGENTS.md:L84-L87
Useful? React with 👍 / 👎.
| _early_plugins_cfg = {} | ||
| if not isinstance(_early_plugins_cfg, dict): | ||
| raise _UserPluginsRootError("invalid_user_plugins_root: plugins must be a mapping") | ||
| _configure_user_plugins_root(_early_plugins_cfg.get("user_root")) |
There was a problem hiding this comment.
If the user's config.yaml is malformed or unreadable, the broad exception above leaves _early_cfg_raw as {}, so this line clears the internal managed-root bridge and silently restores <HERMES_HOME>/plugins. This also bypasses a root pinned through the managed /etc/hermes overlay because that overlay is never reached after the parse failure; an unprivileged user can therefore corrupt their own YAML and make startup discover a same-named plugin from their writable tree. The privileged root resolution must distinguish an absent key from a failed config/overlay read and abort in the latter case.
AGENTS.md reference: AGENTS.md:L84-L87
Useful? React with 👍 / 👎.
| with pytest.raises(hc.UserPluginsRootError, match="must be owned by root"): | ||
| hc.configure_user_plugins_root(ordinary) |
There was a problem hiding this comment.
When pytest runs as UID 0, ordinary and its pytest-created ancestors are root-owned, so this assertion does not observe the expected ownership error (on the usual /tmp layout it instead sees the world-writable /tmp error). The related non-root-ancestor test can fail to raise for the same reason. Construct the intended st_uid values in the mocked lstat results rather than relying on the account running the suite.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f3c0d7ee24
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| current = raw | ||
| while True: | ||
| info = current.lstat() | ||
| if stat.S_ISLNK(info.st_mode): |
There was a problem hiding this comment.
When a root-owned publication directory contains a user-owned/writable plugin subdirectory, source file, or symlink to a writable target, this validation still succeeds because the loop inspects only the configured root and its parents. The general and specialized loaders subsequently import those descendants, allowing the service user to rewrite executable code despite managed mode treating the tree as administrator-controlled and read-only. Validate the ownership, modes, and symlink status of each loaded plugin path or recursively enforce the contract before enabling the root.
AGENTS.md reference: AGENTS.md:L84-L87
Useful? React with 👍 / 👎.
| externally_managed | ||
| and incumbent is not None | ||
| and incumbent.source == "user" | ||
| and manifest.source != "user" | ||
| ): |
There was a problem hiding this comment.
When the managed tree provides a context engine with the same name as a bundled engine (for example, context.engine: lcm), this winner selection does not control context-engine bootstrap: agent/agent_init.py:2310-2326 loads plugins/context_engine/<name> first and consults get_plugin_context_engine() only if the bundled lookup returns nothing. The bundled implementation therefore runs instead of the administrator-published version, contrary to the managed-root collision contract applied to the other specialized provider paths.
AGENTS.md reference: AGENTS.md:L54-L57
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c25eee5a3a
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| def configure_user_plugins_root_from_startup_config( | ||
| *, | ||
| user_config_path: Path | None = None, | ||
| ) -> Path | None: |
There was a problem hiding this comment.
This helper is currently called only by hermes_cli.main and gateway.run, but the documented python -m tui_gateway.entry path (ui-tui/README.md:16) and python -m acp_adapter entry point (acp_adapter/__main__.py:1-5) also construct agents and discover plugins without importing either caller. When those processes start directly with plugins.user_root configured, the internal bridge remains unset and plugin discovery falls back to the writable <HERMES_HOME>/plugins tree; initialize this boundary before tui_gateway.server/ACP agent imports as well.
AGENTS.md reference: AGENTS.md:L54-L57
Useful? React with 👍 / 👎.
| return d if d.is_dir() else None | ||
| except Exception: | ||
| return None | ||
| d = get_user_plugins_dir() / "model-providers" |
There was a problem hiding this comment.
If the managed root is removed or made insecure between startup validation and lazy model-provider discovery, this now raises from get_user_plugins_dir() after _discover_providers() has already set _discovered = True. Several callers swallow that first exception, such as agent/agent_init.py:132-142; every later get_provider_profile() then skips discovery and returns the bundled/legacy registrations accumulated before this lookup, bypassing the managed-root failure. Reset _discovered when discovery raises, as the general plugin manager already does.
AGENTS.md reference: AGENTS.md:L84-L87
Useful? React with 👍 / 👎.
| if user_dir is not None and externally_managed: | ||
| for child in sorted(user_dir.iterdir()): | ||
| if not child.is_dir() or child.name.startswith(("_", ".")): | ||
| continue | ||
| _import_plugin_dir(child, "user") |
There was a problem hiding this comment.
When a managed model-provider collides with a bundled or legacy provider but raises during import, _import_plugin_dir() catches the exception and returns without removing the earlier registration. This new final import pass therefore leaves get_provider_profile() serving the bundled/legacy implementation despite the managed tree being authoritative, potentially selecting different credentials or endpoints. In managed mode, an import failure for an authoritative provider must fail discovery or otherwise suppress the colliding registration rather than silently falling back.
AGENTS.md reference: AGENTS.md:L84-L87
Useful? React with 👍 / 👎.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c25eee5a3a
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| # The gateway is also a supported direct process entrypoint | ||
| # (``python -m gateway.run``), so it cannot rely on hermes_cli.main import side | ||
| # effects to establish the executable-code discovery boundary. | ||
| configure_user_plugins_root_from_startup_config() |
There was a problem hiding this comment.
When python -m gateway.run starts with plugins.user_root, this call runs only after agent.conversation_compression imports agent.model_metadata; agent/model_metadata.py:511-514 immediately calls providers.list_providers(), completing discovery against the writable profile-local plugin tree and setting _discovered. The managed root is therefore configured too late to prevent a profile-local model provider from remaining registered. Fresh evidence in this revision is that the gateway bootstrap call now exists, but its placement after the agent imports still leaves the discovery boundary open; run it before the first agent import.
AGENTS.md reference: AGENTS.md:L54-L57
Useful? React with 👍 / 👎.
| if user_dir is not None and externally_managed: | ||
| for child in sorted(user_dir.iterdir()): | ||
| if not child.is_dir() or child.name.startswith(("_", ".")): | ||
| continue | ||
| _import_plugin_dir(child, "user") |
There was a problem hiding this comment.
When a managed provider's canonical name equals an alias already registered by a bundled or legacy provider—for example, a managed profile named claude while the bundled anthropic profile owns that alias—this final import writes _REGISTRY["claude"] but leaves _ALIASES["claude"] = "anthropic". Because get_provider_profile() resolves aliases before consulting the registry, requests for the managed provider still select the bundled endpoint and credentials despite this authoritative final pass. Clear conflicting aliases or otherwise give managed canonical names precedence.
AGENTS.md reference: AGENTS.md:L54-L57
Useful? React with 👍 / 👎.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: cb9a4d6d25
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| *, | ||
| user_config_path: Path | None = None, | ||
| ) -> Path | None: | ||
| """Resolve and install ``plugins.user_root`` for every process entrypoint. |
There was a problem hiding this comment.
When an operator launches the installed hermes-agent command, pyproject.toml:337 imports run_agent:main directly, but run_agent.py imports model_tools and discovers plugins without calling this bootstrap helper. A configured managed root is therefore ignored and discovery uses the writable profile-local tree. Fresh evidence beyond the earlier entry-point comment is the still-uncovered hermes-agent console script; initialize the boundary before run_agent.py imports model_tools.
AGENTS.md reference: AGENTS.md:L54-L57
Useful? React with 👍 / 👎.
| if _selected_engine is None and not _copy_failed: | ||
| try: | ||
| from plugins.context_engine import load_context_engine | ||
| _selected_engine = load_context_engine(_engine_name) |
There was a problem hiding this comment.
When a root-managed context-engine plugin collides with a bundled engine but fails to import/register, _load_general_context_engine() returns None and this branch immediately loads the bundled implementation. Thus the administrator-authoritative engine silently falls back to different code instead of failing closed. Fresh evidence beyond the earlier precedence comment is that managed discovery now runs first, but its failure result still enters this bundled fallback.
AGENTS.md reference: AGENTS.md:L54-L57
Useful? React with 👍 / 👎.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 67b364c3a5
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| *, | ||
| user_config_path: Path | None = None, | ||
| ) -> Path | None: | ||
| """Resolve and install ``plugins.user_root`` for every process entrypoint. |
There was a problem hiding this comment.
When plugins.user_root is configured and an operator uses the documented python batch_runner.py ... entry point, batch_runner.py:49 imports run_agent, which reaches model_tools.py:215 and performs plugin discovery before this startup bridge is ever called. The batch process therefore falls back to the writable profile-local plugin tree, allowing unintended code to run in every experiment worker instead of using the administrator-published tree. Fresh evidence beyond the earlier entry-point comments is the separately documented batch-runner path in website/docs/guides/python-library.md:180-186; invoke this bootstrap before its agent/model-tools imports as well.
AGENTS.md reference: AGENTS.md:L54-L57
Useful? React with 👍 / 👎.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: cbf884b9cf
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| value = user_plugins.get("user_root") | ||
| if "user_root" in managed_plugins: | ||
| value = managed_plugins["user_root"] |
There was a problem hiding this comment.
When plugins.user_root is configured using an established config reference such as ${env:PLUGIN_ROOT}, this startup reader passes the unexpanded template to configure_user_plugins_root(), which rejects it as a relative path and prevents every bootstrapped entry point from starting. The normal config path recursively expands both user and managed values in hermes_cli/config.py:7852-7862; apply the same expansion before selecting and validating user_root. The new entry-point tests use only literal paths, so they do not exercise this config-propagation path.
AGENTS.md reference: AGENTS.md:L84-L87
Useful? React with 👍 / 👎.
|
@codex review |
|
Codex Review: Something went wrong. Try again later by commenting “@codex review”. ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: aa87703b41
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| "plugins": { | ||
| # Optional administrator-owned, read-only plugin publication tree. | ||
| # Empty preserves the legacy <HERMES_HOME>/plugins behavior. | ||
| "user_root": "", | ||
| }, |
There was a problem hiding this comment.
Because adding this default automatically exposes plugins.user_root in CONFIG_SCHEMA, the dashboard can save the setting through PUT /api/config, but that endpoint only writes YAML and never calls configure_user_plugins_root_from_startup_config() or reports that a restart is required. If a running dashboard switches from the default to a managed root, subsequent discovery and install operations continue using the writable <HERMES_HOME>/plugins tree for the rest of the process, despite the UI showing the administrator-owned root. Apply and validate the bridge while saving (including invalidating discovery caches), or reject the live change with an explicit restart requirement.
AGENTS.md reference: AGENTS.md:L84-L87
Useful? React with 👍 / 👎.
| if user_dir is not None and externally_managed: | ||
| for child in sorted(user_dir.iterdir()): | ||
| if not child.is_dir() or child.name.startswith(("_", ".")): | ||
| continue | ||
| if not _import_plugin_dir(child, "user"): | ||
| raise RuntimeError("managed_provider_import_failed") |
There was a problem hiding this comment.
This managed-provider path still delegates to _import_plugin_dir(), which uses spec.loader.exec_module() and can therefore execute a timestamp-valid stale .pyc. If an administrator publishes updated same-sized source while preserving its second-resolution mtime and the tree contains bytecode from the prior release, Hermes silently runs the old provider code; the memory, cron, and dashboard API managed paths retain the same loader behavior. The source-byte execution added for the general PluginManager needs to cover these sibling managed import paths as well.
AGENTS.md reference: AGENTS.md:L54-L57
Useful? React with 👍 / 👎.
|
@codex review |
|
Codex Review: Didn't find any major issues. Already looking forward to the next diff. Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 577ff40c81
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| new = _plugins_section(replacement, scope="replacement").get("user_root") | ||
| return old != new |
There was a problem hiding this comment.
When plugins.user_root comes from the managed overlay, existing is the raw user YAML and normally has no value, while the dashboard/desktop received the overlaid value from GET /api/config and sends it back in every full-config save; environment references have the same raw-versus-expanded mismatch. This comparison therefore reports a root change and returns 409 even when the user edits an unrelated setting, effectively disabling normal config saves in these managed deployments. Compare normalized effective roots or exclude the unchanged startup-only field from the submitted replacement.
AGENTS.md reference: AGENTS.md:L84-L87
Useful? React with 👍 / 👎.
| ): | ||
| spec.loader = _ManagedSourceLoader(fullname, spec.origin) | ||
| spec.cached = None | ||
| return spec |
There was a problem hiding this comment.
In managed mode this finder is inserted at sys.meta_path[0], but it returns PathFinder's spec even when the source is outside the managed root. Consequently any later custom meta-path finder that would instrument or replace an otherwise filesystem-resolvable module—such as tracing, coverage, or assertion-rewriting hooks—is bypassed process-wide. Return the modified spec only for managed sources and return None otherwise so normal import resolution continues.
Useful? React with 👍 / 👎.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d52ce13af4
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| for nested in sorted(child.iterdir()): | ||
| if not nested.is_dir() or nested.name.startswith("."): | ||
| continue | ||
| plugin_dirs.append((nested, f"{child.name}/{nested.name}")) |
There was a problem hiding this comment.
When a real category contains a manifest-less fixture, example, or vendor directory with pinned dependencies, this unconditional append audits that directory even though PluginManager._scan_directory_level() skips manifest-less children at the category depth. For example, <root>/image_gen/real/plugin.yaml plus <root>/image_gen/examples/requirements.txt produces OSV findings attributed to plugin:image_gen/examples, although Hermes never loads it. Fresh evidence beyond the earlier flat-plugin report is that the new has_manifest guard fixes flat plugins but actual categories still append every child without checking for a plugin or dashboard manifest.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4cd889813d
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| # user config, whose references intentionally retain profile semantics. | ||
| from hermes_cli.env_loader import load_hermes_dotenv | ||
|
|
||
| load_hermes_dotenv(hermes_home=user_path.parent) |
There was a problem hiding this comment.
When a legacy profile sets HERMES_SAFE_MODE=1 in its .env—a mode upgrade that _load_hermes_dotenv() explicitly preserves—the safe_mode value used above was captured before this load. Bootstrap therefore continues to resolve and validate plugins.user_root; an absent or insecure configured root aborts startup instead of entering safe mode, while a valid root leaves the managed bridge installed contrary to the stated safe-mode boundary. Re-check safe mode immediately after this call and clear/return before processing the plugin root, with a real-entrypoint temporary-HERMES_HOME test.
AGENTS.md reference: AGENTS.md:L84-L87
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c9133e5b39
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| key = manifest.key or manifest.name | ||
| if key == name or key == f"context_engine/{name}": |
There was a problem hiding this comment.
When a managed plugin's directory key differs from its manifest name—for example, directory corporate declaring name: foo—and the plugin is disabled, this check returns false for configured engine foo because it compares only the path-derived key. init_agent() can then load the bundled plugins/context_engine/foo implementation, bypassing the managed publication's public-name reservation. Include manifest.name in the claimed identities and cover the disable-to-agent-resolution path with a real temporary managed root.
AGENTS.md reference: AGENTS.md:L84-L87
Useful? React with 👍 / 👎.
| with tool_registry._lock: | ||
| tool_registry._tools.clear() | ||
| tool_registry._tools.update(tool_registry_state[0]) | ||
| tool_registry._plugin_override_policy.clear() | ||
| tool_registry._plugin_override_policy.update(tool_registry_state[1]) |
There was a problem hiding this comment.
When an earlier managed plugin registers an image, video, web, browser, TTS, transcription, dashboard-auth, or secret-source provider and a later plugin fails, this rollback restores only manager, tool, and platform state. Fresh evidence after the earlier rollback report is that registries such as agent.image_gen_registry._providers are still never snapshotted or restored; if the publication is repaired by removing or disabling the first plugin and discovery is retried in-process, its stale provider remains selectable. Make registration transactional across these sibling registries as well.
AGENTS.md reference: AGENTS.md:L54-L57
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0fa5093192
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| # Python plugin manifest/module. Treat that complete layout as a | ||
| # terminal managed plugin while leaving malformed or incomplete | ||
| # dashboard directories for the dashboard scanner to reject. | ||
| if (child / "dashboard" / "manifest.json").is_file(): | ||
| continue |
There was a problem hiding this comment.
When a managed tree contains a valid selected memory, cron, or model provider plus a dashboard-only sibling whose manifest.json is malformed, this existence-only branch declares the sibling complete. Fresh evidence beyond the dashboard-startup parse check is that the direct provider resolvers call validate_managed_plugin_generation() but never _discover_dashboard_plugins(), so they can import the selected Python provider while accepting a partially published generation. Parse and validate dashboard manifests in the shared generation validator rather than deferring validation to dashboard startup.
AGENTS.md reference: AGENTS.md:L54-L57
Useful? React with 👍 / 👎.
| r"[A-Za-z0-9_][A-Za-z0-9_.-]{0,127}" | ||
| if managed | ||
| else r"[A-Za-z0-9][A-Za-z0-9_.-]{0,127}", |
There was a problem hiding this comment.
When managed memory providers foo and foo.bar coexist and foo contains bar.py, this new validation accepts both names, but discovery loads that helper as _hermes_user_memory.foo.bar; the later foo.bar provider then hits the module cache and reuses the helper instead of its own __init__.py, producing incorrect registration or availability results. Encode the directory identity before constructing module names and eviction prefixes; the cron-provider loader repeats the same collision.
AGENTS.md reference: AGENTS.md:L54-L57
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d4c67e372c
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
|
||
| validate_managed_plugin_generation(user_dir) | ||
| for name, child in _iter_managed_provider_dirs(user_dir): | ||
| seen.add(name) |
There was a problem hiding this comment.
When managed directory foo declares public manifest name bar, this records only foo as seen. A bundled memory provider named bar is therefore still imported during discovery—even when the managed provider is disabled through the supported bar identity—and find_provider_dir("bar") can likewise fall through to it. Reserve every value from _managed_provider_identities() before scanning lower-authority providers; the cron-provider loop has the same gap.
AGENTS.md reference: AGENTS.md:L54-L57
Useful? React with 👍 / 👎.
| if not isinstance(dashboard_data, dict): | ||
| raise TypeError("dashboard manifest must be a JSON object") |
There was a problem hiding this comment.
When a managed dashboard-only sibling contains a JSON object with an invalid name such as {"name": []}, this shared scan accepts it, so a direct memory, cron, or model-provider resolver can import the selected Python provider. Dashboard discovery later rejects the same generation when it uses that list as a key at hermes_cli/web_server.py:19539, meaning the supposedly complete publication is accepted on one executable path and rejected on another. Reuse the dashboard manifest schema and cross-manifest validation here instead of checking only that the root is an object.
AGENTS.md reference: AGENTS.md:L54-L57
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 30563eb0a6
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if child.name in disabled_managed_identities: | ||
| continue |
There was a problem hiding this comment.
When a disabled managed model provider claims google, this guard suppresses lower providers only when their directory is literally named google. The bundled gemini directory still imports and registers google as an alias in plugins/model-providers/gemini/__init__.py:51-54, so get_provider_profile("google") returns Gemini and executes the lower-authority provider despite the explicit disable. Fresh evidence beyond the earlier directory-identity fixes is that canonical names and aliases are registered only after this filename guard; remove every reserved identity from the resulting registry/alias map or suppress modules based on their registration claims, and exercise the real disable-config-to-resolution path.
AGENTS.md reference: AGENTS.md:L84-L87
Useful? React with 👍 / 👎.
| previous = discovered.get(child.name) | ||
| if previous is not None and previous != child: | ||
| raise UserPluginsRootError( | ||
| "invalid_user_plugins_root: managed model provider is " | ||
| f"published in multiple layouts: {child.name}" | ||
| ) | ||
| discovered[child.name] = child |
There was a problem hiding this comment.
When managed directories have distinct leaf names but overlapping activation identities—for example, directory foo declares manifest name bar while another provider is published in directory bar—this check accepts both because it deduplicates only child.name. Disabling foo then adds bar to disabled_managed_identities, causing the actual bar provider to be skipped at line 417; without a disable, management commands still have an ambiguous public identity. Build and validate a unique index from _managed_provider_identities() before importing, as the memory and cron scanners do, and cover the config-to-provider-resolution behavior against a temporary HERMES_HOME.
AGENTS.md reference: AGENTS.md:L84-L87
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 99c1e1701a
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| manifest.source == "user" | ||
| and is_user_plugins_root_externally_managed() | ||
| ): | ||
| return f"platforms/{manifest.key}" |
There was a problem hiding this comment.
When a flat managed plugin such as directory corporate declares name: telegram-platform and kind: platform, this branch assigns it the collision identity platforms/corporate, while the bundled Telegram adapter is normalized to platforms/telegram. If the managed plugin is disabled by its supported directory key, the bundled deferred adapter remains and platform_registry.get("telegram") loads it, unexpectedly activating the lower-authority implementation. Derive the managed platform collision identity through _platform_name_from_manifest() as well, while retaining the directory key for activation.
AGENTS.md reference: AGENTS.md:L84-L87
Useful? React with 👍 / 👎.
| for manifest in user_manifests: | ||
| managed_reserved_identities.add(self._collision_key(manifest)) | ||
| managed_reserved_identities.add(manifest.name) |
There was a problem hiding this comment.
When two flat managed directories have different path keys but both declare name: shared, this set silently collapses the duplicate identity while winners retains both directory-keyed manifests. Because activation later accepts manifest.name, plugins.enabled: [shared] imports both modules, so one allow-list identity unexpectedly authorizes multiple managed plugins. Build a unique managed identity index and reject duplicate flat public names before loading.
AGENTS.md reference: AGENTS.md:L54-L57
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c0939f266d
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if dashboard_manifest.is_file(): | ||
| continue |
There was a problem hiding this comment.
When the managed tree publishes the supported JS-only layout <root>/spotify/dashboard/manifest.json and plugins.enabled contains spotify so its assets can load, this branch emits no PluginManifest; consequently spotify never enters managed_reserved_identities, and the bundled plugins/spotify plugin is loaded under the same allow-list entry, executing lower-authority Python code. Fresh evidence beyond the earlier dashboard-only validation fix is that validation now accepts this layout but general discovery discards its authority identity; retain a non-executable reservation for dashboard-only entries and exercise the config-to-real-import path.
AGENTS.md reference: AGENTS.md:L84-L87
Useful? React with 👍 / 👎.
| lookup_key = manifest.key or manifest.name | ||
| if "/" not in lookup_key: | ||
| for identity in {lookup_key, manifest.name}: |
There was a problem hiding this comment.
When two managed categorized plugins share a public name—matching existing category pairs such as image_gen/fal and video_gen/fal—this guard deliberately excludes them from ambiguity validation, while the activation gate still treats manifest.name as an alias for every plugin. Thus a legacy or manually persisted plugins.enabled: [fal] imports both managed modules even though one allow-list identity should not authorize two independent plugins. Fresh evidence beyond the earlier flat-identity finding is the new slash check combined with the unchanged bare-name activation path; either reject duplicate activation aliases across categories or stop accepting an ambiguous bare alias.
AGENTS.md reference: AGENTS.md:L54-L57
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 60ef44028d
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| # Re-establish argv authority even on an idempotent call. The normal | ||
| # dotenv load intentionally overrides stale process values, but it | ||
| # must not downgrade an explicit CLI isolation flag between early and | ||
| # later discovery phases. | ||
| safe_mode = _startup_safe_mode() |
There was a problem hiding this comment.
When HERMES_SAFE_MODE=1 comes from the project .env, the initial bootstrap has already configured plugins.user_root because it loads only the profile dotenv; hermes_cli.main then loads the project dotenv and invokes this function again, but this early return leaves HERMES_INTERNAL_USER_PLUGINS_ROOT installed. The invocation therefore still reports externally managed mode, revalidates the managed tree while building the system prompt, and refuses plugin mutations despite safe mode promising to leave the root unconfigured. Handle the newly true safe_mode before the completed-bootstrap return and cover the real two-stage dotenv startup path.
AGENTS.md reference: AGENTS.md:L84-L87
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f999d24621
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| spec.loader.exec_module(cli_mod) | ||
| from hermes_cli.managed_plugin_loader import exec_module_source_aware | ||
|
|
||
| exec_module_source_aware(spec, cli_mod, cli_file) |
There was a problem hiding this comment.
When an active managed memory provider's cli.py defines register_cli and then raises during initialization, this call falls into the catch at line 782, silently omitting the command and leaving the partially initialized module in sys.modules; a later parser build then reuses that module and may expose a command whose clean import would fail. Fresh evidence beyond the earlier provider-helper finding is that discover_plugin_cli_commands() has its own module insertion and swallow path, so evict this CLI module and propagate a UserPluginsRootError through the parser boundary for managed providers.
AGENTS.md reference: AGENTS.md:L54-L57
Useful? React with 👍 / 👎.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 932e897f33
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if not name or name in ("builtin", "in-process", "inprocess"): | ||
| return InProcessCronScheduler() | ||
|
|
||
| managed_provider = False |
There was a problem hiding this comment.
When resolve_cron_scheduler() is the first code to import hermes_cli.config and the managed publication is malformed, config's eager provider scan raises UserPluginsRootError, but the preceding config lookup catches it and leaves name empty. Execution therefore returns the built-in scheduler before this new managed-provider error handling runs, allowing a supported resolver path to continue despite an invalid authoritative root; rethrow UserPluginsRootError from the config lookup as well.
AGENTS.md reference: AGENTS.md:L84-L87
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a2e322ef7d
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| # explicit argv isolation boundary afterward so ``HERMES_SAFE_MODE=0`` in a | ||
| # legacy .env cannot downgrade ``hermes --safe-mode`` for later scanners. The | ||
| # idempotent bootstrap never rereads or replaces the configured plugin root. | ||
| _configure_user_plugins_root() |
There was a problem hiding this comment.
When HERMES_SAFE_MODE=1 is supplied by the supported project .env fallback, this reassertion happens only after importing hermes_cli.config; that import eagerly calls _inject_profile_env_vars() and providers.list_providers(), so managed or profile-local model-provider code can already execute before safe mode clears the bridge. Fresh evidence after the earlier bridge-clearing fix is this provider-capable import ordering, which the new test covers only by invoking bootstrap and dotenv loading directly without importing hermes_cli.config; load the project dotenv before that import or otherwise establish its safe-mode value first, and validate the real CLI startup path.
AGENTS.md reference: AGENTS.md:L84-L87
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 86d15961a9
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| # Provider metadata is normally best-effort, but an administrator- | ||
| # published provider boundary is authoritative. Swallowing its | ||
| # validation/import failure after bootstrap would let commands proceed | ||
| # as though the managed generation were valid. |
There was a problem hiding this comment.
When Docker starts with plugins.user_root configured and the persistent profile still contains a writable plugins/model-providers directory, docker/stage2-hook.sh runs scripts/docker_config_migrate.py, which imports hermes_cli.config without first calling the new bootstrap. This eager provider injection therefore sees no internal managed-root bridge and imports providers from the profile-local tree before gateway startup, bypassing the root-owned execution boundary. Bootstrap this production entrypoint before importing hermes_cli.config, and cover the actual migration subprocess against a temporary HERMES_HOME.
AGENTS.md reference: AGENTS.md:L84-L87
Useful? React with 👍 / 👎.
1 participant
Outcome
Adds one fail-closed
plugins.user_rootauthority for externally managed, root-owned plugin publication. Loader, CLI, dashboard, security audit, model providers, memory and cron resolve the same validated root; write-capable plugin commands refuse the externally managed mode.Security contract
Validation
hermes-root-owned-plugin-discovery-source-receipt-2026-08-12.md.Limits
Source review only. This PR does not create
/var/lib/pandora/discord-runtime-publisher/plugins, install a package, change live config, restart Hermes or authorize Discord runtime promotion. Rollout remains a separate F4-S4 gate after Pandora NousResearch#294 and pandora-box NousResearch#60.