feat: add OKR domain with shortcuts, skill docs, and workflow#453
Open
yoho-dd wants to merge 1 commit intolarksuite:mainfrom
Open
feat: add OKR domain with shortcuts, skill docs, and workflow#453yoho-dd wants to merge 1 commit intolarksuite:mainfrom
yoho-dd wants to merge 1 commit intolarksuite:mainfrom
Conversation
|
|
📝 WalkthroughWalkthroughAdded a new OKR service domain with six CLI shortcuts (list, get, periods, progress-add, progress-get, review), supporting API calls, result normalization, error mapping, period detection, and rich-text handling; includes comprehensive unit tests and user-facing documentation. Changes
Sequence Diagram(s)mermaid Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
Greptile SummaryThis PR adds a complete
Confidence Score: 4/5Safe to merge after fixing the DryRun/Execute period inconsistency; all other findings are minor. One P1 finding remains: the DryRun output for +list omits the auto-detected period_ids, making --dry-run actively misleading for the most common use case (no explicit --period-id). The P2 page-size gap is minor. All prior P1 concerns from earlier threads have been resolved. shortcuts/okr/okr_list.go (DryRun/Execute period inconsistency) Important Files Changed
Sequence DiagramsequenceDiagram
participant User
participant CLI as lark-cli (okr shortcuts)
participant LarkAPI as Lark OKR API
User->>CLI: lark okr +list [--period-id P]
alt --period-id not provided
CLI->>LarkAPI: GET /okr/v1/periods (page_size=20)
LarkAPI-->>CLI: period list
CLI->>CLI: findCurrentPeriod()
end
CLI->>LarkAPI: GET /okr/v1/users/{uid}/okrs?period_ids=...
LarkAPI-->>CLI: OKR list
CLI-->>User: formatted output
User->>CLI: lark okr +progress-add --target-id X --target-type Y --text ...
CLI->>CLI: Validate (target-id/type required unless --data)
CLI->>CLI: buildProgressBody (flags override --data)
CLI->>LarkAPI: POST /okr/v1/progress_records
LarkAPI-->>CLI: progress_id
CLI-->>User: Progress record added successfully!
User->>CLI: lark okr +review --user-ids U --period-id P
CLI->>LarkAPI: GET /okr/v1/reviews/query?user_ids=U&period_ids=P
LarkAPI-->>CLI: review list
CLI-->>User: formatted review output
Reviews (2): Last reviewed commit: "feat: add OKR domain with shortcuts, ski..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Nitpick comments (3)
shortcuts/okr/okr_get_test.go (1)
96-99: Tighten validation assertion to check error intent.
err != nilalone can mask unrelated failures; assert the message indicates--okr-idsvalidation.Suggested tightening
err := runMountedOkrShortcut(t, shortcut, []string{"+get", "--okr-ids", "1,2,3,4,5,6,7,8,9,10,11", "--as", "bot"}, f, stdout) if err == nil { t.Fatal("expected validation error for too many IDs") } + if !strings.Contains(err.Error(), "--okr-ids") { + t.Fatalf("expected --okr-ids validation error, got: %v", err) + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@shortcuts/okr/okr_get_test.go` around lines 96 - 99, The test currently only checks that an error occurred when too many IDs are passed; update the assertion to verify the error is specifically about `--okr-ids` validation. After calling runMountedOkrShortcut in okr_get_test.go, assert err is non-nil and then check that err.Error() contains the `--okr-ids` substring (or the exact validation message your CLI returns); if it does not, fail the test with a message showing the actual error. Add a strings import if needed. Reference: runMountedOkrShortcut, the test around the err variable in okr_get_test.go.shortcuts/okr/okr_review_test.go (1)
97-100: Assert validation error content, not just existence.This currently passes on any error type. Please also assert the message indicates the
--user-idsvalidation path.Suggested tightening
err := runMountedOkrShortcut(t, shortcut, []string{"+review", "--user-ids", "1,2,3,4,5,6", "--period-id", "p1", "--as", "bot"}, f, stdout) if err == nil { t.Fatal("expected validation error for too many user IDs") } + if !strings.Contains(err.Error(), "--user-ids") { + t.Fatalf("expected --user-ids validation error, got: %v", err) + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@shortcuts/okr/okr_review_test.go` around lines 97 - 100, The test currently only checks that runMountedOkrShortcut returned an error; change it to assert the error message contains the user-IDs validation path so we verify the specific failure. After calling runMountedOkrShortcut (in okr_review_test.go with the call to runMountedOkrShortcut), replace the generic nil check with an assertion that err is non-nil and that err.Error() (or the returned error string) contains the `--user-ids` indicator (or the validation key like `user-ids`/`userIds`)—use the test helper you already use (t.Fatalf/require/assert) to fail the test and print the actual error when the expected substring is missing. Ensure you reference the same shortcut variable and inputs used in the failing call so only the message/content check is added.shortcuts/okr/okr_periods_test.go (1)
253-255: Remove the dummyinit()import workaround.This adds dead code just to keep
commonimported. It is cleaner to drop the unused import than to ship a no-op package initializer.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@shortcuts/okr/okr_periods_test.go` around lines 253 - 255, Remove the no-op init() that exists solely to reference common.File; instead delete the unused import of the common package from shortcuts/okr/okr_periods_test.go and remove the entire init() function (the symbol init and the dummy reference _ = common.File). Ensure any real uses of common are kept or replaced—if tests actually need utilities from the common package, import it properly and use the required identifiers, otherwise drop the import and init stub.
🤖 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_list.go`:
- Around line 180-209: detectCurrentPeriod currently only requests a single page
(page_size=20) via runtime.DoAPI and returns an empty string when no current
period is found, which allows silent fallback to all periods; update
detectCurrentPeriod to page through the periods API (use the response's
next/continuation token from the API result returned by HandleOkrApiResult) and
call findCurrentPeriod on each page until a current period is found, and if no
page yields a current period return a non-nil error (use the existing
WrapOkrError/ErrCode pattern) instead of "", so callers won't silently drop
period_ids; keep existing symbols detectCurrentPeriod, findCurrentPeriod,
HandleOkrApiResult and runtime.DoAPI to locate the logic.
- Around line 36-53: DryRun currently skips resolving the current period when
runtime.Str("period-id") is empty, producing a misleading preview compared to
Execute; update DryRun (the DryRun function in okr_list.go) to mirror Execute's
behavior: when runtime.Str("period-id") is empty, resolve the current period
(the same helper or logic Execute uses) and set params["period_ids"] to that
resolved id before building the common.NewDryRunAPI()
GET("/open-apis/okr/v1/users/"+userID+"/okrs").Params(params) request so the
dry-run output matches the real +list execution.
In `@shortcuts/okr/okr_periods_test.go`:
- Around line 227-235: TestDryRunNilSafety currently only checks s.DryRun != nil
and therefore can miss panics; change it to actually invoke s.DryRun with a
minimal RuntimeContext and fail the test if it panics (or alternatively
remove/rename the test). Locate TestDryRunNilSafety and the Shortcuts()
iteration, construct a minimal context object expected by DryRun (matching the
type used by the shortcuts), call s.DryRun(minimalCtx) inside the t.Run and use
defer/recover or testing helpers to assert no panic occurs (or explicitly
t.Fatal on panic), ensuring every non-nil s.DryRun is exercised rather than just
checked for nil.
In `@shortcuts/okr/okr_periods.go`:
- Around line 118-120: The pagination hint is being written to the formatted
output writer variable w using fmt.Fprintf(w, ...), which mixes advisory text
into the command's stdout; change the write target to stderr (e.g., use
os.Stderr or the command's error writer) so the hint "More periods available.
Use --page-token ..." is emitted to stderr when hasMore && pageToken != "";
update the fmt.Fprintf call that references hasMore, pageToken and w in the OKR
periods listing logic to write to stderr instead of w.
In `@shortcuts/okr/okr_progress_add.go`:
- Around line 21-63: The current code treats --data as a partial payload but
then unconditionally overrides fields (target_id, target_type, source_title,
source_url) which breaks the caller-supplied payload; change the behavior so
that when JSON is provided via runtime.Str("data") you do not overwrite values
already present in the parsed body: after json.Unmarshal into body, only set
body["target_id"], body["target_type"], body["source_title"], and
body["source_url"] if those keys are missing (use the runtime.Str flags as
fallbacks), and only call resolveTargetType(targetType) if you must set
target_type; likewise ensure content/text handling respects an explicit content
key in the provided JSON (only use --text/--content when body has no content).
This affects the code around variables body, runtime.Str("data"),
runtime.Str("target-id")/("target-type"), resolveTargetType,
wrapPlainTextContent, and the default source_* assignment.
In `@shortcuts/okr/okr_util.go`:
- Around line 97-117: HandleOkrApiResult currently ignores type-assertion
failures and can return nil,nil for malformed success payloads; update
HandleOkrApiResult to strictly validate the response shape: after confirming
code==0 (using util.ToFloat64 and larkCode), verify that resultMap["data"]
exists and is a map[string]interface{} (or expected type) and if not return a
descriptive error (use or add a helper similar to WrapOkrError or call
common.HandleApiResult) instead of returning nil,nil; also ensure non-present
"code" branch still returns errors from common.HandleApiResult and preserve
existing WrapOkrError behavior for non-zero larkCode.
In `@skills/lark-workflow-okr-review/SKILL.md`:
- Around line 32-49: The fenced workflow block that begins with "{周期} ─► okr
+periods ──► 获取周期 ID" lacks a language tag and triggers markdownlint MD040; add
a language identifier (e.g., change the opening triple backticks to ```text)
immediately before that diagram and keep the closing triple backticks as-is so
the block becomes a labeled code fence (```text ... ```).
---
Nitpick comments:
In `@shortcuts/okr/okr_get_test.go`:
- Around line 96-99: The test currently only checks that an error occurred when
too many IDs are passed; update the assertion to verify the error is
specifically about `--okr-ids` validation. After calling runMountedOkrShortcut
in okr_get_test.go, assert err is non-nil and then check that err.Error()
contains the `--okr-ids` substring (or the exact validation message your CLI
returns); if it does not, fail the test with a message showing the actual error.
Add a strings import if needed. Reference: runMountedOkrShortcut, the test
around the err variable in okr_get_test.go.
In `@shortcuts/okr/okr_periods_test.go`:
- Around line 253-255: Remove the no-op init() that exists solely to reference
common.File; instead delete the unused import of the common package from
shortcuts/okr/okr_periods_test.go and remove the entire init() function (the
symbol init and the dummy reference _ = common.File). Ensure any real uses of
common are kept or replaced—if tests actually need utilities from the common
package, import it properly and use the required identifiers, otherwise drop the
import and init stub.
In `@shortcuts/okr/okr_review_test.go`:
- Around line 97-100: The test currently only checks that runMountedOkrShortcut
returned an error; change it to assert the error message contains the user-IDs
validation path so we verify the specific failure. After calling
runMountedOkrShortcut (in okr_review_test.go with the call to
runMountedOkrShortcut), replace the generic nil check with an assertion that err
is non-nil and that err.Error() (or the returned error string) contains the
`--user-ids` indicator (or the validation key like `user-ids`/`userIds`)—use the
test helper you already use (t.Fatalf/require/assert) to fail the test and print
the actual error when the expected substring is missing. Ensure you reference
the same shortcut variable and inputs used in the failing call so only the
message/content check is added.
🪄 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: 310c4c4c-c5fe-41c9-9b78-0ba163dd2663
📒 Files selected for processing (26)
internal/registry/service_descriptions.jsonshortcuts/okr/okr_get.goshortcuts/okr/okr_get_test.goshortcuts/okr/okr_list.goshortcuts/okr/okr_list_test.goshortcuts/okr/okr_periods.goshortcuts/okr/okr_periods_test.goshortcuts/okr/okr_progress_add.goshortcuts/okr/okr_progress_add_test.goshortcuts/okr/okr_progress_get.goshortcuts/okr/okr_progress_get_test.goshortcuts/okr/okr_review.goshortcuts/okr/okr_review_test.goshortcuts/okr/okr_shortcut_test.goshortcuts/okr/okr_util.goshortcuts/okr/okr_util_test.goshortcuts/okr/shortcuts.goshortcuts/register.goskills/lark-okr/SKILL.mdskills/lark-okr/references/lark-okr-get.mdskills/lark-okr/references/lark-okr-list.mdskills/lark-okr/references/lark-okr-periods.mdskills/lark-okr/references/lark-okr-progress-add.mdskills/lark-okr/references/lark-okr-progress-get.mdskills/lark-okr/references/lark-okr-review.mdskills/lark-workflow-okr-review/SKILL.md
Comment on lines
+36
to
+53
| DryRun: func(ctx context.Context, runtime *common.RuntimeContext) *common.DryRunAPI { | ||
| userID := runtime.Str("user-id") | ||
| if userID == "" { | ||
| userID = runtime.Config.UserOpenId | ||
| } | ||
| params := map[string]interface{}{ | ||
| "user_id_type": "open_id", | ||
| "offset": runtime.Int("offset"), | ||
| "limit": runtime.Int("limit"), | ||
| "lang": runtime.Str("lang"), | ||
| } | ||
| if pid := runtime.Str("period-id"); pid != "" { | ||
| params["period_ids"] = pid | ||
| } | ||
| return common.NewDryRunAPI(). | ||
| GET("/open-apis/okr/v1/users/" + userID + "/okrs"). | ||
| Params(params) | ||
| }, |
There was a problem hiding this comment.
Keep DryRun consistent with the default execution path.
When --period-id is omitted, Execute first resolves the current period and then sends period_ids. DryRun skips that step and previews an unfiltered /okrs request, so the dry-run output is misleading for the normal +list flow.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@shortcuts/okr/okr_list.go` around lines 36 - 53, DryRun currently skips
resolving the current period when runtime.Str("period-id") is empty, producing a
misleading preview compared to Execute; update DryRun (the DryRun function in
okr_list.go) to mirror Execute's behavior: when runtime.Str("period-id") is
empty, resolve the current period (the same helper or logic Execute uses) and
set params["period_ids"] to that resolved id before building the
common.NewDryRunAPI()
GET("/open-apis/okr/v1/users/"+userID+"/okrs").Params(params) request so the
dry-run output matches the real +list execution.
Comment on lines
+180
to
+209
| func detectCurrentPeriod(runtime *common.RuntimeContext) (string, error) { | ||
| queryParams := make(larkcore.QueryParams) | ||
| queryParams.Set("page_size", "20") | ||
|
|
||
| apiResp, err := runtime.DoAPI(&larkcore.ApiReq{ | ||
| HttpMethod: http.MethodGet, | ||
| ApiPath: "/open-apis/okr/v1/periods", | ||
| QueryParams: queryParams, | ||
| }) | ||
|
|
||
| var result map[string]interface{} | ||
| if err == nil { | ||
| if parseErr := json.Unmarshal(apiResp.RawBody, &result); parseErr != nil { | ||
| return "", WrapOkrError(ErrCodeOkrInternalError, fmt.Sprintf("failed to parse periods response: %v", parseErr), "detect current period") | ||
| } | ||
| } | ||
|
|
||
| data, err := HandleOkrApiResult(result, err, "list periods for auto-detection") | ||
| if err != nil { | ||
| return "", err | ||
| } | ||
|
|
||
| items, _ := data["items"].([]interface{}) | ||
| current := findCurrentPeriod(items) | ||
| if current == nil { | ||
| return "", nil | ||
| } | ||
|
|
||
| id, _ := current["id"].(string) | ||
| return id, nil |
There was a problem hiding this comment.
Paginate period discovery and fail closed when no current period exists.
detectCurrentPeriod only inspects the first page (page_size=20) and returns "" with no error when nothing matches. That means tenants with more than 20 periods can miss the active one, and Execute will then silently drop period_ids and list OKRs across all periods instead of the current period.
Suggested direction
func detectCurrentPeriod(runtime *common.RuntimeContext) (string, error) {
- queryParams := make(larkcore.QueryParams)
- queryParams.Set("page_size", "20")
-
- apiResp, err := runtime.DoAPI(&larkcore.ApiReq{
- HttpMethod: http.MethodGet,
- ApiPath: "/open-apis/okr/v1/periods",
- QueryParams: queryParams,
- })
-
- var result map[string]interface{}
- if err == nil {
- if parseErr := json.Unmarshal(apiResp.RawBody, &result); parseErr != nil {
- return "", WrapOkrError(ErrCodeOkrInternalError, fmt.Sprintf("failed to parse periods response: %v", parseErr), "detect current period")
- }
- }
-
- data, err := HandleOkrApiResult(result, err, "list periods for auto-detection")
- if err != nil {
- return "", err
- }
-
- items, _ := data["items"].([]interface{})
- current := findCurrentPeriod(items)
- if current == nil {
- return "", nil
- }
-
- id, _ := current["id"].(string)
- return id, nil
+ pageToken := ""
+ for {
+ queryParams := make(larkcore.QueryParams)
+ queryParams.Set("page_size", "20")
+ if pageToken != "" {
+ queryParams.Set("page_token", pageToken)
+ }
+
+ apiResp, err := runtime.DoAPI(&larkcore.ApiReq{
+ HttpMethod: http.MethodGet,
+ ApiPath: "/open-apis/okr/v1/periods",
+ QueryParams: queryParams,
+ })
+
+ var result map[string]interface{}
+ if err == nil {
+ if parseErr := json.Unmarshal(apiResp.RawBody, &result); parseErr != nil {
+ return "", WrapOkrError(ErrCodeOkrInternalError, fmt.Sprintf("failed to parse periods response: %v", parseErr), "detect current period")
+ }
+ }
+
+ data, err := HandleOkrApiResult(result, err, "list periods for auto-detection")
+ if err != nil {
+ return "", err
+ }
+
+ items, _ := data["items"].([]interface{})
+ if current := findCurrentPeriod(items); current != nil {
+ id, _ := current["id"].(string)
+ return id, nil
+ }
+
+ hasMore, _ := data["has_more"].(bool)
+ nextToken, _ := data["page_token"].(string)
+ if !hasMore || nextToken == "" {
+ return "", fmt.Errorf("no current OKR period found; pass --period-id explicitly")
+ }
+ pageToken = nextToken
+ }
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| func detectCurrentPeriod(runtime *common.RuntimeContext) (string, error) { | |
| queryParams := make(larkcore.QueryParams) | |
| queryParams.Set("page_size", "20") | |
| apiResp, err := runtime.DoAPI(&larkcore.ApiReq{ | |
| HttpMethod: http.MethodGet, | |
| ApiPath: "/open-apis/okr/v1/periods", | |
| QueryParams: queryParams, | |
| }) | |
| var result map[string]interface{} | |
| if err == nil { | |
| if parseErr := json.Unmarshal(apiResp.RawBody, &result); parseErr != nil { | |
| return "", WrapOkrError(ErrCodeOkrInternalError, fmt.Sprintf("failed to parse periods response: %v", parseErr), "detect current period") | |
| } | |
| } | |
| data, err := HandleOkrApiResult(result, err, "list periods for auto-detection") | |
| if err != nil { | |
| return "", err | |
| } | |
| items, _ := data["items"].([]interface{}) | |
| current := findCurrentPeriod(items) | |
| if current == nil { | |
| return "", nil | |
| } | |
| id, _ := current["id"].(string) | |
| return id, nil | |
| func detectCurrentPeriod(runtime *common.RuntimeContext) (string, error) { | |
| pageToken := "" | |
| for { | |
| queryParams := make(larkcore.QueryParams) | |
| queryParams.Set("page_size", "20") | |
| if pageToken != "" { | |
| queryParams.Set("page_token", pageToken) | |
| } | |
| apiResp, err := runtime.DoAPI(&larkcore.ApiReq{ | |
| HttpMethod: http.MethodGet, | |
| ApiPath: "/open-apis/okr/v1/periods", | |
| QueryParams: queryParams, | |
| }) | |
| var result map[string]interface{} | |
| if err == nil { | |
| if parseErr := json.Unmarshal(apiResp.RawBody, &result); parseErr != nil { | |
| return "", WrapOkrError(ErrCodeOkrInternalError, fmt.Sprintf("failed to parse periods response: %v", parseErr), "detect current period") | |
| } | |
| } | |
| data, err := HandleOkrApiResult(result, err, "list periods for auto-detection") | |
| if err != nil { | |
| return "", err | |
| } | |
| items, _ := data["items"].([]interface{}) | |
| if current := findCurrentPeriod(items); current != nil { | |
| id, _ := current["id"].(string) | |
| return id, nil | |
| } | |
| hasMore, _ := data["has_more"].(bool) | |
| nextToken, _ := data["page_token"].(string) | |
| if !hasMore || nextToken == "" { | |
| return "", fmt.Errorf("no current OKR period found; pass --period-id explicitly") | |
| } | |
| pageToken = nextToken | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@shortcuts/okr/okr_list.go` around lines 180 - 209, detectCurrentPeriod
currently only requests a single page (page_size=20) via runtime.DoAPI and
returns an empty string when no current period is found, which allows silent
fallback to all periods; update detectCurrentPeriod to page through the periods
API (use the response's next/continuation token from the API result returned by
HandleOkrApiResult) and call findCurrentPeriod on each page until a current
period is found, and if no page yields a current period return a non-nil error
(use the existing WrapOkrError/ErrCode pattern) instead of "", so callers won't
silently drop period_ids; keep existing symbols detectCurrentPeriod,
findCurrentPeriod, HandleOkrApiResult and runtime.DoAPI to locate the logic.
Comment on lines
+227
to
+235
| func TestDryRunNilSafety(t *testing.T) { | ||
| for _, s := range Shortcuts() { | ||
| t.Run(s.Command, func(t *testing.T) { | ||
| // DryRun functions should not panic when called with basic RuntimeContext | ||
| // This just verifies the functions exist and are callable | ||
| if s.DryRun == nil { | ||
| t.Skipf("no DryRun for %s", s.Command) | ||
| } | ||
| }) |
There was a problem hiding this comment.
TestDryRunNilSafety never exercises DryRun.
This only re-checks s.DryRun != nil, so it can pass even if every DryRun panics. Either invoke s.DryRun(...) with a minimal test runtime here or rename/remove the test to avoid false confidence.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@shortcuts/okr/okr_periods_test.go` around lines 227 - 235,
TestDryRunNilSafety currently only checks s.DryRun != nil and therefore can miss
panics; change it to actually invoke s.DryRun with a minimal RuntimeContext and
fail the test if it panics (or alternatively remove/rename the test). Locate
TestDryRunNilSafety and the Shortcuts() iteration, construct a minimal context
object expected by DryRun (matching the type used by the shortcuts), call
s.DryRun(minimalCtx) inside the t.Run and use defer/recover or testing helpers
to assert no panic occurs (or explicitly t.Fatal on panic), ensuring every
non-nil s.DryRun is exercised rather than just checked for nil.
Comment on lines
+118
to
+120
| if hasMore && pageToken != "" { | ||
| fmt.Fprintf(w, "More periods available. Use --page-token %s to see next page.\n", pageToken) | ||
| } |
There was a problem hiding this comment.
Send the pagination hint to stderr instead of the result stream.
More periods available... is a hint, but it is currently written via the OutFormat writer. That mixes advisory text into the command’s formatted output; please route it to stderr instead.
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_periods.go` around lines 118 - 120, The pagination hint is
being written to the formatted output writer variable w using fmt.Fprintf(w,
...), which mixes advisory text into the command's stdout; change the write
target to stderr (e.g., use os.Stderr or the command's error writer) so the hint
"More periods available. Use --page-token ..." is emitted to stderr when hasMore
&& pageToken != ""; update the fmt.Fprintf call that references hasMore,
pageToken and w in the OKR periods listing logic to write to stderr instead of
w.
Comment on lines
+21
to
+63
| if dataStr := runtime.Str("data"); dataStr != "" { | ||
| if err := json.Unmarshal([]byte(dataStr), &body); err != nil { | ||
| return nil, fmt.Errorf("--data must be a valid JSON object: %v", err) | ||
| } | ||
| } | ||
|
|
||
| if targetID := runtime.Str("target-id"); targetID != "" { | ||
| body["target_id"] = targetID | ||
| } | ||
| if targetType := runtime.Str("target-type"); targetType != "" { | ||
| resolved, err := resolveTargetType(targetType) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
| body["target_type"] = resolved | ||
| } | ||
|
|
||
| // Handle content: --text for plain text, --content for rich text JSON | ||
| if text := runtime.Str("text"); text != "" { | ||
| body["content"] = wrapPlainTextContent(text) | ||
| } else if contentStr := runtime.Str("content"); contentStr != "" { | ||
| var content interface{} | ||
| if err := json.Unmarshal([]byte(contentStr), &content); err != nil { | ||
| return nil, fmt.Errorf("--content must be a valid JSON object: %v", err) | ||
| } | ||
| body["content"] = content | ||
| } | ||
|
|
||
| // Default source info | ||
| if _, ok := body["source_title"]; !ok { | ||
| sourceTitle := runtime.Str("source-title") | ||
| if sourceTitle == "" { | ||
| sourceTitle = "lark-cli" | ||
| } | ||
| body["source_title"] = sourceTitle | ||
| } | ||
| if _, ok := body["source_url"]; !ok { | ||
| sourceURL := runtime.Str("source-url") | ||
| if sourceURL == "" { | ||
| sourceURL = "https://github.com/larksuite/cli" | ||
| } | ||
| body["source_url"] = sourceURL | ||
| } |
There was a problem hiding this comment.
Make --data a true full-payload mode or drop the override claim.
Right now --data does not override the other flags: --target-id / --target-type are still mandatory, and after unmarshalling --data you overwrite it again with flag values and default source_* fields. That makes the CLI send a different payload than the caller supplied.
💡 One way to align the implementation with the current flag description
func buildProgressBody(runtime *common.RuntimeContext) (map[string]interface{}, error) {
body := make(map[string]interface{})
if dataStr := runtime.Str("data"); dataStr != "" {
if err := json.Unmarshal([]byte(dataStr), &body); err != nil {
return nil, fmt.Errorf("--data must be a valid JSON object: %v", err)
}
+ return body, nil
}
if targetID := runtime.Str("target-id"); targetID != "" {
body["target_id"] = targetID
}
@@
Flags: []common.Flag{
- {Name: "target-id", Desc: "target objective or key result ID", Required: true},
- {Name: "target-type", Desc: "target type: objective or key_result", Required: true, Enum: []string{"objective", "key_result", "2", "3"}},
+ {Name: "target-id", Desc: "target objective or key result ID"},
+ {Name: "target-type", Desc: "target type: objective or key_result", Enum: []string{"objective", "key_result", "2", "3"}},
@@
Validate: func(ctx context.Context, runtime *common.RuntimeContext) error {
+ if runtime.Str("data") == "" {
+ if runtime.Str("target-id") == "" || runtime.Str("target-type") == "" {
+ return fmt.Errorf("--target-id and --target-type are required unless --data is provided")
+ }
+ }
+
text := runtime.Str("text")
content := runtime.Str("content")
data := runtime.Str("data")Also applies to: 78-96
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@shortcuts/okr/okr_progress_add.go` around lines 21 - 63, The current code
treats --data as a partial payload but then unconditionally overrides fields
(target_id, target_type, source_title, source_url) which breaks the
caller-supplied payload; change the behavior so that when JSON is provided via
runtime.Str("data") you do not overwrite values already present in the parsed
body: after json.Unmarshal into body, only set body["target_id"],
body["target_type"], body["source_title"], and body["source_url"] if those keys
are missing (use the runtime.Str flags as fallbacks), and only call
resolveTargetType(targetType) if you must set target_type; likewise ensure
content/text handling respects an explicit content key in the provided JSON
(only use --text/--content when body has no content). This affects the code
around variables body, runtime.Str("data"),
runtime.Str("target-id")/("target-type"), resolveTargetType,
wrapPlainTextContent, and the default source_* assignment.
Comment on lines
+97
to
+117
| func HandleOkrApiResult(result interface{}, err error, action string) (map[string]interface{}, error) { | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
|
|
||
| resultMap, _ := result.(map[string]interface{}) | ||
| codeVal, hasCode := resultMap["code"] | ||
| if !hasCode { | ||
| data, err := common.HandleApiResult(result, err, action) | ||
| return data, err | ||
| } | ||
|
|
||
| code, _ := util.ToFloat64(codeVal) | ||
| larkCode := int(code) | ||
| if larkCode != 0 { | ||
| rawMsg, _ := resultMap["msg"].(string) | ||
| return nil, WrapOkrError(larkCode, rawMsg, action) | ||
| } | ||
|
|
||
| data, _ := resultMap["data"].(map[string]interface{}) | ||
| return data, nil |
There was a problem hiding this comment.
Reject malformed success payloads instead of treating them as empty success.
This helper currently drops both type-assertion failures. If the OKR API returns code=0 but omits data or changes its shape, callers will get nil, nil here and render an empty success result instead of surfacing a response-format error.
🛡️ Tighten the response-shape checks
func HandleOkrApiResult(result interface{}, err error, action string) (map[string]interface{}, error) {
if err != nil {
return nil, err
}
- resultMap, _ := result.(map[string]interface{})
+ resultMap, ok := result.(map[string]interface{})
+ if !ok {
+ return nil, WrapOkrError(ErrCodeOkrInternalError, "unexpected response format", action)
+ }
codeVal, hasCode := resultMap["code"]
if !hasCode {
data, err := common.HandleApiResult(result, err, action)
return data, err
}
@@
- data, _ := resultMap["data"].(map[string]interface{})
- return data, nil
+ data, ok := resultMap["data"].(map[string]interface{})
+ if !ok {
+ return nil, WrapOkrError(ErrCodeOkrInternalError, "response missing object data", action)
+ }
+ return data, nil
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@shortcuts/okr/okr_util.go` around lines 97 - 117, HandleOkrApiResult
currently ignores type-assertion failures and can return nil,nil for malformed
success payloads; update HandleOkrApiResult to strictly validate the response
shape: after confirming code==0 (using util.ToFloat64 and larkCode), verify that
resultMap["data"] exists and is a map[string]interface{} (or expected type) and
if not return a descriptive error (use or add a helper similar to WrapOkrError
or call common.HandleApiResult) instead of returning nil,nil; also ensure
non-present "code" branch still returns errors from common.HandleApiResult and
preserve existing WrapOkrError behavior for non-zero larkCode.
Comment on lines
+32
to
+49
| ``` | ||
| {周期} ─► okr +periods ──► 获取周期 ID | ||
| │ | ||
| ▼ | ||
| okr +list ──► OKR 列表（Objective + KR + 进度百分比） | ||
| │ | ||
| ▼ | ||
| okr +get ──► 各 OKR 详情（含 progress_record_list） | ||
| │ | ||
| ▼ | ||
| task +get-my-tasks ──► 关联任务完成情况（可选） | ||
| │ | ||
| ▼ | ||
| AI 汇总 ──► 结构化复盘报告 | ||
| │ | ||
| ▼ | ||
| doc +create ──► 写入飞书文档（可选） | ||
| ``` |
There was a problem hiding this comment.
Add a language tag to the fenced workflow block.
This currently triggers markdownlint MD040.
Suggested fix
-```
+```text
{周期} ─► okr +periods ──► 获取周期 ID
│
▼
okr +list ──► OKR 列表（Objective + KR + 进度百分比）
│
▼
okr +get ──► 各 OKR 详情（含 progress_record_list）
│
▼
task +get-my-tasks ──► 关联任务完成情况（可选）
│
▼
AI 汇总 ──► 结构化复盘报告
│
▼
doc +create ──► 写入飞书文档（可选）
-```
+```📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| ``` | |
| {周期} ─► okr +periods ──► 获取周期 ID | |
| │ | |
| ▼ | |
| okr +list ──► OKR 列表（Objective + KR + 进度百分比） | |
| │ | |
| ▼ | |
| okr +get ──► 各 OKR 详情（含 progress_record_list） | |
| │ | |
| ▼ | |
| task +get-my-tasks ──► 关联任务完成情况（可选） | |
| │ | |
| ▼ | |
| AI 汇总 ──► 结构化复盘报告 | |
| │ | |
| ▼ | |
| doc +create ──► 写入飞书文档（可选） | |
| ``` |
🧰 Tools
🪛 markdownlint-cli2 (0.22.0)
[warning] 32-32: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@skills/lark-workflow-okr-review/SKILL.md` around lines 32 - 49, The fenced
workflow block that begins with "{周期} ─► okr +periods ──► 获取周期 ID" lacks a
language tag and triggers markdownlint MD040; add a language identifier (e.g.,
change the opening triple backticks to ```text) immediately before that diagram
and keep the closing triple backticks as-is so the block becomes a labeled code
fence (```text ... ```).
Add comprehensive OKR (Objectives and Key Results) support to lark-cli with 6 shortcuts, skill documentation, and a review workflow: Shortcuts: - +list: List OKRs for a user (auto-detects current period) - +get: Batch get OKR details by ID(s) - +periods: List OKR periods - +progress-add: Add a progress record to an objective or key result - +progress-list: Get a progress record by ID - +review: Query OKR reviews Also includes: - OKR service description in registry - lark-okr skill with SKILL.md and 6 reference docs (Chinese) - lark-workflow-okr-review workflow skill - Full test coverage (28 tests with race detection)
yoho-dd
force-pushed
the
claude/research-okr-lark-cli-3CZec
branch
from
c3078ac to
36262a2
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (4)
shortcuts/okr/okr_list.go (2)
36-53:⚠️ Potential issue | 🟡 MinorKeep
DryRunaligned with the default execution path.When
--period-idis omitted,Executeresolves the current period and usually sendsperiod_ids, butDryRunpreviews an unfiltered request. That makes the dry-run output misleading for the normal+listflow.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@shortcuts/okr/okr_list.go` around lines 36 - 53, The DryRun implementation in okr_list.go diverges from the real Execute path by not including the resolved current period when "--period-id" is omitted; update DryRun to mirror Execute's behavior: when runtime.Str("period-id") is empty, call the same resolver used by Execute to obtain the current period id (or compute the default period id) and set params["period_ids"] to that id, then return common.NewDryRunAPI().GET(...).Params(params) so the dry-run preview matches the actual +list request; modify the DryRun function to reuse the existing period-resolution helper (the same code/path Execute uses) rather than leaving period_ids out.
183-212:⚠️ Potential issue | 🟠 MajorCurrent-period auto-detection can silently fall back to “all periods.”
detectCurrentPeriodonly inspects the first page and returns""when nothing matches.Executethen skipsperiod_ids, so tenants with many periods—or no detectable active period—quietly get a cross-period OKR list instead of a current-period result. Please paginate throughhas_more/page_tokenand fail closed when no current period is found.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@shortcuts/okr/okr_list.go` around lines 183 - 212, detectCurrentPeriod currently only checks the first page and returns "" if no active period is found, causing a silent fall-back to all-period results; update detectCurrentPeriod to paginate using the QueryParams "page_size" and "page_token" and to loop calling runtime.DoAPI with ApiReq until the response's "has_more" is false or findCurrentPeriod(items) returns non-nil, calling HandleOkrApiResult for each page to validate results; if no current period is found after exhausting pages, return a non-empty error (i.e., fail closed) instead of "" so callers know detection failed.shortcuts/okr/okr_periods.go (1)
118-120:⚠️ Potential issue | 🟡 MinorRoute the pagination hint to stderr.
This message is a hint, not result data. Writing it via the formatted output writer contaminates stdout/json output; please send it to the command’s stderr writer instead.
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_periods.go` around lines 118 - 120, The pagination hint is being written to the stdout writer variable 'w' (if hasMore && pageToken != ""), which contaminates command output; instead write that message to the command's stderr writer (do not use 'w'). Locate the block around the hasMore/pageToken check in okr_periods.go and replace fmt.Fprintf(w, ...) with writing to the command's stderr (e.g., obtain the stderr writer the command provides—ErrOrStderr()/cmd.Err() or an errWriter variable—and call fmt.Fprintf(errWriter, "More periods available. Use --page-token %s to see next page.\n", pageToken) or fmt.Fprintln accordingly) so only result data goes to stdout.shortcuts/okr/okr_periods_test.go (1)
227-235:⚠️ Potential issue | 🟡 MinorThis test never exercises
DryRun.Right now it only re-checks that
s.DryRunis non-nil, so it still passes if aDryRunpanics. Please either invoke eachDryRunwith a minimal runtime/context here or rename/remove the test to avoid false confidence.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@shortcuts/okr/okr_periods_test.go` around lines 227 - 235, TestDryRunNilSafety currently only checks s.DryRun != nil and never calls it; update the test to actually invoke each s.DryRun to catch panics by creating and passing a minimal valid RuntimeContext (or use the package's test helper/new constructor for RuntimeContext if available) when iterating Shortcuts(), e.g., for each s where s.DryRun != nil call s.DryRun(minimalCtx) (and optionally wrap with recovery or use assert/require NotPanics) so the test verifies DryRun is callable without panicking; alternatively remove/rename the test if you prefer not to execute DryRun.
🤖 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_review.go`:
- Around line 84-93: HandleOkrApiResult currently does an unchecked type
assertion on data["review_list"] into reviewList which treats malformed or
missing payloads as an empty result; update the code to validate presence and
type of data["review_list"] (use the comma-ok form when assigning to reviewList)
and if the key is missing or not a []interface{} return or surface an
error/diagnostic instead of falling back to "No OKR reviews found." Ensure
runtime.OutFormat receives a clear error message or non-success response when
review_list is malformed so callers/operators can detect the upstream contract
break.
In `@skills/lark-okr/SKILL.md`:
- Around line 26-30: The note claiming "复盘查询最多 5 个 user_id 和 5 个 period_id"
incorrectly implies the `+review` CLI supports multiple period IDs; update
SKILL.md to reflect the current CLI surface by rewording that sentence to state
that while the backend supports up to 5 user_ids and 5 period_ids, the `+review`
command currently accepts a single `--period-id` (include the `+review` and
`--period-id` symbols verbatim) — alternatively, if you choose to implement
multi-period input, add multi-`--period-id` parsing to the `+review` handler and
update the docs accordingly.
- Around line 21-24: The docs claim --source-url must start with http/https but
the CLI handler for the OKR progress add command does not enforce that; either
update the docs or add validation in the CLI code that handles the --source-url
flag (the OKR progress add command handler) to check that the provided
--source-url string starts with "http://" or "https://", and if not either
prepend a default scheme (e.g., "https://") or return a user-facing error;
implement the check where the command parses flags/arguments (the OKR progress
add command's Run/RunE handler that reads --source-url) so behavior and
documentation match.
---
Duplicate comments:
In `@shortcuts/okr/okr_list.go`:
- Around line 36-53: The DryRun implementation in okr_list.go diverges from the
real Execute path by not including the resolved current period when
"--period-id" is omitted; update DryRun to mirror Execute's behavior: when
runtime.Str("period-id") is empty, call the same resolver used by Execute to
obtain the current period id (or compute the default period id) and set
params["period_ids"] to that id, then return
common.NewDryRunAPI().GET(...).Params(params) so the dry-run preview matches the
actual +list request; modify the DryRun function to reuse the existing
period-resolution helper (the same code/path Execute uses) rather than leaving
period_ids out.
- Around line 183-212: detectCurrentPeriod currently only checks the first page
and returns "" if no active period is found, causing a silent fall-back to
all-period results; update detectCurrentPeriod to paginate using the QueryParams
"page_size" and "page_token" and to loop calling runtime.DoAPI with ApiReq until
the response's "has_more" is false or findCurrentPeriod(items) returns non-nil,
calling HandleOkrApiResult for each page to validate results; if no current
period is found after exhausting pages, return a non-empty error (i.e., fail
closed) instead of "" so callers know detection failed.
In `@shortcuts/okr/okr_periods_test.go`:
- Around line 227-235: TestDryRunNilSafety currently only checks s.DryRun != nil
and never calls it; update the test to actually invoke each s.DryRun to catch
panics by creating and passing a minimal valid RuntimeContext (or use the
package's test helper/new constructor for RuntimeContext if available) when
iterating Shortcuts(), e.g., for each s where s.DryRun != nil call
s.DryRun(minimalCtx) (and optionally wrap with recovery or use assert/require
NotPanics) so the test verifies DryRun is callable without panicking;
alternatively remove/rename the test if you prefer not to execute DryRun.
In `@shortcuts/okr/okr_periods.go`:
- Around line 118-120: The pagination hint is being written to the stdout writer
variable 'w' (if hasMore && pageToken != ""), which contaminates command output;
instead write that message to the command's stderr writer (do not use 'w').
Locate the block around the hasMore/pageToken check in okr_periods.go and
replace fmt.Fprintf(w, ...) with writing to the command's stderr (e.g., obtain
the stderr writer the command provides—ErrOrStderr()/cmd.Err() or an errWriter
variable—and call fmt.Fprintf(errWriter, "More periods available. Use
--page-token %s to see next page.\n", pageToken) or fmt.Fprintln accordingly) so
only result data goes to stdout.
🪄 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: 6b159f12-7a14-4e6c-96ff-570bcff3cbfd
📒 Files selected for processing (26)
internal/registry/service_descriptions.jsonshortcuts/okr/okr_get.goshortcuts/okr/okr_get_test.goshortcuts/okr/okr_list.goshortcuts/okr/okr_list_test.goshortcuts/okr/okr_periods.goshortcuts/okr/okr_periods_test.goshortcuts/okr/okr_progress_add.goshortcuts/okr/okr_progress_add_test.goshortcuts/okr/okr_progress_get.goshortcuts/okr/okr_progress_get_test.goshortcuts/okr/okr_review.goshortcuts/okr/okr_review_test.goshortcuts/okr/okr_shortcut_test.goshortcuts/okr/okr_util.goshortcuts/okr/okr_util_test.goshortcuts/okr/shortcuts.goshortcuts/register.goskills/lark-okr/SKILL.mdskills/lark-okr/references/lark-okr-get.mdskills/lark-okr/references/lark-okr-list.mdskills/lark-okr/references/lark-okr-periods.mdskills/lark-okr/references/lark-okr-progress-add.mdskills/lark-okr/references/lark-okr-progress-get.mdskills/lark-okr/references/lark-okr-review.mdskills/lark-workflow-okr-review/SKILL.md
✅ Files skipped from review due to trivial changes (13)
- internal/registry/service_descriptions.json
- skills/lark-okr/references/lark-okr-review.md
- skills/lark-okr/references/lark-okr-progress-get.md
- shortcuts/okr/okr_review_test.go
- skills/lark-okr/references/lark-okr-periods.md
- skills/lark-okr/references/lark-okr-get.md
- shortcuts/okr/okr_shortcut_test.go
- skills/lark-okr/references/lark-okr-list.md
- skills/lark-workflow-okr-review/SKILL.md
- shortcuts/okr/okr_util_test.go
- skills/lark-okr/references/lark-okr-progress-add.md
- shortcuts/okr/okr_get_test.go
- shortcuts/okr/shortcuts.go
🚧 Files skipped from review as they are similar to previous changes (8)
- shortcuts/register.go
- shortcuts/okr/okr_progress_add_test.go
- shortcuts/okr/okr_progress_get_test.go
- shortcuts/okr/okr_list_test.go
- shortcuts/okr/okr_get.go
- shortcuts/okr/okr_progress_get.go
- shortcuts/okr/okr_progress_add.go
- shortcuts/okr/okr_util.go
Comment on lines
+84
to
+93
| reviewList, _ := data["review_list"].([]interface{}) | ||
|
|
||
| outData := map[string]interface{}{ | ||
| "review_list": reviewList, | ||
| } | ||
|
|
||
| runtime.OutFormat(outData, nil, func(w io.Writer) { | ||
| if len(reviewList) == 0 { | ||
| fmt.Fprintln(w, "No OKR reviews found.") | ||
| return |
There was a problem hiding this comment.
Don’t collapse malformed responses into “No OKR reviews found.”
HandleOkrApiResult does not guarantee that review_list exists or has the expected type. With the unchecked assertion here, a bad success payload is treated as an empty result, which hides an upstream contract break from users.
Suggested guard
- reviewList, _ := data["review_list"].([]interface{})
+ reviewRaw, ok := data["review_list"]
+ if !ok {
+ return WrapOkrError(ErrCodeOkrInternalError, "missing review_list in response", "query OKR reviews")
+ }
+ reviewList, ok := reviewRaw.([]interface{})
+ if !ok {
+ return WrapOkrError(ErrCodeOkrInternalError, "invalid review_list type in response", "query OKR reviews")
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| reviewList, _ := data["review_list"].([]interface{}) | |
| outData := map[string]interface{}{ | |
| "review_list": reviewList, | |
| } | |
| runtime.OutFormat(outData, nil, func(w io.Writer) { | |
| if len(reviewList) == 0 { | |
| fmt.Fprintln(w, "No OKR reviews found.") | |
| return | |
| reviewRaw, ok := data["review_list"] | |
| if !ok { | |
| return WrapOkrError(ErrCodeOkrInternalError, "missing review_list in response", "query OKR reviews") | |
| } | |
| reviewList, ok := reviewRaw.([]interface{}) | |
| if !ok { | |
| return WrapOkrError(ErrCodeOkrInternalError, "invalid review_list type in response", "query OKR reviews") | |
| } | |
| outData := map[string]interface{}{ | |
| "review_list": reviewList, | |
| } | |
| runtime.OutFormat(outData, nil, func(w io.Writer) { | |
| if len(reviewList) == 0 { | |
| fmt.Fprintln(w, "No OKR reviews found.") | |
| return |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@shortcuts/okr/okr_review.go` around lines 84 - 93, HandleOkrApiResult
currently does an unchecked type assertion on data["review_list"] into
reviewList which treats malformed or missing payloads as an empty result; update
the code to validate presence and type of data["review_list"] (use the comma-ok
form when assigning to reviewList) and if the key is missing or not a
[]interface{} return or surface an error/diagnostic instead of falling back to
"No OKR reviews found." Ensure runtime.OutFormat receives a clear error message
or non-success response when review_list is malformed so callers/operators can
detect the upstream contract break.
Comment on lines
+21
to
+24
| > **进展记录注意**： | ||
| > 1. 创建进展记录需提供 `target_id`（Objective 或 KR 的 ID）和 `--target-type`（`objective` 或 `key_result`）。 | ||
| > 2. `--text` 提供纯文本输入，会自动转换为飞书富文本格式；`--content` 支持完整的富文本 JSON。 | ||
| > 3. 进展记录需要 `source_url`（必须是 http/https 开头的 URL），默认使用 lark-cli 的 GitHub 地址。 |
There was a problem hiding this comment.
This overstates --source-url validation.
The CLI currently defaults source_url, but it does not enforce an http/https prefix in shortcuts/okr/okr_progress_add.go. Please either relax this wording or add the missing validation in code so the skill doc matches actual behavior.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@skills/lark-okr/SKILL.md` around lines 21 - 24, The docs claim --source-url
must start with http/https but the CLI handler for the OKR progress add command
does not enforce that; either update the docs or add validation in the CLI code
that handles the --source-url flag (the OKR progress add command handler) to
check that the provided --source-url string starts with "http://" or "https://",
and if not either prepend a default scheme (e.g., "https://") or return a
user-facing error; implement the check where the command parses flags/arguments
(the OKR progress add command's Run/RunE handler that reads --source-url) so
behavior and documentation match.
Comment on lines
+26
to
+30
| > **查询注意**： | ||
| > 1. OKR 列表接口最多每页返回 10 条（`--limit` 最大为 10）。 | ||
| > 2. `batch_get` 接口最多同时查询 10 个 OKR ID。 | ||
| > 3. 复盘查询最多 5 个 user_id 和 5 个 period_id。 | ||
| > 4. 所有时间戳为**毫秒**级 Unix 时间。 |
There was a problem hiding this comment.
The review-query note implies multi-period support that the CLI doesn’t expose.
+review currently accepts a single --period-id, so “最多 5 个 period_id” reads as a supported multi-period CLI input even though users cannot provide it here. Please reword this to match the current shortcut surface, or add multi-period support to +review.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@skills/lark-okr/SKILL.md` around lines 26 - 30, The note claiming "复盘查询最多 5 个
user_id 和 5 个 period_id" incorrectly implies the `+review` CLI supports multiple
period IDs; update SKILL.md to reflect the current CLI surface by rewording that
sentence to state that while the backend supports up to 5 user_ids and 5
period_ids, the `+review` command currently accepts a single `--period-id`
(include the `+review` and `--period-id` symbols verbatim) — alternatively, if
you choose to implement multi-period input, add multi-`--period-id` parsing to
the `+review` handler and update the docs accordingly.
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
3 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.
Add comprehensive OKR (Objectives and Key Results) support to lark-cli with 6 shortcuts, skill documentation, and a review workflow:
Shortcuts:
Also includes:
Summary
Changes
Test Plan
lark xxxcommand works as expectedRelated Issues
Summary by CodeRabbit
New Features
Documentation