fix: validate base shortcut JSON object inputs#458
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughAdds pre-execution JSON validation to multiple base shortcuts by introducing Validate hooks and tightening JSON parsing/error messages so that --json inputs are enforced to be the expected object/array shapes before DryRun/Execute. Changes
Sequence DiagramsequenceDiagram
participant User
participant CLI as Shortcut Framework
participant Shortcut as Base Shortcut
participant Validator as Validate Hook
participant Parser as parseJSONObject
participant Formatter as Error Formatter
User->>CLI: run command with --json
CLI->>Shortcut: initialize runtime, invoke Shortcut
Shortcut->>Validator: Validate(ctx, runtime)
Validator->>Parser: parseJSONObject(runtime.Str("json"), "json")
alt parsed object ok
Parser-->>Validator: parsed object
Validator-->>Shortcut: nil (validation passed)
Shortcut->>CLI: proceed to DryRun/Execute
else syntax error
Parser-->>Formatter: formatJSONError / tip
Formatter-->>Validator: error
Validator-->>CLI: return validation error
CLI-->>User: surface error (e.g., "--json must be a JSON object" + tip)
else non-object (null/array)
Parser-->>Formatter: common.FlagErrorf("--json must be a JSON object"...)
Formatter-->>Validator: error
Validator-->>CLI: return validation error
CLI-->>User: surface error with tip
end
Estimated Code Review Effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly Related PRs
Suggested Reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
kongenpei has reached the 50-review limit for trial accounts. To continue receiving code reviews, upgrade your plan.
🚀 PR Preview Install Guide🧰 CLI updatenpm i -g https://pkg.pr.new/larksuite/cli/@larksuite/cli@fb25ba2eb20424a6b398ce098eb27810c8833cf6🧩 Skill updatenpx skills add larksuite/cli#codex/base-json-object-validation -y -g |
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
shortcuts/base/helpers.go (1)
33-47:⚠️ Potential issue | 🟠 Major
nullbypasses object-only validation inparseJSONObject.At line 46, the function returns success when the payload is JSON
null, sincejson.Unmarshaltreatsnullas valid input formap[string]interface{}(returning nil error and a nil map). This allows non-object input through validation and can cause downstream issues.Add an explicit nil check after successful parsing:
Proposed fix
func parseJSONObject(pc *parseCtx, raw string, flagName string) (map[string]interface{}, error) { resolved, err := loadJSONInput(pc, raw, flagName) if err != nil { return nil, err } var result map[string]interface{} if err := common.ParseJSON([]byte(resolved), &result); err != nil { var syntaxErr *json.SyntaxError if errors.As(err, &syntaxErr) { return nil, formatJSONError(flagName, "object", err) } return nil, common.FlagErrorf("--%s must be a JSON object; %s", flagName, jsonInputTip(flagName)) } + if result == nil { + return nil, common.FlagErrorf("--%s must be a JSON object; %s", flagName, jsonInputTip(flagName)) + } return result, nil }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@shortcuts/base/helpers.go` around lines 33 - 47, parseJSONObject currently accepts JSON "null" because common.ParseJSON succeeds but yields a nil map; after calling common.ParseJSON in parseJSONObject (the block that assigns to var result map[string]interface{}), add an explicit nil check (if result == nil) and return the same flag-style error used for non-object inputs (e.g. return nil, common.FlagErrorf("--%s must be a JSON object; %s", flagName, jsonInputTip(flagName))). This keeps existing syntax error handling (formatJSONError) intact and ensures only real JSON objects are accepted.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@shortcuts/base/helpers.go`:
- Around line 33-47: parseJSONObject currently accepts JSON "null" because
common.ParseJSON succeeds but yields a nil map; after calling common.ParseJSON
in parseJSONObject (the block that assigns to var result
map[string]interface{}), add an explicit nil check (if result == nil) and return
the same flag-style error used for non-object inputs (e.g. return nil,
common.FlagErrorf("--%s must be a JSON object; %s", flagName,
jsonInputTip(flagName))). This keeps existing syntax error handling
(formatJSONError) intact and ensures only real JSON objects are accepted.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 25258953-69da-4368-ab79-f1243e3dfd30
📒 Files selected for processing (15)
shortcuts/base/base_execute_test.goshortcuts/base/base_shortcut_helpers.goshortcuts/base/base_shortcuts_test.goshortcuts/base/field_ops.goshortcuts/base/helpers.goshortcuts/base/helpers_test.goshortcuts/base/record_batch_create.goshortcuts/base/record_batch_update.goshortcuts/base/record_ops.goshortcuts/base/record_search.goshortcuts/base/record_upsert.goshortcuts/base/view_ops.goshortcuts/base/view_set_group.goshortcuts/base/view_set_sort.goshortcuts/base/view_set_visible_fields.go
There was a problem hiding this comment.
kongenpei has reached the 50-review limit for trial accounts. To continue receiving code reviews, upgrade your plan.
* fix: validate base shortcut JSON object inputs * fix: reject null in base JSON object parser --------- Co-authored-by: kongenpei <kongenpei@users.noreply.github.com>
Summary
Tighten base shortcut JSON validation so commands that require JSON objects fail early with actionable CLI errors instead of deferring malformed payloads to dry-run or API passthrough.
Changes
@file.json, the command reference, and thelark-baseskill.Test Plan
lark xxxcommand works as expectedmake unit-testgo mod tidy(no changes togo.modorgo.sum)go run github.com/golangci/golangci-lint/v2/cmd/golangci-lint@v2.1.6 run --new-from-rev=origin/mainRelated Issues
Summary by CodeRabbit
Bug Fixes
Documentation