Preserve quoted cron expressions in frontmatter updates - #55502
Conversation
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
|
✅ PR Code Quality Reviewer completed the code quality review. No actionable PR review findings; the patch looks correct and I did not submit review comments.
|
|
🧠 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
|
|
✅ Ponytail Reviewer completed successfully! Lean already. Ship.
|
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. No ADR enforcement needed: PR does not have the implementation label and has only 23 new lines of code in business logic directories (threshold is 100).
|
There was a problem hiding this comment.
Pull request overview
Fixes #55333 by preserving valid quoted cron expressions during frontmatter mutations.
Changes:
- Post-process marshaled frontmatter with
QuoteCronExpressions. - Add regression coverage for unrelated updates to scheduled workflows.
Show a summary per file
| File | Description |
|---|---|
pkg/parser/workflow_update.go |
Re-quotes cron expressions after YAML marshaling. |
pkg/parser/frontmatter_helpers_test.go |
Verifies cron quoting and updates are preserved. |
Review details
💡 Add a code-review agent skill for context-aware, tailored reviews. Learn more in the docs.
- Files reviewed: 2/2 changed files
- Comments generated: 0
- Review effort level: Balanced
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.
|
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /diagnosing-bugs and /tdd — the fix is correct and well-tested. Approving.
📋 Key Themes & Highlights
Positive Highlights
- ✅ Root cause properly addressed:
QuoteCronExpressionsis applied immediately afteryaml.Marshal, which is exactly where cron values get unquoted - ✅ Regression test is present, descriptive, and follows the existing Arrange/Act/Assert pattern in this file
- ✅ The fix is a single-line surgical change — minimal blast radius
- ✅ Test validates both the cron preservation and that the unrelated frontmatter mutation (
engine: copilot) still applies, confirming no side effects
Minor Observation
The test uses assert.Contains to check that the output contains cron: "0 14 * * 1-5". This is correct but does not verify the full reconstructed YAML structure. That said, it is consistent with how the neighbouring tests in this file are written, so it's the right style call here.
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · copilot · sonnet46 · 21.8 AIC · ⌖ 9.75 AIC · ⊞ 7.6K
Comment /matt to run again
There was a problem hiding this comment.
The fix is correct and well-placed. QuoteCronExpressions is applied immediately after YAML marshaling (where go-yaml strips quotes from cron strings), and the regex safely skips already-quoted values by requiring the value to start with a digit. The new integration test validates the round-trip preservation. No blocking issues.
🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · copilot · sonnet46 · 17.5 AIC · ⌖ 8.98 AIC · ⊞ 6.2K
|
Excellent work, Your changes do exactly what is needed: ✅ Implementation — Added the missing ✅ Regression Coverage — New test ✅ Focused & Minimal — One-liner fix + focused test, no unrelated changes. This PR is ready for review and looks great for merge.
|
|
Please use the
I also attempted a branch refresh for this PR in this run. Run: https://github.com/github/gh-aw/actions/runs/32777439671
|
UpdateWorkflowFrontmattercould emit unquoted cron expressions after YAML marshaling, producing invalid workflow frontmatter during mutations such asgh aw mcp add.Frontmatter serialization
QuoteCronExpressionspost-processing before reconstructing the workflow file.Regression coverage
gh-aw-pr-sous-chefRun: https://github.com/github/gh-aw/actions/runs/32777439671