fix(#158): surface Gemini finish_reason in degrade reasoning + raise e2e floor to 4096 - #160
Merged
Merged
Conversation
…e2e floor to 4096 Two fixes from the post-#155 full-pipeline finding (#158): 1. **Diagnostic upgrade (`signalforge.grade.engine`).** Add `_format_degrade_reasoning(exc)` helper. When the wrapped cause is `LLMResponseFormatError`, surface its bare `message` (which names the vendor `finish_reason` field + value via `LLMProvider.unclean_finish_reason_message`) into the degraded `GradingResult.reasoning` — e.g. `"call failed: GradeLLMError: Gemini response did not finish with a clean stop reason (finish_reason='MAX_TOKENS')..."` instead of the bare `"call failed: GradeLLMError"`. For every other cause (auth / rate-limit / parser / budget) the existing class-name shape is preserved verbatim so the audit corpus stays diff-clean for the 90% case — only the response-shape branch grows the diagnostic. Two new unit tests pin the new + preserved shapes. 2. **Floor bump (e2e overlay).** Raise `grade_max_output_tokens` overlay from 2048 to 4096 in both `test_e2e_gemini_smoke.py` and the `[gemini]` parametrization of `test_e2e_bigquery_smoke.py`. The 2048 figure was the in-isolation 5-pair smoke floor verified in #155 DEC-008; the first full-pipeline run found 5–6/108 pairs still degrade at 2048 (Gemini's per-pair `reasoning` length is high-variance enough that the in-isolation floor is not the full-fixture floor). 4096 is the new #158 fixture-scale floor. Doc reframes in lockstep: - `docs/grade-ops.md` + `docs/draft-ops.md` — Gemini row of the recommended-floors table bumped 2048 → 4096, with a "fixture-scale caveat" callout explaining the floor is necessary but not sufficient and operators should watch `aggregate_complete` on their own fixture. - `plans/super/155-...md` DEC-008 — addendum reframing the 2048 "verified safe" claim as scoped to the in-isolation probe; the DEC stays as historical record of the 2048 figure's provenance. Existing `test_grade_artifacts_safety_blocked_response_degrades_pair` updated to assert the new `startswith("call failed: GradeLLMError: ")` + `"finish_reason='SAFETY'" in reasoning` shape rather than the bare class name — the contract pin moves with the broadened reasoning. The acceptance gate from #158 (full-fixture e2e against Gemini returning `aggregate_complete=True`) is only verified by re-running `tests/cli/test_e2e_bigquery_smoke.py[gemini]` + `test_e2e_gemini_smoke.py` under the live env-var stack (`SF_RUN_GEMINI=1 GOOGLE_API_KEY=… SF_RUN_BQ=1 ANTHROPIC_API_KEY=… GOOGLE_CLOUD_PROJECT=… uv run pytest -m "e2e and gemini" --no-cov`) — those are excluded from default CI. Local 2589-test suite + ruff + pyright clean. Beads: bd_1-scaffolding-we0 (diagnostic), bd_1-scaffolding-ila (floor). Closes #158. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. 🗂️ Base branches to auto review (4)
Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughThis PR implements the resolution to ChangesGemini max_output_tokens floor bump and degraded reasoning enhancement
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related issues
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
There was a problem hiding this comment.
Pull request overview
This PR addresses #158 by improving Gemini degrade diagnostics and raising the Gemini grader e2e token floor to reduce full-fixture truncation degrades.
Changes:
- Adds
_format_degrade_reasoning(...)soLLMResponseFormatErrordetails, including vendorfinish_reason, are preserved in degraded grading results. - Raises Gemini e2e grading overlays from 2048 to 4096 max output tokens.
- Updates related tests, operational docs, and the historical #155 plan record.
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
src/signalforge/grade/engine.py |
Adds the degrade-reasoning formatter and uses it when building degraded grading results. |
tests/grade/test_engine.py |
Adds unit coverage for broadened response-format reasoning and preserved legacy shapes. |
tests/grade/test_gemini_neutrality.py |
Updates the Gemini safety-blocked contract assertion for the new diagnostic reasoning. |
tests/cli/test_e2e_gemini_smoke.py |
Raises Gemini grader e2e token overlay and updates comments/assertion messages. |
tests/cli/test_e2e_bigquery_smoke.py |
Raises the Gemini parametrized grader overlay to 4096. |
docs/grade-ops.md |
Updates grader floor guidance and Gemini diagnostic documentation. |
docs/draft-ops.md |
Mirrors the Gemini floor update in drafter operations guidance. |
plans/super/155-gemini-truncation-e2e-gap.md |
Adds historical context reframing the prior 2048 floor. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Owner
Author
|
@CodeRabbit review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
tests/cli/test_e2e_gemini_smoke.py (1)
69-69:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winUpdate the stale token-cap reference in the invariant doc block.
Line 69 still says this assertion proves the 2048 cap, but this test now pins 4096. Keeping it stale can mislead triage.
Suggested edit
- assertion that proves the 2048 cap fixes the truncation bug.** + assertion that proves the 4096 cap fixes the truncation bug.**🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/cli/test_e2e_gemini_smoke.py` at line 69, Update the stale invariant doc block that still references the "2048" token-cap to reflect the current test pin of "4096": locate the docstring/comment in tests/cli/test_e2e_gemini_smoke.py that mentions "2048" (the statement describing the assertion that proves the cap) and change that literal and any explanatory text to "4096" so the comment matches the actual assertion and avoids misleading triage.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@tests/cli/test_e2e_gemini_smoke.py`:
- Line 69: Update the stale invariant doc block that still references the "2048"
token-cap to reflect the current test pin of "4096": locate the
docstring/comment in tests/cli/test_e2e_gemini_smoke.py that mentions "2048"
(the statement describing the assertion that proves the cap) and change that
literal and any explanatory text to "4096" so the comment matches the actual
assertion and avoids misleading triage.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: b7733a5d-5c31-4fad-96fe-5896dc267d3f
📒 Files selected for processing (8)
docs/draft-ops.mddocs/grade-ops.mdplans/super/155-gemini-truncation-e2e-gap.mdsrc/signalforge/grade/engine.pytests/cli/test_e2e_bigquery_smoke.pytests/cli/test_e2e_gemini_smoke.pytests/grade/test_engine.pytests/grade/test_gemini_neutrality.py
Owner
Author
Live e2e verified ✅Ran the #158 acceptance gate against real Gemini + BigQuery: SF_RUN_GEMINI=1 GOOGLE_API_KEY=… SF_RUN_BQ=1 ANTHROPIC_API_KEY=… GOOGLE_CLOUD_PROJECT=… \
uv run pytest -m "e2e and gemini" --no-cov
SF_RUN_GEMINI=1 GOOGLE_API_KEY=… SF_RUN_BQ=1 ANTHROPIC_API_KEY=… GOOGLE_CLOUD_PROJECT=… \
uv run pytest "tests/cli/test_e2e_bigquery_smoke.py::test_e2e_signalforge_generate_against_austin_bikeshare[gemini]" \
-m e2e --no-cov
Both runs hit the load-bearing assertion Marker-selection follow-up worth noting
|
Four review-feedback fixes, all real: 1. `docs/grade-ops.md` (Copilot l.599): the safety-filter handling prose still pointed at `GeminiProvider.extract_text_blocks` as the raise site, but post-#155 the provider-neutral `LLMProvider.is_clean_completion` gate in `call_llm` is the primary raise site — and it fires on partial-text MAX_TOKENS too, not just zero-text-parts. Reworded to name both paths and route them through the same conservative-degrade contract. 2. `docs/draft-ops.md` (Copilot l.595): the Gemini drafter floor row leaned on grader-side evidence (#158) while the section lead-in says floors are "from live drafting runs." Qualified the 4096 figure as a *conservative mirror* of the grader floor, explicitly flagged as pending Gemini-drafter live validation when that lands. 3. `plans/super/155-...md` DEC-008 (Copilot l.56): the #158 addendum carried a broken-sentence placeholder ("See `plans/super/155-...` was the in-isolation verification" — typo from a half-edit). Replaced with a concrete pointer to issue #158 + the durable lesson in memory `in-isolation-smoke-misses-pipeline-drift`. 4. `tests/cli/test_e2e_gemini_smoke.py:69` (CodeRabbit outside-diff): module-docstring invariant #6 still said "the 2048 cap fixes the truncation bug" — stale after the floor bump. Now says "the 4096 cap (#158) fixes the full-fixture truncation bug." No code changes (doc + docstring only). Full local validation (ruff + pyright + 2589 pytest) clean. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Owner
Author
PR Review SummaryAll four review findings landed in commit Fixed (4 items)
False Positives (0 items)No false positives — every comment identified a real bug. Validation
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Two fixes from the post-#155 full-pipeline finding (#158):
1. Diagnostic upgrade (
signalforge.grade.engine)New
_format_degrade_reasoning(exc)helper. When the wrapped cause isLLMResponseFormatError, surface its baremessage(which names the vendorfinish_reasonfield + value viaLLMProvider.unclean_finish_reason_message) into the degradedGradingResult.reasoning:Before:
After:
For every other cause (auth / rate-limit / parser / budget) the existing class-name shape is preserved verbatim — only the response-shape branch grows the diagnostic, so the audit corpus stays diff-clean for the 90% case.
Two new unit tests pin both the new + preserved shapes.
2. Floor bump (e2e overlay)
grade_max_output_tokensoverlay raised 2048 → 4096 in bothtest_e2e_gemini_smoke.pyand the[gemini]parametrization oftest_e2e_bigquery_smoke.py.The 2048 figure was the in-isolation 5-pair smoke floor verified in #155 DEC-008. The first full-pipeline run found 5–6/108 pairs still degrade at 2048 (Gemini's per-pair
reasoninglength is high-variance enough that the in-isolation floor is not the full-fixture floor). 4096 is the new #158 fixture-scale floor.Doc reframes (in lockstep)
docs/grade-ops.md+docs/draft-ops.md— Gemini row of the recommended-floors table bumped 2048 → 4096, with a "fixture-scale caveat" callout explaining the floor is necessary but not sufficient and operators should watchaggregate_completeon their own fixture.plans/super/155-...mdDEC-008 — addendum reframing the 2048 "verified safe" claim as scoped to the in-isolation probe; the DEC stays as historical record of the 2048 figure's provenance.tests/grade/test_gemini_neutrality.py— the existing safety-blocked-response contract test moves frombad.reasoning == "call failed: GradeLLMError"tobad.reasoning.startswith("call failed: GradeLLMError: ")+"finish_reason='SAFETY'" in bad.reasoning. The contract pin moves with the broadened reasoning.Validation
uv run ruff check . && uv run ruff format --check . && uv run pyright: clean.SF_RUN_GEMINI=1 GOOGLE_API_KEY=… SF_RUN_BQ=1 ANTHROPIC_API_KEY=… GOOGLE_CLOUD_PROJECT=… \ uv run pytest -m "e2e and gemini" --no-covGradingReport.aggregate_complete=True(the bug Gemini grader: max_output_tokens=2048 still degrades 5-6/108 pairs on full-fixture e2e (post-#155 finding) #158 filed). If any residual pair still degrades, the new reasoning string names the exactfinish_reason.Beads
bd_1-scaffolding-we0— diagnostic upgradebd_1-scaffolding-ila— floor bump + doc reframeCloses #158.
🤖 Generated with Claude Code
Summary by CodeRabbit
Documentation
Improvements
Tests