Targeted custom-lint cleanup: append byte conversion, context propagation, error wrapping, and param-count reduction#43938
Conversation
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
This PR performs a targeted lint-driven cleanup across a few workflow/compiler/CLI/parser helpers, focusing on byte-slice appends, context propagation in sleeps, error wrapping semantics, and reducing helper parameter counts without changing behavior.
Changes:
- Simplified
[]byteappendusage by appending strings directly in action-cache JSON serialization. - Improved context propagation in rate-limit sleeping by relying on the passed
context.Context(while preserving nil-context behavior). - Adjusted frontmatter file-read error handling to satisfy wrapping requirements without exposing
os.PathErrorvia unwrapping. - Replaced a large helper parameter list with a small params struct for remote symlink component resolution.
Show a summary per file
| File | Description |
|---|---|
| pkg/workflow/action_cache.go | Removes unnecessary []byte(...) conversions when appending string fragments to []byte buffers. |
| pkg/cli/logs_rate_limit.go | Uses the provided context for sleep cancellation handling and preserves nil-context behavior. |
| pkg/workflow/compiler_orchestrator_frontmatter.go | Wraps file-read failures using %w while avoiding returning os.PathError in the unwrap chain. |
| pkg/parser/remote_download_file.go | Introduces a params struct to reduce helper parameter count without changing symlink resolution logic. |
Review details
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 4/4 changed files
- Comments generated: 1
- Review effort level: Low
| @@ -77,12 +84,15 @@ func fetchRateLimit() (rateLimitResource, error) { | |||
| // If ctx is cancelled before the timer expires, it stops the timer and returns | |||
| // ctx.Err() so callers can propagate cancellation immediately. | |||
🤖 PR Triage — §28848998303
New PR — Targeted lint cleanup: append byte conversion, context propagation, error wrapping (4 files, +60/-27). Non-functional, low-risk. Review with #43939.
|
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. No ADR enforcement needed: PR #43938 does not have the 'implementation' label and has ≤100 new lines of code in business logic directories (60 additions detected, threshold is 100). |
|
✅ Test Quality Sentinel completed test quality analysis. No test files were added or modified in this PR. Test Quality Sentinel skipped. |
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅ |
|
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /codebase-design and /tdd — the changes are clean and low-risk; three minor observations are worth a look before merge.
📋 Key Themes & Highlights
Key Themes
- Stale test description (
logs_rate_limit.go):contextCauseand the nil-channel approach are correct, but the test assertion string still says the old behaviour (nil
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · 72.8 AIC · ⌖ 7.7 AIC · ⊞ 6.6K
Comment /matt to run again
| // Intentionally not wrapping to avoid exposing internal path details | ||
| return nil, fmt.Errorf("failed to read file: %v", err) //nolint:errorlint // intentionally not wrapping to avoid exposing os.PathError | ||
| // Keep the user-facing message while avoiding exposure of os.PathError internals. | ||
| return nil, fmt.Errorf("failed to read file: %w", frontmatterReadError{message: err.Error()}) |
There was a problem hiding this comment.
[/codebase-design] `frontmatterReadError` is a value type (not a pointer), has no exported fields, and is not accessible outside this package — so callers can never do `errors.As(err, &frontmatterReadError{})` to discriminate read failures from parse failures. The wrapping via `%w` satisfies the linter, but the wrapped sentinel adds no practical information over the raw error string.
💡 Alternative to consider
If the goal is lint compliance without an intermediate sentinel, a simple fmt.Errorf around the message string already breaks the os.PathError chain:
return nil, fmt.Errorf("failed to read file: %s", err.Error())If frontmatterReadError is intended to be testable via errors.As, it needs to be exported and the test in TestParseFrontmatterSection_FileReadError should assert on it. As-is, the type is unreachable by callers.
@copilot please address this.
There was a problem hiding this comment.
Review: Targeted custom-lint cleanup
Clean, well-scoped set of low-risk fixes. Two non-blocking observations below; no blocking issues.
Change-by-change notes
pkg/workflow/action_cache.go — append byte conversion
All []byte("literal") → bare string literal in append calls. Functionally identical; the compiler handles the conversion. ✅
pkg/parser/remote_download_file.go — param-count reduction
The remoteSymlinkComponentParams struct is clean. Field names match the original parameters exactly. ctx and client stay as positional args (correct — they're cross-cutting infra concerns, not domain data). ✅
pkg/cli/logs_rate_limit.go — context propagation
Using a nil-safe done channel is the idiomatic Go pattern for optional cancellation. The contextCause helper is correct. One observation: the doc comment on sleepWithContext still says "returns ctx.Err()" but the function now returns context.Cause(ctx), which is a behavioural difference for WithCancelCause callers. See inline comment.
pkg/workflow/compiler_orchestrator_frontmatter.go — error wrapping
frontmatterReadError wrapper preserves the user-facing message while blocking errors.As from surfacing *os.PathError to callers — the intent is correct. One observation: frontmatterReadError has no Unwrap(), which is intentional but could confuse a future maintainer. See inline comment.
🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · 82.5 AIC · ⌖ 5.97 AIC · ⊞ 4.8K
| // Intentionally not wrapping to avoid exposing internal path details | ||
| return nil, fmt.Errorf("failed to read file: %v", err) //nolint:errorlint // intentionally not wrapping to avoid exposing os.PathError | ||
| // Keep the user-facing message while avoiding exposure of os.PathError internals. | ||
| return nil, fmt.Errorf("failed to read file: %w", frontmatterReadError{message: err.Error()}) |
There was a problem hiding this comment.
The %w wrapping of frontmatterReadError means callers can now do errors.As(err, &frontmatterReadError{}) — but frontmatterReadError doesn't implement Unwrap(), so errors.Is/errors.As traversal stops at the outer fmt.Errorf wrapper and won't reach the underlying *os.PathError.
That's the intended behaviour (hiding os.PathError), but it's subtle. Consider adding an explicit // Unwrap is intentionally not implemented comment inside frontmatterReadError, or at minimum a comment on this line noting that the chain stops here. Without it a future maintainer may add Unwrap() thinking it's an oversight.
@copilot please address this.
|
@copilot merge main and recompile |
…targeted-cleanup-again Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Done. Merged main ( |
|
@copilot run pr-finisher skill |
|
@copilot please run the
|
PR finisher complete. All conditions checked:
|
Branch is up to date with main (merge commit |
🤖 PR Triage · Run §28868091227
Score breakdown: Impact 15 · Urgency 10 · Quality 15 Rationale: Non-functional lint cleanup (+62/-28, 4 files) — append conversion, context propagation, error wrapping, param-count. CI success (but unstable merge state). Labels unchanged from prior run. Batch with other lint PRs when available. Labels:
|
|
🛠️ Agentic Maintenance updated this pull request branch. |
|
@copilot please run the
|
Branch is up to date with main. The CI failure in run 28878619289 is in the PR Sous Chef workflow's |
|
@copilot please run the
|
This PR addresses the focused lint-monster cleanup set for four files, excluding the broader function-length and process-environment backlogs. It removes low-risk non-functional lint findings around append usage, context handling, error wrapping semantics, and excessive function parameters.
Action cache serialization cleanup (
pkg/workflow/action_cache.go)[]byte(...)conversions inappendcalls when appending string literals/fragments to[]bytebuffers.Rate-limit context propagation (
pkg/cli/logs_rate_limit.go)context.Contextinstead of defaulting tocontext.Background().Error wrapping semantics (
pkg/workflow/compiler_orchestrator_frontmatter.go)%wto satisfy wrapping requirements.os.PathErrordetails through the chain.Parameter-count reduction (
pkg/parser/remote_download_file.go)remoteSymlinkComponentParams) forresolveRemoteSymlinkComponent, without changing resolution logic.