Repository navigation
Let normal users onboard to hosted Subrouter - #120
lawrencecchen wants to merge 9 commits into
Conversation
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
📝 WalkthroughWalkthroughAdds Stack Auth hosted login, tenant authentication, hosted account management, remote commands, and shared Claude state migration. The change also adds tenant-scoped proxy APIs, hosted credential validation, and provider-specific Codex, Claude, and API-key workflows. ChangesHosted Stack Auth and tenant services
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant User
participant sr_cloud
participant StackAuth
participant MultiTenant
participant HostedBroker
User->>sr_cloud: Start hosted login
sr_cloud->>StackAuth: Start and poll CLI authentication
sr_cloud->>StackAuth: Exchange selected team
StackAuth-->>sr_cloud: Return tenant key and proxy URL
sr_cloud->>HostedBroker: Upload hosted account
HostedBroker->>MultiTenant: Send tenant-scoped account request
MultiTenant-->>HostedBroker: Return account result
HostedBroker-->>sr_cloud: Return upload result
Possibly related PRs
🚥 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: 12
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
cmd/subrouter/sr_cloud.go (2)
553-580: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
sr storage hostedsaves an unready hosted source, then reports an error.
parseCredentialSourcenow mapshosted,cmux,team,shared, andcloudtoCredentialSourceHosted. This function has no hosted readiness check. Line 570 guards onlyCredentialSourceTeam, whichparseCredentialSourcecan no longer return.On a machine that has not run
sr login,sr storage hostedtherefore reaches line 573, writescredentialSource: hostedwith noHostedURLand noTenantKey, and only then fails insideprintCredentialSourceat line 601. Two consequences follow: the config is persisted in an unusable state, andrestartInstalledDaemon()at line 580 is skipped, so the failure looks partial.Add the same
HostedReadyguard thatcloudSetupapplies at line 369, beforeSaveConfig.Lines 555 and 570 also test
CredentialSourceTeam, which is now unreachable fromparseCredentialSource. Remove them or route them through the hosted checks.🐛 Proposed guard
- if source == broker.CredentialSourceTeam && !config.Ready() { - return fmt.Errorf("team credential storage requires login and a selected team; run 'sr login'") - } + if source == broker.CredentialSourceHosted { + candidate := config + candidate.CredentialSource = source + if !candidate.HostedReady() { + return fmt.Errorf("hosted cmux requires login and a selected team; run 'sr login'") + } + } config.CredentialSource = source🤖 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 553 - 580, Update the credential-source handling around parseCredentialSource and broker.SaveConfig to treat CredentialSourceHosted as requiring hosted readiness, using the same HostedReady validation as cloudSetup before persisting configuration. Remove or replace the now-unreachable CredentialSourceTeam checks in the load-error and readiness branches, ensuring sr storage hosted fails before SaveConfig when HostedURL or TenantKey is missing.
439-465: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
sr team currentdiscards a rotated refresh token and requires the network.Lines 440-445 refresh the Stack session and overwrite
config.AccessTokenandconfig.RefreshTokenin memory. Thecurrentbranch at lines 460-465 then returns without callingbroker.SaveConfig. The rotated refresh token is lost while the Stack server has already rotated it. Thedefaultbranch at line 503 drops it the same way. Onlylistpersists it, at line 459.The
currentbranch reads onlyconfig.TeamIDandconfig.TeamName, which are already on disk. It needs neither the refresh nor theListTeamscall, so today it also fails when the machine is offline.Move the refresh and the team listing into the branches that need them.
🐛 Proposed restructure
client := nativeStackClient(config, r.client) - tokens, err := client.Refresh(ctx, config.RefreshToken) - if err != nil { - return fmt.Errorf("refresh Stack session: %w", err) - } - config.AccessToken = tokens.AccessToken - config.RefreshToken = tokens.RefreshToken - stackTeams, err := client.ListTeams(ctx, tokens.AccessToken) - if err != nil { - return err - } - switch args[0] { + if args[0] == "current" { + if config.TeamID == "" { + return fmt.Errorf("no team selected; run 'sr team list' then 'sr team use <team>'") + } + fmt.Fprintf(r.out, "%s (%s)\n", config.TeamName, config.TeamID) + return nil + } + tokens, err := client.Refresh(ctx, config.RefreshToken) + if err != nil { + return fmt.Errorf("refresh Stack session: %w", err) + } + config.AccessToken = tokens.AccessToken + config.RefreshToken = tokens.RefreshToken + stackTeams, err := client.ListTeams(ctx, tokens.AccessToken) + if err != nil { + // Persist the rotated token even when listing fails. + _ = broker.SaveConfig(path, config) + return err + } + switch args[0] {Then remove the now-duplicated
currentcase, and save the config on thedefaultbranch as well.🤖 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 439 - 465, Restructure the team command switch so the current branch reads local TeamID and TeamName without refreshing or listing teams, allowing offline use. Move session refresh and ListTeams into the list/ls branch, preserving token updates and config persistence there. Remove the duplicated current case and ensure the default branch also saves the updated config before returning.cmd/subrouter/sr_setup.go (1)
262-281: 🔒 Security & Privacy | 🟡 Minor | ⚡ Quick winWarn when purge cannot revoke the session.
When
StackProjectIDorStackPublishableis empty,logoutErrstays nil and no revocation is attempted. The purge then deletes the config at line 282 and reports success.The plan text at line 228 already told the user that cleanup will "revoke and delete the cmux.com session". For a config written before native Stack Auth, only the delete happens. The remote session stays valid, and after
DeleteConfigremoves the tokens the user can no longer revoke it.
cloudLogoutincmd/subrouter/sr_cloud.golines 407-412 already prints a warning for this exact condition. Print the same warning here.🔒️ Proposed fix
var logoutErr error if cloudConfig.StackProjectID != "" && cloudConfig.StackPublishable != "" { logoutErr = nativeStackClient(cloudConfig, nil).SignOut( ctx, cloudConfig.AccessToken, cloudConfig.RefreshToken, ) + } else { + fmt.Fprintln( + out, + "warning: this legacy session cannot be revoked because its retired auth endpoint no longer exists; revoke it in the cmux.com dashboard", + ) }🤖 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.go` around lines 262 - 281, Update the purge cleanup flow around cloudConfig.StackProjectID and cloudConfig.StackPublishable to warn when either value is empty and no remote sign-out is attempted. Reuse the same warning message and logging approach as cloudLogout, while preserving the existing SignOut and logoutErr handling for complete native Stack Auth configurations.cmd/subrouter/sr_claude_upload.go (1)
43-51: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winName the active credential source in the message.
The branch now covers
CredentialSourceHosted, but Line 46 still states "team storage". A user who ransr storage hostedsees a message about team storage.sr.goat Line 387 uses "hosted cmux" for the same condition, so the two messages disagree. Select the wording from the matched source.🐛 Proposed fix
- switch config.EffectiveCredentialSource() { - case broker.CredentialSourceTeam, broker.CredentialSourceHosted: + switch source := config.EffectiveCredentialSource(); source { + case broker.CredentialSourceTeam, broker.CredentialSourceHosted: if requireServer { + label := "team storage" + if source == broker.CredentialSourceHosted { + label = "hosted cmux" + } return fmt.Errorf( - "team storage uses '%s account import --only claude:%s'", + "%s uses '%s account import --only claude:%s'", + label, r.programOrSubrouter(), name, ) } return nil🤖 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_upload.go` around lines 43 - 51, Update the error message in the CredentialSourceTeam/CredentialSourceHosted branch to name the matched credential source, using “team storage” for CredentialSourceTeam and “hosted cmux” for CredentialSourceHosted, consistent with the wording in sr.go. Preserve the existing command and error behavior.
🧹 Nitpick comments (21)
internal/agents/claude/store.go (1)
380-427: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueInconsistent empty-check for
SharedStateDirbetweeninitInstanceDirandprepareSharedState.Line 392 checks
s.SharedStateDir == ""(exact match), while line 413 insideprepareSharedStatechecksstrings.TrimSpace(s.SharedStateDir) == "". IfSharedStateDirwere ever a whitespace-only string,prepareSharedStatetreats it as empty (no-op) butinitInstanceDirwould still skip creating localclaudeHighGrowthDirscopies, since its own check evaluatesfalse. The instance would end up with neither shared nor local high-growth directories. This requires an unusualSharedStateDirvalue to trigger, sinceDefaultStoreonly produces""or a real path.Use the same trimmed check in both places, for example by extracting a small helper.
🤖 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 `@internal/agents/claude/store.go` around lines 380 - 427, Use a consistent trimmed emptiness check for SharedStateDir in initInstanceDir and prepareSharedState. Update initInstanceDir’s high-growth directory condition to treat whitespace-only values as empty, or extract and reuse a helper so both methods apply identical behavior.cmd/subrouter/main.go (2)
219-219: 🔒 Security & Privacy | 🔵 TrivialPrefer the environment variable for the tenant-key secret in deployment.
--stack-tenant-key-secretaccepts a signing secret on the command line. Any local user can read it from the process argument list. The flag already falls back toSUBROUTER_STACK_TENANT_KEY_SECRET, and existing flags such as--admin-tokenuse the same pattern, so this is not a new defect.Document the environment variable as the supported deployment path, and keep the flag for local development only.
🤖 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` at line 219, Update the help text and configuration guidance for stackTenantKeySecret to identify SUBROUTER_STACK_TENANT_KEY_SECRET as the supported deployment configuration, while retaining --stack-tenant-key-secret for local development. Preserve the existing environment-variable fallback behavior and align the wording with the established --admin-token pattern.
240-270: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winValidate
--public-urlat startup, and name both the flag and the variable in the error.Two points on this block:
*publicURLreceives no validation.proxy.MultiTenantusesPublicURLto build theproxyUrlin the/_subrouter/auth/stackresponse. The CLI persists that value asHostedURL, wherebroker.Config.Validaterequires an HTTPS origin without path, query, or fragment. A malformed--public-urltherefore fails on the client duringsr login, not at server startup. Validate it here so a misconfigured deployment fails fast.- The error at line 266 names only the environment variables. An operator who passed
--stack-project-idsees an error aboutSUBROUTER_STACK_PROJECT_ID. Name both forms.♻️ Proposed change
if stackLoginConfigured != 0 && stackLoginConfigured != len(stackLoginValues) { - return errors.New("hosted Stack login requires SUBROUTER_STACK_PROJECT_ID, SUBROUTER_STACK_PUBLISHABLE_CLIENT_KEY, and SUBROUTER_STACK_TENANT_KEY_SECRET") + return errors.New("hosted Stack login requires all of --stack-project-id, --stack-publishable-client-key, and --stack-tenant-key-secret (or SUBROUTER_STACK_PROJECT_ID, SUBROUTER_STACK_PUBLISHABLE_CLIENT_KEY, and SUBROUTER_STACK_TENANT_KEY_SECRET)") } if *stackTenantKeySecret != "" && len(*stackTenantKeySecret) < 32 { return errors.New("SUBROUTER_STACK_TENANT_KEY_SECRET must be at least 32 bytes") } + if trimmed := strings.TrimRight(*publicURL, "/"); trimmed != "" { + parsed, parseErr := url.Parse(trimmed) + if parseErr != nil || parsed.Host == "" || + (parsed.Scheme != "https" && parsed.Scheme != "http") || + parsed.RawQuery != "" || parsed.Fragment != "" { + return errors.New("--public-url must be an origin such as https://sr.example.com") + } + }🤖 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 240 - 270, Validate the resolved *publicURL value during startup using the same HTTPS-origin rules required by broker.Config.Validate, rejecting paths, queries, fragments, and other malformed values before serving requests. Update the hosted Stack login configuration error to name both the corresponding CLI flags and environment variables, covering stackLoginValues and its partial-configuration check.cmd/subrouter/sr_claude.go (2)
218-222: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRename the
cloudClaudeEnvironmentparameters, which are no longer local-only.
proxyClaudeTonow passes a hosted proxy root URL and a tenant key into this function, but the parameters are still namedlocalandlocalProxyToken. The names contradict the values in the hosted path.Rename them to
baseURLandproxyTokento matchproxyClaudeTo.🤖 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 218 - 222, Rename the cloudClaudeEnvironment parameters local and localProxyToken to baseURL and proxyToken, and update all references within the function and its callers as needed, including proxyClaudeTo, without changing behavior.
71-80: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueExtract the hosted-remote resolution, which now exists twice.
This block resolves the selected remote and requires
ok,Name == "cmux", and a non-emptyTenantKey.routeClaudeProfileThroughHostedincmd/subrouter/sr_cloud.golines 1202-1208 applies the same three conditions with a different error message.Extract one helper that returns the hosted server or an error, so the rule for a usable hosted remote has one definition.
🤖 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 71 - 80, The hosted remote validation is duplicated between the shown Claude routing block and routeClaudeProfileThroughHosted. Extract a shared helper that calls selectedRemoteServer, requires ok, server.Name == "cmux", and a non-empty TenantKey, returning the validated server or the appropriate error; update both callers to use it while preserving their existing proxy/profile behavior.cmd/subrouter/sr_hosted_login_test.go (1)
162-164: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAssert the uploaded token payload.
This check covers only
providerandlabel.hostedCodexAddbuilds the nestedtokensmap by hand with the keysaccessToken,refreshToken,idToken, andaccountID. A misspelled key, or a droppedrefreshToken, still passes this assertion and produces an unusable hosted credential.Assert the
tokensmap contents as well.💚 Proposed addition
if uploaded["provider"] != "codex" || uploaded["label"] != "hosted@example.com" { t.Fatalf("upload = %#v", uploaded) } + tokens, ok := uploaded["tokens"].(map[string]any) + if !ok { + t.Fatalf("tokens = %#v", uploaded["tokens"]) + } + for _, key := range []string{"accessToken", "refreshToken", "idToken"} { + if value, _ := tokens[key].(string); value == "" { + t.Fatalf("tokens[%q] is empty: %#v", key, tokens) + } + }🤖 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_hosted_login_test.go` around lines 162 - 164, Extend the assertion in the hostedCodexAdd upload test to validate the nested uploaded["tokens"] map, including the expected accessToken, refreshToken, idToken, and accountID keys and values. Keep the existing provider and label checks, and fail the test when any token field is missing, misspelled, or incorrect.cmd/subrouter/sr_cloud.go (1)
86-116: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winA transient poll error aborts the whole login.
Line 88 returns as soon as
PollCLIfails. The loop runs for up to 15 minutes while the user completes the browser approval. One network blip, one proxy hiccup, or one HTTP 429 from the Stack API therefore discards an approval that may already have succeeded, and the user must restartsr login.Tolerate transient failures. Retry with backoff up to the existing deadline, and return only when the deadline expires or the error is terminal.
♻️ Sketch of the retry structure
var refreshToken string + pollFailures := 0 for { poll, pollErr := stackClient.PollCLI(ctx, start.PollingCode) - if pollErr != nil { - return fmt.Errorf("poll cmux.com login: %w", pollErr) - } - switch poll.Status { + if pollErr != nil { + pollFailures++ + if pollFailures > 5 { + return fmt.Errorf("poll cmux.com login: %w", pollErr) + } + } else { + pollFailures = 0 + switch poll.Status { // ... existing status handling + } }Keep the existing
deadlineas the overall bound.🤖 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 86 - 116, Update the polling loop around stackClient.PollCLI to tolerate transient poll errors instead of immediately returning them. Retry failed polls with backoff while respecting the existing deadline and context cancellation, but return terminal errors immediately; preserve the current success, expired/used, unexpected-status, and refresh-token validation behavior.internal/broker/config.go (1)
115-170: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winCompute
Normalized()once, and reuseisLoopbackHostfor the base URL.
Validatecallsc.Normalized()three times: at line 116, at line 134, and at line 145. Each call copies the struct and repeats several string operations.Validateruns at the start of everydoJSONanddoHostedJSONrequest, so this sits on the request path.The new
isLoopbackHosthelper also duplicates the inline loopback logic at lines 125-129. Call the helper in both places so one definition governs loopback.♻️ Proposed change
func (c Config) Validate() error { - baseURL, err := url.Parse(c.Normalized().BaseURL) + normalized := c.Normalized() + baseURL, err := url.Parse(normalized.BaseURL) if err != nil { return fmt.Errorf("invalid cmux.com base URL: %w", err) } if baseURL.Host == "" || baseURL.User != nil || (baseURL.Path != "" && baseURL.Path != "/") || baseURL.RawQuery != "" || baseURL.Fragment != "" { return errors.New("cmux.com base URL must be an origin without credentials, path, query, or fragment") } - host := strings.ToLower(baseURL.Hostname()) - loopback := host == "localhost" - if ip := net.ParseIP(host); ip != nil { - loopback = ip.IsLoopback() - } if baseURL.Scheme != "https" && - !(baseURL.Scheme == "http" && loopback) { + !(baseURL.Scheme == "http" && isLoopbackHost(baseURL.Hostname())) { return errors.New("cmux.com base URL must use HTTPS, except for a loopback development server") } - switch c.Normalized().CredentialSource { + switch normalized.CredentialSource { case "", CredentialSourceTeam, CredentialSourceLocal, CredentialSourceLegacy, CredentialSourceHosted: default: return fmt.Errorf( "credential source must be %q, %q, %q, or %q", CredentialSourceTeam, CredentialSourceLocal, CredentialSourceLegacy, CredentialSourceHosted, ) } - normalized := c.Normalized() if normalized.CredentialSource == CredentialSourceHosted {Remove the now-unused
netimport only if no other reference remains.🤖 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 `@internal/broker/config.go` around lines 115 - 170, Update Config.Validate to assign c.Normalized() once and reuse that normalized value for BaseURL, CredentialSource, TenantKey, and HostedURL validation instead of calling it repeatedly. Replace the inline base-URL loopback detection with isLoopbackHost(baseURL.Hostname()), preserving the existing localhost and IP loopback behavior, and remove the net import if no longer used.internal/stackauth/client.go (1)
407-422: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUse one loopback definition across packages.
This function accepts only the literal hosts
localhost,127.0.0.1, and::1.internal/broker/config.godefinesisLoopbackHost, which usesnet.ParseIP(...).IsLoopback()and accepts the whole loopback range. A developer who runs a hosted Subrouter onhttp://127.0.0.2:31415passes broker validation but fails here.Consider exporting one loopback predicate and calling it from both packages.
♻️ Proposed change in
internal/stackauth/client.goif parsed.Scheme == "https" { return nil } - if parsed.Scheme == "http" && (parsed.Hostname() == "localhost" || parsed.Hostname() == "127.0.0.1" || parsed.Hostname() == "::1") { - return nil - } + if parsed.Scheme == "http" && loopbackHost(parsed.Hostname()) { + return nil + } return errors.New("URL must use HTTPS, except on loopback") } + +func loopbackHost(host string) bool { + host = strings.ToLower(strings.TrimSpace(host)) + if host == "localhost" { + return true + } + ip := net.ParseIP(host) + return ip != nil && ip.IsLoopback() +}Add
"net"to the imports.🤖 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 `@internal/stackauth/client.go` around lines 407 - 422, Unify loopback detection by exporting the existing predicate behind isLoopbackHost in internal/broker/config.go and reusing it from validateHTTPSOriginOrURL. Replace the hard-coded localhost, 127.0.0.1, and ::1 checks with the shared net.ParseIP-based predicate, preserving HTTPS requirements and HTTP acceptance for all loopback addresses.internal/broker/client.go (1)
328-330: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winSurface the hosted error body, as
doJSONdoes.This branch discards
dataand reports only the numeric status andhttp.StatusText. A 400 or 409 from the hosted API becomeshosted Subrouter request failed (400): Bad Requestwith no reason.
doJSONat line 516 callsapiErrorFromResponse(response.StatusCode, data)and preserves the server message. The hosted path carries thesr add codex,sr add claude, andsr account removeflows, so the user sees these errors directly during onboarding.Decode the error field from the response instead of dropping it. Do not echo the whole body verbatim, so a server that reflects a request cannot leak an uploaded credential into terminal output.
♻️ Proposed change
if response.StatusCode < 200 || response.StatusCode >= 300 { - return fmt.Errorf("hosted Subrouter request failed (%d): %s", response.StatusCode, http.StatusText(response.StatusCode)) + message := http.StatusText(response.StatusCode) + var wire struct { + Error string `json:"error"` + Message string `json:"message"` + } + if json.Unmarshal(data, &wire) == nil { + if wire.Error != "" { + message = wire.Error + } else if wire.Message != "" { + message = wire.Message + } + } + return fmt.Errorf("hosted Subrouter request failed (%d): %s", response.StatusCode, message) }🤖 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 `@internal/broker/client.go` around lines 328 - 330, Update the non-2xx response handling in the hosted Subrouter request flow to extract the server’s error field from data and include only that decoded message in the returned error, matching the existing apiErrorFromResponse behavior used by doJSON. Preserve the status code/status text fallback when the body has no usable error field, and never print the entire response body.internal/proxy/multitenant_test.go (3)
497-509: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueCover the membership-lookup failure branches.
handleStackAuthreturns 503 whenStackTeamsis nil and whenListTeamsreturns an error. Neither branch has a test.fakeStackTeamsalready carries anerrfield, so add a case that sets it and a case that leavesStackTeamsnil.🤖 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 `@internal/proxy/multitenant_test.go` around lines 497 - 509, Extend TestStackLoginAcceptsAnotherTeamAfterMembershipCheck with cases covering handleStackAuth’s 503 responses: one using fakeStackTeams with its err field set, and another with StackTeams left nil. Assert each request returns HTTP 503 while preserving the existing successful membership-check coverage.
460-464: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winCover the tenant account delete route.
The test uploads and lists an account. It never calls
DELETE /t/<key>/_subrouter/accounts/<id>, sohandleTenantAccountDeletehas no coverage, including its 404 branch. Add a delete step after the list assertion.💚 Proposed addition
if list.Code != http.StatusOK || !strings.Contains(list.Body.String(), "openai-apikey:work") { t.Fatalf("accounts = %d: %s", list.Code, list.Body.String()) } + missing := httptest.NewRecorder() + handler.ServeHTTP(missing, httptest.NewRequest( + http.MethodDelete, "/t/"+key+"/_subrouter/accounts/absent", nil)) + if missing.Code != http.StatusNotFound { + t.Fatalf("delete missing status = %d: %s", missing.Code, missing.Body.String()) + } + removed := httptest.NewRecorder() + handler.ServeHTTP(removed, httptest.NewRequest( + http.MethodDelete, "/t/"+key+"/_subrouter/accounts/apikey:openai-apikey:work", nil)) + if removed.Code != http.StatusOK { + t.Fatalf("delete status = %d: %s", removed.Code, removed.Body.String()) + }As per coding guidelines, run
go test ./...after the change.🤖 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 `@internal/proxy/multitenant_test.go` around lines 460 - 464, Add a DELETE request after the existing account list assertion in the tenant account test, targeting /t/<key>/_subrouter/accounts/<id> and asserting successful deletion; also add a request for a nonexistent account ID to cover handleTenantAccountDelete’s 404 branch. Run go test ./... after updating the test.Source: Coding guidelines
470-492: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAssert one expected status per case.
Line 490 accepts 401 or 403 for both cases. The invalid-token case must return 401 and the wrong-team case must return 403. As written, a change that swaps the two responses still passes. Put the expected status in the table.
💚 Proposed change
- for name, verifier := range map[string]fakeStackVerifier{ - "invalid token": {err: errors.New("bad token")}, - "wrong team": {claims: stackauth.Claims{ - ProjectID: "project", SelectedTeamID: "other-team", - }}, - } { + cases := map[string]struct { + verifier fakeStackVerifier + status int + }{ + "invalid token": { + verifier: fakeStackVerifier{err: errors.New("bad token")}, + status: http.StatusUnauthorized, + }, + "wrong team": { + verifier: fakeStackVerifier{claims: stackauth.Claims{ + ProjectID: "project", SelectedTeamID: "other-team", + }}, + status: http.StatusForbidden, + }, + } + for name, tc := range cases { t.Run(name, func(t *testing.T) { handler := (&MultiTenant{ - Base: base, Registry: registry, StackVerifier: verifier, + Base: base, Registry: registry, StackVerifier: tc.verifier, StackTeams: fakeStackTeams{}, StackTenantKeySecret: []byte("0123456789abcdef0123456789abcdef"), }).Handler(base.Handler()) @@ - if response.Code != http.StatusUnauthorized && response.Code != http.StatusForbidden { - t.Fatalf("status = %d", response.Code) + if response.Code != tc.status { + t.Fatalf("status = %d, want %d", response.Code, tc.status) }🤖 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 `@internal/proxy/multitenant_test.go` around lines 470 - 492, Update the table-driven cases around MultiTenant.Handler to include an expected HTTP status for each verifier scenario: 401 Unauthorized for “invalid token” and 403 Forbidden for “wrong team.” Replace the combined status condition with an exact comparison against the case’s expected status.internal/stackauth/verifier_test.go (2)
76-96: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winExtend the rejection table to the remaining security branches.
The table covers anonymous, wrong project, and missing team. It does not cover expired tokens, a wrong
iss, a wrongaud, oris_restricted. These branches guard against replay and cross-project token use. Add cases so a later change cannot silently remove them.💚 Proposed additional cases
for name, overrides := range map[string]map[string]any{ "anonymous": {"is_anonymous": true}, "wrong project": {"project_id": "project-2"}, "missing team": {"selected_team_id": ""}, + "restricted": {"is_restricted": true}, + "expired": {"exp": now.Add(-time.Hour).Unix()}, + "wrong issuer": {"iss": "https://attacker.example/projects/project-1"}, + "wrong audience": {"aud": []string{"project-2"}}, + "wrong role": {"role": "anon"}, } {A separate test is also needed for a header whose
algis notES256, becausesignedTokenalways writesES256. As per coding guidelines, rungo test ./...after the change.🤖 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 `@internal/stackauth/verifier_test.go` around lines 76 - 96, Extend the rejection table in the verifier test around signedToken and verifier.Verify with cases for an expired token, incorrect iss, incorrect aud, and is_restricted=true, preserving the existing rejection assertion. Add a separate test that constructs a token header using an algorithm other than ES256 and verifies it is rejected, since signedToken always emits ES256. Run go test ./... to validate the changes.Source: Coding guidelines
130-132: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRemove the zero-time assertion.
This assertion checks how the standard library formats the zero
time.Time. It does not verify any behavior of thestackauthpackage. Assert onExpiresAtproduced byVerifyinstead, or delete the block.♻️ Proposed removal
- if !strings.Contains((Claims{}).ExpiresAt.String(), "0001") { - t.Fatal("keep Claims.ExpiresAt usable") - }Remove the now-unused
stringsimport.🤖 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 `@internal/stackauth/verifier_test.go` around lines 130 - 132, Remove the zero-time formatting assertion involving Claims.ExpiresAt.String() from the test, along with the now-unused strings import. Keep assertions focused on ExpiresAt values produced by Verify, or delete this assertion block if no package behavior is being tested.internal/proxy/multitenant.go (2)
296-309: 🩺 Stability & Availability | 🔵 TrivialAdd rate limiting to the Stack exchange endpoint.
Any unauthenticated caller reaches this handler. Each request drives JWT verification, a possible JWKS fetch, a possible Stack
ListTeamscall, registry file locking, and a disk write. Add a per-IP limit in front of the route, and add a metric for rejected exchanges so abuse is 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 `@internal/proxy/multitenant.go` around lines 296 - 309, Add per-IP rate limiting before the handleStackAuth request processing begins, covering unauthenticated callers before JWT verification or downstream Stack operations. When requests exceed the limit, reject them and increment a dedicated metric for rejected Stack exchanges; preserve the existing method, configuration, and token validation behavior for allowed requests.
400-403: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUse
server.MaxBodyBytesfor the upload limit.Line 400 hardcodes 1 MiB while the surrounding
Servercarries a configuredMaxBodyBytes. The two limits diverge, so operators cannot lower this route. Fall back to 1 MiB only whenMaxBodyBytesis zero.♻️ Proposed change
+ limit := server.MaxBodyBytes + if limit <= 0 { + limit = 1 << 20 + } var input tenantAccountUpload - if err := json.NewDecoder(io.LimitReader(r.Body, 1<<20)).Decode(&input); err != nil { + if err := json.NewDecoder(io.LimitReader(r.Body, limit)).Decode(&input); err != nil { http.Error(w, "invalid JSON body", http.StatusBadRequest) return }🤖 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 `@internal/proxy/multitenant.go` around lines 400 - 403, Update the JSON decoding path around the Server’s request handler to use server.MaxBodyBytes as the io.LimitReader limit, falling back to 1 MiB only when MaxBodyBytes is zero. Preserve the existing invalid JSON response and early return behavior.internal/tenant/tenant_test.go (1)
123-132: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winAssert determinism and namespace separation for
DeriveKey.The test verifies rejection paths only.
DeriveKeymust return the same key for the same inputs and a different key for a different namespace. The second property keeps one Stack project from deriving another project's tenant key for an identical team ID.💚 Proposed additions
for _, id := range []string{"", ".", "..", "../team", "team/other", "team other"} { if ValidExternalID(id) { t.Fatalf("invalid external ID %q accepted", id) } } + if ValidExternalID(`team\other`) || ValidExternalID(strings.Repeat("a", 129)) { + t.Fatal("backslash or oversized external ID accepted") + } + secret := []byte("0123456789abcdef0123456789abcdef") + first, err := DeriveKey(secret, "project-a", "team-1") + if err != nil { + t.Fatal(err) + } + again, err := DeriveKey(secret, "project-a", "team-1") + if err != nil { + t.Fatal(err) + } + other, err := DeriveKey(secret, "project-b", "team-1") + if err != nil { + t.Fatal(err) + } + if first != again { + t.Fatalf("derivation is not deterministic: %s != %s", first, again) + } + if first == other { + t.Fatal("namespace does not separate derived keys") + } + if !ValidKeyFormat(first) { + t.Fatalf("derived key %q has an invalid format", first) + }Add the
stringsimport.🤖 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 `@internal/tenant/tenant_test.go` around lines 123 - 132, Extend TestExternalTenantRejectsTraversalAndWeakSecret to assert DeriveKey determinism by deriving the same valid secret, stack, and team inputs twice and comparing the results, then assert namespace separation by deriving with a different stack or project namespace and confirming the key differs. Add the strings import if needed by the test implementation, while preserving the existing rejection checks.internal/stackauth/verifier.go (2)
141-148: 🚀 Performance & Scalability | 🔵 Trivial | 🏗️ Heavy liftDo not hold
v.muacross the JWKS HTTP request.
keyholdsv.muwhilefetchKeysperforms the network call. The default client allows 15 seconds, so one slow JWKS response blocks every concurrentVerifycall on this verifier. Fetch outside the mutex, then re-acquire it to publish the keys, and coalesce concurrent fetches so only one request goes out.🤖 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 `@internal/stackauth/verifier.go` around lines 141 - 148, Update the verifier flow around fetchKeys to avoid holding v.mu during the JWKS HTTP request: release the mutex before fetching, then re-acquire it to publish the refreshed keys. Add fetch coalescing so concurrent Verify calls wait for and reuse a single in-flight JWKS request, while preserving the existing cache-hit and force-refresh behavior.
196-204: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winReplace the deprecated
IsOnCurvecall.
key.Curve.IsOnCurveis deprecated and triggersSA1019linting. Use thecrypto/ecdhP-256 API to validate the SEC1 uncompressed point before storing the ECDSA public key.🤖 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 `@internal/stackauth/verifier.go` around lines 196 - 204, Replace the deprecated key.Curve.IsOnCurve validation in the verifier’s JWK key-processing block with crypto/ecdh P-256 validation of the SEC1 uncompressed point, and only store the ECDSA key in keys[item.KeyID] when that validation succeeds. Preserve the existing behavior of skipping invalid coordinates and retain the ECDSA public-key construction for valid points.Source: Linters/SAST tools
cmd/subrouter/sr.go (1)
68-86: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winClarify
sr remoteversussr serverin the help text.
sr remote -vis accepted, but the help documents both baresr remotecommands andsr servercommands separately. Add one line markingsr serveras the legacy Form, or mark it deprecated, so users do not treat them as independent remote commands.🤖 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.go` around lines 68 - 86, The help text in the subrouter command usage should clarify that sr server is the legacy form of sr remote, rather than a separate command family. Update the usage section near the sr remote entries to add a concise legacy or deprecated marker for sr server, while preserving the existing remote command descriptions.
🤖 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/sr_cloud.go`:
- Around line 197-204: Update matchNativeStackTeam so an empty selector does not
choose teams[0] when multiple teams are available. Return an error listing the
available teams and prompting the user to select one; preserve the existing
no-teams error and allow the sole team to be selected automatically.
- Around line 414-426: In the logout flow surrounding clearDefaultServer, clear
the default server before printing the success message. If clearDefaultServer
fails, return an error that explicitly indicates logout is incomplete and
cleanup is still required; only print “Logged out…” after both configuration
persistence and remote cleanup succeed.
In `@cmd/subrouter/sr_server.go`:
- Around line 86-99: Update srRemoteHelp to document the supported remote rename
operation by adding a “rename <old> <new>” usage entry alongside the existing
add and remove entries; leave the other help text unchanged.
- Around line 481-494: The cmux branch in serverUse can leave Codex targeting
cmux with stale broker configuration when broker.LoadConfig or broker.SaveConfig
fails. Reorder the operations so hosted broker configuration is successfully
persisted before committing the cmux selection, or restore the previous
selection when either broker operation fails; add regression coverage for both
failure paths.
In `@internal/agents/claude/store.go`:
- Around line 711-740: Update UpsertCredentialProfile to capture and return the
error from FindProfile(name) after RegisterProfile succeeds, returning the
retrieved profile only when the lookup succeeds instead of discarding the error
and allowing an empty profile with nil error.
- Around line 429-461: Update migrateDirectoryToShared so symlink targets from
os.Readlink are joined with filepath.Dir(source) only when relative; preserve
absolute targets before comparing normalized absolute paths. Add a regression
test that invokes migration twice and verifies the second call succeeds for the
existing symlink.
In `@internal/broker/client.go`:
- Around line 197-224: The hosted account loading path in the HostedReady branch
of the client must preserve account health status. Extend the hosted accounts
response handling to obtain each account’s status via the existing account/usage
status endpoint or response fields, decode it into SharedAccount.Health, and
ensure sr status and sr account list can report NEEDS REPAIR for unhealthy
accounts.
In `@internal/proxy/multitenant.go`:
- Around line 473-485: Add the same nil guard used by handleTenantAccountUpload
to handleTenantAccountDelete before accessing server.AccountRef.store. Return
HTTP 503 and stop processing when AccountRef is unavailable; otherwise preserve
the existing account-removal flow.
- Around line 373-378: In the tenant creation response flow around writeJSON,
validate that m.PublicURL is configured before constructing proxyURL, returning
a clear client error consistent with the existing missing-secret check. Keep the
trimmed URL construction for valid values, and set the response Cache-Control
header to no-store before writing the body so the tenantKey is not cached.
In `@internal/stackauth/client_test.go`:
- Around line 33-39: Replace handler-goroutine t.Fatal/t.Fatalf assertions with
captured request values and explicit HTTP error responses, then assert after the
client call returns: in internal/stackauth/client_test.go lines 33-39, record
grant_type and client_id for /auth/oauth/token and validate them after
client.Refresh; in internal/broker/hosted_client_test.go lines 16-18, record
r.URL.Path, return an explicit error response, and assert paths after the client
calls; in cmd/subrouter/sr_hosted_login_test.go lines 56-59, record the
Authorization header for /_subrouter/auth/stack and assert it after
runner.cloudLogin returns.
In `@internal/stackauth/verifier.go`:
- Around line 68-74: Throttle forced JWKS refreshes in the verifier flow around
v.key and verifyES256 by enforcing a minimum interval between refresh attempts,
including concurrent requests. Preserve the existing key lookup and retry
behavior, but reuse the last-refresh timestamp or equivalent shared state so
repeated invalid signatures do not trigger an upstream request on every token.
In `@internal/tenant/tenant.go`:
- Around line 137-152: Update ValidExternalID and the tenant validation flow to
reject external IDs that collide under case folding, preserving existing IDs and
derived keys. Ensure case-insensitive duplicates such as team-A and team-a
cannot be accepted together, while retaining the current allowed-character and
length checks.
---
Outside diff comments:
In `@cmd/subrouter/sr_claude_upload.go`:
- Around line 43-51: Update the error message in the
CredentialSourceTeam/CredentialSourceHosted branch to name the matched
credential source, using “team storage” for CredentialSourceTeam and “hosted
cmux” for CredentialSourceHosted, consistent with the wording in sr.go. Preserve
the existing command and error behavior.
In `@cmd/subrouter/sr_cloud.go`:
- Around line 553-580: Update the credential-source handling around
parseCredentialSource and broker.SaveConfig to treat CredentialSourceHosted as
requiring hosted readiness, using the same HostedReady validation as cloudSetup
before persisting configuration. Remove or replace the now-unreachable
CredentialSourceTeam checks in the load-error and readiness branches, ensuring
sr storage hosted fails before SaveConfig when HostedURL or TenantKey is
missing.
- Around line 439-465: Restructure the team command switch so the current branch
reads local TeamID and TeamName without refreshing or listing teams, allowing
offline use. Move session refresh and ListTeams into the list/ls branch,
preserving token updates and config persistence there. Remove the duplicated
current case and ensure the default branch also saves the updated config before
returning.
In `@cmd/subrouter/sr_setup.go`:
- Around line 262-281: Update the purge cleanup flow around
cloudConfig.StackProjectID and cloudConfig.StackPublishable to warn when either
value is empty and no remote sign-out is attempted. Reuse the same warning
message and logging approach as cloudLogout, while preserving the existing
SignOut and logoutErr handling for complete native Stack Auth configurations.
---
Nitpick comments:
In `@cmd/subrouter/main.go`:
- Line 219: Update the help text and configuration guidance for
stackTenantKeySecret to identify SUBROUTER_STACK_TENANT_KEY_SECRET as the
supported deployment configuration, while retaining --stack-tenant-key-secret
for local development. Preserve the existing environment-variable fallback
behavior and align the wording with the established --admin-token pattern.
- Around line 240-270: Validate the resolved *publicURL value during startup
using the same HTTPS-origin rules required by broker.Config.Validate, rejecting
paths, queries, fragments, and other malformed values before serving requests.
Update the hosted Stack login configuration error to name both the corresponding
CLI flags and environment variables, covering stackLoginValues and its
partial-configuration check.
In `@cmd/subrouter/sr_claude.go`:
- Around line 218-222: Rename the cloudClaudeEnvironment parameters local and
localProxyToken to baseURL and proxyToken, and update all references within the
function and its callers as needed, including proxyClaudeTo, without changing
behavior.
- Around line 71-80: The hosted remote validation is duplicated between the
shown Claude routing block and routeClaudeProfileThroughHosted. Extract a shared
helper that calls selectedRemoteServer, requires ok, server.Name == "cmux", and
a non-empty TenantKey, returning the validated server or the appropriate error;
update both callers to use it while preserving their existing proxy/profile
behavior.
In `@cmd/subrouter/sr_cloud.go`:
- Around line 86-116: Update the polling loop around stackClient.PollCLI to
tolerate transient poll errors instead of immediately returning them. Retry
failed polls with backoff while respecting the existing deadline and context
cancellation, but return terminal errors immediately; preserve the current
success, expired/used, unexpected-status, and refresh-token validation behavior.
In `@cmd/subrouter/sr_hosted_login_test.go`:
- Around line 162-164: Extend the assertion in the hostedCodexAdd upload test to
validate the nested uploaded["tokens"] map, including the expected accessToken,
refreshToken, idToken, and accountID keys and values. Keep the existing provider
and label checks, and fail the test when any token field is missing, misspelled,
or incorrect.
In `@cmd/subrouter/sr.go`:
- Around line 68-86: The help text in the subrouter command usage should clarify
that sr server is the legacy form of sr remote, rather than a separate command
family. Update the usage section near the sr remote entries to add a concise
legacy or deprecated marker for sr server, while preserving the existing remote
command descriptions.
In `@internal/agents/claude/store.go`:
- Around line 380-427: Use a consistent trimmed emptiness check for
SharedStateDir in initInstanceDir and prepareSharedState. Update
initInstanceDir’s high-growth directory condition to treat whitespace-only
values as empty, or extract and reuse a helper so both methods apply identical
behavior.
In `@internal/broker/client.go`:
- Around line 328-330: Update the non-2xx response handling in the hosted
Subrouter request flow to extract the server’s error field from data and include
only that decoded message in the returned error, matching the existing
apiErrorFromResponse behavior used by doJSON. Preserve the status code/status
text fallback when the body has no usable error field, and never print the
entire response body.
In `@internal/broker/config.go`:
- Around line 115-170: Update Config.Validate to assign c.Normalized() once and
reuse that normalized value for BaseURL, CredentialSource, TenantKey, and
HostedURL validation instead of calling it repeatedly. Replace the inline
base-URL loopback detection with isLoopbackHost(baseURL.Hostname()), preserving
the existing localhost and IP loopback behavior, and remove the net import if no
longer used.
In `@internal/proxy/multitenant_test.go`:
- Around line 497-509: Extend
TestStackLoginAcceptsAnotherTeamAfterMembershipCheck with cases covering
handleStackAuth’s 503 responses: one using fakeStackTeams with its err field
set, and another with StackTeams left nil. Assert each request returns HTTP 503
while preserving the existing successful membership-check coverage.
- Around line 460-464: Add a DELETE request after the existing account list
assertion in the tenant account test, targeting
/t/<key>/_subrouter/accounts/<id> and asserting successful deletion; also add a
request for a nonexistent account ID to cover handleTenantAccountDelete’s 404
branch. Run go test ./... after updating the test.
- Around line 470-492: Update the table-driven cases around MultiTenant.Handler
to include an expected HTTP status for each verifier scenario: 401 Unauthorized
for “invalid token” and 403 Forbidden for “wrong team.” Replace the combined
status condition with an exact comparison against the case’s expected status.
In `@internal/proxy/multitenant.go`:
- Around line 296-309: Add per-IP rate limiting before the handleStackAuth
request processing begins, covering unauthenticated callers before JWT
verification or downstream Stack operations. When requests exceed the limit,
reject them and increment a dedicated metric for rejected Stack exchanges;
preserve the existing method, configuration, and token validation behavior for
allowed requests.
- Around line 400-403: Update the JSON decoding path around the Server’s request
handler to use server.MaxBodyBytes as the io.LimitReader limit, falling back to
1 MiB only when MaxBodyBytes is zero. Preserve the existing invalid JSON
response and early return behavior.
In `@internal/stackauth/client.go`:
- Around line 407-422: Unify loopback detection by exporting the existing
predicate behind isLoopbackHost in internal/broker/config.go and reusing it from
validateHTTPSOriginOrURL. Replace the hard-coded localhost, 127.0.0.1, and ::1
checks with the shared net.ParseIP-based predicate, preserving HTTPS
requirements and HTTP acceptance for all loopback addresses.
In `@internal/stackauth/verifier_test.go`:
- Around line 76-96: Extend the rejection table in the verifier test around
signedToken and verifier.Verify with cases for an expired token, incorrect iss,
incorrect aud, and is_restricted=true, preserving the existing rejection
assertion. Add a separate test that constructs a token header using an algorithm
other than ES256 and verifies it is rejected, since signedToken always emits
ES256. Run go test ./... to validate the changes.
- Around line 130-132: Remove the zero-time formatting assertion involving
Claims.ExpiresAt.String() from the test, along with the now-unused strings
import. Keep assertions focused on ExpiresAt values produced by Verify, or
delete this assertion block if no package behavior is being tested.
In `@internal/stackauth/verifier.go`:
- Around line 141-148: Update the verifier flow around fetchKeys to avoid
holding v.mu during the JWKS HTTP request: release the mutex before fetching,
then re-acquire it to publish the refreshed keys. Add fetch coalescing so
concurrent Verify calls wait for and reuse a single in-flight JWKS request,
while preserving the existing cache-hit and force-refresh behavior.
- Around line 196-204: Replace the deprecated key.Curve.IsOnCurve validation in
the verifier’s JWK key-processing block with crypto/ecdh P-256 validation of the
SEC1 uncompressed point, and only store the ECDSA key in keys[item.KeyID] when
that validation succeeds. Preserve the existing behavior of skipping invalid
coordinates and retain the ECDSA public-key construction for valid points.
In `@internal/tenant/tenant_test.go`:
- Around line 123-132: Extend TestExternalTenantRejectsTraversalAndWeakSecret to
assert DeriveKey determinism by deriving the same valid secret, stack, and team
inputs twice and comparing the results, then assert namespace separation by
deriving with a different stack or project namespace and confirming the key
differs. Add the strings import if needed by the test implementation, while
preserving the existing rejection checks.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: dc9ad266-a816-4995-bb24-a9066c72295a
📒 Files selected for processing (25)
cmd/subrouter/main.gocmd/subrouter/main_test.gocmd/subrouter/sr.gocmd/subrouter/sr_claude.gocmd/subrouter/sr_claude_upload.gocmd/subrouter/sr_cloud.gocmd/subrouter/sr_hosted_login_test.gocmd/subrouter/sr_server.gocmd/subrouter/sr_server_test.gocmd/subrouter/sr_setup.gointernal/agents/claude/shared_state_test.gointernal/agents/claude/store.gointernal/agents/claude/store_upsert_test.gointernal/broker/client.gointernal/broker/client_test.gointernal/broker/config.gointernal/broker/hosted_client_test.gointernal/proxy/multitenant.gointernal/proxy/multitenant_test.gointernal/stackauth/client.gointernal/stackauth/client_test.gointernal/stackauth/verifier.gointernal/stackauth/verifier_test.gointernal/tenant/tenant.gointernal/tenant/tenant_test.go
💤 Files with no reviewable changes (1)
- internal/broker/client_test.go
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (3)
cmd/subrouter/main_test.go (1)
66-85: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winCover the newly accepted loopback forms.
This PR widened loopback acceptance from
localhostto any loopback IP. The valid set covers onlyhttp://127.0.0.1. Add an IPv6 loopback case and a fragment rejection case so the new branch and the fragment rule stay covered.♻️ Proposed additions
"http://127.0.0.1:31415", + "http://[::1]:31415", + "http://localhost:31415", } {"http://sr.example.com", + "https://sr.example.com#fragment", } {🤖 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_test.go` around lines 66 - 85, Extend the table-driven cases in the validation test around validatePublicSubrouterURL: add a valid IPv6 loopback URL (such as http://[::1]:31415) and add a URL containing a fragment to the invalid cases. Keep the existing coverage and assertion structure unchanged.cmd/subrouter/sr_onboarding_review_test.go (1)
235-262: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winThe helper silently discards most override fields.
saveHostedTestConfigaccepts a fullbroker.Configbut copies onlyTeamID,TeamName,StackAPIURL, andCredentialSource. A future test that overridesHostedURL,TenantKey, orAccessTokengets the default hosted value instead, and the test still passes for the wrong reason. Narrow the parameter to the fields the helper supports, or merge every non-empty field.♻️ Proposed change
-func saveHostedTestConfig(t *testing.T, path string, override broker.Config) { +// hostedTestOverride names the fields saveHostedTestConfig applies. +type hostedTestOverride struct { + TeamID string + TeamName string + StackAPIURL string + CredentialSource broker.CredentialSource +} + +func saveHostedTestConfig(t *testing.T, path string, override hostedTestOverride) { t.Helper()🤖 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_onboarding_review_test.go` around lines 235 - 262, Update saveHostedTestConfig so its override behavior is complete: either narrow the parameter to only the supported fields, or merge every applicable non-empty broker.Config field, including HostedURL, TenantKey, AccessToken, and the remaining credentials and hosted settings. Ensure callers cannot silently provide overrides that are ignored.internal/stackauth/verifier.go (1)
192-214: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winKeep the shared JWKS fetch independent of the initiator.
fetchKeysuses the caller’sctxforhttp.NewRequestWithContext, so a cancelled request can returncontext.Canceledto all callers waiting onv.fetchDone. DetachfetchKeysfrom the initiating context, or otherwise ensure a cancelled fetch does not cancel waiters or populatev.lastFetchErr. Also preserve the reset after a failed signature-triggered refresh so it is not treated as a completed forced fetch.🤖 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 `@internal/stackauth/verifier.go` around lines 192 - 214, Update the shared fetch flow around fetchKeys so the JWKS request is detached from the initiating caller’s context, preventing cancellation from propagating to waiters or being stored in lastFetchErr. Preserve the failed signature-triggered refresh reset behavior, including clearing the forced-fetch state after a failed forced refresh rather than treating it as completed.
🤖 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/sr_onboarding_review_test.go`:
- Around line 143-162: Update
TestServerUseCMUXLeavesSelectionUntouchedWhenBrokerSaveFails to skip when
running as root in addition to the existing Windows skip. Check the current user
ID before creating the permission-restricted directory, and retain the test’s
existing behavior for non-root POSIX environments.
In `@cmd/subrouter/sr_server.go`:
- Around line 480-503: Update the hosted configuration setup around
previousHosted and rollbackHosted to record whether hostedConfigPath existed
before LoadConfig. When rollbackHosted runs after a successful save, remove the
configuration file if it was originally absent; otherwise restore previousHosted
with SaveConfig. Preserve the existing error joining behavior and ensure
file-removal errors are returned alongside the original cause.
In `@internal/proxy/proxy.go`:
- Around line 1030-1039: Update the accounts-listing handler around the
healthByAccount construction to avoid calling AccountRef.Statuses, since it
refreshes providers and persists credentials. Use the existing cached health
data, populated by a background refresh mechanism, and keep the GET path
read-only without upstream calls or credential writes.
In `@internal/stackauth/client.go`:
- Around line 119-122: Update the network-error check around the net.Error
handling to remove the deprecated networkErr.Temporary() call. Preserve timeout
detection via networkErr.Timeout(), and explicitly recognize connection-level
retryable errors such as syscall.ECONNRESET and syscall.ECONNREFUSED, adding the
syscall import if needed.
---
Nitpick comments:
In `@cmd/subrouter/main_test.go`:
- Around line 66-85: Extend the table-driven cases in the validation test around
validatePublicSubrouterURL: add a valid IPv6 loopback URL (such as
http://[::1]:31415) and add a URL containing a fragment to the invalid cases.
Keep the existing coverage and assertion structure unchanged.
In `@cmd/subrouter/sr_onboarding_review_test.go`:
- Around line 235-262: Update saveHostedTestConfig so its override behavior is
complete: either narrow the parameter to only the supported fields, or merge
every applicable non-empty broker.Config field, including HostedURL, TenantKey,
AccessToken, and the remaining credentials and hosted settings. Ensure callers
cannot silently provide overrides that are ignored.
In `@internal/stackauth/verifier.go`:
- Around line 192-214: Update the shared fetch flow around fetchKeys so the JWKS
request is detached from the initiating caller’s context, preventing
cancellation from propagating to waiters or being stored in lastFetchErr.
Preserve the failed signature-triggered refresh reset behavior, including
clearing the forced-fetch state after a failed forced refresh rather than
treating it as completed.
🪄 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: d654d542-10ea-46d2-859e-a25d1c17583c
📒 Files selected for processing (25)
cmd/subrouter/main.gocmd/subrouter/main_test.gocmd/subrouter/sr.gocmd/subrouter/sr_claude.gocmd/subrouter/sr_claude_upload.gocmd/subrouter/sr_cloud.gocmd/subrouter/sr_hosted_login_test.gocmd/subrouter/sr_onboarding_review_test.gocmd/subrouter/sr_server.gocmd/subrouter/sr_setup.gocmd/subrouter/sr_setup_test.gointernal/agents/claude/shared_state_test.gointernal/agents/claude/store.gointernal/broker/client.gointernal/broker/config.gointernal/broker/hosted_client_test.gointernal/proxy/multitenant.gointernal/proxy/multitenant_test.gointernal/proxy/proxy.gointernal/stackauth/client.gointernal/stackauth/client_test.gointernal/stackauth/verifier.gointernal/stackauth/verifier_test.gointernal/tenant/tenant.gointernal/tenant/tenant_test.go
🚧 Files skipped from review as they are similar to previous changes (14)
- cmd/subrouter/sr_claude_upload.go
- internal/broker/hosted_client_test.go
- internal/tenant/tenant_test.go
- cmd/subrouter/sr_setup.go
- internal/agents/claude/store.go
- internal/proxy/multitenant_test.go
- internal/agents/claude/shared_state_test.go
- cmd/subrouter/sr.go
- internal/proxy/multitenant.go
- cmd/subrouter/main.go
- internal/broker/client.go
- cmd/subrouter/sr_hosted_login_test.go
- cmd/subrouter/sr_cloud.go
- internal/broker/config.go
| func TestServerUseCMUXLeavesSelectionUntouchedWhenBrokerSaveFails(t *testing.T) { | ||
| if runtime.GOOS == "windows" { | ||
| t.Skip("POSIX directory permissions required") | ||
| } | ||
| root := t.TempDir() | ||
| t.Setenv("HOME", root) | ||
| t.Setenv("CODEX_HOME", filepath.Join(root, "codex")) | ||
| configDir := filepath.Join(root, "readonly") | ||
| configPath := filepath.Join(configDir, "cloud.json") | ||
| t.Setenv("SUBROUTER_CLOUD_CONFIG", configPath) | ||
| if err := os.MkdirAll(configDir, 0o700); err != nil { | ||
| t.Fatal(err) | ||
| } | ||
| saveHostedTestConfig(t, configPath, broker.Config{ | ||
| CredentialSource: broker.CredentialSourceLocal, | ||
| }) | ||
| if err := os.Chmod(configDir, 0o500); err != nil { | ||
| t.Fatal(err) | ||
| } | ||
| t.Cleanup(func() { _ = os.Chmod(configDir, 0o700) }) |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
The permission-based failure does not hold when the test runs as root.
os.Chmod(configDir, 0o500) makes broker.SaveConfig fail because os.CreateTemp cannot create the temporary file. Root ignores the write bit, so SaveConfig succeeds, serverUse returns nil, and the test fails at Line 177. Many CI containers run tests as root. Skip the test for uid 0 in addition to the Windows skip.
💚 Proposed fix
if runtime.GOOS == "windows" {
t.Skip("POSIX directory permissions required")
}
+ if os.Geteuid() == 0 {
+ t.Skip("root ignores directory write permissions")
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| func TestServerUseCMUXLeavesSelectionUntouchedWhenBrokerSaveFails(t *testing.T) { | |
| if runtime.GOOS == "windows" { | |
| t.Skip("POSIX directory permissions required") | |
| } | |
| root := t.TempDir() | |
| t.Setenv("HOME", root) | |
| t.Setenv("CODEX_HOME", filepath.Join(root, "codex")) | |
| configDir := filepath.Join(root, "readonly") | |
| configPath := filepath.Join(configDir, "cloud.json") | |
| t.Setenv("SUBROUTER_CLOUD_CONFIG", configPath) | |
| if err := os.MkdirAll(configDir, 0o700); err != nil { | |
| t.Fatal(err) | |
| } | |
| saveHostedTestConfig(t, configPath, broker.Config{ | |
| CredentialSource: broker.CredentialSourceLocal, | |
| }) | |
| if err := os.Chmod(configDir, 0o500); err != nil { | |
| t.Fatal(err) | |
| } | |
| t.Cleanup(func() { _ = os.Chmod(configDir, 0o700) }) | |
| func TestServerUseCMUXLeavesSelectionUntouchedWhenBrokerSaveFails(t *testing.T) { | |
| if runtime.GOOS == "windows" { | |
| t.Skip("POSIX directory permissions required") | |
| } | |
| if os.Geteuid() == 0 { | |
| t.Skip("root ignores directory write permissions") | |
| } | |
| root := t.TempDir() | |
| t.Setenv("HOME", root) | |
| t.Setenv("CODEX_HOME", filepath.Join(root, "codex")) | |
| configDir := filepath.Join(root, "readonly") | |
| configPath := filepath.Join(configDir, "cloud.json") | |
| t.Setenv("SUBROUTER_CLOUD_CONFIG", configPath) | |
| if err := os.MkdirAll(configDir, 0o700); err != nil { | |
| t.Fatal(err) | |
| } | |
| saveHostedTestConfig(t, configPath, broker.Config{ | |
| CredentialSource: broker.CredentialSourceLocal, | |
| }) | |
| if err := os.Chmod(configDir, 0o500); err != nil { | |
| t.Fatal(err) | |
| } | |
| t.Cleanup(func() { _ = os.Chmod(configDir, 0o700) }) |
🤖 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_onboarding_review_test.go` around lines 143 - 162, Update
TestServerUseCMUXLeavesSelectionUntouchedWhenBrokerSaveFails to skip when
running as root in addition to the existing Windows skip. Check the current user
ID before creating the permission-restricted directory, and retain the test’s
existing behavior for non-root POSIX environments.
| previousHosted, err = broker.LoadConfig(hostedConfigPath) | ||
| if err != nil { | ||
| return err | ||
| } | ||
| nextHosted := previousHosted | ||
| nextHosted.CredentialSource = broker.CredentialSourceHosted | ||
| nextHosted.HostedURL = server.URL | ||
| nextHosted.TenantKey = server.TenantKey | ||
| if !nextHosted.HostedReady() { | ||
| return fmt.Errorf("cmux hosted requires login and a selected team; run 'sr login'") | ||
| } | ||
| if err := broker.SaveConfig(hostedConfigPath, nextHosted); err != nil { | ||
| return err | ||
| } | ||
| hostedSaved = true | ||
| } | ||
| rollbackHosted := func(cause error) error { | ||
| if !hostedSaved { | ||
| return cause | ||
| } | ||
| if rollbackErr := broker.SaveConfig(hostedConfigPath, previousHosted); rollbackErr != nil { | ||
| return errors.Join(cause, fmt.Errorf("restore hosted configuration: %w", rollbackErr)) | ||
| } | ||
| return cause |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
Rollback can create a hosted config file that did not exist before.
broker.LoadConfig returns Config{Version: 1, BaseURL: DefaultBaseURL} when the file is absent, so previousHosted holds a synthesized default rather than a prior on-disk state. If store.save or writeCodexConfigForServer then fails, rollbackHosted calls SaveConfig with that default. SaveConfig generates a new LocalProxyToken and writes the file. The failure path therefore leaves a new credential file where none existed.
In practice HostedReady() at Line 488 requires a populated config, so this is reachable only when the file existed and was later removed. Record whether the file existed, and remove it during rollback instead of rewriting it.
🛡️ Proposed fix
var (
hostedConfigPath string
previousHosted broker.Config
hostedSaved bool
+ hostedExisted bool
)
if server.Name == "cmux" && strings.TrimSpace(server.TenantKey) != "" {
hostedConfigPath, err = broker.DefaultConfigPath()
if err != nil {
return err
}
+ if _, statErr := os.Stat(hostedConfigPath); statErr == nil {
+ hostedExisted = true
+ } else if !errors.Is(statErr, os.ErrNotExist) {
+ return statErr
+ }
previousHosted, err = broker.LoadConfig(hostedConfigPath) rollbackHosted := func(cause error) error {
if !hostedSaved {
return cause
}
+ if !hostedExisted {
+ if removeErr := os.Remove(hostedConfigPath); removeErr != nil &&
+ !errors.Is(removeErr, os.ErrNotExist) {
+ return errors.Join(cause, fmt.Errorf("remove hosted configuration: %w", removeErr))
+ }
+ return cause
+ }
if rollbackErr := broker.SaveConfig(hostedConfigPath, previousHosted); rollbackErr != nil {📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| previousHosted, err = broker.LoadConfig(hostedConfigPath) | |
| if err != nil { | |
| return err | |
| } | |
| nextHosted := previousHosted | |
| nextHosted.CredentialSource = broker.CredentialSourceHosted | |
| nextHosted.HostedURL = server.URL | |
| nextHosted.TenantKey = server.TenantKey | |
| if !nextHosted.HostedReady() { | |
| return fmt.Errorf("cmux hosted requires login and a selected team; run 'sr login'") | |
| } | |
| if err := broker.SaveConfig(hostedConfigPath, nextHosted); err != nil { | |
| return err | |
| } | |
| hostedSaved = true | |
| } | |
| rollbackHosted := func(cause error) error { | |
| if !hostedSaved { | |
| return cause | |
| } | |
| if rollbackErr := broker.SaveConfig(hostedConfigPath, previousHosted); rollbackErr != nil { | |
| return errors.Join(cause, fmt.Errorf("restore hosted configuration: %w", rollbackErr)) | |
| } | |
| return cause | |
| var ( | |
| hostedConfigPath string | |
| previousHosted broker.Config | |
| hostedSaved bool | |
| hostedExisted bool | |
| ) | |
| if server.Name == "cmux" && strings.TrimSpace(server.TenantKey) != "" { | |
| hostedConfigPath, err = broker.DefaultConfigPath() | |
| if err != nil { | |
| return err | |
| } | |
| if _, statErr := os.Stat(hostedConfigPath); statErr == nil { | |
| hostedExisted = true | |
| } else if !errors.Is(statErr, os.ErrNotExist) { | |
| return statErr | |
| } | |
| previousHosted, err = broker.LoadConfig(hostedConfigPath) | |
| if err != nil { | |
| return err | |
| } | |
| nextHosted := previousHosted | |
| nextHosted.CredentialSource = broker.CredentialSourceHosted | |
| nextHosted.HostedURL = server.URL | |
| nextHosted.TenantKey = server.TenantKey | |
| if !nextHosted.HostedReady() { | |
| return fmt.Errorf("cmux hosted requires login and a selected team; run 'sr login'") | |
| } | |
| if err := broker.SaveConfig(hostedConfigPath, nextHosted); err != nil { | |
| return err | |
| } | |
| hostedSaved = true | |
| } | |
| rollbackHosted := func(cause error) error { | |
| if !hostedSaved { | |
| return cause | |
| } | |
| if !hostedExisted { | |
| if removeErr := os.Remove(hostedConfigPath); removeErr != nil && | |
| !errors.Is(removeErr, os.ErrNotExist) { | |
| return errors.Join(cause, fmt.Errorf("remove hosted configuration: %w", removeErr)) | |
| } | |
| return cause | |
| } | |
| if rollbackErr := broker.SaveConfig(hostedConfigPath, previousHosted); rollbackErr != nil { | |
| return errors.Join(cause, fmt.Errorf("restore hosted configuration: %w", rollbackErr)) | |
| } | |
| return cause |
🤖 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 480 - 503, Update the hosted
configuration setup around previousHosted and rollbackHosted to record whether
hostedConfigPath existed before LoadConfig. When rollbackHosted runs after a
successful save, remove the configuration file if it was originally absent;
otherwise restore previousHosted with SaveConfig. Preserve the existing error
joining behavior and ensure file-removal errors are returned alongside the
original cause.
| healthByAccount := map[string]*safeHealth{} | ||
| if s.AccountRef != nil { | ||
| for _, status := range s.AccountRef.Statuses(r.Context(), false) { | ||
| if !status.AuthChecked { | ||
| continue | ||
| } | ||
| healthByAccount[string(status.Provider)+"\x00"+status.ID] = &safeHealth{ | ||
| OK: status.AuthValid, Message: status.Error, | ||
| } | ||
| } |
There was a problem hiding this comment.
🚀 Performance & Scalability | 🟠 Major | 🏗️ Heavy lift
The accounts listing endpoint now performs upstream refreshes and writes credentials.
Statuses is not a read-only call. For every stored OAuth account it calls RefreshStoredIfExpired, and for every Claude profile it calls RefreshCredentialIfExpired; both reach the provider and persist rotated tokens, then r.replace(account) mutates in-memory state (see Lines 442 and 466). Two consequences follow.
- Latency and availability:
handleAccountspreviously served an in-memory list. It now blocks on one upstream round trip per account, with no per-account timeout beyond the request context, so one slow provider stalls the whole response. - Side effects on a read path: a plain GET rewrites credential files. Repeated polling of this endpoint drives repeated refresh attempts.
Serve cached health instead. Compute statuses on a background interval, or gate the refresh behind an explicit query parameter and default to the cached value.
As per coding guidelines: "Keep credential handling read-only unless a command explicitly delegates to the upstream account manager, such as cx".
🤖 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 `@internal/proxy/proxy.go` around lines 1030 - 1039, Update the
accounts-listing handler around the healthByAccount construction to avoid
calling AccountRef.Statuses, since it refreshes providers and persists
credentials. Use the existing cached health data, populated by a background
refresh mechanism, and keep the GET path read-only without upstream calls or
credential writes.
Source: Coding guidelines
| var networkErr net.Error | ||
| return errors.As(err, &networkErr) && | ||
| (networkErr.Timeout() || networkErr.Temporary()) | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🌐 Web query:
Is net.Error Temporary deprecated in Go and what is the recommended replacement?
💡 Result:
Yes, the Temporary method in the net.Error interface is deprecated as of Go 1.18 [1][2]. The official Go documentation states that Temporary is deprecated because temporary errors are not well-defined, noting that most errors previously labeled as temporary are actually timeouts, while other exceptions are surprising and inconsistent [3][4]. There is no direct, universal replacement for net.Error.Temporary [2]. The Go team has explicitly advised against its use, recommending that developers stop relying on it [1][5]. If you were using Temporary for specific network operations, such as handling transient errors in a server Accept loop, the recommended approach is to identify and handle specific error types that are relevant to your application's context, rather than relying on a generic temporary status [2][6]. For example, if you are handling Accept errors, you might specifically look for errors related to system limits (such as EMFILE or ENFILE) using errors.Is or type assertions, instead of assuming all temporary errors should be treated the same way [2][6].
Citations:
- 1: net: deprecate Temporary error status golang/go#45729
- 2: https://groups.google.com/g/golang-nuts/c/-JcZzOkyqYI/m/vNNiVn_LAwAJ
- 3: https://pkg.go.dev/net@go1.26.3
- 4: https://pkg.go.dev/net
- 5: net: document the meaning of "Temporary" and "Timeout" golang/go#48268
- 6: net/http: server.Serve() uses deprecated net.Error.Temporary() golang/go#66208
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== file exists and relevant lines =="
if [ -f internal/stackauth/client.go ]; then
sed -n '1,160p' internal/stackauth/client.go | nl -ba
else
echo "internal/stackauth/client.go not found"
fi
echo
echo "== golangci/golangci-lint config and staticcheck settings =="
fd -a 'golangci.*|\.golangci.*' . | sed 's#^\./##' | while read -r f; do
echo "--- $f"
sed -n '1,220p' "$f"
done
echo
echo "== go module/toolchain settings =="
sed -n '1,80p' go.mod 2>/dev/null || true
rg -n "temporarily|temporary|SA1019|staticcheck|golangci|lint" .github .golangci.yml .golangci.yaml golangci.yml golangci.yaml . 2>/dev/null | head -n 200 || trueRepository: manaflow-ai/subrouter
Length of output: 236
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== file exists and relevant lines =="
if [ -f internal/stackauth/client.go ]; then
cat -n internal/stackauth/client.go | sed -n '1,160p'
else
echo "internal/stackauth/client.go not found"
fi
echo
echo "== go module/toolchain settings =="
if [ -f go.mod ]; then
cat -n go.mod | sed -n '1,80p'
fi
echo
echo "== lint-related config and references =="
find . -maxdepth 3 \( -name 'golangci*' -o -name '.golangci*' -o -iname '*golangci*' \) -type f 2>/dev/null | while read -r f; do
printf -- '\n--- %s\n' "$f"
cat -n "$f" | sed -n '1,220p'
done
echo
echo "== search relevant temporary/lint references =="
grep -RIn --include='*.go' --include='*.yaml' --include='*.yml' --include='*.json' --include='Makefile' --include='*.md' \
-E 'Temporary|SA1019|golangci|staticcheck|lint' . 2>/dev/null | head -n 200 || trueRepository: manaflow-ai/subrouter
Length of output: 7357
Replace the deprecated net.Error.Temporary check.
Temporary is deprecated, and this linter warning can block the build. Use Timeout() plus explicit checks for connection-level retryable errors; add "syscall" to the imports if you handle ECONNRESET/ECONNREFUSED.
🧰 Tools
🪛 golangci-lint (2.12.2)
[error] 121-121: SA1019: networkErr.Temporary has been deprecated since Go 1.18 because it shouldn't be used: Temporary errors are not well-defined. Most "temporary" errors are timeouts, and the few exceptions are surprising. Do not use this method.
(staticcheck)
🤖 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 `@internal/stackauth/client.go` around lines 119 - 122, Update the
network-error check around the net.Error handling to remove the deprecated
networkErr.Temporary() call. Preserve timeout detection via
networkErr.Timeout(), and explicitly recognize connection-level retryable errors
such as syscall.ECONNRESET and syscall.ECONNREFUSED, adding the syscall import
if needed.
Source: Linters/SAST tools
|
This work landed on main through #123 (ae783ba). That squash includes this branch's hosted-onboarding commits: Closing as superseded. If something here isn't covered, feel free to reopen. Generated by Claude Code |
2 participants
Implements the normal-user hosted flow:
sr loginuses Stack Auth native CLI authentication and maps the selected Stack team to an isolated hosted tenantsr add codexandsr add claudeauthenticate in temporary homes, upload credentials, and leave local agent auth unchangedsr remoteswitches between built-in local, hosted cmux, and named self-hosted serversVerification:
go test ./...go vet ./...go build ./cmd/subrouterThe commits keep the regression tests separate from the implementation.
Need help on this PR? Tag
@codesmith-botwith what you need. Autofix is disabled.Summary by cubic
Enables normal-user onboarding to hosted Subrouter using native Stack Auth with a built-in
cmuxremote. Users can log in, pick a team, and add Codex/Claude to a tenant-scoped hosted pool without changing local auth.New Features
sr loginuses native Stack Auth (internal/stackauth): loads hosted public config, binds the chosen team, and deterministically derives a tenant key (internal/tenant.DeriveKey). Config now storeshostedUrl,tenantKey, and Stack fields.sr remote/sr remotesmanage and switch betweenlocal, built-incmux, and custom remotes; multi-tenant server exposes/t/<tenant-key>/_subrouter/....sr add codexandsr add claudesupport hosted onboarding: OAuth runs in a temporary store, credentials are uploaded to the hosted tenant, and no local profile is left behind; local Claude profiles share~/.claudehistory to avoid duplicates.internal/stackauth.Verifier) and supports team-to-tenant exchange;/accountsincludes per-account auth health. Broker config addshostedcredential source and Stack settings; public Subrouter URL must be a clean HTTPS origin (orhttp://127.0.0.1).Bug Fixes
experimental_bearer_token="subrouter"and only usingenv_key="SUBROUTER_CODEX_DUMMY_API_KEY"when forced.Written for commit 266f41f. Summary will update on new commits.
Summary by CodeRabbit
New Features
Bug Fixes