- Notifications
You must be signed in to change notification settings - Fork948
feat: add workspace SSH execution tool for AI SDK#18924
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Conversation
It works! Seeing some weird errors though |
e976eea to678cabcCompareChange-Id: I61f694a89e33c60ab6e5a68b6773755bff1840a4Signed-off-by: Thomas Kosiewski <tk@coder.com>
678cabc to3970946Comparecoderabbitaibot commentedJul 21, 2025 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
📝 WalkthroughWalkthroughA new tool, Changes
Sequence Diagram(s)sequenceDiagram participant User participant ToolHandler as WorkspaceBash Handler participant SDK as codersdk.Client participant Workspace participant Agent participant SSH User->>ToolHandler: Provide workspace name & command ToolHandler->>ToolHandler: Normalize workspace input ToolHandler->>SDK: Find workspace & agent alt Workspace stopped ToolHandler->>SDK: Start workspace and wait for build end ToolHandler->>Agent: Wait for agent to be ready ToolHandler->>SSH: Establish SSH connection ToolHandler->>SSH: Execute command SSH-->>ToolHandler: Return output & exit code ToolHandler-->>User: Return trimmed output & exit codeEstimated code review effort4 (~90 minutes) ✨ Finishing Touches
🪧 TipsChatThere are 3 ways to chat withCodeRabbit:
SupportNeed help? Create a ticket on oursupport page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
@CodeRabbit review |
✅ Actions performedReview triggered.
|
1 similar comment
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Actionable comments posted: 0
🧹 Nitpick comments (2)
codersdk/toolsdk/bash.go (1)
96-108:Consider adding timeout configuration for SSH operations.The SSH dial and connection operations don't have explicit timeouts, which could cause the tool to hang indefinitely if the agent is unresponsive.
Consider adding a timeout to the dial options:
// Dial agent conn, err := wsClient.DialAgent(ctx, workspaceAgent.ID, &workspacesdk.DialAgentOptions{ BlockEndpoints: false,+// Add a reasonable timeout if the context doesn't already have one })You might also want to ensure the context has a deadline if it doesn't already:
// At the beginning of the handler, after validationif_,ok:=ctx.Deadline();!ok {varcancel context.CancelFuncctx,cancel=context.WithTimeout(ctx,5*time.Minute)defercancel()}codersdk/toolsdk/toolsdk_test.go (1)
31-50:Helper function follows established patterns but consider test isolation.The
setupWorkspaceForAgentfunction replicates the SSH test setup pattern correctly. However, there are a few observations:
- The function creates a hardcoded username "myuser" which could potentially cause issues in concurrent tests
- The function properly uses
t.Helper()to improve error reporting- The nolint comments are appropriate for test code
Consider using a unique username to avoid potential conflicts:
-userClient, user := coderdtest.CreateAnotherUserMutators(t, client, first.OrganizationID, nil, func(r *codersdk.CreateUserRequestWithOrgs) {-r.Username = "myuser"-})+userClient, user := coderdtest.CreateAnotherUserMutators(t, client, first.OrganizationID, nil, func(r *codersdk.CreateUserRequestWithOrgs) {+r.Username = fmt.Sprintf("myuser-%d", time.Now().UnixNano())+})However, based on the retrieved learnings, unique identifiers should be used in concurrent tests to prevent race conditions.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
codersdk/toolsdk/bash.go(1 hunks)codersdk/toolsdk/bash_test.go(1 hunks)codersdk/toolsdk/toolsdk.go(2 hunks)codersdk/toolsdk/toolsdk_test.go(4 hunks)
🧰 Additional context used
🧠 Learnings (3)
codersdk/toolsdk/toolsdk.go (1)
Learnt from: CR
PR: coder/coder#0
File: .cursorrules:0-0
Timestamp: 2025-07-21T14:32:43.035Z
Learning: Applies to coderd/coderdtest/**/* : Thecoderdtest package incoderd/coderdtest/ provides utilities for creating test instances of the Coder server, setting up test users and workspaces, and mocking external components.
codersdk/toolsdk/toolsdk_test.go (10)
Learnt from: CR
PR: coder/coder#0
File: .cursorrules:0-0
Timestamp: 2025-07-21T14:32:43.035Z
Learning: Applies to coderd/coderdtest/**/* : Thecoderdtest package incoderd/coderdtest/ provides utilities for creating test instances of the Coder server, setting up test users and workspaces, and mocking external components.
Learnt from: CR
PR: coder/coder#0
File: .cursorrules:0-0
Timestamp: 2025-07-21T14:32:43.034Z
Learning: Applies to coderd/coderd.go : The REST API is defined incoderd/coderd.go and uses Chi for HTTP routing.
Learnt from: CR
PR: coder/coder#0
File: CLAUDE.md:0-0
Timestamp: 2025-07-21T14:32:56.474Z
Learning: Applies to **/*_test.go : Never use hardcoded names in concurrent Go tests
Learnt from: CR
PR: coder/coder#0
File: .cursorrules:0-0
Timestamp: 2025-07-21T14:32:43.035Z
Learning: Applies to **/*.go : Coder emphasizes clear error handling, with specific patterns required: Concise error messages that avoid phrases like "failed to"; Wrapping errors with%w to maintain error chains; Using sentinel errors with the "err" prefix (e.g.,errNotFound).
Learnt from: CR
PR: coder/coder#0
File: .cursorrules:0-0
Timestamp: 2025-07-21T14:32:43.035Z
Learning: Applies to coderd/dbauthz/*.go : The database authorization (dbauthz) system enforces fine-grained access control across all database operations. All database operations must pass through this layer to ensure security.
Learnt from: ThomasK33
PR:#18809
File: coderd/apidoc/swagger.json:2160-2186
Timestamp: 2025-07-21T14:30:40.568Z
Learning: For the Coder repository, API specification or standards compliance issues should be raised in the Go source files (e.g., handler annotations or swaggo comments), not in auto-generated files like coderd/apidoc/swagger.json.
Learnt from: CR
PR: coder/coder#0
File: .cursorrules:0-0
Timestamp: 2025-07-21T14:32:43.035Z
Learning: Applies to coderdenttest/**/* : Enterprise features have dedicated test utilities in thecoderdenttest package.
Learnt from: CR
PR: coder/coder#0
File: .cursorrules:0-0
Timestamp: 2025-07-21T14:32:43.035Z
Learning: Applies to **/*_test.go : All tests should run in parallel usingt.Parallel() to ensure efficient testing and expose potential race conditions.
Learnt from: CR
PR: coder/coder#0
File: .cursorrules:0-0
Timestamp: 2025-07-21T14:32:43.035Z
Learning: Applies to **/*_test.go : All tests must uset.Parallel() to run concurrently, which improves test suite performance and helps identify race conditions.
Learnt from: CR
PR: coder/coder#0
File: CLAUDE.md:0-0
Timestamp: 2025-07-21T14:32:56.474Z
Learning: Applies to **/*_test.go : Use unique identifiers in concurrent Go tests to prevent race conditions (e.g., fmt.Sprintf with t.Name() and time.Now().UnixNano())
codersdk/toolsdk/bash_test.go (5)
Learnt from: CR
PR: coder/coder#0
File: CLAUDE.md:0-0
Timestamp: 2025-07-21T14:32:56.474Z
Learning: Applies to **/*_test.go : Never use hardcoded names in concurrent Go tests
Learnt from: CR
PR: coder/coder#0
File: .cursorrules:0-0
Timestamp: 2025-07-21T14:32:43.035Z
Learning: Applies to coderd/coderdtest/**/* : Thecoderdtest package incoderd/coderdtest/ provides utilities for creating test instances of the Coder server, setting up test users and workspaces, and mocking external components.
Learnt from: CR
PR: coder/coder#0
File: CLAUDE.md:0-0
Timestamp: 2025-07-21T14:32:56.474Z
Learning: Applies to **/*_test.go : Use unique identifiers in concurrent Go tests to prevent race conditions (e.g., fmt.Sprintf with t.Name() and time.Now().UnixNano())
Learnt from: CR
PR: coder/coder#0
File: .cursorrules:0-0
Timestamp: 2025-07-21T14:32:43.035Z
Learning: Applies to **/*_test.go : All tests should run in parallel usingt.Parallel() to ensure efficient testing and expose potential race conditions.
Learnt from: CR
PR: coder/coder#0
File: .cursorrules:0-0
Timestamp: 2025-07-21T14:32:43.035Z
Learning: Applies to **/*_test.go : All tests must uset.Parallel() to run concurrently, which improves test suite performance and helps identify race conditions.
🔇 Additional comments (6)
codersdk/toolsdk/toolsdk.go (1)
36-36:LGTM!The new tool constant and registration follow the established patterns in the codebase.
Also applies to: 187-187
codersdk/toolsdk/bash_test.go (1)
1-162:Excellent test coverage!The test file follows all best practices:
- All tests use
t.Parallel()as required- Comprehensive coverage of input validation, error scenarios, and normalization
- Well-structured test cases with clear naming
- Good use of table-driven tests for normalization
codersdk/toolsdk/bash.go (1)
1-296:Well-implemented SSH execution tool!The implementation is thorough and handles various edge cases well:
- Comprehensive workspace name format support
- Auto-start functionality for stopped workspaces
- Proper error handling and exit code extraction
- Good separation of concerns with helper functions
- Clear documentation and examples
codersdk/toolsdk/toolsdk_test.go (3)
19-19:Import addition looks correct.The import for
agenttestis properly added and will be used in the new SSH execution test.
462-462:Good improvement: using test context instead of background context.The change from
context.Background()tot.Context()is a proper improvement that:
- Ensures test timeout compliance
- Allows for proper cancellation when tests are stopped
- Follows Go testing best practices
399-448:No parallel execution in toolsdk_test.go—hardcoded “myuser” is safeTestTools (func TestTools at line 55) and its “WorkspaceSSHExec” subtest (around line 399) do not invoke t.Parallel(), so all steps—including the hardcoded “myuser” in setupWorkspaceForAgent—run sequentially. There’s no concurrent test execution here, so no race condition risk.
Likely an incorrect or invalid review comment.
326c024 intomainUh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Add SSH Command Execution Tool for Coder Workspaces
This PR adds a new AI tool
coder_workspace_ssh_execthat allows executing commands in Coder workspaces via SSH. The tool provides functionality similar to thecoder ssh <workspace> <command>CLI command.Key features:
workspace(uses current user)owner/workspaceowner--workspaceworkspace.agent(specific agent)owner/workspace.agentThe implementation includes:
This tool enables AI assistants to execute commands in user workspaces, making it possible to automate tasks and provide more interactive assistance.
Summary by CodeRabbit