feat(base): require --fields on +table-create - #2221
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThe ChangesExplicit table schema creation
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant CLI
participant validateTableCreate
participant TableCreateRequest
CLI->>validateTableCreate: validate --fields JSON
validateTableCreate-->>CLI: return validation error or valid schema
validateTableCreate->>TableCreateRequest: pass validated fields
Possibly related PRs
🚥 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 |
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
|
Thanks for the review — pushed 38677e1. Two of the three points were valid; one I'd like to push back on with evidence. 1. Typed metadata on the rejection tests — fixed for the invalid-schema case.
2. Same request for the missing-flag test — not applicable at that layer. The suggestion was to assert The envelope-level contract is already pinned where it actually holds, in 3. Stale fieldless-create guidance — fixed. Fair catch: both the tip and the skill note read as if the fieldless path were still reachable through Re-ran the full gate after the changes: Heads-up on an overlap: #2153 also rewrites |
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>
Address review feedback on the +table-create schema validation. - The invalid-fields-JSON rejection now asserts category, subtype and param through assertInvalidArgumentValidation, plus the preserved *json.SyntaxError cause, instead of only asserting that some error came back. - Document why the missing-flag test asserts cobra's text rather than errs metadata, and pin that layer boundary: cobra's ValidateRequiredFlags emits a plain error and the dispatcher types it later (cmd/root_test.go). The test now fails if that boundary moves, so the weaker assertion cannot silently outlive its reason. - Reword the --fields tip and the lark-base skill note: both described the fieldless path as if it were still reachable through +table-create. They now say the command rejects omitted / blank / empty schemas up front, while keeping why the schema must be declared here. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
38677e1 to
6950fca
Compare
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
The cobra Required declaration and flag Desc already advertise the --fields requirement, so the tips paragraph restating it (and its SKILL.md / coverage.md echoes) is dropped. The select-field example now carries multiple/hue/lightness so agents copy a complete option shape.
🚀 PR Preview Install Guide🧰 CLI updatenpm i -g https://pkg.pr.new/larksuite/cli/@larksuite/cli@c7b76a7a521efb4a883c7b1f3453810f50b5a5a2🧩 Skill updatenpx skills add CarolSum/cli#feat/base-table-create-require-fields -y -g |
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