[eb-v2 review] Code Tools: parameter validation, allowlist isolation, and wizard fixes - #1655
chiang-daniel wants to merge 4 commits into
Conversation
…nd as a typed flag Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…e generated docstrings valid for any description Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
WalkthroughThe change updates code-tool parameter validation, generated code, editor state resolution, form events, timeout classification, and allowlist handling. It adds API and unit coverage for these behaviors. ChangesCode-tool authoring flow
Parameter validation
Code-tool execution robustness
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Step1Form
participant CodeToolPage
participant resolveStep2Code
participant CodeEditor
Step1Form->>CodeToolPage: announce schema row change
CodeToolPage->>resolveStep2Code: pass current code and schema state
resolveStep2Code->>CodeToolPage: return resolved code and flags
CodeToolPage->>CodeEditor: update editor content
Possibly related PRs
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
🧪 Generate unit tests (beta)
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 |
| const requiredParams = params.filter((p) => p.required) | ||
| const optionalParams = params.filter((p) => !p.required) | ||
|
|
||
| // Schema property names are used verbatim: the sandbox calls run() with |
There was a problem hiding this comment.
This deletes safePythonName and the reserved-word table it used to rewrite parameter names in the generated stub. That rewrite was the actual bug behind the datamodel guard: it silently produced a stub whose parameter name didn't match the schema, so the sandbox's keyword call could never reach it. Now the stub uses the schema names verbatim and the reserved-word case is rejected at authoring instead of being papered over here.
| const safeDesc = toolDescription.replace(/"""/g, "''") | ||
| // Escape backslashes then double-quotes so the description can neither close the | ||
| // triple-quoted docstring nor leave a dangling escape (e.g. a trailing " or \). | ||
| const safeDesc = toolDescription.replace(/\\/g, "\\\\").replace(/"/g, '\\"') |
There was a problem hiding this comment.
The placeholder generator embeds the tool description inside a triple-quoted docstring. The old escaping only replaced literal """, which left two ways to emit non-compiling Python: a description ending in a backslash escaped the closing quote, and a stray double-quote could still break out. Escaping backslashes first, then double-quotes, makes the description safe for any input — verified round-trip in the tests.
| * Decide the Code step's editor contents when the user advances from the | ||
| * Define step. | ||
| * | ||
| * Keyed on whether code already exists — not on the wizard step — because the |
There was a problem hiding this comment.
The wizard decided the code editor's contents from current_step, but the documented Back path pops the shallow-routing history so current_step reads "define" on a return — the discriminator was effectively always "first visit", and the code-preserving branch was unreachable. So returning to the code step wiped the user's authored code with a fresh placeholder. This pure helper keys the decision on whether code already exists instead of on the step, and the page (+page.svelte) now calls it. The clone seed is consumed once so a later return can't re-apply it over edits.
| // and tested-state tracking — react to structural row edits. The initial | ||
| // start_with_one seed runs before mount, so add_button_el is still undefined | ||
| // then and no spurious event fires. | ||
| function announce_row_change() { |
There was a problem hiding this comment.
Adding or removing a FormList row is a plain button click, which fires no native input/change event — so ancestor on:change handlers (the unsaved-changes guard, and the eval builder's tested-state tracking) never saw structural row edits. This dispatches a bubbling change from a stable in-tree anchor when a row is added or removed, the way a native control would. The start_with_one seed runs before mount, while the anchor is still undefined, so no spurious event fires on init.
| def validate_parameters_schema(self, info: ValidationInfo) -> Self: | ||
| validate_schema_dict(self.parameters_schema, require_object=True) | ||
|
|
||
| # Parameters are passed to run() as keyword arguments keyed by these names, |
There was a problem hiding this comment.
Personal review requested: Reserved-word rejection lives in the data-model validator with a loaded_from_file exemption, so an already-broken tool still loads but a PATCH that revalidates it now fails loudly. Do you agree with the layer (data model rather than API or client) and the posture (load old broken tools silently, reject on the next write)? The alternative postures are reject on load too, or warn-not-block on write.
| # Parameters are passed to run() as keyword arguments keyed by these names, | ||
| # and a Python reserved word can never be declared as a run() parameter, so | ||
| # reject it at authoring. Loaded files are exempt so old projects still open. | ||
| # Soft keywords (match/case/type/_) are valid identifiers and stay allowed. |
There was a problem hiding this comment.
Personal review requested: The check uses keyword.iskeyword, which deliberately allows soft keywords (match, case, type, _) because they are valid identifiers and work fine as run() parameters — only hard keywords are rejected. Confirm you want soft keywords allowed rather than reserved defensively (the stricter alternative would use keyword.issoftkeyword too).
| if not self.loaded_from_file(info): | ||
| for param_name in self.parameters_schema.get("properties", {}): | ||
| if keyword.iskeyword(param_name): | ||
| raise ValueError( |
There was a problem hiding this comment.
A parameter named after a Python keyword (e.g. from, class) is impossible: the sandbox invokes run() with keyword arguments keyed by the exact schema property names, and no reserved word can be a run() parameter. The old client helper hid this by renaming the parameter in the generated stub, so the stub declared from_param while the sandbox still called from=... — the tool was permanently broken and authoring never warned. Rejecting it here, in the data model, catches every write path. loaded_from_file is exempt so existing projects still open (see the flag on the posture).
| default=None, | ||
| description="Human-readable error message, set when is_error is True.", | ||
| ) | ||
| timed_out: bool = Field( |
There was a problem hiding this comment.
This is the structural signal that replaces the text-sniffing above. Adding it to ToolCallResult lets the timeout be reported once, where it's known (the execution runtime), and read as a boolean everywhere else — no caller has to re-derive "was this a timeout?" by parsing a human-readable message.
| kind = "timeout" if is_timeout else "call_error" | ||
| # The result carries timeouts as a typed flag; ordinary failures | ||
| # whose text merely mentions a timeout must stay call_error. | ||
| kind = "timeout" if result.timed_out else "call_error" |
There was a problem hiding this comment.
Nested-call timeout classification was a substring match on the result text ("timed out" in output). Any ordinary failure whose message happened to mention a timeout was misclassified as a real ToolTimeout, and conversely a timeout worded differently would be missed. The result now carries the timeout as a structural timed_out flag (added on ToolCallResult), so classification reads a boolean the runtime sets rather than guessing from free text.
| try: | ||
| fn_name = await self._canonical_tool_name(tool_id) | ||
| except Exception as exc: | ||
| # A broken entry (e.g. a deleted RAG config) must not take the |
There was a problem hiding this comment.
A single unresolvable allowlist entry — e.g. a nested tool pointing at a RAG config the user deleted — used to throw while building the name map and take the entire nested-tool surface down with it, so every healthy tool became uncallable. This isolates the failure per entry: a broken one is logged and left out of the map, and calls to it fail with the standard "not available" error while the rest keep working. list_tools() gets the matching treatment just below.
| "name": fn_name, | ||
| "description": "(unavailable)", | ||
| "name": tool_id, | ||
| "description": f"(unavailable: {exc})", |
There was a problem hiding this comment.
Personal review requested: This changes the list_tools contract: a broken allowlist entry now renders as (unavailable: <reason>) under its raw id, instead of failing the whole call. That's a deliberate degraded-UX choice — the surface stays usable and the broken entry is visible, but its intended name is unknowable without resolving it. Confirm you're happy exposing the id and the raw exception text here, versus a generic "unavailable" with no detail.
|
|
||
| # Lazy import — PythonCodeTool is phase 2; for now return a | ||
| # lightweight wrapper that has the tool's identity and definition. | ||
| # Lazy import — PythonCodeTool pulls in the multiprocessing execution |
There was a problem hiding this comment.
The comment described a design that no longer exists — a "phase 2 / lightweight wrapper" placeholder — when this returns the full PythonCodeTool execution runtime. Left as-is it would mislead the next reader into thinking the real tool wasn't wired up yet. Rewritten to state the actual reason the import is lazy: it pulls in the multiprocessing runtime that most registry lookups don't need.
![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)
app/web_ui/src/lib/utils/code_tool_helpers.ts (1)
197-205: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDrop the
as stringcast.
useCloneis a separate boolean, so TypeScript cannot narrowcloneCodethrough it. TestcloneCodedirectly and the cast becomes unnecessary.♻️ Proposed refactor
if (code === "") { - const useClone = !!cloneCode return { - code: useClone ? (cloneCode as string) : newPlaceholder, + code: cloneCode ? cloneCode : newPlaceholder, generatedPlaceholder: newPlaceholder, schemaChangedHint: false, - cloneConsumed: useClone, + cloneConsumed: !!cloneCode, } }🤖 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 `@app/web_ui/src/lib/utils/code_tool_helpers.ts` around lines 197 - 205, Update the empty-code branch in the relevant helper to test cloneCode directly when selecting the returned code value, rather than using the separate useClone boolean for narrowing; remove the unnecessary as string cast while preserving cloneConsumed’s existing boolean behavior.
🤖 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 `@app/web_ui/src/lib/utils/code_tool_helpers.ts`:
- Around line 216-223: Update the schemaChangedHint decision in the
newPlaceholder comparison so description-only edits do not mark the schema as
changed; compare the generated parameter list or equivalent parameter-specific
representation instead of the full placeholder, while preserving cloneConsumed
and generatedPlaceholder behavior.
---
Nitpick comments:
In `@app/web_ui/src/lib/utils/code_tool_helpers.ts`:
- Around line 197-205: Update the empty-code branch in the relevant helper to
test cloneCode directly when selecting the returned code value, rather than
using the separate useClone boolean for narrowing; remove the unnecessary as
string cast while preserving cloneConsumed’s existing boolean 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: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: dc2c2cba-ff2c-47f3-8d30-dfb4c80716bd
📒 Files selected for processing (11)
app/desktop/studio_server/test_code_tool_api.pyapp/web_ui/src/lib/utils/code_tool_helpers.test.tsapp/web_ui/src/lib/utils/code_tool_helpers.tsapp/web_ui/src/lib/utils/form_list.svelteapp/web_ui/src/routes/(app)/tools/[project_id]/add_tools/code_tool/+page.sveltelibs/core/kiln_ai/datamodel/code_tool.pylibs/core/kiln_ai/datamodel/test_code_tool.pylibs/core/kiln_ai/sandbox/test_code_tool_execution.pylibs/core/kiln_ai/tools/base_tool.pylibs/core/kiln_ai/tools/code_tool.pylibs/core/kiln_ai/tools/tool_registry.py
| if (newPlaceholder !== generatedPlaceholder) { | ||
| return { | ||
| code, | ||
| generatedPlaceholder: newPlaceholder, | ||
| schemaChangedHint: true, | ||
| cloneConsumed: false, | ||
| } | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
The hint fires for description-only edits.
newPlaceholder embeds the escaped toolDescription as well as the parameter list. A user who returns to the Define step, edits only the description, and continues gets schemaChangedHint: true. The UI then shows "Schema changed — check run()'s parameters." even though no parameter changed.
Two options: compare only the generated parameter list to decide the hint, or reword the message so it covers any regenerated-stub difference.
🤖 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 `@app/web_ui/src/lib/utils/code_tool_helpers.ts` around lines 216 - 223, Update
the schemaChangedHint decision in the newPlaceholder comparison so
description-only edits do not mark the schema as changed; compare the generated
parameter list or equivalent parameter-specific representation instead of the
full placeholder, while preserving cloneConsumed and generatedPlaceholder
behavior.
📊 Coverage ReportOverall Coverage: 93% Diff: origin/review/eb-v2/base...HEAD
Summary
|
|
Superseded. The branch is now split into seven review-only views with the current tip; this area is inside them. Closing so there is one set to review. |
1 participant
Surface: code-tool execution runtime, the CodeTool data model, and the code-tool authoring wizard. Commits:
afe821bd1 (landed as c8876814e)(isolate broken allowlist entries so one dead entry can't take the whole nested-tool surface down; carry tool timeouts as a typed flag instead of sniffing output text),4a63c3336 (landed as 34e6367a0)(reject reserved-word parameter names at authoring and generate docstrings that stay valid for any description),9665236fc (landed as 5dd2487f8)(preserve authored code when returning to the wizard's code step), plus one hunk of1c9a0cb1d (landed as one hunk of 6f1afc872)(make FormList row add/remove emit a change event so ancestor handlers notice structural edits). Around half the diff is tests. Three Personal review requested comments flag the layer/posture choices.🤖 Generated with Claude Code
CI note: the "Check API Schema Bindings" failure here is an artifact of this review PR's pinned snapshot — the canonical schema verification lives on
dchiang/eb-v2-merge. No action needed from reviewers.