Skip to content
Merged
Show file tree
Hide file tree
Changes from 3 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
63 changes: 49 additions & 14 deletions actions/setup/js/handle_agent_failure.cjs
Original file line number Diff line number Diff line change
Expand Up @@ -55,12 +55,21 @@ const ALLOWED_FILES_ERROR_RE = /^(?<summary>.*outside the allowed-files list) \(

/**
* Parse action failure issue expiration from environment.
* @returns {number} Expiration in hours (defaults to 168 when unset/invalid)
*
* A value of "0" is an explicit signal from the compiler that no maintenance
* workflow will exist to enforce expiration, so expiration must be disabled
* (no expiration marker is written to failure issues). Missing/invalid values
* fall back to the 168-hour default for backwards compatibility with older
* generated lock files that always set a positive value.
* @returns {number} Expiration in hours (0 means disabled; defaults to 168 when unset/invalid)
*/
function getActionFailureIssueExpiresHours() {
const raw = process.env.GH_AW_ACTION_FAILURE_ISSUE_EXPIRES_HOURS || "";
if (raw === "") {
return DEFAULT_ACTION_FAILURE_ISSUE_EXPIRES_HOURS;
}
const parsed = Number.parseInt(raw, 10);
if (Number.isInteger(parsed) && parsed > 0) {
if (Number.isInteger(parsed) && parsed >= 0) {
return parsed;
}
return DEFAULT_ACTION_FAILURE_ISSUE_EXPIRES_HOURS;
Expand Down Expand Up @@ -590,24 +599,50 @@ async function ensureParentIssue(previousParentNumber = null, ownerOverride, rep
const existingIssue = searchResult.data.items[0];
core.info(`Found existing parent issue #${existingIssue.number}: ${existingIssue.html_url}`);

// Check the sub-issue count
const subIssueCount = await getSubIssueCount(owner, repo, existingIssue.number);
// Enforce the parent issue's own expiration marker: an expired parent
// must not keep receiving new sub-issues, mirroring isReusableFailureIssue's
// handling of individual per-run failure issues.
let existingBody;
if (typeof existingIssue.body === "string") {
existingBody = existingIssue.body;
} else {
// The search API response may omit or truncate the body field; fetch the
// full issue to reliably read the expiration marker.
const issueResult = await github.rest.issues.get({
owner,
repo,
issue_number: existingIssue.number,
});
existingBody = issueResult.data.body || "";
}
const parentExpirationDate = extractExpirationDate(existingBody);

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()) {

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.

[/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.

core.info(`Parent issue #${existingIssue.number} has expired (expired ${parentExpirationDate.toISOString()})`);
core.info(`Creating a new parent issue (previous parent #${existingIssue.number} has expired)`);

// Fall through to create a new parent issue, passing the previous parent number
previousParentNumber = existingIssue.number;
Comment on lines 631 to 632
} else {
// Parent issue is within limits, return it
if (subIssueCount !== null) {
core.info(`Parent issue has ${subIssueCount} sub-issues (within limit of ${MAX_SUB_ISSUES})`);
// Check the sub-issue count
const subIssueCount = await getSubIssueCount(owner, repo, existingIssue.number);

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)`);

// Fall through to create a new parent issue, passing the previous parent number
previousParentNumber = existingIssue.number;
} else {
// Parent issue is within limits, return it
if (subIssueCount !== null) {
core.info(`Parent issue has ${subIssueCount} sub-issues (within limit of ${MAX_SUB_ISSUES})`);
}
return {
number: existingIssue.number,
node_id: existingIssue.node_id,
};
}
return {
number: existingIssue.number,
node_id: existingIssue.node_id,
};
}
}
} catch (error) {
Expand Down
63 changes: 61 additions & 2 deletions actions/setup/js/handle_agent_failure.test.cjs
Original file line number Diff line number Diff line change
Expand Up @@ -74,9 +74,12 @@ describe("handle_agent_failure", () => {
expect(getActionFailureIssueExpiresHours()).toBe(48);
});

it("returns default for invalid values", () => {
it("returns 0 (disabled) when the compiler explicitly opts out of expiration", () => {
process.env.GH_AW_ACTION_FAILURE_ISSUE_EXPIRES_HOURS = "0";
expect(getActionFailureIssueExpiresHours()).toBe(168);
expect(getActionFailureIssueExpiresHours()).toBe(0);
});

it("returns default for invalid values", () => {
process.env.GH_AW_ACTION_FAILURE_ISSUE_EXPIRES_HOURS = "invalid";
expect(getActionFailureIssueExpiresHours()).toBe(168);
});
Expand Down Expand Up @@ -704,6 +707,62 @@ describe("handle_agent_failure", () => {
expect(searchMock).toHaveBeenCalledWith(expect.objectContaining({ q: expect.stringContaining('"[aw] Failed runs"') }));
});

it("creates a new parent issue when the existing parent issue has expired", async () => {
const createCommentMock = vi.fn();
const createIssueMock = vi.fn(async ({ title }) => ({
data: {
number: title === "[aw] Failed runs" ? 300 : 301,
html_url: `https://github.com/owner/repo/issues/${title === "[aw] Failed runs" ? 300 : 301}`,
node_id: title === "[aw] Failed runs" ? "I_parent_new" : "I_child",
},
}));
const expiredParentBody = "This issue tracks failures.\n\n> - [x] expires <!-- gh-aw-expires: 2000-01-01T00:00:00.000Z --> on Jan 1, 2000, 12:00 AM UTC";
const searchMock = vi.fn(async ({ q }) => {
if (q.includes("is:pr")) {
return { data: { total_count: 0, items: [] } };
}
if (q.includes('"[aw] Failed runs"')) {
return {
data: {
total_count: 1,
items: [{ number: 199, html_url: "https://github.com/owner/repo/issues/199", node_id: "I_parent_old", body: expiredParentBody }],
},
};
}
return { data: { total_count: 0, items: [] } };
});

process.env.GH_AW_GROUP_REPORTS = "true";

const graphqlMock = vi.fn(async () => ({ repository: { issue: { subIssues: { totalCount: 0 } } } }));

global.github = {
rest: {
search: {
issuesAndPullRequests: searchMock,
},
issues: {
create: createIssueMock,
createComment: createCommentMock,
},
pulls: { get: vi.fn() },
},
graphql: graphqlMock,
};

await main();

// github.rest.issues.create is always invoked with a single options object
// (see github.rest.issues.create({...}) call sites in handle_agent_failure.cjs),
// so destructuring the first call argument yields the options object itself.
const parentCreateCall = createIssueMock.mock.calls.map(([call]) => call).find(call => call.title === "[aw] Failed runs");
expect(parentCreateCall).toBeDefined();
expect(parentCreateCall.body).toContain("previous parent issue #199");
// Expired parent must not be reused: getSubIssueCount must not be queried
// for the expired parent #199, since the expiration check short-circuits first.
expect(graphqlMock).not.toHaveBeenCalledWith(expect.stringContaining("subIssues"), expect.objectContaining({ issueNumber: 199 }));
});

it("escapes workflow IDs before searching for legacy XML marker matches", async () => {
const createCommentMock = vi.fn(async () => ({ data: { id: 1001 } }));
const createIssueMock = vi.fn();
Expand Down
2 changes: 2 additions & 0 deletions docs/src/content/docs/reference/ephemerals.md
Original file line number Diff line number Diff line change
Expand Up @@ -142,6 +142,8 @@ Customize the runner:

`action_failure_issue_expires` sets expiration, in hours, for failure issues opened by the conclusion job, including grouped parent issues when `group-reports: true`. The default is `168` (7 days).

Explicitly setting `action_failure_issue_expires` in `aw.json` is treated as an opt-in: it causes `agentics-maintenance.yml` to be generated (if not already generated by another expiring safe output) so the scheduled `close-expired-entities` job can enforce it. If `action_failure_issue_expires` is left unset and no other workflow output requires scheduled maintenance, the implicit 168-hour default is not written into failure issues, since there would be no scheduled job to close them; failure issues are created without an expiration marker in that case.

If `.github/workflows/aw.json` is present but cannot be loaded, parsed, or validated, compilation keeps the default `168`-hour expiration and emits a warning that identifies the config path and fallback value.

`disabled_jobs` lets you omit specific maintenance jobs from the generated workflow. Job IDs are case-insensitive, and `_` / `-` are treated equivalently.
Expand Down
99 changes: 97 additions & 2 deletions pkg/workflow/maintenance_workflow.go
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ import (
"fmt"
"os"
"path/filepath"
"strconv"
"strings"

"github.com/github/gh-aw/pkg/constants"
Expand Down Expand Up @@ -196,11 +197,18 @@ func GenerateMaintenanceWorkflow(ctx context.Context, opts GenerateMaintenanceWo
runsOnValue := FormatRunsOn(configuredRunsOn, defaultRunsOn)

// Scan workflows for expires fields and track the minimum expires value
hasExpires, minExpires, triggerReason := scanWorkflowsForExpires(workflowDataList)
hasExpires, minExpires, triggerReason := scanWorkflowsForExpires(workflowDataList, repoConfig)

if !hasExpires {
maintenanceLog.Print("No workflows use expires field, skipping maintenance workflow generation")

// No maintenance workflow means no scheduled close-expired-issues consumer.
// Since the implicit 168-hour action-failure default was not an opt-in
// (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)

// Delete existing maintenance workflow file if it exists (no expires means no need for maintenance)
maintenanceFile := filepath.Join(workflowDir, "agentics-maintenance.yml")
if _, err := os.Stat(maintenanceFile); err == nil {
Expand Down Expand Up @@ -341,6 +349,11 @@ func autoUpgradeCronFrom(cfg *RepoConfig) string {
func handleMaintenanceDisabled(workflowDataList []*WorkflowData, workflowDir string) error {
maintenanceLog.Print("Maintenance disabled via repo config, skipping generation")

// Explicit opt-out means no scheduled close-expired-issues consumer will
// exist regardless of any action_failure_issue_expires configuration, so
// disable the runtime expiration marker just like the no-recognized-source case.
disableDefaultActionFailureExpiryMarkers(workflowDataList, workflowDir)

// Warn if any workflow uses expires — those features rely on maintenance
// and will silently become no-ops when it is disabled.
for _, workflowData := range workflowDataList {
Expand Down Expand Up @@ -395,7 +408,15 @@ func allCopilotWorkflowsUseOrgBilling(workflowDataList []*WorkflowData) bool {
// scanWorkflowsForExpires checks all workflow data for expires fields and returns
// whether any expires fields are set, the minimum expires value in hours, and the
// first reason that triggered maintenance workflow generation.
func scanWorkflowsForExpires(workflowDataList []*WorkflowData) (bool, int, string) {
//
// repoConfig may be nil. When maintenance.action_failure_issue_expires is
// explicitly configured in aw.json, it is treated as an opt-in trigger for
// maintenance workflow generation (see IsActionFailureIssueExpiresExplicit).
// The implicit 168-hour default is intentionally excluded from this scan:
// since safe-outputs.report-failure-as-issue defaults to true, treating the
// implicit default as an always-on trigger would force agentics-maintenance.yml
// into essentially every repository with a gh-aw workflow.
func scanWorkflowsForExpires(workflowDataList []*WorkflowData, repoConfig *RepoConfig) (bool, int, string) {
hasExpires := false
minExpires := 0 // Track minimum expires value in hours
triggerReason := ""
Expand Down Expand Up @@ -465,5 +486,79 @@ func scanWorkflowsForExpires(workflowDataList []*WorkflowData) (bool, int, strin
}
}

// Check for an explicitly configured action-failure issue expiry. Unlike the
// implicit 168-hour default, an explicit value is an opt-in and should both
// trigger maintenance workflow generation and participate in the minimum
// expires calculation, but only when some workflow could actually create
// action-failure issues (report-failure-as-issue defaults to true).
if repoConfig.IsActionFailureIssueExpiresExplicit() && anyWorkflowMayReportFailureAsIssue(workflowDataList) {
hasExpires = true
expires := repoConfig.ActionFailureIssueExpiresHours()
setTriggerReason(fmt.Sprintf("maintenance.action_failure_issue_expires=%dh is explicitly configured in %s", expires, RepoConfigFileName))
maintenanceLog.Printf("Repo config explicitly sets action_failure_issue_expires to %d hours", expires)
if minExpires == 0 || expires < minExpires {
minExpires = expires
}
}

return hasExpires, minExpires, triggerReason
}

// disableDefaultActionFailureExpiryMarkers rewrites the already-compiled lock
// files for workflowDataList so that the implicit 168-hour action-failure
// expiration marker is disabled (set to "0", meaning "no expiration"). This is
// called only when scanWorkflowsForExpires determined that no maintenance
// workflow will be generated, so the implicit default (which is not an
// opt-in) must not be advertised in failure issues since nothing would enforce
// it. Workflows that explicitly configured maintenance.action_failure_issue_expires
// are never affected by this function, because an explicit configuration
// always makes scanWorkflowsForExpires report hasExpires=true.
func disableDefaultActionFailureExpiryMarkers(workflowDataList []*WorkflowData, workflowDir string) {
defaultLine := fmt.Sprintf("GH_AW_ACTION_FAILURE_ISSUE_EXPIRES_HOURS: %q", strconv.Itoa(DefaultActionFailureIssueExpiresHours))
disabledLine := `GH_AW_ACTION_FAILURE_ISSUE_EXPIRES_HOURS: "0"`

for _, workflowData := range workflowDataList {
if workflowData == nil || workflowData.WorkflowID == "" {
continue
}
lockFile := filepath.Join(workflowDir, workflowData.WorkflowID+".lock.yml")
content, err := os.ReadFile(lockFile)
if err != nil {
// Lock file may not exist (e.g. --no-emit compiles); nothing to patch.
continue
Comment on lines +523 to +526
}
if !strings.Contains(string(content), defaultLine) {
continue
}
updated := strings.ReplaceAll(string(content), defaultLine, disabledLine)

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.

[/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.

if updated == string(content) {
continue
}
if err := os.WriteFile(lockFile, []byte(updated), 0o644); err != nil {
maintenanceLog.Printf("Warning: failed to disable action-failure expiry marker in %s: %v", lockFile, err)
continue
}
maintenanceLog.Printf("Disabled implicit action-failure expiry marker in %s (no maintenance workflow will enforce it)", lockFile)
}
}

// anyWorkflowMayReportFailureAsIssue returns true unless every workflow in the
// list explicitly disables safe-outputs.report-failure-as-issue. The setting
// defaults to enabled (true) when unset, so an empty or all-nil-SafeOutputs
// list is treated as "may report" as well.
func anyWorkflowMayReportFailureAsIssue(workflowDataList []*WorkflowData) bool {
sawAny := false
for _, workflowData := range workflowDataList {
if workflowData == nil {
continue
}
sawAny = true
if workflowData.SafeOutputs == nil || workflowData.SafeOutputs.ReportFailureAsIssue == nil {
return true
}
if workflowData.SafeOutputs.ReportFailureAsIssue.String() != "false" {
return true
}
}
return !sawAny

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.

[/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 disabled

Add a unit test: scanWorkflowsForExpires(nil, repoConfigWithExplicit) should return hasExpires=false.

@copilot please address this.

}
Loading
Loading