UN-3815 [FIX] Apply organization scoping to prompt-studio child models - #2213
Open
athul-rs wants to merge 9 commits into
Open
UN-3815 [FIX] Apply organization scoping to prompt-studio child models#2213athul-rs wants to merge 9 commits into
athul-rs wants to merge 9 commits into
Conversation
Contributor
Summary by CodeRabbit
WalkthroughThe changes enforce organization-aware querying, constrain Prompt Studio lookups, remove file deletion routes, narrow row-locking behavior, and add organization-path and cross-organization isolation tests. ChangesOrganization isolation and access control
Endpoint and transaction changes
Estimated code review effort: 4 (Complex) | ~45 minutes Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Contributor
|
| Filename | Overview |
|---|---|
| backend/utils/models/org_path_discovery.py | Pins deterministic organization paths for Prompt Studio child models; the previously reported nullable-adapter scenario remains unconfirmed. |
| backend/utils/models/org_aware_manager.py | Applies organization predicates through the configured paths while retaining unscoped behavior when no organization context exists. |
| backend/prompt_studio/prompt_studio_core_v2/views.py | Adds tool-scoped request-ID resolution and transactional default-profile switching. |
| backend/prompt_studio/tests/test_cross_org_isolation.py | Covers cross-organization manager isolation, request-ID scoping, worker context, default switching, and removal of the obsolete route. |
Flowchart
%%{init: {'theme': 'neutral'}}%%
flowchart LR
R[Organization-scoped request] --> C[UserContext organization]
C --> M[OrgAwareManager]
M --> P[Configured organization path]
P --> T[CustomTool organization]
P --> A[Adapter organization]
M --> Q[Prompt Studio child queryset]
Q --> D[Documents]
Q --> I[Indexes]
Q --> O[Outputs]
Q --> PR[Prompts and profiles]
Reviews (9): Last reviewed commit: "Merge remote-tracking branch 'origin/mai..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
![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: 2
🧹 Nitpick comments (2)
backend/file_management/views.py (1)
28-33: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winStale docstring still advertises DELETE.
The class docstring still says the viewset "Handles GET,POST,PUT,PATCH and DELETE" but the delete action (and its URL route) is now gone.
✏️ Proposed docstring fix
"""FileManagement view. - Handles GET,POST,PUT,PATCH and DELETE + Handles GET, POST, PUT, and PATCH """🤖 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 `@backend/file_management/views.py` around lines 28 - 33, Update the FileManagementViewSet class docstring to remove DELETE from the listed supported operations, leaving only the methods and actions still exposed by the viewset.backend/prompt_studio/tests/test_cross_org_isolation.py (1)
100-104: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winNo
tearDownto resetUserContextafter mutating tests.
test_no_org_context_is_unfiltered(Line 143) sets the org identifier toNone, andtest_worker_context_sees_its_own_org(Line 151) sets it to org B; neither is restored. SinceUserContextlooks like process-level/thread-local state (not something Django's transactionalTestCaserolls back), whichever of these runs last leaves stale org context for the next test class in the same run.♻️ Proposed fix
def setUp(self) -> None: self.a = OrgFixture(f"org-a-{secrets.token_hex(3)}") self.b = OrgFixture(f"org-b-{secrets.token_hex(3)}") # End state: acting as org A, as a request would. UserContext.set_organization_identifier(self.a.org.organization_id) + + def tearDown(self) -> None: + UserContext.set_organization_identifier(None)🤖 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 `@backend/prompt_studio/tests/test_cross_org_isolation.py` around lines 100 - 104, Update the test fixture class containing setUp, OrgFixture, and the affected isolation tests with a tearDown method that clears or restores UserContext’s organization identifier after every test. Ensure tests that mutate the context, including test_no_org_context_is_unfiltered and test_worker_context_sees_its_own_org, cannot leak state into subsequent tests.
🤖 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 `@backend/prompt_studio/prompt_studio_core_v2/views.py`:
- Around line 446-453: The default-profile update flow should resolve the target
ProfileManager before clearing the current default, so invalid or cross-tool IDs
leave existing state unchanged. In the relevant view method, move the
get_object_or_404 lookup for prompt_tool and request.data["default_profile"]
ahead of the reset, then wrap target validation and both default updates in
transaction.atomic().
In `@backend/prompt_studio/prompt_studio_output_manager_v2/views.py`:
- Around line 127-132: Update fetch_default_output_response() after the
organization-scoped ToolStudioPrompt.objects.filter() lookup to explicitly
detect an empty queryset and raise the existing tool-not-found error. Preserve
the scoped tool_id and organization filters, and continue using the queryset for
valid tools.
---
Nitpick comments:
In `@backend/file_management/views.py`:
- Around line 28-33: Update the FileManagementViewSet class docstring to remove
DELETE from the listed supported operations, leaving only the methods and
actions still exposed by the viewset.
In `@backend/prompt_studio/tests/test_cross_org_isolation.py`:
- Around line 100-104: Update the test fixture class containing setUp,
OrgFixture, and the affected isolation tests with a tearDown method that clears
or restores UserContext’s organization identifier after every test. Ensure tests
that mutate the context, including test_no_org_context_is_unfiltered and
test_worker_context_sees_its_own_org, cannot leak state into subsequent tests.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 99c5a213-933f-481f-a966-d43ed583fd72
📒 Files selected for processing (15)
backend/file_management/urls.pybackend/file_management/views.pybackend/prompt_studio/prompt_profile_manager_v2/models.pybackend/prompt_studio/prompt_studio_core_v2/migration_utils.pybackend/prompt_studio/prompt_studio_core_v2/views.pybackend/prompt_studio/prompt_studio_document_manager_v2/models.pybackend/prompt_studio/prompt_studio_index_manager_v2/models.pybackend/prompt_studio/prompt_studio_index_manager_v2/prompt_studio_index_helper.pybackend/prompt_studio/prompt_studio_output_manager_v2/models.pybackend/prompt_studio/prompt_studio_output_manager_v2/views.pybackend/prompt_studio/prompt_studio_v2/models.pybackend/prompt_studio/tests/__init__.pybackend/prompt_studio/tests/test_cross_org_isolation.pybackend/utils/models/org_path_discovery.pybackend/utils/tests/test_org_path_discovery.py
💤 Files with no reviewable changes (1)
- backend/file_management/urls.py
athul-rs
changed the title
athul-rs
marked this pull request as draft
July 27, 2026 19:16
get_org_path resolves the shortest FK chain from a model to Organization and breaks ties by field declaration order. Reordering two fields can therefore swap in a different path of the same length, and if that path runs through a nullable FK the org filter becomes an INNER JOIN that silently drops every row with a NULL — which reads as missing records rather than as an error. Pin the five prompt-studio models to their currently resolved paths so both consumers (OrgAwareManager and OrganizationFilterBackend) are frozen on the same value, and add tests that fail if a pin drifts from discovery or starts traversing a nullable FK. ProfileManager resolves to vector_store__organization rather than prompt_studio_tool__organization: BFS reaches AdapterInstance (which carries the organization FK) before CustomTool, and prompt_studio_tool is nullable, so pinning there would drop tool-less profiles.
Custom DRF @action methods never call filter_queryset(), so OrganizationFilterBackend does not run on them and a raw .objects lookup inside one carries no organization predicate. Five prompt-studio models have no organization FK and used a plain manager, leaving roughly 44 such call sites relying on the caller to pass a correct id. - Scope at the model layer: OrgAwareManager on DocumentManager, IndexManager, PromptStudioOutputManager, ToolStudioPrompt and ProfileManager. No migration — no manager sets use_in_migrations, so swapping objects serializes nothing. - Scope the lookups that take an id straight from the request: delete_for_ide now requires the document to belong to the tool the caller already passed authz on, get_output_for_tool_default filters prompts by organization, and make_profile_default constrains its secondary lookup to the same tool. All three use get_object_or_404 so a non-matching id is a 404 rather than an unhandled DoesNotExist, which the DRF handler would turn into a 500. - Drop the file/delete route and action: it has no caller, and it deleted a document over GET. - select_for_update(of=("self",)) where the org filter now adds joins, so Postgres does not also lock rows in DocumentManager, CustomTool or AdapterInstance. Tests cover the org isolation matrix, same-org access, worker context (org is set there, so the manager filters) and the no-org fail-open path.
…aults make_profile_default cleared is_default across every profile on the tool and only then resolved the id from the request body. A non-matching id left the tool with no default at all, and the two writes were not in a transaction. Resolve first, then clear and set inside a single transaction, so a rejected id changes nothing. Adds a regression test for that, plus a tearDown resetting the thread-local UserContext (TestCase rollback does not clear it, so the org-switching tests leaked into later classes) and drops DELETE from the FileManagement docstring now the route is gone.
athul-rs
force-pushed
the
UN-3794-org-scoping
branch
from
July 29, 2026 09:42
65613a0 to
14f94cd
Compare
athul-rs
marked this pull request as ready for review
July 29, 2026 09:42
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
backend/prompt_studio/tests/test_cross_org_isolation.py (1)
163-207: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExercise the actions, not only their ORM predicates.
These tests recreate the intended lookups directly, so they cannot catch a regression in
delete_for_ideormake_profile_default’s HTTP 404 mapping or mutation order. Add authenticated action requests that assert 404 and preserve the original default profile.🤖 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 `@backend/prompt_studio/tests/test_cross_org_isolation.py` around lines 163 - 207, Add authenticated HTTP action tests covering delete_for_ide and make_profile_default, rather than only direct DocumentManager/ProfileManager lookups. Use cross-organization or cross-tool IDs, assert each endpoint returns 404, and verify the target tool’s existing default profile remains unchanged after each rejected request.
🤖 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.
Nitpick comments:
In `@backend/prompt_studio/tests/test_cross_org_isolation.py`:
- Around line 163-207: Add authenticated HTTP action tests covering
delete_for_ide and make_profile_default, rather than only direct
DocumentManager/ProfileManager lookups. Use cross-organization or cross-tool
IDs, assert each endpoint returns 404, and verify the target tool’s existing
default profile remains unchanged after each rejected request.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 7b3f6192-48dd-4517-b09f-875acf509d01
📒 Files selected for processing (15)
backend/file_management/urls.pybackend/file_management/views.pybackend/prompt_studio/prompt_profile_manager_v2/models.pybackend/prompt_studio/prompt_studio_core_v2/migration_utils.pybackend/prompt_studio/prompt_studio_core_v2/views.pybackend/prompt_studio/prompt_studio_document_manager_v2/models.pybackend/prompt_studio/prompt_studio_index_manager_v2/models.pybackend/prompt_studio/prompt_studio_index_manager_v2/prompt_studio_index_helper.pybackend/prompt_studio/prompt_studio_output_manager_v2/models.pybackend/prompt_studio/prompt_studio_output_manager_v2/views.pybackend/prompt_studio/prompt_studio_v2/models.pybackend/prompt_studio/tests/__init__.pybackend/prompt_studio/tests/test_cross_org_isolation.pybackend/utils/models/org_path_discovery.pybackend/utils/tests/test_org_path_discovery.py
💤 Files with no reviewable changes (1)
- backend/file_management/urls.py
🚧 Files skipped from review as they are similar to previous changes (9)
- backend/prompt_studio/prompt_studio_output_manager_v2/views.py
- backend/prompt_studio/prompt_studio_core_v2/migration_utils.py
- backend/prompt_studio/prompt_studio_document_manager_v2/models.py
- backend/prompt_studio/prompt_profile_manager_v2/models.py
- backend/prompt_studio/prompt_studio_index_manager_v2/prompt_studio_index_helper.py
- backend/prompt_studio/prompt_studio_index_manager_v2/models.py
- backend/utils/tests/test_org_path_discovery.py
- backend/file_management/views.py
- backend/prompt_studio/prompt_studio_v2/models.py
…ol_default filter() does not raise ObjectDoesNotExist, so the except branch could never fire and the tool-not-found message was dead. Empty is the right result here anyway: it covers a missing tool, an out-of-org tool, and a newly created project that has no prompts yet, which is a normal state that must not 400.
filter_queryset_by_organization returned the queryset unfiltered when the request carried no organization context, which is the opposite of what a scoping helper should do — and its own docstring already claimed it returned an empty queryset. Six internal viewsets set skip_org_filter = True, which disables OrganizationFilterBackend and leaves this helper as their only tenant boundary across roughly 39 call sites. The internal auth middleware logs a warning and continues when X-Organization-ID is missing, so any caller holding the internal service key reached those endpoints without context by omitting the header, reading across every organization — and through the file-execution viewset, writing and deleting too. Return none() instead, and log loudly, so a caller that legitimately has no context is visible rather than silently served everything. Deliberately not rejecting header-less /internal/ requests in the middleware: the leader-elected reaper calls without the header on purpose, to scan across organizations. It queries the model directly rather than through this helper, so failing closed leaves it working.
Contributor
|
@greptileai please review this |
Contributor
Author
|
@greptileai re-review this PR |
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)
backend/prompt_studio/prompt_studio_core_v2/views.py (1)
453-456: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winReturn 400 when
default_profileis missing.
request.data["default_profile"]raisesKeyErrorwhen the field is omitted. Django REST Framework then returns a 500 response. Validate the field with a serializer or use.get()and return a 400 response before the scoped lookup.Proposed fix
+ default_profile_id = request.data.get("default_profile") + if default_profile_id is None: + return Response( + {"detail": "default_profile is required."}, + status=status.HTTP_400_BAD_REQUEST, + ) + profile_manager = get_object_or_404( ProfileManager, - pk=request.data["default_profile"], + pk=default_profile_id, prompt_studio_tool=prompt_tool, )🤖 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 `@backend/prompt_studio/prompt_studio_core_v2/views.py` around lines 453 - 456, Update the view logic around the ProfileManager lookup to validate that default_profile is present before accessing request.data["default_profile"]. Return a 400 response when it is omitted, while preserving the existing scoped lookup through prompt_studio_tool for valid values; use the view’s established validation or error-response pattern.
🧹 Nitpick comments (1)
backend/prompt_studio/prompt_studio_core_v2/views.py (1)
137-139: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMark the class configuration as
ClassVar.Ruff reports RUF012 for both mutable class attributes. Add
ClassVarannotations to make the shared viewset configuration explicit without changing behavior.🤖 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 `@backend/prompt_studio/prompt_studio_core_v2/views.py` around lines 137 - 139, Annotate the mutable ordering and ordering_fields class attributes in the surrounding viewset with ClassVar, importing ClassVar from typing if needed. Preserve their existing list values and behavior while satisfying Ruff RUF012.Source: Linters/SAST tools
🤖 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 `@backend/prompt_studio/prompt_studio_core_v2/views.py`:
- Around line 453-456: Update the view logic around the ProfileManager lookup to
validate that default_profile is present before accessing
request.data["default_profile"]. Return a 400 response when it is omitted, while
preserving the existing scoped lookup through prompt_studio_tool for valid
values; use the view’s established validation or error-response pattern.
---
Nitpick comments:
In `@backend/prompt_studio/prompt_studio_core_v2/views.py`:
- Around line 137-139: Annotate the mutable ordering and ordering_fields class
attributes in the surrounding viewset with ClassVar, importing ClassVar from
typing if needed. Preserve their existing list values and behavior while
satisfying Ruff RUF012.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 5ae00466-ec2f-47b0-a872-58ea3d565c73
📒 Files selected for processing (1)
backend/prompt_studio/prompt_studio_core_v2/views.py
chandrasekharan-zipstack
left a comment
chandrasekharan-zipstack
left a comment
Contributor
There was a problem hiding this comment.
Standardized review — PR #2213
Verdict: REQUEST CHANGES
Summary — Critical: 0 · High: 4 · Medium: 9 · Low: 9 · Lenses run: 16/16
Reviewed under the unstract:standard-review 16-lens rubric. Findings below are deduplicated against the existing CodeRabbit and Greptile threads — anything already raised there is not repeated. Specifically not re-raised:
- CodeRabbit's "exercise the actions, not only their ORM predicates" on
test_cross_org_isolation.py— I agree with it and rate it higher than Trivial; only the part it did not cover (themake_profile_defaultordering fix having vacuous coverage) is filed below. - Greptile's "shared adapters hide tool profiles" on
org_path_discovery.py:47— the author's rebuttal is correct;AdapterInstanceModelManagerdoes scope every sharing path to the org. Closed on the merits. - CodeRabbit's
get_output_for_tool_defaultempty-200 thread, which the author answered and CodeRabbit accepted. Only the third cause of empty that the thread never discussed is filed below. - CodeRabbit's stale-DELETE docstring on
file_management/views.py. Residual nit: the replacement line now readsHandles GET, POST, PUT and PATCH, buturls.pyroutes only GET and POST — noupdate/partial_updateexists on the viewset.
Lens checklist
| # | Lens | Result |
|---|---|---|
| 1 | Spec & intent | Clean |
| 2 | Architectural fit | See H2, M7 |
| 3 | Correctness & edge cases | See H4, M1, M3, M5, M6, M9 |
| 4 | Security | See H2 |
| 5 | Data integrity & migrations | See M1, M2 |
| 6 | Concurrency | Clean — of=("self",) rationale verified correct at both sites; positive filters give INNER JOINs, so no nullable-outer-join hazard |
| 7 | API & contract compatibility | See H1 |
| 8 | Reliability & resilience | See H1 |
| 9 | Performance & cost | Clean |
| 10 | Observability | See H4 |
| 11 | Operational safety | See H1 — no flag, no deploy gate |
| 12 | LLM/agent | N/A — no model calls touched |
| 13 | Testing | See H3, M2 |
| 14 | Dependencies & build | N/A — none changed. Confirmed no migration needed: no manager sets use_in_migrations |
| 15 | Code quality | Low only |
| 16 | Doc & comment accuracy | See M8, M9, and Lows |
Unanchored findings (outside the diff hunks)
[High] [Lens 8, 11] — validate_tool_instances_internal returns success: true having validated nothing. backend/tool_instance_v2/internal_views.py:337-397. Function-based @api_view, no filter backend, so filter_queryset_by_organization is its only scoping. Header-less, tool_instances is now empty, the loop never runs, validation_errors stays empty, and it returns HTTP 200 {"success": true, "errors": []}. The adapter-ID migration inside that loop (:355-360) is skipped too. Worker side, workers/shared/workflow/execution/tool_validation.py:120-133 then logs ✅ Validated N tool instances successfully using the requested count, not len(validated_instances). This is the sharpest instance of H1 and the reason I would argue H1 up to Critical if any deployed worker can omit the header.
[Medium] [Lens 3] — import_prompts attaches profile_manager=None to every imported prompt. backend/prompt_studio/prompt_studio_core_v2/prompt_studio_helper.py:3014-3016 → :3062. ProfileManager.objects.filter(...).first() is now org-scoped and returns None rather than raising when the filter empties; there is no None check before :3062 passes it into every ToolStudioPrompt.objects.create(...). The sibling sync_prompts at :3146-3153 does raise on exactly this — the omission looks accidental rather than deliberate.
[Medium] [Lens 5] — Org-scoped .delete() in sync_prompts can leave survivors. prompt_studio_helper.py:3159-3161. ToolStudioPrompt.objects.filter(tool_id=tool).delete() is now filtered by tool_id__organization; prompts the scope misses survive alongside their recreated replacements inside the same transaction.atomic(). deleted_count is only used to decide whether to bump modified_at, never to verify the delete was complete.
[Medium] [Lens 3] — check_files_history reads org from the header but sets it from the body. backend/workflow_manager/internal_views.py:2584-2595 installs request.data["organization_id"] into StateStore, but filter_queryset_by_organization reads request.organization_id, populated only from the header. A body-only caller previously worked and now gets .none() → Workflow.DoesNotExist → 404 "not found or access denied", which blames authorization for a context-plumbing mismatch inside one function.
Low (9)
backend/prompt_studio/tests/test_cross_org_isolation.py:112, :156, :163, :208— review-artifact tags (A-1,A-3,A-4,A-5,B1, "the reported call sites") resolve to nothing in the repo. Same for "pinned as-is rather than changed under a security fix" atorg_path_discovery.py:41-42. RepoCLAUDE.mdasks that comments read correctly without the authoring session's context.backend/file_management/views.py:31— "Handles GET, POST, PUT and PATCH"; only GET and POST are routed.- Three symbols orphaned by the route removal, each had exactly one caller and this PR deleted it:
file_management/serializer.py:53(FileInfoIdeSerializer),file_management/file_management_helper.py:229(delete_file),prompt_studio_output_manager_v2/constants.py:12(TOOL_NOT_FOUND). backend/utils/tests/test_org_path_discovery.py:27-30—test_pin_is_returnedreduces tod.get(k) == d[k]; it can only fail if the short-circuit is deleted outright.backend/utils/tests/test_organization_scoping.py:23-26, :59-62—_Request.__init__guards onis not None, so theNoneleg offor falsy in ("", None)produces an object with no attribute at all — byte-identical totest_missing_org_context_returns_nothing.test_cross_org_isolation.py:96,test_organization_scoping.py:29—@pytest.mark.django_dbis a no-op onTestCasesubclasses and does not drive tier selection (backend/conftest.py:38-44marks on either signal).test_cross_org_isolation.py:100-110—OrgFixture.__init__sets thread-local org context as a construction side effect, andunittestskipstearDownwhensetUpraises.self.addCleanup(UserContext.set_organization_identifier, None)as the first statement ofsetUpruns even on failure.test_organization_scoping.py:47-53— depends on Django's private_base_managerMRO resolution; abase_manager_nameadded toBaseModellater would silently re-scope the queryset and point the failure at the helper.- Django admin changelists for all five models now use
OrgAwareManagervia_default_manager, and/admin/is not matched byOrganizationMiddleware, so the list depends on whateverStateStoreholds on that thread.
Verified clean, for the record
All five pins match what _discover_org_path returns today (BFS field ordering traced per model). _base_manager stays a plain unfiltered Manager (no base_manager_name on BaseModel), so cascade deletes, forward-FK descriptors and the pre_delete receiver at prompt_studio_index_manager_v2/models.py:122-141 are unaffected. Zero references to file/delete across unstract, unstract-cloud and unstract-docs — the UI deletes via DELETE /prompt-studio/file/<tool_id> (ManageDocsModal.jsx:674-687), so the route removal is correct, and the sibling-route guard in test_file_delete_route_removed is a nice touch. tests/groups.yaml collects both new test directories and CI runs them. The CONCURRENCY_MODE → RuntimeError → fail-open path in StateStore is real but latent — the env var is set in no compose, helm or env file in either repo.
Open questions
- Can any currently deployed worker call an internal endpoint without
X-Organization-ID? Three in-repo comments say yes during rolling deploys. That answer decides whether H1 is High or Critical. - Is
OrgAwareManager's fail-open deliberate policy, or an artifact of it predating the fail-closed backend? This PR pins it in a test and argues the opposite in a docstring, in the same diff. SELECT count(*) FROM custom_tool WHERE organization_id IS NULL(andadapter_instance) — several Mediums collapse to nothing if that is zero.
Reviewed with unstract:standard-review v0.18.1 (16-lens rubric, 4 specialist agents). Comments are advisory; event: COMMENT, no merge gate.
Fixes the failure modes the newly-scoped managers introduced, and corrects the
comments that described the scoping inaccurately.
- get_or_create now goes through _base_manager at both call sites. Django
applies a manager's filter to the get half but not the create half, so a row
the org scope hid made get miss and create collide with the unique
constraint. Both callers already hold org-verified parents.
- mark_extraction_status: the internal endpoint returns 500 instead of
200 {"success": false}. The worker never read the body, so a failed write
was silently dropped and every later Answer Prompt re-ran the full X2Text
extraction. The bare `except Exception` is narrowed, and the worker logs at
ERROR with the cost spelled out.
- make_profile_default validates default_profile up front: a missing key was a
KeyError and a non-UUID value a Django ValidationError, both 500s next to
the 404 this action already returned. The write is now
save(update_fields=["is_default"]) so it cannot clobber a concurrent edit
from its pre-transaction snapshot.
- get_output_for_tool_default and latest_outputs_by_keys validate tool_id as a
UUID (a non-UUID raised while the query was built, giving a 500) and refuse
to run with no organization in context, which compiled to
`organization_id IS NULL` and served a blank project that has real outputs.
- delete_for_ide warns when no index managers are visible: the delete
otherwise returned 200 while leaving Redis indexing flags behind. Its
handler keeps the broad catch — Redis, the object store and the database
are all in play and share no base class — but now logs type, document and
stack.
- The lazy summarize migration distinguishes "profile is filtered out" from
"profile does not exist"; the first never self-heals and no longer hides
behind the same INFO line.
- OrgAwareManager logs when it fails open on an exception. That arm catches
more than its stated cause: StateStore.get raises RuntimeError for any
unrecognised CONCURRENCY_MODE. The org-is-None arm stays silent — it is the
normal state for every Celery query.
- Comment corrections: "six internal viewsets" undercounted a ~35-call-site
surface; "custom @action methods never call filter_queryset()" is wrong,
since get_object() does filter and it is the raw .objects lookups beside it
that do not; the pin comment overstated what the test proves and omitted
that org_filter_paths outranks the pin at the view layer; and seven
backward-compat comments still described the header as optional after the
helper began failing closed.
Tests: the nullable-hop assertion now covers the terminal organization FK,
which is the nullable one on every pin, with the exemptions written down.
make_profile_default is exercised through the view — allow path and rejection
path. Mutation-tested: the rejection case fails only on clear-then-resolve
*without* the transaction, which is what the code did before; reverting the
ordering alone is safe because the 404 rolls the clear back, so the test
docstring says that rather than the reviewer's stronger claim.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Contributor
Unstract test resultsPer-group results
Critical paths
|
Contributor
Author
|
@greptileai re-review this PR |
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
3 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.
What
DocumentManager,IndexManager,PromptStudioOutputManager,ToolStudioPrompt,ProfileManager— via the existingOrgAwareManager.delete_for_ide,get_output_for_tool_default,make_profile_default.Organizationinstead of re-deriving it by BFS on every fresh process.file/deleteroute and action.select_for_update(of=("self",))where the new org filter introduces joins.Why
OrganizationFilterBackendruns infilter_queryset(), which custom DRF@actionmethods never call. These five models have noorganizationFK and used a plainBaseModelManager, so a raw.objectslookup inside a custom action carried no organization predicate at all — roughly 44 call sites relying on the caller to pass a correct id.OrgAwareManageralready existed for exactly this shape but was only wired to one model (ExecutionLog).get_org_pathreturns the shortest FK chain toOrganizationand breaks ties by field declaration order. Reordering two fields on a model can swap in a different path of the same length. If that path runs through a nullable FK, Django turns the filter into an INNER JOIN and silently drops every row with a NULL — data loss that presents as missing records, not as an error. This applies toOrganizationFilterBackendin production today, independent of anything else in this PR.file/deleteis dead and shaped wrong. No caller anywhere in the frontend or backend;prompt-studio/file/<tool_id>(DELETE →delete_for_ide) is the live path. It also performed a delete over GET, which makes it prefetchable.How
objects = OrgAwareManager()on the four models with no custom manager;ProfileManagerModelManagernow extendsOrgAwareManagerinstead ofBaseModelManager. No migration — no manager setsuse_in_migrations, so swappingobjectsserializes nothing (makemigrations --check --dry-runis clean).ORG_PATH_OVERRIDESinorg_path_discovery.py, keyed by model label and checked before BFS, so both consumers (OrgAwareManagerandOrganizationFilterBackend) are frozen on the same value. Each pin is set to the path BFS resolves today, so this changes no behaviour on its own.ProfileManagerpins tovector_store__organization, notprompt_studio_tool__organization: BFS reachesAdapterInstance(which carries the organization FK) beforeCustomTool, andprompt_studio_toolis nullable, so pinning there would drop tool-less profiles.AdapterInstanceis org-owned and the serializer's FK queryset uses the org-scoped default manager, so this scopes to the same organization.get_object_or_404, so a non-matching id is a 404 rather than an unhandledModel.DoesNotExist— whichmiddleware.exception.drf_logging_exc_handlerdoes not map, and would surface as a 500.select_for_update(of=("self",))inprompt_studio_index_helperandmigration_utils: the org filter adds INNER JOINs, and PostgresFOR UPDATEwithoutof=locks rows in every joined table (DocumentManager,CustomTool,AdapterInstance).Can this PR break any existing features. If yes, please list possible items. If no, please explain why. (PS: Admins do not merge the PR without this section filled)
Yes — three areas, each covered by a test:
internal_api_auth.py,scheduler/tasks.py,workflow_helper.py), soOrgAwareManagerfilters there too — it is not a no-op outside requests. Indexing and execution pass because the worker's org matches the data's org.test_worker_context_sees_its_own_orgcovers this. Any path that legitimately spans organizations would now return empty; none was found.get_or_createunder a filtering manager. If thegethalf is filtered out while the row exists, thecreatehalf hits the unique constraint. Only reachable across organizations, but the failure mode changes from silently-wrong toIntegrityError.select_for_updatelock scope. Addressed withof=("self",); without it the joins would widen the lock.ProfileManagerandIndexManagerare the two affected call sites.Management commands and shell keep full access:
UserContext.get_organization()returnsNoneoutside a request and the manager fails open, unchanged.test_no_org_context_is_unfilteredpins that.file/deleteremoval is the one behaviour change with no in-repo caller to break. Any external API consumer of that endpoint is unknowable from this repo — worth a release note.Database Migrations
None.
makemigrations --check --dry-runis clean; no manager setsuse_in_migrations, so replacingobjectsdoes not produce a migration.Env Config
None.
Relevant Docs
None.
Related Issues or PRs
UN-3815
Dependencies Versions
Unchanged.
Notes on Testing
backend/prompt_studio/tests/test_cross_org_isolation.py— two fully populated organizations, then per-model checks that org A cannot reach org B's rows, that org A's own rows stay visible, that worker context still sees its own org, and that the no-org path stays unfiltered. Every isolation assertion was confirmed to fail againstmainbefore the fix, so the tests actually bite.backend/utils/tests/test_org_path_discovery.py— asserts each pin still matches what BFS resolves, and that no pin traverses a nullable FK (with one documented exception,ToolStudioPrompt.tool_id, which is the path already in force).main: identical failure sets (36, all pre-existing inworkflow_manager/execution/tests/test_pg_finalization_fixes.py), zero new.Not covered by automation: a real two-org Prompt Studio cycle (upload → index → run → delete) in a compose stack. Worth doing manually before merge.
Screenshots
Checklist
I have read and understood the Contribution Guidelines.