docs(skills): slim SKILL.md context and token usage across domains#1452
docs(skills): slim SKILL.md context and token usage across domains#1452liangshuo-1 wants to merge 7 commits into
Conversation
…erences Rewrite the always-loaded SKILL.md from a 168-line monolith into a slim core: a positioning line plus 7 mental-model "通用准则" ordered by agent attention priority (silent/proactive rules first; loud-triggered and low-frequency ones last), and a short routing list. Mechanics and edge cases move into on-demand references/ (loaded only when relevant). References (named with the lark-shared- prefix, matching the per-domain convention): - lark-shared-auth-split-flow.md split-flow steps (marked must-read) - lark-shared-high-risk-approval.md exit-10 envelope forms + predict/preview - lark-shared-identity-and-permissions.md identity model + scope recovery - lark-shared-config-init.md first-run config (blocking, no split-flow) - lark-shared-update-notice.md _notice handling (update/skills/deprecated) Fix doc-vs-implementation drift confirmed against the code: - exit-10 keys on exit code 10, not the type string; covers both the flat (type=confirmation_required) and typed (type=confirmation + subtype) envelopes, and reads the confirm flag from hint (--yes / --force). - distinguish permission_violations (raw API) vs missing_scopes (CLI error). - complete _notice keys (update / skills / deprecated_command). - identity failure is silent-or-loud per command, not always empty. Switch description to Chinese; bump version 1.0.0 -> 1.1.0. Change-Id: I2dff478ecdc05a13f2d750944f637ed2374961e7
Change-Id: I4068d821adf1b249d00950a8f6ff53414825aeff
Regenerate lark-im SKILL.md (4253 -> 2040 tokens, tiktoken cl100k_base, -52%) via per-domain gen-skills flags. Drop the scope permission table and the duplicated schema-usage note, compact API Resources to a terse identity index, render the Shortcuts table as one-line routing, and move Shortcuts ahead of the concept sections (executable-first). Full detail stays in --help / schema (the source of truth); Go Desc and --help are unchanged. Add the NOT boundary, a good/bad sender example, Flag-type rationale, and anti-hallucination guidance with a safe fallback (guide users to the Feishu client; do not auto-execute high-risk raw-API writes). The master template is variabilized with defaults preserving current output, so other domains regenerate byte-identical (verified). Eval (skillave, IM, 9 cases x 3 runs, lark-cli-eval-analyze two-table compare): mean pass_rate 0.939 -> 0.969, duration -22%, tool-calls -26%, zero structural regression; anti-hallucination case 0.58 -> 1.00. Depends on larksuite-cli-registry change (skill-meta.yaml per-domain flags + gen-skills.py); must merge together with (or before) this PR. Change-Id: I33c62664388542bfd8ba85ea06c1c5a493c0d935
Drop the redundant Feed-pin parenthetical from the SKILL.md frontmatter description so it fits the non-waivable <=200-token skill-quality gate (206 -> 194, cl100k_base). Behavior-neutral: only the WHEN-routing copy is shortened; skill body and commands are unchanged. Mirrors the same one-line trim in larksuite-cli-registry skill-meta.yaml (im.description).
Change-Id: I40c3d0429b1352949b5b09a79c5700b26fca955d
Squashed net diff of PR #1426 (github-base-token-improve): slim lark-base SKILL.md + formula/workflow references, split the workflow guide/schema into per-step references, add guess-tolerant flag aliases and misuse hints for base record shortcuts (with containment-based suggest), and drive import adjustments. Combined onto eval/skills-combined alongside #1450 #1389 #1395 #1410. Change-Id: Iab1f0c4f1a4c93fd9dbd49bc702cb0ef41022bda
…s structure - references (im/vc/minutes/whiteboard): drop key catalogs, field/scope tables and other content already emitted by `event list`/`event schema`; keep only jq recipes, domain semantic gotchas, required params, and per-key identity. ~55% token cut across the four references (4343 -> 1940, cl100k_base). - vc: document the 3 previously-undocumented recording keys, then replace the hardcoded key catalog with a pointer to `event list` to stop doc-vs-code drift. - SKILL.md Topic index: reword rows to advertise recipes/gotchas and route structure lookups to the CLI, matching the slimmed references. Validated by a command-correctness eval (skill+CLI available): 21/21 across the four domains, identity correct per key, whiteboard required `-p whiteboard_id` preserved; the VC recording-transcript key (previously unanswerable from the skill alone) now resolves. Change-Id: I666e9706ae6ef3e2bbcd56a3fb70c4f8be94182c
📝 WalkthroughWalkthroughThis PR implements batch field listing ( ChangesUnified Field Operations and Flag Improvements with Documentation Restructure
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
Suggested reviewers
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
|
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #1452 +/- ##
==========================================
+ Coverage 72.88% 73.34% +0.45%
==========================================
Files 738 751 +13
Lines 69553 69450 -103
==========================================
+ Hits 50695 50939 +244
+ Misses 15043 14755 -288
+ Partials 3815 3756 -59 ☔ 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@cbd729757bbdf8ad140d2de39006a5c251a652a2🧩 Skill updatenpx skills add larksuite/cli#eval/skills-combined -y -g |
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (7)
internal/suggest/suggest.go (1)
107-120: ⚡ Quick winEliminate repeated rune conversions.
len([]rune(a))andlen([]rune(b))are each called multiple times (lines 116, 119), allocating new rune slices on every call. Compute the rune slices once and reuse them.♻️ Proposed refactor
func containsSegment(a, b string) bool { a = strings.TrimLeft(a, "+-") b = strings.TrimLeft(b, "+-") + ra, rb := []rune(a), []rune(b) - if len([]rune(a)) > len([]rune(b)) { + if len(ra) > len(rb) { a, b = b, a + ra, rb = rb, ra } - return len([]rune(a)) >= 5 && strings.Contains(b, a) + return len(ra) >= 5 && strings.Contains(b, a) }🤖 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 `@internal/suggest/suggest.go` around lines 107 - 120, The containsSegment function repeatedly converts strings to []rune causing allocations; modify containsSegment to compute runesA := []rune(a) and runesB := []rune(b) once (after trimming "+/-"), use len(runesA) and len(runesB) and strings.Contains on the original b/a strings as appropriate, and swap runes (and the original string variables if you prefer) when you need to ensure runesA is the shorter side so the subsequent length check and containment use the precomputed rune lengths instead of calling len([]rune(...)) multiple times.skills/lark-base/references/lark-base-workflow-guide.md (2)
82-83: ⚡ Quick winClarify the ChangeRecordTrigger guidance.
Lines 82–83 recommend using a single
ChangeRecordTriggerwithcondition_listfor scenarios where "修改为 X 或 新增 X" (modify to X or create as X). This is a significant design choice that differs from the naive "create two workflows" approach.This guidance is consistent with
lark-base-workflow-schema.mdline 67, which restates the same rule. However, to help users trust and apply this guidance, consider adding a brief example or explanation of whyChangeRecordTrigger + condition_listis superior (e.g., "single entry point, simpler maintenance, explicit intent").🤖 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-base/references/lark-base-workflow-guide.md` around lines 82 - 83, Update the guidance around ChangeRecordTrigger to explicitly recommend a single ChangeRecordTrigger with condition_list for "修改为 X 或 新增 X" scenarios: state that using ChangeRecordTrigger plus condition_list provides a single entry point, simpler maintenance, and clearer intent versus separate AddRecordTrigger and SetRecordTrigger workflows, and add a concise example description showing condition_list matching both "modify to X" and "create as X" cases (reference ChangeRecordTrigger and condition_list).
7-14: ⚡ Quick winConsolidate read-order and decision-tree guidance across guide and schema.
Both
lark-base-workflow-guide.md(lines 7–14, "先判断任务类型" table) andlark-base-workflow-schema.md(lines 5–10, "读取顺序") describe the decision path for when to consult which resource. The concepts are similar but phrased differently:
- Guide: Task-type table (list/get/enable/modify/create/explain) with "是否读 schema" column
- Schema: Numbered read-order steps (1. query/enable/disable, 2. create/update, 3. determine step types, 4. add common-types-and-refs)
While both are useful (guide for task lookup, schema for workflow construction), there is some potential for users to feel confused about which guidance to follow first.
Consider adding a brief sentence at the top of each file clarifying the intended entry point:
- Guide: "Start here if you know what task you want to perform (list/modify/create/etc.)"
- Schema: "Once in guide, use this for detailed JSON structure and step type reference"
This would prevent the perception of duplication and make the division of labor clearer.
🤖 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-base/references/lark-base-workflow-guide.md` around lines 7 - 14, Add a one-line clarifying entry sentence to each file: in skills/lark-base/references/lark-base-workflow-guide.md (lines 7-14) prepend the table with "Start here if you know what task you want to perform (list/modify/create/etc.)." and in skills/lark-base/references/lark-base-workflow-schema.md (lines 5-10) prepend the numbered read-order with "Use this after consulting the guide above for detailed JSON structure and step-type reference." — make only those single-line insertions so the guide remains the task-first entry point and the schema remains the detailed, follow-up reference.skills/lark-base/references/lark-base-workflow-schema.md (1)
33-33: ⚡ Quick winClarify the "连线/扩展/输入" principle.
Line 33 states: "总原则:连线写
children,扩展标识写meta,输入参数写data."This is a good organizing principle, but the guide shows JSON examples in
lark-base-workflow-guide.mdlines 104–133 that contain onlyid,type,title,next, anddata. There is nometafield visible in the minimal example or in any of the 6 provided action step files.Please clarify:
- Is
metacurrently in use in any step type, or is it a placeholder for future extension?- Should users see
metain their examples, or is it optional/implementation-internal?Adding a brief note here (e.g., "Note:
metais reserved for future extension; most workflows do not use it") would reduce confusion.🤖 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-base/references/lark-base-workflow-schema.md` at line 33, Add a one-sentence clarification under the principle line stating that `meta` is reserved for extensions and is optional/implementation-internal (most existing step types and the minimal examples only use `id`, `type`, `title`, `next`, `data`, and do not include `meta`), and optionally note that if a step type uses `meta` it will be documented in that step's spec; update the sentence near "总原则:连线写 `children`,扩展标识写 `meta`,输入参数写 `data`" to include this brief note.skills/lark-base/references/workflow-steps/trigger-lark-message.md (1)
1-40: ⚖️ Poor tradeoffAll four trigger reference pages omit output documentation—critical for workflow authors needing to reference trigger outputs downstream.
Each of these trigger references documents configuration inputs completely but lacks an outputs section defining the variables available for subsequent steps. The upstream contracts in
common-types-and-refs.mddefine these outputs; they should be reflected in each trigger's reference page for discoverability and completeness.
trigger-lark-message.md#L1-L40: Add outputs section documentingSender,AtUser,SenderGroup,MessageSendTime,MessageContent,MessageType,MessageID,MessageLink,ParentID,ThreadID,Attachmentswith constraint thatSenderGroup/MessageLinkare unavailable for single-chat.trigger-reminder.md#L1-L30: Add outputs section documenting shared record trigger outputs:{fieldId}(with sub-properties),startTime,recordId,recordLink,recordCreatedUser,recordCreatedTime,recordModifiedUser,recordModifiedTime, plus dynamic field outputs.trigger-set-record.md#L1-L38: Add outputs section documenting shared record trigger outputs (same as ReminderTrigger).trigger-timer.md#L1-L27: Add outputs section documentingscheduleTimeavailable output.Verification needed: Confirm whether output documentation should be added to each trigger file (recommended for user discoverability) or whether centralization in
common-types-and-refs.mdis the intended design. If per-file outputs are preferred, generate diffs for all four files with appropriate output sections based on the upstream contract definitions.🤖 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-base/references/workflow-steps/trigger-lark-message.md` around lines 1 - 40, Add per-trigger "Outputs" documentation sections to surface downstream variables defined by the upstream contracts instead of relying only on common-types-and-refs.md: in skills/lark-base/references/workflow-steps/trigger-lark-message.md (lines 1-40) insert an Outputs section documenting Sender, AtUser, SenderGroup (note: SenderGroup and MessageLink not present for single-chat), MessageSendTime, MessageContent, MessageType, MessageID, MessageLink, ParentID, ThreadID, Attachments; in skills/lark-base/references/workflow-steps/trigger-reminder.md (lines 1-30) add Outputs documenting shared record trigger outputs: each dynamic {fieldId} with sub-properties (id, value, fieldType), startTime, recordId, recordLink, recordCreatedUser, recordCreatedTime, recordModifiedUser, recordModifiedTime and mention dynamic field outputs; in skills/lark-base/references/workflow-steps/trigger-set-record.md (lines 1-38) add the same shared record Outputs as ReminderTrigger; and in skills/lark-base/references/workflow-steps/trigger-timer.md (lines 1-27) add an Outputs section documenting scheduleTime; ensure each new section briefly states types and any chat-scene constraints and references common-types-and-refs.md for full schemas.shortcuts/base/record_ops.go (1)
53-53: 💤 Low valueConsider improving error message readability.
The error message
--record-id/--record-ids and --json are mutually exclusivecould be more readable as--record-id / --record-ids and --json are mutually exclusive(with spaces around the slash).🤖 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/base/record_ops.go` at line 53, The error message passed to baseFlagErrorf in the record selection code should be made more readable by adding spaces around the slash; update the string in the return that constructs the error (the call to baseFlagErrorf that currently reads "--record-id/--record-ids and --json are mutually exclusive") to "--record-id / --record-ids and --json are mutually exclusive" (the surrounding code returning recordSelection{} and the baseFlagErrorf call remain the same).shortcuts/base/dashboard_block_get_data.go (1)
22-22: ⚡ Quick winHidden flag lacks validation, creating inconsistency with similar pattern.
Unlike
BaseDataQuery(which adds a hiddentable-idflag and explicitly rejects it inValidate), this command registers a hiddendashboard-idflag but does not validate or reject it. The flag is never referenced inDryRunorExecute, so it would be silently ignored if provided.Since the Tips (Line 27) state "This command does not need --dashboard-id", consider either:
- Adding validation to reject
--dashboard-idwith a helpful error (matching thebase_data_query.gopattern), or- Removing the flag entirely if it serves no purpose.
🤖 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/base/dashboard_block_get_data.go` at line 22, The hidden flag "dashboard-id" (registered as {Name: "dashboard-id", Hidden: true}) is never validated and will be silently ignored; follow the BaseDataQuery pattern by updating the command's Validate method to detect when the user passed --dashboard-id and return a clear error (e.g., "this command does not accept --dashboard-id"), so that Validate rejects the flag rather than ignoring it; alternatively remove the hidden flag registration if you prefer that behavior—apply the change in the command's Validate implementation (and keep DryRun/Execute unchanged).
🤖 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 `@internal/suggest/suggest.go`:
- Around line 107-120: Add a new table-driven unit test in
internal/suggest/suggest_test.go that directly exercises containsSegment: create
cases for (1) sigil-trimmed containment (e.g. "+block-list" vs
"+base-block-list" should return true), (2) the minimum-length guard (a short
fragment like "list" vs "+base-list" should return false because it’s < 5
runes), and (3) a non-containment case that returns false; iterate the table,
call containsSegment(a,b) and assert expected boolean for each case so the sigil
stripping, rune-length check, and substring logic are covered.
In `@shortcuts/base/base_execute_test.go`:
- Around line 1069-1191: Add a new unit test in
shortcuts/base/base_execute_test.go that exercises the single-table compact
branch for +field-list: use newExecuteFactory, register an httpmock.Stub for GET
/.../tables/<tbl>/fields returning a field with "options" containing extra keys
(e.g. name and color), call runShortcut with BaseFieldList and args including
"--compact" and the table id, and assert the output retains the option "name"
but omits extra option fields (e.g. no "color"); this will cover the compact
projection logic added in the single-list branch in field_ops.go (the code path
invoked by BaseFieldList with "--compact").
In `@shortcuts/base/base_shortcuts_test.go`:
- Around line 72-77: Replace substring-only error assertions with typed-error
metadata and cause checks: for shortcuts/base/base_shortcuts_test.go (lines
72-77) change the check of BaseFieldSearchOptions.Validate to use
errs.ProblemOf(err, "validation", "required", "field-id") (or the equivalent
project helper) and assert the Param by doing errors.As(err,
*errs.ValidationError) to inspect Param == "field-id", and also assert the
wrapped cause if applicable; for shortcuts/base/base_shortcuts_test.go (lines
1116-1123) replace the `"use only one"` substring assertion with errs.ProblemOf
checking the appropriate category/subtype/param for the mutually-exclusive alias
error and use errors.As to verify the ValidationError.Param and any cause; for
shortcuts/base/helpers_test.go (lines 545-549) replace the keyword+query
conflict substring check with errs.ProblemOf for the conflict error, use
errors.As to assert the ValidationError.Param equals "keyword" (or the correct
param name), and verify the original cause is preserved via errors.Unwrap /
errors.Is as appropriate. Use the symbols BaseFieldSearchOptions.Validate,
errs.ProblemOf, and *errs.ValidationError in your changes.
In `@shortcuts/base/field_ops.go`:
- Around line 244-246: isBaseTableID currently returns true for any ref that
simply starts with "tbl", causing names like "tbl Orders" to be misclassified;
update isBaseTableID to perform a stricter shape check (for example match a
well-defined ID regex such as ^tbl_[A-Za-z0-9-]+$ or ^tbl:[A-Za-z0-9]+$) and
return false for anything that doesn't match so the normal resolution path is
used as a fallback; ensure the function name is kept (isBaseTableID) so callers
continue to use it and test with example inputs like "tbl123", "tbl_abc-1", and
"tbl Orders" to verify behavior.
In `@shortcuts/base/table_ops.go`:
- Around line 190-201: The pagination break logic in listEveryTable is
incorrectly using the returned total even when listAllTables falls back to the
current page size, causing early termination; update the condition so total is
only treated as authoritative when it exceeds the current batch size — i.e.,
change the break clause to use (total > len(batch) && len(items) >= total)
instead of (total > 0 && len(items) >= total), leaving the other checks
(len(batch) == 0 || len(batch) < pageLimit) unchanged; this ensures
listEveryTable and its use of listAllTables correctly page through all tables
for name-based resolution.
In `@skills/lark-base/references/workflow-steps/trigger-change-record.md`:
- Around line 5-11: The JSON example uses the wrong field name "condition" —
update the example in trigger-change-record.md so the payload uses
"condition_list" (e.g., "condition_list": null or an appropriate array) instead
of "condition" and ensure the example matches the field table; verify the JSON
block containing "table_name", "trigger_control_list" and the condition field is
corrected to use "condition_list".
---
Nitpick comments:
In `@internal/suggest/suggest.go`:
- Around line 107-120: The containsSegment function repeatedly converts strings
to []rune causing allocations; modify containsSegment to compute runesA :=
[]rune(a) and runesB := []rune(b) once (after trimming "+/-"), use len(runesA)
and len(runesB) and strings.Contains on the original b/a strings as appropriate,
and swap runes (and the original string variables if you prefer) when you need
to ensure runesA is the shorter side so the subsequent length check and
containment use the precomputed rune lengths instead of calling len([]rune(...))
multiple times.
In `@shortcuts/base/dashboard_block_get_data.go`:
- Line 22: The hidden flag "dashboard-id" (registered as {Name: "dashboard-id",
Hidden: true}) is never validated and will be silently ignored; follow the
BaseDataQuery pattern by updating the command's Validate method to detect when
the user passed --dashboard-id and return a clear error (e.g., "this command
does not accept --dashboard-id"), so that Validate rejects the flag rather than
ignoring it; alternatively remove the hidden flag registration if you prefer
that behavior—apply the change in the command's Validate implementation (and
keep DryRun/Execute unchanged).
In `@shortcuts/base/record_ops.go`:
- Line 53: The error message passed to baseFlagErrorf in the record selection
code should be made more readable by adding spaces around the slash; update the
string in the return that constructs the error (the call to baseFlagErrorf that
currently reads "--record-id/--record-ids and --json are mutually exclusive") to
"--record-id / --record-ids and --json are mutually exclusive" (the surrounding
code returning recordSelection{} and the baseFlagErrorf call remain the same).
In `@skills/lark-base/references/lark-base-workflow-guide.md`:
- Around line 82-83: Update the guidance around ChangeRecordTrigger to
explicitly recommend a single ChangeRecordTrigger with condition_list for "修改为 X
或 新增 X" scenarios: state that using ChangeRecordTrigger plus condition_list
provides a single entry point, simpler maintenance, and clearer intent versus
separate AddRecordTrigger and SetRecordTrigger workflows, and add a concise
example description showing condition_list matching both "modify to X" and
"create as X" cases (reference ChangeRecordTrigger and condition_list).
- Around line 7-14: Add a one-line clarifying entry sentence to each file: in
skills/lark-base/references/lark-base-workflow-guide.md (lines 7-14) prepend the
table with "Start here if you know what task you want to perform
(list/modify/create/etc.)." and in
skills/lark-base/references/lark-base-workflow-schema.md (lines 5-10) prepend
the numbered read-order with "Use this after consulting the guide above for
detailed JSON structure and step-type reference." — make only those single-line
insertions so the guide remains the task-first entry point and the schema
remains the detailed, follow-up reference.
In `@skills/lark-base/references/lark-base-workflow-schema.md`:
- Line 33: Add a one-sentence clarification under the principle line stating
that `meta` is reserved for extensions and is optional/implementation-internal
(most existing step types and the minimal examples only use `id`, `type`,
`title`, `next`, `data`, and do not include `meta`), and optionally note that if
a step type uses `meta` it will be documented in that step's spec; update the
sentence near "总原则:连线写 `children`,扩展标识写 `meta`,输入参数写 `data`" to include this
brief note.
In `@skills/lark-base/references/workflow-steps/trigger-lark-message.md`:
- Around line 1-40: Add per-trigger "Outputs" documentation sections to surface
downstream variables defined by the upstream contracts instead of relying only
on common-types-and-refs.md: in
skills/lark-base/references/workflow-steps/trigger-lark-message.md (lines 1-40)
insert an Outputs section documenting Sender, AtUser, SenderGroup (note:
SenderGroup and MessageLink not present for single-chat), MessageSendTime,
MessageContent, MessageType, MessageID, MessageLink, ParentID, ThreadID,
Attachments; in skills/lark-base/references/workflow-steps/trigger-reminder.md
(lines 1-30) add Outputs documenting shared record trigger outputs: each dynamic
{fieldId} with sub-properties (id, value, fieldType), startTime, recordId,
recordLink, recordCreatedUser, recordCreatedTime, recordModifiedUser,
recordModifiedTime and mention dynamic field outputs; in
skills/lark-base/references/workflow-steps/trigger-set-record.md (lines 1-38)
add the same shared record Outputs as ReminderTrigger; and in
skills/lark-base/references/workflow-steps/trigger-timer.md (lines 1-27) add an
Outputs section documenting scheduleTime; ensure each new section briefly states
types and any chat-scene constraints and references common-types-and-refs.md for
full schemas.
🪄 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: b83fcce8-0697-4f50-a530-2a7d2266815c
📒 Files selected for processing (68)
internal/suggest/suggest.goshortcuts/base/base_data_query.goshortcuts/base/base_dryrun_ops_test.goshortcuts/base/base_execute_test.goshortcuts/base/base_shortcuts_test.goshortcuts/base/dashboard_block_get_data.goshortcuts/base/field_list.goshortcuts/base/field_list_batch.goshortcuts/base/field_ops.goshortcuts/base/field_search_options.goshortcuts/base/helpers_test.goshortcuts/base/record_get.goshortcuts/base/record_list.goshortcuts/base/record_ops.goshortcuts/base/record_query.goshortcuts/base/record_search.goshortcuts/base/shortcuts.goshortcuts/base/table_ops.goshortcuts/drive/drive_import.goshortcuts/drive/drive_import_test.goskill-template/domains/im.mdskill-template/skill-template.mdskills/lark-base/SKILL.mdskills/lark-base/references/dashboard-block-data-config.mdskills/lark-base/references/formula-examples.mdskills/lark-base/references/formula-field-guide.mdskills/lark-base/references/formula-functions-extended.mdskills/lark-base/references/lark-base-cell-value.mdskills/lark-base/references/lark-base-dashboard-usecase.mdskills/lark-base/references/lark-base-dashboard.mdskills/lark-base/references/lark-base-field-json.mdskills/lark-base/references/lark-base-workflow-guide.mdskills/lark-base/references/lark-base-workflow-schema.mdskills/lark-base/references/workflow-steps/action-add-record.mdskills/lark-base/references/workflow-steps/action-delay.mdskills/lark-base/references/workflow-steps/action-find-record.mdskills/lark-base/references/workflow-steps/action-generate-ai-text.mdskills/lark-base/references/workflow-steps/action-http-client.mdskills/lark-base/references/workflow-steps/action-lark-message.mdskills/lark-base/references/workflow-steps/action-set-record.mdskills/lark-base/references/workflow-steps/branch-if-else.mdskills/lark-base/references/workflow-steps/branch-switch.mdskills/lark-base/references/workflow-steps/common-types-and-refs.mdskills/lark-base/references/workflow-steps/system-loop.mdskills/lark-base/references/workflow-steps/trigger-add-record.mdskills/lark-base/references/workflow-steps/trigger-button.mdskills/lark-base/references/workflow-steps/trigger-change-record.mdskills/lark-base/references/workflow-steps/trigger-lark-message.mdskills/lark-base/references/workflow-steps/trigger-reminder.mdskills/lark-base/references/workflow-steps/trigger-set-record.mdskills/lark-base/references/workflow-steps/trigger-timer.mdskills/lark-drive/references/lark-drive-import.mdskills/lark-event/SKILL.mdskills/lark-event/references/lark-event-im.mdskills/lark-event/references/lark-event-minutes.mdskills/lark-event/references/lark-event-vc.mdskills/lark-event/references/lark-event-whiteboard.mdskills/lark-im/SKILL.mdskills/lark-mail/SKILL.mdskills/lark-shared/SKILL.mdskills/lark-shared/references/lark-shared-auth-split-flow.mdskills/lark-shared/references/lark-shared-config-init.mdskills/lark-shared/references/lark-shared-high-risk-approval.mdskills/lark-shared/references/lark-shared-identity-and-permissions.mdskills/lark-shared/references/lark-shared-update-notice.mdskills/lark-task/SKILL.mdskills/lark-task/references/lark-task-complete.mdskills/lark-task/references/lark-task-create.md
| // containsSegment reports whether one name contains the other as a substring | ||
| // after stripping the "+"/"--" sigils. It catches hallucinated names that drop | ||
| // a namespace prefix (e.g. "+block-list" for "+base-block-list"), which share | ||
| // almost no prefix and sit far beyond the edit-distance budget. The shorter | ||
| // side must be at least 5 runes so generic fragments like "list" do not match | ||
| // half the catalog. | ||
| func containsSegment(a, b string) bool { | ||
| a = strings.TrimLeft(a, "+-") | ||
| b = strings.TrimLeft(b, "+-") | ||
| if len([]rune(a)) > len([]rune(b)) { | ||
| a, b = b, a | ||
| } | ||
| return len([]rune(a)) >= 5 && strings.Contains(b, a) | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Check for tests covering containsSegment.
# Test: Search for containsSegment in test files.
rg -n 'containsSegment' --glob '*_test.go'Repository: larksuite/cli
Length of output: 39
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# 1) Find call sites
echo "== Call sites in suggest.go =="
rg -n 'containsSegment\(' internal/suggest/suggest.go
# 2) Locate the specific test and show surrounding content
echo "== TestClosest_HallucinatedSharesPrefix in suggest_test.go =="
rg -n 'TestClosest_HallucinatedSharesPrefix' internal/suggest/suggest_test.go
sed -n '1,220p' internal/suggest/suggest_test.go | sed -n '/TestClosest_HallucinatedSharesPrefix/,$p' | head -n 200Repository: larksuite/cli
Length of output: 2347
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== suggest.go around containsSegment call =="
sed -n '1,200p' internal/suggest/suggest.go | sed -n '40,160p'
echo "== Remaining tests in suggest_test.go (beyond first 220 lines if any) =="
wc -l internal/suggest/suggest_test.go
sed -n '1,260p' internal/suggest/suggest_test.goRepository: larksuite/cli
Length of output: 5267
Add direct unit tests for containsSegment
internal/suggest/suggest_test.go has no tests that exercise containsSegment itself (no coverage for the sigil-trimmed substring logic or the “>= 5 runes” minimum), so cases like "+block-list" vs "+base-block-list" are not asserted. Add a table-driven test for containsSegment covering: containment after trimming ("+block-list" / "+base-block-list"), the minimum-length guard (reject short fragments like "list"), and non-containment.
🤖 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 `@internal/suggest/suggest.go` around lines 107 - 120, Add a new table-driven
unit test in internal/suggest/suggest_test.go that directly exercises
containsSegment: create cases for (1) sigil-trimmed containment (e.g.
"+block-list" vs "+base-block-list" should return true), (2) the minimum-length
guard (a short fragment like "list" vs "+base-list" should return false because
it’s < 5 runes), and (3) a non-containment case that returns false; iterate the
table, call containsSegment(a,b) and assert expected boolean for each case so
the sigil stripping, rune-length check, and substring logic are covered.
| t.Run("list resolves table name", func(t *testing.T) { | ||
| factory, stdout, reg := newExecuteFactory(t) | ||
| reg.Register(&httpmock.Stub{ | ||
| Method: "GET", | ||
| URL: "/open-apis/base/v3/bases/app_x/tables", | ||
| Body: map[string]interface{}{ | ||
| "code": 0, | ||
| "data": map[string]interface{}{"tables": []interface{}{ | ||
| map[string]interface{}{"id": "tbl_orders", "name": "Orders"}, | ||
| }, "total": 1}, | ||
| }, | ||
| }) | ||
| reg.Register(&httpmock.Stub{ | ||
| Method: "GET", | ||
| URL: "/open-apis/base/v3/bases/app_x/tables/tbl_orders/fields", | ||
| Body: map[string]interface{}{ | ||
| "code": 0, | ||
| "data": map[string]interface{}{"fields": []interface{}{ | ||
| map[string]interface{}{"id": "fld_name", "name": "Name", "type": "text"}, | ||
| }, "total": 1}, | ||
| }, | ||
| }) | ||
| if err := runShortcut(t, BaseFieldList, []string{"+field-list", "--base-token", "app_x", "--table-id", "Orders"}, factory, stdout); err != nil { | ||
| t.Fatalf("err=%v", err) | ||
| } | ||
| if got := stdout.String(); !strings.Contains(got, `"fields"`) || !strings.Contains(got, `"name": "Name"`) { | ||
| t.Fatalf("stdout=%s", got) | ||
| } | ||
| }) | ||
|
|
||
| t.Run("list batch multiple tables", func(t *testing.T) { | ||
| factory, stdout, reg := newExecuteFactory(t) | ||
| reg.Register(&httpmock.Stub{ | ||
| Method: "GET", | ||
| URL: "/open-apis/base/v3/bases/app_x/tables/tbl_a/fields", | ||
| Body: map[string]interface{}{ | ||
| "code": 0, | ||
| "data": map[string]interface{}{"fields": []interface{}{ | ||
| map[string]interface{}{"id": "fld_a", "name": "Name", "type": "text", "style": map[string]interface{}{"type": "plain"}}, | ||
| }, "total": 1}, | ||
| }, | ||
| }) | ||
| reg.Register(&httpmock.Stub{ | ||
| Method: "GET", | ||
| URL: "/open-apis/base/v3/bases/app_x/tables/tbl_b/fields", | ||
| Body: map[string]interface{}{ | ||
| "code": 0, | ||
| "data": map[string]interface{}{"fields": []interface{}{ | ||
| map[string]interface{}{"id": "fld_b", "name": "Status", "type": "select", "options": []interface{}{map[string]interface{}{"name": "Todo", "color": "red"}}}, | ||
| }, "total": 1}, | ||
| }, | ||
| }) | ||
| if err := runShortcut(t, BaseFieldListBatch, []string{"+field-list-batch", "--base-token", "app_x", "--table-id", "tbl_a", "--table-id", "tbl_b", "--compact"}, factory, stdout); err != nil { | ||
| t.Fatalf("err=%v", err) | ||
| } | ||
| got := stdout.String() | ||
| if !strings.Contains(got, `"tables"`) || !strings.Contains(got, `"table_id": "tbl_a"`) || !strings.Contains(got, `"table_id": "tbl_b"`) || !strings.Contains(got, `"options": [`) || !strings.Contains(got, `"Todo"`) || !strings.Contains(got, `"style"`) || strings.Contains(got, `"color"`) { | ||
| t.Fatalf("stdout=%s", got) | ||
| } | ||
| }) | ||
|
|
||
| t.Run("list batch resolves table names", func(t *testing.T) { | ||
| factory, stdout, reg := newExecuteFactory(t) | ||
| reg.Register(&httpmock.Stub{ | ||
| Method: "GET", | ||
| URL: "/open-apis/base/v3/bases/app_x/tables", | ||
| Body: map[string]interface{}{ | ||
| "code": 0, | ||
| "data": map[string]interface{}{"tables": []interface{}{ | ||
| map[string]interface{}{"id": "tbl_orders", "name": "Orders"}, | ||
| }, "total": 1}, | ||
| }, | ||
| }) | ||
| reg.Register(&httpmock.Stub{ | ||
| Method: "GET", | ||
| URL: "/open-apis/base/v3/bases/app_x/tables/tbl_a/fields", | ||
| Body: map[string]interface{}{ | ||
| "code": 0, | ||
| "data": map[string]interface{}{"fields": []interface{}{ | ||
| map[string]interface{}{"id": "fld_a", "name": "Name", "type": "text"}, | ||
| }, "total": 1}, | ||
| }, | ||
| }) | ||
| reg.Register(&httpmock.Stub{ | ||
| Method: "GET", | ||
| URL: "/open-apis/base/v3/bases/app_x/tables/tbl_orders/fields", | ||
| Body: map[string]interface{}{ | ||
| "code": 0, | ||
| "data": map[string]interface{}{"fields": []interface{}{ | ||
| map[string]interface{}{"id": "fld_order", "name": "Status", "type": "select"}, | ||
| }, "total": 1}, | ||
| }, | ||
| }) | ||
| if err := runShortcut(t, BaseFieldListBatch, []string{"+field-list-batch", "--base-token", "app_x", "--table-id", "tbl_a", "--table-id", "Orders"}, factory, stdout); err != nil { | ||
| t.Fatalf("err=%v", err) | ||
| } | ||
| got := stdout.String() | ||
| if !strings.Contains(got, `"table_id": "tbl_a"`) || !strings.Contains(got, `"table_id": "tbl_orders"`) || !strings.Contains(got, `"table_ref": "Orders"`) || !strings.Contains(got, `"table_name": "Orders"`) { | ||
| t.Fatalf("stdout=%s", got) | ||
| } | ||
| }) | ||
|
|
||
| t.Run("list batch default keeps full fields", func(t *testing.T) { | ||
| factory, stdout, reg := newExecuteFactory(t) | ||
| reg.Register(&httpmock.Stub{ | ||
| Method: "GET", | ||
| URL: "/open-apis/base/v3/bases/app_x/tables/tbl_b/fields", | ||
| Body: map[string]interface{}{ | ||
| "code": 0, | ||
| "data": map[string]interface{}{"fields": []interface{}{ | ||
| map[string]interface{}{"id": "fld_b", "name": "Status", "type": "select", "options": []interface{}{map[string]interface{}{"name": "Todo", "color": "red"}}}, | ||
| }, "total": 1}, | ||
| }, | ||
| }) | ||
| if err := runShortcut(t, BaseFieldListBatch, []string{"+field-list-batch", "--base-token", "app_x", "--table-id", "tbl_b"}, factory, stdout); err != nil { | ||
| t.Fatalf("err=%v", err) | ||
| } | ||
| got := stdout.String() | ||
| if !strings.Contains(got, `"table_id": "tbl_b"`) || !strings.Contains(got, `"color": "red"`) { | ||
| t.Fatalf("stdout=%s", got) | ||
| } | ||
| }) | ||
|
|
There was a problem hiding this comment.
Add direct execute coverage for single-table +field-list --compact.
These additions cover batch compact/default, but the new single-list compact branch (shortcuts/base/field_ops.go, Line 156-158) is still untested in this file. Please add one case asserting projection behavior (for example, select option name retained while extra option fields are omitted).
🧪 Suggested test skeleton
t.Run("list resolves table name", func(t *testing.T) {
...
})
+t.Run("list compact projects field payload", func(t *testing.T) {
+ factory, stdout, reg := newExecuteFactory(t)
+ reg.Register(&httpmock.Stub{
+ Method: "GET",
+ URL: "/open-apis/base/v3/bases/app_x/tables/tbl_x/fields",
+ Body: map[string]interface{}{
+ "code": 0,
+ "data": map[string]interface{}{"fields": []interface{}{
+ map[string]interface{}{
+ "id": "fld_status", "name": "Status", "type": "select",
+ "options": []interface{}{map[string]interface{}{"name": "Todo", "color": "red"}},
+ },
+ }, "total": 1},
+ },
+ })
+ if err := runShortcut(t, BaseFieldList, []string{"+field-list", "--base-token", "app_x", "--table-id", "tbl_x", "--compact"}, factory, stdout); err != nil {
+ t.Fatalf("err=%v", err)
+ }
+ got := stdout.String()
+ if !strings.Contains(got, `"options": [`) || !strings.Contains(got, `"Todo"`) || strings.Contains(got, `"color"`) {
+ t.Fatalf("stdout=%s", got)
+ }
+})
+
t.Run("list batch multiple tables", func(t *testing.T) {
...
})🤖 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/base/base_execute_test.go` around lines 1069 - 1191, Add a new unit
test in shortcuts/base/base_execute_test.go that exercises the single-table
compact branch for +field-list: use newExecuteFactory, register an httpmock.Stub
for GET /.../tables/<tbl>/fields returning a field with "options" containing
extra keys (e.g. name and color), call runShortcut with BaseFieldList and args
including "--compact" and the table id, and assert the output retains the option
"name" but omits extra option fields (e.g. no "color"); this will cover the
compact projection logic added in the single-list branch in field_ops.go (the
code path invoked by BaseFieldList with "--compact").
Source: Coding guidelines
| func TestFieldSearchOptionsRequiresFieldRef(t *testing.T) { | ||
| err := BaseFieldSearchOptions.Validate(context.Background(), newBaseTestRuntime(map[string]string{}, nil, nil)) | ||
| if err == nil || !strings.Contains(err.Error(), "--field-id is required") { | ||
| t.Fatalf("err=%v", err) | ||
| } | ||
| } |
There was a problem hiding this comment.
New error-path tests should assert typed error metadata, not message substrings. The shared root cause is substring-only checks on err.Error(), which can pass even when error classification (category/subtype/param) or cause chaining regresses.
shortcuts/base/base_shortcuts_test.go#L72-L77: replace substring matching witherrs.ProblemOfassertions anderrors.As(..., *errs.ValidationError)forParam, plus cause checks.shortcuts/base/base_shortcuts_test.go#L1116-L1123: assert typed validation metadata for the mutually-exclusive alias failure instead of"use only one"substring.shortcuts/base/helpers_test.go#L545-L549: use typed metadata assertions for thekeyword+queryconflict path and verify cause propagation.
As per coding guidelines, **/*_test.go: “Error-path tests must assert typed metadata via errs.ProblemOf (category / subtype / param) and cause preservation, not message substrings alone.” Based on learnings, Param must be asserted via errors.As on *errs.ValidationError.
📍 Affects 2 files
shortcuts/base/base_shortcuts_test.go#L72-L77(this comment)shortcuts/base/base_shortcuts_test.go#L1116-L1123shortcuts/base/helpers_test.go#L545-L549
🤖 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/base/base_shortcuts_test.go` around lines 72 - 77, Replace
substring-only error assertions with typed-error metadata and cause checks: for
shortcuts/base/base_shortcuts_test.go (lines 72-77) change the check of
BaseFieldSearchOptions.Validate to use errs.ProblemOf(err, "validation",
"required", "field-id") (or the equivalent project helper) and assert the Param
by doing errors.As(err, *errs.ValidationError) to inspect Param == "field-id",
and also assert the wrapped cause if applicable; for
shortcuts/base/base_shortcuts_test.go (lines 1116-1123) replace the `"use only
one"` substring assertion with errs.ProblemOf checking the appropriate
category/subtype/param for the mutually-exclusive alias error and use errors.As
to verify the ValidationError.Param and any cause; for
shortcuts/base/helpers_test.go (lines 545-549) replace the keyword+query
conflict substring check with errs.ProblemOf for the conflict error, use
errors.As to assert the ValidationError.Param equals "keyword" (or the correct
param name), and verify the original cause is preserved via errors.Unwrap /
errors.Is as appropriate. Use the symbols BaseFieldSearchOptions.Validate,
errs.ProblemOf, and *errs.ValidationError in your changes.
Sources: Coding guidelines, Learnings
| func isBaseTableID(ref string) bool { | ||
| return strings.HasPrefix(strings.TrimSpace(ref), "tbl") | ||
| } |
There was a problem hiding this comment.
Harden table-ID detection to avoid false positives on table names.
Line 245 treats any ref starting with tbl as an ID. Names like tbl Orders will bypass resolution and fail later API calls (for example at Line 176 / Line 149). Use a stricter ID shape check (or fallback resolution) instead of prefix-only matching.
💡 Suggested fix
func isBaseTableID(ref string) bool {
- return strings.HasPrefix(strings.TrimSpace(ref), "tbl")
+ ref = strings.TrimSpace(ref)
+ if ref == "" {
+ return false
+ }
+ // Keep ID detection strict enough to avoid misclassifying human-readable table names.
+ return strings.HasPrefix(ref, "tbl_") && !strings.ContainsAny(ref, " \t/")
}🤖 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/base/field_ops.go` around lines 244 - 246, isBaseTableID currently
returns true for any ref that simply starts with "tbl", causing names like "tbl
Orders" to be misclassified; update isBaseTableID to perform a stricter shape
check (for example match a well-defined ID regex such as ^tbl_[A-Za-z0-9-]+$ or
^tbl:[A-Za-z0-9]+$) and return false for anything that doesn't match so the
normal resolution path is used as a fallback; ensure the function name is kept
(isBaseTableID) so callers continue to use it and test with example inputs like
"tbl123", "tbl_abc-1", and "tbl Orders" to verify behavior.
| func listEveryTable(runtime *common.RuntimeContext, baseToken string) ([]map[string]interface{}, error) { | ||
| const pageLimit = 100 | ||
| offset := 0 | ||
| items := []map[string]interface{}{} | ||
| for { | ||
| batch, total, err := listAllTables(runtime, baseToken, offset, pageLimit) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
| items = append(items, batch...) | ||
| if len(batch) == 0 || len(batch) < pageLimit || (total > 0 && len(items) >= total) { | ||
| break |
There was a problem hiding this comment.
Pagination can stop after the first full page when total is omitted.
listAllTables falls back total to current page size when the API omits it. With a full first page (100) and more tables available, the len(items) >= total condition can break early and miss tables for name-based resolution in +field-list-batch.
💡 Suggested fix
- if len(batch) == 0 || len(batch) < pageLimit || (total > 0 && len(items) >= total) {
+ if len(batch) == 0 || len(batch) < pageLimit {
break
}🤖 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/base/table_ops.go` around lines 190 - 201, The pagination break
logic in listEveryTable is incorrectly using the returned total even when
listAllTables falls back to the current page size, causing early termination;
update the condition so total is only treated as authoritative when it exceeds
the current batch size — i.e., change the break clause to use (total >
len(batch) && len(items) >= total) instead of (total > 0 && len(items) >=
total), leaving the other checks (len(batch) == 0 || len(batch) < pageLimit)
unchanged; this ensures listEveryTable and its use of listAllTables correctly
page through all tables for name-based resolution.
| ```json | ||
| { | ||
| "table_name": "任务表", | ||
| "trigger_control_list": [], | ||
| "condition": null | ||
| } | ||
| ``` |
There was a problem hiding this comment.
Fix JSON example: field should be condition_list, not condition.
The JSON example uses "condition": null (line 9), but the field table (line 17) documents the correct field name as condition_list. This mismatch will cause confusion when users try to construct actual workflow JSON payloads.
🔧 Proposed fix
{
"table_name": "任务表",
"trigger_control_list": [],
- "condition": null
+ "condition_list": []
}📝 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.
| ```json | |
| { | |
| "table_name": "任务表", | |
| "trigger_control_list": [], | |
| "condition": null | |
| } | |
| ``` |
🤖 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-base/references/workflow-steps/trigger-change-record.md` around
lines 5 - 11, The JSON example uses the wrong field name "condition" — update
the example in trigger-change-record.md so the payload uses "condition_list"
(e.g., "condition_list": null or an appropriate array) instead of "condition"
and ensure the example matches the field table; verify the JSON block containing
"table_name", "trigger_control_list" and the condition field is corrected to use
"condition_list".
精简多个域 skill 的常驻上下文,降低 token 占用并提升路由 / 可执行性:
--help/schema