fix(tinyflows): preserve explicit node timeouts, align discover timeout, log cap pins#4877
Conversation
📝 WalkthroughWalkthroughThe PR adds diagnostics for pinned agent iteration budgets, revises harness and flow discovery timeouts, updates registry expectations, and migrates canvas validation tests to imperative save metadata. ChangesExecution timeout behavior
Registry expectations
Canvas validation tests
Estimated code review effort: 3 (Moderate) | ~25 minutes Possibly related PRs
Suggested labels: Suggested reviewers: 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: 76c4abfa6e
ℹ️ 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".
| /// ~10s/iteration can take up to ~500s — the old 300s bound could clip a | ||
| /// legitimate long discovery run before the iteration cap ever got a chance | ||
| /// to (post-merge Codex P2 finding). | ||
| const FLOW_DISCOVER_TIMEOUT_SECS: u64 = 600; |
There was a problem hiding this comment.
When flows_discover is launched from the desktop UI, discoverWorkflows still sends timeoutMs: 310_000 (app/src/services/api/flowsApi.ts), and callCoreRpc aborts the request at that client budget. With this server timeout now allowing valid discovery runs of roughly 500-600s, those runs will still fail in the UI at about 310s while the core continues thinking, so the user never receives the suggestions. Please update the frontend per-call timeout and its test expectation to match the new server bound.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in e62e88a — raised FLOW_DISCOVER_TIMEOUT_MS in app/src/services/api/flowsApi.ts from 310_000 to 610_000 to mirror FLOW_BUILD_TIMEOUT_MS/FLOW_RESUME_TIMEOUT_MS, matching the server-side 600s FLOW_DISCOVER_TIMEOUT_SECS bound. Updated the discover-timeout assertions in flowsApi.test.ts accordingly (build/resume assertions left untouched).
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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/subconscious/profiles/memory.rs`:
- Around line 176-181: Update the debug log in the surrounding
memory-construction flow to include the stable per-tick/job correlation
identifier already used by the event context or automation origin as a
structured field. Preserve the existing mode and max_tool_iterations values, and
keep the log’s stable grep-friendly prefix while making each subconscious tick
distinguishable.
In `@src/openhuman/tinyflows/caps.rs`:
- Around line 955-956: Update the timeout handling around requested_timeout_secs
and clamp_run_timeout_secs so an explicitly provided timeout_secs must parse as
a valid unsigned integer; reject negative, fractional, string, and other
malformed values instead of treating them as absent and applying the default
budget, while preserving the existing default behavior when the field is
omitted.
🪄 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: 0376e1ca-9b66-4183-b36b-2c59270c29f4
📒 Files selected for processing (5)
src/openhuman/agent/task_dispatcher/executor.rssrc/openhuman/flows/ops.rssrc/openhuman/skill_runtime/run_machinery.rssrc/openhuman/subconscious/profiles/memory.rssrc/openhuman/tinyflows/caps.rs
76c4abf to
86916a0
Compare
coderabbitai
Bot
added
agent
Rebasing tinyhumansai#4877 onto main did not fix Rust Core Coverage on its own — a concurrent workflow-tools PR expanded workflow_builder's tool belt to 29 tools (edit_workflow, create_workflow, duplicate_flow, list_node_kinds, resume_flow_run, etc.) and tinyhumansai#4869 added memory_recall/memory_hybrid_search, but no commit updated the loader test's exact-tool-list assertion or the flows::schemas controller/handler coverage tests for the new list_all_runs function. Both were exact-list assertions against agent.toml/schemas.rs, so they now fail on main itself, independent of this PR's diff. Updated the expected lists to match the current agent.toml [tools].named and the registered flows controllers, confirmed via cargo test.
…r bound Addresses @chatgpt-codex-connector on src/openhuman/flows/ops.rs:3017 (PR tinyhumansai#4877, comment 3584685815). FLOW_DISCOVER_TIMEOUT_SECS was raised to 600 server-side, but discoverWorkflows still sent timeoutMs: FLOW_DISCOVER_TIMEOUT_MS = 310_000, so the client aborted at ~310s while a worst-case discovery run kept going server-side. Raised FLOW_DISCOVER_TIMEOUT_MS to 610_000 to mirror FLOW_BUILD_TIMEOUT_MS / FLOW_RESUME_TIMEOUT_MS, and updated flowsApi.test.ts's discover-timeout assertions accordingly.
Addresses @coderabbitai on src/openhuman/subconscious/profiles/memory.rs:181 (PR tinyhumansai#4877, comment 3584695533). mode and max_tool_iterations repeat across ticks, so the post-construction iteration-budget pin log couldn't distinguish concurrent/successive subconscious ticks. Mint the "subconscious:tick:<secs>" id once and thread it through the pin log (as a structured tick_id field), the decision-agent spawn log, set_event_context, and the TrustedAutomation origin's job_id — previously the event context and origin job_id each called now_secs() independently, so they could disagree across a second boundary within the same tick.
Rebase + CI reconciliationRebased onto
This is a concurrent-merge collision: a workflow-tools PR expanded Also addressed in this push:
|
coderabbitai
Bot
removed
rust-core
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/subconscious/profiles/memory.rs`:
- Around line 169-174: Update the tick correlation ID initialization near
tick_id in the subconscious tick flow to use a unique per-tick value, such as
the runner’s monotonic ID or a UUID, instead of now_secs() as u64. Preserve
reuse of this ID for the pin log, agent event context, and turn origin job_id.
🪄 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: 54750c7b-e96d-40a8-b207-5fb745e1d3d9
📒 Files selected for processing (5)
app/src/services/api/flowsApi.test.tsapp/src/services/api/flowsApi.tssrc/openhuman/agent_registry/agents/loader.rssrc/openhuman/flows/schemas.rssrc/openhuman/subconscious/profiles/memory.rs
| // Stable per-tick correlation id — minted once and reused below for | ||
| // the pin log, the agent's event context, and the turn origin's | ||
| // `job_id`, so `mode`/`max_tool_iterations` (which repeat across | ||
| // ticks) don't leave concurrent/successive subconscious ticks | ||
| // indistinguishable in logs. | ||
| let tick_id = format!("subconscious:tick:{}", now_secs() as u64); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Locate the file and nearby references.
git ls-files | rg '^src/openhuman/subconscious/profiles/memory\.rs$|^src/openhuman/subconscious/profiles/'
# Show the relevant section with line numbers.
sed -n '1,260p' src/openhuman/subconscious/profiles/memory.rs
# Search for tick_id / job_id / correlation / now_secs usage in the same area.
rg -n "tick_id|job_id|now_secs|correlation|TrustedAutomation|max_tool_iterations|mode" src/openhuman/subconscious -SRepository: tinyhumansai/openhuman
Length of output: 37196
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '1,260p' src/openhuman/subconscious/profiles/memory.rsRepository: tinyhumansai/openhuman
Length of output: 11926
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the runner tick ID and the local time helpers.
sed -n '240,380p' src/openhuman/subconscious/instance.rs
printf '\n---\n'
sed -n '580,620p' src/openhuman/subconscious/instance.rs
printf '\n---\n'
sed -n '430,455p' src/openhuman/subconscious/profiles/memory.rsRepository: tinyhumansai/openhuman
Length of output: 7114
Use a unique per-tick correlation ID. now_secs() as u64 truncates to whole seconds, so concurrent or rapid successive ticks can share the same tick_id, making the log context and TrustedAutomation.job_id ambiguous. Reuse a monotonic runner ID or a UUID here.
🤖 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/subconscious/profiles/memory.rs` around lines 169 - 174, Update
the tick correlation ID initialization near tick_id in the subconscious tick
flow to use a unique per-tick value, such as the runner’s monotonic ID or a
UUID, instead of now_secs() as u64. Preserve reuse of this ID for the pin log,
agent event context, and turn origin job_id.
Source: Coding guidelines
…ut, log cap pins Post-merge follow-up on PR tinyhumansai#4870 (issue tinyhumansai#4868) addressing Codex/CodeRabbit findings that shipped unaddressed: - caps.rs: scale_timeout_for_iteration_cap was applied unconditionally to base_timeout_secs in run_via_harness, so an agent node that explicitly set timeout_secs (a fast-fail/SLA bound) got silently scaled up to match a high iteration cap. Added resolve_run_timeout_secs, which only scales the *default* 240s timeout — an explicit caller value is clamped (10..=600) but never widened. - flows/ops.rs: flow_discovery now resolves to a 50-iteration cap (like workflow_builder), but FLOW_DISCOVER_TIMEOUT_SECS was left at 300s while workflow_builder's equivalent got bumped to 600s. Raised it to 600 to match. - Added state-transition debug logs at the three post-construction agent.set_max_tool_iterations(...) overrides (task_dispatcher/executor.rs, skill_runtime/run_machinery.rs, subconscious/profiles/memory.rs) noting the iteration budget was pinned after construction, mirroring factory.rs's cap-resolution logging convention.
Rebasing tinyhumansai#4877 onto main did not fix Rust Core Coverage on its own — a concurrent workflow-tools PR expanded workflow_builder's tool belt to 29 tools (edit_workflow, create_workflow, duplicate_flow, list_node_kinds, resume_flow_run, etc.) and tinyhumansai#4869 added memory_recall/memory_hybrid_search, but no commit updated the loader test's exact-tool-list assertion or the flows::schemas controller/handler coverage tests for the new list_all_runs function. Both were exact-list assertions against agent.toml/schemas.rs, so they now fail on main itself, independent of this PR's diff. Updated the expected lists to match the current agent.toml [tools].named and the registered flows controllers, confirmed via cargo test.
…r bound Addresses @chatgpt-codex-connector on src/openhuman/flows/ops.rs:3017 (PR tinyhumansai#4877, comment 3584685815). FLOW_DISCOVER_TIMEOUT_SECS was raised to 600 server-side, but discoverWorkflows still sent timeoutMs: FLOW_DISCOVER_TIMEOUT_MS = 310_000, so the client aborted at ~310s while a worst-case discovery run kept going server-side. Raised FLOW_DISCOVER_TIMEOUT_MS to 610_000 to mirror FLOW_BUILD_TIMEOUT_MS / FLOW_RESUME_TIMEOUT_MS, and updated flowsApi.test.ts's discover-timeout assertions accordingly.
Addresses @coderabbitai on src/openhuman/subconscious/profiles/memory.rs:181 (PR tinyhumansai#4877, comment 3584695533). mode and max_tool_iterations repeat across ticks, so the post-construction iteration-budget pin log couldn't distinguish concurrent/successive subconscious ticks. Mint the "subconscious:tick:<secs>" id once and thread it through the pin log (as a structured tick_id field), the decision-agent spawn log, set_event_context, and the TrustedAutomation origin's job_id — previously the event context and origin job_id each called now_secs() independently, so they could disagree across a second boundary within the same tick.
…controls moved to FlowCanvasPage
197c72e to
6eff011
Compare
|
Fixed a pre-existing Root cause: a canvas-refactor PR moved the Save / Discard / dirty-badge buttons out of Fix: re-scoped the test to drive the canvas through its actual current contract — the imperative
No changes needed in Verified: |
coderabbitai
Bot
added
agent
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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
`@app/src/components/flows/canvas/__tests__/EditableFlowCanvas.validation.test.tsx`:
- Around line 134-135: Update the save calls in EditableFlowCanvas validation
tests, including the initialDirty case, to run inside an awaited async act and
await the asynchronous save path before assertions. Preserve the existing save
and error expectations while ensuring saving, baseline, and forcedDirty updates
remain within the act scope.
🪄 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: 276bd628-6d32-4951-ab8c-b560cb8c48bd
📒 Files selected for processing (10)
app/src/components/flows/canvas/__tests__/EditableFlowCanvas.validation.test.tsxapp/src/services/api/flowsApi.test.tsapp/src/services/api/flowsApi.tssrc/openhuman/agent/task_dispatcher/executor.rssrc/openhuman/agent_registry/agents/loader.rssrc/openhuman/flows/ops.rssrc/openhuman/flows/schemas.rssrc/openhuman/skill_runtime/run_machinery.rssrc/openhuman/subconscious/profiles/memory.rssrc/openhuman/tinyflows/caps.rs
🚧 Files skipped from review as they are similar to previous changes (9)
- src/openhuman/skill_runtime/run_machinery.rs
- src/openhuman/agent/task_dispatcher/executor.rs
- src/openhuman/subconscious/profiles/memory.rs
- app/src/services/api/flowsApi.ts
- src/openhuman/flows/schemas.rs
- app/src/services/api/flowsApi.test.ts
- src/openhuman/flows/ops.rs
- src/openhuman/tinyflows/caps.rs
- src/openhuman/agent_registry/agents/loader.rs
| act(() => ref.current?.save()); | ||
| expect(onSave).toHaveBeenCalledTimes(1); |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '--- test file outline ---'
ast-grep outline app/src/components/flows/canvas/__tests__/EditableFlowCanvas.validation.test.tsx --view expanded || true
echo
echo '--- implementation candidates ---'
fd -a 'EditableFlowCanvas*' app/src/components/flows/canvas || true
rg -n "save\\s*\\(" app/src/components/flows/canvas -g '!**/*.test.*' -g '!**/__tests__/**' || true
echo
echo '--- cited test file excerpt ---'
sed -n '110,220p' app/src/components/flows/canvas/__tests__/EditableFlowCanvas.validation.test.tsxRepository: tinyhumansai/openhuman
Length of output: 5782
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '--- cited test file excerpt with line numbers ---'
cat -n app/src/components/flows/canvas/__tests__/EditableFlowCanvas.validation.test.tsx | sed -n '120,220p'Repository: tinyhumansai/openhuman
Length of output: 5080
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Locate the implementation and nearby tests
rg -n "save\\s*\\(" app/src/components/flows/canvas -g '!**/*.test.*' -g '!**/__tests__/**' -g '!**/*.d.ts' || true
echo
echo '--- test file excerpt ---'
cat -n app/src/components/flows/canvas/__tests__/EditableFlowCanvas.validation.test.tsx | sed -n '120,220p'Repository: tinyhumansai/openhuman
Length of output: 5185
🏁 Script executed:
#!/bin/bash
set -euo pipefail
pwd
ls app/src/components/flows/canvasRepository: tinyhumansai/openhuman
Length of output: 383
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '--- outline ---'
ast-grep outline app/src/components/flows/canvas/EditableFlowCanvas.tsx --view expanded || true
echo
echo '--- save-related symbols ---'
rg -n "useImperativeHandle|save\\s*:|\\bsave\\b|onSave|setState|await " app/src/components/flows/canvas/EditableFlowCanvas.tsx
echo
echo '--- relevant excerpt ---'
cat -n app/src/components/flows/canvas/EditableFlowCanvas.tsx | sed -n '1,260p'Repository: tinyhumansai/openhuman
Length of output: 14884
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '--- save implementation slice ---'
cat -n app/src/components/flows/canvas/EditableFlowCanvas.tsx | sed -n '520,620p'Repository: tinyhumansai/openhuman
Length of output: 4610
Await save() inside act. ref.current?.save() calls an async save path that awaits onSave and then updates saving, baseline, and forcedDirty; the sync act can leave those updates outside the act scope and make the save/error tests flaky. Use await act(async () => { ref.current?.save(); }) for the save calls here, including initialDirty.
🤖 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
`@app/src/components/flows/canvas/__tests__/EditableFlowCanvas.validation.test.tsx`
around lines 134 - 135, Update the save calls in EditableFlowCanvas validation
tests, including the initialDirty case, to run inside an awaited async act and
await the asynchronous save path before assertions. Preserve the existing save
and error expectations while ensuring saving, baseline, and forcedDirty updates
remain within the act scope.
1 participant
Summary
Follow-up to #4870 (issue #4868, systemic agent iteration-cap fix), addressing review findings that shipped unaddressed on
main: two real Codex P2 bugs and three CodeRabbit logging-convention nits.Fix 1 (Codex P2, real) —
scale_timeout_for_iteration_capinsrc/openhuman/tinyflows/caps.rswas applied unconditionally tobase_timeout_secsinOpenHumanAgentRunner::run_via_harness, so an agent node that explicitly settimeout_secs(e.g. a fast-fail/SLA bound like120) on a 50-iteration-cap agent got silently bumped up to 600s. Addedresolve_run_timeout_secs, which only widens the default (notimeout_secssupplied) 240s bound via iteration-cap scaling; an explicit caller value is clamped to10..=600as before but never scaled up.clamp_run_timeout_secs's existing test (Some(120) -> 120) is untouched; added new tests covering the combination:resolve_run_timeout_secs_preserves_an_explicit_request_even_for_a_high_cap_agent—Some(120)+ cap 50 stays120.resolve_run_timeout_secs_scales_the_default_up_for_a_high_cap_agent—None+ cap 50 scales to600.resolve_run_timeout_secs_leaves_low_cap_agents_unscaled_either_way.Fix 2 (Codex P2, real) —
flow_discovery(src/openhuman/flows/ops.rs) resolves to the same 50-iteration effective cap asworkflow_builder, butflows_discoverwas still wrapped inFLOW_DISCOVER_TIMEOUT_SECS = 300whileworkflow_buildergotFLOW_BUILD_TIMEOUT_SECS = 600for the same cap. A worst-case ~10s/iteration run at 50 iterations (~500s) could get killed by the 300s wall clock before the iteration budget was ever exhausted. RaisedFLOW_DISCOVER_TIMEOUT_SECSto600to match, with doc comment mirroringFLOW_BUILD_TIMEOUT_SECS's. The timeout log message interpolates the const directly, so it picks up the new value automatically.Fix 3 (CodeRabbit x3, logging convention) — the three post-construction
agent.set_max_tool_iterations(...)overrides had no log line (unlikefactory.rs's definition-cap resolution logging). Added a concise state-transition log at each, noting the iteration budget was pinned post-construction and overrides the session builder's resolved cap:src/openhuman/agent/task_dispatcher/executor.rs(TASK_RUN_MAX_ITERATIONS/ 200)src/openhuman/skill_runtime/run_machinery.rs(WORKFLOW_RUN_MAX_ITERATIONS/ 200)src/openhuman/subconscious/profiles/memory.rs(mode-specific 15/30 cap)Test plan
GGML_NATIVE=OFF cargo check --manifest-path Cargo.toml— clean, no new warnings.cargo test --manifest-path Cargo.toml --lib openhuman::tinyflows:: -- --test-threads=1— 129 passed, 0 failed (includes 3 newresolve_run_timeout_secs_*tests).cargo test --manifest-path Cargo.toml --lib openhuman::flows:: -- --test-threads=1— 290 passed, 0 failed.cargo test --manifest-path Cargo.toml --lib openhuman::agent:: -- --test-threads=1— 964 passed, 0 failed, after excluding two pre-existing failures unrelated to this diff and confirmed to reproduce identically on unmodifiedupstream/main(verified viagit stash/git stash poparound each):turn_dispatches_spawn_subagent_through_full_path(stack overflow) andturn_triggers_configured_memory_agent_before_parent_prompt(assertion mismatch). Neither touches any file this PR changes.cargo fmt— no diffs beyond the changes above.PR submission checklist
cargo check/cargo fmtclean.Summary by CodeRabbit