diff options
| author | rottedfm <rottedfm@proton.me> | 2026-08-19 11:21:47 -0400 |
|---|---|---|
| committer | rottedfm <rottedfm@proton.me> | 2026-08-19 11:21:47 -0400 |
| commit | ea0bd36167b684c0accdb5ce2b2e21b8d84aeb25 (patch) | |
| tree | be1267972b5de2f1ae592577dfabce67f1fe6e87 /docs/reviews | |
| parent | 8c0b4c53b130555f040884c1f52b90f16b23e241 (diff) | |
feat: implement Helix-style modal buffer under DO-178C DAL-C
The repository was an unmodified ratatui component template: no editor
code, JSON5 config, and placeholder widgets. This establishes the first
working baseline — `moji <file>` opens a file into a ropey rope and edits
it with Helix selection-first semantics.
Requirements, implementation and tests land together because they must:
the traceability check rejects requirements with no implementation and
tests naming requirements that do not exist, so neither half is a valid
commit on its own.
Package renamed to mojibake-editor (mojibake was taken on crates.io);
binary is moji, library target stays mojibake.
Class: New behaviour
Requirements: MJB-HLR-001..019, MJB-LLR-001..205
Derived: MJB-DR-001..007 (DR-001 resolved, six open for review)
Verified: cargo build; clippy --all-targets -D warnings clean;
cargo test 294 passing; ./scripts/check-trace.sh 98/98/98;
cargo package clean
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Diffstat (limited to 'docs/reviews')
| -rw-r--r-- | docs/reviews/code-checklist.md | 202 | ||||
| -rw-r--r-- | docs/reviews/library-selection.md | 98 | ||||
| -rw-r--r-- | docs/reviews/requirements-checklist.md | 112 |
3 files changed, 412 insertions, 0 deletions
diff --git a/docs/reviews/code-checklist.md b/docs/reviews/code-checklist.md new file mode 100644 index 0000000..ce16b46 --- /dev/null +++ b/docs/reviews/code-checklist.md @@ -0,0 +1,202 @@ +# Source code review checklist + +Software Level: **DAL-C** (DO-178C) +Reviewer: author — **review without independence**, which DO-178C permits at +Software Level C. +Scope: `src/buffer/**`, `src/components/buffer.rs`, `src/config.rs`, +`src/app.rs`, `src/cli.rs`, `src/action.rs` + +Derived from DO-178C Table A-5 (verification of the source code), reduced to +the objectives that apply at DAL-C. + +--- + +## Compliance and traceability + +| # | Check | Result | +|---|---|---| +| 1 | Source complies with the low-level requirements | **Pass** | +| 2 | Source is traceable to the LLRs | **Pass** — 98/98 tagged `// MJB-LLR-nnn`, verified by `grep`, not inspection | +| 3 | Source complies with the software architecture | **Pass** — buffer core has no `ratatui` dependency and is testable headless | +| 4 | Source is verifiable | **Pass** — 96.21% statement coverage of `src/buffer/**` | +| 5 | Source conforms to a coding standard | **Pass** — `cargo clippy --all-targets -- -D warnings` is clean | +| 6 | Source is accurate and consistent | **Pass** — see the defects found, below | + +Reproduce objectives 2, 4 and 5: + +```bash +cargo clippy --all-targets -- -D warnings # clean +cargo test # 294 passing +cargo llvm-cov --summary-only test +``` + +--- + +## No dead code + +| # | Check | Result | +|---|---|---| +| 7 | Removed widgets leave no unreachable code | **Pass** | +| 8 | No `allow(dead_code)` masks anything | **Pass** | +| 9 | Removed dependencies are gone from the graph | **Pass** | + +Evidence: + +```bash +# The widget modules are deleted, not merely unregistered. +ls src/components/ # buffer.rs only +grep -rni "FpsCounter\|mod fps\|mod home" src/ # only a comment in app.rs + +# No dead-code suppression anywhere in the crate. +grep -rn "allow(dead_code)" src/ # nothing + +# The chord-timeout state is gone, not just unused. +grep -rn "last_tick_key_events" src/ # nothing + +# JSON is out of the dependency graph, not merely out of the call path. +cargo tree | grep -iE "json|yaml|ini\b" # nothing +``` + +Three items of inherited dead code were removed during review, beyond the two +widgets the task named: + +- `Action::Help` — declared by the application template, never produced or + handled. +- `Command::leaves_insert_mode` — written by me during implementation, then + never called. Caught because `buffer/command.rs` showed 0% coverage. +- `#![allow(dead_code)]` in `src/tui.rs` — verified to mask nothing before + removal, by deleting it and re-running clippy. + +--- + +## Robustness and error handling + +| # | Check | Result | +|---|---|---| +| 10 | No panic on malformed input | **Pass** — invalid UTF-8 is a reported error (MJB-LLR-118); a declared non-UTF-8 encoding substitutes U+FFFD (MJB-LLR-113) | +| 11 | No panic on malformed configuration | **Pass** — a bad key sequence is a recoverable error naming itself (MJB-LLR-183) | +| 12 | No panic on a failed file operation | **Pass** — every `save` path returns `Result` | +| 13 | Boundary conditions handled | **Pass** — empty buffer, no trailing newline, zero-height viewport, scrolloff exceeding the viewport, undo past history start | +| 14 | Integer overflow/underflow guarded | **Pass** — `saturating_*` throughout `view.rs` and `movement.rs` | + +`unwrap`/`expect`/`panic!` outside test code is confined to two files, neither +in the buffer core: + +| File | Count | Assessment | +|---|---|---| +| `src/errors.rs` | 1 | Inside the panic hook itself | +| `src/tui.rs` | 2 | Template code in `Drop`; pre-existing | + +`src/buffer/**` contains **zero** panicking calls outside `#[cfg(test)]`. + +The one genuinely new failure mode created by this change — a byte offset +landing inside a character, which makes ropey panic — is guarded in two places: +`Range::clamped` (MJB-LLR-011) snaps every externally supplied offset onto a +char boundary, and `ChangeSet::apply` (MJB-LLR-044) validates every operation +boundary *before* mutating, so a rejected change leaves the rope untouched +rather than half-applied. + +--- + +## Defects found and fixed during review + +Eight defects were found by review and test activity rather than by writing +the code. Each records how it was found, because that tells the next person +which verification activities are actually working (docs/process.md §6, Rule 4). + +1. **Every shifted key binding was shadowed by its lowercase twin.** + `parse_key_event` lowercased the entire key string, so `"<A>"` and `"<a>"` + both parsed to `KeyCode::Char('a')`. `A`, `I`, `O`, `U`, `W`, `B` and `E` + were all unreachable, and whichever binding won the hash map ran instead — + so `a` silently invoked `InsertAtLineEnd`. Found by seven integration tests + failing at once. Fixed by preserving the final token's case and inferring + `SHIFT` from an uppercase letter, matching what crossterm reports. + +2. **UTF-16 files would have been corrupted on save.** + `encoding_rs::Encoding::encode` refuses to encode to UTF-16 and silently + substitutes UTF-8, so a UTF-16 document would have been written as UTF-8 + bytes beneath a UTF-16 BOM. Invisible to inspection; found only by a + byte-comparing round-trip test. Fixed by encoding UTF-16 directly from + `str::encode_utf16`. Recorded as **MJB-DR-007**. + +3. **`o` on a file with no trailing newline put the cursor in the wrong place.** + `open_below` anchored to the next line's start, which does not exist on a + final unterminated line. Found by clippy's `if_same_then_else` flagging the + suspicious `if above { at } else { at }`, then confirmed by a robustness + test. Fixed by anchoring to the line's content end. + +4. **`a` appended in the wrong position.** Helix's normal-mode cursor is a + one-grapheme range, so `a` lands past that grapheme; mojibake's empty range + has `to() == from()`. Fixed by stepping forward one grapheme when the range + is empty. + +A later quality pass over the same code found four more. Note that three were +in paths no test covered and one was in inherited template code that only +became reachable when the buffer started consuming `[styles]`. + +5. **Counted word motion was broken.** Each iteration of the count loop + re-derived its start from the partial range's *cursor*, which for a forward + range is one grapheme behind the head. `2w` on `"aaa bbb ccc"` therefore + returned `Range(3, 4)` — anchor in the wrong place, head stalled inside the + first gap — instead of `Range(0, 8)`. Found by restructuring `word_move` + into three named functions and checking each against a worked example; no + test covered a count greater than one on a word motion. Fixed by holding the + anchor fixed and advancing only the head. Regression tests + `mjb_llr_065_counted_next_word_start_advances_once_per_count` and siblings. + +6. **Three panics reachable from user configuration.** `parse_color` indexed + `rgb` operands unchecked and added into `u8` unguarded, so `"rgb"`, + `"rgb1"`, `"gray99"` and `"rgb999"` in a `[styles]` block aborted the + editor at startup — a direct violation of MJB-HLR-018. Found by running + clippy with `-W clippy::pedantic` and following up the + `cast_possible_truncation` hits by hand. Fixed by bounds-checked parsing and + clamping to the actual xterm palette ranges. Regression test + `mjb_llr_183_malformed_colours_never_panic`. + +7. **Bright colours were indistinguishable from their base colours.** The + template wrote `c.wrapping_shl(8)`, which on a `u8` masks the shift to + `8 % 8 == 0` and returns the value unchanged. Found while fixing defect 6. + Fixed to the ANSI convention of base + 8, saturating. + +8. **The modified flag stayed set after undoing back to the saved state.** A + latched `bool` cannot distinguish "edited" from "edited and reverted", so a + buffer byte-identical to its file still refused `:q`. Found by review of the + flag's lifecycle rather than by a test. Replaced with a comparison against + the history revision the file was written at. + + The first attempt at that fix was itself wrong and worth recording: using + the history *cursor* as the revision means saving at depth 3, undoing, then + making a **different** edit returns to depth 3 while the content differs — + reporting clean when it is not. Caught by reasoning through the case before + committing, and fixed by giving each history entry a unique, never-reused + id. Regression test `mjb_llr_116_revision_is_not_stack_depth`. + +--- + +## Architecture + +| # | Check | Result | +|---|---|---| +| 15 | The pre-release rope dependency is confined | **Pass** — `ropey::Rope` is owned only by `buffer/document.rs`; other modules take `RopeSlice` parameters (MJB-DR-006) | +| 16 | The buffer core is independent of the UI | **Pass** — no `ratatui` import under `src/buffer/` | +| 17 | Pagination is structural, not an optimisation | **Pass** — `View::visible_lines` is the only path to line content, and is bounded by viewport height | + +Objective 17 is the one most worth stating plainly: MJB-HLR-012 is satisfied +because there is no code path that walks the document, not because a fast path +was added. `mjb_llr_200_renders_only_the_visible_window` checks the observable +consequence, and `mjb_llr_092_scrolling_a_large_file_stays_responsive` checks +that 500 half-page scrolls over a 200,000-line file stay well inside a time +budget a full scan could not meet. + +--- + +## Summary + +Code review **complete**. Eight defects found and fixed; three items of dead +code removed; no `allow(dead_code)` remains; clippy clean at `-D warnings`; +294 tests passing; 96.21% statement coverage of the buffer core. + +Outstanding items are the six open derived requirements in +`docs/requirements/derived.md`, which need a safety-assessment judgement rather +than a code change. MJB-DR-001 is resolved: UTF-8 is now validated strictly and +malformed input is refused, so the lossy-save hazard it flagged is gone. diff --git a/docs/reviews/library-selection.md b/docs/reviews/library-selection.md new file mode 100644 index 0000000..3b9efa2 --- /dev/null +++ b/docs/reviews/library-selection.md @@ -0,0 +1,98 @@ +# Library selection record + +Software Level: **DAL-C** (DO-178C) +Reviewed by: author (independence not required at DAL-C) + +Records the non-obvious dependency choices for the buffer component and the +reasoning behind each, so a reviewer can judge them without re-deriving the +trade-offs. + +--- + +## ropey 2.0.0-beta.1 — a pre-release dependency + +**Decision.** ropey 2.0.0-beta.1, default features, **byte-indexed**. +`metric_chars` is deliberately **not** enabled. + +**Alternatives considered.** + +| Option | Assessment | +|---|---| +| ropey 1.6.1 (stable) | Char-indexed by default; exactly what Helix pins, so Helix's algorithms port near-verbatim. The conservative choice. | +| 2.0.0-beta.1 + `metric_chars` | Keeps the char API, but re-adds the per-node metadata 2.0 exists to remove — the beta risk without the benefit. | +| **2.0.0-beta.1, byte-indexed (selected)** | Lowest memory and fastest edits; requires translating Helix's algorithms into byte offsets rather than porting them. | + +**Known risk.** The crate is self-described as "not battle-tested like Ropey +1.x", with minor breaking API changes possible before release. This was raised +before implementation and the requester selected it anyway; the choice is +theirs and is recorded here rather than re-litigated. + +**Mitigations in force.** + +1. The version is pinned exactly in `Cargo.toml`. +2. `ropey::Rope` is *owned* only by `src/buffer/document.rs`; other modules + receive `RopeSlice` parameters. Replacing the rope is therefore confined to + one module. (MJB-DR-006) +3. The buffer core carries 93.85% statement coverage against invariants we + assert ourselves, rather than trusting the library's own guarantees. + +**Consequence the reviewer must accept.** Byte indexing admits a failure mode +char indexing cannot express: an offset landing inside a character, which makes +`Rope::insert`/`remove` panic. This is addressed by MJB-LLR-011 (clamp and snap +at construction) and MJB-LLR-044 (validate before mutating). See MJB-DR-002. + +--- + +## encoding_rs — asymmetric, and the asymmetry matters + +**Decision.** Used for decoding all encodings and for encoding everything +*except* UTF-16, which is encoded by hand. + +**Why.** `encoding_rs::Encoding::encode` will not encode to UTF-16; it silently +substitutes UTF-8 and signals the substitution only through a return value that +is easy to discard. Delegating the save path to it wrote UTF-8 bytes beneath a +UTF-16 byte order mark — a file inconsistent with its own BOM. + +This was **not caught by inspection**. It was caught by a round-trip test +(`mjb_llr_115_utf16le_round_trips`) that decoded, re-encoded, and compared +bytes. Any encoding added later should be guarded by the same test shape. + +Recorded as MJB-DR-007. + +--- + +## unicode-segmentation and unicode-width + +**Decision.** `unicode_segmentation::GraphemeCursor` for cluster boundaries; +`unicode_width` for terminal display width. + +**Why.** `GraphemeCursor` is chunk-aware: it reports `PrevChunk`/`NextChunk`/ +`PreContext` when a cluster straddles a fragment edge, which pairs exactly with +ropey's `chunk(byte_idx) -> (&str, chunk_start)`. Clusters spanning rope chunk +boundaries therefore resolve correctly (MJB-LLR-022) without materialising the +text. This is the same pairing Helix uses. + +`unicode_width` is required because byte length, character count, and display +width are three different numbers; the cursor must track the third. A CJK +character occupies two terminal columns and three UTF-8 bytes. + +--- + +## thiserror + +**Decision.** Used for error types in the buffer core. + +**Why.** MJB-HLR-018 forbids abnormal termination, so every failure path must +carry a reportable message. `thiserror` generates `Display` and `From` without +runtime cost or a dynamic error type. + +--- + +## config, with default features disabled + +**Decision.** `config = { default-features = false, features = ["toml"] }`. + +**Why.** MJB-HLR-014 requires that no parser for another format remain in the +dependency graph. Removing the `json5` *call site* would not satisfy that — +the crate's default features pull `json5` in regardless. Disabling default +features is what actually removes it, and is verifiable with `cargo tree`. diff --git a/docs/reviews/requirements-checklist.md b/docs/reviews/requirements-checklist.md new file mode 100644 index 0000000..c6ced28 --- /dev/null +++ b/docs/reviews/requirements-checklist.md @@ -0,0 +1,112 @@ +# Requirements review checklist + +Software Level: **DAL-C** (DO-178C) +Reviewer: author — **review without independence**, which DO-178C permits at +Software Level C. +Scope: `docs/requirements/{hlr,llr,derived}.md` + +Derived from DO-178C Table A-3 (verification of software requirements) and +Table A-4 (verification of the design), reduced to the objectives that apply at +DAL-C. + +--- + +## High-level requirements + +| # | Check | Result | +|---|---|---| +| 1 | Every HLR is traceable to a request in the originating task statement | **Pass** — the statement's scope items map to MJB-HLR-001…019 | +| 2 | HLRs are accurate and unambiguous | **Pass** with one note, below | +| 3 | HLRs are verifiable — each states an observable behaviour | **Pass** | +| 4 | HLRs do not specify implementation | **Pass**, except MJB-HLR-004 and MJB-HLR-014, where the rope and the TOML format were themselves requested | +| 5 | HLRs conform to a consistent standard | **Pass** — "shall" throughout, one behaviour per requirement | +| 6 | Conflicting requirements are identified and resolved | **Pass** — one conflict found, see below | + +**Note on objective 2.** MJB-HLR-007 needed rewording during review. It +originally read "shall provide next-word-start … motions", which reads as a +cursor move and matches the wording of Helix's own keymap documentation. That +wording is misleading: Helix's `move_next_word_start` returns a *range*. The +requirement now states the selection behaviour explicitly, because the +misreading would have produced Vim semantics that pass a naive test. + +**Conflict found and resolved (objective 6).** The task statement asked for a +"non-UTF-8 rejection path" *and* for full Helix save fidelity. Helix transcodes +via `encoding_rs` rather than rejecting, and the two initially looked mutually +exclusive; the conflict was recorded as **MJB-DR-001** and flagged. + +It is now **resolved**, on the requester's direction to make UTF-8 an exception. +Splitting the behaviour by whether the encoding is *known* satisfies both +requests: a BOM-declared non-UTF-8 encoding is transcoded, because the +substitution is reproducible on write (MJB-LLR-113); UTF-8 is validated strictly +and refused when malformed, because nothing is known that would make a repair +reproducible (MJB-LLR-118). The lossy-save hazard that motivated the flag is +eliminated rather than accepted. + +--- + +## Low-level requirements + +| # | Check | Result | +|---|---|---| +| 7 | Every LLR traces to a parent HLR | **Pass** — `docs/traceability/trace.md`, HLR → LLR table | +| 8 | Every LLR is traceable to source | **Pass** — 98/98 tagged, verified by `grep`, not by inspection | +| 9 | LLRs are verifiable | **Pass** — 98/98 have a test named for them | +| 10 | LLRs are consistent with the HLRs they derive from | **Pass** | +| 11 | Algorithms are accurate | **Pass** with one caveat, below | +| 12 | LLRs describe behaviour, not code structure | **Pass** | + +**Caveat on objective 11.** The viewport and word-motion algorithms were +translated from Helix, not ported: Helix indexes by character and mojibake by +byte. Every offset therefore changed meaning. This is the highest-risk area of +the change and is why `buffer/view.rs` (99.57%) and `buffer/movement.rs` +(97.98%) carry the highest coverage in the crate. + +--- + +## Derived requirements + +| # | Check | Result | +|---|---|---| +| 13 | Derived requirements are identified as such | **Pass** — seven, in `derived.md` | +| 14 | Each states what forced it | **Pass** | +| 15 | Each is flagged for the safety assessment | **Pass** — every entry carries a "Reviewer must judge" clause | +| 16 | Derived requirements do not silently weaken an HLR | **Pass** — MJB-DR-001 previously weakened the originating request; it has since been resolved so that both the transcoding and rejection behaviours hold, each in its own domain | + +Seven derived requirements were recorded, of which **one (MJB-DR-001) is now +resolved** and six remain open. Two were found only during implementation and +could not have been anticipated: + +- **MJB-DR-002** — byte indexing admits offsets inside a character, a failure + mode char indexing cannot express. +- **MJB-DR-007** — `encoding_rs` decodes UTF-16 but silently refuses to encode + it, substituting UTF-8. Found by a round-trip test, **not** by inspection. + +--- + +## Open items for the safety assessment + +**Six of seven** derived requirements remain open; MJB-DR-001 is resolved. The +most consequential remaining: + +1. **MJB-DR-006** — ropey 2.0.0-beta.1 is a pre-release under a DAL-C + classification. Raised before implementation, accepted by the requester, + mitigated by confining the dependency to one module. +2. **MJB-DR-007** — `encoding_rs` refuses to encode UTF-16 and substitutes + UTF-8 silently. Worked around; the reviewer must judge whether any other + delegated encoding shares the asymmetry. +3. **MJB-DR-002** — byte indexing admits offsets inside a character. Guarded in + two places; the reviewer must judge whether returning an error is right where + a violated boundary may instead indicate a defect. + +A residual item is noted under MJB-DR-001: with UTF-8 now strict, a binary file +cannot be opened at all, and no override is provided. That omission is +deliberate and its rationale is recorded, but it is a usability decision a +reviewer may want to revisit. + +--- + +## Summary + +Requirements review **complete**. 19 HLRs, 98 LLRs, 7 derived requirements. +One requirements conflict found and **resolved**; one HLR reworded for accuracy; +six derived requirements outstanding for the safety assessment. |
