fix: improve truncation-aware parse failure logging

[stepwise-ai-dev]

Summary

Fixes #411 by surfacing an actionable message when a parse or recipe failure follows a model response that ended because of max_tokens.

Changes

• Propagates truncation metadata through generation validation errors and record-drop warnings.
• Normalizes Anthropic stop_reason into canonical completion choice finish_reason and prefers canonical finish reasons before raw response fallback.
• Adds regression coverage for sync generation, async generation, Anthropic stop reasons, sync DatasetBuilder logging, and async scheduler row-drop logging.

Validation

• uv run --group dev pytest packages/data-designer-engine/tests/engine/models/test_facade.py packages/data-designer-engine/tests/engine/models/test_model_errors.py packages/data-designer-engine/tests/engine/models/clients/test_anthropic.py packages/data-designer-engine/tests/engine/dataset_builders/test_dataset_builder.py packages/data-designer-engine/tests/engine/dataset_builders/test_async_scheduler.py
• make check-engine

Attention Areas

• packages/data-designer-engine/src/data_designer/engine/models/facade.py - truncation detection now treats finish_reason=length and finish_reason=max_tokens as max-token truncation.
• packages/data-designer-engine/src/data_designer/engine/models/clients/adapters/anthropic_translation.py - Anthropic stop_reason is now preserved as choice finish_reason.

Fixes #411

[greptile-apps]

Greptile Summary

This PR adds a planning document for issue #411 that describes how to surface actionable guidance when a parse failure follows a model response that was truncated due to hitting the output-token budget or context-window limit. No production code is changed; the plan must be approved before implementation begins (per CONTRIBUTING.md).

• The plan proposes populating choices[0].finish_reason in parse_anthropic_response() to canonicalize Anthropic's stop_reason, then accumulating a GenerationTruncationReason enum value across parse/correction attempts inside generate() / agenerate(), and finally formatting provider-specific remediation messages inside handle_llm_exceptions().
• The plan's "Explicitly excluded" file list omits test_dataset_builder.py from the implementation scope, but the PR description explicitly credits "sync DatasetBuilder logging" regression coverage — these two are in direct conflict and need to be reconciled before the plan is approved.

Confidence Score: 4/5

The plan is technically coherent and well-aligned with the existing codebase, but the contradiction between its explicit exclusion of test_dataset_builder.py and the PR description's claim of DatasetBuilder coverage means maintainers may approve the plan under a false understanding of what will actually be implemented.

The plan is internally consistent and fits cleanly into the existing response type hierarchy and error-handling pipeline. The one concrete issue — the mismatch between the PR description's stated coverage and the plan's explicit exclusion list — means the implementation scope is ambiguous for approving reviewers. Resolving that discrepancy is the only prerequisite before the plan can be approved and implementation can begin.

plans/411/truncation-aware-parse-failure-guidance.md — the Explicitly excluded list conflicts with the PR description's claimed test coverage.

Important Files Changed

Filename	Overview
plans/411/truncation-aware-parse-failure-guidance.md	New plan document for truncation-aware parse failure guidance; well-aligned with the codebase but contains an inconsistency between its "Explicitly excluded" files list and the PR description's claimed test coverage.

Flowchart

%%{init: {'theme': 'neutral'}}%%
flowchart TD
    A[ModelFacade generate / agenerate] --> B[Call completion]
    B --> C{Parser succeeds?}
    C -- Yes --> D[Return result - discard accumulated truncation_reason]
    C -- No --> E[Classify finish_reason from choices]
    E --> F{Canonical reason found?}
    F -- Yes --> G[Merge into accumulated GenerationTruncationReason]
    F -- No --> H[Try raw-response fallback via get_value_from]
    H --> G
    G --> I{Retries or restarts left?}
    I -- Correction --> J[Append error as user message]
    J --> B
    I -- Restart --> K[Reset messages, keep accumulation]
    K --> B
    I -- Exhausted --> L[Raise GenerationValidationFailureError with truncation_reason]
    L --> M[catch_llm_exceptions calls handle_llm_exceptions]
    M --> N{truncation_reason?}
    N -- MAX_TOKENS --> O[Advise increase max_tokens in inference_parameters]
    N -- MODEL_CONTEXT_WINDOW_EXCEEDED --> P[Advise reduce prompt or use larger-context model]
    N -- None --> Q[Existing generic validation message]
    O --> R[Raise ModelGenerationValidationFailureError]
    P --> R
    Q --> R

Loading

%%{init: {'theme': 'base', 'themeVariables': {"darkMode": true, "background": "#0d1117", "primaryColor": "#21262d", "primaryTextColor": "#e6edf3", "primaryBorderColor": "#8b949e", "lineColor": "#8b949e", "textColor": "#e6edf3", "edgeLabelBackground": "#161b22", "actorBkg": "#21262d", "actorBorder": "#8b949e", "actorTextColor": "#e6edf3", "actorLineColor": "#8b949e", "signalColor": "#8b949e", "signalTextColor": "#e6edf3", "noteBkgColor": "#373320", "noteBorderColor": "#d4a72c", "noteTextColor": "#f0e6c0", "labelBoxBkgColor": "#21262d", "labelBoxBorderColor": "#8b949e", "labelTextColor": "#e6edf3", "loopTextColor": "#e6edf3", "activationBkgColor": "#30363d", "activationBorderColor": "#8b949e"}}}%%
flowchart TD
    A[ModelFacade generate / agenerate] --> B[Call completion]
    B --> C{Parser succeeds?}
    C -- Yes --> D[Return result - discard accumulated truncation_reason]
    C -- No --> E[Classify finish_reason from choices]
    E --> F{Canonical reason found?}
    F -- Yes --> G[Merge into accumulated GenerationTruncationReason]
    F -- No --> H[Try raw-response fallback via get_value_from]
    H --> G
    G --> I{Retries or restarts left?}
    I -- Correction --> J[Append error as user message]
    J --> B
    I -- Restart --> K[Reset messages, keep accumulation]
    K --> B
    I -- Exhausted --> L[Raise GenerationValidationFailureError with truncation_reason]
    L --> M[catch_llm_exceptions calls handle_llm_exceptions]
    M --> N{truncation_reason?}
    N -- MAX_TOKENS --> O[Advise increase max_tokens in inference_parameters]
    N -- MODEL_CONTEXT_WINDOW_EXCEEDED --> P[Advise reduce prompt or use larger-context model]
    N -- None --> Q[Existing generic validation message]
    O --> R[Raise ModelGenerationValidationFailureError]
    P --> R
    Q --> R

Loading

Prompt To Fix All With AI

Fix the following 1 code review issue. Work through them one at a time, proposing concise fixes.

---

### Issue 1 of 1
plans/411/truncation-aware-parse-failure-guidance.md:119-123
**Contradiction between "Explicitly excluded" list and PR description**

The plan marks `test_dataset_builder.py` as explicitly excluded from the implementation, but the PR description's Changes section reads: *"Adds regression coverage for sync generation, async generation, Anthropic stop reasons, **sync DatasetBuilder logging**, and async scheduler row-drop logging."* A reviewer reading only the PR description would expect DatasetBuilder test coverage to land in this PR; the plan would deliver something different. Either the PR description should be updated to remove "sync DatasetBuilder logging", or the exclusion list here should be revised to allow it.

_{Reviews (2): Last reviewed commit: "docs: plan #411 truncation guidance" | Re-trigger Greptile}

[stepwise-ai-dev]

Thanks for the automated review. Greptile, DCO, semantic title, and the agentic CI gate are now passing. The only failing check is the linked-issue gate because #411 does not yet have the maintainer-added triaged label; happy to adjust the PR if maintainers want any changes before/after triage.

[github-actions]

Stale PR reminder

This PR has had failing checks for 7 days without activity.

Failing checks: check

Please push an update or leave a comment if you're still working on this.
Otherwise, this PR will be automatically closed in 7 days.

To prevent auto-close, add the keep-open label.

[github-actions]

Issue #411 has been triaged. The linked issue check is being re-evaluated.

[andreatgretel]

Thanks for the contribution, this is a nice quality-of-life fix. I reviewed the truncation metadata flow, Anthropic stop_reason mapping, and dropped-row guidance. No major blockers from me, but I'd like to see a small cleanup pass before merge:

• In facade.py, _get_response_value() and _get_first_response_item() duplicate helpers that already exist in models/clients/parsing.py as get_value_from() and get_first_value_or_none(). Please reuse the existing helpers to avoid drift.
• The raw response fallback in _response_was_truncated_by_max_tokens() is useful for future/custom adapters, but please add a short comment saying it is for adapters that do not populate choices[0].finish_reason yet.
• With correction steps or restarts, truncated_by_max_tokens currently reflects the final failed parse attempt only. Please accumulate it with OR across attempts so earlier truncation still gives users the helpful max_tokens guidance.

Focused tests and smoke checks passed locally. Once those small items are addressed, this looks good to merge from my side.

[nabinchha]

One broader simplification suggestion, especially now that the legacy sync builder has been removed from main: could we avoid threading truncated_by_max_tokens through both exception classes and then reading it with getattr() in the builder?

The current flow is roughly:

finish_reason → boolean → GenerationValidationFailureError → boolean → ModelGenerationValidationFailureError → builder formatting

The async scheduler already logs the formatted public exception, so I think the same behavior can be implemented with fewer layers:

• Normalize the provider termination reason in the client adapter.
• Accumulate the truncation state locally across correction attempts/restarts.
• Raise a dedicated internal validation-error subtype (or carry a typed reason only on the internal validation error).
• Format the actionable message in handle_llm_exceptions().
• Let the scheduler log the existing ModelGenerationValidationFailureError normally.

For the narrow scope of #411, an internal MaxTokensGenerationFailureError(GenerationValidationFailureError) subtype would be enough. That would avoid adding truncated_by_max_tokens to the public model error, avoid the builder getattr(), and allow the obsolete sync-builder hunk and test to be dropped during the rebase.

There is also one Anthropic nuance worth accounting for in the design: Anthropic can terminate with either max_tokens or model_context_window_exceeded, and both can leave structured output truncated. They need different guidance:

• max_tokens: increase inference_parameters.max_tokens.
• model_context_window_exceeded: reduce the prompt/context/schema or use a model with a larger context window; increasing max_tokens will not help.

If we want this PR to support both, a small canonical truncation-reason enum on the internal error would be cleaner than a boolean. If we intentionally keep this PR limited to #411's output-token case, the dedicated subtype plus a follow-up for context-window truncation would keep this change small and explicit.

Overall, I think the behavior is worth keeping; this is mainly an opportunity to reduce coupling and make the Anthropic boundary clearer before merge.

[nabinchha]

One broader simplification suggestion, especially now that the legacy sync builder has been removed from main: could we avoid threading truncated_by_max_tokens through both exception classes and then reading it with getattr() in the builder?

The current flow is roughly:

finish_reason → boolean → GenerationValidationFailureError → boolean → ModelGenerationValidationFailureError → builder formatting

The async scheduler already logs the formatted public exception, so I think the same behavior can be implemented with fewer layers:

• Normalize the provider termination reason in the client adapter.
• Accumulate the truncation state locally across correction attempts/restarts.
• Raise a dedicated internal validation-error subtype (or carry a typed reason only on the internal validation error).
• Format the actionable message in handle_llm_exceptions().
• Let the scheduler log the existing ModelGenerationValidationFailureError normally.

For the narrow scope of #411, an internal MaxTokensGenerationFailureError(GenerationValidationFailureError) subtype would be enough. That would avoid adding truncated_by_max_tokens to the public model error, avoid the builder getattr(), and allow the obsolete sync-builder hunk and test to be dropped during the rebase.

There is also one Anthropic nuance worth accounting for in the design: Anthropic can terminate with either max_tokens or model_context_window_exceeded, and both can leave structured output truncated. They need different guidance:

• max_tokens: increase inference_parameters.max_tokens.
• model_context_window_exceeded: reduce the prompt/context/schema or use a model with a larger context window; increasing max_tokens will not help.

If we want this PR to support both, a small canonical truncation-reason enum on the internal error would be cleaner than a boolean. If we intentionally keep this PR limited to #411's output-token case, the dedicated subtype plus a follow-up for context-window truncation would keep this change small and explicit.

Overall, I think the behavior is worth keeping; this is mainly an opportunity to reduce coupling and make the Anthropic boundary clearer before merge.

[andreatgretel]

@stepwise-ai-dev, are you planning to continue working on this PR?

We think the behavior is worth keeping, but main has changed significantly since this was opened, including removal of the sync engine. The implementation also needs to be adapted to the current error flow and address the review feedback around correction attempts and provider-specific termination reasons.

If you are not planning to continue, we will likely close this PR and implement a smaller version in a separate maintainer PR. Please let us know. Thanks again for the contribution.

[stepwise-ai-dev]

Yes, I'm planning to continue. Thanks for checking in, and for the clear feedback on the current error flow. I've reviewed the changes on main and both reviews, and I'm restarting this from current main without the obsolete sync-builder changes.

I've prepared a smaller plan around the current async path: normalize provider termination reasons, accumulate truncation across correction and restart attempts, keep the reason internal, and let the scheduler log the existing public error. It also distinguishes max_tokens from model_context_window_exceeded so the guidance stays accurate.

I'll add the plan to the PR for review before resuming implementation.

[greptile-apps]

Contradiction between "Explicitly excluded" list and PR description

The plan marks test_dataset_builder.py as explicitly excluded from the implementation, but the PR description's Changes section reads: "Adds regression coverage for sync generation, async generation, Anthropic stop reasons, sync DatasetBuilder logging, and async scheduler row-drop logging." A reviewer reading only the PR description would expect DatasetBuilder test coverage to land in this PR; the plan would deliver something different. Either the PR description should be updated to remove "sync DatasetBuilder logging", or the exclusion list here should be revised to allow it.

Prompt To Fix With AI

This is a comment left during a code review.
Path: plans/411/truncation-aware-parse-failure-guidance.md
Line: 119-123

Comment:
**Contradiction between "Explicitly excluded" list and PR description**

The plan marks `test_dataset_builder.py` as explicitly excluded from the implementation, but the PR description's Changes section reads: *"Adds regression coverage for sync generation, async generation, Anthropic stop reasons, **sync DatasetBuilder logging**, and async scheduler row-drop logging."* A reviewer reading only the PR description would expect DatasetBuilder test coverage to land in this PR; the plan would deliver something different. Either the PR description should be updated to remove "sync DatasetBuilder logging", or the exclusion list here should be revised to allow it.

How can I resolve this? If you propose a fix, please make it concise.

Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!

[andreatgretel]

Thanks for revising this against current main. The plan addresses the earlier feedback from @nabinchha and me, and the direction looks good. Approved to proceed with implementation.

Two non-blocking follow-ups:

• Put direct parse_anthropic_response() coverage in test_anthropic_translation.py.
• Update the PR description to reflect the new plan and remove the obsolete sync DatasetBuilder claims.