Fix actionlint failure on generated maintenance workflow operation input - #56506
Conversation
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
operation input
There was a problem hiding this comment.
Pull request overview
Fixes invalid empty choice values in generated maintenance workflows while preserving no-operation behavior.
Changes:
- Uses
noneas the dispatch sentinel. - Handles both
noneand empty workflow-call inputs. - Updates generated YAML and condition tests.
Show a summary per file
| File | Description |
|---|---|
pkg/workflow/side_repo_maintenance.go |
Uses the sentinel for side-repository dispatch choices. |
pkg/workflow/maintenance_workflow_yaml.go |
Emits a valid standard dispatch choice. |
pkg/workflow/maintenance_conditions.go |
Centralizes no-operation condition handling. |
pkg/workflow/maintenance_workflow_jobs_test.go |
Updates expected generated conditions. |
.github/workflows/agentics-maintenance.yml |
Regenerates the maintenance workflow. |
Review details
💡 Add a code-review agent skill for context-aware, tailored reviews. Learn more in the docs.
- Files reviewed: 5/5 changed files
- Comments generated: 0
- Review effort level: Balanced
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅
|
|
✅ Test Quality Sentinel completed test quality analysis. Test Quality Sentinel skipped because pre-fetch PR data was unavailable: unable to fetch test file diff
|
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. See the comment below for the result and any generated ADR draft. No ADR enforcement needed: PR does not have the 'implementation' label and has <=100 new lines of code in business logic directories.
|
|
✅ PR Code Quality Reviewer completed the code quality review.
|
|
Warning Firewall blocked 4 domainsThe following domains were blocked by the firewall during workflow execution:
[!TIP] tools:
github:
mode: gh-proxySee GitHub Tools for more information on To allow these domains, add them to the network:
allowed:
- defaults
- "ab.chatgpt.com"
- "api.github.com"
- "chatgpt.com"
- "github.com"See Network Configuration for more information.
|
Comment MemoryNote This comment is managed by comment memory.It stores persistent context for this thread in the code block at the top of this comment.
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Verdict
Non-blocking change. The none sentinel is wired through the generated conditions consistently, and I did not find a changed-line regression that would justify blocking merge.
Highlights
workflow_dispatchnow emits a valid non-empty choice default.- The condition helpers preserve the old empty-string behavior for
workflow_callwhile accepting the new dispatch sentinel. - Generated YAML and unit-test expectations were updated in sync.
🔎 Code quality review by PR Code Quality Reviewer · pi · gpt54 · 8.64 AIC · ⌖ 7.32 AIC · ⊞ 7K
Comment /review to run again
There was a problem hiding this comment.
The fix correctly replaces the empty-string choice option with 'none' as the sentinel value for inputs.operation, resolving the actionlint failure.
The approach is sound:
- A named constant
maintenanceNoOperationValuecentralises the sentinel string. - Two helpers (
buildOperationIsEmptyCondition/buildOperationIsNotEmptyCondition) encapsulate the dual-check, ensuring bothworkflow_call(which keepsdefault: '') andworkflow_dispatch(which now defaults to'none') are handled consistently. - The generated
.yml, Go generator, and tests are all updated in lock-step.
No blocking issues found.
🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · copilot · sonnet46 · 18.3 AIC · ⌖ 9.63 AIC · ⊞ 6.2K
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /diagnosing-bugs and /tdd — commenting (no blocking issues, but two improvements worth addressing).
📋 Key Themes & Highlights
Key Themes
- Inconsistent string-building style:
side_repo_maintenance.gouses raw+concatenation while the parallel function inmaintenance_workflow_yaml.gousesfmt.Sprintf— a minor but real maintainability gap. - Missing unit tests for new helpers:
buildOperationIsEmptyCondition/buildOperationIsNotEmptyConditionare untested directly; coverage depends on incidental string matching in integration-style tests.
Positive Highlights
- ✅ Root cause is correctly addressed — the
'none'sentinel removes the actionlint violation without breakingworkflow_callcallers that still use an empty string. - ✅ Both conditions (
isEmpty/isNotEmpty) are factored into named helpers rather than inlined in multiple call sites — good abstraction. - ✅ The
isNotEmptyhelper deliberately avoids negating an OR to preserve the!=style already used inrun_operation— consistent rendering. - ✅ Tests updated to match the new condition strings, and search-range constants bumped to accommodate the longer strings.
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · copilot · sonnet46 · 25.9 AIC · ⌖ 11.7 AIC · ⊞ 7.6K
Comment /matt to run again
| @@ -352,9 +352,9 @@ on: | |||
| description: 'Optional maintenance operation to run' | |||
| required: false | |||
| type: choice | |||
There was a problem hiding this comment.
[/diagnosing-bugs] String concatenation here is inconsistent with maintenance_workflow_yaml.go, which uses fmt.Sprintf + %[1]s for the same substitution. A future rename of maintenanceNoOperationValue could silently miss this spot.
💡 Suggested change
Adopt the same fmt.Sprintf pattern used in maintenance_workflow_yaml.go:
return fmt.Sprintf(` workflow_dispatch:
inputs:
operation:
...
default: '%[1]s'
options:
- '%[1]s'
`, maintenanceNoOperationValue)This keeps the two YAML-building functions consistent and makes the substitution explicit.
@copilot please address this.
| BuildStringLiteral(""), | ||
| ), | ||
| BuildEquals( | ||
| BuildPropertyAccess("inputs.operation"), |
There was a problem hiding this comment.
[/tdd] The two new helpers buildOperationIsEmptyCondition and buildOperationIsNotEmptyCondition have no direct unit tests. Coverage currently comes only from the integration-style string-match tests in maintenance_workflow_jobs_test.go, which means a logic error inside either helper (e.g. swapping BuildOr for BuildAnd) would produce a wrong condition string that the existing tests would catch only incidentally.
💡 Suggested test
Add a small table-driven test in maintenance_conditions_test.go (or equivalent):
func TestBuildOperationIsEmptyCondition(t *testing.T) {
node := buildOperationIsEmptyCondition()
got := renderCondition(node) // use whatever renderer is available
want := `inputs.operation == '' || inputs.operation == 'none'`
require.Equal(t, want, got)
}
func TestBuildOperationIsNotEmptyCondition(t *testing.T) {
node := buildOperationIsNotEmptyCondition()
got := renderCondition(node)
want := `inputs.operation != '' && inputs.operation != 'none'`
require.Equal(t, want, got)
}This gives a regression anchor directly on the helpers and documents their intended output as a specification.
@copilot please address this.
There was a problem hiding this comment.
This helper layer is a bit heavier than the feature needs; inline the inputs.operation checks directly at the two call sites and keep the sentinel constant local to the workflow change. net: -10 lines possible.
Warning
Firewall blocked 4 domains
The following domains were blocked by the firewall during workflow execution:
ab.chatgpt.comapi.github.comchatgpt.comgithub.com
[!TIP]
api.github.com is blocked because GitHub API access uses the built-in GitHub tools by default. Instead of adding api.github.com to network.allowed, use tools.github.mode: gh-proxy for direct pre-authenticated GitHub CLI access without requiring network access to api.github.com:
tools:
github:
mode: gh-proxySee GitHub Tools for more information on gh-proxy mode.
To allow these domains, add them to the network.allowed list in your workflow frontmatter:
network:
allowed:
- defaults
- "ab.chatgpt.com"
- "api.github.com"
- "chatgpt.com"
- "github.com"See Network Configuration for more information.
Generated by ✂️ Ponytail Reviewer for #56506 · codex · mai10 · 8.99 AIC · ⌖ 0.266 AIC · ⊞ 12.8K
Comment /ponytail to run again
| // `inputs.operation` value represents "no operation selected", i.e. it is | ||
| // either an empty string (workflow_call default) or the choice sentinel value | ||
| // used by the workflow_dispatch "operation" input. | ||
| func buildOperationIsEmptyCondition() ConditionNode { |
There was a problem hiding this comment.
pkg/workflow/maintenance_conditions.go:16-50: yagni: two tiny helpers for one extra / branch. Inline the comparisons directly at the call sites.
|
@pelikhan Unfortunately I hit an unexpected error while processing your comment. I've automatically reported this to GitHub. You can ask me to try again later by mentioning me in a new comment. If you want to contact GitHub about this error, please mention the following identifier so they can better serve you: Sorry for the inconvenience! |
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
This reverts commit 88cc94a. 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>
Fixed Go formatting in |
|
🎉 This pull request is included in a new release. Release: |
The generated
agentics-maintenance.ymlworkflow_dispatch.operationchoice input used''as both the default and an option, which actionlint rejects (string should not be empty). Removing the empty option alone just trades it for a different actionlint error, since the default then no longer matches any option.Changes
nonesentinel —pkg/workflow/maintenance_workflow_yaml.goandpkg/workflow/side_repo_maintenance.gonow emit'none'as theworkflow_dispatchchoice default/option instead of''. Theworkflow_calloperationinput stays a plainstringwith an empty-string default, since it isn't achoiceand isn't subject to this restriction.''and'none'as equivalent "no operation" — addedbuildOperationIsEmptyCondition()/buildOperationIsNotEmptyCondition()helpers inpkg/workflow/maintenance_conditions.goso every generatedif:condition (schedule-only jobs,run_operation,cleanup-cache-memory, etc.) recognizes both theworkflow_callempty string and theworkflow_dispatchsentinel as "nothing selected", instead of relying on a single empty-string check.!=comparisons (rather than negating an OR), sorun_operation's condition keeps the same!=style as its other operation exclusion checks instead of mixing!(x == y)andx != y.agentics-maintenance.ymland updated the corresponding Go unit tests to match the new condition strings.Before:
After:
agentics-maintenance.ymlfails actionlint #55733