Stop devpod's stream logger from mangling a command's stderr - #623
Merged
Merged
Conversation
`dl <ws> -- <cmd>` reaches the container through `devpod ssh --command`, which asks for no pty, and without a pty the container's stderr is folded into devpod's own and reformatted by devpod's stream logger on the way out. Measured against devpod 0.26.1, `echo ERR >&2` came back as a timestamp, a coloured `info` tag, the text, and `stream_logger.go:492` -- unparseable as a compiler's diagnostics, which is what the published contract promises a caller gets. devpod's `--log-output json` is now passed on that invocation, and the stderr filter reads each line as a log record: the level is a field rather than a coloured tag, and the bare `message` is forwarded, which for the command's stderr is the command's bytes. Not `raw`, though it is the obvious choice. devpod buries a nonzero remote exit in a `fatal` line (it type-asserts on `*ssh.ExitError` after wrapping it three times with `%w`), and dl recovers the status by reading that line's tag. `raw` drops the tag, the recovery returns nothing, and `dl ws -- 'exit 42'` silently stops exiting 42 -- trading the contract's first clause for its last. json keeps the level, and a level the container cannot forge: devpod escapes whatever the container writes into a record of its own at `info`. Scoped to the command arm. A bare attach gets a pty, the container's stderr never touches this stream there, and json would only strip the colour off devpod's own warnings to the person reading them. A line that is not a record falls through to the plain-text predicates, so an older devpod and the attach route behave exactly as before.
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
Reviewer's GuideThe PR switches non-PTY DevPod command sessions to JSON logging, unwraps log records in the stderr filter without compromising remote exit-status recovery or attach behavior, and formalizes the resulting clean-stderr guarantee through tests and documentation. Sequence diagram for clean stderr and remote exit recoverysequenceDiagram
participant Caller
participant DL
participant Devpod
participant Container
Caller->>DL: dl workspace -- command
DL->>Devpod: ssh workspace --log-output json --command payload
Devpod->>Container: execute payload without pty
Container-->>Devpod: stderr line
Devpod-->>DL: JSON info record with message
DL->>DL: LogRecord.parse(line)
DL-->>Caller: bare message on stderr
Container-->>Devpod: nonzero exit status
Devpod-->>DL: JSON fatal record with status text
DL->>DL: remote_status_in(message)
DL-->>Caller: command exit status
Flow diagram for StderrFilter record handlingflowchart TD
Start["Read stderr line"] --> Parse["LogRecord.parse(line)"]
Parse -->|record| Message["Use record.message"]
Parse -->|parse miss| Plain["Use original line"]
Message --> Level{"level is fatal?"}
Level -->|yes| Status["remote_status_in(message)"]
Level -->|no| Forward["Forward message"]
Plain --> Legacy["recovered_status(line) and legacy predicates"]
Status -->|status found| Hold["Hold back report"]
Status -->|no status| Forward
Legacy -->|status found| Hold
Legacy -->|no status| Forward
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:
|
…atim # Conflicts: # CHANGELOG.md # docs/agents-using-dl.md
Merged
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The last broken clause
docs/agents-using-dl.mdpublishes a subprocess contract, and until now one clause of itwas a section headed "stderr is not yours yet".
dl <ws> -- <cmd>reaches the containerthrough
devpod ssh --command, which asks for no pty, and without a pty the container'sstderr is folded into devpod's own and reformatted by devpod's stream logger on the way
out.
Measured on this host, devpod 0.26.1,
dl <ws> -- sh -c 'echo ERR >&2':Timestamp, colour, level tag and a Go source location, all gone. stdout was already clean,
so a caller could parse a command's JSON but not its compiler's diagnostics.
The fix, and the trap next to it
devpod's global
--log-outputtakesplain(the default),raworjson.rawis theobvious choice and would have been a silent regression of the contract's first clause.
devpod means to pass a remote exit status through and cannot: its top-level handler
type-asserts on
*ssh.ExitErrorafter wrapping it three times with%w, so every nonzeroremote exit lands on the generic failure path and exits 1 with the real status buried in a
fatalline. dl recovers the number by reading that line, anchored on the wordfatalsoa remote program printing the same sentence cannot impersonate the report. Measured:
Under
rawthe tag is gone,recovered_statusreturnsNone, anddl ws -- 'exit 42'stops exiting 42.
So json, which keeps the level as structured data. That is both a stronger anchor than a
coloured tag and one the remote program cannot forge: devpod wraps whatever the container
writes in a record of its own at
info, escaping it, so a container printing an entirefatal record verbatim arrives as that record's
message(measured, and pinned by a unittest).
--silentis wrong for the opposite reason: it swallows the command's stderr.StderrFilter::pushnow parses each line as a record and forwards the baremessage. Aparse miss falls through to the plain-text predicates unchanged, so a devpod too old to
know the flag, the attach route's plain log, and anything on the stream that is not a log
line at all behave exactly as they did. The
--debug-hint hold-back is matched against themessage rather than the raw line, so it works on both readings.
Scoped to the non-pty case, on purpose
--log-output jsongoes on the--commandarm ofdevpod_sessionand nowhere else.A bare attach gets a pty, and under one the container's stderr never touches this stream --
the existing comment saying devpod's stderr there "carries devpod's own warnings and
nothing else" is right. The only thing json would change is the look of those warnings to
the person sitting in front of them, trading a coloured
warntag for nothing. So aninteractive
dl <ws>logs exactly as it did.a_command_asks_devpod_to_log_in_json_and_an_attach_does_notpins both halves.
The flag goes after the workspace id:
session_manager'sworkspace_named_byreadsthe id out of
devpod ssh <id>by position, and a flag in front of it would make everysession anonymous to the manager. Cobra accepts a global flag in that position (measured
against the real binary).
Verified against a live container
Built binary, real workspace, real cache:
Every clause of the published contract was replayed against that container with the new
binary, the
2>&1merge recipe included. All pass.Docs and tests
test_stderr_is_the_commands_output_verbatimwas a strict xfail whose stated purposewas to turn the suite red the day the transport was fixed. It is a normal passing test
now, and a second e2e case asserts the wrapper is absent by name rather than only that
the tail matches.
list, with two honest caveats: lines are read one at a time, so an unterminated last line
gains a newline; and devpod's logger strips ANSI from what it carries, so a tool that
colours its errors arrives uncoloured (the command sees a pipe, so most would anyway).
2>&1merge still works, is still shown, and is no longer presented as the requiredform -- only as what to reach for when you want one interleaved stream.
test_agent_contract_doc.pygained the fifth promise phrase.Nothing contradicted the research
Everything in the brief reproduced: the three log modes, stdout untouched in all of them,
--silentswallowing stderr, andrawlosing thefataltag. Two things worth adding:devpod's logger strips ANSI escapes from the message in both
plainandjson(onlyrawpreserves them), and dl's own provisioning
devpod ssh --commandcalls go throughdevpod::runrather thandevpod_session, so they keep plain logging -- their stderr isdl's business, not a caller's.
Expect a small CHANGELOG/docs conflict with #621.
🤖 Generated with Claude Code
Summary by Sourcery
Preserve command stderr while retaining remote exit-status handling by using structured devpod logging only for non-interactive command sessions.
New Features:
Bug Fixes:
Enhancements:
Documentation:
Tests: