fix(pformat): tag each argument with its slot number instead of counting empty slots - #5326
fix(pformat): tag each argument with its slot number instead of counting empty slots#5326yh928 wants to merge 3 commits into
Conversation
📝 WalkthroughWalkthroughP-Format tool calls now use explicit ChangesIndexed P-Format arguments
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant Model
participant Dispatcher
participant parse_call
participant PFormatToolParams
Model->>Dispatcher: indexed P-Format tool call
Dispatcher->>parse_call: tokenize index/value pairs
parse_call->>PFormatToolParams: resolve indexed parameters and types
PFormatToolParams-->>parse_call: ordered parameter definitions
parse_call-->>Dispatcher: parsed named arguments
Dispatcher-->>Model: tool-call result
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
agent
![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: 58135af16f
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/openhuman/agent/pformat.rs (1)
419-425: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider splitting the growing
mod testsinto a separate file.This file now extends past 768 lines including its inline test module, exceeding the guideline to "Prefer Rust modules of approximately 500 lines or fewer and maintain small, single-responsibility Unix-style modules." The sibling
dispatcher_tests.rsalready establishes the pattern of externalizing tests from their production module in this directory. Movingmod testshere into apformat_tests.rs(or similar) would bringpformat.rsback under the guideline without touching test content.🤖 Prompt for 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. In `@src/openhuman/agent/pformat.rs` around lines 419 - 425, The inline mod tests in pformat.rs has grown the production module beyond the preferred size. Move the entire tests module, including make_registry and all existing test content, into a sibling external test file such as pformat_tests.rs; declare or include that test module from pformat.rs using the established dispatcher_tests.rs pattern, without changing test behavior.Source: Coding guidelines
🤖 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/pformat.rs`:
- Around line 279-341: Update the parse_response call path and the rejection
tracing::debug! events in the p-format parsing flow to include an available turn
or request correlation identifier alongside tool. Thread the identifier from the
session loop’s ChatResponse context through the relevant parser functions, or
remove these rejection logs if no identifier can be provided; do not retain
uncorrelated "[pformat]" rejection events.
---
Nitpick comments:
In `@src/openhuman/agent/pformat.rs`:
- Around line 419-425: The inline mod tests in pformat.rs has grown the
production module beyond the preferred size. Move the entire tests module,
including make_registry and all existing test content, into a sibling external
test file such as pformat_tests.rs; declare or include that test module from
pformat.rs using the established dispatcher_tests.rs pattern, without changing
test behavior.
🪄 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: d62f8755-b921-43ff-996f-7dee983893d4
📒 Files selected for processing (4)
src/openhuman/agent/dispatcher.rssrc/openhuman/agent/dispatcher_tests.rssrc/openhuman/agent/pformat.rssrc/openhuman/tinyagents/model.rs
|
| Filename | Overview |
|---|---|
| src/openhuman/agent/pformat.rs | Core parsing module rewritten: slot-indexed format replaces bare-positional, parse_call now rejects odd token counts, non-numeric indices, and out-of-range slots; from_schema reordered to required-first then optional alphabetically; render_signature generates numbered placeholders |
| src/openhuman/agent/dispatcher.rs | System-prompt instructions updated to teach the model the new indexed call form; example call and rules text updated, empty-slot rule removed |
| src/openhuman/agent/dispatcher_tests.rs | Existing dispatcher tests updated to new name[index |
| src/openhuman/agent/harness/subagent_runner/tool_prep.rs | Subagent tool instructions updated to the new indexed format, mirroring the main dispatcher changes |
| src/openhuman/agent/prompts/render_helpers.rs | Subagent system prompt updated with new indexed call example and description; safe mechanical update |
| src/openhuman/agent/prompts/types.rs | Single doc-comment update to PFormat variant description; trivial |
| src/openhuman/tinyagents/model.rs | One test updated to use new indexed form for the legacy p-format fallback path; no logic change |
Flowchart
%%{init: {'theme': 'neutral'}}%%
flowchart TD
A["Model emits name[body]"] --> B[parse_call]
B --> C{Tool in registry?}
C -- No --> Z1[return None]
C -- Yes --> D[split_pipes body - honours escapes]
D --> E{token count even?}
E -- No --> Z2[return None - odd token count]
E -- Yes --> F[chunks_exact 2 - index and value pairs]
F --> G{raw_index parses as usize?}
G -- No --> Z3[return None - non-numeric or bare-positional]
G -- Yes --> H{slot within params.len?}
H -- No --> Z4[return None - out-of-range slot]
H -- Yes --> I{value.trim is empty?}
I -- Yes --> J[skip - argument omitted]
I -- No --> K[coerce_value per schema type]
K --> L[insert into args map - last-write-wins]
J --> M{more pairs?}
L --> M
M -- Yes --> F
M -- No --> N[return Some - name and args]
Reviews (2): Last reviewed commit: "fix(pformat): teach the sub-agent prompt..." | Re-trigger Greptile
…arser reads
The parser now requires a slot number before each value, and returns `None` for
a call that does not carry one. The main dispatcher's protocol block was updated
with it; two sub-agent paths were not, so a sub-agent was told to write the old
grammar and every tool call it made was dropped.
- `subagent_runner/tool_prep.rs` taught `name[arg1|arg2|...|argN]` with
"leave a slot empty to omit that argument" — the empty-slot counting this
change exists to remove.
- `render_helpers.rs`'s sub-agent protocol block taught `tool_name[arg1|arg2]`
and "match the order shown ... (alphabetical by parameter name)".
Both now carry the same two rules the main block does: a `Call as:` signature
numbers its slots, and you send only the arguments you are passing, each with
its number. The worked example is the same one (`get_weather[0|London|1|metric]`)
so the three blocks cannot drift into describing different grammars.
Also drops the two references the prompts had no business making — a section
heading by name (`## Tools`) and a relative position ("signature above") — since
neither survives a reordered or filtered prompt.
Reported by codex on tinyhumansai#5326 (P1).
coderabbitai
Bot
added
the
rust-core
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/tool_prep.rs`:
- Around line 90-101: Use “indexed” consistently instead of “positional” in the
sub-agent protocol wording. Update
src/openhuman/agent/harness/subagent_runner/tool_prep.rs lines 90-101 and
src/openhuman/agent/prompts/render_helpers.rs lines 437-440, while preserving
the existing indexed P-Format examples and rules.
In `@src/openhuman/agent/prompts/render_helpers.rs`:
- Around line 437-440: Update the sub-agent prompt text near the tool-call
example to document the P-Format escape for representing a literal backslash,
alongside the existing \| and \] guidance. Keep this block consistent with the
main text-mode instructions without changing the surrounding
argument-substitution behavior.
🪄 Autofix
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: 16db9bab-3eba-4273-aefb-9b520d1f7a31
📒 Files selected for processing (3)
src/openhuman/agent/harness/subagent_runner/tool_prep.rssrc/openhuman/agent/prompts/render_helpers.rssrc/openhuman/agent/prompts/types.rs
…ing empty slots
P-Format was bare positional, and a skipped argument was written as an empty
slot (`name[||value]`). That made the *count of leading delimiters*
load-bearing, and it is the single thing models get wrong most:
- `GMAIL_LIST_THREADS[||50|<query>]` failed schema validation 12 times in one
turn before the turn was cut short.
- A live `GMAIL_LIST_THREADS` wrote four leading empties where three were
needed, so `query` and `user_id` each landed one slot late, in `user_id` and
`verbose`. The call RAN, searching Gmail with the account id set to the
search text.
Both are off-by-one on a delimiter, and both bound arguments to the wrong
parameter silently. The signature now numbers its slots and a call tags each
value with the slot it fills:
signature get_weather[0|<location>|1|<unit>]
call get_weather[0|London|1|metric]
sparse get_weather[1|metric]
Only the arguments being sent appear, so there is nothing to miscount.
The parser refuses rather than guesses: a missing, non-numeric, or
out-of-range index, or an odd token count, returns None. The old bare
positional form is therefore rejected too — deliberately, since parsing it
positionally would resurrect the exact failure this replaces. The failure mode
moves from a wrong call that succeeds to a malformed call the model is told
about.
Required-first ordering is kept: it is why the minimal call is `name[0|value]`
rather than an arbitrary index.
Note for future edits to the typed-turn decoder: `parse_pformat_call` splits on
raw `|` WITHOUT honouring escapes, so its array is a list of fragments whose
join reconstructs the body, not a list of arguments. An escaped pipe arrives as
two elements. Tagging those fragments numbers the halves of one value; the
join must stay verbatim, and the model's own indices ride inside it. A test
covers it (`an_escaped_pipe_stays_inside_its_argument`).
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01SRSNnqQsokuGmkbpLoLCGy
…arser reads
The parser now requires a slot number before each value, and returns `None` for
a call that does not carry one. The main dispatcher's protocol block was updated
with it; two sub-agent paths were not, so a sub-agent was told to write the old
grammar and every tool call it made was dropped.
- `subagent_runner/tool_prep.rs` taught `name[arg1|arg2|...|argN]` with
"leave a slot empty to omit that argument" — the empty-slot counting this
change exists to remove.
- `render_helpers.rs`'s sub-agent protocol block taught `tool_name[arg1|arg2]`
and "match the order shown ... (alphabetical by parameter name)".
Both now carry the same two rules the main block does: a `Call as:` signature
numbers its slots, and you send only the arguments you are passing, each with
its number. The worked example is the same one (`get_weather[0|London|1|metric]`)
so the three blocks cannot drift into describing different grammars.
Also drops the two references the prompts had no business making — a section
heading by name (`## Tools`) and a relative position ("signature above") — since
neither survives a reordered or filtered prompt.
Reported by codex on tinyhumansai#5326 (P1).
12fc086 to
8929270
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. |
…arser reads
The parser now requires a slot number before each value, and returns `None` for
a call that does not carry one. The main dispatcher's protocol block was updated
with it; two sub-agent paths were not, so a sub-agent was told to write the old
grammar and every tool call it made was dropped.
- `subagent_runner/tool_prep.rs` taught `name[arg1|arg2|...|argN]` with
"leave a slot empty to omit that argument" — the empty-slot counting this
change exists to remove.
- `render_helpers.rs`'s sub-agent protocol block taught `tool_name[arg1|arg2]`
and "match the order shown ... (alphabetical by parameter name)".
Both now carry the same two rules the main block does: a `Call as:` signature
numbers its slots, and you send only the arguments you are passing, each with
its number. The worked example is the same one (`get_weather[0|London|1|metric]`)
so the three blocks cannot drift into describing different grammars.
Also drops the two references the prompts had no business making — a section
heading by name (`## Tools`) and a relative position ("signature above") — since
neither survives a reordered or filtered prompt.
Reported by codex on tinyhumansai#5326 (P1).
8929270 to
f7182df
Compare
![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.
…arser reads
The parser now requires a slot number before each value, and returns `None` for
a call that does not carry one. The main dispatcher's protocol block was updated
with it; two sub-agent paths were not, so a sub-agent was told to write the old
grammar and every tool call it made was dropped.
- `subagent_runner/tool_prep.rs` taught `name[arg1|arg2|...|argN]` with
"leave a slot empty to omit that argument" — the empty-slot counting this
change exists to remove.
- `render_helpers.rs`'s sub-agent protocol block taught `tool_name[arg1|arg2]`
and "match the order shown ... (alphabetical by parameter name)".
Both now carry the same two rules the main block does: a `Call as:` signature
numbers its slots, and you send only the arguments you are passing, each with
its number. The worked example is the same one (`get_weather[0|London|1|metric]`)
so the three blocks cannot drift into describing different grammars.
Also drops the two references the prompts had no business making — a section
heading by name (`## Tools`) and a relative position ("signature above") — since
neither survives a reordered or filtered prompt.
Reported by codex on tinyhumansai#5326 (P1).
f7182df to
d1cdbe3
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.
…jections The sub-agent block documented `\|` and `\]` but not `\\`, which `split_pipes` also decodes. That is the same defect this PR opened to fix, one notch quieter: where the slot numbering drifted and calls were dropped whole, an undocumented escape leaves a value carrying a backslash encoded on a guess. All three P-Format prompt blocks now name the same three escapes. Adds a test that asserts the sub-agent block documents every escape the parser honours, so the two cannot drift again without something failing. The rejection logs now carry a correlation field. `parse_call` is pure and takes no context, but it runs inside the turn's task, so the ambient chat thread id is readable from `thread_context` without threading an argument through a parser that has no other use for one — `<none>` outside a turn. Dropping the logs instead was the wrong trade: they are the only signal that a model's call was refused, which is exactly the failure this PR is about. agent::pformat 24, agent::prompts 72 pass. Reported by CodeRabbit on tinyhumansai#5326. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01SRSNnqQsokuGmkbpLoLCGy
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. |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/openhuman/agent/prompts/mod_tests.rs (1)
793-797: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winClean up the temporary workspace after the test.
This test creates a UUID-named directory under
std::env::temp_dir()and never removes it. Repeated test runs leave directories behind. Use a scoped temporary-directory helper or cleanup guard.🤖 Prompt for 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. In `@src/openhuman/agent/prompts/mod_tests.rs` around lines 793 - 797, Update the test around the workspace setup to use a scoped temporary-directory helper or cleanup guard, ensuring the UUID-named directory is removed when the test exits, including on failure; preserve the existing workspace path usage.
🤖 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.
Nitpick comments:
In `@src/openhuman/agent/prompts/mod_tests.rs`:
- Around line 793-797: Update the test around the workspace setup to use a
scoped temporary-directory helper or cleanup guard, ensuring the UUID-named
directory is removed when the test exits, including on failure; preserve the
existing workspace path usage.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 82bb0574-12a5-47ba-82ea-75629bcb138f
📒 Files selected for processing (8)
src/openhuman/agent/dispatcher.rssrc/openhuman/agent/dispatcher_tests.rssrc/openhuman/agent/harness/subagent_runner/tool_prep.rssrc/openhuman/agent/pformat.rssrc/openhuman/agent/prompts/mod_tests.rssrc/openhuman/agent/prompts/render_helpers.rssrc/openhuman/agent/prompts/types.rssrc/openhuman/agent/tinyagents/model.rs
🚧 Files skipped from review as they are similar to previous changes (7)
- src/openhuman/agent/prompts/types.rs
- src/openhuman/agent/tinyagents/model.rs
- src/openhuman/agent/prompts/render_helpers.rs
- src/openhuman/agent/dispatcher.rs
- src/openhuman/agent/dispatcher_tests.rs
- src/openhuman/agent/harness/subagent_runner/tool_prep.rs
- src/openhuman/agent/pformat.rs
1 participant
Summary
<name>placeholders in the rendered signature.Problem
See #5325. In short: the count of leading delimiters was load-bearing, and two live failures came from getting it wrong by one —
GMAIL_LIST_THREADS[||50|<query>]failing schema validation 12 times in one turn, and a later call that shiftedqueryanduser_idone slot late and ran, searching Gmail with the account id set to the search text.The second is the one that motivates the strictness here: it did not fail. A wrong call succeeded.
Solution
Indices remove the counting. There is nothing to miscount in
[3|value|4|me].Refusing beats guessing. An odd token count, a non-numeric index, or an index the schema has no parameter for returns
None. That includes the old bare-positional form, deliberately: parsing it positionally is exactly the silent misbinding this replaces. The failure mode moves from a wrong call that succeeds to a malformed call the model is told about.Repeated slots take the last value rather than failing the call — rare, and the later value is the model's latest intent.
Required-first ordering is kept, and is why the minimal call is
name[0|value]rather than an arbitrary number the model has to look up.One trap worth knowing about, now documented in the code
The typed-turn decoder's
parse_pformat_callsplits on raw|without honouring escapes. Its array is therefore a list of fragments whose join reconstructs the body, not a list of arguments — an escaped pipe arrives as two elements. Index-tagging those fragments numbers the halves of a single value.An earlier revision of this PR did exactly that and
an_escaped_pipe_stays_inside_its_argumentcaught it. The join must stay verbatim; the model's own indices ride inside it. Both the reconstruction sites now say so.Submission Checklist
Closes #NNNin the## RelatedsectionTesting
Six new cases, four of them failure paths:
an_out_of_range_slot_is_refusedan_odd_token_count_is_refuseda_bare_positional_call_is_refused_not_reinterpreted— the guarantee that replaces the old failure modethe_live_gmail_call_binds_correctly_with_indices— the two live shapes, written with indices, plus an assertion that a stray delimiter is now refused rather than misbounda_sparse_call_names_only_the_slots_it_fillsa_repeated_slot_takes_the_last_valuePlus the existing grammar suite rewritten to the new form (24 cases).
cargo test --lib: 12600 passed. The 2 remaining failures (tinyplace::manifest) reproduce on the base commit with no changes applied.Impact
[3|x|4|y]rather than[||||x|y]).GMAIL_LIST_THREADSturn: what a refusal reads like to the model, and whether it corrects in one turn, is the observation still owed.Related
Closes #5325
Summary by CodeRabbit