fix(cli): escape LLM judge feedback quotes

[Alok-Jadhao]

PR description

Summary

Fixes the --exit-code LLM-judge failure output when feedback contains embedded double quotes or backslashes.

Previously, feedback such as:

The agent added "(Design)" to Jordan Lee's name...

could produce an ambiguous feedback="..." value that breaks simple shell-based parsing.

Changes

Escape " as \" and \ as \\ in ``_format_feedback_snippet.
Preserve the existing `key=value` FAIL-line format.
Add unit coverage for quote and backslash escaping.
Add regression coverage using the reported `(Design)` LLM-judge feedback example.

Scope

This implements Option 1 from #84. No new JSONL output mode or CLI flag is introduced.

Verification

pytest -q tests/test_cli.py -k "feedback_snippet or evaluate_exit_code_llm_judge"
pytest -q tests/test_cli.py
git diff --check

Closes #84.

[caohy1988]

Code review

Verdict: approve with one small fix. The core is right: escaping backslashes before quotes is the correct order, and escaping after truncation is the correct sequence (truncating after escaping could split a \" pair and leave a dangling backslash at the ellipsis boundary). I checked both consumers of _format_feedback_snippet — the feedback="{snippet}" emission at cli.py:701 and the fallback line at :712 — and both are double-quoted contexts, so the escaping is right for both. This matches Option 1 from #84 as described.

Findings:

1. Escaped output can exceed max_chars (low). The docstring promises the snippet "truncates to max_chars", and test_max_chars_param_respected pins len(out) == 50 — but escaping happens after truncation, so a 50-char snippet dense with quotes/backslashes can grow to ~100 chars. The existing length test only passes because its input contains no escapable characters. Not worth reordering (the current order is the correct one for escape integrity) — just amend the docstring to say the cap applies to the pre-escape text, and consider a companion test asserting the post-escape behavior so the next reader doesn't "fix" the ordering.

2. Optional simplification (nit). json.dumps(snippet) would produce the quoted-and-escaped field in one step (and also handle any future control characters), letting the call site emit feedback={json.dumps(snippet)} instead of hand-rolling the two replaces. Fine either way — the hand-rolled version is small and tested.

3. CI hasn't run. Only check-changes and CLA have executed; the test/format workflows are pending maintainer approval for a first-time contributor, so the suite claims in the PR description are currently unverified by CI. A maintainer needs to approve the workflow run before merge.

[caohy1988]

Full review - Ready with one fix

I reviewed the complete diff, linked issue #84, caller behavior, tests, and current CI at head 0cf1824. The quote/backslash escaping direction matches the requested fix, but the new helper no longer honors its documented size bound.

P2 - Escaping occurs after the length cap

src/bigquery_agent_analytics/cli.py:619-625 truncates snippet to max_chars and then escapes every quote and backslash. Each escaped character adds another character after the cap. A direct reproduction with the default limit returns length 240 for 120 quotes, and likewise for 120 backslashes, although the helper promises a bounded snippet.

Please build the escaped value within the requested limit (without splitting an escape pair), and add quote-only and backslash-only boundary tests that assert len(result) <= max_chars.

Current CI is green. This finding is based on the helper's explicit contract and a direct executable reproduction.

[caohy1988]

Review: Approve

Small, correct, well-scoped fix implementing exactly Option 1 from #84. The escaping is applied in the right function, in the right order (backslash first, then quote), at the right stage (after whitespace collapsing and truncation, so escape sequences can never be split). Ran tests/test_cli.py locally: 137/137 pass.

Context note for other reviewers: there's no injection/SQL/shell-eval concern here to begin with — the FAIL line is a human/CI-log string written to stderr via typer.echo (cli.py:707), not SQL, not JSON, not shell-executed — so the C-style \" / \\ convention is the appropriate choice for a double-quoted key=value field. Without the fix, judge feedback containing " (the "(Design)" example from #84) breaks naive "-splitting parsers (awk -F'"', cut -d'"'); exit codes and scoring are unaffected.

Findings

MINOR — max_chars no longer bounds the returned length (src/bigquery_agent_analytics/cli.py:619-625)
Escaping happens after truncation, so a feedback string of 120 quotes/backslashes returns ~240 chars. The docstring still says "truncates to max_chars" and "keep the visual width capped" — now only true pre-escape. This is the correct order (escaping before truncation could split \ from "), but the docstring should be softened, e.g. "truncates to max_chars before escaping". Worst case ~2× on a 120-char budget, harmless for a CI log line.

NIT — PR description's verification command doesn't select the new tests
pytest -q tests/test_cli.py -k "feedback_snippet or evaluate_exit_code_llm_judge" selects only 2 tests (verified: 2 passed, 135 deselected). The new test class TestFormatFeedbackSnippet has no underscores, so -k feedback_snippet doesn't match it — the new coverage only runs under the full-file invocation (which does pass, 137). Substantively fine; the selective command just doesn't exercise the new tests.

NIT (pre-existing, out of scope) — cli.py:681: session_id and metric_name are interpolated unquoted/unescaped into the FAIL line. A session ID containing whitespace or " would still defeat key=value parsing. Low risk (ADK session IDs are typically UUIDs); worth noting if a JSONL mode (Option 2 in #84) is ever reconsidered.

Positives

• Escape order correct (\\ before " — reversing would double-escape)
• Applied after " ".join(feedback.split()), so newlines/tabs/CRs are already neutralized — no multi-line log-injection vector
• Regression test uses the exact (Design) example from #84 and asserts apostrophe preservation (Jordan Lee's), which correctly stays unescaped inside a double-quoted field
• Single call site; the fallback FAIL line (cli.py:716-717) reuses the same escaped snippet — no double-escaping path
• All three verification claims from the PR description reproduced locally

Review by @caohy1988's assistant (Kimi Code CLI).

[caohy1988]

Requesting changes at exact head 0cf1824 because the new helper does not honor its stated output bound.

src/bigquery_agent_analytics/cli.py:619-625 truncates the raw snippet to max_chars and only then escapes quotes and backslashes. As a result, 120 quote characters or 120 backslashes produce a 240-character result, violating the max_chars contract and allowing unexpectedly wide CI output.

Please construct escaped output within the final output budget (without splitting an escape pair) and add quote-only/backslash-only regression tests asserting len(result) <= max_chars. The existing focused helper tests pass (7 tests), and the failure was independently reproduced on the exact head.

[Alok-Jadhao]

Thanks for the review. I've updated the implementation to address the output-length issue while preserving the existing feedback="..." format.

Changes:

• Escape backslashes and double quotes before emitting the feedback snippet.
• Ensure truncation is applied against the escaped output so the final feedback field remains within max_chars.
• Added regression coverage for embedded quotes/backslashes, including quote-only and backslash-only boundary cases.
• Kept the existing CLI output format and did not introduce the JSONL option.

I'll also run the full tests/test_cli.py suite and git diff --check before the final update.