packages/mongo-schema/schema_context.js (1)

31-34: Consider returning a copy of the errors array to prevent external mutation.

validationErrors() returns the internal _errors array directly. If a caller mutates this array, it would corrupt the context's state without triggering reactivity updates. Returning a shallow copy ([...this._errors]) would be safer.

🛡️ Proposed defensive copy

   validationErrors() {
     if (this._dep) this._dep.depend();
-    return this._errors;
+    return [...this._errors];
   }

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@packages/mongo-schema/schema_context.js` around lines 31 - 34, The
validationErrors method currently returns the internal _errors array directly,
risking external mutation; change validationErrors (and keep the existing
_dep.depend() call) to return a shallow copy of the array (e.g., using spread or
slice) so callers get a defensive copy of this._errors and external mutations
won't corrupt the context or bypass reactivity.

packages/mongo-schema/tests/schema_errors_tests.js (1)

5-23: Consider adding coverage for DENY_INSERT and DENY_UPDATE error types.

The test verifies most ErrorTypes constants but omits DENY_INSERT and DENY_UPDATE which are defined in schema_errors.js. These may be covered by integration tests, but adding them here would ensure complete coverage of the constants.

💡 Add missing assertions

   test.equal(MongoSchema.ErrorTypes.FAILED_REGULAR_EXPRESSION, 'regEx');
   test.equal(MongoSchema.ErrorTypes.KEY_NOT_IN_SCHEMA, 'keyNotInSchema');
+  test.equal(MongoSchema.ErrorTypes.DENY_INSERT, 'denyInsert');
+  test.equal(MongoSchema.ErrorTypes.DENY_UPDATE, 'denyUpdate');
 });

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@packages/mongo-schema/tests/schema_errors_tests.js` around lines 5 - 23, The
test is missing assertions for the DENY_INSERT and DENY_UPDATE error type
constants; update the Tinytest 'errors - ErrorTypes constants exist' to include
test.equal(MongoSchema.ErrorTypes.DENY_INSERT, 'denyInsert') and
test.equal(MongoSchema.ErrorTypes.DENY_UPDATE, 'denyUpdate') so the constants
defined in schema_errors.js (DENY_INSERT and DENY_UPDATE) are asserted like the
other ErrorTypes.

packages/mongo-schema/tests/collection_integration_tests.js (1)

103-109: Add an explicit client/server split assertion for bypassSchema.

This test validates the server happy path, but it doesn’t lock in the “server-only” contract. Add a client-side assertion (or guard) so regressions don’t accidentally allow bypass behavior in client contexts.

As per coding guidelines, packages/**: “Focus on API backwards compatibility, DDP/reactivity correctness, and client/server split.”

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@packages/mongo-schema/tests/collection_integration_tests.js` around lines 103
- 109, This test currently exercises the server-only bypassSchema behavior; add
an explicit client/server split by asserting Meteor.isServer at the top when
running the happy-path (so the existing server behavior is validated only on
server), and add a client-side branch that verifies bypassSchema does not allow
schema bypass on the client (e.g. in the Meteor.isClient branch call
col.insertAsync({anything:'goes'},{bypassSchema:true}) and assert the operation
either throws or that the resulting document (via col.findOneAsync or the insert
result) does not contain the extra field), referencing Tinytest.addAsync,
bypassSchema, Mongo.Collection, attachSchema, MongoSchema, insertAsync and
findOneAsync to locate where to change the test.

packages/mongo-schema/schema_jsonschema.js (1)

26-32: oneOf produces empty schema {} for types without bsonType.

When compiling oneOf types, if a resolved type has no bsonType (like MongoSchema.Any or custom constructors), line 29 returns {}. While this is valid JSON Schema (allows any value), it effectively makes the entire oneOf constraint meaningless since {} matches anything.

Consider logging a warning in development, or explicitly handling the Any case:

♻️ Handle Any type explicitly

   if (typeName === 'oneOf') {
-    result.oneOf = desc.resolvedType.resolvedTypes.map(rt => {
-      if (rt.bsonType) return { bsonType: rt.bsonType };
-      return {};
-    });
+    const oneOfSchemas = desc.resolvedType.resolvedTypes
+      .filter(rt => rt.bsonType) // Skip types without bsonType
+      .map(rt => ({ bsonType: rt.bsonType }));
+    if (oneOfSchemas.length > 0) {
+      result.oneOf = oneOfSchemas;
+    }
+    // If all types lack bsonType, result has no type constraint
     return result;
   }

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@packages/mongo-schema/schema_jsonschema.js` around lines 26 - 32, The oneOf
branch currently maps resolved types to {} when rt.bsonType is missing, which
makes the oneOf meaningless; update the logic in the oneOf handling (look for
the typeName === 'oneOf' block and desc.resolvedType.resolvedTypes) to
explicitly detect and handle MongoSchema.Any or other non-bsonType constructors
instead of returning plain {} — for example produce a schema that signals "any"
via an explicit marker (or skip/include a more specific discriminator), and/or
emit a dev-only warning via your logging facility when a resolved type lacks
bsonType so reviewers know oneOf has a catch-all entry; ensure you reference rt
constructors (e.g., MongoSchema.Any) when deciding behavior so the oneOf
constraint remains meaningful.

packages/mongo-schema/collection_integration.js (1)

130-132: Consider removing async keyword from fire-and-forget function.

applyDatabaseEnforcement is declared async but returns immediately after scheduling a Meteor.startup callback. The actual async work happens inside the callback. This is harmless but slightly misleading since the returned Promise resolves before any database work occurs.

♻️ Optional refactor

-async function applyDatabaseEnforcement(collection, options) {
+function applyDatabaseEnforcement(collection, options) {
   // Server-only, deferred to after startup
   if (typeof Meteor === 'undefined' || !Meteor.isServer) return;

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@packages/mongo-schema/collection_integration.js` around lines 130 - 132, The
function applyDatabaseEnforcement is declared async but returns immediately
(work is done inside a Meteor.startup callback), so remove the misleading async
keyword and make it a regular function; update its declaration
(applyDatabaseEnforcement) and any internal use that assumes a returned Promise
(if any callers await it, stop awaiting or change them to rely on startup
behavior), and ensure the Meteor.startup callback still schedules the async work
(leave the inner async/await logic unchanged).

packages/mongo-schema/schema_validate.js (1)

243-249: Custom validators in modifier mode have limited field access.

When validating modifier fields, rootDoc is set to modifier[op] (e.g., the $set object). This means custom validators using this.field('otherField') can only see fields within the same operator, not fields being set via different operators like $inc or $setOnInsert.

This may be intentional for SimpleSchema compatibility, but consider documenting this limitation or enhancing the context to search across all operator payloads (similar to applyAutoValuesModifier's field() implementation in schema_clean.js).

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@packages/mongo-schema/schema_validate.js` around lines 243 - 249, Custom
validators running in modifier mode are given rootDoc = modifier[op], so when
validateField processes modifier operators (in schema_validate.js looping over
modifier[op] and calling validateField) a custom validator's
this.field('otherField') can only see fields inside the same operator; update
validateField's modifier-mode context so field resolution searches across all
operator payloads (not just modifier[op]) similar to applyAutoValuesModifier's
field() behavior in schema_clean.js, or at minimum document this limitation in
the API; specifically, adjust the logic that builds the rootDoc/context passed
into validateField (and any helper that implements this.field) to merge or
lookup values across all modifier operators (e.g., iterate modifier keys and
fallback lookups) so validations can access fields set by other operators like
$inc or $setOnInsert.

packages/mongo-schema/schema.js (1)

140-145: labels() mutates the IR which may affect extended schemas.

The labels() method directly mutates desc.label in the IR. If schemas share IR references (e.g., through extend() without deep cloning), label changes on one schema could affect another.

Looking at extend() (lines 36-39), it creates a new MongoSchema which calls parseDefinition and creates a fresh IR, so this appears safe. However, if future changes introduce IR sharing for performance, this mutation could become problematic.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@packages/mongo-schema/schema.js` around lines 140 - 145, labels() currently
mutates IR entries (desc.label) which can leak to other schemas if IRs are
shared; change it to avoid in-place mutation by creating a new descriptor with
the updated label and replacing the IR entry (e.g., read desc via
this._ir.get(key), create a shallow clone with the new label, then
this._ir.set(key, clonedDesc)) or maintain a separate label-overlay map; ensure
the change uses the existing parseDefinition/MongoSchema construction semantics
so extend() still produces independent schemas.

packages/mongo-schema/schema_clean.js (2)

23-25: Inconsistent mutation pattern with removeNullsFromArrays.

Unlike other cleaning steps that reassign result, removeNullsFromArrays only mutates in place and returns undefined. This works because result is already mutable at this point, but the inconsistency could cause confusion if the order of operations changes or if mutate: false semantics need adjustment.

♻️ Optional: Return result for consistency

-  if (options.removeNullsFromArrays) {
-    removeNullsFromArrays(ir, result);
-  }
+  if (options.removeNullsFromArrays) {
+    result = removeNullsFromArrays(ir, result);
+  }

And update the function:

 function removeNullsFromArrays(ir, doc) {
   for (const [key, value] of Object.entries(doc)) {
     if (Array.isArray(value)) {
       doc[key] = value.filter(item => item !== null);
     }
     if (typeof value === 'object' && value !== null && !Array.isArray(value) && !(value instanceof Date)) {
       removeNullsFromArrays(ir, value);
     }
   }
+  return doc;
 }

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@packages/mongo-schema/schema_clean.js` around lines 23 - 25, The call to
removeNullsFromArrays mutates result in place but doesn't return it, causing an
inconsistent mutation pattern; update the removeNullsFromArrays function to
return the cleaned object (return result) and change its invocation in the
options.removeNullsFromArrays branch to reassign result (e.g., result =
removeNullsFromArrays(ir, result)) so the flow matches other cleaning steps and
supports future non-mutating semantics.

---

266-274: autoConvertTypes only processes top-level keys.

Similar to removeEmptyStrings, this function doesn't recurse into nested objects. Nested values like { profile: { age: "25" } } won't be auto-converted to { profile: { age: 25 } }.

This appears consistent across the flat cleaning functions, but differs from the recursive approach used by trimStrings and applyDefaultValues.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@packages/mongo-schema/schema_clean.js` around lines 266 - 274,
autoConvertTypes currently only converts top-level fields; update it to recurse
into nested objects and arrays so nested values (e.g., profile.age) are
converted too: for each entry use ir.get(key) as now, but if value is a plain
object call autoConvertTypes recursively with that nested object and the
appropriate nested schema/IR (derived from desc.resolvedType or
desc.schema/inner IR on desc), and if value is an array map over elements
(recursing for object elements and using convertValue for primitives). Keep
using convertValue(desc.resolvedType.name) for primitive conversions, and
preserve the existing checks for null/undefined; update autoConvertTypes, its
recursive calls, and handling of arrays accordingly.

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

packages/mongo-schema/schema_definition.js (1)

100-109: Edge case: empty array [] throws an unclear error.

The array shorthand check at Line 101 requires exactly one element (def.length === 1). An empty array [] falls through to Line 108 and throws with JSON output "[]", which may be confusing. Consider adding a specific error message for empty arrays.

Proposed improvement for clearer error message

   // Shorthand: Array of type (e.g., { tags: [String] })
   if (Array.isArray(def) && def.length === 1) {
     return { type: Array, _arrayItemType: def[0] };
   }
+  // Invalid: empty array
+  if (Array.isArray(def) && def.length === 0) {
+    throw new Error('MongoSchema: Array shorthand requires exactly one element, e.g., [String]');
+  }
   // Full form object

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@packages/mongo-schema/schema_definition.js` around lines 100 - 109, The code
falls through and throws an unclear error when an empty array shorthand (def ===
[]) is provided; add an explicit branch before the existing Array.isArray(def)
&& def.length === 1 check to detect Array.isArray(def) && def.length === 0 and
throw a specific Error (e.g., "MongoSchema: Empty array shorthand [] is not
allowed; expected one type like [String]") so callers see a clear message; keep
the existing behavior for def.length === 1 (return { type: Array,
_arrayItemType: def[0] }) and for full-form objects (the typeof def === 'object'
&& def.type !== undefined branch).

packages/mongo-schema/schema_validate.js (1)

234-239: Minor inconsistency: denyUpdate triggers regardless of value presence.

At line 234, denyInsert checks value !== undefined, but at line 237, denyUpdate does not. In modifier validation, this is fine since only touched fields are validated. However, in document validation mode with isUpdate: true, this could trigger for fields that aren't actually present.

Consider adding the same guard for consistency:

Proposed fix

   if (desc.denyInsert && options.isInsert && value !== undefined) {
     errors.push({ name: key, type: ErrorTypes.DENY_INSERT, value, message: `${label} cannot be set during insert` });
   }
-  if (desc.denyUpdate && options.isUpdate) {
+  if (desc.denyUpdate && options.isUpdate && value !== undefined) {
     errors.push({ name: key, type: ErrorTypes.DENY_UPDATE, value, message: `${label} cannot be set during update` });
   }

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@packages/mongo-schema/schema_validate.js` around lines 234 - 239, The
denyUpdate branch is missing the same presence guard used by denyInsert, causing
DENY_UPDATE errors during document validation when the field isn't present;
update the condition around desc.denyUpdate in schema_validate.js (the block
that calls errors.push with type ErrorTypes.DENY_UPDATE) to only run when
options.isUpdate is true AND value !== undefined, mirroring the denyInsert check
so only present/touched fields trigger the error.

packages/mongo-schema/types.js (1)

164-172: Consider documenting that oneOf descriptors lack a top-level bsonType.

The oneOf type descriptor omits bsonType, which is intentional since JSON Schema compilation handles it via the resolvedTypes array. However, documenting this explicitly in the return object or JSDoc would help maintainability.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@packages/mongo-schema/types.js` around lines 164 - 172, The oneOf branch
(when typeDef._isOneOf) returns an object without a top-level bsonType; update
the code or JSDoc to explicitly document that behavior so future maintainers
know JSON Schema compilation uses resolvedTypes instead. Add a short inline
comment or JSDoc on the returned object from the oneOf block (or add an explicit
field like bsonType: undefined/null with a clarifying comment) and mention that
resolvedTypes is used for JSON Schema compilation; reference the oneOf branch,
the typeDef._isOneOf check, and the resolvedTypes variable when adding the
documentation.

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

packages/mongo-schema/schema_validate.js (1)

234-239: ⚠️ Potential issue | 🟠 Major

denyInsert/denyUpdate constraints require isInsert/isUpdate flags from callers.

The constraint checks here are correctly implemented, but as previously noted, collection_integration.js must pass the isInsert/isUpdate flags to schema.validate() for these constraints to be enforced. Without those flags, denyInsert and denyUpdate restrictions will silently pass.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@packages/mongo-schema/schema_validate.js` around lines 234 - 239, The
denyInsert/denyUpdate checks in schema_validate.js rely on callers to set
options.isInsert/options.isUpdate, so update collection_integration.js to pass
these flags into schema.validate during insert and update flows: when performing
an insert call schema.validate(document, { ..., isInsert: true, ... }) and for
updates call schema.validate(updateDoc, { ..., isUpdate: true, ... }); ensure
any existing options object passed into schema.validate is preserved/merged
rather than overwritten so other validation options remain intact.

packages/mongo-schema/schema_validate.js (1)

449-453: Minor: siblingField() in modifier validators has same issue.

The siblingField implementation here (line 452) delegates to this.field(name) similar to runCustomValidator. However, since modifier fields are typically flat dot-paths in mergedFields, the impact is less severe than in plain document validation. Consider applying the same fix for consistency.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@packages/mongo-schema/schema_validate.js` around lines 449 - 453,
siblingField currently delegates to this.field(name) which may not correctly
resolve sibling values for modifier validation; update siblingField in the
modifier validator to mirror the same resolution logic used for field (and as
fixed in runCustomValidator) so it looks up mergedFields using the flat dot-path
(mergedFields[name]) and returns { isSet: mergedFields[name] !== undefined,
value: mergedFields[name], operator: null } (keeping parentField and field
behavior unchanged) to ensure consistent sibling lookup for modifier
validations.

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes