fix(credential): mint env app-secret tenant tokens#1679
Open
bezhai wants to merge 1 commit into
Open
Conversation
|
|
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 (3)
🚧 Files skipped from review as they are similar to previous changes (3)
📝 WalkthroughWalkthrough
ChangesTAT fallback from app secret
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Suggested reviewers
Possibly related PRs
Poem
🚥 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 `@internal/credential/credential_provider.go`:
- Around line 73-95: The TAT minting path in credential_provider.go is caching
failures because s.tatOnce.Do in the token retrieval flow makes s.tatErr sticky
after the first error. Update the logic around the GetToken/FetchTAT path so
only a successful token is memoized (via s.tatResult), while transient errors
from httpClient, FetchTAT, or empty token cases do not permanently poison the
source; on later calls, the code should retry minting with a healthy context
instead of immediately returning the stored error.
- Around line 74-81: Wrap the `CredentialProvider.ResolveToken` TAT failure path
so `httpClient()` and `FetchTAT()` errors return typed `errs.*` values instead
of raw errors. In `ResolveToken`, keep already-typed errors unchanged, and for
untyped transport/network failures convert them with
`errs.NewNetworkError(errs.SubtypeNetworkTransport, ...)` while attaching the
original error via `.WithCause(err)`. Make sure the `s.tatErr` assignment
preserves the wrapped error so the CLI’s category/subtype contract and
`errors.Is`/`errors.Unwrap` behavior remain intact.
🪄 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: 3403586c-e87b-4e30-b2aa-3a1debffbac1
📒 Files selected for processing (3)
internal/credential/credential_provider.gointernal/credential/credential_provider_test.gointernal/credential/integration_test.go
bezhai
force-pushed
the
fix/env-app-secret-tat
branch
from
June 30, 2026 12:20
0665f70 to
9176a76
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 `@internal/credential/credential_provider_test.go`:
- Around line 288-293: The error-path assertion for `ResolveToken` is reading
typed metadata directly from `*errs.NetworkError`; update the test to validate
the CLI typed-error contract via `errs.ProblemOf` instead. Use `errs.ProblemOf`
to assert the Problem-level fields exposed by the returned error, especially
category and subtype, and keep the existing cause-preservation checks intact.
Remove the direct `errors.As`/`netErr.Subtype` dependence so the test matches
the intended typed metadata API.
In `@internal/credential/credential_provider.go`:
- Around line 69-82: The app-secret fallback in
extensionTokenSource.resolveTATFromAppSecret is matching only on token type and
account state, but it should also require the requested TokenSpec.AppID to match
s.account.AppID before minting a TAT. Update the guard in
resolveTATFromAppSecret to return early when req.AppID does not equal the
selected account’s AppID, so FetchTAT is only called for the intended app and
the cached s.tatResult cannot be reused for a different app request.
🪄 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: e2fee3ee-910c-4620-8559-81d615c7304d
📒 Files selected for processing (3)
internal/credential/credential_provider.gointernal/credential/credential_provider_test.gointernal/credential/integration_test.go
🚧 Files skipped from review as they are similar to previous changes (1)
- internal/credential/integration_test.go
bezhai
force-pushed
the
fix/env-app-secret-tat
branch
from
June 30, 2026 12:27
9176a76 to
67e4754
Compare
There was a problem hiding this comment.
♻️ Duplicate comments (1)
internal/credential/credential_provider_test.go (1)
253-282: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winAssert the typed contract for the mismatched-App error path.
Lines 276-278 only check for a non-nil error, so this test would still pass if
ResolveTokenstarted returning the wrong typed problem. Please assert the returned problem metadata witherrs.ProblemOfhere as well (and useerrors.Asonly if you also need to checkParam). As per coding guidelines, “Error-path tests must assert typed metadata viaerrs.ProblemOf(category/subtype/param) and cause preservation, not message substrings alone.” Based on learnings,errs.ProblemOfexposes only Problem-level fields, soParamassertions neederrors.Ason the concrete validation error type.🤖 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/credential_provider_test.go` around lines 253 - 282, The mismatched-App error assertion in TestCredentialProvider_SelectedExtensionAppSecretDoesNotMintTATForDifferentApp is too weak because it only checks for a non-nil error. Update the ResolveToken error-path check to verify the returned typed problem with errs.ProblemOf for the expected category/subtype, and preserve cause/param validation as needed using errors.As on the concrete validation error type. Keep the existing setup around ResolveAccount and ResolveToken, but make the failure assertion strict enough to catch the wrong problem type.Sources: Coding guidelines, Learnings
🤖 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.
Duplicate comments:
In `@internal/credential/credential_provider_test.go`:
- Around line 253-282: The mismatched-App error assertion in
TestCredentialProvider_SelectedExtensionAppSecretDoesNotMintTATForDifferentApp
is too weak because it only checks for a non-nil error. Update the ResolveToken
error-path check to verify the returned typed problem with errs.ProblemOf for
the expected category/subtype, and preserve cause/param validation as needed
using errors.As on the concrete validation error type. Keep the existing setup
around ResolveAccount and ResolveToken, but make the failure assertion strict
enough to catch the wrong problem type.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 7dee1b9c-9ae6-4caa-b154-5b891b5d3aa0
📒 Files selected for processing (3)
internal/credential/credential_provider.gointernal/credential/credential_provider_test.gointernal/credential/integration_test.go
🚧 Files skipped from review as they are similar to previous changes (2)
- internal/credential/integration_test.go
- internal/credential/credential_provider.go
bezhai
force-pushed
the
fix/env-app-secret-tat
branch
from
June 30, 2026 13:03
67e4754 to
48ff8d3
Compare
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
Fix env-backed bot authentication so
LARKSUITE_CLI_APP_ID+LARKSUITE_CLI_APP_SECRETcan mint a tenant access token instead of failing withtoken_missing.Changes
Test Plan
make unit-testlark-cli wiki +space-list --as botflow reaches the token endpoint with env app credentials and no longer fails locally withtoken_missinggo vet ./...gofmt -l internal/credential/credential_provider.go internal/credential/credential_provider_test.go internal/credential/integration_test.gogo mod tidyRelated Issues
Summary by CodeRabbit