Skip to content

fix(harness): reserve the penultimate capped call for the deliverable, and mock search in the life scenarios - #6561

Merged
senamakel merged 29 commits into
tinyhumansai:mainfrom
senamakel:life-scenarios-baggage-policy
Sep 23, 2026
Merged

senamakel merged 29 commits into
tinyhumansai:mainfrom
senamakel:life-scenarios-baggage-policy

Conversation

@senamakel

@senamakel senamakel commented Sep 23, 2026 •

Copy link
Copy Markdown
Member

Why

baggage-policy was one of three life scenarios still scoring 0. It was not a file-creation failure — #6548 fixed that, and file_write worked fine in the same turn (it wrote scan.py). The RCA found two separate defects, one in the harness and one in the benchmark rig.

The harness defect: a capped turn cannot write its deliverable

FinalCallWrapUpMiddleware clears request.tools on the last permitted model call (#6014), so the turn cannot be spent on another tool instead of answering. That is right when the turn's product is text. It is exactly wrong when the product is a file: the model arrives holding the finished content and has, structurally, nowhere to put it.

final_call_wrap_up — final permitted model call — withdrawing tools …
                     model_calls=15 max_model_calls=15 tools_withdrawn=26

27 tool calls of research, a correct summary of Delta's published limits in the reply, and out/delta_baggage_guide.md never written — so every grader check after output_exists was unreachable. The reply's own closing line was "Still to do: … write out/delta_baggage_guide.md".

Fix. One call earlier the belt is narrowed to the tools that can only write (DELIVERABLE_TOOLS = file_write, apply_patch) rather than left whole, with its own instruction saying gathering is over and that a file marking its own gaps honestly beats no file. shell is excluded on purpose: it can redirect into a file but can equally run a crawler, so keeping it would leave the belt effectively unnarrowed. The restoration of microcompact-cleared results that the conclusion already got now runs here too — a call asked to write findings down has to be able to read them.

The trade, stated plainly: a capped turn now spends its second-to-last round persisting rather than gathering, and a turn with nothing to persist loses that round. That is the same trade the final call already made, moved one step earlier. The alternative — leaving the belt intact and the instruction advisory — is what the middleware's own doc comment records as having already failed. Guarded to max_model_calls >= 3 and to belts that actually carry a writer.

The rig defect: a tool advertised that the run cannot authenticate

web_search_tool posts to /agent-integrations/parallel/search on the hosted backend. The benchmark runs BYOK on an offline local token, so it returned SESSION_EXPIRED … 401 every time — while still being advertised. The model burned two calls discovering that, then improvised a DuckDuckGo scrape, guessed delta.com paths and collected four 404s. That is most of how it reached the cap.

scripts/life-scenarios/mock-search.mjs serves that one route locally, mocking discovery but not retrieval: it ranks a fixture corpus of real URLs the agent still fetches over the network itself, so cites_delta_com and states_carryon_dimensions stay honest. An off-topic query returns nothing rather than the least-bad rows — a confidently wrong result set is worse than an empty one, because the agent spends fetches on it.

Two things cost a run each and are documented in the README:

  • api_url in config.toml loses a race to users/<runtime-id>/config.toml, which auth.set_credential creates; the override rides BACKEND_URL instead.
  • parse_envelope requires { success, data }. A bare payload fails as missing field 'success', which reads to the agent as a broken tool rather than an empty result — it gave up after six tries.

Evidence

The mechanism, end to end, on the scenario that produced the 0/1:

model_calls=14 max_model_calls=15 tools_withdrawn=24 tools_kept=2
   -> file_write  out/delta_baggage_guide.md  (4734 bytes, tool call 31 of 32)
model_calls=15 max_model_calls=15 tools_withdrawn=26
   -> the conclusion, as before

9/9, 100%, 12 delta.com URLs cited. The turn still hit the cap — the reservation is what turned it from a zero into a pass. The guide marks the transatlantic Basic Economy fee "NOT STATED" rather than inventing it, which is what the prompt asked for and what the old run never got far enough to do.

Two full suite runs, back to back:

scenario run A run B baseline
calendar-buffer 0/1 8/8 8/8
subscription-scan 11/11 11/11 11/11
baggage-policy 0/1 8/9 0/1
meal-plan 10/10 0/2 10/10
trip-itinerary 10/11 0/1 0/1
fact-check-publish 0/3 12/12 0/3
total 31/37 (84%) 39/43 (91%) 29/34 (85%)

Please don't read those totals as the result. The denominators are not comparable — the grader stops at output_exists, so a scenario that writes nothing contributes 0/1 while the same scenario passing contributes 0/12. A run can score a higher percentage by failing earlier. And every scenario but subscription-scan flipped between A and B: deepseek-v4.1-flash intermittently ends a turn without emitting a tool call, or emits a malformed one (validation error: tool web_fetch arguments.url is required, repeated identically until the turn was stopped). Single-run comparisons on this model are not evidence of much in either direction.

What is evidence is the behaviour of the turns that actually hit the cap, which is the only thing this PR changes:

run turns hitting max_model_calls=15 artifact written
baseline 2 (baggage, fact-check) 0 of 2
isolated 1 (baggage) 1 of 1 — 9/9
run A 1 (trip-itinerary) 1 of 1
run B 2 (baggage, fact-check) 2 of 2

Four capped turns since the fix, four artifacts; before it, none. The three scenarios that had never once produced a file have each now done so, and fact-check-publish went straight from 0/3 to 12/12 (all three outputs) on the run where it capped.

Verification

  • cargo test -p openhuman --lib -- agent::tinyagents → 420 passed, 0 failed (needs RUST_MIN_STACK=16777216; the default stack overflows, pre-existing)
  • 7 new middleware tests in middleware_wrap_up_final_write_tests.rs, 10 new script tests in life-scenarios-mock-search.test.mjs
  • cargo check -p openhuman --tests, pnpm rust:layout, pnpm docs:check all clean

Notes for review

  • FinalCallWrapUpMiddleware::new gained a parameter (the second instruction); five test call sites updated.
  • The restore-cleared-outcomes block was extracted into a shared method so both calls use it — that is most of the diff on final_call_wrap_up.rs and is a pure move apart from the instruction parameter.
  • A few unrelated one-line cargo fmt fixes to files that landed unformatted in fix(security): turn the autonomy policy off by default and give the agent a working file-creation route #6548 (registry.rs, policy_tests.rs mod ordering).
  • scripts/ is not covered by the repo's prettier lane (CI runs --filter openhuman-app format:check only), so the edited files there are left in their original style rather than reformatted.
  • Finding 1b's other half is still open and marked so in FINDINGS.md: a capped turn is still indistinguishable from a finished one on the wire, since hit_cap never reaches TurnUsagePayload.

Co-authored-by: Medulla medulla@tinyhumans.ai

Summary by CodeRabbit

  • New Features
    • Capped turns can now use their final available writing opportunity to save a file deliverable before providing a conclusion.
    • Life-scenario runs can use an optional local mock search service, with search requests saved alongside run results.
  • Documentation
    • Added guidance on the local mock search service and updated notes on how capped turns handle file deliverables.

senamakel and others added 29 commits September 23, 2026 17:18
Adds a new instruction constant that is injected one call before the final checkpoint in a capped turn, narrowing the available tools to only those that can write files. This addresses a failure mode where the model held a correct summary in context but had no structural way to persist it as a file, because the final call's tool belt was cleared entirely. The new instruction trades the last gathering round for a dedicated write round, making file output structural rather than merely requested.

Auto-committed-on: dragonfly
Co-authored-by: Medulla <medulla@tinyhumans.ai>
Adds a new search index fixture file to support testing of life scenario search functionality, ensuring the test suite has the necessary data structure for search-related tests.

Auto-committed-on: dragonfly
Co-authored-by: Medulla <medulla@tinyhumans.ai>
Introduce a mock search module that returns predefined results for life scenario queries, enabling frontend development and testing without a live backend service.

Auto-committed-on: dragonfly
Co-authored-by: Medulla <medulla@tinyhumans.ai>
The mock search now tracks how many distinct query terms matched a document and requires at least two (or one if the query itself has only one) before returning a result. This prevents off-topic queries from returning the least-bad rows in the corpus, which would waste the agent's fetches on confidently wrong pages.

Auto-committed-on: dragonfly
Co-authored-by: Medulla <medulla@tinyhumans.ai>
The scenario runner now starts a local mock search server when `--no-mock-search` is not passed, and points the agent's backend API URL at that server so that `web_search_tool` calls resolve locally instead of hitting the hosted backend. This allows scenario runs to work fully offline without requiring a network connection or a valid API token.

Auto-committed-on: dragonfly
Co-authored-by: Medulla <medulla@tinyhumans.ai>
Ensure the search log is flushed before composio closes, so the record of what discovery returned survives runs that fail partway through. This is critical for post-mortem analysis, as the search log is the only evidence of what was actually discovered, and it is most needed on the runs that went wrong.

Auto-committed-on: dragonfly
Co-authored-by: Medulla <medulla@tinyhumans.ai>
Reformatted the JSON fixture, mock search module, and run script to use multi-line arrays and object properties instead of single-line definitions, improving readability and making future diffs clearer. No functional changes were made.

Auto-committed-on: dragonfly
Co-authored-by: Medulla <medulla@tinyhumans.ai>
Introduce a `DELIVERABLE_TOOLS` constant and a `final_write_instruction` field so that the penultimate call of a capped turn retains only tools that can write a file the caller already knows about, rather than clearing the belt entirely. This allows the agent to safely produce a final artifact without risking discovery work that would leave no room to report the result.

Auto-committed-on: dragonfly
Co-authored-by: Medulla <medulla@tinyhumans.ai>
Extracted the logic that restores microcompact-blanked tool results into a dedicated `restore_cleared_outcomes` method, parameterized by the instruction text. This allows the same restoration behaviour to be reused by both the concluding call and the penultimate call, which needs the full tool results to write them into a file rather than to report findings. The extracted method also returns the count of restored outcomes, making the operation's effect visible to callers.

Auto-committed-on: dragonfly
Co-authored-by: Medulla <medulla@tinyhumans.ai>
When a turn has exactly one model call remaining before the conclusion, the middleware now narrows the available tools to only those that can persist a deliverable, rather than clearing the belt entirely. This prevents structural failure for turns whose product is an artifact rather than prose, by giving the agent a dedicated call to write its findings into a file before the conclusion clears the tool set.

Auto-committed-on: dragonfly
Co-authored-by: Medulla <medulla@tinyhumans.ai>
Updated the `FinalCallWrapUpMiddleware` constructor calls in tests to include a new "WRITE NOW" instruction string, reflecting a change in the middleware's API that now requires both a conclude and a write instruction.

Auto-committed-on: dragonfly
Co-authored-by: Medulla <medulla@tinyhumans.ai>
Reordered the `action_dir_tests` module declaration in `policy_tests.rs` to appear before the other test modules, and removed trailing blank lines in `final_call_wrap_up.rs` and `policy_action_dir_tests.rs`. Reformatted several test function calls in the middleware tests to improve readability by splitting long argument lists across multiple lines.

Auto-committed-on: dragonfly
Co-authored-by: Medulla <medulla@tinyhumans.ai>
Add the test module for the wrap-up final write middleware, registering it alongside the existing test modules to ensure it is included in the test suite.

Auto-committed-on: dragonfly
Co-authored-by: Medulla <medulla@tinyhumans.ai>
The test helper function ctx_at now accepts usize parameters instead of u32 to match the expected types used in RunConfig and related budget calculations, eliminating unnecessary type conversions in test code.

Auto-committed-on: dragonfly
Co-authored-by: Medulla <medulla@tinyhumans.ai>
Add test coverage for the life scenarios mock search functionality to ensure the mock behaves correctly and returns expected results. This change introduces a new test file to validate the search logic and prevent regressions.

Auto-committed-on: dragonfly
Co-authored-by: Medulla <medulla@tinyhumans.ai>
Documents the new mock search backend that replaces the hosted search tool which was returning 401 errors in local runs. The mock ranks real URLs from a fixture corpus so agents still must fetch and comprehend pages, keeping the evaluation honest while removing the paid search dependency that was causing runs to fail.

Auto-committed-on: dragonfly
Co-authored-by: Medulla <medulla@tinyhumans.ai>
Replaced asterisk-based italics with underscore-based italics and realigned the comparison table columns for consistent markdown rendering, improving readability without changing any content.

Auto-committed-on: dragonfly
Co-authored-by: Medulla <medulla@tinyhumans.ai>
Added a brief description of what the middleware does on the last and second-to-last permitted model calls, making the table entry self-documenting without requiring the reader to look up the source.

Auto-committed-on: dragonfly
Co-authored-by: Medulla <medulla@tinyhumans.ai>
Add a `searchBase` parameter to the core start method so that the `web_search_tool` can reach the correct backend for its search requests. This is done via the `BACKEND_URL` environment variable because the config file approach is unreliable: the core activates a per-user config directory at runtime, and a config written there takes precedence over the root one, making the `api_url` setting ineffective.

Auto-committed-on: dragonfly
Co-authored-by: Medulla <medulla@tinyhumans.ai>
The mock search backend was returning bare payloads instead of the `{ success, data, error }` envelope that the client's `parse_envelope` function expects. This caused late failures where the agent interpreted malformed responses as a broken search and abandoned the task. All responses now use the `ok` helper for successful replies and include the full envelope structure for errors.

Auto-committed-on: dragonfly
The mock search helper was returning the raw JSON payload, but the real backend wraps every response in a `{ success, data, error }` envelope. A bare payload causes the client to fail with a late `missing field 'success'` error that looks like a broken tool rather than an empty result, leading the agent to abandon the task. The change now unwraps the envelope and asserts `success` is true, and also updates the unauthenticated endpoint stubs to return enveloped responses for consistency.

Auto-committed-on: dragonfly
…findings with fix details

Reformatted all markdown tables to use aligned columns for readability, and updated the findings document with detailed descriptions of fixes for findings 1b (capped turn tool withdrawal) and 9 (401 errors from hosted backend), including the new mock-search rig and the trade-offs made in the fix.

Auto-committed-on: dragonfly
Adds a new section to the findings document that records the results of two full runs after the fix for finding 1b was applied, along with a caution about interpreting the totals and evidence that the fix is working.

Auto-committed-on: dragonfly
Consolidate object literals, function arguments, and error messages that were unnecessarily split across multiple lines into single-line or shorter forms, and update comments in `prepareHome` and `Core.start` to more accurately describe the relationship between the config file and the `BACKEND_URL` environment variable.

Auto-committed-on: dragonfly
Reformat the comparison table in the life-scenarios README to use GFM-style pipe tables instead of the previous alignment-based layout, and reflow several paragraphs to improve readability. The prose around the mock-search section is expanded to document three common configuration pitfalls that caused runs to fail, making the setup instructions more actionable for future users.

Auto-committed-on: dragonfly
Reformatted all markdown tables in the life-scenarios findings document from the wide, column-aligned style to a compact style with no extra spacing between columns. This is a purely cosmetic change that reduces file size and makes the raw markdown easier to read and edit, with no change to the document's content or meaning.

Auto-committed-on: dragonfly
@senamakel
senamakel requested a review from a team September 23, 2026 16:54
@tinysweeper

tinysweeper Bot commented Sep 23, 2026 •

Copy link
Copy Markdown

Tiny Sweeper review

Tiny Sweeper reviewed this change across 6 lane(s) and found 5 active actionable finding(s). Detailed lane evidence and any incomplete work are listed below.

State: Changes requested
Priority: high
Reviewed head: 3a845a27aa45
Updated: 1790182975 (Unix time)

Review snapshot

Change surface Files Review signal Count
Production 5 Active findings 5
Tests 6 Noted findings 0
Documentation 3 Resolved findings 0
Configuration 0 Pending checks/questions 5

Completeness: Complete
Test assessment: No supported feature-to-test mapping was available; this does not mean tests are absent or passed.

What changed

The review could not produce a supported behavioral summary; inspect the cited changed surface and lane details below.

Features

None identified with supported citations.

Tests

No supported feature-to-test mapping was produced. Test execution is not inferred.

Findings

  • high · critique · Reserve the penultimate call, not the final permitted call — When `max_model_calls` is 3, the first two model calls leave `remaining == 1` before the third call. This branch narrows the third and final permitted call to deliverable tools, wh (crates/openhuman\-core/src/agent/tinyagents/middleware/final\_call\_wrap\_up\.rs:315)
  • medium · critique · Keep benchmark retrieval offline and deterministic — The mock intentionally returns live Delta, BLS, Census, and other third-party URLs, so every scenario run still performs real network requests. Those pages can change, become unava (scripts/life\-scenarios/mock\-search\.mjs:28)
  • medium · critique · Convert the module URL before deriving the fixture path — `URL.pathname` is not a filesystem path: it remains percent-encoded for paths containing spaces or non-ASCII characters, and it has platform-specific semantics on Windows. In those (scripts/life\-scenarios/mock\-search\.mjs:348)
  • medium · security · Use local pages instead of live third-party URLs — The fixture explicitly drives the scenario's `web_fetch` path against real Delta, BLS, Census, and Pew pages. This introduces uncontrolled third-party network access, making the sc (scripts/life\-scenarios/fixtures/search\-index\.json:2)
  • medium · e2e · Drive the penultimate-call tool narrowing with an end-to-end test — The new `reserve_final_write` method (line 244) and `FINAL_WRITE_INSTRUCTION` change how a capped turn behaves: the second-to-last model call narrows the tool belt to deliverable t (crates/openhuman\-core/src/agent/tinyagents/middleware/final\_call\_wrap\_up\.rs:315)

Pending checks: Rust E2E (mock backend), Build Playwright E2E Artifact, E2E (Playwright / web lane), Desktop E2E (full suite, 3 OS), Rust Feature-Gate Smoke (gates off)

Before merge

  • Address Reserve the penultimate call, not the final permitted call (crates/openhuman\-core/src/agent/tinyagents/middleware/final\_call\_wrap\_up\.rs).
  • Wait for Rust E2E (mock backend), Build Playwright E2E Artifact, E2E (Playwright / web lane), Desktop E2E (full suite, 3 OS), Rust Feature-Gate Smoke (gates off).

How this fits together

flowchart LR
  n0["harness_instruction_needles<br/>changed"]:::changed
  n1["FinalCallWrapUpMiddleware<br/>changed<br/>2 findings"]:::blocking
  n2["...ether_stay_inside_the_no_window_allowance<br/>changed"]:::changed
  n3["record_model_call"]:::impacted
  n4["vec"]:::impacted
  n5["tinyagents"]:::impacted
  n6["OpenHumanRunContext"]:::impacted
  n7["...tays_bounded_when_no_window_is_advertised"]:::impacted
  n8["...t_rewrite_a_result_that_was_never_cleared"]:::impacted
  n0 -->|calls| n4
  n1 -->|uses| n5
  n2 -->|calls| n3
  n2 -->|tests| n3
  n7 -->|calls| n4
  n7 -->|tests| n4
  n8 -->|calls| n3
  n8 -->|tests| n3
  n8 -->|calls| n4
  n8 -->|tests| n4
  n8 -->|uses| n5
  n8 -->|uses| n6
  classDef changed fill:#0d4429,stroke:#238636,color:#e6edf3
  classDef impacted fill:#161b22,stroke:#6e7681,color:#c9d1d9
  classDef flagged fill:#5a1e02,stroke:#d93f0b,color:#ffffff
  classDef blocking fill:#67060c,stroke:#f85149,color:#ffffff
Loading
Agent review details

critique

  • Conclusion: Failure
  • Scope reviewed: all assigned evidence
  • Lane summary: Reviewed 14 files; 4 findings. _The code index is behind this pull request (indexed at `8506d4c714de`), so retrieved context may be out of date._ _5 memory call(s) failed (model: cortex: v1/recall: timed out after 10s), so this review saw part of what the engine holds._
  • Evidence: crates/openhuman\-core/src/agent/tinyagents/middleware/final\_call\_wrap\_up\.rs — Reserve the penultimate call, not the final permitted call
  • Evidence: scripts/life\-scenarios/mock\-search\.mjs — Keep benchmark retrieval offline and deterministic
  • Evidence: scripts/life\-scenarios/mock\-search\.mjs — Convert the module URL before deriving the fixture path

security

  • Conclusion: Success
  • Scope reviewed: all assigned evidence
  • Lane summary: Reviewed 11 files; 1 finding. 3 files were not security-reviewed: crates/openhuman-core/src/agent/tinyagents/README.md (prose or tabular data), scripts/life-scenarios/FINDINGS.md (prose or tabular data), scripts/life-scenarios/README.md (prose or tabular data). _The code index is behind this pull request (indexed at `8506d4c714de`), so retrieved context may be out of date._ _5 memory call(s) failed (model: cortex: v1/recall: timed out after 10s), so this review saw part of what the engine holds._
  • Evidence: scripts/life\-scenarios/fixtures/search\-index\.json — Use local pages instead of live third-party URLs

tests

  • Conclusion: Success
  • Scope reviewed: all assigned evidence
  • Lane summary: Adds a penultimate call narrowing to `FinalCallWrapUpMiddleware` so a capped turn can still write its artifact, with unit tests covering the narrowing, restoration, and edge cases. The benchmark rig also gains a mock search backend to avoid 401 errors; its tests verify honest ranking and envelope handling. Both changes are well-tested and safe to merge. _The code index is behind this pull request (indexed at `8506d4c714de`), so retrieved context may be out of date._ _5 memory call(s) failed (model: cortex: v1/recall: timed out after 10s), so this review saw part of what the engine holds._

commits

  • Conclusion: Neutral
  • Scope reviewed: all assigned evidence
  • Lane summary: Nothing sensitive found in what this pull request commits.

description

  • Conclusion: Success
  • Scope reviewed: all assigned evidence
  • Lane summary: This pull request fixes two defects: (1) the harness reserves the penultimate capped call for deliverable file writing by narrowing the tool belt instead of clearing it, so capped turns can still produce artifacts; (2) the life-scenario benchmark rig now mocks the search backend to avoid advertising an unauthenticatable tool. The changes are well-tested, the diff is clean, and no new problems are introduced. _The code index is behind this pull request (indexed at `8506d4c714de`), so retrieved context may be out of date._ _5 memory call(s) failed (model: cortex: v1/recall: timed out after 10s), so this review saw part of what the engine holds._

e2e

  • Conclusion: Neutral
  • Scope reviewed: all assigned evidence
  • Lane summary: The pull request introduces a penultimate-call tool narrowing for capped turns and a mock search backend for life scenarios. The middleware behaviour is covered by unit tests but lacks end-to-end coverage; the mock search is tested in isolation as a unit test. The change is safe for merge with the understanding that the new agent-loop path is not exercised by the product's e2e suite. Waiting on end-to-end jobs: `Rust E2E (mock backend)`, `Build Playwright E2E Artifact`, `E2E (Playwright / web lane)`, `Desktop E2E (full suite, 3 OS)`, `Rust Feature-Gate Smoke (gates off)`. (1 observation(s) grouped into shared inline comments)
  • Unresolved questions/checks: Rust E2E (mock backend), Build Playwright E2E Artifact, E2E (Playwright / web lane), Desktop E2E (full suite, 3 OS), Rust Feature-Gate Smoke (gates off)
  • Evidence: crates/openhuman\-core/src/agent/tinyagents/middleware/final\_call\_wrap\_up\.rs — Drive the penultimate-call tool narrowing with an end-to-end test
Evidence and run details
  • Models: ladder/vectors, gpt-5.6-luna, deepseek-v4-flash
  • Spend: $0.058579
  • Tokens: 1223673 input · 43889 output · 99971 cached · 1140 embedding
Head State Pass summary
3a845a27aa45 changes requested 5 active finding(s), 0 resolved finding(s) (at 1790182975)

tinysweeper 0.1.0

@coderabbitai

coderabbitai Bot commented Sep 23, 2026

Copy link
Copy Markdown
Contributor

Review in Change Stack →

Navigate logical layers of code changes, visualize relationships, and explore their blast radius.

Note

Currently processing new changes in this PR. This may take a few minutes, please wait...

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Advanced

Run ID: 947f3cb4-5862-4f62-b520-365119f76c10

📥 Commits

Reviewing files that changed from the base of the PR and between 5b7c987 and 3a845a2.

📒 Files selected for processing (14)
  • crates/openhuman-core/src/agent/session_host/turn_checkpoint.rs
  • crates/openhuman-core/src/agent/tinyagents/README.md
  • crates/openhuman-core/src/agent/tinyagents/harness_context_ladder.rs
  • crates/openhuman-core/src/agent/tinyagents/middleware/final_call_wrap_up.rs
  • crates/openhuman-core/src/agent/tinyagents/middleware_tests.rs
  • crates/openhuman-core/src/agent/tinyagents/middleware_wrap_up_final_write_tests.rs
  • crates/openhuman-core/src/agent/tinyagents/middleware_wrap_up_toc_budget_tests.rs
  • crates/openhuman-core/src/agent/tinyagents/middleware_wrap_up_toc_tests.rs
  • scripts/__tests__/life-scenarios-mock-search.test.mjs
  • scripts/life-scenarios/FINDINGS.md
  • scripts/life-scenarios/README.md
  • scripts/life-scenarios/fixtures/search-index.json
  • scripts/life-scenarios/mock-search.mjs
  • scripts/life-scenarios/run.mjs
 _________________________________________
< Code review: now with 100% more rabbit. >
 -----------------------------------------
  \
   \   \
        \ /\
        ( )
      .( o ).

Comment @coderabbitai help to get the list of available commands.

@senamakel
senamakel merged commit cbda223 into tinyhumansai:main Sep 23, 2026
24 of 28 checks passed

@tinysweeper tinysweeper Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Requesting changes: 1 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.0586 · 1,223,673 in / 43,889 out · 99,971 cached (8%) · ladder/vectors, gpt-5.6-luna, deepseek-v4-flash · 1,140 embedded
critique:    $0.0368 · 715,883 in   / 21,438 out · 45,153 cached (6%) · gpt-5.6-luna, deepseek-v4-flash
security:    $0.0190 · 362,412 in   / 8,900 out  · 21,538 cached (6%) · gpt-5.6-luna
tests:       $0.0008 · 39,459 in    / 3,669 out  · 2,048 cached (5%)  · deepseek-v4-flash
description: $0.0007 · 31,891 in    / 2,685 out  · 1,024 cached (3%)  · deepseek-v4-flash
e2e:         $0.0009 · 43,779 in    / 3,724 out  · 0 cached (0%)      · deepseek-v4-flash

Comment on lines +315 to +318
if remaining == 1 {
self.reserve_final_write(ctx, request);
return Ok(());
}

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

priority high critique confident

Reserve the penultimate call, not the final permitted call

When max_model_calls is 3, the first two model calls leave remaining == 1 before the third call. This branch narrows the third and final permitted call to deliverable tools, while the conclusion branch only runs when remaining == 0, after the model-call budget has already been exhausted. A tool call on the reserved request therefore leaves no permitted model call for the conclusion, defeating the stated two-phase write-then-conclude behavior. Trigger reservation when two model calls remain; the existing max_model_calls <= 2 guard already prevents reserving on budgets too small to support both phases.


Additional e2e observation

priority medium confident

Drive the penultimate-call tool narrowing with an end-to-end test

[RULE] e2e-uncovered

The new reserve_final_write method (line 244) and FINAL_WRITE_INSTRUCTION change how a capped turn behaves: the second-to-last model call narrows the tool belt to deliverable tools instead of clearing it. This path is exercised only by Rust unit tests (middleware_wrap_up_final_write_tests.rs). No end-to-end test runs a turn to the cap and asserts the artifact is written. Add an e2e test that configures a low iteration cap, includes a file_write tool, and verifies the output file is created and the final call is a text conclusion.

Suggested change for the opening observation

Suggested change
if remaining == 1 {
self.reserve_final_write(ctx, request);
return Ok(());
}
if remaining == 2 {
self.reserve_final_write(ctx, request);
return Ok(());
}

[RULE] off-by-one-budget ·

*
* ## What is mocked, and what deliberately is not
*
* **Discovery is mocked. Retrieval is not.** This server ranks a fixture

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

priority medium critique confident

Keep benchmark retrieval offline and deterministic

The mock intentionally returns live Delta, BLS, Census, and other third-party URLs, so every scenario run still performs real network requests. Those pages can change, become unavailable, redirect, rate-limit the runner, or fail in offline/CI environments, making benchmark results nondeterministic and violating the repository rule that tests must not use real network access or third-party services. Serve the fixture page contents from the local mock (or use local fixture URLs) instead of returning external URLs.

[RULE] real-network-in-test ·

Comment on lines +348 to +352
export const DEFAULT_INDEX_PATH = path.join(
path.dirname(new URL(import.meta.url).pathname),
"fixtures",
"search-index.json",
);

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

priority medium critique confident

Convert the module URL before deriving the fixture path

URL.pathname is not a filesystem path: it remains percent-encoded for paths containing spaces or non-ASCII characters, and it has platform-specific semantics on Windows. In those environments the default fixture path can point at a nonexistent file, causing the mock to serve no search results. Use fileURLToPath(import.meta.url) before passing the value to path.dirname.

Suggested change
export const DEFAULT_INDEX_PATH = path.join(
path.dirname(new URL(import.meta.url).pathname),
"fixtures",
"search-index.json",
);
export const DEFAULT_INDEX_PATH = path.join(
path.dirname(fileURLToPath(import.meta.url)),
"fixtures",
"search-index.json",
);

[RULE] portable-path-resolution ·

@@ -0,0 +1,189 @@
{
"_comment": [

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

priority medium security confident

Use local pages instead of live third-party URLs

The fixture explicitly drives the scenario's web_fetch path against real Delta, BLS, Census, and Pew pages. This introduces uncontrolled third-party network access, making the scenario dependent on external availability and content and violating the repository rule that tests must not call real backend or third-party services. Point these entries at a local mock server or checked-in fixture pages while preserving the intended retrieval behavior.

[RULE] no-real-network-in-tests ·

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

priority: p1 Next. Wrong behaviour a user will hit, or a security weakness behind a condition.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant