✨ feat(exec): container execution block on exec profiles - #527
Conversation
…iner `[exec.profiles.<name>.container]` wraps that profile's command in `<runtime> run` instead of running it on the host: `image` required, `runtime` auto-detected (docker first, then podman) and honoured verbatim when set, `extra_args` spliced in after gwm's own flags and before the image. The block rides a profile only. The inline `gwm exec -- <cmd>` is the frozen 1.0 surface (#319) and `ExecConfig` pledges it unchanged, so no config file gets to containerise a command line that used to run locally. The mount is the feature rather than the wrapper. A linked worktree's `.git` is a FILE holding the absolute host path of `<main>/.git/worktrees/<id>`, so a container with only the worktree mounted answers no git command at all. gwm mirrors host paths and mounts the commondir alongside, skipping the second mount when it already lives inside the worktree (the main checkout, reachable via an explicit slug). Once a host-absolute path has to be reproduced anyway, `/workspace` buys nothing, and `{path}` / `GWM_PATH` stay true on both sides. argv, never a shell string: no token is quoted, joined or re-parsed at any point. The reference implementation joins and shell-quotes because it feeds tmux; this one executes argv directly, and GHSA-fffq-vg6f-gxqm makes that an invariant rather than a happy consequence. The TUI exec picker wraps the same way, so a profile cannot mean "host" there and "container" on the CLI. `run_in_dirs_parallel` now takes an argv PER item, since the mount is the worktree's own path. `validate_exec_profile` replaces `validate_exec_profile_command` and destructures the profile exhaustively, so the next field added cannot be silently skipped by a validator (the `gwm doctor` failure mode from #392). Tests ship with the change rather than in their own commit: the signature change means src-alone and tests-alone both fail to build, and a commit that does not build breaks bisect. Each new guard was run red first by neutralising the line it protects, then restored. refs #421
…lock The `[exec]` section of the schema page gains the field table, the exact command gwm builds, and the reason the gitdir mount exists rather than a list of flags. English and French, since a missing French half was the gap 1.5.0 shipped with. The CLI reference gets a paragraph under `gwm exec`, and `examples/gwm.toml.example` a commented profile, so `gwm init` writes the schema it documents. Four decisions are written down rather than left to be rediscovered from the code: the block rides a profile only, the command is the container's CMD (an image with an ENTRYPOINT receives it as arguments), there is no `interactive` knob because a TTY per container means nothing across a fan-out, and `extra_args` lands last so a repeated flag overrides gwm's. Taking `-w /workspace` is documented with its consequence attached, since the worktree stays mounted at its host path. Caches point at named volumes rather than host paths: `.gwm.toml` is committed and travels between machines, and there is no `~` / `$VAR` expansion in `extra_args`. The roadmap item is marked done in the three files that carry it, with what shipped narrower than the reference and why, including the note that `[aliases]` needed nothing despite the issue title: a gwm alias is argv substitution towards a subcommand, so it inherits the container through the profile it expands to. closes #421
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
… refuse Windows Three findings from the Codex review, all on the container wrapper itself. **The mount was undone by ownership.** With a rootful Docker on Linux the container runs as uid 0 while the bind-mounted tree belongs to the host user, and git refuses such a repository as `dubious ownership` — so the gitdir mount this feature exists for bought nothing there. Every path gwm mounts is now declared `safe.directory` through `GIT_CONFIG_*` environment: nothing is written to any config file, and the set is exactly the mounted set rather than the blanket `*`, so the ownership check stays on for everything else. Same fix CI providers apply to their own checkouts. File ownership itself is unchanged and documented: `extra_args = ["--user", "…"]` is there for whoever needs it, and is not a default because images expect a writable `CARGO_HOME` and `apt-get`. **The TUI overlay lost its terminal.** `gwm exec` allocating no TTY is correct — a terminal per container means nothing across a fan-out — but the exec overlay spawns into a real pty (`portable_pty::openpty`), so the same profile lost stdin and its terminal there: no REPL, no debugger, no prompting command, capabilities it has when it runs on the host in that same overlay. `wrap_interactive` adds `-i -t` for that path only, and a test pins that the tty flags are the ONLY difference between the two wraps. **Windows is refused, not half-honoured.** The design mirrors host paths, and `C:\…` is neither mountable nor resolvable inside a Linux container; worse, a linked worktree's `.git` file would still name a drive-letter path, so even a translated mount would leave git unable to answer. A profile carrying `[container]` now fails there with a message saying so, rather than reaching `docker run` to fail obscurely. The argv tests move under `#[cfg(unix)]` for the same reason: `components()` renders a POSIX path as `\wt\feat-1` on Windows, so they were asserting a shape that platform can never produce. refs #421
…hat a mount cannot express Three more findings from the Codex review, all on the wrapper. **The container outlived its overlay.** Killing the pty leader kills the `docker` client, never the container: the daemon owns it, and `--rm` only fires once it exits. So closing the exec overlay with `Esc` left a long command still writing to the worktree, invisibly. The interactive wrap now names its container (`--name gwm-<worktree>-<n>`, reduced to the character class a container name accepts) and `PtyOverlay` gained an optional teardown argv, run once on `kill` — including through the early return taken when the client was already reaped, because that says nothing about the container. Every other overlay (lazygit, review, a host command) carries none: the process-group signal already covers those. **SELinux left both mounts unreadable.** On an enforcing host (Fedora, RHEL) an unlabelled bind mount gets the container process `EACCES` on the worktree and the gitdir, so both announced runtimes fail before the command runs. `selinux_relabel = true` suffixes gwm's own mounts with `:z`. It is a field rather than a documented `extra_args` recipe because `extra_args` cannot reach the mounts gwm builds itself, and it is opt-in rather than detected because relabelling writes a shared label to the host tree, recursively, including the main checkout's `.git`. **A `:` in a mounted path is refused.** It is legal in a Unix path but is the field separator of `-v source:destination`, so the mount cannot be expressed and the runtime would reject the spec with a message about neither the worktree nor gwm. Both the worktree and the gitdir are checked. `build_container_argv` therefore returns a `Result`, propagated through `ExecPlan::argv_for` so the fan-out fails before running anything. refs #421
…resolve every mount before running one Four findings from the third Codex pass, all on the teardown added in the previous commit. **Two gwm processes agreed on a container name.** `seq` restarts at 1 in every process, so two TUIs opening their first overlay on the same worktree both produced `gwm-<stem>-1`. The loser of that race does not merely fail to start: its teardown then runs `docker rm -f` on the winner's container. The name carries gwm's pid now, and a test pins that two pids never collide at the same seq. **`extra_args` could take the name over.** A runtime honours the LAST `--name` it is given, so `extra_args = ["--name", "custom"]` left the overlay tearing down a container that was never started, and possibly one belonging to something else. `--name` (and `--name=…`) is refused in `extra_args`, on both the config-validation path and the command path. `validate_container_image` becomes `validate_container` and destructures the block exhaustively, so the next field cannot be added without a decision about validating it. **A worktree could run before another was refused.** `gwm exec` resolves everything upfront by contract (#326) so nothing runs when any target is unusable, but the `:`-in-path refusal was evaluated inside the sequential loop: the first worktree ran, then the second failed. Every argv is built before the first spawn now. The guard is an end-to-end test over two worktrees, and it was verified red — with the pre-resolution removed, the first worktree's `docker run` argv appears on stdout before the refusal. (The first attempt at that probe left the pre-resolution in place and passed vacuously; the second one is the real one.) **The teardown ran from gwm's own directory.** A `runtime` may be a relative wrapper script, which the pty spawn resolves against the worktree; the teardown inherited gwm's cwd, so it could run a different binary, or none, and leave the container up. It carries the worktree now. refs #421
…pace, and tear down a container the overlay never got Two findings from the fourth Codex pass; the third is refuted below. **Upfront resolution was per repo, not per workspace.** The previous commit built every argv before running anything *within* a repo, but `cmd_exec_workspace` runs repos in sequence, so a worktree of the last repo holding a `:` in its path surfaced only after the first repo had already run its command. That is exactly the guarantee #326 exists for. Every repo's argv is now built before any repo runs, with an end-to-end test over a two-repo workspace, verified red. **A spawn could fail after launching the container.** `PtyOverlay::spawn` is fallible after `spawn_command` (the reader clone and the writer take both are), and only the `Ok` arm attached the teardown, so a container that had already reached the daemon would keep running with no overlay to close it. The teardown body becomes a free-standing `run_teardown_now`, called from the `Err` arm too. **Refuted: "verify the container's identity before `rm -f`".** The pass argued a name collision could make gwm remove a container it did not create, and suggested a `--cidfile`. The name is `gwm-<worktree>-<pid>-<seq>`, so a collision requires another gwm process with the SAME pid — and two live processes cannot share one. Any container answering to that name is therefore an orphan of a previous gwm on the same worktree, whose removal is the wanted outcome, and the colliding `docker run` fails visibly in the overlay meanwhile. A cidfile would trade that for reading a file back before building an argv, in a path that cannot use a shell. refs #421
Boucle de review Codex : arrêt sur stagnation après 4 passes
Basculement à la passe 3 : à partir de là, la review ne parle plus du wrapper mais du teardown écrit pour la passe 2, puis du nom écrit pour la passe 3. Signal d'auto-alimentation sur deux passes consécutives, donc arrêt. Trajectoire de gravité : le finding le plus grave de la passe 1 était « git ne répond pas dans le conteneur sous Docker rootful » (le cœur de la feature). Celui de la passe 4 est « et si un pid réutilisé rencontrait un conteneur orphelin homonyme ». 13 findings, tous traités : 12 corrigés (safe.directory, TTY de l'overlay, refus Windows, teardown du conteneur, SELinux, refus du Ce que la review ne pouvait pas donner, et qui a été fait à la place : le chemin conteneur n'avait jamais été exécuté. Une exécution réelle (OrbStack 29.4.0, Elle valide aussi le comportement ENTRYPOINT documenté : Reste à valider hors review, non couvert ici : un hôte Linux avec Docker rootful (le chemin |
…x gates erase its users The Windows job did not fail an assertion, it failed `rustc` under `-D warnings`, so no test of that crate ran at all: the Windows coverage was unknown rather than red. Three items in `tests/exec_tests.rs` lost every caller once the `#[cfg(unix)]` gates erase the container tests: the `build_container_argv` import, the `mount_sources` helper, and `common::init_repo` through `mod common`. Each is gated now. Verified by reproducing the Windows configuration locally, since `cargo test --no-run` cannot: `unix` is true there. Rewriting every `#[cfg(unix)]` to `#[cfg(any())]` and every `#[cfg(windows)]` to `#[cfg(all())]` across the four touched test files, then `RUSTFLAGS="-D warnings" cargo check --all-targets`, reproduces exactly the three CI errors before the fix and reports nothing after it. refs #421
# Conflicts: # CHANGELOG.md
Unrelated to #421, and required to merge anything: `dev` is red at 7202e74. That commit made the flake verify the MSRV floor, so the binding became `devShells.default = assert msrvOk; pkgs.mkShell { … }`, while this test asserted the literal substring `devShells.default = pkgs.mkShell`. Nothing in the change was wrong; the test owned a spelling instead of a property. It now reads the line that carries the binding and asserts that the default devShell IS a `pkgs.mkShell`, whatever guard stands between the `=` and it. Still a real guard, not a neutralisation: swapping `pkgs.mkShell` for `pkgs.buildEnv` in the flake turns it red again (verified). The failure was invisible from this branch until it merged: CI tests `refs/pull/N/merge`, so it ran my `flake_tests.rs` against their newer `flake.nix` — a semantic conflict git cannot see.
…otes #527 (container execution, #421) landed while this branch was in review, and it carries its own fix for the flake devShell assertion. Four conflicts: - `tests/flake_tests.rs` — both sides fixed the same broken literal. Took `dev`'s, which is already merged and does the same work; keeping mine would have diverged the file for nothing, and the commit that added it is now redundant. - `tests/cli_workspace_tests.rs` — pure adjacency, my #515 workspace-note test and their #421 fixture landed at the same spot. Both kept. - the two docs roadmaps — kept this branch's #515 line, which still says "in review as PR #530", and `dev`'s #421 line, which now carries its tick. Gate on the merged tree: fmt, clippy and 94 test suites green, no failures. refs #515
The heading carried a ✅ and the entry a ticked box while PR #530 was still in review, which both `docs/7.roadmap.md` and its French mirror correctly said. #421's tick, one item down, appeared only once #527 merged — that is the convention, and this merge is where it is visible side by side. refs #515
Description
[exec.profiles.<name>.container]runs a profile's command insidedocker run/podman runinstead of on the host. Three fields:image(required),runtime(auto-detected, docker first then podman) andextra_args(spliced in after gwm's own flags, before the image).The wrapper is the cheap half. The mount is the feature: a linked worktree's
.gitis a file holding the absolute host path of<main>/.git/worktrees/<id>, so a container with only the worktree mounted answers no git command at all: nogit status, nogit describeto stamp a version, no commit, no hook, no coding agent that touches git. gwm mirrors host paths and mounts the main checkout's gitdir alongside, which is the column the reference implementation loses by default.Closes #421
Type of change
Changes
[exec.profiles.<name>.container](src/config.rs):imagerequired and non-empty,runtimeoptional,extra_argsoptional.deny_unknown_fields, so a key copy-pasted from the reference's wider schema is refused rather than ignored.build_container_argv(src/exec.rs): pure, no spawn, noPATHread, no filesystem. Emits<runtime> run --rm -v <wt>:<wt> -v <main>/.git:<main>/.git -w <wt> <extra_args…> <image> <cmd…>, skipping the second mount when the gitdir already lives inside the worktree (the main checkout, reachable via an explicit slug).ContainerPlancarries the resolved runtime + commondir across the fan-out;resolve_container_runtimetakes an injected availability predicate so the tests never depend on the runner having a container runtime.src/cli.rs,src/tui/app.rs): the CLI fan-out (single-repo and--workspace, each repo resolving its own commondir) and the TUI exec picker. The same profile cannot mean "host" in one and "container" in the other.run_in_dirs_parallelnow takes an argv per item: the mount is the worktree's own path, so no two worktrees run the same argv.validate_exec_profilereplacesvalidate_exec_profile_commandand destructures the profile exhaustively, so the next field added toExecProfilecannot be silently skipped by a validator (thegwm doctorfailure mode from [Feature]: add Symfony preset #392).━━ feat-1 (/path) [docker rust:1.90].gwm-toml, CLI reference, roadmap), README,examples/gwm.toml.example, CHANGELOG, ROADMAP.Decisions the issue asked to be resolved rather than discovered
gwm exec -- <cmd>is the frozen 1.0 surface (1.0: decide final shape of the exec/clean MVP cuts (#313) #319) andExecConfigpledges it unchanged, so no config file gets to containerise a command line that used to run locally.[aliases]needed nothing despite the issue title: a gwm alias is argv substitution towards a subcommand, not a custom command, sot = "exec --profile ci"inherits the container through the profile it expands to.interactive/ TTY knob.gwm execis a fan-out over N worktrees, where a TTY per container means nothing. Omitting it is the resolution of the conflict the issue names; it belongs to the surfaces that can honour it (multiplexer windows, PTY overlay).ENTRYPOINTreceives it as arguments;extra_args = ["--entrypoint", ""]opts out. Nosh -cgymnastics, those existed only because the reference builds a string.runtimeis honoured even when absent fromPATH, since the spawn error names it better than a config error could.-w,-eand-vare alreadydocker runflags andextra_argslands last, so a repeat overrides gwm's own. No~/$VARexpansion; for caches the docs point at named volumes, which are portable where a host path in a committed.gwm.tomlis not.gwm execinjects noGWM_*today, and mirroring host paths keeps{path}/GWM_PATHtrue on both sides, so the reference'sMAIN_WORKTREE_PATHremapping bug has no counterpart here.Tests
cargo testpasses locally (also on a stripped CI-like PATH:PATH="$(dirname "$(command -v cargo)"):/usr/bin:/bin" cargo test)cargo fmt --checkpassescargo clippy --all-targets -- -D warningspassestests/New coverage:
tests/exec_tests.rs: argv shape, the gitdir mount for a real linked worktree (asserts the path the.gitfile actually names is covered by a mount, not merely that an extra-vexists), the dedupe (including a trailing-separator path, which is what git2 hands out for a main worktree),extra_argsordering, the no-quoting invariant, runtime resolution (preference, explicit override, no-runtime error), and the frozen inline surface staying host-only.tests/config_tests.rs: parse,deny_unknown_fields, missingimage, emptyimagerejected on the config-validation path.tests/cli_workspace_tests.rs: end-to-end through the binary withruntime = "echo", proving each repo mounts its own gitdir. Crossing them would be worse than the bug this fixes: git would answer, against the wrong repository.tests/tui_app_tests.rs: the picker wraps a container profile and leaves a plain one alone.Each new guard was run red first by neutralising the line it protects (gitdir mount, dedupe, config validation, TUI wrap), then restored. A green guard proves nothing on its own.
Actual container execution is not CI-testable and stays out, per the issue.
Notes for reviewers
run_in_dirs_parallelchanges signature, so src-alone and tests-alone both fail to build, and a commit that does not build breaks bisect. The four that follow are the Codex review loop, one per pass.extra_argsopens no escalation. It is config-sourced, likecommand, andcommand = ["docker", "run", "--privileged", …]was already expressible in a profile before this PR.gwm execdeliberately has no trust gate (it runs the user's own command against their own worktrees), unchanged here.gwm doctordoes not probe the runtime binary the way it probes hook binaries. That is deliberate and consistent with "an explicitruntimeis honoured even when absent fromPATH": a wrapper script or anerdctlshim must not be reported as broken.Path::starts_withis component-wise) while the mount is normalised throughcomponents(). Both cases are pinned.Verified by running it, and what is NOT verified
The container path was exercised for real before merging, not only reasoned about: OrbStack 29.4.0, image
alpine/git, a throwaway repo with a linked worktree.That is the claim of #421, reproduced rather than deduced. The same run confirms the documented ENTRYPOINT behaviour:
alpine/gitdeclaresENTRYPOINT ["git"]andcommand = ["status", "--short", "--branch"]arrives as arguments to it.Four environments remain unvalidated. None of them is exercised by CI, and none of them can be, since container execution is not CI-testable (the issue says so):
safe.directorydeclaration is what keeps git answering as uid 0 against a host-owned tree. Verified by construction (the argv is pinned by a test), never observed. Also unobserved: files the command creates are owned by root on the host, which is documented but not measuredselinux_relabel = trueappends:zto gwm's own mounts. The argv is pinned; the actualEACCES-without-it and the recursive relabel's cost on a large.gitare notruntime, but every real run so far went through dockerkillruns<runtime> rm -f <name>and the unit test standstouchin for it. A container actually interrupted mid-build has not been observedA reviewer with any of these hosts is the fastest way to close the gap. Everything else in the PR is covered by the suite.
Linked issues / docs
docs/4.configuration/1.gwm-toml.mdanddocs/fr/4.configuration/1.gwm-toml.md