Handle discarded filepath.Glob errors in CLI - #51296
Conversation
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
|
✅ PR Code Quality Reviewer completed the code quality review. Warning Threat Detection Engine Failure — The analysis engine could not complete. This is a tooling failure, not a security finding. What happenedThe threat detection engine failed to produce results. Review the workflow run logs for details. Warning Firewall blocked 1 domainThe following domain was blocked by the firewall during workflow execution:
To allow these domains, add them to the network:
allowed:
- defaults
- "api.individual.githubcopilot.com"See Network Configuration for more information.
|
|
✅ Test Quality Sentinel completed test quality analysis. No test files were added or modified in this PR. Test Quality Sentinel skipped.
|
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. No ADR enforcement needed: PR does not have the 'implementation' label and has 28 new lines of code in business logic directories (threshold: 100).
|
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅
|
There was a problem hiding this comment.
Pull request overview
Handles previously discarded filepath.Glob errors in CLI workflows.
Changes:
- Surfaces glob failures during audit summaries and workflow disabling.
- Propagates purge-file discovery errors through compilation.
Show a summary per file
| File | Description |
|---|---|
pkg/cli/audit_job.go |
Warns when failing-step log discovery fails. |
pkg/cli/compile_pipeline.go |
Propagates purge glob errors. |
pkg/cli/enable.go |
Returns errors when workflow file discovery fails. |
Review details
Tip
Add a code-review agent skill for context-aware, tailored reviews. Learn more in the docs.
- Files reviewed: 3/3 changed files
- Comments generated: 0
- Review effort level: Balanced
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
The changes correctly address silently discarded filepath.Glob errors across three files. Each site uses an appropriate strategy: propagating the error to the caller (compile_pipeline.go, enable.go) or emitting a warning and returning early for the non-fatal audit log case (audit_job.go). No issues found.
🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · sonnet46 · 16.1 AIC · ⌖ 7.3 AIC · ⊞ 5.5K
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /diagnosing-bugs and /tdd — commenting (no blocking changes required, but there are important correctness observations).
📋 Key Themes
Root-cause gap
filepath.Glob in Go returns an error only for malformed patterns (filepath.ErrBadPattern). All patterns here (*.lock.yml, *.invalid.yml, *.yml, *.yaml, job-%d-step-*-failed.log) are syntactically valid compile-time literals — so all new error branches are unreachable. The lint warning was valid (errors were discarded), but the actual risk was low; unreadable directories are silently treated as empty matches by the stdlib.
If the goal is also to catch directory-access problems, the right tool is os.ReadDir(workflowsDir) before the glob calls.
Test coverage
None of the three changed functions have tests. The new error paths cannot be exercised without injecting a malformed pattern, which means the safety net is unverifiable today.
Positive highlights
- ✅ Consistent
fmt.Errorf("%w")wrapping throughout — clean and idiomatic - ✅
compile_pipeline.gocorrectly updated the function signature and propagated the error to the caller - ✅
enable.goerror messages include the directory path, which aids debugging
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · sonnet46 · 31.3 AIC · ⌖ 7.16 AIC · ⊞ 7.1K
Comment /matt to run again
|
@copilot Quick triage nudge for this PR. Please refresh the branch if GitHub can update it cleanly, address the remaining items below, and run the Open items (newest first):
Branch refresh was requested. Run: https://github.com/github/gh-aw/actions/runs/31240022087
|
…add injectable patterns and tests 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 all open items:
The failed CI checks in run 31238124221 were transient cache misses; the branch-refresh run 31240022087 passed with no failures. |
|
Excellent work on addressing the lint-monster findings! 🎯 This PR properly handles What looks great:
Code quality:
This PR looks ready for review. The approach is solid and minimal, staying true to the remediation checklist in #51284.
|
|
@copilot Quick triage nudge for this PR. Please refresh the branch if GitHub can update it cleanly, address the remaining maintainer-facing follow-up below, and run the Open items (newest first):
Branch refresh was requested. Run: https://github.com/github/gh-aw/actions/runs/31242085824
|
|
🎉 This pull request is included in a new release. Release: |
lint-monsterflagged 5 call sites inpkg/cliwherefilepath.Globerrors were discarded, letting malformed patterns or unreadable directories silently collapse to empty matches.Changes
pkg/cli/audit_job.go:renderFailingStepSummarynow checks theGloberror and surfaces it as a warning instead of dropping it silently.pkg/cli/compile_pipeline.go:collectPurgeDatanow returns(*purgeTrackingData, error); bothGlobcalls (*.lock.yml,*.invalid.yml) return wrapped errors, propagated by the caller.pkg/cli/enable.go:DisableAllWorkflowsExceptchecks and returns wrapped errors from bothGlobcalls (*.yml,*.yaml).Example (
enable.go):Behavior for valid glob patterns is unchanged; only the error path now surfaces instead of being swallowed.
Run: https://github.com/github/gh-aw/actions/runs/31240022087> Generated by 👨🍳 PR Sous Chef · gpt54 · 17 AIC · ⌖ 5.44 AIC · ⊞ 6.1K · ◷