[Workspaces] Polish per-user resolver UX: scoped error, surfaced log, shared constants#9797
Conversation
… shared constants Follow-ups after the per-user workspace resolver merge (#9755): * AMBIGUOUS recovery message no longer advertises `--workspace` as a universal fix — it only exists on `sky launch` / `sky jobs launch`, so callers running `sky status` / `sky queue` etc. were being pointed at a flag that doesn't apply. The three recovery options now read as parallel choices instead of replacements ("Or, for a one-shot override..."). * On `sky launch` / `sky jobs launch`, when the resolver implicitly picks PREFERRED or SINGLE_MEMBERSHIP, log "Using workspace 'X' (source: ...)" at INFO so users see where their cluster / job actually landed. EXPLICIT (user-named) and DEFAULT_FALLBACK (pre-existing implicit behavior) stay silent to avoid noise on the common cases. The whitelist correctly uses the prefixed task name (`sky.launch`) — the previous draft compared against the bare enum value and never matched at runtime. * `sky workspace info` payload contract: when the resolver raises WorkspaceAmbiguousError / NoWorkspaceAccessError / PermissionDenied, the handler now returns a state-coded `source` (ambiguous / no-access / permission-denied) plus a short single-line `note`, instead of dumping the multi-line exception message into `note` (which broke the CLI tree renderer). The CLI appends the AMBIGUOUS recovery hint as a separate paragraph below the tree, via a `WorkspaceAmbiguousError.recovery_hint()` classmethod shared between the exception and the CLI. * Move the `WORKSPACE_SOURCE_*` constants to a new leaf module `sky/workspaces/constants.py` so client-side callers (CLI) no longer drag the resolver's server-only deps (check, global_user_state, backend_utils, permission, resource_checker) into a read path. Tested: * `pytest tests/unit_tests/test_sky/test_cli_workspace.py tests/unit_tests/test_sky/users/test_preferred_workspace_endpoints.py tests/unit_tests/test_sky/workspaces/test_resolve_workspace_for_user.py tests/unit_tests/test_sky/server/requests/test_executor.py tests/unit_tests/test_sky/workspaces/test_resolver_compat_matrix.py` — 103 passed, including new regressions: - bare `'launch'` (no `sky.` prefix) MUST NOT match the whitelist (pins the prefix protocol from both sides) - DEFAULT_FALLBACK must stay silent (locks the new silent set) - AMBIGUOUS message keeps `--workspace` only in the launch footnote - ambiguous / no-access / permission-denied payloads carry their state code + short note Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
There was a problem hiding this comment.
Code Review
This pull request refactors workspace resolution error handling, CLI formatting, and logging. It introduces a new sky/workspaces/constants.py module to decouple workspace source constants from heavy server-side dependencies. The WorkspaceAmbiguousError exception is updated to expose a reusable recovery_hint() classmethod, and the CLI now renders this hint as a separate paragraph to maintain tree alignment. Additionally, the server's workspace GET endpoint now returns state-coded sources and short notes for ambiguous, no-access, and permission-denied states, and implicit workspace resolutions are now logged at the INFO level for resource-creating commands. There are no review comments, so no further feedback is provided.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
|
Manual test:
|
aylei
left a comment
There was a problem hiding this comment.
LGTM, can we have a smoke test covering this?
Summary
Follow-ups after the per-user workspace resolver merge (#9755):
--workspaceas a universal fix — it only exists onsky launch/sky jobs launch, so callers runningsky status/sky queueetc. were being pointed at a flag that doesn't apply on their command. Reworded so the three recovery options read as parallel choices (not replacements): "Or, for a one-shot override onsky launch/sky jobs launch, pass--workspace <name>."sky launch/sky jobs launchlands onPREFERREDorSINGLE_MEMBERSHIP, logUsing workspace 'X' (source: …)at INFO so the user sees where their cluster / job actually landed.EXPLICIT(user-named) andDEFAULT_FALLBACK(pre-existing implicit behavior) stay silent to avoid noise on the common cases. The whitelist now correctly uses the prefixed task name (sky.launch) — the previous draft compared against the bare enum value and never matched at runtime.sky workspace infopayload contract. When the resolver raisesWorkspaceAmbiguousError/NoWorkspaceAccessError/PermissionDeniedError, the handler now returns a state-codedsource(ambiguous/no-access/permission-denied) plus a short single-linenote, instead of dumping the multi-line exception text intonote(which broke the CLI tree renderer). The CLI appends the AMBIGUOUS recovery hint as a separate paragraph below the tree, via aWorkspaceAmbiguousError.recovery_hint()classmethod shared between the exception and the CLI.WORKSPACE_SOURCE_*constants to a new leaf modulesky/workspaces/constants.pyso client-side callers (CLI) no longer drag the resolver's server-only deps (check,global_user_state,backend_utils,permission,resource_checker) into a read path.Tested
103 passed. New regressions added:
test_resolution_log_silent_for_bare_launch_without_prefix— bare'launch'(nosky.prefix) must NOT match the whitelist. Pins the prefix protocol from both sides (positive tests use prefixed names, this one fails the moment the prefix is dropped).test_resolution_log_silent_when_source_default_fallback— DEFAULT_FALLBACK must stay silent.test_ambiguous_message_scopes_workspace_flag_to_launch—--workspacemust live in the launch footnote, not the universal section.test_ambiguous_resolver_surfaces_as_state_coded_source+test_no_workspace_access_surfaces_message_verbatim+test_permission_denied_surfaces_permission_denied_source— error payloads carry their state code + short note, never the multi-line exception text.test_ambiguous_source_appends_recovery_hint_below_tree— CLI appendsrecovery_hint()as a separate paragraph below the tree (asserts ordering vs. theAccessible:row).Manual verification:
defaultaccess →sky launcherrors with the new message structure (universalsky workspace use/~/.sky/config.yamloptions + parallel "Or, for a one-shot override..." footnote).sky workspace info→ tree stays aligned (Source: ambiguous, shortNote:), recovery hint renders as a separate paragraph below.preferred_workspace='team-a'runssky launch→ streamed output includesUsing workspace 'team-a' (source: preferred).fromexecutor.py. Repeat with no preferred but access todefault→ no extra log line.