Skip to content

feat: add early JSON validation for tool call arguments - #29

Closed
echobt wants to merge 1 commit into
mainfrom
feat/early-tool-call-json-validation
Closed

feat: add early JSON validation for tool call arguments#29
echobt wants to merge 1 commit into
mainfrom
feat/early-tool-call-json-validation

Conversation

@echobt

Copy link
Copy Markdown
Contributor

Summary

This PR adds early JSON validation for tool call arguments during streaming. Previously, malformed JSON in tool call arguments was only detected at execution time. Now, validation can be performed as soon as tool calls are marked complete, providing earlier feedback and better error messages.

Changes

  • Add validate_arguments() method to StreamToolCall in streaming.rs
  • Add is_valid_complete() helper method
  • Add complete_tool_call_validated() method to StreamContent for opt-in validation

Verification

cargo check -p cortex-engine
cargo test -p cortex-engine --lib -- streaming

@greptile-apps

Copy link
Copy Markdown

Greptile Overview

Greptile Summary

This PR introduces early JSON validation for tool call arguments during streaming, enabling validation as soon as tool calls are marked complete rather than waiting until execution time. Three new methods are added to the streaming module: validate_arguments() performs JSON parsing validation, is_valid_complete() checks both completion status and argument validity, and complete_tool_call_validated() provides opt-in validation when marking a tool call complete.

Key Changes:

  • Added StreamToolCall::validate_arguments() method that checks if arguments contain valid JSON using serde_json::from_str, treating empty arguments as valid
  • Added StreamToolCall::is_valid_complete() helper that returns true only when both the complete flag is set and arguments are valid JSON
  • Added StreamContent::complete_tool_call_validated() method that marks a tool call complete and validates its arguments in one operation, returning a Result

Implementation Notes:

  • Empty or whitespace-only arguments are considered valid (no validation error)
  • Error messages include helpful context from the serde_json parser
  • The new methods are opt-in; existing complete_tool_call() continues to work without validation

Confidence Score: 4.5/5

  • This PR is safe to merge with minimal risk - adds optional validation functionality without changing existing behavior
  • High confidence due to: (1) well-contained changes that add new opt-in methods without modifying existing functionality, (2) clear error handling with descriptive messages, (3) follows existing code patterns and style, (4) handles edge cases like empty arguments appropriately. Minor deduction for one design consideration around setting complete=true before validation succeeds.
  • No files require special attention - the changes are straightforward validation additions

Important Files Changed

FilenameOverview
src/cortex-engine/src/streaming.rsAdds early JSON validation for tool call arguments with three new methods: validate_arguments(), is_valid_complete(), and complete_tool_call_validated()

Sequence Diagram

sequenceDiagram
participant Caller
participant StreamContent
participant StreamToolCall
participant JSONParser
Note over Caller,JSONParser: Early Validation Flow (New)
Caller->>StreamContent: complete_tool_call_validated(id)
StreamContent->>StreamContent: Find tool call by id
alt Tool call found
StreamContent->>StreamToolCall: Set complete = true
StreamContent->>StreamToolCall: validate_arguments()
StreamToolCall->>StreamToolCall: Check if arguments empty
alt Arguments empty
StreamToolCall-->>StreamContent: Ok(())
else Arguments not empty
StreamToolCall->>JSONParser: from_str::<Value>(&arguments)
alt Valid JSON
JSONParser-->>StreamToolCall: Ok(Value)
StreamToolCall-->>StreamContent: Ok(())
else Invalid JSON
JSONParser-->>StreamToolCall: Err(e)
StreamToolCall-->>StreamContent: Err("Invalid JSON...")
end
end
StreamContent-->>Caller: Result
else Tool call not found
StreamContent-->>Caller: Err("Tool call not found")
end
Note over Caller,JSONParser: Helper Method Usage
Caller->>StreamToolCall: is_valid_complete()
StreamToolCall->>StreamToolCall: Check complete flag
alt Is complete
StreamToolCall->>StreamToolCall: validate_arguments()
StreamToolCall-->>Caller: true/false
else Not complete
StreamToolCall-->>Caller: false
end
Loading

@greptile-appsgreptile-appsBot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

1 file reviewed, 1 comment

Edit Code Review Agent Settings | Greptile

Comment on lines +149 to +152
pub fn complete_tool_call_validated(&mut self, id: &str) -> Result<(), String> {
if let Some(tc) = self.tool_calls.iter_mut().find(|tc| tc.id == id) {
tc.complete = true;
tc.validate_arguments()

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

sets complete = true even when validation fails - may want to only set complete if validation passes

Suggested change
pubfn complete_tool_call_validated(&mutself,id:&str) -> Result<(),String>{
ifletSome(tc) = self.tool_calls.iter_mut().find(|tc| tc.id == id){
tc.complete = true;
tc.validate_arguments()
ifletSome(tc) = self.tool_calls.iter_mut().find(|tc| tc.id == id){
let validation = tc.validate_arguments();
if validation.is_ok(){
tc.complete = true;
}
validation
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/cortex-engine/src/streaming.rs
Line: 149:152
Comment:
sets `complete = true` even when validation fails - may want to only set complete if validation passes
```suggestion if let Some(tc) = self.tool_calls.iter_mut().find(|tc| tc.id == id) { let validation = tc.validate_arguments(); if validation.is_ok() { tc.complete = true; } validation```
How can I resolve this? If you propose a fix, please make it concise.

@echobt

Copy link
Copy Markdown
ContributorAuthor

Closing to consolidate: This early JSON validation will be merged with PRs #25 and #34 into a consolidated streaming/timeout improvements PR.

@echobtechobt closed this Feb 4, 2026
echobt added a commit that referenced this pull request Feb 4, 2026
## Summary
This PR consolidates **3 feature PRs** for streaming and timeout improvements.
### Included PRs:
- #25: Add per-chunk timeout for SSE streaming to prevent hangs
- #29: Add early JSON validation for tool call arguments
- #34: Add per-tool timeout in batch execution
### Key Changes:
- Added CHUNK_TIMEOUT_SECS constant for SSE streaming (60s)
- Wrapped SSE event iteration with tokio::time::timeout
- Added validate_arguments() method to StreamToolCall
- Added is_valid_complete() helper and complete_tool_call_validated() method
- Added DEFAULT_TOOL_TIMEOUT_SECS constant (60 seconds) for batch
- Added tool_timeout_secs field to BatchToolArgs for configuration
- Applied individual timeout to each tool execution in execute_parallel()
### Files Modified:
- src/cortex-engine/src/client/cortex.rs
- src/cortex-engine/src/streaming.rs
- src/cortex-engine/src/tools/handlers/batch.rs
- src/cortex-engine/src/tools/unified_executor.rs
Closes#25, #29, #34
echobt added a commit that referenced this pull request Feb 4, 2026
* feat: consolidated streaming and timeout improvements
## Summary
This PR consolidates **3 feature PRs** for streaming and timeout improvements.
### Included PRs:
- #25: Add per-chunk timeout for SSE streaming to prevent hangs
- #29: Add early JSON validation for tool call arguments
- #34: Add per-tool timeout in batch execution
### Key Changes:
- Added CHUNK_TIMEOUT_SECS constant for SSE streaming (60s)
- Wrapped SSE event iteration with tokio::time::timeout
- Added validate_arguments() method to StreamToolCall
- Added is_valid_complete() helper and complete_tool_call_validated() method
- Added DEFAULT_TOOL_TIMEOUT_SECS constant (60 seconds) for batch
- Added tool_timeout_secs field to BatchToolArgs for configuration
- Applied individual timeout to each tool execution in execute_parallel()
### Files Modified:
- src/cortex-engine/src/client/cortex.rs
- src/cortex-engine/src/streaming.rs
- src/cortex-engine/src/tools/handlers/batch.rs
- src/cortex-engine/src/tools/unified_executor.rs
Closes#25, #29, #34
* fix(batch): implement timeout_secs parameter for overall batch timeout
Address Greptile review feedback: The timeout_secs parameter was
documented and accepted but never used. Now it properly wraps the
entire parallel execution with a batch-level timeout, separate from
the per-tool timeout_secs.
- Add batch-level timeout wrapper around execute_parallel
- Return descriptive error message when batch times out
- Add test for batch timeout behavior
Sign up for freeto 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

@echobt