aboutsummaryrefslogtreecommitdiff
path: root/docs/reviews
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
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')
-rw-r--r--docs/reviews/code-checklist.md202
-rw-r--r--docs/reviews/library-selection.md98
-rw-r--r--docs/reviews/requirements-checklist.md112
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.