fix: normalize vc meeting events contract - #1544
Open
Ren1104 wants to merge 9 commits into
Open
Conversation
github-actions
Bot
added
domain/vc
|
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
Changesvc +meeting-events: bot-only auth + compact/raw view
Sequence Diagram(s)sequenceDiagram
participant Agent
participant Execute as +meeting-events Execute
participant BotAPI as /open-apis/bot/v3/info
participant MeetingAPI as /open-apis/vc/v1/meetings/{id}
participant EventsAPI as VC Events API
Agent->>Execute: --as bot --view compact
Execute->>EventsAPI: fetch raw events (page_token, time range)
EventsAPI-->>Execute: raw event list + has_more/page_token
alt view=raw
Execute-->>Agent: raw JSON + NDJSON metadata count row
else view=compact
Execute->>BotAPI: GET bot identity
BotAPI-->>Execute: bot open_id, name
Execute->>MeetingAPI: GET participants / current_roster
MeetingAPI-->>Execute: roster list
Execute->>Execute: buildNormalizedMeetingEvents (normalize meeting/bot/roster/events)
alt format=ndjson
Execute-->>Agent: NDJSON rows + metadata row
else format=pretty
Execute-->>Agent: compact pretty text (bot label + roster + timeline)
end
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
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 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 |
🚀 PR Preview Install Guide🧰 CLI updatenpm i -g https://pkg.pr.new/larksuite/cli/@larksuite/cli@f61d6b4cd31514e1bf2f3951642a2d9a484c370d🧩 Skill updatenpx skills add larksuite/cli#features/F-vc-meeting-events-contract -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: 2
🤖 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/vc/vc_meeting_events_test.go`:
- Around line 637-648: The test for VCMeetingEvents.Validate at line 637 needs
to add typed Problem metadata assertions per test guidelines. After the existing
Param field assertion using errors.As for *errs.ValidationError, also call
errs.ProblemOf(err) on the validation error and assert the typed metadata fields
(at least Category and Subtype) to ensure comprehensive error-path testing while
maintaining the Param check via errors.As since ProblemOf does not expose it.
In `@shortcuts/vc/vc_meeting_events.go`:
- Around line 121-124: The error handling in the botInfo declaration is
incorrectly wrapping the network failure from runtime.BotInfo() as a validation
error using errs.NewValidationError with errs.SubtypeInvalidArgument. Replace
this with the appropriate network error type, use .WithCause(err) to preserve
the full error chain instead of flattening it with %v formatting, and remove the
.WithParam("--as") call since the flag itself was valid and the actual failure
was the remote fetch.
🪄 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: 45162c13-5801-4152-ad24-35ea3ac02dff
📒 Files selected for processing (5)
shortcuts/vc/vc_meeting_events.goshortcuts/vc/vc_meeting_events_test.goskills/lark-vc-agent/SKILL.mdskills/lark-vc-agent/references/lark-vc-agent-meeting-events.mdskills/lark-vc-agent/references/lark-vc-agent-meeting-list-active.md
Comment on lines
+637
to
+648
| err := VCMeetingEvents.Validate(context.Background(), runtime) | ||
| if err == nil { | ||
| t.Fatal("expected validation error for raw pretty") | ||
| } | ||
| var ve *errs.ValidationError | ||
| if !errors.As(err, &ve) { | ||
| t.Fatalf("expected *errs.ValidationError, got %T: %v", err, err) | ||
| } | ||
| if ve.Param != "--view" { | ||
| t.Errorf("Param = %q, want %q", ve.Param, "--view") | ||
| } | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Add typed Problem assertions for this error-path test.
At Line 637, this test only checks *errs.ValidationError and Param. Per test guidelines, also assert typed metadata from errs.ProblemOf(err) (at least Category/Subtype), while keeping Param assertion via errors.As since ProblemOf does not expose Param.
Suggested assertion update
err := VCMeetingEvents.Validate(context.Background(), runtime)
if err == nil {
t.Fatal("expected validation error for raw pretty")
}
+prob, ok := errs.ProblemOf(err)
+if !ok {
+ t.Fatalf("expected typed problem, got %T: %v", err, err)
+}
+if prob.Category != errs.CategoryValidation {
+ t.Errorf("Category = %q, want %q", prob.Category, errs.CategoryValidation)
+}
+if prob.Subtype != errs.SubtypeInvalidArgument {
+ t.Errorf("Subtype = %q, want %q", prob.Subtype, errs.SubtypeInvalidArgument)
+}
var ve *errs.ValidationError
if !errors.As(err, &ve) {
t.Fatalf("expected *errs.ValidationError, got %T: %v", err, err)
}
if ve.Param != "--view" {
t.Errorf("Param = %q, want %q", ve.Param, "--view")
}As per coding guidelines, error-path tests should assert typed metadata via errs.ProblemOf, and based on learnings Param should be checked via errors.As(*errs.ValidationError).
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| err := VCMeetingEvents.Validate(context.Background(), runtime) | |
| if err == nil { | |
| t.Fatal("expected validation error for raw pretty") | |
| } | |
| var ve *errs.ValidationError | |
| if !errors.As(err, &ve) { | |
| t.Fatalf("expected *errs.ValidationError, got %T: %v", err, err) | |
| } | |
| if ve.Param != "--view" { | |
| t.Errorf("Param = %q, want %q", ve.Param, "--view") | |
| } | |
| } | |
| err := VCMeetingEvents.Validate(context.Background(), runtime) | |
| if err == nil { | |
| t.Fatal("expected validation error for raw pretty") | |
| } | |
| prob, ok := errs.ProblemOf(err) | |
| if !ok { | |
| t.Fatalf("expected typed problem, got %T: %v", err, err) | |
| } | |
| if prob.Category != errs.CategoryValidation { | |
| t.Errorf("Category = %q, want %q", prob.Category, errs.CategoryValidation) | |
| } | |
| if prob.Subtype != errs.SubtypeInvalidArgument { | |
| t.Errorf("Subtype = %q, want %q", prob.Subtype, errs.SubtypeInvalidArgument) | |
| } | |
| var ve *errs.ValidationError | |
| if !errors.As(err, &ve) { | |
| t.Fatalf("expected *errs.ValidationError, got %T: %v", err, err) | |
| } | |
| if ve.Param != "--view" { | |
| t.Errorf("Param = %q, want %q", ve.Param, "--view") | |
| } | |
| } |
🤖 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/vc/vc_meeting_events_test.go` around lines 637 - 648, The test for
VCMeetingEvents.Validate at line 637 needs to add typed Problem metadata
assertions per test guidelines. After the existing Param field assertion using
errors.As for *errs.ValidationError, also call errs.ProblemOf(err) on the
validation error and assert the typed metadata fields (at least Category and
Subtype) to ensure comprehensive error-path testing while maintaining the Param
check via errors.As since ProblemOf does not expose it.
Sources: Coding guidelines, Learnings
Comment on lines
121
to
124
| botInfo, err := runtime.BotInfo() | ||
| if err != nil { | ||
| return errs.NewValidationError(errs.SubtypeInvalidArgument, "fetch bot identity for compact meeting-events output: %v", err).WithParam("--as") | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Locate BotInfo() and its error origin
rg -nP --type=go -C6 'func \([^)]*\) BotInfo\(\)' shortcuts/common/
rg -nP --type=go -C4 'botInfoFunc\s*=' shortcuts/common/Repository: larksuite/cli
Length of output: 3083
🏁 Script executed:
# Find and examine the fetchBotInfo function to see what error it returns
rg -nP --type=go -A15 'func \([^)]*\) fetchBotInfo\(\)' shortcuts/common/Repository: larksuite/cli
Length of output: 1281
🏁 Script executed:
# Also check the actual code at the target file and lines
sed -n '115,130p' shortcuts/vc/vc_meeting_events.goRepository: larksuite/cli
Length of output: 789
Wrap bot-identity fetch failure as a network error, not a validation error.
runtime.BotInfo() performs a network call to /bot/v3/info and returns raw errors (from fetchBotInfo(), which uses bare fmt.Errorf). Wrapping any failure as errs.NewValidationError(errs.SubtypeInvalidArgument, ...) mislabels transport/auth failures as argument problems. Three issues:
- A raw lower-layer error gets incorrectly classified as a validation error instead of a network failure.
%vflattens the error, so.WithCauseis missing anderrors.Is/errors.Unwrapno longer work..WithParam("--as")names a flag that didn't fail; the user's--as botinput was valid — the remote fetch failed.
Fix by wrapping as a network error and preserving the underlying cause:
botInfo, err := runtime.BotInfo()
if err != nil {
- return errs.NewValidationError(errs.SubtypeInvalidArgument, "fetch bot identity for compact meeting-events output: %v", err).WithParam("--as")
+ return errs.NewNetworkError(errs.SubtypeNetworkTransport, "fetch bot identity for compact meeting-events output").
+ WithCause(err).
+ WithHint("retry, or verify the bot token/permissions for /bot/v3/info")
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| botInfo, err := runtime.BotInfo() | |
| if err != nil { | |
| return errs.NewValidationError(errs.SubtypeInvalidArgument, "fetch bot identity for compact meeting-events output: %v", err).WithParam("--as") | |
| } | |
| botInfo, err := runtime.BotInfo() | |
| if err != nil { | |
| return errs.NewNetworkError(errs.SubtypeNetworkTransport, "fetch bot identity for compact meeting-events output"). | |
| WithCause(err). | |
| WithHint("retry, or verify the bot token/permissions for /bot/v3/info") | |
| } |
🤖 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/vc/vc_meeting_events.go` around lines 121 - 124, The error handling
in the botInfo declaration is incorrectly wrapping the network failure from
runtime.BotInfo() as a validation error using errs.NewValidationError with
errs.SubtypeInvalidArgument. Replace this with the appropriate network error
type, use .WithCause(err) to preserve the full error chain instead of flattening
it with %v formatting, and remove the .WithParam("--as") call since the flag
itself was valid and the actual failure was the remote fetch.
Source: Coding guidelines
Source-Branch: features/F-vc-meeting-events-contract Source-Commit: 1cdb506 Source-Subject: fix: normalize vc meeting events contract Repo: larksuite-cli Synced-By: bytedance Timestamp: 20260623_091211Z
Source-Branch: features/F-vc-meeting-events-contract Source-Commit: f57b62c Source-Subject: fix: stabilize meeting events validation examples Repo: larksuite-cli Synced-By: bytedance Timestamp: 20260623_091922Z
Source-Branch: features/F-vc-meeting-events-contract Source-Commit: d29e99a Source-Subject: fix: keep meeting events examples deterministic Repo: larksuite-cli Synced-By: bytedance Timestamp: 20260623_092620Z
Source-Branch: features/F-vc-meeting-events-contract Source-Commit: 79cbc83 Source-Subject: fix: align meeting events pagination test Repo: larksuite-cli Synced-By: bytedance Timestamp: 20260623_093249Z
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1544 +/- ##
==========================================
+ Coverage 73.97% 74.08% +0.11%
==========================================
Files 787 787
Lines 76293 76591 +298
==========================================
+ Hits 56436 56741 +305
+ Misses 15625 15582 -43
- Partials 4232 4268 +36 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
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
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
vc +meeting-eventsbot-facing compact output with bot identity, self markers, server current roster, normalized time/status fields, pretty labels, and raw escape hatchValidation
go build ./shortcuts/vcvia controlled compile runnerfeatures/F-vc-meeting-events-contractGate4 acceptance will continue with MR checks, remote UT/semantic, FT, and QA lanes under the same verification identity.
Summary by CodeRabbit
Release Notes
New Features
+meeting-events --viewwithcompact(default) andrawoutput modes.compactoutput now includes normalizedmeeting, bot identity,current_roster, and event fields.Changed Behavior
+meeting-eventsnow supports bot authentication only (--as bot).--view rawis available only with--format json, preserving the legacy event envelope (without bot/roster metadata).Bug Fixes / Improvements
Documentation