fix(core): Move the policy scope version by one on each accepted write (no-changelog) (#40802)

Co-authored-by: Claude Opus 5 <[email protected]>

packages/cli/src/modules/type-availability-policies/README.md

@@ -188,7 +188,8 @@ A policy lives in three tables:
 - **attachments**, which bind a document to a scope with a priority and a floor flag.
 
 `projectId: null` is the instance scope. An unwritten scope has no row: it allows everything and
-reports version `0`, so a first write sends `expectedVersion: 0`.
+reports version `0`, so a first write sends `expectedVersion: 0`. One accepted write moves the
+version by exactly one, whatever it changed.
 
 Evaluation is `instance ∩ project`, and a project can only restrict further. `delegate` is the
 one exception: an instance `delegate` is satisfied only by an explicit project `allow` rule,

packages/cli/src/modules/type-availability-policies/__tests__/type-availability-policy.service.test.ts

@@ -901,6 +901,7 @@ describe('TypeAvailabilityPolicyService', () => {
 			expect(policyRepository.findByIdAndKind.mock.invocationCallOrder[0]).toBeLessThan(
 				attachmentRepository.listScopeIdsAttachedToPolicy.mock.invocationCallOrder[0],
 			);
+			expect(scopeRepository.bumpVersion).toHaveBeenCalledWith(scope.id, ROOT);
 		});
 
 		it('rejects a delegate defaultAction at project scope before opening a transaction', async () => {
@@ -945,7 +946,7 @@ describe('TypeAvailabilityPolicyService', () => {
 			});
 			policyRepository.createPolicy.mockResolvedValue(makePolicy({ rules: [], version: 1 }));
 			scopeRepository.findScopeById.mockResolvedValue(
-				makeScope({ defaultAction: 'delegate', version: 2 }),
+				makeScope({ defaultAction: 'delegate', version: 1 }),
 			);
 
 			await expect(
@@ -1014,7 +1015,7 @@ describe('TypeAvailabilityPolicyService', () => {
 			const createdPolicy = makePolicy({ rules: [RULE], version: 1 });
 			policyRepository.createPolicy.mockResolvedValue(createdPolicy);
 			scopeRepository.findScopeById.mockResolvedValue(
-				makeScope({ defaultAction: 'deny', version: 2 }),
+				makeScope({ defaultAction: 'deny', version: 1 }),
 			);
 
 			const result = await service.setEffectivePolicy(
@@ -1025,8 +1026,9 @@ describe('TypeAvailabilityPolicyService', () => {
 				'user-1',
 			);
 
-			expect(result.version).toBe(2);
+			expect(result.version).toBe(1);
 			expect(result.rules).toEqual([RULE]);
+			expect(scopeRepository.bumpVersion).not.toHaveBeenCalled();
 			expect(attachmentRepository.replaceAttachmentsForScope).toHaveBeenCalledWith(
 				createdScope.id,
 				[{ policyId: createdPolicy.id, priority: 0, isFloor: false }],
@@ -1047,7 +1049,7 @@ describe('TypeAvailabilityPolicyService', () => {
 				projectId: null,
 				scopeId: createdScope.id,
 				before: null,
-				after: { defaultAction: 'deny', version: 2 },
+				after: { defaultAction: 'deny', version: 1 },
 				rulesBefore: null,
 				rulesAfter: [RULE],
 				warningCount: 0,
@@ -1065,7 +1067,7 @@ describe('TypeAvailabilityPolicyService', () => {
 			const updatedPolicy = makePolicy({ rules: [RULE], version: 2 });
 			policyRepository.updateRules.mockResolvedValue(updatedPolicy);
 			scopeRepository.findScopeById.mockResolvedValue(
-				makeScope({ defaultAction: 'deny', version: 3 }),
+				makeScope({ defaultAction: 'deny', version: 2 }),
 			);
 
 			await service.setEffectivePolicy(
@@ -1082,6 +1084,7 @@ describe('TypeAvailabilityPolicyService', () => {
 				'user-2',
 				ROOT,
 			);
+			expect(scopeRepository.bumpVersion).not.toHaveBeenCalled();
 			expect(eventService.emit).toHaveBeenCalledTimes(3);
 			expect(eventService.emit).toHaveBeenCalledWith(
 				'node-type-policy-document-updated',
@@ -1097,7 +1100,7 @@ describe('TypeAvailabilityPolicyService', () => {
 				projectId: null,
 				scopeId: scope.id,
 				before: { defaultAction: 'allow', version: 1 },
-				after: { defaultAction: 'deny', version: 3 },
+				after: { defaultAction: 'deny', version: 2 },
 				rulesBefore: [],
 				rulesAfter: [RULE],
 				warningCount: 0,
@@ -1171,6 +1174,31 @@ describe('TypeAvailabilityPolicyService', () => {
 			expect(result.defaultAction).toBe('deny');
 			expect(result.version).toBe(1);
 		});
+
+		it('moves the version once when neither the rules nor the default action change', async () => {
+			const scope = makeScope({ defaultAction: 'allow', version: 1 });
+			scopeRepository.findScopeByKindAndProject.mockResolvedValue(scope);
+			const existingPolicy = makePolicy({ rules: [RULE], version: 1 });
+			attachmentRepository.listAttachmentsForScope.mockResolvedValue([
+				{ policyId: existingPolicy.id, rules: [RULE], priority: 0, isFloor: false },
+			]);
+			policyRepository.findByIdAndKind.mockResolvedValue(existingPolicy);
+			policyRepository.updateRules.mockResolvedValue(existingPolicy);
+			scopeRepository.findScopeById.mockResolvedValue(
+				makeScope({ defaultAction: 'allow', version: 2 }),
+			);
+
+			await service.setEffectivePolicy(
+				KIND,
+				null,
+				{ rules: [RULE], defaultAction: 'allow' },
+				1,
+				'user-2',
+			);
+
+			expect(scopeRepository.updateDefaultAction).not.toHaveBeenCalled();
+			expect(scopeRepository.bumpVersion).toHaveBeenCalledTimes(1);
+		});
 	});
 
 	describe('evaluateComposedType', () => {

packages/cli/src/modules/type-availability-policies/database/repositories/type-availability-policy-scope.repository.ts

@@ -187,6 +187,8 @@ export class TypeAvailabilityPolicyScopeRepository extends BaseRepository<TypeAv
 	 *
 	 * Joins the caller's transaction when `ctx` carries one, so the read-back lock is held
 	 * for the rest of the caller's work; opens one otherwise, since the lock needs it.
+	 *
+	 * The new row starts at version 1, which is the first write's own bump.
 	 */
 	async createScopeIfAbsent(
 		input: NewPolicyScope,
@@ -242,6 +244,9 @@ export class TypeAvailabilityPolicyScopeRepository extends BaseRepository<TypeAv
 	 * Call this under the same `ctx` as whatever changed the scope's *effective* policy —
 	 * an attachment added, removed or reordered, or an attached policy's content edited —
 	 * so the version and the change commit together.
+	 *
+	 * One write moves the version once. Do not call this in a write that also ran
+	 * `createScopeIfAbsent` or `updateDefaultAction`: each of those sets the version itself.
 	 */
 	async bumpVersion(id: string, ctx: OperationContext): Promise<void> {
 		await this.managerFor(ctx).increment(TypeAvailabilityPolicyScope, { id }, 'version', 1);

packages/cli/src/modules/type-availability-policies/type-availability-policy.service.ts

@@ -829,6 +829,9 @@ export class TypeAvailabilityPolicyService {
 				}
 			}
 
+			const scopeCreated = scope === null;
+			const defaultActionChanged = scope !== null && scope.defaultAction !== input.defaultAction;
+
 			const scopeId = scope
 				? scope.id
 				: (
@@ -838,7 +841,7 @@ export class TypeAvailabilityPolicyService {
 						)
 					).id;
 
-			if (scope && scope.defaultAction !== input.defaultAction) {
+			if (defaultActionChanged) {
 				await this.scopeRepository.updateDefaultAction(
 					scopeId,
 					input.defaultAction,
@@ -885,11 +888,12 @@ export class TypeAvailabilityPolicyService {
 				);
 			}
 
-			// This composed write always bumps the scope's freshness signal once, on top of
-			// whatever `updateDefaultAction` bumped on its own — a document-only edit (no
-			// `defaultAction` change) still must move the version, since it changes what the
-			// scope effectively enforces.
-			await this.scopeRepository.bumpVersion(scopeId, ctx);
+			// One accepted write moves the version by exactly one. The insert and
+			// `updateDefaultAction` each move it already, so bump here only when neither ran —
+			// a document-only edit still changes what the scope enforces and must move it too.
+			if (!scopeCreated && !defaultActionChanged) {
+				await this.scopeRepository.bumpVersion(scopeId, ctx);
+			}
 
 			const scopeAfterRow = await this.scopeRepository.findScopeById(scopeId, ctx);
 			const scopeAfter = {
@@ -908,7 +912,7 @@ export class TypeAvailabilityPolicyService {
 			};
 		});
 
-		// This path always bumps the scope's version, so it always invalidates.
+		// This path always moves the scope's version, so it always invalidates.
 		await this.invalidateScopes([{ kind, projectId }]);
 
 		this.eventService.emit('node-type-policy-scope-updated', {

packages/cli/test/integration/policy/credential-type-policy-instance.controller.test.ts

@@ -111,6 +111,44 @@ describe('credential type availability policy instance controller admin happy pa
 		expect(persisted.body.data.rules).toEqual(response.body.data.rules);
 	});
 
+	test('each accepted PUT /instance moves the version by exactly one', async () => {
+		const agent = testServer.authAgentFor(owner);
+		const rule = (id: string, value: string) => ({
+			id,
+			action: 'deny' as const,
+			selector: { kind: 'name' as const, value },
+		});
+
+		const unwritten = await agent.get('/credential-type-policies/instance');
+		expect(unwritten.body.data.version).toBe(0);
+
+		const created = await agent
+			.put('/credential-type-policies/instance')
+			.send({ rules: [rule('r1', 'slackApi')], defaultAction: 'allow', version: 0 });
+		expect(created.statusCode).toBe(200);
+		expect(created.body.data.version).toBe(1);
+
+		const rulesOnly = await agent.put('/credential-type-policies/instance').send({
+			rules: [rule('r2', 'githubApi')],
+			defaultAction: 'allow',
+			version: created.body.data.version,
+		});
+		expect(rulesOnly.statusCode).toBe(200);
+		expect(rulesOnly.body.data.version).toBe(2);
+
+		const actionOnly = await agent.put('/credential-type-policies/instance').send({
+			rules: [rule('r2', 'githubApi')],
+			defaultAction: 'deny',
+			version: rulesOnly.body.data.version,
+		});
+		expect(actionOnly.statusCode).toBe(200);
+		expect(actionOnly.body.data.version).toBe(3);
+
+		const persisted = await agent.get('/credential-type-policies/instance');
+		expect(persisted.body.data.version).toBe(3);
+		expect(persisted.body.data.defaultAction).toBe('deny');
+	});
+
 	test('policy document CRUD persists, distinctly from node type policy documents', async () => {
 		const created = await testServer
 			.authAgentFor(owner)

packages/cli/test/integration/policy/type-availability-policy-instance.controller.test.ts

@@ -194,6 +194,85 @@ describe('node type availability policy instance controller admin happy path', (
 		);
 	});
 
+	test('each accepted PUT /instance moves the version by exactly one', async () => {
+		const agent = testServer.authAgentFor(owner);
+		const rule = (id: string, value: string) => ({
+			id,
+			action: 'deny' as const,
+			selector: { kind: 'name' as const, value },
+		});
+
+		const unwritten = await agent.get('/node-type-policies/instance');
+		expect(unwritten.body.data.version).toBe(0);
+
+		// Each write sends the version the previous response reported. A client that tracks the
+		// version this way must never see a gap, or its next write looks stale to it.
+		const created = await agent
+			.put('/node-type-policies/instance')
+			.send({ rules: [rule('r1', 'a.b')], defaultAction: 'allow', version: 0 });
+		expect(created.statusCode).toBe(200);
+		expect(created.body.data.version).toBe(1);
+
+		const rulesOnly = await agent.put('/node-type-policies/instance').send({
+			rules: [rule('r2', 'c.d')],
+			defaultAction: 'allow',
+			version: created.body.data.version,
+		});
+		expect(rulesOnly.statusCode).toBe(200);
+		expect(rulesOnly.body.data.version).toBe(2);
+
+		const actionOnly = await agent.put('/node-type-policies/instance').send({
+			rules: [rule('r2', 'c.d')],
+			defaultAction: 'deny',
+			version: rulesOnly.body.data.version,
+		});
+		expect(actionOnly.statusCode).toBe(200);
+		expect(actionOnly.body.data.version).toBe(3);
+
+		const persisted = await agent.get('/node-type-policies/instance');
+		expect(persisted.body.data.version).toBe(3);
+		expect(persisted.body.data.defaultAction).toBe('deny');
+		expect(persisted.body.data.rules).toEqual([rule('r2', 'c.d')]);
+	});
+
+	test('a PUT /instance that changes the rules and the default action together moves the version by one', async () => {
+		const agent = testServer.authAgentFor(owner);
+
+		const first = await agent
+			.put('/node-type-policies/instance')
+			.send({ rules: [], defaultAction: 'allow', version: 0 });
+		expect(first.statusCode).toBe(200);
+
+		const both = await agent.put('/node-type-policies/instance').send({
+			rules: [{ id: 'r1', action: 'deny', selector: { kind: 'name', value: 'a.b' } }],
+			defaultAction: 'deny',
+			version: first.body.data.version,
+		});
+
+		expect(both.statusCode).toBe(200);
+		expect(both.body.data.version).toBe(first.body.data.version + 1);
+	});
+
+	test('a PUT /instance that changes nothing still moves the version by one', async () => {
+		const agent = testServer.authAgentFor(owner);
+		const body = {
+			rules: [
+				{ id: 'r1', action: 'deny' as const, selector: { kind: 'name' as const, value: 'a.b' } },
+			],
+			defaultAction: 'allow' as const,
+		};
+
+		const first = await agent.put('/node-type-policies/instance').send({ ...body, version: 0 });
+		expect(first.statusCode).toBe(200);
+
+		const repeat = await agent
+			.put('/node-type-policies/instance')
+			.send({ ...body, version: first.body.data.version });
+
+		expect(repeat.statusCode).toBe(200);
+		expect(repeat.body.data.version).toBe(first.body.data.version + 1);
+	});
+
 	test('PUT /instance with a stale version returns 409', async () => {
 		const first = await testServer
 			.authAgentFor(owner)