test(reindex): add whale fixture bounds - #3891
Conversation
Problem: scale-outlier coverage was split between ad hoc builders and an unregistered campaign shape.\n\nWhat changed: add deterministic dimensions, bounded on-disk 804-revision construction, giant attachment bytes, and a multi-million-record streaming fixture. Register the dimensions in the existing reindex campaign manifest and cover the real Codex stream dispatcher.\n\nCompatibility/migration: test-only changes; production schemas and semantics are unchanged.\n\nCo-Authored-By: Claude <noreply@anthropic.com>
Replace the recursive in-memory Codex incident payload with a deterministic on-disk fixture pack covering 804 revisions, a 90 MB terminal snapshot, a 12 MiB inline image, and a two-million-event stream. Preserve bounded image evidence on ordinary Codex messages and calibrate the complete recovery proof's explicit timeout to its current production-route envelope. Ref polylogue-3hdz2.
Align the bounded writer and reusable state record with their declared BinaryIO and streaming iterator contracts. Ref polylogue-3hdz2.
Record the verified generator, parser, incident-scale recovery proof, and static publish gate for polylogue-3hdz2.
|
Warning Review limit reached
Next review available in: 50 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughThe change adds deterministic Codex whale fixtures and manifest generation, routes the live recovery proof through production acquisition, validates fixture-derived evidence, adds streaming and inline-image parser coverage, and centralizes raw-materialization blob limits. ChangesCodex whale fixtures and parser updates
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant ProofTest
participant CodexRevisionChainFixture
participant AcquisitionService
participant SQLiteBackend
ProofTest->>CodexRevisionChainFixture: acquire revision chain
CodexRevisionChainFixture->>AcquisitionService: provide revision source files
AcquisitionService->>SQLiteBackend: persist raw rows
SQLiteBackend-->>ProofTest: return acquired identities and sizes
ProofTest->>CodexRevisionChainFixture: read manifest and source hashes
ProofTest-->>ProofTest: validate recovery and promotion evidence
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 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 `@tests/infra/whale_fixtures.py`:
- Around line 149-154: Update write_revision so revision 0 resets the chain head
by treating any existing source_path as having no predecessor size, rather than
reading its terminal size. Preserve the existing previous-size behavior for
later revisions and ensure rerunning write_codex_whale_fixture_pack in an
existing output directory does not trigger the strict-growth assertion for
revision 0.
- Around line 285-293: Update the `_run` helper to collect each revision’s
authoritative size, check `AcquireResult.errors` after every `acquire_sources`
call, and fail immediately with the acquisition error details when nonzero;
return both the raw IDs and fixture-derived sizes. Update the caller in
`test_codex_804_live_proof.py` to unpack both values and pass the returned sizes
to `fixture.write_manifest` instead of rebuilding sizes from persisted database
state.
In `@tests/unit/scenarios/test_codex_804_live_proof.py`:
- Line 238: Replace the vacuous length comparison in the test around
fixture_manifest with assertions about the manifest’s expected JSON structure
and contents, using the existing manifest schema or keys rather than
REVISION_COUNT. Keep the check focused on validating that fixture_manifest is a
correctly shaped manifest.
In `@tests/unit/sources/test_parsers_codex.py`:
- Around line 1463-1469: Update the assertions for the parsed message in the
test around result.messages[0] so every block’s text excludes the raw
“data:image/png;base64” URL, while retaining the existing message-level digest
and image-inspection checks. Replace the one-block digest assertion only if
needed to add the per-block data-URL omission check.
🪄 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: 55735efa-d837-4122-a7e5-a99ee5eaee71
📒 Files selected for processing (11)
.beads/issues.jsonldevtools/pytest_timeout_overrides.tomlpolylogue/sources/parsers/codex.pytests/infra/generate_whale_fixture.pytests/infra/reindex_campaign.pytests/infra/whale_fixtures.pytests/unit/infra/test_whale_fixtures.pytests/unit/maintenance/test_reindex_campaign.pytests/unit/scenarios/test_codex_804_live_proof.pytests/unit/sources/test_codex_whale_fixtures.pytests/unit/sources/test_parsers_codex.py
| def write_revision(self, source_path: Path, revision: int) -> int: | ||
| if not 0 <= revision < self.dimensions.revision_count: | ||
| raise ValueError(f"revision must be in [0, {self.dimensions.revision_count})") | ||
| source_path.parent.mkdir(parents=True, exist_ok=True) | ||
| temporary_path = source_path.with_name(f".{source_path.name}.revision-{revision:03d}.tmp") | ||
| previous_size = source_path.stat().st_size if source_path.exists() else 0 |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Reset the chain head at revision 0.
Revision 0 does not copy a predecessor, but line 154 still reads the size of an existing source_path. If output_dir already holds a fixture from an earlier run, line 228 compares previous_size (the terminal size) against target_bytes (4,096) and raises revision 0 is not strictly larger. write_codex_whale_fixture_pack does not clear the file first, so re-running tests/infra/generate_whale_fixture.py into the same directory fails with a misleading growth assertion.
Treat revision 0 as the start of a new chain.
🐛 Proposed fix
source_path.parent.mkdir(parents=True, exist_ok=True)
temporary_path = source_path.with_name(f".{source_path.name}.revision-{revision:03d}.tmp")
- previous_size = source_path.stat().st_size if source_path.exists() else 0
+ if revision == 0:
+ source_path.unlink(missing_ok=True)
+ previous_size = source_path.stat().st_size if source_path.exists() else 0📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| def write_revision(self, source_path: Path, revision: int) -> int: | |
| if not 0 <= revision < self.dimensions.revision_count: | |
| raise ValueError(f"revision must be in [0, {self.dimensions.revision_count})") | |
| source_path.parent.mkdir(parents=True, exist_ok=True) | |
| temporary_path = source_path.with_name(f".{source_path.name}.revision-{revision:03d}.tmp") | |
| previous_size = source_path.stat().st_size if source_path.exists() else 0 | |
| def write_revision(self, source_path: Path, revision: int) -> int: | |
| if not 0 <= revision < self.dimensions.revision_count: | |
| raise ValueError(f"revision must be in [0, {self.dimensions.revision_count})") | |
| source_path.parent.mkdir(parents=True, exist_ok=True) | |
| temporary_path = source_path.with_name(f".{source_path.name}.revision-{revision:03d}.tmp") | |
| if revision == 0: | |
| source_path.unlink(missing_ok=True) | |
| previous_size = source_path.stat().st_size if source_path.exists() else 0 |
🤖 Prompt for 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.
In `@tests/infra/whale_fixtures.py` around lines 149 - 154, Update write_revision
so revision 0 resets the chain head by treating any existing source_path as
having no predecessor size, rather than reading its terminal size. Preserve the
existing previous-size behavior for later revisions and ensure rerunning
write_codex_whale_fixture_pack in an existing output directory does not trigger
the strict-growth assertion for revision 0.
![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: cf68f697eb
ℹ️ 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".
| state_record: dict[str, object] = {"record_type": "state"} | ||
| for _ in range(dimensions.stream_event_count): | ||
| yield state_record |
There was a problem hiding this comment.
The two-million-event fixture repeatedly yields the same dictionary, so it cannot expose the production memory failure it claims to guard: codex.parse_stream() delegates to _parse_records(), which executes record_list = list(records), and a real JSONL iterator produces two million distinct decoded dictionaries that all remain resident. This synthetic stream retains only one dictionary plus a list of references, allowing the regression test to pass while a real multi-million-record session can still exhaust memory; generate disposable distinct records and assert a bounded-memory parse (which will require removing the eager list conversion).
AGENTS.md reference: AGENTS.md:L477-L482
Useful? React with 👍 / 👎.
| ordinary_blob_limit_bytes: int = _ORDINARY_BLOB_LIMIT_BYTES | ||
| whale_blob_limit_bytes: int = _WHALE_BLOB_LIMIT_BYTES |
There was a problem hiding this comment.
These threshold fields are independent copies of the daemon's 64 MiB and 8 GiB limits, and the new tests only compare the copied numbers; the incident proof invokes unbounded backfill_historical_revision_evidence() and the rebuild path rather than _maybe_run_raw_materialization_whale_pass(). Consequently, changing either production limit or breaking the ordinary-to-whale candidate routing leaves every new assertion green even though this 804-revision component can no longer converge through the daemon. Exercise whale_pass_candidate/the daemon route with a component above the ordinary envelope and below the resolved whale envelope instead of certifying duplicated constants.
AGENTS.md reference: AGENTS.md:L477-L482
Useful? React with 👍 / 👎.
| input_id=f"{input_ref.input_id}:fixture-manifest-sha256:{fixture_manifest_digest}", | ||
| corpus_id=f"fixture-manifest:sha256:{fixture_manifest_digest}", |
There was a problem hiding this comment.
When fixture record content changes without changing revision sizes, this receipt still emits the same input_id and corpus_id: write_manifest() records only dimensions, sizes, a session id, and feature labels, with no per-revision or terminal content hash. The previous program digest committed to the serialized payloads, whereas this replacement allows materially different corpora to produce indistinguishable successful recovery receipts. Include content hashes in the manifest—or bind the acquired raw/blob hashes directly—before using its digest as the experiment input identity.
Useful? React with 👍 / 👎.
| content_blocks = content_blocks_from_segments(content) | ||
| content_blocks.extend( | ||
| ParsedContentBlock(type=BlockType.TEXT, text=summary) for summary in inline_image_summaries | ||
| ) |
There was a problem hiding this comment.
For Codex's input_image segment shape, content_blocks_from_segments() emits no block, and this extension replaces the image solely with a BlockType.TEXT summary. The archive therefore still reports no image attachment, while MCP projection treats the summary as ordinary prose and returns it even when include_attachments is false. Emit a typed IMAGE block with the MIME/evidence, optionally alongside searchable bounded text, so attachment filters and block-type consumers retain the source semantics.
Useful? React with 👍 / 👎.
| temporary_path = source_path.with_name(f".{source_path.name}.revision-{revision:03d}.tmp") | ||
| previous_size = source_path.stat().st_size if source_path.exists() else 0 |
There was a problem hiding this comment.
When the generator is rerun in the same output directory—or restarted after an interrupted run—revision 0 observes the existing snapshot's size as previous_size. The later strict-growth check then compares the 4,096-byte revision-0 target against that existing partial or 90 MB file and raises, so the new generation CLI cannot safely regenerate its advertised output without manual cleanup. Treat revision 0 as a fresh chain by removing/replacing the old source first or ignoring its prior size.
Useful? React with 👍 / 👎.
| inline_image_summaries = _codex_inline_image_summaries(content) | ||
| if inline_image_summaries: | ||
| text = "\n".join((text, *inline_image_summaries)) if text else "\n".join(inline_image_summaries) |
There was a problem hiding this comment.
When a human-authored Codex message contains an inline image, appending the generated MIME/size/hash summary to ParsedMessage.text causes _write_messages() to include those synthetic metadata tokens in the message's word_count; because the message remains human_authored, session authored_user_word_count and downstream user-word/cost analytics now attribute them to the user. Keep bounded attachment evidence out of authored prose—such as on a typed attachment block—or otherwise exclude it from word-count derivation.
AGENTS.md reference: AGENTS.md:L65-L70
Useful? React with 👍 / 👎.
| image_url = item.get("image_url") | ||
| if not isinstance(image_url, str): | ||
| continue | ||
| summary = _sanitize_codex_data_url(image_url) |
There was a problem hiding this comment.
For every ordinary inline image, this new call reaches _sanitize_codex_data_url(), which evaluates encoded.encode(...) and therefore allocates a second full-size copy of the base64 payload before hashing it. On the giant-image inputs this change is intended to harden, parsing already retains the decoded JSON string, so the extra contiguous allocation can substantially raise peak RSS or trigger an OOM before the bounded summary is produced. Feed bounded ASCII chunks into the digest instead of encoding the entire payload at once.
Useful? React with 👍 / 👎.
Problem: automated review found that fixture reruns could inherit a stale terminal size, acquisition failures were reduced to later count mismatches, and the receipt manifest was rebuilt from persisted rows with a vacuous shape check. The inline-image regression also did not prove every block omitted raw data URLs. What changed: treat revision zero as a fresh chain while preserving atomic replacement, return generated sizes from acquisition with fail-fast error checks, bind and validate the manifest against those generated dimensions, and prove rerun idempotency plus block-level data URL omission. Verification: focused fixture and parser tests passed; the 804-revision production-route proof passed in 492.32 seconds; devtools verify --quick passed all 24 steps before commit. Co-Authored-By: Codex <noreply@openai.com>
Bind fixture receipts to revision content and exercise the production whale admission thresholds. Keep distinct stream records bounded through a private replay spool, preserve inline images as typed blocks, and avoid charging image evidence as authored prose. The incident-scale recovery scenario now proves the 804-row component against persisted hashes and the actual ordinary and whale envelopes. Ref polylogue-3hdz2. Co-Authored-By: Codex <noreply@openai.com>
The prior Circle job started before the exact-head PR scope carrier was published and failed only its structured carrier step. Create a branch-only synchronization event after publishing the carrier for this commit; squash merge leaves no separate empty commit on master. Ref polylogue-3hdz2. Co-Authored-By: Codex <noreply@openai.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: db6cbfd576
ℹ️ 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".
| timestamp = timestamp_pair[1] if timestamp_pair is not None else None | ||
|
|
||
| content_blocks = content_blocks_from_segments(content) | ||
| content_blocks.extend(inline_image_blocks) |
There was a problem hiding this comment.
When a Codex message contains an input_image but no text, this extension creates an image block and then the existing if not text and not has_structured guard discards the entire message because IMAGE is not included in has_structured. Although this revision adds a typed block, image-only user turns therefore remain absent from the archive; treat any nonempty content-block list, including image blocks, as sufficient message evidence.
Useful? React with 👍 / 👎.
| return () | ||
| blocks: list[ParsedContentBlock] = [] | ||
| for item in content: | ||
| if not isinstance(item, dict) or item.get("type") not in {"input_image", "image"}: |
There was a problem hiding this comment.
For a type: "image" segment carrying a data URL in image_url, content_blocks_from_segments() already emits an IMAGE block, and this helper emits a second block for the same segment. Such messages are persisted and rendered with duplicate attachment blocks, inflating block counts and attachment evidence; restrict this additional conversion to the unsupported input_image shape or enrich the existing image block instead.
Useful? React with 👍 / 👎.
| # The parser needs two lookahead-derived indexes before the materializing | ||
| # pass. Persist a private replay spool so a multi-million-record JSONL | ||
| # stream stays bounded by one decoded record instead of list(records). | ||
| with tempfile.TemporaryFile(mode="w+b") as spool: |
There was a problem hiding this comment.
When production ingest supplies a one-shot Codex JSONL iterator, every decoded record is now pickled into tempfile.TemporaryFile() under the default temporary directory before parsing. A large session therefore requires an additional full-stream-sized, potentially larger allocation on /tmp; deployments using a small tmpfs will fail with ENOSPC even when the archive/index volume has ample capacity, so the new memory-bounded path cannot ingest the whale it targets. Place this spill beside the resolved archive/index tier, as the existing revision-census spill does, or avoid retaining a complete disk copy.
Useful? React with 👍 / 👎.
|
@codex review |
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 `@polylogue/sources/parsers/codex.py`:
- Around line 2803-2808: Update the message admission check in the parser around
_codex_inline_image_blocks and content_blocks_from_segments so BlockType.IMAGE
counts as structured content, preserving image-only messages and their bounded
image evidence. Add a regression test covering a message with only an inline
image and verifying it is retained.
🪄 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: 699e2044-5cd7-4fa8-941f-9033b52ec2e6
📒 Files selected for processing (8)
polylogue/daemon/cli.pypolylogue/product/raw_authority.pypolylogue/sources/parsers/codex.pytests/infra/whale_fixtures.pytests/unit/infra/test_whale_fixtures.pytests/unit/scenarios/test_codex_804_live_proof.pytests/unit/sources/test_codex_whale_fixtures.pytests/unit/sources/test_parsers_codex.py
|
Codex Review: Didn't find any major issues. More of your lovely PRs please. 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". |
Count typed image blocks as structured Codex message content so an image-only turn survives parser admission with bounded evidence. Add a regression that proves the message and sanitized image block remain while the raw data URL does not. Ref polylogue-3hdz2. Co-Authored-By: Codex <noreply@openai.com>
|
@codex review |
|
Codex Review: Didn't find any major issues. What shall we delve into next? 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". |
1 participant
Summary
Add a deterministic, private-data-free whale fixture pack for Codex and bind it into the reindex campaign manifest and incident-scale recovery proof.
Problem
The scale-hardening suite lacked one reusable fixture covering the observed 804-revision chain, 90,822,451-byte terminal snapshot, two-million-event stream, and giant inline base64 attachment. The existing 804 proof built recursive payloads in memory, had a stale 420-second timeout for a measured 501-second production route, and ordinary Codex message images were silently dropped even though function-call outputs retained bounded evidence.
Solution
The generator writes revisions and base64 content in bounded chunks, emits a content-bound manifest, and exposes a command-line generation module. The campaign manifest carries dimensions bound to the production ordinary and whale limits. Codex stream parsing now spools distinct decoded records to a private disk replay so lookahead remains bounded in memory. Ordinary inline images persist as typed image blocks with MIME, approximate byte count, and encoded-payload hash without storing data URLs or inflating authored word counts. The 804 proof uses production acquisition and whale admission, source remediation, crash/checkpoint handling, suffix-only resume, inactive candidate construction, promotion, and public/canonical assertions while preserving the timestamp and authority hardening from #3874.
Verification
Residual state
The frozen current-master full-suite failure ledger remains under polylogue-93xe; this PR does not claim a green full suite or any live production reindex receipt.
Ref polylogue-3hdz2.