-
Notifications
You must be signed in to change notification settings - Fork 621
fix(config): make agent-binding hints workspace-aware and surface user-identity risks #728
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,59 @@ | ||
| // Copyright (c) 2026 Lark Technologies Pte. Ltd. | ||
| // SPDX-License-Identifier: MIT | ||
|
|
||
| package auth | ||
|
|
||
| import ( | ||
| "strings" | ||
| "testing" | ||
|
|
||
| "github.com/larksuite/cli/internal/cmdutil" | ||
| "github.com/larksuite/cli/internal/core" | ||
| ) | ||
|
|
||
| // TestAuthListRun_NotConfigured_ReturnsExitZero pins the contract that | ||
| // `lark-cli auth list` is a read-only probe and must not fail-hard when no | ||
| // config exists yet — scripts and AI agents use it as an idempotent "do I | ||
| // have any users?" check, so the exit code carries semantic weight. Pair | ||
| // that with the existing "configured but no logged-in users" branch (also | ||
| // exit 0) and both empty states are consistent. | ||
| func TestAuthListRun_NotConfigured_ReturnsExitZero(t *testing.T) { | ||
| t.Setenv("LARKSUITE_CLI_CONFIG_DIR", t.TempDir()) | ||
|
|
||
| f, _, stderr, _ := cmdutil.TestFactory(t, nil) | ||
| if err := authListRun(&ListOptions{Factory: f}); err != nil { | ||
| t.Fatalf("auth list should succeed when not configured (exit 0); got: %v", err) | ||
| } | ||
| // Local workspace → hint must mention init, not bind. | ||
| out := stderr.String() | ||
| if !strings.Contains(out, "config init") { | ||
| t.Errorf("local hint missing config init: %s", out) | ||
| } | ||
| if strings.Contains(out, "config bind") { | ||
| t.Errorf("local hint must not mention config bind: %s", out) | ||
| } | ||
| } | ||
|
Comment on lines
+20
to
+35
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Stabilize workspace state explicitly in the local-workspace test. This test assumes local workspace behavior but never sets Suggested patch func TestAuthListRun_NotConfigured_ReturnsExitZero(t *testing.T) {
t.Setenv("LARKSUITE_CLI_CONFIG_DIR", t.TempDir())
+ prev := core.CurrentWorkspace()
+ t.Cleanup(func() { core.SetCurrentWorkspace(prev) })
+ core.SetCurrentWorkspace(core.WorkspaceLocal)
f, _, stderr, _ := cmdutil.TestFactory(t, nil)
if err := authListRun(&ListOptions{Factory: f}); err != nil {
t.Fatalf("auth list should succeed when not configured (exit 0); got: %v", err)
}🤖 Prompt for AI Agents |
||
|
|
||
| // TestAuthListRun_NotConfigured_AgentWorkspace_RoutesToBindHelp covers the | ||
| // reason this hint exists workspace-aware in the first place: an AI agent | ||
| // in OpenClaw / Hermes that probes auth list before binding gets routed to | ||
| // `config bind --help` instead of the local-only `config init`. | ||
| func TestAuthListRun_NotConfigured_AgentWorkspace_RoutesToBindHelp(t *testing.T) { | ||
| t.Setenv("LARKSUITE_CLI_CONFIG_DIR", t.TempDir()) | ||
|
|
||
| prev := core.CurrentWorkspace() | ||
| t.Cleanup(func() { core.SetCurrentWorkspace(prev) }) | ||
| core.SetCurrentWorkspace(core.WorkspaceOpenClaw) | ||
|
|
||
| f, _, stderr, _ := cmdutil.TestFactory(t, nil) | ||
| if err := authListRun(&ListOptions{Factory: f}); err != nil { | ||
| t.Fatalf("auth list should still succeed under agent workspace; got: %v", err) | ||
| } | ||
| out := stderr.String() | ||
| if !strings.Contains(out, "config bind --help") { | ||
| t.Errorf("agent hint must point at config bind --help: %s", out) | ||
| } | ||
| if strings.Contains(out, "config init") { | ||
| t.Errorf("agent hint must not mention config init: %s", out) | ||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,62 @@ | ||
| // Copyright (c) 2026 Lark Technologies Pte. Ltd. | ||
| // SPDX-License-Identifier: MIT | ||
|
|
||
| package config | ||
|
|
||
| import ( | ||
| "os" | ||
| "path/filepath" | ||
| "strings" | ||
| "testing" | ||
|
|
||
| "github.com/larksuite/cli/internal/cmdutil" | ||
| ) | ||
|
|
||
| // runHermesBindWithIdentity boots a Hermes-shaped fake env, runs `config bind` | ||
| // with the given identity preset in flag (non-TUI) mode, and returns captured | ||
| // stderr. Hermes is the simplest source to fake (single .env file). | ||
| func runHermesBindWithIdentity(t *testing.T, identity string) string { | ||
| t.Helper() | ||
| saveWorkspace(t) | ||
| configDir := t.TempDir() | ||
| t.Setenv("LARKSUITE_CLI_CONFIG_DIR", configDir) | ||
|
|
||
| hermesHome := t.TempDir() | ||
| t.Setenv("HERMES_HOME", hermesHome) | ||
| envContent := "FEISHU_APP_ID=cli_hermes_abc\nFEISHU_APP_SECRET=hermes_secret_123\nFEISHU_DOMAIN=lark\n" | ||
| if err := os.WriteFile(filepath.Join(hermesHome, ".env"), []byte(envContent), 0600); err != nil { | ||
| t.Fatalf("write .env: %v", err) | ||
| } | ||
|
|
||
| f, _, stderr, _ := cmdutil.TestFactory(t, nil) | ||
| err := configBindRun(&BindOptions{ | ||
| Factory: f, | ||
| Source: "hermes", | ||
| Identity: identity, | ||
| Lang: "zh", | ||
| }) | ||
| if err != nil { | ||
| t.Fatalf("bind failed: %v", err) | ||
| } | ||
| return stderr.String() | ||
|
Comment on lines
+18
to
+41
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Call the shared env scrubber before binding. This helper only sets ♻️ Proposed fix func runHermesBindWithIdentity(t *testing.T, identity string) string {
t.Helper()
saveWorkspace(t)
+ clearAgentEnv(t)
configDir := t.TempDir()🤖 Prompt for AI Agents |
||
| } | ||
|
|
||
| // TestConfigBindRun_UserDefaultIdentity_WarnsAboutImpersonation covers the | ||
| // gap that previously slipped through: a fresh flag-mode bind landing on | ||
| // user-default. warnIdentityEscalation requires a previous bot lock to fire, | ||
| // and IdentityUserDefaultDesc only renders in TUI selection — so without | ||
| // noticeUserDefaultRisk the user/AI never see the impersonation risk on a | ||
| // first-time user-default bind. | ||
| func TestConfigBindRun_UserDefaultIdentity_WarnsAboutImpersonation(t *testing.T) { | ||
| out := runHermesBindWithIdentity(t, "user-default") | ||
| if !strings.Contains(out, bindMsgZh.IdentityEscalationMessage) { | ||
| t.Errorf("user-default bind must surface IdentityEscalationMessage; got: %s", out) | ||
| } | ||
| } | ||
|
|
||
| func TestConfigBindRun_BotOnlyIdentity_NoImpersonationWarning(t *testing.T) { | ||
| out := runHermesBindWithIdentity(t, "bot-only") | ||
| if strings.Contains(out, bindMsgZh.IdentityEscalationMessage) { | ||
| t.Errorf("bot-only bind must NOT warn about impersonation; got: %s", out) | ||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -90,15 +90,15 @@ func TestConfigShowRun_NotConfiguredReturnsStructuredError(t *testing.T) { | |
| t.Fatal("expected error") | ||
| } | ||
|
|
||
| var exitErr *output.ExitError | ||
| if !errors.As(err, &exitErr) { | ||
| t.Fatalf("error type = %T, want *output.ExitError", err) | ||
| var cfgErr *core.ConfigError | ||
| if !errors.As(err, &cfgErr) { | ||
| t.Fatalf("error type = %T, want *core.ConfigError", err) | ||
| } | ||
| if exitErr.Code != output.ExitValidation { | ||
| t.Fatalf("exit code = %d, want %d", exitErr.Code, output.ExitValidation) | ||
| if cfgErr.Code != output.ExitValidation { | ||
| t.Fatalf("exit code = %d, want %d", cfgErr.Code, output.ExitValidation) | ||
| } | ||
| if exitErr.Detail == nil || exitErr.Detail.Type != "config" || exitErr.Detail.Message != "not configured" { | ||
| t.Fatalf("detail = %#v, want config/not configured", exitErr.Detail) | ||
| if cfgErr.Type != "config" || cfgErr.Message != "not configured" { | ||
| t.Fatalf("detail = %+v, want config/not configured", cfgErr) | ||
| } | ||
|
Comment on lines
+93
to
102
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Cover the empty-config case too. This test only exercises the missing-file path. Please add an empty 🤖 Prompt for AI Agents |
||
| } | ||
|
|
||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Don’t swallow config-load failures before returning “not configured”.
At Line 43,
LoadMultiAppConfigerrors are discarded, so Line 48 can incorrectly returnNotConfiguredErroreven for malformed JSON or permission errors. That hides the real remediation path.Suggested fix
As per coding guidelines: Make error messages structured, actionable, and specific for AI agent parsing.
📝 Committable suggestion
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 48-48: cmd/auth/list.go#L48
Added line
#L48was not covered by tests🤖 Prompt for AI Agents