feat: validate credentials after config init#1151
Open
dc-bytedance wants to merge 7 commits into
Open
Conversation
|
|
github-actions
Bot
added
the
size/L
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughCentralizes tenant-access-token exchange in 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:
|
Previously a deterministic credential-rejection signal from the post-init probe was printed as a best-effort stderr warning with exit 0. An AI-agent caller that keys off exit code and stdout could miss it entirely. Propagate the *errs.AuthenticationError up through RunE instead: config init now exits 3 with a structured authentication error envelope when the saved App ID / App Secret are rejected by the open platform. The error is re-wrapped with a jargon-free message and an actionable hint, preserving the upstream code and chaining the original cause. Scope is unchanged from the empirical-lane policy: only AuthenticationError (any non-zero TAT body code or HTTP 401/403) propagates. Ambiguous failures (transport, 5xx, JSON parse, timeout, http-client init) stay silent and non-blocking, so valid configurations are never disturbed by upstream noise. The saved config is intentionally not rolled back; the stdout envelope still records what was saved, while exit 3 + stderr report that it does not authenticate.
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, hard-to-miss 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 stays silent on anything ambiguous (network errors, server 5xx, unknown transport failures, timeout) so valid configurations are never disturbed by upstream noise.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, bounded by a single 3-secondcontext.WithTimeout. WhenFetchTATreturns*errs.AuthenticationError(a deterministic credential-rejection signal — any non-zero TAT body code, or HTTP 401/403),runProbereturns a re-wrapped*errs.AuthenticationErrorsoconfig initexits 3 with a structured authentication error envelope. The re-wrap carries a jargon-free message (configured credentials may be invalid: please verify the App ID and App Secret) + an actionableHint, preserves the upstreamCode, and chains the original viaCause. Every other outcome — ambiguous failures (NetworkError/InternalError: transport, 5xx, parse, timeout, http-client init) and probe-endpoint outcomes — returnsnil, keepingconfig initsilent and exit-0.runProbeinto all fourcmd/config/init.gopaths (non-interactive,--new, interactive TUI, legacy readline) asif err := runProbe(...); err != nil { return err }. Probe is skipped when the user reused an existing secret (no fresh plaintext in memory). The saved config is intentionally not rolled back on rejection — the stdout envelope records what was saved, while exit 3 + stderr report that it does not authenticate.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"), any-non-zero-body-code coverage (e.g.99999→ still propagates), HTTP 401/403, silent-and-nil ambiguous cases (HTTP 5xx, transport error, parse failure, http-client init error, timeout), probe request shape (URL, method, Bearer header, JSON body withfrom: "lark-cli/<version>"), and brand routing. Each credential-rejection case asserts a propagated*errs.AuthenticationErrorwith the expectedCode; an invariant assertsrunProbewrites nothing to stderr.Test Plan
make unit-testpassed (cmd/configincl. 15TestRunProbe_*+internal/credentialTestFetchTAT_*, under-race)make build+go vet ./...+gofmtcleantests_e2e/config/passed — verified viaLARK_CLI_BIN=$PWD/lark-cli go test ./tests_e2e/config/against the live endpoint, includingTestConfigInit_CredentialRejection_ExitsThree(real endpoint rejects a fake App ID → exit 3 + typed authentication envelope). The Docker eval sandbox was attempted first but failed at its ownauth statussetup gate (environmental, unrelated to this change), so the E2E suite was run directly against the real binary instead.type: authenticationenvelope with the jargon-free message, stdout still carries the saved-config JSON; (2) network-unreachable → silent, exit 0 (regression guard); (3)--brand larkroutes to open.larksuite.com (confirmed at the CONNECT layer); (4) real App ID + wrong secret → exit 3code 10014; (5) §2.6 vocabulary clean in the envelope and both source files. Reviewer judged exit-3 + typed envelope a clear improvement over a missable stderr warning, and consistent with the rest of the CLI's auth-failure contract (a token-rejectedim +chat-listexits 3 with the same envelope shape).type: authenticationenvelope (code 10014), stdout still records the saved config JSONtype: authenticationenvelope (code 10003)HTTPS_PROXY=http://127.0.0.1:1): exit 0, stdout JSON, no error (transport failure stays silent per design)--brand larkwith wrong secret: exit 3,type: authenticationenvelope (lark host routing confirmed at the CONNECT layer)Consumer note
When
config initexits non-zero, key off the exit code, read stdout as the config record and stderr as the diagnostic. On a credential rejection, stdout still carries the saved-config JSON (the config was written) while stderr carries the typed error envelope and exit is 3 — these are not contradictory (saved ≠ authenticates). Do not parse stderr as a single JSON document: it may contain a leadingOK: Configuration savedline before the error envelope.Related Issues
N/A