diff --git a/apps/api/src/app/auth/auth.controller.spec.ts b/apps/api/src/app/auth/auth.controller.spec.ts index 3006211365..c6ea48abfa 100644 --- a/apps/api/src/app/auth/auth.controller.spec.ts +++ b/apps/api/src/app/auth/auth.controller.spec.ts @@ -354,22 +354,10 @@ describe('AuthController', () => { ); }); - it('throws an HttpException', async () => { - await expect( - controller.signIn(signInRequest, mockResponse), - ).rejects.toThrow(HttpException); - }); - - it('maps the error to HTTP 401', async () => { - await expect( - controller.signIn(signInRequest, mockResponse), - ).rejects.toMatchObject({ status: HttpStatus.UNAUTHORIZED }); - }); - - it('exposes the domain error message', async () => { + it('rethrows the domain error for the filter to answer 401', async () => { await expect( controller.signIn(signInRequest, mockResponse), - ).rejects.toThrow('Invalid email or password'); + ).rejects.toBeInstanceOf(InvalidEmailOrPasswordError); }); it('calls authService.signIn with the request', async () => { diff --git a/apps/api/src/app/auth/auth.controller.ts b/apps/api/src/app/auth/auth.controller.ts index ec119aff45..7f9f1bbd51 100644 --- a/apps/api/src/app/auth/auth.controller.ts +++ b/apps/api/src/app/auth/auth.controller.ts @@ -38,7 +38,6 @@ import { RequestPasswordResetCommand, RequestPasswordResetResponse, TooManyLoginAttemptsError, - ExpectedAuthError, CreateCliLoginCodeResponse, ExchangeCliLoginCodeCommand, ExchangeCliLoginCodeResponse, @@ -152,30 +151,22 @@ export class AuthController { return result; } catch (error) { - // Expected auth errors are legitimate user-facing outcomes (wrong - // password, rate limit reached) — not application bugs. Log them at - // warn level without stack trace so Datadog error dashboards stay - // focused on real incidents. - // Kept by hand: no kind yields 401, nor 429 with bannedUntil in the - // body, so DomainExceptionFilter cannot produce these answers. - if (error instanceof ExpectedAuthError) { + // Kept by hand: no kind answers 429 with bannedUntil in the body, so + // DomainExceptionFilter cannot produce this answer. Invalid credentials + // carry `unauthenticated` and are answered 401 by the filter. + if (error instanceof TooManyLoginAttemptsError) { this.logger.warn(`POST /auth/signin - ${error.name}`, { email: maskEmail(signInRequest.email), reason: error.message, }); - if (error instanceof TooManyLoginAttemptsError) { - throw new HttpException( - { - message: error.message, - bannedUntil: error.bannedUntil.toISOString(), - }, - HttpStatus.TOO_MANY_REQUESTS, - ); - } - - // InvalidEmailOrPasswordError (and future ExpectedAuthError subclasses) - throw new HttpException(error.message, HttpStatus.UNAUTHORIZED); + throw new HttpException( + { + message: error.message, + bannedUntil: error.bannedUntil.toISOString(), + }, + HttpStatus.TOO_MANY_REQUESTS, + ); } throw error; diff --git a/apps/cli/src/application/services/GitService.realGit.spec.ts b/apps/cli/src/application/services/GitService.realGit.spec.ts index 715184254a..5bc064ce55 100644 --- a/apps/cli/src/application/services/GitService.realGit.spec.ts +++ b/apps/cli/src/application/services/GitService.realGit.spec.ts @@ -19,9 +19,18 @@ describe('GitService against real git', () => { let repoPath: string; let service: GitService; + // A git hook exports GIT_DIR and friends; inherited, they point every git + // call here at the repository running the hook instead of the temp repo + // (`init --bare` then flips its core.bare). Jest sandboxes process.env, so + // the clean env has to be passed to each call. + const env = Object.fromEntries( + Object.entries(process.env).filter(([key]) => !key.startsWith('GIT_')), + ); + const git = (args: string, cwd: string = repoPath) => execSync(`git ${args}`, { cwd, + env, encoding: 'utf-8', stdio: ['pipe', 'pipe', 'pipe'], }); @@ -33,6 +42,7 @@ describe('GitService against real git', () => { root = mkdtempSync(path.join(tmpdir(), 'packmind-git-service-')); repoPath = path.join(root, 'repo'); execSync(`git init -q -b main "${repoPath}"`, { + env, stdio: ['pipe', 'pipe', 'pipe'], }); git('config user.email test@packmind.com'); @@ -41,7 +51,14 @@ describe('GitService against real git', () => { git('commit -q --allow-empty -m "initial commit"'); git('branch feature'); - service = new GitService(); + service = new GitService(undefined, (cmd, opts) => ({ + stdout: execSync(`git ${cmd}`, { + ...opts, + env, + encoding: 'utf-8', + stdio: ['pipe', 'pipe', 'pipe'], + }), + })); }); afterEach(() => { @@ -93,6 +110,7 @@ describe('GitService against real git', () => { it('reports the branch exists', () => { const remotePath = path.join(root, 'origin.git'); execSync(`git init -q --bare "${remotePath}"`, { + env, stdio: ['pipe', 'pipe', 'pipe'], }); git(`remote add origin "${remotePath}"`); diff --git a/packages/accounts/src/application/useCases/createInvitations/CreateInvitationsUseCase.spec.ts b/packages/accounts/src/application/useCases/createInvitations/CreateInvitationsUseCase.spec.ts index 4dfdb2bf50..12eb1fae0f 100644 --- a/packages/accounts/src/application/useCases/createInvitations/CreateInvitationsUseCase.spec.ts +++ b/packages/accounts/src/application/useCases/createInvitations/CreateInvitationsUseCase.spec.ts @@ -1,4 +1,7 @@ -import { UserNotFoundError } from '@packmind/node-utils'; +import { + MembershipOrganizationNotFoundError, + UserNotFoundError, +} from '@packmind/node-utils'; import { mockInterface, stubLogger, @@ -664,7 +667,7 @@ describe('CreateInvitationsUseCase', () => { }; await expect(useCase.execute(command)).rejects.toThrow( - `Organization ${organizationId} not found`, + MembershipOrganizationNotFoundError, ); }); diff --git a/packages/accounts/src/domain/errors/AccountsError.spec.ts b/packages/accounts/src/domain/errors/AccountsError.spec.ts index ba70c11785..8bfe37646f 100644 --- a/packages/accounts/src/domain/errors/AccountsError.spec.ts +++ b/packages/accounts/src/domain/errors/AccountsError.spec.ts @@ -18,6 +18,7 @@ import { CliLoginCodeUserNotFoundError } from './CliLoginCodeUserNotFoundError'; import { CliLoginCodeMembershipNotFoundError } from './CliLoginCodeMembershipNotFoundError'; import { CliLoginCodeOrganizationNotFoundError } from './CliLoginCodeOrganizationNotFoundError'; import { CliLoginCodeApiKeyError } from './CliLoginCodeApiKeyError'; +import { InvalidEmailOrPasswordError } from './InvalidEmailOrPasswordError'; describe('EmailAlreadyExistsError', () => { const error = new EmailAlreadyExistsError('test@example.com'); @@ -337,3 +338,23 @@ describe('CliLoginCodeApiKeyError', () => { expect(isDomainError(error)).toBe(false); }); }); + +describe('InvalidEmailOrPasswordError', () => { + const error = new InvalidEmailOrPasswordError(); + + it('is a domain error', () => { + expect(isDomainError(error)).toBe(true); + }); + + it('answers unauthenticated', () => { + expect(error.kind).toBe('unauthenticated'); + }); + + it('carries the invalid_credentials reason', () => { + expect(error.reason).toBe('invalid_credentials'); + }); + + it('keeps an empty context, so the log never names the account', () => { + expect(error.context).toEqual({}); + }); +}); diff --git a/packages/accounts/src/domain/errors/AccountsError.ts b/packages/accounts/src/domain/errors/AccountsError.ts index d4a91b5082..8448a80ec4 100644 --- a/packages/accounts/src/domain/errors/AccountsError.ts +++ b/packages/accounts/src/domain/errors/AccountsError.ts @@ -18,7 +18,8 @@ export type AccountsErrorReason = | 'user_cannot_change_own_role' | 'cannot_demote_last_admin' | 'invalid_authentication_type' - | 'user_creation_fields_required'; + | 'user_creation_fields_required' + | 'invalid_credentials'; export type AccountsErrorContext = { organizationId?: string; diff --git a/packages/accounts/src/domain/errors/ExpectedAuthError.ts b/packages/accounts/src/domain/errors/ExpectedAuthError.ts index b04f28a6b6..39fc1858eb 100644 --- a/packages/accounts/src/domain/errors/ExpectedAuthError.ts +++ b/packages/accounts/src/domain/errors/ExpectedAuthError.ts @@ -1,10 +1,9 @@ /** - * Base class for expected authentication errors. + * Base class for expected authentication errors that no `kind` can answer yet. * - * These represent legitimate user-facing outcomes (wrong password, rate limit - * reached, etc.) — NOT application bugs. Callers (e.g. NestJS controllers, - * exception filters) should log instances of this class at `warn` level - * without stack traces, and map them to the appropriate HTTP response. + * Only `TooManyLoginAttemptsError` is left: it must answer 429 with + * `bannedUntil` in the body, which `DomainExceptionFilter` cannot produce, so + * the sign-in controller still maps it by hand and logs it at `warn`. */ export abstract class ExpectedAuthError extends Error { protected constructor(message: string, name: string) { diff --git a/packages/accounts/src/domain/errors/InvalidEmailOrPasswordError.ts b/packages/accounts/src/domain/errors/InvalidEmailOrPasswordError.ts index fe4f96e82e..6e17cc215f 100644 --- a/packages/accounts/src/domain/errors/InvalidEmailOrPasswordError.ts +++ b/packages/accounts/src/domain/errors/InvalidEmailOrPasswordError.ts @@ -1,7 +1,17 @@ -import { ExpectedAuthError } from './ExpectedAuthError'; +import { AccountsError } from './AccountsError'; -export class InvalidEmailOrPasswordError extends ExpectedAuthError { +/** + * One error for an unknown email, a wrong password and a social account with + * no password, so the answer never tells which of the three it was. + */ +export class InvalidEmailOrPasswordError extends AccountsError { constructor() { - super('Invalid email or password', 'InvalidEmailOrPasswordError'); + super( + 'unauthenticated', + 'invalid_credentials', + {}, + 'Invalid email or password', + ); + this.name = 'InvalidEmailOrPasswordError'; } } diff --git a/packages/deployments/src/application/jobs/PublishArtifactsDelayedJob.ts b/packages/deployments/src/application/jobs/PublishArtifactsDelayedJob.ts index c6ffb568ab..3180ed39ab 100644 --- a/packages/deployments/src/application/jobs/PublishArtifactsDelayedJob.ts +++ b/packages/deployments/src/application/jobs/PublishArtifactsDelayedJob.ts @@ -7,7 +7,12 @@ import { SSEEventPublisher, WorkerListeners, } from '@packmind/node-utils'; -import { DistributionStatus, GitCommit, IGitPort } from '@packmind/types'; +import { + DistributionStatus, + GitCommit, + IGitPort, + NoChangesDetectedError, +} from '@packmind/types'; import { Job } from 'bullmq'; import { distributionIdsOf, @@ -91,7 +96,7 @@ export class PublishArtifactsDelayedJob extends AbstractAIDelayedJob< filesDeleted: input.fileUpdates.delete.length, }); } catch (error) { - if (error instanceof Error && error.message === 'NO_CHANGES_DETECTED') { + if (error instanceof NoChangesDetectedError) { this.logger.info( `[${this.origin}] No changes detected for distributions ${distributionIds.join(', ')}`, ); diff --git a/packages/deployments/src/application/useCases/RemovePackageFromTargetsUseCase.spec.ts b/packages/deployments/src/application/useCases/RemovePackageFromTargetsUseCase.spec.ts index 86653feb84..622ac706af 100644 --- a/packages/deployments/src/application/useCases/RemovePackageFromTargetsUseCase.spec.ts +++ b/packages/deployments/src/application/useCases/RemovePackageFromTargetsUseCase.spec.ts @@ -44,6 +44,7 @@ import { GitRepo, createGitProviderId, createGitCommitId, + NoChangesDetectedError, } from '@packmind/types'; describe('RemovePackageFromTargetsUseCase', () => { @@ -415,7 +416,7 @@ describe('RemovePackageFromTargetsUseCase', () => { beforeEach(() => { mockDistributionRepository.listByTargetIds.mockResolvedValue([]); mockGitPort.commitToGit.mockRejectedValue( - new Error('NO_CHANGES_DETECTED'), + new NoChangesDetectedError(), ); }); diff --git a/packages/deployments/src/application/useCases/RemovePackageFromTargetsUseCase.ts b/packages/deployments/src/application/useCases/RemovePackageFromTargetsUseCase.ts index e6071935f5..2346963ca8 100644 --- a/packages/deployments/src/application/useCases/RemovePackageFromTargetsUseCase.ts +++ b/packages/deployments/src/application/useCases/RemovePackageFromTargetsUseCase.ts @@ -32,6 +32,7 @@ import { FileUpdates, CodingAgent, RenderMode, + NoChangesDetectedError, } from '@packmind/types'; import { PackageService } from '../services/PackageService'; import { TargetService } from '../services/TargetService'; @@ -144,10 +145,7 @@ export class RemovePackageFromTargetsUseCase implements IRemovePackageFromTarget firstTargetData.fileUpdates.delete, ); } catch (error) { - if ( - error instanceof Error && - error.message === 'NO_CHANGES_DETECTED' - ) { + if (error instanceof NoChangesDetectedError) { this.logger.info('No changes detected for package removal', { repositoryId, packageSlug: pkg.slug, diff --git a/packages/git/src/application/useCases/commitToGit/CommitToGitUseCase.spec.ts b/packages/git/src/application/useCases/commitToGit/CommitToGitUseCase.spec.ts index 736c1dd895..4cde5a46da 100644 --- a/packages/git/src/application/useCases/commitToGit/CommitToGitUseCase.spec.ts +++ b/packages/git/src/application/useCases/commitToGit/CommitToGitUseCase.spec.ts @@ -10,6 +10,7 @@ import { GitProviderNotFoundError, GitProviderVendor, GitProviderVendors, + NoChangesDetectedError, NoFilesToCommitError, } from '@packmind/types'; import { IGitRepo } from '../../../domain/repositories/IGitRepo'; @@ -200,6 +201,40 @@ describe('CommitToGitUseCase', () => { ).rejects.toBeInstanceOf(NoFilesToCommitError); }); + describe('when the provider reports no changes', () => { + const commit = () => + commitToGit.commitToGit( + mockGitRepo, + [{ path: 'test/file1.txt', content: 'unchanged' }], + 'Commit message', + ); + + beforeEach(() => { + mockGitProviderRepository.findById.mockResolvedValue(mockGitProvider); + mockGithubRepository.commitFiles.mockResolvedValue({ + sha: 'no-changes', + message: 'Commit message', + author: 'test@example.com', + url: '', + }); + }); + + it('throws NoChangesDetectedError', async () => { + await expect(commit()).rejects.toBeInstanceOf(NoChangesDetectedError); + }); + + // Callers outside this repo still match on the message. + it('keeps the NO_CHANGES_DETECTED message', async () => { + await expect(commit()).rejects.toThrow(/^NO_CHANGES_DETECTED$/); + }); + + it('does not record a commit', async () => { + await commit().catch(() => undefined); + + expect(mockGitCommitService.addCommit).not.toHaveBeenCalled(); + }); + }); + describe('when deleteFiles parameter is provided', () => { it('passes deleteFiles parameter to commitFiles', async () => { const files = [{ path: 'test/file.txt', content: 'test content' }]; diff --git a/packages/git/src/application/useCases/commitToGit/CommitToGitUseCase.ts b/packages/git/src/application/useCases/commitToGit/CommitToGitUseCase.ts index 414ef57f0f..4147680eb8 100644 --- a/packages/git/src/application/useCases/commitToGit/CommitToGitUseCase.ts +++ b/packages/git/src/application/useCases/commitToGit/CommitToGitUseCase.ts @@ -4,6 +4,7 @@ import { GitCommit, DeleteItem, DeleteItemType, + NoChangesDetectedError, NoFilesToCommitError, } from '@packmind/types'; import { CommitFile } from '../../../domain/repositories/IGitRepo'; @@ -143,9 +144,11 @@ export class CommitToGitUseCase { repo: repo.repo, fileCount: files.length, }); - // Sentinel: deployment logic catches this rather than treating it as a - // failure. - throw new Error('NO_CHANGES_DETECTED'); + throw new NoChangesDetectedError({ + gitRepoId: repo.id, + owner: repo.owner, + repo: repo.repo, + }); } return this.gitCommitService.addCommit(commitData); diff --git a/packages/linter-ast/src/application/ConsoleLogRemovalService.spec.ts b/packages/linter-ast/src/application/ConsoleLogRemovalService.spec.ts index 94a985d568..cfe110d13c 100644 --- a/packages/linter-ast/src/application/ConsoleLogRemovalService.spec.ts +++ b/packages/linter-ast/src/application/ConsoleLogRemovalService.spec.ts @@ -1,5 +1,6 @@ import { ConsoleLogRemovalService } from './ConsoleLogRemovalService'; import { ProgrammingLanguage } from '@packmind/types'; +import { LinterAstInternalError } from '../core/LinterAstInternalError'; describe('ConsoleLogRemovalService', () => { let consoleLogRemovalService: ConsoleLogRemovalService; @@ -17,9 +18,7 @@ describe('ConsoleLogRemovalService', () => { program, ProgrammingLanguage.TYPESCRIPT, ), - ).rejects.toThrow( - 'ConsoleLogRemovalService only supports JAVASCRIPT, received: TYPESCRIPT', - ); + ).rejects.toBeInstanceOf(LinterAstInternalError); }); it('throws error for Python language', async () => { @@ -30,9 +29,7 @@ describe('ConsoleLogRemovalService', () => { program, ProgrammingLanguage.PYTHON, ), - ).rejects.toThrow( - 'ConsoleLogRemovalService only supports JAVASCRIPT, received: PYTHON', - ); + ).rejects.toBeInstanceOf(LinterAstInternalError); }); }); diff --git a/packages/linter-ast/src/application/ConsoleLogRemovalService.ts b/packages/linter-ast/src/application/ConsoleLogRemovalService.ts index 8d0b4f2360..00aea56c4f 100644 --- a/packages/linter-ast/src/application/ConsoleLogRemovalService.ts +++ b/packages/linter-ast/src/application/ConsoleLogRemovalService.ts @@ -1,5 +1,6 @@ import { ProgrammingLanguage } from '@packmind/types'; import JavaScriptParser from '../parsers/JavaScriptParser'; +import { LinterAstInternalError } from '../core/LinterAstInternalError'; export class ConsoleLogRemovalService { private readonly jsParser: JavaScriptParser; @@ -10,14 +11,16 @@ export class ConsoleLogRemovalService { /** * Removes console method-call statements from JavaScript source code using AST parsing. - * @throws Error if language is not JAVASCRIPT + * @throws LinterAstInternalError if language is not JAVASCRIPT */ async removeConsoleLogStatements( sourceCode: string, language: ProgrammingLanguage, ): Promise { if (language !== ProgrammingLanguage.JAVASCRIPT) { - throw new Error( + throw new LinterAstInternalError( + 'console_removal_language_unsupported', + { language }, `ConsoleLogRemovalService only supports JAVASCRIPT, received: ${language}`, ); } @@ -81,7 +84,11 @@ export class ConsoleLogRemovalService { return cleaned; } catch (error) { - throw new Error(`Can not parse JS CODE ${error}`); + throw new LinterAstInternalError( + 'console_removal_parse_failed', + { language, cause: String(error) }, + `Can not parse JS CODE ${error}`, + ); } } } diff --git a/packages/linter-ast/src/application/LinterAstAdapter.spec.ts b/packages/linter-ast/src/application/LinterAstAdapter.spec.ts index bb6ea7b6b0..85a71f28b6 100644 --- a/packages/linter-ast/src/application/LinterAstAdapter.spec.ts +++ b/packages/linter-ast/src/application/LinterAstAdapter.spec.ts @@ -102,5 +102,14 @@ describe('LinterAstAdapter', () => { adapter.parseSourceCode('code', 'GENERIC' as ProgrammingLanguage), ).rejects.toThrow(ParserNotAvailableError); }); + + it('throws an internal error', async () => { + await expect( + adapter.parseSourceCode('code', 'GENERIC' as ProgrammingLanguage), + ).rejects.toMatchObject({ + kind: 'internal', + reason: 'parser_not_available', + }); + }); }); }); diff --git a/packages/linter-ast/src/core/LinterAstInternalError.ts b/packages/linter-ast/src/core/LinterAstInternalError.ts new file mode 100644 index 0000000000..68b8e4de82 --- /dev/null +++ b/packages/linter-ast/src/core/LinterAstInternalError.ts @@ -0,0 +1,26 @@ +import { PackmindInternalError } from '@packmind/types'; + +export type LinterAstInternalErrorReason = + | 'parser_not_available' + | 'console_removal_language_unsupported' + | 'console_removal_parse_failed'; + +export type LinterAstInternalErrorContext = { + language?: string; + cause?: string; +}; + +/** + * Base for the linter-ast broken invariants. Callers only ever ask for a + * language they checked first, so a miss here is our bug, not the caller's. + */ +export class LinterAstInternalError extends PackmindInternalError { + constructor( + reason: LinterAstInternalErrorReason, + context: LinterAstInternalErrorContext, + message: string, + ) { + super(reason, context, message); + this.name = 'LinterAstInternalError'; + } +} diff --git a/packages/linter-ast/src/core/ParserError.ts b/packages/linter-ast/src/core/ParserError.ts index 934a54f64f..4a2d63260f 100644 --- a/packages/linter-ast/src/core/ParserError.ts +++ b/packages/linter-ast/src/core/ParserError.ts @@ -1,8 +1,14 @@ -export class ParserNotAvailableError extends Error { +import { LinterAstInternalError } from './LinterAstInternalError'; + +export class ParserNotAvailableError extends LinterAstInternalError { public readonly originalError?: Error; constructor(language: string, cause?: Error) { - super(`Parser for ${language} not available`); + super( + 'parser_not_available', + { language }, + `Parser for ${language} not available`, + ); this.name = 'ParserNotAvailableError'; this.originalError = cause; } diff --git a/packages/linter-ast/src/index.ts b/packages/linter-ast/src/index.ts index 93e99c456d..7609864600 100644 --- a/packages/linter-ast/src/index.ts +++ b/packages/linter-ast/src/index.ts @@ -4,6 +4,7 @@ export { ParserNotAvailableError, ParserInitializationError, } from './core/ParserError'; +export { LinterAstInternalError } from './core/LinterAstInternalError'; export type { ASTNode } from './core/types/ast.types'; export { LinterAstAdapter } from './application/LinterAstAdapter'; diff --git a/packages/linter-execution/src/application/useCases/ExecuteLinterProgramsUseCase.ts b/packages/linter-execution/src/application/useCases/ExecuteLinterProgramsUseCase.ts index a839c511ce..7a765e5fbb 100644 --- a/packages/linter-execution/src/application/useCases/ExecuteLinterProgramsUseCase.ts +++ b/packages/linter-execution/src/application/useCases/ExecuteLinterProgramsUseCase.ts @@ -9,6 +9,7 @@ import { LinterExecutionViolation, } from '@packmind/types'; import { PackmindLogger } from '@packmind/logger'; +import { LinterExecutionInternalError } from '../../domain/errors/LinterExecutionInternalError'; const origin = 'ExecuteLinterProgramsUseCase'; @@ -160,7 +161,12 @@ export class ExecuteLinterProgramsUseCase implements IExecuteLinterProgramsUseCa ); return func as (input: ASTNode | string) => unknown[] | number[]; } catch (error) { - throw new Error(`Failed to parse program: ${this.normalizeError(error)}`); + const cause = this.normalizeError(error); + throw new LinterExecutionInternalError( + 'program_parse_failed', + { cause }, + `Failed to parse program: ${cause}`, + ); } } diff --git a/packages/linter-execution/src/domain/errors/LinterExecutionInternalError.ts b/packages/linter-execution/src/domain/errors/LinterExecutionInternalError.ts new file mode 100644 index 0000000000..4f80863de3 --- /dev/null +++ b/packages/linter-execution/src/domain/errors/LinterExecutionInternalError.ts @@ -0,0 +1,18 @@ +import { PackmindInternalError } from '@packmind/types'; + +export type LinterExecutionInternalErrorReason = 'program_parse_failed'; + +export type LinterExecutionInternalErrorContext = { + cause?: string; +}; + +export class LinterExecutionInternalError extends PackmindInternalError { + constructor( + reason: LinterExecutionInternalErrorReason, + context: LinterExecutionInternalErrorContext, + message: string, + ) { + super(reason, context, message); + this.name = 'LinterExecutionInternalError'; + } +} diff --git a/packages/node-utils/src/application/AbstractAdminUseCase.ts b/packages/node-utils/src/application/AbstractAdminUseCase.ts index 1e73f9aee7..3e673da953 100644 --- a/packages/node-utils/src/application/AbstractAdminUseCase.ts +++ b/packages/node-utils/src/application/AbstractAdminUseCase.ts @@ -8,6 +8,7 @@ import { AbstractMemberUseCase, MemberContext } from './AbstractMemberUseCase'; import { OrganizationAdminRequiredError, UserAccessError, + UserAccessInternalError, } from './UserAccessErrors'; const defaultOrigin = 'AbstractAdminUseCase'; @@ -34,7 +35,7 @@ export abstract class AbstractAdminUseCase< protected override handleValidationError( error: UserAccessError, command: Command, - ): Error | never { + ): UserAccessError { this.logger.error('Admin validation failed', { userId: command.userId, organizationId: command.organizationId, @@ -44,7 +45,10 @@ export abstract class AbstractAdminUseCase< if (error.reason === 'user_not_an_admin') { const organizationId = error.context.organizationId; if (!organizationId) { - throw new Error( + // Unreachable: membership lookup has already failed without an id. + throw new UserAccessInternalError( + 'organization_id_missing', + { userId: error.context.userId }, 'Organization ID is required for admin access operations', ); } diff --git a/packages/node-utils/src/application/AbstractMemberUseCase.spec.ts b/packages/node-utils/src/application/AbstractMemberUseCase.spec.ts index 13889a5e2c..fb5e8f07bd 100644 --- a/packages/node-utils/src/application/AbstractMemberUseCase.spec.ts +++ b/packages/node-utils/src/application/AbstractMemberUseCase.spec.ts @@ -11,6 +11,7 @@ import { } from '@packmind/types'; import { AbstractMemberUseCase, MemberContext } from './AbstractMemberUseCase'; import { + MembershipOrganizationNotFoundError, UserNotFoundError, UserNotInOrganizationError, } from './UserAccessErrors'; @@ -230,4 +231,36 @@ describe('AbstractMemberUseCase', () => { expect(mockExecuteForMembers).not.toHaveBeenCalled(); }); }); + + describe('when the membership exists but the organization is gone', () => { + beforeEach(() => { + mockGetUserById.mockResolvedValue(buildUser()); + mockGetOrganizationById.mockResolvedValue(null); + }); + + it('throws MembershipOrganizationNotFoundError', async () => { + await expect(useCase.execute(command)).rejects.toBeInstanceOf( + MembershipOrganizationNotFoundError, + ); + }); + + it('keeps the ids in the context', async () => { + await expect(useCase.execute(command)).rejects.toMatchObject({ + kind: 'not_found', + context: { userId, organizationId }, + }); + }); + }); + + describe('when the command names no organization', () => { + beforeEach(() => { + mockGetUserById.mockResolvedValue(buildUser()); + }); + + it('throws UserNotInOrganizationError', async () => { + await expect( + useCase.execute({ userId, organizationId: '' }), + ).rejects.toBeInstanceOf(UserNotInOrganizationError); + }); + }); }); diff --git a/packages/node-utils/src/application/AbstractMemberUseCase.ts b/packages/node-utils/src/application/AbstractMemberUseCase.ts index 90ce0b7f00..7c757bd2e8 100644 --- a/packages/node-utils/src/application/AbstractMemberUseCase.ts +++ b/packages/node-utils/src/application/AbstractMemberUseCase.ts @@ -14,6 +14,7 @@ import { UserOrganizationMembership, } from '@packmind/types'; import { + MembershipOrganizationNotFoundError, OrganizationContext, UserAccessError, UserAccessErrorContext, @@ -116,7 +117,7 @@ export abstract class AbstractMemberUseCase< protected handleValidationError( error: UserAccessError, command: Command, - ): Error | never { + ): UserAccessError { this.logger.error('Member validation failed', { userId: command.userId, organizationId: command.organizationId, @@ -130,16 +131,14 @@ export abstract class AbstractMemberUseCase< command: Command & MemberContext, ): Promise; - private translateUserAccessError(error: UserAccessError): Error { + private translateUserAccessError(error: UserAccessError): UserAccessError { const { userId, organizationId } = error.context; switch (error.reason) { case 'user_not_found': return new UserNotFoundError({ userId, organizationId }); case 'user_not_in_organization': - return new UserNotInOrganizationError( - this.toOrganizationContext({ userId, organizationId }), - ); + return new UserNotInOrganizationError({ userId, organizationId }); default: return error; } @@ -157,19 +156,23 @@ export abstract class AbstractMemberUseCase< const organizationId = createOrganizationId(command.organizationId); const membership = this.findMembership(user, organizationId, context); - const organization = await this.fetchOrganization(organizationId); + const organization = await this.fetchOrganization(organizationId, { + userId: command.userId, + organizationId, + }); return { user, organization, membership }; } private async fetchOrganization( organizationId: OrganizationId, + context: OrganizationContext, ): Promise { const organization = await this.accountsPort.getOrganizationById(organizationId); if (!organization) { - throw new Error(`Organization ${organizationId} not found`); + throw new MembershipOrganizationNotFoundError(context); } return organization; @@ -199,24 +202,9 @@ export abstract class AbstractMemberUseCase< ); if (!membership) { - throw new UserNotInOrganizationError(this.toOrganizationContext(context)); + throw new UserNotInOrganizationError(context); } return membership; } - - private toOrganizationContext( - context: UserAccessErrorContext, - ): OrganizationContext { - if (!context.organizationId) { - throw new Error( - 'Organization ID is required for member access operations', - ); - } - - return { - userId: context.userId, - organizationId: context.organizationId, - }; - } } diff --git a/packages/node-utils/src/application/UserAccessErrors.spec.ts b/packages/node-utils/src/application/UserAccessErrors.spec.ts index 85a2324857..fa9a5f1002 100644 --- a/packages/node-utils/src/application/UserAccessErrors.spec.ts +++ b/packages/node-utils/src/application/UserAccessErrors.spec.ts @@ -2,13 +2,16 @@ import { createOrganizationId, createUserId, isDomainError, + isInternalError, } from '@packmind/types'; import { SpaceAdminRequiredError } from './AbstractSpaceAdminUseCase'; import { SpaceMembershipRequiredError } from './AbstractSpaceMemberUseCase'; import { + MembershipOrganizationNotFoundError, OrganizationAdminRequiredError, UserAccessError, UserNotFoundError, + UserAccessInternalError, UserNotInOrganizationError, } from './UserAccessErrors'; @@ -56,6 +59,45 @@ describe('UserAccessError kinds', () => { }); }); + describe('when a MembershipOrganizationNotFoundError is constructed', () => { + const error = new MembershipOrganizationNotFoundError({ + userId, + organizationId, + }); + + it('exposes the not_found kind', () => { + expect(error.kind).toBe('not_found'); + }); + + it('exposes its reason', () => { + expect(error.reason).toBe('organization_not_found'); + }); + + it('keeps the organization id out of the message', () => { + expect(error.message).not.toContain(organizationId); + }); + + it('satisfies the DomainError guard', () => { + expect(isDomainError(error)).toBe(true); + }); + }); + + describe('when a UserAccessInternalError is constructed', () => { + const error = new UserAccessInternalError( + 'organization_id_missing', + { userId }, + 'Organization ID is required for admin access operations', + ); + + it('satisfies the InternalError guard', () => { + expect(isInternalError(error)).toBe(true); + }); + + it('is not a domain error', () => { + expect(isDomainError(error)).toBe(false); + }); + }); + describe('when an OrganizationAdminRequiredError is constructed', () => { const error = new OrganizationAdminRequiredError({ userId, diff --git a/packages/node-utils/src/application/UserAccessErrors.ts b/packages/node-utils/src/application/UserAccessErrors.ts index 866f51b9f4..a9327e8b51 100644 --- a/packages/node-utils/src/application/UserAccessErrors.ts +++ b/packages/node-utils/src/application/UserAccessErrors.ts @@ -1,8 +1,14 @@ -import { DomainError, DomainErrorKind, PackmindCommand } from '@packmind/types'; +import { + DomainError, + DomainErrorKind, + PackmindCommand, + PackmindInternalError, +} from '@packmind/types'; export type UserAccessErrorReason = | 'user_not_found' | 'user_not_in_organization' + | 'organization_not_found' | 'user_not_an_admin' | 'space_membership_required' | 'space_admin_required'; @@ -49,8 +55,13 @@ export class UserNotFoundError extends UserAccessError { } } +/** + * Takes the base context, organization id optional: a command naming no + * organization is still a caller outside the one it asked for, and the answer + * stays the 403 rather than an invariant failure. + */ export class UserNotInOrganizationError extends UserAccessError { - constructor(context: OrganizationContext) { + constructor(context: UserAccessErrorContext) { super( 'forbidden', 'user_not_in_organization', @@ -72,3 +83,32 @@ export class OrganizationAdminRequiredError extends UserAccessError { this.name = 'OrganizationAdminRequiredError'; } } + +/** + * The caller holds a membership whose organization row is gone. Answered 404 + * rather than 500: there is nothing the caller can reach there. + */ +export class MembershipOrganizationNotFoundError extends UserAccessError { + constructor(context: OrganizationContext) { + super( + 'not_found', + 'organization_not_found', + context, + 'The organization could not be found.', + ); + this.name = 'MembershipOrganizationNotFoundError'; + } +} + +export type UserAccessInternalErrorReason = 'organization_id_missing'; + +export class UserAccessInternalError extends PackmindInternalError { + constructor( + reason: UserAccessInternalErrorReason, + context: UserAccessErrorContext, + message: string, + ) { + super(reason, context, message); + this.name = 'UserAccessInternalError'; + } +} diff --git a/packages/node-utils/src/nest/filters/DomainExceptionFilter.spec.ts b/packages/node-utils/src/nest/filters/DomainExceptionFilter.spec.ts index 0c8455534d..559f9cae4f 100644 --- a/packages/node-utils/src/nest/filters/DomainExceptionFilter.spec.ts +++ b/packages/node-utils/src/nest/filters/DomainExceptionFilter.spec.ts @@ -217,6 +217,31 @@ describe('DomainExceptionFilter', () => { }); }); + describe('when the exception is an unauthenticated domain error', () => { + beforeEach(() => { + filter.catch( + new TestDomainError( + 'unauthenticated', + 'invalid_credentials', + 'Invalid email or password', + ), + host, + ); + }); + + it('responds with 401', () => { + expect(capturedStatus()).toBe(HttpStatus.UNAUTHORIZED); + }); + + it('returns the message and reason to the caller', () => { + expect(capturedBody()).toEqual({ + statusCode: 401, + message: 'Invalid email or password', + reason: 'invalid_credentials', + }); + }); + }); + // The policy table, stated as behaviour: a new kind added to the union // without a row here fails to compile, and a row given the wrong status // fails here. @@ -225,6 +250,7 @@ describe('DomainExceptionFilter', () => { ['not_found', HttpStatus.NOT_FOUND], ['invalid_input', HttpStatus.BAD_REQUEST], ['conflict', HttpStatus.CONFLICT], + ['unauthenticated', HttpStatus.UNAUTHORIZED], ] satisfies ReadonlyArray<[DomainErrorKind, number]>)( 'when the domain error kind is %s', (kind, expectedStatus) => { diff --git a/packages/node-utils/src/nest/filters/DomainExceptionFilter.ts b/packages/node-utils/src/nest/filters/DomainExceptionFilter.ts index 5553636206..73d313885c 100644 --- a/packages/node-utils/src/nest/filters/DomainExceptionFilter.ts +++ b/packages/node-utils/src/nest/filters/DomainExceptionFilter.ts @@ -40,6 +40,7 @@ const KIND_POLICY: Record = { not_found: { status: HttpStatus.NOT_FOUND, logLevel: 'warn' }, invalid_input: { status: HttpStatus.BAD_REQUEST, logLevel: 'warn' }, conflict: { status: HttpStatus.CONFLICT, logLevel: 'warn' }, + unauthenticated: { status: HttpStatus.UNAUTHORIZED, logLevel: 'warn' }, }; /** diff --git a/packages/skills/src/application/useCases/createSkill/CreateSkillUseCase.spec.ts b/packages/skills/src/application/useCases/createSkill/CreateSkillUseCase.spec.ts index 547515309e..8071949fe6 100644 --- a/packages/skills/src/application/useCases/createSkill/CreateSkillUseCase.spec.ts +++ b/packages/skills/src/application/useCases/createSkill/CreateSkillUseCase.spec.ts @@ -6,6 +6,7 @@ import { SpaceMembershipRequiredError, UserNotFoundError, UserNotInOrganizationError, + MembershipOrganizationNotFoundError, } from '@packmind/node-utils'; import { mockInterface, @@ -546,7 +547,7 @@ describe('CreateSkillUseCase', () => { it('throws error', async () => { await expect(usecase.execute(command)).rejects.toThrow( - `Organization ${organizationId} not found`, + MembershipOrganizationNotFoundError, ); }); diff --git a/packages/skills/src/application/useCases/deleteSkillsBatch/DeleteSkillsBatchUseCase.spec.ts b/packages/skills/src/application/useCases/deleteSkillsBatch/DeleteSkillsBatchUseCase.spec.ts index 953f497ba2..d93b53e112 100644 --- a/packages/skills/src/application/useCases/deleteSkillsBatch/DeleteSkillsBatchUseCase.spec.ts +++ b/packages/skills/src/application/useCases/deleteSkillsBatch/DeleteSkillsBatchUseCase.spec.ts @@ -6,6 +6,7 @@ import { SpaceMembershipRequiredError, UserNotFoundError, UserNotInOrganizationError, + MembershipOrganizationNotFoundError, } from '@packmind/node-utils'; import { mockInterface, @@ -498,7 +499,7 @@ describe('DeleteSkillsBatchUseCase', () => { it('throws error', async () => { await expect(usecase.execute(command)).rejects.toThrow( - `Organization ${organizationId} not found`, + MembershipOrganizationNotFoundError, ); }); diff --git a/packages/skills/src/application/useCases/findSkillBySlug/FindSkillBySlugUseCase.spec.ts b/packages/skills/src/application/useCases/findSkillBySlug/FindSkillBySlugUseCase.spec.ts index af5179cdd5..9f570f5400 100644 --- a/packages/skills/src/application/useCases/findSkillBySlug/FindSkillBySlugUseCase.spec.ts +++ b/packages/skills/src/application/useCases/findSkillBySlug/FindSkillBySlugUseCase.spec.ts @@ -1,6 +1,7 @@ import { UserNotFoundError, UserNotInOrganizationError, + MembershipOrganizationNotFoundError, } from '@packmind/node-utils'; import { PackmindLogger } from '@packmind/logger'; import { userFactory } from '@packmind/accounts/test'; @@ -403,7 +404,7 @@ describe('FindSkillBySlugUseCase', () => { it('throws error', async () => { await expect(usecase.execute(command)).rejects.toThrow( - `Organization ${organizationId} not found`, + MembershipOrganizationNotFoundError, ); }); }); diff --git a/packages/skills/src/application/useCases/getSkillById/GetSkillByIdUseCase.spec.ts b/packages/skills/src/application/useCases/getSkillById/GetSkillByIdUseCase.spec.ts index a0a356c340..8eba053240 100644 --- a/packages/skills/src/application/useCases/getSkillById/GetSkillByIdUseCase.spec.ts +++ b/packages/skills/src/application/useCases/getSkillById/GetSkillByIdUseCase.spec.ts @@ -5,6 +5,7 @@ import { SpaceMembershipRequiredError, UserNotFoundError, UserNotInOrganizationError, + MembershipOrganizationNotFoundError, } from '@packmind/node-utils'; import { mockInterface, @@ -393,7 +394,7 @@ describe('GetSkillByIdUseCase', () => { it('throws error', async () => { await expect(usecase.execute(command)).rejects.toThrow( - `Organization ${organizationId} not found`, + MembershipOrganizationNotFoundError, ); }); }); diff --git a/packages/skills/src/application/useCases/getSkillWithFiles/GetSkillWithFilesUseCase.spec.ts b/packages/skills/src/application/useCases/getSkillWithFiles/GetSkillWithFilesUseCase.spec.ts index 96a2f28389..d64282eb8f 100644 --- a/packages/skills/src/application/useCases/getSkillWithFiles/GetSkillWithFilesUseCase.spec.ts +++ b/packages/skills/src/application/useCases/getSkillWithFiles/GetSkillWithFilesUseCase.spec.ts @@ -1,6 +1,7 @@ import { UserNotFoundError, UserNotInOrganizationError, + MembershipOrganizationNotFoundError, } from '@packmind/node-utils'; import { userFactory } from '@packmind/accounts/test'; import { @@ -344,7 +345,7 @@ describe('GetSkillWithFilesUseCase', () => { it('throws error', async () => { await expect(usecase.execute(command)).rejects.toThrow( - `Organization ${organizationId} not found`, + MembershipOrganizationNotFoundError, ); }); }); diff --git a/packages/skills/src/application/useCases/listSkillVersions/ListSkillVersionsUseCase.spec.ts b/packages/skills/src/application/useCases/listSkillVersions/ListSkillVersionsUseCase.spec.ts index fa34c2114d..fca29b0242 100644 --- a/packages/skills/src/application/useCases/listSkillVersions/ListSkillVersionsUseCase.spec.ts +++ b/packages/skills/src/application/useCases/listSkillVersions/ListSkillVersionsUseCase.spec.ts @@ -1,6 +1,7 @@ import { UserNotFoundError, UserNotInOrganizationError, + MembershipOrganizationNotFoundError, } from '@packmind/node-utils'; import { userFactory } from '@packmind/accounts/test'; import { @@ -266,7 +267,7 @@ describe('ListSkillVersionsUseCase', () => { it('throws organization not found error', async () => { await expect(usecase.execute(command)).rejects.toThrow( - `Organization ${organizationId} not found`, + MembershipOrganizationNotFoundError, ); }); }); diff --git a/packages/skills/src/application/useCases/saveSkillVersion/SaveSkillVersionUseCase.spec.ts b/packages/skills/src/application/useCases/saveSkillVersion/SaveSkillVersionUseCase.spec.ts index d623de5442..cbcaad6157 100644 --- a/packages/skills/src/application/useCases/saveSkillVersion/SaveSkillVersionUseCase.spec.ts +++ b/packages/skills/src/application/useCases/saveSkillVersion/SaveSkillVersionUseCase.spec.ts @@ -2,6 +2,7 @@ import { PackmindEventEmitterService, UserNotFoundError, UserNotInOrganizationError, + MembershipOrganizationNotFoundError, } from '@packmind/node-utils'; import { PackmindLogger } from '@packmind/logger'; import { organizationFactory, userFactory } from '@packmind/accounts/test'; @@ -625,7 +626,7 @@ describe('SaveSkillVersionUseCase', () => { it('throws error', async () => { await expect(usecase.execute(command)).rejects.toThrow( - `Organization ${organizationId} not found`, + MembershipOrganizationNotFoundError, ); }); }); diff --git a/packages/standards/src/application/useCases/getStandardById/GetStandardByIdUseCase.spec.ts b/packages/standards/src/application/useCases/getStandardById/GetStandardByIdUseCase.spec.ts index 854fd5dd16..cb2bab64f5 100644 --- a/packages/standards/src/application/useCases/getStandardById/GetStandardByIdUseCase.spec.ts +++ b/packages/standards/src/application/useCases/getStandardById/GetStandardByIdUseCase.spec.ts @@ -3,6 +3,7 @@ import { SpaceMembershipRequiredError, UserNotFoundError, UserNotInOrganizationError, + MembershipOrganizationNotFoundError, } from '@packmind/node-utils'; import { mockInterface, @@ -466,7 +467,7 @@ describe('GetStandardByIdUseCase', () => { accountsAdapter.getOrganizationById.mockResolvedValue(null); await expect(usecase.execute(command)).rejects.toThrow( - `Organization ${organizationId} not found`, + MembershipOrganizationNotFoundError, ); }); diff --git a/packages/types/src/errors/DomainError.spec.ts b/packages/types/src/errors/DomainError.spec.ts index a76897d9a9..81d7082dd3 100644 --- a/packages/types/src/errors/DomainError.spec.ts +++ b/packages/types/src/errors/DomainError.spec.ts @@ -2,15 +2,18 @@ import { isDomainError } from './DomainError'; describe('isDomainError', () => { describe('when the value has a valid kind and reason string', () => { - describe.each(['forbidden', 'not_found', 'invalid_input', 'conflict'])( - 'and the kind is %s', - (kind) => { - it('returns true', () => { - const value = { kind, reason: 'example reason' }; - expect(isDomainError(value)).toBe(true); - }); - }, - ); + describe.each([ + 'forbidden', + 'not_found', + 'invalid_input', + 'conflict', + 'unauthenticated', + ])('and the kind is %s', (kind) => { + it('returns true', () => { + const value = { kind, reason: 'example reason' }; + expect(isDomainError(value)).toBe(true); + }); + }); }); describe('when the value is null', () => { diff --git a/packages/types/src/errors/DomainError.ts b/packages/types/src/errors/DomainError.ts index 3acf58014e..e9e12e04ed 100644 --- a/packages/types/src/errors/DomainError.ts +++ b/packages/types/src/errors/DomainError.ts @@ -16,12 +16,16 @@ * - `conflict`: the command is well-formed, but the current state of the * resource forbids it — a duplicate, or an operation the resource's role * rules out. + * - `unauthenticated`: the caller has not proven who they are — credentials + * that do not match. Distinct from `forbidden`, where the caller is known + * and their rights are the subject. */ export type DomainErrorKind = | 'forbidden' | 'not_found' | 'invalid_input' - | 'conflict'; + | 'conflict' + | 'unauthenticated'; export interface DomainError { readonly kind: DomainErrorKind; @@ -33,6 +37,7 @@ const VALID_KINDS = [ 'not_found', 'invalid_input', 'conflict', + 'unauthenticated', ] as const satisfies readonly DomainErrorKind[]; export function isDomainError(value: unknown): value is DomainError { diff --git a/packages/types/src/git/errors/GitError.ts b/packages/types/src/git/errors/GitError.ts index 7d1dd89920..22753b6ff9 100644 --- a/packages/types/src/git/errors/GitError.ts +++ b/packages/types/src/git/errors/GitError.ts @@ -22,7 +22,8 @@ export type GitErrorReason = | 'git_repo_already_linked_as_standard' | 'invalid_install_state' | 'git_remote_access_forbidden' - | 'git_remote_repository_not_found'; + | 'git_remote_repository_not_found' + | 'no_changes_detected'; export type GitErrorContext = { organizationId?: string; diff --git a/packages/types/src/git/errors/NoChangesDetectedError.ts b/packages/types/src/git/errors/NoChangesDetectedError.ts new file mode 100644 index 0000000000..2824d10a95 --- /dev/null +++ b/packages/types/src/git/errors/NoChangesDetectedError.ts @@ -0,0 +1,16 @@ +import { GitError, GitErrorContext } from './GitError'; + +/** + * The provider accepted the commit but had nothing to write: the files already + * match. Deployment flows catch it and record `no_changes` rather than a + * failure. + * + * The message is the old sentinel string on purpose: callers outside this + * repo still match on `error.message === 'NO_CHANGES_DETECTED'`. + */ +export class NoChangesDetectedError extends GitError { + constructor(context: GitErrorContext = {}) { + super('conflict', 'no_changes_detected', context, 'NO_CHANGES_DETECTED'); + this.name = 'NoChangesDetectedError'; + } +} diff --git a/packages/types/src/git/errors/index.ts b/packages/types/src/git/errors/index.ts index 71237804d9..01fc2d25ab 100644 --- a/packages/types/src/git/errors/index.ts +++ b/packages/types/src/git/errors/index.ts @@ -14,6 +14,7 @@ export * from './GitRemoteAccessForbiddenError'; export * from './GitRemoteRepositoryNotFoundError'; export * from './InvalidGitProviderCredentialsError'; export * from './MissingGitInputError'; +export * from './NoChangesDetectedError'; export * from './NoFilesToCommitError'; export * from './NoTrackedRepositoryError'; export * from './RepositoryAlreadyTrackedError';