parser: introduce ErrUnsupportedSyntax sentinel; remove all errstringmatch nolints#46421
Conversation
…ch nolints Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
…ch nolints - Add ErrUnsupportedSyntax to pkg/parser/schedule_parser.go; wrap all four "syntax is not supported" return sites with %w so callers can use errors.Is instead of substring matching. - Replace strings.Contains(err.Error(), "syntax is not supported") in schedule_preprocessing.go with errors.Is(err, parser.ErrUnsupportedSyntax); drop the //nolint:errstringmatch comment. - Refactor isWindowsLockError in update_extension_check.go to extract err.Error() into a local string before the loop; drop nolint. - Pre-extract err.Error() in add_interactive_git.go and project_command.go; drop both nolint suppressions. - Remove misplaced //nolint:errstringmatch from audit.go (isPermissionErrorStr takes a string parameter, not error; the linter never fired there). Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Introduces typed classification for unsupported schedule syntax while revising external error-string checks.
Changes:
- Adds and wraps
ErrUnsupportedSyntax. - Uses
errors.Isduring schedule preprocessing. - Removes
errstringmatchsuppressions, including through pre-extracted error strings.
Show a summary per file
| File | Description |
|---|---|
pkg/parser/schedule_parser.go |
Adds and wraps the sentinel error. |
pkg/workflow/schedule_preprocessing.go |
Uses typed error classification. |
pkg/cli/update_extension_check.go |
Revises Windows lock detection. |
pkg/cli/project_command.go |
Revises scope-error detection. |
pkg/cli/add_interactive_git.go |
Revises merged-PR detection. |
pkg/cli/audit.go |
Removes an unnecessary suppression. |
Review details
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 6/6 changed files
- Comments generated: 4
- Review effort level: Medium
| // recognised schedule syntax that is explicitly not supported (e.g. "daily at | ||
| // 2pm"). Callers that need to distinguish this from a completely unrecognised | ||
| // input should test with errors.Is. | ||
| var ErrUnsupportedSyntax = errors.New("unsupported schedule syntax") |
| errMsg := err.Error() | ||
| return strings.Contains(errMsg, "already merged") || strings.Contains(errMsg, "MERGED") |
| errMsg := err.Error() | ||
| if strings.Contains(errMsg, "INSUFFICIENT_SCOPES") || errorutil.IsNotFoundError(err) { |
| errMsg := "" | ||
| if err != nil { | ||
| errMsg = err.Error() |
|
✅ 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 #46421 does not have the 'implementation' label and has only 20 new lines of code in business logic directories (threshold: 100). |
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅ |
|
✅ PR Code Quality Reviewer completed the code quality review. |
|
@copilot please run the
|
🤖 PR Triage
Rationale: Sentinel-error cleanup across parser. Pair with #46414 for batch review — related parser error-handling improvements.
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review Summary
The ErrUnsupportedSyntax sentinel and its use in schedule_preprocessing.go are well-implemented. The audit.go nolint removal is also correct.
Two blocking issues remain:
1. Missing sentinel test coverage (see existing comment on schedule_parser.go)
The four %w-wrapped branches have no test asserting errors.Is(err, parser.ErrUnsupportedSyntax). Removing %w from any branch would still pass all existing tests — the public contract is unverified.
2. External-error callsites hide the problem rather than fix it (see existing comments on add_interactive_git.go, project_command.go, update_extension_check.go)
Extracting err.Error() into a local variable only bypasses the linter; it does not improve correctness, and it removes the rationale comments that documented why string matching is unavoidable. The original //nolint:errstringmatch // <reason> form was more transparent. Restoring those suppressions with their explanatory comments is the correct approach for external errors the codebase does not own.
🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · 19 AIC · ⌖ 4.35 AIC · ⊞ 5K
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /tdd and /codebase-design — requesting changes on two themes.
📋 Key Themes & Highlights
Key Themes
- Missing sentinel tests (
/tdd): the newErrUnsupportedSyntaxpublic contract has no test coverage — existing tests only assert on error message substrings, so silently removing any%wwrap would still pass. - Rationale stripped from external-error callsites (
/codebase-design): replacing//nolint:errstringmatch // <reason>with a bare local variable hides why string matching is unavoidable here, and makes the linter suppression implicit instead of explicit.
Positive Highlights
- ✅ The
ErrUnsupportedSyntaxsentinel itself is well-named and correctly documented. - ✅
schedule_preprocessing.go— theerrors.Isreplacement is the cleanest change in the PR. - ✅ Dropping the misplaced
nolintinaudit.go(function takesstring, noterror) is a correct cleanup. - ✅ All four
ParseSchedulebranches consistently wrap the sentinel via%w.
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · 15.6 AIC · ⌖ 4.41 AIC · ⊞ 6.7K
Comment /matt to run again
There was a problem hiding this comment.
REQUEST_CHANGES — one high-severity behaviour regression and one medium UX issue must be fixed before merge.
### Blocking issues
High — silent regression at lines 290 & 310 of schedule_parser.go
The PR switches schedule_preprocessing.go from strings.Contains(err, "syntax is not supported") to errors.Is(err, parser.ErrUnsupportedSyntax), but two existing error sites ("minute intervals with 'on weekdays' are not supported") were not updated to wrap the sentinel. Those invalid schedules now silently pass through as if they were valid cron expressions, instead of being rejected. This is a direct behaviour regression.
Medium — redundant sentinel text in error messages
All four sites use %w to wrap ErrUnsupportedSyntax, which appends : unsupported schedule syntax to messages that already say "... syntax is not supported". This is noise in user-visible output; a private wrapper type that implements errors.Is without contributing to .Error() would solve both the classification and the message cleanly.
The existing comments on the three external-error callsites (pre-extracting err.Error()) are also still open — those cases genuinely cannot use typed errors, but they should retain the rationale comments that this PR removed.
🔎 Code quality review by PR Code Quality Reviewer · 47.7 AIC · ⌖ 4.61 AIC · ⊞ 5.6K
Comment /review to run again
Comments that could not be inline-anchored
pkg/parser/schedule_parser.go:290
Silent regression: minute-interval errors not wrapped with ErrUnsupportedSyntax — errors.Is(err, parser.ErrUnsupportedSyntax) in schedule_preprocessing.go will not catch these, so "minute intervals with 'on weekdays' are not supported" errors fall through and the invalid schedule is silently left unchanged instead of rejected.
<details>
<summary>💡 Suggested fix</summary>
Both line 290 and 310 need the sentinel wrapped:
// before:
return "", errors.New("minute intervals wit…
</details>
<details><summary>pkg/parser/schedule_parser.go:85</summary>
**Redundant sentinel suffix pollutes user-visible error messages**: wrapping `ErrUnsupportedSyntax` via `%w` appends `: unsupported schedule syntax` to every already-descriptive message, e.g. `'daily at <time>' syntax is not supported. ... use standard cron syntax (e.g., '0 14 * * *'): unsupported schedule syntax`.
<details>
<summary>💡 Suggested fix</summary>
Implement a private wrapper type that satisfies `errors.Is` without contributing to `.Error()`:
```go
type unsupportedSyntaxError str…
</details>
…yntax tests Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
…Is tests
- Revert pkg/cli/{add_interactive_git,project_command,update_extension_check}.go
to use explicit //nolint:errstringmatch with documented rationale instead of
the pre-extraction workaround. The linter's own fixture establishes nolint
as the supported pattern for unavoidable external CLI text matching.
- Add TestErrUnsupportedSyntaxIs in pkg/parser/schedule_parser_test.go with
table-driven cases for daily-at-time, weekly-at-time, both monthly forms, and
a negative ordinary-parse-error case; uses assert.ErrorIs / assert.NotErrorIs
to satisfy testifylint.
Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
Addressed all reviewer feedback in commit
|
|
@copilot resolve the merge conflicts on this branch. |
|
@copilot please run the Unresolved review feedback:
|
…ntroduce-sentinel-errors # Conflicts: # .github/workflows/agentic-auto-upgrade.yml Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Resolved. The only conflict was in |
All unresolved review threads have been addressed in prior commits on this branch. Here's the summary:
Hand-off: CI must be re-triggered by a maintainer (close/reopen PR, |
Four production callsites used
strings.Contains(err.Error(), ...)for error classification, requiring//nolint:errstringmatchsuppressions to silence the custom linter. The recurring case — distinguishing "explicitly unsupported schedule syntax" from "not a schedule at all" inschedule_preprocessing.go— was fixable with a proper sentinel.Changes
pkg/parser/schedule_parser.go— new sentinelAll four
"syntax is not supported"error returns now wrap this sentinel via%w.pkg/workflow/schedule_preprocessing.go— useerrors.IsReplaced the substring check with type-safe classification:
External-error callsites — pre-extract the error string
For the three remaining callsites where errors come from the OS or the
ghCLI (anderrors.Isis genuinely inapplicable),err.Error()is extracted to a localstringvariable before anystrings.Containscall. This satisfies the linter without pretending these are typed errors we own:pkg/cli/update_extension_check.go— Windows locked-binary detectionpkg/cli/add_interactive_git.go— already-merged detectionpkg/cli/project_command.go— GraphQLINSUFFICIENT_SCOPESdetectionpkg/cli/audit.go— drop misplaced nolintisPermissionErrorStrtakes astringparameter, noterror; the linter never fired there. The suppression was unnecessary.