Skip to content

fix: improve error hints for sandbox and initialization issues#384

Merged
JackZhao10086 merged 4 commits intomainfrom
feat/tweak_keychain_sandbox_prompt
Apr 10, 2026
Merged

fix: improve error hints for sandbox and initialization issues#384
JackZhao10086 merged 4 commits intomainfrom
feat/tweak_keychain_sandbox_prompt

Conversation

@JackZhao10086
Copy link
Copy Markdown
Collaborator

@JackZhao10086 JackZhao10086 commented Apr 10, 2026

Summary

Improve keychain-related error hints to make common failure scenarios easier to diagnose.
This update clarifies what users should do when keychain access fails in sandbox or CI environments, and when the keychain has not been initialized properly.

Changes

  • Improve the generic keychain access error hint with clearer guidance for sandbox and CI environments
  • Add a suggestion to retry outside the sandbox when keychain access may be restricted
  • Refine the uninitialized keychain hint to distinguish permission-related issues from cases that require re-running lark-cli config init

Test Plan

  • Unit tests pass
  • Manually verify related lark commands work locally

Related Issues

Summary by CodeRabbit

  • Bug Fixes
    • Improved error messages for keychain access failures: troubleshooting hints now explicitly suggest trying the operation outside the sandbox. When initialization is detected as the cause, messages also include a clear “Otherwise” instruction to run the CLI configuration/init command to reconfigure the tool.

Clarify the error message for uninitialized keychain by combining both possible scenarios (sandbox/CI environment and normal usage) into a single hint to avoid confusion.
Add suggestion to try running outside sandbox when keychain access fails. Also update hint for uninitialized keychain case to include same suggestion.
@github-actions github-actions bot added the size/M Single-domain feat or fix with limited business impact label Apr 10, 2026
@coderabbitai
Copy link
Copy Markdown

coderabbitai bot commented Apr 10, 2026

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: f9b71404-ebda-4981-bd61-64022771c482

📥 Commits

Reviewing files that changed from the base of the PR and between 7932635 and f4ad035.

📒 Files selected for processing (1)
  • internal/keychain/keychain.go
🚧 Files skipped from review as they are similar to previous changes (1)
  • internal/keychain/keychain.go

📝 Walkthrough

Walkthrough

Adjusted the troubleshooting hint text in the keychain error wrapper (wrapError) to add guidance to try running the operation "outside the sandbox" and, when the error is errNotInitialized, to also instruct users to reconfigure the CLI via lark-cli config init. No control flow or error-handling logic changed.

Changes

Cohort / File(s) Summary
Error Hint Text Updates
internal/keychain/keychain.go
Expanded troubleshooting hint strings produced by wrapError to include a sandbox-outside instruction; for errNotInitialized, also added an explicit "Otherwise" clause instructing lark-cli config init. Text-only change; no logic altered.

Estimated code review effort

🎯 1 (Trivial) | ⏱️ ~3 minutes

Possibly related PRs

Suggested reviewers

  • liangshuo-1

Poem

🐰 A tiny tweak, a helpful cue,
Try outside the sandbox — that will do!
If keys are absent, don’t lament,
Run lark-cli config init — that’s the intent. ✨

🚥 Pre-merge checks | ✅ 3
✅ Passed checks (3 passed)
Check name Status Explanation
Title check ✅ Passed The title accurately summarizes the main change: improving error hints for sandbox and initialization issues in keychain error handling.
Description check ✅ Passed The description includes all required template sections (Summary, Changes, Test Plan, Related Issues) with appropriate detail levels for the changeset.
Docstring Coverage ✅ Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.

✏️ 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 feat/tweak_keychain_sandbox_prompt

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

@greptile-apps
Copy link
Copy Markdown

greptile-apps bot commented Apr 10, 2026

Greptile Summary

This PR updates two error hint strings in internal/keychain/keychain.go to provide more actionable guidance when keychain access fails — specifically adding sandbox/CI environment context and distinguishing permission failures from uninitialized-keychain failures. The logic is unchanged; only the user-facing hint text is modified.

Confidence Score: 5/5

Safe to merge — changes are limited to user-facing hint strings with no logic impact.

No new logic, security, or correctness issues introduced. The only open items (comma splice and missing backticks around lark-cli config init) were already raised in prior review threads and are cosmetic P2 issues that do not affect runtime behavior.

No files require special attention.

Important Files Changed

Filename Overview
internal/keychain/keychain.go Two hint strings updated with additional sandbox/CI guidance; no logic changes. Outstanding grammar issues (comma splice, missing backticks on lark-cli config init) were already flagged in previous review threads.

Flowchart

%%{init: {'theme': 'neutral'}}%%
flowchart TD
    A[keychain op: Get / Set / Remove] --> B{err == nil or ErrNotFound?}
    B -- yes --> C[return err as-is]
    B -- no --> D{errors.Is errNotInitialized?}
    D -- yes --> E["hint: master key missing\n+ sandbox/CI advice\n+ suggest lark-cli config init"]
    D -- no --> F["hint: keychain locked/inaccessible\n+ sandbox/CI advice"]
    E --> G[output.ErrWithHint → ExitAPI]
    F --> G
Loading

Reviews (3): Last reviewed commit: "docs(keychain): fix typo in error messag..." | Re-trigger Greptile

Copy link
Copy Markdown

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 1

🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@internal/keychain/keychain.go`:
- Around line 39-42: The two hint string assignments (the default hint and the
one inside the errors.Is(err, errNotInitialized) branch) contain comma splices
and inconsistent capitalization; update those literal strings (the variable hint
and the branch where errors.Is(err, errNotInitialized) sets hint) to use
grammatically correct sentences, remove comma splices, fix capitalization (e.g.,
"you" not "You" after commas), and standardize punctuation and phrasing so each
hint reads as clear, separate sentences (mention checking sandbox/CI permissions
and, in the not-initialized case, instruct to reconfigure with "lark-cli config
init" as a separate sentence).
🪄 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: 3fab4d05-f800-45ba-89b3-2d18700e8ffd

📥 Commits

Reviewing files that changed from the base of the PR and between 6ec19cb and 7932635.

📒 Files selected for processing (1)
  • internal/keychain/keychain.go

@github-actions
Copy link
Copy Markdown

github-actions bot commented Apr 10, 2026

🚀 PR Preview Install Guide

🧰 CLI update

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

🧩 Skill update

npx skills add larksuite/cli#feat/tweak_keychain_sandbox_prompt -y -g

@JackZhao10086 JackZhao10086 merged commit d2a8340 into main Apr 10, 2026
16 checks passed
@JackZhao10086 JackZhao10086 deleted the feat/tweak_keychain_sandbox_prompt branch April 10, 2026 06:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

size/M Single-domain feat or fix with limited business impact

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants