Merged
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 an OKR domain to the CLI: new shortcuts Changes
Sequence DiagramsequenceDiagram
participant User as "User / CLI"
participant CLI as "lark-cli OKR shortcuts"
participant API as "Lark OKR API"
participant Pager as "Pagination / Aggregator"
User->>CLI: run `okr +cycle-detail --cycle-id=123 --dry-run`
CLI->>API: GET /open-apis/okr/v2/cycles/123/objectives?page_size=100
API-->>CLI: returns page (items, has_more, page_token)
CLI->>Pager: aggregate objectives
alt objectives present
loop each objective
CLI->>API: GET /open-apis/okr/v2/objectives/{id}/key_results?page_size=100
API-->>CLI: returns KR page(s)
CLI->>Pager: collect & paginate key_results
end
end
CLI->>CLI: convert domain -> Resp structs (ToResp), marshal output
CLI-->>User: print JSON result (cycle_id, objectives[], total)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 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 |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #522 +/- ##
==========================================
+ Coverage 59.05% 59.23% +0.18%
==========================================
Files 384 388 +4
Lines 32632 32956 +324
==========================================
+ Hits 19270 19521 +251
- Misses 11553 11605 +52
- Partials 1809 1830 +21 ☔ 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@3ff3041c2c51f4072d8ddd20491e7ccab94870c0🧩 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: 8
🧹 Nitpick comments (3)
shortcuts/okr/okr_cli_resp_test.go (1)
12-34: Consider testing response contract instead of instantiation only.These assertions will pass even if JSON field tags/serialization regress. Add marshal/unmarshal checks for expected keys.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@shortcuts/okr/okr_cli_resp_test.go` around lines 12 - 34, Replace the current instantiation-only assertions with a serialization contract test: for each response struct (RespCycle, RespObjective, RespKeyResult) marshal an instance to JSON (json.Marshal), assert the resulting JSON contains the expected field keys (e.g. "id" or whatever the struct's json tags define), then json.Unmarshal the JSON back into the struct and assert fields match the original values; update the TestRespStructs helper to perform these marshal/unmarshal checks for each of RespCycle, RespObjective and RespKeyResult so regressions in json tags/serialization are caught.shortcuts/okr/shortcuts_test.go (1)
12-16: Strengthen this test to assert exact shortcut membership.
len(list) > 0won’t catch accidental loss of one of the two OKR commands. Assert expected commands/services explicitly.Proposed test improvement
func TestShortcutsRegistration(t *testing.T) { convey.Convey("Shortcuts() returns all commands", t, func() { list := Shortcuts() - convey.So(len(list), convey.ShouldBeGreaterThan, 0) + convey.So(list, convey.ShouldHaveLength, 2) + convey.So(list[0].Service, convey.ShouldEqual, "okr") + convey.So(list[1].Service, convey.ShouldEqual, "okr") + convey.So([]string{list[0].Command, list[1].Command}, convey.ShouldResemble, []string{ + "+cycle-list", + "+cycle-detail", + }) }) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@shortcuts/okr/shortcuts_test.go` around lines 12 - 16, Replace the weak length check in TestShortcutsRegistration with explicit membership assertions: call Shortcuts() into list (as currently done) and then assert that list contains the exact expected OKR shortcut entries (e.g., by constructing an expected slice of command identifiers and comparing as a set or using convey.ShouldContain / convey.ShouldResemble to verify presence/exact match), so that any accidental removal of one of the two OKR commands is caught.shortcuts/okr/okr_cli_resp.go (1)
64-64:omitemptywon't omit these optional nested structs.In Go,
omitemptydoes not suppress zero-valued non-pointer struct fields.unit,style,text_run,docs_link,mention,back_color,text_color, andlinkwill still marshal as empty objects unless these fields become pointers. If they are meant to be optional in the response schema, switch them to*Resp...and only set them when present.Also applies to: 168-198
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@shortcuts/okr/okr_cli_resp.go` at line 64, The struct fields declared as non-pointer structs (e.g. Unit RespIndicatorUnit `json:"unit,omitempty"` and the other fields style, text_run, docs_link, mention, back_color, text_color, link) must be changed to pointer types so omitempty actually omits them; update declarations such as RespIndicatorUnit -> *RespIndicatorUnit (and RespIndicatorStyle, RespTextRun, RespDocsLink, RespMention, back/text color and link types) in the same struct(s) and ensure any code that constructs these structs (constructors, unmarshallers, tests) sets these fields to nil when absent and allocates (&Type{...}) when present so JSON marshaling will omit unset optional nested fields.
🤖 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_cli_resp.go`:
- Around line 37-48: The JSON omitempty on non-pointer numeric fields causes
legitimate zero values to be omitted; update the listed structs so fields that
may legitimately be zero are pointer types (e.g. change RespCycle.Score to
*float64; change RespIndicator.StartValue, TargetValue, CurrentValue to *int32
or *float64 as appropriate; change RespKeyResult.Score and Weight to pointer
types; change RespObjective.Score and Weight to pointers; change numeric fields
in RespContentColor and RespContentImageItem Width/Height to pointer types) OR
remove `omitempty` where you want zeros to always serialize—apply these changes
to the structs RespCycle, RespIndicator, RespKeyResult, RespObjective,
RespContentColor, RespContentImageItem and any similar content structs to ensure
consumers can distinguish "not set" vs "set to 0".
In `@shortcuts/okr/okr_cycle_detail_test.go`:
- Around line 289-401: Test covers paginated objectives but not paginated
key_results, so exercise the has_more/page_token branch in
shortcuts/okr/okr_cycle_detail.go by adding paginated stubs for each objective's
key_results: for each of the endpoints
"/open-apis/okr/v2/objectives/obj-p1/key_results" and
"/open-apis/okr/v2/objectives/obj-p2/key_results" register at least two
httpmock.Stub responses — first response with "has_more": true and a
"page_token" that contains one or more key_result items, then a second response
for the same URL that returns the remaining items with "has_more": false (no
page_token) — this will ensure TestCycleDetailExecute_Pagination (function
TestCycleDetailExecute_Pagination) consumes multiple pages and verifies
aggregation across pages.
- Around line 132-150: The unit test TestCycleDetailDryRun covers the shortcut
harness but you must add a real CLI E2E dry-run test for the new shortcut;
create a new test under tests/cli_e2e/dryrun (or the okr domain subfolder) that
invokes the real CLI binary with the arguments
["+cycle-detail","--cycle-id","456","--dry-run"] and asserts the captured stdout
contains "456" and the API path "/open-apis/okr/v2/cycles/456/objectives";
mirror expectations from TestCycleDetailDryRun and reuse the same cycle id so
the E2E verifies the actual CLI path rather than only runCycleDetailShortcut.
In `@shortcuts/okr/okr_cycle_list_test.go`:
- Around line 196-233: The unit tests TestCycleListDryRun and
TestCycleListDryRun_WithTimeRange only exercise cmdutil.TestFactory and miss the
repository-level dry-run E2E required for new shortcuts; add a new E2E test that
runs the installed CLI command for the shortcut (invoke "okr +cycle-list
--dry-run" with the same flags used in TestCycleListDryRun and
TestCycleListDryRun_WithTimeRange—e.g., --user-id/--user-id-type and
--time-range) under the project’s dry-run E2E suite so the installed-command
wiring and environment contract are validated; ensure the new test asserts the
dry-run output includes the user id and the API path "/open-apis/okr/v2/cycles"
and mirrors the scenarios covered by TestCycleListDryRun and
TestCycleListDryRun_WithTimeRange.
In `@shortcuts/okr/okr_openapi.go`:
- Around line 401-408: The ToString method on type FromEntityType is missing the
case for FromEntityTypeKeyResult so conversions of key-result alignments produce
an empty string; update FromEntityType.ToString() to handle the
FromEntityTypeKeyResult branch (returning the canonical string, e.g.,
"key_result") and ensure Alignment.ToResp() will receive the proper value, then
add a regression test that constructs an alignment with FromEntityTypeKeyResult
and asserts the produced response contains from_entity_type == "key_result".
In `@shortcuts/okr/shortcuts.go`:
- Line 10: Update the stale doc comment for the Shortcuts function to reflect
the OKR package behavior: replace "Shortcuts returns all whiteboard shortcuts."
with a correct description such as "Shortcuts returns all OKR shortcuts." so the
function comment matches the package semantics and tooling (e.g., godoc) for the
Shortcuts function.
In `@skills/lark-okr/references/lark-okr-cycle-detail.md`:
- Line 3: Update the broken prerequisite link in
skills/lark-okr/references/lark-okr-cycle-detail.md by changing the relative
path target from ../lark-shared/SKILL.md to ../../lark-shared/SKILL.md so the
reference resolves correctly from the current directory; locate the line
containing "**前置条件：** 请先阅读 `../lark-shared/SKILL.md`" and replace the path
accordingly.
In `@skills/lark-okr/references/lark-okr-cycle-list.md`:
- Line 3: Update the broken shared-skill link and remove the stray fragment:
replace occurrences of the relative path string "../lark-shared/SKILL.md" in
lark-okr-cycle-list.md with the correct two-level-up path
"../../lark-shared/SKILL.md", and remove the dangling text "invalid 状态。" (and
any identical editing artifact) from the file so the link points to the valid
shared SKILL.md and there is no leftover fragment.
---
Nitpick comments:
In `@shortcuts/okr/okr_cli_resp_test.go`:
- Around line 12-34: Replace the current instantiation-only assertions with a
serialization contract test: for each response struct (RespCycle, RespObjective,
RespKeyResult) marshal an instance to JSON (json.Marshal), assert the resulting
JSON contains the expected field keys (e.g. "id" or whatever the struct's json
tags define), then json.Unmarshal the JSON back into the struct and assert
fields match the original values; update the TestRespStructs helper to perform
these marshal/unmarshal checks for each of RespCycle, RespObjective and
RespKeyResult so regressions in json tags/serialization are caught.
In `@shortcuts/okr/okr_cli_resp.go`:
- Line 64: The struct fields declared as non-pointer structs (e.g. Unit
RespIndicatorUnit `json:"unit,omitempty"` and the other fields style, text_run,
docs_link, mention, back_color, text_color, link) must be changed to pointer
types so omitempty actually omits them; update declarations such as
RespIndicatorUnit -> *RespIndicatorUnit (and RespIndicatorStyle, RespTextRun,
RespDocsLink, RespMention, back/text color and link types) in the same struct(s)
and ensure any code that constructs these structs (constructors, unmarshallers,
tests) sets these fields to nil when absent and allocates (&Type{...}) when
present so JSON marshaling will omit unset optional nested fields.
In `@shortcuts/okr/shortcuts_test.go`:
- Around line 12-16: Replace the weak length check in TestShortcutsRegistration
with explicit membership assertions: call Shortcuts() into list (as currently
done) and then assert that list contains the exact expected OKR shortcut entries
(e.g., by constructing an expected slice of command identifiers and comparing as
a set or using convey.ShouldContain / convey.ShouldResemble to verify
presence/exact match), so that any accidental removal of one of the two OKR
commands is caught.
🪄 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: 1d57843b-dba3-40db-bfdf-c875a8bf785a
📒 Files selected for processing (18)
README.mdREADME.zh.mdinternal/registry/service_descriptions.jsonshortcuts/okr/okr_cli_resp.goshortcuts/okr/okr_cli_resp_test.goshortcuts/okr/okr_cycle_detail.goshortcuts/okr/okr_cycle_detail_test.goshortcuts/okr/okr_cycle_list.goshortcuts/okr/okr_cycle_list_test.goshortcuts/okr/okr_openapi.goshortcuts/okr/okr_openapi_test.goshortcuts/okr/shortcuts.goshortcuts/okr/shortcuts_test.goshortcuts/register.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.md
syh-cpdsss
force-pushed
the
feat/okr_dev
branch
from
128c3ff to
bed3708
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
tests/cli_e2e/okr/okr_cycle_list_test.go (1)
20-57:⚠️ Potential issue | 🟠 MajorSame env-var / config-isolation gap as
okr_cycle_detail_test.go.Both dry-run subtests need
LARKSUITE_CLI_APP_ID,APP_SECRET,BRAND, and a tempLARKSUITE_CLI_CONFIG_DIRper coding guidelines. See the comment onokr_cycle_detail_test.gofor the fix pattern; apply it toTestOKR_CycleListDryRunandTestOKR_CycleListDryRun_WithTimeRange(or factor into a shared helper).As per coding guidelines: "Dry-run E2E tests must set env vars
LARKSUITE_CLI_APP_ID,APP_SECRET,BRANDand use--dry-runflag".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/cli_e2e/okr/okr_cycle_list_test.go` around lines 20 - 57, Both dry-run tests are missing required env var/config isolation; set LARKSUITE_CLI_APP_ID, APP_SECRET, BRAND and a temp LARKSUITE_CLI_CONFIG_DIR before invoking clie2e.RunCmd in TestOKR_CycleListDryRun and TestOKR_CycleListDryRun_WithTimeRange (or factor into a shared helper); create a temp dir via t.TempDir() and export it as LARKSUITE_CLI_CONFIG_DIR and use t.Setenv (or os.Setenv) to set LARKSUITE_CLI_APP_ID, APP_SECRET and BRAND so the tests run isolated and satisfy the "Dry-run E2E tests must set env vars" guideline.
🧹 Nitpick comments (5)
shortcuts/okr/okr_openapi.go (1)
249-260: Consider treating"0"as an unset timestamp.If the upstream OKR API returns
"0"for unset/nullable timestamp fields (not uncommon in Lark open-APIs),formatTimestamp("0")currently renders as"1970-01-01 08:00:00"(in UTC+8) /"1970-01-01 00:00:00"(UTC), which looks like valid data. A tiny guard would avoid user confusion:♻️ Suggested tweak
func formatTimestamp(ts string) string { - if ts == "" { + if ts == "" || ts == "0" { return "" } millis, err := strconv.ParseInt(ts, 10, 64) if err != nil { return ts } + if millis <= 0 { + return "" + } t := time.UnixMilli(millis) return t.Format("2006-01-02 15:04:05") }Skip if you've confirmed the upstream API never returns
"0"for these fields.🤖 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 249 - 260, The formatTimestamp function currently formats "0" as the Unix epoch; update formatTimestamp to treat ts == "0" as an unset value by returning an empty string early (similar to the existing empty check) before parsing, so calls to formatTimestamp("0") will return "" instead of a 1970 timestamp.shortcuts/okr/okr_cycle_detail.go (3)
95-141: N+1 fetch pattern: consider capping work or parallelizing bounded.For a cycle with
Nobjectives, this issuesNsequential paginated key-result fetches, each enforcing a 500ms inter-page sleep. A moderately sized cycle (e.g., 50 objectives × 2 pages) spends ≥25s purely in sleeps before any rate-limiter even fires. Options:
- Skip the inter-page sleep when only one page has been fetched (the first page never needs pre-sleep — you already do this via
krPage > 0, but the 500ms still fires between page 1 and page 2 of every objective).- Run the per-objective KR fetches with a small bounded
errgroup(e.g., concurrency 4–8) while still respecting thectx.- Expose a
--max-objectivesor--limitflag for users who only need a preview.Not a correctness issue — flagging for UX/performance follow-up.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@shortcuts/okr/okr_cycle_detail.go` around lines 95 - 141, The per-objective fetch loop (loop over objectives using obj := &objectives[i], krPage, time.Sleep(500 * time.Millisecond), runtime.DoAPIJSON) causes an N+1 sequential delay; fix by running the per-objective key-result pagination in a bounded parallel worker set (use a context-aware errgroup or worker pool with concurrency 4–8) so each objective’s pagination (the inner loop that updates krQuery, calls runtime.DoAPIJSON, unmarshals items into keyResults, and checks ctx.Err()) runs concurrently but limited; also remove or change the unconditional 500ms pre-page sleep to only apply when retry/rate-limit metadata indicates a need (or apply it per-worker between pages), and ensure ctx cancellation is respected inside each worker and when aggregating respObjectives.
163-171: Text fallback dumps raw JSON forContent/Notes.
RespObjective.Content/RespKeyResult.Contentare JSON-serializedContentBlockstrings (perokr_openapi.goToResp). Printing them viaptrStrin the pretty-text fallback will dump a blob like{"blocks":[{"block_element_type":"paragraph",...}]}on stdout, which is noisy for humans. Consider a tiny helper that renders only the text runs (joined by spaces) for the non-JSON format path, or at minimum truncate/ellipsize.Non-blocking; JSON output format is unaffected.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@shortcuts/okr/okr_cycle_detail.go` around lines 163 - 171, The pretty-text fallback is printing raw JSON for RespObjective.Content and RespKeyResult.Content because the code uses ptrStr(o.Content) inside the runtime.OutFormat writer lambda; implement a small helper (e.g., renderContentText(content *string) string) that parses the ContentBlock JSON (or extracts text runs) and returns a human-friendly string (joined runs or a truncated/ellipsized fallback on parse error), then replace ptrStr(o.Content) and ptrStr(kr.Content) (and ptrStr(o.Notes) if Notes contains JSON) in the anonymous function with calls to renderContentText to produce readable text in the non-JSON output. Ensure helper is resilient to nil and invalid JSON and keeps existing numeric formatting (ptrFloat64, etc.) unchanged.
75-86: Silently dropping items onjson.Marshal/Unmarshalfailure can hide API schema drift.If the upstream OKR API evolves (adds a new enum value, changes a field type, etc.), affected items are dropped from results with no signal to the user — results will appear incomplete without any indication why. The same pattern exists in
okr_cycle_list.go(lines 140–151) for cycles and again for key results below (lines 124–134).Consider logging a warning to stderr per skipped item (or at least an aggregated count), e.g. via
runtime.Warnf/ stderr logger used elsewhere in the codebase, so the user/agent can tell real data was discarded.♻️ Suggested handling (per-loop)
- for _, item := range itemsRaw { - raw, err := json.Marshal(item) - if err != nil { - continue - } - var obj Objective - if err := json.Unmarshal(raw, &obj); err != nil { - continue - } - objectives = append(objectives, obj) - } + skipped := 0 + for _, item := range itemsRaw { + raw, err := json.Marshal(item) + if err != nil { + skipped++ + continue + } + var obj Objective + if err := json.Unmarshal(raw, &obj); err != nil { + skipped++ + continue + } + objectives = append(objectives, obj) + } + if skipped > 0 { + fmt.Fprintf(os.Stderr, "warning: skipped %d objective item(s) that failed to decode on page %d\n", skipped, page) + }As per coding guidelines: "Program output (JSON envelopes) must go to stdout; progress, warnings, and hints must go to stderr".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@shortcuts/okr/okr_cycle_detail.go` around lines 75 - 86, The loop that decodes items (itemsRaw -> json.Marshal/json.Unmarshal into Objective and appends to objectives) silently drops entries on Marshal/Unmarshal errors; change the behavior to log a warning to stderr (use runtime.Warnf or the project's stderr logger) when an item is skipped, including the error and identifying context (index or the raw item), and optionally track a dropped counter and emit an aggregated warning after the loop; apply the same pattern used for Objective decoding in okr_cycle_detail.go to the analogous parsing code in okr_cycle_list.go and the key-result parsing blocks so schema-drift or malformed items are visible.shortcuts/okr/okr_cycle_list.go (1)
44-54:cycleOverlapssilently excludes cycles with unparseable timestamps.If
cycle.StartTimeorcycle.EndTimecan't be parsed as millisecond epoch, the cycle is dropped from the filtered set with no signal. In practice the upstream API should always return numeric strings, but combined with the silent unmarshal skip in the loop above, a user with a malformed cycle gets no indication why their cycle doesn't appear.Consider either (a) including such cycles in the filtered set (fail-open when
--time-rangeis specified but timestamps are invalid) or (b) emitting a stderr warning identifying the offending cycle ID.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@shortcuts/okr/okr_cycle_list.go` around lines 44 - 54, cycleOverlaps currently silently drops cycles when cycle.StartTime or cycle.EndTime fail to parse; update cycleOverlaps to both surface the error and fail-open: when strconv.ParseInt on Cycle.StartTime or Cycle.EndTime returns an error, write a concise warning to stderr identifying the affected Cycle (e.g., Cycle.ID) and the parse error, then return true so the cycle is included in the filtered set; keep the existing logic for successfully parsed timestamps. This change touches the cycleOverlaps function and the Cycle.StartTime/EndTime handling.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tests/cli_e2e/okr/okr_cycle_detail_test.go`:
- Around line 18-35: The test TestOKR_CycleDetailDryRun calls clie2e.RunCmd with
--dry-run but never sets the required environment variables, causing dry-run
validation to fail; fix it by setting the same dry-run env config before
invoking clie2e.RunCmd (e.g., call the existing helper setDryRunConfigEnv(t) or
add equivalent code to export LARKSUITE_CLI_APP_ID, LARKSUITE_CLI_APP_SECRET and
LARKSUITE_CLI_BRAND) so the dry-run path in clie2e.RunCmd can validate without
contacting real APIs.
---
Duplicate comments:
In `@tests/cli_e2e/okr/okr_cycle_list_test.go`:
- Around line 20-57: Both dry-run tests are missing required env var/config
isolation; set LARKSUITE_CLI_APP_ID, APP_SECRET, BRAND and a temp
LARKSUITE_CLI_CONFIG_DIR before invoking clie2e.RunCmd in
TestOKR_CycleListDryRun and TestOKR_CycleListDryRun_WithTimeRange (or factor
into a shared helper); create a temp dir via t.TempDir() and export it as
LARKSUITE_CLI_CONFIG_DIR and use t.Setenv (or os.Setenv) to set
LARKSUITE_CLI_APP_ID, APP_SECRET and BRAND so the tests run isolated and satisfy
the "Dry-run E2E tests must set env vars" guideline.
---
Nitpick comments:
In `@shortcuts/okr/okr_cycle_detail.go`:
- Around line 95-141: The per-objective fetch loop (loop over objectives using
obj := &objectives[i], krPage, time.Sleep(500 * time.Millisecond),
runtime.DoAPIJSON) causes an N+1 sequential delay; fix by running the
per-objective key-result pagination in a bounded parallel worker set (use a
context-aware errgroup or worker pool with concurrency 4–8) so each objective’s
pagination (the inner loop that updates krQuery, calls runtime.DoAPIJSON,
unmarshals items into keyResults, and checks ctx.Err()) runs concurrently but
limited; also remove or change the unconditional 500ms pre-page sleep to only
apply when retry/rate-limit metadata indicates a need (or apply it per-worker
between pages), and ensure ctx cancellation is respected inside each worker and
when aggregating respObjectives.
- Around line 163-171: The pretty-text fallback is printing raw JSON for
RespObjective.Content and RespKeyResult.Content because the code uses
ptrStr(o.Content) inside the runtime.OutFormat writer lambda; implement a small
helper (e.g., renderContentText(content *string) string) that parses the
ContentBlock JSON (or extracts text runs) and returns a human-friendly string
(joined runs or a truncated/ellipsized fallback on parse error), then replace
ptrStr(o.Content) and ptrStr(kr.Content) (and ptrStr(o.Notes) if Notes contains
JSON) in the anonymous function with calls to renderContentText to produce
readable text in the non-JSON output. Ensure helper is resilient to nil and
invalid JSON and keeps existing numeric formatting (ptrFloat64, etc.) unchanged.
- Around line 75-86: The loop that decodes items (itemsRaw ->
json.Marshal/json.Unmarshal into Objective and appends to objectives) silently
drops entries on Marshal/Unmarshal errors; change the behavior to log a warning
to stderr (use runtime.Warnf or the project's stderr logger) when an item is
skipped, including the error and identifying context (index or the raw item),
and optionally track a dropped counter and emit an aggregated warning after the
loop; apply the same pattern used for Objective decoding in okr_cycle_detail.go
to the analogous parsing code in okr_cycle_list.go and the key-result parsing
blocks so schema-drift or malformed items are visible.
In `@shortcuts/okr/okr_cycle_list.go`:
- Around line 44-54: cycleOverlaps currently silently drops cycles when
cycle.StartTime or cycle.EndTime fail to parse; update cycleOverlaps to both
surface the error and fail-open: when strconv.ParseInt on Cycle.StartTime or
Cycle.EndTime returns an error, write a concise warning to stderr identifying
the affected Cycle (e.g., Cycle.ID) and the parse error, then return true so the
cycle is included in the filtered set; keep the existing logic for successfully
parsed timestamps. This change touches the cycleOverlaps function and the
Cycle.StartTime/EndTime handling.
In `@shortcuts/okr/okr_openapi.go`:
- Around line 249-260: The formatTimestamp function currently formats "0" as the
Unix epoch; update formatTimestamp to treat ts == "0" as an unset value by
returning an empty string early (similar to the existing empty check) before
parsing, so calls to formatTimestamp("0") will return "" instead of a 1970
timestamp.
🪄 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: 0b45f4a2-ac15-4785-9351-d4e5776aab7b
📒 Files selected for processing (19)
README.mdREADME.zh.mdinternal/registry/service_descriptions.jsonshortcuts/okr/okr_cli_resp.goshortcuts/okr/okr_cycle_detail.goshortcuts/okr/okr_cycle_detail_test.goshortcuts/okr/okr_cycle_list.goshortcuts/okr/okr_cycle_list_test.goshortcuts/okr/okr_openapi.goshortcuts/okr/okr_openapi_test.goshortcuts/okr/shortcuts.goshortcuts/okr/shortcuts_test.goshortcuts/register.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.mdtests/cli_e2e/okr/okr_cycle_detail_test.gotests/cli_e2e/okr/okr_cycle_list_test.go
✅ Files skipped from review due to trivial changes (10)
- README.zh.md
- README.md
- internal/registry/service_descriptions.json
- shortcuts/okr/shortcuts_test.go
- skills/lark-okr/references/lark-okr-cycle-detail.md
- skills/lark-okr/SKILL.md
- skills/lark-okr/references/lark-okr-contentblock.md
- skills/lark-okr/references/lark-okr-cycle-list.md
- shortcuts/okr/okr_cycle_detail_test.go
- shortcuts/okr/okr_cycle_list_test.go
🚧 Files skipped from review as they are similar to previous changes (2)
- shortcuts/register.go
- shortcuts/okr/okr_openapi_test.go
syh-cpdsss
force-pushed
the
feat/okr_dev
branch
2 times, most recently
from
76a52d6 to
77fae8a
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
shortcuts/okr/okr_openapi.go (2)
235-246: Timestamp formatting uses local timezone without offset — output is ambiguous and non-reproducible.
time.UnixMilli(millis)returns atime.Timein the local timezone, and the layout"2006-01-02 15:04:05"emits no offset. CLI output will therefore differ depending on the machine'sTZenvironment (or the container's configured timezone), and consumers cannot tell which zone the timestamps are in. For a machine-parseable JSON envelope likeRespCycle.CreateTime/EndTime, prefer a zoned or UTC representation (e.g. RFC3339).♻️ Suggested fix
- t := time.UnixMilli(millis) - return t.Format("2006-01-02 15:04:05") + t := time.UnixMilli(millis).UTC() + return t.Format(time.RFC3339)🤖 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 235 - 246, The formatTimestamp function emits a local-time string without zone info making outputs ambiguous; update formatTimestamp to convert the parsed milliseconds to UTC (or include zone offset) and emit a machine-parseable format such as RFC3339 so RespCycle.CreateTime/EndTime are reproducible across hosts: parse millis with strconv.ParseInt as before, call time.UnixMilli(millis).UTC() (or use .In(...) if a specific zone is needed) and Format(time.RFC3339), preserving the same empty-string and error-return behavior otherwise.
289-295: Silently droppingjson.Marshalerrors onContentBlockserialization.When
json.Marshal(k.Content)/json.Marshal(o.Content)/json.Marshal(o.Notes)fails, the error is discarded and the correspondingcontent/notesfield is simply omitted from the response, with no log and no signal to the caller. In practicejson.Marshalon these structs shouldn't fail, but if an unexpected value (e.g. aNaNfloat sneaks into a future field) ever triggers an error, the response will look like the data legitimately had no content, masking a real bug. Consider at least logging at debug/warn level to stderr, or returning the error.Also applies to: 319-332
🤖 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 289 - 295, The current serialization of ContentBlock silently ignores json.Marshal errors (see the k.Content block and the similar o.Content / o.Notes usages), so update these sites to handle errors instead of dropping them: when calling json.Marshal(k.Content) (and the corresponding json.Marshal(o.Content) / json.Marshal(o.Notes)), check for err != nil and either return the error to the caller from the enclosing function or at minimum log the error (with context identifying the field and record) before continuing; ensure the same change is applied to the other occurrences referenced (the block around o.Content/o.Notes) so failures are not silently masked.shortcuts/okr/okr_cycle_detail.go (1)
95-155: N+1 API calls executed strictly sequentially — latency scales linearly with cycle size.Fetching key results for each objective is done one objective at a time, with an additional 500 ms gap between every KR page. A cycle with, say, 50 objectives spends 50× the per-objective round-trip time at minimum, all serialized on the request goroutine. For an interactive CLI that's usually fine, but it's worth considering a small bounded worker pool (e.g. 4–8 concurrent objectives with a shared rate limiter) for noticeably better UX on large cycles. Keeping sequential is acceptable for v1; flagging as a follow-up.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@shortcuts/okr/okr_cycle_detail.go` around lines 95 - 155, The loop over objectives performs sequential N+1 API calls (see objectives slice, runtime.DoAPIJSON, krQuery, common.PaginationMeta) which should be parallelized with a bounded worker pool and shared rate limiter: implement a helper (e.g., fetchKeyResults(obj *Objective, ctx) using the existing pagination logic that returns []KeyResult or error, then launch a limited number of goroutines controlled by a semaphore (buffered channel size 4–8) and a shared rate.Limiter/ticker to call fetchKeyResults concurrently for multiple objectives; ensure you honor ctx.Err() in the helper, replace the fixed time.Sleep with the rate limiter wait, collect results into a thread-safe structure (use a mutex or index-based result slice) and then convert with obj.ToResp and KeyResult.ToResp before appending to respObjectives.
🤖 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_cycle_detail.go`:
- Around line 64-67: Replace the blocking time.Sleep(500 * time.Millisecond) in
the pagination loop (where page is incremented) with a cancellable wait using
select over ctx.Done() and time.After(500*time.Millisecond) so cancellation
unblocks immediately; do the same for the key-results loop (the sleep at lines
around the key-results processing) and ensure you still check ctx.Err() before
proceeding after the wait to abort cleanly.
---
Nitpick comments:
In `@shortcuts/okr/okr_cycle_detail.go`:
- Around line 95-155: The loop over objectives performs sequential N+1 API calls
(see objectives slice, runtime.DoAPIJSON, krQuery, common.PaginationMeta) which
should be parallelized with a bounded worker pool and shared rate limiter:
implement a helper (e.g., fetchKeyResults(obj *Objective, ctx) using the
existing pagination logic that returns []KeyResult or error, then launch a
limited number of goroutines controlled by a semaphore (buffered channel size
4–8) and a shared rate.Limiter/ticker to call fetchKeyResults concurrently for
multiple objectives; ensure you honor ctx.Err() in the helper, replace the fixed
time.Sleep with the rate limiter wait, collect results into a thread-safe
structure (use a mutex or index-based result slice) and then convert with
obj.ToResp and KeyResult.ToResp before appending to respObjectives.
In `@shortcuts/okr/okr_openapi.go`:
- Around line 235-246: The formatTimestamp function emits a local-time string
without zone info making outputs ambiguous; update formatTimestamp to convert
the parsed milliseconds to UTC (or include zone offset) and emit a
machine-parseable format such as RFC3339 so RespCycle.CreateTime/EndTime are
reproducible across hosts: parse millis with strconv.ParseInt as before, call
time.UnixMilli(millis).UTC() (or use .In(...) if a specific zone is needed) and
Format(time.RFC3339), preserving the same empty-string and error-return behavior
otherwise.
- Around line 289-295: The current serialization of ContentBlock silently
ignores json.Marshal errors (see the k.Content block and the similar o.Content /
o.Notes usages), so update these sites to handle errors instead of dropping
them: when calling json.Marshal(k.Content) (and the corresponding
json.Marshal(o.Content) / json.Marshal(o.Notes)), check for err != nil and
either return the error to the caller from the enclosing function or at minimum
log the error (with context identifying the field and record) before continuing;
ensure the same change is applied to the other occurrences referenced (the block
around o.Content/o.Notes) so failures are not silently masked.
🪄 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: 129b8b0d-51f7-4a20-acaa-38e8814e4aa8
📒 Files selected for processing (19)
README.mdREADME.zh.mdinternal/registry/service_descriptions.jsonshortcuts/okr/okr_cli_resp.goshortcuts/okr/okr_cycle_detail.goshortcuts/okr/okr_cycle_detail_test.goshortcuts/okr/okr_cycle_list.goshortcuts/okr/okr_cycle_list_test.goshortcuts/okr/okr_openapi.goshortcuts/okr/okr_openapi_test.goshortcuts/okr/shortcuts.goshortcuts/okr/shortcuts_test.goshortcuts/register.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.mdtests/cli_e2e/okr/okr_cycle_detail_test.gotests/cli_e2e/okr/okr_cycle_list_test.go
✅ Files skipped from review due to trivial changes (12)
- README.md
- shortcuts/okr/shortcuts_test.go
- README.zh.md
- internal/registry/service_descriptions.json
- shortcuts/okr/shortcuts.go
- skills/lark-okr/references/lark-okr-cycle-detail.md
- shortcuts/okr/okr_cli_resp.go
- skills/lark-okr/references/lark-okr-contentblock.md
- skills/lark-okr/SKILL.md
- skills/lark-okr/references/lark-okr-cycle-list.md
- shortcuts/okr/okr_cycle_detail_test.go
- shortcuts/okr/okr_cycle_list_test.go
🚧 Files skipped from review as they are similar to previous changes (5)
- shortcuts/register.go
- tests/cli_e2e/okr/okr_cycle_detail_test.go
- tests/cli_e2e/okr/okr_cycle_list_test.go
- shortcuts/okr/okr_openapi_test.go
- shortcuts/okr/okr_cycle_list.go
syh-cpdsss
force-pushed
the
feat/okr_dev
branch
2 times, most recently
from
099c5b5 to
09004fa
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
shortcuts/okr/okr_cycle_list_test.go (1)
367-427: Strengthen pagination assertions (optional).The test only checks
len(cycles) == 2, which proves both stubs matched but not that:
- both
cycle-p1andcycle-p2actually appear in the output, and- the second request carried
page_token=next_page.Since the stub
CapturedBody/CapturedHeaders(and captured query via URL matching) are available onhttpmock.Stub, you can assert the 2nd call included the forwarded token and verify the merged IDs. Purely a test-quality improvement.♻️ Example
data := decodeEnvelope(t, stdout) cycles, _ := data["cycles"].([]interface{}) if len(cycles) != 2 { t.Fatalf("cycles count = %d, want 2", len(cycles)) } + ids := make([]string, 0, 2) + for _, c := range cycles { + m, _ := c.(map[string]interface{}) + if id, _ := m["id"].(string); id != "" { + ids = append(ids, id) + } + } + sort.Strings(ids) + if !reflect.DeepEqual(ids, []string{"cycle-p1", "cycle-p2"}) { + t.Fatalf("got ids=%v", ids) + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@shortcuts/okr/okr_cycle_list_test.go` around lines 367 - 427, Update TestCycleListExecute_Pagination to assert the actual items and that pagination token was forwarded: after decodeEnvelope(t, stdout) verify the returned cycles slice contains entries with "id" == "cycle-p1" and "cycle-p2" (use the same decodeEnvelope + type asserts used now), and inspect the second registered httpmock.Stub (the one you registered for the "Second page") to assert its captured query/body includes page_token=next_page (use reg and the httpmock.Stub's captured fields) so the test ensures the token was forwarded to the second request as well as both IDs appear.
🤖 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_cycle_list.go`:
- Around line 126-158: The loop in the OKR cycles fetch (inside the function
that uses ctx, page, runtime.DoAPIJSON, queryParams and appends to allCycles
with Cycle) uses an uninterruptible time.Sleep(500 * time.Millisecond) between
pages which ignores ctx cancellation; replace that sleep with a cancellable wait
using select on ctx.Done() and time.After(500 * time.Millisecond) (same approach
used in okr_cycle_detail.go) so the loop exits promptly when ctx is cancelled
and otherwise continues to set page_token from common.PaginationMeta and fetch
the next page.
In `@shortcuts/okr/okr_openapi.go`:
- Around line 236-246: formatTimestamp currently uses time.UnixMilli(millis) and
then Format which renders in the process local timezone; change it to produce
timezone-agnostic output by converting the parsed time to UTC (e.g. call UTC()
on the Time returned by time.UnixMilli) before formatting, or alternatively use
a layout that includes the zone offset such as time.RFC3339 when formatting in
formatTimestamp so the timestamp is unambiguous across hosts and users.
---
Nitpick comments:
In `@shortcuts/okr/okr_cycle_list_test.go`:
- Around line 367-427: Update TestCycleListExecute_Pagination to assert the
actual items and that pagination token was forwarded: after decodeEnvelope(t,
stdout) verify the returned cycles slice contains entries with "id" ==
"cycle-p1" and "cycle-p2" (use the same decodeEnvelope + type asserts used now),
and inspect the second registered httpmock.Stub (the one you registered for the
"Second page") to assert its captured query/body includes page_token=next_page
(use reg and the httpmock.Stub's captured fields) so the test ensures the token
was forwarded to the second request as well as both IDs appear.
🪄 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: 79604184-9f6f-45a0-bc1d-9e9f95d32ce6
📒 Files selected for processing (19)
README.mdREADME.zh.mdinternal/registry/service_descriptions.jsonshortcuts/okr/okr_cli_resp.goshortcuts/okr/okr_cycle_detail.goshortcuts/okr/okr_cycle_detail_test.goshortcuts/okr/okr_cycle_list.goshortcuts/okr/okr_cycle_list_test.goshortcuts/okr/okr_openapi.goshortcuts/okr/okr_openapi_test.goshortcuts/okr/shortcuts.goshortcuts/okr/shortcuts_test.goshortcuts/register.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.mdtests/cli_e2e/okr/okr_cycle_detail_test.gotests/cli_e2e/okr/okr_cycle_list_test.go
✅ Files skipped from review due to trivial changes (12)
- README.zh.md
- shortcuts/okr/shortcuts_test.go
- shortcuts/okr/shortcuts.go
- README.md
- internal/registry/service_descriptions.json
- tests/cli_e2e/okr/okr_cycle_detail_test.go
- skills/lark-okr/references/lark-okr-cycle-detail.md
- tests/cli_e2e/okr/okr_cycle_list_test.go
- skills/lark-okr/references/lark-okr-contentblock.md
- skills/lark-okr/SKILL.md
- shortcuts/okr/okr_cycle_detail_test.go
- skills/lark-okr/references/lark-okr-cycle-list.md
🚧 Files skipped from review as they are similar to previous changes (3)
- shortcuts/register.go
- shortcuts/okr/okr_cli_resp.go
- shortcuts/okr/okr_openapi_test.go
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
shortcuts/okr/okr_openapi.go (1)
235-246:⚠️ Potential issue | 🟡 Minor
formatTimestampstill renders in process-local timezone.This was flagged previously and remains unresolved.
time.UnixMilli(millis)returns aTimein the localLocation, and formatting without a zone indicator makescreate_time/start_time/end_timehost-TZ dependent (the test atokr_openapi_test.goLine 22 even acknowledges this by skipping the time-of-day assertion). Force UTC or include the zone so the output is unambiguous.♻️ Suggested fix
- t := time.UnixMilli(millis) - return t.Format("2006-01-02 15:04:05") + return time.UnixMilli(millis).UTC().Format("2006-01-02 15:04:05 UTC")🤖 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 235 - 246, formatTimestamp currently formats time in the process-local timezone (via time.UnixMilli) making create_time/start_time/end_time host-TZ dependent; change formatTimestamp so it produces an unambiguous UTC timestamp (or includes the zone) by converting the parsed time to UTC (use t.UTC()) and formatting with a timezone-aware layout such as "2006-01-02 15:04:05Z07:00" (or an RFC3339 layout) so all outputs from formatTimestamp are deterministic across hosts.
🧹 Nitpick comments (4)
shortcuts/okr/okr_cycle_list.go (2)
90-100: Minor DryRun / Execute param inconsistency.
DryRunadvertises"page_size": 100(int) whileExecutesendspage_size=100as a string vialarkcore.QueryParams. Both are semantically fine against Lark, but users inspecting dry-run output and comparing to real request traces may be briefly confused. Consider sending"100"in the dry-run map for exact parity.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@shortcuts/okr/okr_cycle_list.go` around lines 90 - 100, The DryRun map in the DryRun function currently sets "page_size" as an integer (100) which differs from the actual Execute request that sends page_size as a string via larkcore.QueryParams; update the DryRun params map in the DryRun function (the params variable inside DryRun in okr_cycle_list.go used to build common.NewDryRunAPI()) to use "page_size": "100" (string) so the DryRun output exactly matches the Execute query encoding.
126-162: Unbounded pagination loop.The pagination loop relies entirely on the server returning
has_more=falseor an emptypage_token. If the API misbehaves (e.g., returnshas_more=truewith a non-empty but repeatedpage_token), this loops indefinitely, gated only byctxcancellation and the 500ms pacing. Consider a safety max (e.g., 1000 pages) or detectingpage_tokennon-progression, emitting a stderr warning on bail-out.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@shortcuts/okr/okr_cycle_list.go` around lines 126 - 162, The pagination loop in the function that builds allCycles can spin indefinitely if the server returns a repeating or always-true pagination signal; add a safety bail-out by tracking pages and last seen page_token: introduce a maxPages constant (e.g., 1000) and a variable lastPageToken, increment a pageCount each iteration and if pageCount > maxPages or pageToken == lastPageToken (non-progression) log a warning and return an error; update the loop that uses page, queryParams, hasMore, pageToken and common.PaginationMeta to set lastPageToken before calling queryParams.Set and check these termination conditions (respecting ctx cancellation) to prevent unbounded loops and surface a clear error when exiting early.shortcuts/okr/okr_openapi_test.go (1)
60-94: Coverage gap:Notesand empty/absentCycleStatusbranches.The test suite doesn't exercise
Objective.NotesJSON serialization (Lines 326-332 inokr_openapi.go) or thec.CycleStatus == nilbranch ofCycle.ToResp(Line 263). Per the coding guideline "Every behavior change must have an accompanying test", consider adding assertions for these paths so future refactors can't silently drop them.As per coding guidelines: "Every behavior change must have an accompanying test".
🤖 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 60 - 94, The test for Objective.ToResp misses exercising Objective.Notes JSON serialization and the Cycle.ToResp branch where CycleStatus is nil; add to the Objective test (the block using Objective{...} and calling obj.ToResp()) a case where Objective.Notes is set (e.g., non-empty Notes) and assert the resp.Notes contains the expected serialized value, and add a separate test for Cycle.ToResp that constructs a Cycle with CycleStatus == nil and asserts the returned response handles nil safely (no panic and expected zero/nil fields); target the Objective.ToResp and Cycle.ToResp code paths to cover the Notes serialization and the c.CycleStatus == nil branch.shortcuts/okr/okr_openapi.go (1)
289-295: Silent swallow ofjson.Marshalerrors.If
json.Marshal(k.Content)fails,Contentbecomesnilin the response with no diagnostic — same pattern at Lines 320-325 and 326-332 inObjective.ToResp. In practiceencoding/jsonrarely errors on these plain structs, but when it does the user sees silently-missing content. Consider propagating the error (changingToRespto return(*RespKeyResult, error)) or at minimum logging viaruntime.Warnf/stderrso the loss is observable.🤖 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 289 - 295, The code silently swallows json.Marshal errors when serializing k.Content (inside KeyResult.ToResp) and the analogous blocks in Objective.ToResp; update these ToResp methods to handle serialization failures instead of dropping Content silently — either change the signatures (e.g., KeyResult.ToResp and Objective.ToResp) to return (resp, error) and propagate the json.Marshal error, or at minimum call runtime.Warnf or write to stderr when json.Marshal returns an error so the failure is observable; ensure you check the error from json.Marshal before assigning result.Content and propagate/log accordingly.
🤖 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_cycle_list.go`:
- Around line 144-155: The loop over itemsRaw silently drops entries when
json.Marshal or json.Unmarshal fails; update the loop in which itemsRaw is
iterated and Cycle values are appended (the block using json.Marshal(item),
json.Unmarshal(raw, &cycle), and appending to allCycles) to emit a stderr
warning instead of failing silently: import fmt and os if needed and call
fmt.Fprintf(os.Stderr, ...) including the item index and either the raw item
value (on marshal error) or the raw JSON bytes and unmarshal error (on unmarshal
error) so failures are observable while retaining the current skip behavior.
In `@shortcuts/okr/okr_openapi_test.go`:
- Around line 19-23: The test for formatTimestamp is too loose and flaky; update
the test in okr_openapi_test.go to assert the exact UTC-formatted output once
formatTimestamp is hardened to UTC: replace the current prefix check
(convey.So(result, convey.ShouldStartWith, "2025-01-01")) with an equality check
asserting result == "2025-01-01 00:00:00 UTC" (use convey.ShouldEqual) and
remove the timezone-avoidance comment so the test enforces the exact UTC string
from formatTimestamp.
---
Duplicate comments:
In `@shortcuts/okr/okr_openapi.go`:
- Around line 235-246: formatTimestamp currently formats time in the
process-local timezone (via time.UnixMilli) making
create_time/start_time/end_time host-TZ dependent; change formatTimestamp so it
produces an unambiguous UTC timestamp (or includes the zone) by converting the
parsed time to UTC (use t.UTC()) and formatting with a timezone-aware layout
such as "2006-01-02 15:04:05Z07:00" (or an RFC3339 layout) so all outputs from
formatTimestamp are deterministic across hosts.
---
Nitpick comments:
In `@shortcuts/okr/okr_cycle_list.go`:
- Around line 90-100: The DryRun map in the DryRun function currently sets
"page_size" as an integer (100) which differs from the actual Execute request
that sends page_size as a string via larkcore.QueryParams; update the DryRun
params map in the DryRun function (the params variable inside DryRun in
okr_cycle_list.go used to build common.NewDryRunAPI()) to use "page_size": "100"
(string) so the DryRun output exactly matches the Execute query encoding.
- Around line 126-162: The pagination loop in the function that builds allCycles
can spin indefinitely if the server returns a repeating or always-true
pagination signal; add a safety bail-out by tracking pages and last seen
page_token: introduce a maxPages constant (e.g., 1000) and a variable
lastPageToken, increment a pageCount each iteration and if pageCount > maxPages
or pageToken == lastPageToken (non-progression) log a warning and return an
error; update the loop that uses page, queryParams, hasMore, pageToken and
common.PaginationMeta to set lastPageToken before calling queryParams.Set and
check these termination conditions (respecting ctx cancellation) to prevent
unbounded loops and surface a clear error when exiting early.
In `@shortcuts/okr/okr_openapi_test.go`:
- Around line 60-94: The test for Objective.ToResp misses exercising
Objective.Notes JSON serialization and the Cycle.ToResp branch where CycleStatus
is nil; add to the Objective test (the block using Objective{...} and calling
obj.ToResp()) a case where Objective.Notes is set (e.g., non-empty Notes) and
assert the resp.Notes contains the expected serialized value, and add a separate
test for Cycle.ToResp that constructs a Cycle with CycleStatus == nil and
asserts the returned response handles nil safely (no panic and expected zero/nil
fields); target the Objective.ToResp and Cycle.ToResp code paths to cover the
Notes serialization and the c.CycleStatus == nil branch.
In `@shortcuts/okr/okr_openapi.go`:
- Around line 289-295: The code silently swallows json.Marshal errors when
serializing k.Content (inside KeyResult.ToResp) and the analogous blocks in
Objective.ToResp; update these ToResp methods to handle serialization failures
instead of dropping Content silently — either change the signatures (e.g.,
KeyResult.ToResp and Objective.ToResp) to return (resp, error) and propagate the
json.Marshal error, or at minimum call runtime.Warnf or write to stderr when
json.Marshal returns an error so the failure is observable; ensure you check the
error from json.Marshal before assigning result.Content and propagate/log
accordingly.
🪄 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: c3da9db2-31e1-497d-95cd-cc273452f394
📒 Files selected for processing (19)
README.mdREADME.zh.mdinternal/registry/service_descriptions.jsonshortcuts/okr/okr_cli_resp.goshortcuts/okr/okr_cycle_detail.goshortcuts/okr/okr_cycle_detail_test.goshortcuts/okr/okr_cycle_list.goshortcuts/okr/okr_cycle_list_test.goshortcuts/okr/okr_openapi.goshortcuts/okr/okr_openapi_test.goshortcuts/okr/shortcuts.goshortcuts/okr/shortcuts_test.goshortcuts/register.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.mdtests/cli_e2e/okr/okr_cycle_detail_test.gotests/cli_e2e/okr/okr_cycle_list_test.go
✅ Files skipped from review due to trivial changes (11)
- shortcuts/okr/shortcuts_test.go
- README.zh.md
- internal/registry/service_descriptions.json
- tests/cli_e2e/okr/okr_cycle_detail_test.go
- tests/cli_e2e/okr/okr_cycle_list_test.go
- skills/lark-okr/references/lark-okr-cycle-detail.md
- skills/lark-okr/references/lark-okr-cycle-list.md
- shortcuts/okr/okr_cycle_detail_test.go
- skills/lark-okr/references/lark-okr-contentblock.md
- shortcuts/okr/okr_cli_resp.go
- shortcuts/okr/okr_cycle_list_test.go
🚧 Files skipped from review as they are similar to previous changes (5)
- shortcuts/okr/shortcuts.go
- README.md
- shortcuts/register.go
- shortcuts/okr/okr_cycle_detail.go
- skills/lark-okr/SKILL.md
Change-Id: I1877c56e33e3620b696351ed9e4c8615dbe17c4b
syh-cpdsss
force-pushed
the
feat/okr_dev
branch
from
09004fa to
d096a51
Compare
Change-Id: I1877c56e33e3620b696351ed9e4c8615dbe17c4b
zhangzq0
approved these changes
Apr 17, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Add the OKR business domain to lark-cli, enabling users and AI agents to query OKR cycles and drill into objectives with key results via two new shortcuts:
okr +cycle-listandokr +cycle-detail.Changes
okr +cycle-list— List OKR cycles for a given user, with optional time-range filtering (--time-range YYYY-MM--YYYY-MM).okr +cycle-detail— List all objectives and their key results under a given OKR cycle. Returns a nested structure withContentBlockrich-text content serialized as JSON strings.skills/lark-okr/SKILL.md) — Skill definition for AI agents, with shortcut references for+cycle-listand+cycle-detailand ContentBlock rich-text format documentation.Test Plan
go test ./shortcuts/okr/ -v -count=1)lark okr +cycle-list --user-id "xxx"andlark okr +cycle-detail --cycle-id xxxwork as expectedRelated Issues
Summary by CodeRabbit
New Features
Documentation
Tests