From 3f7cc66144d284ff56cdd7e6ee383bed5f86d60e Mon Sep 17 00:00:00 2001 From: Malo Date: Thu, 1 Oct 2026 16:13:53 +0200 Subject: [PATCH 1/7] =?UTF-8?q?=E2=9C=A8=20feat(types):=20add=20an=20unaut?= =?UTF-8?q?henticated=20domain=20error=20kind=20answering=20401?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Sign-in failures are the caller's fault and safe to show, but no kind answered 401, so the auth controller hand-mapped them to an HttpException. A dedicated kind lets the filter answer them centrally, distinct from forbidden where the caller is known and their rights are the subject. Refs #862 Co-Authored-By: Claude Opus 5.5 --- .../filters/DomainExceptionFilter.spec.ts | 33 +++++++++++++++++++ .../src/nest/filters/DomainExceptionFilter.ts | 1 + packages/types/src/errors/DomainError.spec.ts | 21 +++++++----- packages/types/src/errors/DomainError.ts | 7 +++- 4 files changed, 52 insertions(+), 10 deletions(-) diff --git a/packages/node-utils/src/nest/filters/DomainExceptionFilter.spec.ts b/packages/node-utils/src/nest/filters/DomainExceptionFilter.spec.ts index 0c8455534d..f2f3cd79a7 100644 --- a/packages/node-utils/src/nest/filters/DomainExceptionFilter.spec.ts +++ b/packages/node-utils/src/nest/filters/DomainExceptionFilter.spec.ts @@ -217,6 +217,38 @@ 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', + }); + }); + + it('logs at warn', () => { + expect(logger.warn).toHaveBeenCalledWith( + 'Domain error mapped to HTTP response', + expect.objectContaining({ kind: 'unauthenticated', statusCode: 401 }), + ); + }); + }); + // 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 +257,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/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 { From 25dfe4d3876392068380f09fdae9e947febe4a63 Mon Sep 17 00:00:00 2001 From: Malo Date: Thu, 1 Oct 2026 16:15:55 +0200 Subject: [PATCH 2/7] =?UTF-8?q?=E2=99=BB=EF=B8=8F=20refactor(accounts):=20?= =?UTF-8?q?answer=20invalid=20sign-in=20credentials=20through=20the=20erro?= =?UTF-8?q?r=20filter?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit InvalidEmailOrPasswordError was kindless, so POST /auth/signin mapped it to a 401 HttpException by hand. It now joins the accounts error family with kind 'unauthenticated' and reason 'invalid_credentials', and the controller rethrows it for DomainExceptionFilter to answer 401. The body gains a reason; message stays the same, which is what the sign-in form shows. TooManyLoginAttemptsError keeps its hand-mapped 429: no kind answers with bannedUntil in the body yet. Refs #862 Co-Authored-By: Claude Opus 5.5 --- apps/api/src/app/auth/auth.controller.spec.ts | 16 ++-------- apps/api/src/app/auth/auth.controller.ts | 31 +++++++------------ .../src/domain/errors/AccountsError.spec.ts | 21 +++++++++++++ .../src/domain/errors/AccountsError.ts | 3 +- .../src/domain/errors/ExpectedAuthError.ts | 9 +++--- .../errors/InvalidEmailOrPasswordError.ts | 16 ++++++++-- 6 files changed, 53 insertions(+), 43 deletions(-) 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/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'; } } From 54ee21bc928ac7636a42b3460484821066b2e4aa Mon Sep 17 00:00:00 2001 From: Malo Date: Thu, 1 Oct 2026 16:18:14 +0200 Subject: [PATCH 3/7] =?UTF-8?q?=E2=99=BB=EF=B8=8F=20refactor(node-utils):?= =?UTF-8?q?=20make=20member=20and=20admin=20base=20use=20cases=20throw=20k?= =?UTF-8?q?inded=20errors?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Every member and admin use case runs these checks, so a bare Error here answered 500 for any of them: - a membership whose organization row is gone now throws MembershipOrganizationNotFoundError (not_found, organization_not_found) with the ids in context; - a command with no organization id now answers the real UserNotInOrganizationError 403, whose constructor accepts an optional organization id, instead of a bare Error masking it; - the unreachable admin branch throws UserAccessInternalError. handleValidationError now returns UserAccessError, so its throw site is typed as kind-carrying. Refs #862 Co-Authored-By: Claude Opus 5.5 --- .../CreateInvitationsUseCase.spec.ts | 7 ++- .../src/application/AbstractAdminUseCase.ts | 8 +++- .../application/AbstractMemberUseCase.spec.ts | 33 ++++++++++++++ .../src/application/AbstractMemberUseCase.ts | 34 +++++--------- .../src/application/UserAccessErrors.spec.ts | 42 ++++++++++++++++++ .../src/application/UserAccessErrors.ts | 44 ++++++++++++++++++- .../createSkill/CreateSkillUseCase.spec.ts | 3 +- .../DeleteSkillsBatchUseCase.spec.ts | 3 +- .../FindSkillBySlugUseCase.spec.ts | 3 +- .../getSkillById/GetSkillByIdUseCase.spec.ts | 3 +- .../GetSkillWithFilesUseCase.spec.ts | 3 +- .../ListSkillVersionsUseCase.spec.ts | 3 +- .../SaveSkillVersionUseCase.spec.ts | 3 +- .../GetStandardByIdUseCase.spec.ts | 3 +- 14 files changed, 155 insertions(+), 37 deletions(-) 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/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/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, ); }); From 9fc2ada9c704cd2ff797cae9695f1effbb3e20f0 Mon Sep 17 00:00:00 2001 From: Malo Date: Thu, 1 Oct 2026 16:19:56 +0200 Subject: [PATCH 4/7] =?UTF-8?q?=E2=99=BB=EF=B8=8F=20refactor(git):=20repla?= =?UTF-8?q?ce=20the=20NO=5FCHANGES=5FDETECTED=20sentinel=20with=20a=20type?= =?UTF-8?q?d=20error?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit CommitToGitUseCase signalled "nothing to commit" with a bare Error that callers recognised by its message. It now throws NoChangesDetectedError (kind 'conflict', reason 'no_changes_detected', repo ids in context), and the deployments catch sites — RemovePackageFromTargetsUseCase and PublishArtifactsDelayedJob — match it with instanceof. The message stays exactly 'NO_CHANGES_DETECTED' because callers outside this repo still compare against it until they are migrated. Refs #862 Co-Authored-By: Claude Opus 5.5 --- .../jobs/PublishArtifactsDelayedJob.ts | 9 +++-- .../RemovePackageFromTargetsUseCase.spec.ts | 3 +- .../RemovePackageFromTargetsUseCase.ts | 6 ++-- .../commitToGit/CommitToGitUseCase.spec.ts | 35 +++++++++++++++++++ .../commitToGit/CommitToGitUseCase.ts | 9 +++-- packages/types/src/git/errors/GitError.ts | 3 +- .../src/git/errors/NoChangesDetectedError.ts | 16 +++++++++ packages/types/src/git/errors/index.ts | 1 + 8 files changed, 71 insertions(+), 11 deletions(-) create mode 100644 packages/types/src/git/errors/NoChangesDetectedError.ts 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/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'; From 4274048aa48c29bc69ad9f60ab00b3b1b7dba081 Mon Sep 17 00:00:00 2001 From: Malo Date: Thu, 1 Oct 2026 16:22:46 +0200 Subject: [PATCH 5/7] =?UTF-8?q?=E2=99=BB=EF=B8=8F=20refactor(linter):=20gi?= =?UTF-8?q?ve=20linter-ast=20and=20linter-execution=20failures=20the=20int?= =?UTF-8?q?ernal=20kind?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit These packages run in the API and in the CLI, and their bare Errors reached the log as unstructured 500s. ConsoleLogRemovalService's guards, ParserNotAvailableError and ExecuteLinterProgramsUseCase's program compilation now throw PackmindInternalError subclasses (a LinterAstInternalError and a LinterExecutionInternalError base), with the language or cause in context. Messages are unchanged and every caller still catches them as before. Refs #862 Co-Authored-By: Claude Opus 5.5 --- .../ConsoleLogRemovalService.spec.ts | 9 +++---- .../application/ConsoleLogRemovalService.ts | 13 +++++++--- .../src/application/LinterAstAdapter.spec.ts | 9 +++++++ .../src/core/LinterAstInternalError.ts | 26 +++++++++++++++++++ packages/linter-ast/src/core/ParserError.ts | 10 +++++-- packages/linter-ast/src/index.ts | 1 + .../useCases/ExecuteLinterProgramsUseCase.ts | 8 +++++- .../errors/LinterExecutionInternalError.ts | 18 +++++++++++++ 8 files changed, 82 insertions(+), 12 deletions(-) create mode 100644 packages/linter-ast/src/core/LinterAstInternalError.ts create mode 100644 packages/linter-execution/src/domain/errors/LinterExecutionInternalError.ts 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'; + } +} From 4796f29a4466fae2e32793cab4713d177f400ae5 Mon Sep 17 00:00:00 2001 From: Packmind Date: Thu, 1 Oct 2026 16:58:45 +0200 Subject: [PATCH 6/7] =?UTF-8?q?=F0=9F=90=9B=20fix(cli):=20keep=20the=20rea?= =?UTF-8?q?l-git=20spec=20out=20of=20the=20repository=20running=20a=20hook?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit GitService.realGit.spec shells out to git init, config and commit in a temp dir, inheriting the environment. Inside a git hook run from a worktree, git exports GIT_DIR, so those calls hit the repository running the hook: the pre-push run committed nine empty "initial commit"s onto the branch, wrote a test user into the shared config and set core.bare (via init --bare), which broke the main checkout. Every git call in the spec now gets the environment without GIT_*; Jest sandboxes process.env, so deleting the keys in the test would not reach child_process. Co-Authored-By: Claude Opus 5.5 --- .../services/GitService.realGit.spec.ts | 20 ++++++++++++++++++- 1 file changed, 19 insertions(+), 1 deletion(-) 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}"`); From 1feecd190e93f22a7d2c9c68e8cef5a46473e5ee Mon Sep 17 00:00:00 2001 From: Malo Date: Thu, 1 Oct 2026 18:18:33 +0200 Subject: [PATCH 7/7] =?UTF-8?q?=E2=9C=85=20test(node-utils):=20stop=20asse?= =?UTF-8?q?rting=20the=20filter's=20log=20call=20for=20unauthenticated?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit backend-tests-redaction forbids asserting on stubbed logger output; the status and body assertions already cover the unauthenticated row. Refs #862 Co-Authored-By: Claude Opus 5.5 --- .../src/nest/filters/DomainExceptionFilter.spec.ts | 7 ------- 1 file changed, 7 deletions(-) diff --git a/packages/node-utils/src/nest/filters/DomainExceptionFilter.spec.ts b/packages/node-utils/src/nest/filters/DomainExceptionFilter.spec.ts index f2f3cd79a7..559f9cae4f 100644 --- a/packages/node-utils/src/nest/filters/DomainExceptionFilter.spec.ts +++ b/packages/node-utils/src/nest/filters/DomainExceptionFilter.spec.ts @@ -240,13 +240,6 @@ describe('DomainExceptionFilter', () => { reason: 'invalid_credentials', }); }); - - it('logs at warn', () => { - expect(logger.warn).toHaveBeenCalledWith( - 'Domain error mapped to HTTP response', - expect.objectContaining({ kind: 'unauthenticated', statusCode: 401 }), - ); - }); }); // The policy table, stated as behaviour: a new kind added to the union