Skip to content

feat: isolate typed clean-slate vNext bindings - #46

Closed
Pigbibi wants to merge 2 commits into
mainfrom
codex/qar-vnext-n1-typed-binding-reslice
Closed

feat: isolate typed clean-slate vNext bindings#46
Pigbibi wants to merge 2 commits into
mainfrom
codex/qar-vnext-n1-typed-binding-reslice

Conversation

@Pigbibi

@Pigbibi Pigbibi commented Jul 14, 2026

Copy link
Copy Markdown
Contributor

QAR-N1 typed-binding clean-slate reslice

Fresh implementation from origin/main@d13dad1cfd2680cfdf9c96bc4687fc3d32ffc015. PR #44/#45 are closed evidence only; no branch, commit, compatibility code, or legacy fallback is reused.

Type-dispatch source of truth

VNextIdentityBinding is a distinct runtime type from legacy V3IdentityBinding. The vNext parser produces only the typed binding, the allocator consumes/returns only it, and PublicationEntry dispatches by actual binding type. There is no caller identity_namespace routing flag and no filename-based dispatch. vNext wire entries carry exact binding_namespace=qar_vnext and binding_version=1; legacy/unknown namespace/version are rejected.

Contract matrix

  • Exact vNext index top-level keys: namespace, schema_version, entries.
  • Shared strict validator covers period/as_of/cadence, schema-contract pairing via contract_version_for_schema, algorithm versions, digests, class/canonical/status, all target grammar, and safe integer display order.
  • Cadence-aware JSON/HTML/MD/manifest; canonical unsuffixed; variants use the complete artifact digest suffix.
  • Optional attachment omission is distinct from explicit null; present values must be valid basename strings.
  • EXACT/CURRENT/HISTORICAL allocation, empty current bootstrap, exact immutable reuse, changed-artifact variant, historical canonical requirement.
  • Attachment/display policy, 0..2**53-1 display boundaries, collision detection, permutation stability, deterministic round-trip.
  • Legacy type cannot enter vNext index; vNext type cannot be decoded as legacy; mixed PublicationPlan entries dispatch independently.
  • Forged metadata, legacy names/classes, unknown namespace/version, schema mismatch, digest mismatch and malformed report adversarial cases.

Scope boundary

Pure identity/index/PublicationPlan foundation only. No migration, compatibility, publisher, build, workflow, Pages, producer, filesystem, network or other production I/O.

Validation

  • Focused typed-binding tests: 23 passed
  • Full pytest: 288 passed
  • Targeted ruff: passed
  • compileall: passed
  • diff-check: passed
  • Full ruff: 28 pre-existing out-of-scope violations in scripts/gate_codex_app_review.py.

Co-Authored-By: Codex <noreply@openai.com>
@github-actions

github-actions Bot commented Jul 14, 2026

Copy link
Copy Markdown

🤖 Codex PR Review

🚫 Merge blocked: 1 serious issue(s) found in high-risk files

🚫 Blocking Issues

These issues must be fixed before this PR can be merged:

1. 🟠 [HIGH] Logic in src/quant_advisor_research/vnext_binding.py

validate_vnext_binding() only accepts semantic_fingerprint_version and artifact_integrity_version when they exactly match the process's current FINGERPRINT_VERSION / ARTIFACT_INTEGRITY_VERSION. That means a future algorithm-version bump will make every previously written vNext index fail to parse, even if binding_namespace, binding_version, and schema_version are still valid. This turns normal version rotation into a wire-compatibility/read-availability break for stored production data. (line 155)

Suggestion: Validate these fields against an explicit allowlist keyed by the supported vNext schema/binding version, or otherwise preserve support for previously emitted algorithm-version values when decoding existing indices.

ℹ️ Other Findings

1. 🟡 [MEDIUM] Logic in src/quant_advisor_research/vnext_identity.py

_validate_index() checks filename collisions and canonical conflicts, but it never enforces any per-period display policy. Two bindings for the same period_key can both parse with display_primary=true or reuse the same display_order, and the index will still round-trip as valid. That lets forged/corrupted indices carry ambiguous primary selection and ordering metadata into later publication planning. (line 98)

Suggestion: While validating each period, track whether a primary binding has already been assigned and which display_order values have been used; reject duplicate orders or multiple primaries for the same period_key.


Review by Codex PR Review bot • PR

Co-Authored-By: Codex <noreply@openai.com>
@Pigbibi

Pigbibi commented Jul 14, 2026

Copy link
Copy Markdown
Contributor Author

Concentrated closure evidence (closure_count=1/max=1):

  • PublicationPlan payload serialization now dispatches by concrete binding type. Legacy V3IdentityBinding retains its legacy payload; VNextIdentityBinding delegates to the typed vNext payload including binding_namespace/binding_version. Mixed-plan regression asserts both paths.
  • CURRENT_MANDATORY exact reuse now requires stored display placement to match the request, so the returned immutable binding and PublicationPlan cannot disagree. Changed display is rejected with identity_reuse_mismatch; matching exact reuse remains covered.

Validation: focused typed-binding 23 passed; full pytest 289 passed; targeted ruff passed; compileall passed; diff-check passed. Full ruff retains only 28 pre-existing violations in scripts/gate_codex_app_review.py.

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: c82bda9c1b

ℹ️ 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".


candidate: SelectedCandidate
binding: V3IdentityBinding
binding: V3IdentityBinding | VNextIdentityBinding

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Bound vNext publication display order

When callers construct PublicationEntry/PublicationPlan directly with a VNextIdentityBinding rather than going through allocate_vnext_identity, this newly admitted binding type bypasses vNext's MAX_SAFE_JSON_INTEGER check: _validate_entry still only rejects negative display_order. A plan can therefore carry values like 9007199254740992 even though the vNext contract requires display_order to stay in 0..2**53-1 at every in-memory boundary, which can hand downstream JSON/Page consumers an unsafe ordering value.

Useful? React with 👍 / 👎.

Comment on lines +274 to +278
entry = None
plan = None
if context.mode is AllocationMode.CURRENT_MANDATORY:
entry = PublicationEntry(candidate, binding, PublicationRole.MANDATORY_CURRENT, display.display_primary, display.display_order)
plan = PublicationPlan((entry,))

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Return recovered entries for historical allocations

When callers use AllocationContext.historical_recovery() for an existing historical artifact, this path returns AllocationResult(..., publication_entry=None, publication_plan=None) because only CURRENT_MANDATORY builds an entry; the new-binding path below does the same. That means the vNext allocator cannot actually feed recovered-history artifacts into the PublicationPlan path unless every caller manually reconstructs a PublicationEntry, so historical recovery results are easy to drop even though the allocator has all of the candidate, binding, display, and RECOVERED_HISTORY role information needed to return the entry.

Useful? React with 👍 / 👎.

return VNextIdentityIndex(VNEXT_INDEX_SCHEMA, ())


def parse_vnext_index(payload: Mapping[str, Any]) -> VNextIdentityIndex:

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Route vNext indexes by namespace

Adding only this vNext-specific parser leaves namespace dispatch to callers: the existing public identity_v3.parse_identity_index still branches on schema_version, so a valid payload like {'namespace':'qar_vnext','schema_version':1,'entries':[...]} is routed to legacy v1 and rejected as invalid_reports_index unless the caller already knows to call parse_vnext_index. Because vNext deliberately reuses schema version 1, generic index readers need a namespace-first entry point; otherwise this reintroduces the caller-side routing the contract says to avoid.

Useful? React with 👍 / 👎.

@Pigbibi

Pigbibi commented Jul 14, 2026

Copy link
Copy Markdown
Contributor Author

Permanent freeze/close-reslice: closure_count=1/max=1, absolute_final=true. Preserve all branch/worktree/commits/comments/unresolved review evidence. No further code action.

@Pigbibi Pigbibi closed this Jul 14, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant