[yamllint-fixer] Fix yamllint indentation warnings in workflow_call on: sections - #55921
[yamllint-fixer] Fix yamllint indentation warnings in workflow_call on: sections#55921github-actions[bot] wants to merge 4 commits into
Conversation
injectWorkflowCallOutputs and injectWorkflowCallSecretsSection re-marshal the on: section with plain yaml.Marshal, dropping the IndentSequence(true) option applied earlier in extractTopLevelYAMLSection. This flattened sequence items (e.g. schedule cron lists) back to the same indent as their parent key, triggering yamllint's default indentation rule. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
|
✅ Ponytail Reviewer completed successfully! Lean already. Ship.
|
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅
|
|
✅ Test Quality Sentinel completed test quality analysis. No test files were added or modified in this PR. Test Quality Sentinel skipped.
|
|
✅ PR Code Quality Reviewer completed the code quality review.
|
|
✅ 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 #55921 does not have the 'implementation' label and has only 5 new lines of code in business logic directories (threshold: 100).
|
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
This change fixes the intended indentation regression and I did not find a correctness, crash, or compatibility issue in the modified lines.
Notes
- Switching these two remarshal sites to
yaml.MarshalWithOptions(..., yaml.IndentSequence(true))matches the existing behavior inextractTopLevelYAMLSection. - The one thing still worth tightening later is consistency with the repository's centralized
DefaultMarshalOptions, but that is maintainability debt, not a merge blocker for this patch. - The requested
grumpy-codersub-agent could not be started in this environment because the executable is unavailable, so this review is based on my direct analysis only.
🔎 Code quality review by PR Code Quality Reviewer · pi · gpt54 · 5.72 AIC · ⌖ 6.86 AIC · ⊞ 7K
Comment /review to run again
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /tdd — one observation on test coverage; no blocking issues.
📋 Key Themes & Highlights
Key Themes
- Missing regression test: the
schedule+workflow_callcombined scenario that triggered this bug has no test case incompiler_workflow_call_test.go. A future refactor droppingIndentSequence(true)from either call site would silently re-introduce the indentation issue.
Positive Highlights
- ✅ Root cause correctly identified and fixed at both call sites (
injectWorkflowCallOutputsandinjectWorkflowCallSecretsSection) - ✅ Fix is consistent with the existing
extractTopLevelYAMLSectionpattern - ✅ Clear, well-written PR description with before/after evidence
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · copilot · sonnet46 · 22.5 AIC · ⌖ 9.88 AIC · ⊞ 7.6K
Comment /matt to run again
There was a problem hiding this comment.
Looks good. The two-line change is correct and symmetric — switching yaml.Marshal to yaml.MarshalWithOptions(..., yaml.IndentSequence(true)) at both injection sites is the right fix for yamllint indentation warnings, and the added comment clearly explains the rationale.
🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · copilot · sonnet46 · 13.7 AIC · ⌖ 9.03 AIC · ⊞ 6.2K
There was a problem hiding this comment.
Pull request overview
Fixes YAML sequence indentation when injecting workflow_call outputs or secrets.
Changes:
- Uses
yaml.IndentSequence(true)in both remarshal paths. - Preserves yamllint-compliant schedule indentation.
Show a summary per file
| File | Description |
|---|---|
pkg/workflow/compiler_workflow_call.go |
Preserves nested sequence indentation during injection. |
Review details
- Files reviewed: 1/1 changed files
- Comments generated: 2
- Review effort level: Balanced
| // Re-marshal to YAML. | ||
| newOnData := map[string]any{"on": onMap} | ||
| newYAML, err := yaml.Marshal(newOnData) | ||
| newYAML, err := yaml.MarshalWithOptions(newOnData, yaml.IndentSequence(true)) |
There was a problem hiding this comment.
Addressed in b3b0593, extended in 06b4ed8.
TestInjectWorkflowCallSecretsSection now has a schedule + workflow_call case (pkg/workflow/compiler_workflow_call_test.go:441):
{
name: "schedule sequence stays indented and cron stays quoted",
onSection: `"on":
schedule:
- cron: "0 0 */2 * *"
workflow_call: {}`,
secrets: []string{"MY_TOKEN"},
wantContain: []string{"MY_TOKEN", " schedule:\n - cron: \"0 0 */2 * *\""},
wantAbsent: []string{" schedule:\n - cron:", "- cron: 0 0"},
},It asserts the four-space nesting positively and pins the flattened two-space form as absent, plus the unquoted cron form after 06b4ed8 also restored parser.QuoteCronExpressions on this path.
Verified it's a real guard rather than a tautology: reverting the secrets call site back to plain yaml.Marshal makes this case fail with
"...\n schedule:\n - cron: 0 0 */2 * *\n workflow_call:\n secrets:..."
does not contain " schedule:\n - cron: \"0 0 */2 * *\""
The equivalent case exists for the outputs path at line 142.
…ession tests Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
Done in 2f45d47. On the failed checks: all 8 jobs in run 32936194474 failed with the same infrastructure error, not a code error: i.e. the shared checkout cache entry was missing, so every dependent job aborted before running anything. Nothing in the diff caused it; the new push should repopulate the cache. Forward-progress pass in this branch:
Note: |
| on: | ||
| schedule: | ||
| - cron: 0 0 */2 * * | ||
| - cron: 0 0 */2 * * |
There was a problem hiding this comment.
@copilot this cron expression should be quoted
There was a problem hiding this comment.
Fixed in 750b100.
Same root cause as the indentation issue, one layer deeper. extractTopLevelYAMLSection marshals the on: section and then post-processes it with parser.QuoteCronExpressions, because goccy drops quotes from cron scalars like 0 0 */2 * *. The workflow_call injection functions re-marshal that section afterwards, which re-introduced the unquoted form — my previous commit restored the indentation but not the quoting.
Both re-marshal sites now route through a small finalizeWorkflowCallOnSection helper that re-applies parser.QuoteCronExpressions, so the two paths stay consistent.
on:
schedule:
- cron: "0 0 */2 * *"Also verified grep -rn 'cron: [^"]' .github/workflows/*.lock.yml now returns nothing across all 294 lock files — these two were the only unquoted ones. The regression tests in TestInjectWorkflowCallOutputs/TestInjectWorkflowCallSecretsSection were extended to assert the quoted form is preserved in addition to the 4-space indentation.
…/secrets Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
|
Please do one focused follow-up pass:
Run: https://github.com/github/gh-aw/actions/runs/32979958387
|
No new code was needed this pass — the schedule+workflow_call regression coverage was already pushed. Replied in-thread on the secrets review thread (#discussion_r3860058001) with the concrete reference. Status:
Both new test cases were confirmed to be genuine regression guards: reverting either call site to plain Nothing actionable is left on my side; this needs maintainer review. One pre-existing note: |
Summary
Reduces yamllint noise in generated
*.lock.ymlfiles by fixing a root cause in the GoYAML generator code: the
on:trigger section forworkflow_call-triggered workflows waslosing its sequence indentation when outputs/secrets were injected.
yamllint config used:
{extends: default, rules: {line-length: disable, document-start: disable, truthy: {check-keys: false}, comments: {require-starting-space: true, min-spaces-from-content: 1}}}Results
Warning breakdown
Root cause
extractTopLevelYAMLSection(pkg/workflow/frontmatter_extraction_yaml.go) already marshalsthe
on:section withyaml.IndentSequence(true)so that sequence items likeschedule:cron lists are indented under their parent key, satisfying yamllint's default
indentationrule.However, for workflows that use
workflow_callas a trigger,injectWorkflowCallOutputsand
injectWorkflowCallSecretsSectioninpkg/workflow/compiler_workflow_call.gore-parse and re-marshal that same
on:section afterward (to injectworkflow_call.outputs/workflow_call.secrets), but did so with plainyaml.Marshal,which uses the default (non-indented) sequence style. This flattened any sequence under
on:— most visiblyschedule:cron lists — back to the same indent level as the key,e.g.:
This affected
smoke-workflow-call.lock.ymlandsmoke-workflow-call-with-inputs.lock.yml,both of which combine a
scheduletrigger withworkflow_call.Changes
pkg/workflow/compiler_workflow_call.go: both re-marshal call sites now passyaml.IndentSequence(true)toyaml.MarshalWithOptions, matching the option used inextractTopLevelYAMLSectionso the indentation style is preserved end-to-end.Notes
.lock.ymlfiles are not included in this PR.Run
gh aw compileafter merging to regenerate them with the improvement.remaining in the current baseline).
Warning
Firewall blocked 1 domain
The following domain was blocked by the firewall during workflow execution:
registry.npmjs.orgTo allow these domains, add them to the
network.allowedlist in your workflow frontmatter:See Network Configuration for more information.