Keep the line breaks in a stored FOS report - #1224
Merged
Merged
Conversation
Widening MAX_TEXT to 8192 bytes bought less than it looked like: _sanitize() stripped every control character including the newline, so 8K of trace arrived as one unbroken line -- barely more readable than the 500 characters it replaced. The flattening was there for a real reason, but not for the row. An embedded newline lets a caller forge a second entry, and that is true of a chat notification and of fosreports.log, whose entries are one timestamped line each and which is read with `tail`. It is not true of a database row. So the guard moved to the two destinations that need it: - _sanitize() keeps \n and strips everything else, after normalising \r\n and lone \r to \n. Without that normalisation the CR is merely replaced by a space, so every line of a DOS-ended report stores with trailing whitespace. The invalid-UTF-8 fallback had to be spelled out as a byte range rather than [[:cntrl:]], because that class includes the newline -- otherwise a machine with the wrong locale silently loses the shape of its report while every other machine keeps it. - _flatten() collapses whitespace to single spaces, and the notification and the log line each call it. The notification is flattened BEFORE its MAX_REASON cut, so the 500 characters are spent on text rather than on indentation. Narrower than the working-1.6 change by fact, not by choice. That branch also had to escape the logs grid's Message column -- DataTables writes cell content with innerHTML and that column had no render, which made an unauthenticated endpoint an HTML sink -- and to replace the modal's .text-wrap, whose `white-space: normal !important` was overriding its <pre>. This branch has no logs tab, no js/fog/task/ and no taskLog viewer at all, so there is nothing here to escape or to wrap. The row is stored faithfully either way, which is what a future viewer would want to find. Verified against a copy of the live database on the 1.6 side, where the code is identical: a CRLF report with terminal escapes and a NUL stores with its 3 line breaks and no CR, NUL or ESC byte, and the same text flattens to 0 line breaks for the notification and the log. Six mutations, all killed. Two survived the first pass, both because no test case carried a newline through the path they guarded. Co-Authored-By: Claude <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Port of #1223.
Widening
MAX_TEXTto 8192 bytes (#1218) bought less than it looked like:_sanitize()stripped every control character including the newline, so 8K of trace arrived as one unbroken line — barely more readable than the 500 characters it replaced.Where the flattening belongs
It was there for a real reason, just not for the row. An embedded newline lets a caller forge a second entry — true of a chat notification, and of
fosreports.log, whose entries are one timestamped line each and which gets read withtail. Not true of a database row.taskLog.logTextReasonMAX_REASONfosreports.logTwo details that are easy to get wrong and are pinned by tests:
\r\nand lone\rare normalised to\nfirst. Without that the CR is merely replaced by a space, so every line of a DOS-ended report — which is most of them — stores with trailing whitespace.[[:cntrl:]]. That class includes the newline, so a machine with the wrong locale would silently lose the shape of its report while every other machine kept it._flatten()is applied before theMAX_REASONcut, so the 500 characters are spent on text rather than on indentation.Narrower than #1223, by fact rather than choice
That branch also had to escape the logs grid's Message column — DataTables writes cell content with
innerHTMLand that column had norender, which made an unauthenticated endpoint an HTML sink — and to replace the modal's.text-wrap, whosewhite-space: normal !importantwas overriding its<pre>.This branch has no logs tab, no
js/fog/task/, and notaskLogviewer at all, so there is nothing here to escape or to wrap, and noFOG_BCACHE_VERbump to make. The row is stored faithfully either way, which is what a future viewer would want to find.Verification
The code is identical to 1.6's, where it was run against a copy of the live database — a CRLF report carrying terminal escapes and a NUL:
Six mutations, all killed. Two survived the first pass, both because no test case carried a newline through the path they guarded.
Full suite: 14 passed, 0 failed.
🤖 Generated with Claude Code
https://claude.ai/code/session_013mJVe4CpK3rRbi9H5GubXd