Skip to content

fix(cli): merge partial scenario updates - #769

Open
92Infinitus92 wants to merge 3 commits into
solana-foundation:mainfrom
LimeChain:fix/scenario-partial-patch
Open

fix(cli): merge partial scenario updates#769
92Infinitus92 wants to merge 3 commits into
solana-foundation:mainfrom
LimeChain:fix/scenario-partial-patch

Conversation

@92Infinitus92

Copy link
Copy Markdown
Contributor

PATCH /v1/scenarios/{id} currently replaces the complete stored scenario. A client that sends only a changed name or description can therefore erase overrides, tags, and backend-only data.

PATCH now merges supplied fields into an existing scenario, rejects unknown fields, and keeps the path id authoritative. Complete scenario documents retain the existing upsert behavior for compatibility.

@greptile-apps

greptile-apps Bot commented Aug 19, 2026

Copy link
Copy Markdown
Contributor

Greptile Summary

The PATCH scenario endpoint now merges partial updates into existing scenarios while retaining full-document upsert compatibility.

  • Keeps omitted scenario fields intact during partial updates.
  • Makes the path identifier authoritative.
  • Recursively rejects unknown fields before mutating stored state.
  • Adds endpoint and helper-level coverage for partial merges, nested validation, invalid input, and upserts.

Confidence Score: 5/5

The PR appears safe to merge.

The recursive supplied-versus-normalized field validation now rejects nested unknown override fields before stored scenarios are mutated, so no blocking failure remains.

Important Files Changed

Filename Overview
crates/cli/src/http/mod.rs Implements validated partial scenario merging and fully addresses the previously reported nested unknown-field bypass without introducing another reachable failure.

Reviews (2): Last reviewed commit: "fix(cli): reject nested unknown scenario..." | Re-trigger Greptile

Comment on lines +272 to +274
if key != "id" && !obj.contains_key(key) {
return Err(format!("Unknown scenario field '{key}'"));
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Nested unknown fields bypass validation

When a PATCH contains a misspelled or unsupported field inside an overrides element, the top-level allowlist accepts overrides and permissive deserialization discards the nested field, causing the endpoint to return success even though the requested property was not stored.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@greptileai check commit 66e589a for a solution, review it

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The solution in this commit effectively addresses the original concern. Here's what was added:

deserialize_scenario_strictly round-trips the value through serde (from_valueScenarioto_value) to get a normalized reference, then calls validate_json_fields to recursively compare the supplied JSON against it.

validate_json_fields walks both trees in parallel — if a key exists in supplied but is absent from normalized (because serde dropped it as unknown), it returns an error with the full dotted path like "Unknown field 'scenario.overrides[0].fetchBeforeUes'".

The test a_nested_unknown_override_field_is_rejected_without_mutating specifically covers the case from my original comment (a misspelled fetchBeforeUes inside an override element), and the arbitrary_nested_override_values_remain_valid test confirms that the open-ended values map still accepts arbitrary nested content — which is the right carve-out since values is typed as serde_json::Value.

One minor edge case to be aware of: the array comparison uses .zip(), which silently truncates if supplied has more elements than normalized. In practice this can't happen here because the supplied array passes through serde unchanged, but it's worth noting if the logic is ever reused in a context where serde could produce a shorter array (e.g., deduplication).

Overall this looks good — the recursive field validation is the right approach and the tests cover the key scenarios.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant