fix(trace): write diagnostics to stderr so they don't corrupt captured stdout by eliebleton-manomano · Pull Request #2375 · DataDog/datadog-ci

eliebleton-manomano · 2026-06-24T10:53:15Z

What and why?

datadog-ci trace -- <cmd> runs the wrapped command with stdio: ['inherit', 'inherit', 'pipe'], so the child's stdout is fd 1 and its bytes are captured verbatim by VAR=$(datadog-ci trace -- cmd). But trace also wrote some of its own diagnostics to fd 1 (console.log / context.stdout.write), so those lines got mixed into the capture.

When the wrapped command emits a secret — e.g. TOKEN=$(… vault write -field=token …) — the captured value gains a newline + diagnostic text and is rejected downstream (configured Vault token contains non-printable characters). It is intermittent and load-correlated: the --no-fail note only fires when the span report fails (e.g. under intake throttling).

These diagnostics belong on stderr, consistent with the adjacent error paths in the same files that already use this.context.stderr.write. The datadog-ci v<version> banner has the same problem — it's printed to stdout on every invocation, so it can corrupt the same captures — and on top of that it's noisy in CI where the CLI runs many times.

Resolves #2381.

How?

Route three trace diagnostics from stdout → stderr:

the --no-fail note (trace.ts) — was console.log, now this.context.stderr.write (with the trailing \n that console.log implied)
the unsupported-CI-provider message (helper.ts, shared CustomSpanCommand)
the missing-API-key message (helper.ts, shared CustomSpanCommand)

The helper.ts changes also harden the span command, which shares the CustomSpanCommand base. No change to stdio inheritance, the childProcess.stderr pipe, or the span/reporting logic.

Also route the version banner to stderr (cli.ts), mirroring the resolved command's --log-format so it stays a JSON line in JSON mode; builtin commands and parse errors fall back to text format. And add DD_CI_SKIP_VERSION_BANNER (1/true) to suppress the banner entirely, following the existing DD_CI_-prefixed toggle convention (e.g. DD_CI_BYPASS_SITE_VALIDATION).

Adds regression tests: the --no-fail note lands on stderr (not stdout), the shared CI-provider assertion is updated to match, and the banner is suppressed when DD_CI_SKIP_VERSION_BANNER is 1/true.

Judgment call left out (happy to add if preferred): the --dry-run payload write in helper.ts is also capturable, but moving it would change span --dry-run | jq behaviour and is lower risk (only under --dry-run), so it's left on stdout.

Review checklist

Feature or bugfix MUST have appropriate tests (unit, integration)

juan-fernandez

Leaving one inline review comment for the stream lifetime issue.

juan-fernandez · 2026-06-26T10:02:44Z

    if (res !== 0) {
      if (this.noFail) {
-        console.log('note: Not failing since --no-fail provided')
+        this.context.stderr.write('note: Not failing since --no-fail provided\n')


[P2] This can write to an ended stderr stream. childProcess.stderr.pipe(this.context.stderr) uses the default {end: true}, so once the wrapped command exits it ends the destination stream. process.stderr tolerates a later write, but regular Writable streams used by programmatic Clipanion callers or tests will emit write after end when this diagnostic is written afterward. The test currently masks that by overriding stderr.end; the command should instead pipe with {end: false} before moving post-child diagnostics onto stderr.

Good catch. I believe this is now fixed and the test was adjusted accordingly.

juan-fernandez

thanks for the contribution! This makes a lot of sense.

A few non-blocking suggestions that could make this safer and easier to maintain:

Add a CLI-level regression test for the banner stream. The new tests cover printVersion(logger), but the customer-facing behavior lives in packages/datadog-ci/src/cli.ts: the automatic banner should go to stderr, explicit datadog-ci version / --version should still go to stdout, and command stdout should stay clean for captures/pipes. A small integration-style test would protect the exact bug class this PR is fixing.
Consider using the existing toBoolean() helper for DD_CI_SKIP_VERSION_BANNER. Right now the new env var accepts exactly 1 or lowercase true; most env parsing in this repo uses toBoolean(), which handles casing consistently.
Consider preserving printVersion API compatibility. printVersion is exported from @datadog/datadog-ci-base/version and re-exported by @datadog/datadog-ci; changing it from printVersion() to printVersion(logger) could break external callers that import it directly. If that API surface matters, making the logger optional would reduce the blast radius.
If DD_CI_SKIP_VERSION_BANNER is intended for customers rather than just as an escape hatch, it may be worth documenting it somewhere discoverable.

datadog-datadog-prod-us1-2 · 2026-06-26T10:08:09Z

Tests

🎉 All green!

🧪 All tests passed
❄️ No new flaky tests detected

_{This comment will be updated automatically if new data arrives.

🔗 Commit SHA: 2d6217b | Docs | Datadog PR Page | Give us feedback!}

rodrigo-roca

there is also the span subcommand which reuses most of the logic, and still use context.stdout.write if I recall correctly. I think we are missing changes in that command, can you take a look at span.ts?

rodrigo-roca · 2026-06-26T10:35:46Z

+  // while mirroring the resolved command's `--log-format` so it stays a JSON line in JSON mode.
+  // Builtin commands (e.g. --help) and command line parse errors fall back to text format.
+  const jsonOutput = command instanceof BaseCommand ? command.logger.isJsonOutput() : false
+  const versionLogger = new Logger((s) => context.stderr.write(s), LogLevel.INFO, {jsonOutput})


is there unit tests for this piece of new logic? If not can we add some?

Added packages/datadog-ci/src/__tests__/version-banner.test.ts — it spawns the real CLI entry point and asserts this logic end-to-end: the banner goes to stderr (never stdout, which is the bug class this fixes), --log-format json is mirrored as a JSON line, version/--version still print to stdout, and DD_CI_SKIP_VERSION_BANNER suppresses it.

An in-process cli.run() test wouldn't cover it, since the banner is emitted under require.main === module.

-- Claudelie

eliebleton-manomano · 2026-06-26T11:03:20Z

Most of the concerns in Juan's comments have been addressed in the latest version. I'll work through the rest in short order.

* Consider preserving `printVersion` API compatibility. `printVersion` is exported from `@datadog/datadog-ci-base/version` and re-exported by `@datadog/datadog-ci`; changing it from `printVersion()` to `printVersion(logger)` could break external callers that import it directly. If that API surface matters, making the logger optional would reduce the blast radius.

The printVersion(logger) signature landed on master with the --log-format json feature (426c548) — this PR doesn't change it. The only change here is an internal early-return for DD_CI_SKIP_VERSION_BANNER, so there's no API-surface change to guard against.

eliebleton-manomano · 2026-06-26T11:15:43Z

Thanks for running the CI! 🙏

Fixed the standalone smoke test: plugin check synthetics run-tests now expects the datadog-ci v<version> banner on stderr. The banner intentionally moved off stdout (so it can't corrupt captures), and run-tests isn't a skip-listed arg like --help/version, so it now lands on stderr — the other smoke cases use --help/version and stay stderr: ''. That was the assertion behind the failing standalone jobs.

Also rebased onto the latest master to pick up the flaky-test fix.

-- Claudelie

…d stdout `datadog-ci trace -- <cmd>` runs the wrapped command with `stdio: ['inherit', 'inherit', 'pipe']`, so the child's stdout is fd 1 itself and its bytes are captured verbatim by `VAR=$(datadog-ci trace -- cmd)`. But `trace` also wrote some of its own diagnostics to fd 1 (`console.log` / `context.stdout.write`), so those lines got mixed into the capture. When the wrapped command emits a secret (e.g. `TOKEN=$(... vault write -field=token ...)`), the captured value gains a newline + diagnostic text and is rejected downstream ("configured Vault token contains non-printable characters"). The corruption is intermittent and load-correlated: the `--no-fail` note only fires when the span report fails (e.g. under intake throttling). Route these diagnostics to stderr instead, consistent with the adjacent error paths that already use `context.stderr.write`: - the `--no-fail` note (trace.ts) - the unsupported-CI-provider message (helper.ts, shared CustomSpanCommand) - the missing-API-key message (helper.ts, shared CustomSpanCommand) The helper.ts changes also harden the `span` command, which shares the base. Adds a regression test asserting the `--no-fail` note lands on stderr (not stdout), and updates the shared CI-provider test assertion accordingly. Co-Authored-By: Claude Opus 4.8 <[email protected]>

The `datadog-ci v<version>` banner was the one remaining diagnostic still written to stdout, so it could corrupt captured output the same way the trace diagnostics did (`VAR=$(datadog-ci -- cmd)`). Route it to stderr instead, mirroring the resolved command's `--log-format` so it stays a JSON line in JSON mode. Builtin commands and parse errors fall back to text format. Also add `DD_CI_SKIP_VERSION_BANNER` (`1`/`true`) to suppress the per-invocation banner entirely for callers that find it too noisy. Co-Authored-By: Claude Opus 4.8 <[email protected]>

eliebleton-manomano · 2026-06-26T11:33:16Z

One more consistency fix for the trace span subcommand (shares the CustomSpanCommand base).

The inherited diagnostics (unsupported-CI-provider, missing-API-key) were already moved to stderr in this PR. But span also had its own four argument-validation messages — missing --name, conflicting/missing/negative duration — still written to stdout while returning a non-zero exit code. That's the same diagnostics-on-stdout issue this PR is fixing elsewhere (and trace's analogous "Missing command to run" was already on stderr), so they could leak into a $(datadog-ci trace span …) capture.

Moved those four to stderr and added a regression assertion (the existing validation tests only checked the exit code, not the stream). The [DRY-RUN] payload stays on stdout, since that's the command's intended output.

-- Claudelie

Drarig29

LGTM

juan-fernandez

awesome! Thanks!

eliebleton-manomano requested review from a team as code owners June 24, 2026 10:53

eliebleton-manomano requested a review from juliomalegria-dd June 24, 2026 10:53

eliebleton-manomano mentioned this pull request Jun 25, 2026

Route diagnostics and the version banner from stdout to stderr (and allow suppressing the banner) #2381

Closed

juan-fernandez reviewed Jun 26, 2026

View reviewed changes

juan-fernandez added the software-delivery Related to [coverage, deployment, dora, junit, measure, tag, trace] label Jun 26, 2026

This comment was marked as outdated.

Sign in to view

rodrigo-roca reviewed Jun 26, 2026

View reviewed changes

eliebleton-manomano force-pushed the fix/trace-diagnostics-to-stderr branch from 9cae238 to a55a308 Compare June 26, 2026 11:00

eliebleton-manomano requested a review from a team as a code owner June 26, 2026 11:00

eliebleton-manomano force-pushed the fix/trace-diagnostics-to-stderr branch from a55a308 to 276856f Compare June 26, 2026 11:15

eliebleton-manomano and others added 2 commits June 26, 2026 13:31

eliebleton-manomano force-pushed the fix/trace-diagnostics-to-stderr branch from 276856f to d6257f8 Compare June 26, 2026 11:32

Merge branch 'master' into fix/trace-diagnostics-to-stderr

2d6217b

Drarig29 force-pushed the fix/trace-diagnostics-to-stderr branch from dda497f to 2d6217b Compare June 26, 2026 12:30

Drarig29 approved these changes Jun 26, 2026

View reviewed changes

juan-fernandez approved these changes Jun 26, 2026

View reviewed changes

Drarig29 merged commit 0dc4028 into DataDog:master Jun 26, 2026
34 checks passed

ava-silver mentioned this pull request Jun 29, 2026

v5.20.0 #2389

Merged

Uh oh!

Conversation

eliebleton-manomano commented Jun 24, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What and why?

How?

Review checklist

Uh oh!

juan-fernandez left a comment

Choose a reason for hiding this comment

Uh oh!

juan-fernandez Jun 26, 2026

Choose a reason for hiding this comment

Uh oh!

eliebleton-manomano Jun 26, 2026

Choose a reason for hiding this comment

Uh oh!

juan-fernandez left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

datadog-datadog-prod-us1-2 Bot commented Jun 26, 2026 • edited by datadog-official Bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

This comment was marked as outdated.

rodrigo-roca left a comment

Choose a reason for hiding this comment

Uh oh!

rodrigo-roca Jun 26, 2026

Choose a reason for hiding this comment

Uh oh!

eliebleton-manomano Jun 26, 2026

Choose a reason for hiding this comment

Uh oh!

eliebleton-manomano commented Jun 26, 2026

Uh oh!

eliebleton-manomano commented Jun 26, 2026

Uh oh!

eliebleton-manomano commented Jun 26, 2026

Uh oh!

Drarig29 left a comment

Choose a reason for hiding this comment

Uh oh!

juan-fernandez left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

eliebleton-manomano commented Jun 24, 2026 •

edited

Loading

juan-fernandez left a comment •

edited

Loading

datadog-datadog-prod-us1-2 Bot commented Jun 26, 2026 •

edited by datadog-official Bot

Loading