sse_emitter: log sanitized single-line tool_buffer on parse failure (Stage 3) - #650
Conversation
There was a problem hiding this comment.
1 issue found across 6 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="server/src/server/tool_parser.cpp">
<violation number="1" location="server/src/server/tool_parser.cpp:585">
P2: Malformed `arguments`/`parameters` strings are now accepted instead of rejected. Previously `json::parse(...)` in a try/catch returned `false`, dropping the call; now `parse_arg_string_or_obj` returns `true` with an empty args object while preserving the raw (unparseable) string as `tool_call.arguments`. Because `parse_tool_calls` returns that call, `tool_calls_` is non-empty in `emit_finish`, so the new "parse failed; suppressing buffered tool text" logging never fires for these buffers, and a call whose arguments are not valid JSON is emitted to clients as a valid tool call. Confirm this is the intended Stage-3 behavior; if the goal is only better diagnostics, consider keeping the suppression for unparseable argument strings.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
| if (!parsed.is_discarded() && parsed.is_object()) { | ||
| out_args = std::move(parsed); | ||
| } else { | ||
| out_args = json::object(); |
There was a problem hiding this comment.
P2: Malformed arguments/parameters strings are now accepted instead of rejected. Previously json::parse(...) in a try/catch returned false, dropping the call; now parse_arg_string_or_obj returns true with an empty args object while preserving the raw (unparseable) string as tool_call.arguments. Because parse_tool_calls returns that call, tool_calls_ is non-empty in emit_finish, so the new "parse failed; suppressing buffered tool text" logging never fires for these buffers, and a call whose arguments are not valid JSON is emitted to clients as a valid tool call. Confirm this is the intended Stage-3 behavior; if the goal is only better diagnostics, consider keeping the suppression for unparseable argument strings.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At server/src/server/tool_parser.cpp, line 585:
<comment>Malformed `arguments`/`parameters` strings are now accepted instead of rejected. Previously `json::parse(...)` in a try/catch returned `false`, dropping the call; now `parse_arg_string_or_obj` returns `true` with an empty args object while preserving the raw (unparseable) string as `tool_call.arguments`. Because `parse_tool_calls` returns that call, `tool_calls_` is non-empty in `emit_finish`, so the new "parse failed; suppressing buffered tool text" logging never fires for these buffers, and a call whose arguments are not valid JSON is emitted to clients as a valid tool call. Confirm this is the intended Stage-3 behavior; if the goal is only better diagnostics, consider keeping the suppression for unparseable argument strings.</comment>
<file context>
@@ -569,82 +569,59 @@ static bool parse_complete_parameter_body(const std::string & body,
+ if (!parsed.is_discarded() && parsed.is_object()) {
+ out_args = std::move(parsed);
+ } else {
+ out_args = json::object();
+ }
+ return true;
</file context>
a2a3005 to
ff4092e
Compare
|
@cubic-dev-ai review |
@dpavlin I have started the AI code review. It will take a few minutes to complete. |
There was a problem hiding this comment.
All reported issues were addressed across 6 files
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
…ason - http_server: suppress 'length' / 'max_tokens' finish reason when generation terminates on EOS token - tool_parser: reject empty tool names and scalar/array arguments strings while preserving object syntax error fallback - tool_parser: extract top-level tool name outside arguments object span in syntax error fallback - tool_parser: support mixed <invoke> and JSON lines siblings in <function_calls> blocks - test_server_unit: add regression tests for all 4 cases
ff4092e to
a5cd646
Compare
- escape_for_logging: escape control bytes unambiguously with fixed-width \u00%02x and \u0000 - log full escaped tool_buffer_ payload on parse failure for journalctl compatibility - test_server_unit: add StderrCapture RAII test guard and logging verification tests
a5cd646 to
7ebe581
Compare
Refs #644 (Stage 3 of 3, builds upon Stage 2)
Description
This PR improves observability and diagnostic logging for streaming tool call parse failures:
SseEmitter::emit_finishlogs the raw intercepted buffer.journalctl), the payload is escaped viaescape_for_logging:\n,\r,\t,\\,\',\0).\u00%02x(avoiding variable-width hex ambiguity when followed by literal hex characters).Verification
test_escape_for_loggingunit test covering NUL bytes, standard escapes, and control characters followed by hex digits.test_emitter_suppresses_malformed_multiline_tool_bufferasserting that the full escaped payload appears on a single stderr line.