Repository navigation
Add team-vault credential leases with local egress - #99
Merged
Merged
Conversation
A wedged Subrouter host takes every client with it. When cmux-mac-mini exhausted its kernel mbuf pool, it stopped accepting TCP entirely while still answering ICMP, so `sr codex` handed codex a base URL that silently hung instead of failing. Every machine on the team was blocked even though each had a healthy daemon on 127.0.0.1:31415 the whole time. Probe the configured server's /_subrouter/health before launching codex and substitute the local daemon when it does not answer. The probe is bounded at 1.5s because a buffer-exhausted host drops SYNs rather than refusing them, so an unbounded dial would hang rather than fail over. Resolution stays pure: defaultCodexBaseURLFor does no network I/O, and the probe runs only on the launch path in codexBaseURLWithFallback. An explicit SUBROUTER_CODEX_BASE_URL or SUBROUTER_CODEX_SERVER is a deliberate pin and is never overridden; SUBROUTER_DISABLE_FALLBACK opts out entirely. Also add sr up/down/restart/health, wrapping the launchd and systemd units that install-daemon and install-systemd already write, so restarting the server does not mean remembering launchctl bootout incantations. These are local-only commands and are never forwarded to a remote server.
|
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:
📝 WalkthroughWalkthroughAdds cloud credential leasing and refresh-repair handling, broker-backed proxy authorization, local daemon installation and lifecycle commands, health-based fallback routing, and cloud-aware Codex/Claude CLI behavior. ChangesCloud credential leasing and proxy integration
Estimated code review effort: 5 (Critical) | ~120 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 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}
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 `@cmd/subrouter/lifecycle.go`:
- Around line 164-177: Update defaultCodexBaseURLForHealth to resolve
SUBROUTER_CODEX_BASE_URL and SUBROUTER_CODEX_SERVER overrides before loading the
persisted default server, returning the explicitly selected endpoint when either
is set. Preserve the existing stored-default fallback when no override is
configured, and do not apply failover during health reporting.
In `@cmd/subrouter/serverfallback.go`:
- Around line 118-120: Remove the configured URL values from the warning emitted
by the fallback logic around the warn check. Keep the message generic, or pass
both baseURL and local through the project’s established redaction helper before
formatting them, ensuring credentials and query parameters cannot reach
stderr/log capture.
- Around line 46-52: Update sameEndpoint to compare parsed schemes as well as
hosts, using a case-insensitive comparison for both components. Return false
when either scheme or host differs, so HTTP and HTTPS endpoints remain distinct
while preserving the existing invalid-URL behavior.
🪄 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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 7a31ad9e-7ea2-4372-8518-e412cb01f71b
📒 Files selected for processing (5)
cmd/subrouter/codex.gocmd/subrouter/lifecycle.gocmd/subrouter/serverfallback.gocmd/subrouter/serverfallback_test.gocmd/subrouter/sr.go
The CLI had no answer to "is this thing working?" During today's outage the only way to tell a wedged server from a routing problem was curl against /_subrouter/health by hand. sr doctor reports the checks in the order a failure bites: daemon installed, daemon healthy, configured server healthy, accounts present. It distinguishes a server that is down but covered by local fallback (warn) from one that is down with no fallback (fail), and exits non-zero only on the latter so it can gate scripts. sr setup is idempotent onboarding: install the daemon, start it, wait for health, then print the one useful next step. sr cleanup prints its plan and does nothing without --yes; credentials survive unless --purge, because losing a refresh-token chain means re-running OAuth for every account. Lifecycle moves under sr server up/down/restart, matching the noun the verbs act on, and sr server status with no name now reports this machine rather than erroring. Both controllers gained remove(), and cleanup and doctor take an injected controller so tests do not depend on the host's launchd state. directSRCommands gains the three new verbs so they work when the binary is invoked as subrouter or cx, not only as sr, with main_test.go's guard list updated to match.
lawrencecchen
changed the title
There was a problem hiding this comment.
🧹 Nitpick comments (2)
cmd/subrouter/sr_server.go (1)
109-124: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd test coverage for the new lifecycle dispatch.
up/start,down/stop,restart, and the arg-count-basedstatusbranching are new dispatch logic, but no test changes forsr_server.goare included in this cohort. Consider adding cases exercising each alias and bothstatus(no name → local health) andstatus <name>(remote) paths.🤖 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 `@cmd/subrouter/sr_server.go` around lines 109 - 124, Add table-driven tests for the lifecycle dispatch in the server subrouter, covering both up/start and down/stop aliases, restart, and status with no name versus a named server. Assert that no-name status invokes local health and status with a name invokes the remote server-status path, while preserving the existing argument validation behavior.cmd/subrouter/sr_setup_test.go (1)
100-134: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd doctor coverage for the "configured server" reachable/fallback/fail branches.
Existing tests only exercise the
configured == ""(local) path inrunDoctorWith's configured-server check. The reachable-remote, fallback-warn, and no-fallback-fail branches (sr_setup.go lines 203-209) are untested, even though doctor's exit code is used to gate scripts.🤖 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 `@cmd/subrouter/sr_setup_test.go` around lines 100 - 134, Add tests for runDoctorWith covering a configured server that is reachable, unreachable with a usable local fallback, and unreachable without a fallback. Configure the server URL and health responses to assert successful continuation for the reachable case, a warning for fallback, and an error/failure result when no fallback exists, including the expected output and doctor status.
🤖 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.
Nitpick comments:
In `@cmd/subrouter/sr_server.go`:
- Around line 109-124: Add table-driven tests for the lifecycle dispatch in the
server subrouter, covering both up/start and down/stop aliases, restart, and
status with no name versus a named server. Assert that no-name status invokes
local health and status with a name invokes the remote server-status path, while
preserving the existing argument validation behavior.
In `@cmd/subrouter/sr_setup_test.go`:
- Around line 100-134: Add tests for runDoctorWith covering a configured server
that is reachable, unreachable with a usable local fallback, and unreachable
without a fallback. Configure the server URL and health responses to assert
successful continuation for the reachable case, a warning for fallback, and an
error/failure result when no fallback exists, including the expected output and
doctor status.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 0eab0ea2-8174-493f-ad26-f5f0b754428f
📒 Files selected for processing (7)
cmd/subrouter/lifecycle.gocmd/subrouter/main.gocmd/subrouter/main_test.gocmd/subrouter/sr.gocmd/subrouter/sr_server.gocmd/subrouter/sr_setup.gocmd/subrouter/sr_setup_test.go
🚧 Files skipped from review as they are similar to previous changes (1)
- cmd/subrouter/lifecycle.go
A cold machine had to run `sr server up` before `sr codex` would work, which is a step nobody remembers and the error it produced (a hung dial) did not explain. Launching an agent now starts the daemon when nothing is serving. Ordering matters and is deliberate: a healthy configured server always wins, so autostart only runs when nothing already answers. That keeps the common case free of extra probes and avoids starting a local daemon that would go unused. Autostart also runs when the configured target *is* this machine's daemon, including under SUBROUTER_DISABLE_FALLBACK, because starting it repairs the configured target rather than substituting a different one. Pinning via SUBROUTER_CODEX_BASE_URL still never redirects traffic elsewhere. For Claude, only the launching forms trigger it; `sr claude list` and the other management subcommands stay quiet. Also fixes help drift found while dogfooding: `sr help` renders usageText in main.go, not the srHelp const, so the new commands were missing from the help users actually see. Both surfaces now list them and a test asserts it.
lawrencecchen
changed the title
There was a problem hiding this comment.
Actionable comments posted: 11
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
cloudflare/packages/worker/src/contract.ts (1)
690-713: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winPreserve terminal refresh signals from standard OAuth error fields.
Dropping the response body means object-form OAuth refresh errors can become non-terminal when the signal lives in
errororerror_descriptionrather thanproviderCode/providerType. For example,{"error":{"message":"invalid_grant: refresh token revoked"}}currently returnsprovider rejected the refreshwith noproviderCode, soterminalRefreshFailurecan fail to block refresh indefinitely and replay a rotated refresh token. Extracterror/error_descriptionas OAuth standard fields and match them against a known-code allowlist instead of persisting the raw body.🤖 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 `@cloudflare/packages/worker/src/contract.ts` around lines 690 - 713, Update refreshError to extract OAuth standard error and error_description fields from parsed provider responses, including nested object messages, and match those values against the existing known terminal-error allowlist used by terminalRefreshFailure. Populate the appropriate providerCode or equivalent terminal signal without persisting the raw response body, while preserving the current status-based error message and sanitized error properties.cmd/subrouter/codex.go (1)
20-28: 🔒 Security & Privacy | 🔴 Critical | ⚡ Quick winGate
localProxyTokenon the resolvedbaseURL.
codexBaseURLWithFallbackreturns a pinnedbaseURLunchanged, whilecloudLocalProxyToken(cloudConfig)returns the stored access token whenever cloud config is team-ready. If a user has team/local credential storage configured and setsSUBROUTER_CODEX_BASE_URLorSUBROUTER_CODEX_SERVERto a non-local endpoint,codexstill writes that token intoSUBROUTER_CODEX_DUMMY_API_KEYand sends it as the Subrouter bearer credential to the pinned endpoint. DerivelocalProxyTokenonly whenbaseURLmatches the local daemon endpoint (or not attach the token for unpinned remote fallbacks).🤖 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 `@cmd/subrouter/codex.go` around lines 20 - 28, Update the localProxyToken derivation in the codex setup flow to depend on the resolved baseURL: only call cloudLocalProxyToken when baseURL matches the local daemon endpoint, and otherwise leave the token unset so it is not attached to pinned or remote endpoints. Preserve the existing cloud configuration error handling and baseURL resolution.
🧹 Nitpick comments (5)
cmd/subrouter/sr_cloud.go (2)
683-717: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winSubstring fallback matching can select the wrong credential.
selectLocalAccountUploadfalls back tostrings.Containsmatching;--only worksilently resolves towork-backupwhen only one partial match exists. For an operation that uploads credentials to a shared vault, prefer requiring an exactlabelorkind:labelmatch.🤖 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 `@cmd/subrouter/sr_cloud.go` around lines 683 - 717, Update selectLocalAccountUpload to remove substring fallback matching via strings.Contains and consider only exact label or kind:label matches. Preserve the existing empty-selector, not-found, and ambiguous-match errors, ensuring selectors like “work” do not resolve to labels such as “work-backup”.
73-77: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winA single transient poll error aborts the whole login.
Any network blip or 5xx from
/auth/pollfails the command and forces the user to restart device approval. Tolerate transient errors until the deadline.♻️ Suggested change
case <-ticker.C: poll, err = client.PollAuth(ctx, start.DeviceCode) if err != nil { - return fmt.Errorf("poll cmux.com login: %w", err) + // Transient broker/network errors must not cancel an + // approval the user may already be completing. + continue }🤖 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 `@cmd/subrouter/sr_cloud.go` around lines 73 - 77, Update the PollAuth handling in the ticker.C branch of the device-login polling loop so transient network or 5xx errors are tolerated and polling continues until the existing deadline. Do not return immediately from this branch; retain the last error as needed and only return a failure when the context/deadline expires, while preserving successful poll handling.cmd/subrouter/main.go (1)
253-265: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueCollapse the two team-mode preconditions.
The second check (
Team && !Ready()) already subsumes the first (Team && LoggedIn && TeamID == ""); only the message differs. Keeping both invites drift.♻️ Suggested simplification
- if cloudConfig.EffectiveCredentialSource() == broker.CredentialSourceTeam && - cloudConfig.LoggedIn() && - cloudConfig.TeamID == "" { - return errors.New("cmux.com login has no selected team; run 'sr team use <team>'") - } - if cloudConfig.EffectiveCredentialSource() == broker.CredentialSourceTeam && - !cloudConfig.Ready() { - return errors.New("team credential storage requires login and a selected team; run 'sr login'") - } + if cloudConfig.EffectiveCredentialSource() == broker.CredentialSourceTeam && !cloudConfig.Ready() { + if cloudConfig.LoggedIn() { + return errors.New("cmux.com login has no selected team; run 'sr team use <team>'") + } + return errors.New("team credential storage requires login and a selected team; run 'sr login'") + }🤖 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 `@cmd/subrouter/main.go` around lines 253 - 265, The two team-mode validation branches in the config-loading flow should be collapsed into a single `CredentialSourceTeam && !cloudConfig.Ready()` check. Remove the redundant `LoggedIn()`/`TeamID` condition and preserve the more specific selected-team error message only if the existing readiness error cannot provide the required behavior; otherwise retain one consistent error message.cmd/subrouter/sr_claude.go (1)
99-111: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRemove the unused
cloudClaudemethod.
cloudClaudeis defined but no code calls it; thesr claudelaunch path already inlines the team-mode readiness check beforeproxyClaude. Keeping it only adds unreachable code.🤖 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 `@cmd/subrouter/sr_claude.go` around lines 99 - 111, Remove the unused srRunner.cloudClaude method, including its associated comment, while preserving the existing sr claude launch path and its inlined team-mode readiness check before proxyClaude.cmd/subrouter/install_systemd_user.go (1)
50-61: 🩺 Stability & Availability | 🔵 TrivialConsider enabling linger so the daemon survives logout.
systemctl --user enable --nowstarts the unit, but on most distros asystemd --usermanager (and its services) is stopped when the user's last session ends unlessloginctl enable-linger <user>has been run. Since this daemon is meant to run continuously as a background credential proxy, installation should check/enable lingering (or clearly warn the user it's required) so the loopback proxy doesn't silently disappear after logout/SSH disconnect.#!/bin/bash # Confirm whether linger enablement is handled elsewhere in the daemon lifecycle code. rg -n -i 'linger' --type=go🤖 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 `@cmd/subrouter/install_systemd_user.go` around lines 50 - 61, Update the installation flow surrounding the systemd user-unit setup to ensure user lingering is enabled before or alongside the existing daemon-reload and enable --now commands. Use the existing command runner and current-user context to invoke loginctl enable-linger for the installing user, or provide an explicit warning if the project handles this elsewhere; avoid duplicating the change if an existing linger lifecycle helper is available.
🤖 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 `@cloudflare/packages/worker/src/index.ts`:
- Around line 1904-1911: Update the credential-expiry handling in the route flow
around credentialExpiresAt and isOAuthKind so an OAuth credential expiring
within the lease window is marked frozen/deprioritized using the same mechanism
as the blocking-refresh case near line 1879, then re-route instead of throwing
immediately. Preserve the existing hard-failure behavior only when no eligible
credential remains.
In `@cmd/subrouter/codex.go`:
- Around line 102-111: Update the pinned-endpoint branch around
ensureLocalHealthy so its failure result is checked and converted into the same
clear “local proxy is unavailable; run '%s doctor'” diagnostic used by the
equivalent unpinned path. Preserve returning baseURL on successful health checks
and keep non-local pinned endpoints unchanged.
In `@cmd/subrouter/install_systemd_user.go`:
- Around line 15-65: Update installUserSystemd to accept the caller-provided
io.Writer and write the three confirmation messages through it instead of
fmt.Printf; update runSetup and all other call sites to pass their existing out
writer, preserving the current message content.
In `@cmd/subrouter/serverfallback_test.go`:
- Around line 186-211: Replace the shared healthy state in
TestEnsureLocalHealthyStartsDeadDaemon and the corresponding test at
cmd/subrouter/serverfallback_test.go lines 239-261 with atomic.Bool; use Load in
each HTTP handler and Store(true) in each start callback, preserving existing
behavior. Run go test ./... before handing off.
In `@cmd/subrouter/sr_claude_upload.go`:
- Around line 41-52: Replace the hardcoded “sr” command name in the team-storage
and local-storage error messages within the credential-source switch with the
dynamic name returned by r.programOrSubrouter(), matching the guidance behavior
later in the function. Preserve the existing message content and
credential-source handling.
In `@cmd/subrouter/sr_cloud.go`:
- Around line 126-131: The cloudLocalProxyToken flow must stop returning
config.AccessToken, since that session token is exposed to agent processes.
Generate and persist a separate random loopback-only bearer secret in the daemon
state directory, return it for LocalProxyToken, and validate proxy requests
against that secret while preserving the existing TeamModeReady gating.
- Around line 447-459: Update the anthropic-key handling in the relevant
subrouter command to read the API key through the existing no-echo secret-input
mechanism instead of promptLine, while retaining promptLine for the
non-sensitive label and preserving the current sk-ant- prefix validation.
In `@cmd/subrouter/sr_setup.go`:
- Around line 258-284: Update the “provider egress” checks in the team-ready and
local-credential branches to use the existing localOK result instead of
unconditionally reporting “ok.” Preserve the current success message when
localOK is true, and report a failing doctorCheck with the local daemon failure
details when it is false, keeping the output consistent with the earlier local
daemon check.
In `@cmd/subrouter/sr.go`:
- Around line 168-172: Update the command dispatch flow around cloudModeConfig
so configuration-load errors do not block the recovery/local verbs help, doctor,
cleanup, login, logout, storage, and setup; allow those commands to dispatch
without a loaded config, while preserving the existing “load credential storage”
error for other commands that require valid configuration.
In `@internal/broker/client_test.go`:
- Around line 39-40: Remove credential-bearing diagnostics from all three
assertions in internal/broker/client_test.go: at lines 39-40 replace the loaded
configuration dump with a generic failure message; at lines 198-203 split the
conditions and report failures without formatting err; at lines 286-292 report
the leak generically without printing err. Preserve the existing assertion
behavior while ensuring no sensitive fixture, error, token, key, request body,
or authorization data is emitted.
In `@internal/broker/client.go`:
- Around line 283-286: Update the cache replacement logic around the lease
assignment to remove the previous lease’s entry from leaseToKey before storing
the new lease mapping. Preserve the existing cache[key] and current lease-to-key
assignments, and ensure the cleanup only removes the superseded mapping when a
prior cached lease exists.
---
Outside diff comments:
In `@cloudflare/packages/worker/src/contract.ts`:
- Around line 690-713: Update refreshError to extract OAuth standard error and
error_description fields from parsed provider responses, including nested object
messages, and match those values against the existing known terminal-error
allowlist used by terminalRefreshFailure. Populate the appropriate providerCode
or equivalent terminal signal without persisting the raw response body, while
preserving the current status-based error message and sanitized error
properties.
In `@cmd/subrouter/codex.go`:
- Around line 20-28: Update the localProxyToken derivation in the codex setup
flow to depend on the resolved baseURL: only call cloudLocalProxyToken when
baseURL matches the local daemon endpoint, and otherwise leave the token unset
so it is not attached to pinned or remote endpoints. Preserve the existing cloud
configuration error handling and baseURL resolution.
---
Nitpick comments:
In `@cmd/subrouter/install_systemd_user.go`:
- Around line 50-61: Update the installation flow surrounding the systemd
user-unit setup to ensure user lingering is enabled before or alongside the
existing daemon-reload and enable --now commands. Use the existing command
runner and current-user context to invoke loginctl enable-linger for the
installing user, or provide an explicit warning if the project handles this
elsewhere; avoid duplicating the change if an existing linger lifecycle helper
is available.
In `@cmd/subrouter/main.go`:
- Around line 253-265: The two team-mode validation branches in the
config-loading flow should be collapsed into a single `CredentialSourceTeam &&
!cloudConfig.Ready()` check. Remove the redundant `LoggedIn()`/`TeamID`
condition and preserve the more specific selected-team error message only if the
existing readiness error cannot provide the required behavior; otherwise retain
one consistent error message.
In `@cmd/subrouter/sr_claude.go`:
- Around line 99-111: Remove the unused srRunner.cloudClaude method, including
its associated comment, while preserving the existing sr claude launch path and
its inlined team-mode readiness check before proxyClaude.
In `@cmd/subrouter/sr_cloud.go`:
- Around line 683-717: Update selectLocalAccountUpload to remove substring
fallback matching via strings.Contains and consider only exact label or
kind:label matches. Preserve the existing empty-selector, not-found, and
ambiguous-match errors, ensuring selectors like “work” do not resolve to labels
such as “work-backup”.
- Around line 73-77: Update the PollAuth handling in the ticker.C branch of the
device-login polling loop so transient network or 5xx errors are tolerated and
polling continues until the existing deadline. Do not return immediately from
this branch; retain the last error as needed and only return a failure when the
context/deadline expires, while preserving successful poll handling.
🪄 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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: fdbef0fc-f3b8-4473-bf00-f64a0eb88a11
📒 Files selected for processing (28)
cloudflare/packages/worker/README.mdcloudflare/packages/worker/src/contract.tscloudflare/packages/worker/src/index.tscloudflare/packages/worker/src/telemetry.tscloudflare/packages/worker/test/contract.test.tscloudflare/packages/worker/test/credential-lease.test.tscmd/subrouter/cloud_mode_test.gocmd/subrouter/codex.gocmd/subrouter/install_systemd_user.gocmd/subrouter/lifecycle.gocmd/subrouter/main.gocmd/subrouter/main_test.gocmd/subrouter/serverfallback.gocmd/subrouter/serverfallback_test.gocmd/subrouter/sr.gocmd/subrouter/sr_claude.gocmd/subrouter/sr_claude_upload.gocmd/subrouter/sr_cloud.gocmd/subrouter/sr_daemon.gocmd/subrouter/sr_server.gocmd/subrouter/sr_setup.gocmd/subrouter/sr_setup_test.godocs/local-egress.mdinternal/broker/client.gointernal/broker/client_test.gointernal/broker/config.gointernal/proxy/credential_broker_test.gointernal/proxy/proxy.go
🚧 Files skipped from review as they are similar to previous changes (3)
- cmd/subrouter/sr_setup_test.go
- cmd/subrouter/sr_server.go
- cmd/subrouter/lifecycle.go
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
cmd/subrouter/sr_cloud.go (1)
469-481: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winTwo secret-entry prompts in
sr_cloud.goecho API keys to the terminal. Both callpromptLine, which has no echo-suppression, for reading sensitive API keys — the root cause is the shared lack of a no-echo secret-input helper.
cmd/subrouter/sr_cloud.go#L469-L481: switch theanthropic-keyprompt ("Anthropic API key (sk-ant-...): ") to a no-echo reader (e.g.golang.org/x/term.ReadPassword), keepingpromptLineonly for the non-sensitive label.cmd/subrouter/sr_cloud.go#L611-L657: switch the"Replacement API key for %s: "prompt inreplacementUploadForSharedAccountto the same no-echo reader.🤖 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 `@cmd/subrouter/sr_cloud.go` around lines 469 - 481, The secret API-key prompts currently use echoing input; update the anthropic-key flow in cmd/subrouter/sr_cloud.go lines 469-481 to keep promptLine for the label but read the key with a shared no-echo helper based on terminal password input, preserving validation. Apply the same no-echo reader to the replacementUploadForSharedAccount prompt in cmd/subrouter/sr_cloud.go lines 611-657, reusing the helper rather than introducing separate implementations.
🧹 Nitpick comments (2)
cloudflare/packages/worker/test/credential-lease.test.ts (1)
634-638: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDocument why the replacement-refresh wait is bounded.
Promise.racewithBun.sleep(50)means the intended interleaving isn't guaranteed on a loaded runner — the final assertion still passes either way, so a lost race silently weakens the test rather than failing it. A short comment noting the timeout exists to avoid hanging when the replacement refresh never fires would keep that tradeoff visible.🤖 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 `@cloudflare/packages/worker/test/credential-lease.test.ts` around lines 634 - 638, Document the bounded wait around Promise.race in the replacement-refresh test, explaining that Bun.sleep(50) prevents the test from hanging if replacementRefreshStarted never fires, while acknowledging the intended interleaving may not be guaranteed under load.cmd/subrouter/sr_cloud.go (1)
482-486: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDuplicated
AccountUploadmap construction for API-key providers.Both the anthropic-key direct-add path and
replacementUploadForSharedAccountbuild the same{"provider","label","apiKey"}map shape. Consider extracting a smallapiKeyAccountUpload(kind, label, key string) broker.AccountUploadhelper to keep the two call sites in sync.Also applies to: 652-656
🤖 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 `@cmd/subrouter/sr_cloud.go` around lines 482 - 486, The API-key AccountUpload map is duplicated between the anthropic direct-add path and replacementUploadForSharedAccount. Extract an apiKeyAccountUpload(kind, label, key string) broker.AccountUpload helper, then use it at both construction sites while preserving provider, label, and trimmed apiKey values.
🤖 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 `@cmd/subrouter/lifecycle_test.go`:
- Around line 21-39: The test’s wait callback must verify success output has not
been written before readiness is checked. Update
TestRestartWaitsForDaemonHealthBeforeReportingSuccess so waitForReady asserts
that out is empty (or records and validates an equivalent event sequence), while
preserving the existing health-check count and final output assertions.
In `@cmd/subrouter/main.go`:
- Around line 270-273: Update serve so TeamModeReady enforces a closed
credential boundary before server construction: reject or bypass the
unconditional Bedrock setup and disable personal API-key fallback, including
SUBROUTER_CLAUDE_FABLE_API_KEY. Ensure the proxy.Server configuration clears the
corresponding legacy credential fields in team mode while preserving normal
credential behavior outside team mode.
In `@internal/broker/client_test.go`:
- Around line 170-178: Update the httptest server handler in the relevant client
test to avoid calling t.Fatal or t.Fatalf from its goroutine; capture handler
decode and assertion failures in a thread-safe result or error value, then
assert on the main test goroutine after the request completes. Preserve the
existing request validation and run go test ./... before handoff.
---
Duplicate comments:
In `@cmd/subrouter/sr_cloud.go`:
- Around line 469-481: The secret API-key prompts currently use echoing input;
update the anthropic-key flow in cmd/subrouter/sr_cloud.go lines 469-481 to keep
promptLine for the label but read the key with a shared no-echo helper based on
terminal password input, preserving validation. Apply the same no-echo reader to
the replacementUploadForSharedAccount prompt in cmd/subrouter/sr_cloud.go lines
611-657, reusing the helper rather than introducing separate implementations.
---
Nitpick comments:
In `@cloudflare/packages/worker/test/credential-lease.test.ts`:
- Around line 634-638: Document the bounded wait around Promise.race in the
replacement-refresh test, explaining that Bun.sleep(50) prevents the test from
hanging if replacementRefreshStarted never fires, while acknowledging the
intended interleaving may not be guaranteed under load.
In `@cmd/subrouter/sr_cloud.go`:
- Around line 482-486: The API-key AccountUpload map is duplicated between the
anthropic direct-add path and replacementUploadForSharedAccount. Extract an
apiKeyAccountUpload(kind, label, key string) broker.AccountUpload helper, then
use it at both construction sites while preserving provider, label, and trimmed
apiKey values.
🪄 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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: ae677267-8572-4e6d-9c36-4a0cf5cb7600
📒 Files selected for processing (27)
cloudflare/packages/worker/src/contract.tscloudflare/packages/worker/src/index.tscloudflare/packages/worker/test/contract.test.tscloudflare/packages/worker/test/credential-lease.test.tscmd/subrouter/cloud_mode_test.gocmd/subrouter/codex.gocmd/subrouter/install_daemon.gocmd/subrouter/install_daemon_test.gocmd/subrouter/install_systemd_test.gocmd/subrouter/install_systemd_user.gocmd/subrouter/lifecycle.gocmd/subrouter/lifecycle_test.gocmd/subrouter/main.gocmd/subrouter/main_test.gocmd/subrouter/serverfallback.gocmd/subrouter/serverfallback_test.gocmd/subrouter/sr.gocmd/subrouter/sr_claude.gocmd/subrouter/sr_claude_test.gocmd/subrouter/sr_cloud.gocmd/subrouter/sr_setup.gocmd/subrouter/sr_setup_test.gointernal/broker/client.gointernal/broker/client_test.gointernal/broker/config.gointernal/proxy/credential_broker_test.gointernal/proxy/proxy.go
🚧 Files skipped from review as they are similar to previous changes (13)
- cloudflare/packages/worker/test/contract.test.ts
- cmd/subrouter/install_systemd_user.go
- cmd/subrouter/serverfallback.go
- cmd/subrouter/sr_claude.go
- internal/broker/config.go
- cmd/subrouter/sr_setup_test.go
- cmd/subrouter/sr_setup.go
- cmd/subrouter/codex.go
- cloudflare/packages/worker/src/contract.ts
- cmd/subrouter/lifecycle.go
- internal/proxy/proxy.go
- cmd/subrouter/sr.go
- cloudflare/packages/worker/src/index.ts
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
cmd/subrouter/cloud_mode_test.go (1)
467-473: 🔒 Security & Privacy | 🟡 Minor | ⚡ Quick winAssert the read-only bind directive explicitly.
Checking only the path permits a writable mount regression. Expect
BindReadOnlyPaths=/home/alice/.codex-accountsexactly.🤖 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 `@cmd/subrouter/cloud_mode_test.go` around lines 467 - 473, Update the expected strings in the test around the want list to assert the complete read-only bind directive for the codex accounts path: use “BindReadOnlyPaths=/home/alice/.codex-accounts” instead of checking only “/home/alice/.codex-accounts”.
🤖 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 `@cmd/subrouter/cloud_mode_test.go`:
- Around line 527-533: Update the assertion in the localAccountUploads test to
avoid formatting uploads with %+v, since that exposes apiKey values from
localAccountUpload.body; report only a safe count or non-sensitive summary when
the expected count is wrong. Before handoff, run go test ./....
---
Outside diff comments:
In `@cmd/subrouter/cloud_mode_test.go`:
- Around line 467-473: Update the expected strings in the test around the want
list to assert the complete read-only bind directive for the codex accounts
path: use “BindReadOnlyPaths=/home/alice/.codex-accounts” instead of checking
only “/home/alice/.codex-accounts”.
🪄 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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: a3eb52d7-da45-4bc0-803f-9776fd84fab6
📒 Files selected for processing (7)
cloudflare/packages/worker/src/contract.tscloudflare/packages/worker/src/index.tscloudflare/packages/worker/test/contract.test.tscloudflare/packages/worker/test/credential-lease.test.tscmd/subrouter/cloud_mode_test.gocmd/subrouter/install_systemd_user.gocmd/subrouter/sr_cloud.go
🚧 Files skipped from review as they are similar to previous changes (6)
- cloudflare/packages/worker/test/contract.test.ts
- cmd/subrouter/install_systemd_user.go
- cloudflare/packages/worker/src/contract.ts
- cloudflare/packages/worker/test/credential-lease.test.ts
- cmd/subrouter/sr_cloud.go
- cloudflare/packages/worker/src/index.ts
lawrencecchen
temporarily deployed
to
subrouter-staging
July 29, 2026 07:25 — with 
GitHub Actions
Inactive
GitHub Actions
Inactive
lawrencecchen
temporarily deployed
to
subrouter-staging
July 29, 2026 08:10 — with
GitHub Actions
Inactive
GitHub Actions
Inactive
lawrencecchen
temporarily deployed
to
subrouter-staging
July 29, 2026 08:17 — with
GitHub Actions
Inactive
GitHub Actions
Inactive
Contributor
Author
|
@coderabbitai review |
✅ Action performedReview finished.
|
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 `@cmd/subrouter/cloud_mode_test.go`:
- Around line 76-78: Stop exposing bearer credentials in assertion failures:
update the token checks in cmd/subrouter/cloud_mode_test.go at lines 76-78 and
91-95. At lines 76-78, replace the formatted proxy token with a fixed safe
failure message; at lines 91-95, report only that a non-empty token was returned
without formatting its value.
In `@internal/proxy/proxy.go`:
- Around line 2058-2084: Update credentialLeaseReport so claudeExhaustionExpiry
is used only when provider == accounts.ProviderClaude. For non-Claude
rate-limited providers, derive RetryAt using their provider-appropriate
semantics, such as the Retry-After header, without applying Claude-specific
reset behavior.
🪄 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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 8dd5bd88-4fdb-4710-9bc2-c9a95621d56a
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (24)
cloudflare/packages/worker/src/contract.tscloudflare/packages/worker/src/index.tscloudflare/packages/worker/test/contract.test.tscloudflare/packages/worker/test/credential-lease.test.tscmd/subrouter/cloud_mode_test.gocmd/subrouter/codex.gocmd/subrouter/install_systemd_user.gocmd/subrouter/lifecycle_test.gocmd/subrouter/main.gocmd/subrouter/main_test.gocmd/subrouter/serverfallback.gocmd/subrouter/serverfallback_test.gocmd/subrouter/sr.gocmd/subrouter/sr_claude.gocmd/subrouter/sr_claude_upload.gocmd/subrouter/sr_cloud.gocmd/subrouter/sr_server.gocmd/subrouter/sr_setup.gocmd/subrouter/sr_setup_test.gogo.modinternal/broker/client.gointernal/broker/client_test.gointernal/proxy/credential_broker_test.gointernal/proxy/proxy.go
🚧 Files skipped from review as they are similar to previous changes (15)
- cmd/subrouter/lifecycle_test.go
- cmd/subrouter/sr_claude_upload.go
- cmd/subrouter/sr_setup.go
- cmd/subrouter/serverfallback_test.go
- cmd/subrouter/sr_setup_test.go
- cmd/subrouter/install_systemd_user.go
- cmd/subrouter/serverfallback.go
- cmd/subrouter/codex.go
- cmd/subrouter/sr.go
- internal/broker/client_test.go
- cloudflare/packages/worker/src/contract.ts
- cmd/subrouter/main.go
- cmd/subrouter/sr_cloud.go
- cloudflare/packages/worker/test/contract.test.ts
- cloudflare/packages/worker/src/index.ts
lawrencecchen
temporarily deployed
to
subrouter-staging
July 29, 2026 08:37 — with
GitHub Actions
Inactive
GitHub Actions
Inactive
lawrencecchen
temporarily deployed
to
subrouter-staging
July 29, 2026 08:47 — with
GitHub Actions
Inactive
GitHub Actions
Inactive
lawrencecchen
temporarily deployed
to
subrouter-staging
July 29, 2026 08:51 — with
GitHub Actions
Inactive
GitHub Actions
Inactive
This was referenced Jul 29, 2026
This branch was previously deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Replaces the shared-Mac data plane with a loopback proxy on every macOS or Linux client. cmux.com chooses a shared team account and returns a five-minute access-only lease. The local daemon sends the provider request from that client machine. Provider refresh tokens stay central.
Refresh custody
invalid_grantis terminal. Transport failures and malformed success responses are ambiguous, freeze the account, and never retry the old refresh token.Local data plane and CLI
sr login,sr logout,sr team list|use, andsr account list|import|repair|delete.sr setup,sr doctor,sr cleanup, andsr daemon start|stop|restart|status|logs.sr codexandsr claudeauto-start and use the loopback daemon in team-vault mode.--all --yes; rollout starts with one exact canary account.The cmux.com API is manaflow-ai/cmux#9099.
Deployment and canary
467267f5-fb1c-4d25-bacc-730a566a42ffatsubrouter.cmux.dev.de30e48e-aa7d-4bb1-8704-a27dcd9d2e71atsubrouter-staging.cmux.dev.OKthrough this Mac's local daemon. Central status remained healthy with no refresh failure.Verification
go test ./... -count=1bun test(50 pass)bun run typecheckSummary by CodeRabbit