Key pageTabs by GObject pointer to fix silent map lookup misses - #5
Merged
Conversation
gotk4 may return a fresh Go wrapper from getter calls like view.SelectedPage() and view.NthPage() that doesn't pointer-equal the wrapper inserted via view.Append. The pageTabs map was keyed by *adw.TabPage, so lookups against a freshly-returned wrapper would silently miss — even though both wrappers reference the same underlying GObject. PR #3 worked around this in renameActiveTab by iterating tabPages and comparing via page.Native(); PR #4 added a second copy of the same workaround in activeSession. Beyond those two, eleven other pageTabs[page] sites were silently failing soft on miss: - selected-page notify handler — sess.da.GrabFocus() skipped on miss, so focus didn't reliably land on the new terminal after Ctrl-1/2/3 / clicking a tab. - close-page handler, project-id resolver, tab-cleanup paths — on a stale lookup these returned 0/false and let the calling branch fall through silently. Fix: change pageTabs from map[*adw.TabPage]int64 to map[uintptr]int64, keyed by the underlying GObject pointer (page.Native()) which is stable across wrapper churn. Add a pageKey() helper and route every read/write/delete through it. Collapse renameActiveTab and activeSession to direct map lookups. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThe Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
charliek
added a commit
that referenced
this pull request
May 23, 2026
…ail-hard, entitlements cleanup, PtySupervisor double-emit Address four findings from the M6-M9 sub-agent review: **1. Linux reorder events for parity with Mac.** Sub-agent #8 caught that the Mac M9 polish wired `Workspace.tabsReordered` / `projectsReordered` events but Linux's `crates/roost-linux/src/daemon/state.rs::reorder_tabs` / `reorder_projects` mutate positions in-place without emitting. Added two new `WorkspaceEvent` variants (`TabsReordered`, `ProjectsReordered`) with the same payload shape as Mac (the post-reorder display order — supplied prefix + sorted unlisted). Both reorder methods now `events.send(...)` after persist. The GTK app.rs match arm currently drops the events with a comment — the UI's drag-reorder path already updates AdwTabBar inline before firing the RPC, so re-applying the broadcast would be double-work. Cross-client convergence is a follow-up slice; the event is emitted so a future `events.subscribe` consumer (or a sibling GTK process) can react. **2. `bundle.sh` codesign fail-hard.** Sub-agent #5 caught that the previous `codesign … || echo "warn ... (continuing)"` form swallowed signature failures with exit 0. A botched signature silently produces a Gatekeeper-rejected app + a notarization failure at release time. Replaced with a `codesign_or_die` helper that exits 1 on failure unless `ROOST_ALLOW_UNSIGNED=1` is set (for the rare dev case where Xcode CLT codesign is unavailable). **3. `Roost.entitlements` cleanup.** Sub-agent #6 caught that `com.apple.security.cs.disable-library-validation` was included based on a misreading of the docs — library validation governs in-process `dlopen` / framework loads, not `execve` of a separately-signed embedded binary (the `Contents/Resources/bin/ roostctl` exec path is governed by Gatekeeper / quarantine, not library validation). Apple notarization will scrutinize any unnecessary hardened-runtime entitlements, so removed. Plist is now an empty dict with a long comment block listing what we INTENTIONALLY don't need (allow-jit, network.*, sandbox) and the conditions under which we'd add each. **4. PtySupervisor double-`.tabExited` race.** Sub-agent #3.b walked through a scenario where the read source yields `.eof` and the bg-reap teardown yields `.forcedExit` onto the same AsyncStream FIFO; the drain task would emit `.tabExited` twice (once from `reapAndCleanup` on `.eof`, once from `.forcedExit`). Added an `emittedExit` bool to the drain task that suppresses the second emit; `reapAndCleanup` is now `@discardableResult -> Bool` so the drain knows whether it actually emitted (vs. the session-already-gone-from-close-race case where it returns false without emitting and the subsequent `.forcedExit` is the one that should fire). All emit-once paths preserved. `swift test`: 132/132. `cargo test -p roost-linux`: 20/20 state-machine tests + 34 IPC-dispatch + the rest still green. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
charliek
added a commit
that referenced
this pull request
May 23, 2026
* fix(linux): Linux IPC robustness sweep (closes #80) Address the deferred CR items from the inline-core refactor (PR #78). Reconciled against the merged code: #1 (parse-error reply), #10 (pending HashSet), #13 (smoke-test ordering) were already done; the rest are fixed here. Project is early-stage, so the bar is long-term correctness. PTY supervisor (crates/roost-linux/src/daemon/pty.rs, tab_session.rs): - #11: wait task now removes the session on child exit (sessions behind Arc<Mutex>), so writes to a dead PTY return NotFound instead of silently succeeding. - #12: close() escalates SIGHUP -> SIGKILL after a grace period. The cloned portable-pty ChildKiller only sends SIGHUP, so a shell that traps/ignores it used to outlive close(); a reaped flag + pid watchdog force-kill it, mirroring the Mac teardown. - #8: send_input/send_resize feed one per-tab serial channel drained by a single task, so keystrokes can't reorder (the old per-call tokio::spawn could under the multi-thread runtime). Workspace state (crates/roost-linux/src/daemon/state.rs): - #4: new project/tab position uses max(position)+1, not len()/count(), so a delete-then-create can't collide. - #5: every mutator now publishes its event while still holding `inner`, so broadcast order matches commit order; persist runs after the drop. The events-resync Resync path is preserved. Server framing (crates/roost-ipc/src/server.rs): - #2: on an envelope decode failure, peel `id` from the raw JSON (string- or number-encoded) so the parse-error reply lands at the client's id, not 0. Lifecycle (crates/roost-linux/src/main.rs, ipc.rs, app.rs, messages.rs): - #7: IPC bind is now a synchronous startup requirement; bind failure aborts startup instead of leaving the UI socket-less. - #6: a second launch dials the new `app.activate` op; the handler forwards to the GTK thread which raises the running window. - #9: events.subscribe returns not-implemented (Linux + Mac) instead of a false {} ACK, so clients don't wait forever. Real streaming is deferred to its first consumer (roostctl watch). Tests: write-after-exit -> NotFound, SIGHUP-ignoring child force-killed, send_input ordering, position max+1 after delete, id-preserving parse error, bind-failure surfaced, events.subscribe not-implemented. Rust workspace + Mac build/clippy/tests green. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * test(mac): IPC handler dispatch harness (#80 follow-up) The Rust handler has tests/ipc_dispatch.rs; the Mac IPCHandlerImpl had no equivalent, leaving its hand-written cross-cutting logic untested — strict unknown-field rejection (decodeParams), ipcDim u16 validation, mapWorkspace/mapPty error-code mapping, the not-implemented/unknown-op paths, and result encoding. The two handlers must stay behaviorally convergent over the shared wire contract, so this guards that. Calls IPCHandlerImpl.handle(op:params:) directly — no socket. Exercises only non-PTY-spawning ops to avoid the forkpty/swift-testing SIGTRAP that disables the PTY paths elsewhere (tab.open stays on the manual pass; the error-mapping ops reach the supervisor only on the lookup-fails path). Seeds 8 tests: events.subscribe→not-implemented (the #9 path), unknown-op, unknown-field strictness, project.rename→not-found, tab.resize out-of-range→invalid-param, tab.resize missing→not-found, identify profile echo, project.create→tab.list round-trip. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * fix(linux): address CodeRabbit review on #80 sweep Resolves 4 of CodeRabbit's 6 findings on PR #86 (the persistence-ordering finding is pre-existing/orthogonal and tracked separately). pty.rs: - Major: spawn() acquired the master reader/writer AFTER spawn_command, so a try_clone_reader/take_writer failure returned with a live child and no wait task — an orphaned PTY. Acquire reader+writer BEFORE spawning, making spawn_command the last fallible step (no orphan). - Major: the exit wait task removed the session by tab_id alone, which could evict a newer session that reused the same tab_id (close() frees the slot synchronously; pty_smoke reuses id 42). Gate the remove on Arc::ptr_eq of the per-spawn `reaped` identity so only the owning waiter deletes. (My prior "monotonic ids" comment over-assumed caller discipline; the supervisor can't rely on it.) - Major: input and resize were unified at TabSession but re-split into the supervisor's two channels (select!), so mixed input/resize wasn't FIFO end-to-end. Merge them into one WriterCmd channel drained by a single ordered loop — genuine submission-order delivery. ipc.rs + messages.rs: - Minor: app.activate ACK'd any payload. Add an empty strict AppActivateParams and decode it so the op validates its envelope (rejects unknown fields) like every other op. IPCHandlerTests.swift: - Minor: replace `as? T != nil` (SwiftLint prefer_type_checking) with a meaningful empty-array assertion on a fresh project's tabs. Rust clippy --all-targets clean; roost-ipc + roost-linux tests green (A3 ordering + dup-spawn still pass under the new channel/guard). Mac swift build + 140 tests green. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * fix(linux): order state.json persistence by commit seq (CR #3) Resolves the last CodeRabbit finding on PR #86. persist_async did synchronous file I/O on the caller thread after dropping `inner`, so two concurrent mutators could write state.json out of commit order — a slow earlier commit could clobber a newer one, regressing restart durability. Keep persistence synchronous (state_persist.rs and restart-reload depend on it; an async writer thread would break that), but make it ordered: - Inner.persist_seq: a monotonic commit counter bumped under the lock when a snapshot is taken, so the seq reflects commit order. - snapshot_for_persist now returns (SnapshotFile, seq). - persist(seq, snapshot) serializes on a new persist_guard mutex and drops any snapshot whose seq is <= the highest already written, so the newest committed snapshot always wins regardless of which thread races first. Newest-wins verified by persist_drops_stale_out_of_order_writes. Rust clippy --all-targets clean; roost-ipc + roost-linux suites green (incl. state_persist). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * style: rustfmt ipc.rs import block The AppActivateParams import addition left the use block un-reflowed; CI's rust-lint (cargo fmt --all -- --check) caught it. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
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.
Summary
pageTabsis now keyed by the underlying GObject pointer (page.Native()) instead of the Go wrapper*adw.TabPage, so every site lookups reliably.renameActiveTab) and PR Terminal usability: clipboard, scrollback, mouse, modifier keys #4 (activeSession). Both collapse to a direct map lookup via the newpageKey()helper.Why
gotk4 may return a fresh
*adw.TabPageGo wrapper from getter calls likeview.SelectedPage()andview.NthPage()— even though it references the same underlyingAdwTabPageGObject inserted earlier viaview.Append. WithpageTabs map[*adw.TabPage]int64, lookups against the fresh wrapper silently missed.PR #3 hit this when wiring
Cmd-Rrename and worked around it with an iteration. PR #4 hit it again inactiveSessionand copied the workaround. Each one-off fix unblocked the immediate feature but left the broader bug class in place.Sites that were silently failing soft
After grepping every
pageTabs[page]access, these are the ones that previously returned(0, false)on a wrapper mismatch and just took a fallback path:selected-pagenotify handler —sess.da.GrabFocus()was skipped on miss, so focus on tab switch (Ctrl-1/2/3or clicking a tab) didn't reliably land on the new terminal.close-pagehandler — silent skip of session cleanup on miss.projectIDForPage— fell through toactiveProjectIDinstead of the real owner.After this PR, every lookup goes through
pageKey(page) = page.Native()and these silent failures are gone.Test plan
go vet ./...cleango test ./...passes (config, core, ghostty, ipc, osc, pty, store, and the cmd/roost pure-logic tests)./build/build.shproduces working binary./roostlaunches without panic; IPC socket bindsCtrl-1,Ctrl-2,Ctrl-3. After each switch, type immediately — characters should appear in the new tab's terminal without needing to click.Cmd-Rrename popover still works.Cmd-Wclose still works without leaks.The change is mechanical (key type swap, every site routed through one helper). It can only resolve silent misses, not introduce new ones —
Native()returns a stable identity, which the existingactiveSessionworkaround already relied on.🤖 Generated with Claude Code
Summary by CodeRabbit