fix(sources): decode artifact sidecars before admission - #3794
Merged
Conversation
Problem: Validation-off stream parsing and append ingestion could let declared workflow sidecars reach generic session admission. The current full-route guard from the merged raw-admission work did not cover those two remaining chokepoints consistently.\n\nWhat changed: Honor path-declared non-session classification before the worker fast stream plan and before append JSON decoding. Admit those bytes through typed raw-artifact storage, while preserving the existing content-aware helper for ordinary and Codex append deltas. Route tests cover durable sidecar evidence, session-shaped payloads, malformed append bytes, and a genuine session.\n\nCompatibility/migration: No schema, blob namespace, raw replay, or parser changes. Existing full-route admission from the current master is intentionally reused.\n\nRef #3784\nRef #3772\nRef #3790
📝 WalkthroughWalkthroughArtifact classification now combines source paths with decoded JSON and JSONL payloads. Session-shaped workflow journals enter session parsing or repair flows. Malformed non-session journals remain typed evidence. ZIP processing and raw revision replay use the same classification rules. ChangesWorkflow journal ingestion
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant Source
participant IngestWorker
participant JSONLDetector
participant SessionParser
participant EvidenceStore
Source->>IngestWorker: provide workflow journal
IngestWorker->>JSONLDetector: inspect decoded JSONL records
JSONLDetector-->>IngestWorker: session classification or no session evidence
IngestWorker->>SessionParser: parse session-shaped journal
IngestWorker->>EvidenceStore: retain malformed non-session journal
SessionParser-->>IngestWorker: materialized session
Possibly related PRs
Suggested labels: 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 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 |
Problem Path-declared sidecars were admitted before either live route inspected their payload. A recoverable session record at such a path was therefore reported as a successful artifact rather than reaching parsing. What changed Sample validation-off stream sidecars before their terminal path decision and let positive decoded session evidence select the parser. Apply the same order to append admission while retaining path-based artifact retention when JSONL cannot be decoded. Add worker and append route regressions. Co-Authored-By: Codex <noreply@openai.com>
Sinity
changed the title
Problem The full live batch route still let workflow-journal path rules exclude session-shaped JSONL before decoded content reached parsing and revision application. What changed Classify sampled and in-memory JSONL content before terminal sidecar handling. Carry the current decoded session through full revision application so it is not reclassified by the retained-raw path gate. Keep malformed journal bytes as typed raw evidence and prove repeated full ingest is idempotent. Co-Authored-By: Codex <noreply@openai.com>
Problem The normal validation-on worker stream plan classified its decoded sample with the source path attached. A workflow-journal path therefore remained a non-session artifact even when the sample proved a recoverable conversation. What changed Use decoded session evidence first, then retain the path classification as the non-session fallback. Add an advisory worker-route regression with malformed JSONL surrounding valid conversational records. Co-Authored-By: Codex <noreply@openai.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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/live/batch.py`:
- Around line 2153-2160: Update the artifact admission flow around the
artifact_classification check and admit_raw_artifact_blob_ref so payload-less
blob-backed sidecars are sampled from their JSONL content and passed through the
same session-first classification as inline payloads. Preserve session routing
for session-shaped samples regardless of size, and admit the artifact only when
the sample contains no session evidence; add a regression test covering a
workflow journal that crosses _STREAMING_FULL_INGEST_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: 0524e01e-bb6e-49b3-958c-92521fd1ff15
📒 Files selected for processing (6)
polylogue/pipeline/services/ingest_worker.pypolylogue/sources/live/append_ingest.pypolylogue/sources/live/batch.pypolylogue/sources/live/batch_support.pytests/unit/pipeline/test_quarantine_fixtures.pytests/unit/sources/test_live_batch_support.py
Problem Large full-ingest sidecars use blob references, so the path-declared artifact branch admitted them without inspecting their JSONL content. Session routing therefore changed at the streaming threshold. What changed Sample blob-backed JSONL before terminal artifact admission and use positive session evidence to continue through parsing and revision application. Make large-path JSONL planning follow the same precedence. Add an actual over-8 MiB workflow-journal regression with repeated-ingest idempotence. Co-Authored-By: Codex <noreply@openai.com>
Problem: one-shot archive ingest excluded workflow-journal paths before reading JSONL, and its post-ingest inventory could recreate the same path-only artifact after a repaired session was stored. What changed: decoded JSON evidence now precedes path fallback in source parsing, archive artifact admission, emitter classification, and workflow artifact inventory. Malformed content retains typed artifact evidence. Co-Authored-By: Codex <noreply@openai.com>
Sinity
changed the title
Problem: terminal sidecar admission only classified a fixed JSONL prefix, so workflow rows could hide a later recoverable Claude session. What changed: stream artifact-admission evidence through a rolling bounded window in worker, append, live batch, archive parser, and workflow inventory routes. The large full-batch regression has an early over-8 MiB artifact row followed by 31 workflow rows and a recoverable session. Co-Authored-By: Codex <noreply@openai.com>
Initialize the decoded session-evidence result before append JSONL probing so the malformed-input fallback remains typed and mypy-safe. Co-Authored-By: Codex <noreply@openai.com>
Problem ZIP member routing discarded path-declared workflow journals before decoded records could establish session eligibility. A Claude Code journal with more than 32 non-conversational rows before a real session was silently skipped. What changed ZIP processing now streams JSONL member evidence before a terminal path classification, passes positive evidence into grouped parsing, and retains malformed members as typed raw artifacts during one-shot archive ingest. Verification Focused source and archive tests cover delayed session evidence, malformed artifact retention, and repeat ingest. Co-Authored-By: Codex <noreply@openai.com>
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 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/insights/claude_workflow_materializer.py`:
- Around line 264-269: Update the artifact detection flow around
jsonl_session_artifact so .jsonl files pass blob_store.blob_path(...) or an open
file object directly, preserving its bounded streaming window and avoiding
full-blob reads. Move the existing blob_store.read_all call into the
non-JSONL/JSON branch, while retaining current OSError and ValueError handling.
In `@polylogue/pipeline/services/archive_ingest.py`:
- Around line 480-484: Replace the full-buffer payload.read() call in the
archive ingestion flow with protection against the 10 GiB ZIP entry limit:
either enforce a smaller artifact-specific size limit before admission or route
the member through a streaming raw-artifact admission path. Update the code
around open_bounded_zip_entry and admit_raw_artifact_payload so valid oversized
ZIP entries cannot be fully materialized in memory.
In `@polylogue/sources/decoder_zip.py`:
- Around line 198-210: Update polylogue/sources/decoder_zip.py lines 198-210 in
zip_entry_session_artifact to inspect bounded .json members in addition to
JSONL/NDJSON, decode parseable session-shaped content, and return its session
evidence. Update polylogue/sources/import_explain.py lines 631-634 to perform
decoded session-evidence classification before applying terminal artifact-path
exclusion, keeping explanation routing consistent with ZIP ingestion.
In `@polylogue/sources/live/append_ingest.py`:
- Around line 112-122: Update the append ingestion flow around
_sample_jsonl_payload_with_detail and parse_payload so the bounded payloads
sample is used only for classification. Decode or stream the complete
plan.payload into parse_payload, ensuring appends with more than 64 valid
records materialize every record while preserving the existing session_artifact
handling.
In `@polylogue/sources/live/batch.py`:
- Around line 319-324: Update the blob-backed source classification around
jsonl_session_artifact so path-declared JSON artifacts are content-validated
before admission. Recognize .json and every supported JSONL extension, decode
blob content with json_loads, and apply classify_artifact consistently with
source_parsing.py before allowing admit_raw_artifact_blob_ref. Keep returning
False for unsupported extensions or invalid/unclassifiable JSON content.
🪄 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: 458fce3c-0334-4c29-b1d1-8c4586034db0
📒 Files selected for processing (18)
polylogue/archive/raw_payload/decode.pypolylogue/archive/raw_payload/streams.pypolylogue/insights/claude_workflow_materializer.pypolylogue/pipeline/services/archive_ingest.pypolylogue/pipeline/services/ingest_worker.pypolylogue/sources/assembly_chatgpt.pypolylogue/sources/decoder_zip.pypolylogue/sources/emitter.pypolylogue/sources/import_explain.pypolylogue/sources/live/append_ingest.pypolylogue/sources/live/batch.pypolylogue/sources/live/batch_support.pypolylogue/sources/source_acquisition.pypolylogue/sources/source_parsing.pytests/unit/pipeline/test_archive_ingest_shared_raw.pytests/unit/pipeline/test_quarantine_fixtures.pytests/unit/sources/test_decoders.pytests/unit/sources/test_live_batch_support.py
💤 Files with no reviewable changes (1)
- polylogue/sources/source_acquisition.py
Comment on lines
+319
to
+324
| if Path(source_path).suffix.lower() != ".jsonl": | ||
| return False | ||
| try: | ||
| return jsonl_session_artifact(blob_store.blob_path(blob_hash), provider=provider) is not None | ||
| except (OSError, ValueError): | ||
| return False |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Classify blob-backed JSON content before artifact admission.
Line 319 returns False for every non-.jsonl source path. A blob-backed path-declared JSON artifact can then reach admit_raw_artifact_blob_ref at Line 2183 without a decoded JSON content check.
polylogue/sources/source_parsing.py checks JSON content with json_loads and classify_artifact before it accepts a path-only artifact decision. Keep the blob-backed route consistent. Evaluate .json content, and all supported JSONL extensions, before artifact-only admission.
🤖 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 `@polylogue/sources/live/batch.py` around lines 319 - 324, Update the
blob-backed source classification around jsonl_session_artifact so path-declared
JSON artifacts are content-validated before admission. Recognize .json and every
supported JSONL extension, decode blob content with json_loads, and apply
classify_artifact consistently with source_parsing.py before allowing
admit_raw_artifact_blob_ref. Keep returning False for unsupported extensions or
invalid/unclassifiable JSON content.
Problem One-shot archive ingestion read whole ZIP artifact members into memory even though ZIP processing permits multi-gigabyte entries. It also scanned every ordinary JSONL member for delayed session evidence. What changed Path-declared non-session ZIP members stream into content-addressed blobs and are admitted by blob reference. Delayed JSONL evidence scanning now runs only when a non-session path rule would otherwise exclude the member. Verification Focused archive and ZIP tests exercise streamed artifact retention, ordinary JSONL parsing, delayed recovery, malformed evidence, and idempotence. Co-Authored-By: Codex <noreply@openai.com>
Problem\nPath-only artifact classification and bounded append sampling could hide valid delayed sessions. ZIP JSON record arrays and import explanations applied inconsistent evidence ordering, while workflow inventory materialization loaded large JSONL blobs in full.\n\nWhat changed\nUse the full append stream for stream-record providers, decode bounded ZIP JSON members before terminal artifact skips, carry recovered classification through emission and import explanation, and use streaming JSONL evidence for workflow inventory admission. Add real archive, CLI explanation, append, and materializer regressions.\n\nCompatibility/migration\nNo schema or migration changes.\n\nRef #3794\n\nCo-Authored-By: Codex <noreply@openai.com>
Problem: import_explain decoded path-classified ZIP JSON members before applying the production per-entry and archive-wide ZIP budgets. That let a later session-shaped member reach decompression and JSON decoding after aggregate admission should have rejected it. What changed: Reuse ZipEntryValidator.filter_entries for explain admission and expose its existing rejection decisions to the read-only explanation payload without changing cursor failure strings. Keep decoded session evidence after admission for safe workflow JSON, and add a tiny-cap regression that fails if the later member is decoded. Verification: devtools test tests/unit/cli/test_import_explain.py tests/unit/sources/test_decoders.py -k "zip or ZipEntryValidator" devtools verify --quick Ref #3794 Co-Authored-By: Claude <noreply@anthropic.com>
Owner
Author
|
Implemented in 0ddea14. Scope: import_explain now routes ZIP members through the production ZipEntryValidator before path classification, bounded decompression, or JSON decode. The validator callback reports the same ratio, per-entry, and aggregate rejection decisions to explain output without changing existing cursor failure strings. Decoded session evidence remains active after admission for safe workflow JSON. Regression: tests/unit/cli/test_import_explain.py::test_import_explain_zip_aggregate_admission_precedes_path_session_decode uses a tiny monkeypatched aggregate cap and real central-directory metadata. It records a failure if workflows/later.json reaches zip_entry_session_artifact. Reverting the admission ordering makes that test fail, while the test allocates only small JSON members. Verification:
Ref #3794 |
Problem ZIP sidecar, preflight, live inbox, decoder, and recovery paths used separate admission logic or reopened members by filename. That allowed duplicate-name confusion, unbounded JSON inspection, and aggregate bypasses before decompression. What changed Add one shared ZipAdmission and bounded opener that carries the admitted ZipInfo, enforces compression, per-entry, and archive-wide declared limits before reads, and caps real decompressed bytes during streaming. Route ChatGPT JSON and .dat sidecars, decoder extraction, import preflight, live ZIP expansion, archive artifact ingestion, explain, and blob recovery through it. Compatibility/migration No archive schema or production data changes. Existing cursor rejection reporting and public decoder exports remain available. Verification Focused affected routes: 200 passed, 2 deselected. Live ZIP routes: 2 passed. Source laws: 5 passed. devtools verify --quick: all 23 steps passed. Ref #3794 Co-Authored-By: Codex <noreply@openai.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
1 participant
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.
Summary
Centralize ZIP admission and bounded decompression behind
polylogue/archive/zip_admission.py. The admission object validates central-directory metadata before opening a member, carries the exact admittedZipInfointo the opener, preserves duplicate-name identity, enforces per-entry and cumulative uncompressed limits, and bounds real decompressed bytes during streaming.Problem
Four independent routes could bypass the ZIP safety contract. ChatGPT JSON sidecars were reopened by filename and were outside the same admission pass as
.datmembers..datstreaming had separate checks and no shared JSON-plus-asset aggregate accounting. Decoder extraction callers reopened admitted members by name. Import preflight used unbounded JSON inspection. These paths could read or decode bytes before the authoritative limits had been applied.Solution
.datmembers through one validator, with one cumulative budget across both kinds. JSON discovery and.datstreaming pass the admittedZipInfodirectly to the bounded opener. Duplicate sidecar names retain first-admitted identity.decoder_zipand its extraction callers use the shared admission and bounded opener. Archive artifact ingestion, source acquisition, and live inbox expansion pass the validatedZipInfothrough instead of reopening by filename.No schema changes or production archive access were performed. Beads were not used.
Acceptance criteria
ZipFile.open; duplicate names keep the first admitted payload..datsidecar streaming.datarchives stop before opening the member that would exceed the aggregate limit; JSON and.datmembers share the same budget.process_zip, archive artifact ingestion, source acquisition, live extraction, and explain paths pass the admittedZipInfoto the bounded opener.ZipFile.openconfirms zero member opens.Anti-vacuity evidence
The regressions exercise production routes and make the implementation mutation observable:
test_rejects_json_sidecar_before_open_for_size_and_ratio_limitsfails if JSON sidecar admission is moved after opening or if the shared metadata checks are removed.test_many_dat_members_obey_aggregate_limit_before_second_readandtest_json_and_dat_members_share_aggregate_limit_before_dat_readtrack actualZipFile.opencalls and fail if the second member is read.test_duplicate_sidecar_name_does_not_replace_first_admitted_memberandtest_bounded_open_preserves_duplicate_zipinfo_identityfail if a caller reopens by filename.test_preflight_rejects_oversized_json_before_openfails on any preflight member read before admission.ZipFile.openset to fail, covering the recovery caller that the independent review identified.Independent review notes
The adversarial review found and drove fixes for separate JSON and
.datadmission counters, unbounded preflight reads, live batch propagation of a bounded-stream rejection, and a blob-integrity recovery reopen-by-name path. A final review found no remaining targeted ZIP admission bypass.Verification
env VIRTUAL_ENV="$PWD/.venv" PATH="$PWD/.venv/bin:$PATH" devtools test tests/unit/sources/test_assembly_chatgpt.py tests/unit/sources/test_import_preflight.py tests/unit/sources/test_decoders.py tests/unit/cli/test_import_explain.py tests/unit/pipeline/test_archive_ingest_shared_raw.py tests/unit/storage/test_blob_integrity.py tests/unit/sources/test_live_watcher.py -k 'not (test_live_full_ingest_expands_inbox_zip_members or test_live_full_ingest_sniffs_zip_provider_for_non_session_siblings)'->200 passed, 2 deselectednix develop --command devtools test tests/unit/sources/test_live_watcher.py -k 'live_full_ingest_expands_inbox_zip_members or live_full_ingest_sniffs_zip_provider_for_non_session_siblings'->2 passed, 95 deselectednix develop --command devtools test tests/unit/sources/test_source_laws.py -k 'iter_source_raw_data_reads_plain_and_zip_sources_contract or iter_source_raw_data_streams_grouped_zip_entries_into_blob_store or iter_source_raw_data_splits_multi_session_zip_entries_for_non_grouped_providers or iter_source_raw_data_streams_preserved_zip_entries_into_blob_store'->5 passed, 128 deselectedenv VIRTUAL_ENV="$PWD/.venv" PATH="$PWD/.venv/bin:$PATH" devtools verify --quick-> all 23 steps passeddevtools verify --quickat commit19245f29a-> all 23 steps passedRef #3794