Skip to content

fix: clean up OAuth token cache on provider deletion#7908

Merged
jh-block merged 3 commits intoblock:mainfrom
vincenzopalazzo:claude/wonderful-haibt
Mar 17, 2026
Merged

fix: clean up OAuth token cache on provider deletion#7908
jh-block merged 3 commits intoblock:mainfrom
vincenzopalazzo:claude/wonderful-haibt

Conversation

@vincenzopalazzo
Copy link
Contributor

Summary

  • Adds a provider cleanup mechanism so cached OAuth token files are removed when a provider configuration is deleted
  • Fixes the issue where re-configuring a deleted OAuth provider (e.g., GitHub Copilot, Databricks) silently reuses stale tokens without opening a browser for fresh authorization
  • Providers register static cleanup functions in the registry; cleanup is called from the UI, backend API (POST /config/providers/{name}/cleanup), and CLI before config keys are deleted

Test plan

  • Configure GitHub Copilot provider (OAuth flow opens browser)
  • Verify ~/.config/goose/githubcopilot/info.json exists after configuration
  • Delete the GitHub Copilot provider in the UI
  • Verify info.json is removed from disk
  • Re-configure GitHub Copilot — should open browser again for fresh OAuth
  • Verify cargo build and cargo test pass
  • Verify frontend TypeScript type-checks (npx tsc --noEmit)

Closes #7890

🤖 Generated with Claude Code

@vincenzopalazzo vincenzopalazzo force-pushed the claude/wonderful-haibt branch from d0f5db7 to e4c94c2 Compare March 15, 2026 11:36
Copy link

@chatgpt-codex-connector chatgpt-codex-connector bot left a comment

Choose a reason for hiding this comment

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

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: d0f5db7f81

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

@@ -79,6 +79,16 @@ async fn init_registry() -> RwLock<ProviderRegistry> {
registry.register::<VeniceProvider, _>(|m| Box::pin(VeniceProvider::from_env(m)), false);
registry.register::<XaiProvider, _>(|m| Box::pin(XaiProvider::from_env(m)), false);
});
// Register cleanup functions for providers with cached state
registry.set_cleanup(
"githubcopilot",

Choose a reason for hiding this comment

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

P1 Badge Register Copilot cleanup under the actual provider name

The cleanup hook is registered for "githubcopilot", but the provider is keyed as "github_copilot" in its metadata and registry entry, so cleanup_provider("github_copilot") never finds this callback. In the UI/API deletion flow (which passes provider.name), GitHub Copilot token cache cleanup is silently skipped, so deleting and reconfiguring Copilot can still reuse stale OAuth state.

Useful? React with 👍 / 👎.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed in f40eb6e — changed "githubcopilot" to "github_copilot" to match the actual provider registry key.


// Clean up provider-specific cache files (e.g., OAuth tokens) before removing config
if let Err(e) =
tokio::runtime::Handle::current().block_on(goose::providers::cleanup_provider(selected_id))

Choose a reason for hiding this comment

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

P1 Badge Remove sync block_on call from provider deletion path

This uses tokio::runtime::Handle::current().block_on(...) inside the configure command flow, which already runs on Tokio; calling block_on from within an active runtime can panic at runtime. As a result, selecting “Remove Custom Provider” can crash the CLI before deletion/cleanup completes.

Useful? React with 👍 / 👎.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed in f40eb6e — made remove_provider() and configure_custom_provider_dialog() async, replacing block_on with .await.

@vincenzopalazzo vincenzopalazzo force-pushed the claude/wonderful-haibt branch from e4c94c2 to 4365b60 Compare March 15, 2026 11:46
Copy link

@chatgpt-codex-connector chatgpt-codex-connector bot left a comment

Choose a reason for hiding this comment

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

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 4365b6011f

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment on lines +256 to +259
return createSseClient({
...opts,
body: opts.body as BodyInit | null | undefined,
headers: opts.headers as unknown as Record<string, string>,

Choose a reason for hiding this comment

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

P1 Badge Pass serialized body to SSE requests

makeSseFn stopped passing serializedBody into createSseClient, but createSseClient builds RequestInit with body: options.serializedBody, so SSE POST payloads are now dropped. This breaks the desktop chat path where useChatStream calls reply() with session_id and user_message: /reply receives an empty/malformed body and cannot deserialize ChatRequest, so streaming replies fail.

Useful? React with 👍 / 👎.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This diff is from the upstream rebase (auto-generated client.gen.ts by @hey-api/openapi-ts), not from this PR. No changes needed on our end.

Copy link
Collaborator

Choose a reason for hiding this comment

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

after rebasing or merging in main, you need to just generate-openapi so that the generated files are based on the combination of your changes and main.

Can you rebase or merge main and run that? You don't need to manually resolve the conflicts on the generated files when rebasing/merging, just run the generator and it will replace the files with correct versions

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done — rebased on main and ran just generate-openapi with the correct @hey-api/openapi-ts v0.93.0. just check-openapi-schema, cargo check, and tsc --noEmit all pass. Should be good now.

vincenzopalazzo and others added 2 commits March 17, 2026 17:55
…ation

When deleting an OAuth provider (e.g., GitHub Copilot, Databricks), cached
token files on disk were not removed, causing re-configuration to silently
reuse stale tokens without opening a browser for fresh authorization.

Adds a provider cleanup mechanism via the registry: providers can register
a static cleanup function that removes their cache files. Called from the
UI, backend API, and CLI before config keys are deleted.

Closes block#7890

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Signed-off-by: Vincenzo Palazzo <vincenzopalazzodev@gmail.com>
- Fix provider name from "githubcopilot" to "github_copilot" to match
  the actual registry key
- Make remove_provider() and configure_custom_provider_dialog() async
  to avoid block_on panic inside tokio runtime

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Signed-off-by: Vincenzo Palazzo <vincenzopalazzodev@gmail.com>
@vincenzopalazzo vincenzopalazzo force-pushed the claude/wonderful-haibt branch from f40eb6e to 3406a75 Compare March 17, 2026 17:03
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Signed-off-by: Vincenzo Palazzo <vincenzopalazzodev@gmail.com>
@vincenzopalazzo vincenzopalazzo force-pushed the claude/wonderful-haibt branch from 3406a75 to 9fe0201 Compare March 17, 2026 17:27
@jh-block jh-block enabled auto-merge March 17, 2026 17:34
@jh-block jh-block added this pull request to the merge queue Mar 17, 2026
Merged via the queue into block:main with commit 8271756 Mar 17, 2026
21 checks passed
@vincenzopalazzo vincenzopalazzo deleted the claude/wonderful-haibt branch March 17, 2026 20:22
jh-block added a commit that referenced this pull request Mar 18, 2026
…ct-ui

* origin/main:
  docs: add Remote Access section with Telegram Gateway documentation (#7955)
  fix: update webmcp blog post metadata image URL (#7967)
  fix: clean up OAuth token cache on provider deletion (#7908)
  fix: hard-coded tool call id in code mode callback (#7939)
  Fix SSE parsers to accept optional space after data: prefix (#7929)
  docs: add GOOSE_INPUT_LIMIT to config-files.md (#7961)
  Add WebMCP for Beginners blog post (#7957)
  Fix download manager (#7933)

# Conflicts:
#	ui/desktop/src/api/index.ts
#	ui/desktop/src/api/sdk.gen.ts
jh-block added a commit to rabi/goose that referenced this pull request Mar 18, 2026
* main: (32 commits)
  Revert message flush & test (block#7966)
  docs: add Remote Access section with Telegram Gateway documentation (block#7955)
  fix: update webmcp blog post metadata image URL (block#7967)
  fix: clean up OAuth token cache on provider deletion (block#7908)
  fix: hard-coded tool call id in code mode callback (block#7939)
  Fix SSE parsers to accept optional space after data: prefix (block#7929)
  docs: add GOOSE_INPUT_LIMIT to config-files.md (block#7961)
  Add WebMCP for Beginners blog post (block#7957)
  Fix download manager (block#7933)
  Improve the formatting of tool calls, show thinking, treat Reasoning and Thinking as the same thing (sorry Kant) (block#7626)
  don't imply running builds all the time in AGENTS.md (block#7865)
  fix: unregister goosed child process's  listener (block#7956)
  feat: adversarial agent for preventing leaking of info and more  (block#7948)
  Update contributing.md (block#7927)
  docs: add credit balance monitoring section (block#7952)
  docs: add Cerebras provider to supported providers list (block#7953)
  docs: add TUI client documentation to ACP clients guide (block#7950)
  fix: removed double dash in pnpm command (block#7951)
  docs: polish ACP docs (block#7946)
  claude adaptive thinking (block#7944)
  ...
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.

OAuth provider configuration not fully deleted - cache file persists

2 participants