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 |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
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:
var actionFailureExpiryRE = 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 {
return false // no workflows → no failure issues possible
}
return false // 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
let existingBody;
try {
if (typeof existingIssue.body === "string") {
existingBody = existingIssue.body;
} else {
const issueResult = await github.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
func TestGenerateMaintenanceWorkflow_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.
|
@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).
|
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.