From 546d002607ff69f9008b1238578ca0be30317301 Mon Sep 17 00:00:00 2001 From: Duncan Williams Date: Mon, 28 Sep 2026 13:52:25 +0100 Subject: [PATCH 1/2] =?UTF-8?q?=F0=9F=90=9B=20fix(standards):=20match=20ru?= =?UTF-8?q?le=20removals=20and=20edits=20by=20content=20when=20the=20id=20?= =?UTF-8?q?is=20unknown?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A client that reads standards from Markdown has no rule ids to send. The CLI stamps `deleteRule` and `updateRule` proposals with a placeholder and carries the rule's text instead — `payload.item.content` for a removal, `payload.oldValue` for an edit. The applier resolved both by id alone, so those proposals matched no rule and applied to nothing, while the batch still reported the standard as updated. Both branches now fall back to that text when the id names no rule. The id still wins whenever it resolves, so nothing changes for a client that sends a real one. The two fall back differently, on purpose: - A removal takes every rule with that content. The client is saying the standard should no longer carry it, and copies hold no id to tell them apart. - An edit takes the rule with that content only when exactly one has it. With several there is nothing to choose between them, and rewriting all would collapse them into duplicates, so it leaves them alone. Fixing this here rather than in the CLI means every client is repaired at once, including versions already installed that will never be upgraded. Co-Authored-By: Claude Opus 5 --- CHANGELOG.MD | 1 + .../StandardChangeProposalApplier.spec.ts | 221 ++++++++++++++++++ .../applier/StandardChangeProposalApplier.ts | 32 ++- 3 files changed, 250 insertions(+), 4 deletions(-) diff --git a/CHANGELOG.MD b/CHANGELOG.MD index 077ae82d40..8a6a8ec481 100644 --- a/CHANGELOG.MD +++ b/CHANGELOG.MD @@ -15,6 +15,7 @@ ## Fixed - Opening a component from All components keeps the rows already picked, and closing it goes back to that list rather than to the package carrying the component +- Rule removals and edits submitted by a client that cannot know rule ids, such as the CLI reading standards from Markdown, are now matched by content instead of being dropped in silence ## Removed diff --git a/packages/types/src/playbookChangeManagement/applier/StandardChangeProposalApplier.spec.ts b/packages/types/src/playbookChangeManagement/applier/StandardChangeProposalApplier.spec.ts index 3ec8389efe..c0e7b8bda1 100644 --- a/packages/types/src/playbookChangeManagement/applier/StandardChangeProposalApplier.spec.ts +++ b/packages/types/src/playbookChangeManagement/applier/StandardChangeProposalApplier.spec.ts @@ -450,6 +450,125 @@ describe('StandardChangeProposalApplier', () => { ).toThrow(ChangeProposalConflictError); }); }); + + describe('when the target id names no rule', () => { + it('rewrites the rule carrying the replaced content', () => { + const rule = ruleFactory({ content: 'Old content' }); + const source = standardVersionFactory({ rules: [rule] }); + const proposal = changeProposalFactory({ + type: ChangeProposalType.updateRule, + payload: { + targetId: createRuleId('unresolved'), + oldValue: 'Old content', + newValue: 'New content', + }, + }); + + const result = applier.applyChangeProposals(source, [ + proposal as ChangeProposal, + ]); + + expect((result.version.rules ?? [])[0].content).toBe('New content'); + }); + + it('preserves the id of the rule it matched', () => { + const rule = ruleFactory({ content: 'Old content' }); + const source = standardVersionFactory({ rules: [rule] }); + const proposal = changeProposalFactory({ + type: ChangeProposalType.updateRule, + payload: { + targetId: createRuleId('unresolved'), + oldValue: 'Old content', + newValue: 'New content', + }, + }); + + const result = applier.applyChangeProposals(source, [ + proposal as ChangeProposal, + ]); + + expect((result.version.rules ?? [])[0].id).toBe(rule.id); + }); + + it('leaves a rule whose content does not match', () => { + const rule = ruleFactory({ content: 'Something else entirely' }); + const source = standardVersionFactory({ rules: [rule] }); + const proposal = changeProposalFactory({ + type: ChangeProposalType.updateRule, + payload: { + targetId: createRuleId('unresolved'), + oldValue: 'Old content', + newValue: 'New content', + }, + }); + + const result = applier.applyChangeProposals(source, [ + proposal as ChangeProposal, + ]); + + expect((result.version.rules ?? [])[0].content).toBe( + 'Something else entirely', + ); + }); + + describe('when several rules carry the replaced content', () => { + it('leaves them all alone, since none can be told from the others', () => { + const rules = [ + ruleFactory({ content: 'Duplicated content' }), + ruleFactory({ content: 'Duplicated content' }), + ]; + const source = standardVersionFactory({ rules }); + const proposal = changeProposalFactory({ + type: ChangeProposalType.updateRule, + payload: { + targetId: createRuleId('unresolved'), + oldValue: 'Duplicated content', + newValue: 'New content', + }, + }); + + const result = applier.applyChangeProposals(source, [ + proposal as ChangeProposal, + ]); + + expect( + (result.version.rules ?? []).map((rule) => rule.content), + ).toEqual(['Duplicated content', 'Duplicated content']); + }); + }); + }); + + describe('when the target id names a rule', () => { + it('ignores another rule sharing the replaced content', () => { + const targeted = ruleFactory({ + id: createRuleId('targeted'), + content: 'Shared content', + }); + const untouched = ruleFactory({ + id: createRuleId('untouched'), + content: 'Shared content', + }); + const source = standardVersionFactory({ + rules: [targeted, untouched], + }); + const proposal = changeProposalFactory({ + type: ChangeProposalType.updateRule, + payload: { + targetId: targeted.id, + oldValue: 'Shared content', + newValue: 'New content', + }, + }); + + const result = applier.applyChangeProposals(source, [ + proposal as ChangeProposal, + ]); + + expect((result.version.rules ?? [])[1].content).toBe( + 'Shared content', + ); + }); + }); }); describe('deleteRule', () => { @@ -512,6 +631,108 @@ describe('StandardChangeProposalApplier', () => { expect((result.version.rules ?? [])[0].id).toBe(ruleToKeep.id); }); }); + + describe('when the target id names no rule', () => { + it('removes the rule carrying the content the proposal names', () => { + const rule = ruleFactory({ content: 'To be deleted' }); + const source = standardVersionFactory({ rules: [rule] }); + const proposal = changeProposalFactory({ + type: ChangeProposalType.deleteRule, + payload: { + targetId: createRuleId('unresolved'), + item: { + id: createRuleId('unresolved'), + content: 'To be deleted', + }, + }, + }); + + const result = applier.applyChangeProposals(source, [ + proposal as ChangeProposal, + ]); + + expect(result.version.rules).toEqual([]); + }); + + it('keeps a rule whose content does not match', () => { + const rule = ruleFactory({ content: 'Keep me' }); + const source = standardVersionFactory({ rules: [rule] }); + const proposal = changeProposalFactory({ + type: ChangeProposalType.deleteRule, + payload: { + targetId: createRuleId('unresolved'), + item: { + id: createRuleId('unresolved'), + content: 'To be deleted', + }, + }, + }); + + const result = applier.applyChangeProposals(source, [ + proposal as ChangeProposal, + ]); + + expect((result.version.rules ?? [])[0].id).toBe(rule.id); + }); + + describe('when several rules carry that content', () => { + it('removes all of them, since the standard is meant to be rid of it', () => { + const rules = [ + ruleFactory({ content: 'Duplicated content' }), + ruleFactory({ content: 'Duplicated content' }), + ruleFactory({ content: 'Keep me' }), + ]; + const source = standardVersionFactory({ rules }); + const proposal = changeProposalFactory({ + type: ChangeProposalType.deleteRule, + payload: { + targetId: createRuleId('unresolved'), + item: { + id: createRuleId('unresolved'), + content: 'Duplicated content', + }, + }, + }); + + const result = applier.applyChangeProposals(source, [ + proposal as ChangeProposal, + ]); + + expect( + (result.version.rules ?? []).map((rule) => rule.content), + ).toEqual(['Keep me']); + }); + }); + }); + + describe('when the target id names a rule', () => { + it('keeps another rule sharing its content', () => { + const targeted = ruleFactory({ + id: createRuleId('targeted'), + content: 'Shared content', + }); + const untouched = ruleFactory({ + id: createRuleId('untouched'), + content: 'Shared content', + }); + const source = standardVersionFactory({ + rules: [targeted, untouched], + }); + const proposal = changeProposalFactory({ + type: ChangeProposalType.deleteRule, + payload: { + targetId: targeted.id, + item: { id: targeted.id, content: 'Shared content' }, + }, + }); + + const result = applier.applyChangeProposals(source, [ + proposal as ChangeProposal, + ]); + + expect((result.version.rules ?? [])[0].id).toBe(untouched.id); + }); + }); }); describe('multiple changes', () => { diff --git a/packages/types/src/playbookChangeManagement/applier/StandardChangeProposalApplier.ts b/packages/types/src/playbookChangeManagement/applier/StandardChangeProposalApplier.ts index aeb73aa92f..b6c3cce9f2 100644 --- a/packages/types/src/playbookChangeManagement/applier/StandardChangeProposalApplier.ts +++ b/packages/types/src/playbookChangeManagement/applier/StandardChangeProposalApplier.ts @@ -85,8 +85,25 @@ export class StandardChangeProposalApplier extends AbstractChangeProposalApplier ) ) { const rules = source.rules || []; + const targetId = changeProposal.payload.targetId; + const matchesTarget = rules.some((rule) => rule.id === targetId); + + // A client that cannot know rule ids — the CLI reads standards from + // Markdown, which carries none — sends a placeholder id and identifies + // the rule by the content it is replacing. Only fall back when exactly + // one rule carries that content: with several, there is no way to tell + // which was meant, and rewriting all of them would collapse them into + // duplicates. + const contentMatches = matchesTarget + ? [] + : rules.filter( + (rule) => rule.content === changeProposal.payload.oldValue, + ); + const fallbackId = + contentMatches.length === 1 ? contentMatches[0].id : undefined; + const updatedRules = rules.map((rule) => { - if (rule.id !== changeProposal.payload.targetId) { + if (rule.id !== targetId && rule.id !== fallbackId) { return rule; } @@ -113,9 +130,16 @@ export class StandardChangeProposalApplier extends AbstractChangeProposalApplier ) ) { const rules = source.rules || []; - const filteredRules = rules.filter( - (rule) => rule.id !== changeProposal.payload.targetId, - ); + const targetId = changeProposal.payload.targetId; + const matchesTarget = rules.some((rule) => rule.id === targetId); + + // As with updateRule, fall back to the content the proposal carries when + // its id names no rule. Every rule with that content goes: the client + // asked for it to be absent, and it holds no id to tell copies apart. + const removedContent = changeProposal.payload.item?.content; + const filteredRules = matchesTarget + ? rules.filter((rule) => rule.id !== targetId) + : rules.filter((rule) => rule.content !== removedContent); return { ...source, From 03a83d2dda762b0549874eb68bfa58d950a3fb23 Mon Sep 17 00:00:00 2001 From: Duncan Williams Date: Mon, 28 Sep 2026 14:29:59 +0100 Subject: [PATCH 2/2] =?UTF-8?q?=F0=9F=90=9B=20fix(standards):=20raise=20a?= =?UTF-8?q?=20conflict=20when=20a=20rule=20target=20resolves=20to=20nothin?= =?UTF-8?q?g?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Review caught two problems with the content fallback. The match read the raw payload while applyDiff reads the effective one, so a reviewer who adjusted an accepted decision would have the rule chosen by the value the client first sent and the edit computed from the value they replaced it with — a different rule, or none. Both branches now resolve through getEffectivePayload. The fallback also left the version untouched when the content matched no rule, or several, and the apply still saved and reported success. That is the silent loss this change exists to remove, reached by a narrower route, so both branches now raise ChangeProposalConflictError instead: the signal applyDiff already uses for a change that cannot be applied cleanly, and one clients already surface. Only the fallback path raises. A proposal whose id resolves behaves exactly as before, so nothing that works today starts failing — an id-less proposal previously applied to nothing at all. The exception worth naming is re-applying an id-less proposal that already took effect, which now reports a conflict rather than passing silently. Co-Authored-By: Claude Opus 5 --- CHANGELOG.MD | 1 + .../StandardChangeProposalApplier.spec.ts | 89 +++++++++++++++---- .../applier/StandardChangeProposalApplier.ts | 57 ++++++------ 3 files changed, 105 insertions(+), 42 deletions(-) diff --git a/CHANGELOG.MD b/CHANGELOG.MD index 8a6a8ec481..abbc51aa71 100644 --- a/CHANGELOG.MD +++ b/CHANGELOG.MD @@ -16,6 +16,7 @@ - Opening a component from All components keeps the rows already picked, and closing it goes back to that list rather than to the package carrying the component - Rule removals and edits submitted by a client that cannot know rule ids, such as the CLI reading standards from Markdown, are now matched by content instead of being dropped in silence +- A rule removal or edit that matches no rule the standard holds now reports a conflict the client can resolve, instead of being accepted and applied to nothing ## Removed diff --git a/packages/types/src/playbookChangeManagement/applier/StandardChangeProposalApplier.spec.ts b/packages/types/src/playbookChangeManagement/applier/StandardChangeProposalApplier.spec.ts index c0e7b8bda1..d6823439b9 100644 --- a/packages/types/src/playbookChangeManagement/applier/StandardChangeProposalApplier.spec.ts +++ b/packages/types/src/playbookChangeManagement/applier/StandardChangeProposalApplier.spec.ts @@ -490,7 +490,7 @@ describe('StandardChangeProposalApplier', () => { expect((result.version.rules ?? [])[0].id).toBe(rule.id); }); - it('leaves a rule whose content does not match', () => { + it('throws ChangeProposalConflictError when no rule carries it', () => { const rule = ruleFactory({ content: 'Something else entirely' }); const source = standardVersionFactory({ rules: [rule] }); const proposal = changeProposalFactory({ @@ -502,17 +502,13 @@ describe('StandardChangeProposalApplier', () => { }, }); - const result = applier.applyChangeProposals(source, [ - proposal as ChangeProposal, - ]); - - expect((result.version.rules ?? [])[0].content).toBe( - 'Something else entirely', - ); + expect(() => + applier.applyChangeProposals(source, [proposal as ChangeProposal]), + ).toThrow(ChangeProposalConflictError); }); describe('when several rules carry the replaced content', () => { - it('leaves them all alone, since none can be told from the others', () => { + it('throws ChangeProposalConflictError, since none can be told from the others', () => { const rules = [ ruleFactory({ content: 'Duplicated content' }), ruleFactory({ content: 'Duplicated content' }), @@ -527,13 +523,38 @@ describe('StandardChangeProposalApplier', () => { }, }); + expect(() => + applier.applyChangeProposals(source, [ + proposal as ChangeProposal, + ]), + ).toThrow(ChangeProposalConflictError); + }); + }); + + describe('when a reviewer adjusted the decision', () => { + it('matches the rule named by the decision, not by the payload', () => { + const rule = ruleFactory({ content: 'Reviewer chose this one' }); + const source = standardVersionFactory({ rules: [rule] }); + const proposal = changeProposalFactory({ + type: ChangeProposalType.updateRule, + payload: { + targetId: createRuleId('unresolved'), + oldValue: 'What the client originally sent', + newValue: 'New content', + }, + status: ChangeProposalStatus.applied, + decision: { + targetId: createRuleId('unresolved'), + oldValue: 'Reviewer chose this one', + newValue: 'New content', + }, + }); + const result = applier.applyChangeProposals(source, [ proposal as ChangeProposal, ]); - expect( - (result.version.rules ?? []).map((rule) => rule.content), - ).toEqual(['Duplicated content', 'Duplicated content']); + expect((result.version.rules ?? [])[0].content).toBe('New content'); }); }); }); @@ -654,7 +675,7 @@ describe('StandardChangeProposalApplier', () => { expect(result.version.rules).toEqual([]); }); - it('keeps a rule whose content does not match', () => { + it('throws ChangeProposalConflictError when no rule carries that content', () => { const rule = ruleFactory({ content: 'Keep me' }); const source = standardVersionFactory({ rules: [rule] }); const proposal = changeProposalFactory({ @@ -668,11 +689,45 @@ describe('StandardChangeProposalApplier', () => { }, }); - const result = applier.applyChangeProposals(source, [ - proposal as ChangeProposal, - ]); + expect(() => + applier.applyChangeProposals(source, [proposal as ChangeProposal]), + ).toThrow(ChangeProposalConflictError); + }); - expect((result.version.rules ?? [])[0].id).toBe(rule.id); + describe('when a reviewer adjusted the decision', () => { + it('removes the rule named by the decision, not by the payload', () => { + const rules = [ + ruleFactory({ content: 'Reviewer chose this one' }), + ruleFactory({ content: 'What the client originally sent' }), + ]; + const source = standardVersionFactory({ rules }); + const proposal = changeProposalFactory({ + type: ChangeProposalType.deleteRule, + payload: { + targetId: createRuleId('unresolved'), + item: { + id: createRuleId('unresolved'), + content: 'What the client originally sent', + }, + }, + status: ChangeProposalStatus.applied, + decision: { + targetId: createRuleId('unresolved'), + item: { + id: createRuleId('unresolved'), + content: 'Reviewer chose this one', + }, + }, + }); + + const result = applier.applyChangeProposals(source, [ + proposal as ChangeProposal, + ]); + + expect( + (result.version.rules ?? []).map((rule) => rule.content), + ).toEqual(['What the client originally sent']); + }); }); describe('when several rules carry that content', () => { diff --git a/packages/types/src/playbookChangeManagement/applier/StandardChangeProposalApplier.ts b/packages/types/src/playbookChangeManagement/applier/StandardChangeProposalApplier.ts index b6c3cce9f2..9580ff2d35 100644 --- a/packages/types/src/playbookChangeManagement/applier/StandardChangeProposalApplier.ts +++ b/packages/types/src/playbookChangeManagement/applier/StandardChangeProposalApplier.ts @@ -2,7 +2,8 @@ import { AbstractChangeProposalApplier } from './AbstractChangeProposalApplier'; import { ChangeProposal } from '../ChangeProposal'; import { ChangeProposalType } from '../ChangeProposalType'; import { StandardVersion } from '../../standards/StandardVersion'; -import { createRuleId } from '../../standards/RuleId'; +import { createRuleId, RuleId } from '../../standards/RuleId'; +import { ChangeProposalConflictError } from './ChangeProposalConflictError'; import { isExpectedChangeProposalType } from './isExpectedChangeProposalType'; import { STANDARD_CHANGE_TYPES } from './types'; @@ -86,21 +87,20 @@ export class StandardChangeProposalApplier extends AbstractChangeProposalApplier ) { const rules = source.rules || []; const targetId = changeProposal.payload.targetId; - const matchesTarget = rules.some((rule) => rule.id === targetId); - - // A client that cannot know rule ids — the CLI reads standards from - // Markdown, which carries none — sends a placeholder id and identifies - // the rule by the content it is replacing. Only fall back when exactly - // one rule carries that content: with several, there is no way to tell - // which was meant, and rewriting all of them would collapse them into - // duplicates. - const contentMatches = matchesTarget - ? [] - : rules.filter( - (rule) => rule.content === changeProposal.payload.oldValue, - ); - const fallbackId = - contentMatches.length === 1 ? contentMatches[0].id : undefined; + + // A client reading standards from Markdown has no rule ids to send, so + // it names the rule by the content it is replacing instead. + let fallbackId: RuleId | undefined; + if (!rules.some((rule) => rule.id === targetId)) { + const matches = rules.filter( + (rule) => + rule.content === this.getEffectivePayload(changeProposal).oldValue, + ); + if (matches.length !== 1) { + throw new ChangeProposalConflictError(changeProposal.id); + } + fallbackId = matches[0].id; + } const updatedRules = rules.map((rule) => { if (rule.id !== targetId && rule.id !== fallbackId) { @@ -131,19 +131,26 @@ export class StandardChangeProposalApplier extends AbstractChangeProposalApplier ) { const rules = source.rules || []; const targetId = changeProposal.payload.targetId; - const matchesTarget = rules.some((rule) => rule.id === targetId); - // As with updateRule, fall back to the content the proposal carries when - // its id names no rule. Every rule with that content goes: the client - // asked for it to be absent, and it holds no id to tell copies apart. - const removedContent = changeProposal.payload.item?.content; - const filteredRules = matchesTarget - ? rules.filter((rule) => rule.id !== targetId) - : rules.filter((rule) => rule.content !== removedContent); + if (rules.some((rule) => rule.id === targetId)) { + return { + ...source, + rules: rules.filter((rule) => rule.id !== targetId), + }; + } + + // Every copy goes: the standard is meant to be rid of that content, and + // copies hold no id to tell them apart. + const removed = this.getEffectivePayload(changeProposal).item.content; + const remaining = rules.filter((rule) => rule.content !== removed); + + if (remaining.length === rules.length) { + throw new ChangeProposalConflictError(changeProposal.id); + } return { ...source, - rules: filteredRules, + rules: remaining, }; }