feat(cmdutil): add X-Cli-Build header for CLI build classification#596
Merged
sang-neo03 merged 2 commits intomainfrom Apr 22, 2026
Merged
feat(cmdutil): add X-Cli-Build header for CLI build classification#596sang-neo03 merged 2 commits intomainfrom
sang-neo03 merged 2 commits intomainfrom
Conversation
Adds X-Cli-Build (official / extended / unknown) so the gateway can distinguish official CLI from ISV-repackaged builds.
📝 WalkthroughWalkthroughAdds build-kind detection and a new transport that injects an Changes
Sequence Diagram(s)sequenceDiagram
participant CLI
participant BuildHeader as BuildHeaderTransport
participant SecPolicy as SecurityPolicyTransport
participant Ext as ExtensionMiddleware
participant Server
CLI->>BuildHeader: HTTP Request
BuildHeader->>BuildHeader: set Header "X-Cli-Build" = DetectBuildKind()
BuildHeader->>SecPolicy: forward request
SecPolicy->>Ext: apply security / extension middleware
Ext->>Server: send network request
Server-->>CLI: response (via reverse chain)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 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 docstrings
🧪 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/M
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #596 +/- ##
==========================================
- Coverage 61.69% 59.88% -1.82%
==========================================
Files 402 404 +2
Lines 35073 42592 +7519
==========================================
+ Hits 21639 25505 +3866
- Misses 11429 15080 +3651
- Partials 2005 2007 +2 ☔ 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@486d9910547ad7767adf49d833349a1d9305c6b7🧩 Skill updatenpx skills add larksuite/cli#feat/cli-build-telemetry -y -g |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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)
internal/cmdutil/factory_default.go (1)
115-123:⚠️ Potential issue | 🟠 MajorAvoid freezing
DetectBuildKind()before the SDK request path runs.
lark.WithHeaders(BaseSecurityHeaders())now callsDetectBuildKind()while constructing the SDK client, sosync.Oncecan cacheofficialbefore a later provider registration. Then the newBuildHeaderTransportcannot correct the value at request time because it reads the already-cached result.Keep
X-Cli-Buildout of the static SDK headers and letBuildHeaderTransportbe the SDK path that callsDetectBuildKind()duringRoundTrip.Proposed fix
diff --git a/internal/cmdutil/secheader.go b/internal/cmdutil/secheader.go @@ func BaseSecurityHeaders() http.Header { + h := baseSecurityHeaders() + h.Set(HeaderBuild, DetectBuildKind()) + return h +} + +func baseSecurityHeaders() http.Header { h := make(http.Header) h.Set(HeaderSource, SourceValue) h.Set(HeaderVersion, build.Version) - h.Set(HeaderBuild, DetectBuildKind()) h.Set(HeaderUserAgent, UserAgentValue()) return h } diff --git a/internal/cmdutil/factory_default.go b/internal/cmdutil/factory_default.go @@ opts := []lark.ClientOptionFunc{ lark.WithEnableTokenCache(false), lark.WithLogLevel(larkcore.LogLevelError), - lark.WithHeaders(BaseSecurityHeaders()), + lark.WithHeaders(baseSecurityHeaders()), }Also applies to: 131-137
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/cmdutil/factory_default.go` around lines 115 - 123, The SDK client is being constructed with lark.WithHeaders(BaseSecurityHeaders()) which triggers DetectBuildKind() early and caches the build kind; remove the X-Cli-Build header from the static headers returned by BaseSecurityHeaders() when creating the opts slice in factory_default.go and instead rely on BuildHeaderTransport (used by buildSDKTransport()/Transport) to call DetectBuildKind() and inject X-Cli-Build at request time in its RoundTrip implementation; update any code paths that append headers to opts (the lark.WithHeaders call) to pass only static security headers and ensure BuildHeaderTransport remains responsible for setting the dynamic X-Cli-Build header.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@internal/cmdutil/secheader_test.go`:
- Line 25: The test intends to verify classification ignores Name(), but the
test-local provider's Name() currently returns "local" so it doesn't spoof;
change the test doubles' Name() implementations (e.g.,
cmdutilLocalProvider.Name()) to return an obviously external/non-builtin value
such as "external-local" or "spoofed-name" so the test truly exercises
Name-based spoofing; update the other test provider Name() occurrences
referenced in the same test (the other block around lines 54-64) to the same
external value to ensure the test fails on any regression that relies on Name().
---
Outside diff comments:
In `@internal/cmdutil/factory_default.go`:
- Around line 115-123: The SDK client is being constructed with
lark.WithHeaders(BaseSecurityHeaders()) which triggers DetectBuildKind() early
and caches the build kind; remove the X-Cli-Build header from the static headers
returned by BaseSecurityHeaders() when creating the opts slice in
factory_default.go and instead rely on BuildHeaderTransport (used by
buildSDKTransport()/Transport) to call DetectBuildKind() and inject X-Cli-Build
at request time in its RoundTrip implementation; update any code paths that
append headers to opts (the lark.WithHeaders call) to pass only static security
headers and ensure BuildHeaderTransport remains responsible for setting the
dynamic X-Cli-Build header.
🪄 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: 4c6ef540-9d69-49f6-a2ce-e44eef024025
📒 Files selected for processing (6)
internal/cmdutil/factory_default.gointernal/cmdutil/secheader.gointernal/cmdutil/secheader_sidecar_test.gointernal/cmdutil/secheader_test.gointernal/cmdutil/transport.gointernal/cmdutil/transport_test.go
Extract classifyBuild as a pure helper so every branch (unknown / extended main-path / extended credential / extended transport / extended fileio / official) is reachable without mutating process-wide provider registries. Also cover: isBuiltinProvider non-pointer values, BuildHeaderTransport nil-Base fallback path, and fix the Name-spoof test so the test double returns a value that actually mimics an ISV provider. Coverage on PR-changed functions: - classifyBuild: 100% (new) - computeBuildKind: 61.5% -> 93.3% - BuildHeaderTransport.RoundTrip: 80% -> 100%
sang-neo03
force-pushed
the
feat/cli-build-telemetry
branch
from
b4d6b3a to
486d991
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (1)
internal/cmdutil/secheader_test.go (1)
230-236: Optional: strengthen thesync.Oncecaching assertion.
TestDetectBuildKind_StableAcrossCallsonly checks that two calls return equal values, which is also true for a non-cached pure function. If you want this test to actually pin thesync.Oncecontract (and not just value stability), consider asserting the cached variable is populated after the first call, or at minimum documenting that stability — not caching — is what's being verified here. Not blocking.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/cmdutil/secheader_test.go` around lines 230 - 236, The test only verifies value stability but not that DetectBuildKind actually caches via sync.Once; update TestDetectBuildKind_StableAcrossCalls to assert the caching behavior by checking the package-level cache is populated after the first call (for example assert the cached buildKind variable is non-empty or set) or, if that variable is unexported/unavailable, change the test to explicitly document that it only verifies stability (or add a test helper that exposes the cached state) — make changes referencing DetectBuildKind and the sync.Once-backed cache so the test proves the sync.Once contract rather than just equal return values.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@internal/cmdutil/secheader_test.go`:
- Around line 230-236: The test only verifies value stability but not that
DetectBuildKind actually caches via sync.Once; update
TestDetectBuildKind_StableAcrossCalls to assert the caching behavior by checking
the package-level cache is populated after the first call (for example assert
the cached buildKind variable is non-empty or set) or, if that variable is
unexported/unavailable, change the test to explicitly document that it only
verifies stability (or add a test helper that exposes the cached state) — make
changes referencing DetectBuildKind and the sync.Once-backed cache so the test
proves the sync.Once contract rather than just equal return values.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 669549f0-1578-4132-9bf5-5e21d95e45c3
📒 Files selected for processing (3)
internal/cmdutil/secheader.gointernal/cmdutil/secheader_test.gointernal/cmdutil/transport_test.go
🚧 Files skipped from review as they are similar to previous changes (2)
- internal/cmdutil/transport_test.go
- internal/cmdutil/secheader.go
liangshuo-1
approved these changes
Apr 22, 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
Adds
X-Cli-Build(official/extended/unknown) so the gateway can tell the official CLI apart from ISV-repackaged builds. Detection runs at request time (not frominit()) to avoid misclassifying extended builds whose providers register later.Changes
secheader.go: addHeaderBuildconstant,DetectBuildKind()(cached viasync.Once),computeBuildKind()(main-module-path + non-builtin provider check), andisBuiltinProvider()usingreflect.Type.PkgPathsoName()can't spoof classification.BaseSecurityHeaders()now emitsX-Cli-Buildalongside existing headers.transport.go: addBuildHeaderTransportthat force-writesX-Cli-Buildon every request, protecting the SDK chain whereSecurityHeaderTransportisn't installed.factory_default.go: wireBuildHeaderTransportintobuildSDKTransport().authsidecartag), and transport header injection.Test Plan
go test ./internal/cmdutil/... -count=1passeshttpbin.org/anythingover the real network, for bothofficialandextendedvaluesstringson the built binary showsX-Cli-Build/DetectBuildKind/computeBuildKindsymbols are compiled inRelated Issues
Summary by CodeRabbit