[shaohuzhang1]

fix: Optimize knowledge base; AI-driven generation of tools and workflows

[shaohuzhang1]

Here is the code review for the provided TypeScript/JS code, focusing on irregularities, potential issues, and optimization suggestions.

1. Critical Issue: Reference Error in getNode

Location:

const startNode = getNode(context.lf, WorkflowNodeType.ToolStartNode)
if (startNode) { ... }

Problem: WorkflowNodeType.ToolStartNode is likely an enum value or string constant representing a node type. The function getNode(lf, nodeId) expects a unique node ID (usually a string UUID). Passing a type/name will almost certainly return undefined unless your specific lf (LogicFlow) instance uses names as IDs, which is non-standard and fragile.
Fix: You must retrieve the start node by its actual ID. If you don't have the ID, you need to find it via graph traversal (e.g., find the single node of type ToolStartNode).

// Pseudo-code fix:
const startNodeIds = context.lf.getNodeDataByType(WorkflowNodeType.ToolStartNode); 
// Or iterate nodes to find the one with that type
const startNode = context.lf.getNodes().find(n => n.type === WorkflowNodeType.ToolStartNode);

2. Logic Bug: Output Preservation & Overwriting

Location:

properties.user_output_field_list = outputs.map((output): ToolOutputField => ({
    type: 'string',
    is_required: true,
    ...cloneDeep(existing.get(String(output.field)) ?? {}),
    field: String(output.field),
    label: trimmed(output.label).slice(0, 128) || String(output.field),
}))

Problem: The spread operator ...cloneDeep(existing...) places existing properties before field and label. However, type: 'string' and is_required: true are hardcoded at the top. If an existing output had type: 'int', it is preserved. But if existing is empty {}, type defaults to 'string'.
More importantly, outputs in the config schema does not define type or is_required. The prompt implies users only care about field and label. If a user updates outputs, they might accidentally lose precise type information if their new input object doesn't match the field exactly (which it should, via parseFields), but the logic existing.get(field) relies entirely on exact field name matching.
Suggestion: Explicitly merge fields to ensure field and label from input override existing, while preserving other complex properties from existing safely. The current logic works mostly because field matches, but it's brittle. Also, cloneDeep inside a map loop is expensive. Map the existing list once outside.

3. Performance: cloneDeep in Loop

Location: Inside apply -> args.outputs !== undefined block.
Problem: cloneDeep is called for every single output item.
Optimization: Clone the existing map values once outside the map, or avoid cloning if you are creating a new object structure anyway. Since you are spreading existing, you are creating a new object, so deep cloning isn't strictly necessary unless existing objects have deeply nested structures that mutate. lodash-clonedeep is heavy. Consider using structured clone or simple spread if nesting is shallow (which ToolOutputField likely is).

4. Validation Gap: Input desc and label Truncation

Location:

if (trimmed(input.desc).length > 128) return new ErrorResult(...)

vs

label: trimmed(output.label).slice(0, 128) || String(output.field),

Inconsistency:

1. Errors on invalid length for inputs.
2. Silently truncates labels for outputs (slice(0, 128)).
3. desc for inputs is validated, but does the backend accept it?
4. label for outputs is silently truncated. This can lead to data loss without feedback.
   Suggestion: Adopt a consistent strategy. Either validate and return errors for both, or truncate both and note it in the result. Silent truncation is usually bad UX for API/Schema definitions.

5. Type Safety: params and returns

Location:

const params: JsonObjectSchema = { type: 'object', properties: {} }
const returns: JsonObjectSchema = { type: 'object', properties: {}, additionalProperties: false }

Issue: params allows any additional property. returns forbids them.
If this tool "Updates" configuration, does it return the signed update? The comment says "交付最新签名" (deliver latest signature).
However, signatureOf(context) is used for the prompt.
The AgentSuccessResult receives ...signatureOf(context).
But wait, signatureOf calls fieldsOf on the context after writeNodeProperties. Does getNode immediately reflect updated properties? In many graph engines (like LogicFlow), getNode reads from the view/data store which might not be updated synchronously until the render cycle or explicit event trigger.
Risk: If writeNodeProperties is async or batched, getNode inside signatureOf might return stale data.
Suggestion: Construct the newSignature manually using the properties you just calculated, rather than reading back from getNode.

6. Code Style & Minor Issues

1. Magic Strings: Reusing 128 limit for label/desc. Define constants MAX_LABEL_LEN, MAX_DESC_LEN.
2. Regex FIELD_PATTERN: /^[A-Za-z_][A-Za-z0-9_]{0,63}$/ enforces max 64 chars. Ensure this aligns with backend database constraints (e.g., PostgreSQL identifiers often max out at 63 bytes due to UTF-8 considerations, or 64). Comment mentions 64.
3. Event Emission: context.lf.graphModel.eventCenter.emit('refreshFieldList', undefined). Emitting undefined is safe but some listeners might expect an object payload. Check if listeners handle undefined.
4. Import Order: Standard linting rules usually prefer grouping: External libs -> Internal aliases (@/) -> Relative imports. Your order seems mostly fine but mixed.

Refactored Example (Addressing Critical Issues)

import { cloneDeep } from 'lodash'
import { getNode, writeNodeProperties } from '@/workflow-canvas/ai-generate/tools/canvas'
import { useNodeAgent } from '@/workflow-canvas/ai-generate/tools/node'
import { AgentSuccessResult, ErrorResult } from '@/workflow-canvas/ai-generate/tools/result'
import type { JsonObjectSchema, NodeAgentContext, NodeAgentModule } from '@/workflow-canvas/ai-generate/type'
import { WorkflowNodeType } from '@/workflow-canvas/types'
import type { ToolInputField, ToolOutputField, ToolConfigArgs } from './types' // Assume args type exists

/**
 * tool-base-node（工具基本信息）的 AI 生成配置：声明工具工作流的函数签名。
 */

const INPUT_TYPES = ['string', 'int', 'float', 'boolean', 'dict', 'array']
const FIELD_PATTERN = /^[A-Za-z_][A-Za-z0-9_]{0,63}$/
const FIELD_DESCRIPTION = '参数名：字母、数字、下划线，不以数字开头，最长 64'
const MAX_TEXT_LEN = 128

const params: JsonObjectSchema = { type: 'object', properties: {} }

const config: JsonObjectSchema = {
  type: 'object',
  properties: {
    inputs: {
      type: 'array',
      items: {
        type: 'object',
        properties: {
          field: { type: 'string', description: FIELD_DESCRIPTION },
          label: { type: 'string', description: `显示名称，最长 ${MAX_TEXT_LEN}` },
          type: { type: 'string', enum: INPUT_TYPES, description: '数据类型' },
          desc: { type: 'string', description: `参数说明，最长 ${MAX_TEXT_LEN} 字` },
          is_required: { type: 'boolean', description: '是否必填，默认 true' },
        },
        required: ['field', 'label', 'type', 'desc'],
      },
      description: '输入参数列表',
    },
    outputs: {
      type: 'array',
      items: {
        type: 'object',
        properties: {
          field: { type: 'string', description: FIELD_DESCRIPTION },
          label: { type: 'string', description: `显示名称，最长 ${MAX_TEXT_LEN}` },
        },
        required: ['field', 'label'],
      },
      description: '输出参数列表',
    },
  },
}

const trimmed = (value: unknown) => String(value ?? '').trim()
const isValidLen = (val: unknown, max: number = MAX_TEXT_LEN) => trimmed(val).length <= max

function fieldsOf<T>(context: NodeAgentContext, key: 'user_input_field_list' | 'user_output_field_list'): T[] {
  const fields = getNode(context.lf, context.nodeId)?.properties[key]
  return Array.isArray(fields) ? (fields as T[]) : []
}

// Helper to find start node by type if ID is not known directly
function getStartNode(lf: any): any {
    const nodes = lf.getNodes();
    return nodes.find((n: any) => n.type === WorkflowNodeType.ToolStartNode);
}

function useAgent(context: NodeAgentContext) {
  return useNodeAgent(context, {
    prompt: [
      'tool-base-node 配置要点...',
      `当前签名：${JSON.stringify({inputs: fieldsOf(context, 'user_input_field_list'), outputs: fieldsOf(context, 'user_output_field_list')})}`, // Simpler JSON
    ].join('\n'),
    tools: [
      {
        name: 'update',
        apply: (args) => {
          const node = getNode(context.lf, context.nodeId)
          if (!node) return new ErrorResult(`节点不存在`)
          if (args.inputs === undefined && args.outputs === undefined) return new ErrorResult('inputs/outputs 至少传一个')

          // Prepare properties to write
          const newProps: Record<string, unknown> = {}
          
          // --- Process Inputs ---
          let finalInputs: ToolInputField[] = []
          if (args.inputs !== undefined) {
            if (!Array.isArray(args.inputs)) return new ErrorResult('inputs MUST be array')
            
            const seen = new Set<string>()
            finalInputs = args.inputs.map((item, index) => {
                const field = String(item.field || '')
                if (!FIELD_PATTERN.test(field)) throw new Error(`index ${index}: bad field name`)
                if (seen.has(field)) throw new Error(`duplicate ${field}`)
                seen.add(field)
                
                const type = String(item.type)
                if (!INPUT_TYPES.includes(type)) throw new Error(`bad type ${type}`)
                
                const label = trimmed(item.label)
                const desc = trimmed(item.desc)
                
                if (label.length === 0) throw new Error(`empty label at ${index}`)
                if (label.length > MAX_TEXT_LEN) throw new Error(`label too long at ${index}`)
                if (desc.length > MAX_TEXT_LEN) throw new Error(`desc too long at ${index}`)

                return {
                    field,
                    label,
                    type,
                    desc,
                    is_required: item.is_required !== false
                }
            })
            newProps.user_input_field_list = finalInputs
          } else {
              finalInputs = fieldsOf(context, 'user_input_field_list')
          }

          // --- Process Outputs ---
          let finalOutputs: ToolOutputField[] = []
          if (args.outputs !== undefined) {
             if (!Array.isArray(args.outputs)) return new ErrorResult('outputs MUST be array')
             
             // Build a lookup map of existing outputs for cheap access
             const existingMap = new Map(fieldsOf(context, 'user_output_field_list').map(f => [f.field, f]))
             
             finalOutputs = args.outputs.map((item, index) => {
                 const field = String(item.field || '')
                 if (!FIELD_PATTERN.test(field)) throw new Error(`bad field name in outputs`)
                 
                 const existing = existingMap.get(field) || {}
                 const label = trimmed(item.label)
                 
                 // Decide strategy: Throw or Truncate? Let's Trim for safety but warn? 
                 // For now, following original logic but safer variable scope
                 const finalLabel = label.slice(0, MAX_TEXT_LEN) || field
                 
                 // Merge: Keep existing specific types like 'desc' if they exist in full object, 
                 // but force our controlled fields
                 return {
                     type: existing.type || 'string',
                     is_required: existing.is_required ?? true,
                     ...existing, 
                     field, // Overwrite
                     label: finalLabel, // Overwrite
                 } as ToolOutputField
             })
             newProps.user_output_field_list = finalOutputs
          } else {
              finalOutputs = fieldsOf(context, 'user_output_field_list')
          }

          // Write Properties
          writeNodeProperties(node, newProps)

          // Sync Start Node
          const startNode = getStartNode(context.lf)
          if (startNode && finalInputs.length > 0) {
             const globalFields = finalInputs.map(f => ({ 
                label: f.label || f.field, 
                value: f.field 
             }))
             writeNodeProperties(startNode, {
                config: {
                  ...(startNode.properties.config || {}),
                  globalFields
                }
             })
             context.lf.graphModel.eventCenter.emit('refreshFieldList')
             startNode.clearNextNodeField(true)
          }

          // Return success with the NEWLY calculated signature (don't rely on getNode re-fetching)
          return new AgentSuccessResult({
            inputs: finalInputs.map(f => ({ field: f.field, label: f.label, type: f.type, is_required: f.is_required, desc: f.desc, ref: ['global', f.field] })),
            outputs: finalOutputs.map(f => ({ field: f.field, label: f.label, ref: ['output', f.field] })) ,
            note: 'updated'
          })
        },
      },
    ],
  })
}

[shaohuzhang1]

Here's my review of the code:

Potential Issues

1. Non-null assertion without guarantee (getNode(context.lf, context.nodeId)!)
If getNode can return undefined, this will throw at runtime. Better to guard:

const node = getNode(context.lf, context.nodeId)
if (!node) return '无法获取当前节点'

2. reference empty-array edge case
argReference returning [] is truthy, so the error branch for "empty documents" won't fire even when there are no documents. If an empty binding is invalid, add an explicit length check:

if (!reference || reference.length === 0) return '...'

3. Truthiness checks on numeric values

...(chunkSize ? { chunk_size: chunkSize } : {})

chunkSize is number | undefined. Since LENGTH_MIN is 50, a valid value is never 0, so this works now — but it's fragile. Use chunkSize !== undefined for clarity/robustness.

4. Boolean field iteration is overly clever

booleanFields.flatMap((field) => [[field, args[field]], [`${field}_type`, 'custom']])

This relies on Object.fromEntries accepting nested arrays of tuples; it works but is hard to read and easy to break. Consider a plain loop or reduce.

5. patterns validation is loose
Invalid splitPatternOptions labels silently fall through to String(pattern) (treated as custom regex). That may be intended, but an arbitrary typo like "###" becomes a literal regex pattern. Consider validating that custom patterns are valid regexes, or at least logging/warning.

Correctness / Consistency

6. Schema/runtime mismatch for paragraph_title_relate_problem
Description says "qa 策略不适用" but the code writes it regardless of split_strategy. Consider filtering it out when strategy is qa, or clarify in the prompt.

7. chunk_size_type / limit_type / patterns_type: 'custom'
These are always set to 'custom' even when only some fields are provided. Confirm backend tolerates these type markers only appearing alongside their value (it looks correct, but worth verifying).

Optimization Suggestions

8. Duplicate enum between schema and constant
SPLIT_STRATEGIES.includes(...) is fine, but consider deriving enum: SPLIT_STRATEGIES — you already do this. Good.

9. Repeated readNodeData + spread
Minor; acceptable.

10. Prompt strings are hardcoded Chinese literals while code/answer should be English
That's a product decision, but flag if mixed-language is undesired.

11. lengthOf return type uses string for error signaling
Using a discriminated result (e.g., { ok: true, value } | { ok: false, message }) is clearer than number | string, since callers must use typeof === 'string' narrowing which is easy to misread as success/failure inverted.

Minor

12. INPUT_DOCUMENT_SCHEMA.meta has no additionalProperties restriction — fine, but inconsistent with returns.additionalProperties: false.

13. Comment references @@ -0,0 +1,154 @@ — that's diff metadata, not part of the file.

[shaohuzhang1]

Here are the issues and suggestions:

Correctness / Robustness

1. _run asserts fail inside a worker thread. assert errors == [None], errors runs on the main thread after finished.wait(10), but if on_complete is invoked with an exception, threading.Event.set() is still called — fine — however any assertion raised in on_complete/on_next (executed in the workflow thread) will be swallowed and only surface as a timeout, making failures hard to diagnose. Consider capturing sys.exc_info() in the callback and re-raising on the main thread.

2. finished.wait(10) timeout is hard-coded and short. On slow CI machines nested loops can exceed 10s. Make it configurable (env var) or larger.

3. Mutable default avoided via node_data or {} — good, but note that passing an empty dict {} as node_data will be replaced by a fresh dict; this is intentional but subtle. Fine.

4. _assign signature is fragile. It accepts (fields_list, value) pairs positionally; a typo like passing a string instead of a list will silently produce wrong field names (".".join("ab")). Add a guard/validation in the helper.

5. on_next calls wf_manage.get_details() but discards the result. This looks like it's meant to exercise detail retrieval mid-iteration; but if it's just for coverage, add an assert on its shape or a comment clarifying intent. As written it may hide regressions.

6. errors.append(error) then assert errors == [None] — order/single-element assumption. If callbacks can fire multiple times (e.g., partial + final), this will break. Assert on all(e is None ...) instead.

7. get_start_node closure reads loop_node["id"] from the outer scope. It works because _run only ever handles one loop node, but the parameter wf/manage shadows expectations. Fine, but consider naming it _get_start_node or documenting that it ignores the passed loop_node.

8. Test asserts on internal Chinese literals and exact concatenation order ("012", "xxxx|yy"). These are implementation-coupled: if iteration order or string concatenation semantics change, tests break even if behavior is still correct. Consider asserting divisibility/count properties where possible.

9. Missing negative cases: no test for

   • loop variable declared but never assigned
   • reassignment isolation across sibling loops at the same nesting level
   • concurrency (two WorkflowManage running simultaneously)
   • references to undefined variables raising AppApiException

10. self.assertNotIn("loop", manage.context["loop1"]["last_context"]) and the analogous "global" checks rely on the exact key name last_context. That's a stable contract? If not, extract the constant.

11. Duplication between tests. The inner-loop body construction is nearly identical; a helper _inner_loop_body(...) would reduce drift.

12. _loop_start(*variables) exposes both index/item and per-variable fields, which is presumably a schema requirement, but the test doesn't clarify why both forms are used. Add a short docstring/comments.

Style / Minor

13. No assertions that on_next actually fired — if the workflow emits zero outputs, the "detail available mid-loop" contract isn't tested. Track a counter and assert > 0.

14. threading.Event + list is not a race-free error channel — appending to a list from a worker and reading after Event.set() is safe in CPython due to GIL, but document it or use a queue.Queue.

15. "NUMBER" literal appears three times; pull into a constant or reference the enum used by production code.

16. x: 0, y: 0 in _node are UI-only fields; irrelevant to the engine — fine but could be omitted if the schema allows.

17. Error message "workflow did not finish" — good; consider including elapsed time and last-seen state for debugging.

Optimization (minor): the tests assume synchronous execution inside the workflow thread; if the engine ever parallelizes loop bodies, tests 13 & 6 need revisiting. Flag this assumption with a comment near _run.