Repository navigation
Add aitools mcp login, logout and list --check - #7551
Draft
simonfaltum wants to merge 66 commits into
Draft
simonfaltum wants to merge 66 commits into
simonfaltum wants to merge 66 commits into
Conversation
simonfaltum
changed the base branch from
aitools-mcp/06a-bridge
to
aitools-mcp/06b-serve
November 12, 2026 12:42
simonfaltum
force-pushed
the
aitools-mcp/07-login
branch
from
November 12, 2026 12:42
f4c096c to
62d20d0
Compare
Contributor
|
Commit: Regression test reportSelected 14 changed tests (limit=20, 0 not selected) 14 tests: regression=14 unreleased=0 coverage=0 failing=0
TestAccept/cmd/aitools/mcp/add-agent-not-installed: branch ✅ | main (8e70c8b) ❌ | latest (v1.26.0) ❌main (8e70c8b): latest (v1.26.0): TestAccept/cmd/aitools/mcp/add-default-config: branch ✅ | main (8e70c8b) ❌ | latest (v1.26.0) ❌main (8e70c8b): latest (v1.26.0): TestAccept/cmd/aitools/mcp/add-text: branch ✅ | main (8e70c8b) ❌ | latest (v1.26.0) ❌main (8e70c8b): latest (v1.26.0): TestAccept/cmd/aitools/mcp/add-two-agents: branch ✅ | main (8e70c8b) ❌ | latest (v1.26.0) ❌main (8e70c8b): latest (v1.26.0): TestAccept/cmd/aitools/mcp/add-two-profiles: branch ✅ | main (8e70c8b) ❌ | latest (v1.26.0) ❌main (8e70c8b): latest (v1.26.0): 9 more failure details omitted, see the full report in the run. |
This was referenced Nov 12, 2026
Collaborator
✅ Integration test reportCommit: 4a8c08c
Top 9 slowest tests (at least 2 minutes):
|
simonfaltum
force-pushed
the
aitools-mcp/06b-serve
branch
from
November 12, 2026 14:37
eec9d1a to
5b1d279
Compare
simonfaltum
force-pushed
the
aitools-mcp/07-login
branch
from
November 12, 2026 14:41
62d20d0 to
d7c9aec
Compare
simonfaltum
force-pushed
the
aitools-mcp/06b-serve
branch
from
November 12, 2026 17:10
5b1d279 to
9187003
Compare
simonfaltum
force-pushed
the
aitools-mcp/07-login
branch
from
November 12, 2026 17:13
d7c9aec to
d2dd977
Compare
simonfaltum
force-pushed
the
aitools-mcp/06b-serve
branch
from
November 13, 2026 00:15
9187003 to
48fac6f
Compare
simonfaltum
force-pushed
the
aitools-mcp/07-login
branch
2 times, most recently
from
November 13, 2026 07:16
0aa7987 to
a08bc42
Compare
Provider sign-in for the external MCP services (GitHub, Slack and others). login opens the sign-in page, prints its URL first and polls the caller's credential until it is ACTIVE. logout deletes the credential and treats a missing one as success. add gains --login and --no-login, and list gains --check, which mints a token per profile, reads sign-in state and checks the recorded binary. The testserver serves the user-credentials GET, POST and DELETE routes. A POST completes a sign-in, which the acceptance scripts use in place of the browser. Co-authored-by: Isaac <no-reply@databricks.com>
…nce cases mcp add now refuses an agent named with --agents whose config directory is missing. These two cases add for claude-code in a fresh HOME, so they create the directory first. Their goldens do not change. Co-authored-by: Isaac <no-reply@databricks.com>
The note ends with a period since "Tidy MCP discovery comments, notes and error mapping" on the discovery branch. The stack now includes that commit, so the golden shows the period. Nothing else in the output changes. Co-authored-by: Isaac <no-reply@databricks.com>
Card 03 moved MCP records into the `mcp` section of ~/.databricks/aitools/skills/.state.json. The list-check and login-flips-active scripts read or edited the old path. The login golden changes only in the echoed jq command. Co-authored-by: Isaac <no-reply@databricks.com>
The sign-in acceptance tests stubbed the service GET with [[Server]] entries, but `add` also lists the services of system.ai, and the stubs do not answer the list. Create the services in the fake workspace from the scripts, as the other MCP acceptance tests do. The sign-in state is kept per workspace, and tests without stubs share one fake workspace per token. Give each sign-in test its own token, so that one test's sign-in cannot end another test's wait. Co-authored-by: Isaac <no-reply@databricks.com>
The add help lists the new --login and --no-login flags. The list output is the one the list rewrite produces: profiles name their config file when their servers are pinned to one, the JSON carries each server's host and status, and an unchanged agent entry is installed even when the recorded binary is gone, which the binary check reports. Co-authored-by: Isaac <no-reply@databricks.com>
Refactor without behavior change: SignIn takes the sign-in URL the caller builds, and no longer takes the host and workspace ID separately; the URL is built where it is printed and recorded. binaryStatus resolves the recorded command with binaryPathFor, so it shares the lookup seam. The full-name check, the agent-reached check, and the unexported token field of the list profile also reuse the existing helpers. Co-authored-by: Isaac <no-reply@databricks.com>
login and logout check the profile binding before any request, as add and serve do, so a profile whose host or workspace changed is refused with the same message, and a missing profile gets add's wording. login names the profile's current workspace in the sign-in URL. logout looks the service up first: a service the profile cannot see is an error instead of 'not signed in'. A service whose source connection was deleted is not found, as Discover drops it. Failures from the API are described as add describes them, so a rejected credential is AUTH_FAILED with the login command. The fake answers a sign-in to a service that does not exist with NOT_FOUND. Co-authored-by: Isaac <no-reply@databricks.com>
…ttempt A FAILED credential from an earlier sign-in is no longer the result of a new one: FAILED counts only once the state has changed since the attempt started. The wait is cancelled after the timeout, which also cuts off a poll that is still in flight, so the command ends at the timeout rather than when the SDK's retries of that poll end. Co-authored-by: Isaac <no-reply@databricks.com>
add --login attempts each installed sign-in server, so one failed sign-in no longer leaves the later servers unsigned. Each server that failed gets the login command, as a server that was not signed in by a plain add does. The sign-in failures are joined with the agent failures, so the exit status and the JSON error both report them. topLevelFailure keeps the failures that are not per-agent when a run error joins both; install and remove never join them, so they are unchanged. Co-authored-by: Isaac <no-reply@databricks.com>
…s own file Each profile group is checked with its own config file, in the context and in the process environment the token store reads, and the environment is restored afterwards. Two groups of one profile name in two files no longer share the ambient file's token store. A token that cannot be minted, or a sign-in that cannot be read, is a warning that names the profile or server and says why, and the result stays unknown. A service the profile can no longer see is a warning with unknown sign-in, not a sign-in that is needed. Co-authored-by: Isaac <no-reply@databricks.com>
The sign-in hints shell-quote the service and profile they print, and share one login command. The sign-in notice names the profile only when the page is about to open, so a login that is already signed in does not print it. The sign-in error messages start lowercase, as the package's other errors do. list's --check comments and command docs no longer cite the design document, and the list command documents that only --check makes network calls. Co-authored-by: Isaac <no-reply@databricks.com>
…wait longer for the background sign-in Git Bash on Windows rewrites the /api/ path of the credential POSTs unless MSYS_NO_PATHCONV is set. The login acceptance test waits for the background command's sign-in line with a retry budget of 30 seconds, instead of the default 2.5 seconds. Co-authored-by: Isaac <no-reply@databricks.com>
Co-authored-by: Isaac <no-reply@databricks.com>
The SDK puts a resource name into the request path unescaped. A service name with a '#' or '?' cut the path short, so logout of such a name sent DELETE to the service itself. The service part of each request is now escaped, and PathSafeName reports the names that change a path.
A service name with "/", "?", "#", "%" or a control character is refused before any request, with the same message as a name without three parts. Co-authored-by: Isaac <no-reply@databricks.com>
The endpoint already escapes the name. This refuses the same names as login and logout before serve reads the state, so the proxy never starts for them. Co-authored-by: Isaac <no-reply@databricks.com>
ConfigFileCommand is the one place a pinned config file enters a hint. On Windows the file was printed with backslashes, which broke the golden outputs and the rule that hints use forward slashes. Co-authored-by: Isaac <no-reply@databricks.com>
…d backgrounds a child The default opener swaps the output streams of pkg/browser for each open, so two opens that overlap (sign-in can start one while add --login starts another) could leave the streams swapped. A mutex now covers the swap and the open. A custom BROWSER command ran with piped stdout and stderr that were copied to stderr before Wait. A command that starts a child and returns, such as one that backgrounds the browser, kept the pipes open, so the copy never ended and the call hung. The command's output now goes to stderr directly, as the streams of the inherited output do, and stdin is inherited as it is for Open. Co-authored-by: Isaac <no-reply@databricks.com>
… failure Serve and Bind passed an empty pinned file to the auth error, so the login command for a server pinned to a config file was bare and logged in to the file the shell reads. A new helper, PinnedFile, gives the file for an entry pinned to one, and the sign-in command uses it too. Co-authored-by: Isaac <no-reply@databricks.com>
…-check warning The rejected-token warning for an unpinned profile gave a bare login command, which writes the file DATABRICKS_CONFIG_FILE names. The file is now derived from the caller's context before the check sets the environment to the profile's file, as the drift hints already do. Co-authored-by: Isaac <no-reply@databricks.com>
…r see Logout looked up the service before it deleted the credential, so a credential for a service whose source connection was deleted or unshared could not be removed. The credential is deleted first. The service is looked up only when there is no credential, to tell a name that is not a service from a caller who is not signed in. Co-authored-by: Isaac <no-reply@databricks.com>
…t fix The hint after a failed sign-in in add --login was printed for every failure except a few, so a refused sign-in (PERMISSION_DENIED) or a cancelled command got a login hint that would not fix it, and the hint was printed in JSON mode, unlike the other hints. The timeout and failed errors already name the login command, so the hint is removed. Co-authored-by: Isaac <no-reply@databricks.com>
… no longer shows A profile whose binding drifted is not checked, and its text listing gave no sign of that. The text now says the check was skipped and why, only when --check ran. A server that never needed sign-in gets no sign-in value when its service cannot be read, and the warning is the only report; a server that did need sign-in stays unknown. Co-authored-by: Isaac <no-reply@databricks.com>
…opener-failure test TestSignInPollsUntilActive read a slice the opener goroutine wrote, with no synchronization. The URL now arrives on a buffered channel. TestSignInReportsOpenerFailure depended on the poll interval winning against the opener goroutine. The credential now stays NOT_FOUND until the failure is printed, so the wait cannot end first. Co-authored-by: Isaac <no-reply@databricks.com>
… rule The doc said such names are refused, while the library escapes the names it reads from state. The commands refuse them before any request, and the library escapes the others. Co-authored-by: Isaac <no-reply@databricks.com>
… sign-in already active login and logout set the profile in the JSON document only after the state is read, so a failure to read it left the profile out. The profile is now set once it is resolved. A login when the credential is already ACTIVE printed 'Signed in' and the sign-in URL, although no page was opened. SignIn now reports that the caller was already signed in. The command says 'Already signed in' and leaves the URL out of the document, and add --login says the same. Co-authored-by: Isaac <no-reply@databricks.com>
…the stream and rate limiter sources Doc comments for runMcpSignIn, mcpLogin and mcpLogout say what each step records. The state checks in SignIn are switches. mcpLoginCommand moves to the top of its file. The comments about pkg/browser's streams and the SDK's rate limiter link to the source they describe. Co-authored-by: Isaac <no-reply@databricks.com>
The exhaustive linter requires every ProvisioningInfoState value in a switch. The states the sign-in does not act on are listed with the wait. Co-authored-by: Isaac <no-reply@databricks.com>
The overlapping-opens test relied on an empty PATH to stop the default opener, which does not hold on Windows, where pkg/browser uses ShellExecute. The launcher is now a variable that the tests replace, so no browser opens on any OS. The stream restore in the test helper runs in a defer, and the test with a backgrounded child waits for that child. Co-authored-by: Isaac <no-reply@databricks.com>
The default opener sent the launcher's error output to io.Discard. exec then copies that output through a pipe, so Run waited for every process still holding the pipe, including the browser the launcher started. The opener held its mutex for that whole time, so a second open, such as the second sign-in of add --login, waited too and printed nothing. The error output now goes to os.DevNull, a file that exec passes through directly. The test stubs the launcher with a command that leaves a child running. Co-authored-by: Isaac <no-reply@databricks.com>
The test checks that the command's stdout reaches the process's stderr and stays off stdout. The doc said that a command holding its output does not hold up Wait. That is true, but for the reason that the output is not piped, so the doc now says that. Co-authored-by: Isaac <no-reply@databricks.com>
A poll of the credential that failed with a server error or a network failure ended the sign-in, while the user may still be on the provider's page. Those failures are now polled again until the wait ends. Any other failed poll, such as a refused credential, still ends it at once. The timeout is now SignInTimeoutError, which carries the failure of the last poll that failed, and the timeout message names it. A poll that reads the credential clears it. Co-authored-by: Isaac <no-reply@databricks.com>
The JSON of login and of add --login named the URL whenever the sign-in was not already active, including when it failed before the page was printed, such as when the credential cannot be read. A URL that was never shown is not a way to finish the sign-in. SignIn now reports whether the URL was printed, and the URL is named only then. Co-authored-by: Isaac <no-reply@databricks.com>
The text listing said 'sign-in needed' without the login command that add prints for the same state. The server line now has a line below it with the login command, pinned to the config file the server is pinned to, as the other hints are. Co-authored-by: Isaac <no-reply@databricks.com>
…kens failed Any error from minting a profile's token marked the token failed, so an unreachable host, such as an offline laptop, was told to log in again. A token is now failed only when the credential is refused, and unknown otherwise, with the warning kept. The checks of a profile also ran on the SDK's five-minute retry budget, one profile after another, so one host that did not answer held the listing for minutes. Each profile's checks now end after the same five-second bound that cmd/auth gives profile validation, and the SDK's request and retry budgets for the profile are bounded too, since resolving the profile does not take the check's context. Co-authored-by: Isaac <no-reply@databricks.com>
A profile with a drifted server was not checked at all, so a server that still matched its profile, added after the host changed, was reported as not matching it. Each server is now skipped on its own status, and says why in the text and in its JSON check: drifted, when the profile no longer matches the server, or unknown, when the server could not be read. A profile with no server to check is not contacted. Co-authored-by: Isaac <no-reply@databricks.com>
…ument The document of add --login had no sign-in per server. The successful, already signed in and skipped sign-ins were not visible at all, and a failure appeared only in the top-level error, which joins the failures of every server. Each server now has a sign_in object with the state that login reports, and the error category and message when the sign-in failed. It is absent without --login, and for a server that reached no agent, where no sign-in was attempted. Co-authored-by: Isaac <no-reply@databricks.com>
The profile carried its own copy of the token check, which every server of the profile repeats in its check. The text now reads the token from the checked servers, and the profile keeps no copy. No output changes. Co-authored-by: Isaac <no-reply@databricks.com>
…date_file.py The setup posts of the list-check and login-flips-active scripts sent their output to /dev/null, where a failure would not show. They write LOG files, as the other aitools scripts do. The list-check script edited the state with inline jq; it now uses update_file.py, with the binary path JSON-escaped so the replacement matches on Windows too. Both servers of that scenario record the binary, so both now report it missing. Co-authored-by: Isaac <no-reply@databricks.com>
The two login hint tests and the two browser stdout tests are table tests now. The doc of signInHint describes the sentence it returns, not a command. openDefault and openWithCommand have doc comments, and the long comments of the sign-in wait and of signInStatus are shorter. No behavior changes. Co-authored-by: Isaac <no-reply@databricks.com>
The helper returns the timeout error for the test's later assertions. The one test that only needs the error to be a timeout discards it explicitly, which the errcheck linter accepts. Co-authored-by: Isaac <no-reply@databricks.com>
The time limit covered every request of a profile together: its token check plus two requests per sign-in server. A profile with several sign-in servers on a slow but healthy workspace ran out of time and reported its later servers as unknown. Each request now gets the limit on its own, so a host that never answers still ends the profile's checks. Co-authored-by: Isaac <no-reply@databricks.com>
A plain error is now a transient token failure, so the guard tests that expect the proxy to stop use a rejected token, and serveProxy takes the pinned config file in the tests added for the transient case. Co-authored-by: Isaac <no-reply@databricks.com>
Main classifies a StateError as STATE_INVALID, so a state file that does not parse reports that category in the JSON sign-in output rather than UNCATEGORIZED. Co-authored-by: Isaac <no-reply@databricks.com>
simonfaltum
force-pushed
the
aitools-mcp/06b-serve
branch
from
November 13, 2026 15:05
7064bbc to
10450c9
Compare
simonfaltum
force-pushed
the
aitools-mcp/07-login
branch
from
November 13, 2026 15:05
a08bc42 to
4a8c08c
Compare
This branch has not been deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Part of a stack that adds the hidden
databricks aitools mcpcommand group, on #7550 (aitools mcp serve). It adds provider sign-in for external MCP services such as GitHub, which each user signs in to before the service's tools work.Changes
mcp login <service>prints the sign-in URL, opens it throughlibs/browser, and waits up to 5 minutes. Server and network errors while polling are retried until the timeout, and the timeout names the last one. Other errors end the sign-in. An already signed-in caller gets "Already signed in" and no URL.mcp logout <service>deletes the caller's credential, even when its service is no longer visible.loginandlogoutrefuse a changed profile before any request, asaddandservedo.mcp add --loginsigns in to each sign-in-required server it installed. Its JSON gains a per-serversign_inobject with the stateloginreports. A failed sign-in exits 1 and keeps the installs.mcp list --checkchecks each profile's token, then each server on its own status. A drifted server or one that cannot be read is skipped, and says why. Each check request ends after 5 seconds. A sign-in that is needed names the login command.loginnames the URL in JSON only when the page was shown.DATABRICKS_CONFIG_FILE.libs/browsergainsOpenOutputToStderrfor JSON output.Openis unchanged for other callers. The default opener no longer waits for a browser its launcher leaves running.Why
A sign-in is judged on the attempt that runs: a FAILED credential from an earlier attempt is deleted before the page opens. A failed poll does not mean the user has left the provider's page, so it does not end the wait. One unreachable workspace held
list --checkfor the SDK's five-minute retry budget. The SDK puts the service name into the request path unescaped, so names are escaped.Decisions to review
SignInreturns(alreadySignedIn, urlShown bool, err error), not a state. The command derives the JSON state and URL from them.cmd/authuses for profile validation. A workspace slower than that showsunknownwhere a longer wait would answer.logoutsends the DELETE before it looks up the service, so a credential for a deleted service can be removed. A mistyped name costs one request.BROWSERcommand's output goes to stderr during sign-in, so it cannot land before the JSON document.list --checksetsDATABRICKS_CONFIG_FILEin the process environment while it checks a profile, because the token store reads the process environment.stateaddsFAILEDandNOT_SIGNED_INto the documented values.Tests
libs/browser:TestOpenDoesNotWaitForLauncherChildfailed before the fix. The overlapping-opens test stubs the launcher, so no browser opens on any OS.libs/aitools/mcp: polls through server errors, an authentication error ends at once, the timeout's last error.cmd/aitools:TestAddChecksBoundsAProfileThatNeverAnswershung past five minutes before the fix;TestAddChecksBoundsEachRequestNotTheProfilecovers a profile whose requests take longer than the limit together. Unreachable profiles are unknown, not failed. Skipped servers, thesign_inJSON, and the login URL rule are covered too.list-check(GitHub's recorded binary is now also missing) andlogin-flips-active.go test -race -count=3on the touched packages; the Taskfile's gotestsum flags withBROWSERunset,BROWSER=noneand a symlinkedTMPDIR;./task test-exp-aitools;go test ./acceptance -run TestAccept/cmd/aitools;GOOS=windows go vetandgo test -cforcmd/aitools,libs/browserandlibs/exec;./task lint-q,./task fmt-q,./task checks.This pull request and its description were written by Isaac.