fix(groups): accept a null filterConditions on create and stop dropping static deviceIds (#3159)

[ToddHebebrand]

What was broken

Creating a static device group from the dashboard failed with a 400 on every attempt, reported in #3159 by @cisspUser01 against 0.103.0.

The create form builds one payload for both verbs and sent filterConditions: null for a static group, while createGroupSchema declared the field .optional() without .nullable():

{"error":"filterConditions: Invalid input: expected object, received null"}

The update schema three lines below it had always been .nullable().optional(). That asymmetry is why editing a group worked and creating one never could.

Why this fix, rather than just loosening the schema

Both sides, for different reasons — and the divergence itself is the defect.

• The schema divergence is the bug, not the symptom. filterConditions is now ONE shared constant (groupFilterConditionsField) consumed by both createGroupSchema and updateGroupSchema, so the two cannot drift apart again without editing the constant that documents why they must match. All three forms — omitted, an object, explicit null — behave identically on both verbs.
• null had to be accepted, not just avoided. This is a public HTTP API (mobile, MCP, partner scripts). null is a legitimate way to say "no filter", the create route already normalized it (payload.filterConditions ?? null), and rejecting it only on create was arbitrary. Widening is backward compatible.
• The client still had to change, but only on create. The form now omits the key for a static create (matching CreateGroupModal, which has always done it correctly, and the documented payload). It deliberately keeps sending explicit null on edit: the update route reads undefined as "leave the filter alone" and null as "clear it", so omitting it there would strand a stale filter on a group the user just converted from dynamic to static.

The silent failure hiding behind the 400

Fixing only the null would have traded a loud 400 for something worse. createGroupSchema had no deviceIds field at all, so Zod silently stripped the devices the user had just hand-picked and returned 201 on an empty group.

Create now accepts deviceIds and materializes the membership in the same request, and rejects a non-empty list on a dynamic group rather than ignoring it.

Because that is a cross-tenant write, the ownership guard POST /:id/devices carried inline is extracted into validateManualMembershipDevices + addManualGroupMemberships in services/groupMembership.ts and shared by both routes — one implementation of the tenancy check instead of two that can drift. It enforces org ownership against the group's org, the site boundary for a site-bound group, and dedupe; membership rows take their orgId from the group, never from the request.

Ordering matters: the batch is validated before the group row is inserted. A return c.json(..., 400) is a normal response, not an exception, so the request transaction still commits — validating late would leave a stranded empty group behind on every rejected batch. Three tests assert db.insert was never called on the rejection paths.

Two adjacent defects fixed in passing

• The POST URL was being read out of the i18n catalog: t("deviceGroupsPage.deviceGroups"), whose value was the literal string /device-groups in all seven locales. A codemod accident that worked only for as long as no translator touched that entry. Restored to a literal and the key removed from the catalogs.
• The failure branch discarded the response body and threw a fixed "Failed to save device group", which is why the reporter had to dig the real 400 out of devtools. It now surfaces the server's message (the pattern CreateGroupModal already uses).

What I verified about the rest of #3159

• Bug 1 (dynamic membership never persisted) is already fixed on origin/main by fix(api): materialize dynamic group membership instead of stranding it on a dead transaction #3181 — the evaluation is awaited at both call sites with the transaction-race explained in-place, and covered by groups_get_create.test.ts. Not re-done here.
• The Groups-list page crash is also already fixed: the legacy matcher now lives in deviceGroupMatching.ts, fully null-hardened, and getGroupDeviceIds no longer runs it for filter-authored groups at all — an empty-membership dynamic group can no longer reach it. deviceGroupMatching.test.ts already regression-covers undefined hostname, missing osType, and malformed rules. Nothing left to harden.

Known adjacent bugs NOT in this PR (deliberately)

Found while tracing this page; both are separate live defects and bundling them would bury a security-sensitive membership change:

1. Group editing and drag-and-drop assignment are dead. The page sends PUT /device-groups/:id, but the router only defines .patch('/:id') — no PUT handler exists anywhere, so Hono falls through to the 404 handler. Needs a focused PR with update-transition tests.
2. POST /device-groups/bulk does not exist, so the bulk script/policy actions on that page 404 too. That one needs an API design decision, not a patch.

Also noted: deviceGroupsPage.put, .post and .windows are orphaned codemod debris in the catalogs, already unreferenced before this change, so left alone.

Tests

Suite	Result
API group routes + membership service (11 files)	157 passed
DeviceGroupsPage.staticGroups.test.tsx (new, 4 cases)	4 passed
Device-groups web suites (4 files)	24 passed
i18n guards incl. locale parity (6 files)	97 passed
no-silent-mutations	100 passed
tsc --noEmit (apps/api)	clean
astro check (apps/web)	0 errors
eslint (all 8 touched files)	clean

New coverage: 8 route cases for the accepted/rejected create shapes, 9 unit cases for the extracted helpers, 4 web cases pinning the create/edit payloads, the literal URL and the surfaced error. Verified non-vacuous by reverting the implementation — 14 API tests and 3 of 4 web tests fail against the unpatched code.

Three sibling test files mocked the membership service and would have 500'd after the extraction; they now pass the real helpers through, so their pre-existing cross-org and site-confinement assertions keep exercising the shared code rather than a mock.

Refs #3159 rather than Closes — leaving it open for @cisspUser01 to verify.

🤖 Generated with Claude Code

Review round (4 agents) — outcome

/pr-review-toolkit:review-pr ran code-reviewer, pr-test-analyzer, silent-failure-hunter and comment-analyzer. 0 critical. What was raised and what I did:

Acted on:

• Type Check failure (CI): two noUncheckedIndexedAccess errors on mock.calls[0][0] in the new create tests. Narrowed with an explicit guard rather than !/as any — fixed in c49fd86.
• Coverage gaps (2): addManualGroupMemberships collapsing a repeated device id, and the device_group.device.add audit event the create path writes (incl. the viaGroupCreate marker). Both added in c49fd86.

Rejected, with reason:

• code-reviewer asked for a try/catch around the static membership insert, mirroring the dynamic branch, arguing a post-insert throw would strand a created group. It would not — the whole handler runs inside one baseDb.transaction opened by withDbAccessContext, so a throw rolls the group insert back and yields a retry-safe 500 with Sentry context. silent-failure-hunter independently reached the same conclusion and pointed the finding the other way: the dynamic branch's swallow is the questionable one, and the pre-existing comment's "would invite a duplicate on retry" rationale does not hold under this transaction model. That branch predates this PR and has a test pinning its 201-and-log behavior, so I left it alone rather than change behavior outside [Device Groups] Two bugs: (1) dynamic group membership never persists after creation, causing the Groups list page to crash; (2) static group creation always fails — frontend sends filterConditions: null, which the backend schema rejects #3159. Worth its own look.

Deferred deliberately (documented, not silently dropped):

• No .onConflictDoNothing() on the manual membership insert, unlike evaluateGroupMembership's. The SELECT-then-INSERT race is pre-existing and only reachable on POST /:id/devices (a brand-new group id has no competing writer), and fixing it with honest added counts means reworking the insert's return handling plus several test mocks.
• Manual adds do not write group_membership_log — only a route-level audit event. Also pre-existing on POST /:id/devices; closing it in the shared helper would change that endpoint's observable behavior, which this PR otherwise preserves exactly.
• The web form discards invalidDevices from a 400, so the user sees the failure class but not which device caused it. Rendering it needs new user-facing copy across seven locales, which is more than this fix should carry.

Verified accurate by comment-analyzer: the transaction-commit claim behind the validate-before-insert ordering, the undefined vs null semantics on PATCH, and the shared-helper claim were each traced to the implementing lines.

On test counts, precisely: 21 new cases, of which 17 fail against unpatched code (14 API + 3 web). The other 4 are baseline non-regression checks (omitted key, object form, empty array, and the edit-path null) that passed before too — they document the contract rather than the fix, and are commented as such.

Related issues

• [UI][API] Device group creation 400s for every multi-org partner — create route reads orgId from the body only, web sends it as a query param #3425 — for a partner with 2+ orgs, create still 400s with orgId is required when partner has multiple organizations: the route reads orgId from the body only while the web client sends it as a query param. Reproduces only with multiple orgs, which is why [Device Groups] Two bugs: (1) dynamic group membership never persists after creation, causing the Groups list page to crash; (2) static group creation always fails — frontend sends filterConditions: null, which the backend schema rejects #3159's reporter never hit it. Deliberately not fixed here — scoped to [UI][API] Device group creation 400s for every multi-org partner — create route reads orgId from the body only, web sends it as a query param #3425.
• [Web][i18n] API route paths live in the locale files as translatable values — one translator edit silently breaks a feature in that locale #3426 — the remaining instances of the API-path-in-locale-files class. This PR fixes only the one call site it touches and removes that one key from the seven catalogs.

[cloudflare-workers-and-pages]

Deploying breeze with    Cloudflare Pages

Latest commit:	c49fd86
Status:	✅  Deploy successful!
Preview URL:	https://8618d421.breeze-9te.pages.dev
Branch Preview URL:	https://fix-3159-static-group-create.breeze-9te.pages.dev

View logs

[ToddHebebrand]

Review run: /pr-review-toolkit:review-pr — code-reviewer, pr-test-analyzer, silent-failure-hunter, comment-analyzer (4 agents, parallel).

Findings: 0 critical. 3 acted on, 1 rejected on the merits, 3 deferred with reasons — all recorded in the PR body.

• Acted: the CI Type Check failure (two noUncheckedIndexedAccess errors on mock.calls[0][0], narrowed with an explicit guard rather than !/as any), plus the two coverage gaps review found — the repeated-device-id dedupe and the device_group.device.add audit event including its viaGroupCreate marker. All in c49fd86.
• Rejected: a request to wrap the static membership insert in try/catch "so a post-insert throw cannot strand a created group." It cannot: the handler runs inside the single baseDb.transaction opened by withDbAccessContext, so a throw rolls the group insert back and produces a retry-safe 500. A second reviewer reached the same conclusion independently and inverted the finding — the pre-existing dynamic branch's swallow is the doubtful one, and its "would invite a duplicate on retry" rationale does not hold under this transaction model. Left untouched (it predates this PR and has a test pinning its behavior); flagged as its own follow-up.
• Deferred: the missing .onConflictDoNothing() on the manual insert, manual adds not writing group_membership_log, and the web form discarding the invalidDevices array. Each is pre-existing on POST /:id/devices and each would either change that endpoint's observable behavior or add user-facing copy across seven locales.

Tests: all 54 checks pass, 1 skipping — Test API, Test Web, Type Check, Lint and all four Integration Tests shards green. Locally: 157 API tests across 11 group-related files, 24 device-groups web tests, 97 i18n guards (incl. locale parity, since 7 catalogs changed), no-silent-mutations 100, tsc --noEmit clean, astro check 0 errors, eslint clean.

Of 21 new cases, 17 fail against unpatched code (14 API + 3 web), verified by reverting the implementation; the other 4 are baseline non-regression checks and are commented as such.

Status: review-clean, CI green, awaiting maintainer merge. Left as Refs #3159 rather than Closes so @cisspUser01 can verify the static-group flow first. Related: #3425 (partner-multi-org orgId query-param bug, deliberately out of scope) and #3426 (remaining API-paths-in-locale-files).