fix: local-bridge robustness, scope-gate order, sentinel contract; packaging/docs closeout (P8-P11, P2/P3/P15)
Closes review #003 (prepublish review for v0.1.0). - P8: StdinSink::poll_shutdown parks an inflight reserve+send on a full channel (waker registered) — a stdin blast followed by EOF delivers the EOF instead of stranding it - P9: five poisoned-lock .expect() sites -> unwrap_or_else(into_inner) - P10: three thread-spawn .expect() sites -> TtyError::AllocFailed - P5: tty:open scope gate runs before carriage/cmd/backend-lookup checks (no backend-name enumeration differential for unscoped ids) - P11: recv_stdout terminates on the zero-length drained sentinel; the sentinel is no longer yielded as an item (doc was already the contract); stderr has no sentinel (doc noted) - P2: exclude AGENTS.md + docs/plans/, drop dead Cargo.lock and docs/research/ entries (package list: 42 files, 659.2KiB) - P3: AGENTS.md phase status (all five landed), ADR range 001..009 (+ alktty-native ADR-009 in the mapping), alkcall guidance corrected to v0.4.x / pin "0.4.0"; architecture README ADR-009 row + landed-phase status - P15: backend.rs doc typo; redundant tokio-stream dev-dep removed; NegotiationError::Io arm logs; set_identity failure logs; input_pump.abort() at session end; TtySessionError::Open carries the accept_bi StreamError (no io::Error flattening); borrowing deserialize in open_via_channels (no params.clone()); error_response_bytes guards an "error" key in fields; trivial inline comments promoted/removed; plan-doc test counts + doc front-matter refreshed; session tests that raced session teardown under the abort change use a GatedBackend (exit held until released) Verification: cargo test 104 lib / --all-features 147; clippy (all-targets + wasm32) -D warnings; fmt; wasm check; doc 0 warnings; publish dry-run OK.
This commit is contained in:
@@ -1,7 +1,7 @@
|
||||
---
|
||||
status: open
|
||||
status: closed
|
||||
last_updated: 2026-09-05
|
||||
resolved: 1, 4, 6, 7, 12, 13, 16 (P14 closed)
|
||||
resolved: 1-16 (P14 closed; P13's accept-loop harness deferred by disposition)
|
||||
reviewed_artifacts:
|
||||
- src/lib.rs
|
||||
- src/wire.rs
|
||||
@@ -30,6 +30,7 @@ base_commit: 918af40 (review #002 resolved)
|
||||
resolutions:
|
||||
- "P1 + P13 resolved 2026-09-05 (87c52e5) — see the Session-1 Resolution section"
|
||||
- "P12 + P6 + P7 + P4 + P16 resolved 2026-09-05 — see the Session-2 Resolution section"
|
||||
- "P8 + P9 + P10 + P5 + P11 + P2 + P3 + P15 resolved 2026-09-05 — see the Session-3 Resolution section"
|
||||
---
|
||||
|
||||
# Code Review #003 — Prepublish Review (v0.1.0)
|
||||
@@ -467,20 +468,20 @@ theirs; don't replicate that nit.
|
||||
| ID | Finding | Fix | Effort | Risk | Status |
|
||||
|----|---------|-----|--------|------|--------|
|
||||
| P1 | pump_session deadlock ≥63 chunks | concurrent drainer + regression test | small | medium (core pump) | ✅ resolved (`87c52e5`) |
|
||||
| P2 | AGENTS.md ships; dead exclude entries | fix `exclude` | trivial | none | open |
|
||||
| P3 | stale AGENTS.md + architecture README | refresh phase/ADR/alkcall text | trivial | none | open |
|
||||
| P2 | AGENTS.md ships; dead exclude entries | fix `exclude` | trivial | none | ✅ resolved (Session 3) |
|
||||
| P3 | stale AGENTS.md + architecture README | refresh phase/ADR/alkcall text | trivial | none | ✅ resolved (Session 3) |
|
||||
| P4 | no README + `readme` field | write README (after P5/P6/P12) | medium | none | ✅ resolved (Session 2) |
|
||||
| P5 | scope gate after backend lookup | reorder checks | trivial | low (test updates) | open |
|
||||
| P5 | scope gate after backend lookup | reorder checks | trivial | low (test updates) | ✅ resolved (Session 3) |
|
||||
| P6 | write paths accept invalid chunks | validate on write; unify empty-payload shape | small | low | ✅ resolved (Session 2) |
|
||||
| P7 | read-after-peek framing corruption | peeked-state guard | small | none | ✅ resolved (Session 2) |
|
||||
| P8 | poll_shutdown missing waker | inflight-future pattern | small | low | open |
|
||||
| P9 | poisoned-lock `.expect()` × 5 | `into_inner()` | trivial | none | open |
|
||||
| P10 | thread-spawn `.expect()` × 3 | map to `TtyError` | trivial | none | open |
|
||||
| P11 | recv_stdout sentinel contract | terminate stream on sentinel (or fix doc) | small | low (semver-visible) | open |
|
||||
| P8 | poll_shutdown missing waker | inflight-future pattern | small | low | ✅ resolved (Session 3) |
|
||||
| P9 | poisoned-lock `.expect()` × 5 | `into_inner()` | trivial | none | ✅ resolved (Session 3) |
|
||||
| P10 | thread-spawn `.expect()` × 3 | map to `TtyError` | trivial | none | ✅ resolved (Session 3) |
|
||||
| P11 | recv_stdout sentinel contract | terminate stream on sentinel (or fix doc) | small | low (semver-visible) | ✅ resolved (Session 3) |
|
||||
| P12 | no crate-root re-exports | `pub use` primary types | trivial | none | ✅ resolved (Session 2) |
|
||||
| P13 | test gaps | backpressure (>64 chunks) + boundary tests | small | none | ✅ resolved (`87c52e5`; accept-loop harness deferred) |
|
||||
| P14 | `Result<(), String>` on register_openable | closed — alkcall parity | none | none | ✅ closed |
|
||||
| P15 | minor polish batch | one batched commit | small | none | open |
|
||||
| P15 | minor polish batch | one batched commit | small | none | ✅ resolved (Session 3) |
|
||||
| P16 | no CHANGELOG.md | start changelog with [0.1.0] | small | none | ✅ resolved (Session 2) |
|
||||
|
||||
## Suggested Session Breakdown
|
||||
@@ -603,6 +604,109 @@ clippy (all-targets + wasm32) `-D warnings`, fmt, wasm check, doc
|
||||
(0 warnings), `cargo publish --dry-run --allow-dirty` (44 files,
|
||||
702.8KiB) — all clean.
|
||||
|
||||
## Session-3 Resolution (P8 + P9 + P10 + P5 + P11 + P2 + P3 + P15, 2026-09-05)
|
||||
|
||||
The `local` robustness batch, the producer-hygiene batch, and the
|
||||
packaging/docs closeout — the review's suggested session breakdown
|
||||
items 3, 4, and 5. This closes review #003.
|
||||
|
||||
- **P8 — `poll_shutdown` waker** (`src/local/pty.rs`): `StdinSink`
|
||||
gained an `inflight_close` slot mirroring `poll_write`'s inflight
|
||||
pattern — on a full stdin channel, `poll_shutdown` parks a
|
||||
`reserve + send(StdinCmd::Eof)` future and polls it on re-poll, so
|
||||
the poller's waker is registered and a stdin blast followed by EOF
|
||||
delivers the EOF instead of stranding it. Distinct from the
|
||||
byte-write slot so the two sends never share a future.
|
||||
- **P9 — poisoned-lock `.expect()` ×5** (`src/local/pty.rs`): the five
|
||||
non-test lock sites (`PtyControl::resize`, both `PtyControl::signal`
|
||||
arms, the reader-thread and writer-thread master-lock acquisitions)
|
||||
use `unwrap_or_else(|e| e.into_inner())` per convention 2.
|
||||
- **P10 — thread-spawn `.expect()` ×3** (`src/local/pty.rs`): the
|
||||
reader/writer/waiter `thread::Builder::spawn` results map to
|
||||
`TtyError::AllocFailed` (the error path existed — `allocate_pty`
|
||||
returns `Result`); fd/thread exhaustion surfaces as an allocation
|
||||
error, not a panic.
|
||||
- **P5 — scope gate first** (`src/adapter.rs`):
|
||||
`validate_and_allocate` now runs the `tty:open` scope check before
|
||||
the carriage/cmd checks and the backend lookup. An
|
||||
authenticated-but-unscoped identity gets `forbidden` regardless of
|
||||
the request body — the `unknown_backend`-vs-`forbidden` name-
|
||||
enumeration differential is gone. The existing
|
||||
`scope_gate_forbidden_without_tty_open` test asserted the scoped
|
||||
behavior and still passes (it uses a *registered* backend name, so
|
||||
the reorder doesn't change its outcome); no test asserted
|
||||
`unknown_backend` with an unscoped identity.
|
||||
- **P11 — sentinel contract** (`src/session.rs`): `recv_stdout` now
|
||||
terminates the stream ON the zero-length stdout sentinel ("drained")
|
||||
— the sentinel is not yielded as an item. This matches the
|
||||
documented contract and the sentinel's meaning; consumers no longer
|
||||
filter. `recv_stderr`'s doc now states stderr has no sentinel (the
|
||||
adapter's stderr pump emits none; that stream ends when the read
|
||||
pump terminates). The three session tests that filtered the
|
||||
sentinel manually now assert the stream ends on it (no filter).
|
||||
The README example's manual `is_empty` break was removed.
|
||||
- **P2 — packaging** (`Cargo.toml`): `exclude` is now
|
||||
`[".opencode/", "AGENTS.md", "docs/reviews/", "docs/plans/",
|
||||
"docs/sdd_process.md"]` — AGENTS.md no longer ships (alkcall
|
||||
parity), `docs/plans/` (absolute `/workspace/...` paths) no longer
|
||||
ships, and both dead entries (`Cargo.lock` — force-included anyway;
|
||||
`docs/research/` — nonexistent) are dropped. Package list verified:
|
||||
42 files, 659.2KiB (was 44 files, 702.8KiB).
|
||||
- **P3 — stale docs**: AGENTS.md now says all five phases are landed,
|
||||
the ADR range is 001..009 with the alktty-native ADR-009 noted in
|
||||
the mapping list, and the alkcall guidance is corrected to "v0.4.x —
|
||||
Pin `alkcall = "0.4.0"` (lockfile resolves 0.4.1)" (was "v0.1.x —
|
||||
pin 0.1.1", which would have downgraded the dependency).
|
||||
`docs/architecture/README.md` gained the ADR-009 table row, the
|
||||
001..009 preamble, and the landed-phase status.
|
||||
- **P15 — polish batch** (one commit, as planned):
|
||||
- `src/backend.rs` doc typo fixed ("`local` feature module) is the
|
||||
reference implementation").
|
||||
- `tokio-stream` dev-dependency removed (inherited from the main
|
||||
dep).
|
||||
- `src/adapter.rs` — the `NegotiationError::Io(_)` arm now logs
|
||||
(`debug!`) for parity with the catch-all arm; `set_identity`
|
||||
failure is logged (`AlreadySet` is benign-by-construction, noted
|
||||
in a comment); `drop(input_pump)` → `input_pump.abort()` at
|
||||
session end (no lingering client→backend task holding the backend
|
||||
stdin half after the exit chunk).
|
||||
- `src/session.rs` — `connect_direct`/`from_bidi_stream_via` map the
|
||||
`accept_bi` `StreamError` into a new `TtySessionError::Open`
|
||||
variant (`#[from]`) instead of flattening it into an anonymous
|
||||
`io::Error(ConnectionReset)` (additive pre-consumers, same logic
|
||||
as review #002 R5); the fail-fast params parse uses
|
||||
`NegotiateRequest::deserialize(¶ms)` (borrowing) instead of
|
||||
`from_value(params.clone())` — no clone.
|
||||
- `src/negotiation.rs` — `error_response_bytes` skips an `"error"`
|
||||
key in `fields` so it cannot overwrite the error code.
|
||||
- Two trivial inline comments promoted or removed (the split-idiom
|
||||
comment in `adapter.rs` removed; the "Already taken" comment in
|
||||
`session.rs` now points at the documented behavior).
|
||||
- `docs/plans/project-setup.md` test counts refreshed (119 lib + 19
|
||||
integration = 138 under `--all-features` at the review baseline,
|
||||
with a note that the suites have grown); `overview.md` and
|
||||
`tty-adapter.md` front-matter `last_updated` dates and the ADR
|
||||
range updated for the ADR-009 edits.
|
||||
- The `input_pump.abort()` change surfaced a latent test race: the
|
||||
`send_stdin_round_trips_to_backend` and
|
||||
`resize_and_signal_dont_error` tests used `MockBackend` (exit
|
||||
resolves on allocate) and were writing while the session data
|
||||
plane was already closing — previously masked by the lingering
|
||||
input pump the abort now reclaims. Both use a new `GatedBackend`
|
||||
(exit held until the test releases it), making the writes
|
||||
deterministic and asserting the exit code.
|
||||
|
||||
Verification on the resolution tree: `cargo test` 104 lib (unchanged) /
|
||||
`cargo test --all-features` 128 lib + 19 integration = 147
|
||||
(unchanged; the two reworked tests kept their names); clippy
|
||||
(all-targets + wasm32) `-D warnings`, fmt, wasm check, doc
|
||||
(0 warnings), `cargo publish --dry-run --allow-dirty` (42 files,
|
||||
659.2KiB) — all clean.
|
||||
|
||||
**Review #003 is closed.** Remaining before publish: tag `v0.1.0` (the
|
||||
CHANGELOG link definition points at it) and `cargo publish` (dry-run
|
||||
clean).
|
||||
|
||||
## Notes
|
||||
|
||||
- Line numbers refer to the tree at `918af40` (review base).
|
||||
|
||||
Reference in New Issue
Block a user