chore: sync with upstream/main (348f217a8)

[Fryuni]

Problem

The fork is 157 commits behind upstream/main. This sync has to keep the features recorded in docs/adr while using upstream's implementations wherever it can.

Change

Merged pingdotgg/t3code main at 348f217a853dd4a23d7fa4834dedb05f545c2075 into the fork. None of the 157 commits implement a fork feature, so every ADR stays accepted. Four files conflicted; the rest merged cleanly.

• Update everything (ADR 0011). Upstream now returns { targets, skipped } and names every skipped machine in the confirmation. Fork-built servers go into skipped with "updates from its own releases" instead of being dropped silently. They are still never targeted.
• Scheduled task dialog (ADR 0016). Upstream wrapped its model picker in a div so the dialog stops shifting (fix(web): scheduled task model picker no longer shifts the dialog pingdotgg/t3code#18940). The fork's wrapper around the model and option pickers already provides this, so the fork's side stays.
• Mobile PR search (ADR 0002). Keeps the fork's shared normalizeSourceControlRepository comparisons beside upstream's new linked-PR suggestions.
• Sidebar. Imports both upstream's useSidebarProjectGroups and the fork's project-PR-list hook (ADR 0014).
• Delegated worktrees (ADR 0008). Upstream's delegate_task isolation (feat(server): run delegated tasks in separate worktrees pingdotgg/t3code#19102) builds its own strategy without createBranch/startFromRemote, so delegated worktrees stay on upstream behavior. The fork's schedule-tool tests now use upstream's test layer, which mocks the launch service and git driver that isolation added.
• Default base branch (ADR 0009). Upstream's dropped-setting check (fix(web): settings warn when an environment silently drops a setting pingdotgg/t3code#18334) also covers the fork's project-only key. An environment that drops it is now reported as not supporting the setting instead of failing silently. A new test covers this.
• CI (ADR 0011). Upstream's changes to cursor-hygiene-webhook.yml are not taken; the workflow stays deleted.
• ADRs now compare against 348f217a8 and record each adaptation.

Verification

• Typecheck is clean for contracts, shared, client-runtime, source-control-forgejo, server, web, mobile and desktop.
• Ran all 88 test files that differ from upstream: everything passes except the known local environment failures, which do not involve fork code: five GitManager own-name tests, and the desktop WSLENV test, which passes once the inherited T3CODE_TELEMETRY_ENABLED is unset. CI is the source of truth for these.
• server-linux-release.yml still sets T3CODE_RELEASE_REPOSITORY, and apps/server/vite.config.ts still turns it into the release define.

Model: Claude Opus 5.5; harness: Claude Code in T3 Code.

🤖 Generated with Claude Code

[chatgpt-codex-connector]

Codex Review Summary

This comment shows the latest Codex review activity on this pull request.

Review	Status	Commit	Review trigger
📝 Code Review	✅ Completed 2026-10-14T22:17:12.287429Z	d362675	PR opened

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review" or "@codex security review".

Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings.

[Fryuni]

@fryuni-s-proval Please review this upstream sync at d362675e8 and give an explicit approve or request-changes recommendation.

This is a maintainer-requested sync, not a feature PR. Its parents are fork main 903cfb5bc and upstream main 348f217a8. Most of the 595 files come straight from upstream. Problems that already exist on either parent are out of scope. Please review how the upstream changes integrate with the fork's ADR features, not upstream's own implementation.

Review units (git show --remerge-diff d362675e8 shows the manual resolutions):

1. Update everything (ADR 0011). apps/web/src/components/sidebar/updateEverything.logic.ts adopts upstream's { targets, skipped } result. A server with releaseRepository is skipped with "updates from its own releases" and never targeted. The fork test in updateEverything.logic.test.ts checks this.
2. Delegated worktrees (ADRs 0008, 0009). Upstream's delegate_task isolation in OrchestratorMcpService.ts builds { type: "worktree", baseRef: <sha>, startFromOrigin: false } and goes straight to ThreadLaunchService.prepareRun. With createBranch and startFromRemote absent, ThreadLaunchService.ts resolves no start remote and creates a branch, so the fork's pre-launch checks are not needed on that path. The fork's schedule-tool tests in OrchestratorMcpService.test.ts now use upstream's layerOrchestratorService.
3. Scheduled task dialog (ADR 0016). ScheduledTasksSettings.tsx keeps the fork's flex wrapper around the model and option pickers, which covers upstream's no-shift div (fix(web): scheduled task model picker no longer shifts the dialog pingdotgg/t3code#18940).
4. Default base branch (ADR 0009). planProjectDefaultThreadBaseBranchPatch goes through upstream's projectOverrideWrites and persistScopedSettingsPatch, so upstream's dropped-setting check (fix(web): settings warn when an environment silently drops a setting pingdotgg/t3code#18334) now reports a server that drops the key. Covered by a new test in scopedSettings.test.ts.
5. Mobile and sidebar. apps/mobile/src/state/queries.ts keeps normalizeSourceControlRepository (ADR 0002) next to upstream's composerPullRequestsFromLinks. Sidebar.tsx imports both hooks (ADR 0014).
6. CI. Upstream's cursor-hygiene-webhook.yml stays deleted (ADR 0011).
7. ADR updates in docs/adr/.

[fryuni-s-proval]

@Fryuni

My recommendation is to approve. I checked each of your review units against the head snapshot and found nothing that blocks the merge. I can't submit a formal GitHub review, so this comment is the recommendation.

What I checked

1. Update everything (ADR 0011). Confirmed.

   • In apps/web/src/components/sidebar/updateEverything.logic.ts (collectServerUpdateTargets), the releaseRepository !== undefined check calls skip("updates from its own releases") and runs continue before anything is added to targets.
   • updateEverything.logic.test.ts:93-103 checks this.
   • The check comes after two others, so a fork server that is already on the target version is left out of both lists, and a disconnected fork server shows "not connected". That matches upstream's rules, so it isn't a problem.

2. Delegated worktrees (ADRs 0008, 0009). Confirmed: the fork's pre-launch checks do nothing for this strategy.

   • OrchestratorMcpService.ts:2058 builds { type: "worktree", baseRef: base.commitSha, startFromOrigin: false }.
   • resolveWorktreeStartRemote (packages/contracts/src/vcs.ts:12-21) returns null when startFromRemote is missing and startFromOrigin is not true.
   • So validateExistingBranchCheckout stops early at ThreadLaunchService.ts:287 because createBranch !== false. validateStartRemote stops early at :322 because the remote isn't "upstream".
   • Bypassing those checks at :901-902 on the prepareRun path (:1114) therefore changes nothing.
   • startFromOrigin: false is passed explicitly, so the server's default origin setting never applies to delegated worktrees. That fits "stays on upstream behavior".

3. Scheduled task dialog (ADR 0016). ScheduledTasksSettings.tsx:1105-1106 wraps ProviderModelPicker in flex min-w-0 flex-wrap items-center gap-1.5, so the picker sits in a stable container. I didn't run the UI, so whether the dialog still shifts is only checked by reading the code.

4. Default base branch (ADR 0009). Confirmed.

   • planProjectDefaultThreadBaseBranchPatch (scopedSettings.ts:328-348) builds its writes through projectOverrideWrites.
   • The new test at scopedSettings.test.ts:420-432 passes the plan to persistScopedSettingsPatch with a server that returns DEFAULT_SERVER_SETTINGS. It expects the "does not support this setting" failure.

5. Mobile and sidebar. Confirmed.

   • apps/mobile/src/state/queries.ts imports both helpers (lines 2 and 5). It uses composerPullRequestsFromLinks at line 98 and the normalized repository comparisons at lines 126-127 and 154-155.
   • Sidebar.tsx:145-146 imports both useOpenProjectPullRequestList and useSidebarProjectGroups.

6. CI. No .github/workflows/cursor-hygiene* file exists on head.

Not covered

• I read the head snapshot. I can't run git show --remerge-diff d362675e8, so I didn't look at the conflict resolutions directly. My checks are of the final code for each unit.
• I didn't review the ADR text in docs/adr/ or the rest of the files that merged without conflicts.
• I didn't run typecheck or tests. Your notes about the GitManager own-name tests and the desktop WSLENV test still need confirming in CI.
• The Codex review on d362675 was still running when you posted. It's worth waiting for its result before merging.

[fryuni-s-proval]

⚫️ [PROBLEM] Quit-path stop timeout now equals the SIGKILL grace, so forced kill can be skipped

⚫ [PROBLEM] This change sets DEFAULT_BACKEND_TERMINATE_GRACE to 5s, and line 507 uses it as forceKillAfter. On quit, stopAllPoolInstances still calls instance.stop({ timeout: Duration.seconds(5) }) (apps/desktop/src/app/DesktopApp.ts:158; the same call is in apps/desktop/src/updates/DesktopUpdates.ts:763). closeRun (lines ~375-379) forks the scope close and returns once timeoutOption(timeout) expires. Before this change, SIGKILL fired at 2s, well inside the 5s budget. Now both fire at about 5s, so the app can finish shutting down before the forced kill runs.

Impact: a backend that is slow to handle SIGTERM, or ignores it, can be left orphaned after quit along with its agents. It still holds the state-dir lock, so on the next launch the server exits with SERVER_EXIT_CODE_STATE_DIR_OWNED. With this PR, that shows the "T3 home is unavailable" dialog and quits.

Fix: keep the quit-path stop timeout strictly longer than the terminate grace. The simplest way is to derive both from one exported constant so they can't drift apart.

// DesktopApp.ts / DesktopUpdates.ts
(instance) => instance.stop({ timeout: Duration.seconds(7) }), // must exceed DEFAULT_BACKEND_TERMINATE_GRACE (5s)

[fryuni-s-proval]

⚫️ [PROBLEM] PR route permanently switches environments during transient connecting/reconnecting

⚫ [PROBLEM] Any environment whose connectionState !== "connected" counts as down here, and that includes the brief connecting/reconnecting phases. On a cold start, when the app returns from the background, or during a short network blip, another environment with the same repo can reach connected first. The effect then calls navigation.setParams(fallback) and rewrites environmentId/projectId. Nothing ever switches it back. Review drafts are keyed by environmentId and projectId (pullRequestReviewKey, pullRequestReviewDrafts.ts:17-25), so an in-progress review seems to disappear, and later actions go to a different machine than the one the user opened.

Fix: fall back only when the original environment is definitely unreachable (offline/error/unsupported, or after a grace period). Also consider carrying drafts over when the route changes. Could be wrong if useWorkspaceEnvironments never reports transitional phases for already-known environments.

const UNREACHABLE = new Set(["offline", "error", "unsupported"]);
const environments = useWorkspaceEnvironments();
const ownDown = environments.some(
  (e) => String(e.environmentId) === params.environmentId && UNREACHABLE.has(e.connectionState),
);
// only compute/apply the fallback when ownDown is true

[fryuni-s-proval]

⚫️ [PROBLEM] Legacy record naming this process's own PID can block startup when the start-time probe fails

⚫ [PROBLEM] legacyOwnerIsLive never checks state.pid === process.pid. A record from a crashed pre-lock server that names our own PID is always "alive". That's common in containers, where the server is often PID 1. Telling it apart then depends entirely on ps -p … -o lstart= or PowerShell, and any probe failure returns true (line 79). Examples: ps is missing in slim/distroless images, BusyBox ps doesn't support these flags, or PowerShell takes more than 2s to start cold.

Impact: the server refuses to start with exit 78 even though no other server exists. The supervisors treat 78 as final and stop retrying, and the user is told to stop a server that doesn't exist.

Fix: a record that names the current process can't be another live owner, so short-circuit before running the external probe.

// oxlint-disable-next-line t3code/no-global-process-runtime
if (state.pid === process.pid || !isProcessAlive(state.pid)) return false;

[fryuni-s-proval]

⚫️ [PROBLEM] Not-applied guard fires during checking/error states and blocks newer updates

⚫ [PROBLEM] This guard compares (state.downloadedVersion ?? state.availableVersion) with notAppliedVersion but never looks at state.status. The check-start and check-failure reducers in updateMachine.ts keep availableVersion. So if a remote run arrives while the poller is checking, or after a check error, and the last-seen version is the one that didn't apply, the run ends right away as failed. It doesn't wait for the in-flight check, which could find a newer release, and it doesn't re-check. The web tooltip (desktopUpdate.logic.ts ~74-77) already limits the same predicate to available/downloaded.

Impact: a server-triggered update can report "did not apply" and skip a newer release the machine could have installed.

Fix: block only when the stale version would actually be acted on, and let checking/downloading/error fall through to the existing branches.

const blockedVersion = isInstallableDesktopUpdateState(state)
  ? state.downloadedVersion
  : state.status === "available"
    ? state.availableVersion
    : null;
if (state.notAppliedVersion !== undefined && blockedVersion === state.notAppliedVersion) {
  return {
    action: "done",
    outcome: "failed",
    reason: `Update ${state.notAppliedVersion} did not apply on this machine. Retry it from the desktop app there.`,
  };
}

[fryuni-s-proval]

Thanks for this one. It's a big change that spans server, desktop, mobile and web, and most of it is careful work with solid tests behind it.

Overview

Summary
The main change is a state-directory ownership lock: an exclusive SQLite transaction on server-owner.sqlite. It comes with a dedicated exit code (78) so the desktop supervisor, the WSL backend and the service launcher stop restarting a server that can never own its directory. Around that, the PR adds:

• a free-space and copy-on-write check before database backups, plus symlink-safe restores
• detection of desktop updates that didn't apply
• one shared 2000px image max-edge rule for web and mobile
• pending handoff previews
• JS REPL / computer-use tool presentation
• Codex follow-up directives and plan-delta coalescing
• relay failure details
• mobile PR environment fallback and cache keys for attachment images
• a Hyprland color-manager workaround
• a crypto.randomUUIDv4() call-site migration

The main risks are in the new lifecycle edges: quit timing, remote update gating, and PR route fallback. They're covered below.

Good points
🟢 Ownership uses an OS-released exclusive SQLite lock, and the release step only clears server-runtime.json when ownerId matches (apps/server/src/serverOwnership.ts). A crashed owner can't leave a stale lock, and a refused process can't delete the real owner's discovery record.
🟢 #handleUpdateRequest checks backup space before it writes pending state or stops the child, and the rejection says the server was not stopped (apps/server/src/serviceLauncher.ts). A full disk now refuses the update while the server keeps running.
🟢 Image fit rules now live in one module, packages/client-runtime/src/imageAttachmentSize.ts, used by both web and mobile. Images that already fit are never decoded or re-encoded, so the platforms can't drift apart on the provider limit.
🟢 describeUpdateErrorCause (apps/desktop/src/updates/DesktopUpdates.ts) redacts URLs, credentials and response bodies, bounds depth and length, and is covered by tests for cycles and unreadable errors. Updater diagnostics are now useful without leaking secrets.

Main Issues

⚫️ apps/desktop/src/backend/DesktopBackendManager.ts:71: the terminate grace is now 5s, the same as the quit-path stop({ timeout: 5s }) in DesktopApp.ts:158 and DesktopUpdates.ts:763. The app can exit before SIGKILL fires, which orphans the backend, and the next launch then hits the state-dir-owned dialog.
⚫️ apps/desktop/src/updates/remoteUpdateFlow.ts:58: the not-applied guard ignores state.status. Remote runs fail with "did not apply" while a check is running or after a check error, when they should wait or re-check for a newer release.
⚫️ apps/mobile/src/features/pull-requests/usePullRequestEnvironmentFallback.ts:15: the hook treats transient connecting/reconnecting as down and permanently rewrites route params to another environment. Review drafts keyed by environment then appear lost.
⚫️ apps/server/src/serverOwnership.ts:58: legacyOwnerIsLive doesn't check whether the recorded PID is the current process. In containers where ps is missing or limited, a stale legacy record can block startup with exit 78.

Additional Notes

• apps/desktop/src/wsl/DesktopWslBackend.ts, line 238: once ownershipRefusedRef is set, re-enabling WSL does nothing until the distro changes or the user toggles WSL off and on. Consider an explicit retry path, or mention this in the message.
• apps/web/src/components/sidebar/updateEverythingRun.tsx, line 130: the "Update did not apply" toast is deduped only in module memory, so it comes back on every launch until the version changes. Consider saving the acknowledgement in localStorage.
• apps/web/src/components/sidebar/useUpdateEverything.ts, line 183: a "skipped" server outcome settles the row as done, which differs from how interrupted provider runs are handled.
• apps/mobile/src/state/asset-url-state.ts, line 4: WorkspaceFilePreviewError doesn't handle the new missing-file-access reason, so it shows a "may be missing" message and a retry that can't succeed.
• apps/web/src/lib/imageCompression.ts, line 450: if the image header can't be parsed within 256 KiB (for example a JPEG with heavy metadata), the max-edge fit is skipped. Consider decoding the image to get its size as a fallback.
• apps/server/src/orchestration-v2/Adapters/CodexAdapterV2.ts, line 510: the logic that fills in the computer-use tool source duplicates packages/client-runtime/src/work-log/toolPresentation.ts lines 128-131. A shared helper would keep them from drifting apart.
• apps/web/src/components/ThreadStatusIndicators.tsx, line 805: the terminal-process label duplicates terminalProcessLabel in Sidebar.tsx:385.
• Test gaps: apps/server/src/serverRuntimeState.test.ts line 89 never runs the PID-reuse start-time probe. packages/client-runtime/src/handoff.test.ts line 76 doesn't cover rolled-back or unstarted runs. apps/web/src/lib/imageCompression.test.ts line 645 doesn't cover a fitted image that is still over the byte limit. apps/mobile/src/lib/composerImages.ts line 686 has no tests for downscaling on the library, pasted-file and share paths.