test(convergence): bind inferred residual proofs (#3899) - #3899
Conversation
Sinity
changed the title
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThe PR adds catalog-aware wire-support receipts, persists and replays their evidence in inferred corpus manifests, adds append capability checks for live providers, and corrects session and latency profile timestamp fallbacks. ChangesWire support and inferred corpus validation
Append capability receipts
Session insight source metadata
Estimated code review effort: 5 (Critical) | ~120 minutes Sequence Diagram(s)sequenceDiagram
participant Catalog
participant WireReceiptBuilder
participant ManifestCompiler
participant CampaignValidator
Catalog->>WireReceiptBuilder: package and element selections
WireReceiptBuilder-->>ManifestCompiler: persisted wire-support evidence
ManifestCompiler-->>CampaignValidator: bound manifest and receipt
CampaignValidator->>WireReceiptBuilder: replay seed and provider set
WireReceiptBuilder-->>CampaignValidator: current parser and route results
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
@codex review |
![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: bd3a088f80
ℹ️ 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 provider == "chatgpt" and package_version == "v1": | ||
| reasons[keyword] = ( | ||
| "ChatGPT v1 parser route retains normalized conversation fields but does not represent " | ||
| "export-only media metadata at this exact package selection" | ||
| ) | ||
| continue |
There was a problem hiding this comment.
When a ChatGPT v1 witness fails to exercise any schema keyword, this branch labels that keyword nonrepresentable without checking whether its path is actually export-only media metadata. A generator regression that omits a parser-relevant construct such as message role/body can therefore still produce construct_coverage.complete == True, allowing the wire receipt and bound inferred corpus to claim executable support despite the missing proof. Restrict this exemption to the specific schema paths the wire normalizer demonstrably discards.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Reviewed at exact head e0cff49. The complete parser witness gate now compares the full parser-relevant identity and content multiset, so a parser omission cannot obtain healthy evidence. The partial-output red-twin test exercises this boundary. This older thread is addressed by the current implementation.
| status="unsupported", | ||
| reason="live append route supports only Codex and Claude Code JSONL identity contracts", | ||
| ) | ||
| if provider == "codex" and not stable_session_identity: |
There was a problem hiding this comment.
When provider="claude-code" and stable_session_identity=False, this function returns supported, but the production route immediately calls _claude_code_tail_matches_existing_identity, which returns False when _existing_provider_session_id is absent and rejects the append. Thus capability consumers receive a positive receipt for an append the cited production route cannot execute; apply the stable-identity requirement to Claude Code as well.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Reviewed at exact head e0cff49. Claude stable identity remains required by the capability route, and the append path rejects an absent archived identity. The existing Claude capability and tail-matching tests cover that contract. This older thread is addressed by the current branch.
| support_entry=None, | ||
| support_receipt_bound=False, |
There was a problem hiding this comment.
When a manifest compiled with wire_support_receipt includes an explicitly unsupported route such as Antigravity, compilation records unsupported_wire_route, but campaign validation recomputes support with support_entry=None and classifies the same catalog element as provider_without_wire_format. The equality check below therefore rejects an otherwise valid bound manifest with schema-inference manifest unsupported decision changed, so all-provider campaign compilation and persisted campaign reads cannot use the new receipt binding. Revalidate against the bound receipt or persist enough receipt evidence to reproduce the original decision.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Reviewed at exact head e0cff49. Campaign validation uses the bound wire-support decision and preserves explicit unsupported-route outcomes. The existing campaign round-trip and route-drift tests cover this persisted authority boundary. This older thread is addressed by the current branch.
| source_updated_at=_iso_datetime(session.updated_at), | ||
| source_sort_key=float(session.updated_at.timestamp()) if session.updated_at is not None else None, | ||
| source_updated_at=_iso_datetime(profile.updated_at), | ||
| source_sort_key=float(source_sort_timestamp.timestamp()) if source_sort_timestamp is not None else None, |
There was a problem hiding this comment.
For a session with created_at but no updated_at, this row now records the creation timestamp in session_latency_profiles.source_sort_key, while _stamp_bundle_materialization still stamps the latency entry from bundle.profile_record.source_sort_key, which remains NULL. The readiness query compares that materialization stamp with sessions.sort_key_ms (which also falls back to created_at), so the latency materialization remains reported missing after every rebuild even though the latency row itself is fresh. Use the latency record's provenance when stamping the latency insight, and keep the profile/materialization fallback behavior aligned.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Reviewed at exact head e0cff49. The latency record retains the fallback sort key used for created-only sessions, and materialization uses that latency provenance. The fallback-sort-key and created-without-updated tests cover the readiness boundary. This older thread is addressed by the current branch.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8f6c536b12
ℹ️ 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".
| source_sort_timestamp = profile.updated_at or profile.created_at | ||
| source_updated_at = source_sort_timestamp.isoformat() if source_sort_timestamp else None |
There was a problem hiding this comment.
When a session has created_at but no updated_at, this now records the creation time as both source_updated_at and input_high_water_mark. That contradicts classify_profile_hwm_source's contract that a missing provider update timestamp has no source-anchored HWM, and it makes provenance depend on the materialization path because _large_session_profile_record_from_row still leaves these fields null for the same created-only input. Keep the creation fallback in source_sort_key for readiness, but preserve null update/HWM provenance when profile.updated_at is absent.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Reviewed at exact head e0cff49. Creation-only sessions retain a source sort key for readiness but do not invent provider update or high-water-mark provenance. The created-without-updated materialization test covers that distinction. This older thread is addressed by the current branch.
| if ( | ||
| raw_entry.get("provider"), | ||
| raw_entry.get("package_version"), | ||
| raw_entry.get("element_kind"), | ||
| ) == (provider, package_version, element_kind): | ||
| return _wire_support_entry_from_payload(raw_entry) |
There was a problem hiding this comment.
When a campaign manifest is read after parse_payload or a provider wire normalizer changes without a schema-catalog change, this reconstructs the old receipt's counts and evidence strings and treats them as current support. The later campaign validation regenerates only a synthetic payload; it never sends that payload through the current parser, and the serialized receipt carries no parser fingerprint to detect drift. A parser regression can therefore leave an old manifest executable even though its claimed production route now rejects the witness; bind the receipt to a checked parser fingerprint or rerun the parser witness during campaign validation.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Continuation at pushed head e78afd8 tightens the parser witness boundary: _parser_artifact_evidence now requires parsed message or block content to occur in the provider-specific raw evidence node as well as preserving identity binding. The focused ID-preserving invented-content red twin fails if that content binding is removed.
There was a problem hiding this comment.
Reviewed at exact head e0cff49. The current parser witness path retains provider-specific content and identity binding, and now also requires complete coverage of all expected parsed messages. The parser witness tests cover both boundaries. This older thread is superseded by the exact-head coverage repair.
| if provider in {Provider.CODEX, Provider.CLAUDE_CODE}: | ||
| identity = self._existing_provider_session_id(path) |
There was a problem hiding this comment.
For every valid Claude Code append, this newly added lookup opens the active index, attaches source.db, and resolves the archived native ID, but _claude_code_tail_matches_existing_identity immediately performs the identical lookup again. Thus the hot append path now doubles its SQLite connection/attach/query work per active Claude file, undermining the read-amplification behavior this path is intended to protect. Pass the already resolved identity into the tail matcher instead of discarding it for Claude.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Reviewed at exact head e0cff49. The existing Claude append route carries the resolved identity through its tail validation path, avoiding the duplicate lookup. The live read-amplification coverage remains in the branch. This older thread is addressed by the current branch.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1c5ea02b66
ℹ️ 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".
| parsed_session_count=0, | ||
| parsed_message_count=0, | ||
| construct_coverage=None, | ||
| if element is not None and not element.supported: |
There was a problem hiding this comment.
When a newly cataloged provider has only elements marked supported=False and no entry in PROVIDER_WIRE_ROUTES, this branch emits healthy unsupported-element rows and continues before adding the provider to missing_routes. Consequently WireSupportReceipt.complete can be true even though the provider has no explicit wire capability route, defeating the receipt's catalog-wide route census; perform the missing-route check at provider scope before skipping unsupported elements.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Reviewed at exact head e0cff49. Missing provider routes are classified at provider scope before unsupported catalog elements are skipped. The existing missing-route receipt test covers this census boundary. This older thread is addressed by the current branch.
| from polylogue.sources.dispatch import parse_payload, require_positive_conversational_evidence | ||
|
|
||
| catalog_providers = tuple(sorted(registry.list_providers())) # type: ignore[attr-defined] | ||
| catalog_providers = tuple(sorted(providers or registry.list_providers())) # type: ignore[attr-defined] |
There was a problem hiding this comment.
When a caller passes providers=() because filtering produced no providers, the truthiness fallback replaces that explicit selection with registry.list_providers() and generates an all-provider receipt. This can unexpectedly bind and revalidate unrelated parser routes in a scoped campaign; distinguish None (use the registry default) from an empty sequence (select nothing), as the manifest catalog helper already does.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Reviewed at exact head e0cff49. An explicit empty provider selection remains distinct from a None default and does not expand to the full registry. The existing provider-selection test covers this scope boundary. This older thread is addressed by the current branch.
|
@codex review |
|
Codex Review: Didn't find any major issues. Another round soon, please! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
|
@codex review |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
🤖 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/schemas/synthetic/wire_formats.py`:
- Around line 1084-1102: Deduplicate the provider filter when building
catalog_providers in the manifest compilation flow, while preserving the
existing sorted ordering. Update the expression around registry.list_providers()
and the caller-supplied providers so repeated provider names produce only one
iteration and one receipt entry.
In `@polylogue/sources/live/batch.py`:
- Around line 3547-3559: The provider-scoped capability lookup must not reuse an
archived identity from another provider at the same path. Update the identity
resolution around _existing_provider_session_id and its underlying
_existing_archive_session_native_id lookup to require the current provider
origin when matching archived sessions, while preserving the existing
Codex/Claude capability flow; add a regression test proving a wrong-origin
identity at the same path is rejected.
In `@tests/infra/inferred_corpus.py`:
- Around line 715-725: Replace the literal comparison in _unsupported_reason
with a named exported constant from wire_formats representing the
catalog-element unsupported reason. Import and use that constant when mapping
support_entry.reason, preserving the existing fallback and fail-closed refusal
classification.
- Around line 971-994: Extract the shared schema-level classification currently
duplicated in the expected-unsupported block and `_unsupported_reason` into one
helper, preserving `_unsupported_reason`’s precedence: unsupported element,
missing schema, missing wire format, then unsupported constructs. Call this
helper from both paths so an unloaded schema on a provider without a wire format
consistently yields `missing_schema`.
- Around line 997-1008: Update compile_inferred_corpus_manifest to build the
wire support-entry index once before the catalog loop, reusing the same
(provider, package_version, element_kind) key structure already used around the
existing index construction. In the live_unsupported calculation, replace the
per-entry _wire_support_entry_from_manifest scan with a lookup into that index
while preserving the current missing-entry behavior.
In `@tests/property/test_inferred_corpus_loop.py`:
- Around line 432-466: Mark
test_inferred_selection_debt_recovers_in_a_fresh_process with the
uses_real_clock marker, since record_convergence_debt and the fresh
child-process retry drain depend on wall-clock time. Keep the existing test flow
and assertions unchanged.
In `@tests/unit/core/test_synthetic_wire_support.py`:
- Around line 606-613: Update the receipt entry lookup in the test to match
provider, package_version, and element_kind, using selection.element_kind from
the coverage probe. Keep the existing coverage and receipt assertions unchanged
so they validate the same catalog element selected by select_synthetic_schema.
🪄 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: 35dc77b2-efeb-4bc3-abdb-7a642336192b
📒 Files selected for processing (16)
polylogue/schemas/packages.pypolylogue/schemas/runtime_registry.pypolylogue/schemas/synthetic/wire_formats.pypolylogue/sources/live/batch.pypolylogue/storage/insights/session/latency_profiles.pypolylogue/storage/insights/session/profiles.pypolylogue/storage/insights/session/rebuild.pytests/infra/convergence_harness.pytests/infra/inferred_corpus.pytests/integration/test_live_read_amplification.pytests/property/test_inferred_corpus_loop.pytests/unit/core/test_synthetic_wire_support.pytests/unit/daemon/test_convergence_stages.pytests/unit/schemas/test_inferred_corpus_manifest.pytests/unit/sources/test_live_batch_support.pytests/unit/storage/test_session_insight_refresh.py
|
Codex Review: Didn't find any major issues. Already looking forward to the next diff. Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c423b684fc
ℹ️ 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".
| assert archive_snapshot(archive_root, session_ids=session_ids) == before | ||
|
|
||
|
|
||
| @pytest.mark.uses_real_clock |
There was a problem hiding this comment.
This bare marker disables the clock guard without documenting why this fresh-process test genuinely requires host time, leaving future maintainers unable to assess whether the nondeterministic exemption is still necessary. Supply the required reason argument, for example describing the subprocess/debt-retry dependency, or remove the exemption if production-only clock reads suffice.
AGENTS.md reference: AGENTS.md:L503-L507
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in signed commit e78afd8. The marker now explains that fresh-process debt recovery crosses a subprocess wall-clock retry boundary. The focused test and final quick gate pass.
There was a problem hiding this comment.
Reviewed at exact head e0cff49. The real-clock marker names the fresh-process subprocess and debt-retry wall-clock boundary. The focused test and quick gate pass with that documented exemption. This older thread is addressed by the current branch.
|
@codex review |
|
@codex review Please review exact PR head |
|
@codex review |
|
Codex Review: Didn't find any major issues. Nice work! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e78afd8f27
ℹ️ 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 ( | ||
| artifact_sessions | ||
| and artifact_evidence | ||
| and artifact_validation_error is None | ||
| and parse_error is None | ||
| ): | ||
| payloads.extend(payload_items) |
There was a problem hiding this comment.
When a parser silently drops most generated messages or strips a field such as role while retaining one message's text, artifact_sessions and artifact_evidence remain nonempty, so this branch admits the entire input payload into construct coverage. Because artifact_coverage is computed from that input and witness health only requires positive parsed counts, the receipt can claim complete executable coverage for constructs the parser never materialized. Compare the parsed output against all generated conversational identities/content, or derive exercised coverage only from parser-bound evidence, before admitting the payload.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Reviewed at exact head e0cff49. The parser witness gate now requires the complete expected message identity and content multiset, and the partial-output red-twin test fails when that gate is removed. This current-head finding is addressed.
| stamp_input_high_water_mark_ms = _epoch_ms_or_none(latency.input_high_water_mark) | ||
| stamp_input_high_water_mark_source = latency.input_high_water_mark_source |
There was a problem hiding this comment.
For every ordinary session rebuild, build_session_latency_profile_record is called without input_high_water_mark or input_high_water_mark_source, so both latency fields are None; switching the materialization stamp to those fields erases the update HWM that this ledger previously inherited from the profile. get_session_latency_profile_insight reads provenance from this materialization row, so even an updated session is now exposed with an unknown input HWM and cannot participate correctly in HWM-based freshness checks. Pass the profile's HWM provenance into the latency record or retain those profile fields when stamping latency.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Reviewed at exact head e0cff49. The ordinary latency rebuild now passes the profile provider high-water mark and source into the latency record, and the real convergence test asserts both the row and materialization marker. This current-head finding is addressed.
| SELECT 1 | ||
| FROM sessions | ||
| WHERE origin = ? AND native_id = ? | ||
| FROM sessions AS s | ||
| JOIN source_tier.raw_sessions AS r ON r.raw_id = s.raw_id | ||
| WHERE s.origin = ? AND r.origin = ? AND s.native_id = ? |
There was a problem hiding this comment.
When Codex append recovery has an indexed native session but its sessions.raw_id is not joinable to source.db, this inner join rejects the session even though the file's session_meta supplies the same native ID. That breaks the repository's cursor-resynthesis and deferred-append paths: their _seed_native_session fixtures intentionally use raw_id="unrelated-raw-id", after which _append_plan now returns None instead of the asserted _AppendPlan. Keep the origin check on the indexed session for this fallback, or update the recovery contract and its existing fixtures to provide a matching raw row.
Useful? React with 👍 / 👎.
Problem: parser witness acceptance only required one artifact-bound message, so a parser returning a strict subset could retain aggregate coverage.\n\nWhat changed: compare the complete identity or content multiset derived from the route-owned parser nodes with parsed output, and add a real parse_payload mutation test that returns one of four messages.\n\nRef #3899.
Problem: Codex cursor resynthesis required an attached source-tier row even when the indexed origin and native id were the only durable identity evidence.\n\nWhat changed: keep the index origin/native-id lookup authoritative and add a real append-plan regression proving an unrelated raw id does not block recovery.\n\nRef #3899.
Problem: ordinary latency materialization omitted the profile high-water mark, then stamped latency freshness with null provenance.\n\nWhat changed: pass the materialized profile high-water mark into the latency record and assert the latency row and materialization marker through the daemon execute route.\n\nRef #3899.
Problem: parser witness coverage checking referenced the normalized parser payload after an exception path where it had not yet been assigned.\n\nWhat changed: initialize the payload from the validated artifact before dispatch-specific normalization.\n\nRef #3899.
Problem: parser payload normalization used a list-incompatible pop through the JSONValue union.\n\nWhat changed: remove the auxiliary native payload with a typed mapping projection that preserves mypy narrowing.\n\nRef #3899.
|
@codex review Please perform a fresh exact-head review of PR #3899 at commit e0cff49 only. Recheck the three residual findings in the current production diff and their anti-vacuity tests: complete parser witness coverage, Codex index-only identity authority with wrong-origin rejection, and provider high-water-mark propagation into ordinary latency materialization. Treat older inline threads as answered only if the exact current head supports that conclusion. Do not review an earlier commit. |
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
tests/infra/inferred_corpus.py (1)
876-891: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winBuild the lookup key with
wire_support_entry_keysemantics, not a hand-written tuple.Line 879 indexes entries with
wire_support_entry_key(entry). Line 889 looks the entry up with a literal(provider, package.version, element.element_kind)tuple. The two must stay in the same field order. Ifwire_support_entry_keychanges its tuple shape, this lookup misses every entry._compile_entrythen receivessupport_entry=Nonefor all selections and each row silently becomeswire_support_selection_unwitnessedinstead of failing loudly.Derive the key from one place so the producer and the consumer cannot drift.
♻️ Proposed refactor to share the key construction
+ def _selection_key(provider: str, package_version: str | None, element_kind: str | None) -> WireSupportEntryKey: + return wire_support_entry_key( + WireSupportEntry( + provider=provider, + status="unsupported", + reason=None, + package_version=package_version, + element_kind=element_kind, + schema_valid=None, + parsed_session_count=0, + parsed_message_count=0, + construct_coverage=None, + ) + )A simpler alternative is to export a
wire_support_selection_key(provider, package_version, element_kind)helper frompolylogue/schemas/synthetic/wire_formats.pyand to call it from bothwire_support_entry_keyand this call site.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/infra/inferred_corpus.py` around lines 876 - 891, Update the support-entry lookup in the surrounding compilation flow to use a shared key-construction helper with the same semantics as wire_support_entry_key, rather than the literal tuple. Reuse that helper from both wire_support_entry_key and the support_entries.get call so producer and consumer field ordering cannot diverge, while preserving the existing _compile_entry behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@polylogue/sources/live/batch.py`:
- Around line 3632-3633: Update _archive_has_native_session to accept the
current path, and constrain its Codex fallback query to indexed sessions whose
corresponding source.db raw_sessions.origin is codex-session. Pass the path from
the append capability flow so mismatched source origins cannot authorize the
indexed codex-id.
In `@tests/unit/daemon/test_convergence_stages.py`:
- Around line 1737-1739: Update the latency assertions in the convergence-stage
test to compare latency["input_high_water_mark"] against the materialization
record’s provider_hwm_ms using that record’s canonical timestamp representation,
while retaining the existing provider_ts source assertion.
---
Outside diff comments:
In `@tests/infra/inferred_corpus.py`:
- Around line 876-891: Update the support-entry lookup in the surrounding
compilation flow to use a shared key-construction helper with the same semantics
as wire_support_entry_key, rather than the literal tuple. Reuse that helper from
both wire_support_entry_key and the support_entries.get call so producer and
consumer field ordering cannot diverge, while preserving the existing
_compile_entry behavior.
🪄 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: c4f48bbf-c823-4d66-ad99-ddf071266009
📒 Files selected for processing (10)
polylogue/schemas/synthetic/wire_formats.pypolylogue/sources/live/batch.pypolylogue/storage/insights/session/rebuild.pytests/infra/inferred_corpus.pytests/property/test_inferred_corpus_loop.pytests/unit/core/test_synthetic_wire_support.pytests/unit/daemon/test_convergence_stages.pytests/unit/schemas/test_inferred_corpus_manifest.pytests/unit/sources/test_live_append_cursor_resynthesis.pytests/unit/sources/test_live_batch_support.py
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e0cff49ea8
ℹ️ 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 message.provider_message_id: | ||
| keys.append(f"id:{message.provider_message_id}") | ||
| elif isinstance(message.text, str) and message.text.strip(): | ||
| keys.append(f"text:{_normalise_evidence_text(message.text)}") |
There was a problem hiding this comment.
At exact head e0cff49, the fresh evidence is that messages with provider IDs contribute only id: keys, never their content. If a parser preserves every ID but drops the body from all but one message, the expected and observed counters still match, the one surviving body keeps artifact_evidence nonempty, and the input payload is admitted into construct coverage as healthy. Compare both identity and parsed content for each generated message so content-loss regressions cannot produce complete parser proof.
Useful? React with 👍 / 👎.
| parser_fingerprint="test-parser", | ||
| ) | ||
|
|
||
| assert processor._append_payload_for_provider(path, "codex", b'{"type":"event_msg"}\n') is None |
There was a problem hiding this comment.
For the ("codex-session", "claude-code-session") parameter, the indexed Codex identity is intentionally authoritative: the joined lookup misses, but the session-meta fallback finds codex-id in the index under codex-session, so _append_payload_for_provider returns a payload/identity tuple and this assertion fails. Keep the rejection case where the indexed origin is wrong, but assert acceptance when only the source-tier origin is mismatched or absent; otherwise the anti-vacuity test contradicts the newly restored index-only contract.
Useful? React with 👍 / 👎.
Problem: parser payload normalization used a list-incompatible pop through the JSONValue union.\n\nWhat changed: remove the auxiliary native payload with a typed mapping projection that preserves mypy narrowing.\n\nRef #3899.
## Summary Complete the Claude vintage pathology-proof continuation at the exact published head by closing three anti-vacuity gaps in source identity, candidate selection, and revision cardinality. The production invariant remains unchanged and still requires exactly two equivalent Claude revisions with one applied and one superseded-equivalent decision. ## Problem The cold exact-head review found that the collision fixture changed both identity predicates at once, the Claude candidate assertion copied an unchanged index, and the missing-revision twin covered only loss of a required row. Those shapes could leave one identity predicate, candidate-index binding, or the exact upper count bound unproven. ## Solution - Add independent same-origin/different-logical-key and different-origin/same-logical-key rows. Baseline selection and every Claude source mutation remain scoped by canonical `claude-ai-export` origin plus the exact `claude-ai:<native-id>` logical source key, and both decoys are asserted unchanged. - Add a Claude candidate red twin that mutates only the copied inactive index, keeps the active index green, and requires the real `verify_archive(..., index_path_override=..., checks=REINDEX_CROSS_TIER_ACCEPTANCE_CHECKS)` route to report the Claude candidate failure. - Add a third in-scope revision with the typed `superseded_prefix` decision. Its aggregate retains one hash, one applied row, one superseded-equivalent row, and one extra typed state while `COUNT(*)` becomes three, independently protecting the exact upper bound. The registered missing-revision deletion and real rebuild acceptance invocation remain present. ## Acceptance and review matrix | Finding or acceptance claim | Disposition | Evidence | | --- | --- | --- | | Codex finding `3742221322`: missing Claude revision red twin | Addressed | Signed commit `630e3857986692e70949ead19c22da1f9f10e5f6`; registered deletion remains in the semantic drift loop and the real rebuild acceptance invocation. | | Current finding `3742409019`: baseline must be green before each mutation | Addressed | Signed commit `de70b68ddbc95e2c707f5dd542645f8c38beca55`; each copied archive is verified immediately after collision insertion and before its drift mutation. | | Current finding `3742409022`: isolate origin and logical-source predicates | Addressed | Signed commit `de70b68ddbc95e2c707f5dd542645f8c38beca55`; orthogonal collision rows independently match one predicate and are asserted untouched. | | Current finding `3742409023`: protect the exact upper revision bound | Addressed | Signed commit `de70b68ddbc95e2c707f5dd542645f8c38beca55`; the third canonical row preserves all other aggregate state and makes the production invariant red at count three. | | Claude candidate acceptance must use the selected candidate index | Addressed in code, runtime blocked | The candidate-only Claude twin keeps the active index green and uses the real cross-tier candidate verifier. The full zoo route remains blocked before these assertions by the Antigravity residual below. | | Antigravity synthetic route | Explicit blocked residual | `UnsupportedSyntheticWireRouteError` at `polylogue/schemas/synthetic/selection.py:100`: Antigravity requires the language-server `.pb` adapter and source-path semantics, which generic JSON generation does not exercise. Ref #3899 remains the enabling scope. | | Live-export recovery | Partial and honestly bounded | The Claude pair remains sanitized measured-shape evidence with `live_export_recovered=false`; this PR makes no live-prevalence or live-current-corpus claim. | ## Bead disposition | Bead | Disposition | Remaining scope | | --- | --- | --- | | `polylogue-kmt1c` | Partial | The broader `polylogue-yazae` pathology-zoo fixture and manifest dependency remains open. No broader live Claude proof or live current-corpus support is claimed. | ## Verification `POLYLOGUE_PYTEST_WORKERS=1 env PATH="$PWD/.venv/bin:$PATH" devtools test tests/unit/infra/test_claude_vintage_live_proof.py` Result: 3 passed in 14.96 seconds through the real Claude parse, ingest, historical revision backfill, convergence, and receipt route. The receipt retained `live_export_recovered=false`. `POLYLOGUE_PYTEST_WORKERS=1 env PATH="$PWD/.venv/bin:$PATH" devtools test tests/unit/maintenance/test_archive_verification.py -k 'pathology_zoo_claude_vintage_registered_invariant_rejects_each_semantic_drift or pathology_zoo_claude_candidate_acceptance_uses_selected_index'` Result: both selected tests reached the known Antigravity fixture boundary and were blocked before their assertions by `UnsupportedSyntheticWireRouteError` at `polylogue/schemas/synthetic/selection.py:100`. No Antigravity support was fabricated. `direnv exec . devtools verify --quick` Result: all 24 steps passed at `de70b68ddbc95e2c707f5dd542645f8c38beca55`, including format, lint, mypy, rendering, layering, policy, and schema promotion checks. Run id: `20260809T052439Z-quick-2665560-d64042d3`. `git diff --check` Result: clean. `git verify-commit de70b68` Result: good GitHub signer binding for `ezo.dev@gmail.com`. ## Review disposition The three current exact-head findings are addressed in signed commit `de70b68ddbc95e2c707f5dd542645f8c38beca55`. The prior missing-revision finding remains addressed in signed commit `630e3857986692e70949ead19c22da1f9f10e5f6`. A fresh exact-head Codex review is requested after this publication. <!-- polylogue-pr-scope:v1 { "assigned_beads": [ "polylogue-kmt1c" ], "beads_digest": "9043243a2aaa4609a5e8e9a05813b84e4036e4248cbcb33daf47691f51bf8d73", "dispositions": [ { "bead_id": "polylogue-kmt1c", "disposition": "partial", "evidence": [ { "kind": "commit", "ref": "de70b68ddbc95e2c707f5dd542645f8c38beca55" }, { "kind": "test", "ref": "tests/unit/maintenance/test_archive_verification.py" }, { "kind": "command", "ref": "devtools verify --quick" } ], "successors": [ "polylogue-yazae" ] } ], "head_sha": "de70b68ddbc95e2c707f5dd542645f8c38beca55", "scope_digest": "ae5ccf25f97295b5d8d6b509a19c0f7219053b95453d3b95c70cb0faf1139e3d", "version": 1 } -->
e0cff49 to
1e559a9
Compare
|
@codex review Please review the exact pushed head |
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
tests/infra/inferred_corpus.py (1)
401-409: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDerive
valid_reasonsfrom theUnsupportedCorpusReasonalias.The literal set at Lines 401-409 duplicates the
UnsupportedCorpusReasonmembers declared at Lines 48-56. This PR had to edit both lists. If a future change edits only one list, deserialization either rejects a valid reason or accepts an unmodelled one. Usetyping.get_argsso one declaration remains authoritative.♻️ Proposed refactor
- valid_reasons = { - "provider_without_wire_format", - "wire_support_selection_unwitnessed", - "wire_support_receipt_incomplete", - "unsupported_wire_route", - "unsupported_element", - "missing_schema", - "unsupported_json_schema_construct", - } + valid_reasons = set(get_args(UnsupportedCorpusReason))Add the import:
from typing import get_args🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/infra/inferred_corpus.py` around lines 401 - 409, Replace the duplicated literal `valid_reasons` set with a value derived from the `UnsupportedCorpusReason` alias using `typing.get_args`; add the `get_args` import and keep `UnsupportedCorpusReason` as the single authoritative declaration.tests/unit/sources/test_live_batch_support.py (1)
5850-5851: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winComplete the truncated comment sentence.
The comment at Line 5849 ends mid-clause with "under
#2718's original", and Line 5850 starts a new sentence. The trailing words were dropped during the edit, so the rationale no longer reads as a complete statement.📝 Proposed fix
- # production session that hit this guard under `#2718`'s original + # production session that hit this guard under `#2718`'s original + # wording. # The structured evidence row, not the diagnostic wording, is the # retry authorization.🤖 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 5850 - 5851, Complete the truncated comment in the retry-authorization test by restoring the missing words after “under `#2718`’s original” so the sentence is grammatically complete and clearly explains the structured evidence row’s role. Leave the test logic and the following diagnostic-wording sentence unchanged.
🤖 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/schemas/synthetic/wire_formats.py`:
- Around line 1385-1386: Update the validation_error composition in the artifact
coverage and try/exception paths. When
_parser_artifact_has_complete_message_coverage returns false, record a witness
validation error explaining the coverage refusal before clearing
artifact_evidence. In the exception handling around parser_errors, preserve the
route exception text and combine it with existing parser errors instead of
overwriting validation_error, so both causes remain in the receipt.
In `@polylogue/sources/live/batch.py`:
- Around line 389-396: Update the supported-provider check in
append_capability_receipt to derive its values from the Provider enum members
for Codex and Claude Code, rather than duplicating string literals. Keep the
existing unsupported receipt behavior unchanged, and ensure the derived set
remains aligned with the Provider-based gate used by the caller.
In `@tests/infra/inferred_corpus.py`:
- Around line 969-985: Build a dictionary of manifest entries keyed by
wire_support_key(...) once before the loop over catalog_entries, then retrieve
live_entry from that index instead of scanning manifest.entries with next.
Preserve the existing missing-entry ValueError behavior when the lookup has no
match.
- Around line 1077-1080: Update the validation around current.to_dict() and
persisted to identify which receipt entry keys differ, and include those
differing keys in the ValueError message. Preserve the existing full-payload
comparison and failure behavior while making the error actionable without
requiring a manual diff.
- Around line 953-965: Update the expected-decision logic around the
provider_entries handling so the no-schema-blocking path returns "committed"
only when this provider has at least one executable spec; a provider whose
entries are all wire-authority refusals must not be committed. Preserve the
existing "nonrepresentable" and "unsupported" branches, and confirm the intended
non-committed decision value for the zero-executable-selection state before
implementing the change.
In `@tests/unit/sources/test_live_batch_support.py`:
- Around line 82-91: Extend the receipt contract assertions around
receipt.to_dict() to always verify the emitted operation value for
append_prefix. Add an explicit assertion for the unsupported-provider case
(provider chatgpt with stable_session_identity true) using the implementation’s
provider-refusal reason, while preserving the existing unstable-session
assertion for codex and claude-code.
---
Outside diff comments:
In `@tests/infra/inferred_corpus.py`:
- Around line 401-409: Replace the duplicated literal `valid_reasons` set with a
value derived from the `UnsupportedCorpusReason` alias using `typing.get_args`;
add the `get_args` import and keep `UnsupportedCorpusReason` as the single
authoritative declaration.
In `@tests/unit/sources/test_live_batch_support.py`:
- Around line 5850-5851: Complete the truncated comment in the
retry-authorization test by restoring the missing words after “under `#2718`’s
original” so the sentence is grammatically complete and clearly explains the
structured evidence row’s role. Leave the test logic and the following
diagnostic-wording sentence unchanged.
🪄 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: dad58f45-04b6-4223-a848-f43c6f20b727
📒 Files selected for processing (6)
polylogue/schemas/synthetic/wire_formats.pypolylogue/sources/live/batch.pytests/infra/inferred_corpus.pytests/unit/daemon/test_convergence_stages.pytests/unit/sources/test_live_batch_support.pytests/unit/storage/test_session_insight_refresh.py
| schema_blocking_reasons = tuple( | ||
| entry.unsupported.reason | ||
| for entry in provider_entries | ||
| if entry.unsupported is not None and entry.unsupported.reason not in _WIRE_AUTHORITY_ONLY_REASONS | ||
| ) | ||
| if any(entry.spec is not None for entry in provider_entries): | ||
| expected_decision = "committed" | ||
| elif provider_entries and all( | ||
| entry.unsupported is not None and entry.unsupported.reason == "unsupported_json_schema_construct" | ||
| for entry in provider_entries | ||
| ): | ||
| elif not schema_blocking_reasons: | ||
| expected_decision = "committed" if coverage.provider in PROVIDER_WIRE_FORMATS else "unsupported" | ||
| elif all(reason == "unsupported_json_schema_construct" for reason in schema_blocking_reasons): | ||
| expected_decision = "nonrepresentable" | ||
| else: | ||
| expected_decision = "unsupported" |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift
A provider with only wire-authority refusals still yields a committed coverage decision.
_WIRE_AUTHORITY_ONLY_REASONS removes wire_support_receipt_incomplete, wire_support_selection_unwitnessed, and unsupported_wire_route from schema_blocking_reasons. Consider a provider that is present in PROVIDER_WIRE_FORMATS and whose every element is refused with wire_support_receipt_incomplete.
- No entry has a
spec, so Line 958 does not apply. schema_blocking_reasonsis empty, so Line 960 applies.- Line 961 sets
expected_decision = "committed".
The handoff then asserts a committed coverage decision for a provider with zero executable selections. Line 929 only checks that the manifest has at least one supported spec across all providers, so one healthy provider hides the vacuum. This defeats the anti-vacuity goal that the wire receipt introduces.
Require an executable spec before the committed result on the no-schema-blocking path.
🐛 Proposed fix
if any(entry.spec is not None for entry in provider_entries):
expected_decision = "committed"
elif not schema_blocking_reasons:
- expected_decision = "committed" if coverage.provider in PROVIDER_WIRE_FORMATS else "unsupported"
+ # No schema-level refusal exists, but no element is executable either.
+ # Only claim "committed" when the provider has no entries at all to
+ # refuse; otherwise the refusal is wire-authority-only and the
+ # provider has nothing to execute.
+ expected_decision = (
+ "committed"
+ if coverage.provider in PROVIDER_WIRE_FORMATS and not provider_entries
+ else "unsupported"
+ )Confirm the intended decision value for this state before applying the change.
📝 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.
| schema_blocking_reasons = tuple( | |
| entry.unsupported.reason | |
| for entry in provider_entries | |
| if entry.unsupported is not None and entry.unsupported.reason not in _WIRE_AUTHORITY_ONLY_REASONS | |
| ) | |
| if any(entry.spec is not None for entry in provider_entries): | |
| expected_decision = "committed" | |
| elif provider_entries and all( | |
| entry.unsupported is not None and entry.unsupported.reason == "unsupported_json_schema_construct" | |
| for entry in provider_entries | |
| ): | |
| elif not schema_blocking_reasons: | |
| expected_decision = "committed" if coverage.provider in PROVIDER_WIRE_FORMATS else "unsupported" | |
| elif all(reason == "unsupported_json_schema_construct" for reason in schema_blocking_reasons): | |
| expected_decision = "nonrepresentable" | |
| else: | |
| expected_decision = "unsupported" | |
| schema_blocking_reasons = tuple( | |
| entry.unsupported.reason | |
| for entry in provider_entries | |
| if entry.unsupported is not None and entry.unsupported.reason not in _WIRE_AUTHORITY_ONLY_REASONS | |
| ) | |
| if any(entry.spec is not None for entry in provider_entries): | |
| expected_decision = "committed" | |
| elif not schema_blocking_reasons: | |
| # No schema-level refusal exists, but no element is executable either. | |
| # Only claim "committed" when the provider has no entries at all to | |
| # refuse; otherwise the refusal is wire-authority-only and the | |
| # provider has nothing to execute. | |
| expected_decision = ( | |
| "committed" | |
| if coverage.provider in PROVIDER_WIRE_FORMATS and not provider_entries | |
| else "unsupported" | |
| ) | |
| elif all(reason == "unsupported_json_schema_construct" for reason in schema_blocking_reasons): | |
| expected_decision = "nonrepresentable" | |
| else: | |
| expected_decision = "unsupported" |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tests/infra/inferred_corpus.py` around lines 953 - 965, Update the
expected-decision logic around the provider_entries handling so the
no-schema-blocking path returns "committed" only when this provider has at least
one executable spec; a provider whose entries are all wire-authority refusals
must not be committed. Preserve the existing "nonrepresentable" and
"unsupported" branches, and confirm the intended non-committed decision value
for the zero-executable-selection state before implementing the change.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1e559a992f
ℹ️ 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".
| WHERE source_path = ? AND origin <> ? | ||
| LIMIT 1 |
There was a problem hiding this comment.
When the row for this path has the expected codex-session source origin but its joined sessions row has a different origin, this query reports no conflict; if a separate indexed Codex session happens to have the same native ID, _archive_has_native_session then succeeds and the append is assigned to that unrelated session. The fresh evidence at this head is that the new mixed-origin test covers each origin permutation only without that separate valid Codex row, so adding one flips the intended rejection to acceptance. Treat any same-path source row as requiring the joined origin match after _existing_archive_session_native_id fails, rather than checking only the source-tier origin.
Useful? React with 👍 / 👎.
|
@codex review Please review exact pushed head |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d11b06bdab
ℹ️ 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 insight_type == "latency": | ||
| stamp_source_updated_at_ms = _epoch_ms_or_none(latency.source_updated_at) | ||
| stamp_source_sort_key_ms = ( | ||
| int(latency.source_sort_key * 1000) if latency.source_sort_key is not None else None |
There was a problem hiding this comment.
For millisecond timestamps whose floating-point seconds representation falls just below the exact value (for example, 1097440214212 becomes a float that stamps as 1097440214211), int(... * 1000) truncates the latency materialization key by one millisecond. The readiness queries compare this stamp exactly with sessions.sort_key_ms, so affected sessions remain stale and are rebuilt on every convergence pass. Preserve the original integer millisecond key or round the converted value instead of truncating it.
Useful? React with 👍 / 👎.
| gate_receipt_path=gate_receipt_path, | ||
| archive_root=archive_root, | ||
| ) | ||
| _validate_current_wire_support_route(manifest, registry) |
There was a problem hiding this comment.
When build_inferred_corpus_convergence_handoff receives a Path with campaign_mode=True, read_inferred_corpus_manifest already invokes _validate_inference_handoff, and the handoff builder invokes it again immediately afterward. This newly added call therefore rebuilds and parses the complete wire-support receipt twice; because each supported selection generates the bounded coverage corpus (up to 128 witnesses), campaign startup and the associated property tests perform hundreds of redundant parser runs. Skip the second handoff validation for a path that was already read in campaign mode, while retaining it for in-memory manifests.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8bf4aa0d3a
ℹ️ 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".
| else: | ||
| values = () |
There was a problem hiding this comment.
Fresh evidence at this head is that the new content check extracts raw message text only for ChatGPT and Codex; for Claude AI and Claude Code it returns an empty set, causing _parser_artifact_messages_have_artifact_bound_content to skip every identified message. A parser regression can therefore preserve all UUIDs, erase most message bodies, and retain only one body to satisfy artifact_evidence, while the receipt still reports complete coverage. Extract and compare provider-specific content for every supported route.
Useful? React with 👍 / 👎.
| if support_entry is not None: | ||
| if support_entry.status == "unsupported": | ||
| reason: UnsupportedCorpusReason = ( | ||
| "unsupported_element" | ||
| if support_entry.reason == CATALOG_ELEMENT_UNSUPPORTED_REASON | ||
| else "unsupported_wire_route" | ||
| ) |
There was a problem hiding this comment.
When a catalog provider has no PROVIDER_WIRE_ROUTES entry, the receipt correctly records it in missing_routes and sets complete false, but its per-selection entries still have status="unsupported". This branch converts those entries into ordinary unsupported_wire_route decisions before considering receipt health, and campaign validation merely regenerates the same incomplete receipt, so a campaign with other executable providers is admitted despite the failed route census. Preserve missing-route entries as wire_support_receipt_incomplete or reject an incomplete bound receipt.
Useful? React with 👍 / 👎.
| except sqlite3.Error: | ||
| return False |
There was a problem hiding this comment.
If the source-tier attach or conflict query raises sqlite3.Error, returning False treats an unknown ownership state as conflict-free. The caller can then accept the global Codex native-ID fallback, so when an unrelated indexed Codex session shares the header ID, a transient read/attach failure can assign this path's append to that session. Return a conflict/unknown result that rejects the append instead of failing open.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 24b7d4a1d4
ℹ️ 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 message.provider_message_id: | ||
| keys.append(f"id:{message.provider_message_id}") | ||
| elif isinstance(message.text, str) and message.text.strip(): | ||
| keys.append(f"text:{_normalise_evidence_text(message.text)}") |
There was a problem hiding this comment.
When a provider parser preserves message IDs and bodies but assigns the wrong role or material origin, this projection—and the later content check—still reports complete coverage because neither compares authoredness with the raw node. Fresh evidence at this head is that the supposedly complete output key contains only provider_message_id or text, so such a regression can admit a healthy campaign while corrupting authored-user counts and related experiments; include the expected role/material-origin semantics in the parser witness comparison.
AGENTS.md reference: AGENTS.md:L65-L70
Useful? React with 👍 / 👎.
| observed_texts = { | ||
| _normalise_evidence_text(text) | ||
| for text in (message.text, *(block.text for block in message.blocks)) | ||
| if isinstance(text, str) and _normalise_evidence_text(text) |
There was a problem hiding this comment.
When a Claude Code or Codex parser retains each message and its block text but drops a tool ID, tool-use block, or provider-reported error/exit metadata, this text-only projection remains identical and the raw input is still admitted into construct coverage even though its action semantics never survived parsing. This is separate from authoredness/body coverage: actions depend on joining tool-use and tool-result blocks by tool ID, and structural outcome fields are load-bearing, so compare parsed block kinds, tool IDs, and outcome fields with their raw nodes before declaring the witness healthy.
AGENTS.md reference: AGENTS.md:L71-L75
Useful? React with 👍 / 👎.
Problem: synthetic parser receipts only compared message identity and text, allowing authoredness and structured tool semantics to disappear without invalidating a witness.\n\nWhat changed: canonical raw witnesses now compare role/material origin plus Claude Code and Codex tool identity and provider-reported outcomes. Focused production-dispatch regressions corrupt each field and require the receipt to fail.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 833a169fe5
ℹ️ 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".
| index == 0 | ||
| and not _parser_artifact_has_complete_semantic_coverage( | ||
| artifact_sessions, provider, parser_payload |
There was a problem hiding this comment.
When a parser regression affects only a non-baseline coverage artifact, this index == 0 guard skips _parser_artifact_has_complete_semantic_coverage, yet that artifact can still contribute schema keywords to the final construct coverage. Fresh evidence at this head is that only the baseline now checks authoredness and structured tool semantics, so a parser may drop or corrupt a role, material_origin, tool ID, or tool outcome exercised exclusively by a later schema branch while the receipt remains complete; apply the semantic comparison to every admitted witness.
AGENTS.md reference: AGENTS.md:L65-L70
Useful? React with 👍 / 👎.
| expected = _parser_artifact_expected_message_keys(provider, payload) | ||
| observed = _parser_artifact_message_keys(sessions) | ||
| return ( | ||
| bool(expected) | ||
| and Counter(expected) == Counter(observed) | ||
| and _parser_artifact_messages_have_artifact_bound_content(sessions, provider, payload) |
There was a problem hiding this comment.
When a parser preserves every message ID and body but assigns them to the wrong provider_session_id—for example, another UUID that occurs elsewhere in the raw payload—_parser_artifact_evidence accepts that session because its identity is merely any payload string, while this new check flattens messages across all sessions into one Counter. The receipt can therefore remain complete even though ingestion will derive the wrong sessions.session_id; compare the expected provider session identity and per-session message grouping rather than only the archive-wide message multiset.
AGENTS.md reference: AGENTS.md:L55-L59
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@polylogue/sources/live/batch.py`:
- Around line 3724-3728: Update the sqlite3.Error handler in the ownership-view
query to emit a warning before returning True, including enough context to
identify the unavailable ownership view and distinguish it from an origin
conflict. Preserve the existing fail-closed return behavior and follow the
logging pattern used by _compact_superseded_raw_snapshots or _converge_paths.
In `@tests/unit/sources/test_live_batch_support.py`:
- Around line 2564-2569: Strengthen the test around _append_payload_for_provider
by isolating the sqlite failure specifically to the ownership lookup used by
_archive_has_native_session, while allowing the other sqlite-backed checks in
_existing_provider_session_id to execute normally. Ensure the setup would
otherwise admit the existing native identity, so the assertion that the append
returns None specifically verifies the fail-closed behavior of
_archive_has_native_session.
🪄 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: c1874c2b-56ac-4441-987e-03be55fb23d8
📒 Files selected for processing (8)
polylogue/schemas/synthetic/wire_formats.pypolylogue/sources/live/batch.pypolylogue/storage/insights/session/rebuild.pytests/infra/inferred_corpus.pytests/unit/core/test_synthetic_wire_support.pytests/unit/daemon/test_convergence_stages.pytests/unit/schemas/test_inferred_corpus_manifest.pytests/unit/sources/test_live_batch_support.py
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 78b45f8c9f
ℹ️ 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 message.provider_message_id in raw_texts_by_identity and ( | ||
| not observed_texts or not observed_texts & raw_texts_by_identity[message.provider_message_id] | ||
| ): |
There was a problem hiding this comment.
When a generated message contains multiple plain text parts or blocks, a parser can retain the message ID and one part while dropping the others, because this condition accepts any nonempty intersection between observed and expected text. Fresh evidence at this head is that the identity and semantic counters use only the provider message ID when present, so no later check accounts for the missing segments; the artifact can therefore still be admitted into construct coverage as executable parser proof. Compare all parser-relevant expected content per identity, preferably as a multiset, rather than requiring only one matching segment.
Useful? React with 👍 / 👎.
| current = build_wire_support_receipt( | ||
| registry=registry, | ||
| seed=witness_seed, | ||
| providers=tuple(cast(str, provider) for provider in raw_providers), |
There was a problem hiding this comment.
When an all-provider campaign manifest is read after the live registry gains a provider, this rebuild is restricted to the old receipt's catalog_providers, so the new provider is never checked for a route or package selection. The outer persisted-manifest validation also derives its provider scope from the manifest entries, meaning the stale campaign is accepted and still appears catalog-complete despite omitting the new provider entirely. Preserve whether the receipt was compiled with the registry-default scope and, for that case, compare against or rebuild from the current registry.list_providers() set.
Useful? React with 👍 / 👎.
1 participant
Summary
Close the remaining convergence-proof review gaps. Every supported synthetic parser witness now validates its identity, every parser-owned text segment, authoredness, and structured tool semantics when present. The receipt also verifies that messages remain in the provider session asserted by that exact raw artifact.
Problem
The prior receipt could preserve IDs and one text segment while losing another, lose authoredness or provider tool outcomes outside the baseline artifact, or accept a correct message set under a foreign provider session key. A persisted all-provider receipt could also omit a subsequently cataloged provider during revalidation, and an unavailable Codex ownership view had no operational signal.
Solution
The synthetic receipt compares raw-wire role and material-origin expectations for every admitted artifact, requires every parser-owned raw text segment through the production text/block representation, compares structured tool kind, ID, error state, and exit code, and enforces the provider-specific session identity for each one-session artifact. It persists whether a receipt used the registry-default provider census, then re-censuses current providers only for that scope. Codex ownership-view SQLite errors remain fail-closed and now emit a warning. Negative tests mutate the real dispatch result for both baseline and coverage artifacts, including re-homing messages to a different raw identity and dropping one segment while retaining its message identity.
Verification
POLYLOGUE_PYTEST_WORKERS=1 direnv exec . devtools test tests/unit/core/test_synthetic_wire_support.py -k 'supported_routes_validate_selected_schema_and_parser_entry_point or authoredness_loss_is_not_accepted_for_every_supported_route or tool_identity_loss_is_not_accepted or structured_tool_outcome_loss_is_not_accepted or rejects_messages_rehomed_to_another_raw_identity': 24 passed.POLYLOGUE_PYTEST_WORKERS=1 direnv exec . devtools test tests/unit/schemas/test_inferred_corpus_manifest.py -k 'rejects_a_bound_receipt_with_a_missing_catalog_route': 1 passed.POLYLOGUE_PYTEST_WORKERS=1 direnv exec . devtools test tests/unit/sources/test_live_batch_support.py -k 'rejects_global_fallback_when_ownership_query_errors': 1 passed.POLYLOGUE_PYTEST_WORKERS=1 direnv exec . devtools test tests/unit/sources/test_live_batch_support.py tests/unit/core/test_synthetic_wire_support.py tests/unit/schemas/test_inferred_corpus_manifest.py -k 'codex_append_identity_rejects_global_fallback_when_ownership_query_errors or parser_witness_segment_loss_is_not_accepted_with_preserved_message_identity or default_scope_campaign_rejects_a_new_provider_during_receipt_revalidation or explicit_scope_campaign_does_not_re_census_unselected_provider or parser_witness_content_loss_is_not_accepted_with_preserved_ids_for_every_supported_route or campaign_read_rejects_wire_route_drift': 10 passed at09fd3e689.devtools verify --quick: 24 checks passed at final head78b45f8c9.Bead disposition
polylogue-rrxe4polylogue-un60n,polylogue-t0m73remain open