feat(server): de-duplicate imported company names (#12145)
## Thinking Path > - Paperclip is the open source app people use to manage AI agents for work > - Importing a company package as a new company takes the company name from the package manifest > - Repeat imports of the same package therefore create several identically named companies, distinguishable only by issue prefix > - Users cannot tell which import they are looking at, which feeds the "my import disappeared" loop of importing again > - This pull request suffixes manifest-derived names with " (2)", " (3)", … on collision, while honoring explicitly typed names verbatim > - The benefit is that every imported company has a recognizable name ## Linked Issues or Issue Description **What existing behavior does this improve?** Naming of companies created by the company package import. **Subsystem affected** Server — company import (`server/src/services/company-portability.ts`). **Current behavior** The new-company branch uses `newCompanyName ?? manifest name ?? "Imported Company"` with no de-duplication. Only the issue prefix is unique. Three imports of the same package yield three companies with the same name. **Proposed behavior** When the name comes from the manifest (no explicit `newCompanyName`), the import checks existing company names case-insensitively and appends the first free " (N)" suffix. Explicit names remain honored verbatim. Name exhaustion (thousands of collisions) falls back to the base name rather than failing the import, since names carry no uniqueness invariant. **Breaking changes** None. Only the default name of newly imported companies changes, and only on collision. ## What Changed - New exported pure helper `dedupeImportedCompanyName(baseName, existingNames)`. - The new-company branch resolves the name through it when no explicit name was provided, reading existing names via `companyService.list()`. ## Verification - `cd server && npx vitest run src/__tests__/company-portability.test.ts` — 87 tests pass (new: pure helper cases and two `importBundle` tests for the suffixed manifest name and the honored explicit name). - `cd server && npx vitest run src/__tests__/company-portability-routes.test.ts src/__tests__/company-portability-import-batching.test.ts` — 44 passed, 1 skipped (pre-existing skip). - `cd server && pnpm run typecheck` — clean. ## Risks - Low risk. The check-then-create has a theoretical race with a concurrent import, but names have no unique constraint — the worst case is today's behavior (a duplicate name). Issue-prefix uniqueness is untouched. ## Model Used - Claude Fable 5 (`claude-fable-5`, Anthropic) with extended thinking and tool use, via Claude Code. ## Checklist - [x] I have included a thinking path that traces from project context to this change - [x] I have specified the model used (with version and capability details) - [x] I have checked ROADMAP.md and confirmed this PR does not duplicate planned core work - [x] I have searched GitHub for duplicate or related PRs and linked them above - [x] I have either (a) linked existing issues with `Fixes: #` / `Closes #` / `Refs #` OR (b) described the issue in-PR following the relevant issue template - [x] I have not referenced internal/instance-local Paperclip issues or links (only public GitHub `#NNN` / `github.com/paperclipai/paperclip` URLs) - [x] My branch name describes the change (e.g. `docs/...`, `fix/...`) and contains no internal Paperclip ticket id or instance-derived details - [x] I have run tests locally and they pass - [x] I have added or updated tests where applicable - [x] I have updated relevant documentation to reflect my changes - [x] I have considered and documented any risks above - [ ] All Paperclip CI gates are green - [ ] Greptile is 5/5 with no open P2s, recommendations, or follow-ups - [x] I will address all Greptile and reviewer comments before requesting merge
This commit is contained in:
Devin Foley
GitHub
parent
fcb84d472d
commit
18b6c788d5
|
|
@ -9,6 +9,10 @@ import type { CompanyPortabilityFileEntry } from "@paperclipai/shared";
|
|||
|
||||
const companySvc = {
|
||||
getById: vi.fn(),
|
||||
// Async-empty default (not a bare vi.fn()): every new-company import reads
|
||||
// the existing names for de-duplication, including describes that never
|
||||
// touch this mock.
|
||||
list: vi.fn(async () => []),
|
||||
create: vi.fn(),
|
||||
update: vi.fn(),
|
||||
};
|
||||
|
|
@ -154,7 +158,7 @@ vi.mock("../routes/org-chart-svg.js", () => ({
|
|||
renderOrgChartPng: vi.fn(async () => Buffer.from("png")),
|
||||
}));
|
||||
|
||||
const { companyPortabilityService, parseGitHubSourceUrl, renderYamlBlock, renderFrontmatter } = await import("../services/company-portability.js");
|
||||
const { companyPortabilityService, dedupeImportedCompanyName, parseGitHubSourceUrl, renderYamlBlock, renderFrontmatter } = await import("../services/company-portability.js");
|
||||
|
||||
function asTextFile(entry: CompanyPortabilityFileEntry | undefined) {
|
||||
expect(typeof entry).toBe("string");
|
||||
|
|
@ -184,6 +188,7 @@ describe("company portability", () => {
|
|||
presentation: null,
|
||||
metadata: null,
|
||||
});
|
||||
companySvc.list.mockResolvedValue([]);
|
||||
companySvc.getById.mockResolvedValue({
|
||||
id: "company-1",
|
||||
name: "Paperclip",
|
||||
|
|
@ -2609,6 +2614,93 @@ describe("company portability", () => {
|
|||
]);
|
||||
});
|
||||
|
||||
it("suffixes a manifest-derived company name that collides with an existing company", async () => {
|
||||
const portability = companyPortabilityService({} as any);
|
||||
|
||||
companySvc.list.mockResolvedValue([
|
||||
{ name: "Imported Paperclip" },
|
||||
// Case-insensitive: an existing "(2)" in any casing blocks that suffix.
|
||||
{ name: "imported paperclip (2)" },
|
||||
]);
|
||||
companySvc.create.mockResolvedValue({
|
||||
id: "company-imported",
|
||||
name: "Imported Paperclip (3)",
|
||||
});
|
||||
accessSvc.ensureMembership.mockResolvedValue(undefined);
|
||||
|
||||
const files = {
|
||||
"COMPANY.md": ["---", 'schema: "agentcompanies/v1"', 'name: "Imported Paperclip"', "---", ""].join("\n"),
|
||||
};
|
||||
|
||||
await portability.importBundle({
|
||||
source: { type: "inline", rootPath: "paperclip-demo", files },
|
||||
include: { company: true, agents: false, projects: false, issues: false },
|
||||
// No newCompanyName: the manifest name is used and must be de-duplicated.
|
||||
target: { mode: "new_company" },
|
||||
collisionStrategy: "rename",
|
||||
}, "user-1");
|
||||
|
||||
expect(companySvc.create).toHaveBeenCalledWith(expect.objectContaining({
|
||||
name: "Imported Paperclip (3)",
|
||||
}));
|
||||
});
|
||||
|
||||
it("skips name de-duplication for agent-safe imports so collisions stay unobservable", async () => {
|
||||
const portability = companyPortabilityService({} as any);
|
||||
|
||||
companySvc.list.mockResolvedValue([{ name: "Imported Paperclip" }]);
|
||||
companySvc.create.mockResolvedValue({
|
||||
id: "company-imported",
|
||||
name: "Imported Paperclip",
|
||||
});
|
||||
accessSvc.listActiveUserMemberships.mockResolvedValue([{ userId: "user-1" }]);
|
||||
accessSvc.copyActiveUserMemberships.mockResolvedValue([]);
|
||||
|
||||
const files = {
|
||||
"COMPANY.md": ["---", 'schema: "agentcompanies/v1"', 'name: "Imported Paperclip"', "---", ""].join("\n"),
|
||||
};
|
||||
|
||||
await portability.importBundle({
|
||||
source: { type: "inline", rootPath: "paperclip-demo", files },
|
||||
include: { company: true, agents: false, projects: false, issues: false },
|
||||
target: { mode: "new_company" },
|
||||
collisionStrategy: "rename",
|
||||
}, "user-1", { mode: "agent_safe", sourceCompanyId: "company-1" });
|
||||
|
||||
// The instance-wide name list must never be consulted for a
|
||||
// company-scoped agent, and no suffix may reflect a collision back.
|
||||
expect(companySvc.list).not.toHaveBeenCalled();
|
||||
expect(companySvc.create).toHaveBeenCalledWith(expect.objectContaining({
|
||||
name: "Imported Paperclip",
|
||||
}));
|
||||
});
|
||||
|
||||
it("honors an explicitly typed company name even when it collides", async () => {
|
||||
const portability = companyPortabilityService({} as any);
|
||||
|
||||
companySvc.list.mockResolvedValue([{ name: "Imported Paperclip" }]);
|
||||
companySvc.create.mockResolvedValue({
|
||||
id: "company-imported",
|
||||
name: "Imported Paperclip",
|
||||
});
|
||||
accessSvc.ensureMembership.mockResolvedValue(undefined);
|
||||
|
||||
const files = {
|
||||
"COMPANY.md": ["---", 'schema: "agentcompanies/v1"', 'name: "Imported Paperclip"', "---", ""].join("\n"),
|
||||
};
|
||||
|
||||
await portability.importBundle({
|
||||
source: { type: "inline", rootPath: "paperclip-demo", files },
|
||||
include: { company: true, agents: false, projects: false, issues: false },
|
||||
target: { mode: "new_company", newCompanyName: "Imported Paperclip" },
|
||||
collisionStrategy: "rename",
|
||||
}, "user-1");
|
||||
|
||||
expect(companySvc.create).toHaveBeenCalledWith(expect.objectContaining({
|
||||
name: "Imported Paperclip",
|
||||
}));
|
||||
});
|
||||
|
||||
it("pauses imported agents and routines when pauseAutomations is requested", async () => {
|
||||
const portability = companyPortabilityService({} as any);
|
||||
|
||||
|
|
@ -5717,3 +5809,20 @@ describe("company portability", () => {
|
|||
expect(preview.plan.issuePlans).toHaveLength(0);
|
||||
});
|
||||
});
|
||||
|
||||
describe("dedupeImportedCompanyName", () => {
|
||||
it("returns the base name when nothing collides", () => {
|
||||
expect(dedupeImportedCompanyName("Paperclip", ["Other Co"])).toBe("Paperclip");
|
||||
expect(dedupeImportedCompanyName("Paperclip", [])).toBe("Paperclip");
|
||||
});
|
||||
|
||||
it("suffixes past every taken candidate, case-insensitively", () => {
|
||||
expect(dedupeImportedCompanyName("Paperclip", ["paperclip"])).toBe("Paperclip (2)");
|
||||
expect(dedupeImportedCompanyName("Paperclip", ["Paperclip", "Paperclip (2)"])).toBe("Paperclip (3)");
|
||||
expect(dedupeImportedCompanyName("Paperclip", ["PAPERCLIP", "paperclip (2)"])).toBe("Paperclip (3)");
|
||||
});
|
||||
|
||||
it("ignores surrounding whitespace in existing names", () => {
|
||||
expect(dedupeImportedCompanyName("Paperclip", [" Paperclip "])).toBe("Paperclip (2)");
|
||||
});
|
||||
});
|
||||
|
|
|
|||
|
|
@ -229,6 +229,25 @@ function assertInlineSourceComplete(source: CompanyPortabilityImport["source"])
|
|||
}
|
||||
}
|
||||
|
||||
/**
|
||||
* Suffix a manifest-derived company name with " (2)", " (3)", … when it
|
||||
* collides case-insensitively with an existing company, so repeat imports of
|
||||
* the same package do not produce several identically named companies that
|
||||
* only differ by issue prefix. Explicit user-typed names bypass this — they
|
||||
* are the caller's deliberate choice.
|
||||
*/
|
||||
export function dedupeImportedCompanyName(baseName: string, existingNames: string[]): string {
|
||||
const normalized = new Set(existingNames.map((name) => name.trim().toLowerCase()));
|
||||
if (!normalized.has(baseName.trim().toLowerCase())) return baseName;
|
||||
for (let suffix = 2; suffix < 10_000; suffix += 1) {
|
||||
const candidate = `${baseName} (${suffix})`;
|
||||
if (!normalized.has(candidate.toLowerCase())) return candidate;
|
||||
}
|
||||
// Pathological: thousands of identically named companies. Give up on the
|
||||
// suffix rather than fail the import — names carry no uniqueness invariant.
|
||||
return baseName;
|
||||
}
|
||||
|
||||
function resolveSkillConflictStrategy(mode: ImportMode, collisionStrategy: CompanyPortabilityCollisionStrategy) {
|
||||
if (mode === "board_full") return collisionStrategy;
|
||||
return collisionStrategy === "skip" ? "skip" as const : "rename" as const;
|
||||
|
|
@ -5241,11 +5260,21 @@ export function companyPortabilityService(db: Db, storage?: StorageService) {
|
|||
throw unprocessable("Safe new-company import requires at least one active user membership on the source company.");
|
||||
}
|
||||
}
|
||||
const requestedCompanyName = asString(input.target.newCompanyName);
|
||||
const manifestCompanyName =
|
||||
sourceManifest.company?.name ?? sourceManifest.source?.companyName ?? "Imported Company";
|
||||
// De-duplicate only for board-driven imports. The lookup reads every
|
||||
// company name in the instance, and reflecting a collision back through
|
||||
// the numeric suffix would let a company-scoped agent (agent_safe mode)
|
||||
// probe for the existence of company names outside its own company.
|
||||
const companyName =
|
||||
asString(input.target.newCompanyName) ??
|
||||
sourceManifest.company?.name ??
|
||||
sourceManifest.source?.companyName ??
|
||||
"Imported Company";
|
||||
requestedCompanyName ??
|
||||
(mode === "agent_safe"
|
||||
? manifestCompanyName
|
||||
: dedupeImportedCompanyName(
|
||||
manifestCompanyName,
|
||||
(await companies.list()).map((company) => company.name),
|
||||
));
|
||||
const created = await companies.create({
|
||||
name: companyName,
|
||||
description: include.company ? (sourceManifest.company?.description ?? null) : null,
|
||||
|
|
|
|||
Loading…
Reference in New Issue