feat(flows): completed_with_warnings run status (run honesty)#4587
Conversation
…reported as success A run whose =-bindings resolved to null (or whose nodes errored) reported green "Completed". The observer already records per-step diagnostics; the status derivation just ignored them. Now flows_run/flows_resume derive status from the settled steps: any step error → "failed", any null-resolution diagnostic → "completed_with_warnings" (amber), else "completed". Precedence: pending_approval > failed > completed_with_warnings > completed. Frontend: FlowRunStatus union + status maps (amber badge, non-pulsing dot) + TERMINAL poller set + i18n (14 locales); also fills the pre-existing 'cancelled' gap in the TS union/maps. cargo flows:: 279 pass (4 new degrade_completed_status tests); typecheck/eslint clean; vitest 167 pass; i18n:check 0 drift.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughThis PR adds ChangesFlow run status extension
Estimated code review effort: 3 (Moderate) | ~25 minutes Possibly related PRs
Suggested labels: Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
Comment |
![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: 1c9a1d8c05
ℹ️ 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".
| return "failed"; | ||
| } | ||
| if steps.iter().any(|s| !s.diagnostics.is_empty()) { | ||
| "completed_with_warnings" |
There was a problem hiding this comment.
When a run gets this new completed_with_warnings status, flows_cancel_run still only treats completed | failed | cancelled as terminal (src/openhuman/flows/ops.rs:1424). That means a settled warning run can later be passed to openhuman.flows_cancel_run, which will fall through and overwrite the run row and flow summary as cancelled, corrupting the history/status for an already-finished run. Please include this new status in the terminal guard before emitting it here.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Confirmed and fixed in 22aaf5c — flows_cancel_run's terminal-status guard now includes completed_with_warnings alongside completed/failed/cancelled, matching FlowRun::status's doc contract that already listed it as terminal. Added a regression test (flows_cancel_run_of_a_completed_with_warnings_run_errors) that forces a settled warning row and asserts cancel is rejected with 'already terminal' and the row's status is untouched. Good catch.
coderabbitai
Bot
added
feature
…_run (addresses @chatgpt-codex-connector on src/openhuman/flows/ops.rs:1629) flows_cancel_run's terminal-status guard only recognized completed/failed/cancelled, so a settled completed_with_warnings run (run honesty, PR2) would fall through the "not in flight" cancel path and get overwritten as cancelled, silently discarding the warning status it already recorded. Add completed_with_warnings to the guard, matching the type-level contract in FlowRun::status's doc comment that already listed it as terminal. Adds a regression test.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/openhuman/flows/ops.rs (1)
1070-1084: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider extracting the duplicated terminal-status computation.
The 6-line block computing
settled/statusand callingfinish_flow_run_rowis now byte-for-byte duplicated betweenflows_runandflows_resume. A shared helper (e.g.fn finalize_terminal_status(settled: &[FlowRunStep], pending_approvals: &[String]) -> &'static str) would keep the precedence rule in one place, reducing the risk of the two call sites drifting (as already happened with theerrorfield above).Also applies to: 1273-1287
🤖 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 1070 - 1084, The terminal-status computation is duplicated in both flows_run and flows_resume, so extract the shared logic into a helper near the existing settle_steps/degrade_completed_status flow. Add a small helper keyed off settle_steps, degrade_completed_status, and the pending_approvals precedence so both call sites use the same status selection and finish_flow_run_row arguments, keeping the rule in one place and preventing drift.
🤖 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 1070-1084: Populate FlowRun.error when
degrade_completed_status(&settled) returns a failed status in
ops::finish_flow_run_row path. In the code that builds status from
settle_steps/config and then calls store::record_run and finish_flow_run_row,
capture the failure details from settled or the step error and pass them through
so failed runs are recorded with a non-empty error instead of None. Update the
flows_run/flows_resume flow to preserve the error detail whenever status is
"failed", using the existing settle_steps, degrade_completed_status, and
finish_flow_run_row symbols to locate the change.
---
Nitpick comments:
In `@src/openhuman/flows/ops.rs`:
- Around line 1070-1084: The terminal-status computation is duplicated in both
flows_run and flows_resume, so extract the shared logic into a helper near the
existing settle_steps/degrade_completed_status flow. Add a small helper keyed
off settle_steps, degrade_completed_status, and the pending_approvals precedence
so both call sites use the same status selection and finish_flow_run_row
arguments, keeping the rule in one place and preventing drift.
🪄 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: fa2d8f24-d0a5-4304-98d3-4439542b6b18
📒 Files selected for processing (22)
app/src/components/flows/FlowRunInspectorDrawer.tsxapp/src/components/flows/FlowRunsDrawer.test.tsxapp/src/hooks/__tests__/useFlowRunPoller.test.tsapp/src/hooks/useFlowRunPoller.tsapp/src/lib/i18n/ar.tsapp/src/lib/i18n/bn.tsapp/src/lib/i18n/de.tsapp/src/lib/i18n/en.tsapp/src/lib/i18n/es.tsapp/src/lib/i18n/fr.tsapp/src/lib/i18n/hi.tsapp/src/lib/i18n/id.tsapp/src/lib/i18n/it.tsapp/src/lib/i18n/ko.tsapp/src/lib/i18n/pl.tsapp/src/lib/i18n/pt.tsapp/src/lib/i18n/ru.tsapp/src/lib/i18n/zh-CN.tsapp/src/services/api/flowsApi.tssrc/openhuman/flows/ops.rssrc/openhuman/flows/ops_tests.rssrc/openhuman/flows/types.rs
| let settled = settle_steps(config, &thread_id, &outcome.output); | ||
| let status = if !outcome.pending_approvals.is_empty() { | ||
| "pending_approval" | ||
| } else { | ||
| degrade_completed_status(&settled) | ||
| }; | ||
| store::record_run(config, flow_id, status).map_err(|e| e.to_string())?; | ||
| finish_flow_run_row( | ||
| config, | ||
| &thread_id, | ||
| status, | ||
| &settle_steps(config, &thread_id, &outcome.output), | ||
| &settled, | ||
| &outcome.pending_approvals, | ||
| None, | ||
| ); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Locate the relevant symbols and surrounding code.
rg -n "degrade_completed_status|finish_flow_run_row|FlowRun::error|error: None|pending_approval|status == \"failed\"" src/openhuman -S
# Show concise outlines of the main file and types file.
ast-grep outline src/openhuman/flows/ops.rs --view expanded || true
ast-grep outline src/openhuman/flows/types.rs --view expanded || true
# Show the relevant line ranges with context.
for f in src/openhuman/flows/ops.rs src/openhuman/flows/types.rs; do
echo "==== $f ===="
wc -l "$f"
done
# Print the relevant snippets around likely line numbers if present.
sed -n '1000,1125p' src/openhuman/flows/ops.rs
echo "==== types excerpt ===="
sed -n '1,260p' src/openhuman/flows/types.rs
# Search for tests around degraded failed path and finish_flow_run_row.
rg -n "degraded|failed.*error|finish_flow_run_row|pending_approval|FlowRunStep|step error|step.*failed" src/openhuman -g '*test*' -g '*.rs' -SRepository: tinyhumansai/openhuman
Length of output: 50379
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== finish_flow_run_row and surrounding call sites =="
sed -n '1510,1565p' src/openhuman/flows/ops.rs
echo
echo "== degrade_completed_status =="
sed -n '1624,1648p' src/openhuman/flows/ops.rs
echo
echo "== relevant tests around degrade_completed_status =="
sed -n '1659,1715p' src/openhuman/flows/ops_tests.rs
echo
echo "== run persistence schema / row mapping =="
sed -n '430,560p' src/openhuman/flows/store.rs
echo
echo "== flow run type contract =="
sed -n '190,232p' src/openhuman/flows/types.rsRepository: tinyhumansai/openhuman
Length of output: 13429
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== ops_tests degraded status block =="
sed -n '1668,1710p' src/openhuman/flows/ops_tests.rs
echo "== finish_flow_run_row definition =="
sed -n '1517,1545p' src/openhuman/flows/ops.rs
echo "== any tests asserting FlowRun.error on failed statuses =="
rg -n 'error.*failed|failed.*error|FlowRun .*error|status: "failed"|status == "failed"' src/openhuman/flows/ops_tests.rs src/openhuman/flows/store_tests.rs -SRepository: tinyhumansai/openhuman
Length of output: 2979
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== flows_resume settle path =="
sed -n '1238,1288p' src/openhuman/flows/ops.rs
echo
echo "== failed run test =="
sed -n '270,330p' src/openhuman/flows/ops_tests.rsRepository: tinyhumansai/openhuman
Length of output: 4273
Populate FlowRun.error for degraded failures. Both flows_run and flows_resume persist status: "failed" with error: None when degrade_completed_status(&settled) trips on a step error. That leaves failed rows inconsistent with FlowRun::error’s contract and strips the only failure detail from run history.
🤖 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 1070 - 1084, Populate FlowRun.error
when degrade_completed_status(&settled) returns a failed status in
ops::finish_flow_run_row path. In the code that builds status from
settle_steps/config and then calls store::record_run and finish_flow_run_row,
capture the failure details from settled or the step error and pass them through
so failed runs are recorded with a non-empty error instead of None. Update the
flows_run/flows_resume flow to preserve the error detail whenever status is
"failed", using the existing settle_steps, degrade_completed_status, and
finish_flow_run_row symbols to locate the change.
There was a problem hiding this comment.
Confirmed and fixed in e1ed6e4. Added failed_step_error_summary(), which names the node(s) whose step settled with status == "error" (the only detail available — the engine's ExecutionStep doesn't carry a message for a step failing under on_error: "continue""/"route", only a run-future Err does, and that path already populated error). Folded it into finalize_terminal_status(), shared by flows_run/flows_resume, which also resolves the nitpick below (the duplicated 6-line terminal-status block) in the same change — one place now owns both the status precedence and the error population, so they can't drift again. Added regression tests: failed_step_error_summary_*, finalize_terminal_status_*, and an end-to-end flows_run_populates_error_when_a_continue_policy_node_errors using an on_error: "continue" node.
There was a problem hiding this comment.
@graycyrus, thanks for the detailed writeup — this looks like a clean fix. Consolidating the precedence + error-population logic into finalize_terminal_status() is exactly right; it removes the duplication between flows_run/flows_resume and closes the gap where a "failed" row could persist with error: None. Good call using node-name attribution in failed_step_error_summary() given the engine doesn't carry a message for continue/route-policy step failures. Appreciate the added regression coverage too.
🧠 Learnings used
Learnt from: sanil-23
Repo: tinyhumansai/openhuman PR: 416
File: src/openhuman/memory/relex.rs:441-464
Timestamp: 2026-04-07T15:49:51.275Z
Learning: When using the `ort` Rust crate v2.x with the `load-dynamic` feature enabled, don’t require individual execution-provider feature flags (e.g., `directml`, `coreml`, `cuda`) alongside `load-dynamic` to get EP registration code. The `ort` crate already compiles EP registration via `#[cfg(any(feature = "load-dynamic", feature = "<ep_name>"))]` guards, and adding per-EP feature flags can pull in static-linking dependencies that conflict with the dynamic loading approach. At runtime, EP availability is determined by what the dynamically loaded ONNX Runtime library (`onnxruntime.dll`/`.so`/`.dylib`) supports; ort docs indicate providers like `directml`/`xnnpack`/`coreml` are available in builds when the platform supports them.
Learnt from: sanil-23
Repo: tinyhumansai/openhuman PR: 416
File: src/openhuman/memory/relex.rs:441-464
Timestamp: 2026-04-07T15:49:51.275Z
Learning: When integrating the `ort` Rust crate v2.x with the `load-dynamic` feature enabled, do NOT also require/enable individual provider EP Cargo features like `directml`, `coreml`, or `cuda`. In `ort` v2.x, EP registration for providers (e.g., DirectML, CoreML, CUDA, etc.) is already compiled in under source-level `#[cfg(any(feature = "load-dynamic", feature = "<provider>"))]` guards, such as in `ep/directml.rs`. Adding provider feature flags alongside `load-dynamic` can pull in static-linking dependencies that conflict with the dynamic-loading approach. Provider availability should be treated as runtime-determined by what the loaded `onnxruntime` library (`onnxruntime.dll`/`libonnxruntime.so`/`libonnxruntime.dylib`) actually supports.
Learnt from: oxoxDev
Repo: tinyhumansai/openhuman PR: 571
File: src/openhuman/local_ai/service/whisper_engine.rs:69-80
Timestamp: 2026-04-14T19:59:04.826Z
Learning: When reviewing Rust code in this repo that uses the upstream `whisper-rs` crate (v0.16.0), do not report `WhisperContextParameters::use_gpu(...)` or `WhisperContextParameters::flash_attn(...)` as missing/invalid APIs. These builder-style methods exist upstream and return `&mut Self`; they are not limited to `WhisperVadContextParams`.
Learnt from: graycyrus
Repo: tinyhumansai/openhuman PR: 1078
File: src/openhuman/agent/agents/welcome/prompt.rs:24-24
Timestamp: 2026-05-01T13:41:00.958Z
Learning: For Rust code under `src/openhuman/**/*.rs`, use `snake_case` for local variables (not `camelCase`). If a local variable name is written in `camelCase`, treat it as a style/lint issue because it will trigger Rust’s `non_snake_case` warning (and related clippy linting, if enabled). Avoid suggesting `camelCase` for any Rust local variable names in this repository.
Learnt from: senamakel
Repo: tinyhumansai/openhuman PR: 1173
File: tests/agent_memory_loader_public.rs:88-88
Timestamp: 2026-05-04T06:50:47.877Z
Learning: In this repository, the general camelCase naming guideline should not be applied to Rust source files. For all .rs files, Rust function (and related) names should use snake_case, and snake_case Rust function names should not be flagged—even for async test functions annotated with attributes like #[tokio::test]. This is consistent with Rust’s non_snake_case lint behavior.
Learnt from: sanil-23
Repo: tinyhumansai/openhuman PR: 3775
File: src/openhuman/inference/openai_oauth/flow_tests.rs:605-638
Timestamp: 2026-06-19T04:39:51.616Z
Learning: When reviewing path usage in the `tinyhumansai/openhuman` Rust code (especially auth/session storage), don’t flag a “path-mismatch” concern for `AuthService::from_config` just because it doesn’t use `workspace_dir`. `AuthService::from_config` resolves its state directory via `state_dir_from_config(config)`, which returns `config_path.parent()` (see `src/openhuman/credentials/core.rs:119-124`). In tests that use `test_config(&tmp)` (where `config_path = tmp.path().join("config.toml")`), creating `AuthProfilesStore::new(tmp.path(), false)` writes into the same directory that `AuthService::from_config` will read (the config parent directory).
Learnt from: senamakel
Repo: tinyhumansai/openhuman PR: 4087
File: src/openhuman/thread_goals/ops.rs:41-47
Timestamp: 2026-06-25T07:34:02.244Z
Learning: In this repo, controller/RPC response shapes follow an established asymmetric convention: (1) read handlers should return the bare serialized value (no `{ result, logs }` envelope), and (2) mutation handlers should return an envelope shaped like `{ result, logs }`. When reviewing Rust RPC controller implementations in `src/openhuman/**/ops.rs`, follow this convention. For the thread-goals surface specifically, do not flag `thread_goals_get` returning a bare `GoalEnvelope` if the frontend/API helper (`app/src/services/api/threadGoalApi.ts`) and existing E2E helpers already `unwrap()`/accept both bare and envelope shapes.
…pe terminal-status logic (addresses @coderabbitai on src/openhuman/flows/ops.rs:1084) flows_run/flows_resume persisted status: "failed" with error: None whenever degrade_completed_status degraded a settled run (a node under an on_error:"continue"/"route" policy errors but the run future still returns Ok) — leaving the row inconsistent with FlowRun::error's own doc contract ("Error message when status == \"failed\"") and losing the only failure detail in run history. Add failed_step_error_summary(), which names the node(s) whose step settled with status "error" (the engine's ExecutionStep carries no message of its own for this case — only a run-future Err, which already has its own message, does). Fold it into a new finalize_terminal_status() shared by both call sites, which also resolves CodeRabbit's nitpick that the two call sites' 6-line terminal-status block was byte-for-byte duplicated (and had already drifted once, on this very error-population gap).
1 participant
Problem
A flow run whose
=-bindings resolved to null (Slackchannel/text, summarize input) reported green "Completed" — a hollow run shown as success. The observer already records per-stepdiagnostics; the terminal status derivation just ignored them (it was derived solely from pending approvals).Fix
flows_run/flows_resumenow derive status from the settled steps:status == error→faileddiagnostics→completed_with_warnings(amber)completedPrecedence:
pending_approval > failed > completed_with_warnings > completed. Singlesettle_stepsread (no extra DB hit).Frontend:
FlowRunStatusunion + inspector status maps (amber badge, non-pulsing dot) +TERMINALpoller set + i18n (14 locales). Also fills the pre-existingcancelledgap in the TS union/maps.Independent of #4586
Reads diagnostics the observer already writes on main. (Trivial
ops.rsrebase if #4586 lands first.)Verification
cargo
flows::279 pass (4 newdegrade_completed_statustests) · typecheck/eslint clean · vitest 167 pass ·i18n:check0 drift. Validated by a live run: a null-binding run now surfaces amber instead of a false green.Summary by CodeRabbit