♻️ refactor: give the remaining shared domain errors a kind (#862) - #531
Open
MaloPromyze wants to merge 7 commits into
Open
MaloPromyze wants to merge 7 commits into
MaloPromyze wants to merge 7 commits into
Conversation
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 <noreply@anthropic.com>
… error filter 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 <noreply@anthropic.com>
…inded errors 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 <noreply@anthropic.com>
…ed error 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 <noreply@anthropic.com>
…e internal kind 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 <noreply@anthropic.com>
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 <noreply@anthropic.com>
Contributor
 |
…ticated 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 <noreply@anthropic.com>
|
This branch has not been deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Explanation
The last OSS pass of PackmindHub/packmind-proprietary#862 (errors thrown from domain and application layers must carry a
kind). A type-aware ESLint rule built on the proprietary side found the sites that still reachDomainExceptionFilterwithout akind; this PR fixes the shared ones.unauthenticatedkind (401). It joinsDomainErrorKindand gets a row in the filter'sKIND_POLICY(warn, message returned, not retried). The frontend has no global 401 handler (useAuthErrorHandlerexists but is never called), so answering 401 never logs anyone out.InvalidEmailOrPasswordErrorjoinsAccountsErrorwith kindunauthenticated/ reasoninvalid_credentials, and the hand-mapped 401 inauth.controller.tsis gone. The body keeps the samemessageand gainsreason.TooManyLoginAttemptsErrorkeeps its hand-mapped 429, because no kind answers 429 withbannedUntilin the body yet.MembershipOrganizationNotFoundError(404) instead of a bareError(500). An emptyorganizationIdnow gives the realUserNotInOrganizationError(403).handleValidationErroris narrowed toUserAccessError, and the unreachable admin branch throws an internal error.NO_CHANGES_DETECTEDbecomesNoChangesDetectedErrorwith the exact same message, because proprietarymarketplacesjobs still match on the message until the next sync. The deployments catch sites now useinstanceof.PackmindInternalErrorsubclasses. Messages are unchanged, so every caller's catch behaves as before.GitService.realGit.spec.tsinherited the hook'sGIT_DIRwhen pushing from a worktree. It committed empty "initial commit"s onto the branch, wrote a test user into the shared.git/configand setcore.bare=true. Each git call in the spec now gets an env withoutGIT_*. I checked this against a throwaway repo: before the fix it received 9 commits and the test user, after it nothing.Relates to PackmindHub/packmind-proprietary#862
Type of Change
Affected Components
reasonfield on the sign-in 401 bodyTesting
Test Details:
Specs assert errors by instance, not by message. The new specs cover the 401 policy row, the sign-in rethrow, the org-row-gone and empty-org-id cases, the
NoChangesDetectedErrorinstance plus its exact message, and the internal linter errors. Nine existing specs in accounts, skills and standards asserted the old org-not-found message; they now assert the class. The pre-push hook (nx affected -t lint test build) passed.TODO List
domain-error-handlingstandard needs anunauthenticatedrow; that is a Packmind playbook update, not a hand editReviewer Notes
NoChangesDetectedErrormessage must stay'NO_CHANGES_DETECTED'until proprietarymarketplacesmoves toinstanceofafter the sync.TooManyLoginAttemptsError(429 +bannedUntil) is still kindless on purpose. It is an open decision on #862.apps/cli-e2e-tests/src/helpers/setupGitRepo.tsfollows the same pattern as the leaking spec. The pre-push hook excludes it, so it is left as is.🤖 Generated with Claude Code