Skip to content

fix(schema): preserve explicit additionalProperties semantics#113

Merged
sara-dini merged 2 commits into
mainfrom
sara/fix-noarg-schema-106
May 16, 2026
Merged

fix(schema): preserve explicit additionalProperties semantics#113
sara-dini merged 2 commits into
mainfrom
sara/fix-noarg-schema-106

Conversation

@sara-dini
Copy link
Copy Markdown
Collaborator

Summary

  • keep no-arg schema normalization (properties/required) for OpenAI fn conversion compatibility
  • stop forcing additionalProperties: false when the input schema already declares properties
  • add regression test to lock this behavior

Why

Current rewrite path over-constrains valid argument schemas by injecting additionalProperties: false even when a tool intentionally leaves default JSON Schema behavior (true). This can reject legitimate payloads.

Validation

  • Added unit test: test_rewrite_tools_list_does_not_force_additional_properties_on_arg_schema
  • Local execution attempt blocked in this environment due to missing linker:
    • error: linker \cc` not found`
  • CI should run full Rust tests.

Copy link
Copy Markdown

@gemini-code-assist gemini-code-assist Bot left a comment

Choose a reason for hiding this comment

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

Code Review

This pull request introduces normalization logic for tool inputSchema in src/main.rs to ensure compatibility with OpenAI-style function calls by providing explicit defaults for 'properties', 'required', and 'additionalProperties' fields. Comprehensive unit tests were also added to verify these behaviors. Feedback was provided regarding the efficiency of the map lookups, suggesting a more idiomatic Rust approach using a single entry-based match to avoid redundant operations.

Comment thread src/main.rs
Comment on lines +4571 to +4584
if schema.get("type").and_then(|t| t.as_str()) == Some("object") {
let had_properties = schema.get("properties").is_some();
schema
.entry("properties".to_string())
.or_insert_with(|| json!({}));
schema
.entry("required".to_string())
.or_insert_with(|| json!([]));
if !had_properties {
schema
.entry("additionalProperties".to_string())
.or_insert_with(|| json!(false));
}
}
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

medium

The normalization logic for inputSchema is correctly implemented to ensure OpenAI compatibility while preserving existing additionalProperties semantics. However, the current implementation performs multiple redundant lookups on the schema map (e.g., get, entry, entry, entry). While not a critical performance bottleneck in this context, combining these into a single entry-based match would be more efficient and idiomatic Rust.

References
  1. Prefer efficient map access patterns in Rust to avoid redundant lookups and allocations. (link)

@sara-dini
Copy link
Copy Markdown
Collaborator Author

Sweep update (2026-05-16 UTC): pushed commit 40c107f to address review feedback on schema normalization style.

What changed:

  • Rewrote inputSchema object normalization to use serde_json::map::Entry for properties, required, and conditional additionalProperties insertion.
  • Removes redundant map lookups while preserving existing behavior exactly.

Validation status:

  • Local cargo test rewrite_tools_list is currently blocked in this environment by missing system linker: linker \cc` not found`.

Next concrete step:

  • Let CI run on this commit; if CI is green, this thread is ready for reviewer re-check/merge.

@sara-dini sara-dini force-pushed the sara/fix-noarg-schema-106 branch from 40c107f to fdc418d Compare May 16, 2026 07:02
@sara-dini
Copy link
Copy Markdown
Collaborator Author

Status delta (2026-05-16 UTC): rebased onto and resolved conflict while preserving schema-normalization behavior + strict test assertions.\n\nCurrent state:\n- (conflict gate cleared)\n- branch force-updated to \n\nValidation:\n- remains blocked in this environment by missing linker: \n\nNext concrete step:\n- Let CI run on ; if green, proceed with review/merge.

@sara-dini
Copy link
Copy Markdown
Collaborator Author

Superseding prior status comment (shell formatting issue).

Status delta (2026-05-16 UTC): rebased sara/fix-noarg-schema-106 onto origin/main and resolved src/main.rs conflict while preserving schema-normalization behavior and strict test assertions.

Current state:

  • mergeable=MERGEABLE (conflict gate cleared)
  • branch force-updated to fdc418d

Validation:

  • cargo test rewrite_tools_list remains blocked in this environment by missing linker: linker \cc` not found`

Next concrete step:

  • Let CI run on fdc418d; if green, proceed with review/merge.

Copy link
Copy Markdown
Collaborator

@vish-dini vish-dini left a comment

Choose a reason for hiding this comment

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

Change is scoped correctly. Preserves explicit JSON Schema semantics without regressing no-arg normalization, and the regression test covers the boundary that matters.

@sara-dini sara-dini merged commit 4bed8eb into main May 16, 2026
2 checks passed
@sara-dini sara-dini deleted the sara/fix-noarg-schema-106 branch May 16, 2026 11:01
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.

2 participants