feat(fields): add FieldPolicy model, layered resolver, and batch resolve action - #2230
Merged
Conversation
…lve action (#2047) New package @happyvertical/smrt-fields — the field-policy store and resolution engine for epic #2045, following the prompts/features/languages architecture family: - FieldPolicy (_smrt_field_policies): sparse override rows keyed (objectRef, fieldName, scopeType, scopeKey) with nullable defaultValue/visibility/help/label/displayOrder/locked and an updatedBy audit ref. scopeKey (userId ?? tenantId ?? '__app__') exists only to keep the conflict-column unique index total (PromptOverride.context trick); tenantId/userId are typed UUID refs (@TenantID / @crossPackageRef). - Write-time validation against the live ObjectRegistry: unknown objectRef/fieldName rejected, defaults type-checked, security rail (transient/sensitive/readPermission, top-level AND _meta) refuses stored defaults, scope-shape and org-lock enforcement, required-field invariant on visibility demotion, and a fail-closed tenant-context write boundary. - resolveFieldPolicy / resolveFieldPolicyExplained: code seed (manifest defaults, description help, _meta.ui incl. cold-start basic rule) → app rows → tenant hierarchy walk (injected loader mirroring smrt-features; default loader dynamic-imports smrt-users, flat fallback without it) → user tier (defaults AND visibility). Resolver-side required-field safety net forces basic visibility when no usable default resolves; effective org lock skips the user layer. Per-(db, objectRef, tenant, user) TTL cache invalidated coarsely per objectRef on save/delete. - FieldPolicyCollection.resolveBatch: custom collection-scoped POST action (path 'resolve', explicit api include; mcp/cli closed) resolving a set of objectRefs for the ambient tenant-context identity only, with sensitive/readPermission-gated/transient fields absent from responses. The sort-order column is displayOrder (an unquoted `order` column is an SQL keyword hazard in the runtime INSERT path); resolved output exposes `order`. Note: _smrt_field_policies rows do NOT ride the client change feed — core's change-feed writer skips _smrt_-prefixed tables and its emit side is private, so live client invalidation remains a core-side decision. New-package wiring: .changeset fixed group, root tsconfig references, root AGENTS.md orientation, package AGENTS.md/CLAUDE.md, __smrt-register__ self-registration, vite/vitest configs with smrtVitestPlugin. Closes #2047
…2047) Address all 8 codex review findings on the initial smrt-fields commit: - P1-1: no generated surface exposes read verbs. API include drops list/get (they would enumerate every tenant's and user's rows on this non-@TenantScoped model); the model CLI is writes-only (the generated CLI invokes over HTTP and the cli-api coherence gate rejects entries without routes); the runtime CLI/MCP surfaces are closed via the collection config, which core makes the runtime registry authority for the item class (ContentContributions precedent). The collection api include mirrors the model posture plus resolveBatch. Reads flow through the context-scoped batch resolver and server-side resolver APIs. Pinned by a new generated-surfaces test driving APIGenerator (list/get 405, create/update/delete open), MCPGenerator, and CLIGenerator. - P1-2: delete() authorizes the caller against the PERSISTED row scope before deleting, with the same fail-closed rules as saves (tenant rows only for the context tenant, user rows only for the context user, app rows never inside a non-bypass tenant context). - P1-3: save() authorizes against the persisted identity BEFORE accepting any mutation, so a foreign row cannot be re-scoped into the caller's tenant/user (the identity-change path would have deleted the original). getPersistedIdentity now carries the persisted tenantId/userId. - P1-4: the missing-smrt-users predicate now walks the full error cause chain and matches ERR_MODULE_NOT_FOUND plus Node's "Cannot find package" message, so standalone consumers get the flat-tenant fallback; exported for direct testing with fabricated error shapes. - P2-5/P2-6: write-time org checks (user-write lock enforcement and the required-field demotion default) are computed by the RESOLVER over the org tiers, so cascading ancestor-tenant locks and defaults are honored at save time instead of only direct-tenant rows; the resolver-side safety net stays authoritative on updates (no self-exclusion). - P2-7: tenant chain nodes discarded by a permission-inheritance break are now excluded from merging AND the explained layer contributions (break points are chain-structural), so sequentially replaying the listed deltas reproduces the merged policy — pinned by a replay helper across all fields. - P2-8: STI meta storage fields are rejected at the write boundary (the resolver excludes them, so rows would silently never apply). Regression tests for every finding (32 -> 40 package tests). Refs #2047
…e batch action (#2047) Address the four round-2 codex findings: - P1 (context-absent bypass): the scope-ownership boundary no longer returns early when no tenant context exists. Without ANY ambient identity (tenancy ALS never entered — e.g. an APIGenerator deployment whose auth middleware only populates request locals), tenant- and user-scope writes/deletes are rejected outright as unattributable; app-scope rows remain the server-side/ops path. In-context enforcement is unchanged. Residual documented in AGENTS.md: ALS-less deployments can still write app rows until #2049 adds fields:policy:manage. Test setup now seeds tenant/user rows under bypass contexts like real ops flows. - P2-1 (runtime transport): core's APIGenerator gains a narrow decorator-metadata dispatch for custom COLLECTION-scoped actions (single-segment paths, api.include-gated, method-matched, 501 when a declared route has no implementation, results sanitized through toPublicJSON like the SvelteKit transport's toPublicResult). Without it, POST /<collection>/resolve degraded into handleCreate with the action segment discarded. Exercised end-to-end in the fields generated-surfaces test: 200 + gated payload, wrong-method 405, invalid-JSON 400. - P2-2 (UUID reference defaults): foreignKey/crossPackageRef defaults must be UUID strings unless the field declares idType 'text' — the columns are native UUID on PostgreSQL/DuckDB and a non-UUID default would only fail at insert time. - P2-3 (over-broad fallback predicate): isMissingUsersDependency now parses the missing-module TARGET from Node's quoted specifier (full cause chain) and falls back only when the target IS smrt-users or a subpath; a transitive failure inside an installed smrt-users (users path as importer) or a missing dist file rethrows instead of silently losing ancestor locks/defaults. Regression tests for every finding (43 package tests, was 40). Refs #2047
…2047) Review of the #2047 slice surfaced two blocking defects plus a set of smaller hardening items. All are fixed here with regression coverage. P1-1 userless-context ownership bypass. `assertScopeOwnedByAmbientContext` guarded the user tier with `context.userId !== undefined && ...`, so the check VANISHED for any ambient context without a user id. Tenancy adapters produce exactly that shape whenever no `resolveUserId` hook is configured (API-key auth, service principals, background jobs, a bare `withTenant({ tenantId })`), and user rows are `tenantId: null` by design, so nothing else contained the write: such a caller could create, re-scope, or delete ANY user's rows in ANY tenant, with the generated PUT echoing the row back as a read primitive. The package rule is now that a missing identity component DENIES rather than skips, applied on the write side and mirrored on the read side in `assertResolutionAllowedInContext` (public `resolveFieldPolicy` export). Context-LESS reads stay allowed: that is the trusted server-side path, and `resolveBatch` never lets a request body select a user. P1-2 tenant tier write-dead over every generated surface. Core's mass-assignment guard treats `tenantId` as server-managed and strips it from every create/update body, while `FieldPolicy` is deliberately not `@TenantScoped` so the tenancy interceptor never repopulates it. A `POST {scopeType:'tenant', tenantId}` therefore always reached scope-shape validation with `tenantId === null` and threw a 500, making the org-admin tier unreachable from REST/SvelteKit. Fixed model-side rather than in core: `save()` now derives a missing tenant/user owner from the ambient context. That grants nothing, because the ownership guard already pinned the value to the ambient one, and it keeps the model as the single validation authority instead of adding an escape hatch to a guard every package depends on. A bypass caller writing another tenant's row still needs a server-side model call. Also in this cycle: - Server-side `updatedBy` stamping pulled forward from the #2050 branch, so audit attribution is not forgeable through the open write routes before #2051 consumes it. - The persisted-scope guard now falls back to a NATURAL-key lookup. Every generated create mints a fresh UUID, so the primary-key lookup always missed while the `conflictColumns` upsert still replaced the occupant. - `isRequiredField` reads `nullable` from the top level as well as `_meta`, matching how `required` was already read from both. - The resolver cache key includes tenant-hierarchy loader identity; an injected loader was being served the default loader's ancestor chain. - AGENTS.md records the deny-on-missing-identity rule, scope attribution, that `withSystemContext()` does not unlock org/user writes, and that the model's cli/mcp config is dead at runtime. Tests: five groups from the review brief. The tenant-tier regression drives one row per scope tier through `APIGenerator`, not `policies.create()` — model-level tests are exactly what hid this. Refs #2047
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: cb323ec2a0
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Contributor
There was a problem hiding this comment.
Pull request overview
Introduces the first slice of the smrt-fields epic by adding a new @happyvertical/smrt-fields package that persists sparse per-field policy overrides (FieldPolicy), resolves effective policy via a layered resolver (code seed → app → tenant chain → user), and exposes a gated resolveBatch custom collection action for client bootstrapping. It also updates core’s runtime APIGenerator to dispatch single-segment, decorator-declared collection-scoped custom actions (e.g. POST /<collection>/resolve) before CRUD routing so the runtime transport matches generated SvelteKit behavior.
Changes:
- Added
@happyvertical/smrt-fieldspackage withFieldPolicymodel, validation/ownership guards, resolver (+ explained variant), TTL cache, andresolveBatchaction. - Added extensive regression/unit tests covering isolation rules, required-field invariant safety net, lock semantics, cache invalidation, and runtime surface exposure.
- Updated core REST runtime generator to support decorator-declared custom collection actions dispatch.
Reviewed changes
Copilot reviewed 22 out of 24 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| tsconfig.json | Adds packages/fields to TS project references. |
| pnpm-lock.yaml | Adds lockfile entries for the new packages/fields workspace importer. |
| packages/fields/vitest.config.ts | Test runner configuration for the new package (SMRT vitest plugin, single fork). |
| packages/fields/vite.config.ts | Library build config wiring for the new package. |
| packages/fields/tsconfig.json | Package TS build configuration (composite build, dist outDir). |
| packages/fields/src/types.ts | Public types for scope/visibility, resolved policy shapes, and resolver/batch contracts. |
| packages/fields/src/models/FieldPolicy.ts | FieldPolicy model with write-time validation, ownership/isolation enforcement, upsert/identity-change handling, and cache invalidation. |
| packages/fields/src/index.ts | Package entrypoint exports and manifest self-registration. |
| packages/fields/src/generated-surfaces.test.ts | Ensures generated REST/CLI/MCP surfaces are closed appropriately and runtime action dispatch works. |
| packages/fields/src/field-policy.test.ts | Model-level validation and security/isolation regression coverage. |
| packages/fields/src/field-policy-resolver.ts | Layered resolver + explained variant, tenant-hierarchy loading, lock and required-field safety net, caching. |
| packages/fields/src/field-policy-resolver.test.ts | Resolver behavior tests including hierarchy, cache keying, isolation, and explain replay contract. |
| packages/fields/src/field-policy-batch.test.ts | Tests for resolveBatch input validation, identity sourcing, and public field gating. |
| packages/fields/src/field-definitions.ts | Live-ObjectRegistry helpers for type/security checks and code-seed derivation. |
| packages/fields/src/collections/FieldPolicyCollection.ts | Collection wrapper providing row fetch helpers and the resolveBatch custom action with fail-closed gating. |
| packages/fields/src/cache.ts | 30s TTL cache keyed by db identity + loader identity, with prefix invalidation. |
| packages/fields/src/smrt-register.ts | Package manifest self-registration bootstrap (issue #1132 pattern). |
| packages/fields/README.md | Package-level usage overview and basic examples. |
| packages/fields/package.json | New package manifest (exports, scripts, deps). |
| packages/fields/CLAUDE.md | Adapter pointing to AGENTS.md. |
| packages/fields/AGENTS.md | Canonical architecture and invariants documentation for the new package. |
| packages/core/src/generators/rest.ts | Adds runtime dispatch for decorator-declared collection-scoped custom actions and result sanitization. |
| AGENTS.md | Adds fields to the root orientation list. |
| .changeset/config.json | Adds @happyvertical/smrt-fields to the fixed version group. |
Files not reviewed (1)
- pnpm-lock.yaml: Generated file
…act (#2047) An independent review of the slice found one schema defect and three ways the new runtime REST custom-action dispatcher diverged from the contract every other transport already shares through `generators/custom-action.ts`. Stray unique index on the policy table. A decorated collection emits its OWN manifest schema for the item's table, and `FieldPolicyCollection` did not repeat FieldPolicy's `conflictColumns` — so its schema fell back to SmrtObject's default unique `(slug, context)` index. Manifest-driven consumer migrations aggregate both schemas onto `_smrt_field_policies`, where that index rejects legitimate layered rows: every policy row has a NULL slug and context, and the app, tenant, and user rows for one field are distinct only by the real natural key. The runtime registry cannot surface this because `getAllSchemas()` is keyed by table name, so the two schemas collapse into a single entry — the new test asserts against the generated manifest instead. Scope was read off the route alone. `ApiCustomRouteConfig.scope` is OPTIONAL and documented to default to `collection` for statics and `item` for instance methods, but the dispatcher required a literal `scope: 'collection'`. A valid static action that omitted it was skipped, so `POST /<collection>/<action>` fell through to CRUD and created an object wherever `create` is exposed. Scope now resolves through `resolveCustomActionMetadata`, with the receiver deciding — a route-only scope cannot manufacture one. An action with no collection-hosted receiver falls through as before unless the route explicitly declared `scope: 'collection'`, which stays a 501 so an action request never degrades into a write. Arguments ignored declared parameters. Every action was invoked with exactly one options object. Now `buildCustomActionInvocationArgs` projects them as the generated SvelteKit, CLI, and MCP transports do, so `action(a, b)` receives two arguments instead of `{a, b}` and `undefined`. Behaviour is unchanged wherever parameter metadata is absent, which is currently every collection-hosted action. Returned failures were served as 200. An action returning the shared `{ ok: false, code, message, status }` convention had the raw object wrapped as a success. `normalizeCustomActionFailure` now applies first, yielding the requested non-2xx status and the redacted payload. Relatedly, an ABSENT request body is no longer conflated with a malformed one: a zero-parameter action is legitimately called with no body, while a body that is present but unparseable stays a 400. Adds `rest-custom-actions.spec.ts` — the dispatcher had no core-side coverage. Refs #2047
`FieldPolicyOptions.defaultValue` was documented as "JSON-encoded default
value (string), or a plain value to be serialized" and implemented to match:
a string was assigned as-is on the assumption it was already encoded, and
anything else was serialized. Strings are the most common default type for
text fields and are the one plain value that branch never reached, so the
natural call — `{ defaultValue: 'Net 30' }` — stored unparseable text and only
failed later, at the JSON.parse in validation.
The ambiguity is inherent to one option carrying both meanings: `'"TBD"'` and
`'TBD'` are indistinguishable, so no sniffing rule can be correct. The channel
has to be explicit.
Which channel keeps which meaning is settled by the callers, not by taste.
Every existing caller passes PRE-ENCODED values: the eleven in-package test
sites, and — decisively — #2049/#2050's gear, whose `planFieldSettingsWrite`
posts `JSON.stringify(draft.defaultValue)` into a `FieldPolicyRowPatch`
typed `defaultValue: string | null`, straight through the generated write
routes to this constructor. Auto-serializing `defaultValue` would therefore
double-encode every gear write across the rest of the epic. So `defaultValue`
keeps the encoded meaning (now typed `string | null` rather than `unknown`,
so the type states it) and the PLAIN value gets the new explicit path:
- `defaultValueRaw` — any value, always serialized, strings included; the
constructor-option twin of the existing `setDefaultValue()` method.
- Supplying both throws rather than silently resolving a precedence.
- The parse failure now names the fix instead of only reporting bad JSON, so
the natural-looking call corrects itself at the point of use.
Storage and every read site are unchanged and stay symmetric: the column
remains JSON-encoded, `getDefaultValue()` and the resolver's `rowToDelta`
still decode it, and `normalizeDefaultValueForPersistence` still treats a
directly-assigned string as already-encoded.
Adds a regression test round-tripping a plain string through save → resolve,
covering the literal-looking-like-JSON case ('null'), the encoded channel
staying encoded, and both rejection paths.
Refs #2047
github-merge-queue
Bot
removed this pull request from the merge queue due to failed status checks
Aug 5, 2026
Contributor
|
Removed from the merge queue — reason: CI_FAILURE A required status check went red on the merge-group head. Open the linked run and read the failing leaf job (shard), not the rollup — the rollup only mirrors it. Latest merge-group validation run for this PR: https://github.com/happyvertical/smrt/actions/runs/31031899003 merge-queue-watchdog: dequeue events are otherwise invisible; this comment makes queue ejections diagnosable (#2197). |
This was referenced Aug 5, 2026
Contributor
|
Removed from the merge queue — reason: MANUAL Someone removed this entry. Removal also restarts validation for every entry that was queued behind it. Latest merge-group validation run for this PR: https://github.com/happyvertical/smrt/actions/runs/31046291621 merge-queue-watchdog: dequeue events are otherwise invisible; this comment makes queue ejections diagnosable (#2197). |
Contributor
Author
|
Removed from the merge queue so the CI-lane fix (#2242) can go first — not a judgement on this PR. Two reasons: its queue validation was consuming the metal fleet that #2242's own validation needs, and it was ahead of #2242 in line while carrying the exact defect #2242 fixes. This PR was already ejected once at 19:30Z by Re-queue once #2242 lands and this should get a clean run. |
Contributor
|
Removed from the merge queue — reason: MANUAL Someone removed this entry. Removal also restarts validation for every entry that was queued behind it. Latest merge-group validation run for this PR: https://github.com/happyvertical/smrt/actions/runs/31066045819 merge-queue-watchdog: dequeue events are otherwise invisible; this comment makes queue ejections diagnosable (#2197). |
willgriffin
added a commit
that referenced
this pull request
Aug 6, 2026
Updating this branch to current main brought in packages/fields from #2230, which the lockfile predates: [ERR_PNPM_OUTDATED_LOCKFILE] pnpm-lock.yaml is not up to date with <ROOT>/packages/fields/package.json Regenerated with pnpm install --lockfile-only.
willgriffin
added a commit
that referenced
this pull request
Aug 6, 2026
… app-cli timeouts (#2245) * fix(app-cli): give the suite explicit test and hook timeouts bridge.test.ts spawns tsx on a fixture through StdioClientTransport and completes an MCP handshake before asserting. The package declared no testTimeout, so that ran against vitest's 5s default. It passed locally and on the affected lane, then timed out three times in the merge queue -- where mode: full actually runs this package -- and ejected an unrelated CI-only PR. Set both budgets explicitly at 30000, the value the rest of the workspace uses; hooks do not inherit testTimeout. Closes #2243 * fix(deps): stop Renovate bumping androidx past the compileSdk 36 pin androidx.core 1.18+ and androidx.lifecycle 2.11+ require compileSdk 37, and both seed apps are pinned to compileSdk 36. The catalogs recorded that coupling only in comments, which Renovate cannot read, so every weekly update bumped core-ktx and lifecycle straight past it and left :sample:checkDebugAarMetadata failing the whole PR. Encode the constraint as two allowedVersions rules so the pin survives the weekly run, matching how renovate.json already expresses the template-package pin. Closes #2234 * fix(deps): restore brace-expansion's lower advisory bound GHSA-rgw5-rvv9-x895 starts at brace-expansion 4.0.0, but #2203 widened the selector to a bare `@<5.0.9`, which also matches every 1.x, 2.x, and 3.x dependency. A consumer of an older major would be forced onto 5.0.9 — an unrelated breaking upgrade for a release outside the advisory. Both reviewers raised this against the identical selectors in #2205; the undici and fast-uri cases landed separately in #2210 with their advisory lower bounds intact. This applies the same correction to the third one. Fixing the upper bound is what keeps the override from going stale as the advisory widens; keeping the lower bound is what stops it reaching past the advisory. The two are independent and both are required. Verified neutral: `pnpm audit` reports `1 low | 6 moderate` before and after, brace-expansion appears in neither report, and the only lockfile change is the mirrored override key — no package resolution moves. Closes #2204 * chore(deps): sync sdk packages to v0.85.5 * chore(deps): refresh the lockfile for packages/fields Updating this branch to current main brought in packages/fields from #2230, which the lockfile predates: [ERR_PNPM_OUTDATED_LOCKFILE] pnpm-lock.yaml is not up to date with <ROOT>/packages/fields/package.json Regenerated with pnpm install --lockfile-only. --------- Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.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
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
First slice of the smrt-fields epic (#2045): the
FieldPolicymodel, the layeredapp → tenant → user resolver over the #2046 code seed, and the
resolveBatchcollection action that client bootstrapping (#2048) consumes.
FieldPolicy(_smrt_field_policies) — sparse override rows keyed(objectRef, fieldName, scopeType, scopeKey). A NULL column means "inheritfrom the lower layer", so resetting a customization is a row DELETE and later
lower-layer changes flow through. Writes validate against the live
ObjectRegistry: unknown objects/fields rejected, defaults type-checked,and the security rail (
sensitive/readPermission/transient) refusesstored defaults outright.
resolveFieldPolicy/resolveFieldPolicyExplained— code seed → approws → tenant chain (hierarchy walk root → leaf, permission-inheritance
breaks discard earlier ancestors) → user rows, with a 30s TTL cache. The
explained variant returns ordered per-layer contributions that replay to the
merged result, so smrt-fields: ObjectForm generator + form-settings gear (org/user policy editor) #2049/smrt-fields: defaults control panel — SettingsCatalog roll-up + AdminShell wiring #2050 UIs never re-derive precedence.
needs a usable resolved default, enforced at write time AND re-enforced at
resolution (a different row's later deletion can invalidate what held).
list/getanywhere: this model isdeliberately not
@TenantScoped, so a generated read would enumerate everytenant's and user's rows. Reads go through the context-scoped
resolveBatchaction or the server-side resolver.
APIGeneratorgains decorator-route dispatch forsingle-segment collection-scoped custom actions, so
POST /<collection>/resolveworks on the runtime transport as it already did on generated SvelteKit routes.
Parent: #2045 (epic stays open).
Review fixes in this cycle
A read-only review of the slice returned needs-fixes with two P1s. Both are
fixed here, each with a regression test that fails without the fix (verified by
reverting the fix and re-running).
P1-1 — userless-context user-scope ownership bypass.
assertScopeOwnedByAmbientContextguarded the user tier withcontext.userId !== undefined && scope.userId !== context.userId, so the checkVANISHED for any ambient context without a user id. Tenancy adapters produce
exactly that shape whenever no
resolveUserIdhook is configured (API-key auth,service principals, background jobs, a bare
withTenant({ tenantId })), anduser rows are
tenantId: nullby design, so nothing else contained the write:such a caller could create, re-scope, or delete ANY user's rows in ANY tenant,
with the generated PUT echoing the row back as a read primitive. The package
rule is now a missing identity component DENIES, it never skips, applied on
the write side and mirrored on the read side in
assertResolutionAllowedInContext(the public
resolveFieldPolicyexport). Context-LESS reads stay allowed —that is the trusted server-side path, and
resolveBatchnever lets a requestbody select a user.
P1-2 — the tenant tier could not be created through any generated write surface.
Core's mass-assignment guard treats
tenantIdas server-managed and strips itfrom every create/update body, while
FieldPolicyis deliberately not@TenantScopedso the tenancy interceptor never repopulates it. APOST {scopeType:'tenant', tenantId}therefore always reached scope-shapevalidation with
tenantId === nulland returned 500 — #2050's entireorg-admin tier was unreachable from REST/SvelteKit, and it went unnoticed
because the model-level tests call
policies.create()directly.Fixed model-side, not in core:
save()now derives a missing tenant/userowner from the ambient context. This grants nothing — the ownership guard
already pinned the value to the ambient one, so the derived value is the only
value that could ever have been accepted — and it keeps the model as the single
validation authority instead of adding an escape hatch to a guard every package
in the monorepo depends on.
packages/coreis unchanged by this fix.Also in this cycle:
updatedBystamping pulled forward from the smrt-fields: defaults control panel — SettingsCatalog roll-up + AdminShell wiring #2050 branch, soaudit attribution is not forgeable through the open write routes before smrt-fields: usage capture + promotion suggestions (learning loop) #2051
consumes it. A client-supplied value is overwritten inside any ambient
context; context-less/system flows keep what they set.
generated create mints a fresh UUID, so the primary-key lookup always missed
while the
conflictColumnsupsert still replaced the occupant.isRequiredFieldreadsnullablefrom the top level as well as_meta,matching how
required— andsensitive/readPermission/transient—were already read from both. Defensive only: on the scanner-manifest path
nullablealways lands in_meta(verified), so this covers theruntime-decorator registration path used by consumer packages without a
baked manifest. No behaviour change is observable in-repo, so it carries no
dedicated test.
injected loader was being served the default loader's ancestor chain.
withSystemContext()does not unlock org/user writes (seeds needsuperAdminBypass), and that the model'scli/mcpconfig is dead atruntime because the registry re-registers the item slot with the
collection's config.
Five test groups from the review brief were added. The P1-2 regression drives
one row per scope tier through
APIGenerator, never throughpolicies.create()— model-level tests are exactly what hid the defect.
Accepted design residuals
Recorded rather than fixed here, deliberately:
resolveBatchis authorized under thepostverb, identical tocreate.An app cannot grant bootstrap-read without also granting policy writes. It is
a property of how core authorizes collection-scoped custom actions, not of
this package; changing it is a core-wide auth decision.
resolveBatch's inputvalidation and
TenantIsolationErrorboth become generic 500s because theresolveErrorHttpStatusopt-in rail does not exist at this landing point.smrt-fields: ObjectForm generator + form-settings gear (org/user policy editor) #2049 adds it; the 400/403 mapping belongs in that cycle, not this one.
applyWritablePolicy'sserverManagedset is unconditional and duplicated insveltekit-generator.ts. Solving P1-2 model-side avoids touching either copy;if a second model ever needs this, the escape hatch belongs in core.
still strips
tenantId, and derivation only fills from the ambient context.Cross-tenant admin writes go through a server-side model call.
Independent review cycle
A codex review (gpt-5.6-sol, xhigh) of the full diff returned four findings,
all in the slice's pre-existing code rather than the fixes above. All four were
verified against the source and fixed in
123e14398:its OWN manifest schema for the item's table, and
FieldPolicyCollectiondidnot repeat FieldPolicy's
conflictColumns— so its schema fell back toSmrtObject's default unique
(slug, context)index. Manifest-driven consumermigrations aggregate both onto
_smrt_field_policies, where that indexrejects legitimate layered rows: every policy row has a NULL slug and
context, and the app/tenant/user rows for one field differ only by the real
natural key. Confirmed in the generated manifest before the fix. The runtime
registry cannot surface it —
getAllSchemas()is keyed by TABLE name, so thetwo schemas collapse into one entry — so the new test asserts against the
generated manifest, and fails without the fix.
ApiCustomRouteConfig.scopeisOPTIONAL and documented to default to
collectionfor statics /itemforinstance methods, but the dispatcher required a literal
scope: 'collection'.A valid static action that omitted it was skipped, so
POST /<collection>/<action>fell through to CRUD and created an objectwherever
createis exposed. Scope now resolves through the sharedresolveCustomActionMetadata, with the receiver deciding.one options object;
buildCustomActionInvocationArgsnow projects them asthe SvelteKit/CLI/MCP transports do. Unchanged wherever parameter metadata is
absent, which is currently every collection-hosted action.
{ ok: false, code, message, status }convention had the raw object wrappedas a success;
normalizeCustomActionFailurenow applies first.Also folded in: an ABSENT request body is no longer conflated with a malformed
one, so a zero-parameter action can be called with no body while a present-but-
unparseable body stays a 400. The dispatcher had no core-side coverage at all;
rest-custom-actions.spec.tsadds it.Review threads
types.ts:52—defaultValuecarried two meanings. The optionwas documented and implemented as "JSON-encoded string, or a plain value to
serialize", so the natural call
{ defaultValue: 'Net 30' }storedunparseable text. The ambiguity is inherent —
'"TBD"'and'TBD'areindistinguishable — so the channel had to become explicit. Which channel
keeps which meaning was settled by the callers, not by taste: every existing
caller passes PRE-ENCODED values, decisively including smrt-fields: ObjectForm generator + form-settings gear (org/user policy editor) #2049/smrt-fields: defaults control panel — SettingsCatalog roll-up + AdminShell wiring #2050's gear,
whose
planFieldSettingsWritepostsJSON.stringify(draft.defaultValue)into a
FieldPolicyRowPatchtypeddefaultValue: string | nullstraightthrough the generated write routes. Auto-serializing
defaultValuewouldhave silently double-encoded every gear write across the rest of the epic.
So
defaultValuekeeps the encoded meaning (now typedstring | nullinstead of
unknown), and the plain value gets the new explicitdefaultValueRaw— the option twin of the existingsetDefaultValue().Supplying both throws; the parse failure now names the fix. Storage and every
read site are unchanged and stay symmetric. Fixed in
c1043544f.already fixed by
123e14398.Validation
@happyvertical/smrt-fields: 4 files, 55 tests passed (43 before thiscycle, 12 added);
tsc --noEmitgreen@happyvertical/smrt-coreFULL suite: 255 files passed / 5 skipped, 3,121tests passed (45 skipped);
tsc --noEmitgreentsc --noEmitalso green forsmrt-tenancy,smrt-users,smrt-cliturbo typecheck: 104/109 tasks successful. The 5 non-successes are all thedocumented nested-worktree
Tsconfig not found .../packages/accountsartifact(a vite-8 oxc-transform / pnpm-symlink issue that does not reproduce in CI's
isolated checkout — see the note in
packages/products/scripts/check-web-engine-code-split.mjs), hit bybuildtasks in packages this branch does not touch, plus the typechecks blocked
behind them.
biome checkclean on every touched filepnpm smrt dev:knowledge-check: fresh, 0 errors, 0 warningspnpm check:agents-chain: 65 chains under the 32,768-byte cap. This slice'schain (root +
packages/fields) is 19,616 bytes — 13,152 headroom. Thelargest chain repo-wide is
packages/usersat 29,997 (2,771 headroom),untouched here.
the userless-context write guard (P1-1), both scope-tier REST tests (P1-2,
which returned 500 with "Tenant-scope field policy rows must set tenantId"),
the resolver read-side mirror, and the hierarchy-loader cache key all fail
without their fix. The
updatedBytest cannot pass without the stamping byconstruction (the body supplies a different id than the assertion expects).
The natural-key authorization test is defence-in-depth and is noted as such
in the test itself: the new-scope check also refuses that write today,
because the scope key is derived from the caller's own owner id.
Rebased onto
origin/main, dropping the duplicate #2046 commit already mergedas PR #2153;
packages/core/agents/generators.mdandpackages/core/src/vite-plugin/web-collections.test.tsare byte-identical tomain.
Closes #2047
{"schema":"hv-agent-run:v1","runtime":"claude","session":"53e2c43c-41e2-4f9f-a63a-996baed10afd","issue":"2047","policy_revision":"1.0.0","status":"complete","validation":["smrt-fields full suite (55 tests) and smrt-core full suite (3121 tests)","tsc --noEmit for core, fields, tenancy, users, cli","biome check on all touched files","pnpm smrt dev:knowledge-check fresh (0 errors, 0 warnings)","pnpm check:agents-chain under cap (fields chain 19616 bytes, 13152 headroom)","codex full-diff review: 4 findings, all verified and fixed","all PR review threads addressed and resolved"]}