fix: use io.Discard instead of os.Stderr as panic fallback in interp#96
Merged
gh-worker-dd-mergequeue-cf854d[bot] merged 3 commits intomainfrom Mar 16, 2026
Merged
Conversation
PRs #78 and #85 introduced panic recovery in interp/api.go and interp/runner_exec.go that use runtime/debug.Stack() and os.Stderr, but those symbols were not added to interpAllowedSymbols, causing TestInterpAllowedSymbols and TestVerificationInterpCleanPass to fail on all platforms. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Collaborator
Author
|
@codex review |
|
Codex Review: Didn't find any major issues. You're on a roll. ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
julesmcrt
requested review from
AlexandreYang,
astuyve,
matt-dz and
thieman
as code owners
Drop the debug.Stack() call from the panic recovery handlers in api.go and runner_exec.go — emitting a raw stack trace leaks internal implementation details and contradicts the sanitization goal of #85. The panic value alone is sufficient for diagnosis. This also removes the need to add runtime/debug and runtime/debug.Stack to the interp allowlist; only os.Stderr remains as a new entry. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
AlexandreYang
approved these changes
Mar 16, 2026
AlexandreYang
approved these changes
Mar 16, 2026
When the runner has no stderr configured, the panic recovery handlers were falling back to os.Stderr, causing rshell to write directly to the process stderr — undesirable for embedders like datadog-agent. Replace the fallback with io.Discard so the message is silently dropped; the caller still receives "internal error" via the returned error. This also removes the need to add os.Stderr to the interp allowlist, leaving it unchanged. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Collaborator
Author
|
@codex review |
|
Codex Review: Didn't find any major issues. Already looking forward to the next diff. ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
julesmcrt
changed the title
AlexandreYang
approved these changes
Mar 16, 2026
gh-worker-dd-devflow-36fce6
bot
added
mergequeue-status: queued
mergequeue-status: in_progress
and removed
mergequeue-status: queued
labels
gh-worker-dd-mergequeue-cf854d
bot
deleted the
worktree-fix-allowedsymbols
branch
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
2 participants
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.
Summary
interp/api.goandinterp/runner_exec.go(added in security: sanitize panic recovery to avoid leaking internal state #85) fell back toos.Stderrwhen the runner had no stderr configuredio.Discard— the caller still receives"internal error"via the returned errorruntime/debug.Stack()call added in security: sanitize panic recovery to avoid leaking internal state #85, which would have leaked internal implementation detailsTest plan
TestInterpAllowedSymbolspasses (no new allowlist entries needed)TestVerificationInterpCleanPasspassesallowedsymbolstest suite passes🤖 Generated with Claude Code