feat(application): support credential injection in ReactiveCleverAgentsApp #128
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
#123 ReactiveCleverAgentsApp has no credentials param, so non-native providers (openai_compatible) can never work through it
cleveragents/cleveractors-core
Reference
cleveragents/cleveractors-core!128
Loading…
Add table
Add a link
Reference in a new issue
No description provided.
Delete branch "feature/application-credential-injection"
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
Adds an optional
credentials: dict[str, dict[str, str]] | Noneparameter toReactiveCleverAgentsApp.__init__, forwarded to the internalAgentFactory(...)built byload_configuration()(ADR-2026 per-request credential injection, already used bycreate_executor/Executor).Previously
ReactiveCleverAgentsApp— the library's documented "Quick start" entry point — had no way to supply anapi_key/base_urlfor a non-native provider (anything outsideopenai/anthropic/google, Actor Configuration Standard §4.4.1, e.g.openai_compatible), so any agent using one failed withConfigurationError: Unsupported provider: ...as soon as it was invoked.create_executor/Executoralready supported credential injection but lacked this class's agent-template resolution and config validation, forcing a choice between the two entry points' feature sets. This closes that gap.ReactiveCleverAgentsApp.__init__accepts and validatescredentials.load_configuration()forwards it to the internalAgentFactory(...).credentials(default) leaves standalone/CLI-mode behavior unchanged.Test plan
ReactiveCleverAgentsAppwith credentials for anopenai_compatibleagent and asserts successfulrun_single_shot()execution against a mocked chat model (features/credential_injection.feature,features/steps/credential_application_steps.py).nox -s lint— greennox -s format -- --check— greennox -s typecheck— green (Pyright strict, zero suppressions)nox -s security_scan— green (bandit + semgrep + vulture)nox -s dead_code— greennox -s unit_tests— green (3019 scenarios passed, 0 failed)nox -s integration_tests— green (352 Robot tests passed)nox -s coverage_report— 96.6% (threshold 96.5%)nox -s build— greenCloses #123
PR Review: !128 (Ticket #123)
Verdict: Request Changes
The implementation is correct and minimal, but the new BDD scenario does not actually verify that
credentialsare forwarded throughReactiveCleverAgentsAppto the internalAgentFactory. Because the chat-model builder is fully mocked, the test would pass even if the forwarding were omitted. This is a test-quality issue that must be addressed before approval.Critical Issues
None.
Major Issues
features/steps/credential_application_steps.py, lines 72–91)The
Whenstep patchescleveractors.agents.llm.build_chat_modelto return a mock unconditionally. Since the mock bypasses the realUnsupported provider: openai_compatiblelogic inllm_client.py, the scenario would succeed even ifReactiveCleverAgentsAppdid not passcredentialstoAgentFactory. This makes the test tautological with respect to the bug being fixed.app.agent_factory.credentialscontains the injectedopenai_compatibleentry and/or that the createdLLMAgentreceived the provider-specific credentials slice.ReactiveCleverAgentsAppwithoutcredentialsfor the same config and asserts it fails withConfigurationError: Unsupported provider: openai_compatible.Minor Issues
features/steps/credential_application_steps.py)The new steps are used only by
features/credential_injection.feature.CONTRIBUTING.mdBDD guidelines prefer extending a step file named after the feature (e.g.,credential_injection_steps.py) rather than adding another component-specific file, unless the project has an explicit convention to split by subsystem. Verify this is intentional.src/cleveractors/core/application.py, lines 69–116)ReactiveCleverAgentsApp.__init__only checks thatcredentialsis adictorNone. It does not validate the nesteddict[str, dict[str, str]]shape.AgentFactorycatches this later duringload_configuration, but the public constructor should fail fast on malformed inner values. Consider callingvalidate_credentials_structure(or an equivalent check) at construction time.Issue #123's acceptance criteria mention both
run_single_shot()andstart_interactive_session(). The new scenario only coversrun_single_shot(). Add coverage for the interactive path.README.mdanddocs/guides/reasoning-aware-llm-agents.mdstill frameReactiveCleverAgentsAppas unable to accept credentials. Update the quick-start/guide examples to show the new optional parameter.Nits
Attributeslist insrc/cleveractors/core/application.pydoes not mention the newcredentialsattribute.tempfile.mkdtemp()infeatures/steps/credential_application_steps.py(line 53) is never cleaned up; consider registering a cleanup.Summary
The code change is exactly what ticket #123 asks for: an optional
credentialsparameter onReactiveCleverAgentsApp, forwarded toAgentFactory, with unchanged standalone behavior when omitted. The changelog entry is accurate. However, the new Behave scenario is too weak to protect against regressions. Strengthen the assertions so they verify that the credentials reach the factory/agent, add the missing negative and interactive scenarios, and then this PR is ready to approve.b0b7696e2d9101063cbdThanks for the review @hurui200320 — addressed in the amended commit (force-pushed, same branch):
Major
app.agent_factory.credentials["openai_compatible"]actually matches the injected dict (not just that a mocked chat model responded). Added a negative scenario (nocredentials, real/unmockedbuild_chat_model) confirming standalone mode still rejectsopenai_compatiblewithUnsupported provider: ...— unchanged behavior, verified rather than assumed.Minor
input/print, feeds one prompt thenexit).credentialsexample to README's Quick start and a cross-reference indocs/guides/reasoning-aware-llm-agents.md.credential_injection.featurealready splits steps across 15+ subsystem-named files (credential_executor_steps.py,credential_factory_steps.py,credential_llm_error_steps.py, etc.).credential_application_steps.pyfollows that established pattern, so left as-is (matches your own caveat: "unless the project has an explicit convention to split by subsystem").Executor's existingcredentialshandling, andExecutor.__init__itself only does the shallowisinstance(..., dict)check (deep structural validation lives solely inAgentFactoryviavalidate_credentials_structure, shared to avoid duplicating that logic). Adding deep validation only on this entry point would diverge from the mirrored design and duplicate validation the factory already performs. Also added a scenario for the constructor's own non-dict-credentials rejection, since coverage showed that branch wasn't actually exercised.Nits
Attributeslist: addedcredentials.tempfile.mkdtemp()cleanup: left as-is — every other step file creating scenario temp dirs in this repo (dozens of them) follows the same no-cleanup pattern, so adding it only here would be inconsistent with existing convention rather than fixing a real gap.All of the above verified via
nox -s lint,nox -s format -- --check,nox -s typecheck,nox -s dead_code, and a targetednox -s unit_tests/coverage_reportrun scoped tofeatures/credential_injection.feature(121 scenarios, 0 failed; the new/changed lines inapplication.pyare now covered).Re-requesting review.
PR Review: !128 (Ticket #123)
Verdict: Approve
The implementation correctly adds optional
credentialsinjection toReactiveCleverAgentsApp, forwarding it to the internalAgentFactoryso that non-native providers (e.g.openai_compatible) work through the README "Quick start" entry point. All concerns from the previous review pass have been addressed: the positive Behave scenario now asserts that credentials actually reachAgentFactory.credentials, a negative scenario verifies unchanged standalone-mode rejection, an interactive-session scenario coversstart_interactive_session(), and the README/guide docs are updated. The change is minimal, type-safe, and consistent with the existingExecutor/AgentFactorycredential-injection design (ADR-2026).Critical Issues
None
Major Issues
None
Minor Issues
None
Nits
None
Summary
src/cleveractors/core/application.py: the newcredentialsparameter is validated, stored, and threaded intoAgentFactory(...)exactly where expected. No existing behavior changes whencredentialsis omitted.features/credential_injection.feature+features/steps/credential_application_steps.py: four new scenarios cover the happy path, credential forwarding, unchanged no-credentials rejection, interactive-session usage, and constructor input validation. The assertions are no longer tautological.README.mdanddocs/guides/reasoning-aware-llm-agents.mdaccurately document the new constructor option.CHANGELOG.mdentry is accurate and appropriately scoped.I reviewed the author’s response to the prior review and agree with the rationale for the items left as-is (step-file naming and temp-directory cleanup follow the project’s established subsystem-based conventions; deep credential-structure validation is intentionally delegated to
AgentFactory/validate_credentials_structure, matchingExecutor). No blocking issues remain. Approved.9101063cbdbd07b39573