fix(code-review): codex reviewers can answer, and a no-verdict review stops suppressing - #87
Conversation
…nswer OpenAI strict structured-output mode refuses a schema where a key in `properties` is absent from `required`, and refuses it at the provider rather than at the model, so the run dies having read nothing: Invalid schema for response_format 'codex_output_schema': ... 'required' ... including every key in properties. Missing 'suggestion'. findingSchema and judgeSchema both declared `suggestion` and omitted it from `required`. It becomes `["string", "null"]` and joins `required`, the spelling start_line and end_line already used: required keeps the schema valid, nullable keeps a reviewer with nothing to suggest from inventing one. A null decodes to the empty string every consumer already guards on. Claude accepts either spelling, so this was green on the worktree roster and fatal on the PR roster, which is the only place it ran. The new test walks each schema generically and fails on any properties key missing from required, so a schema that gains a field fails here rather than on the next pull request review.
Suppression counted any review this App authored of the head, so a run where every reviewer died posted a review saying so and that review then refused the re-run. The pull request displayed a review, had reviewed nothing, and the marker recording the failure was what prevented anyone fixing it. Only --force got past it. A head counts as reviewed only when a review the App authored carries a marker reading verdict=complete. The data was posted all along and never read back: the reviews query asked for each review's commit and authorship and not its body, so the marker was out of reach. ReadReviewedCommits becomes ReadPriorReviews, returning each authored review with its head and its body. It parses nothing — the transport does not own the review format, and the caller already holds it. An App-authored review carrying no parseable marker counts as no verdict. It is the safer of the two readings: being wrong costs one re-run, where the other direction costs a pull request displaying a review nobody performed. --force is unchanged for a genuinely complete review.
…has bodies The verdict check added a second GraphQL query returning each authored review's commit and body. ReadSubmittedReviews already returned exactly that, for approval, and pages at reviewsPerPage because a review body is the largest document on a pull request — where the new query inherited pageSize, sized for reads that carry no body, and could overrun the response limit and fail the read. Both readers now come through ReadSubmittedReviews, and the CLI filters on ByViewer. The comment justifying two functions by one of them needing no body went with the function: the run needs the body now, so the cheaper query could not answer the question it was there to answer.
Three candidates, each anchored to the file it came from: the strict-mode required-key rule that made every posted review die silently, what a posted review has to carry before it suppresses the next one, and why a body-carrying GraphQL query has its own page size. Replaces an earlier candidate describing the suppression defect, which named two symbols this branch deletes.
|
![agtk-code-review[bot]](https://avatars.githubusercontent.com/in/4870124?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review by agtk — panel standard-codex
1 RED. 1 posted as inline comments, 0 against a whole file.
What's good
- Suggestion fields are required but nullable, preserving optional advice while satisfying strict schema requirements.
- Generic schema tests catch missing required properties in nested objects and verify null suggestions decode safely.
- Reusing the submitted-review reader removes duplicate query code and preserves pagination sized for review bodies.
Ran and reported nothing: security-codex
Absent from the reviewed copy (1): a symlink, a submodule or a file too large to read is not code this review looked at.
panel standard-codex, 4 runs, 1 convention docs read · range main...b6d2843f9edadefd8d5aadd2d07c216a446675df · manifest built-in default
Suppression accepted any App-authored complete review of the head, where approval reads the newest whose commit and marker head both match. The two disagreed in both directions. An older complete review spoke for a newer forced run that failed, so suppression refused the re-review while approval refused the head, and the only way out was a --force nothing told anyone to type — the bug this branch set out to fix, rebuilt one condition further along. A marker naming another commit also counted, though a body can be edited after it is posted and approval rejects exactly that. lastReview becomes reviewapprove.LastReview and carriesAVerdictFor calls it. Two commands turning on one question must not answer it twice.
There was a problem hiding this comment.
Review by agtk — panel standard-codex
This review did not reach a verdict: no reviewer answered, so nothing looked at this change
Could not answer (2)
correctness-codex: correctness-codex ran and did not answer: codex ended the turn: You've hit your usage limit. Upgrade to Pro (https://chatgpt.com/explore/pro), visit https://chatgpt.com/codex/settings/usage to purchase more credits or try again at 12:28 PM.security-codex: security-codex ran and did not answer: codex ended the turn: You've hit your usage limit. Upgrade to Pro (https://chatgpt.com/explore/pro), visit https://chatgpt.com/codex/settings/usage to purchase more credits or try again at 12:28 PM.
This review is partial: what these would have found is unknown, not absent.
Absent from the reviewed copy (1): a symlink, a submodule or a file too large to read is not code this review looked at.
panel standard-codex, 2 runs, 2 could not answer, 1 convention docs read · range main...875b6f6adc8d97823b4de532735a5e198a41f6aa · manifest built-in default
There was a problem hiding this comment.
Review by agtk — panel standard
No findings survived the panel.
Ran and reported nothing: correctness, security
Absent from the reviewed copy (1): a symlink, a submodule or a file too large to read is not code this review looked at.
panel standard, 3 runs, 1 convention docs read, $2.2861 · range main...875b6f6adc8d97823b4de532735a5e198a41f6aa · manifest built-in default
There was a problem hiding this comment.
Approved by agtk
875b6f6adc8d97823b4de532735a5e198a41f6aa carries a review by this installation that reached a verdict. Every finding it reports at or above AMBER is marked a false positive on its thread by an account that can push, and every comment thread on this pull request is resolved.
A person typed this. No review run can reach it, and nothing overrides it: a finding is cleared by changing the code or by a written statement on its thread, and by nothing else.
1 participant
Two bugs that compound: no codex reviewer could answer, and the empty review it posted then blocked the re-run. Together they made
agtk code-review run --pron this repo's default roster produce a pull request that displays a review, having reviewed nothing, and refuse to try again.Every codex reviewer died on the answer schema
OpenAI strict structured-output mode refuses a schema where a key in
propertiesis absent fromrequired, and refuses it at the provider rather than at the model — so the run dies having read nothing.findingSchemaandjudgeSchemaboth declaredsuggestionand omitted it fromrequired.suggestionis now["string", "null"]and required, in both — the spellingstart_lineandend_linealready used. Required keeps the schema valid; nullable keeps a reviewer with nothing to suggest from inventing one. A null decodes to the empty string every consumer already guards on.Claude accepts either spelling, so this was green on the worktree roster (claudecode) and fatal on the PR roster (codex) — the only place it ran.
internal/reviewrun/schema_test.gowalks each schema sent to a provider and asserts generically that every key in apropertiesappears in that object'srequired, reading the JSON rather than listing today's field names.A review that reached no verdict suppressed the review that would
Suppression counted any App-authored review of the head, so a run where every reviewer died posted a review saying so, and that review then refused the re-run. Only
--forcegot past it.A head now counts as reviewed only when a review this App authored carries a marker reading
verdict=complete. The data was posted all along and never read back: the reviews query asked for each review's commit and authorship and not its body.ByViewerreviews count; anyone who can review a PR can type the characters that open a marker.--forceis unchanged for a genuinely complete review.The check reads
ReadSubmittedReviews, the reader approval already used, andReadReviewedCommitsis gone. A second query against the same connection for the same reviews — one of which could not answer the question — was what the cheaper body-less query bought.Verification
Both diagnoses were confirmed against the live provider before fixing: the old schema is rejected with a 400 naming
suggestion, the new shape is accepted and returns"suggestion": null. PR #86's two posted reviews both carryverdict=incompleteat the current head.make checkgreen. Reviewed locally with thedeeppanel over the branch, then again over the fix commit — clean.Release notes at
docs/releases/v0.12.1.md.