feat: already-imported transfer error names the landed company (#12144)
## Thinking Path
> - Paperclip is the open source app people use to manage AI agents for
work
> - Chunked company-import transfers are deduplicated by content: a
byte-identical zip that already finished an apply is rejected
> - The rejection said only "this exact package was already imported by
a completed transfer" without saying where that import went
> - Users who could not find the earlier import read the rejection as
data loss and kept retrying, or exported again and created duplicate
companies
> - This pull request makes the declaration response carry the company
the completed apply created, and both clients name it in the error
> - The benefit is that the dedupe rejection now points at the existing
import instead of implying it vanished
## Linked Issues or Issue Description
**What existing behavior does this improve?**
The `alreadyCompleted` rejection when re-declaring a chunked
company-import transfer.
**Subsystem affected**
Shared transfer contract
(`packages/shared/src/company-import-transfer.ts`), transfer declaration
route (`server/src/routes/companies.ts`), web import page, CLI import
command.
**Current behavior**
`POST /api/companies/import/transfers` returns `alreadyCompleted: true`
with no pointer to the earlier import. Web and CLI raise "This exact
package was already imported by a completed transfer. Re-export the
package to import it again."
**Proposed behavior**
The response includes an optional `company` field (`{id, name,
issuePrefix} | null`) resolved from the completed run's company link.
Web and CLI raise a shared message: `… It created the company
"Paperclip" (PAPA) — open it from the company switcher. Re-export the
package to import it again.` A company that was deleted since (or a link
that was never written) degrades to `null` and the original message.
**Breaking changes**
None. The new response field is optional; old clients ignore it.
## What Changed
- `CompanyImportTransferCreated` gains optional `company`, plus a shared
`buildAlreadyImportedMessage` used by both clients.
- The declaration route's `alreadyCompleted` branch resolves the landed
company null-safely via `companyService.getById`.
- Web (`ui/src/pages/CompanyImport.tsx`) and CLI
(`cli/src/commands/client/company.ts`) raise the shared message.
## Verification
- `cd packages/shared && npx vitest run
src/company-import-transfer.test.ts` — 3 tests (named company, id
fallback, no-company original message).
- `cd server && npx vitest run
src/__tests__/company-import-transfer-routes.test.ts` — 24 tests; the
re-declaration test now asserts the company payload and the
deleted-company null path.
- `cd cli && npx vitest run
src/__tests__/company-import-transfer.test.ts` — 17 tests; new test pins
the named-company message.
- `cd ui && npx vitest run src/pages/CompanyImport.test.tsx` — 23 tests.
- `pnpm run typecheck` clean in shared, server, ui, cli.
## Risks
- Low risk. The lookup runs only on the `alreadyCompleted` branch and is
null-safe; the transfer run is already scoped to the requesting actor
(user + instance context in the actor key), so the response never names
a company the caller did not import.
## 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:
parent
868e210a95
commit
fcb84d472d
|
|
@ -320,6 +320,24 @@ describe("uploadCompanyImportTransfer", () => {
|
|||
await expect(uploadCompanyImportTransfer(api, zipBytes)).rejects.toThrow(/already imported/);
|
||||
expect(putRaw).not.toHaveBeenCalled();
|
||||
});
|
||||
|
||||
it("names the company the completed transfer created", async () => {
|
||||
const { api, putRaw } = fakeApi({
|
||||
post: vi.fn().mockResolvedValue({
|
||||
transferId: "transfer-1",
|
||||
status: "completed",
|
||||
alreadyCompleted: true,
|
||||
totalParts: 2,
|
||||
missingParts: [],
|
||||
company: { id: "company-2", name: "Paperclip", issuePrefix: "PAPA" },
|
||||
}),
|
||||
});
|
||||
|
||||
await expect(uploadCompanyImportTransfer(api, zipBytes)).rejects.toThrow(
|
||||
/landed in the company "Paperclip" \(PAPA\)/,
|
||||
);
|
||||
expect(putRaw).not.toHaveBeenCalled();
|
||||
});
|
||||
});
|
||||
|
||||
describe("company import command over the chunked transfer path", () => {
|
||||
|
|
|
|||
|
|
@ -14,6 +14,7 @@ import type {
|
|||
CompanyPortabilityImportResult,
|
||||
} from "@paperclipai/shared";
|
||||
import {
|
||||
buildAlreadyImportedMessage,
|
||||
companyImportTransferApplyPath,
|
||||
companyImportTransferPartPath,
|
||||
companyImportTransferPreviewPath,
|
||||
|
|
@ -1160,9 +1161,9 @@ export async function uploadCompanyImportTransfer(
|
|||
if (created.alreadyCompleted) {
|
||||
// The server keys transfers by content, and this exact zip already
|
||||
// finished an apply — its spooled parts are gone, so it cannot re-run.
|
||||
throw new Error(
|
||||
"This exact package was already imported by a completed transfer. Re-export the package to import it again.",
|
||||
);
|
||||
// Name the company that apply created so the rejection points at the
|
||||
// existing import instead of reading as data loss.
|
||||
throw new Error(buildAlreadyImportedMessage(created.company));
|
||||
}
|
||||
const missing = new Set(created.missingParts);
|
||||
let uploadedParts = manifest.parts.length - missing.size;
|
||||
|
|
|
|||
|
|
@ -0,0 +1,29 @@
|
|||
import { describe, expect, it } from "vitest";
|
||||
import { buildAlreadyImportedMessage } from "./company-import-transfer.js";
|
||||
|
||||
describe("buildAlreadyImportedMessage", () => {
|
||||
it("names the landed company with its prefix", () => {
|
||||
expect(
|
||||
buildAlreadyImportedMessage({ id: "company-2", name: "Paperclip", issuePrefix: "PAPA" }),
|
||||
).toBe(
|
||||
'This exact package was already imported by a completed transfer. The earlier import landed in the company "Paperclip" (PAPA) — open it from the company switcher. Re-export the package to import it again.',
|
||||
);
|
||||
});
|
||||
|
||||
it("falls back to the company id when the name is gone and omits an absent prefix", () => {
|
||||
expect(
|
||||
buildAlreadyImportedMessage({ id: "company-2", name: null, issuePrefix: null }),
|
||||
).toBe(
|
||||
'This exact package was already imported by a completed transfer. The earlier import landed in the company "company-2" — open it from the company switcher. Re-export the package to import it again.',
|
||||
);
|
||||
});
|
||||
|
||||
it("keeps the original message when no company is known", () => {
|
||||
expect(buildAlreadyImportedMessage(null)).toBe(
|
||||
"This exact package was already imported by a completed transfer. Re-export the package to import it again.",
|
||||
);
|
||||
expect(buildAlreadyImportedMessage(undefined)).toBe(
|
||||
"This exact package was already imported by a completed transfer. Re-export the package to import it again.",
|
||||
);
|
||||
});
|
||||
});
|
||||
|
|
@ -78,6 +78,29 @@ export interface CompanyImportTransferCreated {
|
|||
alreadyCompleted: boolean;
|
||||
totalParts: number;
|
||||
missingParts: number[];
|
||||
/**
|
||||
* Where the prior completed apply landed, so an alreadyCompleted rejection
|
||||
* can point at the existing company instead of reading as data loss.
|
||||
* Null/absent when the run's company link was never written or the company
|
||||
* has since been deleted.
|
||||
*/
|
||||
company?: { id: string; name: string | null; issuePrefix: string | null } | null;
|
||||
}
|
||||
|
||||
/**
|
||||
* Human-facing message for an `alreadyCompleted` declaration. Shared by the
|
||||
* web and CLI clients so the copy (and the pointer to the landed company)
|
||||
* stays identical everywhere the rejection surfaces.
|
||||
*/
|
||||
export function buildAlreadyImportedMessage(
|
||||
company: CompanyImportTransferCreated["company"],
|
||||
): string {
|
||||
// "landed in", not "created": a completed transfer may have created a new
|
||||
// company or merged into an existing one, and the caller cannot tell which.
|
||||
const location = company
|
||||
? ` The earlier import landed in the company "${company.name ?? company.id}"${company.issuePrefix ? ` (${company.issuePrefix})` : ""} — open it from the company switcher.`
|
||||
: "";
|
||||
return `This exact package was already imported by a completed transfer.${location} Re-export the package to import it again.`;
|
||||
}
|
||||
|
||||
/** Response of the resume-polling GET for one transfer. */
|
||||
|
|
|
|||
|
|
@ -727,11 +727,28 @@ describeEmbeddedPostgres("company import transfer routes", () => {
|
|||
(await request(app).post(`/api/companies/import/transfers/${transferId}/apply`).send(importMeta)).status,
|
||||
).toBe(200);
|
||||
|
||||
// The rejection names the company the completed apply created so the
|
||||
// caller can find the earlier import instead of reading it as data loss.
|
||||
mockCompanyService.getById.mockResolvedValue({
|
||||
id: companyId,
|
||||
name: "Paperclip",
|
||||
issuePrefix: "PAPA",
|
||||
});
|
||||
const redeclared = await request(app).post("/api/companies/import/transfers").send(body);
|
||||
expect(redeclared.status).toBe(200);
|
||||
expect(redeclared.body.transferId).toBe(transferId);
|
||||
expect(redeclared.body.alreadyCompleted).toBe(true);
|
||||
expect(redeclared.body.missingParts).toEqual([]);
|
||||
expect(mockCompanyService.getById).toHaveBeenCalledWith(companyId);
|
||||
expect(redeclared.body.company).toEqual({ id: companyId, name: "Paperclip", issuePrefix: "PAPA" });
|
||||
|
||||
// A company deleted since the apply (or an attach that never happened)
|
||||
// degrades to company: null, never a 500.
|
||||
mockCompanyService.getById.mockResolvedValue(null);
|
||||
const redeclaredAfterDelete = await request(app).post("/api/companies/import/transfers").send(body);
|
||||
expect(redeclaredAfterDelete.status).toBe(200);
|
||||
expect(redeclaredAfterDelete.body.alreadyCompleted).toBe(true);
|
||||
expect(redeclaredAfterDelete.body.company).toBeNull();
|
||||
|
||||
const reApplied = await request(app)
|
||||
.post(`/api/companies/import/transfers/${transferId}/apply`)
|
||||
|
|
|
|||
|
|
@ -62,7 +62,7 @@ import {
|
|||
} from "../services/index.js";
|
||||
import { isCloudManagedInstance } from "../services/cloud-instance.js";
|
||||
import type { StorageService } from "../storage/types.js";
|
||||
import { assertBoard, assertCompanyAccess, assertInstanceAdmin, getActorInfo } from "./authz.js";
|
||||
import { assertBoard, assertCompanyAccess, assertInstanceAdmin, getActorInfo, hasCompanyAccess } from "./authz.js";
|
||||
import { COMPANY_IMPORT_ROUTE_PATH } from "./company-import-paths.js";
|
||||
|
||||
// A company import can arrive one of two ways on the import + preview routes:
|
||||
|
|
@ -729,12 +729,25 @@ export function companyRoutes(db: Db, storage?: StorageService, options?: Compan
|
|||
containerRef: { kind: "chunked_zip_upload" },
|
||||
});
|
||||
if (alreadyCompleted) {
|
||||
// Tell the caller WHERE the prior apply landed. companyId on the run is
|
||||
// written best-effort after apply and the company may have been deleted
|
||||
// since, so this lookup is null-safe and the field stays optional. The
|
||||
// actor key proves this caller ran the original import, but their
|
||||
// access may have been revoked since — withhold the identity unless
|
||||
// they can still reach the company today (hasCompanyAccess, so a
|
||||
// revoked caller sees the same null as a deleted company).
|
||||
const landedCompany = run.companyId && hasCompanyAccess(req, run.companyId)
|
||||
? await svc.getById(run.companyId)
|
||||
: null;
|
||||
res.json({
|
||||
transferId: run.id,
|
||||
status: "completed",
|
||||
alreadyCompleted: true,
|
||||
totalParts: declared.parts.length,
|
||||
missingParts: [],
|
||||
company: landedCompany
|
||||
? { id: landedCompany.id, name: landedCompany.name ?? null, issuePrefix: landedCompany.issuePrefix ?? null }
|
||||
: null,
|
||||
} satisfies CompanyImportTransferCreated);
|
||||
return;
|
||||
}
|
||||
|
|
|
|||
|
|
@ -52,7 +52,7 @@ import {
|
|||
} from "../components/FileTree";
|
||||
import { readZipArchive } from "../lib/zip";
|
||||
import { formatMegabytes } from "../lib/import-preflight";
|
||||
import type { CompanyImportTransferDeclaration } from "@paperclipai/shared/company-import-transfer";
|
||||
import { buildAlreadyImportedMessage, type CompanyImportTransferDeclaration } from "@paperclipai/shared/company-import-transfer";
|
||||
import {
|
||||
CHUNKED_IMPORT_THRESHOLD_BYTES,
|
||||
IMPORT_TRANSFER_PART_ATTEMPTS,
|
||||
|
|
@ -944,10 +944,10 @@ export function CompanyImport() {
|
|||
const created = await companiesApi.importTransferCreate(manifest);
|
||||
if (created.alreadyCompleted) {
|
||||
// The server keys transfers by content, and this exact zip already
|
||||
// finished an apply — its parts are gone, so it cannot be re-run.
|
||||
throw new Error(
|
||||
"This exact package was already imported by a completed transfer. Re-export the package to import it again.",
|
||||
);
|
||||
// finished an apply — its parts are gone, so it cannot be re-run. Name
|
||||
// the company that apply created so this reads as "your import exists
|
||||
// over there", not as data loss.
|
||||
throw new Error(buildAlreadyImportedMessage(created.company));
|
||||
}
|
||||
const missing = new Set(created.missingParts);
|
||||
let uploadedParts = manifest.parts.length - missing.size;
|
||||
|
|
|
|||
Loading…
Reference in New Issue