fix(parser): wrap upstream errors with %w in import pipeline#46414
Conversation
…eline Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Audit the parser import pipeline for non-wrapping fmt.Errorf calls where a
genuine upstream error exists but was being discarded (swallowed), preventing
errors.Is/errors.As from inspecting the real cause.
Changes:
- remote_resolve_path.go: split filepath.Rel condition so that when
filepath.Rel itself errors (rare, Windows-only), the error is wrapped with
%w. The path-escape case (relativePath == "..") has no error to wrap so it
keeps its original format.
- remote_fetch_wasm.go: same split for the WASM build variant.
- github_urls.go: wrap strconv.ParseInt errors in parseNumberedURL and
parseRunURL, enabling errors.As(&strconv.NumError{}) by callers.
- schedule_parser.go: split the strconv.Atoi+interval<1 condition in
parseLongIntervalParts so that parse failures (non-integer input) wrap the
underlying NumError, while the value-range check (interval < 1) keeps its
original message.
os.Stat errors are intentionally NOT wrapped to prevent *os.PathError from
leaking into the user-facing error chain (enforced by TestUserFacingErrorsDontLeakInternals).
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Preserves upstream parser errors for errors.Is/errors.As traversal.
Changes:
- Wraps numeric parsing and path-resolution errors.
- Separates parsing failures from validation failures.
- Reschedules the generated auto-upgrade workflow.
Show a summary per file
| File | Description |
|---|---|
pkg/parser/schedule_parser.go |
Wraps interval parsing errors. |
pkg/parser/remote_resolve_path.go |
Wraps native path-resolution errors. |
pkg/parser/remote_fetch_wasm.go |
Mirrors path handling for WASM. |
pkg/parser/github_urls.go |
Wraps invalid GitHub numeric IDs. |
.github/workflows/agentic-auto-upgrade.yml |
Changes the weekly schedule. |
Review details
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 5/5 changed files
- Comments generated: 4
- Review effort level: Medium
| if relErr != nil { | ||
| allowedFolder := filepath.Base(normalizedSecurityBase) | ||
| remoteLog.Printf("Security: Path escapes allowed folder: %s (rel error: %v)", filePath, relErr) | ||
| return "", fmt.Errorf("security: path %s must be within %s folder: %w", filePath, allowedFolder, relErr) |
There was a problem hiding this comment.
Addressed in c9a688e. Replaced the bare %w wrapping with a pathRelError type (pkg/parser/path_rel_error.go). Its Error() returns only the sanitised message (no internal absolute paths exposed), while Unwrap() returns the underlying relErr for errors.Is/errors.As traversal. Both remote_resolve_path.go and remote_fetch_wasm.go now use &pathRelError{…}.
| number, err := strconv.ParseInt(numberText, 10, 32) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("invalid %s number: %s", label, numberText) | ||
| return nil, fmt.Errorf("invalid %s number %s: %w", label, numberText, err) |
There was a problem hiding this comment.
Addressed in c9a688e. Added TestParseGitHubURL_NumErrorWrapping in pkg/parser/github_urls_test.go with four table-driven cases covering numbered URLs (PR, issue), run URL, and job URL, each asserting errors.As(err, &(*strconv.NumError)(nil)) is reachable.
| if err != nil { | ||
| return 0, "", fmt.Errorf("invalid interval '%s', must be a positive integer: %w", intervalStr, err) | ||
| } | ||
| if interval < 1 { | ||
| return 0, "", fmt.Errorf("invalid interval '%s', must be a positive integer", intervalStr) |
There was a problem hiding this comment.
Addressed in c9a688e. Added TestParseLongIntervalParts_NumErrorWrapping in pkg/parser/schedule_parser_test.go with three subtests: non-numeric interval asserts errors.As reaches *strconv.NumError; zero and negative intervals (value-range branch, no upstream error) assert it does not. Uses assert.ErrorAs/assert.NotErrorAs per testifylint convention.
| on: | ||
| schedule: | ||
| - cron: "21 3 * * 5" # Weekly (auto-upgrade) | ||
| - cron: "11 4 * * 6" # Weekly (auto-upgrade) |
There was a problem hiding this comment.
Addressed in c9a688e. The cron schedule has been reverted to "21 3 * * 5" — the deterministic value generated by make recompile --schedule-seed github/gh-aw.
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅ |
|
✅ PR Code Quality Reviewer completed the code quality review. |
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. No ADR enforcement needed: PR #46414 does not have the 'implementation' label and has ≤100 new lines of code in business logic directories (20 additions detected). |
|
✅ Test Quality Sentinel completed test quality analysis. No test files were added or modified in this PR. PR #46414 only modified production code files (pkg/parser/*.go) and workflow configuration. Test Quality Sentinel skipped. |
![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 /diagnosing-bugs and /tdd — requesting changes on two themes: missing regression tests for the new %w contracts, and an unexplained cron schedule change bundled into a parser fix.
📋 Key Themes & Highlights
Key Themes
- Missing regression tests — the three wrapping sites (
github_urls.go,schedule_parser.go,remote_resolve_path.go) each introduce a newerrors.As-reachable cause, but no test asserts that the cause is actually reachable. Without these, the contract can silently regress. - Out-of-scope change — the cron reschedule in
agentic-auto-upgrade.ymlis unrelated to parser error wrapping and should be in a separate PR or explained in the description. - Error message content shift — wrapping
filepath.Relfailures now embeds OS-level path diagnostics (e.g. Windows cross-drive paths) in user-facing import errors. This is probably intentional but worth a short note in the PR description.
Positive Highlights
- ✅ Surgical and well-scoped — only wraps errors where an upstream cause exists; fresh errors for validation-only paths are correctly left as-is.
- ✅ PR description explains the
os.Statintentional non-wrap andTestUserFacingErrorsDontLeakInternalsinvariant clearly. - ✅ Consistent pattern applied uniformly across all three files in the import pipeline.
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · 22.3 AIC · ⌖ 6.79 AIC · ⊞ 6.7K
Comment /matt to run again
There was a problem hiding this comment.
Review Summary
The error-wrapping changes in the import pipeline are correct and well-reasoned. The PR description explains the deliberate non-wrapping of os.Stat errors to preserve the TestUserFacingErrorsDontLeakInternals invariant — this is a good call.
Existing review comments (from prior Copilot pass) cover the main gaps:
remote_resolve_path.goline 183 — wrappingfilepath.Relerror may expose internal paths in user-facing messages on Windows; confirmFormatImportErrorstrips internal path data.github_urls.goline 135 — no tests verifyerrors.Asreaches*strconv.NumErrorfor the new wrapped errors.schedule_parser.goline 229 — no regression test verifies thestrconv.Atoicause is actually wrapped.agentic-auto-upgrade.ymlline 37 — unrelated cron schedule change (Friday → Saturday) with no explanation in the PR description.
No new blocking issues found beyond those already raised.
🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · 14.6 AIC · ⌖ 4.54 AIC · ⊞ 5K
There was a problem hiding this comment.
Review: minor concerns only — existing comments cover the key issues
The mechanical changes are correct: filepath.Rel error path split, strconv.Parse* wrapping, and schedule-parser split all preserve original semantics while properly chaining upstream errors. No logic regressions introduced.
All blocking concerns (missing errors.As regression tests, potential internal-path exposure in wrapped relErr messages, and the out-of-scope cron reschedule) are already surfaced in the existing review thread. No new comments needed from this pass.
🔎 Code quality review by PR Code Quality Reviewer · 20.6 AIC · ⌖ 4.25 AIC · ⊞ 5.6K
Comment /review to run again
🤖 PR Triage
Rationale: CHANGES_REQUESTED; reviewer found issues. Defer until agent addresses feedback. Related to #46421.
|
|
@copilot please run the
|
…or tests, revert cron Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
All four review threads have been addressed in commit
Local validation: |
|
@copilot please run the Unresolved review feedback:
|
All four threads have been replied to (commit c9a688e). The code changes were already in place from the previous push; this pass adds the per-thread replies so they can be resolved. Local validation ( |
|
\n@copilot please run the skill, address any unresolved review feedback, and rerun checks once the branch is up to date.
|
|
@copilot please run the
|
All review feedback was addressed in commit
All four review threads from commit Thread resolution status: All threads have been replied to. Two are already marked outdated (the CI: A run was in progress at triage time (29652709348); the prior completed run (29646879784) succeeded. The branch needs a human to re-trigger CI (close/reopen or |
|
@copilot please run the
|
No new actionable items since the last pass. Status:
No code changes required. Ready for maintainer review and CI confirmation. |
Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
|
@copilot resolve the merge conflicts on this branch. |
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Resolved in commit
|
A repository-quality audit found 93 non-wrapping
fmt.Errorfcalls inpkg/parser. In the import pipeline, several cases had an accessible upstream error that was silently discarded rather than wrapped, breakingerrors.Is/errors.Astraversal.Changes
remote_resolve_path.go/remote_fetch_wasm.go: Split the combinedfilepath.Relcheck into twoifblocks so that afilepath.Relfailure (Windows cross-drive paths) wraps the upstream error, while the path-escape check (relativePath == "..") continues to produce a fresh error as before.github_urls.go: Wrapstrconv.ParseInterrors inparseNumberedURLandparseRunURL, enabling callers to inspect*strconv.NumErrorviaerrors.As.schedule_parser.go: SplitparseLongIntervalPartsto separate thestrconv.Atoiparse-failure path (now wraps*strconv.NumError) from theinterval < 1value-range check (no upstream error; message unchanged).Intentionally not changed
os.Staterrors in the "file not found" path are not wrapped.TestUserFacingErrorsDontLeakInternalsexplicitly enforces that*os.PathErrormust not appear in the user-facingFormattedParserErrorchain. Wrappingos.Statwith%wwould propagate the path error throughImportError.Cause → FormattedParserError.Unwrap(), breaking that invariant.