Skip to content

feat: add canonical typed review identity model - #83

Closed
Pigbibi wants to merge 3 commits into
mainfrom
codex/canonical-typed-identity-model
Closed

feat: add canonical typed review identity model#83
Pigbibi wants to merge 3 commits into
mainfrom
codex/canonical-typed-identity-model

Conversation

@Pigbibi

@Pigbibi Pigbibi commented Jul 12, 2026

Copy link
Copy Markdown
Contributor

Summary

  • add a pure typed canonical identity model for structured review contracts
  • preserve operators and policy states as explicit token kinds
  • compute stable contract, behavior, and fingerprint digests while excluding severity
  • reject evidence, persistence, reviewer prose, and runtime concerns from this layer

Scope

This is R1 of the replacement chain for superseded PR #82. It introduces only the secret-free typed identity model and focused tests. It does not add reviewer adaptation, evidence binding, persistence/history, workflow changes, or runtime adoption.

Validation

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

Pigbibi and others added 2 commits July 13, 2026 04:57
Co-Authored-By: Codex <noreply@openai.com>
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: 2 serious issue(s) found in high-risk files

⚖️ Codex Review Arbitration

🚫 block: Both current blocking findings remain valid on the cumulative PR diff. First, the secret-leak finding is proven by the implementation: _IDENTIFIER accepts any token matching ^[A-Za-z_][A-Za-z0-9_.-]*(?:\(\))?$, while _reject_secret_text() only rejects a short substring list (github_pat_, ghp_, aws_secret_access_key, akia, sk-, eyj). Tokens such as ghs_... and temporary AWS access keys starting with ASIA... still satisfy _IDENTIFIER, are not secret_ref, and are serialized into _payload_json and canonical_json(). The included test name test_secret_free_typed_tokens_only is contract evidence that raw secret-like tokens are supposed to be excluded, so this bypass is not cleared. Second, the severity-authentication finding is explicitly confirmed by the code and tests: severity is added to canonical, returned by as_record(), and revalidated by verify_identity_record(), but contract_key, behavior_digest, and fingerprint_v2 are computed without severity. The test test_policy_and_severity_semantics asserts that changing severity from high to critical leaves all three digests unchanged, so verify_identity_record() will accept a tampered severity value. There is no contract conflict with the prior blocking finding: the prior issue required canonicalizing :: representation in the same file/category/severity, while the current findings concern secret rejection and unauthenticated severity, so the required behaviors do not contradict each other.

🚫 Blocking Issues

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

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

The "secret-free" guarantee is bypassable because _reject_secret_text only checks a short substring denylist. Non-secret_ref identifiers such as ghs_... GitHub tokens or temporary AWS ASIA... access keys still match _IDENTIFIER, are not rejected here, and would be serialized into _payload_json/canonical_json, leaking secrets into storage and logs. (line 81)

Suggestion: Do not rely on a small denylist for secret detection. Either tighten the identifier grammar so raw credentials cannot match at all, or run a comprehensive secret-pattern rejector over every non-secret_ref token before accepting it.

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

severity is stored in the returned record but is intentionally excluded from contract_key, behavior_digest, and fingerprint_v2. As a result, verify_identity_record() will accept a record whose severity was changed from low to critical (or vice versa), which makes the API look like it verified a field that is actually unauthenticated. (line 169)

Suggestion: Either remove severity from the verified record entirely, or add a separate digest/signature that covers every field returned by verify_identity_record() and enforce that check there.

ℹ️ Other Findings

1. 🟡 [MEDIUM] Logic in scripts/canonical_typed_identity.py

Anchor validation only enforces alternating identifier / :: tokens, so even-length sequences ending with :: are accepted as valid anchors. That allows malformed identities like ["Review", "::"] to be hashed and persisted even though they do not identify a concrete symbol path. (line 112)

Suggestion: Require anchor lists to have odd length and explicitly assert that the final token is an identifier.


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: 0270b353c9

ℹ️ 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/canonical_typed_identity.py Outdated
Comment thread scripts/canonical_typed_identity.py
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: 9520846df3

ℹ️ 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 +116 to +119
if anchors:
expected_kind = "identifier" if index % 2 == 0 else "operator"
if token["kind"] != expected_kind or (expected_kind == "operator" and token["value"] != "::"):
raise IdentityError("anchors must alternate identifier and ::")

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 anchors that end with namespace separators

When anchors has an even number of tokens, e.g. [identifier("Namespace"), operator("::")], this positional check accepts it even though the normalized anchor represents an incomplete qualified symbol ending in ::. That lets malformed contract identities be hashed and verified as canonical records; require anchor lists to end on an identifier after enforcing the alternation.

Useful? React with 👍 / 👎.

if any(not isinstance(record[key], str) or not _DIGEST.fullmatch(record[key]) for key in expected):
raise IdentityError("record contains invalid digest")
payload = {key: value for key, value in record.items() if key not in expected}
identity = validate_identity(payload)

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 records that are not already canonical

When verify_identity_record() is given a record whose payload is merely normalizable, such as scope.repo set to AcMe/Audit-Bridge with digests computed for the lowercase canonical payload, this call re-canonicalizes the payload and accepts the record instead of rejecting the non-canonical JSON it actually received. That undermines verification for stored or signed records because altered-but-equivalent text can pass digest checks; compare the supplied payload to identity.payload before returning.

Useful? React with 👍 / 👎.

_CATEGORIES = frozenset({"bug", "contract", "logic", "performance", "reliability", "security"})
_SEVERITIES = frozenset({"critical", "high", "medium", "low"})
_OWNER = re.compile(r"^[A-Za-z0-9](?:[A-Za-z0-9-]{0,37}[A-Za-z0-9])?$")
_REPO = re.compile(r"^[A-Za-z0-9._-]{1,100}$")

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 dangling identifier punctuation

The identifier pattern accepts malformed symbols such as Class., module..name, or validate-() because dots and hyphens are allowed anywhere before the optional call suffix. In those cases the strict typed identity layer will hash and verify incomplete or ambiguous symbol tokens as canonical contract identities; require non-empty identifier segments and a real identifier before ().

Useful? React with 👍 / 👎.

@Pigbibi

Pigbibi commented Jul 12, 2026

Copy link
Copy Markdown
Contributor Author

Superseded by bounded fresh-main replacement #84. This PR remains frozen and will not be merged.

@Pigbibi Pigbibi closed this Jul 12, 2026
@Pigbibi
Pigbibi deleted the codex/canonical-typed-identity-model branch July 13, 2026 15:56
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