Add Windows ConPTY startup coverage to CLI integration workflow

[Copilot]

gh aw can hang at process startup under Windows ConPTY even for non-interactive commands like version and --help. The existing Windows integration workflow covered redirected stdio and stdin hang cases, but not a live pseudoconsole path.

• What changed

  • Added a dedicated ConPTY smoke step to /home/runner/work/gh-aw/gh-aw/.github/workflows/windows-cli-integration.yml.
  • The step creates a real Windows pseudoconsole, launches gh-aw.exe inside it, and asserts that startup-only commands complete and produce expected output.

• Coverage added

  • gh-aw.exe --help
  • gh-aw.exe version

• Why this matters

  • This exercises the exact terminal environment implicated by the report: a live ConPTY session where startup terminal probing can block before any output is emitted.
  • It complements the existing redirected-stdio and closed-stdin checks rather than overlapping them.

• Implementation notes

  • Uses inline C# from PowerShell to create the pseudoconsole via CreatePseudoConsole and STARTUPINFOEX.
  • Applies an explicit timeout so the workflow fails fast if startup blocks.

foreach ($command in @(
  @{ name = "--help"; args = "--help"; expected = "GitHub Agentic Workflows" },
  @{ name = "version"; args = "version"; expected = "gh aw version " }
)) {
  $output = [ConPtySmoke]::Run($env:BINARY, $command.args, $conPtyTimeoutMs)
  if ($output -notmatch [regex]::Escape($command.expected)) {
    throw "Expected ConPTY output for '$($command.name)' to contain '$($command.expected)'"
  }
}

[Copilot]

Pull request overview

Adds a ConPTY-based smoke test to the Windows CLI integration workflow to detect gh-aw.exe startup hangs when launched inside a real Windows pseudoconsole (the environment implicated by reported hangs), complementing existing redirected-stdio and stdin-hang checks.

Changes:

• Adds a new “[ConPTY] startup probe hang detection” workflow step.
• Implements an inline C# ConPTY host that launches gh-aw.exe and enforces a hard timeout.
• Verifies expected output for --help and version inside the ConPTY session.

Show a summary per file

File	Description
.github/workflows/windows-cli-integration.yml	Adds a ConPTY pseudoconsole startup smoke test to catch Windows pseudoconsole-specific startup hangs.

Copilot's findings

Tip

Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

• Files reviewed: 1/1 changed files
• Comments generated: 1

[github-actions]

🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅

[github-actions]

✅ Design Decision Gate 🏗️ completed the design decision gate check.

No ADR enforcement needed: PR #41035 does not have the 'implementation' label and has 0 new lines of code in business logic directories (1 file changed, ≤100 additions). Neither enforcement condition is met.

[github-actions]

✅ PR Code Quality Reviewer completed the code quality review.

[github-actions]

✅ Test Quality Sentinel completed test quality analysis.

No test files were added or modified in this PR. The only changed file is .github/workflows/windows-cli-integration.yml (a CI workflow YAML file). Test Quality Sentinel skipped.

[github-actions]

@copilot review all comments and address unresolved review feedback.

Generated by 👨‍🍳 PR Sous Chef · 45.4 AIC · ⌖ 0.975 AIC · ⊞ 17.3K · ◷

[pelikhan]

@copilot Fox support for countty pseudo consoles

[github-actions]

REQUEST_CHANGES — one high-severity correctness issue and two medium diagnostic/reliability issues.

Blocking issues summary

🔴 High — ANSI escape sequences can produce false test failures (line 547)

The ConPTY is a real terminal; gh-aw.exe will see isatty() == true and may emit ANSI colour codes. These bytes pass straight through the pipe to $output. A literal [regex]::Escape match against "GitHub Agentic Workflows" will fail if colour codes appear within that phrase — the test reports a startup hang when the binary is healthy. Fix: strip VT sequences before matching.

🟡 Medium — ThrowLastWin32 discards the Windows system error message (line 400)

new Win32Exception(errorCode, operationName) sets the exception Message to operationName only; the Win32 system description is lost. CI failures caused by infrastructure issues (low handles, access denied, etc.) will be much harder to diagnose.

🟡 Medium — readTask disposed-while-reading on timeout path (line 498)

On timeout, TerminateProcess + throw unwinds the stack and C# using declarations dispose StreamReader/FileStream while the thread-pool read is still in flight, producing an unobserved ObjectDisposedException. Fix: close the ConPTY and drain the task before throwing.

🔎 Code quality review by PR Code Quality Reviewer · 98.5 AIC · ⌖ 8.38 AIC · ⊞ 5.1K

[github-actions]

ANSI/VT escape sequences will break this match when the binary uses color output under a live terminal. The test can report false failures even when the binary starts up correctly.

💡 Why this matters and how to fix it

A ConPTY is a real pseudo-terminal. When gh-aw.exe is launched inside one it will see isatty() as true, which causes most CLI frameworks (Cobra, Lipgloss, BubbleTea) to enable ANSI colour output. The raw VT bytes are passed straight through to the stdoutRead pipe — the ConPTY does not strip them. So $output could look like:

GitHub \x1b[1mAgentic\x1b[0m Workflows

[regex]::Escape("GitHub Agentic Workflows") produces the pattern GitHub\ Agentic\ Workflows, which won't match the interleaved escape bytes. The test throws, reporting a startup hang — the exact condition it was designed to detect — when the binary is actually healthy.

Suggested fix — strip ANSI sequences before matching:

# strip ANSI/VT escape sequences before asserting content
$clean = $output -replace "`e\[[\d;]*[A-Za-z]", ""
if ($clean -notmatch [regex]::Escape($command.expected)) {
  throw "Expected ConPTY output for '$($command.name)' to contain '$($command.expected)', got:`n$output"
}

Or strip in the C# Run() helper before returning (keeps all call sites clean).

[github-actions]

ThrowLastWin32 silently discards the Windows system error description. When any P/Invoke call fails in CI, the diagnostic you see is just the operation name with no indication of what actually went wrong.

💡 Details and fix

new Win32Exception(int nativeErrorCode, string message) uses the second argument as the full Message, overriding the system-derived description. So instead of:

Win32Exception (0x5): CreatePipe(stdin): Access is denied

you get:

Win32Exception (0x5): CreatePipe(stdin)

Fix — preserve the system message alongside the operation name:

private static void ThrowLastWin32(string operation)
{
    int err = Marshal.GetLastWin32Error();
    string sysMsg = new Win32Exception(err).Message;
    throw new Win32Exception(err, $"{operation}: {sysMsg}");
}

[github-actions]

On timeout, readTask is abandoned while StreamReader/FileStream are still in scope and will be disposed as the stack unwinds. Disposing a stream with an in-flight async read causes an unobserved ObjectDisposedException on the thread-pool task.

💡 What happens and a safe fix

When TerminateProcess is called and TimeoutException is thrown:

1. The C# using-declaration scopes for reader and outputStream dispose both objects as the method exits.
2. The readTask thread-pool thread is mid-flight in a blocking Read on the pipe.
3. Disposing outputStream while the read is in flight raises ObjectDisposedException on the background task.
4. In current .NET Core, unobserved task exceptions are swallowed — but this is a fragile assumption.

Safer approach: close the ConPTY before throwing so the pipe gets EOF, then wait for the read to drain cleanly before disposal:

if (waitResult == WAIT_TIMEOUT)
{
    TerminateProcess(processInfo.hProcess, 1);
    // Trigger pipe EOF so readTask completes before the using-declarations dispose the stream.
    ClosePseudoConsole(pseudoConsole);
    pseudoConsole = IntPtr.Zero;
    try { readTask.GetAwaiter().GetResult(); } catch { /* drain; output is irrelevant on timeout */ }
    throw new TimeoutException($"ConPTY child timed out after {timeoutMs}ms for: {commandLine}");
}

[github-actions]

Skills-Based Review 🧠

Applied /tdd — requesting changes on two correctness issues: a Win32 API contract violation in the harness and VT-sequence noise that can produce silent false negatives in the assertions.

📋 Key Themes & Highlights

Issues to address

• Win32 API contract violation (pseudoConsoleValuePtr, line 462): the heap buffer passed to UpdateProcThreadAttribute must remain valid until DeleteProcThreadAttributeList. The current finally frees it before CreateProcessW, violating this contract. Fix: move the pointer into the outer variable block and free it after DeleteProcThreadAttributeList in the outer finally.
• VT-sequence false negatives (line 547): ConPTY output is raw terminal bytes. If the binary emits color codes that split an expected literal, the -match [regex]::Escape(...) assertion silently passes on broken output. Fix: strip VT sequences with -replace '\x1b\[[0-9;]*[A-Za-z]', '' before asserting.

Smaller suggestions (non-blocking)

• Raw VT in error message (line 548): clean the output before interpolating it into the throw message to avoid unreadable CI logs.
• readTask has no timeout (line 508): after ClosePseudoConsole the drain should complete immediately, but a bounded Task.Wait(timeoutMs) removes the theoretical hang path.
• TerminateProcess unchecked (line 497): check the return value to distinguish a race from a real failure.

Positive highlights

• ✅ P/Invoke wiring is correct — PROC_THREAD_ATTRIBUTE_PSEUDOCONSOLE = 0x00020016 matches the SDK constant.
• ✅ The outer finally block covers all Win32 handles comprehensively; no handle leaks in the happy or error paths.
• ✅ WaitForSingleObject + TerminateProcess timeout pattern is exactly right for turning a hang into a fast CI failure.
• ✅ The step neatly complements the existing redirected-stdio and closed-stdin checks without duplicating them.

🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · 104 AIC · ⌖ 10.9 AIC · ⊞ 6.4K

[github-actions]

[/tdd] pseudoConsoleValuePtr is freed here before CreateProcessW (line 471) and DeleteProcThreadAttributeList (line 527), but MSDN requires the buffer passed to UpdateProcThreadAttribute to remain valid for the lifetime of the attribute list.

💡 Fix: move into outer scope + outer finally

Declare pseudoConsoleValuePtr in the outer variable block alongside attributeList, remove this inner try/finally, and free it in the outer finally after DeleteProcThreadAttributeList:

// outer locals
IntPtr attributeList = IntPtr.Zero;
IntPtr pseudoConsoleValuePtr = IntPtr.Zero;   // ← add here
...
// inline — no inner try/finally needed
pseudoConsoleValuePtr = Marshal.AllocHGlobal(IntPtr.Size);
Marshal.WriteIntPtr(pseudoConsoleValuePtr, pseudoConsole);
if (!UpdateProcThreadAttribute(attributeList, 0,
    (IntPtr)PROC_THREAD_ATTRIBUTE_PSEUDOCONSOLE,
    pseudoConsoleValuePtr, (IntPtr)IntPtr.Size,
    IntPtr.Zero, IntPtr.Zero))
    ThrowLastWin32("UpdateProcThreadAttribute");

// outer finally — free AFTER DeleteProcThreadAttributeList
if (attributeList != IntPtr.Zero)
{
    DeleteProcThreadAttributeList(attributeList);
    Marshal.FreeHGlobal(attributeList);
}
if (pseudoConsoleValuePtr != IntPtr.Zero)
    Marshal.FreeHGlobal(pseudoConsoleValuePtr);

The current code works in practice because the heap rarely reuses the 8-byte block before CreateProcessW finishes, but it violates the API contract and could fail under heap pressure or future CLR changes.

[github-actions]

[/tdd] ConPTY output contains raw VT/ANSI escape sequences. If the binary injects color codes within the expected literal (e.g. GitHub\x1b[1m Agentic\x1b[0m Workflows), [regex]::Escape on the plain string will never match — the test silently becomes a no-op, defeating its regression-guard purpose.

💡 Strip VT sequences before asserting

The existing Invoke-CliProcess helper already watches for unexpected ANSI (line 139). Apply the same strip-then-assert pattern here:

$cleanOutput = $output -replace "\x1b\[[0-9;]*[A-Za-z]", ""
if ($cleanOutput -notmatch [regex]::Escape($command.expected)) {
  throw "Expected ConPTY output for $(($command.name)!) to contain ..."
}

This ensures the assertion tests the textual content rather than the byte stream.

[github-actions]

[/tdd] The $output dumped in the error message contains raw VT escape sequences, making CI logs practically unreadable when this assertion fires.

💡 Log the clean output, not the raw bytes

$cleanOutput = $output -replace "\x1b\[[0-9;]*[A-Za-z]", ""
if ($cleanOutput -notmatch [regex]::Escape($command.expected)) {
  throw "Expected ConPTY output for $($command.name) to contain $($command.expected), got:`n$cleanOutput"
}

This is a minor point that pays dividends the first time you need to debug a CI failure.

[github-actions]

[/tdd] After ClosePseudoConsole (line 505), ReadToEndAsync should complete because the pipe reaches EOF, but this call has no timeout. If there is a race in ConPTY teardown where EOF is delayed, the step will hang indefinitely — the same problem this PR is designed to detect in the binary.

💡 Add a bounded wait

bool completed = readTask.Wait(timeoutMs);
if (!completed)
    throw new TimeoutException($"ConPTY output drain timed out after {timeoutMs}ms");
string output = readTask.GetAwaiter().GetResult();

Or if you prefer a simple defensive cap, use Task.Wait(TimeSpan.FromSeconds(5)) — the process has already exited so a very short window is sufficient.

[github-actions]

[/tdd] TerminateProcess return value is not checked. If the call fails (process already exited with a different race), the runner may have a leaked background process and the thrown TimeoutException hides whether termination succeeded.

💡 A defensive check

if (!TerminateProcess(processInfo.hProcess, 1))
{
    int err = Marshal.GetLastWin32Error();
    // ERROR_ACCESS_DENIED (5) typically means the process already exited; safe to ignore.
    if (err != 5)
        throw new Win32Exception(err, "TerminateProcess");
}
throw new TimeoutException(...);

Done

[pelikhan]

@copilot run pr-finisher skill

[Copilot]

@copilot Fox support for countty pseudo consoles

Fixed in the latest commit. Added an attributeListInitialized boolean that is set to true only after the second InitializeProcThreadAttributeList call succeeds. The finally block now only calls DeleteProcThreadAttributeList when that flag is true, while FreeHGlobal still runs whenever attributeList != IntPtr.Zero.

[Copilot]

@copilot run pr-finisher skill

All review threads have been addressed in commit e1b86a7. Here's what was fixed:

From the previous session (64c5404):

• attributeListInitialized flag guards DeleteProcThreadAttributeList so it's only called on a successfully initialized buffer

In e1b86a7:

• ThrowLastWin32 now includes the Win32 system message (new Win32Exception(err).Message) so CI failures show the actual error description
• pseudoConsoleValuePtr lifetime — moved to outer scope and freed after DeleteProcThreadAttributeList, satisfying the Win32 API contract that the buffer must remain valid for the attribute list's lifetime
• Timeout path — checks TerminateProcess return value (ignores ERROR_ACCESS_DENIED=5 race), closes the ConPTY to signal EOF, then drains readTask before throwing to avoid ObjectDisposedException on the background thread
• readTask.Wait(timeoutMs) — bounded drain after normal process exit so the step can't hang on a slow ConPTY EOF race
• ANSI/VT stripping — $output -replace "\e[[\d;]*[A-Za-z]", ""applied before-match` and in the error message, preventing false failures when the binary emits colour codes under the live terminal

---

• ✅ Reviews — all 9 unresolved threads addressed in code
• ✅ Checks (local) — make recompile and make agent-report-progress pass clean
• ❓ Checks (CI) — stale after agent push; needs human re-trigger. No prior CI failures unrelated to this PR.
• ✅ Mergeable — PR is open, not draft, no conflicts

Hand-off: CI must be re-triggered by a maintainer (close/reopen PR or push) before merge.