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..028f675b29 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,28 @@ 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; + +/** + * 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, filePath: string, + ruleIdsByContent?: RuleIdsByContent, ): FieldChange[] { const localParsed = parseStandardMd(localContent, filePath); const serverParsed = parseStandardMd(deployedContent, filePath); @@ -105,8 +133,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_RULE_ID); + }; + for (const update of updates) { - const ruleId = createRuleId('unresolved'); + const ruleId = resolveRuleId(update.oldValue, 'update'); changes.push({ type: ChangeProposalType.updateRule, payload: { @@ -118,7 +162,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..34b1af0da3 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,207 @@ 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('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')); + + const { proposals } = await buildProposals( + [makeEntry({ changeType: 'updated' })], + getCtx, + fetchRuleIds, + ); + + 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); + }); + }); + + 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..91dd3ea743 100644 --- a/apps/cli/src/infra/commands/playbook/submit/proposalBuilder.ts +++ b/apps/cli/src/infra/commands/playbook/submit/proposalBuilder.ts @@ -12,6 +12,8 @@ import { compareStandardFields, compareCommandFields, compareSkillDefinitionFields, + RuleIdsByContent, + UNRESOLVED_RULE_ID, } from '../../../../application/utils/artifactComparison'; import { normalizePath } from '../../../../application/utils/pathUtils'; import { logWarningConsole } from '../../../utils/consoleLogger'; @@ -142,6 +144,7 @@ function buildUpdatedStandardProposals( entry: PlaybookChangeEntry, artifactId: string | null, deployedContent: string | null, + ruleIdsByContent?: RuleIdsByContent, ): ProposalItem[] { if (!artifactId) return []; @@ -156,6 +159,7 @@ function buildUpdatedStandardProposals( entry.content, deployedContent, entry.filePath, + ruleIdsByContent, ); return fieldChanges.map((change) => ({ ...base, @@ -445,9 +449,66 @@ 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, so one unreachable standard costs one request. + */ +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 +516,7 @@ export async function buildProposals( }> { const proposals: ProposalItem[] = []; const skipped: SkippedEntry[] = []; + const ruleIdCache = new Map(); const updateSources = new Map< string, { @@ -546,15 +608,42 @@ export async function buildProposals( } switch (entry.artifactType) { - case 'standard': - proposals.push( - ...buildUpdatedStandardProposals( - entry, + 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, - deployedContent, - ), + entry.artifactName, + ruleIdCache, + ); + } + 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': proposals.push( ...buildUpdatedCommandProposals(entry, artifactId, deployedContent), 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, 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