Saving background activity intervals decoded Effect Duration values a second time during authorization. This failed the save and interrupted pending requests on the connection.
Consume the RPC middleware's already-decoded payloads while keeping the same settings, provider, project, asset and thread-preparation scope checks. Suspend scope calculation inside the request effect so a throw stays request-local. Preserve main's successful-request analytics callback.
Original implementation and diagnosis by @bfowler in #18129. Maintainer takeover preserves the contributor commits and merges main without rewriting history.
SchemaError: Expected number for the nested idleClientTtl Duration. Restoring the fix passes. The in-memory regression proves authorization; live verification below proves persistence.Same isolated worktree server and Chromium client, 1280×800, against actual main and candidate source. Main: Advanced background activity, Git fetch 30 → 35 seconds fails with “All fibers interrupted without error”, disconnects and leaves the persisted interval unchanged. Candidate: the same edit persists 35 seconds, survives reload, and a further edit to 40 seconds plus a second RPC succeeds on the same connection. No browser mutation substitutes for the save. A local font was injected only for readable Linux captures.
Before — main: failed save
After — saved 35 seconds, reopened after reload
Saved background interval after reload
Recording: interval edit from 30 to 35 seconds
Limits: Settings briefly showed a separate “unsupported setting” warning during the first successful save; the file and subsequent RPC confirmed persistence. That client-side dropped-setting detection is unchanged here. macOS, Windows, iOS, Android, Electron desktop, remote/relay/tunnel modes were not exercised. This common server authorization path serves those clients, but no native-platform proof is claimed.
Human authorization review is required by Macroscope's approvability decision. Automated sub-agent review does not replace it. Current-head correctness, lint, typecheck and test jobs passed. Macroscope approvability is neutral (Needs Human Approval); an older canceled native fingerprint attempt has a successful replacement on this head. This PR remains unmerged and blocked on human authorization review.
Implementation: GPT-6.1-Sol; tests: GPT-6-Astra; reviews: Claude Opus 5.5 and GPT-6-Sol, through Codex/Claude harnesses in T3 Code.
Verdict: Needs Human Approval
Macroscope's review found this PR requires human approval — This is a focused runtime bug fix that preserves existing scope checks while preventing decoded settings payloads from interrupting the WebSocket connection. Because it modifies the shared authorization middleware used by WebSocket RPCs, the security-sensitive boundary warrants human review.
You can add or adjust custom eligibility rules. Learn more.
No actionable comments were generated in the recent review. 🎉
3f0a717f-11ba-4501-b9f2-a6b538cf0b68Reviewing files that changed from the base of the PR and between cbbf7e5a8740fbd7fb71851090b832569f1ca16c and 9aecd4d1aeb819f47c3b48271112bf3aba6b38a1.
apps/server/src/auth/RpcAuthorization.tspackages/contracts/src/clientRpcPermissions.tsIncluded review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 9 remain after this review.
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
Suggested reviewers: juliusmarminge
9aecdThis 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.
9aecdPermission 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 Blast Radius
Trust Boundaries and Controls
Resilience and Maintainability Implications
Hardening Proposals
Failed Failed checks: 1 warning
| Check name | Status | Explanation | Resolution |
|---|---|---|---|
| Description check | Warning | The description clearly explains the problem, implementation, affected authorization paths, verification results, platform limits, and remaining review gate. It does not provide an explicit link to th… | Add a link to the triaged issue or approval discussion and identify the maintainer approval comment. If this focused fix does not require prior approval, explain why it qualifies as an obvious bug fix and why its scope is limited to the exi… |
| Check name | Status | Explanation |
|---|---|---|
| Linked Issues check | Passed | 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 | Passed | The changes remain within the double-decode authorization defect in #17233. The assetsCreateUrl, projectsMutate, and gitPreparePullRequestThread changes remove the same decoded-payload hazard. T… |
| Title check | Passed | The title clearly identifies the server fix for background activity settings saves and connection stability. It is concise and uses the expected conventional format. |
Explanation
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 effect. The regression test sends Duration values at the top level and in backgroundActivity.overrides through RpcTest and RpcAuthorization.layer, then verifies that the handler runs.
Explanation
The changes remain within the double-decode authorization defect in #17233. The assetsCreateUrl, projectsMutate, and gitPreparePullRequestThread changes remove the same decoded-payload hazard. The Effect.suspend change prevents authorization defects from affecting the connection. The added test supports the settings fix. No unrelated change is shown.
Explanation
The description clearly explains the problem, implementation, affected authorization paths, verification results, platform limits, and remaining review gate. It does not provide an explicit link to the triaged issue or discussion with maintainer approval of the direction and scope, nor does it explain why the change qualifies for an approval exemption.
Resolution
Add a link to the triaged issue or approval discussion and identify the maintainer approval comment. If this focused fix does not require prior approval, explain why it qualifies as an obvious bug fix and why its scope is limited to the existing authorization behavior described here.
Comment @coderabbitai help to get the list of available commands.
Added 1 commit
Taking over this one. I’m updating the decoded-payload authorization fix against main, retaining scope enforcement, and verifying settings saves through the RPC path. Human authorization review will remain required.
Added 3 commits
Independent verification of the reported Settings → Providers → Advanced → Health check interval failure on macOS 27.0.1 arm64, T3 Code (Beta) 0.1.0, local desktop environment. The running app uses the Performance background profile: the attempted change was 60 → 600 seconds. Typing and the step buttons both interrupted the environment connection; restarting T3 did not resolve it, and the saved interval remained 60 seconds.
I checked the candidate at e8eb1110ce9f3de021bfa2d56a57a273e807ff4c with an additional isolated regression fixture for this exact field. It sends numeric milliseconds through NDJSON HTTP RPC, the real WsRpcGroup, real RpcAuthorization.layer, and real ServerSettingsService persistence in a temporary test home with an in-memory database. It does not touch the installed T3 state.
| Check | Current main 7e289ae1f77b9137580688aaccf00d48906984ad | PR head e8eb1110c |
|---|---|---|
Authorized update sends providerHealthRefreshInterval: 600000; handler receives a 600-second Duration; file and subsequent server.getSettings read back 600 seconds | Fails with a connection-level Defect before persistence | Pass |
Same duration patch with only providers:manage yields a typed missing settings:write error; no handler effects or file change | Defect instead of typed denial | Pass |
Duration patch plus provider mutation with only settings:write yields a typed missing providers:manage error; no handler effects or file change | Defect instead of typed denial | Pass |
The three additional cases fail on main and pass on this PR. The existing 26 authorization tests also pass: 29/29 on the candidate. Scoped lint for the fixture is clean, and the server typecheck including the fixture exits 0. Test runtime: Node.js 26.12.0, Effect 4.0.5, Vitest 5.0.1; the root package declares Node 24, so this is additional diagnostic evidence rather than a claim of the canonical toolchain run.
Current integration finding: GitHub reports mergeable: CONFLICTING. A local merge of this head into the main revision above conflicts in packages/contracts/src/clientRpcPermissions.ts, around the imports and decoder declarations: main added the stacked-action input/decoder while this PR removes the prepare-thread decoder. The new stacked-action scope requirements need to be preserved when updating the branch.
Limits: the candidate check exercises serialized HTTP RPC and persistence, not an Electron UI or WebSocket reconnect journey. It does not replace the human authorization review already required for this PR. No installed build was modified.
Save as apps/server/src/auth/RpcAuthorization.duration-verification.test.ts on either revision and run:
| 1 | vp test run apps/server/src/auth/RpcAuthorization.duration-verification.test.ts apps/server/src/auth/RpcAuthorization.test.ts --silent |
| 1 | import * as NodeServices from "@effect/platform-node/NodeServices"; |
| 2 | import { expect, it } from "@effect/vitest"; |
| 3 | import { |
| 4 | AuthOrchestrationReadScope, |
| 5 | AuthProvidersManageScope, |
| 6 | AuthSettingsWriteScope, |
| 7 | ServerSettings, |
| 8 | WS_METHODS, |
| 9 | WsRpcGroup, |
| 10 | type AuthEnvironmentScope, |
| 11 | } from "@t3tools/contracts"; |
| 12 | import * as Duration from "effect/Duration"; |
| 13 | import * as Effect from "effect/Effect"; |
| 14 | import * as FileSystem from "effect/FileSystem"; |
| 15 | import * as Layer from "effect/Layer"; |
| 16 | import * as Schema from "effect/Schema"; |
| 17 | import * as HttpRouter from "effect/http/HttpRouter"; |
| 18 | import * as RpcSerialization from "effect/rpc/RpcSerialization"; |
| 19 | import * as RpcServer from "effect/rpc/RpcServer"; |
| 20 | import { resolveServerBackgroundActivitySettings } from "@t3tools/shared/backgroundActivitySettings"; |
| 21 | import * as ServerConfig from "../config.ts"; |
| 22 | import * as SqlitePersistence from "../persistence/Sqlite.ts"; |
| 23 | import * as ServerSettingsModule from "../serverSettings.ts"; |
| 24 | import * as RpcAuthorization from "./RpcAuthorization.ts"; |
| 25 | import * as ServerSecretStore from "./ServerSecretStore.ts"; |
| 26 | |
| 27 | const group = WsRpcGroup.omit( |
| 28 | ...[...WsRpcGroup.requests.keys()].filter( |
| 29 | ( |
| 30 | tag, |
| 31 | ): tag is Exclude< |
| 32 | keyof typeof RpcAuthorization.RPC_REQUIRED_SCOPES, |
| 33 | typeof WS_METHODS.serverUpdateSettings | typeof WS_METHODS.serverGetSettings |
| 34 | > => tag !== WS_METHODS.serverUpdateSettings && tag !== WS_METHODS.serverGetSettings, |
| 35 | ), |
| 36 | ); |
| 37 | |
| 38 | const testSettings = ServerSettingsModule.layer.pipe( |
| 39 | Layer.provide(ServerSecretStore.layer), |
| 40 | Layer.provideMerge(Layer.fresh(SqlitePersistence.layerMemory)), |
| 41 | Layer.provideMerge( |
| 42 | Layer.fresh(ServerConfig.layerTest(process.cwd(), { prefix: "t3-duration-rpc-verification-" })), |
| 43 | ), |
| 44 | ); |
| 45 | |
| 46 | const decodeSavedSettings = Schema.decodeUnknownEffect(Schema.fromJsonString(ServerSettings)); |
| 47 | const testServices = testSettings.pipe(Layer.provideMerge(NodeServices.layer)); |
| 48 | |
| 49 | const patch = { |
| 50 | backgroundActivity: { |
| 51 | schemaVersion: 1, |
| 52 | profile: "custom", |
| 53 | baseProfile: "performance", |
| 54 | overrides: { |
| 55 | automaticGitFetchInterval: 15000, |
| 56 | providerHealthRefreshInterval: 600000, |
| 57 | hostPowerMonitorActiveInterval: 30000, |
| 58 | hostPowerMonitorIdleInterval: 120000, |
| 59 | idleClientTtl: 45000, |
| 60 | pauseWhenHostLocked: true, |
| 61 | pauseWhenHostLowPower: false, |
| 62 | pauseWhenClientLowPower: false, |
| 63 | pauseWhenOnBattery: false, |
| 64 | }, |
| 65 | }, |
| 66 | }; |
| 67 | |
| 68 | const verify = Effect.fn("DurationVerification.verify")(function* ( |
| 69 | scopes: ReadonlyArray<AuthEnvironmentScope>, |
| 70 | missing?: AuthEnvironmentScope, |
| 71 | includeProviderMutation = false, |
| 72 | ) { |
| 73 | const service = yield* ServerSettingsModule.ServerSettingsService; |
| 74 | const config = yield* ServerConfig.ServerConfig; |
| 75 | const fs = yield* FileSystem.FileSystem; |
| 76 | yield* service.updateSettings({ |
| 77 | backgroundActivity: { schemaVersion: 1, profile: "performance", overrides: {} }, |
| 78 | }); |
| 79 | expect( |
| 80 | Duration.toSeconds( |
| 81 | resolveServerBackgroundActivitySettings(yield* service.getSettings) |
| 82 | .providerHealthRefreshInterval, |
| 83 | ), |
| 84 | ).toBe(60); |
| 85 | const before = yield* fs.readFileString(config.settingsPath); |
| 86 | let handled = 0; |
| 87 | const handlers = group.toLayer({ |
| 88 | [WS_METHODS.serverUpdateSettings]: ({ patch, providerInstanceMutation }) => |
| 89 | Effect.gen(function* () { |
| 90 | handled++; |
| 91 | expect( |
| 92 | Duration.toSeconds(patch.backgroundActivity!.overrides!.providerHealthRefreshInterval!), |
| 93 | ).toBe(600); |
| 94 | return yield* providerInstanceMutation === undefined |
| 95 | ? service.updateSettings(patch) |
| 96 | : service.updateProviderInstance(providerInstanceMutation, patch); |
| 97 | }), |
| 98 | [WS_METHODS.serverGetSettings]: () => service.getSettings, |
| 99 | }); |
| 100 | const { handler, dispose } = HttpRouter.toWebHandler( |
| 101 | RpcServer.layerHttp({ group, path: "/rpc", protocol: "http", disableFatalDefects: true }).pipe( |
| 102 | Layer.provide(handlers), |
| 103 | Layer.provide(RpcAuthorization.layer(scopes)), |
| 104 | Layer.provide(RpcSerialization.layerNdjson), |
| 105 | ), |
| 106 | ); |
| 107 | yield* Effect.addFinalizer(() => Effect.promise(dispose)); |
| 108 | const post = (id: number, tag: string, payload: unknown) => |
| 109 | Effect.promise(async () => { |
| 110 | const response = await handler( |
| 111 | new Request("http://test/rpc", { |
| 112 | method: "POST", |
| 113 | headers: { "content-type": "application/json" }, |
| 114 | body: JSON.stringify({ _tag: "Request", id, tag, payload, headers: [] }) + "\n", |
| 115 | }), |
| 116 | ); |
| 117 | const text = await response.text(); |
| 118 | return text |
| 119 | .trim() |
| 120 | .split("\n") |
| 121 | .flatMap((line) => JSON.parse(line)); |
| 122 | }); |
| 123 | const frames = yield* post(1, WS_METHODS.serverUpdateSettings, { |
| 124 | patch, |
| 125 | ...(includeProviderMutation |
| 126 | ? { |
| 127 | providerInstanceMutation: { operation: "remove", instanceId: "codex_test" }, |
| 128 | } |
| 129 | : {}), |
| 130 | }); |
| 131 | if (missing !== undefined) { |
| 132 | expect(frames).toMatchObject([{ _tag: "Exit", exit: { _tag: "Failure" } }]); |
| 133 | expect(JSON.stringify(frames)).toContain(`"requiredPermission":"${missing}"`); |
| 134 | expect(handled).toBe(0); |
| 135 | expect(yield* fs.readFileString(config.settingsPath)).toBe(before); |
| 136 | return; |
| 137 | } |
| 138 | expect(frames).toMatchObject([{ _tag: "Exit", exit: { _tag: "Success" } }]); |
| 139 | expect(handled).toBe(1); |
| 140 | const saved = yield* decodeSavedSettings(yield* fs.readFileString(config.settingsPath)); |
| 141 | expect( |
| 142 | Duration.toSeconds(saved.backgroundActivity.overrides.providerHealthRefreshInterval!), |
| 143 | ).toBe(600); |
| 144 | const again = yield* post(2, WS_METHODS.serverGetSettings, {}); |
| 145 | expect(again).toMatchObject([ |
| 146 | { |
| 147 | _tag: "Exit", |
| 148 | exit: { |
| 149 | _tag: "Success", |
| 150 | value: { |
| 151 | backgroundActivity: { overrides: { providerHealthRefreshInterval: 600000 } }, |
| 152 | }, |
| 153 | }, |
| 154 | }, |
| 155 | ]); |
| 156 | }); |
| 157 | |
| 158 | it.live( |
| 159 | "persists the exact 60-to-600-second health interval through serialized authorized RPC", |
| 160 | () => |
| 161 | verify([AuthSettingsWriteScope, AuthOrchestrationReadScope]).pipe( |
| 162 | Effect.scoped, |
| 163 | Effect.provide(testServices), |
| 164 | ), |
| 165 | ); |
| 166 | |
| 167 | it.live("rejects a serialized duration patch with only the provider grant", () => |
| 168 | verify([AuthProvidersManageScope], AuthSettingsWriteScope).pipe( |
| 169 | Effect.scoped, |
| 170 | Effect.provide(testServices), |
| 171 | ), |
| 172 | ); |
| 173 | |
| 174 | it.live("rejects a serialized duration and provider mutation without the provider grant", () => |
| 175 | verify([AuthSettingsWriteScope], AuthProvidersManageScope, true).pipe( |
| 176 | Effect.scoped, |
| 177 | Effect.provide(testServices), |
| 178 | ), |
| 179 | ); |
Reproduction and verification: GPT-6 through Codex Desktop, on behalf of @OscarSwanros.
