fix(delegate_tool): address PR #43134 multi-provider review feedback#43185
Open
rafaumeu wants to merge 2 commits into
Open
fix(delegate_tool): address PR #43134 multi-provider review feedback#43185rafaumeu wants to merge 2 commits into
rafaumeu wants to merge 2 commits into
Conversation
added 2 commits
June 9, 2026 19:55
Add optional model and provider fields to individual tasks in delegate_tool batch mode. Fallback chain: per-task field > delegation config > parent model. Closes NousResearch#18591
Fixes from 8-reviewer multi-provider code review (ricardocamiloconsir):
1. [BLOCKER] Fix stale `creds` reference in override_acp_command
- Changed `creds.get('command')` to `task_creds.get('command')`
- When per-task provider override is used, ACP command now comes from
the correct per-task credentials, not the delegation-level ones
- Regression test added (TestStaleCredsReferenceRegression)
2. [SHOULD FIX] Narrow broad `except Exception` in _resolve_task_credentials
- Split into ValueError, (KeyError, LookupError), and catch-all
- Added logger.exception() for unexpected errors (was silent before)
- Preserves full tracebacks in observability tooling
3. [SHOULD FIX] Document fail-fast batch abort semantics
- Added inline comment explaining that credential resolution failure
for task N aborts the entire batch
- References PR NousResearch#43134 review discussion for future improvement
4. [NICE TO HAVE] Add .strip() to model/provider inputs
- Guards against whitespace-only strings like ' '
- Test added (test_whitespace_model_stripped_before_resolution)
16 tests passing (14 original + 2 regression).
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
2 participants
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.
Summary
Addresses all actionable items from the 8-reviewer multi-provider code review on PR #43134 (by @ricardocamiloconsir).
This PR is a follow-up to #43134. It includes the original feature PLUS all review fixes.
Fixes Applied
credsreference inoverride_acp_commandcreds.get("command")→task_creds.get("command")except Exceptionswallows unexpected errorsValueError,(KeyError, LookupError), and catch-all withlogger.exception().strip()to both inputsNew Tests
TestStaleCredsReferenceRegression::test_task_creds_used_for_acp_command_fallback— verifies ACP command comes from per-task credsTestStaleCredsReferenceRegression::test_whitespace_model_stripped_before_resolution— verifies" "treated as empty16/16 tests passing (14 original + 2 regression).
Reviewer Scorecard Addressed
credsreference bug → FIXEDexcept Exceptionblocking → FIXED (narrowed + logging)dict | strreturn type → NOT CHANGED (follow-up item)Closes #18591. Supersedes #43134.