Skip to content

fix(proxy): eliminate fabricated 429 storm, harden launchd service lifecycle - #951

Merged
murdore merged 1 commit into
releasefrom
fix/proxy-bug-fixes
Apr 15, 2026
Merged

murdore merged 1 commit into
releasefrom
fix/proxy-bug-fixes

Conversation

@murdore

@murdore murdore commented Apr 14, 2026 •

Copy link
Copy Markdown
Contributor

Summary

  • Eliminate fabricated 429 storm: Remove per-account exponential cooldown/backoff that amplified real Anthropic 429s into a local fail-closed storm. Replace with inline same-account retries (up to 5) using upstream Retry-After header, capped at 30s per wait. Every request now attempts all accounts before returning a synthesized 429 to the client.
  • Harden launchd service lifecycle: Replace process.argv[1]-pinned plist entrypoint with a trampoline script that probes PATH candidates and validates each with --version. Auto-updater now rewrites plist before restart and aborts on version mismatch (prevents pnpm store-mismatch from restarting into the wrong version).
  • Improve update observability: Guard stdout/stderr logged to proxy-guard.log (was stdio: "ignore"). pnpm resolved via multi-candidate probing with full diagnostic logging. suppressVersion includes error details and stderr.

Changes

Proxy routing (claudeProxyRoutes.ts, routingPolicy.ts, proxyTypes.ts, usageStats.ts)

  • Remove applyRateLimitCooldown, partitionAccountsByCooldown, getAccountCooldownUntil, clearAccountCooldown
  • Remove AUTH_COOLDOWN_MS (5min), RATE_LIMIT_BACKOFF_CAP_MS (10min)
  • Remove coolingUntil/backoffLevel from RuntimeAccountState, CooldownSkippedAccount type, recordCooldown
  • Add parseRetryAfterMs helper, MAX_RATE_LIMIT_SAME_ACCOUNT_RETRIES (5), MAX_RATE_LIMIT_RETRY_DELAY_MS (30s cap)
  • On 429: return retrySameAccount + retryAfterMs → loop waits min(retryAfterMs, 30s), retries same account, then rotates

Launchd / auto-update (proxy.ts)

  • Trampoline at ~/.neurolink/bin/neurolink-proxy probes PATH candidates with --version, falls back to baked-in node+script
  • proxy install validates trampoline before writing plist (aborts on failure)
  • Auto-updater rewrites trampoline + plist before launchctl kickstart
  • Post-install version-mismatch → abort + suppress with full diagnostic (prevents store-mismatch restart)
  • resolveFullPnpmPath probes NEUROLINK_PNPM_PATH, PNPM_HOME, which, common paths — logs all candidates
  • Guard output persisted to proxy-guard.log
  • Fix bare require() → _require (ESM safety via createRequire)

Regression coverage (48 tests in continuous-test-suite-bugfixes.ts)

  • parseRetryAfterMs edge cases, no cooldown exports/types/fields remain
  • Simulated 3-account burst proves all accounts attempted, zero-upstream-attempt impossible
  • Retry-after cap verified structurally, no stale cooldown=5min log messages
  • Trampoline probing, install validation, sh -n syntax checks
  • pnpm candidate logging, environmental skip, version-mismatch abort

Test plan

  • npx tsc --noEmit --strict — 0 errors
  • npx tsx test/continuous-test-suite-bugfixes.ts — 48/48 pass
  • pnpm run build — 0 errors, 0 warnings
  • Pre-commit hooks pass (check, format, lint, build)
  • Manual: neurolink proxy install on macOS with a global pnpm install
  • Manual: Trigger auto-update cycle and verify trampoline resolves correctly
  • Manual: Simulate burst 429 traffic and confirm no fabricated local 429s with zero upstream attempts

Summary by CodeRabbit

  • Bug Fixes

    • Improved proxy stability with enhanced binary resolution and startup validation
    • Replaced persistent rate-limit cooldowns with per-account retries that respect server Retry-After headers for smarter throttling
    • Enhanced proxy guard logging with detailed diagnostics written to ~/.neurolink/logs/proxy-guard.log
  • Changes

    • Removed backoff level and cooling duration from proxy status output

Copilot AI review requested due to automatic review settings April 14, 2026 19:16
@vercel

vercel Bot commented Apr 14, 2026 •

Copy link
Copy Markdown

The latest updates on your projects. Learn more about Vercel for GitHub.

Project Deployment Actions Updated (UTC)
neurolink Ready Ready Preview, Comment Apr 14, 2026 7:56pm

@github-actions

github-actions Bot commented Apr 14, 2026 •

Copy link
Copy Markdown
Contributor

✅ Single Commit Policy - COMPLIANT

Status: Policy requirements met • 1 commit • Valid format • Ready for merge

📊 View validation details

📝 Commit Details

  • Hash: c351a4564df46c2da3d5713d602aeb561f957876
  • Message: fix(proxy): eliminate fabricated 429 storm, harden launchd service lifecycle
  • Author: Sachin Sharma

✅ Validation Results

  • Single commit requirement met
  • No merge commits in branch
  • Semantic commit message format verified
  • Ready for squash merge to release branch

🤖 Automated validation by NeuroLink Single Commit Enforcement

@coderabbitai

coderabbitai Bot commented Apr 14, 2026 •

Copy link
Copy Markdown

Warning

Rate limit exceeded

@murdore has exceeded the limit for the number of commits that can be reviewed per hour. Please wait 36 minutes and 16 seconds before requesting another review.

Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 36 minutes and 16 seconds.

⌛ How to resolve this issue?

After the wait time has elapsed, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

We recommend that you space out your commits to avoid hitting the rate limit.

🚦 How do rate limits work?

CodeRabbit enforces hourly rate limits for each developer per organization.

Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout.

Please see our FAQ for further information.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 16766a59-8424-4739-ab67-3775adfb98cd

📥 Commits

Reviewing files that changed from the base of the PR and between 65e864b and c351a45.

📒 Files selected for processing (6)
  • src/cli/commands/proxy.ts
  • src/lib/proxy/routingPolicy.ts
  • src/lib/proxy/usageStats.ts
  • src/lib/server/routes/claudeProxyRoutes.ts
  • src/lib/types/proxy.ts
  • test/continuous-test-suite-bugfixes.ts

Walkthrough

This PR refactors the account rate-limiting system from persistent cooldown tracking to inline per-account retries, and introduces a launchd trampoline wrapper for stable binary resolution at daemon startup. It removes cooldown-related state tracking (backoff levels, cooling timestamps) and replaces account partitioning with per-account retry budgets capped at 5 retries per 429 response.

Changes

Cohort / File(s) Summary
Launchd Trampoline & Guard Stability
src/cli/commands/proxy.ts
Introduced launchd-stable trampoline wrapper and helper functions (writeTrampoline, probeBinVersion, resolveFullPnpmPath) to dynamically resolve neurolink binary at launch. Updated buildPlist() to invoke trampoline script instead of Node entrypoint. Enhanced autoupdater/guard logic with pnpm binary validation, detailed install diagnostics, trampoline probing after updates, and guard stdout/stderr logging to file instead of ignoring. Added parseExistingPlistArgs() to preserve --env-file / --config flags during plist rewrites. Removed backoffLevel and coolingUntil from /status stats.
Rate-Limit Retry Strategy
src/lib/server/routes/claudeProxyRoutes.ts
Removed all persistent cooldown gating and account partitioning logic. Changed 429 handling to inline per-account retries: fetchAnthropicAccountResponse now parses upstream retry-after header and returns retryAfterMs; handleAnthropicRoutedClaudeRequest implements up to 5 retries on same account before rotating. Replaced cooldown-based advance semantics with exhaustion-based primary account rotation. Updated failure response to emit retry-after: 1 only after all accounts rate-limit and per-account budgets are exhausted.
Cooldown Utilities Removal
src/lib/proxy/routingPolicy.ts
Removed exported cooldown/rate-limit utilities: CooldownSkippedAccount, RuntimeAccountState, getAccountCooldownUntil, partitionAccountsByCooldown, applyRateLimitCooldown, clearAccountCooldown, and DEFAULT_COOLDOWN_FLOOR_MS constant. Added new parseRetryAfterMs() helper to interpret upstream Retry-After header and convert to milliseconds.
Account Statistics Cleanup
src/lib/proxy/usageStats.ts
Removed recordCooldown() exported function. Stopped resetting currentBackoffLevel on success. Removed currentBackoffLevel: 0 field initialization from new AccountStats entries.
Type System Updates
src/lib/types/proxy.ts
Extended AnthropicUpstreamFetchResult with optional retryAfterMs?: number field. Removed currentBackoffLevel and coolingUntil from AccountStats. Removed coolingUntil and backoffLevel from RuntimeAccountState. Deleted CooldownSkippedAccount<T> type.
Test Coverage Expansion
test/continuous-test-suite-bugfixes.ts
Removed cooldown-related unit tests; added parseRetryAfterMs assertions covering null, numeric strings, HTTP-date format, clamping, and invalid input. Updated makeRuntimeState helper by removing cooldown/backoff fields. Added regression tests validating: removed exports for cooldown functions, launchd trampoline invocation patterns, pnpm resolution logic, 429 retry message text, shell script syntax validation, and buildProxyTranslationPlan contract enforcement.

Sequence Diagram

sequenceDiagram
    actor Client
    participant Router as Request Router
    participant AcctMgr as Account Manager
    participant Anthropic as Anthropic API
    participant Backoff as Inline Retry Logic

    Client->>Router: Request (claude)
    Router->>AcctMgr: Get eligible accounts
    AcctMgr-->>Router: [Account A, Account B, Account C, ...]
    
    Router->>Backoff: Retry budget = 5
    
    loop Retry up to 5 times on same account
        Backoff->>Anthropic: Forward request to Account A
        alt 429 Rate Limited
            Anthropic-->>Backoff: 429 + Retry-After: 30s
            Backoff->>Backoff: Sleep min(30s, 30s cap)
            Backoff->>Backoff: Decrement retry budget
        else Success
            Anthropic-->>Client: 200 + Response
            break
            end
        end
    end
    
    alt Retry budget exhausted (all 5 retries failed)
        Backoff->>Router: Advance to next account
        Router->>AcctMgr: Try Account B (same retry budget reset)
        Note over Router,AcctMgr: Repeat per-account retry flow
    else All accounts exhausted
        Router-->>Client: 429 + Retry-After: 1
    end
Loading

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~75 minutes

Possibly related PRs

Suggested labels

released

Poem

🐰 Bouncing past the cooldowns cold,
Retries now inline and bold,
Trampoline springs the daemon high,
No backoff levels—just retry!
Fresh state tracking, lean and swift,
Rate limits lift with every shift. 🚀

🚥 Pre-merge checks | ✅ 2 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 33.33% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (2 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly and specifically summarizes the two main changes: removing the fabricated cooldown mechanism and hardening the launchd service lifecycle, matching the PR's core objectives and code changes.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch fix/proxy-bug-fixes

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@github-actions

Copy link
Copy Markdown
Contributor

🤖 AI Review & Build Compliance ✅

Status: AI analysis complete • Build rules validated • Ready for review

📊 View detailed analysis results

🛡️ Analysis Complete

  • ✅ Security scan (vulnerabilities, API keys)
  • ✅ TypeScript safety & code quality
  • ✅ Error handling & best practices
  • ✅ Build rule enforcement validated
  • ✅ Commit format & compliance checks

📋 Ready for Merge When

  • All CI checks passing
  • Manual review approved
  • Any AI-flagged issues resolved

🤖 AI analysis complete - check individual code comments for specific feedback

Comment thread test/continuous-test-suite-bugfixes.ts Dismissed

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR updates the Claude proxy’s upstream rate-limit handling and the macOS launchd/auto-update lifecycle to reduce locally amplified 429 storms and make service restarts resilient to pnpm/global-store path churn.

Changes:

  • Replaces per-account cooldown/backoff with inline same-account 429 retries driven by upstream Retry-After, then rotates accounts.
  • Adds a launchd “trampoline” entrypoint and hardens the auto-updater (plist rewrite before kickstart, pnpm resolution probing, guard logging).
  • Updates types and usage stats to remove cooldown/backoff fields, and extends the bugfix test suite with regression checks.

Reviewed changes

Copilot reviewed 6 out of 6 changed files in this pull request and generated 5 comments.

Show a summary per file
File Description
src/lib/server/routes/claudeProxyRoutes.ts Removes cooldown partitioning and implements inline 429 retry + rotation logic.
src/lib/proxy/routingPolicy.ts Removes cooldown helpers and adds parseRetryAfterMs.
src/lib/proxy/usageStats.ts Removes cooldown/backoff stats tracking.
src/lib/types/proxy.ts Removes cooldown/backoff fields from public types and adds retryAfterMs to fetch result type.
src/cli/commands/proxy.ts Adds trampoline script support, defensive pnpm resolution, plist rewriting, and guard log file output.
test/continuous-test-suite-bugfixes.ts Adds regression/contract tests for the new retry logic and launchd/updater behavior.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment on lines +114 to +118
/** Minimum retries per account on 429 before rotating to the next account. */
const MAX_RATE_LIMIT_SAME_ACCOUNT_RETRIES = 5;
/** Max time to sleep between 429 retries. Caps large upstream retry-after values
* so we don't hold the client connection open for minutes. */
const MAX_RATE_LIMIT_RETRY_DELAY_MS = 30_000;

Copilot AI Apr 14, 2026

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

MAX_RATE_LIMIT_SAME_ACCOUNT_RETRIES is documented as a “Minimum retries per account” but it’s used as a maximum (you rotate once the counter reaches it). This is confusing and also affects the synthesized 429 message (“…after X retries each”). Please update the comment (and/or constant name) so it matches the actual behavior and semantics (retries vs total attempts).

Copilot uses AI. Check for mistakes.
Comment on lines +4351 to +4360
if (
fetchResult.retrySameAccount &&
fetchResult.retryAfterMs !== undefined &&
rateLimitSameAccountRetries < MAX_RATE_LIMIT_SAME_ACCOUNT_RETRIES
) {
rateLimitSameAccountRetries += 1;
const delayMs = Math.min(
fetchResult.retryAfterMs || 1_000,
MAX_RATE_LIMIT_RETRY_DELAY_MS,
);

Copilot AI Apr 14, 2026

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As written, the 429 retry loop performs up to MAX_RATE_LIMIT_SAME_ACCOUNT_RETRIES retries after the first 429 (i.e., up to MAX_RATE_LIMIT_SAME_ACCOUNT_RETRIES + 1 upstream attempts per account). If the intent is “max attempts per account”, adjust the counter/condition accordingly; otherwise consider clarifying the log/error wording to explicitly mean retries-after-first-attempt.

Copilot uses AI. Check for mistakes.
Comment thread src/cli/commands/proxy.ts
Comment on lines +2459 to +2470
const xml = readFileSync(PLIST_PATH, "utf-8");
// Extract --env-file value: <string>--env-file</string>\n <string>VALUE</string>
const envMatch = xml.match(
/<string>--env-file<\/string>\s*<string>([^<]+)<\/string>/,
);
const configMatch = xml.match(
/<string>--config<\/string>\s*<string>([^<]+)<\/string>/,
);
return {
envFile: envMatch?.[1],
configFile: configMatch?.[1],
};

Copilot AI Apr 14, 2026

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

parseExistingPlistArgs() extracts values directly from XML but does not unescape XML entities. Since buildPlist() escapes these values again, any path containing &, <, > etc. will be double-escaped when the updater rewrites the plist (e.g., &amp; -> &amp;amp;), breaking --env-file / --config paths. Consider unescaping the matched values before returning them, or store these args in a non-XML state file instead of reparsing the plist.

Copilot uses AI. Check for mistakes.
Comment thread src/cli/commands/proxy.ts Outdated
Comment on lines +503 to +511
const guardLogPath = join(guardLogDir, "proxy-guard.log");
const logFd = openSync(guardLogPath, "a");

const child = spawn(process.execPath, args, {
detached: true,
stdio: "ignore",
stdio: ["ignore", logFd, logFd],
});
child.unref();
closeSync(logFd); // parent closes its copy; child keeps the fd

Copilot AI Apr 14, 2026

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

spawnFailOpenGuard() opens proxy-guard.log and closes it only after a successful spawn(). If spawn() throws, the catch block returns without closing logFd, leaking a file descriptor. Wrap the spawn in a try/finally (or explicitly closeSync(logFd) in the error path) after openSync() succeeds.

Copilot uses AI. Check for mistakes.
Comment on lines +513 to +548
name: "429 regression: simulated 3-account burst proves all accounts attempted",
category: "429-regression",
fn: async () => {
// Simulate the behavioral contract of the new retry loop:
// - 3 accounts, each gets up to 5 retries
// - Every retry is an upstream attempt (no local skip)
// - Total upstream attempts = 3 accounts × 5 retries = 15
const MAX_RETRIES = 5;
const accounts = ["acct-A", "acct-B", "acct-C"];
const upstreamAttempts: string[] = [];
let sawRateLimit = false;

for (const account of accounts) {
let retries = 0;
while (retries < MAX_RETRIES) {
retries++;
// Every iteration is a real upstream attempt
upstreamAttempts.push(account);
// Simulate upstream 429 with retry-after
sawRateLimit = true;
// In the real code, sleep(retryAfterMs) happens here
// then continue to retry same account
}
// After 5 retries exhausted, rotate to next account
}

return (
upstreamAttempts.length === 15 &&
upstreamAttempts.filter((a) => a === "acct-A").length === 5 &&
upstreamAttempts.filter((a) => a === "acct-B").length === 5 &&
upstreamAttempts.filter((a) => a === "acct-C").length === 5 &&
sawRateLimit === true
);
},
},
{

Copilot AI Apr 14, 2026

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This “simulated 3-account burst” test doesn’t exercise the production retry/rotation code; it will pass even if the real routing loop regresses (it’s only testing the simulation). To make this regression meaningful, consider extracting the 429 retry/rotation logic into a testable helper or adding an integration-style test that drives handleAnthropicRoutedClaudeRequest (or a smaller exported unit) with a stubbed upstream returning 429s.

Suggested change
name: "429 regression: simulated 3-account burst proves all accounts attempted",
category: "429-regression",
fn: async () => {
// Simulate the behavioral contract of the new retry loop:
// - 3 accounts, each gets up to 5 retries
// - Every retry is an upstream attempt (no local skip)
// - Total upstream attempts = 3 accounts × 5 retries = 15
const MAX_RETRIES = 5;
const accounts = ["acct-A", "acct-B", "acct-C"];
const upstreamAttempts: string[] = [];
let sawRateLimit = false;
for (const account of accounts) {
let retries = 0;
while (retries < MAX_RETRIES) {
retries++;
// Every iteration is a real upstream attempt
upstreamAttempts.push(account);
// Simulate upstream 429 with retry-after
sawRateLimit = true;
// In the real code, sleep(retryAfterMs) happens here
// then continue to retry same account
}
// After 5 retries exhausted, rotate to next account
}
return (
upstreamAttempts.length === 15 &&
upstreamAttempts.filter((a) => a === "acct-A").length === 5 &&
upstreamAttempts.filter((a) => a === "acct-B").length === 5 &&
upstreamAttempts.filter((a) => a === "acct-C").length === 5 &&
sawRateLimit === true
);
},
},
{

Copilot uses AI. Check for mistakes.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 6

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (2)
src/lib/server/routes/claudeProxyRoutes.ts (1)

4186-4206: ⚠️ Potential issue | 🟠 Major

401→429 flows still bypass the new Retry-After loop.

This new 429 path correctly surfaces retryAfterMs to the outer loop, but a post-refresh 429 in handleAnthropicAuthRetry() still rotates immediately instead of reusing it. Refreshed OAuth accounts therefore skip the 30s cap and the 5 same-account retries introduced here.

Also applies to: 4350-4380

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@src/lib/server/routes/claudeProxyRoutes.ts` around lines 4186 - 4206, The 429
branch currently returns retryAfterMs but post-refresh 429s from
handleAnthropicAuthRetry() still trigger an immediate rotate; update the
auth-retry flow so a 401→429 path preserves the same-account retry semantics:
change handleAnthropicAuthRetry (and its callers in claudeProxyRoutes.ts) to
return the unified result object that includes retryAfterMs and
retrySameAccount, and ensure the caller checks that return value and uses
retrySameAccount=true (and the passed retryAfterMs) instead of rotating the
account; make sure the tracer/logging (e.g., tracer?.recordRetry, logAttempt)
uses that returned info so the 30s cap and same-account retry count are honored.
src/cli/commands/proxy.ts (1)

973-981: ⚠️ Potential issue | 🟡 Minor

Keep the /status payload aligned with the text renderer.

This drops cooling from the live stats shape, but printStatusStats() still reads that field and treats undefined as "active". Text mode will now silently report every account as active unless you either remove that column there or add a replacement state field here.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@src/cli/commands/proxy.ts` around lines 973 - 981, The /status payload
mapping in the accounts array currently omits the cooling field required by
printStatusStats(); update the mapping (the accounts:
Object.values(stats.accounts).map(...) block) to include the cooling state (or a
replacement state field) for each account—e.g., add cooling: account.cooling (or
state: account.cooling ? 'cooling' : 'active')—so printStatusStats() reads the
expected property instead of getting undefined.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@src/cli/commands/proxy.ts`:
- Around line 2108-2118: The catch block that handles restartErr currently
returns without clearing the updateRestartInProgress flag, leaving it true and
preventing the health loop from treating parent disappearance as terminal;
modify the restartErr handler (the catch around launchctl kickstart in proxy.ts)
to reset updateRestartInProgress (e.g., set updateRestartInProgress = false or
call the existing clear-reset helper if present) before calling suppressVersion
and returning so the flag is cleared on the restart error path.
- Around line 2459-2470: The plist parser currently returns XML-escaped strings
for envFile and configFile (extracted in the block that reads PLIST_PATH), which
causes double-escaping when buildPlist() later re-escapes them; update the
extraction logic (the regex match that sets envMatch and configMatch and the
returned envFile/configFile) to XML-decode/unescape standard entities (&amp;,
&lt;, &gt;, &quot;, &#39;, etc.) before returning so buildPlist() receives raw
paths; you can implement this by running the matched group values through a
small XML/entity decode utility or using an XML parser to extract string node
text, then return the decoded values for envFile and configFile.
- Around line 2629-2655: The trampoline check currently only verifies the
trampoline can run; update the logic in proxy.ts (around probeBinVersion and
TRAMPOLINE_PATH) to also compare trampolineVersion against the expected
neurolink version and treat mismatches as fatal: obtain the installer’s
target/expected version (e.g., the package’s version or the installer’s
TARGET_VERSION variable), and if probeBinVersion(TRAMPOLINE_PATH) !==
expectedVersion, log a clear error (include trampolineVersion and
expectedVersion) and exit (process.exit(1)) just like the current failure path
so proxy install fails when the trampoline resolves to the wrong neurolink
version.
- Around line 2020-2084: The plist rewrite is ineffective because after writing
PLIST_PATH with writeFileSync the code still uses launchctl kickstart -k which
restarts the in-memory job (ignoring the new plist); modify the restart logic
that runs after buildPlist/writeFileSync so it fully unloads and reloads the
LaunchAgent instead of kickstart: call launchctl bootout (or legacy unload) for
gui/$UID/$LABEL and then launchctl bootstrap (or legacy load) with PLIST_PATH
(handle errors/exit codes and permissions, and preserve existing env/config args
parsed by parseExistingPlistArgs); ensure any logging (logger.always) and
suppressVersion behavior remains correct if the bootout/bootstrap fails.

In `@src/lib/server/routes/claudeProxyRoutes.ts`:
- Around line 103-105: Auth-failure branches are not updating the fill-first
primary, so primaryAccountIndex remains pointing at a broken account for
subsequent requests; update each auth-failure exit path to call
advancePrimaryIfCurrent() (or call the account-disable helper you use) before
returning so the primaryAccountIndex advances permanently, specifically replace
the fall-through-only behavior in the auth-retry/auth-fail branches (the blocks
that currently only move to the next account for the current request) with a
call to advancePrimaryIfCurrent() (or the permanent-disable routine) so future
requests start with a healthy primary.

In `@test/continuous-test-suite-bugfixes.ts`:
- Around line 513-546: The test simulates only 5 total upstream attempts per
account but production does 1 initial call plus up to 5 retries (6
attempts/account), so update the simulation: change the loop semantics (or
MAX_RETRIES) in this test block so each account produces 6 upstreamAttempts (use
MAX_RETRIES = 6 or loop once for the initial attempt then 5 retries), keep using
the same variables (MAX_RETRIES, accounts, upstreamAttempts, sawRateLimit) and
update the final assertions to expect total upstreamAttempts.length === 18 and 6
attempts for "acct-A", "acct-B", and "acct-C" and sawRateLimit === true.

---

Outside diff comments:
In `@src/cli/commands/proxy.ts`:
- Around line 973-981: The /status payload mapping in the accounts array
currently omits the cooling field required by printStatusStats(); update the
mapping (the accounts: Object.values(stats.accounts).map(...) block) to include
the cooling state (or a replacement state field) for each account—e.g., add
cooling: account.cooling (or state: account.cooling ? 'cooling' : 'active')—so
printStatusStats() reads the expected property instead of getting undefined.

In `@src/lib/server/routes/claudeProxyRoutes.ts`:
- Around line 4186-4206: The 429 branch currently returns retryAfterMs but
post-refresh 429s from handleAnthropicAuthRetry() still trigger an immediate
rotate; update the auth-retry flow so a 401→429 path preserves the same-account
retry semantics: change handleAnthropicAuthRetry (and its callers in
claudeProxyRoutes.ts) to return the unified result object that includes
retryAfterMs and retrySameAccount, and ensure the caller checks that return
value and uses retrySameAccount=true (and the passed retryAfterMs) instead of
rotating the account; make sure the tracer/logging (e.g., tracer?.recordRetry,
logAttempt) uses that returned info so the 30s cap and same-account retry count
are honored.
🪄 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: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: ad1d64e5-eaf2-45f8-aff2-92285a7de3ae

📥 Commits

Reviewing files that changed from the base of the PR and between 79adf00 and 65e864b.

📒 Files selected for processing (6)
  • src/cli/commands/proxy.ts
  • src/lib/proxy/routingPolicy.ts
  • src/lib/proxy/usageStats.ts
  • src/lib/server/routes/claudeProxyRoutes.ts
  • src/lib/types/proxy.ts
  • test/continuous-test-suite-bugfixes.ts
💤 Files with no reviewable changes (1)
  • src/lib/proxy/usageStats.ts

Comment thread src/cli/commands/proxy.ts
Comment thread src/cli/commands/proxy.ts
Comment thread src/cli/commands/proxy.ts
Comment thread src/cli/commands/proxy.ts
Comment thread src/lib/server/routes/claudeProxyRoutes.ts
Comment thread test/continuous-test-suite-bugfixes.ts
@murdore

murdore commented Apr 14, 2026

Copy link
Copy Markdown
Contributor Author

Review Feedback Addressed (Cycle 1)

All actionable review comments from @copilot-pull-request-reviewer and @coderabbitai have been addressed.

Fixes applied

src/lib/server/routes/claudeProxyRoutes.ts

  • [Rebrand to neurolink #1] Clarified MAX_RATE_LIMIT_SAME_ACCOUNT_RETRIES JSDoc: "Maximum same-account retries on 429 (total attempts per account = this + 1)"
  • [feat: Complete Visual Ecosystem + Automated NPM Publishing v1.1.0 #2] Updated synthesized 429 message to say "attempts each (1 initial + N retries)" — removes off-by-one ambiguity
  • [build(deps): Bump esbuild and vitest #3] Added advancePrimaryIfCurrent() calls to all auth-failure exit paths: OAuth no-refresh-token, API-key auth failure, and auth-retry exhaustion. The fill-first primary now advances permanently after an auth failure, not just for the current request. Extended handleAnthropicNonOkResponse signature with enabledAccounts/orderedAccounts
  • Skipped [feat: implement comprehensive CLI tool with visual documentation and … #4]: 401→429 flows in handleAnthropicAuthRetry still rotate immediately. Unifying that return signature is a non-trivial refactor for a rare edge case (refreshed token immediately hits rate limit). Happy to do it in a follow-up if you'd like

src/cli/commands/proxy.ts

test/continuous-test-suite-bugfixes.ts

Validation

  • TypeScript (tsc --noEmit --strict): ✅ 0 errors
  • Regression tests (continuous-test-suite-bugfixes.ts): ✅ 48/48 pass
  • Build (pnpm run build): ✅ 0 errors, 0 warnings
  • Pre-commit hooks: ✅ passed (check, format, lint, build)

Requesting re-review

@copilot-pull-request-reviewer @coderabbitai — all feedback from this cycle has been addressed. Please re-review.

@murdore

murdore commented Apr 14, 2026

Copy link
Copy Markdown
Contributor Author

@coderabbitai review

@coderabbitai

coderabbitai Bot commented Apr 14, 2026

Copy link
Copy Markdown
✅ Actions performed

Review triggered.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

@github-actions

Copy link
Copy Markdown
Contributor

🤖 AI Review & Build Compliance ✅

Status: AI analysis complete • Build rules validated • Ready for review

📊 View detailed analysis results

🛡️ Analysis Complete

  • ✅ Security scan (vulnerabilities, API keys)
  • ✅ TypeScript safety & code quality
  • ✅ Error handling & best practices
  • ✅ Build rule enforcement validated
  • ✅ Commit format & compliance checks

📋 Ready for Merge When

  • All CI checks passing
  • Manual review approved
  • Any AI-flagged issues resolved

🤖 AI analysis complete - check individual code comments for specific feedback

@coderabbitai

coderabbitai Bot commented Apr 14, 2026

Copy link
Copy Markdown

Tip

For best results, initiate chat on the files or code changes.

@murdore — thank you for the detailed Cycle 1 summary. All 12 fixes look well-considered. A few highlights worth calling out:

  • [#5 CRITICAL] The bootout + bootstrap sequence is the correct fix — kickstart -k is notorious for silently ignoring on-disk plist changes, so this was essential for trampoline rewrites to actually take effect.
  • [#7 + #8] The unescapeXml() round-trip protection is a subtle but important correctness fix; double-encoding &amp; → &amp;amp; would have caused hard-to-diagnose plist parse failures.
  • [#9] The trampoline version guard on proxy install is a good defensive check — PATH shadowing by a stale binary is exactly the kind of silent failure that's painful to debug in production.
  • Skipped [#4] — the deferral is reasonable. The 401→429 path in handleAnthropicAuthRetry is an uncommon race (token refresh succeeds but immediately rate-limits), and unifying the return signature is a non-trivial refactor worth doing in isolation. Happy to track it as a follow-up issue if you'd like.

The validation results (48/48 tests, tsc --strict clean, pre-commit hooks passed) look solid. The incremental review of the new commits is already underway.

Copilot AI commented Apr 14, 2026

Copy link
Copy Markdown
Contributor

Review Feedback Addressed (Cycle 1)

All actionable review comments from @copilot-pull-request-reviewer and @coderabbitai have been addressed....

Fixed a Prettier formatting issue in src/cli/commands/proxy.ts that was causing the CI format check to fail (commit b2b2bda). The changes were purely whitespace/line-break adjustments with no semantic impact.

…fecycle

Remove the per-account exponential cooldown/backoff system that amplified
real Anthropic 429s into a local fail-closed storm. Replace with inline
same-account retries (up to 5) using the upstream Retry-After header,
capped at 30 seconds per wait. Every request now attempts all accounts
before returning a synthesized 429 to the client.

Proxy routing changes:
- Remove applyRateLimitCooldown, partitionAccountsByCooldown,
  getAccountCooldownUntil, clearAccountCooldown, DEFAULT_COOLDOWN_FLOOR_MS
- Remove AUTH_COOLDOWN_MS (5min) and RATE_LIMIT_BACKOFF_CAP_MS (10min)
- Remove coolingUntil/backoffLevel from RuntimeAccountState
- Remove CooldownSkippedAccount type, recordCooldown from usageStats
- Add parseRetryAfterMs helper for upstream header parsing
- Add MAX_RATE_LIMIT_SAME_ACCOUNT_RETRIES (5) and
  MAX_RATE_LIMIT_RETRY_DELAY_MS (30s cap)
- On 429: return retrySameAccount + retryAfterMs to the loop
- Main loop: wait min(retryAfterMs, 30s), retry same account, then rotate

Launchd / auto-update hardening:
- Replace process.argv[1] plist entrypoint with a trampoline script
  (~/.neurolink/bin/neurolink-proxy) that probes PATH candidates with
  --version and falls back to the baked-in node+script from install time
- proxy install validates the trampoline before writing the plist
- Auto-updater rewrites trampoline + plist before launchctl kickstart
- Post-install version-mismatch check aborts restart and suppresses with
  diagnostic to prevent store-mismatch pnpm from restarting wrong version
- resolveFullPnpmPath probes multiple candidates (NEUROLINK_PNPM_PATH,
  PNPM_HOME, which, common paths) and logs all with per-candidate status
- Guard stdout/stderr now written to proxy-guard.log (was stdio: "ignore")
- suppressVersion includes error detail and stderr excerpts
- Fix bare require() calls to use _require (createRequire) for ESM safety

Regression coverage (48 tests):
- parseRetryAfterMs edge cases
- No cooldown exports/types/fields remain
- Simulated 3-account burst proves all accounts attempted
- Zero-upstream-attempt local 429 impossible
- Retry-after cap verified structurally
- Trampoline candidate probing, install validation, sh -n syntax checks
- Pnpm candidate logging, environmental skip, version-mismatch abort
@github-actions

Copy link
Copy Markdown
Contributor

🤖 AI Review & Build Compliance ✅

Status: AI analysis complete • Build rules validated • Ready for review

📊 View detailed analysis results

🛡️ Analysis Complete

  • ✅ Security scan (vulnerabilities, API keys)
  • ✅ TypeScript safety & code quality
  • ✅ Error handling & best practices
  • ✅ Build rule enforcement validated
  • ✅ Commit format & compliance checks

📋 Ready for Merge When

  • All CI checks passing
  • Manual review approved
  • Any AI-flagged issues resolved

🤖 AI analysis complete - check individual code comments for specific feedback

@murdore

murdore commented Apr 14, 2026

Copy link
Copy Markdown
Contributor Author

@coderabbitai review

The previous commit was squashed with a small prettier-only fix from Copilot (no semantic changes). Please re-review the current HEAD to confirm all cycle 1 fixes are still intact and acceptable.

@coderabbitai

coderabbitai Bot commented Apr 14, 2026

Copy link
Copy Markdown

@murdore — understood, triggering a re-review of the current HEAD now. Since CodeRabbit performs incremental reviews, it will pick up the squashed commit and verify the cycle 1 fixes are still intact. The prettier-only formatting change should not affect any of the semantic correctness checks.

✅ Actions performed

Review triggered.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

@murdore
murdore merged commit 9ef0505 into release Apr 15, 2026
15 checks passed
@murdore
murdore deleted the fix/proxy-bug-fixes branch April 15, 2026 03:40
@github-actions

Copy link
Copy Markdown
Contributor

🎉 This PR is included in version 9.54.5 🎉

The release is available on:

Your semantic-release bot 📦🚀

This branch was successfully deployed

1 active deployment
Preview — c351a456 Deployed Apr 14, 2026 by vercel[bot]
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants