Skip to content

feat: define canonical review contract identity schema - #82

Closed
Pigbibi wants to merge 4 commits into
mainfrom
codex/pr-review-canonical-identity-schema
Closed

feat: define canonical review contract identity schema#82
Pigbibi wants to merge 4 commits into
mainfrom
codex/pr-review-canonical-identity-schema

Conversation

@Pigbibi

@Pigbibi Pigbibi commented Jul 12, 2026

Copy link
Copy Markdown
Contributor

Summary

  • add a pure contract_identity.v2 schema and canonicalizer
  • preserve ordered operators and reject malformed, oversized, or secret-bearing canonical identity inputs
  • compute and verify bridge-owned contract_key, behavior_digest, and fingerprint_v2
  • enforce strict GitHub repository and repository-relative POSIX path boundaries

Scope

This is the first sequential replacement slice for the frozen review-convergence PRs. It introduces only the validated identity data model and tests. It does not integrate runtime call sites, history persistence, workflows, arbitration, or the convergence state machine.

Validation

  • python3 -m unittest tests.test_contract_identity (8 passed)
  • python3 -m unittest tests.test_run_codex_pr_review (54 passed)
  • python3 -m unittest discover tests (628 passed, 1 skipped)
  • python3 -m ruff check service scripts tests
  • python3 -m compileall -q service scripts tests
  • actionlint .github/workflows/*.yml
  • git diff --check origin/main...HEAD

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

github-actions Bot commented Jul 12, 2026

Copy link
Copy Markdown

🤖 Codex PR Review

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

⚖️ Codex Review Arbitration

🚫 block: All three current findings remain valid against the PR diff. In scripts/contract_identity.py, contract_key is derived only from schema/scope/anchors/predicates, behavior_digest only from contract_key plus behavior clauses, and fingerprint_v2 only from those two digests; verify_persisted_identity() recomputes and compares only those three fields. The code never binds evidence into any verified digest, and tests/test_contract_identity.py::test_digest_tamper_order_and_evidence_binding_are_explicit explicitly asserts that changing evidence.head_sha and evidence.diff_digest leaves fingerprint_v2 unchanged, so the evidence-rebinding finding is proven. The redaction-collision findings are also proven by the regexes themselves: the CREDENTIAL pattern uses [:=]\s*(?P<value>\S+), so for strings like authorization == required or token === missing it can consume the first = as an assignment separator and replace authorization == / token === with a secret placeholder before tokenization, destroying operator-preserving canonicalization. Separately, the BEARER pattern (?i)\bbearer\s+\S+ redacts any bearer <word> sequence, including policy-state text such as bearer required and bearer forbidden, collapsing opposite behaviors to the same placeholder. There is no contract conflict with the prior blocking finding: the prior requirement was to authenticate or ignore persisted digests across the trust boundary, while the current findings either add missing evidence binding or fix unrelated canonicalization collisions; none of them reverse that prior behavior.

🚫 Blocking Issues

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

1. 🟠 [HIGH] Security in scripts/contract_identity.py

evidence never contributes to any verified bridge-owned digest: contract_key is built from scope/anchors/predicates, behavior_digest from behavior clauses, and fingerprint_v2 from those two digests only. As a result, verify_persisted_identity() will still accept a record after evidence.head_sha, evidence.diff_digest, or evidence.location_or_hunk_digest has been changed, which breaks the audit trail and lets a finding be rebound to a different diff or hunk without detection. (line 358)

Suggestion: Include the evidence fields in a verified digest (either by folding them into fingerprint_v2 or by adding a separate evidence_digest that verify_persisted_identity() also recomputes and checks).

2. 🟠 [HIGH] Logic in scripts/contract_identity.py

The credential redaction regex treats any single = as an assignment separator even when it is the first character of == or ===. A clause such as authorization == required or token === missing is therefore partially replaced with a secret placeholder before tokenization, destroying the operator-preserving canonicalization and causing distinct predicates to collapse to the wrong identity. (line 68)

Suggestion: Only redact true assignment forms, e.g. require = not to be adjacent to another = ((?<![=!<>])=(?![=])), or perform redaction after tokenization so comparison operators stay intact.

3. 🟠 [HIGH] Logic in scripts/contract_identity.py

The BEARER pattern redacts any bearer <word> sequence, including non-secret policy states like bearer required and bearer forbidden. Those opposite behaviors both canonicalize to the same <SECRET:BEARER:n> placeholder, so bearer-auth requirements and prohibitions can end up with the same behavior_digest. (line 66)

Suggestion: Narrow the bearer matcher to actual token shapes (for example long opaque/JWT-like values) or exempt the same safe state vocabulary used for credential values so state-only clauses are preserved verbatim.


Review by Codex PR Review bot • PR

@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: 692b2594b8

ℹ️ 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 thread scripts/contract_identity.py Outdated
Comment thread scripts/contract_identity.py Outdated
Comment thread scripts/contract_identity.py
Comment thread scripts/contract_identity.py
Comment thread scripts/contract_identity.py Outdated
Pigbibi and others added 2 commits July 13, 2026 03:47
Co-Authored-By: Codex <noreply@openai.com>
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: fb4d2384cd

ℹ️ 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 thread scripts/contract_identity.py Outdated
Comment thread scripts/contract_identity.py Outdated
Comment thread scripts/contract_identity.py
Co-Authored-By: Codex <noreply@openai.com>
@Pigbibi

Pigbibi commented Jul 12, 2026

Copy link
Copy Markdown
Contributor Author

Superseded by the smaller typed-model replacement chain beginning with #83. PR #82 mixed canonical identity, heuristic secret parsing, evidence, and persistence concerns and will not be merged. Follow-up slices: R2 structured adapter, R3 evidence binding, R4 trusted persist/replay, then runtime adoption.

@Pigbibi Pigbibi closed this Jul 12, 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