feat(base): improve workflow update execution guidance - #2126
feat(base): improve workflow update execution guidance#2126TanDianxiong wants to merge 1 commit into
Conversation
|
|
github-actions
Bot
added
domain/base
📝 WalkthroughWalkthroughWorkflow shortcuts now document explicit write operations, post-write verification, reminder offset direction, and JSON input from files or stdin. Lark Base workflow references and CLI tests were updated to reflect these rules. ChangesWorkflow guidance and validation
Estimated code review effort: 2 (Simple) | ~15 minutes Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 `@skills/lark-base/references/lark-base-workflow-schema.md`:
- Around line 238-239: Update the ReminderTrigger offset documentation to
explicitly define offset=0 as triggering at the referenced date/time, while
preserving the existing positive-before and negative-after semantics; only
remove zero from the allowed values if the service rejects it.
In `@tests/cli_e2e/base/base_workflow_update_dryrun_test.go`:
- Line 40: Update the assertion on DryRunGet("api.0.body.steps") to verify the
steps array has length zero, rather than only checking IsArray(), while
preserving the existing output context passed to the assertion.
🪄 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 Plus
Run ID: 68cc6e17-376d-416a-b8f8-61c20d21fad2
📒 Files selected for processing (8)
shortcuts/base/base_shortcuts_test.goshortcuts/base/workflow_disable.goshortcuts/base/workflow_enable.goshortcuts/base/workflow_update.goskills/lark-base/SKILL.mdskills/lark-base/references/lark-base-workflow-guide.mdskills/lark-base/references/lark-base-workflow-schema.mdtests/cli_e2e/base/base_workflow_update_dryrun_test.go
| > ⚠️ **先翻译方向，再填写数值：** `offset=N` 表示日期前 N 个单位触发（提前 N）；`offset=-N` 表示日期后 N 个单位触发（延后 N）。提交和回读时都按这句自然语言复核，不要只比较绝对值。 | ||
|
|
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Locate target file:"
fd -a 'lark-base-workflow-schema\.md$' . || true
echo
echo "Relevant target section:"
if [ -f skills/lark-base/references/lark-base-workflow-schema.md ]; then
sed -n '220,270p' skills/lark-base/references/lark-base-workflow-schema.md | cat -n -v
else
echo "target file missing"
fi
echo
echo "Search ReminderTrigger/offset references in repo:"
rg -n "ReminderTrigger|offset|前 N 个单位|后 N 个单位|at the referenced date|referenced date|referenced date/time|日期前|日期后" skills/lark-base/references/lark-base-workflow-schema.md . --glob '!node_modules' --glob '!dist' --glob '!build' | head -n 200Repository: larksuite/cli
Length of output: 32160
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Git status/diff summary:"
git diff --stat || true
echo
echo "Exact ReminderTrigger/offset text with line numbers:"
sed -n '236,262p' skills/lark-base/references/lark-base-workflow-schema.md | nl -ba -v236 | sed -e 's/\x0d$//' -e 's/\x00$//'
echo
echo "Related workflow/trigger implementation search:"
rg -n "ReminderTrigger|reminderTrigger|reminder_trigger|offset.*MINUTE|MINUTE|ReminderTrigger|triggerType|trigger" \
internal shortcuts tests . --glob '!node_modules' --glob '!dist' --glob '!build' --glob '!*.json' | head -n 200Repository: larksuite/cli
Length of output: 270
Clarify the zero-offset semantics in ReminderTrigger.
MINUTE and DAY both allow offset=0, but the direction rule only defines positive offsets as “before” and negative offsets as “after.” Document that offset=0 means the referenced date/time, or remove it if the service rejects zero.
🤖 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` around lines 238 -
239, Update the ReminderTrigger offset documentation to explicitly define
offset=0 as triggering at the referenced date/time, while preserving the
existing positive-before and negative-after semantics; only remove zero from the
allowed values if the service rejects it.
| require.Equal(t, "/open-apis/base/v3/bases/app_example/workflows/wkf_example", clie2e.DryRunGet(out, "api.0.url").String(), out) | ||
| require.Equal(t, "Updated workflow", clie2e.DryRunGet(out, "api.0.body.title").String(), out) | ||
| require.Equal(t, "OFF", clie2e.DryRunGet(out, "api.0.body.status").String(), out) | ||
| require.True(t, clie2e.DryRunGet(out, "api.0.body.steps").IsArray(), out) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Assert that steps is empty, not merely an array.
IsArray() would pass if a regression replaced the stdin payload with any non-empty steps array. Assert length zero to verify the exact {"steps":[]} input is preserved.
As per coding guidelines, contract tests must assert the changed field or behavior directly so reverting the implementation causes failure.
Proposed assertion
require.True(t, clie2e.DryRunGet(out, "api.0.body.steps").IsArray(), out)
+require.Len(t, clie2e.DryRunGet(out, "api.0.body.steps").Array(), 0, out)📝 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.
| require.True(t, clie2e.DryRunGet(out, "api.0.body.steps").IsArray(), out) | |
| require.True(t, clie2e.DryRunGet(out, "api.0.body.steps").IsArray(), out) | |
| require.Len(t, clie2e.DryRunGet(out, "api.0.body.steps").Array(), 0, out) |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tests/cli_e2e/base/base_workflow_update_dryrun_test.go` at line 40, Update
the assertion on DryRunGet("api.0.body.steps") to verify the steps array has
length zero, rather than only checking IsArray(), while preserving the existing
output context passed to the assertion.
Source: Coding guidelines
|
|
||
| 1. 用 `+workflow-list` / `+workflow-get` 定位准确的 `workflow_id` 并读取现状。 | ||
| 2. **始终执行用户要求的写命令。** 预读结果已经符合目标，不代表本次显式请求已完成；`+workflow-enable`、`+workflow-disable` 和保持相同目标值的 `+workflow-update` 都应按请求执行。不要把这条规则扩展到重复 `+workflow-create`。 | ||
| 3. 写入成功后对同一个 `workflow_id` 执行 `+workflow-get`，逐项核对用户要求的状态或字段。不能只以 update/enable/disable 的成功回执作为最终证据。 |
There was a problem hiding this comment.
这里面我感觉应该相信写操作返回的状态？如果 get 一次，其实耗 token 还挺大的，因为 workflow 配置可能比较复杂
🚀 PR Preview Install Guide🧰 CLI updatenpm i -g https://pkg.pr.new/larksuite/cli/@larksuite/cli@fe41c8f3520bc8a1219f7caefd36d3390be2c2fe🧩 Skill updatenpx skills add TanDianxiong/cli#auto-research-sync/01KYP9Z4SKA4YTSR27AX864802/mr-1294-758485a5 -y -g |
3 participants
Summary
Improve Base workflow mutation reliability by making update payload input more flexible and clarifying the required write-and-verify flow.
Changes
base +workflow-updatepayloads.Test Plan
git diff --check(passed)Related Issues
Auto research task: 01KYP9Z4SKA4YTSR27AX864802
Summary by CodeRabbit
New Features
+workflow-update --jsonnow supports input from files or standard input.Documentation
Tests