Skip to content

fix(service): advertise only supported identities in --as help - #2257

Open
Pr1p wants to merge 2 commits into
larksuite:mainfrom
Pr1p:codex/fix-2206-as-flag-help
Open

fix(service): advertise only supported identities in --as help#2257
Pr1p wants to merge 2 commits into
larksuite:mainfrom
Pr1p:codex/fix-2206-as-flag-help

Conversation

@Pr1p

@Pr1p Pr1p commented Aug 10, 2026

Copy link
Copy Markdown

Summary

API method commands registered the --as flag with a hardcoded "identity type: user | bot" help string even when the method metadata restricts identity (e.g. mail user_mailbox.drafts create only supports user). The --as help and shell completion are now derived from the method's declared identities, keeping --help in sync with the runtime CheckIdentity validation.

Changes

  • internal/cmdutil: AddAPIIdentityFlag accepts a supported-identity list; nil keeps the unrestricted default (user | bot), a non-nil slice is rendered verbatim in help and completion.
  • cmd/service: restricted API method commands pass their declared identities when registering --as; unrestricted methods keep the default.
  • cmd/api, cmd/whoami: explicitly keep the unrestricted user | bot shape.
  • Tests: API identity flag renders user-only/bot-only/default shapes; service-method regression tests cover restricted (user-only) and unrestricted commands.

Test Plan

  • Unit tests pass: go test ./internal/cmdutil ./cmd/service ./cmd/api ./cmd/whoami
  • go vet passes on the affected packages
  • gofmt clean
  • golangci-lint run --new-from-rev=origin/main reports 0 issues
  • Manual local verification confirms the lark-cli mail user_mailbox.drafts create --help flow shows identity type: user only

Related Issues

Summary by CodeRabbit

  • New Features

    • Identity selection now reflects the identities supported by each service method.
    • Command help and shell completion display only permitted identity options.
  • Bug Fixes

    • Prevented unsupported identity types from appearing for restricted methods.
    • Preserved user and bot options for unrestricted commands.
  • Tests

    • Added coverage for user-only, bot-only, and unrestricted identity configurations.

API method commands now derive --as help text and shell completion from their declared identities, matching the CheckIdentity runtime check. Restricted methods no longer advertise unsupported identities.

Fixes larksuite#2206.
@CLAassistant

CLAassistant commented Aug 10, 2026

Copy link
Copy Markdown

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
You have signed the CLA already but the status is still pending? Let us recheck it.

@coderabbitai

coderabbitai Bot commented Aug 10, 2026

Copy link
Copy Markdown

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro Plus

Run ID: 076d553b-318b-4305-aae4-bb23e72ebd07

📥 Commits

Reviewing files that changed from the base of the PR and between ee8065c and d368c75.

📒 Files selected for processing (1)
  • internal/cmdutil/identity_flag_test.go

📝 Walkthrough

Walkthrough

AddAPIIdentityFlag now accepts supported identity values. Service commands pass method restrictions so --as help and completion reflect user-only, bot-only, or unrestricted methods.

Changes

Identity restrictions

Layer / File(s) Summary
Identity flag contract
internal/cmdutil/identity_flag.go, internal/cmdutil/identity_flag_test.go, cmd/api/api.go, cmd/whoami/whoami.go
AddAPIIdentityFlag accepts optional supported identities. Nil input retains user | bot; restricted input controls usage text and completion values. Existing callers pass nil.
Service identity wiring
cmd/service/service.go, cmd/service/service_test.go
Service commands pass permitted identities for restricted methods. Tests cover user-only and unrestricted help text.

Estimated code review effort: 2 (Simple) | ~15 minutes

Suggested reviewers: liangshuo-1

🚥 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 concisely describes the primary change to advertise only supported identities in --as help.
Description check ✅ Passed The description includes the required summary, changes, test plan, and related issue sections with relevant implementation and verification details.
Linked Issues check ✅ Passed The changes address issue #2206 by deriving --as help text and completion values from each method's supported identities.
Out of Scope Changes check ✅ Passed All changed files and tests support identity filtering, unrestricted defaults, or regression coverage for the linked issue.
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

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 the size/L Large or sensitive change across domains or core paths label Aug 10, 2026
@Pr1p
Pr1p marked this pull request as ready for review August 10, 2026 07:01
@Pr1p
Pr1p requested a review from liangshuo-1 as a code owner August 10, 2026 07:01

@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)
internal/cmdutil/identity_flag_test.go (1)

19-34: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win

Test completion and the non-nil empty restriction.

These tests assert only flag usage text. AddAPIIdentityFlag also changes as completion values and distinguishes nil from a non-nil empty list. A regression in either behavior will pass this suite.

Invoke the registered as completion callback. Assert unrestricted, user-only, bot-only, and empty-list results directly.

As per coding guidelines, “Every behavior change must have an accompanying test, and contract tests must assert the changed field or behavior directly.”

Also applies to: 57-102

🤖 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/cmdutil/identity_flag_test.go` around lines 19 - 34, Expand the
tests for AddAPIIdentityFlag to invoke the registered as completion callback and
directly assert its results for nil (unrestricted), user-only, bot-only, and
non-nil empty restrictions. Verify that nil and an empty list produce distinct
completion behavior, while retaining the existing visibility, default-value, and
usage assertions.

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.

Outside diff comments:
In `@internal/cmdutil/identity_flag_test.go`:
- Around line 19-34: Expand the tests for AddAPIIdentityFlag to invoke the
registered as completion callback and directly assert its results for nil
(unrestricted), user-only, bot-only, and non-nil empty restrictions. Verify that
nil and an empty list produce distinct completion behavior, while retaining the
existing visibility, default-value, and usage assertions.

ℹ️ Review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro Plus

Run ID: f0a57659-09ad-4c38-8a47-1d15c49e0a2b

📥 Commits

Reviewing files that changed from the base of the PR and between 2016120 and ee8065c.

📒 Files selected for processing (6)
  • cmd/api/api.go
  • cmd/service/service.go
  • cmd/service/service_test.go
  • cmd/whoami/whoami.go
  • internal/cmdutil/identity_flag.go
  • internal/cmdutil/identity_flag_test.go

Addresses the CodeRabbit review on larksuite#2257: invoke the registered --as completion callback and assert unrestricted (user | bot), user-only, bot-only, and non-nil empty restriction results, including the nil vs empty distinction.
@coderabbitai coderabbitai Bot mentioned this pull request Aug 11, 2026
2 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

size/L Large or sensitive change across domains or core paths

Projects

None yet

Development

Successfully merging this pull request may close these issues.

mail user_mailbox.drafts create: --help advertises --as bot but the command rejects it

2 participants