🐛 fix(standards): match rule removals and edits by content when the id is unknown

CHANGELOG.MD

@@ -27,6 +27,8 @@
 - A repository cloned over `ssh://` or with a user in its URL no longer gets a CLI-managed connection of its own
 - A self-managed GitLab installed under a path such as `/gitlab` matches its connection: the path is no longer read as part of the group
 - 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
 - A GitLab repository in a subgroup keeps its full path (`group/subgroup/repo`) when tracked from the CLI, so a token or app connection links it instead of duplicating it
 
 ## Removed

packages/types/src/playbookChangeManagement/applier/StandardChangeProposalApplier.spec.ts

@@ -450,6 +450,146 @@ 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('throws ChangeProposalConflictError when no rule carries it', () => {
+          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',
+            },
+          });
+
+          expect(() =>
+            applier.applyChangeProposals(source, [proposal as ChangeProposal]),
+          ).toThrow(ChangeProposalConflictError);
+        });
+
+        describe('when several rules carry the replaced content', () => {
+          it('throws ChangeProposalConflictError, 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',
+              },
+            });
+
+            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 ?? [])[0].content).toBe('New 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 +652,142 @@ 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('throws ChangeProposalConflictError when no rule carries that content', () => {
+          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',
+              },
+            },
+          });
+
+          expect(() =>
+            applier.applyChangeProposals(source, [proposal as ChangeProposal]),
+          ).toThrow(ChangeProposalConflictError);
+        });
+
+        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', () => {
+          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', () => {

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';
 
@@ -85,8 +86,24 @@ export class StandardChangeProposalApplier extends AbstractChangeProposalApplier
       )
     ) {
       const rules = source.rules || [];
+      const targetId = changeProposal.payload.targetId;
+
+      // 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 !== changeProposal.payload.targetId) {
+        if (rule.id !== targetId && rule.id !== fallbackId) {
           return rule;
         }
 
@@ -113,13 +130,27 @@ export class StandardChangeProposalApplier extends AbstractChangeProposalApplier
       )
     ) {
       const rules = source.rules || [];
-      const filteredRules = rules.filter(
-        (rule) => rule.id !== changeProposal.payload.targetId,
-      );
+      const targetId = changeProposal.payload.targetId;
+
+      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,
       };
     }