feat(multi-user): port park-cli multi-user surface (Users[]/CurrentUs…#1257
Open
kyeliu99 wants to merge 1 commit into
Open
feat(multi-user): port park-cli multi-user surface (Users[]/CurrentUs…#1257kyeliu99 wants to merge 1 commit into
kyeliu99 wants to merge 1 commit into
Conversation
…er, holder-gate, R2-aware config, sidecar sweep) into lark-cli
|
liurunzhou seems not to be a GitHub user. You need a GitHub account to be able to sign the CLA. If you have already a GitHub account, please add the email address used for this commit to your account. You have signed the CLA already but the status is still pending? Let us recheck it. |
📝 WalkthroughWalkthroughIntroduces multi-user auth: core schema/versioning, new local auth storage (KV, tokens, locks), user index/profile, migration, holder mismatch gate, users subcommands (list/use/logout), login/logout refactors with JSON warnings, config/profile commands serialized with locks, credential/bootstrap user override, and consistent R2 error/hint routing. ChangesMulti-user auth + storage + migration + CLI integration
Sequence Diagram(s)sequenceDiagram
autonumber
participant User
participant CLI
participant CoreConfig
participant LocalAuth
participant OAuth
User->>CLI: auth login --user/--json
CLI->>CoreConfig: Resolve profile (login mode)
CLI->>OAuth: Device flow (authorize + token)
OAuth-->>CLI: Access/refresh token
CLI->>OAuth: GetUserInfo (open_id/union_id/name)
CLI->>CoreConfig: Resolve holder (flag/env/current)
CLI->>CLI: Verify holder (warn or abort)
CLI->>LocalAuth: Store tokens (rollback on failure)
CLI->>LocalAuth: Upsert user profile + user index (under lock)
CLI-->>User: Success JSON (+holder_mismatch_warning when soft)
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
✨ Finishing Touches🧪 Generate unit tests (beta)
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 20
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
internal/core/config.go (1)
188-214:⚠️ Potential issue | 🟠 Major | ⚡ Quick winRestore the
"default"profile alias in app lookup helpers.This drops the existing
FindApp("default")contract, so selector paths that still rely on"default"resolving to the active profile will now miss or treat it as a literal name.FindAppIndexshould stay in sync too.Suggested fix
func (m *MultiAppConfig) FindApp(name string) *AppConfig { + if name == "default" && m.CurrentApp != "" && m.CurrentApp != "default" { + if app := m.FindApp(m.CurrentApp); app != nil { + return app + } + } for i := range m.Apps { if m.Apps[i].Name != "" && m.Apps[i].Name == name { return &m.Apps[i] } } @@ func (m *MultiAppConfig) FindAppIndex(name string) int { + if name == "default" && m.CurrentApp != "" && m.CurrentApp != "default" { + return m.FindAppIndex(m.CurrentApp) + } for i := range m.Apps { if m.Apps[i].Name != "" && m.Apps[i].Name == name { return i } }Based on learnings, in larksuite/cli
internal/core/config.go,MultiAppConfig.FindApptreats the string"default"as an alias for the currently active app.🤖 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/core/config.go` around lines 188 - 214, FindApp and FindAppIndex dropped support for the special alias "default"; restore it by detecting if the incoming name == "default" at the top of both MultiAppConfig.FindApp and MultiAppConfig.FindAppIndex and replacing name with the currently active app identifier (use the MultiAppConfig field that holds the active app, e.g. m.Active or m.ActiveApp) before running the existing loops over m.Apps; ensure both functions use the same replacement logic so they remain in sync.cmd/auth/login.go (1)
433-438:⚠️ Potential issue | 🟠 Major | ⚡ Quick winMirror the immediate-path JSON failure contract in
--device-code.When
opts.JSONis true, this branch currently returnserrs.NewAuthenticationError(...), so the root handler emits a typed error envelope on stderr. The immediate device-flow path already writes anauthorization_failedevent to stdout and returnsoutput.ErrBare(output.ExitAuth). That asymmetry breaks the documented NDJSON contract for the headless--no-wait→--device-codeflow and corrupts2>&1pipelines that expect JSON only on stdout.Suggested fix
if !result.OK { if shouldRemoveLoginRequestedScope(result) { cleanupRequestedScope() } + if opts.JSON { + enc := json.NewEncoder(f.IOStreams.Out) + enc.SetEscapeHTML(false) + if err := enc.Encode(map[string]interface{}{ + "event": "authorization_failed", + "error": result.Message, + }); err != nil { + return errs.NewInternalError(errs.SubtypeSDKError, "failed to write JSON output: %v", err).WithCause(err) + } + return output.ErrBare(output.ExitAuth) + } return errs.NewAuthenticationError(errs.SubtypeUnknown, "authorization failed: %s", result.Message) }As per coding guidelines,
**/*.go: stdout is data (JSON envelopes), stderr is everything else (progress, warnings, hints). Never mix them as it corrupts pipe chains.🤖 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/auth/login.go` around lines 433 - 438, When opts.JSON is true and the authorization branch sees !result.OK, don't return errs.NewAuthenticationError (which sends a typed error on stderr); instead mirror the device-code immediate-path: if shouldRemoveLoginRequestedScope(result) call cleanupRequestedScope(), then emit the "authorization_failed" NDJSON envelope to stdout (same envelope shape used by the device-code path) and return output.ErrBare(output.ExitAuth). For non-JSON paths keep the existing errs.NewAuthenticationError behavior. Locate this logic around the !result.OK check in login.go and reuse the same event-formatting and output.ErrBare return used by the device-code flow.
🧹 Nitpick comments (4)
internal/auth/purge_test.go (1)
175-192: ⚡ Quick winExercise a real failing leg here instead of asserting on a literal.
This test never makes
PurgeUserArtifactsbuild an aggregated error; it only proves that a hand-written string contains"; ". If the implementation regresses to returning the first error only, this still stays green. A tiny fakeRoot/KV that forcesDeleteUserProfileFororDeleteUserto fail would lock the actual contract instead of a placeholder 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 `@internal/auth/purge_test.go` around lines 175 - 192, The test currently only asserts a hand-written string and doesn't exercise PurgeUserArtifacts's real error-aggregation path; replace the placeholder check by constructing a failing Root/KV (or small fake) that makes DeleteUserProfileFor and DeleteUser return distinct errors, call PurgeUserArtifacts(root, "cli_app", "ou_alice") against that fake root (after keyring.MockInit()), and assert the returned error is non-nil and its Error() contains both component messages joined with "; " to lock the aggregation behavior of PurgeUserArtifacts and the DeleteUserProfileFor/DeleteUser legs.cmd/auth/users_use_profile_fallback_test.go (1)
59-64: ⚡ Quick winUse
cmdutil.TestFactoryfor these command-path tests.These cases call
authUsersUseRun, so the direct&cmdutil.Factory{...}setup is bypassing the standard unit-test factory wiring. Switching tocmdutil.TestFactory(t, config)will keep these tests aligned with the rest of the suite asFactoryevolves.As per coding guidelines,
Use cmdutil.TestFactory(t, config) for test factories in unit tests.Also applies to: 110-113
🤖 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/auth/users_use_profile_fallback_test.go` around lines 59 - 64, Replace the manual test factory construction that creates a &cmdutil.Factory (setting IOStreams and Invocation.Profile) with the standard helper cmdutil.TestFactory(t, config) so the authUsersUseRun tests use the same test wiring as the suite; locate the test setup where streams := &cmdutil.IOStreams and f := &cmdutil.Factory{IOStreams: streams, Invocation: cmdutil.InvocationContext{Profile: "ghost"}} and instantiate f via cmdutil.TestFactory(t, /* appropriate config map or nil */) and then set the profile on the returned Factory (or pass it via the config) before calling authUsersUseRun to ensure consistent factory behavior across tests.cmd/auth/users_test.go (1)
24-29: ⚡ Quick winUse the shared fake-home helper in this test fixture.
This helper only overrides
HOME, so Windows runs can still resolveUSERPROFILEand leak auth/storage state outside the temp dir. Please switch this to the repo’ssetFakeOSHome(t, dir)helper.Based on learnings, use a shared
setFakeOSHome(t, dir)helper that sets both theHOMEandUSERPROFILEenvironment variables viat.Setenv.🤖 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/auth/users_test.go` around lines 24 - 29, In stubUsersConfig replace the direct t.Setenv("HOME", t.TempDir()) call with the repo helper that sets both HOME and USERPROFILE: create a tempDir := t.TempDir() (or reuse the existing temp dir used for LARKSUITE_CLI_CONFIG_DIR), then call setFakeOSHome(t, tempDir); this ensures both HOME and USERPROFILE are set for Windows CI while leaving the rest of stubUsersConfig (including keyring.MockInit and LARKSUITE_CLI_CONFIG_DIR) unchanged.cmd/auth/users_logout_profile_fallback_test.go (1)
47-52: ⚡ Quick winPrefer
cmdutil.TestFactoryover hand-rolled factories in these tests.These tests execute config/auth command paths, so
&cmdutil.Factory{...}can drift from the canonical wiring the rest of the suite gets for IO, keychain, and future factory defaults.cmdutil.TestFactory(t, config)is the safer fixture here, and you can still overrideInvocation.Profileor buffers afterward.As per coding guidelines,
Use cmdutil.TestFactory(t, config) for test factories in unit tests.Also applies to: 91-94, 129-132
🤖 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/auth/users_logout_profile_fallback_test.go` around lines 47 - 52, Replace the hand-constructed factory in the test with the canonical test fixture: use cmdutil.TestFactory(t, config) instead of &cmdutil.Factory{...}; after creating the test factory you can set its IOStreams buffers and override Invocation.Profile (e.g. set f.IOStreams = streams and f.Invocation.Profile = "ghost") so the test uses the standard wiring for IO, keychain and other defaults; apply the same change for the other occurrences referenced (lines around 91-94 and 129-132) to ensure consistency.
🤖 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/auth/login_holder.go`:
- Around line 123-176: In verifyHolder, sanitize holderOpenId and freshOpenId
before using them in human-facing strings: call the existing display-sanitizer
(or add one, e.g., sanitizeDisplay) on the IDs and on formatHolderLabel inputs
and use those sanitized values when building hints, the errs.NewConfigError
messages, and the soft-warning message string; keep the original raw
holderOpenId/freshOpenId in the holderMismatchWarning struct fields and in any
structured/error fields, but never interpolate the raw IDs into message or hint
text (update the code paths that call errs.NewConfigError, the "flag" and "env"
hint construction, and the "" branch message construction to use the sanitized
variants).
In `@cmd/auth/login_postflock_r2_test.go`:
- Around line 35-37: The test currently sets LARKSUITE_CLI_CONFIG_DIR and HOME
but not USERPROFILE, which makes Windows runs nondeterministic; update the setup
in cmd/auth/login_postflock_r2_test.go so the fake home is deterministic by
either calling the shared helper setFakeOSHome(t, dir) or by adding
t.Setenv("USERPROFILE", dir) immediately after t.Setenv("HOME", ...); ensure you
still set LARKSUITE_CLI_CONFIG_DIR to dir.
In `@cmd/auth/logout_test.go`:
- Around line 206-210: The test currently discards errors from setup calls
(larkauth.SetStoredToken, larkauth.SaveUserProfileFor,
larkauth.RecordUserActivity) which can hide fixture failures; change each
ignored call to capture its error and call t.Fatalf on non-nil (e.g. if err :=
larkauth.SetStoredToken(...); err != nil { t.Fatalf("SetStoredToken failed: %v",
err) }) and do the same for SaveUserProfileFor and RecordUserActivity (keep ctx
:= larkauth.ForUser(u.app, u.oid) as-is).
- Around line 22-28: Replace direct t.Setenv("HOME", ...) usage with the repo
test helper setFakeOSHome(t, dir) so both Unix and Windows home envs are
consistently set; in this file update stubLogoutConfig (and the other similar
call sites where HOME is set: the other setup helpers around the second and
third occurrences) to call setFakeOSHome(t, cfgDir) (or the appropriate temp
dir) after keyring.MockInit(), ensuring tests use the shared helper that sets
HOME and USERPROFILE.
In `@cmd/auth/logout.go`:
- Around line 78-79: The current code captures profileName from
app.ProfileName() before acquiring the flock, which can become stale; instead,
only snapshot an explicit --profile flag (detect the flag source), and for the
normal path recompute the target app after the lock using the locked multi state
(i.e., call FindAppIndex against the post-lock multi to select the active
profile). Concretely: remove/avoid using the pre-lock profileName for the
non-explicit case, detect whether a profile was explicitly passed, and if not
call multi (the locked snapshot) to resolve the active app before calling
FindAppIndex and performing the logout operations.
In `@cmd/auth/users_logout.go`:
- Around line 125-139: The removal of the keychain token
(larkauth.RemoveStoredToken) happens before persisting the updated multi-app
config (core.SaveMultiAppConfig), which can leave config.json pointing to a user
with no token if the save fails; change the sequence so you remove the victim
from app.Users and clear app.CurrentUser as currently done, then call
core.SaveMultiAppConfig(multi) first and only after a successful save call
larkauth.RemoveStoredToken(app.AppId, victim.UserOpenId); keep the existing
warning behavior if RemoveStoredToken fails (log to f.IOStreams.ErrOut) but do
not remove the token unless the config save succeeded.
In `@cmd/auth/users_use.go`:
- Around line 103-108: The post-lock reload can return nil/empty causing a panic
when dereferencing multi; after calling core.LoadMultiAppConfig() check that
multi is non-nil and that multi.FindAppIndex(app.ProfileName()) is valid before
using it, and if not return
core.PassThroughOrNotConfigured(errOrNewNotConfiguredError) (i.e., guard the
dereference of multi and handle the not-configured path), updating the block
around LoadMultiAppConfig, multi, FindAppIndex and the subsequent use of idx
accordingly.
In `@cmd/bootstrap_user_test.go`:
- Around line 19-199: Tests call BootstrapInvocationContext which triggers
runMigrationOnce; set t.Setenv("LARKSUITE_CLI_CONFIG_DIR", t.TempDir()) at the
start of each test (or in clearUserEnv helper) to isolate config state, and
stub/reset the migration sync.Once by replacing runMigrationOnce with a no-op
per test or resetting the migrateOnce variable before invoking
BootstrapInvocationContext so each test starts with a fresh migration state;
reference BootstrapInvocationContext, runMigrationOnce and migrateOnce to locate
where to apply the env setup and the stub/reset.
In `@cmd/config/bind_lock_path_test.go`:
- Around line 47-48: Replace direct t.Setenv("HOME", ...) calls in the tests
with a helper that sets both HOME and USERPROFILE to the same temp dir;
implement a helper like setFakeOSHome(t testing.TB, dir string) that calls
t.Setenv("HOME", dir) and t.Setenv("USERPROFILE", dir), then use
setFakeOSHome(t, t.TempDir()) in place of t.Setenv("HOME", ...) in the test body
(also update the other occurrences around the second fixture at the later
lines). Ensure the existing t.Setenv("LARKSUITE_CLI_CONFIG_DIR", configDir)
remains unchanged.
In `@cmd/config/bind_r2_test.go`:
- Around line 35-45: The test currently hardcodes a future schema version as the
literal 2 when writing the hermes fixture (see hermesDir/ hermesConfigPath setup
in bind_r2_test.go); change the fixture to compute the future version as
core.CurrentSchemaVersion+1 and use that numeric value in the JSON payload and
any corresponding assertion messages (also update the other fixture at the
second occurrence around lines 73-75) so the test remains forward-compatible
when CurrentSchemaVersion is bumped. Use core.CurrentSchemaVersion+1 (formatted
into the JSON string or built with a small struct marshaled to JSON) wherever
the literal 2 is used for the “future” schema version and update assertions that
mention the version accordingly.
- Around line 50-58: The test sets up HERMES_HOME and calls configBindRun with
BindOptions{Factory: f, Source: "hermes"} but does not clear ambient agent env
vars that finalizeSource checks, so a CI/dev environment with OPENCLAW_* or
LARK_CHANNEL_* will cause a source-mismatch before the R2 path runs; before
writing the .env and invoking configBindRun (and similarly in the block around
lines 112-120), explicitly unset or clear any detection env used by
finalizeSource (e.g., remove OPENCLAW_* and LARK_CHANNEL_* variables from
os.Environ via os.Unsetenv for each OPENCLAW_* and LARK_CHANNEL_* key) so the
test environment deterministically pins Hermes and exercises the R2 path when
calling configBindRun/BindOptions and finalizeSource.
In `@cmd/config/bind_uat_cleanup_test.go`:
- Around line 33-180: Tests call cleanupKeychainFromData which uses
larkauth.NewLocalRoot(core.GetConfigDir()) and therefore mutates the real config
root; isolate the config dir in each test by setting the env var before calling
cleanupKeychainFromData (e.g. call t.Setenv("LARKSUITE_CLI_CONFIG_DIR",
t.TempDir()) at the top of each test) and keep keyring.MockInit() as well, or
add a small helper (e.g. initTestEnv or similar) that performs both
keyring.MockInit() and t.Setenv("LARKSUITE_CLI_CONFIG_DIR", t.TempDir()) then
use that helper in all tests in this file.
In `@cmd/config/init_flock_test.go`:
- Around line 41-42: These tests call
core.SetCurrentWorkspace(core.WorkspaceLocal) and never restore global state;
capture the previous workspace (e.g., prev := core.CurrentWorkspace()) before
changing it and register a cleanup to restore it (t.Cleanup(func(){
core.SetCurrentWorkspace(prev) })) immediately after setting the env and before
mutating state; apply this pattern in each test/subtest that calls
core.SetCurrentWorkspace (including the occurrences around the lines shown) so
global workspace is reset after the test.
In `@internal/auth/context.go`:
- Around line 69-113: The sanitizer sanitizeOpenIdForPath used by
AuthContext.sanitizedUserOpenId and sanitizedAppId is lossy and can alias
different IDs; change it to produce a filesystem-safe but collision-resistant
segment by preserving the current sanitized prefix and appending a stable hash
suffix (e.g., "-" + hex of SHA256 of the original trimmed string) so the result
remains safe for joins but is unique per original ID; keep the empty-string ->
"_" behavior and the same allowed characters for the prefix, and update any
callers that rely on sanitizeOpenIdForPath (e.g., storage path resolution) to
use the new non-lossy output.
- Around line 43-47: ForUser currently returns an AuthContext with an empty
appId but a non-empty userOpenId which creates an invalid state; change ForUser
to detect when strings.TrimSpace(appId) == "" and in that case return the
zero-value AuthContext (e.g. return AuthContext{}), otherwise return the trimmed
values as before. Update the ForUser function so empty/blank appId collapses to
the single-user (zero) context.
In `@internal/auth/storage.go`:
- Around line 346-373: The looksLikeJSON check in keychainTokenStore.saveSlot is
too weak (it only checks first non-space byte via looksLikeJSON) and lets
malformed JSON like "{not json" be persisted; replace the heuristic with a
proper JSON validation (e.g., call json.Valid(envelope) or attempt
json.Unmarshal into an interface{}) inside saveSlot (or update looksLikeJSON to
use json.Valid) and return a clear error (e.g., "auth: token envelope is not
valid JSON") when validation fails before calling keychain.Set.
In `@internal/auth/user_index.go`:
- Around line 142-145: The code in the json.Unmarshal error path writes a raw
warning to os.Stderr (fmt.Fprintf(os.Stderr,...)) which is forbidden from
internal/; change the logic in the UserIndex load/parse path to emit the warning
via a caller-supplied writer or logger instead (e.g., accept an io.Writer or
logging hook on the UserIndex loader or package-level variable and use that
instead of os.Stderr), update the error branch that currently calls fmt.Fprintf
to call the provided writer/log function with the same message and retain the
same return (UserIndex{Users: map[string]UserIndexEntry{}}, nil).
In `@internal/core/config.go`:
- Around line 375-383: The error message produced when app == nil in
CurrentAppConfig currently reports profile %q using profileOverride, which can
be empty and hide a stale raw.CurrentApp selector; update the ConfigError
construction in the app == nil branch (the block that returns &ConfigError{...})
to detect when profileOverride == "" and instead include raw.CurrentApp (or both
raw.CurrentApp and profileOverride) in the Message/Hint so the error reports the
dangling CurrentApp value; adjust the Message and/or Hint fields within that
ConfigError to clearly show the stale selector and available profiles.
In `@internal/migrate/migrate.go`:
- Around line 47-50: The current check collapses all failures from
core.LoadMultiAppConfig() into a noOp; change it to propagate actual errors and
only no-op for the benign "no config yet" case (or explicit
forward-incompatible/already-current cases handled later). Concretely, update
the logic around core.LoadMultiAppConfig() so that if err != nil you
return/propagate that err (instead of returning noOp), and only return noOp when
err == nil and multi == nil (or when your explicit schema-version checks later
determine a safe no-op); apply the same fix to the similar branch around lines
68-70 so parse/permission/corruption errors are not swallowed.
In `@internal/validate/sanitize.go`:
- Around line 17-42: The comment overstates OSC handling: SanitizeForTerminal
currently strips OSC sequences terminated by BEL (ESC ] ... BEL) but not the ST
form (ESC ] ... ESC \), so the C0 sweep can remove the ESC and leave the OSC
payload visible; update the ansiEscape logic used by SanitizeForTerminal to
recognize and consume both OSC terminators (BEL and the two-byte ST sequence ESC
followed by backslash) — e.g. extend the existing regex/parse in ansiEscape to
match ESC ] .*? (BEL|ESC\\) so the entire OSC payload is removed and not left
for the C0 sweep; alternatively, if you choose not to change behavior, shorten
the comment to avoid claiming full OSC coverage.
---
Outside diff comments:
In `@cmd/auth/login.go`:
- Around line 433-438: When opts.JSON is true and the authorization branch sees
!result.OK, don't return errs.NewAuthenticationError (which sends a typed error
on stderr); instead mirror the device-code immediate-path: if
shouldRemoveLoginRequestedScope(result) call cleanupRequestedScope(), then emit
the "authorization_failed" NDJSON envelope to stdout (same envelope shape used
by the device-code path) and return output.ErrBare(output.ExitAuth). For
non-JSON paths keep the existing errs.NewAuthenticationError behavior. Locate
this logic around the !result.OK check in login.go and reuse the same
event-formatting and output.ErrBare return used by the device-code flow.
In `@internal/core/config.go`:
- Around line 188-214: FindApp and FindAppIndex dropped support for the special
alias "default"; restore it by detecting if the incoming name == "default" at
the top of both MultiAppConfig.FindApp and MultiAppConfig.FindAppIndex and
replacing name with the currently active app identifier (use the MultiAppConfig
field that holds the active app, e.g. m.Active or m.ActiveApp) before running
the existing loops over m.Apps; ensure both functions use the same replacement
logic so they remain in sync.
---
Nitpick comments:
In `@cmd/auth/users_logout_profile_fallback_test.go`:
- Around line 47-52: Replace the hand-constructed factory in the test with the
canonical test fixture: use cmdutil.TestFactory(t, config) instead of
&cmdutil.Factory{...}; after creating the test factory you can set its IOStreams
buffers and override Invocation.Profile (e.g. set f.IOStreams = streams and
f.Invocation.Profile = "ghost") so the test uses the standard wiring for IO,
keychain and other defaults; apply the same change for the other occurrences
referenced (lines around 91-94 and 129-132) to ensure consistency.
In `@cmd/auth/users_test.go`:
- Around line 24-29: In stubUsersConfig replace the direct t.Setenv("HOME",
t.TempDir()) call with the repo helper that sets both HOME and USERPROFILE:
create a tempDir := t.TempDir() (or reuse the existing temp dir used for
LARKSUITE_CLI_CONFIG_DIR), then call setFakeOSHome(t, tempDir); this ensures
both HOME and USERPROFILE are set for Windows CI while leaving the rest of
stubUsersConfig (including keyring.MockInit and LARKSUITE_CLI_CONFIG_DIR)
unchanged.
In `@cmd/auth/users_use_profile_fallback_test.go`:
- Around line 59-64: Replace the manual test factory construction that creates a
&cmdutil.Factory (setting IOStreams and Invocation.Profile) with the standard
helper cmdutil.TestFactory(t, config) so the authUsersUseRun tests use the same
test wiring as the suite; locate the test setup where streams :=
&cmdutil.IOStreams and f := &cmdutil.Factory{IOStreams: streams, Invocation:
cmdutil.InvocationContext{Profile: "ghost"}} and instantiate f via
cmdutil.TestFactory(t, /* appropriate config map or nil */) and then set the
profile on the returned Factory (or pass it via the config) before calling
authUsersUseRun to ensure consistent factory behavior across tests.
In `@internal/auth/purge_test.go`:
- Around line 175-192: The test currently only asserts a hand-written string and
doesn't exercise PurgeUserArtifacts's real error-aggregation path; replace the
placeholder check by constructing a failing Root/KV (or small fake) that makes
DeleteUserProfileFor and DeleteUser return distinct errors, call
PurgeUserArtifacts(root, "cli_app", "ou_alice") against that fake root (after
keyring.MockInit()), and assert the returned error is non-nil and its Error()
contains both component messages joined with "; " to lock the aggregation
behavior of PurgeUserArtifacts and the DeleteUserProfileFor/DeleteUser legs.
🪄 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: 460fbb10-b643-4c14-b8d0-0298b41c4426
📒 Files selected for processing (89)
cmd/auth/auth.gocmd/auth/list.gocmd/auth/list_r2_test.gocmd/auth/login.gocmd/auth/login_config_test.gocmd/auth/login_holder.gocmd/auth/login_holder_gate_test.gocmd/auth/login_holder_sanitize_test.gocmd/auth/login_holder_test.gocmd/auth/login_holder_username_test.gocmd/auth/login_legacy_workflow_test.gocmd/auth/login_new_user_test.gocmd/auth/login_postflock_r2_test.gocmd/auth/login_result.gocmd/auth/login_result_test.gocmd/auth/login_step8_test.gocmd/auth/login_test.gocmd/auth/logout.gocmd/auth/logout_test.gocmd/auth/users.gocmd/auth/users_list.gocmd/auth/users_logout.gocmd/auth/users_logout_active_user_test.gocmd/auth/users_logout_profile_fallback_test.gocmd/auth/users_test.gocmd/auth/users_use.gocmd/auth/users_use_profile_fallback_test.gocmd/bootstrap.gocmd/bootstrap_user_test.gocmd/config/bind.gocmd/config/bind_lock_path_test.gocmd/config/bind_r2_test.gocmd/config/bind_uat_cleanup_test.gocmd/config/config_test.gocmd/config/default_as.gocmd/config/init.gocmd/config/init_currentuser_test.gocmd/config/init_flock_test.gocmd/config/remove.gocmd/config/remove_sweep_test.gocmd/config/show.gocmd/config/show_path_lock_skip_test.gocmd/config/show_r2_test.gocmd/config/strict_mode.gocmd/global_flags.gocmd/global_flags_test.gocmd/profile/add.gocmd/profile/list.gocmd/profile/profile_list_currentuser_test.gocmd/profile/profile_remove_self_toggle_test.gocmd/profile/profile_test.gocmd/profile/remove.gocmd/profile/remove_sweep_test.gocmd/profile/rename.gocmd/profile/use.gointernal/auth/context.gointernal/auth/context_test.gointernal/auth/purge.gointernal/auth/purge_test.gointernal/auth/storage.gointernal/auth/storage_test.gointernal/auth/user_index.gointernal/auth/user_index_test.gointernal/auth/user_profile.gointernal/auth/user_profile_test.gointernal/cmdutil/factory.gointernal/cmdutil/factory_default.gointernal/cmdutil/factory_default_step7_test.gointernal/core/config.gointernal/core/config_multiuser_test.gointernal/core/config_step6_test.gointernal/core/config_test.gointernal/core/errors.gointernal/core/notconfigured.gointernal/core/passthrough_or_notconfigured_test.gointernal/core/require_config_r2_test.gointernal/core/resolve_login_test.gointernal/credential/decorate_user_resolution_test.gointernal/credential/default_provider.gointernal/credential/default_provider_r2_test.gointernal/credential/default_provider_step7_test.gointernal/credential/integration_test.gointernal/envvars/envvars.gointernal/errcompat/promote.gointernal/errcompat/promote_r2_test.gointernal/migrate/migrate.gointernal/migrate/migrate_test.gointernal/validate/sanitize.goskills/lark-shared/SKILL.md
Comment on lines
+123
to
+176
| func verifyHolder(holderOpenId, holderUserName, holderSource, freshOpenId, freshUserName string) (*holderMismatchWarning, error) { | ||
| if holderOpenId == "" || holderOpenId == freshOpenId { | ||
| return nil, nil | ||
| } | ||
|
|
||
| switch holderSource { | ||
| case "flag": | ||
| hint := "you passed --user " + holderOpenId + " but the device you authorized was " + freshOpenId + | ||
| ". Re-run with `--user " + freshOpenId + "` to register the user you actually authorized," + | ||
| " or re-authorize on a device signed in as " + holderOpenId + "." | ||
| return nil, errs.NewConfigError(errs.SubtypeInvalidArgument, | ||
| "login holder mismatch: requested user %q but authorized user is %q", | ||
| holderOpenId, freshOpenId). | ||
| WithHint(hint) | ||
| case "env": | ||
| hint := "LARKSUITE_CLI_OPEN_ID is set to " + holderOpenId + " but the device you authorized was " + freshOpenId + | ||
| " — unset the env var or re-run with `--user " + freshOpenId + "`." | ||
| return nil, errs.NewConfigError(errs.SubtypeInvalidArgument, | ||
| "login holder mismatch: requested user %q but authorized user is %q", | ||
| holderOpenId, freshOpenId). | ||
| WithHint(hint) | ||
| case "": | ||
| // Implied holder (AppConfig.CurrentUser). Soft mismatch — emit a | ||
| // warning and let login proceed. The Message must: | ||
| // 1. NOT recommend "re-run without --user" (the operator already | ||
| // did not pass --user; that hint is self-contradictory). | ||
| // 2. Tell the operator the new active-user semantics: the fresh | ||
| // user is appended, the active user is unchanged, and the | ||
| // explicit switch command. | ||
| // 3. Render "Alice (ou_alice)" when display names are available | ||
| // so the human reader can map open_ids to people; fall back | ||
| // to bare open_id when the name is unknown. | ||
| holderLabel := formatHolderLabel(holderUserName, holderOpenId) | ||
| freshLabel := formatHolderLabel(freshUserName, freshOpenId) | ||
| message := "[lark-cli] [WARN] auth login: the active profile's currentUser is " + holderLabel + | ||
| " but the device you authorized was " + freshLabel + | ||
| ". Login will proceed and add " + freshLabel + " to the profile;" + | ||
| " the active user stays " + holderLabel + | ||
| ". Run `lark-cli auth users use " + freshOpenId + "` to switch the active user." | ||
| return &holderMismatchWarning{ | ||
| HolderOpenId: holderOpenId, | ||
| HolderUserName: holderUserName, | ||
| FreshOpenId: freshOpenId, | ||
| FreshUserName: freshUserName, | ||
| Message: message, | ||
| }, nil | ||
| default: | ||
| // Fail-closed: an unknown source is a programmer bug at the | ||
| // resolver, not the operator's fault. Surface it as an internal | ||
| // error so it gets fixed instead of silently downgrading the | ||
| // gate's safety property. | ||
| return nil, errs.NewInternalError(errs.SubtypeUnknown, | ||
| "verifyHolder: unknown holderSource %q (expected \"\", \"flag\", or \"env\") — cannot decide whether to abort or advise; refusing to proceed", | ||
| holderSource) |
There was a problem hiding this comment.
Sanitize open_id before interpolating it into human-readable messages.
These branches still embed raw holderOpenId / freshOpenId into ConfigError.Message, Hint, and the soft advisory text. When lookup misses, holderOpenId can come straight from --user or LARKSUITE_CLI_OPEN_ID, so ANSI/C0/RTL bytes survive into the human-readable stderr/JSON message fields this PR is otherwise sanitizing.
Keep the raw IDs in structured fields only, and sanitize the display copy before building message strings.
Also applies to: 201-205
🤖 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/auth/login_holder.go` around lines 123 - 176, In verifyHolder, sanitize
holderOpenId and freshOpenId before using them in human-facing strings: call the
existing display-sanitizer (or add one, e.g., sanitizeDisplay) on the IDs and on
formatHolderLabel inputs and use those sanitized values when building hints, the
errs.NewConfigError messages, and the soft-warning message string; keep the
original raw holderOpenId/freshOpenId in the holderMismatchWarning struct fields
and in any structured/error fields, but never interpolate the raw IDs into
message or hint text (update the code paths that call errs.NewConfigError, the
"flag" and "env" hint construction, and the "" branch message construction to
use the sanitized variants).
Comment on lines
+35
to
+37
| dir := t.TempDir() | ||
| t.Setenv("LARKSUITE_CLI_CONFIG_DIR", dir) | ||
| t.Setenv("HOME", t.TempDir()) |
There was a problem hiding this comment.
Set USERPROFILE alongside HOME here.
Setting only HOME leaves Windows runs machine-dependent if any code in this path resolves the OS home directory. Please use the shared helper for fake home setup, or set both env vars explicitly.
Based on learnings: use a shared setFakeOSHome(t, dir) helper that sets both HOME and USERPROFILE via t.Setenv; relying on HOME alone leaves Windows nondeterministic.
🤖 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/auth/login_postflock_r2_test.go` around lines 35 - 37, The test currently
sets LARKSUITE_CLI_CONFIG_DIR and HOME but not USERPROFILE, which makes Windows
runs nondeterministic; update the setup in cmd/auth/login_postflock_r2_test.go
so the fake home is deterministic by either calling the shared helper
setFakeOSHome(t, dir) or by adding t.Setenv("USERPROFILE", dir) immediately
after t.Setenv("HOME", ...); ensure you still set LARKSUITE_CLI_CONFIG_DIR to
dir.
Comment on lines
+22
to
+28
| func stubLogoutConfig(t *testing.T, currentUser string, users []core.AppUser) (*cmdutil.Factory, string, string) { | ||
| t.Helper() | ||
| keyring.MockInit() | ||
| cfgDir := t.TempDir() | ||
| t.Setenv("LARKSUITE_CLI_CONFIG_DIR", cfgDir) | ||
| t.Setenv("HOME", t.TempDir()) | ||
|
|
There was a problem hiding this comment.
Use the shared fake-home helper here.
These setups only override HOME. On Windows, os.UserHomeDir can still resolve through USERPROFILE/HOMEDRIVE/HOMEPATH, so the keyring/file paths stay machine-dependent and the tests can flap. Please switch these call sites to the repo’s setFakeOSHome(t, dir) helper. Based on learnings: "In larksuite/cli Go tests for home-directory resolution ... use a shared setFakeOSHome(t, dir) test helper that sets both the HOME (Unix) and USERPROFILE (Windows) environment variables via t.Setenv."
Also applies to: 87-91, 177-181
🤖 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/auth/logout_test.go` around lines 22 - 28, Replace direct
t.Setenv("HOME", ...) usage with the repo test helper setFakeOSHome(t, dir) so
both Unix and Windows home envs are consistently set; in this file update
stubLogoutConfig (and the other similar call sites where HOME is set: the other
setup helpers around the second and third occurrences) to call setFakeOSHome(t,
cfgDir) (or the appropriate temp dir) after keyring.MockInit(), ensuring tests
use the shared helper that sets HOME and USERPROFILE.
Comment on lines
+206
to
+210
| _ = larkauth.SetStoredToken(&larkauth.StoredUAToken{AppId: u.app, UserOpenId: u.oid, AccessToken: "tok"}) | ||
| ctx := larkauth.ForUser(u.app, u.oid) | ||
| now := time.Now().UTC() | ||
| _ = larkauth.SaveUserProfileFor(root, ctx, larkauth.UserProfile{UserOpenId: u.oid, UserName: u.name, CachedAt: now, FirstAuthAt: now}) | ||
| _ = larkauth.RecordUserActivity(root, ctx, nil) |
There was a problem hiding this comment.
Fail fast on fixture setup errors in the preservation test.
These writes are part of the test preconditions, but their errors are discarded. If one of them fails, the test can still “pass” by never creating Carol’s state in the first place. Please assert each error with t.Fatalf(...) so the preservation check is meaningful.
🔧 Suggested fix
- _ = larkauth.SetStoredToken(&larkauth.StoredUAToken{AppId: u.app, UserOpenId: u.oid, AccessToken: "tok"})
+ if err := larkauth.SetStoredToken(&larkauth.StoredUAToken{AppId: u.app, UserOpenId: u.oid, AccessToken: "tok"}); err != nil {
+ t.Fatalf("SetStoredToken(%s): %v", u.oid, err)
+ }
ctx := larkauth.ForUser(u.app, u.oid)
now := time.Now().UTC()
- _ = larkauth.SaveUserProfileFor(root, ctx, larkauth.UserProfile{UserOpenId: u.oid, UserName: u.name, CachedAt: now, FirstAuthAt: now})
- _ = larkauth.RecordUserActivity(root, ctx, nil)
+ if err := larkauth.SaveUserProfileFor(root, ctx, larkauth.UserProfile{UserOpenId: u.oid, UserName: u.name, CachedAt: now, FirstAuthAt: now}); err != nil {
+ t.Fatalf("SaveUserProfileFor(%s): %v", u.oid, err)
+ }
+ if err := larkauth.RecordUserActivity(root, ctx, nil); err != nil {
+ t.Fatalf("RecordUserActivity(%s): %v", u.oid, err)
+ }
}🤖 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/auth/logout_test.go` around lines 206 - 210, The test currently discards
errors from setup calls (larkauth.SetStoredToken, larkauth.SaveUserProfileFor,
larkauth.RecordUserActivity) which can hide fixture failures; change each
ignored call to capture its error and call t.Fatalf on non-nil (e.g. if err :=
larkauth.SetStoredToken(...); err != nil { t.Fatalf("SetStoredToken failed: %v",
err) }) and do the same for SaveUserProfileFor and RecordUserActivity (keep ctx
:= larkauth.ForUser(u.app, u.oid) as-is).
Comment on lines
+78
to
79
| profileName := app.ProfileName() | ||
|
|
There was a problem hiding this comment.
Resolve the implicit target profile after acquiring the flock.
profileName is captured from the unlocked preflight read. If another command switches the current profile before Line 93 reloads under lock, FindAppIndex(profileName) will wipe the previously active profile instead of the profile that is active in the locked snapshot. Only snapshot an explicit --profile; otherwise recompute the target app from the post-lock multi.
Also applies to: 93-111
🤖 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/auth/logout.go` around lines 78 - 79, The current code captures
profileName from app.ProfileName() before acquiring the flock, which can become
stale; instead, only snapshot an explicit --profile flag (detect the flag
source), and for the normal path recompute the target app after the lock using
the locked multi state (i.e., call FindAppIndex against the post-lock multi to
select the active profile). Concretely: remove/avoid using the pre-lock
profileName for the non-explicit case, detect whether a profile was explicitly
passed, and if not call multi (the locked snapshot) to resolve the active app
before calling FindAppIndex and performing the logout operations.
Comment on lines
+346
to
+373
| func (s *keychainTokenStore) saveSlot(_ slotKind, envelope []byte) error { | ||
| key, err := s.accountKeyFor() | ||
| if err != nil { | ||
| return err | ||
| } | ||
| // Cheap envelope sanity check: keychain backends accept any | ||
| // string, but envelopes are always JSON, so a caller passing raw | ||
| // garbage gets a typed error rather than a silent write. | ||
| if !looksLikeJSON(envelope) { | ||
| return fmt.Errorf("auth: token envelope is not JSON") | ||
| } | ||
| return keychain.Set(keychain.LarkCliService, key, string(envelope)) | ||
| } | ||
|
|
||
| // looksLikeJSON skips a full json.Valid call: this is a programmer-error | ||
| // guard, not validation. | ||
| func looksLikeJSON(b []byte) bool { | ||
| for _, c := range b { | ||
| switch c { | ||
| case ' ', '\t', '\r', '\n': | ||
| continue | ||
| case '{': | ||
| return true | ||
| default: | ||
| return false | ||
| } | ||
| } | ||
| return false |
There was a problem hiding this comment.
Reject malformed JSON before persisting token envelopes.
looksLikeJSON only checks that the first non-space byte is {, so malformed payloads like {not json pass and get written to the keychain. That violates the “JSON envelope” contract and turns a bad write into a later parse/read failure.
Proposed fix
func (s *keychainTokenStore) saveSlot(_ slotKind, envelope []byte) error {
key, err := s.accountKeyFor()
if err != nil {
return err
}
- // Cheap envelope sanity check: keychain backends accept any
- // string, but envelopes are always JSON, so a caller passing raw
- // garbage gets a typed error rather than a silent write.
- if !looksLikeJSON(envelope) {
- return fmt.Errorf("auth: token envelope is not JSON")
+ // Envelopes must be JSON objects, not just "object-shaped" text.
+ if !looksLikeJSONObject(envelope) {
+ return fmt.Errorf("auth: token envelope must be a JSON object")
}
return keychain.Set(keychain.LarkCliService, key, string(envelope))
}
-// looksLikeJSON skips a full json.Valid call: this is a programmer-error
-// guard, not validation.
-func looksLikeJSON(b []byte) bool {
+func looksLikeJSONObject(b []byte) bool {
+ if !json.Valid(b) {
+ return false
+ }
for _, c := range b {
switch c {
case ' ', '\t', '\r', '\n':
continue
case '{':
return true
default:
return false
}
}
return false
}📝 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.
Suggested change
| func (s *keychainTokenStore) saveSlot(_ slotKind, envelope []byte) error { | |
| key, err := s.accountKeyFor() | |
| if err != nil { | |
| return err | |
| } | |
| // Cheap envelope sanity check: keychain backends accept any | |
| // string, but envelopes are always JSON, so a caller passing raw | |
| // garbage gets a typed error rather than a silent write. | |
| if !looksLikeJSON(envelope) { | |
| return fmt.Errorf("auth: token envelope is not JSON") | |
| } | |
| return keychain.Set(keychain.LarkCliService, key, string(envelope)) | |
| } | |
| // looksLikeJSON skips a full json.Valid call: this is a programmer-error | |
| // guard, not validation. | |
| func looksLikeJSON(b []byte) bool { | |
| for _, c := range b { | |
| switch c { | |
| case ' ', '\t', '\r', '\n': | |
| continue | |
| case '{': | |
| return true | |
| default: | |
| return false | |
| } | |
| } | |
| return false | |
| func (s *keychainTokenStore) saveSlot(_ slotKind, envelope []byte) error { | |
| key, err := s.accountKeyFor() | |
| if err != nil { | |
| return err | |
| } | |
| // Envelopes must be JSON objects, not just "object-shaped" text. | |
| if !looksLikeJSONObject(envelope) { | |
| return fmt.Errorf("auth: token envelope must be a JSON object") | |
| } | |
| return keychain.Set(keychain.LarkCliService, key, string(envelope)) | |
| } | |
| func looksLikeJSONObject(b []byte) bool { | |
| if !json.Valid(b) { | |
| return false | |
| } | |
| for _, c := range b { | |
| switch c { | |
| case ' ', '\t', '\r', '\n': | |
| continue | |
| case '{': | |
| return true | |
| default: | |
| return false | |
| } | |
| } | |
| return false | |
| } |
🤖 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/storage.go` around lines 346 - 373, The looksLikeJSON check in
keychainTokenStore.saveSlot is too weak (it only checks first non-space byte via
looksLikeJSON) and lets malformed JSON like "{not json" be persisted; replace
the heuristic with a proper JSON validation (e.g., call json.Valid(envelope) or
attempt json.Unmarshal into an interface{}) inside saveSlot (or update
looksLikeJSON to use json.Valid) and return a clear error (e.g., "auth: token
envelope is not valid JSON") when validation fails before calling keychain.Set.
Comment on lines
+142
to
+145
| if err := json.Unmarshal(data, &idx); err != nil { | ||
| // Corrupt: surface on stderr, return empty so the next Record reseeds. | ||
| fmt.Fprintf(os.Stderr, "[lark-cli] [WARN] auth: user index corrupt, rebuilding empty: %v\n", err) | ||
| return UserIndex{Users: map[string]UserIndexEntry{}}, nil |
There was a problem hiding this comment.
Don't write raw warnings to os.Stderr from internal/.
This path bypasses the command-layer stderr envelope contract and is likely to trip the repo's forbidigo rule for internal/ packages. Please route the warning through a caller-supplied writer/log hook instead of emitting directly here.
Based on learnings, os.Stderr is forbidden in internal/ packages and should not be used for ad-hoc warnings there.
🤖 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/user_index.go` around lines 142 - 145, The code in the
json.Unmarshal error path writes a raw warning to os.Stderr
(fmt.Fprintf(os.Stderr,...)) which is forbidden from internal/; change the logic
in the UserIndex load/parse path to emit the warning via a caller-supplied
writer or logger instead (e.g., accept an io.Writer or logging hook on the
UserIndex loader or package-level variable and use that instead of os.Stderr),
update the error branch that currently calls fmt.Fprintf to call the provided
writer/log function with the same message and retain the same return
(UserIndex{Users: map[string]UserIndexEntry{}}, nil).
Comment on lines
375
to
383
| app := raw.CurrentAppConfig(profileOverride) | ||
| if app == nil { | ||
| return nil, &ConfigError{ | ||
| Code: 3, | ||
| Type: "config", | ||
| Message: fmt.Sprintf("profile %q not found", profileOverride), | ||
| Hint: fmt.Sprintf("available profiles: %s", formatProfileNames(raw.ProfileNames())), | ||
| Rung: RungProfile, | ||
| } |
There was a problem hiding this comment.
Report the dangling CurrentApp value instead of profile "" not found.
When profileOverride is empty and raw.CurrentApp points at a removed profile, this path returns profile "" not found. That hides the stale selector the operator actually needs to fix and makes the remediation misleading.
Suggested fix
app := raw.CurrentAppConfig(profileOverride)
if app == nil {
+ missingProfile := profileOverride
+ if missingProfile == "" {
+ missingProfile = raw.CurrentApp
+ }
return nil, &ConfigError{
Code: 3,
Type: "config",
- Message: fmt.Sprintf("profile %q not found", profileOverride),
+ Message: fmt.Sprintf("profile %q not found", missingProfile),
Hint: fmt.Sprintf("available profiles: %s", formatProfileNames(raw.ProfileNames())),
Rung: RungProfile,
}
}🤖 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/core/config.go` around lines 375 - 383, The error message produced
when app == nil in CurrentAppConfig currently reports profile %q using
profileOverride, which can be empty and hide a stale raw.CurrentApp selector;
update the ConfigError construction in the app == nil branch (the block that
returns &ConfigError{...}) to detect when profileOverride == "" and instead
include raw.CurrentApp (or both raw.CurrentApp and profileOverride) in the
Message/Hint so the error reports the dangling CurrentApp value; adjust the
Message and/or Hint fields within that ConfigError to clearly show the stale
selector and available profiles.
Comment on lines
+47
to
+50
| multi, err := core.LoadMultiAppConfig() | ||
| if err != nil || multi == nil { | ||
| // No config yet — first `auth login` will stamp SchemaVersion. | ||
| return noOp |
There was a problem hiding this comment.
Don't collapse every config-load failure into noOp.
These branches currently swallow parse/permission/corruption errors from core.LoadMultiAppConfig() the same way as "no config yet". That loses the R2 error-transparency contract and makes real config failures disappear instead of reaching the bootstrap logger. Please no-op only for the specific benign cases (missing config / already-current / explicitly forward-incompatible), and propagate the rest.
Also applies to: 68-70
🤖 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/migrate/migrate.go` around lines 47 - 50, The current check
collapses all failures from core.LoadMultiAppConfig() into a noOp; change it to
propagate actual errors and only no-op for the benign "no config yet" case (or
explicit forward-incompatible/already-current cases handled later). Concretely,
update the logic around core.LoadMultiAppConfig() so that if err != nil you
return/propagate that err (instead of returning noOp), and only return noOp when
err == nil and multi == nil (or when your explicit schema-version checks later
determine a safe no-op); apply the same fix to the similar branch around lines
68-70 so parse/permission/corruption errors are not swallowed.
Comment on lines
17
to
+42
| // SanitizeForTerminal strips ANSI escape sequences, C0 control characters | ||
| // (except \n and \t), and dangerous Unicode from text, preserving the actual | ||
| // readable content. It should be applied to table format output and stderr | ||
| // messages, but NOT to json/ndjson output where programmatic consumers need | ||
| // the raw data. | ||
| // (except \n and \t), DEL (0x7f), and dangerous Unicode (zero-width joiners, | ||
| // RTL overrides, etc.) from text, preserving the readable content. | ||
| // | ||
| // API responses may contain injected ANSI sequences that clear the screen, | ||
| // fake a colored "OK" status, or change the terminal title. In AI Agent | ||
| // scenarios, such injections can also pollute the LLM's context window | ||
| // with misleading output. | ||
| // Apply this anywhere a string is destined for a terminal — table-format | ||
| // stdout, every stderr message, and ALSO any human-readable string that is | ||
| // embedded inside a JSON/NDJSON payload for compatibility (e.g. a `message` | ||
| // field that mirrors what stderr just printed). The rule is "sanitize on | ||
| // write to a TTY," not "sanitize before json.Marshal": a JSON consumer that | ||
| // pretty-prints the payload still surfaces those bytes to a terminal, and | ||
| // the sanitization is a one-way transform that strips only renderer-control | ||
| // codes — readable characters, including Unicode letters, are preserved. | ||
| // | ||
| // Do NOT apply to typed identity / data fields whose programmatic consumers | ||
| // need raw bytes (e.g. `holder_user_name`, `requested[]`, `granted[]`): | ||
| // those carry data, and any escape-stripping there is the consumer's job | ||
| // once they know what context they will render to. JSON's own escaping | ||
| // already protects byte-level transport for the wire format. | ||
| // | ||
| // API responses (and persisted MultiAppConfig values originally sourced | ||
| // from API responses) may contain injected ANSI sequences that clear the | ||
| // screen, fake a colored "OK" status, or change the terminal title. In AI | ||
| // Agent scenarios, such injections can also pollute the LLM's context | ||
| // window with misleading output. The sanitize-on-write boundary keeps the | ||
| // data layer pristine while preventing any TTY-bound rendering surface | ||
| // from being hijacked. |
There was a problem hiding this comment.
The new sanitizer contract overstates OSC coverage.
SanitizeForTerminal currently strips ESC ] ... BEL, but not the other common OSC terminator form ESC ] ... ESC \. In that case the C0 sweep removes the ESC bytes and leaves the OSC payload behind as visible junk, so “strips ANSI escape sequences” is not quite true yet. Either extend ansiEscape to consume the ST form too, or narrow this new comment.
Possible regex update
-var ansiEscape = regexp.MustCompile(`\x1b\[[0-9;?>=!]*[a-zA-Z]|\x1b\][^\x07]*\x07`)
+var ansiEscape = regexp.MustCompile(`\x1b\[[0-9;?>=!]*[a-zA-Z]|\x1b\][^\x07\x1b]*(?:\x07|\x1b\\)`)🤖 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/validate/sanitize.go` around lines 17 - 42, The comment overstates
OSC handling: SanitizeForTerminal currently strips OSC sequences terminated by
BEL (ESC ] ... BEL) but not the ST form (ESC ] ... ESC \), so the C0 sweep can
remove the ESC and leave the OSC payload visible; update the ansiEscape logic
used by SanitizeForTerminal to recognize and consume both OSC terminators (BEL
and the two-byte ST sequence ESC followed by backslash) — e.g. extend the
existing regex/parse in ansiEscape to match ESC ] .*? (BEL|ESC\\) so the entire
OSC payload is removed and not left for the C0 sweep; alternatively, if you
choose not to change behavior, shorten the comment to avoid claiming full OSC
coverage.
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
2 participants
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 / 概述
EN: Port the multi-user surface from park-cli into lark-cli — per-profile Users[] with an active CurrentUser, a holder-mismatch gate around auth login, R2 schema-version-aware config I/O,
and a sidecar/user-index sweep on every UAT remove path. No park-cli changes; existing single-user behavior preserved.
中文: 将 park-cli 的 multi-user 能力适配进 lark-cli —— 每个 profile 持有 Users[] 与一个 active CurrentUser,在 auth login 上加身份持有者(holder)校验门,配置读写感知 R2 schema 版本,并在所有
UAT 删除路径上同步清理 sidecar 与 user index。不改动 park-cli,不破坏现有单用户行为。
What's in / 主要变更
EN
advisory + structured holder_mismatch_warning JSON field on implied-holder mismatch (legacy "logout-and-login-as-someone-else" stays unbroken).
read-only show skips it.
stderr.
memo.
中文
holder_mismatch_warning 结构化字段),保留旧版"登出再登另一人"工作流。
Summary by CodeRabbit
New Features
auth users list,auth users use, andauth users logoutcommands--userflag andLARKSUITE_CLI_OPEN_IDenvironment variable for user selection during login and commandsBug Fixes
Documentation
auth login --jsonincluding holder mismatch warnings