Feat/okr semi plain text and patch#1671
Merged
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 SemiPlainContent conversion helpers, style-aware OKR shortcuts for simple and richtext output/input, a new incremental patch shortcut, current active cycle detection, and updated docs and tests for the new response and content formats. ChangesOKR Style Mode, Patch Shortcut, and Cycle Enhancements
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #1671 +/- ##
==========================================
- Coverage 74.48% 74.45% -0.04%
==========================================
Files 850 851 +1
Lines 86703 87129 +426
==========================================
+ Hits 64583 64873 +290
- Misses 17175 17282 +107
- Partials 4945 4974 +29 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
🚀 PR Preview Install Guide🧰 CLI updatenpm i -g https://pkg.pr.new/larksuite/cli/@larksuite/cli@3fc2a6c4c108e697675b87e53f7cbf469c1fb220🧩 Skill updatenpx skills add larksuite/cli#feat/okr-semi-plain-text-and-patch -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: 9
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (5)
shortcuts/okr/okr_cycle_list_test.go (1)
393-419: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winAvoid hard-coding a “current” cycle to 2026.
Executeusestime.Now(), so this fixture stops being active after 2026-12-31 and the test will start failing. Build the mocked timestamps relative totime.Now().UTC()instead.Proposed deterministic fixture shape
+ now := time.Now().UTC() + ms := func(t time.Time) string { + return strconv.FormatInt(t.UnixMilli(), 10) + } + activeStart := ms(now.Add(-24 * time.Hour)) + activeEnd := ms(now.Add(24 * time.Hour)) + pastStart := ms(now.AddDate(-2, 0, 0)) + pastEnd := ms(now.AddDate(-1, 0, 0)) + reg.Register(&httpmock.Stub{ ... - "start_time": "1767225600000", // 2026-01-01 00:00:00 - "end_time": "1798761599000", // 2026-12-31 23:59:59 UTC + "start_time": activeStart, + "end_time": activeEnd, ... - "start_time": "1704067200000", // 2024-01-01 - "end_time": "1719791999999", // 2024-06-30 23:59:59 + "start_time": pastStart, + "end_time": pastEnd,This also requires adding
strconvto the imports.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@shortcuts/okr/okr_cycle_list_test.go` around lines 393 - 419, The TestCycleListExecute_WithCycles fixture hard-codes a “current” cycle window for 2026, which will break once Execute()’s time.Now() falls outside that range. Update the mocked cycle timestamps to be computed relative to time.Now().UTC() so the active/past cycle setup stays stable over time, and add strconv where needed to format the generated timestamps in the httpmock.Stub body.skills/lark-okr/references/lark-okr-cycle-detail.md (1)
37-80: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winOutput JSON example doesn't match
--style simpledefault.The documented output example shows fields like
create_time,update_time,owner,position,deadline,category_id, and stringifiedcontent/notes. However, with the default--style simple,+cycle-detailreturnsRespObjectiveSimple/RespKeyResultSimplewhich omit most of these fields and useSemiPlainContentobjects forcontent/notes. Either update the example to showsimple-style output (matching the default), or add a second example forrichtextand clarify which example corresponds to which style.Cross-reference:
shortcuts/okr/okr_cycle_detail.go:98-155showsRespObjectiveSimplecontains onlyid,score,weight,content(as*SemiPlainContent),notes(as*SemiPlainContent), andkey_results(as[]RespKeyResultSimple).🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@skills/lark-okr/references/lark-okr-cycle-detail.md` around lines 37 - 80, The `+cycle-detail` JSON example is inconsistent with the default `--style simple` output, since `RespObjectiveSimple` and `RespKeyResultSimple` do not include fields like `create_time`, `update_time`, `owner`, `position`, `deadline`, `category_id`, or stringified `content`/`notes`. Update the example to match the simple-style shape returned by `okr_cycle_detail.go` (using the `RespObjectiveSimple` / `RespKeyResultSimple` symbols), or add a separate example for `richtext` and clearly label which output belongs to each style.shortcuts/okr/okr_progress_create.go (1)
269-283: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winHonor
--stylewhen emitting the create result.
Executealways usesrecord.ToResp(), so+progress-create --style simplestill returns richtextprogress.contentwhenever the API includes content. That leaves create inconsistent with the new simple/richtext response contract already implemented in+progress-get.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@shortcuts/okr/okr_progress_create.go` around lines 269 - 283, `okr_progress_create.go`’s `Execute` currently always uses `record.ToResp()`, so the create output ignores `--style` and can emit richtext `progress.content` even in simple mode. Update the result-building path in `Execute` to honor the requested style the same way `+progress-get` does, using the existing response conversion/helpers to choose between simple and richtext output before calling `runtime.OutFormat`. Keep the `progress` payload consistent with the style contract and only include `Content` when richtext is requested.shortcuts/okr/okr_progress_update.go (1)
212-226: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winHonor
--stylewhen emitting the update result.
Executealways serializesrecord.ToResp(), so+progress-update --style simplestill emits richtextprogress.contentwhenever content is present. That makes update inconsistent with+progress-getand the new simple/richtext response contract.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@shortcuts/okr/okr_progress_update.go` around lines 212 - 226, `OKRProgressUpdate.Execute` currently always outputs `record.ToResp()` as richtext, so it ignores the selected `--style` and can emit `progress.content` even in simple mode. Update the response-building logic in `Execute` (and the `resp := record.ToResp()` / `runtime.OutFormat(...)` path) to honor the requested style the same way `progress-get` does, returning a simple response when `--style simple` is used and only including richtext content when the style requires it.tests/cli_e2e/okr/okr_progress_test.go (1)
68-90: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick winVerify the dry-run request payload, not only the URL.
These cases only assert the path/method, so they will still pass if the new
--styleplumbing builds the wrong request body. Please inspect the emitted JSON and assert the content structure/query fields that actually changed in this PR.
As per coding guidelines, dry-run E2E tests validate request structure without calling real APIs, and every behavior change needs a test alongside the change.Also applies to: 93-115, 206-250
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/cli_e2e/okr/okr_progress_test.go` around lines 68 - 90, The dry-run E2E checks in TestOKR_ProgressCreateDryRun (and the related OKR dry-run tests) only verify the URL/method, so they miss regressions in the new request body plumbing. Update the assertions to inspect the emitted JSON from clie2e.RunCmd and validate the request payload fields that changed with --style, especially the content structure and any query/body fields tied to progress creation. Use the existing test helpers and the relevant OKR command path to confirm the dry-run output matches the expected request shape.Source: Coding guidelines
🧹 Nitpick comments (1)
skills/lark-okr/references/lark-okr-progress-get.md (1)
43-64: 🧹 Nitpick | 🔵 TrivialClarify example JSON for style-dependent
contentshape.The example JSON shows
"content": "{...}"as a string placeholder, but the actual output shape differs by--style:
simple:contentis aSemiPlainContentobject (e.g.,{"text":"...","mention":[...]})richtext:contentis a JSON string representing aContentBlockConsider updating the example to show the concrete
simplestyle output (since it's the default) or add a comment noting thatcontentis either an object or string depending on--style.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@skills/lark-okr/references/lark-okr-progress-get.md` around lines 43 - 64, The example for the progress response uses a vague string placeholder for progress.content, but the shape depends on the --style option. Update the example in lark-okr-progress-get.md to make the default simple case explicit by showing progress.content as a SemiPlainContent object, and note that richtext returns a JSON string ContentBlock; reference the progress.content and progress_rate sections so the style-dependent behavior is clear.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@shortcuts/okr/okr_cycle_detail.go`:
- Line 29: The new style flag on the `+cycle-detail` command is changing the
default response shape, so preserve the existing `ContentBlock` JSON behavior by
making `richtext` the default in the flag definition for the `style` option.
Update the `okr_cycle_detail.go` command setup so callers that omit `--style`
continue to get the old richtext output, while `simple` remains an explicit
opt-in via the same flag and enum.
In `@shortcuts/okr/okr_cycle_list_test.go`:
- Around line 264-267: The test in okr_cycle_list_test is only checking that
current_active_cycles is empty, so a missing field can still pass because the
type assertion yields nil. Tighten the assertion around the response contract by
explicitly verifying the current_active_cycles key exists in the decoded data
map and that its value has the expected slice type before checking its length;
use the existing test logic in the OKR cycle list response assertions to fail
when the field is absent or has the wrong shape.
In `@shortcuts/okr/okr_openapi_test.go`:
- Around line 859-930: Add a direct regression test in TestToSimpleMethods for
the separate Progress.ToSimple() converter, since the current coverage only
exercises ProgressV1.ToSimple(). Use the Progress symbol from okr_cli_resp.go
and assert the distinct field mapping for UpdateTime, CreateTime,
ProgressPercent, and ProgressStatus so the new behavior is covered alongside the
change.
In `@shortcuts/okr/okr_openapi.go`:
- Around line 737-738: ToContentBlock() is flattening mention markers from
ToSemiPlain() by stripping all @{userID} placeholders from Text and appending
mentions at the end, which loses the original inline order. Update
ToContentBlock (and the helper logic around placeholderRE/multiSpaceRE) to parse
s.Text into alternating text and mention segments, then rebuild the paragraph in
sequence so round-trips like “Hello @{ou_123}, world” preserve the mention
position.
In `@shortcuts/okr/okr_patch_test.go`:
- Around line 44-596: The patch validation tests are too dependent on error
strings and direct concrete-type checks, so they can miss regressions in typed
metadata or preserved causes. Update the error-path cases in TestPatchValidate_*
to assert errs.ProblemOf(err) for category/subtype, and use errors.As only to
verify *errs.ValidationError.Param. Also add cause-preservation checks for
wrapped failures instead of relying on substrings alone, using the existing
runPatchShortcut and errs.ValidationError symbols to locate the affected
assertions.
In `@shortcuts/okr/okr_patch.go`:
- Around line 130-135: The deadline validation in the OKR patch path is too
permissive because `strconv.ParseInt` only checks that `deadline` is numeric,
not that it is a positive Unix-millis value. Update the validation in the
`runtime.Str("deadline")` block inside the patch logic to reject non-millisecond
inputs like seconds or negative numbers by explicitly enforcing a positive
minimum length/range and/or validating against a millisecond timestamp threshold
before assigning `p.Deadline`.
In `@shortcuts/okr/okr_progress_create_test.go`:
- Around line 429-478: The new simple-mode validation tests are only checking
error strings, so they should be updated to assert typed error metadata in the
progress-create validation path. In the relevant
TestProgressCreateValidate_SimpleMode_* cases, use errs.ProblemOf on the
returned error to verify category and subtype, and use errors.As with
errs.ValidationError when you need to assert Param for the validation failure.
Also ensure the underlying cause/unwrap chain is preserved for the invalid JSON
case rather than relying only on err.Error() substring matches.
In `@shortcuts/okr/okr_progress_create.go`:
- Around line 43-61: The simple-mode conversion in OKR progress creation is
reordering inline mentions by stripping placeholders and appending mentions
afterward. Update the `okr_progress_create.go` path that builds `contentV1` from
`SemiPlainContent` so mention positions are preserved when calling
`sp.ToContentBlock()` and converting to V1, instead of losing the original
inline order in `--style simple`.
In `@shortcuts/okr/okr_progress_update_test.go`:
- Around line 358-403: The new simple-mode validation tests only assert on
err.Error(), so they miss regressions in typed error metadata and unwrap/cause
behavior. Update the tests in
TestProgressUpdateValidate_SimpleMode_InvalidSemiPlainJSON,
TestProgressUpdateValidate_SimpleMode_EmptyMention, and
TestProgressUpdateValidate_SimpleMode_ImagesNotSupported to assert problem
fields with errs.ProblemOf, and use errors.As with errs.ValidationError when
checking Param-specific validation details. Keep the existing
runProgressUpdateShortcut setup, but replace substring-only checks with typed
assertions that verify category, subtype, param, and preserved cause where
applicable.
---
Outside diff comments:
In `@shortcuts/okr/okr_cycle_list_test.go`:
- Around line 393-419: The TestCycleListExecute_WithCycles fixture hard-codes a
“current” cycle window for 2026, which will break once Execute()’s time.Now()
falls outside that range. Update the mocked cycle timestamps to be computed
relative to time.Now().UTC() so the active/past cycle setup stays stable over
time, and add strconv where needed to format the generated timestamps in the
httpmock.Stub body.
In `@shortcuts/okr/okr_progress_create.go`:
- Around line 269-283: `okr_progress_create.go`’s `Execute` currently always
uses `record.ToResp()`, so the create output ignores `--style` and can emit
richtext `progress.content` even in simple mode. Update the result-building path
in `Execute` to honor the requested style the same way `+progress-get` does,
using the existing response conversion/helpers to choose between simple and
richtext output before calling `runtime.OutFormat`. Keep the `progress` payload
consistent with the style contract and only include `Content` when richtext is
requested.
In `@shortcuts/okr/okr_progress_update.go`:
- Around line 212-226: `OKRProgressUpdate.Execute` currently always outputs
`record.ToResp()` as richtext, so it ignores the selected `--style` and can emit
`progress.content` even in simple mode. Update the response-building logic in
`Execute` (and the `resp := record.ToResp()` / `runtime.OutFormat(...)` path) to
honor the requested style the same way `progress-get` does, returning a simple
response when `--style simple` is used and only including richtext content when
the style requires it.
In `@skills/lark-okr/references/lark-okr-cycle-detail.md`:
- Around line 37-80: The `+cycle-detail` JSON example is inconsistent with the
default `--style simple` output, since `RespObjectiveSimple` and
`RespKeyResultSimple` do not include fields like `create_time`, `update_time`,
`owner`, `position`, `deadline`, `category_id`, or stringified
`content`/`notes`. Update the example to match the simple-style shape returned
by `okr_cycle_detail.go` (using the `RespObjectiveSimple` /
`RespKeyResultSimple` symbols), or add a separate example for `richtext` and
clearly label which output belongs to each style.
In `@tests/cli_e2e/okr/okr_progress_test.go`:
- Around line 68-90: The dry-run E2E checks in TestOKR_ProgressCreateDryRun (and
the related OKR dry-run tests) only verify the URL/method, so they miss
regressions in the new request body plumbing. Update the assertions to inspect
the emitted JSON from clie2e.RunCmd and validate the request payload fields that
changed with --style, especially the content structure and any query/body fields
tied to progress creation. Use the existing test helpers and the relevant OKR
command path to confirm the dry-run output matches the expected request shape.
---
Nitpick comments:
In `@skills/lark-okr/references/lark-okr-progress-get.md`:
- Around line 43-64: The example for the progress response uses a vague string
placeholder for progress.content, but the shape depends on the --style option.
Update the example in lark-okr-progress-get.md to make the default simple case
explicit by showing progress.content as a SemiPlainContent object, and note that
richtext returns a JSON string ContentBlock; reference the progress.content and
progress_rate sections so the style-dependent behavior is clear.
🪄 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: 8d06f070-4a97-4395-a5a5-3b2f61f4c080
📒 Files selected for processing (29)
shortcuts/okr/okr_batch_create.goshortcuts/okr/okr_batch_create_test.goshortcuts/okr/okr_cli_resp.goshortcuts/okr/okr_cycle_detail.goshortcuts/okr/okr_cycle_list.goshortcuts/okr/okr_cycle_list_test.goshortcuts/okr/okr_openapi.goshortcuts/okr/okr_openapi_test.goshortcuts/okr/okr_patch.goshortcuts/okr/okr_patch_test.goshortcuts/okr/okr_progress_create.goshortcuts/okr/okr_progress_create_test.goshortcuts/okr/okr_progress_get.goshortcuts/okr/okr_progress_update.goshortcuts/okr/okr_progress_update_test.goshortcuts/okr/shortcuts.goskills/lark-okr/SKILL.mdskills/lark-okr/references/lark-okr-alignments.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-indicators.mdskills/lark-okr/references/lark-okr-patch.mdskills/lark-okr/references/lark-okr-progress-create.mdskills/lark-okr/references/lark-okr-progress-get.mdskills/lark-okr/references/lark-okr-progress-update.mdtests/cli_e2e/okr/okr_cycle_detail_test.gotests/cli_e2e/okr/okr_progress_test.gotests/cli_e2e/okr/okr_shortcuts_test.go
syh-cpdsss
force-pushed
the
feat/okr-semi-plain-text-and-patch
branch
from
June 30, 2026 07:56
b3309b4 to
3cd27f1
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@shortcuts/okr/okr_cycle_list.go`:
- Around line 222-225: The new “Current active cycle(s)” branch in OkrCycleList
output is untested, so extend okr_cycle_list_test.go to cover the non-empty
currentActiveCycles path. Add or update a test around OkrCycleList/its rendering
behavior that sets currentActiveCycles with at least one cycle and asserts the
output includes the “Current active cycle(s):” section and the expected cycle
row format.
🪄 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: 07dc43be-045b-4474-bb2b-33e36e468a2a
📒 Files selected for processing (15)
shortcuts/okr/okr_cli_resp.goshortcuts/okr/okr_cycle_list.goshortcuts/okr/okr_cycle_list_test.goshortcuts/okr/okr_openapi.goshortcuts/okr/okr_openapi_test.goshortcuts/okr/okr_patch_test.goskills/lark-okr/SKILL.mdskills/lark-okr/references/lark-okr-alignments.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-indicators.mdskills/lark-okr/references/lark-okr-patch.mdskills/lark-okr/references/lark-okr-progress-create.mdskills/lark-okr/references/lark-okr-progress-update.md
✅ Files skipped from review due to trivial changes (6)
- skills/lark-okr/references/lark-okr-alignments.md
- skills/lark-okr/references/lark-okr-patch.md
- skills/lark-okr/references/lark-okr-indicators.md
- skills/lark-okr/references/lark-okr-cycle-detail.md
- skills/lark-okr/references/lark-okr-cycle-list.md
- skills/lark-okr/references/lark-okr-progress-update.md
🚧 Files skipped from review as they are similar to previous changes (6)
- shortcuts/okr/okr_cycle_list_test.go
- shortcuts/okr/okr_patch_test.go
- skills/lark-okr/references/lark-okr-contentblock.md
- shortcuts/okr/okr_openapi.go
- skills/lark-okr/references/lark-okr-progress-create.md
- shortcuts/okr/okr_openapi_test.go
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@shortcuts/okr/okr_openapi_test.go`:
- Around line 949-951: The current assertion in the OKR OpenAPI test only
verifies that CreateTime is non-empty, which can miss a swapped or incorrectly
formatted V2 timestamp mapping. Update the test around simpleProgressV2 to
assert the exact mapping for both fields: CreateTime should match the formatted
progressV2.CreateTime value and ModifyTime should match the formatted
progressV2.UpdateTime value, so the timestamp conversion logic is fully covered.
🪄 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: de88150a-93dc-4649-bde9-83d811d5c640
📒 Files selected for processing (12)
shortcuts/okr/okr_cycle_list_test.goshortcuts/okr/okr_openapi.goshortcuts/okr/okr_openapi_test.goshortcuts/okr/okr_patch.goshortcuts/okr/okr_patch_test.goshortcuts/okr/okr_progress_create.goshortcuts/okr/okr_progress_create_test.goshortcuts/okr/okr_progress_update.goshortcuts/okr/okr_progress_update_test.goskills/lark-okr/references/lark-okr-cycle-detail.mdskills/lark-okr/references/lark-okr-progress-get.mdtests/cli_e2e/okr/okr_progress_test.go
✅ Files skipped from review due to trivial changes (2)
- skills/lark-okr/references/lark-okr-progress-get.md
- skills/lark-okr/references/lark-okr-cycle-detail.md
🚧 Files skipped from review as they are similar to previous changes (8)
- shortcuts/okr/okr_progress_create.go
- shortcuts/okr/okr_progress_update.go
- shortcuts/okr/okr_patch.go
- shortcuts/okr/okr_progress_create_test.go
- shortcuts/okr/okr_cycle_list_test.go
- shortcuts/okr/okr_openapi.go
- shortcuts/okr/okr_patch_test.go
- shortcuts/okr/okr_progress_update_test.go
syh-cpdsss
force-pushed
the
feat/okr-semi-plain-text-and-patch
branch
from
June 30, 2026 09:19
8984dd4 to
4cb415b
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@shortcuts/okr/okr_openapi_test.go`:
- Around line 949-951: The timestamp assertion in the Progress V2 test is too
weak because it only checks that CreateTime is non-empty, so regressions in the
CreateTime/ModifyTime mapping could still pass. Update the assertions in the
simpleProgressV2 check to verify exact values against
formatTimestamp(progressV2.CreateTime) and
formatTimestamp(progressV2.UpdateTime), using the existing Progress V2 fields
and the formatTimestamp helper to validate both CreateTime and ModifyTime.
🪄 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: f460c35f-36c5-47f6-afbd-33784a4a5a9d
📒 Files selected for processing (29)
shortcuts/okr/okr_batch_create.goshortcuts/okr/okr_batch_create_test.goshortcuts/okr/okr_cli_resp.goshortcuts/okr/okr_cycle_detail.goshortcuts/okr/okr_cycle_list.goshortcuts/okr/okr_cycle_list_test.goshortcuts/okr/okr_openapi.goshortcuts/okr/okr_openapi_test.goshortcuts/okr/okr_patch.goshortcuts/okr/okr_patch_test.goshortcuts/okr/okr_progress_create.goshortcuts/okr/okr_progress_create_test.goshortcuts/okr/okr_progress_get.goshortcuts/okr/okr_progress_update.goshortcuts/okr/okr_progress_update_test.goshortcuts/okr/shortcuts.goskills/lark-okr/SKILL.mdskills/lark-okr/references/lark-okr-alignments.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-indicators.mdskills/lark-okr/references/lark-okr-patch.mdskills/lark-okr/references/lark-okr-progress-create.mdskills/lark-okr/references/lark-okr-progress-get.mdskills/lark-okr/references/lark-okr-progress-update.mdtests/cli_e2e/okr/okr_cycle_detail_test.gotests/cli_e2e/okr/okr_progress_test.gotests/cli_e2e/okr/okr_shortcuts_test.go
✅ Files skipped from review due to trivial changes (7)
- skills/lark-okr/references/lark-okr-cycle-detail.md
- skills/lark-okr/references/lark-okr-indicators.md
- skills/lark-okr/references/lark-okr-progress-update.md
- skills/lark-okr/references/lark-okr-alignments.md
- skills/lark-okr/references/lark-okr-cycle-list.md
- skills/lark-okr/references/lark-okr-patch.md
- skills/lark-okr/references/lark-okr-progress-get.md
🚧 Files skipped from review as they are similar to previous changes (20)
- shortcuts/okr/okr_batch_create_test.go
- tests/cli_e2e/okr/okr_cycle_detail_test.go
- shortcuts/okr/shortcuts.go
- shortcuts/okr/okr_cycle_list.go
- skills/lark-okr/references/lark-okr-contentblock.md
- tests/cli_e2e/okr/okr_shortcuts_test.go
- shortcuts/okr/okr_patch.go
- shortcuts/okr/okr_progress_get.go
- shortcuts/okr/okr_batch_create.go
- shortcuts/okr/okr_progress_update.go
- tests/cli_e2e/okr/okr_progress_test.go
- shortcuts/okr/okr_progress_create.go
- shortcuts/okr/okr_cycle_list_test.go
- skills/lark-okr/references/lark-okr-progress-create.md
- shortcuts/okr/okr_openapi.go
- shortcuts/okr/okr_cycle_detail.go
- shortcuts/okr/okr_progress_create_test.go
- shortcuts/okr/okr_progress_update_test.go
- shortcuts/okr/okr_cli_resp.go
- shortcuts/okr/okr_patch_test.go
syh-cpdsss
force-pushed
the
feat/okr-semi-plain-text-and-patch
branch
from
June 30, 2026 09:46
4cb415b to
2661b9a
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
shortcuts/okr/okr_openapi_test.go (1)
949-951: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winStrengthen the V2 timestamp assertions to exact source mappings.
This still won't catch
CreateTime/UpdateTimebeing swapped, andModifyTimeis not asserted at all.Suggested test tightening
- if simpleProgressV2.CreateTime == nil || *simpleProgressV2.CreateTime == "" { - t.Fatal("expected non-empty CreateTime for Progress V2") - } + if simpleProgressV2.CreateTime == nil || *simpleProgressV2.CreateTime != formatTimestamp(progressV2.CreateTime) { + t.Fatalf("unexpected CreateTime: %+v", simpleProgressV2.CreateTime) + } + if simpleProgressV2.ModifyTime == nil || *simpleProgressV2.ModifyTime != formatTimestamp(progressV2.UpdateTime) { + t.Fatalf("unexpected ModifyTime: %+v", simpleProgressV2.ModifyTime) + }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@shortcuts/okr/okr_openapi_test.go` around lines 949 - 951, The Progress V2 timestamp test only checks that CreateTime is non-empty, so it can miss CreateTime/UpdateTime being swapped and does not verify ModifyTime. Tighten the assertions in the relevant test around simpleProgressV2 so each timestamp field is checked against its exact expected source mapping: assert CreateTime, UpdateTime, and ModifyTime individually and compare each one to the correct originating timestamp value.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@shortcuts/okr/okr_openapi_test.go`:
- Around line 60-62: The okr_openapi_test assertions for RespCycle are too loose
because they only check that StartTime and EndTime are non-empty. Tighten the
test around the resp mapping by asserting the exact converted timestamp values
produced from the source fields in the response, using the existing RespCycle
assertions in okr_openapi_test to verify StartTime maps to the expected start
value and EndTime maps to the expected end value.
- Around line 770-802: The round-trip test in
TestSemiPlainContentToContentBlock_PlaceholderStripping only checks that
mentions exist, not that their positions are preserved. Update the assertions
against SemiPlainContent.ToContentBlock() so the resulting Paragraph.Elements
are verified in the original interleaved order (text, mention, text, mention)
rather than allowing all mentions to be appended at the end. Confirm the
stripped text segments and the mention UserID values using the existing
cb.Blocks[0].Paragraph.Elements structure.
---
Duplicate comments:
In `@shortcuts/okr/okr_openapi_test.go`:
- Around line 949-951: The Progress V2 timestamp test only checks that
CreateTime is non-empty, so it can miss CreateTime/UpdateTime being swapped and
does not verify ModifyTime. Tighten the assertions in the relevant test around
simpleProgressV2 so each timestamp field is checked against its exact expected
source mapping: assert CreateTime, UpdateTime, and ModifyTime individually and
compare each one to the correct originating timestamp value.
🪄 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: 8ae83dfb-984a-4322-884e-6ca0e1913b76
📒 Files selected for processing (29)
shortcuts/okr/okr_batch_create.goshortcuts/okr/okr_batch_create_test.goshortcuts/okr/okr_cli_resp.goshortcuts/okr/okr_cycle_detail.goshortcuts/okr/okr_cycle_list.goshortcuts/okr/okr_cycle_list_test.goshortcuts/okr/okr_openapi.goshortcuts/okr/okr_openapi_test.goshortcuts/okr/okr_patch.goshortcuts/okr/okr_patch_test.goshortcuts/okr/okr_progress_create.goshortcuts/okr/okr_progress_create_test.goshortcuts/okr/okr_progress_get.goshortcuts/okr/okr_progress_update.goshortcuts/okr/okr_progress_update_test.goshortcuts/okr/shortcuts.goskills/lark-okr/SKILL.mdskills/lark-okr/references/lark-okr-alignments.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-indicators.mdskills/lark-okr/references/lark-okr-patch.mdskills/lark-okr/references/lark-okr-progress-create.mdskills/lark-okr/references/lark-okr-progress-get.mdskills/lark-okr/references/lark-okr-progress-update.mdtests/cli_e2e/okr/okr_cycle_detail_test.gotests/cli_e2e/okr/okr_progress_test.gotests/cli_e2e/okr/okr_shortcuts_test.go
✅ Files skipped from review due to trivial changes (9)
- skills/lark-okr/references/lark-okr-patch.md
- skills/lark-okr/references/lark-okr-cycle-detail.md
- skills/lark-okr/references/lark-okr-alignments.md
- skills/lark-okr/references/lark-okr-progress-get.md
- skills/lark-okr/references/lark-okr-contentblock.md
- skills/lark-okr/references/lark-okr-cycle-list.md
- skills/lark-okr/references/lark-okr-progress-create.md
- skills/lark-okr/references/lark-okr-indicators.md
- skills/lark-okr/references/lark-okr-progress-update.md
🚧 Files skipped from review as they are similar to previous changes (18)
- shortcuts/okr/shortcuts.go
- shortcuts/okr/okr_batch_create_test.go
- tests/cli_e2e/okr/okr_shortcuts_test.go
- tests/cli_e2e/okr/okr_cycle_detail_test.go
- shortcuts/okr/okr_batch_create.go
- tests/cli_e2e/okr/okr_progress_test.go
- shortcuts/okr/okr_cycle_list.go
- shortcuts/okr/okr_progress_create.go
- shortcuts/okr/okr_progress_get.go
- shortcuts/okr/okr_progress_create_test.go
- shortcuts/okr/okr_cycle_list_test.go
- shortcuts/okr/okr_cli_resp.go
- shortcuts/okr/okr_progress_update_test.go
- shortcuts/okr/okr_progress_update.go
- shortcuts/okr/okr_patch.go
- shortcuts/okr/okr_cycle_detail.go
- shortcuts/okr/okr_patch_test.go
- shortcuts/okr/okr_openapi.go
…+ patch shortcut Add semi-plain text (simple) format for OKR content I/O, and a new `+patch` shortcut for incremental updates to objectives and key results.
syh-cpdsss
force-pushed
the
feat/okr-semi-plain-text-and-patch
branch
from
July 1, 2026 08:32
2661b9a to
3fc2a6c
Compare
zhangzq0
approved these changes
Jul 2, 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
This PR enhances the OKR skill with three major improvements: semi-plain text content format with mention position preservation, a new
+patchshortcut for partial updates, and simplified+cycle-listresponse with pre-computed active cycle detection. Additionally, it adds comprehensive skill documentation for native alignment and indicator APIs to guide agents through complex OKR workflows.Changes
shortcuts/okr/okr_openapi.go): IntroducedSemiPlainContentstruct and conversion methods that preserve mention positions when converting betweenContentBlockand simplified text format, enabling lossless round-trip conversions for OKR content editing.+patchshortcut (shortcuts/okr/okr_patch.go): Partial update shortcut for Objectives and Key Results supportingcontent,notes,score, anddeadlinefields. Supports bothsimple(semi-plain JSON) andrichtext(full ContentBlock) input styles with comprehensive validation.+cycle-listresponse (shortcuts/okr/okr_cycle_list.go): Removed rarely-used fields (create_time,update_time,owner,score,tenant_cycle_id) to reduce payload size. Addedcurrent_active_cyclesfield that pre-filters cycles where current time falls within the period and status isdefault/normal, eliminating the need for agents to manually compare timestamps.skills/lark-okr/references/lark-okr-alignments.md): Combined workflow documentation forobjective.alignments list,objective.alignments create, andalignments deletenative APIs with parameter tables, examples, and complete usage workflows.skills/lark-okr/references/lark-okr-indicators.md): Combined workflow documentation forobjective.indicators list,key_result.indicators list, andindicators patchnative APIs, including field descriptions, update restrictions, and when to use vs. the+indicator-updateshortcut.skills/lark-okr/SKILL.md): Added+patchshortcut to the shortcuts table, and linked new API guides in the API Resources section for alignments and indicators.+patchshortcut (1172 lines),isCurrentActiveCyclefunction (10 scenarios), and updated existing tests to reflect simplified response structures.+patchshortcut and progress shortcuts intests/cli_e2e/okr/.Test Plan
make unit-test)go vet ./...passesgofmt -l .produces no outputgo mod tidydoes not changego.mod/go.sumlark-cli okr <command>flow works as expected:lark-cli okr +patch --level objective --target-id <id> --content '{"text":"test","mention":[]}' --as userlark-cli okr +patch --level key-result --target-id <id> --score 0.7 --as userlark-cli okr +cycle-list --user-id <id> --as userreturns simplified structure withcurrent_active_cycleslark-cli okr objective.alignments list --objective-id <id> --as userlark-cli okr objective.indicators list --objective-id <id> --as usergo test ./tests/cli_e2e/okr/...)Related Issues
Summary by CodeRabbit
okr +patchto incrementally update objective/key result fields (supports--dry-run).--style(simple/richtext, defaultsimple) to+cycle-detailand progress commands (+progress-create/get/update), changing request/response and console output accordingly.+cycle-listwithcurrent_active_cycles(and “Current active cycle(s)” output when present).+patch, cycle list/detail, progress commands, and SemiPlainContent formatting behavior.