fix(config): make agent-binding hints workspace-aware and surface user-identity risks#728
Open
liangshuo-1 wants to merge 1 commit intomainfrom
Open
fix(config): make agent-binding hints workspace-aware and surface user-identity risks#728liangshuo-1 wants to merge 1 commit intomainfrom
liangshuo-1 wants to merge 1 commit intomainfrom
Conversation
|
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:
📝 WalkthroughWalkthroughCentralizes workspace-aware "not configured" and "no active profile" errors in Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
github-actions
Bot
added
the
size/L
🚀 PR Preview Install Guide🧰 CLI updatenpm i -g https://pkg.pr.new/larksuite/cli/@larksuite/cli@9a89fc1dc35a5236a5e4703122de135ff45f122b🧩 Skill updatenpx skills add larksuite/cli#feat/agent-binding-hints -y -g |
liangshuo-1
force-pushed
the
feat/agent-binding-hints
branch
from
faf62e1 to
429060e
Compare
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #728 +/- ##
==========================================
+ Coverage 64.38% 64.66% +0.28%
==========================================
Files 512 517 +5
Lines 45426 45841 +415
==========================================
+ Hits 29249 29645 +396
- Misses 13604 13613 +9
- Partials 2573 2583 +10 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
cmd/config/strict_mode.go (1)
101-149:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winOnly warn when the effective mode actually changes.
resolveStrictModeStatusprefers profile overrides, so a--globalupdate can trip theoldEffective == botcheck even when that profile’s effective mode stays unchanged. That produces a false escalation warning for global updates on profiles that pin their own strict mode. Compare the post-change effective mode, or skip this warning when the current profile has its own override.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/config/strict_mode.go` around lines 101 - 149, The code currently computes oldEffective via resolveStrictModeStatus before mutating multi/app and then warns if oldEffective was bot and the new mode is user/off, which can produce false warnings when a profile has its own override; update setStrictMode to compute the new effective mode after persisting the change (call resolveStrictModeStatus again with the mutated multi and app) and only emit the identity-escalation warning if the new effective mode is different (oldEffective == StrictModeBot && newEffective != StrictModeBot), or alternately skip the warning entirely when the target profile has a non-nil app.StrictMode override; reference setStrictMode, resolveStrictModeStatus, multi, app and app.StrictMode to locate and implement the fix.cmd/config/bind_test.go (1)
152-160:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winExpand
clearAgentEnvto cover every workspace signal.
DetectWorkspaceFromEnvalso checksOPENCLAW_SERVICE_MARKER,OPENCLAW_SERVICE_VERSION,OPENCLAW_GATEWAY_PORT,OPENCLAW_SHELL, and the remaining Hermes vars. As written, the “no signals at all” tests can still inherit a host workspace and take the wrong branch. Reuse this helper to wipe the full list.♻️ Proposed fix
for _, k := range []string{ "OPENCLAW_CLI", "OPENCLAW_HOME", "OPENCLAW_STATE_DIR", "OPENCLAW_CONFIG_PATH", + "OPENCLAW_SERVICE_MARKER", "OPENCLAW_SERVICE_VERSION", "OPENCLAW_GATEWAY_PORT", "OPENCLAW_SHELL", "HERMES_HOME", "HERMES_QUIET", "HERMES_EXEC_ASK", "HERMES_GATEWAY_TOKEN", "HERMES_SESSION_KEY", } {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/config/bind_test.go` around lines 152 - 160, The helper clearAgentEnv currently clears only a subset of workspace env vars; update it to wipe every workspace/signal variable that DetectWorkspaceFromEnv checks so tests truly simulate "no signals". Add OPENCLAW_SERVICE_MARKER, OPENCLAW_SERVICE_VERSION, OPENCLAW_GATEWAY_PORT, OPENCLAW_SHELL and any remaining Hermes variables referenced by DetectWorkspaceFromEnv to the slice used in clearAgentEnv (the function name to update), using t.Setenv(k, "") as done now so all detection branches are neutralized.
🧹 Nitpick comments (1)
cmd/auth/login.go (1)
51-55: ⚡ Quick winReuse the shared strict-mode guard here.
This inline branch duplicates
Factory.CheckStrictMode(internal/cmdutil/factory.go:159-168), so the login error shape can drift from the rest of the CLI again. Delegating to the shared helper would keep the AI-facing envelope consistent.♻️ Suggested simplification
- if mode := f.ResolveStrictMode(cmd.Context()); mode == core.StrictModeBot { - return output.ErrWithHint(output.ExitValidation, "strict_mode", - fmt.Sprintf("strict mode is %q, user login is disabled in this profile", mode), - "if the user explicitly wants to switch to user identity, see `lark-cli config strict-mode --help` (confirm with the user before switching; switching does NOT require re-bind)") - } + if err := f.CheckStrictMode(cmd.Context(), core.AsUser); err != nil { + return err + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/auth/login.go` around lines 51 - 55, Replace the inline strict-mode branch with the shared guard by calling Factory.CheckStrictMode so the error shape stays consistent; specifically, remove the if block that uses f.ResolveStrictMode and instead invoke f.CheckStrictMode(cmd.Context(), core.StrictModeBot) (the CheckStrictMode helper in internal/cmdutil/factory.go) to perform the validation and return its result.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@cmd/config/bind_warning_test.go`:
- Around line 18-41: The helper runHermesBindWithIdentity should call the shared
environment scrubber to remove any leftover agent env vars before setting
HERMES_HOME; insert a call to clearAgentEnv(t) at the start of
runHermesBindWithIdentity so OPENCLAW_* (and related) variables are cleared
prior to running configBindRun, ensuring the Hermes warning path is not
bypassed.
In `@cmd/config/config_test.go`:
- Around line 93-102: Add a new test case that covers the "empty config" path by
creating an empty config.json fixture and invoking the same code path exercised
by the existing test (the caller that triggers configShowRun); assert that the
returned error is still a *core.ConfigError with Code == output.ExitValidation
and Type == "config" and Message == "not configured" (use the same cfgErr
variable checks as in the current test). Place the empty fixture alongside the
existing test fixtures and add a subtest or additional test function that points
the code under test to the empty file so the new "not-configured" branch in
configShowRun is covered.
In `@cmd/config/init_guard_test.go`:
- Around line 14-69: The tests for guardAgentWorkspace are flaky because they
only set OPENCLAW_HOME/HERMES_HOME while DetectWorkspaceFromEnv also checks
other env vars; update each test (TestGuardAgentWorkspace_LocalAllows,
TestGuardAgentWorkspace_OpenClawRefuses, TestGuardAgentWorkspace_HermesRefuses,
TestGuardAgentWorkspace_ForceInitOverride) to call clearAgentEnv(t) at the start
to clear the full workspace signal set before setting the specific env var(s),
so guardAgentWorkspace(&ConfigInitOptions{...}) uses a clean env; reference
clearAgentEnv(t), guardAgentWorkspace, ConfigInitOptions and
DetectWorkspaceFromEnv when making the change.
In `@internal/core/config.go`:
- Around line 226-229: The current code collapses real LoadMultiAppConfig()
failures into NotConfiguredError(); change the logic in the function that calls
LoadMultiAppConfig() so that if LoadMultiAppConfig() returns a non-nil err you
propagate/return that err directly (or wrap it) instead of mapping it to
NotConfiguredError(), and only return NotConfiguredError() when err == nil and
(raw == nil || len(raw.Apps) == 0); refer to LoadMultiAppConfig(), raw and
raw.Apps and NotConfiguredError() to locate the conditional to adjust.
In `@internal/core/notconfigured_test.go`:
- Around line 21-127: Tests for workspace-specific hint text use loose substring
checks; tighten them by asserting exact or canonical values (compare against
localInitHint and agentBindHint) and add the missing negative assertions so
local workspaces never contain agent hints and agent workspaces never contain
local hints. Update tests referencing NotConfiguredError, NoActiveProfileError,
and reconfigureHint to (1) directly compare cfgErr.Hint or reconfigureHint()
result to localInitHint when SetCurrentWorkspace(WorkspaceLocal) is used, (2)
directly compare to agentBindHint when using WorkspaceOpenClaw/WorkspaceHermes,
and (3) add explicit negative checks that local hints do not contain
agentBindHint and agent hints do not contain localInitHint.
---
Outside diff comments:
In `@cmd/config/bind_test.go`:
- Around line 152-160: The helper clearAgentEnv currently clears only a subset
of workspace env vars; update it to wipe every workspace/signal variable that
DetectWorkspaceFromEnv checks so tests truly simulate "no signals". Add
OPENCLAW_SERVICE_MARKER, OPENCLAW_SERVICE_VERSION, OPENCLAW_GATEWAY_PORT,
OPENCLAW_SHELL and any remaining Hermes variables referenced by
DetectWorkspaceFromEnv to the slice used in clearAgentEnv (the function name to
update), using t.Setenv(k, "") as done now so all detection branches are
neutralized.
In `@cmd/config/strict_mode.go`:
- Around line 101-149: The code currently computes oldEffective via
resolveStrictModeStatus before mutating multi/app and then warns if oldEffective
was bot and the new mode is user/off, which can produce false warnings when a
profile has its own override; update setStrictMode to compute the new effective
mode after persisting the change (call resolveStrictModeStatus again with the
mutated multi and app) and only emit the identity-escalation warning if the new
effective mode is different (oldEffective == StrictModeBot && newEffective !=
StrictModeBot), or alternately skip the warning entirely when the target profile
has a non-nil app.StrictMode override; reference setStrictMode,
resolveStrictModeStatus, multi, app and app.StrictMode to locate and implement
the fix.
---
Nitpick comments:
In `@cmd/auth/login.go`:
- Around line 51-55: Replace the inline strict-mode branch with the shared guard
by calling Factory.CheckStrictMode so the error shape stays consistent;
specifically, remove the if block that uses f.ResolveStrictMode and instead
invoke f.CheckStrictMode(cmd.Context(), core.StrictModeBot) (the CheckStrictMode
helper in internal/cmdutil/factory.go) to perform the validation and return its
result.
🪄 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: 348235ed-53e4-400c-a096-d6728099d28f
📒 Files selected for processing (25)
cmd/auth/list.gocmd/auth/login.gocmd/config/bind.gocmd/config/bind_test.gocmd/config/bind_warning_test.gocmd/config/config_test.gocmd/config/default_as.gocmd/config/init.gocmd/config/init_guard_test.gocmd/config/show.gocmd/config/strict_mode.gocmd/config/strict_mode_warning_test.gocmd/doctor/doctor.gocmd/profile/remove.gocmd/profile/rename.gocmd/profile/use.gocmd/prune.gocmd/root_integration_test.gointernal/cmdutil/factory.gointernal/core/config.gointernal/core/notconfigured.gointernal/core/notconfigured_test.gointernal/core/secret_resolve.gointernal/credential/credential_provider.gointernal/credential/default_provider.go
Comment on lines
+18
to
+41
| 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() |
There was a problem hiding this comment.
Call the shared env scrubber before binding.
This helper only sets HERMES_HOME; a leftover OPENCLAW_* signal from the host shell can still make the command auto-detect OpenClaw and bypass the Hermes warning path. Call clearAgentEnv(t) first.
♻️ Proposed fix
func runHermesBindWithIdentity(t *testing.T, identity string) string {
t.Helper()
saveWorkspace(t)
+ clearAgentEnv(t)
configDir := t.TempDir()🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@cmd/config/bind_warning_test.go` around lines 18 - 41, The helper
runHermesBindWithIdentity should call the shared environment scrubber to remove
any leftover agent env vars before setting HERMES_HOME; insert a call to
clearAgentEnv(t) at the start of runHermesBindWithIdentity so OPENCLAW_* (and
related) variables are cleared prior to running configBindRun, ensuring the
Hermes warning path is not bypassed.
Comment on lines
+93
to
102
| 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) | ||
| } |
There was a problem hiding this comment.
Cover the empty-config case too.
This test only exercises the missing-file path. Please add an empty config.json fixture so the new not-configured branch in configShowRun stays covered.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@cmd/config/config_test.go` around lines 93 - 102, Add a new test case that
covers the "empty config" path by creating an empty config.json fixture and
invoking the same code path exercised by the existing test (the caller that
triggers configShowRun); assert that the returned error is still a
*core.ConfigError with Code == output.ExitValidation and Type == "config" and
Message == "not configured" (use the same cfgErr variable checks as in the
current test). Place the empty fixture alongside the existing test fixtures and
add a subtest or additional test function that points the code under test to the
empty file so the new "not-configured" branch in configShowRun is covered.
Comment on lines
+14
to
+69
| func TestGuardAgentWorkspace_LocalAllows(t *testing.T) { | ||
| t.Setenv("OPENCLAW_HOME", "") | ||
| t.Setenv("OPENCLAW_CLI", "") | ||
| t.Setenv("HERMES_HOME", "") | ||
|
|
||
| if err := guardAgentWorkspace(&ConfigInitOptions{}); err != nil { | ||
| t.Errorf("local workspace should allow init, got: %v", err) | ||
| } | ||
| } | ||
|
|
||
| func TestGuardAgentWorkspace_OpenClawRefuses(t *testing.T) { | ||
| t.Setenv("OPENCLAW_HOME", t.TempDir()) | ||
|
|
||
| err := guardAgentWorkspace(&ConfigInitOptions{}) | ||
| if err == nil { | ||
| t.Fatal("expected refusal in OpenClaw context, got nil") | ||
| } | ||
| var cfgErr *core.ConfigError | ||
| if !errors.As(err, &cfgErr) { | ||
| t.Fatalf("error type = %T, want *core.ConfigError", err) | ||
| } | ||
| if cfgErr.Type != "openclaw" { | ||
| t.Errorf("type = %q, want %q", cfgErr.Type, "openclaw") | ||
| } | ||
| if !strings.Contains(cfgErr.Hint, "config bind --help") { | ||
| t.Errorf("hint must point to config bind --help; got %q", cfgErr.Hint) | ||
| } | ||
| if !strings.Contains(cfgErr.Hint, "--force-init") { | ||
| t.Errorf("hint must mention --force-init escape hatch; got %q", cfgErr.Hint) | ||
| } | ||
| } | ||
|
|
||
| func TestGuardAgentWorkspace_HermesRefuses(t *testing.T) { | ||
| t.Setenv("HERMES_HOME", t.TempDir()) | ||
|
|
||
| err := guardAgentWorkspace(&ConfigInitOptions{}) | ||
| if err == nil { | ||
| t.Fatal("expected refusal in Hermes context, got nil") | ||
| } | ||
| var cfgErr *core.ConfigError | ||
| if !errors.As(err, &cfgErr) { | ||
| t.Fatalf("error type = %T, want *core.ConfigError", err) | ||
| } | ||
| if cfgErr.Type != "hermes" { | ||
| t.Errorf("type = %q, want %q", cfgErr.Type, "hermes") | ||
| } | ||
| } | ||
|
|
||
| func TestGuardAgentWorkspace_ForceInitOverride(t *testing.T) { | ||
| t.Setenv("OPENCLAW_HOME", t.TempDir()) | ||
|
|
||
| // --force-init must let the user proceed even inside an Agent context. | ||
| if err := guardAgentWorkspace(&ConfigInitOptions{ForceInit: true}); err != nil { | ||
| t.Errorf("--force-init should bypass the guard, got: %v", err) | ||
| } | ||
| } |
There was a problem hiding this comment.
Clear the full workspace signal set in the guard tests.
These tests only toggle OPENCLAW_HOME / HERMES_HOME, but DetectWorkspaceFromEnv also inspects several other OpenClaw and Hermes env vars. Reuse clearAgentEnv(t) here so a leftover host signal can’t flip the detected workspace and make the guard assertions flaky.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@cmd/config/init_guard_test.go` around lines 14 - 69, The tests for
guardAgentWorkspace are flaky because they only set OPENCLAW_HOME/HERMES_HOME
while DetectWorkspaceFromEnv also checks other env vars; update each test
(TestGuardAgentWorkspace_LocalAllows, TestGuardAgentWorkspace_OpenClawRefuses,
TestGuardAgentWorkspace_HermesRefuses,
TestGuardAgentWorkspace_ForceInitOverride) to call clearAgentEnv(t) at the start
to clear the full workspace signal set before setting the specific env var(s),
so guardAgentWorkspace(&ConfigInitOptions{...}) uses a clean env; reference
clearAgentEnv(t), guardAgentWorkspace, ConfigInitOptions and
DetectWorkspaceFromEnv when making the change.
Comment on lines
226
to
229
| raw, err := LoadMultiAppConfig() | ||
| if err != nil || raw == nil || len(raw.Apps) == 0 { | ||
| return nil, &ConfigError{Code: 2, Type: "config", Message: "not configured", Hint: "run `lark-cli config init --new` in the background. It blocks and outputs a verification URL — retrieve the URL and open it in a browser to complete setup."} | ||
| return nil, NotConfiguredError() | ||
| } |
There was a problem hiding this comment.
Don't collapse real config-load failures into "not configured".
LoadMultiAppConfig() can fail for malformed JSON or I/O problems. Returning NotConfiguredError() here hides the real cause from auth/profile callers and makes broken configs look like missing bindings. Please only map the missing/empty-config cases to the workspace-aware error.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@internal/core/config.go` around lines 226 - 229, The current code collapses
real LoadMultiAppConfig() failures into NotConfiguredError(); change the logic
in the function that calls LoadMultiAppConfig() so that if LoadMultiAppConfig()
returns a non-nil err you propagate/return that err directly (or wrap it)
instead of mapping it to NotConfiguredError(), and only return
NotConfiguredError() when err == nil and (raw == nil || len(raw.Apps) == 0);
refer to LoadMultiAppConfig(), raw and raw.Apps and NotConfiguredError() to
locate the conditional to adjust.
There was a problem hiding this comment.
Actionable comments posted: 8
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
cmd/config/config_test.go (1)
85-102:⚠️ Potential issue | 🟠 Major | ⚡ Quick winMake the local-workspace expectation deterministic in this test.
This test hard-asserts local
Type/Messagevalues, but it doesn’t clear workspace env markers. IfOPENCLAW_HOME/HERMES_HOMEis present in CI, assertions can fail for the wrong reason.Suggested fix
func TestConfigShowRun_NotConfiguredReturnsStructuredError(t *testing.T) { t.Setenv("LARKSUITE_CLI_CONFIG_DIR", t.TempDir()) + t.Setenv("OPENCLAW_HOME", "") + t.Setenv("HERMES_HOME", "") f, _, _, _ := cmdutil.TestFactory(t, nil)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/config/config_test.go` around lines 85 - 102, The test relies on local workspace detection and must clear any CI env markers; before calling TestFactory/configShowRun, unset workspace-related env vars by using t.Setenv for "OPENCLAW_HOME" and "HERMES_HOME" (set them to empty) so the local-workspace path detection is deterministic, then proceed with ConfigShowOptions{Factory: f} and the existing assertions against configShowRun; this ensures the error produced is the expected core.ConfigError with Type "config" and Message "not configured".
♻️ Duplicate comments (1)
internal/core/config.go (1)
226-229:⚠️ Potential issue | 🟠 Major | ⚡ Quick winDon't collapse real config-load failures into "not configured".
LoadMultiAppConfig()can fail for reasons other than a missing config file (malformed JSON, permission errors, I/O issues). Mapping all errors toNotConfiguredError()here hides the real cause from callers and makes broken configs look like missing bindings.Only map the missing-config or empty-config cases to the workspace-aware error:
🛠️ Proposed fix
func RequireConfigForProfile(kc keychain.KeychainAccess, profileOverride string) (*CliConfig, error) { raw, err := LoadMultiAppConfig() - if err != nil || raw == nil || len(raw.Apps) == 0 { + if err != nil { + // Only treat "file not found" or "no apps" as "not configured"; + // propagate parse/permission errors so the real issue surfaces. + if errors.Is(err, os.ErrNotExist) || strings.Contains(err.Error(), "no apps") { + return nil, NotConfiguredError() + } + return nil, err + } + if raw == nil || len(raw.Apps) == 0 { return nil, NotConfiguredError() } return ResolveConfigFromMulti(raw, kc, profileOverride) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/core/config.go` around lines 226 - 229, Currently the code maps any error from LoadMultiAppConfig() to NotConfiguredError(); change the logic in the initializer so that if LoadMultiAppConfig() returns a non-nil err you propagate that err (return err) immediately, and only if err == nil but raw == nil or len(raw.Apps) == 0 do you return NotConfiguredError(); update the block around the LoadMultiAppConfig() call (variables raw, err and the NotConfiguredError() return) to implement this ordering so real I/O/parse errors are preserved while only missing/empty configs become NotConfiguredError().
🧹 Nitpick comments (1)
cmd/config/strict_mode_warning_test.go (1)
38-50: ⚡ Quick winAssert the
⚠️warning prefix in positive cases.At Line 38 and Line 48, the tests validate message content but not the required warning prefix. Add a prefix assertion so formatting regressions are caught.
Proposed test hardening
out := runStrictMode(t, "user") if !strings.Contains(out, bindMsgZh.IdentityEscalationMessage) { t.Errorf("bot→user transition must surface IdentityEscalationMessage; got: %s", out) } +if !strings.Contains(out, "⚠️ "+bindMsgZh.IdentityEscalationMessage) { + t.Errorf("bot→user transition must include warning prefix; got: %s", out) +}out := runStrictMode(t, "off") if !strings.Contains(out, bindMsgZh.IdentityEscalationMessage) { t.Errorf("bot→off transition must surface IdentityEscalationMessage; got: %s", out) } +if !strings.Contains(out, "⚠️ "+bindMsgZh.IdentityEscalationMessage) { + t.Errorf("bot→off transition must include warning prefix; got: %s", out) +}As per coding guidelines: "Every behavior change must have an accompanying test".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/config/strict_mode_warning_test.go` around lines 38 - 50, Update the tests to assert the required warning emoji prefix is present: in TestStrictMode_BotToOff_WarnsAboutIdentityRisk (and the analogous bot→user test) check that the output contains the prefixed string "⚠️ " + bindMsgZh.IdentityEscalationMessage rather than only bindMsgZh.IdentityEscalationMessage; use runStrictMode to get out and assert strings.Contains(out, "⚠️ "+bindMsgZh.IdentityEscalationMessage) so formatting regressions are caught while leaving existing message content checks intact.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@cmd/auth/list.go`:
- Around line 43-49: LoadMultiAppConfig errors are being ignored causing
malformed/permission errors to be masked and incorrectly returning
core.NotConfiguredError(); update the cmd/auth/list.go flow to capture and check
the error returned by core.LoadMultiAppConfig(), and only return
core.NotConfiguredError() when the call produced no error but returned a
nil/empty multi (i.e., config truly missing); otherwise propagate or wrap the
actual error (include structured context) so callers see parse/permission
failures instead of a misleading NotConfiguredError.
In `@cmd/config/default_as.go`:
- Around line 23-26: The call to core.LoadMultiAppConfig() in
cmd/config/default_as.go currently maps every error to
core.NotConfiguredError(), hiding I/O or JSON parsing problems; update the code
around LoadMultiAppConfig() to inspect the returned error (e.g., check for a
specific "not found"/empty-config sentinel or use errors.Is/errors.As to detect
os.IsNotExist) and only return core.NotConfiguredError() when the config is
truly missing/empty, otherwise return or wrap the original error so malformed/IO
errors surface; reference the LoadMultiAppConfig() call and the
NotConfiguredError() usage to locate and change the handling logic.
In `@cmd/profile/remove.go`:
- Line 37: cmd/profile/remove.go now returns core.NotConfiguredError() on config
load failure; add a unit test that triggers the config-load failure and asserts
the command returns that specific error. Create a test (e.g.,
TestRemove_NotConfiguredError) that invokes the remove command entrypoint from
cmd/profile/remove.go (or the exported function that calls core.LoadConfig)
while arranging the environment or stubbing/mocking core.LoadConfig to return an
error, then assert the returned error is core.NotConfiguredError(); ensure the
test is placed alongside existing profile command tests and only exercises this
new branch.
In `@cmd/profile/rename.go`:
- Line 37: The new branch returns core.NotConfiguredError() from the call path
that invokes LoadMultiAppConfig; add a unit test that simulates
LoadMultiAppConfig failing and asserts the returned error is a *core.ConfigError
(and equals core.NotConfiguredError()) — locate the call site in
cmd/profile/rename.go (the function that returns core.NotConfiguredError()) and
write a test that injects/mocks LoadMultiAppConfig to return the not-configured
error, then call the Rename command entrypoint and assert the error type via a
type assertion to *core.ConfigError and/or compare using errors.Is to
core.NotConfiguredError(); name the test something like
TestRename_NotConfigured_ReturnsConfigError and keep it focused on the error
shape.
In `@cmd/profile/use.go`:
- Line 36: Add a unit test that verifies the "profile use" command returns
core.NotConfiguredError when config loading fails: create a test (e.g.,
cmd/profile/use_test.go -> TestProfileUse_ConfigLoadNotConfiguredError) that
injects/mocks the config loader to return an error, runs the profile use command
entrypoint (the command's Run/Execute handler in cmd/profile/use.go), and
asserts the returned error equals core.NotConfiguredError and that any
returned/generic guidance includes the workspace-aware message; ensure the test
isolates the config loader dependency so this behavior cannot regress.
In `@internal/core/notconfigured.go`:
- Around line 56-60: Update the local guidance returned by reconfigureHint so it
matches the canonical local hint: when CurrentWorkspace().IsLocal() is true,
return the command using the --new flag (i.e., change the string "lark-cli
config init" to "lark-cli config init --new") so the reconfigureHint output is
consistent with the module’s canonical local init guidance; locate this logic in
the reconfigureHint function and adjust only the local branch, leaving the
non-local return of agentBindHint unchanged.
In `@internal/credential/credential_provider.go`:
- Line 206: Add a unit test that triggers the fallback branch in
internal/credential/credential_provider.go where the function returns
core.NotConfiguredError() when no source resolves an account: instantiate the
CredentialProvider (or the function under test that iterates sources), supply
mock/empty credential sources that fail to resolve (e.g., return nil/no-account
and no other error), call the resolver method, and assert the returned error is
exactly core.NotConfiguredError (or has that sentinel/type) and that no account
is returned; name the test to indicate it covers the "NotConfiguredError"
fallback so future changes cannot regress this behavior.
In `@internal/credential/default_provider.go`:
- Line 39: Add a unit test for ResolveAccount that simulates failure to load the
multi-app config and asserts the returned error is classified as
core.NotConfiguredError(); specifically, arrange the test to force the multi-app
config loader to fail (mock/stub the loader or provide an environment where it
cannot load), call ResolveAccount, and assert the error returned matches or is
recognized as core.NotConfiguredError() (use errors.Is or the project's error
comparison helper) so the behavior change at ResolveAccount returning
core.NotConfiguredError() is covered.
---
Outside diff comments:
In `@cmd/config/config_test.go`:
- Around line 85-102: The test relies on local workspace detection and must
clear any CI env markers; before calling TestFactory/configShowRun, unset
workspace-related env vars by using t.Setenv for "OPENCLAW_HOME" and
"HERMES_HOME" (set them to empty) so the local-workspace path detection is
deterministic, then proceed with ConfigShowOptions{Factory: f} and the existing
assertions against configShowRun; this ensures the error produced is the
expected core.ConfigError with Type "config" and Message "not configured".
---
Duplicate comments:
In `@internal/core/config.go`:
- Around line 226-229: Currently the code maps any error from
LoadMultiAppConfig() to NotConfiguredError(); change the logic in the
initializer so that if LoadMultiAppConfig() returns a non-nil err you propagate
that err (return err) immediately, and only if err == nil but raw == nil or
len(raw.Apps) == 0 do you return NotConfiguredError(); update the block around
the LoadMultiAppConfig() call (variables raw, err and the NotConfiguredError()
return) to implement this ordering so real I/O/parse errors are preserved while
only missing/empty configs become NotConfiguredError().
---
Nitpick comments:
In `@cmd/config/strict_mode_warning_test.go`:
- Around line 38-50: Update the tests to assert the required warning emoji
prefix is present: in TestStrictMode_BotToOff_WarnsAboutIdentityRisk (and the
analogous bot→user test) check that the output contains the prefixed string "⚠️
" + bindMsgZh.IdentityEscalationMessage rather than only
bindMsgZh.IdentityEscalationMessage; use runStrictMode to get out and assert
strings.Contains(out, "⚠️ "+bindMsgZh.IdentityEscalationMessage) so formatting
regressions are caught while leaving existing message content checks intact.
🪄 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: c612a9a8-a372-40e4-8a41-30e152c3026f
📒 Files selected for processing (25)
cmd/auth/list.gocmd/auth/login.gocmd/config/bind.gocmd/config/bind_test.gocmd/config/bind_warning_test.gocmd/config/config_test.gocmd/config/default_as.gocmd/config/init.gocmd/config/init_guard_test.gocmd/config/show.gocmd/config/strict_mode.gocmd/config/strict_mode_warning_test.gocmd/doctor/doctor.gocmd/profile/remove.gocmd/profile/rename.gocmd/profile/use.gocmd/prune.gocmd/root_integration_test.gointernal/cmdutil/factory.gointernal/core/config.gointernal/core/notconfigured.gointernal/core/notconfigured_test.gointernal/core/secret_resolve.gointernal/credential/credential_provider.gointernal/credential/default_provider.go
✅ Files skipped from review due to trivial changes (1)
- internal/core/notconfigured_test.go
🚧 Files skipped from review as they are similar to previous changes (5)
- internal/core/secret_resolve.go
- cmd/auth/login.go
- cmd/config/init_guard_test.go
- cmd/config/bind_warning_test.go
- cmd/root_integration_test.go
Comment on lines
43
to
49
| multi, _ := core.LoadMultiAppConfig() | ||
| if multi == nil || len(multi.Apps) == 0 { | ||
| fmt.Fprintln(f.IOStreams.ErrOut, "Not configured yet. Run `lark-cli config init` to initialize.") | ||
| return nil | ||
| // Surface as a structured error so the workspace-aware hint | ||
| // (config init in local; config bind --help in agent ws) is | ||
| // picked up by the standard error pathway. | ||
| return core.NotConfiguredError() | ||
| } |
There was a problem hiding this comment.
Don’t swallow config-load failures before returning “not configured”.
At Line 43, LoadMultiAppConfig errors are discarded, so Line 48 can incorrectly return NotConfiguredError even for malformed JSON or permission errors. That hides the real remediation path.
Suggested fix
import (
+ "errors"
"fmt"
+ "os"
@@
func authListRun(opts *ListOptions) error {
f := opts.Factory
- multi, _ := core.LoadMultiAppConfig()
+ multi, err := core.LoadMultiAppConfig()
+ if err != nil {
+ if errors.Is(err, os.ErrNotExist) {
+ return core.NotConfiguredError()
+ }
+ return output.Errorf(output.ExitValidation, "config", "failed to load config: %v", err)
+ }
if multi == nil || len(multi.Apps) == 0 {
// Surface as a structured error so the workspace-aware hint
// (config init in local; config bind --help in agent ws) is
// picked up by the standard error pathway.
return core.NotConfiguredError()
}As per coding guidelines: Make error messages structured, actionable, and specific for AI agent parsing.
📝 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
| multi, _ := core.LoadMultiAppConfig() | |
| if multi == nil || len(multi.Apps) == 0 { | |
| fmt.Fprintln(f.IOStreams.ErrOut, "Not configured yet. Run `lark-cli config init` to initialize.") | |
| return nil | |
| // Surface as a structured error so the workspace-aware hint | |
| // (config init in local; config bind --help in agent ws) is | |
| // picked up by the standard error pathway. | |
| return core.NotConfiguredError() | |
| } | |
| multi, err := core.LoadMultiAppConfig() | |
| if err != nil { | |
| if errors.Is(err, os.ErrNotExist) { | |
| return core.NotConfiguredError() | |
| } | |
| return output.Errorf(output.ExitValidation, "config", "failed to load config: %v", err) | |
| } | |
| if multi == nil || len(multi.Apps) == 0 { | |
| // Surface as a structured error so the workspace-aware hint | |
| // (config init in local; config bind --help in agent ws) is | |
| // picked up by the standard error pathway. | |
| return core.NotConfiguredError() | |
| } |
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 48-48: cmd/auth/list.go#L48
Added line #L48 was not covered by tests
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@cmd/auth/list.go` around lines 43 - 49, LoadMultiAppConfig errors are being
ignored causing malformed/permission errors to be masked and incorrectly
returning core.NotConfiguredError(); update the cmd/auth/list.go flow to capture
and check the error returned by core.LoadMultiAppConfig(), and only return
core.NotConfiguredError() when the call produced no error but returned a
nil/empty multi (i.e., config truly missing); otherwise propagate or wrap the
actual error (include structured context) so callers see parse/permission
failures instead of a misleading NotConfiguredError.
| multi, err := core.LoadMultiAppConfig() | ||
| if err != nil { | ||
| return output.ErrWithHint(output.ExitValidation, "config", "not configured", "run: lark-cli config init") | ||
| return core.NotConfiguredError() |
There was a problem hiding this comment.
Please add coverage for the new not-configured return path.
Line 37 now returns core.NotConfiguredError() on config load failure; add a targeted test for this branch to lock in the new workspace-aware behavior.
As per coding guidelines: Every behavior change must have an accompanying test.
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 37-37: cmd/profile/remove.go#L37
Added line #L37 was not covered by tests
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@cmd/profile/remove.go` at line 37, cmd/profile/remove.go now returns
core.NotConfiguredError() on config load failure; add a unit test that triggers
the config-load failure and asserts the command returns that specific error.
Create a test (e.g., TestRemove_NotConfiguredError) that invokes the remove
command entrypoint from cmd/profile/remove.go (or the exported function that
calls core.LoadConfig) while arranging the environment or stubbing/mocking
core.LoadConfig to return an error, then assert the returned error is
core.NotConfiguredError(); ensure the test is placed alongside existing profile
command tests and only exercises this new branch.
| multi, err := core.LoadMultiAppConfig() | ||
| if err != nil { | ||
| return output.ErrWithHint(output.ExitValidation, "config", "not configured", "run: lark-cli config init") | ||
| return core.NotConfiguredError() |
There was a problem hiding this comment.
Add a regression test for the new “not configured” branch.
Line 37 changes the error contract to core.NotConfiguredError(), but this new path is currently untested in patch coverage. Please add a unit test that asserts the returned error shape (*core.ConfigError) for the LoadMultiAppConfig failure case.
As per coding guidelines: Every behavior change must have an accompanying test.
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 37-37: cmd/profile/rename.go#L37
Added line #L37 was not covered by tests
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@cmd/profile/rename.go` at line 37, The new branch returns
core.NotConfiguredError() from the call path that invokes LoadMultiAppConfig;
add a unit test that simulates LoadMultiAppConfig failing and asserts the
returned error is a *core.ConfigError (and equals core.NotConfiguredError()) —
locate the call site in cmd/profile/rename.go (the function that returns
core.NotConfiguredError()) and write a test that injects/mocks
LoadMultiAppConfig to return the not-configured error, then call the Rename
command entrypoint and assert the error type via a type assertion to
*core.ConfigError and/or compare using errors.Is to core.NotConfiguredError();
name the test something like TestRename_NotConfigured_ReturnsConfigError and
keep it focused on the error shape.
| multi, err := core.LoadMultiAppConfig() | ||
| if err != nil { | ||
| return output.ErrWithHint(output.ExitValidation, "config", "not configured", "run: lark-cli config init") | ||
| return core.NotConfiguredError() |
There was a problem hiding this comment.
Add a test for profile use config-load failure returning NotConfiguredError.
Line 36 changed behavior to the centralized not-configured error path; please add a focused test to prevent regressions in this command’s workspace-aware guidance.
As per coding guidelines: Every behavior change must have an accompanying test.
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 36-36: cmd/profile/use.go#L36
Added line #L36 was not covered by tests
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@cmd/profile/use.go` at line 36, Add a unit test that verifies the "profile
use" command returns core.NotConfiguredError when config loading fails: create a
test (e.g., cmd/profile/use_test.go ->
TestProfileUse_ConfigLoadNotConfiguredError) that injects/mocks the config
loader to return an error, runs the profile use command entrypoint (the
command's Run/Execute handler in cmd/profile/use.go), and asserts the returned
error equals core.NotConfiguredError and that any returned/generic guidance
includes the workspace-aware message; ensure the test isolates the config loader
dependency so this behavior cannot regress.
Comment on lines
+56
to
+60
| func reconfigureHint() string { | ||
| if CurrentWorkspace().IsLocal() { | ||
| return "please run `lark-cli config init` to reconfigure" | ||
| } | ||
| return agentBindHint |
There was a problem hiding this comment.
Align local reconfigureHint command with the canonical local init guidance.
At Line 58, local guidance uses config init, while the module’s canonical local hint uses config init --new. Keeping one command shape avoids conflicting remediation hints in AI-parsed error flows.
Proposed consistency fix
func reconfigureHint() string {
if CurrentWorkspace().IsLocal() {
- return "please run `lark-cli config init` to reconfigure"
+ return "please run `lark-cli config init --new` to reconfigure"
}
return agentBindHint
}As per coding guidelines: "Make error messages structured, actionable, and specific for AI agent parsing".
📝 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 reconfigureHint() string { | |
| if CurrentWorkspace().IsLocal() { | |
| return "please run `lark-cli config init` to reconfigure" | |
| } | |
| return agentBindHint | |
| func reconfigureHint() string { | |
| if CurrentWorkspace().IsLocal() { | |
| return "please run `lark-cli config init --new` to reconfigure" | |
| } | |
| return agentBindHint | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@internal/core/notconfigured.go` around lines 56 - 60, Update the local
guidance returned by reconfigureHint so it matches the canonical local hint:
when CurrentWorkspace().IsLocal() is true, return the command using the --new
flag (i.e., change the string "lark-cli config init" to "lark-cli config init
--new") so the reconfigureHint output is consistent with the module’s canonical
local init guidance; locate this logic in the reconfigureHint function and
adjust only the local branch, leaving the non-local return of agentBindHint
unchanged.
| return acct, nil | ||
| } | ||
| return nil, fmt.Errorf("no credential provider returned an account; run 'lark-cli config' to set up") | ||
| return nil, core.NotConfiguredError() |
There was a problem hiding this comment.
Cover the new NotConfiguredError fallback with a unit test.
Line 206 now returns core.NotConfiguredError() when no source resolves an account; this branch should be explicitly tested to prevent silent regressions in credential error classification.
As per coding guidelines: Every behavior change must have an accompanying test.
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 206-206: internal/credential/credential_provider.go#L206
Added line #L206 was not covered by tests
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@internal/credential/credential_provider.go` at line 206, Add a unit test that
triggers the fallback branch in internal/credential/credential_provider.go where
the function returns core.NotConfiguredError() when no source resolves an
account: instantiate the CredentialProvider (or the function under test that
iterates sources), supply mock/empty credential sources that fail to resolve
(e.g., return nil/no-account and no other error), call the resolver method, and
assert the returned error is exactly core.NotConfiguredError (or has that
sentinel/type) and that no account is returned; name the test to indicate it
covers the "NotConfiguredError" fallback so future changes cannot regress this
behavior.
429060e to
7aa911a
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (5)
internal/core/config.go (1)
226-229:⚠️ Potential issue | 🟠 Major | ⚡ Quick winDon't collapse all load failures into not-configured.
This keeps the workspace-aware hint for missing config, but it also turns malformed JSON / I/O errors into the same response. That weakens diagnostics for every caller of
RequireConfigForProfile.If empty configs should still count as not configured, that case needs to be split out before returning here.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/core/config.go` around lines 226 - 229, The code currently collapses all LoadMultiAppConfig failures into NotConfiguredError; change RequireConfigForProfile so that if LoadMultiAppConfig returns a non-nil err you return that err directly, and only treat the case where err == nil but raw == nil || len(raw.Apps) == 0 as NotConfiguredError(); update the block around LoadMultiAppConfig (referencing LoadMultiAppConfig, RequireConfigForProfile, and NotConfiguredError) to first check err and return it, then check for empty/missing apps and return NotConfiguredError.internal/credential/default_provider.go (1)
35-40:⚠️ Potential issue | 🟠 Major | ⚡ Quick winPreserve the original config-load failure here.
LoadMultiAppConfig()can fail for malformed JSON or I/O problems. Returningcore.NotConfiguredError()for every failure makes a broken config look identical to a missing one, which hides the actionable error fromauth loginand other callers.If the empty-app case should still be workspace-aware, that needs to be split out before this return.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/credential/default_provider.go` around lines 35 - 40, The current ResolveAccount method in DefaultAccountProvider replaces any LoadMultiAppConfig() failure with core.NotConfiguredError(), which hides real JSON/I/O errors; update DefaultAccountProvider.ResolveAccount to return the original err when core.LoadMultiAppConfig() fails (i.e., return nil, err) so callers see actionable errors from core.LoadMultiAppConfig(), and if you still need special handling for an empty-app/workspace-aware case, detect and handle the empty-app scenario before converting or mapping errors to core.NotConfiguredError().internal/core/notconfigured.go (1)
56-60:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winReuse the canonical local init hint here.
reconfigureHint()still returns the oldconfig initwording on the local branch, which diverges fromlocalInitHintand will send callers back to the stale reconfigure guidance.♻️ Proposed fix
func reconfigureHint() string { if CurrentWorkspace().IsLocal() { - return "please run `lark-cli config init` to reconfigure" + return localInitHint } return agentBindHint }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/core/notconfigured.go` around lines 56 - 60, reconfigureHint() currently returns a hardcoded "lark-cli config init" message for local workspaces; update it to return the canonical localInitHint instead so callers get the consistent guidance. In the reconfigureHint function, replace the string returned when CurrentWorkspace().IsLocal() with localInitHint and keep returning agentBindHint otherwise, referencing reconfigureHint, CurrentWorkspace().IsLocal(), and localInitHint to locate the change.internal/core/notconfigured_test.go (1)
109-116:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winPin the local reconfigure hint to the canonical string.
strings.Contains(got, "config init")still passes if the helper returns the stale local wording, so this test won’t catch the regression inreconfigureHint().♻️ Proposed fix
func TestReconfigureHint_Local(t *testing.T) { saveAndRestoreWorkspace(t) SetCurrentWorkspace(WorkspaceLocal) got := reconfigureHint() - if !strings.Contains(got, "config init") { - t.Errorf("local reconfigure hint must mention config init; got %q", got) + if got != localInitHint { + t.Errorf("local reconfigure hint = %q, want %q", got, localInitHint) } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/core/notconfigured_test.go` around lines 109 - 116, The test TestReconfigureHint_Local should assert the canonical reconfigure hint exactly instead of using strings.Contains; update the assertion in TestReconfigureHint_Local to compare the returned value from reconfigureHint() to the exact expected string (or a shared canonical constant) so stale/local phrasing regressions are caught—locate the test and replace the contains check with an equality check against the canonical hint for WorkspaceLocal, referencing reconfigureHint() and the expected string/constant.cmd/config/bind_warning_test.go (1)
18-41:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winCall the shared env scrubber before binding.
runHermesBindWithIdentitystill inherits anyOPENCLAW_*signals from the host shell, which can reroute auto-detection to OpenClaw and bypass the Hermes warning path this test is meant to exercise.♻️ Proposed fix
func runHermesBindWithIdentity(t *testing.T, identity string) string { t.Helper() saveWorkspace(t) + clearAgentEnv(t) configDir := t.TempDir() t.Setenv("LARKSUITE_CLI_CONFIG_DIR", configDir)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/config/bind_warning_test.go` around lines 18 - 41, runHermesBindWithIdentity inherits OPENCLAW_* env vars from the host which can bypass the Hermes warning path; before calling configBindRun in runHermesBindWithIdentity, invoke the shared test environment scrubber to remove OPENCLAW_* signals (use the existing helper used elsewhere in tests) so the call to configBindRun(&BindOptions{...}) runs with a clean env and triggers the Hermes warning path.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@cmd/config/show.go`:
- Around line 44-53: LoadMultiAppConfig currently turns an empty Apps list into
a generic error so the local check (config == nil || len(config.Apps) == 0)
never runs; change the contract so the loader returns a sentinel error (e.g.
define core.ErrNoApps or core.ErrEmptyConfig) when Apps is empty, then in
cmd/config/show.go update the error handling around core.LoadMultiAppConfig() to
detect errors.Is(err, core.ErrNoApps) (or use a new
core.IsNotConfiguredError(err) helper) and return core.NotConfiguredError();
remove the redundant post-load nil/len check after this change.
---
Duplicate comments:
In `@cmd/config/bind_warning_test.go`:
- Around line 18-41: runHermesBindWithIdentity inherits OPENCLAW_* env vars from
the host which can bypass the Hermes warning path; before calling configBindRun
in runHermesBindWithIdentity, invoke the shared test environment scrubber to
remove OPENCLAW_* signals (use the existing helper used elsewhere in tests) so
the call to configBindRun(&BindOptions{...}) runs with a clean env and triggers
the Hermes warning path.
In `@internal/core/config.go`:
- Around line 226-229: The code currently collapses all LoadMultiAppConfig
failures into NotConfiguredError; change RequireConfigForProfile so that if
LoadMultiAppConfig returns a non-nil err you return that err directly, and only
treat the case where err == nil but raw == nil || len(raw.Apps) == 0 as
NotConfiguredError(); update the block around LoadMultiAppConfig (referencing
LoadMultiAppConfig, RequireConfigForProfile, and NotConfiguredError) to first
check err and return it, then check for empty/missing apps and return
NotConfiguredError.
In `@internal/core/notconfigured_test.go`:
- Around line 109-116: The test TestReconfigureHint_Local should assert the
canonical reconfigure hint exactly instead of using strings.Contains; update the
assertion in TestReconfigureHint_Local to compare the returned value from
reconfigureHint() to the exact expected string (or a shared canonical constant)
so stale/local phrasing regressions are caught—locate the test and replace the
contains check with an equality check against the canonical hint for
WorkspaceLocal, referencing reconfigureHint() and the expected string/constant.
In `@internal/core/notconfigured.go`:
- Around line 56-60: reconfigureHint() currently returns a hardcoded "lark-cli
config init" message for local workspaces; update it to return the canonical
localInitHint instead so callers get the consistent guidance. In the
reconfigureHint function, replace the string returned when
CurrentWorkspace().IsLocal() with localInitHint and keep returning agentBindHint
otherwise, referencing reconfigureHint, CurrentWorkspace().IsLocal(), and
localInitHint to locate the change.
In `@internal/credential/default_provider.go`:
- Around line 35-40: The current ResolveAccount method in DefaultAccountProvider
replaces any LoadMultiAppConfig() failure with core.NotConfiguredError(), which
hides real JSON/I/O errors; update DefaultAccountProvider.ResolveAccount to
return the original err when core.LoadMultiAppConfig() fails (i.e., return nil,
err) so callers see actionable errors from core.LoadMultiAppConfig(), and if you
still need special handling for an empty-app/workspace-aware case, detect and
handle the empty-app scenario before converting or mapping errors to
core.NotConfiguredError().
🪄 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: b66c05af-eed1-4574-a06c-0e8754c9c554
📒 Files selected for processing (26)
cmd/auth/list.gocmd/auth/login.gocmd/config/bind.gocmd/config/bind_test.gocmd/config/bind_warning_test.gocmd/config/config_test.gocmd/config/default_as.gocmd/config/init.gocmd/config/init_guard_test.gocmd/config/show.gocmd/config/strict_mode.gocmd/config/strict_mode_warning_test.gocmd/doctor/doctor.gocmd/profile/remove.gocmd/profile/rename.gocmd/profile/use.gocmd/prune.gocmd/root_integration_test.gointernal/cmdutil/factory.gointernal/core/config.gointernal/core/notconfigured.gointernal/core/notconfigured_test.gointernal/core/secret_resolve.gointernal/credential/credential_provider.gointernal/credential/default_provider.gotests/cli_e2e/config/bind_test.go
✅ Files skipped from review due to trivial changes (3)
- cmd/config/config_test.go
- cmd/config/init_guard_test.go
- cmd/config/strict_mode.go
🚧 Files skipped from review as they are similar to previous changes (8)
- cmd/profile/rename.go
- cmd/profile/remove.go
- cmd/auth/list.go
- cmd/config/default_as.go
- cmd/profile/use.go
- cmd/prune.go
- cmd/config/init.go
- cmd/root_integration_test.go
liangshuo-1
force-pushed
the
feat/agent-binding-hints
branch
from
7aa911a to
79087a1
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
cmd/auth/login.go (1)
246-263:⚠️ Potential issue | 🟠 Major | ⚡ Quick winEmit
AgentTimeoutHintfor JSON blocking flow too.In the current flow,
auth login --jsonstill blocks for polling, but the timeout/recovery hint is only printed in the non-JSON branch. That leaves agent callers without the guidance this PR is adding.Suggested patch
if opts.JSON { data := map[string]interface{}{ "event": "device_authorization", "verification_uri": authResp.VerificationUri, "verification_uri_complete": authResp.VerificationUriComplete, "user_code": authResp.UserCode, "expires_in": authResp.ExpiresIn, } encoder := json.NewEncoder(f.IOStreams.Out) encoder.SetEscapeHTML(false) if err := encoder.Encode(data); err != nil { return output.Errorf(output.ExitInternal, "internal", "failed to write JSON output: %v", err) } } else { fmt.Fprintf(f.IOStreams.ErrOut, msg.OpenURL) fmt.Fprintf(f.IOStreams.ErrOut, " %s\n\n", authResp.VerificationUriComplete) - fmt.Fprintln(f.IOStreams.ErrOut, msg.AgentTimeoutHint) } + fmt.Fprintln(f.IOStreams.ErrOut, msg.AgentTimeoutHint)As per coding guidelines: Design CLI flags, help text, and error messages with AI agent consumption in mind; every error message will be parsed by AI agents.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/auth/login.go` around lines 246 - 263, The JSON output branch for device authorization omits the AgentTimeoutHint, so agents using opts.JSON don't receive the timeout/recovery guidance; update the JSON branch in the login flow (where opts.JSON is checked and the map `data` is built from `authResp`) to include the `AgentTimeoutHint` (e.g., add an "agent_timeout_hint" or similar field containing `msg.AgentTimeoutHint` to the `data` map) before encoding with `json.NewEncoder(f.IOStreams.Out)` so JSON consumers get the same hint the non-JSON branch writes to `f.IOStreams.ErrOut`.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@cmd/config/strict_mode.go`:
- Around line 138-140: The warning is using the requested target `mode` instead
of the resolved effective mode after the mutation; recompute the effective mode
into a new variable (e.g. newEffective) after applying the profile/global change
and replace the condition so it compares oldEffective to newEffective (e.g. if
oldEffective == core.StrictModeBot && newEffective != core.StrictModeBot) before
printing the IdentityEscalationMessage, using the existing symbols oldEffective
and mode as context when locating the update in strict_mode.go.
---
Outside diff comments:
In `@cmd/auth/login.go`:
- Around line 246-263: The JSON output branch for device authorization omits the
AgentTimeoutHint, so agents using opts.JSON don't receive the timeout/recovery
guidance; update the JSON branch in the login flow (where opts.JSON is checked
and the map `data` is built from `authResp`) to include the `AgentTimeoutHint`
(e.g., add an "agent_timeout_hint" or similar field containing
`msg.AgentTimeoutHint` to the `data` map) before encoding with
`json.NewEncoder(f.IOStreams.Out)` so JSON consumers get the same hint the
non-JSON branch writes to `f.IOStreams.ErrOut`.
🪄 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: 87ebde17-7620-41b2-9579-4b6f8e4f7300
📒 Files selected for processing (28)
cmd/auth/list.gocmd/auth/login.gocmd/auth/login_messages.gocmd/auth/login_messages_test.gocmd/config/bind.gocmd/config/bind_test.gocmd/config/bind_warning_test.gocmd/config/config_test.gocmd/config/default_as.gocmd/config/init.gocmd/config/init_guard_test.gocmd/config/show.gocmd/config/strict_mode.gocmd/config/strict_mode_warning_test.gocmd/doctor/doctor.gocmd/profile/remove.gocmd/profile/rename.gocmd/profile/use.gocmd/prune.gocmd/root_integration_test.gointernal/cmdutil/factory.gointernal/core/config.gointernal/core/notconfigured.gointernal/core/notconfigured_test.gointernal/core/secret_resolve.gointernal/credential/credential_provider.gointernal/credential/default_provider.gotests/cli_e2e/config/bind_test.go
✅ Files skipped from review due to trivial changes (2)
- internal/credential/default_provider.go
- tests/cli_e2e/config/bind_test.go
🚧 Files skipped from review as they are similar to previous changes (13)
- cmd/profile/remove.go
- internal/core/config.go
- cmd/auth/list.go
- cmd/config/default_as.go
- internal/cmdutil/factory.go
- cmd/config/config_test.go
- cmd/config/init_guard_test.go
- cmd/config/bind_warning_test.go
- cmd/config/show.go
- cmd/config/init.go
- internal/credential/credential_provider.go
- cmd/config/strict_mode_warning_test.go
- internal/core/notconfigured_test.go
liangshuo-1
force-pushed
the
feat/agent-binding-hints
branch
2 times, most recently
from
635a843 to
a905fb4
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
cmd/auth/login_messages.go (1)
62-62: ⚡ Quick winPrefer parser-stable plain text in AI-agent hint strings
At Line 62 and Line 98, consider removing Markdown emphasis (
**...**) and symbol-heavy punctuation for more deterministic agent/log parsing while keeping the same instructions.Suggested wording adjustment
- AgentTimeoutHint: "... **不要短 timeout 反复重试**——每次重启会作废上一轮的 device code ...", + AgentTimeoutHint: "... 不要用短 timeout 反复重试。每次重启都会作废上一轮 device_code ...", - AgentTimeoutHint: "... **Do NOT retry with a short timeout** — each restart invalidates the previous device code ...", + AgentTimeoutHint: "... Do not retry with a short timeout. Each restart invalidates the previous device_code ...",As per coding guidelines: "Design CLI flags, help text, and error messages with AI agent consumption in mind; every error message will be parsed by AI agents."
Also applies to: 98-98
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/auth/login_messages.go` at line 62, Update the AI-agent hint strings to be parser-stable by removing Markdown emphasis (**...**) and symbol-heavy punctuation: edit the AgentTimeoutHint constant and the repeated AI-agent hint used later to use plain text (no bolding or extraneous symbols) while preserving the same instructions (mention timeout ≥ 600s, recommend --no-wait --json to obtain device_code, then use --device-code <code> to resume polling, and warn not to retry with short timeouts because each restart invalidates the previous device code). Ensure the revised strings remain clear and concise for CLI and AI-agent parsing.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@cmd/auth/list_test.go`:
- Around line 20-35: The test TestAuthListRun_NotConfigured_ReturnsExitZero
assumes a local workspace but never sets global workspace state; explicitly set
the workspace to local at the start by saving the current workspace (old :=
core.CurrentWorkspace()), calling core.SetCurrentWorkspace(core.LocalWorkspace)
before exercising authListRun, and defer-restoring the original workspace (defer
core.SetCurrentWorkspace(old)) so state is stable and isolated for this test.
In `@cmd/profile/rename.go`:
- Around line 35-37: The current handling of the error from
core.LoadMultiAppConfig() incorrectly maps every failure to
core.NotConfiguredError(); change it to distinguish a true “not configured”
sentinel (or file-not-found) from other failures: call
core.LoadMultiAppConfig(), inspect err (using errors.Is against whatever
sentinel error the core package exposes or os.IsNotExist for missing files), and
only return core.NotConfiguredError() when the error clearly indicates a
missing/empty config; for all other errors (parse, permission, corruption)
propagate or wrap the original error instead of collapsing it into
NotConfiguredError(). Ensure the change touches the error handling around
core.LoadMultiAppConfig() and preserves the use of core.NotConfiguredError()
only for the appropriate sentinel case.
---
Nitpick comments:
In `@cmd/auth/login_messages.go`:
- Line 62: Update the AI-agent hint strings to be parser-stable by removing
Markdown emphasis (**...**) and symbol-heavy punctuation: edit the
AgentTimeoutHint constant and the repeated AI-agent hint used later to use plain
text (no bolding or extraneous symbols) while preserving the same instructions
(mention timeout ≥ 600s, recommend --no-wait --json to obtain device_code, then
use --device-code <code> to resume polling, and warn not to retry with short
timeouts because each restart invalidates the previous device code). Ensure the
revised strings remain clear and concise for CLI and AI-agent parsing.
🪄 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: ad22863d-0d7f-418e-a1a7-563d1b55cf5d
📒 Files selected for processing (29)
cmd/auth/list.gocmd/auth/list_test.gocmd/auth/login.gocmd/auth/login_messages.gocmd/auth/login_messages_test.gocmd/config/bind.gocmd/config/bind_test.gocmd/config/bind_warning_test.gocmd/config/config_test.gocmd/config/default_as.gocmd/config/init.gocmd/config/init_guard_test.gocmd/config/show.gocmd/config/strict_mode.gocmd/config/strict_mode_warning_test.gocmd/doctor/doctor.gocmd/profile/remove.gocmd/profile/rename.gocmd/profile/use.gocmd/prune.gocmd/root_integration_test.gointernal/cmdutil/factory.gointernal/core/config.gointernal/core/notconfigured.gointernal/core/notconfigured_test.gointernal/core/secret_resolve.gointernal/credential/credential_provider.gointernal/credential/default_provider.gotests/cli_e2e/config/bind_test.go
✅ Files skipped from review due to trivial changes (5)
- cmd/auth/login_messages_test.go
- cmd/profile/remove.go
- cmd/config/init_guard_test.go
- tests/cli_e2e/config/bind_test.go
- internal/core/notconfigured_test.go
🚧 Files skipped from review as they are similar to previous changes (12)
- internal/core/secret_resolve.go
- internal/credential/default_provider.go
- cmd/auth/list.go
- cmd/profile/use.go
- cmd/doctor/doctor.go
- internal/cmdutil/factory.go
- internal/core/notconfigured.go
- cmd/auth/login.go
- cmd/config/bind_warning_test.go
- cmd/config/init.go
- cmd/config/strict_mode.go
- cmd/config/bind.go
Comment on lines
+20
to
+35
| 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) | ||
| } | ||
| } |
There was a problem hiding this comment.
Stabilize workspace state explicitly in the local-workspace test.
This test assumes local workspace behavior but never sets core.CurrentWorkspace() to local. Since workspace is process-global, leaked state from another test can make this assertion flaky.
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
Verify each finding against the current code and only fix it if needed.
In `@cmd/auth/list_test.go` around lines 20 - 35, The test
TestAuthListRun_NotConfigured_ReturnsExitZero assumes a local workspace but
never sets global workspace state; explicitly set the workspace to local at the
start by saving the current workspace (old := core.CurrentWorkspace()), calling
core.SetCurrentWorkspace(core.LocalWorkspace) before exercising authListRun, and
defer-restoring the original workspace (defer core.SetCurrentWorkspace(old)) so
state is stable and isolated for this test.
…r-identity risks
AI agents running inside OpenClaw / Hermes were routinely creating a parallel
app via `config init --new` instead of binding to the agent's existing app,
because every "not configured" hint and several deny errors hard-coded
`config init` regardless of workspace. Once bound, the same agents could
silently grant themselves user identity (impersonation) without the user
ever seeing a risk message in chat.
Changes:
- Introduce `core.NotConfiguredError` / `NoActiveProfileError` /
`reconfigureHint` helpers that branch on `CurrentWorkspace()`. In agent
workspaces they point at `lark-cli config bind --help` (a help page, not
a ready-to-run command) so AI must read the binding workflow and confirm
identity preset with the user before acting. In local terminals they
preserve the previous `config init --new` guidance.
- Migrate every `config init` hint that should be workspace-aware:
RequireConfigForProfile, default credential provider, credential provider
fallback, secret-resolve mismatch, config show, strict-mode entry-point
errors, default-as, profile use/rename/remove, auth list, doctor's
config_file check (which now also wraps the OS-level "no such file"
noise into the user-shaped "not configured" message).
- Refuse `config init` when run inside an OpenClaw / Hermes workspace by
default; add `--force-init` for the rare case the user genuinely wants
a parallel app. Without this guard, hint fixes were undone the moment
AI ignored them.
- Rewrite the strict-mode deny errors in cmd/auth/login.go, cmd/prune.go,
and internal/cmdutil/factory.go. The previous "AI agents are strictly
prohibited from modifying this setting" terminated AI reasoning while
providing no real gate. New errors point at `config strict-mode --help`
with the legitimate confirmation flow and explicitly note that switching
does NOT require re-bind. Integration test envelopes updated.
- Tighten `config bind --help` and `config strict-mode --help` to encode
the user-confirmation discipline directly: identity preset semantics
(bot-only vs user-default), "DO NOT switch without explicit user
confirmation", and a cross-reference clarifying that `config bind` is
for changing the underlying app while `config strict-mode` is the
policy-only switch (resolves an ambiguity an audit run found).
- Surface user-identity (impersonation) risk at every config write that
newly grants it, by reusing the canonical IdentityEscalationMessage
string from bind_messages.go:
- `noticeUserDefaultRisk` fires on flag-mode bind landing on
user-default, including the first-time case `warnIdentityEscalation`
misses (it requires a previous bot lock).
- `setStrictMode` warns when transitioning bot → user or bot → off
(newly permits user identity); stays quiet on narrowing changes
and on off → user (off already permitted user).
- Add tests: notconfigured_test.go (workspace branches),
init_guard_test.go (refuse + --force-init bypass), bind_warning_test.go
(user-default warning fires; bot-only does not), strict_mode_warning_test.go
(5 transitions covering both warn and no-warn paths).
Two follow-ups intentionally deferred: the keychain master-key hint at
internal/keychain/keychain.go:42 still suggests `config init` because the
keychain package can't import core (would be circular); fixing requires
either parameterizing the hint via callback or extracting workspace into
its own package. The lark-shared skill doc still tells AI to run
`config init` for first-time setup; updating the skill is in scope for
a follow-up PR.
Change-Id: I02273e044d9e061d211ceaa4f3ed5a3fb28325b3
liangshuo-1
force-pushed
the
feat/agent-binding-hints
branch
from
a905fb4 to
9a89fc1
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
AI agents in OpenClaw / Hermes were routinely creating a parallel app via
config init --newinstead of binding to the agent's existing app — every "not configured" hint and several deny errors hard-codedconfig initregardless of workspace. Once bound, the same agents could silently grant themselves user identity (impersonation) without the user ever seeing a risk message in chat. This PR fixes both classes of failure.Changes
Workspace-aware "not configured" routing
core.NotConfiguredError/NoActiveProfileError/reconfigureHinthelpers. In agent workspaces they point atlark-cli config bind --help(a help page, not a ready-to-run command) so AI must read the binding workflow and confirm identity preset with the user. Local terminal still getsconfig init --new.RequireConfigForProfile, default credential provider, credential provider fallback, secret-resolve mismatch,config show, strict-mode entry-point errors,default-as,profile use/rename/remove,auth list,doctor'sconfig_filecheck (also wraps the OS-levelno such filenoise into a user-shaped message).Refuse
config initinside agent workspacescmd/config/init.gonow refuses by default whenOPENCLAW_HOME/HERMES_HOMEis set;--force-initprovides an escape hatch for the rare legitimate case. Without this last-line guard, hint fixes were undone the moment AI ignored them.Strict-mode deny errors point at the legitimate path
cmd/auth/login.go,cmd/prune.go,internal/cmdutil/factory.go. The old "AI agents are strictly prohibited from modifying this setting" terminated AI reasoning without providing a real gate. New errors point atconfig strict-mode --helpwith the user-confirmation flow and note that switching does NOT require re-bind. Integration test envelopes updated.Help text encodes the confirmation discipline
config bind --helpandconfig strict-mode --helpnow spell out the identity-preset semantics (bot-only vs user-default), the "DO NOT switch without explicit user confirmation" rule, and a cross-reference clarifying thatconfig bindis for swapping the underlying app whileconfig strict-modeis the policy-only switch (resolves an audit-found ambiguity).Surface user-identity (impersonation) risk at the moment of grant
IdentityEscalationMessagefrombind_messages.goat two new sites:noticeUserDefaultRiskfires on flag-modebind --identity user-default, including the first-time casewarnIdentityEscalationpreviously missed.setStrictModewarns when bot → user or bot → off (newly permits user identity); stays quiet on narrowing changes (user → bot, off → bot) and on off → user (off already permitted user).⚠️and emit before the success line, making it structurally hard for an AI to summarize away.Tests
internal/core/notconfigured_test.go— workspace branchescmd/config/init_guard_test.go— refuse +--force-initbypasscmd/config/bind_warning_test.go— user-default warning fires; bot-only stays quietcmd/config/strict_mode_warning_test.go— 5 transitions covering both warn and no-warn pathscmd/root_integration_test.go— strict-mode deny envelopes updated for new wordingEnd-to-end audit by simulated AI agent (build binary, run real commands, follow each hint) shows every scenario converging in ≤2 steps for 没安装 / 主动安装 / 已经安装 / 后安装 / 重装 across local + OpenClaw + Hermes.
Test plan
lark-cli auth listshould suggestconfig bind --help, notconfig init.lark-cli config init --newinsideOPENCLAW_HOMEshould refuse and mention--force-init.lark-cli config bind --source hermes --identity user-defaultshould print the impersonation warning to stderr before the success line.lark-cli config strict-mode userfrom abotbaseline should print the same warning beforeStrict mode set to user.unset OPENCLAW_HOME HERMES_HOME) still suggestsconfig init --neweverywhere — no agent-hint leakage.Out of scope (follow-ups)
internal/keychain/keychain.go:42master-key hint still suggestsconfig init— keychain can't import core (circular). Needs either a callback-based hint or extracting workspace into its own package.skills/lark-shared/SKILL.mdstill tells AI to runconfig initfor first-time setup. Updating the skill belongs in a follow-up PR.config strict-modeyet — the discipline lives in help text, not in a structured confirmation gate. Considered as a separate PR if desired.Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Tests