e2e: harden CLI E2E retry, cleanup, and domain selection#1709
Conversation
|
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:
📝 WalkthroughWalkthroughThe PR adds shared CLI E2E domain resolution for CI and PR labels, updates CLI E2E execution to retry structured errors and classify cleanup warnings, and adjusts docs fetch coverage and tests to use the v1 compatibility flag. ChangesCLI E2E domain routing
CLI retry and cleanup warnings
Docs fetch compatibility
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 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 |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
tests/cli_e2e/core_test.go (1)
383-394: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
useFastDefaultRetrymutates package-level globals — not parallel-safe.Overwriting
defaultRetryInitialDelay/defaultRetryMaxDelayand restoring viat.Cleanupworks only because these tests run serially. If any test in this package is later markedt.Parallel(), this becomes a data race under-race. Consider documenting the serial-only assumption here (or gating retry timing through injectedRetryOptions) to prevent a future footgun.🤖 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 `@tests/cli_e2e/core_test.go` around lines 383 - 394, The helper useFastDefaultRetry mutates package-level retry globals, so it is not safe if tests in this package ever run in parallel. Update the helper to avoid shared-state overrides where possible, ideally by threading retry timing through injected RetryOptions or similar test-scoped configuration; if that refactor is too large, add an explicit comment near useFastDefaultRetry noting the serial-only assumption so future t.Parallel() usage does not introduce a race.
🤖 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.
Nitpick comments:
In `@tests/cli_e2e/core_test.go`:
- Around line 383-394: The helper useFastDefaultRetry mutates package-level
retry globals, so it is not safe if tests in this package ever run in parallel.
Update the helper to avoid shared-state overrides where possible, ideally by
threading retry timing through injected RetryOptions or similar test-scoped
configuration; if that refactor is too large, add an explicit comment near
useFastDefaultRetry noting the serial-only assumption so future t.Parallel()
usage does not introduce a race.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: fca14ddb-be9b-4e39-b7ab-93d8c8e0f309
📒 Files selected for processing (2)
tests/cli_e2e/core.gotests/cli_e2e/core_test.go
d732802 to
7658240
Compare
7658240 to
44d58ae
Compare
e98e947 to
2f3407b
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (2)
scripts/e2e_domains.js (1)
37-44: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low valueRepeated
go listsubprocess spawns without memoization.
domainExistsshells out togo liston every call, andclassifyPath/addDomaincan call it multiple times for the same domain across several changed files in the same PR. Caching results in aMapwould avoid redundant subprocess spawns.⚡ Memoize domainExists
+const domainExistsCache = new Map(); + function domainExists(domain) { + if (domainExistsCache.has(domain)) return domainExistsCache.get(domain); + let exists; try { execFileSync("go", ["list", `./tests/cli_e2e/${domain}`], { stdio: "ignore" }); - return true; + exists = true; } catch { - return false; + exists = false; } + domainExistsCache.set(domain, exists); + return exists; }Also applies to: 68-115
🤖 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 `@scripts/e2e_domains.js` around lines 37 - 44, The domain existence check is spawning repeated go list subprocesses for the same domain; memoize the result in domainExists so classifyPath and addDomain can reuse it across multiple files. Add a Map-based cache keyed by domain inside the e2e_domains.js flow, and have domainExists consult the cache before calling execFileSync, then store both success and failure results for later calls.scripts/domain-map.js (1)
14-17: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winPrefix matching is order-dependent on
domain-map.json.
findPathMappingreturns the first prefix match via.find(), so correctness silently depends ondomain-map.jsonlisting more specific prefixes before more general ones (e.g.shortcuts/doc/beforeshortcuts/). Since that JSON file isn't in this diff, I can't confirm the ordering is safe, and any future entry added out of order will silently mis-route domains.♻️ Make matching order-independent
function findPathMapping(filePath) { const normalized = normalizeRepoPath(filePath); - return (domainMap.pathMappings || []).find((entry) => normalized.startsWith(entry.prefix)); + return (domainMap.pathMappings || []) + .slice() + .sort((a, b) => b.prefix.length - a.prefix.length) + .find((entry) => normalized.startsWith(entry.prefix)); }🤖 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 `@scripts/domain-map.js` around lines 14 - 17, The prefix lookup in findPathMapping is order-dependent because it returns the first match from domainMap.pathMappings, so make the selection order-independent by choosing the most specific matching entry instead of relying on JSON ordering. Update findPathMapping to evaluate all matching prefixes and pick the best match based on prefix length or specificity, using normalizeRepoPath and the existing entry.prefix values, so future additions to domainMap.pathMappings cannot silently mis-route domains.
🤖 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.
Nitpick comments:
In `@scripts/domain-map.js`:
- Around line 14-17: The prefix lookup in findPathMapping is order-dependent
because it returns the first match from domainMap.pathMappings, so make the
selection order-independent by choosing the most specific matching entry instead
of relying on JSON ordering. Update findPathMapping to evaluate all matching
prefixes and pick the best match based on prefix length or specificity, using
normalizeRepoPath and the existing entry.prefix values, so future additions to
domainMap.pathMappings cannot silently mis-route domains.
In `@scripts/e2e_domains.js`:
- Around line 37-44: The domain existence check is spawning repeated go list
subprocesses for the same domain; memoize the result in domainExists so
classifyPath and addDomain can reuse it across multiple files. Add a Map-based
cache keyed by domain inside the e2e_domains.js flow, and have domainExists
consult the cache before calling execFileSync, then store both success and
failure results for later calls.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 5d120e07-e1ef-41e4-9fdf-c278af74d49a
📒 Files selected for processing (12)
.github/workflows/ci.ymlMakefilescripts/ci-workflow.test.shscripts/domain-map.jsscripts/domain-map.jsonscripts/e2e_domains.jsscripts/e2e_domains.test.jsscripts/pr-labels/index.jstests/cli_e2e/core.gotests/cli_e2e/core_test.gotests/cli_e2e/drive/helpers.gotests/cli_e2e/drive/helpers_test.go
💤 Files with no reviewable changes (4)
- tests/cli_e2e/drive/helpers_test.go
- tests/cli_e2e/drive/helpers.go
- tests/cli_e2e/core_test.go
- tests/cli_e2e/core.go
✅ Files skipped from review due to trivial changes (1)
- scripts/domain-map.json
f420058 to
3045b22
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
scripts/e2e_domains.js (2)
37-44: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winCache
domainExistsresults to avoid redundant subprocess spawns.
domainExistsruns a synchronousgo listsubprocess per call with no memoization; for PRs touching many files across the same domain(s), this results in repeated identical subprocess invocations.⚡ Proposed fix
+const domainExistsCache = new Map(); + function domainExists(domain) { + if (domainExistsCache.has(domain)) return domainExistsCache.get(domain); + let exists; try { execFileSync("go", ["list", `./tests/cli_e2e/${domain}`], { stdio: "ignore" }); - return true; + exists = true; } catch { - return false; + exists = false; } + domainExistsCache.set(domain, exists); + return exists; }🤖 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 `@scripts/e2e_domains.js` around lines 37 - 44, The domainExists helper currently spawns the same synchronous go list subprocess repeatedly with no reuse. Add memoization inside domainExists in scripts/e2e_domains.js so repeated checks for the same domain return a cached result instead of re-running execFileSync, keeping the try/catch behavior but avoiding duplicate subprocess calls.
165-180: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winGITHUB_OUTPUT entries aren't newline-safe.
reason/path-derived values are written as single-linekey=valueentries; a changed-file path containing an embedded newline would corrupt the GITHUB_OUTPUT file and could misparse subsequent keys. Since this data ultimately originates from PR-controlled filenames (same root cause as the injection risk inci.yml), consider using thekey<<EOFheredoc delimiter form for multi-line-safe output.🤖 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 `@scripts/e2e_domains.js` around lines 165 - 180, The emit() helper writes GITHUB_OUTPUT entries as single-line key=value pairs, which is unsafe for reason/path-derived data that may contain embedded newlines. Update emit() to write multi-line-safe GitHub Actions outputs using the key<<EOF heredoc form when appending to process.env.GITHUB_OUTPUT, while keeping the console output unchanged. Use the existing emit() function and the resolved values map to locate the output-writing logic..github/workflows/ci.yml (1)
277-278: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winConsider gating the build step on
mode != 'skip'too.Domain resolution now runs before
make build, but the build still executes unconditionally even when mode resolves toskip— wasting CI time on docs-only PRs that this feature is meant to optimize for.Also applies to: 323-326
🤖 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 @.github/workflows/ci.yml around lines 277 - 278, The CI workflow still runs the Build lark-cli step unconditionally even when the resolved mode is skip, so update the affected build steps in the workflow to respect the same mode check used by the domain resolution logic. Add the mode != 'skip' gate around the Build lark-cli job step (and the other referenced build block) so make build only runs when the workflow is not skipping, using the existing mode variable and nearby conditional patterns in the workflow.
🤖 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 337-350: The live CLI E2E workflow step is still interpolating
`${{ steps.e2e_domains.outputs.* }}` directly inside the shell script, which
creates the same injection risk as the dry-run job while real credentials are
available. Update the `Live CLI E2E` run block to follow the same env-var
mitigation pattern used elsewhere: pass `mode`, `reason`, and `live_packages`
from `steps.e2e_domains.outputs` into `env:` and reference only those shell
variables in the script, keeping `gotestsum` and the surrounding `if`/`echo`
logic intact.
- Around line 285-297: The CI step is interpolating untrusted e2e domain outputs
directly inside the shell script, which creates a script-injection risk. Update
the workflow job that runs the dry-run CLI E2E command to pass
steps.e2e_domains.outputs.mode, reason, and dry_packages through env instead of
embedding them in run:, then reference those values as shell variables in the
script. Keep the existing logic in the same run block, but ensure all references
to those outputs are read from the environment rather than from direct GitHub
expression expansion.
---
Nitpick comments:
In @.github/workflows/ci.yml:
- Around line 277-278: The CI workflow still runs the Build lark-cli step
unconditionally even when the resolved mode is skip, so update the affected
build steps in the workflow to respect the same mode check used by the domain
resolution logic. Add the mode != 'skip' gate around the Build lark-cli job step
(and the other referenced build block) so make build only runs when the workflow
is not skipping, using the existing mode variable and nearby conditional
patterns in the workflow.
In `@scripts/e2e_domains.js`:
- Around line 37-44: The domainExists helper currently spawns the same
synchronous go list subprocess repeatedly with no reuse. Add memoization inside
domainExists in scripts/e2e_domains.js so repeated checks for the same domain
return a cached result instead of re-running execFileSync, keeping the try/catch
behavior but avoiding duplicate subprocess calls.
- Around line 165-180: The emit() helper writes GITHUB_OUTPUT entries as
single-line key=value pairs, which is unsafe for reason/path-derived data that
may contain embedded newlines. Update emit() to write multi-line-safe GitHub
Actions outputs using the key<<EOF heredoc form when appending to
process.env.GITHUB_OUTPUT, while keeping the console output unchanged. Use the
existing emit() function and the resolved values map to locate the
output-writing logic.
🪄 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: 19a0860f-d9a0-424a-8b76-efbcdf045e16
📒 Files selected for processing (14)
.github/workflows/ci.ymlMakefilescripts/ci-workflow.test.shscripts/domain-map.jsscripts/domain-map.jsonscripts/e2e_domains.jsscripts/e2e_domains.test.jsscripts/pr-labels/index.jstests/cli_e2e/core.gotests/cli_e2e/core_test.gotests/cli_e2e/docs/coverage.mdtests/cli_e2e/docs/docs_fetch_dryrun_test.gotests/cli_e2e/drive/helpers.gotests/cli_e2e/drive/helpers_test.go
✅ Files skipped from review due to trivial changes (1)
- tests/cli_e2e/docs/coverage.md
🚧 Files skipped from review as they are similar to previous changes (7)
- Makefile
- scripts/pr-labels/index.js
- scripts/domain-map.json
- tests/cli_e2e/drive/helpers_test.go
- scripts/ci-workflow.test.sh
- tests/cli_e2e/drive/helpers.go
- tests/cli_e2e/core_test.go
3045b22 to
8f71254
Compare
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
tests/cli_e2e/wiki/helpers_test.go (1)
341-362: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winStale
lastTransientErrcan misreport a genuine "still exists" failure as transient.
lastTransientErris set whenever a transient error occurs but is never cleared on a subsequent successful (non-error) poll. If a transient blip happens early and later polls succeed but simply confirm the node is still present, the eventual timeout message ("kept hitting transient errors") misattributes the failure, obscuring the real cause during CI debugging.🐛 Proposed fix: reset lastTransientErr on successful checks
for { deleted, err := isWikiNodeDeleted(ctx, nodeToken) if err != nil { if isWikiVerifyTransientError(err) { lastTransientErr = err } else { return err } + } else { + lastTransientErr = nil } if deleted { 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 `@tests/cli_e2e/wiki/helpers_test.go` around lines 341 - 362, The wiki delete verification loop in isWikiNodeDeleted can misreport a timeout as transient because lastTransientErr is never cleared after a successful poll. Update the polling logic so that any non-error check that confirms the node is still present resets lastTransientErr before continuing, while keeping genuine transient failures recorded only until the next successful check. This change should be made in the helper around isWikiNodeDeleted and the deadline timeout handling that emits the “kept hitting transient errors” message.
🧹 Nitpick comments (1)
scripts/domain-map.js (1)
18-44: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winPrefix matching lacks path-segment boundaries.
findPathMapping,matchesFullFallback, andisSkippablePathall use barestartsWith/endsWithon normalized paths. If any two prefixes indomain-map.jsonshare a common string prefix at a non-segment boundary (e.g.shortcuts/imvsshortcuts/image), a file under the longer directory could be misrouted to the wrong domain. Since prefixes are sorted by string length (not segment count) for specificity, this is a real edge case rather than purely theoretical.Consider requiring the matched prefix to be followed by
/or be an exact match:🛡️ Proposed fix
function findPathMapping(filePath) { const normalized = normalizeRepoPath(filePath); - return pathMappingsBySpecificity.find((entry) => normalized.startsWith(entry.prefix)); + return pathMappingsBySpecificity.find( + (entry) => normalized === entry.prefix || normalized.startsWith(`${entry.prefix}/`) + ); }Since I don't have visibility into
scripts/domain-map.json, please confirm whether any prefixes actually overlap this way.🤖 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 `@scripts/domain-map.js` around lines 18 - 44, The path checks in findPathMapping, matchesFullFallback, and isSkippablePath rely on raw startsWith/endsWith against normalized paths, which can misclassify paths when one prefix is only a string prefix of another at a non-segment boundary. Update the matching logic to require either an exact match or a path-segment boundary (for example, a trailing slash after the prefix) while keeping the existing specificity behavior in pathMappingsBySpecificity and the skip/full-fallback helpers. Also verify whether any entries in domainMap or domain-map.json overlap this way so the boundary-aware match covers the real cases.
🤖 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.
Outside diff comments:
In `@tests/cli_e2e/wiki/helpers_test.go`:
- Around line 341-362: The wiki delete verification loop in isWikiNodeDeleted
can misreport a timeout as transient because lastTransientErr is never cleared
after a successful poll. Update the polling logic so that any non-error check
that confirms the node is still present resets lastTransientErr before
continuing, while keeping genuine transient failures recorded only until the
next successful check. This change should be made in the helper around
isWikiNodeDeleted and the deadline timeout handling that emits the “kept hitting
transient errors” message.
---
Nitpick comments:
In `@scripts/domain-map.js`:
- Around line 18-44: The path checks in findPathMapping, matchesFullFallback,
and isSkippablePath rely on raw startsWith/endsWith against normalized paths,
which can misclassify paths when one prefix is only a string prefix of another
at a non-segment boundary. Update the matching logic to require either an exact
match or a path-segment boundary (for example, a trailing slash after the
prefix) while keeping the existing specificity behavior in
pathMappingsBySpecificity and the skip/full-fallback helpers. Also verify
whether any entries in domainMap or domain-map.json overlap this way so the
boundary-aware match covers the real cases.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 73269703-d7ba-4822-9aaa-1f859711a91c
📒 Files selected for processing (15)
.github/workflows/ci.ymlMakefilescripts/ci-workflow.test.shscripts/domain-map.jsscripts/domain-map.jsonscripts/e2e_domains.jsscripts/e2e_domains.test.jsscripts/pr-labels/index.jstests/cli_e2e/core.gotests/cli_e2e/core_test.gotests/cli_e2e/docs/coverage.mdtests/cli_e2e/docs/docs_fetch_dryrun_test.gotests/cli_e2e/drive/helpers.gotests/cli_e2e/drive/helpers_test.gotests/cli_e2e/wiki/helpers_test.go
✅ Files skipped from review due to trivial changes (2)
- tests/cli_e2e/docs/coverage.md
- scripts/domain-map.json
🚧 Files skipped from review as they are similar to previous changes (6)
- Makefile
- scripts/ci-workflow.test.sh
- tests/cli_e2e/docs/docs_fetch_dryrun_test.go
- tests/cli_e2e/drive/helpers.go
- scripts/pr-labels/index.js
- tests/cli_e2e/core.go
8f71254 to
8fb665a
Compare
8fb665a to
6d1c360
Compare
PR Quality SummaryCI did not complete successfully. Use the failed check links below to decide whether this PR needs a code change or a rerun. Failed checksdeterministic-gate
|
🚀 PR Preview Install Guide🧰 CLI updatenpm i -g https://pkg.pr.new/larksuite/cli/@larksuite/cli@5f2994ab5064d465d5d3f13591ccad46e0c89fae🧩 Skill updatenpx skills add yxzhaao/cli#e2e-retryable-default -y -g |
6d1c360 to
5f2994a
Compare
Summary
RunCmdretry structured service failures whenerror.retryable=true, while keeping explicitRunCmdWithRetryfor custom predicates or retry budgetsscripts/domain-map.json;pr-labelsreads the same map for label domainstests/cli_e2epackage unfiltered, then domain packages withDryRun|Regressionmake buildwhen E2E mode isskipValidation
make script-testnode --test scripts/e2e_domains.test.jsbash scripts/ci-workflow.test.shGITHUB_TOKEN=$(gh auth token) node scripts/pr-labels/test.jsgo test ./tests/cli_e2e ./tests/cli_e2e/drive ./tests/cli_e2e/wiki -run 'TestRunCmd|TestRunCmdWithRetry|TestWaitForCondition|TestDeleteDriveResourceAndVerify|TestWikiVerifyTransientResult' -count=1\n-go test ./cmd -run 'TestIntegration_StrictModeUser_ProfileOverride_ServiceBotOnlyMethodReturnsEnvelope' -count=1 -v\n-go test ./cmd -run 'TestIntegration_StrictMode' -count=1\n-go test ./cmd/... -count=1