Skip to content

Centralize tool result error handling in format with error_line helper#948

Merged
LasmarKhalifa merged 1 commit into
mainfrom
06-15/tool-result-error-handling
Jun 25, 2026
Merged

Centralize tool result error handling in format with error_line helper#948
LasmarKhalifa merged 1 commit into
mainfrom
06-15/tool-result-error-handling

Conversation

@LasmarKhalifa

@LasmarKhalifa LasmarKhalifa commented Jun 15, 2026

Copy link
Copy Markdown
Contributor

Part of #919

Foundation PR for the tool result formatter implementations.

@juniper-shopify juniper-shopify left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Nice foundation PR! The error-handling centralization is clean — pulling the error path into an early-return at the top of format means every downstream formatter only needs to think about the success case. Makes the rest of your stack cleaner.

Two small things to address:

  1. The error_line doc comment could use a couple of examples (see inline comment).
  2. A test for error_line when content is nil would round out the edge-case coverage.

And lets switch to the xml parser we had talked about

Comment thread lib/roast/cogs/agent/providers/claude/tool_result.rb
Comment thread test/roast/cogs/agent/providers/claude/tool_result_test.rb
Comment thread lib/roast/cogs/agent/providers/claude/tool_result.rb
@LasmarKhalifa LasmarKhalifa force-pushed the 06-15/tool-result-error-handling branch 5 times, most recently from cc4bb2a to 2408fb9 Compare June 23, 2026 18:33
@LasmarKhalifa LasmarKhalifa marked this pull request as ready for review June 23, 2026 18:40

LasmarKhalifa commented Jun 25, 2026

Copy link
Copy Markdown
Contributor Author

Merge activity

  • Jun 25, 6:03 PM UTC: A user started a stack merge that includes this pull request via Graphite.
  • Jun 25, 6:04 PM UTC: Graphite rebased this pull request as part of a merge.
  • Jun 25, 6:06 PM UTC: Graphite rebased this pull request as part of a merge.
  • Jun 25, 6:06 PM UTC: @LasmarKhalifa merged this pull request with Graphite.

@LasmarKhalifa LasmarKhalifa force-pushed the 06-15/tool-result-error-handling branch from 2408fb9 to b189337 Compare June 25, 2026 18:03
@LasmarKhalifa LasmarKhalifa force-pushed the 06-15/tool-result-error-handling branch from b189337 to ebba0ec Compare June 25, 2026 18:05
@LasmarKhalifa LasmarKhalifa merged commit acfdc0c into main Jun 25, 2026
3 checks passed
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