Refactor pre-agent audit error extraction into focused helpers (largefunc backlog slice) - #47856
Conversation
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
|
✅ PR Code Quality Reviewer completed the code quality review. |
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅ |
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. |
There was a problem hiding this comment.
Pull request overview
Refactors pre-agent audit error extraction into focused helpers while preserving fallback priority.
Changes:
- Separates flat and nested workflow-log scanning.
- Extracts annotation, fallback, and agent-stdio classification helpers.
- Introduces one behavior discrepancy on workflow-log read failure.
Show a summary per file
| File | Description |
|---|---|
pkg/cli/audit_report.go |
Decomposes audit log parsing and fallback selection. |
Review details
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 1/1 changed files
- Comments generated: 1
- Review effort level: Medium
| jobDirs, err := os.ReadDir(workflowLogsDir) | ||
| if err != nil { | ||
| return nil | ||
| return nil, nil |
|
✅ Test Quality Sentinel completed test quality analysis. No test files were added or modified in this PR. Test Quality Sentinel skipped. |
There was a problem hiding this comment.
Clean refactor — helper extraction is faithful, decision order preserved, and the call sites in extractPreAgentStepErrors are now much easier to follow. Minor: extractLastStepFallbackError drops the workflowLogsDir path from its log message — worth restoring for debuggability, but not blocking.
🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · sonnet46 · 18.6 AIC · ⌖ 5.12 AIC · ⊞ 5K
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /codebase-design — two targeted suggestions, no blocking issues.
📋 Key Themes & Highlights
Key Themes
- Boolean trap in
appendErrorAnnotation: theflat boolparameter exists solely to select a log message format string. Replacing it with alogLabel stringwould make call sites self-documenting. - Lost log context:
extractLastStepFallbackErrordropped theworkflowLogsDirpath from the fallback log message — minor but useful for diagnosing missing logs in production.
Positive Highlights
- ✅ Clean single-responsibility decomposition — each helper does exactly one thing
- ✅ Decision order (annotations → agent-stdio → last-step fallback) is crystal-clear at the top of
extractPreAgentStepErrors - ✅
updateLastStepandextractGHErrorLinesare now independently unit-testable - ✅
classifyAgentStdioLinesextraction makesextractAgentStdioFailureExcerpteasier to follow - ✅ Existing integration tests through
extractPreAgentStepErrorsstill cover the full pipeline
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · sonnet46 · 32.6 AIC · ⌖ 4.72 AIC · ⊞ 6.7K
Comment /matt to run again
| filePath, stepKey string, | ||
| num int, | ||
| maxMessageLen int, | ||
| flat bool, |
There was a problem hiding this comment.
[/codebase-design] The flat bool parameter is a boolean trap — call sites must know what true means, and the only difference is the log message wording.
💡 Suggestion: replace the flag with a log label string
func appendErrorAnnotation(
errorAnnotations []ErrorInfo,
filePath, stepKey string,
num int,
maxMessageLen int,
logLabel string, // "flat job log" or "step"
) []ErrorInfo {
...
auditReportLog.Printf("Extracted ##[error] annotations from %s %s (%s %d)", logLabel, stepKey, logLabel, num)This makes both call sites self-documenting without a boolean whose meaning must be reverse-engineered.
@copilot please address this.
| func extractLastStepFallbackError(lastStep *stepLog, maxMessageLen int) []ErrorInfo { | ||
| if lastStep == nil { | ||
| auditReportLog.Printf("No step log files found in %s", workflowLogsDir) | ||
| auditReportLog.Printf("No step log files found for fallback extraction") |
There was a problem hiding this comment.
[/codebase-design] The log message "No step log files found for fallback extraction" lost the directory path from the original message. This makes diagnosing missing logs harder in production.
The original code logged "No step log files found in %s", workflowLogsDir — the path was useful context. Since extractLastStepFallbackError no longer has access to workflowLogsDir, consider passing it as a parameter or logging it in the caller.
@copilot please address this.
There was a problem hiding this comment.
Non-blocking observations
The refactor correctly preserves the three-tier decision order (##[error] annotations → agent-stdio fallback → last-step content) and the structural decomposition is a clear improvement in readability. Two issues worth fixing before the next caller is added:
Findings
appendErrorAnnotationflat bool— medium. The flag bleeds log-formatting policy into a data helper. Drop it and let call sites log their own context.extractLastStepFallbackErrorlost path in log — low. TheworkflowLogsDirpath context was present in the originalnillog message; it is now gone, making the message less useful to operators. Pass or log the directory.
🔎 Code quality review by PR Code Quality Reviewer · sonnet46 · 43.9 AIC · ⌖ 4.74 AIC · ⊞ 5.7K
Comment /review to run again
Comments that could not be inline-anchored
pkg/cli/audit_report.go:141
appendErrorAnnotation's flat bool parameter leaks a formatting concern into a generic helper: the flag exists solely to alter a log message prefix. A future call site that passes the wrong value silently produces misleading operator logs.
<details>
<summary>💡 Suggested fix</summary>
Drop the flat parameter and let each call site log its own context:
// scanFlatStepLog — before calling helper:
auditReportLog.Printf("Extracted ##[error] annotations from flat job log %s (job %d…
</details>
<details><summary>pkg/cli/audit_report.go:318</summary>
**`extractLastStepFallbackError` nil-path log message lost the directory context**: the original logged `workflowLogsDir` to aid debugging; the refactored version emits a generic message with no path information.
<details>
<summary>💡 Suggested fix</summary>
Pass the path to the function or log it at the call site:
```go
func extractLastStepFallbackError(lastStep *stepLog, workflowLogsDir string, maxMessageLen int) []ErrorInfo {
if lastStep == nil {
auditReportLog.Printf("No step…
</details>Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
🏗️ Design Decision Gate — ADR RequiredThis PR makes significant changes to core business logic (167 new lines in 📄 Draft ADR committed:
📋 What to do next
Once an ADR is linked in the PR body, this gate will re-run and verify the implementation matches the decision. ❓ Why ADRs Matter
ADRs create a searchable, permanent record of why the codebase looks the way it does. Future contributors (and your future self) will thank you. 📋 Michael Nygard ADR Format ReferenceAn ADR must contain these four sections to be considered complete:
All ADRs are stored in
|
|
@copilot run pr-finisher |
|
Hey One item that would strengthen this before merge:
If you would like a hand adding these tests, you can assign this prompt to your coding agent:
|
|
@copilot please run the
|
Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
Addressed the remaining review feedback in 927a10f. The branch is up to date with |
|
@copilot please run the skill, confirm the branch is up to date, address the remaining review observations if they still apply, and rerun or retrigger checks on the current HEAD. The latest Copilot response says the branch is ready but CI is stale after the agent push; please make sure fresh checks run for this commit.
|
|
Triage | Category: Recommended action: Score breakdown: impact=18 (code quality, maintainability), urgency=12 (CI passing, not draft), quality=15 (CI green, ADR, tests)
|
|
🎉 This pull request is included in a new release. Release: |
This PR addresses one targeted slice of the shared
largefuncbacklog inpkg/cli/pkg/workflowby decomposing a long audit-report function without changing behavior. The focus here is pre-agent failure extraction inpkg/cli, where log parsing, fallback selection, and annotation handling were previously bundled in one large function.Scope:
pkg/clilargefunc sliceextractPreAgentStepErrorsinpkg/cli/audit_report.gointo smaller, single-purpose helpers.##[error]annotations, then agent-stdio fallback (when agent ran), then last-step fallback.Log-structure handling extracted
workflow-logs/*.txtjob logsworkflow-logs/<job>/<step>.txtstep logsRelated micro-refactor in same path
extractAgentStdioFailureExcerptinto a dedicated helper to keep that function under limit and easier to read.