fix(exec): don't treat shell_exec() null return as an error#3723
Merged
fix(exec): don't treat shell_exec() null return as an error#3723
Conversation
PHP's shell_exec() returns null both when a command produces no output AND when execution fails — the two cases are indistinguishable. Flagging null as an error causes false-positive error spans for any intentionally silent command (e.g. Symfony's `stty 2>/dev/null` called internally by Terminal::hasSttyAvailable()), which is especially harmful when DD_APM_FEATURES=error_rare_sample_tracer_drop is set, since those spans cannot be sampled out. The fix from cff2034 (v1.8.0) added hooks on two specific Symfony methods to suppress their spans, but the root defect in postHookShell() remained. Any other shell_exec() call returning null — from a Symfony bundle, third-party package, or application code — still produced an error span. Per PHP docs: "It is not possible to detect execution failures using this function." Remove the null check entirely; the false return check above it already handles the case where PHP itself cannot invoke the shell. Adds a regression test asserting that shell_exec('true') — which returns null due to no output — produces a clean span with no error. Fixes: APMS-18967
Leiyks
force-pushed
the
leiyks/fix-shell-exec-false-positives
branch
from
67f32fe to
ba93b1d
Compare
|
✨ Fix all issues with BitsAI or with Cursor
|
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #3723 +/- ##
==========================================
- Coverage 68.81% 68.79% -0.02%
==========================================
Files 166 166
Lines 19030 19030
Branches 1797 1797
==========================================
- Hits 13095 13092 -3
- Misses 5121 5125 +4
+ Partials 814 813 -1
Flags with carried forward coverage won't be shown. Click here to find out more. Continue to review full report in Codecov by Sentry.
🚀 New features to boost your workflow:
|
Benchmarks [ tracer ]Benchmark execution time: 2026-03-25 12:53:14 Comparing candidate commit ba93b1d in PR branch Found 0 performance improvements and 2 performance regressions! Performance is the same for 192 metrics, 0 unstable metrics. scenario:MessagePackSerializationBench/benchMessagePackSerialization
scenario:MessagePackSerializationBench/benchMessagePackSerialization-opcache
|
bwoebi
approved these changes
Mar 25, 2026
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
3 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
shell_exec()returningnullwas flagged as an error span. Per PHP docs, null means either no output or failure — the two are indistinguishable. This caused false-positive error spans for any silent command (e.g. Symfony'sstty 2>/dev/null), which is especially disruptive withDD_APM_FEATURES=error_rare_sample_tracer_dropsince those spans can't be sampled away.A prior partial fix (
cff203464) hooked two specific Symfony methods, but left the root defect inpostHookShell()alive for all other callers.Changes
shell_exec() returned nullerror condition frompostHookShell()— thefalsereturn check above it covers genuine PHP-level failurestestShellExecNullReturnIsNotAnError: assertsshell_exec('true')(null return, no output) produces a clean span with no errorFixes: APMS-18967 / #3127