Repository navigation
fix: keep bounded PG harness failure diagnostics - #1750
Merged
Merged
Conversation
The PostgreSQL harness replaced a failure diagnostic larger than MAX_CAPTURED_OUTPUT_BYTES with one fixed line, and a child stream that overflowed its 64 KiB ring was discarded with a bare error. A large failing suite therefore lost the failing test name and assertion. The final diagnostic is now sanitized in full first, then bounded to a head and tail excerpt with a marker holding the original byte count. Cuts fall on UTF-8 code point boundaries and the result stays within the limit. Child output is redacted before it enters the ring, never afterwards: a truncated raw tail may begin inside a secret or PEM block, and redaction state cannot be recovered from it. createStreamRedactor redacts per complete line (exact secrets, postgresql URLs, PEM BEGIN through END, including blocks still open at end of stream); the trailing partial line is held in a bounded buffer and an overlong line is dropped with a marker. When a redacted stream still overflowed, the run still fails, but the thrown error carries the bounded sanitized stdout and stderr tails in stderr, because harnessErrorDetails reads stderr only. The same applies to the expanded sanitized output branch. Container logs are redacted the same way. Raw truncated output (a secret containing a newline disables the redactor) is still omitted, fail closed. Previously discarded tail content now surfaces after redaction. Scripts only, so no changeset. Signed-off-by: Bernardo Donadio <bcdonadio@bcdonadio.com>
Round 1 review of the harness diagnostics found secret leaks and an acceptance gap in the line-wise redactor. PEM state now comes from one ordered scanner that walks complete BEGIN and END markers left to right, seeded with the current block depth, for normal lines and for dropped text alike. A block opened right after an END, a BEGIN nested before the first END, an END prefix that is not a complete marker, and an END on an overlong line inside an open block are all handled, and state is committed in both directions. Dropped text keeps a 79 character carry so a marker split across chunks is still recognised; labels over 64 characters are not markers. The line bound now applies to every complete line, and a dropped line counts as capture truncation, so an exit-0 child whose output was discarded rejects like a ring overflow regardless of chunk boundaries. The ring start also advances to a UTF-8 code point boundary. A BEGIN with no END stays open to the end of the stream, which fails closed. Signed-off-by: Bernardo Donadio <bcdonadio@bcdonadio.com>
Signed-off-by: Bernardo Donadio <bcdonadio@bcdonadio.com>
Round 2 review found that the stream redactor accepted PEM labels of at most 64 characters, while sanitizeHarnessText accepts any non-empty hyphen-free label. A block with a longer label therefore surfaced its body once the ring had dropped the BEGIN line, where the full-text pass could no longer repair it. Complete lines now use the sanitizer's marker grammar. Dropped text is scanned by a streaming marker scanner that keeps only a keyword-prefix carry and the marker state, so a label of any length can span writes. A BEGIN keyword opens a block at once, confirmed or not, and only a confirmed END marker closes one; a hyphen inside a label never completes a marker. The UTF-8 ring test now overflows the ring with several lines under the line cap, so it exercises the code point cut. The old long-label test that asserted ordinary text is replaced by public-path tests. Signed-off-by: Bernardo Donadio <bcdonadio@bcdonadio.com>
bcdonadio
enabled auto-merge
October 25, 2026 05:43
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: dfbedd210c
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
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.
Why
When a PostgreSQL harness run failed with more than 64 KiB of output (
MAX_CAPTURED_OUTPUT_BYTES), the harness threw away the whole diagnostic and printed onlyPostgreSQL harness diagnostic exceeded the safe capture limit. This happened at two points. The 64 KiB ring of the vitest child overflowed, and the bounded raw tail was discarded because it could not be sanitized safely. The final diagnostic was all-or-nothing. A large failing suite such as surface parity therefore never showed the failing test or assertion. This hid the causes of #1599, #1619, #1647 and #1718.Closes #1622
How
All changes are in
scripts/postgresql-harness.mjs:sanitizedHarnessErrorDetailssanitizes the full text first. If the result is over the limit, it keeps a marker with the byte count, a head, an elision line and a tail, all within the limit and cut on UTF-8 code point boundaries.postgresql://URLs and PEM blocks, using the same marker grammar assanitizeHarnessText.error.stderr, which the final diagnostic reads. It is never returned as successful output.Previously discarded tail content now surfaces after redaction.
Testing
test/scripts/postgresql-harness.test.ts: 167/167. The new tests go throughrunProcess/runSanitizedProcesswith fake children. They cover:tsc --noEmitproduced no output.main.Reviewer guide
Start with
createStreamRedactor(scanPemMarkersfor complete lines,scanDroppedfor overlong text), thenrunProcess's truncation flags,capturedOutputFailure, andboundedDiagnosticExcerpt. The tests are grouped near the end of the harness test file.