Repository navigation
feat(#704): surface agent errors in workflow logs - #764
Conversation
When an agent run fails, the root cause error is buried in the transcript JSONL artifact. Operators must download and manually search the artifact to diagnose failures. This adds two improvements: 1. In-process error surfacing (run.go): After transcript extraction, parse the JSONL for result events with is_error=true and emit ::error:: annotations to stderr. These appear in the GitHub Actions job summary, making failures diagnosable without downloading artifacts. 2. Standalone extraction script (extract-transcript-error.sh): A shell script that extracts errors from transcript files or directories. Usable by post-scripts, the triage agent (after downloading artifacts via gh run download), and operators debugging failed runs. The transcript parser is intentionally lightweight: it scans only for result-type lines (fast string contains check) and parses the last one. Error messages are truncated to 2000 chars and sanitized to prevent GHA command injection. New files: - internal/cli/transcript.go: JSONL parsing and error emission - internal/cli/transcript_test.go: 15 unit tests - internal/scaffold/.../extract-transcript-error.sh: standalone tool Modified: - internal/cli/run.go: call extractTranscriptErrors after iteration loop when agent exits non-zero Closes #704 Signed-off-by: fullsend-code <fullsend-code@users.noreply.github.com>
|
fullsend review is working on this — view logs |
Site previewPreview: https://c7b29a71-site.fullsend-ai.workers.dev Commit: |
Review: #764Head SHA: 1a4e166 SummaryThis PR adds well-structured transcript error surfacing for agent failures, directly addressing issue #704. The Go implementation ( FindingsMediumNone. Low
Info
FooterOutcome: approve Previous runReview: #764Head SHA: b8e7569 SummaryClean, well-scoped change that adds transcript error surfacing to GitHub Actions workflow logs. The Go implementation is solid — proper use of FindingsMedium
Low
Info
FooterOutcome: approve |
![fullsend-ai-review[bot]](https://avatars.githubusercontent.com/in/3478501?s=60&v=4){kind=small}
ralphbean
left a comment
ralphbean
left a comment
There was a problem hiding this comment.
Well-scoped, useful feature that directly addresses #704. The Go implementation is solid overall — clean separation of concerns, good test coverage, and correct integration point in run.go. Five findings below, the most important being unsanitized values interpolated into GHA workflow commands (filename and subtype) and inconsistent sanitization in the shell script.
| // agent failures diagnosable without downloading artifacts. | ||
| func emitTranscriptErrors(w io.Writer, summaries []transcriptErrorSummary) { | ||
| for _, s := range summaries { | ||
| // Sanitize the error message to prevent GHA command injection. |
There was a problem hiding this comment.
[critical] s.Source (derived from the JSONL filename via filepath.Base) is interpolated directly into the ::error:: workflow command without sanitization. Only msg goes through sanitizeOutput. A crafted filename containing %0A or :: sequences could inject additional GHA commands.
The fallback on line 142 has the same issue with s.Subtype.
Fix:
fmt.Fprintf(w, "::error title=Agent Error (%s)::%s\n", sanitizeOutput(s.Source), msg)And for the fallback:
msg = fmt.Sprintf("agent terminated with error (subtype: %s)", sanitizeOutput(s.Subtype))|
|
||
| # Emit GHA annotation if running in GitHub Actions. | ||
| if [[ "${GITHUB_ACTIONS:-}" == "true" ]]; then | ||
| # Sanitize for GHA: replace :: with : : to prevent command injection. |
There was a problem hiding this comment.
[important] The sanitization here only strips :: but does not handle %0A/%0D (URL-encoded newlines) that GitHub Actions also interprets as line breaks in workflow commands. The Go sanitizeOutput function handles these correctly. An attacker-controlled transcript message containing %0A::warning::spoofed would inject additional annotations.
Fix — add after the existing :: replacement:
safe_msg="${safe_msg//%0A/ }"
safe_msg="${safe_msg//%0a/ }"
safe_msg="${safe_msg//%0D/ }"
safe_msg="${safe_msg//%0d/ }"| } | ||
|
|
||
| // truncateError trims an error message to maxTranscriptErrorLength. | ||
| // If truncated, appends an ellipsis indicator. |
There was a problem hiding this comment.
[moderate] truncateError uses byte length (len(msg)) and byte slicing (msg[:maxTranscriptErrorLength]). If the 2000-byte boundary falls in the middle of a multi-byte UTF-8 character (emoji, CJK, etc.), the result contains an incomplete rune.
Fix — walk back to a valid rune boundary, e.g.:
truncated := msg[:maxTranscriptErrorLength]
for len(truncated) > 0 && !utf8.Valid([]byte(truncated)) {
truncated = truncated[:len(truncated)-1]
}
return truncated + "… (truncated)"|
|
||
| // isResultLine does a fast prefix/contains check to avoid parsing every | ||
| // JSONL line. Claude Code transcripts can be very large. | ||
| func isResultLine(line []byte) bool { |
There was a problem hiding this comment.
[minor] isResultLine converts []byte to string via strings.Contains(string(line), ...), which allocates a new string (up to 1 MB per line, twice). Since this is meant to be a cheap pre-filter to avoid json.Unmarshal, use bytes.Contains instead to avoid the allocation:
func isResultLine(line []byte) bool {
return bytes.Contains(line, []byte(`"type":"result"`)) ||
bytes.Contains(line, []byte(`"type": "result"`))
}| // File 2: success result (should not appear in summaries). | ||
| ok2 := `{"type":"result","subtype":"success","is_error":false,"result":"all good"}` | ||
| if err := os.WriteFile(filepath.Join(dir, "agent2.jsonl"), []byte(ok2), 0o644); err != nil { | ||
| t.Fatal(err) |
There was a problem hiding this comment.
[nit] Typo: TestExtractTranscriptErrors_MultiplFiles → TestExtractTranscriptErrors_MultipleFiles (missing 'e' in 'Multiple').
|
/fix those inline comments, please |
|
fullsend fix is working on this — view logs |
- Sanitize s.Source and s.Subtype in emitTranscriptErrors to prevent GHA command injection via crafted filenames or subtypes [critical] - Add %0A/%0D sanitization to shell script's GHA annotation output to match the Go sanitizeOutput behavior [important] - Fix truncateError to walk back to valid UTF-8 rune boundary before truncating, avoiding incomplete multi-byte characters [moderate] - Switch isResultLine from strings.Contains(string(line)) to bytes.Contains to avoid unnecessary allocations [minor] - Fix typo: MultiplFiles → MultipleFiles in test name [nit] Addresses review feedback on #764 Signed-off-by: fullsend-fix <fullsend-code@users.noreply.github.com>
🔧 Fix agent — iteration 1 (human-triggered)Addressed all 5 inline review findings from ralphbean: sanitized Source/Subtype in GHA commands (critical), added %0A/%0D sanitization to shell script (important), fixed UTF-8 truncation (moderate), switched to bytes.Contains (minor), fixed test name typo (nit). All 14 transcript tests pass. Fixed (5):
Tests: passed Updated by fullsend fix agent |
|
fullsend review is working on this — view logs |
ralphbean
left a comment
ralphbean
left a comment
There was a problem hiding this comment.
LGTM. Three notes inline for a future pass.
| echo "::error title=Agent Error (${basename})::${safe_msg}" | ||
| fi | ||
| } | ||
|
|
There was a problem hiding this comment.
[important] Noted, deferring. Go fix added sanitizeOutput(s.Source) for the title, but this line still interpolates basename raw. Needs the same :: and %0A/%0D replacement.
| return | ||
| fi | ||
|
|
||
| # Check if the result indicates an error. |
There was a problem hiding this comment.
[moderate] Noted, deferring. Missing jq silently reports no errors (the || true guards swallow the failure). A command -v jq check at the top would fail fast.
| lastTranscriptDir := filepath.Join(lastIterDir, "transcripts") | ||
| if errorSummaries := extractTranscriptErrors(lastTranscriptDir); len(errorSummaries) > 0 { | ||
| printer.StepWarn(fmt.Sprintf("Found %d transcript error(s) — emitting to workflow log", len(errorSummaries))) | ||
| emitTranscriptErrors(os.Stderr, errorSummaries) |
There was a problem hiding this comment.
[moderate] Noted, deferring. Emits ::error:: unconditionally. Existing pattern at line 953 gates on isCI. Consider wrapping.
Add a `test` job to the CI workflow that runs `make test`, which exercises go-vet, go-test, script-test, and lint. This would have caught the TestFileModeMatchesFilesystem regression from PR #764. Note: `test` is not yet a required check — add it to branch protection rules to enforce it on PRs. Fixes #1024 Assisted-by: Claude Opus 4.6 <noreply@anthropic.com> Signed-off-by: Ralph Bean <rbean@redhat.com>
…ensions The review agent's security checklist listed generic injection types (SQL, command, LDAP, path traversal) but omitted GitHub Actions workflow command injection. This caused a false safety assertion on PR #764 where unsanitized variables in ::error title=...:: were missed. GHA workflow command injection is a distinct attack class where unsanitized values in ::error::, ::warning::, ::set-output::, etc. can inject arbitrary workflow commands. Updated two files to cover this gap: - code-review SKILL.md: Added GHA workflow command injection to the Security dimension's injection checklist with specific commands to audit, escape sequences to check, and a verification instruction requiring all interpolated variables to be checked individually. - pr-review security sub-agent: Added GHA workflow command injection to the owned injection types and added a dedicated paragraph with the same audit guidance. Note: make lint could not run due to Go module cache permission errors in the sandbox (infrastructure issue, not code-related). Changes are markdown-only and do not affect Go compilation. Closes #989
When an agent run fails, the root cause error is buried in the transcript JSONL artifact. Operators must download and manually search the artifact to diagnose failures.
This adds two improvements:
In-process error surfacing (run.go): After transcript extraction,
parse the JSONL for result events with is_error=true and emit
::error:: annotations to stderr. These appear in the GitHub
Actions job summary, making failures diagnosable without
downloading artifacts.
Standalone extraction script (extract-transcript-error.sh): A
shell script that extracts errors from transcript files or
directories. Usable by post-scripts, the triage agent (after
downloading artifacts via gh run download), and operators
debugging failed runs.
The transcript parser is intentionally lightweight: it scans only for result-type lines (fast string contains check) and parses the last one. Error messages are truncated to 2000 chars and sanitized to prevent GHA command injection.
New files:
Modified:
loop when agent exits non-zero
Closes #704
Post-script verification
agent/704-surface-transcript-errors)a97ff3b4fbc3f8942a369347ee2f47240737314f..HEAD)