From e5330e957b9b148f4ffa99fbe2920e3fd4adfad0 Mon Sep 17 00:00:00 2001 From: deepseek-v4-pro Date: Mon, 17 Aug 2026 12:13:35 +0000 Subject: [PATCH] docs: mark review #001 findings resolved (M1, M2, L2, L4, L5, N1-N3, N5) Update the remediation plan with a resolution section and per-finding status. Nine findings resolved in commit 9944153; L1, L3, L6, N4, N6 remain open. --- docs/reviews/001-code-review.md | 97 ++++++++++++++++++++++----------- 1 file changed, 64 insertions(+), 33 deletions(-) diff --git a/docs/reviews/001-code-review.md b/docs/reviews/001-code-review.md index b3b6f02..2f3cbaa 100644 --- a/docs/reviews/001-code-review.md +++ b/docs/reviews/001-code-review.md @@ -1,5 +1,5 @@ --- -status: open +status: partially-resolved (M1, M2, L2, L4, L5, N1, N2, N3, N5) last_updated: 2026-08-17 reviewed_artifacts: - src/lib.rs @@ -521,48 +521,79 @@ Highlights: ## Remediation Plan -| ID | Finding | Fix | Effort | Risk | -|----|---------|-----|--------|------| -| L5 | `cargo fmt` fails | run `cargo fmt`, commit | trivial | none | -| N1 | 9 rustdoc warnings | fix 9 link sites | trivial | none | -| N2 | stale doc paths | fix 2 paths | trivial | none | -| N3 | `unsafe` vs AGENTS.md §14 | amend convention wording | trivial | none | -| L4 | mocks leak into public API | `pub(crate)` on `MockBackend`/`MockControl` | trivial | none | -| N5 | `rand_seed`/`nanos_seed` ×4 | consolidate | trivial | none | -| M2 | `wait()` swallows `MalformedExitChunk` | propagate the variant | small | low | -| L2 | consumer stdout/stderr routing untested | add emitting test backend | small | low | -| M1 | negotiation-rejection frame unhandled | implement disambiguation read | medium | medium (wire-facing) | -| L1 | channels `input` ignored | decide drop-vs-pass-through | small | low | -| L3 | `open_via_channels` 0% covered | end-to-end channels consumer test | medium | low | -| L6 | pty bridge error paths untested | targeted error-path tests | medium | low | -| N4 | sleep-based timing | readiness signals | small | low | -| N6 | MSRV unverified | CI MSRV job or bump | small | none | +| ID | Finding | Fix | Effort | Risk | Status | +|----|---------|-----|--------|------|--------| +| L5 | `cargo fmt` fails | run `cargo fmt`, commit | trivial | none | ✅ resolved | +| N1 | 9 rustdoc warnings | fix 9 link sites | trivial | none | ✅ resolved | +| N2 | stale doc paths | fix 2 paths | trivial | none | ✅ resolved | +| N3 | `unsafe` vs AGENTS.md §14 | amend convention wording | trivial | none | ✅ resolved | +| L4 | mocks leak into public API | `pub(crate)` on `MockBackend`/`MockControl` | trivial | none | ✅ resolved | +| N5 | `rand_seed`/`nanos_seed` ×4 | consolidate | trivial | none | ✅ resolved | +| M2 | `wait()` swallows `MalformedExitChunk` | propagate the variant | small | low | ✅ resolved | +| L2 | consumer stdout/stderr routing untested | add emitting test backend | small | low | ✅ resolved | +| M1 | negotiation-rejection frame unhandled | implement disambiguation read | medium | medium (wire-facing) | ✅ resolved | +| L1 | channels `input` ignored | decide drop-vs-pass-through | small | low | open | +| L3 | `open_via_channels` 0% covered | end-to-end channels consumer test | medium | low | open | +| L6 | pty bridge error paths untested | targeted error-path tests | medium | low | open | +| N4 | sleep-based timing | readiness signals | small | low | open | +| N6 | MSRV unverified | CI MSRV job or bump | small | none | open | -### Recommended Order +### Resolution (2026-08-17, commit `9944153`) -1. **L5 + N1 + N2 + N3 + L4 + N5** — the trivial hygiene batch. One - commit, no behavior change, makes `cargo fmt`/`cargo doc` clean and - the public API tidy. Do this first so the tree passes its own - verification gate. -2. **M2** — small, low-risk, closes a swallowed-error path in the - consumer half. -3. **L2** — small, high-value: the consumer half's happy path is - currently untested. -4. **M1** — the one real protocol gap. Medium effort and wire-facing, - so it needs publisher sign-off on the disambiguation approach before - implementation. -5. **L1 + L3** — the channels consumer path; do together since L3's +Nine findings were resolved in a single commit: + +- **M1** — `TtySession::from_halves` now peeks the first response byte + and returns `NegotiationRejected` on a `0x00`-prefixed error frame + (ADR-052 §5 disambiguation). `ChunkReader` gained + `peek_stream_type`/`read_chunk_after_peek` to support the peek. + Test: `connect_direct_returns_negotiation_rejected`. +- **M2** — `wait()` now surfaces `MalformedExitChunk` instead of + collapsing it to `NoExitChunk`. The exit watch channel carries a + cloneable `ExitOutcome` enum (the `TtySessionError` payloads are not + `Clone`); `MalformedExitChunk` now carries a `String`. Test: + `wait_returns_malformed_exit_chunk`. +- **L2** — added `EmittingBackend` and + `recv_stdout_and_stderr_route_backend_data`, covering the consumer + read-pump stdout/stderr routing with real data. +- **L4** — `MockBackend`/`MockControl`/`MockStdinSink` are now + `#[cfg(test)] pub(crate)`, removed from the public API. +- **L5** — `cargo fmt` (the BAST drift test was unformatted). +- **N1** — all 9 rustdoc intra-doc links fixed; `cargo doc` is clean. +- **N2** — stale doc paths fixed (`crates/tty/` and `docs/research/`). +- **N3** — AGENTS.md §14 amended to accurately describe the `local` + module's `libc::kill` `unsafe` blocks. +- **N5** — `nanos_seed` consolidated into `tests/common/mod.rs`. + +Post-fix coverage: `session.rs` 79.71% → 87.43% lines, total 90.74% → +91.47% lines. All verification gates pass (`cargo test`, +`cargo test --all-features`, clippy native + wasm, fmt, doc, wasm +check). + +### Remaining (open) + +- **L1** — channels `input` ignored; needs a publisher decision + (drop the parameter vs pass-through to `drive_session`). +- **L3** — `open_via_channels` still 0% covered; needs the channels + harness shared across modules. +- **L6** — pty bridge error paths untested. +- **N4** — sleep-based timing in signal/cancel tests. +- **N6** — MSRV unverified. + +### Recommended Order (remaining) + +1. **L1 + L3** — the channels consumer path; do together since L3's test will exercise L1's code. -6. **L6** — pty bridge error paths; medium effort, lower priority than +2. **L6** — pty bridge error paths; medium effort, lower priority than the consumer-half work. -7. **N4 + N6** — test hardening and MSRV; defer until CI exists. +3. **N4 + N6** — test hardening and MSRV; defer until CI exists. --- ## Notes - All line numbers refer to the tree at commit `18c4924` (the last - commit on `main` at review time). + commit on `main` at review time). The resolution section above + reflects the tree at commit `9944153`. - The coverage numbers are from `cargo llvm-cov --all-features` on the same tree. The `--show-missing-lines` output was used to attribute gaps; the full report is at `target/llvm-cov/html`.