fix(base): improve record history output and validation - #2298
Open
huarenmin13 wants to merge 8 commits into
Open
fix(base): improve record history output and validation#2298huarenmin13 wants to merge 8 commits into
huarenmin13 wants to merge 8 commits into
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughBase record-history listing now requires an explicit record ID, validates pagination parameters, supports pretty output, and documents confirmed-row selection. Projection aliases accept ChangesBase record CLI behavior
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant BaseRecordHistoryList
participant RecordHistoryAPI
participant OutputFormatter
BaseRecordHistoryList->>RecordHistoryAPI: request history for explicit record_id
RecordHistoryAPI-->>BaseRecordHistoryList: return history response
BaseRecordHistoryList->>OutputFormatter: format selected output mode
OutputFormatter-->>BaseRecordHistoryList: return rendered history
🚥 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 |
github-actions
Bot
added
domain/base
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@skills/lark-base/SKILL.md`:
- Around line 45-58: Update the guidance around the strict N-history output
rules to define that when pagination completes with fewer than N verified
histories, strict result mode is suspended and a limitation or clarification is
returned instead. State that this exception takes precedence over the
exact-N-lines requirement and that missing entries must never be fabricated or
padded, while preserving the existing uncertainty handling for count, order, or
target.
🪄 Autofix
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: b0eb6cdf-1b79-4b04-af84-127ed839ebcd
📒 Files selected for processing (1)
skills/lark-base/SKILL.md
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@skills/lark-base/references/lark-base-record-history-list.md`:
- Line 47: 更新纯结果模式下的发送前校验：除确认非空行数等于 N 外，还必须确认总行数等于
N，并验证每一行都非空；保留最新在前、每行均为历史记录且无额外文字的现有要求，不满足任一条件时先重写。
🪄 Autofix
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: dcb2c487-5195-4c84-ae55-43171b75e756
📒 Files selected for processing (2)
skills/lark-base/SKILL.mdskills/lark-base/references/lark-base-record-history-list.md
zgz2048
reviewed
Aug 11, 2026
🚀 PR Preview Install Guide🧰 CLI updatenpm i -g https://pkg.pr.new/larksuite/cli/@larksuite/cli@df5172f0e451b2518df4986d3dd14e26dddb8e2b🧩 Skill updatenpx skills add huarenmin13/cli#docs/base-history-exact-results -y -g |
huarenmin13
marked this pull request as draft
August 11, 2026 12:13
huarenmin13
force-pushed
the
docs/base-history-exact-results
branch
from
August 12, 2026 08:40
1378164 to
c18d215
Compare
1. Require an explicit confirmed record ID and document deterministic record resolution
2. Add shared-format pretty history output and projection aliases without changing default JSON
3. Reject explicitly non-positive history cursors and cover request, help, formatting,
and dry-run behavior
```ai-signature
改动范围: 将 Codebase MR 1438 的最终十文件差异移植到 GitHub PR 2298，覆盖 record history 命令、record list 投影别名、Base 引导文档与对应单元及 dry-run 回归测试
思考过程: 以 GitHub 最新 main 为基线执行三方内容合并，保留 GitHub 独立演进；删除原 PR 针对 base_history_003 的 Skill 改动，并仅按 GitHub 当前分母重算 coverage 指标
改动原因: GitHub PR 原先承载的是已确认应撤回的单题文档方案，需要由已评审的 MR 1438 最终通用实现完整替换，同时避免复制 Codebase 的多轮提交与 revert 历史
Break Change: 行为 breaking | record history 要求显式确认的 record ID，且显式非正 --max-version 现在返回 typed validation error
```
Co-authored-by: BASE Infra Harness <ai@base-infra-harness.noreply.local>
AI-SHA256: 5bf66267670f435c7d577d2444b61b209322eb5a73f15e47e671a5b8dd2603bf
huarenmin13
force-pushed
the
docs/base-history-exact-results
branch
from
August 12, 2026 11:46
c18d215 to
da5223c
Compare
huarenmin13
changed the title
huarenmin13
marked this pull request as ready for review
August 12, 2026 11:47
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
tests/cli_e2e/base/base_record_list_dryrun_test.go (1)
198-216: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winAssert the search output path.
This test passes
--output search.ndjson, but it does not assertdata.output.dryRunRecordSearchsets this field inshortcuts/base/record_ops.goLines 296-298. A regression that drops the output path from the dry-run contract would still pass.As per coding guidelines, every behavior change requires a nearby regression test that asserts fields or side effects directly.
Proposed assertion
require.Equal(t, "ndjson", gjson.Get(out, "data.export_format").String(), out) require.Equal(t, int64(500), gjson.Get(out, "data.requested_limit").Int(), out) + require.Equal(t, "search.ndjson", gjson.Get(out, "data.output").String(), 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_record_list_dryrun_test.go` around lines 198 - 216, Add an assertion in TestBaseRecordSearchDryRunNDJSONCapsFirstPageAndKeepsQuery that data.output equals "search.ndjson", alongside the existing dry-run contract assertions, so the requested output path is explicitly covered.Source: Coding guidelines
shortcuts/base/record_ops.go (1)
230-233: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick winAlign the NDJSON dry-run and execution page limits.
dryRunRecordListanddryRunRecordSearchusemaxInlineRecordReadLimit(200), while execution usesndjsonRecordPageSize(500). For requested limits from 201 through 500, dry-run reports a different first API request. Use one shared constant.🤖 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` around lines 230 - 233, Update the NDJSON request-limit logic near requestLimit and the dry-run implementations dryRunRecordList and dryRunRecordSearch to use one shared page-size constant instead of separate maxInlineRecordReadLimit and ndjsonRecordPageSize values. Ensure dry-run and execution select the same first API request limit for requested limits from 201 through 500.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@shortcuts/base/record_history_list.go`:
- Around line 171-180: Update the has-more handling in the record history
pagination flow around formatRecordHistoryPrettyValue to validate
page.NextMaxVersion as a positive integer before printing continuation guidance.
Return errs.NewInternalError(errs.SubtypeInvalidResponse, ...) for missing,
non-integer, fractional, non-positive, string, or object values; preserve valid
cursor formatting and add regression coverage for both valid and invalid cursor
values.
In `@tests/cli_e2e/base/base_record_history_dryrun_test.go`:
- Around line 16-76: Add a self-contained live E2E test alongside
TestBaseRecordHistoryListDryRunUsesExplicitRecordID that creates temporary Base,
table, and record resources, updates the record, then invokes
+record-history-list with --record-id. Assert the returned history contains the
change and validates cursor behavior, and ensure all created resources are
cleaned up with test-safe teardown.
---
Outside diff comments:
In `@shortcuts/base/record_ops.go`:
- Around line 230-233: Update the NDJSON request-limit logic near requestLimit
and the dry-run implementations dryRunRecordList and dryRunRecordSearch to use
one shared page-size constant instead of separate maxInlineRecordReadLimit and
ndjsonRecordPageSize values. Ensure dry-run and execution select the same first
API request limit for requested limits from 201 through 500.
In `@tests/cli_e2e/base/base_record_list_dryrun_test.go`:
- Around line 198-216: Add an assertion in
TestBaseRecordSearchDryRunNDJSONCapsFirstPageAndKeepsQuery that data.output
equals "search.ndjson", alongside the existing dry-run contract assertions, so
the requested output path is explicitly covered.
🪄 Autofix
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: 88e1a74a-1d08-4e72-91ff-f9c2fd5e0437
📒 Files selected for processing (10)
shortcuts/base/base_execute_test.goshortcuts/base/base_shortcuts_test.goshortcuts/base/base_skill_contract_test.goshortcuts/base/record_history_list.goshortcuts/base/record_list.goshortcuts/base/record_ops.goskills/lark-base/references/lark-base-record-history-list.mdtests/cli_e2e/base/base_record_history_dryrun_test.gotests/cli_e2e/base/base_record_list_dryrun_test.gotests/cli_e2e/base/coverage.md
Contributor
Author
|
Addressed the current CodeRabbit review in 5fb2b28:
Validation: go test ./shortcuts/base, go test ./tests/cli_e2e/base, go vet ./..., skill format check, and git diff --check. |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
tests/cli_e2e/base/base_record_list_dryrun_test.go (1)
198-216: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winTest the search cap with a limit above 500.
Line 206 passes
--limit 500. The assertion at Line 212 also expects500. This test passes even if the first-page cap is removed.Use
--limit 2000, keep the request body limit assertion at500, and changerequested_limitto2000.Proposed test adjustment
- "--limit", "500", + "--limit", "2000", ... - require.Equal(t, int64(500), gjson.Get(out, "data.requested_limit").Int(), out) + require.Equal(t, int64(2000), gjson.Get(out, "data.requested_limit").Int(), 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_record_list_dryrun_test.go` around lines 198 - 216, Update TestBaseRecordSearchDryRunNDJSONCapsFirstPageAndKeepsQuery to pass --limit 2000, retain the request body limit assertion at 500, and change the data.requested_limit assertion to 2000 so the test verifies capping only the first-page request.
🤖 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.
Nitpick comments:
In `@tests/cli_e2e/base/base_record_list_dryrun_test.go`:
- Around line 198-216: Update
TestBaseRecordSearchDryRunNDJSONCapsFirstPageAndKeepsQuery to pass --limit 2000,
retain the request body limit assertion at 500, and change the
data.requested_limit assertion to 2000 so the test verifies capping only the
first-page request.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: bb5e17f3-e4d6-4fa0-93c9-1a7c3cdc4c2e
📒 Files selected for processing (4)
shortcuts/base/base_execute_test.goshortcuts/base/record_history_list.goshortcuts/base/record_ops.gotests/cli_e2e/base/base_record_list_dryrun_test.go
🚧 Files skipped from review as they are similar to previous changes (1)
- shortcuts/base/base_execute_test.go
1. Reject missing or invalid next_max_version values before emitting pagination guidance
2. Use the execution page size in NDJSON dry runs and assert the requested output path
3. Cover valid and invalid history cursors and prove a 2000-row request is capped to a 500-row first
page
```ai-signature
改动范围: shortcuts/base/record_history_list.go、record_ops.go 及邻近单元和 dry-run E2E 测试，仅处理 PR 2298 中有效 CodeRabbit 评论，不新增 live E2E 或修改主 Skill
思考过程: 分页提示只接受解码后的正整数游标；NDJSON dry-run 复用执行阶段的五百行页大小，并以两千行请求验证真实截断而非相等值偶然通过，同时直接断言导出路径契约
改动原因: 原实现可能把不可用的服务端游标打印成命令，同时 dry-run 对 201 到 500 条请求报告的首屏大小与真实执行不一致，初版测试也未真正证明五百行上限或 search 输出路径
Break Change: 否
```
Co-authored-by: BASE Infra Harness <ai@base-infra-harness.noreply.local>
AI-SHA256: a362c47d739d6de5676b87431fbc015d1eaa3d47d46d711e2535baa4dad7ffce
huarenmin13
force-pushed
the
docs/base-history-exact-results
branch
from
August 12, 2026 14:20
5fb2b28 to
3a3b2e2
Compare
Contributor
Author
|
Follow-up CodeRabbit nitpick addressed in 3a3b2e2: the record-search dry-run test now requests 2000 rows, asserts the first API page is capped at 500, and preserves requested_limit=2000. The focused test passes. |
huarenmin13
marked this pull request as draft
August 12, 2026 15:06
1. Remove the record history test that reads and pins Markdown reference text. ```ai-signature 改动范围: 仅修改 shortcuts/base/base_skill_contract_test.go，删除直接读取 lark-base-record-history-list.md 并逐句断言文案的测试函数。 思考过程: 将 Markdown 文案视为非行为契约，保留 record history 的参数校验、格式化与 dry-run 行为测试，避免文档措辞调整导致无意义回归失败。 改动原因: 用户明确要求 PR 中的 .md 改动不需要测试，因此移除唯一直接绑定 Markdown 内容的新增测试，不扩大到其他命令行为测试。 Break Change: 否 ``` Co-authored-by: BASE Infra Harness <ai@base-infra-harness.noreply.local> AI-SHA256: 3f98641b8fff6e59b6b4b3bbf2381b5aa54c8f40aa0e52fc9b80487a355f31f5
1. Condense the record history prerequisites into generic record selection rules. 2. Remove the positive and negative examples from the reference. ```ai-signature 改动范围: 仅修改 skills/lark-base/references/lark-base-record-history-list.md，精简使用前置并删除正反例章节。 思考过程: 保留调用前确认同表 record_id、不得自行选择记录或扩展整表扫描的核心约束，移除具体链接、视图位置和命令示例以提升通用性。 改动原因: 用户要求使用前置更精简、表述更通用，并删除正反例；本次不涉及命令行为或测试。 Break Change: 否 ``` Co-authored-by: BASE Infra Harness <ai@base-infra-harness.noreply.local> AI-SHA256: 7e69951d47c1535cd5fe0fa9ebd7018af2e17373e5851c229dac46d68eaefa84
huarenmin13
marked this pull request as ready for review
August 13, 2026 03:06
1. Remove the new Base affordance file and its Markdown-only tests 2. Keep selection and field quoting guidance in the existing record history reference 3. Restore the pre-existing command tips and retain only behavior-focused regressions ```ai-signature 改动范围: 删除新增 affordance/base.md 与对应 source/help 文案测试，调整 record-history 现有 reference、覆盖说明和原命令 Tips 思考过程: 用户要求不新增 Base affordance 文件且 Markdown 不需要专门测试，因此保留运行时代码测试，把跨命令选行和字段引号说明收敛到已有 reference 改动原因: 新增 affordance 与文案测试扩大了 PR 改动面，并重复承载已有 history reference 的 agent 工作流说明 Break Change: 否 ``` Co-authored-by: BASE Infra Harness <ai@base-infra-harness.noreply.local> AI-SHA256: 4c7e3db10025f412d4b2cb4443d94724b5997b72b7e8f97fafdae2f08633b5a2
1. Describe the record ID as uniquely resolving the user-selected target 2. Avoid implying that users must confirm an opaque internal record ID directly ```ai-signature 改动范围: 仅调整现有 lark-base record-history reference 的一处目标记录选择表述 思考过程: CLI 运行时只要求有效 record_id，agent 工作流要求用户确认目标而不是直接确认内部 ID，因此需要区分产品事实和操作约束 改动原因: 原表述可能被误解为 base-cli 能验证 record_id 的用户确认来源，与实际产品边界不一致 Break Change: 否 ``` Co-authored-by: BASE Infra Harness <ai@base-infra-harness.noreply.local> AI-SHA256: bd6c9584cc38689e1c3d9f26514425e3e38ffe2f2cff7b2bb0c65ff316aaf8dc
huarenmin13
changed the title
zhouyue-bytedance
previously approved these changes
Aug 13, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Summary
Make the existing single-record history command safer for agent workflows and easier to read while preserving its default JSON envelope. This replaces the previous
base_history_003-specific documentation change.Changes
skills/lark-base/references/lark-base-record-history-list.md; no new Base affordance file is introduced.+record-listexposes record IDs in the default Markdown_record_idcolumn and, for--format json, at.data.record_id_list[0].--format prettyhistory rendering while routing supported structured formats through the shared output formatter.--fieldprojection alias and field-name quoting guidance in the existing history reference.--max-versioncursors instead of silently omitting them.skills/lark-base/SKILL.mdunchanged.Compatibility
--format table,--format csv, and--format ndjsoninvocations now use the shared formatter instead of falling back to the JSON envelope.--max-version <= 0now returns a typedinvalid_argumenterror.Test Plan
make buildgo test ./shortcuts/basego test ./tests/cli_e2e/basemake vetmake fmt-checknode scripts/skill-format-check/index.jsQUALITY_GATE_CHANGED_FROM=upstream/main make quality-gategit diff --checkLive E2E was not added because the history endpoint has no deterministic, self-contained create/cleanup fixture in this suite. The substitute coverage verifies request shape, cursor validation, default JSON compatibility, structured-format routing, and pretty rendering with synthetic history responses.