fix: add post-result watchdog to codex_harness to prevent CLI hang-on-exit#48499
Conversation
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
…-exit Mirrors the mechanism already present in copilot_harness.cjs: - After Codex writes a terminal safe-output (add-labels, hide-comment, noop, etc.), arm an inactivity watchdog. - If Codex is still running after POST_RESULT_WATCHDOG_IDLE_TIMEOUT_MS (20s) of silence, send SIGTERM then SIGKILL. - When the watchdog fires and a terminal safe-output already exists, treat the run as exit 0 — the agent completed its work; the hang is cosmetic. Without this, a Codex CLI that hangs after its final turn causes workflows like AI Moderator (Content Moderation) to hit their step timeout and receive an action_required conclusion. Fixes #48192 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 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
Adds a post-result watchdog to prevent Codex CLI exit hangs from causing false workflow failures.
Changes:
- Adds configurable watchdog arming and successful-exit suppression.
- Adds nine watchdog and timeout-resolution tests.
Show a summary per file
| File | Description |
|---|---|
actions/setup/js/codex_harness.cjs |
Implements the Codex post-result watchdog. |
actions/setup/js/codex_harness.test.cjs |
Tests watchdog behavior and timeout configuration. |
Review details
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comments suppressed due to low confidence (1)
actions/setup/js/codex_harness.cjs:542
- This predicate is broader than the Copilot watchdog it is meant to mirror:
hasExpectedSafeOutputstreatsmissing_dataas task output, whilecopilot_harness.cjs:180and its test at lines 272–277 explicitly classifymissing_dataas non-terminal. A Codex agent that reports missing cache/input data and then remains quiet while continuing other work will therefore be terminated after 20 seconds. Use a shared terminal-output predicate (or otherwise exclude every non-terminal diagnostic type) and add amissing_dataregression test.
shouldArm: () => hasExpectedSafeOutputs(safeOutputsPath, { logger: log }) || hasNoopInSafeOutputs(safeOutputsPath, { logger: log }),
- Files reviewed: 2/2 changed files
- Comments generated: 1
- Review effort level: Medium
| postResultWatchdog: safeOutputsPath | ||
| ? { | ||
| shouldArm: () => hasExpectedSafeOutputs(safeOutputsPath, { logger: log }) || hasNoopInSafeOutputs(safeOutputsPath, { logger: log }), |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review: post-result watchdog for codex_harness.cjs
The implementation correctly mirrors the existing watchdog in copilot_harness.cjs. Logic, tests, and exports are all well-structured.
One non-blocking finding: see inline comment — the hasNoopInSafeOutputs branch inside the watchdog check (line 594) is unreachable dead code because the noop guard at line 583 always runs first. Simplifying that condition (or extracting a shared helper matching copilot_harness.cjs's hasTerminalSafeOutput) would improve clarity and parity.
🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · sonnet46 · 27.7 AIC · ⌖ 5.29 AIC · ⊞ 5K
| // The agent completed its work and wrote its output — the hang on exit is a cosmetic | ||
| // failure, not a task failure. Retrying would reproduce the same hang pattern without | ||
| // producing a different output. | ||
| if (result.watchdogFired && safeOutputsPath && (hasExpectedSafeOutputs(safeOutputsPath, { logger: log }) || hasNoopInSafeOutputs(safeOutputsPath, { logger: log }))) { |
There was a problem hiding this comment.
The hasNoopInSafeOutputs(...) call here is unreachable dead code. The noop guard at line 583 always executes first and breaks the loop when a noop is present, so the noop branch of this condition can never be reached.
For consistency with copilot_harness.cjs (which uses a unified hasTerminalSafeOutput()), consider simplifying to:
if (result.watchdogFired && safeOutputsPath && hasExpectedSafeOutputs(safeOutputsPath, { logger: log })) {or extracting a shared hasTerminalSafeOutput helper in safeoutputs_cli.cjs that covers both cases — matching how copilot_harness.cjs handles this.
This is non-blocking — runtime behavior is correct — but the dead code makes intent harder to follow. @copilot please address this.
🧪 Test Quality Sentinel Report
📊 Metrics (9 tests)
✅ Test Coverage SummaryIntegration Tests (3) — Core watchdog suppression feature:
Unit Tests (6) — Config parsing for
Verdict
Rationale: The 3 integration tests (33%) provide strong behavioral coverage of the new watchdog feature and include critical negative testing. The 6 unit tests comprehensively cover config edge cases and input validation. No violations detected. Test inflation (~3:1) is acceptable for integration tests with process spawning and temporary file setup/teardown. Feature correctly implements post-result watchdog suppression to prevent CLI hangs after terminal safe-outputs.
|
There was a problem hiding this comment.
✅ Test Quality Sentinel: 63/100. 33% design tests (integration watchdog tests), 67% implementation tests (config parsing). All thresholds passed. No violations. The 3 integration tests provide strong behavioral coverage with critical negative test protecting against false suppression. Config tests comprehensively cover edge cases.
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /diagnosing-bugs and /tdd — this is a clean, well-tested port of a proven pattern. Two minor issues flagged as inline comments.
📋 Key Themes & Highlights
Issues Found
- Duplicated suppression predicate (
codex_harness.cjsline 590) — theshouldArmcondition and the post-watchdog suppression check express the same logic independently; if one changes the other silently diverges. - Missing stderr assertions in noop integration test — the noop watchdog test only checks
status === 0andcallCount === 1, but not the diagnostic log lines that confirm the watchdog path was actually taken.
Positive Highlights
- ✅ Exact mirror of the battle-tested
copilot_harnesspattern — low risk - ✅ Watchdog only arms after a terminal safe-output is written — no false positives
- ✅
resolvePostResultWatchdogIdleTimeoutMsis well-tested with all edge cases (zero, negative, non-numeric, clamp) - ✅ Integration tests use a real stubbed process with
setIntervalhang — exercises the full OS-level SIGTERM flow
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · sonnet46 · 39.5 AIC · ⌖ 4.87 AIC · ⊞ 6.7K
Comment /matt to run again
| } | ||
|
|
||
| // When the post-result watchdog fired (SIGTERM sent to a hanging Codex process) and the | ||
| // safe-outputs file already contains a terminal result, treat the run as a success. |
There was a problem hiding this comment.
[/diagnosing-bugs] The suppression check at line 590 duplicates the shouldArm predicate defined at lines 536–537 (hasExpectedSafeOutputs || hasNoopInSafeOutputs). If the arming condition ever changes, the two copies can silently drift out of sync, causing the watchdog to arm on one condition but suppress on a different one.
💡 Suggested fix
Extract the condition into a shared variable:
const hasTerminalSafeOutput = () =>
hasExpectedSafeOutputs(safeOutputsPath, { logger: log }) ||
hasNoopInSafeOutputs(safeOutputsPath, { logger: log });
postResultWatchdog: safeOutputsPath
? { shouldArm: hasTerminalSafeOutput, inactivityTimeoutMs: POST_RESULT_WATCHDOG_IDLE_TIMEOUT_MS }
: undefined,
// later...
if (result.watchdogFired && safeOutputsPath && hasTerminalSafeOutput()) {This is one source of truth and mirrors how copilot_harness.cjs structures the same check.
@copilot please address this.
| }, | ||
| encoding: "utf8", | ||
| timeout: 15000, | ||
| }); |
There was a problem hiding this comment.
[/tdd] The noop integration test (lines 730–755) doesn't assert the log message post-result watchdog fired after terminal safe-output was emitted or the late-activity exit suppressed diagnostic, unlike the terminal-output test above it. This means a regression that silently swallows the watchdog path (e.g., exits 0 for the wrong reason) wouldn't be caught.
💡 Suggested addition
Add the same stderr assertions as the first watchdog test:
expect(result.stderr).toContain('post-result watchdog fired after terminal safe-output was emitted');
expect(result.stderr).toContain('late-activity exit suppressed');@copilot please address this.
There was a problem hiding this comment.
REQUEST_CHANGES: watchdog logic is sound but has real duplication and correctness debt to fix before merge.
💡 Themes
- Cross-file duplication: the timeout-resolution constants/function are copy-pasted verbatim between
codex_harness.cjsandcopilot_harness.cjsrather than shared fromprocess_runner.cjs, which will drift over time. - Redundant logic: the new watchdog-fired success branch duplicates a noop check that's already unconditionally handled a few lines above, doing an unnecessary extra file read and confusing which branch owns which case.
- Unbounded timeout override:
resolvePostResultWatchdogIdleTimeoutMsclamps only the minimum, so a misconfigured env var can silently disable the watchdog and reintroduce the exact hang-until-hard-timeout bug this PR fixes. - Misleading logs: the generic "attempt failed" log line prints
watchdogFired=truebefore the run is reclassified as a success a few lines later, making log-based debugging/alerting noisy. - Test coverage for the new suppression paths (noop, terminal safe-output, no-output fallback) is solid; the SIGKILL escalation path is already covered generically in
process_runner.test.cjsso that's not a gap specific to this PR.
None of these are severe correctness bugs that break the feature outright, but the duplication and unbounded-timeout issues are worth fixing before merge to avoid silent regressions.
🔎 Code quality review by PR Code Quality Reviewer · sonnet50 · 75.2 AIC · ⌖ 4.83 AIC · ⊞ 7.4K
Comment /review to run again
| // arms a watchdog timer and kills the Codex process if it is still running after | ||
| // POST_RESULT_WATCHDOG_IDLE_TIMEOUT_MS of inactivity. This prevents the step from | ||
| // hitting its hard timeout when Codex hangs on exit after completing its work. | ||
| const MIN_POST_RESULT_WATCHDOG_TIMEOUT_MS = 50; |
There was a problem hiding this comment.
Watchdog constants/logic duplicated verbatim from copilot_harness.cjs — drift risk.
💡 Details
MIN_POST_RESULT_WATCHDOG_TIMEOUT_MS, DEFAULT_POST_RESULT_WATCHDOG_IDLE_TIMEOUT_MS, and resolvePostResultWatchdogIdleTimeoutMs (lines 91-106) are byte-for-byte copies of the same block in copilot_harness.cjs (lines 85-107). Two independent copies of the same timeout-resolution logic will silently drift the next time one file is tuned (e.g. changing the default from 20s to 30s) and the other is forgotten — exactly the kind of subtle cross-harness inconsistency that is hard to catch in review.
Fix: extract this into process_runner.cjs (or a small shared module) and import it from both harnesses, e.g.:
// process_runner.cjs
function resolvePostResultWatchdogIdleTimeoutMs(env = process.env) { ... }
module.exports = { ..., resolvePostResultWatchdogIdleTimeoutMs, MIN_POST_RESULT_WATCHDOG_TIMEOUT_MS, DEFAULT_POST_RESULT_WATCHDOG_IDLE_TIMEOUT_MS };| // The agent completed its work and wrote its output — the hang on exit is a cosmetic | ||
| // failure, not a task failure. Retrying would reproduce the same hang pattern without | ||
| // producing a different output. | ||
| if (result.watchdogFired && safeOutputsPath && (hasExpectedSafeOutputs(safeOutputsPath, { logger: log }) || hasNoopInSafeOutputs(safeOutputsPath, { logger: log }))) { |
There was a problem hiding this comment.
Watchdog-fired success branch partially overlaps the noop check just above it, causing a redundant duplicate file read.
💡 Details
Line 583 already breaks out with lastExitCode = 0 whenever hasNoopInSafeOutputs is true, regardless of watchdogFired. So by the time line 594 runs, the noop half of its || condition can never be true — it only ever matters via hasExpectedSafeOutputs. Keeping the dead hasNoopInSafeOutputs re-check here does an unnecessary second synchronous read+parse of safe-outputs.jsonl on every failed attempt and muddies which branch actually owns the noop-suppression semantics.
Fix: drop the redundant call so this branch only checks what it can actually reach:
if (result.watchdogFired && safeOutputsPath && hasExpectedSafeOutputs(safeOutputsPath, { logger: log })) {| function resolvePostResultWatchdogIdleTimeoutMs(env = process.env) { | ||
| const configuredTimeoutMs = Number(env.GH_AW_HARNESS_WATCHDOG_TIMEOUT_MS); | ||
| if (!Number.isFinite(configuredTimeoutMs) || configuredTimeoutMs <= 0) { | ||
| return DEFAULT_POST_RESULT_WATCHDOG_IDLE_TIMEOUT_MS; | ||
| } | ||
| return Math.max(MIN_POST_RESULT_WATCHDOG_TIMEOUT_MS, configuredTimeoutMs); |
There was a problem hiding this comment.
resolvePostResultWatchdogIdleTimeoutMs clamps only the lower bound — an absurdly large override effectively disables the watchdog.
💡 Details
Math.max(MIN_POST_RESULT_WATCHDOG_TIMEOUT_MS, configuredTimeoutMs) only guards against too-small values. If GH_AW_HARNESS_WATCHDOG_TIMEOUT_MS is misconfigured (e.g. a stray extra zero, or Number.MAX_SAFE_INTEGER), the watchdog effectively never fires, silently reintroducing the exact hang-until-hard-timeout failure this PR exists to fix — with no warning logged.
Fix: clamp an upper bound too, e.g.:
const MAX_POST_RESULT_WATCHDOG_TIMEOUT_MS = 10 * 60 * 1000;
return Math.min(MAX_POST_RESULT_WATCHDOG_TIMEOUT_MS, Math.max(MIN_POST_RESULT_WATCHDOG_TIMEOUT_MS, configuredTimeoutMs));| log( | ||
| `attempt ${attempt + 1} failed:` + | ||
| ` exitCode=${result.exitCode}` + | ||
| ` watchdogFired=${result.watchdogFired}` + |
There was a problem hiding this comment.
The "attempt failed" log line fires with watchdogFired=true even on runs later reclassified as success, making logs misleading.
💡 Details
When the watchdog fires and a terminal safe-output exists, the code at line ~594 converts the run into a success and breaks. But the generic failure log block at lines 564-578 already ran and printed attempt N failed: exitCode=... watchdogFired=true ... before that reclassification happens. Anyone grepping logs/alerts for "attempt failed" will see false positives for what are actually successful watchdog-suppressed runs, undermining log-based monitoring/alerting for this exact new code path.
Fix: move the watchdog-success check before the failure-log block (or explicitly log a follow-up line noting the reclassification), so the log stream doesn't contradict itself.
The AI Moderator (Content Moderation) workflow uses
engine: codex. When Codex completes its work and writes a terminal safe-output but then hangs on exit, the step waits idle untiltimeout-minutesis reached — producing a falseaction_requiredconclusion.copilot_harness.cjsalready had a post-result watchdog (added in #44254), butcodex_harness.cjswas missing the equivalent.Changes
actions/setup/js/codex_harness.cjsresolvePostResultWatchdogIdleTimeoutMs()(defaults to 20s, respectsGH_AW_HARNESS_WATCHDOG_TIMEOUT_MSoverride)postResultWatchdogtorunProcess: arms once any terminal safe-output (add-labels,hide-comment,noop, etc.) is written, sends SIGTERM after 20s of inactivity, SIGKILL after 5s graceactions/setup/js/codex_harness.test.cjsresolvePostResultWatchdogIdleTimeoutMsunit tests