fix(shell): stop PowerShell wrapper leaking a stray exit-code line to stdout#3425
Conversation
… stdout The wrapper ended with `return $exitCode`, which in PowerShell writes the value to the output stream — appending a bare `0` after every command's real output and corrupting captures like `$out = wt list --format json`. Exit-code propagation is already handled by $global:LASTEXITCODE and Write-Error, so the return was redundant. Add a regression test whose mock exits with a real code (the existing test's mock left $LASTEXITCODE unset, hiding the bug).
worktrunk-bot
left a comment
There was a problem hiding this comment.
The core fix is correct — I reproduced it against pwsh 7.6 with the rendered wrapper: with return $exitCode, $out = wt list captures MOCK_OUTPUT_LINE 0 (stray trailing 0); without it, $out is just MOCK_OUTPUT_LINE. $LASTEXITCODE still propagates (3 on a failing mock), and the process exit code is unchanged. The regression test correctly forces $LASTEXITCODE to be set via exit 0 in the mock, which is what the pre-existing test missed. Good catch on why the old test didn't see it.
One accuracy nit on the added comment (not blocking, and not a regression — pre-existing behavior is identical before and after this PR):
Setting
$global:LASTEXITCODEplus the Write-Error below already propagates both$LASTEXITCODEand$?.
The $LASTEXITCODE half holds, but the $? half doesn't. A non-terminating Write-Error … -ErrorAction SilentlyContinue sets $? = $false only within the function's own scope; because the wrapper is (deliberately) a simple function, that failure signal doesn't cross the function boundary, so $? is $true in the caller even when wt failed. This is why the load-bearing justification for dropping return is really just $global:LASTEXITCODE — the Write-Error doesn't actually reach the caller's $?.
Worth softening the comment so a future maintainer doesn't rely on $? reflecting a wt failure. The underlying $?-doesn't-propagate gap predates this PR and is out of scope here, but if it's worth fixing it'd be a separate change (and can't use Write-Error, since the wrapper must stay a simple function).
Verification (pwsh 7.6)
Isolated — Write-Error does set $? in its own scope:
Write-Error 'x' -ErrorAction SilentlyContinue; $? # -> False
But through the wrapper function it doesn't reach the caller:
function foo { Write-Error 'boom' -ErrorAction SilentlyContinue }
foo; $? # -> True
Rendered wrapper against a mock that does exit 3:
$env:WORKTRUNK_BIN='mock-wt-fail.ps1'; . rendered-wrapper.ps1
wt list; $q = $?
# $q -> True (does NOT reflect the failure)
# $LASTEXITCODE -> 3 (correct)
Capture cleanliness (the actual fix), success mock:
old (return $exitCode): $out = wt list -> "MOCK_OUTPUT_LINE 0"
new (no return): $out = wt list -> "MOCK_OUTPUT_LINE"
Write-Error in a simple function does not set the caller's $?; only $global:LASTEXITCODE propagates a wt failure to scripts/CI. Soften the comment (and the regression-test docstring) so a future maintainer does not rely on $? reflecting a failure.
Problem
The PowerShell shell-integration wrapper ends its
wtfunction withreturn $exitCode. In PowerShell,return <value>writes the value to the function's output (success) stream — it does not set the process/$LASTEXITCODE. So everywtinvocation appends a stray exit-code line (0on success) to stdout, right after the real command output:This corrupts any capture of a command's output, e.g.
$out = wt list --format jsonyields JSON with a trailing0, andwt ... | ...pipelines see an extra element.Exit-code propagation is already handled without the
return:$global:LASTEXITCODE = $exitCode(line 77) propagates$LASTEXITCODEWrite-Error … -ErrorAction SilentlyContinuesets$?to$falseon failureSo
return $exitCodewas redundant for propagation and its only observable effect was the stdout pollution.Fix
Remove the trailing
return $exitCodefrom the wrapper (templates/powershell.ps1) and document why it must not be reintroduced.Verification
Reproduced against
pwsh7.6 with the rendered wrapper: with thereturn,wt listemits<output>then a bare0; without it, only<output>.Added a regression test
test_powershell_wrapper_no_stray_exit_code_on_stdout(intests/integration_tests/shell_powershell.rs,shell-integration-testsfeature). It uses a.ps1mock that emits one line andexit 0so$LASTEXITCODEis actually set — the pre-existing wrapper test's mock only callsWrite-Output, leaving$LASTEXITCODEunset, soreturn $nullemitted nothing and the bug stayed hidden. The new test asserts stdout is exactly the mock's line, which fails before this change (a stray0follows) and passes after.The
src/shell/…init_powershell.snapsnapshot is regenerated to drop thereturnline.Why the existing test didn't catch it
test_powershell_wrapper_passes_short_flags_throughwritesforeach ($a in $args) { Write-Output $a }as its mock. A PowerShell.ps1that never runs a native command orexitleaves$LASTEXITCODEat its prior value ($nullin a fresh session), so the wrapper'sreturn $exitCodebecamereturn $null, which emits nothing to the output stream. A real nativewt.exealways sets$LASTEXITCODE, so the pollution only manifests in real use.