fix(verifier): normalize structured provider content - #2887
Conversation
stranske
added
agent:codex
|
Bugbot is not enabled for your account, so this pull request was not reviewed. Enable Bugbot in the Cursor dashboard to get automatic reviews on future PRs. |
|
Warning Review limit reachedYou’ve reached a temporary PR review limit under our Fair Usage Limits Policy. Next review available in: 25 minutes Your organization has reached its usage spending cap. Adjust your spending cap in the billing tab. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (2)
Comment |
|
Runner dispatch state for codex on PR #2887. Do not edit. |
There was a problem hiding this comment.
Pull request overview
This PR updates the PR verifier’s structured-output parsing so it can reliably handle providers that return structured “content blocks” (e.g., Anthropic-style lists of {type, text} blocks) by coercing them into a single text payload before schema parsing.
Changes:
- Add
_coerce_response_content()to normalize non-string LLM response content into a string for downstream JSON/schema parsing. - Broaden
_parse_llm_response()to acceptcontent: object, parse the coerced string, and store a normalizedraw_content. - Add a regression test covering structured (block-based) response content in the comparison runner.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| scripts/langchain/pr_verifier.py | Normalizes LLM response content (string vs block lists) before structured parsing and uses normalized raw_content. |
| tests/scripts/test_pr_verifier_structured_output.py | Adds test coverage for block-based structured response content handling. |
![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: 3496de0cbd
ℹ️ 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".
🤖 Bot Comment Handler
The agent has been assigned to this PR to address the bot review comments. Instructions for agent
The bot comment handler workflow has prepared context in the artifacts. |
Concatenate provider text blocks without a separator so a JSON document split across blocks reassembles faithfully, and coerce schema-repair responses through the same normalization so a structured repair reply is not rejected as CONCERNS.
3496de0 to
c823739
Compare
Review response — both findings were valid and are fixed in
|
| Reverted change | Failing test | Observed failure |
|---|---|---|
"".join → "\n".join |
test_comparison_runner_concatenates_split_text_blocks |
payload no longer parses; invoke call count 1 → 2 (spurious repair) |
| repair-reply normalization removed | test_comparison_runner_normalizes_structured_repair_response |
assert 'CONCERNS' == 'PASS' — exactly the P1's predicted symptom |
Source was restored and re-verified clean afterwards.
Validation on pushed head c8237395
pytest tests/scripts/test_pr_verifier_structured_output.py tests/test_structured_output.py tests/scripts/test_pr_verifier_compare.py→ 51 passed (structured-output suite went 33 → 35 with the two new regressions)ruff checkclean;ruff format --check→ already formatted;mypy scripts/langchain/pr_verifier.py→ no issuesgit diff --checkclean
Rebased onto current main (2321dbeb, picking up #2888/#2889 — no file overlap with this change) before pushing.
* fix(verifier): normalize structured provider content * fix(verifier): preserve split text blocks and normalize repair responses Concatenate provider text blocks without a separator so a JSON document split across blocks reassembles faithfully, and coerce schema-repair responses through the same normalization so a structured repair reply is not rejected as CONCERNS. --------- Co-authored-by: Codex Automation <codex-automation@users.noreply.github.com> Co-authored-by: Cursor Automation <cursor-automation@users.noreply.github.com>
Provider Comparison ReportProvider Summary
📋 Full Provider Details (click to expand)openai
anthropic
Agreement
DisagreementNo major disagreements detected. Unique Insights
🔍 LangSmith Traces |
Closer verifier disposition — the FAIL is a scope mismatch, not a defectProvider Comparison Report That claim is true about the diff and wrong about which PR should be judged against those criteria. Audited against the merged commits rather than the PR body:
Root cause of the false verdict: the follow-up PR was opened with the standard source-issue preamble ( The fix is independently confirmed working by this very run. Run Actions taken this round:
Carried forward, not dropped: CodeRabbit's 18:33:02Z thread on Process note for the next agent: when opening a bounded follow-up PR that fixes tooling rather than implementing the source issue, do not carry the source issue's acceptance-criteria block into the PR body, or |
Three CodeRabbit findings arrived at 18:33Z against code that had already merged (#2887, #2888), so they surfaced on an unrelated open PR's diff instead of anywhere actionable. - pr_verifier: a repair reply of only thinking/metadata blocks was truthy, so the serialized envelope was returned as repair text and the parser failed on JSON the provider never sent. The repair path is now stricter than the parse path: no text blocks, or blank text, means no repair. The parse path keeps its json.dumps fallback for diagnostics. - enforce_gate_branch_protection: two of the three snapshot sites hardcoded strict: True, so --snapshot --allow-non-strict wrote a health artifact claiming strict enforcement that --apply would not create. - enforce_gate_branch_protection: the --context help still advertised 'Gate / gate' as the default; DEFAULT_CONTEXTS is 'summary', and #2858 is specifically about not requiring 'Gate / gate'. Co-authored-by: Cursor <cursoragent@cursor.com>
) * fix: close the review findings that landed after their PRs merged Three CodeRabbit findings arrived at 18:33Z against code that had already merged (#2887, #2888), so they surfaced on an unrelated open PR's diff instead of anywhere actionable. - pr_verifier: a repair reply of only thinking/metadata blocks was truthy, so the serialized envelope was returned as repair text and the parser failed on JSON the provider never sent. The repair path is now stricter than the parse path: no text blocks, or blank text, means no repair. The parse path keeps its json.dumps fallback for diagnostics. - enforce_gate_branch_protection: two of the three snapshot sites hardcoded strict: True, so --snapshot --allow-non-strict wrote a health artifact claiming strict enforcement that --apply would not create. - enforce_gate_branch_protection: the --context help still advertised 'Gate / gate' as the default; DEFAULT_CONTEXTS is 'summary', and #2858 is specifically about not requiring 'Gate / gate'. Co-authored-by: Cursor <cursoragent@cursor.com> * fix(gate): keep already-strict snapshot target under allow-non-strict --allow-non-strict accepts a non-strict floor; an already-strict policy is still in sync. Snapshot desired.strict now mirrors that accepted current value so health_summarize does not render "✅ In sync" beside a True → False transition. Co-authored-by: Cursor <cursoragent@cursor.com> * fix(gate): wire already-strict snapshot target under allow-non-strict Implements the snapshot_desired_strict path the prior commit's regression test expects, so health artifacts stay consistent when an already-strict policy is accepted under --allow-non-strict. Co-authored-by: Cursor <cursoragent@cursor.com> * fix(gate): preserve already-strict on apply and cover review gaps Pass the effective strict target into update_status_checks under --allow-non-strict, and add the split-text, fetch-error, and apply context-drift regressions CodeRabbit flagged on #2890. Co-authored-by: Cursor <cursoragent@cursor.com> * chore(autofix): formatting/lint --------- Co-authored-by: Codex Automation <codex-automation@users.noreply.github.com> Co-authored-by: Cursor <cursoragent@cursor.com> Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
Closes #2878
Automated Status Summary
Scope
scripts/check_consumer_sync_drift.py:420-456already recordspending_sync_prs, butbuild_report()still sets top-level status todrift;main()then returns 1 for every non-converged state (scripts/check_consumer_sync_drift.py:841-860). The workflow runs on source pushes and at 05:10 UTC (.github/workflows/health-68-consumer-sync-drift.yml:6-17), before the 05:30 janitor, and comments on the durable issue for every failure (health-68-consumer-sync-drift.yml:81-201). Live evidence showed 249 runs with zero successes in 49 days and 223 comments on #2210. This is a current observability break: expected, covered propagation is reported as failure.Context for Agent
Related Issues/PRs
Tasks
scripts/check_consumer_sync_drift.py:358-495to parse current sync provenance and classify repo-level remediation states.scripts/sync_manifest_compiler.pyand require an open PR marker/branch to match it before state becomescovered.scripts/check_consumer_sync_drift.py:841-860soconvergedand fullycoveredreports exit zero;blocked,untracked_drift, lookup errors, and expired coverage exit non-zero..github/workflows/health-68-consumer-sync-drift.ymlto run after Maint 71 and invoke issue mutation only for actionable failure states..github/scripts/consumer_sync_drift_issue_body.jsso body/marker/comment output renders the five states and does not append an unchanged covered-state comment.tests/scripts/test_check_consumer_sync_drift.pywith current, superseded, blocked, expired, and lookup-error fixtures..github/scripts/__tests__/consumer-sync-drift-issue-body.test.jswith comment-suppression and actionable-state cases.docs/ops/CONSUMER_REPO_MAINTENANCE.mdanddocs/ops/DURABLE_TRACKING_ISSUES.mdwith the state and SLO contract.Acceptance criteria
python -m pytest tests/scripts/test_check_consumer_sync_drift.py -qandnode --test .github/scripts/__tests__/consumer-sync-drift-issue-body.test.jspass.coveredand process exit 0.coveredfixture;tests/scripts/test_check_consumer_sync_drift.py::test_current_sync_pr_covers_driftmust fail or change the report tountracked_drift. Restore the fixture before review.