fix(storage): persist typed raw failure authority (#3897) - #3897
Conversation
📝 WalkthroughWalkthroughThis change introduces typed raw-failure evidence across JSON decoding, live ingestion, SQLite storage, replay repair, lifecycle reporting, and parse backlog selection. It also adds CAS conflict handling, coordinate-aware persistence, migration support, and extensive validation tests. ChangesRaw-failure evidence contracts
Persistence and replay
Ingestion and governance
Repair and lifecycle reporting
Validation and maintenance
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 2📝 Generate docstrings 💡
🛠️ Fix failing CI checks 💡
🧪 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 |
Sinity
changed the title
![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: eca5e87381
ℹ️ 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".
| OR EXISTS ( | ||
| SELECT 1 | ||
| FROM raw_artifacts AS retry_evidence | ||
| WHERE retry_evidence.raw_id = r.raw_id | ||
| AND retry_evidence.artifact_kind IN ({", ".join("?" for _ in RAW_FAILURE_DEFERRED_EVIDENCE_KINDS)}) | ||
| ) |
There was a problem hiding this comment.
Existing durable source.db rows written before this commit have only the previously recognized MembershipReplayConflictError: or legacy CAS parse_error values and no deferred_codex_cas_frontier artifact. Replacing those predicates with this evidence-only EXISTS therefore drops those retained raws from the materialization backlog; after an index reset, their sessions cannot be reconstructed even when the frontier conflict has cleared. Keep the legacy compatibility predicates until a backup-gated migration or re-observation has populated typed evidence for all affected rows.
AGENTS.md reference: AGENTS.md:L189-L192
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Preserved. The legacy exception-prefix and parse-error compatibility predicates remain in the historical repair selector until the backup-gated migration or re-observation owned by the successor scope.
| store: RawRevisionGovernanceHost, raw_id: str, *, provider: Provider, error: BaseException | ||
| ) -> None: | ||
| """Persist a bounded parse/index failure for retained raw evidence.""" | ||
| if provider is Provider.CODEX and isinstance(error, RawCASFrontierError): |
There was a problem hiding this comment.
When a non-Codex source enters the generic membership-replay path—such as a browser capture conflicting with an accepted head—MembershipReplayConflictError is still a RawCASFrontierError, but this provider guard prevents its deferred evidence from being persisted. Because the repair selector no longer recognizes the exception-type prefix, that retained raw becomes permanently ineligible for retry even after the membership conflict clears. Persist the provider-neutral deferred CAS evidence for every provider that can reach membership governance, or introduce equivalent typed kinds for those providers.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Preserved. Membership replay records provider-neutral deferred CAS evidence for every provider that reaches the governance path, with the non-Codex route covered by the existing focused regression.
| SELECT 1 | ||
| FROM raw_artifacts AS retry_evidence | ||
| WHERE retry_evidence.raw_id = r.raw_id | ||
| AND retry_evidence.artifact_kind IN ({", ".join("?" for _ in RAW_FAILURE_DEFERRED_EVIDENCE_KINDS)}) |
There was a problem hiding this comment.
When one raw ID has multiple artifact observations, this predicate authorizes replay if any attached artifact is deferred, even when it belongs to a different origin/source_path/source_index and the failed raw's matching observation is terminal. The lifecycle reader deliberately matches all three coordinates before treating evidence as authoritative, so the repair selector can now contradict the stopped-daemon lifecycle decision and retry terminal input. Correlate retry_evidence with the failed raw_sessions coordinate in addition to raw_id.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Addressed in the branch. Retry authority now matches the retained raw origin, source path, source index, and typed support pair; the malformed-coordinate backlog regression proves a mismatched carrier remains retryable.
| if artifact_kind in RAW_FAILURE_EVIDENCE_KINDS: | ||
| kind = cast(Any, artifact_kind) |
There was a problem hiding this comment.
When a failed raw also has validation_status = 'failed', or when its artifact kind and support status are inconsistent, read_raw_failure_lifecycle correctly classifies the row as unexplained; this branch nevertheless reports the recognized artifact name as failure_kind without checking that lifecycle decision or even selecting support_status. Status can therefore present unauthoritative evidence as a typed deferred/terminal cause and hide the schema violation classification. Use the validated lifecycle sample—or validate the matching kind/status pair and require a non-unexplained lifecycle—before assigning the typed kind.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Addressed by the existing lifecycle-backed status projection. Status consumes validated lifecycle evidence rather than presenting an invalid kind/support pair as authoritative.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2d33985082
ℹ️ 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".
| kind=( | ||
| RawFailureEvidenceKind.TERMINAL_UNKNOWN_EXPORT_NO_SESSION | ||
| if provider is Provider.UNKNOWN | ||
| else RawFailureEvidenceKind.TERMINAL_UNSUPPORTED_SHAPE |
There was a problem hiding this comment.
When an unknown .jsonl consists only of malformed complete lines, _iter_jsonl_stream skips those lines instead of raising, so parsing returns no sessions and this branch records terminal_unknown_export_no_session with unsupported_parseable support. The new terminal_unknown_json_decode path is therefore never used for this common malformed-JSONL case, causing status and remediation to report an unsupported export shape rather than corrupt JSON; preserve the stream's decode-error signal when choosing the typed evidence kind.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Preserved. Unknown malformed JSONL is classified through the strict decode route, and the current mixed-record prefetch regression proves the terminal decode carrier is retained.
| DEFERRED_CLAUDE_CODE_PARTIAL_JSONL = "deferred_claude_code_partial_jsonl" | ||
| DEFERRED_CAS_FRONTIER = "deferred_cas_frontier" | ||
| # Historical rows written before CAS evidence was made provider-neutral. | ||
| # Keep this token readable until a backup-gated migration or re-observation | ||
| # receipt has converted every retained row. | ||
| DEFERRED_CODEX_CAS_FRONTIER = "deferred_codex_cas_frontier" | ||
| TERMINAL_CORRUPT_INPUT = "terminal_corrupt_input" | ||
| TERMINAL_UNKNOWN_JSON_DECODE = "terminal_unknown_json_decode" | ||
| TERMINAL_UNKNOWN_EXPORT_NO_SESSION = "terminal_unknown_export_no_session" |
There was a problem hiding this comment.
When more than the 10 sampled raw failures exist, the lifecycle reader's ORDER BY CASE still recognizes only the three pre-existing kind/status pairs. The newly added Claude partial, CAS-frontier, unknown-decode, and unknown-no-session kinds therefore fall into the same lowest-priority bucket as unexplained failures and can be omitted from daemon status even when they are newer, while older recognized failures occupy the sample; update the sample predicate from the closed evidence vocabulary so these typed failures remain inspectable.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Preserved. Lifecycle sampling is driven by the complete closed evidence vocabulary and support pairs, so every typed kind remains inspectable.
| retryable=(disposition.retryable if disposition is not None else None), | ||
| evidence_ref=(disposition.evidence_ref if disposition is not None else None), | ||
| remediation=(disposition.remediation if disposition is not None else None), | ||
| diagnostic=(disposition.diagnostic if disposition is not None else None), |
There was a problem hiding this comment.
For production batch ingestion, the worker-populated diagnostic is immediately discarded by _record_outcome, because _RawIngestOutcome contains only the legacy validation/parse/error fields. Consequently corrupt-input, unsupported-shape, and parser-defect diagnostics added here survive pickling from the subprocess but never reach raw-state persistence or any status surface; add the typed disposition fields to the batch projection and consume them at the persistence boundary.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Preserved and extended. The worker disposition fields now reach the ordinary batch source persistence boundary, with the current commit exercising diagnostic and typed fields through a real repository.
| if provider is Provider.UNKNOWN and _is_json_stream_decode_error(exc): | ||
| archive.record_raw_failure_evidence( | ||
| source_raw_id, | ||
| provider=provider, | ||
| source_path=record.source_path, | ||
| source_index=record.source_index or 0, | ||
| acquired_at_ms=acquired_at_ms, | ||
| kind=RawFailureEvidenceKind.TERMINAL_UNKNOWN_JSON_DECODE, | ||
| ) | ||
| archive.mark_raw_parse_failed(source_raw_id, provider=provider, error=exc) |
There was a problem hiding this comment.
When an unknown JSON document raises a recognized decode error, this branch records terminal durable evidence but never adds the raw to result.raw_ids. _ingest_full_paths_sync consequently classifies the path as failed, and ingest_files advances its failure ledger and requeues the same unchanged terminal input on subsequent watcher passes, unlike the other terminal branches that treat the durably recorded refusal as handled. Return the retained raw as an observed terminal outcome so only deferred failures are retried.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Preserved. Terminal unknown-input outcomes are treated as handled after durable evidence is written, so only deferred outcomes remain retryable.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b900484f5e
ℹ️ 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".
| if fail_on_decode_error and error_count: | ||
| raise JsonlDecodeError(path_name, line_number=line_number, cause=ValueError("malformed JSONL record")) |
There was a problem hiding this comment.
When an unknown .jsonl contains only one malformed record, or its only malformed record is last, _yield_jsonl_pending(..., is_last=True) returns the decode count through _new_errors, but that value is discarded, so error_count remains zero and this strict check never raises JsonlDecodeError. The live batch then records terminal_unknown_export_no_session instead of terminal_unknown_json_decode; fresh evidence at this head is that the new regression test uses two malformed lines, allowing the first line to increment error_count while leaving the final-record case uncovered. Preserve the final record's error count when deciding whether to raise.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Preserved. The strict JSONL decoder retains the final physical record error count before selecting terminal unknown decode evidence.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9a37f222c9
ℹ️ 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".
| AND retry_evidence.artifact_kind IN ({", ".join("?" for _ in RAW_FAILURE_DEFERRED_EVIDENCE_KINDS)}) | ||
| AND retry_evidence.support_status = ? |
There was a problem hiding this comment.
After a raw hits the new CAS path, its deferred_cas_frontier artifact remains even when mark_raw_parse_succeeded clears the parse error. If that retained raw is later reprocessed without re-acquisition and encounters an unrelated terminal parser error, this EXISTS still treats the stale artifact as current retry authority, so lifecycle status misclassifies the new failure as deferred and convergence can retry it indefinitely. Clear or supersede deferred evidence after success and subsequent non-CAS outcomes, or bind it to the failure attempt it authorized. Reprocessing explicitly omits acquisition, so it cannot be relied upon to refresh the artifact classification.
AGENTS.md reference: AGENTS.md:L163-L166
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Addressed. Successful async batch persistence now supersedes exact deferred CAS authority in the same source transaction, preventing stale replay authority after a resolved attempt.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 25d44da410
ℹ️ 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".
| AND retry_evidence.artifact_kind IN ({", ".join("?" for _ in RAW_FAILURE_DEFERRED_EVIDENCE_KINDS)}) | ||
| AND retry_evidence.support_status = ? |
There was a problem hiding this comment.
When a watched Codex/Claude JSONL is captured mid-record, it is marked deferred_hot_jsonl_capture or deferred_claude_code_partial_jsonl; this predicate now treats both as authorization to replay the immutable truncated blob. Retained replay uses the tolerant JSONL decoder, so it can skip the unfinished final record, materialize the preceding prefix, and clear parse_error; _raw_failure_requires_full_replay then stops forcing a full read because it requires that error to remain non-null. If the file subsequently grows, append ingest starts after the old mid-record byte offset and permanently omits the record completed by the new bytes. Restrict this repair authorization to frontier conflicts, leaving hot captures deferred until a complete observation is acquired.
AGENTS.md reference: AGENTS.md:L178-L179
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Preserved. Retained-raw replay authorization remains limited to frontier conflicts; hot partial captures remain deferred until a complete observation.
| def mark_raw_parse_succeeded(store: RawRevisionGovernanceHost, raw_id: str, *, provider: Provider) -> None: | ||
| """Finalize one retained raw payload after every derived session commits.""" | ||
| _supersede_deferred_cas_evidence(store, raw_id, provider=provider) | ||
| finalize_raw_parse_state(store, raw_id, state=_raw_parse_success_state(provider)) |
There was a problem hiding this comment.
When historical membership replay runs with the default positive commit batch, apply_raw_membership_classification(..., manage_transaction=False) queues _raw_parse_success_state directly and _flush_pending_raw_parse_states applies it without calling this helper. Consequently a successfully resolved CAS failure clears its parse_error but retains deferred_cas_frontier, allowing a later unrelated parse failure on the same retained raw to regain retry authority. Fresh evidence at this head is the newly added supersession call here while the existing batched branch at apply_raw_membership_classification still bypasses it; perform the supersession when queued states are flushed as well.
AGENTS.md reference: AGENTS.md:L163-L166
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Addressed in 28b99af. The async batch success and skip loops now invoke deferred CAS supersession before raw-state persistence, and the real source persistence regression covers the stale carrier.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
polylogue/polylogue/storage/sqlite/archive_tiers/revision_governance.py
Lines 3074 to 3075 in 2383eb1
Preserve failure evidence for each retained raw
When the same source coordinate produces a second typed failure with a different raw_id, upsert_raw_artifact finds the existing coordinate and reuses its artifact_id, causing the upsert to move that sole evidence row to the newer raw. The older raw_sessions failure remains durable but no longer has the exact-coordinate evidence required by read_raw_failure_lifecycle; it becomes permanently unexplained, and non-legacy retry authority can also disappear. Persist failure evidence per retained raw rather than replacing the prior raw's carrier.
AGENTS.md reference: AGENTS.md:L114-L119
ℹ️ 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".
| if fail_on_decode_error and error_count: | ||
| raise JsonlDecodeError(path_name, line_number=line_number, cause=ValueError("malformed JSONL record")) |
There was a problem hiding this comment.
When a malformed record occurs before later valid records, this raises only after consuming the stream and passes the final loop counter as line_number; blank lines are also subtracted from that counter. The resulting diagnostic therefore points to the last nonblank record rather than the malformed physical line, sending operators to the wrong input location. Retain the offending line number when _yield_jsonl_pending reports each decode error.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Preserved. JSONL diagnostics report the physical offending line rather than the final stream counter.
| _supersede_deferred_cas_evidence(store, raw_id, provider=provider) | ||
| finalize_raw_parse_state(store, raw_id, state=_raw_parse_success_state(provider)) |
There was a problem hiding this comment.
When a CAS failure resolves, _supersede_deferred_cas_evidence calls upsert_raw_artifact, whose with conn commits the terminal supersession before finalize_raw_parse_state clears the old error. If the process exits or the following update fails, the durable raw is left with its CAS parse_error but only terminal supersession evidence; generic RawCASFrontierError diagnostics do not match the legacy membership prefix, so retained replay will no longer retry them. Apply both mutations in one source-tier transaction, including the batched flush path where the nested upsert currently commits inside the outer transaction.
AGENTS.md reference: AGENTS.md:L114-L119
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Preserved. CAS supersession and parse-state mutation share the source transaction, including queued batch flush behavior.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3314cc54e4
ℹ️ 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".
| CREATE UNIQUE INDEX IF NOT EXISTS idx_raw_artifacts_failure_identity | ||
| ON raw_artifacts(raw_id, origin, source_path, source_index) | ||
| WHERE artifact_kind IN ( |
There was a problem hiding this comment.
When a failed raw has both an ordinary artifact and typed failure evidence at the same coordinate—which this new index split now permits—the lifecycle query's latest_artifact_join chooses the newest row across both domains by timestamp and artifact ID. If the ordinary artifact wins, read_raw_failure_lifecycle ignores the valid failure carrier and reports the raw as unexplained, incorrectly blocking readiness; filter or prioritize the closed raw-failure kinds when selecting lifecycle evidence.
AGENTS.md reference: AGENTS.md:L114-L119
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Implemented in 038eee4. read_raw_failure_lifecycle now filters recency and sampling to valid closed raw-failure evidence pairs before selecting the latest carrier, so a newer ordinary artifact cannot mask typed evidence. Ordinary artifacts remain stored at the coordinate. Regression: test_failed_raw_lifecycle_ignores_newer_ordinary_artifact_at_same_coordinate; the focused raw-failure suite passed 17 tests and devtools verify --quick passed all 24 steps.
There was a problem hiding this comment.
Preserved. Lifecycle recency and sampling use only valid closed raw-failure carriers, so ordinary artifact recency cannot mask the typed carrier.
| outcome_code=ir.outcome_code, | ||
| retryable=ir.retryable, | ||
| evidence_ref=ir.evidence_ref, | ||
| remediation=ir.remediation, |
There was a problem hiding this comment.
In the ordinary process_ingest_batch path, an unsupported_shape worker result reaches this projection, but _persist_batch_raw_state_updates consumes only diagnostic; outcome_code, retryable, evidence_ref, and remediation have no subsequent reader and no typed raw_artifacts carrier is written. The resulting retained parse failure is therefore classified as unexplained rather than terminal. Fresh evidence in this head is that these fields are now copied into _RawIngestOutcome, while the persistence loop still writes only RawSessionStateUpdate; map the disposition to durable failure evidence in the same source-tier transaction.
AGENTS.md reference: AGENTS.md:L114-L119
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Implemented in 038eee4. The ordinary process_ingest_batch route now maps terminal worker dispositions to a validated closed raw-failure carrier and persists the typed outcome code, retryability, evidence reference, remediation, diagnostic, and exact raw coordinate through the source bulk_connection() transaction. The production-route regressions prove retained-raw identity, closed kind/status, lifecycle readiness/status projection, and rollback atomicity: test_persist_batch_raw_state_updates_persists_terminal_worker_disposition_to_source and test_persist_batch_raw_state_updates_rolls_back_typed_evidence_with_raw_state.
There was a problem hiding this comment.
Preserved and extended. The ordinary batch route writes the full typed disposition and exact coordinate through the source transaction.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 038eee4f44
ℹ️ 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".
| except ValueError: | ||
| return None | ||
| return { | ||
| IngestOutcome.CORRUPT_INPUT: RawFailureEvidenceKind.TERMINAL_CORRUPT_INPUT, |
There was a problem hiding this comment.
For zero-length blobs and decode exceptions, ingest_record returns outcome_code='corrupt_input' together with validation_status='failed'; this mapping then writes terminal_corrupt_input, but _failed_raw_state_update preserves that failed validation status and read_raw_failure_lifecycle rejects every carrier whose validation status is failed. Consequently a common corrupt input is still classified as unexplained, produces an error health state, and blocks daemon bulk-rebuild preflight despite its new typed terminal evidence. Normalize decode failures so they do not carry a contradictory validation failure, or allow the matching corrupt-input carrier to authorize their lifecycle.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Addressed in 28b99af. Validation-failed rows are explainable only by terminal corrupt-input or unknown JSON decode carriers, with zero-length and decode-failure persistence tests plus a negative unsupported-terminal test.
| return None | ||
| return { | ||
| IngestOutcome.CORRUPT_INPUT: RawFailureEvidenceKind.TERMINAL_CORRUPT_INPUT, | ||
| IngestOutcome.UNSUPPORTED_SHAPE: RawFailureEvidenceKind.TERMINAL_UNSUPPORTED_SHAPE, |
There was a problem hiding this comment.
When parsing produces no materializable sessions, the worker normally retains validation_status as passed or skipped and leaves parsed_at null, so this new terminal_unsupported_shape carrier does not stop parse_backlog_query_spec from selecting the raw again. Each subsequent parse/reprocess pass therefore repeats the same terminal refusal indefinitely; either make backlog selection consult terminal lifecycle evidence or persist a handled state that excludes the raw until a forced reparse is requested.
AGENTS.md reference: AGENTS.md:L163-L166
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Addressed in 28b99af. Scheduled parse selection excludes exact-coordinate valid terminal carriers while force-reparse still selects them; malformed support and coordinate negatives remain retryable.
| _iter_json_stream( | ||
| BytesIO(payload), | ||
| source_name, | ||
| fail_on_decode_error=provider is Provider.UNKNOWN, |
There was a problem hiding this comment.
When live parse-stage prefetch is enabled, a small unknown JSONL containing both a valid generic session record and a malformed record is admitted as a candidate, but live_parse_worker decodes it without fail_on_decode_error=True and caches the partial session list. _ingest_full_records_archive then consumes cached_sessions and skips this newly strict inline decoder entirely, so the malformed input is materialized as a successful partial conversation without terminal_unknown_json_decode evidence. Pass the same strict option through prefetch or avoid caching unknown-provider parses.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Addressed in 28b99af. Both prefetch worker branches pass strict decoding for Provider.UNKNOWN, and the real mixed JSONL watcher route proves no partial session is cached or materialized.
|
Scope for the next repair commit(s): address the four exact-head lifecycle gaps identified in comments 3742338895, 3742338898, and 3742338901. I will make async batch success supersede stale deferred CAS authority with the same transactional semantics as the synchronous route; align typed terminal corrupt-input evidence with validation/readiness lifecycle rules and cover zero-length/decode persistence; carry strict unknown-provider JSONL decoding through prefetch and prevent malformed mixed input from being partially accepted; and exclude terminal unsupported-shape rows from parse backlog retry. I will preserve the source-v30 coordinate-carrier migration and backup requirement, add real-route regressions, run focused tests plus one devtools verify and one devtools verify --quick at the publish boundary, then update the exact-head PR carrier/body. No Beads mutation, live migration, production reprocessing, or merge is in scope. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 28b99af28d
ℹ️ 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".
| evidence_kind = _raw_failure_evidence_kind(outcome) | ||
| if source_backend is not None and outcome is not None and evidence_kind is not None: | ||
| await source_backend.save_raw_failure_evidence( |
There was a problem hiding this comment.
When a raw previously recorded terminal_corrupt_input or terminal_unsupported_shape and a later forced reprocess reaches a parser-defect, validation-rejection, or transient outcome, evidence_kind is None, so this branch updates the current parse failure but leaves the old terminal carrier authoritative. The lifecycle reader then attributes the new failure to that stale terminal cause, and the normal backlog excludes it from retry. Replace or supersede prior failure evidence for every new attempt outcome, not only when the new outcome maps to one of these two terminal kinds. This matters because reprocess deliberately reuses retained raw evidence without acquisition.
AGENTS.md reference: AGENTS.md:L163-L166
Useful? React with 👍 / 👎.
28b99af to
0c0e928
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0c0e92807c
ℹ️ 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".
| if isinstance(state.parse_error, str) and state.parse_error: | ||
| _retire_raw_failure_evidence(store, raw_id, manage_transaction=False) |
There was a problem hiding this comment.
When retained-raw indexing raises MembershipReplayConflictError, _index_parsed_for_retained_raw first calls this generic finalizer, which retires any carrier and commits the parse_error; only after the exception unwinds does the live-batch handler call mark_raw_parse_failed to persist deferred_cas_frontier in a second transaction. If the daemon exits between those commits, source.db permanently presents this retryable conflict as unexplained, and _preflight_raw_failure_lifecycle refuses a bulk rebuild. Persist the typed CAS carrier with the first failure-state mutation, or avoid committing the generic state before the typed handler runs.
AGENTS.md reference: AGENTS.md:L114-L119
Useful? React with 👍 / 👎.
| if validation_failed and not has_trusted_raw_failure_provenance( | ||
| classification_reason, | ||
| artifact_kind=evidence_kind, | ||
| outcome_code=outcome_code, | ||
| ): |
There was a problem hiding this comment.
When an operator applies terminal_corrupt_input to a historical raw whose validation_status is failed, maintenance/raw_failure_disposition_apply.py::_apply_candidate writes the plain kind string as classification_reason; this new check requires a JSON worker-disposition-v1 provenance receipt and a corrupt_input outcome, so the reviewed, backup-gated disposition is still classified as unexplained and bulk-rebuild preflight remains blocked. Make the actuator emit an authoritative structured reason (or recognize its immutable disposition receipt) so its documented recovery workflow can actually close these historical failures.
AGENTS.md reference: AGENTS.md:L189-L192
Useful? React with 👍 / 👎.
| {_raw_artifact_coordinate_predicate(artifact_alias="retry_evidence", raw_alias="r")} | ||
| AND retry_evidence.artifact_kind IN ({", ".join("?" for _ in RAW_FAILURE_REPLAY_AUTHORITY_EVIDENCE_KINDS)}) | ||
| AND retry_evidence.support_status = ? | ||
| ) | ||
| OR r.parse_error LIKE '{_MEMBERSHIP_REPLAY_CONFLICT_ERROR_PREFIX}%' |
There was a problem hiding this comment.
When a historical CAS failure is explicitly classified by raw_failure_disposition_apply as terminal_corrupt_input or terminal_unsupported_shape, that actuator intentionally leaves the original parse_error intact. If the diagnostic has the legacy membership prefix or one of the two exact legacy CAS messages, this predicate still selects the raw for retained replay even though its exact-coordinate carrier is now terminal, so repair can retry input after a reviewed terminal disposition. Keep the legacy compatibility bridge only when no authoritative terminal carrier exists for the raw.
AGENTS.md reference: AGENTS.md:L114-L119
Useful? React with 👍 / 👎.
Persist retained-raw CAS evidence with the first failure-state mutation, emit worker-disposition-v1 classification for reviewed corrupt input, and let exact terminal carriers veto legacy retry markers. Add production-route red twins for all three review findings.\n\nRef #3897
|
@circleci rerun |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 16
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
tests/unit/sources/test_live_batch_support.py (1)
5752-5769: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winThe preceding comment is now truncated mid-sentence.
Lines 5752-5757 end with
"exactly what happened to a real production session that hit this guard under#2718's original"and then stop. The replacement lines 5758-5759 begin a separate statement. The original sentence lost its ending when the parse-error assertion was replaced.Close the sentence so the rationale stays readable.
✏️ Proposed comment repair
# polylogue-5iz4: this guard's refusal is transient/retry-eligible by # construction (a later pass over the same durable bytes can succeed # once sibling evidence resolves), but a plain RuntimeError leaves the # retry-candidate query (storage/repair.py) nothing stable to match # once the message text drifts -- exactly what happened to a real # production session that hit this guard under `#2718`'s original - # The structured evidence row, not the diagnostic wording, is the - # retry authorization. + # wording. The structured evidence row, not the diagnostic wording, + # is therefore the retry authorization asserted below.🤖 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/unit/sources/test_live_batch_support.py` around lines 5752 - 5769, Complete the truncated explanatory comment immediately before the SQLite assertion, adding the missing ending to the sentence about the production session and issue `#2718`. Do not alter the assertion or surrounding logic; keep the comment focused on the retry-candidate matching rationale.polylogue/storage/sqlite/archive_tiers/revision_governance.py (1)
3021-3035: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winDeclare the retire + state-update transaction explicitly.
Both statements currently land in one transaction only because
_retire_raw_failure_evidencestarts sqlite3's implicit transaction and thewith conn:insideapply_source_raw_state_updatecommits or rolls back that same implicit transaction. The atomicity is real but incidental.mark_raw_parse_failedon Line 3048 states the same guarantee explicitly with an outerwith conn:andmanage_transaction=Falseon both calls.Use the same explicit shape here. It keeps a later edit from silently splitting the retire and the state update into two commits, which would leave a raw with retired evidence and no recorded failure.
♻️ Proposed change
def finalize_raw_parse_state(store: RawRevisionGovernanceHost, raw_id: str, *, state: RawSessionStateUpdate) -> None: """Commit one typed source parse state after its index outcome.""" - if isinstance(state.parse_error, str) and state.parse_error: - _retire_raw_failure_evidence(store, raw_id, manage_transaction=False) - apply_source_raw_state_update( - store._ensure_source_conn(), - raw_id, - state=state, - manage_transaction=True, - ) + conn = store._ensure_source_conn() + with conn: + if isinstance(state.parse_error, str) and state.parse_error: + _retire_raw_failure_evidence(store, raw_id, manage_transaction=False) + apply_source_raw_state_update( + conn, + raw_id, + state=state, + manage_transaction=False, + )🤖 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/storage/sqlite/archive_tiers/revision_governance.py` around lines 3021 - 3035, Make finalize_raw_parse_state explicitly wrap _retire_raw_failure_evidence and apply_source_raw_state_update in one outer connection transaction, matching mark_raw_parse_failed. Reuse the same connection and pass manage_transaction=False to both operations so retirement and state persistence commit or roll back atomically; preserve the existing parse_error condition.polylogue/sources/decoder_json.py (1)
285-292: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMake
fail_on_decode_errorkeyword-only oniter_json_stream_with.The function is exposed in
__all__, and the only existing third-party call site can safely switch tofail_on_decode_error=. Making the flag keyword-only prevents transposition with the adjacentunpack_listsboolean and keeps the public signature aligned with the wrapper signatures.🤖 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/decoder_json.py` around lines 285 - 292, Update iter_json_stream_with so fail_on_decode_error is declared keyword-only after the existing positional parameters, matching the wrapper signatures. Adjust the known external call site to pass fail_on_decode_error by name, while preserving unpack_lists behavior and the public export.
🤖 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/core/raw_failure_evidence.py`:
- Around line 184-194: Update RAW_FAILURE_TERMINAL_EVIDENCE_KINDS to derive its
values by filtering RawFailureEvidenceKind for kind.lifecycle == "terminal",
matching the derivation used by
RAW_FAILURE_TERMINAL_EVIDENCE_SUPPORT_STATUS_PAIRS. Remove the hand-maintained
terminal-kind list while preserving the resulting frozenset type.
- Line 85: Update the JSON serialization in the failure carrier’s encoding
function to call polylogue.core.json.dumps instead of the standard json.dumps,
preserving the existing payload ordering and compact separator options so
raw_failure_classification_reason and raw_failure_outcome_code remain
consistently comparable.
In `@polylogue/pipeline/services/ingest_batch/_core.py`:
- Around line 2399-2414: Update the batch failure handling around
_failed_raw_state_update to reuse a single outcome value by hoisting
outcomes.get(rid) before the update_raw_state call and passing or retaining it
for subsequent processing. In the source_backend evidence branch, retire raw
failure evidence whenever no terminal evidence kind is resolved, including
outcome is None, while preserving save_raw_failure_evidence for typed failures
with a resolved kind.
- Around line 2286-2297: Update _raw_failure_evidence_kind to convert invalid or
unknown outcome_code values to IngestOutcome.LEGACY_UNKNOWN before the carrier
mapping, rather than returning None from the ValueError handler. Preserve None
for a missing outcome and keep the existing terminal mappings unchanged, while
ensuring unknown outcomes resolve through the mapping without losing failure
evidence.
In `@polylogue/pipeline/services/ingest_batch/_models.py`:
- Around line 74-81: The outcome_code default in the batch summary model should
use the canonical IngestOutcome.SUCCESS.value rather than the bare "success"
literal. Import IngestOutcome from polylogue.core.enums and update the
outcome_code field, keeping it aligned with the mirrored field in
ingest_worker.py and _raw_failure_evidence_kind.
In `@polylogue/sources/decoder_json.py`:
- Around line 218-223: Update _yield_jsonl_pending to return the first caught
decode failure alongside its line information, and have _iter_jsonl_stream
retain it in a first_decode_error local. When raising JsonlDecodeError, pass
that stored exception as cause instead of creating a generic ValueError, and use
an explicit is None check for the first_decode_error_line fallback.
In `@polylogue/sources/live/batch.py`:
- Around line 2099-2102: Update the large-payload branch that calls
_parse_path_as_session_artifact() to exclude paths only when parsing fails and
provider is not Provider.UNKNOWN, matching the condition around
_parse_payload_as_session_artifact(). Ensure unknown providers continue to the
streaming and strict-decoding path.
In `@polylogue/storage/raw_failure_lifecycle.py`:
- Around line 176-192: Update the summary SQL used by _lifecycle to stop
grouping by the per-raw a.classification_reason JSON document. Derive and group
by a boolean trusted-provenance flag instead, while retaining a representative
classification_reason via an aggregate such as MIN for downstream
raw_failure_outcome_code and has_trusted_raw_failure_provenance; preserve the
existing origin, validation_status, artifact_kind, and support_status grouping
and ordering so summary rows remain bounded.
In `@polylogue/storage/sqlite/archive_tiers/revision_governance.py`:
- Around line 3225-3229: Update the sync retire logic around retired_kinds to
derive it from the shared RAW_FAILURE_EVIDENCE_KINDS constant, excluding
TERMINAL_SUPERSEDED_DEFERRED_CAS_FRONTIER.value, instead of iterating
RawFailureEvidenceKind. Add RAW_FAILURE_EVIDENCE_KINDS to the existing import
block and preserve the current update behavior.
- Around line 3123-3138: Update the classification_reason call in the affected
failure-recording flow to derive retryable from kind, preserving true for
DEFERRED_CAS_FRONTIER and DEFERRED_CODEX_CAS_FRONTIER rather than hard-coding
false. Simplify trusted_validation_failure by removing the redundant
outcome_code == "corrupt_input" conjunct, since the kind membership already
implies it.
In `@polylogue/storage/sqlite/archive_tiers/source_write.py`:
- Around line 1292-1300: Update the existing-artifact query in the archive write
flow around conn.execute and fetchone() to add a deterministic ORDER BY
tiebreaker and LIMIT 1. Order by a stable unique artifact identifier, such as
artifact_id, so coordinates matching multiple artifact_kind rows always select
the same carrier while preserving the existing predicate and transaction
behavior.
In `@polylogue/storage/sqlite/archive_tiers/source.py`:
- Around line 578-604: The failure-kind partition is duplicated across the live
DDL and historical migration. In
polylogue/storage/sqlite/archive_tiers/source.py:578-604, update SOURCE_DDL to
interpolate a single sorted list derived from RawFailureEvidenceKind for both
indexes; in
polylogue/storage/sqlite/migrations/source/030_raw_failure_coordinate_carriers.sql:1-34,
keep the historical literal list unchanged and add a version-30 parity test
asserting it matches the enum values.
In `@polylogue/storage/sqlite/queries/artifacts.py`:
- Around line 326-357: The retirement UPDATE in the artifact-retirement flow
must use a CAS-neutral dedicated retirement kind with lifecycle "resolution",
preserving each carrier’s original kind for previous_artifact_kind receipts, and
set last_observed_at_ms to the current observation timestamp. Update the
RawFailureEvidenceKind definition and the query/parameters around the
retired-kinds UPDATE, following the timestamp handling used near lines 226-227.
- Around line 226-227: Update save_raw_failure_evidence to accept an
observed_at_ms keyword and use it for both first_observed_at_ms and
last_observed_at_ms, falling back to acquired_at_ms only when no observation
time is provided. At the caller boundary, supply the current clock value for
each write while preserving acquired_at_ms as the raw artifact timestamp.
- Around line 202-206: Update the trusted_validation_failure expression in the
artifact query logic to use the existing RAW_FAILURE_VALIDATION_FAILURE_KINDS
constant instead of duplicating the literal evidence-kind set, preserving the
current validation_failed and outcome_code conditions.
In `@tests/unit/pipeline/test_ingest_batch.py`:
- Around line 4118-4181: The
test_process_ingest_batch_public_route_persists_corrupt_input_readiness
assertions should not require exact decoder diagnostics from msgspec. Replace
the diagnostic parameter and exact parse_error equality with assertions that
validation_status is "failed" and parse_error starts with "decode:", while
preserving the existing terminal artifact support_status and lifecycle/status
assertions.
---
Outside diff comments:
In `@polylogue/sources/decoder_json.py`:
- Around line 285-292: Update iter_json_stream_with so fail_on_decode_error is
declared keyword-only after the existing positional parameters, matching the
wrapper signatures. Adjust the known external call site to pass
fail_on_decode_error by name, while preserving unpack_lists behavior and the
public export.
In `@polylogue/storage/sqlite/archive_tiers/revision_governance.py`:
- Around line 3021-3035: Make finalize_raw_parse_state explicitly wrap
_retire_raw_failure_evidence and apply_source_raw_state_update in one outer
connection transaction, matching mark_raw_parse_failed. Reuse the same
connection and pass manage_transaction=False to both operations so retirement
and state persistence commit or roll back atomically; preserve the existing
parse_error condition.
In `@tests/unit/sources/test_live_batch_support.py`:
- Around line 5752-5769: Complete the truncated explanatory comment immediately
before the SQLite assertion, adding the missing ending to the sentence about the
production session and issue `#2718`. Do not alter the assertion or surrounding
logic; keep the comment focused on the retry-candidate matching rationale.
🪄 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: 776d5eb4-d1fa-4e59-a8e6-80ed61546ccf
📒 Files selected for processing (39)
docs/plans/layering-surface-baseline.jsondocs/plans/layering.yamlpolylogue/core/errors.pypolylogue/core/raw_failure_evidence.pypolylogue/daemon/status.pypolylogue/maintenance/raw_failure_disposition_apply.pypolylogue/pipeline/services/ingest_batch/_core.pypolylogue/pipeline/services/ingest_batch/_models.pypolylogue/pipeline/services/ingest_worker.pypolylogue/pipeline/services/planning_backlog.pypolylogue/sources/decoder_json.pypolylogue/sources/decoders.pypolylogue/sources/live/batch.pypolylogue/sources/live/parse_prefetch.pypolylogue/storage/raw/artifacts.pypolylogue/storage/raw_failure_lifecycle.pypolylogue/storage/repair.pypolylogue/storage/sqlite/archive_tiers/archive.pypolylogue/storage/sqlite/archive_tiers/revision_governance.pypolylogue/storage/sqlite/archive_tiers/source.pypolylogue/storage/sqlite/archive_tiers/source_write.pypolylogue/storage/sqlite/async_sqlite_raw.pypolylogue/storage/sqlite/migrations/source/030.train.jsonpolylogue/storage/sqlite/migrations/source/030_raw_failure_coordinate_carriers.sqlpolylogue/storage/sqlite/queries/artifacts.pypolylogue/storage/sqlite/queries/raw_reads.pypolylogue/storage/sqlite/query_store_maintenance.pytests/unit/daemon/test_raw_failure_sample.pytests/unit/maintenance/test_raw_failure_disposition_apply.pytests/unit/pipeline/test_ingest_batch.pytests/unit/pipeline/test_parsing_service.pytests/unit/sinex/test_ingest_atomicity.pytests/unit/sources/test_decoders.pytests/unit/sources/test_live_batch_support.pytests/unit/sources/test_live_watcher_parse_stage_equivalence.pytests/unit/storage/test_archive_tiers_source_write.pytests/unit/storage/test_durable_migrations.pytests/unit/storage/test_repair.pytests/unit/storage/test_revision_replay.py
💤 Files with no reviewable changes (1)
- docs/plans/layering-surface-baseline.json
| } | ||
| if trusted_validation_failure: | ||
| payload["provenance"] = RAW_FAILURE_TRUSTED_PROVENANCE | ||
| return json.dumps(payload, sort_keys=True, separators=(",", ":")) |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check the repository JSON helper contract and existing usage conventions.
fd -t f 'json.py' polylogue/core --exec sed -n '380,440p'
rg -nP --type=py '\bjson\.dumps\s*\(' polylogue | head -50
rg -nP --type=py 'from polylogue\.core\.json import' polylogue | head -30Repository: Sinity/polylogue
Length of output: 11237
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== file outline =="
ast-grep outline polylogue/core/raw_failure_evidence.py || true
echo "== relevant file contents =="
cat -n polylogue/core/raw_failure_evidence.py | sed -n '1,140p'
echo "== json.py relevant exports/import helpers =="
cat -n polylogue/core/json.py | sed -n '320,455p'
grep -n '^def ' polylogue/core/json.py | sed -n '1,80p'
echo "== calls to raw_failure helpers around json =="
rg -n 'raw_failure_outcome_code|has_trusted_raw_failure_provenance|load_json_json|dump_json_json|json.dumps|json.loads|from polylogue\.core\.import json|import .*json' polylogue/core raw_failure_evidence.py polylogue/core/raw_failure_evidence.py 2>/dev/null || trueRepository: Sinity/polylogue
Length of output: 18231
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== backend defaults and config =="
grep -nE '_BACKEND|BACKEND|core\.json|dumps_bytes|load_json_json|dump_json_json|canonical_json|to_payload|from_dict|read.*json|write.*json' polylogue/core/raw_failure_evidence.py polylogue/core/raw_failure_evidence.py 2>/dev/null || true
rg -nE 'class RawFailure|raw_failure_classification_reason|validated_raw_failure_evidence_kind|load_json_json|dump_json_json|to_payload|read.*json|write.*json' polylogue --python -g '*.py' | head -120
echo "== deterministic json contract probe =="
python3 - <<'PY'
import ast
from pathlib import Path
json_path = Path("polylogue/core/json.py")
tree = ast.parse(json_path.read_text())
for func_name in ("_BACKEND", "dumps", "dumps_bytes", "loads"):
nodes = [n for n in tree.body if isinstance(n, (ast.Assign, ast.AnnAssign)) and any(getattr(t, "id", None) == func_name for t in ast.walk(n.target)) if isinstance(n, ast.Assign)] or [n for n in tree.body if isinstance(n, ast.Assign)]
print("__BACKEND assignment candidates")
for node in [n for n in ast.walk(tree) if isinstance(n, ast.Assign)]:
for target in node.targets:
if isinstance(target, ast.Name) and target.id == "_BACKGET":
print(ast.get_source_segment(json_path.read_text(), node))
for node in tree.body:
if isinstance(node, ast.Assign):
for target in node.targets:
if isinstance(target, ast.Name) and target.id == "_BACKEND":
print(f"_BACKEND line={node.lineno} value={ast.unparse(node.value)}")
print("dumps signature:", ast.get_source_segment(json_path.read_text(), next(n for n in tree.body if isinstance(n, ast.FunctionDef) and n.name == "dumps")))
PYRepository: Sinity/polylogue
Length of output: 403
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== relevant searches without -E flag =="
rg -n -g '*.py' 'class RawFailure|raw_failure_classification_reason|validated_raw_failure_evidence_kind|load_json_json|dump_json_json|to_payload|read.*json|write.*json|_BACKEND|_BACKGET' polylogue | head -200
echo "== json.py top imports and backend assignments =="
sed -n '1,120p' polylogue/core/json.py | cat -n
echo "== deterministic json contract probe =="
python3 - <<'PY'
import ast
from pathlib import Path
json_path = Path("polylogue/core/json.py")
src = json_path.read_text()
tree = ast.parse(src)
for node in tree.body:
if isinstance(node, ast.Assign):
for target in node.targets:
if isinstance(target, ast.Name) and target.id == "_BACKEND":
print(f"line={node.lineno} value={ast.unparse(node.value)}")
if isinstance(node, ast.FunctionDef):
if node.name == "dumps":
print("dumps_function=")
print(ast.get_source_segment(src, node))
elif node.name == "dumps_bytes":
print("dumps_bytes_function_signature=")
print(src.splitlines()[node.lineno-1])
with open("temp_json_check.py", "w", encoding="utf-8") as f:
f.write('''
def stdlib_compact_sort(payload):
import json
return json.dumps(payload, sort_keys=True, separators=(",", ":"))
def core_json_string_via_dumps_bytes(payload):
import json as _stdlib_json
separators = (",", ":")
return _stdlib_json.dumps(
payload, default=None, sort_keys=True, indent=None, separators=separators, ensure_ascii=False,
).encode("utf-8").decode("utf-8")
payload = {"a": 1, "b": 2}
print(stdlib_compact_sort(payload) == core_json_string_via_dumps_bytes(payload))
''')
PY
python3 temp_json_check.py
rm temp_json_check.pyRepository: Sinity/polylogue
Length of output: 28364
Use polylogue.core.json.dumps for the failure carrier.
raw_failure_classification_reason and raw_failure_outcome_code both use JSON for a shared, persisted artifact. This carrier needs consistent encoding/decoding behavior through the repository JSON facade so the comparison stays invariant.
🤖 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/core/raw_failure_evidence.py` at line 85, Update the JSON
serialization in the failure carrier’s encoding function to call
polylogue.core.json.dumps instead of the standard json.dumps, preserving the
existing payload ordering and compact separator options so
raw_failure_classification_reason and raw_failure_outcome_code remain
consistently comparable.
Source: Linters/SAST tools
| RAW_FAILURE_TERMINAL_EVIDENCE_KINDS = frozenset( | ||
| { | ||
| RawFailureEvidenceKind.TERMINAL_CORRUPT_INPUT.value, | ||
| RawFailureEvidenceKind.TERMINAL_UNKNOWN_JSON_DECODE.value, | ||
| RawFailureEvidenceKind.TERMINAL_UNKNOWN_EXPORT_NO_SESSION.value, | ||
| RawFailureEvidenceKind.TERMINAL_UNSUPPORTED_SHAPE.value, | ||
| } | ||
| ) | ||
| RAW_FAILURE_TERMINAL_EVIDENCE_SUPPORT_STATUS_PAIRS = tuple( | ||
| sorted((kind.value, kind.support_status.value) for kind in RawFailureEvidenceKind if kind.lifecycle == "terminal") | ||
| ) |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
Derive RAW_FAILURE_TERMINAL_EVIDENCE_KINDS from the lifecycle property.
Lines 184-191 hand-list the terminal kinds. Lines 192-194 derive the terminal pairs from kind.lifecycle == "terminal". Two sources for one partition can drift when a new kind is added. Derive the set from the same property.
♻️ Proposed refactor
-RAW_FAILURE_TERMINAL_EVIDENCE_KINDS = frozenset(
- {
- RawFailureEvidenceKind.TERMINAL_CORRUPT_INPUT.value,
- RawFailureEvidenceKind.TERMINAL_UNKNOWN_JSON_DECODE.value,
- RawFailureEvidenceKind.TERMINAL_UNKNOWN_EXPORT_NO_SESSION.value,
- RawFailureEvidenceKind.TERMINAL_UNSUPPORTED_SHAPE.value,
- }
-)
+RAW_FAILURE_TERMINAL_EVIDENCE_KINDS = frozenset(
+ kind.value for kind in RawFailureEvidenceKind if kind.lifecycle == "terminal"
+)📝 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.
| RAW_FAILURE_TERMINAL_EVIDENCE_KINDS = frozenset( | |
| { | |
| RawFailureEvidenceKind.TERMINAL_CORRUPT_INPUT.value, | |
| RawFailureEvidenceKind.TERMINAL_UNKNOWN_JSON_DECODE.value, | |
| RawFailureEvidenceKind.TERMINAL_UNKNOWN_EXPORT_NO_SESSION.value, | |
| RawFailureEvidenceKind.TERMINAL_UNSUPPORTED_SHAPE.value, | |
| } | |
| ) | |
| RAW_FAILURE_TERMINAL_EVIDENCE_SUPPORT_STATUS_PAIRS = tuple( | |
| sorted((kind.value, kind.support_status.value) for kind in RawFailureEvidenceKind if kind.lifecycle == "terminal") | |
| ) | |
| RAW_FAILURE_TERMINAL_EVIDENCE_KINDS = frozenset( | |
| kind.value for kind in RawFailureEvidenceKind if kind.lifecycle == "terminal" | |
| ) | |
| RAW_FAILURE_TERMINAL_EVIDENCE_SUPPORT_STATUS_PAIRS = tuple( | |
| sorted((kind.value, kind.support_status.value) for kind in RawFailureEvidenceKind if kind.lifecycle == "terminal") | |
| ) |
🤖 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/core/raw_failure_evidence.py` around lines 184 - 194, Update
RAW_FAILURE_TERMINAL_EVIDENCE_KINDS to derive its values by filtering
RawFailureEvidenceKind for kind.lifecycle == "terminal", matching the derivation
used by RAW_FAILURE_TERMINAL_EVIDENCE_SUPPORT_STATUS_PAIRS. Remove the
hand-maintained terminal-kind list while preserving the resulting frozenset
type.
| def _raw_failure_evidence_kind(outcome: _RawIngestOutcome | None) -> RawFailureEvidenceKind | None: | ||
| """Map terminal worker input outcomes to closed source-tier carriers.""" | ||
| if outcome is None: | ||
| return None | ||
| try: | ||
| outcome_code = IngestOutcome.from_string(outcome.outcome_code) | ||
| except ValueError: | ||
| return None | ||
| return { | ||
| IngestOutcome.CORRUPT_INPUT: RawFailureEvidenceKind.TERMINAL_CORRUPT_INPUT, | ||
| IngestOutcome.UNSUPPORTED_SHAPE: RawFailureEvidenceKind.TERMINAL_UNSUPPORTED_SHAPE, | ||
| }.get(outcome_code) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Inspect the IngestOutcome enum and its from_string contract.
rg -nP -C 12 'class IngestOutcome' --type=py
ast-grep run --pattern 'def from_string($$$): $$$' --lang python polylogue/core/enums.pyRepository: Sinity/polylogue
Length of output: 154
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Py files mentioning IngestOutcome:"
rg -n --type=py 'IngestOutcome' . || true
echo
echo "Candidate files:"
git ls-files '*ingest_batch*' '*enums*' | sed -n '1,200p'
echo
echo "Core enum files:"
fd -a 'enums\.py$|.*enum.*\.py$' . | sed -n '1,80p'Repository: Sinity/polylogue
Length of output: 8882
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "PolylogueStrEnum and IngestOutcome implementation:"
sed -n '450,550p' polylogue/core/enums.py
echo
echo "Context around _failed_raw_state_update and _raw_failure_evidence_kind:"
sed -n '2260,2305p' polylogue/pipeline/services/ingest_batch/_core.py
echo
echo "Relevant tests around raw failure evidence mapping:"
sed -n '3860,3945p' tests/unit/pipeline/test_ingest_batch.py
echo
echo "Read-only behavioral probe of the enum.from_string contract from installed source metadata (no repo execution):"
python3 - <<'PY'
import ast
from pathlib import Path
src = Path('polylogue/core/enums.py').read_text()
tree = ast.parse(src)
class_finder = {}
for node in tree.body:
if isinstance(node, ast.ClassDef) and node.name == 'PolylogueStrEnum':
for item in node.body:
if isinstance(item, ast.FunctionDef) and item.name == 'from_string':
class_finder['PolylogueStrEnum.from_string'] = ast.get_source_segment(src, item)
elif isinstance(node, ast.ClassDef) and node.name == 'IngestOutcome':
for item in node.body:
if isinstance(item, ast.FunctionDef) and item.name == 'from_string':
class_finder['IngestOutcome.from_string'] = ast.get_source_segment(src, item)
for name, segment in class_finder.items():
print(f'--- {name} ---')
print(segment)
PYRepository: Sinity/polylogue
Length of output: 9109
Handle invalid IngestOutcome strings before mapping to terminal carriers.
IngestOutcome.from_string(...) raises ValueError for unrecognized codes instead of returning an unknown sentinel, so the except ValueError branch is reachable and currently skips invalid outcome_code values without a carrier. Resolve invalid or unknown codes explicitly before .get(...), such as by mapping LEGACY_UNKNOWN, to avoid losing the failure cause.
🤖 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/pipeline/services/ingest_batch/_core.py` around lines 2286 - 2297,
Update _raw_failure_evidence_kind to convert invalid or unknown outcome_code
values to IngestOutcome.LEGACY_UNKNOWN before the carrier mapping, rather than
returning None from the ValueError handler. Preserve None for a missing outcome
and keep the existing terminal mappings unchanged, while ensuring unknown
outcomes resolve through the mapping without losing failure evidence.
| outcome = outcomes.get(rid) | ||
| evidence_kind = _raw_failure_evidence_kind(outcome) | ||
| if source_backend is not None: | ||
| if outcome is not None and evidence_kind is not None: | ||
| await source_backend.save_raw_failure_evidence( | ||
| rid, | ||
| artifact_kind=evidence_kind.value, | ||
| support_status=evidence_kind.support_status.value, | ||
| outcome_code=outcome.outcome_code, | ||
| retryable=outcome.retryable, | ||
| evidence_ref=outcome.evidence_ref, | ||
| remediation=outcome.remediation, | ||
| diagnostic=outcome.diagnostic, | ||
| ) | ||
| elif outcome is not None: | ||
| await source_backend.retire_raw_failure_evidence(rid) |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
An untyped failure with no recorded outcome keeps its stale replay authority.
When outcomes.get(rid) returns None, neither branch runs, so no carrier is saved and none is retired. _failed_raw_state_update on Line 2270 explicitly handles that same outcome is None case, which shows the batch can mark a raw failed without a worker outcome record. In that path an earlier DEFERRED_CAS_FRONTIER carrier survives the new failure and continues to authorize replay.
The sync path does not have this gap. finalize_raw_parse_state in polylogue/storage/sqlite/archive_tiers/revision_governance.py Line 3028 retires prior evidence whenever a non-empty parse_error is recorded, regardless of whether a typed disposition exists. Retire here too when no terminal kind is resolved.
The outcome local also re-reads outcomes.get(rid), which _failed_raw_state_update already received on Line 2394. Hoist it above the update_raw_state call.
🐛 Proposed fix
for rid, error in failed_raw_ids.items():
+ outcome = outcomes.get(rid)
await service.repository.update_raw_state(
rid,
state=_failed_raw_state_update(
- outcome=outcomes.get(rid),
+ outcome=outcome,
error=error,
validation_mode=validation_mode,
),
)
- outcome = outcomes.get(rid)
evidence_kind = _raw_failure_evidence_kind(outcome)
if source_backend is not None:
if outcome is not None and evidence_kind is not None:
await source_backend.save_raw_failure_evidence(
rid,
artifact_kind=evidence_kind.value,
support_status=evidence_kind.support_status.value,
outcome_code=outcome.outcome_code,
retryable=outcome.retryable,
evidence_ref=outcome.evidence_ref,
remediation=outcome.remediation,
diagnostic=outcome.diagnostic,
)
- elif outcome is not None:
+ else:
await source_backend.retire_raw_failure_evidence(rid)📝 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.
| outcome = outcomes.get(rid) | |
| evidence_kind = _raw_failure_evidence_kind(outcome) | |
| if source_backend is not None: | |
| if outcome is not None and evidence_kind is not None: | |
| await source_backend.save_raw_failure_evidence( | |
| rid, | |
| artifact_kind=evidence_kind.value, | |
| support_status=evidence_kind.support_status.value, | |
| outcome_code=outcome.outcome_code, | |
| retryable=outcome.retryable, | |
| evidence_ref=outcome.evidence_ref, | |
| remediation=outcome.remediation, | |
| diagnostic=outcome.diagnostic, | |
| ) | |
| elif outcome is not None: | |
| await source_backend.retire_raw_failure_evidence(rid) | |
| for rid, error in failed_raw_ids.items(): | |
| outcome = outcomes.get(rid) | |
| await service.repository.update_raw_state( | |
| rid, | |
| state=_failed_raw_state_update( | |
| outcome=outcome, | |
| error=error, | |
| validation_mode=validation_mode, | |
| ), | |
| ) | |
| evidence_kind = _raw_failure_evidence_kind(outcome) | |
| if source_backend is not None: | |
| if outcome is not None and evidence_kind is not None: | |
| await source_backend.save_raw_failure_evidence( | |
| rid, | |
| artifact_kind=evidence_kind.value, | |
| support_status=evidence_kind.support_status.value, | |
| outcome_code=outcome.outcome_code, | |
| retryable=outcome.retryable, | |
| evidence_ref=outcome.evidence_ref, | |
| remediation=outcome.remediation, | |
| diagnostic=outcome.diagnostic, | |
| ) | |
| else: | |
| await source_backend.retire_raw_failure_evidence(rid) |
🤖 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/pipeline/services/ingest_batch/_core.py` around lines 2399 - 2414,
Update the batch failure handling around _failed_raw_state_update to reuse a
single outcome value by hoisting outcomes.get(rid) before the update_raw_state
call and passing or retaining it for subsequent processing. In the
source_backend evidence branch, retire raw failure evidence whenever no terminal
evidence kind is resolved, including outcome is None, while preserving
save_raw_failure_evidence for typed failures with a resolved kind.
| # Keep the worker's typed disposition intact through the batch summary so | ||
| # the raw-state persistence boundary can retain the same evidence instead | ||
| # of reconstructing it from free-form error text. | ||
| outcome_code: str = "success" | ||
| retryable: bool | None = False | ||
| evidence_ref: str | None = None | ||
| remediation: str | None = None | ||
| diagnostic: str | None = None |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
Use the enum value for the outcome_code default.
outcome_code defaults to the bare literal "success". The mirrored field in polylogue/pipeline/services/ingest_worker.py Line 110 defaults to IngestOutcome.SUCCESS.value. If the enum value changes, this literal stops matching and _raw_failure_evidence_kind in _core.py would parse a stale code.
♻️ Proposed change
- outcome_code: str = "success"
+ outcome_code: str = IngestOutcome.SUCCESS.valueThis requires importing IngestOutcome from polylogue.core.enums in this module.
🤖 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/pipeline/services/ingest_batch/_models.py` around lines 74 - 81,
The outcome_code default in the batch summary model should use the canonical
IngestOutcome.SUCCESS.value rather than the bare "success" literal. Import
IngestOutcome from polylogue.core.enums and update the outcome_code field,
keeping it aligned with the mirrored field in ingest_worker.py and
_raw_failure_evidence_kind.
| CREATE UNIQUE INDEX IF NOT EXISTS idx_raw_artifacts_source_identity | ||
| ON raw_artifacts(origin, source_path, source_index); | ||
| ON raw_artifacts(origin, source_path, source_index) | ||
| WHERE artifact_kind NOT IN ( | ||
| 'deferred_hot_jsonl_capture', | ||
| 'deferred_claude_code_partial_jsonl', | ||
| 'deferred_cas_frontier', | ||
| 'deferred_codex_cas_frontier', | ||
| 'terminal_corrupt_input', | ||
| 'terminal_superseded_deferred_cas_frontier', | ||
| 'terminal_unknown_json_decode', | ||
| 'terminal_unknown_export_no_session', | ||
| 'terminal_unsupported_shape' | ||
| ); | ||
|
|
||
| CREATE UNIQUE INDEX IF NOT EXISTS idx_raw_artifacts_failure_identity | ||
| ON raw_artifacts(raw_id, origin, source_path, source_index) | ||
| WHERE artifact_kind IN ( | ||
| 'deferred_hot_jsonl_capture', | ||
| 'deferred_claude_code_partial_jsonl', | ||
| 'deferred_cas_frontier', | ||
| 'deferred_codex_cas_frontier', | ||
| 'terminal_corrupt_input', | ||
| 'terminal_superseded_deferred_cas_frontier', | ||
| 'terminal_unknown_json_decode', | ||
| 'terminal_unknown_export_no_session', | ||
| 'terminal_unsupported_shape' | ||
| ); |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
The failure-kind index partition is hardcoded in two SQL sites. Both sites list the nine RawFailureEvidenceKind values as SQL literals. If a new evidence kind is added to the enum and one list is not updated, the new kind falls under idx_raw_artifacts_source_identity, and two failed raws at the same coordinate collide on insert. Keep one authority for the partition and add a parity test.
polylogue/storage/sqlite/archive_tiers/source.py#L578-L604: interpolate the kind list intoSOURCE_DDLfromRawFailureEvidenceKind, using the same sorted order for both indexes.polylogue/storage/sqlite/migrations/source/030_raw_failure_coordinate_carriers.sql#L1-L34: keep the literal list, because a migration must stay frozen at its historical schema, and add a test that asserts the migration list equals the enum values at version 30.
📍 Affects 2 files
polylogue/storage/sqlite/archive_tiers/source.py#L578-L604(this comment)polylogue/storage/sqlite/migrations/source/030_raw_failure_coordinate_carriers.sql#L1-L34
🤖 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/storage/sqlite/archive_tiers/source.py` around lines 578 - 604, The
failure-kind partition is duplicated across the live DDL and historical
migration. In polylogue/storage/sqlite/archive_tiers/source.py:578-604, update
SOURCE_DDL to interpolate a single sorted list derived from
RawFailureEvidenceKind for both indexes; in
polylogue/storage/sqlite/migrations/source/030_raw_failure_coordinate_carriers.sql:1-34,
keep the historical literal list unchanged and add a version-30 parity test
asserting it matches the enum values.
| trusted_validation_failure=( | ||
| validation_failed | ||
| and evidence_kind.value in {"terminal_corrupt_input", "terminal_unknown_json_decode"} | ||
| and outcome_code == "corrupt_input" | ||
| ), |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Reuse RAW_FAILURE_VALIDATION_FAILURE_KINDS instead of the literal set.
polylogue/core/raw_failure_evidence.py already defines this exact pair of kinds as RAW_FAILURE_VALIDATION_FAILURE_KINDS, and has_trusted_raw_failure_provenance reads it when the carrier is validated. Two copies can drift, and a drift silently produces carriers that never validate.
♻️ Proposed refactor
from polylogue.core.raw_failure_evidence import (
RAW_FAILURE_DEFERRED_SUPPORT_STATUS,
RAW_FAILURE_EVIDENCE_KINDS,
RAW_FAILURE_REPLAY_AUTHORITY_EVIDENCE_KINDS,
+ RAW_FAILURE_VALIDATION_FAILURE_KINDS,
RawFailureEvidenceKind,
raw_failure_classification_reason,
validated_raw_failure_evidence_kind,
)
@@
trusted_validation_failure=(
validation_failed
- and evidence_kind.value in {"terminal_corrupt_input", "terminal_unknown_json_decode"}
+ and evidence_kind.value in RAW_FAILURE_VALIDATION_FAILURE_KINDS
and outcome_code == "corrupt_input"
),📝 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.
| trusted_validation_failure=( | |
| validation_failed | |
| and evidence_kind.value in {"terminal_corrupt_input", "terminal_unknown_json_decode"} | |
| and outcome_code == "corrupt_input" | |
| ), | |
| trusted_validation_failure=( | |
| validation_failed | |
| and evidence_kind.value in RAW_FAILURE_VALIDATION_FAILURE_KINDS | |
| and outcome_code == "corrupt_input" | |
| ), |
🤖 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/storage/sqlite/queries/artifacts.py` around lines 202 - 206, Update
the trusted_validation_failure expression in the artifact query logic to use the
existing RAW_FAILURE_VALIDATION_FAILURE_KINDS constant instead of duplicating
the literal evidence-kind set, preserving the current validation_failed and
outcome_code conditions.
| acquired_at_ms, | ||
| acquired_at_ms, |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
Observation timestamps never advance for repeated evidence writes.
Both first_observed_at_ms and last_observed_at_ms are set to the raw row's acquired_at_ms, which is constant for a given raw. RAW_ARTIFACT_UPSERT_SQL then writes the same value on every update. Recency-ordered readers depend on this column: supersede_deferred_cas_evidence at Line 269 orders by last_observed_at_ms DESC, artifact_id DESC, and read_raw_failure_lifecycle in polylogue/storage/raw_failure_lifecycle.py selects the newest carrier the same way. A later disposition therefore does not order after an earlier one, and selection falls back to artifact_id comparison. Pass an explicit observation time for the current write.
🐛 Proposed direction
- acquired_at_ms,
- acquired_at_ms,
+ observed_at_ms,
+ observed_at_ms,Add an observed_at_ms: int keyword to save_raw_failure_evidence, supply the current clock at the caller boundary, and keep acquired_at_ms only as the fallback when no clock is available.
🤖 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/storage/sqlite/queries/artifacts.py` around lines 226 - 227, Update
save_raw_failure_evidence to accept an observed_at_ms keyword and use it for
both first_observed_at_ms and last_observed_at_ms, falling back to
acquired_at_ms only when no observation time is provided. At the caller
boundary, supply the current clock value for each write while preserving
acquired_at_ms as the raw artifact timestamp.
| await conn.execute( | ||
| f""" | ||
| UPDATE raw_artifacts | ||
| SET artifact_kind = ?, | ||
| support_status = ?, | ||
| classification_reason = ?, | ||
| parse_as_session = 0, | ||
| schema_eligible = 0 | ||
| WHERE raw_id = ? | ||
| AND origin IS ? | ||
| AND source_path IS ? | ||
| AND source_index IS ? | ||
| AND artifact_kind IN ({placeholders}) | ||
| """, | ||
| ( | ||
| RawFailureEvidenceKind.TERMINAL_SUPERSEDED_DEFERRED_CAS_FRONTIER.value, | ||
| RawFailureEvidenceKind.TERMINAL_SUPERSEDED_DEFERRED_CAS_FRONTIER.support_status.value, | ||
| raw_failure_classification_reason( | ||
| diagnostic=None, | ||
| evidence_ref=None, | ||
| outcome_code="failure_attempt_replaced", | ||
| remediation="inspect the current parser failure before retrying", | ||
| retryable=False, | ||
| trusted_validation_failure=False, | ||
| ), | ||
| raw_id, | ||
| origin, | ||
| source_path, | ||
| source_index, | ||
| *sorted(retired_kinds), | ||
| ), | ||
| ) |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
Retirement reuses the CAS-specific kind and leaves last_observed_at_ms unchanged.
Two points in this UPDATE:
- Every retired carrier, including a retired
deferred_hot_jsonl_captureorterminal_unsupported_shape, becomesterminal_superseded_deferred_cas_frontier. The stored kind then contradicts the recorded outcome codefailure_attempt_replaced, and durable receipts that quoteprevious_artifact_kindlose the distinction. Add a dedicated retirement kind whoselifecycleis also"resolution", or rename the existing kind to a CAS-neutral token. - The UPDATE does not set
last_observed_at_ms. Readers that pick the newest carrier by that column cannot see that retirement happened after the original write. This is the same root cause as the comment on Lines 226-227.
🧰 Tools
🪛 OpenGrep (1.26.0)
[ERROR] 326-357: SQL query built via f-string passed to execute()/executemany(). Use parameterized queries with placeholders instead.
(coderabbit.sql-injection.python-fstring-execute)
🤖 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/storage/sqlite/queries/artifacts.py` around lines 326 - 357, The
retirement UPDATE in the artifact-retirement flow must use a CAS-neutral
dedicated retirement kind with lifecycle "resolution", preserving each carrier’s
original kind for previous_artifact_kind receipts, and set last_observed_at_ms
to the current observation timestamp. Update the RawFailureEvidenceKind
definition and the query/parameters around the retired-kinds UPDATE, following
the timestamp handling used near lines 226-227.
| @pytest.mark.asyncio | ||
| @pytest.mark.parametrize( | ||
| ("payload", "diagnostic"), | ||
| [ | ||
| (b"", "decode: Input is a zero-length, empty document"), | ||
| (b"{", "decode: Input data was truncated"), | ||
| ], | ||
| ids=["zero-length-public-route", "decode-failure-public-route"], | ||
| ) | ||
| async def test_process_ingest_batch_public_route_persists_corrupt_input_readiness( | ||
| tmp_path: Path, | ||
| monkeypatch: pytest.MonkeyPatch, | ||
| payload: bytes, | ||
| diagnostic: str, | ||
| ) -> None: | ||
| """The real worker route makes corrupt input terminal and status-readable.""" | ||
| initialize_active_archive_root(tmp_path) | ||
| BlobStore(tmp_path / "blob").write_from_bytes(payload) | ||
| with sqlite3.connect(tmp_path / "source.db") as conn: | ||
| raw_id = write_source_raw_session( | ||
| conn, | ||
| origin=Origin.CODEX_SESSION, | ||
| source_path="public-corrupt.jsonl", | ||
| source_index=0, | ||
| payload=payload, | ||
| acquired_at_ms=1, | ||
| ) | ||
|
|
||
| config = Config(archive_root=tmp_path, render_root=tmp_path / "render", sources=[]) | ||
| monkeypatch.setattr( | ||
| "polylogue.config.load_polylogue_config", | ||
| lambda: SimpleNamespace(schema_validation="advisory", sinex_mode="off"), | ||
| ) | ||
| repository = SessionRepository(backend=SQLiteBackend(db_path=tmp_path / "index.db"), archive_root=tmp_path) | ||
| service = ParsingService(repository=repository, archive_root=tmp_path, config=config, ingest_workers=1) | ||
| parse_result = ParseResult() | ||
| try: | ||
| await ingest_batch_core.process_ingest_batch( | ||
| service, | ||
| repository.backend, | ||
| [raw_id], | ||
| parse_result, | ||
| None, | ||
| repair_message_fts=False, | ||
| ) | ||
| finally: | ||
| await repository.close() | ||
|
|
||
| with sqlite3.connect(tmp_path / "source.db") as conn: | ||
| assert conn.execute( | ||
| "SELECT validation_status, parse_error FROM raw_sessions WHERE raw_id = ?", (raw_id,) | ||
| ).fetchone() == ("failed", diagnostic) | ||
| assert conn.execute( | ||
| "SELECT artifact_kind, support_status FROM raw_artifacts WHERE raw_id = ?", (raw_id,) | ||
| ).fetchone() == (RawFailureEvidenceKind.TERMINAL_CORRUPT_INPUT.value, "decode_failed") | ||
|
|
||
| lifecycle = read_raw_failure_lifecycle(tmp_path / "source.db") | ||
| assert lifecycle.terminal == 1 | ||
| assert lifecycle.unexplained == 0 | ||
| assert lifecycle.blocking is False | ||
| status = raw_failure_info_for_root(tmp_path) | ||
| assert status["terminal_rejections"] == 1 | ||
| assert status["unexplained_failures"] == 0 | ||
|
|
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Identify the JSON decode backend and whether the pinned decoder wording is backend-specific.
set -euo pipefail
# The decode helper and its fallback chain.
fd -t f 'json.py' -p 'polylogue/core' --exec cat -n {}
# Any other place that pins these exact decoder phrases.
rg -n 'zero-length, empty document|Input data was truncated'
# Declared JSON backend dependencies.
fd -t f 'pyproject.toml' -d 2 --exec rg -n 'msgspec|orjson|ujson|simplejson' {}Repository: Sinity/polylogue
Length of output: 22584
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== locate ingest_batch.py =="
fd -t f 'test_ingest_batch.py' --exec sh -c 'echo "--- $1"; sed -n "4020,4190p" "$1"; wc -l "$1"' sh {}
echo
echo "== JSON load helpers/usages around raw parsing =="
rg -n "def .*parse|loads|parse_error|validation_status|RawFailureEvidenceKind|decode:|JSONDecodeError|json_loads|json.loads" tests/unit/pipeline/test_ingest_batch.py tests polylogue --glob '*.py' | head -n 200Repository: Sinity/polylogue
Length of output: 27171
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== decode.py =="
fd -t f 'decode.py' -p 'polylogue/archive/raw_payload' --exec sh -c 'echo "--- $1"; cat -n "$1"' sh {}
echo
echo "== run helper probe for error wording and backend availability =="
python3 - <<'PY'
import importlib.util
from pathlib import Path
msgspec_json_path = importlib.util.find_spec("msgspec.json")
print("msgspec_json_available=", msgspec_json_path is not None)
if msgspec_json_path is not None:
import msgspec.json
cases = (b"", b"{")
for data in cases:
try:
msgspec.json.decode(data)
except Exception as exc:
print("msgspec", repr(data), type(exc).__name__, str(exc))
else:
msgspec_error_text = None
try:
import json
for data in cases:
try:
json.loads(data)
except Exception as exc:
print("stdlib", repr(data), type(exc).__name__, str(exc))
except Exception as exc:
print("stdlib_failure", repr(exc))
print("msgspec error text used by assertions:", msgspec_error_text)
PYRepository: Sinity/polylogue
Length of output: 22982
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== locate python version and JSON load helper probe output =="
python3 - <<'PY'
import sys
print("python=", sys.version)
try:
import importlib.util
msgspec_json_path = importlib.util.find_spec("msgspec.json")
print("msgspec_json_available=", msgspec_json_path is not None)
except Exception as exc:
print("msgspec_search_error=", type(exc).__name__, str(exc))
for mod_name in ("json", "polylogue.core.json"):
try:
import importlib
m = importlib.import_module(mod_name)
print(f"{mod_name}_backend=", getattr(m, "backend", None))
except Exception as exc:
print(f"{mod_name}_import_error=", type(exc).__name__, str(exc))
cases = [0, b"", b"{}", b"{"]
for mod_name in ("json", "polylogue.core.json"):
print("==", mod_name, "==")
try:
import importlib
m = importlib.import_module(mod_name)
loads = getattr(m, "loads", None)
if loads is None:
print("no loads")
continue
for data in cases:
try:
print(repr(data), loads(data))
except Exception as exc:
print(repr(data), type(exc).__name__, str(exc))
# Also print the exact prefix to avoid whitespace mismatch.
print("startswith_decode:", str(exc).startswith("decode:"))
except Exception as exc:
print("import_error=", type(exc).__name__, str(exc))
PYRepository: Sinity/polylogue
Length of output: 967
Avoid pinning msgspec decoder text in corrupt-input assertions.
The real route can surface polylogue.core.json.loads decode errors as "decode: " plus the active decoder message. These exact third-party phrases change with the backend upgrade or absence, while the batch helper parametrization controls its own wording already. Assert the stable failure surface: validation_status == "failed", parse_error.startswith("decode:"), and the artifact support status.
🤖 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/unit/pipeline/test_ingest_batch.py` around lines 4118 - 4181, The
test_process_ingest_batch_public_route_persists_corrupt_input_readiness
assertions should not require exact decoder diagnostics from msgspec. Replace
the diagnostic parameter and exact parse_error equality with assertions that
validation_status is "failed" and parse_error starts with "decode:", while
preserving the existing terminal artifact support_status and lifecycle/status
assertions.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b64adb1554
ℹ️ 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".
| evidence_ref=None, | ||
| outcome_code=outcome_code, | ||
| remediation=None, | ||
| retryable=False, |
There was a problem hiding this comment.
When mark_raw_parse_failed handles a RawCASFrontierError, it records DEFERRED_CAS_FRONTIER through this function, but the durable classification_reason always says "retryable": false. CAS frontier conflicts are explicitly transient and the repair selector retries this carrier after the frontier clears, so operator-facing artifact inspection now reports the opposite disposition from the behavior that the carrier authorizes. Set this field according to the evidence kind—true for CAS-frontier deferrals while retaining false for terminal and hot-capture outcomes.
AGENTS.md reference: AGENTS.md:L114-L119
Useful? React with 👍 / 👎.
Problem: deferred partial captures and CAS frontier refusals were durable only as parse_error prose, while unknown exports were either excluded or collapsed into generic failure kinds. Repair eligibility therefore depended on unstable diagnostic wording. What changed: add typed deferred and terminal evidence kinds, classify Claude partial JSONL and unknown JSON inputs at the live ingest boundary, persist typed Codex CAS frontier failures, carry diagnostics through ingest results, expose the evidence in daemon status, and gate raw materialization and replay on durable deferred evidence. Verification: 14 focused managed tests passed; devtools verify --quick passed all 24 steps including mypy, rendering, layering, policy checks, and schema promotion audit. Compatibility: existing transient SQLite-lock and missing-blob retry receipts remain supported. Historical CAS rows with prose-only evidence are no longer authorized by repair; a backup-gated migration or re-observation must supply structured evidence before retry.
Problem: status selected the newest raw_artifacts observation even when it was unrelated to failure authority, and the unknown-provider exception boundary treated every ValueError as JSON decode evidence. Changes: constrain status projection to the newest artifact in RAW_FAILURE_EVIDENCE_KINDS, compare RawFailureEvidenceKind.value explicitly for lifecycle classification, and recognize only JSONDecodeError, UnicodeDecodeError, or PartialJsonStreamError at the live unknown-input boundary. Add regressions for newer unrelated artifacts, malformed JSON, invalid UTF-8, and semantic ValueError failures. Verification: 17 focused managed tests passed, covering the existing 14 plus three new exact regressions. devtools verify --quick passed all 24 steps. Operational scope: historical CAS failures that retain only prose parse_error text still require backup-gated migration or re-observation to receive structured deferred evidence. No live migration, repair actuator, push, or PR operation was performed.
Problem: the raw-materialization SQL EXISTS predicate authorized deferred failure rows, but its scalar failure-artifact projection selected the newest artifact of any kind. A newer unrelated artifact then failed the Python retryability recheck and dropped the authorized candidate.\n\nWhat changed: restrict the scalar projection to deferred raw-failure evidence kinds and add a regression with a newer unrelated CAS artifact.\n\nCompatibility: raw_artifacts coordinate uniqueness and upsert replacement already ensure a later terminal observation at the same origin, source path, and source index replaces stale deferred evidence. No migration is required.
Problem: the append failure regression patched the document parser, but Codex append records use the streaming parser. The intended injected failure was therefore bypassed and the test observed an unrelated empty-session rejection. What changed: patch the production streaming parser selected by the append route so the regression proves persisted failure state for the original exception.
Problem: historical CAS failures lost retry authorization, CAS evidence was gated to Codex, and repair/status paths could correlate or project evidence beyond the failed artifact coordinate. What changed: restore the exact legacy CAS values and typed exception prefix as bounded compatibility predicates, persist provider-neutral deferred CAS evidence for every RawCASFrontierError, require null-safe origin/path/index matches in repair selectors, and derive daemon sample kinds from the validated lifecycle projection. Compatibility: the old Codex-named artifact kind remains readable for retained rows. Removing the legacy prose and token bridges requires a backup-gated migration or re-observation receipt. Verification: 10 focused real-route tests passed; env PATH="$PWD/.venv/bin:$PATH" devtools verify --quick passed all 24 steps; git diff --check passed.
Require deferred repair and live replay decisions to match the typed artifact kind with its declared support status and exact raw coordinates. Preserve the existing historical prose bridges while rejecting contradictory evidence.
Preserve complete-line JSONL decode failures as a typed signal for strict unknown-source ingestion. Record terminal unknown decode evidence and return the retained observation as handled while leaving deferred authority failures retryable.
Derive lifecycle sample priority from the closed evidence kind and support-status vocabulary so newly valid typed rows remain visible ahead of unexplained failures. Keep status projection validation tied to the same typed evidence pairs.
Carry typed subprocess disposition fields through the batch outcome projection and retain the worker diagnostic at raw-state persistence. Keep validation-only failures free of parse-error or warning guesses when no diagnostic exists.
Correlate deferred lifecycle evidence with the failed raw artifact's raw_id, origin, source_path, and source_index in every repair selector. Add a production-route mutation proving neighboring observations cannot authorize replay.
Use one closed-kind, support-status, validation-status, and lifecycle validator for raw failure classification and daemon status projection. Preserve terminal decode receipts as handled and keep malformed evidence visibly unexplained with production-route regressions.
Terminalize exact-coordinate deferred CAS evidence after a successful parse or a later non-CAS failure. Keep neighboring observations untouched and prevent stale retry authority from requeueing unrelated failures.
Preserve the final pending record's decode count so strict unknown JSONL ingestion records terminal decode evidence for a malformed last line. Add a full live-ingest regression covering the final-record route. Signed-off-by: Sinity <ezo.dev@gmail.com>
Separate deferred lifecycle evidence from retained-replay authority. Hot JSONL captures remain deferred until a complete source observation, while only provider-neutral CAS frontier evidence authorizes replay of retained blobs. Signed-off-by: Sinity <ezo.dev@gmail.com>
Apply deferred CAS supersession when queued raw parse-success states flush after a positive commit batch. Exercise the production membership classification route with manage_transaction disabled. Signed-off-by: Sinity <ezo.dev@gmail.com>
Keep ordinary source-coordinate artifacts deduplicated while giving typed failure evidence an exact raw-coordinate carrier. Make parse-state and evidence updates one transaction, and cover lifecycle authority plus rollback through ArchiveStore.
Track blank physical lines separately from decoded records so strict malformed-record errors identify the actual offending line. Add a regression through the production JSON stream helper.
Problem: the lifecycle projection let a newer ordinary artifact mask a valid closed failure carrier, and the ordinary ingest-batch boundary dropped typed worker dispositions before source-tier finalization. What changed: lifecycle selection now considers only valid closed failure carriers. Terminal input dispositions are written as structured raw-artifact evidence using the retained raw's exact coordinate inside the existing source bulk transaction. Production-route regressions cover coexistence, typed fields, closed kind/status, readiness projection, retained identity, and rollback atomicity. Compatibility/migration: the v30 coordinate-carrier schema and ordinary artifact coexistence remain unchanged; no migration or production data operation is included. Ref 3897
Supersede deferred CAS authority during successful async batch persistence, keep terminal corrupt and decode evidence explainable under validation failure, enforce strict unknown-provider prefetch decoding, and exclude exact typed terminal failures from scheduled parse retry. Preserve retained raw bytes and the existing source-v30 migration shape.
Persist retained-raw CAS evidence with the first failure-state mutation, emit worker-disposition-v1 classification for reviewed corrupt input, and let exact terminal carriers veto legacy retry markers. Add production-route red twins for all three review findings.\n\nRef #3897
b64adb1 to
c9b9d42
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c9b9d42d06
ℹ️ 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".
| CREATE UNIQUE INDEX IF NOT EXISTS idx_raw_artifacts_failure_identity | ||
| ON raw_artifacts(raw_id, origin, source_path, source_index) | ||
| WHERE artifact_kind IN ( |
There was a problem hiding this comment.
After this migration, one raw coordinate may have both an ordinary observation and a typed failure carrier, but raw_failure_disposition_apply._validate_candidate() still joins every matching artifact and takes an unordered fetchone(). When an operator applies a reviewed terminal disposition to an unexplained raw that already has a malformed or deferred typed carrier, it can select the older ordinary row; _apply_candidate() then changes that row into another failure carrier and violates this new unique index, aborting the backup-gated recovery. Select the existing failure carrier deterministically or upsert a separate carrier while preserving the ordinary observation.
AGENTS.md reference: AGENTS.md:L114-L119
Useful? React with 👍 / 👎.
| if source_backend is not None: | ||
| await source_backend.supersede_deferred_cas_evidence(rid) |
There was a problem hiding this comment.
When a previously terminal raw is selected with force_reparse=True after parser support is added, a successful batch reaches this branch, but it supersedes only deferred CAS evidence and leaves the old terminal carrier intact; the synchronous mark_raw_parse_succeeded() path has the same limitation. The session initially appears in index.db, but after an index reset _raw_materialization_candidate_ids() excludes the raw because terminal evidence still exists, so the successfully recovered session disappears from the rebuilt archive. Retire all exact-coordinate lifecycle failure evidence atomically with a successful parse, not just CAS carriers.
AGENTS.md reference: AGENTS.md:L163-L166
Useful? React with 👍 / 👎.
1 participant
Summary
Close the reviewed raw-failure authority gaps at exact head
a74e948a0ba5b77a34581c40ec6b3e7616d2da5e, rebased onto currentorigin/masterincluding #3902. The implementation preserves retained raw bytes, provider-neutral CAS evidence, legacy selectors, and fail-closed replay semantics.Problem
The exact-head review found that successful CAS resolution could remain lifecycle-terminal, validation-failed evidence lacked a structural trusted disposition boundary, repair candidate selection could bypass validation authority, and a later parser failure could reuse an earlier failure carrier. Legacy migration fixtures also retained schema objects that did not exist at their declared historical versions.
Solution
Acceptance criteria
test_persist_batch_success_supersedes_deferred_cas_evidence_in_source_transactiontest_persist_batch_corrupt_input_remains_terminal_in_lifecycle; public-route readiness regressiontest_validation_failed_unsupported_terminal_evidence_remains_unexplainedand trusted-provenance validationtest_unknown_mixed_jsonl_prefetch_falls_back_to_strict_decodetest_parse_backlog_excludes_terminal_failure_authority_until_forced_reparsetest_parse_backlog_keeps_malformed_terminal_evidence_retryabletest_generic_parse_state_failure_retires_prior_failure_authority; 7 focused migration tests passedpolylogue-reindex-source-remediation; no live mutation was performedBead disposition
polylogue-dyicapolylogue-reindex-source-remediation.Verification
The full test suite was not run. A prior broader exploratory selection reported 150 passed and 14 failures; the migration-fixture failures were subsequently repaired, and the remaining identified failures were unrelated repair paths blocked by unreleased durable change trains for source versions 27 through 30. No Beads command, live production mutation, live migration, production reprocessing, or merge was run.
Review disposition
The exact-head review findings are covered by the preceding repair commits and
f2c3370b2/0c0e92807/a74e948a0. Adversarial iteration 1 found that historical fixtures preinstalled v30 raw-artifact indexes;a74e948a0restores the pre-v30 shape and the seven migration routes pass. The PR remains open, non-draft, and unmerged.