Skip to content

fix(core): return True when image-strip retry succeeds in strip_all_images_from_session#591

Open
Rome-1 wants to merge 1 commit into
usestrix:mainfrom
Rome-1:fix/strip-images-retry-return
Open

fix(core): return True when image-strip retry succeeds in strip_all_images_from_session#591
Rome-1 wants to merge 1 commit into
usestrix:mainfrom
Rome-1:fix/strip-images-retry-return

Conversation

@Rome-1

@Rome-1 Rome-1 commented Jun 26, 2026

Copy link
Copy Markdown

What & why

strip_all_images_from_session is the recovery path invoked by _run_cycle (strix/core/execution.py) when the model rejects image input (HTTP 400/404/422): it rebuilds the session with images swapped for placeholders and re-adds the items. The retry block re-added the items inside contextlib.suppress(Exception) and then unconditionally raised, which produced two bad outcomes:

  • A successful retry still raised the original error, so the caller logged "image-strip recovery failed", set stripped = False, and threw away a session that had actually been rebuilt correctly — the scan never retried.
  • A double failure swallowed the genuine second error (the real reason the re-add failed) and re-raised the stale first one.

This change drops the suppress/raise dance: the first attempt runs, and on failure a single retry runs outside any suppression. A successful retry now falls through to return True so recovery proceeds; a failed retry propagates the genuine second exception. Behaviour on the first-attempt-success path is unchanged.

Testing

  • pytest tests/test_sessions.py — 2 passed. The new tests script a session whose add_items fails once then succeeds (asserts True is returned and add_items was called twice) and one that fails twice (asserts the second exception propagates). Both tests fail against the current main implementation and pass with this change.
  • ruff check / ruff format --check — clean (the repo enables the UP/pyupgrade and S/bandit rule sets; the intentional broad-except retry carries a scoped # noqa: BLE001).
  • bandit — no findings; pyupgrade --py310-plus — no changes.
  • mypy is not asserted: the repo's pinned pre-commit mypy hook crashes with an internal error on the bundled openai SDK on main as well, so this PR makes no mypy claim.

…_images_from_session

The unconditional re-raise after the suppressed retry meant a successful
retry still signalled failure (recovery in _run_cycle was discarded) and a
double failure swallowed the genuine second error behind the suppressed
original. Retry directly so a successful retry falls through to return True
and a failed retry propagates the real second exception.
@greptile-apps

greptile-apps Bot commented Jun 26, 2026

Copy link
Copy Markdown
Contributor

Greptile Summary

This PR fixes a logic inversion in the strip_all_images_from_session retry path where a successful retry still re-raised the original exception, causing the caller to discard a correctly-rebuilt session.

  • strix/core/sessions.py: Drops the contextlib.suppress + unconditional raise pattern and replaces it with a bare retry call. On success the retry falls through to return True; on failure the second exception propagates naturally instead of being swallowed and replaced by the stale first one.
  • tests/test_sessions.py: Adds two async tests via a minimal _FakeSession stub — one asserting True is returned and add_items called twice when the retry succeeds, one asserting the second exception propagates on double failure. Both rely on asyncio_mode = "auto" already configured in pyproject.toml, so no additional markers are needed.

Confidence Score: 5/5

Safe to merge — the change is minimal, directly targeted at a well-described logical inversion, and is covered by new tests that exercise both the success and double-failure paths.

The old suppress/raise dance meant a successful retry threw away a correctly-rebuilt session; the new code correctly falls through to return True. The fix touches only four lines, removes complexity, and the two new tests would have caught the original bug. No other behaviour on the success path is altered.

No files require special attention.

Important Files Changed

Filename Overview
strix/core/sessions.py Fixes the retry logic in strip_all_images_from_session so a successful retry now returns True instead of unconditionally re-raising the original exception.
tests/test_sessions.py New test file with two async tests covering the fixed retry paths; asyncio_mode = "auto" in pyproject.toml ensures they execute correctly.

Reviews (1): Last reviewed commit: "fix(sessions): return True when add_item..." | Re-trigger Greptile

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant