fix(#1230): run OutputPipeline on post-review before posting to forge - #2444
Conversation
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. |
Site previewPreview: https://88f76682-site.fullsend-ai.workers.dev Commit: |
|
🤖 Finished Review · ✅ Success · Started 8:01 PM UTC · Completed 8:12 PM UTC |
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
|
Looks good to me Previous runReviewFindingsLow
Security coverage verification: All five free-text Previous run (2)ReviewFindingsLow
Info
Security coverage verification: All five free-text Previous runReviewFindingsMedium
Low
Labels: PR fixes a security gap in the post-review CLI command Previous run (3)ReviewFindingsLow
Info
Security coverage verification: All five free-text Previous run (4)ReviewFindingsMedium
Low
Labels: PR fixes a security gap in the post-review CLI command |
|
/fs-fix |
|
🤖 Finished Fix · ❌ Failure · Started 7:55 PM UTC · Completed 8:02 PM UTC |
6deedbe to
fe4e8ea
Compare
|
🤖 Review · |
|
🤖 Finished Fix · ✅ Success · Started 8:10 PM UTC · Completed 8:17 PM UTC |
…message Address two review findings on PR #2444: 1. [incomplete-sanitization] Apply pipeline.Scan() to Severity and Category fields in ReviewFinding, which are interpolated into Markdown posted to the forge by formatFindingComment. 2. [misleading-log-message] Change warning from "Redacted N secret(s)" to "Sanitized review body (N finding(s))" since pipeline findings include unicode normalization, not just secrets. Addresses review feedback on #2444
🔧 Fix agent — iteration 1 (human-triggered)Addressed both review findings: (1) added Severity and Category field sanitization in sanitizeReviewResult to close the incomplete-sanitization gap, and (2) fixed the misleading log message to not claim secret redaction when findings may be unicode normalization. Fixed (2):
Tests: passed Updated by fullsend fix agent |
|
🤖 Finished Review · ✅ Success · Started 8:21 PM UTC · Completed 8:32 PM UTC |
|
|
||
| // Sanitize the main body. | ||
| if r.Body != "" { | ||
| result := pipeline.Scan(r.Body) |
There was a problem hiding this comment.
[low] logging-pattern
The warning log for body sanitization emits a summary count but does not log per-finding details (scanner name and detail). The established pattern in scan.go:194-196 and run.go:1831 logs each finding individually after the summary line, which helps operators diagnose what was sanitized.
| result := pipeline.Scan(r.Body) | ||
| if result.Sanitized != "" { | ||
| r.Body = result.Sanitized | ||
| printer.StepWarn(fmt.Sprintf("Sanitized review body (%d finding(s))", len(result.Findings))) |
There was a problem hiding this comment.
[low] repetitive-pattern
The scan-check-assign pattern is repeated identically for each of the five finding fields (Severity, Category, Description, Remediation, and Body). Extracting a sanitizeField(pipeline, field) helper would reduce repetition and make it easier to add fields in the future.
|
/fs-fix Fix finding "[logging-pattern] internal/cli/postreview.go:546" |
|
🤖 Finished Fix · ✅ Success · Started 8:57 PM UTC · Completed 9:04 PM UTC |
|
🤖 Finished Review · ✅ Success · Started 9:15 PM UTC · Completed 9:26 PM UTC |
| printer.StepWarn(fmt.Sprintf(" %s: %s", f.Name, f.Detail)) | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
[low] logging-gap
When a secret is redacted from a finding field (Severity, Category, Description, or Remediation), the sanitization is applied silently with no log output. The body sanitization path logs via printer.StepWarn but the per-finding-field loop has no logging. Logging condition should check len(result.Findings) > 0 rather than result.Sanitized != empty.
| printer.StepWarn(fmt.Sprintf(" %s: %s", f.Name, f.Detail)) | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
[low] repetitive-pattern
The scan-check-assign pattern is repeated identically for four finding fields. Extracting a sanitizeField helper would reduce duplication and make it harder to forget logging when new fields are added.
| // sanitizeReviewResult runs the security output pipeline over all | ||
| // user-visible text fields in a ReviewResult. This catches leaked | ||
| // secrets and zero-width–obfuscated tokens before they reach the | ||
| // forge API. |
There was a problem hiding this comment.
[low] data-exposure
sanitizeReviewResult does not sanitize the File field of ReviewFinding. While File is used as a structured GitHub API path parameter and the API would reject invalid paths, sanitizing it would complete the defense-in-depth boundary.
The post-review command posted review content directly to the GitHub API without running it through the security output pipeline. When invoked standalone (outside fullsend run), secrets and zero-width- obfuscated tokens in agent output could reach the forge unredacted. Call security.OutputPipeline().Scan() on the review body and finding text fields (description, remediation) before any forge API call. This matches the pattern used by fullsend scan output and the sandbox post-tool hooks. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…message Address two review findings on PR #2444: 1. [incomplete-sanitization] Apply pipeline.Scan() to Severity and Category fields in ReviewFinding, which are interpolated into Markdown posted to the forge by formatFindingComment. 2. [misleading-log-message] Change warning from "Redacted N secret(s)" to "Sanitized review body (N finding(s))" since pipeline findings include unicode normalization, not just secrets. Addresses review feedback on #2444
Add individual finding logging (scanner name + detail) after the summary count line in sanitizeReviewResult, matching the established pattern in scan.go:194-196 and run.go:1831. Addresses review feedback on #2444
4cb8250 to
5dc2d8f
Compare
|
🤖 Finished Review · ✅ Success · Started 7:09 PM UTC · Completed 7:17 PM UTC |
|
🤖 Finished Retro · ✅ Success · Started 8:04 PM UTC · Completed 8:17 PM UTC |
Retro: PR #2444 —
|
| Dimension | Rating | Notes |
|---|---|---|
| Review quality | Mixed | Correctly caught the medium incomplete-sanitization bug, but exhibited a "finding treadmill" — surfacing new low-severity findings in each subsequent iteration that existed in the original code. |
| Rework rate | High | 2 fix cycles for findings the first review should have caught. |
| Token cost | Elevated | 4 review runs + 2 fix runs + associated CI for what was a straightforward 59-line security patch. |
| Time to resolution | 5 days | Mostly human latency (PR created June 18, first /fs-fix on June 22). Agent turnaround was fast. |
Key observations
-
Finding treadmill: The review agent found [logging-pattern] and [repetitive-pattern] in iteration 2, and [data-exposure] in iteration 3 — all low-severity findings that existed in the code from commit 1. The review agent inspected the same function in iteration 1 (it found the related Severity/Category gap) but failed to surface these findings until later passes.
-
Marginal findings: The [data-exposure] finding on the
Filefield was flagged despite the review agent's own acknowledgment that "File is a structured GitHub API path parameter and the API would reject invalid paths." The [repetitive-pattern] finding (extract a helper) is a style suggestion, not a correctness issue. -
Triage guidance was incomplete: The triage agent recommended sanitizing "the review body and inline comment bodies" but didn't enumerate all fields in the
ReviewFindingstruct. The code agent followed this guidance faithfully but missed fields the triage didn't mention.
Proposals
No new proposals filed. All improvement areas identified in this retro are well-covered by existing open issues:
- Finding treadmill / first-pass completeness: #1367 (surfaces new low findings across passes), #1582 (catch all findings in first pass)
- Low-severity findings driving rework: #2029 (use COMMENT verdict for low-only re-reviews), #1368 (approval with residual findings drives rework)
- Suppressing non-actionable findings: #1881 (suppress findings the agent deems non-actionable)
- Code agent field coverage: #1214 (analyze all usage sites), #2008 (validate against triage acceptance criteria)
- Review iteration deduplication: #1013, #1212 (both recently closed — fixes may be in flight)
This PR is a good test case for validating whether progress on these issues reduces churn. The next time a similar sanitization PR goes through the pipeline, we should see fewer review iterations if #1367 and #1582 are addressed.
…eading log message Address two review findings on PR fullsend-ai#2444: 1. [incomplete-sanitization] Apply pipeline.Scan() to Severity and Category fields in ReviewFinding, which are interpolated into Markdown posted to the forge by formatFindingComment. 2. [misleading-log-message] Change warning from "Redacted N secret(s)" to "Sanitized review body (N finding(s))" since pipeline findings include unicode normalization, not just secrets. Addresses review feedback on fullsend-ai#2444
Add individual finding logging (scanner name + detail) after the summary count line in sanitizeReviewResult, matching the established pattern in scan.go:194-196 and run.go:1831. Addresses review feedback on fullsend-ai#2444
…ize-post-review fix(fullsend-ai#1230): run OutputPipeline on post-review before posting to forge
The post-review command posted review content directly to the GitHub API without running it through the security output pipeline. When invoked standalone (outside fullsend run), secrets and zero-width- obfuscated tokens in agent output could reach the forge unredacted.
Call security.OutputPipeline().Scan() on the review body and finding text fields (description, remediation) before any forge API call. This matches the pattern used by fullsend scan output and the sandbox post-tool hooks.
Co-Authored-By: Claude Opus 4.6 noreply@anthropic.com
Closes #1230
Post-script verification
agent/1230-sanitize-post-review)59159d05e2cb01f868054a4c7143303d6049214b..HEAD)