fix(config): save empty config before clearing keychain entries

[OwenYWT]

Summary

Prevent config remove from leaving config and keychain state inconsistent when saving the empty config fails.

Changes

• save the empty config before removing app secrets and user tokens
• add a regression test proving save failure preserves the existing config and keychain-backed secret reference

Test Plan

• GOCACHE=/tmp/go-build-cache GOMODCACHE=/tmp/go-mod-cache /opt/homebrew/opt/go/bin/go test ./cmd/... ./internal/output

Summary by CodeRabbit

• Bug Fixes

  • Improved config removal safety to prevent orphaned secrets when saving fails.

• Tests

  • Added test coverage for config removal failure scenarios to ensure existing configurations are preserved.

[coderabbitai]

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 1bc9d729-15f6-4299-b129-bbcdee48578b

📥 Commits

Reviewing files that changed from the base of the PR and between d3d92e3 and 755439f.

📒 Files selected for processing (2)

• cmd/config/config_test.go
• cmd/config/remove.go

---

📝 Walkthrough

Walkthrough

The PR adds a test verifying that config removal gracefully preserves existing config and keychain secrets when the save operation fails. It reorders the removal flow to save an empty config before deleting keychain items, with error propagation if the save fails.

Changes

Cohort / File(s)	Summary
Test Infrastructure cmd/config/config_test.go	Added recordingConfigKeychain test double to record keychain removal calls and new test TestConfigRemoveRun_SaveFailurePreservesExistingConfigAndSecrets that verifies config and secrets remain intact when save fails.
Removal Logic cmd/config/remove.go	Reordered the removal sequence to save an empty MultiAppConfig before removing keychain secrets and auth tokens, with early error return if the initial save fails.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Possibly related issues

• bug: config remove clears keychain before saving config, leaving inconsistent state on failure #114: Directly addresses the issue by implementing the atomic save-before-delete pattern to ensure config and keychain consistency when removal fails.

Poem

🐰 Before the secrets flee, the config falls away,
A save, then sweep—atomicity saves the day!
No half-done removals, no scattered debris,
The rabbits applaud this protective decree! 🎉

🚥 Pre-merge checks | ✅ 2 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (2 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title clearly and concisely describes the main change: reordering the config removal flow to save the empty config before clearing keychain entries.
Description check	✅ Passed	The description includes Summary, Changes, and Test Plan sections matching the template, though Test Plan is minimal and Related Issues is missing.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests

---

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[greptile-apps]

Greptile Summary

This PR fixes a correctness bug in configRemoveRun where keychain entries could be permanently deleted even when the empty-config save failed, leaving the on-disk config referencing secrets that no longer exist. The fix reorders the operations — write empty config first, then clean up keychain entries only on success — and adds a targeted regression test that validates the failure path preserves both the config file and keychain references intact.

Confidence Score: 5/5

Safe to merge — the fix is correct and the single inline comment is a pre-existing non-blocking style note.

All findings are P2 or lower; the save-before-cleanup reordering is logically sound and the regression test correctly validates the exact failure scenario introduced.

No files require special attention.

Important Files Changed

Filename	Overview
cmd/config/remove.go	Correctly reorders save-before-cleanup to prevent config/keychain inconsistency on save failure; minor pre-existing style issue with unhandled error from RemoveStoredToken.
cmd/config/config_test.go	Adds a well-structured regression test that validates save failure leaves both the config file and all keychain references intact.

Sequence Diagram

sequenceDiagram
    participant CLI as config remove
    participant Disk as Config File
    participant KC as Keychain

    CLI->>Disk: LoadMultiAppConfig()
    Disk-->>CLI: original config (apps[])

    CLI->>Disk: SaveMultiAppConfig(empty)
    alt save succeeds
        Disk-->>CLI: ok
        loop for each app
            CLI->>KC: RemoveSecretStore(appSecret)
            CLI->>KC: RemoveStoredToken(appId, userId)
        end
        CLI-->>CLI: PrintSuccess
    else save fails
        Disk-->>CLI: error
        CLI-->>CLI: return error (keychain untouched)
    end

Loading

_{Reviews (1): Last reviewed commit: "fix(config): save empty config before cl..." | Re-trigger Greptile}

[greptile-apps]

auth.RemoveStoredToken return value silently discarded

The error from auth.RemoveStoredToken is silently dropped. While RemoveSecretStore is explicitly documented as best-effort, RemoveStoredToken returns an error and callers elsewhere in the codebase may expect failures to be observable. Consider explicitly discarding like the rest of the cleanup pattern:

Suggested change

	for _, user := range app.Users {
	auth.RemoveStoredToken(app.AppId, user.UserOpenId)
	}
	_ = auth.RemoveStoredToken(app.AppId, user.UserOpenId)