fix(ai): separate a truncated generation from a successful one - #53
seonghobae wants to merge 1 commit into
Conversation
A chat-completions gateway reports `finish_reason` per choice, and `length` (or the `max_tokens` variant some gateways emit) means the content is a prefix of the intended answer. `finish_reason` appeared nowhere in this repository, so `_content` handed that prefix to schema validation like any other reply. A prefix can satisfy a model whose later fields are optional, and every production call site in `analysis.py` builds report sections from exactly such models. A half-written interpretation therefore reached the customer's report as if it were whole. When the prefix did fail validation, the schema-repair loop re-asked under the same ceiling with a longer conversation, which can only truncate again while spending another generation. `_content` now raises `NimTruncationError`, a `NimError` subclass so existing provider-failure handling is unchanged. An absent or unrecognised `finish_reason` still means unknown, never truncated, so no failure is invented for gateways that omit the field. This is the `finish_reason` half of issue #49. The token-ceiling half, deriving `max_tokens` from a model catalog instead of the hardcoded 4096 default, needs `settings.py`, which PRs #31 and #39 currently occupy, and stays on that issue. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
Warning Review limit reachedNext included review available in 13 minutes. View limit detailsLimit details: You’ve used the included review currently available. You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Review configuration: ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (4)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Merge-order note: #39 moves the file this change lives in
I merged both locally to find out what that costs. No pushes. The call site merges cleanly: git follows the rename and the fixed line lands in the new file. The single conflict is about names, not logic, because that branch renames Resolution, whichever order the two land in: keep that branch's exception names and label attribute, keep this change's addition, and drop the The point worth stating plainly is that this must not be resolved by taking one side wholesale. Details and the same note are on #39. |
![opencode-agent[bot]](https://avatars.githubusercontent.com/in/1549082?s=60&v=4){kind=small}
There was a problem hiding this comment.
Pull request overview
OpenCode could not approve from deterministic current-head evidence because GitHub Checks have failed.
Findings
1. HIGH Current-head GitHub Checks - Fix failed required checks before approval
- Problem: Failed same-head checks remain for
95aea3dbf0382d79ceb7834fcb7877247c970a1d. - Root cause: The model-unavailable evidence fallback is allowed only when peer GitHub Checks are complete and clean.
- Fix: Read and fix the failed check logs below, then rerun the current-head checks.
- Regression test: Keep the model-unavailable fallback gated on an empty failed-check rollup.
Failed checks:
- CodeQL PR/CodeQL compatibility analysis (actions): FAILURE (https://github.com/ContextualWisdomLab/four-pillars/actions/runs/34855787232/job/104144705884)
- CodeQL PR/CodeQL compatibility analysis (python): FAILURE (https://github.com/ContextualWisdomLab/four-pillars/actions/runs/34855787232/job/104144705875)
- CodeQL compatibility analysis (actions) check run: failure (https://github.com/ContextualWisdomLab/four-pillars/actions/runs/34855787232/job/104144705884)
- CodeQL compatibility analysis (python) check run: failure (https://github.com/ContextualWisdomLab/four-pillars/actions/runs/34855787232/job/104144705875)
- Strix Security Scan/strix: FAILURE (https://github.com/ContextualWisdomLab/four-pillars/actions/runs/34855786989/job/104146902951)
- Strix Security Scan/strix: failure (https://github.com/ContextualWisdomLab/four-pillars/actions/runs/34855786989/job/104146902951)
Changed-File Evidence Map
flowchart LR
PR["PR changed files"] --> Evidence["OpenCode bounded evidence"]
Evidence --> S1["Repository file: .gitignore"]
S1 --> I1["repository behavior"]
I1 --> R1["Review risk: Repository file: .gitignore"]
R1 --> V1["required checks"]
Evidence --> S2["Repository file: CHANGELOG.md"]
S2 --> I2["repository behavior"]
I2 --> R2["Review risk: Repository file: CHANGELOG.md"]
R2 --> V2["required checks"]
Evidence --> S3["Python package: nim.py"]
S3 --> I3["Python runtime API"]
I3 --> R3["Review risk: Python package: nim.py"]
R3 --> V3["pytest plus coverage"]
Evidence --> S4["Test: test_generation_truncation.py"]
S4 --> I4["regression suite"]
I4 --> R4["Review risk: Test: test_generation_truncation.py"]
R4 --> V4["targeted test run"]
OpenCode Review Overview
|
seonghobae
added
bug
ChatGPT Codex Connector
|
Exact-head admission audit: 현재 blocker: 활성 CHANGES_REQUESTED 1건; terminal workflow: CodeQL PR:failure. 유효 commit·diff·review evidence를 보존한 채 Draft/Proposed로 교정합니다. Base 이동이나 queue 대기만을 이유로 Close하지 않으며, Force Push·synthetic status/approval·manual rerun·bypass는 사용하지 않습니다. Blocker 수리 후 새 exact head에서 Checks와 review admission을 다시 받아야 합니다. |
1 participant
What customers felt
finish_reasondid not appear anywhere in this repository. A gateway that stopsat the token ceiling returns
finish_reason: "length"with content that is aprefix of the intended answer, and
_contentpassed that prefix straight intoschema validation.
Every production call site in
analysis.pybuilds a report section from a modelwhose later fields are optional, so a prefix validates. A half-written natal,
daewoon, annual, or monthly interpretation therefore reached the customer's PDF
as a complete section, with no signal anywhere that it had been cut off.
When a prefix did fail validation, the schema-repair loop re-asked under the
same
max_tokenswith a conversation that now also carried the truncated replyand the full JSON Schema. That can only truncate again, and each pass spends
another generation.
The change
_contentreadsfinish_reasonfrom the choice it already has and raisesNimTruncationErrorforlengthand for themax_tokensvariant some gatewaysemit. The new class subclasses
NimError, so every caller that already handles aprovider failure keeps working without a change.
An absent or unrecognised
finish_reasonmeans the reason is unknown, nottruncated. Gateways that omit the field behave exactly as before, and no failure
is invented for them.
Scope
This is the
finish_reasonhalf of #49. The other half, derivingmax_tokensfrom a model or deployment catalog instead of the hardcoded 4096 default, has to
change
settings.py, which PRs #31 and #39 both occupy. It stays on #49 ratherthan being forced into this branch.
tests/test_nim.pyandtests/test_nim_errors.pybelong to #39 and areuntouched. The new cases live in their own file.
Verification
Run on this branch, not quoted from an earlier head.
pytest -m 'not nim_live' -W error::ResourceWarning --cov=four_pillarsruff check .compileall src scriptsscripts/check_docs.pyscripts/product_gap_audit.pyThe new file failed with
ImportError: cannot import name 'NimTruncationError'before the source change and passes 6 cases after it.
graphify-out/is added to.gitignorein the same commit because the analysisrun for this head wrote artifacts into the tree and they are not product sources.
🤖 Generated with Claude Code