fix(review): record the shape a dropped file_summaries recovery read - #873
Conversation
summary.file_summaries arrives in several shapes the schema does not accept, and the recovery drops what it cannot read while logging only counts. A production review dropped all four of its entries, so the Changed Files table rendered with no summaries and nothing recorded what had arrived: the session row stores the response after the rewrite, so its file_summaries reads [] there. When the recovery saves nothing, the warning now also carries the node type file_summaries arrived as, and the node type and field names of the first dropped entry, which is what tells an unknown key apart from a non-textual value or a level of nesting nobody expected. Those names are the model's text, so each goes through LogSafe, at most eight are listed, each is cut at forty characters and the rest are counted. No value is logged, and the node stays out of the session row. A conforming array is still returned untouched and in silence, and a recovery that saves some entries keeps the count line it always logged, so one stray entry does not start logging a shape on every review. No key is added to PATH_KEYS or SUMMARY_KEYS: the shape has to be observed before the lists grow.
Dependency Review✅ No vulnerabilities or license issues or OpenSSF Scorecard issues found.Scanned FilesNone |
🤖 ThrillhouseBot PR SummaryWhat this PR doesWhen the walkthrough recovery of summary.file_summaries recovers nothing, the parser's warning now also names the node type file_summaries arrived as and the node type plus field names of the first dropped entry — each name routed through LogSafe.oneLine and bounded to eight names of forty characters with the remainder counted — so a blank Changed Files table can be diagnosed after the fact without logging any model prose. Description vs. ImplementationNo mismatch found between the PR description and the change. Control-Flow Diagram🔀 Show diagramflowchart TD
A["normalizeFileSummaries(root)"] --> B{"nothing recovered from file_summaries?"}
B -- "no" --> C["warn recovered n / dropped m (unchanged count line)"]
B -- "yes" --> D["firstEntry(fileSummaries): element 0, single entry, or first property value"]
D --> E["describeEntry: node type + field names"]
E --> F["first 8 property names via LogSafe.oneLine, each cut at 40 chars, remainder counted"]
F --> G["warn: it arrived as NODETYPE, first entry ... and N more"]
C --> H["summary.set file_summaries to normalized"]
G --> H
Changes Overview
Changed Files
Risk Assessment
Things to double-check1 lower-confidence finding
|
| Check | Type | Status | Detail |
|---|---|---|---|
| test | check-run | ⏳ Pending | - |
Automated review by ThrillhouseBot. Reply with /review to re-run.
![thrillhousebot[bot]](https://avatars.githubusercontent.com/in/3996985?s=60&v=4){kind=small}
There was a problem hiding this comment.
ThrillhouseBot noted 1 lower-confidence item(s) under Things to double-check in the PR summary (not posted as inline threads):
- LOW: Sanitize test embeds a raw U+000A inside a JSON string, which a plain ObjectMapper rejects by default (
src/test/java/dev/thiagogonzaga/thrillhousebot/review/ai/FileSummariesDropDiagnosticsTest.java:131)
The test builds the forged field name as: var forged = "one" + ch(0x0A) + ch(0x202E) + "two"; and then embeds it verbatim into the raw document at: var entry = new StringBuilder("{"" + longName + "": 1, "" + forged + "": 1"); — so the JSON text contains a literal U+000A character inside a quoted string value. RFC 8259 forbids unescaped control characters (U+0000–U+001F) inside JSON strings, and Jackson's default JsonReadFeature.ALLOW_UNQUOTED_CONTROL_CHARS is disabled, so com.fasterxml.jackson.databind.node readTree on such input throws "Illegal unquoted character (#10)". The parser instance under test is plain — private final ReviewResponseParser parser = new ReviewResponseParser(new ObjectMapper()); — so unless ReviewResponseParser.parse internally deserializes with a mapper that enables ALLOW_UNQUOTED_CONTROL_CHARS (its read path is not in the provided material), the raw U+000A is rejected at JSON-syntax level before normalizeFileSummaries' recovery or LogSafe.oneLine ever sees it: the test either errors (if parse propagates) or the captured line is a parse-failure record and the assertions on the truncated/bounded names fail. Note that the author-supplied red and green outputs show this input parsing cleanly, which conflicts with Jackson's documented default behavior; I cannot fully reconcile that from the diff, so treat this as a verification request — CI will show whether the run stays green and only the parse path (not the mapper configuration) settles which behavior is real. Even in the lenient case the intent survives unchanged: the escape-sequence form below produces the identical in-memory field name (onetwo) after Jackson unescapes it, so the sanitizer's newline-collapsing is what is actually exercised rather than the JSON parser's tolerance.
thrillhousebot
Bot
added
bug
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
|
/review |
There was a problem hiding this comment.
No new issues in this revision, but 1 previous finding(s) remain unresolved — fix them, or reply on their review thread with why they are deferred. A finding listed only under "Things to double-check" has no thread: clear it by commenting @thrillhousebot resolved path/to/File.java:42 — <the finding's title> on this PR.
The sanitizing test embeds U+000A directly in the JSON document rather than as an escape, which reads as invalid JSON until one knows that extractJson escapes control characters inside string literals before the document is read. Say so at the line, so the deliberate choice is not mistaken for a test that only happens to pass.
|
@thrillhousebot resolved src/test/java/dev/thiagogonzaga/thrillhousebot/review/ai/FileSummariesDropDiagnosticsTest.java:131 — Sanitize test embeds a raw U+000A inside a JSON string, which a plain ObjectMapper rejects by default Refuted: the mapper never sees that character raw. |
|
The next review will close every previous finding this comment names by its |
There was a problem hiding this comment.
Required CI is now green for 78455f9, so the approval the earlier review held back is posted. The code was not re-reviewed: that review found no issues, and only the CI gate held its approval.
Choosing between the two warning lines inside normalizeFileSummaries put its cognitive complexity at 19, over the 15 the analysis allows. The choice is its own decision and reads as one: which line a recovery earns depends only on whether it saved anything, not on which shape it walked. Behaviour is unchanged.
|
There was a problem hiding this comment.
Required CI is now green for 1e1732b, so the approval the earlier review held back is posted. The code was not re-reviewed: that review found no issues, and only the CI gate held its approval.
1 participant
What type of PR is this?
Description
ReviewResponseParser.normalizeFileSummariesrecoverssummary.file_summariesfrom several shapes the schema does not accept — a map keyed by path, entries keyedfile/description,"path: summary"strings, a single object where the array belongs — and drops what it cannot read. The warning it ends on counted the drops and said nothing about what it had read:That is the whole record of a review whose Changed Files table rendered with no summaries. The session row stores the response after this rewrite, so
summary.file_summariesreads[]there, and by the time anyone looks the shape that arrived is gone. A count cannot tell an unknown key from a non-textual value, from a level of nesting nobody expected, or from a scalar.When the recovery saves nothing, the warning now also carries the node type
file_summariesarrived as, and the node type and field names of the first dropped entry:Names only, never values: a name is all it takes to decide whether
PATH_KEYS/SUMMARY_KEYSshould grow, while a value carries the model's prose about the diff. The model chooses those names, so each one goes throughLogSafe.oneLineat the point it is read, and the line is bounded in both directions — at most eight names, each cut at forty characters, with the rest counted (… and 3 more) — since an entry is free to carry a thousand keys or one key a megabyte long.Three things deliberately do not change:
{path, summary}entries still returns before any of this and logs nothing.PATH_KEYSorSUMMARY_KEYS. The point of the change is to capture the evidence; the production response is not available to read, andfile_path/change_summaryin the test is a stand-in chosen to exercise the code, not an observed shape. Widening the lists on a guess would be the same mistake in a new place.Verified against the code
Every claim in the issue holds.
normalizeFileSummariesdoes drop unreadable entries and log only counts;PATH_KEYSandSUMMARY_KEYSare as the issue lists them; and the response the session row keeps is written after normalization, so the pre-normalization node is not recoverable from it.On recording the node on the session row
Left out, on purpose.
ReviewSession.aiResponseJsonis a singleTEXTcolumn holding the whole serialized response, so preserving the originalfile_summariesmeans either a second column and a migration or an ad-hoc field inside that JSON, and what it would hold is a whole walkthrough's worth of model prose — the largest free-text field in the response after the findings — written on exactly the rounds that already failed. The question it would answer is "what shape arrived", and the node type plus the first entry's field names answer that in one bounded log line that costs nothing on a healthy review. The javadoc onnormalizeFileSummariesstates that trade-off so the next reader does not re-open it. If a real case ever needs the values rather than the shape, that is the moment to pay for the column.Related Issues
Fixes #872
How Has This Been Tested?
New
FileSummariesDropDiagnosticsTestcaptures theLogRecordthe parser emits, so the assertions do not depend on which handler is installed. Five tests: an all-dropped array, the map form / a scalar / a single entry (each of the three nodes the description can report on), the sanitizing and bounding of the names, a partly recovered response, and a conforming one.Red, against the unfixed parser:
The two guard tests — the partly recovered response and the conforming one — passed before the fix and still pass, which is what makes them guards.
Green, with the fix:
Patch coverage on the
src/maindiff againstorigin/main, line and branch, fromtarget/site/jacoco/jacoco.xml:Checklist
Additional Notes
The sanitizing test drives the real forgery characters through the parser: a field name carrying U+000A and a bidi override (U+202E) comes out of the line as
one two, collapsed rather than deleted, so neither can forge a record boundary or reorder what an operator reads.LogSafeInvariantTestderives its untrusted accessors from the response records and so does not reach a name read off aJsonNode; the name is wrapped where it is read, which is where that class's javadoc says the wrap belongs.A
CHANGELOG.mdentry is added under## [Unreleased]→### Fixed.