fix(server): bind services + importers packages in dofigen#2113
Merged
mxkaske merged 1 commit intofeat/services-status-reportfrom Apr 23, 2026
Merged
fix(server): bind services + importers packages in dofigen#2113mxkaske merged 1 commit intofeat/services-status-reportfrom
mxkaske merged 1 commit intofeat/services-status-reportfrom
Conversation
Adds `packages/services/package.json` and `packages/importers/package.json`
to the `install` stage bind list so `bun install --frozen-lockfile` can
resolve the dep graph inside the Docker build.
Without this, the moment main receives:
- `@openstatus/services` as a dep of `apps/server` (landing via the
status-report PR that this branch targets), bun install fails with
a missing workspace package.
- `@openstatus/importers` becomes a transitive dep of
`@openstatus/services` later in the stack (via the import-domain
migration), so we bind it proactively in the same commit rather
than landing a second dofigen touch-up.
`packages/importers` already exists in the repo and isn't used by
apps/server directly at this point, so binding it early is a no-op
for bun until it actually becomes a dep. Alternative was two sequential
fix PRs; one covers the whole stack cleaner.
`Dockerfile` + `dofigen.lock` are regenerated from `dofigen.yml` via
`dofigen update` — both are generated outputs that must stay in sync
with the yml source.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
The latest updates on your projects. Learn more about Vercel for GitHub. 3 Skipped Deployments
|
Member
Author
|
@claude review |
|
Claude finished @mxkaske's task in 1m 29s —— View job Code Review
|
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
mxkaske
added a commit
that referenced
this pull request
Apr 23, 2026
Adds `packages/services/package.json` and `packages/importers/package.json`
to the `install` stage bind list so `bun install --frozen-lockfile` can
resolve the dep graph inside the Docker build.
Without this, the moment main receives:
- `@openstatus/services` as a dep of `apps/server` (landing via the
status-report PR that this branch targets), bun install fails with
a missing workspace package.
- `@openstatus/importers` becomes a transitive dep of
`@openstatus/services` later in the stack (via the import-domain
migration), so we bind it proactively in the same commit rather
than landing a second dofigen touch-up.
`packages/importers` already exists in the repo and isn't used by
apps/server directly at this point, so binding it early is a no-op
for bun until it actually becomes a dep. Alternative was two sequential
fix PRs; one covers the whole stack cleaner.
`Dockerfile` + `dofigen.lock` are regenerated from `dofigen.yml` via
`dofigen update` — both are generated outputs that must stay in sync
with the yml source.
Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
mxkaske
added a commit
that referenced
this pull request
Apr 23, 2026
* feat(services): migrate status-report domain onto service layer
First domain migration on top of the services scaffold (PR 0). Freezes
the `ServiceContext` shape and the single-object `{ ctx, input }`
argument convention across all three surfaces — tRPC / Connect / Slack.
## Services
- `packages/services/src/status-report/` — create, add-update, resolve
(convenience wrapper over add-update), update (metadata+associations),
update-update (edit a single update row), delete, delete-update, list,
get, notify.
- Shared internals: `validatePageComponentIds`,
`updatePageComponentAssociations`, `getReportInWorkspace`,
`getReportUpdateInWorkspace`, `getUpdatesForReport`,
`getPageComponentIdsForReport`.
- All mutations run inside `withTransaction`, emit an `emitAudit` row
before returning, and throw the typed `ServiceError` hierarchy.
- Zod schemas are the contract; `.parse()` runs at every service entry
as defence-in-depth.
- `notifyStatusReport` is the separate-call notification dispatch that
the dashboard invokes as its second mutation — preserves today's
two-call pattern required by the Edge runtime.
## Surfaces
- **tRPC** (`packages/api/src/router/statusReport.ts`): every procedure
now calls a service. External contract is preserved bit-for-bit —
`create` still returns `{ ...initialUpdate, notifySubscribers }`,
`list` still returns the enriched `{updates,pageComponents,page}` shape
the dashboard consumes, etc. `emailRouter.sendStatusReport` becomes a
thin wrapper over `notifyStatusReport`.
- **Connect RPC** (`apps/server/src/routes/rpc/services/status-report/`):
handler extracts proto → parse → service call → convert back. Domain
helpers previously exported from this folder (and consumed by Slack)
are gone; callers go through services directly.
- **Slack** (`apps/server/src/routes/slack/interactions.ts`): the four
status-report branches (create / add-update / update / resolve) call
services. The maintenance branch still writes to db directly — that
migrates in PR 2.
## Adapters
- `packages/api/src/service-adapter.ts` — `toServiceCtx(ctx)` +
`toTRPCError(err)` (ServiceError → TRPCError).
- `apps/server/src/routes/rpc/adapter.ts` — `toServiceCtx(rpcCtx)` +
`toConnectError(err)`. Synthesises `actor.keyId` as `ws:<workspaceId>`
until the auth interceptor captures the real key id (called out in
the plan's open questions).
- `apps/server/src/routes/slack/service-adapter.ts` — refetches the full
workspace for the `PendingAction`'s workspaceId and builds a Slack
actor context; `toSlackMessage(err)` renders human-readable failures.
## Enforcement
- Biome `noRestrictedImports` (nursery rule) scoped to
`packages/api/src/router/statusReport.ts` +
`apps/server/src/routes/rpc/services/status-report/**`. Tests are
ignored since setup/cleanup legitimately manipulates db state.
- Subpath export `@openstatus/services/status-report` so surfaces
import the domain barrel directly.
- `packages/api` / `apps/server` now declare `@openstatus/services` as
a dependency.
## Tests
- Integration tests in `packages/services/src/status-report/__tests__/`
cover happy paths, workspace isolation, cascading deletes, the
`ForbiddenError` / `NotFoundError` paths, and the slack-actor audit
branch. `expectAuditRow` asserts on the in-memory audit buffer.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
* fix(services): address Cubic review + unblock dashboard build
- **Dashboard build**: `data-table-row-actions.tsx:35` accesses
`row.original.page.customDomain`, which the old tRPC zod schema made
non-nullable. My migrated service returned `page: Page | null`,
breaking the type. Narrow `page` to non-null in the tRPC `get` / `list`
wrappers via a tiny `narrowPage` helper that throws on the shouldn't-
happen case (status report without its page) — matches the old zod
parse behaviour.
- **`listStatusReports` silent truncation (Cubic finding)**: the tRPC
wrapper hard-coded `limit: 100`, silently dropping reports beyond
that ceiling for workspaces with >100 reports. Service now allows an
unbounded `limit` (min 1, no max); tRPC passes a `10_000` sentinel
and Connect keeps its external `max=100` cap in the handler.
- **`LIMIT_EXCEEDED` Connect mapping (Cubic finding)**: map to
`Code.ResourceExhausted` rather than `Code.PermissionDenied` —
quota/rate-limit is the documented meaning of ResourceExhausted in
Connect/gRPC, while PermissionDenied means "authorised but not
entitled". Correct semantics on the wire.
Biome `include` key (Cubic's third finding) is verified correct for
Biome 1.8.3 — OverridePattern schema documents `include` as the glob
selector, and our override fires as expected (it flags the Connect
test file in the scoped dir). No change.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
* fix(services/status-report): address Cubic findings on the maintenance PR
Applies to status-report since the same patterns were copy-pasted across
both domains:
- **Dedupe pageComponentIds** in `validatePageComponentIds` — duplicate
ids in the input would hit the composite-PK constraint on
`status_report_to_page_component` during insert.
- **Batch list enrichment** — `listStatusReports` previously fanned out
3 queries per row (updates / component associations / page + page
components). Dashboard calls it with `limit: 10_000`, so the N+1 was
real. Rewritten as three batched `IN` queries regardless of row count;
pages are deduplicated so reports sharing a page share the Page object.
`getStatusReport` reuses the batch path with a singleton.
- **Idempotent `delete`** — the old tRPC `statusReport.delete` silently
succeeded when the row was missing; my service throws `NotFoundError`.
Catch and swallow in the tRPC wrapper only. Connect still returns
404 on missing — external API semantics preserved.
- **Connect numeric-id error** — `parsePageComponentIds` was throwing
`invalidDateFormatError` for non-numeric input. Replaced with an
inline `ConnectError(Code.InvalidArgument)` with the correct message.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
* fix(server): bind services + importers packages in dofigen (#2113)
Adds `packages/services/package.json` and `packages/importers/package.json`
to the `install` stage bind list so `bun install --frozen-lockfile` can
resolve the dep graph inside the Docker build.
Without this, the moment main receives:
- `@openstatus/services` as a dep of `apps/server` (landing via the
status-report PR that this branch targets), bun install fails with
a missing workspace package.
- `@openstatus/importers` becomes a transitive dep of
`@openstatus/services` later in the stack (via the import-domain
migration), so we bind it proactively in the same commit rather
than landing a second dofigen touch-up.
`packages/importers` already exists in the repo and isn't used by
apps/server directly at this point, so binding it early is a no-op
for bun until it actually becomes a dep. Alternative was two sequential
fix PRs; one covers the whole stack cleaner.
`Dockerfile` + `dofigen.lock` are regenerated from `dofigen.yml` via
`dofigen update` — both are generated outputs that must stay in sync
with the yml source.
Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
* fix(services): stub RESEND_API_KEY for tests
`packages/services/src/status-report/notify.ts` imports from
`@openstatus/subscriptions`, which transitively pulls in
`@openstatus/emails`. The emails package validates `RESEND_API_KEY`
via `@t3-oss/env-core` at module-load time and throws on missing —
which killed `@openstatus/services#test` in CI (neither the test.yml
workflow nor local `pnpm test` sets the var).
Adds a bun test preload that stubs the env var with a dummy value
before any test file is imported. Tests never exercise the real
Resend dispatch path, so this is scoped to unblocking the import
graph rather than mocking the dispatcher itself.
Added:
- packages/services/bunfig.toml — points bun:test at the preload.
- packages/services/test/preload.ts — sets the dummy env var.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
* test(status-report): align with new service-layer error semantics
Two CI failures surfaced after the services migration changed error
shapes for status-report writes:
- **tRPC `statusReport.create` / `.updateStatus` cross-workspace tests**
expected `FORBIDDEN` when a caller passed pageComponentIds belonging
to another workspace. The service's `validatePageComponentIds` now
throws `NotFoundError` for the same case — intentionally, so cross-
workspace lookups don't leak resource existence. Updated both tests
to assert `NOT_FOUND` and left a comment pointing at the migration
rationale.
- **Connect `CreateStatusReport` page-mismatch test** asserted the
error message contained "does not match the page ID". The service
throws `ConflictError("pageId X does not match the page (Y) of the
selected components.")` — no "ID" in the wording. Loosened the
assertion to "does not match the page" so it matches the actual
service string.
Both are pure test updates — the new service behavior is the intended
contract; the tests had drifted from the pre-migration wording.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
* fix(services): address post-migration review on #2101
Five findings from the PR review, fixed in one commit:
**P1 — tRPC `LIMIT_EXCEEDED` → `TOO_MANY_REQUESTS`**
`packages/api/src/service-adapter.ts` mapped `LIMIT_EXCEEDED` to
`FORBIDDEN`. `FORBIDDEN` means "authenticated but not allowed to do
this on this resource" — plan quota exhaustion is semantically
different. Mirrors the Cubic fix on the Connect side (which moved
from `PermissionDenied` to `ResourceExhausted`).
**P2 — fragile Drizzle join result key in `enrichReportsBatch`**
`packages/services/src/status-report/list.ts` read
`row.status_report_to_page_component.statusReportId` — Drizzle
auto-derives that key from the schema variable name at import time,
so a rename would silently produce `undefined` at runtime. Replaced
with explicit-column select (`reportId:
statusReportsToPageComponents.statusReportId`) which is typed and
rename-safe.
**P2 — Slack `NOT_FOUND` messages leak internal row IDs**
`NotFoundError` formats as `"<entity> <id> not found"` (e.g.
`"page_component 17 not found"`). For API consumers that's fine —
they're developers. For Slack workspace users those IDs are
meaningless noise and surface internals. `toSlackMessage` now
returns a fixed `"Couldn't find what you were looking for."` for
`NOT_FOUND`; other `ServiceError` codes keep their hand-written
messages (they're sentence-style to start with).
**P3 — test cleanup without try/finally**
`packages/services/src/status-report/__tests__/status-report.test.ts`
had 13 tests that did inline `db.delete(statusReport)` cleanup at
the end of their body — skipped on assertion failure. Replaced with
a shared `createdReportIds` array + `afterEach` drain so cleanup
runs regardless of test outcome.
**P3 — hoisted `toServiceCtx` out of Slack `executeAction` switch**
Every service-backed branch loaded the workspace row via
`toServiceCtx`. Hoisted the call above the switch — halves db
lookups per Slack action execution. The `createMaintenance` branch
(still on db until PR 2) doesn't use the ctx but the extra lookup
there is negligible.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
* fix(services/status-report): pageId follows components on update
Two server-side tests were failing after the migration:
- `updates pageId when changing components to different page`
- `clears pageId when removing all components`
Both expected `report.pageId` to track the association set:
- Swap components to a different page → `pageId` moves.
- Empty the association set → `pageId` becomes null.
The service was enforcing a stricter invariant — it *threw* a
`ConflictError` when the new components' page differed from the
report's current page, and left `pageId` untouched when associations
were cleared. That was an overcautious choice in the initial
migration; no caller today wants a "report pinned to page X
regardless of its components" guarantee.
Simplified to: whenever `pageComponentIds` is supplied,
`updateValues.pageId = validated.pageId`. The zero-component case
sets `pageId = null`; the one-page case moves the report to that
page. Mixed-page inputs are still rejected by
`validatePageComponentIds`'s `componentPageIds.size > 1` check, so
the only reachable states here are "one page" or "no components".
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
---------
Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
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.
Summary
Stacked on #2101. Adds
packages/services/package.jsonandpackages/importers/package.jsonto theinstallstage bind list inapps/server/dofigen.ymlsobun install --frozen-lockfilecan resolve the workspace dep graph inside the Docker build.Without this fix:
`packages/importers` already exists in the repo and isn't used by `apps/server` directly at this commit, so binding it early is a no-op for bun until services starts pulling it in downstream.
Why this branches from #2101 and not from main
At the time of writing, none of the services-migration stack (#2100–#2112) has merged yet. Main has no `packages/services/` directory, so a bind entry for it would point at nothing. The fix has to ride with the PR that first requires it — that's #2101.
Why not amend #2101 directly
Force-pushing #2101 would require rebasing all ten downstream PRs. This sidecar PR stacks on top of #2101 instead: it inherits the services scaffolding it needs, adds the dofigen entries, and merges right after #2101 lands on main without touching any other stacked PR.
Files
Out of scope
Other apps (`apps/dashboard`, `apps/status-page`, `apps/workflows`, `apps/private-location`) have their own `dofigen.yml` files and likely need similar updates (`packages/api` now transitively depends on `@openstatus/services`). Those are separate fixes per your scope.
Test plan
🤖 Generated with Claude Code