Conversation
A promptless aid <workspace> on a terminal now spawns a background 'aid --boot-up … up' child (dl's own up verb, serialized by the existing per-workspace launch lock) and reads the agent's prompt from the terminal while the container comes up — typed free of shell quoting, submitted with Enter, injected through the same rewrite an argv prompt takes. Empty Enter or Ctrl-D is the plain session a bare aid always started; a piped stdin or DEVLAUNCH_NO_TTY=1 keeps the one-shot behaviour untouched. The boot's output is parked in a log and replayed after the Enter, and Ctrl-C at the editor tears the whole boot down through the shared interrupt disposition. Rider: aid now forwards --update-cache verbatim to dl — dl re-spawns its detached completion refresh through current_exe, which under aid is aid, and that line used to die as 'aid needs a workspace' on every launch.
The submission's first loop consumes the Enter that ends line one without keeping it, so a paste of 'fix this\nand then that' reached the agent as 'fix thisand then that' — two lines glued into one word. The Enter is now put back whenever the drain finds queued lines behind it. The same comment also claimed a final unterminated fragment is drained; in cooked mode only completed lines are readable, so the fragment stays queued and reaches the agent as type-ahead — the comment now says what actually happens. Pinned by the new pty test, which is also the drain's first end-to-end coverage.
tests/interactive.rs polls devpod_calls() every 25ms while the shim is still appending — the overlap test watches for the up mid-edit — and the shim's append is a buffered write, so a poll can read a torn final line. The parser skipped nothing and panicked on it. Torn lines are now skipped: they are whole on the next poll, and nothing is still writing by the time the post-exit assertions read the log. A race in test infrastructure, so there is no deterministic failing test for it; the hardening is the fix.
BootChild::spawn creates the log before spawning; when the spawn itself failed (unresolvable current_exe, exec error) the fallback to the serial launch left the just-created file in the temp directory. The error path now removes it. Not deterministically testable — forcing spawn to fail needs a broken current_exe — and the fix is the two remove_file calls.
A minor bump, by the rule the release history states: purely additive releases take a patch, and this one changes what an existing command line does. A promptless `aid <workspace>` on a terminal now boots the workspace in the background and asks for the prompt while it does, instead of attaching straight into the agent's plain session — the old behaviour is one empty Enter away, and a piped stdin or DEVLAUNCH_NO_TTY=1 gets it unchanged, but the default moved and the version says so. Also in this release: the paste drain (a multi-line prompt arrives whole), and aid forwarding --update-cache to dl so the completion refresh an aid launch fires stops dying on arrival. dl's public surface grows interactive_terminal() and read_terminal_submission(); devlaunch-core's public surface is untouched, so the public-api snapshot does not move. Also carries the rust/Cargo.lock entries for the five workspace crates. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Reviewer's GuideImplements an interactive prompt flow for Sequence diagram for interactive aid workspace boot and prompt collectionsequenceDiagram
actor User
participant Terminal
participant Aid_main as aid_run
participant Interactive as interactive_collect_prompt
participant Dl as dl_lib
participant BootChild as aid_boot_child
User->>Terminal: run "aid <workspace>" (no prompt)
Terminal->>Aid_main: argv
Aid_main->>Interactive: collect_prompt(parsed: AidArgs)
Interactive->>Dl: interactive_terminal()
Dl-->>Interactive: bool (true for TTY and no DEVLAUNCH_NO_TTY)
Interactive->>Interactive: build_boot_args(parsed: AidArgs)
Interactive->>BootChild: BootChild::spawn(boot_args)
BootChild-->>Interactive: Some(BootChild)
Interactive->>Dl: read_terminal_submission()
Dl-->>Interactive: typed_prompt: String
Interactive-->>Aid_main: (parsed.with_prompt(typed_prompt), Some(BootChild))
Aid_main->>Aid_main: build_dl_args(parsed: AidArgs)
Aid_main->>BootChild: BootChild::finish()
BootChild->>Terminal: relay() boot log to stderr
BootChild-->>Aid_main: boot done
Aid_main->>Dl: dl::run(dl_args) %% foreground attach
Dl-->>Terminal: agent session starts
Terminal-->>User: interactive agent conversation
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 1 issue, and left some high level feedback:
- In
BootChild::spawnthe boot log filename is only keyed byprocess::id(), so concurrent interactive boots from the same process will share a log file; consider incorporating the workspace/spec or a monotonic counter/UUID into the filename to avoid collisions and interleaved output. - The interactive test
World::withhardcodestempdir_in("/tmp"), which assumes a Unix-like filesystem; if you want these tests to be more portable, it may be safer to rely on the defaulttempdir()location or gate this on the target platform.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `BootChild::spawn` the boot log filename is only keyed by `process::id()`, so concurrent interactive boots from the same process will share a log file; consider incorporating the workspace/spec or a monotonic counter/UUID into the filename to avoid collisions and interleaved output.
- The interactive test `World::with` hardcodes `tempdir_in("/tmp")`, which assumes a Unix-like filesystem; if you want these tests to be more portable, it may be safer to rely on the default `tempdir()` location or gate this on the target platform.
## Individual Comments
### Comment 1
<location path="rust/dl/src/lib.rs" line_range="201" />
<code_context>
+ }
+ }
+ }
+ String::from_utf8_lossy(&bytes).trim_end().to_owned()
+}
+
</code_context>
<issue_to_address>
**issue (bug_risk):** Trimming with `trim_end()` also strips trailing spaces/tabs, which may be unintended for prompts where trailing whitespace is significant.
Because `trim_end()` removes all trailing Unicode whitespace, prompts ending with user-provided spaces or tabs will be altered before reaching the agent, while argv-based prompts keep those characters. If exact prompt bytes are important, restrict trimming to `
`/`
` (e.g., by manually removing only those characters) instead of using `trim_end()`.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
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:
|
# Conflicts: # CHANGELOG.md
blooop
left a comment
blooop
left a comment
There was a problem hiding this comment.
This was generated by AI during review.
Retrospective review of a merged PR. #329 merged nine minutes after opening on CI-green auto-merge, so this is the review it did not get. Every finding below was re-verified against current main (a5f7ed8), which already carries #343, #348, #369 and #376 — anything that only reproduced at 5ce1286 was dropped. Fixed point: git diff $(git merge-base aee2de0^1 aee2de0^2)...aee2de0^2 = 172975c...5ce1286. Reproductions were run against a debug build of main with the launch_scenario.py world and test/fixtures/devpod_shim.py.
The two axes below were run as independent passes and are not merged or reranked. A third section holds the directed hunt at the race this PR introduces, which is neither a standards question nor a spec one.
Standards
1. Blocking — rust/aid/src/interactive.rs:65-67: the boot log is a predictable path in a world-writable directory, opened with a symlink-following truncate.
let log = std::env::temp_dir().join(format!("devlaunch-aid-boot-{}.log", std::process::id()));
let out = std::fs::File::create(&log).ok()?;File::create is O_WRONLY|O_CREAT|O_TRUNC — no O_EXCL, no O_NOFOLLOW — at a name any other user on the host can compute. This is CWE-377/CWE-59, not just the collision Sourcery named. Reproduced: with the victim's own /tmp sprayed with devlaunch-aid-boot-<pid>.log -> $VICTIM symlinks over a 400-pid window, one aid <workspace> run left
victim is still a regular file? True
VICTIM NOW CONTAINS: 'Workspace devlaunch-main-zovomobo is ready\n...'
i.e. an arbitrary file the victim can write was truncated and overwritten with devpod's boot output — and finish() (interactive.rs:113) then unlinks the attacker's symlink, so the planted link is gone by the time anyone looks. Separately, the log is created world-readable (measured 0664 under umask 002, 0644 under the usual 022), publishing private repo and workspace names plus the whole container build log to every user on the box.
It also bypasses the repo's own temp-dir contract: devlaunch_core::osext::temp_dir() (osext.rs:88) is the validated gettempdir equivalent, and clients/gh.rs:322-329 is the in-repo idiom for exactly this — tempfile::Builder::new().prefix(...).tempfile_in(osext::temp_dir()) followed by interrupt::register_file(...).
Fix: use that idiom, exposed through dl the way interactive_terminal() already is (aid's [dependencies] rule allows only dl; tempfile is currently a dev-dependency of aid only). Registering the log with interrupt::register_file also fixes finding 5 below for free.
2. Non-blocking — rust/dl/src/lib.rs:242-247: DEVLAUNCH_NO_TTY is read two different ways, so the one documented switch is two switches.
no_tty_requested does a bare matches!(value, "" | "0" | "false" | "no"), while every other reader normalizes first — clients/ssh.rs:153: !FALSEY.contains(&crate::osext::strip(value).to_lowercase().as_str()). So DEVLAUNCH_NO_TTY=NO, =False, or ="0 " suppresses aid's prompt editor while ssh still hands over a pty. The function's own doc (lib.rs:239) claims "The falsey list is clients/ssh.rs's (FALSEY)" — the repo sanctions copying the constant, not diverging the reading — and README.md:527 documents one row, DEVLAUNCH_NO_TTY=1 | No prompt question, no pty. The unit tests at lib.rs:520-535 only try exact lowercase spellings, so they lock the divergence in.
Fix: strip + lowercase before the match; add "NO", "False", " 0 " to the test table.
3. Minor. rust/aid/src/main.rs:67 spells "--update-cache" as a bare literal where flows/lifecycle.rs:233 holds it as UPDATE_CACHE_FLAG — Shotgun Surgery with a silent failure mode, which is the exact bug the comment above it says it is fixing; re-export the constant through dl as BOOT_WORD already is. interactive.rs:163-166 reaches into rewrite::Task::Agent { prompt, .. } from outside rewrite though AidArgs already owns agent()/with_prompt() (Feature Envy; a wants_prompt() beside them closes it). interactive.rs:130-146 re-opens and re-seeks the log on every 50 ms tick for the whole container boot; hold the File in BootChild. banner is pub(crate) with one caller seven lines away.
Refuted and not reported: the FALSEY array duplication itself (ssh.rs:52-56 argues for it explicitly); BootChild leaking on the build_dl_args None path (unreachable — collect_prompt only spawns for Task::Agent, and only Task::Retired returns None).
Spec
No spec available.
closingIssuesReferences is empty and the body carries no #n. The five branch commits cite no ticket; the PR's only comments are Sourcery's. A full-text sweep of issues over 2026-08-18→08-24 (while the workspace boots, DEVLAUNCH_NO_TTY, boot-up, interactive prompt) turns up only downstream artifacts — #327, filed three minutes before this PR about docker-load errors hit while testing the finished flow, its parent map #326, and #317, whose pty half was "overtaken on main". Map #326 settles it in the author's own words: "Filed as follow-up from testing the interactive aid prompt (PR for that feature is separate)" — the feature deliberately had no wayfinder node. The branch touches nothing under docs/, and docs/rust-rewrite-plan.md (the closest thing this repo has to a written spec) has no row for a promptless-aid flow.
The PR body is detailed enough to look like a spec — it even enumerates "accepted warts, deliberate" — but it was written by the implementing author after the code, so scoring the diff against it would only confirm the diff agrees with its own description. Nothing on this axis is scored, and no requirements were inferred from the code.
Directed hunt: the race, the log, the terminal, trim_end, test strength
4. Blocking — rust/aid/src/interactive.rs:63-93 + rust/aid/src/main.rs:104: a SIGTERM or SIGHUP delivered to aid while the user is typing orphans the boot child, its devpod up, and the staged plaintext GH_TOKEN.
interactive.rs:23-27 argues the teardown case, and it argues only the one that works:
The child is deliberately left in aid's process group: a terminal Ctrl-C mid-editing reaches both processes…
That holds for SIGINT, which the line discipline sends group-wide. #348 then added two signals that are not group-wide — and DRAINED's own doc (dl/src/lib.rs:118-121) says why they matter: every one of them "leaves dl holding a staged plaintext token and a live devpod up child if nothing intervenes." Nothing intervenes here: BootChild is registered with neither interrupt::register_file nor note_foreground_child, so when the drain runs during the prompt read, FOREGROUND_PGID is still 0 and FILES is empty. aid exits tidily and cleans up nothing.
Reproduced on current main, driving the binary through a pty without setsid (so aid is an ordinary foreground process as it is under a user's shell, rather than the session leader portable_pty makes it — that is why the existing pty suite cannot see this: killing the session leader hangs up the terminal, which SIGHUPs the whole foreground group and cleans up by accident):
before signal: aid=178695 boot-child=[178697] devpod-up=178787 token=[devlaunch-gh-gpXncc.env]
aid exited rc=143
AT +3s: boot-child-alive=[178697] devpod-up-alive=True staged-token-left=[devlaunch-gh-gpXncc.env] boot-log-left=[devlaunch-aid-boot-178695.log]
SIGHUP is identical (rc=129, everything survives). timeout 300 aid <ws>, a cancelled CI step, a supervisor stop, or a plain pkill aid all take this path, and it leaks a plaintext GitHub token to disk plus a container build nobody is waiting on any more.
Fix: give the boot child its own process group (process_group(0)) and note_foreground_child(pgid) for the duration of the read, clearing it in finish() — or add a second killpg slot to the interrupt registry for it. Either way the boot child becomes something the existing drain already knows how to kill, which is the machinery #348 built.
5. Non-blocking — rust/aid/src/interactive.rs:113: any signal exit during the interactive prompt leaks the boot log.
remove_file lives only on the finish() path, which a drain never reaches. Reproduced as a red test: a Ctrl-C at the editor, in the shape of a_ctrl_c_at_the_editor_tears_the_whole_boot_down, leaves $TMPDIR/devlaunch-aid-boot-<pid>.log behind. Subsumed by finding 1's interrupt::register_file.
6. Non-blocking — rust/dl/src/lib.rs:231-235: the terminal gate tests the wrong descriptor pair, so aid <ws> 2>/dev/null hangs silently while a container builds.
interactive_terminal() is isatty(0) && isatty(1), borrowed from ssh's terminal_usable, where that pair is right because the pair carries the session. Here the banner and the > are written to stderr (interactive.rs:182-188) and nothing at all goes to stdout. Reproduced: with stdin/stdout on a pty and stderr to /dev/null, aid printed nothing, blocked forever on a line it never asked for, and started a background workspace build anyway:
aid still running? True
anything on the terminal: b''
did the boot start anyway? True
The mirror case is a needless refusal: aid <ws> | tee log has a perfectly good prompt terminal on stderr and gets the one-shot path. Fix: gate on isatty(0) && isatty(2), or put the banner on stdout.
7. trim_end() (rust/dl/src/lib.rs:296) — the author's rebuttal holds. No change wanted.
Verified: read_terminal_submission has exactly one caller in the tree (interactive.rs:174), its result goes through AidArgs::with_prompt into the agent's prompt argv word, and nothing else consumes it. The trim is not merely harmless, it is load-bearing — the read consumes the terminating newline into bytes and the paste tail pushes another, so without it every prompt would ship with a trailing \n. For a multi-line paste it only strips trailing blank lines at the very end; leading indentation, interior blank lines and interior whitespace are untouched, so a pasted code block survives intact. There is no caller for which trailing whitespace is significant — no path, no heredoc, no delimiter. The one asymmetry is real but immaterial for the same reason: an argv prompt (aid <ws> "fix the bug ") keeps its trailing spaces where a typed one does not.
The narrow point worth keeping: the trim is baked into a pub dl function whose name promises "read a submission", not "read a prompt". If a second caller ever reads something structured through it, the trim becomes a bug at a distance. Moving the .trim_end() to the aid call site — or naming the guarantee in the doc comment — costs nothing and retires the question.
8. Non-blocking — test strength: the wait is pinned, the replay and the failure report are not.
Mutation testing on current main:
- Making
BootChild::finisha no-op turnsthe_boot_runs_while_the_prompt_is_still_being_typedred. The overlap and the wait are genuinely covered. - Keeping the wait and the log removal but making
relay()never run and deleting the"aid: the background boot exited {code}"/"was killed"arms leaves all 59cargo test -p aidtests green. So two of the three behavioursfinish()documents — replaying the parked build output, and telling the user a boot failed — are unpinned; the module could silently stop doing both and CI would not notice. This is the same blind spot the PR body's "boot refusals surface only after Enter and print twice" wart lives in.
Fix: assert the pty text contains the shim's boot output after Enter, and add a scenario whose up exits non-zero to pin the failure line.
Also on tests: Sourcery's tempdir_in("/tmp") (rust/aid/tests/interactive.rs:45) is still there and still ignores TMPDIR, so the pty suite fails on any host or CI runner that points TMPDIR at a per-job directory or makes /tmp unusable. tempfile::Builder::new().prefix("aidpty").tempdir() is the one-word fix. (Note this is independent of the TMPDIR the Ctrl-C test sets for the child, which is deliberate and should stay.)
Disposition of the three Sourcery findings
| # | Sourcery's point | Status on main |
Disposition |
|---|---|---|---|
| 1 | boot log keyed only by process::id() |
still present, interactive.rs:65-67 |
Upheld, and understated. The collision as stated is not actually reachable — collect_prompt runs once per process, so one aid never has two boots. The real defect at that line is the predictable path in a world-writable directory: proven arbitrary-file truncation via symlink, plus a world-readable build log and no cleanup on a signal. See findings 1 and 5. |
| 2 | test World::with hardcodes tempdir_in("/tmp") |
still present, tests/interactive.rs:45 |
Upheld, non-blocking. See finding 8. |
| 3 | trim_end() strips significant trailing whitespace |
still present, dl/src/lib.rs:296 |
Not upheld — the author is right. Verified on the merits: one caller, the value is a natural-language prompt, and the trim is required to remove the newline the read consumes. Sourcery's concern has no caller behind it. See finding 7. |
The author's trim_end rebuttal is correct and should not have been the only one of the three that got a reply.
Verdict
Request changes (posted as a comment — GitHub refuses a change request on a merged PR; the written verdict is the gate).
Blocking, and both are follow-up work on main rather than anything this PR can still change:
rust/aid/src/interactive.rs:65-67— predictable-path, symlink-following, world-readable boot log in/tmp. Proven arbitrary-file clobber. Fix with thetempfile+osext::temp_dir()+interrupt::register_fileidiom already inclients/gh.rs.rust/aid/src/interactive.rs:63-93/main.rs:104— SIGTERM/SIGHUP during the prompt read orphans the boot child, itsdevpod up, and a staged plaintextGH_TOKEN. Proven. Fix by making the boot child visible to the drain #348 built.
Non-blocking: the DEVLAUNCH_NO_TTY reading divergence (2), the leaked boot log on Ctrl-C (5, subsumed by 1), the stdout-vs-stderr terminal gate (6), the unpinned replay and failure report (8), tempdir_in("/tmp") in the pty suite (8), and the minors in (3).
The feature itself is well built — the process-not-thread choice, the reuse of the per-workspace launch lock, and the pty suite are the right calls, and the overlap is genuinely covered by a test that dies when you break it. What the nine-minute merge cost was the second look at what happens when the other side of the race — the process, not the user — is the one that gets killed.
1 participant
Summary
aid <workspace>with no prompt on a terminal is now interactive: the workspace starts booting in the background as anaid --boot-up … upchild (dl's ownupverb — the prewarm shape the per-workspace launch lock already serializes) while the prompt is typed at the terminal, free of shell quoting. Enter launches with the typed prompt through the existing rewrite; empty Enter or Ctrl-D is the plain session a bareaidalways started. The boot's output is parked in a log and replayed after the Enter, and Ctrl-C at the editor tears the boot down through the shared interrupt disposition (pty-tested).DEVLAUNCH_NO_TTY=1keeps the old one-shot behavior byte-for-byte — the existing integration suite passes unchanged.--update-cacheverbatim to dl. dl respawns its detached completion refresh throughcurrent_exe, which under aid is aid, and that line used to die as "aid needs a workspace" on every aid launch.interactive_terminal()andread_terminal_submission()(aid's only route to libc stays dl); devlaunch-core's public surface is untouched, so the public-api snapshot does not move.Test plan
rust/aid/tests/interactive.rs(portable-pty, dev-only; wired into CI beside the interrupt step): typed prompt with literal quotes reaches the agent; empty Enter is the plain session; the shim'supis logged before Enter is pressed (the overlap itself); Ctrl-C at the editor exits 130, kills the blockeddevpod up, and removes the staged token; a pasted multi-line prompt arrives whole.--autorm/--force, keeps--devcontainerand unknown flags), typed-prompt injection through the existing quoting/prompt-flags,DEVLAUNCH_NO_TTYfalsey list.cargo test --workspace,clippy --all-targets -D warnings,fmt --check, prek hooks,--lockedbuild at 0.4.0.test_aid_leaves_an_interactive_agent_runningupdated to press the empty Enter.Review notes
--devcontainerprints "Ignoring --devcontainer" twice (boot replay + foreground — dropping the flag from the foreground would break the boot-failed retry); boot refusals surface only after Enter and print twice; an unterminated paste fragment still reaches the agent as type-ahead (cooked-mode limit — bracketed paste would be a redesign); twoDEVLAUNCH_TIMINGsummaries per interactive run (in CHANGELOG).🤖 Generated with Claude Code
Summary by Sourcery
Allow promptless terminal launches to gather the agent prompt while prewarming the workspace, without changing scripted behavior.
New Features:
aid <workspace>launches collect the agent prompt while the workspace boots in the background, with empty submissions preserving plain sessions and non-terminal invocations retaining one-shot behavior.Bug Fixes:
aidtodlso they complete successfully.Enhancements:
dlwhile keeping the devlaunch-core public API unchanged.Build:
CI:
Documentation:
Tests: