feat: restore client-secret app credentials - #2322
Conversation
Codecov Report❌ Patch coverage is ❌ Your patch check has failed because the patch coverage (56.60%) is below the target coverage (60.00%). You can increase the patch coverage or adjust the target coverage. Additional details and impacted files@@ Coverage Diff @@
## main #2322 +/- ##
==========================================
+ Coverage 76.44% 76.46% +0.01%
==========================================
Files 1025 1026 +1
Lines 113704 113766 +62
==========================================
+ Hits 86921 86990 +69
+ Misses 20111 20091 -20
- Partials 6672 6685 +13 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
📝 WalkthroughWalkthroughThe CLI adds ChangesConfiguration app restoration
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant User
participant ConfigInit
participant RestoreFlow
participant AppRegistration
participant Keychain
participant ConfigStore
participant AppProbe
User->>ConfigInit: run config init --restore
ConfigInit->>RestoreFlow: selected profile and app ID
RestoreFlow->>AppRegistration: request credentials for app ID
AppRegistration-->>RestoreFlow: restored credentials and brand
RestoreFlow->>Keychain: store app secret
RestoreFlow->>ConfigStore: save profile configuration
RestoreFlow->>AppProbe: probe restored app
AppProbe-->>User: restore result
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 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 |
🚀 PR Preview Install Guide🧰 CLI updatenpm i -g https://pkg.pr.new/larksuite/cli/@larksuite/cli@e919dc02f730cf6df699b183246cba0bd7d89a20🧩 Skill updatenpx skills add larksuite/cli#feat/app-credential-restore -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
🧹 Nitpick comments (3)
cmd/config/init_restore_test.go (1)
836-848: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd
defer cancel()for the cancellable context.Line 836 creates
ctx, canceland only callscancelinside the transport closure. If the transport is never reached, the context leaks andgo vet'slostcancelcheck can report the unusedcancelpath. Other tests in this file usedefer cancel().🛠️ Proposed fix
ctx, cancel := context.WithCancel(context.Background()) + defer cancel() var beginAppID string🤖 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/init_restore_test.go` around lines 836 - 848, Add defer cancel() immediately after creating the cancellable context in the test setup, while retaining the existing cancel() call in the restoreRoundTripFunc closure. Update the context initialization around ctx and cancel so every execution path releases the context.internal/auth/app_registration.go (1)
163-172: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueReuse the validated
userCodein the returned struct.Line 164 validates
userCode. Line 172 callsgetStr(data, "user_code")again for the returnedUserCodefield. The two values are equal today, so behavior is correct. Reusing the local variable keeps the validated value as the single source for the response and prevents a future edit from returning an unvalidated string.♻️ Proposed refactor
return &AppRegistrationResponse{ DeviceCode: deviceCode, - UserCode: getStr(data, "user_code"), + UserCode: userCode, VerificationUri: verificationUri,🤖 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 `@internal/auth/app_registration.go` around lines 163 - 172, Reuse the validated userCode variable when assigning the UserCode field in the AppRegistrationResponse returned by the registration flow, instead of calling getStr again; leave the existing validation and other response fields unchanged.cmd/config/init.go (1)
363-371: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a test for the relocated
--namevalidation.
validateRestoreFlagsruns first, so--restorewith--namefails before this block. That ordering is correct. Codecov reports lines 367-369 as uncovered, andTestConfigInit_NameStillAcceptedForOrdinaryInituses arunFoverride, so it never reachesconfigInitRun. Add one case that callsconfigInitRunwith an invalidProfileNameand assertserrs.SubtypeInvalidArgumentwithParam == "--name".🤖 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/init.go` around lines 363 - 371, Add a test that invokes configInitRun directly with an invalid ProfileName during ordinary initialization, without the runF override. Assert the returned validation error has errs.SubtypeInvalidArgument and Param equal to "--name", covering the core.ValidateProfileName failure path while preserving the existing restore-flag ordering.Source: Linters/SAST tools
🤖 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/init_restore_test.go`:
- Around line 1043-1052: Isolate configuration for both Factory-backed tests in
cmd/config/init_restore_test.go: at lines 1043-1052, create a temp config
directory and set LARKSUITE_CLI_CONFIG_DIR for every subtest, while writing
config.json only for malformedConfig cases; at lines 783-790, set
LARKSUITE_CLI_CONFIG_DIR to t.TempDir() before cmdutil.TestFactory(t, nil).
---
Nitpick comments:
In `@cmd/config/init_restore_test.go`:
- Around line 836-848: Add defer cancel() immediately after creating the
cancellable context in the test setup, while retaining the existing cancel()
call in the restoreRoundTripFunc closure. Update the context initialization
around ctx and cancel so every execution path releases the context.
In `@cmd/config/init.go`:
- Around line 363-371: Add a test that invokes configInitRun directly with an
invalid ProfileName during ordinary initialization, without the runF override.
Assert the returned validation error has errs.SubtypeInvalidArgument and Param
equal to "--name", covering the core.ValidateProfileName failure path while
preserving the existing restore-flag ordering.
In `@internal/auth/app_registration.go`:
- Around line 163-172: Reuse the validated userCode variable when assigning the
UserCode field in the AppRegistrationResponse returned by the registration flow,
instead of calling getStr again; leave the existing validation and other
response fields unchanged.
🪄 Autofix
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 Plus
Run ID: 65ee6cd5-c298-4e0c-bc24-8085eff3291b
📒 Files selected for processing (10)
cmd/config/config_test.gocmd/config/init.gocmd/config/init_interactive.gocmd/config/init_interactive_test.gocmd/config/init_restore.gocmd/config/init_restore_test.gointernal/auth/app_registration.gointernal/auth/app_registration_test.gointernal/recovery/context.gointernal/recovery/hint_test.go
58b8a76 to
e919dc0
Compare
github-actions
Bot
added
size/L
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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/init_restore.go`:
- Around line 65-68: Update the restore success output in
cmd/config/init_restore.go lines 65-68 to use msg.ConfigSaved or a
restore-specific message instead of msg.AppCreated. In
cmd/config/init_restore_test.go lines 102-108, capture stderr from
cmdutil.TestFactory and assert that it contains the restore success message and
does not report application creation.
In `@internal/auth/app_registration_test.go`:
- Around line 87-89: The test currently checks only the value of app_id, so it
cannot distinguish an omitted field from an explicitly empty one. Update the
relevant app-registration request assertions to check map-key presence directly:
require app_id to exist for restore requests and require the key to be absent
for normal requests, while preserving the existing value assertion where
applicable.
🪄 Autofix
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 Plus
Run ID: d83bbf1e-333f-4466-80d7-33e5615ebeae
📒 Files selected for processing (6)
cmd/config/init.gocmd/config/init_interactive.gocmd/config/init_restore.gocmd/config/init_restore_test.gointernal/auth/app_registration.gointernal/auth/app_registration_test.go
| output.PrintSuccess(f.IOStreams.ErrOut, fmt.Sprintf(msg.AppCreated, app.AppId)) | ||
| output.PrintJson(f.IOStreams.Out, map[string]interface{}{ | ||
| "appId": app.AppId, "appSecret": "****", "brand": app.Brand, | ||
| }) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Report restore success instead of app creation.
The restore path updates an existing application. msg.AppCreated reports the wrong operation. Use msg.ConfigSaved or add a restore-specific message. Capture stderr in the success test and assert that it does not report application creation.
cmd/config/init_restore.go#L65-L68: replacemsg.AppCreatedwith a restore-appropriate success message.cmd/config/init_restore_test.go#L102-L108: capture stderr fromcmdutil.TestFactoryand assert the restore success message.
As per coding guidelines: “Preserve established CLI behavior, tests, lint, CI, output contracts, and public APIs unless a breaking change is explicitly requested.”
📍 Affects 2 files
cmd/config/init_restore.go#L65-L68(this comment)cmd/config/init_restore_test.go#L102-L108
🤖 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/init_restore.go` around lines 65 - 68, Update the restore success
output in cmd/config/init_restore.go lines 65-68 to use msg.ConfigSaved or a
restore-specific message instead of msg.AppCreated. In
cmd/config/init_restore_test.go lines 102-108, capture stderr from
cmdutil.TestFactory and assert that it contains the restore success message and
does not report application creation.
Source: Coding guidelines
| if got := r.Form.Get("app_id"); got != c.appID { | ||
| t.Errorf("begin app_id = %q, want %q", got, c.appID) | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Assert that normal requests omit app_id.
r.Form.Get("app_id") returns "" for both an absent field and app_id=. The test passes if RequestAppRegistration always sends an empty app_id field. Assert map-key presence for restore requests and absence for normal requests.
Proposed test change
- if got := r.Form.Get("app_id"); got != c.appID {
+ _, present := r.Form["app_id"]
+ if want := c.appID != ""; present != want {
+ t.Errorf("begin app_id present = %t, want %t", present, want)
+ }
+ if got := r.Form.Get("app_id"); got != c.appID {
t.Errorf("begin app_id = %q, want %q", got, c.appID)
}As per coding guidelines: “Every behavior change requires a nearby regression test that fails when the implementation is reverted; tests should assert fields, requests, typed errors, or side effects directly.”
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if got := r.Form.Get("app_id"); got != c.appID { | |
| t.Errorf("begin app_id = %q, want %q", got, c.appID) | |
| } | |
| _, present := r.Form["app_id"] | |
| if want := c.appID != ""; present != want { | |
| t.Errorf("begin app_id present = %t, want %t", present, want) | |
| } | |
| if got := r.Form.Get("app_id"); got != c.appID { | |
| t.Errorf("begin app_id = %q, want %q", got, c.appID) | |
| } |
🤖 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 `@internal/auth/app_registration_test.go` around lines 87 - 89, The test
currently checks only the value of app_id, so it cannot distinguish an omitted
field from an explicitly empty one. Update the relevant app-registration request
assertions to check map-key presence directly: require app_id to exist for
restore requests and require the key to be absent for normal requests, while
preserving the existing value assertion where applicable.
Source: Coding guidelines
1 participant
Summary
When the locally stored client secret can no longer be read, users need a way to re-register the same application instead of creating a new one. This PR adds
config init --restore, which uses the App ID and brand from the selected existing profile and writes back the newly issued client secret.Changes
--restoremode and local flag conflict validation incmd/config/init.goandcmd/config/init_restore.go--profilecontext, send its App ID in the app-registration begin form, and reuse the existing poll flow unchangedTest Plan
make unit-testpassedgo vet ./...,gofmt -l .,go mod tidy,golangci-lint, and repository lint checks)Related Issues
N/A
Summary by CodeRabbit
New Features
--restoreoption to restore an existing app configuration.Bug Fixes