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
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Team Run ID: 📒 Files selected for processing (8)
🚧 Files skipped from review as they are similar to previous changes (5)
Included review availability: Your plan provides up to 10 included reviews per hour; 7 remain after this review. 📝 WalkthroughWalkthroughTool schemas now direct callers to copy runtime-issued identifiers from prior results, source listings, goal lists, or page snapshots. Fabricated identifier examples were removed, and extraction-tool tests verify the updated documentation. ChangesRuntime identifier documentation
Estimated code review effort: 2 (Simple) | ~10 minutes Merge Risk: ⚪ Minimal · up to This change replaces misleading example identifiers with guidance to obtain real runtime-issued values, without changing runtime behavior; no actionable merge-blocking risk remains beyond normal checks and review. Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Full details: Linked Issues checkExplanation The pull request replaces fabricated examples with references to valid runtime ID sources across extract handles, goal IDs, memory source IDs, and browser references. The added tests verify that fabricated handle examples are absent.
Warning Your free Security trial is over. An organization admin can upgrade to Advanced for continuous pull request security review or dismiss this notice. 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
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. |
|
Maintainer review (review only — no changes pushed to your branch). The change is right and I would like to see it land. The argument in the description is the correct one: a schema description is a prompt, an example id in it is an instruction, and adding a "do not guess" rule alongside a guessable sample loses to the sample. Not widening the parser or the cache to accept a fabricated id is also the right call — that converts a wasted turn into a wrong result. The It is 1. The string is identical there, so it is a straight re-target, no re-wording needed. 2. #[cfg(test)]
#[path = "extract_tool_tests.rs"]
mod tests;and move only your two new tests — Everything else in the PR — After the rebase the net diff is 8 files, |
…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
…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
2a4f0ae to
6a8918a
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. |
|
Rebased onto The three moves:
Everything else ( One thing worth noting about the test move. The auto-merge silently resolved the test module to Verified locally: The substance is unchanged, and the |
![tinysweeper[bot]](https://avatars.githubusercontent.com/in/4526585?s=60&v=4){kind=small}
There was a problem hiding this comment.
tinysweeper found nothing blocking. Approving.
$0.0091 · 119,479 in / 934 out · 23,552 cached (20%) · openrouter/openai/text-embedding-3-small, deepseek/deepseek-v4-flash · 429 embedded
critique: $0.0036 · 58,591 in / 329 out · 23,552 cached (40%) · deepseek/deepseek-v4-flash
security: $0.0042 · 46,716 in / 300 out · 0 cached (0%) · deepseek/deepseek-v4-flash
tests: $0.0013 · 14,172 in / 305 out · 0 cached (0%) · deepseek/deepseek-v4-flash
How this change flows0 changed behaviours across 5 relationships. 6 surrounding behaviours are shown (60 graph nodes walked). 34 further behaviours left out to keep the diagram readable. flowchart LR
n0["Value"]:::impacted
n1["ControllerFuture"]:::impacted
n2["load_config_with_timeout"]:::impacted
n3["to_json"]:::impacted
n4["current_goals_guard"]:::impacted
n5["handle_reflect"]:::impacted
n3 -->|uses| n0
n5 -->|uses| n0
n5 -->|uses| n1
n5 -->|calls| n2
n5 -->|calls| n4
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
Green: changed behaviour. Grey: surrounding behaviour. Arrows name the call, use, implementation, or test relationship. Orange: has findings. Red: has a finding that blocks the merge. |
tinysweeper
Bot
added
the
priority: p3
coderabbitai
Bot
removed
the
rust-core
2 participants
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
Documentation
Tests