fix(core): Close review requests when a source control pull archives or cascade-deletes a workflow (#35798)

Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
2026-08-28 17:22:01 +08:00 · 2026-08-07 19:52:12 +02:00
parent 5bf5f385fd
commit fd366ceba1
11 changed files with 591 additions and 92 deletions
@@ -42,6 +42,7 @@ import type { DataTableSizeValidator } from '@/modules/data-table/data-table-siz
 import type { DataTableRepository } from '@/modules/data-table/data-table.repository';
 import type { RedactionEnforcementService } from '@/modules/redaction/redaction-enforcement.service';
 import type { WorkflowHistoryService } from '@/workflows/workflow-history/workflow-history.service';
+import type { WorkflowMutationHooksProxy } from '@/workflows/workflow-mutation-hooks-proxy.service';
 import type { WorkflowPublishGuardProxy } from '@/workflows/workflow-publish-guard-proxy.service';
 import type { WorkflowService } from '@/workflows/workflow.service';

@@ -76,6 +77,7 @@ describe('SourceControlImportService', () => {
 	const workflowService = mock<WorkflowService>();
 	const workflowHistoryService = mock<WorkflowHistoryService>();
 	const workflowPublishGuard = mock<WorkflowPublishGuardProxy>();
+	const workflowMutationHooks = mock<WorkflowMutationHooksProxy>();
 	const dataTableRepository = mock<DataTableRepository>();
 	const dataTableColumnRepository = mock<DataTableColumnRepository>();
 	const dataTableDDLService = mock<DataTableDDLService>();
@@ -127,6 +129,7 @@ describe('SourceControlImportService', () => {
 		activeWorkflowManager,
 		executionPersistence,
 		workflowPublishGuard,
+		workflowMutationHooks,
 	);

 	const globMock = fastGlob.default as unknown as Mock<(...args: string[]) => Promise<string[]>>;
@@ -792,6 +795,131 @@ describe('SourceControlImportService', () => {
 			expect(workflowService.activateWorkflow).not.toHaveBeenCalled();
 		});

+		describe('workflow mutation hooks on pull-archive', () => {
+			const mockUserId = 'user-id-123';
+			const mockUser = Object.assign(new User(), { id: mockUserId });
+			const mockWorkflowFile = '/mock/workflow1.json';
+
+			const remoteWorkflow = (overrides: Record<string, unknown> = {}) => ({
+				id: 'workflow1',
+				name: 'Workflow',
+				nodes: [],
+				connections: {},
+				versionId: 'v2',
+				parentFolderId: null,
+				active: false,
+				...overrides,
+			});
+
+			const setupPull = (options: {
+				existing?: Partial<WorkflowEntity>;
+				remote?: Record<string, unknown>;
+			}) => {
+				userRepository.findOne.mockResolvedValue(mockUser);
+				projectRepository.getPersonalProjectForUserOrFail.mockResolvedValue(
+					Object.assign(new Project(), { id: 'project1', type: 'personal' }),
+				);
+				workflowRepository.findByIds.mockResolvedValue(
+					options.existing
+						? [
+								Object.assign(new WorkflowEntity(), {
+									id: 'workflow1',
+									name: 'Workflow',
+									active: false,
+									isArchived: false,
+									...options.existing,
+								}),
+							]
+						: [],
+				);
+				folderRepository.find.mockResolvedValue([]);
+				sharedWorkflowRepository.findWithFields.mockResolvedValue([]);
+				workflowRepository.upsert.mockResolvedValue({
+					identifiers: [{ id: 'workflow1' }],
+					generatedMaps: [],
+					raw: [],
+				});
+				fsReadFile.mockResolvedValue(JSON.stringify(remoteWorkflow(options.remote)));
+
+				return [mock<SourceControlledFile>({ file: mockWorkflowFile, id: 'workflow1' })];
+			};
+
+			it('should fire afterWorkflowArchived when the pull archives an existing workflow', async () => {
+				const candidates = setupPull({ existing: {}, remote: { isArchived: true } });
+
+				await service.importWorkflowFromWorkFolder(candidates, mockUserId);
+
+				expect(workflowMutationHooks.afterWorkflowArchived).toHaveBeenCalledTimes(1);
+				expect(workflowMutationHooks.afterWorkflowArchived).toHaveBeenCalledWith('workflow1');
+				// The hook observes a committed mutation, so it must run after the upsert
+				expect(workflowRepository.upsert.mock.invocationCallOrder[0]).toBeLessThan(
+					workflowMutationHooks.afterWorkflowArchived.mock.invocationCallOrder[0],
+				);
+			});
+
+			it('should not fire for a new workflow imported already archived', async () => {
+				const candidates = setupPull({ remote: { isArchived: true } });
+
+				await service.importWorkflowFromWorkFolder(candidates, mockUserId);
+
+				expect(workflowMutationHooks.afterWorkflowArchived).not.toHaveBeenCalled();
+			});
+
+			it('should not fire when the workflow is already archived locally', async () => {
+				const candidates = setupPull({
+					existing: { isArchived: true },
+					remote: { isArchived: true },
+				});
+
+				await service.importWorkflowFromWorkFolder(candidates, mockUserId);
+
+				expect(workflowMutationHooks.afterWorkflowArchived).not.toHaveBeenCalled();
+			});
+
+			it('should not fire for a plain update that leaves the workflow unarchived', async () => {
+				const candidates = setupPull({ existing: {}, remote: {} });
+
+				await service.importWorkflowFromWorkFolder(candidates, mockUserId);
+
+				expect(workflowMutationHooks.afterWorkflowArchived).not.toHaveBeenCalled();
+			});
+
+			it('should not fire when the pull unarchives a workflow', async () => {
+				const candidates = setupPull({
+					existing: { isArchived: true },
+					remote: { isArchived: false },
+				});
+
+				await service.importWorkflowFromWorkFolder(candidates, mockUserId);
+
+				expect(workflowMutationHooks.afterWorkflowArchived).not.toHaveBeenCalled();
+			});
+
+			it('should not fire when the upsert fails', async () => {
+				const candidates = setupPull({ existing: {}, remote: { isArchived: true } });
+				workflowRepository.upsert.mockResolvedValue({
+					identifiers: [],
+					generatedMaps: [],
+					raw: [],
+				});
+
+				await expect(service.importWorkflowFromWorkFolder(candidates, mockUserId)).rejects.toThrow(
+					'Failed to upsert workflow',
+				);
+
+				expect(workflowMutationHooks.afterWorkflowArchived).not.toHaveBeenCalled();
+			});
+
+			it('should fire even when saving workflow history subsequently fails', async () => {
+				const candidates = setupPull({ existing: {}, remote: { isArchived: true } });
+				workflowHistoryService.findVersion.mockRejectedValueOnce(new Error('history unavailable'));
+
+				await service.importWorkflowFromWorkFolder(candidates, mockUserId);
+
+				expect(workflowMutationHooks.afterWorkflowArchived).toHaveBeenCalledWith('workflow1');
+			});
+		});
+
 		describe('autoPublish parameter', () => {
 			const mockUserId = 'user-id-123';
 			const mockUser = Object.assign(new User(), { id: mockUserId });
@@ -2815,6 +2943,69 @@ describe('SourceControlImportService', () => {
 				expect(executionPersistence.hardDeleteByWorkflowId).toHaveBeenCalledWith('wf-1');
 				expect(folderRepository.delete).toHaveBeenCalledWith({ id: In(['folder1']) });
 			});
+
+			it('should fire beforeWorkflowDeleted before trigger teardown and folder deletion', async () => {
+				const candidates = [mock<SourceControlledFile>({ id: 'folder1' })];
+				const straggler = Object.assign(new WorkflowEntity(), { id: 'wf-1', active: true });
+				folderRepository.getAllFolderIdsInHierarchy.mockResolvedValueOnce([]);
+				workflowRepository.find.mockResolvedValueOnce([straggler]);
+				workflowRepository.findOne.mockResolvedValueOnce(straggler);
+
+				await service.deleteFoldersNotInWorkfolder(candidates as any);
+
+				expect(workflowMutationHooks.beforeWorkflowDeleted).toHaveBeenCalledTimes(1);
+				expect(workflowMutationHooks.beforeWorkflowDeleted).toHaveBeenCalledWith('wf-1');
+				// An abort must leave the workflow untouched: hook before trigger teardown
+				expect(
+					workflowMutationHooks.beforeWorkflowDeleted.mock.invocationCallOrder[0],
+				).toBeLessThan(activeWorkflowManager.remove.mock.invocationCallOrder[0]);
+				expect(
+					workflowMutationHooks.beforeWorkflowDeleted.mock.invocationCallOrder[0],
+				).toBeLessThan(folderRepository.delete.mock.invocationCallOrder[0]);
+			});
+
+			it('should abort folder deletion when beforeWorkflowDeleted rejects', async () => {
+				const candidates = [mock<SourceControlledFile>({ id: 'folder1', name: 'My folder' })];
+				const straggler = Object.assign(new WorkflowEntity(), { id: 'wf-1', active: true });
+				folderRepository.getAllFolderIdsInHierarchy.mockResolvedValueOnce([]);
+				workflowRepository.find.mockResolvedValueOnce([straggler]);
+				workflowRepository.findOne.mockResolvedValueOnce(straggler);
+				workflowMutationHooks.beforeWorkflowDeleted.mockRejectedValueOnce(new Error('hook failed'));
+
+				await expect(service.deleteFoldersNotInWorkfolder(candidates as any)).rejects.toThrow(
+					'Failed to delete folder(s) "My folder" (folder1) while pulling from source control: hook failed',
+				);
+
+				expect(activeWorkflowManager.remove).not.toHaveBeenCalled();
+				expect(folderRepository.delete).not.toHaveBeenCalled();
+			});
+
+			it('should fire the afterWorkflowDeleted sweep once, after the folder row delete', async () => {
+				const candidates = [mock<SourceControlledFile>({ id: 'folder1' })];
+				const straggler = Object.assign(new WorkflowEntity(), { id: 'wf-1', active: false });
+				folderRepository.getAllFolderIdsInHierarchy.mockResolvedValueOnce([]);
+				workflowRepository.find.mockResolvedValueOnce([straggler]);
+				workflowRepository.findOne.mockResolvedValueOnce(straggler);
+
+				await service.deleteFoldersNotInWorkfolder(candidates as any);
+
+				expect(workflowMutationHooks.afterWorkflowDeleted).toHaveBeenCalledTimes(1);
+				expect(workflowMutationHooks.afterWorkflowDeleted).toHaveBeenCalledWith('wf-1');
+				// Only the row delete cascades the workflows away, so the sweep must run after it
+				expect(
+					workflowMutationHooks.afterWorkflowDeleted.mock.invocationCallOrder[0],
+				).toBeGreaterThan(folderRepository.delete.mock.invocationCallOrder[0]);
+			});
+
+			it('should not fire the sweep when the deleted folders contained no workflows', async () => {
+				const candidates = [mock<SourceControlledFile>({ id: 'folder1' })];
+				folderRepository.getAllFolderIdsInHierarchy.mockResolvedValueOnce([]);
+				workflowRepository.find.mockResolvedValueOnce([]);
+
+				await service.deleteFoldersNotInWorkfolder(candidates as any);
+
+				expect(workflowMutationHooks.afterWorkflowDeleted).not.toHaveBeenCalled();
+			});
 		});
 	});

@@ -3376,6 +3567,111 @@ describe('SourceControlImportService', () => {
 				expect(executionPersistence.hardDeleteByWorkflowId).toHaveBeenCalledWith('wf-inactive');
 				expect(projectRepository.delete).toHaveBeenCalledWith({ id: In(['project-1']) });
 			});
+
+			it('should fire beforeWorkflowDeleted for each straggler before project deletion', async () => {
+				const candidates = [mock<SourceControlledFile>({ id: 'project-1' })];
+				sharedWorkflowRepository.find.mockResolvedValueOnce([
+					{ workflowId: 'wf-active' },
+					{ workflowId: 'wf-inactive' },
+				] as SharedWorkflow[]);
+				workflowRepository.findOne
+					.mockResolvedValueOnce(
+						Object.assign(new WorkflowEntity(), { id: 'wf-active', active: true }),
+					)
+					.mockResolvedValueOnce(
+						Object.assign(new WorkflowEntity(), { id: 'wf-inactive', active: false }),
+					);
+
+				await service.deleteTeamProjectsNotInWorkfolder(candidates);
+
+				expect(workflowMutationHooks.beforeWorkflowDeleted).toHaveBeenCalledTimes(2);
+				expect(workflowMutationHooks.beforeWorkflowDeleted).toHaveBeenCalledWith('wf-active');
+				expect(workflowMutationHooks.beforeWorkflowDeleted).toHaveBeenCalledWith('wf-inactive');
+				// An abort must leave the workflow untouched: hook before trigger teardown
+				expect(
+					workflowMutationHooks.beforeWorkflowDeleted.mock.invocationCallOrder[0],
+				).toBeLessThan(activeWorkflowManager.remove.mock.invocationCallOrder[0]);
+				expect(
+					workflowMutationHooks.beforeWorkflowDeleted.mock.invocationCallOrder[0],
+				).toBeLessThan(projectRepository.delete.mock.invocationCallOrder[0]);
+			});
+
+			it('should abort project deletion when beforeWorkflowDeleted rejects', async () => {
+				const candidates = [mock<SourceControlledFile>({ id: 'project-1', name: 'My project' })];
+				sharedWorkflowRepository.find.mockResolvedValueOnce([
+					{ workflowId: 'wf-1' },
+				] as SharedWorkflow[]);
+				workflowRepository.findOne.mockResolvedValueOnce(
+					Object.assign(new WorkflowEntity(), { id: 'wf-1', active: false }),
+				);
+				workflowMutationHooks.beforeWorkflowDeleted.mockRejectedValueOnce(new Error('hook failed'));
+
+				await expect(service.deleteTeamProjectsNotInWorkfolder(candidates)).rejects.toThrow(
+					'Failed to delete project(s) "My project" (project-1) while pulling from source control: hook failed',
+				);
+
+				expect(executionPersistence.hardDeleteByWorkflowId).not.toHaveBeenCalled();
+				expect(projectRepository.delete).not.toHaveBeenCalled();
+			});
+
+			it('should run all hooks before any teardown, so a late rejection leaves earlier workflows untouched', async () => {
+				const candidates = [mock<SourceControlledFile>({ id: 'project-1', name: 'My project' })];
+				sharedWorkflowRepository.find.mockResolvedValueOnce([
+					{ workflowId: 'wf-1' },
+					{ workflowId: 'wf-2' },
+				] as SharedWorkflow[]);
+				workflowRepository.findOne
+					.mockResolvedValueOnce(Object.assign(new WorkflowEntity(), { id: 'wf-1', active: true }))
+					.mockResolvedValueOnce(
+						Object.assign(new WorkflowEntity(), { id: 'wf-2', active: false }),
+					);
+				workflowMutationHooks.beforeWorkflowDeleted
+					.mockResolvedValueOnce(undefined)
+					.mockRejectedValueOnce(new Error('hook failed'));
+
+				await expect(service.deleteTeamProjectsNotInWorkfolder(candidates)).rejects.toThrow(
+					'Failed to delete project(s) "My project" (project-1) while pulling from source control: hook failed',
+				);
+
+				// wf-1's hook already passed, but wf-2's rejection must abort before
+				// ANY teardown — wf-1 keeps its triggers and execution history
+				expect(activeWorkflowManager.remove).not.toHaveBeenCalled();
+				expect(executionPersistence.hardDeleteByWorkflowId).not.toHaveBeenCalled();
+				expect(projectRepository.delete).not.toHaveBeenCalled();
+			});
+
+			it('should fire the afterWorkflowDeleted sweep once, after the project row delete', async () => {
+				const candidates = [mock<SourceControlledFile>({ id: 'project-1' })];
+				sharedWorkflowRepository.find.mockResolvedValueOnce([
+					{ workflowId: 'wf-active' },
+					{ workflowId: 'wf-inactive' },
+				] as SharedWorkflow[]);
+				workflowRepository.findOne
+					.mockResolvedValueOnce(
+						Object.assign(new WorkflowEntity(), { id: 'wf-active', active: true }),
+					)
+					.mockResolvedValueOnce(
+						Object.assign(new WorkflowEntity(), { id: 'wf-inactive', active: false }),
+					);
+
+				await service.deleteTeamProjectsNotInWorkfolder(candidates);
+
+				// The sweep searches globally for orphaned requests, so one call covers the batch
+				expect(workflowMutationHooks.afterWorkflowDeleted).toHaveBeenCalledTimes(1);
+				expect(workflowMutationHooks.afterWorkflowDeleted).toHaveBeenCalledWith('wf-active');
+				expect(
+					workflowMutationHooks.afterWorkflowDeleted.mock.invocationCallOrder[0],
+				).toBeGreaterThan(projectRepository.delete.mock.invocationCallOrder[0]);
+			});
+
+			it('should not fire the sweep when the deleted projects contained no workflows', async () => {
+				const candidates = [mock<SourceControlledFile>({ id: 'project-1' })];
+				sharedWorkflowRepository.find.mockResolvedValueOnce([]);
+
+				await service.deleteTeamProjectsNotInWorkfolder(candidates);
+
+				expect(workflowMutationHooks.afterWorkflowDeleted).not.toHaveBeenCalled();
+			});
 		});
 	});

@@ -55,6 +55,7 @@ import { TagService } from '@/services/tag.service';
 import { assertNever } from '@/utils';
 import { validateWorkflowNodeGroups, sanitizeNodeGroupDescriptions } from '@/workflow-helpers';
 import { WorkflowHistoryService } from '@/workflows/workflow-history/workflow-history.service';
+import { WorkflowMutationHooksProxy } from '@/workflows/workflow-mutation-hooks-proxy.service';
 import { WorkflowPublishGuardProxy } from '@/workflows/workflow-publish-guard-proxy.service';
 import { WorkflowService } from '@/workflows/workflow.service';

@@ -153,6 +154,7 @@ export class SourceControlImportService {
 		private readonly activeWorkflowManager: ActiveWorkflowManager,
 		private readonly executionPersistence: ExecutionPersistence,
 		private readonly workflowPublishGuard: WorkflowPublishGuardProxy,
+		private readonly workflowMutationHooks: WorkflowMutationHooksProxy,
 	) {
 		this.gitFolder = path.join(instanceSettings.n8nFolder, SOURCE_CONTROL_GIT_FOLDER);
 		this.workflowExportFolder = path.join(this.gitFolder, SOURCE_CONTROL_WORKFLOW_EXPORT_FOLDER);
@@ -848,6 +850,12 @@ export class SourceControlImportService {

 		this.logger.debug(`Updating workflow id ${id ?? 'new'}`);

+		// The upsert below writes `isArchived` directly instead of going through
+		// `WorkflowService.archive()`, so detect the transition to run its
+		// side effects (e.g. closing open review requests) ourselves.
+		const archivedByPull =
+			!!existingWorkflow && !existingWorkflow.isArchived && !!importedWorkflow.isArchived;
+
 		const upsertResult = await this.workflowRepository.upsert(
 			{
 				...importedWorkflow,
@@ -861,6 +869,10 @@ export class SourceControlImportService {
 			});
 		}

+		if (archivedByPull) {
+			await this.workflowMutationHooks.afterWorkflowArchived(id);
+		}
+
 		try {
 			await this.saveOrUpdateWorkflowHistory(
 				{ id, versionId, nodes, connections, nodeGroups },
@@ -1789,13 +1801,15 @@ export class SourceControlImportService {
 				select: ['id'],
 				where: { parentFolder: { id: In(folderIds) } },
 			});
-			await this.deactivateWorkflowsAndHardDeleteExecutions(
+			const cascadedWorkflowIds = await this.deactivateWorkflowsAndHardDeleteExecutions(
 				workflows.map((workflow) => workflow.id),
 			);

 			await this.folderRepository.delete({
 				id: In(candidateIds),
 			});
+
+			await this.sweepAfterWorkflowCascade(cascadedWorkflowIds);
 		} catch (error) {
 			throw this.deletionError('folder', candidates, error);
 		}
@@ -1816,13 +1830,15 @@ export class SourceControlImportService {
 				select: ['workflowId'],
 				where: { projectId: In(candidateIds), role: 'workflow:owner' },
 			});
-			await this.deactivateWorkflowsAndHardDeleteExecutions(
+			const cascadedWorkflowIds = await this.deactivateWorkflowsAndHardDeleteExecutions(
 				ownedWorkflows.map((sw) => sw.workflowId),
 			);

 			await this.projectRepository.delete({
 				id: In(candidateIds),
 			});
+
+			await this.sweepAfterWorkflowCascade(cascadedWorkflowIds);
 		} catch (error) {
 			throw this.deletionError('project', candidates, error);
 		}
@@ -1839,29 +1855,58 @@ export class SourceControlImportService {
 	 * pulling user holds `workflow:delete` on, and the pull already ran it for
 	 * those (see `deleteWorkflowsNotInWorkfolder`). Any workflow still standing
 	 * was skipped by that permission check, yet the FK cascade below deletes it
-	 * regardless — so we do the physical cleanup directly beforehand. Deletion
-	 * hooks (`workflow.delete`/`workflow.afterDelete`) and the `workflow-deleted`
-	 * event don't fire for these workflows — a pre-existing gap for any
-	 * cascade-deleted workflow.
+	 * regardless — so we do the physical cleanup directly beforehand. The
+	 * `beforeWorkflowDeleted` mutation hook fires here so modules can run their
+	 * pre-delete side effects (e.g. closing open review requests), and the
+	 * caller fires `afterWorkflowDeleted` once the cascade has run (see
+	 * {@link sweepAfterWorkflowCascade}) — but external hooks
+	 * (`workflow.delete`/`workflow.afterDelete`) and the `workflow-deleted`
+	 * event still don't fire, a pre-existing gap for any cascade-deleted
+	 * workflow.
 	 *
 	 * REVIEW(question): should we instead extract the permission-free part of
 	 * `WorkflowService.delete` (deactivate + drain + delete row + hooks/events)
-	 * into an internal method and call it here? That would restore hook/event
-	 * parity, at the cost of refactoring a hot service for this edge path.
+	 * into an internal method and call it here? That would restore full
+	 * hook/event parity, at the cost of refactoring a hot service for this
+	 * edge path.
 	 */
 	private async deactivateWorkflowsAndHardDeleteExecutions(workflowIds: string[]) {
+		const workflows: WorkflowEntity[] = [];
 		for (const workflowId of workflowIds) {
 			const workflow = await this.workflowRepository.findOne({
 				select: ['id', 'active'],
 				where: { id: workflowId },
 			});
-			if (!workflow) continue;
+			if (workflow) workflows.push(workflow);
+		}

+		// The hook may throw to abort the deletion, so it runs for every workflow
+		// before any destructive teardown — a rejection halfway through the batch
+		// must not leave earlier workflows deactivated with their executions gone.
+		for (const workflow of workflows) {
+			await this.workflowMutationHooks.beforeWorkflowDeleted(workflow.id);
+		}
+
+		for (const workflow of workflows) {
 			if (workflow.active) {
 				await this.activeWorkflowManager.remove(workflow.id);
 			}
 			await this.executionPersistence.hardDeleteByWorkflowId(workflow.id);
 		}
+
+		return workflows.map((workflow) => workflow.id);
+	}
+
+	/**
+	 * Fire `afterWorkflowDeleted` once the folder/project row delete has
+	 * cascaded the given workflows away, mirroring `WorkflowService.delete`:
+	 * the sweep behind the hook closes review requests opened after the
+	 * pre-delete hooks ran and now left without a workflow. It searches
+	 * globally for such orphans, so one call covers the whole batch.
+	 */
+	private async sweepAfterWorkflowCascade(cascadedWorkflowIds: string[]) {
+		if (cascadedWorkflowIds.length === 0) return;
+		await this.workflowMutationHooks.afterWorkflowDeleted(cascadedWorkflowIds[0]);
 	}

 	/** Contextual error for a failed deletion during pull, so the operator learns which resource to look at. */
@@ -1,5 +1,6 @@
 process.env.N8N_ENV_FEAT_WORKFLOW_REVIEWS = 'true';

+import type { SourceControlledFile } from '@n8n/api-types';
 import {
 	createTeamProject,
 	createWorkflow,
@@ -9,18 +10,29 @@ import {
 } from '@n8n/backend-test-utils';
 import type { Project, User } from '@n8n/db';
 import {
+	FolderRepository,
+	ProjectRepository,
+	SharedWorkflowRepository,
+	UserRepository,
 	WorkflowRepository,
 	WorkflowReviewRequestAuthorRepository,
 	WorkflowReviewRequestRepository,
 	WorkflowReviewRequestWorkflowRepository,
 } from '@n8n/db';
 import { Container } from '@n8n/di';
+import type { InstanceSettings } from 'n8n-core';
+import { readFile } from 'node:fs/promises';
 import { v4 as uuid } from 'uuid';
+import { mock } from 'vitest-mock-extended';

 import { ActiveWorkflowManager } from '@/active-workflow-manager';
+import { SourceControlImportService } from '@/modules/source-control.ee/source-control-import.service.ee';
 import { WorkflowReviewPolicyService } from '@/services/workflow-review-policy.service';
+import { WorkflowHistoryService } from '@/workflows/workflow-history/workflow-history.service';
+import { WorkflowMutationHooksProxy } from '@/workflows/workflow-mutation-hooks-proxy.service';
 import { EnterpriseWorkflowService } from '@/workflows/workflow.service.ee';
 import { WorkflowService } from '@/workflows/workflow.service';
+import { createFolder } from '@test-integration/db/folders';
 import { createOwner } from '@test-integration/db/users';
 import { createWorkflowHistoryItem } from '@test-integration/db/workflow-history';
 import type { SuperAgentTest } from '@test-integration/types';
@@ -28,6 +40,15 @@ import * as utils from '@test-integration/utils';

 mockInstance(ActiveWorkflowManager);

+// `readFile` must be mocked at the module level: the source-control import service
+// imports it as a named binding, which `vi.spyOn` can't intercept under Vitest.
+// The default implementation passes through to the real fs.
+vi.mock('node:fs/promises', async (importOriginal) => {
+	const actual = await importOriginal<typeof import('node:fs/promises')>();
+	const mockedReadFile = vi.fn(actual.readFile);
+	return { ...actual, readFile: mockedReadFile, default: { ...actual, readFile: mockedReadFile } };
+});
+
 const testServer = utils.setupTestServer({
 	endpointGroups: ['workflow-reviews', 'workflows'],
 	enabledFeatures: ['feat:workflowReviews'],
@@ -64,6 +85,7 @@ beforeEach(async () => {
 		'WorkflowPublishHistory',
 		'WorkflowEntity',
 		'WorkflowHistory',
+		'Folder',
 		'ProjectRelation',
 		'Project',
 		'User',
@@ -230,3 +252,152 @@ describe('auto-close with the instance policy disabled', () => {
 		expect(closed?.state).toBe('closed');
 	});
 });
+
+describe('auto-close on source-control pull', () => {
+	const mockFileData = new Map<string, string>();
+	const fsReadFile = vi.mocked(readFile);
+	let importService: SourceControlImportService;
+
+	beforeAll(() => {
+		// Manual construction with real persistence + the real hooks proxy (whose
+		// provider the workflow-reviews module registered), fs and non-workflow
+		// dependencies mocked — same pattern as the environments integration tests.
+		importService = new SourceControlImportService(
+			mock(), // logger
+			mock(), // errorReporter
+			mock(), // variablesService
+			mock(), // credentialsRepository
+			Container.get(ProjectRepository),
+			mock(), // projectRelationRepository
+			mock(), // tagRepository
+			Container.get(SharedWorkflowRepository),
+			mock(), // sharedCredentialsRepository
+			Container.get(UserRepository),
+			mock(), // variablesRepository
+			Container.get(WorkflowRepository),
+			mock(), // workflowTagMappingRepository
+			mock(), // workflowService
+			mock(), // credentialsService
+			mock(), // tagService
+			Container.get(FolderRepository),
+			mock<InstanceSettings>({ n8nFolder: '/mock' }),
+			mock(), // sourceControlContextFactory
+			mock(), // sourceControlScopedService
+			Container.get(WorkflowHistoryService),
+			mock(), // dataTableRepository
+			mock(), // dataTableColumnRepository
+			mock(), // dataTableDDLService
+			mock(), // redactionEnforcementService
+			mock(), // dataTableSizeValidator
+			mock(), // activeWorkflowManager
+			mock(), // executionPersistence
+			mock(), // workflowPublishGuard
+			Container.get(WorkflowMutationHooksProxy),
+		);
+	});
+
+	beforeEach(() => {
+		mockFileData.clear();
+		fsReadFile.mockImplementation(async (path) => {
+			const pathStr = typeof path === 'string' ? path : path.toString();
+			const data = mockFileData.get(pathStr);
+			if (data === undefined) throw new Error(`Trying to access invalid file in test: ${pathStr}`);
+			return data;
+		});
+	});
+
+	afterAll(() => {
+		// Restore the pass-through implementation given to vi.fn(actual.readFile)
+		fsReadFile.mockReset();
+	});
+
+	const putWorkflowFile = (remote: { id: string }) => {
+		const file = `/mock/${remote.id}.json`;
+		mockFileData.set(file, JSON.stringify(remote));
+		return mock<SourceControlledFile>({ id: remote.id, file });
+	};
+
+	const remoteWorkflow = (id: string, overrides: Record<string, unknown> = {}) => ({
+		id,
+		name: 'Remote Workflow',
+		versionId: uuid(),
+		nodes: [],
+		connections: {},
+		settings: {},
+		parentFolderId: null,
+		active: false,
+		isArchived: false,
+		owner: { type: 'personal', personalEmail: owner.email },
+		...overrides,
+	});
+
+	test('a pull that archives the workflow closes the open review, decision unchanged', async () => {
+		const { workflow, versionId } = await createReviewableWorkflow();
+		const request = await createOpenReview(workflow.id, versionId, {
+			decision: 'changes_requested',
+		});
+
+		const candidate = putWorkflowFile(remoteWorkflow(workflow.id, { isArchived: true }));
+		await importService.importWorkflowFromWorkFolder([candidate], owner.id);
+
+		const archived = await Container.get(WorkflowRepository).findOneBy({ id: workflow.id });
+		expect(archived?.isArchived).toBe(true);
+
+		const closed = await requestRepository.findById(request.id, {});
+		expect(closed?.state).toBe('closed');
+		expect(closed?.decision).toBe('changes_requested');
+		expect(closed?.closedById).toBeNull();
+		// The publish guard keys off open requests — none left to block publishing
+		expect(await requestRepository.findOpenRequestForWorkflow(workflow.id, {})).toBeNull();
+	});
+
+	test('an already-approved (closed) review is untouched by a pull-archive', async () => {
+		const { workflow, versionId } = await createReviewableWorkflow();
+		const request = await createOpenReview(workflow.id, versionId, {
+			state: 'closed',
+			decision: 'approved',
+		});
+
+		const candidate = putWorkflowFile(remoteWorkflow(workflow.id, { isArchived: true }));
+		await importService.importWorkflowFromWorkFolder([candidate], owner.id);
+
+		const untouched = await requestRepository.findById(request.id, {});
+		expect(untouched?.state).toBe('closed');
+		expect(untouched?.decision).toBe('approved');
+		expect(untouched?.updatedAt).toEqual(request.updatedAt);
+	});
+
+	test('a pull that updates the workflow without archiving leaves the review open', async () => {
+		const { workflow, versionId } = await createReviewableWorkflow();
+		const request = await createOpenReview(workflow.id, versionId);
+
+		const candidate = putWorkflowFile(remoteWorkflow(workflow.id, { name: 'Updated by pull' }));
+		await importService.importWorkflowFromWorkFolder([candidate], owner.id);
+
+		const updated = await Container.get(WorkflowRepository).findOneBy({ id: workflow.id });
+		expect(updated?.name).toBe('Updated by pull');
+
+		const stillOpen = await requestRepository.findById(request.id, {});
+		expect(stillOpen?.state).toBe('open');
+	});
+
+	test('a pull that deletes a folder closes reviews of cascade-deleted workflows', async () => {
+		const folder = await createFolder(ownerProject, { name: 'Deleted remotely' });
+		const { workflow, versionId } = await createReviewableWorkflow();
+		await Container.get(WorkflowRepository).save({ id: workflow.id, parentFolder: folder });
+		const request = await createOpenReview(workflow.id, versionId);
+
+		await importService.deleteFoldersNotInWorkfolder([
+			mock<SourceControlledFile>({ id: folder.id, name: folder.name }),
+		]);
+
+		// The workflow went with the folder cascade...
+		expect(await Container.get(WorkflowRepository).findOneBy({ id: workflow.id })).toBeNull();
+
+		// ...but its review was properly closed first, not left orphaned open
+		const closed = await requestRepository.findById(request.id, {});
+		expect(closed?.state).toBe('closed');
+		expect(closed?.closedById).toBeNull();
+		expect(await linkRepository.findByRequestId(request.id, {})).toHaveLength(0);
+	});
+});
@@ -235,12 +235,16 @@ describe('WorkflowReviewInboxService.getDetail', () => {
 			expect(detail.workflowVersionId).toBeNull();
 		});

-		// An open review with no covered workflow is a dead leftover (nothing can
-		// decide or update it), so it is hidden — from its requester too
-		it('reports an open review whose workflows were all deleted as not found', async () => {
+		// An open review can transiently cover no workflow when a delete orphaned
+		// it and the sweep hasn't closed it yet — it stays readable until then
+		it('returns an open review with no workflows when its workflow was deleted', async () => {
 			workflowRepository.findLinkedWorkflowDetailsByRequestId.mockResolvedValue([]);

-			await expect(service.getDetail(requester, requestId)).rejects.toThrow(NotFoundError);
+			const detail = await service.getDetail(requester, requestId);
+
+			expect(detail.state).toBe('open');
+			expect(detail.workflows).toEqual([]);
+			expect(detail.workflowName).toBeNull();
 		});
 	});

@@ -2185,7 +2185,7 @@ describe('GET /workflow-review-requests/summary', () => {
 		expect(response.body.data).toEqual({ open: 1, closed: 0 });
 	});

-	test('does not count an open review orphaned by a workflow hard delete', async () => {
+	test('still counts an open review orphaned by a workflow hard delete until a sweep closes it', async () => {
 		await seedInboxRequests();
 		const orphan = await requestRepository.createRequest(
 			{
@@ -2197,17 +2197,16 @@ describe('GET /workflow-review-requests/summary', () => {
 			{},
 		);
 		const workflow = await linkToNewWorkflow(orphan.id);
-		// Bypasses the auto-close hook: the cascade removes the link row and
-		// leaves the request open — as the folder cascade or a create/delete race would
+		// Bypasses the auto-close hook and the sweep: the cascade removes the link
+		// row and leaves the request open — visible until the next delete sweeps it
 		await workflowEntityRepository.delete({ id: workflow.id });

-		// Owner exercises the global scope, member the project-scoped filter.
-		// The closed seed request has no link rows and must stay counted.
+		// Owner exercises the global scope, member the project-scoped filter
 		const ownerResponse = await ownerAgent.get('/workflow-review-requests/summary').expect(200);
-		expect(ownerResponse.body.data).toEqual({ open: 1, closed: 1 });
+		expect(ownerResponse.body.data).toEqual({ open: 2, closed: 1 });

 		const memberResponse = await memberAgent.get('/workflow-review-requests/summary').expect(200);
-		expect(memberResponse.body.data).toEqual({ open: 1, closed: 1 });
+		expect(memberResponse.body.data).toEqual({ open: 2, closed: 1 });
 	});

 	test('returns 403 when feature is disabled', async () => {
@@ -2247,7 +2246,7 @@ describe('GET /workflow-review-requests/inbox', () => {
 		expect(response.body.data.hasMore).toBe(false);
 	});

-	test('omits an open review orphaned by a workflow hard delete', async () => {
+	test('still lists an open review orphaned by a workflow hard delete until a sweep closes it', async () => {
 		const { openRequest } = await seedInboxRequests();
 		const orphan = await requestRepository.createRequest(
 			{
@@ -2259,8 +2258,8 @@ describe('GET /workflow-review-requests/inbox', () => {
 			{},
 		);
 		const workflow = await linkToNewWorkflow(orphan.id);
-		// Bypasses the auto-close hook: the cascade removes the link row and
-		// leaves the request open — as the folder cascade or a create/delete race would
+		// Bypasses the auto-close hook and the sweep: the cascade removes the link
+		// row and leaves the request open — visible until the next delete sweeps it
 		await workflowEntityRepository.delete({ id: workflow.id });

 		// Owner exercises the global scope, member the project-scoped filter
@@ -2268,17 +2267,17 @@ describe('GET /workflow-review-requests/inbox', () => {
 			.get('/workflow-review-requests/inbox')
 			.query({ state: 'open', limit: 15 })
 			.expect(200);
-		expect(ownerResponse.body.data.data.map((row: { id: string }) => row.id)).toEqual([
-			openRequest.id,
-		]);
+		expect(ownerResponse.body.data.data.map((row: { id: string }) => row.id).sort()).toEqual(
+			[openRequest.id, orphan.id].sort(),
+		);

 		const memberResponse = await memberAgent
 			.get('/workflow-review-requests/inbox')
 			.query({ state: 'open', limit: 15 })
 			.expect(200);
-		expect(memberResponse.body.data.data.map((row: { id: string }) => row.id)).toEqual([
-			openRequest.id,
-		]);
+		expect(memberResponse.body.data.data.map((row: { id: string }) => row.id).sort()).toEqual(
+			[openRequest.id, orphan.id].sort(),
+		);
 	});

 	test('still lists a closed review whose workflow was hard-deleted', async () => {
@@ -2684,17 +2683,21 @@ describe('GET /workflow-review-requests/:workflowReviewRequestId', () => {
 		});
 	});

-	test('no longer opens an open review after its workflow was hard-deleted', async () => {
+	test('still opens an open review after its workflow was hard-deleted', async () => {
 		const workflow = await createWorkflow({}, teamProject);
 		await createWorkflowHistoryItem(workflow.id, { versionId: 'version-pinned' });
 		const request = await seedRequest(workflow.id, 'version-pinned', owner);

-		// Bypasses the auto-close hook: the cascade removes the link row and
-		// leaves the request open — a dead leftover nothing can decide or update
+		// Bypasses the auto-close hook and the sweep: the cascade removes the link
+		// row and leaves the request open until the next delete sweeps it closed
 		await workflowEntityRepository.delete({ id: workflow.id });

-		// 404 even for the requester (owner) — their inbox no longer lists it either
-		await ownerAgent.get(`/workflow-review-requests/${request.id}`).expect(404);
+		const response = await ownerAgent.get(`/workflow-review-requests/${request.id}`).expect(200);
+
+		expect(response.body.data.id).toBe(request.id);
+		expect(response.body.data.state).toBe('open');
+		expect(response.body.data.workflows).toEqual([]);
+		expect(response.body.data.workflowName).toBeNull();
 	});

 	test('still opens a closed review after its workflow was deleted', async () => {
@@ -61,7 +61,7 @@ export class WorkflowReviewAutoCloseService implements WorkflowMutationHooks {
 			});
 		} catch (error) {
 			// The delete has already committed; failing it now would be worse than a
-			// request that stays open, which the inbox hides until the next sweep.
+			// request that stays open until the next sweep closes it.
 			this.logger.error('Failed to close workflow review request(s) left without a workflow', {
 				workflowId,
 				error,
@@ -136,15 +136,6 @@ export class WorkflowReviewInboxService {
 			this.workflowReviewRequestReviewerRepository.findByRequestIds([request.id]),
 		]);

-		// An open request whose link rows all cascaded away with a workflow hard
-		// delete is a dead leftover: nothing can act on it and the inbox hides it,
-		// so hide it here too — for the requester as well, matching their inbox.
-		// A closed request keeps zero link rows legitimately (history of a deleted
-		// workflow) and stays readable.
-		if (request.state === 'open' && workflowRows.length === 0) {
-			throw new NotFoundError('Could not find review request');
-		}
-
 		const readableRows = await this.filterReadableWorkflowRows(user, workflowRows);
 		// Someone who reaches this review through its project has no reason to learn it
 		// exists once they can read none of the workflows it covers. The requester already
@@ -2017,6 +2017,42 @@ describe('WorkflowService', () => {
 			expect(updateCall?.[1]?.[2]).toEqual(expectedActor);
 			expect(afterUpdateCall?.[1]?.[2]).toEqual(expectedActor);
 		});
+
+		// Bulk import paths (e.g. the n8n-packages workflow importer) pass entities
+		// that may carry `isArchived` from the imported payload. Archiving must only
+		// happen through `archive()`, which runs its side effects (review auto-close,
+		// events) — so `update()` must never persist the flag. If this test breaks,
+		// those import paths silently gain an archive bypass.
+		test('does not persist isArchived from the update payload', async () => {
+			const workflow = mock<WorkflowEntity>({
+				id: WORKFLOW_ID,
+				isArchived: false,
+				versionId: 'v1',
+				nodes: [],
+				connections: {},
+				settings: {},
+				activeVersionId: undefined as unknown as string,
+				tags: [],
+			});
+			workflowFinderServiceMock.findWorkflowForUser.mockResolvedValue(workflow);
+			workflowRepositoryMock.findOne.mockResolvedValue(workflow);
+
+			const user = mock<User>({
+				id: 'user-1',
+				role: mock<Role>({ slug: 'global:admin' }),
+			});
+
+			await workflowService.update(
+				user,
+				{ nodes: [], connections: {}, isArchived: true } as unknown as WorkflowEntity,
+				WORKFLOW_ID,
+			);
+
+			expect(workflowRepositoryMock.update).toHaveBeenCalledWith(
+				WORKFLOW_ID,
+				expect.not.objectContaining({ isArchived: expect.anything() }),
+			);
+		});
 	});

 	describe('archive() and unarchive() hooks', () => {
@@ -127,6 +127,7 @@ describe('SourceControlImportService', () => {
 			mock(), // activeWorkflowManager
 			mock(), // executionPersistence
 			mock(), // workflowPublishGuard
+			mock(), // workflowMutationHooks
 		);
 	});