Harden report_incomplete issue handling when incomplete_signals is missing/empty#47533
Conversation
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
incomplete_signals is missing/empty
|
Great work! 👍 This PR looks ready to merge. ✅ What looks good:
This is a solid hardening change for the agentic workflows infrastructure that prevents
|
There was a problem hiding this comment.
Pull request overview
Hardens incomplete-report issue creation when signal arrays are absent or empty.
Changes:
- Adds configurable fallback item synthesis.
- Adds generic incomplete-signal fallback behavior.
- Updates tool guidance and regression tests.
Show a summary per file
| File | Description |
|---|---|
actions/setup/js/safe_outputs_tools.json |
Clarifies report_incomplete guidance. |
actions/setup/js/missing_issue_helpers.cjs |
Supports fallback item builders. |
actions/setup/js/missing_issue_helpers.test.cjs |
Tests fallback behavior. |
actions/setup/js/create_report_incomplete_issue.cjs |
Synthesizes a generic incomplete signal. |
actions/setup/js/create_report_incomplete_issue.test.cjs |
Covers missing and empty signal arrays. |
Review details
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 5/5 changed files
- Comments generated: 1
- Review effort level: Medium
| { | ||
| "name": "report_incomplete", | ||
| "description": "Signal that the task could not be completed due to an infrastructure or tool failure (e.g., MCP server crash, missing authentication, inaccessible repository). Use this when required tools or data are unavailable and the task cannot be meaningfully performed. This is distinct from noop (no action needed) — it indicates an active failure that prevented the task from running. The workflow framework will treat this as a failure signal even when the agent exits successfully.", | ||
| "description": "Signal that the task could not be completed due to an infrastructure or tool failure (e.g., MCP server crash, missing authentication, inaccessible repository). Use this when required tools or data are unavailable and the task cannot be meaningfully performed. This is distinct from noop (no action needed) — it indicates an active failure that prevented the task from running. Provide a specific reason and optional details so downstream issue aggregation can preserve complete incomplete-signal context. The workflow framework will treat this as a failure signal even when the agent exits successfully.", |
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅ |
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. No ADR enforcement needed: PR does not have the 'implementation' label and has 0 new lines of code in business logic directories. |
|
✅ PR Code Quality Reviewer completed the code quality review. |
|
✅ Test Quality Sentinel completed test quality analysis. |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review: Harden report_incomplete issue handling
The logic is sound and the test coverage is good. One existing inline comment already flags the blocking issue:
pkg/workflow/js/safe_outputs_tools.json is not updated. The report_incomplete description was updated only in actions/setup/js/safe_outputs_tools.json, but the compiler-embedded copy at pkg/workflow/js/safe_outputs_tools.json still has the old text. The TestSafeOutputsToolsJSONInSync test will fail until that copy is also updated.
All other changes look correct:
fallbackItemswiring inmissing_issue_helpers.cjsis clean and correctly avoids false errors.- Fallback items are well-formed (reason, details, timestamp).
- Tests cover both missing and empty signal paths.
🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · sonnet46 16.2 AIC · ⌖ 4.51 AIC · ⊞ 5K
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /diagnosing-bugs and /tdd — requesting changes on two issues.
📋 Key Themes & Highlights
Blocking Issues
- Sync gap:
pkg/workflow/js/safe_outputs_tools.jsonstill has the oldreport_incompletedescription —TestSafeOutputsToolsJSONInSyncwill fail on CI. - Test coverage gap: the
message?.reasonbranch infallbackItemsis never exercised; the fallbackdetailsstring is also unasserted.
Positive Highlights
- ✅ Clean extension of
buildMissingIssueHandlervia optionalfallbackItems— existing callers are unaffected. - ✅ Good defensive checks: missing/empty/non-array all redirect to fallback.
- ✅ New test file follows the established mock pattern from
missing_issue_helpers.test.cjs.
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · sonnet46 23.7 AIC · ⌖ 4.74 AIC · ⊞ 6.7K
Comment /matt to run again
| { | ||
| "name": "report_incomplete", | ||
| "description": "Signal that the task could not be completed due to an infrastructure or tool failure (e.g., MCP server crash, missing authentication, inaccessible repository). Use this when required tools or data are unavailable and the task cannot be meaningfully performed. This is distinct from noop (no action needed) — it indicates an active failure that prevented the task from running. The workflow framework will treat this as a failure signal even when the agent exits successfully.", | ||
| "description": "Signal that the task could not be completed due to an infrastructure or tool failure (e.g., MCP server crash, missing authentication, inaccessible repository). Use this when required tools or data are unavailable and the task cannot be meaningfully performed. This is distinct from noop (no action needed) — it indicates an active failure that prevented the task from running. Provide a specific reason and optional details so downstream issue aggregation can preserve complete incomplete-signal context. The workflow framework will treat this as a failure signal even when the agent exits successfully.", |
There was a problem hiding this comment.
[/diagnosing-bugs] The report_incomplete description update was applied only to the runtime copy (actions/setup/js/safe_outputs_tools.json) but not to the compiler-embedded copy at pkg/workflow/js/safe_outputs_tools.json. TestSafeOutputsToolsJSONInSync will fail until both files are in sync.
💡 Fix
Apply the same description change to pkg/workflow/js/safe_outputs_tools.json (the report_incomplete entry), so both copies stay identical and the sync test passes.
@copilot please address this.
| it("gracefully handles missing incomplete_signals", async () => { | ||
| const handler = await main({}); | ||
| const result = await handler({ workflow_name: "Test Workflow", run_url: "https://github.com/test-owner/test-repo/actions/runs/123" }); | ||
|
|
There was a problem hiding this comment.
[/tdd] The test passes {}} as the message to main() (the config factory), not to the handler itself. The handler receives the message with workflow_name and run_url in the second call. This is correct — but the test name says "gracefully handles missing incomplete_signals" without asserting what details appears in the issue body, only that incomplete_signal_not_provided appears. A missing assertion on the fallback details field (which uses message?.reason) means a regression there would go undetected.
💡 Suggested addition
Add an assertion that the issue body contains the fallback details string when message.reason is absent:
expect(mockGithub.rest.issues.create).toHaveBeenCalledWith(
expect.objectContaining({
body: expect.stringContaining("Missing or empty incomplete_signals array"),
})
);And a second test where reason is provided so the message?.reason branch is exercised.
@copilot please address this.
🧪 Test Quality Sentinel Report✅ Test Quality Score: 82/100 — Excellent
📊 Metrics (25 tests)
📋 Test BreakdownFile:
File: Validation Tests (8): Missing workflow_name, fallback GH_AW_WORKFLOW_NAME, message precedence, fallback GH_AW_WORKFLOW_SOURCE_URL, construct run_url from env, missing items field, empty items array, non-array items Fallback Logic (2): Use fallbackItems when field missing, use fallbackItems when empty Max Count (2): Enforce limit, allow higher limits Config Resolution (2): Default title prefix, override with config.title_prefix Issue Lifecycle (7): Add comment to existing issue, render item in comment, create new issue, apply labels (string and array), merge defaultLabels with config labels, deduplicate overlapping labels Error Handling (1): GitHub API errors return success: false with error message All 25 tests are design tests verifying behavioral contracts. 3 implementation details (label merging, comment rendering patterns) justified by critical business logic. No mocking violations detected.
|
| File | Test LOC | Prod LOC | Ratio | Status |
|---|---|---|---|---|
create_report_incomplete_issue.test.cjs |
78 | 51 | 1.53:1 | ✅ Pass |
missing_issue_helpers.test.cjs |
463 | 241 | 1.92:1 | ✅ Pass |
Analysis: Both files under 2:1 threshold individually. Test setup boilerplate (mocks, template creation) is necessary for isolated vitest unit testing. High test count (25) justified by coverage of critical fallback paths, validation logic, and error handling—precisely what the PR intends to harden.
Verdict
✅ Passed. 12% implementation tests (threshold: 30%). No violations. Tests verify user-visible behavior: missing signal handling, fallback resolution, issue search/creation, label management, and error resilience. The two new tests in
create_report_incomplete_issue.test.cjsdirectly validate the PR's hardening objective.
🧪 Test quality analysis by Test Quality Sentinel · haiku45 14 AIC · ⌖ 7.58 AIC · ⊞ 7.1K · ◷
Comment /review to run again
There was a problem hiding this comment.
❌ Request changes — 3 issues must be fixed before merge
The fallback hardening logic in missing_issue_helpers.cjs is sound, but the new test file has critical problems that prevent it from running at all, and the fallback detail field reads a non-existent property.
Blocking issues
- Critical — ESM
importin.cjsfile (create_report_incomplete_issue.test.cjs): The file uses ES module syntax but has a.cjsextension; Node.js will throwSyntaxErrorat load time and the tests will never execute. - High — module-scope side effects with no cleanup (same file):
mkdirSync,writeFileSync, andprocess.envmutation run at import time with noafterAllteardown, leaking state across test workers. - Medium — dead
message?.reasonbranch (create_report_incomplete_issue.cjsline 21):reasonis not a field on the message object; the expression always resolves toundefined, making the fallback detail string always the hardcoded constant regardless of caller input.
🔎 Code quality review by PR Code Quality Reviewer · sonnet46 34.2 AIC · ⌖ 4.95 AIC · ⊞ 5.7K
Comment /review to run again
Comments that could not be inline-anchored
actions/setup/js/create_report_incomplete_issue.test.cjs:2
ESM import syntax in a .cjs file will crash at runtime: Node.js unconditionally parses .cjs files as CommonJS; the import statements will throw SyntaxError: Cannot use import statement in a CommonJS module and the test suite will fail to load.
<details>
<summary>💡 Suggested fix</summary>
Rename the file to .test.mjs (and update the vitest config glob if needed), or rewrite imports as require():
// keep .cjs — convert to require
const { describe, it, expect, beforeEac…
</details>
<details><summary>actions/setup/js/create_report_incomplete_issue.test.cjs:12</summary>
**Module-scope filesystem writes and env mutation have no teardown**: `mkdirSync`, `writeFileSync`, and `process.env.GH_AW_PROMPTS_DIR =` run once at import time and are never cleaned up, polluting the OS temp dir and leaking the env var into any test module loaded in the same worker process.
<details>
<summary>💡 Suggested fix</summary>
Move setup into lifecycle hooks and restore state afterward:
```js
let promptsDir;
beforeAll(() => {
promptsDir = fs.mkdtempSync(path.join(os.tmpdir(), "g…
</details>
<details><summary>actions/setup/js/create_report_incomplete_issue.cjs:21</summary>
**`message?.reason` reads a field that doesn't exist on this message type**: The top-level message object carries `workflow_name`, `run_url`, and `incomplete_signals` — not `reason`. This expression always evaluates to `undefined`, silently discarding any caller-supplied context and falling back to the hardcoded string unconditionally.
<details>
<summary>💡 Suggested fix</summary>
If the goal is to surface a human-readable description from the caller, use an actual field that exists on the me…
</details>
|
@copilot please run the Run: https://github.com/github/gh-aw/actions/runs/29998171888
|
Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
Fixed in the latest commit: updated |
|
🎉 This pull request is included in a new release. Release: |
safe_outputscould fail after a successful agent run whencreate_report_incomplete_issuewas emitted without a non-emptyincomplete_signalsarray. This change makes that path resilient by defaulting malformed payloads instead of failing batch processing.Validation/handler hardening
buildMissingIssueHandlerwith optionalfallbackItemssupport.create_report_incomplete_issuefallback behaviorfallbackItemsforcreate_report_incomplete_issue.incomplete_signalsnow resolves to a single generic signal:reason: "incomplete_signal_not_provided"details: preserved from message reason when availabletimestamp: generated at handling timePrompt/tooling guidance alignment
report_incompletetool description to emphasize providing specific reason/details so downstream incomplete-signal aggregation remains high fidelity.Regression coverage
incomplete_signalsin the report-incomplete issue path.