fix: add stable review contract identities#79
Conversation
Co-Authored-By: Codex <noreply@openai.com>
🤖 Codex PR Review🚫 Merge blocked: 2 serious issue(s) found in high-risk files ⚖️ Codex Review Arbitration🚫 block: Both current findings remain valid in the shown source. First, 🚫 Blocking IssuesThese issues must be fixed before this PR can be merged: 1. 🟠 [HIGH] Logic in
|
![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: b0470232d7
ℹ️ 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 not isinstance(findings, list): | ||
| return [], False | ||
| if status not in {"blocking", "clear", "cleared", "overflow", "invalid_history"}: | ||
| if status not in {"blocking", "clear", "cleared", "overflow", "invalid_history", "initial_review", "remediation", "arbitration_required", "blocked_human_contract"}: |
There was a problem hiding this comment.
When a trusted history marker uses one of the newly accepted statuses such as arbitration_required or blocked_human_contract, parse_finding_history now returns valid history, but has_active_blocking_history() only treats blocking plus overflow/invalid sentinels as active. If the next primary review is clean, active_blocking_history stays false and the PR can be allowed and re-serialized as clear without the independent confirmation these statuses require. Either reject these statuses here or include the blocking ones in the active/confirmation checks.
Useful? React with 👍 / 👎.
| text = " ".join( | ||
| str(finding.get(field) or "") for field in ("description", "suggestion") | ||
| ) | ||
| anchors = re.findall(r"`([^`]{1,120})`", text) | ||
| anchors.extend(re.findall(r"\b[A-Za-z_]\w*(?:\.[A-Za-z_]\w*)+\b", text)) |
There was a problem hiding this comment.
Because the subject is built from backtick anchors in both description and suggestion, two findings for the same described contract stop sharing a contract_key as soon as their opposing suggestions name different code symbols. For example, prior `PanelContract` ... / Return ReviewResult(...)and current PanelContract ... / `Raise `ReviewError hash as different contracts, so conflicting_contract_findings() never compares their behavior and auto-remediation may proceed on contradictory instructions.
Useful? React with 👍 / 👎.
| } | ||
| prior_by_contract: dict[str, dict[str, Any]] = {} | ||
| for round_state in reversed(history): | ||
| if round_state.get("status") in {"clear", "cleared"} and not round_state.get("findings"): |
There was a problem hiding this comment.
A cleared round can contain the findings that independent arbitration already invalidated, but this loop only stops on clear/cleared rounds with no findings and otherwise indexes those cleared findings as prior active contracts. When a later review reports the same contract with different behavior, contract_conflict is set and auto_fix_allowed is disabled even though the earlier finding was resolved; cleared rounds with findings should be skipped or treated as resolving those contract keys, matching previous_matching_findings().
Useful? React with 👍 / 👎.
| decision.update( | ||
| { | ||
| "blocked": True, | ||
| "contract_conflict": True, |
There was a problem hiding this comment.
When a conflict detected here coexists with another repeated finding, the repeated finding can still trigger arbitration using only previous_findings, and apply_arbitration_result() later overwrites contract_conflict/auto_fix_allowed from that arbiter response. If the arbiter returns contract_conflict: false for the repeated finding, the conflict found in this block is lost and auto-remediation is re-enabled; include these conflict matches in arbitration or preserve the flag after arbitration.
Useful? React with 👍 / 👎.
| "contract_key": _contract_key(finding), | ||
| "behavior": _normalize_contract_text(finding.get("suggestion") or finding.get("description")), |
There was a problem hiding this comment.
For findings with a non-empty but generic suggestion, this ignores the description even when the description contains the actual required behavior. If two rounds share the same contract anchor and suggestion such as Update the guard, but the descriptions flip from “must reject” to “must accept”, the behavior digest stays identical, so the change is treated as a repeat instead of a contract conflict and auto-remediation can follow the contradictory current instruction.
Useful? React with 👍 / 👎.
| if not isinstance(findings, list): | ||
| findings = [] | ||
| findings = [ | ||
| {**finding, **_contract_finding(finding)} |
There was a problem hiding this comment.
This merge overwrites the reviewer-provided description and suggestion with _contract_finding()'s history-sanitized versions before formatting the PR comment and decision artifact. For findings that mention normal code identifiers containing digits, such as fingerprint_v2 or HTTP2Settings, the history scrubber replaces those tokens with [REDACTED], so the human-facing review can lose the exact symbol the author needs to fix; keep sanitized copies only for the history marker/identity fields.
Useful? React with 👍 / 👎.
|
|
||
|
|
||
| def _normalize_contract_text(value: Any) -> str: | ||
| text = _sanitize_history_text(value, 240).lower() |
There was a problem hiding this comment.
Using the history scrubber before extracting contract tokens collapses common versioned identifiers to the same redacted token. In a same-file/category review, anchors such as TokenV2Contract and TokenV3Contract therefore produce the same subject, which can make unrelated version-specific findings repeat or conflict with each other and incorrectly disable auto-remediation.
Useful? React with 👍 / 👎.
Co-Authored-By: Codex <noreply@openai.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 00eb36a9ed
ℹ️ 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".
| contract_conflicts = conflicting_contract_findings( | ||
| finding_history, decision["blocking_findings"] | ||
| ) |
There was a problem hiding this comment.
When contract_conflicts is non-empty for the normal opposite-behavior case, previous_findings is often empty because matching now requires the same (contract_key, behavior_digest). This block sets next_action to contract_arbitration, but the later arbitration condition only looks at previous_findings/repeats, so the independent arbiter is never called and the prior conflicting finding is never sent with the diff; a false-positive conflict can only remain blocked instead of being cleared or confirmed by the arbiter.
Useful? React with 👍 / 👎.
| return "|".join(sorted(anchor for anchor in normalized if anchor)) or _normalize_contract_text( | ||
| finding.get("suggestion") or text | ||
| ) |
There was a problem hiding this comment.
When a reviewer describes the same contract in prose without backticks or dotted symbols, this fallback uses the suggestion as the contract_key subject. Opposite findings such as prior “return a blocked result” and current “raise an error” then hash to different contracts, so conflicting_contract_findings() and previous_matching_findings() treat them as unrelated and leave auto-remediation enabled instead of requiring contract arbitration.
Useful? React with 👍 / 👎.
| if status in {"clear", "cleared"}: | ||
| resolved.add(identity) | ||
| continue |
There was a problem hiding this comment.
Because this reverse scan records every cleared identity even after a newer blocking round for the same identity was already added to prior_by_contract, a history like blocked A → cleared A → blocked A will have the latest active A suppressed by the older clear. If the current finding then changes A's behavior, conflicting_contract_findings() returns no conflict and can leave the reintroduced blocker outside contract arbitration.
Useful? React with 👍 / 👎.
1 participant
Summary
This is C2 of the replacement sequence for #77. It does not change the convergence state machine or required check names.
Verification