Repository navigation
contract: publish findings with identity, severity and head binding - #176
Conversation
The review result crossed the boundary as prose plus an anchor-only audit trail, so a consumer could not say whether a blocker on this head is the same blocker it saw on the last one. The review bot's ready gate needs exactly that: it carries open blocker/major findings forward and may only page a maintainer once none remain. Adds a findings list, one entry per published finding, each with a finding_id that is stable across heads (file plus the whole normalized text, deliberately not the line, so an unrelated edit above a finding does not lose it), the severity it was actually published with, its anchor, and the head it was found on. Duplicate identities are kept rather than collapsed, because two findings the pipeline chose to publish separately are two findings. No join with the disposition audit: those records are keyed by anchor, an anchor is not unique between a withheld and a published candidate, and anchor resolution can move a line after they are written, so a join would mislabel findings rather than inform. The aggregate finding_dispositions field is unchanged; STRICT_API_VERSION goes to 1.2.0 so consumers can detect the addition. Scope note: the RFC also asks for a per-finding recheck, so that a carried blocker retires only on an explicit fixed verdict and silence never reads as a fix. That needs the review step to be handed the carried findings and the model to answer about each one; neither exists yet, and disposition_records discards head_recheck today. Emitting carried_from / head_recheck / findings_missing_carried now would be fields no producer fills, so they wait for the slice that wires them. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
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: 11fa857263
ℹ️ 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".
| path = str(comment.get("file") or "").strip() | ||
| if not path: | ||
| return None | ||
| line = comment.get("line") | ||
| return {"path": path, "line": int(line) if isinstance(line, int) else None} |
There was a problem hiding this comment.
When a persisted comment has a digit-string line or a normalized-equivalent path such as b/foo.py, the publisher accepts the line via _as_line_number and canonicalizes the path via normalize_path, but this helper reports line: null and hashes the unnormalized path; conversely, a boolean line becomes 1 here even though publication rejects it. The structured anchor and finding_id can therefore disagree with the actual published finding or change across heads, so reuse the publication path and line normalization rules here.
Useful? React with 👍 / 👎.
| # The same published set, one entry each, identified and bound to the | ||
| # head it was found on, so a consumer can carry an open blocker to the | ||
| # next head rather than re-deriving "the same finding" from prose. | ||
| "findings": findings, |
There was a problem hiding this comment.
When get_review_result receives a well-formed run ID that no longer exists, it returns unknown_run_result, which now advertises contract version 1.2.0 but still omits the newly required findings field. A consumer gated on 1.2.0 and reading result["findings"] will therefore fail on this supported response instead of receiving an empty collection; add "findings": [] to the unknown-run shape.
Useful? React with 👍 / 👎.
Implements the SDK half of the PR state machine RFC (#174).
Why
The review result crossed the boundary as prose plus an anchor-only audit trail, so a consumer could not say whether a blocker on this head is the same blocker it saw on the last one. The review bot's ready gate needs exactly that: it carries open blocker/major findings forward and pages a maintainer only when none remain.
What it adds
build_review_resultgains afindingslist, one entry per published finding:finding_idseverityanchor{path, line}, ornullfor a fileless findinghead_shaSTRICT_API_VERSIONgoes to1.2.0; the aggregatefinding_dispositionsfield is unchanged for existing consumers.Three decisions worth noting:
Scope note
The RFC also asks for a per-finding recheck, so a carried blocker retires only on an explicit
fixedverdict and silence never reads as a fix. That needs the review step to be handed the carried findings and the model to answer about each one; neither exists yet, anddisposition_recordsdiscardshead_rechecktoday. Emittingcarried_from/head_recheck/findings_missing_carriednow would be fields no producer fills, so they wait for the slice that wires them.Until then a consumer can carry findings by identity: a blocker whose id reappears on the new head is still open.
Tests: 11 new in
test/test_contract.py. Full suite green apart from threetest_mcp.pyfailures that fail identically on a cleanorigin/maincheckout (missing MCP module in this environment, plus one interrupted/failed assertion). Codex review gate: APPROVE.🤖 Generated with Claude Code