feat(agents): add random backend-selection strategy for virtual provider LLM agents #158
No reviewers
Labels
No labels
auto/blocked-by-deps
auto/ci-timeout
auto/claimed-implementer
auto/claimed-merge
auto/claimed-reviewer
auto/driver-down
auto/invariant-violation
auto/last-attempt-tier-0
auto/last-attempt-tier-1
auto/last-attempt-tier-2
auto/last-attempt-tier-min
Automation Tracking
auto/needs-conflict-resolution
auto/needs-implementer
auto/postmortem
auto/ready-to-merge
auto/restart-throttled
auto/revert
auto/sentinel
auto/stale-inactivity
auto/unstable
Blocked
Bounty
$100
Bounty
$1000
Bounty
$10000
Bounty
$20
Bounty
$2000
Bounty
$250
Bounty
$50
Bounty
$500
Bounty
$5000
Bounty
$750
MoSCoW
Could have
MoSCoW
Must have
MoSCoW
Should have
Needs Feedback
Points
1
Points
13
Points
2
Points
21
Points
3
Points
34
Points
5
Points
55
Points
8
Points
88
Priority
Backlog
Priority
CI Blocker
Priority
Critical
Priority
High
Priority
Low
Priority
Medium
Signed-off: Owner
Signed-off: Scrum Master
Signed-off: Tech Lead
Spike
State
Completed
State
Duplicate
State
In Progress
State
In Review
State
Paused
State
Unverified
State
Verified
State
Wont Do
Type
Automation
Type
Bug
Type
Discussion
Type
Documentation
Type
Epic
Type
Feature
Type
Legendary
Type
Refactor
Type
Support
Type
Task
Type
Testing
No project
No assignees
2 participants
Notifications
Due date
No due date set.
Blocks
#156 Add random backend-selection strategy for virtual provider LLM agents
cleveragents/cleveractors-core
Reference
cleveragents/cleveractors-core!158
Loading…
Add table
Add a link
Reference in a new issue
No description provided.
Delete branch "feature/m1-virtual-provider-random-strategy"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
Summary
Implements issue #156 (the second increment of Epic #154, ADR-2038), extending the
backends/strategyvirtual-provider routing introduced in #155 (PR #157) with the second required selection strategy:strategy: random.strategy: randomis now accepted by both validation paths (validate_agentsandbuild_virtual_chat_model's own defensive validation) --VALID_VIRTUAL_PROVIDER_STRATEGIESnow includes"random"alongside"round_robin".RandomStrategy(newSelectionStrategyimplementation, Strategy pattern, same precedent asRoundRobinStrategy) selects a backend index viarandom.randrange, with a# nosec B311justification comment -- this is a capacity-distribution concern, not a security boundary (ADR-2038 "Selection strategies"). UnlikeRoundRobinStrategy, no concurrency guard is needed: there is no shared position to advance.strategyvalues other thanround_robin/randomstill raiseConfigurationErrornaming the unsupported value -- two existing scenarios that used"random"as an example of an unsupported value now use"weighted"instead.round_robinand untouched by this change -- onlynext_index's algorithm differs.docs/index.md: Version bumped 1.8.0 -> 1.9.0, new §21.1 revision-history row (the §4.4.7 body text already documentedrandom's semantics ahead of time).docs/guides/virtual-provider-backend-routing.mdandexamples/packages/virtual-provider-pooled-agent.yamlupdated to showrandomis now supported (previously documented as "planned as a follow-up").benchmarks/virtual_provider_benchmark.py): addedRandomStrategyBenchmarkand a random-strategyVirtualChatModel._selectcase.Test plan
nox -s lint-- greennox -s format -- --check-- greennox -s typecheck-- green (Pyright strict, zero suppressions)nox -s unit_tests-- green, full suite (155 features / 3152 scenarios)nox -s coverage_report-- 97% (threshold 96.5%)nox -s security_scan-- green (bandit + semgrep); bandit B311 suppressed with a justification commentnox -s dead_code-- green (vulture)nox -s integration_tests-- green, full suite (368 tests), including a new Robot test case exercisingstrategy: randomthrough the library API (Executor -> LLMAgent -> VirtualChatModel -> RandomStrategy)nox -s docs-- mkdocs build greenCloses #156
PR Review: !158 (Ticket #156)
Verdict: Approve
Reviewed the full branch (single atomic commit
4e2cb1a, first line matching the ticket's Commit Message exactly, rebase-clean ondevelop), the ticket #156 acceptance criteria, Epic #154, ADR-2038, anddocs/index.md§4.4.7. There are no prior review comments or author responses on this PR (and none on the ticket), so nothing was decided out-of-scope or deferred — the full review stands.The implementation satisfies every acceptance criterion:
strategy: randomis accepted by both validation paths (VALID_VIRTUAL_PROVIDER_STRATEGIESandbuild_virtual_chat_model's defensive check);RandomStrategyselects uniformly viarandom.randrangewith the required# nosec B311justification comment and correctly needs no concurrency guard (stateless, unlikeRoundRobinStrategy); unsupported values still fail fast naming the value (the two scenarios that used"random"as the unsupported example were correctly switched to"weighted"); statistical-coverage and exactly-one-backend Behave scenarios plus a full-chain Robot integration test are added; docs are consistent throughout (standard bumped 1.8.0 → 1.9.0 in both places, §21.1 revision-history row added, guide/example/CHANGELOG updated, no stale "planned as a follow-up" references remain anywhere). Benchmarks cover the new strategy. No correctness, security, or performance issues found.Critical Issues
None.
Major Issues
None.
Minor Issues
features/virtual_provider.feature:129-132("Random strategy forwards each call to exactly one backend") asserts only call counts, androbot/virtual_provider_routing.robot:28-39asserts onlyResult Is Valid Actor Resultplus per-backend request counts — yet the Robot keyword's docstring claims it proves "that backend's response is returned unmodified". The ticket's acceptance criterion explicitly requires "forwards the call/response unmodified". Both layers already hold the data to assert this cheaply: in Behave, assertcontext.invocation_order == [<the called backend's name>]; in Robot, assert the response matches whichever backend received the request (e.g.final-answer-from-backend-0/-1). The forwarding code is shared with round_robin (whose tests do assert content), so this is a test-strengthening suggestion, not a bug.Nits
benchmarks/virtual_provider_benchmark.py:47-49—_make_virtual_chat_model'sstrategyparameter is typed as the concrete unionRoundRobinStrategy | RandomStrategy | None; theSelectionStrategyABC (importable from the same module) is the natural type and keeps a future third strategy from touching this signature (the extensibility rationale in ADR-2038 A-5).src/cleveractors/agents/llm_virtual_provider.py:130— at 121 characters this is now the longest line insrc/. Lint/format pass (E501 is ignored project-wide and the formatter doesn't split trailing comments), but putting the# nosec B311justification on its own line above thereturnwould keep the file within the configured 88-column style.src/cleveractors/agents/llm_virtual_provider.py:102-105 and 126-129— thenum_backends <= 0→ConfigurationError("num_backends must be positive, ...")guard is duplicated across both strategies; hoisting it intoSelectionStrategy(or a shared helper) would prevent a third copy whenweightedlands.robot/virtual_provider_routing.robot:3-7androbot/VirtualProviderRoutingTestLib.py:1-25still describe the suite as exercising "the real round-robin routing code" /RoundRobinStrategyonly; it now also coversRandomStrategy.strategy: random. This is defensible as-is — retry/streaming pinning lives inVirtualChatModel/call_with_retryand is strategy-independent, and it is covered by the round_robin scenarios — so no action is strictly required; noting it only because it maps to an explicit acceptance criterion of #156.Summary
A clean, well-scoped increment that completes the second (and final) child issue of the virtual-provider Epic #154. The implementation follows the established Strategy pattern exactly, reuses the #155 selection-and-forward machinery without modification (so retry pinning, streaming pinning, error propagation, provider/model defaulting, and credential isolation all carry over unchanged), and the test additions are meaningful — statistical coverage at a failure probability of ~1e-35, a fresh zero-backends guard scenario, strategy build assertions, and an end-to-end Robot case through the real
build_chat_model→build_virtual_chat_modelchain. Documentation, standard versioning, revision history, changelog, and benchmarks are all consistent. The minor and nit items above are non-blocking polish. Nice work.4e2cb1af794b98ce0067Addressed rui.hu's review (approved, with Minor/Nit polish items) by amending the commit:
Minor Issue 1 (fixed) — Random-path tests now assert the response is forwarded unmodified, not just that a backend was called:
features/steps/virtual_provider_steps.py:exactly one configured backend should have been callednow also assertscontext.invocation_order == [<called backend>].robot/VirtualProviderRoutingTestLib.py:Exactly One Backend Should Have Received A Requestnow also asserts the response containsfinal-answer-from-backend-{N}for whichever backend was called.Nit 1 (fixed) —
benchmarks/virtual_provider_benchmark.py:_make_virtual_chat_model'sstrategyparameter is now typed as theSelectionStrategyABC instead of the concrete union.Nit 2 (not applied) — Verified via
nox -s security_scan: bandit's# nosecsuppression only works on the same physical line as the flagged call; moving it to the line above (as suggested) makes bandit re-flag B311 and fails the security gate. Left as-is.Nit 3 (fixed) — The duplicated
num_backends <= 0guard is hoisted into a sharedSelectionStrategy._validate_num_backendshelper, called by bothRoundRobinStrategyandRandomStrategy.Nit 4 (fixed) —
robot/virtual_provider_routing.robotandrobot/VirtualProviderRoutingTestLib.pydocstrings now mentionRandomStrategyalongsideRoundRobinStrategy.Nit 5 (skipped) — Per your own note, no action required (strategy-independent, already covered by round_robin scenarios).
Verified via nox (scoped to this feature):
lint,typecheck,security_scan,dead_code(full suite, all green);unit_tests -- features/virtual_provider.feature(43/43 scenarios passed);integration_tests -- robot/virtual_provider_routing.robot(2/2 passed);coverage_report -- features/virtual_provider.feature(99% onllm_virtual_provider.py, the one pre-existing uncovered line is an unrelated exception-reraise branch);benchmark(VirtualChatModelSelectBenchmark, no regressions).Commit amended in place (no force-push risk to shared history — single-commit feature branch).