mirror of
https://github.com/alibaba/open-code-review.git
synced 2026-09-14 19:59:52 +08:00
Kite
41917e250e
* fix(tool): repair serialized code_comment arguments instead of losing the batch
The code_comment schema declares `comments` as an array, but a model
occasionally serializes that array into a string. The nested string then
needs one more level of escaping than the model applies, and the level it
drops is almost always a double quote inside prose — review text quoting a
term with "..." where the JSON string needs \"...\". json.Unmarshal fails
and every comment in the batch is lost.
Measured on 194 benchmark runs (4540 tool calls): all 307 calls that passed
a native array succeeded, and all 13 that passed a string failed — a clean
split, so the string fallback path recovered nothing. Those 13 batches held
37 comments; roughly half eventually resurfaced because the model happened
to re-file them later, and the rest were gone.
The damage is mechanical, so the repair is too. A bare quote inside a JSON
string is either that string's terminator or content that should have been
escaped, and what follows tells them apart: a real terminator is always
followed by ',', '}', ']', ':' or the end of the text. Escaping the rest —
plus bare control characters — recovers 12 of the 13 real batches, 34
comments. The remaining one has C++ braces that happen to mimic a
terminator, and it keeps today's behaviour.
A repaired batch is accepted only when it parses, every entry still carries
non-empty content, and the entry count did not fall below the number of
"content" fields in the original text. The count check is what rules out a
repair that merged two comments by misreading a terminator; without it a
lossy repair would look successful.
When the repair does not hold up, the original parser error is returned
unchanged. Its "invalid character" wording is load-bearing: replaying the
13 failures against the model shows that phrasing makes it regenerate the
batch (13/13), while describing the schema violation instead makes it
resend the same broken string (4/13).
The model sees a plain success, so a comment_args_repaired run warning is
the only record that a violation happened — without it the repair would
absorb an unbounded number of them unobserved.
* fix(tool): close two gaps in the serialized-comments repair
Both found by running ocr review on the previous commit.
Ending a string early re-reads the prose after it as structure, and the
three existing checks could not see that: valid JSON, non-empty content and
an unchanged entry count all hold while a value is silently truncated. A
repaired entry must now carry only fields the schema defines — review prose
almost never spells a real field name, so a stray key is the signature of
exactly that mistake. Verified to reject nothing across the 13 real failure
samples.
A window remains and cannot be closed from local evidence: inside prose,
`"a word", "existing_code":"x"` is byte-for-byte identical to a genuine
terminator followed by the next field. The guard shrinks it to prose citing
this schema's own field names in that exact shape; repairedCommentsAcceptable
now documents that rather than implying the checks are exhaustive.
The dropped escaping level also hits backslashes, not just quotes: prose
citing a regex (\d) or a path (C:\Users) left a backslash opening no valid
sequence, and the batch was lost to the same root cause the repair exists
for. Backslashes that open no legal escape are now escaped, and control
characters cover all of 0x00-0x1F as JSON requires instead of just the three
with short forms. A backslash before b/f/n/r/t/u stays untouched, so `C:\bin`
still decodes to a backspace — JSON cannot tell that from a genuine escape,
and genuine escapes are far more common here.
Real samples still repair 12 of 13 and recover 34 comments, unchanged, so
neither guard cost any recovery.
* fix(tool): withhold applicable output when a repair may have truncated a value
The serialized-comments repair decides at every bare quote whether it ends
the string or belongs to the content, and it never revisits that choice. When
it guesses wrong the value is cut short, the result can still parse, and all
four acceptance checks pass — so the batch is accepted with a value the model
never wrote. The four checks constrain structure, not content, and only
`content` was required to be non-empty.
That is tolerable for prose. It is not tolerable for `suggestion_code`: a
truncated value there is syntactically incomplete code, and both the SARIF
`fixes` array and the GitHub ```suggestion block turn it into a one-click
commit. Nothing downstream could tell such a value apart from a good one,
because the warning carried only a character count.
Detect the truncation, then withhold what a consumer could act on.
The detector is structural, not statistical. A terminator is only ever
misjudged at a quote followed by ',' '}' ']' or ':', and in prose only a
closing quote is followed by punctuation — an opening quote is followed by
the term. Prose quotes a term with a pair, so the closing quote is the
even-numbered one, and a value cut there keeps an odd number of quotes while
a correctly read value keeps whole pairs. Parity therefore identifies the
mistake. A code snippet can carry an odd count legitimately, so it is flagged
with nothing lost; that direction costs a restatable hint, never a comment.
Handling differs per field, by consequence:
- suggestion_code is dropped when *any* field of its entry is suspect. Its
deleted region comes from StartLine/EndLine, which derive from
existing_code, so a truncated anchor is just as dangerous: whole-line
matching means the cut anchor matches nothing, every deterministic
resolver declines, and the LLM re-location then guesses line numbers from
a mutilated excerpt — resolver.go already names this failure "looking
located while pointing at an unrelated line". A fix built on those numbers
deletes the wrong region. Clearing the field closes SARIF and the GitHub
action at once, since both gate on it being non-empty; neither needs to
know this check exists.
- path is deleted so the defaultPath fallback applies. A truncated path is
non-empty, so it would otherwise suppress that fallback and travel into
the artifactLocation URI and the fingerprint naming a file that does not
exist.
- content and existing_code are reported and left intact. Prose cut short
still reads, and existing_code is the anchor every resolver needs.
thinking is deliberately not checked. knownCommentFields accepts it, but that
set answers whether a field is legal while this one answers whether its
truncation needs acting on. A truncated thinking reaches the JSON output only,
with no terminal or viewer rendering, and because the drop is per-entry,
suspecting it would take a usable suggestion_code down with it. A test pins
that decision so it is not "completed" later.
Comments are never the casualty. Declining the repair instead would return
the batch to the original error, and the measured resurfacing rate for a
resent batch is only about half, so withholding a suggestion strictly beats
withholding a finding.
The repair now returns a CommentRepair describing what it did, and the run
warning is rendered from it, so a truncated recovery is finally
distinguishable from a clean one in stderr, JSON and SARIF alike. Without the
field names the count alone made every repair look equally safe.
Verified against the observed failure shape — quotes all present, only the
escaping level dropped — where nothing is flagged and suggestions survive
intact, including values that legitimately contain quotes. make check, make
test under -race and make coverage (91.4%) all pass, with every function in
the repair unit at 100% statement coverage.
* fix(tool): accept only lossless repairs of serialized code_comment args
5e00179 detects when the repair may have cut a value short, then keeps the
batch and withholds what a consumer could act on. This reverses that choice:
a batch carrying any suspect value is now declined outright.
The reasoning in 5e00179 weighed "withholding a suggestion" against
"withholding a finding" and preferred the former, since a resent batch
resurfaces only about half the time. That comparison overstates what the kept
finding is. Its anchor is the cut value, and matchConsecutive compares whole
lines, so the anchor matches nothing: every deterministic resolver declines
and the LLM re-location assigns line numbers from a mutilated excerpt. What
survives is a finding with no suggestion, no deterministic position, and
content the model never wrote. The real choice is between that and roughly
even odds of the batch coming back whole, with its anchor, its suggestion_code
and an exact position.
Declining also leaves the path free of side effects, which is the point. The
batches it turns away are exactly the ones that were lost before this repair
existed, so no behaviour regresses; only the accepted set changes, and every
batch in it carries values the model can be shown to have meant. The cost is
real and accepted: a suspect batch is no longer partially recovered.
Consequences:
- flagSuspectTruncations and its in-place deletes collapse into the
hasSuspectTruncation predicate. CommentRepair drops SuspectFields,
DroppedSuggestions and DroppedPaths, and Message() loses its three
optional clauses. An accepted repair is an intact one, so no lossy
outcome is left for a warning to describe.
- The parity detector's limits are documented where it lives. It catches a
cut at an even-numbered quote and misses an odd-numbered one, which needs
a closing quote with no opening quote before it in the same value. The
check is also batch-wide, so a value with a naturally odd count (a Go rune
literal '"', say) declines a batch the repair handled losslessly. That
forgoes a recovery but matches pre-repair behaviour exactly, so it is
recorded rather than designed around.
- knownCommentFields is pinned against the embedded schema by a test. A
comment field added to tools.json without updating the set would fail the
unknown-field check for every repaired batch using it, switching the
repair off silently.
Comments across the unit are cut to what changes a reader's actions: the
parity derivation, the one-way-error invariant of the scan, why thinking is
excluded, why illegal backslashes are left alone, and the warning against
"improving" the parser's error wording. Argument for the design belongs in
the review, not in the source.
make check, make test (-race) and make coverage (91.3%) pass, and every
function in the repair unit stays at 100% statement coverage.