fix(codex): bound oversized rollout inspection - #1115
Conversation
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughThe rollout classifier now performs bounded descriptor-based inspection, detects file changes during reads, validates first and latest metadata records, and reports indeterminate results for inspection failures. Tests cover invalid latest metadata and oversized rollouts. ChangesRollout inspection
Estimated code review effort: 3 (Moderate) | ~20 minutes Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
✅ Deterministic PR hygiene checks passed. |
⏳ DRAFT
What to do
Review readiness checklist
3/4 boxes ticked. This PR stays in draft until every box above is ticked. |
|
@coderabbitai review |
✅ Action performedReview finished.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/codex/native-residue.ts`:
- Around line 412-420: Update the validation loop in the residue-detection
function so both first and latest session_meta records are fully validated
before deciding the result; track whether either validated model_provider is
"opencodex", then return residue only after the loop, otherwise preserve the
existing indeterminate behavior. Add a focused regression test near the existing
native-residue tests covering a valid OpenCodex first record with invalid latest
metadata and asserting indeterminate.
- Around line 375-389: Update the rollout inspection flow around the content
read and JSONL loop to open and stat a single file descriptor, then read at most
the captured size using fixed-size chunks rather than readFileSync. Parse each
JSONL record incrementally without creating an array for all lines, restat the
same descriptor after reading, and return indeterminate for short reads or any
metadata change; preserve the existing unreadable-rollout handling.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 87be70f4-acb9-4ade-90c3-5962166b3b29
📒 Files selected for processing (2)
src/codex/native-residue.tstests/codex-native-residue.test.ts
|
@coderabbitai review |
✅ Action performedReview finished.
|
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/codex/native-residue.ts`:
- Around line 385-408: Replace the full-buffer conversion and subsequent
line-array parsing in the rollout inspection flow with incremental parsing of
bounded read chunks. Track only the current partial JSONL record plus the first
and latest session_meta payloads, processing each complete line as it arrives
and preserving behavior across chunk boundaries; remove the content.split("\n")
approach and avoid retaining all parsed lines.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 13e9739a-f219-40ef-b3a6-b637076019f5
📒 Files selected for processing (2)
src/codex/native-residue.tstests/codex-native-residue.test.ts
|
@coderabbitai review |
✅ Action performedReview finished.
|
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/codex/native-residue.ts`:
- Around line 419-427: Update the descriptor-read loop around readSync to
enforce opened.size as the hard read bound: stop only when totalRead reaches
opened.size, request no more than the remaining observed bytes, and return
indeterminate if readSync returns zero before reaching that size. After the
bounded read, compare the post-read descriptor stat against opened before
continuing, preserving the existing inspection-limit handling.
- Around line 427-453: Update the read-and-parse flow around the chunk decoding
in the native residue implementation to use a single streaming TextDecoder,
decoding each buffer with stream mode and flushing it after the read loop before
processing the remaining partial text. Preserve the existing
rolloutSessionMetaPayload handling and first/latest assignments, and add a
focused regression test that splits a non-ASCII session ID across
ROLLOUT_READ_CHUNK_BYTES.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: d5cf7bc6-7260-4863-a51e-eb16398863bf
📒 Files selected for processing (2)
src/codex/native-residue.tstests/codex-native-residue.test.ts
f509e68 to
926621d
Compare
![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: 926621db5e
ℹ️ 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".
|
Thank you — this is landing as #1136 in the stacked campaign. All five of your commits were cherry-picked with your authorship intact, and I kept them as a chain rather than squashing so the progression (bound the read → decode in stream → parse incrementally → re-stat the pathname → scan once and preserve BOM) stays legible in review. The property I checked hardest before adopting: the bound governs how much is read, not what counts as residue. A bounded reader that quietly reclassified a truncated rollout as clean would hide exactly the state this code exists to find — your malformed/truncated cases show it does not. Thanks also for splitting the unrelated native-history bound out into its own PR when asked; that is why this one was straightforward to land. Verified on the stack head: Leaving this PR open for you to close or continue. |
|
Closing this PR as agreed: the five commits were adopted upstream into #1136 with authorship intact, verified on the stack head (65 pass / 0 fail, typecheck clean, privacy scan green). Thanks for the careful review of the bounded-read property — glad the malformed/truncated cases confirmed a truncated rollout is never reclassified as clean. |
2 participants
Summary
This pull request is intentionally separate from the GitHub Copilot Responses normalization work in #1111, following maintainer review. It targets the native startup/history safety gap only.
Verification
bun test ./tests/codex-native-residue.test.ts— pass.bun run typecheck— pass.bun run privacy:scan— pass.git diff --check— pass.Checklist
Review readiness checklist
This PR stays in draft until every box below is ticked. Tick all four boxes once the requirements are met:
All CI tests are green on my local testing.
I pushed my PR to the latest dev commit.
I resolved all correct Codex and CodeRabbit findings.
My PR is ready for review.
Summary by CodeRabbit