Repository navigation
Add OpenHuman host profile to memory eval - #252
Conversation
Co-authored-by: Medulla <medulla@tinyhumans.ai>
Tiny Sweeper reviewTiny Sweeper reviewed this change across 6 lane(s) and found 11 active actionable finding(s). Detailed lane evidence and any incomplete work are listed below. State: Changes requested Review snapshot
Completeness: Complete What changedThe review could not produce a supported behavioral summary; inspect the cited changed surface and lane details below. FeaturesNone identified with supported citations. TestsNo supported feature-to-test mapping was produced. Test execution is not inferred. Findings
Before merge
Agent review detailscritique
security
tests
commits
description
e2e
Evidence and run details
|
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
|
Note Currently processing new changes in this PR. This may take a few minutes, please wait... ⚙️ Run configuration
📒 Files selected for processing (11)
Comment |
![tinysweeper[bot]](https://avatars.githubusercontent.com/in/4526585?s=60&v=4){kind=small}
There was a problem hiding this comment.
Requesting changes: 2 lane(s) blocking, worst finding is high.
Fix or reply to the findings below and push. The next review clears this automatically once they are gone — you should not need to dismiss anything by hand.
$0.0384 · 545,267 in / 32,810 out · 48,080 cached (9%) · gpt-5.6-luna, glm-5.3-flash
critique: $0.0220 · 289,698 in / 19,410 out · 29,710 cached (10%) · gpt-5.6-luna, glm-5.3-flash
security: $0.0157 · 186,919 in / 9,200 out · 18,242 cached (10%) · gpt-5.6-luna
tests: $0.0002 · 16,455 in / 1,132 out · 0 cached (0%) · glm-5.3-flash
description: $0.0001 · 16,323 in / 359 out · 0 cached (0%) · glm-5.3-flash
e2e: $0.0002 · 18,670 in / 691 out · 0 cached (0%) · glm-5.3-flash
| tokio::spawn(async move { memory.pre_turn_dated(pre, false, async { None }).await }); | ||
| match tokio::time::timeout(PRE_TURN_TIMEOUT, &mut task).await { | ||
| Ok(context) => Ok((pack(context??.pack), false, ms(started))), | ||
| Err(_) => { |
There was a problem hiding this comment.
Return the empty pack without awaiting the timed-out task
When pre_turn_dated exceeds PRE_TURN_TIMEOUT, this branch immediately awaits the spawned task to completion before returning. That means a slow recall operation still blocks the probe and the evaluation cannot continue at the host deadline; the recorded elapsed only measures time until the timeout, so the reported latency also hides the extra wait. The timed-out task should be retained for later cleanup/flush, or otherwise detached while the empty pack is returned at the deadline.
[RULE] deadline-bypass ·
| | Run | Scope | Recall / synthesis pack hit | Synthesis model answer | Pre-turn p95 | Timeout rate | CortexDB cost | | ||
| | --- | --- | --- | --- | --- | --- | --- | | ||
| | `openhuman-cortex-mock` | `tool_heavy,compaction`, 8 probes | 8/8 · 8/8 | no answer model | 48 ms | 0/27 scripted turns | $0.000 | | ||
| | `openhuman-cortex-mock-full` | all 12 scenarios, 53 probes | 41/52 · 40/52 | no answer model | 358 ms | 0/175 calls | $0.000 | |
There was a problem hiding this comment.
Reconcile the probe count with the reported hit rates
This row says the run covered 53 probes, but both recall and synthesis results use a denominator of 52, with no explanation for the omitted probe. That makes the documented coverage and hit rates inconsistent and prevents readers from reproducing or trusting the measurement. Make the probe count and denominators agree, or explicitly document why one probe is excluded.
[RULE] inconsistent-metrics ·
| .flat_map(|r| &r.probes) | ||
| .filter(|probe| probe.timed_out) | ||
| .count(); |
There was a problem hiding this comment.
Count timeouts only for pre-turn probes
The denominator below counts only probes whose via starts with pre_turn, but this numerator counts every timed-out probe. If a non-pre-turn probe times out, it increases the numerator without increasing the denominator, producing an incorrect rate or even a value above 100%. Apply the same via filter when counting timed-out probes.
| .flat_map(|r| &r.probes) | |
| .filter(|probe| probe.timed_out) | |
| .count(); | |
| .flat_map(|r| &r.probes) | |
| .filter(|probe| probe.via.starts_with("pre_turn") && probe.timed_out) | |
| .count(); |
[RULE] inconsistent-filtering ·
| tokio::spawn( | ||
| async move { memory.pre_turn_dated(pre, false, async { None }).await }, | ||
| ); | ||
| match tokio::time::timeout(PRE_TURN_TIMEOUT, &mut task).await { |
There was a problem hiding this comment.
Reserve the turn index before allowing timed-out tasks to overlap
When this timeout branch stores the still-running task and returns, self.next is not advanced until the later post_turn completes. If the caller starts another user call before the pending task finishes, that call constructs another PreTurn with the same turn index and can log concurrently with the first task. The resulting conversation can contain duplicate user-turn indices and assistant logging in the wrong order. Reserve/increment the turn position before launching or accepting an overlapping turn, or serialize subsequent turns until the pending pre-turn has committed.
[RULE] concurrent-turn-ordering ·
| /// those tasks running, so their accepted user turns may still land. | ||
| pub(crate) async fn flush(&mut self) -> Result<usize, Error> { | ||
| let mut logged = 0; | ||
| for task in self.pending.drain(..) { |
There was a problem hiding this comment.
Retain pending tasks when flushing encounters an error
If any drained task returns a JoinError or an inner pre-turn error, ? exits the loop immediately. The remaining JoinHandles have already been removed from self.pending; dropping Tokio join handles detaches those tasks rather than cancelling them, so they can continue writing memory after flush has reported failure and can never be observed by a later flush. Await all tasks while retaining or aggregating errors, or restore unprocessed handles to self.pending before returning.
[RULE] unobserved-background-task ·
| runner.run(job).await?; | ||
| } | ||
| } | ||
| agent.flush().await?; |
There was a problem hiding this comment.
Clean up the evaluation namespace on every exit path
After data is stored, any error from agent.flush(), engine.export(), or the later forget() returns immediately through ?, bypassing the cleanup at the end of the function. A failed probe therefore leaves its namespace and items behind, which can pollute subsequent evaluation runs or accumulate indefinitely. Arrange for the forget operation to run in an error-safe cleanup guard or explicitly clean up before propagating each post-store error.
[RULE] cleanup-on-error ·
| Ok(context) => Ok((pack(context??.pack), false, ms(started))), | ||
| Err(_) => { | ||
| let elapsed = ms(started); | ||
| task.await??; |
There was a problem hiding this comment.
Detach timed-out pre-turn tasks instead of awaiting them
After timeout returns Err, the spawned task is still running because the handle was borrowed. Awaiting it here blocks until pre_turn_dated completes, so a slow or stuck recall defeats the documented 1500 ms host deadline and prevents the simulated host from continuing with an empty pack. Drop or otherwise detach the handle after the timeout so the write can finish asynchronously while this function returns at the deadline.
| task.await??; | |
| drop(task); |
[RULE] timeout-not-enforced ·
| pub(crate) echoed_items: usize, | ||
| } | ||
|
|
||
| fn mean(values: impl Iterator<Item = usize>) -> f64 { |
There was a problem hiding this comment.
Add a test module for loop_guard's pure helpers
The repo's rules require sibling <module>_tests.rs unit tests and ~80% coverage of meaningful behaviour with every behaviour change. loop_guard.rs adds pure, easily tested logic — mean (including its empty-slice guard) and the duplicate-rate closure (including its lines == 0 branch) — plus the echo-scan counting, and none of it is covered; the only verification is the manual run recorded in the docs. Extract rate into a named function next to mean and add loop_guard_tests.rs covering the empty and zero-division branches so a regression in the reported rates fails a test rather than only a future eval run.
[RULE] missing-test-coverage ·
| self | ||
| } | ||
|
|
||
| /// Wait for pre-turn tasks whose host deadline expired. OpenHuman leaves |
There was a problem hiding this comment.
Test the pre-turn timeout and flush path, not only the on-time path
The docs (openhuman-host.md) and the code comments claim a timed-out task keeps running and can still log the user turn, and timed_out: context.is_none() plus flush() exist to model that. The new agent_tests.rs only covers the on-time path (assert!(!record.timed_out)); the timeout branch and flush are never executed by any test, so if the pending-task bookkeeping regressed (e.g. flush stopped counting logs, or timed-out turns were dropped instead of logged) nothing would fail. Add a test that forces the 1500 ms deadline to expire — e.g. by using an engine whose pre_turn_dated is delayed beyond PRE_TURN_TIMEOUT (the constant is pub(crate), so a test-shorter deadline is also possible) — and asserts record.timed_out is true and flush() returns the late-logged count.
[RULE] untested-invariant ·
| "--json" => parsed.json = Some(value()?), | ||
| "--label" => parsed.label = value()?, | ||
| "--llm" => parsed.llm = true, | ||
| "--host" => parsed.host = value()?, |
There was a problem hiding this comment.
Drive the new openhuman/loop-guard flags end to end
The new eval flags are the external surface of this change: --host openhuman changes the recall policy, prompt wrapping and reply logging, and --loop-guard fails the run when stored items contain an injected <memory-context> tag. There is no e2e workflow in the tree and no CI job runs scripts/memory-eval.sh with these flags; the candidate lexical hits in integration/cortexdb/ are unrelated comment text. Coverage relies on unit tests (agent_tests.rs, kpi_tests.rs) plus the manually recorded measured runs in docs/evals/openhuman-host.md. A test would have to run the eval binary through scripts/memory-eval.sh --host openhuman --loop-guard --only none (or cargo run --example memory_eval ... --engine reference --loop-guard) and assert exit status 0 with "echoed_items": 0 in the report, so a regression that re-introduces pack echo or breaks the flag parsing fails a job rather than a documentation table.
[RULE] e2e-uncovered ·
tinysweeper
Bot
added
the
priority: p1
![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: 61bfc4082c
ℹ️ About Codex in GitHub
Your team has set up Codex to 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 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| loop { | ||
| let mut req = ListRequest::new(layout.holistic_filter(), 100); | ||
| req.cursor = cursor; | ||
| let page = engine.export(req).await?; |
There was a problem hiding this comment.
On CortexDB, the lifecycle stores used by ScriptedAgent return at WaitFor::Accepted, so flush() only waits for timed-out pre-turn tasks to finish and does not guarantee that those turns (or the latest assistant turns) are listable. Exporting immediately can therefore omit the tail of the 500-turn thread and report echoed_items == 0 even when a not-yet-visible stored item contains the tag; the subsequent filtered forget can race the same writes as well. Count the accepted user/assistant writes and perform the same visibility-settle step used by Eval::settle before exporting.
Useful? React with 👍 / 👎.
| Ok(context) => Ok((pack(context??.pack), false, ms(started))), | ||
| Err(_) => { | ||
| let elapsed = ms(started); | ||
| task.await??; |
There was a problem hiding this comment.
Whenever a probe's pre-turn exceeds 1500 ms, this await prevents Eval::probe from invoking the answer model or starting the next probe until the memory task actually finishes. That is the opposite of the OpenHuman timeout path being mirrored, which proceeds with an empty pack while the task remains in the background; in the documented live runs where many probes time out, this serializes request load and can skew later timeout results, and a stuck task hangs the eval despite the deadline. Retain or detach the handle and flush it separately instead of awaiting it on this path.
Useful? React with 👍 / 👎.
1 participant
Summary
Add the first OpenHuman host eval profile to
memory_eval. Scripted turns now use OpenHuman's recall policy, 1500 ms pre-turn deadline, continued background task after a timeout, and bounded logged tool-result lines. Pre-turn probes obey the same deadline. A JSON-scripted 500-turn loop guard measures pack growth and repeated lines and fails if an injected<memory-context>tag reaches stored items. Reports include p99 and timeout rate. The eval script can reuse its existing Docker image when Docker Hub is unavailable.This is a scoped part of #251. The live model step, memory tool, source/import paths, larger JSON scenario suite, leakage audit, fuzzing, and engine guide remain in that issue.
Related issue
Refs #251
API or behavior changes
None to the library API. The
memory_evalexample gains--host openhumanand--loop-guard;scripts/memory-eval.shgainsREUSE_IMAGE=1.Validation
cargo fmt --all -- --checkcargo clippy --all-targets --all-features -- -D warningscargo build --all-targets --all-featurescargo test --all-features --quietbash -n scripts/memory-eval.shtool_heavyrepeats: 0/6 recall pack hits in each, 5/6 synthesis pack hits in each, 10–12/19 pre-turn timeouts; CortexDB model cost $0.012–$0.050 per runThe live repeats expose a latency gap against the proposed <1% timeout gate. They are a six-probe scenario, not an estimate of the full suite. Each run used
--enrich-wait 90and the existing--llmanswer model.Tests
Added eval unit coverage for logged tool-result formatting, pack tag isolation in stored turns, and combined scripted/probe timeout arithmetic. The full 500-turn behavior is covered by the executable eval; no transport-fault injection was added in this slice.
Documentation
Updated
docs/evals/README.mdand addeddocs/evals/openhuman-host.mdwith the host parity table, commands, measured results, and the remaining #251 work.Checklist
#[allow(...)],#[ignore], or relaxed lints.envcontents in the diff or the descriptionSummary by CodeRabbit
New Features
Documentation