feat: add config set-app-secret to rotate a profile's app secret#1605
Open
luozhixiong01 wants to merge 6 commits into
Open
feat: add config set-app-secret to rotate a profile's app secret#1605luozhixiong01 wants to merge 6 commits into
luozhixiong01 wants to merge 6 commits into
Conversation
github-actions
Bot
added
the
size/L
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds ChangesApp secret rotation
Sequence Diagram(s)sequenceDiagram
participant User
participant setAppSecretRun
participant credential.FetchTAT
participant core.ForStorage
participant core.SaveMultiAppConfig
participant stdout
User->>setAppSecretRun: run config set-app-secret
alt --yes not set
setAppSecretRun-->>User: confirmation-required error with target
else --yes set
setAppSecretRun->>credential.FetchTAT: verify secret with 3-second timeout
alt verification succeeds
credential.FetchTAT-->>setAppSecretRun: verified
setAppSecretRun->>core.ForStorage: store verified secret
alt config migration is needed
setAppSecretRun->>core.SaveMultiAppConfig: persist updated app secret ref
end
setAppSecretRun->>stdout: write JSON or terminal success output
else verification fails
credential.FetchTAT-->>setAppSecretRun: typed config or network error
end
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 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 |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1605 +/- ##
==========================================
- Coverage 74.64% 74.60% -0.05%
==========================================
Files 806 807 +1
Lines 81386 81651 +265
==========================================
+ Hits 60752 60916 +164
- Misses 16101 16188 +87
- Partials 4533 4547 +14 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
🚀 PR Preview Install Guide🧰 CLI updatenpm i -g https://pkg.pr.new/larksuite/cli/@larksuite/cli@b0269d67edb37a3af4719a8a4ad80e9a02724bd5🧩 Skill updatenpx skills add larksuite/cli#feat/app-secret-rotate -y -g |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@cmd/config/set_app_secret.go`:
- Around line 157-175: The verification block in set_app_secret should preserve
the original error cause on every failure path so callers can unwrap and match
it later. Update the error returns in the HttpClient initialization failure and
both credential.FetchTAT branches to attach the underlying err via the typed
error’s cause-setting method, using the existing symbols f.HttpClient,
credential.FetchTAT, and the errs.NewNetworkError / errs.NewConfigError
constructors. Also consider reclassifying the HttpClient initialization failure
as an internal error instead of a network transport error, while keeping the
same target and hint behavior.
🪄 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: 865b4459-529f-4ecf-ba2b-a1b257e63601
📒 Files selected for processing (9)
cmd/config/config.gocmd/config/set_app_secret.gocmd/config/set_app_secret_test.goerrs/problem.goerrs/problem_test.goerrs/types.gointernal/errclass/classify.gointernal/errclass/classify_internal_test.gointernal/errclass/classify_test.go
luozhixiong01
force-pushed
the
feat/app-secret-rotate
branch
from
65ed8ae to
949f643
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
cmd/config/set_app_secret_test.go (2)
392-399: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winPin the new structured
targetmetadata on verify failures.These checks only prove the app identity indirectly via hint substrings. Since this PR adds
targetmetadata so failure envelopes can identify the affected profile/app, please assertcfgErr.Target/netErr.Targetdirectly as well; otherwise the envelope contract can regress while the hint text still passes.Also applies to: 438-443
🤖 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 `@cmd/config/set_app_secret_test.go` around lines 392 - 399, The verification tests for set_app_secret are only checking hint substrings, but they should also lock in the new structured target metadata on failure envelopes. Update the relevant assertions around cfgErr and netErr in set_app_secret_test.go to directly verify the Target field (for the app/profile identity) in addition to the existing Hint checks, using the existing cfgErr and netErr references so the envelope contract cannot regress while the hint text still passes.
762-767: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAssert stderr stays empty on successful runs.
The success-path tests only inspect stdout. For commands under
cmd/**, stdout is the program-output channel and stderr must stay reserved for warnings/progress, so an accidental mixed-channel success would currently slip through. Capturef.IOStreams.ErrOutand assert it remains empty in both JSON and pretty-mode cases. As per coding guidelines, "stdout is for program output (JSON envelopes), stderr is for progress, warnings, and hints; never mix them."Also applies to: 776-928
🤖 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 `@cmd/config/set_app_secret_test.go` around lines 762 - 767, The success-path tests are only checking stdout, so mixed-channel output on cmd/** commands could go unnoticed. Update the relevant test setup around the shared helper and success cases to capture f.IOStreams.ErrOut and assert it stays empty for both JSON and pretty-mode runs. Use the existing helper that builds f, kc, and outBuf, and add stderr assertions alongside the current stdout checks.Source: Coding guidelines
🤖 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 `@cmd/config/set_app_secret_test.go`:
- Around line 122-141: The error-path tests in set_app_secret_test should assert
typed problem metadata instead of only checking concrete type, exit code, or
strings. Update these cases to use errs.ProblemOf(err) to verify
category/subtype/cause preservation, and keep using errors.As only when you need
the Param from *errs.ValidationError because errs.ProblemOf does not expose it.
Apply the same metadata assertions consistently in the other related test blocks
that currently only check type, exit code, or message.
- Around line 450-476: The transport failure test in setAppSecretRun only checks
the wrapped NetworkError and exit code, so it can miss loss of the underlying
cause. Introduce a sentinel transport error in
TestSetAppSecret_VerifyBeforeWrite_NetworkTransport, use it in the fakeRT
handler, and assert with errors.Is after errors.As that the returned error
preserves that cause. Keep the existing setAppSecretFactory,
SetAppSecretOptions, and output.ExitCodeOf checks, and add typed metadata
assertions via errs.ProblemOf for the NetworkError shape if applicable.
---
Nitpick comments:
In `@cmd/config/set_app_secret_test.go`:
- Around line 392-399: The verification tests for set_app_secret are only
checking hint substrings, but they should also lock in the new structured target
metadata on failure envelopes. Update the relevant assertions around cfgErr and
netErr in set_app_secret_test.go to directly verify the Target field (for the
app/profile identity) in addition to the existing Hint checks, using the
existing cfgErr and netErr references so the envelope contract cannot regress
while the hint text still passes.
- Around line 762-767: The success-path tests are only checking stdout, so
mixed-channel output on cmd/** commands could go unnoticed. Update the relevant
test setup around the shared helper and success cases to capture
f.IOStreams.ErrOut and assert it stays empty for both JSON and pretty-mode runs.
Use the existing helper that builds f, kc, and outBuf, and add stderr assertions
alongside the current stdout checks.
🪄 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: 567d1ba6-39b0-4bc0-a9ae-c7bcaefaec0f
📒 Files selected for processing (9)
cmd/config/config.gocmd/config/set_app_secret.gocmd/config/set_app_secret_test.goerrs/problem.goerrs/problem_test.goerrs/types.gointernal/errclass/classify.gointernal/errclass/classify_internal_test.gointernal/errclass/classify_test.go
✅ Files skipped from review due to trivial changes (2)
- internal/errclass/classify.go
- internal/errclass/classify_internal_test.go
🚧 Files skipped from review as they are similar to previous changes (6)
- cmd/config/config.go
- errs/problem.go
- errs/problem_test.go
- cmd/config/set_app_secret.go
- errs/types.go
- internal/errclass/classify_test.go
Add a config set-app-secret command so users and agents can rotate a profile's app secret after resetting it on the open platform, instead of hacking the OS keychain (which fails because the stored value lives in a closed secure store). The command reads the new value from stdin, previews the target and exits 10 without --yes, verifies the value via FetchTAT before writing, stores it through core.ForStorage (the same primitive as config init and config bind), and leaves other profiles untouched. It also redirects the invalid_client hint to the new command and adds an optional target field to the error envelope so the affected bot is named without exposing the secret.
…set-app-secret tests
luozhixiong01
force-pushed
the
feat/app-secret-rotate
branch
from
e3ced8b to
dd0e1e9
Compare
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.
Summary
After resetting a bot's app secret on the Lark/Feishu open platform, the local lark-cli keeps using the old secret and has no command to update just that profile's secret — users (and AI agents acting for them) were forced to hack the OS keychain, which fails because lark-cli's secure storage rejects externally created entries. This PR adds
config set-app-secretto safely rotate one profile's app secret, and redirects the invalid_client auth-failure hint away from the destructiveconfig initto this new command.Changes
config set-app-secretincmd/config/set_app_secret.go(registered incmd/config/config.go): reads the new secret from stdin (--app-secret-stdin); without--yesit previews the target (profile + app_id) and exits 10 without reading stdin or writing; verifies the secret viacredential.FetchTATbefore writing; stores it viacore.ForStorage(same primitive asconfig init/config bind); migrates only the target profile's reference for plaintext/file sources; never touches other profiles, the active selection, or stored loginsinvalid_clientreturns exit 3 (replace the secret), transient/network failure returns aNetworkErrorexit 4 (retry); neither writes anythingtarget{profile, app_id, is_active}field to the error envelope inerrs/problem.goanderrs/types.go(WithTargetbuilders), so confirmation and failure envelopes name which bot will change (the field never carries the secret)ConfigHint(SubtypeInvalidClient)ininternal/errclass/classify.gofromconfig inittoconfig set-app-secret(preview-first, no--yes)Test Plan
make unit-testpassedlark-cli config set-app-secret --app-secret-stdin(preview → exit 10 + target, nothing written);printf bogus | lark-cli config set-app-secret --app-secret-stdin --yes(invalid_client → exit 3, config.json unchanged)Related Issues
N/A
Summary by CodeRabbit
config set-app-secretto rotate a bot profile’s app secret via--app-secret-stdinwith a preview/confirmation flow (--yes).config set-app-secret) instead of reinitializing.targetidentity context (profile/app and active state) when applicable.