Skip to content

Reverts https://github.com/inkeep/agents/pull/1474#3012

Merged
robert-inkeep merged 2 commits intomainfrom
revert-dee33
Apr 4, 2026
Merged

Reverts https://github.com/inkeep/agents/pull/1474#3012
robert-inkeep merged 2 commits intomainfrom
revert-dee33

Conversation

@robert-inkeep
Copy link
Copy Markdown
Collaborator

Reverts #1474

@changeset-bot
Copy link
Copy Markdown

changeset-bot bot commented Apr 4, 2026

🦋 Changeset detected

Latest commit: 404dd51

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-cli Patch
@inkeep/agents-api 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 Apr 4, 2026

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

Project Deployment Actions Updated (UTC)
agents-api Ready Ready Preview, Comment Apr 4, 2026 8:42pm
agents-docs Ready Ready Preview, Comment Apr 4, 2026 8:42pm
agents-manage-ui Ready Ready Preview, Comment Apr 4, 2026 8:42pm

Request Review

@pullfrog
Copy link
Copy Markdown
Contributor

pullfrog bot commented Apr 4, 2026

TL;DR — Reverts PR #1474, which removed manual useMemo and useCallback calls in favor of the React Compiler. This restores all explicit memoization across ~100 UI components, the CLI, and the docs site.

Key changes

  • Restore useMemo and useCallback imports and call sites — Re-adds explicit memoization hooks removed in Remove useMemo and useCallback usages #1474 across the manage UI, CLI merge app, and docs feedback dialog.
  • Restore useMemo for column definitions and derived data — Table components (api-keys-table, dataset-items-table, evaluation-jobs-list, etc.) regain useMemo<ColumnDef<T>[]>(...) wrappers.
  • Restore useCallback for event handlers and form submit functions — Components like trigger-form, apps-table, and members-table regain useCallback-wrapped handlers.
  • Revert CSS custom property cast patternchart.tsx and sonner.tsx go back to the ['--prop' as string] cast instead of @ts-expect-error with plain keys.
  • Restore JsonSchemaStateData type export — Re-exports the previously removed type from json-schema.ts.
  • Remove biome-ignore comments added post-Remove useMemo and useCallback usages #1474 — Several useExhaustiveDependencies suppression comments that were added after Remove useMemo and useCallback usages #1474 are no longer needed with explicit deps restored.

Summary | 100 files | 2 commits | base: mainrevert-dee33

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

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: Low

🟠⚠️ Major (1) 🟠⚠️

🟠 1) CLAUDE.md / biome.jsonc Documentation-Code Inconsistency

Issue: This revert adds back useMemo and useCallback across 110 files and removes them from the biome restricted imports list. However, CLAUDE.md:109 explicitly states:

React Compiler: React Compiler is enabled for this repo. Do not add memo, useMemo, or useCallback; rely on the compiler unless a maintainer explicitly requests an exception

Why: After this revert lands, the documented guidance in CLAUDE.md will directly contradict the actual codebase practice. This creates confusion for contributors (human and AI) who follow the documented guidelines, only to find the codebase does the opposite. The lint rule change (biome.jsonc) enables useMemo/useCallback imports, but the docs still say not to use them.

Fix: If the decision is to revert to manual memoization:

  1. Update CLAUDE.md line 109 to reflect that manual memoization IS used in this repo
  2. Remove or update the message in biome.jsonc that warns against these imports
  3. Consider adding a brief note explaining why manual memoization is preferred over React Compiler's automatic handling

Alternatively, if the original #1474 removal was correct but caused issues:

  • Document what specific issues prompted the revert
  • Consider a more targeted fix rather than a full revert

Refs:


💡 APPROVE WITH SUGGESTIONS

Summary: This is a mechanical revert of PR #1474, which is low-risk from a code correctness standpoint — the code worked before #1474 was merged. However, the revert creates a documentation inconsistency: CLAUDE.md still says "do not add useMemo or useCallback" while this PR does exactly that. Please update CLAUDE.md to reflect the new/restored approach to memoization before or alongside merging.

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

Note: No sub-reviewers dispatched — this is a mechanical revert with a single documentation consistency issue.

Copy link
Copy Markdown
Contributor

@pullfrog pullfrog bot left a comment

Choose a reason for hiding this comment

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

Clean revert — no actionable issues found.

This is a straightforward git revert of PR #1474 (which removed useMemo/useCallback in favor of React Compiler automatic memoization), plus a changeset commit. The revert cleanly restores all 110 files to their pre-#1474 state:

  • biome.jsonc: Lint rule reverted to only ban memo (not useCallback/useMemo)
  • useMemo: Restored for column definitions, computed values, context values, and the chart IIFE → useMemo conversion
  • useCallback: Restored for event handlers, fetch functions, and functions referenced in useEffect deps
  • biome-ignore comments: Removed — no longer needed since functions are properly wrapped in useCallback
  • CSS custom properties: Reverted from @ts-expect-error back to ['--custom-prop' as string] pattern
  • JsonSchemaStateData export: Restored in json-schema.ts
  • Form handler pattern: Reverted from form.handleSubmit(async (data) => {...}) back to the wrapping-at-call-site pattern (e.g., page.client.tsx)
  • No merge conflict markers detected across the diff

Note: AGENTS.md line 109 still says "Do not add memo, useMemo, or useCallback; rely on the compiler" — this will need updating to reflect the new policy after this revert lands.

Pullfrog  | View workflow run | Using Claude Opus𝕏

@github-actions
Copy link
Copy Markdown
Contributor

github-actions bot commented Apr 4, 2026

Preview URLs

Use these stable preview aliases for testing this PR:

These point to the same Vercel preview deployment as the bot comment, but they stay stable and easier to find.

Raw Vercel deployment URLs

@robert-inkeep robert-inkeep added this pull request to the merge queue Apr 4, 2026
@itoqa
Copy link
Copy Markdown

itoqa bot commented Apr 4, 2026

Ito Test Report ✅

18 test cases ran. 3 additional findings, 15 passed.

The unified local non-production run executed 18 test cases with 15 passing and 3 failing, confirming stable behavior across core auth and invitation journeys, device authorization (including rapid approval interaction), protected-route gating, project/work-app routing, traces/member UX behaviors, and mobile link/login usability without relying on API or auth bypass stubs. The most important issues were a high-severity Slack link token replay flaw (one-time-use not enforced, allowing reused valid tokens to succeed) plus two medium-severity integrity gaps where credential creation can submit with link-to-server enabled but no valid MCP server selected and webhook trigger toggles can persist the wrong final enabled state under rapid clicks due to stale-state race conditions.

✅ Passed (15)
Category Summary Screenshot
Adversarial Rapid repeated approve interactions converged to one terminal authorized state with no oscillation and no uncaught console errors. ADV-1
Adversarial Signed-out deep links to /device and /link stayed behind authentication gates with no protected action execution. ADV-4
Edge Malicious login and invitation returnUrl payloads remained same-origin and did not execute off-origin or script redirects. EDGE-1
Edge A valid invitation link without email produced a stable invalid-link recovery state without hanging. EDGE-2
Edge Applying custom ranges with future-end intent did not trigger backend validation errors; traces and AI Calls remained functional. EDGE-3
Edge Interrupted login and invitation flows recovered successfully and completed to /default/projects without full browser reset. EDGE-4
Edge At 390x844, login/link flows remained usable with no horizontal overflow and readable/tappable expired-link error guidance content. EDGE-5
Logic Single submit created one project and navigated to the new project agents route without duplicate records. LOGIC-1
Logic Work Apps overview rendered expected cards and install actions routed correctly (GitHub internal route, Slack OAuth launch). LOGIC-3
Happy-path In clean cookie context, completed login with admin@example.com via password method and a single submit. Navigation resolved safely to /default/projects with no external callback redirect behavior. ROUTE-1
Happy-path Created a real invitation, validated recipient-mismatch behavior when opened under the wrong authenticated user, then executed the valid acceptance flow with required params and a single accept action. Flow completed to tenant-scoped /default/projects and remained consistent. ROUTE-2
Happy-path Authenticated device flow auto-reached confirm state and a single Approve click produced the approved terminal state with close-window guidance and no visible error alert. ROUTE-3
Happy-path Valid token produced Account Linked success with @qa-user, and reloading the same URL remained stable without duplicate-link crash behavior. ROUTE-4
Happy-path Back navigation from AI Calls breakdown returned to Traces with timeRange, agentId, and hasErrors query values intact. ROUTE-5
Happy-path Members list ordering remained stable after refresh and aligns with case-insensitive name-or-email fallback sorting behavior. ROUTE-6
ℹ️ Additional Findings (3)

These findings are unrelated to the current changes but were observed during testing.

Category Summary Screenshot
Adversarial 🟠 Rapid webhook status toggles can persist an enabled state even when the final user intent is disabled. ADV-2
Adversarial ⚠️ Replayed Slack link token was accepted as a successful link instead of being rejected after first use. ADV-3
Logic 🟠 Credential creation can still submit when link-to-server is enabled but no valid MCP server is selected. LOGIC-2
🟠 Trigger status toggle race abuse
  • What failed: Final persisted enabled state can differ from the user’s final toggle intent after rapid interaction.
  • Impact: Trigger enablement may land in the wrong state after fast operator input, causing automations to run when users intended them off (or vice versa). This can create unintended webhook executions until manually corrected.
  • Steps to reproduce:
    1. Open a project's Webhooks triggers tab with at least one trigger row.
    2. Rapidly click the enabled/disabled switch several times before the first update request settles.
    3. Refresh the page and compare persisted state with the final click intent.
  • Stub / mock context: No stubs, mocks, or bypasses were applied for this test in the recorded run.
  • Code analysis: I reviewed the toggle handler and switch binding in the production table component. Each click computes newEnabled from row.original.enabled (stale until refresh), and rapid clicks can dispatch multiple updates before disabled state propagates, so requests can race with stale intent.
  • Why this is likely a bug: The handler uses stale row state as the source of truth during rapid toggling, so persisted state can diverge from the latest user action in normal UI interaction.

Relevant code:

agents-manage-ui/src/components/project-triggers/project-triggers-table.tsx (lines 67-79)

const toggleEnabled = useCallback(
  async (triggerId: string, agentId: string, currentEnabled: boolean) => {
    const newEnabled = !currentEnabled;
    setLoadingTriggers((prev) => new Set(prev).add(triggerId));

    try {
      const result = await updateTriggerEnabledAction(
        tenantId,
        projectId,
        agentId,
        triggerId,
        newEnabled
      );

agents-manage-ui/src/components/project-triggers/project-triggers-table.tsx (lines 194-200)

<Switch
  checked={row.original.enabled}
  onCheckedChange={() =>
    toggleEnabled(row.original.id, row.original.agentId, row.original.enabled)
  }
  disabled={isLoading || !canManage}
/>
⚠️ Tampered/expired Slack link token replay
  • What failed: The replayed token returns a second success (Account Linked) instead of being rejected as already used.
  • Impact: A Slack link token can be replayed within its validity window, so one-time-use link semantics are not enforced. This weakens account-linking integrity and allows unexpected repeated success on sensitive link URLs.
  • Steps to reproduce:
    1. Sign in with a valid app session.
    2. Open /link with a valid Slack token and wait for Account Linked success.
    3. Re-open the same token URL before the token expires.
    4. Observe that the flow returns success again instead of rejecting replay.
  • Stub / mock context: The run used local developer UI configuration tweaks that hide dev overlays, but Slack token verification and linking were not bypassed or mocked; the replay behavior comes from normal application code paths.
  • Code analysis: Reviewed the link page client flow, Slack verify-token route, and token utility. The API accepts any still-valid JWT and explicitly returns success when an existing mapping belongs to the same user, with no consumed-token check or nonce/jti persistence lookup.
  • Why this is likely a bug: Production code only verifies signature/expiry/schema and then treats an existing same-user mapping as success, so replayed unexpired tokens are not single-use.

Relevant code:

packages/agents-work-apps/src/slack/routes/users.ts (lines 165-175)

const verifyResult = await verifySlackLinkToken(body.token);

if (!verifyResult.valid || !verifyResult.payload) {
  const isExpired = verifyResult.error?.includes('\"exp\" claim timestamp check failed');
  const errorMessage = isExpired
    ? 'Token expired. Please run /inkeep link in Slack to get a new one.'
    : verifyResult.error ||
      'Invalid or expired link token. Please run /inkeep link in Slack to get a new one.';
  return c.json({ error: errorMessage }, 400);
}

packages/agents-work-apps/src/slack/routes/users.ts (lines 201-213)

if (existingLink && existingLink.inkeepUserId === inkeepUserId) {
  logger.info(
    { slackUserId, tenantId, inkeepUserId: body.userId },
    'Slack user already linked to same account'
  );
  return c.json({
    success: true,
    linkId: existingLink.id,
    slackUsername: existingLink.slackUsername || undefined,
    slackTeamId: teamId,
    tenantId,
  });
}

packages/agents-core/src/utils/slack-link-token.ts (lines 117-153)

export async function verifySlackLinkToken(token: string): Promise<VerifySlackLinkTokenResult> {
  const result = await verifyJwt(token, { issuer: ISSUER, audience: AUDIENCE });

  if (!result.valid || !result.payload) {
    return {
      valid: false,
      error: result.error,
    };
  }

  const parseResult = SlackLinkTokenPayloadSchema.safeParse(result.payload);

  if (!parseResult.success) {
    return {
      valid: false,
      error: `Invalid token schema: ${parseResult.error.issues.map((e) => e.message).join(', ')}` ,
    };
  }

  return {
    valid: true,
    payload: parseResult.data,
  };
}
🟠 Credential form invalid server selection guard
  • What failed: Submission is expected to be blocked until a valid MCP server is selected, but create still proceeds and sends a create request.
  • Impact: Users can submit invalid credential-linking state and trigger failed backend writes instead of getting immediate client-side validation. This creates avoidable errors and inconsistent setup flow.
  • Steps to reproduce:
    1. Open the bearer credential form for a project.
    2. Enable the link-to-server option.
    3. Leave MCP server selection invalid or empty while filling required credential fields.
    4. Submit the form and observe that create still executes instead of being blocked.
  • Stub / mock context: No stubs, mocks, or bypasses were applied for this test in the recorded run.
  • Code analysis: I reviewed the credential form validation and bearer create flow. The submit guard blocks only sentinel values (loading/error) and does not block an empty selection while link-to-server is enabled; the page-level handler still executes credential creation before any required server-link validation exists.
  • Why this is likely a bug: The production form logic allows a submit path that violates the stated UI requirement (valid server must be selected when linking is enabled), and the create handler performs writes even in that invalid state.

Relevant code:

agents-manage-ui/src/components/credentials/views/credential-form.tsx (lines 123-133)

const onSubmit = async (data: CredentialFormData) => {
  const isInvalidServerSelection =
    shouldLinkToServer && (data.selectedTool === 'loading' || data.selectedTool === 'error');
  const isInvalidExternalAgentSelection =
    shouldLinkToExternalAgent &&
    (data.selectedExternalAgent === 'loading' || data.selectedExternalAgent === 'error');
  try {
    if (isInvalidServerSelection) {
      toast('Please select a valid MCP server');
      return;
    }

agents-manage-ui/src/app/[tenantId]/projects/[projectId]/credentials/new/bearer/page.tsx (lines 78-101)

await createCredentialInStore({
  tenantId,
  projectId,
  storeId: data.credentialStoreId,
  key: credentialKeyToSet,
  value: credentialValueToSet,
  metadata: data.metadata,
});

newCredential = await findOrCreateCredential(tenantId, projectId, {
  id: newCredentialId,
  createdBy: user?.email ?? undefined,
  name: data.name.trim(),
  type: data.credentialStoreType,
  credentialStoreId: data.credentialStoreId,
  retrievalParams,
});

if (data.selectedTool && newCredential) {
  const updatedTool = {
    credentialReferenceId: newCredential.id,
  };
  await updateMCPTool(tenantId, projectId, data.selectedTool, updatedTool);
}

Commit: 404dd51

View Full Run


Tell us how we did: Give Ito Feedback

Merged via the queue into main with commit daee0b6 Apr 4, 2026
28 checks passed
@robert-inkeep robert-inkeep deleted the revert-dee33 branch April 4, 2026 21:27
@github-actions
Copy link
Copy Markdown
Contributor

github-actions bot commented Apr 4, 2026

🔎💬 Inkeep AI search and chat service is syncing content for source 'Inkeep Agent Framework Docs'

@claude claude bot mentioned this pull request Apr 5, 2026
github-merge-queue bot pushed a commit that referenced this pull request Apr 5, 2026
* revert

* Create beige-bottles-tease.md
@pullfrog pullfrog bot mentioned this pull request Apr 5, 2026
@inkeep-internal-ci inkeep-internal-ci bot mentioned this pull request Apr 5, 2026
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.

1 participant