copy agent#2934
Conversation
![pullfrog[bot]](https://avatars.githubusercontent.com/in/1768019?s=60&v=4){kind=small}
There was a problem hiding this comment.
Solid feature implementation — agent copy (same-project duplicate and cross-project import) with well-structured data access, proper authorization checks, and comprehensive test coverage. A few issues worth addressing before merge, most notably a variable shadowing bug in the import handler and a docs/UI label mismatch.
Claude Opus ｜ 𝕏
|
|
||
| if (process.env.ENVIRONMENT !== 'test') { | ||
| const userId = c.get('userId'); | ||
| const tenantId = c.get('tenantId'); |
There was a problem hiding this comment.
Bug: variable shadowing. const tenantId = c.get('tenantId') shadows the tenantId from c.req.valid('param') on line 61. This means the returned tenantId (line 100) comes from the validated path param, but the canViewProject call on line 84 uses the middleware-injected value. If these ever diverge (e.g. a middleware bug or a crafted request), authorization could silently check the wrong tenant. Remove the inner declaration and reuse the outer tenantId.
| const tenantId = c.get('tenantId'); | |
| const userId = c.get('userId'); | |
| if (!userId || !tenantId) { |
| export const importAgentHandler = async (c: ImportAgentHandlerContext) => { | ||
| const { tenantId, projectId } = c.req.valid('param'); | ||
| const body = c.req.valid('json'); | ||
|
|
||
| if (body.sourceProjectId === projectId) { | ||
| throw createApiError({ | ||
| code: 'bad_request', | ||
| message: | ||
| 'Source and target project must differ. Use /duplicate to copy within the same project.', | ||
| }); | ||
| } | ||
|
|
||
| if (process.env.ENVIRONMENT !== 'test') { |
There was a problem hiding this comment.
Architecture concern: process.env.ENVIRONMENT branching. The import handler uses process.env.ENVIRONMENT !== 'test' in two places — once here to skip auth, and again in the route body to skip Doltgres branching. This pattern isn't used elsewhere in the codebase and creates a production code path that's untestable in integration tests. Consider extracting the canViewProject check into a middleware or injecting a canViewSourceProject callback through the handler context so tests can supply a stub without relying on a global env check.
| 1. Open the agent's menu and select **Duplicate**. | ||
| 2. Choose the **Target project**. The current project is selected by default. | ||
| 3. Change the suggested name and ID. | ||
| 4. Click **Copy agent**. |
There was a problem hiding this comment.
The docs say Click **Copy agent** but the actual button in duplicate-agent-section.tsx reads "Duplicate agent". Update the docs to match the UI label.
| (value) => value || undefined, | ||
| DuplicateAgentRequestSchema.shape.newAgentName | ||
| ), | ||
| }); |
There was a problem hiding this comment.
The DuplicateAgentFormSchema wraps newAgentName in z.preprocess to coerce falsy values to undefined, but DuplicateAgentRequestSchema.shape.newAgentName is already .optional(). Unless there's a specific edge case with empty-string form inputs, the preprocess layer is redundant. If it IS needed (react-hook-form sends "" instead of undefined), a brief inline comment would help future readers understand why.
| await createSkill(params.targetDb)({ | ||
| tenantId: params.targetScopes.tenantId, | ||
| projectId: params.targetScopes.projectId, | ||
| name: params.sourceSkill.name, | ||
| description: params.sourceSkill.description, | ||
| content: params.sourceSkill.content, | ||
| metadata: params.sourceSkill.metadata ?? null, | ||
| files: normalizeSkillFiles(params.sourceSkill.files), | ||
| }); |
There was a problem hiding this comment.
Fragile implicit assumption: createSkill derives id from name (id: data.name), so the imported skill will match the agent's skill references only if sourceSkill.id === sourceSkill.name. This is currently true because createSkill always sets id = name, but this coupling isn't enforced by types and would break silently if skill ID generation changes. Consider passing id: params.sourceSkill.id explicitly if the createSkill signature allows it, or add an assertion sourceSkill.id === sourceSkill.name as a guard.
| try { | ||
| return await targetDb.transaction(async (tx) => { |
There was a problem hiding this comment.
The entire import operation runs inside targetDb.transaction(), which is good for atomicity. However, loadReferencedSourceDependencies (line 668) reads from sourceDb outside the transaction. If a concurrent write mutates the source project between the read and the transactional write, the import could use stale data. This is likely acceptable given the intended workflow (point-in-time copy), but worth documenting as a known trade-off.
| }); | ||
|
|
||
| const { isSubmitting } = form.formState; | ||
| const { data: projects, isError: projectsError } = useProjectsQuery({ |
There was a problem hiding this comment.
Nit: useProjectsQuery destructures data and isError, but projects (from data) is accessed directly without a guard for loading/error state. If the query is still loading, projects could be undefined, and currentProject, otherProjects, etc. would fail. The enabled: isOpen guard helps, but the initial render when isOpen flips to true could briefly hit this. Confirm that useProjectsQuery returns a stable empty array default for data.
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
PR Review Summary
(0) New Issues | (5) Pending from Prior Review | Risk: Medium
Delta Review: This review covers changes since the prior automated review (commits
98f297a6..73a57389). The Ito test failure (race condition on rapid double-submit) correlates with the TOCTOU issue flagged previously.
Delta Changes Analysis
The delta consists primarily of a UI architecture refactor that moves the duplicate functionality from a modal within "New Agent" to a dedicated dialog accessible from each agent's context menu:
| File | Change | Assessment |
|---|---|---|
duplicate-agent-section.tsx |
Refactored DuplicateAgentSection → DuplicateAgentDialog; props now include source agent info directly |
✅ Clean — proper state reset in handleOpenChange, React Compiler-compliant |
import-agent-section.tsx |
Deleted | ✅ Consolidation into DuplicateAgentDialog |
new-agent-item.tsx |
Simplified to only "Create blank agent" | ✅ Cleaner UX |
agent-item-menu.tsx |
Added "Duplicate" menu action | ✅ Follows existing patterns |
field.tsx |
Deleted (no longer needed) | ✅ Cleanup |
No new issues introduced in the delta. The refactored dialog properly resets state on close, follows React patterns, and maintains accessibility (DialogTitle + DialogDescription present).
🕐 Pending Recommendations (5)
The following issues from the prior review remain unaddressed:
- 🟠
agent.ts:83API key users bypass source project authorization —!userId.startsWith('apikey:')skipscanViewProjectcheck for API keys - 🟠
agentDuplicate.ts:44-54TOCTOU race condition — existence check and create not wrapped in transaction (Ito test confirms this manifests as intermittent 500s on rapid double-submit) - 🟠
testsMissing test coverage for race conditions and dependency conflicts beyond tools - 🟡
agentImport.ts:1HTTPException import breaks data-access layer boundary — DAL should usecreateApiError, not Hono's HTTPException - 🟡
agentImport.ts:148-244Sequential dependency loading could be parallelized per AGENTS.md performance guidelines
Resolved since prior review:
- ✅
buildWarningSummaryduplication —import-agent-section.tsxwas deleted, eliminating the duplicate
🚫 REQUEST CHANGES
Summary: The delta changes (UI refactor) are clean and well-implemented. However, 5 issues from the prior review remain unaddressed, including 3 Major findings:
- API key authorization bypass — API keys can import from any project in the tenant without explicit view permission. Verify this is intentional and document if so, or add project-level authz.
- TOCTOU race condition — The Ito test confirms rapid concurrent requests can hit intermittent 500s. Wrap existence check + create in a transaction (as done in
agentImport.ts). - Missing test coverage — Add tests for the race condition path and dependency conflict scenarios for non-tool resources.
The Minor items (HTTPException import, sequential loading) can be addressed in follow-up but don't block merge.
Discarded (1)
| Location | Issue | Reason Discarded |
|---|---|---|
duplicate-agent-section.tsx:91 |
State initialization from props without sync | LOW confidence; properly handled by handleOpenChange reset; each dialog instance has stable props |
Reviewers (2)
| Reviewer | Returned | Main Findings | Consider | While You're Here | Inline Comments | Pending Recs | Discarded |
|---|---|---|---|---|---|---|---|
pr-review-frontend |
1 | 0 | 0 | 0 | 0 | 0 | 1 |
pr-review-standards |
0 | 0 | 0 | 0 | 0 | 0 | 0 |
| Total | 1 | 0 | 0 | 0 | 0 | 5 | 1 |
Note: Delta review scoped to changes since 98f297a6. Prior findings carried forward as Pending Recommendations.
Ito Test Report ✅16 test cases ran. 16 passed. Across a unified run of 16 test cases, all passed (16/16) with zero failures, and the duplicate/import workflows behaved consistently with implemented API and UI logic without surfacing a likely production defect. The most important findings were that same-project duplicate and cross-project import (including mobile) redirected correctly while preserving source agents, edge guards returned expected 400/404/409 responses (including same-project import rejection and missing/existing ID handling), dialog degraded-mode and state-reset behavior were intentional/correct, concurrency/race and two-tab conflicts produced only one final copy, and security/authorization checks held with malicious input blocked, unauthenticated access returning 401/login redirect, and hidden source projects protected by not-found semantics. ✅ Passed (16)
Commit: Tell us how we did: Give Ito Feedback |
|
This pull request has been automatically marked as stale because it has not had recent activity. If this PR is still relevant:
Thank you for your contributions! |
|
This pull request has been automatically closed due to inactivity. If you'd like to continue working on this, please:
Thank you for your understanding! |
|
Thanks for the contribution! Your PR has been mirrored into Inkeep's internal monorepo, where review and merge happen. Your commit attribution is preserved as @dimaMachina. What happens next:
For Inkeep maintainers (link goes to a private repo and is not accessible to external contributors): https://github.com/inkeep/agents-private/pull/1650 See the repository's CONTRIBUTING.md for more context on how public PRs flow. This comment will be updated as the bridge state changes. |
There was a problem hiding this comment.
Caution
This PR opens a cross-project data-read path for API-key callers: the new import endpoint never authorizes the body-supplied sourceProjectId, so a project-scoped key can read any other project's full agent definition in the same tenant. See the inline comment on agent.ts.
Reviewed changes — adds same-project agent duplication (POST /agents/{agentId}/duplicate) and cross-project import (POST /agents/import) across agents-core, the manage API, and the dashboard UI.
- Cross-project import service —
importFullAgentServerSide(agentImport.ts) loads a source agent's project-scoped dependencies (tools, external agents, data/artifact components, functions, skills), recreates any missing ones in the target inside a transaction, errors on differing-config conflicts, and emitscredential_missingwarnings. - Same-project duplication —
duplicateFullAgentServerSide(agentDuplicate.ts) clones a full agent within one project viabuildCopiedAgentDefinition. - Portability helpers —
agentPortability.tsstrips non-portable root keys (tools,externalAgents,teamAgents,functions,triggers), normalizes transfer/delegate targets, and collects referenced dependency IDs. - Routes + authz — both endpoints require
editon the target; import additionally checkscanViewProjecton the source, with a test-environment branch that uses a single DB client and skips the source authz check. - Validation schemas —
DuplicateAgentRequestSchema,ImportAgentRequestSchema,ImportAgentWarningSchema,ImportAgentResponseSchema. - Dashboard UI —
duplicate-agent-section.tsx(radio of duplicate vs. cross-project import), import section component, server actions, API client functions, and sharedField/comboboxcomponents. - Tests + docs — unit + route tests for both flows, plus a "Copying an Agent" docs section.
⚠️ Production cross-project read path and source-project authorization are untested
The import route handler branches on process.env.ENVIRONMENT === 'test'. In tests it calls importFullAgentServerSide(db, db) — the same DB client for source and target — and the source-project canViewProject check is wrapped in the same ENVIRONMENT !== 'test' guard. The production branch that actually reads the source project from its Doltgres main-branch ref (withRef + getProjectMainResolvedRef) and enforces source authorization is therefore exercised by no test at all.
- The single-DB shortcut means tests never validate that
sourceDbreads from a different ref thantargetDb— the core mechanism of cross-project import. - The only coverage of the source authz check is a unit test that calls
importAgentHandlerdirectly withENVIRONMENTforced todevelopment; the route-level happy-path test uses an agent with no project-scoped dependencies, soensureReferencedDependenciesInTargetProjectis never driven through the HTTP layer.
Technical details
# Production cross-project read path and source authorization are untested
## Affected sites
- `agents-api/src/domains/manage/routes/agent.ts:437-456` — `ENVIRONMENT === 'test'` selects `importFullAgentServerSide(db, db)`; production `withRef`/`getProjectMainResolvedRef` branch is never hit in CI.
- `agents-api/src/domains/manage/routes/agent.ts:77-103` — source `canViewProject` check is skipped entirely under `ENVIRONMENT === 'test'`.
- `agents-api/src/__tests__/manage/routes/crud/agent.test.ts` — route-level import happy path uses a minimal agent (`canUse: []`, no deps), so dependency recreation is not covered via the route.
- `agents-api/src/__tests__/manage/data/agentImport.test.ts` — conflict-detection coverage exists only for the tool case; `externalAgent`, `dataComponent`, `artifactComponent`, `function`, and `skill` conflict branches are untested.
## Required outcome
- Cross-project reads via distinct source/target refs are exercised by at least one test (or the test-only single-DB branch is justified as faithful to production).
- The source-project authorization decision (allow + deny, including the `apikey:` path) is covered.
- Conflict detection is covered for resource types beyond tools, or the shared helper is tested once generically.
## Open questions for the human
- Is the `ENVIRONMENT === 'test'` branch in production route code a deliberate seam, or should the source-DB acquisition be injected so tests can cover the real dual-ref path without env branching?ℹ️ Nitpicks
- Both new test files define inline logger mocks (
agentDuplicate.test.ts:34, and the equivalent inagentImport.test.ts);CLAUDE.mdmandatescreateMockLoggerModule()from@inkeep/agents-core/test-utilsto avoid drift when the logger interface changes.
Claude Opus ｜ 𝕏
| }); | ||
| } | ||
|
|
||
| if (userId !== 'system' && !userId.startsWith('apikey:')) { |
There was a problem hiding this comment.
🚨 apikey: and system callers skip the source-project access check, but sourceProjectId arrives in the request body, not the path. The upstream API-key validation (manageAuth.ts:247) only checks the key against the path project — the import target — so the source project is never authorized for these callers. A project-scoped API key with edit on the target can read any other project's full agent definition in the same tenant: tool configs, function executeCode, skill content, data-component props, and external-agent URLs.
Technical details
# API-key callers can read any source project via import
## Affected sites
- `agents-api/src/domains/manage/routes/agent.ts:88` — `if (userId !== 'system' && !userId.startsWith('apikey:'))` skips `canViewProject` on `sourceProjectId` for key/system callers.
- `agents-api/src/middleware/manageAuth.ts:247` — API-key project validation compares the key's `projectId` to the *path* project (`extractProjectIdFromPath`), i.e. the import target, never the body's `sourceProjectId`.
- `agents-api/src/middleware/projectAccess.ts:57` — target `requireProjectPermission('edit')` also bypasses for key/system, so nothing else covers the source.
- `packages/agents-core/src/data-access/manage/agentImport.ts:286-291` — the `credential_missing` warning echoes the source project's `credentialReferenceId` back to the caller, compounding the cross-project leak.
## Required outcome
- A caller authenticated by a project-scoped API key must not be able to read a `sourceProjectId` it has no access to. Either authorize the source for key callers (e.g. require the key's project to equal `sourceProjectId`) or reject cross-project import for API-key auth entirely.
## Open questions for the human
- Should API-key callers be permitted to perform cross-project import at all, given keys are single-project scoped? If not, the cleanest fix is to deny the key/system bypass for the *source* check specifically.| if ( | ||
| !areNormalizedValuesEqual( | ||
| normalizeTool(params.sourceTool, credentialReferenceId), | ||
| normalizeTool(existingTool, existingTool.credentialReferenceId ?? null) |
There was a problem hiding this comment.
credential_missing warnings and imports tools "disconnected"). But normalizeTool folds credentialReferenceId into the equality check: when the same tool id already exists in the target with its own credential connected, the source side resolves to null (credential missing in target) while the existing side carries a real id, so this comparison throws a spurious 409. In practice almost any pre-existing credentialed tool will collide on import.
Technical details
# Tool conflict false-positive on project-scoped credentials
## Affected sites
- `packages/agents-core/src/data-access/manage/agentImport.ts:70-79` — `normalizeTool` includes `credentialReferenceId`.
- `packages/agents-core/src/data-access/manage/agentImport.ts:316-327` — conflict check compares `normalizeTool(sourceTool, <resolved, often null>)` against `normalizeTool(existingTool, existingTool.credentialReferenceId)`.
## Required outcome
- An identically-configured tool that already exists in the target should be reused (not 409) regardless of credential linkage, consistent with the documented "credentials are not copied" behavior.
## Suggested approach
- Exclude `credentialReferenceId` from the tool/external-agent equality fingerprint, or compare credential linkage only when both sides resolve to a present credential. Same reasoning applies to `normalizeExternalAgent`.|
|
||
| if (process.env.ENVIRONMENT !== 'test') { | ||
| const userId = c.get('userId'); | ||
| const tenantId = c.get('tenantId'); |
There was a problem hiding this comment.
ℹ️ This const tenantId shadows the path-param tenantId destructured at line 67. The authz check below uses this context-derived value, while the value returned from the handler (and used for the actual import) is the path-param one. Upstream tenantAccess middleware guarantees the two match today, so this is a latent footgun rather than a live bug — reuse the outer binding instead of re-declaring it.
Ito Diff Report ❌Tested: Across 18 end-to-end and code-verified scenarios, 17 passed and 1 failed, showing that import/duplicate behavior is mostly correct: divergent config and duplicate-ID cases return deterministic 409s, missing dependencies are created, credential logic preserves shared links or nulls missing references with aggregated warnings, sanitizer rules remove non-portable and trigger metadata while normalizing relation payloads, authorization guards correctly mask unauthorized source-project access and return 401 without user context, and UI duplicate/import flows stayed stable under rapid submits and late project switches. ❌ Failures (1)
🟠 Concurrent import conflict returns server error
Relevant code:
return await targetDb.transaction(async (tx) => {
const existingTargetAgent = await getAgentById(tx)({
scopes: { tenantId, projectId: targetProjectId, agentId: newAgentId },
});
if (existingTargetAgent) {
throw createApiError({
code: 'conflict',
message: `An agent with ID '${newAgentId}' already exists`,
});
} catch (error) {
if (!(error instanceof HTTPException)) {
throwIfUniqueConstraintError(error, `An agent with ID '${newAgentId}' already exists`);
}
export function isUniqueConstraintError(error: unknown): boolean {
const err = error as
| { cause?: { code?: string; message?: string }; message?: string }
| null
| undefined;
return (
err?.cause?.code === '23505' ||
!!err?.cause?.message?.includes('1062') ||
!!err?.message?.includes('already exists')
);
}✅ Verified Passing (17)
↪️ Inherited from Prior Run (16)
Tell us how we did: Give Ito Feedback |
|
Claude encountered an error —— View job PR Review in Progress
|
Ito Diff Report ❌Tested: Across 14 test cases, 12 passed and 2 failed, with successful verification of conversation filtering and read consistency, model-resolution and recovery behavior, MCP prompt rendering rules, and end-to-end project model create/edit workflows including rapid-submit deduplication. The two important remaining medium-severity defects are an agent import race where the losing concurrent request can return 500 internal_server_error instead of a deterministic 409 conflict, and a stale/deleted appId path in GET /run/v1/conversations that returns 401 Invalid Token instead of the expected App not found (404/not_found) response. ❌ Failures (2)
🟠 Stale app identifier returns wrong auth error
Relevant code:
const app = await getAppById(runDbClient)(appIdHeader);
if (!app) {
return { authResult: null, failureMessage: 'App not found' };
}
if (reqData.appId && !subAgentId) {
if (!apiKey) {
return { authResult: null, failureMessage: 'Bearer token required for app credential auth' };
}
return tryAppCredentialAuth(reqData);
}
if (!attempt.authResult) {
logger.error(
{ failureMessage: attempt.failureMessage },
'API key authentication error - no valid auth method found'
);
throw new HTTPException(401, {
message: 'Invalid Token',
});
}
if (appId) {
const appRecord = await getAppByIdForProject(runDbClient)({
scopes: { tenantId, projectId },
id: appId,
});
if (!appRecord) {
throw createApiError({
code: 'not_found',
message: 'App not found',
});
}
}🟠 Concurrent agent import returns 500 instead of conflict
Relevant code:
return await targetDb.transaction(async (tx) => {
const existingTargetAgent = await getAgentById(tx)({
scopes: { tenantId, projectId: targetProjectId, agentId: newAgentId },
});
if (existingTargetAgent) {
throw createApiError({
code: 'conflict',
message: `An agent with ID '${newAgentId}' already exists`,
});
}
const importedAgent = await createFullAgentServerSide(tx, logger)(
targetProjectScopes,
importedAgentDefinition
);
return {
data: importedAgent as ImportAgentResponse['data'],
warnings,
};
});
} catch (error) {
if (!(error instanceof HTTPException)) {
throwIfUniqueConstraintError(error, `An agent with ID '${newAgentId}' already exists`);
}
throw error;
}
export function isUniqueConstraintError(error: unknown): boolean {
const err = error as
| { cause?: { code?: string; message?: string }; message?: string }
| null
| undefined;
return (
err?.cause?.code === '23505' ||
!!err?.cause?.message?.includes('1062') ||
!!err?.message?.includes('already exists')
);
}✅ Verified Passing (12)
↪️ Inherited from Prior Run (17)
Tell us how we did: Give Ito Feedback |
|
This pull request has been automatically marked as stale because it has not had recent activity. If this PR is still relevant:
Thank you for your contributions! |
|
@dimaMachina can you look at the "TOCTOU" issue that has been flagged in the PR comments? |
2 participants
Agent Duplicate & Cross-Project Import
Adds two new agent copy flows — same-project duplication and cross-project import — across the API, data access layer, and dashboard UI.
New API Endpoints
POST/agents/{agentId}/duplicatePOST/agents/importCore Logic (
agents-core)agentDuplicate.ts—duplicateFullAgentServerSide: clones a full agent definition (viabuildCopiedAgentDefinition) within the same project. Validates the new ID differs from the source and that no agent with that ID already exists.agentImport.ts—importFullAgentServerSide: copies an agent from a source project into a target project inside a transaction. Loads all referenced project-scoped dependencies (tools, external agents, data components, artifact components, functions, skills) from the source, then ensures each exists in the target — creating them if missing, erroring on conflicts with differing configuration. Normalizes resource shapes for stable equality comparison. Producescredential_missingwarnings when imported tools or external agents reference credentials absent in the target project.agentPortability.ts— Shared helpers:buildCopiedAgentDefinitionstrips non-portable root keys (tools,externalAgents,teamAgents,functions,triggers), normalizes transfer/delegate targets, and re-validates throughvalidateAndTypeAgentData.collectReferencedDependencyIdswalks sub-agents to collect all dependency IDs needed for cross-project import.Validation Schemas
New Zod schemas and inferred types in
agents-core:DuplicateAgentRequestSchema—{ newAgentId, newAgentName? }ImportAgentRequestSchema—{ sourceProjectId, sourceAgentId, newAgentId, newAgentName? }ImportAgentWarningSchema—{ code: 'credential_missing', resourceType, resourceId, credentialReferenceId }ImportAgentResponseSchema—{ data, warnings }Authorization
editpermission on the target project.viewaccess on the source project (viacanViewProject).withRef/getProjectMainResolvedRef.Dashboard UI (
agents-manage-ui)new-agent-item.tsx— "New Agent" dialog now presents a radio group: Create blank agent or Copy existing agent.duplicate-agent-section.tsx— Renders the copy form. When the selected source project matches the current project, calls the duplicate endpoint. When a different project is selected, calls the import endpoint. Displays post-importcredential_missingwarnings.import-agent-section.tsx— Standalone import form component for the cross-project flow with project/agent combobox selectors and auto-prefilled ID/name.duplicateAgentAction,importAgentAction) and API client functions added for both flows.FieldUI components (Field,FieldContent,FieldTitle,FieldDescription,FieldLabel) used in the radio group layout.Documentation
agents-docs/content/visual-builder/sub-agents.mdx— New "Copying an Agent" section documenting both same-project and cross-project copy flows, including behavior around triggers, credentials, and team-agent limitations.Tests
agentDuplicate.test.ts— Unit tests for same-project duplication (success, ID collision, same-ID rejection).agentImport.test.ts— Unit tests for cross-project import covering dependency recreation, conflict detection, credential warnings, team-agent rejection, and same-project guard.agent.test.ts— Route-level CRUD tests for both/duplicateand/importendpoints.Claude Opus｜ 𝕏