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/code-checklist.md | |
| 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/code-checklist.md')
| -rw-r--r-- | docs/reviews/code-checklist.md | 202 |
1 files changed, 202 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. |
