Repository navigation
Conversation
C6 / Beads mdo-6w5.2 (epic mdo-6w5, PR order 2 of 6; stacked on the C1
branch).
- tests/cli.rs re-implemented temp_output_for as temp_output_for_test and
had already lost stem sanitization (diverged from production). The four
call sites now call mdo_cli::temp_output_for directly, so tests compute
the same per-source private paths production does, sanitization included.
The mirror is deleted, along with the Hash/Hasher imports it alone needed.
- The one-line derive_output alias had a single caller; main.rs now calls
the real target, input.with_extension("html"), and lib no longer exposes
the alias.
- SETUP_SAMPLE_FILE_NAME / SETUP_SAMPLE_MARKDOWN had zero external callers;
both are now private. (Windows SetupSample? not referenced: mdo-setup and
mdo-open never touch them.)
- wrapper.rs's own empty_path_dir duplication is intentionally untouched:
recorded in the issue as having an honest reason.
Gates: fmt --check, clippy -D warnings, cargo test --all-targets
--all-features --locked (90 tests) all green, including the Linux --open
tests that exercise the corrected temp-output path contract.
Beads-Ref: mdo-6w5.2
Agent-Signature: kilo-kilo-auto-efficient on behalf of matt
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
| watcher_error_after(60), | ||
| watcher_error_after(60), | ||
| ]; | ||
| let (renders, _clock) = pump_scripted(script); |
There was a problem hiding this comment.
WARNING: This test cannot fail for the property its name pins
watcher_errors_inside_the_drain_do_not_extend_the_window asserts only the render count, but a pump that wrongly reset the deadline in the Ok(Err(e)) drain arm still yields exactly one render here: the errors at 60/60 ms would push the deadline to 370 ms instead of 250 ms, timeout still fires, and the render count is unchanged. The only discriminating observable is elapsed virtual time, which is discarded as _clock. Consider asserting the returned clock (final elapsed of ~250 ms: 50 ms to the first relevant event, then the 200 ms quiet window) so a noise-driven deadline reset actually fails this test. sibling_noise_waits_out_but_does_not_extend_the_window discards _clock the same way.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
| // (bounded by one debounce) and renders; the sibling is then seen | ||
| // as the next outer event, classified irrelevant, and skipped. | ||
| let (renders, _clock) = pump_scripted(vec![ | ||
| modify("sample.md", Path::new("/watch"), 50), |
There was a problem hiding this comment.
SUGGESTION: The sibling is never delivered inside the drain window
sibling("other.md", ..., 1000) is ready long after the remaining 200 ms window, so the drain's Ok(Ok(event)) arm never sees it — it only surfaces later as the next outer-loop event and is skipped. A pump bug that reset the deadline on irrelevant events delivered mid-drain would still pass this test as written. A sibling scripted inside the window (e.g. ready_after 100) would exercise the "irrelevant events neither extend nor cut the window" arm the test name refers to.
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.
SUGGESTION: The fake channel models a timeout here, not channel death
In FakeRx::now_or_elapsed, an exhausted script yields Err(RecvTimeoutError::Timeout), so during the drain "channel death" is indistinguishable from "quiet window elapsed" — RecvTimeoutError::Disconnected never reaches pump's Err(_) => break arm from this script. The render count is correct either way; either have the fake report Disconnected when the script is exhausted, or reword the name/comment to "script exhausted while the window is open".
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 bool result is now unused, and its doc is stale
This call site discards the bool, and it is the only call site left: the one-shot exit code now comes from render_and_report(...).is_ok() (line 441) and the watch callback returns Result<(), RenderError>. Yet the doc on report_render (lines 518–522) still reads "Returns whether the render succeeded, so exit-code policy stays local to main." Either update that doc or drop the -> bool return from report_render.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
Code Review SummaryStatus: 4 Issues Found | Recommendation: Address before merge Overview
Issue Details (click to expand)WARNING
SUGGESTION
Files Reviewed (4 files)
Fix these issues in Kilo Cloud Reviewed by efficient · Input: 58.1K · Output: 25.3K · Cached: 369.9K |
a4d4448 to
8bd30e5
Compare
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
agent/6w5-c6-test-mirror, which stacks on C1)What
New
src/watch.rs(lib crate) owning all three watch-mode mechanics behind one interface:renderonce immediately, then once per settled burst (trailing-edge debounce).Err(transient filesystem state mid-rename) never ends watch mode; channel death ends it gracefully withOk(()).👀 Watching…/🔁 File changed…/⚠️ Watcher error) belong to the module; rendering + reporting stay CLI-side via the callback.main.rs keeps CLI-local helpers (
render_and_report,launch_open_output) and exit-code policy; the--watch --openlaunch hook remains a one-time duty consumed exactly on the first callback (pre-module behavior preserved, byte-compatible print order included).Coverage change
tests/cli.rskeeps one spawned-binary e2e smoke:watch_survives_atomic_rename.watch_ignores_sibling_changesmoved into module unit tests (12 new).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)