feat(base): require --fields on +table-create - #2221
Open
CarolSum wants to merge 1 commit into
Open
Conversation
A table created without --fields gets the platform default schema. Those default fields then sit in the table alongside every field the caller adds afterwards, and no field command removes them all, so the only clean recovery is to drop the table and start over. Make --fields required so the schema is declared up front, the way +base-create already recommends via --table-name + --fields. - Mark --fields Required on +table-create, and reject blank / non-array / empty-array values in Validate: cobra's MarkFlagRequired only checks that the flag was set, so --fields "" and --fields "[]" would still reach the API with no fields body and fall back to the default schema. - Validate runs ahead of the dry-run branch, so --dry-run can no longer preview an invocation the real call would reject. - Update the lark-base skill, e2e coverage notes and the live e2e helper. BREAKING CHANGE: `lark-cli base +table-create --base-token <t> --name <n>` without --fields now fails with a validation error instead of creating a default-schema table. Callers that relied on create-empty-then-add-fields must pass the schema to --fields. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
github-actions
Bot
added
domain/base
|
lijiehong seems not to be a GitHub user. You need a GitHub account to be able to sign the CLA. If you have already a GitHub account, please add the email address used for this commit to your account. You have signed the CLA already but the status is still pending? Let us recheck it. |
📝 WalkthroughWalkthroughThe ChangesExplicit table schema creation
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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 `@shortcuts/base/base_shortcuts_test.go`:
- Around line 1224-1226: Strengthen the invalid-fields rejection test in
shortcuts/base/base_shortcuts_test.go:1224-1226 by using errs.ProblemOf to
assert the validation category and subtype, errors.As to inspect
*errs.ValidationError and verify its Param, and an errors.Is or equivalent
assertion for the JSON parse cause. Apply the same typed-error assertions in
shortcuts/base/table_create_test.go:35-41, including Param == "--fields" and
preservation of the wrapped Cobra required-flag cause.
In `@shortcuts/base/table_create.go`:
- Line 34: Update the fieldless-create guidance in
shortcuts/base/table_create.go lines 34-34 and skills/lark-base/SKILL.md lines
84-84 to state that omitted, blank, or empty schemas are rejected before table
creation, rather than creating platform-default fields; keep both descriptions
consistent with the current validation behavior.
🪄 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: 0d1d98bd-f502-4032-8af9-7371cc8f301e
📒 Files selected for processing (8)
shortcuts/base/base_dryrun_ops_test.goshortcuts/base/base_shortcuts_test.goshortcuts/base/table_create.goshortcuts/base/table_create_test.goshortcuts/base/table_ops.goskills/lark-base/SKILL.mdtests/cli_e2e/base/coverage.mdtests/cli_e2e/base/helpers_test.go
Comment on lines
+1224
to
1226
| if err := BaseTableCreate.Validate(ctx, newBaseTestRuntime(map[string]string{"base-token": "b", "name": "Orders", "fields": "{"}, nil, nil)); err == nil { | ||
| t.Fatal("invalid fields json should fail CLI validate") | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Protect typed validation errors in both new rejection tests.
Both tests currently accept any error with matching presence or text. This does not protect the machine-readable validation contract.
shortcuts/base/base_shortcuts_test.go#L1224-L1226: assert category and subtype witherrs.ProblemOf, assertParamthrougherrors.Ason*errs.ValidationError, and verify the JSON parse cause.shortcuts/base/table_create_test.go#L35-L41: assert category and subtype witherrs.ProblemOf, assertParam == "--fields"througherrors.As, and verify the wrapped Cobra required-flag cause.
As per coding guidelines, error-path tests must assert typed metadata and preserve the cause. Based on learnings, errs.ProblemOf does not expose Param; read it from *errs.ValidationError through errors.As.
📍 Affects 2 files
shortcuts/base/base_shortcuts_test.go#L1224-L1226(this comment)shortcuts/base/table_create_test.go#L35-L41
🤖 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 1224 - 1226, Strengthen
the invalid-fields rejection test in
shortcuts/base/base_shortcuts_test.go:1224-1226 by using errs.ProblemOf to
assert the validation category and subtype, errors.As to inspect
*errs.ValidationError and verify its Param, and an errors.Is or equivalent
assertion for the JSON parse cause. Apply the same typed-error assertions in
shortcuts/base/table_create_test.go:35-41, including Param == "--fields" and
preservation of the wrapped Cobra required-flag cause.
Sources: Coding guidelines, Learnings
| }, | ||
| Tips: []string{ | ||
| `Example: lark-cli base +table-create --base-token <base_token> --name "Tasks" --fields '[{"name":"Title","type":"text"},{"name":"Status","type":"select","options":[{"name":"Todo"},{"name":"Done"}]}]'`, | ||
| "--fields is required: a table created without it gets the platform default schema, and those default fields stay in the table alongside the ones you add.", |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Update the stale fieldless-create guidance.
The command now rejects missing --fields before it creates a table. Both locations describe the prior platform behavior as if it remains reachable through +table-create.
shortcuts/base/table_create.go#L34-L34: state that the command rejects omitted, blank, and empty schemas before platform-default fields can be created.skills/lark-base/SKILL.md#L84-L84: replace the claim that omission creates a table with the current validation behavior.
📍 Affects 2 files
shortcuts/base/table_create.go#L34-L34(this comment)skills/lark-base/SKILL.md#L84-L84
🤖 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_create.go` at line 34, Update the fieldless-create
guidance in shortcuts/base/table_create.go lines 34-34 and
skills/lark-base/SKILL.md lines 84-84 to state that omitted, blank, or empty
schemas are rejected before table creation, rather than creating
platform-default fields; keep both descriptions consistent with the current
validation behavior.
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
2 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
+table-createwithout--fieldscreates a table with the platform default schema. Those default fields then coexist with every field the caller adds afterwards, and no field command removes them all — the only clean recovery is to delete the table and start over. This makes--fieldsrequired, so the schema is declared up front, matching what+base-createalready recommends via--table-name+--fields.Concretely, the failure this prevents: an agent runs
+table-create --name X, gets a table carrying the default schema, adds its own N fields, and ends up with a table of N + default-schema fields — while reporting N to the user, because the extra columns were never part of its plan.Changes
--fieldsRequiredon+table-create, so--helpand the machine-readable schema both advertise it.--fieldsinValidate. Cobra'sMarkFlagRequiredonly checks that the flag was set, so--fields ""and--fields "[]"would still reach the API with nofieldsbody and silently fall back to the default schema. Both now return a typed*errs.ValidationErrorwithparam: --fieldsand a hint. This mirrors the two-layer pattern already used bysheets/+history-revert.Validateruns ahead of the dry-run branch, so--dry-runcan no longer preview an invocation the real call would reject.skills/lark-base/SKILL.md,tests/cli_e2e/base/coverage.md, and the live e2e helper (all three existing live-e2e callers already pass a schema, so no live coverage is lost).Test Plan
make unit-testclean; new contract tests inshortcuts/base/table_create_test.gocover theRequireddeclaration, missing / blank / empty-array / non-object-item rejection, the dry-run gate, and the valid path. Each fails if the corresponding code change is reverted.lark-cli base +table-createflow works as expected:--fields→{"type":"validation","subtype":"invalid_argument","message":"required flag(s) \"fields\" not set"}--fields '[]'→{"type":"validation","subtype":"invalid_argument","message":"--fields must define at least one field","param":"--fields","hint":"An empty array is not a schema: ..."}--fields '[{"name":"Title","type":"text"}]'→POST /open-apis/base/v3/bases/:base_token/tableswith the schema inline, unchanged from beforego vet ./...,gofmt -l .,go mod tidy(no change),golangci-lint run --new-from-rev=origin/main(0 issues)Notes for reviewers
This is a breaking change:
+table-create --base-token <t> --name <n>without--fieldsnow fails instead of creating a default-schema table. Callers that relied on create-empty-then-add-fields must pass the schema to--fields. If you would rather stage it, the same validation could first ship as a deprecation warning on the fieldless path — happy to rework it that way.One thing I deliberately left alone: the missing-flag case still surfaces cobra's terse
required flag(s) "fields" not set, withoutparamorhint, because enriching that path touches the shared dispatcher incmd/root.goand belongs in its own PR. The flag description and tips carry the guidance in--helpmeanwhile.Related Issues
Summary by CodeRabbit
New Features
--fields.Bug Fixes
Tests