fix: name the Windows command interpreter in the Runtime prompt section - #1573
Open
Ramnath0521 wants to merge 2 commits into
Open
fix: name the Windows command interpreter in the Runtime prompt section#1573Ramnath0521 wants to merge 2 commits into
Ramnath0521 wants to merge 2 commits into
Conversation
TurnRunner built the Runtime prompt block with
os.environ.get("SHELL", ""). SHELL is a POSIX variable that Windows
never sets, and system_prompt.j2 renders the field unconditionally, so
every Windows turn told the model:
## Runtime
- OS: Windows
- Shell:
An empty declared field is worse than an absent one: the model is told a
shell exists, given nothing to identify it, and falls back to POSIX
idioms that then fail on the Windows command runner.
process_tree.default_command_shell() names the interpreter that actually
runs agent commands, and create_owned_subprocess_shell now calls it for
its Windows branch instead of reading COMSPEC separately, so the value
reported to the model and the value handed to CreateProcess cannot
drift. POSIX behaviour is unchanged.
Refs TokenRhythm#941
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Refs #941.
What is actually wrong
The issue attributes the Unix-only command to the bundled PPTX skill's Bash-oriented examples. That is not where it comes from —
tailappears nowhere insrc/opensquilla/skills/bundled/pptx/, so editingSKILL.mdwould not change this behaviour. The| tail -3was produced by the model, and the reason it reaches for Bash idioms is upstream of the skill.TurnRunnerbuilds the Runtime prompt block inengine/runtime.py:SHELLis a POSIX variable; Windows does not set it.identity/templates/system_prompt.j2renders the field unconditionally, so every Windows turn ships this:An empty declared field is worse than an absent one: the model is told a shell exists, given nothing to identify it, and falls back to the POSIX idioms that dominate its priors —
| tail -3,2>&1,where soffice || echo. MeanwhileCOMSPECis populated and unused.Reproduction
Windows 11, Python 3.12, run from PowerShell (not Git Bash — Git Bash sets
SHELLand masks this):Rendering the real prompt through
assemble_system_promptwithruntime_infobuilt asruntime.pybuilds it yields the blank- Shell:line above. After the change the same render yields:The change
process_tree.default_command_shell()names the interpreter that actually runs agent commands, andcreate_owned_subprocess_shellnow calls it for its Windows branch instead of readingCOMSPECseparately. The value reported to the model and the value handed toCreateProcessare therefore the same expression and cannot drift.POSIX behaviour is deliberately unchanged — still
$SHELL. Worth noting for a maintainer: on POSIXcreate_owned_subprocess_shellexecs/bin/shrather than$SHELL, so that field is arguably imprecise there too. That is pre-existing, orthogonal to a Windows report, and left alone.Refsrather thanFixes: naming the interpreter removes the cause of the guessing, but it makes correct commands likely, not guaranteed. The issue's other asks — not fail-chaining install/verify/discovery into one command, and treating a missing LibreOffice as loss of visual QA only — are separate behavioural changes and are not in this PR.Scope
Scope boundary:
process_tree.default_command_shell()plus its two call sites — the Windows branch ofcreate_owned_subprocess_shelland theruntime_info["shell"]value inengine/runtime.py.Non-goals: the PPTX skill text, the shape of generated dependency probes, LibreOffice/visual-QA fallback behaviour, and POSIX shell reporting.
Branch
Base branch: main
Target exception: N/A
Issue
Linked issue: Refs #941
Release Note
Release note: The agent's Runtime prompt section now names the Windows command interpreter (
COMSPEC) instead of leaving the shell field blank.Tests
Ruff:
ruff check src tests— All checks passed. (ruff format --checkreports these files as unformatted, but reports them identically on unmodifiedmain; I confirmed that against a pristine checkout and left it alone. CI does not runruff format.)Pytest, on Windows 11 / Python 3.12, in three passes:
pytest tests/test_process_tree.py tests/test_identity -q: 117 passed, 14 skipped.pytest tests/functional -v: 20 passed, 15 skipped in 80s (skips are credential-gated live provider/Telegram/browser tests). This includestest_gateway_stop_process_tree_e2e.py, the closest e2e coverage of the modified module, which passes. These must be run serially: underpytest -n autothe socket-binding gateway tests deadlock, which is presumably why CI gives them a dedicated job.pytest -q -n auto --ignore=tests/functional --ignore=tests/live: 25,100 passed, 207 failed, 718 skipped.On those 207: they are pre-existing on this platform, not introduced here. Re-running that exact set of ids against unmodified
maingives 186 failures, versus 187 with the patch applied — and the two sets differ in both directions (8 fail only with the patch, 7 only without), which is the signature of parallel-execution flakiness rather than a regression. All 7 that failed only on the patched side pass when run serially. The bulk are Windows-environment failures concentrated intest_opensquilla_home_migration.py(61) andtest_turn_ingress_rpc.py(25), neither of which this change can reach.I have not tried to fix any of them; flagging the number so it is not mistaken for something this PR caused.
Build:
mypy src/opensquilla --show-error-codes— Success: no issues found in 1547 source files.Regression tests: added
Notes: Four cases in
tests/test_process_tree.py. Two cover the bug and two are controls, so the suite cannot pass by breaking the other platform:test_default_command_shell_reports_comspec_on_windowsmaintest_default_command_shell_falls_back_when_comspec_missingtest_default_command_shell_keeps_posix_shelltest_default_command_shell_is_empty_when_posix_shell_unsetAll four were written first and confirmed failing with
AttributeError: module 'opensquilla.process_tree' has no attribute 'default_command_shell'before the implementation existed. The existing POSIX assertion intests/test_identity/test_user_profile_prompt.py(- Shell: /bin/bash) still passes unchanged.Environment note: checks were run against
uv sync --extra dev --extra recommended --extra mcp --frozen, matching CI, after an earlier run on a stale environment produced misleading tool-version results.Maintainer Live Check
Maintainer live check: no
Surface: N/A
Third-Party Origin
Third-party origin: none
Authored by Claude (an AI coding agent) on the account owner's machine and with their authorization. The reproduction, the failing-test-first sequence and every check above were genuinely executed here rather than asserted; the account owner reviewed the diff before this was opened. Flagging the AI authorship plainly rather than leaving it to be inferred — happy to take any correction in review.
🤖 Generated with Claude Code