Skip to content

Fix on.needs being emitted into compiled workflow on: section - #49864

Merged
pelikhan merged 4 commits into
mainfrom
copilot/fix-invalid-workflow-on-needs
Aug 3, 2026
Merged

Fix on.needs being emitted into compiled workflow on: section#49864
pelikhan merged 4 commits into
mainfrom
copilot/fix-invalid-workflow-on-needs

Conversation

Copilot AI commented Aug 2, 2026

Copy link
Copy Markdown
Contributor

Specifying on.needs in frontmatter caused the needs: key to be serialized into the compiled workflow's on: section, producing invalid GitHub Actions YAML.

Root cause

extractTopLevelYAMLSection marshaled frontmatter["on"] directly to YAML without stripping the needs key, which is a compiler-internal directive — not a valid GitHub Actions on: trigger.

Changes

  • pkg/workflow/frontmatter_extraction_yaml.go — When serializing the on: section, call excludeMapKeys(valueMap, "needs") to create a clean copy before marshaling. The needs data is still consumed downstream by extractOnNeeds to wire job dependencies.
  • pkg/workflow/on_needs_integration_test.go — Assert that needs is absent from the compiled on: section (both via parsed YAML and raw string check).
  • pkg/workflow/yaml_test.go — Add TestExtractTopLevelYAMLSectionExcludesOnNeeds unit test verifying the key is excluded and the original map is not mutated.

Before / after

# Before (invalid)
"on":
  needs:
    - custom_job
  workflow_dispatch: ...

# After (correct)
"on":
  workflow_dispatch: ...

The on.needs job-wiring behaviour for pre_activation/activation/agent is unchanged.


Run: https://github.com/github/gh-aw/actions/runs/30771772506

Generated by 👨‍🍳 PR Sous Chef · gpt54 · 13.5 AIC · ⌖ 5.05 AIC · ⊞ 8.3K ·
Comment /souschef to run again

Copilot AI and others added 2 commits August 2, 2026 22:35
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Copilot AI changed the title [WIP] Fix invalid workflow caused by on.needs property Fix on.needs being emitted into compiled workflow on: section Aug 2, 2026
Copilot AI requested a review from pelikhan August 2, 2026 22:45
@pelikhan
pelikhan marked this pull request as ready for review August 2, 2026 22:47
Copilot AI review requested due to automatic review settings August 2, 2026 22:47
@github-actions

github-actions Bot commented Aug 2, 2026

Copy link
Copy Markdown
Contributor

Test Quality Sentinel completed test quality analysis.

@github-actions

github-actions Bot commented Aug 2, 2026

Copy link
Copy Markdown
Contributor

Design Decision Gate 🏗️ completed the design decision gate check.

No ADR enforcement needed: PR #49864 does not have the 'implementation' label and has 68 new lines of code in business logic directories (threshold: 100).

@github-actions

github-actions Bot commented Aug 2, 2026

Copy link
Copy Markdown
Contributor

🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅

@github-actions

github-actions Bot commented Aug 2, 2026

Copy link
Copy Markdown
Contributor

⚠️ PR Code Quality Reviewer failed during code quality review.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Fixes invalid workflow YAML caused by emitting compiler-only on.needs as a trigger.

Changes:

  • Excludes needs when serializing the on section.
  • Adds unit and integration regression coverage.
Show a summary per file
File Description
pkg/workflow/frontmatter_extraction_yaml.go Removes on.needs before YAML serialization.
pkg/workflow/on_needs_integration_test.go Tests compiled output and dependency wiring.
pkg/workflow/yaml_test.go Tests exclusion without input mutation.

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: 1
  • Review effort level: Balanced

Comment on lines +92 to +95
// Check for needs: inside on: section (as a real key, not a job-level needs)
if inOnSection && strings.HasPrefix(trimmed, "needs:") && !strings.HasPrefix(trimmed, "#") {
return true
}

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Addressed in 06e7928. containsNeedsInOnSection now normalizes an optional leading # and fails if # needs: appears under the compiled on: block.

@github-actions github-actions Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The fix is correct and well-tested. excludeMapKeys returns a new map (no mutation), the on.needs key is stripped only for the on: section, and downstream consumers (extractOnNeeds) are unaffected. Tests cover both the unit-level and integration-level scenarios including a raw-string check.

🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · sonnet46 · 13.7 AIC · ⌖ 10.3 AIC · ⊞ 5.4K

@github-actions github-actions Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Skills-Based Review 🧠

Applied /tdd and /diagnosing-bugs — one minor suggestion on the raw-text test helper, but the fix is correct and well-tested.

📋 Key Themes & Highlights

Key Themes

  • Root cause properly addressed: excludeMapKeys creates a clean copy before marshaling, so the original frontmatter map is not mutated — important for downstream extractOnNeeds consumers.
  • Test coverage good: Both a unit test (TestExtractTopLevelYAMLSectionExcludesOnNeeds) and an integration test assertion cover the fix.
  • One fragility flag: containsNeedsInOnSection raw-text parser could false-positive on job-level needs: keys; the structured YAML assertion already provides sufficient coverage.

Positive Highlights

  • ✅ No mutation of input map — defensive copy via excludeMapKeys
  • ✅ Unit test explicitly verifies map immutability (the mutation guard)
  • ✅ Clear PR description with before/after YAML examples

🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · sonnet46 · 22.5 AIC · ⌖ 8.23 AIC · ⊞ 7.1K
Comment /matt to run again

if trimmed == "on:" || strings.HasPrefix(trimmed, `"on":`) {
inOnSection = true
continue
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[/tdd] containsNeedsInOnSection parses raw YAML text to detect needs: inside on:. A needs: key at job level (e.g. jobs.activation.needs) could also match if the heuristic mis-tracks the section boundary — producing a false positive. The structured map check on line 55 already verifies the fix at the parsed-YAML level; the raw-text check adds fragility without extra safety.

💡 Suggested improvement

Either remove containsNeedsInOnSection (the structured assert on line 55 is sufficient), or tighten it so it cannot match needs: lines deeply indented inside jobs:. A comment explaining why both checks coexist would also help future readers.

@copilot please address this.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Addressed in 06e7928. I kept the raw-text regression check, but tightened it to only consider direct child keys within on: so it catches commented-out # needs: lines without matching unrelated nested/job-level needs: entries.

@github-actions

github-actions Bot commented Aug 2, 2026

Copy link
Copy Markdown
Contributor

🧪 Test Quality Sentinel Report

Test Quality Score: 87/100 — Excellent

Analyzed 9 test(s): 9 design, 0 implementation, 0 violation(s).

📊 Metrics (9 tests)
Metric Value
Analyzed 9 (Go: 9, JS: 0)
✅ Design 9 (100%)
⚠️ Implementation 0 (0%)
Edge/error coverage 8 (89%)
Duplicate clusters 0
Inflation NO
🚨 Violations 0
Test File Classification Issues
TestOnNeedsCompilesAndWiresActivationDependencies on_needs_integration_test.go behavioral_contract, design_test, high_value
TestExtractTopLevelYAMLSectionExcludesOnNeeds yaml_test.go behavioral_contract, design_test, high_value
TestExtractTopLevelYAMLSectionWithOrdering yaml_test.go behavioral_contract, design_test, high_value
TestCleanYAMLNullValues yaml_test.go behavioral_contract, design_test, high_value
TestUnquoteYAMLKey yaml_test.go behavioral_contract, design_test, high_value
TestUnquoteYAMLTopLevelKey yaml_test.go behavioral_contract, design_test, high_value
TestMarshalWithFieldOrder yaml_test.go behavioral_contract, design_test, high_value
TestMarshalWithFieldOrder_OrdersNestedEnvWithSecretsRecursively yaml_test.go behavioral_contract, design_test, high_value
TestFormatYAMLValue yaml_test.go behavioral_contract, design_test, high_value

Key Observations

  • TestOnNeedsCompilesAndWiresActivationDependencies (integration): End-to-end test for the bug fix. Compiles a real workflow with on.needs, then asserts the compiled on: section does not contain needs: (negative assertion), while pre_activation and activation jobs do depend on the on.needs job (positive assertion). Includes a helper containsNeedsInOnSection for raw YAML text validation. Strong behavioral contract.
  • TestExtractTopLevelYAMLSectionExcludesOnNeeds (unit): Tests the invariant at the function level — on.needs is stripped from the compiled on: section, workflow_dispatch is preserved, and the original frontmatter map is not mutated. Side-effect safety check is an excellent design invariant.
  • TestExtractTopLevelYAMLSectionWithOrdering: Table-driven; verifies alphabetical field ordering for both on: and permissions: sections. Parses generated YAML and checks line order, which is a strong structural assertion.
  • TestFormatYAMLValue: Comprehensive 25-case table-driven test covering all type variants (bool, int, uint, float, nil, struct) and quoting edge cases for YAML reserved keywords.
  • TestMarshalWithFieldOrder_OrdersNestedEnvWithSecretsRecursively: Verifies recursive alphabetical ordering across deeply nested maps (steps > with > env), using position-based assertions rather than just equality.
  • All tests carry proper build tags (//go:build integration and //go:build !integration), descriptive t.Errorf messages, and zero mock library usage.

Verdict

passed. 0% implementation tests (threshold: 30%). No violations. Tests directly encode the bug fix as a behavioral contract at both the unit and integration levels.

🧪 Test quality analysis by Test Quality Sentinel · sonnet46 · 39.1 AIC · ⌖ 8.37 AIC · ⊞ 8.4K ·
Comment /review to run again

@github-actions github-actions Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

✅ Test Quality Sentinel: 87/100. 0% implementation tests (threshold: 30%).

@github-actions github-actions Bot mentioned this pull request Aug 2, 2026
@gh-aw-bot

Copy link
Copy Markdown
Collaborator

@copilot Please take a quick pass on the remaining PR hygiene items, then run the pr-finisher skill.

Notes to check before merging:

Branch refresh may help once you are ready.

Generated by 👨‍🍳 PR Sous Chef · gpt54 · 13.5 AIC · ⌖ 5.05 AIC · ⊞ 8.3K ·
Comment /souschef to run again

Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>

Copilot AI commented Aug 2, 2026

Copy link
Copy Markdown
Contributor Author

@copilot Please take a quick pass on the remaining PR hygiene items, then run the pr-finisher skill.

Notes to check before merging:...

Handled in 06e7928. I tightened the raw on: helper, checked the prior PR Code Quality Reviewer failure (the failing run was the old review pass, not a reproducible code/test failure), and re-ran local make fmt, make lint, make test-unit, and make test. CI on this new head is still stale until a maintainer re-triggers it.

Copilot AI requested a review from gh-aw-bot August 2, 2026 23:46
@pelikhan
pelikhan merged commit 78efb61 into main Aug 3, 2026
29 checks passed
@pelikhan
pelikhan deleted the copilot/fix-invalid-workflow-on-needs branch August 3, 2026 00:02
@github-actions

github-actions Bot commented Aug 3, 2026

Copy link
Copy Markdown
Contributor

🎉 This pull request is included in a new release.

Release: v0.84.3

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Including on.needs for custom jobs results in invalid workflow

4 participants