From a41d51a174439a0fa56a591024b48a8a67edcc02 Mon Sep 17 00:00:00 2001 From: Duncan Williams Date: Wed, 23 Sep 2026 15:59:32 +0100 Subject: [PATCH 1/2] =?UTF-8?q?=F0=9F=90=9B=20fix(cli):=20resolve=20rule?= =?UTF-8?q?=20ids=20before=20submitting=20a=20standard?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A rule removal or edit is applied by matching payload.targetId against the rule's id, and compareStandardFields stamped every one of them with the placeholder createRuleId('unresolved'). The local Markdown carries no ids, so there was nothing else to put there. StandardChangeProposalApplier filters on `rule.id !== targetId`, so the placeholder matches nothing and the change is applied to nothing. With `--no-review`, where proposals go straight to batchApply, that is the whole story: the CLI prints "1 standard updated", drops the staged change, and the rule is still there. Nothing retries it, because the local playbook no longer knows anything is pending. The CLI already had the missing piece: StandardsGateway.getRules returns { id, content } for a standard and was called from nowhere. buildProposals now takes a fetcher, reads the rules once per standard, and passes a content-to-id map down to compareStandardFields. Left deliberately alone: - StandardDiffStrategy keeps the placeholder. It feeds `playbook diff`, which only prints, so no id is ever dereferenced. - A rule whose content is not in the map keeps the placeholder too, so a failed or partial lookup behaves as it did before rather than blocking a submit. It now warns instead of reporting success in silence. Co-Authored-By: Claude Opus 5 --- CHANGELOG.MD | 2 + .../utils/artifactComparison.spec.ts | 90 +++++++++++ .../application/utils/artifactComparison.ts | 41 ++++- .../playbook/submit/proposalBuilder.spec.ts | 143 +++++++++++++++++- .../playbook/submit/proposalBuilder.ts | 61 +++++++- .../infra/commands/playbook/submitHandler.ts | 21 ++- 6 files changed, 352 insertions(+), 6 deletions(-) diff --git a/CHANGELOG.MD b/CHANGELOG.MD index 7cd8c2d0c6..bd594d7cfc 100644 --- a/CHANGELOG.MD +++ b/CHANGELOG.MD @@ -8,6 +8,8 @@ ## Fixed +- `playbook submit` now resolves a standard's rule ids before submitting, so removing or editing a rule is applied rather than silently dropped when submitted with `--no-review` + ## Removed # [1.18.0] - 2026-09-22 diff --git a/apps/cli/src/application/utils/artifactComparison.spec.ts b/apps/cli/src/application/utils/artifactComparison.spec.ts index 6b5ec76d62..733685f97d 100644 --- a/apps/cli/src/application/utils/artifactComparison.spec.ts +++ b/apps/cli/src/application/utils/artifactComparison.spec.ts @@ -7,6 +7,10 @@ import { compareStandardFields, } from './artifactComparison'; +jest.mock('../../infra/utils/consoleLogger', () => ({ + logWarningConsole: jest.fn(), +})); + const PACKMIND_PATH = '.packmind/standards/my-standard.md'; function buildStandard(opts: { @@ -178,6 +182,92 @@ describe('compareStandardFields', () => { ); }); + describe('when rule ids are supplied', () => { + const DELETED_RULE = 'Completely unique rule xyz'; + + function deletionChanges(ruleIds?: ReadonlyMap) { + const local = buildStandard({ rules: ['Rule one'] }); + const deployed = buildStandard({ rules: ['Rule one', DELETED_RULE] }); + return compareStandardFields(local, deployed, PACKMIND_PATH, ruleIds); + } + + it('targets a deleted rule by its server id', () => { + const changes = deletionChanges( + new Map([ + ['Rule one', 'rule-1'], + [DELETED_RULE, 'rule-2'], + ]), + ); + expect(changes).toContainEqual( + expect.objectContaining({ + type: ChangeProposalType.deleteRule, + payload: expect.objectContaining({ targetId: 'rule-2' }), + }), + ); + }); + + it('carries the server id on the deleted rule item', () => { + const changes = deletionChanges(new Map([[DELETED_RULE, 'rule-2']])); + expect(changes).toContainEqual( + expect.objectContaining({ + type: ChangeProposalType.deleteRule, + payload: expect.objectContaining({ + item: { id: 'rule-2', content: DELETED_RULE }, + }), + }), + ); + }); + + it('targets an updated rule by the id of the content it replaces', () => { + const local = buildStandard({ + rules: ['Use camelCase for all variable names'], + }); + const deployed = buildStandard({ + rules: ['Use camelCase for variable names'], + }); + const changes = compareStandardFields( + local, + deployed, + PACKMIND_PATH, + new Map([['Use camelCase for variable names', 'rule-9']]), + ); + expect(changes).toContainEqual( + expect.objectContaining({ + type: ChangeProposalType.updateRule, + payload: expect.objectContaining({ targetId: 'rule-9' }), + }), + ); + }); + + describe('when the deleted rule is absent from the supplied ids', () => { + it('falls back to the unresolved placeholder', () => { + const changes = deletionChanges(new Map([['Rule one', 'rule-1']])); + expect(changes).toContainEqual( + expect.objectContaining({ + type: ChangeProposalType.deleteRule, + payload: expect.objectContaining({ targetId: 'unresolved' }), + }), + ); + }); + }); + }); + + describe('when no rule ids are supplied', () => { + it('falls back to the unresolved placeholder', () => { + const local = buildStandard({ rules: ['Rule one'] }); + const deployed = buildStandard({ + rules: ['Rule one', 'Completely unique rule xyz'], + }); + const changes = compareStandardFields(local, deployed, PACKMIND_PATH); + expect(changes).toContainEqual( + expect.objectContaining({ + type: ChangeProposalType.deleteRule, + payload: expect.objectContaining({ targetId: 'unresolved' }), + }), + ); + }); + }); + describe('when parsing fails', () => { it('returns empty array', () => { const result = compareStandardFields( diff --git a/apps/cli/src/application/utils/artifactComparison.ts b/apps/cli/src/application/utils/artifactComparison.ts index 11dcd4cdb5..e73eea9069 100644 --- a/apps/cli/src/application/utils/artifactComparison.ts +++ b/apps/cli/src/application/utils/artifactComparison.ts @@ -3,11 +3,21 @@ import { ChangeProposalType, canonicalJsonStringify, createRuleId, + RuleId, } from '@packmind/types'; import { parseCommandFile } from './parseCommandFile'; import { parseStandardMd } from './parseStandardMd'; import { matchUpdatedRules } from './ruleSimilarity'; +import { logWarningConsole } from '../../infra/utils/consoleLogger'; + +const RULE_LABEL_MAX_LENGTH = 60; + +function truncateRule(content: string): string { + return content.length > RULE_LABEL_MAX_LENGTH + ? `${content.slice(0, RULE_LABEL_MAX_LENGTH)}...` + : content; +} export type FieldChange = { type: ChangeProposalType; @@ -25,10 +35,21 @@ export type SkillDefinitionInput = { additionalProperties?: Record; }; +/** + * Maps a deployed rule's content to its server-side id. + * + * `deleteRule` and `updateRule` proposals are applied by matching + * `payload.targetId` against the rule's real id, so a proposal built without + * one silently applies to nothing. The caller fetches the ids; passing the map + * is optional so that callers which only display a diff stay synchronous. + */ +export type RuleIdsByContent = ReadonlyMap; + export function compareStandardFields( localContent: string, deployedContent: string, filePath: string, + ruleIdsByContent?: RuleIdsByContent, ): FieldChange[] { const localParsed = parseStandardMd(localContent, filePath); const serverParsed = parseStandardMd(deployedContent, filePath); @@ -105,8 +126,24 @@ export function compareStandardFields( addedRules, ); + // The placeholder kept for callers that supply no ids matches no rule, so + // the change it names is applied to nothing. A caller that did supply ids + // and still missed one is warned, rather than left to read a success line + // for a change that was dropped. + const resolveRuleId = (content: string, changeLabel: string): RuleId => { + const resolved = ruleIdsByContent?.get(content); + if (resolved) return createRuleId(resolved); + + if (ruleIdsByContent) { + logWarningConsole( + `Could not resolve the rule "${truncateRule(content)}" in ${filePath} to a known rule; its ${changeLabel} may not be applied.`, + ); + } + return createRuleId('unresolved'); + }; + for (const update of updates) { - const ruleId = createRuleId('unresolved'); + const ruleId = resolveRuleId(update.oldValue, 'update'); changes.push({ type: ChangeProposalType.updateRule, payload: { @@ -118,7 +155,7 @@ export function compareStandardFields( } for (const rule of remainingDeleted) { - const ruleId = createRuleId('unresolved'); + const ruleId = resolveRuleId(rule, 'removal'); changes.push({ type: ChangeProposalType.deleteRule, payload: { diff --git a/apps/cli/src/infra/commands/playbook/submit/proposalBuilder.spec.ts b/apps/cli/src/infra/commands/playbook/submit/proposalBuilder.spec.ts index 019376c93e..219ffb7d6d 100644 --- a/apps/cli/src/infra/commands/playbook/submit/proposalBuilder.spec.ts +++ b/apps/cli/src/infra/commands/playbook/submit/proposalBuilder.spec.ts @@ -1,4 +1,4 @@ -import { ChangeProposalType } from '@packmind/types'; +import { ChangeProposalType, PackmindLockFileFile } from '@packmind/types'; import { PlaybookChangeEntry } from '../../../../domain/repositories/IPlaybookLocalRepository'; import { PackmindLockFile } from '../../../../domain/repositories/PackmindLockFile'; import { @@ -1120,3 +1120,144 @@ describe('buildProposals', () => { }); }); }); + +describe('buildProposals rule id resolution', () => { + const DELETED_RULE = 'Completely unique rule xyz'; + const PACKMIND_FILE = '.packmind/standards/my-standard.md'; + const CLAUDE_FILE = '.claude/rules/packmind/standard-my-standard.md'; + + const DEPLOYED_WITH_EXTRA_RULE = [ + '# My Standard', + '', + 'A description of the standard.', + '', + '## Rules', + '', + '* Do not use var', + '* Always use const', + `* ${DELETED_RULE}`, + ].join('\n'); + + function makeStandardLockFile(files: PackmindLockFileFile[]) { + return makeLockFile({ + artifacts: { + 'my-standard': { + source: 'user', + name: 'My Standard', + type: 'standard', + id: 'artifact-1', + version: 1, + spaceId: 'space-123', + packageIds: [], + files, + }, + }, + }); + } + + function makeCtx(files: PackmindLockFileFile[]) { + return jest.fn().mockResolvedValue( + makeTargetContext({ + lockFile: makeStandardLockFile(files), + deployedFiles: files.map((file) => ({ + path: file.path, + content: DEPLOYED_WITH_EXTRA_RULE, + })), + }), + ); + } + + afterEach(() => jest.clearAllMocks()); + + it('targets a deleted rule by the id fetched from the standard', async () => { + const getCtx = makeCtx([{ path: PACKMIND_FILE, agent: 'packmind' }]); + const fetchRuleIds = jest + .fn() + .mockResolvedValue(new Map([[DELETED_RULE, 'rule-xyz']])); + + const { proposals } = await buildProposals( + [makeEntry({ changeType: 'updated' })], + getCtx, + fetchRuleIds, + ); + + expect(proposals).toContainEqual( + expect.objectContaining({ + type: ChangeProposalType.deleteRule, + payload: expect.objectContaining({ targetId: 'rule-xyz' }), + }), + ); + }); + + it('fetches the rules with the space and artifact of the standard', async () => { + const getCtx = makeCtx([{ path: PACKMIND_FILE, agent: 'packmind' }]); + const fetchRuleIds = jest.fn().mockResolvedValue(new Map()); + + await buildProposals( + [makeEntry({ changeType: 'updated' })], + getCtx, + fetchRuleIds, + ); + + expect(fetchRuleIds).toHaveBeenCalledWith('space-123', 'artifact-1'); + }); + + describe('when several agents render the same standard', () => { + it('fetches that standard rules once', async () => { + const files: PackmindLockFileFile[] = [ + { path: PACKMIND_FILE, agent: 'packmind' }, + { path: CLAUDE_FILE, agent: 'claude' }, + ]; + const getCtx = makeCtx(files); + const fetchRuleIds = jest.fn().mockResolvedValue(new Map()); + + await buildProposals( + [ + makeEntry({ changeType: 'updated' }), + makeEntry({ + changeType: 'updated', + filePath: CLAUDE_FILE, + codingAgent: 'claude', + }), + ], + getCtx, + fetchRuleIds, + ); + + expect(fetchRuleIds).toHaveBeenCalledTimes(1); + }); + }); + + describe('when the rule lookup fails', () => { + it('still builds the proposals', async () => { + const getCtx = makeCtx([{ path: PACKMIND_FILE, agent: 'packmind' }]); + const fetchRuleIds = jest.fn().mockRejectedValue(new Error('offline')); + + const { proposals } = await buildProposals( + [makeEntry({ changeType: 'updated' })], + getCtx, + fetchRuleIds, + ); + + expect(proposals.length).toBeGreaterThan(0); + }); + }); + + describe('when no fetcher is provided', () => { + it('falls back to the unresolved placeholder', async () => { + const getCtx = makeCtx([{ path: PACKMIND_FILE, agent: 'packmind' }]); + + const { proposals } = await buildProposals( + [makeEntry({ changeType: 'updated' })], + getCtx, + ); + + expect(proposals).toContainEqual( + expect.objectContaining({ + type: ChangeProposalType.deleteRule, + payload: expect.objectContaining({ targetId: 'unresolved' }), + }), + ); + }); + }); +}); diff --git a/apps/cli/src/infra/commands/playbook/submit/proposalBuilder.ts b/apps/cli/src/infra/commands/playbook/submit/proposalBuilder.ts index e87ae3a544..61f120a24d 100644 --- a/apps/cli/src/infra/commands/playbook/submit/proposalBuilder.ts +++ b/apps/cli/src/infra/commands/playbook/submit/proposalBuilder.ts @@ -12,6 +12,7 @@ import { compareStandardFields, compareCommandFields, compareSkillDefinitionFields, + RuleIdsByContent, } from '../../../../application/utils/artifactComparison'; import { normalizePath } from '../../../../application/utils/pathUtils'; import { logWarningConsole } from '../../../utils/consoleLogger'; @@ -142,6 +143,7 @@ function buildUpdatedStandardProposals( entry: PlaybookChangeEntry, artifactId: string | null, deployedContent: string | null, + ruleIdsByContent?: RuleIdsByContent, ): ProposalItem[] { if (!artifactId) return []; @@ -156,6 +158,7 @@ function buildUpdatedStandardProposals( entry.content, deployedContent, entry.filePath, + ruleIdsByContent, ); return fieldChanges.map((change) => ({ ...base, @@ -445,9 +448,51 @@ function toSkippedEntry( }; } +/** + * Fetches a standard's rule ids once and remembers the answer, including the + * failure. A lookup that fails must not stop the submit: the proposals still + * carry the rule content, so the worst case is the behaviour we had before. + */ +async function resolveRuleIds( + fetchRuleIds: RuleIdsFetcher, + spaceId: string, + standardId: string, + artifactName: string, + cache: Map, +): Promise { + const cacheKey = `${spaceId}/${standardId}`; + if (cache.has(cacheKey)) return cache.get(cacheKey); + + let resolved: RuleIdsByContent | undefined; + try { + resolved = await fetchRuleIds(spaceId, standardId); + } catch (error) { + const reason = error instanceof Error ? error.message : String(error); + logWarningConsole( + `Could not read the rules of "${artifactName}" (${reason}); rule removals and edits may not be applied.`, + ); + resolved = undefined; + } + + cache.set(cacheKey, resolved); + return resolved; +} + +/** + * Looks up the rules a deployed standard currently has, keyed by content. + * + * Rule removals and edits are applied by id, and the local Markdown carries + * none, so without this the proposal names no rule and is dropped in silence. + */ +export type RuleIdsFetcher = ( + spaceId: string, + standardId: string, +) => Promise; + export async function buildProposals( changes: PlaybookChangeEntry[], getTargetContext: (entry: PlaybookChangeEntry) => Promise, + fetchRuleIds?: RuleIdsFetcher, ): Promise<{ proposals: ProposalItem[]; conflicts: ArtifactConflict[]; @@ -455,6 +500,7 @@ export async function buildProposals( }> { const proposals: ProposalItem[] = []; const skipped: SkippedEntry[] = []; + const ruleIdCache = new Map(); const updateSources = new Map< string, { @@ -546,15 +592,28 @@ export async function buildProposals( } switch (entry.artifactType) { - case 'standard': + case 'standard': { + // One lookup per standard, reused when several agents render it. + let ruleIdsByContent: RuleIdsByContent | undefined; + if (fetchRuleIds && deployedContent) { + ruleIdsByContent = await resolveRuleIds( + fetchRuleIds, + entry.spaceId, + artifactId, + entry.artifactName, + ruleIdCache, + ); + } proposals.push( ...buildUpdatedStandardProposals( entry, artifactId, deployedContent, + ruleIdsByContent, ), ); break; + } case 'command': proposals.push( ...buildUpdatedCommandProposals(entry, artifactId, deployedContent), diff --git a/apps/cli/src/infra/commands/playbook/submitHandler.ts b/apps/cli/src/infra/commands/playbook/submitHandler.ts index 26aca859ae..38736a03df 100644 --- a/apps/cli/src/infra/commands/playbook/submitHandler.ts +++ b/apps/cli/src/infra/commands/playbook/submitHandler.ts @@ -18,7 +18,11 @@ import { duplicateNameKey, } from './submit/duplicateNameChecker'; import { createTargetContextResolver } from './submit/targetContextResolver'; -import { buildProposals, ProposalItem } from './submit/proposalBuilder'; +import { + buildProposals, + ProposalItem, + RuleIdsFetcher, +} from './submit/proposalBuilder'; import { validateProposalSkillDescriptions } from './submit/skillDescriptionValidator'; import { fetchAvailablePackageSlugs, @@ -180,12 +184,25 @@ export async function playbookSubmitHandler( packmindCliHexa, }); + // Rule removals and edits are applied by rule id, and a standard's Markdown + // carries none, so they are read back from the standard before submitting. + const fetchRuleIds: RuleIdsFetcher = async (spaceId, standardId) => { + const rules = await packmindCliHexa + .getPackmindGateway() + .standards.getRules(spaceId, standardId); + return new Map(rules.map((rule) => [rule.content, rule.id])); + }; + // Build proposals const { proposals: allProposals, conflicts, skipped, - } = await buildProposals(submittableChanges, resolver.getTargetContext); + } = await buildProposals( + submittableChanges, + resolver.getTargetContext, + fetchRuleIds, + ); // Pre-flight: reject skill proposals whose description exceeds the limit. // The backend enforces the same cap; failing fast here keeps the error tied From e5fdaeaba5d6c534b813b4c0790a384feefeec3c Mon Sep 17 00:00:00 2001 From: Duncan Williams Date: Wed, 23 Sep 2026 16:47:19 +0100 Subject: [PATCH 2/2] =?UTF-8?q?=F0=9F=90=9B=20fix(cli):=20keep=20the=20sta?= =?UTF-8?q?ged=20change=20when=20a=20rule=20id=20cannot=20be=20resolved?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Review caught that the fallback undid the fix. If getRules failed, the submit carried on without a map, the placeholder went out, the server accepted the standard update while skipping the rule operations, and the success path then cleared the staged change. That is the data loss this branch exists to remove, reached by a different route, and a warning is thin cover when the next line reads "1 standard updated" and there is nothing left to retry. A standard whose removals or edits still carry the placeholder is now reported through the skipped path the builder already uses for stale deployed content, so the handler keeps the staged change and exits 1. Additions are exempt. They name no existing rule, so a failed lookup does not make them ambiguous and a standard that only gained rules still submits. That is why the check is on the built proposals rather than on the lookup: it catches a partial answer that omits one rule just as well as a lookup that failed outright. Co-Authored-By: Claude Opus 5 --- .../application/utils/artifactComparison.ts | 9 ++- .../playbook/submit/proposalBuilder.spec.ts | 67 ++++++++++++++++++- .../playbook/submit/proposalBuilder.ts | 48 ++++++++++--- .../commands/playbook/submitHandler.spec.ts | 7 ++ 4 files changed, 119 insertions(+), 12 deletions(-) diff --git a/apps/cli/src/application/utils/artifactComparison.ts b/apps/cli/src/application/utils/artifactComparison.ts index e73eea9069..028f675b29 100644 --- a/apps/cli/src/application/utils/artifactComparison.ts +++ b/apps/cli/src/application/utils/artifactComparison.ts @@ -45,6 +45,13 @@ export type SkillDefinitionInput = { */ export type RuleIdsByContent = ReadonlyMap; +/** + * Stands in for a rule id that could not be resolved. It matches no rule, so a + * proposal carrying it is applied to nothing; callers that asked for ids treat + * its presence as a failure rather than shipping it. + */ +export const UNRESOLVED_RULE_ID = 'unresolved'; + export function compareStandardFields( localContent: string, deployedContent: string, @@ -139,7 +146,7 @@ export function compareStandardFields( `Could not resolve the rule "${truncateRule(content)}" in ${filePath} to a known rule; its ${changeLabel} may not be applied.`, ); } - return createRuleId('unresolved'); + return createRuleId(UNRESOLVED_RULE_ID); }; for (const update of updates) { diff --git a/apps/cli/src/infra/commands/playbook/submit/proposalBuilder.spec.ts b/apps/cli/src/infra/commands/playbook/submit/proposalBuilder.spec.ts index 219ffb7d6d..34b1af0da3 100644 --- a/apps/cli/src/infra/commands/playbook/submit/proposalBuilder.spec.ts +++ b/apps/cli/src/infra/commands/playbook/submit/proposalBuilder.spec.ts @@ -1229,7 +1229,20 @@ describe('buildProposals rule id resolution', () => { }); describe('when the rule lookup fails', () => { - it('still builds the proposals', async () => { + it('skips the entry rather than submitting an unresolved removal', async () => { + const getCtx = makeCtx([{ path: PACKMIND_FILE, agent: 'packmind' }]); + const fetchRuleIds = jest.fn().mockRejectedValue(new Error('offline')); + + const { skipped } = await buildProposals( + [makeEntry({ changeType: 'updated' })], + getCtx, + fetchRuleIds, + ); + + expect(skipped).toHaveLength(1); + }); + + it('submits no proposal for the skipped standard', async () => { const getCtx = makeCtx([{ path: PACKMIND_FILE, agent: 'packmind' }]); const fetchRuleIds = jest.fn().mockRejectedValue(new Error('offline')); @@ -1239,7 +1252,57 @@ describe('buildProposals rule id resolution', () => { fetchRuleIds, ); - expect(proposals.length).toBeGreaterThan(0); + expect(proposals).toEqual([]); + }); + + describe('when the standard only gained rules', () => { + it('submits it anyway, since an addition names no existing rule', async () => { + const deployedSubset = [ + '# My Standard', + '', + 'A description of the standard.', + '', + '## Rules', + '', + '* Do not use var', + ].join('\n'); + const getCtx = jest.fn().mockResolvedValue( + makeTargetContext({ + lockFile: makeStandardLockFile([ + { path: PACKMIND_FILE, agent: 'packmind' }, + ]), + deployedFiles: [{ path: PACKMIND_FILE, content: deployedSubset }], + }), + ); + const fetchRuleIds = jest.fn().mockRejectedValue(new Error('offline')); + + const { proposals } = await buildProposals( + [makeEntry({ changeType: 'updated' })], + getCtx, + fetchRuleIds, + ); + + expect(proposals).toContainEqual( + expect.objectContaining({ type: ChangeProposalType.addRule }), + ); + }); + }); + }); + + describe('when a changed rule is missing from the fetched ids', () => { + it('skips the entry', async () => { + const getCtx = makeCtx([{ path: PACKMIND_FILE, agent: 'packmind' }]); + const fetchRuleIds = jest + .fn() + .mockResolvedValue(new Map([['Some other rule', 'rule-other']])); + + const { skipped } = await buildProposals( + [makeEntry({ changeType: 'updated' })], + getCtx, + fetchRuleIds, + ); + + expect(skipped).toHaveLength(1); }); }); diff --git a/apps/cli/src/infra/commands/playbook/submit/proposalBuilder.ts b/apps/cli/src/infra/commands/playbook/submit/proposalBuilder.ts index 61f120a24d..91dd3ea743 100644 --- a/apps/cli/src/infra/commands/playbook/submit/proposalBuilder.ts +++ b/apps/cli/src/infra/commands/playbook/submit/proposalBuilder.ts @@ -13,6 +13,7 @@ import { compareCommandFields, compareSkillDefinitionFields, RuleIdsByContent, + UNRESOLVED_RULE_ID, } from '../../../../application/utils/artifactComparison'; import { normalizePath } from '../../../../application/utils/pathUtils'; import { logWarningConsole } from '../../../utils/consoleLogger'; @@ -448,10 +449,25 @@ function toSkippedEntry( }; } +/** + * True when a rule removal or edit ended up with the placeholder id. The server + * matches these by id, so such a proposal is applied to nothing while the batch + * still reports success and the staged change is cleared. Additions are exempt: + * they name no existing rule. + */ +function hasUnresolvedRuleTarget(proposals: ProposalItem[]): boolean { + return proposals.some( + (proposal) => + (proposal.type === ChangeProposalType.deleteRule || + proposal.type === ChangeProposalType.updateRule) && + (proposal.payload as { targetId?: string } | undefined)?.targetId === + UNRESOLVED_RULE_ID, + ); +} + /** * Fetches a standard's rule ids once and remembers the answer, including the - * failure. A lookup that fails must not stop the submit: the proposals still - * carry the rule content, so the worst case is the behaviour we had before. + * failure, so one unreachable standard costs one request. */ async function resolveRuleIds( fetchRuleIds: RuleIdsFetcher, @@ -604,14 +620,28 @@ export async function buildProposals( ruleIdCache, ); } - proposals.push( - ...buildUpdatedStandardProposals( - entry, - artifactId, - deployedContent, - ruleIdsByContent, - ), + const standardProposals = buildUpdatedStandardProposals( + entry, + artifactId, + deployedContent, + ruleIdsByContent, ); + // Shipping a placeholder would lose the change in silence, which is + // what this resolution exists to prevent. Keep the staged change so + // the operator can retry instead. + if (fetchRuleIds && hasUnresolvedRuleTarget(standardProposals)) { + logWarningConsole( + `Skipping "${entry.artifactName}" — its rules could not be matched to the deployed standard, so a rule removal or edit would not be applied.`, + ); + skipped.push( + toSkippedEntry( + entry, + 'rule removals and edits could not be matched to the deployed standard', + ), + ); + continue; + } + proposals.push(...standardProposals); break; } case 'command': diff --git a/apps/cli/src/infra/commands/playbook/submitHandler.spec.ts b/apps/cli/src/infra/commands/playbook/submitHandler.spec.ts index 597715df7e..ef27b8c050 100644 --- a/apps/cli/src/infra/commands/playbook/submitHandler.spec.ts +++ b/apps/cli/src/infra/commands/playbook/submitHandler.spec.ts @@ -87,6 +87,13 @@ describe('playbookSubmitHandler', () => { let mockLockFileRepository: jest.Mocked; beforeEach(() => { mockGateway = createMockPackmindGateway(); + // The submit path reads a standard's rule ids back before proposing a + // removal or an edit, so the fixtures' deployed rules need ids here. + mockGateway.standards.getRules.mockResolvedValue([ + { id: 'rule-do-not-use-var', content: 'Do not use var' }, + { id: 'rule-use-semicolons', content: 'Use semicolons' }, + { id: 'rule-old', content: 'Old rule' }, + ]); mockGateway.changeProposals.batchCreate.mockResolvedValue({ created: 1, skipped: 0,