An undecodable DEVLAUNCH_AID_AGENT is a name to refuse, not an unset variable - #413
Conversation
…variable `aid` read the variable with `std::env::var(..).ok()`, which reports a value that is not valid UTF-8 as *unset*. So `DEVLAUNCH_AID_AGENT=$'\xff'` did not name a broken agent, it named no agent at all: `default_agent` saw `None`, returned the default, and aid opened the workspace and started claude. The refusal `UnknownAgentInEnvironment` exists to give -- the one `DEVLAUNCH_AID_AGENT=nope` already got -- was unreachable for the only values that cannot be typed as a string. Same class as the `DEVLAUNCH_NO_TTY` divergence, milder consequence: a wrong agent rather than an inverted opt-out. The fix could not be local, which is why the finding was filed rather than fixed alongside that one. `aid` depends on `dl` and on nothing else of devlaunch, on purpose (`aid/Cargo.toml`, and the module doc at the top of `aid/src/main.rs`), so it cannot reach `osext` and cannot spell the reading correctly for itself either: the lossy decode is the whole of what makes it right. A copy of it in `aid` would be the second reading of one boundary that b52f4fa had just finished deleting. So `osext::env_str` becomes binary surface and `dl` re-exports it beside `shell` and `python_repr`, which is the door `aid` already reaches core's quoting through. `aid`'s dependency story is unchanged: the whole of devlaunch it can see is still dl's entry point and the three things dl hands it. One reader promoted, not the module opened. `strip`, `home_dir` and `temp_dir` were `pub` inside a `pub(crate) mod` and are now `pub(crate)`, so making the module public promotes exactly the one function with a caller outside the crate -- the same shape `json` already has, which is `pub` for `JsonKind` alone. Whether the rest of `osext` should follow is #406's open question, and it wants a second caller before it is answered rather than this one. The regression test spawns the binary with the byte in the environment, because that is the only place the bug lives: the predicate underneath was always correct, and a unit test of it is green before the change. `aid_with` is keyed by `OsStr` values now for the same reason -- the value that reproduces this is not a string. `public-api.rest.txt` gains the two rows by hand; CI's regeneration is authoritative if it disagrees. Refs #402, map #406.
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
Reviewer's GuideRoutes aid's agent environment lookup through the shared lossy OS-environment reader exposed by Sequence diagram for refusing an undecodable agent namesequenceDiagram
participant Env as OS environment
participant Aid as aid
participant DL as dl
participant Core as devlaunch_core::osext
participant Rewrite as rewrite
Env->>Aid: DEVLAUNCH_AID_AGENT=raw invalid UTF-8 bytes
Aid->>DL: env_str(AGENT_ENV_VAR)
DL->>Core: env_str(name)
Core-->>DL: Some(String with U+FFFD)
DL-->>Aid: Some(agent_name)
Aid->>Rewrite: parse_aid_args(argv, agent_name)
Rewrite-->>Aid: UnknownAgentInEnvironment
Aid-->>Env: Refusal - no workspace or agent launch
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
#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.
Codecov Report✅ All modified and coverable lines are covered by tests. 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:
|
#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.
blooop
left a comment
blooop
left a comment
There was a problem hiding this comment.
This was generated by AI during review.
Reviewed at merge-base 57955a3...136e00b. Everything below was run in a scratch worktree; cargo output is pasted where a claim rests on it.
Preflight: ci, rust, e2e, prek, public-api, packaging, coverage all green. review/gate are red only because no wf-review report exists yet — the job's own message says so ("The external reviewer refused ... and there is no self-review to stand in for it. Post a wf-review report on this pull request and re-run this job"). Not a code failure.
Standards
1. CONFIRMED — the module this PR makes public has three dead intra-doc links. osext's module doc links [strip], [home_dir] and [temp_dir]; all three are now pub(crate), so rustdoc drops the links and warns. This PR's own public-api job says it (18:00:30):
warning: public documentation for `osext` links to private item `strip`
--> devlaunch-core/src/osext.rs:14:9
warning: public documentation for `osext` links to private item `home_dir`
--> devlaunch-core/src/osext.rs:18:9
warning: public documentation for `osext` links to private item `temp_dir`
--> devlaunch-core/src/osext.rs:22:9
The same job on #414 and #415 emits none of them, so these are this diff's. Reproduced locally with cargo doc -p devlaunch-core --no-deps. Three of that doc's four bullets now render as plain text on the one module the PR promotes, and the doc is the first thing an external reader of devlaunch_core::osext lands on. timing already ships this warning shape, so it is not a new class — but it is a new instance, and a one-word edit each (drop the brackets, or link them as crate::osext::strip with --document-private-items off). Not blocking; worth the commit.
2. Design alternative, not blocking. The stated goal — "one reader promoted rather than the module opened" — is reachable more literally without opening the module: keep pub(crate) mod osext; and add pub use osext::env_str; at core's root. That exports exactly one item (one snapshot row, not two), leaves the module doc private so finding 1 disappears, and dl re-exports devlaunch_core::env_str unchanged from aid's side. The cost is losing osext:: from the path in the snapshot, which is a real readability loss — so this is a judgement call rather than a defect. Recording it because the PR body argues the narrow shape and this is the narrower one.
Checked and clean:
- The two hand-edited snapshot rows are right, and not on my reading of them. CI's
public-apijob regenerates all three snapshots with the pinnedcargo-public-api 0.52.0into a scratch tree anddiff -us them against the repo (scripts/public-api-snapshots.sh,ci.yml:434-443). It passed on136e00bhaving diffed three files, so the rows are byte-identical to a real regeneration. For what it is worth independently: sort position is right (notices<osext<runner), andpub mod+ one signature row matchesjson's convention atpublic-api.rest.txt:2258. - The demotion is not a surface break.
strip/home_dir/temp_dirwerepubinside apub(crate) mod, so unreachable outside the crate by construction.grep -rn "osext" rust/ --include=*.rsoutsidedevlaunch-core/src/returns nothing, and every caller is acrate::osext::path inside core. aid's dependency story is intact.aid/Cargo.tomlstill namesdlas its only devlaunch dependency andgrep -rn "devlaunch_core\|devlaunch-core" rust/aid/returns nothing.dl::env_strreaches core's reading the same wayshellandpython_reprdo.- Nit, pre-existing: the crate doc still describes the binary surface as "the four layer modules" while
json,shell,timing,noticesand nowosextare alsopub. Already drifted before this PR; one more instance.
Spec
Against #402.
- "a
DEVLAUNCH_AID_AGENTholding undecodable bytes silently selects the default agent instead of refusing withUnknownAgentInEnvironment" — fixed, and the test is genuinely red on the old code rather than green-either-way. Revertedrust/aid/src/main.rsalone to57955a3on top of this branch and ran the new test:
thread 'an_agent_name_that_does_not_decode_is_refused_rather_than_read_as_unset' panicked at aid/tests/rewrite.rs:151:9:
assertion `left == right` failed: expected exit 1; stdout: ; stderr: aid -> dl devlaunch-main-zovomobo -- '... claude --dangerously-skip-permissions hi'
Workspace devlaunch-main-zovomobo is already running, attaching...
left: Some(0)
right: Some(1)
That is the bug, from a spawned binary carrying a real OsStr::from_bytes(b"\xff") in its environment — Command::env takes AsRef<OsStr>, so the raw byte reaches the child unmangled and the non-UTF-8 path is the one under test. Unmodified branch: 12/12 green.
- "Either re-export the reader through
dl... or haverewritetake the already-decoded value" — the first option, which the ticket offers by name. - "
DEVLAUNCH_AID_AGENT=$'\xff'should refuse by name, not fall through to the default" — met; the refusal names the value as'\u{fffd}', andworld.devpod_calls().is_empty()pins that nothing opened. - "the general fix is to make
osextbinary surface" — deliberately deferred to #406 and said so. Consistent with the ticket, which files it as a "while there" rather than a requirement.
Nothing unverifiable on this one.
Verdict
Approve. No blocking findings. One confirmed defect worth a follow-up commit before merge: the three rustdoc::private_intra_doc_links warnings introduced in the newly-public osext module doc.
Making `osext` public turned its module doc into public documentation, and three
of its four bullets link items that stayed `pub(crate)`. rustdoc drops such a
link, renders the name as plain text, and warns:
warning: public documentation for `osext` links to private item `strip`
--> devlaunch-core/src/osext.rs:14:9
with `home_dir` and `temp_dir` behind it. Three warnings this diff introduced,
on the one module it promotes, in the first paragraphs an external reader of
`devlaunch_core::osext` lands on. The brackets are off now and the prose is
otherwise untouched, so the bullets read exactly as they did -- they just no
longer promise a link that was never going to render. A short note above them
says why only `env_str` is linked, so the next reader restores the brackets on
purpose or not at all.
The review offered a second shape: keep `pub(crate) mod osext` and put
`pub use osext::env_str;` at core's root, which exports one item instead of a
module and makes the module doc private again, dissolving all three at once.
Not taken, for three reasons.
The public path is the smallest of them: `devlaunch_core::env_str` loses the
`osext::` that says which boundary the reader belongs to, and the snapshot row
is where anyone reviewing the surface reads it.
The second is that the dead-link shape is the crate's, not this module's.
`cargo doc -p devlaunch-core --no-deps` emits 112 of these warnings; `timing`
ships five, `clients::devpod` eight, `clients::gh` more. Collapsing `osext`
would take three of 112 and leave the pattern standing everywhere else, so it
buys tidiness in one module rather than a class of warning gone.
The third is that `pub mod osext` is the seam #406 asks its question at --
whether the rest of the module should follow `env_str` out. A root re-export
answers "no" by construction and would have to be unpicked to answer "yes".
Cost against benefit: the alternative also rewrites `public-api.rest.txt` for a
shape `cargo-public-api` cannot be run against in this container (it needs
nightly), so the snapshot edit would be a guess pushed to CI to adjudicate.
That is a lot of motion for a cosmetic finding on an approved branch, and this
edit is four lines that need no snapshot at all.
`cargo doc -p devlaunch-core --no-deps` now names `osext` in no warning.
cargo test --workspace, cargo clippy --locked --all-targets -D warnings,
cargo fmt --check: all clean.
Refs #402, map #406.
|
Finding 1 fixed in 4f925e5. The brackets are off Took the one-word edit rather than your finding 2, and the deciding number was one I went looking for after reading it: that Two smaller weights on the same side. Your other two notes are recorded and untouched here: the crate doc still saying "the four layer modules", and the snapshot rows, which your regeneration argument already settles. Gate on the new head: |
This branch changes what aid does with a value the user set. An undecodable DEVLAUNCH_AID_AGENT used to read as unset, so the default agent started and the workspace opened with nothing said about the variable; it is refused by name now. That is user-visible, and the convention 14 of the last 20 merges follow is a line under [Unreleased]. [Unreleased] had no Fixed section on this branch, so the entry adds the heading. Markdown only -- nothing under rust/ is touched, and cargo fmt --check is clean.
|
Added the missing CHANGELOG entry (4e200b7), under a new It was owed because this branch changes what Markdown only -- nothing under |
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.
Conflict was CHANGELOG.md alone: main's aid-agent entry (#413) and this branch's two entries both land under [Unreleased]'s second '### Fixed'. Kept both.
Three resolutions, only one of which git flagged. CHANGELOG.md: textual conflict, kept both sides. lifecycle.rs:6986: no textual conflict, but the test main gained in #428 (a_record_removed_while_the_plan_sat_there_is_not_reported_as_re_pointed) still bound devpod_home as a bare PathBuf where the signatures this branch rewrites want &DevpodHome. Two E0308s in a tree git called clean. Now DevpodHome::at(..), matching its eight siblings. osext.rs: no textual conflict, but system_words was written pub while the module was pub(crate); #413 has since made osext a pub mod, so the merge would have added a public-surface row absent from both snapshots and reddened the public-api job. Demoted to pub(crate) alongside strip, home_dir and temp_dir, and its doc bullet unbracketed to match. It has no consumer outside the crate.
1 participant
Closes the
DEVLAUNCH_AID_AGENTfinding from the 2026-08-24 architecture review.The bug
aid/src/main.rsread the variable withand
osext's own module doc names why that is wrong:std::env::var(..).ok()reports a value that is not valid UTF-8 as unset. So
DEVLAUNCH_AID_AGENT=$'\xff'was not a broken agent name, it was no agent name —rewrite::default_agentsawNone, handed backclaude, and aid opened theworkspace and started the wrong agent. The
UnknownAgentInEnvironmentrefusalthat
DEVLAUNCH_AID_AGENT=nopealready gets was unreachable for exactly thevalues that cannot be written as a string.
Same class as the
DEVLAUNCH_NO_TTYdivergence fixed in b52f4fa, milderconsequence: a wrong agent rather than an inverted opt-out.
The constraint that shaped the fix
aiddepends ondland on no other devlaunch crate, deliberately — the rule isstated in
aid/Cargo.tomland in the module doc at the top ofaid/src/main.rs,and Python pinned it as an AST test. So
aidcannot reachosext, and it cannotspell the reading correctly for itself either: the lossy decode is the fix. A
copy of it inside
aidwould be a second reading of one boundary, which is thething b52f4fa had just finished deleting.
So the reader is promoted and re-exported, following the path
shellandpython_repralready take (dl/src/lib.rs):devlaunch_core::osextbecomespub, withenv_stras its one public item.strip,home_dirandtemp_dirwerepubinside apub(crate) modand arenow
pub(crate), so the module going public promotes exactly the one functionthat has a caller outside the crate — the same shape
jsonalready has, whichis
pubforJsonKindalone.dlre-exportsenv_str.aidcallsdl::env_str(rewrite::AGENT_ENV_VAR).aid's dependency story is unchanged: the whole of devlaunch it can see is stilldl's entry point and the three items dl hands it.
What this deliberately does not decide. #406 parks "should
osextbecomebinary surface wholesale" until a second caller asks for it. This promotes one
reader, not the module.
Red, then green
The test spawns the binary with the raw byte in its environment, because the
boundary is where the bug lives — the predicate underneath was always correct, so
a unit test of
default_agentis green before the change and proves nothing.aid_withis keyed byOsStrvalues now for the same reason: the value thatreproduces this cannot be written as a
&str.Before the change,
cargo test -p aid --test rewrite an_agent_name_that_does_not_decode:That is the bug itself in the failure output: exit 0, the default agent started,
the workspace attached. After the change the same test passes, and the run ends
with no devpod call made. The name is U+FFFD because the reading is a lossy
decode: the undecodable byte is present under the replacement character, which
is what makes the value a name to refuse rather than a variable to ignore.
Gate
cargo test --workspace— green (2 full runs; one intermediate run hitdl --test picker::what_is_typed_...on a pty echo race,"> wayfind"vs"> wayfinder". It passes alone 3/3 and the whole suite passes on the verynext run of this same code; same family as The aid pty tests flake on a loaded cargo test --workspace run #401, unrelated to this diff, which
touches nothing the picker reads.)
cargo clippy --locked --all-targets -- -D warnings— cleancargo fmt --check— cleanpublic_api_snapshots.rs— green.cargo-public-apineeds nightly and cannotbe installed here, so
public-api.rest.txtgains its two rows by hand andCI's regeneration is authoritative if it disagrees.
Refs #402, map #406.
Summary by Sourcery
Refuse undecodable
DEVLAUNCH_AID_AGENTvalues instead of treating them as unset and starting the default agent.Bug Fixes:
aidfrom launching the default agent whenDEVLAUNCH_AID_AGENTcontains undecodable bytes by refusing the value as an unknown agent.Enhancements:
dlso entry points consistently preserve present-but-non-UTF-8 values.Tests: