feat(env): support boe/pre/online environment switching#2006
Closed
fineday009 wants to merge 1 commit into
Closed
Conversation
Retarget the whole CLI at a non-production environment without adding an --env flag, so no command surface changes and production paths stay on their existing brand-based resolution. - LARKSUITE_CLI_ENDPOINT_DOMAIN overrides every resolved endpoint (open/accounts/mcp/applink) ahead of brand resolution - LARKSUITE_CLI_EXTRA_HEADERS injects arbitrary headers (X-TT-ENV for boe/pre swimlanes) into both the HTTP client and SDK transport chains - LARK_CLI_SHOW_LOGID surfaces x-tt-logid; errors always, all responses when enabled - LARK_CLI_DEBUG_HEADERS dumps the final outbound headers from the innermost transport layer, with credentials redacted - env/larkenv wraps all of it: one setup command, per-environment config isolation under ~/.lark-cli-env, QR-based login that avoids inheriting the browser's session All four variables are inert when unset. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
weibiao.x seems not to be a GitHub user. You need a GitHub account to be able to sign the CLA. If you have already a GitHub account, please add the email address used for this commit to your account. You have signed the CLA already but the status is still pending? Let us recheck it. |
📝 WalkthroughWalkthroughAdds the ChangesEnvironment and transport features
Estimated code review effort: 4 (Complex) | ~60 minutes Possibly related issues
Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ 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
![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 (1)
internal/cmdutil/transport_test.go (1)
100-123: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winAssert the newly added debug transport in every chain.
Each test claims
RetryTransportwraps the base transport, but production insertsDebugHeaderTransportbetween them. The new diagnostic layer can be removed without failing these tests.
internal/cmdutil/transport_test.go#L100-L123: assertRetryTransport.Baseis*DebugHeaderTransport.internal/cmdutil/transport_test.go#L132-L159: assertRetryTransport.Baseis*DebugHeaderTransport.internal/cmdutil/transport_test.go#L167-L190: assertRetryTransport.Baseis*DebugHeaderTransport.🤖 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/cmdutil/transport_test.go` around lines 100 - 123, Update the transport-chain assertions in internal/cmdutil/transport_test.go at ranges 100-123, 132-159, and 167-190 so each RetryTransport assertion verifies that RetryTransport.Base is a DebugHeaderTransport before the underlying base transport. Preserve the existing chain checks and failure diagnostics while adding this assertion in all three affected test cases.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 `@env/larkenv`:
- Around line 84-109: The apply_env function currently accepts unsupported
environment names, enabling config-directory traversal and unintended endpoint
defaults. Add validation at the start of apply_env so only boe, pre, and online
are accepted; for any other value, emit a failure return before exporting
LARKSUITE_CLI_CONFIG_DIR or creating directories, while preserving the existing
handling for supported environments.
- Around line 187-194: Update the secret-input flow in the app-secret
configuration block to use IFS= read -r so leading and trailing whitespace is
preserved. Add cleanup trap handling around stty -echo that restores terminal
echo on normal completion and signals, while retaining the existing stdin-based
secret submission and cleanup.
In `@internal/cmdutil/debug_header_transport.go`:
- Around line 23-30: Add "proxy-authorization" to the sensitiveHeaders map used
by the debug transport so this credential is redacted while its presence remains
reported. Add a regression test covering a request with Proxy-Authorization and
verify the debug output does not expose its value.
In `@internal/cmdutil/env_header_transport.go`:
- Around line 22-35: Update parseExtraHeaders in
internal/cmdutil/env_header_transport.go to reject malformed segments and blank
header names by returning the established typed validation error instead of
silently skipping them; adjust internal/cmdutil/env_header_transport_test.go
lines 26-31 to assert that malformed input returns this validation error rather
than being omitted.
In `@internal/cmdutil/factory_default.go`:
- Around line 118-123: Add a focused test for cachedHttpClientFunc that
constructs the cached HTTP client and verifies its transport chain contains
DebugHeaderTransport, RetryTransport, SecurityHeaderTransport,
EnvHeaderTransport, auth.SecurityPolicyTransport, and LogIDTransport in the
expected wrapping order. Keep the existing buildSDKTransport tests unchanged.
---
Outside diff comments:
In `@internal/cmdutil/transport_test.go`:
- Around line 100-123: Update the transport-chain assertions in
internal/cmdutil/transport_test.go at ranges 100-123, 132-159, and 167-190 so
each RetryTransport assertion verifies that RetryTransport.Base is a
DebugHeaderTransport before the underlying base transport. Preserve the existing
chain checks and failure diagnostics while adding this assertion in all three
affected test cases.
🪄 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: a34c245d-ba97-4f89-8003-459623d8e55e
📒 Files selected for processing (11)
env/larkenvinternal/cmdutil/debug_header_transport.gointernal/cmdutil/env_header_transport.gointernal/cmdutil/env_header_transport_test.gointernal/cmdutil/factory_default.gointernal/cmdutil/logid_transport.gointernal/cmdutil/logid_transport_test.gointernal/cmdutil/transport_test.gointernal/core/endpoint_override.gointernal/core/endpoint_override_test.gointernal/core/types.go
Comment on lines
+84
to
+109
| apply_env() { | ||
| export LARKSUITE_CLI_CONFIG_DIR="$CONFIG_ROOT/config/$1" | ||
| case "$1" in | ||
| boe) | ||
| export LARKSUITE_CLI_ENDPOINT_DOMAIN="feishu-boe.cn" | ||
| # boe 是内网域名，必须绕开公司外部 relay 代理 | ||
| export LARK_CLI_NO_PROXY=1 | ||
| ;; | ||
| pre) | ||
| export LARKSUITE_CLI_ENDPOINT_DOMAIN="feishu-pre.cn" | ||
| ;; | ||
| online) | ||
| # 不覆盖 endpoint，走开源默认 feishu.cn / larksuite.com；仅隔离配置目录 | ||
| ;; | ||
| esac | ||
| # Append rather than overwrite, so a caller-supplied LARKSUITE_CLI_EXTRA_HEADERS | ||
| # (e.g. "x-use-boe: 1") survives alongside LARK_LANE. | ||
| if [ -n "${LARK_LANE:-}" ]; then | ||
| if [ -n "${LARKSUITE_CLI_EXTRA_HEADERS:-}" ]; then | ||
| export LARKSUITE_CLI_EXTRA_HEADERS="${LARKSUITE_CLI_EXTRA_HEADERS}; X-TT-ENV: $LARK_LANE" | ||
| else | ||
| export LARKSUITE_CLI_EXTRA_HEADERS="X-TT-ENV: $LARK_LANE" | ||
| fi | ||
| fi | ||
| mkdir -p "$LARKSUITE_CLI_CONFIG_DIR" | ||
| } |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Reject unsupported environment names.
init and login accept arbitrary values here. For example, init ../../tmp escapes CONFIG_ROOT, while init staging silently uses online endpoints. Restrict this function to boe, pre, and online, returning a failure for anything else.
🤖 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 `@env/larkenv` around lines 84 - 109, The apply_env function currently accepts
unsupported environment names, enabling config-directory traversal and
unintended endpoint defaults. Add validation at the start of apply_env so only
boe, pre, and online are accepted; for any other value, emit a failure return
before exporting LARKSUITE_CLI_CONFIG_DIR or creating directories, while
preserving the existing handling for supported environments.
Comment on lines
+187
to
+194
| printf 'App Secret (输入不回显): ' >&2 | ||
| stty -echo 2>/dev/null || true | ||
| read -r secret | ||
| stty echo 2>/dev/null || true | ||
| printf '\n' >&2 | ||
| # --app-secret-stdin keeps the secret out of the process list. | ||
| printf '%s' "$secret" | "$BIN" config init --app-id "$3" --app-secret-stdin | ||
| unset secret |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Preserve the secret and always restore terminal echo.
read -r trims leading/trailing IFS whitespace, and an interrupt after stty -echo exits with echo disabled. Use IFS= read -r secret and a cleanup trap that restores echo on exit/signals.
🤖 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 `@env/larkenv` around lines 187 - 194, Update the secret-input flow in the
app-secret configuration block to use IFS= read -r so leading and trailing
whitespace is preserved. Add cleanup trap handling around stty -echo that
restores terminal echo on normal completion and signals, while retaining the
existing stdin-based secret submission and cleanup.
Comment on lines
+23
to
+30
| // sensitiveHeaders are redacted in the dump; their presence is still reported. | ||
| var sensitiveHeaders = map[string]bool{ | ||
| "authorization": true, | ||
| "x-lark-mcp-uat": true, | ||
| "x-lark-mcp-tat": true, | ||
| "cookie": true, | ||
| "x-larksuite-key": true, | ||
| } |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Redact Proxy-Authorization too.
A request carrying that standard credential header is printed verbatim when debug logging is enabled, contradicting the redaction guarantee. Add it to sensitiveHeaders and cover it with a regression test.
🤖 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/cmdutil/debug_header_transport.go` around lines 23 - 30, Add
"proxy-authorization" to the sensitiveHeaders map used by the debug transport so
this credential is redacted while its presence remains reported. Add a
regression test covering a request with Proxy-Authorization and verify the debug
output does not expose its value.
Source: Coding guidelines
Comment on lines
+22
to
+35
| func parseExtraHeaders(raw string) map[string]string { | ||
| out := map[string]string{} | ||
| for _, part := range strings.Split(raw, ";") { | ||
| i := strings.Index(part, ":") | ||
| if i < 0 { | ||
| continue | ||
| } | ||
| k := strings.TrimSpace(part[:i]) | ||
| v := strings.TrimSpace(part[i+1:]) | ||
| if k != "" { | ||
| out[k] = v | ||
| } | ||
| } | ||
| return out |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Reject malformed extra-header configuration instead of silently changing it.
Malformed segments and blank keys are currently discarded, so a misspelled lane header can send requests without the requested routing. Return a typed validation error and test that failure.
internal/cmdutil/env_header_transport.go#L22-L35: make malformed segments/header names fail instead of skipping them.internal/cmdutil/env_header_transport_test.go#L26-L31: replace silent-skip expectations with assertions for the validation error.
📍 Affects 2 files
internal/cmdutil/env_header_transport.go#L22-L35(this comment)internal/cmdutil/env_header_transport_test.go#L26-L31
🤖 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/cmdutil/env_header_transport.go` around lines 22 - 35, Update
parseExtraHeaders in internal/cmdutil/env_header_transport.go to reject
malformed segments and blank header names by returning the established typed
validation error instead of silently skipping them; adjust
internal/cmdutil/env_header_transport_test.go lines 26-31 to assert that
malformed input returns this validation error rather than being omitted.
Source: Coding guidelines
Comment on lines
+118
to
+123
| rt = &DebugHeaderTransport{Base: rt} | ||
| rt = &RetryTransport{Base: rt} | ||
| rt = &SecurityHeaderTransport{Base: rt} | ||
| rt = &EnvHeaderTransport{Base: rt} | ||
| rt = &auth.SecurityPolicyTransport{Base: rt} // Add our global response interceptor | ||
| rt = &LogIDTransport{Base: rt} |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Add a direct test for the cached HTTP client chain.
The generic client gained three middleware layers, but the updated tests only inspect buildSDKTransport. Assert this chain directly so removing a layer from cachedHttpClientFunc fails a test.
🤖 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/cmdutil/factory_default.go` around lines 118 - 123, Add a focused
test for cachedHttpClientFunc that constructs the cached HTTP client and
verifies its transport chain contains DebugHeaderTransport, RetryTransport,
SecurityHeaderTransport, EnvHeaderTransport, auth.SecurityPolicyTransport, and
LogIDTransport in the expected wrapping order. Keep the existing
buildSDKTransport tests unchanged.
Source: Coding guidelines
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
Lets the CLI be retargeted at a non-production environment without adding an
--envflag, so the command surface is unchanged and production paths keep their existing brand-based resolution. Everything is driven by four environment variables that are inert when unset.Changes
LARKSUITE_CLI_ENDPOINT_DOMAIN— set to a bare domain to override every resolved endpoint (open/accounts/mcp/applink). Short-circuitscore.ResolveEndpointsahead of brand resolution.LARKSUITE_CLI_EXTRA_HEADERS— injects arbitraryKey: Valueheaders (;-separated) into outbound requests. Wired into both the HTTP client chain and the SDK transport chain, so shortcuts, API commands, and SDK calls all carry them.LARK_CLI_SHOW_LOGID— surfaces thex-tt-logidresponse header to stderr for request correlation. Prints on HTTP errors (>= 400) by default; prints for every response when enabled.LARK_CLI_DEBUG_HEADERS— dumps the final outbound request line and headers from the innermost transport layer, after every other layer has contributed.Authorization/Cookieand similar are redacted.env/larkenv— a wrapper script that composes the above: one setup command, per-environment config isolation under~/.lark-cli-env, and QR-code login so authorization does not inherit whatever account the default browser is signed in as.internal/cmdutil/transport_test.go— existing chain-composition assertions updated for the two new layers.Test Plan
lark-cli <domain> <command>flow works as expectedgo build ./...,go vet ./...,gofmt -l .clean;go mod tidyproduces no diffgolangci-lint run --new-from-rev=483aadee— 0 issuesgo test -race ./...run in a clean worktree against the same baseline: identical failure sets, no new failuresRelated Issues
🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Bug Fixes