Classify Copilot SDK session.idle timeouts as agentic_engine_timeout#43413
Conversation
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
session.idle timeouts as agentic_engine_timeout
🤖 PR Triage — Run §28715668077
Small targeted JS fix with test. Batch with other similar Go/JS bugfix PRs for review.
|
|
@copilot revert action pins changes |
There was a problem hiding this comment.
Pull request overview
This PR updates the JavaScript-based agent log classifier used by gh-aw’s workflow setup steps so that Copilot SDK session.idle idle-timeout failures are classified as agentic_engine_timeout, improving downstream conclusion/reporting behavior.
Changes:
- Expanded
AGENTIC_ENGINE_TIMEOUT_PATTERNto match both signal-based cancellations andTimeout after <n>ms waiting for session.idle. - Added focused Vitest coverage to ensure the regex and
detectErrors(...)returnagenticEngineTimeout: truefor the new signature. - Re-indented two embedded
action_pins.jsondata files (formatting-only, large diff).
Show a summary per file
| File | Description |
|---|---|
| actions/setup/js/detect_agent_errors.cjs | Extends timeout detection to include Copilot SDK session.idle idle-timeouts. |
| actions/setup/js/detect_agent_errors.test.cjs | Adds regression tests covering the new timeout signature and classification output. |
| pkg/workflow/data/action_pins.json | Large formatting-only reindent of embedded action pin metadata. |
| pkg/actionpins/data/action_pins.json | Large formatting-only reindent of embedded action pin metadata. |
Review details
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 2/4 changed files
- Comments generated: 3
- Review effort level: Low
| // Pattern: Agentic engine timeout. | ||
| // Covers both timeout signatures observed in engine logs: | ||
| // 1) Process killed by signal after step timeout-minutes: | ||
| // [copilot-harness] ... process closed exitCode=1 signal=SIGTERM ... | ||
| // 2) Copilot SDK idle-timeout while waiting for session.idle: | ||
| // [sdk-driver] error: Timeout after 870000ms waiting for session.idle | ||
| // The second form can occur even when the driver collected output, and should | ||
| // still be classified as a timeout for conclusion/reporting purposes. | ||
| const AGENTIC_ENGINE_TIMEOUT_PATTERN = /(?:signal=SIG(?:TERM|KILL|INT)|Timeout after \d+ms waiting for session\.idle)/; |
| { | ||
| "entries": { | ||
| "actions-ecosystem/action-add-labels@v1.1.3": { | ||
| "repo": "actions-ecosystem/action-add-labels", | ||
| "version": "v1.1.3", | ||
| "sha": "c96b68fec76a0987cd93957189e9abd0b9a72ff1", | ||
| "inputs": { | ||
| "github_token": { | ||
| "description": "A GitHub token.", | ||
| "default": "${{ github.token }}" | ||
| }, | ||
| "labels": { | ||
| "description": "The labels' name to be added. Must be separated with line breaks if there're multiple labels.", | ||
| "required": true | ||
| }, | ||
| "number": { | ||
| "description": "The number of the issue or pull request." | ||
| }, | ||
| "repo": { | ||
| "description": "The owner and repository name. e.g.) Codertocat/Hello-World", | ||
| "default": "${{ github.repository }}" | ||
| "entries": { | ||
| "actions-ecosystem/action-add-labels@v1.1.3": { | ||
| "repo": "actions-ecosystem/action-add-labels", | ||
| "version": "v1.1.3", |
| { | ||
| "entries": { | ||
| "actions-ecosystem/action-add-labels@v1.1.3": { | ||
| "repo": "actions-ecosystem/action-add-labels", | ||
| "version": "v1.1.3", | ||
| "sha": "c96b68fec76a0987cd93957189e9abd0b9a72ff1", | ||
| "inputs": { | ||
| "github_token": { | ||
| "description": "A GitHub token.", | ||
| "default": "${{ github.token }}" | ||
| }, | ||
| "labels": { | ||
| "description": "The labels' name to be added. Must be separated with line breaks if there're multiple labels.", | ||
| "required": true | ||
| }, | ||
| "number": { | ||
| "description": "The number of the issue or pull request." | ||
| }, | ||
| "repo": { | ||
| "description": "The owner and repository name. e.g.) Codertocat/Hello-World", | ||
| "default": "${{ github.repository }}" | ||
| "entries": { | ||
| "actions-ecosystem/action-add-labels@v1.1.3": { | ||
| "repo": "actions-ecosystem/action-add-labels", | ||
| "version": "v1.1.3", |
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅ |
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. |
|
✅ Test Quality Sentinel completed test quality analysis. |
|
✅ PR Code Quality Reviewer completed the code quality review. |
🧪 Test Quality Sentinel Report✅ Test Quality Score: 100/100 — Excellent
📊 Metrics (2 tests)
Verdict
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.
|
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /diagnosing-bugs and /tdd — the bug fix is correct and well-motivated, with a small number of follow-up improvements to address.
📋 Key Themes & Highlights
Issues Found
- Stale diagnostic message (line 152,
detect_agent_errors.cjs): thestderrlog still says "killed by signal" for what may be an SDK idle-timeout — misleading for operators - Stale JSDoc (line 14,
detect_agent_errors.cjs):agentic_engine_timeoutdescription only mentions signal-based kills, not the new SDK case - Missing negative test: no test guards that the regex does not match similar-looking strings like
Timeout after Nms waiting for session.connected - Missing compound-detection test: the new
detectErrorstest only asserts the timeout in isolation; the existing signal timeout has a combined test (withinferenceAccessError) that the SDK variant should mirror
Positive Highlights
- ✅ Regex is correct and properly anchored (
session\.idlewith escaped dot) - ✅ Non-capturing group
(?:...)keeps the pattern clean - ✅ Two regression tests added — pattern-level and function-level — matching the existing test structure
- ✅ Bug root cause is addressed at the right layer (centralised classifier), not patched at call sites
- ✅ Clear, thorough comment block explaining both timeout signatures and why the second form must still be classified as a timeout
@copilot please address the review comments above.
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · 60.8 AIC · ⌖ 8.39 AIC · ⊞ 6.6K
Comment /matt to run again
Comments that could not be inline-anchored
actions/setup/js/detect_agent_errors.cjs:152
[/diagnosing-bugs] The diagnostic stderr message is now stale: it says "engine process was killed by signal" which will mislead operators when an SDK idle-timeout occurs — the new case has nothing to do with signals.
<details>
<summary>💡 Suggested update</summary>
// Before:
process.stderr.write("[detect-agent-errors] Detected timeout: engine process was killed by signal (step timeout-minutes likely exceeded)\n");
// After:
process.stderr.write("[detect-agent-errors] Detected time…
</details>
<details><summary>actions/setup/js/detect_agent_errors.cjs:14</summary>
**[/diagnosing-bugs]** The JSDoc description for `agentic_engine_timeout` is now outdated — it only mentions signal-based kills, not the new SDK idle-timeout case.
<details>
<summary>💡 Suggested update</summary>
```js
// - agentic_engine_timeout: The agentic engine timed out. Covers two
// signatures: (1) engine process killed by SIGTERM/SIGKILL/SIGINT (step
// timeout-minutes reached), or (2) Copilot SDK idle-timeout
// ("Timeout after <n>ms waiting for session.idle").Ke…
actions/setup/js/detect_agent_errors.test.cjs:89
[/tdd] Missing negative test: the regex could potentially match superficially similar log lines (e.g., Timeout after 1000ms waiting for session.connected). A negative test guards the specificity of the session.idle anchor.
<details>
<summary>💡 Suggested negative test</summary>
it("does not match timeouts waiting for states other than session.idle", () => {
expect(AGENTIC_ENGINE_TIMEOUT_PATTERN.test(
"Timeout after 5000ms waiting for session.connected"
)).toBe(false);
…
</details>
<details><summary>actions/setup/js/detect_agent_errors.test.cjs:337</summary>
**[/tdd]** The `detectErrors` test only asserts `agenticEngineTimeout: true` in isolation. Consider also adding a case where the SDK idle-timeout message co-occurs with another error (like `inferenceAccessError`) to verify compound detection still works, mirroring the existing combined test for signal-based timeout.
<details>
<summary>💡 Suggested test</summary>
```js
it("detects SDK session.idle timeout alongside inference access error", () => {
const log = "Access denied by policy setting…
</details>There was a problem hiding this comment.
Review: Classify session.idle timeouts as agentic_engine_timeout
The regex change is correct and the new pattern is well-anchored. Four non-blocking issues need addressing before this is solid.
📋 Findings (4 issues, all medium or low severity)
1 · Stale stderr diagnostic message + module JSDoc (detect_agent_errors.cjs, lines 13 and 152) — when the new SDK idle-timeout arm fires, the operator reads "engine process was killed by signal (step timeout-minutes likely exceeded)" which is factually wrong and will actively mislead incident triage. The module-level JSDoc also still documents the output as signal-only. Both should be updated to mention the SDK idle-timeout case.
2 · No negative boundary tests for the new regex arm (detect_agent_errors.test.cjs) — there are no negative cases for near-miss strings (session.connected, lowercase timeout, underscore separator). A regex as broad as /Timeout after .* waiting/ would pass all current tests.
3 · detectErrors integration test is a duplicate (detect_agent_errors.test.cjs, line ~329) — it uses the same single-line log string as the unit test above it and asserts identical outcomes. It adds zero coverage. Should test multiline/embedded-noise context to justify its existence.
4 · 1512 lines of JSON reformatting bundled unrelated (pkg/actionpins/data/action_pins.json, pkg/workflow/data/action_pins.json) — both files changed 2-space → 4-space indent with no functional change, burying the actual regex change and making independent revert/bisect impossible.
🔎 Code quality review by PR Code Quality Reviewer · 82.5 AIC · ⌖ 6.34 AIC · ⊞ 5.4K
Comment /review to run again
Comments that could not be inline-anchored
actions/setup/js/detect_agent_errors.cjs:54
Stale stderr diagnostic + module JSDoc after pattern expansion: the stderr message on line 152 and the module JSDoc on line 13 still describe agentic_engine_timeout as a signal-kill only, misleading incident triage for the new SDK idle-timeout case.
<details>
<summary>💡 What needs updating</summary>
Line 152 stderr message (unchanged, but now wrong for half the cases):
// Current (wrong for SDK idle-timeout case):
process.stderr.write("[detect-agent-errors] Detected timeout: …
</details>
<details><summary>actions/setup/js/detect_agent_errors.test.cjs:96</summary>
**Missing negative boundary tests for the new regex arm**: the test suite has no negative cases for strings that almost-but-don't-quite match the new `session.idle` arm, leaving the regex boundary completely untested.
<details>
<summary>💡 Suggested additions</summary>
```js
it("does not match near-miss SDK timeout strings", () => {
// lowercase t — case-sensitive by design
expect(AGENTIC_ENGINE_TIMEOUT_PATTERN.test(
"timeout after 870000ms waiting for session.idle"
)).toBe(false);
…
</details>
<details><summary>actions/setup/js/detect_agent_errors.test.cjs:340</summary>
**`detectErrors` integration test for SDK idle-timeout is redundant with the unit test above**: both tests use the identical log string and assert identical outcomes — the integration test adds zero incremental coverage.
<details>
<summary>💡 What to fix</summary>
A `detectErrors` integration test earns its existence by testing *context* the unit regex test cannot — e.g. multiline logs, embedded noise before/after the timeout line, or verifying that no other flag is spuriously set. The curren…
</details>
<details><summary>pkg/actionpins/data/action_pins.json:1</summary>
**1512 lines of unrelated JSON reformatting bundled into a one-line regex fix**: both `action_pins.json` files changed from 2-space to 4-space indentation (756 additions + 756 deletions each) with no functional content change.
<details>
<summary>💡 Why this matters</summary>
- Inflates the diff by ~100×, burying the actual regex/test change.
- Makes `git bisect` and `git blame` unreliable for both the logic change and any future pin updates — you cannot independently revert them.
- Any confli…
</details>
Done in 51a1f63 — reverted the action pins formatting-only changes. |
|
@copilot please run the
|
Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
Addressed in bf39487. I ran the pr-finisher flow, fixed the outstanding code-review blocker by making the timeout diagnostic generic for all timeout signatures, reverted the action-pins formatting churn, and finalized the ADR follow-up content in the branch. |
|
|
|
@copilot please run the
|
Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
Addressed in eb195db. I reran the pr-finisher pass, added the remaining timeout-classifier test coverage requested by github-actions review feedback (negative boundary cases and mixed-signal detection), and revalidated locally ( |
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>
Ran the pr-finisher pass on PR #43413.
|
Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
|
🎉 This pull request is included in a new release. Release: |
PR Code Quality Reviewerintermittently hard-reds on long Copilot SDK runs withTimeout after <n>ms waiting for session.idle, but those failures were not mapped to the timeout class inconclusionoutputs. This left a real timeout signature unclassified and skewed failure accounting/handling.Timeout signature coverage
AGENTIC_ENGINE_TIMEOUT_PATTERNinactions/setup/js/detect_agent_errors.cjsto match both existing signal-based timeouts and SDK idle-timeout messages:signal=SIGTERM|SIGKILL|SIGINTTimeout after <n>ms waiting for session.idleClassifier behavior alignment
detect_agent_errorsso workflows consumingsteps.detect-agent-errors.outputs.agentic_engine_timeoutreceive the correct flag for SDK idle-timeout runs.Regression tests
actions/setup/js/detect_agent_errors.test.cjsfor:session.idletimeout log linesdetectErrors(...)returningagenticEngineTimeout: truefor that signature