feat: support project-level profile binding#1502
Closed
ennann wants to merge 1 commit into
Closed
Conversation
📝 WalkthroughWalkthroughAdds project-scoped profile binding to the CLI via a ChangesProject-scoped profile binding
Sequence Diagram(s)sequenceDiagram
participant User
participant BootstrapInvocationContext
participant core_ResolveProjectProfile
participant DefaultAccountProvider
participant core_ProjectProfileNotFoundError
rect rgba(173, 216, 230, 0.5)
Note over User,core_ResolveProjectProfile: Bootstrap phase
User->>BootstrapInvocationContext: run command (no --profile flag)
BootstrapInvocationContext->>core_ResolveProjectProfile: walk upward from cwd to .git
core_ResolveProjectProfile-->>BootstrapInvocationContext: ProjectProfileBinding{Profile, ConfigPath}
BootstrapInvocationContext-->>User: InvocationContext{ProfileSource=project, ProfileConfigPath, Profile}
end
rect rgba(255, 228, 196, 0.5)
Note over User,core_ProjectProfileNotFoundError: Credential resolution phase
User->>DefaultAccountProvider: ResolveAccount(profile)
DefaultAccountProvider->>DefaultAccountProvider: LoadMultiAppConfig
DefaultAccountProvider->>core_ProjectProfileNotFoundError: profile not in config
core_ProjectProfileNotFoundError-->>User: ConfigError{message, hint with configPath}
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 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
There was a problem hiding this comment.
Pull request overview
Note
Copilot was unable to run its full agentic suite in this review.
Adds project-local profile binding support so the CLI can resolve the effective profile from a repo-level .lark-cli.json, including new profile subcommands and fail-closed behavior when a bound profile is missing globally.
Changes:
- Introduces project config discovery/parsing/writing (
.lark-cli.json) andProfileSourcetracking (cli/project/global). - Adds
profile current,profile bind, andprofile unbindcommands plus tests around binding behavior. - Updates credential/config command paths to surface a dedicated “project profile not found” error when appropriate.
Reviewed changes
Copilot reviewed 19 out of 19 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| internal/credential/integration_test.go | Adds integration coverage for fail-closed behavior when a project-bound profile is missing. |
| internal/credential/default_provider.go | Tracks profile source/path and fails closed for missing project-bound profiles. |
| internal/core/project_config_test.go | Adds unit tests for project config discovery, IO, and validation rules. |
| internal/core/project_config.go | Implements .lark-cli.json discovery, load/save/remove, and error helpers. |
| internal/core/config.go | Introduces ProfileSource to record where the effective profile came from. |
| internal/cmdutil/factory_default.go | Threads profile source/path through credential provider construction. |
| internal/cmdutil/factory.go | Extends InvocationContext with profile source/path fields. |
| cmd/profile/project_bind.go | Adds profile bind / profile unbind commands for project bindings. |
| cmd/profile/profile_test.go | Adds tests for bind/unbind and profile current project source output. |
| cmd/profile/profile.go | Registers new profile subcommands. |
| cmd/profile/current.go | Adds profile current command to show effective profile + source + config path. |
| cmd/config/strict_mode.go | Uses shared “no active profile” error that handles missing project-bound profiles. |
| cmd/config/show.go | Uses shared “no active profile” error that handles missing project-bound profiles. |
| cmd/config/default_as.go | Uses shared “no active profile” error that handles missing project-bound profiles. |
| cmd/config/active_profile.go | Introduces shared helper to return project-profile-not-found vs no-active-profile errors. |
| cmd/bootstrap_test.go | Adds bootstrap tests for project profile discovery and override/skip behaviors. |
| cmd/bootstrap.go | Resolves invocation profile from project config unless explicitly skipped/overridden. |
| cmd/auth/logout.go | Surfaces project-profile-not-found error when applicable. |
| cmd/auth/list.go | Surfaces project-profile-not-found error when applicable. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+117
to
+121
| func SaveProjectConfig(path, profile string) error { | ||
| fields := map[string]json.RawMessage{} | ||
| if data, err := vfs.ReadFile(path); err == nil { | ||
| _ = json.Unmarshal(data, &fields) | ||
| } |
Comment on lines
+116
to
+117
| // SaveProjectConfig writes the minimal project profile binding. | ||
| func SaveProjectConfig(path, profile string) error { |
| if data, err := vfs.ReadFile(path); err == nil { | ||
| _ = json.Unmarshal(data, &fields) | ||
| } | ||
| profileJSON, err := json.Marshal(strings.TrimSpace(profile)) |
Comment on lines
70
to
+72
| func NewDefaultAccountProvider(kc func() keychain.KeychainAccess, profile string) *DefaultAccountProvider { | ||
| return NewDefaultAccountProviderWithSource(kc, profile, "", "") | ||
| } |
Comment on lines
+20
to
+23
| func NewCmdProfileBind(f *cmdutil.Factory) *cobra.Command { | ||
| cmd := &cobra.Command{ | ||
| Use: "bind <name>", | ||
| Short: "Bind the current project to a profile", |
Comment on lines
61
to
+64
| app := multi.CurrentAppConfig(f.Invocation.Profile) | ||
| if app == nil && f.Invocation.Profile != "" && f.Invocation.ProfileSource == core.ProfileSourceProject { | ||
| return core.ProjectProfileNotFoundError(f.Invocation.Profile, f.Invocation.ProfileConfigPath, multi.ProfileNames()) | ||
| } |
ennann
force-pushed
the
codex/project-profile-binding
branch
from
6d479d1 to
fa68216
Compare
|
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 9
🧹 Nitpick comments (1)
cmd/profile/profile_test.go (1)
51-132: ⚡ Quick winAdd error-path tests for the new bind/unbind/current flows with typed metadata assertions.
The new coverage is mostly happy-path. Please add failure-path cases and assert typed error metadata (
category/subtype/param, plus cause preservation) for the new command behaviors.As per coding guidelines,
**/*_test.go: “Error-path tests must assert typed metadata viaerrs.ProblemOf(category/subtype/param) and cause preservation, not message substrings alone.”🤖 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/profile/profile_test.go` around lines 51 - 132, Add new test cases for error scenarios in the profileBindRun, profileUnbindRun, and profileCurrentRun functions. For each of these three functions, create additional test cases that trigger error conditions (such as missing git root, invalid configuration, or missing files) and assert the typed error metadata using errs.ProblemOf to verify the category, subtype, and param fields are correctly set. Additionally, verify that the error cause is properly preserved in each failure scenario, following the coding guidelines that require error-path tests to assert typed metadata rather than relying on message substring matching alone.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@cmd/bootstrap_test.go`:
- Around line 168-170: The test
`TestBootstrapInvocationContext_ProfileCurrentReadsMalformedProjectConfig` only
asserts that `BootstrapInvocationContext` returns a non-nil error but does not
validate the typed error metadata (category/subtype) or verify cause
preservation. Update the test to use `errs.ProblemOf` to extract and assert the
error's category and subtype, and verify that the error cause chain is properly
preserved. Follow the error-path testing guidelines that require asserting typed
metadata rather than just checking for error existence.
- Around line 81-170: The four test functions
TestBootstrapInvocationContext_ProjectProfile,
TestBootstrapInvocationContext_ProfileFlagOverridesProjectProfile,
TestBootstrapInvocationContext_ProfileBindSkipsMalformedProjectConfig, and
TestBootstrapInvocationContext_ProfileCurrentReadsMalformedProjectConfig are not
isolating their config state, which could cause them to leak into host or user
configuration during test execution. Add a call to t.Setenv to set the
LARKSUITE_CLI_CONFIG_DIR environment variable to a temporary directory at the
beginning of each test function to ensure tests use isolated configuration state
rather than the system or user configuration.
In `@cmd/bootstrap.go`:
- Around line 55-70: The completion command checks in the first loop are
incorrectly matching any occurrence of "completion", "__complete", or
"__completeNoDesc" in the args array, which can trigger false positives when
these strings appear as normal positional values. Move these completion command
checks from the initial loop that iterates through all args into the switch
statement on parts[0] so they only match when these strings appear in the actual
command position. Keep the help flag checks in the initial loop since those are
legitimate short-circuit cases, but restrict the completion command matching to
the positional arguments switch logic like "profile" and "config".
In `@cmd/profile/profile_test.go`:
- Around line 83-101: The test function
TestProfileUnbindRun_RemovesNearestProjectConfig does not isolate the
LARKSUITE_CLI_CONFIG_DIR environment variable, which can cause config state to
leak across tests. Add config isolation at the beginning of this test function
by calling setupProfileConfigDir(t) or by using
t.Setenv("LARKSUITE_CLI_CONFIG_DIR", t.TempDir()) to ensure each test has a
clean isolated config directory.
In `@cmd/profile/project_bind.go`:
- Line 50: In the project_bind.go file, update the validation error parameter
metadata to use flag-style format. Change all instances where WithParam("name")
is called to WithParam("--name") instead. This applies to the
errs.NewValidationError calls around line 50 and also at lines 58-60 to ensure
consistent flag-style parameter identification in CLI validation error messages
as per the coding guidelines for cmd/**/*.go files.
In `@internal/core/project_config_test.go`:
- Around line 133-159: The current test
TestRemoveProjectProfile_PreservesOtherFields only covers the case where other
fields remain after profile removal, but does not test the file-deletion path
that occurs when the profile is the only field in the configuration. Create a
new test function (for example, TestRemoveProjectProfile_DeletesFileWhenEmpty)
that writes a project config containing only the profile field, calls
RemoveProjectProfile with that path, and verifies that both fileRemoved and
profileRemoved return true. Additionally, verify that the config file no longer
exists on the filesystem after the operation to ensure the deletion path is
properly tested.
In `@internal/core/project_config.go`:
- Line 131: The validate.AtomicWrite calls at line 131 and lines 159-160 are
returning raw errors without project-config context, making error handling
inconsistent. Instead of directly returning the result of validate.AtomicWrite,
capture the error, check if it exists, and wrap it with ConfigError providing
the file path and operation context (such as "write" or "save") before
returning. This ensures all errors from the validate.AtomicWrite function
include consistent ConfigError context for reliable downstream handling and
output.
- Around line 119-121: The code at lines 119-121 silently ignores errors when
reading and unmarshaling the existing .lark-cli.json file, which can result in
losing unrelated fields in the project config. Instead of ignoring read and
unmarshal errors, the code should properly handle these failures by checking if
the file exists first, and if it does exist, it should return an error or fail
appropriately when the file cannot be read or parsed, rather than continuing
with incomplete field data. Replace the current error-ignoring pattern with
proper error handling that preserves existing configuration data or explicitly
fails when the existing config cannot be reliably read and unmarshaled.
In `@internal/credential/integration_test.go`:
- Around line 117-118: The test assertion for the ConfigError in the error-path
test is using brittle substring checks with strings.Contains on both the Message
and Hint fields of cfgErr. Instead of checking if the message contains
"configured by project" and the hint contains "/repo/.lark-cli.json", replace
these substring checks with exact equality assertions (using the == operator) to
compare the actual values of cfgErr.Message and cfgErr.Hint against the complete
expected strings. This will provide deterministic coverage and prevent false
positives from accidental wording changes.
---
Nitpick comments:
In `@cmd/profile/profile_test.go`:
- Around line 51-132: Add new test cases for error scenarios in the
profileBindRun, profileUnbindRun, and profileCurrentRun functions. For each of
these three functions, create additional test cases that trigger error
conditions (such as missing git root, invalid configuration, or missing files)
and assert the typed error metadata using errs.ProblemOf to verify the category,
subtype, and param fields are correctly set. Additionally, verify that the error
cause is properly preserved in each failure scenario, following the coding
guidelines that require error-path tests to assert typed metadata rather than
relying on message substring matching alone.
🪄 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: 9f4f89f2-c6e7-4447-af0b-704b7425f5c0
📒 Files selected for processing (19)
cmd/auth/list.gocmd/auth/logout.gocmd/bootstrap.gocmd/bootstrap_test.gocmd/config/active_profile.gocmd/config/default_as.gocmd/config/show.gocmd/config/strict_mode.gocmd/profile/current.gocmd/profile/profile.gocmd/profile/profile_test.gocmd/profile/project_bind.gointernal/cmdutil/factory.gointernal/cmdutil/factory_default.gointernal/core/config.gointernal/core/project_config.gointernal/core/project_config_test.gointernal/credential/default_provider.gointernal/credential/integration_test.go
Comment on lines
+81
to
+170
| func TestBootstrapInvocationContext_ProjectProfile(t *testing.T) { | ||
| repo := t.TempDir() | ||
| if err := os.Mkdir(filepath.Join(repo, ".git"), 0700); err != nil { | ||
| t.Fatalf("Mkdir(.git): %v", err) | ||
| } | ||
| if err := os.WriteFile(filepath.Join(repo, core.ProjectConfigFileName), []byte(`{"profile":"bytedance"}`), 0600); err != nil { | ||
| t.Fatalf("WriteFile(project config): %v", err) | ||
| } | ||
| sub := filepath.Join(repo, "sub") | ||
| if err := os.MkdirAll(sub, 0700); err != nil { | ||
| t.Fatalf("MkdirAll(sub): %v", err) | ||
| } | ||
| cmdutil.TestChdir(t, sub) | ||
|
|
||
| inv, err := BootstrapInvocationContext([]string{"auth", "status"}) | ||
| if err != nil { | ||
| t.Fatalf("BootstrapInvocationContext() error = %v", err) | ||
| } | ||
| if inv.Profile != "bytedance" { | ||
| t.Fatalf("profile = %q, want bytedance", inv.Profile) | ||
| } | ||
| if inv.ProfileSource != core.ProfileSourceProject { | ||
| t.Fatalf("ProfileSource = %q, want project", inv.ProfileSource) | ||
| } | ||
| wantRepo, err := filepath.EvalSymlinks(repo) | ||
| if err != nil { | ||
| t.Fatalf("EvalSymlinks(repo): %v", err) | ||
| } | ||
| if inv.ProfileConfigPath != filepath.Join(wantRepo, core.ProjectConfigFileName) { | ||
| t.Fatalf("ProfileConfigPath = %q", inv.ProfileConfigPath) | ||
| } | ||
| } | ||
|
|
||
| func TestBootstrapInvocationContext_ProfileFlagOverridesProjectProfile(t *testing.T) { | ||
| repo := t.TempDir() | ||
| if err := os.Mkdir(filepath.Join(repo, ".git"), 0700); err != nil { | ||
| t.Fatalf("Mkdir(.git): %v", err) | ||
| } | ||
| if err := os.WriteFile(filepath.Join(repo, core.ProjectConfigFileName), []byte(`{"profile":"project"}`), 0600); err != nil { | ||
| t.Fatalf("WriteFile(project config): %v", err) | ||
| } | ||
| cmdutil.TestChdir(t, repo) | ||
|
|
||
| inv, err := BootstrapInvocationContext([]string{"--profile", "cli", "auth", "status"}) | ||
| if err != nil { | ||
| t.Fatalf("BootstrapInvocationContext() error = %v", err) | ||
| } | ||
| if inv.Profile != "cli" { | ||
| t.Fatalf("profile = %q, want cli", inv.Profile) | ||
| } | ||
| if inv.ProfileSource != core.ProfileSourceCLI { | ||
| t.Fatalf("ProfileSource = %q, want cli", inv.ProfileSource) | ||
| } | ||
| if inv.ProfileConfigPath != "" { | ||
| t.Fatalf("ProfileConfigPath = %q, want empty", inv.ProfileConfigPath) | ||
| } | ||
| } | ||
|
|
||
| func TestBootstrapInvocationContext_ProfileBindSkipsMalformedProjectConfig(t *testing.T) { | ||
| repo := t.TempDir() | ||
| if err := os.Mkdir(filepath.Join(repo, ".git"), 0700); err != nil { | ||
| t.Fatalf("Mkdir(.git): %v", err) | ||
| } | ||
| if err := os.WriteFile(filepath.Join(repo, core.ProjectConfigFileName), []byte(`{`), 0600); err != nil { | ||
| t.Fatalf("WriteFile(project config): %v", err) | ||
| } | ||
| cmdutil.TestChdir(t, repo) | ||
|
|
||
| inv, err := BootstrapInvocationContext([]string{"profile", "bind", "bytedance"}) | ||
| if err != nil { | ||
| t.Fatalf("BootstrapInvocationContext() error = %v", err) | ||
| } | ||
| if inv.Profile != "" || inv.ProfileSource != core.ProfileSourceGlobal { | ||
| t.Fatalf("invocation = %#v, want global without profile", inv) | ||
| } | ||
| } | ||
|
|
||
| func TestBootstrapInvocationContext_ProfileCurrentReadsMalformedProjectConfig(t *testing.T) { | ||
| repo := t.TempDir() | ||
| if err := os.Mkdir(filepath.Join(repo, ".git"), 0700); err != nil { | ||
| t.Fatalf("Mkdir(.git): %v", err) | ||
| } | ||
| if err := os.WriteFile(filepath.Join(repo, core.ProjectConfigFileName), []byte(`{`), 0600); err != nil { | ||
| t.Fatalf("WriteFile(project config): %v", err) | ||
| } | ||
| cmdutil.TestChdir(t, repo) | ||
|
|
||
| if _, err := BootstrapInvocationContext([]string{"profile", "current"}); err == nil { | ||
| t.Fatal("BootstrapInvocationContext() error = nil, want malformed project config error") | ||
| } |
There was a problem hiding this comment.
Isolate config state in the new bootstrap tests.
These new tests should set LARKSUITE_CLI_CONFIG_DIR to a temp dir to avoid leaking host/user config into resolution behavior.
As per coding guidelines, **/*_test.go: “Isolate config state in tests with t.Setenv("LARKSUITE_CLI_CONFIG_DIR", t.TempDir()).”
🤖 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/bootstrap_test.go` around lines 81 - 170, The four test functions
TestBootstrapInvocationContext_ProjectProfile,
TestBootstrapInvocationContext_ProfileFlagOverridesProjectProfile,
TestBootstrapInvocationContext_ProfileBindSkipsMalformedProjectConfig, and
TestBootstrapInvocationContext_ProfileCurrentReadsMalformedProjectConfig are not
isolating their config state, which could cause them to leak into host or user
configuration during test execution. Add a call to t.Setenv to set the
LARKSUITE_CLI_CONFIG_DIR environment variable to a temporary directory at the
beginning of each test function to ensure tests use isolated configuration state
rather than the system or user configuration.
Source: Coding guidelines
Comment on lines
+168
to
+170
| if _, err := BootstrapInvocationContext([]string{"profile", "current"}); err == nil { | ||
| t.Fatal("BootstrapInvocationContext() error = nil, want malformed project config error") | ||
| } |
There was a problem hiding this comment.
Assert typed error metadata for malformed project config.
TestBootstrapInvocationContext_ProfileCurrentReadsMalformedProjectConfig currently only asserts non-nil error. Please assert typed metadata (category/subtype) via errs.ProblemOf and verify cause preservation as part of this error-path contract.
As per coding guidelines, **/*_test.go: “Error-path tests must assert typed metadata via errs.ProblemOf (category / subtype / param) and cause preservation, not message substrings alone.” Based on learnings, if param must be asserted, use errors.As(err, *errs.ValidationError) rather than errs.ProblemOf.
🤖 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/bootstrap_test.go` around lines 168 - 170, The test
`TestBootstrapInvocationContext_ProfileCurrentReadsMalformedProjectConfig` only
asserts that `BootstrapInvocationContext` returns a non-nil error but does not
validate the typed error metadata (category/subtype) or verify cause
preservation. Update the test to use `errs.ProblemOf` to extract and assert the
error's category and subtype, and verify that the error cause chain is properly
preserved. Follow the error-path testing guidelines that require asserting typed
metadata rather than just checking for error existence.
Sources: Coding guidelines, Learnings
Comment on lines
+55
to
+70
| for _, arg := range args { | ||
| if arg == "-h" || arg == "--help" || arg == "completion" || arg == "__complete" || arg == "__completeNoDesc" { | ||
| return true | ||
| } | ||
| } | ||
| parts := firstPositionalArgs(args, 2) | ||
| if len(parts) == 0 { | ||
| return false | ||
| } | ||
| switch parts[0] { | ||
| case "profile": | ||
| return len(parts) < 2 || parts[1] != "current" | ||
| case "config": | ||
| return len(parts) >= 2 && (parts[1] == "bind" || parts[1] == "init" || parts[1] == "remove") | ||
| default: | ||
| return false |
There was a problem hiding this comment.
Restrict completion-command skipping to command positions.
skipProjectProfileLookup currently treats any token equal to "completion", "__complete", or "__completeNoDesc" as a completion command. That can match normal positional values and incorrectly bypass project-profile resolution.
Suggested fix
func skipProjectProfileLookup(args []string) bool {
for _, arg := range args {
- if arg == "-h" || arg == "--help" || arg == "completion" || arg == "__complete" || arg == "__completeNoDesc" {
+ if arg == "-h" || arg == "--help" {
return true
}
}
parts := firstPositionalArgs(args, 2)
if len(parts) == 0 {
return false
}
switch parts[0] {
+ case "completion", "__complete", "__completeNoDesc":
+ return true
case "profile":
return len(parts) < 2 || parts[1] != "current"
case "config":
return len(parts) >= 2 && (parts[1] == "bind" || parts[1] == "init" || parts[1] == "remove")
default:
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 `@cmd/bootstrap.go` around lines 55 - 70, The completion command checks in the
first loop are incorrectly matching any occurrence of "completion",
"__complete", or "__completeNoDesc" in the args array, which can trigger false
positives when these strings appear as normal positional values. Move these
completion command checks from the initial loop that iterates through all args
into the switch statement on parts[0] so they only match when these strings
appear in the actual command position. Keep the help flag checks in the initial
loop since those are legitimate short-circuit cases, but restrict the completion
command matching to the positional arguments switch logic like "profile" and
"config".
Comment on lines
+83
to
+101
| func TestProfileUnbindRun_RemovesNearestProjectConfig(t *testing.T) { | ||
| repo := t.TempDir() | ||
| if err := os.Mkdir(filepath.Join(repo, ".git"), 0700); err != nil { | ||
| t.Fatalf("Mkdir(.git): %v", err) | ||
| } | ||
| path := filepath.Join(repo, core.ProjectConfigFileName) | ||
| if err := os.WriteFile(path, []byte(`{"profile":"bytedance"}`), 0600); err != nil { | ||
| t.Fatalf("WriteFile(project config): %v", err) | ||
| } | ||
| sub := filepath.Join(repo, "sub") | ||
| if err := os.MkdirAll(sub, 0700); err != nil { | ||
| t.Fatalf("MkdirAll(sub): %v", err) | ||
| } | ||
| cmdutil.TestChdir(t, sub) | ||
|
|
||
| f, _, _, _ := cmdutil.TestFactory(t, nil) | ||
| if err := profileUnbindRun(f); err != nil { | ||
| t.Fatalf("profileUnbindRun() error = %v", err) | ||
| } |
There was a problem hiding this comment.
Isolate config state in TestProfileUnbindRun_RemovesNearestProjectConfig.
This test skips LARKSUITE_CLI_CONFIG_DIR isolation. Add setupProfileConfigDir(t) at the top to avoid ambient config leakage across tests.
Suggested patch
func TestProfileUnbindRun_RemovesNearestProjectConfig(t *testing.T) {
+ setupProfileConfigDir(t)
repo := t.TempDir()
if err := os.Mkdir(filepath.Join(repo, ".git"), 0700); err != nil {
t.Fatalf("Mkdir(.git): %v", err)
}As per coding guidelines, **/*_test.go: “Isolate config state in tests with t.Setenv("LARKSUITE_CLI_CONFIG_DIR", t.TempDir()).”
🤖 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/profile/profile_test.go` around lines 83 - 101, The test function
TestProfileUnbindRun_RemovesNearestProjectConfig does not isolate the
LARKSUITE_CLI_CONFIG_DIR environment variable, which can cause config state to
leak across tests. Add config isolation at the beginning of this test function
by calling setupProfileConfigDir(t) or by using
t.Setenv("LARKSUITE_CLI_CONFIG_DIR", t.TempDir()) to ensure each test has a
clean isolated config directory.
Source: Coding guidelines
| func profileBindRun(f *cmdutil.Factory, name string) error { | ||
| name = strings.TrimSpace(name) | ||
| if err := core.ValidateProfileName(name); err != nil { | ||
| return errs.NewValidationError(errs.SubtypeInvalidArgument, "%v", err).WithParam("name").WithCause(err) |
There was a problem hiding this comment.
Use flag-style param metadata for bind <name> validation errors.
The typed validation envelope should identify the CLI input in flag-style form; both branches currently use "name" instead of a flag-form parameter key (for example, "--name").
Suggested patch
- return errs.NewValidationError(errs.SubtypeInvalidArgument, "%v", err).WithParam("name").WithCause(err)
+ return errs.NewValidationError(errs.SubtypeInvalidArgument, "%v", err).WithParam("--name").WithCause(err)
@@
return errs.NewValidationError(errs.SubtypeInvalidArgument, "profile %q not found", name).
WithHint("available profiles: %s", formatProfileNameList(multi.ProfileNames())).
- WithParam("name")
+ WithParam("--name")As per coding guidelines, cmd/**/*.go: “Use errs.NewValidationError(errs.SubtypeInvalidArgument, ...).WithParam("--flag") for user flag/argument validation failures.”
Also applies to: 58-60
🤖 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/profile/project_bind.go` at line 50, In the project_bind.go file, update
the validation error parameter metadata to use flag-style format. Change all
instances where WithParam("name") is called to WithParam("--name") instead. This
applies to the errs.NewValidationError calls around line 50 and also at lines
58-60 to ensure consistent flag-style parameter identification in CLI validation
error messages as per the coding guidelines for cmd/**/*.go files.
Source: Coding guidelines
Comment on lines
+133
to
+159
| func TestRemoveProjectProfile_PreservesOtherFields(t *testing.T) { | ||
| path := writeProjectConfig(t, t.TempDir(), `{"profile":"bytedance","defaults":{"appId":"cli_x"}}`) | ||
| fileRemoved, profileRemoved, err := RemoveProjectProfile(path) | ||
| if err != nil { | ||
| t.Fatalf("RemoveProjectProfile() error = %v", err) | ||
| } | ||
| if fileRemoved || !profileRemoved { | ||
| t.Fatalf("RemoveProjectProfile() = fileRemoved:%v profileRemoved:%v, want file kept and profile removed", fileRemoved, profileRemoved) | ||
| } | ||
| data, err := os.ReadFile(path) | ||
| if err != nil { | ||
| t.Fatalf("ReadFile(project config): %v", err) | ||
| } | ||
| var fields map[string]json.RawMessage | ||
| if err := json.Unmarshal(data, &fields); err != nil { | ||
| t.Fatalf("Unmarshal(project config): %v", err) | ||
| } | ||
| if _, ok := fields["defaults"]; !ok { | ||
| t.Fatalf("defaults field missing after profile removal: %s", string(data)) | ||
| } | ||
| if _, ok := fields["profile"]; ok { | ||
| t.Fatalf("profile field still present after removal: %s", string(data)) | ||
| } | ||
| if _, err := LoadProjectConfig(path); err == nil { | ||
| t.Fatal("LoadProjectConfig() error = nil after profile removal, want missing profile") | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick win
Add coverage for the file-deletion branch in RemoveProjectProfile
Current tests validate the “preserve other fields” path, but not the len(fields)==0 path that should remove the file entirely.
Suggested test case
+func TestRemoveProjectProfile_RemovesFileWhenOnlyProfileField(t *testing.T) {
+ path := writeProjectConfig(t, t.TempDir(), `{"profile":"bytedance"}`)
+ fileRemoved, profileRemoved, err := RemoveProjectProfile(path)
+ if err != nil {
+ t.Fatalf("RemoveProjectProfile() error = %v", err)
+ }
+ if !fileRemoved || !profileRemoved {
+ t.Fatalf("RemoveProjectProfile() = fileRemoved:%v profileRemoved:%v, want both true", fileRemoved, profileRemoved)
+ }
+ if _, err := os.Stat(path); !errors.Is(err, os.ErrNotExist) {
+ t.Fatalf("expected file removed, stat err = %v", err)
+ }
+}As per coding guidelines, “Every behavior change needs a test alongside the change.”
📝 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 TestRemoveProjectProfile_PreservesOtherFields(t *testing.T) { | |
| path := writeProjectConfig(t, t.TempDir(), `{"profile":"bytedance","defaults":{"appId":"cli_x"}}`) | |
| fileRemoved, profileRemoved, err := RemoveProjectProfile(path) | |
| if err != nil { | |
| t.Fatalf("RemoveProjectProfile() error = %v", err) | |
| } | |
| if fileRemoved || !profileRemoved { | |
| t.Fatalf("RemoveProjectProfile() = fileRemoved:%v profileRemoved:%v, want file kept and profile removed", fileRemoved, profileRemoved) | |
| } | |
| data, err := os.ReadFile(path) | |
| if err != nil { | |
| t.Fatalf("ReadFile(project config): %v", err) | |
| } | |
| var fields map[string]json.RawMessage | |
| if err := json.Unmarshal(data, &fields); err != nil { | |
| t.Fatalf("Unmarshal(project config): %v", err) | |
| } | |
| if _, ok := fields["defaults"]; !ok { | |
| t.Fatalf("defaults field missing after profile removal: %s", string(data)) | |
| } | |
| if _, ok := fields["profile"]; ok { | |
| t.Fatalf("profile field still present after removal: %s", string(data)) | |
| } | |
| if _, err := LoadProjectConfig(path); err == nil { | |
| t.Fatal("LoadProjectConfig() error = nil after profile removal, want missing profile") | |
| } | |
| } | |
| func TestRemoveProjectProfile_PreservesOtherFields(t *testing.T) { | |
| path := writeProjectConfig(t, t.TempDir(), `{"profile":"bytedance","defaults":{"appId":"cli_x"}}`) | |
| fileRemoved, profileRemoved, err := RemoveProjectProfile(path) | |
| if err != nil { | |
| t.Fatalf("RemoveProjectProfile() error = %v", err) | |
| } | |
| if fileRemoved || !profileRemoved { | |
| t.Fatalf("RemoveProjectProfile() = fileRemoved:%v profileRemoved:%v, want file kept and profile removed", fileRemoved, profileRemoved) | |
| } | |
| data, err := os.ReadFile(path) | |
| if err != nil { | |
| t.Fatalf("ReadFile(project config): %v", err) | |
| } | |
| var fields map[string]json.RawMessage | |
| if err := json.Unmarshal(data, &fields); err != nil { | |
| t.Fatalf("Unmarshal(project config): %v", err) | |
| } | |
| if _, ok := fields["defaults"]; !ok { | |
| t.Fatalf("defaults field missing after profile removal: %s", string(data)) | |
| } | |
| if _, ok := fields["profile"]; ok { | |
| t.Fatalf("profile field still present after removal: %s", string(data)) | |
| } | |
| if _, err := LoadProjectConfig(path); err == nil { | |
| t.Fatal("LoadProjectConfig() error = nil after profile removal, want missing profile") | |
| } | |
| } | |
| func TestRemoveProjectProfile_RemovesFileWhenOnlyProfileField(t *testing.T) { | |
| path := writeProjectConfig(t, t.TempDir(), `{"profile":"bytedance"}`) | |
| fileRemoved, profileRemoved, err := RemoveProjectProfile(path) | |
| if err != nil { | |
| t.Fatalf("RemoveProjectProfile() error = %v", err) | |
| } | |
| if !fileRemoved || !profileRemoved { | |
| t.Fatalf("RemoveProjectProfile() = fileRemoved:%v profileRemoved:%v, want both true", fileRemoved, profileRemoved) | |
| } | |
| if _, err := os.Stat(path); !errors.Is(err, os.ErrNotExist) { | |
| t.Fatalf("expected file removed, stat err = %v", 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 `@internal/core/project_config_test.go` around lines 133 - 159, The current
test TestRemoveProjectProfile_PreservesOtherFields only covers the case where
other fields remain after profile removal, but does not test the file-deletion
path that occurs when the profile is the only field in the configuration. Create
a new test function (for example, TestRemoveProjectProfile_DeletesFileWhenEmpty)
that writes a project config containing only the profile field, calls
RemoveProjectProfile with that path, and verifies that both fileRemoved and
profileRemoved return true. Additionally, verify that the config file no longer
exists on the filesystem after the operation to ensure the deletion path is
properly tested.
Source: Coding guidelines
Comment on lines
+119
to
+121
| if data, err := vfs.ReadFile(path); err == nil { | ||
| _ = json.Unmarshal(data, &fields) | ||
| } |
There was a problem hiding this comment.
Fail closed when existing .lark-cli.json cannot be parsed or read
On Line 119 and Line 120, read/unmarshal failures are ignored and the code continues writing. That can silently wipe unrelated fields in an existing project config instead of preserving them.
Suggested fix
func SaveProjectConfig(path, profile string) error {
fields := map[string]json.RawMessage{}
- if data, err := vfs.ReadFile(path); err == nil {
- _ = json.Unmarshal(data, &fields)
- }
+ data, err := vfs.ReadFile(path)
+ switch {
+ case err == nil:
+ if err := json.Unmarshal(data, &fields); err != nil {
+ return &ConfigError{Code: 3, Type: "config", Message: fmt.Sprintf("invalid project config %s: %v", path, err)}
+ }
+ case !errors.Is(err, os.ErrNotExist):
+ return &ConfigError{Code: 3, Type: "config", Message: fmt.Sprintf("cannot read project config %s: %v", path, err)}
+ }
profileJSON, err := json.Marshal(strings.TrimSpace(profile))
if err != nil {
return &ConfigError{Code: 3, Type: "config", Message: fmt.Sprintf("failed to marshal project config: %v", err)}
}📝 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
| if data, err := vfs.ReadFile(path); err == nil { | |
| _ = json.Unmarshal(data, &fields) | |
| } | |
| data, err := vfs.ReadFile(path) | |
| switch { | |
| case err == nil: | |
| if err := json.Unmarshal(data, &fields); err != nil { | |
| return &ConfigError{Code: 3, Type: "config", Message: fmt.Sprintf("invalid project config %s: %v", path, err)} | |
| } | |
| case !errors.Is(err, os.ErrNotExist): | |
| return &ConfigError{Code: 3, Type: "config", Message: fmt.Sprintf("cannot read project config %s: %v", path, 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 `@internal/core/project_config.go` around lines 119 - 121, The code at lines
119-121 silently ignores errors when reading and unmarshaling the existing
.lark-cli.json file, which can result in losing unrelated fields in the project
config. Instead of ignoring read and unmarshal errors, the code should properly
handle these failures by checking if the file exists first, and if it does
exist, it should return an error or fail appropriately when the file cannot be
read or parsed, rather than continuing with incomplete field data. Replace the
current error-ignoring pattern with proper error handling that preserves
existing configuration data or explicitly fails when the existing config cannot
be reliably read and unmarshaled.
| if err != nil { | ||
| return &ConfigError{Code: 3, Type: "config", Message: fmt.Sprintf("failed to marshal project config: %v", err)} | ||
| } | ||
| return validate.AtomicWrite(path, append(data, '\n'), 0600) |
There was a problem hiding this comment.
Wrap atomic-write failures with project-config context
Line 131 and Line 159-160 return raw validate.AtomicWrite errors. This drops consistent ConfigError context (path + operation), making downstream handling/output less reliable.
Suggested fix
- return validate.AtomicWrite(path, append(data, '\n'), 0600)
+ if err := validate.AtomicWrite(path, append(data, '\n'), 0600); err != nil {
+ return &ConfigError{Code: 3, Type: "config", Message: fmt.Sprintf("cannot write project config %s: %v", path, err)}
+ }
+ return nil
@@
- if err := validate.AtomicWrite(path, append(next, '\n'), 0600); err != nil {
- return false, false, err
+ if err := validate.AtomicWrite(path, append(next, '\n'), 0600); err != nil {
+ return false, false, &ConfigError{Code: 3, Type: "config", Message: fmt.Sprintf("cannot write project config %s: %v", path, err)}
}Also applies to: 159-160
🤖 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/project_config.go` at line 131, The validate.AtomicWrite calls
at line 131 and lines 159-160 are returning raw errors without project-config
context, making error handling inconsistent. Instead of directly returning the
result of validate.AtomicWrite, capture the error, check if it exists, and wrap
it with ConfigError providing the file path and operation context (such as
"write" or "save") before returning. This ensures all errors from the
validate.AtomicWrite function include consistent ConfigError context for
reliable downstream handling and output.
Comment on lines
+117
to
+118
| if !strings.Contains(cfgErr.Message, "configured by project") || !strings.Contains(cfgErr.Hint, "/repo/.lark-cli.json") { | ||
| t.Fatalf("ConfigError = %#v", cfgErr) |
There was a problem hiding this comment.
Strengthen this error-path assertion (avoid substring-only checks)
Line 117 relies on strings.Contains for both Message and Hint, which is brittle and can pass on accidental wording changes. Since the test already narrows to *core.ConfigError, assert exact expected fields for deterministic coverage.
Suggested tightening
- if !strings.Contains(cfgErr.Message, "configured by project") || !strings.Contains(cfgErr.Hint, "/repo/.lark-cli.json") {
+ if cfgErr.Message != `profile "missing" is configured by project but not found` ||
+ cfgErr.Hint != "project config: /repo/.lark-cli.json; run: lark-cli profile list; available profiles: bytedance" {
t.Fatalf("ConfigError = %#v", cfgErr)
}As per coding guidelines, error-path tests should not rely on message substrings alone.
🤖 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/credential/integration_test.go` around lines 117 - 118, The test
assertion for the ConfigError in the error-path test is using brittle substring
checks with strings.Contains on both the Message and Hint fields of cfgErr.
Instead of checking if the message contains "configured by project" and the hint
contains "/repo/.lark-cli.json", replace these substring checks with exact
equality assertions (using the == operator) to compare the actual values of
cfgErr.Message and cfgErr.Hint against the complete expected strings. This will
provide deterministic coverage and prevent false positives from accidental
wording changes.
Source: Coding guidelines
Author
|
Closing this draft iteration to reopen with a cleaner single-commit PR and the finalized project config path. |
This was referenced Jun 17, 2026
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
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
Add project-level profile binding with
.lark-cli.json, so commands run inside a repository can use the intended profile without passing--profileevery time.This keeps the existing precedence model intact: an explicit
--profileflag still wins over the project binding, and the global current profile remains the fallback when no project binding exists.Changes
.lark-cli.json, stopping at the Git root.profile currentto show the effective profile, source, config path, app ID, and brand.profile bind <name>andprofile unbindfor managing project-level bindings..lark-cli.jsonwhen updating or removing the profile binding, so the file can be extended later.Test Plan
make buildgofmt -l .go vet ./...go mod tidy && git diff --exit-code -- go.mod go.sumgo run github.com/golangci/golangci-lint/v2/cmd/golangci-lint@v2.1.6 run --new-from-rev=origin/maingo test ./cmd ./internal/core ./cmd/profile ./cmd/config ./cmd/auth ./internal/credential ./internal/cmdutil--profileoverridemake unit-testis currently blocked by an existingshortcuts/minutesfailure that also reproduces on cleanorigin/mainThe tests cover realistic named profiles and hyphenated aliases such as
bytedance,team-prod, andlark-boe.Related Issues
中文说明
这个 PR 增加项目级 profile 绑定能力。项目内存在
.lark-cli.json时，普通命令不需要再显式传--profile，CLI 会自动使用项目绑定的 profile；显式--profile仍然最高优先级。项目配置文件只保存 profile 名称，不保存凭证、token、登录用户或 app secret。这样既能解决多租户项目的默认 profile 问题，也不会把机器本地状态写进仓库。
Summary by CodeRabbit
Release Notes
New Features
profile bind,profile unbind, andprofile currentto manage and inspect project-bound profiles stored in.lark-cli.json.Bug Fixes
Tests