Repository navigation
Produce explicit carried-finding rechecks in Direct and Strict reviews - #180
Conversation
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![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: 54259315d6
ℹ️ 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".
| extra_tools=review_repo_tools(_repo_path(ctx)), | ||
| ) | ||
| rechecks, recheck_missing = checked_rechecks( | ||
| carried, recheck_output.get("finding_rechecks") if recheck_result.ok else [], |
There was a problem hiding this comment.
When the recheck agent returns valid finding_rechecks with the permitted base status needs_review—a likely response when a finding is still_affected—run_agent_step sets recheck_result.ok to false, so this branch discards every answer by passing an empty list to checked_rechecks. The structured result then reports all carried IDs as missing instead of returning their explicit outcomes; validate the returned records independently of the agent's overall status, while recording any escalation separately.
Useful? React with 👍 / 👎.
| duplicates.add(identity) | ||
| missing.append("duplicate finding recheck: " + identity) | ||
| seen.add(identity) | ||
| if (row.get("head_sha") != head_sha |
There was a problem hiding this comment.
For Direct requests that use uppercase hexadecimal SHAs, plan() and validate() accept and case-fold the expected/evidence heads, but a FindingRecheck using that same uppercase SHA is rejected here because its raw value is compared with the normalized lowercase head. Since commit SHA syntax elsewhere explicitly accepts either case, normalize row["head_sha"] before comparing so an otherwise valid completion is not reported as incomplete.
Useful? React with 👍 / 👎.
A later review omitting an old blocker does not prove it was fixed. Direct 1.1 and Strict 1.3 now accept carried findings and return explicit, head-bound fixed/still_affected/unverified answers for each ID. Direct binds the carried set to its issued context; Strict performs a read-only recheck and persists the answers through the public result contract.
Missing, duplicate, malformed, or wrong-head answers report incomplete rechecks, including runs that have not reached the review step. ReviewBot can use this contract to retain blockers until explicit resolution.
Validation: 131 targeted SDK, policy, review-step, and contract tests pass in the repository virtualenv; specification freshness passes; code review gate approved after adding pre-review failure and malformed-output regression tests.