Doctor: expose gateway runtime findings#97075
Conversation
|
@clawsweeper please review current head 1507359 for the opt-in gateway health/daemon structured findings slice. Focus areas: default-disabled lint behavior, read-only runtime probing, no service mutation in structured checks, and no public SDK/plugin/config surface change. Validation/proof is in the PR body. |
|
🦞👀 I queued a lightweight read-only assist pass. It will post a separate answer comment and will not edit the durable ClawSweeper review comment or trigger close, merge, repair, label, or branch changes. Request: please review current head 1507359 for the opt-in gateway health/daemon structured findings slice. Focus areas: default-disabled lint behavior, read-only runtime probing, no service mutation in structured checks, and no public SDK/plugin/config surface change. Validation/proof is in the PR body. |
|
Codex review: needs maintainer review before merge. Reviewed June 27, 2026, 8:16 PM ET / 00:16 UTC. Summary PR surface: Source +197, Tests +195. Total +392 across 5 files. Reproducibility: not applicable. This PR adds opt-in diagnostic capability rather than fixing a reproduced bug. The changed behavior is covered by PR-body terminal output, focused tests, a human review validation run, and the passing Real behavior proof check. Review metrics: 1 noteworthy metric.
Stored data model Merge readiness Overall follows the weaker of proof and patch quality, so missing proof can cap an otherwise strong patch. Next step before merge
Security Review detailsBest possible solution: Land the default-disabled structured Gateway Doctor checks after maintainer review, while keeping service install/start/restart mutation in the legacy Doctor contribution path. Do we have a high-confidence way to reproduce the issue? Not applicable: this PR adds opt-in diagnostic capability rather than fixing a reproduced bug. The changed behavior is covered by PR-body terminal output, focused tests, a human review validation run, and the passing Real behavior proof check. Is this the best way to solve the issue? Yes: this is the best fit for the stated goal because it uses existing Gateway probe/service-state helpers, keeps checks default-disabled and read-only, and leaves service repair policy in the legacy Doctor contribution path. AGENTS.md: found and applied where relevant. Codex review notes: model internal, reasoning high; reviewed against 7bbd09047bd7. Label changesLabel justifications:
Evidence reviewedPR surface: Source +197, Tests +195. Total +392 across 5 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
|
|
ClawSweeper assist: This needs a full correctness review to verify the current head Evidence:
Suggested next action: Use Source: #97075 (comment) |
|
@clawsweeper re-review Current head Validation:
|
|
🦞🧹 I asked ClawSweeper to review this item again. |
6cbbcf9 to
981ba19
Compare
|
@clawsweeper re-review Current head Validation:
|
|
🦞🧹 I asked ClawSweeper to review this item again. |
galiniliev
left a comment
There was a problem hiding this comment.
This PR adds opt-in structured Doctor findings for Gateway reachability and local daemon state, while leaving the existing Doctor contribution path in charge of service install/start/restart behavior. That matters because these checks become selectable by doctor --lint --only and --all, so the structured path must stay read-only and avoid leaking Gateway URL material.
Findings: none blocking.
LOC: +395/-3 (5 files)
By: Gio Della-Libera (@giodl73-repo, acct 2025-09-30) | OpenClaw: 187 PRs, 14 issues, 138 commits/12mo | GitHub: 6437 commits, 221 PRs, 29 issues, 17 reviews/12mo
Behavior reviewed: adds core/doctor/gateway-health and core/doctor/gateway-daemon as defaultEnabled: false structured checks, maps the existing doctor:gateway-health and doctor:gateway-daemon contributions to those IDs, and keeps normal doctor execution on the legacy contribution functions.
Best-fix verdict: best. The implementation puts the structured lint-only behavior in src/flows/doctor-core-checks.runtime.ts and wires it through src/flows/doctor-core-checks.ts/src/flows/doctor-health-contributions.ts, which matches the existing split between contribution-owned Doctor behavior and non-mutating lint checks. It does not duplicate service mutation logic or move Gateway repair policy into lint.
Alternatives considered:
- Reuse
runGatewayHealthChecksdirectly from the structured check: rejected because that path updates Doctor flow state, emits terminal notes, and can trigger legacy follow-on behavior rather than returning pureHealthFindingrecords. - Add repair hooks to the new daemon check: rejected because
src/commands/doctor-gateway-daemon-flow.tsstill owns install/start/restart prompts and platform repair policy. - Make the checks default-enabled: rejected because Gateway probing and service inspection would change default
doctor --lintcost/side effects;defaultEnabled: falsepreserves the current default selection contract insrc/flows/doctor-lint-flow.ts.
Code read: src/flows/doctor-core-checks.runtime.ts, src/flows/doctor-core-checks.ts, src/flows/doctor-health-contributions.ts, src/flows/doctor-lint-flow.ts, src/commands/doctor-lint.ts, src/commands/doctor-gateway-health.ts, src/commands/doctor-gateway-daemon-flow.ts, src/gateway/call.ts, src/gateway/connection-details.ts, src/cli/daemon-cli/probe.ts, src/gateway/probe.ts, src/commands/gateway-health-auth-diagnostic.ts, src/daemon/service.ts, src/daemon/service-runtime.ts, and adjacent tests.
Evidence map:
- Changed surface: structured Doctor health checks and contribution mapping.
- Runtime entry point:
runDoctorLintCliresolves contribution health checks, thenrunDoctorLintChecksskips default-disabled checks unless--onlyor--allselects them. - Owner boundary: Gateway service mutation remains in
maybeRepairGatewayDaemon; the new daemon structured check only callsreadGatewayServiceState. - Caller/callee:
createGatewayHealthCheckandcreateGatewayDaemonCheckcall the runtime collectors; the collectors callbuildGatewayProbeConnectionDetails/probeGatewayStatusandresolveGatewayService/readGatewayServiceStaterespectively. - Sibling invariant: existing default-disabled checks such as session locks/state integrity use the same selection model.
- Security-sensitive path: unreachable Gateway findings redact the target with
redactSensitiveUrlLikeString, and the connection-detail builder already redacts display text for unsafe URL errors.
Validation run on PR head 981ba1906fa4ae27d9e20c3c99646fd9ebbf02b2 in /tmp/openclaw-pr-97075:
node scripts/run-vitest.mjs run src/flows/doctor-core-checks.runtime.test.ts src/flows/doctor-core-checks.test.ts src/flows/doctor-health-contributions.test.ts --reporter=verbose-> 99 passed./home/galini/GitHub/openclaw4/node_modules/.bin/oxfmt --check src/flows/doctor-core-checks.runtime.ts src/flows/doctor-core-checks.runtime.test.ts-> passed./home/galini/GitHub/openclaw4/node_modules/.bin/oxlint src/flows/doctor-core-checks.runtime.ts src/flows/doctor-core-checks.runtime.test.ts-> passed.git diff --check origin/main...HEAD-> passed.
CI spot check: live PR metadata shows Real behavior proof, runner-admission, actionlint, and Socket PR alerts succeeded at the current head. The PR is still draft, so this is a correctness review, not a merge-ready signal.
Remaining uncertainty: I did not run a real local Gateway service or remote Gateway instance from this environment. The PR body and CI real-behavior proof cover explicit CLI --only checks; my local proof covers the structured selection, redaction, daemon skip, and read-only service-state logic.
981ba19 to
e381ff4
Compare
|
@clawsweeper re-review Current head Prior addressed findings remain in place:
Validation before rebase remains applicable to the unchanged diff:
Post-rebase hosted checks are green/skipped, including Real behavior proof. |
|
🦞🧹 I asked ClawSweeper to review this item again. |
e381ff4 to
1b00f70
Compare
|
Merged via squash.
|
* feat(doctor): expose gateway runtime findings * fix(doctor): redact gateway health targets
* feat(doctor): expose gateway runtime findings * fix(doctor): redact gateway health targets
* feat(doctor): expose gateway runtime findings * fix(doctor): redact gateway health targets
* feat(doctor): expose gateway runtime findings * fix(doctor): redact gateway health targets
Summary
core/doctor/gateway-healthandcore/doctor/gateway-daemonHealthFindingrecordstargetBehavior
doctor --lintdoes not run these checks because both aredefaultEnabled: false--only core/doctor/gateway-health,--only core/doctor/gateway-daemon, or--allcan surface the findingsValidation
node scripts/run-vitest.mjs run src/flows/doctor-core-checks.runtime.test.ts src/flows/doctor-core-checks.test.ts src/flows/doctor-health-contributions.test.ts --reporter=verbose(99 passed)pnpm exec oxfmt --check src/flows/doctor-core-checks.runtime.ts src/flows/doctor-core-checks.runtime.test.tspnpm exec oxlint src/flows/doctor-core-checks.runtime.ts src/flows/doctor-core-checks.runtime.test.tsnode scripts/plugin-sdk-surface-report.mjs --checkgit diff --checknode scripts/run-tsgo.mjs -p test/tsconfig/tsconfig.test.src.json --incremental --tsBuildInfoFile .artifacts/tsgo-cache/test-src.tsbuildinfopnpm tsgo:coreReal behavior proof
Local source CLI, Windows worktree, no local Gateway service running:
node scripts/run-node.mjs doctor --lint --json --only core/doctor/gateway-daemonok:false,checksRun:1, onecore/doctor/gateway-daemonwarning:Gateway service is not installed.node scripts/run-node.mjs doctor --lint --json --only core/doctor/gateway-healthok:false,checksRun:1, onecore/doctor/gateway-healthwarning:Gateway is not reachable: connect ECONNREFUSED 127.0.0.1:18789Focused regression proof on current head covers a remote Gateway URL containing userinfo and a sensitive query token. The structured finding target is redacted as
wss://***:***@gateway.example.test/rpc?token=***&safe=value, and the serialized findings do not contain the original userinfo or token value.Attempted default
node scripts/run-node.mjs doctor --lint --json, but the local source command timed out in this fresh worktree. The new checks are default-disabled and covered by focused lint-flow tests plus explicit--onlyCLI proof above.Follow-up Fix
981ba1906fa4ae27d9e20c3c99646fd9ebbf02b2also points remote Gateway health findings atgateway.remote.urlinstead of the nonexistentgateway.url, covering both prepare-failure and unreachable findings.