Skip to content

External Agent: Add Windows-compatible agent name derivation#729

Open
keyu98 wants to merge 2 commits intoentireio:mainfrom
keyu98:feature/windows-support-external-agent
Open

External Agent: Add Windows-compatible agent name derivation#729
keyu98 wants to merge 2 commits intoentireio:mainfrom
keyu98:feature/windows-support-external-agent

Conversation

@keyu98
Copy link

@keyu98 keyu98 commented Mar 19, 2026

Summary

External agent discovery currently fails to correctly derive agent names on Windows because:

  1. Windows executables have file extensions (.exe, .bat, .cmd) that get included in the agent name.
  2. The Unix executable-bit check (mode & 0o111) rejects all binaries on Windows, since Windows doesn't set POSIX execute permission bits.

Changes

  • Strip Windows executable extensions before deriving the agent name via a new stripExeExt() helper. This removes .exe, .bat, and .cmd suffixes so that entire-agent-foo.exe correctly resolves to agent name foo. On Unix this is a no-op.
  • Skip the executable-bit check on Windows (runtime.GOOS != "windows"), since Windows filesystems do not expose POSIX permission bits.

Files changed

  • cmd/entire/cli/agent/external/discovery.go

@keyu98 keyu98 requested a review from a team as a code owner March 19, 2026 09:25
@keyu98 keyu98 changed the title feat: Add Windows-compatible agent name derivation External Agent: Add Windows-compatible agent name derivation Mar 19, 2026
Copy link
Contributor

@nodo nodo left a comment

Choose a reason for hiding this comment

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

Thank you for the contribution @keyu98 ❤️ I have added a suggestion to simplify stripExeExt, also do you mind adding tests for it? The idea is great, just a couple of minor points.

@keyu98
Copy link
Author

keyu98 commented Mar 24, 2026

Thank you for the contribution @keyu98 ❤️ I have added a suggestion to simplify stripExeExt, also do you mind adding tests for it? The idea is great, just a couple of minor points.

Thanks for the review! I've simplified stripExeExt as suggested and added tests for it in the latest commit.

Copy link
Contributor

@nodo nodo left a comment

Choose a reason for hiding this comment

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

LGTM, please have a look at the failing test, then I am happy to approve.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

2 participants