Repository navigation
fix(frontend): surface nameless tools (workflow references) in the commit-diff Tools section - #5038
Conversation
dosubot
Bot
added
the
size:L
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughThis PR reworks tool normalization and diff classification in the commitDiff module. ChangesTool identity and diff classification
Estimated code review effort: 3 (Moderate) | ~25 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 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 |
…mmit-diff Tools section
The agent tool flow adds workflow-reference tools ({type:"reference", slug}) via "Reference a workflow" — they have no function.name, so normalizeTool dropped them (return null) and they were invisible to the classifier's Tools diff: adding/removing/editing one never showed in the commit summary, and the config panel's Tools header never tinted for them (the per-item row indicator already handled them via agentItemIdentity, so header and rows disagreed).
normalizeTool now surfaces every tool subtype, keyed by the canonical agentItemIdentity (function name / reference slug / platform op / positional fallback), with a fingerprint (whole-tool hash) for edit detection across subtypes; field-level detail stays function-only. NormalizedTool gains rawKey, fingerprint, isFunction. Note: the agent picker offers no builtin tools ("No built-in tools here") — the builtin/platform handling covers the prompt playground, which reuses this classifier and does have them, plus legacy/defensive cover.
a7200b6 to
c2983b7
Compare
ardaerzin
changed the title
|
@coderabbitai review |
✅ Action performedReview finished.
|
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
web/packages/agenta-entities/src/workflow/commitDiff/classify.ts (1)
96-103: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winFingerprint-based edit detection can yield "edited" rows with no visible detail.
Broadening edit detection to whole-object
fingerprintis a net improvement (catches changes the old description/paramsJson check missed), but forisFunctiontools it introduces a new possibility: a function tool's fingerprint can change due to fieldsdiffToolFieldsdoesn't inspect (anything besidesdescription/params.properties), yielding an "edited" item with emptyfieldChangesandtoolRowDetailreturningundefined. Previously this couldn't happen because "edited" itself was derived from the description/paramsJson diff.Consider a fallback detail (e.g.
"tool changed") intoolRowDetail/edited.mapwhenfieldsis empty but the tool is a function tool, so users aren't shown an unexplained "edited" badge.💡 Suggested fallback detail
function toolRowDetail(fields: ToolFieldChange[]): string | undefined { const descChanged = fields.some((f) => f.field === "description") const paramCount = fields.filter((f) => f.field !== "description").length const parts: string[] = [] if (descChanged) parts.push("description") if (paramCount) parts.push(paramCount === 1 ? "1 parameter" : `${paramCount} parameters`) - if (!parts.length) return undefined + if (!parts.length) return "changed" return `${parts.join(" & ")} changed` }
🧹 Nitpick comments (1)
web/packages/agenta-entities/tests/unit/agent-commit-diff.test.ts (1)
151-180: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueGood coverage for reference/builtin tools; consider adding added/edited builtin cases too.
Current tests cover reference-tool add/edit and builtin-tool remove. An added/edited builtin (e.g.
{type:"web_search"}) case would round out coverage of the fallback branches innormalizeTool/toolsSection.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: c113df24-45f1-428c-af42-fa40b1150f6f
📒 Files selected for processing (4)
web/packages/agenta-entities/src/workflow/commitDiff/accessors.tsweb/packages/agenta-entities/src/workflow/commitDiff/classify.tsweb/packages/agenta-entities/src/workflow/commitDiff/types.tsweb/packages/agenta-entities/tests/unit/agent-commit-diff.test.ts
| const fn = isObj(raw.function) ? raw.function : undefined | ||
| const fnName = fn && typeof fn.name === "string" ? fn.name : undefined | ||
|
|
||
| // Function / gateway tool. | ||
| if (fnName) { | ||
| const parsed = parseGatewayToolName(fnName) | ||
| const params = isObj(fn?.parameters) ? (fn?.parameters as Record<string, unknown>) : {} |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Preserve flat function-tool shapes.
The previous normalizer handled function data from raw.function or raw; this now only checks raw.function. A tool shaped like {name, description, parameters} falls through as generic "Tool" with positional identity, no rawKey, no params, and no field-level diffs. Update both normalization and agentItemIdentity so flat function tools still key as fn:<name>.
Proposed fix
- const fn = isObj(raw.function) ? raw.function : undefined
+ const fn = isObj(raw.function)
+ ? raw.function
+ : (raw.type === undefined || raw.type === "function") && typeof raw.name === "string"
+ ? raw
+ : undefinedAlso align web/packages/agenta-entities/src/workflow/commitDiff/identity.ts:
const fn = isObj(rec.function) ? rec.function : undefined
if (fn && typeof fn.name === "string" && fn.name) return `fn:${fn.name}`
+ if (
+ (rec.type === undefined || rec.type === "function") &&
+ typeof rec.name === "string" &&
+ rec.name
+ )
+ return `fn:${rec.name}`📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const fn = isObj(raw.function) ? raw.function : undefined | |
| const fnName = fn && typeof fn.name === "string" ? fn.name : undefined | |
| // Function / gateway tool. | |
| if (fnName) { | |
| const parsed = parseGatewayToolName(fnName) | |
| const params = isObj(fn?.parameters) ? (fn?.parameters as Record<string, unknown>) : {} | |
| const fn = isObj(raw.function) | |
| ? raw.function | |
| : (raw.type === undefined || raw.type === "function") && typeof raw.name === "string" | |
| ? raw | |
| : undefined | |
| const fnName = fn && typeof fn.name === "string" ? fn.name : undefined | |
| // Function / gateway tool. | |
| if (fnName) { | |
| const parsed = parseGatewayToolName(fnName) | |
| const params = isObj(fn?.parameters) ? (fn?.parameters as Record<string, unknown>) : {} |
…ly edits Address review feedback on #5038. (1) Regression fix: the rewritten normalizeTool only checked raw.function, so a flat legacy function tool {name,description,parameters} (no wrapper) fell through to the generic 'Tool' branch — lost its fn:<name> identity, rawKey, params and field diffs. Restore flat-function detection in BOTH normalizeTool (accessors) and agentItemIdentity (identity), guarded on type undefined/"function" so reference/builtin tools aren't misread. (2) toolRowDetail now returns "changed" instead of undefined when fingerprint-based edit detection flags a change diffToolFields can't itemize, so an 'edited' badge is never unexplained. Added flat-function + added/removed builtin tests (33/33).
|
@coderabbitai review |
✅ Action performedReview finished.
|
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)
web/packages/agenta-entities/src/workflow/commitDiff/classify.ts (1)
103-105: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winGate field-level diffs by
isFunction.Line 105 still computes
diffToolFieldsfor every fingerprint change, but theNormalizedToolcontract says only function tools get field-level diffs. Reference/builtin edits should register as edited with the generic"changed"detail.Proposed fix
- edited.push({tool, fields: diffToolFields(prev, tool)}) + edited.push({ + tool, + fields: prev.isFunction && tool.isFunction ? diffToolFields(prev, tool) : [], + })
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: fe169db3-3e32-4945-a591-b18d752a130c
📒 Files selected for processing (4)
web/packages/agenta-entities/src/workflow/commitDiff/accessors.tsweb/packages/agenta-entities/src/workflow/commitDiff/classify.tsweb/packages/agenta-entities/src/workflow/commitDiff/identity.tsweb/packages/agenta-entities/tests/unit/agent-commit-diff.test.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- web/packages/agenta-entities/tests/unit/agent-commit-diff.test.ts
- web/packages/agenta-entities/src/workflow/commitDiff/accessors.ts
Address review feedback on #5038. diffToolFields was called for every fingerprint change; only function tools have inspectable fields (the NormalizedTool contract). Reference/builtin edits now skip it and register as edited with the generic "changed" detail. No behavior change today (non-function tools carry empty params so diffToolFields already returned []), but makes the contract explicit and defensive.
|
@coderabbitai review |
✅ Action performedReview finished.
|
What
Follow-up to #5035. The commit-diff classifier's
normalizeTooldropped any tool without afunction.name(return null). The agent tool flow adds workflow-reference tools ({type: "reference", slug}) via "Reference a workflow" — these have nofunction.name, so they were invisible to the Tools section:agentItemIdentity), so the header and its rows disagreed.Fix
normalizeToolnow surfaces every tool subtype instead of dropping the nameless ones:agentItemIdentity(function name → reference slug → platform op → positional fallback), so entries never collapse;fingerprint(whole-tool stable hash) drives edit detection across all subtypes — function tools keep their field-level detail (diffToolFields); reference/builtin edits register via the fingerprint;NormalizedToolgainsrawKey(technical name shown in the detail view),fingerprint, andisFunction.Labels: function/gateway via
parseGatewayToolName, reference → slug, builtin → humanized type.Testing
rawKey. 31/31 pass.tsc --noEmitclean for@agenta/entitiesand@agenta/entity-ui; eslint/prettier/turbo-lint green.