Add DATABRICKS_BEARER_COMMAND for externally brokered tokens - #531
Add DATABRICKS_BEARER_COMMAND for externally brokered tokens#531dhruv0811 wants to merge 5 commits into
Conversation
`DATABRICKS_BEARER` lets a caller supply a pre-fetched bearer and skip the OAuth path, but it is a value, and a value cannot be rewritten in a running process. That makes it unusable for any caller whose bearer expires and has to be re-minted mid-session: an external credential broker, or a sidecar that holds the refresh token on the caller's behalf. Add the command form of the same hatch. When `DATABRICKS_BEARER_COMMAND` is set, `get_databricks_token` runs it and returns what it prints, on every fetch rather than once. Because every token consumer already funnels through that one function (`ucode auth-token` for Claude Code's apiKeyHelper and Codex's auth command, `mcp-proxy`, `gateway_proxy`, and the in-process agent launchers), they all pick this up without further change. Details: - Argv is `shlex.split`, not handed to `sh -c`, so this stays cross-platform for the same reason `build_auth_token_argv` moved off the POSIX pipeline. - Failure is closed, not a fall-through to OAuth. A broker-backed profile carries no OAuth cache to refresh, so falling through would report a misleading stale-login error instead of the real cause. This matches how `auth-token --use-pat` already fails closed. - `has_valid_databricks_auth` short-circuits on it too, otherwise `ensure_databricks_auth` probes the CLI and can open a browser for a workspace whose auth the caller already owns. - Precedence is unchanged where it already existed: a non-empty `DATABRICKS_BEARER` still wins, so `--use-pat` (which exports one) is unaffected. - The command's stdout is the bearer, so the debug log records only the exit code and stderr rather than going through `_format_subprocess_result`, which includes stdout on a non-zero exit. Inert unless the variable is set: every existing path is unchanged.
There was a problem hiding this comment.
🟡 Changes recommended
The command parsing uses POSIX shlex.split() semantics which can break Windows-style paths containing backslashes, undermining the PR’s cross-platform intent.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
Adds support for externally brokered Databricks bearer tokens by introducing DATABRICKS_BEARER_COMMAND, allowing get_databricks_token() to re-run a caller-provided command on each fetch (instead of relying on a static env var that can’t be updated mid-process).
Changes:
- Add
DATABRICKS_BEARER_COMMANDshort-circuit inget_databricks_token()andhas_valid_databricks_auth(), with fail-closed behavior and stderr-only debug logging. - Add a focused test suite covering precedence, re-execution per fetch, argv parsing (no shell), and failure modes.
File summaries
| File | Description |
|---|---|
| src/ucode/databricks.py | Implements the new command-based bearer resolution and auth short-circuiting logic. |
| tests/test_databricks.py | Adds tests validating behavior, precedence, and “no OAuth CLI fallback” guarantees. |
Review details
- Files reviewed: 2/2 changed files
- Comments generated: 1
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
`shlex.split` runs POSIX rules, which treat backslashes as escapes: a command of `C:\bin\broker.exe --arg` tokenized to `C:binbroker.exe`. That defeats the point of keeping this path shell-free for cross-platform use. `posix=False` is not the fix either. It preserves the backslashes but keeps the quotes inside the token, so a quoted path containing spaces breaks instead. Windows accepts the whole command line as one string and lets CreateProcess split it, which is the exact inverse of the string `build_auth_shell_command` emits there via `list2cmdline`. So pass the string through on Windows and keep `shlex.split` on POSIX. `run`'s annotation widens to `list[str] | str` to match what `subprocess.run` already accepts.
There was a problem hiding this comment.
🔵 Needs a closer look
The bearer-command path currently accepts stdout as a token even on non-zero exit codes, and the new tests hardcode PATH separators in a way that breaks Windows compatibility.
Review details
Suppressed comments (2)
Previously missed (2) — in code that hasn't changed since the last review.
src/ucode/databricks.py:1172
_bearer_from_commandreturns stdout as the token even when the command exits non-zero. That can accidentally treat an error message printed to stdout as a bearer (and masks genuine failures). It should fail closed on non-zero exit codes, regardless of stdout contents.
tests/test_databricks.py:3471_envhardcodes:when prepending to PATH. This will break these tests on Windows (PATH usesos.pathsep, typically;) and is easy to make platform-correct.
- Files reviewed: 2/2 changed files
- Comments generated: 0 new
- Review effort level: Lite
The resolver returned stdout whenever it was non-empty, regardless of the exit code, so a broker that printed a diagnostic to stdout and exited non-zero had that diagnostic forwarded as a bearer. It then failed as a 401 far from the real cause, which is exactly the misdirection this path exists to avoid. Require a zero exit as well as a non-empty stdout, matching `_fetch` in `get_databricks_token`, which already ignores output on a non-zero return. Also use `os.pathsep` rather than a literal `:` when the tests prepend to PATH. The fakes are `#!/bin/sh` scripts, so these tests stay POSIX-only either way (as does the existing `TestGetDatabricksToken` helper), but there is no reason to spell the separator by hand.
There was a problem hiding this comment.
🟡 Changes recommended
The new tests include a brittle PATH assumption and one test’s behavior doesn’t actually exercise the intended “printed no token” branch.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
- Files reviewed: 2/2 changed files
- Comments generated: 2
- Review effort level: Lite
`test_fails_closed_when_the_command_prints_no_token` exited 7 and asserted on "exited 7", so it duplicated the non-zero test added alongside it and left the zero-exit-empty-stdout branch uncovered. It now exits 0 with nothing on stdout and asserts the stderr reaches the error, so both failure branches are covered and each name matches its case. Also read PATH via `os.environ.get` so the helper does not KeyError in a hermetic environment.
There was a problem hiding this comment.
🔵 Needs a closer look
The new error paths include the full DATABRICKS_BEARER_COMMAND value in exceptions, which can leak sensitive command-line arguments into logs.
Review details
Suppressed comments (2)
Previously missed (1) — in code that hasn't changed since the last review.
src/ucode/databricks.py:1160
- The raised RuntimeError includes the full value of DATABRICKS_BEARER_COMMAND (a user-provided command line). If the command embeds sensitive arguments (client secrets, refresh tokens, etc.), this will leak into CLI output/CI logs. Consider omitting the command string (or only including the executable name) in the exception message.
This issue also appears on line 1170 of the same file.
src/ucode/databricks.py:1172
- This error message also echoes the full DATABRICKS_BEARER_COMMAND string, which can inadvertently expose secrets if they are passed as command arguments. Prefer leaving the command out (or redacting it) while still including the exit code and stderr.
reason = f"exited {result.returncode}" if result.returncode else "printed no token"
detail = f" Stderr: {stderr}" if stderr else ""
raise RuntimeError(f"DATABRICKS_BEARER_COMMAND {reason}. Command: {command}.{detail}")
- Files reviewed: 2/2 changed files
- Comments generated: 0 new
- Review effort level: Lite
`configure --workspaces` runs `databricks auth login` unconditionally. That
login cannot help when a bearer is supplied from outside: it is interactive, and
`get_databricks_token` returns before it would ever reach the OAuth path. In a
sandbox whose credential comes from a broker there is no browser to satisfy it,
so `ucode configure` hangs. Found by running this branch inside a Kubernetes
sandbox Pod, where configure sat on `databricks auth login` forever.
Both hatches are affected, so this fixes the pre-existing `DATABRICKS_BEARER`
case too, and the two checks collapse into one predicate:
def external_bearer_configured() -> bool
used by `has_valid_databricks_auth` and by configure's forced-login branch.
`--use-pat` already had its own non-interactive path; this gives the same
property to a caller that brings its own bearer.
Summary
DATABRICKS_BEARERlets a caller supply a pre-fetched bearer and skip the OAuth path. It is a value, though, and a value can't be rewritten in a running process. So it doesn't work for any caller whose bearer expires and has to be re-minted mid-session: an external credential broker, or a sidecar holding the refresh token on the caller's behalf.This adds the command form of the same hatch. When
DATABRICKS_BEARER_COMMANDis set,get_databricks_tokenruns it and returns what it prints, on every fetch rather than once.The reason it's this small: every token consumer already funnels through that one function, so they all inherit it.
Design points
shlex.split, notsh -c. Same reasonbuild_auth_token_argvmoved off the POSIXdatabricks ... | jqpipeline in Windows: ucode claude fails with apiKeyHelper POSIX shell error #116: plain argv runs identically on macOS, Linux, and Windows.auth-token --use-patalready fails closed (cli.py), for the same reason.has_valid_databricks_authshort-circuits on it too. Otherwiseensure_databricks_authprobes the CLI and can open a browser for a workspace whose auth the caller already owns.DATABRICKS_BEARERstill wins, so--use-pat(which exports one viaensure_pat_bearer) is unaffected._format_subprocess_result, which includes stdout on a non-zero exit.Motivation
We're wiring Databricks credentials into sandboxed coding-agent sessions in omnigent, where the sandbox deliberately holds no long-lived credential. It gets a short-lived handle plus the coordinates of a broker that vends a workspace token on demand, and the token is re-minted per request rather than written to disk.
ucodealready does the right thing structurally:apiKeyHelperand codex'sauth.commandare re-invoked commands, not baked values. The only missing piece was a way to point that resolution at something other than the local OAuth cache. With this, the whole integration on our side is exporting one env var, and no per-harness config code.Useful outside that case too: any CI or M2M setup whose bearer outlives a single fetch but not the session.
Test plan
tests/test_databricks.py::TestBearerCommand, 7 cases. Each puts a recording fakedatabricksonPATHthat would happily serve a token, so asserting the marker file is absent asserts the OAuth path was never reached.token-1, thentoken-2), which is the point of the change--coords 'a path'arrives as two argv entries)DATABRICKS_BEARERstill winshas_valid_databricks_authshort-circuits, CLI untouchedNot run locally:
tests/test_e2e.py(needs a workspace). Inert unless the variable is set, so every existing path is unchanged.Copilot review
"
shlex.splitbreaks Windows paths" — confirmed and fixed.shlex.split(r"C:\bin\broker.exe --arg")returns['C:binbroker.exe', '--arg'], which defeats the cross-platform point of keeping this path shell-free.Took a different fix than suggested, though.
posix=Falseswaps one Windows failure for another: it keeps the backslashes but leaves the quotes inside the token, so"C:\Program Files\ucode\ucode.exe" auth-tokentokenizes as['"C:\\Program Files\\ucode\\ucode.exe"', ...]. Instead, Windows takes the whole command line as one string and lets CreateProcess split it, which is the exact inverse of the stringbuild_auth_shell_commandalready emits there vialist2cmdline. So the string passes through on Windows andshlex.splitstays on POSIX, and both a bareC:\...path and a quoted path with spaces round-trip correctly.run's annotation widens tolist[str] | strto match whatsubprocess.runalready accepts. Covered bytest_windows_hands_the_command_line_over_verbatim.Second Copilot pass
"Returns stdout as the token even on a non-zero exit" — correct, and a real hole: a broker that printed its error to stdout and exited non-zero had that error forwarded as a bearer, resurfacing as a 401 far from the cause. Now requires a zero exit as well as non-empty stdout, matching
_fetchinget_databricks_token, which already ignores output on a non-zero return. Covered bytest_fails_closed_when_the_command_exits_non_zero."
_envhardcodes:when prepending to PATH" — switched toos.pathsep. Worth being clear that this does not make the tests Windows-capable: the fakedatabricksand broker are#!/bin/shscripts, so they are POSIX-only regardless of separator, as is the existingTestGetDatabricksToken._fake_databrickshelper they mirror. CI runsubuntu-latestfor both jobs. Making this file cross-platform means replacing the shell-script fakes repo-wide, which is a separate change.Found by running this branch in a Kubernetes sandbox
configure --workspacescalledrun_databricks_loginunconditionally, soucode configureinside a sandbox Pod sat on an interactivedatabricks auth loginforever. That login cannot help a caller who brings their own bearer: it needs a browser the Pod does not have, andget_databricks_tokenreturns before it would ever reach the OAuth path.Both hatches were affected, so this also fixes the pre-existing
DATABRICKS_BEARERcase, and the two short-circuit checks collapse into one predicate:now used by
has_valid_databricks_authand by configure's forced-login branch.--use-patalready had a non-interactive path; this gives the same property to a caller supplying a bearer or a bearer command. Covered byTestForcedLoginWithExternalBearer(skips for both hatches, still logs in when neither is set).One more gap this surfaced, left alone here: ucode's bootstrap hard-requires the
databricksCLI binary even whenDATABRICKS_BEARER_COMMANDmakes it unnecessary for minting, and its auto-install shellssudo, which fails in a rootless Pod (sh: 1: sudo: not found). Worth a separate look.