fix(gateway): gate chat-triggered restart through operator policy - #7
fix(gateway): gate chat-triggered restart through operator policy#7Gabriel300p wants to merge 15 commits into
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: 29e77b27c2
ℹ️ 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".
| _RESTART_COMMAND = re.compile( | ||
| r"^\s*/restart(?:@[A-Za-z0-9_]+)?(?:\s+.*)?\s*$", | ||
| re.IGNORECASE | re.DOTALL, | ||
| ) |
There was a problem hiding this comment.
When an enabled policy receives text such as /restart@ops-bot or /restart@, this regex rejects it because the suffix is not limited to [A-Za-z0-9_]+, but MessageEvent.get_command() in gateway/platforms/base.py unconditionally discards everything after @ and returns restart. The request therefore reaches _handle_restart_command even when gateway.restart_command_enabled is false, providing a trivial bypass on platforms that accept free-form command text; identify restart requests using the same parser semantics as dispatch.
AGENTS.md reference: AGENTS.md:L54-L57
Useful? React with 👍 / 👎.
| def _pre_gateway_dispatch(*, event: Any = None, **_kwargs: Any) -> Optional[dict[str, str]]: | ||
| """Rewrite disabled restart commands before auth and normal dispatch.""" | ||
| text = _extract_event_text(event) | ||
| if not _is_restart_command(text): | ||
| return None | ||
| if _restart_command_enabled(): | ||
| return None | ||
| return {"action": "rewrite", "text": _BLOCKED_COMMAND} |
There was a problem hiding this comment.
When /restart is sent while an agent is running, rewriting it to this plugin command prevents the active-session fast path from recognizing it because that path uses resolve_command(), which only resolves built-ins. With the default display.busy_input_mode: interrupt, the rewritten text consequently falls into the ordinary follow-up path and interrupts or redirects the active turn instead of invoking _handle_blocked; other busy modes may queue it. Thus a supposedly side-effect-free denial can disrupt an in-progress experiment and may not return the policy explanation.
AGENTS.md reference: AGENTS.md:L84-L87
Useful? React with 👍 / 👎.
| def _load_runtime_config() -> Mapping[str, Any]: | ||
| """Load the current profile config lazily so policy changes apply live.""" | ||
| from hermes_cli.config import load_config | ||
|
|
||
| config = load_config() | ||
| if not isinstance(config, Mapping): | ||
| raise TypeError("Hermes config must be a mapping") | ||
| return config |
There was a problem hiding this comment.
If the plugin is already enabled and the last valid config allowed restart, malformed YAML does not raise here: hermes_cli.config.load_config() deliberately returns the last-known-good config (or defaults on a fresh load). _restart_command_enabled() therefore sees the old true/absent value and permits /restart, contrary to the documented and tested fail-closed behavior for a parse failure. This matters during manual or non-atomic config edits intended to disable restarts, so the policy must use a read path that exposes parse/read failure rather than silently serving fallback state.
AGENTS.md reference: AGENTS.md:L84-L87
Useful? React with 👍 / 👎.
29e77b2 to
1b606ee
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1b606ee379
ℹ️ 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 _is_restart_command(text): | ||
| return None |
There was a problem hiding this comment.
When an idle gateway has a quick_commands alias whose target is /restart, sending that alias bypasses this check because the hook only sees the original text and returns here. The cold dispatch path later expands alias targets in gateway/run.py and dispatches the resulting canonical restart command to _handle_restart_command, despite gateway.restart_command_enabled: false; the policy must also gate commands after alias resolution or resolve aliases before deciding.
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: 976683b62b
ℹ️ 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".
| try: | ||
| from hermes_cli.commands import resolve_command | ||
|
|
||
| definition = resolve_command(command) |
There was a problem hiding this comment.
When an idle gateway defines a quick_commands alias targeting /restart, this resolves only central registry aliases and returns the typed alias unchanged, so the policy allows it; gateway/run.py then expands the quick-command target later at lines 11858-11878 and reaches _handle_restart_command despite gateway.restart_command_enabled: false. Fresh evidence in this revision is that moving the gate into restart_policy.py still calls only commands.resolve_command and the new alias test mocks that resolver rather than exercising a real quick command.
AGENTS.md reference: AGENTS.md:L54-L57
Useful? React with 👍 / 👎.
| from hermes_cli import plugins | ||
|
|
||
| return plugins.invoke_hook(hook_name, **kwargs) | ||
| return core_results + plugins.invoke_hook(hook_name, **kwargs) |
There was a problem hiding this comment.
With the policy disabled, a non-restart message passes the core check, but this still returns any plugin result such as {"action": "rewrite", "text": "/restart"}; the gateway applies that rewrite at gateway/run.py lines 11101-11105 and later invokes the restart handler at line 11998 without another policy check. Thus an enabled pre-dispatch plugin can accidentally or deliberately revive a blocked chat restart, contrary to this function's stated terminal security boundary; validate the rewritten command before returning it or gate at the final canonical dispatch point.
AGENTS.md reference: AGENTS.md:L54-L57
Useful? React with 👍 / 👎.
|
@codex review Please review the current head |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 336bb3a0d5
ℹ️ 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 canonical_gateway_command(event, gateway) != "restart": | ||
| return None | ||
| if restart_command_enabled(): | ||
| return None |
There was a problem hiding this comment.
When the policy is false, a harmless built-in such as /status returns here without reading the policy, but gateway/run.py lines 11909-11944 then lets a command:status or command:* hook rewrite it to restart; the recomputed canonical name reaches the restart handler at lines 11998-11999, which writes restart markers and requests the drain. Fresh evidence in the current head is this separate decision-hook rewrite path, rather than the previously reported pre_gateway_dispatch plugin rewrite. Reapply the policy after command-hook rewrites or enforce it at the final canonical restart dispatch.
AGENTS.md reference: AGENTS.md:L54-L57
Useful? React with 👍 / 👎.
|
@codex address that feedback Fix the two remaining rewrite bypasses on current head Required behavior:
Do not merge, deploy, edit live config or activate runtime. |
|
To use Codex here, create an environment for this repo. |
1 participant
Context
This replaces the restart-policy portion of stale PR #4 with a minimal core boundary on current
main. PR #4 is closed as a contaminated recovery PR: stale base and more than 40 unrelated files.ClickUp source: DTC-03
86e2k3fph.Why this is core, not a plugin
A control gated by
plugins.enableddisappears ifconfig.yamlbecomes malformed and normal config loading falls back to defaults. The final design addshermes_cli.restart_policyand invokes it directly fromhermes_cli.lifecycleforpre_gateway_dispatch.It is always shipped with the runtime, independent of plugin discovery, and remains inert when the policy setting is absent. A core
skipis terminal: compatibility plugins are not invoked afterward, so a later rewrite cannot revive the request.Canonical command boundary
The policy mirrors the gateway's active dispatch order:
MessageEvent.get_command()semantics, including arguments and every@suffixform;hermes_cli.commands.resolve_command();GatewayRunner.config.quick_commandsmap active in the live process;type: aliasentries are followed; every hop re-checks built-in precedence.Using the live
GatewayRunner.configobject is deliberate: it is the same mapping the later dispatch sink will expand, including managed-overlay results and the last active config if the YAML becomes malformed after startup.Covered bypass forms include:
/restart/restart <args>/restart@HermesBot/restart@ops-bot/restart@restartBuilt-in commands retain precedence over same-name quick commands.
type: exec, malformed aliases, cycles, lookalikes and unrelated commands do not invent a restart capability.Blocking behavior
When
gateway.restart_command_enabled: false, the boundary schedules a policy notice on the current event loop and returns:{"action":"skip","reason":"restart_disabled_by_operator_policy"}The request stops before auth/pairing, active-session busy handling, restart markers, drain or process restart. Notice delivery is best effort; failure to reply never re-enables restart.
Strict policy semantics
The policy reader parses the raw active-profile and managed YAML documents directly. It never uses the normal fail-open/default-merging loader for this security decision.
Configuration
No plugin enablement is required. A supervised deployment/restart of the merged code is still required before the boundary exists in the live process.
Verification
The focused suite covers parser/suffix variants, registry aliases, active quick-command mappings in object and dict config shapes, alias chains, built-in precedence, cycles, exec/malformed entries, active-session independence, detached notice delivery, missing/invalid values, strict user and managed YAML parsing, managed precedence, core availability without plugin discovery and terminal core skip before compatibility plugins.
Repository CI for head
336bb3a0d5da2aa6eed5c6a6adb2e711ba7d9364is pending. A fresh independent review must evaluate the final core + quick-command design; old plugin reviews are retained as historical findings, not treated as approval.Live gates after merge
gateway.restart_command_enabled: false;/statusand unrelated quick commands remain functional;No runtime activation is included in this PR.