[Silvertree2010]

Found while testing crates/engine/src/script.rs:50-54 (parse, compact command {json} branch) (no existing issue): Script compact lines accept non-object params (JSON steps reject them).

Cause

The compact command.id <json> / command.id=<json> branch of script::parse built the Step straight from whatever JSON followed the id. It skipped the params must-be-an-object check that step_of applies to JSON steps, so a.b 5 or a.b [1] parsed as valid steps.

Fix

After parsing the JSON in the compact branch, parse now rejects any value that isn't an object. It returns line N: params must be an object, matching the JSON form's wording. The regression test covers numbers, arrays, strings, booleans and null, and checks that an object still parses.

Verified

• Regression test script::tests::compact_lines_require_object_params: fails on main without the fix and passes with it (cargo test -p designcraft-engine --lib compact_lines_require_object_params).
• cargo xtask ci passes except menus::tests::go_back_after_presentation_returns_to_the_last_spread_presented, menus::tests::go_back_and_go_forward_retrace_the_pages_viewed, menus::tests::go_back_returns_to_the_view_a_jump_left_and_scrolling_adds_no_entry, menus::tests::layout_menu_goes_through_pages_and_spreads_with_their_shortcuts, menus::tests::next_spread_at_the_last_spread_is_no_jump, panel_docking::tests::the_dock_is_at_the_right_in_english_and_the_left_in_arabic, panels::styles::tests::named_character_styles_and_redundant_local_values_are_not_paragraph_overrides, panels::styles::tests::paragraph_color_override_is_marked_and_cleared_from_the_row_menu, shortcuts_dialog::tests::a_recorded_chord_runs_nothing_changes_the_menus_at_once_and_ok_keeps_a_custom_set, shortcuts_dialog::tests::an_indesign_set_imports_through_the_review_as_a_new_set, shortcuts_dialog::tests::assign_takes_a_held_shortcut_in_one_step_for_the_menus_and_the_keys, shortcuts_dialog::tests::cancel_puts_the_keys_and_the_sets_back, toolbar::tests::the_tools_panel_lays_out_like_indesigns, which also fail on main without this change.

Not changed

A compact line with nothing after the id still gets empty {} params. An explicit null after the id is now rejected like any other non-object value. The JSON array and JSON-lines paths and step_of are unchanged.

🤖 Generated with AI by ApolloOffice, see the Co-Authored-By trailers for the models

Summary by CodeRabbit

• Bug Fixes
  • Compact-line parsing now reports an error when params is not an object. Empty params continue to default to an empty object.

[coderabbitai]

No actionable comments were generated in the recent review.

Recent review info

Run configuration

• Source mode: Auto
• Repository: storytold/designcraft
• Branch: main
• File: .coderabbit.yaml
• Commit: 98d8016
• Layers: Repository YAML
• Review profile: CHILL
• Plan: Advanced
• Run ID: 60bcb746-280e-4bdc-976f-9bb09bad0d82

Commits

Reviewing files that changed from the base of the PR and between 98d8016 and 8b9f297.

Files selected for processing: 1

• crates/engine/src/script.rs

Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 7 remain after this review.

Walkthrough

Compact-line parsing now rejects non-object params with a params must be an object error. Empty params still default to {}. Tests cover non-object values and successful object params.

Changes: Compact-line params validation

Layer / File(s)	Summary
Validate compact-line params crates/engine/src/script.rs	The parser rejects scalar, array, string, and null params. Tests confirm object params remain accepted.

Estimated code review effort: 2 (Simple) | ~5 minutes

Change: Bug fix

Suggested reviewers: echelon

Merge Risk: Minimal · up to 8b9f2

The change preserves supported script workflows while rejecting out-of-contract compact parameters with a clear error.

Architecture risk: Medium · up to 8b9f2

The change affects 1 system.

Changed systems: crates

Architecture concerns
No architecture-level concerns identified.

Review details

Systems and components

• observed — crates (service) was modified; 1 changed file maps to changed impact.

Before / after behavior

• observed — Modified behavior in crates/engine/src/script.rs: Compact-line parsing now accepts parsed params only when they are a JSON object; previously it constructed a Step with any JSON value. Invalid values return params must be an object.
• observed — Modified behavior in crates/engine/src/script.rs: Added a test asserting compact-line parsing rejects numeric, array, string, and null params, and accepts object params.

Reliability and maintainability

• inferred — Risk-relevant change factors for crates: blast_radius_1; direct_dependents_1

Pre-merge checks: 4 · 1

Failed checks: 1 warning

Check name	Status	Explanation	Resolution
Docstring Coverage		Docstring coverage is 50.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff with cyclomatic complexity of at least 2. Documenta…	Write docstrings for the functions missing them to satisfy the coverage threshold.

Passed checks: 4 passed

Check name	Status	Explanation
Description Check		Check skipped - CodeRabbit’s high-level summary is enabled.
Title check		The title clearly and concisely describes the main change: requiring object-valued params in compact script lines.
Linked Issues check		Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check		Check skipped because no linked issues were found for this pull request.

Full details: Docstring Coverage

Explanation

Docstring coverage is 50.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff with cyclomatic complexity of at least 2. Documentation-only edits and deletions within surviving functions are included; unchanged diff context and fully deleted functions are excluded. Analyzed 2 functions across 1 files.

Resolution

Write docstrings for the functions missing them to satisfy the coverage threshold.

Finishing Touches

Generate unit tests (beta)

• Create a new PR

• Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

[FredApps]

ReviewerA — source-read COMMENT (not an approval)

I read the diff: compact script lines require object parameters. A script line command {...} parsed to any JSON value as parameters (a.b 5, a.b [1], a.b "x", a.b=true, a.b null), and now only an object is accepted, otherwise the line fails with line 1: `params` must be an object; the test lists the five bad forms. Commands read their parameters as a JSON object, so a scalar would have failed later with a less clear error (or been mistaken for something else), and failing at the parse step with the line number is the better behaviour. a.b null being rejected is a small compatibility break for scripts that wrote null for "no parameters": say so, or treat null as an empty object. I have not run it, and no exact-head CI result exists for this head. Nothing was run.

I ran nothing and have no exact-head CI result for this PR, so this is not an approval. Where a diff was long I read its code hunks rather than every generated or documentation line.

Status: waiting_author.