Repository navigation
fix(proxy): harden updater fallback and terminal errors - #1166
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
✅ 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 |
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (13)
📝 WalkthroughWalkthroughThe proxy separates launchd updater workers from fail-open cleanup, tracks runtime activity and request errors, resolves global installers for self-updates, hardens Claude fallback retries and timeouts, and reports streaming lifecycle outcomes. ChangesProxy runtime and telemetry
Launchd updater and installer
Fallback and streaming reliability
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant Launchd
participant UpdaterWorker
participant ProxyStatus
participant GlobalInstaller
participant PackageManager
Launchd->>UpdaterWorker: Start updater-only worker
UpdaterWorker->>ProxyStatus: Poll runtime activity
ProxyStatus-->>UpdaterWorker: Return quiet-window state
UpdaterWorker->>GlobalInstaller: Resolve owning installer
GlobalInstaller->>PackageManager: Probe and install package
UpdaterWorker->>Launchd: Kickstart validated proxy
Possibly related PRs
Suggested labels: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
ESLint install failed due to a network error. 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 |
🤖 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 |
Documentation Validation Results🚀 Documentation validation passed!
📦 Build artifact uploaded successfully. Ready for deployment preview. Commit: |
There was a problem hiding this comment.
Pull request overview
Hardens the NeuroLink Claude proxy’s self-updater and fallback/stream error handling to prevent unsafe restarts, reduce indefinite hangs, and improve operator diagnostics + telemetry accuracy.
Changes:
- Introduces in-process request activity tracking and exposes it via
/status/neurolink proxy statusto gate updater install/restart on true runtime idleness. - Replaces updater package-manager selection with a “global root + bin dir must be writable and must own the running install” resolver, and adds richer install-failure diagnostics.
- Adds bounded fallback initialization/stream idle timeouts + a retry for transient network failures, and propagates Anthropic SSE terminal errors/incomplete streams as OpenAI
server_errorevents.
Reviewed changes
Copilot reviewed 13 out of 13 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| test/proxyUpdaterFallback.test.ts | Adds focused tests for proxy activity tracking, runtime error finalization, installer resolution, fallback retry/timeout behavior, and stream error bridging. |
| test/proxyReliabilityHardening.test.ts | Updates launchd/updater source invariants to reflect the new updater worker + installer probing behavior. |
| test/continuous-test-suite-bugfixes.ts | Updates continuous regression checks to match the new kickstart/installer/updater logging behavior. |
| src/lib/types/proxy.ts | Adds types for runtime activity snapshots, request metadata, and global-installer resolution/probes. |
| src/lib/types/cli.ts | Adds updaterOnly guard arg and persists updater PID in proxy state. |
| src/lib/server/routes/openaiProxyRoutes.ts | Tracks terminal stream errors and finalizes lifecycle after streaming completes (or on cancel/error). |
| src/lib/server/routes/claudeProxyRoutes.ts | Adds fallback timeouts, retry-on-transient-network behavior, and preserves fallback failure diagnostics for terminal errors. |
| src/lib/proxy/proxyActivity.ts | Implements request activity tracking and response-body completion tracking. |
| src/lib/proxy/openaiFormat.ts | Extends Claude→OpenAI stream transform to treat Anthropic error/incomplete streams as terminal error events (no fake success). |
| src/lib/proxy/globalInstaller.ts | Adds global installer resolution/probing and install failure description utilities. |
| src/cli/commands/proxy.ts | Spawns a dedicated updater-only worker for launchd installs; gates updates on runtime idleness; improves error telemetry and /status payload. |
| docs/features/claude-proxy.md | Documents updater-only worker behavior and new default auto-update semantics. |
| docs/features/claude-proxy-config-reference.md | Updates guard/updater documentation and adds the new environment variables for installer selection. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
src/lib/server/routes/openaiProxyRoutes.ts (1)
235-294: 📐 Maintainability & Code Quality | 🔵 TrivialConsider reusing the shared stream-outcome tracker instead of a bespoke implementation.
This hand-rolled
trackedStream/finishLifecycle/lifecycleWrittenpattern duplicates the same "track terminal stream outcome (success/error/cancel) exactly once" problem already solved bycreateStreamTerminalOutcomeTracker/trackUpstreamReadableStreamin../../proxy/streamOutcome.js, whichclaudeProxyRoutes.tsalready imports and uses. Extracting/exporting that helper for reuse here would avoid two parallel implementations of the same lifecycle-tracking logic drifting apart over time.🤖 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 `@src/lib/server/routes/openaiProxyRoutes.ts` around lines 235 - 294, Replace the bespoke trackedStream, finishLifecycle, and lifecycleWritten logic with the shared createStreamTerminalOutcomeTracker/trackUpstreamReadableStream utilities from ../../proxy/streamOutcome.js. Export or adapt the existing helper as needed, preserving the current 200 success, 502 stream-error, and 499 client-cancelled lifecycle outcomes and ensuring each outcome is recorded exactly once.
🤖 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 `@src/cli/commands/proxy.ts`:
- Around line 1273-1275: Sanitize the request-derived model value before
interpolating it into the unstructured logger.always message in the proxy
request logging flow. Update the logging around model and preserve the existing
method, path, stream, and toolCount output while ensuring CR/LF characters
cannot create forged log entries.
- Around line 714-721: Move the updater log directory creation and file opening
in the proxy startup flow into the existing try/catch surrounding updater setup.
If mkdirSync or openSync fails, catch the error and disable or skip updater
logging without aborting launchd-managed proxy startup; preserve normal logging
when setup succeeds.
---
Nitpick comments:
In `@src/lib/server/routes/openaiProxyRoutes.ts`:
- Around line 235-294: Replace the bespoke trackedStream, finishLifecycle, and
lifecycleWritten logic with the shared
createStreamTerminalOutcomeTracker/trackUpstreamReadableStream utilities from
../../proxy/streamOutcome.js. Export or adapt the existing helper as needed,
preserving the current 200 success, 502 stream-error, and 499 client-cancelled
lifecycle outcomes and ensuring each outcome is recorded exactly once.
🪄 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
Run ID: 7936c08f-e384-41bd-8d28-21fc1a9fc31b
📒 Files selected for processing (13)
docs/features/claude-proxy-config-reference.mddocs/features/claude-proxy.mdsrc/cli/commands/proxy.tssrc/lib/proxy/globalInstaller.tssrc/lib/proxy/openaiFormat.tssrc/lib/proxy/proxyActivity.tssrc/lib/server/routes/claudeProxyRoutes.tssrc/lib/server/routes/openaiProxyRoutes.tssrc/lib/types/cli.tssrc/lib/types/proxy.tstest/continuous-test-suite-bugfixes.tstest/proxyReliabilityHardening.test.tstest/proxyUpdaterFallback.test.ts
Tara-ag
left a comment
Tara-ag
left a comment
There was a problem hiding this comment.
Review summary
Reviewed 13 changed files. I did not raise any new blocking issues this run — the existing review threads (Copilot + CodeRabbit) already cover the substantive concerns I would have flagged:
writableDirectory()should also require traverse (X_OK) permission (src/lib/proxy/globalInstaller.ts:33) — already raised.- Anthropic SSE
errorevents should terminate the transform rather than leave the stream open (src/lib/proxy/openaiFormat.ts:932) — already raised. - OpenAI bridge stream cancel path should not misclassify a terminal stream error as
499 client_cancelled(src/lib/server/routes/openaiProxyRoutes.ts:290) — already raised. - Updater log setup should fail open so an unwritable log dir does not abort launchd-managed startup (
src/cli/commands/proxy.ts:714-721) — already raised. - Request-derived
modelshould be sanitized before unstructured logging (src/cli/commands/proxy.ts:1273-1275) — already raised.
Blocking-criteria check
- No hardcoded secrets or credentials.
- No critical security vulnerabilities (injection, SSRF, unsafe eval, secret/PII exposure) introduced.
- No breaking change to the public SDK API; changes are confined to the CLI/proxy path.
- No violation of a non-negotiable CLAUDE.md Critical Rule that ESLint does not enforce.
- Fewer than 3 new MAJOR issues raised by this review.
Notes
- The new request-activity tracking (
proxyActivity.ts) correctly holds requests open until the response body settles, and the updater gates install/restart on true runtime idleness. - The global-installer resolver correctly rejects package managers whose writable global root does not own the running entry script, addressing the reported pnpm/npm shadowing problem.
- Fallback translation now has bounded initialization/stream idle timeouts, upstream abort on timeout, and one transient-network retry.
- Anthropic SSE errors and incomplete streams are propagated as OpenAI
server_errorevents without a fake[DONE]success terminator. - Uncaught adapter errors and invalid JSON are recorded in request logs, body capture, and usage stats while returning a generic client-safe response.
Approving — please address the existing reviewer comments before merging.
🤖 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 |
0fc02b1 to
d2f93bd
Compare
🤖 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 |
|
🎉 This PR is included in version 9.87.4 🎉 The release is available on: Your semantic-release bot 📦🚀 |
Tara-ag
left a comment
Tara-ag
left a comment
There was a problem hiding this comment.
Review Summary
Reviewed 13 changed files. Raised 6 new inline comments (2 Major, 4 Minor). No blocking criteria are met this run.
New issues
Major (2)
src/lib/proxy/openaiFormat.ts:932— Terminal Anthropicerrorevents emit an OpenAI error frame but do not terminate the transform, so the stream can linger if the upstream connection stays alive.src/lib/server/routes/openaiProxyRoutes.ts:290— The stream cancel path always records499 client_cancelled, even when a terminal upstream stream error was already observed, misclassifying the lifecycle outcome.
Minor (4)
src/lib/proxy/globalInstaller.ts:33—writableDirectory()checks onlyW_OK; directories also need traverse (X_OK) permission for reliable installability detection.src/cli/commands/proxy.ts:705— Updater log file is created with default umask; consider0o700directory and0o600log file permissions.src/cli/commands/proxy.ts:1275— Request-derivedmodelis logged unsanitized; CR/LF characters can forge log entries.src/lib/proxy/proxyActivity.ts:42—trackProxyResponsedrops the cancellation reason when cancelling the upstream reader.
Existing comments
Acknowledged the 5 prior review comments from copilot-pull-request-reviewer and coderabbitai; I did not duplicate them.
Decision
Approve. The issues raised are correctness/quality improvements, not security blockers or public-API-breaking changes. The two Major items should be addressed in a follow-up to avoid stream-lifecycle misclassification.
Description
Hardens the Claude proxy updater, fallback execution, stream termination, and adapter-level error telemetry based on the recent live failure analysis.
Root Causes Addressed
fetch failedbypassed structured request/error statisticsChanges
NEUROLINK_PROXY_AUTO_UPDATE=off|false|0bootout/bootstrapwith atomiclaunchctl kickstart -k; never rewrite or unload the stable plist during an updateserver_errorevents without fake[DONE]success/statusandneurolink proxy statusTesting
pnpm run buildincluding package/declaration build, browser build, CLI build, and publintpnpm run test:unithas one pre-existing failure:test/modelPool.test.tstimes out at its fixed 5-second limit. It reproduces unchanged in the untouched original checkout; all 18 other tests in that file pass.Deployment Notes
The currently installed older proxy cannot gain the new updater worker by itself. Upgrade once to the release containing this PR and restart through the normal operator workflow; subsequent launchd-managed releases auto-update by default. Set
NEUROLINK_PROXY_AUTO_UPDATE=offbefore that restart to opt out. Updater diagnostics are written to~/.neurolink/logs/proxy-updater.log.The live proxy was not restarted, upgraded, reconfigured, or used for test traffic while developing this change. No dependencies were added.
Summary by CodeRabbit