feat: add workspace SSH execution tool for AI SDK #18924
Conversation
|
It works! Seeing some weird errors though |
ThomasK33
force-pushed
the
thomask33/07-20-feat_toolsdk_add_ssh_exec_tool
branch
2 times, most recently
from
e976eea to
678cabc
Compare
Change-Id: I61f694a89e33c60ab6e5a68b6773755bff1840a4 Signed-off-by: Thomas Kosiewski <tk@coder.com>
ThomasK33
force-pushed
the
thomask33/07-20-feat_toolsdk_add_ssh_exec_tool
branch
from
678cabc to
3970946
Compare
📝 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 code
Estimated code review effort4 (~90 minutes) ✨ Finishing Touches
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support 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.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
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 validation if _, ok := ctx.Deadline(); !ok { var cancel context.CancelFunc ctx, cancel = context.WithTimeout(ctx, 5*time.Minute) defer cancel() }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/**/* : The coderdtest package in coderd/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/**/* : The coderdtest package in coderd/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 in coderd/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 the coderdenttest 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 using t.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 use t.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/**/* : The coderdtest package in coderd/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 using t.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 use t.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.
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