fix(flows): warn (not block) on inference-provider readiness while authoring, fail runs cleanly (B45) - #5212
Merged
graycyrus merged 12 commits intoJul 27, 2026
Conversation
Adds OpenHumanBackendModel::probe_readiness, a cheap real-completion check that catches the "signed in but no provider API key configured for this account" HTTP 400 the managed backend otherwise only returns mid-run. Fails open on timeout/5xx/any non-definitive error; only rejects on the exact "API key not configured for provider" class. factory::probe_inference_readiness wraps this behind the existing construction path (create_chat_model_with_model_id_inner), so BYOK/local roles get their existing construction-time error and only the managed backend gets the extra network probe. Re-exported from inference::provider for the flows gate landing in the next commit. verify_session_active is bumped to pub(crate) so the flows gate can reuse the exact same session check every other custom-provider construction already gates on.
…45, 2-3/5) An `agent` node's completion needs a working LLM provider, but no author-time gate inspected that at all — compute_required_connections only walks tool_call Composio nodes. A signed-in user with no provider API key configured on the managed backend only found out mid-run, several layers deep as "capability error: graph error: capability error: model error: ...". validate_inference_readiness (wired into run_builder_gates between validate_agent_refs and validate_connection_refs) rejects a graph whose agent node(s) can't currently reach a working provider: - Layer 1 (sync): scheduler_gate::is_signed_out, then inference::provider::factory::verify_session_active. Skipped under #[cfg(test)], matching every other call site of that same check — unit tests use fresh tempdir workspaces with no stored app-session JWT by design and have nothing to do with session state. - Layer 2 (async): probe_inference_readiness for the resolved role, cached per (role, config.config_path) for 60s so a propose/edit/save authoring burst doesn't re-probe the network on every call. Only a successful result is cached; a rejection is always re-checked. The cache is cleared whenever is_signed_out flips true. A dynamic `=`-derived agent_ref is excluded, mirroring the existing `=`-skip pattern in validate_connection_refs — its route isn't knowable statically. build_builder_proposal now surfaces the same evaluation as inference_status/inference_message on the proposal payload (present only when the graph has an applicable agent node), computed via the shared evaluate_inference_readiness helper so the gate and the UI-facing status can never disagree.
…s (B45, 4/5) Explains the new author-time gate: an agent node needs a working LLM provider, checked automatically on every propose/edit/revise/save call. Tells the agent what to do for each status (signed_out -> tell the user to sign in; provider_not_configured -> point them at Settings > Providers; error -> relay the construction detail) and that this is separate from, and in addition to, the existing Composio connection requirements.
…B45, 5/5) - inference_gate_skips_when_no_agent_nodes: tool_call-only graph short-circuits. - inference_gate_rejects_when_signed_out: Layer 1 via SignedOutTestGuard. - inference_gate_passes_when_model_constructs / proposal_includes_inference_status_for_agent_graph: role resolves to a local (ollama:) provider, so construction succeeding is the whole check — no network, and no dependency on the process-global test_provider_override seam (which would otherwise race any other test in the binary that also installs it). - inference_gate_surfaces_construction_error: role points at a cloud slug absent from cloud_providers, failing purely on a config lookup. - proposal_omits_inference_status_for_tool_call_only_graph. None of these touch the network or the managed backend.
Contributor
📝 WalkthroughWalkthroughAdds inference-provider readiness probing for agent workflows, exposes advisory status during authoring, enforces readiness before runtime execution, documents resulting states, and adds coverage for signed-out, configured, failing, cached, and non-agent scenarios. ChangesInference readiness
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related PRs
Suggested labels: Suggested reviewers: Sequence Diagram(s)sequenceDiagram
participant WorkflowBuilder
participant ProposalBuilder
participant ProviderFactory
participant ManagedBackend
participant FlowRunner
participant TinyflowsEngine
WorkflowBuilder->>ProposalBuilder: submit workflow with agent node
ProposalBuilder->>ProviderFactory: evaluate inference readiness
ProviderFactory->>ManagedBackend: probe provider readiness
ManagedBackend-->>ProviderFactory: readiness result
ProviderFactory-->>ProposalBuilder: status and optional message
ProposalBuilder-->>WorkflowBuilder: advisory workflow proposal
FlowRunner->>ProviderFactory: validate readiness before execution
ProviderFactory-->>FlowRunner: errors or success
FlowRunner->>TinyflowsEngine: execute only when ready
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
Comment |
coderabbitai
Bot
added
agent
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f758b8447e
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
|
| Filename | Overview |
|---|---|
| src/openhuman/flows/ops.rs | Adds the inference-readiness two-layer check: advisory evaluate_inference_readiness wired into build_builder_proposal, hard gate validate_inference_readiness wired into run_flow_body preflight. Multi-role probing (BTreeMap per distinct role), negative-result caching, and cache invalidation on sign-out are all correctly implemented. Minor classification mismatch between classify_inference_error_message and is_provider_not_configured_error's secondary branch. |
| src/openhuman/inference/provider/openhuman_backend_model.rs | Adds probe_readiness (cheap max_tokens:1 ping, 5s timeout, fail-open on 5xx/timeout/transport) and is_provider_not_configured_error (narrow 400+BAD_REQUEST match). The secondary not-configured-for-provider branch is appropriately tight and tested. |
| src/openhuman/flows/ops_tests.rs | Comprehensive new tests: multi-role probing regression, dynamic-agent_ref Layer-1 coverage, run-time preflight early exit, negative-cache count verified via local axum server, and proposal advisory status shape. No network calls in any test. |
| src/openhuman/inference/provider/factory.rs | Adds probe_inference_readiness (construction + managed-backend conditional probe) and promotes verify_session_active to pub(crate) for reuse by the flows gate Layer 1. |
| src/openhuman/flows/agents/workflow_builder/prompt.md | Adds Inference provider readiness section documenting the advisory status vocabulary and LLM instructions for each status variant. |
| src/openhuman/inference/provider/mod.rs | Re-exports probe_inference_readiness from factory. Minimal, correct change. |
Sequence Diagram
sequenceDiagram
participant U as User/Copilot
participant BG as build_builder_proposal
participant EIR as evaluate_inference_readiness
participant CP as cached_probe_inference_readiness
participant PR as probe_readiness (backend)
participant RFB as run_flow_body
participant VIR as validate_inference_readiness
participant ENG as tinyflows engine
U->>BG: propose_workflow / edit_workflow
BG->>EIR: advisory
EIR->>EIR: Layer 1 is_signed_out / verify_session_active
EIR->>CP: per distinct role
CP->>CP: cache hit - return cached result
CP->>PR: cache miss - max_tokens:1 ping 5s timeout
PR-->>CP: Ok or Err
CP->>CP: cache result TTL 60s
CP-->>EIR: Ok / Err
EIR-->>BG: InferenceReadinessEvaluation
BG-->>U: proposal + inference_status
Note over U,BG: Authoring ALWAYS succeeds
U->>RFB: flows_run / flows_run_detached
RFB->>VIR: preflight enforced
VIR->>EIR: same evaluation reads cache if within TTL
EIR-->>VIR: evaluation
alt status not ready
VIR-->>RFB: error messages
RFB->>RFB: record_run failed + finish_flow_run_row
RFB-->>U: Err actionable message
Note over RFB,ENG: Engine NEVER invoked
else status ready
VIR-->>RFB: empty
RFB->>ENG: execute graph
end
Reviews (3): Last reviewed commit: "fix(inference): tighten provider-not-con..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (5)
src/openhuman/flows/ops_tests.rs (2)
3407-3423: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueThe seeded app-session is a no-op under
cfg(test).
evaluate_inference_readinessguards theverify_session_activecall with#[cfg(not(test))](ops.rs Line 2048), so nothing in this binary reads the token this helper stores. Either drop the helper and the call at Line 3506, or keep it with a comment explaining it guards against the cfg gate being removed later.Also applies to: 3504-3507
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/openhuman/flows/ops_tests.rs` around lines 3407 - 3423, Remove the unused seed_app_session_for_gate_test helper and its invocation in the gate test, since evaluate_inference_readiness excludes verify_session_active under cfg(test). Do not retain the token setup unless adding a clear comment documenting that it protects against future removal of this cfg gate.
3496-3528: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider adding direct coverage for
classify_inference_error_message.The status vocabulary (
signed_out/provider_not_configured/error) is a wire contract consumed by the prompt and the proposal payload, but only theerrorbucket is exercised here. A small table test over the classifier would pin all three mappings without touching config or the network.As per coding guidelines: "Add tests for new or changed behavior; untested code is incomplete."
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/openhuman/flows/ops_tests.rs` around lines 3496 - 3528, In ops_tests.rs, add a focused table-driven unit test for classify_inference_error_message covering representative inputs that map to signed_out, provider_not_configured, and error. Assert each returned status string exactly matches the wire-contract vocabulary, without involving configuration, graph setup, or network calls.Source: Coding guidelines
src/openhuman/flows/agents/workflow_builder/prompt.md (1)
226-252: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueSection placement fragments the numbered procedure.
Line 253 resumes the authoring checklist at item
3.; inserting an H2 immediately before it puts another block between the list's earlier items and its continuation, and most Markdown renderers restart numbering after an intervening heading. Moving this section below item 6 (after Line 283) keeps the step list contiguous.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/openhuman/flows/agents/workflow_builder/prompt.md` around lines 226 - 252, Move the “Inference provider readiness” section so it appears after item 6 of the authoring checklist, preserving the existing checklist order and content. Keep the section’s heading and guidance unchanged while ensuring the numbered procedure remains contiguous through item 6.src/openhuman/flows/ops.rs (1)
1835-1937: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚖️ Poor tradeoffConsider moving this ~300-line subsystem into its own module.
ops.rsis already far past the size guidance; the cache, role mapping, classifier, and evaluator form a self-contained unit that would sit naturally in a dedicated file (e.g.src/openhuman/flows/inference_readiness.rs) withops.rsretaining only the gate call.As per coding guidelines: "Keep files preferably at or below approximately 500 lines and favor small, single-responsibility modules."
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/openhuman/flows/ops.rs` around lines 1835 - 1937, Move the self-contained inference-readiness subsystem from ops.rs into a dedicated module such as inference_readiness.rs, including the cache, role mapping, classifier, evaluator, and related helpers like cached_probe_inference_readiness and invalidate_inference_probe_cache_if_signed_out. Expose only the interfaces needed by ops.rs, update imports and call sites, and leave ops.rs retaining only the gate integration.Source: Coding guidelines
src/openhuman/inference/provider/openhuman_backend_model.rs (1)
406-739: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winFile now well exceeds the ~500-line guideline. This PR's additions (probe logic + ~190 lines of new tests) push the file from roughly 420 to ~739 lines. The repo already has precedent for splitting tests into a sibling
_tests.rs(e.g.ops.rs/ops_tests.rsper the PR stack). Consider movingmod tests { ... }intoopenhuman_backend_model_tests.rsto keep this module focused and under the guideline.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/openhuman/inference/provider/openhuman_backend_model.rs` around lines 406 - 739, Move the entire `mod tests` module from `openhuman_backend_model.rs` into a sibling `openhuman_backend_model_tests.rs` test module, preserving all existing test helpers and test behavior. Wire the new module into the production file using the repository’s existing sibling-test-module pattern, and keep implementation logic such as `OpenHumanBackendModel`, `probe_readiness`, and `project_managed_usage` in the original module.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/openhuman/flows/ops.rs`:
- Around line 1939-1956: The role probe must cover every distinct eligible agent
role, including roles selected through specialist agent_ref routing rather than
only config.model. Update agent_node_role and its callers to resolve each node’s
actual dispatch role, iterate all eligible agent nodes, and deduplicate roles
before probing while preserving the existing summarization fallback for plain
agents.
In `@src/openhuman/inference/provider/openhuman_backend_model.rs`:
- Around line 258-276: Restrict is_provider_not_configured_error to the exact
documented API-key message and BAD_REQUEST shape; remove the broader
code_is_bad_request && message.contains("not configured") alternative. Update or
add coverage in is_provider_not_configured_error_rejects_other_400s for
unrelated BAD_REQUEST messages such as “Model X not configured,” ensuring they
are rejected.
---
Nitpick comments:
In `@src/openhuman/flows/agents/workflow_builder/prompt.md`:
- Around line 226-252: Move the “Inference provider readiness” section so it
appears after item 6 of the authoring checklist, preserving the existing
checklist order and content. Keep the section’s heading and guidance unchanged
while ensuring the numbered procedure remains contiguous through item 6.
In `@src/openhuman/flows/ops_tests.rs`:
- Around line 3407-3423: Remove the unused seed_app_session_for_gate_test helper
and its invocation in the gate test, since evaluate_inference_readiness excludes
verify_session_active under cfg(test). Do not retain the token setup unless
adding a clear comment documenting that it protects against future removal of
this cfg gate.
- Around line 3496-3528: In ops_tests.rs, add a focused table-driven unit test
for classify_inference_error_message covering representative inputs that map to
signed_out, provider_not_configured, and error. Assert each returned status
string exactly matches the wire-contract vocabulary, without involving
configuration, graph setup, or network calls.
In `@src/openhuman/flows/ops.rs`:
- Around line 1835-1937: Move the self-contained inference-readiness subsystem
from ops.rs into a dedicated module such as inference_readiness.rs, including
the cache, role mapping, classifier, evaluator, and related helpers like
cached_probe_inference_readiness and
invalidate_inference_probe_cache_if_signed_out. Expose only the interfaces
needed by ops.rs, update imports and call sites, and leave ops.rs retaining only
the gate integration.
In `@src/openhuman/inference/provider/openhuman_backend_model.rs`:
- Around line 406-739: Move the entire `mod tests` module from
`openhuman_backend_model.rs` into a sibling `openhuman_backend_model_tests.rs`
test module, preserving all existing test helpers and test behavior. Wire the
new module into the production file using the repository’s existing
sibling-test-module pattern, and keep implementation logic such as
`OpenHumanBackendModel`, `probe_readiness`, and `project_managed_usage` in the
original module.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 2febe3b6-a75c-4fcd-a0ac-a2adf0c65b22
📒 Files selected for processing (6)
src/openhuman/flows/agents/workflow_builder/prompt.mdsrc/openhuman/flows/ops.rssrc/openhuman/flows/ops_tests.rssrc/openhuman/inference/provider/factory.rssrc/openhuman/inference/provider/mod.rssrc/openhuman/inference/provider/openhuman_backend_model.rs
validate_inference_readiness used to run inside run_builder_gates, hard- rejecting propose_workflow/edit_workflow/save_workflow whenever an agent node's LLM provider wasn't reachable. A live judge run (flow 104aab90) showed the failure mode: the gate correctly caught provider_not_configured but then refused to let the copilot propose the graph at all, so the user never saw the workflow it had already built for them. Remove the call from run_builder_gates so authoring always succeeds. build_builder_proposal still evaluates readiness and attaches it to the proposal as an advisory inference_status/inference_message pair for the UI to render a nudge alongside the built workflow. The hard rejection moves to run time in a follow-up commit.
Authoring no longer blocks on provider readiness, so a flow whose agent node can't currently reach a working LLM provider can now be saved and run. Add a preflight to run_flow_body (shared by flows_run and flows_run_detached) that checks readiness before the tinyflows engine executes: on a non-ready result it finalizes the run row as failed with a clear, actionable message and returns without invoking the engine, so upstream fetch/prep nodes never do pointless work and the opaque mid-run "capability error: ... API key not configured for provider" never surfaces to the user. Reuses validate_inference_readiness (and its process-global probe cache), so a run immediately after a proposal/edit reads the cached result instead of re-probing the network. dry_run_workflow is unaffected — it never routes through run_flow_body.
INFERENCE_PROBE_CACHE previously served only a successful (Ok) probe from cache; a rejection was always re-probed live. In a single authoring turn (edit -> validate -> propose, plus a run's own preflight) that meant a signed-in-but-unconfigured account re-hit the managed backend on every call -- 4 network round trips observed in one live judge-flagged turn. Cache both outcomes for the same short TTL. This is safe because probe_inference_readiness (and OpenHumanBackendModel::probe_readiness beneath it) already fails open on anything transient -- a timeout, a transport error, a 5xx -- so an Err reaching this cache is always the definitive, config-level "not ready" signal, never a flake a naive cache would freeze in place. invalidate_inference_probe_cache_if_signed_out still clears the whole cache immediately on sign-out/back-in.
…B45) The prompt told the builder to treat inference-provider readiness as a hard prerequisite: "do not build or save an agent-node flow until it passes." That directive no longer matches the code (readiness is now advisory, never a blocker) and is exactly what produced the judge-flagged failure mode -- the builder tried to honor it, could not propose past the gate, and trailed off with no workflow shown to the user. Rewrite the section: always build and propose the graph regardless of inference_status; when it is not "ready", say so in plain language alongside the proposal (sign in / configure a provider in Settings) and stop there. Explicitly rule out refusing to propose, swapping the agent node for a code/transform node, and looping to try to resolve it.
Replace inference_gate_rejects_when_signed_out, which asserted the old (now-wrong) hard-reject behavior, with two tests pinning the corrected contract: run_builder_gates_does_not_reject_when_signed_out (authoring is never blocked) and proposal_surfaces_signed_out_inference_status (the proposal still warns via inference_status/inference_message). Add flows_run_fails_cleanly_without_invoking_engine_when_inference_not_ready, covering the new run-time preflight end to end: flows_create succeeds while signed out, the subsequent flows_run fails with an actionable message, and the persisted run row is failed with no steps -- proving the engine never ran. Add cached_probe_inference_readiness_caches_a_negative_result, using a real local axum mock that counts requests to prove a definitive provider_not_configured result is served from cache on a repeat call within the TTL rather than re-hitting the network. inference_gate_skips_when_no_agent_nodes, inference_gate_passes_when_model_constructs, inference_gate_surfaces_construction_error, proposal_includes_inference_status_for_agent_graph, proposal_omits_inference_status_for_tool_call_only_graph, and the B18 validate_required_arg_resolvability_rejects_a_null_resolved_arg reject test are unchanged and still pass.
graycyrus
changed the title
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (3)
src/openhuman/flows/ops.rs (2)
752-787: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueLog the advisory readiness status on the proposal path.
inference_readinessdrives a user-visible branch backed by a network probe, but thebuild_builder_proposalinfo log at Line 755 omits it — a signed-out/unconfigured proposal is indistinguishable from a ready one in logs.♻️ Suggested diagnostic
tracing::info!( target: "flows", %name, node_count = graph.nodes.len(), require_approval, warning_count = warnings.len(), revision, + inference_status = inference_readiness.as_ref().map(|e| e.status).unwrap_or("n/a"), "[flows] build_builder_proposal: proposal ready for user review" );As per coding guidelines: "New or changed flows must include verbose, grep-friendly diagnostics covering entry/exit, branches, external calls...".
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/openhuman/flows/ops.rs` around lines 752 - 787, Update the proposal-ready log in build_builder_proposal to include the advisory inference_readiness status, distinguishing absent readiness from each reported status. Preserve the existing payload behavior, including omission of inference fields when no evaluation is present.Source: Coding guidelines
4488-4517: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueReuse the
record_failedfinalization instead of duplicating it.This block replicates the
record_failedclosure defined at Lines 4569-4588 verbatim (record_run "failed" →current_persisted_steps→finish_flow_run_row). Hoisting that closure above the preflight keeps the two failure paths from drifting.♻️ Sketch
+ let record_failed = |error: &str| { /* moved up from below, unchanged */ }; + let inference_errors = validate_inference_readiness(config, &flow.graph).await; if !inference_errors.is_empty() { let detail = inference_errors.join(" "); let msg = format!("This flow's AI step needs a working AI provider to run. {detail}"); tracing::warn!(...); - if let Err(rec_err) = store::record_run(config, flow_id, "failed") { ... } - let observed = current_persisted_steps(config, &thread_id); - finish_flow_run_row(config, &thread_id, flow_id, "failed", &observed, &[], Some(&msg)); + record_failed(&msg); return Err(msg); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/openhuman/flows/ops.rs` around lines 4488 - 4517, Hoist the existing record_failed closure above the inference-readiness preflight in run_flow_body, then replace the duplicated record_run, current_persisted_steps, and finish_flow_run_row sequence in the inference_errors branch with a call to that closure. Preserve the existing error message and early return while ensuring both failure paths reuse the same finalization logic.src/openhuman/flows/ops_tests.rs (1)
3452-3518: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winProcess-global signed-out seam shared across concurrent
#[tokio::test]tests.SignedOutTestGuardmutates process-wide state while other tests in this file concurrently depend on it being unset, so the "no other test observes this override" assumption is the single root cause behind both risks below.
src/openhuman/flows/ops_tests.rs#L3452-L3518: confirm the guard serializes access (internal mutex) or serialize these tests explicitly, soproposal_includes_inference_status_for_agent_graph(Line 3579) can't observe"signed_out"instead of"ready".src/openhuman/flows/ops_tests.rs#L3778-L3802: verifyinvalidate_inference_probe_cache_if_signed_outdoesn't clear the wholeINFERENCE_PROBE_CACHE; if it does, a concurrent guard between the two probes breaks thehit_count == 1assertion.Based on learnings: process-global caches/seams in
src/openhuman/flows/*_tests.rscan race because#[tokio::test]tests run concurrently, making exact-count assertions flaky.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/openhuman/flows/ops_tests.rs` around lines 3452 - 3518, The process-global SignedOutTestGuard and inference probe cache can race across concurrent tests. In src/openhuman/flows/ops_tests.rs:3452-3518, make SignedOutTestGuard access mutually exclusive or explicitly serialize the signed-out tests with proposal_includes_inference_status_for_agent_graph; in src/openhuman/flows/ops_tests.rs:3778-3802, update invalidate_inference_probe_cache_if_signed_out so it invalidates only the signed-out entry rather than clearing INFERENCE_PROBE_CACHE, preserving the hit_count assertion under concurrency.Source: Learnings
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@src/openhuman/flows/ops_tests.rs`:
- Around line 3452-3518: The process-global SignedOutTestGuard and inference
probe cache can race across concurrent tests. In
src/openhuman/flows/ops_tests.rs:3452-3518, make SignedOutTestGuard access
mutually exclusive or explicitly serialize the signed-out tests with
proposal_includes_inference_status_for_agent_graph; in
src/openhuman/flows/ops_tests.rs:3778-3802, update
invalidate_inference_probe_cache_if_signed_out so it invalidates only the
signed-out entry rather than clearing INFERENCE_PROBE_CACHE, preserving the
hit_count assertion under concurrency.
In `@src/openhuman/flows/ops.rs`:
- Around line 752-787: Update the proposal-ready log in build_builder_proposal
to include the advisory inference_readiness status, distinguishing absent
readiness from each reported status. Preserve the existing payload behavior,
including omission of inference fields when no evaluation is present.
- Around line 4488-4517: Hoist the existing record_failed closure above the
inference-readiness preflight in run_flow_body, then replace the duplicated
record_run, current_persisted_steps, and finish_flow_run_row sequence in the
inference_errors branch with a call to that closure. Preserve the existing error
message and early return while ensuring both failure paths reuse the same
finalization logic.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 5f4e291f-2949-4678-9b67-a67648a6ac91
📒 Files selected for processing (3)
src/openhuman/flows/agents/workflow_builder/prompt.mdsrc/openhuman/flows/ops.rssrc/openhuman/flows/ops_tests.rs
🚧 Files skipped from review as they are similar to previous changes (1)
- src/openhuman/flows/agents/workflow_builder/prompt.md
…pe-complexity) CI's clippy gate failed on the nested HashMap<(String, PathBuf), (Instant, Result<(), String>)> type embedded directly in the INFERENCE_PROBE_CACHE static. Extract InferenceProbeCacheEntry/InferenceProbeCacheMap type aliases so the static's declared type stays readable and clippy::type-complexity is satisfied, with no behavior change.
…gs A+B, C) evaluate_inference_readiness previously collected every applicable `agent` node but derived the Layer-2 probe role from ONLY the graph's first node — a second (or later) node pinned to a different `config.model` (routing to a different, possibly broken, provider) was never checked at all, so a graph mixing e.g. a `chat` step and a `reasoning` step could falsely pass. Fix: - agent_node_role(config, node) now also resolves a static (non-`=`) `agent_ref` whose custom AgentRegistryEntry itself pins a model (e.g. `hint:reasoning`), via the same sync accessor (agent_registry::find_custom_in_config) and precedence OpenHumanAgentRunner::run_via_harness applies. A static agent_ref that instead resolves to a shipped/TOML harness AgentDefinition falls back to the default role with a TODO(B45) note — ModelSpec::Inherit needs a live parent model this pre-run gate has no access to, so a partial Exact/Hint-only resolver would be silently wrong for Inherit-using definitions. - evaluate_inference_readiness groups agent nodes by their distinct effective role and runs the (still-cached) Layer-2 probe once per distinct role, reporting provider_not_configured/error if ANY role fails and naming every offending node/role in the message. Finding C: a graph whose only agent node(s) have a dynamic (`=`-derived) agent_ref used to be filtered out of the check entirely, so an all-dynamic- ref graph returned no readiness signal at all — a signed-out session went unreported. Such nodes now stay in scope for Layer 1 (signed-out/session) and get a default-role Layer 2 probe; only their exact per-model role resolution is skipped, since their concrete route isn't known until run time. Adds agent_node_role_prefers_custom_registry_entry_model_pin_over_default, inference_gate_probes_every_distinct_agent_node_role, and inference_gate_reports_signed_out_for_dynamic_agent_ref_only_graph to ops_tests.rs.
…umented signal (B45 finding D)
is_provider_not_configured_error's doc claimed it matches only the exact
backend shape ("API key not configured for provider"), but the code also
matched any 400 BAD_REQUEST whose message merely contained the generic
substring "not configured" — an unrelated validation error mentioning "not
configured" (a webhook target, a feature flag, ...) would be misclassified
as a provider-key problem. Tighten the BAD_REQUEST-coded tolerance branch to
require "not configured for provider" specifically, and update the doc to
state exactly what it matches. Keeps fail-open behavior for everything else.
Adds is_provider_not_configured_error_rejects_generic_not_configured_400
(pins the tightened contract) and
is_provider_not_configured_error_tolerates_not_configured_for_provider_wording_drift
(pins the still-supported narrower tolerance).
coderabbitai
Bot
removed
the
agent
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/openhuman/flows/ops_tests.rs (1)
3578-3587: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚖️ Poor tradeoffConsider splitting the readiness tests into a dedicated module file.
ops_tests.rsis well past the ~500-line target; the inference-readiness suite is a cohesive block that could live in its own*_tests.rsmodule. As per coding guidelines: "Keep files preferably at or below approximately 500 lines and favor small, single-responsibility modules."🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/openhuman/flows/ops_tests.rs` around lines 3578 - 3587, Move the cohesive inference-readiness test suite, including the multi-role agent-node tests and related helpers, from ops_tests.rs into a dedicated *_tests.rs module. Register the new module using the existing test-module conventions and preserve all test behavior and visibility requirements.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@src/openhuman/flows/ops_tests.rs`:
- Around line 3578-3587: Move the cohesive inference-readiness test suite,
including the multi-role agent-node tests and related helpers, from ops_tests.rs
into a dedicated *_tests.rs module. Register the new module using the existing
test-module conventions and preserve all test behavior and visibility
requirements.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 531c176e-bbbf-49a7-9c10-5861a40c7e02
📒 Files selected for processing (3)
src/openhuman/flows/ops.rssrc/openhuman/flows/ops_tests.rssrc/openhuman/inference/provider/openhuman_backend_model.rs
🚧 Files skipped from review as they are similar to previous changes (2)
- src/openhuman/inference/provider/openhuman_backend_model.rs
- src/openhuman/flows/ops.rs
senamakel
approved these changes
Jul 27, 2026
senamakel
left a comment
senamakel
left a comment
Member
There was a problem hiding this comment.
Final review at head 2e306eb: the managed-backend probe is deliberately narrow and fail-open for transient/non-definitive errors; flow authoring remains advisory while execution preflights and finalizes failed runs before engine work; distinct effective agent roles, dynamic refs, caching, and provider-error classification have regression coverage. PR CI Gate, Rust core coverage, fmt/clippy, feature-gate smoke, CodeRabbit, and Greptile are green, with no unresolved threads. I found no blocking correctness or safety issue.
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
agentnode could be built/saved cleanly, then fail at run time with an opaqueHTTP 400: {"error":"API key not configured for provider"}.inference_status), so the copilot still shows the built workflow and tells the user to configure their provider.Problem
No author-time signal existed for an
agentnode whose managed-backend LLM provider has no API key configured on the account. The flow passed every gate, saved, and only failed on an actual completion at run time with an opaque 400 — late and confusing. The user is authenticated (JWT resolves); this is an account-level provider-config gap, distinct from "not logged in" and from a Composio app connection.Solution
build_builder_proposalattachesinference_status(ready/signed_out/provider_not_configured/error) +inference_message. Proposing/editing/saving is never blocked by it —validate_inference_readinesswas removed fromrun_builder_gates. Theworkflow_builderprompt now proposes the graph regardless and explains, in plain language, that it needs the provider configured (sign in / Settings > Providers) before it will run.run_flow_body(shared byflows_runandflows_run_detached) runs the readiness check for graphs with agent nodes before the tinyflows engine executes. On failure it finalizes the run row asfailedwith an actionable message and returns without invoking the engine, so the error shows in the run-details sidebar and upstream nodes don't run pointlessly.dry_run_workflowis unaffected (stays sandboxed).max_tokens:1backend probe, 5s timeout, fail-open on transient/5xx/timeout so it only ever fires on the definitive client-config 400). Cached per(role, config_path)for 60s; negative results are now cached too, eliminating the repeated per-turn re-probing.Submission Checklist
run_builder_gates_does_not_reject_when_signed_out,proposal_surfaces_signed_out_inference_status,flows_run_fails_cleanly_without_invoking_engine_when_inference_not_ready,cached_probe_inference_readiness_caches_a_negative_result, plus retainedinference_gate_skips_when_no_agent_nodes/_passes_when_model_constructs/_surfaces_construction_error, proposal-status tests, and probe tests. No test hits the network (localollama:provider / axum mock).openhuman::flows+ 260openhuman::inference::providertests green locally; CIcargo-llvm-covgate authoritative.Impact
Related
AI Authored PR Metadata
Linear Issue
Commit & Branch
fix/flows-inference-readiness-gate8356f1ff4Validation Run
pnpm --filter openhuman-app format:check— N/A, noapp/srcchangespnpm typecheck— N/A, no TypeScript changescargo test --lib -- openhuman::flows::(468 passed) +openhuman::inference::provider::(260 passed)GGML_NATIVE=OFF cargo checkclean;rustup run 1.96.1 cargo fmt --all --checkclean (pinned toolchain)Validation Blocked
command:N/Aerror:N/Aimpact:N/ABehavior Changes
Parity Contract
agent_refskipped.Duplicate / Superseded PR Handling
Summary by CodeRabbit