fix(prompts): stop showing example ids the model cannot legitimately send - #5320
fix(prompts): stop showing example ids the model cannot legitimately send#5320yh928 wants to merge 2 commits into
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (7)
🚧 Files skipped from review as they are similar to previous changes (7)
📝 WalkthroughWalkthroughTool schemas now require runtime-issued result handles, memory source IDs, goal IDs, and browser references instead of fabricated example values. ChangesRuntime identifier documentation
Estimated code review effort: 1 (Trivial) | ~5 minutes Possibly related PRs
Suggested labels: Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
coderabbitai
Bot
added
bug
rust-core
|
| Filename | Overview |
|---|---|
| src/openhuman/agent/harness/subagent_runner/extract_tool.rs | Removes the res_1 example from the result_id parameter description; adds a code comment recording two live production misuse runs to prevent the example from being re-introduced. |
| src/openhuman/memory/goals/schemas.rs | Replaces "(e.g. 'g1')" with "taken from a goals list you have read" for edit and delete id fields; the sequential-ID risk warning present in tools.rs is not repeated here. |
| src/openhuman/memory/goals/tools.rs | Replaces "(e.g. 'g1')" with instructional text plus an explicit "Ids are sequential, so a guessed one lands/deletes a real goal you never looked at" warning for both edit and delete. |
| src/openhuman/memory/query/cover_window.rs | Replaces the slack:#eng / gmail:abc example with "copied from tree.list_sources" for the source_id field. |
| src/openhuman/memory/query/query_source.rs | Same source_id description fix as cover_window.rs — example replaced with "copied from tree.list_sources". |
| src/openhuman/memory/tree/retrieval/schemas.rs | Two source_id FieldSchema comment strings updated — both drop the slack:#eng / gmail:abc examples in favour of "copied from tree.list_sources". |
| src/openhuman/tools/impl/browser/browser.rs | Changes @ref (e.g. @e1) to "@ref copied from the latest page snapshot" in the selector description, preventing the model from guessing stale element references across navigations. |
Sequence Diagram
sequenceDiagram
participant M as Model
participant S as Schema description
participant T as Tool runtime
participant C as Cache / Store
Note over M,C: Before this PR
S-->>M: result_id handle example res_1
M->>T: extract_from_result result_id res_1
T->>C: lookup res_1
C-->>T: cache-miss
T-->>M: Error
Note over M,C: After this PR
S-->>M: Copy handle from earlier result
M->>T: some_tool returns placeholder result_id res_abc42
M->>T: extract_from_result result_id res_abc42
T->>C: lookup res_abc42
C-->>T: hit
T-->>M: extracted answer
Reviews (2): Last reviewed commit: "fix(prompts): stop showing example ids t..." | Re-trigger Greptile
![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/agent/harness/subagent_runner/extract_tool.rs`:
- Line 192: Update the description of the oversized tool output placeholder
handle to state that emitted handles may expire after cache eviction; when a
handle is no longer valid, rerun the original tool to obtain a new result
instead of sending the expired handle.
- Around line 170-183: Add focused semantic regression tests for the
runtime-identifier model-facing descriptions: in
src/openhuman/agent/harness/subagent_runner/extract_tool.rs:170-183 verify exact
placeholder-handle copying and no fabricated handles, and at 192 verify no-stash
guidance; test source_id guidance in cover_window at
src/openhuman/memory/query/cover_window.rs:43 and query_source at
src/openhuman/memory/query/query_source.rs:31; test the query_source and
cover_window controller schemas in
src/openhuman/memory_tree/retrieval/schemas.rs:106 and 168; test edit and delete
goal-ID guidance in src/openhuman/memory_goals/schemas.rs:94 and 118; test
GoalsEditTool and GoalsDeleteTool in src/openhuman/memory_goals/tools.rs:133 and
185; and test snapshot references plus CSS/text selector support in
src/openhuman/tools/impl/browser/browser.rs:773.
🪄 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 Plus
Run ID: 4f6e103a-c320-4966-9c52-24c5a1e06a35
📒 Files selected for processing (7)
src/openhuman/agent/harness/subagent_runner/extract_tool.rssrc/openhuman/memory/query/cover_window.rssrc/openhuman/memory/query/query_source.rssrc/openhuman/memory_goals/schemas.rssrc/openhuman/memory_goals/tools.rssrc/openhuman/memory_tree/retrieval/schemas.rssrc/openhuman/tools/impl/browser/browser.rs
…send Six tool schemas illustrated an id-shaped argument with a sample value. A sample is the shape a model reproduces, so it sent the sample. `extract_from_result` showed `result_id="res_1"`. Two live runs called it with `res_1` against a cache that had never issued a handle, and both got a cache-miss instead of the data they wanted. A handle is minted at runtime and only exists once a result was actually stashed, so no example of one can ever be valid — the fix is to show none and say where the real one comes from. The same shape appeared in five more places: - `memory_goals` edit/delete — `(e.g. 'g1')`. Goal ids are sequential, so a guessed one lands on, or deletes, a real goal the model never looked at. - `memory` query source + cover window, and the memory-tree retrieval schema — `(e.g. `slack:#eng`, `gmail:abc`)`. Source ids are provider-specific strings only `tree.list_sources` can tell you. - the browser element selector — `@ref (e.g. @e1)`. Refs come from the latest page snapshot. Each now names the call that issues the id instead of inventing one. No prohibition rules and no parser tolerance were added: the fix is what the schema shows. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01SRSNnqQsokuGmkbpLoLCGy
ef5be20 to
352f770
Compare
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
…tract Two halves of one rule. The text says a handle is only valid if it appeared in an earlier result, but not that one can stop being valid — the cache holds a bounded number of recent entries, and `execute` already answers an evicted handle with "re-run the original tool". Unsaid, an eviction reads to the model as "wrong handle", which invites exactly the guess this PR removed the sample handle to stop. Now stated where the model reads it. Adds the regression the descriptions had none of: no model-facing text may contain a handle-shaped literal (`res_1` and friends), and `result_id` must still say where a real handle comes from and that it expires. A description is a prompt, and an example in it is an instruction — the removal has to be pinned or the next edit helpfully adds the example back. Two live runs called this tool with `res_1` against a cache that had never issued one. `description` and `parameters_schema` are lifted to free functions so the test can read them without a live cache and model source; the trait methods delegate. extract_tool 5 pass. Reported by CodeRabbit on tinyhumansai#5320. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01SRSNnqQsokuGmkbpLoLCGy
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
There was a problem hiding this comment.
yh928 has reached the 50-credit limit for trial accounts. To continue receiving code reviews, upgrade your plan.
|
Everything else on this head is green, and both review threads are addressed in |
…tract Two halves of one rule. The text says a handle is only valid if it appeared in an earlier result, but not that one can stop being valid — the cache holds a bounded number of recent entries, and `execute` already answers an evicted handle with "re-run the original tool". Unsaid, an eviction reads to the model as "wrong handle", which invites exactly the guess this PR removed the sample handle to stop. Now stated where the model reads it. Adds the regression the descriptions had none of: no model-facing text may contain a handle-shaped literal (`res_1` and friends), and `result_id` must still say where a real handle comes from and that it expires. A description is a prompt, and an example in it is an instruction — the removal has to be pinned or the next edit helpfully adds the example back. Two live runs called this tool with `res_1` against a cache that had never issued one. `description` and `parameters_schema` are lifted to free functions so the test can read them without a live cache and model source; the trait methods delegate. extract_tool 5 pass. Reported by CodeRabbit on tinyhumansai#5320. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01SRSNnqQsokuGmkbpLoLCGy
e4df37c to
2a4f0ae
Compare
There was a problem hiding this comment.
yh928 has reached the 50-credit limit for trial accounts. To continue receiving code reviews, upgrade your plan.
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
|
Correction to my earlier comment: the link check has since passed on this head, so no re-run is needed. It was a transient timeout against external hosts (the failing run reported 0 errors and 10 timeouts), and the branch touches no markdown. |
1 participant
Summary
extract_from_resulthandle loses its example entirely: no example of a runtime-minted handle can ever be valid.Problem
A sample is the shape a model reproduces, so the model sends the sample.
extract_from_resultdocumentedresult_idasres_1. Two live runs called it with the literalres_1against a cache that had never issued a handle, and both got a cache-miss instead of the data they wanted.The same shape appeared in
memory_goalsedit/delete ((e.g. 'g1')— ids are sequential, so a guess deletes a real goal), the memory query source / cover window / memory-tree retrieval schemas ((e.g. \slack:#eng`)— source ids onlytree.list_sourcescan produce), and the browser element selector (@ref (e.g. @e1)` — refs change every navigation).Solution
Each description now names where the real id comes from. No prohibition rules and no parser tolerance were added — the fix is what the schema shows. A rule saying "do not guess" competes with an example that shows a guessable value, and the example wins; widening the parser or the cache to accept a fabricated id would turn a wasted turn into a wrong result.
The
extract_from_resultcase carries a code comment recording the two live runs, so the example is not reintroduced as a readability improvement.Submission Checklist
Closes #NNNin the## RelatedsectionImpact
Related
Closes #5316
Summary by CodeRabbit