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 and projectsMutate.
• 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.

Supersedes #17250, which was closed for not listing the model and harness. Rebased on current main.

Written by Claude Opus 5.5 (claude-opus-5-5) in Claude Code, running in T3 Code. Reviewed by GPT-6.1-Sol (gpt-6.1-sol) in Codex, running in T3 Code.

[macroscopeapp]

Approvability

Verdict: Not approved

Macroscope's review found this PR not approvable — This focused bug fix prevents double-decoding of transformed RPC payloads and adds regression coverage, but it changes production authorization middleware and connection-level failure handling. Changes under the server auth directory require human review.

Notes:

• Diff unchanged. Approvability was decided on eligibility alone.

^{You can add or adjust custom eligibility rules. Learn more.}

[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: 3f0a717f-11ba-4501-b9f2-a6b538cf0b68

📥 Commits

Reviewing files that changed from the base of the PR and between cbbf7e5 and 9aecd4d.

📒 Files selected for processing: 2

• 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

RPC authorization now uses decoded payloads for scope calculation instead of decoding them again. Scope calculation runs inside an effect. A middleware test covers settings updates that contain duration values.

Changes: RPC authorization payload handling

Layer / File(s)	Summary
Server authorization for decoded payloads apps/server/src/auth/RpcAuthorization.ts, apps/server/src/auth/RpcAuthorization.test.ts	The middleware derives method and payload types from WsRpcGroup. Scope helpers use decoded settings, asset URL, and project-mutation payloads. Scope calculation runs inside an effect. A test checks a settings update containing duration values.
Client thread-preparation scopes packages/contracts/src/clientRpcPermissions.ts	The helper uses the supplied thread-preparation input without schema decoding. Its conditional scope selection remains unchanged.

Priority: ➖ Normal

Estimated code review effort: 3 (Moderate) | ~20 minutes

Change: Bug fix · Severity of issue fixed: Medium

Possibly related PRs

• pingdotgg/t3code#17250: Both changes use decoded RPC payloads in authorization, suspend scope calculation, add a duration-based middleware test, and remove redundant thread-preparation input decoding.

Suggested reviewers: juliusmarminge

Merge Risk: ⚪ Minimal · up to 9aecd

This change lets background activity settings save without a second decode failing and dropping the connection. No merge-blocking risk is evident from the supplied evidence.

Security architecture risk: 🔵 Low · up to 9aecd

Permission checks are preserved, and failures are better isolated to individual requests. No new authorization bypass was identified, but request-validation ordering was not independently confirmed at runtime.

Retained concerns
No architecture-level concerns identified.

Security review details

Security Blast Radius

• observed — The changed authorization path controls authenticated environment RPC operations, including configuration/provider changes, project mutations, host-file versus attachment URL access, and worktree/thread preparation. Scope calculation remains tied to the connection's authenticated session; the change does not itself grant additional scopes.

Trust Boundaries and Controls

• inferred — The intended trust sequence is attacker-controlled wire input, RPC schema decoding, session-scope authorization, then handler execution. Removing helper decoders is safe only under that ordering. Schema declarations, middleware installation, and typed client dispatch support this design, but the external runtime's internal ordering was not directly inspected.

Resilience and Maintainability Implications

• observed — The changed selector and gate contain no mutation, reservation, cleanup, or retry logic. Missing grants take the failure branch before the handler effect is returned. Mutation ownership, atomicity, and recovery implementations are not changed by this PR; broader runtime replay behavior remains outside the verified coverage.

Hardening Proposals

• proposed — Add wire-level boundary tests showing that malformed payloads cannot reach mutation handlers and that a scope-calculation defect fails only its request while the connection remains usable. This would directly verify the runtime guarantees on which decoder removal and request isolation depend.

Pre-merge checks: 4

Passed checks: 4 passed

Check name	Status	Explanation
Title check		The title clearly and concisely describes the primary fix: saving background activity settings without dropping the connection.
Description check		The description explains the problem, root cause, fix, affected components, linked issue, regression coverage, and agent attribution. The “Fix” section serves the required Change section, and the veri…
Linked Issues check		For #17233, RpcAuthorization.ts now uses the RPC-decoded payload for serverUpdateSettings and preserves settings and provider scope checks. Effect.suspend defers scope calculation to the request…
Out of Scope Changes check		The changes remain within the double-decode authorization defect in #17233. The assetsCreateUrl, projectsMutate, and gitPreparePullRequestThread changes remove the same decoded-payload hazard. T…

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create a new PR

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

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