fix(cli): status/doctor use identity-verified live proxy - #642
Conversation
📝 WalkthroughWalkthrough
ChangesProxy diagnostics
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
|
@codex review |
|
✅ Action performedReview finished.
|
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
![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: e87deb20e1
ℹ️ 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".
| const listen = selectListenTarget(config, pid, pid ? readRuntimePort(pid) : null); | ||
| const health = await checkProxyHealth(listen); | ||
| // Prefer identity-verified liveness (runtime-port + /healthz) over ocx.pid alone (#618). | ||
| const live = await findLiveProxy(); |
There was a problem hiding this comment.
When the config is malformed, these new findLiveProxy() calls default their configFn to loadConfig(), which invokes warnAndBackupInvalidConfig; consequently, even with no PID/runtime record, ocx status --json and ocx doctor now create a new config.json.invalid-* file and emit a warning to stderr. This breaks the explicit read-only behavior covered by tests/cli-status-json.test.ts:244-271 and can accumulate backups every time diagnostics are run. Pass the already-read diagnostics config into findLiveProxy (and do likewise for the call in doctor.ts) or provide a non-mutating config reader.
AGENTS.md reference: AGENTS.md:L93-L95
Useful? React with 👍 / 👎.
| // Prefer identity-verified liveness (runtime-port + /healthz) over ocx.pid alone (#618). | ||
| const live = await findLiveProxy(); | ||
| const pidFile = readPid(); | ||
| const pid = live?.pid ?? pidFile; |
There was a problem hiding this comment.
When an orphaned runtime record answers with a legacy, PID-less health response while ocx.pid names a different still-running Ocx-shaped process, findLiveProxy() deliberately returns pid: null so the stale PID is not attributed to that listener, but this nullish fallback immediately restores pidFile. The label and JSON then claim the verified live proxy has that unrelated PID, and the analogous fallbacks in doctor.ts:724,732 inspect that process as the proxy. Use live ? live.pid : pidFile so a live result's authoritative null is preserved, and cover this caller behavior with a focused regression test.
AGENTS.md reference: AGENTS.md:L93-L95
Useful? React with 👍 / 👎.
| ? { | ||
| port: live.port, | ||
| hostname: live.hostname, | ||
| source: "runtime" as const, |
There was a problem hiding this comment.
If both PID/runtime metadata are absent or unusable but the proxy answers on config.port, findLiveProxy() succeeds through its configured-port fallback; this branch nevertheless reports listen.source as "runtime". That makes the documented JSON runtime/config source inaccurate precisely in the missing-state scenario this change handles. Have liveness return its discovery source, or retain "config" when the successful target came from the configured fallback.
AGENTS.md reference: AGENTS.md:L96-L97
Useful? React with 👍 / 👎.
…642-status-live-proxy
Keep doctor on the read-only diagnostics config path. Preserve authoritative null live pids, surface runtime vs config listen source from findLiveProxy, and cover resolveStatusPid.
|
Addressed remaining Codex review on latest dig:
Focused tests: |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@tests/cli-status-json.test.ts`:
- Around line 288-293: Strengthen the consumer-level regressions for an
identity-verified live proxy with a missing or stale PID file: in
tests/cli-status-json.test.ts:288-293, add a collectStatus() case asserting
running state, health/dashboard URLs, resolved PID, fallback port, and
listen.source; keep src/cli/status.ts:143-169 focused on the liveness-first
branch and ensure src/cli/status.ts:263-280 reports the verified proxy as
running. Add the matching doctor regression in src/cli/doctor.ts:722-736,
asserting at src/cli/doctor.ts:758-761 that diagnostics use the verified live
port and at src/cli/doctor.ts:824-825 that no unnecessary proxy-restart hint is
emitted.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: f6c93a91-92eb-466e-915f-53cc96ca4e8d
📒 Files selected for processing (5)
src/cli/doctor.tssrc/cli/status.tssrc/server/proxy-liveness.tstests/cli-status-json.test.tstests/proxy-liveness.test.ts
| test("resolveStatusPid preserves an authoritative null from live orphan checks", () => { | ||
| expect(resolveStatusPid({ pid: null }, 4242)).toBeNull(); | ||
| expect(resolveStatusPid({ pid: 1111 }, 4242)).toBe(1111); | ||
| expect(resolveStatusPid(null, 4242)).toBe(4242); | ||
| expect(resolveStatusPid(null, null)).toBeNull(); | ||
| }); |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | 🏗️ Heavy lift
Add consumer-level regressions for verified live proxies without a usable PID file.
The new unit test only validates resolveStatusPid; it cannot catch incorrect status JSON/URLs or doctor restart and memory behavior when findLiveProxy() discovers an orphaned runtime record.
tests/cli-status-json.test.ts#L288-L293: add acollectStatus()regression with a missing/stale PID file and identity-verified runtime-record proxy; assertrunning, health/dashboard URLs, resolved PID, fallback port, andlisten.source.src/cli/status.ts#L143-L169: keep the status regression focused on the liveness-first branch.src/cli/status.ts#L263-L280: assert the returned JSON reports the verified proxy as running even without a valid PID file.src/cli/doctor.ts#L722-L736: add a doctor regression for the same scenario.src/cli/doctor.ts#L758-L761: assert doctor fetches runtime diagnostics from the verified live port.src/cli/doctor.ts#L824-L825: assert doctor does not emit an unnecessary proxy-restart hint.
As per path instructions, “A behavior change in src/ should come with a focused regression test near the existing tests for that subsystem.”
📍 Affects 3 files
tests/cli-status-json.test.ts#L288-L293(this comment)src/cli/status.ts#L143-L169src/cli/status.ts#L263-L280src/cli/doctor.ts#L722-L736src/cli/doctor.ts#L758-L761src/cli/doctor.ts#L824-L825
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tests/cli-status-json.test.ts` around lines 288 - 293, Strengthen the
consumer-level regressions for an identity-verified live proxy with a missing or
stale PID file: in tests/cli-status-json.test.ts:288-293, add a collectStatus()
case asserting running state, health/dashboard URLs, resolved PID, fallback
port, and listen.source; keep src/cli/status.ts:143-169 focused on the
liveness-first branch and ensure src/cli/status.ts:263-280 reports the verified
proxy as running. Add the matching doctor regression in
src/cli/doctor.ts:722-736, asserting at src/cli/doctor.ts:758-761 that
diagnostics use the verified live port and at src/cli/doctor.ts:824-825 that no
unnecessary proxy-restart hint is emitted.
Source: Path instructions
|
Transition complete:
|
1 participant
Summary
ocx status --jsonandocx doctorpreferfindLiveProxy()overocx.pidalone so a healthy Task Scheduler proxy is not reported stopped.running: true.Test plan
bun run typecheckCloses #618
Summary by CodeRabbit
ocx statusaccuracy by verifying proxy liveness through health checks and runtime information.ocx doctordiagnostics to identify live proxies more reliably.