fix: keep Codex MCP responsive during startup - #560
Conversation
|
Warning Review limit reachedYou’ve reached a temporary PR review limit under our Fair Usage Limits Policy. Next review available in: 44 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (5)
📝 WalkthroughWalkthroughMadar now starts stdio transport while initial graph reconciliation runs in a worker, keeps discovery responsive, rejects graph-backed calls until watcher readiness, adds a 180-second managed Codex startup timeout, and publishes corresponding tests, documentation, and 0.31.2 metadata. ChangesMCP readiness during background reconciliation
Codex managed MCP startup configuration
0.31.2 release documentation and metadata
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Codex
participant StdioServer
participant BackgroundWorker
participant Watcher
Codex->>StdioServer: Start MCP stdio transport
StdioServer->>BackgroundWorker: Start initial reconciliation
BackgroundWorker->>Watcher: Run graph auto-refresh
StdioServer-->>Codex: Respond to initialize and discovery
Codex->>StdioServer: Request graph-backed operation
StdioServer-->>Codex: Return readiness error until watcher is idle
Watcher-->>BackgroundWorker: Report reconciliation completion or failure
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (1)
tests/unit/background-auto-refresh.test.ts (1)
92-160: 📐 Maintainability & Code Quality | 🔵 Trivial | 🏗️ Heavy liftCover the transition from blocked to ready.
This test stops without publishing a usable graph and matching watcher state. Extend it to complete reconciliation, then assert that a graph-backed request succeeds; otherwise a permanently gated server regression passes unnoticed.
🤖 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/unit/background-auto-refresh.test.ts` around lines 92 - 160, The test should complete the slow auto-refresh reconciliation before finishing, then issue a graph-backed request through serveGraphStdio and assert it succeeds once the watcher and graph are ready. Extend the existing “completes MCP discovery…” scenario using its graphPath, completionMarker, and output assertions, while preserving the current startup-time failure checks.
🤖 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/auto-refresh.md`:
- Line 3: Update the watcher-health guidance’s refusal-state list to include the
starting state, matching the documented fail-closed behavior for graph-backed
requests before the refresh worker begins. Keep the existing refusal states and
surrounding readiness guidance unchanged.
In `@README.md`:
- Around line 74-76: Update the README readiness guidance near the Madar upgrade
instructions to document the complete graph-readiness contract: require startup
completion, a non-blocking watcher state, and matching published policy in
addition to an idle watcher. Align the wording with the readiness conditions in
docs/reference/cli-and-mcp.md, and avoid implying that madar status reporting
idle alone guarantees graph-backed calls succeed.
In `@src/infrastructure/background-auto-refresh.ts`:
- Around line 95-108: Update markFailed to make all watcher-state read and write
operations best-effort: catch persistence errors, report them through the
existing stderr/controller failure-reporting path, and prevent any exception
from escaping worker failure handling. Preserve the existing PID guard and
failed-state contents when persistence succeeds.
In `@src/runtime/stdio-server.ts`:
- Around line 196-225: Update the readiness calculation in the surrounding
startup-status function to require that controller.failureReason() is absent, so
a terminal failure from completedFailureController prevents ready from becoming
true even when stale watcher state is idle and policy-matching. If runtime
failures are recoverable, clear the controller failure signal after successful
reconciliation while preserving the existing watcher-state readiness checks.
---
Nitpick comments:
In `@tests/unit/background-auto-refresh.test.ts`:
- Around line 92-160: The test should complete the slow auto-refresh
reconciliation before finishing, then issue a graph-backed request through
serveGraphStdio and assert it succeeds once the watcher and graph are ready.
Extend the existing “completes MCP discovery…” scenario using its graphPath,
completionMarker, and output assertions, while preserving the current
startup-time failure checks.
🪄 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: CHILL
Plan: Pro Plus
Run ID: a55247c2-db06-4719-933e-3d411ab47f03
📒 Files selected for processing (18)
CHANGELOG.mdREADME.mddocs/auto-refresh.mddocs/mcp-registry/server.jsondocs/reference/cli-and-mcp.mddocs/release.mddocs/tutorials/agent-quickstarts.mdpackage-lock.jsonpackage.jsonsbom.cdx.jsonsrc/infrastructure/background-auto-refresh.tssrc/infrastructure/install.tssrc/infrastructure/watch.tssrc/runtime/stdio-server.tssrc/runtime/stdio/prompts.tssrc/runtime/stdio/resources.tstests/unit/background-auto-refresh.test.tstests/unit/install.test.ts
1 participant
Summary
startup_timeout_sec = 180in the managed Codex MCP block without replacing unrelated TOML0.31.2Root cause
madar serve --stdio --auto-refreshperformed its initial graph reconciliation on the same event-loop thread that had to answer the MCP initialization handshake. Large workspaces and synchronized folders could therefore exceed Codex's default 30-second startup timeout before the transport became responsive.User impact
Codex can initialize and inspect the Madar MCP server while reconciliation continues. Graph-backed answers remain unavailable until the graph is safe to read, so responsiveness does not expose stale or partially written evidence. Existing installations should rerun
madar codex installafter upgrading to receive the 180-second managed startup window.Validation
npm run release:verifynpm run registry:validatenpm run typechecknpm run build@lubab/madar@0.31.2(396 files, 732.6 kB)0.31.2Closes #559
Summary by CodeRabbit
New Features
Bug Fixes
Documentation