Gate the zellij stage on DEVLAUNCH_ZELLIJ; retire DEVLAUNCH_NO_ZELLIJ - #425
Conversation
DEVLAUNCH_ZELLIJ is now the one signal that says a launch wants zellij: it decides whether the setup pass carries the zellij stage as well as whether a dl <spec> -- <cmd> makes sure a session exists first. DEVLAUNCH_NO_ZELLIJ is retired and read by nothing. The seconds are why it was worth looking at, not what decided it. The install was opt-out while every use of it was opt-in, so the default combination spent 2.2s to 3.5s of every cold launch provisioning a capability the same defaults guaranteed nothing would touch. Four states, two of them coherent; one variable collapses the table to those two. ZellijSwitch::requested is the only function that turns a value into the switch, and ZellijWrap::from_host is derived from it, so the stage and the wrap cannot come to disagree about one variable. Closes the build of #391.
Reviewer's GuideThe PR makes zellij consistently opt-in: Sequence diagram for opt-in zellij launch provisioning and wrappingsequenceDiagram
participant User
participant Launch
participant Cache as VerdictCache
participant Setup as SetupPass
participant Wrap as ZellijWrap
participant Container
User->>Launch: dl <spec> -- <command>
Launch->>ZellijSwitch: requested(DEVLAUNCH_ZELLIJ)
ZellijSwitch-->>Launch: Install or Skip
Launch->>Cache: Check switches
alt DEVLAUNCH_ZELLIJ is set and cache differs
Launch->>Setup: setup_stages(..., ZellijSwitch::Install)
Setup->>Container: Run ZELLIJ_STAGE
Container-->>Setup: zellij installed
Setup->>Cache: Record switches
else DEVLAUNCH_ZELLIJ unset
Launch->>Setup: setup_stages(..., ZellijSwitch::Skip)
Setup-->>Launch: No zellij stage
end
alt ZellijSwitch::Install
Launch->>Wrap: from_host(host)
Wrap-->>Launch: Beside
Launch->>Container: Start zellij session
else ZellijSwitch::Skip
Launch->>Wrap: from_host(host)
Wrap-->>Launch: Off
end
Launch->>Container: Run command
Flow diagram for zellij switch and provisioning gatesflowchart TD
A[DEVLAUNCH_ZELLIJ value] --> B[provisioning_disabled parse]
B --> C{ZellijSwitch.requested}
C -->|set truthy| D[Install]
C -->|unset or falsey| E[Skip]
D --> F{ToolsSwitch allows provisioning}
F -->|yes| G[Run ZELLIJ_STAGE]
F -->|DEVLAUNCH_NO_TOOLS=1| H[Skip stage]
E --> H
D --> I[ZellijWrap::from_host]
I --> J[Beside: create session]
E --> K[Off: no session wrap]
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
Codecov Report❌ Patch coverage is
Additional details and impacted files
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
blooop
left a comment
There was a problem hiding this comment.
This was generated by AI during review. Fresh context: I did not see this code
written and read nothing about it but the diff, the tree and #391/#404.
Reviewed at 68064d3 against origin/main (57955a3), which it is a fast-forward on.
Axis 1 — correctness
No defect found in the Rust. The four things most worth breaking, each checked
directly rather than through a test name:
A stale DEVLAUNCH_NO_ZELLIJ=1 is inert, and the proof is stronger than the guard.
strings rust/target/release/dl | grep -o 'DEVLAUNCH_[A-Z_]*' | sort -u returns twelve
names and DEVLAUNCH_NO_ZELLIJ is not among them. The name is absent from the shipped
binary, so it cannot be read at all, let alone read as a consent. The readers are also
enumerable by hand and there are exactly two — Host::from_process (launch.rs:201)
and ZellijSwitch::from_env (provision.rs:383) — both naming ZELLIJ_VAR.
The two readers really cannot drift about that one variable. ZellijWrap::from_host
(launch.rs:1245) is now a match over ZellijSwitch::requested, and launch.rs's own
switched_on is no longer on the zellij path at all — one parse, one polarity, derived
rather than duplicated. the_wrap_and_the_stage_read_one_signal is close to tautological
by construction, which is the correct shape for this guard: it fails exactly when
from_host stops delegating, which is the drift.
The and with DEVLAUNCH_NO_TOOLS=1 holds structurally, not by comment.
provision.rs:1028 is if let (ToolsSwitch::Install, ZellijSwitch::Install) = (tools, zellij).
ToolsSwitch::Skip drops the stage whatever the zellij switch says. Nothing else builds
stages: setup_pass is the sole non-test caller (provision.rs:1961).
Turning it on later works, and by a cheaper route than the docs claim — see finding 2.
Boundaries and the reverse direction. requested(None) == Skip; every FALSEY value
reads Skip; anything else reads Install. Turning the variable back off costs one
untrusted top-up and then re-stabilises, because record rewrites MarkerSwitches
(verdict_cache.rs:215). A marker from an older build still fails to deserialise and is
untrusted, which is the harmless direction.
Axis 2 — repo standards and guards
All green (see "what I ran"). Neither known flake appeared, so there is nothing to
attribute. cargo public-api snapshots are unchanged, correctly: ZELLIJ_DISABLE_VAR
was pub(crate).
The three new prose guards are anchored to headings that exist exactly once, and
section() asserts that uniqueness rather than returning an empty string — so a rename
fails loudly. The cost-table guard is asserted from zellij_script() containing
PIXI_BOOTSTRAP, which it does (provision.rs:724), so the table's dominant term is
held against the script rather than against a number. That is the right seam.
What the guards do not cover is the two sentences below, which is where the findings are.
1. docs/workspace-tools.md:345 states the value grammar backwards
Both variables read the same values: anything but empty,
0,falseornocounts
as unset.
provisioning_disabled(Some("1")) is !FALSEY.contains("1") → true →
ZellijSwitch::Install. So DEVLAUNCH_ZELLIJ=1 counts as set, and README.md:355
says so in the same words with the right ending ("counts as set"). As written, the page
tells a reader that the only documented way to switch the feature on reads as unset —
i.e. that the feature cannot be switched on.
The sentence's own tail is right ("=0 … which is what each of them reads as unset"),
which is what makes this look mechanical: the old ending "means yes, turn it off" was
replaced with "counts as unset" and the leading "anything but" was left in. One-word fix.
No guard sees this sentence; the three new ones read the opening, the switch row and the
cost table only.
2. docs/workspace-tools.md:295 and CHANGELOG.md:28 require a restart that is not required
setting the variable on a workspace that is already up does nothing until its next
dl <workspace> restart
dl <ws> up is enough, and this PR's own new test demonstrates it:
a_launch_that_asks_for_zellij_carries_the_stage_and_one_that_does_not_pays_nothing
(rust/dl/tests/launch.rs:734) uses World::with(&["--warm"]) — a workspace already
running, per up_on_a_running_workspace_says_so_and_still_provisions_the_tools — runs
dl MAIN up with DEVLAUNCH_ZELLIJ=1, and asserts the zellij stage is reported.
The mechanism is the one the changelog paragraph describes and then misroutes:
run_up_verb tops up with PassOccasion::TopUp (launch.rs:2833), provision() returns
CachedProvisioned only when verdicts.trusted (provision.rs:1837), and the marker's
MarkerSwitches disagree, so the pass travels carrying the stage. A bare dl <ws> on a
running workspace really does skip it — run_attach's fast-attach arm (launch.rs:2723)
calls provision_tools not at all — so the neighbouring sentence about attaching is
correct. It is only restart that is over-prescribed.
Why it matters rather than being a nicety: restart stops and restarts the container,
killing whatever is running inside it. Pointing a reader at that to pick up a package
they could have had from the prewarm verb is a real cost, and the changelog repeats it.
3. docs/workspace-tools.md:269 overstates what the retirement removed
Asking for the capability now gets you all of it.
DEVLAUNCH_NO_TOOLS=1 DEVLAUNCH_ZELLIJ=1 still produces the incoherent pair the
subsection says was deleted: setup_stages drops the stage (provision.rs:1028) while
ZellijWrap::from_host reads only host.zellij and still returns Beside, so
dl <ws> -- cmd runs zellij attach -b devlaunch … || true; cmd in a container with no
zellij — "install nothing, then start a session in it", verbatim.
I tried to refute this and got half way: the behaviour is correct and soft (the attach is
|| true, the command runs), and the page does say NO_TOOLS overrides at lines 204 and
339, and the "It can never fail a launch" paragraph covers the outcome. So the code is
fine and the claim is only local prose. But the new guard cannot see it either:
the_wrap_and_the_stage_read_one_signal passes ToolsSwitch::Install for all eleven
values, so the one remaining way for the wrap and the stage to disagree is the one case it
does not walk. Worth one clause of qualification in the sentence, and worth a
ToolsSwitch::Skip row in that loop if the claim is meant to be structural.
4. §3 — provisioning_disabled is a bool now read at two opposite polarities
provision.rs:312. ZellijSwitch::requested reads
if provisioning_disabled(value) { Self::Install } — literally "if provisioning is
disabled, install". The eight-line doc comment apologising for that is the tell; prose
explaining what a value means is an invariant the type was supposed to carry.
Transformation: have the shared parse return a polarity-free sum and let each switch
interpret it.
enum SwitchValue { Set, Unset }
fn switch_value(value: Option<&str>) -> SwitchValue
// ToolsSwitch: Set => Skip, Unset => Install
// ZellijSwitch: Set => Install, Unset => SkipWhat it removes: the apology, and the one line in the diff where a well-meaning "this
reads backwards" fix silently restores the old default for every user — the exact
regression this PR is most exposed to. Blast radius: two call sites in provision.rs;
gh::forwarding_disabled and ssh::tty_disabled keep their own copies and are untouched.
Refutation, stated because it is most of the answer: the truth table is pinned by
the_zellij_stage_is_off_unless_the_launch_asks_for_it, so that flip fails a test. This is
a modelling finding, not a defect, and reasonable to decline.
5. §4 — a new comment claims a coupling the code does not have
launch.rs:4315:
The values walked here are the ones
provision::tests::the_wrap_and_the_stage_read_one_signal
asks the setup pass about, so the list is the shared one
They are three independent literals and they already differ:
launch.rs:4319 walks ["", "0", "false", "no", "FALSE", " no "]; provision.rs:4752
walks ["", "0", "false", "no", "NO", " no "]; the drift test at provision.rs:4779 walks
neither exactly (it has FALSE but not NO, beside but not anything). Adding a value
to one adds it to nothing else. The drift test is the one that matters and it does cover
the union of the interesting cases, so the assertion is fine — the sentence is what has
already gone stale, on the day it was written.
Nit, not a finding: zellij_contract.rs asserts row.contains("Install")
case-sensitively while its sibling checks lowercase first. A reworded row opening
"install zellij into…" fails a guard whose message says the row does not mention
installing.
What I ran
Worktree at 68064d3, pixi's pinned toolchain on PATH:
cargo test --workspace— exit 0. 1258devlaunch-coreunit tests, 53dl/tests/launch.rs,
and every other suite green; 0 failed anywhere. Neither
a_contended_up_of_a_running_workspace_runs_no_up_at_allnor
a_pasted_multi_line_prompt_arrives_whole_rather_than_leakingfailed, so no flake to
attribute either way.cargo clippy --locked --all-targets -- -D warnings— clean, no output.cargo fmt --check— clean.cargo build --release -p dl -p aidthenpytest test/ -q— 410 passed, 25 deselected.strings rust/target/release/dl | grep -o 'DEVLAUNCH_[A-Z_]*' | sort -u— the inertness
proof in Axis 1.grep -rn NO_ZELLIJover the whole tree — three prose sites (CHANGELOG.md,
docs/workspace-tools.md,provision.rsdoc comments and one assertion message), no reader.- Read
setup_stages,provision,setup_pass,VerdictCache::{trusted,record},
run_attach,run_up_verb,ZellijWrap::from_host,section/bullets/bolded_spans,
and the heading list ofdocs/workspace-tools.md.
Verdict
Comment. No blocking correctness defect: the default moves the right way, the retired
variable is provably unreadable, and the and with DEVLAUNCH_NO_TOOLS is structural.
Findings 1 and 2 should be fixed before merge. Both are wrong sentences in the
user-facing page this PR exists to change, both are cheap, and finding 2 is contradicted
by a test in the same commit. Finding 3 is a clause. Findings 4 and 5 are fair to decline
with a reply.
Four prose findings from review, and a guard for the one that a guard can hold.
The value grammar had its polarity reversed: "anything but empty, `0`, `false`
or `no` counts as unset", where `provisioning_disabled(Some("1"))` is true and
yields `ZellijSwitch::Install`. README says "counts as set" in the same words,
so the two pages disagreed and this one told a reader that `DEVLAUNCH_ZELLIJ=1`
reads as not asking. `the_value_grammar_calls_a_bare_1_a_consent` now holds the
sentence against `requested`, anchored on the denial list rather than on the
phrasing, so the next reversal fails a test instead of shipping.
`restart` was over-prescribed. `dl <ws> up` tops up a running workspace and the
marker's recorded switches make the old verdict untrusted, so the pass travels
carrying the stage: this PR's own `launch.rs:734` proves it against a `--warm`
world. `restart` stops the container and kills what is in it, which is a real
cost to point somebody at for a package the prewarm verb would have installed.
Fixed in the page and in the changelog.
"Asking for the capability now gets you all of it" overstated the retirement:
`DEVLAUNCH_NO_TOOLS=1 DEVLAUNCH_ZELLIJ=1` still drops the stage while the wrap
still returns `Beside`, which is the incoherent pair by the one route left. The
code is fine and fails soft; the sentence now says so, and
`the_tools_opt_out_asks_for_no_zellij` pins the wrap side of it. Not widened
into `the_wrap_and_the_stage_read_one_signal`: that guard is about one variable
having one reader, and a `ToolsSwitch::Skip` row would make its `assert_eq` a
different assertion.
The comment claiming a shared value list is true now rather than reworded.
`ZELLIJ_DENIALS` and `ZELLIJ_CONSENTS` are one `#[cfg(test)]` literal that both
tests walk, so a value added to one is added to both.
|
Addressed at 1. The value grammar stated backwards. Fixed, and guarded. Confirmed: A guard belongs here, and it is 2. Confirmed the whole chain: The page's "Existing workspaces" now leads with 3. "Asking for the capability now gets you all of it". Qualified, and pinned. Widening the drift guard declined, with a narrower guard instead. Confirmed: I did not widen 4. A polarity-free The finding is fair and the refutation you wrote is most of my answer: the truth table is pinned twice over, by 5. The comment claiming a shared list. Made true rather than reworded. You are right that they were three independent literals. The two the comment named happened to agree today, which is the worst state for a claim like that to be in. Nit: case-sensitive What I ran (worktree at
|
Two prose conflicts, both resolved by taking both halves. CHANGELOG.md: main cut [0.14.0], so this branch's '### Changed' sits above main's '### Fixed' under [Unreleased]. docs/workspace-tools.md: this branch's 'installs a package into a container and starts a session' with main's 'two lines into a profile'. Every Rust file auto-merged. Also corrected two claims the inversion makes load-bearing. 'A bare dl <ws> against a running workspace attaches without a pass, which is the one case that picks nothing up' is wrong, in docs and CHANGELOG both. run_attach on a running workspace goes straight to self.attach(id, verb.command()), and verb.command() carries the '-- <cmd>', so dl <ws> -- <cmd> runs no pass either. That is the invocation zellij exists for: opting in and immediately running a command against a container already up wraps it in a zellij that was never installed, and the wrap fails quietly. Now says an attach of any shape, and says to bring it up once first. README's 'Every variable here reads the same values' was true of the table's 'no' rows and is not true of the table. Three of its nine rows are not switches: DEVLAUNCH_AID_AGENT and DEVPOD_SSH_CONFIG take a value, and timing::Mode::requested counts only empty and 0 as off, so DEVLAUNCH_TIMING=false turns timing on. Scoped to switches and the three named.
Two conflicts, both from #425 making zellij opt-in: - `up_on_a_running_workspace_says_so_and_still_provisions_the_tools` lists the stages that report. main removed the zellij line because the stage no longer runs unless asked; this branch added the title line because that stage now runs for every arm. Both, not either. - The CHANGELOG, mechanically: main's zellij entries sat under Unreleased, and Unreleased is what 0.15.0 is cutting, so they belong in its `### Changed` beside the id move.
blooop/devlaunch#425 made the zellij stage opt-in: skip is the default now and DEVLAUNCH_ZELLIJ=1 is the only thing that asks for the install, so this export is read by nothing from 0.15.0 on. Kept rather than deleted, because dl on this host is 0.14.0, which still reads it and would otherwise put zellij back into every workspace on create. The comment and both README passages now carry the end date and say to drop the line once this host is on 0.15.0, and that asking for zellij again is DEVLAUNCH_ZELLIJ=1 rather than the absence of it. The README had said 'devlaunch reads the variable from 0.4.0' and 'devlaunch 0.4.0+ honors DEVLAUNCH_NO_ZELLIJ' with no end date, both of which stop being true at 0.15.0.
Builds the decision in #391; the slice is #404.
The zellij stage stops being unconditional.
DEVLAUNCH_ZELLIJis now the onesignal that says a launch wants zellij: it gates the setup pass's zellij stage
as well as the session wrap it already gated.
DEVLAUNCH_NO_ZELLIJis retiredand read by nothing.
The seconds are why this was worth looking at, not what decided it. The install
was opt-out while every use of it was opt-in, so the default combination spent
2.2s to 3.5s of every cold launch provisioning a capability the same defaults
guaranteed nothing would touch. Four states, two of them coherent; one variable
collapses the table to those two. 1.70s of the stage is bootstrapping pixi, and
on the lend path that stage is the only thing that puts pixi in a container at
all.
What changed
ZellijSwitch::requestedreadsDEVLAUNCH_ZELLIJwith the sense inverted,through the same
provisioning_disabledparseToolsSwitchuses. Thepolarity differs because the variables do; the parse is shared so
=0cannotmean one thing in one and something else in the other.
ZELLIJ_DISABLE_VARis gone, replaced by
ZELLIJ_VARinprovision.rs, whichlaunch.rsre-exports rather than defining a second constant for.
ZellijWrap::from_hostis derived fromZellijSwitch::requestedratherthan parsing the variable a second time. That is the Three-state tools probe: only the official claude layout counts as provisioned #151 shape taken to its
end: one signal, one parse, so the two readers cannot disagree.
setup_stages' gate is unchanged code. TheandwithToolsSwitchstillholds, so
DEVLAUNCH_NO_TOOLS=1overrides a launch that did ask.transfer_scriptis touched. Nothing inverdict_cache.rschanges but prose: the marker already records whichswitches a pass ran under, which is exactly what makes setting the variable on
a running workspace land the stage on its next
restart.Tests
the_zellij_stage_is_off_unless_the_launch_asks_for_it— the new default, andthat both spellings of the shared
FALSEYlist still answer the same way.the_wrap_and_the_stage_read_one_signal— for every value the wrap's owndenial/consent list walks,
ZellijWrap::from_hostand whethersetup_stagescarries
ZELLIJ_STAGEagree. Verified to fail when the two readers are givendivergent parses.
the_retired_opt_out_is_read_nowhere_at_all— reads every.rsfile in theworkspace and asserts exactly one zellij environment-variable string
literal. The switches take their values as parameters, so no test can hand a
process environment to a reader; what makes a stale
DEVLAUNCH_NO_ZELLIJ=1inert is that nothing names it, and that is a property of the source. This is
the one way the retirement could bite silently: a stale opt-out turning
provisioning on for the people who asked for it off.
a_launch_that_did_not_ask_for_zellij_carries_no_zellij_stage/a_container_a_launch_asked_zellij_for_gets_the_stage— the inverted pair,with the hostname and title stages untouched either way.
a_launch_that_asks_for_zellij_carries_the_stage_and_one_that_does_not_pays_nothingin
dl/tests/launch.rs— the same verb on the same workspace, told apart onlyby the variable, from outside the binary.
flows::provision::zellij_contract, besidelending_contractand readingits section splitter rather than writing a second one: the guarantee
sentence's polarity (asserted on the opening sentence, so a paragraph three
below explaining the ask cannot satisfy it), the switch row naming both things
the variable now does, and that the cost table carries the pixi-bootstrap term
— asserted from
zellij_scriptreally containingPIXI_BOOTSTRAP, not fromthe numbers. Both prose guards verified to fail against the old wording.
Docs
docs/workspace-tools.md: the guarantee sentence, the one-switch subsection,"Existing workspaces" (now explaining the verdict-cache mechanism that makes the
opt-in work), and the cost table, which now reports the 1.70s bootstrap it had
been omitting alongside the ~0.5s install. README's bullet and env-var table.
CHANGELOG entries for the default change and the retired variable.
Note on the ticket's pointer: the zellij cost figures live in
docs/workspace-tools.md, notdocs/performance.md— that page carries nozellij numbers, so nothing moved and no guard was repointed.
🤖 Generated with Claude Code
Summary by Sourcery
Make zellij provisioning and session setup opt-in through DEVLAUNCH_ZELLIJ, retire DEVLAUNCH_NO_ZELLIJ, and align documentation and validation with the new behavior.
Bug Fixes:
Enhancements:
Documentation:
Tests: