Add post-sync readiness endpoint and bounded ocx ready wait - #569
Conversation
|
Important Review skippedNo new commits to review since the last review. ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThe change adds per-server readiness gates, a strict unauthenticated ChangesProxy readiness flow
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant CLI
participant findLiveProxy
participant Proxy
participant ReadinessGate
participant StartupSync
CLI->>findLiveProxy: Discover proxy
findLiveProxy->>Proxy: Check identity
CLI->>Proxy: GET /readyz
Proxy->>ReadinessGate: Read status
ReadinessGate-->>Proxy: pending, ready, or failed
Proxy-->>CLI: Sanitized readiness response
StartupSync->>ReadinessGate: Mark ready or failed
Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
|
Please change the PR to ready for review once you are finished @diegocantarero |
|
NEEDS-SECURITY-REVIEW — the design is sound, the boundary is what needs a reviewer. The separation is the right one. The tests are unusually strong for a PR this size. Why it still needs security review. This deliberately adds a new unauthenticated endpoint. The response is narrowly sanitized to fixed identity and status fields, and The three conflicts all want both sides kept, not a choice:
What happens next: rebase keeping both sides of those three, mark it ready when you are satisfied, run fresh CI, then request security review. |
|
Thanks for putting this together. The separation between process liveness and post-sync readiness is the right design, and it addresses a real startup race. Before maintainer review, please:
The design direction is right and the startup race is real — we want this. |
0a1b94f to
55f4937
Compare
![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: 55f493777a
ℹ️ 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".
[shipping-github] Addressed feedbackfeedback: issue_comment:5154177408 |
[shipping-github] Security review requested@lidge-jun — requesting the deliberate security review you asked for on the unauthenticated Head: This is the last open gate before merge; please review the boundary. |
|
Please resolve conflicts. |
d54ceec to
15613e6
Compare
|
✅ Deterministic PR hygiene checks passed. |
4a4b5a3 to
a3205ab
Compare
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
There was a problem hiding this comment.
Actionable comments posted: 8
🤖 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 `@docs-site/src/content/docs/reference/cli/lifecycle.md`:
- Line 151: Update the lifecycle documentation sentence around the `--wait`
behavior to replace “terminal `failed`” with “the terminal `failed` state,”
preserving the rest of the polling and timeout wording.
In `@src/server/index.ts`:
- Around line 591-620: Update the /readyz handling around
readinessGate.getStatus() to check isDraining() first and report a non-ready
status while the listener drains, without mutating readinessGate. Use "pending"
for draining so polling supervisors can continue, then preserve the existing 503
response and Retry-After behavior for non-ready statuses and the 200 response
for ready status.
In `@src/server/proxy-liveness.ts`:
- Around line 145-147: Remove the legacy deadlineMs fallback and type cast from
findLiveProxy. Use io.deadlineAt directly and pass the original io as probeIo,
preserving the existing LivenessIo contract without accepting an undocumented
alias.
In `@tests/cli-ready-subprocess.test.ts`:
- Around line 112-121: Adjust the subprocess timing test around runCli and its
elapsedMs assertion: increase the process kill budget and raise the elapsed-time
ceiling beyond 2,000 ms so CLI startup is not mistaken for readiness timeout
behavior. Keep the existing readiness-hit, timedOut, and exitCode assertions
unchanged.
In `@tests/cli-ready.test.ts`:
- Line 777: Update the assertion in the relevant describe block around
readySource to use a whitespace-tolerant regular expression for the remainingMs
=== undefined ternary and its verifyPidFn fallback, rather than matching a
newline and literal indentation. Preserve the existing non-wait contract while
allowing the source formatting to change.
- Around line 207-263: Correct the timing comments in the three tests around
runReady to reflect that each now stub increments before returning: the
effective deadlines are 5100ms, 1500ms, and 4000ms respectively. Keep the test
logic and assertions unchanged unless you instead offset each initial t value
consistently, as done in the existing deadline test.
In `@tests/proxy-liveness.test.ts`:
- Around line 449-459: Replace the vacuous JSON.stringify checks in the test
“the gate exposes only the fixed status enum (no reason/changedAt payload)” with
assertions that directly verify the gate object exposes only getStatus,
markReady, and markFailed. Preserve the existing failed-status assertion and
assert the actual public key set rather than closure state or serialized
function properties.
In `@tests/update-notify.test.ts`:
- Line 127: Add an explicit assertion immediately after computing serverIndex
from cli.search, requiring the result to be nonnegative before using it in the
startup ordering comparison. Keep the existing ordering assertion unchanged so a
missing startServer(port) match fails loudly instead of passing with -1.
🪄 Autofix
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: 32cfee2b-72d9-4165-b80e-546078114862
📒 Files selected for processing (21)
README.mddocs-site/src/content/docs/ja/reference/cli/lifecycle.mddocs-site/src/content/docs/ko/reference/cli/lifecycle.mddocs-site/src/content/docs/reference/cli/lifecycle.mddocs-site/src/content/docs/ru/reference/cli/lifecycle.mddocs-site/src/content/docs/zh-cn/reference/cli/lifecycle.mdsrc/cli/help.tssrc/cli/index.tssrc/cli/ready.tssrc/codex/desired-state.tssrc/server/index.tssrc/server/proxy-liveness.tssrc/server/readiness.tsstructure/03_catalog-and-subagents.mdtests/cli-catalog-prewarm.test.tstests/cli-ready-subprocess.test.tstests/cli-ready.test.tstests/cli-restart-health.test.tstests/proxy-liveness.test.tstests/server-live.test.tstests/update-notify.test.ts
Extract the 750ms per-probe ceiling as DEFAULT_PROBE_TIMEOUT_MS in proxy-liveness and import it from the ready CLI instead of redeclaring IO_TIMEOUT_CAP_MS, so liveness and readiness defaults cannot diverge.
…tion verifyPidIdentity spawns WMIC/PowerShell (up to seconds on Windows) and is only needed for kill targets. runReady's production discovery now passes verifyPidFn: () => null so no OS command-line check runs outside the wait deadline; the /healthz identity marker and strict /readyz contract validation are unchanged.
Rebase the readiness PR onto current dev and resolve the two conflict surfaces against dev's modern startup path: - src/cli/index.ts: keep syncCodexOnStartIfEnabled (Codex toggle + lidge-jun#1046 stale app-server warning) and thread the readiness gate into it as the fourth argument, so /readyz reflects the real sync outcome without bypassing the toggle or the write-tracking contract. - src/codex/desired-state.ts: accept an optional readiness gate; mark ready immediately when the Codex integration is explicitly OFF (nothing to sync), otherwise drive the gate via runStartupReadinessSync from the raw sync outcome (ready only on ok=true with no nonempty warning). - src/server/readiness.ts: runStartupReadinessSync returns the raw sync outcome so callers keep the lidge-jun#1046 write flags without a second call. - structure/03_catalog-and-subagents.md: un-indent the pre-existing cache paragraph and fix the Startup readiness heading to top-level markdown. - tests/cli-ready.test.ts: update the source-level wiring guard to assert the new syncCodexOnStartIfEnabled(port, config, undefined, readinessGate) contract instead of the pre-rebase inline wiring.
- src/server/index.ts: /readyz reports pending (503) while the listener is draining instead of advertising ready; the one-shot readiness gate is not mutated on shutdown (startup-sync ownership preserved). - src/server/proxy-liveness.ts: drop the untyped deadlineMs alias in findLiveProxy; no caller supplies it and LivenessIo only declares deadlineAt. - tests/server-live.test.ts: add a draining /readyz regression test. - tests/proxy-liveness.test.ts: replace the vacuous JSON.stringify privacy assertion with a direct own-property surface check. - tests/cli-ready-subprocess.test.ts: raise the subprocess timing ceiling so cold Bun startup on CI is not mistaken for readiness timeout behavior. - tests/cli-ready.test.ts: correct off-by-one-tick deadline comments and make the non-wait wiring assertion whitespace-tolerant. - tests/update-notify.test.ts: fail loudly when startServer(port) is not found instead of silently passing via -1. - docs-site/.../lifecycle.md: clarify the terminal failed wording.
… branch The generic OPTIONS handler ran before the /readyz exact-GET route, so OPTIONS /readyz (and /readyz/) answered 204 instead of the deterministic JSON 404 the exact-method contract promises. Reject both paths in the OPTIONS branch and add regression coverage.
…ontract Compare the DECODED pathname for /readyz routing so an encoded variant like /readyz%2F (which decodes to /readyz/) cannot bypass the exact-path rejection and reach the GUI fallback (serveGuiFile decodes the pathname and would serve index.html with 200). GET, POST, and OPTIONS on encoded variants all answer the deterministic JSON 404. Strengthen the OPTIONS regression test to assert the JSON 404 body (content-type, error.type, and the path-specific error.message) and add encoded-path coverage for GET, POST, and OPTIONS.
6d2a8ce to
bb1aa2e
Compare
✅ READY
Review readiness checklist
✅ 4/4 boxes ticked. This pull request is already Ready for Review. |
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
|
✅ Action performedReview finished.
|
|
|
|
|
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: bb1aa2e358
ℹ️ 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.
Actionable comments posted: 3
🤖 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 `@docs-site/src/content/docs/reference/cli/lifecycle.md`:
- Line 152: Update every readiness reference to document the complete --timeout
contract: in docs-site/src/content/docs/reference/cli/lifecycle.md lines
152-152, state that values are positive integers from 1 through 300; in
README.md lines 165-165, add the positive-integer constraint; in
docs-site/src/content/docs/ja/reference/cli/lifecycle.md lines 125-126,
docs-site/src/content/docs/ko/reference/cli/lifecycle.md lines 153-153,
docs-site/src/content/docs/ru/reference/cli/lifecycle.md lines 164-164, and
docs-site/src/content/docs/zh-cn/reference/cli/lifecycle.md lines 124-124, add
equivalent localized wording; and in structure/03_catalog-and-subagents.md lines
47-50, document that --timeout requires --wait and accepts positive integers
from 1 through 300.
In `@src/server/proxy-liveness.ts`:
- Around line 246-253: Update the ReadinessProbeResult interface to make status
non-nullable and require pid and port as valid numbers, matching the non-null
values emitted by validateReadyzBody. Keep null handling at the probe result
level for callers such as src/cli/ready.ts, including its optional chaining and
fallback behavior.
In `@tests/cli-ready-subprocess.test.ts`:
- Around line 164-171: Update the runCli invocation in this subprocess test to
pass the same 10,000 ms killAfterMs budget used by the sibling test, while
preserving the existing arguments and environment variables.
🪄 Autofix
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: 9f80db9f-877d-480f-9aa2-8f61f676c9d5
📒 Files selected for processing (21)
README.mddocs-site/src/content/docs/ja/reference/cli/lifecycle.mddocs-site/src/content/docs/ko/reference/cli/lifecycle.mddocs-site/src/content/docs/reference/cli/lifecycle.mddocs-site/src/content/docs/ru/reference/cli/lifecycle.mddocs-site/src/content/docs/zh-cn/reference/cli/lifecycle.mdsrc/cli/help.tssrc/cli/index.tssrc/cli/ready.tssrc/codex/desired-state.tssrc/server/index.tssrc/server/proxy-liveness.tssrc/server/readiness.tsstructure/03_catalog-and-subagents.mdtests/cli-catalog-prewarm.test.tstests/cli-ready-subprocess.test.tstests/cli-ready.test.tstests/cli-restart-health.test.tstests/proxy-liveness.test.tstests/server-live.test.tstests/update-notify.test.ts
- src/cli/ready.ts: clear the cached status/pid/port when discovery stops finding the proxy, so a proxy that reported pending and then exited is reported as unreachable at timeout instead of stale pending (P2). - src/server/proxy-liveness.ts: narrow ReadinessProbeResult to non-null members (a foreign/unreadable body yields a null RESULT, never a null member); the producers only ever emit valid values. - tests/cli-ready.test.ts: regression test for the stale-pending fix. - tests/cli-ready-subprocess.test.ts: enlarge the second subprocess kill budget to 10s (CI cold-start tolerance, matching the sibling test). - README, lifecycle docs (en + 4 locales), structure doc: document that --timeout accepts positive integer seconds from 1-300.
|
✅ Action performedReview finished.
|
|
|
|
|
|
[GD] Addressed feedback feedbacks:
commit: a93fc4a |
[GD] Verdict: approve-commentTLDR
Full verdictSemantic propagation
UsefulnessThe PR addresses a genuine startup race: the proxy listener binds before the post-startup Codex catalog sync settles, so a consumer treating Bugs / correctness
Security
Spec / standards
Reviews
Base / CI
Simplification (for the PR owner)Simplify was explicitly requested. Candidate pass on the rebased head: nothing worth simplifying. The Gatenone — draft cleared, marked ready for review, Bottom lineShip. The readiness contract is well-designed and safely sanitized, the maintainer overtake rebased it onto current |
[GD] Merge readyPR: #569 — Add post-sync readiness endpoint and bounded ocx ready wait Reviews
Tip freshness
Checks
Residualnone Ready to merge. |
|
Thanks @diegocantarero — merging this. Why it helps: the liveness/readiness split closes a real startup race where a consumer could treat Ship it. |
3 participants
Summary
GET /healthzas immediate process livenessGET /readyzfor post-sync readinessocx ready [--json] [--wait [--timeout <seconds>]]Problem
The listener can bind before startup catalog/config synchronization finishes. A consumer that treats
/healthzas readiness can therefore connect to a live proxy before model catalog convergence.This closes that portable startup race. It was observed while macOS restored a Codex client during login, but the implementation is platform-neutral and does not include macOS process-management policy.
Behavior
/healthzis unchanged./readyzreturns200only forready.pendingandfailedreturn503withRetry-After: 1.POST /readyzandGET /readyz/return a JSON404(the endpoint matches exact pathname +GETonly, regardless of whether the packaged dashboard build exists).ok: truewith no nonempty warningocx readyvalidates service identity and HTTP/status consistency--waituses one hard deadline across discovery, probes, polling, and sleep/readyzfail closed as readiness unavailableocx ready --waitinstead of guessing from liveness/readyzunauthenticated response contractExactly these six fields, nothing else:
{ "service": "opencodex", "version": "<string>", "uptime": "<number>", "pid": "<integer>", "port": "<integer>", "status": "pending | ready | failed" }Why this is safe for an unauthenticated caller: every field is already exposed by the existing unauthenticated
/healthz(service,version,uptime,pid,port) or is the readiness state itself (status) that the endpoint exists to report. No sync message, warning text, catalog path, provider output, account data, or diagnostic text is included, and the strict/readyzprobe treats any foreign or malformed body asunreachable.Compatibility and privacy
/healthzclients retain their behaviorstartServer(port)callers remain source-compatible and fail closed as pending unless they own and transition a supplied gateMerge-ready work (rebase + review fixes)
dev(fa51fce54), keeping both sides of the previously conflicting hunks:startServer(port, { readinessGate })withscheduleCatalogPrewarm(), the management-auth imports, anddeadlineAtdiscovery budgeting with thesourcediscriminatordev's existingdeadlineAtdiscovery budget instead of the duplicatedeadlineMs/probeBudgetmechanismdocs-site/.../reference/cli/lifecycle.mdin all locales and documented the 1-300 second--timeoutrangeAbortSignal.timeoutstub ordering/readyzcontract deterministic (JSON404for non-GET/trailing-slash paths)Validation
bun run typecheck: passEPERMtests and acodex-v2-gateassertion that depends on an npm-stylenode_modules/.bin/codex.cmdshim; both reproduce outside this PR); the maintained GitHub CI matrix is the full-suite acceptance gatebun run privacy:scan: passbun run build): passReview readiness checklist
This PR stays in draft until every box below is ticked. Tick all four boxes once the requirements are met:
All CI tests are green on my local testing.
I pushed my PR to the latest dev commit.
I resolved all correct Codex and CodeRabbit findings.
My PR is ready for review.
Summary by CodeRabbit
New Features
ocx readycommand for one-time or polling-based readiness checks./readyzendpoint with pending, ready, and failed states./readyzfail closed safely.Documentation
/healthzbehavior across supported languages.