feat(base): add typed NDJSON workflows for professional data analysis - #2196
feat(base): add typed NDJSON workflows for professional data analysis#2196zgz2048 wants to merge 15 commits into
Conversation
📝 WalkthroughWalkthroughThis PR adds NDJSON export for Base record list, search, and get commands. It adds typed datasets, manifests, pagination, validation, artifact handling, JQ support, CLI routing, tests, and updated Base analysis guidance. ChangesRecord export pipeline
Estimated code review effort: 5 (Critical) | ~120 minutes Suggested labels: Suggested reviewers: Sequence Diagram(s)sequenceDiagram
participant CLI
participant RecordShortcut
participant RecordExporter
participant RecordAPI
participant ArtifactFiles
CLI->>RecordShortcut: select NDJSON format and export options
RecordShortcut->>RecordExporter: validate and start export
RecordExporter->>RecordAPI: request paginated records
RecordAPI-->>RecordExporter: return record matrix pages
RecordExporter->>ArtifactFiles: write NDJSON and manifest
ArtifactFiles-->>CLI: return export metadata
🚥 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
0c14a6d to
99d6a7d
Compare
…djson # Conflicts: # skills/lark-base/SKILL.md
🚀 PR Preview Install Guide🧰 CLI updatenpm i -g https://pkg.pr.new/larksuite/cli/@larksuite/cli@ebd4a36e4a1419b883941306995483fbead373f0🧩 Skill updatenpx skills add zgz2048/cli#codex/base-record-ndjson -y -g |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 13
🧹 Nitpick comments (5)
shortcuts/base/recordexport/dataset_test.go (1)
69-69: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUse the comma-ok form for these type assertions.
If a regression changes the cell shape, these assertions panic instead of producing a readable test failure. Use the comma-ok form and call
t.Fatalfwith the actual value.Also applies to: 150-157
🤖 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/recordexport/dataset_test.go` at line 69, Update the type assertions in the dataset test, including the assertion around first[7] and the additional assertions at the referenced locations, to use comma-ok checks. When an assertion fails, call t.Fatalf with the actual cell value and stop the test; otherwise continue using the successfully asserted map value.shortcuts/base/record_export.go (1)
193-276: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExtract the shared pagination loop.
executeRecordListNDJSONandexecuteRecordSearchNDJSONcontain the same loop. Only the request call differs. The row-count guard, the accumulate step, theremaining/currentOffsetarithmetic, and the termination condition are duplicated. Extract a helper that takes afunc(offset, limit int) (map[string]any, error)fetcher, so a future fix to the pagination arithmetic applies to both commands.🤖 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_export.go` around lines 193 - 276, Extract the duplicated pagination logic from executeRecordListNDJSON and executeRecordSearchNDJSON into a shared helper that accepts a func(offset, limit int) (map[string]any, error) fetcher. Keep page parsing, row-count validation, accumulation, offset/remaining updates, and termination behavior in the helper; have each command provide only its request-specific fetcher and retain its existing finalization flow.shortcuts/base/recordexport/manifest.go (2)
336-361: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low value
bestExamplemarshals every cell of every column.The function calls
json.Marshalonce per record per column to find the shortest non-empty value. For the 2000-row NDJSON limit and a wide table, this runs tens of thousands of extra marshals after the rows are already written. Stop the scan once a value is small enough, for example once the encoded length is at or below a threshold, or cap the scan at the first N records.The name is also misleading. The function selects the shortest example, not the most representative one. Consider
shortestExample.🤖 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/recordexport/manifest.go` around lines 336 - 361, Limit the work performed by bestExample while preserving selection of the shortest non-empty value: stop scanning once an encoded value meets a small-size threshold or after a bounded number of records, using a clearly defined constant. Rename bestExample and its callers to shortestExample to accurately describe the behavior.
65-65: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueColumn order is lost in the manifest.
Columnsis amap[string]ColumnManifest, so the manifest does not preserve the dataset column order. A consumer that builds a table or a DataFrame from the manifest cannot reproduce the original field order. Add an orderedcolumn_order []stringfield, or changeColumnsto an ordered slice.Also applies to: 121-135
🤖 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/recordexport/manifest.go` at line 65, Add an ordered column sequence to the manifest model alongside Columns, such as a column_order []string JSON field, and populate it wherever the manifest is constructed or serialized (including the code around lines 121-135). Preserve the existing column definitions while ensuring consumers can reconstruct the original dataset field order.shortcuts/base/recordexport/dataset.go (1)
274-283: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winConsider detecting
revdrift across pages.
AppendPagecompares the timezone and the source columns, but notRev.recordExportAccumulatorkeeps the first page'sRev, so the manifest labels the whole dataset with a revision that later rows no longer match.TestRecordListNDJSONSerializesPagesAbove200inshortcuts/base/record_export_test.goasserts this behavior withrev100 and 101, so it looks intentional. If it is intentional, record the drift in the manifest or in a stderr warning, because row-level data changed between pages while the schema did not.🤖 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/recordexport/dataset.go` around lines 274 - 283, Update Dataset.AppendPage and the manifest accumulation flow to detect when page.Dataset.Rev differs from the dataset’s initial Rev, while preserving the existing successful append behavior. Record the revision drift in the manifest or emit a stderr warning so consumers are informed that row data spans multiple revisions; retain the existing first-Rev behavior unless the surrounding contract requires otherwise.
🤖 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/base_execute_test.go`:
- Around line 2821-2829: The BaseRecordBatchUpdate test should assert the
complete ignored_fields structure rather than only checking that stdout contains
“ignored_fields” and “Formula”. Validate the ignored field’s id, name, and
reason values, matching the structured contract asserted by the corresponding
markdown test.
In `@shortcuts/base/record_export_test.go`:
- Around line 234-241: Update the collision test around runShortcut and the
jq-records table test around lines 403-414 to validate typed errors with the
existing assertInvalidArgumentValidation pattern. Assert the expected category,
subtype, and parameter, and verify cause preservation through errs.ProblemOf
instead of relying only on problem.Hint or err.Error() substring checks.
In `@shortcuts/base/record_export.go`:
- Around line 325-346: Update the error path after saveRecordManifest in the
record export flow to remove the record file created by saveRecordNDJSON before
returning the manifest-save error. Preserve the original manifest error and
ensure cleanup targets paths.recordRelative through the existing file I/O
abstraction.
In `@shortcuts/base/recordexport/dataset_test.go`:
- Around line 180-193: Update the error-path tests around
TestDatasetAppendRejectsSchemaChange and the related tests near the referenced
range to assert concrete error types with errors.As: expect *SchemaChangedError
for schema changes and *MatrixError for matrix failures. Verify the typed error
metadata, including the changed field where applicable, and preserve/assert the
underlying cause instead of checking only err != nil or message text.
In `@shortcuts/base/recordexport/dataset.go`:
- Around line 399-412: Update the ignored_fields parsing block in the record
export flow so missing id, name, or reason keys default to empty strings without
returning a MatrixError. Retain the type validation for present values,
returning the existing error when any supplied value is not a string, and
continue processing advisory warning objects without aborting valid exports.
- Around line 171-188: Validate column names while building exportColumns in the
surrounding dataset construction flow, rejecting any duplicate non-system names
with a MatrixError before returning the page; preserve the existing
RecordIDColumnName handling so the system join key continues to take precedence
over a same-named source field.
In `@skills/lark-base/references/lark-base-cell-value.md`:
- Around line 64-72: Align the datetime serialization contract across the
serializer, the artifact test, and the documentation: choose one RFC3339 output
format regarding milliseconds and apply it consistently. Update the relevant
record-export serializer and its test, then revise the datetime read-value
example in the cell-value documentation to match.
- Around line 51-60: Update the select-field examples in the complete example
and the record upsert example to use option-name arrays consistently with the
documented select write/read contract, replacing scalar "状态" values while
preserving the examples’ intended data.
In `@skills/lark-base/references/lark-base-data-analysis-sop.md`:
- Around line 168-172: 在“单表简单筛选与统计：jq”段落中，将本地分析说明里的 `js -s` 更正为 `jq -s`，保持与前文
`jq -s` 工作流及 `--jq-records` 等价命令一致。
- Around line 134-136: Update the physical-type table entries around the rows
containing inline code at lines 134–136 and 139 so every literal pipe is escaped
as \| (or replaced with an equivalent representation that contains no raw table
delimiter). Preserve the documented type and example values while keeping the
Markdown table correctly aligned.
In `@skills/lark-base/SKILL.md`:
- Around line 34-36: Clarify the workflow guidance around the Base command
section so lark-drive is used only for file import/export, while record
list/search/get operations that produce local NDJSON analysis artifacts remain
in this Base workflow. Update the related routing statements, including the
references near the high-frequency data-analysis guidance and other
Base-to-local export instructions, without changing the copy or table-copy
paths.
In `@tests/cli_e2e/base/base_record_list_dryrun_test.go`:
- Around line 118-169: Add live E2E coverage alongside the dry-run tests, using
the existing base-record test helpers and bot credential configuration to create
a temporary base/table and records, run record-list, record-search, and
record-get with NDJSON output, and verify real records plus NDJSON and manifest
publication. Make the flow self-contained with deferred cleanup of all created
resources and generated artifacts, while preserving the existing dry-run
assertions.
- Around line 147-153: Update the test assertions in the record-list dry-run
test to verify that data.output equals the requested search.ndjson destination.
Keep the existing export-format and requested-limit assertions unchanged, and
assert the output path directly so destination handling is covered.
---
Nitpick comments:
In `@shortcuts/base/record_export.go`:
- Around line 193-276: Extract the duplicated pagination logic from
executeRecordListNDJSON and executeRecordSearchNDJSON into a shared helper that
accepts a func(offset, limit int) (map[string]any, error) fetcher. Keep page
parsing, row-count validation, accumulation, offset/remaining updates, and
termination behavior in the helper; have each command provide only its
request-specific fetcher and retain its existing finalization flow.
In `@shortcuts/base/recordexport/dataset_test.go`:
- Line 69: Update the type assertions in the dataset test, including the
assertion around first[7] and the additional assertions at the referenced
locations, to use comma-ok checks. When an assertion fails, call t.Fatalf with
the actual cell value and stop the test; otherwise continue using the
successfully asserted map value.
In `@shortcuts/base/recordexport/dataset.go`:
- Around line 274-283: Update Dataset.AppendPage and the manifest accumulation
flow to detect when page.Dataset.Rev differs from the dataset’s initial Rev,
while preserving the existing successful append behavior. Record the revision
drift in the manifest or emit a stderr warning so consumers are informed that
row data spans multiple revisions; retain the existing first-Rev behavior unless
the surrounding contract requires otherwise.
In `@shortcuts/base/recordexport/manifest.go`:
- Around line 336-361: Limit the work performed by bestExample while preserving
selection of the shortest non-empty value: stop scanning once an encoded value
meets a small-size threshold or after a bounded number of records, using a
clearly defined constant. Rename bestExample and its callers to shortestExample
to accurately describe the behavior.
- Line 65: Add an ordered column sequence to the manifest model alongside
Columns, such as a column_order []string JSON field, and populate it wherever
the manifest is constructed or serialized (including the code around lines
121-135). Preserve the existing column definitions while ensuring consumers can
reconstruct the original dataset field order.
🪄 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: 3340746b-2a55-4fc3-8b73-d5e3e945339f
📒 Files selected for processing (31)
shortcuts/base/base_execute_test.goshortcuts/base/base_shortcuts_test.goshortcuts/base/record_export.goshortcuts/base/record_export_test.goshortcuts/base/record_get.goshortcuts/base/record_json_shorthand_test.goshortcuts/base/record_list.goshortcuts/base/record_markdown.goshortcuts/base/record_markdown_test.goshortcuts/base/record_ops.goshortcuts/base/record_query.goshortcuts/base/record_search.goshortcuts/base/recordexport/dataset.goshortcuts/base/recordexport/dataset_test.goshortcuts/base/recordexport/errors.goshortcuts/base/recordexport/manifest.goshortcuts/base/recordexport/ndjson.goshortcuts/common/runner.goshortcuts/common/types.goskills/lark-base/SKILL.mdskills/lark-base/references/lark-base-cell-value.mdskills/lark-base/references/lark-base-data-analysis-cloud.mdskills/lark-base/references/lark-base-data-analysis-pandas.mdskills/lark-base/references/lark-base-data-analysis-python-stdlib.mdskills/lark-base/references/lark-base-data-analysis-sop.mdskills/lark-base/references/lark-base-data-query-guide.mdskills/lark-base/references/lark-base-data-query.mdskills/lark-base/references/lark-base-field-json.mdskills/lark-base/references/lark-base-record-upsert.mdtests/cli_e2e/base/base_record_list_dryrun_test.gotests/cli_e2e/base/base_skill_contract_test.go
💤 Files with no reviewable changes (1)
- tests/cli_e2e/base/base_skill_contract_test.go
| "ignored_fields": []interface{}{map[string]interface{}{ | ||
| "id": "fld_formula", | ||
| "name": "Formula", | ||
| "reason": "READONLY: formula field cannot be written through OpenAPI.", | ||
| }}, | ||
| }, | ||
| }, | ||
| }) | ||
| if err := runShortcut(t, BaseRecordBatchUpdate, []string{"+record-batch-update", "--base-token", "app_x", "--table-id", "tbl_x", "--json", `{"update_records":{"rec_1":{"Status":["Done"]}}}`}, factory, stdout); err != nil { | ||
| if err := runShortcut(t, BaseRecordBatchUpdate, []string{"+record-batch-update", "--base-token", "app_x", "--table-id", "tbl_x", "--json", `{"update_records":{"rec_1":{"Status":["Done"],"Formula":"ignored"}}}`}, factory, stdout); err != nil { |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Assert the structured ignored-field contract.
The test only checks that stdout contains "ignored_fields" and "Formula". It does not verify id or reason. A regression to name-only ignored fields would pass.
Assert the complete structured value, as the markdown test does.
As per coding guidelines, “Every behavior change must have an accompanying test, and contract tests must assert the changed field or behavior directly so reverting the implementation causes failure.”
🤖 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 2821 - 2829, The
BaseRecordBatchUpdate test should assert the complete ignored_fields structure
rather than only checking that stdout contains “ignored_fields” and “Formula”.
Validate the ignored field’s id, name, and reason values, matching the
structured contract asserted by the corresponding markdown test.
Source: Coding guidelines
| if err == nil { | ||
| t.Fatal("runShortcut() error = nil") | ||
| } | ||
| problem, ok := errs.ProblemOf(err) | ||
| if !ok || problem.Hint == "" || !strings.Contains(problem.Hint, "--overwrite") { | ||
| t.Fatalf("problem = %#v", problem) | ||
| } | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Assert typed error metadata on these error paths.
Both tests check only the message text. The collision test reads problem.Hint only. The jq-records table test compares err.Error() substrings. Neither asserts Category, Subtype, or Param, so a change to the error classification would not fail these tests. Other cases in this file already use assertInvalidArgumentValidation; apply the same pattern here.
As per coding guidelines: "Error-path tests must assert typed metadata through errs.ProblemOf (category, subtype, and param) and verify cause preservation rather than relying only on message substrings."
💚 Proposed fix for the collision test
problem, ok := errs.ProblemOf(err)
- if !ok || problem.Hint == "" || !strings.Contains(problem.Hint, "--overwrite") {
+ if !ok || problem.Subtype != errs.SubtypeFailedPrecondition ||
+ !strings.Contains(problem.Hint, "--overwrite") {
t.Fatalf("problem = %#v", problem)
}
+ var validationErr *errs.ValidationError
+ if !errors.As(err, &validationErr) || validationErr.Param != "--output" {
+ t.Fatalf("param = %#v", validationErr)
+ }Also applies to: 403-414
🤖 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_export_test.go` around lines 234 - 241, Update the
collision test around runShortcut and the jq-records table test around lines
403-414 to validate typed errors with the existing
assertInvalidArgumentValidation pattern. Assert the expected category, subtype,
and parameter, and verify cause preservation through errs.ProblemOf instead of
relying only on problem.Hint or err.Error() substring checks.
Source: Coding guidelines
| recordFileSizeBytes, err := saveRecordNDJSON(fio, paths.recordRelative, accumulator.dataset) | ||
| if err != nil { | ||
| return err | ||
| } | ||
| manifest := recordexport.BuildManifest(accumulator.dataset, recordexport.ManifestOptions{ | ||
| BaseToken: runtime.Str("base-token"), | ||
| TableID: baseTableID(runtime), | ||
| Rev: accumulator.rev, | ||
| QueryContext: accumulator.queryContext, | ||
| Offset: startOffset, | ||
| RequestedLimit: requestedLimit, | ||
| PageCount: accumulator.pageCount, | ||
| HasMore: accumulator.hasMore, | ||
| RecordFile: paths.recordAbsolute, | ||
| RecordFileSizeBytes: recordFileSizeBytes, | ||
| ManifestFile: paths.manifestAbsolute, | ||
| IgnoredFields: accumulator.ignoredFields, | ||
| RecordNotFound: accumulator.recordNotFound, | ||
| }) | ||
| if err := saveRecordManifest(fio, paths.manifestRelative, manifest); err != nil { | ||
| return err | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: List the fileio.FileIO interface methods to find a delete/remove capability.
set -euo pipefail
fd -t f 'fileio' | head -50
echo '--- FileIO interface ---'
ast-grep run --pattern 'type FileIO interface { $$$ }' --lang go .
rg -n -C3 'func .*LocalFileIO\) (Remove|Delete)' --type=goRepository: larksuite/cli
Length of output: 1814
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '--- fileio types ---'
cat -n extension/fileio/types.go | sed -n '1,180p'
echo '--- localfileio implementation ---'
cat -n internal/vfs/localfileio/localfileio.go | sed -n '1,260p'
echo '--- record_export relevant sections ---'
cat -n shortcuts/base/record_export.go | sed -n '280,420p'
echo '--- artifact existence check ---'
rg -n -C4 'output file already exists|ensureRecordExportTargets|saveRecordNDJSON|saveRecordManifest' shortcuts/base/record_export.go --type=go
echo '--- localfileio tests for artifact behavior if present ---'
cat -n internal/vfs/localfileio/localfileio_test.go | sed -n '1,260p'Repository: larksuite/cli
Length of output: 22323
Remove the record file when manifest save fails.
saveRecordNDJSON writes the .ndjson first; when saveRecordManifest fails, ensureRecordExportTargets can reject a later run because the orphan .ndjson exists. Handle this partial-update case by cleaning up the record file when the manifest cannot be saved, or write the manifest before the record file.
🤖 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_export.go` around lines 325 - 346, Update the error
path after saveRecordManifest in the record export flow to remove the record
file created by saveRecordNDJSON before returning the manifest-save error.
Preserve the original manifest error and ensure cleanup targets
paths.recordRelative through the existing file I/O abstraction.
| func TestDatasetAppendRejectsSchemaChange(t *testing.T) { | ||
| first, err := ParseMatrix(matrixFixture("Name", "fld_name", "text", "rec_1", "Alice")) | ||
| if err != nil { | ||
| t.Fatal(err) | ||
| } | ||
| second, err := ParseMatrix(matrixFixture("Name", "fld_other", "text", "rec_2", "Bob")) | ||
| if err != nil { | ||
| t.Fatal(err) | ||
| } | ||
| dataset := first.Dataset | ||
| if err := dataset.AppendPage(second); err == nil { | ||
| t.Fatal("AppendPage() error = nil, want schema change") | ||
| } | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Assert the concrete error types on the error paths.
These three tests check only err != nil. shortcuts/base/record_export.go branches on errors.As(err, &schemaChanged) for *SchemaChangedError and maps it to errs.SubtypeFailedPrecondition. If AppendPage ever returned a plain error, TestDatasetAppendRejectsSchemaChange would still pass while the CLI error class degraded to an internal error. Use errors.As for *SchemaChangedError and *MatrixError.
As per coding guidelines: "Error-path tests must assert typed metadata ... and verify cause preservation rather than relying only on message substrings" and "contract tests must assert the changed field or behavior directly so reverting the implementation causes failure."
💚 Proposed fix
dataset := first.Dataset
- if err := dataset.AppendPage(second); err == nil {
- t.Fatal("AppendPage() error = nil, want schema change")
- }
+ err = dataset.AppendPage(second)
+ var schemaChanged *SchemaChangedError
+ if !errors.As(err, &schemaChanged) {
+ t.Fatalf("AppendPage() error = %v, want *SchemaChangedError", err)
+ }
}- if err == nil {
- t.Fatal("ParseMatrix() error = nil")
- }
+ var matrixErr *MatrixError
+ if !errors.As(err, &matrixErr) {
+ t.Fatalf("ParseMatrix() error = %v, want *MatrixError", err)
+ }Also applies to: 307-327
🤖 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/recordexport/dataset_test.go` around lines 180 - 193, Update
the error-path tests around TestDatasetAppendRejectsSchemaChange and the related
tests near the referenced range to assert concrete error types with errors.As:
expect *SchemaChangedError for schema changes and *MatrixError for matrix
failures. Verify the typed error metadata, including the changed field where
applicable, and preserve/assert the underlying cause instead of checking only
err != nil or message text.
Source: Coding guidelines
| sourceColumns := make([]Column, 0, len(fields)) | ||
| exportColumns := []Column{{ | ||
| Name: RecordIDColumnName, System: true, | ||
| }} | ||
| exportSourceIndexes := make([]int, 0, len(fields)) | ||
| for index := range fields { | ||
| column, err := sourceColumn(fields[index], fieldIDs[index], fieldTypes[index]) | ||
| if err != nil { | ||
| return Page{}, err | ||
| } | ||
| sourceColumns = append(sourceColumns, column) | ||
| // The system join key intentionally wins over a same-named Base field. | ||
| if column.Name == RecordIDColumnName { | ||
| continue | ||
| } | ||
| exportColumns = append(exportColumns, column) | ||
| exportSourceIndexes = append(exportSourceIndexes, index) | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Duplicate field names silently collapse to one column.
exportColumns can contain two columns with the same Name. WriteNDJSON writes object[column.Name], and BuildManifest writes manifest.Columns[column.Name]. In both cases the later column overwrites the earlier one, so one field's values disappear from the artifact without any warning. Reject duplicate names here with a MatrixError, or disambiguate the exported name.
🛠️ Proposed fix
sourceColumns := make([]Column, 0, len(fields))
exportColumns := []Column{{
Name: RecordIDColumnName, System: true,
}}
exportSourceIndexes := make([]int, 0, len(fields))
+ exportNames := map[string]bool{RecordIDColumnName: true}
for index := range fields {
column, err := sourceColumn(fields[index], fieldIDs[index], fieldTypes[index])
if err != nil {
return Page{}, err
}
sourceColumns = append(sourceColumns, column)
// The system join key intentionally wins over a same-named Base field.
if column.Name == RecordIDColumnName {
continue
}
+ if exportNames[column.Name] {
+ return Page{}, &MatrixError{Reason: fmt.Sprintf("duplicate field name %q", column.Name)}
+ }
+ exportNames[column.Name] = true
exportColumns = append(exportColumns, column)
exportSourceIndexes = append(exportSourceIndexes, index)
}📝 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.
| sourceColumns := make([]Column, 0, len(fields)) | |
| exportColumns := []Column{{ | |
| Name: RecordIDColumnName, System: true, | |
| }} | |
| exportSourceIndexes := make([]int, 0, len(fields)) | |
| for index := range fields { | |
| column, err := sourceColumn(fields[index], fieldIDs[index], fieldTypes[index]) | |
| if err != nil { | |
| return Page{}, err | |
| } | |
| sourceColumns = append(sourceColumns, column) | |
| // The system join key intentionally wins over a same-named Base field. | |
| if column.Name == RecordIDColumnName { | |
| continue | |
| } | |
| exportColumns = append(exportColumns, column) | |
| exportSourceIndexes = append(exportSourceIndexes, index) | |
| } | |
| sourceColumns := make([]Column, 0, len(fields)) | |
| exportColumns := []Column{{ | |
| Name: RecordIDColumnName, System: true, | |
| }} | |
| exportSourceIndexes := make([]int, 0, len(fields)) | |
| exportNames := map[string]bool{RecordIDColumnName: true} | |
| for index := range fields { | |
| column, err := sourceColumn(fields[index], fieldIDs[index], fieldTypes[index]) | |
| if err != nil { | |
| return Page{}, err | |
| } | |
| sourceColumns = append(sourceColumns, column) | |
| // The system join key intentionally wins over a same-named Base field. | |
| if column.Name == RecordIDColumnName { | |
| continue | |
| } | |
| if exportNames[column.Name] { | |
| return Page{}, &MatrixError{Reason: fmt.Sprintf("duplicate field name %q", column.Name)} | |
| } | |
| exportNames[column.Name] = true | |
| exportColumns = append(exportColumns, column) | |
| exportSourceIndexes = append(exportSourceIndexes, index) | |
| } |
🤖 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/recordexport/dataset.go` around lines 171 - 188, Validate
column names while building exportColumns in the surrounding dataset
construction flow, rejecting any duplicate non-system names with a MatrixError
before returning the page; preserve the existing RecordIDColumnName handling so
the system join key continues to take precedence over a same-named source field.
| | `text`、`formula`、`lookup`、`auto_number`、`not_support` | `string|null` | `"进行中"`；formula、lookup 不保留结果的原始类型 | | ||
| | `datetime`、`created_at`、`updated_at` | `string|null` | `"2026-08-05T10:30:00.000+08:00"`；RFC3339，固定三位毫秒 | | ||
| | `number` | `number|null` | `12.5`；JSON 整数和小数均为 number | |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Escape literal pipes in the physical-type table.
The cells at Lines 134-136 and 139 contain | inside inline code. Markdown treats each pipe as a column separator. The rendered table is malformed, and content can be lost. Escape the pipes as \| or use a representation without raw table delimiters.
Also applies to: 139-139
🧰 Tools
🪛 markdownlint-cli2 (0.23.2)
[warning] 134-134: Table column count
Expected: 3; Actual: 4; Too many cells, extra data will be missing
(MD056, table-column-count)
[warning] 135-135: Table column count
Expected: 3; Actual: 4; Too many cells, extra data will be missing
(MD056, table-column-count)
[warning] 136-136: Table column count
Expected: 3; Actual: 4; Too many cells, extra data will be missing
(MD056, table-column-count)
🤖 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-data-analysis-sop.md` around lines 134
- 136, Update the physical-type table entries around the rows containing inline
code at lines 134–136 and 139 so every literal pipe is escaped as \| (or
replaced with an equivalent representation that contains no raw table
delimiter). Preserve the documented type and example values while keeping the
Markdown table correctly aligned.
Source: Linters/SAST tools
| ### 单表简单筛选与统计：jq | ||
|
|
||
| 一次性查询先用 `+record-list` / `+record-search` 的 filter/sort 验证。需要用户长期打开、共享或复用时，再把同一套 filter/sort 沉淀为视图。 | ||
| NDJSON 每行是一条 record。单表短筛选、计数和简单聚合可直接用 jq；下面筛选“状态”包含“进行中”的记录，并统计记录数和金额合计： | ||
|
|
||
| Example: 将已验证的筛选排序写入视图： | ||
| 默认导出后使用本地 `jq -s`，同一 artifact 可反复查询而无需重新下载。表达式很短且只执行一次，或本地 jq 不可用时，可改用 `--jq-records '<expr>'` 等价 `js -s '<expr>' records.ndjson`；注意：通用的 `--jq` 只处理 stdout 里的内容，`--jq-records` 才能处理 NDJSON 文件内容。 |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Use the correct jq command in the example.
Line 172 says js -s, but the preceding workflow uses jq -s. Change js to jq; otherwise users copying the local analysis path can receive a command-not-found failure.
🤖 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-data-analysis-sop.md` around lines 168
- 172, 在“单表简单筛选与统计：jq”段落中，将本地分析说明里的 `js -s` 更正为 `jq -s`，保持与前文 `jq -s` 工作流及
`--jq-records` 等价命令一致。
| - **高频：数据分析。** 记录作为分析、解析、比较或可复用的本地输入时，首选 `--output <path>.ndjson --minimal-stdout`，并按 [Base 数据表查询与分析 SOP](references/lark-base-data-analysis-sop.md) 处理。 | ||
| - **低频：在线复制。** 复制整个 Base 使用 `+base-copy`，复制 Base 内单张数据表使用 `+table-copy`。 | ||
| - **更低频：文件导入/导出。** 本地文件与 Base 之间的导入/导出转 `lark-drive`；具体格式、参数、路径限制和仅结构导出规则由 `lark-drive` 负责，导入完成后再回到 Base 命令。 |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Separate record-analysis artifacts from file transfer.
Line 34 introduces local NDJSON analysis artifacts, but Lines 26 and 55 still route all Base-to-local export to lark-drive. State that lark-drive handles file import/export, while record-list/search/get NDJSON analysis artifacts use this Base workflow. Otherwise users can be routed away from the commands added by this PR.
🧰 Tools
🪛 LanguageTool
[uncategorized] ~35-~35: “张” 不能与 “数据表” 搭配，请更换量词。
Context: ...。** 复制整个 Base 使用 +base-copy，复制 Base 内单张数据表使用 +table-copy。 - 更低频：文件导入/导出。 本地文...
(wa5)
🤖 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/SKILL.md` around lines 34 - 36, Clarify the workflow
guidance around the Base command section so lark-drive is used only for file
import/export, while record list/search/get operations that produce local NDJSON
analysis artifacts remain in this Base workflow. Update the related routing
statements, including the references near the high-frequency data-analysis
guidance and other Base-to-local export instructions, without changing the copy
or table-copy paths.
| func TestBaseRecordListDryRunInfersNDJSONAndCapsFirstPage(t *testing.T) { | ||
| result := runBaseDryRun(t, 0, | ||
| "base", "+record-list", | ||
| "--base-token", "app_x", | ||
| "--table-id", "tbl_x", | ||
| "--offset", "50", | ||
| "--limit", "2000", | ||
| "--output", "exports/records.ndjson", | ||
| ) | ||
|
|
||
| out := result.Stdout | ||
| require.Equal(t, "/open-apis/base/v3/bases/app_x/tables/tbl_x/records?limit=200&offset=50", gjson.Get(out, "data.api.0.url").String(), out) | ||
| require.Equal(t, "ndjson", gjson.Get(out, "data.export_format").String(), out) | ||
| require.Equal(t, int64(2000), gjson.Get(out, "data.requested_limit").Int(), out) | ||
| require.Equal(t, "exports/records.ndjson", gjson.Get(out, "data.output").String(), out) | ||
| } | ||
|
|
||
| func TestBaseRecordSearchDryRunNDJSONCapsFirstPageAndKeepsQuery(t *testing.T) { | ||
| result := runBaseDryRun(t, 0, | ||
| "base", "+record-search", | ||
| "--base-token", "app_x", | ||
| "--table-id", "tbl_x", | ||
| "--keyword", "Alice", | ||
| "--search-field", "Name", | ||
| "--offset", "25", | ||
| "--limit", "500", | ||
| "--output", "search.ndjson", | ||
| ) | ||
|
|
||
| out := result.Stdout | ||
| require.Equal(t, int64(25), gjson.Get(out, "data.api.0.body.offset").Int(), out) | ||
| require.Equal(t, int64(200), gjson.Get(out, "data.api.0.body.limit").Int(), out) | ||
| require.Equal(t, "Alice", gjson.Get(out, "data.api.0.body.keyword").String(), out) | ||
| 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) | ||
| } | ||
|
|
||
| func TestBaseRecordGetDryRunInfersNDJSON(t *testing.T) { | ||
| result := runBaseDryRun(t, 0, | ||
| "base", "+record-get", | ||
| "--base-token", "app_x", | ||
| "--table-id", "tbl_x", | ||
| "--record-id", "rec_x", | ||
| "--output", "record.ndjson", | ||
| ) | ||
|
|
||
| out := result.Stdout | ||
| require.Equal(t, "POST", gjson.Get(out, "data.api.0.method").String(), out) | ||
| require.Equal(t, "rec_x", gjson.Get(out, "data.api.0.body.record_id_list.0").String(), out) | ||
| require.Equal(t, "ndjson", gjson.Get(out, "data.export_format").String(), out) | ||
| require.Equal(t, "record.ndjson", gjson.Get(out, "data.output").String(), out) | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift
Add live E2E coverage for the NDJSON workflow.
These tests only inspect dry-run request plans. They do not verify real record reads, NDJSON and manifest publication, or cleanup. Add a self-contained create/use/cleanup flow with required credentials.
As per coding guidelines, “New flows or behavior changes require live E2E coverage with a self-contained create/use/cleanup workflow and bot credentials where applicable.”
🤖 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 118 - 169,
Add live E2E coverage alongside the dry-run tests, using the existing
base-record test helpers and bot credential configuration to create a temporary
base/table and records, run record-list, record-search, and record-get with
NDJSON output, and verify real records plus NDJSON and manifest publication.
Make the flow self-contained with deferred cleanup of all created resources and
generated artifacts, while preserving the existing dry-run assertions.
Source: Coding guidelines
| out := result.Stdout | ||
| require.Equal(t, int64(25), gjson.Get(out, "data.api.0.body.offset").Int(), out) | ||
| require.Equal(t, int64(200), gjson.Get(out, "data.api.0.body.limit").Int(), out) | ||
| require.Equal(t, "Alice", gjson.Get(out, "data.api.0.body.keyword").String(), out) | ||
| 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) | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Assert the record-search output path.
The test passes --output search.ndjson but does not assert data.output. A regression can discard the destination while preserving NDJSON inference and the request limit.
Proposed test 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)As per coding guidelines, “contract tests must assert the changed field or behavior directly.”
📝 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.
| out := result.Stdout | |
| require.Equal(t, int64(25), gjson.Get(out, "data.api.0.body.offset").Int(), out) | |
| require.Equal(t, int64(200), gjson.Get(out, "data.api.0.body.limit").Int(), out) | |
| require.Equal(t, "Alice", gjson.Get(out, "data.api.0.body.keyword").String(), out) | |
| 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) | |
| } | |
| out := result.Stdout | |
| require.Equal(t, int64(25), gjson.Get(out, "data.api.0.body.offset").Int(), out) | |
| require.Equal(t, int64(200), gjson.Get(out, "data.api.0.body.limit").Int(), out) | |
| require.Equal(t, "Alice", gjson.Get(out, "data.api.0.body.keyword").String(), out) | |
| 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 147 - 153,
Update the test assertions in the record-list dry-run test to verify that
data.output equals the requested search.ndjson destination. Keep the existing
export-format and requested-limit assertions unchanged, and assert the output
path directly so destination handling is covered.
Source: Coding guidelines
1 participant
Summary
Add a typed NDJSON artifact workflow for Base record reads so agents can perform professional local analytics—joins, window and calendar calculations, multi-value expansion, attribution, and deeper insight generation—without loading hundreds of raw records into the model context.
The existing Markdown and raw JSON paths remain compatible. The new path complements Base cloud aggregation: agents use local analysis when a complete task dataset can be kept within 2,000 records per table, and fall back to the Cloud SOP when it cannot.
Why
The current Base record response is a compact matrix (
fields,field_id_list,field_type_list,record_id_list, anddata[][]). It is efficient on the wire, but requires every downstream agent to reconstruct rows and types before using jq, Python, or a dataframe engine. Returning the matrix inline also consumes model tokens and makes repeated analysis expensive.This feature moves the deterministic transformation into
lark-clionce:Changes
Typed Base record artifacts
--format ndjsontobase +record-list,+record-search, and+record-get.--output <path>.ndjson; an output path implies NDJSON unless a conflicting format was explicitly provided.<path>.manifest.json, while stdout returns the manifest rather than record bodies.--minimal-stdoutfor repeated workflows that only need artifact paths,records_count, andhas_more.--overwritewith actionable collision errors.--jquseful for NDJSON mode by applying it to the JSON manifest on stdout.Complete local reads up to 2,000 records
--limitvalues up to 2,000 while keeping inline Markdown/JSON reads capped at 200.Stable typed row model and manifest
record_idas the non-null join key; it wins over a user field with the same name.field_idandfield_typein manifest column metadata.[]and checkbox empty cells tofalse.revwhen available; it is omitted for older server versions that do not return it.Agent analysis routing and professional Base semantics
records_count, with predicate/projection pushdown for large tables before choosing a path.+data-queryDSL out of context unless the Cloud path actually selects it, reducing prompt size and avoiding mixed execution plans.SKILL.md; behavior is covered at the command/export boundary instead.User and agent impact
Test Plan
make buildmake unit-test(full-raceunit suite acrosscmd,internal,shortcuts, andextension)go test ./shortcuts/base/... ./shortcuts/common/... ./tests/cli_e2e/base/...go vet ./...gofmt -l .returned no filesgo mod tidyproduced nogo.mod/go.sumchangesgolangci-lint v2.1.6 run --new-from-rev=upstream/mainreturned0 issues+table-list: all 50 returned tables contained numericrecords_countandrev.+base-block-list: all 94 returned table blocks contained numericrecords_countandrev.+record-list, stdout manifest, and file manifest returned the same table revision (998in the test Base).Related Issues
Summary by CodeRabbit