Open
Conversation
|
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:
📝 WalkthroughWalkthroughAdds OKR progress-record support and image upload: new CLI shortcuts (+progress-list/get/create/update/delete, +upload-image), new OpenAPI v1/v2 progress/content types and converters, response DTOs and status enum, extensive unit and e2e tests, README/skill documentation updates. (50 words) Changes
Sequence Diagram(s)sequenceDiagram
participant CLI as CLI (lark-cli)
participant Runtime as runtime.DoAPI / HTTP client
participant OKRAPI as OKR API (/open-apis/okr/v1 or v2)
participant Storage as FileService (multipart)
CLI->>Runtime: Validate flags → build JSON body or multipart
alt Dry-run
Runtime-->>CLI: Emit constructed request description
else Execute
Runtime->>OKRAPI: POST/PUT/GET/DELETE (JSON) or POST /images/upload (multipart)
Note right of Runtime: multipart includes file bytes + fields
OKRAPI->>Storage: (if image) accept file, return file_token,url
OKRAPI-->>Runtime: JSON response (code, data)
Runtime-->>CLI: Parse → RespProgress / envelope → print
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 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 |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #574 +/- ##
==========================================
+ Coverage 60.35% 61.41% +1.06%
==========================================
Files 393 407 +14
Lines 33588 34824 +1236
==========================================
+ Hits 20272 21388 +1116
- Misses 11430 11459 +29
- Partials 1886 1977 +91 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
🚀 PR Preview Install Guide🧰 CLI updatenpm i -g https://pkg.pr.new/larksuite/cli/@larksuite/cli@dc49fef43e4ddfd778e12d75f7e44251ed0861cd🧩 Skill updatenpx skills add larksuite/cli#feat/okr_dev -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: 11
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
README.zh.md (1)
9-16:⚠️ Potential issue | 🟡 MinorKeep the Skill count consistent.
Lines 9 and 15 still say
22, while Line 16 now says23.📝 Proposed doc fix
-飞书官方 CLI 工具，由 [larksuite](https://github.com/larksuite) 团队维护 — 让人类和 AI Agent 都能在终端中操作飞书。覆盖消息、文档、多维表格、电子表格、幻灯片、日历、邮箱、任务、会议等核心业务域，提供 200+ 命令及 22 个 AI Agent [Skills](./skills/)。 +飞书官方 CLI 工具，由 [larksuite](https://github.com/larksuite) 团队维护 — 让人类和 AI Agent 都能在终端中操作飞书。覆盖消息、文档、多维表格、电子表格、幻灯片、日历、邮箱、任务、会议等核心业务域，提供 200+ 命令及 23 个 AI Agent [Skills](./skills/)。 @@ -- **为 Agent 原生设计** — 22 个 [Skills](./skills/) 开箱即用，适配主流 AI 工具，Agent 无需额外适配即可操作飞书 +- **为 Agent 原生设计** — 23 个 [Skills](./skills/) 开箱即用，适配主流 AI 工具，Agent 无需额外适配即可操作飞书🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@README.zh.md` around lines 9 - 16, The README contains inconsistent Skill counts: update the occurrences of "22 个 AI Agent [Skills](./skills/)" and "23 个 AI Agent [Skills](./skills/)" so they match (choose the correct canonical number and replace all instances), specifically edit the two places showing "22 个 AI Agent [Skills](./skills/)" and the place showing "23 个 AI Agent [Skills](./skills/)" to use the same number and keep the link text "[Skills](./skills/)" unchanged.
🧹 Nitpick comments (4)
skills/lark-okr/references/lark-okr-image-upload.md (1)
37-37: Optional wording polish:图片内容can be made more precise.
Consider “图片块” to match ContentGallery terminology and reduce ambiguity.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@skills/lark-okr/references/lark-okr-image-upload.md` at line 37, Replace the ambiguous phrase "图片内容" with the more precise term "图片块" in the sentence that reads "获取返回的 `file_token`，用于构建 ContentBlock 中的图片内容。", so that it aligns with ContentGallery terminology and reduces ambiguity when referring to image elements inside ContentBlock; ensure the updated sentence reads something like "获取返回的 `file_token`，用于构建 ContentBlock 中的图片块。".shortcuts/okr/okr_openapi.go (1)
450-457: Slice aliasing between V1 and V2 gallery images.
ContentGallery.ToV1reuses the same backing slice (ImageList: g.Images) — and the inverseContentGalleryV1.ToV2also reuses the slice (line 569). If either side is later mutated after conversion, the other is silently affected. Not an issue under current request/response-only flows, but worth documenting or defensively copying if these conversions ever get used in a mutation path.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@shortcuts/okr/okr_openapi.go` around lines 450 - 457, ContentGallery.ToV1 currently assigns the Images slice directly to ImageList (ImageList: g.Images), creating shared backing arrays with ContentGalleryV1.ToV2 and risking silent mutation; update ToV1 (and verify ToV2) to return a shallow copy of the slice (allocate a new slice and copy elements) so the returned ContentGalleryV1.ImageList does not share backing storage with the original ContentGallery.Images, referencing the functions ContentGallery.ToV1 and ContentGalleryV1.ToV2 to find the conversion points.shortcuts/okr/okr_progress_update.go (1)
114-131:DryRunsilently swallowsparseUpdateProgressRecordParamserror.
p, _ := parseUpdateProgressRecordParams(runtime)drops the error. Today this is safe only becauseValidateis guaranteed to run first and rejects invalid--contentJSON; however,parseUpdateProgressRecordParamsalso handles--progress-percent/--progress-status, whichValidatevalidates redundantly. If either side drifts, a returned-nilpwould NPE onp.UserIDType/p.ProgressIDinsideDryRun. Either (a) collapse the duplicated validation and trust the parse path, or (b) at minimum guard againstp == nilbefore dereferencing.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@shortcuts/okr/okr_progress_update.go` around lines 114 - 131, The DryRun implementation for DryRun in okr_progress_update.go currently ignores errors from parseUpdateProgressRecordParams by using p, _ := parseUpdateProgressRecordParams(runtime), which can yield a nil p and cause NPEs when accessing p.UserIDType or p.ProgressID; update DryRun to check and handle the returned error (or nil p) from parseUpdateProgressRecordParams: call p, err := parseUpdateProgressRecordParams(runtime), if err or p == nil then return a sensible empty/failed DryRun API (or propagate the error) instead of dereferencing p; alternatively collapse duplicated validation by moving validation into parseUpdateProgressRecordParams and using its result in both Validate and DryRun so DryRun can safely rely on a non-nil p.shortcuts/okr/okr_image_upload.go (1)
77-84: DryRun body representstarget_typeas int while Execute sends it as a string.In
DryRunthe body shows"target_type": targetTypeVal(numeric), butExecute(line 108) submitstarget_typeas a decimal string via multipart (fmt.Sprintf("%d", targetTypeVal)). Users relying on--dry-runto predict the exact wire payload will see a different JSON shape than what is actually posted. Consider stringifying inDryRunas well for consistency with multipart semantics.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@shortcuts/okr/okr_image_upload.go` around lines 77 - 84, The DryRun payload represents target_type as a numeric value while Execute sends it as a decimal string; update the DryRun builder (the block that returns common.NewDryRunAPI().POST(...).Body(...)) to stringify targetTypeVal (use the same fmt.Sprintf("%d", targetTypeVal) format used in Execute) so the DryRun body and the multipart Execute payload match; keep target_id and file handling identical and only change target_type to a string to ensure consistency between DryRun and Execute.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@internal/registry/service_descriptions.json`:
- Around line 68-69: The EN and ZH entries for the "OKR" service in
service_descriptions.json are semantically mismatched: the "zh" description
mentions progress records while "en" does not; update the "en" description for
the "OKR" entry (the object under key "en" alongside "zh") to include the same
capabilities (e.g., add "progress records" or equivalent phrase) so both locales
describe the same features, or alternatively remove "progress records" from the
"zh" description to keep them aligned—edit the "en" description string to mirror
the ZH items (object title "OKR", description field).
In `@README.md`:
- Line 16: Update the inconsistent skill count in the README by changing the
intro sentence that currently states "22 AI Agent Skills" to match the later
line "23 AI Agent [Skills]"; locate the intro paragraph and replace the numeric
value 22 with 23 so both the intro and the "Wide Coverage — 15 business domains,
200+ curated commands, 23 AI Agent [Skills]" line are consistent.
- Line 158: Update the `lark-okr` table row so its description mirrors the
Features section by mentioning progress management; change the text for
`lark-okr` (the table entry string `lark-okr`) to read something like "Query,
create, update OKRs; manage objectives & key results, alignments, indicators,
and progress" so the skill row explicitly includes progress management alongside
objectives, key results, alignments, and indicators.
In `@README.zh.md`:
- Line 159: Update the `lark-okr` skill table row to mention OKR progress
records by adding "OKR 进展记录" (or simply "进展记录") into the description cell
alongside the existing items so the row reads e.g. "查询、创建、更新
OKR，管理目标、关键结果、对齐、指标和进展记录", editing the table row that contains the `lark-okr`
token to keep wording consistent with the feature list.
In `@shortcuts/okr/okr_image_upload.go`:
- Around line 22-55: The Validate function for OKRUploadImage uses
filepath.Ext(filePath) and checks against allowedImageExts but doesn't normalize
case, so extensions like "PNG" are rejected; update the validation in Validate
to normalize the extracted extension (e.g., ext :=
strings.ToLower(strings.TrimSpace(filepath.Ext(filePath)))) before checking
allowedImageExts so lookups use the lowercase keys defined in allowedImageExts;
ensure you import strings if not already present and keep the same error message
and runtime.Str usage.
In `@shortcuts/okr/okr_progress_create.go`:
- Around line 219-228: The CLI output uses the internal type name "ProgressV1"
in the user-facing messages; update the strings in the runtime.OutFormat closure
(the block referencing resp.ID, resp.ModifyTime, resp.ProgressRate.Percent,
resp.Content) to use a user-friendly label such as "Progress" (e.g., change
"Created ProgressV1 Record" -> "Created Progress Record" and "ProgressV1:
%.1f%%" -> "Progress: %.1f%%") while leaving the surrounding formatting and nil
checks intact.
In `@shortcuts/okr/okr_progress_get.go`:
- Around line 78-83: The pretty printer is emitting the internal model name
"ProgressV1" to users; change the displayed labels inside the runtime.OutFormat
callback (the fmt.Fprintf calls that reference resp and print the header
"ProgressV1 Record [%s]\n") to use the public/domain terminology (e.g.,
"Progress Record" or "OKR Progress Record") and update any other user-facing
label that contains "ProgressV1" so the output uses command/domain names instead
of the internal model name; keep the rest of the logic (resp.ID,
resp.ModifyTime, ProgressRate printing) unchanged.
In `@shortcuts/okr/okr_progress_update.go`:
- Around line 164-172: Change the user-facing strings that leak the internal
type name "ProgressV1" to neutral wording in both okr_progress_update.go and
okr_progress_create.go: update the runtime.OutFormat output in the block that
writes "Updated ProgressV1 Record [...]" (function using resp.ID,
resp.ModifyTime, resp.ProgressRate.Percent, resp.Content) to something like
"Updated progress record [...]" and replace "ProgressV1: %.1f%%" with "Progress:
%.1f%%" (similarly change the "Created ProgressV1 Record" text in the create
file) so only neutral terms (e.g., "progress" or "progress record") appear to
users while keeping the same resp fields and formatting logic.
In `@skills/lark-okr/references/lark-okr-progress-create.md`:
- Around line 17-22: Update the example invocation for the lark-cli okr
+progress-create command to use a valid signed int64 for --target-id: replace
the current oversized value (9876543210987654321) with the suggested valid ID
(8876543210987654321) so the CLI validation for the --target-id parameter
passes.
In `@skills/lark-okr/references/lark-okr-progress-delete.md`:
- Around line 19-23: Replace the placeholder-style parameter cell that currently
shows `--progress-id <id>` with the plain flag name `--progress-id` to match the
rest of the OKR docs; keep the "必填" as "是" and move the type/validation detail
("进展记录 ID（int64 类型，正整数）") into the 说明 column so the table row reads
`--progress-id | 是 | — | 进展记录 ID（int64 类型，正整数）`, ensuring consistency with other
flag rows.
In `@skills/lark-okr/SKILL.md`:
- Around line 32-33: Update the "ContentBlock 富文本格式" bullet to note that
progress records also use ContentBlock and are included in the ContentBlock
summary; specifically edit the line referencing `ContentBlock` (the second
bullet) so it reads something like "ContentBlock 富文本格式 —
Objective/KeyResult/Notes 和进度记录（Progress Records）字段使用的富文本格式说明", ensuring
`ContentBlock` and "进度记录"/"Progress Records" are mentioned so the summary is not
stale.
---
Outside diff comments:
In `@README.zh.md`:
- Around line 9-16: The README contains inconsistent Skill counts: update the
occurrences of "22 个 AI Agent [Skills](./skills/)" and "23 个 AI Agent
[Skills](./skills/)" so they match (choose the correct canonical number and
replace all instances), specifically edit the two places showing "22 个 AI Agent
[Skills](./skills/)" and the place showing "23 个 AI Agent [Skills](./skills/)"
to use the same number and keep the link text "[Skills](./skills/)" unchanged.
---
Nitpick comments:
In `@shortcuts/okr/okr_image_upload.go`:
- Around line 77-84: The DryRun payload represents target_type as a numeric
value while Execute sends it as a decimal string; update the DryRun builder (the
block that returns common.NewDryRunAPI().POST(...).Body(...)) to stringify
targetTypeVal (use the same fmt.Sprintf("%d", targetTypeVal) format used in
Execute) so the DryRun body and the multipart Execute payload match; keep
target_id and file handling identical and only change target_type to a string to
ensure consistency between DryRun and Execute.
In `@shortcuts/okr/okr_openapi.go`:
- Around line 450-457: ContentGallery.ToV1 currently assigns the Images slice
directly to ImageList (ImageList: g.Images), creating shared backing arrays with
ContentGalleryV1.ToV2 and risking silent mutation; update ToV1 (and verify ToV2)
to return a shallow copy of the slice (allocate a new slice and copy elements)
so the returned ContentGalleryV1.ImageList does not share backing storage with
the original ContentGallery.Images, referencing the functions
ContentGallery.ToV1 and ContentGalleryV1.ToV2 to find the conversion points.
In `@shortcuts/okr/okr_progress_update.go`:
- Around line 114-131: The DryRun implementation for DryRun in
okr_progress_update.go currently ignores errors from
parseUpdateProgressRecordParams by using p, _ :=
parseUpdateProgressRecordParams(runtime), which can yield a nil p and cause NPEs
when accessing p.UserIDType or p.ProgressID; update DryRun to check and handle
the returned error (or nil p) from parseUpdateProgressRecordParams: call p, err
:= parseUpdateProgressRecordParams(runtime), if err or p == nil then return a
sensible empty/failed DryRun API (or propagate the error) instead of
dereferencing p; alternatively collapse duplicated validation by moving
validation into parseUpdateProgressRecordParams and using its result in both
Validate and DryRun so DryRun can safely rely on a non-nil p.
In `@skills/lark-okr/references/lark-okr-image-upload.md`:
- Line 37: Replace the ambiguous phrase "图片内容" with the more precise term "图片块"
in the sentence that reads "获取返回的 `file_token`，用于构建 ContentBlock 中的图片内容。", so
that it aligns with ContentGallery terminology and reduces ambiguity when
referring to image elements inside ContentBlock; ensure the updated sentence
reads something like "获取返回的 `file_token`，用于构建 ContentBlock 中的图片块。".
🪄 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: 1f910ed0-c9ff-4f7b-b054-94c88c3cf7b0
📒 Files selected for processing (31)
README.mdREADME.zh.mdinternal/registry/service_descriptions.jsonshortcuts/okr/okr_cli_resp.goshortcuts/okr/okr_image_upload.goshortcuts/okr/okr_image_upload_test.goshortcuts/okr/okr_openapi.goshortcuts/okr/okr_openapi_test.goshortcuts/okr/okr_progress_create.goshortcuts/okr/okr_progress_create_test.goshortcuts/okr/okr_progress_delete.goshortcuts/okr/okr_progress_delete_test.goshortcuts/okr/okr_progress_get.goshortcuts/okr/okr_progress_get_test.goshortcuts/okr/okr_progress_list.goshortcuts/okr/okr_progress_list_test.goshortcuts/okr/okr_progress_update.goshortcuts/okr/okr_progress_update_test.goshortcuts/okr/shortcuts.goskills/lark-okr/SKILL.mdskills/lark-okr/references/lark-okr-contentblock.mdskills/lark-okr/references/lark-okr-cycle-detail.mdskills/lark-okr/references/lark-okr-cycle-list.mdskills/lark-okr/references/lark-okr-entities.mdskills/lark-okr/references/lark-okr-image-upload.mdskills/lark-okr/references/lark-okr-progress-create.mdskills/lark-okr/references/lark-okr-progress-delete.mdskills/lark-okr/references/lark-okr-progress-get.mdskills/lark-okr/references/lark-okr-progress-list.mdskills/lark-okr/references/lark-okr-progress-update.mdtests/cli_e2e/okr/okr_progress_test.go
Comment on lines
68
to
+69
| "en": { "title": "OKR", "description": "Lark OKR objectives, key results, alignments, indicators" }, | ||
| "zh": { "title": "OKR", "description": "飞书 OKR 目标、关键结果、对齐、量化指标" } | ||
| "zh": { "title": "OKR", "description": "飞书 OKR 目标、关键结果、对齐、量化指标、进展记录" } |
There was a problem hiding this comment.
Keep okr EN/ZH descriptions semantically aligned.
zh now includes progress records, but en does not. This creates localized capability mismatch.
Suggested fix
- "en": { "title": "OKR", "description": "Lark OKR objectives, key results, alignments, indicators" },
+ "en": { "title": "OKR", "description": "Lark OKR objectives, key results, alignments, indicators, progress records" },📝 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
| "en": { "title": "OKR", "description": "Lark OKR objectives, key results, alignments, indicators" }, | |
| "zh": { "title": "OKR", "description": "飞书 OKR 目标、关键结果、对齐、量化指标" } | |
| "zh": { "title": "OKR", "description": "飞书 OKR 目标、关键结果、对齐、量化指标、进展记录" } | |
| "en": { "title": "OKR", "description": "Lark OKR objectives, key results, alignments, indicators, progress records" }, | |
| "zh": { "title": "OKR", "description": "飞书 OKR 目标、关键结果、对齐、量化指标、进展记录" } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@internal/registry/service_descriptions.json` around lines 68 - 69, The EN and
ZH entries for the "OKR" service in service_descriptions.json are semantically
mismatched: the "zh" description mentions progress records while "en" does not;
update the "en" description for the "OKR" entry (the object under key "en"
alongside "zh") to include the same capabilities (e.g., add "progress records"
or equivalent phrase) so both locales describe the same features, or
alternatively remove "progress records" from the "zh" description to keep them
aligned—edit the "en" description string to mirror the ZH items (object title
"OKR", description field).
syh-cpdsss
force-pushed
the
feat/okr_dev
branch
from
1b5248b to
2fe2499
Compare
|
|
||
| ```json | ||
| { | ||
| "file_token": "boxbcc6DmPfgi4rNXIaGfptc9HX", |
There was a problem hiding this comment.
🛑 Gitleaks has detected a secret with rule-id generic-api-key in commit 2fe2499.
If this secret is a true positive, please rotate the secret ASAP.
If this secret is a false positive, you can add the fingerprint below to your .gitleaksignore file and commit the change to this branch.
echo 2fe2499daae36a935fd3e3f097bc8b646c2e3f28:skills/lark-okr/references/lark-okr-image-upload.md:generic-api-key:45 >> .gitleaksignore
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (5)
shortcuts/okr/okr_progress_create.go (1)
219-228:⚠️ Potential issue | 🟡 MinorUse user-facing wording instead of
ProgressV1.This still exposes the internal DTO name in CLI output; use neutral labels like “Progress Record” and “Progress”.
📝 Proposed fix
runtime.OutFormat(result, nil, func(w io.Writer) { - fmt.Fprintf(w, "Created ProgressV1 Record [%s]\n", resp.ID) + fmt.Fprintf(w, "Created Progress Record [%s]\n", resp.ID) fmt.Fprintf(w, " ModifyTime: %s\n", resp.ModifyTime) if resp.ProgressRate != nil && resp.ProgressRate.Percent != nil { - fmt.Fprintf(w, " ProgressV1: %.1f%%\n", *resp.ProgressRate.Percent) + fmt.Fprintf(w, " Progress: %.1f%%\n", *resp.ProgressRate.Percent) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@shortcuts/okr/okr_progress_create.go` around lines 219 - 228, The CLI output is using internal DTO names; update the strings inside the runtime.OutFormat call to use user-facing labels: change "Created ProgressV1 Record [%s]\n" to something like "Created Progress Record [%s]\n" and change " ProgressV1: %.1f%%\n" to " Progress: %.1f%%\n" (keep resp.ID, resp.ModifyTime, resp.ProgressRate.Percent and resp.Content usage the same) in the anonymous func passed to runtime.OutFormat so the output shows neutral labels rather than DTO names.shortcuts/okr/okr_progress_update.go (1)
164-172:⚠️ Potential issue | 🟡 MinorUse user-facing wording instead of
ProgressV1.This still exposes the internal DTO name in CLI output; use neutral labels like “Progress Record” and “Progress”.
📝 Proposed fix
runtime.OutFormat(result, nil, func(w io.Writer) { - fmt.Fprintf(w, "Updated ProgressV1 Record [%s]\n", resp.ID) + fmt.Fprintf(w, "Updated Progress Record [%s]\n", resp.ID) fmt.Fprintf(w, " ModifyTime: %s\n", resp.ModifyTime) if resp.ProgressRate != nil && resp.ProgressRate.Percent != nil { - fmt.Fprintf(w, " ProgressV1: %.1f%%\n", *resp.ProgressRate.Percent) + fmt.Fprintf(w, " Progress: %.1f%%\n", *resp.ProgressRate.Percent) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@shortcuts/okr/okr_progress_update.go` around lines 164 - 172, Update the user-facing CLI messages in the runtime.OutFormat block that currently expose the internal DTO name: change the "Updated ProgressV1 Record [%s]\n" string to "Updated Progress Record [%s]\n" and the " ProgressV1: %.1f%%\n" label to a neutral " Progress: %.1f%%\n"; keep using resp.ID and resp.ProgressRate.Percent (and the existing Content/ModifyTime output) but replace any other occurrences of the "ProgressV1" token in that printing block to "Progress" to avoid leaking internal type names.skills/lark-okr/references/lark-okr-image-upload.md (1)
43-49:⚠️ Potential issue | 🟡 MinorReplace token-shaped examples with obvious placeholders.
These sample values look like real API tokens and have already triggered secret scanning. Use non-secret placeholders to keep docs scanner-safe. Based on learnings, Never commit secrets, tokens, or internal sensitive data.
📝 Proposed doc fix
{ - "file_token": "boxbcc6DmPfgi4rNXIaGfptc9HX", - "url": "https://example.larksuite.com/download?file_token=XXXXXXXXX", + "file_token": "FILE_TOKEN_PLACEHOLDER", + "url": "https://example.larksuite.com/download?file_token=FILE_TOKEN_PLACEHOLDER", "file_name": "screenshot.png", "size": 102400 }{ - "file_token": "XXXXXXXX", + "file_token": "FILE_TOKEN_PLACEHOLDER", "width": 800, "height": 600Also applies to: 83-85
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@skills/lark-okr/references/lark-okr-image-upload.md` around lines 43 - 49, Replace token-shaped sample values in the JSON example with obvious non-secret placeholders: change "file_token": "boxbcc6DmPfgi4rNXIaGfptc9HX" to something like "file_token": "<FILE_TOKEN_PLACEHOLDER>", the "url" query value "XXXXXXXXX" to "<FILE_TOKEN_PLACEHOLDER>" or "<DOWNLOAD_TOKEN>", and any real-looking file names (e.g., "screenshot.png") to a benign placeholder like "<FILE_NAME>". Update the example JSON in lark-okr-image-upload.md (and the duplicate instances referenced at lines 83-85) so no token-shaped strings or internal-looking identifiers remain.README.zh.md (1)
159-159:⚠️ Potential issue | 🟡 MinorMention progress records in the
lark-okrskill row.The feature table includes OKR 进展记录, but the Agent Skills row is still missing it.
📝 Proposed doc fix
-| `lark-okr` | 查询、创建、更新 OKR，管理目标、关键结果、对齐和指标 | +| `lark-okr` | 查询、创建、更新 OKR，管理目标、关键结果、对齐、指标和进展记录 |🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@README.zh.md` at line 159, Update the `lark-okr` skill table row to include the missing "OKR 进展记录" feature: edit README.zh.md and change the `lark-okr` row (the line containing "`lark-okr` | 查询、创建、更新 OKR，管理目标、关键结果、对齐和指标") to also list "OKR 进展记录" so it matches the feature table (e.g., append "、OKR 进展记录" to that cell).skills/lark-okr/SKILL.md (1)
33-33:⚠️ Potential issue | 🟡 MinorInclude progress records in the ContentBlock summary.
Progress record content also uses ContentBlock, so this summary is still stale.
📝 Proposed doc fix
-- [`ContentBlock 富文本格式`](references/lark-okr-contentblock.md) — Objective/KeyResult/Notes 字段使用的富文本格式说明 +- [`ContentBlock 富文本格式`](references/lark-okr-contentblock.md) — Objective/KeyResult/Notes/进展记录内容使用的富文本格式说明🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@skills/lark-okr/SKILL.md` at line 33, Update the ContentBlock summary in SKILL.md so it explicitly states that progress records also use the ContentBlock rich-text format; modify the existing bullet/link for "ContentBlock 富文本格式" (references/lark-okr-contentblock.md) and its descriptive text to mention Objective/KeyResult/Notes fields and Progress Records, ensuring the summary and link target reflect that progress record content is covered.
🧹 Nitpick comments (2)
tests/cli_e2e/okr/okr_progress_test.go (2)
38-39: Nit: preferassert.Containsoverassert.True(strings.Contains(...)).Throughout the file,
assert.Contains(t, output, substr, msg)gives clearer failure diffs than wrappingstrings.Containsinassert.True, and removes the need to importstrings. Purely optional.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/cli_e2e/okr/okr_progress_test.go` around lines 38 - 39, Replace uses of assert.True(t, strings.Contains(output, ...)) with assert.Contains(t, output, ...) to get clearer failure output; update the two assertions that check for "/open-apis/okr/v1/images/upload" and "POST" accordingly (replace the calls in the test function that contains those assert.True lines) and remove the now-unnecessary strings import from the test file.
91-113:_WithProgressvariant doesn't actually verify progress fields.
TestOKR_ProgressCreateDryRun_WithProgress(and the analogousTestOKR_ProgressUpdateDryRun_WithProgressat lines 183-204) only asserts the same API path as the base test — it never checks thatprogress_rate,75,normal, orprogress_statusappear in the dry-run output. As written, the test would still pass even if--progress-percent/--progress-statuswere silently dropped from the request body, defeating the purpose of the variant.Consider asserting on the progress-related substrings so the test meaningfully differs from the base case, e.g.:
Proposed additional assertions
output := result.Stdout assert.True(t, strings.Contains(output, "/open-apis/okr/v1/progress_records/"), "dry-run should contain API path, got: %s", output) + assert.Contains(t, output, "progress_rate", "dry-run should include progress_rate body field, got: %s", output) + assert.Contains(t, output, "75", "dry-run should include progress percent value, got: %s", output)Same treatment for
TestOKR_ProgressUpdateDryRun_WithProgress(check for100/done).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/cli_e2e/okr/okr_progress_test.go` around lines 91 - 113, TestOKR_ProgressCreateDryRun_WithProgress currently only asserts the API path and never verifies that progress fields are present; update the test (and the analogous TestOKR_ProgressUpdateDryRun_WithProgress) to also assert that the dry-run output contains the progress-related substrings: in TestOKR_ProgressCreateDryRun_WithProgress check for "progress_rate" (or "progress_percent"), "75" and "progress_status" / "normal" using assert.True(strings.Contains(output, ...)) so the dry-run includes the progress percent and status; similarly, in TestOKR_ProgressUpdateDryRun_WithProgress add assertions that the output contains the updated progress values (e.g., "100" and "done") so the tests fail if those flags are dropped from the request body.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@README.zh.md`:
- Line 16: Update the inconsistent skill count in the Chinese README by making
all occurrences agree with the corrected number (change the text fragment
"**覆盖面广** — 15 大业务域、200+ 精选命令、23 个 AI Agent [Skills](./skills/)" and any nearby
instances that say "22" such as the intro sentence and the "Agent-native"
bullet) so every mention of the Skills count reads "23" (or the intended
canonical number) across the file.
In `@shortcuts/okr/okr_progress_create.go`:
- Around line 58-72: The code currently accepts --progress-status alone but
ignores it because progress_rate is only set when --progress-percent is
provided; update the validation in the block handling
runtime.Str("progress-percent") and runtime.Str("progress-status") so that if
runtime.Str("progress-status") is set but runtime.Str("progress-percent") is
empty you return a FlagError (e.g., via common.FlagErrorf) stating that
--progress-status requires --progress-percent; keep using ParseProgressStatus to
validate the status and continue to set progressRate = &ProgressRateV1{Percent:
&percent} and progressRate.Status = int32Ptr(int32(status)) when both are
present.
In `@shortcuts/okr/okr_progress_update.go`:
- Around line 35-49: The code currently allows --progress-status to be validated
but then silently ignored if --progress-percent is not provided because
progressRate.Status is only set inside the percent branch; update
parseUpdateProgressRecordParams to reject status-only updates by checking
runtime.Str("progress-status") when runtime.Str("progress-percent") is empty and
returning a FlagErrorf like "--progress-status requires --progress-percent"
(apply the same check in both occurrences around the percent/status handling
blocks), and keep using ParseProgressStatus to validate the status before
returning the error so users cannot pass a status without a percent; reference
the existing variables/functions progressRate, runtime.Str("progress-percent"),
runtime.Str("progress-status"), and ParseProgressStatus to locate the changes.
---
Duplicate comments:
In `@README.zh.md`:
- Line 159: Update the `lark-okr` skill table row to include the missing "OKR
进展记录" feature: edit README.zh.md and change the `lark-okr` row (the line
containing "`lark-okr` | 查询、创建、更新 OKR，管理目标、关键结果、对齐和指标") to also list "OKR 进展记录"
so it matches the feature table (e.g., append "、OKR 进展记录" to that cell).
In `@shortcuts/okr/okr_progress_create.go`:
- Around line 219-228: The CLI output is using internal DTO names; update the
strings inside the runtime.OutFormat call to use user-facing labels: change
"Created ProgressV1 Record [%s]\n" to something like "Created Progress Record
[%s]\n" and change " ProgressV1: %.1f%%\n" to " Progress: %.1f%%\n" (keep
resp.ID, resp.ModifyTime, resp.ProgressRate.Percent and resp.Content usage the
same) in the anonymous func passed to runtime.OutFormat so the output shows
neutral labels rather than DTO names.
In `@shortcuts/okr/okr_progress_update.go`:
- Around line 164-172: Update the user-facing CLI messages in the
runtime.OutFormat block that currently expose the internal DTO name: change the
"Updated ProgressV1 Record [%s]\n" string to "Updated Progress Record [%s]\n"
and the " ProgressV1: %.1f%%\n" label to a neutral " Progress: %.1f%%\n"; keep
using resp.ID and resp.ProgressRate.Percent (and the existing Content/ModifyTime
output) but replace any other occurrences of the "ProgressV1" token in that
printing block to "Progress" to avoid leaking internal type names.
In `@skills/lark-okr/references/lark-okr-image-upload.md`:
- Around line 43-49: Replace token-shaped sample values in the JSON example with
obvious non-secret placeholders: change "file_token":
"boxbcc6DmPfgi4rNXIaGfptc9HX" to something like "file_token":
"<FILE_TOKEN_PLACEHOLDER>", the "url" query value "XXXXXXXXX" to
"<FILE_TOKEN_PLACEHOLDER>" or "<DOWNLOAD_TOKEN>", and any real-looking file
names (e.g., "screenshot.png") to a benign placeholder like "<FILE_NAME>".
Update the example JSON in lark-okr-image-upload.md (and the duplicate instances
referenced at lines 83-85) so no token-shaped strings or internal-looking
identifiers remain.
In `@skills/lark-okr/SKILL.md`:
- Line 33: Update the ContentBlock summary in SKILL.md so it explicitly states
that progress records also use the ContentBlock rich-text format; modify the
existing bullet/link for "ContentBlock 富文本格式"
(references/lark-okr-contentblock.md) and its descriptive text to mention
Objective/KeyResult/Notes fields and Progress Records, ensuring the summary and
link target reflect that progress record content is covered.
---
Nitpick comments:
In `@tests/cli_e2e/okr/okr_progress_test.go`:
- Around line 38-39: Replace uses of assert.True(t, strings.Contains(output,
...)) with assert.Contains(t, output, ...) to get clearer failure output; update
the two assertions that check for "/open-apis/okr/v1/images/upload" and "POST"
accordingly (replace the calls in the test function that contains those
assert.True lines) and remove the now-unnecessary strings import from the test
file.
- Around line 91-113: TestOKR_ProgressCreateDryRun_WithProgress currently only
asserts the API path and never verifies that progress fields are present; update
the test (and the analogous TestOKR_ProgressUpdateDryRun_WithProgress) to also
assert that the dry-run output contains the progress-related substrings: in
TestOKR_ProgressCreateDryRun_WithProgress check for "progress_rate" (or
"progress_percent"), "75" and "progress_status" / "normal" using
assert.True(strings.Contains(output, ...)) so the dry-run includes the progress
percent and status; similarly, in TestOKR_ProgressUpdateDryRun_WithProgress add
assertions that the output contains the updated progress values (e.g., "100" and
"done") so the tests fail if those flags are dropped from the request body.
🪄 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: 609fe1b7-536e-4c87-a617-0406ca52bf25
📒 Files selected for processing (31)
README.mdREADME.zh.mdinternal/registry/service_descriptions.jsonshortcuts/okr/okr_cli_resp.goshortcuts/okr/okr_image_upload.goshortcuts/okr/okr_image_upload_test.goshortcuts/okr/okr_openapi.goshortcuts/okr/okr_openapi_test.goshortcuts/okr/okr_progress_create.goshortcuts/okr/okr_progress_create_test.goshortcuts/okr/okr_progress_delete.goshortcuts/okr/okr_progress_delete_test.goshortcuts/okr/okr_progress_get.goshortcuts/okr/okr_progress_get_test.goshortcuts/okr/okr_progress_list.goshortcuts/okr/okr_progress_list_test.goshortcuts/okr/okr_progress_update.goshortcuts/okr/okr_progress_update_test.goshortcuts/okr/shortcuts.goskills/lark-okr/SKILL.mdskills/lark-okr/references/lark-okr-contentblock.mdskills/lark-okr/references/lark-okr-cycle-detail.mdskills/lark-okr/references/lark-okr-cycle-list.mdskills/lark-okr/references/lark-okr-entities.mdskills/lark-okr/references/lark-okr-image-upload.mdskills/lark-okr/references/lark-okr-progress-create.mdskills/lark-okr/references/lark-okr-progress-delete.mdskills/lark-okr/references/lark-okr-progress-get.mdskills/lark-okr/references/lark-okr-progress-list.mdskills/lark-okr/references/lark-okr-progress-update.mdtests/cli_e2e/okr/okr_progress_test.go
✅ Files skipped from review due to trivial changes (11)
- README.md
- skills/lark-okr/references/lark-okr-contentblock.md
- internal/registry/service_descriptions.json
- skills/lark-okr/references/lark-okr-cycle-detail.md
- skills/lark-okr/references/lark-okr-progress-list.md
- skills/lark-okr/references/lark-okr-progress-delete.md
- shortcuts/okr/okr_image_upload_test.go
- skills/lark-okr/references/lark-okr-progress-get.md
- shortcuts/okr/okr_progress_update_test.go
- skills/lark-okr/references/lark-okr-entities.md
- shortcuts/okr/okr_progress_create_test.go
🚧 Files skipped from review as they are similar to previous changes (11)
- shortcuts/okr/shortcuts.go
- shortcuts/okr/okr_progress_delete.go
- shortcuts/okr/okr_progress_get.go
- skills/lark-okr/references/lark-okr-cycle-list.md
- shortcuts/okr/okr_progress_list_test.go
- shortcuts/okr/okr_progress_get_test.go
- shortcuts/okr/okr_cli_resp.go
- shortcuts/okr/okr_progress_list.go
- skills/lark-okr/references/lark-okr-progress-create.md
- shortcuts/okr/okr_image_upload.go
- shortcuts/okr/okr_openapi_test.go
syh-cpdsss
force-pushed
the
feat/okr_dev
branch
from
2fe2499 to
2a9b376
Compare
|
|
||
| ```json | ||
| { | ||
| "file_token": "boxbcc6DmPfgi4rNXIaGfptc9HX", |
There was a problem hiding this comment.
🛑 Gitleaks has detected a secret with rule-id generic-api-key in commit 2a9b376.
If this secret is a true positive, please rotate the secret ASAP.
If this secret is a false positive, you can add the fingerprint below to your .gitleaksignore file and commit the change to this branch.
echo 2a9b37618fa76af3cce2c964b993e09f5180de65:skills/lark-okr/references/lark-okr-image-upload.md:generic-api-key:45 >> .gitleaksignore
2a9b376 to
2bfb322
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (4)
README.zh.md (1)
15-15:⚠️ Potential issue | 🟡 MinorFix the remaining skill count inconsistency.
Line 15 still says "22 个 [Skills]" but should match the corrected count of 23 used in Lines 9 and 16. This was partially addressed in previous reviews but Line 15 was missed.
📝 Proposed fix
-- **为 Agent 原生设计** — 22 个 [Skills](./skills/) 开箱即用,适配主流 AI 工具,Agent 无需额外适配即可操作飞书 +- **为 Agent 原生设计** — 23 个 [Skills](./skills/) 开箱即用,适配主流 AI 工具,Agent 无需额外适配即可操作飞书🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@README.zh.md` at line 15, Update the remaining inconsistent skill count in README.zh.md by changing the string "为 Agent 原生设计 — 22 个 [Skills](./skills/) 开箱即用，适配主流 AI 工具，Agent 无需额外适配即可操作飞书" to use "23 个 [Skills]" so it matches the corrected occurrences on Lines 9 and 16; locate the exact sentence (the one starting with "为 Agent 原生设计") and replace the numeral 22 with 23.shortcuts/okr/okr_progress_update.go (1)
42-49:⚠️ Potential issue | 🟠 MajorReject status-only updates instead of silently dropping them.
--progress-statuscurrently passes validation without--progress-percent, butparseUpdateProgressRecordParamsonly includesstatusinside the percent branch, so the user’s update becomes a no-op.Suggested fix
var progressRate *ProgressRateV1 + if runtime.Str("progress-status") != "" && runtime.Str("progress-percent") == "" { + return nil, common.FlagErrorf("--progress-status requires --progress-percent") + } if v := runtime.Str("progress-percent"); v != "" { percent, err := strconv.ParseFloat(v, 64) if err != nil || percent < 0 || percent > 100 { return nil, common.FlagErrorf("--progress-percent must be a number between 0 and 100"){Name: "progress-percent", Desc: "progress percentage (0-100)"}, - {Name: "progress-status", Desc: "progress status: normal | overdue | done", Enum: []string{"normal", "overdue", "done"}}, + {Name: "progress-status", Desc: "progress status: normal | overdue | done; requires --progress-percent", Enum: []string{"normal", "overdue", "done"}},if v := runtime.Str("progress-status"); v != "" { if _, ok := ParseProgressStatus(v); !ok { return common.FlagErrorf("--progress-status must be one of: normal | overdue | done") } + if runtime.Str("progress-percent") == "" { + return common.FlagErrorf("--progress-status requires --progress-percent") + } }Also applies to: 102-106
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@shortcuts/okr/okr_progress_update.go` around lines 42 - 49, The handler allows a validated --progress-status (via runtime.Str and ParseProgressStatus) to be set but then only attaches status to the progress payload inside the percent branch of parseUpdateProgressRecordParams, causing status-only updates to be dropped; modify parseUpdateProgressRecordParams so that when ParseProgressStatus returns ok you set progressRate.Status (and include it in the returned payload) regardless of whether --progress-percent was provided, and add validation to reject status-only updates if policy requires percent together (or conversely accept status-only by ensuring progressRate.Status is applied outside the percent branch); update references in parseUpdateProgressRecordParams and any callers to ensure progressRate (and its Status field) is included in the constructed update request.shortcuts/okr/okr_progress_create.go (1)
222-226:⚠️ Potential issue | 🟡 MinorRemove the internal
ProgressV1name from CLI output.
ProgressV1is an implementation detail; keep human-readable output version-neutral.Suggested wording cleanup
runtime.OutFormat(result, nil, func(w io.Writer) { - fmt.Fprintf(w, "Created ProgressV1 Record [%s]\n", resp.ID) + fmt.Fprintf(w, "Created Progress Record [%s]\n", resp.ID) fmt.Fprintf(w, " ModifyTime: %s\n", resp.ModifyTime) if resp.ProgressRate != nil && resp.ProgressRate.Percent != nil { - fmt.Fprintf(w, " ProgressV1: %.1f%%\n", *resp.ProgressRate.Percent) + fmt.Fprintf(w, " Progress: %.1f%%\n", *resp.ProgressRate.Percent) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@shortcuts/okr/okr_progress_create.go` around lines 222 - 226, The CLI output prints an internal implementation name "ProgressV1" — update the fmt.Fprintf call inside runtime.OutFormat (the block using resp.ID, resp.ModifyTime and resp.ProgressRate.Percent) to remove "ProgressV1" and use a version-neutral label such as "Progress" (i.e., change the string " ProgressV1: ..." to " Progress: ..."), keeping the rest of the formatting and use of resp.ProgressRate.Percent unchanged.skills/lark-okr/references/lark-okr-image-upload.md (1)
43-49:⚠️ Potential issue | 🟡 MinorUse unmistakable placeholders for token examples.
The sample
file_tokenand URL query value look secret-like and have already tripped secret scanning. Replace them with clearly synthetic placeholders so Gitleaks does not keep blocking the PR. Based on learnings, Never commit secrets, tokens, or internal sensitive data.Suggested doc-only cleanup
{ - "file_token": "boxbcc6DmPfgi4rNXIaGfptc9HX", - "url": "https://example.larksuite.com/download?file_token=XXXXXXXXX", + "file_token": "FILE_TOKEN_PLACEHOLDER", + "url": "https://example.larksuite.com/download?file_token=FILE_TOKEN_PLACEHOLDER", "file_name": "screenshot.png", "size": 102400 }{ - "file_token": "XXXXXXXX", + "file_token": "FILE_TOKEN_PLACEHOLDER", "width": 800, "height": 600 }Also applies to: 80-86
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@skills/lark-okr/references/lark-okr-image-upload.md` around lines 43 - 49, The example JSON contains secret-like values—replace the "file_token" value and the URL query parameter with unmistakably synthetic placeholders (e.g., FILE_TOKEN_EXAMPLE or <FILE_TOKEN_EXAMPLE>) so scanners won’t flag them; update the "file_token" field and the "url" value in the sample JSON (and the duplicate occurrences later in the document) to use these clearly non-secret placeholders while keeping "file_name" and "size" unchanged. Ensure all occurrences of the token-like string in the document are replaced consistently so Gitleaks won't block the PR.
🧹 Nitpick comments (1)
shortcuts/okr/okr_progress_delete_test.go (1)
81-97: Minor:TestProgressDeleteValidate_Validlargely duplicatesTestProgressDeleteExecute_Success.Both register a success stub for
DELETE /open-apis/okr/v1/progress_records/<id>and assert no error. Since the validation path with a valid numeric id is already indirectly exercised by the Execute and DryRun tests, this case adds little unique coverage. Consider either dropping it or narrowing it to a pure validation check (e.g., by asserting the command proceeds past Validate without needing a stub — perhaps via--dry-run) to keep intent distinct from the Execute success test.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@shortcuts/okr/okr_progress_delete_test.go` around lines 81 - 97, TestProgressDeleteValidate_Valid is redundant with TestProgressDeleteExecute_Success; either remove it or convert it into a pure validation check: delete the HTTP stub registration (reg.Register(...)) and invoke runProgressDeleteShortcut with the same args plus "--dry-run" (or otherwise trigger only Validate) so the test only asserts that Validate passes for a numeric progress id; reference TestProgressDeleteValidate_Valid, runProgressDeleteShortcut, and progressDeleteTestConfig to locate and update the test.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@shortcuts/okr/okr_progress_create.go`:
- Around line 59-63: The flag parsing for --progress-percent uses
strconv.ParseFloat and only checks numeric range, but ParseFloat accepts "NaN"
and "Inf" which bypass range checks; update both parsing sites (the
runtime.Str("progress-percent") branch where percent is assigned and the second
identical ParseFloat block later) to explicitly detect and reject NaN/Inf after
parsing by using math.IsNaN(percent) || math.IsInf(percent, 0) and return the
same FlagErrorf("--progress-percent must be a number between 0 and 100") when
true; also add unit tests that pass "--progress-percent NaN" and
"--progress-percent Inf" to the OKR progress creation command and assert those
produce the flag error.
In `@shortcuts/okr/okr_progress_update.go`:
- Around line 36-40: ParseFloat accepts "NaN"/"Inf", so after parsing the value
returned by runtime.Str("progress-percent") in both places where
strconv.ParseFloat is used, add explicit checks using math.IsNaN(percent) and
math.IsInf(percent, 0) before the existing range check; if either is true,
return the same FlagErrorf("--progress-percent must be a number between 0 and
100"). Update both occurrences (the blocks that parse
runtime.Str("progress-percent")) and add a unit test that invokes the command
with `--progress-percent NaN` to assert it fails with the expected flag error.
---
Duplicate comments:
In `@README.zh.md`:
- Line 15: Update the remaining inconsistent skill count in README.zh.md by
changing the string "为 Agent 原生设计 — 22 个 [Skills](./skills/) 开箱即用，适配主流 AI
工具，Agent 无需额外适配即可操作飞书" to use "23 个 [Skills]" so it matches the corrected
occurrences on Lines 9 and 16; locate the exact sentence (the one starting with
"为 Agent 原生设计") and replace the numeral 22 with 23.
In `@shortcuts/okr/okr_progress_create.go`:
- Around line 222-226: The CLI output prints an internal implementation name
"ProgressV1" — update the fmt.Fprintf call inside runtime.OutFormat (the block
using resp.ID, resp.ModifyTime and resp.ProgressRate.Percent) to remove
"ProgressV1" and use a version-neutral label such as "Progress" (i.e., change
the string " ProgressV1: ..." to " Progress: ..."), keeping the rest of the
formatting and use of resp.ProgressRate.Percent unchanged.
In `@shortcuts/okr/okr_progress_update.go`:
- Around line 42-49: The handler allows a validated --progress-status (via
runtime.Str and ParseProgressStatus) to be set but then only attaches status to
the progress payload inside the percent branch of
parseUpdateProgressRecordParams, causing status-only updates to be dropped;
modify parseUpdateProgressRecordParams so that when ParseProgressStatus returns
ok you set progressRate.Status (and include it in the returned payload)
regardless of whether --progress-percent was provided, and add validation to
reject status-only updates if policy requires percent together (or conversely
accept status-only by ensuring progressRate.Status is applied outside the
percent branch); update references in parseUpdateProgressRecordParams and any
callers to ensure progressRate (and its Status field) is included in the
constructed update request.
In `@skills/lark-okr/references/lark-okr-image-upload.md`:
- Around line 43-49: The example JSON contains secret-like values—replace the
"file_token" value and the URL query parameter with unmistakably synthetic
placeholders (e.g., FILE_TOKEN_EXAMPLE or <FILE_TOKEN_EXAMPLE>) so scanners
won’t flag them; update the "file_token" field and the "url" value in the sample
JSON (and the duplicate occurrences later in the document) to use these clearly
non-secret placeholders while keeping "file_name" and "size" unchanged. Ensure
all occurrences of the token-like string in the document are replaced
consistently so Gitleaks won't block the PR.
---
Nitpick comments:
In `@shortcuts/okr/okr_progress_delete_test.go`:
- Around line 81-97: TestProgressDeleteValidate_Valid is redundant with
TestProgressDeleteExecute_Success; either remove it or convert it into a pure
validation check: delete the HTTP stub registration (reg.Register(...)) and
invoke runProgressDeleteShortcut with the same args plus "--dry-run" (or
otherwise trigger only Validate) so the test only asserts that Validate passes
for a numeric progress id; reference TestProgressDeleteValidate_Valid,
runProgressDeleteShortcut, and progressDeleteTestConfig to locate and update the
test.
🪄 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: bdc9e3d1-7ee2-41ab-b271-5e171dd61920
📒 Files selected for processing (31)
README.mdREADME.zh.mdinternal/registry/service_descriptions.jsonshortcuts/okr/okr_cli_resp.goshortcuts/okr/okr_image_upload.goshortcuts/okr/okr_image_upload_test.goshortcuts/okr/okr_openapi.goshortcuts/okr/okr_openapi_test.goshortcuts/okr/okr_progress_create.goshortcuts/okr/okr_progress_create_test.goshortcuts/okr/okr_progress_delete.goshortcuts/okr/okr_progress_delete_test.goshortcuts/okr/okr_progress_get.goshortcuts/okr/okr_progress_get_test.goshortcuts/okr/okr_progress_list.goshortcuts/okr/okr_progress_list_test.goshortcuts/okr/okr_progress_update.goshortcuts/okr/okr_progress_update_test.goshortcuts/okr/shortcuts.goskills/lark-okr/SKILL.mdskills/lark-okr/references/lark-okr-contentblock.mdskills/lark-okr/references/lark-okr-cycle-detail.mdskills/lark-okr/references/lark-okr-cycle-list.mdskills/lark-okr/references/lark-okr-entities.mdskills/lark-okr/references/lark-okr-image-upload.mdskills/lark-okr/references/lark-okr-progress-create.mdskills/lark-okr/references/lark-okr-progress-delete.mdskills/lark-okr/references/lark-okr-progress-get.mdskills/lark-okr/references/lark-okr-progress-list.mdskills/lark-okr/references/lark-okr-progress-update.mdtests/cli_e2e/okr/okr_progress_test.go
✅ Files skipped from review due to trivial changes (12)
- skills/lark-okr/references/lark-okr-cycle-detail.md
- skills/lark-okr/references/lark-okr-contentblock.md
- internal/registry/service_descriptions.json
- shortcuts/okr/okr_progress_delete.go
- skills/lark-okr/references/lark-okr-progress-get.md
- skills/lark-okr/references/lark-okr-progress-delete.md
- skills/lark-okr/references/lark-okr-progress-list.md
- skills/lark-okr/references/lark-okr-progress-create.md
- shortcuts/okr/okr_cli_resp.go
- shortcuts/okr/okr_progress_create_test.go
- shortcuts/okr/okr_image_upload_test.go
- skills/lark-okr/SKILL.md
🚧 Files skipped from review as they are similar to previous changes (8)
- README.md
- shortcuts/okr/shortcuts.go
- shortcuts/okr/okr_progress_get_test.go
- shortcuts/okr/okr_progress_list_test.go
- shortcuts/okr/okr_progress_get.go
- skills/lark-okr/references/lark-okr-entities.md
- shortcuts/okr/okr_image_upload.go
- tests/cli_e2e/okr/okr_progress_test.go
There was a problem hiding this comment.
Actionable comments posted: 7
♻️ Duplicate comments (4)
shortcuts/okr/okr_progress_update.go (1)
96-106:⚠️ Potential issue | 🟠 MajorStatus-only updates are still silently dropped (cross-check missing).
--progress-statuspassesValidatewithout--progress-percent, butparseUpdateProgressRecordParams(lines 35-49) only setsprogressRate.Statusinside the percent branch — solark-cli okr +progress-update --progress-id X --content … --progress-status donevalidates fine, sends a body with noprogress_rateat all, and the status change is silently lost. The create shortcut already enforces this cross-check atokr_progress_create.go:155-157; please mirror that here.🛠️ Suggested fix
if v := runtime.Str("progress-status"); v != "" { if _, ok := ParseProgressStatus(v); !ok { return common.FlagErrorf("--progress-status must be one of: normal | overdue | done") } + if runtime.Str("progress-percent") == "" { + return common.FlagErrorf("--progress-status requires --progress-percent") + } }Also update the flag description on line 72 to hint at this constraint:
- {Name: "progress-status", Desc: "progress status: normal | overdue | done", Enum: []string{"normal", "overdue", "done"}}, + {Name: "progress-status", Desc: "progress status (requires --progress-percent): normal | overdue | done", Enum: []string{"normal", "overdue", "done"}},🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@shortcuts/okr/okr_progress_update.go` around lines 96 - 106, The validation currently allows --progress-status without --progress-percent, but parseUpdateProgressRecordParams only sets progressRate.Status when a percent is provided so status changes get dropped; update the Validate logic to require that if runtime.Str("progress-status") is non-empty then runtime.Str("progress-percent") must also be provided (return a FlagErrorf if not), mirror the same cross-check used in the create flow, and also update the progress-status flag description to mention that status requires supplying --progress-percent so callers know the constraint; ensure ParseProgressStatus and parseUpdateProgressRecordParams behavior remains consistent with this validation.README.zh.md (1)
15-16:⚠️ Potential issue | 🟡 MinorSkill count on line 15 still says "22 个"; should be "23".
Lines 9 and 16 were updated to "23 个", but the "为 Agent 原生设计" bullet on line 15 was missed, leaving the README self-inconsistent.
📝 Suggested fix
-- **为 Agent 原生设计** — 22 个 [Skills](./skills/) 开箱即用，适配主流 AI 工具，Agent 无需额外适配即可操作飞书 +- **为 Agent 原生设计** — 23 个 [Skills](./skills/) 开箱即用，适配主流 AI 工具，Agent 无需额外适配即可操作飞书🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@README.zh.md` around lines 15 - 16, Update the "为 Agent 原生设计" bullet that currently reads "22 个 [Skills](./skills/)" to "23 个 [Skills](./skills/)" so it matches the other occurrences; look for the exact text "为 Agent 原生设计 — 22 个 [Skills](./skills/)" in README.zh.md and replace 22 with 23 to keep the document consistent with the "23 个" used elsewhere.shortcuts/okr/okr_progress_create.go (1)
155-157:⚠️ Potential issue | 🟡 MinorGrammar in error message: "must provided with" → "must be provided with".
Also, since this branch is triggered by
--progress-statusbeing set, the message reads more naturally in the other direction.🛠️ Suggested fix
- if v := runtime.Str("progress-percent"); v == "" { - return common.FlagErrorf("--progress-percent must provided with --progress-status") + if p := runtime.Str("progress-percent"); p == "" { + return common.FlagErrorf("--progress-status requires --progress-percent") }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@shortcuts/okr/okr_progress_create.go` around lines 155 - 157, Fix the grammar and clarify direction in the error string inside the check that validates runtime.Str("progress-percent") — update the common.FlagErrorf call currently returning "--progress-percent must provided with --progress-status" to a correct, clearer message such as "--progress-percent must be provided when --progress-status is set" (locate the check using runtime.Str("progress-percent") and the common.FlagErrorf invocation in okr_progress_create.go).shortcuts/okr/okr_image_upload.go (1)
51-54:⚠️ Potential issue | 🟡 MinorCase-sensitive extension check still rejects legitimately named files.
filepath.Extpreserves the original casing butallowedImageExtsonly has lowercase keys, sophoto.PNG,logo.JPG,pic.HEIC(common on macOS/iOS/Windows exports) are rejected even though the help text on line 42 advertises them. Normalize to lowercase before the lookup.🛠️ Proposed fix
import ( "context" "encoding/json" "errors" "fmt" "path/filepath" "strconv" + "strings"- ext := filepath.Ext(filePath) - if !allowedImageExts[ext] { + ext := strings.ToLower(filepath.Ext(filePath)) + if !allowedImageExts[ext] { return common.FlagErrorf("--file must be an image (supported: JPG, JPEG, PNG, WEBP, GIF, BMP, ICO, TIFF, HEIC), got %q", ext) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@shortcuts/okr/okr_image_upload.go` around lines 51 - 54, The extension check uses filepath.Ext(filePath) and looks it up in allowedImageExts but misses mixed-case extensions; normalize ext to lowercase (e.g., strings.ToLower(ext)) before the lookup so files like "photo.PNG", "logo.JPG", or "pic.HEIC" are accepted by the existing allowedImageExts map in the same validation block in okr_image_upload.go.
🧹 Nitpick comments (4)
shortcuts/okr/okr_progress_create.go (1)
166-186:DryRunsilently swallowsparseCreateProgressRecordParamserrors.
p, _ := parseCreateProgressRecordParams(runtime)discards the error, and thenp.UserIDType,p.TargetID, etc. are dereferenced. AlthoughValidateruns beforeDryRunin the normal flow and catches--contentJSON errors, the parse function is duplicating logic that can still produce a nilp(e.g., if the JSON sneaks through, or if the function is later extended). Consider either:
- having
Validatebe the single source of truth and lettingparseCreateProgressRecordParamsrely on its preconditions (thenpis safe to dereference), or- returning a
DryRunAPIthat surfaces the error viaDescwhenerr != nil.At minimum, a
nilguard onpwould prevent a panic if the Validate/Parse contract drifts in the future.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@shortcuts/okr/okr_progress_create.go` around lines 166 - 186, The DryRun closure currently ignores errors from parseCreateProgressRecordParams (p, _ := parseCreateProgressRecordParams(runtime)), which can lead to nil derefs when using p.UserIDType, p.TargetID, etc.; update DryRun in okr_progress_create.go to handle the error: call parseCreateProgressRecordParams and if err != nil return a common.NewDryRunAPI() that describes the parse error (e.g., via Desc) so the error is surfaced, or at minimum add a nil guard before dereferencing p and return a descriptive DryRunAPI when p == nil; reference the DryRun function, parseCreateProgressRecordParams, and common.NewDryRunAPI()/Desc to implement the fix.shortcuts/okr/okr_image_upload.go (2)
105-108: Minor: preferstrconv.Itoaoverfmt.Sprintf("%d", ...).Micro-readability/perf nit:
fd.AddField("target_type", strconv.Itoa(targetTypeVal))is the idiomatic Go form for integer → decimal string, avoiding the reflection path insidefmt.Sprintf.strconvis already imported.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@shortcuts/okr/okr_image_upload.go` around lines 105 - 108, Replace the use of fmt.Sprintf for converting the integer targetTypeVal to string when adding the "target_type" form field: in the NewFormdata usage where fd.AddField("target_type", fmt.Sprintf("%d", targetTypeVal)) is called, use strconv.Itoa(targetTypeVal) instead (keep fd, AddField and the "target_type" key unchanged).
91-114: Consider adding a pre-upload size check to fail fast on oversized images.
shortcuts/im/helpers.go:1128validates file size before uploading (5 MB limit for IM images). The OKR image upload endpoint currently opens and streams the file without any size validation. Adding an analogous pre-upload check would prevent unnecessary I/O and provide clearer error messages for users. The specific size limit for the OKR endpoint is not documented in the API; consider checking Lark/Feishu OKR upload constraints or aligning with platform-wide image limits (10 MB) when implementing.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@shortcuts/okr/okr_image_upload.go` around lines 91 - 114, Add a pre-upload size check right after the runtime.FileIO().Stat(filePath) call using info.Size() and fail fast if the file exceeds the OKR image limit (use a named constant like maxOKRImageSize, e.g. 10*1024*1024 if you choose 10MB); return the same style of error used in shortcuts/im/helpers.go (around helpers.go:1128) so the message and handling are consistent, and do this before calling runtime.FileIO().Open(filePath) and before constructing fd with larkcore.NewFormdata and calling runtime.DoAPI.shortcuts/okr/okr_openapi_test.go (1)
386-399: Test adds little value beyond exercisingcore.ResolveOpenBaseURL.
TestParseCreateProgressRecordParams_BrandAwareSourceURLdoesn't actually exerciseparseCreateProgressRecordParams; it only verifies constants fromcore.ResolveOpenBaseURL. Consider driving the check through the actualparseCreateProgressRecordParamscode path (or renaming the test) so it would catch regressions in the OKR-side brand-aware default logic rather than just thecorehelper.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@shortcuts/okr/okr_openapi_test.go` around lines 386 - 399, Test currently only exercises core.ResolveOpenBaseURL; instead invoke the OKR parsing path so the test covers parseCreateProgressRecordParams logic. Update TestParseCreateProgressRecordParams_BrandAwareSourceURL to construct a minimal ContentBlock JSON input, call parseCreateProgressRecordParams with BrandFeishu and BrandLark, and assert the returned params/sourceURL equals core.ResolveOpenBaseURL(core.BrandFeishu)+"/app" and core.ResolveOpenBaseURL(core.BrandLark)+"/app" respectively; alternatively, if you intend to keep it as a unit test of the helper, rename the test to reflect core.ResolveOpenBaseURL behavior. Use the existing test name and function parseCreateProgressRecordParams and the constants core.BrandFeishu/core.BrandLark in your assertions to make the intent clear.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@shortcuts/okr/okr_image_upload_test.go`:
- Around line 132-143: The test stub registered via reg.Register using
httpmock.Stub returns a fake "file_token" value of "xxxxxxxxx" which triggers
Gitleaks; update the stub in the POST "/open-apis/okr/v1/images/upload" block to
use a clearly non-secret placeholder (e.g., "test-file-token" or
"placeholder-file-token") for the "file_token" field so assertions still pass
but avoid secret-shaped literals in the map returned by the stub.
In `@shortcuts/okr/okr_openapi.go`:
- Around line 378-380: ContentGalleryV1 currently reuses ContentImageItem which
serializes JSON with snake_case tags (file_token) but the OKR v1 API expects
camelCase (fileToken), so update the model and conversion logic: define a
V1-specific image struct (e.g., ContentImageItemV1 or similar) with json tags
using camelCase, change ContentGalleryV1.ImageList to use that type, and modify
ContentGallery.ToV1() and ContentGalleryV1.ToV2() to map fields explicitly
(create new V1 image instances copying fileToken/src/width/height from V2 items
and vice versa) instead of assigning the slice by reference so the serialized
JSON uses the correct camelCase keys.
- Around line 711-721: The Progress struct's UpdateTime field must be renamed to
ModifyTime and its json tag changed from `update_time` to `modify_time` so
unmarshalling captures the API's `modify_time` value; update the Progress type
definition (field previously named UpdateTime) to ModifyTime
`json:"modify_time"` and search/replace all references/usages of
Progress.UpdateTime (e.g., any code reading/writing UpdateTime, methods, tests,
or JSON handling) to use Progress.ModifyTime to keep the codebase consistent.
- Around line 530-538: ContentMention.ToV1() currently maps UserID -> OpenID
without checking that the ID is actually an open_id, which can silently mislabel
union_id/user_id values; update handling so either: validate mention IDs against
the chosen --user-id-type and return an error (or skip/convert) when mismatched,
or restrict/annotate the CLI flag when content contains mentions (e.g., require
--user-id-type=open_id), and add clear help text documenting the limitation;
locate the conversion in ContentMention.ToV1() and the CLI flag
parsing/validation logic for --user-id-type to implement ID-type checks and
surface validation errors or update the flag help text accordingly.
In `@skills/lark-okr/references/lark-okr-entities.md`:
- Line 174: Replace the typo "oke +progress-list" with the correct shortcut "okr
+progress-list" in the references table entry that links to
lark-okr-progress-list.md so the shortcut matches the other entries and the link
target; search for the exact string "oke +progress-list" and update it to "okr
+progress-list" to keep naming consistent.
- Around line 325-328: The bottom reference list is missing the entry for the
progress-list operation; add a new list item for "okr +progress-list" pointing
to lark-okr-progress-list.md (consistent with the other entries like "okr
+progress-get", "okr +progress-create", etc.) so the documented inline "okr
+progress-list" (mentioned earlier) is included in the references; ensure the
format and ordering match the existing four entries.
In `@skills/lark-okr/references/lark-okr-image-upload.md`:
- Around line 43-50: Replace the secret-looking placeholder value for the JSON
key "file_token" (currently "XXXXXXXXX") with a clearly non-credential
placeholder such as REPLACE_WITH_FILE_TOKEN or example-file-token in the Lark
OKR image upload example; update both the inline example under the "file_token"
key and the gallery example (the duplicate at line 84) so neither uses a generic
API-key shaped string and Gitleaks false positives are avoided.
---
Duplicate comments:
In `@README.zh.md`:
- Around line 15-16: Update the "为 Agent 原生设计" bullet that currently reads "22 个
[Skills](./skills/)" to "23 个 [Skills](./skills/)" so it matches the other
occurrences; look for the exact text "为 Agent 原生设计 — 22 个 [Skills](./skills/)"
in README.zh.md and replace 22 with 23 to keep the document consistent with the
"23 个" used elsewhere.
In `@shortcuts/okr/okr_image_upload.go`:
- Around line 51-54: The extension check uses filepath.Ext(filePath) and looks
it up in allowedImageExts but misses mixed-case extensions; normalize ext to
lowercase (e.g., strings.ToLower(ext)) before the lookup so files like
"photo.PNG", "logo.JPG", or "pic.HEIC" are accepted by the existing
allowedImageExts map in the same validation block in okr_image_upload.go.
In `@shortcuts/okr/okr_progress_create.go`:
- Around line 155-157: Fix the grammar and clarify direction in the error string
inside the check that validates runtime.Str("progress-percent") — update the
common.FlagErrorf call currently returning "--progress-percent must provided
with --progress-status" to a correct, clearer message such as
"--progress-percent must be provided when --progress-status is set" (locate the
check using runtime.Str("progress-percent") and the common.FlagErrorf invocation
in okr_progress_create.go).
In `@shortcuts/okr/okr_progress_update.go`:
- Around line 96-106: The validation currently allows --progress-status without
--progress-percent, but parseUpdateProgressRecordParams only sets
progressRate.Status when a percent is provided so status changes get dropped;
update the Validate logic to require that if runtime.Str("progress-status") is
non-empty then runtime.Str("progress-percent") must also be provided (return a
FlagErrorf if not), mirror the same cross-check used in the create flow, and
also update the progress-status flag description to mention that status requires
supplying --progress-percent so callers know the constraint; ensure
ParseProgressStatus and parseUpdateProgressRecordParams behavior remains
consistent with this validation.
---
Nitpick comments:
In `@shortcuts/okr/okr_image_upload.go`:
- Around line 105-108: Replace the use of fmt.Sprintf for converting the integer
targetTypeVal to string when adding the "target_type" form field: in the
NewFormdata usage where fd.AddField("target_type", fmt.Sprintf("%d",
targetTypeVal)) is called, use strconv.Itoa(targetTypeVal) instead (keep fd,
AddField and the "target_type" key unchanged).
- Around line 91-114: Add a pre-upload size check right after the
runtime.FileIO().Stat(filePath) call using info.Size() and fail fast if the file
exceeds the OKR image limit (use a named constant like maxOKRImageSize, e.g.
10*1024*1024 if you choose 10MB); return the same style of error used in
shortcuts/im/helpers.go (around helpers.go:1128) so the message and handling are
consistent, and do this before calling runtime.FileIO().Open(filePath) and
before constructing fd with larkcore.NewFormdata and calling runtime.DoAPI.
In `@shortcuts/okr/okr_openapi_test.go`:
- Around line 386-399: Test currently only exercises core.ResolveOpenBaseURL;
instead invoke the OKR parsing path so the test covers
parseCreateProgressRecordParams logic. Update
TestParseCreateProgressRecordParams_BrandAwareSourceURL to construct a minimal
ContentBlock JSON input, call parseCreateProgressRecordParams with BrandFeishu
and BrandLark, and assert the returned params/sourceURL equals
core.ResolveOpenBaseURL(core.BrandFeishu)+"/app" and
core.ResolveOpenBaseURL(core.BrandLark)+"/app" respectively; alternatively, if
you intend to keep it as a unit test of the helper, rename the test to reflect
core.ResolveOpenBaseURL behavior. Use the existing test name and function
parseCreateProgressRecordParams and the constants
core.BrandFeishu/core.BrandLark in your assertions to make the intent clear.
In `@shortcuts/okr/okr_progress_create.go`:
- Around line 166-186: The DryRun closure currently ignores errors from
parseCreateProgressRecordParams (p, _ :=
parseCreateProgressRecordParams(runtime)), which can lead to nil derefs when
using p.UserIDType, p.TargetID, etc.; update DryRun in okr_progress_create.go to
handle the error: call parseCreateProgressRecordParams and if err != nil return
a common.NewDryRunAPI() that describes the parse error (e.g., via Desc) so the
error is surfaced, or at minimum add a nil guard before dereferencing p and
return a descriptive DryRunAPI when p == nil; reference the DryRun function,
parseCreateProgressRecordParams, and common.NewDryRunAPI()/Desc to implement the
fix.
🪄 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: 23ec4734-2f30-49d0-b64e-17bb2cc6d9fd
📒 Files selected for processing (31)
README.mdREADME.zh.mdinternal/registry/service_descriptions.jsonshortcuts/okr/okr_cli_resp.goshortcuts/okr/okr_image_upload.goshortcuts/okr/okr_image_upload_test.goshortcuts/okr/okr_openapi.goshortcuts/okr/okr_openapi_test.goshortcuts/okr/okr_progress_create.goshortcuts/okr/okr_progress_create_test.goshortcuts/okr/okr_progress_delete.goshortcuts/okr/okr_progress_delete_test.goshortcuts/okr/okr_progress_get.goshortcuts/okr/okr_progress_get_test.goshortcuts/okr/okr_progress_list.goshortcuts/okr/okr_progress_list_test.goshortcuts/okr/okr_progress_update.goshortcuts/okr/okr_progress_update_test.goshortcuts/okr/shortcuts.goskills/lark-okr/SKILL.mdskills/lark-okr/references/lark-okr-contentblock.mdskills/lark-okr/references/lark-okr-cycle-detail.mdskills/lark-okr/references/lark-okr-cycle-list.mdskills/lark-okr/references/lark-okr-entities.mdskills/lark-okr/references/lark-okr-image-upload.mdskills/lark-okr/references/lark-okr-progress-create.mdskills/lark-okr/references/lark-okr-progress-delete.mdskills/lark-okr/references/lark-okr-progress-get.mdskills/lark-okr/references/lark-okr-progress-list.mdskills/lark-okr/references/lark-okr-progress-update.mdtests/cli_e2e/okr/okr_progress_test.go
✅ Files skipped from review due to trivial changes (13)
- skills/lark-okr/references/lark-okr-contentblock.md
- shortcuts/okr/shortcuts.go
- internal/registry/service_descriptions.json
- skills/lark-okr/references/lark-okr-cycle-detail.md
- shortcuts/okr/okr_progress_update_test.go
- skills/lark-okr/references/lark-okr-progress-list.md
- skills/lark-okr/references/lark-okr-progress-delete.md
- shortcuts/okr/okr_progress_get_test.go
- skills/lark-okr/references/lark-okr-progress-get.md
- shortcuts/okr/okr_progress_delete_test.go
- shortcuts/okr/okr_progress_list_test.go
- shortcuts/okr/okr_progress_create_test.go
- tests/cli_e2e/okr/okr_progress_test.go
🚧 Files skipped from review as they are similar to previous changes (5)
- skills/lark-okr/references/lark-okr-cycle-list.md
- shortcuts/okr/okr_progress_delete.go
- README.md
- skills/lark-okr/references/lark-okr-progress-create.md
- skills/lark-okr/SKILL.md
syh-cpdsss
force-pushed
the
feat/okr_dev
branch
from
2bfb322 to
61ff1a0
Compare
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-okr/references/lark-okr-entities.md (1)
7-15:⚠️ Potential issue | 🟡 MinorFix the tree connectors for the new progress-record nodes.
The added
ProgressRecordsiblings make both branches show multiple└──terminal children. Use├──for non-final siblings so the overview renders the intended hierarchy.🛠️ Proposed docs fix
Cycle (用户周期) └── Objective (目标) ├── KeyResult (关键结果) - │ └── Indicator (指标) + │ ├── Indicator (指标) │ └── list<ProgressRecord> (进展记录列表) - └── Indicator (指标) + ├── Indicator (指标) └── list<ProgressRecord> (进展记录列表)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@skills/lark-okr/references/lark-okr-entities.md` around lines 7 - 15, The ASCII-tree showing Cycle → Objective → KeyResult/Indicator has incorrect terminal connectors: both ProgressRecord siblings are using "└──" which makes multiple terminal children appear as final nodes; update the tree under Objective and under KeyResult so non-final children use "├──" and only the last child uses "└──" — adjust the lines for Indicator and list<ProgressRecord>/ProgressRecord nodes (references: Cycle, Objective, KeyResult, Indicator, list<ProgressRecord>, ProgressRecord) so the hierarchy renders correctly.
♻️ Duplicate comments (3)
README.zh.md (1)
15-15:⚠️ Potential issue | 🟡 MinorFix the inconsistent skill count.
Line 15 still says "22 个" but should be "23 个" to match the updated counts on Lines 9 and 16.
📝 Proposed fix
-- **为 Agent 原生设计** — 22 个 [Skills](./skills/) 开箱即用，适配主流 AI 工具,Agent 无需额外适配即可操作飞书 +- **为 Agent 原生设计** — 23 个 [Skills](./skills/) 开箱即用，适配主流 AI 工具,Agent 无需额外适配即可操作飞书🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@README.zh.md` at line 15, Update the inconsistent skill count in the README by changing the phrase "**为 Agent 原生设计** — 22 个 [Skills](./skills/) 开箱即用..." to use "23 个" so it matches the counts on Lines 9 and 16; locate the exact text snippet "**为 Agent 原生设计** — 22 个 [Skills](./skills/)" and replace "22" with "23".skills/lark-okr/references/lark-okr-entities.md (1)
320-328:⚠️ Potential issue | 🟡 MinorRestore the missing
okr +progress-listreference.Line 174 documents
okr +progress-list, but the bottom reference index still omits it, so readers cannot discover the list reference from this section.🛠️ Proposed docs fix
- [okr +progress-get](lark-okr-progress-get.md) — 获取进展记录 - [okr +progress-create](lark-okr-progress-create.md) — 创建进展记录 - [okr +progress-update](lark-okr-progress-update.md) — 更新进展记录 - [okr +progress-delete](lark-okr-progress-delete.md) — 删除进展记录 +- [okr +progress-list](lark-okr-progress-list.md) — 列出进展记录🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@skills/lark-okr/references/lark-okr-entities.md` around lines 320 - 328, Add the missing reference entry for "okr +progress-list" to the bottom reference list so the documented "okr +progress-list" (mentioned around line 174) is discoverable; follow the same bullet format as the other entries and add a line like "- [okr +progress-list](lark-okr-progress-list.md) — 列出进展记录" in the references block that contains "okr +progress-get", "okr +progress-create", etc., ensuring the link target uses the corresponding file name for the progress-list page.shortcuts/okr/okr_progress_create.go (1)
223-228:⚠️ Potential issue | 🟡 MinorRemove the internal
ProgressV1name from CLI output.The human-readable output still exposes the internal API model name. Use the domain label already used by the get/update shortcuts.
Suggested output text fix
- fmt.Fprintf(w, "Created ProgressV1 Record [%s]\n", resp.ID) + fmt.Fprintf(w, "Created Progress Record [%s]\n", resp.ID) fmt.Fprintf(w, " ModifyTime: %s\n", resp.ModifyTime) if resp.ProgressRate != nil && resp.ProgressRate.Percent != nil { - fmt.Fprintf(w, " ProgressV1: %.1f%%\n", *resp.ProgressRate.Percent) + fmt.Fprintf(w, " Progress: %.1f%%\n", *resp.ProgressRate.Percent) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@shortcuts/okr/okr_progress_create.go` around lines 223 - 228, The CLI output is leaking the internal model name "ProgressV1"; update the runtime.OutFormat block that prints the created record (referencing runtime.OutFormat, resp.ID, resp.ModifyTime, resp.ProgressRate) to remove "ProgressV1" and use the domain label used by the get/update shortcuts (e.g., "Progress" or "Progress Record") instead; change the fmt.Fprintf that currently prints "Created ProgressV1 Record [%s]\n" to the domain-friendly text and keep the existing ModifyTime and ProgressRate formatting unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@shortcuts/okr/okr_openapi_test.go`:
- Around line 290-312: Extend TestContentBlockV1JSON to include a gallery
ContentBlockElementV1 that contains a ContentGalleryV1 with at least one
ContentImageItemV1 so JSON serialization asserts the expected V1 keys (use the
struct field that maps to "fileToken" rather than "file_token"); specifically
add an element referencing ContentGalleryV1 and a ContentImageItemV1 (with its
FileToken set to a test value) and assert the marshaled JSON contains "gallery",
"imageItem" (or the gallery/item key used), and "fileToken" and the token string
to prevent regression of the JSON tag behavior for ContentImageItemV1.
In `@shortcuts/okr/okr_progress_create.go`:
- Around line 60-63: The percent parsing currently allows huge and negative
values; change the validation in the runtime.Str("progress-percent") parsing
paths to enforce the documented 0–100 range: after strconv.ParseFloat(v, 64)
keep the existing NaN/Inf checks but replace percent < -99999999999 || percent >
99999999999 with percent < 0 || percent > 100 and return an error message like
"--progress-percent must be a number between 0 and 100". Apply the same change
to the other parsing branches that set progress_rate.percent so all assignments
to progress_rate.percent validate 0–100 consistently.
In `@shortcuts/okr/okr_progress_update.go`:
- Around line 37-40: The progress-percent parsing currently allows negative and
>100 values; update the validation in the runtime.Str("progress-percent")
parsing branch (and the other occurrences handling the parsed percent at the
other two locations) to keep the NaN/Inf guards but enforce percent >= 0 &&
percent <= 100, and return the FlagErrorf with a corrected message like
"--progress-percent must be a number between 0 and 100" when out of range; look
for the percent variable and strconv.ParseFloat usage and replace the current
extreme bounds check with the 0–100 range check for all three occurrences.
In `@skills/lark-okr/references/lark-okr-entities.md`:
- Around line 308-316: Add the image-upload OAuth scope row to the scopes table:
insert a row for `okr:okr.progress.file:upload` with permission type "写" and
description like "上传/附加图像或文件到进展记录 (用于 okr +upload-image)"; update any
surrounding ordering/formatting so the table remains aligned and consistent with
other `okr:okr.progress:*` entries (reference the existing table and rows such
as `okr:okr.progress:readonly`, `okr:okr.progress:writeonly`, and
`okr:okr.progress:delete` to place this new scope).
---
Outside diff comments:
In `@skills/lark-okr/references/lark-okr-entities.md`:
- Around line 7-15: The ASCII-tree showing Cycle → Objective →
KeyResult/Indicator has incorrect terminal connectors: both ProgressRecord
siblings are using "└──" which makes multiple terminal children appear as final
nodes; update the tree under Objective and under KeyResult so non-final children
use "├──" and only the last child uses "└──" — adjust the lines for Indicator
and list<ProgressRecord>/ProgressRecord nodes (references: Cycle, Objective,
KeyResult, Indicator, list<ProgressRecord>, ProgressRecord) so the hierarchy
renders correctly.
---
Duplicate comments:
In `@README.zh.md`:
- Line 15: Update the inconsistent skill count in the README by changing the
phrase "**为 Agent 原生设计** — 22 个 [Skills](./skills/) 开箱即用..." to use "23 个" so it
matches the counts on Lines 9 and 16; locate the exact text snippet "**为 Agent
原生设计** — 22 个 [Skills](./skills/)" and replace "22" with "23".
In `@shortcuts/okr/okr_progress_create.go`:
- Around line 223-228: The CLI output is leaking the internal model name
"ProgressV1"; update the runtime.OutFormat block that prints the created record
(referencing runtime.OutFormat, resp.ID, resp.ModifyTime, resp.ProgressRate) to
remove "ProgressV1" and use the domain label used by the get/update shortcuts
(e.g., "Progress" or "Progress Record") instead; change the fmt.Fprintf that
currently prints "Created ProgressV1 Record [%s]\n" to the domain-friendly text
and keep the existing ModifyTime and ProgressRate formatting unchanged.
In `@skills/lark-okr/references/lark-okr-entities.md`:
- Around line 320-328: Add the missing reference entry for "okr +progress-list"
to the bottom reference list so the documented "okr +progress-list" (mentioned
around line 174) is discoverable; follow the same bullet format as the other
entries and add a line like "- [okr +progress-list](lark-okr-progress-list.md) —
列出进展记录" in the references block that contains "okr +progress-get", "okr
+progress-create", etc., ensuring the link target uses the corresponding file
name for the progress-list page.
🪄 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: 0d654077-33fe-42b3-b20d-dc961623e9ae
📒 Files selected for processing (31)
README.mdREADME.zh.mdinternal/registry/service_descriptions.jsonshortcuts/okr/okr_cli_resp.goshortcuts/okr/okr_image_upload.goshortcuts/okr/okr_image_upload_test.goshortcuts/okr/okr_openapi.goshortcuts/okr/okr_openapi_test.goshortcuts/okr/okr_progress_create.goshortcuts/okr/okr_progress_create_test.goshortcuts/okr/okr_progress_delete.goshortcuts/okr/okr_progress_delete_test.goshortcuts/okr/okr_progress_get.goshortcuts/okr/okr_progress_get_test.goshortcuts/okr/okr_progress_list.goshortcuts/okr/okr_progress_list_test.goshortcuts/okr/okr_progress_update.goshortcuts/okr/okr_progress_update_test.goshortcuts/okr/shortcuts.goskills/lark-okr/SKILL.mdskills/lark-okr/references/lark-okr-contentblock.mdskills/lark-okr/references/lark-okr-cycle-detail.mdskills/lark-okr/references/lark-okr-cycle-list.mdskills/lark-okr/references/lark-okr-entities.mdskills/lark-okr/references/lark-okr-image-upload.mdskills/lark-okr/references/lark-okr-progress-create.mdskills/lark-okr/references/lark-okr-progress-delete.mdskills/lark-okr/references/lark-okr-progress-get.mdskills/lark-okr/references/lark-okr-progress-list.mdskills/lark-okr/references/lark-okr-progress-update.mdtests/cli_e2e/okr/okr_progress_test.go
✅ Files skipped from review due to trivial changes (17)
- README.md
- skills/lark-okr/references/lark-okr-contentblock.md
- internal/registry/service_descriptions.json
- skills/lark-okr/references/lark-okr-cycle-detail.md
- shortcuts/okr/okr_progress_delete.go
- skills/lark-okr/references/lark-okr-progress-delete.md
- shortcuts/okr/shortcuts.go
- skills/lark-okr/references/lark-okr-progress-list.md
- skills/lark-okr/references/lark-okr-progress-get.md
- skills/lark-okr/references/lark-okr-progress-create.md
- skills/lark-okr/references/lark-okr-cycle-list.md
- shortcuts/okr/okr_progress_delete_test.go
- shortcuts/okr/okr_progress_list_test.go
- tests/cli_e2e/okr/okr_progress_test.go
- shortcuts/okr/okr_progress_list.go
- shortcuts/okr/okr_progress_create_test.go
- shortcuts/okr/okr_cli_resp.go
🚧 Files skipped from review as they are similar to previous changes (3)
- skills/lark-okr/SKILL.md
- shortcuts/okr/okr_progress_update_test.go
- shortcuts/okr/okr_image_upload_test.go
syh-cpdsss
force-pushed
the
feat/okr_dev
branch
3 times, most recently
from
8bcab00 to
556e0b2
Compare
Change-Id: I06878082fb42e341bb807dac91732489708700d1
syh-cpdsss
force-pushed
the
feat/okr_dev
branch
from
556e0b2 to
dc49fef
Compare
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
Add OKR progress record management shortcuts to enable creating, reading, updating, deleting, and listing progress records for objectives and key results.
Also includes image upload support for rich text content in progress records.
Changes
+progress-createshortcut to create progress records for objectives/key results+progress-getshortcut to retrieve a single progress record by ID+progress-listshortcut to list all progress records for an objective or key result+progress-updateshortcut to update progress record content+progress-deleteshortcut to delete progress records+upload-imageshortcut to upload images for use in progress record rich textTest Plan
go test ./shortcuts/okr/...)LARK_CLI_BIN=/tmp/lark-cli go test ./tests/cli_e2e/okr/...)go vet ./shortcuts/okr/...passesRelated Issues
Summary by CodeRabbit
New Features
Documentation
Tests