Repository navigation
fix(workflow): accept schema-supported frontmatter shapes - #71809
Conversation
Preserve typed config for scalar safe-job needs, cache arrays, daily credit objects, and K/M credit budgets without weakening ordinary templatable integers or invalid-input warnings. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
|
✅ Ponytail Reviewer completed successfully! Ponytail ReviewerOutcome: The agent completed and reported no action. A no-op output alone does not verify that all required work was completed or that publication was possible. Agent messages are omitted from this comment. The category uses structured run status, not interpretation of agent prose.
|
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅
|
|
✅ PR Code Quality Reviewer completed the code quality review.
|
|
✅ Test Quality Sentinel completed test quality analysis. Test Quality SentinelOutcome: The agent completed and reported no action. A no-op output alone does not verify that all required work was completed or that publication was possible. Agent messages are omitted from this comment. The category uses structured run status, not interpretation of agent prose.
|
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. See the comment below for the result and any generated ADR draft. Commit pushed:
|
![github-advanced-security[bot]](https://cdn.statically.io/img/avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
🏗️ Design Decision Gate — ADR RequiredThis PR makes significant changes to core business logic (305 new lines in 📄 Draft ADR committed: The draft records this decision: typed frontmatter parsing accepts every shape the schema allows by adapting at the decode boundary. In detail, scalar
📋 What to do next
Once an ADR is linked in the PR body, this gate will re-run and check that the implementation matches the decision. ❓ Why ADRs Matter
ADRs create a searchable, permanent record of why the codebase looks the way it does. 📋 Michael Nygard ADR Format ReferenceAn ADR must contain these four sections to be considered complete:
All ADRs are stored in
|
![github-actions[bot]](https://cdn.statically.io/img/avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review: no blocking issues found
The frontmatter shape handling and regression coverage look internally consistent on the changed lines. I did not identify an actionable correctness, security, or performance defect requiring a merge block. This was a static review; tests were not executed.
🔎 Code quality review by PR Code Quality Reviewer · copilot · gpt50 · ~7 AIC · ⌖ ~1 AIC · ⊞ 1.6K
Comment /review to run again
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /diagnosing-bugs and /tdd (per pr-triage classification: bug_fix). The fix correctly widens parsing to accept schema-supported shapes (needs: scalar, cache: array, max-ai-credits/max-daily-ai-credits K/M-suffix/expression/object forms) that were previously rejected by strict typed unmarshal — verified against main_workflow_schema.json and docs/src/content/docs/reference/{cost-management,safe-outputs}.md, confirming no scope-creep. go build and the full new test suite pass.
Left 3 minor, non-blocking inline suggestions — approving as the changes are solid and well-tested.
📋 Key Themes & Highlights
Key Themes
- Implicit nil-safety invariant (
aic_limit_parser.go): the object-form recursion'slimit.String()call is safe only because an outer check happens to mirror an inner short-circuit condition — not a live bug, but fragile to future edits. - Test traceability (
frontmatter_shapes_test.go): 4 new test functions give strong shape-based coverage (positive/negative/round-trip), but none anchor back to the 'public workflow sweep' failures cited in the PR description. - Two parsing paths for
needs(frontmatter_normalization.govssafe_jobs.go): correct and tested, but worth a comment explaining why typed-unmarshal normalization and manual map parsing both exist for the same field.
Positive Highlights
- ✅
config.Cachetype widening (map[string]any→any) correctly matches the schema's documentedoneOf(object or array) and doesn't affect runtime cache-step generation, which reads from raw YAML separately. - ✅ Comprehensive new test file covers supported shapes, AI-credit edge cases (K/M suffix, expressions,
-1sentinel, object form), and invalid-shape rejection. - ✅ Raw frontmatter is not mutated during
needsnormalization — a clean, side-effect-free approach.
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · copilot · sonnet50 · ~180 AIC · ⌖ ~16 AIC · ⊞ 10.2K
Comment /matt to run again
There was a problem hiding this comment.
🔵 Needs a closer look
The reported drift in 27 generated lock files leaves compiler-output compatibility unresolved and requires maintainer review.
1 open issue
What changed in this PR
This PR aligns gh-aw’s typed frontmatter parsing with schema-supported forms, preventing valid configurations from being dropped after generic parse warnings.
Changes:
- Normalize scalar safe-job dependencies without modifying raw frontmatter.
- Support cache objects and arrays, expanded credit-limit syntax, and daily-credit objects.
- Add regression coverage for valid shapes, invalid inputs, round trips, and compiled output.
| File | Description |
|---|---|
pkg/workflow/frontmatter_types.go |
Allow multiple cache shapes and clarify the normalized daily-credit field. |
pkg/workflow/frontmatter_shapes_test.go |
Test parsing, non-mutation, round trips, warnings, and compiled output. |
pkg/workflow/frontmatter_parsing.go |
Decode credit limits separately and validate cache shapes. |
pkg/workflow/frontmatter_normalization.go |
Normalize scalar safe-job dependencies using cloned maps. |
pkg/workflow/aic_limit_parser.go |
Parse credit-specific scalar syntax and daily-credit objects. |
Review effort: Max
Guard recursive credit parsing, clarify object-value errors, remove sensitive input from diagnostics and suffix-helper logs, anchor public-sweep regressions, and finalize the proposed ADR. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
|
In response to #71809 (comment): No code change is needed for this status-only notice. I have not treated reported_no_action as validation evidence; the focused regression checks and their limitations are documented separately. |
|
In response to #71809 (comment): The three actionable inline suggestions from this review have been accepted: explicit nil safety, a public-sweep regression anchor, and an explanation of the two dependency parsing paths. Each inline thread has its own implementation reply. |
|
In response to #71809 (comment): No code change is needed for this completion notice or its no-blocking-issues review. The follow-up batch preserves the existing validation boundaries and addresses the separately reported parser and documentation feedback. |
|
In response to #71809 (comment): No code change is needed for this status-only notice, and reported_no_action is not being treated as proof of coverage. Added concrete sweep-provenance and parser-error/debug-redaction regressions and ran the focused suites independently. |
|
In response to #71809 (comment): Accepted the draft for review and finalized the ADR's context, rationale, alternatives, and consequences. It remains Proposed pending maintainer acceptance and is now linked in the PR body. |
|
In response to #71809 (comment): Accepted. Completed the ADR, documented the three alternatives and trade-offs, retained Proposed status, and added its link to the PR body; the existing generated-lock drift limitation remains explicit. |
|
In response to #71809 (comment): Confirmed: this is the earlier response to the status-only Ponytail notice, not a new change request. No additional code or validation is needed for this acknowledgement. |
|
In response to #71809 (comment): Confirmed: commit c4f6cbd contains the three accepted review improvements, and their inline threads are resolved. This acknowledgement adds no further implementation request. |
|
In response to #71809 (comment): Confirmed: the original completion notice and this response require no additional changes. The parser and documentation feedback was handled in the previous batch. |
|
In response to #71809 (comment): Confirmed: the prior batch added and ran the sweep-provenance and diagnostic-redaction regressions. This response is an acknowledgement, not a new test requirement. |
|
In response to #71809 (comment): Confirmed: the finalized proposed ADR is committed and linked in the current PR body. This acknowledgement needs no additional documentation changes. |
|
In response to #71809 (comment): The ADR requirement is already satisfied by the finalized, linked document, including its alternatives and trade-offs. This prior response adds no new work, and the generated-lock drift limitation remains documented. |
|
🎉 This pull request is included in a new release. Release: |
Schema-supported frontmatter forms can fail typed parsing, producing a generic warning and dropping the entire typed configuration. This fixes the shape mismatches found in the public workflow sweep without suppressing genuine invalid-input diagnostics.
Changes
ADR: ADR-71809: Accept Schema-Supported Frontmatter Shapes in Typed Parsing (proposed).
Validation
Build, formatting, focused frontmatter and adjacent credit/safe-job tests, impacted unit tests, and change-scoped custom lint passed. Go lint passed with zero issues. All 348 local workflows compiled with zero generic frontmatter parse warnings; no workflows were executed.
The full validation gate is not green: the workflow drift check reports differences in 27 generated lock files, including upstream-managed workflows. The check restored all lock files, and this PR contains only the parsing fixes, regression tests, and the design decision record. No permissions or checkout-exemption changes are included.