Analyzing agentic_maintenance.yml generation issues - #50107
Conversation
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
|
@copilot review changes and finish work |
Reviewed and verified the work in What the change does:
Validation: |
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅ |
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. No ADR enforcement needed: PR does not have the 'implementation' label and has ≤100 new lines of code in business logic directories (47 additions in default business logic directories, threshold is 100). |
|
✅ Test Quality Sentinel completed test quality analysis. |
|
✅ PR Code Quality Reviewer completed the code quality review. |
There was a problem hiding this comment.
Pull request overview
Prevents implicit noop defaults from unnecessarily generating maintenance workflows.
Changes:
- Tracks whether
noopwas implicitly added. - Skips maintenance generation for implicit
noopconfigurations and adds tests/docs. - Regenerates a workflow lock file with OIDC environment exclusions.
Show a summary per file
| File | Description |
|---|---|
pkg/workflow/safe_outputs_config_extraction.go |
Marks default noop as implicit. |
pkg/workflow/noop.go |
Adds internal implicit-state metadata. |
pkg/workflow/maintenance_workflow.go |
Ignores implicit noop during expiration scanning. |
pkg/workflow/maintenance_workflow_test.go |
Tests implicit and explicit noop behavior. |
docs/src/content/docs/reference/ephemerals.md |
Documents lazy maintenance generation. |
.github/workflows/squad-game-planner.lock.yml |
Excludes Actions OIDC variables from sandbox environments. |
Review details
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 6/6 changed files
- Comments generated: 1
- Review effort level: Balanced
| t.Run("implicit noop does not trigger maintenance", func(t *testing.T) { | ||
| trueVal := "true" | ||
| hasExpires, minExpires, triggerReason := scanWorkflowsForExpires([]*WorkflowData{ | ||
| { | ||
| Name: "implicit-noop", | ||
| SafeOutputs: &SafeOutputsConfig{ | ||
| NoOp: &NoOpConfig{ | ||
| ReportAsIssue: &trueVal, | ||
| Implicit: true, | ||
| }, | ||
| }, | ||
| }, | ||
| }) | ||
| require.False(t, hasExpires) | ||
| require.Equal(t, 0, minExpires) | ||
| require.Empty(t, triggerReason) | ||
| }) |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /diagnosing-bugs and /tdd — one minor observation, no blocking issues.
📋 Key Themes & Highlights
Key Themes
- Root cause properly addressed: The fix correctly distinguishes implicit (auto-injected) from explicit (user-authored) noop configurations using a dedicated
Implicit boolfield, rather than patching the symptom. - Good regression test coverage: Two test cases cover both the broken case (implicit noop must not trigger) and the valid case (explicit noop must trigger). The spec is clear and well-named.
- Documentation is accurate: The new paragraph in
ephemerals.mdcorrectly describes the lazy generation semantics. - Lock file security hygiene: Excluding
ACTIONS_ID_TOKEN_REQUEST_TOKENandACTIONS_ID_TOKEN_REQUEST_URLfrom the agent container is a solid security improvement.
Minor Observation
- The
"explicit noop triggers maintenance"test subcase skips asserting thetriggerReasonstring, leaving a gap that a future message-string change could silently exploit. See inline comment.
Positive Highlights
- ✅
Implicit bool \yaml:"-"`` prevents accidental YAML serialization of an internal field - ✅ Comment in
maintenance_workflow.goexplains why the guard exists, not just what it does - ✅ Both test cases use table-driven style consistent with the rest of the file
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · sonnet46 · 29.2 AIC · ⌖ 7.9 AIC · ⊞ 7.1K
Comment /matt to run again
| }) | ||
|
|
||
| t.Run("explicit noop triggers maintenance", func(t *testing.T) { | ||
| trueVal := "true" |
There was a problem hiding this comment.
[/tdd] The "explicit noop triggers maintenance" test does not assert triggerReason content — only hasExpires and minExpires are checked.
💡 Suggested addition
Adding an assertion on triggerReason ensures a future refactor of the message string does not silently pass:
require.Contains(t, triggerReason, "explicit-noop")
require.Contains(t, triggerReason, "no-op issue reporting")@copilot please address this.
There was a problem hiding this comment.
The changes are clean and correct.
Implicit boolfield withyaml:"-"tag correctly prevents serialization while carrying the flag through compilation- The guard
!workflowData.SafeOutputs.NoOp.Implicitis precisely placed to skip maintenance workflow generation for auto-injected noops - Two new tests cover both the implicit (no trigger) and explicit (triggers) cases
- Documentation update in
ephemerals.mdaccurately describes the lazy-generation behavior - Lock file update in
squad-game-planner.lock.ymladds the two missing--exclude-envflags forACTIONS_ID_TOKEN_REQUEST_TOKENandACTIONS_ID_TOKEN_REQUEST_URL
No blocking issues found.
🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · sonnet46 · 17.4 AIC · ⌖ 13.2 AIC · ⊞ 5.4K
🧪 Test Quality Sentinel Report✅ Test Quality Score: 82/100 — Excellent
📊 Metrics (2 tests)
Verdict
|
|
@copilot run pr-finisher skill |
…al struct Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
There was a problem hiding this comment.
Blocking issue: the Implicit gate on noop maintenance-workflow generation creates orphaned, never-expiring issues for the common case (implicit noop with default report-as-issue=true).
💡 Themes
- Regression risk (high):
NoOp.Implicitdistinguishes config provenance (user authorednoop:vs. auto-injected), but issue-creation behavior (ReportAsIssuedefaulting totrue) is unconditional on that flag. Gating maintenance-workflow generation onImplicittherefore disables cleanup for a class of repos that still actively create noop-reported issues — see inline comment. - Test coverage for the new flag is good at the
scanWorkflowsForExpiresunit level, but the import-merge path (imports.go) and full compile pipeline are not covered for the new field, so an incorrect merge across imported files would go undetected. - Two generated
.lock.ymldiffs (env-exclude additions) look like unrelated regen noise, not reviewed as core logic changes here.
Recommend gating on whether report-as-issue was itself explicitly authored (or simply keep the original NoOp != nil check and only suppress it if ReportAsIssue was never enabled), rather than gating on the whole config's implicit/explicit provenance.
🔎 Code quality review by PR Code Quality Reviewer · auto · 138.1 AIC · ⌖ 3.85 AIC · ⊞ 7.9K
Comment /review to run again
| // Only explicitly configured noop outputs trigger maintenance generation: | ||
| // the implicit default must not pollute repositories with a maintenance | ||
| // workflow they never asked for. | ||
| if workflowData.SafeOutputs.NoOp != nil && !workflowData.SafeOutputs.NoOp.Implicit { |
There was a problem hiding this comment.
This silently orphans noop-reported issues: implicit noop configs still default ReportAsIssue=true and create issues, but now never get a maintenance workflow to expire/close them.
💡 Why this is a regression
extractSafeOutputsConfig (safe_outputs_config_extraction.go) sets config.NoOp.ReportAsIssue = &trueVal for the auto-injected implicit noop fallback — issue creation is enabled by default regardless of whether the config is implicit or explicit. Before this PR, scanWorkflowsForExpires triggered maintenance-workflow generation for any NoOp config (implicit or not) as long as report-as-issue was enabled, so those issues would eventually be closed/expired.
This change adds !workflowData.SafeOutputs.NoOp.Implicit to the gate, which skips maintenance generation for the common case (a workflow with safe-outputs: present but no explicit noop: block — likely the majority of workflows). The result: any repo relying on the implicit noop fallback will now silently accumulate noop-reported issues with no automated cleanup mechanism ever generated, which is a worse outcome for those repos than the pre-PR behavior.
A more correct fix would gate on whether the user explicitly enabled report-as-issue (or explicitly authored report-as-issue: true), not on whether the whole noop: block was authored — since issue creation itself is unconditional on Implicit.
// still creates issues by default here (safe_outputs_config_extraction.go):
config.NoOp.ReportAsIssue = &trueVal // Default to reporting to issue
config.NoOp.Implicit = true
// ...but maintenance generation is now skipped for the same config:
if workflowData.SafeOutputs.NoOp != nil && !workflowData.SafeOutputs.NoOp.Implicit {
|
@copilot run pr-finisher skill |
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Addressed in a05cd4e / 916da06: the regression test now builds the noop config via |
|
@copilot resolve the merge conflicts on this branch. |
…ntic-maintenance-cause # Conflicts: # .github/workflows/daily-assign-issue-to-user.lock.yml Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
|
@copilot PR #50107 still needs follow-up before a maintainer can investigate quickly.
Run: https://github.com/github/gh-aw/actions/runs/30864142711
|
This was already resolved in eff1564 — the branch is merged with |
|
🎉 This pull request is included in a new release. Release: |
Pull request created by AI Agent