Repository navigation
fix(gateway): restart managed external supervisor - #221
Conversation
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
|
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Team Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
Warning Billing warning: we have not been able to collect payment for this subscription for more than 72 hours. Please update the payment method or pay any pending invoices in Billing to avoid service interruption. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![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: 9214a03b67
ℹ️ 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".
![evaos-code-review-bot[bot]](https://avatars.githubusercontent.com/in/4184532?s=60&v=4){kind=small}
There was a problem hiding this comment.
Walkthrough
PR: #221 - fix(gateway): restart managed external supervisor
Head: 9214a03b67604f9be4755efb4334ecb123bec323 into r30/upstream-5fc308a-base. Review event: COMMENT.
Estimated review effort: 1/5 (~14 min)
Changed Files
| File | Status | Churn | Purpose | Risk |
|---|---|---|---|---|
hermes_cli/gateway.py |
modified | +82/-0 | Changed file | Moderate: validated P2 finding |
tests/hermes_cli/test_gateway_service.py |
modified | +144/-0 | Test coverage | Low |
Review Signal
Validated inline findings: 1 (P0: 0, P1: 0, P2: 1, P3: 0).
Dropped findings before posting: 0. High-severity findings: 0.
Maintainer Analysis
Changed behavior:
- Plain restart in a managed flat-profile runtime now signals the current externally supervised gateway and waits for a replacement PID instead of using generic service/manual restart logic.
- The same early return currently also intercepts restart requests carrying
--allor--system.
Affected invariants:
- Explicit restart scope flags must not be silently discarded.
- A managed handoff must accept only a new profile-local process whose argv contains the external-supervisor marker.
- Unmanaged runtimes must retain existing restart behavior.
Evidence:
- hermes_cli/gateway.py:8413 performs the managed early return before reading
args.systemand without checkingargs.all. - tests/hermes_cli/test_gateway_service.py exercises command routing only with
system=False, all=False. - The replacement tests mock PID and argv behavior; they do not cover scoped restart flags.
Limitations:
- Per instructions, no tests, builds, package scripts, app commands, or shell commands were run.
- Review was limited to the supplied checkout diff and context.
No-finding rationale: The remaining new failure paths and replacement-identity checks are internally consistent with the stated fail-closed managed-supervisor design; no additional concrete current-path defect was validated from the provided evidence.
Risk Taxonomy
- Runtime correctness: 1
Validation and Proof
No required validation recommendation selected; rely on existing GitHub checks and human review.
Proof status: not_applicable - No required behavior proof selected for this changed surface.
Related Context
Related issues/PRs: #217, #201.
Pre-merge checklist
- Inline comments target current RIGHT-side diff lines.
- No secret-like content survived into posted inline comments.
- REQUEST_CHANGES is only used when eligible P0/P1 findings survive validation.
- Required behavior proof is present or not applicable.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2127a57586
ℹ️ 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".
There was a problem hiding this comment.
Walkthrough
PR: #221 - fix(gateway): restart managed external supervisor
Head: 2127a575867f09df6adba0825e9ef05249f5407a into r30/upstream-5fc308a-base. Review event: COMMENT.
Estimated review effort: 2/5 (~24 min)
Changed Files
| File | Status | Churn | Purpose | Risk |
|---|---|---|---|---|
hermes_cli/gateway.py |
modified | +125/-0 | Changed file | Moderate: validated P2 finding |
tests/hermes_cli/test_gateway_service.py |
modified | +243/-0 | Test coverage | Elevated: large change |
Review Signal
Validated inline findings: 1 (P0: 0, P1: 0, P2: 1, P3: 0).
Dropped findings before posting: 0. High-severity findings: 0.
Maintainer Analysis
Changed behavior:
- Managed flat-profile gateway restarts now signal the externally supervised process and wait for a profile-local running replacement instead of using generic service or manual restart routing.
- Replacement acceptance requires strict PID/lock identity, the external-supervisor argv marker, and matching running runtime status.
Affected invariants:
- A replacement is identified by both PID and process start time.
- Managed mode must not fall through to credential-incomplete generic restart paths.
- Startup-failed or ambiguous replacement state must fail closed.
Evidence:
- hermes_cli/gateway.py:384 compares only
replacement[0]withprevious_identity[0], despite both values being full identity tuples. - The added tests cover a different replacement PID but do not cover PID reuse with a changed start time.
Limitations:
- Review was limited to the supplied checkout diff; no shell commands, tests, builds, package scripts, app commands, or arbitrary PR code were executed.
No-finding rationale: No additional correctness, security, data-loss, CI, release, or test defect was validated from the supplied diff.
Risk Taxonomy
- Runtime correctness: 1
Validation and Proof
No required validation recommendation selected; rely on existing GitHub checks and human review.
Proof status: not_applicable - No required behavior proof selected for this changed surface.
Related Context
Related issues/PRs: #217, #201.
Pre-merge checklist
- Inline comments target current RIGHT-side diff lines.
- No secret-like content survived into posted inline comments.
- REQUEST_CHANGES is only used when eligible P0/P1 findings survive validation.
- Required behavior proof is present or not applicable.
1 participant
Outcome
Adds the smallest r30 release-family correction for managed profile gateways supervised by evaOS systemd templates. In managed flat-profile mode,
hermes gateway restartnow asks the already-running profile-local gateway to drain and exit through its existing SIGUSR1/exit-75 contract, then requires one replacement carrying--external-supervisor.Scope
main.Deterministic proof
tests/hermes_cli/test_gateway_service.py: 95 passed, 1 skipped.git diff --check: pass.Proof boundary
This proves source behavior only. Benjamin still requires immutable r30.6 plus PCS 0.1.94 installation and the affected live restart/Telegram/isolation check before the issue can close.
Supports #217 and #201.