Uh oh!
There was an error while loading. Please reload this page.
Suppress unenforceable action-failure expiry markers when no maintenance workflow will exist to close them - #51425
Conversation
Thanks for the detailed plan on action-failure expiry! 👋 This is a comprehensive issue addressing a real defect in maintenance workflow generation. Since this is in WIP stage, here's what you'll want to focus on as the implementation progresses:
The plan looks well-structured and directly addresses the root causes identified in issue #50923. Once you've implemented the changes and added tests, this should be ready to go!
|
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
❌ Design Decision Gate 🏗️ failed during design decision gate check.
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅
|
✅ PR Code Quality Reviewer completed the code quality review. Warning Threat Detection Engine Failure — The analysis engine could not complete. This is a tooling failure, not a security finding. What happenedThe threat detection engine failed to produce results. Review the workflow run logs for details. Warning Firewall blocked 1 domainThe following domain was blocked by the firewall during workflow execution:
To allow these domains, add them to the network:
allowed:
- defaults
- "api.individual.githubcopilot.com"See Network Configuration for more information.
|
✅ Test Quality Sentinel completed test quality analysis.
|
There was a problem hiding this comment.
Pull request overview
Aligns action-failure expiration markers with available maintenance cleanup.
Changes:
- Detects explicit expiry configuration and generates maintenance accordingly.
- Disables unenforceable markers and handles expired grouped parents.
- Adds regression tests and documentation.
Show a summary per file
| File | Description |
|---|---|
pkg/workflow/repo_config.go | Tracks explicit expiry configuration. |
pkg/workflow/repo_config_test.go | Tests explicit-setting detection. |
pkg/workflow/maintenance_workflow.go | Coordinates maintenance generation and marker suppression. |
pkg/workflow/maintenance_workflow_test.go | Tests expiry scheduling and marker behavior. |
docs/src/content/docs/reference/ephemerals.md | Documents opt-in expiration semantics. |
actions/setup/js/handle_agent_failure.cjs | Supports disabled expiration and expired parents. |
actions/setup/js/handle_agent_failure.test.cjs | Tests runtime expiration behavior. |
Review details
Tip
Add a code-review agent skill for context-aware, tailored reviews. Learn more in the docs.
- Files reviewed: 7/7 changed files
- Comments generated: 6
- Review effort level: Balanced
| // Scan workflows for expires fields and track the minimum expires value | ||
| hasExpires, minExpires, triggerReason := scanWorkflowsForExpires(workflowDataList) | ||
| hasExpires, minExpires, triggerReason := scanWorkflowsForExpires(workflowDataList, repoConfig) |
| return true | ||
| } | ||
| } | ||
| return !sawAny |
| // (see scanWorkflowsForExpires), disable the runtime expiration marker in | ||
| // the already-compiled lock files so failure issues do not claim an | ||
| // expiration that nothing will enforce. | ||
| disableDefaultActionFailureExpiryMarkers(workflowDataList, workflowDir) |
| const parsed = Number.parseInt(raw, 10); | ||
| if (Number.isInteger(parsed) && parsed > 0) { | ||
| if (Number.isInteger(parsed) && parsed >= 0) { | ||
| return parsed; |
| // Fall through to create a new parent issue, passing the previous parent number | ||
| previousParentNumber = existingIssue.number; |
| content, err := os.ReadFile(lockFile) | ||
| if err != nil { | ||
| // Lock file may not exist (e.g. --no-emit compiles); nothing to patch. | ||
| continue |
There was a problem hiding this comment.
The changes are well-structured and the logic is sound.
- The
getActionFailureIssueExpiresHoursearly-return for empty string beforeparseIntis a correctness improvement — avoidsparseInt("", 10)returningNaN, good. disableDefaultActionFailureExpiryMarkersuses a specific env var string match, making false-positive replacements very unlikely.anyWorkflowMayReportFailureAsIssueconservatively returnstruefor empty/nil workflow lists — correct safe default.- The double-unmarshal in
UnmarshalJSON(struct + map for presence detection) is idiomatic and functionally correct. - Test coverage is thorough across all new branches.
No blocking issues found.
🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · sonnet46 · 29.2 AIC · ⌖ 9.93 AIC · ⊞ 5.5K
🧪 Test Quality Sentinel Report✅ Test Quality Score: 92/100 — Excellent
📊 Metrics (16 tests)
Test Quality HighlightsStrengths:
Key regression protections:
Verdict
|
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /diagnosing-bugs and /tdd — requesting changes on three targeted issues.
📋 Key Themes & Highlights
Key Themes
- Lock-file patch fragility:
strings.ReplaceAllis coupled to the exact numeric value ofDefaultActionFailureIssueExpiresHours; a regex match on any non-zero value would be more robust. - Empty workflow list edge case:
anyWorkflowMayReportFailureAsIssuereturnstruefor an empty list, so an explicitaction_failure_issue_expireswith no workflows would incorrectly trigger maintenance generation. - Unhandled network error in parent-issue body fetch:
github.rest.issues.getinsideensureParentIssuehas no try/catch; a transient error aborts grouped-issue handling entirely. - Missing test for
handleMaintenanceDisabledpatch path:disableDefaultActionFailureExpiryMarkersis called from two code sites but only one is covered end-to-end.
Positive Highlights
- ✅ Excellent compile/runtime separation: the compiler patches
0into lock files and the runtime honours it — avoids runtime config lookups. - ✅ The
ActionFailureIssueExpiresExplicitapproach (double-unmarshal to detect key presence) is clean and minimally invasive. - ✅ Strong test coverage added for
scanWorkflowsForExpiresopt-in semantics — all four combination cases are exercised. - ✅ Parent-issue expiry check correctly short-circuits before the graphql sub-issue count query, reducing API calls on the expired path.
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · sonnet46 · 54.2 AIC · ⌖ 8.03 AIC · ⊞ 7.1K
Comment /matt to run again
| if !strings.Contains(string(content), defaultLine) { | ||
| continue | ||
| } | ||
| updated := strings.ReplaceAll(string(content), defaultLine, disabledLine) |
There was a problem hiding this comment.
[/diagnosing-bugs]strings.ReplaceAll matches the exact string GH_AW_ACTION_FAILURE_ISSUE_EXPIRES_HOURS: "168". If DefaultActionFailureIssueExpiresHours ever changes, lock files compiled with the old default will silently be missed — no patch, no warning, unenforceable marker lives on.
💡 Suggestion
Match the non-zero value with a regex instead:
varactionFailureExpiryRE=regexp.MustCompile(`GH_AW_ACTION_FAILURE_ISSUE_EXPIRES_HOURS: "[1-9]\d*"`)
// ...updated:=actionFailureExpiryRE.ReplaceAllString(string(content), `GH_AW_ACTION_FAILURE_ISSUE_EXPIRES_HOURS: "0"`)This patches any positive default regardless of its numeric value, and removes the coupling to DefaultActionFailureIssueExpiresHours at patch-time.
@copilot please address this.
| return true | ||
| } | ||
| } | ||
| return !sawAny |
There was a problem hiding this comment.
[/diagnosing-bugs]anyWorkflowMayReportFailureAsIssue returns !sawAny (i.e. true) when the list is empty. An empty workflowDataList passed to scanWorkflowsForExpires would therefore treat an explicit action_failure_issue_expires as an opt-in trigger even though there are no workflows to create failure issues — generating a maintenance workflow for no reason.
💡 Suggestion
Return false (not true) when sawAny is false — an empty list cannot produce failure issues:
if!sawAny {
returnfalse// no workflows → no failure issues possible
}
returnfalse// all workflows explicitly disabledAdd a unit test: scanWorkflowsForExpires(nil, repoConfigWithExplicit) should return hasExpires=false.
@copilot please address this.
| if (subIssueCount !== null && subIssueCount >= MAX_SUB_ISSUES) { | ||
| core.warning(`Parent issue #${existingIssue.number} has ${subIssueCount} sub-issues (max: ${MAX_SUB_ISSUES})`); | ||
| core.info(`Creating a new parent issue (previous parent #${existingIssue.number} is full)`); | ||
| if (parentExpirationDate && parentExpirationDate.getTime() <= Date.now()) { |
There was a problem hiding this comment.
[/diagnosing-bugs] The expiration check uses <= Date.now() (expired at or before now), which is correct. However, the branch that fetches the full issue body on a truncated search result has no error handling — if github.rest.issues.get throws (e.g. rate-limit, network error), the exception propagates uncaught up through ensureParentIssue, potentially aborting the entire grouped-issue flow rather than gracefully falling back to creating a new parent.
💡 Suggestion
letexistingBody;try{if(typeofexistingIssue.body==="string"){existingBody=existingIssue.body;}else{constissueResult=awaitgithub.rest.issues.get({ owner, repo,issue_number: existingIssue.number});existingBody=issueResult.data.body||"";}}catch(err){core.warning(`Could not fetch body for parent issue #${existingIssue.number}: ${err.message}. Treating as unexpired.`);existingBody="";}Treating a fetch failure as "not expired" is conservative and mirrors the existing pattern of falling through to sub-issue count checks when data is unavailable.
@copilot please address this.
| require.Contains(t, string(preserved), `GH_AW_ACTION_FAILURE_ISSUE_EXPIRES_HOURS: "168"`) | ||
| } | ||
| func TestGenerateMaintenanceWorkflow_CreatesWorkflowDirRecursively(t *testing.T) { |
There was a problem hiding this comment.
[/tdd] The new test TestGenerateMaintenanceWorkflow_DisablesImplicitActionFailureExpiryMarker only verifies the no-maintenance path. A corresponding test for the case where maintenance: false is explicitly set (the handleMaintenanceDisabled code path) is missing — disableDefaultActionFailureExpiryMarkers is called from two sites but only one is tested end-to-end.
💡 Suggested test outline
funcTestGenerateMaintenanceWorkflow_DisablesImplicitMarkerWhenMaintenanceDisabled(t*testing.T) {
tmpDir:=t.TempDir()
// write lock file with implicit 168h marker// call GenerateMaintenanceWorkflow with RepoConfig{Maintenance: {Disabled: true}}// assert marker patched to "0"// assert agentics-maintenance.yml does not exist
}This closes the gap and ensures both call sites of disableDefaultActionFailureExpiryMarkers are covered.
@copilot please address this.
gh-aw-bot
commented
Aug 8, 2026
@copilot Quick triage nudge for this PR. Please refresh the branch if needed, re-check the remaining maintainer-facing feedback, run the Open review context (newest first):
Run: https://github.com/github/gh-aw/actions/runs/31269773406
|
Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
Addressed in Follow-up fixes pushed:
Validation run locally: |
PR Triage
Addresses unenforceable action-failure expiry markers when no maintenance workflow exists to close them; adds compile-time opt-in logic and runtime fixes with new regression tests + docs update. Sizable diff (568/-). CI mostly green, some jobs still in progress at triage time. AI reviewer approved (one prior review dismissed).
|
🎉 This pull request is included in a new release. Release: |
<!-- Please let the below note in for people that find this PR --> > [!NOTE] > Are you waiting for the changes in this PR to be merged? > It would be very helpful if you could [test the resulting artifacts](https://github.com/dotnet/maui/wiki/Testing-PR-Builds) from this PR and let us know in a comment if this change resolves your issue. Thank you! ## Summary - regenerate all 14 Copilot-based agentic workflows with gh-aw v0.86.2 - stage the installed Copilot CLI in a portable runner-local path instead of invoking `/usr/local/bin/copilot` - explicitly disable shell access in the automatic issue/PR labeler, as required when `min-integrity: none` - adopt gh-aw v0.86.2's intentional disabled-expiry sentinel for action-failure issues when no maintenance workflow exists ## Root cause The failing runs install Copilot CLI successfully, but gh-aw v0.85.4 then invokes `/usr/local/bin/copilot`. That path no longer exists on the hosted runner, so the engine exits with `ENOENT` before producing output or making a model request. gh-aw v0.86.2 includes the upstream path-portability fix and resolves the installed executable from `PATH`. The same generated path affected every Copilot workflow, including the labeler, whose failure reporting is intentionally disabled. Regeneration also incorporates [github/gh-aw#51425](github/gh-aw#51425). Because this repository does not generate an `agentics-maintenance.yml` consumer for the implicit action-failure expiry, all 14 lockfiles now set `GH_AW_ACTION_FAILURE_ISSUE_EXPIRES_HOURS` to `"0"`. In gh-aw v0.86.2, explicit `0` means expiration is disabled; it prevents unenforceable expiry markers and does not make failure issues expire immediately. ## Validation - `gh aw compile --strict --validate --actionlint --no-emit` - verified all 14 lockfiles use gh-aw v0.86.2 and portable Copilot staging - verified no generated workflow references `/usr/local/bin/copilot` - verified all 14 generated locks use gh-aw's `"0"` disabled-expiry sentinel consistently No existing open MAUI PR addresses this gh-aw/Copilot path failure. Fixes#37430Fixes#37431Fixes#37432Fixes#37435Fixes#37442 --------- Co-authored-by: Copilot CI <copilot-ci@microsoft.com> Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 74cff858-a5a6-486e-a1d3-1d516fc3bdaa Copilot-Session: d00747b7-96f3-4e7a-8dfb-e3a48db04b2d
Action-failure issues get a 168-hour expiration marker regardless of whether
agentics-maintenance.yml(the workflow that closes expired issues) is actually generated.scanWorkflowsForExpiresonly considers explicit safe-output/no-op expiry, so the implicit action-failure default silently produces issues with an expiry deadline nobody enforces. Grouped parent failure issues also carried the marker but their reuse path never checked it.Compile-time: opt-in maintenance generation
RepoConfignow tracks whethermaintenance.action_failure_issue_expireswas explicitly set inaw.json(vs. left at the implicit default) — required a secondary raw-JSON pass since Go can't distinguish "absent" from "explicit zero" viaomitempty.scanWorkflowsForExpirestreats an explicitaction_failure_issue_expiresas an opt-in trigger foragentics-maintenance.yml, folding it into the min-expires scheduling calculation when any workflow may report failures as issues.maintenance: false), the already-compiled lock file'sGH_AW_ACTION_FAILURE_ISSUE_EXPIRES_HOURSis patched from168down to0— there being no scheduled consumer to enforce it. The marker is left untouched when another expiry source already triggers maintenance.Runtime: honor disabled state and expire parent issues
getActionFailureIssueExpiresHours()inhandle_agent_failure.cjsnow treats an explicit"0"as "expiration disabled" instead of falling back to the 168h default; only a missing/invalid value falls back.ensureParentIssue()now checks the existing grouped parent issue's expiration marker before reusing it (previously only sub-issue count was checked), falling through to create a new parent chained to the expired one — mirroring the existing per-run issue reuse logic.{ "maintenance": { "action_failure_issue_expires": 72 } }Setting this explicitly now generates
agentics-maintenance.ymlif nothing else would; leaving it unset with no other expiring safe output means failure issues are created without a marker instead of an unenforceable one.Docs
ephemerals.mdupdated to describe the opt-in semantics ofaction_failure_issue_expires.Out of scope: side-repository (
failure-issue-repo) maintenance coverage is left as a follow-up.