Repository navigation
Deepen mdo C3: one handler invocation contract - #45
Conversation
![kilo-code-bot[bot]](https://avatars.githubusercontent.com/in/2193792?s=60&v=4){kind=small}
|
|
||
| pub fn run() -> io::Result<()> { | ||
| let mdo = sibling_binary("mdo.exe")?; | ||
| let mdo = mdo_cli::handler::sibling_binary(&std::env::current_exe()?, "mdo.exe")?; |
There was a problem hiding this comment.
[WARNING]: Dead check and now-unused import left over from the sibling_binary unification
handler::sibling_binary only returns Ok when the sibling passes is_file() (src/handler.rs:53), so the if !mdo.exists() block below can never fire and its "expected mdo.exe next to mdo-setup.exe at ..." error is unreachable. Deleting the local sibling_binary fn also leaves use std::path::PathBuf; at line 321 unused in Windows builds (unused_imports warning). The Linux branch (linux_setup::run, line 96) already relies on the shared error alone - aligning this branch keeps the two platforms consistent.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
| #[cfg(target_os = "windows")] | ||
| use std::process::Stdio; | ||
|
|
||
| use crate::handler; |
There was a problem hiding this comment.
[SUGGESTION]: Import unused on non-Linux/non-Windows targets
use crate::handler; is referenced only inside #[cfg(target_os = "linux")] and #[cfg(target_os = "windows")] code (lines 327, 511, 535 and the cfg-gated test modules), so on any other target - e.g. the not(any(linux, windows)) fallback path at lines 286-305 - it is an unused import. The file already cfg-guards such platform-only imports (#[cfg(any(target_os = "linux", target_os = "windows"))] use std::fs; at lines 29-30); matching that pattern keeps clippy -D warnings clean on cross-builds.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
| assert!(!wrapper.contains(handler::OPEN_FLAGS[0])); | ||
| } | ||
|
|
||
| use super::*; |
There was a problem hiding this comment.
[SUGGESTION]: Duplicate use super::*;
The module already globs the same use super::*; at line 1001; this second one (inserted above the pre-existing registry_command_uses_open_flag_without_wrapper test) is a redundant leftover from adding the new contract test. Duplicate glob imports compile cleanly, but only the one at the top of the module is needed.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
|
|
||
| #[test] | ||
| fn unknown_termination_falls_back_to_failure() { | ||
| // Signal deaths (and nested nested process terminations) offer no |
There was a problem hiding this comment.
[SUGGESTION]: Duplicated word in comment
Comment-only: "nested nested" looks like a doubled word.
| // Signal deaths (and nested nested process terminations) offer no | |
| // Signal deaths (and nested process terminations) offer no |
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
Code Review SummaryStatus: 3 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 re-review of HEAD 5f2bdf1 (since 9eea29c). The only hand-written change in this window is the Previous Review Summaries (2 snapshots, latest commit 9eea29c)Current summary above is authoritative. Previous snapshots are kept for context only. Previous review (commit 9eea29c)Status: 3 Issues Found | Recommendation: Address before merge Overview
Issue Details (click to expand)WARNING
SUGGESTION
Files Reviewed (5 files)
Fix these issues in Kilo Cloud Notes: Incremental re-review of HEAD 9eea29c (since ba5b88d). Both hunks in the incremental diff are fixes and verified sound: the removed Previous review (commit ba5b88d)Status: 4 Issues Found | Recommendation: Address before merge Overview
Issue Details (click to expand)WARNING
SUGGESTION
Files Reviewed (5 files)
Notes: Read-only review of HEAD ba5b88d. The extraction of Reviewed by efficient · Input: 45.2K · Output: 7.9K · Cached: 284.2K |
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
- mdo-setup.rs: handler::sibling_binary already returns NotFound when the sibling isn't a regular file, so the post-call is_file check was dead code. - file_manager.rs: removed duplicate 'use super::*;' in windows_tests module. Agent-Signature: kilo-kilo-auto-efficient on behalf of matt
f59f85c to
5e07917
Compare
9eea29c to
5f2bdf1
Compare
1 participant
C3: Unify the Open-as-HTML handler invocation contract
Beads mdo-6w5.4 · parent epic mdo-6w5 (PR order 4 of 6; stacked on the C2 branch
agent/6w5-c2-watch-module→ C6 → C1)What
New
src/handler.rs(lib crate) owning the two facts that were spelled six times across the platform adapters:OPEN_FLAGS+open_argv(): the--openargv prefix spelled once. The file argument stays platform presentation (%fplaceholder,%1in a registry command, real path when spawned). Consumers: the Linux desktop-entry Exec template, the Windows registry command builder,mdo-open's forward.sibling_binary(exe, name): one discovery function, one error mode (NotFoundnaming both the anchor exe and the path checked, printable as-is).mdo-setup's two local copies are deleted; Windows onboarding's inline eageris_filecheck now applies uniformly through the shared error.In-pass fixes (recorded in the issue):
mdo-opensilent exits now print a message before a non-zero exit:current_exe()failure, missing sibling target in any branch, spawn failures.exit_code_from_statussplit intoexit_code_from_parts(success, code)so the truncation rule is unit-testable portably: a failure whose low byte is zero (e.g. 0x100) must not read as success — 4 portable tests + a windows-only end-to-end run by the repo's Windows CI.Contract-anchored emission tests (2): the Linux
Exec=line and the Windows registry command derive their expectations fromhandler::OPEN_FLAGSrather than restating literals — the two emitted forms can no longer drift apart. (Windows-side byte-exact literal tests already present remain.)Gates
fmt --check ✓ · clippy -D warnings ✓ ·
cargo test --all-targets --all-features --locked✓ (105 tests, +4 handler unit tests +4 mdo-open exit-code tests) · byte-identity harness vs pre-C1 binary still passesSigned: kilo-kilo-auto-efficient on behalf of matt (Beads mdo-6w5.4)