feat: disconnect any provider, purging cached secret + identity (task 004)#5
Merged
Conversation
… 004) Adds a disconnect action for every connected source. Disconnecting purges every secret MLT cached for that source under its OWN keychain service (a user-entered API key and/or a refreshed-OAuth copy), forgets the cached account identity, and clears consent — so the tile disappears and refresh stops immediately, without a restart, and the source can be reconnected afterwards. The vendor's own credential store is only ever read, never written or deleted (ADR 0012/0016). - core: SourceDescriptor gains oauth_cache_key + cached_secret_keys() (the exact set of self-cached keys to purge); IdentityStore gains clear_identity (write-through delete with rollback, mirroring set_identity). - tauri: rename remove_api_key -> disconnect_source, generalised to any source kind; set_source_enabled's disable path now routes through the shared disconnect() core (purges secret + identity, then clears consent) instead of merely de-consenting. - claude (adjacent behaviour change): the OAuth refresher now returns a fresh vendor token as-is WITHOUT reading our keychain cache, avoiding a macOS keychain prompt on every refresh; the cache is consulted only when the vendor token is missing or stale. - ui: "Remove" -> "Disconnect"; removeApiKey -> disconnectSource. - docs: mark task 004 done (PRD §4 + task doc). Closes task 004.
📝 WalkthroughWalkthroughThis PR implements account disconnect functionality across both OAuth and API-key sources, unifying the removal flow under a single command that purges cached secrets and clears identity state. It also enhances the task extension to inline specification documents and hand tasks directly to the agent. ChangesUnified Account Disconnect Flow
Task Spec Inlining and Handoff
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
.omp/extensions/mlt-tasks/index.ts (1)
67-68: 💤 Low valueInlined spec can break the surrounding code fence.
The spec is wrapped in a 4-backtick ````markdown fence. A task doc that itself contains a fence of 4+ backticks would terminate the wrapper early and corrupt the prompt structure handed to the agent. Three-backtick fences are safe, but consider computing a fence longer than any run of backticks present in
specto be robust.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.omp/extensions/mlt-tasks/index.ts around lines 67 - 68, The current lines.push call in .omp/extensions/mlt-tasks/index.ts inlines task spec using a fixed 4-backtick fence which can be broken if spec contains equal-or-longer backtick runs; update the logic around the spec rendering (where spec is used and where lines.push is called) to compute the longest consecutive backtick sequence in spec and generate a fence of length (maxRun + 1), then use that fence (e.g., fence + 'markdown' and closing fence) instead of the hardcoded four backticks so the wrapper cannot be prematurely terminated.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.omp/extensions/mlt-tasks/index.ts:
- Line 185: Await or handle rejections from pi.sendUserMessage called with
startPrompt(task, branch, spec) so transport/agent errors don't become unhandled
and the UI toast reflects real delivery status; update the call at
pi.sendUserMessage(startPrompt(task, branch, spec), idle ? undefined : {
deliverAs: 'followUp' }) to either await the promise (inside the surrounding
async function) or attach .catch(...) and on success/failure update the toast
accordingly, ensuring any thrown error is propagated to the command’s existing
error handling path.
In `@src-tauri/src/lib.rs`:
- Around line 195-203: The current flow deletes cached secrets via
descriptor.cached_secret_keys()/secrets.delete(...) then returns early if
identity.clear_identity(...) or consent.set_enabled(..., false) fails, which can
leave consent true after secrets are purged; change the post-delete steps so
they are non-fatal and always attempt to disable consent: replace the chained
map_err? calls for identity.clear_identity and consent.set_enabled with explicit
error handling that (1) always calls consent.set_enabled(&descriptor.id, false)
(retry/log if it fails) even if clear_identity fails, (2) does not early-return
after a post-delete failure but collects/logs errors, and (3) guarantees the
consent flag is forced false (best-effort, with retries or compensating action)
before finishing so ApiKey sources cannot remain reported as connected after
secret deletion.
---
Nitpick comments:
In @.omp/extensions/mlt-tasks/index.ts:
- Around line 67-68: The current lines.push call in
.omp/extensions/mlt-tasks/index.ts inlines task spec using a fixed 4-backtick
fence which can be broken if spec contains equal-or-longer backtick runs; update
the logic around the spec rendering (where spec is used and where lines.push is
called) to compute the longest consecutive backtick sequence in spec and
generate a fence of length (maxRun + 1), then use that fence (e.g., fence +
'markdown' and closing fence) instead of the hardcoded four backticks so the
wrapper cannot be prematurely terminated.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 13978e9e-e824-4349-8b77-58148ba6b8f2
📒 Files selected for processing (10)
.omp/extensions/mlt-tasks/index.tscrates/adapters/src/identity.rscrates/core/src/ports.rscrates/core/src/providers/claude.rscrates/core/src/sources.rsdocs/PRD.mddocs/tasks/004-disconnect-provider.mdsrc-tauri/src/lib.rssrc/lib/usage.tssrc/routes/+page.svelte
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
1 issue found across 10 files
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
…ommand Addresses PR #5 review (CodeRabbit + cubic). - disconnect() ordering (src-tauri): purge secret -> clear consent -> forget identity (was secret -> identity -> consent). For an `ApiKey` source `SourceState::active` is consent alone, so the old order could leave a source enabled-but-keyless — still reading as "connected" and refreshing against a now-missing key — if the cosmetic identity-cache clear failed after the secret was already purged. Consent is now cleared before the identity clear (so it sticks), and any error is still surfaced. The secret is still purged first, so a failure there aborts with nothing else changed. Regression test added: disconnect_clears_consent_even_when_forgetting_identity_fails. - mlt-tasks extension: handle pi.sendUserMessage rejection instead of fire-and-forget. The call goes through the prompt flow, which can reject (e.g. model/API-key validation when idle); surface it via a toast rather than leaving an unhandled rejection after the success toast. - mlt-tasks extension: compute the inlined-spec wrapper fence dynamically (longest backtick run + 1, min 3) so a task doc containing a >=4-backtick fence can no longer terminate the wrapper early.
Owner
Author
Update — addressed automated review (commit
|
This was referenced Jun 3, 2026
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
1 participant
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.
TL;DR
Implements task 004 — disconnect a provider without restart. Adds a disconnect
action for every connected source. Disconnecting purges every secret MLT cached for
that source under its own keychain service, forgets the cached account identity,
and clears consent — so the tile disappears and refresh stops immediately, with no
restart, and the source can be reconnected afterwards.
Security invariant preserved end-to-end: only copies we wrote are deleted; the
vendor's own credential store (e.g. Claude Code's keychain item) is only ever read,
never written or deleted (ADR 0012 / 0016).
Closes task 004. All four acceptance criteria ticked in
docs/tasks/004-disconnect-provider.md.Beyond the disconnect feature, this branch also changes the Claude OAuth refresher
(
crates/core/src/providers/claude.rs): when Claude Code's own token is fresh it is nowreturned as-is, without reading our keychain cache, so the OS no longer prompts for
keychain access on every refresh. The cache (and a network refresh) are consulted only
when the vendor token is missing or stale. It's adjacent to task 004 (it surfaces while
exercising connect/reconnect), but it is a real behaviour change — please review it on its
own merits, not as "cleanup". Covered by a new test that fails loudly if the cache is read.
What changed, by layer (dependency order)
crates/core/src/ports.rsIdentityStoregainsclear_identity(idempotent).crates/core/src/sources.rsSourceDescriptor.oauth_cache_key+cached_secret_keys()— the exact set of keys MLT itself cached for a source.crates/core/src/providers/claude.rscrates/adapters/src/identity.rsFileIdentityStore::clear_identity— write-through delete with rollback, mirroringset_identity.src-tauri/src/lib.rsremove_api_key→disconnect_source(now works for any source kind); shareddisconnect()core;set_source_enableddisable path purges instead of de-consenting.src/lib/usage.ts,src/routes/+page.svelteremoveApiKey→disconnectSource; button label "Remove" → "Disconnect".docs/PRD.md,docs/tasks/004-disconnect-provider.md.omp/extensions/mlt-tasks/index.ts/mlt:start-tasknow inlines the task spec and hands it straight to the agent. Pure local dev tooling — no app behaviour.Where to start reading
crates/core/src/sources.rs—cached_secret_keys()defines what disconnect purges.src-tauri/src/lib.rs—disconnect()is the shared core; note secrets are purgedbefore consent is cleared, so a keychain failure leaves the source still connected
rather than half-removed.
crates/core/src/providers/claude.rs— the refresher fast-path (reviewer note above).Risk & invariants
cached_secret_keys()returns onlyapi_key.<id>and/or our
oauth.<provider>copy — never the vendor's own item. Asserted bycatalog_declares_each_source_self_cached_secretandcached_secret_keys_lists_only_what_mlt_caches_itself.no-op success; another source's secret is left untouched
(
disconnect_is_idempotent_and_scoped_to_this_sources_own_keys).keychain error.
Test coverage (all new/affected, green locally)
mlt-core:cached_secret_keys_lists_only_what_mlt_caches_itself,catalog_declares_each_source_self_cached_secret,refresher_skips_the_cache_when_the_vendor_token_is_fresh.mlt-adapters:clear_forgets_an_identity_and_persists_the_removal,clear_is_idempotent_for_an_absent_source.mlt(tauri):disconnect_purges_an_api_key_sources_secret_and_consent,disconnect_purges_a_reused_logins_cached_oauth_copy_and_identity,disconnect_is_idempotent_and_scoped_to_this_sources_own_keys.make lint(clippy-D warnings),make purity, andmake test(pre-push) all pass.Manual QA
Per the task doc: disconnect a provider, then confirm via Keychain Access that our
cached item (
com.bigshotpictures.mlt) is gone and the vendor's own item is untouched,the tile disappears without a restart, and the provider reconnects cleanly.
References
docs/tasks/004-disconnect-provider.md0016 API-key connection model,
0017 provider account identity
Summary by cubic
Add a disconnect action for any provider that purges our cached secret(s), clears consent, and forgets identity so the tile disappears immediately without a restart. Meets task 004 and never touches vendor credential stores.
New Features
disconnect_sourceor by disabling consent: purge our keychain entries (api_key.*and anyoauth.*we cached), clear consent, then callIdentityStore::clear_identity. Takes effect instantly; providers can be reconnected. UI: "Remove" → "Disconnect";removeApiKey→disconnectSource.com.bigshotpictures.mltitems are deleted; operation is idempotent and scoped to the selected source. Ordering hardened to purge → clear consent → forget identity to avoid an enabled-but-keyless state.Refactors
IdentityStore::clear_identity;SourceDescriptor.oauth_cache_keyandcached_secret_keys()declare exactly what to purge.remove_api_keygeneralized todisconnect_source, andset_source_enabled’s disable path routes through the shared disconnect flow.Written for commit 4d657bb. Summary will update on new commits.
Summary by CodeRabbit
New Features
Documentation