A terminal beside the agent: zellij in every container (#242) - #243
Conversation
zellij lands on PATH in every container dl launches, from pixi, container-side, as a stage of the setup pass that every `devpod up` already pays -- so it costs no round trip, and a failure is contained and named rather than able to fail a launch. The route was decided by the ticket's falsification measurement rather than by taste. A warm `pixi global install zellij` against the shared package cache cost 0.56s / 0.23s / 0.23s over three fresh devcontainers/base:ubuntu containers (3.0s cold, filling 167MB), so the host-side static-musl-binary mount -- faster, but a new upstream GitHub-release dependency and a mount that only lands at container creation -- stays a recorded alternative. Deliberately not a third row in REQUIRED_TOOLS: that tuple is also what the probe asks about and what the host lends, so zellij there would put every container holding gh and a real claude back onto the ~300MB lending path on every launch and still never install zellij, because a successful lend returns ahead of the install trip. `DEVLAUNCH_ZELLIJ=1` then makes `dl <spec> -- <cmd>` ensure the session exists before running the command. Off by default. The command runs beside the session rather than inside a pane of it, because a pane would take its stdin, stdout and exit status away from dl -- and `zellij -s <name> action new-pane` works from a command that is in no session at all, so the pane costs nothing either way. A bare `dl <ws>` attach is untouched: it sends no command to wrap.
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
Reviewer's GuideThis PR provisions zellij into every devlaunch-managed container via a new setup stage and adds an opt‑in wrapping of remote commands so agents can open panes in a named zellij session, while keeping launch behavior, exit statuses, and spawn counts unchanged by default. File-Level Changes
Assessment against linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #243 +/- ##
==========================================
+ Coverage 93.16% 93.19% +0.02%
==========================================
Files 25 25
Lines 3715 3730 +15
==========================================
+ Hits 3461 3476 +15
Misses 254 254
🚀 New features to boost your workflow:
|
Four review residues, none of which changed what the feature does. Spec found the one factual overstatement: "a bare `dl <ws>` attach is untouched" is true of the *session* and not of the command that can precede it. `attach_workspace` puts the opt-in dotfiles refresh in front of the shell, and a refresh is a command, so with both switches on it is wrapped like any other. The effect is benign -- the session is waiting when the shell arrives -- but the claim as written was false, so the README and CHANGELOG now say the narrower true thing, and a test pins the composition rather than leaving the two switches assumed to compose. Standards found a trap rather than a bug: `provision_script`'s `tools` argument parameterizes which tools are installed and not the profile lines, so `provision_script((ZELLIJ_TOOL,))` would still write the claude-shim prepend. That is exactly why `zellij_script` assembles itself from the shared helpers, and the docstring now says so at the definition instead of leaving the next reader to discover it. `set -u` in `zellij_script`, for the same reason `provision_script` sets it: they are the same helpers. Its `exec >&2` stays deliberately absent, since a stage cannot redirect itself. And `setup_script` now accepts the stages its caller already built. `setup_stages` stopped being a pure function of `workspace` when the zellij stage became conditional, so `_setup_pass` was reading the environment twice -- once for the script it sends and once for the list it matches the outcomes against. A disagreement would have surfaced as a phantom `not reached`. pixi run ci: exit 0, 1821 passed, pylint 10.00/10.
blooop
left a comment
blooop
left a comment
There was a problem hiding this comment.
This was generated by AI during review.
Two independent fresh-context review axes (Standards, Spec), run in parallel; findings are not merged or reranked across axes. Fixed point: merge-base with main (c89536f); diff reviewed at head fc275d4. Spec = ticket #242 and its comments.
Standards
Gates, run first-hand on a fresh clone. pixi run ci real exit 0; pylint 10.00/10; ty clean; 1820 passed / 24 deselected. Matches the PR's claims exactly.
Red-ability, verified rather than read. Reverting devlaunch/ to the merge-base fails all 23 new tests. Because most of that is AttributeError on new names — which proves little — the axis ran five mutations of its own instead of trusting the PR's table: dropping >&2 on the stage, dropping the warm early-exit, forcing the stage past provisioning_disabled(), ; → && in the wrap, and dropping the profile prepend. Each killed exactly the test it should. TestZellijProvisioning drives real bash against a sandboxed $HOME/PATH with faked pixi/curl, so these are behavioural greens, not fixture-shaped ones. No false-green pattern found.
Non-blocking — a trap in the neighbour, not a bug here. zellij_script() repeats provision_script's skeleton, and the docstring argues the pieces are shared, which is true. But the reason it cannot simply call provision_script((ZELLIJ_TOOL,)) is that provision_script's profile lines ignore its tools argument entirely — verified: that call still emits the claude-shim prepend. The duplication is correct; what was missing was the reason being written down where the next reader meets it.
[Resolved during review — provision_script's docstring now states the limit at the definition site: tools parameterizes which tools are installed and not what they need on PATH, and the profile lines must be parameterized before it is reused. Left as documentation rather than a profile_lines refactor, since no production caller passes tools at all.]
Nit — set -u divergence. zellij_script() had neither set -u nor exec >&2 where provision_script has both. The >&2 is deliberate and explained; the missing set -u looked like an omission.
[Resolved during review — set -u added, with the exec >&2 absence now stated as the deliberate half: a stage cannot redirect itself, because _stage_snippet interpolates it into if <command>; then.]
Nit — setup_stages() evaluated twice per pass. Harmless while it only interpolated a workspace id; now that it reads provisioning_disabled(), the script _setup_pass sends and the list it matches outcomes against are two separate readings of the environment, which can in principle disagree and yield a phantom not reached.
[Resolved during review — setup_script takes an optional stages, and _setup_pass passes the tuple it already built.]
Nit, carried not fixed. os.environ.get(X, "").strip().lower() not in _FALSEY is now the 5th verbatim copy across four modules. Pre-existing; this PR extends it by one, and no test pins the vocabulary as shared.
Axis verdict: Approve
Spec
Every Done-when landed, each checked against the ticket's own wording.
"Every container devlaunch launches has a working zellij on PATH… no dependence on the user's dotfiles and no edit to any repo's devcontainer.json" — the axis ran the branch's real setup_script("revws") non-TTY in devcontainers/base:ubuntu: stage zellij ok, and a brand-new login shell resolved /home/vscode/.pixi/bin/zellij (0.44.3). Warm re-run 0.013s, profile line landed exactly once.
"Provisioning zellij must not be able to fail a launch… the wrap degrades to running the command directly" — verified structurally and live: with the hostname stage failing 1 the zellij stage still ran, and in a container with no zellij the wrapped payload printed sh: zellij: not found on stderr, put only AGENT_STDOUT in the redirect file, and exited 7. Both halves of the contract hold.
"An opt-in wrap, default off" — the switch reads os.environ.get(ZELLIJ_WRAP_VAR, "") against _FALSEY after .strip().lower(), so unset is off and a truthiness read would break it (confirmed by mutation).
Falsification condition, re-measured independently. The ticket said "Measure it and put the number in the PR; do not choose on taste." A number is in the PR, and the axis re-measured it on its own containers: warm 0.43s / 0.20s / 0.83s, cold 2.27s, cache 167MB — same order as the PR's 0.56/0.23/0.23 and 3.0s. The condition does not fire; the container-side pixi route is justified by measurement.
All three declared deviations upheld, each checked at the source. Not in REQUIRED_TOOLS: probe_script gates on _all_present(REQUIRED_TOOLS) and provision_tools returns True on a successful _transfer before reaching provision_script, so a zellij row really would make a gh+claude container re-lend every launch and still never install zellij. Beside rather than inside a pane: covered by the ticket's own "or equivalent", and verified live — the pane opened from a non-TTY command and exit 7 survived. restart rather than recreate: restart calls workspace_stop then workspace_up, which pays the setup pass.
Non-blocking. "A bare dl <workspace> is untouched, switched on or off" is stricter than the code: attach_workspace → dotfiles_update → workspace_ssh(command=…), so with the refresh opted in a bare attach does get the ensure prepended and the session created.
[Resolved during review — I reproduced the path independently before acting. The effect is benign, arguably the nicest arrival there is, but the claim was false as written: README and CHANGELOG now say the narrower true thing, and a new test pins the two switches composing rather than leaving it assumed. Verified red-able by neutralizing the wrap.]
Non-blocking. Done-when 1's "every container" is narrowed by DEVLAUNCH_NO_TOOLS — deliberate, argued in code, and documented in README; flagged only because the ticket's wording is unconditional.
Nit. The PR's mutation table claims each mutation killed its test "and no others"; of four re-run, three reddened extra tests. All in the safe direction, and the named test reddened in every case.
Axis verdict: Approve
Verdict
Approve at c18ff8a. Both axes approve independently, and the two things this PR could most easily have got wrong were checked by measurement rather than by reading: the route decision was re-measured on fresh containers by an axis that did not write it, and the "cannot fail a launch" guarantee was observed live in a container with no zellij, where the command still ran and returned its own exit status. The one factual overstatement — "a bare attach is untouched" — was real, and I reproduced it myself before narrowing the prose and pinning the true behaviour in a test.
Worth recording beyond this PR: the builder declined the ticket's own suggested mechanism (zellij as a third row in REQUIRED_TOOLS) and was right to, because that tuple also drives the probe and the lend; a row there would have made every already-equipped container re-lend ~300MB on every launch and still never install zellij. And zellij attach -b <name> exits 1 when the session already exists — the common path — so the || true is load-bearing rather than defensive.
Residues resolved on the branch; pixi run ci exit 0, 1821 passed, pylint 10.00/10. GitHub refuses the approve state on a same-account review, so this written verdict is what the gate reads.
blooop
left a comment
blooop
left a comment
There was a problem hiding this comment.
This was generated by AI during review.
Two independent fresh-context review axes (Standards, Spec), run in parallel; findings are not merged or reranked across axes. Fixed point: merge-base with main (c89536f); diff reviewed at head fc275d4. Spec = ticket #242 and its comments.
Standards
Gates, run first-hand on a fresh clone. pixi run ci real exit 0; pylint 10.00/10; ty clean; 1820 passed / 24 deselected. Matches the PR's claims exactly.
Red-ability, verified rather than read. Reverting devlaunch/ to the merge-base fails all 23 new tests. Because most of that is AttributeError on new names — which proves little — the axis ran five mutations of its own instead of trusting the PR's table: dropping >&2 on the stage, dropping the warm early-exit, forcing the stage past provisioning_disabled(), ; → && in the wrap, and dropping the profile prepend. Each killed exactly the test it should. TestZellijProvisioning drives real bash against a sandboxed $HOME/PATH with faked pixi/curl, so these are behavioural greens, not fixture-shaped ones. No false-green pattern found.
Non-blocking — a trap in the neighbour, not a bug here. zellij_script() repeats provision_script's skeleton, and the docstring argues the pieces are shared, which is true. But the reason it cannot simply call provision_script((ZELLIJ_TOOL,)) is that provision_script's profile lines ignore its tools argument entirely — verified: that call still emits the claude-shim prepend. The duplication is correct; what was missing was the reason being written down where the next reader meets it.
[Resolved during review — provision_script's docstring now states the limit at the definition site: tools parameterizes which tools are installed and not what they need on PATH, and the profile lines must be parameterized before it is reused. Left as documentation rather than a profile_lines refactor, since no production caller passes tools at all.]
Nit — set -u divergence. zellij_script() had neither set -u nor exec >&2 where provision_script has both. The >&2 is deliberate and explained; the missing set -u looked like an omission.
[Resolved during review — set -u added, with the exec >&2 absence now stated as the deliberate half: a stage cannot redirect itself, because _stage_snippet interpolates it into if <command>; then.]
Nit — setup_stages() evaluated twice per pass. Harmless while it only interpolated a workspace id; now that it reads provisioning_disabled(), the script _setup_pass sends and the list it matches outcomes against are two separate readings of the environment, which can in principle disagree and yield a phantom not reached.
[Resolved during review — setup_script takes an optional stages, and _setup_pass passes the tuple it already built.]
Nit, carried not fixed. os.environ.get(X, "").strip().lower() not in _FALSEY is now the 5th verbatim copy across four modules. Pre-existing; this PR extends it by one, and no test pins the vocabulary as shared.
Axis verdict: Approve
Spec
Every Done-when landed, each checked against the ticket's own wording.
"Every container devlaunch launches has a working zellij on PATH… no dependence on the user's dotfiles and no edit to any repo's devcontainer.json" — the axis ran the branch's real setup_script("revws") non-TTY in devcontainers/base:ubuntu: stage zellij ok, and a brand-new login shell resolved /home/vscode/.pixi/bin/zellij (0.44.3). Warm re-run 0.013s, profile line landed exactly once.
"Provisioning zellij must not be able to fail a launch… the wrap degrades to running the command directly" — verified structurally and live: with the hostname stage failing 1 the zellij stage still ran, and in a container with no zellij the wrapped payload printed sh: zellij: not found on stderr, put only AGENT_STDOUT in the redirect file, and exited 7. Both halves of the contract hold.
"An opt-in wrap, default off" — the switch reads os.environ.get(ZELLIJ_WRAP_VAR, "") against _FALSEY after .strip().lower(), so unset is off and a truthiness read would break it (confirmed by mutation).
Falsification condition, re-measured independently. The ticket said "Measure it and put the number in the PR; do not choose on taste." A number is in the PR, and the axis re-measured it on its own containers: warm 0.43s / 0.20s / 0.83s, cold 2.27s, cache 167MB — same order as the PR's 0.56/0.23/0.23 and 3.0s. The condition does not fire; the container-side pixi route is justified by measurement.
All three declared deviations upheld, each checked at the source. Not in REQUIRED_TOOLS: probe_script gates on _all_present(REQUIRED_TOOLS) and provision_tools returns True on a successful _transfer before reaching provision_script, so a zellij row really would make a gh+claude container re-lend every launch and still never install zellij. Beside rather than inside a pane: covered by the ticket's own "or equivalent", and verified live — the pane opened from a non-TTY command and exit 7 survived. restart rather than recreate: restart calls workspace_stop then workspace_up, which pays the setup pass.
Non-blocking. "A bare dl <workspace> is untouched, switched on or off" is stricter than the code: attach_workspace → dotfiles_update → workspace_ssh(command=…), so with the refresh opted in a bare attach does get the ensure prepended and the session created.
[Resolved during review — I reproduced the path independently before acting. The effect is benign, arguably the nicest arrival there is, but the claim was false as written: README and CHANGELOG now say the narrower true thing, and a new test pins the two switches composing rather than leaving it assumed. Verified red-able by neutralizing the wrap.]
Non-blocking. Done-when 1's "every container" is narrowed by DEVLAUNCH_NO_TOOLS — deliberate, argued in code, and documented in README; flagged only because the ticket's wording is unconditional.
Nit. The PR's mutation table claims each mutation killed its test "and no others"; of four re-run, three reddened extra tests. All in the safe direction, and the named test reddened in every case.
Axis verdict: Approve
Verdict
Approve at c18ff8a. Both axes approve independently, and the two things this PR could most easily have got wrong were checked by measurement rather than by reading: the route decision was re-measured on fresh containers by an axis that did not write it, and the "cannot fail a launch" guarantee was observed live in a container with no zellij, where the command still ran and returned its own exit status. The one factual overstatement — "a bare attach is untouched" — was real, and I reproduced it myself before narrowing the prose and pinning the true behaviour in a test.
Worth recording beyond this PR: the builder declined the ticket's own suggested mechanism (zellij as a third row in REQUIRED_TOOLS) and was right to, because that tuple also drives the probe and the lend; a row there would have made every already-equipped container re-lend ~300MB on every launch and still never install zellij. And zellij attach -b <name> exits 1 when the session already exists — the common path — so the || true is load-bearing rather than defensive.
Residues resolved on the branch; pixi run ci exit 0, 1821 passed, pylint 10.00/10. GitHub refuses the approve state on a same-account review, so this written verdict is what the gate reads.
1 participant
Closes #242
Every container
dllaunches now haszellijonPATH, and an opt-in switch makesdl <spec> -- <cmd>run beside a named zellij session, so the agent can open a terminal next to itself withzellij -s devlaunch action new-pane -- <cmd>.The route, and the number that decided it
Container-side pixi. The falsification condition does not fire.
A warm
pixi global install zellijinsidemcr.microsoft.com/devcontainers/base:ubuntu, against the shared package cache bound the way #240 binds it, over three fresh containers:gh/claude)The ticket's condition was "more than a few seconds". A fifth of a second is not that, so the host-side static-musl-binary mount — faster still, but a new upstream GitHub-release dependency plus a bind mount that only lands at container creation — stays the recorded alternative rather than the implementation.
Two things follow from the route that are worth a reviewer's attention:
devpod up, so an existing workspace picks zellij up on its nextdl <workspace> restart. The ticket's done-when said "recreate", which was written for the mount route; a recreate also works but is not needed. The README says so in those words.Where it is provisioned from, and why not the obvious place
The tempting change is a third row in
REQUIRED_TOOLS. That is a trap, and the tests pin it shut:REQUIRED_TOOLSis not just an install list —probe_scriptasks whether all of it is present, and a container answering "missing" is lent the host's ~300MB claude. zellij there would put every container that already holdsghand a realclaudeback onto the lending path on every launch, and would still never install zellij, because a successful lend returns before the network-install trip is reached.So zellij is provisioned as a stage of the setup pass instead. That is where "cost the feature, not the launch" already lives structurally: a stage's failure is contained by the
ifaround it, reported by name at its declared level, and cannot change the pass's exit status. The launch lock and_pixi_cache_up_argsare the models; this reuses the mechanism rather than restating the rule.zellij_script()is assembled entirely out of the piecesprovision_scriptis assembled from — the pixi bootstrap, the install line, the profile resolution, the hash-guarded prepend — so nothing about how a tool gets installed is written twice.The wrap, and the judgement call in it
DEVLAUNCH_ZELLIJ=1, default off. Off means no existing invocation changes meaning.The command runs beside the session, not inside a pane of it, and this is the one place I departed from the ticket's literal wording (
zellij attach -c <session> -- <cmd>— it says "or equivalent"). The reasoning:zellij -s <name> action new-paneworks from a command that is in no session at all. The capability needs the session to exist, not to contain the agent.dl <ws> -- cmd > filemust put the command's output in the file, andTestSessionExitStatusexists entirely to keep the remote program's own status intact through devpod's mangling.Both designs deliver the pane; only one breaks every scripted caller, including every wayfinder agent launch. So the payload is:
attach -b(create detached and return) rather than-c, since the command is not on a terminal. The|| trueis load-bearing, not defensive — measured: a secondzellij attach -b <name>exits 1 once the session exists, which is the case every launch after the first takes.;and not&&so the payload exits with the command's status. A container where zellij never installed swallows a 127 and runs the command exactly as before.A bare
dl <workspace>is untouched, switched on or off. It sends no--command— which is precisely what gets it a pty from devpod — so there is nothing to wrap, and giving it one would cost either the terminal or a ~1.7s round trip in front of every shell (#183's lesson). You land in a login shell withzellijonPATHandzellij attach -c devlaunchreaches the session by hand. This is stated as a test, not just as prose.One fixed session name rather than one per workspace: a zellij server lives inside a container and dies with it, so two workspaces cannot collide, and a constant is a name a human can type without looking it up.
Verification
Red first. All 23 new tests fail with
devlaunch/reverted tomain.Mutation-checked, because a false green cost this repo a round trip recently. Ten targeted mutations, each breaking exactly one decision, each killing its test and no others:
>&2on the stagetest_the_install_never_speaks_on_the_protocols_stdouttest_a_container_that_already_has_it_installs_nothingtest_a_cold_container_gets_it_and_the_next_shell_can_find_itDEVLAUNCH_NO_TOOLSgatetest_the_tools_opt_out_asks_for_no_zellijREQUIRED_TOOLStest_it_is_never_a_required_tool&&instead of;and no|| truetest_a_session_that_is_already_there_is_not_an_errortest_it_is_off_unless_switched_ontest_a_bare_attach_is_untouched_even_switched_ontest_a_switch_set_to_a_denial_is_still_offtest_a_stage_warns_by_name_unless_it_asks_to_be_quieterEnd to end in a real container, running the actual generated scripts non-TTY the way
devpod ssh --commandruns them:ok, stdout carried only marked protocol lines, install noise on stderr where_setup_passleaves it visible./home/vscode/.pixi/bin/zellij, 0.44.3).ok.provision_script's identical line).> fileredirect captured the command's output and nothing else.Local CI: exit 0, pylint 10.00/10, 1820 passed. Exit code captured directly, not through a pipeline.
Files changed
devlaunch/tools.py—ZELLIJ_TOOL,ZELLIJ_STAGE,zellij_script(), and the gated stage insetup_stagesdevlaunch/dl.py—ZELLIJ_WRAP_VAR,ZELLIJ_SESSION,zellij_wrap_enabled(),_with_zellij_session(), and the one-line change at the single wrap point inworkspace_sshtest/unit/test_tools.py—TestZellijProvisioning; the new writer registered inTestProfileGuards; three hostname-stage tests narrowed to name the stage they are about now that the pass runs twotest/test_dl.py—TestZellijSessionWraptest/test_devpod_spawn_counts.py—TestOptInZellijWrap: the feature adds no devpod round trip on any pathREADME.md,CHANGELOG.mdFor the reviewer to weigh
up, paying one failed pixi attempt each time. The same accepted residualprovision_toolsdocuments for the lend retry loop, and for the same reason: breaking the loop needs per-container retry state, which is more machinery than the case is worth.DEVLAUNCH_NO_TOOLSwhile the hostname stage deliberately does not — installing zellij is tool provisioning; naming a container is not.Summary by Sourcery
Add zellij provisioning to all devlaunch workspaces and introduce an opt-in mechanism to run workspace commands alongside a zellij session without changing existing behavior by default.
New Features:
Enhancements:
Documentation:
Tests: