Repository navigation
fix(server): let SDK consumers mount the Codex proxy door - #1454
Conversation
createAllRoutes assembled two of the three proxy doors. createCodexProxyRoutes was neither imported nor re-exported from the routes barrel, and no flag selected it, so Codex proxying was reachable only by running `neurolink proxy start`. A consumer embedding the server could not expose it at all — not even deliberately. Adds a codexProxy flag and mounts the door under it. The unified `proxy` flag now means every door rather than two of three: a consumer opting into proxying is opting into proxying, and the omission is what let this go unnoticed. Off by default, as before. While wiring it, a wider gap: the server entry re-exported no proxy factory at all. Claude and OpenAI were reachable only through the deep path ./routes/index.js, which is not a package export, so a consumer of "@juspay/neurolink/server" could never mount one directly. All three are exported now. Codex was also the one configurator with no apply/restore round-trip test — issue #1368 was closed as covered while the writer with the most to get wrong had none. It edits TOML by regex rather than round-tripping JSON, keeps its snapshot in a sidecar file, and rewrites a top-level selector that must stay in the preamble: a model_provider read from inside a [profiles.*] table would be written back as the global selector, silently repointing every Codex run at another provider. The test drives the real writer against a config carrying exactly that hazard, in two cases. The first — a user selector present — passes against both a correct writer and a broken one, because document-wide and preamble-scoped matching agree when the preamble already has a selector. The second case is the one that separates them: no global selector, only a profile's. Confirmed by regressing the writer to match document-wide and watching it fail, then restoring it. Both tests run through the surface a consumer uses: the SDK seam is checked by importing the built package and calling createAllRoutes, not by reading source.
|
Warning Review limit reached
Next review available in: 2 minutes Limit details: You’ve used all 2 included reviews currently available. You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?Wait for the limit to reset, then comment An organization admin can change what happens after included review limits in Billing. How do review limits work?CodeRabbit enforces per-developer PR review limits within each organization. For paid Pro and Pro+ reviews, CodeRabbit uses a developer's included PR review attempts over the past 7 days to set the current hourly allowance. At typical activity levels, the full plan allowance applies. Higher sustained activity can lower the allowance until earlier attempts leave the 7-day 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)
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 |
✅ Single Commit Policy - COMPLIANTStatus: Policy requirements met • 1 commit • Valid format • Ready for merge 📊 View validation details📝 Commit Details
✅ Validation Results
🤖 Automated validation by NeuroLink Single Commit Enforcement |
🤖 AI Review & Build Compliance ✅Status: AI analysis complete • Build rules validated • Ready for review 📊 View detailed analysis results🛡️ Analysis Complete
📋 Ready for Merge When
🤖 AI analysis complete - check individual code comments for specific feedback |
Review SummaryDecision: APPROVED ✅ FindingsNo issues found. This is a clean feature addition that properly exposes Codex proxy functionality through the SDK. Changes ReviewedFile:
File:
File:
File:
File:
Impact on Existing Code
Review ScopeReviewed 5 changed files covering:
The implementation is solid, well-tested, and follows established patterns in the codebase. No action required beyond merging. |
|
🎉 This PR is included in version 11.17.1 🎉 The release is available on: Your semantic-release bot 📦🚀 |
Two gaps that share a root cause: Codex was treated as CLI-only.
1. An SDK consumer could not mount the Codex door at all
createAllRoutesassembled two of the three proxy doors.createCodexProxyRouteswas neither imported nor re-exported from the routes barrel, and no flag selected it — so Codex proxying was reachable only by runningneurolink proxy start.Verified through the published surface, not by reading source:
The unified
proxyflag now means every door rather than two of three. A consumer opting into proxying is opting into proxying, and the silent omission is exactly what let this sit unnoticed. Flag me if you'd ratherproxystayed Claude+OpenAI and Codex required its own opt-in — it's a judgement call, and reverting it is one line.A wider gap found while wiring it: the server entry re-exported no proxy factory. Claude and OpenAI were reachable only via the deep path
./routes/index.js, which is not a package export — so@juspay/neurolink/serverconsumers could never mount one directly. All three are exported now.2. Codex had no configurator round-trip test — #1368 was closed as covered
Codex is the writer with the most to get wrong: it edits TOML by regex rather than round-tripping JSON, keeps its snapshot in a sidecar file, and rewrites a top-level selector that must stay in the preamble. A
model_providerread from inside a[profiles.*]table would be written back as the global selector, silently repointing every Codex run at another provider.The test drives the real writer against a config carrying that hazard, in two cases:
Worth recording: my first version of case B asserted
/^model_provider = /mon the whole file. That matches the line inside[profiles.work]too, so it reported a bug that did not exist. The writer was pristine the whole time; the assertion is now scoped to the preamble.Verification
tsc --noEmit --stricteslint src testpnpm run buildcontinuous-test-suite-proxycontinuous-test-suite-serverscontinuous-test-suite-codexStacks cleanly with #1453 (Codex model discovery); the two touch different files.