🤖 refactor: add bounded raw history row scanning - #4214
Conversation
|
@codex review Generated with |
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. |
🛡️ Codex Security Review · Automatically triggeredSecurity review completed. No security issues were found in this pull request. Reviewed commit: Only the user who started this review can view the report in Codex. ℹ️ About Codex security reviews in GitHubThis is an experimental Codex feature. Security reviews are triggered when:
Once complete, Codex will leave suggestions, or a comment if no findings are found. |
![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: c4fd2b2972
ℹ️ 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".
|
Addressed the beginRow cancellation finding: push now checks the signal before hashing or delivering raw bytes. Both normal and false-returning raw visitors remain uncalled when beginRow aborts, and the scan rejects with the exact supplied reason. The two new cases fail before the fix and pass afterward. Raw47 tests/237 assertions, Node22 cancellation guards, canonical static and independent review pass. Generated with |
|
@codex review Generated with |
🛡️ Codex Security Review · Automatically triggeredSecurity review completed. No security issues were found in this pull request. Reviewed commit: Only the user who started this review can view the report in Codex. ℹ️ About Codex security reviews in GitHubThis is an experimental Codex feature. Security reviews are triggered when:
Once complete, Codex will leave suggestions, or a comment if no findings are found. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: dec7d07f2d
ℹ️ 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".
Addressed 3989243370: the scanner now checks cancellation immediately after both awaited setup operations. Deterministic empty-file tests hold open and stat separately, abort with an object reason, then verify rejection with that exact reason, no row callback, and a closed file handle. Both cases failed before the fix and pass afterward. The complete reader/scalar composition passes 91 tests / 4,712 assertions; raw canonical static checks and independent review pass.Generated with |
|
@codex review Generated with |
🛡️ Codex Security Review · Automatically triggeredSecurity review completed. No security issues were found in this pull request. Reviewed commit: Only the user who started this review can view the report in Codex. ℹ️ About Codex security reviews in GitHubThis is an experimental Codex feature. Security reviews are triggered when:
Once complete, Codex will leave suggestions, or a comment if no findings are found. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4a2dec6ab6
ℹ️ 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".
Fixed the Flake Hash Check failure by applying the exact offline-cache hash computed in run 34602110408, job 103271747811. The completed dependency derivation reported sha256-5V2PO5L0AlCoSLgxjXYDRq/aCzmXYnCRiPss/IDOtYM=. This is the dependency update required by the streaming reader. Local Nix is unavailable, so the fresh-head CI check will verify the result; application code and the 91 passing reader/scalar tests are unchanged.Generated with |
|
@codex review Generated with |
🛡️ Codex Security Review · Automatically triggeredSecurity review completed. No security issues were found in this pull request. Reviewed commit: Only the user who started this review can view the report in Codex. ℹ️ About Codex security reviews in GitHubThis is an experimental Codex feature. Security reviews are triggered when:
Once complete, Codex will leave suggestions, or a comment if no findings are found. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: cdacaafb31
ℹ️ 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".
|
Addressed both current scanner findings:
All nine new adversarial tests failed before the fixes and pass afterward. The complete scanner suite passes 58 tests / 266 assertions; Node 22 passes 16 new adversarial checks plus the previous 17 checks. Canonical static checks and independent review passed. Generated with |
|
@codex review Please review the current head. This remains part of the cancellation phase and will merge only with the completed, approved, green phase. Generated with |
🛡️ Codex Security Review · Automatically triggeredSecurity review completed. No security issues were found in this pull request. Reviewed commit: Only the user who started this review can view the report in Codex. ℹ️ About Codex security reviews in GitHubThis is an experimental Codex feature. Security reviews are triggered when:
Once complete, Codex will leave suggestions, or a comment if no findings are found. |
|
Codex Review: Didn't find any major issues. Swish! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
07752d5 to
1c9725b
Compare
|
@codex review Please review the current head after the complete phase was composed on current main. All known findings have fixes and replies in their owning layers; the eight-member phase stays held until all members are approved and green. Generated with |
|
Codex Review: Didn't find any major issues. You're on a roll. Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
🛡️ Codex Security Review · Automatically triggeredSecurity review completed. No security issues were found in this pull request. Reviewed commit: Only the user who started this review can view the report in Codex. ℹ️ About Codex security reviews in GitHubThis is an experimental Codex feature. Security reviews are triggered when:
Once complete, Codex will leave suggestions, or a comment if no findings are found. |
Signed-off-by: Thomas Kosiewski <tk@coder.com> --- _Generated with `xum` • Model: `unavailable` • Thinking: `unavailable` • Cost: `$unavailable`_ <!-- mux-attribution: model=unavailable thinking=unavailable costs=unavailable --> Change-Id: I8e8213dd9ce3c49524397757d99ebad2af2d6164
1c9725b to
e9a86bd
Compare
|
@codex review Please review updated head Final integrated validation passes on #4191 All eight members remain held until current-head review approval and required CI are complete. Generated with |
|
Codex Review: Didn't find any major issues. Chef's kiss. Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
🛡️ Codex Security Review · Automatically triggeredSecurity review completed. No security issues were found in this pull request. Reviewed commit: Only the user who started this review can view the report in Codex. ℹ️ About Codex security reviews in GitHubThis is an experimental Codex feature. Security reviews are triggered when:
Once complete, Codex will leave suggestions, or a comment if no findings are found. |
1 participant
Adds a raw JSONL reader that streams large strings, keys and numbers without assembling a whole row. This is the inactive parsing prerequisite for the oversized-history witness finding in #4182; the runtime integration in #4182 consumes it through #4221.
The reader reports byte ranges, content digests, JSON/UTF-8 validity and duplicate decoded keys. Consumers receive provisional tokens, optional isolated raw chunks, and awaited row completion. Explicit replacement decoding supports legacy identity accounting while keeping strict raw validity separate. Abort, early exit and visitor errors close the file and parser.
Uses pinned stream-json 3.6.0 with scalar packing disabled. Retained memory depends on the read chunk plus nesting and object-key bookkeeping; this is not an absolute constant-memory guarantee for arbitrarily deep or wide objects. Callers must bound their own retention and revalidate file stamps before treating descriptors as evidence.
Validation: 58 tests / 266 assertions, including 12 MiB scalar fixtures, one-byte reads, malformed UTF-8/JSON, split escapes, duplicate keys, LF/EOF framing, backpressure and cleanup. A bundled Node 22 smoke run passed 17 checks; four additional abort-guard checks passed after fixing cancellation from beginRow; canonical make static-check passed. Deterministic held-open and held-stat tests on an empty file verify exact abort reasons and file-handle closure after awaited setup. Runtime compatibility was checked against the repository's Node 22 Docker runtime. No provider calls.
The Nix offline dependency-cache hash is updated to the value computed by CI for the added streaming parser dependency. A prior Flake Hash Check confirmed the generated value. Local Nix is unavailable.
Risk: the new dependency and streaming-token contract need careful review before activation.
Raw callbacks receive copies bounded by the read chunk, so mutation of the full backing buffer cannot corrupt parsed tokens, digests or later buffered rows. Cancellation remains observable after awaited visitor completion and asynchronous parser/file disposal. Nine adversarial regressions failed before these fixes and now pass; an additional bundled Node 22 run passed 16 mutation and late-cancellation checks. Canonical static checks passed on the final scanner fix.
Final integrated validation passes on #4191
052fc084517c1a6fd8cfbecdf08c05b635358a32(4026 tests / 36442 assertions) and #420902651d365f63c9dc04d95744207f42f8065d6f35(4095 tests / 36847 assertions), across 54 affected suites each. Full source/test TypeScript andmake static-checkpass on both exact commits.The complete cancellation phase is ordered #4214 → #4215 → #4219 → #4221 → #4182 → #4187 → #4191 → #4209. The reader prerequisites and runtime changes merge together only after every member has current-head approval and green CI.
Generated with
xum• Model:unavailable• Thinking:unavailable• Cost:$unavailable