✨ feat(api): map domain errors to HTTP status codes - #485
Open
MaloPromyze wants to merge 28 commits into
Open
MaloPromyze wants to merge 28 commits into
MaloPromyze wants to merge 28 commits into
Conversation
Adds scripts/agent-gate.mjs, the verification gate for the orchestrator / subagent pipeline: scope check, autofix, scoped and repo-wide Nx checks, then the unit's named test. Prints OK, or the failing step alone with the command that reproduces it. Writes .claude/features/<slug>/metrics.jsonl per run, so the gate-stage failure distribution is collected without the caller having to remember. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Adds the charter, decision-log and unit-spec templates, the return-record JSON schema, and the tier→model config. scripts/agent-record.mjs validates a unit executor's record and appends it to the feature's records.jsonl. The record shape is strict on purpose: unknown fields are rejected and prose is capped, so a subagent cannot grow the orchestrator's context by reporting at length. Cross-field rules catch a record that claims done while its own check failed. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Adds context-scout, unit-executor and reconcile as subagent definitions, and the two phase-1 skills that produce a feature's charter and decision log. unit-executor is deliberately minimal: everything unit-specific arrives in the inline spec, and a long standing prompt is paid on every unit against the same capacity that produces valid edits. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…dev loop The orchestrator skill carries the per-unit loop: baseline, route, retrieve, specify inline, dispatch one subagent, gate, route on which stage failed, record, commit. Typecheck failures route as capability and escalate the tier; test failures route as specification and re-spec at the same tier. Blocked units walk a four-rung ladder — acceptance criterion, decision entry, new decision appended by the orchestrator — and only reach a human when scope or a criterion would change. agentic-doc-ingest reads the charter, decision log and records, and never the code: the records carry public_surface_changed and behaviour_delta, filled in while the change was still in front of the executor. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The orchestrator had ADaPT's planning half — never pre-decompose — but not its
decomposition half: nothing ever split a unit, so the escalation ladder ended at
the top tier and then at a human.
A unit the strongest model cannot do from a complete spec is evidence about the
unit, not about the executor. Same for a second scope violation: an executor that
keeps reaching outside the declared files is usually right that the work does not
fit inside them. Both now route to a split.
- Two routing rows: `scope` failing twice on one unit, and any stage still
failing at the top tier.
- §7b — split into two, both halves re-enter at the bottom tier with attempt
counts reset, numbered U-014a / U-014b after the parent so the metrics can tell
an over-sized unit from a weak tier.
- models.json: splitAfterTopTierFailures 1, splitAfterScopeFailures 2.
- return-record.schema.json: unit_id widened to ^U-[0-9]{3}[a-z]?$ — without it
every split half is rejected by the validator. Verified both ways.
- A split-rate metric, and the measured gate timings in the README.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Phase 1a and 1b existed to settle what and how. The other reason to hold both conversations before any code — getting an overall view of the size of the work, and deciding whether the orchestrator runs it in one pass or several — was nowhere in the skills or the templates, so it lived only in someone's head at the moment they invoked phase 2. Size is knowable only once the decisions exist: a fork settled one way is two units, settled the other way is seven. So the verdict belongs at the close of 1b. - charter.template.md: a `Size and sessions` section — rough unit-count range, `one session` | `split`, and for a split the ACs per session with dependencies and the reason the cut falls there. Empty at framing. - agentic-feature-framing: a coarse read only — is this one feature at all — explicitly not a unit count, since the decisions that set it do not exist yet. - agentic-design-session: a closing sizing step, with the signals that justify a split and the ones that do not. Unit size stays phase 2's problem. A split is not a re-framing: same charter, same log, the run is cut, not the feature. - agentic-orchestrator: honours the verdict. On `split` it owns one session's ACs and halts on the others like any out-of-scope item; on an empty section it asks rather than sizing the feature itself. The unit count stays a rough read throughout. Nothing upfront is a spec — the only specs are the inline ones written for the unit about to run. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…mething The gate checked only the exit code of the exit criterion, which meant the one thing it exists to enforce was not enforced. Three ways a unit went green having proved nothing, all reproduced against this repo: - A unit that changed no files. An empty diff is inside any declared scope, `nx affected` with no files is a no-op, and the criterion passes off the existing suite. The gate printed OK. - A criterion naming a test that does not exist. A `--testNamePattern` matching nothing skips every test and jest still exits 0: `45 skipped, 0 of 4 total`, exit 0, gate OK. - Both together, which is what a unit-executor that silently did nothing actually produces. So `scope` now fails an empty diff, and `tests` reads jest's `Tests:` line and fails a criterion that exited 0 having run no assertion. Refactors are the case with no new assertion to make, and the orchestrator skill already describes gating them on the existing tests instead. That is now declared rather than implied: `exit_criterion.kind` is `behavioural` by default and requires a passing assertion; `characterization` requires the existing tests green and no test file modified, since a refactor that edits its own tests proves nothing. An unknown kind halts. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Nx reads `-t` as `--targets` and never forwards it to jest, so the criterion form seeded by the unit-spec template, `nx test spaces -t 'removeMember'`, ran the whole project suite. Every unit was judged by tests that already passed before it started. Measured on this repo: `nx test linter-ast` and `nx test linter-ast -t 'aTestNameThatDoesNotExist'` print byte-identical summaries. Also documents `exit_criterion.kind`, which the gate now enforces, and the two checks that an exit code alone does not give: no-op units and criteria that assert nothing. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The README had the mechanics and the reasoning but not the thing someone opening it actually wants first: what you say, in what order, and where each phase stops. The closing slide of the talk points people here, so it is the file that has to answer that. Covers the three phases as conversations rather than commands, what to act on during the loop (OK, FAIL, HALT, and that only HALT is yours), why restarting the orchestrator is cheap and often correct, and the three things to get right before the first run. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
CLAUDE.md asks that PACKMIND_EDITION be oss, which meant remembering to export it in every shell and prefixing it onto commands. Project settings set it once for the whole session instead, team-wide. Node cannot be handled the same way — the version comes from the shell, and any PATH we could write here would be machine-specific. The README now says to set it once per machine with `nvm alias default 24` rather than listing `nvm use` as something to remember; the gate compares only the major version, so a patch bump in .nvmrc does not invalidate that alias. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…esign Phases 1a and 1b of the agentic pipeline for the redo of PR #462. No code. The charter drops the CLI from scope entirely: `packmindEdition.ts` now decides feature-absence from the `PACKMIND_EDITION` header rather than from a 404, so the sentinel collision the original PR worked around no longer exists. Three decisions depart from that PR, each because measuring contradicted it: - D-003 carries `kind` through the base constructor instead of a subclass class field. swc at es2022 does not downlevel class fields, so a bare declaration yields `undefined` under `swcTransform` (spaces, standards, skills, ...) and `'not_found'` under `swcTransformWithDefineFields` (accounts). The PR's field initializer is correct, but correct by a convention whose failure is invisible in the author's own package. - D-004 keeps the filter out of the `@packmind/node-utils` barrel rather than writing `@Catch()` as a call. `@packmind/node-utils` resolves to src, so all 17 consumers' Jest parse node-utils source and only 5 enable decorators. A file no consumer imports is a file no consumer parses. Also flips `packages/node-utils/.swcrc` to `decorators: true`, which the build needs and the PR did not mention. - D-013 records that `SkillsGatewayApi.getSkillBySlug` maps any 404 to `null`, so a lost space membership renders as skill-not-found. That is what D-006's anti-enumeration choice asks for; written down so it is not refiled as a bug. D-002 was found rather than designed: `ServerErrorResponse` already declares an optional `reason` for exactly this purpose and has never been given one. All six known unknowns are decided, none deferred. Sizing verdict: one orchestrator session, 6-10 units — no subset of this is releasable alone. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
An error's HTTP status is a property of the error, but a status number is false at half its throw sites — the same errors are raised inside BullMQ workers. So the contract is semantic: a kind, and a stable snake_case reason for the wire. No status number appears here; the adapter that has a response to write is the only layer entitled to turn kind into a number. It lives in `packages/types` rather than beside its only current consumer because `packages/types` is `env:shared` and `node-utils` is `env:node`, and the module-boundary rule would put the union permanently out of the frontend's reach — and the frontend is exactly the client that wants to branch on it. U-001, first attempt, cheap tier. D-001, D-005. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The status was always a property of these errors, stored everywhere except
on the error — twenty controllers grew their own instanceof ladders, and the
HTTP intent leaked into the domain as prose. Each class now states its kind
once, at the super() call, and the adapter layer reads it.
kind rides the base constructor rather than a subclass class-field because
swc does not downlevel class fields: under the transform most packages use,
`class Sub extends Base { kind; }` yields undefined, and under the one
`accounts` uses it yields the right value. The same source, opposite results,
in two suites that both have to be green — and the failure mode is a silent
undefined that turns a 403 back into a 500 in production while the author's
own package's tests pass. A constructor parameter cannot be written wrong.
Messages are untouched: they interpolate UUIDs and rewriting them turns the
skills and standards suites red, so that lands with its spec fallout.
U-002, first attempt, top tier. D-003, D-006, D-014.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
They were access errors that happened to be declared next to the use cases throwing them rather than next to their siblings, and they carried no reason at all — a client could have seen the status and nothing else. Extending the base gives them reason, context and the constructor-assigned kind from one mechanism instead of two. Membership answers not_found, not forbidden. Dozens of space-scoped use cases throw it with no controller handling, so a blanket 403 would make every one of them an existence oracle: guess a space id, read the status, learn whether it exists. 404 makes "no such space" and "not your space" indistinguishable, and the message is worded to confirm neither. Space-admin stays forbidden, because reaching it already proves membership — the caller is inside the space, so saying it exists discloses nothing they do not have. spaceId joins the context type as optional rather than being redeclared on the subclasses: a redeclared field compiles to undefined under the swc transform most suites here use, which would erase the whole context, not just the id. The 26 specs asserting these classes assert by class reference and are untouched. The one that pinned message text now asserts the class. U-003, first attempt, top tier. D-006, D-007, D-008, D-015. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Nobody has ever read these messages: the response body was always "Internal server error", so a UUID in the text cost nothing. The filter later in this feature makes them reachable — they land in the frontend surfaces that render error.message and in the CLI's stderr — so rewording them is a consequence of the feature, not a cleanup beside it. Deferring it would ship the regression first and fix it after it had been seen. Second person, because the reader is the person who was denied, not an operator reading a log. Identifier-free, because a UUID tells that reader nothing they can act on. The ids stay on .context, which reaches the logs, which is where operators are. The sixteen specs that pinned the old strings now assert the error class, matching what the abstract use-case specs already did. The AC-7 test checks the property — that no id passed in appears in the message — rather than an expected string, so a future rewording cannot quietly retire it. Verified beyond the gate, which runs lint and build but not these suites: skills, standards and packmind-cli all green. U-004, first attempt, top tier. D-008. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The span status is the service's own verdict on whether it worked, and a user being told they may not do something is the service working. Leaving it ERROR means the error ratio climbs with ordinary permission denials — which makes it useless as an alerting signal exactly when adoption grows. Without this, the feature would convert 500s into clean 403s at the HTTP layer while still reporting them as service errors in telemetry. Whoever watches the dashboard would read that as a regression. recordException stays unconditional, so a specific denial is still diagnosable in its own trace. Only the aggregate verdict changes. The criterion runs the whole withSpan suite rather than just the new tests, so it proves both halves: a domain error no longer marks the span failed, and a plain Error still does. U-005, first attempt, cheap tier. D-010. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The sweep that found the assertions pinning the old UserNotFoundError text searched for "User not found:" with a colon. These two assert the substring "User not found" without one, so they were missed and the commands suite has been red since the rewording landed. The gate runs lint and build, not the suites of projects a unit touches, so nothing caught this — the drift check did. Worth remembering that a substring assertion is invisible to a sweep written around the fuller string. U-006, first attempt, cheap tier. D-008. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The charter's premise was that every throw site funnels through AbstractMemberUseCase, so the subject is always the caller. It is false for two of the six classes: packages/accounts re-exports both and throws them about a target user at five sites. An admin changing the role of a user who does not exist would have been told "Your user account could not be found." That is worse than the UUID the rewording removed — the old message was at least about the right person. Subject-neutral wording is true in both readings, and costs two strings. No new class, no accounts call site touched: distinct target-user errors would be correct rather than merely true, but the charter budgeted six annotations, not two more classes and their fallout. not_found stands. It was justified by the record being the caller's own, but its other argument holds for a target too — the request is unresolvable, and a missing user is what 404 means. Found by the drift check, not by a test. No unit deviated; it is invisible from inside any single one. U-007, first attempt, cheap tier. D-016. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The reconcile pass found two things no unit could see from the inside, and both wanted deciding before the filter lands. D-016 is committed separately with its code. D-017 settles how the booted-app spec proves registration: the literal reading of D-012 — boot the real AppModule — has no precedent here and pulls TypeORM, JWT and every hexa into a unit-test run, so it would fail for reasons unrelated to the filter. Boot a minimal module for selection and mapping; read AppModule's own provider metadata for installation. Neither half replaces the other, which is what D-012 asked for. Also corrects three unit exit criteria that claimed to describe AC-1, AC-4 and AC-6. D-012 reserves those for the booted-app spec, so the labels now say "prepares" — otherwise transcribing them would report three ACs as verified by tests that cannot verify them. AC-7 and AC-10 are filled in: they are genuinely met and independently confirmed. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@packmind/node-utils maps to source, not to built output, so every consuming package's Jest compiles node-utils' TypeScript with its own swc transform. Seventeen projects import it and five parse decorators. src/index.ts does `export * from './nest'`, so a filter placed in that barrel would be parsed by the twelve that do not — and swc with decorators: false fails on @catch() at parse time, not lazily, breaking suites that never use the filter. Hiding the file from the barrel removes the problem instead of guarding the symptom, but then apps/api cannot reach it. Hence a subpath, the way @packmind/git/schemas already works. apps/api uses swcTransformWithDecorators and is the only project that will import it. The target does not exist yet; U-008 creates it. Dangling entries are already tolerated here. Made by hand rather than by a unit: tsconfig.base.json is a gate guardrail, so no executor can edit it. Approved by Malo before the change. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The first real consumer of the contract. Three branches, in order:
HttpException passes through untouched — which is what makes leaving twenty
controller ladders alone free, since some of them disagree with the class
default on purpose; a domain error maps kind to 403/404 with a
{statusCode, message, reason} body; anything else keeps today's 500.
The body reuses a shape the frontend already declares, with reason documented
there as the discriminator clients branch on instead of matching prose. So no
client changes, and the unannotated-error path stays byte-identical.
Domain errors log at warn without a stack, which ExpectedAuthError's docstring
has asked for since it was written and nothing was ever in a position to
honour. A denial is an expected outcome of a working system; paging it at
ERROR with a stack is what trains people to ignore the channel.
The file sits on its own subpath, out of every barrel: node-utils resolves to
source, so a barrel-reachable @catch() is parsed by the twelve importing
projects that do not enable decorators, and fails there with Expression
expected.
node-utils' own Jest was the case D-004 missed — @swc/jest ignores .swcrc when
given explicit options, so the package owning the filter was the one that
could not parse it. It moves to swcTransformWithDecoratorsOnly, not
swcTransformWithDecorators: the latter sets useDefineForClassFields false
across all 31 suites, which is the exact axis that turns a class field into a
silent undefined. It passes green, but green is not the property at issue.
U-008, second attempt after a correct block, top tier.
D-002, D-004, D-005, D-009, D-011, D-018.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…lled APP_FILTER rather than useGlobalFilters in main.ts: it is DI-aware, so the filter can take a logger, and it sits inside the module graph the test boots. Per-controller binding would be twenty registrations and a silent gap on the twenty-first. The spec is the point of this unit. A filter can map every kind correctly and never be selected by Nest — wrong metadata, wrong registration, a barrel that did not re-export it — and a unit test passes in all of those cases while every endpoint still returns 500. So the claim is split: booting a minimal module on port 0 and driving it with fetch proves Nest selects the filter and maps correctly; reading AppModule's own provider metadata proves the real graph installs it. Booting AppModule itself would need TypeORM, JWT and every hexa to stand up, and would fail for reasons unrelated to the filter. The plain-Error and HttpException cases are asserted but prove passthrough, not registration — Nest's default handler produces the same bodies. AC-4 is asserted as byte-identity: the same class answers both "not your space" and "no such space", so the two responses cannot be told apart. That is the anti-enumeration property, checked on the wire rather than argued. Webpack keeps an alias map independent of tsconfig and aliased the package to a directory, so the subpath resolved to src/filters and only nx build api failed. Fourth and last resolver; D-019 writes the set down. U-009, second attempt after a correct block, top tier. D-002, D-006, D-011, D-012, D-017, D-019. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…g it A global @catch() intercepts everything, including the http-errors that body-parser throws. main.ts sets a 15mb body limit for bulk imports, so an oversized upload answers 413 today — and the filter, which reproduced only one of Nest's two default branches, turned that into a 500 with an ERROR-level page. The feature that exists to stop ordinary client errors looking like crashes had quietly created a new one. Every test passed, because the integration spec only threw new Error('boom'), which takes the same branch under both implementations. The drift check found it by reading the call graph, not the tests. The fix is not the missing branch. It is to stop copying a dependency's private behaviour: BaseExceptionFilter now handles everything that is not a domain error. That also restores the isHeadersSent guard — sse.controller streams through @res(), where an unguarded write throws ERR_HTTP_HEADERS_SENT — the IntrinsicException suppression that was already missing, and the unhandled-error log's original ExceptionsHandler origin, which had become AppController via the injected logger. AC-8 and AC-9 are now true by construction rather than by a test kept in sync with someone else's internals. The specs assert the absence of a stack on the warn payload, which objectContaining never did. U-010, second attempt: the first failed typecheck alone — swc strips types without checking them, so tests and build both passed a missing override. D-002, D-009, D-011, D-020. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Every AC now names the command that proves it. Only the fills the drift check judged honest: AC-8 cites both specs, because the booted app proves the status and body while the unit spec proves the ERROR-level log; AC-11 cites the unit spec alone, since log level is not an HTTP-observable property. D-020 records why the non-domain path delegates rather than reproduces. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Contributor
 |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
…hell `stepScoped` appended the changed-file list straight onto `commands.scoped` and ran it with `shell: true`. The paths are whatever the executor created, and scope validation is glob-only, so a filename like `packages/x/$(...).ts` satisfies a declared `packages/x/**/*.ts` and then gets evaluated by the shell with the developer's credentials. Quote the joined list the way `stepAutofix` already quotes its arguments. It is one `--files=` argument, so the list is quoted whole rather than file by file. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Every other barrel here uses `export * from`; this one imported the symbol only to name it again on the next line. The filters barrel was the exception, so this removes an inconsistency rather than introducing a rule. Gated as a characterization unit: no test file changed, and the existing integration spec reaches the filter through app.module.ts, which imports it through this barrel — so the spec passing is what proves the re-export still resolves, in webpack as well as in Jest. U-011, first attempt, cheap tier. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The gate ran ESLint and never the Packmind CLI, so a unit could be green under scoped and wide and still break CI on a rule neither of them knows about. That is exactly what happened: twelve units passed, and the standards check failed after the PR was opened. Changed files only, so a violation names the unit that introduced it instead of inheriting one from a file nobody touched. The awkward part is that the CLI exits 1 for two unrelated reasons — it found violations, and it could not run at all. An expired key, a missing build, no network and a real violation are indistinguishable by exit code, and treating the environment as a unit failure would blame the executor for it and turn the metrics into noise. That is the failure baseline already exists to prevent. So an unavailable checker is recorded as skipped and the unit proceeds, while violations fail the gate. The skip carries its reason into metrics and is never silent, and CI runs the identical lint with a valid key, so it remains the backstop. Verified both paths: an expired key skips, a violation fails. A standards failure routes like `tests`, not like `typecheck` — the executor wrote valid code against a house rule nobody told it about, so it re-specs at the same tier with the rule quoted rather than escalating. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The repo's @backend-tests-redaction standard forbids asserting on stubbed logger output, and the Packmind CLI blocks CI on it. No other spec in the monorepo does it; there is no ignore file and no disable comment anywhere. The conflict was genuine rather than technical: AC-11 is itself a statement about logging — an access denial is logged at warn without a stack — and for a log level there is no observable behaviour other than the log. The standard assumes logging is never the requirement under test, which is true everywhere else and false here. It yields anyway. A standard the whole codebase observes should not acquire its first exception inside a feature PR, decided by that feature's author, and the cost is bounded: the behaviour ships correct and described, only its protection is lost. D-021 records the accepted cost so it is not rediscovered as an oversight — nothing now catches a change from warn to error, or a stack reappearing in the payload. The adjacent assertion reading a warnPayload() helper was not flagged, because the rule matches expect(logger.…) directly. It is removed too: keeping it would be the same assertion hidden from the matcher, which evades the rule instead of satisfying it. Sixteen tests remain, covering status, body, the exact key set, HttpException passthrough, http-errors status preservation and the headers-sent path. U-012, first attempt, cheap tier. D-021. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
cteyton
reviewed
Sep 14, 2026
| @@ -0,0 +1,179 @@ | |||
| #!/usr/bin/env node | |||
| // scripts/agent-record.mjs | |||
Contributor
There was a problem hiding this comment.
Does it make sense to keep this script here ?
If yes, could we move it in a dedicated or different folder maybe ?
cteyton
reviewed
Sep 14, 2026
| @@ -0,0 +1,15 @@ | |||
| { | |||
Contributor
There was a problem hiding this comment.
does it make sens to keep these files ?
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
2 participants
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.
Redo of #462, addressing PackmindHub/packmind-proprietary#773.
The problem
apps/apihas no exception filter — no@Catch()anywhere in the tree — so NestJSfalls back to its default handler, which maps every non-
HttpExceptionto500 Internal Server Errorand logs it at ERROR with a full stack trace.A user whose account was deleted, a user acting outside their organization, and a user
without admin rights therefore all get the same answer as a genuine crash: a 500 whose
body reads
"Internal server error". The frontend can only show a generic failure, theCLI can only print one, the on-call engineer sees a stack trace for an ordinary
permission denial, and the service error ratio counts it.
The translation layer that produces these errors was already there and already correct —
every authenticated use case funnels through
AbstractMemberUseCase.execute, whichdeliberately re-types a
UserAccessErrorinto something specific. Nothing consumed itsoutput.
ExpectedAuthError's docstring has asked for this since it was written:The behaviour
A domain error is handled by the filter. Everything else is exactly as it was.
kind{ statusCode, message, reason }, logged atwarnwithout a stackHttpExceptionThe last two rows are not reimplemented. The filter handles the domain case and delegates
everything else to
BaseExceptionFilter, so non-domain behaviour is identical to todayby construction rather than by a test that has to track a dependency's internals.
Adoption is opt-in per error class, and that is the whole mechanism
An error declares a kind — never a status number, because the same errors are raised
inside BullMQ workers where a status means nothing — and a stable snake_case reason
for the wire. Once a class declares its kind, every site that throws it answers with the
right status: no change at the throw site, none in the controller. A class that has not
opted in keeps producing exactly the 500-with-stack it produces today. There is no flag
day.
Six classes opt in here, all of them access errors that funnel through
AbstractMemberUseCase, which covers every authenticated endpoint at once:UserAccessError(base)forbiddenUserNotFoundErrornot_foundUserNotInOrganizationErrorforbiddenOrganizationAdminRequiredErrorforbiddenSpaceMembershipRequiredErrornot_foundSpaceAdminRequiredErrorforbiddenSpaceMembershipRequiredErroranswers 404 on anti-enumeration grounds. Dozens ofspace-scoped use cases throw it with no controller handling, so a blanket 403 would turn
every one of them into an existence oracle: guess a space id, read the status, learn
whether it exists. 404 makes "no such space" and "not your space" indistinguishable, and
the integration spec asserts the two responses are byte-identical.
User-visible: the access messages are rewritten
These messages have never been seen by anyone, because the body was always
"Internal server error". The moment the filter ships they reach the frontend surfacesthat render
error.messageand the CLI's stderr — so rewording them is a consequence ofthis change, not a cleanup beside it.
They no longer interpolate UUIDs. The ids stay on
.context, which reaches the logs(where operators are) and which the filter does not serialize. Two of the five are
subject-neutral rather than second-person, because
packages/accountsthrows them abouta target user as well as the caller.
Also
withSpanno longer setsSpanStatusCode.ERRORfor a domain error, so an ordinarydenial stops counting against the service error ratio. The exception is still recorded;
only the span's aggregate verdict changes.
ServerErrorResponseshape the frontend already declares,with
reasondocumented there as the discriminator clients branch on instead ofmatching prose. No client changes.
Deliberately not in this PR
apps/cliorapps/frontend.instanceofladders stay. They throwHttpException, which thefilter passes through untouched, and some disagree with the class default on purpose.
Deleting them is a per-route judgement.
should be driven by what still shows up as a 500 once these six are done.
MemberNotFoundError(packages/spaces) leaks a target user's id and wants its ownissue.
Notes for review
The filter sits on its own subpath (
@packmind/node-utils/filters) and is deliberatelynot exported from any barrel.
@packmind/node-utilsresolves to source, so abarrel-reachable
@Catch()would be parsed by the twelve importing projects that do notenable decorators and fail there with
Expression expected. That is also why fourseparate resolver configs needed an entry — tsconfig paths, Jest's generated mapping,
node-utils'
.swcrc, and apps/api's webpack aliases.Verification
it with
fetch. A filter can map every kind correctly and still never be selected byNest — a unit test passes in that case while every endpoint returns 500 — so the spec
also asserts
AppModule's provider metadata directly.cli-e2e-testsagainst a live stack.🤖 Generated with Claude Code