fix(mail): avoid real keychain access in mail scope tests

[haidaodashushu]

Summary

• Mail scope tests (TestConfirmSendMissingScopeReply/ReplyAll/Forward) were calling auth.SetStoredToken/RemoveStoredToken which accessed the real macOS keychain via go-keyring, causing persistent popup dialogs when the master key was missing
• Add keyring.MockInit() in mailShortcutTestFactory to swap the keyring backend to an in-memory implementation during tests, completely avoiding real keychain access
• Only one file changed: shortcuts/mail/mail_shortcut_test.go (added 1 import + 1 line)

Test plan

• TestConfirmSendMissingScopeReply, TestConfirmSendMissingScopeReplyAll, TestConfirmSendMissingScopeForward pass without keychain popups
• All 131 mail package tests pass
• No changes to public/shared code (factory.go, runner.go, helpers.go are untouched)

🤖 Generated with Claude Code

[coderabbitai]

📝 Walkthrough

Walkthrough

A test setup modification that initializes an in-memory keyring mock in the mail shortcut test factory, replacing reliance on system keyring during test execution.

Changes

Cohort / File(s)	Summary
Test Setup Enhancement shortcuts/mail/mail_shortcut_test.go	Added keyring.MockInit() call in mailShortcutTestFactory() to initialize an in-memory keyring mock, with import of github.com/zalando/go-keyring. Token setup and cleanup flows remain unchanged.

Estimated code review effort

🎯 1 (Trivial) | ⏱️ ~3 minutes

Suggested labels

size/M

Suggested reviewers

• infeng

Poem

🐰 A keyring mock hops into place,
No system calls in test space,
In-memory whispers, clean and bright,
The tests now run purely—just right! 🔐

🚥 Pre-merge checks | ✅ 2 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 55.56% 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: avoiding real keychain access in mail scope tests, which aligns perfectly with the changeset's purpose.
Description check	✅ Passed	The PR description covers all required template sections with clear summary, specific changes, comprehensive test plan with checkboxes, and acknowledges no related issues.

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

✨ Finishing Touches

📝 Generate docstrings

• Create stacked PR
• Commit on current branch

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch fix/mail-test-avoid-real-keychain

---

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

[greptile-apps]

Greptile Summary

This PR fixes macOS keychain popup dialogs that appeared when running TestConfirmSendMissingScopeReply, TestConfirmSendMissingScopeReplyAll, and TestConfirmSendMissingScopeForward tests. The actual change is a minimal 2-line addition: importing github.com/zalando/go-keyring in the test file and calling keyring.MockInit() inside mailShortcutTestFactory, which redirects all keyring operations to an in-memory store for the duration of the test process — the right level of fix for the problem.

• Adds keyring.MockInit() to mailShortcutTestFactory so auth.SetStoredToken/auth.RemoveStoredToken calls hit an in-memory backend instead of the real OS keychain
• The go-keyring package (v0.2.8) was already a dependency — no new external packages introduced
• The t.Cleanup teardown for RemoveStoredToken is preserved, keeping the in-memory store tidy between test runs
• Note: the PR description references a larger refactor (caching scope on RuntimeContext, Factory.StoredScopeOverride) that was not included in the final commit — the actual shipped change is simpler and more surgical

Confidence Score: 5/5

Safe to merge — change is confined to test infrastructure with no production code affected

The change is two lines in a test helper: one import and one keyring.MockInit() call. The go-keyring package is already a project dependency, MockInit() is a well-established test utility in that library, and the fix directly addresses the described problem without touching any production code paths.

No files require special attention

Important Files Changed

Filename	Overview
shortcuts/mail/mail_shortcut_test.go	Adds keyring.MockInit() and the corresponding import to redirect keychain operations to an in-memory mock; no logic changes, no issues found

Sequence Diagram

sequenceDiagram
    participant T as Test
    participant MF as mailShortcutTestFactory
    participant KR as go-keyring (MockInit)
    participant AU as auth.SetStoredToken
    participant KC as macOS Keychain

    Note over T,KC: Before this PR
    T->>MF: mailShortcutTestFactory(t)
    MF->>AU: SetStoredToken(token)
    AU->>KC: write to real keychain (⚠️ popup)

    Note over T,KC: After this PR
    T->>MF: mailShortcutTestFactory(t)
    MF->>KR: keyring.MockInit()
    KR-->>MF: backend = in-memory store
    MF->>AU: SetStoredToken(token)
    AU->>KR: write to in-memory store (✅ no popup)

Loading

_{Reviews (2): Last reviewed commit: "fix(mail): use in-memory keyring in mail..." | Re-trigger Greptile}

[github-actions]

🚀 PR Preview Install Guide

🧰 CLI update

npm i -g https://pkg.pr.new/larksuite/cli/@larksuite/cli@5e18e5a400474b4c4c793b6562218e21fb6d2e88

🧩 Skill update

npx skills add larksuite/cli#fix/mail-test-avoid-real-keychain -y -g

[coderabbitai]

Actionable comments posted: 1

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)

shortcuts/mail/mail_shortcut_test.go (1)

32-40: ⚠️ Potential issue | 🟡 Minor

Clear LARKSUITE_CLI_DEFAULT_AS in this helper.

ResolveAs checks that env var before cfg.DefaultAs, so a developer shell with LARKSUITE_CLI_DEFAULT_AS=bot will bypass this new user default and stop exercising the scope-cache path. Please neutralize it here to keep the tests hermetic.

Suggested tweak

 func mailShortcutTestFactory(t *testing.T) (*cmdutil.Factory, *bytes.Buffer, *bytes.Buffer, *httpmock.Registry) {
 	t.Helper()
 	t.Setenv("HOME", t.TempDir())
+	t.Setenv("LARKSUITE_CLI_DEFAULT_AS", "")
 
 	cfg := mailTestConfig()
 	scope := "mail:user_mailbox.messages:write mail:user_mailbox.messages:read mail:user_mailbox.message:modify mail:user_mailbox.message:readonly mail:user_mailbox.message.address:read mail:user_mailbox.message.subject:read mail:user_mailbox.message.body:read mail:user_mailbox:readonly"
 	f, stdout, stderr, reg := cmdutil.TestFactory(t, cfg)

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@shortcuts/mail/mail_shortcut_test.go` around lines 32 - 40, The helper
mailShortcutTestFactory must clear the LARKSUITE_CLI_DEFAULT_AS environment
variable so ResolveAs doesn't pick up a developer's shell value and bypass
cfg.DefaultAs; update mailShortcutTestFactory to call
t.Setenv("LARKSUITE_CLI_DEFAULT_AS", "") (or unset) before calling
cmdutil.TestFactory so tests exercise the scope-cache path that ResolveAs and
cfg.DefaultAs are intended to cover.

🤖 Prompt for all review comments with AI agents

Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@shortcuts/common/runner.go`:
- Around line 456-472: getStoredScope currently conflates “no stored token” and
“token present with empty Scope” by returning "" which breaks scope-validation
paths (checkShortcutScopes and mail validators); modify getStoredScope (or add a
companion like getStoredTokenPresence) to return both the stored scope and a
presence flag (e.g., (*string, bool) or (string, bool)) so callers
(checkShortcutScopes, RuntimeContext, mail-specific validators) can distinguish
a missing token from a token that has an empty Scope, keep honoring
Factory.StoredScopeOverride, and update all callers noted (including the mail
validators and the RuntimeContext usage) to bypass checks only when
presence==false rather than when scope=="".

---

Outside diff comments:
In `@shortcuts/mail/mail_shortcut_test.go`:
- Around line 32-40: The helper mailShortcutTestFactory must clear the
LARKSUITE_CLI_DEFAULT_AS environment variable so ResolveAs doesn't pick up a
developer's shell value and bypass cfg.DefaultAs; update mailShortcutTestFactory
to call t.Setenv("LARKSUITE_CLI_DEFAULT_AS", "") (or unset) before calling
cmdutil.TestFactory so tests exercise the scope-cache path that ResolveAs and
cfg.DefaultAs are intended to cover.

🪄 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: 54dc3f35-ec17-4d60-9e41-6fcce13dc74c

📥 Commits

Reviewing files that changed from the base of the PR and between 79f43dc and 7ebd091.

📒 Files selected for processing (4)

• internal/cmdutil/factory.go
• shortcuts/common/runner.go
• shortcuts/mail/helpers.go
• shortcuts/mail/mail_shortcut_test.go