Copilot harness: terminate idle process after terminal safe output#44440
Conversation
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅ |
|
✅ PR Code Quality Reviewer completed the code quality review. |
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. No ADR enforcement needed: PR #44440 does not have the 'implementation' label and has 0 new lines of code in business logic directories (threshold: 100). |
|
✅ Test Quality Sentinel completed test quality analysis. |
There was a problem hiding this comment.
Pull request overview
This pull request updates the agent harness process lifecycle so that Copilot CLI sessions don’t hang until the GitHub Actions step hard-timeout after already producing terminal safe output. It adds a “post-result” inactivity watchdog to the shared process runner and wires it into the Copilot harness using safe-output–based terminal detection.
Changes:
- Added
postResultWatchdogsupport torunProcess(...)to terminate idle child processes after an arming condition becomes true. - Introduced
hasTerminalSafeOutput(...)and integrated it into Copilot harness execution to arm the watchdog once terminal safe output is detected. - Added unit tests covering watchdog termination behavior and terminal safe-output detection.
Show a summary per file
| File | Description |
|---|---|
| actions/setup/js/process_runner.cjs | Adds optional post-result inactivity watchdog logic to the shared subprocess runner. |
| actions/setup/js/process_runner.test.cjs | Adds tests validating watchdog termination and non-arming behavior. |
| actions/setup/js/copilot_harness.cjs | Adds terminal safe-output detection and wires the post-result watchdog into Copilot execution. |
| actions/setup/js/copilot_harness.test.cjs | Adds unit tests for hasTerminalSafeOutput(...). |
Review details
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 4/4 changed files
- Comments generated: 4
- Review effort level: Low
| const watchdogPollIntervalMs = Math.max(50, Number(postResultWatchdog?.pollIntervalMs) || 1000); | ||
| const watchdogTermGraceMs = Math.max(50, Number(postResultWatchdog?.termGraceMs) || 5000); | ||
| const watchdogInactivityTimeoutMs = Math.max(50, Number(postResultWatchdog?.inactivityTimeoutMs) || 0); |
| if (sentSigtermAt > 0 && Date.now() - sentSigtermAt >= watchdogTermGraceMs) { | ||
| log(`attempt ${attempt + 1}: post-result watchdog forcing process exit after ${watchdogTermGraceMs}ms grace (SIGKILL)`); | ||
| child.kill("SIGKILL"); | ||
| } |
🧪 Test Quality Sentinel Report✅ Test Quality Score: 82/100 — Excellent
📊 Metrics (5 tests)
Verdict✅ Passed. 0% implementation tests (threshold: 30%). All tests verify behavioral contracts with strong edge-case coverage. Strengths:
Technical notes:
Warning Firewall blocked 1 domainThe following domain was blocked by the firewall during workflow execution:
network:
allowed:
- defaults
- "awmgmcpg"See Network Configuration for more information.
|
Review SummaryREQUEST_CHANGES — 4 issues flagged across 📋 Issues to address
@copilot please address the review comments above.
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /diagnosing-bugs, /tdd, and /codebase-design — requesting changes on correctness and coverage gaps.
📋 Key Themes & Highlights
Key Issues
missing_datanot inNON_TERMINAL_SAFE_OUTPUT_TYPES—evaluate_outcomes.cjstreatsmissing_dataas diagnostic (noop), but this function does not, so an agent emitting onlymissing_dataentries will arm the watchdog and be killed prematurely.- Watchdog-killed run may trigger unintended retries — when the process is killed with a non-zero exit code after a real task output (non-noop), the retry loop enters the full error classifier with no guard for "terminal output already present."
- SIGKILL escalation path is untested — the SIGTERM test uses a process that exits on SIGTERM; the SIGKILL branch is never exercised.
> 0guard at line 135 is logically unreachable —Math.max(50, NaN || 0)always yields ≥ 50 whenpostResultWatchdogis present.
Positive Highlights
- ✅ Clean separation:
hasTerminalSafeOutputis a pure function, easy to test and reuse. - ✅ SIGTERM→SIGKILL escalation with configurable grace period is the right pattern for forced shutdown.
- ✅
shouldArmcallback makes the watchdog loosely coupled — good interface design. - ✅ Bounded timing minimums (50 ms floor) prevent accidental zero-delay loops.
- ✅ Good test coverage for the happy paths; the watchdog "not armed" test correctly validates the safety guard.
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · 106.3 AIC · ⌖ 6.6 AIC · ⊞ 6.6K
Comment /matt to run again
| process.stderr.write(`[copilot-harness] ${message}\n`); | ||
| } | ||
|
|
||
| const NON_TERMINAL_SAFE_OUTPUT_TYPES = new Set(["missing_tool", "report_incomplete"]); |
There was a problem hiding this comment.
[/diagnosing-bugs] missing_data is absent from NON_TERMINAL_SAFE_OUTPUT_TYPES, so the watchdog arms prematurely when an agent emits only missing_data entries.
evaluate_outcomes.cjs defines NOOP_TYPES = new Set(["noop", "missing_tool", "missing_data", "report_incomplete"]) — treating missing_data as diagnostic. This function must stay in sync with that definition.
💡 Suggested fix
const NON_TERMINAL_SAFE_OUTPUT_TYPES = new Set(["missing_tool", "missing_data", "report_incomplete"]);Without this fix an agent that emits only {type: "missing_data"} will arm the watchdog and get killed after 20 s of inactivity even though the run is purely diagnostic.
@copilot please address this.
| } | ||
| : undefined, | ||
| }); | ||
| lastExitCode = result.exitCode; |
There was a problem hiding this comment.
[/diagnosing-bugs] When the watchdog kills the process, result.exitCode will be non-zero (signal) and the retry loop will treat it as a transient failure — potentially re-running the agent even though a terminal safe output was already written.
The noop guard at line 1044 will catch the pure-noop case, but an agent that emits comment_issue or another real action type before stalling has no guard: it will enter the full retry classifier.
💡 Suggested fix
Check for terminal safe output at the top of the retry classifier (after result.exitCode is read) before expensive error detection:
if (result.exitCode !== 0 && safeOutputsPath && hasTerminalSafeOutput(safeOutputsPath)) {
log(`attempt ${attempt + 1}: terminal safe output present — treating watchdog-killed run as success`);
lastExitCode = 0;
break;
}This prevents a retry after a successful task output when the process lingered and was killed by the watchdog.
@copilot please address this.
| const parsed = JSON.parse(trimmed); | ||
| const type = parsed && typeof parsed.type === "string" ? parsed.type : ""; | ||
| if (!type) continue; | ||
| if (type === "noop" || !NON_TERMINAL_SAFE_OUTPUT_TYPES.has(type)) { |
There was a problem hiding this comment.
[/diagnosing-bugs] The noop special-case in the condition is redundant — noop is not in NON_TERMINAL_SAFE_OUTPUT_TYPES, so !NON_TERMINAL_SAFE_OUTPUT_TYPES.has('noop') is already true. The explicit check obscures the intent.
💡 Suggested simplification
if (!NON_TERMINAL_SAFE_OUTPUT_TYPES.has(type)) {
return true;
}Removing the type === "noop" arm makes the logic self-documenting: "return true for any type not in the non-terminal set."
@copilot please address this.
| } | ||
| }); | ||
|
|
||
| it("returns false for diagnostic-only output", () => { |
There was a problem hiding this comment.
[/tdd] Missing test: a JSONL file containing a mix of diagnostic-only entries followed by a terminal entry should return true, and vice versa. The current tests only cover single-entry files, leaving the multi-entry scan path untested.
💡 Suggested additional tests
it('returns true when terminal entry follows diagnostic entries', () => {
const lines = [
JSON.stringify({ type: 'missing_tool' }),
JSON.stringify({ type: 'noop', reason: 'done' }),
].join('\n');
fs.writeFileSync(filePath, lines, 'utf8');
expect(hasTerminalSafeOutput(filePath)).toBe(true);
});
it('returns false when file contains only multiple diagnostic entries', () => {
const lines = [
JSON.stringify({ type: 'missing_tool' }),
JSON.stringify({ type: 'report_incomplete' }),
].join('\n');
fs.writeFileSync(filePath, lines, 'utf8');
expect(hasTerminalSafeOutput(filePath)).toBe(false);
});@copilot please address this.
| termGraceMs: 200, | ||
| }, | ||
| }); | ||
| expect(result.exitCode).not.toBe(0); |
There was a problem hiding this comment.
[/tdd] result.exitCode is asserted as not.toBe(0) — but when killed by SIGKILL on Linux, code in the close event is null and process_runner.cjs coerces it to 1. This assertion passes today but doesn't document the expected code clearly; toBe(1) would make the contract explicit.
Also missing: a test that verifies the SIGKILL escalation path (i.e., when SIGTERM is sent but the process ignores it and termGraceMs expires). The current test uses a simple setInterval that terminates on SIGTERM, so the SIGKILL branch is never exercised.
💡 Suggested SIGKILL test
it('escalates to SIGKILL when process ignores SIGTERM', async () => {
const logs = [];
const result = await runProcess({
command: process.execPath,
args: ['-e', 'process.on("SIGTERM", () => {}); setInterval(() => {}, 500);'],
attempt: 0,
log: msg => logs.push(msg),
postResultWatchdog: {
shouldArm: () => true,
inactivityTimeoutMs: 50,
pollIntervalMs: 25,
termGraceMs: 150,
},
});
expect(logs.some(l => l.includes('SIGKILL'))).toBe(true);
expect(result.exitCode).toBe(1);
});```
</details>
@copilot please address this.| let sentSigtermAt = 0; | ||
| const watchdogPollIntervalMs = Math.max(50, Number(postResultWatchdog?.pollIntervalMs) || 1000); | ||
| const watchdogTermGraceMs = Math.max(50, Number(postResultWatchdog?.termGraceMs) || 5000); | ||
| const watchdogInactivityTimeoutMs = Math.max(50, Number(postResultWatchdog?.inactivityTimeoutMs) || 0); |
There was a problem hiding this comment.
[/codebase-design] Math.max(50, Number(...) || 0) silently accepts NaN and 0 from misconfigured callers because Number(undefined) is NaN, and NaN || 0 is 0 — so watchdogInactivityTimeoutMs becomes 50 even when no value is passed, bypassing the guard at line 135 (> 0) only because the minimum is already 50.
The behaviour is correct incidentally, but the guard logic at line 135 is now unreachable for any caller — watchdogInactivityTimeoutMs will always be ≥ 50. Consider documenting the invariant or restructuring so the check at line 135 is meaningful.
💡 Context
\n\nIf you intend the watchdog to be disabled when `inactivityTimeoutMs` is absent, initialise the variable to `0` and apply `Math.max(50, ...)" only after confirming a value was provided:\n\n```js\nconst watchdogInactivityTimeoutMs =\n postResultWatchdog?.inactivityTimeoutMs != null\n ? Math.max(50, Number(postResultWatchdog.inactivityTimeoutMs))\n : 0;\n```\n\nThis makes the disable-path (`=== 0`) reachable again and the guard at line 135 meaningful.\n\n
There was a problem hiding this comment.
REQUEST_CHANGES — two correctness issues must be fixed before merge
This PR solves a real problem (hung agent processes after terminal output) and the implementation is structurally sound. However two logic bugs in the new code must be addressed:
🔴 Issues found
1. High — retry loop fires on non-noop watchdog kills (harness, line 1044)
When the watchdog terminates the process after detecting a non-noop terminal safe-output (e.g. a PR review comment was submitted), the retry guard only checks hasNoopInSafeOutputs. It returns false, so the harness falls through to the generic result.hasOutput retry at line 1179 and re-runs the agent up to maxRetries times — potentially emitting duplicate PR comments, issues, or other side effects. See inline comment for the fix.
2. Medium — idle window starts before arm, not after (process_runner, line 143)
The arm check and the idle-threshold check execute in the same setInterval tick. When the process is already quiet at the moment the watchdog arms, SIGTERM fires instantly on that same tick. The logged inactivityTimeout=20000ms message is then misleading and the intended grace window never actually runs. Reset lastActivityAt = Date.now() when watchdogArmed flips to true to give the process its full 20 s window.
🔎 Code quality review by PR Code Quality Reviewer · 178.4 AIC · ⌖ 6.68 AIC · ⊞ 5.4K
Comment /review to run again
Comments that could not be inline-anchored
actions/setup/js/copilot_harness.cjs:1044
Retry guard gap: watchdog kills on non-noop terminal outputs bypass the noop check, causing the harness to retry an already-completed agent.
<details>
<summary>💡 Details and suggested fix</summary>
When the watchdog fires because a non-noop terminal safe-output was written (e.g. comment_issue, submit_pull_request_review, etc.), runProcess returns with a non-zero exit code (SIGTERM/SIGKILL exit) and result.hasOutput = true.
The retry guard at this line only calls `hasNoop…
actions/setup/js/process_runner.cjs:143
Zero-grace idle window on arm: if the process is already silent when the watchdog arms, SIGTERM fires immediately on the same poll tick — the logged 20 s idle timeout is misleading.
<details>
<summary>💡 Details and suggested fix</summary>
The arm check and the idle check run in the same setInterval callback with no separation:
// Same tick:
watchdogArmed = postResultWatchdog.shouldArm(); // flips to true
if (watchdogArmed) {
log('... armed inactivityTimeout=20000ms ...'); /…
</details>
There was a problem hiding this comment.
Review: Copilot harness watchdog
The approach is sound — a post-result inactivity watchdog is the right fix for processes that stay alive after emitting a terminal safe output. The implementation is clean and the tests cover the core paths well.
One correctness issue needs addressing before merge:
missing_data not in NON_TERMINAL_SAFE_OUTPUT_TYPES
missing_data is a diagnostic/non-actionable output type, consistent with how evaluate_outcomes.cjs treats it (NOOP_TYPES line 44 includes it alongside missing_tool and report_incomplete). The new NON_TERMINAL_SAFE_OUTPUT_TYPES set omits it, so an agent that emits only missing_data would incorrectly arm the watchdog and get terminated — even though no real task output was produced.
Detail
evaluate_outcomes.cjs:44:NOOP_TYPES = new Set(["noop", "missing_tool", "missing_data", "report_incomplete"])safeoutputs_cli.cjs:159:DIAGNOSTIC_SAFE_OUTPUT_TYPES = new Set(["noop", "missing_tool", "report_incomplete"])— also missingmissing_databut that is a pre-existing inconsistency- This PR's
NON_TERMINAL_SAFE_OUTPUT_TYPESshould includemissing_datato be consistent withevaluate_outcomes.cjs - A test case for
missing_datain thehasTerminalSafeOutputtest suite would close the coverage gap
🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · 116 AIC · ⌖ 6.2 AIC · ⊞ 4.8K
| process.stderr.write(`[copilot-harness] ${message}\n`); | ||
| } | ||
|
|
||
| const NON_TERMINAL_SAFE_OUTPUT_TYPES = new Set(["missing_tool", "report_incomplete"]); |
There was a problem hiding this comment.
missing_data is missing from NON_TERMINAL_SAFE_OUTPUT_TYPES, which will cause hasTerminalSafeOutput() to return true when an agent emits only a missing_data entry — incorrectly arming the post-result watchdog.
evaluate_outcomes.cjs classifies missing_data alongside missing_tool and report_incomplete in NOOP_TYPES (line 44), treating it as a diagnostic/non-actionable output. This PR's set should do the same:
const NON_TERMINAL_SAFE_OUTPUT_TYPES = new Set(["missing_tool", "report_incomplete", "missing_data"]);Without this fix, an agent that only emits missing_data would have its process terminated early by the watchdog even though no real work output was produced.
@copilot please address this.
|
@copilot run pr-finisher skill |
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
|
🚀 Smoke Pi MISSION COMPLETE! Pi delivered. 🥧 |
|
🚀 Smoke Antigravity MISSION COMPLETE! Antigravity has spoken. ✨ |
|
✅ All tools validated successfully! Agent Container Smoke Test confirms agent container is ready. |
|
✨ The prophecy is fulfilled... Smoke Codex has completed its mystical journey. The stars align. 🌟 |
|
🎬 THE END — Smoke Claude MISSION: ACCOMPLISHED! The hero saves the day! ✨ |
|
🚀 Smoke Gemini MISSION COMPLETE! Gemini has spoken. ✨ |
|
❌ Smoke Copilot Small failed. Please review the logs for details. |
|
📰 BREAKING: Smoke Copilot is now investigating this pull request. Sources say the story is developing... |
|
📰 BREAKING: Smoke Copilot - AOAI (Entra) is now investigating this pull request. Sources say the story is developing... |
|
📰 BREAKING: Smoke Copilot - AOAI (apikey) is now investigating this pull request. Sources say the story is developing... |
Agent Container Tool Check
Result: 12/12 tools available ✅ Overall Status: PASS Warning Firewall blocked 1 domainThe following domain was blocked by the firewall during workflow execution:
network:
allowed:
- defaults
- "awmgmcpg"See Network Configuration for more information.
|
Smoke Test Results
Overall Status: FAIL Warning Firewall blocked 1 domainThe following domain was blocked by the firewall during workflow execution:
network:
allowed:
- defaults
- "localhost"See Network Configuration for more information.
|
|
Smoke test: FAIL Warning Firewall blocked 6 domainsThe following domains were blocked by the firewall during workflow execution:
network:
allowed:
- defaults
- "accounts.google.com"
- "android.clients.google.com"
- "clients2.google.com"
- "contentautofill.googleapis.com"
- "safebrowsingohttpgateway.googleapis.com"
- "www.google.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 6 domainsThe following domains were blocked by the firewall during workflow execution:
network:
allowed:
- defaults
- "accounts.google.com"
- "android.clients.google.com"
- "clients2.google.com"
- "contentautofill.googleapis.com"
- "safebrowsingohttpgateway.googleapis.com"
- "www.google.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 6 domainsThe following domains were blocked by the firewall during workflow execution:
network:
allowed:
- defaults
- "accounts.google.com"
- "android.clients.google.com"
- "clients2.google.com"
- "contentautofill.googleapis.com"
- "safebrowsingohttpgateway.googleapis.com"
- "www.google.com"See Network Configuration for more information.
|
There was a problem hiding this comment.
💥 Automated smoke test review - all systems nominal!
Warning
Firewall blocked 6 domains
The following domains were blocked by the firewall during workflow execution:
accounts.google.comandroid.clients.google.comclients2.google.comcontentautofill.googleapis.comsafebrowsingohttpgateway.googleapis.comwww.google.com
To allow these domains, add them to the
network.allowedlist in your workflow frontmatter:
network:
allowed:
- defaults
- "accounts.google.com"
- "android.clients.google.com"
- "clients2.google.com"
- "contentautofill.googleapis.com"
- "safebrowsingohttpgateway.googleapis.com"
- "www.google.com"See Network Configuration for more information.
💥 [THE END] — Illustrated by Smoke Claude · 80.7 AIC · ⌖ 14.5 AIC · ⊞ 8.4K
Comment /smoke-claude to run again
| @@ -76,6 +76,7 @@ const PROMPT_FILE_INLINE_THRESHOLD_LABEL = "100KB"; | |||
| const MAX_ENV_VAR_PREVIEW_LENGTH = 120; | |||
| const OUTPUT_TAIL_MAX_CHARS = 600; | |||
| const OUTPUT_TAIL_MAX_LINES = 12; | |||
There was a problem hiding this comment.
Nice — introducing a named constant for the output tail improves readability. Consider documenting the unit (chars).
| * env?: NodeJS.ProcessEnv | ||
| * env?: NodeJS.ProcessEnv, | ||
| * postResultWatchdog?: { | ||
| * shouldArm: () => boolean, |
There was a problem hiding this comment.
The expanded JSDoc for runner options is helpful. Ensure the terminate-on-idle behavior is covered by a unit test asserting the timeout path.
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 6 domainsThe following domains were blocked by the firewall during workflow execution:
network:
allowed:
- defaults
- "accounts.google.com"
- "android.clients.google.com"
- "clients2.google.com"
- "contentautofill.googleapis.com"
- "safebrowsingohttpgateway.googleapis.com"
- "www.google.com"See Network Configuration for more information.
|
|
🎉 This pull request is included in a new release. Release: |
Issue Triage runs were producing valid terminal safe output (
noop) but the Copilot CLI process could remain alive until the step hard-timeout, flipping successful no-op runs to failure. This change makes terminal-output completion an explicit process-lifecycle boundary.Terminal-output aware shutdown path
copilot_harness.cjs(hasTerminalSafeOutput).noopand non-diagnostic task outputs as terminal; excludes diagnostic-only entries (missing_tool,report_incomplete).Post-result inactivity watchdog in process runner
runProcess(...)with optionalpostResultWatchdog.SIGTERM; escalates toSIGKILLafter grace period.Copilot harness integration
✨ PR Review Safe Output Test - Run 28993848517
Warning
Firewall blocked 6 domains
The following domains were blocked by the firewall during workflow execution:
accounts.google.comandroid.clients.google.comclients2.google.comcontentautofill.googleapis.comsafebrowsingohttpgateway.googleapis.comwww.google.comSee Network Configuration for more information.