fix: fall back to name lookup for UUID-shaped workspace names#24340
fix: fall back to name lookup for UUID-shaped workspace names#24340
Conversation
johnstcn
changed the title
… name saga Three packages had their own bespoke workspace-resolution logic with varying levels of UUID handling, separator support, and error behavior. Consolidate into a single codersdk.ResolveWorkspace method that does UUID-first lookup with 404-fallback to name-based lookup. - cli/root.go namedWorkspace now delegates to client.ResolveWorkspace - toolsdk GetWorkspace handler: 15 lines become 1 - toolsdk bash.go namedWorkspace+splitNameAndOwner: deleted, replaced with single-line delegation - Unit tests with httptest mock cover all resolution paths
splitWorkspaceIdentifier said "invalid workspace identifier" but TestDelete/InvalidWorkspaceIdentifier expected "invalid workspace name". Use "invalid workspace name" in both places for consistency.
- Rewrite stale toolsdk test comment that said "documents that bug" when the bug is now fixed - Remove unused parsedUUID variable from test mock handler - Replace context.Background() with t.Context() in all unit tests (Go 1.24+ convention)
- Add agent-backed WorkspaceBash regression coverage for UUID-like workspace names so bash/findWorkspaceAndAgent path is exercised - Strengthen ResolveWorkspace negative-path unit tests to prove the fallback request happens and invalid identifiers fail before HTTP - Refresh stale doc comments to match current behavior
There was a problem hiding this comment.
Pull request overview
Fixes workspace resolution when the user supplies an identifier that parses as a UUID (including dashless 32-hex strings) but is actually intended to be a workspace name, by adding a shared SDK resolver with a 404-based fallback.
Changes:
- Added
codersdk.Client.ResolveWorkspace, which tries UUID lookup first and falls back to owner/name lookup on 404. - Updated CLI and toolsdk workspace lookup paths to delegate to
ResolveWorkspace. - Added regression/unit tests covering UUID-like names across SDK, CLI, and toolsdk.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
codersdk/workspaces.go |
Introduces ResolveWorkspace + shared identifier splitting and 404 fallback logic. |
codersdk/workspaces_test.go |
Adds focused unit tests for resolver behavior (UUID, name, owner/name, UUID-like fallback, errors). |
codersdk/toolsdk/toolsdk.go |
Simplifies GetWorkspace tool handler to a single ResolveWorkspace call (with normalization). |
codersdk/toolsdk/bash.go |
Removes now-redundant parsing/-- handling and delegates to ResolveWorkspace. |
codersdk/toolsdk/toolsdk_test.go |
Adds regression tests ensuring tools resolve UUID-like workspace names correctly. |
cli/root.go |
Updates namedWorkspace to delegate to SDK resolver (fixing CLI 404 regression). |
cli/show_test.go |
Adds CLI regression test for showing a workspace whose name is a dashless UUID-like string. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
/coder-agents-review |
![coder-agents-review[bot]](https://avatars.githubusercontent.com/in/3454270?s=60&v=4){kind=small}
There was a problem hiding this comment.
Solid fix for a real bug. The UUID-like name resolution, 404-only fallback, and error type discrimination are correct. Test coverage at three levels (unit, integration, regression) is thorough. The centralization into codersdk.ResolveWorkspace eliminates three separate UUID-parse-and-dispatch implementations.
3 P3, 2 P4, 5 Nit. The P3s are: duplicate parsing function (DEREM-2), require.Equal in HTTP handler goroutines (DEREM-3), missing transport-error test case (DEREM-4), and an error message that says "workspace name" when it means "identifier" (DEREM-7). None blocks the feature.
CI is failing on lint. The emdash at workspaces_test.go:128 (DEREM-1) is a contributing cause. The PR description says "single source of truth for workspace resolution," but splitWorkspaceIdentifier duplicates cli/root.go:splitNamedWorkspace (DEREM-2); consider consolidating or updating the description.
"The UUIDLikeNameFallback test is the gem: it proves the actual bug fix by verifying the UUID endpoint fires first, gets a 404, and the name endpoint catches the fall." (Bisky)
🤖 This review was automatically generated with Coder Agents.
- Export SplitWorkspaceIdentifier, consolidate with cli splitNamedWorkspace - Use strings.Cut instead of Split+switch - Fix error message: 'workspace name' -> 'workspace identifier' - Add NameValid guard to skip wasted fallback for dashed UUIDs - Fix require in handler goroutines (use assert) - Add TransportError test case - Fix em-dash lint violation - Inline namedWorkspace in toolsdk/bash.go
Add OwnerWithUUIDLikeName and Forbidden cases.
|
/coder-agents-review |
There was a problem hiding this comment.
All 8 code fixes from round 1 verified against the new diff. CI green. The consolidation into SplitWorkspaceIdentifier and ResolveWorkspace is clean; zero references to the old functions remain. The NameValid guard correctly prevents wasted name-based round-trips for dashed UUIDs. DEREM-5 (empty owner/name gap) remains accepted as pre-existing.
1 P4, 1 Nit new this round. The P4 is a narrow test coverage gap (no error-path test for name-based resolution). The Nit is an unused struct field. Nothing blocking.
"The test suite with atomic hit counters is an effective pattern for verifying multi-step resolution logic." (Kite)
🤖 This review was automatically generated with Coder Agents.
namedWorkspaceincli/root.goparsed workspace identifiers withuuid.Parsefirst and returned immediately on success, even when no workspace had that UUID as its actual ID. This caused 404 errors for any workspace whose name was a valid 32-char hex string (dashless UUID).codersdk.ResolveWorkspace: tries UUID lookup first, falls back to name lookup on 404.NameValidguard skips the fallback for standard dashed UUIDs (36 chars > 32-char name limit).codersdk.SplitWorkspaceIdentifier, replacing the duplicatesplitNamedWorkspaceincli/root.go(usesstrings.Cut).namedWorkspacefromcli/root.go; all 28 call sites now useclient.ResolveWorkspacedirectly.namedWorkspaceandsplitNameAndOwnerfromcodersdk/toolsdk/bash.go; inlineclient.ResolveWorkspace.GetWorkspacetool handler to a singleResolveWorkspacecall.cli/show_test.goandcodersdk/toolsdkfor workspaces with UUID-like names.