fix(core): tenancy-scope datasets get and delete#18750
Merged
Merged
Conversation
DatasetsManager.get and DatasetsManager.delete used to look up a dataset by its primary key alone, so any caller who knew the id could read or delete it regardless of tenant. This pushes the tenancy predicate into the storage layer via an optional filter (option (a) — no fetch-then-assert). - Storage contract: getDatasetById / deleteDataset and item-mutation inputs accept optional filters. All adapters (inmemory, libsql, pg, mysql, mongodb, spanner) honor the predicate when set. - Core: DatasetsManager.get / .delete accept flat organizationId / projectId args, stash them on the returned Dataset, and forward on every downstream storage call (getDetails, update, item ops, startExperimentAsync). - Server: GET / PATCH / DELETE /datasets/:id accept organizationId and projectId query params. - Client: getDataset, updateDataset, deleteDataset accept optional tenancy. - Tests: shared _test-utils tenancy suite (auto-runs against every adapter), DatasetsManager unit tests, and server handler tests. On tenancy mismatch, get throws NOT_FOUND (returns null at the storage layer) and delete is a silent no-op — matching how a missing id already behaves, so existence does not leak through error timing or messages. Legacy calls that omit tenancy are unchanged, so this is fully backwards-compatible. MASTRA-4438 Co-Authored-By: Mastra Code (anthropic/claude-opus-4-7) <noreply@mastra.ai>
🦋 Changeset detectedLatest commit: bc92a74 The changes in this PR will be included in the next version bump. This PR includes changesets to release 27 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
WalkthroughThis PR adds optional tenancy scoping ( ChangesTenancy scoping for dataset operations
Estimated code review effort: 4 (Complex) | ~60 minutes Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
|
The latest updates on your projects. Learn more about Vercel for GitHub.
1 Skipped Deployment
|
NikAiyer
requested review from
DanielSLew,
TheIsrael1,
YujohnNattrass,
abhiaiyer91,
mfrachet,
rase- and
wardpeet
as code owners
Contributor
PR triageLinked issue check skipped for core contributor @NikAiyer. PR complexity score
Applied label: Changed test gateChanged tests failed against the base branch as expected. Label: |
Co-Authored-By: Mastra Code (anthropic/claude-opus-4-7) <noreply@mastra.ai>
dane-ai-mastra
Bot
added
tests: green ✅
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (1)
stores/_test-utils/src/domains/datasets/index.ts (1)
84-237: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a scoped
updateDatasetcontract test.This suite covers scoped
getDatasetByIdanddeleteDataset, but notupdateDataseteven though scoped PATCH/update is part of this change. Adding a shared adapter test here would catch stores that still update by bareid.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@stores/_test-utils/src/domains/datasets/index.ts` around lines 84 - 237, Add a tenancy-scoping contract test for updateDataset in the datasets test suite so scoped PATCH/update behavior is covered alongside getDatasetById and deleteDataset. Reuse the existing tenancy patterns in datasetsStorage and verify that updateDataset with mismatched organizationId/projectId does not modify the record, while the matching tenancy does; this will catch adapters that still update by bare id.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.changeset/chatty-areas-grab.md:
- Around line 12-14: The changelog entry for the optional tenancy arguments
needs a wording fix because it incorrectly says `deleteDataset` returns a 404 on
tenant mismatch. Update the text in the changeset to keep the 404 behavior only
for `getDataset` and `updateDataset`, and rephrase the `deleteDataset` part so
it clearly states a silent no-op on mismatch without promising an error
response.
In `@packages/core/src/datasets/dataset.ts`:
- Around line 31-43: The Dataset tenancy scope is only applied to some reads and
mutations, but several handle methods still bypass it and can access
cross-tenant data. Update the remaining Dataset surface methods such as getItem,
listItems, deleteItem, listVersions, getItemHistory, and the experiment/listing
helpers to consistently forward `#scope` to storage calls. Use the existing scope
plumbing in Dataset.getDetails and Dataset.startExperimentAsync as the pattern
so all handle operations are tenancy-safe.
In `@packages/core/src/storage/types.ts`:
- Around line 2587-2607: Single-item dataset deletes are missing tenancy scoping
in the storage contract, so add an optional filters field to the delete-item
input alongside AddDatasetItemInput and UpdateDatasetItemInput. Update
DatasetsStorage.deleteItem(...) to first call getDatasetById({ id:
args.datasetId, filters: args.filters }) before proceeding to
_doDeleteItem(...), and make Dataset.deleteItem(...) pass this.#scope through so
scoped handles enforce the tenant.
- Around line 2562-2563: The update filter documentation in
DatasetTenancyFilters is incorrect because update operations do not no-op when
the scoped row is missing; the storage base and adapters throw NOT_FOUND
instead. Update the doc comment on the filters field in DatasetTenancyFilters to
describe the actual update behavior, and keep the wording aligned with the
storage base and adapter implementations that enforce the not-found error.
In `@packages/server/src/server/handlers/datasets.ts`:
- Around line 302-307: The dataset delete handler is doing a preflight
`mastra.datasets.get()` before `mastra.datasets.delete()`, which turns
tenant-scoped idempotent deletes back into a 404 on tenancy mismatch. Remove the
existence check from the delete path in `handler` and let
`mastra.datasets.delete` handle scoped deletes directly using `datasetId`,
`organizationId`, and `projectId` so the silent no-op behavior is preserved.
In `@stores/spanner/src/storage/domains/datasets/index.ts`:
- Around line 315-317: Scope the dataset update in the `update` flow by tenancy
instead of only on the readback. In `datasets/index.ts`, adjust the
`this.db.update` call inside the dataset update method so the write itself
includes the tenant-scoping criteria from `args.filters` (not just
`getDatasetById`), preventing cross-tenant mutation before the `NOT_FOUND`
check.
---
Nitpick comments:
In `@stores/_test-utils/src/domains/datasets/index.ts`:
- Around line 84-237: Add a tenancy-scoping contract test for updateDataset in
the datasets test suite so scoped PATCH/update behavior is covered alongside
getDatasetById and deleteDataset. Reuse the existing tenancy patterns in
datasetsStorage and verify that updateDataset with mismatched
organizationId/projectId does not modify the record, while the matching tenancy
does; this will catch adapters that still update by bare id.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 35037395-3d53-4d7f-970d-31feccf98a4b
📒 Files selected for processing (26)
.changeset/chatty-areas-grab.md.changeset/pink-mails-attend.md.changeset/swift-things-push.md.changeset/tired-donuts-fly.mdclient-sdks/client-js/src/client.tsclient-sdks/client-js/src/route-types.generated.tsclient-sdks/client-js/src/types.tsclient-sdks/client-js/src/utils/index.tspackages/cli/src/commands/api/route-metadata.generated.tspackages/core/src/datasets/__tests__/manager.test.tspackages/core/src/datasets/dataset.tspackages/core/src/datasets/experiment/index.tspackages/core/src/datasets/experiment/types.tspackages/core/src/datasets/manager.tspackages/core/src/storage/domains/datasets/base.tspackages/core/src/storage/domains/datasets/inmemory.tspackages/core/src/storage/types.tspackages/server/src/server/handlers/datasets.test.tspackages/server/src/server/handlers/datasets.tspackages/server/src/server/schemas/datasets.tsstores/_test-utils/src/domains/datasets/index.tsstores/libsql/src/storage/domains/datasets/index.tsstores/mongodb/src/storage/domains/datasets/index.tsstores/mysql/src/storage/domains/datasets/index.tsstores/pg/src/storage/domains/datasets/index.tsstores/spanner/src/storage/domains/datasets/index.ts
… updateDataset test, single-item deleteItem plumbing Follow-up on the initial MASTRA-4438 patch, addressing CodeRabbit + audit findings: - Dataset handle now gates unscoped read paths (getItem, listItems, listVersions, getItemHistory, listExperiments, getExperiment, listExperimentResults, updateExperimentResult, deleteExperiment) with a private #assertScope() that runs a scoped getDatasetById preflight and throws DATASET_NOT_FOUND on miss. Closes the last known handle-level cross-tenant read gap. - DeleteDatasetItemInput now carries optional filters; base deleteItem enforces a parent-existence gate identical to batchDeleteItems, so a leaked itemId cannot delete an item from a dataset in another tenant. Updated inmemory + libsql/pg/mysql/mongodb/spanner adapters to the new signature (parent gate in base means adapter bodies unchanged). - Shared _test-utils/datasets adds three scoped-updateDataset contract tests (cross-tenant NOT_FOUND, matching-tenant apply, mixed-tenancy sanity). Runs against every adapter via createTestSuite. - Sharpen changeset wording: reads/updates throw NOT_FOUND (404 over HTTP), deletes silently no-op on tenancy mismatch. - Server handler test for cross-tenant delete rewritten to reflect the silent-no-op contract (returns success, row untouched). - Verified: @mastra/core 266 tests + tsc --noEmit; @mastra/libsql 817 tests (includes shared tenancy + new update-scope tests); @mastra/server 11 handler tests + check:permissions + check:core-imports. Route metadata + route types regenerated (no diff — additive optional query params). Related: MASTRA-4438 Co-Authored-By: Mastra Code (anthropic/claude-opus-4-7) <noreply@mastra.ai>
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/core/src/datasets/dataset.ts (1)
227-236: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winForward
#scopeintolistItems()instead of relying on preflight.
ListDatasetItemsInputalready acceptsfilters, but this path drops them after#assertScope(). Keep the tenancy predicate in the storage query to avoid fetch-then-use gaps.Proposed fix
return store.listItems({ datasetId: this.id, search: args?.search, pagination: { page: args?.page ?? 0, perPage: args?.perPage ?? 20 }, + filters: this.#scope, });🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/core/src/datasets/dataset.ts` around lines 227 - 236, The dataset item listing path in Dataset.listItems() currently relies on `#assertScope`() and then drops the tenancy predicate before calling store.listItems(), which can create a fetch-then-use gap. Update the listItems() flow to pass this.#scope through ListDatasetItemsInput.filters (alongside datasetId, search, and pagination) so the store query enforces tenancy directly, and keep the version branch unchanged unless it also needs the same scope propagation.
♻️ Duplicate comments (1)
packages/core/src/datasets/dataset.ts (1)
209-212: 🔒 Security & Privacy | 🟠 Major | 🏗️ Heavy liftBind child lookups and mutations to this dataset, not just the parent preflight.
#assertScope()only provesthis.idis in scope. These methods then use caller-supplied item/experiment IDs without proving those child records belong tothis.id, so a known cross-dataset child ID can still be read or mutated through a valid scoped handle. Add dataset/scoped storage predicates or verify ownership before returning/updating/deleting. This is the remaining unresolved part of the earlier handle-surface tenancy concern.Also applies to: 287-290, 397-430
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/core/src/datasets/dataset.ts` around lines 209 - 212, The child lookup/mutation paths are only doing the parent scope check via `#assertScope`(), but still trust caller-provided item/experiment IDs that may belong to a different dataset. Update the affected Dataset methods such as getItem and the other child accessors/mutators in this class to verify ownership against this.id before returning, updating, or deleting records, either by adding dataset-scoped store predicates or by checking the fetched child record’s dataset association. Keep the fix local to the Dataset API surface so cross-dataset child IDs cannot be accessed through a valid scoped handle.
🧹 Nitpick comments (2)
packages/server/src/server/handlers/datasets.test.ts (2)
161-175: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winAdd projectId mismatch coverage for update and delete.
GET covers
projectIdmismatch, but UPDATE and DELETE only exerciseorganizationId. Add parallelprojectId: 'proj_2'cases so query-param forwarding for both tenancy dimensions is protected.Also applies to: 205-227
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/server/src/server/handlers/datasets.test.ts` around lines 161 - 175, Add missing projectId mismatch coverage for the update and delete dataset handler tests. In the UPDATE_DATASET_ROUTE.handler and DELETE_DATASET_ROUTE.handler cases, mirror the existing organizationId mismatch assertions by also passing a different projectId (for example, proj_2) so both tenancy dimensions are verified. Keep the new checks alongside the existing dataset creation and rejection expectations in datasets.test.ts.
108-121: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winAssert the 404 status, not just the exception type.
These tests would still pass if the handler threw an
HTTPExceptionwith the wrong status. Since the requirement is “not found/no info leak,” assert the exception status/message shape as well.Also applies to: 124-138, 161-175
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/server/src/server/handlers/datasets.test.ts` around lines 108 - 121, The GET_DATASET_ROUTE.handler tests only assert that an HTTPException is thrown, so they can pass even if the status is wrong. Update the affected cases in datasets.test.ts to verify the exception’s 404/not found shape, not just the type, by asserting the status/message on the thrown error for GET_DATASET_ROUTE.handler and the related organization mismatch scenarios.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@packages/core/src/datasets/dataset.ts`:
- Around line 227-236: The dataset item listing path in Dataset.listItems()
currently relies on `#assertScope`() and then drops the tenancy predicate before
calling store.listItems(), which can create a fetch-then-use gap. Update the
listItems() flow to pass this.#scope through ListDatasetItemsInput.filters
(alongside datasetId, search, and pagination) so the store query enforces
tenancy directly, and keep the version branch unchanged unless it also needs the
same scope propagation.
---
Duplicate comments:
In `@packages/core/src/datasets/dataset.ts`:
- Around line 209-212: The child lookup/mutation paths are only doing the parent
scope check via `#assertScope`(), but still trust caller-provided item/experiment
IDs that may belong to a different dataset. Update the affected Dataset methods
such as getItem and the other child accessors/mutators in this class to verify
ownership against this.id before returning, updating, or deleting records,
either by adding dataset-scoped store predicates or by checking the fetched
child record’s dataset association. Keep the fix local to the Dataset API
surface so cross-dataset child IDs cannot be accessed through a valid scoped
handle.
---
Nitpick comments:
In `@packages/server/src/server/handlers/datasets.test.ts`:
- Around line 161-175: Add missing projectId mismatch coverage for the update
and delete dataset handler tests. In the UPDATE_DATASET_ROUTE.handler and
DELETE_DATASET_ROUTE.handler cases, mirror the existing organizationId mismatch
assertions by also passing a different projectId (for example, proj_2) so both
tenancy dimensions are verified. Keep the new checks alongside the existing
dataset creation and rejection expectations in datasets.test.ts.
- Around line 108-121: The GET_DATASET_ROUTE.handler tests only assert that an
HTTPException is thrown, so they can pass even if the status is wrong. Update
the affected cases in datasets.test.ts to verify the exception’s 404/not found
shape, not just the type, by asserting the status/message on the thrown error
for GET_DATASET_ROUTE.handler and the related organization mismatch scenarios.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 975195b5-6516-45fd-8c44-6f9e89b846da
📒 Files selected for processing (13)
.changeset/chatty-areas-grab.mdpackages/core/src/datasets/dataset.tspackages/core/src/storage/domains/datasets/base.tspackages/core/src/storage/domains/datasets/inmemory.tspackages/core/src/storage/types.tspackages/server/src/server/handlers/datasets.test.tspackages/server/src/server/handlers/datasets.tsstores/_test-utils/src/domains/datasets/index.tsstores/libsql/src/storage/domains/datasets/index.tsstores/mongodb/src/storage/domains/datasets/index.tsstores/mysql/src/storage/domains/datasets/index.tsstores/pg/src/storage/domains/datasets/index.tsstores/spanner/src/storage/domains/datasets/index.ts
🚧 Files skipped from review as they are similar to previous changes (10)
- .changeset/chatty-areas-grab.md
- packages/core/src/storage/types.ts
- stores/spanner/src/storage/domains/datasets/index.ts
- stores/mysql/src/storage/domains/datasets/index.ts
- packages/core/src/storage/domains/datasets/inmemory.ts
- stores/libsql/src/storage/domains/datasets/index.ts
- packages/core/src/storage/domains/datasets/base.ts
- stores/pg/src/storage/domains/datasets/index.ts
- packages/server/src/server/handlers/datasets.ts
- stores/mongodb/src/storage/domains/datasets/index.ts
Address CodeRabbit round-2 review on MASTRA-4438: - Dataset.getItem / getItemHistory / getExperiment / listExperimentResults now verify the returned child row's parent datasetId matches this.id before returning. Prevents a caller with a valid handle to dataset A from reading item/experiment rows belonging to dataset B via a caller-supplied child id. - Dataset.updateExperimentResult / deleteExperiment throw NOT_FOUND on ownership mismatch, matching the existing deleteItem contract. - Dataset.listItems now forwards this.#scope via ListDatasetItemsInput.filters instead of an extra preflight roundtrip. - Server handler tests: assert HTTP 404 status on scope-mismatch (not just HTTPException) and add projectId-mismatch cases for update + delete routes. Co-Authored-By: Mastra Code (anthropic/claude-opus-4-7) <noreply@mastra.ai>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
packages/core/src/datasets/dataset.ts (1)
422-425: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winRe-run the tenant scope check before experiment mutations/results access.
#assertExperimentOwnership()validates onlyexperiment.datasetId === this.id; it does not prove the parent dataset still matches#scope. SincelistExperimentResults(),updateExperimentResult(), anddeleteExperiment()rely on this helper, a scoped/stale handle can bypass the same per-operation tenancy check used by the other handle methods.Suggested fix
async `#assertExperimentOwnership`(experimentId: string): Promise<void> { + await this.#assertScope(); const experimentsStore = await this.#getExperimentsStore(); const experiment = await experimentsStore.getExperimentById({ id: experimentId });🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/core/src/datasets/dataset.ts` around lines 422 - 425, Re-run the tenant scope check inside `#assertExperimentOwnership` before allowing experiment access or mutation. The helper currently only verifies experiment.datasetId against this.id, so update it to also validate the dataset’s current scope via `#checkScope` (or the equivalent dataset fetch/validation used by other handle methods) before returning. Keep the fix localized in `#assertExperimentOwnership` so listExperimentResults(), updateExperimentResult(), and deleteExperiment() inherit the same per-operation tenancy guard.
🧹 Nitpick comments (1)
packages/core/src/datasets/__tests__/dataset.test.ts (1)
612-612: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAssert the exact
MastraError.idfor these ownership failures.These tests name specific failure modes, but
.toThrow(MastraError)would also pass for the wrong storage/user error. MatchEXPERIMENT_NOT_FOUNDandEXPERIMENT_RESULT_MISSING_EXPERIMENT_IDso regressions in the ownership gate order are caught.Suggested test tightening
- await expect(ds.deleteExperiment({ experimentId: otherExp.id })).rejects.toThrow(MastraError); + await expect(ds.deleteExperiment({ experimentId: otherExp.id })).rejects.toMatchObject({ + id: 'EXPERIMENT_NOT_FOUND', + });Apply the same pattern to
listExperimentResults, cross-datasetupdateExperimentResult, and the missing-experimentIdcase.Also applies to: 627-627, 639-645
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/core/src/datasets/__tests__/dataset.test.ts` at line 612, The ownership-failure tests are too broad because they only assert a MastraError was thrown, so they can miss the wrong error path. Tighten the assertions in dataset.test.ts for deleteExperiment, listExperimentResults, cross-dataset updateExperimentResult, and the missing experimentId case to check the exact MastraError.id values, specifically EXPERIMENT_NOT_FOUND and EXPERIMENT_RESULT_MISSING_EXPERIMENT_ID, using the relevant dataset methods to verify the correct gate order.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/core/src/datasets/dataset.ts`:
- Around line 461-475: The Dataset handle already enforces `experimentId` at
runtime in `Dataset.updateExperimentResult`, so tighten the method signature to
require `experimentId` by using `UpdateExperimentResultInput & { experimentId:
string }` while keeping the existing missing-ID guard for JavaScript callers.
Update the `updateExperimentResult` method in `Dataset` so TypeScript callers
cannot pass an input without `experimentId`, and preserve the current ownership
check via `#assertExperimentOwnership` before delegating to
`#getExperimentsStore().updateExperimentResult`.
---
Duplicate comments:
In `@packages/core/src/datasets/dataset.ts`:
- Around line 422-425: Re-run the tenant scope check inside
`#assertExperimentOwnership` before allowing experiment access or mutation. The
helper currently only verifies experiment.datasetId against this.id, so update
it to also validate the dataset’s current scope via `#checkScope` (or the
equivalent dataset fetch/validation used by other handle methods) before
returning. Keep the fix localized in `#assertExperimentOwnership` so
listExperimentResults(), updateExperimentResult(), and deleteExperiment()
inherit the same per-operation tenancy guard.
---
Nitpick comments:
In `@packages/core/src/datasets/__tests__/dataset.test.ts`:
- Line 612: The ownership-failure tests are too broad because they only assert a
MastraError was thrown, so they can miss the wrong error path. Tighten the
assertions in dataset.test.ts for deleteExperiment, listExperimentResults,
cross-dataset updateExperimentResult, and the missing experimentId case to check
the exact MastraError.id values, specifically EXPERIMENT_NOT_FOUND and
EXPERIMENT_RESULT_MISSING_EXPERIMENT_ID, using the relevant dataset methods to
verify the correct gate order.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: b5fcc0f5-b7c0-4fdb-8339-6da52716e3d5
📒 Files selected for processing (3)
packages/core/src/datasets/__tests__/dataset.test.tspackages/core/src/datasets/dataset.tspackages/server/src/server/handlers/datasets.test.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/server/src/server/handlers/datasets.test.ts
…p tests Address CodeRabbit round-3 review on MASTRA-4438: - #assertExperimentOwnership now calls #assertScope() first, so stale scoped handles cannot bypass the per-operation tenancy check on listExperimentResults / updateExperimentResult / deleteExperiment. - Narrow Dataset.updateExperimentResult's input type to require experimentId (kept the runtime guard for JS callers). - Tighten ownership-failure tests to assert MastraError.id (EXPERIMENT_NOT_FOUND / EXPERIMENT_RESULT_MISSING_EXPERIMENT_ID) instead of only the error class. Co-Authored-By: Mastra Code (anthropic/claude-opus-4-7) <noreply@mastra.ai>
YujohnNattrass
approved these changes
Jul 2, 2026
# Conflicts: # packages/core/src/datasets/dataset.ts # stores/mysql/src/storage/domains/datasets/index.ts
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.
Summary
DatasetsManager.get({ id })andDatasetsManager.delete({ id })looked up a dataset by its primary key alone — noorganizationId/projectIdpredicate. Any caller who knew a dataset id could read or delete it regardless of which tenant it belonged to.listDatasetsalready scopes correctly viafilters, so this PR closes the asymmetry.Fix pushes tenancy into the SQL WHERE clause at the storage layer (option (a) in the ticket — no fetch-then-assert, which would leak existence via error timing).
Linear: MASTRA-4438
What changed
Storage contract (
packages/core/src/storage)getDatasetByIdanddeleteDatasetaccept optionalfilters?: DatasetTenancyFilters.AddDatasetItemInput,UpdateDatasetItemInput,BatchInsertItemsInput,BatchDeleteItemsInput) andUpdateDatasetInputaccept optionalfiltersfor the internal existence check.Adapters — inmemory, libsql, pg, mysql, mongodb, spanner
Core (
DatasetsManager,Dataset)DatasetsManager.get/.deleteaccept flat{ id, organizationId?, projectId? }.Datasethandle and forwarded togetDetails,update,addItem, item batch ops, andstartExperimentAsync.Server (
packages/server)GET /datasets/:id,PATCH /datasets/:id, andDELETE /datasets/:idaccept optionalorganizationIdandprojectIdquery params.Client (
client-sdks/client-js)getDataset,updateDataset,deleteDatasetaccept optional tenancy args.Behavior
get→ storage returnsnull; manager throwsNOT_FOUND; HTTP returns 404 — same as a truly missing id, so existence does not leak.delete→ silent no-op, matching the existing "delete non-existent is no-op" semantic.Example
Test plan
Automated
@mastra/core— 266 dataset tests pass, no type errors.@mastra/server— 11 handler tests (4 preexisting + 7 new tenancy tests) pass.check:permissions+check:core-importsclean. Build clean.@mastra/libsql— 46 dataset tests pass (includes new shared tenancy suite).@mastra/pg— 91 dataset tests pass (includes new shared tenancy suite).@mastra/mongodb,@mastra/mysql,@mastra/spanner— build clean; mysql unit tests pass. Their docker-backed integration tests should run in CI (containers were not up locally).@mastra/client-js— typecheck clean.New tests
_test-utilsdatasets suite — auto-runs against every store adapter.DatasetsManagerunit tests verifying tenancy is forwarded throughget,delete, and back onto the returnedDatasethandle.Follow-ups (out of scope)
listDatasetsat the HTTP + client-js layer.experimentsandscorer-definitionsget/delete.services/agent-learning/exploratory/experiments/src/runExperiment.tsnow that the manager enforces tenancy.ELI5
This PR makes sure you can’t use a dataset ID to read or delete a dataset that belongs to a different tenant. If the dataset isn’t in the tenant you asked for,
getbehaves like it doesn’t exist (404/NOT_FOUND) anddeletesilently does nothing.Summary
organizationIdand/orprojectId) forDatasetsManager.getandDatasetsManager.delete, including carrying that scope on the returnedDatasethandle.getDatasetByIdreturnsnullon tenancy mismatch.deleteDatasetbecomes a silent no-op on tenancy mismatch to avoid existence leakage.GET,PATCH, andDELETE /datasets/:idto accept optionalorganizationId/projectIdquery params and forward them into Mastra dataset operations (withDELETEavoiding cross-tenant existence leakage via conditional preflight).getDataset,updateDataset, anddeleteDatasetaccept optional tenancy arguments and send them as query params.Datasetnow reasserts scope/ownership before experiment mutations and result operations (not just dataset reads).updateExperimentResultinput now requiresexperimentId, with improved ownership/missing-id error behavior.