feat: tool-type scaffold generator and registration records#131
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:
📝 WalkthroughWalkthroughThe PR adds JSON-backed tool type records, a scaffold generator for coordinated backend/frontend artifacts and fixtures, static manifest extraction, validation tests, workflow documentation, and an expanded benchmark search query. ChangesTool type scaffolding
Benchmark search scope
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Contributor
participant scaffold_tool_type.py
participant ToolTypeRecord
participant Repository
Contributor->>scaffold_tool_type.py: provide record or CLI name
scaffold_tool_type.py->>ToolTypeRecord: load or construct registration
scaffold_tool_type.py->>Repository: stage coordinated artifact updates
scaffold_tool_type.py->>Repository: write files or print dry-run plan
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
🧹 Nitpick comments (1)
tests/test_scaffold_tool_type.py (1)
542-574: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider adding coverage for
--write-record-only.The CLI supports
--write-record-only(scripts/scaffold_tool_type.pyLines 547-554) but no test in this file exercises it. Worth a small test verifying it writes only the record JSON and skips codegen.🤖 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 `@tests/test_scaffold_tool_type.py` around lines 542 - 574, Add a focused test in the scaffold tool type test suite covering the --write-record-only CLI option: verify it writes the record JSON output and does not invoke or produce generated code. Reuse the existing test helpers and assertions for scripts/scaffold_tool_type.py rather than adding unrelated coverage.
🤖 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 `@CONTRIBUTING.md`:
- Around line 163-166: Make result-side workflow guidance conditional on result
registration: update CONTRIBUTING.md lines 163-166 to qualify the TODO list and
“seven sites” claim, docs/architecture.md line 78 to qualify the generated
artifact list, and tool_types/README.md lines 21-22 to qualify result-side
completion steps. Ensure use-only tools created with --no-result are described
as omitting result dispatch, TypedDict/guard, parser-fixture, and
overlap-fixture artifacts.
- Line 164: Update the verification commands in CONTRIBUTING.md (line 164) and
tool_types/README.md (line 24) to use one complete checklist covering scaffold,
parser, adversarial, frontend, generator, and integration tests as applicable,
replacing the current sync/order-only coverage.
In `@models/tool_type_registry.py`:
- Around line 289-296: Update the guard_name and builder_name properties to
derive their identifiers from the tool result’s dispatch_id rather than
self.snake_name. Keep the generated naming consistent with js_render_result_name
and guard_name_for_dispatch_id so records whose name differs from dispatch_id
resolve to the same guard and builder symbols.
- Around line 146-149: Update ToolResultRecord.from_mapping() to validate that
priority is a non-negative integer after the existing type check, raising
ValueError for negative values while preserving acceptance of zero and positive
priorities.
In `@scripts/scaffold_tool_type.py`:
- Around line 131-140: Ensure dry-run emissions retain the staged artifact paths
in the result returned by _flush and emit, while still skipping filesystem
writes. Update the dry_run branch so it does not bypass tracking in
self._written, allowing main’s “Dry run complete” message to report the actual
planned artifact count without changing normal write behavior.
- Around line 158-163: Replace the unchecked str.replace insertions in the
scaffold generation flow—including guard imports, guard-function/tool-name
additions, exporter branches, registry imports, and test ordering imports—with a
shared validation helper or _insert_before-style operation that raises when the
anchor is missing. In the ordering-test logic, validate and insert before_guard
and after_guard independently rather than gating both on before_guard. Add
coverage mirroring test_emit_marker_miss_leaves_repo_unchanged to verify a
missing anchor fails without modifying the repository.
- Around line 457-485: The marker and brace-parsing logic is duplicated across
the scaffold and manifest generator, risking divergent behavior. Keep or
relocate _parse_handlers_from_text from scripts/scaffold_tool_type.py as the
single shared helper, then update scripts/gen_tool_types_manifest.py’s
_load_known_tool_types to read the file with UTF-8 encoding and delegate to that
helper; apply no direct change to the shared parser beyond making it importable
if needed.
In `@tool_types/README.md`:
- Line 40: Update the result.priority description in the README to describe it
as the relative priority among overlapping predicates, noting that dispatch
selects the highest matching priority rather than implying it is always 1.
---
Nitpick comments:
In `@tests/test_scaffold_tool_type.py`:
- Around line 542-574: Add a focused test in the scaffold tool type test suite
covering the --write-record-only CLI option: verify it writes the record JSON
output and does not invoke or produce generated code. Reuse the existing test
helpers and assertions for scripts/scaffold_tool_type.py rather than adding
unrelated coverage.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 39d73387-4f1c-41b4-9c75-1495f9872fa1
📒 Files selected for processing (7)
CONTRIBUTING.mddocs/architecture.mdmodels/tool_type_registry.pyscripts/gen_tool_types_manifest.pyscripts/scaffold_tool_type.pytests/test_scaffold_tool_type.pytool_types/README.md
Also fix ruff format and search benchmark date-window flake.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@tests/test_scaffold_tool_type.py`:
- Around line 139-157: Strengthen
test_write_record_only_writes_json_without_codegen and the anchor-failure test
by snapshotting all emitter target files before invoking the scaffold command.
Assert the intentional fixture corruption actually changes its content, then
compare every snapshotted file after the command to ensure no generated artifact
was modified, while preserving the existing record and failure assertions.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: e28c6ae4-bfa7-49da-9e6a-140ee2729d69
📒 Files selected for processing (8)
CONTRIBUTING.mddocs/architecture.mdmodels/tool_type_registry.pyscripts/gen_tool_types_manifest.pyscripts/scaffold_tool_type.pytests/benchmarks/test_search_bench.pytests/test_scaffold_tool_type.pytool_types/README.md
🚧 Files skipped from review as they are similar to previous changes (5)
- docs/architecture.md
- tool_types/README.md
- CONTRIBUTING.md
- models/tool_type_registry.py
- scripts/scaffold_tool_type.py
There was a problem hiding this comment.
🧹 Nitpick comments (2)
scripts/scaffold_tool_type.py (2)
297-316: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDead
elsebranch:body_linesis never empty.
keys = record.use_input_keys or ("input",)(Line 300) always defaults to a non-empty tuple, sobody_lines(Line 301) can never be empty and the fallback" // TODO: read tool.input fields."at Line 302 is unreachable.♻️ Simplify
- body_lines = [f" const {key} = inp.{key} ?? '';" for key in keys] - body = "\n".join(body_lines) if body_lines else " // TODO: read tool.input fields." + body = "\n".join(f" const {key} = inp.{key} ?? '';" for key in keys)🤖 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 `@scripts/scaffold_tool_type.py` around lines 297 - 316, Remove the unreachable empty-list fallback in _plan_js_tool_use and construct body directly from the guaranteed non-empty keys derived from record.use_input_keys or ("input",). Preserve the generated input-reading statements and all surrounding summary and body rendering behavior.
450-486: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRobust patch helpers now support repeated scaffolding; one implicit assumption worth documenting.
_append_to_block(Lines 474-485) finds the closing delimiter via the first\n)/\n]afterblock_start, which correctly handles repeated appends across multiple scaffold runs (validated bytest_append_to_block_inserts_before_closing_parenandtest_scaffold_two_result_tools_passes_dispatch_sync). It implicitly assumes no nested(/[appears inside the block before its true closing delimiter — true for every current call site (plain import lists,Literal[...]), but not asserted. Worth a short comment noting the assumption so a future call site with a more complex block doesn't silently mis-locate the delimiter.Separately, the ast-grep "XPath injection" hints on
text.find(...)(Lines 458, 476, 481) are false positives — this is in-process Python string search, not XPath evaluation; no action needed there.🤖 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 `@scripts/scaffold_tool_type.py` around lines 450 - 486, Document the parsing assumption in _append_to_block with a brief comment: it locates the first newline followed by ) or ] and therefore requires block_start’s contents to contain no nested ( or [ delimiters before the true closing delimiter. Leave the existing search behavior unchanged, and do not modify the text.find calls.Source: Linters/SAST tools
🤖 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 `@scripts/scaffold_tool_type.py`:
- Around line 297-316: Remove the unreachable empty-list fallback in
_plan_js_tool_use and construct body directly from the guaranteed non-empty keys
derived from record.use_input_keys or ("input",). Preserve the generated
input-reading statements and all surrounding summary and body rendering
behavior.
- Around line 450-486: Document the parsing assumption in _append_to_block with
a brief comment: it locates the first newline followed by ) or ] and therefore
requires block_start’s contents to contain no nested ( or [ delimiters before
the true closing delimiter. Leave the existing search behavior unchanged, and do
not modify the text.find calls.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: cbf638a1-ce6e-4398-bd40-7e49ceda0711
📒 Files selected for processing (4)
models/tool_type_registry.pyscripts/gen_tool_types_manifest.pyscripts/scaffold_tool_type.pytests/test_scaffold_tool_type.py
🚧 Files skipped from review as they are similar to previous changes (1)
- models/tool_type_registry.py
|
Thanks, the anchor fix and the alpha/beta test look right, that was the main thing. |
I will add one comment for this. thanks for the comment. |
clean6378-max-it
force-pushed
the
feat/tool-type-scaffold
branch
from
edf4dde to
ff45f6b
Compare
3 participants
Closes #128
New tool types used to mean hand-editing seven files. Now you add a JSON record and run scripts/scaffold_tool_type.py; it generates the dispatch/export/JS/manifest stubs and you finish the TODOs.
Staged writes (no half-updated tree), --no-result for use-only tools, docs updated. pytest -q + npm test green.
Summary by CodeRabbit
--dry-runand--write-record-onlymodes to preview or limit generated changes.