feat: support speaker list and nolark speaker replace - #1594
Merged
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (8)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (5)
📝 WalkthroughWalkthrough
ChangesMinutes speaker replacement
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 markdownlint-cli2 (0.22.1)skills/lark-minutes/SKILL.mdmarkdownlint-cli2 wrapper config was not available before execution skills/lark-minutes/references/lark-minutes-speaker-replace.mdmarkdownlint-cli2 wrapper config was not available before execution 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 |
github-actions
Bot
added
the
size/L
🚀 PR Preview Install Guide🧰 CLI updatenpm i -g https://pkg.pr.new/larksuite/cli/@larksuite/cli@0c906f958fc98049071ebbe51a1e8735c3f4e90a🧩 Skill updatenpx skills add larksuite/cli#feat/speaker_support_nolark -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: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
skills/lark-minutes/SKILL.md (1)
220-220: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick winStale description contradicts new speaker-id workflow.
Line 220 still reads "了解参数和限制（仅支持用户 ID，不支持姓名）" which is outdated. The new workflow uses
--from-speaker-idwithspeaker_id(not user ID), and the restriction is about display names vs. speaker IDs, not user IDs vs. names. Update to reflect the current parameter model.🤖 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-minutes/SKILL.md` at line 220, Update the `+speaker-replace` documentation in `SKILL.md` to match the current speaker-id workflow. The stale note about “仅支持用户 ID，不支持姓名” is outdated; revise it to reference the `--from-speaker-id` / `speaker_id` model and clarify the actual restriction is about display names versus speaker IDs. Keep the instruction aligned with the existing `+speaker-replace` guidance and `references/lark-minutes-speaker-replace.md`.
🧹 Nitpick comments (1)
skills/lark-minutes/references/lark-minutes-speaker-replace.md (1)
92-103: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider documenting the conditional
from_speaker_inputoutput field.When
--from-speaker-idis a display name that gets resolved to aspeaker_id, the output also includesfrom_speaker_inputshowing the original input. This is helpful for debugging resolution. Consider adding it to the output table with a note that it appears only when resolution occurred.🤖 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-minutes/references/lark-minutes-speaker-replace.md` around lines 92 - 103, Add the conditional `from_speaker_input` field to the output table in the `lark-minutes-speaker-replace` reference, alongside `minute_token`, `from_speaker_id`, and `to_user_id`. Document that `from_speaker_input` preserves the original `--from-speaker-id` value and is only present when the input was resolved from a display name to a `speaker_id`, so readers can understand when and why it appears.
🤖 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/minutes/minutes_speaker_replace_test.go`:
- Around line 35-38: The missing-from-speaker test expectation is stale because
MinutesSpeakerReplace.Validate now returns a typed validation error instead of
the old “required flag(s) ...” text. Update the test in MinutesSpeakerReplace
test cases to assert the typed error via errors.As into the validation error
type and verify the Param field is "--from-speaker-id", rather than matching the
malformed substring. Use the existing MinutesSpeakerReplace.Validate path and
keep the assertion on the error metadata, since errs.ProblemOf does not expose
Param.
In `@shortcuts/minutes/minutes_speaker_replace.go`:
- Around line 33-34: The flag help for minutes speaker replacement is out of
sync with the actual lookup behavior: `from-speaker-id` currently resolves both
opaque speaker IDs and display names through the speaker list, so the
description should be updated to match what `Run` and the new
resolution/not-found path actually accept. Change the help text in
`minutes_speaker_replace.go` so `--from-speaker-id` is described as preferring
an opaque `speaker_id` but also allowing a display name to be resolved, and keep
`from-user-id` clearly marked as deprecated.
In `@shortcuts/minutes/minutes_speakers_test.go`:
- Around line 26-44: The speaker-resolution test is missing an assertion for the
typed validation metadata on both error paths. Update minutes_speakers_test.go
in the resolveSpeakerIDByName test cases to verify the unwrapped
*errs.ValidationError from errors.As has Param set to "--from-speaker-id" for
both the not-found and duplicate-name branches, alongside the existing subtype
and hint checks.
In `@shortcuts/minutes/minutes_speakers.go`:
- Around line 96-103: `resolveSpeakerReplaceFrom()` is still prioritizing the
deprecated `from-user-id` flag over `from-speaker-id`, which makes `Execute`
diverge from `Validate` and `buildSpeakerReplaceRequestBodyResolved()`. Update
the fallback logic so the new `from-speaker-id` takes precedence when both are
set, and only fall back to `from-user-id` when the new flag is absent. Keep the
behavior consistent with the speaker replacement flow in `minutes_speakers.go`
and the request-building code in `minutes_speaker_replace.go`.
---
Outside diff comments:
In `@skills/lark-minutes/SKILL.md`:
- Line 220: Update the `+speaker-replace` documentation in `SKILL.md` to match
the current speaker-id workflow. The stale note about “仅支持用户 ID，不支持姓名” is
outdated; revise it to reference the `--from-speaker-id` / `speaker_id` model
and clarify the actual restriction is about display names versus speaker IDs.
Keep the instruction aligned with the existing `+speaker-replace` guidance and
`references/lark-minutes-speaker-replace.md`.
---
Nitpick comments:
In `@skills/lark-minutes/references/lark-minutes-speaker-replace.md`:
- Around line 92-103: Add the conditional `from_speaker_input` field to the
output table in the `lark-minutes-speaker-replace` reference, alongside
`minute_token`, `from_speaker_id`, and `to_user_id`. Document that
`from_speaker_input` preserves the original `--from-speaker-id` value and is
only present when the input was resolved from a display name to a `speaker_id`,
so readers can understand when and why it appears.
🪄 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: 7cc25fea-8c77-41b5-8564-2a922bad83c1
📒 Files selected for processing (7)
shortcuts/minutes/minutes_speaker_replace.goshortcuts/minutes/minutes_speaker_replace_test.goshortcuts/minutes/minutes_speakers.goshortcuts/minutes/minutes_speakers_test.goskills/lark-minutes/SKILL.mdskills/lark-minutes/references/lark-minutes-speaker-replace.mdtests/cli_e2e/minutes/minutes_speaker_replace_test.go
zhangjun-bytedance
force-pushed
the
feat/speaker_support_nolark
branch
from
June 26, 2026 03:10
7122a76 to
ea316b5
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
shortcuts/minutes/minutes_speaker_replace.go (1)
45-64: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winReject ambiguous source flags before executing the write.
When both
--from-speaker-idand deprecated--from-user-idare provided, validation passes and later code silently prefersfrom_speaker_id. For a transcript-mutating command, require exactly one source identifier so users do not replace the wrong speaker by accident.Proposed fix
fromSpeakerID := strings.TrimSpace(runtime.Str("from-speaker-id")) fromUserID := strings.TrimSpace(runtime.Str("from-user-id")) if fromSpeakerID == "" && fromUserID == "" { return errs.NewValidationError(errs.SubtypeInvalidArgument, "--from-speaker-id is required").WithParam("--from-speaker-id") } + if fromSpeakerID != "" && fromUserID != "" { + return errs.NewValidationError(errs.SubtypeInvalidArgument, "--from-speaker-id and --from-user-id cannot be used together"). + WithParam("--from-user-id"). + WithHint("Remove deprecated --from-user-id when --from-speaker-id is provided.") + }🤖 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/minutes/minutes_speaker_replace.go` around lines 45 - 64, The MinutesSpeakerReplace command currently accepts both source flags and then silently prefers fromSpeakerID over fromUserID, which can cause the wrong speaker to be replaced. Update the validation in the command flow that reads runtime.Str for --from-speaker-id and --from-user-id so exactly one source identifier is allowed before any write path runs, and return a validation error when both are set or when neither is set. Keep the existing common.ValidateUserIDTyped checks in the same MinutesSpeakerReplace logic, but apply them only after the source flag ambiguity is rejected.
🤖 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/minutes/minutes_speaker_replace.go`:
- Around line 67-75: In minutes_speaker_replace.go, the DryRun path in the
minute speaker replace command is leaking the raw display name into the planned
PUT body via buildSpeakerReplaceRequestBody, while Execute resolves it first.
Update the DryRunAPI flow so the resolved-id case is represented with a
placeholder or resolved-id value instead of the original display name, keeping
the GET lookup from minuteTranscriptSpeakerlistPath as the source of resolution
and ensuring the PUT preview matches what Execute will actually send.
---
Outside diff comments:
In `@shortcuts/minutes/minutes_speaker_replace.go`:
- Around line 45-64: The MinutesSpeakerReplace command currently accepts both
source flags and then silently prefers fromSpeakerID over fromUserID, which can
cause the wrong speaker to be replaced. Update the validation in the command
flow that reads runtime.Str for --from-speaker-id and --from-user-id so exactly
one source identifier is allowed before any write path runs, and return a
validation error when both are set or when neither is set. Keep the existing
common.ValidateUserIDTyped checks in the same MinutesSpeakerReplace logic, but
apply them only after the source flag ambiguity is rejected.
🪄 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: c07f6df9-1870-48d2-bd85-8dd9ba8a5334
📒 Files selected for processing (7)
shortcuts/minutes/minutes_speaker_replace.goshortcuts/minutes/minutes_speaker_replace_test.goshortcuts/minutes/minutes_speakers.goshortcuts/minutes/minutes_speakers_test.goskills/lark-minutes/SKILL.mdskills/lark-minutes/references/lark-minutes-speaker-replace.mdtests/cli_e2e/minutes/minutes_speaker_replace_test.go
🚧 Files skipped from review as they are similar to previous changes (5)
- shortcuts/minutes/minutes_speakers_test.go
- shortcuts/minutes/minutes_speakers.go
- tests/cli_e2e/minutes/minutes_speaker_replace_test.go
- skills/lark-minutes/SKILL.md
- shortcuts/minutes/minutes_speaker_replace_test.go
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1594 +/- ##
=======================================
Coverage 74.74% 74.75%
=======================================
Files 799 800 +1
Lines 80380 80459 +79
=======================================
+ Hits 60084 60147 +63
- Misses 15849 15857 +8
- Partials 4447 4455 +8 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
zhangjun-bytedance
force-pushed
the
feat/speaker_support_nolark
branch
from
June 26, 2026 03:26
ea316b5 to
0c906f9
Compare
calendar-assistant
approved these changes
Jun 26, 2026
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
2 participants
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
Changes
Test Plan
lark-cli <domain> <command>flow works as expectedRelated Issues
Summary by CodeRabbit
New Features
minutes +speaker-replacenow uses--from-speaker-idas the primary source-speaker input, including speakerlist-based name resolution.from_speaker_inputwhen applicable).Bug Fixes
--from-speaker-id.Documentation
--from-speaker-id.Tests
--from-speaker-idresolution and output.