Skip to content

feat: clean-slate vNext schema v1 identity foundation - #48

Closed
Pigbibi wants to merge 2 commits into
mainfrom
codex/qar-vnext-n1-schema-v1-foundation-only
Closed

feat: clean-slate vNext schema v1 identity foundation#48
Pigbibi wants to merge 2 commits into
mainfrom
codex/qar-vnext-n1-schema-v1-foundation-only

Conversation

@Pigbibi

@Pigbibi Pigbibi commented Jul 14, 2026

Copy link
Copy Markdown
Contributor

Scope

Fresh clean-slate foundation-only reslice from origin/main@d13dad1cfd2680cfdf9c96bc4687fc3d32ffc015.

Type/wire namespace is the only dispatch source of truth. This PR adds only immutable VNextIdentityBinding/VNextIdentityIndex plus strict schema-v1 parser/serializer. It has no SelectedCandidate, allocation mode, raw-report verification, PublicationEntry, PublicationPlan, publisher/build/Pages/I/O, legacy compatibility, dual-read, or migration.

Fixed contract

  • namespace qar_vnext_identity.v1, binding discriminator qar_vnext_binding.v1;
  • fixed semantic/artifact algorithm pair; unknown/future/mismatch fail closed;
  • strict schema-contract pairing via contract_version_for_schema();
  • cadence-aware daily/weekly/monthly targets;
  • canonical unsuffixed, variant full artifact-digest suffix;
  • omission vs explicit null for optional md/manifest;
  • V3 canonical/variant only, pending status only;
  • exactly one canonical per period, full artifact identity uniqueness, target/digest collision checks;
  • per-period display-primary/order invariants and safe integer bound;
  • snapshot, validate-all-before-sort, deterministic permutation-stable roundtrip.

Validation matrix

Focused adversarial tests cover version/namespace/type rejection, target suffixes, optional attachments, cadence/period, canonical/display policy, duplicate identity/collision, safe integers, Mapping snapshot, and deterministic roundtrip.

  • focused: python3 -m pytest -q tests/test_vnext_identity_schema_v1_foundation.py — 18 passed
  • full: python3 -m pytest -q — 283 passed
  • python3 -m ruff check src tests — passed
  • python3 -m compileall -q src tests — passed
  • git diff --check — passed

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

@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: 1b954cffec

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

Comment on lines +198 to +201
period = (binding.period_key, binding.as_of, binding.cadence)
previous = digest_periods.get(binding.artifact_integrity_digest)
if previous is not None and previous != period:
raise _error("artifact_digest_conflict")

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 Reject conflicting metadata for reused artifact digests

When two same-period bindings reuse an artifact_integrity_digest (for example an unsuffixed canonical plus a .variant-<digest> variant), this check only compares period_key/as_of/cadence. That means the same full-artifact digest can be accepted with a different semantic_digest or report schema/contract metadata because the canonical and variant target names do not collide. Since the artifact digest is supposed to identify the validated report bytes, allowing it to bind conflicting report identity metadata breaks the fail-closed identity contract and can corrupt later artifact-to-binding matching.

Useful? React with 👍 / 👎.

@github-actions

github-actions Bot commented Jul 14, 2026

Copy link
Copy Markdown

🤖 Codex PR Review

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

⚖️ Codex Review Arbitration

🚫 block: The current findings are not cleared by the PR source of truth. First, _validate_binding() only enforces binding.period_key == canonical_period_identity(binding.cadence, binding.as_of).key; it never compares binding.as_of to a canonicalized period date, while _check_target() bakes the raw binding.as_of into every filename. Nothing in the new docs or tests proves alternate weekly/monthly dates for the same logical period are rejected, so the PR still permits non-canonical as_of representations to survive validation. Second, the new contract doc declares qar_vnext_identity.v1 an immutable wire namespace where unknown/future/mismatched versions fail closed, but the implementation still allows report_schema_version in { "5", "6" } and derives acceptance from the mutable helper contract_version_for_schema(...). That means future changes in time_contract.py can widen what schema-v1 accepts without changing this module. The added test does not refute this because it only rejects integer 6, not string "6" with whatever pairing the helper may later return. The prior blocking finding was about artifact digest uniqueness; these current findings target different logic and do not reverse that earlier required behavior.

🚫 Blocking Issues

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

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

_validate_binding() only checks that period_key matches canonical_period_identity(cadence, as_of).key, but it never canonicalizes or validates as_of itself. For weekly/monthly cadences this allows multiple different as_of dates to represent the same logical period, so two producers can emit different filenames and wire payloads for the same period and still pass validation. That breaks the contract requirement for a single canonical timestamp representation and can cause digest/name mismatches across systems. (line 117)

Suggestion: Derive the canonical period object once and validate both the period key and the canonical date representation from it. Reject bindings whose as_of is not the canonical as_of for that cadence/period, and add tests proving that alternate dates within the same week/month fail closed.

2. 🟠 [HIGH] Logic in src/quant_advisor_research/vnext_identity_schema_v1.py

Schema-v1 is supposed to fail closed for future contract changes, but this module hard-codes report_schema_version as {"5", "6"} and then delegates acceptance to contract_version_for_schema(). If time_contract.py later learns a new valid pairing for report schema "6", this parser will start accepting it automatically under the existing qar_vnext_identity.v1 namespace, silently widening a supposedly fixed wire contract. (line 119)

Suggestion: Pin schema-v1 to an immutable local allowlist of exact report schema/version pairings intended for this namespace instead of consulting a mutable helper for future expansion. Add a regression test that report_schema_version="6" is rejected even if contract_version_for_schema() is updated elsewhere.


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

Final closure evidence (closure_count=1/max=1, absolute_final=true):

  • _validate_index() now rejects any repeated artifact_integrity_digest in the complete vNext index, including same-period canonical/variant or conflicting report/semantic metadata. Exact duplicate identity remains separately rejected.
  • VNextIdentityIndex.__post_init__() validates then canonical-sorts bindings with the shared stable key, so in-memory equality/hash and parser/serializer round-trip are permutation-stable.
  • Added regressions for same-digest conflicts and equality/hash/round-trip under reversed input order.

Final validation: focused 18 passed; full pytest 283 passed; targeted ruff, compileall, and diff-check passed. Foundation-only boundary remains unchanged: no allocation/PublicationPlan/raw-report/publisher/I/O/legacy compatibility.

@Pigbibi

Pigbibi commented Jul 14, 2026

Copy link
Copy Markdown
Contributor Author

Permanent freeze/close-reslice: closure_count=1/max=1, absolute_final=true; fingerprint=1e2d72cb050664ab59e8. Preserve branch/worktree/commits/comments/unresolved review evidence/tests. 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