Skip to content

feat(interp): add WarningsWriter option and Warnings accessor for sandbox diagnostics#200

Merged
matt-dz merged 1 commit intomainfrom
claude/jovial-knuth-e8a24e
Apr 27, 2026
Merged

feat(interp): add WarningsWriter option and Warnings accessor for sandbox diagnostics#200
matt-dz merged 1 commit intomainfrom
claude/jovial-knuth-e8a24e

Conversation

@matt-dz
Copy link
Copy Markdown
Collaborator

@matt-dz matt-dz commented Apr 27, 2026

What does this PR do?

Decouples AllowedPaths sandbox diagnostics from r.stderr so library callers can attribute the two streams independently.

  • New interp.WarningsWriter(io.Writer) RunnerOption — routes buffered sandbox warnings to a dedicated sink. Rejects nil. Defaults to r.stderr when unset, preserving today's behaviour byte-for-byte.
  • New (*interp.Runner).Warnings() []string accessor — returns sandbox warnings programmatically (one entry per warning, nil when none) so integrators can render them in their own structured output instead of scraping a writer.
  • The flush at the end of interp.New() now writes to r.warningsWriter and no longer nils the underlying buffer, so the accessor remains usable after construction.

Motivation

Closes the rshell side of #191. When AllowedPaths contains a directory that cannot be opened on the current host (e.g. /var/log on Windows, C:\var\log on Linux), allowedpaths.New records a diagnostic that gets flushed to r.stderr during Runner construction. For library callers like PAR that surface r.stderr in structured output, this means every script execution in the session ends up with a non-empty stderr — making the result look like a command failure even though exitCode is 0.

Reasoning behind the chosen shape:

  • Conservative default (r.stderr) rather than process stderr or io.Discard, because rshell is overwhelmingly used as a library — every real caller already wires up r.stderr for command output, and silently dropping warnings on un-migrated callers would hide misconfiguration.
  • Both an option and an accessor so streaming integrations (logger, file) and structured integrations (API response field) can each pick the path that fits.
  • Buffer retained after flush because the producer emits at construction time only — there's no double-flush risk, and dropping the buffer would force callers to choose between the writer and the accessor.

This is the first of three coordinated PRs:

  1. rshell (this PR) — library API.
  2. datadog-agent — bump rshell, wire the new option in PAR, populate the response field.
  3. dd-source — additive sandbox_warnings field on the PAR response schema; UI hides the section when empty.

Testing

go test ./... clean. New tests in interp/allowed_paths_test.go:

  • TestSandboxWarningsDefaultGoToStderr — regression check that the default sink is r.stderr and that warnings never land on stdout.
  • TestSandboxWarningsRoutedToDedicatedWriter — verifies the WarningsWriter writer receives the warnings and r.stderr stays clean.
  • TestSandboxWarningsAccessor — three subtests covering the slice shape, the empty case, and that the accessor still works when the streaming flush is suppressed via io.Discard.
  • TestWarningsWriterRejectsNilWarningsWriter(nil) returns an error.

make fmt clean. go vet ./... clean. Symbol-allowlist analyzer (analysis package) re-runs clean — initial draft used strings.TrimRight which is not on the interp allowlist, rewritten to use strings.HasSuffix + slice instead.

Checklist

  • Tests added/updated
  • Documentation updated (if applicable) — short note added to the AllowedPaths section of README.md pointing callers at the new option/accessor

🤖 Generated with Claude Code

…dbox diagnostics

AllowedPaths warnings are flushed to r.stderr during Runner construction.
For library callers (e.g. PAR) that surface r.stderr in structured output,
this means every script execution in a session ends up with a non-empty
stderr whenever the configured allowlist contains a path that does not
exist on the host (e.g. /var/log on Windows), even though exitCode is 0
— making the result look like a command failure.

Decouple sandbox diagnostics from r.stderr so callers can attribute the
two streams independently:

- WarningsWriter(io.Writer) routes the buffered warnings to a dedicated
  sink. Defaults to r.stderr when unset, preserving today's behaviour
  byte-for-byte for callers that don't opt in.
- Runner.Warnings() returns the warnings as []string for integrators
  that prefer to render them in their own structured output rather
  than scraping a writer.

Closes the rshell side of #191. The PAR (datadog-agent) and remote-action
UI (dd-source) changes will land separately and consume this API.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@matt-dz matt-dz marked this pull request as ready for review April 27, 2026 19:15
@matt-dz matt-dz added this pull request to the merge queue Apr 27, 2026
Merged via the queue into main with commit fa70d7c Apr 27, 2026
34 checks passed
@matt-dz matt-dz deleted the claude/jovial-knuth-e8a24e branch April 27, 2026 19:25
gh-worker-dd-mergequeue-cf854d Bot pushed a commit to DataDog/datadog-agent that referenced this pull request Apr 27, 2026
### What does this PR do?

- Bumps `github.com/DataDog/rshell` from `v0.0.13` to `v0.0.14`. No call-site changes.

### Motivation

- v0.0.13..v0.0.14 contains a single commit, [DataDog/rshell#200](DataDog/rshell#200), which adds an opt-in `interp.WarningsWriter(io.Writer)` option and a `Runner.Warnings() []string` accessor. Today's PAR rshell handler does not pass either, so behaviour is byte-for-byte unchanged: sandbox warnings continue to fall back to `r.stderr`, which is where the handler already routes them.
- This bump lands in isolation so the follow-up PR — wiring the new accessor into `RunCommandOutputs.SandboxWarnings` to fix [DataDog/rshell#191](DataDog/rshell#191) — is a clean, reviewable diff with no `go.mod`/`go.sum` noise.

### Describe how you validated your changes

- `go get github.com/DataDog/rshell@v0.0.14` — single-line `go.mod` change, expected `go.sum` update.
- `go build ./pkg/privateactionrunner/...` — clean.
- `go test -count=1 ./pkg/privateactionrunner/bundles/remoteaction/rshell/...` — all tests pass.

### Additional Notes

- Follow-up PR (forthcoming) will wire `runner.Warnings()` into the PAR rshell handler's response. The corresponding contract change in dd-source (additive `sandboxWarnings?: string[]` field on `RunCommandOutputs`) is open at [DataDog/dd-source#423305](DataDog/dd-source#423305).

🤖 Generated with [Claude Code](https://claude.com/claude-code)

Co-authored-by: matthew.deguzman <matthew.deguzman@datadoghq.com>
matt-dz added a commit to DataDog/datadog-agent that referenced this pull request Apr 29, 2026
When the AllowedPaths list configured for the runCommand action contains
a path that does not exist on the host (e.g. /var/log on Windows), rshell
emits a non-fatal "AllowedPaths: skipping" diagnostic during runner
construction. Until v0.0.13 those diagnostics flushed onto the runner's
stderr, which the handler then surfaced in RunCommandOutputs.Stderr —
making every script execution in the session look like a failure (non-
empty stderr) even though ExitCode was 0.

rshell v0.0.14 adds a dedicated WarningsWriter sink and a Warnings()
accessor (DataDog/rshell#200). Wire both:

- Pass interp.WarningsWriter(io.Discard) so sandbox diagnostics no
  longer fall through to the runner's stderr.
- Add SandboxWarnings []string to RunCommandOutputs (json:"sandboxWarnings,omitempty")
  and populate it from runner.Warnings() in the response. nil/empty when
  the sandbox configuration is clean, so the field is omitted from the
  wire output.

The dd-source contract change adding the field to the action manifest
is at DataDog/dd-source#423305.

Closes DataDog/rshell#191.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
gh-worker-dd-mergequeue-cf854d Bot pushed a commit to DataDog/datadog-agent that referenced this pull request Apr 29, 2026
…50002)

### What does this PR do?

- Adds an optional `sandboxWarnings: string[]` field to `RunCommandOutputs` for the Private Action Runner `runCommand` action.
- Routes rshell sandbox diagnostics into that field instead of the action's `stderr`, by passing `interp.WarningsWriter(io.Discard)` and reading `runner.Warnings()` after `interp.New(...)`.

### Motivation

When `AllowedPaths` for `runCommand` contains a path that does not exist on the host (e.g. `/var/log` on Windows, `C:\var\log` on Linux), rshell emits a non-fatal `AllowedPaths: skipping ...` diagnostic during runner construction. Up to v0.0.13 the library flushed those diagnostics onto the runner's stderr writer, which the handler surfaced in `RunCommandOutputs.Stderr` — making every script execution look like a failure (non-empty stderr) even though `ExitCode == 0`.

[DataDog/rshell#200](DataDog/rshell#200) (in v0.0.14) decoupled sandbox diagnostics from the runner's stderr writer behind an opt-in `WarningsWriter` option and a `Runner.Warnings()` accessor. This PR adopts both.

Closes [DataDog/rshell#191](DataDog/rshell#191) on the agent side. The matching contract change in dd-source — additive `sandboxWarnings?: string[]` on `RunCommandOutputs` — is in [DataDog/dd-source#423305](DataDog/dd-source#423305).

### Describe how you validated your changes

- `go build ./pkg/privateactionrunner/...` — clean.
- `go test -count=1 ./pkg/privateactionrunner/bundles/remoteaction/rshell/...` — all tests pass, including two new ones:
  - `TestRunCommandSandboxWarningsKeepStderrClean`: configures one valid + one missing `AllowedPaths` entry, runs `echo hello`, asserts `Stderr` is empty (no sandbox diagnostic leakage), and `SandboxWarnings` carries one `AllowedPaths: skipping ...` entry naming the missing path.
  - `TestRunCommandSandboxWarningsNilWhenCleanConfig`: a clean configuration must produce `nil` `SandboxWarnings` so the field is omitted from the JSON wire output.

### Additional Notes

- `SandboxWarnings` uses `json:"sandboxWarnings,omitempty"`. Old PAR consumers that don't know about the field continue to deserialize the response unchanged.
- `interp.WarningsWriter(io.Discard)` is the right choice here because we read messages back via the accessor — there's no value in also writing them to the runner's stderr buffer.

🤖 Generated with [Claude Code](https://claude.com/claude-code)

Co-authored-by: matthew.deguzman <matthew.deguzman@datadoghq.com>
gh-worker-dd-mergequeue-cf854d Bot pushed a commit to DataDog/datadog-agent that referenced this pull request Apr 29, 2026
) (#50110)

Backport of #49958 to `7.79.x`.

### What does this PR do?

- Bumps `github.com/DataDog/rshell` from `v0.0.13` to `v0.0.14`. No call-site changes.

### Motivation

- v0.0.13..v0.0.14 contains a single commit, [DataDog/rshell#200](DataDog/rshell#200), which adds an opt-in `interp.WarningsWriter(io.Writer)` option and a `Runner.Warnings() []string` accessor. Today's PAR rshell handler does not pass either, so behaviour is byte-for-byte unchanged: sandbox warnings continue to fall back to `r.stderr`, which is where the handler already routes them.
- Backporting keeps `7.79.x` aligned with `main` so the follow-up PR wiring `runner.Warnings()` into `RunCommandOutputs.SandboxWarnings` can be cherry-picked cleanly without `go.mod`/`go.sum` noise.

### Describe how you validated your changes

- Cherry-picked `e083f5b19df2121690b8f9924fae312468bc4cb1` from `main`. Conflict in `go.sum` (expected — branch had drifted on unrelated deps); resolved by keeping the `7.79.x` baseline and running `dda inv tidy`, which reconciled it to swap only the `DataDog/rshell` entries from v0.0.13 → v0.0.14.
- `git diff` confirms only `go.mod`, `go.sum` (rshell entries only), and the new release note changed.

### Additional Notes

- Original PR: #49958 (merge commit `e083f5b19df2121690b8f9924fae312468bc4cb1`).

🤖 Generated with [Claude Code](https://claude.com/claude-code)

Co-authored-by: matthew.deguzman <matthew.deguzman@datadoghq.com>
gh-worker-dd-mergequeue-cf854d Bot pushed a commit to DataDog/datadog-agent that referenced this pull request Apr 29, 2026
…ated output field (#50100)

Backport a55c3db from #50002.

 ___

### What does this PR do?

- Adds an optional `sandboxWarnings: string[]` field to `RunCommandOutputs` for the Private Action Runner `runCommand` action.
- Routes rshell sandbox diagnostics into that field instead of the action's `stderr`, by passing `interp.WarningsWriter(io.Discard)` and reading `runner.Warnings()` after `interp.New(...)`.

### Motivation

When `AllowedPaths` for `runCommand` contains a path that does not exist on the host (e.g. `/var/log` on Windows, `C:\var\log` on Linux), rshell emits a non-fatal `AllowedPaths: skipping ...` diagnostic during runner construction. Up to v0.0.13 the library flushed those diagnostics onto the runner's stderr writer, which the handler surfaced in `RunCommandOutputs.Stderr` — making every script execution look like a failure (non-empty stderr) even though `ExitCode == 0`.

[DataDog/rshell#200](DataDog/rshell#200) (in v0.0.14) decoupled sandbox diagnostics from the runner's stderr writer behind an opt-in `WarningsWriter` option and a `Runner.Warnings()` accessor. This PR adopts both.

Closes [DataDog/rshell#191](DataDog/rshell#191) on the agent side. The matching contract change in dd-source — additive `sandboxWarnings?: string[]` on `RunCommandOutputs` — is in [DataDog/dd-source#423305](DataDog/dd-source#423305).

### Describe how you validated your changes

- `go build ./pkg/privateactionrunner/...` — clean.
- `go test -count=1 ./pkg/privateactionrunner/bundles/remoteaction/rshell/...` — all tests pass, including two new ones:
  - `TestRunCommandSandboxWarningsKeepStderrClean`: configures one valid + one missing `AllowedPaths` entry, runs `echo hello`, asserts `Stderr` is empty (no sandbox diagnostic leakage), and `SandboxWarnings` carries one `AllowedPaths: skipping ...` entry naming the missing path.
  - `TestRunCommandSandboxWarningsNilWhenCleanConfig`: a clean configuration must produce `nil` `SandboxWarnings` so the field is omitted from the JSON wire output.

### Additional Notes

- `SandboxWarnings` uses `json:"sandboxWarnings,omitempty"`. Old PAR consumers that don't know about the field continue to deserialize the response unchanged.
- `interp.WarningsWriter(io.Discard)` is the right choice here because we read messages back via the accessor — there's no value in also writing them to the runner's stderr buffer.

🤖 Generated with [Claude Code](https://claude.com/claude-code)

Co-authored-by: spencer.gilbert <spencer.gilbert@datadoghq.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants