Uh oh!
There was an error while loading. Please reload this page.
Fix premature copilot-sdk readiness timeout and unhelpful engine failure context - #53299
Conversation
…failure context Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
PR TriageCategory: bug | Risk: medium | Score: 55/100
Recommended action: fast_track
|
There was a problem hiding this comment.
Pull request overview
Fixes Copilot SDK startup timing and improves engine failure diagnostics.
Changes:
- Extends sidecar startup timeout to 60 seconds.
- Extracts harness errors and filters wrapped infrastructure noise.
- Adds regression tests for both behaviors.
Show a summary per file
| File | Description |
|---|---|
actions/setup/js/copilot_sdk_sidecar.cjs | Extends and exports the startup timeout. |
actions/setup/js/copilot_harness.test.cjs | Verifies the startup budget. |
actions/setup/js/handle_agent_failure.cjs | Improves failure-context extraction and filtering. |
actions/setup/js/handle_agent_failure.test.cjs | Adds failure-reporting regression tests. |
Review details
💡 Add a code-review agent skill for context-aware, tailored reviews. Learn more in the docs.
- Files reviewed: 4/4 changed files
- Comments generated: 1
- Review effort level: Balanced
| * These lines carry the actionable root cause even though the harness prefix marks them as | ||
| * infrastructure output, so they are extracted as engine error details. | ||
| */ | ||
| const HARNESS_UNEXPECTED_ERROR_RE = /^\[(?:copilot|claude|codex)-harness\]\s*unexpected error:\s*(.+)$/; |
✅ Design Decision Gate 🏗️ completed the design decision gate check. No ADR enforcement needed: PR #53299 does not have the 'implementation' label and has 0 new lines of code in business logic directories (4 files changed, none in src/, lib/, pkg/, internal/, app/, core/, domain/, services/, or api/).
|
✅ Ponytail Reviewer completed successfully! Lean already. Ship.
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅
|
✅ PR Code Quality Reviewer completed the code quality review.
|
✅ Test Quality Sentinel completed test quality analysis. Test Quality Sentinel skipped because pre-fetch PR data was unavailable: unable to fetch test file diff
|
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.
|
There was a problem hiding this comment.
Request changes
The failure-context parser still misses terminal harness errors from the timestamped Claude/Codex wrappers, so this is not yet a general fix for the bug class the PR is addressing.
Blocking theme
HARNESS_UNEXPECTED_ERROR_REonly matches[<engine>-harness] unexpected error: ...with no timestamp between the prefix and message.claude_harness.cjsandcodex_harness.cjslog as[claude-harness] <ISO timestamp> unexpected error: ..., so their terminal failures still bypassError detailsextraction.- That leaves the reporter falling back to tail parsing and potentially surfacing infrastructure noise again for those engines.
🔎 Code quality review by PR Code Quality Reviewer · gpt54 · 15.6 AIC · ⌖ 5.53 AIC · ⊞ 6.9K
Comment /review to run again
| * These lines carry the actionable root cause even though the harness prefix marks them as | ||
| * infrastructure output, so they are extracted as engine error details. | ||
| */ | ||
| const HARNESS_UNEXPECTED_ERROR_RE = /^\[(?:copilot|claude|codex)-harness\]\s*unexpected error:\s*(.+)$/; |
There was a problem hiding this comment.
This regex is too narrow for the actual harness output: claude_harness.cjs and codex_harness.cjs prepend an ISO timestamp after the prefix ([claude-harness] 2026-... unexpected error: ...), so those failures still will not be extracted into Error details and the reporter will fall back to tail noise again.
💡 Fix and rationale
Allow an optional timestamp/token section between the harness prefix and unexpected error: instead of requiring the message to start immediately after ], for example:
constHARNESS_UNEXPECTED_ERROR_RE=/^\[(?:copilot|claude|codex)-harness\](?:\s+\S+)?\s+unexpectederror:\s*(.+)$/;or, more robustly, capture everything after the prefix until unexpected error:.
Without that, this patch only fixes Copilot while Claude/Codex still misclassify the same terminal harness failures.
There was a problem hiding this comment.
Review
The PR addresses two real bugs: the 5 s copilot-SDK startup timeout causing false failures during package extraction, and infrastructure "continuation" lines polluting the failure-context tail. Both fixes are correct and well-tested.
Blocking issue: HARNESS_UNEXPECTED_ERROR_RE doesn't match claude/codex harness output format
This was already flagged in an open review comment (line 46, handle_agent_failure.cjs) and is the only blocking issue I found.
copilot_harness.cjslog() writes [copilot-harness] <message> (no timestamp), so the regex works there. But claude_harness.cjs and codex_harness.cjs write [claude-harness] <ISO-timestamp> <message> — the regex anchor unexpected error: immediately after \]\s* will never match those lines. The "copilot|claude|codex" alternation in the constant is misleading until the timestamp is accounted for.
Fix: make the timestamp optional in the pattern, e.g.:
constHARNESS_UNEXPECTED_ERROR_RE=/^\[(?:copilot|claude|codex)-harness\](?:\s+[\d:TZ.-]+)?\s*unexpectederror:\s*(.+)$/;And add a test case using a [claude-harness] 2026-...T...Z unexpected error: ... line to confirm the match.
Non-blocking observations
COPILOT_SDK_SERVER_STARTUP_TIMEOUT_MSexport: exporting the constant is a reasonable choice for testability; the test assertion (≥ 30000) correctly guards against future regressions. The 60 s value is generous but justified given real-world ~7 s extraction times and the fast-path rejection on process exit.- Infra continuation heuristic: the leading-whitespace heuristic is simple and effective for the observed log format. An empty line resets
previousWasInfra, which is correct (blank lines separate logical blocks). agentLinesfilter chain: the refactor to multi-line.filter(...)improves readability without changing semantics.
🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · sonnet46 · 23.7 AIC · ⌖ 10 AIC · ⊞ 5.6K
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /diagnosing-bugs and /tdd — commenting on one correctness issue (regex mismatch for harness log format).
📋 Key Themes & Highlights
Key Themes
- Regex/format mismatch:
HARNESS_UNEXPECTED_ERROR_REwon't matchclaude-harnessorcodex-harnesslines because theirlog()functions insert an ISO timestamp between the prefix and the message. Onlycopilot-harnessomits the timestamp. The existing review comment (#3795878652) flags this on line 46 — it should be addressed before merging. - Continuation-line filtering: The new
infraContinuationLinesscan is correct but only handles one indentation level; multi-level wraps would survive. Acceptable for the current patterns but worth a comment.
Positive Highlights
- ✅ Root cause properly addressed — timeout increased to 60 s with a clear comment explaining the fast-fail path still applies
- ✅
COPILOT_SDK_SERVER_STARTUP_TIMEOUT_MSexported and asserted in a test — makes the budget observable and regression-proof - ✅ Three well-structured test cases covering harness-error extraction, continuation filtering, and infra-only log; good Arrange/Act/Assert layout
- ✅ Good progressive filtering approach: extract known error patterns first, fall back to tail only when nothing matched
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · sonnet46 · 47.3 AIC · ⌖ 8.88 AIC · ⊞ 7.7K
Comment /matt to run again
🎉 This pull request is included in a new release. Release: |
The Daily GitHub Docs SEO Optimizer run failed with an engine failure whose only reported context was
The Docker agent cgroup cannot be passed through, so pids.max/pids.current are unavailable.— an unrelated AWF warning. Two separate defects: the copilot-sdk sidecar aborted a server that was still starting, and the failure reporter surfaced infrastructure noise instead of the actual error.Root cause
From the run's
agent-stdio.log:The Copilot CLI spent ~6.9s extracting its bundle before binding the port; the readiness probe's 5s budget expired first and killed a healthy server.
The failure issue then reported the wrong line because
[copilot-harness] …matchesAWF_INFRA_LINE_REand was stripped, while the indented continuation of a preceding[WARN]line did not match and survived as the only "agent output".Changes
copilot_sdk_sidecar.cjs—COPILOT_SDK_SERVER_STARTUP_TIMEOUT_MS5s → 60s. The startupPromise.racestill rejects immediately on childerror/exit, so a genuinely broken server continues to fail fast; the larger budget only affects the slow-start path. Constant is now exported so the budget is asserted in tests.handle_agent_failure.cjs[<copilot|claude|codex>-harness] unexpected error: <message>intoerrorMessages, so harness terminal failures are reported as Error details rather than falling through to the log tail.[WARN]/[INFO]messages are no longer mistaken for agent output.handle_agent_failure.test.cjscovering harness-error extraction, continuation filtering, and the infra-only log path; one incopilot_harness.test.cjsguarding the startup budget.Replaying the original log through
buildEngineFailureContext()now produces: