Repository navigation
fix(prompt): append the tool-policy boundary instead of prepending it (#5704) - #5821
Conversation
…tinyhumansai#5704) `build_system_prompt` put the `## Tool Policy Boundary` block in front of the whole assembled prompt. Every line of that block is session-scoped — agent id, channel, entry point, risk level, allowed permission, the allowed-tool list, a restricted count — so the prompt's first diverging byte sat at offset 0 and the inference backend's automatic prefix cache had nothing to reuse behind it, even when the safety preamble, tool catalogue and workspace sections were byte-identical. That contradicts what the surrounding code already protects: `SystemPromptBuilder::for_subagent` keeps `DateTimeSection` out precisely to avoid one varying value inside the prompt, and the connected-server overview is sorted so it "does not reshuffle between turns and cost its cached prefix". It also restores the archetype/persona as the prompt's opening line. The prepend replaced it with the same constant heading for every agent, which is a silent layout change for anything that identified an agent that way. The composition moved into a pure `append_tool_policy_boundary(prompt, boundary)` so the ordering is testable without standing up a session. Four tests, three of which fail against the prepend: - the boundary lands after the prompt body - the persona stays the opening line - two agents differing only in the boundary share the whole body as a common leading prefix (this is the cache property itself, not a proxy) - no boundary leaves the prompt untouched (passes either way) `cargo test --lib turn::context` 4 passed; reverting the helper to prepend gives 1 passed / 3 failed. `cargo fmt --all` clean.
![tinysweeper[bot]](https://avatars.githubusercontent.com/in/4526585?s=60&v=4){kind=small}
How this change flows0 changed behaviours across 4 relationships. 6 surrounding behaviours are shown (60 graph nodes walked). 38 further behaviours left out to keep the diagram readable. flowchart LR
n0["fetch_learned_context"]:::impacted
n1["build_system_prompt"]:::impacted
n2["bound_cached_transcript_messages"]:::impacted
n3["trim_history"]:::impacted
n4["extend"]:::impacted
n5["openhuman"]:::impacted
n0 -->|uses| n5
n1 -->|uses| n5
n2 -->|calls| n4
n3 -->|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
|
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 (1)
Included review availability: Your plan provides up to 10 included reviews per hour; 9 remain after this review. 📝 WalkthroughWalkthroughThe system prompt builder now appends the optional tool-policy boundary after the assembled prompt. A helper handles absent boundaries. Tests verify ordering, persona placement, shared prompt prefixes, and unchanged output without a boundary. ChangesSystem prompt boundary placement
Estimated code review effort: 2 (Simple) | ~10 minutes Merge Risk: ⚪ Minimal · up to This localized change appends the session-specific tool-policy boundary so stable prompt content can retain a shared prefix while preserving the same policy information. No actionable merge-blocking risk remains beyond normal checks and review. Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
Full details: Linked Issues checkExplanation The changes satisfy issue Warning Your free Security trial is over. An organization admin can activate Security or dismiss this notice. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
|
Marking this as the fix we're taking for #5704. #5759 and #5752 have been closed in its favour. The deciding factors: this is Two items from #5752 (@gac0812) are not covered here, and I've asked for both to be re-filed separately so they aren't lost with that PR:
Nothing needed on this PR — flagging so the cluster resolution is on the record. |
|
Maintainer review pass — no changes needed, this one is ready from my side. Verified at head
Extracting Nothing for @ntdatt812 to do here. Flagging one thing for whoever merges, following up on the note above: the two items carried over from #5752 (the |
M3gA-Mind
left a comment
M3gA-Mind
left a comment
There was a problem hiding this comment.
Approved after a maintainer-side verification pass.
Verified on the current head: MERGEABLE against main, zero failing and zero pending required checks, and no unresolved, non-outdated review threads.
This is one of two required approvals; a second maintainer review is still needed before merge.
…ects, privacy sheet An audit of six PRs merged in the last week found none with e2e coverage of what they changed. Three shipped tests their authors reasonably believed were sufficient: tinyhumansai#5799 added 62 lines to `tests/json_rpc_e2e.rs` that are a determinism fix for a different test; tinyhumansai#5821's four unit tests exercise an extracted pure helper and would pass if `build_system_prompt` stopped calling it; tinyhumansai#5939's two tests are vitest, not an e2e lane. Added, each driving the changed path and asserting the changed behaviour: - tinyhumansai#5799 `json_rpc_migrate_hermes_refuses_null_driver_without_naming_openclaw`. Drives the Hermes migration RPC into a configured null driver and asserts the refusal, that it no longer hard-codes "OpenClaw" in a message both migrations raise, and that the source workspace really is byte-identical afterwards. The third arm tinyhumansai#5799 added is unreachable with modules on and stays with its gates-off unit test; the doc comment says so rather than faking it. - tinyhumansai#5821 two tests on the real `Agent::build_system_prompt`, asserting the tool-policy boundary does not open the prompt and that it is the prompt's final block — the property a prefix cache keys on, and the one a revert to prepending destroys. - tinyhumansai#5939 two cases on `/webhooks`, asserting the query and the fragment survive BOTH redirect hops. They assert the final destination, so a fix to only the first hop still fails them. - tinyhumansai#5845 a new spec opening the privacy sheet, which no e2e had ever done. Also flips `BUG: /skills?tab=channels drops the tab`, which pinned pre-fix behaviour with the note "Flip the two assertions below when that lands". It landed in d434f1e (tinyhumansai#5924), four hours before that spec was last touched, so the test asserts the opposite of shipped behaviour.
…ects, privacy sheet An audit of six PRs merged in the last week found none with e2e coverage of what they changed. Three shipped tests their authors reasonably believed were sufficient: tinyhumansai#5799 added 62 lines to `tests/json_rpc_e2e.rs` that are a determinism fix for a different test; tinyhumansai#5821's four unit tests exercise an extracted pure helper and would pass if `build_system_prompt` stopped calling it; tinyhumansai#5939's two tests are vitest, not an e2e lane. Added, each driving the changed path and asserting the changed behaviour: - tinyhumansai#5799 `json_rpc_migrate_hermes_refuses_null_driver_without_naming_openclaw`. Drives the Hermes migration RPC into a configured null driver and asserts the refusal, that it no longer hard-codes "OpenClaw" in a message both migrations raise, and that the source workspace really is byte-identical afterwards. The third arm tinyhumansai#5799 added is unreachable with modules on and stays with its gates-off unit test; the doc comment says so rather than faking it. - tinyhumansai#5821 two tests on the real `Agent::build_system_prompt`, asserting the tool-policy boundary does not open the prompt and that it is the prompt's final block — the property a prefix cache keys on, and the one a revert to prepending destroys. - tinyhumansai#5939 two cases on `/webhooks`, asserting the query and the fragment survive BOTH redirect hops. They assert the final destination, so a fix to only the first hop still fails them. - tinyhumansai#5845 a new spec opening the privacy sheet, which no e2e had ever done. Also flips `BUG: /skills?tab=channels drops the tab`, which pinned pre-fix behaviour with the note "Flip the two assertions below when that lands". It landed in d434f1e (tinyhumansai#5924), four hours before that spec was last touched, so the test asserts the opposite of shipped behaviour.
…ects, privacy sheet An audit of six PRs merged in the last week found none with e2e coverage of what they changed. Three shipped tests their authors reasonably believed were sufficient: tinyhumansai#5799 added 62 lines to `tests/json_rpc_e2e.rs` that are a determinism fix for a different test; tinyhumansai#5821's four unit tests exercise an extracted pure helper and would pass if `build_system_prompt` stopped calling it; tinyhumansai#5939's two tests are vitest, not an e2e lane. Added, each driving the changed path and asserting the changed behaviour: - tinyhumansai#5799 `json_rpc_migrate_hermes_refuses_null_driver_without_naming_openclaw`. Drives the Hermes migration RPC into a configured null driver and asserts the refusal, that it no longer hard-codes "OpenClaw" in a message both migrations raise, and that the source workspace really is byte-identical afterwards. The third arm tinyhumansai#5799 added is unreachable with modules on and stays with its gates-off unit test; the doc comment says so rather than faking it. - tinyhumansai#5821 two tests on the real `Agent::build_system_prompt`, asserting the tool-policy boundary does not open the prompt and that it is the prompt's final block — the property a prefix cache keys on, and the one a revert to prepending destroys. - tinyhumansai#5939 two cases on `/webhooks`, asserting the query and the fragment survive BOTH redirect hops. They assert the final destination, so a fix to only the first hop still fails them. - tinyhumansai#5845 a new spec opening the privacy sheet, which no e2e had ever done. Also flips `BUG: /skills?tab=channels drops the tab`, which pinned pre-fix behaviour with the note "Flip the two assertions below when that lands". It landed in d434f1e (tinyhumansai#5924), four hours before that spec was last touched, so the test asserts the opposite of shipped behaviour.
…icy-boundary-placement\n\nfix(prompt): append the tool-policy boundary instead of prepending it (tinyhumansai#5704)\n
…ects, privacy sheet\n\nAn audit of six PRs merged in the last week found none with e2e coverage of\nwhat they changed. Three shipped tests their authors reasonably believed were\nsufficient: tinyhumansai#5799 added 62 lines to `tests/json_rpc_e2e.rs` that are a\ndeterminism fix for a different test; tinyhumansai#5821's four unit tests exercise an\nextracted pure helper and would pass if `build_system_prompt` stopped calling\nit; tinyhumansai#5939's two tests are vitest, not an e2e lane.\n\nAdded, each driving the changed path and asserting the changed behaviour:\n\n- tinyhumansai#5799 `json_rpc_migrate_hermes_refuses_null_driver_without_naming_openclaw`.\n Drives the Hermes migration RPC into a configured null driver and asserts the\n refusal, that it no longer hard-codes "OpenClaw" in a message both migrations\n raise, and that the source workspace really is byte-identical afterwards.\n The third arm tinyhumansai#5799 added is unreachable with modules on and stays with its\n gates-off unit test; the doc comment says so rather than faking it.\n\n- tinyhumansai#5821 two tests on the real `Agent::build_system_prompt`, asserting the\n tool-policy boundary does not open the prompt and that it is the prompt's\n final block — the property a prefix cache keys on, and the one a revert to\n prepending destroys.\n\n- tinyhumansai#5939 two cases on `/webhooks`, asserting the query and the fragment survive\n BOTH redirect hops. They assert the final destination, so a fix to only the\n first hop still fails them.\n\n- tinyhumansai#5845 a new spec opening the privacy sheet, which no e2e had ever done.\n\nAlso flips `BUG: /skills?tab=channels drops the tab`, which pinned pre-fix\nbehaviour with the note "Flip the two assertions below when that lands". It\nlanded in 786d7ba (tinyhumansai#5924), four hours before that spec was last touched, so\nthe test asserts the opposite of shipped behaviour.\n
Closes #5704.
build_system_promptput## Tool Policy Boundaryin front of the whole assembled prompt:Every line of that block is session-scoped — agent id, channel, entry point, risk level, allowed permission, the allowed-tool list, a restricted count. Putting it first moves the prompt's first diverging byte to offset 0, so two turns share no cached prefix at all even when the safety preamble, tool catalogue and workspace sections behind it are byte-identical. It renders whenever anything is blocked or hidden, which is the normal state for any scoped toolbelt.
This is the same concern the surrounding code already goes out of its way to protect:
SystemPromptBuilder::for_subagentdeliberately excludesDateTimeSection— "Repeat spawns of the same sub-agent definition must produce byte-identical system prompts so the inference backend's automatic prefix cache can reuse the prefill".connections::connected_overviewis sorted "so a prompt built from this does not reshuffle between turns and cost its cached prefix".One varying value was worth excluding; seven varying values ahead of everything were not caught.
Appending loses nothing — the model reads the whole system message either way — and it restores the archetype/persona as the opening line, which the prepend had replaced with the same constant heading for every agent.
Testability
The placement lived inside a session method, so I pulled the composition into a pure helper:
Everything that decides the ordering is now in those two arguments, so it can be tested without standing up a session.
Tests
Four, three of which fail against the prepend (verified by reverting the helper:
1 passed; 3 failed):The third asserts the cache property directly rather than through a proxy — it walks the two prompts byte-by-byte and requires the shared prefix to cover the entire stable body. That is the thing the issue is actually about, so I wanted the test to measure it rather than to assert an ordering that merely implies it.
The fourth is included deliberately and passes either way; it keeps the set honest about what this change does not alter.
Verification
cargo test --lib turn::context— 4 passed.cargo fmt --all— clean.Summary by CodeRabbit