Fail workflows when agents report incomplete - #59316
Conversation
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. See the comment below for the result and any generated ADR draft. No ADR enforcement needed: PR does not have the 'implementation' label and has ≤100 new lines of code in business logic directories.
|
|
✅ Test Quality Sentinel completed test quality analysis. Test Quality Sentinel skipped because pre-fetch PR data was unavailable: unable to fetch test file diff
|
|
✅ Ponytail Reviewer completed successfully! 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
- "ab.chatgpt.com"See Network Configuration for more information.
|
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅ 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
- "registry.npmjs.org"See Network Configuration for more information.
|
|
✅ PR Code Quality Reviewer completed the code quality review. 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
- "github.com"See Network Configuration for more information.
|
Comment MemoryNote This comment is managed by comment memory.It stores persistent context for this thread in the code block at the top of this comment. 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
- "github.com"See Network Configuration for more information.
|
There was a problem hiding this comment.
Requesting changes
This change fixes the workflow conclusion bug, but the PR as pushed also strips the executable bit from a dozen existing .sh/.py entrypoints outside the stated diff. That is a runtime-breaking change for any workflow or skill that invokes those helpers directly via their shebangs.
Blocking theme
Restoring the expected 100755 mode on those command scripts is required before this can merge. Leaving them at 100644 turns runnable helpers into non-executable files and will cause Permission denied failures in automation paths that execute them by path.
Warning
Firewall blocked 1 domain
The following domain was blocked by the firewall during workflow execution:
github.com
To allow these domains, add them to the network.allowed list in your workflow frontmatter:
network:
allowed:
- defaults
- "github.com"See Network Configuration for more information.
🔎 Code quality review by PR Code Quality Reviewer · copilot · gpt54 · 41.3 AIC · ⌖ 7.32 AIC · ⊞ 23.5K
Comment /review to run again
There was a problem hiding this comment.
🟡 Changes recommended
The test isolation and registration-only exception coverage issues should be addressed before approval.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
Ensures meaningful report_incomplete signals fail Actions jobs independently of optional issue reporting.
Changes:
- Marks incomplete agent reports as failed.
- Adds regression coverage for issue-reporting scenarios.
- Adds a patch changeset.
File summaries
| File | Review |
|---|---|
actions/setup/js/handle_agent_failure.test.cjs |
Moderate: restore GH_AW_FAILURE_REPORT_AS_ISSUE after the test to prevent test leakage. |
actions/setup/js/handle_agent_failure.cjs |
Moderate: verify setFailed is not called for registration-only reports. |
.changeset/fix-report-incomplete-conclusion.md |
Nit: document the registration-only exception. |
Review details
Suppressed comments (1)
.changeset/fix-report-incomplete-conclusion.md:5
- This release note states that every
report_incompletefails the workflow, but the implementation intentionally preserves the task-completion registration-only exception. Mention that exception (or qualify this as a meaningful signal) so the published behavior matches the code.
Fail the workflow conclusion when an agent emits `report_incomplete`, even when the agent process succeeds or failure issue reporting is disabled.
- Files reviewed: 3/3 changed files
- Comments generated: 2
- Review effort level: Balanced
💡 Add a code-review agent skill for context-aware, tailored reviews. Learn more in the docs.
| } | ||
| // Continue after marking the step failed so optional issue reporting can still run. | ||
| // eslint-disable-next-line gh-aw-custom/require-return-after-core-setfailed | ||
| core.setFailed("Agent reported that it could not complete the task"); |
| vi.resetModules(); | ||
| const { main: mainFn } = require("./handle_agent_failure.cjs"); | ||
| await mainFn(); |
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /diagnosing-bugs (fallback heuristic used for skill selection — triage skipped for this small, well-scoped diff).
📋 Analysis
Root cause: Correctly addressed. core.setFailed() now fires as soon as a first-class report_incomplete signal is detected (line ~3582), independent of whether failure-issue reporting later succeeds, fails, or is disabled — closing the gap where a green job could mask a reported incomplete task (#59274).
Control flow check: Traced all early-return paths after the new setFailed() call (reportFailureAsIssue disabled → return; checkoutPRSuccess === "false" → return; hasCompletedDespiteJobFailure && !hasReportIncomplete → return). All occur after the new call, so the failed state is never silently dropped.
Lint suppression: The require-return-after-core-setfailed custom rule is deliberately disabled with an inline justification comment, since execution must continue to attempt optional issue reporting. This is a legitimate, well-documented use of the escape hatch.
Regression tests: Two new cases cover exactly the previously-missing scenarios — issue creation throwing after a report_incomplete signal, and failure-issue reporting disabled entirely — both asserting core.setFailed was still invoked with the expected message.
No actionable issues found. Approving.
Warning
Firewall blocked 1 domain
The following domain was blocked by the firewall during workflow execution:
registry.npmjs.org
To allow these domains, add them to the network.allowed list in your workflow frontmatter:
network:
allowed:
- defaults
- "registry.npmjs.org"See Network Configuration for more information.
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · copilot · sonnet50 · 93.8 AIC · ⌖ 14.6 AIC · ⊞ 10.3K
Comment /matt to run again
There was a problem hiding this comment.
Reviewed with Impeccable harden/audit lenses (bug-fix change type: failure-conclusion propagation logic).
No blocking issues found. The change is minimal and well-scoped:
core.setFailed()is only invoked for genuinereport_incompletesignals (task-registration-only signals are still correctly excluded viataskCompleteRegistrationIssueOnlyReportIncomplete).- Execution intentionally continues after
setFailed()so optional issue reporting still runs — documented with a justifiedeslint-disable-next-linecomment. - New tests cover both edge cases: issue creation failing after
setFailed, and issue reporting being disabled entirely. - Confirmed the step (
handle_agent_failure) runs in theconclusionjob, socore.setFailed()correctly fails that job/step without impacting the JS bundle inlining (no.lock.ymlrecompile needed since scripts are loaded fromrunner.tempat runtime, not inlined).
Approving.
Warning
Firewall blocked 2 domains
The following domains were blocked by the firewall during workflow execution:
github.laiyagushi.comregistry.npmjs.org
To allow these domains, add them to the network.allowed list in your workflow frontmatter:
network:
allowed:
- defaults
- "github.com"
- "registry.npmjs.org"See Network Configuration for more information.
🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · copilot · sonnet50 · 112.5 AIC · ⌖ 13.3 AIC · ⊞ 8.3K
|
🎉 This pull request is included in a new release. Release: |
A successful agent process could emit
report_incompletewhile leaving the Actions job green. Failure-issue reporting could run or fail independently without propagating the incomplete state to the job conclusion.Failure propagation
core.setFailed()when a meaningfulreport_incompletesignal is detected.Regression coverage
report_incompleteleaves workflow green when the agent exits 0 #59274