Repository navigation
Conversation
![kilo-code-bot[bot]](https://avatars.githubusercontent.com/in/2193792?s=60&v=4){kind=small}
|
|
||
| use std::io; | ||
| use std::path::Path; | ||
| use std::time::Duration; |
There was a problem hiding this comment.
SUGGESTION: Duration import is unused on Windows and macOS builds
After the split, this unconditional import's only use is LAUNCH_FAILURE_WINDOW inside launch_via_xdg_open, which is gated #[cfg(all(unix, not(target_os = "macos")))] (launch.rs:118). Before the split, Duration was always used via StageTimings in lib.rs, so the combined import never triggered a lint; now Windows builds (the Run Windows tests CI job will surface it in its output) and any macOS build emit an unused_imports warning.
Gate the import with the same cfg as its sole consumer, matching the pattern render.rs uses for cfg-specific imports (e.g. #[cfg(unix)] use std::io::Write;) — i.e. the single line is replaced by:
#[cfg(all(unix, not(target_os = "macos")))]
use std::time::Duration;(Not offered as a fenced suggestion since the fix spans two lines, and the one-line attribute form would trip cargo fmt --check.)
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
Code Review SummaryStatus: 1 Issues Found | Recommendation: Address before merge Overview
Incremental review since Fix these issues in Kilo Cloud Issue Details (click to expand)SUGGESTION
Files Reviewed (5 files)
Previous Review Summary (commit c5d2bb2)Current summary above is authoritative. Previous snapshots are kept for context only. Previous review (commit c5d2bb2)Status: 1 Issues Found | Recommendation: Address before merge Overview
Fix these issues in Kilo Cloud Issue Details (click to expand)SUGGESTION
Verification Performed (pure-relocation claim)
Files Reviewed (5 files)
Reviewed by efficient · Input: 51.4K · Output: 6.4K · Cached: 236.8K |
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
C3 / Beads mdo-6w5.4 (epic mdo-6w5, PR order 4 of 6; stacked on the C2 branch). The fact "the registered handler runs mdo --open <file> and siblings are found beside the running exe" was spelled six times: the Linux desktop Exec template, the Windows handler/registry command variants, mdo-open's forwarding argv per cfg-branch, mdo-setup's two sibling_binary copies, and windows setup's sibling resolution. New src/handler.rs in the lib crate owns the contract: - OPEN_FLAGS + open_argv(): the --open argv prefix spelled once; the file argument itself stays platform presentation (%f placeholder, %1 in a registry command, real path when spawned). - sibling_binary(exe, name): one discovery function, one error mode - a NotFound io error naming BOTH the anchor executable and the path that was checked - so callers print it as-is instead of inventing their own not-found story. - Desktop-entry template, Windows registry command builders, and mdo-open's forward become thin adapters consuming it. Windows-only emission tests now derive their expectation from the contract instead of restating it (2 emitted-form tests: Linux Exec line + Windows registry command), so Linux and Windows cannot drift apart. - mdo-setup's two sibling_binary copies are deleted; the eager is_file check Windows onboarding used to spell inline now applies everywhere through the shared error. In-pass: mdo-open no longer exits silently. current_exe() failure, a missing sibling target in any branch, and spawn failures all print a message before the non-zero exit (a GUI-launched binary may still have no stderr to show it on Windows, but silence is worse everywhere). exit_code_from_status is split into exit_code_from_parts(success, code) so the truncation semantics are unit-testable portably: a failure whose low byte is zero (e.g. 0x100) must not read as success. Covered by four portable tests plus a Windows-only end-to-end (only Windows can produce such a status; the repo's CI runner exercises it). Gates: fmt --check, clippy --all-targets --all-features -D warnings, cargo test --all-targets --all-features --locked all green; the byte-identity harness against the pre-C1 binary still passes for the once-mode CLI surface. Beads-Ref: mdo-6w5.4 Agent-Signature: kilo-kilo-auto-efficient on behalf of matt
C5 / Beads mdo-6w5.5 (epic mdo-6w5, PR order 5 of 6; stacked on the C3 branch). Mirror image for the Linux side of the Windows shape the code already has (pure plan + thin executor + typed results): - linux_install_plan(data_home, current_exe, set_default) is a PURE function: the desktop-file/icon targets, the entry content (which now shows it carries the handler invocation contract), the default mimes, and the legacy-cleanup root - unit-testable without spawn-capture. - apply_linux_install_plan(plan) is the only file-touching step; it returns LinuxInstallReport (paths written + LinuxDefaultOutcome: Applied / NeedsManualHint / NotRequested) instead of reporting through stdout mid-apply. - print_linux_install_report(report) is the executor-caller's CLI surface; the exact lines mirror the pre-plan flow byte for byte (verified: install output diffed against the pre-epic binary modulo the fixture paths; both exit 0). - The three _impl trampolines (install/uninstall/integration_installed across linux/windows/not-any cfg bodies) are collapsed into the pub fns directly, on every platform including the not(any) fallbacks. - Dead pubs removed: install_linux_for_exe and install_windows_for_exe had zero external callers; the Windows body moves into install() verbatim (only the handler locals rename to avoid the new `handler` module import shadow). Doc references updated. SCOPE LINE per ADR-0004 (Deferred): strictly the v0.6 minimal owned-registration check - no repair flows, no remnant cleanup, no partial-state classification; reopening ADR-0004 is out of scope. The scripts/uninstall-*.legacy helper scripts are a known duplication but explicitly out of scope; a retirement decision would be its own ADR. Gates: fmt --check, clippy --all-targets --all-features -D warnings, cargo test --all-targets --all-features --locked all green (3 new Linux plan/executor tests; Windows-side tests unchanged and still asserting the registry plan byte-exactly). Beads-Ref: mdo-6w5.5 Agent-Signature: kilo-kilo-auto-efficient on behalf of matt
C4 / Beads mdo-6w5.6 (epic mdo-6w5, PR order 6 of 6 - last so code moves once into the shapes C1/C2/C3/C5/C6 created). lib.rs was the #2 churn file: 1703 lines carrying four implementations plus a 565-line inline test module. Pure relocation, external interface unchanged (zero caller churn: the crate root re-exports every public name at its old path): - src/render.rs (1006) - the render pipeline per GLOSSARY: parsing, highlight splicing, sanitation, title derivation, stragglers sorted by caller at relocation time (dir_to_file_url: only caller is convert; front-matter/strip helpers: derive_title's), and the single deep entry point convert() with its request/outcome/error types and the O_NOFOLLOW output writer. - src/page.rs (379) - page assembly: assets and CSS/JS consts, wrap_html5, escape_style_end_tags, plus the calendar/datetime math stragglers sorted by caller (wrap_html5's timestamp footer) and html_escape (all callers are wrap_html5's). wrap_html5/source_modified_unix_secs become pub(crate) for convert's base-href and timestamp hand-off. - src/temp_output.rs (167) - temp output: per-source private paths, stem sanitation, private dirs (pub(crate) ensure_private_dir/private_temp_ root for the sample lifecycle at the root). - src/launch.rs (98) - browser openers: ShellExecuteW/opener path, macOS `open`, the xdg-open/gio fallback chain and reaper window. - src/lib.rs (76) - crate docs, module decls, root re-exports, and the setup-sample orchestration (open_setup_sample composes render + temp_output + launch through the same root names). The inline tests moved beside the concepts they test: render owns the splice/sanitize/front-matter/placeholder-safety/title/dir_to_file_url groups; page owns wrap_html5, the CSS-escape test, and the datetime tests; temp_output owns the stem tests and their helpers; the root keeps the setup-sample test. Test counts unchanged. Gates: fmt --check, clippy --all-targets --all-features -D warnings, cargo test --all-targets --all-features --locked all green (113 tests); the byte-identity harness against the pre-epic binary passes all 11 scenarios including rendered-HTML cmp - the split changed no behavior. Beads-Ref: mdo-6w5.6 Agent-Signature: kilo-kilo-auto-efficient on behalf of matt
maphew
force-pushed
the
agent/6w5-c5-linux-plan
branch
from
October 7, 2026 17:06
fce849f to
2239ee7
Compare
maphew
force-pushed
the
agent/6w5-c4-lib-split
branch
from
October 7, 2026 17:06
c5d2bb2 to
7d3d389
Compare
maphew
force-pushed
the
agent/6w5-c5-linux-plan
branch
from
October 7, 2026 17:16
2239ee7 to
326f012
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
C4: Split lib.rs by its four real concerns
Beads mdo-6w5.6 · parent epic mdo-6w5 (PR order 6 of 6; stacked on C5
agent/6w5-c5-linux-plan→ C3 → C2 → C6 → C1)What
lib.rs (1703 lines, the #2 churn file) splits into the four modules named in GLOSSARY.md — pure relocation, zero caller churn (the root re-exports every public name at its old path):
render.rsdir_to_file_url(caller:convert), the deepconvert()+ request/outcome/error types, O_NOFOLLOW output writerpage.rswrap_html5,escape_style_end_tags,html_escape(all callers:wrap_html5), calendar/datetime math (timestamp footer);wrap_html5/source_modified_unix_secsbecomepub(crate)temp_output.rspub(crate)helpers for the sample lifecycle)launch.rsopen, xdg-open/gio fallback chainlib.rsInline tests moved beside the concepts: render (splice/sanitize/front-matter/placeholder-safety/title/dir_to_file_url), page (wrap_html5, CSS escape, datetime), temp_output (stem tests + helpers), root (setup-sample). Test counts unchanged.
Gates
fmt --check ✓ · clippy -D warnings ✓ ·
cargo test --all-targets --all-features --locked✓ (113 tests) · byte-identity harness vs the pre-epic binary: all 11 scenarios identical including rendered-HTML cmp — behavior unchangedSigned: kilo-kilo-auto-efficient on behalf of matt (Beads mdo-6w5.6)