Add generated tool wrapper abstraction#2333
Conversation
📝 WalkthroughWalkthroughAdds a new ChangesGenerated Tool Framework
Sequence Diagram(s)sequenceDiagram
participant Caller
participant GeneratedTool
participant SchemaCleanr
participant GeneratedToolAdapter
Caller->>GeneratedTool: new(definition, adapter)
GeneratedTool->>SchemaCleanr: validate(definition.parameters_schema)
SchemaCleanr-->>GeneratedTool: validation result
GeneratedTool->>GeneratedToolAdapter: adapter.id() (verify matches definition.adapter_id)
GeneratedTool-->>Caller: Result::Ok(GeneratedTool) or Err
Caller->>GeneratedTool: execute(args)
GeneratedTool->>GeneratedToolAdapter: execute(definition, args)
GeneratedToolAdapter-->>GeneratedTool: ToolResult
GeneratedTool-->>Caller: ToolResult
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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 |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/openhuman/tools/generated.rs (1)
65-73: ⚡ Quick winAdd debug/trace diagnostics on validation and adapter-mismatch error paths.
These branches are key contract failures; lightweight structured logs would improve operability and debugging.
As per coding guidelines: “Use
log/tracingatdebugortracelevel on … error paths … and any branch that is hard to infer from tests alone.”Also applies to: 129-138
🤖 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/tools/generated.rs` around lines 65 - 73, The validation and adapter-mismatch branches lack diagnostics; add lightweight structured tracing/log statements around validate_definition and before the anyhow::bail in the adapter check (and the similar branch at the other location) to record context like definition.name, definition.adapter_id, and adapter.id(); use tracing::debug! or tracing::trace! (or log::debug!) to emit these fields immediately before returning the error so operators can correlate failures without changing control flow.
🤖 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/tools/generated.rs`:
- Around line 129-139: The validate_definition function allows a blank or
whitespace adapter_id; update validate_definition to also trim and check
definition.adapter_id (e.g., let adapter_id = definition.adapter_id.trim()) and
call anyhow::bail! when it's empty (message like "generated tool `{name}`
adapter_id must be non-empty"), before validating parameters_schema so
GeneratedToolDefinition cannot pass with an empty adapter_id; keep the existing
name/description/schema checks and use the same error style as the other guards.
---
Nitpick comments:
In `@src/openhuman/tools/generated.rs`:
- Around line 65-73: The validation and adapter-mismatch branches lack
diagnostics; add lightweight structured tracing/log statements around
validate_definition and before the anyhow::bail in the adapter check (and the
similar branch at the other location) to record context like definition.name,
definition.adapter_id, and adapter.id(); use tracing::debug! or tracing::trace!
(or log::debug!) to emit these fields immediately before returning the error so
operators can correlate failures without changing control flow.
🪄 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
Run ID: 532b008d-f2d3-46c6-89a0-82a98ed654ba
📒 Files selected for processing (2)
src/openhuman/tools/generated.rssrc/openhuman/tools/mod.rs
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/openhuman/tools/generated.rs (1)
72-84:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winTrim
adapter_idconsistently before matching.
validate_definitiononly trimsadapter_idto reject blank values, but Line 72 still compares the raw string." echo-adapter "now passes validation and then fails the adapter check even when the logical ID matches. Normalize once on construction, or compare againstdefinition.adapter_id.trim()here, and add a padded-whitespace test next to the blank-id case.Also applies to: 149-150
🤖 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/tools/generated.rs` around lines 72 - 84, Normalize definition.adapter_id before comparisons: change code so adapter IDs are compared using a trimmed value (either by trimming and storing adapter_id at construction/validation in validate_definition, or by using definition.adapter_id.trim() wherever compared against adapter.id()). Update the comparison at the if statement that uses adapter.id() != definition.adapter_id and the similar check around lines 149-150 to use the trimmed form, and add a unit test that covers an adapter_id with surrounding whitespace (e.g., " echo-adapter ") alongside the existing blank-id test to ensure padded whitespace is accepted and matches correctly.
🤖 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.
Outside diff comments:
In `@src/openhuman/tools/generated.rs`:
- Around line 72-84: Normalize definition.adapter_id before comparisons: change
code so adapter IDs are compared using a trimmed value (either by trimming and
storing adapter_id at construction/validation in validate_definition, or by
using definition.adapter_id.trim() wherever compared against adapter.id()).
Update the comparison at the if statement that uses adapter.id() !=
definition.adapter_id and the similar check around lines 149-150 to use the
trimmed form, and add a unit test that covers an adapter_id with surrounding
whitespace (e.g., " echo-adapter ") alongside the existing blank-id test to
ensure padded whitespace is accepted and matches correctly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 499eda29-08a4-474a-b1e4-9d06ffe01298
📒 Files selected for processing (1)
src/openhuman/tools/generated.rs
graycyrus
left a comment
There was a problem hiding this comment.
Nice abstraction — the adapter pattern is clean, tests are solid, and the defensive validation is appreciated. One correctness issue to fix before this is good to go.
| File | Changes |
|---|---|
src/openhuman/tools/generated.rs |
New: GeneratedToolDefinition, GeneratedToolAdapter trait, GeneratedTool wrapper, batch conversion, validation, tests |
src/openhuman/tools/mod.rs |
Register generated module |
Note on #2133 coverage: This PR delivers the abstraction layer (definition + adapter + wrapper). The remaining acceptance criteria — registry integration, visibility/whitelisting, and tool-inspection surfaces — presumably land in follow-up PRs. Worth tracking explicitly so nothing slips.
|
Thanks for calling out the scope note. I fixed the correctness issue in b8076ed. For #2133 coverage: this PR is intentionally the abstraction layer only ( |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/openhuman/tools/generated.rs (1)
142-146: 💤 Low valueOptional: validate_definition now contains redundant
.trim()calls.Since
normalize_definitionis always invoked beforevalidate_definitionin the only caller (GeneratedTool::new), the.trim()calls insidevalidate_definition(lines 149, 153, 156) are now redundant — they operate on already-trimmed strings. You could simplify the empty-checks todefinition.name.is_empty()/definition.description.is_empty()/definition.adapter_id.is_empty()for clarity, or keep the trims as a defensive belt-and-braces ifvalidate_definitionmay be reused independently in the future. Not blocking.🤖 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/tools/generated.rs` around lines 142 - 146, The validate_definition function currently calls .trim() on fields that have already been normalized by normalize_definition, so update validate_definition to remove the redundant .trim() calls and use simple empty checks (e.g., definition.name.is_empty(), definition.description.is_empty(), definition.adapter_id.is_empty()); locate validate_definition and its caller GeneratedTool::new to confirm normalize_definition is still invoked first and ensure behavior remains unchanged (or keep trims if you prefer to retain defensive checks).
🤖 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/tools/generated.rs`:
- Around line 142-146: The validate_definition function currently calls .trim()
on fields that have already been normalized by normalize_definition, so update
validate_definition to remove the redundant .trim() calls and use simple empty
checks (e.g., definition.name.is_empty(), definition.description.is_empty(),
definition.adapter_id.is_empty()); locate validate_definition and its caller
GeneratedTool::new to confirm normalize_definition is still invoked first and
ensure behavior remains unchanged (or keep trims if you prefer to retain
defensive checks).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: f0e3da56-a3c6-4c1b-aa37-4278e92be81d
📒 Files selected for processing (1)
src/openhuman/tools/generated.rs
|
@graycyrus Thanks again for the review. I addressed the normalization issue in b8076ed and resolved the thread; current checks are green. Could you please take another look when you have a chance? |
graycyrus
left a comment
There was a problem hiding this comment.
Continuation review — previous changes addressed ✓
The [major] whitespace normalization finding from the prior review is resolved. Commit b8076ed adds normalize_definition() which trims name, description, and adapter_id before validation and storage — clean approach, and the new generated_tool_normalizes_definition_fields test covers it well.
No new issues found in the latest commit. Code is clean — well-structured abstraction with solid validation and good test coverage.
graycyrus
left a comment
There was a problem hiding this comment.
Looks good, nice work!
Summary
Toolwrapper.Tooltrait objects after schema validation and adapter-id checks.Refs #2133
Checklist
codex/oh-2133-generated-tools6254c676b2a660c3a486c710a2d6a8092884dedcsrc/openhuman/tools/generated.rs,src/openhuman/tools/mod.rscargo fmt --manifest-path Cargo.toml --all --check;cargo test --manifest-path Cargo.toml generated_tool;node scripts/check-pr-checklist.mjs /tmp/openhuman-2133-pr-body.mdpnpm pr:checklist /tmp/openhuman-2133-pr-body.mdblocked withzsh:1: command not found: pnpm; directnodechecklist script passedSummary by CodeRabbit
New Features
Improvements
Tests