Repository navigation
feat(agent): add registered business outcome mapping and outcome consistency fixes (T38) - #167
Merged
yuluo-yx merged 1 commit intoOct 15, 2026
Conversation
…istency fixes (T38)
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Describe what this PR does / why we need it
Second and closing part of T38 (#141), following the "keep it simple" agreement: a registered business-outcome mapper is now the only component allowed to reinterpret a successful raw tool result as a business failure (no global string guessing), the reserved
ToolOutcome.BUSINESS_FAILEDvocabulary gets its first producer, and three outcome-consistency gaps found along the way are closed.Does this pull request fix one issue?
Fixes #141
Describe how you did it
BusinessOutcomeMapper(functional interface) +BusinessOutcomeMapperRegistrymirroringToolEffectRegistry:builder().declare(toolName, mapper),fromTools(...)picking upBusinessOutcomeAwaretools,merge(). The default registry is empty and results of unregistered tools are never inspected (a test feeds a{"error": true}payload to an unregistered tool and asserts it stays a plain success).ToolCallResponse.from(...): when a registered mapper marks a successful raw result as error, the response is built with statusToolOutcome.BUSINESS_FAILED— the vocabulary reserved in PR feat(agent): unify tool failure status contract and remove null-status error paths (T38) #159 now has its first producer.isError()recognizesBUSINESS_FAILEDas a failure, so the model-visible message derived from the response carriestool.error=true— conversions no longer lose the outcome (one of the issue's acceptance criteria).LargeResultEvictionInterceptorkeeps the original response metadata instead of dropping it when evicting a large result.ToolEmulatorInterceptorreturnssuccess(...)instead of the legacyof(...): an emulated result is a successful outcome and must not look like a failure to retry interceptors.ToolRetryInterceptor's exhausted-retries exit carries forward the last failure's structuredfailureKind/exceptionTypeentries.Describe how to verify it
New tests:
BusinessOutcomeMapperRegistryTest(6 cases: no-guessing, declare, fromTools, merge, null-mapper rejection, argument validation),AgentToolNodeBusinessOutcomeTest(4 cases run end to end through the node, assertingBUSINESS_FAILED+ metadata + model-visibletool.error),LargeResultEvictionInterceptorTest(metadata preservation); plus one new retry case and one emulator status assertion.Run:
Full regression: graph-core 701 + agent-framework 1122 tests; only the known pre-existing Windows-environment failures remain (the FileSystem* symlink / virtual-root family and the skills/prompt-template cases reproduce on clean main, as documented in feat(agent): unify tool failure status contract and remove null-status error paths (T38) #159).
Special notes for reviews
SUCCESS_STATUS(previously no status, so retry interceptors retried them as failures), andisError()returns true forBUSINESS_FAILED.BUSINESS_FAILEDresponses are notSUCCESS_STATUS, so retry interceptors retry them like other failures; if a business failure should be terminal for the retry policy, the mapper can keep returning a success result. Happy to adjust."evicted_to_filesystem"intentionally stays outside theToolOutcomevocabulary (it is a success variant, not a failure); let me know if you would rather fold it in or map it toSUCCESS.