chore(engine): rename correction-step counter for clarity

[nabinchha]

Summary

Closes #371.

The local counter curr_num_correction_steps in ModelFacade.generate and ModelFacade.agenerate was incremented before each parse attempt, so it actually counted parse attempts (initial + corrections), not corrections taken. The name suggested the latter, which made the surrounding <= max_correction_steps check read like a possible off-by-one even though the math worked out (with max_correction_steps=1 you get 1 initial + 1 correction = 2 attempts, which is the intended behavior).

This PR:

• Renames the local variable curr_num_correction_steps → parse_attempts in both the sync (generate) and async (agenerate) code paths.
• Adds a brief comment near the declaration in generate describing the semantics, and a back-reference comment in agenerate.
• Leaves the public API (max_correction_steps, max_conversation_restarts) and behavior unchanged. The error message text ("…despite N correction steps…") is also unchanged.

I went with the rename rather than moving the increment into the except block, because moving the increment would change behavior when max_correction_steps == 0 (no early-raise short-circuit reached) and would interact subtly with the restart-reset path. The rename captures the actual semantics without touching control flow.

Test plan

• make lint / ruff check clean on facade.py
• pytest packages/data-designer-engine/tests/engine/models/test_facade.py — 68 passed
• pytest packages/data-designer-engine/tests — 1975 passed
• Spot-checked the parametrized correction-step tests in test_facade.py (e.g. total_calls for (max_correction_steps, max_conversation_restarts) combinations) — still pass, confirming behavior is unchanged.

[greptile-apps]

Greptile Summary

Renames the local counter curr_num_correction_steps to parse_attempts in both the sync generate and async agenerate methods of ModelFacade, clarifying that the variable tracks all parse attempts (initial + corrections) rather than just corrections. No logic, public API, or behavior changes are introduced.

• parse_attempts replaces curr_num_correction_steps in all declaration, increment, comparison, and reset sites across both code paths.
• A semantics comment is added in generate; agenerate cross-references it with a back-pointer comment.

Confidence Score: 5/5

Safe to merge — this is a local variable rename with no behavior changes.

The change touches only two local variable names across the sync and async code paths, with identical rename applied at every declaration, increment, comparison, and reset site. No remaining references to the old name exist in the codebase, and the surrounding control flow is untouched.

No files require special attention.

Important Files Changed

Filename	Overview
packages/data-designer-engine/src/data_designer/engine/models/facade.py	Pure rename of local variable curr_num_correction_steps → parse_attempts in both generate and agenerate; no logic changes, no remaining references to the old name.

Flowchart

%%{init: {'theme': 'neutral'}}%%
flowchart TD
    A[Start: parse_attempts=0, curr_num_restarts=0] --> B[Call LLM completion]
    B --> C{Tool calls?}
    C -- Yes --> D[Process tool calls\nupdate checkpoint] --> B
    C -- No --> E[Append assistant message\nparse_attempts += 1]
    E --> F{parser succeeds?}
    F -- Yes --> G[Return output_obj]
    F -- No: ParserException --> H{max_correction_steps == 0\nAND max_conversation_restarts == 0?}
    H -- Yes --> I[Raise immediately]
    H -- No --> J{parse_attempts <= max_correction_steps?}
    J -- Yes --> K[Append user error message\nfor correction] --> B
    J -- No --> L{curr_num_restarts < max_conversation_restarts?}
    L -- Yes --> M[parse_attempts = 0\ncurr_num_restarts += 1\nRestore checkpoint] --> B
    L -- No --> N[Raise GenerationValidationFailureError]

Loading

_{Reviews (1): Last reviewed commit: "chore(engine): rename correction-step co..." | Re-trigger Greptile}

[github-actions]

PR #627 Review — chore(engine): rename correction-step counter for clarity

Summary

Pure local-variable rename in ModelFacade.generate and ModelFacade.agenerate: curr_num_correction_steps → parse_attempts. Adds a clarifying comment in the sync path and a back-reference comment in the async path. No behavior change; public API (max_correction_steps, max_conversation_restarts) and the error message text are untouched. Closes #371.

Findings

Correctness

• Rename is complete and consistent. Verified via grep: zero remaining curr_num_correction_steps references repo-wide, and all six occurrences in both generate and agenerate are renamed symmetrically (declaration, increment, bound check, restart reset).
• Control flow is unchanged — no reordering of the increment vs. the <= max_correction_steps check, no change to the max_correction_steps == 0 and max_conversation_restarts == 0 short-circuit, no change to the restart-reset path. The PR author explicitly rejected the alternative of moving the increment into except, with a correct rationale (would change behavior at max_correction_steps == 0 and interact with the restart path).
• The new name accurately describes the semantics. The counter is incremented before each parse attempt, so with max_correction_steps=1 the sequence is: attempt 1 → parse_attempts=1, 1 <= 1 True → inject correction → attempt 2 → parse_attempts=2, 2 <= 1 False → fall through. That matches "initial + max_correction_steps corrections" as the comment claims.
• The existing error message "Unsuccessful generation despite {max_correction_steps} correction steps…" still reads correctly because it references the public config value, not the internal counter.

Code quality & conventions

• Follows AGENTS.md invariants: typed code unchanged, from __future__ import annotations already present, absolute imports preserved.
• The comment is appropriately narrow — it explains the why (why the bound check reads as it does given pre-increment), which is exactly what the style guide prefers over restating the what.
• The back-reference in agenerate ("See generate for a description…") is a reasonable DRY choice, though if generate is ever deleted or split the pointer rots silently. Minor — not worth changing.

Risk / blast radius

• Minimal. Local variable only; not exposed in any public surface, not used in any serialization format, not logged. Blast radius is confined to the two methods.
• Scope creep: none. The PR resists the temptation to also move the increment or rename the public parameter, which is the right call for a chore/clarity PR.

Tests

• No new tests required — behavior is unchanged. PR description confirms the full engine suite (1975 tests) still passes, including the parametrized correction-step tests in test_facade.py.
• No test file references the renamed variable (it was always local), so nothing had to move.

Nits (optional, non-blocking)

• The comment phrase "permits exactly max_correction_steps corrections after the initial attempt" is accurate but a bit dense. A reader stepping through the first time may find it easier as: "the initial attempt is counted as attempt 1, so the bound permits max_correction_steps subsequent correction attempts." Matter of taste.
• The two occurrences of parse_attempts = 0 (declaration + restart reset) are easy to miss if someone later refactors — consider a tiny helper or a named constant later, but not in this PR.

Verdict

Approve — merge as-is. Small, well-motivated, correctly-scoped cleanup. The rename closes the readability gap flagged in #371 without changing behavior, and the author chose the safer of the two possible fixes (rename vs. restructure) with clear reasoning. No issues found.