Repository navigation
Deepen mdo C2: watch mode becomes a debounce module - #49
Conversation
C2 / Beads mdo-6w5.3 (epic mdo-6w5, PR order 3 of 6; stacked on the C6 branch). Watch mode's mechanics were interleaved with main.rs's CLI plumbing: parent-dir registration rationale, event relevance, the trailing-edge debounce drain, channel death, and the render-failure continuation all lived inline around ~180 lines with only two spawned-binary e2e tests as coverage, both unix-gated. New src/watch.rs in the lib crate owns all three mechanics behind one interface: - watch::run(input, render) - one deep watch entry point. Registers the watch BEFORE rendering (a save landing during the initial render is queued, not lost), runs the render callback once immediately, then once per settled burst. A render callback Err (transient filesystem state mid-rename) never ends watch mode; channel death ends it gracefully with Ok(()). - The debounce timing and the loop clock are injectable at pump level: unit tests script a fake event channel (relative arrival times) against a virtual clock, so the semantics are covered cross-platform by construction: burst collapse, bursts running longer than the window, events after the window, sibling noise, watcher error storms, channel death during a drain, callback errors, and event-relevance gates. - watch-mode UX lines (Watching / File changed, re-rendering / Watcher error) are the module's; rendering and its reporting stay with the CLI via the callback. main.rs keeps the CLI-local helpers (render_and_report, launch_open_- output) and its exit-code policy; the -watch -open launch hook stays a one-time duty consumed exactly on the first callback, as before the module existed. tests/cli.rs keeps one spawned-binary e2e smoke test (watch_survives_atomic_rename); the watch_ignores_sibling_changes e2e moves into the module unit tests. Gates: fmt --check, clippy --all-targets --all-features -D warnings, and cargo test --all-targets --all-features --locked all green (12 new watch unit tests; byte-identity harness against the pre-C1 binary still passes for the once-mode CLI surface). Beads-Ref: mdo-6w5.3 Agent-Signature: kilo-kilo-auto-efficient on behalf of matt
![kilo-code-bot[bot]](https://avatars.githubusercontent.com/in/2193792?s=60&v=4){kind=small}
| use std::process::Command; | ||
| use std::time::{SystemTime, UNIX_EPOCH}; | ||
|
|
||
| use mdo_cli::temp_output_for; |
There was a problem hiding this comment.
[WARNING]: temp_output_for import is unused on non-Linux targets
All four call sites (lines 573, 657, 708, 752) sit inside #[cfg(target_os = "linux")] test functions, but this top-level import carries no gate, so macOS and Windows builds of the test target emit an unused_imports warning (and cargo clippy --all-targets -D warnings fails there). The file's own convention gates such imports (#[cfg(unix)] use std::time::{Duration, Instant}; at line 8, #[cfg(target_os = "linux")] use std::os::unix::fs::{symlink, PermissionsExt}; at line 11) — apply the same gate here.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
| } | ||
|
|
||
| #[test] | ||
| fn channel_death_during_drain_renders_and_exits() { |
There was a problem hiding this comment.
[WARNING]: Test name/comment over-claim: channel death during the drain is never exercised
Traced with the harness: recv() delivers the modify and opens the window; the drain then polls recv_timeout against an empty script, and FakeRx::now_or_elapsed (lines 288-305) can only return RecvTimeoutError::Timeout — it never constructs Disconnected — so the drain exits via the timeout arm of Err(_) => break (line 166), and death is only seen on the following outer recv(). The pending burst renders because the window timed out, not because channel death broke the drain, and the Disconnected-during-drain path is inexpressible with this harness. Production behavior is identical either way (both variants take the same break), so this is naming/coverage, not a product bug: consider renaming it (e.g. quiet_drain_renders_then_death_ends_watch) or teaching FakeRx a per-entry disconnect mode for recv_timeout.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
| ); | ||
| if pending_open_launch { | ||
| // The launch hook is a one-time post-initial-render duty | ||
| // (the pump runs the callback immediately): consume the |
There was a problem hiding this comment.
[SUGGESTION]: The immediate callback run belongs to watch::run, not the pump
watch::run invokes the callback once immediately (src/watch.rs:92) before it ever calls pump (src/watch.rs:99); pump only runs the callback for post-window re-renders (src/watch.rs:175). Pointing at the pump could send a reader hunting inside it for the initial launch.
| // (the pump runs the callback immediately): consume the | |
| // (watch::run runs the callback immediately): consume the |
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
| } | ||
| pending_open_launch = false; | ||
| } | ||
| result.map(|_| ()) |
There was a problem hiding this comment.
[SUGGESTION]: No-op map — result is already Result<(), RenderError>
render_and_report returns Result<(), RenderError> (main.rs:474), so .map(|_| ()) maps Ok(()) to Ok(()). Unlike outcome.map(|_| ()) inside render_and_report (main.rs:484), which legitimately erases a ConvertOutcome, this erases nothing and just reads like it does.
| result.map(|_| ()) | |
| result |
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
| private_output, | ||
| css_override, | ||
| }); | ||
| report_render(input, output, &outcome, verbose); |
There was a problem hiding this comment.
[SUGGESTION]: report_render's success bool is discarded; its doc's exit-code justification is now stale
This is report_render's only remaining call site, and nothing consumes the returned bool — the exit-code signal now comes from render_and_report(...).is_ok() (main.rs:441). The helper's doc (main.rs:521-522, unchanged by this PR) still reads "Returns whether the render succeeded, so exit-code policy stays local to main", which no longer holds. Consider dropping the bool return or refreshing the doc.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
|
|
||
| /// Fake channel half driving `pump` from a script. Events ready within | ||
| /// the polled timeout are delivered on that poll; anything later goes | ||
| /// past the window (timed out), exactly as a real channel would. |
There was a problem hiding this comment.
[SUGGESTION]: FakeRx re-charges a queued item's ready_after from zero after a Timeout
When now_or_elapsed times out it advances the clock by the whole timeout but leaves the front item with its original relative ready_after, so the next poll/recv re-charges the full wait, double-counting time the Timeout already consumed (a real channel would deliver a queued item at its absolute readiness time). No current test verdict depends on this (traced), but the "exactly as a real channel would" phrasing overreaches, and any future assertion touching virtual time could be silently off.
| /// past the window (timed out), exactly as a real channel would. | |
| /// past the window (timed out). |
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
Code Review SummaryStatus: 5 Issues Found | Recommendation: Address before merge Overview
Issue Details (click to expand)WARNING
SUGGESTION
Files Reviewed (7 files)
Fix these issues in Kilo Cloud Notes: Incremental review since f59f85c: generated metrics artifacts (history snapshot, latest.json, index.html) are internally consistent, and the single code change gates the Previous Review Summary (commit f59f85c)Current summary above is authoritative. Previous snapshots are kept for context only. Previous review (commit f59f85c)Status: 6 Issues Found | Recommendation: Address before merge Overview
Issue Details (click to expand)WARNING
SUGGESTION
Files Reviewed (4 files)
Notes: The refactor was verified as behavior-preserving against base e3290fb: register-before-render ordering, print order (✅ Converted → 🌐/❌ launch → 👀 Watching; 🔁 File changed printed before re-render output), one-shot exit-code policy, one-time Reviewed by efficient · Input: 48.8K · Output: 8.5K · Cached: 613.1K |
f59f85c to
5e07917
Compare
1 participant
C2: Deepen watch mode into a debounce module
Beads mdo-6w5.3 · parent epic mdo-6w5 (PR order 3 of 6; stacked on the C6 branch, which stacks on C1)
What
New `src/watch.rs` (lib crate) owning all three watch-mode mechanics behind one interface:
```rust
watch::run(input, render) -> notify::Result<()>
```
`main.rs` keeps CLI-local helpers (`render_and_report`, `launch_open_output`) and exit-code policy; the `--watch --open` launch hook remains a one-time duty consumed exactly on the first callback (pre-module behavior preserved, byte-compatible print order included).
Coverage change
Gates
fmt --check ✓ · clippy -D warnings ✓ · `cargo test --all-targets --all-features --locked` ✓ (101 tests: 66 lib + 21 cli + 17 others)
Signed: kilo-kilo-auto-efficient on behalf of matt (Beads mdo-6w5.3)