Repository navigation
Suppress missing-terminal-output failures for cancelled agent jobs - #69366
Conversation
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅
|
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. See the comment below for the result and any generated ADR draft. Design Decision Gate 🏗️Outcome: The agent completed and reported no action. A no-op output alone does not verify that all required work was completed or that publication was possible. Agent messages are omitted from this comment. The category uses structured run status, not interpretation of agent prose.
|
|
✅ Ponytail Reviewer completed successfully! Ponytail ReviewerOutcome: The agent completed and reported no action. A no-op output alone does not verify that all required work was completed or that publication was possible. Agent messages are omitted from this comment. The category uses structured run status, not interpretation of agent prose.
|
|
✅ Test Quality Sentinel completed test quality analysis. Test Quality SentinelOutcome: The agent completed and reported no action. A no-op output alone does not verify that all required work was completed or that publication was possible. Agent messages are omitted from this comment. The category uses structured run status, not interpretation of agent prose.
|
|
🔎 PR Code Quality Reviewer is reviewing code quality for this pull request... |
There was a problem hiding this comment.
🟡 Changes recommended
Mixed-output cancellations still propagate suppressed diagnostics into failure issue titles, categories, and bodies.
1 open finding
What changed in this PR
Adjusts gh-aw conclusion handling to suppress missing-terminal-output failures for cancelled agent jobs.
Changes:
- Filters cancellation-related incomplete signals while retaining other failure checks.
- Adds regression coverage for cancellation and preserved failure reporting.
| File | Description |
|---|---|
| actions/setup/js/handle_agent_failure.test.cjs | Tests cancellation suppression and preserved failure reporting. |
| actions/setup/js/handle_agent_failure.cjs | Filters missing-terminal incomplete signals for cancelled agents. |
🧠 Review effort: Max
💡 Add a code-review agent skill for context-aware, tailored reviews. Learn more in the docs.
| // Cancellation can interrupt the agent before it emits a terminal safe output. | ||
| const reportIncompleteItems = agentOutputResult.items.filter(item => item.type === "report_incomplete" && !(agentConclusion === "cancelled" && item.reason === "missing_terminal_safe_output")); |
There was a problem hiding this comment.
Fixed in 92b4606: collector-correlated missing-terminal diagnostics and classification are removed from issue reporting, while the independent missing-tool failure remains.
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /diagnosing-bugs and /tdd — requesting changes on a trust-boundary gap in the fix.
📋 Key Themes & Highlights
Key Themes
- Trust boundary violated: The new filter matches on the agent-controlled
item.reasonfield directly, bypassing the file's own documented invariant ("Only the collector-written root metadata is trusted; report_incomplete.reason is agent-controlled", line 4409) used elsewhere for the same purpose viacollectorEmptyOutputCause. As written, a manually emittedreport_incomplete({reason: "missing_terminal_safe_output"})on a cancelled run is silently suppressed even without collector corroboration. - Test coverage reinforces the gap rather than catching it: the
withCollectorMetadata: falseparametrized case asserts the suppression succeeds, which is the risky path, not a safe one. - Root cause is correctly identified — collector-synthesized incomplete signals on cancellation are a real false-positive source, and the regression matrix for success/failure/timed_out/other-reason cases is solid.
Positive Highlights
- ✅ Good instinct to add regression coverage across all relevant
agentConclusionvalues, not just the cancelled case. - ✅ The accompanying comment explaining why cancellation can interrupt terminal output emission is clear and helpful for future readers.
See inline comments for the concrete one-line fix (switch to collectorEmptyOutputCause) and the test reframing needed to lock in the trust boundary.
@copilot please address the review comments above.
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · copilot · sonnet50 · 95 AIC · ⌖ 15 AIC · ⊞ 10.1K
Comment /matt to run again
| if (agentOutputResult.success && agentOutputResult.items && agentOutputResult.items.length > 0) { | ||
| const reportIncompleteItems = agentOutputResult.items.filter(item => item.type === "report_incomplete"); | ||
| // Cancellation can interrupt the agent before it emits a terminal safe output. | ||
| const reportIncompleteItems = agentOutputResult.items.filter(item => item.type === "report_incomplete" && !(agentConclusion === "cancelled" && item.reason === "missing_terminal_safe_output")); |
There was a problem hiding this comment.
[/diagnosing-bugs] This filter trusts the agent-controlled item.reason field directly, which contradicts the invariant this same file documents at line 4409 — "Only the collector-written root metadata is trusted; report_incomplete.reason is agent-controlled." An agent (or injected content causing the agent) could emit a manual report_incomplete({reason: "missing_terminal_safe_output"}) call, and a cancelled run would now silently suppress that signal even though it wasn't collector-synthesized.
💡 Suggested fix
Use the trusted agentOutputResult.collectorEmptyOutputCause (already available further down at line 4416) instead of the agent-supplied item.reason:
const collectorEmptyOutputCause = agentOutputResult.collectorEmptyOutputCause;
const reportIncompleteItems = agentOutputResult.items.filter(
item => item.type === "report_incomplete" && !(agentConclusion === "cancelled" && collectorEmptyOutputCause === "missing_terminal_safe_output" && item.reason === collectorEmptyOutputCause)
);This keeps the root-cause fix (ignore synthesized empty-output signals on cancellation) while closing the gap where a manual/malicious report_incomplete call with a matching reason string on a cancelled run would be silently dropped.
@copilot please address this.
There was a problem hiding this comment.
Fixed in 92b4606: suppression now requires the trusted collectorEmptyOutputCause; an agent-supplied matching reason without collector corroboration still triggers failure handling.
| expect(createCommentMock).not.toHaveBeenCalled(); | ||
| }); | ||
|
|
||
| it.each([true, false])("skips cancelled agents with missing terminal output (collector metadata: %s)", async withCollectorMetadata => { |
There was a problem hiding this comment.
[/tdd] The withCollectorMetadata: false case actually demonstrates the bug the fix introduces, not a safe scenario: it proves a bare agent-supplied report_incomplete({reason: "missing_terminal_safe_output"}) on a cancelled run is suppressed with zero collector corroboration.
💡 Suggested test framing
Rename/refocus this parametrized case so it documents the trust boundary rather than asserting the suppression is correct, e.g.:
it("does NOT suppress agent-supplied report_incomplete without collector corroboration", async () => {
// No collectorEmptyOutputCause present — this item is agent-controlled only.
// Failure handling should still activate.
expect(createIssueMock).toHaveBeenCalled();
});See the companion inline comment on handle_agent_failure.cjs line 4208 for the root-cause fix that makes this test pass against trusted collector metadata instead of the raw item.reason.
@copilot please address this.
There was a problem hiding this comment.
Fixed in 92b4606: the no-metadata case now asserts agent-authored incomplete output still triggers reporting; collector-backed cancellation and mixed-output behavior are covered separately.
|
@copilot address the following outstanding work in one pass:
Push the necessary fixes, reply to each listed review thread and resolve it when addressed. Ignore feedback already answered or resolved. Use the pr-finisher skill and stop when only human review or CI remains; do not trigger CI. Sous-chef head: 1657bd3
|
…-agent-jobs Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
Jobs cancelled by
engine.concurrency.cancel-in-progresscan carry a collector-synthesizedreport_incompletesignal, incorrectly failing conclusion handling and opening a failure issue.report_incompleteitems with reasonmissing_terminal_safe_outputwhen the agent conclusion iscancelled. Preserve other incomplete reports and independent failure checks.engine.concurrency.cancel-in-progressare reported asmissing_terminal_safe_outputfailures (regression since v0.91.x) #69050