feat(auth): add external credential integration#2033
Open
evandance wants to merge 1 commit into
Open
Conversation
github-actions
Bot
added
domain/mail
|
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:
📝 WalkthroughWalkthroughThis PR adds Standard and Extended CLI editions with external credential configuration, process-based providers, proxy HTTP/WebSocket transports, identity-aware events, diagnostics, secure file URL validation, release installers, and Extended update support. ChangesExtended edition and external credential platform
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ 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 |
🚀 PR Preview Install Guide🧰 CLI updatenpm i -g https://pkg.pr.new/larksuite/cli/@larksuite/cli@a7d2747f845d443c9c5cc742a3461942a14b9b52🧩 Skill updatenpx skills add larksuite/cli#feat/external-credential-platform -y -g |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #2033 +/- ##
==========================================
- Coverage 75.06% 74.97% -0.10%
==========================================
Files 905 924 +19
Lines 96048 97735 +1687
==========================================
+ Hits 72101 73272 +1171
- Misses 18397 18730 +333
- Partials 5550 5733 +183 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 13
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
.goreleaser.yml (1)
14-37: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winExclude unsupported RISC-V target combinations.
Go does not support
darwin/riscv64orwindows/riscv64, and GoReleaser expands both builds into the full OS/architecture matrix. Addignoreentries for those pairs in bothstandardandextended, or list only supportedtargets; otherwise the release builds are likely to fail before archives are produced.Proposed configuration
goarch: - amd64 - arm64 - riscv64 + ignore: + - goos: darwin + goarch: riscv64 + - goos: windows + goarch: riscv64🤖 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 @.goreleaser.yml around lines 14 - 37, Add exclusions for the unsupported darwin/riscv64 and windows/riscv64 combinations in both the standard and extended GoReleaser build configurations. Keep linux/riscv64 and all currently supported OS/architecture combinations unchanged, using each build’s ignore entries or an equivalent supported-target list.
🧹 Nitpick comments (3)
cmd/doctor/doctor_test.go (1)
259-264: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winPrefer
cmdutil.TestFactoryover manual&cmdutil.Factory{}construction.
status_test.go's equivalent scenario (external credential provider override) usescmdutil.TestFactory(t, cfg)then overridesf.Credential. This test instead hand-builds the Factory with onlyConfig,Credential, andIOStreamsset, skipping the shared helper. As per path instructions, "Usecmdutil.TestFactory(t, config)for test factories."♻️ Suggested refactor
- out := &bytes.Buffer{} - f := &cmdutil.Factory{ - Config: func() (*core.CliConfig, error) { return cfg, nil }, - Credential: cred, - IOStreams: &cmdutil.IOStreams{Out: out, ErrOut: &bytes.Buffer{}}, - } + f, out, _, _ := cmdutil.TestFactory(t, cfg) + f.Credential = cred🤖 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 `@cmd/doctor/doctor_test.go` around lines 259 - 264, Replace the manual cmdutil.Factory construction in the affected doctor test with cmdutil.TestFactory(t, cfg), then override its Credential field with cred and preserve the existing output stream setup as needed.Source: Path instructions
internal/identitydiag/diagnostics.go (1)
438-476: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick win
fetchExternalUserInfoloses HTTP status on non-JSON error bodies.Unlike its sibling
fetchBotInfo(which unmarshals first but always surfacesHTTP %dwhen status is bad, even if parsing fails),fetchExternalUserInforeturns immediately on any JSON decode error before checkingresp.StatusCode. A non-2xx response with a non-JSON body (gateway timeout, proxy auth failure returning HTML/plain text, empty body) will surface only"decode user identity response: ...", hiding the actual HTTP status — undermining the diagnostic value of this exact code path.🐛 Proposed fix
resp, err := httpClient.Do(req) if err != nil { return nil, err } defer resp.Body.Close() + body, _ := io.ReadAll(io.LimitReader(resp.Body, 1<<20)) var envelope struct { Code int `json:"code"` Msg string `json:"msg"` Data struct { OpenID string `json:"open_id"` Name string `json:"name"` } `json:"data"` } - if err := json.NewDecoder(io.LimitReader(resp.Body, 1<<20)).Decode(&envelope); err != nil { - return nil, fmt.Errorf("decode user identity response: %w", err) - } - if resp.StatusCode < 200 || resp.StatusCode >= 300 || envelope.Code != 0 { - return nil, fmt.Errorf("user identity verification failed: HTTP %d, code %d: %s", resp.StatusCode, envelope.Code, envelope.Msg) - } + parseErr := json.Unmarshal(body, &envelope) + if resp.StatusCode < 200 || resp.StatusCode >= 300 { + if parseErr == nil && envelope.Code != 0 { + return nil, fmt.Errorf("user identity verification failed: HTTP %d, code %d: %s", resp.StatusCode, envelope.Code, envelope.Msg) + } + return nil, fmt.Errorf("user identity verification failed: HTTP %d", resp.StatusCode) + } + if parseErr != nil { + return nil, fmt.Errorf("decode user identity response: %w", parseErr) + } + if envelope.Code != 0 { + return nil, fmt.Errorf("user identity verification failed: code %d: %s", envelope.Code, envelope.Msg) + }🤖 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/identitydiag/diagnostics.go` around lines 438 - 476, Update fetchExternalUserInfo so HTTP status validation is performed even when decoding the response body fails. Preserve the decoded API error details when available, but ensure non-2xx responses with invalid or empty JSON still return an error containing the HTTP status; keep successful responses requiring valid JSON and a non-empty OpenID.cmd/update/edition_extended_test.go (1)
40-43: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winIsolate config state with
LARKSUITE_CLI_CONFIG_DIR.
TestExtendedUpdateUsesExtendedReleaseInstallerruns without--check, sorunEditionUpdatecallsresolveSkillsBrand(opts.Factory, io.ErrOut), which reads on-disk configuration. Without pinning the config dir to a temp path, the test can read/pollute the real config and become environment-dependent.♻️ Isolate config directory
func TestExtendedUpdateUsesExtendedReleaseInstaller(t *testing.T) { + t.Setenv("LARKSUITE_CLI_CONFIG_DIR", t.TempDir()) oldFetch, oldInstall := fetchExtendedLatest, installExtendedAs per coding guidelines: "set
LARKSUITE_CLI_CONFIG_DIRtot.TempDir()witht.Setenvto isolate configuration state."🤖 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 `@cmd/update/edition_extended_test.go` around lines 40 - 43, Update TestExtendedUpdateUsesExtendedReleaseInstaller to call t.Setenv for LARKSUITE_CLI_CONFIG_DIR with t.TempDir() before invoking the command, isolating resolveSkillsBrand configuration access from the real environment.Source: Coding guidelines
🤖 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/show.go`:
- Around line 72-82: Replace the loose result map in cmd/config/show.go lines
72-82 with a typed internal-config response struct. Define a distinct typed
external-provider response struct for cmd/config/show.go lines 107-125, using
omitempty on optional fields. Update cmd/config/config_test.go lines 525-545 to
unmarshal into the external response type or an equivalent typed assertion
struct, using pointer fields for values that may be omitted.
- Around line 92-101: Update the credential resolution flow around
ActiveExtensionProviderName and ResolveAccount to return typed errs failures or
preserve existing typed errors via .WithCause(err), rather than propagating raw
provider or LoadMultiAppConfig errors. Ensure both error paths retain their
original causes so command root handling does not classify them as
internal/unknown.
In `@cmd/doctor/doctor_test.go`:
- Around line 280-283: Update the config_file assertion in the doctor test to
validate configCheck.Message, or otherwise directly verify the skip reason,
rather than checking configCheck.Hint. Ensure the test requires the skip message
to mention local config usage while preserving the existing skip-status
assertion.
In `@cmd/version/version_test.go`:
- Line 18: Update the test setup around the factory creation to use
cmdutil.TestFactory(t, config) instead of cmdutil.NewDefault. Before creating
the factory, set LARKSUITE_CLI_CONFIG_DIR with t.Setenv to t.TempDir(),
preserving the test’s isolated configuration behavior.
- Around line 24-34: Update the version output assertion in the test to compare
got.Capabilities with build.Capabilities() in addition to the existing version
and edition checks, ensuring capability changes cannot pass unnoticed.
In `@cmd/version/version.go`:
- Around line 30-34: Replace the map[string]interface{} payload in the version
output path with a typed versionReport struct. Define versionReport with JSON
tags for version, edition, and capabilities, then instantiate it from
build.Version, build.Edition, and build.Capabilities() before passing it to
output.PrintJson.
- Around line 37-38: Update the version output flow around fmt.Fprintf to wrap
any write failure in errs.NewInternalError(...).WithCause(err) before returning,
while preserving successful output behavior. Add an error-path test for the
version command that verifies the typed error’s category, subtype, and original
writer error cause.
In `@internal/errclass/classify.go`:
- Line 163: Add a focused test for the unrecognized-category fallback in the
classification logic, asserting that the returned result has Origin set to
"lark". Keep the existing normal classification tests unchanged and ensure the
test would fail if the fallback assignment in the classifier were removed or
altered.
In `@internal/event/consume/consume_test.go`:
- Line 90: Update the handshake test around doHello to decode the returned Hello
message and directly assert that Hello.Identity equals "user", while retaining
the existing SubscriptionID assertion. Ensure the test would fail if
hello.Identity is not populated on the wire.
In `@internal/externalcredential/trust_unix_extended_test.go`:
- Line 24: Replace the fixture writes in the test setup around both
`vfs.WriteFile` calls with `os.WriteFile`, preserving the existing paths, data,
and permissions. Remove the `vfs` import if it is no longer referenced, and
ensure the standard-library `os` import is available.
In `@internal/externalcredential/trust_unix_extended.go`:
- Around line 18-39: Update validateAdminControlledPath to normalize target once
and use that cleaned value for both current initialization and the leaf
comparison. Ensure paths containing trailing components or ./ still execute the
final IsRegular and executable permission checks.
In `@scripts/install-extended.sh`:
- Around line 36-42: Update the latest-version fetch in the release-resolution
block to apply the same connection timeout, total transfer timeout, and maximum
redirect limits used by the checksum/archive downloads. Keep the existing
HTTPS-only options and URL extraction behavior unchanged.
In `@shortcuts/mail/mail_watch.go`:
- Around line 398-415: Update the proxy-mode branch around proxySource.Start to
use the same cancellable watchCtx and signal.Notify handling as the WebSocket
path. Run proxySource.Start asynchronously and route its result through the
existing startErrCh/shutdownBySignal select so Ctrl+C or termination cancels the
context, invokes handleMailWatchSignal, and preserves deferred
unsubscribeWithLog cleanup.
---
Outside diff comments:
In @.goreleaser.yml:
- Around line 14-37: Add exclusions for the unsupported darwin/riscv64 and
windows/riscv64 combinations in both the standard and extended GoReleaser build
configurations. Keep linux/riscv64 and all currently supported OS/architecture
combinations unchanged, using each build’s ignore entries or an equivalent
supported-target list.
---
Nitpick comments:
In `@cmd/doctor/doctor_test.go`:
- Around line 259-264: Replace the manual cmdutil.Factory construction in the
affected doctor test with cmdutil.TestFactory(t, cfg), then override its
Credential field with cred and preserve the existing output stream setup as
needed.
In `@cmd/update/edition_extended_test.go`:
- Around line 40-43: Update TestExtendedUpdateUsesExtendedReleaseInstaller to
call t.Setenv for LARKSUITE_CLI_CONFIG_DIR with t.TempDir() before invoking the
command, isolating resolveSkillsBrand configuration access from the real
environment.
In `@internal/identitydiag/diagnostics.go`:
- Around line 438-476: Update fetchExternalUserInfo so HTTP status validation is
performed even when decoding the response body fails. Preserve the decoded API
error details when available, but ensure non-2xx responses with invalid or empty
JSON still return an error containing the HTTP status; keep successful responses
requiring valid JSON and a non-empty OpenID.
🪄 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 Plus
Run ID: 7527cb25-5814-48b5-aaee-006c45147961
📒 Files selected for processing (108)
.github/workflows/ci.yml.github/workflows/release.yml.goreleaser.ymlMakefilecmd/auth/auth.gocmd/auth/auth_test.gocmd/auth/qrcode.gocmd/auth/status.gocmd/auth/status_test.gocmd/build.gocmd/config/config.gocmd/config/config_test.gocmd/config/show.gocmd/doctor/doctor.gocmd/doctor/doctor_test.gocmd/event/bus.gocmd/event/consume.gocmd/event/consume_external_proxy_test.gocmd/event/format_helpers_test.gocmd/root.gocmd/root_test.gocmd/update/edition_extended.gocmd/update/edition_extended_test.gocmd/update/edition_standard.gocmd/update/update.gocmd/update/update_test.gocmd/version/version.gocmd/version/version_test.godocs/external-credential-platform.mderrs/ERROR_CONTRACT.mderrs/marshal_test.goerrs/problem.goerrs/subtypes.gogo.modinternal/build/edition_extended.gointernal/build/edition_standard.gointernal/client/client.gointernal/cmdutil/factory.gointernal/cmdutil/factory_default.gointernal/cmdutil/factory_external_credential_test.gointernal/core/config.gointernal/core/config_test.gointernal/credential/credential_provider.gointernal/credential/default_provider.gointernal/credential/types.gointernal/errclass/classify.gointernal/errclass/classify_test.gointernal/event/bus/bus.gointernal/event/bus/handle_hello_test.gointernal/event/consume/consume.gointernal/event/consume/consume_test.gointernal/event/consume/handshake.gointernal/event/consume/handshake_test.gointernal/event/consume/startup.gointernal/event/consume/startup_fork_test.gointernal/event/consume/startup_guard_test.gointernal/event/protocol/messages.gointernal/event/source/external_proxy.gointernal/event/source/external_proxy_standard.gointernal/event/source/external_proxy_test.gointernal/event/source/source.gointernal/extendedupdate/update.gointernal/extendedupdate/update_test.gointernal/externalcredential/availability_extended.gointernal/externalcredential/availability_standard.gointernal/externalcredential/config_path_darwin.gointernal/externalcredential/config_path_unix.gointernal/externalcredential/config_path_windows.gointernal/externalcredential/file_url.gointernal/externalcredential/process.gointernal/externalcredential/process_standard.gointernal/externalcredential/process_test.gointernal/externalcredential/profile_selection.gointernal/externalcredential/profile_selection_standard_test.gointernal/externalcredential/system_config.gointernal/externalcredential/system_config_test.gointernal/externalcredential/transport.gointernal/externalcredential/transport_standard.gointernal/externalcredential/transport_test.gointernal/externalcredential/trust_extended.gointernal/externalcredential/trust_unix_extended.gointernal/externalcredential/trust_unix_extended_test.gointernal/externalcredential/trust_windows_extended.gointernal/identitydiag/diagnostics.gointernal/identitydiag/diagnostics_test.gointernal/output/errors.gointernal/output/errors_test.gointernal/output/exitcode.goscripts/install-extended.ps1scripts/install-extended.shshortcuts/apps/apps_file_download.goshortcuts/apps/apps_file_get.goshortcuts/apps/apps_file_list.goshortcuts/apps/apps_file_sign.goshortcuts/apps/apps_file_sign_test.goshortcuts/apps/apps_file_upload.goshortcuts/apps/apps_file_upload_test.goshortcuts/apps/apps_html_publish.goshortcuts/apps/file_common.goshortcuts/apps/file_common_external_credential_test.goshortcuts/event/subscribe.goshortcuts/mail/helpers.goshortcuts/mail/mail_signature.goshortcuts/mail/mail_signature_external_credential_test.goshortcuts/mail/mail_watch.goshortcuts/mail/signature_compose.goshortcuts/minutes/minutes_download.goshortcuts/minutes/minutes_download_test.go
evandance
force-pushed
the
feat/external-credential-platform
branch
2 times, most recently
from
July 23, 2026 11:18
8c0f154 to
da6ad79
Compare
evandance
changed the title
There was a problem hiding this comment.
♻️ Duplicate comments (3)
cmd/version/version.go (2)
29-34: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick winReplace the loose JSON map with a typed report struct.
map[string]interface{}is used for the--jsonversion payload, which the coding guidelines explicitly disallow at boundaries ("Parsemap[string]interface{}into typed structs at boundaries... Do not introduce new loose-map code"). Define a smallversionReport{Version, Edition, Capabilities}struct with JSON tags instead.♻️ Proposed fix
+type versionReport struct { + Version string `json:"version"` + Edition string `json:"edition"` + Capabilities []string `json:"capabilities"` +} + if opts.json { - output.PrintJson(opts.factory.IOStreams.Out, map[string]interface{}{ - "version": build.Version, - "edition": build.Edition, - "capabilities": build.Capabilities(), - }) + output.PrintJson(opts.factory.IOStreams.Out, versionReport{ + Version: build.Version, + Edition: build.Edition, + Capabilities: build.Capabilities(), + }) return nil }🤖 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 `@cmd/version/version.go` around lines 29 - 34, Replace the map[string]interface{} payload in the opts.json branch with a typed versionReport struct containing Version, Edition, and Capabilities fields, each with the appropriate JSON tags. Pass the versionReport instance to output.PrintJson while preserving the existing build.Version, build.Edition, and build.Capabilities() values.Source: Coding guidelines
37-38: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winWrap the write failure in a typed error.
Returning the raw
fmt.Fprintferror bypasses the typederrs.*stderr envelope contract required for command-facing failures. Wrap it:errs.NewInternalError(errs.SubtypeFileIO, ...).WithCause(err)(or the appropriate I/O subtype).🤖 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 `@cmd/version/version.go` around lines 37 - 38, Update the version output flow around fmt.Fprintf to wrap any write failure in the command-facing errs.* typed error envelope, using errs.NewInternalError with the appropriate file-I/O subtype and attaching the original error via WithCause before returning it.Source: Coding guidelines
internal/externalcredential/trust_unix_extended.go (1)
18-54: 🔒 Security & Privacy | 🔴 Critical | ⚡ Quick winNormalize
targetonce and compare against the cleaned value.
currentis cleaned viafilepath.Clean, but the leaf comparison at line 29 (current == target) still uses the raw argument. Non-canonical inputs (trailing slash,./prefix, etc.) never satisfy this comparison, so the leaf'sIsRegular()/executable-bit checks are silently skipped and the function can returnnil(trusted) without ever validating the actual file. This was already flagged on a prior commit with a reproduction script; it's still present.🔒 Proposed fix
func validateAdminControlledPath(target string, executable bool) error { devOverride := build.Version == "DEV" && os.Getenv(devSystemConfigPathEnv) != "" - current := filepath.Clean(target) + target = filepath.Clean(target) + current := target for {🤖 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/externalcredential/trust_unix_extended.go` around lines 18 - 54, Normalize target once at the start of validateAdminControlledPath and use that canonical value for both current initialization and the leaf comparison. Ensure the regular-file and executable checks run for non-canonical inputs such as trailing slashes or ./ prefixes, while preserving the existing ownership and permission validation.
🧹 Nitpick comments (1)
.goreleaser.yml (1)
7-37: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider YAML anchors to cut duplication between build stanzas.
The
standardandextendedbuild entries repeatenv,ldflags,goos, andgoarchverbatim; onlyid/binary/tagsdiffer. An anchor/alias would keep both in sync as flags evolve.🤖 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 @.goreleaser.yml around lines 7 - 37, Deduplicate the shared build configuration between the standard and extended entries using a YAML anchor and alias. Anchor the repeated env, ldflags, goos, and goarch settings, then reuse them in both build stanzas while preserving each entry’s id, binary, and extended tags.
🤖 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 `@cmd/version/version.go`:
- Around line 29-34: Replace the map[string]interface{} payload in the opts.json
branch with a typed versionReport struct containing Version, Edition, and
Capabilities fields, each with the appropriate JSON tags. Pass the versionReport
instance to output.PrintJson while preserving the existing build.Version,
build.Edition, and build.Capabilities() values.
- Around line 37-38: Update the version output flow around fmt.Fprintf to wrap
any write failure in the command-facing errs.* typed error envelope, using
errs.NewInternalError with the appropriate file-I/O subtype and attaching the
original error via WithCause before returning it.
In `@internal/externalcredential/trust_unix_extended.go`:
- Around line 18-54: Normalize target once at the start of
validateAdminControlledPath and use that canonical value for both current
initialization and the leaf comparison. Ensure the regular-file and executable
checks run for non-canonical inputs such as trailing slashes or ./ prefixes,
while preserving the existing ownership and permission validation.
---
Nitpick comments:
In @.goreleaser.yml:
- Around line 7-37: Deduplicate the shared build configuration between the
standard and extended entries using a YAML anchor and alias. Anchor the repeated
env, ldflags, goos, and goarch settings, then reuse them in both build stanzas
while preserving each entry’s id, binary, and extended tags.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 812b869b-8a36-49a9-b2e9-120e7b74068e
📒 Files selected for processing (107)
.github/workflows/ci.yml.github/workflows/release.yml.goreleaser.ymlMakefilecmd/auth/auth.gocmd/auth/auth_test.gocmd/auth/qrcode.gocmd/auth/status.gocmd/auth/status_test.gocmd/build.gocmd/config/config.gocmd/config/config_test.gocmd/config/show.gocmd/doctor/doctor.gocmd/doctor/doctor_test.gocmd/event/bus.gocmd/event/consume.gocmd/event/consume_external_proxy_test.gocmd/event/format_helpers_test.gocmd/root.gocmd/root_test.gocmd/update/edition_extended.gocmd/update/edition_extended_test.gocmd/update/edition_standard.gocmd/update/update.gocmd/update/update_test.gocmd/version/version.gocmd/version/version_test.goerrs/ERROR_CONTRACT.mderrs/marshal_test.goerrs/problem.goerrs/subtypes.gogo.modinternal/build/edition_extended.gointernal/build/edition_standard.gointernal/client/client.gointernal/cmdutil/factory.gointernal/cmdutil/factory_default.gointernal/cmdutil/factory_external_credential_test.gointernal/core/config.gointernal/core/config_test.gointernal/credential/credential_provider.gointernal/credential/default_provider.gointernal/credential/types.gointernal/errclass/classify.gointernal/errclass/classify_test.gointernal/event/bus/bus.gointernal/event/bus/handle_hello_test.gointernal/event/consume/consume.gointernal/event/consume/consume_test.gointernal/event/consume/handshake.gointernal/event/consume/handshake_test.gointernal/event/consume/startup.gointernal/event/consume/startup_fork_test.gointernal/event/consume/startup_guard_test.gointernal/event/protocol/messages.gointernal/event/source/external_proxy.gointernal/event/source/external_proxy_standard.gointernal/event/source/external_proxy_test.gointernal/event/source/source.gointernal/extendedupdate/update.gointernal/extendedupdate/update_test.gointernal/externalcredential/availability_extended.gointernal/externalcredential/availability_standard.gointernal/externalcredential/config_path_darwin.gointernal/externalcredential/config_path_unix.gointernal/externalcredential/config_path_windows.gointernal/externalcredential/file_url.gointernal/externalcredential/process.gointernal/externalcredential/process_standard.gointernal/externalcredential/process_test.gointernal/externalcredential/profile_selection.gointernal/externalcredential/profile_selection_standard_test.gointernal/externalcredential/system_config.gointernal/externalcredential/system_config_test.gointernal/externalcredential/transport.gointernal/externalcredential/transport_standard.gointernal/externalcredential/transport_test.gointernal/externalcredential/trust_extended.gointernal/externalcredential/trust_unix_extended.gointernal/externalcredential/trust_unix_extended_test.gointernal/externalcredential/trust_windows_extended.gointernal/identitydiag/diagnostics.gointernal/identitydiag/diagnostics_test.gointernal/output/errors.gointernal/output/errors_test.gointernal/output/exitcode.goscripts/install-extended.ps1scripts/install-extended.shshortcuts/apps/apps_file_download.goshortcuts/apps/apps_file_get.goshortcuts/apps/apps_file_list.goshortcuts/apps/apps_file_sign.goshortcuts/apps/apps_file_sign_test.goshortcuts/apps/apps_file_upload.goshortcuts/apps/apps_file_upload_test.goshortcuts/apps/apps_html_publish.goshortcuts/apps/file_common.goshortcuts/apps/file_common_external_credential_test.goshortcuts/event/subscribe.goshortcuts/mail/helpers.goshortcuts/mail/mail_signature.goshortcuts/mail/mail_signature_external_credential_test.goshortcuts/mail/mail_watch.goshortcuts/mail/signature_compose.goshortcuts/minutes/minutes_download.goshortcuts/minutes/minutes_download_test.go
🚧 Files skipped from review as they are similar to previous changes (92)
- internal/build/edition_standard.go
- internal/build/edition_extended.go
- internal/event/protocol/messages.go
- cmd/version/version_test.go
- internal/externalcredential/process_standard.go
- shortcuts/apps/file_common_external_credential_test.go
- shortcuts/mail/mail_signature_external_credential_test.go
- internal/externalcredential/config_path_windows.go
- internal/event/consume/handshake_test.go
- internal/externalcredential/trust_unix_extended_test.go
- internal/externalcredential/config_path_darwin.go
- internal/externalcredential/availability_extended.go
- internal/externalcredential/profile_selection_standard_test.go
- internal/event/consume/handshake.go
- internal/externalcredential/config_path_unix.go
- internal/errclass/classify.go
- internal/event/consume/startup_guard_test.go
- scripts/install-extended.ps1
- cmd/auth/auth.go
- cmd/event/format_helpers_test.go
- cmd/update/edition_standard.go
- internal/externalcredential/availability_standard.go
- cmd/event/consume_external_proxy_test.go
- internal/externalcredential/system_config.go
- shortcuts/apps/apps_file_get.go
- shortcuts/apps/apps_file_sign_test.go
- Makefile
- internal/event/consume/consume.go
- cmd/auth/qrcode.go
- shortcuts/apps/apps_file_list.go
- go.mod
- internal/output/errors.go
- cmd/event/consume.go
- errs/marshal_test.go
- cmd/config/config.go
- shortcuts/minutes/minutes_download_test.go
- internal/externalcredential/transport_standard.go
- cmd/update/update_test.go
- internal/event/source/source.go
- shortcuts/apps/apps_file_download.go
- shortcuts/apps/file_common.go
- internal/externalcredential/trust_extended.go
- cmd/update/edition_extended_test.go
- .github/workflows/release.yml
- internal/externalcredential/system_config_test.go
- internal/event/source/external_proxy_standard.go
- cmd/event/bus.go
- cmd/update/update.go
- internal/output/errors_test.go
- cmd/auth/status.go
- errs/problem.go
- internal/externalcredential/profile_selection.go
- cmd/config/show.go
- cmd/root_test.go
- shortcuts/mail/mail_signature.go
- shortcuts/apps/apps_html_publish.go
- shortcuts/apps/apps_file_upload.go
- internal/client/client.go
- internal/credential/default_provider.go
- shortcuts/apps/apps_file_upload_test.go
- shortcuts/apps/apps_file_sign.go
- internal/externalcredential/file_url.go
- internal/event/bus/handle_hello_test.go
- internal/credential/credential_provider.go
- internal/credential/types.go
- cmd/build.go
- internal/extendedupdate/update_test.go
- internal/cmdutil/factory.go
- cmd/doctor/doctor.go
- cmd/config/config_test.go
- cmd/update/edition_extended.go
- shortcuts/mail/signature_compose.go
- internal/event/bus/bus.go
- cmd/root.go
- internal/errclass/classify_test.go
- scripts/install-extended.sh
- internal/identitydiag/diagnostics.go
- internal/event/consume/startup.go
- internal/event/consume/startup_fork_test.go
- internal/externalcredential/transport_test.go
- internal/event/source/external_proxy.go
- shortcuts/minutes/minutes_download.go
- internal/core/config_test.go
- errs/ERROR_CONTRACT.md
- shortcuts/mail/mail_watch.go
- internal/cmdutil/factory_external_credential_test.go
- shortcuts/event/subscribe.go
- internal/externalcredential/transport.go
- internal/core/config.go
- internal/extendedupdate/update.go
- internal/externalcredential/process_test.go
- .github/workflows/ci.yml
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/identitydiag/diagnostics_test.go`:
- Line 479: Isolate configuration in both tests by calling
t.Setenv("LARKSUITE_CLI_CONFIG_DIR", t.TempDir()) immediately before each
cmdutil.TestFactory call: internal/identitydiag/diagnostics_test.go lines
479-479 and 507-507. Apply the same setup at both sites.
🪄 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 Plus
Run ID: 51142e8e-aaed-490f-9170-5d767f245fa1
📒 Files selected for processing (107)
.github/workflows/ci.yml.github/workflows/release.yml.goreleaser.ymlMakefilecmd/auth/auth.gocmd/auth/auth_test.gocmd/auth/qrcode.gocmd/auth/status.gocmd/auth/status_test.gocmd/build.gocmd/config/config.gocmd/config/config_test.gocmd/config/show.gocmd/doctor/doctor.gocmd/doctor/doctor_test.gocmd/event/bus.gocmd/event/consume.gocmd/event/consume_external_proxy_test.gocmd/event/format_helpers_test.gocmd/root.gocmd/root_test.gocmd/update/edition_extended.gocmd/update/edition_extended_test.gocmd/update/edition_standard.gocmd/update/update.gocmd/update/update_test.gocmd/version/version.gocmd/version/version_test.goerrs/ERROR_CONTRACT.mderrs/marshal_test.goerrs/problem.goerrs/subtypes.gogo.modinternal/build/edition_extended.gointernal/build/edition_standard.gointernal/client/client.gointernal/cmdutil/factory.gointernal/cmdutil/factory_default.gointernal/cmdutil/factory_external_credential_test.gointernal/core/config.gointernal/core/config_test.gointernal/credential/credential_provider.gointernal/credential/default_provider.gointernal/credential/types.gointernal/errclass/classify.gointernal/errclass/classify_test.gointernal/event/bus/bus.gointernal/event/bus/handle_hello_test.gointernal/event/consume/consume.gointernal/event/consume/consume_test.gointernal/event/consume/handshake.gointernal/event/consume/handshake_test.gointernal/event/consume/startup.gointernal/event/consume/startup_fork_test.gointernal/event/consume/startup_guard_test.gointernal/event/protocol/messages.gointernal/event/source/external_proxy.gointernal/event/source/external_proxy_standard.gointernal/event/source/external_proxy_test.gointernal/event/source/source.gointernal/extendedupdate/update.gointernal/extendedupdate/update_test.gointernal/externalcredential/availability_extended.gointernal/externalcredential/availability_standard.gointernal/externalcredential/config_path_darwin.gointernal/externalcredential/config_path_unix.gointernal/externalcredential/config_path_windows.gointernal/externalcredential/file_url.gointernal/externalcredential/process.gointernal/externalcredential/process_standard.gointernal/externalcredential/process_test.gointernal/externalcredential/profile_selection.gointernal/externalcredential/profile_selection_standard_test.gointernal/externalcredential/system_config.gointernal/externalcredential/system_config_test.gointernal/externalcredential/transport.gointernal/externalcredential/transport_standard.gointernal/externalcredential/transport_test.gointernal/externalcredential/trust_extended.gointernal/externalcredential/trust_unix_extended.gointernal/externalcredential/trust_unix_extended_test.gointernal/externalcredential/trust_windows_extended.gointernal/identitydiag/diagnostics.gointernal/identitydiag/diagnostics_test.gointernal/output/errors.gointernal/output/errors_test.gointernal/output/exitcode.goscripts/install-extended.ps1scripts/install-extended.shshortcuts/apps/apps_file_download.goshortcuts/apps/apps_file_get.goshortcuts/apps/apps_file_list.goshortcuts/apps/apps_file_sign.goshortcuts/apps/apps_file_sign_test.goshortcuts/apps/apps_file_upload.goshortcuts/apps/apps_file_upload_test.goshortcuts/apps/apps_html_publish.goshortcuts/apps/file_common.goshortcuts/apps/file_common_external_credential_test.goshortcuts/event/subscribe.goshortcuts/mail/helpers.goshortcuts/mail/mail_signature.goshortcuts/mail/mail_signature_external_credential_test.goshortcuts/mail/mail_watch.goshortcuts/mail/signature_compose.goshortcuts/minutes/minutes_download.goshortcuts/minutes/minutes_download_test.go
🚧 Files skipped from review as they are similar to previous changes (93)
- internal/build/edition_extended.go
- internal/externalcredential/availability_standard.go
- internal/externalcredential/availability_extended.go
- cmd/auth/qrcode.go
- internal/externalcredential/system_config_test.go
- internal/externalcredential/config_path_unix.go
- internal/event/consume/handshake_test.go
- internal/externalcredential/config_path_darwin.go
- internal/externalcredential/config_path_windows.go
- internal/event/consume/consume_test.go
- internal/event/source/external_proxy_standard.go
- internal/output/exitcode.go
- cmd/event/format_helpers_test.go
- shortcuts/apps/apps_file_list.go
- internal/externalcredential/profile_selection_standard_test.go
- cmd/version/version_test.go
- internal/build/edition_standard.go
- internal/errclass/classify.go
- cmd/update/edition_standard.go
- cmd/update/update_test.go
- internal/event/consume/startup_guard_test.go
- shortcuts/apps/apps_file_sign_test.go
- shortcuts/apps/file_common_external_credential_test.go
- internal/externalcredential/process_standard.go
- internal/event/consume/consume.go
- scripts/install-extended.ps1
- errs/marshal_test.go
- errs/subtypes.go
- cmd/auth/auth.go
- shortcuts/mail/mail_signature_external_credential_test.go
- internal/extendedupdate/update_test.go
- internal/externalcredential/trust_unix_extended_test.go
- internal/externalcredential/trust_extended.go
- internal/event/source/source.go
- shortcuts/apps/apps_file_download.go
- internal/event/bus/bus.go
- Makefile
- cmd/update/update.go
- cmd/root_test.go
- internal/output/errors.go
- shortcuts/apps/apps_file_get.go
- cmd/version/version.go
- cmd/update/edition_extended.go
- cmd/update/edition_extended_test.go
- internal/externalcredential/system_config.go
- cmd/event/consume_external_proxy_test.go
- go.mod
- internal/output/errors_test.go
- internal/credential/types.go
- shortcuts/mail/mail_signature.go
- shortcuts/apps/file_common.go
- internal/event/protocol/messages.go
- .github/workflows/ci.yml
- shortcuts/apps/apps_file_sign.go
- internal/cmdutil/factory.go
- internal/externalcredential/profile_selection.go
- cmd/event/consume.go
- errs/problem.go
- cmd/build.go
- internal/externalcredential/transport_test.go
- cmd/root.go
- .github/workflows/release.yml
- cmd/config/show.go
- internal/event/consume/startup.go
- shortcuts/apps/apps_html_publish.go
- internal/externalcredential/transport_standard.go
- .goreleaser.yml
- cmd/config/config_test.go
- internal/externalcredential/file_url.go
- internal/event/consume/startup_fork_test.go
- shortcuts/apps/apps_file_upload.go
- cmd/auth/status.go
- shortcuts/apps/apps_file_upload_test.go
- internal/credential/default_provider.go
- cmd/event/bus.go
- shortcuts/minutes/minutes_download_test.go
- shortcuts/event/subscribe.go
- internal/errclass/classify_test.go
- internal/event/bus/handle_hello_test.go
- scripts/install-extended.sh
- internal/externalcredential/trust_unix_extended.go
- internal/event/source/external_proxy.go
- errs/ERROR_CONTRACT.md
- internal/credential/credential_provider.go
- shortcuts/mail/signature_compose.go
- shortcuts/minutes/minutes_download.go
- shortcuts/mail/helpers.go
- internal/extendedupdate/update.go
- internal/identitydiag/diagnostics.go
- shortcuts/mail/mail_watch.go
- internal/externalcredential/process_test.go
- internal/cmdutil/factory_default.go
- internal/externalcredential/transport.go
evandance
force-pushed
the
feat/external-credential-platform
branch
from
July 23, 2026 11:54
da6ad79 to
e9d88d3
Compare
Collaborator
Author
|
Follow-up review disposition:
All inline threads have been replied to and resolved; CI is rerunning on the amended single commit. |
evandance
force-pushed
the
feat/external-credential-platform
branch
from
July 23, 2026 12:03
e9d88d3 to
43c92e4
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
cmd/doctor/doctor_test.go (1)
256-260: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse
cmdutil.TestFactoryinstead of a hand-builtFactory.This new test hand-constructs a
cmdutil.Factorywhile the sibling test at Line 217 was just migrated tocmdutil.TestFactory. Use the helper here too for consistency and to inherit the standard wiring.As per path instructions: "Use
cmdutil.TestFactory(t, config)for test factories".♻️ Proposed change
- out := &bytes.Buffer{} - f := &cmdutil.Factory{ - Config: func() (*core.CliConfig, error) { return cfg, nil }, - Credential: cred, - IOStreams: &cmdutil.IOStreams{Out: out, ErrOut: &bytes.Buffer{}}, - } + f, out, _, _ := cmdutil.TestFactory(t, cfg) + f.Credential = cred🤖 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 `@cmd/doctor/doctor_test.go` around lines 256 - 260, Replace the hand-built cmdutil.Factory in the affected test with cmdutil.TestFactory(t, config), passing the test configuration and preserving the test’s required credential and I/O setup through the helper’s standard wiring.Source: Path instructions
🤖 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 @.github/workflows/ci.yml:
- Around line 274-287: Update the “Coverage summary” step to check for and read
coverage-standard.txt instead of coverage.txt, including its go tool cover -func
invocation. Leave the extended profile handling and coverage threshold step
unchanged.
---
Nitpick comments:
In `@cmd/doctor/doctor_test.go`:
- Around line 256-260: Replace the hand-built cmdutil.Factory in the affected
test with cmdutil.TestFactory(t, config), passing the test configuration and
preserving the test’s required credential and I/O setup through the helper’s
standard wiring.
🪄 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 Plus
Run ID: 9a8379eb-d76a-4491-b93f-b9be7cbd785b
📒 Files selected for processing (110)
.github/workflows/ci.yml.github/workflows/release.yml.goreleaser.ymlMakefilecmd/auth/auth.gocmd/auth/auth_test.gocmd/auth/qrcode.gocmd/auth/status.gocmd/auth/status_test.gocmd/build.gocmd/config/config.gocmd/config/config_test.gocmd/config/show.gocmd/doctor/doctor.gocmd/doctor/doctor_test.gocmd/event/bus.gocmd/event/consume.gocmd/event/consume_external_proxy_test.gocmd/event/format_helpers_test.gocmd/root.gocmd/root_test.gocmd/update/edition_extended.gocmd/update/edition_extended_test.gocmd/update/edition_standard.gocmd/update/update.gocmd/update/update_test.gocmd/version/version.gocmd/version/version_test.goerrs/ERROR_CONTRACT.mderrs/marshal_test.goerrs/problem.goerrs/subtypes.gogo.modinternal/build/edition_extended.gointernal/build/edition_standard.gointernal/client/client.gointernal/cmdutil/factory.gointernal/cmdutil/factory_default.gointernal/cmdutil/factory_external_credential_test.gointernal/core/config.gointernal/core/config_test.gointernal/credential/credential_provider.gointernal/credential/default_provider.gointernal/credential/types.gointernal/errclass/classify.gointernal/errclass/classify_internal_test.gointernal/errclass/classify_test.gointernal/event/bus/bus.gointernal/event/bus/handle_hello_test.gointernal/event/consume/consume.gointernal/event/consume/consume_test.gointernal/event/consume/handshake.gointernal/event/consume/handshake_test.gointernal/event/consume/startup.gointernal/event/consume/startup_fork_test.gointernal/event/consume/startup_guard_test.gointernal/event/protocol/messages.gointernal/event/source/external_proxy.gointernal/event/source/external_proxy_standard.gointernal/event/source/external_proxy_test.gointernal/event/source/source.gointernal/extendedupdate/update.gointernal/extendedupdate/update_test.gointernal/externalcredential/availability_extended.gointernal/externalcredential/availability_standard.gointernal/externalcredential/config_path_darwin.gointernal/externalcredential/config_path_unix.gointernal/externalcredential/config_path_windows.gointernal/externalcredential/file_url.gointernal/externalcredential/process.gointernal/externalcredential/process_standard.gointernal/externalcredential/process_test.gointernal/externalcredential/profile_selection.gointernal/externalcredential/profile_selection_standard_test.gointernal/externalcredential/system_config.gointernal/externalcredential/system_config_test.gointernal/externalcredential/transport.gointernal/externalcredential/transport_standard.gointernal/externalcredential/transport_test.gointernal/externalcredential/trust_extended.gointernal/externalcredential/trust_unix_extended.gointernal/externalcredential/trust_unix_extended_test.gointernal/externalcredential/trust_windows_extended.gointernal/identitydiag/diagnostics.gointernal/identitydiag/diagnostics_test.gointernal/output/errors.gointernal/output/errors_test.gointernal/output/exitcode.goscripts/install-extended.ps1scripts/install-extended.shshortcuts/apps/apps_file_download.goshortcuts/apps/apps_file_get.goshortcuts/apps/apps_file_list.goshortcuts/apps/apps_file_sign.goshortcuts/apps/apps_file_sign_test.goshortcuts/apps/apps_file_upload.goshortcuts/apps/apps_file_upload_test.goshortcuts/apps/apps_html_publish.goshortcuts/apps/file_common.goshortcuts/apps/file_common_external_credential_test.goshortcuts/event/subscribe.goshortcuts/mail/helpers.goshortcuts/mail/mail_signature.goshortcuts/mail/mail_signature_external_credential_test.goshortcuts/mail/mail_watch.goshortcuts/mail/mail_watch_test.goshortcuts/mail/signature_compose.goshortcuts/minutes/minutes_download.goshortcuts/minutes/minutes_download_test.gotests/plugin_e2e/degrade_subsystem_test.go
🚧 Files skipped from review as they are similar to previous changes (93)
- internal/externalcredential/config_path_windows.go
- internal/externalcredential/system_config_test.go
- internal/externalcredential/availability_extended.go
- internal/externalcredential/profile_selection_standard_test.go
- cmd/event/consume_external_proxy_test.go
- internal/externalcredential/process_standard.go
- cmd/event/bus.go
- shortcuts/apps/file_common_external_credential_test.go
- internal/externalcredential/config_path_unix.go
- internal/errclass/classify.go
- internal/build/edition_standard.go
- internal/build/edition_extended.go
- cmd/update/update_test.go
- errs/subtypes.go
- errs/marshal_test.go
- internal/externalcredential/system_config.go
- cmd/build.go
- internal/extendedupdate/update_test.go
- internal/output/errors.go
- cmd/version/version.go
- internal/event/consume/handshake.go
- internal/externalcredential/availability_standard.go
- cmd/event/format_helpers_test.go
- internal/event/consume/startup_guard_test.go
- cmd/config/config.go
- internal/externalcredential/config_path_darwin.go
- cmd/auth/qrcode.go
- internal/event/consume/consume_test.go
- internal/event/source/source.go
- internal/event/source/external_proxy_standard.go
- errs/problem.go
- shortcuts/apps/apps_file_sign_test.go
- shortcuts/apps/apps_html_publish.go
- internal/externalcredential/trust_extended.go
- shortcuts/apps/apps_file_sign.go
- internal/externalcredential/trust_unix_extended.go
- cmd/root.go
- internal/output/exitcode.go
- internal/externalcredential/trust_unix_extended_test.go
- internal/event/consume/handshake_test.go
- cmd/update/edition_extended_test.go
- cmd/update/update.go
- shortcuts/mail/mail_signature.go
- shortcuts/apps/apps_file_download.go
- shortcuts/mail/mail_signature_external_credential_test.go
- internal/event/consume/startup_fork_test.go
- cmd/doctor/doctor.go
- shortcuts/apps/apps_file_get.go
- shortcuts/apps/apps_file_list.go
- go.mod
- cmd/update/edition_standard.go
- internal/externalcredential/file_url.go
- internal/event/consume/startup.go
- scripts/install-extended.ps1
- internal/output/errors_test.go
- internal/event/bus/bus.go
- errs/ERROR_CONTRACT.md
- .github/workflows/release.yml
- internal/externalcredential/transport_test.go
- shortcuts/minutes/minutes_download_test.go
- internal/event/consume/consume.go
- internal/event/bus/handle_hello_test.go
- .goreleaser.yml
- internal/errclass/classify_test.go
- cmd/config/show.go
- cmd/event/consume.go
- Makefile
- scripts/install-extended.sh
- internal/client/client.go
- internal/externalcredential/profile_selection.go
- internal/event/protocol/messages.go
- cmd/auth/auth_test.go
- shortcuts/mail/signature_compose.go
- cmd/auth/status.go
- shortcuts/apps/file_common.go
- shortcuts/apps/apps_file_upload_test.go
- internal/credential/types.go
- shortcuts/mail/helpers.go
- shortcuts/apps/apps_file_upload.go
- internal/extendedupdate/update.go
- internal/cmdutil/factory.go
- cmd/config/config_test.go
- internal/externalcredential/transport.go
- internal/cmdutil/factory_external_credential_test.go
- cmd/update/edition_extended.go
- internal/identitydiag/diagnostics.go
- internal/core/config_test.go
- shortcuts/event/subscribe.go
- internal/credential/credential_provider.go
- shortcuts/minutes/minutes_download.go
- internal/event/source/external_proxy.go
- internal/cmdutil/factory_default.go
- internal/externalcredential/process_test.go
evandance
force-pushed
the
feat/external-credential-platform
branch
from
July 23, 2026 12:16
43c92e4 to
20ec014
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
cmd/doctor/doctor_test.go (1)
245-260: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUse
cmdutil.TestFactoryfor consistency with the sibling test.This test hand-builds
&cmdutil.Factory{...}(onlyConfig/Credential/IOStreamsset, leavingHttpClient/LarkClient/Keychainnil), right after the adjacent test at line 217 was refactored to usecmdutil.TestFactory(t, cfg). Using the same helper here would keep the factory fully populated and avoid the now-unusedt.Setenv("LARKSUITE_CLI_CONFIG_DIR", ...)at line 246 (unused sinceConfigis a closure that bypasses disk config entirely).♻️ Suggested consistency fix
- out := &bytes.Buffer{} - f := &cmdutil.Factory{ - Config: func() (*core.CliConfig, error) { return cfg, nil }, - Credential: cred, - IOStreams: &cmdutil.IOStreams{Out: out, ErrOut: &bytes.Buffer{}}, - } + f, out, _, _ := cmdutil.TestFactory(t, cfg) + f.Credential = cred🤖 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 `@cmd/doctor/doctor_test.go` around lines 245 - 260, Update TestDoctor_EnvironmentProviderDoesNotRequireLocalConfig to construct its factory with cmdutil.TestFactory(t, cfg), then assign the test credential provider and output buffer through that factory as needed. Remove the now-unnecessary LARKSUITE_CLI_CONFIG_DIR environment setup and avoid hand-building a partial cmdutil.Factory.internal/externalcredential/trust_windows_extended.go (1)
46-120: 🔒 Security & Privacy | 🔵 Trivial | 🏗️ Heavy liftAdd Windows-specific ACL trust coverage.
internal/externalcredential/trust_windows_extended.goimplements owner/DACL validation, but there is nointernal/externalcredential/trust_windows_extended_test.gowhile the Unix variant has dedicated tests. Add Windows-gated coverage forvalidateWindowsACL,sidIn, andvalidateAdminControlledPathto protect this security boundary.🤖 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/externalcredential/trust_windows_extended.go` around lines 46 - 120, Add a Windows-only test file covering validateWindowsACL, sidIn, and validateAdminControlledPath. Create representative trusted and untrusted ACL/owner scenarios, including file and directory handling, and assert both acceptance and rejection paths so the Windows security boundary is exercised without affecting non-Windows builds.internal/externalcredential/process.go (1)
286-313: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winStderr from the credential process is discarded, hiding the only diagnostic detail on failure.
cmd.Stderr = io.Discardmeans that when the process exits non-zero and doesn't produce valid JSON, the resulting error is just "external credential process exited unsuccessfully" with the bare*exec.ExitErroras cause — any diagnostic text the administrator's tool printed (missing binary output, permission errors, auth failures) is lost. Since this is an admin-configured integration point, that stderr content is valuable for troubleshooting.♻️ Capture a bounded stderr excerpt for diagnostics
var stdout limitedBuffer cmd.Stdout = &stdout - cmd.Stderr = io.Discard + var stderr limitedBuffer + cmd.Stderr = &stderr err = cmd.Run()Then include
stderr.String()(truncated) in the fallback error's hint/cause whendecodeErr != nil.🤖 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/externalcredential/process.go` around lines 286 - 313, Update the credential process execution flow around cmd.Stderr and the fallback error in processCredential (or the nearest enclosing function) to capture stderr in a bounded buffer instead of discarding it, then include the truncated stderr excerpt in the diagnostic hint or cause when decodeErr is non-nil and the process exits unsuccessfully. Preserve existing timeout, context, response-size, and JSON error classification behavior.
🤖 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/cmdutil/factory.go`:
- Around line 244-250: Guard cmd against nil before invoking GetRisk in
RequireBuiltinCredentialProviderForCommand. Preserve the RiskRead early return
for non-nil commands, while allowing nil commands to continue to the existing
command-name handling without dereferencing cmd.
---
Nitpick comments:
In `@cmd/doctor/doctor_test.go`:
- Around line 245-260: Update
TestDoctor_EnvironmentProviderDoesNotRequireLocalConfig to construct its factory
with cmdutil.TestFactory(t, cfg), then assign the test credential provider and
output buffer through that factory as needed. Remove the now-unnecessary
LARKSUITE_CLI_CONFIG_DIR environment setup and avoid hand-building a partial
cmdutil.Factory.
In `@internal/externalcredential/process.go`:
- Around line 286-313: Update the credential process execution flow around
cmd.Stderr and the fallback error in processCredential (or the nearest enclosing
function) to capture stderr in a bounded buffer instead of discarding it, then
include the truncated stderr excerpt in the diagnostic hint or cause when
decodeErr is non-nil and the process exits unsuccessfully. Preserve existing
timeout, context, response-size, and JSON error classification behavior.
In `@internal/externalcredential/trust_windows_extended.go`:
- Around line 46-120: Add a Windows-only test file covering validateWindowsACL,
sidIn, and validateAdminControlledPath. Create representative trusted and
untrusted ACL/owner scenarios, including file and directory handling, and assert
both acceptance and rejection paths so the Windows security boundary is
exercised without affecting non-Windows builds.
🪄 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 Plus
Run ID: 49331db9-942e-405d-aa31-c9028fa335a2
📒 Files selected for processing (110)
.github/workflows/ci.yml.github/workflows/release.yml.goreleaser.ymlMakefilecmd/auth/auth.gocmd/auth/auth_test.gocmd/auth/qrcode.gocmd/auth/status.gocmd/auth/status_test.gocmd/build.gocmd/config/config.gocmd/config/config_test.gocmd/config/show.gocmd/doctor/doctor.gocmd/doctor/doctor_test.gocmd/event/bus.gocmd/event/consume.gocmd/event/consume_external_proxy_test.gocmd/event/format_helpers_test.gocmd/root.gocmd/root_test.gocmd/update/edition_extended.gocmd/update/edition_extended_test.gocmd/update/edition_standard.gocmd/update/update.gocmd/update/update_test.gocmd/version/version.gocmd/version/version_test.goerrs/ERROR_CONTRACT.mderrs/marshal_test.goerrs/problem.goerrs/subtypes.gogo.modinternal/build/edition_extended.gointernal/build/edition_standard.gointernal/client/client.gointernal/cmdutil/factory.gointernal/cmdutil/factory_default.gointernal/cmdutil/factory_external_credential_test.gointernal/core/config.gointernal/core/config_test.gointernal/credential/credential_provider.gointernal/credential/default_provider.gointernal/credential/types.gointernal/errclass/classify.gointernal/errclass/classify_internal_test.gointernal/errclass/classify_test.gointernal/event/bus/bus.gointernal/event/bus/handle_hello_test.gointernal/event/consume/consume.gointernal/event/consume/consume_test.gointernal/event/consume/handshake.gointernal/event/consume/handshake_test.gointernal/event/consume/startup.gointernal/event/consume/startup_fork_test.gointernal/event/consume/startup_guard_test.gointernal/event/protocol/messages.gointernal/event/source/external_proxy.gointernal/event/source/external_proxy_standard.gointernal/event/source/external_proxy_test.gointernal/event/source/source.gointernal/extendedupdate/update.gointernal/extendedupdate/update_test.gointernal/externalcredential/availability_extended.gointernal/externalcredential/availability_standard.gointernal/externalcredential/config_path_darwin.gointernal/externalcredential/config_path_unix.gointernal/externalcredential/config_path_windows.gointernal/externalcredential/file_url.gointernal/externalcredential/process.gointernal/externalcredential/process_standard.gointernal/externalcredential/process_test.gointernal/externalcredential/profile_selection.gointernal/externalcredential/profile_selection_standard_test.gointernal/externalcredential/system_config.gointernal/externalcredential/system_config_test.gointernal/externalcredential/transport.gointernal/externalcredential/transport_standard.gointernal/externalcredential/transport_test.gointernal/externalcredential/trust_extended.gointernal/externalcredential/trust_unix_extended.gointernal/externalcredential/trust_unix_extended_test.gointernal/externalcredential/trust_windows_extended.gointernal/identitydiag/diagnostics.gointernal/identitydiag/diagnostics_test.gointernal/output/errors.gointernal/output/errors_test.gointernal/output/exitcode.goscripts/install-extended.ps1scripts/install-extended.shshortcuts/apps/apps_file_download.goshortcuts/apps/apps_file_get.goshortcuts/apps/apps_file_list.goshortcuts/apps/apps_file_sign.goshortcuts/apps/apps_file_sign_test.goshortcuts/apps/apps_file_upload.goshortcuts/apps/apps_file_upload_test.goshortcuts/apps/apps_html_publish.goshortcuts/apps/file_common.goshortcuts/apps/file_common_external_credential_test.goshortcuts/event/subscribe.goshortcuts/mail/helpers.goshortcuts/mail/mail_signature.goshortcuts/mail/mail_signature_external_credential_test.goshortcuts/mail/mail_watch.goshortcuts/mail/mail_watch_test.goshortcuts/mail/signature_compose.goshortcuts/minutes/minutes_download.goshortcuts/minutes/minutes_download_test.gotests/plugin_e2e/degrade_subsystem_test.go
🚧 Files skipped from review as they are similar to previous changes (92)
- cmd/config/config.go
- cmd/auth/qrcode.go
- cmd/event/format_helpers_test.go
- internal/event/consume/handshake.go
- internal/externalcredential/config_path_darwin.go
- internal/event/consume/handshake_test.go
- internal/event/source/external_proxy_standard.go
- internal/externalcredential/system_config_test.go
- shortcuts/apps/file_common_external_credential_test.go
- cmd/event/consume_external_proxy_test.go
- internal/output/errors.go
- shortcuts/apps/apps_file_sign_test.go
- cmd/update/edition_extended_test.go
- internal/externalcredential/availability_standard.go
- internal/build/edition_extended.go
- cmd/version/version.go
- cmd/event/bus.go
- internal/build/edition_standard.go
- shortcuts/apps/apps_file_get.go
- cmd/build.go
- internal/event/source/source.go
- internal/externalcredential/process_standard.go
- cmd/update/update.go
- cmd/update/update_test.go
- shortcuts/mail/mail_signature_external_credential_test.go
- Makefile
- internal/event/consume/startup_guard_test.go
- errs/subtypes.go
- internal/externalcredential/trust_extended.go
- cmd/auth/auth.go
- cmd/root_test.go
- internal/errclass/classify.go
- internal/output/errors_test.go
- internal/errclass/classify_internal_test.go
- internal/extendedupdate/update_test.go
- internal/externalcredential/profile_selection_standard_test.go
- internal/externalcredential/config_path_windows.go
- internal/event/protocol/messages.go
- cmd/update/edition_standard.go
- errs/marshal_test.go
- internal/event/consume/consume.go
- internal/client/client.go
- scripts/install-extended.sh
- shortcuts/apps/apps_file_list.go
- internal/event/consume/startup.go
- .goreleaser.yml
- internal/event/bus/handle_hello_test.go
- errs/ERROR_CONTRACT.md
- internal/event/consume/consume_test.go
- internal/externalcredential/transport_test.go
- shortcuts/apps/apps_file_download.go
- tests/plugin_e2e/degrade_subsystem_test.go
- internal/event/bus/bus.go
- internal/externalcredential/transport_standard.go
- shortcuts/apps/apps_file_sign.go
- cmd/update/edition_extended.go
- scripts/install-extended.ps1
- cmd/event/consume.go
- internal/externalcredential/trust_unix_extended_test.go
- shortcuts/mail/mail_watch_test.go
- shortcuts/mail/helpers.go
- cmd/version/version_test.go
- shortcuts/apps/apps_file_upload_test.go
- internal/credential/types.go
- internal/externalcredential/file_url.go
- shortcuts/apps/file_common.go
- shortcuts/mail/mail_signature.go
- cmd/root.go
- internal/event/consume/startup_fork_test.go
- cmd/config/show.go
- shortcuts/minutes/minutes_download_test.go
- .github/workflows/release.yml
- internal/cmdutil/factory_external_credential_test.go
- shortcuts/mail/mail_watch.go
- internal/externalcredential/profile_selection.go
- shortcuts/mail/signature_compose.go
- shortcuts/apps/apps_file_upload.go
- shortcuts/event/subscribe.go
- cmd/doctor/doctor.go
- internal/event/source/external_proxy.go
- .github/workflows/ci.yml
- internal/extendedupdate/update.go
- internal/externalcredential/trust_unix_extended.go
- internal/externalcredential/transport.go
- cmd/config/config_test.go
- internal/identitydiag/diagnostics.go
- internal/core/config.go
- internal/externalcredential/process_test.go
- internal/core/config_test.go
- internal/cmdutil/factory_default.go
- shortcuts/minutes/minutes_download.go
- internal/credential/credential_provider.go
evandance
force-pushed
the
feat/external-credential-platform
branch
from
July 23, 2026 12:32
20ec014 to
9c4b842
Compare
evandance
force-pushed
the
feat/external-credential-platform
branch
from
July 24, 2026 08:37
9c4b842 to
a7d2747
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
1 participant
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
Add opt-in external credential integration for Extended builds while keeping the Standard distribution unchanged. Credential ownership and request authorization can remain outside the CLI when required by the runtime environment.
Changes
Test Plan
Related Issues