aboutsummaryrefslogtreecommitdiff
path: root/docs/reviews/code-checklist.md
diff options
context:
space:
mode:
authorrottedfm <rottedfm@proton.me>2026-08-19 11:21:47 -0400
committerrottedfm <rottedfm@proton.me>2026-08-19 11:21:47 -0400
commitea0bd36167b684c0accdb5ce2b2e21b8d84aeb25 (patch)
treebe1267972b5de2f1ae592577dfabce67f1fe6e87 /docs/reviews/code-checklist.md
parent8c0b4c53b130555f040884c1f52b90f16b23e241 (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.md202
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.