test: inject user env only for cli e2e tests with uat#541
Merged
liangshuo-1 merged 1 commit intolarksuite:mainfrom Apr 17, 2026
Merged
test: inject user env only for cli e2e tests with uat#541liangshuo-1 merged 1 commit intolarksuite:mainfrom
liangshuo-1 merged 1 commit intolarksuite:mainfrom
Conversation
github-actions
Bot
added
the
size/S
📝 WalkthroughWalkthroughRefactored E2E test environment variable injection by moving credentials from CI workflow exports to per-request handling in the test harness. Added Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (1 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 |
yxzhaao
changed the title
yxzhaao
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (3)
tests/cli_e2e/core.go (2)
149-164:req.Envis silently overridden byTEST_*injection whenDefaultAs == "user".Caller-supplied per-request overrides are copied into
overridesfirst, then clobbered by theTEST_BOT1_APP_ID/TEST_USER_ACCESS_TOKENinjection for user commands. That is the opposite of the usual "explicit request beats implicit environment" precedence and will surprise future callers that try to pin a specificLARKSUITE_CLI_APP_ID/LARKSUITE_CLI_USER_ACCESS_TOKENviareq.Envfor a user test. No callers ofreq.Envexist yet, so cheap to fix now.♻️ Apply injection first, then let
req.Envwinfunc buildCommandEnv(req Request) []string { env := append([]string{}, os.Environ()...) overrides := map[string]string{} - for k, v := range req.Env { - overrides[k] = v - } // Keep user-token injection scoped to user-only test commands so bot - // commands continue to use config-init credentials in the same process. + // commands continue to use config-init credentials from the inherited env. if req.DefaultAs == "user" { if appID := os.Getenv("TEST_BOT1_APP_ID"); appID != "" { if token := os.Getenv("TEST_USER_ACCESS_TOKEN"); token != "" { overrides["LARKSUITE_CLI_APP_ID"] = appID overrides["LARKSUITE_CLI_USER_ACCESS_TOKEN"] = token } } } + // req.Env takes precedence so callers can pin credentials per invocation. + for k, v := range req.Env { + overrides[k] = v + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/cli_e2e/core.go` around lines 149 - 164, The env injection currently copies req.Env into overrides then conditionally sets LARKSUITE_CLI_APP_ID / LARKSUITE_CLI_USER_ACCESS_TOKEN when DefaultAs == "user", which clobbers caller-supplied values; change buildCommandEnv to apply the TEST_BOT1_APP_ID / TEST_USER_ACCESS_TOKEN injection into overrides first (if present) and then copy req.Env entries into overrides so req.Env wins (i.e., later assignments from req.Env overwrite any injected keys).
160-161: Nit: reuseenvvarsconstants instead of hardcoding the variable names.
internal/envvarsalready exportsCliAppID/CliUserAccessToken(the same strings consumed byextension/credential/env/env.go). Referencing the constants keeps the injection key in lockstep with the consumer if it ever gets renamed. Optional.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/cli_e2e/core.go` around lines 160 - 161, Replace the hardcoded environment variable keys in the overrides map with the exported constants from internal/envvars: import the package and use envvars.CliAppID instead of "LARKSUITE_CLI_APP_ID" and envvars.CliUserAccessToken instead of "LARKSUITE_CLI_USER_ACCESS_TOKEN" so the test stays in sync with the consumer; update the overrides[...] assignments in tests/cli_e2e/core.go to use those constants.tests/cli_e2e/core_test.go (1)
214-225: LGTM; consider tightening coverage a touch.The subtest is focused and uses specific sentinel values (
cli_app_test,uat_test) that make theNotContainsassertions robust against parent-env noise. A couple of optional additions worth considering, especially since the bar for this behavior is "don't leak user credentials to bot commands":
- Partial-credential case: only one of
TEST_BOT1_APP_ID/TEST_USER_ACCESS_TOKENset → assert neitherLARKSUITE_CLI_*is injected.- Empty
DefaultAs(default identity) → assert no injection, matching the explicit-opt-in contract.req.Envoverride behavior (pairs with the precedence comment oncore.go).🧪 Suggested extra cases
t.Run("skips injection when only one test var is set", func(t *testing.T) { t.Setenv("TEST_BOT1_APP_ID", "cli_app_test") t.Setenv("TEST_USER_ACCESS_TOKEN", "") env := buildCommandEnv(Request{DefaultAs: "user"}) assert.NotContains(t, env, "LARKSUITE_CLI_APP_ID=cli_app_test") assert.NotContains(t, env, "LARKSUITE_CLI_USER_ACCESS_TOKEN=") }) t.Run("skips injection when DefaultAs is empty", func(t *testing.T) { t.Setenv("TEST_BOT1_APP_ID", "cli_app_test") t.Setenv("TEST_USER_ACCESS_TOKEN", "uat_test") env := buildCommandEnv(Request{}) assert.NotContains(t, env, "LARKSUITE_CLI_APP_ID=cli_app_test") assert.NotContains(t, env, "LARKSUITE_CLI_USER_ACCESS_TOKEN=uat_test") })🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/cli_e2e/core_test.go` around lines 214 - 225, Add two focused subtests in tests/cli_e2e/core_test.go exercising buildCommandEnv: one where only TEST_BOT1_APP_ID or only TEST_USER_ACCESS_TOKEN is set (call buildCommandEnv(Request{DefaultAs: "user"}) and assert neither LARKSUITE_CLI_APP_ID nor LARKSUITE_CLI_USER_ACCESS_TOKEN are present), and one where DefaultAs is empty (buildCommandEnv(Request{}) with both env vars set and assert no injection); optionally add a test to cover req.Env override behavior referenced in core.go to ensure Request.Env takes precedence over process env when present.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@tests/cli_e2e/core_test.go`:
- Around line 214-225: Add two focused subtests in tests/cli_e2e/core_test.go
exercising buildCommandEnv: one where only TEST_BOT1_APP_ID or only
TEST_USER_ACCESS_TOKEN is set (call buildCommandEnv(Request{DefaultAs: "user"})
and assert neither LARKSUITE_CLI_APP_ID nor LARKSUITE_CLI_USER_ACCESS_TOKEN are
present), and one where DefaultAs is empty (buildCommandEnv(Request{}) with both
env vars set and assert no injection); optionally add a test to cover req.Env
override behavior referenced in core.go to ensure Request.Env takes precedence
over process env when present.
In `@tests/cli_e2e/core.go`:
- Around line 149-164: The env injection currently copies req.Env into overrides
then conditionally sets LARKSUITE_CLI_APP_ID / LARKSUITE_CLI_USER_ACCESS_TOKEN
when DefaultAs == "user", which clobbers caller-supplied values; change
buildCommandEnv to apply the TEST_BOT1_APP_ID / TEST_USER_ACCESS_TOKEN injection
into overrides first (if present) and then copy req.Env entries into overrides
so req.Env wins (i.e., later assignments from req.Env overwrite any injected
keys).
- Around line 160-161: Replace the hardcoded environment variable keys in the
overrides map with the exported constants from internal/envvars: import the
package and use envvars.CliAppID instead of "LARKSUITE_CLI_APP_ID" and
envvars.CliUserAccessToken instead of "LARKSUITE_CLI_USER_ACCESS_TOKEN" so the
test stays in sync with the consumer; update the overrides[...] assignments in
tests/cli_e2e/core.go to use those constants.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 4453b309-c945-45db-ab05-28608225166c
📒 Files selected for processing (3)
.github/workflows/ci.ymltests/cli_e2e/core.gotests/cli_e2e/core_test.go
💤 Files with no reviewable changes (1)
- .github/workflows/ci.yml
liangshuo-1
approved these changes
Apr 17, 2026
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #541 +/- ##
=======================================
Coverage 59.09% 59.09%
=======================================
Files 384 384
Lines 32672 32672
=======================================
Hits 19307 19307
Misses 11556 11556
Partials 1809 1809 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
🚀 PR Preview Install Guide🧰 CLI updatenpm i -g https://pkg.pr.new/larksuite/cli/@larksuite/cli@2014032b8473431bd87caf0ff3d01d532431ded0🧩 Skill updatenpx skills add yxzhaao/cli#feat/cli-e2e-user-env-injection -y -g |
liangshuo-1
approved these changes
Apr 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
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Changes
Test Plan
lark xxxcommand works as expectedRelated Issues
Summary by CodeRabbit
Tests
Chores