Skip to content

fix: add stderr warnings when auto permission grant is skipped or fails#1015

Merged
fangshuyu-768 merged 1 commit into
mainfrom
fix/permission-grant-stderr-warning
May 21, 2026
Merged

fix: add stderr warnings when auto permission grant is skipped or fails#1015
fangshuyu-768 merged 1 commit into
mainfrom
fix/permission-grant-stderr-warning

Conversation

@fangshuyu-768

@fangshuyu-768 fangshuyu-768 commented May 21, 2026

Copy link
Copy Markdown
Collaborator

Summary

Closes #963

When a resource is created with bot identity, the CLI attempts to auto-grant full_access to the current user. However, if the user open_id is missing or the grant API call fails, the result was only written to the JSON permission_grant field and easily overlooked by users.

This PR adds visible stderr warnings in both cases so users are clearly informed and can take action.

Changes

  • When UserOpenId is empty (permission_grant.go:51): print a stderr warning advising the user to run lark-cli auth login or grant permission manually.
  • When the grant API call fails (permission_grant.go:80): print a stderr warning including the error reason, advising the user to retry later or grant permission manually.

Impact

All 12 shortcuts that call AutoGrantCurrentUserDrivePermission (docs, drive, sheets, slides, base, markdown, wiki) automatically benefit — no per-shortcut changes needed.

Test

All existing tests pass without modification:

  • TestDocsCreateV2BotAutoGrantSkippedWithoutCurrentUser
  • TestDocsCreateV2BotAutoGrantFailureDoesNotFailCreate
  • TestDocsCreateV1WikiSpaceAutoGrantFailure
  • TestDriveUploadBotAutoGrantSuccess
  • TestDriveUploadUserSkipsPermissionGrantAugmentation

Summary by CodeRabbit

  • Improvements
    • Auto-grant operations now emit clear stderr warnings when skipped or when a grant attempt fails, and returned results include user-facing hints (e.g., log in, retry later, or grant permission manually).
  • Tests
    • Added tests that capture and assert stderr warnings and verify the new status/hint behaviors for skipped and failed auto-grant scenarios.

Review Change Stack

@coderabbitai

coderabbitai Bot commented May 21, 2026

Copy link
Copy Markdown

Note

Reviews paused

It 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 reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

  • @coderabbitai resume to resume automatic reviews.
  • @coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

  • ▶️ Resume reviews
  • 🔍 Trigger review
📝 Walkthrough

Walkthrough

The PR adds stderr warning messages in shortcuts/common/permission_grant.go when auto-granting bot-created resources is skipped for a missing runtime.UserOpenId() and when the permission-grant API call fails, and updates docs create and unit tests to capture and assert those stderr warnings.

Changes

Permission Grant Failure Warnings

Layer / File(s) Summary
Permission grant failure warning outputs
shortcuts/common/permission_grant.go
autoGrantCurrentUserDrivePermission now constructs the skipped/failed result into a local variable, writes a localized warning to runtime.IO().ErrOut (skipped when UserOpenId missing; failed when permissions API returns an error including the compacted error), then returns the result. buildPermissionGrantResult also sets hint text for PermissionGrantSkipped and PermissionGrantFailed statuses.
Unit tests for permission grant stderr warnings
shortcuts/common/permission_grant_test.go
Adds tests TestAutoGrantStderrWarning_SkippedNoUser and TestAutoGrantStderrWarning_GrantFailed that validate returned statuses and hint strings and assert stderr contains the corresponding warnings; grant-failed uses httpmock.
Docs create tests capture stderr
shortcuts/doc/docs_create_test.go
TestDocsCreateV2BotAutoGrantSkippedWithoutCurrentUser and TestDocsCreateV2BotAutoGrantFailureDoesNotFailCreate now capture stderr via cmdutil.TestFactory and assert stderr contains the respective “auto-grant was skipped” and “auto-grant failed” warnings.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Possibly related PRs

Suggested labels

size/M

Suggested reviewers

  • kongenpei

Poem

🐰 I nibbled logs beneath the moonlit glow,
I warn when grants decide they cannot go,
No silent skips — stderr sings the tale,
Tests listen close and ensure the trail,
A rabbit hops off, pleased the warnings show.

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% 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 specifically describes the main change: adding stderr warnings when auto permission grant is skipped or fails, which aligns with the primary objective of the PR.
Description check ✅ Passed The description is complete with all required sections: Summary (with issue closure), Changes (detailing the two scenarios with file locations), Impact (mentioning affected shortcuts), and Test (listing passing tests).
Linked Issues check ✅ Passed The code changes implement the core requirement from #963: adding visible warnings when auto-grant fails or is skipped, enabling users to take corrective action to grant permissions.
Out of Scope Changes check ✅ Passed All changes are directly related to the stated objective: modifying permission grant logic to emit warnings, adding hint fields to results, and updating tests to verify the new behavior.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch fix/permission-grant-stderr-warning

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 and usage tips.

@github-actions github-actions Bot added the size/L Large or sensitive change across domains or core paths label May 21, 2026

@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.

🧹 Nitpick comments (1)
shortcuts/common/permission_grant.go (1)

75-81: ⚡ Quick win

Consider caching the compacted error to avoid redundant computation.

The error is compacted twice—once for the result message (line 78) and once for the stderr output (line 80). Storing the result in a variable would eliminate the duplicate call.

♻️ Proposed refactor to cache the compacted error
 	if err != nil {
+		compactedErr := compactPermissionGrantError(err)
 		result := buildPermissionGrantResult(
 			PermissionGrantFailed,
 			userOpenID,
-			fmt.Sprintf("Resource was created, but granting current user %s failed: %s. You can retry later or continue using bot identity.", permissionGrantPermMessage(), compactPermissionGrantError(err)),
+			fmt.Sprintf("Resource was created, but granting current user %s failed: %s. You can retry later or continue using bot identity.", permissionGrantPermMessage(), compactedErr),
 		)
-		fmt.Fprintf(runtime.IO().ErrOut, "⚠ 资源已创建,但自动授权失败:%s。您可以稍后重试或手动为资源授权。\n", compactPermissionGrantError(err))
+		fmt.Fprintf(runtime.IO().ErrOut, "⚠ 资源已创建,但自动授权失败:%s。您可以稍后重试或手动为资源授权。\n", compactedErr)
 		return result
 	}
🤖 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/common/permission_grant.go` around lines 75 - 81, The
compactPermissionGrantError(err) call is executed twice; cache its result in a
local variable (e.g., compactErr) and reuse it when building the
PermissionGrantResult via buildPermissionGrantResult and when writing to
runtime.IO().ErrOut with fmt.Fprintf; update the block containing
buildPermissionGrantResult, permissionGrantPermMessage,
compactPermissionGrantError, and the fmt.Fprintf call to reference the cached
variable instead of calling compactPermissionGrantError twice.
🤖 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/common/permission_grant.go`:
- Around line 75-81: The compactPermissionGrantError(err) call is executed
twice; cache its result in a local variable (e.g., compactErr) and reuse it when
building the PermissionGrantResult via buildPermissionGrantResult and when
writing to runtime.IO().ErrOut with fmt.Fprintf; update the block containing
buildPermissionGrantResult, permissionGrantPermMessage,
compactPermissionGrantError, and the fmt.Fprintf call to reference the cached
variable instead of calling compactPermissionGrantError twice.

ℹ️ Review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: bd14196b-7e54-46de-ae90-3e0d7b750eb9

📥 Commits

Reviewing files that changed from the base of the PR and between 42746d6 and 2f8c9b5.

📒 Files selected for processing (1)
  • shortcuts/common/permission_grant.go

@codecov

codecov Bot commented May 21, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 88.23529% with 2 lines in your changes missing coverage. Please review.
✅ Project coverage is 67.71%. Comparing base (94b103d) to head (4c510d6).
⚠️ Report is 3 commits behind head on main.

Files with missing lines Patch % Lines
shortcuts/common/permission_grant.go 88.23% 2 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1015      +/-   ##
==========================================
+ Coverage   67.68%   67.71%   +0.03%     
==========================================
  Files         576      575       -1     
  Lines       54547    54360     -187     
==========================================
- Hits        36919    36811     -108     
+ Misses      14567    14501      -66     
+ Partials     3061     3048      -13     

☔ View full report in Codecov by Sentry.
📢 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.

@github-actions

github-actions Bot commented May 21, 2026

Copy link
Copy Markdown

🚀 PR Preview Install Guide

🧰 CLI update

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

🧩 Skill update

npx skills add larksuite/cli#fix/permission-grant-stderr-warning -y -g

@github-actions github-actions Bot added the domain/ccm PR touches the ccm domain label May 21, 2026
@fangshuyu-768
fangshuyu-768 force-pushed the fix/permission-grant-stderr-warning branch from d522e43 to 0ade710 Compare May 21, 2026 09:32
When a resource is created with bot identity, the CLI attempts to
auto-grant full_access to the current user. If the user open_id is
missing or the grant API call fails, the result was only written to
the JSON permission_grant field and easily overlooked.

Changes:
- Add stderr warnings when auto-grant is skipped or fails
- Add 'hint' field to permission_grant JSON output with failure reason
  and actionable next step (e.g. auth login, check scope, retry)
- Add end-to-end skipped/failed tests across all affected shortcuts
  (doc, drive, sheets, slides, wiki, markdown, base)

Closes #963
@fangshuyu-768
fangshuyu-768 force-pushed the fix/permission-grant-stderr-warning branch from 0ade710 to 4c510d6 Compare May 21, 2026 10:05
@fangshuyu-768
fangshuyu-768 merged commit 816927f into main May 21, 2026
20 checks passed
@fangshuyu-768
fangshuyu-768 deleted the fix/permission-grant-stderr-warning branch May 21, 2026 10:17
@liangshuo-1 liangshuo-1 mentioned this pull request May 21, 2026
3 tasks
tuxedomm pushed a commit to zhumiaoxin/cli that referenced this pull request Jun 6, 2026
…1015)

When a resource is created with bot identity, the CLI attempts to
auto-grant full_access to the current user. If the user open_id is
missing or the grant API call fails, the result was only written to
the JSON permission_grant field and easily overlooked.

Changes:
- Add stderr warnings when auto-grant is skipped or fails
- Add 'hint' field to permission_grant JSON output with failure reason
  and actionable next step (e.g. auth login, check scope, retry)
- Add end-to-end skipped/failed tests across all affected shortcuts
  (doc, drive, sheets, slides, wiki, markdown, base)

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

Labels

domain/ccm PR touches the ccm 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.

cli创建的文档无法给用户授权,导致用户不能修改、管理文档

2 participants