Skip to content

[dotnet-code] Consolidate shell argv helper#536

Open
michelle-clayton-work wants to merge 3 commits into
mainfrom
dotnet-code-shell-argv-helper-eaa3b82eebbe6f4f
Open

[dotnet-code] Consolidate shell argv helper#536
michelle-clayton-work wants to merge 3 commits into
mainfrom
dotnet-code-shell-argv-helper-eaa3b82eebbe6f4f

Conversation

@michelle-clayton-work

Copy link
Copy Markdown
Contributor

Summary

Consolidated shell argv assembly through small unexported helpers so stateless and persistent shell launch paths share the same extra-argv composition logic. This keeps the Go shell internals closer to the .NET ResolvedShell shape, where shell-specific suffixes are selected separately from combining constructor-provided extra argv.

.NET Reference

  • dotnet/src/Microsoft.Agents.AI.Tools.Shell/ShellResolver.cs - resolves shell kind and combines optional extra argv with stateless/persistent shell suffixes.

Public API and Behavior

No public Go API changed. No intentional behavior change was made.

Tests

  • Added TestResolvedShellArgvIncludesExtraArgv to preserve existing stateless and persistent argv shapes with ShellArgv extras.
  • gofmt -w tool/shelltool/localshell.go tool/shelltool/localshell_internal_test.go
  • go test ./tool/shelltool

Notes

Rejected sampled candidates:

  • dotnet/src/Microsoft.Agents.AI.Workflows/HandoffWorkflowBuilder.cs - Go handoff-related workflow builder changes would touch public/broader workflow behavior, and existing open [dotnet-code] workflow PRs already cover nearby workflow internals.
  • dotnet/src/Microsoft.Agents.AI.Workflows/IExternalRequestContext.cs - the Go external request area already has direct behavior coverage, and the sampled .NET interface did not suggest a narrow production cleanup without churn.
  • dotnet/src/Microsoft.Agents.AI.Mcp/Skills/McpJsonContext.cs - the .NET file is source-generated JSON context plumbing, with no useful direct Go equivalent beyond existing MCP JSON helpers.

Checked for open shelltool/ShellResolver [dotnet-code] PR overlap and found none before editing.

Generated by .NET-to-Go Code Portability Refactoring Agent · 520.7 AIC · ⌖ 31.4 AIC · ⊞ 20.8K ·

Closes #486

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Comment thread tool/shelltool/localshell.go Fixed

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Pull request overview

This PR refactors the internal shell argv composition in tool/shelltool by consolidating “extra argv + shell-specific suffix” assembly into small unexported helpers, so stateless and persistent shell launch paths share the same composition logic (mirroring the .NET ResolvedShell shape more closely).

Changes:

  • Replaced the standalone combineArgv helper with resolvedShell.argvWithExtra and resolvedShell.launchArgv to centralize argv assembly.
  • Updated stateless and persistent argv construction to use the new helpers (no intended behavior change).
  • Added a unit test to lock in expected argv shapes when extraArgv is present for both stateless and persistent bash paths.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.

File Description
tool/shelltool/localshell.go Consolidates argv assembly via argvWithExtra/launchArgv and updates stateless/persistent argv builders to use them.
tool/shelltool/localshell_internal_test.go Adds coverage ensuring extraArgv is included in both stateless and persistent argv outputs.

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

@github-actions

This comment has been minimized.

@michelle-clayton-work

Copy link
Copy Markdown
Contributor Author

@copilot respond to PR comments and fix codeQL failure

Copilot AI and others added 2 commits July 17, 2026 21:21
Co-authored-by: michelle-clayton-work <262183035+michelle-clayton-work@users.noreply.github.com>
…intent

Co-authored-by: michelle-clayton-work <262183035+michelle-clayton-work@users.noreply.github.com>

Copilot AI commented Jul 17, 2026

Copy link
Copy Markdown
Contributor

@copilot respond to PR comments and fix codeQL failure

Fixed in commit fix: use slices.Clone in argvWithExtra to avoid overflow and clarify intent. Replaced make([]string, 0, len(s.extraArgv)+len(suffix)) with slices.Clone(s.extraArgv) to eliminate the potentially overflowing capacity sum. The slices package is already imported in other files in this package. CodeQL now reports 0 alerts.

@github-actions

Copy link
Copy Markdown
Contributor

API Consistency Review — No Parity Issues

This PR consolidates shell argv assembly into two unexported helpers (argvWithExtra, launchArgv) on the internal resolvedShell type. All changed symbols are unexported; no public Go API was added or modified.

Scope determination: Out of scope for cross-repo parity review. The change is a Go-internal refactor with no user-visible contract changes, matching the PR's own statement: "No public Go API changed. No intentional behavior change was made."

No cross-repo consistency issues found.

Generated by Go API Consistency Review Agent for issue #536 · 28.8 AIC · ⌖ 11.5 AIC · ⊞ 26.8K ·

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.

[dotnet-code] Consolidate shell argv helper

4 participants