fix(#2903): show bootstrap scan finding details#2925
Closed
sjsjsjjs534 wants to merge 1 commit into
Closed
Conversation
Signed-off-by: zhangsixiao <zhangsixiao@bytedance.com>
E2E tests did not runE2E tests run automatically for org/repo members and collaborators on pull requests. For other contributors, a maintainer must add the See E2E testing guide for details. |
1 similar comment
E2E tests did not runE2E tests run automatically for org/repo members and collaborators on pull requests. For other contributors, a maintainer must add the See E2E testing guide for details. |
PR Summary by QodoShow bootstrap scan finding details in fail-open warnings
AI Description
Diagram
High-Level Assessment
Files changed (2)
|
Code Review by Qodo
1. Global stderr capture race
|
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
Comment on lines
+153
to
+172
| func captureStderr(t *testing.T, fn func()) string { | ||
| t.Helper() | ||
|
|
||
| oldStderr := os.Stderr | ||
| r, w, err := os.Pipe() | ||
| require.NoError(t, err) | ||
| os.Stderr = w | ||
| defer func() { | ||
| os.Stderr = oldStderr | ||
| }() | ||
|
|
||
| fn() | ||
|
|
||
| require.NoError(t, w.Close()) | ||
| var buf bytes.Buffer | ||
| _, err = io.Copy(&buf, r) | ||
| require.NoError(t, err) | ||
| require.NoError(t, r.Close()) | ||
| return buf.String() | ||
| } |
There was a problem hiding this comment.
1. Global stderr capture race 🐞 Bug ☼ Reliability
captureStderr replaces the process-global os.Stderr without synchronization, which can interleave with other tests (including t.Parallel tests in this package) and produce flaky captures or data races. It also doesn’t guarantee the pipe fds are closed if the callback exits early (e.g., FailNow/panic), leaking descriptors and leaving stderr redirected.
Agent Prompt
### Issue description
`captureStderr` mutates global `os.Stderr` and relies on non-deferred cleanup after `fn()` returns. This can flake when other tests run concurrently (package contains `t.Parallel()` tests) and can leak/leave redirected stderr if `fn()` exits early.
### Issue Context
This is test-only but can destabilize CI. The repo already has other tests that swap `os.Stderr`, so a shared, hardened helper is preferable.
### Fix Focus Areas
- internal/cli/bootstrap_scan_test.go[153-172]
### Implementation notes
- Add a package-level `var stderrMu sync.Mutex` and lock around the entire capture to serialize global `os.Stderr` swapping.
- Use `defer` to restore `os.Stderr` and close both `w` and `r` even if `fn()` aborts early.
- Restore `os.Stderr` immediately after closing `w` (before reading from `r`) to minimize the window where stderr points at a closed pipe.
- Optionally refactor other tests that swap `os.Stderr` to use the same helper to avoid future reintroductions.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
60
to
65
| fmt.Fprintf(os.Stderr, "WARNING: agent definition %q has critical injection findings (fail_mode: open)\n", agentPath) | ||
| printScanFindings(result.Findings) | ||
| } else if len(result.Findings) > 0 { | ||
| fmt.Fprintf(os.Stderr, "WARNING: agent definition %q has %d injection finding(s)\n", agentPath, len(result.Findings)) | ||
| printScanFindings(result.Findings) | ||
| } |
There was a problem hiding this comment.
2. Misleading injection warning label 🐞 Bug ◔ Observability
Bootstrap scan warnings/counts are phrased as “injection finding(s)”, but security.InputPipeline() also includes UnicodeNormalizer which emits non-injection findings (e.g., ansi_escape, null_byte). With printScanFindings(result.Findings) now printing all findings, users can see non-injection findings under an “injection” warning and the counts become misleading.
Agent Prompt
### Issue description
Bootstrap scan warnings say “injection finding(s)” but the pipeline returns findings from both the unicode normalizer and the injection scanner. After this PR, `printScanFindings` prints those mixed findings, making the warning label/count inaccurate.
### Issue Context
`InputPipeline()` runs `NewUnicodeNormalizer()` then `NewContextInjectionScanner()`. Unicode normalizer findings are security-relevant, but they aren’t “injection” findings.
### Fix Focus Areas
- internal/cli/bootstrap_scan.go[55-65]
- internal/cli/bootstrap_scan.go[83-93]
- internal/cli/bootstrap_scan.go[103-113]
### Implementation options
- Easiest: change wording everywhere from “injection finding(s)” to “security finding(s)”.
- Or: filter/count only `f.Scanner == "context_injection"` for “injection” wording, and optionally print grouped sections (e.g., “Unicode findings” vs “Injection findings”).
- Consider including `f.Scanner` in `printScanFindings` output to reduce ambiguity.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
|
🤖 Finished Retro · ✅ Success · Started 7:20 AM UTC · Completed 7:24 AM UTC |
|
PR #2925 was an external contributor PR that received only third-party bot review (qodo-code-review), no fullsend agent participation, no human review feedback, and was closed without merge. The retro dispatch was wasteful — there is no fullsend agent workflow to analyze. Existing issue #2942 already proposes the correct fix (skip retro when no fullsend agents participated). No new proposals are warranted; the single actionable improvement is already tracked. |
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
Fixes #2903
Tests
go test ./internal/cli -run TestScanRuntimeContent -count=1env TMPDIR=/private/var/folders/t6/2b3hch457vld0w_q_m84tp_w0000gn/T/ go test ./internal/cli -count=1go vet ./...pre-commit run