Skip to content

fix: improve error handling in policy generation and rollout - #24

Merged
tangym merged 2 commits into
mainfrom
yemingtang/error-handling-fixes
May 8, 2026
Merged

fix: improve error handling in policy generation and rollout#24
tangym merged 2 commits into
mainfrom
yemingtang/error-handling-fixes

Conversation

@tangym

@tangym tangym commented May 6, 2026

Copy link
Copy Markdown
Collaborator

Summary

Fix two error handling issues that caused noisy failures and poor user experience during pipeline runs.

Changes

Commit 1: Retry structured policy generation on transient parse failures

Problem: The policy stage calls generate_structured() which can return a valid HTTP response with malformed JSON that doesn't parse into a policy dict. This immediately raised ValueError and failed the pipeline, even though a simple retry usually succeeds.

Fix:

  • Retry structured generation up to 2 attempts on parse failure
  • Log a warning on each retry attempt
  • Include actionable hint in error message when retries exhausted: "rerun the command to retry" and "check your endpoint's token rate limit and quota"
  • Include last response text (truncated) for debugging

Note: The rate limiter (_with_retries inside generate_structured()) handles HTTP-level errors (429/5xx). This retry is a separate layer for content-quality failures where the HTTP call succeeded but the output is unusable.

Commit 2: Propagate LLM errors from rollout instead of burying in transcript

Problem: Two broad except Exception blocks in rollout.py caught classified LLM errors (LLMRateLimitError, LLMAuthError, etc.) and either:

  • Treated rate-limit errors as "bad auditor output" and retried with a guidance prompt (auditor loop, L659)
  • Buried LLM errors as [TARGET ERROR] in the transcript, then surfaced them as RuntimeError: scenario ended with target_error with a noisy traceback (target turn loop, L741)

Fix: Add explicit catch for (LLMAuthError, LLMInputError, LLMRateLimitError, LLMProviderError) before the broad except Exception that re-raises immediately. The runner's top-level handler at L393 already presents these with clean, actionable messages. Non-LLM errors (runtime crashes, JSON parse failures) still follow the existing transcript recording path.

Testing

uv run pytest -q  # 528 passed (1 pre-existing docker permission failure)

tangym added 2 commits May 6, 2026 21:29
The policy stage calls generate_structured() which can return valid HTTP
responses with malformed JSON that doesn't parse into a policy dict.
Previously this immediately raised ValueError and failed the pipeline.

- Retry structured generation up to 2 attempts on parse failure
- Log a warning on each retry attempt
- Include actionable hint in error message (rerun command, check endpoint
  quota) when retries are exhausted
- Include last response text (truncated) for debugging

The rate limiter (_with_retries) already handles HTTP-level errors (429,
5xx) inside generate_structured(). This retry is a separate layer for
content-quality failures where the HTTP call succeeded but the output
is unusable.
The auditor retry loop (L659) and target turn loop (L741) both used
broad 'except Exception' which caught LLMRateLimitError, LLMAuthError,
and other classified LLM errors. This caused:

- Rate-limit errors treated as 'bad auditor output' and retried with a
  guidance prompt instead of propagating for clean error display
- Auth errors wasting 2 more attempts before recording as [AUDITOR ERROR]
- LLM errors in target turns buried as [TARGET ERROR] in the transcript
  then surfaced as 'RuntimeError: scenario ended with target_error'
  with a noisy traceback instead of a clean one-line message

Fix: add an explicit catch for (LLMAuthError, LLMInputError,
LLMRateLimitError, LLMProviderError) before the broad 'except Exception'
that re-raises immediately. The runner's top-level handler at L393
already knows how to present these cleanly.

Non-LLM errors (e.g. target runtime crashes, JSON parse failures) still
follow the existing path — recorded in transcript and surfaced as
target_error.
@tangym

tangym commented May 7, 2026

Copy link
Copy Markdown
Collaborator Author

@microsoft-github-policy-service agree company="Microsoft"

@tangym
tangym merged commit da841a8 into main May 8, 2026
3 checks passed
AaronAspinwall123 added a commit that referenced this pull request May 8, 2026
Resolves conflicts in p2m/runner.py and p2m/stages/rollout.py introduced by main's centralized-logging refactor (PR #22) and policy/rollout error-handling improvements (PR #24).

p2m/runner.py: kept the artifact-cache stage path (prepare_artifact_plan / activate_artifact_plan / override_cacheable_output_paths) and combined it with main's logging style. The two stage-skip messages now use log.info with the new '[stage] Skipped' prefix instead of the deleted _progress() helper, and _progress() itself was removed since main's logging configuration writes to sys.__stderr__ via RichHandler, neutralizing the original Phoenix sys.stderr-wrapping concern.

p2m/stages/rollout.py: dropped the sys.__stderr__ rollout progress writer in favor of main's log.info / log.warning calls (same rationale). Kept the 're' import that the cache code uses for _VERSIONED_ARTIFACT_RE; dropped the now-unused 'sys' import.

Verified: pytest passes for tests/test_artifact_cache.py, tests/test_runner_artifact_cache.py, tests/test_runner_progress.py, and tests/test_viewer_server_artifacts.py (56 passed, 2 skipped).

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
@tangym
tangym deleted the yemingtang/error-handling-fixes branch May 9, 2026 00:48
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.

2 participants