fix(doctor): skip cleanup hints for active gateway and downgrade exec SecretRef errors to warnings#52515
fix(doctor): skip cleanup hints for active gateway and downgrade exec SecretRef errors to warnings#52515daniel-knox wants to merge 1 commit into
Conversation
Greptile SummaryThis PR fixes two false-positive issues in Key changes:
Issues found:
Confidence Score: 4/5
Prompt To Fix All With AIThis is a comment left during a code review.
Path: src/config/types.secrets.ts
Line: 143-145
Comment:
**`process.argv` fallback causes false-positive doctor detection**
The `process.argv.some(arg => arg.includes('doctor'))` fallback will incorrectly set `isDoctorContext = true` whenever any CLI argument happens to contain the string `"doctor"` — for example, a config file named `doctor-config.json`, a task argument like `run doctor-task`, or any test harness whose path includes "doctor".
In those non-doctor contexts, a genuinely unresolved `exec:` SecretRef would be silently downgraded to a `console.warn` instead of throwing, making failures invisible.
The env-var approach already covers all code paths triggered from within `doctorCommand` (it's set before the `try` block), so this fallback is both redundant and risky. Consider removing it:
```suggestion
const isDoctorContext = params.allowExecFailuresInDoctor ??
(process.env.OPENCLAW_ALLOW_EXEC_SECRETREF_FAILURES === 'true');
```
How can I resolve this? If you propose a fix, please make it concise.
---
This is a comment left during a code review.
Path: src/daemon/inspect.ts
Line: 28-31
Comment:
**Dead `env` parameter in refactored function**
The `env` parameter is kept in the signature with its default value, but it is never referenced anywhere in the new function body. The old implementation read `env.OPENCLAW_PROFILE` to derive the gateway label; the new implementation receives concrete `ExtraGatewayService` objects whose `label` is already resolved, making `env` entirely unused.
```suggestion
export function renderGatewayServiceCleanupHints(
services: ExtraGatewayService[] = [],
): string[] {
```
How can I resolve this? If you propose a fix, please make it concise.Reviews (1): Last reviewed commit: "fix(doctor): skip cleanup hints for acti..." | Re-trigger Greptile |
| const isDoctorContext = params.allowExecFailuresInDoctor ?? | ||
| (process.env.OPENCLAW_ALLOW_EXEC_SECRETREF_FAILURES === 'true' || | ||
| process.argv.some(arg => arg.includes('doctor'))); |
There was a problem hiding this comment.
process.argv fallback causes false-positive doctor detection
The process.argv.some(arg => arg.includes('doctor')) fallback will incorrectly set isDoctorContext = true whenever any CLI argument happens to contain the string "doctor" — for example, a config file named doctor-config.json, a task argument like run doctor-task, or any test harness whose path includes "doctor".
In those non-doctor contexts, a genuinely unresolved exec: SecretRef would be silently downgraded to a console.warn instead of throwing, making failures invisible.
The env-var approach already covers all code paths triggered from within doctorCommand (it's set before the try block), so this fallback is both redundant and risky. Consider removing it:
| const isDoctorContext = params.allowExecFailuresInDoctor ?? | |
| (process.env.OPENCLAW_ALLOW_EXEC_SECRETREF_FAILURES === 'true' || | |
| process.argv.some(arg => arg.includes('doctor'))); | |
| const isDoctorContext = params.allowExecFailuresInDoctor ?? | |
| (process.env.OPENCLAW_ALLOW_EXEC_SECRETREF_FAILURES === 'true'); |
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/config/types.secrets.ts
Line: 143-145
Comment:
**`process.argv` fallback causes false-positive doctor detection**
The `process.argv.some(arg => arg.includes('doctor'))` fallback will incorrectly set `isDoctorContext = true` whenever any CLI argument happens to contain the string `"doctor"` — for example, a config file named `doctor-config.json`, a task argument like `run doctor-task`, or any test harness whose path includes "doctor".
In those non-doctor contexts, a genuinely unresolved `exec:` SecretRef would be silently downgraded to a `console.warn` instead of throwing, making failures invisible.
The env-var approach already covers all code paths triggered from within `doctorCommand` (it's set before the `try` block), so this fallback is both redundant and risky. Consider removing it:
```suggestion
const isDoctorContext = params.allowExecFailuresInDoctor ??
(process.env.OPENCLAW_ALLOW_EXEC_SECRETREF_FAILURES === 'true');
```
How can I resolve this? If you propose a fix, please make it concise.| export function renderGatewayServiceCleanupHints( | ||
| services: ExtraGatewayService[] = [], | ||
| env: Record<string, string | undefined> = process.env as Record<string, string | undefined>, | ||
| ): string[] { |
There was a problem hiding this comment.
env parameter in refactored function
The env parameter is kept in the signature with its default value, but it is never referenced anywhere in the new function body. The old implementation read env.OPENCLAW_PROFILE to derive the gateway label; the new implementation receives concrete ExtraGatewayService objects whose label is already resolved, making env entirely unused.
| export function renderGatewayServiceCleanupHints( | |
| services: ExtraGatewayService[] = [], | |
| env: Record<string, string | undefined> = process.env as Record<string, string | undefined>, | |
| ): string[] { | |
| export function renderGatewayServiceCleanupHints( | |
| services: ExtraGatewayService[] = [], | |
| ): string[] { |
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/daemon/inspect.ts
Line: 28-31
Comment:
**Dead `env` parameter in refactored function**
The `env` parameter is kept in the signature with its default value, but it is never referenced anywhere in the new function body. The old implementation read `env.OPENCLAW_PROFILE` to derive the gateway label; the new implementation receives concrete `ExtraGatewayService` objects whose `label` is already resolved, making `env` entirely unused.
```suggestion
export function renderGatewayServiceCleanupHints(
services: ExtraGatewayService[] = [],
): string[] {
```
How can I resolve this? If you propose a fix, please make it concise.There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a7bb02f63c
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| const nonLegacyExtraServices = extraServices.filter((svc) => !svc.legacy); | ||
| const cleanupHints = renderGatewayServiceCleanupHints(nonLegacyExtraServices); |
There was a problem hiding this comment.
Keep legacy services in the manual cleanup hint list
When doctor finds only legacy services and the user does not remove them automatically (or cleanup is skipped for system-scope entries), this filter drops every detected service before rendering hints. In that path maybeScanExtraGatewayServices() still reports the legacy service, but it no longer prints any actionable cleanup commands, so the fallback guidance disappears exactly when automatic cleanup did not happen.
Useful? React with 👍 / 👎.
| services: ExtraGatewayService[] = [], | ||
| env: Record<string, string | undefined> = process.env as Record<string, string | undefined>, | ||
| ): string[] { | ||
| const profile = env.OPENCLAW_PROFILE; | ||
| switch (process.platform) { | ||
| case "darwin": { | ||
| const label = resolveGatewayLaunchAgentLabel(profile); | ||
| return [`launchctl bootout gui/$UID/${label}`, `rm ~/Library/LaunchAgents/${label}.plist`]; | ||
| } | ||
| case "linux": { | ||
| const unit = resolveGatewaySystemdServiceName(profile); | ||
| return [ | ||
| `systemctl --user disable --now ${unit}.service`, | ||
| `rm ~/.config/systemd/user/${unit}.service`, | ||
| ]; | ||
| } | ||
| case "win32": { | ||
| const task = resolveGatewayWindowsTaskName(profile); | ||
| return [`schtasks /Delete /TN "${task}" /F`]; | ||
| if (services.length === 0) { | ||
| return []; |
There was a problem hiding this comment.
Pass detected extra services into status cleanup hint rendering
renderGatewayServiceCleanupHints() now returns [] unless a service list is supplied, but src/cli/daemon-cli/status.print.ts:309 still calls it with no arguments. As a result, openclaw status/openclaw gateway status will report extra gateway-like services yet never show any cleanup hint, which regresses the existing troubleshooting flow for that command.
Useful? React with 👍 / 👎.
a7bb02f to
806935a
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 806935a8be
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| break; | ||
| } | ||
| case "linux": { | ||
| hints.push(`systemctl --user disable --now ${svc.label}`); |
There was a problem hiding this comment.
Use system-manager commands for system-scoped extra units
When doctor --deep finds an old gateway-like unit in /etc/systemd/system, /usr/lib/systemd/system, or /lib/systemd/system, findExtraGatewayServices() marks it scope: "system" and still routes it through this hint builder. systemctl --help documents --user as “Connect to user service manager”, so this command never disables the system unit that was actually detected. In that case doctor now prints a cleanup recipe that looks exact but leaves the conflicting service installed.
Useful? React with 👍 / 👎.
… SecretRef errors to warnings - Fix false cleanup hints: renderGatewayServiceCleanupHints() now takes detected services as input and generates per-service hints instead of hardcoded main gateway hints - Fix SecretRef exit code 1: assertSecretInputResolved() now downgrades exec: SecretRef resolution failures to warnings during doctor execution - Doctor sets OPENCLAW_ALLOW_EXEC_SECRETREF_FAILURES=true to enable lenient handling for unresolvable keychain/exec refs when gateway isn't running Fixes openclaw#52510
806935a to
8c2069b
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8c2069be8e
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| hints.push(`launchctl bootout gui/$UID/${svc.label}`); | ||
| const plistPath = svc.detail.startsWith("plist:") | ||
| ? svc.detail.slice(6).trim() | ||
| : `~/Library/LaunchAgents/${svc.label}.plist`; | ||
| hints.push(`rm "${plistPath}"`); |
There was a problem hiding this comment.
Use the system launchctl domain for system plists
When findExtraGatewayServices({ deep: true }) reports an extra plist from /Library/LaunchDaemons as scope: "system", this helper still emits launchctl bootout gui/$UID/... plus a plain rm. That recipe only targets the logged-in user's GUI domain, so openclaw doctor --deep gives macOS users cleanup commands that do not unload or remove the detected LaunchDaemon, leaving the duplicate gateway in place.
Useful? React with 👍 / 👎.
|
This pull request has been automatically marked as stale due to inactivity. |
|
Codex review: needs real behavior proof before merge. Reviewed June 6, 2026, 12:46 AM ET / 04:46 UTC. Summary PR surface: Source +42, Generated 0. Total +42 across 6 files. Reproducibility: unclear. The review failed before ClawSweeper could establish a reproduction path. Review metrics: none identified. Merge readiness Overall follows the weaker of proof and patch quality, so missing proof can cap an otherwise strong patch. Risk before merge
Maintainer options:
Next step before merge
Review detailsBest possible solution: Retry the Codex review after fixing the execution failure. Do we have a high-confidence way to reproduce the issue? Unclear. The review failed before ClawSweeper could establish a reproduction path. Is this the best way to solve the issue? Unclear. Retry the review first so ClawSweeper can evaluate the actual issue and fix direction. AGENTS.md: unclear because the file could not be read completely. Codex review notes: model gpt-5.5, reasoning high; reviewed against 9313471fa579. Label changesLabel changes:
Label justifications:
Evidence reviewedPR surface: Source +42, Generated 0. Total +42 across 6 files. View PR surface stats
What I checked:
Likely related people:
What the crustacean ranks mean
Shiny media proof means a screenshot, video, or linked artifact directly shows the changed behavior. Runtime, network, CSP, and security claims still need visible diagnostics. How this review workflow works
|
|
Codex review: found issues before merge. What this changes: The PR changes doctor gateway cleanup hints to render from detected extra services, wraps doctor execution in a process env flag that downgrades unresolved exec SecretRefs to warnings, and updates generated plugin SDK API baselines. Maintainer follow-up before merge: This is an open implementation PR with real product value, but it has concrete blocking review findings and touches security-sensitive exported SecretRef behavior that requires maintainer/secops judgment rather than an automated replacement PR. Review findings:
Review detailsBest possible solution: Revise the fix so cleanup hints are rendered from actual detected service metadata in both doctor and status, commands honor user vs system service scope, unremoved legacy services still get actionable manual guidance, and doctor SecretRef warnings use an explicit read-only inspection path while exported strict helpers remain strict for runtime and plugin callers. Full review comments:
Overall correctness: patch is incorrect Acceptance criteria:
What I checked:
Likely related people:
Remaining risk / open question:
Codex review notes: model gpt-5.5, reasoning high; reviewed against e46dccb35374. |
|
ClawSweeper PR egg 🎁 Pass real behavior proof to wake the egg and unlock a hatchable treat. Where did the egg go?
|
|
This pull request has been automatically marked as stale due to inactivity. |
Fixes #52510
Problem
openclaw doctorhas two false-positive issues:Cleanup hints suggest removing the active gateway - When doctor detects ANY extra gateway-like services (e.g., an unrelated
imessage-watcher), it unconditionally generates cleanup commands for the main gateway plist. Following these hints kills the running gateway and causes downtime.SecretRef exit code 1 - Doctor exits with code 1 when encountering
exec:keychain:SecretRef values in config. These are resolved at gateway runtime and are expected to be unavailable during doctor execution.Changes
Bug 1:
src/daemon/inspect.ts+src/commands/doctor-gateway-services.tsrenderGatewayServiceCleanupHints()now takes the actual detected extra services as inputBug 2:
src/config/types.secrets.ts+src/commands/doctor.tsOPENCLAW_ALLOW_EXEC_SECRETREF_FAILURES=trueduring execution (with proper try/finally cleanup)assertSecretInputResolved()detects doctor context and downgradesexec:SecretRef failures to warnings instead of throwingTesting
renderGatewayServiceCleanupHints, so signature change is safeexec:keychain:SecretRefs are present