diff --git a/CHANGELOG.MD b/CHANGELOG.MD index 7a42545d48..bd594d7cfc 100644 --- a/CHANGELOG.MD +++ b/CHANGELOG.MD @@ -4,10 +4,12 @@ ## Changed -- Markdown is now writable as markdown wherever it is written, and not on standards and commands alone. Editing a file of a skill, and writing a package description from the new-package page, the package edit form or either of the Context drawers, all carried the WYSIWYG editor by itself: what it would actually save could only be read back by saving it, and content it lays out differently from how it was pasted had nowhere to be corrected. Those five editors now carry the WYSIWYG and Raw tabs the standard and command forms already had. They open on WYSIWYG as before, switching between the two keeps whatever is unsaved, and Save reads the pane you are in, so a keystroke made the instant before clicking it is included either way +- Skill files and package descriptions (new-package page, package edit form, Context drawers) now offer the WYSIWYG and Raw tabs that standards and commands already had ## 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/api/src/app/organizations/spaces/commands/commands.controller.spec.ts b/apps/api/src/app/organizations/spaces/commands/commands.controller.spec.ts index a76ff5f495..44e5fae40a 100644 --- a/apps/api/src/app/organizations/spaces/commands/commands.controller.spec.ts +++ b/apps/api/src/app/organizations/spaces/commands/commands.controller.spec.ts @@ -1,5 +1,9 @@ import { commandFactory } from '@packmind/commands/test'; -import { BadRequestException, NotFoundException } from '@nestjs/common'; +import { BadRequestException } from '@nestjs/common'; +import { + CommandNotFoundError, + CommandSlugAlreadyExistsError, +} from '@packmind/commands'; import { PackmindLogger } from '@packmind/logger'; import { AuthenticatedRequest } from '@packmind/node-utils'; import { stubLogger, createMockInstance } from '@packmind/test-utils'; @@ -197,7 +201,7 @@ describe('OrganizationsSpacesRecipesController', () => { }); }); - it('throws NotFoundException for non-existent recipe', async () => { + it('throws CommandNotFoundError for non-existent recipe', async () => { const orgId = createOrganizationId('org-123'); const spaceId = createSpaceId('space-456'); const recipeId = createCommandId('recipe-1'); @@ -219,7 +223,7 @@ describe('OrganizationsSpacesRecipesController', () => { await expect( controller.getCommandById(orgId, spaceId, recipeId, request), - ).rejects.toThrow(NotFoundException); + ).rejects.toThrow(CommandNotFoundError); }); it('propagates errors from service', async () => { @@ -301,7 +305,7 @@ describe('OrganizationsSpacesRecipesController', () => { }); }); - it('throws NotFoundException for empty versions list', async () => { + it('throws CommandNotFoundError for empty versions list', async () => { const orgId = createOrganizationId('org-123'); const spaceId = createSpaceId('space-456'); const recipeId = createCommandId('recipe-1'); @@ -310,7 +314,7 @@ describe('OrganizationsSpacesRecipesController', () => { await expect( controller.getCommandVersionsById(orgId, spaceId, recipeId), - ).rejects.toThrow(NotFoundException); + ).rejects.toThrow(CommandNotFoundError); }); it('propagates errors from service', async () => { @@ -327,6 +331,35 @@ describe('OrganizationsSpacesRecipesController', () => { }); }); + describe('createRecipe', () => { + describe('when the slug already exists in the space', () => { + it('propagates CommandSlugAlreadyExistsError', async () => { + const orgId = createOrganizationId('org-123'); + const spaceId = createSpaceId('space-456'); + const request = { + user: { userId: createUserId('user-1') }, + } as unknown as AuthenticatedRequest; + + commandsService.addCommand.mockRejectedValue( + new CommandSlugAlreadyExistsError('my-command', spaceId), + ); + + await expect( + controller.createCommand( + orgId, + spaceId, + { + name: 'My command', + content: 'content', + slug: 'my-command', + } as Parameters[2], + request, + ), + ).rejects.toThrow(CommandSlugAlreadyExistsError); + }); + }); + }); + describe('updateRecipe', () => { describe('when update is successful', () => { const orgId = createOrganizationId('org-123'); @@ -442,17 +475,13 @@ describe('OrganizationsSpacesRecipesController', () => { name: 'Test User', }, } as unknown as AuthenticatedRequest; - const error = new Error( - `Recipe ${recipeId} does not belong to space ${spaceId}`, - ); + const error = new CommandNotFoundError(recipeId, spaceId); commandsService.updateCommandFromUI.mockRejectedValue(error); await expect( controller.updateCommand(orgId, spaceId, recipeId, updateData, request), - ).rejects.toThrow( - `Recipe ${recipeId} does not belong to space ${spaceId}`, - ); + ).rejects.toThrow(CommandNotFoundError); }); }); @@ -533,17 +562,13 @@ describe('OrganizationsSpacesRecipesController', () => { name: 'Test User', }, } as unknown as AuthenticatedRequest; - const error = new Error( - `Recipe ${recipeId} does not belong to space ${spaceId}`, - ); + const error = new CommandNotFoundError(recipeId, spaceId); commandsService.deleteCommand.mockRejectedValue(error); await expect( controller.deleteCommand(orgId, spaceId, recipeId, request), - ).rejects.toThrow( - `Recipe ${recipeId} does not belong to space ${spaceId}`, - ); + ).rejects.toThrow(CommandNotFoundError); }); }); @@ -689,12 +714,12 @@ describe('OrganizationsSpacesRecipesController', () => { }); describe('when service returns null', () => { - it('throws NotFoundException', async () => { + it('throws CommandNotFoundError', async () => { commandsService.getLatestVersionNumber.mockResolvedValue(null); await expect( controller.getCommandLatestVersion(orgId, spaceId, recipeId, request), - ).rejects.toThrow(NotFoundException); + ).rejects.toThrow(CommandNotFoundError); }); }); }); diff --git a/apps/api/src/app/organizations/spaces/commands/commands.controller.ts b/apps/api/src/app/organizations/spaces/commands/commands.controller.ts index 4fe83e78ea..6aa9e9192e 100644 --- a/apps/api/src/app/organizations/spaces/commands/commands.controller.ts +++ b/apps/api/src/app/organizations/spaces/commands/commands.controller.ts @@ -1,11 +1,9 @@ import { BadRequestException, Body, - ConflictException, Controller, Delete, Get, - NotFoundException, Param, Patch, Post, @@ -18,11 +16,11 @@ import { OrganizationId, Command, CommandId, - CommandSlugAlreadyExistsError, CommandVersion, SpaceId, UserId, } from '@packmind/types'; +import { CommandNotFoundError } from '@packmind/commands'; import { CommandsService } from './commands.service'; import { OrganizationAccessGuard } from '../../guards/organization-access.guard'; @@ -81,25 +79,11 @@ export class OrganizationsSpacesCommandsController { }, ); - try { - return await this.commandsService.getCommandsBySpace( - spaceId, - organizationId, - userId, - ); - } catch (error) { - const errorMessage = - error instanceof Error ? error.message : String(error); - this.logger.error( - 'GET /organizations/:orgId/spaces/:spaceId/recipes - Failed to fetch recipes', - { - organizationId, - spaceId, - error: errorMessage, - }, - ); - throw error; - } + return this.commandsService.getCommandsBySpace( + spaceId, + organizationId, + userId, + ); } /** @@ -124,39 +108,16 @@ export class OrganizationsSpacesCommandsController { }, ); - try { - const recipe = await this.commandsService.getCommandById( - id, - organizationId, - spaceId, - userId, - ); - if (!recipe) { - this.logger.warn( - 'GET /organizations/:orgId/spaces/:spaceId/recipes/:id - Recipe not found', - { - organizationId, - spaceId, - recipeId: id, - }, - ); - throw new NotFoundException(`Recipe with id ${id} not found`); - } - return recipe; - } catch (error) { - const errorMessage = - error instanceof Error ? error.message : String(error); - this.logger.error( - 'GET /organizations/:orgId/spaces/:spaceId/recipes/:id - Failed to fetch recipe', - { - organizationId, - spaceId, - recipeId: id, - error: errorMessage, - }, - ); - throw error; + const recipe = await this.commandsService.getCommandById( + id, + organizationId, + spaceId, + userId, + ); + if (!recipe) { + throw new CommandNotFoundError(id, spaceId); } + return recipe; } /** @@ -186,42 +147,13 @@ export class OrganizationsSpacesCommandsController { }, ); - try { - return await this.commandsService.addCommand( - recipe, - organizationId, - userId, - spaceId, - request.clientSource, - ); - } catch (error) { - if (error instanceof CommandSlugAlreadyExistsError) { - this.logger.warn( - 'POST /organizations/:orgId/spaces/:spaceId/recipes - Slug already exists', - { - organizationId, - spaceId, - slug: error.slug, - userId, - }, - ); - throw new ConflictException(error.message); - } - - const errorMessage = - error instanceof Error ? error.message : String(error); - this.logger.error( - 'POST /organizations/:orgId/spaces/:spaceId/recipes - Failed to create recipe', - { - organizationId, - spaceId, - recipeName: recipe.name, - userId, - error: errorMessage, - }, - ); - throw error; - } + return this.commandsService.addCommand( + recipe, + organizationId, + userId, + spaceId, + request.clientSource, + ); } /** @@ -253,45 +185,28 @@ export class OrganizationsSpacesCommandsController { }, ); - try { - const updatedCommand = await this.commandsService.updateCommandFromUI({ - recipeId: id, - spaceId, + const updatedCommand = await this.commandsService.updateCommandFromUI({ + recipeId: id, + spaceId, + organizationId, + name: updateData.name, + content: updateData.content, + userId, + source: request.clientSource, + }); + + this.logger.info( + 'PATCH /organizations/:orgId/spaces/:spaceId/recipes/:id - Recipe updated successfully', + { organizationId, - name: updateData.name, - content: updateData.content, + spaceId, + recipeId: id, + newVersion: updatedCommand.version, userId, - source: request.clientSource, - }); - - this.logger.info( - 'PATCH /organizations/:orgId/spaces/:spaceId/recipes/:id - Recipe updated successfully', - { - organizationId, - spaceId, - recipeId: id, - newVersion: updatedCommand.version, - userId, - }, - ); - - return updatedCommand; - } catch (error) { - const errorMessage = - error instanceof Error ? error.message : String(error); - this.logger.error( - 'PATCH /organizations/:orgId/spaces/:spaceId/recipes/:id - Failed to update recipe', - { - organizationId, - spaceId, - recipeId: id, - recipeName: updateData.name, - userId, - error: errorMessage, - }, - ); - throw error; - } + }, + ); + + return updatedCommand; } /** @@ -329,38 +244,23 @@ export class OrganizationsSpacesCommandsController { }, ); - try { - await this.commandsService.deleteCommandsBatch( - ids, + await this.commandsService.deleteCommandsBatch( + ids, + spaceId, + userId, + organizationId, + request.clientSource, + ); + + this.logger.info( + 'DELETE /organizations/:orgId/spaces/:spaceId/recipes - Recipes deleted successfully in batch', + { + organizationId, spaceId, + count: ids.length, userId, - organizationId, - request.clientSource, - ); - - this.logger.info( - 'DELETE /organizations/:orgId/spaces/:spaceId/recipes - Recipes deleted successfully in batch', - { - organizationId, - spaceId, - count: ids.length, - userId, - }, - ); - } catch (error) { - const errorMessage = - error instanceof Error ? error.message : String(error); - this.logger.error( - 'DELETE /organizations/:orgId/spaces/:spaceId/recipes - Failed to delete recipes in batch', - { - organizationId, - spaceId, - recipeIds: ids, - error: errorMessage, - }, - ); - throw error; - } + }, + ); } /** @@ -386,39 +286,23 @@ export class OrganizationsSpacesCommandsController { }, ); - try { - await this.commandsService.deleteCommand( - id, - spaceId, + await this.commandsService.deleteCommand( + id, + spaceId, + organizationId, + userId, + request.clientSource, + ); + + this.logger.info( + 'DELETE /organizations/:orgId/spaces/:spaceId/recipes/:id - Recipe deleted successfully', + { organizationId, + spaceId, + recipeId: id, userId, - request.clientSource, - ); - - this.logger.info( - 'DELETE /organizations/:orgId/spaces/:spaceId/recipes/:id - Recipe deleted successfully', - { - organizationId, - spaceId, - recipeId: id, - userId, - }, - ); - } catch (error) { - const errorMessage = - error instanceof Error ? error.message : String(error); - this.logger.error( - 'DELETE /organizations/:orgId/spaces/:spaceId/recipes/:id - Failed to delete recipe', - { - organizationId, - spaceId, - recipeId: id, - userId, - error: errorMessage, - }, - ); - throw error; - } + }, + ); } /** @@ -447,7 +331,7 @@ export class OrganizationsSpacesCommandsController { }); if (version === null) { - throw new NotFoundException(`Recipe ${id} not found`); + throw new CommandNotFoundError(id, spaceId); } return { version }; @@ -472,39 +356,14 @@ export class OrganizationsSpacesCommandsController { }, ); - try { - const versions = await this.commandsService.getCommandVersionsById(id); - if (!versions || versions.length === 0) { - this.logger.warn( - 'GET /organizations/:orgId/spaces/:spaceId/recipes/:id/versions - No versions found', - { - organizationId, - spaceId, - recipeId: id, - }, - ); - throw new NotFoundException( - `No versions found for recipe with id ${id}`, - ); - } - // Superset: add command-named twin `commandId` beside `recipeId`. - return versions.map((version) => ({ - ...version, - commandId: version.recipeId, - })); - } catch (error) { - const errorMessage = - error instanceof Error ? error.message : String(error); - this.logger.error( - 'GET /organizations/:orgId/spaces/:spaceId/recipes/:id/versions - Failed to fetch recipe versions', - { - organizationId, - spaceId, - recipeId: id, - error: errorMessage, - }, - ); - throw error; + const versions = await this.commandsService.getCommandVersionsById(id); + if (!versions || versions.length === 0) { + throw new CommandNotFoundError(id, spaceId); } + // Superset: add command-named twin `commandId` beside `recipeId`. + return versions.map((version) => ({ + ...version, + commandId: version.recipeId, + })); } } 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 diff --git a/packages/commands/src/application/adapter/CommandsAdapter.ts b/packages/commands/src/application/adapter/CommandsAdapter.ts index 392fea7fb0..0a8a883748 100644 --- a/packages/commands/src/application/adapter/CommandsAdapter.ts +++ b/packages/commands/src/application/adapter/CommandsAdapter.ts @@ -35,6 +35,7 @@ import { UpdateCommandFromUIResponse, UserId, } from '@packmind/types'; +import { CommandsAdapterPortsMissingError } from '../../domain/errors'; import { ICommandsDelayedJobs } from '../../domain/jobs/ICommandsDelayedJobs'; import { DeployCommandsJobFactory } from '../../infra/jobs/DeployCommandsJobFactory'; import { CommandsServices } from '../services/CommandsServices'; @@ -101,9 +102,16 @@ export class CommandsAdapter ); if (!this.isReady()) { - throw new Error( - 'RecipesAdapter: Required ports/delayed jobs not provided.', - ); + const missingPorts = Object.entries({ + [IGitPortName]: this.gitPort, + [IDeploymentPortName]: this.deploymentPort, + [IAccountsPortName]: this.accountsPort, + [ISpacesPortName]: this.spacesPort, + commandsDelayedJobs: this.commandsDelayedJobs, + }) + .filter(([, port]) => !port) + .map(([name]) => name); + throw new CommandsAdapterPortsMissingError(missingPorts); } this._captureCommand = new CaptureCommandUseCase( diff --git a/packages/commands/src/application/services/CommandService.spec.ts b/packages/commands/src/application/services/CommandService.spec.ts index 6ae03489e0..76e3f6712b 100644 --- a/packages/commands/src/application/services/CommandService.spec.ts +++ b/packages/commands/src/application/services/CommandService.spec.ts @@ -15,6 +15,7 @@ import { commandFactory } from '../../../test/commandFactory'; import { commandVersionFactory } from '../../../test/commandVersionFactory'; import { ICommandRepository } from '../../domain/repositories/ICommandRepository'; import { ICommandVersionRepository } from '../../domain/repositories/ICommandVersionRepository'; +import { CommandNotFoundError } from '../../domain/errors'; import { CreateCommandData, CommandService, @@ -275,10 +276,10 @@ describe('RecipeService', () => { commandRepository.findById = jest.fn().mockResolvedValue(null); }); - it('throws an error with the correct message', async () => { + it('throws CommandNotFoundError', async () => { await expect( commandService.updateCommand(nonExistentCommandId, updateData), - ).rejects.toThrow(`Recipe with id ${nonExistentCommandId} not found`); + ).rejects.toThrow(CommandNotFoundError); }); }); }); @@ -324,10 +325,10 @@ describe('RecipeService', () => { commandRepository.findById = jest.fn().mockResolvedValue(null); }); - it('throws an error with the correct message', async () => { + it('throws CommandNotFoundError', async () => { await expect( commandService.deleteCommand(nonExistentCommandId, userId), - ).rejects.toThrow(`Recipe with id ${nonExistentCommandId} not found`); + ).rejects.toThrow(CommandNotFoundError); }); it('does not call deleteById', async () => { @@ -378,13 +379,13 @@ describe('RecipeService', () => { commandRepository.findById = jest.fn().mockResolvedValue(null); }); - it('throws an error with the correct message', async () => { + it('throws CommandNotFoundError', async () => { await expect( commandService.markCommandAsMoved( nonExistentCommandId, destinationSpaceId, ), - ).rejects.toThrow(`Recipe with id ${nonExistentCommandId} not found`); + ).rejects.toThrow(CommandNotFoundError); }); }); }); @@ -552,14 +553,14 @@ describe('RecipeService', () => { commandRepository.findById = jest.fn().mockResolvedValue(null); }); - it('throws an error with the correct message', async () => { + it('throws CommandNotFoundError', async () => { await expect( commandService.duplicateCommandToSpace( nonExistentCommandId, destinationSpaceId, newUserId, ), - ).rejects.toThrow(`Recipe with id ${nonExistentCommandId} not found`); + ).rejects.toThrow(CommandNotFoundError); }); }); diff --git a/packages/commands/src/application/services/CommandService.ts b/packages/commands/src/application/services/CommandService.ts index dab467559c..0f919b40f6 100644 --- a/packages/commands/src/application/services/CommandService.ts +++ b/packages/commands/src/application/services/CommandService.ts @@ -4,6 +4,7 @@ import { ICommandVersionRepository } from '../../domain/repositories/ICommandVer import { ICommandRepository } from '../../domain/repositories/ICommandRepository'; import { CommandRepository } from '../../infra/repositories/CommandRepository'; import { PackmindLogger } from '@packmind/logger'; +import { CommandNotFoundError } from '../../domain/errors'; import { createCommandId, createCommandVersionId, @@ -55,29 +56,21 @@ export class CommandService { userId: commandData.userId, }); - try { - const recipeId = createCommandId(uuidv4()); + const recipeId = createCommandId(uuidv4()); - const recipe: Command = { - id: recipeId, - ...commandData, - movedTo: null, - }; + const recipe: Command = { + id: recipeId, + ...commandData, + movedTo: null, + }; - const savedCommand = await this.commandRepository.add(recipe); - this.logger.info('Recipe added to repository successfully', { - recipeId, - name: commandData.name, - }); + const savedCommand = await this.commandRepository.add(recipe); + this.logger.info('Recipe added to repository successfully', { + recipeId, + name: commandData.name, + }); - return savedCommand; - } catch (error) { - this.logger.error('Failed to add recipe', { - name: commandData.name, - error: error instanceof Error ? error.message : String(error), - }); - throw error; - } + return savedCommand; } async listCommandsBySpace( @@ -89,20 +82,12 @@ export class CommandService { includeDeleted: opts?.includeDeleted ?? false, }); - try { - const recipes = await this.commandRepository.findBySpaceId(spaceId, opts); - this.logger.info('Recipes retrieved by space successfully', { - spaceId, - count: recipes.length, - }); - return recipes; - } catch (error) { - this.logger.error('Failed to list recipes by space', { - spaceId, - error: error instanceof Error ? error.message : String(error), - }); - throw error; - } + const recipes = await this.commandRepository.findBySpaceId(spaceId, opts); + this.logger.info('Recipes retrieved by space successfully', { + spaceId, + count: recipes.length, + }); + return recipes; } async countBySpaceIds(spaceIds: SpaceId[]): Promise> { @@ -112,24 +97,16 @@ export class CommandService { async getCommandById(id: CommandId): Promise { this.logger.info('Getting recipe by ID', { id }); - try { - const recipe = await this.commandRepository.findById(id); - if (recipe) { - this.logger.info('Recipe found successfully', { - id, - name: recipe.name, - }); - } else { - this.logger.warn('Recipe not found', { id }); - } - return recipe; - } catch (error) { - this.logger.error('Failed to get recipe by ID', { + const recipe = await this.commandRepository.findById(id); + if (recipe) { + this.logger.info('Recipe found successfully', { id, - error: error instanceof Error ? error.message : String(error), + name: recipe.name, }); - throw error; + } else { + this.logger.warn('Recipe not found', { id }); } + return recipe; } async findCommandBySlug( @@ -142,33 +119,24 @@ export class CommandService { organizationId, }); - try { - const recipe = await this.commandRepository.findBySlug( + const recipe = await this.commandRepository.findBySlug( + slug, + organizationId, + opts, + ); + if (recipe) { + this.logger.info('Recipe found by slug and organization successfully', { slug, organizationId, - opts, - ); - if (recipe) { - this.logger.info('Recipe found by slug and organization successfully', { - slug, - organizationId, - recipeId: recipe.id, - }); - } else { - this.logger.warn('Recipe not found by slug and organization', { - slug, - organizationId, - }); - } - return recipe; - } catch (error) { - this.logger.error('Failed to find recipe by slug and organization', { + recipeId: recipe.id, + }); + } else { + this.logger.warn('Recipe not found by slug and organization', { slug, organizationId, - error: error instanceof Error ? error.message : String(error), }); - throw error; } + return recipe; } async updateCommand( @@ -181,58 +149,40 @@ export class CommandService { userId: commandData.userId, }); - try { - const existingCommand = await this.commandRepository.findById(recipeId); - if (!existingCommand) { - this.logger.error('Recipe not found for update', { recipeId }); - throw new Error(`Recipe with id ${recipeId} not found`); - } - - const updatedCommand: Command = { - id: recipeId, - ...commandData, - spaceId: existingCommand.spaceId, - movedTo: existingCommand.movedTo, - }; - - const savedCommand = await this.commandRepository.add(updatedCommand); - this.logger.info('Recipe updated in repository successfully', { - recipeId, - version: commandData.version, - }); - - return savedCommand; - } catch (error) { - this.logger.error('Failed to update recipe', { - recipeId, - error: error instanceof Error ? error.message : String(error), - }); - throw error; + const existingCommand = await this.commandRepository.findById(recipeId); + if (!existingCommand) { + throw new CommandNotFoundError(recipeId); } + + const updatedCommand: Command = { + id: recipeId, + ...commandData, + spaceId: existingCommand.spaceId, + movedTo: existingCommand.movedTo, + }; + + const savedCommand = await this.commandRepository.add(updatedCommand); + this.logger.info('Recipe updated in repository successfully', { + recipeId, + version: commandData.version, + }); + + return savedCommand; } async deleteCommand(recipeId: CommandId, userId: UserId): Promise { this.logger.info('Deleting recipe and all its versions', { recipeId }); - try { - const recipe = await this.commandRepository.findById(recipeId); - if (!recipe) { - this.logger.error('Recipe not found for deletion', { recipeId }); - throw new Error(`Recipe with id ${recipeId} not found`); - } + const recipe = await this.commandRepository.findById(recipeId); + if (!recipe) { + throw new CommandNotFoundError(recipeId); + } - await this.commandRepository.deleteById(recipeId, userId); + await this.commandRepository.deleteById(recipeId, userId); - this.logger.info('Recipe deleted successfully', { - recipeId, - }); - } catch (error) { - this.logger.error('Failed to delete recipe', { - recipeId, - error: error instanceof Error ? error.message : String(error), - }); - throw error; - } + this.logger.info('Recipe deleted successfully', { + recipeId, + }); } async hardDeleteCommand(recipeId: CommandId): Promise { @@ -255,59 +205,50 @@ export class CommandService { destinationSpaceId, }); - try { - const original = await this.commandRepository.findById(recipeId); - if (!original) { - throw new Error(`Recipe with id ${recipeId} not found`); - } - - const newCommandId = createCommandId(uuidv4()); - const newCommand: Command = { - id: newCommandId, - name: original.name, - slug: original.slug, - content: original.content, - version: original.version, - gitCommit: original.gitCommit, - userId: newUserId, - spaceId: destinationSpaceId, - movedTo: null, - }; - const savedCommand = await this.commandRepository.add(newCommand); - - const versions = - await this.commandVersionRepository.findByCommandId(recipeId); - - if (versions.length > 0) { - const newVersions = versions.map((version) => ({ - id: createCommandVersionId(uuidv4()), - recipeId: newCommandId, - name: version.name, - slug: version.slug, - content: version.content, - version: version.version, - gitCommit: version.gitCommit, - userId: version.userId, - })); - await this.commandVersionRepository.addMany(newVersions); - } - - this.logger.info('Recipe duplicated to space successfully', { - originalRecipeId: recipeId, - newCommandId, - destinationSpaceId, - versionsCount: versions.length, - }); + const original = await this.commandRepository.findById(recipeId); + if (!original) { + throw new CommandNotFoundError(recipeId); + } - return savedCommand; - } catch (error) { - this.logger.error('Failed to duplicate recipe to space', { - recipeId, - destinationSpaceId, - error: error instanceof Error ? error.message : String(error), - }); - throw error; + const newCommandId = createCommandId(uuidv4()); + const newCommand: Command = { + id: newCommandId, + name: original.name, + slug: original.slug, + content: original.content, + version: original.version, + gitCommit: original.gitCommit, + userId: newUserId, + spaceId: destinationSpaceId, + movedTo: null, + }; + const savedCommand = await this.commandRepository.add(newCommand); + + const versions = + await this.commandVersionRepository.findByCommandId(recipeId); + + if (versions.length > 0) { + const newVersions = versions.map((version) => ({ + id: createCommandVersionId(uuidv4()), + recipeId: newCommandId, + name: version.name, + slug: version.slug, + content: version.content, + version: version.version, + gitCommit: version.gitCommit, + userId: version.userId, + })); + await this.commandVersionRepository.addMany(newVersions); } + + this.logger.info('Recipe duplicated to space successfully', { + originalRecipeId: recipeId, + newCommandId, + destinationSpaceId, + versionsCount: versions.length, + }); + + return savedCommand; } async markCommandAsMoved( @@ -319,24 +260,16 @@ export class CommandService { destinationSpaceId, }); - try { - const recipe = await this.commandRepository.findById(recipeId); - if (!recipe) { - throw new Error(`Recipe with id ${recipeId} not found`); - } + const recipe = await this.commandRepository.findById(recipeId); + if (!recipe) { + throw new CommandNotFoundError(recipeId); + } - await this.commandRepository.markAsMoved(recipeId, destinationSpaceId); + await this.commandRepository.markAsMoved(recipeId, destinationSpaceId); - this.logger.info('Recipe marked as moved successfully', { - recipeId, - destinationSpaceId, - }); - } catch (error) { - this.logger.error('Failed to mark recipe as moved', { - recipeId, - error: error instanceof Error ? error.message : String(error), - }); - throw error; - } + this.logger.info('Recipe marked as moved successfully', { + recipeId, + destinationSpaceId, + }); } } diff --git a/packages/commands/src/application/services/CommandVersionService.ts b/packages/commands/src/application/services/CommandVersionService.ts index 0011a1e57c..38d84bea94 100644 --- a/packages/commands/src/application/services/CommandVersionService.ts +++ b/packages/commands/src/application/services/CommandVersionService.ts @@ -28,54 +28,37 @@ export class CommandVersionService { version: commandVersionData.version, }); - try { - const versionId = createCommandVersionId(uuidv4()); - this.logger.debug('Generated recipe version ID', { versionId }); - - const newCommandVersion: CommandVersion = { - id: versionId, - ...commandVersionData, - }; - - this.logger.debug('Adding recipe version to repository'); - const savedVersion = - await this.commandVersionRepository.add(newCommandVersion); - - this.logger.info('Recipe version added to repository successfully', { - versionId, - recipeId: commandVersionData.recipeId, - version: commandVersionData.version, - }); + const versionId = createCommandVersionId(uuidv4()); + this.logger.debug('Generated recipe version ID', { versionId }); - return savedVersion; - } catch (error) { - this.logger.error('Failed to add recipe version', { - recipeId: commandVersionData.recipeId, - version: commandVersionData.version, - error: error instanceof Error ? error.message : String(error), - }); - throw error; - } + const newCommandVersion: CommandVersion = { + id: versionId, + ...commandVersionData, + }; + + this.logger.debug('Adding recipe version to repository'); + const savedVersion = + await this.commandVersionRepository.add(newCommandVersion); + + this.logger.info('Recipe version added to repository successfully', { + versionId, + recipeId: commandVersionData.recipeId, + version: commandVersionData.version, + }); + + return savedVersion; } async listCommandVersions(recipeId: CommandId): Promise { this.logger.info('Listing recipe versions', { recipeId }); - try { - const versions = - await this.commandVersionRepository.findByCommandId(recipeId); - this.logger.info('Recipe versions retrieved successfully', { - recipeId, - count: versions.length, - }); - return versions; - } catch (error) { - this.logger.error('Failed to list recipe versions', { - recipeId, - error: error instanceof Error ? error.message : String(error), - }); - throw error; - } + const versions = + await this.commandVersionRepository.findByCommandId(recipeId); + this.logger.info('Recipe versions retrieved successfully', { + recipeId, + count: versions.length, + }); + return versions; } async getLatestCommandVersions( @@ -85,23 +68,15 @@ export class CommandVersionService { count: recipeIds.length, }); - try { - const versions = - await this.commandVersionRepository.findLatestByCommandIds(recipeIds); + const versions = + await this.commandVersionRepository.findLatestByCommandIds(recipeIds); - this.logger.info('Latest recipe versions retrieved successfully', { - requestedCount: recipeIds.length, - foundCount: versions.length, - }); + this.logger.info('Latest recipe versions retrieved successfully', { + requestedCount: recipeIds.length, + foundCount: versions.length, + }); - return versions; - } catch (error) { - this.logger.error('Failed to get latest recipe versions', { - count: recipeIds.length, - error: error instanceof Error ? error.message : String(error), - }); - throw error; - } + return versions; } async getCommandVersionsByIds( @@ -111,23 +86,15 @@ export class CommandVersionService { count: commandVersionIds.length, }); - try { - const versions = - await this.commandVersionRepository.findByIds(commandVersionIds); + const versions = + await this.commandVersionRepository.findByIds(commandVersionIds); - this.logger.info('Recipe versions retrieved by IDs successfully', { - requestedCount: commandVersionIds.length, - foundCount: versions.length, - }); + this.logger.info('Recipe versions retrieved by IDs successfully', { + requestedCount: commandVersionIds.length, + foundCount: versions.length, + }); - return versions; - } catch (error) { - this.logger.error('Failed to get recipe versions by IDs', { - count: commandVersionIds.length, - error: error instanceof Error ? error.message : String(error), - }); - throw error; - } + return versions; } async getCommandVersion( @@ -137,33 +104,24 @@ export class CommandVersionService { ): Promise { this.logger.info('Getting recipe version', { recipeId, version }); - try { - const recipeVersion = - await this.commandVersionRepository.findByCommandIdAndVersion( - recipeId, - version, - allowedSpaceIds, - ); - - if (recipeVersion) { - this.logger.info('Recipe version found successfully', { - recipeId, - version, - versionId: recipeVersion.id, - }); - } else { - this.logger.warn('Recipe version not found', { recipeId, version }); - } - - return recipeVersion; - } catch (error) { - this.logger.error('Failed to get recipe version', { + const recipeVersion = + await this.commandVersionRepository.findByCommandIdAndVersion( + recipeId, + version, + allowedSpaceIds, + ); + + if (recipeVersion) { + this.logger.info('Recipe version found successfully', { recipeId, version, - error: error instanceof Error ? error.message : String(error), + versionId: recipeVersion.id, }); - throw error; + } else { + this.logger.warn('Recipe version not found', { recipeId, version }); } + + return recipeVersion; } async getCommandVersionById( @@ -171,27 +129,19 @@ export class CommandVersionService { ): Promise { this.logger.info('Getting recipe version by ID', { versionId: id }); - try { - const recipeVersion = await this.commandVersionRepository.findById(id); - - if (recipeVersion) { - this.logger.info('Recipe version found by ID successfully', { - versionId: id, - recipeId: recipeVersion.recipeId, - version: recipeVersion.version, - }); - } else { - this.logger.warn('Recipe version not found by ID', { versionId: id }); - } - - return recipeVersion; - } catch (error) { - this.logger.error('Failed to get recipe version by ID', { + const recipeVersion = await this.commandVersionRepository.findById(id); + + if (recipeVersion) { + this.logger.info('Recipe version found by ID successfully', { versionId: id, - error: error instanceof Error ? error.message : String(error), + recipeId: recipeVersion.recipeId, + version: recipeVersion.version, }); - throw error; + } else { + this.logger.warn('Recipe version not found by ID', { versionId: id }); } + + return recipeVersion; } async deleteCommandVersionsForCommand( @@ -203,41 +153,32 @@ export class CommandVersionService { deletedBy, }); - try { - const versions = - await this.commandVersionRepository.findByCommandId(recipeId); + const versions = + await this.commandVersionRepository.findByCommandId(recipeId); - if (versions.length === 0) { - this.logger.info('No recipe versions found to delete', { recipeId }); - return; - } - - this.logger.debug('Deleting recipe versions', { - recipeId, - versionCount: versions.length, - }); + if (versions.length === 0) { + this.logger.info('No recipe versions found to delete', { recipeId }); + return; + } - for (const version of versions) { - await this.commandVersionRepository.deleteById(version.id, deletedBy); - this.logger.debug('Recipe version deleted', { - recipeId, - versionId: version.id, - version: version.version, - }); - } + this.logger.debug('Deleting recipe versions', { + recipeId, + versionCount: versions.length, + }); - this.logger.info('All recipe versions deleted successfully', { - recipeId, - deletedCount: versions.length, - deletedBy, - }); - } catch (error) { - this.logger.error('Failed to delete recipe versions for recipe', { + for (const version of versions) { + await this.commandVersionRepository.deleteById(version.id, deletedBy); + this.logger.debug('Recipe version deleted', { recipeId, - deletedBy, - error: error instanceof Error ? error.message : String(error), + versionId: version.id, + version: version.version, }); - throw error; } + + this.logger.info('All recipe versions deleted successfully', { + recipeId, + deletedCount: versions.length, + deletedBy, + }); } } diff --git a/packages/commands/src/application/useCases/captureCommand/CaptureCommandUseCase.spec.ts b/packages/commands/src/application/useCases/captureCommand/CaptureCommandUseCase.spec.ts index 9653d705aa..7e2a3c667d 100644 --- a/packages/commands/src/application/useCases/captureCommand/CaptureCommandUseCase.spec.ts +++ b/packages/commands/src/application/useCases/captureCommand/CaptureCommandUseCase.spec.ts @@ -16,12 +16,12 @@ import { Organization, OrganizationId, Command, - CommandSlugAlreadyExistsError, Space, SpaceId, User, UserId, } from '@packmind/types'; +import { CommandSlugAlreadyExistsError } from '../../../domain/errors'; import { spaceFactory } from '@packmind/spaces/test'; import slug from 'slug'; import { v4 as uuidv4 } from 'uuid'; diff --git a/packages/commands/src/application/useCases/captureCommand/CaptureCommandUseCase.ts b/packages/commands/src/application/useCases/captureCommand/CaptureCommandUseCase.ts index 1eb3a66f5e..6c2bbfe859 100644 --- a/packages/commands/src/application/useCases/captureCommand/CaptureCommandUseCase.ts +++ b/packages/commands/src/application/useCases/captureCommand/CaptureCommandUseCase.ts @@ -15,9 +15,12 @@ import { IAccountsPort, ICaptureCommandUseCase, ISpacesPort, - CommandSlugAlreadyExistsError, CommandStep, } from '@packmind/types'; +import { + CommandSlugAlreadyExistsError, + CommandSpaceNotAccessibleError, +} from '../../../domain/errors'; import slug from 'slug'; import { CommandService } from '../../services/CommandService'; import { CommandVersionService } from '../../services/CommandVersionService'; @@ -62,23 +65,11 @@ export class CaptureCommandUseCase const userId = createUserId(userIdString); const spaceId = createSpaceId(spaceIdString); - // Verify the space belongs to the organization const space = await this.spacesPort.getSpaceById(spaceId); - if (!space) { - this.logger.warn('Space not found', { spaceId }); - throw new Error(`Space with id ${spaceId} not found`); + if (!space || space.organizationId !== organizationId) { + throw new CommandSpaceNotAccessibleError(spaceId, organizationId); } - if (space.organizationId !== organizationId) { - this.logger.warn('Space does not belong to organization', { - spaceId, - spaceOrganizationId: space.organizationId, - requestOrganizationId: organizationId, - }); - throw new Error( - `Space ${spaceId} does not belong to organization ${organizationId}`, - ); - } this.logger.info('Starting captureRecipe process', { name, organizationId, @@ -86,94 +77,83 @@ export class CaptureCommandUseCase spaceId, }); - try { - const existingCommands = - await this.commandService.listCommandsBySpace(spaceId); - const existingSlugs = new Set(existingCommands.map((r) => r.slug)); + const existingCommands = + await this.commandService.listCommandsBySpace(spaceId); + const existingSlugs = new Set(existingCommands.map((r) => r.slug)); - const commandSlug = this.resolveSlug( - command.slug, - name, - existingSlugs, - spaceId, - ); - this.logger.info('Resolved slug', { slug: commandSlug }); - - const content = - providedSummary !== undefined - ? this.assembleCommandContent( - providedSummary, - whenToUse || [], - contextValidationCheckpoints || [], - steps || [], - ) - : legacyContent || ''; - - const initialVersion = 1; - const recipe = await this.commandService.addCommand({ - name, - content, - slug: commandSlug, - version: initialVersion, - gitCommit: undefined, - userId, - spaceId, - }); - this.logger.info('Recipe entity created successfully', { - recipeId: recipe.id, - name, - organizationId, - userId, - spaceId, - }); + const commandSlug = this.resolveSlug( + command.slug, + name, + existingSlugs, + spaceId, + ); + this.logger.info('Resolved slug', { slug: commandSlug }); + + const content = + providedSummary !== undefined + ? this.assembleCommandContent( + providedSummary, + whenToUse || [], + contextValidationCheckpoints || [], + steps || [], + ) + : legacyContent || ''; + + const initialVersion = 1; + const recipe = await this.commandService.addCommand({ + name, + content, + slug: commandSlug, + version: initialVersion, + gitCommit: undefined, + userId, + spaceId, + }); + this.logger.info('Recipe entity created successfully', { + recipeId: recipe.id, + name, + organizationId, + userId, + spaceId, + }); - const recipeVersion = await this.commandVersionService.addCommandVersion({ - recipeId: recipe.id, - name, - slug: commandSlug, - content, - version: initialVersion, - gitCommit: undefined, - userId, - }); - this.logger.info('Initial recipe version created successfully', { - versionId: recipeVersion.id, - recipeId: recipe.id, - version: initialVersion, - }); + const recipeVersion = await this.commandVersionService.addCommandVersion({ + recipeId: recipe.id, + name, + slug: commandSlug, + content, + version: initialVersion, + gitCommit: undefined, + userId, + }); + this.logger.info('Initial recipe version created successfully', { + versionId: recipeVersion.id, + recipeId: recipe.id, + version: initialVersion, + }); - this.logger.info('CaptureRecipe process completed successfully', { - recipeId: recipe.id, - versionId: recipeVersion.id, - name, - organizationId, - userId, - spaceId, - }); + this.logger.info('CaptureRecipe process completed successfully', { + recipeId: recipe.id, + versionId: recipeVersion.id, + name, + organizationId, + userId, + spaceId, + }); - this.eventEmitterService.emit( - new CommandCreatedEvent({ - id: createCommandId(recipe.id), - spaceId, - organizationId, - userId, - source, - originSkill, - directUpdate: command.directUpdate, - }), - ); - - return recipe; - } catch (error) { - this.logger.error('Failed to capture recipe', { - name, + this.eventEmitterService.emit( + new CommandCreatedEvent({ + id: createCommandId(recipe.id), + spaceId, organizationId, userId, - spaceId, - error: error instanceof Error ? error.message : String(error), - }); - throw error; - } + source, + originSkill, + directUpdate: command.directUpdate, + }), + ); + + return recipe; } private resolveSlug( diff --git a/packages/commands/src/application/useCases/captureCommandWithPackages/CaptureCommandWithPackagesUseCase.ts b/packages/commands/src/application/useCases/captureCommandWithPackages/CaptureCommandWithPackagesUseCase.ts index fc668e697b..b51ce6a84d 100644 --- a/packages/commands/src/application/useCases/captureCommandWithPackages/CaptureCommandWithPackagesUseCase.ts +++ b/packages/commands/src/application/useCases/captureCommandWithPackages/CaptureCommandWithPackagesUseCase.ts @@ -11,6 +11,7 @@ import { createOrganizationId, createSpaceId, } from '@packmind/types'; +import { CommandSpaceNotAccessibleError } from '../../../domain/errors'; import { CaptureCommandUseCase } from '../captureCommand/CaptureCommandUseCase'; const origin = 'CaptureRecipeWithPackagesUseCase'; @@ -56,14 +57,8 @@ export class CaptureCommandWithPackagesUseCase }); const space = await this.spacesPort.getSpaceById(spaceId); - if (!space) { - throw new Error(`Space with id ${spaceId} not found`); - } - - if (space.organizationId !== organizationId) { - throw new Error( - `Space ${spaceId} does not belong to organization ${organizationId}`, - ); + if (!space || space.organizationId !== organizationId) { + throw new CommandSpaceNotAccessibleError(spaceId, organizationId); } this.logger.info('Capturing recipe', { name }); diff --git a/packages/commands/src/application/useCases/deleteCommand/DeleteCommandUseCase.spec.ts b/packages/commands/src/application/useCases/deleteCommand/DeleteCommandUseCase.spec.ts index b46ce13d3c..5ba1cd4df2 100644 --- a/packages/commands/src/application/useCases/deleteCommand/DeleteCommandUseCase.spec.ts +++ b/packages/commands/src/application/useCases/deleteCommand/DeleteCommandUseCase.spec.ts @@ -32,6 +32,10 @@ import { commandFactory } from '../../../../test/commandFactory'; import { CommandService } from '../../services/CommandService'; import { CommandVersionService } from '../../services/CommandVersionService'; import { DeleteCommandUseCase } from './DeleteCommandUseCase'; +import { + CommandNotFoundError, + CommandSpaceNotAccessibleError, +} from '../../../domain/errors'; describe('DeleteRecipeUseCase', () => { let deleteCommandUseCase: DeleteCommandUseCase; @@ -235,10 +239,51 @@ describe('DeleteRecipeUseCase', () => { commandService.getCommandById.mockResolvedValue(null); }); - it('throws an error with the correct message', async () => { + it('throws CommandNotFoundError', async () => { await expect( deleteCommandUseCase.execute(nonExistentCommand), - ).rejects.toThrow(`Recipe ${nonExistentCommandId} not found`); + ).rejects.toBeInstanceOf(CommandNotFoundError); + }); + }); + + describe('when recipe belongs to another space', () => { + beforeEach(() => { + commandService.getCommandById.mockResolvedValue( + commandFactory({ + id: recipeId, + userId, + spaceId: createSpaceId(uuidv4()), + }), + ); + }); + + it('throws CommandNotFoundError', async () => { + await expect( + deleteCommandUseCase.execute(command), + ).rejects.toBeInstanceOf(CommandNotFoundError); + }); + + it('does not delete the recipe', async () => { + await deleteCommandUseCase.execute(command).catch(() => undefined); + + expect(commandService.deleteCommand).not.toHaveBeenCalled(); + }); + }); + + describe('when space belongs to another organization', () => { + beforeEach(() => { + spacesPort.getSpaceById.mockResolvedValue( + spaceFactory({ + id: spaceId, + organizationId: createOrganizationId(uuidv4()), + }), + ); + }); + + it('throws CommandSpaceNotAccessibleError', async () => { + await expect( + deleteCommandUseCase.execute(command), + ).rejects.toBeInstanceOf(CommandSpaceNotAccessibleError); }); }); diff --git a/packages/commands/src/application/useCases/deleteCommand/DeleteCommandUseCase.ts b/packages/commands/src/application/useCases/deleteCommand/DeleteCommandUseCase.ts index a523ab14e9..d13a98b675 100644 --- a/packages/commands/src/application/useCases/deleteCommand/DeleteCommandUseCase.ts +++ b/packages/commands/src/application/useCases/deleteCommand/DeleteCommandUseCase.ts @@ -1,6 +1,10 @@ import { CommandService } from '../../services/CommandService'; import { CommandVersionService } from '../../services/CommandVersionService'; import { PackmindLogger } from '@packmind/logger'; +import { + CommandNotFoundError, + CommandSpaceNotAccessibleError, +} from '../../../domain/errors'; import { AbstractSpaceMemberUseCase, SpaceMemberContext, @@ -56,81 +60,43 @@ export class DeleteCommandUseCase organizationId, }); - try { - // Verify the space belongs to the organization - const space = await this.spacesPort.getSpaceById(spaceId); - if (!space) { - this.logger.error('Space not found', { spaceId }); - throw new Error(`Space with id ${spaceId} not found`); - } - - if (space.organizationId !== organizationId) { - this.logger.error('Space does not belong to organization', { - spaceId, - spaceOrganizationId: space.organizationId, - requestOrganizationId: organizationId, - }); - throw new Error( - `Space ${spaceId} does not belong to organization ${organizationId}`, - ); - } - - this.logger.info('Fetching recipe to validate space ownership', { - recipeId, - }); - const existingCommand = - await this.commandService.getCommandById(recipeId); + const space = await this.spacesPort.getSpaceById(spaceId); + if (!space || space.organizationId !== organizationId) { + throw new CommandSpaceNotAccessibleError(spaceId, organizationId); + } - if (!existingCommand) { - this.logger.error('Recipe not found', { recipeId }); - throw new Error(`Recipe ${recipeId} not found`); - } + this.logger.info('Fetching recipe to validate space ownership', { + recipeId, + }); + const existingCommand = await this.commandService.getCommandById(recipeId); - // Security validation: ensure recipe belongs to the specified space - if (existingCommand.spaceId !== spaceId) { - this.logger.error('Recipe does not belong to specified space', { - recipeId, - recipeSpaceId: existingCommand.spaceId, - requestedSpaceId: spaceId, - }); - throw new Error( - `Recipe ${recipeId} does not belong to space ${spaceId}`, - ); - } + if (!existingCommand || existingCommand.spaceId !== spaceId) { + throw new CommandNotFoundError(recipeId, spaceId); + } - this.logger.info('Deleting recipe', { recipeId }); - await this.commandService.deleteCommand(recipeId, userId as UserId); + this.logger.info('Deleting recipe', { recipeId }); + await this.commandService.deleteCommand(recipeId, userId as UserId); - this.logger.info('Deleting all recipe versions for recipe', { recipeId }); - await this.commandVersionService.deleteCommandVersionsForCommand( - recipeId, - userId, - ); + this.logger.info('Deleting all recipe versions for recipe', { recipeId }); + await this.commandVersionService.deleteCommandVersionsForCommand( + recipeId, + userId, + ); - const event = new CommandDeletedEvent({ - id: recipeId, - spaceId, - organizationId: createOrganizationId(organizationId), - userId: createUserId(userId), - source, - }); - this.eventEmitterService.emit(event); - this.logger.info('RecipeDeletedEvent emitted', { - recipeId, - spaceId, - }); + const event = new CommandDeletedEvent({ + id: recipeId, + spaceId, + organizationId: createOrganizationId(organizationId), + userId: createUserId(userId), + source, + }); + this.eventEmitterService.emit(event); + this.logger.info('RecipeDeletedEvent emitted', { + recipeId, + spaceId, + }); - this.logger.info('Recipe deletion completed successfully', { recipeId }); - return {}; - } catch (error) { - this.logger.error('Failed to delete recipe', { - recipeId, - spaceId, - userId, - organizationId, - error: error instanceof Error ? error.message : String(error), - }); - throw error; - } + this.logger.info('Recipe deletion completed successfully', { recipeId }); + return {}; } } diff --git a/packages/commands/src/application/useCases/deleteCommandsBatch/DeleteCommandsBatchUseCase.ts b/packages/commands/src/application/useCases/deleteCommandsBatch/DeleteCommandsBatchUseCase.ts index 7e048cb38a..9bb397c3bc 100644 --- a/packages/commands/src/application/useCases/deleteCommandsBatch/DeleteCommandsBatchUseCase.ts +++ b/packages/commands/src/application/useCases/deleteCommandsBatch/DeleteCommandsBatchUseCase.ts @@ -30,31 +30,20 @@ export class DeleteCommandsBatchUseCase implements IDeleteCommandsBatchUseCase { organizationId, }); - try { - await Promise.all( - recipeIds.map((recipeId) => - this.deleteCommandUseCase.execute({ - // eslint-disable-next-line @typescript-eslint/no-explicit-any - recipeId: recipeId as any, - spaceId, - userId, - organizationId, - }), - ), - ); - this.logger.info('Recipes batch deleted successfully', { - count: recipeIds.length, - }); - return {}; - } catch (error) { - this.logger.error('Failed to delete recipes batch', { - count: recipeIds.length, - spaceId, - userId, - organizationId, - error: error instanceof Error ? error.message : String(error), - }); - throw error; - } + await Promise.all( + recipeIds.map((recipeId) => + this.deleteCommandUseCase.execute({ + // eslint-disable-next-line @typescript-eslint/no-explicit-any + recipeId: recipeId as any, + spaceId, + userId, + organizationId, + }), + ), + ); + this.logger.info('Recipes batch deleted successfully', { + count: recipeIds.length, + }); + return {}; } } diff --git a/packages/commands/src/application/useCases/findCommandBySlug/FindCommandBySlugUseCase.ts b/packages/commands/src/application/useCases/findCommandBySlug/FindCommandBySlugUseCase.ts index add249bdf7..ee36a93335 100644 --- a/packages/commands/src/application/useCases/findCommandBySlug/FindCommandBySlugUseCase.ts +++ b/packages/commands/src/application/useCases/findCommandBySlug/FindCommandBySlugUseCase.ts @@ -25,25 +25,16 @@ export class FindCommandBySlugUseCase { organizationId, }); - try { - const recipe = await this.commandService.findCommandBySlug( - slug, - organizationId, - opts, - ); - this.logger.info('Recipe search by slug and organization completed', { - slug, - organizationId, - found: !!recipe, - }); - return recipe; - } catch (error) { - this.logger.error('Failed to find recipe by slug and organization', { - slug, - organizationId, - error: error instanceof Error ? error.message : String(error), - }); - throw error; - } + const recipe = await this.commandService.findCommandBySlug( + slug, + organizationId, + opts, + ); + this.logger.info('Recipe search by slug and organization completed', { + slug, + organizationId, + found: !!recipe, + }); + return recipe; } } diff --git a/packages/commands/src/application/useCases/getCommandById/GetCommandByIdUseCase.spec.ts b/packages/commands/src/application/useCases/getCommandById/GetCommandByIdUseCase.spec.ts index 57cf096518..f1e7508eaf 100644 --- a/packages/commands/src/application/useCases/getCommandById/GetCommandByIdUseCase.spec.ts +++ b/packages/commands/src/application/useCases/getCommandById/GetCommandByIdUseCase.spec.ts @@ -1,4 +1,5 @@ import { GetCommandByIdUseCase } from './GetCommandByIdUseCase'; +import { CommandSpaceNotAccessibleError } from '../../../domain/errors'; import { CommandService } from '../../services/CommandService'; import { commandFactory } from '../../../../test/commandFactory'; import { @@ -241,7 +242,7 @@ describe('GetRecipeByIdUseCase', () => { }); describe('when space is not found', () => { - it('throws error', async () => { + it('throws CommandSpaceNotAccessibleError', async () => { const organizationId = createOrganizationId('org-1'); const spaceId = createSpaceId('space-1'); const userId = createUserId('user-1'); @@ -278,12 +279,12 @@ describe('GetRecipeByIdUseCase', () => { spaceId, recipeId, }), - ).rejects.toThrow(`Space with id ${spaceId} not found`); + ).rejects.toBeInstanceOf(CommandSpaceNotAccessibleError); }); }); describe('when space does not belong to organization', () => { - it('throws error', async () => { + it('throws CommandSpaceNotAccessibleError', async () => { const organizationId = createOrganizationId('org-1'); const differentOrgId = createOrganizationId('org-2'); const spaceId = createSpaceId('space-1'); @@ -325,14 +326,12 @@ describe('GetRecipeByIdUseCase', () => { spaceId, recipeId, }), - ).rejects.toThrow( - `Space ${spaceId} does not belong to organization ${organizationId}`, - ); + ).rejects.toBeInstanceOf(CommandSpaceNotAccessibleError); }); }); describe('when recipe does not belong to organization', () => { - it('throws error', async () => { + it('answers as if the recipe did not exist', async () => { const organizationId = createOrganizationId('org-1'); const spaceId = createSpaceId('space-1'); const userId = createUserId('user-1'); @@ -376,9 +375,7 @@ describe('GetRecipeByIdUseCase', () => { spaceId, recipeId, }), - ).rejects.toThrow( - `Recipe ${recipeId} does not belong to space ${spaceId}`, - ); + ).resolves.toEqual({ recipe: null }); }); }); @@ -430,7 +427,7 @@ describe('GetRecipeByIdUseCase', () => { }); describe('when recipe does not belong to space', () => { - it('throws error for recipe with different spaceId', async () => { + it('answers as if the recipe did not exist', async () => { const organizationId = createOrganizationId('org-1'); const spaceId = createSpaceId('space-1'); const differentSpaceId = createSpaceId('space-2'); @@ -474,9 +471,7 @@ describe('GetRecipeByIdUseCase', () => { spaceId, recipeId, }), - ).rejects.toThrow( - `Recipe ${recipeId} does not belong to space ${spaceId}`, - ); + ).resolves.toEqual({ recipe: null }); }); }); diff --git a/packages/commands/src/application/useCases/getCommandById/GetCommandByIdUseCase.ts b/packages/commands/src/application/useCases/getCommandById/GetCommandByIdUseCase.ts index 1c390e8225..bb01ea4025 100644 --- a/packages/commands/src/application/useCases/getCommandById/GetCommandByIdUseCase.ts +++ b/packages/commands/src/application/useCases/getCommandById/GetCommandByIdUseCase.ts @@ -12,6 +12,7 @@ import { Command, CommandId, } from '@packmind/types'; +import { CommandSpaceNotAccessibleError } from '../../../domain/errors'; import { CommandService } from '../../services/CommandService'; const origin = 'GetRecipeByIdUseCase'; @@ -41,54 +42,25 @@ export class GetCommandByIdUseCase spaceId: command.spaceId, }); - try { - // Verify the space belongs to the organization - const space = await this.spacesPort.getSpaceById(command.spaceId); - if (!space) { - this.logger.warn('Space not found', { spaceId: command.spaceId }); - throw new Error(`Space with id ${command.spaceId} not found`); - } - - if (space.organizationId !== command.organizationId) { - this.logger.warn('Space does not belong to organization', { - spaceId: command.spaceId, - spaceOrganizationId: space.organizationId, - requestOrganizationId: command.organizationId, - }); - throw new Error( - `Space ${command.spaceId} does not belong to organization ${command.organizationId}`, - ); - } - - const recipe = await this.commandService.getCommandById(command.recipeId); - - if (!recipe) { - this.logger.info('Recipe not found', { id: command.recipeId }); - return { recipe: null }; - } + const space = await this.spacesPort.getSpaceById(command.spaceId); + if (!space || space.organizationId !== command.organizationId) { + throw new CommandSpaceNotAccessibleError( + command.spaceId, + command.organizationId, + ); + } - if (recipe.spaceId !== command.spaceId) { - this.logger.warn('Recipe does not belong to space', { - recipeId: command.recipeId, - recipeSpaceId: recipe.spaceId, - requestSpaceId: command.spaceId, - }); - throw new Error( - `Recipe ${command.recipeId} does not belong to space ${command.spaceId}`, - ); - } + const recipe = await this.commandService.getCommandById(command.recipeId); - this.logger.info('Recipe retrieved successfully', { - id: command.recipeId, - }); - return { recipe }; - } catch (error) { - this.logger.error('Failed to get recipe by ID', { - id: command.recipeId, - error: error instanceof Error ? error.message : String(error), - }); - throw error; + if (!recipe || recipe.spaceId !== command.spaceId) { + this.logger.info('Recipe not found', { id: command.recipeId }); + return { recipe: null }; } + + this.logger.info('Recipe retrieved successfully', { + id: command.recipeId, + }); + return { recipe }; } /** diff --git a/packages/commands/src/application/useCases/getCommandVersion/GetCommandVersionUseCase.ts b/packages/commands/src/application/useCases/getCommandVersion/GetCommandVersionUseCase.ts index 8bd23b25e6..c774b7b8bc 100644 --- a/packages/commands/src/application/useCases/getCommandVersion/GetCommandVersionUseCase.ts +++ b/packages/commands/src/application/useCases/getCommandVersion/GetCommandVersionUseCase.ts @@ -22,25 +22,16 @@ export class GetCommandVersionUseCase { ): Promise { this.logger.info('Getting recipe version', { recipeId, version }); - try { - const recipeVersion = await this.commandVersionService.getCommandVersion( - recipeId, - version, - allowedSpaceIds, - ); - this.logger.info('Recipe version retrieved successfully', { - recipeId, - version, - found: !!recipeVersion, - }); - return recipeVersion; - } catch (error) { - this.logger.error('Failed to get recipe version', { - recipeId, - version, - error: error instanceof Error ? error.message : String(error), - }); - throw error; - } + const recipeVersion = await this.commandVersionService.getCommandVersion( + recipeId, + version, + allowedSpaceIds, + ); + this.logger.info('Recipe version retrieved successfully', { + recipeId, + version, + found: !!recipeVersion, + }); + return recipeVersion; } } diff --git a/packages/commands/src/application/useCases/listCommandVersions/ListCommandVersionsUseCase.ts b/packages/commands/src/application/useCases/listCommandVersions/ListCommandVersionsUseCase.ts index 4f0e1b3008..7bcc480be5 100644 --- a/packages/commands/src/application/useCases/listCommandVersions/ListCommandVersionsUseCase.ts +++ b/packages/commands/src/application/useCases/listCommandVersions/ListCommandVersionsUseCase.ts @@ -20,20 +20,12 @@ export class ListCommandVersionsUseCase { ): Promise { this.logger.info('Listing recipe versions', { recipeId }); - try { - const versions = - await this.commandVersionService.listCommandVersions(recipeId); - this.logger.info('Recipe versions listed successfully', { - recipeId, - count: versions.length, - }); - return versions; - } catch (error) { - this.logger.error('Failed to list recipe versions', { - recipeId, - error: error instanceof Error ? error.message : String(error), - }); - throw error; - } + const versions = + await this.commandVersionService.listCommandVersions(recipeId); + this.logger.info('Recipe versions listed successfully', { + recipeId, + count: versions.length, + }); + return versions; } } diff --git a/packages/commands/src/application/useCases/listCommandsBySpace/ListCommandsBySpaceUseCase.spec.ts b/packages/commands/src/application/useCases/listCommandsBySpace/ListCommandsBySpaceUseCase.spec.ts index 7e5ff50a07..83b0d2bdd8 100644 --- a/packages/commands/src/application/useCases/listCommandsBySpace/ListCommandsBySpaceUseCase.spec.ts +++ b/packages/commands/src/application/useCases/listCommandsBySpace/ListCommandsBySpaceUseCase.spec.ts @@ -1,4 +1,5 @@ import { ListCommandsBySpaceUseCase } from './ListCommandsBySpaceUseCase'; +import { CommandSpaceNotAccessibleError } from '../../../domain/errors'; import { CommandService } from '../../services/CommandService'; import { commandFactory } from '../../../../test/commandFactory'; import { @@ -351,7 +352,7 @@ describe('ListRecipesBySpaceUseCase', () => { }); describe('when space is not found', () => { - it('throws Space not found error', async () => { + it('throws CommandSpaceNotAccessibleError', async () => { const organizationId = createOrganizationId('org-1'); const spaceId = createSpaceId('space-1'); const userId = createUserId('user-1'); @@ -386,12 +387,12 @@ describe('ListRecipesBySpaceUseCase', () => { organizationId, spaceId, }), - ).rejects.toThrow(`Space with id ${spaceId} not found`); + ).rejects.toBeInstanceOf(CommandSpaceNotAccessibleError); }); }); describe('when space does not belong to organization', () => { - it('throws Space does not belong to organization error', async () => { + it('throws CommandSpaceNotAccessibleError', async () => { const organizationId = createOrganizationId('org-1'); const differentOrgId = createOrganizationId('org-2'); const spaceId = createSpaceId('space-1'); @@ -431,9 +432,7 @@ describe('ListRecipesBySpaceUseCase', () => { organizationId, spaceId, }), - ).rejects.toThrow( - `Space ${spaceId} does not belong to organization ${organizationId}`, - ); + ).rejects.toBeInstanceOf(CommandSpaceNotAccessibleError); }); }); diff --git a/packages/commands/src/application/useCases/listCommandsBySpace/ListCommandsBySpaceUseCase.ts b/packages/commands/src/application/useCases/listCommandsBySpace/ListCommandsBySpaceUseCase.ts index fae5c808aa..e7f4539362 100644 --- a/packages/commands/src/application/useCases/listCommandsBySpace/ListCommandsBySpaceUseCase.ts +++ b/packages/commands/src/application/useCases/listCommandsBySpace/ListCommandsBySpaceUseCase.ts @@ -10,6 +10,7 @@ import { ListCommandsBySpaceCommand, ListCommandsBySpaceResponse, } from '@packmind/types'; +import { CommandSpaceNotAccessibleError } from '../../../domain/errors'; import { CommandService } from '../../services/CommandService'; const origin = 'ListRecipesBySpaceUseCase'; @@ -39,46 +40,25 @@ export class ListCommandsBySpaceUseCase organizationId: command.organizationId, }); - try { - // Verify the space belongs to the organization - const space = await this.spacesPort.getSpaceById(command.spaceId); - if (!space) { - this.logger.warn('Space not found', { - spaceId: command.spaceId, - }); - throw new Error(`Space with id ${command.spaceId} not found`); - } - - if (space.organizationId !== command.organizationId) { - this.logger.warn('Space does not belong to organization', { - spaceId: command.spaceId, - spaceOrganizationId: space.organizationId, - requestOrganizationId: command.organizationId, - }); - throw new Error( - `Space ${command.spaceId} does not belong to organization ${command.organizationId}`, - ); - } - - const recipes = await this.commandService.listCommandsBySpace( + const space = await this.spacesPort.getSpaceById(command.spaceId); + if (!space || space.organizationId !== command.organizationId) { + throw new CommandSpaceNotAccessibleError( command.spaceId, - { includeDeleted: command.includeDeleted }, + command.organizationId, ); + } - this.logger.info('Recipes listed by space successfully', { - spaceId: command.spaceId, - organizationId: command.organizationId, - count: recipes.length, - }); + const recipes = await this.commandService.listCommandsBySpace( + command.spaceId, + { includeDeleted: command.includeDeleted }, + ); - return { recipes }; - } catch (error) { - this.logger.error('Failed to list recipes by space', { - spaceId: command.spaceId, - organizationId: command.organizationId, - error: error instanceof Error ? error.message : String(error), - }); - throw error; - } + this.logger.info('Recipes listed by space successfully', { + spaceId: command.spaceId, + organizationId: command.organizationId, + count: recipes.length, + }); + + return { recipes }; } } diff --git a/packages/commands/src/application/useCases/updateCommandFromUI/UpdateCommandFromUIUseCase.ts b/packages/commands/src/application/useCases/updateCommandFromUI/UpdateCommandFromUIUseCase.ts index 7071d0112e..b9750f7c6e 100644 --- a/packages/commands/src/application/useCases/updateCommandFromUI/UpdateCommandFromUIUseCase.ts +++ b/packages/commands/src/application/useCases/updateCommandFromUI/UpdateCommandFromUIUseCase.ts @@ -12,6 +12,10 @@ import { UpdateCommandFromUICommand, UpdateCommandFromUIResponse, } from '@packmind/types'; +import { + CommandNotFoundError, + CommandSpaceNotAccessibleError, +} from '../../../domain/errors'; import { CommandService } from '../../services/CommandService'; import { CommandVersionService } from '../../services/CommandVersionService'; @@ -49,40 +53,16 @@ export class UpdateCommandFromUIUseCase userId, }); - // Verify the space belongs to the organization const space = await this.spacesPort.getSpaceById(spaceId); - if (!space) { - this.logger.warn('Space not found', { spaceId }); - throw new Error(`Space with id ${spaceId} not found`); - } - - if (space.organizationId !== organizationId) { - this.logger.warn('Space does not belong to organization', { - spaceId, - spaceOrganizationId: space.organizationId, - requestOrganizationId: organizationId, - }); - throw new Error( - `Space ${spaceId} does not belong to organization ${organizationId}`, - ); + if (!space || space.organizationId !== organizationId) { + throw new CommandSpaceNotAccessibleError(spaceId, organizationId); } this.logger.info('Fetching existing recipe', { recipeId }); const existingCommand = await this.commandService.getCommandById(recipeId); - if (!existingCommand) { - this.logger.error('Recipe not found', { recipeId }); - throw new Error(`Recipe with id ${recipeId} not found`); - } - - // Security validation: ensure recipe belongs to the specified space - if (existingCommand.spaceId !== spaceId) { - this.logger.error('Recipe does not belong to specified space', { - recipeId, - recipeSpaceId: existingCommand.spaceId, - requestedSpaceId: spaceId, - }); - throw new Error(`Recipe ${recipeId} does not belong to space ${spaceId}`); + if (!existingCommand || existingCommand.spaceId !== spaceId) { + throw new CommandNotFoundError(recipeId, spaceId); } const nextVersion = existingCommand.version + 1; diff --git a/packages/commands/src/domain/errors/CommandNotFoundError.ts b/packages/commands/src/domain/errors/CommandNotFoundError.ts new file mode 100644 index 0000000000..c3f3989f0c --- /dev/null +++ b/packages/commands/src/domain/errors/CommandNotFoundError.ts @@ -0,0 +1,22 @@ +import { CommandsError } from './CommandsError'; + +/** + * The command does not exist, or it belongs to another space than the one + * asked for. + * + * One error for both, with one message, on purpose: a caller must not be + * able to tell a command outside their space from one that was never there. + * The space id that was asked for is kept in the context, where only the log + * sees it. + */ +export class CommandNotFoundError extends CommandsError { + constructor(commandId: string, spaceId?: string) { + super( + 'not_found', + 'command_not_found', + { commandId, ...(spaceId ? { spaceId } : {}) }, + 'This command does not exist, or you do not have access to it.', + ); + this.name = 'CommandNotFoundError'; + } +} diff --git a/packages/commands/src/domain/errors/CommandSlugAlreadyExistsError.ts b/packages/commands/src/domain/errors/CommandSlugAlreadyExistsError.ts new file mode 100644 index 0000000000..4dbd845c8b --- /dev/null +++ b/packages/commands/src/domain/errors/CommandSlugAlreadyExistsError.ts @@ -0,0 +1,23 @@ +import { CommandsError } from './CommandsError'; + +/** + * A command with this slug already exists in the space. + * + * The slug is what the caller typed, so it stays in the message to tell them + * what to change; the space id goes to the context, where only the log sees + * it. + */ +export class CommandSlugAlreadyExistsError extends CommandsError { + constructor( + public readonly slug: string, + public readonly spaceId: string, + ) { + super( + 'conflict', + 'command_slug_already_exists', + { commandSlug: slug, spaceId }, + `A command with slug "${slug}" already exists in this space`, + ); + this.name = 'CommandSlugAlreadyExistsError'; + } +} diff --git a/packages/commands/src/domain/errors/CommandSpaceNotAccessibleError.ts b/packages/commands/src/domain/errors/CommandSpaceNotAccessibleError.ts new file mode 100644 index 0000000000..2f42e6f2c5 --- /dev/null +++ b/packages/commands/src/domain/errors/CommandSpaceNotAccessibleError.ts @@ -0,0 +1,25 @@ +import { CommandsError } from './CommandsError'; + +/** + * The space does not exist, or it belongs to another organization. + * + * One error for both, with one message, on purpose: a caller outside the + * organization must not be able to tell a space that is not theirs from one + * that was never there. The organization id that was asked for is kept in the + * context, where only the log sees it. + * + * Named `CommandSpaceNotAccessible` rather than `SpaceNotAccessible` because + * `@packmind/deployments`, `@packmind/editions` and `@packmind/standards` + * already export space not-found classes. + */ +export class CommandSpaceNotAccessibleError extends CommandsError { + constructor(spaceId: string, organizationId?: string) { + super( + 'not_found', + 'space_not_accessible', + { spaceId, ...(organizationId ? { organizationId } : {}) }, + 'This space does not exist, or you do not have access to it.', + ); + this.name = 'CommandSpaceNotAccessibleError'; + } +} diff --git a/packages/commands/src/domain/errors/CommandsAdapterPortsMissingError.ts b/packages/commands/src/domain/errors/CommandsAdapterPortsMissingError.ts new file mode 100644 index 0000000000..885b771768 --- /dev/null +++ b/packages/commands/src/domain/errors/CommandsAdapterPortsMissingError.ts @@ -0,0 +1,19 @@ +import { CommandsInternalError } from './CommandsInternalError'; + +/** + * The adapter was initialized without the ports or delayed jobs its use + * cases need. + * + * A wiring invariant: `missingPorts` names which ones were absent; no + * request could avoid it. + */ +export class CommandsAdapterPortsMissingError extends CommandsInternalError { + constructor(missingPorts: string[]) { + super( + 'commands_adapter_ports_missing', + { missingPorts }, + `CommandsAdapter: Required ports/delayed jobs not provided: ${missingPorts.join(', ')}.`, + ); + this.name = 'CommandsAdapterPortsMissingError'; + } +} diff --git a/packages/commands/src/domain/errors/CommandsError.spec.ts b/packages/commands/src/domain/errors/CommandsError.spec.ts new file mode 100644 index 0000000000..a092ad4f6e --- /dev/null +++ b/packages/commands/src/domain/errors/CommandsError.spec.ts @@ -0,0 +1,170 @@ +import { isDomainError, isInternalError } from '@packmind/types'; +import { CommandsError } from './CommandsError'; +import { CommandSlugAlreadyExistsError } from './CommandSlugAlreadyExistsError'; +import { CommandSpaceNotAccessibleError } from './CommandSpaceNotAccessibleError'; +import { CommandNotFoundError } from './CommandNotFoundError'; + +describe('CommandsError', () => { + const error = new CommandsError( + 'conflict', + 'command_slug_already_exists', + { commandSlug: 'my-command', spaceId: 'space-1' }, + 'A command with slug "my-command" already exists in this space', + ); + + it('is a domain error', () => { + expect(isDomainError(error)).toBe(true); + }); + + it('is not an internal error', () => { + expect(isInternalError(error)).toBe(false); + }); + + it('answers its own kind', () => { + expect(error.kind).toBe('conflict'); + }); + + it('answers its own reason', () => { + expect(error.reason).toBe('command_slug_already_exists'); + }); + + it('keeps the ids in the context', () => { + expect(error.context).toEqual({ + commandSlug: 'my-command', + spaceId: 'space-1', + }); + }); +}); + +describe('CommandSlugAlreadyExistsError', () => { + const error = new CommandSlugAlreadyExistsError('my-command', 'space-1'); + + it('is a domain error', () => { + expect(isDomainError(error)).toBe(true); + }); + + it('is not an internal error', () => { + expect(isInternalError(error)).toBe(false); + }); + + it('is a commands error', () => { + expect(error).toBeInstanceOf(CommandsError); + }); + + it('answers conflict', () => { + expect(error.kind).toBe('conflict'); + }); + + it('answers its own reason', () => { + expect(error.reason).toBe('command_slug_already_exists'); + }); + + it('keeps the slug and space in the context', () => { + expect(error.context).toEqual({ + commandSlug: 'my-command', + spaceId: 'space-1', + }); + }); + + it('names itself', () => { + expect(error.name).toBe('CommandSlugAlreadyExistsError'); + }); + + it('keeps the slug in the message', () => { + expect(error.message).toContain('my-command'); + }); + + it('does not leak the space id in the message', () => { + expect(error.message).not.toContain('space-1'); + }); +}); + +describe('CommandSpaceNotAccessibleError', () => { + const error = new CommandSpaceNotAccessibleError('space-1', 'org-1'); + + it('is a domain error', () => { + expect(isDomainError(error)).toBe(true); + }); + + it('is a commands error', () => { + expect(error).toBeInstanceOf(CommandsError); + }); + + it('answers not_found', () => { + expect(error.kind).toBe('not_found'); + }); + + it('answers its own reason', () => { + expect(error.reason).toBe('space_not_accessible'); + }); + + it('keeps the space and organization in the context', () => { + expect(error.context).toEqual({ + spaceId: 'space-1', + organizationId: 'org-1', + }); + }); + + describe('when no organization is given', () => { + it('leaves the organization out of the context', () => { + expect(new CommandSpaceNotAccessibleError('space-1').context).toEqual({ + spaceId: 'space-1', + }); + }); + }); + + it('names itself', () => { + expect(error.name).toBe('CommandSpaceNotAccessibleError'); + }); + + it('does not leak the ids in the message', () => { + expect(error.message).toBe( + 'This space does not exist, or you do not have access to it.', + ); + }); +}); + +describe('CommandNotFoundError', () => { + const error = new CommandNotFoundError('command-1', 'space-1'); + + it('is a domain error', () => { + expect(isDomainError(error)).toBe(true); + }); + + it('is a commands error', () => { + expect(error).toBeInstanceOf(CommandsError); + }); + + it('answers not_found', () => { + expect(error.kind).toBe('not_found'); + }); + + it('answers its own reason', () => { + expect(error.reason).toBe('command_not_found'); + }); + + it('keeps the command and space in the context', () => { + expect(error.context).toEqual({ + commandId: 'command-1', + spaceId: 'space-1', + }); + }); + + describe('when no space is given', () => { + it('leaves the space out of the context', () => { + expect(new CommandNotFoundError('command-1').context).toEqual({ + commandId: 'command-1', + }); + }); + }); + + it('names itself', () => { + expect(error.name).toBe('CommandNotFoundError'); + }); + + it('does not leak the ids in the message', () => { + expect(error.message).toBe( + 'This command does not exist, or you do not have access to it.', + ); + }); +}); diff --git a/packages/commands/src/domain/errors/CommandsError.ts b/packages/commands/src/domain/errors/CommandsError.ts new file mode 100644 index 0000000000..d47527a1b9 --- /dev/null +++ b/packages/commands/src/domain/errors/CommandsError.ts @@ -0,0 +1,38 @@ +import { DomainError, DomainErrorKind } from '@packmind/types'; + +export type CommandsErrorReason = + | 'command_not_found' + | 'command_slug_already_exists' + | 'space_not_accessible'; + +export type CommandsErrorContext = { + commandId?: string; + commandSlug?: string; + spaceId?: string; + organizationId?: string; +}; + +/** + * Base for the commands domain errors, in the same shape as `StandardsError`: + * the `kind` decides the HTTP answer, the literal `reason` is what a client + * branches on, and `context` carries the ids for the log instead of only + * being interpolated into the message. + */ +export class CommandsError extends Error implements DomainError { + readonly kind: DomainErrorKind; + readonly reason: CommandsErrorReason; + readonly context: CommandsErrorContext; + + constructor( + kind: DomainErrorKind, + reason: CommandsErrorReason, + context: CommandsErrorContext, + message: string, + ) { + super(message); + this.name = 'CommandsError'; + this.kind = kind; + this.reason = reason; + this.context = context; + } +} diff --git a/packages/commands/src/domain/errors/CommandsInternalError.spec.ts b/packages/commands/src/domain/errors/CommandsInternalError.spec.ts new file mode 100644 index 0000000000..4edacb1601 --- /dev/null +++ b/packages/commands/src/domain/errors/CommandsInternalError.spec.ts @@ -0,0 +1,124 @@ +import { isDomainError, isInternalError } from '@packmind/types'; +import { CommandsInternalError } from './CommandsInternalError'; +import { CommandsAdapterPortsMissingError } from './CommandsAdapterPortsMissingError'; +import { + DeployCommandsDelayedJobNotCreatedError, + DeployCommandsQueueNotInitializedError, +} from './DeployCommandsQueueErrors'; + +describe('CommandsInternalError', () => { + const error = new CommandsInternalError( + 'commands_adapter_ports_missing', + { missingPorts: ['IGitPort'] }, + 'CommandsAdapter: Required ports/delayed jobs not provided: IGitPort.', + ); + + it('is an internal error', () => { + expect(isInternalError(error)).toBe(true); + }); + + it('is not a domain error', () => { + expect(isDomainError(error)).toBe(false); + }); + + it('answers its own reason', () => { + expect(error.reason).toBe('commands_adapter_ports_missing'); + }); + + it('keeps the ids in the context', () => { + expect(error.context).toEqual({ missingPorts: ['IGitPort'] }); + }); +}); + +describe('CommandsAdapterPortsMissingError', () => { + const error = new CommandsAdapterPortsMissingError([ + 'IGitPort', + 'commandsDelayedJobs', + ]); + + it('is an internal error', () => { + expect(isInternalError(error)).toBe(true); + }); + + it('is not a domain error', () => { + expect(isDomainError(error)).toBe(false); + }); + + it('is a commands internal error', () => { + expect(error).toBeInstanceOf(CommandsInternalError); + }); + + it('answers its own reason', () => { + expect(error.reason).toBe('commands_adapter_ports_missing'); + }); + + it('keeps the missing ports in the context', () => { + expect(error.context).toEqual({ + missingPorts: ['IGitPort', 'commandsDelayedJobs'], + }); + }); + + it('names the missing ports in the message', () => { + expect(error.message).toContain('IGitPort, commandsDelayedJobs'); + }); + + it('names itself', () => { + expect(error.name).toBe('CommandsAdapterPortsMissingError'); + }); +}); + +describe('DeployCommandsQueueNotInitializedError', () => { + const error = new DeployCommandsQueueNotInitializedError(); + + it('is an internal error', () => { + expect(isInternalError(error)).toBe(true); + }); + + it('is not a domain error', () => { + expect(isDomainError(error)).toBe(false); + }); + + it('is a commands internal error', () => { + expect(error).toBeInstanceOf(CommandsInternalError); + }); + + it('answers its own reason', () => { + expect(error.reason).toBe('deploy_commands_queue_not_initialized'); + }); + + it('carries an empty context', () => { + expect(error.context).toEqual({}); + }); + + it('names itself', () => { + expect(error.name).toBe('DeployCommandsQueueNotInitializedError'); + }); +}); + +describe('DeployCommandsDelayedJobNotCreatedError', () => { + const error = new DeployCommandsDelayedJobNotCreatedError(); + + it('is an internal error', () => { + expect(isInternalError(error)).toBe(true); + }); + + it('is not a domain error', () => { + expect(isDomainError(error)).toBe(false); + }); + + it('is a commands internal error', () => { + expect(error).toBeInstanceOf(CommandsInternalError); + }); + + it('answers its own reason', () => { + expect(error.reason).toBe('deploy_commands_delayed_job_not_created'); + }); + + it('carries an empty context', () => { + expect(error.context).toEqual({}); + }); + + it('names itself', () => { + expect(error.name).toBe('DeployCommandsDelayedJobNotCreatedError'); + }); +}); diff --git a/packages/commands/src/domain/errors/CommandsInternalError.ts b/packages/commands/src/domain/errors/CommandsInternalError.ts new file mode 100644 index 0000000000..e75860a60e --- /dev/null +++ b/packages/commands/src/domain/errors/CommandsInternalError.ts @@ -0,0 +1,33 @@ +import { PackmindInternalError } from '@packmind/types'; + +export type CommandsInternalErrorReason = + | 'commands_adapter_ports_missing' + | 'deploy_commands_queue_not_initialized' + | 'deploy_commands_delayed_job_not_created'; + +export type CommandsInternalErrorContext = { + missingPorts?: string[]; +}; + +/** + * Base for the commands broken invariants, in the same shape as + * `CommandsError` but on the internal side: no `kind` to choose — it is + * always 500, logged with its stack and with its message withheld from the + * client — a literal `reason` union naming the invariant and a typed `context` + * carrying the ids. + * + * The line between this and `CommandsError` is fault, not severity: if the + * caller could have avoided it by asking for something else, it is a + * `CommandsError`. If the wiring is broken or stored state has lost an + * invariant, it is this. + */ +export class CommandsInternalError extends PackmindInternalError { + constructor( + reason: CommandsInternalErrorReason, + context: CommandsInternalErrorContext, + message: string, + ) { + super(reason, context, message); + this.name = 'CommandsInternalError'; + } +} diff --git a/packages/commands/src/domain/errors/DeployCommandsQueueErrors.ts b/packages/commands/src/domain/errors/DeployCommandsQueueErrors.ts new file mode 100644 index 0000000000..ff1960e2f1 --- /dev/null +++ b/packages/commands/src/domain/errors/DeployCommandsQueueErrors.ts @@ -0,0 +1,30 @@ +import { CommandsInternalError } from './CommandsInternalError'; + +/** + * The lifecycle guards on the deploy-commands queue handle. Both name a step + * of our own wiring that ran out of order — the queue is built and + * initialized by the adapter that owns it, never by a request — so neither is + * anything a caller can provoke or correct. + */ + +export class DeployCommandsQueueNotInitializedError extends CommandsInternalError { + constructor() { + super( + 'deploy_commands_queue_not_initialized', + {}, + 'Queue not initialized. Call initialize() first.', + ); + this.name = 'DeployCommandsQueueNotInitializedError'; + } +} + +export class DeployCommandsDelayedJobNotCreatedError extends CommandsInternalError { + constructor() { + super( + 'deploy_commands_delayed_job_not_created', + {}, + 'DelayedJob not created. Call createQueue() first.', + ); + this.name = 'DeployCommandsDelayedJobNotCreatedError'; + } +} diff --git a/packages/commands/src/domain/errors/index.ts b/packages/commands/src/domain/errors/index.ts new file mode 100644 index 0000000000..1945808840 --- /dev/null +++ b/packages/commands/src/domain/errors/index.ts @@ -0,0 +1,7 @@ +export * from './CommandsError'; +export * from './CommandsInternalError'; +export * from './CommandsAdapterPortsMissingError'; +export * from './CommandNotFoundError'; +export * from './CommandSlugAlreadyExistsError'; +export * from './CommandSpaceNotAccessibleError'; +export * from './DeployCommandsQueueErrors'; diff --git a/packages/commands/src/index.ts b/packages/commands/src/index.ts index c85d319208..dd3094318b 100644 --- a/packages/commands/src/index.ts +++ b/packages/commands/src/index.ts @@ -1,4 +1,5 @@ export { CommandsHexa } from './CommandsHexa'; +export * from './domain/errors'; export * from './domain/jobs'; export * from './infra/schemas'; export { DeployCommandsCallback } from './application/jobs/DeployCommandsDelayedJob'; diff --git a/packages/commands/src/infra/jobs/DeployCommandsJobFactory.ts b/packages/commands/src/infra/jobs/DeployCommandsJobFactory.ts index 2016f30289..a7c2993165 100644 --- a/packages/commands/src/infra/jobs/DeployCommandsJobFactory.ts +++ b/packages/commands/src/infra/jobs/DeployCommandsJobFactory.ts @@ -3,6 +3,10 @@ import { PackmindLogger } from '@packmind/logger'; import { IDeploymentPort } from '@packmind/types'; import { DeployCommandsDelayedJob } from '../../application/jobs/DeployCommandsDelayedJob'; import { DeployCommandsInput } from '../../domain/jobs/DeployCommands'; +import { + DeployCommandsDelayedJobNotCreatedError, + DeployCommandsQueueNotInitializedError, +} from '../../domain/errors'; const origin = 'DeployRecipesJobFactory'; @@ -25,14 +29,14 @@ export class DeployCommandsJobFactory implements IJobFactory => { if (!this._delayedJob) { - throw new Error('Queue not initialized. Call initialize() first.'); + throw new DeployCommandsQueueNotInitializedError(); } const jobId = await this._delayedJob.addJob(input); return jobId; }, initialize: async (): Promise => { if (!this._delayedJob) { - throw new Error('DelayedJob not created. Call createQueue() first.'); + throw new DeployCommandsDelayedJobNotCreatedError(); } await this._delayedJob.initialize(); this.logger.info('DeployRecipes queue initialized'); @@ -45,9 +49,7 @@ export class DeployCommandsJobFactory implements IJobFactory { this.logger.info('Finding recipes by user ID', { userId }); - try { - const recipes = await this.repository.find({ where: { userId } }); - this.logger.info('Recipes found by user ID', { - userId, - count: recipes.length, - }); - return recipes; - } catch (error) { - this.logger.error('Failed to find recipes by user ID', { - userId, - error: error instanceof Error ? error.message : String(error), - }); - throw error; - } + const recipes = await this.repository.find({ where: { userId } }); + this.logger.info('Recipes found by user ID', { + userId, + count: recipes.length, + }); + return recipes; } async findBySpaceId( @@ -111,34 +94,26 @@ export class CommandRepository includeDeleted: opts?.includeDeleted ?? false, }); - try { - const recipes = await this.repository.find({ - where: { spaceId }, - relations: ['gitCommit'], - withDeleted: opts?.includeDeleted ?? false, - }); + const recipes = await this.repository.find({ + where: { spaceId }, + relations: ['gitCommit'], + withDeleted: opts?.includeDeleted ?? false, + }); - const createdByUserId = await this.getCreatedByMany( - recipes.map((recipe) => recipe.userId), - ); + const createdByUserId = await this.getCreatedByMany( + recipes.map((recipe) => recipe.userId), + ); - const commandsWithScope = recipes.map((recipe) => ({ - ...recipe, - createdBy: createdByUserId.get(recipe.userId), - })); + const commandsWithScope = recipes.map((recipe) => ({ + ...recipe, + createdBy: createdByUserId.get(recipe.userId), + })); - this.logger.info('Recipes with scope found by space ID', { - spaceId, - count: commandsWithScope.length, - }); - return commandsWithScope; - } catch (error) { - this.logger.error('Failed to find recipes with scope by space ID', { - spaceId, - error: error instanceof Error ? error.message : String(error), - }); - throw error; - } + this.logger.info('Recipes with scope found by space ID', { + spaceId, + count: commandsWithScope.length, + }); + return commandsWithScope; } async countBySpaceIds(spaceIds: SpaceId[]): Promise> { @@ -150,22 +125,15 @@ export class CommandRepository spaceCount: spaceIds.length, }); - try { - const rows = await this.repository - .createQueryBuilder('recipe') - .select('recipe.space_id', 'spaceId') - .addSelect('COUNT(*)', 'count') - .where('recipe.space_id IN (:...spaceIds)', { spaceIds }) - .groupBy('recipe.space_id') - .getRawMany<{ spaceId: SpaceId; count: string }>(); - - return new Map(rows.map((row) => [row.spaceId, Number(row.count)])); - } catch (error) { - this.logger.error('Failed to count recipes by space IDs', { - error: error instanceof Error ? error.message : String(error), - }); - throw error; - } + const rows = await this.repository + .createQueryBuilder('recipe') + .select('recipe.space_id', 'spaceId') + .addSelect('COUNT(*)', 'count') + .where('recipe.space_id IN (:...spaceIds)', { spaceIds }) + .groupBy('recipe.space_id') + .getRawMany<{ spaceId: SpaceId; count: string }>(); + + return new Map(rows.map((row) => [row.spaceId, Number(row.count)])); } async markAsMoved( @@ -177,26 +145,18 @@ export class CommandRepository destinationSpaceId, }); - try { - await this.repository.manager.transaction(async (manager) => { - const transactionalRepository = manager.getRepository(CommandSchema); - await transactionalRepository.update( - { id: recipeId }, - { movedTo: destinationSpaceId }, - ); - await transactionalRepository.softDelete({ id: recipeId }); - }); + await this.repository.manager.transaction(async (manager) => { + const transactionalRepository = manager.getRepository(CommandSchema); + await transactionalRepository.update( + { id: recipeId }, + { movedTo: destinationSpaceId }, + ); + await transactionalRepository.softDelete({ id: recipeId }); + }); - this.logger.info('Recipe marked as moved successfully', { - recipeId, - destinationSpaceId, - }); - } catch (error) { - this.logger.error('Failed to mark recipe as moved', { - recipeId, - error: error instanceof Error ? error.message : String(error), - }); - throw error; - } + this.logger.info('Recipe marked as moved successfully', { + recipeId, + destinationSpaceId, + }); } } diff --git a/packages/commands/src/infra/repositories/CommandVersionRepository.ts b/packages/commands/src/infra/repositories/CommandVersionRepository.ts index 4c39c117a1..8507499bac 100644 --- a/packages/commands/src/infra/repositories/CommandVersionRepository.ts +++ b/packages/commands/src/infra/repositories/CommandVersionRepository.ts @@ -2,11 +2,7 @@ import { ICommandVersionRepository } from '../../domain/repositories/ICommandVer import { CommandVersionSchema } from '../schemas/CommandVersionSchema'; import { Repository } from 'typeorm'; import { PackmindLogger } from '@packmind/logger'; -import { - localDataSource, - AbstractRepository, - getErrorMessage, -} from '@packmind/node-utils'; +import { localDataSource, AbstractRepository } from '@packmind/node-utils'; import { CommandId, CommandVersion, SpaceId } from '@packmind/types'; const origin = 'RecipeVersionRepository'; @@ -39,25 +35,17 @@ export class CommandVersionRepository async findByCommandId(recipeId: CommandId): Promise { this.logger.info('Finding recipe versions by recipe ID', { recipeId }); - try { - const versions = await this.repository.find({ - // eslint-disable-next-line @typescript-eslint/no-explicit-any - where: { recipeId: recipeId as any }, // TypeORM compatibility with branded types - order: { version: 'DESC' }, - relations: ['gitCommit'], - }); - this.logger.info('Recipe versions found by recipe ID', { - recipeId, - count: versions.length, - }); - return versions; - } catch (error) { - this.logger.error('Failed to find recipe versions by recipe ID', { - recipeId, - error: getErrorMessage(error), - }); - throw error; - } + const versions = await this.repository.find({ + // eslint-disable-next-line @typescript-eslint/no-explicit-any + where: { recipeId: recipeId as any }, // TypeORM compatibility with branded types + order: { version: 'DESC' }, + relations: ['gitCommit'], + }); + this.logger.info('Recipe versions found by recipe ID', { + recipeId, + count: versions.length, + }); + return versions; } async findLatestByCommandIds( @@ -74,30 +62,22 @@ export class CommandVersionRepository count: uniqueCommandIds.length, }); - try { - const versions = await this.repository - .createQueryBuilder('recipeVersion') - .where('recipeVersion.recipeId IN (:...recipeIds)', { - recipeIds: uniqueCommandIds as string[], - }) - .distinctOn(['recipeVersion.recipeId']) - .orderBy('recipeVersion.recipeId', 'ASC') - .addOrderBy('recipeVersion.version', 'DESC') - .getMany(); - - this.logger.info('Latest recipe versions found by recipe IDs', { - requestedCount: uniqueCommandIds.length, - foundCount: versions.length, - }); + const versions = await this.repository + .createQueryBuilder('recipeVersion') + .where('recipeVersion.recipeId IN (:...recipeIds)', { + recipeIds: uniqueCommandIds as string[], + }) + .distinctOn(['recipeVersion.recipeId']) + .orderBy('recipeVersion.recipeId', 'ASC') + .addOrderBy('recipeVersion.version', 'DESC') + .getMany(); + + this.logger.info('Latest recipe versions found by recipe IDs', { + requestedCount: uniqueCommandIds.length, + foundCount: versions.length, + }); - return versions; - } catch (error) { - this.logger.error('Failed to find latest recipe versions by recipe IDs', { - count: uniqueCommandIds.length, - error: getErrorMessage(error), - }); - throw error; - } + return versions; } async findByCommandIdAndVersion( @@ -119,41 +99,29 @@ export class CommandVersionRepository return null; } - try { - const recipeVersion = await this.repository - .createQueryBuilder('rv') - .innerJoin('rv.recipe', 'recipe') - .where('rv.command_id = :recipeId', { recipeId }) - .andWhere('rv.version = :version', { version }) - .andWhere('recipe.space_id IN (:...allowedSpaceIds)', { - allowedSpaceIds, - }) - .getOne(); - - if (recipeVersion) { - this.logger.info('Recipe version found by recipe ID and version', { - recipeId, - version, - versionId: recipeVersion.id, - }); - } else { - this.logger.warn('Recipe version not found by recipe ID and version', { - recipeId, - version, - }); - } - - return recipeVersion; - } catch (error) { - this.logger.error( - 'Failed to find recipe version by recipe ID and version', - { - recipeId, - version, - error: getErrorMessage(error), - }, - ); - throw error; + const recipeVersion = await this.repository + .createQueryBuilder('rv') + .innerJoin('rv.recipe', 'recipe') + .where('rv.command_id = :recipeId', { recipeId }) + .andWhere('rv.version = :version', { version }) + .andWhere('recipe.space_id IN (:...allowedSpaceIds)', { + allowedSpaceIds, + }) + .getOne(); + + if (recipeVersion) { + this.logger.info('Recipe version found by recipe ID and version', { + recipeId, + version, + versionId: recipeVersion.id, + }); + } else { + this.logger.warn('Recipe version not found by recipe ID and version', { + recipeId, + version, + }); } + + return recipeVersion; } } diff --git a/packages/types/src/commands/errors/CommandSlugAlreadyExistsError.ts b/packages/types/src/commands/errors/CommandSlugAlreadyExistsError.ts deleted file mode 100644 index 671dd09416..0000000000 --- a/packages/types/src/commands/errors/CommandSlugAlreadyExistsError.ts +++ /dev/null @@ -1,30 +0,0 @@ -// Error.captureStackTrace is V8-only, so it is absent from the standard Error type. -interface IErrorWithCaptureStackTrace { - captureStackTrace: ( - error: Error, - constructor: new (...args: unknown[]) => unknown, - ) => void; -} - -function hasCaptureStackTrace( - error: typeof Error, -): error is typeof Error & IErrorWithCaptureStackTrace { - return ( - typeof (error as unknown as IErrorWithCaptureStackTrace) - .captureStackTrace === 'function' - ); -} - -export class CommandSlugAlreadyExistsError extends Error { - constructor( - public readonly slug: string, - public readonly spaceId: string, - ) { - super(`A command with slug "${slug}" already exists in this space`); - this.name = 'RecipeSlugAlreadyExistsError'; - - if (hasCaptureStackTrace(Error)) { - Error.captureStackTrace(this, CommandSlugAlreadyExistsError); - } - } -} diff --git a/packages/types/src/commands/errors/index.ts b/packages/types/src/commands/errors/index.ts deleted file mode 100644 index 770962b874..0000000000 --- a/packages/types/src/commands/errors/index.ts +++ /dev/null @@ -1 +0,0 @@ -export * from './CommandSlugAlreadyExistsError'; diff --git a/packages/types/src/commands/index.ts b/packages/types/src/commands/index.ts index 81a7495b48..71b529ab6a 100644 --- a/packages/types/src/commands/index.ts +++ b/packages/types/src/commands/index.ts @@ -2,6 +2,5 @@ export * from './CommandId'; export * from './Command'; export * from './CommandVersion'; export * from './contracts'; -export * from './errors'; export * from './events'; export * from './ports'; diff --git a/packages/types/src/marketplaces/jobs/PublishPluginToMarketplaceJob.ts b/packages/types/src/marketplaces/jobs/PublishPluginToMarketplaceJob.ts index 4458c7a969..81b37a3987 100644 --- a/packages/types/src/marketplaces/jobs/PublishPluginToMarketplaceJob.ts +++ b/packages/types/src/marketplaces/jobs/PublishPluginToMarketplaceJob.ts @@ -16,7 +16,8 @@ export const PUBLISH_PLUGIN_TO_MARKETPLACE_QUEUE = /** * Ids only, which the worker re-loads so all the heavy lifting happens off the - * request thread. + * request thread — plus the package version the publish pinned, when package + * releases are enabled; without it the live package content is published. */ export interface PublishPluginToMarketplaceJobInput { marketplaceDistributionId: MarketplaceDistributionId; @@ -24,6 +25,7 @@ export interface PublishPluginToMarketplaceJobInput { packageId: PackageId; organizationId: OrganizationId; userId: UserId; + packageVersion?: string; } /**