refactor: encapsulate Attrs/Modifiers raw bits (audit round 3, R5/A5) - #30
Merged
Merged
Conversation
… R5/A5) Attrs and Modifiers exposed their inner byte as `pub u8`, leaking the bit representation across the crate boundary. Make the field private and add a small API instead: - Attrs::new / bits / insert / remove (plus existing is_bold/… predicates) - Modifiers::new / bits (plus is_shift/is_ctrl/is_alt/is_meta) All external construction (`Attrs(x)` / `Modifiers(x)`) becomes `::new(x)`, all external `.0` reads become `.bits()`, and the VT parser's in-place SGR toggling uses insert/remove. No PROTOCOL_VERSION bump: a serde newtype serializes identically whether its field is public or private, so the postcard wire format is byte-for-byte unchanged. Rationale recorded in ADR-0009. The audit report is updated: R5/A5 done, and P5 dropped (its WebSocket-bandwidth premise does not hold — the web terminal already sends char-only text, so cell-attribute RLE would not affect it). Tests: cargo test (all crates, 26 suites) / clippy --all-targets -D warnings / fmt --check green. Generated with [Claude Code](https://claude.ai/code) via [Happy](https://happy.engineering) Co-Authored-By: Claude <noreply@anthropic.com> Co-Authored-By: Happy <yesreply@happy.engineering>
Coverage reportGenerated by |
mizu-jun
added a commit
that referenced
this pull request
Jul 5, 2026
Audit round 3: reliability, performance, and encapsulation improvements (PRs #26–#30). No protocol or snapshot version change — compatible with 1.12.0. Generated with [Claude Code](https://claude.ai/code) via [Happy](https://happy.engineering) Co-Authored-By: Claude <noreply@anthropic.com> Co-Authored-By: Happy <yesreply@happy.engineering>
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.
Implements findings R5/A5 from the round 3 audit (docs/plans/audit-round3-2026h2.md).
Change
AttrsandModifiersexposed their inner byte aspub u8, leaking the bit representation across the crate boundary. The field is now private with a small API:Attrs::new/bits/insert/remove(plus the existingis_bold/… predicates)Modifiers::new/bits(plusis_shift/is_ctrl/is_alt/is_meta)All external construction (
Attrs(x)/Modifiers(x)) →::new(x), all external.0reads →.bits(), and the VT parser's in-place SGR toggling usesinsert/remove.No PROTOCOL_VERSION bump
The audit listed this as a "next PROTOCOL bump" candidate, but it is not wire-breaking: a
serdenewtype serializes as its inner value regardless of field visibility, so thepostcardoutput is byte-for-byte identical. Bumping would create a false incompatibility. Rationale recorded in ADR-0009.Audit report updates
GridDiffto char-onlyMessage::Text(ws.rs::pty_message_to_text), so cell-attribute RLE would not touch the web path at all, and the postcard path it would affect is local IPC where the audit itself rated the gain negligible. Real web-bandwidth work would be WebSocket permessage-deflate.Tests
cargo test(all crates, 26 suites),cargo clippy --all-targets -- -D warnings,cargo fmt --checkall green.Remaining deferred audit items: P3, P6 (both need GPU profiling / a running app).