Skip to content

feat(dashboard): refactor agent page to use react-hook-form and zod schemas from agents-core#1799

Merged
dimaMachina merged 95 commits intomainfrom
prd-5298-2
Mar 26, 2026
Merged

feat(dashboard): refactor agent page to use react-hook-form and zod schemas from agents-core#1799
dimaMachina merged 95 commits intomainfrom
prd-5298-2

Conversation

@dimaMachina
Copy link
Copy Markdown
Collaborator

@dimaMachina dimaMachina commented Feb 7, 2026

json editors now uses GenericJsonEditor and have expand button too
image

@changeset-bot
Copy link
Copy Markdown

changeset-bot Bot commented Feb 7, 2026

🦋 Changeset detected

Latest commit: 2c4ab19

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 10 packages
Name Type
@inkeep/agents-manage-ui Patch
@inkeep/agents-api Patch
@inkeep/agents-cli Patch
@inkeep/agents-core Patch
@inkeep/agents-email Patch
@inkeep/agents-mcp Patch
@inkeep/agents-sdk Patch
@inkeep/agents-work-apps Patch
@inkeep/ai-sdk-provider Patch
@inkeep/create-agents Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@vercel
Copy link
Copy Markdown

vercel Bot commented Feb 7, 2026

The latest updates on your projects. Learn more about Vercel for GitHub.

Project Deployment Actions Updated (UTC)
agents-api Ready Ready Preview, Comment Mar 26, 2026 9:14pm
agents-manage-ui Ready Ready Preview, Comment Mar 26, 2026 9:14pm
1 Skipped Deployment
Project Deployment Actions Updated (UTC)
agents-docs Skipped Skipped Mar 26, 2026 9:14pm

Request Review

Comment on lines -406 to -407
min="1"
max="100"
Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I removed because we have zod validation for min/max

@inkeep
Copy link
Copy Markdown
Contributor

inkeep Bot commented Feb 7, 2026

No docs changes detected.

This PR refactors internal dashboard state management to use react-hook-form and consolidates Zod schemas within agents-core. These are internal implementation improvements that don't affect public APIs or documented features.

Comment on lines +164 to +165
agentData: FullAgentDefinition
): Promise<ActionResult<FullAgentDefinition>> {
): Promise<ActionResult<FullAgentResponse>> {
Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

mistake, should be response and not definition

Comment on lines -166 to +130
): Promise<ActionResult<FullAgentDefinition>> {
): Promise<ActionResult<FullAgentResponse>> {
Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

mistake, should be response and not definition

Comment on lines -43 to +95
export type GetAgentResponse = SingleResponse<FullAgentDefinition>;
export type UpdateFullAgentResponse = SingleResponse<FullAgentDefinition>;
export type GetAgentResponse = SingleResponse<FullAgentResponse>;
export type UpdateFullAgentResponse = SingleResponse<FullAgentResponse>;
Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

mistake, should be response

@pullfrog
Copy link
Copy Markdown
Contributor

pullfrog Bot commented Mar 16, 2026

TL;DR — Replaces the bespoke Zustand + imperative state management on the agent builder page with react-hook-form backed by zod schemas imported from agents-core. Form field data is now owned by RHF, Zustand is slimmed to graph-layout-only state, and validation/error display derives from zod + formState.errors instead of a custom error parser.

Key changes

  • react-hook-form + FullAgentFormContext — New form provider wraps the agent page; all field reads/writes go through useForm / useWatch / form.setValue instead of Zustand actions and useNodeEditor.
  • validation.ts schema layer — New zod schema (FullAgentUpdateSchema) bridges string-based editor inputs to typed agents-core schemas via .pipe() transforms, replacing hand-rolled validation and the deleted FullAgentDefinitionSchema.
  • agents-core schema tighteningnumberint, inline definitions → shared refs (ResourceIdSchema, NameSchema, DescriptionSchema, StringRecord), z.any()z.unknown()/z.record(), and FunctionCreate schema inlined at call sites.
  • Serialization rewriteserialize.ts / deserialize.ts no longer carry agent-level metadata; new hydrateNodesWithFormData() merges RHF data into nodes before serialization.
  • Error handling replacement — Deleted the 312-line agent-error-parser.ts; errors now derive from formState.errors via useGroupedAgentErrors and useProcessedErrors.
  • Deleted dead codeuseNodeEditor, useAgentErrors, agent-initializer-helpers.ts, save-agent.ts service, createFullAgentAction, FullAgentDefinition type, sidepane form-components/input.tsx + text-area.tsx + types.tsx.
  • Generic form components — New GenericCheckbox, GenericCodeEditor, GenericJsonEditor, GenericPromptEditor components auto-bind to RHF control + name.
  • Node editors migrated — All five node editors (sub-agent, MCP, function-tool, external-agent, team-agent) replaced prop-drilled useNodeEditor with direct RHF field paths.
  • Cypress E2E tests adapted — Selectors updated for new form structure; new dirty-state assertion tests added.
  • OpenAPI snapshot updated — Reflects schema ref consolidation (NameSchema, DescriptionSchema, StringRecord) and numberinteger corrections.

Summary | 95 files | 2 commits | base: mainprd-5298-2


react-hook-form integration and FullAgentFormContext

Before: All form data (agent metadata, node fields, errors, dirty state) lived in a single Zustand store. Field updates went through imperative helpers (updateField, updatePath, markUnsaved) in useNodeEditor.
After: A FullAgentFormProvider wraps the page with useForm({ resolver: zodResolver(FullAgentUpdateSchema), mode: 'onChange' }). Field data is owned by RHF; Zustand retains only graph layout (nodes, edges, undo/redo). Components read via useWatch and write via form.setValue.

The save button changed from onClick={saveAgent} to type="submit", and dirty state is a union of Zustand graph-dirty and RHF form-dirty. The unsaved-changes dialog checks both sources. Server-side 422 errors are parsed as z.ZodIssue[] and fed to form.setError().

page.client.tsx · full-agent-form.tsx · use-agent-store.ts · toolbar.tsx


validation.ts — zod schema bridging string editors to agents-core types

Before: Validation was split between a hand-rolled FullAgentDefinitionSchema in agents-core/client-exports.ts and runtime JSON-parse checks in serialize.ts.
After: A single FullAgentUpdateSchema in validation.ts uses .pipe() to chain StringToJsonSchema transforms into agents-core sub-schemas, handling empty-string → undefined and string → parsed-JSON conversions.

The schema also defines MCPRelationSchema for MCP tool connections and exports serializeAgentForm() to convert API responses into form defaults. FullAgentDefinitionSchema and ContextConfigApiInsertSchema are deleted from client-exports.ts.

validation.ts · client-exports.ts


agents-core schema tightening and shared refs

Before: OpenAPI schemas used inline string/number definitions and loose anyOf unions; FunctionCreate was a standalone component; several fields accepted z.any().
After: Fields reference shared schemas (ResourceIdSchema, NameSchema, DescriptionSchema, StringRecord), counts use z.int(), z.any() replaced with z.unknown() / z.record(), and FunctionCreate is inlined at usage sites.

Change Fields affected
numberinteger numEvents, timeInSeconds, stepCountIs, transferCountIs
Inline → shared ref name, description, headers across Sub/Tool/Function/TeamAgent schemas
z.any() → typed providerOptions, headersSchema, contextVariables
Added .trim() model, prompt, executeCode, ResourceIdSchema

schemas.ts · openapi.json


Serialization rewrite — serialize.ts and deserialize.ts

Before: serializeAgentData(nodes, edges, metadata) carried agent-level fields (models, prompt, contextConfig, stopWhen) through a metadata bag and performed JSON-parse validation inline.
After: serializeAgentData(nodes, edges, mcpRelations?, functionToolFormData?, ...) only produces subAgents, functions, and functionTools. Agent-level fields go directly from form data. A new hydrateNodesWithFormData() merges RHF state into node objects before serialization.

deserialize.ts input type narrowed from FullAgentDefinition to AgentGraphData. Removed isDefault from node data (default sub-agent is now derived from useWatch on defaultSubAgentId).

serialize.ts · deserialize.ts · serialize.test.ts


Error handling — from custom parser to formState.errors

Before: A 312-line agent-error-parser.ts converted zod issues to user-friendly messages with custom category mapping and node-error lookups via Zustand.
After: Errors derive directly from formState.errors. useGroupedAgentErrors destructures them into semantic groups (subAgents, functionTools, tools, agentSettings). useProcessedErrors extracts per-node errors for ErrorIndicator badges.

How do server-side errors integrate? On a 422 response, the submit handler parses the body as z.ZodIssue[] and calls form.setError() for each issue path, so backend validation errors appear inline alongside client-side ones.

agent-error-summary.tsx · use-grouped-agent-errors.ts · agent-error-parser.ts (deleted)


Node editors — from useNodeEditor to RHF field paths

Before: Each node editor used useNodeEditor() for imperative updateField/updatePath helpers, local useState synced from selectedNode.data, and manual JSON parse/validate loops.
After: Editors use useFullAgentFormContext() with typed field paths (e.g., `subAgents.${id}.name`). Generic components (GenericInput, GenericJsonEditor, etc.) auto-bind to control + name. No local state or manual markUnsaved().

useNodeEditor.ts (161 lines) is deleted entirely. useDeleteNode replaces its delete logic. useAutoPrefillId replaces useAutoPrefillIdZustand with a form-aware implementation.

sub-agent-node-editor.tsx · metadata-editor.tsx · use-node-editor.ts (deleted)

Pullfrog  | View workflow run | Using Claude Code | Triggered by Pullfrogpullfrog.com𝕏

Copy link
Copy Markdown
Contributor

@claude claude Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

PR Review Summary

(2) Total Issues | Risk: Medium

Delta review — scoped to changes since commit 1f0ef814ba74. The delta contains 3 commits (primarily merging main and one squashed refactor commit). This review verifies the status of previously identified issues and checks for new issues in delta changes.

🟠⚠️ Major (2) 🟠⚠️

Both MAJOR issues below were identified in prior reviews (Feb 10, 35+ days ago) and remain unresolved in the current code:

🟠 1) unsaved-changes-dialog.tsx:51-60 Navigation proceeds after failed save — data loss risk

Issue: Line 56 uses isValid (captured at render time from useFormState) to decide whether to proceed with navigation after await onSubmit(). This checks form validation state, not whether the submission actually succeeded. If the form passes client-side validation but the server rejects the save (network error, server validation error, permission denied), isValid remains true and users are navigated away.

Why: This is a confirmed data loss risk validated by Ito E2E test ROUTE-7. Users click "Save changes", the API fails, and they're navigated away believing their changes were saved. The dialog closes unconditionally (line 59).

Fix: Use isSubmitSuccessful from form state after submission:

async function handleSaveAndLeave() {
  if (isSubmitting) return;
  await onSubmit();
  if (form.formState.isSubmitSuccessful) {
    proceedWithNavigation();
  }
  setShowUnsavedDialog(false);
}

Refs:


🟠 2) validation.ts:255-262 Status update toggles don't persist disabled state

Issue: The serializeAgentForm function at lines 258-259 forces numEvents and timeInSeconds to default values (10 and 30) via nullish coalescing (??) even when they should be undefined to represent a disabled toggle state. The UI in metadata-editor.tsx uses checked={numEvents !== undefined} to determine checkbox state, so rehydrating with forced defaults makes both toggles appear enabled after reload.

Why: This is the root cause of Ito test failure LOGIC-4: "After disabling one status update mode and saving, reload restored both modes enabled." Users intentionally disable a mode, save, reload, and find both modes re-enabled.

Fix: Preserve undefined during serialization:

statusUpdates: {
  ...statusUpdates,
  enabled: statusUpdates.enabled ?? false,
  numEvents: statusUpdates.numEvents, // Don't force default
  timeInSeconds: statusUpdates.timeInSeconds, // Don't force default
  prompt: statusUpdates.prompt ?? '',
  statusComponents: serializeJson(statusUpdates.statusComponents),
},

Refs:

🟡 Minor (1) 🟡

Inline Comments:

  • 🟡 Minor: page.client.tsx:668 Error parsing catch block loses original error string for debugging

💭 Consider (1) 💭

Inline Comments:

  • 💭 Consider: generic-checkbox.tsx:15 Local FormFieldWrapperProps duplicates shared type from peer components

🕐 Pending Recommendations (2)

Both MAJOR issues above were originally raised in prior automated reviews and have persisted across 8+ review cycles spanning 35+ days:

✅ Previously Resolved

The following issues from earlier reviews were addressed in prior commits:

  1. Missing shouldDirty: true in setValue calls — ✅ All setValue calls now include { shouldDirty: true }
  2. Error message references wrong component name — ✅ Error message now correctly references <FullAgentFormProvider />
  3. Subscription cleanup in context-config.tsx — ✅ Now returns cleanup function from useEffect (line 23)

ℹ️ Ito E2E Test Status

From the most recent test run (March 14):

Test Status Issue
LOGIC-1 ❌ Failed Prompt autocomplete missing context/header suggestions — initial state not populated on page load

Earlier test runs confirmed:

  • LOGIC-4 ❌ — Status update toggles persistence (introduced by this PR — addressed in Major #2)
  • ROUTE-7 ❌ — Save-and-leave with validation errors (introduced by this PR — addressed in Major #1)

🚫 REQUEST CHANGES

Summary: The delta from merging main contains minimal PR-specific code changes. However, two MAJOR issues identified in prior reviews remain unresolved after 35+ days:

  1. Data loss riskhandleSaveAndLeave uses isValid (render-time validation state) instead of isSubmitSuccessful (actual submission result). If the API call fails, users are navigated away thinking their changes were saved.

  2. Status update toggle persistence — Serialization forces defaults for numEvents and timeInSeconds, overwriting the undefined state that represents a disabled toggle. Users disable a mode, save, reload, and find it re-enabled.

Both issues have been validated by Ito E2E tests and have concrete fix suggestions above. Addressing these before merge will prevent user-facing bugs that cause data loss and confusing UX.

The overall refactoring architecture is sound — migrating from Zustand to react-hook-form with Zod validation is a good direction that improves code organization and leverages established patterns. These two fixes are the final blockers.


Discarded (8)
Location Issue Reason Discarded
validation.ts:209-351 serializeAgentForm missing unit tests Existing serialize.test.ts covers serialization logic; incremental test improvement, not a blocker
validation.ts:138-148 FullAgentSubAgentSchema untested Schema composition is tested indirectly via integration; LOW severity
validation.ts:95-102 MCPRelationSchema untested Schema used in serialize.test.ts; coverage exists at integration level
validation.ts:104-117 Function tool schemas untested Similar — integration coverage exists
validation.ts:302-313 @ts-expect-error for headers Pre-existing pattern, not introduced by this PR's core changes
node-types.tsx:42-62 Node data interfaces weakened Intentional design — form state now lives in react-hook-form, not node data
validation.ts:99 selectedTools nullable.optional Valid design — null = all tools, undefined = not specified
validation.ts:56-186 Mixed z.strictObject vs z.object Intentional — strictObject provides better validation for form inputs
Reviewers (7)
Reviewer Returned Main Findings Consider While You're Here Inline Comments Pending Recs Discarded
pr-review-standards 1 0 0 0 0 1 0
pr-review-frontend 1 0 0 0 0 1 0
pr-review-errors 3 0 0 0 1 1 1
pr-review-tests 4 0 0 0 0 0 4
pr-review-types 5 0 0 0 0 2 3
pr-review-precision 2 0 0 0 0 2 0
pr-review-consistency 3 0 1 0 1 0 1
Total 19 0 1 0 2 7 9

Note: Main findings count is 0 because both MAJOR issues were already raised in prior reviews and are tracked as Pending Recommendations.

issues.forEach(({ path, code, message }) => {
form.setError(path.join('.') as any, { type: code, message });
});
} catch (parseError) {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 MINOR: Error parsing catch block loses original error string

Issue: When JSON.parse(res.error) fails at line 664, the catch block logs 'Failed to parse validation errors' with only parseError but doesn't include res.error in the log, losing the original malformed error string.

Why: When debugging parse failures, engineers will see a SyntaxError but won't know what string was being parsed. The original res.error value is lost.

Fix: Include the original error string:

} catch (parseError) {
  console.error('Failed to parse validation errors:', parseError, 'Original error:', res.error);
  toast.error('Failed to save agent', { closeButton: true });
}

Refs:

FormMessage,
} from '@/components/ui/form';

export interface FormFieldWrapperProps<
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💭 CONSIDER: Local FormFieldWrapperProps duplicates shared type

Issue: This component defines FormFieldWrapperProps locally (lines 15-25) while peer components (GenericCodeEditor, GenericJsonEditor, GenericPromptEditor) import from ./form-field-wrapper.

Why: Duplicate type definitions can drift over time and create inconsistencies.

Fix: Import the shared type:

import { type FormFieldWrapperProps } from './form-field-wrapper';

interface GenericCheckboxProps<TFieldValues extends FieldValues>
  extends Omit<FormFieldWrapperProps<TFieldValues>, 'children'> {
  // ... checkbox-specific props
}

Refs:

Copy link
Copy Markdown
Contributor

@claude claude Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

PR Review Summary

(0) Total Issues | Risk: Medium

Delta review — scoped to changes since commit 720f3335eece. The delta contains 1 commit (fix inkeep pull tests) affecting 11 files — all test fixture updates and a minor schema ordering fix in schemas.ts.

✅ Delta Assessment

The delta is clean. Changes are limited to:

Category Files Changed Impact
CLI test fixtures 10 Updated snapshots to use explicit sub-agent names instead of empty strings
Schema ordering 1 Moved .openapi() call after .omit() in FunctionApiInsertSchema

No new issues introduced in the delta. The test fixture updates align with the schema change that now requires non-empty name fields for sub-agents.

🕐 Pending Recommendations (2)

Both issues below were identified in prior reviews (35+ days ago) and remain unresolved in unchanged code:

  • 🟠 unsaved-changes-dialog.tsx:51-60 Navigation proceeds after failed save — uses isValid (render-time validation state) instead of isSubmitSuccessful to decide navigation after save. If API call fails, users are navigated away believing changes were persisted. Data loss risk.

  • 🟠 validation.ts:255-262 Status update toggles don't persist disabled stateserializeAgentForm forces numEvents: 10 and timeInSeconds: 30 defaults even when undefined (disabled). On reload, disabled toggles appear re-enabled.

Note: These issues have been flagged in 8 consecutive automated reviews since Feb 10 (35+ days). The fixes are straightforward:

  1. unsaved-changes-dialog.tsx:56: Change if (isValid)if (form.formState.isSubmitSuccessful)
  2. validation.ts:258-259: Change statusUpdates.numEvents ?? 10statusUpdates.numEvents (preserve undefined)

🚫 REQUEST CHANGES

Summary: The delta is clean — test fixtures and schema ordering only. However, 2 MAJOR issues from prior reviews remain unresolved after 35+ days. The isValid vs isSubmitSuccessful bug is a confirmed data loss risk that should be addressed before merge. Both fixes are ~1-line changes.

Discarded (0)

No findings discarded.

Reviewers (9)
Reviewer Returned Main Findings Consider While You're Here Inline Comments Pending Recs Discarded
pr-review-frontend 0 0 0 0 0 0 0
pr-review-standards 0 0 0 0 0 0 0
pr-review-errors 0 0 0 0 0 0 0
pr-review-types 0 0 0 0 0 0 0
pr-review-tests 0 0 0 0 0 0 0
pr-review-precision 0 0 0 0 0 0 0
pr-review-product 0 0 0 0 0 0 0
pr-review-consistency 0 0 0 0 0 0 0
pr-review-docs 0 0 0 0 0 0 0
Total 0 0 0 0 0 0 0

Note: All 9 reviewers reported no new issues in the delta. The delta only contains test fixture updates and minor schema ordering — no substantive code changes to review.

@itoqa
Copy link
Copy Markdown

itoqa Bot commented Mar 16, 2026

Ito Test Report ❌

12 test cases ran. 11 passed, 1 failed.

✅ Most validated flows worked as expected (routing baseline, save/reload metadata, validation/autocomplete coverage, unsaved-changes behavior, pruning behavior, mobile usability, and adversarial resilience checks). 🔍 One real defect remains: editing a sub-agent name can silently rewrite that sub-agent's ID, which breaks deep-link identity stability after save/reload.

✅ Passed (11)
Test Case Summary Timestamp Screenshot
ROUTE-1 Seeded agent page rendered with graph and core toolbar controls, Agent Settings preserved pane context, and selecting the known sub-agent updated URL query to pane=node with matching nodeId. 35:53 ROUTE-1_35-53.png
ROUTE-2 Description and agent prompt edits saved successfully and persisted after reload. 7:48 ROUTE-2_7-48.png
LOGIC-4 After setting context variables and headers schema keys, prompt autocomplete showed contextVariablesValue, headers.testHeadersJsonSchemaValue, and $env. as expected. 15:34 LOGIC-4_15-34.png
LOGIC-5 Dirty-state navigation triggered unsaved dialog; Discard navigated to /default/projects immediately, and Save changes persisted then navigated with success feedback. 19:44 LOGIC-5_19-44.png
EDGE-1 Deletion attempts against the default sub-agent were prevented: keyboard delete had no effect, delete UI was unavailable, and default configuration remained intact. 19:44 EDGE-1_19-44.png
EDGE-2 Newly added unconnected Function Tool/MCP nodes disappeared after save+reload, while existing seeded connected graph nodes persisted. 19:44 EDGE-2_19-44.png
EDGE-5 At 390x844, required-field validation surfaced in the validation panel and critical controls remained operable without obstructive blocking behavior. 19:44 EDGE-5_19-44.png
ADV-1 Abusive prompt content was handled as editor content and the page remained stable through save with no execution takeover observed. 24:51 ADV-1_24-51.png
ADV-2 Malformed and suspicious custom-header payloads were exercised in the dialog and UI remained responsive without failure. 24:51 ADV-2_24-51.png
ADV-3 High-frequency graph edits and undo/redo cycles completed, then reload succeeded with the editor still operational. 24:51 ADV-3_24-51.png
ADV-4 Concurrent saves across two tabs completed and both tabs reloaded successfully with understandable persisted state behavior. 24:51 ADV-4_24-51.png
❌ Failed (1)
Test Case Summary Timestamp Screenshot
ROUTE-3 Field edits persisted, but the deep-link target identity drifted after save/reload (nodeId changed), so deep-link stability expectation failed. 7:48 ROUTE-3_7-48.png
Sub-agent pane edits persist via hydrate + serialize path – Failed
  • Where: Sub-agent editor deep-link flow (pane=node&nodeId=...) on the agent builder page.

  • Steps to reproduce: Open an existing sub-agent via deep-link, edit the sub-agent name, save, then reload the same deep-link.

  • What failed: The sub-agent content persists, but the sub-agent ID is auto-regenerated from name edits, so the original deep-link nodeId no longer points to the same entity.

  • Code analysis: The sub-agent editor unconditionally enables ID auto-prefill with isEditing: false, and the hook rewrites id whenever name changes if the ID field is not already dirty. During serialization/deserialization, graph node identity is keyed by sub-agent ID, so this rewrite changes the node key and breaks deep-link stability.

  • Relevant code:

    agents-manage-ui/src/components/agent/sidepane/nodes/sub-agent-node-editor.tsx (lines 83-88)

    useAutoPrefillId({
      form,
      nameField: path('name'),
      idField: path('id'),
      isEditing: false,
    });

    agents-manage-ui/src/hooks/use-auto-prefill-id.ts (lines 35-39)

    useEffect(() => {
      if (!isEditing && nameValue && !isIdFieldModified) {
        const generatedId = generateIdFromName(nameValue);
        form.setValue(idField, generatedId as any, { shouldValidate: true });
      }
    }, [nameValue, idField, isEditing]);

    agents-manage-ui/src/features/agent/domain/deserialize.ts (lines 129-131 and 190-192)

    const subAgentIds: string[] = Object.keys(data.subAgents);
    for (const subAgentId of subAgentIds) {
      const subAgent = data.subAgents[subAgentId];
    
      const agentNode: Node = {
        id: subAgentId,
        type: nodeType,
  • Why this is likely a bug: Existing sub-agent identity should remain stable unless the user explicitly changes ID, but the current logic mutates ID implicitly from name edits and breaks canonical deep-link routing.

  • Introduced by this PR: Yes – this PR modified the relevant code.

  • Timestamp: 7:48

📋 View Recording

Screen Recording

Copy link
Copy Markdown
Contributor

@claude claude Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

PR Review Summary

(4) Total Issues | Risk: Medium

Delta review — scoped to changes since commit 2b65e3688e43. The delta contains 84 commits, most from merging main. PR-specific changes include new syncSavedAgentGraph.ts, validation schema updates, and React Query migration.

🟠⚠️ Major (2) 🟠⚠️

🟠 1) validation.test.ts Missing test coverage for duplicate sub-agent ID validation

Issue: The new .superRefine() at validation.ts:169-189 validates that sub-agent IDs are unique across all nodes, but this critical validation logic has zero test coverage.

Why: If a regression is introduced in the duplicate detection logic (e.g., off-by-one in the loop, incorrect path construction for error messages), the system would silently accept duplicate sub-agent IDs. Users could create agents with conflicting identifiers, causing runtime errors or data corruption when the backend processes them.

Fix: Add test case to validation.test.ts:

it('should reject duplicate sub-agent IDs', () => {
  const result = FullAgentUpdateSchema.safeParse({
    ...createSchema(''),
    subAgents: {
      'node-1': { id: 'same-id', name: 'A', type: 'internal', ... },
      'node-2': { id: 'same-id', name: 'B', type: 'internal', ... }
    }
  });
  expect(result.success).toBe(false);
  expect(result.error.issues).toContainEqual(
    expect.objectContaining({
      path: ['subAgents', 'node-1', 'id'],
      message: expect.stringContaining('must be unique')
    })
  );
});

Refs:


🟠 2) validation.test.ts Missing test coverage for defaultSubAgentId transform

Issue: The .transform() at validation.ts:211-217 maps defaultSubAgentId from the node key to the actual sub-agent ID via value.subAgents?.[defaultSubAgentId]?.id. This critical ID resolution logic has no test coverage.

Why: If the transform logic regresses (typo in property access, or optional chaining produces undefined unexpectedly), the defaultSubAgentId could become undefined after transformation, breaking agent configuration. The refine validates input, not the transformed output.

Fix: Add test cases:

it('should transform defaultSubAgentId to the actual sub-agent id', () => {
  const result = FullAgentUpdateSchema.safeParse({
    ...createSchema(''),
    defaultSubAgentId: 'node-key',
    subAgents: {
      'node-key': { id: 'actual-sub-agent-id', name: 'Test', type: 'internal', ... }
    }
  });
  expect(result.data.defaultSubAgentId).toBe('actual-sub-agent-id');
});

it('should handle defaultSubAgentId when sub-agent ID is renamed', () => {
  // Test when node key differs from the sub-agent id field
});

Refs:

🟡 Minor (1) 🟡

Inline Comments:

  • 🟡 Minor: features/agent/domain/index.ts:2 Missing barrel export for syncSavedAgentGraph

💭 Consider (4) 💭

💭 1) sync-saved-agent-graph.test.ts Missing edge case: tool nodes referencing non-existent sub-agents
Issue: If a tool node references a sub-agent ID that the backend didn't return (e.g., concurrent deletion), the node is kept with a stale subAgentId.
Why: Could cause UI inconsistency where tools appear connected to deleted sub-agents.
Fix: Add test verifying orphaned tool nodes are properly filtered.

💭 2) sync-saved-agent-graph.test.ts Missing edge case: multiple tools claiming same relation ID
Issue: The claimRelationId function implements first-come-first-served allocation, but this deduplication behavior isn't tested.
Why: If logic regresses, multiple nodes could receive the same relationshipId.
Fix: Add test verifying unique relation ID assignment.

💭 3) sync-saved-agent-graph.test.ts Missing edge case: edgeId pointing to removed edges
Issue: The test covers nodeId becoming null when dropped, but not the edgeId path.
Fix: Add test: it('clears edgeId when the selected edge is removed', ...)

💭 4) validation.ts:211-217 Schema transform performs business logic
Issue: The transform derives defaultSubAgentId from sub-agent lookup. In agents-core, schema transforms typically handle coercion (e.g., transformToJson), not ID resolution.
Why: Introduces divergence where validation schemas perform business logic.
Fix: Consider extracting to serialization pipeline or adding clarifying comment.

🕐 Pending Recommendations (1)

  • 🟠 unsaved-changes-dialog.tsx:56 Navigation proceeds after failed save — uses isValid instead of isSubmitSuccessful (raised Feb 10, 43+ days ago)

🚫 REQUEST CHANGES

Summary: The delta introduces valuable improvements (React Query migration, duplicate sub-agent ID validation, graph sync logic), but the new validation logic (superRefine for duplicate IDs, transform for defaultSubAgentId) lacks test coverage. These are critical paths that affect agent configuration correctness. Additionally, the 43-day-old MAJOR issue (isValid vs isSubmitSuccessful in save-and-leave flow) remains unaddressed — this is a confirmed data loss risk where users navigate away after failed saves.

Blocking items:

  1. Add tests for duplicate sub-agent ID validation
  2. Add tests for defaultSubAgentId transform
  3. Fix the save-and-leave navigation issue (change isValid to form.formState.isSubmitSuccessful)
Discarded (2)
Location Issue Reason Discarded
sub-agent-node-editor.tsx:77-78 isPersistedSubAgent timing with renamed IDs LOW confidence finding — the form.reset() is called synchronously after save, so timing is likely not an issue. Edge case requires more investigation.
sync-saved-agent-graph.ts:1-158 Function not wired up to any component The function IS being used — it's imported and tested. The inline synchronization in page.client.tsx is a different approach, not duplication. Both may be intentional for different use cases.
Reviewers (4)
Reviewer Returned Main Findings Consider While You're Here Inline Comments Pending Recs Discarded
pr-review-frontend 4 0 1 0 0 1 2
pr-review-standards 0 0 0 0 0 0 0
pr-review-tests 5 2 3 0 0 0 0
pr-review-consistency 6 0 0 0 1 0 0
Total 15 2 4 0 1 1 2

Note: pr-review-consistency returned 3 INFO findings confirming patterns are consistent — not counted as actionable.

@@ -1,3 +1,2 @@
export * from './agent-initializer-helpers';
export * from './deserialize';
export * from './serialize';
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Minor: Missing barrel export for syncSavedAgentGraph

Issue: The new syncSavedAgentGraph function follows the same domain function pattern as deserializeAgentData and serializeAgentData, but isn't exported from this barrel file.

Why: This breaks the established convention where all domain functions are importable from @/features/agent/domain. Consumers would need to import directly from the file path instead.

Fix: (1-click apply)

Suggested change
export * from './serialize';
export * from './deserialize';
export * from './serialize';
export * from './sync-saved-agent-graph';

Refs:

@pullfrog
Copy link
Copy Markdown
Contributor

pullfrog Bot commented Mar 24, 2026

TL;DR — Replaces the imperative zustand-based form/validation/error state on the agent page with a declarative react-hook-form + zodResolver architecture, and introduces a deterministic graph identity system that decouples stable domain keys from volatile React Flow IDs. Form state, dirty tracking, field-level validation, error display, and node identity all flow through a single FullAgentFormProvider backed by Zod schemas from agents-core, while node.data is stripped to a thin identity envelope — eliminating data duplication, ~300 net lines, and several custom hooks.

Key changes

  • Introduce FullAgentFormProvider with zodResolver — a React context wraps the agent page, wiring useForm() to FullAgentFormSchema in mode: 'onChange' so every field is schema-validated as the user types
  • Add deterministic graph key system — new graph-keys.ts, graph-identity.ts, sub-agent-identity.ts, and function-tool-identity.ts modules replace unstable generateId() UUIDs with stable domain-meaningful identifiers derived from persisted IDs
  • Strip node.data to thin identity envelopes — all business fields (name, description, prompt, models, code, etc.) removed from node data; nodes store only nodeKey + minimal refs; editors read exclusively from RHF via useWatch()
  • Collapse connection state into mcpRelations and functionToolRelations RHF record maps — replaces scattered tempSelectedTools/tempHeaders/tempToolPolicies on node data with factory-created form entries
  • Replace serializeAgentData with editorToPayload — reads business data directly from SerializeAgentFormState with requireFormValue() fail-fast guards; hydrateNodesWithFormData() deleted entirely
  • Expand syncSavedAgentGraph for full post-save reconciliation — reconciles server-assigned IDs for tool canUse relations, external/team agent delegate relations; renames node IDs to deterministic graph keys
  • Slim the zustand store to graph-only concerns — metadata, errors, and show-errors state removed; store holds only nodes, edges, undo/redo history, and UI flags
  • Refactor error display to useFormState + useGroupedAgentErrors — groups errors by category and hides the panel when an editable input is focused
  • Fix function tool "requires approval" not persisting — hydrate needsApproval from canUse relations back into form state during apiToFormValues()
  • Fix O(N²) re-renders in function tool nodes — targeted useWatch per node replaces watching the entire functionTools map
  • Add 7 new test files — graph identity, function tool identity, form-state defaults, roundtrip, and sync-saved-agent-graph scenarios
  • Update Cypress selectors — label-based selectors replace #id selectors; validation errors assert against .react-flow__panel
  • Fix CLI pull-v4 snapshots and minor API type changesTeamRelation.targetAgent.description becomes optional; defaultSubAgentId uses $ref to ResourceId

Summary | 128 files | 2747 commits | base: mainprd-5298-2


Deterministic graph identity system

Before: Nodes were identified by random generateId() UUIDs; deep-links broke on re-render/save because IDs were ephemeral. Business data was duplicated between node.data and form state, causing drift.
After: Every node gets a stable nodeKey derived from persisted IDs (e.g. sub-agent ID, relationship:${relationshipId}, mcp:${subAgentId}:${toolId}). URL params store graph keys, surviving saves and re-renders. node.data is a thin envelope — nodeKey plus minimal refs like toolId.

Factory functions (getSubAgentGraphKey, getMcpGraphKey, getFunctionToolGraphKey, getExternalAgentGraphKey, getTeamAgentGraphKey) produce deterministic keys. findNodeByGraphKey() and getNodeGraphKey() provide the lookup API. Edge keys are canonicalized as a2a:sourceNodeKey:targetNodeKey.

Why separate node.id from nodeKey?

React Flow's node.id is ephemeral — it can be a temporary UUID for unsaved nodes. After save, syncSavedAgentGraph renames IDs to match server-assigned relationships. The nodeKey in node.data is the stable domain identity that URL params, form field paths, and edge lookups reference. This separation lets the graph reconcile without breaking open editors or navigation state.

graph-keys.ts · graph-identity.ts · node-types.tsx · sub-agent-identity.ts


FullAgentFormProvider and schema-driven validation

Before: Agent metadata lived in a zustand store with manual setMetadata calls; validation ran imperatively via validateSerializedData() at submit time, returning StructuredValidationError[] parsed and set via setErrors.
After: A FullAgentFormProvider wraps the entire agent page, providing a useForm() instance with zodResolver(FullAgentFormSchema) in mode: 'onChange'. Field-level errors surface immediately as the user types, and server-side errors map directly to form fields via form.setError.

The FullAgentFormSchema composes sub-schemas from agents-core with custom transforms — StringToJsonSchema for editor fields, NullToUndefinedSchema for <input type="number"> edge cases, and .superRefine for cross-field validations like duplicate sub-agent IDs. A new apiToFormValues() function replaces extractAgentMetadata() for initial hydration, merging tool policies, headers, and relation data into the flat form shape.

How does the save flow work now?

form.handleSubmiteditorToPayload() (reads form data via requireFormValue() + graph nodes for structure) → updateFullAgentAction() → on success: syncSavedAgentGraph() reconciles IDs → form.reset(apiToFormValues(res.data)) resets dirty tracking → on validation error: server errors mapped to form.setError(path, { type, message }).

full-agent-form.tsx · validation.ts · page.client.tsx


Connection state collapsed into RHF record maps

Before: MCP tool selection, headers, and tool policies lived as tempSelectedTools/tempHeaders/tempToolPolicies fields on node.data, requiring manual sync between node data and serialization. Function tool approval state was similarly scattered.
After: mcpRelations and functionToolRelations are RHF record maps keyed by getMcpRelationFormKey()/getFunctionToolRelationFormKey(). Factory helpers (createMcpRelationFormInput, createFunctionToolRelationFormInput) produce well-typed defaults. Edge removal triggers synchronous form.unregister() — only relationshipId is unregistered for MCP relations to avoid a race condition.

Record map Key derivation Fields
mcpRelations getMcpRelationFormKey({ nodeId }) selectedTools, toolPolicies, headers, relationshipId
functionToolRelations getFunctionToolRelationFormKey(nodeKey) needsApproval, relationshipId

form-state-defaults.ts · serialize.ts · mcp-node-editor.tsx


Post-save graph sync and simplified serialization

Before: serializeAgentData returned a full FullAgentDefinition including metadata, calling hydrateNodesWithFormData() to merge form data back into nodes before reading node.data. The saveAgent service branched between create and update.
After: editorToPayload() reads all business data from a SerializeAgentFormState bundle with requireFormValue() fail-fast guards — no hydration step. After save, syncSavedAgentGraph reconciles three categories of server-assigned IDs: tool canUse relations, external agent delegate relations, and team agent delegate relations. Node IDs are renamed to deterministic graph keys; URL selection state is preserved via findNodeByGraphKey.

The saveAgent service and createFullAgent client function are deleted — the page always calls updateFullAgentAction directly.

serialize.ts · sync-saved-agent-graph.ts · deserialize.ts


Zustand store slimmed to graph-only state

Before: useAgentStore held metadata, errors, showErrors plus actions like setMetadata, setErrors, clearErrors, getNodeErrors, getEdgeErrors, deleteSelected.
After: The store holds only nodes, edges, dirty, history/future (undo/redo), and UI flags (isSidebarSessionOpen, hasOpenModelConfig, jsonSchemaMode). All form-related state moved to react-hook-form.

Dirty tracking combines agentDirtyState from zustand (node positions, graph structure) with isDirty from useFormState — both must be false for the page to be considered "clean." The onEdgesChange handler now synchronously calls form.unregister() for removed edges instead of deferred requestAnimationFrame cleanup.

use-agent-store.ts · unsaved-changes-dialog.tsx · use-animate-graph.ts


Error display driven by useFormState

Before: Errors were parsed by agent-error-parser, stored in zustand via setErrors, and displayed via AgentErrorSummary which received errorSummary, onClose, onNavigateToEdge props.
After: AgentErrorSummary reads isSubmitted from useFormState and groups errors via useGroupedAgentErrors (categorizing by subAgents, functionTools, externalAgents, teamAgents, tools, agentSettings). A useWindowFocus hook hides the panel when the user is actively editing an input.

The flatNestedFieldMessage utility recursively extracts messages from nested react-hook-form error objects, joining them with separators for readable display.

agent-error-summary.tsx · use-grouped-agent-errors.ts · error-indicator.tsx

Pullfrog  | View workflow run | Triggered by Pullfrog | Using Claude Opus𝕏

@itoqa
Copy link
Copy Markdown

itoqa Bot commented Mar 24, 2026

Ito Test Report ❌

20 test cases ran. 7 failed, 13 passed.

Overall, 20 test cases ran with 13 passing and 7 failing, and all failures were high-severity blockers tied to a single production defect introduced by this PR: SubAgentNode assumes a model string and calls modelName.split(...) even though model fields are optional, causing deterministic crash/error-boundary behavior when model resolution is undefined. Despite that central crash breaking core editor access across agent and node routes (including deep links, invalid-link self-heal paths, unsaved save-and-leave/race/mobile interruption flows, and default settings load), the rest of the covered functionality passed when reachable, including graph integrity behaviors (default sub-agent init, duplicate ID validation with correct Go-to targeting, rename relation persistence, default-node delete protection, MCP disconnect cleanup), metadata/prompt/JSON validation flows, rapid submit stability, inert handling of payload-like text, and rejection of malicious/reserved IDs with acceptance of valid URL-safe IDs.

❌ Failed (7)
Category Summary Screenshot
Adversarial ⚠️ Rapid unsaved-dialog toggle scenario was blocked by repeated crash/error-boundary states. ADV-2
Adversarial ⚠️ Mobile interruption journey failed because pane transitions repeatedly crashed the agent page. ADV-6
Edge ⚠️ Agent editor crashes when sub-agent model resolution produces undefined and renderer calls string operations. EDGE-1
Edge ⚠️ Invalid deep-link query path crashes instead of self-healing to a stable pane. EDGE-5
Happy-path ⚠️ Agent settings route crashes into error boundary instead of rendering RHF-backed defaults pane. ROUTE-1
Happy-path ⚠️ Node deep-link route crashes before sub-agent editor and prompt state can be verified. ROUTE-2
Happy-path ⚠️ Save-and-leave flow could not run because the agent page crashed before dialog completion. ROUTE-6
⚠️ Unsaved dialog race under rapid action toggles
  • What failed: Instead of deterministic dialog handling, the page repeatedly fell into crash/error-boundary states before race-condition outcomes could be validated.
  • Impact: Users hitting rapid interaction patterns can be forced into unstable editor states and cannot trust save/discard behavior. This undermines reliability of interruption safety in an already risky workflow.
  • Introduced by this PR: Yes – this PR modified the relevant code
  • Steps to reproduce:
    1. Open /default/projects/activities-planner/agents/activities-planner?pane=node&nodeId=get-coordinates-agent.
    2. Create dirty state and open Unsaved Changes via an attempted navigation.
    3. Rapidly alternate dialog/navigation actions (Go back, reopen, Discard, Save changes).
    4. Observe recurrent crash/error-boundary states instead of deterministic dialog outcomes.
  • Code analysis: The same node-rendering path is exercised during pane updates and re-renders in this scenario. The unchecked modelName.split call can throw whenever model resolution yields undefined, causing the recurrent crashes observed during rapid UI transitions.
  • Why this is likely a bug: The crash mechanism is consistent with a deterministic null-handling defect in production render code, not with a one-off test harness artifact.

Relevant code:

agents-manage-ui/src/components/agent/nodes/sub-agent-node.tsx (lines 72-77)

const isDefault = id === defaultSubAgentId;
const projectModel = project?.models;
const modelName =
  subAgent.models?.base.model ?? agentModel.base.model ?? (projectModel?.base.model as string);

const dataComponentsById = createLookup(dataComponents);

agents-manage-ui/src/components/agent/nodes/sub-agent-node.tsx (lines 88-94)

const [modelSlug] = modelName.split('/', 1);

const ModelIcon = {
  openai: OpenAIIcon,
  anthropic: AnthropicIcon,
  google: GoogleIcon,
}[modelSlug];

agents-manage-ui/src/components/agent/configuration/agent-types.tsx (lines 1-9)

type ModelSettings = {
  model?: string;
  providerOptions?: string; // JSON string representation for form compatibility
};

export type AgentModels = {
  base?: ModelSettings;
  structuredOutput?: ModelSettings;
  summarizer?: ModelSettings;
};
⚠️ Mobile viewport + navigation interruption journey
  • What failed: The editor repeatedly crashed into error boundaries before mobile interruption checks could complete, preventing validation of tappable save/discard controls.
  • Impact: Mobile users can be blocked from completing critical edit and navigation-protection flows due to hard crashes. This creates elevated risk of abandoned edits and inconsistent state handling on small screens.
  • Introduced by this PR: Yes – this PR modified the relevant code
  • Steps to reproduce:
    1. Resize viewport to 390x844 while on the agent node pane.
    2. Create dirty state in the node form.
    3. Trigger interruption actions (back, forward, refresh, pane transitions) that should preserve Unsaved Changes protection.
    4. Observe repeated page crashes/error boundaries before dialog checks can complete.
  • Code analysis: Mobile transitions still render the same sub-agent nodes, so the unsafe model-name dereference remains active across viewport modes. The code path can throw regardless of desktop/mobile once a node resolves without a concrete model string.
  • Why this is likely a bug: The failing behavior matches a direct production-code null dereference that is independent of viewport and reproducible under normal UI transitions.

Relevant code:

agents-manage-ui/src/components/agent/nodes/sub-agent-node.tsx (lines 72-77)

const isDefault = id === defaultSubAgentId;
const projectModel = project?.models;
const modelName =
  subAgent.models?.base.model ?? agentModel.base.model ?? (projectModel?.base.model as string);

const dataComponentsById = createLookup(dataComponents);

agents-manage-ui/src/components/agent/nodes/sub-agent-node.tsx (lines 88-94)

const [modelSlug] = modelName.split('/', 1);

const ModelIcon = {
  openai: OpenAIIcon,
  anthropic: AnthropicIcon,
  google: GoogleIcon,
}[modelSlug];

agents-manage-ui/src/components/agent/configuration/agent-types.tsx (lines 1-9)

type ModelSettings = {
  model?: string;
  providerOptions?: string; // JSON string representation for form compatibility
};

export type AgentModels = {
  base?: ModelSettings;
  structuredOutput?: ModelSettings;
  summarizer?: ModelSettings;
};
⚠️ Agent editor crashes when sub-agent model is unresolved
  • What failed: The page hits a runtime exception while rendering sub-agent nodes, so the editor crashes instead of showing a stable form where dirty-state reset behavior can be validated.
  • Impact: This blocks editing and validation workflows for affected agents because the page crashes before user actions can proceed. Teams cannot reliably verify or save agent configuration when model inheritance is incomplete.
  • Introduced by this PR: Yes – this PR modified the relevant code
  • Steps to reproduce:
    1. Navigate to the activities-planner agent page with pane=agent.
    2. Load a sub-agent whose effective model is not resolved from sub-agent, agent, or project-level inheritance.
    3. Allow the canvas to render SubAgentNode cards.
    4. Observe runtime failure instead of a stable editor.
  • Code analysis: The sub-agent node renderer assumes model data is always present and performs unguarded property access and string splitting. The same PR also made model fields optional in shared node typing, so undefined model values are expected and must be handled defensively.
  • Why this is likely a bug: Production UI code allows model values to be optional but still dereferences and splits them as guaranteed strings, creating a deterministic runtime crash path.

Relevant code:

agents-manage-ui/src/components/agent/nodes/sub-agent-node.tsx (lines 72-76)

const isDefault = id === defaultSubAgentId;
const projectModel = project?.models;
const modelName =
  subAgent.models?.base.model ?? agentModel.base.model ?? (projectModel?.base.model as string);

const dataComponentsById = createLookup(dataComponents);

agents-manage-ui/src/components/agent/nodes/sub-agent-node.tsx (lines 84-89)

const isDelegating = status === 'delegating';
const isInvertedDelegating = status === 'inverted-delegating';
const isExecuting = status === 'executing';

const [modelSlug] = modelName.split('/', 1);

agents-manage-ui/src/components/agent/configuration/agent-types.tsx (lines 1-9)

type ModelSettings = {
  model?: string;
  providerOptions?: string; // JSON string representation for form compatibility
};

export type AgentModels = {
  base?: ModelSettings;
  structuredOutput?: ModelSettings;
  summarizer?: ModelSettings;
};
⚠️ Invalid deep-link query self-heals
  • What failed: Invalid deep links trigger a full crash state because the same sub-agent model rendering bug executes before the route can recover.
  • Impact: Resilience behavior for malformed links is broken, so shared or stale URLs can take users to a dead-end crash page. This increases support burden and lowers confidence in route stability.
  • Introduced by this PR: Yes – this PR modified the relevant code
  • Steps to reproduce:
    1. Open /default/projects/activities-planner/agents/activities-planner?pane=node&nodeId=does-not-exist.
    2. Then open /default/projects/activities-planner/agents/activities-planner?pane=edge&edgeId=missing-edge.
    3. Observe crash overlay instead of self-heal/fallback behavior.
  • Code analysis: The route-level self-heal expectation is preempted by a render-time exception in the node component. Optional model fields and serialized form defaults make undefined model values valid inputs, but the UI does not defensively handle them.
  • Why this is likely a bug: The app throws before fallback logic due to an unguarded production render operation on optional data, so the observed instability is a true code defect.

Relevant code:

agents-manage-ui/src/components/agent/nodes/sub-agent-node.tsx (lines 73-88)

const projectModel = project?.models;
const modelName =
  subAgent.models?.base.model ?? agentModel.base.model ?? (projectModel?.base.model as string);

const dataComponentsById = createLookup(dataComponents);
const artifactComponentsById = createLookup(artifactComponents);

const dataComponentNames =
  dataComponentIds.map((id) => dataComponentsById[id]?.name).filter(Boolean) || [];
const artifactComponentNames =
  artifactComponentIds.map((id) => artifactComponentsById[id]?.name).filter(Boolean) || [];

const [modelSlug] = modelName.split('/', 1);

agents-manage-ui/src/components/agent/form/validation.ts (lines 261-273)

return {
  base: {
    ...models.base,
    providerOptions: serializeJson(models.base?.providerOptions),
  },
  structuredOutput: {
    ...models.structuredOutput,
    providerOptions: serializeJson(models.structuredOutput?.providerOptions),
  },
  summarizer: {
    ...models.summarizer,
    providerOptions: serializeJson(models.summarizer?.providerOptions),
  },
};
⚠️ Load agent settings pane with RHF-backed defaults
  • What failed: The page enters an error boundary because sub-agent model display logic calls .split() on an undefined model string; expected behavior is graceful inherited-model rendering without crashing.
  • Impact: Core agent editing is blocked because the main route crashes before users can safely view or edit settings. This creates a hard stop for authoring and verification flows.
  • Introduced by this PR: Yes – this PR modified the relevant code
  • Steps to reproduce:
    1. Open /default/projects/activities-planner/agents/activities-planner?pane=agent.
    2. Wait for the canvas and side pane render.
    3. Observe crash/error boundary instead of the normal settings view.
  • Code analysis: I reviewed the sub-agent node render path and form serialization/model typing in agents-manage-ui. The node UI assumes a model string exists, but form/model schemas allow it to be absent and serialized defaults can produce empty model objects, making the split call unsafe in normal inherited-model scenarios.
  • Why this is likely a bug: Production UI code dereferences modelName as a definite string despite upstream types/data paths explicitly allowing missing model values, so the crash is caused by application logic rather than test setup.

Relevant code:

agents-manage-ui/src/components/agent/nodes/sub-agent-node.tsx (lines 73-88)

const projectModel = project?.models;
const modelName =
  subAgent.models?.base.model ?? agentModel.base.model ?? (projectModel?.base.model as string);

const dataComponentsById = createLookup(dataComponents);
const artifactComponentsById = createLookup(artifactComponents);

const dataComponentNames =
  dataComponentIds.map((id) => dataComponentsById[id]?.name).filter(Boolean) || [];
const artifactComponentNames =
  artifactComponentIds.map((id) => artifactComponentsById[id]?.name).filter(Boolean) || [];

const [modelSlug] = modelName.split('/', 1);

agents-manage-ui/src/components/agent/configuration/agent-types.tsx (lines 1-4)

type ModelSettings = {
  model?: string;
  providerOptions?: string; // JSON string representation for form compatibility
};

agents-manage-ui/src/components/agent/form/validation.ts (lines 261-273)

return {
  base: {
    ...models.base,
    providerOptions: serializeJson(models.base?.providerOptions),
  },
  structuredOutput: {
    ...models.structuredOutput,
    providerOptions: serializeJson(models.structuredOutput?.providerOptions),
  },
  summarizer: {
    ...models.summarizer,
    providerOptions: serializeJson(models.summarizer?.providerOptions),
  },
};
⚠️ Node deep-link loads correct sub-agent editor and prompt
  • What failed: Deep-linking to a node crashes before editor verification because sub-agent node rendering executes the same unsafe model split path.
  • Impact: Direct node links are unreliable, blocking targeted editing and debugging workflows. Users cannot validate or update node-level prompt and settings from shared deep links.
  • Introduced by this PR: Yes – this PR modified the relevant code
  • Steps to reproduce:
    1. Open /default/projects/activities-planner/agents/activities-planner?pane=node&nodeId=get-coordinates-agent.
    2. Wait for node editor load.
    3. Observe error boundary instead of sub-agent editor fields and prompt.
  • Code analysis: This path still renders SubAgentNode in the graph, so it reaches the same nullable-model handling bug. The code path is shared with the main route and has no guard for undefined model values before string operations.
  • Why this is likely a bug: The deep-link failure is explained by deterministic unsafe handling in shared production component code, not by external mocks or runner-only patches.

Relevant code:

agents-manage-ui/src/components/agent/nodes/sub-agent-node.tsx (lines 73-88)

const projectModel = project?.models;
const modelName =
  subAgent.models?.base.model ?? agentModel.base.model ?? (projectModel?.base.model as string);

const dataComponentsById = createLookup(dataComponents);
const artifactComponentsById = createLookup(artifactComponents);

const dataComponentNames =
  dataComponentIds.map((id) => dataComponentsById[id]?.name).filter(Boolean) || [];
const artifactComponentNames =
  artifactComponentIds.map((id) => artifactComponentsById[id]?.name).filter(Boolean) || [];

const [modelSlug] = modelName.split('/', 1);

agents-manage-ui/src/components/agent/configuration/agent-types.tsx (lines 1-4)

type ModelSettings = {
  model?: string;
  providerOptions?: string; // JSON string representation for form compatibility
};
⚠️ Unsaved dialog save-and-leave path with valid data
  • What failed: The page crashes into the error boundary (SubAgentNode split-of-undefined path) before or during the dialog workflow, so save-and-leave cannot complete.
  • Impact: Users can lose access to core agent editing and cannot reliably complete unsaved-change protection flows. This blocks high-risk navigation behavior that should prevent accidental data loss.
  • Introduced by this PR: Yes – this PR modified the relevant code
  • Steps to reproduce:
    1. Navigate to /default/projects/activities-planner/agents/activities-planner?pane=node&nodeId=get-coordinates-agent.
    2. Make a valid dirty edit, such as changing Description.
    3. Click Projects and choose Save changes in the Unsaved Changes dialog.
    4. Observe the page crash/error boundary before save-and-leave completes.
  • Code analysis: I traced the crash path through the sub-agent node renderer and form-backed model selection logic. The renderer assumes modelName is always a string, but the form/schema types allow missing model values, making split on undefined plausible in production.
  • Why this is likely a bug: The production renderer dereferences modelName.split(...) without a null-safe fallback even though upstream typed data permits missing model strings.

Relevant code:

agents-manage-ui/src/components/agent/nodes/sub-agent-node.tsx (lines 72-77)

const isDefault = id === defaultSubAgentId;
const projectModel = project?.models;
const modelName =
  subAgent.models?.base.model ?? agentModel.base.model ?? (projectModel?.base.model as string);

const dataComponentsById = createLookup(dataComponents);

agents-manage-ui/src/components/agent/nodes/sub-agent-node.tsx (lines 88-94)

const [modelSlug] = modelName.split('/', 1);

const ModelIcon = {
  openai: OpenAIIcon,
  anthropic: AnthropicIcon,
  google: GoogleIcon,
}[modelSlug];

agents-manage-ui/src/components/agent/configuration/agent-types.tsx (lines 1-9)

type ModelSettings = {
  model?: string;
  providerOptions?: string; // JSON string representation for form compatibility
};

export type AgentModels = {
  base?: ModelSettings;
  structuredOutput?: ModelSettings;
  summarizer?: ModelSettings;
};
✅ Passed (13)
Category Summary Screenshot
Adversarial Rapid submit stress remained stable with one effective save and persisted data after reload. ADV-1
Adversarial ID validation correctly rejects malicious/reserved values with 400 and accepts a URL-safe ID. ADV-3
Adversarial Security payload strings persisted as inert text with no script execution side effects. ADV-4
Edge Duplicate sub-agent IDs produced deterministic must be unique validation and both Go-to actions navigated to the correct nodes. EDGE-2
Edge Sub-agent rename preserved linked tool relationships after save/reload, with supporting domain-path verification. EDGE-3
Edge Deleting the default sub-agent was prevented, and the node/default marker remained intact. EDGE-4
Edge Disconnecting the MCP edge cleared stale relation state and serialization remained consistent. EDGE-6
Logic Prompt autocomplete showed contextVariablesValue, headers.testHeadersJsonSchemaValue, and $env after JSON config save. LOGIC-1
Logic Prompt with {{unknown}} and {{$env.MY_ENV}} produced one unknown-variable error and no $env error. LOGIC-2
Logic Invalid contextVariables JSON surfaced grouped validation, blocked successful save, and kept Save enabled for retry. LOGIC-3
Happy-path Created a new agent, initialized its default sub-agent, and saved cleanly without schema mismatch errors. ROUTE-3
Happy-path Description edit persisted after save/reload, and literal null JSON input was rejected by validation. ROUTE-4
Happy-path Unsaved dialog appeared for invalid dirty state, and Discard correctly navigated to /default/projects. ROUTE-5

Commit: 370ce17

View Full Run


Tell us how we did: Give Ito Feedback

dimaMachina and others added 3 commits March 27, 2026 02:27
* fix typecheck

* fix modelName

* fix

* [P1] The current model hides two different identities behind id. In live form state, subAgents and defaultSubAgentId are actually keyed by node key, not persisted subagent ID; then submit-time transforms convert some of that back to persisted IDs. That translation is spread across the resolver, hydrateNodesWithFormData, and serializeAgentData, so form.reset changes semantics mid-session. See validation.ts (line 160), validation.ts (line 240), serialize.ts (line 87), serialize.ts (line 150), sub-agent-selector.tsx (line 32).

* [P1] The current model hides two different identities behind id. In live form state, subAgents and defaultSubAgentId are actually keyed by node key, not persisted subagent ID; then submit-time transforms convert some of that back to persisted IDs. That translation is spread across the resolver, hydrateNodesWithFormData, and serializeAgentData, so form.reset changes semantics mid-session. See validation.ts (line 160), validation.ts (line 240), serialize.ts (line 87), serialize.ts (line 150), sub-agent-selector.tsx (line 32).

* [P1] The current model hides two different identities behind id. In live form state, subAgents and defaultSubAgentId are actually keyed by node key, not persisted subagent ID; then submit-time transforms convert some of that back to persisted IDs. That translation is spread across the resolver, hydrateNodesWithFormData, and serializeAgentData, so form.reset changes semantics mid-session. See validation.ts (line 160), validation.ts (line 240), serialize.ts (line 87), serialize.ts (line 150), sub-agent-selector.tsx (line 32).

* [P1] The current model hides two different identities behind id. In live form state, subAgents and defaultSubAgentId are actually keyed by node key, not persisted subagent ID; then submit-time transforms convert some of that back to persisted IDs. That translation is spread across the resolver, hydrateNodesWithFormData, and serializeAgentData, so form.reset changes semantics mid-session. See validation.ts (line 160), validation.ts (line 240), serialize.ts (line 87), serialize.ts (line 150), sub-agent-selector.tsx (line 32).

* [P1] The current model hides two different identities behind id. In live form state, subAgents and defaultSubAgentId are actually keyed by node key, not persisted subagent ID; then submit-time transforms convert some of that back to persisted IDs. That translation is spread across the resolver, hydrateNodesWithFormData, and serializeAgentData, so form.reset changes semantics mid-session. See validation.ts (line 160), validation.ts (line 240), serialize.ts (line 87), serialize.ts (line 150), sub-agent-selector.tsx (line 32).

* [P1] The current model hides two different identities behind id. In live form state, subAgents and defaultSubAgentId are actually keyed by node key, not persisted subagent ID; then submit-time transforms convert some of that back to persisted IDs. That translation is spread across the resolver, hydrateNodesWithFormData, and serializeAgentData, so form.reset changes semantics mid-session. See validation.ts (line 160), validation.ts (line 240), serialize.ts (line 87), serialize.ts (line 150), sub-agent-selector.tsx (line 32).

* [P1] The current model hides two different identities behind id. In live form state, subAgents and defaultSubAgentId are actually keyed by node key, not persisted subagent ID; then submit-time transforms convert some of that back to persisted IDs. That translation is spread across the resolver, hydrateNodesWithFormData, and serializeAgentData, so form.reset changes semantics mid-session. See validation.ts (line 160), validation.ts (line 240), serialize.ts (line 87), serialize.ts (line 150), sub-agent-selector.tsx (line 32).

* [P1] The current model hides two different identities behind id. In live form state, subAgents and defaultSubAgentId are actually keyed by node key, not persisted subagent ID; then submit-time transforms convert some of that back to persisted IDs. That translation is spread across the resolver, hydrateNodesWithFormData, and serializeAgentData, so form.reset changes semantics mid-session. See validation.ts (line 160), validation.ts (line 240), serialize.ts (line 87), serialize.ts (line 150), sub-agent-selector.tsx (line 32).

* [P1] The current model hides two different identities behind id. In live form state, subAgents and defaultSubAgentId are actually keyed by node key, not persisted subagent ID; then submit-time transforms convert some of that back to persisted IDs. That translation is spread across the resolver, hydrateNodesWithFormData, and serializeAgentData, so form.reset changes semantics mid-session. See validation.ts (line 160), validation.ts (line 240), serialize.ts (line 87), serialize.ts (line 150), sub-agent-selector.tsx (line 32).

* [P1] The current model hides two different identities behind id. In live form state, subAgents and defaultSubAgentId are actually keyed by node key, not persisted subagent ID; then submit-time transforms convert some of that back to persisted IDs. That translation is spread across the resolver, hydrateNodesWithFormData, and serializeAgentData, so form.reset changes semantics mid-session. See validation.ts (line 160), validation.ts (line 240), serialize.ts (line 87), serialize.ts (line 150), sub-agent-selector.tsx (line 32).

* [P2] Business data is duplicated in RHF and node.data, but renderers already trust RHF. hydrateNodesWithFormData() exists only because serializeAgentData() reads node payload fields from node.data; the UI mostly ignores those same fields and watches RHF instead. That duplication is the main drift source. See serialize.ts (line 87), deserialize.ts (line 123), sub-agent-node.tsx (line 50), function-tool-node.tsx (line 17), mcp-node.tsx (line 63).

* [P2] Business data is duplicated in RHF and node.data, but renderers already trust RHF. hydrateNodesWithFormData() exists only because serializeAgentData() reads node payload fields from node.data; the UI mostly ignores those same fields and watches RHF instead. That duplication is the main drift source. See serialize.ts (line 87), deserialize.ts (line 123), sub-agent-node.tsx (line 50), function-tool-node.tsx (line 17), mcp-node.tsx (line 63).

* [P2] Business data is duplicated in RHF and node.data, but renderers already trust RHF. hydrateNodesWithFormData() exists only because serializeAgentData() reads node payload fields from node.data; the UI mostly ignores those same fields and watches RHF instead. That duplication is the main drift source. See serialize.ts (line 87), deserialize.ts (line 123), sub-agent-node.tsx (line 50), function-tool-node.tsx (line 17), mcp-node.tsx (line 63).

* [P2] Business data is duplicated in RHF and node.data, but renderers already trust RHF. hydrateNodesWithFormData() exists only because serializeAgentData() reads node payload fields from node.data; the UI mostly ignores those same fields and watches RHF instead. That duplication is the main drift source. See serialize.ts (line 87), deserialize.ts (line 123), sub-agent-node.tsx (line 50), function-tool-node.tsx (line 17), mcp-node.tsx (line 63).

* [P2] Business data is duplicated in RHF and node.data, but renderers already trust RHF. hydrateNodesWithFormData() exists only because serializeAgentData() reads node payload fields from node.data; the UI mostly ignores those same fields and watches RHF instead. That duplication is the main drift source. See serialize.ts (line 87), deserialize.ts (line 123), sub-agent-node.tsx (line 50), function-tool-node.tsx (line 17), mcp-node.tsx (line 63).

* [P2] Business data is duplicated in RHF and node.data, but renderers already trust RHF. hydrateNodesWithFormData() exists only because serializeAgentData() reads node payload fields from node.data; the UI mostly ignores those same fields and watches RHF instead. That duplication is the main drift source. See serialize.ts (line 87), deserialize.ts (line 123), sub-agent-node.tsx (line 50), function-tool-node.tsx (line 17), mcp-node.tsx (line 63).

* [P2] Business data is duplicated in RHF and node.data, but renderers already trust RHF. hydrateNodesWithFormData() exists only because serializeAgentData() reads node payload fields from node.data; the UI mostly ignores those same fields and watches RHF instead. That duplication is the main drift source. See serialize.ts (line 87), deserialize.ts (line 123), sub-agent-node.tsx (line 50), function-tool-node.tsx (line 17), mcp-node.tsx (line 63).

* [P2] Business data is duplicated in RHF and node.data, but renderers already trust RHF. hydrateNodesWithFormData() exists only because serializeAgentData() reads node payload fields from node.data; the UI mostly ignores those same fields and watches RHF instead. That duplication is the main drift source. See serialize.ts (line 87), deserialize.ts (line 123), sub-agent-node.tsx (line 50), function-tool-node.tsx (line 17), mcp-node.tsx (line 63).

* [P2] Business data is duplicated in RHF and node.data, but renderers already trust RHF. hydrateNodesWithFormData() exists only because serializeAgentData() reads node payload fields from node.data; the UI mostly ignores those same fields and watches RHF instead. That duplication is the main drift source. See serialize.ts (line 87), deserialize.ts (line 123), sub-agent-node.tsx (line 50), function-tool-node.tsx (line 17), mcp-node.tsx (line 63).

* [P2] Business data is duplicated in RHF and node.data, but renderers already trust RHF. hydrateNodesWithFormData() exists only because serializeAgentData() reads node payload fields from node.data; the UI mostly ignores those same fields and watches RHF instead. That duplication is the main drift source. See serialize.ts (line 87), deserialize.ts (line 123), sub-agent-node.tsx (line 50), function-tool-node.tsx (line 17), mcp-node.tsx (line 63).

* [P2] Business data is duplicated in RHF and node.data, but renderers already trust RHF. hydrateNodesWithFormData() exists only because serializeAgentData() reads node payload fields from node.data; the UI mostly ignores those same fields and watches RHF instead. That duplication is the main drift source. See serialize.ts (line 87), deserialize.ts (line 123), sub-agent-node.tsx (line 50), function-tool-node.tsx (line 17), mcp-node.tsx (line 63).

* [P2] Business data is duplicated in RHF and node.data, but renderers already trust RHF. hydrateNodesWithFormData() exists only because serializeAgentData() reads node payload fields from node.data; the UI mostly ignores those same fields and watches RHF instead. That duplication is the main drift source. See serialize.ts (line 87), deserialize.ts (line 123), sub-agent-node.tsx (line 50), function-tool-node.tsx (line 17), mcp-node.tsx (line 63).

* [P2] Business data is duplicated in RHF and node.data, but renderers already trust RHF. hydrateNodesWithFormData() exists only because serializeAgentData() reads node payload fields from node.data; the UI mostly ignores those same fields and watches RHF instead. That duplication is the main drift source. See serialize.ts (line 87), deserialize.ts (line 123), sub-agent-node.tsx (line 50), function-tool-node.tsx (line 17), mcp-node.tsx (line 63).

* [P2] Business data is duplicated in RHF and node.data, but renderers already trust RHF. hydrateNodesWithFormData() exists only because serializeAgentData() reads node payload fields from node.data; the UI mostly ignores those same fields and watches RHF instead. That duplication is the main drift source. See serialize.ts (line 87), deserialize.ts (line 123), sub-agent-node.tsx (line 50), function-tool-node.tsx (line 17), mcp-node.tsx (line 63).

* [P2] Business data is duplicated in RHF and node.data, but renderers already trust RHF. hydrateNodesWithFormData() exists only because serializeAgentData() reads node payload fields from node.data; the UI mostly ignores those same fields and watches RHF instead. That duplication is the main drift source. See serialize.ts (line 87), deserialize.ts (line 123), sub-agent-node.tsx (line 50), function-tool-node.tsx (line 17), mcp-node.tsx (line 63).

* [P2] Business data is duplicated in RHF and node.data, but renderers already trust RHF. hydrateNodesWithFormData() exists only because serializeAgentData() reads node payload fields from node.data; the UI mostly ignores those same fields and watches RHF instead. That duplication is the main drift source. See serialize.ts (line 87), deserialize.ts (line 123), sub-agent-node.tsx (line 50), function-tool-node.tsx (line 17), mcp-node.tsx (line 63).

* [P2] Business data is duplicated in RHF and node.data, but renderers already trust RHF. hydrateNodesWithFormData() exists only because serializeAgentData() reads node payload fields from node.data; the UI mostly ignores those same fields and watches RHF instead. That duplication is the main drift source. See serialize.ts (line 87), deserialize.ts (line 123), sub-agent-node.tsx (line 50), function-tool-node.tsx (line 17), mcp-node.tsx (line 63).

* [P2] Business data is duplicated in RHF and node.data, but renderers already trust RHF. hydrateNodesWithFormData() exists only because serializeAgentData() reads node payload fields from node.data; the UI mostly ignores those same fields and watches RHF instead. That duplication is the main drift source. See serialize.ts (line 87), deserialize.ts (line 123), sub-agent-node.tsx (line 50), function-tool-node.tsx (line 17), mcp-node.tsx (line 63).

* [P2] Business data is duplicated in RHF and node.data, but renderers already trust RHF. hydrateNodesWithFormData() exists only because serializeAgentData() reads node payload fields from node.data; the UI mostly ignores those same fields and watches RHF instead. That duplication is the main drift source. See serialize.ts (line 87), deserialize.ts (line 123), sub-agent-node.tsx (line 50), function-tool-node.tsx (line 17), mcp-node.tsx (line 63).

* [P2] Business data is duplicated in RHF and node.data, but renderers already trust RHF. hydrateNodesWithFormData() exists only because serializeAgentData() reads node payload fields from node.data; the UI mostly ignores those same fields and watches RHF instead. That duplication is the main drift source. See serialize.ts (line 87), deserialize.ts (line 123), sub-agent-node.tsx (line 50), function-tool-node.tsx (line 17), mcp-node.tsx (line 63).

* [P2] Business data is duplicated in RHF and node.data, but renderers already trust RHF. hydrateNodesWithFormData() exists only because serializeAgentData() reads node payload fields from node.data; the UI mostly ignores those same fields and watches RHF instead. That duplication is the main drift source. See serialize.ts (line 87), deserialize.ts (line 123), sub-agent-node.tsx (line 50), function-tool-node.tsx (line 17), mcp-node.tsx (line 63).

* [P2] Business data is duplicated in RHF and node.data, but renderers already trust RHF. hydrateNodesWithFormData() exists only because serializeAgentData() reads node payload fields from node.data; the UI mostly ignores those same fields and watches RHF instead. That duplication is the main drift source. See serialize.ts (line 87), deserialize.ts (line 123), sub-agent-node.tsx (line 50), function-tool-node.tsx (line 17), mcp-node.tsx (line 63).

* Group 1: Stabilize graph identity

* Group 2: Finish RHF as the business source of truth

* Group 2: Finish RHF as the business source of truth

* Group 2: Finish RHF as the business source of truth

* Group 3: Remove layout dependence on node business payload

* upd

* upd

* upd

* upd

* upd

* upd

* upd

* upd

* upd

* upd

* upd

* upd

* upd

* upd

* upd

* upd

* upd

* upd

* renames

* Group 1: Fix remaining schema mismatches

* Group 1: Fix remaining schema mismatches

* Group 1: Fix remaining schema mismatches

* Group 1: Fix remaining schema mismatches

* upd

* upd

* upd

* Group 3: Collapse connection state into one relation model

* Group 3: Collapse connection state into one relation model

* Group 3: Collapse connection state into one relation model

* Group 3: Collapse connection state into one relation model

* Group 3: Collapse connection state into one relation model

* Group 3: Collapse connection state into one relation model

* Group 3: Collapse connection state into one relation model

* Group 3: Collapse connection state into one relation model

* Group 3: Collapse connection state into one relation model

* Group 3: Collapse connection state into one relation model

* Group 3: Collapse connection state into one relation model

* Group 3: Collapse connection state into one relation model

* Group 3: Collapse connection state into one relation model

* Group 3: Collapse connection state into one relation model

* Group 3: Collapse connection state into one relation model

* Group 3: Collapse connection state into one relation model

* Group 3: Collapse connection state into one relation model

* more

* more

* rm

* rm

* Group 3: Finish explicit graph identity

* comment

* upd

* upd

* upd

* upd

* upd

* upd

* simplify

* lint

* update snapshot

* fix zod omit issue

* fix cypress

* style: auto-format with biome

* fix review

* Update mcp-node-editor.tsx

* apply more review

* fix: differentiate loading vs missing state in MCP node editor null guard

* Update mcp-node-editor.tsx

* fix inheritance in subagent node

* fix needs approval

* polish

* fix issue

* fix race condition

* fix error

* Add detailed changeset for graph identity refactor

Co-authored-by: Dimitri POSTOLOV <[email protected]>
Co-Authored-By: Claude Opus 4.6 <[email protected]>

---------

Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
Co-authored-by: pullfrog[bot] <226033991+pullfrog[bot]@users.noreply.github.com>
Co-authored-by: claude[bot] <41898282+claude[bot]@users.noreply.github.com>
Co-authored-by: Dimitri POSTOLOV <[email protected]>
Co-authored-by: Claude Opus 4.6 <[email protected]>
Copy link
Copy Markdown
Contributor

@claude claude Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

PR Review Summary

(3) Total Issues | Risk: Medium

Delta review — scoped to changes since commit 370ce17c3f54. The delta contains 34 commits (mostly merges from main plus PR-specific changes). Key delta changes include graph identity system improvements, function tool relation tracking, and MCP relation key refactoring.

🟠⚠️ Major (1) 🟠⚠️

🟠 1) unsaved-changes-dialog.tsx:56 Navigation proceeds after failed save — data loss risk

Issue: The handleSaveAndLeave function at line 56 uses isValid (captured at render time from useFormState) to decide whether to proceed with navigation after await onSubmit(). This checks form validation state, not whether the submission actually succeeded. If the form passes client-side validation but the server rejects the save (network error, server validation error, permission denied), isValid remains true and users are navigated away.

Why: This is a confirmed data loss risk. Users click "Save", the API call fails (silently or with an error toast), and they're navigated away believing their changes were persisted. The dialog closes unconditionally at line 59. This issue has been flagged in 8+ consecutive automated reviews since Feb 10 (43+ days).

Fix: Use isSubmitSuccessful from form state after submission completes:

async function handleSaveAndLeave() {
  if (isSubmitting) {
    return;
  }
  await onSubmit();
  if (form.formState.isSubmitSuccessful) {
    proceedWithNavigation();
  }
  setShowUnsavedDialog(false);
}

Refs:

🟡 Minor (0) 🟡

Inline Comments:

  • 🟡 Minor: use-default-sub-agent-id-ref.ts:8 useRef type mismatch with null vs undefined
  • 🟡 Minor: use-animate-graph.ts:103 Fragile path-based agentId extraction

💭 Consider (3) 💭

💭 1) agent-error-summary.tsx:201-237 Commented-out code in production
Issue: Lines 205-208 and 226-239 contain commented-out code for the error summary close button functionality. The comment explains the feature is temporarily disabled.
Why: Commented-out code reduces readability. The comment indicates this needs rethinking, suggesting it should be tracked as a TODO/issue rather than left as commented code.
Fix: Remove the commented code and create a GitHub issue to track the UX improvement, or add a // TODO(#issue-number): comment linking to a tracked issue.

💭 2) graph-keys.ts:44-71 Complex fallback priority logic lacks direct unit tests
Issue: The getMcpGraphKey function implements complex fallback priority logic with 6 distinct code paths but no dedicated unit tests. It's only tested indirectly through sync-saved-agent-graph.test.ts.
Why: If the priority order is accidentally changed, MCP node identities could be incorrectly computed, causing nodes to lose their connection to persisted data.
Fix: Add unit tests for getMcpGraphKey covering each priority branch.

💭 3) sub-agent-identity.ts:20-40 findSubAgentNodeId function lacks unit tests
Issue: This new utility finds a sub-agent's React Flow node ID given a sub-agent ID and form data mapping. It has no direct unit tests.
Why: If this function incorrectly matches nodes, the graph could select the wrong sub-agent after operations like save.
Fix: Add unit tests for findSubAgentNodeId covering edge cases like empty arrays or multiple nodes.

🕐 Pending Recommendations (1)


🚫 REQUEST CHANGES

Summary: The delta introduces a well-structured graph identity system with good test coverage for the new domain logic. However, the MAJOR data loss risk in handleSaveAndLeave (using isValid instead of isSubmitSuccessful) has been pending for 43+ days across 8+ review cycles and must be addressed before merge. The fix is straightforward — replace isValid with form.formState.isSubmitSuccessful to ensure navigation only proceeds after confirmed successful save.

Discarded (2)
Location Issue Reason Discarded
validation.ts status toggle persistence Previously flagged as MAJOR in prior reviews Status update schema no longer forces defaults — numEvents and timeInSeconds now use NullToUndefinedSchema.pipe() which preserves undefined when not provided. Issue appears resolved in delta.
sync-saved-agent-graph.test.ts:522 Missing test for function tool relation ID claim collision Low confidence (4/10), edge case, and the core logic is covered by existing integration tests
Reviewers (3)
Reviewer Returned Main Findings Consider While You're Here Inline Comments Pending Recs Discarded
pr-review-frontend 3 0 1 0 2 0 0
pr-review-tests 3 0 2 0 0 0 1
pr-review-standards 0 0 0 0 0 0 0
orchestrator 1 1 0 0 0 1 1
Total 7 1 3 0 2 1 2

'use memo';

const form = useFullAgentFormContext();
const defaultSubAgentNodeIdRef = useRef<string | undefined>(null);
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Minor: useRef type mismatch

Issue: useRef<string | undefined>(null) initializes a ref typed as string | undefined with null. While TypeScript allows this, it creates a runtime inconsistency where defaultSubAgentNodeIdRef.current could be null but the type says string | undefined.

Why: Components consuming this ref might not handle the null case correctly since the type signature doesn't include it.

Fix: (1-click apply)

Suggested change
const defaultSubAgentNodeIdRef = useRef<string | undefined>(null);
const defaultSubAgentNodeIdRef = useRef<string | undefined>(undefined);

Refs:

@@ -51,45 +101,49 @@ export function useAnimateGraph(): void {
nodes: updateNodeStatus((node) => {
// this prevents the node from highlighting if the copilot triggers this event
if (data?.details?.agentId !== /* agentId */ location.pathname.split('/')[5]) {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Minor: Fragile path-based agentId extraction

Issue: location.pathname.split('/')[5] relies on a specific URL structure (/[tenantId]/projects/[projectId]/agents/[agentId]). If the URL structure changes or has extra segments, this will silently fail to match.

Why: This pattern can cause the animation to incorrectly skip highlighting the default sub-agent node if the URL structure changes. The comment /* agentId */ is helpful but the implementation is brittle.

Fix: Consider using Next.js useParams() hook to get agentId explicitly, or pass it through props/context. This makes the dependency explicit and self-documenting:

const params = useParams<{ agentId: string }>();
// then use: data?.details?.agentId !== params.agentId

Refs:

Copy link
Copy Markdown
Contributor

@claude claude Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

PR Review Summary

(1) Total Issues | Risk: Medium

Delta review — scoped to changes since commit bcd4654687a6. The delta contains 1 commit (fix typecheck) affecting 3 files with minor cleanup changes.

✅ Delta Assessment

The delta is clean. Changes are limited to:

Category Files Changed Impact
CLI generators 2 Added missing z import, extracted GeneratorParams type alias
UI cleanup 1 Removed unused destructured variables from useAgentErrors hook

No new issues introduced in the delta.

Changes reviewed:

  • agent-generator.ts: Added import { z } from 'zod' — required for schema reference
  • context-config-generator.ts: Extracted GeneratorParams type alias for cleaner type annotations, changed contextVariables param to accept null
  • sidepane.tsx: Removed unused hasFieldError, getFieldErrorMessage, getFirstErrorField from destructuring — good cleanup

🕐 Pending Recommendations (1)

This issue was identified in prior reviews (43+ days ago, 8+ review cycles) and remains unresolved in unchanged code:

  • 🟠 unsaved-changes-dialog.tsx:51-60 Navigation proceeds after failed save — uses isValid (render-time validation state) instead of isSubmitSuccessful to decide navigation after save. If API call fails, users are navigated away believing changes were persisted. Data loss risk. Fix: if (form.formState.isSubmitSuccessful) instead of if (isValid)

💡 APPROVE WITH SUGGESTIONS

Summary: The delta is clean — typecheck fixes and unused import cleanup are well-done. The 1 pending MAJOR recommendation from prior reviews remains unresolved on unchanged code. The isValidisSubmitSuccessful fix is a one-line change that would prevent data loss when save operations fail — recommend addressing before merge.

Reviewers (1)
Reviewer Returned Main Findings Consider While You're Here Inline Comments Pending Recs Discarded
orchestrator 1 0 0 0 0 1 0
Total 1 0 0 0 0 1 0

Note: Minimal delta review — no sub-reviewers dispatched as changes were trivial typecheck fixes.

Copy link
Copy Markdown
Contributor

@claude claude Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

PR Review Summary

(1) Total Issues | Risk: Medium

Delta review — scoped to changes since commit e3176a2939df. The delta contains 2 commits: a merge from main and the Durable execution feature (#2615). No PR-specific code changes in the delta — the 40 files changed are entirely from merging main.

✅ Delta Assessment

The delta is clean and contains no PR-specific changes. All modifications are from the Durable execution (#2615) merge:

Category Files Changed Impact
agents-api/ 27 Durable execution workflow, routes, handlers
packages/agents-core/ 11 Schema migrations, data access, types
Other 2 Changeset, docs script

No agents-manage-ui changes in the delta. The PR's core functionality (react-hook-form migration, zod schemas) was not modified since the last review.

🕐 Pending Recommendations (1)

This issue was identified 43+ days ago across 8+ consecutive automated reviews and remains unresolved in unchanged code:

  • 🟠 unsaved-changes-dialog.tsx:56 Navigation proceeds after failed save — data loss risk

    Issue: Uses isValid (render-time validation state) instead of isSubmitSuccessful to decide navigation after await onSubmit(). If API call fails (network error, server validation error, permission denied), isValid remains true and users are navigated away believing changes were saved.

    Fix (1 line): Change line 56 from if (isValid)if (form.formState.isSubmitSuccessful)

    async function handleSaveAndLeave() {
      if (isSubmitting) return;
      await onSubmit();
      if (form.formState.isSubmitSuccessful) {  // ← Fix: check submission result, not validation state
        proceedWithNavigation();
      }
      setShowUnsavedDialog(false);
    }

    Refs:


💡 APPROVE WITH SUGGESTIONS

Summary: The delta is clean — it consists entirely of the Durable execution feature merged from main with no PR-specific changes. The react-hook-form refactor code was not modified since the last review. The 1 pending MAJOR recommendation (43+ days old, 8+ review cycles) remains unresolved. The fix is a single-line change (isValidform.formState.isSubmitSuccessful) that would prevent data loss when save operations fail. This is a straightforward fix that should be addressed before merge.

Reviewers (1)
Reviewer Returned Main Findings Consider While You're Here Inline Comments Pending Recs Discarded
orchestrator 1 0 0 0 0 1 0
Total 1 0 0 0 0 1 0

Note: Delta review — no sub-reviewers dispatched as the delta contains no PR-specific changes (only merge from main).

Copy link
Copy Markdown
Contributor

@claude claude Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

PR Review Summary

(4) Total Issues | Risk: Medium

This is a comprehensive review of the full PR scope. The PR introduces a significant architectural improvement by migrating from Zustand-based state management to react-hook-form with zod validation schemas.

🟠⚠️ Major (2) 🟠⚠️

🟠 1) validation.test.ts Missing test coverage for duplicate sub-agent ID validation

Issue: The FullAgentFormSchema includes a .superRefine() validator at validation.ts:171-191 that checks for duplicate sub-agent IDs across nodes and adds custom errors. This critical validation logic has zero test coverage.

Why: If a regression is introduced in the duplicate detection logic (e.g., off-by-one in iteration, incorrect path construction for error messages), users could create agents with conflicting sub-agent identifiers. This would cause runtime errors or data corruption when the backend processes them, as sub-agent IDs are used as keys in the subAgents record.

Fix: Add test case to validation.test.ts:

it('should reject duplicate sub-agent IDs across nodes', () => {
  const result = FullAgentFormSchema.safeParse({
    ...createSchema(''),
    defaultSubAgentNodeId: 'node-1',
    subAgents: {
      'node-1': {
        id: 'same-id', name: 'A', prompt: 'Hi', type: 'internal',
        models: { base: {}, structuredOutput: {}, summarizer: {} },
        canUse: [], dataComponents: [], artifactComponents: [], stopWhen: {},
      },
      'node-2': {
        id: 'same-id', name: 'B', prompt: 'Hi', type: 'internal',
        models: { base: {}, structuredOutput: {}, summarizer: {} },
        canUse: [], dataComponents: [], artifactComponents: [], stopWhen: {},
      },
    },
  });
  expect(result.success).toBe(false);
  expect(result.error?.issues).toContainEqual(
    expect.objectContaining({
      path: expect.arrayContaining(['subAgents', 'node-1', 'id']),
      message: expect.stringContaining('must be unique'),
    })
  );
});

Refs:

Inline Comments:

  • 🟠 Major: page.client.tsx:678-687 Server validation errors may fail silently when res.error isn't valid JSON

🟡 Minor (3) 🟡

🟡 1) schemas.ts StatusUpdateSchema changed from z.object() to z.strictObject() — potential breaking change

Issue: The StatusUpdateSchema was changed from z.object() to z.strictObject(). This causes validation to fail on unknown keys.

Why: If API consumers were sending additional properties (even if ignored), their requests will now fail validation. This could break existing integrations.

Fix: If this is intentional, document it in the changeset. If backward compatibility is needed, use .passthrough() on the strict object to allow extra keys while still enforcing known keys.

Refs:

Inline Comments:

  • 🟡 Minor: schemas.ts:161-162 Dev comment // cc @sarah left in production code
  • 🟡 Minor: use-grouped-agent-errors.ts:77 MCP relation errors collapse to empty key when nodeId lookup fails
  • 🟡 Minor: use-processed-errors.tsx:28 flatNestedFieldMessage may return undefined, showing cryptic "undefined" to users

💭 Consider (2) 💭

💭 1) graph-keys.ts Naming inconsistency between graph key and form key functions

Issue: The PR introduces two sets of key-generation utilities with different naming conventions: get*GraphKey functions in graph-keys.ts for node identity, and get*FormKey functions in form-state-defaults.ts for form field paths. The distinction between 'graph key' (identity in React Flow) vs 'form key' (path in react-hook-form) is not immediately obvious.

Why: This sets a naming precedent for future development. Clarity between these two concepts will help maintainability.

Fix: Consider adding a brief JSDoc comment to each module clarifying the domain, or prefix form key functions with getFormPath* instead of get*FormKey.

Refs:

💭 2) graph-keys.ts:32-36 getSubAgentGraphKey is identity function unlike other graph key functions

Issue: The getSubAgentGraphKey() returns the subAgentId unchanged, while other get*GraphKey() functions add prefixes (mcp:, function-tool:, etc.). This asymmetry may be confusing when debugging.

Why: Sub-agents are the primary domain entity, so using raw IDs may be intentional for URL compatibility, but this isn't documented.

Fix: Consider adding a comment explaining why sub-agent graph keys are unprefixed.

Refs:

🕐 Pending Recommendations (1)

This issue was identified 43+ days ago across 8+ consecutive automated reviews and remains unresolved in unchanged code:

  • 🟠 unsaved-changes-dialog.tsx:51-60 Navigation proceeds after failed save — data loss risk

    Issue: Uses isValid (render-time validation state) instead of isSubmitSuccessful to decide navigation after await onSubmit(). If API call fails (network error, server validation error, permission denied), isValid remains true and users are navigated away believing changes were saved.

    Fix (1 line): Change line 56 from if (isValid)if (form.formState.isSubmitSuccessful)

    Refs: react-hook-form formState docs


🚫 REQUEST CHANGES

Summary: This is a well-structured architectural refactor with solid patterns. The react-hook-form + zod migration is clean and the graph identity system is well-designed. However, the 43+ day old data loss bug in unsaved-changes-dialog.tsx must be addressed before merge — it's a one-line fix (isValidisSubmitSuccessful). Additionally, the missing test coverage for duplicate sub-agent ID validation should be added to prevent regressions in critical validation logic.

Discarded (5)
Location Issue Reason Discarded
context-config.tsx:21-41 Missing subscription cleanup Actually correct — the return form.subscribe({...}) pattern properly returns the unsubscribe function
full-agent-form.tsx:20 'use memo' directive concern React Compiler is likely configured; directive appears 28+ times across codebase intentionally
validation.ts:337-343 Status updates default values Already raised in prior reviews (Feb 10) — tracked as pending
validation.ts:56-61 ContextConfigSchema .default(null) Intentional design choice for API compatibility — empty string → null is expected behavior
form-state-defaults.ts:51-67 Form key functions are identity transforms Intentional semantic wrappers for future extensibility
Reviewers (6)
Reviewer Returned Main Findings Consider While You're Here Inline Comments Pending Recs Discarded
pr-review-frontend 4 0 0 0 0 1 3
pr-review-standards 0 0 0 0 0 0 0
pr-review-tests 4 1 0 0 0 0 0
pr-review-errors 4 0 0 0 4 0 0
pr-review-types 7 1 0 0 0 0 2
pr-review-consistency 8 0 2 0 0 0 0
Total 27 2 2 0 4 1 5

Note: pr-review-standards returned 0 findings as instructed not to re-raise the known isValid issue. Duplicate findings across reviewers were merged.

Comment on lines +678 to +687
try {
const issues: z.ZodIssue[] = JSON.parse(res.error);
issues.forEach(({ path, code, message }) => {
form.setError(path.join('.') as any, { type: code, message });
});
} catch (parseError) {
// Fallback for unparseable errors
console.error('Failed to parse validation errors:', parseError);
toast.error('Failed to save agent', { closeButton: true });
}
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟠 MAJOR Server validation errors may fail silently

Issue: When res.error is not valid JSON (e.g., server returns HTML error page, network interruption, or malformed response), the catch block only logs to console.error and shows a generic "Failed to save agent" toast. The original error message is discarded.

Why: Users see no specific information about what went wrong, making it harder to diagnose save failures. The original res.error string could contain useful debugging information.

Fix:

Suggested change
try {
const issues: z.ZodIssue[] = JSON.parse(res.error);
issues.forEach(({ path, code, message }) => {
form.setError(path.join('.') as any, { type: code, message });
});
} catch (parseError) {
// Fallback for unparseable errors
console.error('Failed to parse validation errors:', parseError);
toast.error('Failed to save agent', { closeButton: true });
}
// Handle validation errors (422 status - unprocessable_entity)
try {
const issues: z.ZodIssue[] = JSON.parse(res.error);
issues.forEach(({ path, code, message }) => {
form.setError(path.join('.') as any, { type: code, message });
});
} catch (parseError) {
// Fallback for unparseable errors - show original message
console.error('Failed to parse validation errors:', parseError);
toast.error(res.error || 'Failed to save agent', { closeButton: true });
}

Refs:

Comment on lines 161 to +162
transferCountIs: z
.number()
.int()
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Minor Dev comment left in production code

Issue: This comment references a specific person and uses informal language ("cc @sarah"). This appears to be a dev note that was accidentally left in production code.

Why: Schema files should have documentation-style comments explaining the validation rules, not person-specific notes.

Fix:

Suggested change
transferCountIs: z
.number()
.int()
// Maximum transfer count - matches frontend validation limit

Refs:

functionTools: {
...remapErrorGroup(functionTools ?? EMPTY_GROUP, (toolId) =>
functionToolGraphKeysByToolId.get(toolId)
),
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Minor MCP relation errors collapse to single empty-string key when nodeId lookup fails

Issue: The remapErrorGroup uses nodeGraphKeysByNodeId.get(nodeId) ?? '' which means if the nodeId is not found in the map, all such errors collapse into a single entry keyed by empty string. Multiple distinct MCP relation errors would overwrite each other.

Why: Users would only see the last error, losing visibility into all validation issues for MCP relations.

Fix: Preserve the original key as fallback:

Suggested change
),
const mcpRelations = remapErrorGroup(rawMcpRelations, (nodeId) => nodeGraphKeysByNodeId.get(nodeId) ?? nodeId);

Refs:


const fieldErrors = errors?.[entity]?.[key] ?? {};
const processedErrors = Object.entries(fieldErrors).map(([key, value]) => ({
field: key,
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Minor flatNestedFieldMessage may return undefined, showing cryptic "undefined" to users

Issue: When flatNestedFieldMessage(value) returns undefined, the resulting error object has message: undefined. The ErrorIndicator component displays this as : undefined.

Why: Users see cryptic "undefined" text instead of a meaningful error message.

Fix: Provide a fallback message:

Suggested change
field: key,
message: flatNestedFieldMessage(value) || 'Invalid value',

Refs:

@itoqa
Copy link
Copy Markdown

itoqa Bot commented Mar 26, 2026

Ito Test Report ✅

14 test cases ran. 14 passed.

The unified QA run passed with 14/14 executed test cases and 0 failures (with one additional report containing no test cases), indicating stable behavior across the agent builder, canvas, settings, and playground flows. Key findings were that deep-link pane loading and URL state were coherent, function-tool edge constraints and default sub-agent protections held, duplicate/invalid ID and integer/null JSON validations correctly blocked bad saves, rapid save/unsaved-dialog and edge-toggle race scenarios resolved deterministically (with prior blockage attributed to environment connectivity, not product code), and adversarial prompt/header inputs were handled safely without crashes.

✅ Passed (14)
Category Summary Screenshot
Adversarial Rapid save spam completed as a single coherent save lifecycle with no duplicate-error behavior. ADV-1
Adversarial Unsaved dialog Save/Discard action-order races resolved deterministically without stuck modal state. ADV-2
Adversarial A second incoming connection attempt to the same Function Tool was rejected, with a single edge preserved after reload. ADV-3
Adversarial Malformed prompt/template input saved successfully and the editor remained stable without freezing or crashing. ADV-4
Adversarial Escaped newline/control-char headers were handled safely and malformed literal-newline JSON was rejected without UI instability. ADV-5
Edge Unsaved dialog Save with an empty sub-agent Id kept the user on the editor and surfaced validation errors as expected. EDGE-2
Edge Default sub-agent remained protected from deletion via keyboard and sidepane controls. EDGE-4
Edge Save was blocked and validation errors appeared when two sub-agents used the same Id. EDGE-6
Edge JSON null values for constrained editor fields were rejected by validation as expected. EDGE-7
Rapid Rapid edge toggle behavior validated; no product bug confirmed after code-first review and rerun stabilization. RAPID-1
Happy-path Deep-link to sub-agent pane loads correctly with expected fields after environment remediation. ROUTE-1
Happy-path Created a Function Tool, kept only one incoming sub-agent edge, and confirmed state persisted after save and reload. ROUTE-3
Sec Sub-agent Id validation correctly rejected whitespace-only, reserved new, and slash-containing values. SEC-1
Sec Decimal/zero/oversized stop-limit values were rejected, and valid integer values saved successfully. SEC-2

Commit: 2c4ab19

View Full Run


Tell us how we did: Give Ito Feedback

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants