Audit remediation + sidebar tab/project drag-and-drop, OSC 2 title fixes#158
Merged
Conversation
Remediates the findings from AUDIT.md across all seven domains. Each was verified against the current code before fixing; a handful of Low-severity cosmetic items (WelcomeView/EmptyProjectView dedup, the palette scrim/shadow literals) were consciously left as documented acceptable exceptions. High-severity data-loss / core-feature bugs - Remote panes no longer lose their remoteness across restart: snapshotNode persists projectPath as identity (the scp spec, verbatim) and workingDirectory as a local-only respawn hint, gated on !isRemote. Same gate applied to TerminalTab.split and LayoutSerializer so a remote pane can't spawn a local shell in a remote-filesystem path. - A failed workspace/projects load no longer lets the next autosave clobber the file: distinguish absent vs present-but-undecodable, refuse to save over an unreadable file, and guard against a newer-than-current schema version. - Control-socket accepted fds now clear the inherited O_NONBLOCK (Darwin propagates it from the listener), so a read racing the client payload no longer fails as a spurious bad_request and a full send buffer can't silently truncate. - SettingsView no longer spawns a blocking `ghostty +help` subprocess inside `body`; the CLI probe is cached for the process lifetime (also off the per-reload config path). - Disabled palette hint rows stay disabled during search (isEnabled was dropped when items were rebuilt) via a PaletteItem.with(score:) copy helper. Other correctness fixes - Control socket: close-vs-use fd race, head-of-line blocking (reads moved off the accept thread) + a request size cap, poll EINTR retry, LOCAL_PEERCRED peer check (replacing the TOCTOU stat), KERN_PROCARGS2 parsed by structure. - Remote spawn: tilde-segment injection closed (only a validated ~user stays unquoted), single-quote-free wire invariant now enforced with a guard. - zmx: drain pipes to EOF before parsing; one `zmx ls` per `session list`; shared list lexer; .public on the exit-status log. - Model: post .zmxSessionsChanged after the kill; split() bails before side effects on an unknown pane; ProjectFileStore rewrites the bound file in place (case-sensitive-volume shadowing) and scans once per applyState. - App core: cmd+enter canonicalized to return (+ keypad Enter); navigateToPane resolves the window after activation; cached hotkey matchers (no per-keystroke UserDefaults reads/parses); AppState tears down its observers in deinit; terminal window identified positively (Settings is not an NSPanel); Updater moved to @observable with callbacks wired before the updater starts. - Views: title replay only on genuine catch-up (no more 250ms poll pinning); performKeyEquivalent side effects gated on first-responder; Carbon hotkey OSStatus checked with rollback; close-button interception retries until the window attaches; NSCursor.push balanced on divider disappear; QuickTerminal show() guards before mutating; O(n^2) palette index lookup precomputed. - Ghostty: guard NULL surface before the non-null C clipboard call; honor the OSC 52 write-confirm flag; cache resolved theme colors on configVersion. Conventions & hygiene - @AppStorage replaced with Preferences-backed @State throughout Settings (the banned UserDefaults.standard seam + double-writes); colors routed through new MactermTheme.warning/.success/.dimOverlay; Logger interpolations marked .public; NSEvent monitor removed on teardown; ProjectPath.currentHome for home resolution; kVK_Escape instead of the magic keycode; dead code pruned. Tests - New: ProjectSourceTests; CommandSource query-path + isEnabled regression; DirectorySource local-listing (ordering/hidden/cap) coverage; ProjectStore mutators + round-trip. - Fixed: tempdir-inject every defaulted store; KilledSessions waits on an expected count (not "anything"); ZmxEnvironment env save/restore + .serialized; LayoutReconciler fixtures throw instead of try!. Scripts & CI - GhosttyKit cache: dropped the restore-keys fallback that re-laundered stale content under each fresh weekly key. - release.yml fails on a malformed tag instead of shipping v0.0.0. - Pinned mise tool versions; swiftlint/swiftformat now cover CLI/ and MactermTests/; removed stale `Tests` excludes. - publish-appcast.sh: idempotent appcast insert, per-clone git identity, nullglob-guarded DMG loop; benchmark.py matches the launched process exactly; errexit-safe status capture in _lib.sh.
github-actions
Bot
added
area:ui
…senberg-cc456c # Conflicts: # Macterm/Views/SplitTreeView.swift
github-actions
Bot
added
benchmark:regression
Contributor
Window-state benchmark
🎉 Labeled
|
Resolves the two PR gates and the findings the reviewer flagged as unaddressed. Lint (gate 1) — resolve the swiftformat/swiftlint conflict the widened scope exposed - SwiftFormat and SwiftLint fought over trailing commas on single-element multiline collections (`[Foo(\n…\n)]`): SwiftFormat omits the comma there, SwiftLint's `trailing_comma` (mandatory) demands it, so each tool's fix was the other's violation. Ceded trailing commas to SwiftFormat (removed the SwiftLint rule) — one tool per concern. - Disabled `optional_data_string_conversion`: it pushes toward the failable `String(bytes:encoding:)`, but the codebase deliberately uses the lossless, non-optional `String(decoding:as:)` (§5.8's argv parse must not drop bytes). - Test-idiom relaxations via a nested MactermTests/.swiftlint.yml (force_unwrapping, large_tuple) and an identifier_name allowlist for the documented H/V tree DSL. Renamed pty `master`/`slave` → `primary`/`secondary` (real fix, not a suppression). `mise run lint` is now clean. RemoteSpawn security tests (gate 2) - Cover the tilde-injection guard (§5.3: `~$(cmd)`, `~`id`` , spaced usernames are fully quoted, never shipped bare), the single-quote-free wire invariant (§5.4: a `'` in a user field swaps the script for a quote-free diagnostic), and end-to-end paneCommand/opArgv quote-freeness. Remaining findings - 1.3 moveTab: moved panes now restamp their routing projectID to the destination (session identity — name/host/path — stays put, so a moved remote pane still tears down over ssh). Fixes notification-click navigation after a move. Added a test asserting routing-vs-session split. - 1.4 NotificationHandler: dropped @preconcurrency; the delegate is now nonisolated and hands only Sendable values (String/Bool) to a @mainactor Task, keeping Swift 6 concurrency checking on. - 1.6: window opacity/blur/glass setters post a lightweight .mactermConfigDidChange instead of regenerate() + full libghostty reload — the reload was only ever used to piggy-back that notification, and fired continuously while dragging a slider. - 2.1: only mark a title delivered when a callback actually received it. - 1.16: the fixed Cmd+N re-front now runs AFTER the configurable hotkey matches (like Cmd+1-9), so a user rebinding to cmd+n wins instead of being shadowed. - Fixed a stale RemoteSpawn comment that claimed profiles are sourced (they are deliberately never sourced).
github-actions
Bot
removed
the
benchmark:regression
The §1.8 change stamped `NSWindow.identifier` on the SwiftUI WindowGroup window (in didBecomeMain) to identify the terminal window positively. Mutating a SwiftUI-managed window's identifier is unsupported and corrupts its window management: the responder's `weak var mainWindow` went stale, so the `keyWindow !== mainWindow` gate in MainAppResponder.handle mis-fired for EVERY key event — Cmd+D (and every other hotkey) fell through to the terminal (ignored), and Cmd+W hit the retarget branch and closed the window instead of the pane. Fix: never stamp the SwiftUI window. Identify the terminal window by the pointer `didBecomeMain` already caches (the first window to become main) — which is the authoritative identity — with the non-panel heuristic only as a last-resort fallback before that pointer exists. This keeps §1.8's goal (don't mistake the Settings window, a plain NSWindow, for the terminal window) without touching SwiftUI's window. BenchmarkControl reads the same cached pointer. Verified in the running debug app with real keystrokes: Cmd+D splits the pane (2→3), Cmd+W closes the pane (3→2) with the app and window still alive. Also fills the coverage gap that let this class of bug ship: adds tests for the static `HotkeyRegistry.matches(event:action:)` cache path (default bindings + rebind invalidation), which nothing exercised before.
Collapse the defer's restore branch into a single `??` expression — swiftlint's statement_position rule rejects the else on its own line. mise run lint clean.
removePane destroyed the pane's surface before checking panes.count / the removing() result, so a precondition-shaped path could tear down a surface and still return. Harmless with today's only callers (closePane routes .onlyPaneLeft into closeTab; the quick terminal drops the whole tab), but the ordering is a latent trap. Move destroySurface() to after the count check and after the removing() guard, in each real branch. Behavior is unchanged — the surface is still destroyed for both .onlyPaneLeft and .removed (the quick-terminal caller relies on that) and never for .notFound — but the irreversible teardown now follows the outcome decision instead of preceding it. Result cases stay covered by TerminalTabTests. (4.16 left as-is by design: the verification recommended keeping TerminalSearchState's Combine switchToLatest debounce — the right operator for cancel-on-latest — and the finding's actionable part, naming the 3-char/300ms constants, already landed.)
github-actions
Bot
added
benchmark:regression
Moving a tab across projects existed only via the right-click "Move to Project" menu; dragging a tab in the sidebar did nothing useful. SwiftUI List's .onMove is per-section and can't express a cross-section move, so this replaces it with a Transferable payload plus per-project drop targets: dropping a tab into a project's list reorders (same project) or moves it (another project) at the insertion offset, and dropping on a project header appends — the only path for a collapsed/empty project, which also spring-opens while hovered. The moved TerminalTab (and its live surfaces/shells) is reused as-is; only IDs travel in the drag payload and the live tab is looked up on drop. moveTab/adoptTab gain an optional destination index, and a new reorderTab covers the same-project drop. The per-project section is extracted into helper subviews so each drag/drop closure type-checks in its own scope (inlined, the whole List blew the solver's time budget).
…argv Two title-pipeline bugs, both verified live: 1. zmx wrapper leaking as the tab name / run: command. For a zmx-wrapped pane, libghostty's foregroundPID is the local `zmx attach` CLIENT; the real shell runs under the daemon, reachable only via ZmxForegroundResolver (by session name). In the async-registration window right after spawn/restore — before `zmx ls` populates the resolver cache — foregroundPID fell back to the client pid, so the tab read `zmx` and, worse, Save Layout captured the wrapper argv (`… zmx attach macterm-… /usr/bin/login … nu …`) as a `run:` command. Fix: for a wrapped pane, foregroundPID returns nil on a cache miss instead of the client pid — the pane reads as resolving (falls back to the shell name, saves no run) until the cache catches up. The client-pid fallback is kept only for UNWRAPPED panes (zmx unbundled / over the socket-path budget), where it's the correct answer. Verified: spawn+immediate-save no longer writes a zmx run:. 2. Claude Code's version shown as the tab title. Claude Code sets process.title = its version, which overwrites p_comm — so `comm` reads `2.1.202` — AND emits the same string as an OSC 2 title. Both fed the tab name. Fix: runningProcessName falls back to the exec_path basename (`claude`) when comm is version-shaped, and receiveReportedTitle discards a bare-version OSC title. A real (non-version) status title still adopts. Verified live: the claude tab now shows `claude`, not `2.1.202`. Tests: PaneTitleTests covers the version-title gate + looksLikeVersionString. (Pre-existing user project files that already captured a zmx `run:` self-heal on the next Save Layout.)
…name
A program-set OSC 0/2 title (e.g. Claude Code's "✳ Claude Code") showed
briefly, then reverted to the process name ("claude") on the next poll — where
vanilla ghostty keeps the title. Root cause: a pid-source mismatch for
zmx-wrapped panes.
`receiveReportedTitle` pins `programTitlePID` to the RESOLVED foreground pid
(`foregroundProgramPID` → the daemon-side shell/program via
ZmxForegroundResolver). But `refreshForegroundProcess` passed
`nsView.foregroundPID` — the local `zmx attach` CLIENT pid — as the value
`applyForegroundRefresh` compares against to expire the title. For a wrapped
pane those two pids are ALWAYS different, so the just-adopted title expired on
the very next 250ms poll.
Fix: `refreshForegroundProcess` now passes `ProcessInspector
.resolvedForegroundPID(forPane:)` (newly exposed) — the same resolver the pin
uses — so a title survives as long as its program holds the foreground. The
provenance gate is unchanged, so shell prompt-titles (nushell's cwd) are still
rejected; only genuine program titles persist.
Verified live: a `claude` tab now shows "✳ Claude Code" persistently; a plain
`nu` tab still shows "nu" (not a cwd/prompt title).
github-actions
Bot
added
benchmark:improvement
MainAppResponder gates hotkeys behind "is a DIFFERENT window (Settings, an alert) key?" via `keyWindow !== mainWindow`. But `mainWindow` (a weak ref set on the first `didBecomeMain`, which fires AFTER responders install) is briefly nil at launch — and a nil pointer is `!==` every real window. So during that window the terminal window itself was treated as "different": Cmd+W hit the retarget branch and closed the window, Cmd+D fell through and was ignored, while plain typing (which doesn't go through this gate) worked. Pre-existing, but easy to hit right after opening the app. Fix: require a KNOWN mainWindow to enter the branch (`if let main = mainWindow, keyWindow !== main`). When mainWindow is unknown, fall through to normal handling — the terminal window is the only window that can be key that early, so acting on its workspace is correct. No change to the real "Settings is key" case: when mainWindow is set and differs, behavior is identical. Verified live: at launch Cmd+D splits the pane and Cmd+W closes a pane (window survives, app stays alive).
The sidebar tab drag did nothing: the project ForEach's `.onMove` put the whole List into reorder mode, and List's reorder gesture hijacks a row's `.draggable`, so the tab rows' Transferable drag never started (and the `.dropDestination`s never received anything). Fix (chosen approach: drag everything): remove the List `.onMove` and reorder projects by dragging the project HEADER — a new `MovableProject` Transferable — instead. With both projects and tabs on the Transferable drag path, they coexist: tab rows drag (reorder within / move across projects), project headers drag (reorder), and headers accept both payloads via stacked `.dropDestination`s (tab → append into the project; project → reorder to that slot). The declared `com.thdxg.macterm.project-move` UTType is added to Info.plist alongside the tab-move type. The right-click "Move to Project" menu stays as a fallback. The move/reorder data layer (moveTab across projects with pane-identity restamp, reorderTab, adoptTab(at:)) is unchanged and unit-tested; only the drag PLUMBING changed. NOTE: the drag GESTURE itself couldn't be verified headlessly (no Quartz/cliclick to script a mouse drag) — needs a manual check in the app.
`.terminalPollEvent` fires often under a busy workload — every keystroke, output/execution transition, OSC title, and tab switch — and each fire ran reschedulePoll, which unconditionally invalidated the pending Timer and built a fresh one (new Timer + RunLoop.add) even when the computed delay was identical. Pure churn on the hot path. Track the delay the timer was scheduled with and skip the teardown/rebuild when a valid timer already has that cadence. The one-shot timer self-invalidates on fire, so a fired (invalid) timer still rebuilds correctly; a nil delay tears the timer down as before. Cadence values are unchanged (PollCadence.nextDelay is untouched and still covered by PollCadenceTests) — only the rebuild frequency drops. This is the one poll-path item from the audit (§1.5b) that hadn't landed; folding it in now also trims per-event work under the workload the window-state benchmark exercises.
Owner
Author
|
On the The three
Focused regressing while the identically-loaded unfocused/minimized states improve, and memory dropping everywhere, is the signature of a busier runner on the focused sample — exactly the cross-run variance the report footnote warns about (single 30s sample, different shared runners). I also traced the hot paths this branch touches and none add per-tick work — the KERN_PROCARGS2 argv parse now stops after That said, I folded in the one poll-path optimization from the audit that hadn't landed ( 🤖 Addressed by Claude Code |
github-actions
Bot
added
benchmark:improvement
… drain High — the O_NONBLOCK fix traded a truncated response for a potential indefinite main-actor hang: a client that sends a request then stops reading would block write(2) on the main actor for any response over the ~8KB AF_UNIX send buffer. Set SO_SNDTIMEO next to SO_RCVTIMEO so the write loop's EAGAIN branch terminates the send (truncating only for a client that isn't reading), and log the truncation. Medium: - Regression tests for the three headline data-loss fixes: snapshotNode persists a remote pane's scp spec verbatim with nil workingDirectory (and a remote pane round-trips as remote); both stores' save() refuses to overwrite after a corrupt-file load, preserving the bytes. - OSC 52 confirm prompts no longer stack: one alert at a time (a looping TUI can't pile up modal dialogs), and the prompt now previews the content. - ZmxClient's post-exit stdout drain no longer blocks forever: it awaits the readabilityHandler's own EOF signal, bounded at 1s, instead of a blocking availableData loop that a descendant holding the write end would wedge. - Cap concurrent control connections with a semaphore so stalled clients can't exhaust the GCD pool; over-cap connections are refused fast. Low: - Op-path (kill/probe) single-quote violation now exits non-zero instead of dropping to a shell that exits 0 — an honest failure, not a silent no-op. Pane path still drops to a shell (a bare exit would vanish the pane). - Soften the acceptLoop close-race comment: stop()'s 1s fallback narrows the race to a wedged-accept-thread window rather than eliminating it. The silent save-refusal surfacing (a user-facing warning) is deferred to a focused follow-up rather than grow this PR further; the refusal itself is the correct default and is now tested.
thdxg
changed the title
Owner
Author
|
Thanks for the thorough review — all findings addressed in b445a1e (title/body also updated to reflect the real scope). High #1 — O_NONBLOCK fix trades truncation for a main-actor hang. Fixed. Set Medium #2 — no regression tests for the headline data-loss fixes. Added: Medium #3 — OSC 52 prompts don't coalesce. Fixed: a Medium #4 — zmx drain can block a pool thread forever. Fixed: replaced the blocking Medium #5 — unbounded connection queue. Fixed: a Low — op-path quote diagnostic exits 0. Fixed: Low — "close-race removed entirely" comment. Softened to note Deferred (deliberately): the silent save-refusal surfacing (a user-facing warning) — the refusal itself is the correct default and is now tested, but wiring a new alert would grow this already-over-scoped PR; I'll do it as a focused follow-up. The over-cap "empty or unreadable request" message is left generic (the real reason is logged) for the same reason. Not changed: the empty- 🤖 Addressed by Claude Code |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Overview
This branch started as codebase-audit remediation and grew two adjacent pieces of work. Since it squash-merges, the scope is spelled out here.
1. Audit remediation (the original scope)
Data-loss, race, perf, and convention fixes across the codebase:
projectPathis identity/verbatim,workingDirectorya local-only hint, gated onisRemotein snapshot/split/serializer); a corrupt persistence file is no longer clobbered by the next autosave (bothWorkspaceStoreandProjectStorelatchloadFailedand refuse to save); control-socket accepted fds clear inheritedO_NONBLOCK(no more spuriousbad_request/truncation).~$(cmd)command-injection hole inRemoteSpawnis closed (strict username allowlist), and the single-quote-free wire-format invariant is now runtime-enforced pre-shellQuote.body..publicon Logger interpolations,@Observablethroughout (incl.Updater),MactermThemefor colors,FocusRestorationfor reshape-adjacent focus, tempdir-injected tests, plus CI/script hardening (cache stale-loop, release-tag hard-fail, nullglob, per-clone git identity).2. Sidebar tab/project drag-and-drop (new feature)
Drag a tab to another project (or reorder within one), and drag project headers to reorder — native
Transferablepayloads (MovableTab/MovableProject), exported UTTypes,adoptTab(at:)/moveTab(toIndex:)/rebind(projectID:). Replaces the List.onMove(which hijacked row drags). The right-click "Move to Project" menu remains as a fallback.3. OSC 2 title + hotkey fixes
claude's) is kept instead of snapping back to the process name;zmx/version-string titles are resolved to the real program name.Review follow-ups addressed
SO_SNDTIMEOon the accepted control-socket fd (theO_NONBLOCKfix otherwise traded truncation for a main-actor hang).ZmxClientdrain bounded (no forever-block); control-connection concurrency cap; op-path quote-violation exits non-zero.Testing
mise run format,lint,testall green. Drag-and-drop gesture verified manually (can't be scripted headlessly).🤖 Generated with Claude Code