fix(server): background activity settings save without dropping the connection

[bfowler]

Fixes #17233

Problem

Changing anything under Settings → General → Background activity → Advanced (for example turning off Pause when locked) failed with "Setting not saved", and the environment connection dropped and reconnected. The same happened for the Git fetch and provider health check intervals and Restore defaults.

RpcScopeAuthorization decoded the serverUpdateSettings payload with Schema.decodeUnknownSync after the RPC server had already decoded it. The patch's DurationFromMillis fields were already Duration objects, so the second decode threw SchemaError: Expected number at ["patch"]["backgroundActivity"]["overrides"]["automaticGitFetchInterval"]. The throw happened synchronously while the RPC server applied middleware, so it was reported as a connection-level defect and the client failed every pending request on that socket.

Fix

• RpcAuthorization.ts reads the payload the RPC server already decoded, typed from the RPC definition, instead of decoding it again. The same change applies to assetsCreateUrl.
• Scope calculation runs inside Effect.suspend, so a future throw there fails only its own request. The /ws server sets disableFatalDefects, so it no longer takes the connection down.
• clientRpcRequiredScopes no longer decodes the gitPreparePullRequestThread input again. Its schema has no transforms today, so it wasn't broken, but it had the same trap. Web, mobile and the server all call it with already-decoded input.
• Regression test: a settings patch with Duration intervals at the top level and in backgroundActivity.overrides, sent through the real authorization middleware. It fails with the error from the issue before this change and passes after.

A sweep of server, contracts, client-runtime, web and mobile found no other place where a transforming schema is decoded twice in a way that throws.

[coderabbitai]

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

• Configuration used: Path: .coderabbit.config.ts
• Review profile: CHILL
• Plan: Advanced
• Run ID: 28f8c32b-2501-4435-9012-a89537a72d39

📥 Commits

Reviewing files that changed from the base of the PR and between a6ec88f and 7af937e.

📒 Files selected for processing (3)

• apps/server/src/auth/RpcAuthorization.test.ts
• apps/server/src/auth/RpcAuthorization.ts
• packages/contracts/src/clientRpcPermissions.ts

Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 9 remain after this review.

---

📝 Walkthrough

Walkthrough

RPC authorization now uses already-decoded payloads to select scopes and suspends per-request scope calculation. A regression test covers settings updates with duration values.

Changes

RPC Authorization

Layer / File(s)	Summary
Decoded payload scope selection apps/server/src/auth/RpcAuthorization.ts, packages/contracts/src/clientRpcPermissions.ts	Authorization scope checks use RPC-derived payload types and read decoded asset, settings, and thread inputs.
Request-scoped calculation and regression test apps/server/src/auth/RpcAuthorization.ts, apps/server/src/auth/RpcAuthorization.test.ts	Scope calculation runs inside Effect.suspend. A middleware test verifies that a duration-bearing settings update reaches its handler.

Priority: ➖ Normal

Estimated code review effort: 2 (Simple) | ~10 minutes

Change: Bug fix · Severity of issue fixed: Medium

Suggested reviewers: juliusmarminge

Merge Risk: ⚪ Minimal · up to 7af93

No actionable issue remains from this review; the change is mergeable after normal checks.

Security Architecture Review

Security architecture risk: 🔵 Low · up to 7af93

The change preserves existing permission checks and is designed to prevent one failed request from disrupting the connection. No introduced security weakness was established, but complete request-failure and interruption behavior remains unverified.

Retained concerns
No architecture-level concerns identified.

Security review details

Security Blast Radius

• inferred — The affected authority boundary is per-connection environment RPC authorization. Payload-dependent changes cover settings and provider mutations, asset reads, and worktree preparation; deferred scope calculation applies to every RPC using this middleware. No new route, scope grant, or downstream credential authority is introduced by the inspected changes.

Trust Boundaries and Controls

• inferred — An attacker cannot obtain server authority merely by bypassing client permission calculation: production requests still pass through server RPC schemas and scope middleware before handlers. Removing decoding from the shared helper weakens local validation for malformed direct callers, but that helper is not the authoritative network security boundary.

Resilience and Maintainability Implications

• observed — Successfully authorized Duration patches enter the existing settings mutation path. Settings writes remain serialized, with atomic persistence in an uninterruptible section and rollback of secret changes on write failure. Failure after persistence can leave a fully committed update without a successful response; these mutation and recovery semantics predate this PR.

🚥 Pre-merge checks | ✅ 4

✅ Passed checks (4 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title clearly and concisely describes the primary fix: preventing background activity settings saves from dropping the server connection.
Description check	✅ Passed	The description explains the problem, root cause, implementation, linked issue, and regression test. It uses a "Fix" heading instead of the template's "Change" heading and does not provide explicit ma…
Linked Issues check	✅ Passed	Issue #17233 requires settings updates with duration values to pass through authorization, preserve settings and provider scope checks, and include a regression test through the real middleware. The P…
Out of Scope Changes check	✅ Passed	The changes to assetsCreateUrl, clientRpcRequiredScopes, and Effect.suspend address the same risk of decoding already-decoded RPC input or allowing a scope-calculation throw to affect the connec…

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create a new PR

• Autopilot · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

---

_{Comment @coderabbitai help to get the list of available commands.}

[maria-rcks]

Note

Written by claude-opus-5-5 on behalf of Maria

Hi! We are cleaning up open PRs, and this one does not say which model or harness was used to create it. If this change is really important, we recommend rebuilding the PR with a newer model and noting the model and harness in the PR description.