Skip to content

fix: align shell guidance with execution - #5900

Merged
Hmbown merged 3 commits into
Hmbown:mainfrom
zhuowp:fix/shell-environment-guidance-upstream
Sep 6, 2026
Merged

fix: align shell guidance with execution#5900
Hmbown merged 3 commits into
Hmbown:mainfrom
zhuowp:fix/shell-environment-guidance-upstream

Conversation

@zhuowp

@zhuowp zhuowp commented Sep 5, 2026

Copy link
Copy Markdown
Contributor

The model-visible lowercase bash tool and the legacy Bash handler do not identify the actual interpreter selected by ShellDispatcher. This lets models infer Bash syntax on PowerShell hosts.

Derive cached tool and command guidance from the existing dispatcher for PowerShell, cmd, Bash, POSIX sh, zsh, fish, and custom executable paths. Preserve the visible foreground tool's timeout/output/approval contract, and the hidden legacy tool's blocking wait / wait=false contract. The tool prefix stays stable for the process. Execution, permissions, schema fields, and tool names are unchanged.

Validation on 47c719b:

  • Full library target compiled; guidance selection: 23 passed, 0 failed, including catalog consistency and actual visible-tool fixture export.
  • lowercase_bash contract selection: 10 passed, 0 failed. Tests use RUST_MIN_STACK=16777216 per upstream development guidance, and installed Git usr/bin on the test PATH for an existing cat-based read-only fixture. Its initial missing-cat failure was resolved without code changes.
  • Rust formatting and diff checks passed. No live-model or native non-Windows acceptance is claimed for this upstream port; downstream model results are a separate experiment.

This complements PowerShell invocation hardening in #4593. The r13 backport is tracked separately in Pinvou#42.

No-Issue: This guidance gap was reproduced during downstream model testing; no matching open upstream issue was identified.

Signed-off-by: z27014 <zhuo.wenpei@h3c.com>
@github-actions

github-actions Bot commented Sep 5, 2026

Copy link
Copy Markdown
Contributor

Thanks @zhuowp for taking the time to contribute.

This repository is observing a maintainer-managed PR intake gate in dry-run mode, so this pull request is staying open. This note helps maintainers prepare the allowlist before any enforcement is considered.

Please read CONTRIBUTING.md for the expected contribution shape. A maintainer can grant recurring PR access by commenting /lgtm on a pull request.

Signed-off-by: z27014 <zhuo.wenpei@h3c.com>

@Hmbown Hmbown left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Kind: approve-with-nits. Verified on main: tools/shell.rs:4361-4374/:4541-4560 hardcode bash/zsh guidance while execution selects the interpreter via shell_dispatcher::global_dispatcher() (shell_dispatcher.rs:548) — deriving both tool descriptions from the dispatcher (new shell/guidance.rs) is the right layer and changes no execution path. Catalog-consistency + ShellKind-matrix tests are solid; DCO and the full CI matrix are green on head. Two nits: (1) nothing pins that foreground_description() keeps the Ask/sandbox_permissions retry sentence from the old hardcoded text — please assert it in shell/tests.rs; (2) the stale failed link check from your first commit is blocking merge state — re-run or we'll clear it on our side. Thanks for the careful upstream port.

Signed-off-by: z27014 <zhuo.wenpei@h3c.com>
@zhuowp

zhuowp commented Sep 6, 2026

Copy link
Copy Markdown
Contributor Author

Addressed the foreground-guidance nit in 985f38b. The existing catalog regression now asserts the complete original Ask / sandbox_permissions retry and approval sentence on the actual visible LowercaseBashTool description. Production guidance is unchanged.

Validation: rustfmt and git diff --check passed; the full library test target rebuilt and tools::shell::tests::shell_catalog_guidance_matches_execution passed (1 passed, 0 failed). An independent repair/review pass also confirmed the assertion covers the requested contract.

@Hmbown
Hmbown merged commit 714479d into Hmbown:main Sep 6, 2026
24 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants