Increase post-result watchdog default to 120s and add frontmatter timeout override - #51292
Conversation
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
|
@copilot use seconds. |
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅
|
|
❌ Design Decision Gate 🏗️ failed during design decision gate check.
|
|
✅ PR Code Quality Reviewer completed the code quality review. 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
- "api.individual.githubcopilot.com"See Network Configuration for more information.
|
|
✅ Test Quality Sentinel completed test quality analysis.
|
There was a problem hiding this comment.
Pull request overview
Raises the shared post-result watchdog default and adds workflow-level configuration.
Changes:
- Raises the default timeout from 20 seconds to 120 seconds.
- Adds frontmatter parsing and environment-variable injection.
- Updates schema, tests, documentation, and release notes.
Show a summary per file
| File | Description |
|---|---|
pkg/workflow/engine.go |
Adds watchdog engine configuration. |
pkg/workflow/engine_helpers.go |
Injects the watchdog environment variable. |
pkg/workflow/engine_helpers_test.go |
Tests injection and precedence. |
pkg/workflow/engine_config_test.go |
Tests frontmatter extraction. |
pkg/workflow/engine_config_parser.go |
Converts timeout values to milliseconds. |
pkg/parser/schemas/main_workflow_schema.json |
Adds schema validation. |
pkg/parser/schema_test.go |
Tests schema bounds. |
docs/src/content/docs/reference/frontmatter-full.md |
Documents frontmatter syntax. |
docs/src/content/docs/reference/engines.md |
Documents watchdog behavior. |
actions/setup/js/process_runner.cjs |
Raises the shared default. |
actions/setup/js/codex_harness.test.cjs |
Verifies the new default. |
.changeset/patch-harness-watchdog-timeout-default-and-frontmatter.md |
Adds the release note. |
Review details
Tip
Add a code-review agent skill for context-aware, tailored reviews. Learn more in the docs.
- Files reviewed: 12/12 changed files
- Comments generated: 3
- Review effort level: Balanced
| return "" | ||
| } | ||
| if inner, ok := extractWrappedGitHubExpression(seconds); ok { | ||
| return "${{ (" + inner + ") * 1000 }}" |
| if v, ok := h["watchdog-timeout"]; ok { | ||
| config.HarnessWatchdogTimeoutMs = parseHarnessWatchdogTimeoutValue(v) |
| | `initial-delay-ms` | `5000` | Delay in ms before the first retry | | ||
| | `backoff-multiplier` | `2` | Multiplier applied to the delay after each retry | | ||
| | `max-delay-ms` | `60000` | Maximum delay cap in ms | | ||
| | `watchdog-timeout` | `120` | Post-result idle watchdog timeout in seconds before terminating a quiet process | |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
The implementation is clean and complete across all layers — schema, Go parser, env injection, JS harness constant, docs, and tests. No blocking issues found.
🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · sonnet46 · 17.4 AIC · ⌖ 7.16 AIC · ⊞ 5.5K
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /codebase-design and /tdd — requesting changes on three targeted issues.
📋 Key Themes & Issues
Issues Found
- Unit inconsistency (
engines.mdline 368):watchdog-timeoutuses seconds while all sibling harness fields use milliseconds — a footgun that is easy to misread. - Unreachable overflow guard (
engine_config_parser.goline 83):maxInt64Div1000can never be reached becauseParseIntValuereturnsint. Replace with a meaningful 600 s upper bound matching the JS-sideMAX_POST_RESULT_WATCHDOG_TIMEOUT_MS. - Schema missing maximum (
main_workflow_schema.jsonline 12808): Values > 600 s are silently clamped by the harness. Adding"maximum": 600+ a schema test gives users early feedback. - Weak precedence test (
engine_helpers_test.goline 91): Only checks the final value; an intermediate assert would make the override order verifiable.
Positive Highlights
- ✅ Clean seconds→ms conversion in the parser with expression passthrough
- ✅ Good coverage: default value, integer input, expression input, and env injection all tested
- ✅ Changeset correctly tagged as
patch
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · sonnet46 · 41.5 AIC · ⌖ 7.97 AIC · ⊞ 7.1K
Comment /matt to run again
| engineLog.Printf("Ignoring invalid harness.watchdog-timeout value: %q", seconds) | ||
| return "" | ||
| } | ||
| const maxInt64Div1000 = int64((1<<63)-1) / 1000 |
There was a problem hiding this comment.
[/codebase-design] The maxInt64Div1000 overflow guard is unreachable — ParseIntValue returns int, which overflows long before reaching int64 max ÷ 1000 on any supported platform.
💡 Details
parseIntOrExpressionValue calls typeutil.ParseIntValue which returns (int, bool). On 64-bit targets int tops out at ~9.2 × 1018, so parsedSeconds will never exceed maxInt64Div1000 (~9.2 × 1015). The guard was copied defensively but adds noise without protection.
The real upper bound enforced by the JS runtime is MAX_POST_RESULT_WATCHDOG_TIMEOUT_MS = 10 * 60 * 1000 (600 s). Consider validating against that instead:
const maxWatchdogSeconds = 600
if parsedSeconds > maxWatchdogSeconds {
engineLog.Printf("Ignoring out-of-range harness.watchdog-timeout value (max 600 s): %q", seconds)
return ""
}This gives users a meaningful error when they exceed the JS-side clamp, keeping Go and JS bounds in sync.
@copilot please address this.
| @@ -365,8 +366,9 @@ All four fields accept a literal integer or a GitHub Actions expression (e.g. `$ | |||
| | `initial-delay-ms` | `5000` | Delay in ms before the first retry | | |||
| | `backoff-multiplier` | `2` | Multiplier applied to the delay after each retry | | |||
| | `max-delay-ms` | `60000` | Maximum delay cap in ms | | |||
There was a problem hiding this comment.
[/codebase-design] watchdog-timeout uses seconds while every other engine.harness field uses milliseconds (initial-delay-ms, max-delay-ms, backoff-multiplier). This unit inconsistency is a user-facing footgun.
💡 Discussion
All sibling fields in the same engine.harness block use milliseconds. A user copying the retry-policy pattern might write watchdog-timeout: 120000 expecting 120 s, but would get 33 hours.
Consider:
- Rename to
watchdog-timeout-ms(aligns with the injected env varGH_AW_HARNESS_WATCHDOG_TIMEOUT_MS) — no s→ms conversion needed, no unit mismatch. - Or keep seconds but rename to
watchdog-timeout-secondsorwatchdog-timeout-sto make the unit explicit.
If the seconds choice is intentional (human-friendly), the docs and schema description should explicitly warn about the unit difference from the other fields.
@copilot please address this.
| "minimum": 1 | ||
| }, | ||
| { | ||
| "type": "string" |
There was a problem hiding this comment.
[/tdd] Schema enforces minimum: 1 but has no maximum, so values above the JS-side MAX_POST_RESULT_WATCHDOG_TIMEOUT_MS (600 s / 10 min) are silently clamped by the harness with no feedback to the user.
💡 Suggestion
Add "maximum": 600 to the schema integer branch to match MAX_POST_RESULT_WATCHDOG_TIMEOUT_MS:
"watchdog-timeout": {
"oneOf": [
{
"type": "integer",
"minimum": 1,
"maximum": 600
},
{ "type": "string" }
]
}And add a negative test in schema_test.go for a value of 601 to document and protect this bound.
@copilot please address this.
| }, | ||
| } | ||
|
|
||
| applyEngineHarnessRetryEnv(env, workflowData) |
There was a problem hiding this comment.
[/tdd] The precedence test (engine.env override wins) calls applyEngineHarnessRetryEnv and applyEngineAndAgentEnv sequentially, but does not assert that the harness-injected value "120000" was first written and then overwritten — it only checks the final value. A race or bug that skips the harness injection entirely would still pass.
💡 Suggestion
Capture an intermediate snapshot to make the test a real precedence proof:
applyEngineHarnessRetryEnv(env, workflowData)
assert.Equal(t, "120000", env["GH_AW_HARNESS_WATCHDOG_TIMEOUT_MS"], "harness injection should write 120000 first")
applyEngineAndAgentEnv(env, workflowData, nil)
assert.Equal(t, "90000", env["GH_AW_HARNESS_WATCHDOG_TIMEOUT_MS"], "engine.env override should win")@copilot please address this.
🧪 Test Quality Sentinel ReportScore: 78/100 — SummaryThis PR adds 85 lines of test code across 4 test files to verify the new Key Metrics
Per-Test BreakdownTest Classifications & Analysis
Flagged Test:
Design Invariants Verified ✅
Compliance Check ✅
Verdict✅ PASS — Implementation test ratio is 17% (well below 30% threshold). Strong design coverage with proper edge cases and no violations. Score reflects "Acceptable" quality: solid behavioral contracts with one lightweight constant-check test.
|
…timeout control Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
|
@copilot Quick triage nudge for this PR. Please refresh the branch if GitHub can update it cleanly, address the remaining items below, and run the Open items (newest first):
Branch refresh was requested. Run: https://github.com/github/gh-aw/actions/runs/31239156270
|
Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
Done — |
The post-result harness watchdog default (20s of stdio inactivity after terminal safe output) was too short for legitimate quiet shell phases in large repos, causing premature SIGTERM and incomplete runs. This updates the shared default to 120s and adds a first-class frontmatter control so timeout behavior can be tuned per workflow while preserving env-var overrides.
Shared watchdog default
DEFAULT_POST_RESULT_WATCHDOG_IDLE_TIMEOUT_MSfrom20000to120000in the shared process runner used by Copilot and Codex harnesses.Frontmatter-configurable watchdog timeout
engine.harness.watchdog-timeout-ms.GH_AW_HARNESS_WATCHDOG_TIMEOUT_MSso harness behavior is configurable from workflow frontmatter.engine.env.GH_AW_HARNESS_WATCHDOG_TIMEOUT_MSstill takes precedence.Schema and docs updates
engine.harness.watchdog-timeout-ms.Behavioral coverage additions
GH_AW_HARNESS_WATCHDOG_TIMEOUT_MS,Run: https://github.com/github/gh-aw/actions/runs/31239156270> Generated by 👨🍳 PR Sous Chef · gpt54 · 13.2 AIC · ⌖ 5.14 AIC · ⊞ 8.5K · ◷