fix: validate drive import folder target#1485
Conversation
github-actions
Bot
added
domain/ccm
|
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:
📝 WalkthroughWalkthrough
ChangesWiki folder-token validation for drive +import
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning Review ran into problems🔥 ProblemsStopped waiting for pipeline failures after 30000ms. One of your pipelines takes longer than our 30000ms fetch window to run, so review may not consider pipeline-failure results for inline comments if any failures occurred after the fetch window. Increase the timeout if you want to wait longer or run a 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@5f239c9a95216e68099d1ffaaca4dd44033837e4🧩 Skill updatenpx skills add larksuite/cli#fix/drive-import-reject-wiki-folder-token -y -g |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
shortcuts/drive/drive_import_common_test.go (1)
315-330: ⚡ Quick winStrengthen typed metadata assertions for the rejection path.
This test already checks
*errs.ValidationError+Param; please also assert category/subtype viaerrs.ProblemOf(err)so the error-path test validates typed metadata by contract, not only message/hint substrings.As per coding guidelines, error-path tests should assert typed metadata via
errs.ProblemOf, and based on learnings, keeperrors.AsforValidationError.ParambecauseProblemOfdoes not exposeParam.🤖 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/drive/drive_import_common_test.go` around lines 315 - 330, The test validation in the rejection path for the `--folder-token` parameter check should use `errs.ProblemOf(err)` in addition to the existing `errors.As` check to validate the error's typed metadata by contract. After the successful `errors.As` assertion for `*errs.ValidationError`, add an assertion that calls `errs.ProblemOf(err)` and validates the returned error metadata (such as category or problem type), while keeping the existing `errors.As` usage for checking the `Param` field since `ProblemOf` does not expose that field. This ensures the error-path test validates both the structured error type and its categorized metadata.Sources: Coding guidelines, Learnings
🤖 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/drive/drive_import_common.go`:
- Around line 314-315: The code at line 314 currently treats all
runtime.CallAPITyped errors as benign wiki-resolution failures and continues
silently. Instead, you need to differentiate between expected wiki-resolution
API failures (such as not-found or permission-denied errors) and unexpected
transport or internal errors. For expected failures like missing wiki nodes or
permission issues, continue with the current fallthrough behavior. For other
error types (transport, internal, or unexpected errors), propagate the error
unchanged by returning it rather than swallowing it, so that actual failures are
not masked as benign results.
---
Nitpick comments:
In `@shortcuts/drive/drive_import_common_test.go`:
- Around line 315-330: The test validation in the rejection path for the
`--folder-token` parameter check should use `errs.ProblemOf(err)` in addition to
the existing `errors.As` check to validate the error's typed metadata by
contract. After the successful `errors.As` assertion for
`*errs.ValidationError`, add an assertion that calls `errs.ProblemOf(err)` and
validates the returned error metadata (such as category or problem type), while
keeping the existing `errors.As` usage for checking the `Param` field since
`ProblemOf` does not expose that field. This ensures the error-path test
validates both the structured error type and its categorized metadata.
🪄 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: f7c69d08-2f00-45b5-877f-d9f24e6fbd12
📒 Files selected for processing (5)
shortcuts/drive/drive_import.goshortcuts/drive/drive_import_common.goshortcuts/drive/drive_import_common_test.goshortcuts/drive/drive_import_test.gotests/cli_e2e/drive/drive_import_dryrun_test.go
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1485 +/- ##
==========================================
+ Coverage 73.35% 73.38% +0.02%
==========================================
Files 750 751 +1
Lines 69264 69905 +641
==========================================
+ Hits 50811 51301 +490
- Misses 14713 14804 +91
- Partials 3740 3800 +60 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
e8c5f3a to
ae21aae
Compare
There was a problem hiding this comment.
♻️ Duplicate comments (1)
shortcuts/drive/drive_import_common.go (1)
277-299:⚠️ Potential issue | 🟠 Major | ⚡ Quick winDo not swallow typed wiki-probe failures.
Line 297 currently treats every
runtime.CallAPITypedfailure as a benign “not wiki” result and continues. That masks transport/internal failures and downgrades error classification. Only expected wiki-resolution probe failures should fall through; otherwise return the typed error unchanged.As per coding guidelines, when a lower layer already returns a typed error, pass it through unchanged to avoid downgrades in error classification.
🤖 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/drive/drive_import_common.go` around lines 277 - 299, The code currently treats all errors from the runtime.CallAPITyped call as benign "not a wiki node" results and falls through to return nil, which masks actual transport and internal failures. Instead of checking only if err == nil, you need to distinguish between expected wiki-probe failures and genuine errors. When err is not nil, check if it is an expected wiki-resolution failure (such as a "not found" or similar expected type); only then continue with the fmt.Fprintf fallthrough. For any other error from the CallAPITyped call, return the typed error unchanged to preserve the error classification and avoid downgrades.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.
Duplicate comments:
In `@shortcuts/drive/drive_import_common.go`:
- Around line 277-299: The code currently treats all errors from the
runtime.CallAPITyped call as benign "not a wiki node" results and falls through
to return nil, which masks actual transport and internal failures. Instead of
checking only if err == nil, you need to distinguish between expected wiki-probe
failures and genuine errors. When err is not nil, check if it is an expected
wiki-resolution failure (such as a "not found" or similar expected type); only
then continue with the fmt.Fprintf fallthrough. For any other error from the
CallAPITyped call, return the typed error unchanged to preserve the error
classification and avoid downgrades.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: bd99e589-a297-4c4f-8f28-9591168079ca
📒 Files selected for processing (5)
shortcuts/drive/drive_import.goshortcuts/drive/drive_import_common.goshortcuts/drive/drive_import_common_test.goshortcuts/drive/drive_import_test.gotests/cli_e2e/drive/drive_import_dryrun_test.go
🚧 Files skipped from review as they are similar to previous changes (4)
- shortcuts/drive/drive_import_test.go
- tests/cli_e2e/drive/drive_import_dryrun_test.go
- shortcuts/drive/drive_import_common_test.go
- shortcuts/drive/drive_import.go
ae21aae to
85019bd
Compare
Change-Id: I43755c3966b0daa06b708d2b3d03294f439547fa
85019bd to
5f239c9
Compare
wittam-01
changed the title
There was a problem hiding this comment.
🧹 Nitpick comments (1)
shortcuts/drive/drive_import_test.go (1)
121-123: ⚡ Quick winAssert wiki probe endpoint/method, not only nil body.
At Line 121-Line 123, this only proves the first call has no body. Please also assert API[0] is
GET /open-apis/wiki/v2/spaces/get_nodeso the test actually locks the wiki-probe contract.Suggested test hardening
var got struct { API []struct { + Method string `json:"method"` + URL string `json:"url"` Body map[string]interface{} `json:"body"` } `json:"api"` } @@ if got.API[0].Body != nil { t.Fatalf("wiki probe should not have a request body, got %#v", got.API[0].Body) } + if got.API[0].Method != "GET" || got.API[0].URL != "/open-apis/wiki/v2/spaces/get_node" { + t.Fatalf("first API call = %s %s, want GET /open-apis/wiki/v2/spaces/get_node", got.API[0].Method, got.API[0].URL) + }🤖 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/drive/drive_import_test.go` around lines 121 - 123, The test assertion at lines 121-123 only verifies that got.API[0].Body is nil, but does not fully lock in the wiki-probe contract. Add additional assertions in the same block to verify that API[0] has the correct HTTP method (GET) and the correct endpoint path (/open-apis/wiki/v2/spaces/get_node) in addition to the existing body nil check. This will ensure the test comprehensively validates the wiki probe request contract rather than just checking for a nil body.
🤖 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 `@shortcuts/drive/drive_import_test.go`:
- Around line 121-123: The test assertion at lines 121-123 only verifies that
got.API[0].Body is nil, but does not fully lock in the wiki-probe contract. Add
additional assertions in the same block to verify that API[0] has the correct
HTTP method (GET) and the correct endpoint path
(/open-apis/wiki/v2/spaces/get_node) in addition to the existing body nil check.
This will ensure the test comprehensively validates the wiki probe request
contract rather than just checking for a nil body.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 6a4573d5-0ce6-439f-a7fb-ee1f403e9642
📒 Files selected for processing (5)
shortcuts/drive/drive_import.goshortcuts/drive/drive_import_common.goshortcuts/drive/drive_import_common_test.goshortcuts/drive/drive_import_test.gotests/cli_e2e/drive/drive_import_dryrun_test.go
🚧 Files skipped from review as they are similar to previous changes (4)
- shortcuts/drive/drive_import.go
- shortcuts/drive/drive_import_common_test.go
- tests/cli_e2e/drive/drive_import_dryrun_test.go
- shortcuts/drive/drive_import_common.go
2 participants
Summary
Add preflight validation for
drive +import --folder-tokenso the CLI verifies whether the provided target token resolves as a Wiki node before uploading import media. This keeps import target handling explicit and guides users to provide a Drive folder token.Changes
wiki.spaces.get_nodewhendrive +import --folder-tokenis provided to verify whether the token is a Wiki node.--folder-tokenwhen the target resolves as a Wiki node.wiki:node:retrieveas a conditional scope.Test Plan
make unit-testgo vet ./...gofmt -l .produced no outputgo mod tidyproduced nogo.mod/go.sumdiffGOTOOLCHAIN=go1.25.9 go run github.com/golangci/golangci-lint/v2/cmd/golangci-lint@v2.1.6 run --new-from-rev=origin/main(0 issues)go test ./shortcuts/drive -run 'TestDriveImport(RejectsWikiFolderToken|ContinuesWhenFolderTokenDoesNotResolveAsWiki|DryRunUsesExtensionlessDefaultName)'LARK_CLI_BIN=/tmp/lark-cli-import-wiki-check go test ./tests/cli_e2e/drive -run TestDriveImportDryRunFolderTokenWikiProbevalidation/invalid_argument, verified a Drive folder target imports docx successfully, and cleaned up all temporary resources.Related Issues
Summary by CodeRabbit
New Features
--folder-tokenis provided, to distinguish wiki nodes from Drive folder tokens.Bug Fixes
--folder-tokenare now rejected with a validation error and guidance to omit--folder-tokenfor Drive root imports.--folder-tokenis validated via the wiki probe; if it doesn’t resolve to a wiki node, the import continues as a Drive folder token.Tests