Skip to content

Audit 2026-10-04: fix fork bugs and red tests - #32

Merged
sheehanmunim merged 14 commits into
mainfrom
audit/2026-10-04
Oct 5, 2026
Merged

sheehanmunim merged 14 commits into
mainfrom
audit/2026-10-04

Conversation

@sheehanmunim

@sheehanmunim sheehanmunim commented Oct 5, 2026 •

Copy link
Copy Markdown
Member

Audit of the fork's own changes (git diff $(git merge-base origin/main upstream/main) origin/main, 1,452 fork commits). Each fix is a small, separate commit. Everything was verified on a scratch clone on the Mac: full vp run -r typecheck plus every package's test suite.

Bugs fixed

Commit Bug How it was verified
fix(voice) Codex voice dropped follow-up questions. Any ask_selected_agent call within 30 s of an answer was treated as a GPT-Live repeat, so a different follow-up got the previous answer and never reached the agent. The answer is now reused only for the same request. New test in CodexVoiceSession.test.ts: fails before the fix, passes after.
fix(notifications) Push re-registration crashed. Re-registering a push token under a new device id hit expo_push_token UNIQUE (500 from /api/notifications/device), and the stale row kept getting the pushes. The old row is now dropped in the same transaction. Reproduced with the exact SQL on SQLite: UNIQUE constraint failed before, a single row after. Typecheck.
fix(desktop) Auto-download never ran. update-available fires inside checkForUpdates while the check still holds the update reservation, so every download was refused and updates sat at "available". It now waits for the check to finish first. New test in DesktopUpdates.test.ts: fails before (available), passes after (downloading). All update tests pass.
fix(desktop) Main window could go blank. The Clerk deep-link handler loaded any <scheme>://<host>/… URL into the main window. Thread links (t3code://threads/… in non-MT builds) and arbitrary hosts blanked it. Only host app is loaded now. New case in desktopProtocolUrl.test.ts.
fix(mobile) Tablets paired as phones. A merge kept the hard-coded deviceType: "mobile" and dropped upstream's tablet detection. Existing upstream test identifies native tablets separately from phones now runs and passes.
fix(mobile) Dead theme option. The mobile theme picker listed the desktop-only MT Code palette, which normalizeMobileThemeId throws away. Separately, mobileTheme.ts pulling in expo-constants stopped 12 mobile test files from loading at all. Mobile suite went from 17 failing files to 2. The remaining 2 are Swift toolchain tests, unrelated (see below).
fix(server) Wrong Claude tier label. The tier reader fell back to ~/.claude.json even with CLAUDE_CONFIG_DIR set, so a configured account could show another login's Max tier. The tests also read the host's real file. ProviderRegistry.test.ts passes on a Max 20x machine. Before, it failed 3 tests there.
fix(scripts) Failed install swap nested the app. swap_install runs with errexit off. A failed "move aside" moved the new app inside the live bundle and still reported success. Ran the extracted function with a simulated mv failure. Before: success, with staging.app nested inside the app. After: clean failure. bash -n passes, and shellcheck shows no new warnings.
fix(scripts) Fleet refresh stopped silently. A failed fleet build left the stamped package.json versions dirty. Every later refresh then skipped with exit 0. The versions are now restored from an EXIT trap. bash -n and shellcheck. Not executed: these scripts install the app.
fix(web) Integrations settings panel crashed when no window global exists: ExternalTerminalSetting read window.desktopBridge unguarded. IntegrationsSettings.test.tsx passes.
docs(readme) Wrong install guide. "Install and first run" linked to the upstream T3 Code install guide. Read-through.

Stale tests fixed (main was red)

These are test-only changes. Each one aligns a test with intentional fork behaviour or a later upstream change; none weakens what the test checks.

  • packages/shared shell PATH order: upstream fix(shared): preserve Windows shell PATH priority pingdotgg/t3code#8748 moved the known CLI dirs to the end of the resolved PATH.
  • Default provider list: no longer expects a devin instance, matching upstream since Devin moved to the ACP registry.
  • servicePreflight message: uses resolveAppDisplayName().
  • Claude stalled-usage probe test: waited for a real 4 s deadline on a TestClock, so it hung for 120 s. It now uses TestClock.withLive.
  • Desktop launcher icon: accepts the Munim/Nightly icon.
  • Preview stalled-capture test: follows the fork's 8 s timeout plus one reattach.
  • threadActionMenu test: a merge-era override hid the item the test expects; it is removed.

Found but not fixed (for the owner)

  • Mac release entitlements: scripts/personal-codesign-mac-dmg.sh re-signs the public Mac release with osx-sign's default entitlements. Those lack com.apple.security.automation.apple-events, so in the hardened, notarized build Apple Events fail with -1743. Not changed because the fix can't be verified without signing and notarizing.
  • Stale blockmaps: the codesign step rewrites the DMG/ZIP but uploads electron-builder's blockmaps for the unsigned files, so differential updates fall back to full downloads.
  • Desktop permission handler: the new default-deny handler on the main window probably also blocks display capture (preview recording) and web notifications. Needs a live check.
  • Mixed release commits: the Windows publish builds origin/main while Mac builds local HEAD, so one tag can ship different commits. personal-release-fleet.sh also writes last-built-sha even when the publish failed.
  • Windows refresh abort: under $ErrorActionPreference="Stop" in personal-refresh-win.ps1, a launcher's stderr aborts the whole refresh.
  • Update Worker: infra/updates/src/worker.js accepts unauthenticated counter writes with an unbounded version. /stats is public and unthrottled. CLIENT_SALT falls back to a public value.
  • Linux discovery: apps/server/src/cli/desktopLaunch.ts doesn't recognise MT Code AppImage .desktop entries.
  • Dead schema: migrations 066/067 (scheduled_for, recurrence_json) have no readers or writers left after the orchestrator-V2 merge.
  • Stale docs: the Devin section of docs/internals/providers.md links to files that no longer exist.
  • Remaining failures are environment-only: invocation.test.ts, ServerEnvironment.test.ts and AntigravityAdapterV2.test.ts fail on macOS (/var vs /private/var realpath), and two mobile Swift script tests fail with Swift 6.4. All of these are unchanged upstream code. serverSettings.test.ts "reloads when a dangling settings link gets its destination" failed once under full-suite load and passed 3 out of 3 runs in isolation, so it looks flaky.

🤖 Generated with Claude Code

sheehanmunim and others added 14 commits October 4, 2026 22:03
Upstream pingdotgg#8748 moved the known Windows CLI directories from the front of
the resolved PATH to the end, and the merge updated upstream's tests but
not the fork's persistent-PATH test, so packages/shared failed on main.
The implementation is correct; the expected order now matches it.

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
The Codex voice bridge treated any ask_selected_agent call within 30 s of
an answer as a GPT-Live repeat ping and replied with the previous answer,
so a different follow-up question was never asked. Reuse the answer only
when the request matches the one it answered, and key the speak-once
dedupe on the request too, so two questions with the same answer are
both spoken.

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
self_hosted_push_devices.expo_push_token is UNIQUE, but the register
upsert only resolved a device_id conflict. A device that came back with
a new device id and the same push token failed the insert (500 from
/api/notifications/device) while the stale row kept receiving its
pushes. Drop any other row holding the token in the same transaction
before upserting, so the newest registration wins.

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
A merge kept the fork's branded label but also its hard-coded
deviceType "mobile", dropping upstream's tablet detection, so every
tablet paired as a phone in the authorized-clients list. Restore the
Device.deviceType mapping. connection.test.ts now mocks expo-constants
(pulled in by the branding module), which is what kept this regression
from being caught.

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
…eme picker

BUILT_IN_THEMES includes MT Code's palette, which by design ships to
web and desktop only, so the mobile picker listed an "MT Code" theme
that normalizeMobileThemeId discards on save. Filter it out.

mobileTheme.ts also imported the branding module (expo-constants) for
the default palette's label, which made 12 mobile test files fail to
load under Vitest. Mobile went back to T3's identity in ee2b9ff, so
the label is the literal again, and the two remaining tests that reach
branding mock expo-constants like the rest of the suite.

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
With CLAUDE_CONFIG_DIR set, Claude Code keeps .claude.json in that
directory; the file in the home directory belongs to the default login.
The tier reader fell back to it anyway, so a configured account could
be labelled with another account's Max 5x/20x tier. Read only the file
the CLI would use.

The checkClaudeProviderStatus tests read the host's real ~/.claude.json
through that path and failed on any machine signed in to a Max 20x
account; they now point CLAUDE_CONFIG_DIR at a missing directory. The
default provider list no longer expects a devin instance, matching
upstream since Devin moved to the ACP registry.

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
…ver ends

servicePreflight.test.ts still expected "T3 Code" in the launcher
message, which the fork builds from resolveAppDisplayName(). The stalled
usage probe test waited on the probe's real 4 s usage deadline under a
TestClock nobody advanced, so it timed out after 120 s; run the probe on
the live clock.

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
The Clerk deep-link handler loaded any <scheme>://<host>/... URL into
the main window, but the protocol handler only serves host "app". In
builds where thread links share the scheme (t3code://threads/...), and
for any other host a web page links to, the window went blank.
isDesktopProtocolUrl now requires host "app"; thread links stay with
DesktopDeepLink.

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
The launcher icon test expected the stock production icon, but personal
builds deliberately use the Munim or Nightly icon. The stalled-capture
preview test expected upstream's three quick capture retries, while the
fork bounds each grab at AUTOMATION_CAPTURE_TIMEOUT_MS and reattaches
once, so the test never finished.

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
ExternalTerminalSetting read window.desktopBridge unguarded, which threw
in the IntegrationsSettings test renderer and took the whole panel down;
guard it like the other desktop-only settings. Also drop a merge-era
override in threadActionMenu.logic.test.ts that hid the auto-settle item
the test then expected.

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
electron-updater emits update-available from inside checkForUpdates,
while the check still holds the update action reservation. The fork's
auto-download ran straight from that event, so tryStartUpdateAction
refused it on every check and updates sat at "available" until the
user clicked. Wait for the check to release its reservation (bounded,
like the prepared-install wait) before downloading.

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
…the old one

swap_install runs as `if ! swap_install`, so set -e is off inside it.
If moving the installed app aside failed (or ditto failed, e.g. a full
disk), the script carried on, moved the staging app *into* the live
bundle and reported the install as done. Each step now returns on
failure, restores the previous app when the final move fails, and the
caller reports the real reason.

Verified by extracting swap_install with a simulated mv failure: before,
it returned success with staging.app nested in the live app; after, it
fails with "could not move the installed app aside" and leaves the app
untouched.

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
The refresh scripts stamp release versions into four package.json files
and restore them only after a successful build. Under set -e a failed
build exited first, leaving the checkout dirty, and every later run then
skipped with "checkout is dirty" and exit 0, so the fleet silently
stopped refreshing. Restore them from an EXIT trap instead.

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
…tion

The link went to the inherited docs/user/install.md, which installs
upstream T3 Code from pingdotgg releases and t3.codes/install.sh.

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Copilot AI balanced review requested due to automatic review settings October 5, 2026 03:29

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

@github-actions github-actions Bot added vouch:trusted PR author is trusted by repo permissions or the VOUCHED list. size:L labels Oct 5, 2026
@sheehanmunim
sheehanmunim merged commit 7fde3ea into main Oct 5, 2026
7 of 23 checks passed
@sheehanmunim
sheehanmunim deleted the audit/2026-10-04 branch October 5, 2026 12:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

size:L vouch:trusted PR author is trusted by repo permissions or the VOUCHED list.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants