fix: address config, SIXEL, session, and IPC audit findings - #82
Conversation
Add a per-process `serverInstanceId` to the `noa.hello` response (serde default, protocol version unchanged). The remote-attach connection manager remembers the instance it first attached to; when an automatic reconnect reaches a different instance it drops the control socket before `reserve_attach`/`resize_pane` and publishes `Detached`, so a pane id reused after a restart never receives input meant for the old pane. A manual retry adopts the new instance. Claude-Session: https://claude.ai/code/session_01YTGJ217L3Zc1eZAGeyftpm
Repeated `!N` overdraw followed by `$` could run hundreds of millions of pixel writes under the terminal lock without growing the image. Charge each non-blank sixel column against a work budget of twice the largest admissible image and reject with `TooBig` once exceeded. Claude-Session: https://claude.ai/code/session_01YTGJ217L3Zc1eZAGeyftpm
Two processes sharing `session.json.tmp` could interleave a truncating write with the other's rename and publish a corrupt file. Stage in `.session.json.<pid>.<seq>.tmp` opened with `create_new`, sync, then rename; remove the staging file on failure. Claude-Session: https://claude.ai/code/session_01YTGJ217L3Zc1eZAGeyftpm
Concurrent initializers that both saw an empty token file raced their `remove_file` calls and could delete the token the other had just published. Hold an exclusive `File::lock` on `<token>.lock` across read → empty-file recovery → publish; fall back to unlocked with a warning if the lock cannot be taken. Claude-Session: https://claude.ai/code/session_01YTGJ217L3Zc1eZAGeyftpm
…, dangling symlink writes - `ConfigOverrides.theme` is a single tri-state `Option<ThemeSetting>`; a later single name clears an earlier light/dark pair and vice versa, across parse, merge and apply. - An empty `font-family*` value resets the list (Ghostty parity) instead of emitting a diagnostic; the settings panel writes that reset line when a family had been configured. - A non-empty `font-family*` update replaces the primary slot (first entry after the last reset) so fallbacks are kept. - `write_config_updates` resolves symlinks via `symlink_metadata`/ `read_link` (relative targets, 40-hop cap), so a link whose target does not exist yet writes the target and keeps the link. Claude-Session: https://claude.ai/code/session_01YTGJ217L3Zc1eZAGeyftpm
Append font-family resets after later includes so child font entries cannot survive a reset. Cover all family variants and repeated saves.
Sparse color passes paint disjoint rows of a sixel band. Charge set bits so valid multicolor images fit their pixel budget while repeated overdraw remains bounded.
Advance scripted server instances only for Attach connections so backfill worker timing cannot change reconnect counts or the simulated restart.
Apply rustfmt to the existing audit changes so the workspace passes the CI formatting check.
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 39277b478b
ℹ️ 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".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| scheduled_retry = true; | ||
| continue; | ||
| }; | ||
| attached_instance = Some(control.server_instance_id()); |
There was a problem hiding this comment.
When the server restarts after an attachment but before its scheduled scrollback fetch, attached_instance remains local to the connection manager while run_scrollback_worker opens an independent control connection without comparing its server_instance_id. If the old attach has not detected its disconnect yet, the generation check still succeeds and scrollback from the new server's reused pane ID is merged into the terminal; the later detach does not remove that unrelated history. Share the expected instance with the worker and reject a mismatch before get_scrollback or merging.
Useful? React with 👍 / 👎.
1 participant
Prevent automatic attachment to reused pane IDs after a server restart, bound SIXEL rendering work, and make configuration and persistence updates reliable.
Compatibility
The
serverInstanceIdhello field is additive. Older servers that omit it retain the previous reconnect behavior. Token provisioning leaves aserver-token.lockfile beside the token; filesystems that cannot acquire the advisory lock retain the existing unlocked behavior with a warning.Validation
cargo test --workspace— 2,625 passed, 15 ignored, on macOS outside the sandbox (shared-memory tests require this).cargo test -p noa-config --lib writer::tests— 27 passed.cargo test -p noa-grid --lib sixel::tests— 15 passed.cargo test -p noa-app --lib remote_attach::tests— 50 passed.cargo build --workspace --lockedcargo fmt --all -- --checkgit diff --check