Skip to content

fix(checks): preserve partial trace on input generation failure - #2718

Merged
kevinmessiaen merged 4 commits into
Giskard-AI:mainfrom
dev404ai:fix/preserve-partial-trace-on-input-generation-failure
Aug 12, 2026
Merged

fix(checks): preserve partial trace on input generation failure#2718
kevinmessiaen merged 4 commits into
Giskard-AI:mainfrom
dev404ai:fix/preserve-partial-trace-on-input-generation-failure

Conversation

@dev404ai

@dev404ai dev404ai commented Aug 11, 2026

Copy link
Copy Markdown
Contributor

Description

When an InteractionSpec yielded an interaction and then raised, ScenarioRunner correctly recorded the input-generation error with return_exception=True, but its final_trace still pointed to the trace from before the spec started. The failing generator had already received the updated trace, so the returned error result lost observable partial progress and left last_interaction_index unset.

This change keeps the generator-driving loop in Trace.with_interaction. If a later yield fails, with_interaction raises InteractionGenerationError(partial_trace) from the original error. ScenarioRunner adopts that partial trace and continues to expose the original exception type, message, traceback, its own __cause__ chain, and return_exception=False behavior; the wrapper is suppressed from the reported context rather than shown as an extra frame.

Direct Trace.with_interaction(spec) and Trace.with_interactions(*specs) callers now receive InteractionGenerationError where the generator's own exception used to surface, so code catching a specific error there — for example InputGenerationException raised by LLMGenerator — needs except InteractionGenerationError and .__cause__. That is the intended consequence of keeping generator handling in one place. Concrete Interaction arguments keep the existing fast path and never raise it, so the in-tree giskard-scan call sites are unaffected, and CancelledError derives from BaseException and is not wrapped, so cancellation and generator cleanup are unchanged.

The existing runner regression now asserts the preserved interaction and its step index, and a second runner regression asserts that a chained generator failure reaches the caller unwrapped with its own cause intact. A direct Trace.with_interactions regression covers prior trace state, multiple successful yields, the original exception cause, and async-generator cleanup. This is a focused follow-up to the input-generation error handling introduced by #2523 and #2599; it does not reopen or replace either scope.

Maintainer-requested architecture

The initial patch duplicated generator advancement in ScenarioRunner and fixed only that call site. Review #4912803161 requested an exception carrying the partial trace so the protocol remains owned by Trace and future call sites can use the same behavior. The revised patch follows that direction: there is one generator loop, direct callers can recover completed progress, and the runner unwraps the chained cause so its external error contract remains stable.

AI assistance disclosure: I used AI coding assistants (Codex and Claude) to help investigate, implement, test, and review this change. I reviewed the resulting diff and verification evidence.

Related Issue

Follow-up to #2523 and #2599. There is no exact open issue for partial-trace loss.

Type of Change

  • 📚 Examples / docs / tutorials / dependencies update
  • 🔧 Bug fix (non-breaking change which fixes an issue)
  • 🥂 Improvement (non-breaking change which improves an existing feature)
  • 🚀 New feature (non-breaking change which adds functionality)
  • 💥 Breaking change (fix or feature that would cause existing functionality to change)
  • 🔐 Security fix

Verification

  • Focused direct and runner regressions: passed, including partial state, original cause, and generator cleanup.
  • Independent reproducer, parameterized by the expected number of preserved interactions: passes at 1 on the patched tree and fails at 0, so it discriminates the two states rather than only reporting one. Async-generator cleanup confirmed.
  • Cancellation, failure-phase, and repeated-state probes: passed, including generator closure under direct and bounded-parallel cancellation, phase and trace boundaries for setup/target/evaluator failures, and stable fresh-trace fingerprints across reused suites and concurrent runs of a shared scenario.
  • Memory scaling over 100-2,000 scenarios in isolated processes: linear, no reuse leak.
  • make format
  • make check
  • make test-unit PACKAGE=giskard-checks — 807 passed, 4 skipped
  • git diff --check

Checklist

  • I've read the CODE_OF_CONDUCT.md document.
  • I've read the CONTRIBUTING.md guide.
  • I've written tests for all new methods and classes that I created.
  • I've written the docstring in NumPy format for all the methods and classes that I created or modified.
  • I've updated the uv.lock running uv lock (only applicable when pyproject.toml has been modified). Not applicable: pyproject.toml was not modified.

@kevinmessiaen
kevinmessiaen self-requested a review August 12, 2026 03:28

@kevinmessiaen kevinmessiaen left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey @dev404ai

Thanks for the contribution

While the fix is good, I would probably let with_interaction raise an InteractionGenerationError(partial_trace) in order to keep the interaction handling the generator logic and easily apply same logic on potential future call sites

@dev404ai

Copy link
Copy Markdown
Contributor Author

Thanks — reworked in that direction.

Trace.with_interaction now raises InteractionGenerationError(partial_trace) from the original generator error. ScenarioRunner adopts the partial trace while preserving its existing exception behavior: with return_exception=False, it re-raises the original generator exception with its own cause chain intact; with return_exception=True, it records that original exception in the result.

One compatibility point to confirm: direct Trace.with_interaction(spec) and Trace.with_interactions(*specs) callers now receive InteractionGenerationError instead of the generator exception itself, which remains available as .__cause__. This follows the architecture you suggested and is documented in the updated PR description. Concrete Interaction arguments keep the existing fast path, so the in-tree giskard-scan call sites are unaffected.

I kept the review follow-up as a second commit so the delta is easy to inspect. Happy to squash if you prefer a single commit.

kevinmessiaen and others added 2 commits August 12, 2026 14:55
Two follow-ups to the InteractionGenerationError work on this PR.

Trace.with_interaction is recursive and its `except Exception` sat at
every level, so a spec driving another spec through the trace had its
inner wrapper caught and re-wrapped. The runner unwraps one level, so
the generic wrapper reached the caller and the inner spec's completed
interactions were dropped. The inner wrapper's partial trace is built
from the outer one, so it already holds strictly more progress and the
root cause: re-raise it untouched instead.

ScenarioRunner re-raised with `raise error from error.__cause__`, which
becomes `raise error from None` when the generator chained its error
implicitly (a bare `raise` inside an `except`). That set
__suppress_context__ and hid the root cause on the raise path while
return_exception=True still reported it. Restore the original context
links around the re-raise so both paths agree.

Adds regressions for nested composition, implicit context chaining, and
a generator that fails before its first yield.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@kevinmessiaen
kevinmessiaen merged commit 6347828 into Giskard-AI:main Aug 12, 2026
32 of 55 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Development

Successfully merging this pull request may close these issues.

2 participants