Improve time-delta validation diagnostics to satisfy error-message lint quality#42651
Conversation
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
|
✅ PR Code Quality Reviewer completed the code quality review. |
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. No ADR enforcement needed: PR #42651 does not have the 'implementation' label and has only 7 new lines of code in business logic directories (threshold: 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. ✅ |
There was a problem hiding this comment.
Pull request overview
This PR aims to improve user-facing validation diagnostics for time-delta / absolute date-time parsing in gh-aw, primarily to satisfy the repository’s error-message lint quality threshold, by adding more actionable examples at failure points.
Changes:
- Improved time-delta validation errors with copyable
Example:guidance for common failure modes. - Enhanced absolute date-time parse failure diagnostics with an explicit example format.
- Updated generated skills reviewer workflow lock files to delegate GitHub CLI minimum-version enforcement to
ensure_gh_cli_min_version.sh.
Show a summary per file
| File | Description |
|---|---|
| pkg/workflow/time_delta.go | Improves validation error messages for time delta and absolute date-time parsing with more actionable examples. |
| .github/workflows/mattpocock-skills-reviewer.lock.yml | Switches the GH CLI min-version enforcement step to call the shared ensure_gh_cli_min_version.sh helper. |
| .github/workflows/impeccable-skills-reviewer.lock.yml | Same GH CLI min-version enforcement delegation as above. |
Review details
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 3/3 changed files
- Comments generated: 2
- Review effort level: Low
| default: | ||
| return nil, fmt.Errorf("unsupported time unit '%s' in time delta: +%s", unit, deltaStr) | ||
| return nil, fmt.Errorf("unsupported time unit '%s' in time delta: +%s. Example: +2w (supported units: mo, w, d, h, m)", unit, deltaStr) |
| - name: Upgrade gh CLI for frontmatter skills | ||
| run: | | ||
| set -euo pipefail | ||
| REQUIRED="2.90.0" | ||
| GH_VERSION=$(gh --version | awk 'NR==1 {print $3}') | ||
| echo "gh version: ${GH_VERSION}" | ||
| if ! printf '%s\n%s\n' "$REQUIRED" "$GH_VERSION" | sort -V -C; then | ||
| echo "gh ${GH_VERSION} is older than ${REQUIRED}, upgrading..." | ||
| bash "${RUNNER_TEMP}/gh-aw/actions/install_gh_cli.sh" | ||
| GH_VERSION=$(gh --version | awk 'NR==1 {print $3}') | ||
| echo "gh version after upgrade: ${GH_VERSION}" | ||
| if ! printf '%s\n%s\n' "$REQUIRED" "$GH_VERSION" | sort -V -C; then | ||
| echo "::error::gh ${GH_VERSION} is older than required ${REQUIRED} (gh skill support requires v2.90.0+)" | ||
| exit 1 | ||
| fi | ||
| fi | ||
| run: bash "${RUNNER_TEMP}/gh-aw/actions/ensure_gh_cli_min_version.sh" "2.90.0" |
There was a problem hiding this comment.
Non-blocking observation on one message string. All logic is unchanged and the other diagnostic improvements are accurate.
Review themes
What was checked
pkg/workflow/time_delta.go— seven error-message-only changes: format verbs, argument counts, example accuracy, unit lists.lock.ymlrefactoring — inline gh-version-check replaced withensure_gh_cli_min_version.sh; script exists atactions/setup/sh/and is deployed to${RUNNER_TEMP}/gh-aw/actions/matching the standard pattern; behavior is equivalent and slightly improved (handles gh-not-found gracefully)
What is correct
- All
fmt.Errorfformat strings have the right number of arguments - The
unsupported unitmessage correctly listsmo, w, d, h, m(matches the regex and switch) %wmid-string does not breakerrors.Unwrap; the appended text after%win the date-time message is fine- The ceiling division
(value+59)/60is arithmetically correct
One comment filed
The hardcoded Example: +90m -> +2h appended to the stop-after minute error creates a misleading contradiction for any input other than exactly 90m (e.g., +61m produces two different mappings in the same sentence). See inline comment for two fix options.
🔎 Code quality review by PR Code Quality Reviewer · 73 AIC · ⌖ 7.04 AIC · ⊞ 1.6K
Comment /review to run again
| case "m": | ||
| if !allowMinutes { | ||
| return nil, fmt.Errorf("minute unit 'm' is not allowed for stop-after. Minimum unit is hours 'h'. Use +%dh instead of +%dm", (value+59)/60, value) | ||
| return nil, fmt.Errorf("minute unit 'm' is not allowed for stop-after. Minimum unit is hours 'h'. Use +%dh instead of +%dm. Example: +90m -> +2h", (value+59)/60, value) |
There was a problem hiding this comment.
Hardcoded example contradicts the dynamic conversion hint for most inputs.
💡 Details and suggested fix
The message already dynamically formats the correct suggestion:
"Use +%dh instead of +%dm"
Appending a hardcoded literal Example: +90m -> +2h creates a contradiction for any value other than 90. For example, +61m produces:
Use +2h instead of +61m. Example: +90m -> +2h
A user sees two different mappings in one sentence, which is confusing.
Option A — Remove the redundant example (the dynamic part is already self-explanatory):
return nil, fmt.Errorf(
"minute unit 'm' is not allowed for stop-after. Minimum unit is hours 'h'. Use +%dh instead of +%dm",
(value+59)/60, value,
)Option B — Make the example dynamic so it always matches the user's actual input:
return nil, fmt.Errorf(
"minute unit 'm' is not allowed for stop-after. Minimum unit is hours 'h'. Use +%dh instead of +%dm (e.g. +%dm -> +%dh)",
(value+59)/60, value, value, (value+59)/60,
)There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /grill-with-docs — requesting changes on two error-message quality issues, one correctness concern.
📋 Key Themes & Highlights
Key Themes
- Static example contradicts dynamic output (line 137):
Example: +90m -> +2his wrong for all inputs except exactly+90m— the message already computes the right conversion dynamically, so the static example adds confusion. - Redundant example (line 86):
Example: +1d12h30mduplicates a value already present in theExpected format likeclause of the same string. - Uninformative example (line 119):
Example: +3don the invalid-number path shows a valid format rather than contrasting with the overflow case that actually triggers this branch.
Positive Highlights
- ✅ Six out of seven messages are genuinely improved — adding
Example:guidance where none existed before is exactly the right fix. - ✅
unsupported time unitmessage (line 141) adds the supported-unit list alongside the example — excellent discoverability improvement. - ✅ Lock-file refactoring is clean:
ensure_gh_cli_min_version.shis compiler-generated fromcompiler_activation_job_builder.go, correctly picked up bymake recompile. - ✅ No parsing logic changed — the fix is safely scoped to user-facing strings.
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · 68.9 AIC · ⌖ 7.57 AIC · ⊞ 6.6K
Comment /matt to run again
| case "m": | ||
| if !allowMinutes { | ||
| return nil, fmt.Errorf("minute unit 'm' is not allowed for stop-after. Minimum unit is hours 'h'. Use +%dh instead of +%dm", (value+59)/60, value) | ||
| return nil, fmt.Errorf("minute unit 'm' is not allowed for stop-after. Minimum unit is hours 'h'. Use +%dh instead of +%dm. Example: +90m -> +2h", (value+59)/60, value) |
There was a problem hiding this comment.
[/grill-with-docs] The hardcoded +90m -> +2h example is only accurate when the input is exactly +90m. For any other input (e.g. +150m), the dynamic Use +3h instead of +150m portion gives the right answer, but the static example (+90m -> +2h) contradicts it — confusing users.
💡 Make the example dynamic
Derive the example from the actual input value:
return nil, fmt.Errorf(
"minute unit 'm' is not allowed for stop-after. Minimum unit is hours 'h'. Use +%dh instead of +%dm. Example: +%dm -> +%dh",
(value+59)/60, value, value, (value+59)/60,
)Alternatively, simply omit the static example — Use +%dh instead of +%dm is already fully actionable and correct.
@copilot please address this.
|
|
||
| if len(matches) == 0 { | ||
| return nil, fmt.Errorf("invalid time delta format: +%s. Expected format like +25h, +3d, +1w, +1mo, +1d12h30m", deltaStr) | ||
| return nil, fmt.Errorf("invalid time delta format: +%s. Expected format like +25h, +3d, +1w, +1mo, +1d12h30m. Example: +1d12h30m", deltaStr) |
There was a problem hiding this comment.
[/grill-with-docs] The appended Example: +1d12h30m is redundant — +1d12h30m already appears verbatim in the preceding Expected format like ... clause, so readers see the same value twice.
💡 Options
- Remove the trailing
Example: +1d12h30mentirely — theExpected format likelist is already the example. - Or, if the lint rule specifically requires the
Example:prefix, replace the duplicate with a simpler variant not already in the list, e.g.Example: +2d(shorter, unambiguous).
@copilot please address this.
| value, err := strconv.Atoi(valueStr) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("invalid number '%s' in time delta: +%s", valueStr, deltaStr) | ||
| return nil, fmt.Errorf("invalid number '%s' in time delta: +%s. Example: +3d", valueStr, deltaStr) |
There was a problem hiding this comment.
[/grill-with-docs] Example: +3d shows a fully valid delta, but doesn't help users understand what makes a number token invalid (typically overflow — e.g. +99999999999999999999h). A more diagnostic example would hint at the cause.
💡 Suggestion
In practice this path is only reachable via strconv.Atoi overflow, since the regex already restricts tokens to digit-only strings. Consider making the cause explicit:
return nil, fmt.Errorf("invalid number '%s' in time delta: +%s (numeric overflow; use values like +3d or +2w)", valueStr, deltaStr)This distinguishes the overflow case from a format error and removes the hollow Example: whose value +3d is valid rather than being contrasted against the invalid input.
@copilot please address this.
|
🎉 This pull request is included in a new release. Release: |
lint-errorsfailed because severalpkg/workflow/time_delta.govalidation paths emitted terse messages without actionable examples, pulling aggregate error-message quality below threshold. This PR tightens those diagnostics by adding concrete, copyable examples at the failure points.Scope
pkg/workflow/time_delta.goerror strings used by time-delta/date parsing and stop-after unit validation.Changes
Example:guidance for:+prefix+90m -> +2h) to align with computed rounding behavior.Behavioral impact