Skip to content

feat: add Base table copy shortcuts - #2019

Open
fongwave wants to merge 5 commits into
mainfrom
codex/base-table-copy
Open

feat: add Base table copy shortcuts#2019
fongwave wants to merge 5 commits into
mainfrom
codex/base-table-copy

Conversation

@fongwave

@fongwave fongwave commented Jul 23, 2026

Copy link
Copy Markdown
Collaborator

Summary

Add Base shortcuts for copying a table and querying asynchronous copy status. Table copies default to schema-only, with explicit support for copying all records and optionally waiting for completion.

Changes

  • Add base +table-copy with table ID/name support, schema-safe defaults, asynchronous continuation output, and bounded wait polling.
  • Add base +table-copy-status for one-shot task status queries using the original identity.
  • Register structured Base copy error codes and preserve typed API errors.
  • Add unit, dry-run E2E, workflow E2E, and Skill guidance for the new commands.

Test Plan

  • make unit-test
  • go vet ./...
  • gofmt -l .
  • go mod tidy leaves go.mod and go.sum unchanged
  • golangci-lint reports 0 issues
  • Base CLI E2E package passes
  • Manual verification confirms schema-only, all-record, wait, and no-wait flows

Related Issues

  • None

Summary by CodeRabbit

  • New Features
    • Added base +table-copy for structure-only or full-record copying, with dry-run, asynchronous waiting, and timeout options.
    • Added base +table-copy-status to monitor copy progress.
  • Bug Fixes
    • Improved request cancellation and timeout handling.
    • Refined table-copy polling and error guidance.
    • Enhanced HTTP mocking to support transport errors.
  • Documentation
    • Updated table-copy usage guidance and E2E coverage notes.

@CLAassistant

CLAassistant commented Jul 23, 2026

Copy link
Copy Markdown

CLA assistant check
All committers have signed the CLA.

@coderabbitai

coderabbitai Bot commented Jul 23, 2026

Copy link
Copy Markdown

Review Change Stack

📝 Walkthrough

Walkthrough

Adds +table-copy and +table-copy-status for Base. The commands support schema or record copying, asynchronous polling, timeout handling, continuation commands, context propagation, error classification, documentation, and unit and CLI E2E coverage.

Changes

Base table copy workflow

Layer / File(s) Summary
Request and error foundations
internal/errclass/*, internal/httpmock/*, shortcuts/common/runner.go, shortcuts/base/helpers.go
Adds Base error metadata, transport-error stubs, and context-aware API execution.
Shortcut contracts and registration
shortcuts/base/table_copy.go, shortcuts/base/shortcuts.go, shortcuts/base/table_copy_ops.go, shortcuts/base/*_test.go
Registers both commands, defines flags and validation, and adds dry-run and contract coverage.
Submission, status, and output flow
shortcuts/base/table_copy_ops.go, shortcuts/base/table_copy_test.go
Implements submission, response projection, status handling, continuation commands, output, and error transformations.
Deadline-aware status polling
shortcuts/base/table_copy_poll.go, shortcuts/base/table_copy_test.go
Adds injectable-clock polling with backoff, request deadlines, retry classification, and timeout-state handling.
CLI integration and guidance
tests/cli_e2e/base/*, skills/lark-base/SKILL.md, tests/cli_e2e/base/coverage.md
Adds dry-run and deployment-gated workflow tests, updates coverage metrics, and documents command usage and identity rules.

Estimated code review effort: 4 (Complex) | ~60 minutes

Sequence Diagram(s)

sequenceDiagram
  participant CLI
  participant executeTableCopy
  participant BaseAPI
  participant pollTableCopy
  participant StatusAPI
  CLI->>executeTableCopy: submit table-copy request
  executeTableCopy->>BaseAPI: create table copy
  BaseAPI-->>executeTableCopy: return state and task_id
  executeTableCopy->>pollTableCopy: wait when requested
  pollTableCopy->>StatusAPI: fetch status with remaining deadline
  StatusAPI-->>pollTableCopy: return task state
  pollTableCopy-->>CLI: success, timeout, or continuation command
Loading

Possibly related PRs

Suggested reviewers: liangshuo-1

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 1.30% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly and concisely identifies the main change: adding Base table copy shortcuts.
Description check ✅ Passed The description includes all required sections and clearly covers the scope, changes, testing, and related issues.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
✨ Finishing Touches 💡 1
📝 Generate docstrings 💡
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch codex/base-table-copy

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@github-actions github-actions Bot added domain/base PR touches the base domain size/L Large or sensitive change across domains or core paths labels Jul 23, 2026
@codecov

codecov Bot commented Jul 23, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 86.01036% with 54 lines in your changes missing coverage. Please review.
✅ Project coverage is 75.44%. Comparing base (cfe76ad) to head (efc9e79).

Files with missing lines Patch % Lines
shortcuts/base/table_copy_poll.go 63.76% 19 Missing and 6 partials ⚠️
shortcuts/base/table_copy_ops.go 91.25% 14 Missing and 9 partials ⚠️
shortcuts/base/table_copy.go 84.61% 3 Missing and 3 partials ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #2019      +/-   ##
==========================================
+ Coverage   75.39%   75.44%   +0.04%     
==========================================
  Files         924      928       +4     
  Lines       98005    98389     +384     
==========================================
+ Hits        73895    74225     +330     
- Misses      18478    18514      +36     
- Partials     5632     5650      +18     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@fongwave
fongwave force-pushed the codex/base-table-copy branch from 5884cdf to 3159090 Compare July 30, 2026 12:15
@fongwave
fongwave marked this pull request as ready for review July 30, 2026 12:44
@fongwave
fongwave requested a review from liangshuo-1 as a code owner July 30, 2026 12:44
@github-actions

github-actions Bot commented Jul 30, 2026

Copy link
Copy Markdown

🚀 PR Preview Install Guide

🧰 CLI update

npm i -g https://pkg.pr.new/larksuite/cli/@larksuite/cli@efc9e79eba0c6a92ec60e095bbddf84d4d52b7a0

🧩 Skill update

npx skills add larksuite/cli#codex/base-table-copy -y -g

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 2

🧹 Nitpick comments (6)
shortcuts/base/table_copy.go (2)

111-113: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

Derive the limit text from tableCopyTimeoutMax.

Both this message and the --timeout usage string on Line 46 hardcode 30m, so changing the constant silently desyncs user-facing text.

♻️ Suggested change
-	if wait && (timeout <= 0 || timeout > tableCopyTimeoutMax) {
-		return errs.NewValidationError(errs.SubtypeInvalidArgument, "--timeout must be greater than 0 and at most 30m").WithParam("--timeout")
-	}
+	if wait && (timeout <= 0 || timeout > tableCopyTimeoutMax) {
+		return errs.NewValidationError(errs.SubtypeInvalidArgument, "--timeout must be greater than 0 and at most %s", tableCopyTimeoutMax).WithParam("--timeout")
+	}
🤖 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 `@shortcuts/base/table_copy.go` around lines 111 - 113, Update the timeout
validation message and the `--timeout` usage string to derive their limit text
from `tableCopyTimeoutMax` instead of hardcoding `30m`. Keep the existing
validation behavior unchanged and use the shared constant consistently so both
user-facing messages stay synchronized.

86-91: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Add coverage for the blank --table-id / --name guards.

Codecov flags Lines 87 and 90 as uncovered. TestBaseTableCopyRejectsInvalidFlagCombinations never exercises whitespace-only values for these two flags, so reverting either guard would not fail the suite. A whitespace case per flag (mirroring the --task-id blank case) closes this.

As per coding guidelines, "Every behavior change must have an accompanying test, and contract tests must assert the changed field or behavior directly so reverting the implementation causes failure."

🤖 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 `@shortcuts/base/table_copy.go` around lines 86 - 91, Add whitespace-only cases
for both --table-id and --name to
TestBaseTableCopyRejectsInvalidFlagCombinations, mirroring the existing blank
--task-id coverage. Assert each case returns the invalid-argument validation
error for its specific flag so removing either guard causes the test to fail.

Sources: Coding guidelines, Linters/SAST tools

shortcuts/base/table_copy_ops.go (2)

85-90: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

submitTableCopy ignores the ctx passed into executeTableCopyWithClock.

The status path uses baseV3CallContext(ctx, ...), but submission goes through baseV3Call, which falls back to runtime.Ctx(). Today these are the same context, so behavior is correct, but the asymmetry means a future caller passing a derived/bounded ctx (as the polling path already does) would silently not bound the submit request.

♻️ Suggested change
-	submit, err := submitTableCopy(runtime, rangeValue)
+	submit, err := submitTableCopy(ctx, runtime, rangeValue)
func submitTableCopy(ctx context.Context, runtime *common.RuntimeContext, rangeValue string) (tableCopySubmitResult, error) {
	// ...
	data, err := baseV3CallContext(ctx, runtime, "POST", baseV3Path("bases", baseToken, "tables", tableRef, "copy"), nil, body)

Also applies to: 251-258

🤖 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 `@shortcuts/base/table_copy_ops.go` around lines 85 - 90, Update
submitTableCopy to accept a context.Context parameter and use it for the
submission request via baseV3CallContext; pass the ctx received by
executeTableCopyWithClock through to submitTableCopy, preserving the existing
error handling and polling behavior.

304-308: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

Name the magic values.

800010109 here and the "failed" state literal on Line 338 are the only unnamed values in a file that otherwise uses tableCopyState* constants; 800010109 is also duplicated in internal/errclass/codemeta_base.go. A tableCopyStateFailed = "failed" const and a named code const would keep this consistent.

Also applies to: 338-339

🤖 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 `@shortcuts/base/table_copy_ops.go` around lines 304 - 308, The table-copy code
uses unnamed status and error-code values; define named constants for the failed
state and code 800010109, reusing the existing shared code constant from
internal/errclass/codemeta_base.go if appropriate. Update tableCopyStatusError
and the state handling around the “failed” literal to reference these constants,
following the existing tableCopyState* naming convention.
shortcuts/base/table_copy_poll.go (2)

56-100: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Extract the repeated timeout-finalization branch.

The if !hasStatus && lastErr != nil { return tableCopyStatus{}, false, lastErr }; return lastStatus, true, nil pattern (or its err-based variant) is duplicated at lines 59-63, 77-81, 84-88, and 96-99. Consolidating into a helper reduces the risk of the branches drifting apart on future edits.

♻️ Proposed refactor
+func tableCopyTimeoutResult(hasStatus bool, lastStatus tableCopyStatus, err error) (tableCopyStatus, bool, error) {
+	if !hasStatus && err != nil {
+		return tableCopyStatus{}, false, err
+	}
+	return lastStatus, true, nil
+}
+
 func pollTableCopy(
 	ctx context.Context,
 	timeout time.Duration,
 	clock tableCopyClock,
 	fetch tableCopyStatusFetcher,
 ) (tableCopyStatus, bool, error) {
 	deadline := clock.Now().Add(timeout)
 	delay := tableCopyPollInitial
 	var lastStatus tableCopyStatus
 	var lastErr error
 	hasStatus := false

 	for {
 		remaining := deadline.Sub(clock.Now())
 		if remaining <= 0 {
-			if !hasStatus && lastErr != nil {
-				return tableCopyStatus{}, false, lastErr
-			}
-			return lastStatus, true, nil
+			return tableCopyTimeoutResult(hasStatus, lastStatus, lastErr)
 		}
 		...
 		if !clock.Now().Before(deadline) {
-			if !hasStatus && lastErr != nil {
-				return tableCopyStatus{}, false, lastErr
-			}
-			return lastStatus, true, nil
+			return tableCopyTimeoutResult(hasStatus, lastStatus, lastErr)
 		}
 		requestBudget := deadline.Sub(clock.Now())
 		if requestBudget <= 0 {
-			if !hasStatus && lastErr != nil {
-				return tableCopyStatus{}, false, lastErr
-			}
-			return lastStatus, true, nil
+			return tableCopyTimeoutResult(hasStatus, lastStatus, lastErr)
 		}
 		...
 		if !clock.Now().Before(deadline) {
-			if !hasStatus && err != nil {
-				return tableCopyStatus{}, false, err
-			}
-			return lastStatus, true, nil
+			return tableCopyTimeoutResult(hasStatus, lastStatus, err)
 		}
🤖 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 `@shortcuts/base/table_copy_poll.go` around lines 56 - 100, Extract the
duplicated timeout-finalization logic from the polling loop into a local helper
that accepts the relevant error value and returns the appropriate status,
completion flag, and error. Replace the repeated branches around deadline
checks, preserving the distinct use of lastErr before fetch and err after fetch
while keeping existing return behavior unchanged.

56-139: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Add coverage for the untested deadline/error and retry-classification branches.

Static analysis flags several non-trivial branches as untested: the deadline-reached-with-only-an-error paths (lines 59-60, 84-85, 87, 97), the ctx-cancellation return (70-72), the invalid-state error (113-114), and the default false (non-retryable) branch of tableCopyPollErrorRetryable (line 128). These are exactly the edge cases that make deadline-bounded polling correct; locking them down with tests would catch regressions if the branch order or classification logic changes later.

🤖 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 `@shortcuts/base/table_copy_poll.go` around lines 56 - 139, Add tests covering
the deadline/error return paths in the polling loop, including expiration before
polling, after timer wait, and after fetch when no successful status exists;
verify context cancellation returns ctx.Err(). Exercise invalid table-copy
states and assert the resulting internal error. Add tableCopyPollErrorRetryable
tests for non-retryable network/API and unknown errors, confirming the default
false classification while preserving existing retryable cases.

Source: Linters/SAST tools

🤖 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 `@tests/cli_e2e/base/base_table_copy_dryrun_test.go`:
- Around line 68-85: Extend the status coverage in
tests/cli_e2e/base/base_table_copy_dryrun_test.go:68-85 with a bot-authenticated
dry-run case that asserts the status request fields. In
tests/cli_e2e/base/base_table_copy_workflow_test.go:100-145, submit and poll one
copy task using bot identity, preserving that identity through status checks.
Update tests/cli_e2e/base/coverage.md:16-17 and :79-80 to retain the user/bot
coverage claims only if the new bot status coverage is present; otherwise narrow
those notes.

In `@tests/cli_e2e/base/base_table_copy_workflow_test.go`:
- Around line 29-38: Add a uniquely named field to the source schema created in
the base table-copy workflow, then inspect the copied table’s schema and assert
that field exists with the expected definition. Update the related assertions in
the workflow around the copied-table validation so the test directly fails when
source fields are omitted, while preserving the existing table-copy checks.

---

Nitpick comments:
In `@shortcuts/base/table_copy_ops.go`:
- Around line 85-90: Update submitTableCopy to accept a context.Context
parameter and use it for the submission request via baseV3CallContext; pass the
ctx received by executeTableCopyWithClock through to submitTableCopy, preserving
the existing error handling and polling behavior.
- Around line 304-308: The table-copy code uses unnamed status and error-code
values; define named constants for the failed state and code 800010109, reusing
the existing shared code constant from internal/errclass/codemeta_base.go if
appropriate. Update tableCopyStatusError and the state handling around the
“failed” literal to reference these constants, following the existing
tableCopyState* naming convention.

In `@shortcuts/base/table_copy_poll.go`:
- Around line 56-100: Extract the duplicated timeout-finalization logic from the
polling loop into a local helper that accepts the relevant error value and
returns the appropriate status, completion flag, and error. Replace the repeated
branches around deadline checks, preserving the distinct use of lastErr before
fetch and err after fetch while keeping existing return behavior unchanged.
- Around line 56-139: Add tests covering the deadline/error return paths in the
polling loop, including expiration before polling, after timer wait, and after
fetch when no successful status exists; verify context cancellation returns
ctx.Err(). Exercise invalid table-copy states and assert the resulting internal
error. Add tableCopyPollErrorRetryable tests for non-retryable network/API and
unknown errors, confirming the default false classification while preserving
existing retryable cases.

In `@shortcuts/base/table_copy.go`:
- Around line 111-113: Update the timeout validation message and the `--timeout`
usage string to derive their limit text from `tableCopyTimeoutMax` instead of
hardcoding `30m`. Keep the existing validation behavior unchanged and use the
shared constant consistently so both user-facing messages stay synchronized.
- Around line 86-91: Add whitespace-only cases for both --table-id and --name to
TestBaseTableCopyRejectsInvalidFlagCombinations, mirroring the existing blank
--task-id coverage. Assert each case returns the invalid-argument validation
error for its specific flag so removing either guard causes the test to fail.
🪄 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: 9a6d8fb7-1d23-4d8e-a8eb-cff874583487

📥 Commits

Reviewing files that changed from the base of the PR and between 6e5308a and 3159090.

📒 Files selected for processing (16)
  • internal/errclass/codemeta_base.go
  • internal/errclass/codemeta_base_test.go
  • internal/httpmock/registry.go
  • internal/httpmock/registry_test.go
  • shortcuts/base/base_shortcuts_test.go
  • shortcuts/base/helpers.go
  • shortcuts/base/shortcuts.go
  • shortcuts/base/table_copy.go
  • shortcuts/base/table_copy_ops.go
  • shortcuts/base/table_copy_poll.go
  • shortcuts/base/table_copy_test.go
  • shortcuts/common/runner.go
  • skills/lark-base/SKILL.md
  • tests/cli_e2e/base/base_table_copy_dryrun_test.go
  • tests/cli_e2e/base/base_table_copy_workflow_test.go
  • tests/cli_e2e/base/coverage.md

Comment thread tests/cli_e2e/base/base_table_copy_dryrun_test.go
Comment on lines +29 to +38
baseToken := createBaseWithRetry(t, ctx, "lark-cli-e2e-table-copy-"+clie2e.GenerateSuffix())
sourceTableID, _, _ := createTableWithRetry(
t,
parentT,
ctx,
baseToken,
"Copy source "+clie2e.GenerateSuffix(),
`[{"name":"Name","type":"text"}]`,
`{"name":"Main","type":"grid"}`,
)

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🎯 Functional Correctness | 🟠 Major | ⚡ Quick win

Assert that the schema itself was copied.

This only proves an empty target table was created. Add a distinct source field and assert it exists on the copied table; an implementation that omits source fields currently passes.

As per coding guidelines, “contract tests must assert the changed field or behavior directly so reverting the implementation causes failure.”

Also applies to: 80-97

🤖 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/base/base_table_copy_workflow_test.go` around lines 29 - 38,
Add a uniquely named field to the source schema created in the base table-copy
workflow, then inspect the copied table’s schema and assert that field exists
with the expected definition. Update the related assertions in the workflow
around the copied-table validation so the test directly fails when source fields
are omitted, while preserving the existing table-copy checks.

Source: Coding guidelines

@github-actions

github-actions Bot commented Jul 30, 2026

Copy link
Copy Markdown

PR Quality Summary

CI did not complete successfully. Use the failed check links below to decide whether this PR needs a code change or a rerun.

Failed checks

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)
shortcuts/base/table_copy_test.go (1)

735-804: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win

Add Category/Subtype assertions to these error-path tests.

As per coding guidelines, **/*_test.go: "Error-path tests must assert typed metadata through errs.ProblemOf (category, subtype, and param) and verify cause preservation rather than relying only on message substrings." These three tests only check problem.Code (Line 806-861 additionally checks Subtype), leaving Category (and, where applicable, Subtype) unverified for a PR whose main goal is fixing error classification/continuation gating.

✅ Suggested additions
 	problem, ok := errs.ProblemOf(err)
-	if !ok || problem.Code != 800010109 {
+	if !ok || problem.Code != 800010109 || problem.Category != errs.CategoryAPI || problem.Subtype != /* expected subtype */ {
 		t.Fatalf("error = %T %v, problem=%#v", err, err, problem)
 	}
 	problem, ok := errs.ProblemOf(err)
-	if !ok || problem.Code != 800030110 || problem.Subtype != errs.SubtypeNotFound {
+	if !ok || problem.Code != 800030110 || problem.Category != errs.CategoryAPI || problem.Subtype != errs.SubtypeNotFound {
 		t.Fatalf("error = %T %v, problem=%#v", err, err, problem)
 	}
 	problem, ok := errs.ProblemOf(err)
-	if !ok || problem.Code != 800070111 {
+	if !ok || problem.Code != 800070111 || problem.Category != errs.CategoryAPI || problem.Subtype != /* expected subtype */ {
 		t.Fatalf("error = %T %v, problem=%#v", err, err, problem)
 	}

Also applies to: 806-861, 862-932

🤖 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 `@shortcuts/base/table_copy_test.go` around lines 735 - 804, Add typed error
assertions to TestBaseTableCopyWaitInvalidTaskDoesNotSuggestMorePolling and the
two adjacent table-copy error-path tests: after errs.ProblemOf, verify the
expected Category, Subtype, and Param values for each classification, and assert
the underlying cause is preserved using the test’s existing cause/error
sentinel. Keep the current code and hint assertions while covering all metadata
required by the error contract.

Source: Coding guidelines

🧹 Nitpick comments (1)
shortcuts/base/table_copy_ops.go (1)

139-161: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

Compute tableCopyWaitCanContinue once and thread it through.

tableCopyWaitCanContinue(pollErr) is evaluated independently in the recovery-envelope path (Line 151) and inside tableCopyWaitError (Line 221). Both use the same pollErr, so today they can't diverge, but the duplication couples two decision points to one pure function by convention only — a future change to the classifier (e.g., adding any non-deterministic or stateful check) could make stdout and the returned error disagree about continuability.

♻️ Suggested refactor
-		if pollErr != nil {
+		if pollErr != nil {
+			canContinue := tableCopyWaitCanContinue(pollErr)
 			recoveryState := status.State
 			if recoveryState == "" {
 				recoveryState = submit.State
 			}
 			recovery := tableCopyOutput{
 				Table:     submit.Table,
 				Range:     rangeValue,
 				State:     recoveryState,
 				Completed: false,
 				TaskID:    submit.TaskID,
 			}
-			if tableCopyWaitCanContinue(pollErr) {
+			if canContinue {
 				recovery.NextAction = "poll_status"
 				recovery.NextCommand = tableCopyNextCommand(runtime, runtime.Str("base-token"), submit.TaskID)
 			}
 			recoveryErr := runtime.OutPartialFailure(recovery, nil)
 			var partialFailure *output.PartialFailureError
 			if !errors.As(recoveryErr, &partialFailure) {
 				return recoveryErr
 			}
-			return tableCopyWaitError(pollErr)
+			return tableCopyWaitError(pollErr, canContinue)
 		}
-func tableCopyWaitError(err error) error {
-	if !tableCopyWaitCanContinue(err) {
+func tableCopyWaitError(err error, canContinue bool) error {
+	if !canContinue {

Also applies to: 213-227

🤖 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 `@shortcuts/base/table_copy_ops.go` around lines 139 - 161, Compute
tableCopyWaitCanContinue(pollErr) once in the poll-error handling flow and pass
the resulting boolean into tableCopyWaitError, reusing it for both
recovery.NextAction/NextCommand and the returned error. Update the
tableCopyWaitError signature and its caller consistently while preserving the
existing 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.

Outside diff comments:
In `@shortcuts/base/table_copy_test.go`:
- Around line 735-804: Add typed error assertions to
TestBaseTableCopyWaitInvalidTaskDoesNotSuggestMorePolling and the two adjacent
table-copy error-path tests: after errs.ProblemOf, verify the expected Category,
Subtype, and Param values for each classification, and assert the underlying
cause is preserved using the test’s existing cause/error sentinel. Keep the
current code and hint assertions while covering all metadata required by the
error contract.

---

Nitpick comments:
In `@shortcuts/base/table_copy_ops.go`:
- Around line 139-161: Compute tableCopyWaitCanContinue(pollErr) once in the
poll-error handling flow and pass the resulting boolean into tableCopyWaitError,
reusing it for both recovery.NextAction/NextCommand and the returned error.
Update the tableCopyWaitError signature and its caller consistently while
preserving the existing behavior.

ℹ️ Review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro Plus

Run ID: 0cd7b70e-8854-44cf-b79b-ca31a142a6e2

📥 Commits

Reviewing files that changed from the base of the PR and between 3159090 and 4c78be8.

📒 Files selected for processing (7)
  • shortcuts/base/table_copy.go
  • shortcuts/base/table_copy_ops.go
  • shortcuts/base/table_copy_test.go
  • skills/lark-base/SKILL.md
  • tests/cli_e2e/base/base_table_copy_dryrun_test.go
  • tests/cli_e2e/base/base_table_copy_workflow_test.go
  • tests/cli_e2e/base/coverage.md
💤 Files with no reviewable changes (1)
  • skills/lark-base/SKILL.md
🚧 Files skipped from review as they are similar to previous changes (2)
  • shortcuts/base/table_copy.go
  • tests/cli_e2e/base/coverage.md

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 `@shortcuts/base/table_copy_test.go`:
- Around line 923-925: Update the error assertions in the table-copy error-path
tests around ProblemOf to require a non-empty problem subtype, and verify the
original upstream API error remains discoverable through errors.Is or errors.As.
Preserve the existing code and category checks while ensuring
tableCopyWaitError’s typed metadata and cause wrapping are covered.
🪄 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: 49fa8656-335d-416f-b51e-e72942d9a26d

📥 Commits

Reviewing files that changed from the base of the PR and between 4c78be8 and 9dc8143.

📒 Files selected for processing (2)
  • shortcuts/base/table_copy_ops.go
  • shortcuts/base/table_copy_test.go

Comment thread shortcuts/base/table_copy_test.go
@fongwave
fongwave force-pushed the codex/base-table-copy branch from 9dc8143 to efc9e79 Compare July 31, 2026 03:13

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 `@skills/lark-base/SKILL.md`:
- Line 91: Update the +table-copy --wait recovery guidance to apply only when an
asynchronous wait remains unfinished: state that the task_id is printed to
stderr in that wait path, while immediately completed submissions should use the
stdout result. Preserve the existing Ctrl-C recovery instructions for unfinished
waits.
🪄 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: 5f672b7a-22e5-412e-bfb1-7dbabad24fbd

📥 Commits

Reviewing files that changed from the base of the PR and between 9dc8143 and efc9e79.

📒 Files selected for processing (16)
  • internal/errclass/codemeta_base.go
  • internal/errclass/codemeta_base_test.go
  • internal/httpmock/registry.go
  • internal/httpmock/registry_test.go
  • shortcuts/base/base_shortcuts_test.go
  • shortcuts/base/helpers.go
  • shortcuts/base/shortcuts.go
  • shortcuts/base/table_copy.go
  • shortcuts/base/table_copy_ops.go
  • shortcuts/base/table_copy_poll.go
  • shortcuts/base/table_copy_test.go
  • shortcuts/common/runner.go
  • skills/lark-base/SKILL.md
  • tests/cli_e2e/base/base_table_copy_dryrun_test.go
  • tests/cli_e2e/base/base_table_copy_workflow_test.go
  • tests/cli_e2e/base/coverage.md
🚧 Files skipped from review as they are similar to previous changes (14)
  • shortcuts/base/shortcuts.go
  • shortcuts/base/base_shortcuts_test.go
  • internal/errclass/codemeta_base.go
  • internal/errclass/codemeta_base_test.go
  • tests/cli_e2e/base/coverage.md
  • internal/httpmock/registry.go
  • shortcuts/base/helpers.go
  • internal/httpmock/registry_test.go
  • shortcuts/common/runner.go
  • shortcuts/base/table_copy_poll.go
  • tests/cli_e2e/base/base_table_copy_dryrun_test.go
  • tests/cli_e2e/base/base_table_copy_workflow_test.go
  • shortcuts/base/table_copy_ops.go
  • shortcuts/base/table_copy_test.go

Comment thread skills/lark-base/SKILL.md
## 身份与权限降级

- 默认显式使用 `--as user` 操作用户资源;只有用户明确要求应用身份时,才直接用 `--as bot`。
- `+table-copy --wait` 提交成功后会在 stderr 打印完整 `task_id`;若进程被 Ctrl-C 终止,可用该 ID 和原身份执行 `+table-copy-status` 续查,不要重新提交复制。

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win

Limit this recovery instruction to unfinished asynchronous waits.

shortcuts/base/table_copy_ops.go:85-197 prints the task_id progress message only after an all-range submission returns a non-success state and enters the wait path. If the submission completes immediately, the command returns before printing that stderr message. State that unfinished asynchronous waits print the task_id; use the stdout result when the task completes immediately.

🤖 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 `@skills/lark-base/SKILL.md` at line 91, Update the +table-copy --wait recovery
guidance to apply only when an asynchronous wait remains unfinished: state that
the task_id is printed to stderr in that wait path, while immediately completed
submissions should use the stdout result. Preserve the existing Ctrl-C recovery
instructions for unfinished waits.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

domain/base PR touches the base domain size/L Large or sensitive change across domains or core paths

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants