Repository navigation
Python: Report repeated native exit codes in persistent PowerShell sessions - #9658
Aditya Kumar (Aditya-XR) wants to merge 2 commits into
Conversation
…ssions A persistent PowerShell session reported exit code 0 when a native command failed with the same code as the previous one. The exit-code check compared $LASTEXITCODE by value, which does not change when the code repeats, and $? reflects Invoke-Expression rather than the native command. Compare by reference instead: PowerShell stores a newly boxed int on every native exit, so the variable holds a different object exactly when the command ran a native process. $LASTEXITCODE is still never written.
GitHub Actions
Inactive
GitHub Actions
Inactive
agent-framework-automation
Bot
added
the
python
|
/review |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
MAF Automated Review — Iteration 1
Result: No findings
Scope: full PR (1 commit(s)): 2c2bfbb4c7a6
Model: gpt-5.6-sol
Overview
The reference-identity check addresses repeated equal native exit codes without clearing the persistent session's $LASTEXITCODE. The regression test covers three repeated failures, a following cmdlet-only success, and state readback, while existing guards preserve cmdlet-error handling and serialize session execution. No Critical, High, or Medium defect remains after reconciliation.
Reviewed the supplied pull-request change set across correctness, security/reliability, architecture, and failure behavior.
No publishable findings remained after source verification for this scope.
|
|
||
| readback = await tool.run("Write-Output $LASTEXITCODE") | ||
| assert readback.exit_code == 0 | ||
| assert readback.stdout.strip() == "3" |
There was a problem hiding this comment.
Aditya Kumar (@Aditya-XR), this assertion already fails on ubuntu-latest: PowerShell returns '\x1b[?1l3', and .strip() leaves the terminal-control prefix intact. Both the Python 3.14 unit-test job and aggregate coverage fail here at this head, although all preceding exit-code assertions pass. I’d normalize or suppress those sequences in the test before comparing the readback, while retaining the exact assertion that the preserved value is 3.
There was a problem hiding this comment.
Thanks, you're right. The prefix comes from PowerShell itself, not from the fix: on Linux and macOS, pwsh reading commands from stdin writes ESC[?1h before it reads each line and ESC[?1l before it runs it, unless TERM is dumb (ConsoleHost.cs: before reading, before running). The ESC[?1h arrives after the previous sentinel, but the ESC[?1l ends up at the start of the next command's stdout. The earlier assertions only check exit codes, so the readback was the first exact stdout comparison to run into it.
3e426b0 removes exactly those two sequences before comparing and keeps the == "3" assertion. On Linux with pwsh 7.6.6 the test now passes with TERM unset and with TERM=xterm-256color, and it still fails with attempt 2 reported rc 0 when I revert the _session.py change.
This also means every persistent PowerShell result on Linux and macOS currently starts with ESC[?1l. That's outside this PR, but I can open a separate issue if you'd like the tool to handle it.
On Linux and macOS, pwsh reading commands from stdin writes ESC[?1h before it reads each line and ESC[?1l before it runs it, unless TERM=dumb, so the $LASTEXITCODE readback came back as '\x1b[?1l3' on ubuntu-latest. Drop those two sequences before comparing; the readback must still be exactly 3.
GitHub Actions
Active
Motivation & Context
In a persistent PowerShell session,
LocalShellToolreportsexit_code=0when a native command fails with the same code as the previous native command.ShellSession._build_scriptdecides whether the command ran a native process by checking whether$LASTEXITCODEchanged ($LASTEXITCODE -ne $__af_last). A repeated code doesn't change the value, and the$?fallback can't catch it, because$?reflectsInvoke-Expression, which succeeded. An agent that reruns a failing test command is told that the second run passed. This affects anyone using the tool on Windows, where PowerShell is the default shell, and anyone runningpwshin persistent mode elsewhere.Description & Review Guide
What are the major changes?
One line in the PowerShell branch of
_build_script: the check compares references instead of values.PowerShell stores a newly boxed int in
$LASTEXITCODEevery time a native process exits, even when the code is the same, so the variable holds a different object exactly when this command ran a native process (or assigned the variable itself).$__af_lastkeeps the reference taken before the command. Like Python: reset $LASTEXITCODE per command in persistent PowerShell sessions #8206, this never writes$LASTEXITCODE, so the user's own commands can still read the previous code. The comments above the check now explain why it compares references.There is also a regression test,
test_persistent_powershell_reports_repeated_native_exit_code. It runs a Python process that exits with 3 three times and expects 3 each time, then checks that a cmdlet-only command reports 0 and that$LASTEXITCODEstill reads back 3. Without the fix it fails withAssertionError: attempt 2 reported rc 0. Liketest_persistent_powershell_returns_formatted_object_output, it is gated on PowerShell being installed rather than onsys.platform == "win32", and it usessys.executableas the native command, so it should also run in theubuntu-latestCI job.What is the impact of these changes?
Repeated native failures now report their own exit code. For the repro in the linked issue (Windows 11, PowerShell 7.6.6):
The other exit-code paths are unchanged. I compared before and after on PowerShell 7.6.6 and Windows PowerShell 5.1 for cmdlet-only commands in a fresh session (where
$LASTEXITCODEis$null) and after a native failure, reading$LASTEXITCODEback, a cmdlet error after a native failure, a native command followed by a cmdlet, several native commands in one command (the last one decides),$global:LASTEXITCODE = 9,throw, and table-formatted output. The only differences were the repeated-code cases, which now report the right code. Bash sessions and stateless mode are untouched.What do you want reviewers to focus on?
Whether relying on reference identity is acceptable. I checked it on PowerShell 7.6.6 and Windows PowerShell 5.1 for exit codes 0, 1, 2, 3, 127, 255 and -1, and both store a new boxed int on every native exit. The alternatives I considered either write
$LASTEXITCODEbefore the command, which Python: reset $LASTEXITCODE per command in persistent PowerShell sessions #8206 avoided so the user's commands can still read it, or need a different way to run the command thanInvoke-Expression.Related Issue
Fixes #9657
This is a one-line correction to the exit-code check added in #8206, so I opened it together with the issue.
AI Assistance
AI assistance details: I used AI coding assistants (Claude Code and Antigravity) to help find the bug and to draft the fix, the test, the linked issue and this description. I did the final review myself: I read and understood every change, checked the repro and test results, and made the final decisions on what to submit.
Contribution Checklist
breaking changelabel (or add "[BREAKING]" to the title prefix, before or after any language prefix) — a workflow keeps the label and title prefix in sync automatically.