feat: validate credentials after config init#1151
Open
dc-bytedance wants to merge 6 commits into
Open
Conversation
|
|
github-actions
Bot
added
the
size/L
|
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 (6)
🚧 Files skipped from review as they are similar to previous changes (6)
📝 WalkthroughWalkthroughThis PR introduces post-config credential probing to the ChangesPost-Config Credential Probing
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 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 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: 3
🤖 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 `@cmd/config/init_probe_test.go`:
- Around line 63-79: The test builds a Factory directly in fakeFactory; replace
that with the repo-standard TestFactory to ensure proper test isolation: in the
shared setup call t.Setenv("LARKSUITE_CLI_CONFIG_DIR", t.TempDir()) and create
the factory via cmdutil.TestFactory(t, config) instead of manually constructing
&cmdutil.Factory and HttpClient; update fakeFactory (and the similar block at
the other occurrence around lines 255-266) to return the factory and error
buffer from cmdutil.TestFactory so tests use the canonical TestFactory and
isolated config dir.
In `@internal/credential/tat_fetch.go`:
- Around line 23-31: The comment block describing error classification in
internal/credential/tat_fetch.go is out of sync: update the line that currently
states “other non-2xx → *errs.NetworkError” to match the runtime behavior where
non-2xx responses (excluding 401/403 and 5xx) are returned as
*errs.InternalError (subtype "sdk_error"); keep the existing rules for
*errs.AuthenticationError (401/403 or body code via Problem.Code) and
*errs.NetworkError (5xx or transport failures wrapped via Cause) so callers see
the same contract as the code path implemented around the non-2xx handling in
the block that returns *errs.InternalError (see the code near the current return
sites for *errs.InternalError in the 112-119 region).
- Around line 137-147: When parsed.Code == 0 you must also validate
parsed.TenantAccessToken is non-empty; if TenantAccessToken is empty, return an
internal SDK error instead of a successful empty token. Update the success
branch in tat_fetch.go (the block that currently returns
parsed.TenantAccessToken) to check parsed.TenantAccessToken (and/or
parsed.TenantAccessToken == "") and return an errs.InternalError (populate
errs.Problem with Category: errs.CategoryInternal, an appropriate Subtype such
as errs.SubtypeSDK, and a clear Message like "missing tenant_access_token on TAT
API success") so callers fail fast with a clear cause.
🪄 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: d60a3a6b-dc9b-4969-b50f-cab448016cba
📒 Files selected for processing (6)
cmd/config/init.gocmd/config/init_probe.gocmd/config/init_probe_test.gointernal/credential/default_provider.gointernal/credential/tat_fetch.gointernal/credential/tat_fetch_test.go
🚀 PR Preview Install Guide🧰 CLI updatenpm i -g https://pkg.pr.new/larksuite/cli/@larksuite/cli@72dabf540f8f3583a1888bdd93f56e821b1d28c0🧩 Skill updatenpx skills add dc-bytedance/cli#feat/config-probe -y -g |
Drop the ad-hoc *HTTPStatusError and classify TAT outcomes via the
canonical errs.* taxonomy:
* HTTP 401/403 + non-zero TAT body code → *errs.AuthenticationError
with Problem.Code carrying the upstream signal
* HTTP 5xx → *errs.NetworkError with
CauseKind "5xx" and
Retryable=true
* 4xx other than 401/403, JSON parse,
or 2xx with empty tenant_access_token → *errs.InternalError with
subtype "sdk_error"
* Transport / DNS / TLS / context cancel → *errs.NetworkError wrapping
the underlying cause
classifyTATError now extracts the typed signal via errors.As, removing
the parallel apiCode return.
Test factory switched to cmdutil.TestFactory(t, nil) with
LARKSUITE_CLI_CONFIG_DIR=t.TempDir() per repo guideline, then HttpClient
overridden with stubRoundTripper to keep per-test response control.
dc-bytedance
force-pushed
the
feat/config-probe
branch
from
e300c0e to
72dabf5
Compare
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #1151 +/- ##
==========================================
+ Coverage 68.77% 68.82% +0.04%
==========================================
Files 628 630 +2
Lines 58670 58777 +107
==========================================
+ Hits 40353 40455 +102
Misses 15021 15021
- Partials 3296 3301 +5 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
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
After
lark-cli config initsaves credentials, validate them against the open platform so that users get immediate feedback when they typed the wrong App ID or App Secret, instead of discovering it later from a failed business request. The check is best-effort: it never blocks the command, never changes the exit code, and stays silent on anything ambiguous (network errors, server 5xx, unknown response codes, timeout).Changes
internal/credential/tat_fetch.go::FetchTAT— a pure-HTTP TAT request helper (no config / no keychain access). Returns typederrs.*errors per the §984 contract:*errs.AuthenticationErrorfor HTTP 401/403 or any non-zero TAT body code,*errs.NetworkErrorfor transport failures and HTTP 5xx (withCauseKind: "5xx"+Retryable: true),*errs.InternalErrorfor 4xx-non-401/403 / JSON parse failures / marshal failures / empty-token-on-success.internal/credential/default_provider.go::doResolveTATto delegate toFetchTAT— observable behaviour unchanged for existing TAT consumers; the underlying transport path now yields typed errors which propagate up the credential chain.cmd/config/init_probe.go::runProbe— best-effort post-init validator. Runs both calls under a single 3-secondcontext.WithTimeout. The warning is fired wheneverFetchTATreturns*errs.AuthenticationError(the typed lane that carries deterministic credential-rejection signals from the upstream API); other lanes (NetworkError/InternalError) stay silent by construction. One stderr line is emitted in that case:warning: configured credentials may be invalid (code <LABEL>): please verify App ID and App Secret in lark-cli config.<LABEL>is the TAT body code (e.g.10014when the App Secret is wrong,10003when the App ID is invalid) orHTTP 401/HTTP 403when the request was rejected pre-body.runProbeinto all fourcmd/config/init.gopaths (non-interactive,--new, interactive TUI, legacy readline). Probe is skipped when the user reused an existing secret (no fresh plaintext in memory).internal/credential/tat_fetch_test.gocovers success, body-code rejection, HTTP 401/403, HTTP 5xx, non-401/403 4xx (400/404/415/429), transport error, parse error, empty-token-on-success, brand routing, context cancel — each asserts the specific typed error and key fields.cmd/config/init_probe_test.go(15 black-box tests) covers the real-world TAT body codes (10014"invalid app secret",10003"invalid param"), unknown-body-code coverage (any non-zero code triggers a warning), HTTP 401/403, silent-on-ambiguous cases (HTTP 5xx, transport error, unknown HTTP, parse failure, HTTP client error), probe request shape (URL, method, Bearer header, JSON body withfrom: "lark-cli/<version>"), brand routing, and 3s timeout.Test Plan
make unit-testpassedmake build+go vet ./...passedtests_e2e/config/, skillave N/A — no shortcut/skill/meta-API changes){"appId":"...","appSecret":"****","brand":"feishu"}, stderrOK: Configuration savedthenwarning: configured credentials may be invalid (code 10014): please verify App ID and App Secret in lark-cli config.warning: ... (code 10003): ...OK:only, no warning, ~0.8s totalHTTPS_PROXY=http://127.0.0.1:1): exit 0, JSON, no probe warning (transport error stays silent per design)--brand larkwith wrong secret: exit 0, JSON"brand":"lark", warning with TAT body code (lark host routing confirmed)Related Issues
N/A
Summary by CodeRabbit