Repository navigation
feat: shared versioning for all binaries based on git - #815
Merged
Merged
Conversation
Contributor
📝 WalkthroughWalkthroughThe workspace now uses a dedicated ChangesGit-derived versioning
Estimated code review effort: 3 (Moderate) | ~25 minutes Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
Comment |
norrietaylor
approved these changes
Jul 17, 2026
10000
View reviewed changes
twitchyliquid64
enabled auto-merge (squash)
July 17, 2026 20:31
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
crates/version/build.rs (1)
25-57: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueVersion derivation logic is sound.
The SemVer ordering strategy is correct: post-release commits encoded as pre-releases of the next patch version (
1.2.4-dev.N.g<hash>) sort above the release tag and below the next release. Pre-release tag extensions (1.2.3-rc1.dev.N.g<hash>) also order correctly. The fallback toCARGO_PKG_VERSIONwith optional commit hash for untagged trees is well-handled.One minor inconsistency: lines 51-52 use the newer
cargo::warning=directive syntax while lines 55-56 and 149/158 use the oldercargo:rustc-env=/cargo:rerun-if-changed=syntax. Both work in modern Cargo, but mixing them is inconsistent.🎨 Suggested syntax consistency
- println!("cargo::warning=Building a release binary from an unclean working tree!!"); - println!("cargo::warning=Always build production binaries from a clean checkout."); + println!("cargo:warning=Building a release binary from an unclean working tree!!"); + println!("cargo:warning=Always build production binaries from a clean checkout.");Or conversely, update all to
cargo::syntax if the project targets Rust ≥ 1.77.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/version/build.rs` around lines 25 - 57, Use a consistent Cargo build-script directive syntax in main and the related helpers: either change the warning directives to the established legacy cargo: form, or update all rustc-env and rerun-if-changed directives to cargo:: if the project requires Rust 1.77 or newer. Preserve the existing warning and environment-variable behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@crates/version/build.rs`:
- Around line 25-57: Use a consistent Cargo build-script directive syntax in
main and the related helpers: either change the warning directives to the
established legacy cargo: form, or update all rustc-env and rerun-if-changed
directives to cargo:: if the project requires Rust 1.77 or newer. Preserve the
existing warning and environment-variable behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: fc704bc0-d5a3-41c5-ab7b-c57f4de20f31
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (16)
Cargo.tomlcrates/minimal/Cargo.tomlcrates/minimal/build.rscrates/minimal/src/lib.rscrates/minimald/Cargo.tomlcrates/minimald/build.rscrates/minimald/src/main.rscrates/minimald/src/rpc.rscrates/minvmd/Cargo.tomlcrates/minvmd/src/main.rscrates/mip/Cargo.tomlcrates/mip/build.rscrates/mip/src/main.rscrates/version/Cargo.tomlcrates/version/build.rscrates/version/src/lib.rs
💤 Files with no reviewable changes (3)
- crates/minimald/build.rs
- crates/mip/build.rs
- crates/minimal/build.rs
gominimal-aw-bot Bot
added a commit
that referenced
this pull request
Oct 3, 2026
When `min session exec` runs a command whose local stdout closes early (e.g. `min session exec <s> -- sh -c 'yes' </dev/null | head -1`), the old exec path replaced this process with ssh, so nothing watched local stdout. When the reader closed, ssh's write failed with BrokenPipe but ssh didn't exit — it sent eow@openssh.com, which the daemon's russh-based SSH server doesn't handle, leaving the remote process running indefinitely. Fix: on the exec path, spawn ssh with stdout piped, relay stdout to this process, and on BrokenPipe kill the ssh child and exit 141 (128+SIGPIPE). Otherwise propagate ssh's exit code via exit_code_of. The interactive attach path (no wire) is unchanged. Routing-Key: inbox-route/I_kwDOSUhdos8AAAABUwHJ5A Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
yermakoffivan
pushed a commit
to yermakoffivan/minimal
that referenced
this pull request
Oct 5, 2026
…imal#1881) * fix(minimal): relay exec stdout to detect BrokenPipe and kill ssh (gominimal#815) When `min session exec` runs a command whose local stdout closes early (e.g. `min session exec <s> -- sh -c 'yes' </dev/null | head -1`), the old exec path replaced this process with ssh, so nothing watched local stdout. When the reader closed, ssh's write failed with BrokenPipe but ssh didn't exit — it sent eow@openssh.com, which the daemon's russh-based SSH server doesn't handle, leaving the remote process running indefinitely. Fix: on the exec path, spawn ssh with stdout piped, relay stdout to this process, and on BrokenPipe kill the ssh child and exit 141 (128+SIGPIPE). Otherwise propagate ssh's exit code via exit_code_of. The interactive attach path (no wire) is unchanged. Routing-Key: inbox-route/I_kwDOSUhdos8AAAABUwHJ5A Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * fix(min): kill ssh when the exec stdout relay fails * fix(min): kill ssh when min is signalled during an exec relay min is now ssh's parent on the exec path, so a SIGTERM, SIGHUP or SIGINT sent only to min's PID would orphan ssh with its stdout closed and the remote command running on. Catch those signals, kill ssh, and exit 128+signo. Log a failed kill instead of discarding it. --------- Co-authored-by: gominimal-aw-bot[bot] <281738952+gominimal-aw-bot[bot]@users.noreply.github.com> Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
bryan-minimal
added a commit
that referenced
this pull request
Oct 15, 2026
`min session exec` runs ssh as its child and `min proxy` as ssh's. A `min` that dies without taking ssh down (SIGKILL, an OOM kill; the catchable signals are handled since #815) leaves both holding the daemon's socket open, and the daemon learns of a lost client only from that socket closing, so the command ran on: a silent `sleep` outlived its killed client for as long as the lab watched (scenario 732, every distro, since early October). The proxy now takes `--owner <pid>` (the `min` that spawned ssh), looks for it every 250 ms and exits when it is gone; dropping the bridge closes the socket, and the daemon's existing client-loss path kills the command. The daemon is unchanged. Test: the watch holds while the owner lives and resolves once it dies. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
evanspearman
pushed a commit
to evanspearman/minimal
that referenced
this pull request
Oct 23, 2026
…gominimal#2226) * fix(minimal): close the daemon connection when the proxy's owner dies `min session exec` runs ssh as its child and `min proxy` as ssh's. A `min` that dies without taking ssh down (SIGKILL, an OOM kill; the catchable signals are handled since gominimal#815) leaves both holding the daemon's socket open, and the daemon learns of a lost client only from that socket closing, so the command ran on: a silent `sleep` outlived its killed client for as long as the lab watched (scenario 732, every distro, since early October). The proxy now takes `--owner <pid>` (the `min` that spawned ssh), looks for it every 250 ms and exits when it is gone; dropping the bridge closes the socket, and the daemon's existing client-loss path kills the command. The daemon is unchanged. Test: the watch holds while the owner lives and resolves once it dies. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> * fix(minimal): the proxy's owner watch takes a lock the kernel releases on death A `kill(pid, 0)` poll can be fooled: a recycled pid reads as alive, and so does a SIGKILLed `min` its parent has not reaped yet. Either leaves the proxy holding the daemon's socket, the leak the watch exists to end. `min` now holds an exclusive `flock` on a file beside the daemon socket for its whole life and hands the proxy its path (`--owner-lock`); the proxy exits the moment it can take that lock, which the kernel releases on any end of the owner. The pid stays as the fallback for a `min` that handed no lock. Tests: the lock watch holds while the lock is held and resolves when it is released; the pid fallback as before. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> * fix(minimal): a zombie owner reads as gone, and each owner lock is its own Review round on the owner watch. The pid fallback treated a SIGKILLed `min` its parent had not reaped as alive, since `kill(pid, 0)` answers for a zombie; on Linux it now reads `/proc/<pid>/stat` and takes state `Z` as gone. The lock watch already was (the kernel drops a flock when the process's descriptors close at exit, before any reaping), but its path was keyed by pid alone, so a recycled pid could re-create and hold the old owner's file; the name now carries the process's start time too. A free lock's file is unlinked before the lock is released, so no new holder can lose its file to that unlink. `cmd_proxy` hands its streams to `proxy_while_owner_lives`, which the tests drive: the daemon side reads EOF once the owner's lock is released, an unreaped owner reads as gone, and `attach_command` names this process as the proxy's `--owner`. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * fix(minimal): sweep stale proxy owner locks under one coordination lock Every attach and exec left its `proxy-owner-<pid>-<start>.lock` beside the daemon socket: the proxy unlinks an owner's file only if it was still watching when the owner went, and a clean completion ends the proxy first. Making a new owner file now first takes `proxy-owners.lock` (one per directory), removes every owner file whose lock is free, then creates and locks this process's, so a sweep never removes a file between its creation and its owner's lock. Without the coordination lock the proxy keeps the pid watch alone, as when no owner file can be made. Test: a free owner file is swept; a held one and the coordination file are kept. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * docs(minimal-client,minimal): clarify SSH attach and proxy lifetime docstrings * fix(minimal): only a missing owner lock means the proxy's owner is gone An open error other than NotFound (EACCES, EMFILE) read as the owner gone, and the proxy then dropped the daemon socket under a live attach, whose command the daemon ended. Only a missing file is the owner unlinking its lock on the way out; every other error keeps the watch going. --------- Co-authored-by: Claude Fable 5.1 <noreply@anthropic.com> Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
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
2 participants
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.
Implement shared versioning across our crates.
Summary by CodeRabbit
New Features
Bug Fixes