fix/task id handling#2023
Conversation
github-actions
Bot
added
domain/task
📝 WalkthroughWalkthroughTask shortcuts now accept GUIDs or task applinks containing ChangesTask identifier workflows
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant CLI
participant TaskIDParser
participant TaskShortcut
participant TaskAPI
CLI->>TaskIDParser: validate --task-id
TaskIDParser-->>TaskShortcut: normalized task GUID(s)
TaskShortcut->>TaskAPI: GET or PATCH task
TaskAPI-->>TaskShortcut: task state and confirmed fields
TaskShortcut-->>CLI: structured completion or update output
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ 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 |
![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
🧹 Nitpick comments (3)
shortcuts/task/task_update.go (1)
58-84: 🗄️ Data Integrity & Integration | 🔵 Trivial | 🏗️ Heavy liftPartial writes on mid-batch failure aren't surfaced.
If a task later in
taskIDsfails PATCH, earlier tasks in the same call have already been mutated, butExecutereturns only the error with no indication of which tasks already succeeded. This loop body predates this PR, but it's directly relevant to this PR's "server-confirmed mutation state" objective — worth considering returning partial results (e.g. which task GUIDs succeeded before the failure) rather than discarding them.🤖 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/task/task_update.go` around lines 58 - 84, The Execute loop should surface partial success when a later task PATCH fails instead of returning only the error. Track the task GUIDs successfully mutated through callTaskAPITyped, and include those results or identifiers with the failure while preserving the existing immediate error behavior for failures before any successful mutation.shortcuts/task/task_complete.go (1)
70-94: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDuplicate "is completed" check.
The
completedAtStr != "" && completedAtStr != "0"truthy check is duplicated foralreadyCompleted(line 72) andstatus(lines 92-94). Consider extracting a small helper to avoid the two independent copies drifting.♻️ Optional helper extraction
+func isTaskCompleted(completedAt string) bool { + return completedAt != "" && completedAt != "0" +} + ... - alreadyCompleted := completedAtStr != "" && completedAtStr != "0" + alreadyCompleted := isTaskCompleted(completedAtStr) ... - status := "todo" - if completedAt != "" && completedAt != "0" { - status = "done" - } + status := "todo" + if isTaskCompleted(completedAt) { + status = "done" + }🤖 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/task/task_complete.go` around lines 70 - 94, Extract the shared completed-at truth check used by the alreadyCompleted assignment and the status calculation into a small helper, then reuse it in both locations. Keep the existing behavior for empty and "0" values while eliminating the duplicated condition.shortcuts/task/shortcuts.go (1)
104-136: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winPreserve the URL-parse error as a cause.
The URL-parse failure branch currently formats the lower-layer error into the message string. Attach it with
.WithCause(err)so callers can retrieve the underlying cause viaerrors.Unwrap/errors.Is, while keeping the--task-idmetadata and hint intact.♻️ Proposed fix
u, err := url.Parse(input) if err != nil { - return "", invalid("invalid task applink: %v", err) + return "", invalid("invalid task applink: %v", err).WithCause(err) }🤖 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/task/shortcuts.go` around lines 104 - 136, Update the URL-parse error branch in parseTaskGUID to attach the original err via WithCause instead of only interpolating it into the validation message. Preserve the existing --task-id parameter metadata and hint, along with the current invalid applink message.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.
Inline comments:
In `@shortcuts/task/task_update.go`:
- Around line 42-56: Update the DryRun handler to construct a preview containing
one PATCH request for every task ID returned by parseTaskGUIDs, matching the
multi-target behavior of Execute. Preserve the existing request path,
parameters, and body for each escaped ID, while retaining the current error
responses for body-building and ID-parsing failures.
In `@tests/cli_e2e/task/task_id_handling_dryrun_test.go`:
- Around line 16-81: Add a live E2E test alongside TestTaskIDHandlingDryRun
covering task +update and/or +complete with an applink GUID, verifying
successful execution, and display-number input such as t12345, verifying
validation failure and no unintended update. Reuse the existing task CLI setup
and result assertions, while keeping the dry-run equivalence and rejection
coverage unchanged.
---
Nitpick comments:
In `@shortcuts/task/shortcuts.go`:
- Around line 104-136: Update the URL-parse error branch in parseTaskGUID to
attach the original err via WithCause instead of only interpolating it into the
validation message. Preserve the existing --task-id parameter metadata and hint,
along with the current invalid applink message.
In `@shortcuts/task/task_complete.go`:
- Around line 70-94: Extract the shared completed-at truth check used by the
alreadyCompleted assignment and the status calculation into a small helper, then
reuse it in both locations. Keep the existing behavior for empty and "0" values
while eliminating the duplicated condition.
In `@shortcuts/task/task_update.go`:
- Around line 58-84: The Execute loop should surface partial success when a
later task PATCH fails instead of returning only the error. Track the task GUIDs
successfully mutated through callTaskAPITyped, and include those results or
identifiers with the failure while preserving the existing immediate error
behavior for failures before any successful mutation.
🪄 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 Plus
Run ID: d112e359-5cdc-42d4-ae18-f84fcec6ed46
📒 Files selected for processing (10)
shortcuts/task/shortcuts.goshortcuts/task/shortcuts_test.goshortcuts/task/task_complete.goshortcuts/task/task_complete_test.goshortcuts/task/task_update.goshortcuts/task/task_update_test.goskills/lark-task/SKILL.mdskills/lark-task/references/lark-task-complete.mdskills/lark-task/references/lark-task-update.mdtests/cli_e2e/task/task_id_handling_dryrun_test.go
🚀 PR Preview Install Guide🧰 CLI updatenpm i -g https://pkg.pr.new/larksuite/cli/@larksuite/cli@b32c0f15d9f41c433a74301014f8045027542020🧩 Skill updatenpx skills add ILUO/cli#fix/task-id-handling -y -g |
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
shortcuts/task/task_update.go (3)
133-152: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winReject empty entries instead of silently discarding them.
Inputs such as
guid-1,,guid-2orguid-1,currently update only the non-empty IDs. Validate every split token withparseTaskGUID(which already produces a typed validation error for empty input) so the requested batch is preserved exactly.As per coding guidelines, do not silently discard unsupported input or writes; return a typed validation error instead.
🤖 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/task/task_update.go` around lines 133 - 152, Update parseTaskGUIDs to validate every token from strings.Split through parseTaskGUID, including whitespace-only and trailing empty entries, instead of skipping them. Return parseTaskGUID’s typed validation error for any empty token while preserving the existing successful GUID collection behavior.Source: Coding guidelines
133-152: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winApply display-number rejection to applink query values.
parseTaskGUIDsdelegates toparseTaskGUID, but the supplied shared parser returns an applink’s extractedguidwithout applying the display-number check. Therefore an applink containingguid=123bypasses the intended rejection and reaches PATCH as an invalid task identifier. Revalidate the extracted GUID inparseTaskGUIDand add an applink regression test.🤖 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/task/task_update.go` around lines 133 - 152, Update parseTaskGUID to revalidate the GUID extracted from applink query values using the existing display-number rejection, ensuring values such as guid=123 fail before PATCH processing. Add a regression test covering an applink with a numeric display GUID and verify parseTaskGUIDs rejects it.
88-109: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winDo not report a successful batch when a task response is missing.
The preceding unchecked type assertion only appends non-nil task objects. A successful PATCH response without
data.taskcan therefore disappear whileExecutestill returns nil, leavingdata.tasksandMeta.Countsmaller than the requested batch. Parse the API response into a typed result and return a typed error when the task result or identity is missing instead of silently dropping it.As per coding guidelines, parse API-bound
map[string]interface{}data into typed structs and use typed command-facing errors.🤖 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/task/task_update.go` around lines 88 - 109, The task update flow around the unchecked task extraction must validate every requested PATCH result instead of silently skipping missing entries. Parse the API response maps into typed result structs, require each result to contain a task and its identity, and return a typed command-facing error when either is absent; only build `tasks` and return success after all results pass validation.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 `@shortcuts/task/task_update.go`:
- Around line 133-152: Update parseTaskGUIDs to validate every token from
strings.Split through parseTaskGUID, including whitespace-only and trailing
empty entries, instead of skipping them. Return parseTaskGUID’s typed validation
error for any empty token while preserving the existing successful GUID
collection behavior.
- Around line 133-152: Update parseTaskGUID to revalidate the GUID extracted
from applink query values using the existing display-number rejection, ensuring
values such as guid=123 fail before PATCH processing. Add a regression test
covering an applink with a numeric display GUID and verify parseTaskGUIDs
rejects it.
- Around line 88-109: The task update flow around the unchecked task extraction
must validate every requested PATCH result instead of silently skipping missing
entries. Parse the API response maps into typed result structs, require each
result to contain a task and its identity, and return a typed command-facing
error when either is absent; only build `tasks` and return success after all
results pass validation.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: f8845e79-5347-4eca-966c-66df996319a7
📒 Files selected for processing (6)
shortcuts/task/shortcuts.goshortcuts/task/shortcuts_test.goshortcuts/task/task_update.goshortcuts/task/task_update_test.gotests/cli_e2e/task/task_id_handling_dryrun_test.gotests/cli_e2e/task/task_id_handling_workflow_test.go
🚧 Files skipped from review as they are similar to previous changes (4)
- tests/cli_e2e/task/task_id_handling_dryrun_test.go
- shortcuts/task/shortcuts.go
- shortcuts/task/shortcuts_test.go
- shortcuts/task/task_update_test.go
3 participants
Summary
Improve the existing Task
+updateand+completeshortcuts without adding a new shortcut. The change distinguishes Task OpenAPI GUIDs from client display IDs, accepts Task applinks directly, and returns server-confirmed mutation state to reduce redundant follow-up reads.Changes
guidfrom Task applinks.t104121with a typedinvalid_argumenterror.+updateoutput with:updated_fieldsconfirmedvalues taken only from the server response.+completeand extend its output with:statuscompleted_atalready_completedtask tasklists taskscommand instead of adding a new shortcut.tasks getcalls when mutation output already confirms the required state.Test Plan
go test -race -count=1 ./shortcuts/taskgo test ./tests/cli_e2e/task -run TestTaskIDHandlingDryRun -count=1go vet ./shortcuts/task ./tests/cli_e2e/taskRelated Issues
Summary by CodeRabbit
--task-id, extracting the GUID fromguid=when provided.status,completed_at, andalready_completed.updated_fieldsand per-task server-confirmed values.--task-idvalues are validated up front to prevent partial updates.--task-idand the expanded result fields.