feat: add transport extension with interceptor pre/post hooks#292
Merged
feat: add transport extension with interceptor pre/post hooks#292
Conversation
Add extension/transport package following the same Provider pattern as credential and fileio extensions. The Interceptor interface uses a PreRoundTrip/post-closure design that guarantees built-in transport decorators (SecurityHeader, SecurityPolicy, Retry) cannot be skipped, overridden, or tampered with by extensions. The original request context is restored after PreRoundTrip to prevent context tampering. Change-Id: I2e51ff67a0e2d8d32944a0565c2a6781110f281f Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
github-actions
Bot
added
the
size/M
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
✅ Files skipped from review due to trivial changes (1)
📝 WalkthroughWalkthroughIntroduces a transport extension API (Provider, Interceptor), a mutex‑guarded global provider registry with Register/GetProvider, wraps default and SDK HTTP transports with an extension middleware that invokes PreRoundTrip/post hooks, and adds unit tests for registry and integration behavior. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Registry as "exttransport.Registry"
participant Provider
participant Interceptor
participant BaseTransport as "base RoundTripper"
Client->>Registry: GetProvider()
Registry-->>Client: provider or nil
alt provider exists
Client->>Provider: ResolveInterceptor(ctx)
Provider-->>Client: interceptor or nil
end
alt interceptor exists
Client->>Interceptor: PreRoundTrip(req)
Interceptor-->>Client: postHook
Client->>Client: restore original req.Context()
Client->>BaseTransport: RoundTrip(req)
BaseTransport-->>Client: resp, err
Client->>Interceptor: postHook(resp, err)
else
Client->>BaseTransport: RoundTrip(req)
BaseTransport-->>Client: resp, err
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning Review ran into problems🔥 ProblemsTimed out fetching pipeline failures after 30000ms Comment |
Verify execution order (Pre/Post hooks called), security header override protection, custom header passthrough, and context tamper prevention. Change-Id: I8d126d777903e967bb5b9a1f3a07ba125f072ec6 Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
🚀 PR Preview Install Guide🧰 CLI updatenpm i -g https://pkg.pr.new/larksuite/cli/@larksuite/cli@f6ca1bbff41e7c724b9242e98d59b5094dc1f110🧩 Skill updatenpx skills add larksuite/cli#feat/transport-extension -y -g |
Change-Id: I8c2265ef6bfbf6a7149df2b92db9fae2e1700c1c Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (3)
extension/transport/registry.go (1)
8-28: Considersync.RWMutexfor better read concurrency.The registry is typically written once during
init()and read many times during transport construction. Usingsync.RWMutexwithRLock()forGetProvider()would allow concurrent reads without blocking.However, since
wrapWithExtensionis called withinsync.OnceValues(transport is cached), this is unlikely to be a performance concern in practice.♻️ Optional: Use RWMutex for read-heavy access pattern
var ( - mu sync.Mutex + mu sync.RWMutex provider Provider ) func Register(p Provider) { mu.Lock() defer mu.Unlock() provider = p } func GetProvider() Provider { - mu.Lock() - defer mu.Unlock() + mu.RLock() + defer mu.RUnlock() return provider }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@extension/transport/registry.go` around lines 8 - 28, The registry uses a sync.Mutex (mu) which serializes all access; change mu to a sync.RWMutex, keep using mu.Lock()/mu.Unlock() in Register(provider) for writes, and switch GetProvider() to use mu.RLock()/mu.RUnlock() so multiple readers can concurrently call GetProvider() while still protecting the shared variable provider.extension/transport/registry_test.go (1)
25-33: Extract repeated reset logic into a test helper.Each test manually resets the global
providerunder lock. Consider extracting this to a helper function for cleaner tests and to ensure consistent reset behavior.♻️ Optional: Add reset helper function
+func resetProvider(t *testing.T) { + t.Helper() + mu.Lock() + provider = nil + mu.Unlock() +} + func TestGetProvider_NilByDefault(t *testing.T) { - mu.Lock() - provider = nil - mu.Unlock() + resetProvider(t) if got := GetProvider(); got != nil { t.Fatalf("expected nil, got %v", got) } }Apply similarly to other test functions.
Also applies to: 35-47, 49-63, 65-77
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@extension/transport/registry_test.go` around lines 25 - 33, Extract the repeated locked reset logic into a test helper (e.g., resetProvider or withResetProvider) and call it from TestGetProvider_NilByDefault and the other tests; the helper should acquire mu, set provider = nil, and release mu to ensure consistent teardown before each test, then replace the inline mu.Lock()/provider = nil/mu.Unlock() blocks in tests that reference provider, mu, and GetProvider with calls to that helper.internal/cmdutil/factory_default_test.go (1)
110-124: Existing test relies on implicit nil state.
TestBuildSDKTransport_IncludesRetryTransportdoesn't explicitly reset the extension provider, relying on the defaultnilstate. Whilet.Cleanupin other tests should restore this, it's safer to explicitly set the expected state at the start of each test for robustness.♻️ Add explicit state reset for robustness
func TestBuildSDKTransport_IncludesRetryTransport(t *testing.T) { + exttransport.Register(nil) // ensure no extension is registered + transport := buildSDKTransport() sec, ok := transport.(*internalauth.SecurityPolicyTransport)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/cmdutil/factory_default_test.go` around lines 110 - 124, TestBuildSDKTransport_IncludesRetryTransport relies on an implicit nil extension provider; explicitly reset the extension provider to nil at the start of the test before calling buildSDKTransport to make the test robust. Add a statement that sets the package's extension provider to nil (using the same setter used in other tests or by assigning the provider variable to nil) at the top of TestBuildSDKTransport_IncludesRetryTransport and, if applicable, call t.Cleanup to restore the previous provider after the test so buildSDKTransport runs with the known nil state.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@extension/transport/registry_test.go`:
- Around line 25-33: Extract the repeated locked reset logic into a test helper
(e.g., resetProvider or withResetProvider) and call it from
TestGetProvider_NilByDefault and the other tests; the helper should acquire mu,
set provider = nil, and release mu to ensure consistent teardown before each
test, then replace the inline mu.Lock()/provider = nil/mu.Unlock() blocks in
tests that reference provider, mu, and GetProvider with calls to that helper.
In `@extension/transport/registry.go`:
- Around line 8-28: The registry uses a sync.Mutex (mu) which serializes all
access; change mu to a sync.RWMutex, keep using mu.Lock()/mu.Unlock() in
Register(provider) for writes, and switch GetProvider() to use
mu.RLock()/mu.RUnlock() so multiple readers can concurrently call GetProvider()
while still protecting the shared variable provider.
In `@internal/cmdutil/factory_default_test.go`:
- Around line 110-124: TestBuildSDKTransport_IncludesRetryTransport relies on an
implicit nil extension provider; explicitly reset the extension provider to nil
at the start of the test before calling buildSDKTransport to make the test
robust. Add a statement that sets the package's extension provider to nil (using
the same setter used in other tests or by assigning the provider variable to
nil) at the top of TestBuildSDKTransport_IncludesRetryTransport and, if
applicable, call t.Cleanup to restore the previous provider after the test so
buildSDKTransport runs with the known nil state.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 7d42898d-ddd7-495a-bfd2-fc8b6c2948ed
📒 Files selected for processing (6)
extension/transport/registry.goextension/transport/registry_test.goextension/transport/types.gointernal/cmdutil/factory_default.gointernal/cmdutil/factory_default_test.gointernal/cmdutil/transport.go
Greptile SummaryThis PR adds an Confidence Score: 5/5Safe to merge; the previous P1 (request not cloned before PreRoundTrip) has been resolved, and all remaining findings are non-blocking P2 suggestions. All findings are P2 or lower — a documentation gap about context.Background() semantics in ResolveInterceptor. No logic errors, security issues, or correctness problems remain. extension/transport/types.go warrants a minor godoc addition; all other files look correct. Important Files Changed
Sequence DiagramsequenceDiagram
participant Caller
participant EM as extensionMiddleware
participant EI as Extension Interceptor
participant SH as SecurityHeaderTransport
participant RT as RetryTransport
participant SRV as HTTP Server
Caller->>EM: RoundTrip(req)
EM->>EM: Clone req, save origCtx
EM->>EI: PreRoundTrip(clonedReq)
Note over EI: May set custom headers,\nstart trace span
EI-->>EM: post callback (or nil)
EM->>EM: req.WithContext(origCtx)\n(context tamper prevented)
EM->>SH: RoundTrip(req)
SH->>RT: RoundTrip(req + security headers)
RT->>SRV: HTTP request (with retry)
SRV-->>RT: response
RT-->>SH: response
SH-->>EM: response
EM->>EI: post(resp, err)\n(logging, span end, metrics)
EM-->>Caller: resp, err
Reviews (3): Last reviewed commit: "style: fix gofmt formatting in transport..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Change-Id: Iafa17d03f0ffad0830b92882e69147eba8249dd3 Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Change-Id: If060b8d27fee9563a9c62ddbc67ed60021e55364 Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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/factory_default_test.go`:
- Around line 246-248: Before installing the stub, capture the current global
provider and restore it in cleanup instead of always registering nil: call a
read/get of the current provider (or capture the return value if
exttransport.Register returns the previous provider) before calling
exttransport.Register(&stubTransportProvider{interceptor: ic}), and in t.Cleanup
use exttransport.Register(previousProvider) to restore that captured value;
update the test occurrences that call
exttransport.Register(&stubTransportProvider{...}) followed by t.Cleanup(func()
{ exttransport.Register(nil) }) (including the other spots mentioned) to use
this capture-and-restore pattern so tests do not clobber pre-existing global
state.
In `@internal/cmdutil/transport.go`:
- Line 119: The file has a formatting issue at the line containing the request
clone call (req = req.Clone(origCtx)) that fails gofmt; run gofmt (or goimports)
over internal/cmdutil/transport.go to fix whitespace/formatting so the line and
surrounding code conform to gofmt output, then re-run lint/CI and commit the
formatted file.
🪄 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: c2913ee2-da83-4157-a46b-5c71cf1880e2
📒 Files selected for processing (3)
extension/transport/registry_test.gointernal/cmdutil/factory_default_test.gointernal/cmdutil/transport.go
✅ Files skipped from review due to trivial changes (1)
- extension/transport/registry_test.go
liangshuo-1
approved these changes
Apr 7, 2026
tuxedomm
added a commit
that referenced
this pull request
Apr 8, 2026
* feat: add transport extension with interceptor pre/post hooks Add extension/transport package following the same Provider pattern as credential and fileio extensions. The Interceptor interface uses a PreRoundTrip/post-closure design that guarantees built-in transport decorators (SecurityHeader, SecurityPolicy, Retry) cannot be skipped, overridden, or tampered with by extensions. The original request context is restored after PreRoundTrip to prevent context tampering. Change-Id: I2e51ff67a0e2d8d32944a0565c2a6781110f281f
2 tasks
This was referenced Apr 17, 2026
Merged
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.
Add extension/transport package following the same Provider pattern as credential and fileio extensions. The Interceptor interface uses a PreRoundTrip/post-closure design that guarantees built-in transport decorators (SecurityHeader, SecurityPolicy, Retry) cannot be skipped, overridden, or tampered with by extensions. The original request context is restored after PreRoundTrip to prevent context tampering.
Change-Id: I2e51ff67a0e2d8d32944a0565c2a6781110f281f
Summary
Changes
Test Plan
lark xxxcommand works as expectedRelated Issues
Summary by CodeRabbit
New Features
Tests