feat(application): support credential injection in ReactiveCleverAgentsApp #128

Merged
CoreRasurae merged 1 commit from feature/application-credential-injection into master 2026-08-10 10:48:32 +00:00
Member

Summary

Adds an optional credentials: dict[str, dict[str, str]] | None parameter to ReactiveCleverAgentsApp.__init__, forwarded to the internal AgentFactory(...) built by load_configuration() (ADR-2026 per-request credential injection, already used by create_executor/Executor).

Previously ReactiveCleverAgentsApp — the library's documented "Quick start" entry point — had no way to supply an api_key/base_url for a non-native provider (anything outside openai/anthropic/google, Actor Configuration Standard §4.4.1, e.g. openai_compatible), so any agent using one failed with ConfigurationError: Unsupported provider: ... as soon as it was invoked. create_executor/Executor already 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 validates credentials.
  • load_configuration() forwards it to the internal AgentFactory(...).
  • Omitting credentials (default) leaves standalone/CLI-mode behavior unchanged.

Test plan

  • New Behave scenario: constructs ReactiveCleverAgentsApp with credentials for an openai_compatible agent and asserts successful run_single_shot() execution against a mocked chat model (features/credential_injection.feature, features/steps/credential_application_steps.py).
  • nox -s lint — green
  • nox -s format -- --check — green
  • nox -s typecheck — green (Pyright strict, zero suppressions)
  • nox -s security_scan — green (bandit + semgrep + vulture)
  • nox -s dead_code — green
  • nox -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 — green

Closes #123

## Summary Adds an optional `credentials: dict[str, dict[str, str]] | None` parameter to `ReactiveCleverAgentsApp.__init__`, forwarded to the internal `AgentFactory(...)` built by `load_configuration()` (ADR-2026 per-request credential injection, already used by `create_executor`/`Executor`). Previously `ReactiveCleverAgentsApp` — the library's documented "Quick start" entry point — had no way to supply an `api_key`/`base_url` for a non-native provider (anything outside `openai`/`anthropic`/`google`, Actor Configuration Standard §4.4.1, e.g. `openai_compatible`), so any agent using one failed with `ConfigurationError: Unsupported provider: ...` as soon as it was invoked. `create_executor`/`Executor` already 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 validates `credentials`. - `load_configuration()` forwards it to the internal `AgentFactory(...)`. - Omitting `credentials` (default) leaves standalone/CLI-mode behavior unchanged. ## Test plan - [x] New Behave scenario: constructs `ReactiveCleverAgentsApp` with credentials for an `openai_compatible` agent and asserts successful `run_single_shot()` execution against a mocked chat model (`features/credential_injection.feature`, `features/steps/credential_application_steps.py`). - [x] `nox -s lint` — green - [x] `nox -s format -- --check` — green - [x] `nox -s typecheck` — green (Pyright strict, zero suppressions) - [x] `nox -s security_scan` — green (bandit + semgrep + vulture) - [x] `nox -s dead_code` — green - [x] `nox -s unit_tests` — green (3019 scenarios passed, 0 failed) - [x] `nox -s integration_tests` — green (352 Robot tests passed) - [x] `nox -s coverage_report` — 96.6% (threshold 96.5%) - [x] `nox -s build` — green Closes #123
CoreRasurae added this to the v2.1.0 milestone 2026-08-09 23:21:08 +00:00
hurui200320 requested changes 2026-08-10 06:54:06 +00:00
Dismissed
hurui200320 left a comment

PR Review: !128 (Ticket #123)

Verdict: Request Changes

The implementation is correct and minimal, but the new BDD scenario does not actually verify that credentials are forwarded through ReactiveCleverAgentsApp to the internal AgentFactory. 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

  • Test does not validate credential forwarding (features/steps/credential_application_steps.py, lines 72–91)
    The When step patches cleveractors.agents.llm.build_chat_model to return a mock unconditionally. Since the mock bypasses the real Unsupported provider: openai_compatible logic in llm_client.py, the scenario would succeed even if ReactiveCleverAgentsApp did not pass credentials to AgentFactory. This makes the test tautological with respect to the bug being fixed.
    • Add assertions that app.agent_factory.credentials contains the injected openai_compatible entry and/or that the created LLMAgent received the provider-specific credentials slice.
    • Add a negative scenario that constructs ReactiveCleverAgentsApp without credentials for the same config and asserts it fails with ConfigurationError: Unsupported provider: openai_compatible.

Minor Issues

  • Feature-specific step file naming (features/steps/credential_application_steps.py)
    The new steps are used only by features/credential_injection.feature. CONTRIBUTING.md BDD 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.
  • Incomplete constructor validation (src/cleveractors/core/application.py, lines 69–116)
    ReactiveCleverAgentsApp.__init__ only checks that credentials is a dict or None. It does not validate the nested dict[str, dict[str, str]] shape. AgentFactory catches this later during load_configuration, but the public constructor should fail fast on malformed inner values. Consider calling validate_credentials_structure (or an equivalent check) at construction time.
  • Missing interactive-session coverage
    Issue #123's acceptance criteria mention both run_single_shot() and start_interactive_session(). The new scenario only covers run_single_shot(). Add coverage for the interactive path.
  • Documentation not updated
    README.md and docs/guides/reasoning-aware-llm-agents.md still frame ReactiveCleverAgentsApp as unable to accept credentials. Update the quick-start/guide examples to show the new optional parameter.

Nits

  • Class docstring Attributes list in src/cleveractors/core/application.py does not mention the new credentials attribute.
  • The temporary directory created with tempfile.mkdtemp() in features/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 credentials parameter on ReactiveCleverAgentsApp, forwarded to AgentFactory, 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.

## PR Review: !128 (Ticket #123) ### Verdict: Request Changes The implementation is correct and minimal, but the new BDD scenario does not actually verify that `credentials` are forwarded through `ReactiveCleverAgentsApp` to the internal `AgentFactory`. 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 - **Test does not validate credential forwarding** (`features/steps/credential_application_steps.py`, lines 72–91) The `When` step patches `cleveractors.agents.llm.build_chat_model` to return a mock unconditionally. Since the mock bypasses the real `Unsupported provider: openai_compatible` logic in `llm_client.py`, the scenario would succeed even if `ReactiveCleverAgentsApp` did **not** pass `credentials` to `AgentFactory`. This makes the test tautological with respect to the bug being fixed. - Add assertions that `app.agent_factory.credentials` contains the injected `openai_compatible` entry and/or that the created `LLMAgent` received the provider-specific credentials slice. - Add a negative scenario that constructs `ReactiveCleverAgentsApp` **without** `credentials` for the same config and asserts it fails with `ConfigurationError: Unsupported provider: openai_compatible`. ### Minor Issues - **Feature-specific step file naming** (`features/steps/credential_application_steps.py`) The new steps are used only by `features/credential_injection.feature`. `CONTRIBUTING.md` BDD 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. - **Incomplete constructor validation** (`src/cleveractors/core/application.py`, lines 69–116) `ReactiveCleverAgentsApp.__init__` only checks that `credentials` is a `dict` or `None`. It does not validate the nested `dict[str, dict[str, str]]` shape. `AgentFactory` catches this later during `load_configuration`, but the public constructor should fail fast on malformed inner values. Consider calling `validate_credentials_structure` (or an equivalent check) at construction time. - **Missing interactive-session coverage** Issue #123's acceptance criteria mention both `run_single_shot()` and `start_interactive_session()`. The new scenario only covers `run_single_shot()`. Add coverage for the interactive path. - **Documentation not updated** `README.md` and `docs/guides/reasoning-aware-llm-agents.md` still frame `ReactiveCleverAgentsApp` as unable to accept credentials. Update the quick-start/guide examples to show the new optional parameter. ### Nits - Class docstring `Attributes` list in `src/cleveractors/core/application.py` does not mention the new `credentials` attribute. - The temporary directory created with `tempfile.mkdtemp()` in `features/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 `credentials` parameter on `ReactiveCleverAgentsApp`, forwarded to `AgentFactory`, 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.
CoreRasurae force-pushed feature/application-credential-injection from b0b7696e2d
Some checks failed
CI / lint (pull_request) Successful in 44s
CI / typecheck (pull_request) Successful in 56s
CI / security (pull_request) Successful in 55s
CI / quality (pull_request) Successful in 1m38s
CI / build (pull_request) Successful in 46s
CI / integration_tests (pull_request) Successful in 3m47s
CI / unit_tests (pull_request) Successful in 6m18s
CI / coverage (pull_request) Successful in 4m10s
CI / status-check (pull_request) Successful in 11s
CI / benchmark (pull_request) Failing after 16m14s
to 9101063cbd
Some checks failed
CI / lint (pull_request) Successful in 1m15s
CI / typecheck (pull_request) Successful in 1m16s
CI / quality (pull_request) Successful in 57s
CI / security (pull_request) Successful in 2m6s
CI / build (pull_request) Successful in 2m27s
CI / integration_tests (pull_request) Successful in 3m14s
CI / unit_tests (pull_request) Successful in 4m8s
CI / coverage (pull_request) Failing after 19m0s
CI / benchmark (pull_request) Failing after 22m55s
CI / status-check (pull_request) Failing after 4s
2026-08-10 08:27:09 +00:00
Compare
Author
Member

Thanks for the review @hurui200320 — addressed in the amended commit (force-pushed, same branch):

Major

  • Test tautology: fixed. The positive scenario now asserts app.agent_factory.credentials["openai_compatible"] actually matches the injected dict (not just that a mocked chat model responded). Added a negative scenario (no credentials, real/unmocked build_chat_model) confirming standalone mode still rejects openai_compatible with Unsupported provider: ... — unchanged behavior, verified rather than assumed.

Minor

  • Interactive-session coverage: added, mirroring the single-shot scenario (mocks input/print, feeds one prompt then exit).
  • Docs: added a credentials example to README's Quick start and a cross-reference in docs/guides/reasoning-aware-llm-agents.md.
  • Step-file naming: checked against the repo's own convention — credential_injection.feature already splits steps across 15+ subsystem-named files (credential_executor_steps.py, credential_factory_steps.py, credential_llm_error_steps.py, etc.). credential_application_steps.py follows that established pattern, so left as-is (matches your own caveat: "unless the project has an explicit convention to split by subsystem").
  • Deep constructor validation: intentionally not added. Issue #123 explicitly asks this parameter to mirror Executor's existing credentials handling, and Executor.__init__ itself only does the shallow isinstance(..., dict) check (deep structural validation lives solely in AgentFactory via validate_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

  • Docstring Attributes list: added credentials.
  • 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 targeted nox -s unit_tests/coverage_report run scoped to features/credential_injection.feature (121 scenarios, 0 failed; the new/changed lines in application.py are now covered).

Re-requesting review.

Thanks for the review @hurui200320 — addressed in the amended commit (force-pushed, same branch): **Major** - Test tautology: fixed. The positive scenario now asserts `app.agent_factory.credentials["openai_compatible"]` actually matches the injected dict (not just that a mocked chat model responded). Added a negative scenario (no `credentials`, real/unmocked `build_chat_model`) confirming standalone mode still rejects `openai_compatible` with `Unsupported provider: ...` — unchanged behavior, verified rather than assumed. **Minor** - Interactive-session coverage: added, mirroring the single-shot scenario (mocks `input`/`print`, feeds one prompt then `exit`). - Docs: added a `credentials` example to README's Quick start and a cross-reference in `docs/guides/reasoning-aware-llm-agents.md`. - Step-file naming: checked against the repo's own convention — `credential_injection.feature` already splits steps across 15+ subsystem-named files (`credential_executor_steps.py`, `credential_factory_steps.py`, `credential_llm_error_steps.py`, etc.). `credential_application_steps.py` follows that established pattern, so left as-is (matches your own caveat: "unless the project has an explicit convention to split by subsystem"). - Deep constructor validation: intentionally not added. Issue #123 explicitly asks this parameter to *mirror* `Executor`'s existing `credentials` handling, and `Executor.__init__` itself only does the shallow `isinstance(..., dict)` check (deep structural validation lives solely in `AgentFactory` via `validate_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** - Docstring `Attributes` list: added `credentials`. - `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 targeted `nox -s unit_tests`/`coverage_report` run scoped to `features/credential_injection.feature` (121 scenarios, 0 failed; the new/changed lines in `application.py` are now covered). Re-requesting review.
hurui200320 left a comment

PR Review: !128 (Ticket #123)

Verdict: Approve

The implementation correctly adds optional credentials injection to ReactiveCleverAgentsApp, forwarding it to the internal AgentFactory so 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 reach AgentFactory.credentials, a negative scenario verifies unchanged standalone-mode rejection, an interactive-session scenario covers start_interactive_session(), and the README/guide docs are updated. The change is minimal, type-safe, and consistent with the existing Executor/AgentFactory credential-injection design (ADR-2026).

Critical Issues

None

Major Issues

None

Minor Issues

None

Nits

None

Summary

  • src/cleveractors/core/application.py: the new credentials parameter is validated, stored, and threaded into AgentFactory(...) exactly where expected. No existing behavior changes when credentials is 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.md and docs/guides/reasoning-aware-llm-agents.md accurately document the new constructor option.
  • CHANGELOG.md entry 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, matching Executor). No blocking issues remain. Approved.

## PR Review: !128 (Ticket #123) ### Verdict: Approve The implementation correctly adds optional `credentials` injection to `ReactiveCleverAgentsApp`, forwarding it to the internal `AgentFactory` so 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 reach `AgentFactory.credentials`, a negative scenario verifies unchanged standalone-mode rejection, an interactive-session scenario covers `start_interactive_session()`, and the README/guide docs are updated. The change is minimal, type-safe, and consistent with the existing `Executor`/`AgentFactory` credential-injection design (ADR-2026). ### Critical Issues None ### Major Issues None ### Minor Issues None ### Nits None ### Summary - `src/cleveractors/core/application.py`: the new `credentials` parameter is validated, stored, and threaded into `AgentFactory(...)` exactly where expected. No existing behavior changes when `credentials` is 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.md` and `docs/guides/reasoning-aware-llm-agents.md` accurately document the new constructor option. - `CHANGELOG.md` entry 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`, matching `Executor`). No blocking issues remain. Approved.
feat(application): support credential injection in ReactiveCleverAgentsApp
Some checks failed
CI / lint (pull_request) Successful in 1m34s
CI / typecheck (pull_request) Successful in 1m31s
CI / build (pull_request) Successful in 1m7s
CI / quality (pull_request) Successful in 2m9s
CI / security (pull_request) Successful in 2m58s
CI / integration_tests (pull_request) Successful in 5m15s
CI / unit_tests (pull_request) Successful in 5m22s
CI / coverage (pull_request) Successful in 4m29s
CI / status-check (pull_request) Successful in 6s
CI / benchmark (pull_request) Has been cancelled
CI / lint (push) Successful in 1m1s
CI / typecheck (push) Successful in 1m36s
CI / quality (push) Successful in 1m34s
CI / security (push) Successful in 2m14s
CI / build (push) Successful in 1m6s
CI / integration_tests (push) Successful in 3m4s
CI / unit_tests (push) Successful in 5m14s
CI / benchmark (push) Has been cancelled
CI / coverage (push) Successful in 5m13s
CI / status-check (push) Successful in 10s
bd07b39573
Add an optional `credentials: dict[str, dict[str, str]] | None` parameter
to `ReactiveCleverAgentsApp.__init__`, validated (must be a dict or None)
and forwarded verbatim to the internal `AgentFactory(...)` constructed by
`load_configuration()`.

Previously `ReactiveCleverAgentsApp` had no way to supply an `api_key`/
`base_url` for a non-native provider (anything outside `openai`/
`anthropic`/`google`, Actor Configuration Standard §4.4.1, e.g.
`openai_compatible`), so any agent using one failed with
`ConfigurationError: Unsupported provider: ...` as soon as it was invoked
via `run_single_shot()`/`start_interactive_session()`. `create_executor`/
`Executor` already supported this (ADR-2026 per-request credential
injection) but lacked this class's agent-template resolution and config
validation, forcing a choice between the two entry points' feature sets.
Threading `credentials` through closes that gap: a single code path now
supports template resolution, validation, and non-native-provider
credential injection together. Omitting `credentials` (the default)
leaves standalone/CLI-mode behavior unchanged.

BDD coverage (`features/credential_injection.feature`,
`features/steps/credential_application_steps.py`) now includes:
- a positive scenario asserting the injected `openai_compatible`
  credentials actually reach `AgentFactory.credentials`, not just that a
  mocked chat model was invoked;
- a negative scenario (no credentials, unmocked provider resolution)
  confirming standalone-mode behavior is unchanged: the agent still
  reports `Unsupported provider: openai_compatible`;
- an interactive-session scenario, mirroring the single-shot one, so both
  entry points named in the issue's acceptance criteria are exercised;
- a constructor-validation scenario for a non-dict `credentials` value.

Also updates the class docstring's Attributes list, and cross-references
the new constructor option from README.md's Quick start and
docs/guides/reasoning-aware-llm-agents.md.

ISSUES CLOSED: #123
CoreRasurae force-pushed feature/application-credential-injection from 9101063cbd
Some checks failed
CI / lint (pull_request) Successful in 1m15s
CI / typecheck (pull_request) Successful in 1m16s
CI / quality (pull_request) Successful in 57s
CI / security (pull_request) Successful in 2m6s
CI / build (pull_request) Successful in 2m27s
CI / integration_tests (pull_request) Successful in 3m14s
CI / unit_tests (pull_request) Successful in 4m8s
CI / coverage (pull_request) Failing after 19m0s
CI / benchmark (pull_request) Failing after 22m55s
CI / status-check (pull_request) Failing after 4s
to bd07b39573
Some checks failed
CI / lint (pull_request) Successful in 1m34s
CI / typecheck (pull_request) Successful in 1m31s
CI / build (pull_request) Successful in 1m7s
CI / quality (pull_request) Successful in 2m9s
CI / security (pull_request) Successful in 2m58s
CI / integration_tests (pull_request) Successful in 5m15s
CI / unit_tests (pull_request) Successful in 5m22s
CI / coverage (pull_request) Successful in 4m29s
CI / status-check (pull_request) Successful in 6s
CI / benchmark (pull_request) Has been cancelled
CI / lint (push) Successful in 1m1s
CI / typecheck (push) Successful in 1m36s
CI / quality (push) Successful in 1m34s
CI / security (push) Successful in 2m14s
CI / build (push) Successful in 1m6s
CI / integration_tests (push) Successful in 3m4s
CI / unit_tests (push) Successful in 5m14s
CI / benchmark (push) Has been cancelled
CI / coverage (push) Successful in 5m13s
CI / status-check (push) Successful in 10s
2026-08-10 10:33:18 +00:00
Compare
CoreRasurae deleted branch feature/application-credential-injection 2026-08-10 10:48:42 +00:00
Sign in to join this conversation.
No reviewers
No milestone
No project
No assignees
2 participants
Notifications
Due date
The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Reference
cleveragents/cleveractors-core!128
No description provided.