Conversation
`dl` gates its interactive prompt on the same variable the ssh transport is gated on, and the two disagreed about what it says. Core lowercases the value and runs it through `osext::strip` before comparing it against the falsey words; `dl` kept a copy of that predicate built from `std::env::var(..).ok()` and a bare `matches!` over the four literals. So `FALSE` and ` no ` were opt-outs to the prompt and falsey to the transport, and a non-UTF-8 value was *unset* to the prompt and present to the transport -- the opt-out-into-opt-in inversion `osext`'s own docs name as the reason `env_str` exists. The copy was not laziness. `osext` is `pub(crate)`, so `dl` could not reach the right reading from outside the crate, and neither half of what makes it right -- the lossy read or Python's `strip` -- can be spelled correctly without it. So the fix is a seam, not a patch: `ssh::tty_disabled_by_environment` is now binary surface, and `dl` asks it instead of answering for itself. Not the sharing `FALSEY`'s note argues against. That note is about two different hatches answering to one constant, where an edit meant for one moves the other. This is one hatch that had two readings. The deleted tests in `dl` only ever covered the spellings where the two happened to agree, which is why the divergence was invisible. Coverage moves to the predicate, where ` NO ` was already pinned, plus the cased and padded spellings the copy got wrong. The regression test is on a real pty: with DEVLAUNCH_NO_TTY=FALSE, aid used to skip the prompt while the transport kept the terminal. `public-api.rest.txt` gains the one new row by hand; CI's regeneration is authoritative if it disagrees.
Reviewer's GuideThe PR eliminates duplicated Sequence diagram for canonical DEVLAUNCH_NO_TTY readingsequenceDiagram
participant User
participant dl
participant SSH as ssh::tty_disabled_by_environment
participant Env as Process environment
User->>dl: interactive_terminal()
dl->>SSH: tty_disabled_by_environment()
SSH->>Env: osext::env_str(DISABLE_VAR)
Env-->>SSH: UTF-8-safe optional value
SSH->>SSH: tty_disabled(value)
SSH-->>dl: terminal disabled or enabled
dl-->>User: Prompt behavior matches SSH transport
Flow diagram for unified DEVLAUNCH_NO_TTY parsingflowchart LR
A[DEVLAUNCH_NO_TTY] --> B[osext::env_str]
B --> C[ssh::tty_disabled_by_environment]
C --> D[ssh transport]
C --> E[dl interactive_terminal]
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:
|
|
Interaction with #413, which is open beside this one. That PR makes The wrapper still earns its place, and Merge order does not matter; the two touch different rows of |
#413 makes osext::env_str reachable from the binaries, which retires the reason this doc gave for existing. The function still earns its keep, for a different reason worth writing down: dl asks for the decision, not the value, and composing it on dl's side would export tty_disabled and DISABLE_VAR to say what one item says -- putting the composition back on the side of the wall that got it wrong the first time.
|
Force-pushed That commit swept in The rewritten commit is the Nothing was lost and no review comment was outstanding on it, which is why this is a force-push rather than a revert commit. |
Fourteen of the last twenty merges carry an entry and this did not. The behaviour change is user-visible in both directions -- a falsey spelling that took the prompt away, and a non-UTF-8 value that took the pty away -- so it is the kind the file exists for.
blooop
left a comment
There was a problem hiding this comment.
This was generated by AI during review.
wf-review on #407 (fix_arch, head dad180f), map #406. Fixed point: git diff 9392779...dad180f. Run in a scratch worktree; every CONFIRMED item below was executed.
Preflight note: the review and gate jobs are red because the external reviewer refused on quota (sourcery-ai[bot]: "you have reached your weekly rate limit of 250000 diff characters"). Every other job is green, including public-api. This report is the stand-in.
Standards
S1 (informational) — the force-push is clean. git diff eb8a29d dad180f is exactly one deletion: rust/devlaunch-core/tests/devpod_layout.rs, 107 lines, a whole new file that was added on the branch and does not exist on origin/main. The current head's merge-base...HEAD is the four intended files and nothing else: aid/tests/interactive.rs +27, public-api.rest.txt +1, clients/ssh.rs +49, dl/src/lib.rs +6/−36. Nothing stray survived.
S2 (minor) — the red path is expensive in a file already known to flake. With the old logic restored the new test fails at interactive.rs:172 after 60.11 s — one expect timeout — and the test loops over two spellings, so a genuine regression can cost ~2 min per run. aid/tests/interactive.rs is in the #401/#416 flake set. The loop comment argues (correctly) for one World rather than two; the timeout is the other half of that cost and is not addressed. A shorter expect for the "banner should appear immediately" case would keep the signal and drop the tax.
S3 (informational) — [crate::osext::env_str] is a private intra-doc link from a pub fn. cargo doc -p devlaunch-core --no-deps warns rustdoc::private_intra_doc_links, but it emits dozens of the same warning across timing, devpod and others. House style, not a defect introduced here. (It is, however, the mechanical symptom of D1 below.)
S4 (minor) — Divergent Change is resolved; the remaining duplication is prose. The falsey-spelling argument is now written out in three places: the PR body's table, ssh.rs's tty_disabled_by_environment doc, the new core test's comment, and the new pty test's comment — four, counting both tests. That is a lot of surface for one three-word predicate to keep in step. Not blocking; noting it because D1 is exactly a case of one of those copies going stale.
Spec
No ticket closes this PR (Part of #406, and #406 records it as "The one defect already fixed — DEVLAUNCH_NO_TTY read two ways"), so the spec read is the PR body's own claims plus #406's line. Findings quote the body.
Confirmed.
-
The pty test is genuinely red on the old logic — verified, not taken on trust. I restored
dl::interactive_terminal's deleted copy (std::env::var("DEVLAUNCH_NO_TTY").ok()+ barematches!) and reran:thread 'a_falsey_no_tty_leaves_the_terminal_alone_on_a_real_pty' panicked at aid/tests/interactive.rs:172:9: "press Enter" never appeared; the pty said: aid -> dl devlaunch-main-zovomobo -- 'CLAUDE_CODE_DISABLE_TERMINAL_TITLE=1 IS_SANDBOX=1 claude --dangerously-skip-permissions'aid goes straight to the launch with no prompt — the bug itself in the output. It does not pass for the wrong reason on this bug. See C1 for the direction it does not cover.
-
public-api.rest.txt's hand-added row is verified by regeneration, not just by the snapshot test. Thepublic-apijob installs the pinned nightlycargo-public-api, regenerates every snapshot anddiff -us it against the committed file (.github/workflows/ci.yml:419+), and it is SUCCESS on this head. So the body's "CI's regeneration is authoritative if the row is off" has been exercised and the row is byte-exact in text and position — after theUnsafeRequestimpl block, beforepub mod devlaunch_core::domain, matching how its neighbours sort. -
The arithmetic half of the wrapper's justification holds.
tty_disabled(ssh.rs:150) andDISABLE_VAR(ssh.rs:50) are bothpub(crate), so composing the decision indlwould need three exported items where one does the job.
C1 (blocking) — the deleted dl coverage is not fully replaced, and the gap survives a mutation the old tests would have killed.
The deleted dl tests were two, not one. unset_and_falsey_values_keep_the_terminal is replaced: core's a_falsey_opt_out_still_allows_a_terminal already covered None, "", "0", "false", "no", and the new a_falsey_word_is_falsey_however_it_is_cased_and_padded adds the cased and padded spellings. Good.
any_other_value_is_a_request_for_no_terminal — which pinned "1" | "true" | "yes" | "anything" as disabling — is not replaced above the pure predicate. All that survives is assert!(tty_disabled(Some("1"))) inside the_opt_out_forces_the_devpod_transport_whatever_the_terminal_says, on the function that was never the broken half.
Proof this is a real hole and not bookkeeping. I replaced the body of the new tty_disabled_by_environment with false — i.e. DEVLAUNCH_NO_TTY=1 stops opting out anywhere in dl, killing the documented escape hatch (README ×4, aid --help, two strings in dl/src/render.rs) — and ran:
cargo test -p aid → 40 / 8 / 1 / 11 passed, 0 failed
cargo test -p dl -p devlaunch-core → all suites passed, 0 failed
The whole workspace stays green. Before this PR that mutation died on dl's own any_other_value_is_a_request_for_no_terminal. So the PR removes the only test in the tree that constrains the truthy direction of the reading dl actually performs, and the one line of genuinely new code — the wrapper — is the line that documents itself "Impure and therefore untested." It is untested in both directions; the pty test happens to cover the falsey one end-to-end.
The fix is one loop entry: a truthy value in the new pty test, asserting the banner does not appear and aid launches straight through. That is the same shape as the existing case and costs one more World-free spawn.
D1 (blocking, documentation) — the amended doc comment asserts a state this branch is not in.
clients/ssh.rs:176:
"It stays a function here now that [
crate::osext::env_str] is reachable from the binaries…"
At dad180f:
devlaunch-core/src/lib.rs:81readspub(crate) mod osext;dl/src/lib.rsre-exports noenv_str(grep -n env_str dl/src/lib.rs→ no hits)public-api.rest.txthas noosextrow (grep -n osext→ no hits)origin/mainalso carriespub(crate) mod osext;- #413, the PR that would make it true, is still OPEN (
state: OPEN, mergedAt: null)
So env_str is not reachable from the binaries, here or on main. The commit that introduced this line — dad180f "The one-reading wrapper is arithmetic now, not a locked door" — replaced a justification that was true with one that presupposes an unmerged PR, and in doing so put the comment in direct contradiction with this PR's own body, which still says:
"The copy was not laziness —
osextispub(crate), sodlcould not reach the right reading."
Both statements are in the same PR and cannot both be right. Either re-word to the conditional the situation actually supports ("stays a function even once #413 exposes env_str, because what dl asks is the decision, not the value") or land this after #413. The reasoning the comment gives is sound; only its tense is wrong, and a reader landing on it today would conclude the wrapper is redundant when it is currently load-bearing.
This also explains S3: the private-link warning is rustdoc telling you the item you linked is not, in fact, public.
Verdict
Request changes.
Blocking:
- C1 — the truthy direction of
DEVLAUNCH_NO_TTYis now unpinned above the pure predicate. Mutatingtty_disabled_by_environmenttofalseleavesaid,dlanddevlaunch-corefully green; the deleteddltest would have caught it. One extra case in the new pty test closes it. - D1 —
ssh.rs:176claimsosext::env_stris reachable from the binaries. It is not, on this branch or on main; #413 is still open, and the claim contradicts this PR's own body.
Non-blocking: S2, S3, S4.
Nothing else is wrong with this change. The force-push is clean, the fix is the right seam, the pty test is honestly red-then-green, and the hand-added public-api row has been checked by the job that regenerates it rather than by the test that reads the same file.
Review of #407 found the hole by mutation: replace `tty_disabled_by_environment`'s whole body with `false` -- killing DEVLAUNCH_NO_TTY=1 for every dl and aid there is -- and `-p aid`, `-p dl` and `-p devlaunch-core` all stay green. Deleting dl's `any_other_value_is_a_request_for_no_terminal` took the only test constraining that direction, and the claim that coverage merely "moves to the predicate" was wrong: the predicate is tested, but nothing said the wrapper consults it. The pty test now carries the truthy case, asserted by what a skipped prompt does rather than by waiting out a banner that never comes -- an absence assertion here would cost the 60-second deadline on the passing path, in a file already in the #401 flake set. Verified against the same mutant: red with it, green without. Also corrects a doc claim that described another open branch's state as this one's. `osext` is `pub(crate)` on this head and on main; #413 proposes exposing `env_str` and is unmerged. The wrapper's justification never needed that to be true, so it now says so without depending on it.
clippy -D warnings, which I ran on every agent's branch and not on my own.
dlgates its interactive prompt on the same variable the ssh transport is gated on, and the two disagreed about what it says.Core lowercases the value and runs it through
osext::stripbefore comparing it against the falsey words (clients/ssh.rs:153).dlkept a copy built fromstd::env::var(..).ok()and a barematches!over the four literals. Three divergences followed:FALSEto_lowercase" no "osext::stripvar().ok()The third is the inversion
osext.rs:11-13exists to prevent, in as many words: "std::env::var(..).ok()reports a non-UTF-8 value as unset, which turns an opt-out into an opt-in."The copy was not laziness —
osextispub(crate), sodlcould not reach the right reading, and neither half of what makes it right can be spelled without it. So the fix is a seam:ssh::tty_disabled_by_environmentis now binary surface anddlasks it instead of answering for itself.Not the sharing
FALSEY's note argues against. That note is about two different hatches answering to one constant, where an edit meant for one silently moves the other. This is one hatch that had two readings.Tests
The deleted
dltests only ever covered the spellings where the two happened to agree, which is why the divergence was invisible — and their doc claimed to test "the readinginteractive_terminalshares with the ssh transport", a sharing that did not exist.Coverage moves to the predicate, where
NOwas already pinned, plus the cased and padded spellings the copy got wrong. The regression test is on a real pty (aid/tests/interactive.rs): withDEVLAUNCH_NO_TTY=FALSE, aid used to skip the prompt while the transport kept the terminal. Verified red before green.Note for review
public-api.rest.txtgains its one new row by hand —cargo-public-apineeds nightly, which the devcontainer does not carry. CI's regeneration is authoritative if the row is off.Part of Close out the 2026-08-24 architecture review.
Summary by Sourcery
Unify
DEVLAUNCH_NO_TTYhandling so prompts and SSH transport consistently agree on whether terminal behavior is disabled.Bug Fixes:
DEVLAUNCH_NO_TTYinterpretation, including case-insensitive, padded, and non-UTF-8 values.Enhancements:
dlimplementation.Documentation:
DEVLAUNCH_NO_TTYbehavior in the changelog.Tests:
Chores: