Skip to content

CXH-2352: Name the inaccessible workspace and how to skip it on a 403 - #60

Open
al-conductorone wants to merge 2 commits into
mainfrom
cxh-2352-403-sync-abort-names-remediation
Open

CXH-2352: Name the inaccessible workspace and how to skip it on a 403#60
al-conductorone wants to merge 2 commits into
mainfrom
cxh-2352-403-sync-abort-names-remediation

Conversation

@al-conductorone

Copy link
Copy Markdown
Contributor

When a workspace can't be accessed (403), the sync now names that workspace and tells the operator to exclude it, instead of aborting with no guidance.

@linear-code

linear-code Bot commented Aug 28, 2026

Copy link
Copy Markdown

CXH-2352

Comment thread pkg/databricks/request.go Outdated
var apiErr *APIError
if errors.As(err, &apiErr) && apiErr.StatusCode == http.StatusForbidden {
return fmt.Errorf(
"workspace %s is inaccessible (403); scope it out with --databricks-exclude-workspaces (BATON_DATABRICKS_EXCLUDE_WORKSPACES): %w",

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🟡 Suggestion: databricks-exclude-workspaces is declared mutually exclusive with workspaces (pkg/config/config.go:93), and workspace-tokens requires workspaces (line 94). So for a workspace-token install — exactly the setup most likely to hit a per-workspace 403 — following this remedy produces a config-validation failure; the correct fix there is to drop the workspace from --databricks-workspaces. Consider mentioning both paths, e.g. "remove it from --databricks-workspaces, or scope it out with --databricks-exclude-workspaces".

Also note ListRoles/ListRuleSets are called from the group Grant/Revoke paths (pkg/connector/groups.go:397, :541), so a provisioning 403 will surface "scope it out of the sync" as its remedy, which reads as misleading advice for a grant failure.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

+1 on both points. exclude is a dead end under token auth (needs --workspaces, mutually exclusive with exclude) — should mention removing from --workspaces too. and yeah, same string on Grant/Revoke via ListRoles/ListRuleSets reads like sync advice for a provisioning failure.

@github-actions

github-actions Bot commented Aug 28, 2026

Copy link
Copy Markdown
Contributor

Connector PR Review: CXH-2352: Name the inaccessible workspace and how to skip it on a 403

Blocking Issues: 1 | Suggestions: 1 | Threads Resolved: 0
Criteria: Criteria status: loaded .claude/skills/ci-review.md from trusted base ef6c2aae70ca.
Review mode: incremental since abd0f34
View review run

Review Summary

The new commit reworks the 403 remedy string in nameWorkspace403Remedy so it also offers removing the workspace from the include list, addressing the prior finding that --databricks-exclude-workspaces is mutually exclusive with workspaces and therefore unusable on workspace-token installs. The fix is the right idea but names a flag that does not exist: the field is declared as workspaces, so the flag is --workspaces / BATON_WORKSPACES, not --databricks-workspaces. The full PR diff was also scanned for security and correctness; the error-enrichment wiring in client.go and the new table test are otherwise sound, and no security issues were found.

Security Issues

None found.

Correctness Issues

  • pkg/databricks/request.go:55 - the remedy points at --databricks-workspaces, but the config field is workspaces (pkg/config/config.go:35), so the real flag is --workspaces / BATON_WORKSPACES (README.md:175); an operator following this message hits unknown flag.

Suggestions

  • pkg/databricks/request.go:55 - on workspace-token installs ValidateConfig requires workspaces and workspace-tokens to be the same length, so the message should also say to drop the paired token; pkg/databricks/request_test.go:44 asserts only on databricks-exclude-workspaces, leaving the include-flag half of the string untested.
Prompt for AI agents
Verify each finding against the current code and only fix it if needed.

## Correctness Issues

In `pkg/databricks/request.go`:
- Around line 55: The 403 remedy message tells the operator to remove the workspace from
  --databricks-workspaces, but no such flag exists. The config field is declared as
  workspaces in pkg/config/config.go (WorkspacesField), so the CLI flag is --workspaces
  and the environment variable is BATON_WORKSPACES (confirmed by README.md line 175).
  Only ExcludeWorkspacesField carries the databricks- prefix. Change the message to name
  --workspaces (BATON_WORKSPACES) instead.

## Suggestions

In `pkg/databricks/request.go`:
- Around line 55: On workspace-token installs, ValidateConfig in pkg/config/config.go
  requires len(Workspaces) == len(WorkspaceTokens), so simply removing a workspace from
  --workspaces makes startup fail with a length-mismatch error. Extend the remedy text to
  say the paired workspace token must be removed too, for example: remove it from
  --workspaces (BATON_WORKSPACES) along with its paired workspace token.

In `pkg/databricks/request_test.go`:
- Around line 44: TestNameWorkspace403Remedy asserts the message contains
  databricks-exclude-workspaces but never checks the include-flag half of the string,
  which is why the wrong flag name slipped through. Add an assertion that the remedy
  names the actual include flag, for example a strings.Contains check on --workspaces.

@github-actions github-actions Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

No blocking issues found.

Comment thread pkg/databricks/request.go
// fix: excluding the workspace scopes it out of the sync. workspaceId is empty
// for account-scoped calls, where a 403 is not a per-workspace access problem,
// so those pass through untouched.
func nameWorkspace403Remedy(workspaceId string, err error) error {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

heads up vs the customer/CXH-2109 shape: this still returns the 403, so sync dies — it just has a better message. the 400 path in workspaces.go logs + returns empty and continues. for Gemini (and the pebble "no full sync found" follow-on), workspace-scoped role Grants (roles.go ListUsers/Groups/SPs) need the same skip-on-403, not only this remedy wrapper.

Comment thread pkg/databricks/client.go
ratelimitData, err := c.Get(ctx, u, &res, vars...)
if err != nil {
return nil, 0, ratelimitData, err
return nil, 0, ratelimitData, nameWorkspace403Remedy(workspaceId, err)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

this is the call behind the customer failure (failed to list service principals / Unauthorized access to Org). enriching here helps ops pick an exclude, but roleBuilder.Grants still propagates the error and aborts the whole sync — skip+empty at the Grants layer is what actually unblocks a full sync.

--databricks-exclude-workspaces is mutually exclusive with --databricks-workspaces,
which workspace-token auth requires, so those installs cannot follow that advice.
Add the 'remove it from --databricks-workspaces' path so the remedy is actionable
for both auth modes (and both sync and grant/revoke 403s).
Comment thread pkg/databricks/request.go
var apiErr *APIError
if errors.As(err, &apiErr) && apiErr.StatusCode == http.StatusForbidden {
return fmt.Errorf(
"workspace %s is inaccessible (403); remove it from --databricks-workspaces, or scope it out with --databricks-exclude-workspaces (BATON_DATABRICKS_EXCLUDE_WORKSPACES): %w",

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🟠 Bug: --databricks-workspaces is not a real flag. The field is declared as "workspaces" in pkg/config/config.go:35, so the CLI flag is --workspaces and the env var is BATON_WORKSPACES (see README.md:175) — only the exclude field carries the databricks- prefix. An operator following this message gets unknown flag. Also worth noting: on workspace-token installs workspaces and workspace-tokens must stay the same length (ValidateConfig), so removing an entry means removing its paired token too.

Suggested change
"workspace %s is inaccessible (403); remove it from --databricks-workspaces, or scope it out with --databricks-exclude-workspaces (BATON_DATABRICKS_EXCLUDE_WORKSPACES): %w",
"workspace %s is inaccessible (403); remove it from --workspaces (BATON_WORKSPACES) along with its paired workspace token, or scope it out with --databricks-exclude-workspaces (BATON_DATABRICKS_EXCLUDE_WORKSPACES): %w",

@github-actions github-actions Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Blocking issues found — see review comments.

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants