Skip to content

refactor: enhance retry mechanism with flexible context management#8284

Open
xtco3o wants to merge 1 commit intoblock:mainfrom
xtco3o:refactor/retry-mechanism-feedback
Open

refactor: enhance retry mechanism with flexible context management#8284
xtco3o wants to merge 1 commit intoblock:mainfrom
xtco3o:refactor/retry-mechanism-feedback

Conversation

@xtco3o
Copy link
Copy Markdown

@xtco3o xtco3o commented Apr 3, 2026

Refactors the retry mechanism with flexible context management and enhanced safety.

Key Changes:

  • Flexible Context Control: Added reset_context support.
  • Rich Failure Feedback: Captures stdout, stderr, and exit_status.
  • Advanced Context Protection: Automatically truncates large outputs based on both line count (200) AND character count (20,000). This prevents LLM context overflow even from massive single-line outputs (like Base64 or minified JSON).
  • Privacy & UI Integrity: Marked retry feedback as agent_only() to keep UI clean.
  • Improved Type Safety: Introduced SuccessCheckResult and ExecutionDetails.

@xtco3o xtco3o changed the title refactor: improve retry mechanism feedback and code quality refactor: enhance retry mechanism with flexible context management Apr 3, 2026
Copy link
Copy Markdown

@chatgpt-codex-connector chatgpt-codex-connector bot left a comment

Choose a reason for hiding this comment

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

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 961b9855cc

ℹ️ About Codex in GitHub

Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".

@xtco3o xtco3o force-pushed the refactor/retry-mechanism-feedback branch from 961b985 to a97cba8 Compare April 3, 2026 11:26
@xtco3o
Copy link
Copy Markdown
Author

xtco3o commented Apr 3, 2026

All requested refactors have been completed:

  • Reduced Redundancy: Extracted append_output_details helper to unify output processing.
  • Improved Maintainability: Added MAX_RETRY_OUTPUT_LINES constant (200) for consistent log truncation.
  • Context Protection: Implemented automatic truncation of stdout/stderr to prevent token overflow during retries.
  • Verified: Unit tests for truncation logic have been added and passed.

@xtco3o xtco3o force-pushed the refactor/retry-mechanism-feedback branch from a97cba8 to e579037 Compare April 3, 2026 11:29
Copy link
Copy Markdown

@chatgpt-codex-connector chatgpt-codex-connector bot left a comment

Choose a reason for hiding this comment

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

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: e579037a3d

ℹ️ About Codex in GitHub

Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".

@xtco3o xtco3o force-pushed the refactor/retry-mechanism-feedback branch 2 times, most recently from 53da0c8 to 2bf500b Compare April 3, 2026 11:44
@xtco3o
Copy link
Copy Markdown
Author

xtco3o commented Apr 3, 2026

Resolved Codex Review:
Non-zero exit status for on_failure commands is now correctly treated as a terminal error. This prevents the retry logic from proceeding with a failed recovery state.

  • Updated execute_on_failure_command to return Err on non-zero exit.
  • Updated related unit tests to reflect this change.

@xtco3o xtco3o force-pushed the refactor/retry-mechanism-feedback branch from 2bf500b to 091b5e5 Compare April 3, 2026 11:45
Copy link
Copy Markdown

@chatgpt-codex-connector chatgpt-codex-connector bot left a comment

Choose a reason for hiding this comment

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

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 091b5e5bc8

ℹ️ About Codex in GitHub

Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".

@xtco3o xtco3o force-pushed the refactor/retry-mechanism-feedback branch 2 times, most recently from 183b468 to 5a0749e Compare April 3, 2026 11:53
Copy link
Copy Markdown
Collaborator

@DOsinga DOsinga left a comment

Choose a reason for hiding this comment

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

Review

Thanks for the contribution — the reset_context feature is a useful idea. A few things need addressing before this can move forward:

Needs an issue first

This adds a new feature (the reset_context field and the feedback-loop retry path) without a prior issue or discussion. Per our contributing guidelines, please open an issue describing the use case and proposed design so we can discuss it before the code: https://github.com/block/goose/blob/main/CONTRIBUTING.md#discussions-issues-and-prs

on_failure error handling is broken in the non-reset path

Codex flagged this correctly. execute_on_failure_command returns Err on non-zero exit, and handle_retry_logic propagates it with ?:

let on_failure_details = if let Some(on_failure_cmd) = &retry_config.on_failure {
    Some(execute_on_failure_command(on_failure_cmd, retry_config).await?)
} else {
    None
};

This means when reset_context: false and the on_failure command fails, the retry aborts entirely. The code below that builds a message with "with failure" status is dead — it can never be reached. You need to decide: should a failing on_failure command abort the retry, or should its output be fed back to the agent? The current code tries to do both and achieves neither.

Remove test_retry_result_enum

This test just asserts that enum variants are not equal to each other and that Clone/Debug work. It can never catch a real bug — please remove it.

Integration tests needed

The new reset_context: false path has no integration test that exercises handle_retry_logic end-to-end. crates/goose/tests/agent.rs already has a retry_tests module — please add tests there (following the mock-provider pattern from max_turns_tests) that cover:

  • reset_context: true resets conversation history (existing behavior, but worth pinning)
  • reset_context: false appends failure feedback as an agent-only user message with stdout/stderr
  • reset_context: false with a failing on_failure command (once you fix the error handling)
  • truncation actually kicks in when output exceeds the limits

Minor

  • Comments like // Execute on_failure if configured and // Add Success Check failure info just restate what the next line does — remove them.
  • truncate_output mixes len() (bytes) with the "chars" label in the truncation message — pick one unit and be consistent.

Copy link
Copy Markdown

@chatgpt-codex-connector chatgpt-codex-connector bot left a comment

Choose a reason for hiding this comment

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

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: e809fe9239

ℹ️ About Codex in GitHub

Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".

Copy link
Copy Markdown

@chatgpt-codex-connector chatgpt-codex-connector bot left a comment

Choose a reason for hiding this comment

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

💡 Codex Review

messages_to_add = Conversation::default();

P2 Badge Preserve failed assistant turn when reset_context is false

In the retry path, handle_retry_logic runs before messages_to_add is merged into conversation, and when a retry is requested this branch unconditionally clears messages_to_add. With reset_context: false, that drops the just-produced assistant response from persisted history, so the next attempt only sees synthetic failure feedback and loses the model’s immediate prior turn, which defeats the intended context-preserving behavior.

ℹ️ About Codex in GitHub

Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".

@xtco3o xtco3o force-pushed the refactor/retry-mechanism-feedback branch from d3d2577 to 5556cc6 Compare April 4, 2026 11:27
Copy link
Copy Markdown

@chatgpt-codex-connector chatgpt-codex-connector bot left a comment

Choose a reason for hiding this comment

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

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 5556cc6fab

ℹ️ About Codex in GitHub

Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".

Copy link
Copy Markdown

@chatgpt-codex-connector chatgpt-codex-connector bot left a comment

Choose a reason for hiding this comment

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

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: f67e6e97bd

ℹ️ About Codex in GitHub

Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".

Copy link
Copy Markdown

@chatgpt-codex-connector chatgpt-codex-connector bot left a comment

Choose a reason for hiding this comment

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

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 0e26f05bbc

ℹ️ About Codex in GitHub

Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".

…edback

Signed-off-by: ai <myaier@googlegroups.com>
@xtco3o xtco3o force-pushed the refactor/retry-mechanism-feedback branch from f260f57 to 577d4a6 Compare April 4, 2026 14:27
Copy link
Copy Markdown

@chatgpt-codex-connector chatgpt-codex-connector bot left a comment

Choose a reason for hiding this comment

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

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 577d4a652b

ℹ️ About Codex in GitHub

Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".

Comment on lines +1746 to +1748
// above (std::mem::take), so we must save even when retry added
// nothing — otherwise the assistant reply is lost from session.
session_manager.replace_conversation(&session_config.id, &conversation).await?;
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P2 Badge Keep append-only persistence on non-retry turns

This branch now persists with replace_conversation even when retry logic returns false because it was skipped or all checks passed, so ordinary no-tool turns rewrite the entire transcript. Since replace_conversation deletes and reinserts all session messages, long chats now pay O(n) database writes per turn (O(n²) across a session), which can materially slow interactions and increase lock contention compared with the previous append-only add_message path.

Useful? React with 👍 / 👎.

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