fix: improve truncation-aware parse failure logging

[stepwise-ai-dev]

📋 Summary

Surfaces actionable guidance when a structured-output parse or recipe failure follows a model response terminated by an output-token or context-window limit. The implementation follows the current model-error and async scheduler flow while preserving generic validation behavior.

🔗 Related Issue

Fixes #411

🔄 Changes

• Normalize Anthropic stop_reason into the canonical completion-choice finish_reason.
• Classify output-token and context-window truncation and retain the strongest reason across correction and conversation-restart attempts.
• Keep the typed reason on the internal generation-validation error and format reason-specific guidance at the existing public model-error boundary.
• Preserve the existing async scheduler behavior: it logs the formatted public exception and drops the failed row, with no scheduler production change.
• Leave the public exception shape, retry policy, configuration surface, and removed sync DatasetBuilder path unchanged.

🔍 Attention Areas

• Anthropic response normalization
• Internal reason and public-boundary formatting
• Canonical-first classification and per-call accumulation
• Approved implementation plan

🧪 Testing

• make test passes — 3,975 passed, 1 skipped
• Unit tests added/updated — 288 focused adapter, error, facade, and scheduler tests pass
• E2E tests added/updated (if applicable) — N/A; this changes deterministic adapter/error-boundary behavior and no live-provider E2E surface
• make check-engine passes.
• make check-all-fix passes with no tracked changes.

✅ Checklist

• Follows commit message conventions
• Commits are signed off (DCO)
• Architecture docs updated (if applicable) — N/A; no public architecture contract changed, and the approved plan is included

[greptile-apps]

Greptile Summary

This PR surfaces actionable remediation guidance when a parse or recipe failure follows a model response that was cut off at the output-token or context-window limit. Previously all parse failures received the same generic validation message regardless of why the response was truncated.

• Adds a GenerationTruncationReason enum and attaches it to GenerationValidationFailureError; handle_llm_exceptions selects reason-specific solution text for MAX_TOKENS and MODEL_CONTEXT_WINDOW_EXCEEDED, falling back to the existing generic message otherwise.
• Normalises Anthropic stop_reason into the canonical choices[0].finish_reason field (matching the OpenAI-compatible path), and classifies canonical reasons first with a raw-response fallback for adapters that have not yet been updated.
• Accumulates the most-severe truncation reason across all correction steps and conversation restarts, with MODEL_CONTEXT_WINDOW_EXCEEDED always winning over MAX_TOKENS so users receive the correct remediation advice.

Confidence Score: 5/5

Safe to merge — the changes are additive and well-scoped, touching only error classification and message formatting with no impact on retry counts, parser logic, or scheduler control flow.

All three production files make purely additive changes: a new enum, an optional field on an internal error, and new helper functions. The accumulation logic is deterministic and tested exhaustively across sync/async paths, correction steps, conversation restarts, both orderings of mixed truncation reasons, and the success-after-failure case. The Anthropic adapter change is backward-compatible — stop_reason absent from the response still yields finish_reason=None, matching prior behaviour.

No files require special attention.

Important Files Changed

Filename	Overview
packages/data-designer-engine/src/data_designer/engine/models/clients/adapters/anthropic_translation.py	Populates canonical choices[0].finish_reason from Anthropic stop_reason; existing message, usage, and raw fields unchanged.
packages/data-designer-engine/src/data_designer/engine/models/errors.py	Adds GenerationTruncationReason enum and optional truncation_reason on GenerationValidationFailureError; handle_llm_exceptions selects reason-specific solution text correctly.
packages/data-designer-engine/src/data_designer/engine/models/facade.py	Adds canonical-first / raw-fallback truncation classification, deterministic accumulation across retries, and threads truncation_reason into both raise paths of generate/agenerate.
packages/data-designer-engine/tests/engine/models/test_facade.py	Comprehensive coverage: single-attempt classification, raw fallback, canonical preference, accumulation across correction steps, context-window precedence, and success-after-truncation path.
packages/data-designer-engine/tests/engine/models/test_model_errors.py	New tests verify truncation_reason storage, reason-specific solution formatting, and that the public error surface remains unchanged.
packages/data-designer-engine/tests/engine/models/clients/test_anthropic_translation.py	Parameterised test confirms all four Anthropic stop reasons map 1:1 to canonical choices[0].finish_reason.
packages/data-designer-engine/tests/engine/dataset_builders/test_async_scheduler.py	Characterisation test verifies that the scheduler's existing non-retryable warning path surfaces the formatted max-tokens guidance without any scheduler-specific production changes.
plans/411/truncation-aware-parse-failure-guidance.md	Design plan document tracking the implementation; the explicitly excluded list for test_dataset_builder.py conflicts with the PR description (addressed in a previous review thread).

Sequence Diagram

sequenceDiagram
    participant Caller
    participant ModelFacade
    participant ModelClient
    participant Parser
    participant Errors as handle_llm_exceptions

    Caller->>ModelFacade: generate(prompt, parser)
    ModelFacade->>ModelClient: completion(request)
    ModelClient-->>ModelFacade: ChatCompletionResponse with finish_reason
    ModelFacade->>Parser: parser(response_text)
    Parser-->>ModelFacade: ParserException

    Note over ModelFacade: Classify finish_reason via canonical choices first, raw fallback if None
    Note over ModelFacade: Merge into accumulated truncation_reason, MODEL_CONTEXT_WINDOW_EXCEEDED wins

    alt more retries available
        ModelFacade->>ModelClient: completion(correction)
        ModelClient-->>ModelFacade: ChatCompletionResponse
        ModelFacade->>Parser: parser(response_text)
        Parser-->>ModelFacade: ParserException again
        Note over ModelFacade: Merge new reason into accumulated truncation_reason
    end

    ModelFacade->>Errors: raise GenerationValidationFailureError(truncation_reason)
    Errors-->>Caller: ModelGenerationValidationFailureError with reason-specific solution text

Loading

_{Reviews (3): Last reviewed commit: "fix: address #411 review follow-ups" | 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.

[andreatnvidia]

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.

[andreatnvidia]

@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.

[andreatnvidia]

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.

[stepwise-ai-dev]

The branch is now updated to current main and the approved plan is implemented in signed commits:

• 1c1ddb0d normalizes Anthropic stop_reason into the canonical choice finish_reason; direct parse_anthropic_response() coverage is in test_anthropic_translation.py as requested.
• 197c462b adds the typed reason only to the internal validation error and formats output-token versus context-window guidance at the existing public error boundary.
• 103a6adc classifies and accumulates reasons across sync/async correction and restart paths, preserves context-window precedence, and characterizes existing scheduler logging without changing scheduler production code.
• 7642a68c adds the requested raw-fallback rationale and records the completed plan-approval gate accurately.

The PR description now matches the implemented scope and removes the obsolete sync DatasetBuilder claims. All required checks on this head are passing. Local validation also completed with 288 focused tests, make check-engine, make check-all-fix, and the full suite (3,975 passed, 1 skipped).

@nabinchha, could you re-review the updated implementation when convenient?

[nabinchha]

Thanks for the careful rework, @stepwise-ai-dev — this is much cleaner and lines up well with the approved direction.

Summary

This revision normalizes Anthropic stop reasons, keeps truncation metadata internal, and surfaces reason-specific guidance through the existing public error boundary. The implementation matches the updated PR description and addresses the earlier feedback around helper reuse, raw-response fallback rationale, retry accumulation, and async scheduler propagation.

Findings

Warnings — Worth addressing

packages/data-designer-engine/src/data_designer/engine/models/facade.py:96 — Context-window precedence can outlive the conversation that exhausted it

• What: truncation_reason is accumulated globally across correction attempts and conversation restarts, and MODEL_CONTEXT_WINDOW_EXCEEDED permanently wins. A correction conversation can exhaust the context window, then a restart resets messages to restart_checkpoint, and the final fresh conversation can fail with max_tokens; the raised error still says increasing max_tokens will not help.
• Why: Once the restart drops the correction history, the earlier context-window limit may no longer constrain the final attempt. In that case increasing inference_parameters.max_tokens could be exactly the useful remedy, so the current guidance can send users in the wrong direction.
• Suggestion: Preserve both observed reasons (or reset conversation-local precedence on restart) and use combined guidance when context exhaustion occurred only in an earlier conversation. A focused regression with max_correction_steps=1, max_conversation_restarts=1, and finish reasons max_tokens → model_context_window_exceeded → max_tokens would exercise the misleading case.

What Looks Good

• The provider boundary now produces the same canonical choice metadata as the OpenAI-compatible path while preserving raw.
• The truncation reason remains internal, so the public exception and scheduler stay decoupled from provider termination details.
• The focused tests cover sync and async behavior, canonical-versus-raw precedence, successful recovery, formatting, and scheduler logging; all 288 touched-module tests pass locally.

Verdict

Needs changes — adjust mixed truncation guidance across conversation restarts so it does not categorically rule out increasing max_tokens after the context has been reset.

---

This review was generated by an AI assistant.