UN-3815 [FIX] Apply organization scoping to prompt-studio child models - #2213
UN-3815 [FIX] Apply organization scoping to prompt-studio child models#2213athul-rs wants to merge 10 commits into
Conversation
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 |
|
| Filename | Overview |
|---|---|
| backend/utils/models/org_path_discovery.py | Pins deterministic organization paths for five Prompt Studio child models; the previously discussed nullable-adapter case remains unverified. |
| backend/utils/models/org_aware_manager.py | Applies discovered organization paths to request-context querysets while retaining unfiltered behavior outside organization context. |
| backend/prompt_studio/prompt_studio_core_v2/views.py | Adds validated, tool-scoped request-ID lookups and transactional profile-default updates. |
| backend/prompt_studio/tests/test_cross_org_isolation.py | Exercises cross-organization manager isolation, same-organization access, worker context, profile-default switching, and route removal. |
Flowchart
%%{init: {'theme': 'neutral'}}%%
flowchart LR
A[Request or worker organization context] --> B[OrgAwareManager]
B --> C[Deterministic pinned FK path]
C --> D[Prompt Studio child queryset]
E[Custom action request ID] --> F[Explicit tool and organization predicates]
F --> D
D --> G[Organization-scoped document, index, output, prompt, and profile records]
Reviews (10): Last reviewed commit: "Merge branch 'main' into UN-3794-org-sco..." | 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}
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
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.
65613a0 to
14f94cd
Compare
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.
|
@greptileai please review this |
|
@greptileai re-review this PR |
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
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>
|
@greptileai re-review this PR |
|
Unstract test resultsPer-group results
Critical paths
|
| document_id, | ||
| profile_manager_id, | ||
| ) | ||
| return JsonResponse( |
There was a problem hiding this comment.
Instead of returning a JsonResponse, can you raise it as an error to keep it consistent with the rest of the codebase and let DRF validation handle it?
There was a problem hiding this comment.
NOT RESOLVED — no push since this was raised, so just re-flagging it rather than treating it as ignored. Still worth doing alongside the other changes on this handler; note the retryable-500 point at internal_views.py:232 interacts with it, so the two are best decided together.
chandrasekharan-zipstack
left a comment
chandrasekharan-zipstack
left a comment
There was a problem hiding this comment.
Standardized review — PR #2213 (FOLLOWUP)
Verdict: REQUEST CHANGES
Prior context fetched — all six gh calls ran clean. Previous review #4865790225 at 0dce94e3 (2026-08-05). "Since" boundary: 0dce94e3. The only author commit since is cc419564 (24 files, 414+/121−); the other three are main merges.
Scope change: NO — cc419564 stays inside the original review surface. No new files, dependencies, public APIs, persisted fields or background jobs.
Summary — Critical: 0 · High: 6 · Medium: 11 · Low: 9 · Lenses run: 16/16
Real progress here — six of the thirteen prior findings are genuinely fixed, and I verified the fixes rather than taking the replies at face value. What holds this at REQUEST CHANGES is that three of the fixes themselves do not do what they claim, and the two headline authz controls are pinned by tests that cannot fail.
Suite confirmed green at head by direct execution against live Postgres (33 passed, 4 subtests). Three findings below are mutation-verified — production code patched, suite re-run, result quoted.
Prior findings status (all 14, nothing dropped)
| # | Severity | Finding | Status |
|---|---|---|---|
| 1 | High | Fail-closing helper, no rollout gate | PARTIALLY RESOLVED — all 7 stale comments fixed; no gate, deliberately. See note below |
| 2 | High | Two scoping layers disagree, pinned in a test | NOT RESOLVED — reworded, but the new wording is still false. See H5 |
| 3 | Medium | "Six internal viewsets" undercounts | PARTIALLY RESOLVED — count dropped; replacement taxonomy also wrong (3 groups, not 2) |
| 4 | High | make_profile_default vacuous coverage |
PARTIALLY RESOLVED — new view-level tests do bite for the transaction property; the tool-scoping control is still vacuous. See H3 |
| 5 | High | mark_extraction_status swallows failures |
RESOLVED — with a new consequence, see M2 |
| 6 | Medium | get_or_create → IntegrityError |
NOT RESOLVED — the fix is unjustified, incomplete and untested. See H1, H2 |
| 7 | Medium | Nullable-hop guard skips the nullable hop | PARTIALLY RESOLVED — walk correctly widened to all hops; keying defect remains, see M4 |
| 8 | Medium | Two override mechanisms | PARTIALLY RESOLVED — documented in both consumers, accurately; still unenforced |
| 9 | Medium | "custom @action never calls filter_queryset()" |
RESOLVED — verified at all seven sites; replacement wording is true |
| 10 | Medium | Two false claims on the output endpoints | RESOLVED — both fixed, plus the same pattern at latest_outputs_by_keys |
| 11 | Medium | Malformed input 500s + full-object save() |
RESOLVED — 400s and update_fields; untested, see M7 |
| 12 | Medium | delete_for_ide silent Redis flags |
PARTIALLY RESOLVED — warning added but fires on the normal path, still 200. See M3 |
| 13 | Medium | migration_utils conflates hidden vs absent |
NOT RESOLVED — the new branch is unreachable. See M1 |
| 14 | — | JsonResponse vs raising (2026-08-24) |
NOT RESOLVED — no push since |
Also from the previous review's unanchored set: validate_tool_instances_internal NOT RESOLVED (see below), import_prompts / sync_prompts NOT RESOLVED (see below), check_files_history OBSOLETE — the function no longer exists at head, removed by a main merge.
On #1, for the record: your reasoning for shipping fail-closed without a gate is sound and I am not re-litigating it. Flagging only that the repo already has Flipt wired (check_feature_flag_status), so the gate is cheaper than the reply implies if you want one.
Unanchored findings (outside the diff hunks)
[High] [Lens 3, 8] — validate_tool_instances_internal still answers {"success": true, "errors": []} after validating zero tools. backend/tool_instance_v2/internal_views.py:345-400. filter_queryset_by_organization now returns .none() when request.organization_id is absent, so tool_instances is [], the loop at :351 never runs, and :391 computes "success": len(validation_errors) == 0 → True, HTTP 200. ToolInstance.objects is a plain ToolInstanceManager, so nothing 404s first. Before this PR the helper returned the queryset unfiltered and validation actually ran — the fail-closed change converts "validated everything" into "validated nothing, reported as pass". InternalAPIAuthMiddleware:158-164 explicitly tolerates a missing header. The handler already holds tool_instance_ids at :328; compare against what came back and return 422/404. This is prior open question 1, still unanswered — if any deployed worker can omit X-Organization-ID, this is Critical.
[High] [Lens 3, 10] — prompt_output returns {"success": true, "data": []} when the newly scoped ToolStudioPrompt manager drops every requested prompt. backend/prompt_studio/prompt_studio_core_v2/internal_views.py:78-93; silent early return at output_manager_helper.py:148-149; consumer at workers/ide_callback/tasks.py:438-448. Pin is tool_id__organization and tool_id is itself nullable (it is in KNOWN_NULLABLE_HOPS). Hidden prompts are dropped with no log; the worker collapses both "success with nothing" and "failure" to [], then emits completed. An LLM run whose outputs were never persisted reads as a completed run with no results. :78-93 is the one place holding both len(prompt_ids) and len(prompts) — compare them.
[Medium] [Lens 3] — two prior unanchored findings remain, both caused by this PR. prompt_studio_helper.py:3002-3004 — import_prompts takes ProfileManager.objects.filter(...).first() with no None check and passes it to every ToolStudioPrompt.objects.create(...) at :3052; the sibling sync_prompts raises on exactly this at :3140. And :3159-3161 — ToolStudioPrompt.objects.filter(tool_id=tool).delete() is now org-filtered, so prompts the scope misses survive alongside their recreated replacements inside the same transaction.atomic(); CustomTool._base_manager is used three lines below, so the asymmetry sits in one screen.
[Medium] — lower-confidence, single-source. ExecutionMetrics serves an all-zeros 200 on missing org context (workflow_manager/internal_views.py:2470-2540). delete_for_ide deletes the DB row before the object-store file with no transaction, so a storage failure returns 400 over an unrecoverable orphan (prompt_studio_core_v2/views.py:1190-1203). The fail-open WARNING is per-queryset, so a bad CONCURRENCY_MODE floods rather than signals (org_aware_manager.py:61-84). ImproperlyConfigured is raised lazily per-query then swallowed into a bool at prompt_studio_index_helper.py:173 — a Django system check would catch it at startup instead.
[Low] — cross-tenant isolation is registered in no tests/critical_paths.yaml entry, so deleting test_cross_org_isolation.py wholesale leaves --fail-on-critical-gap green.
Lens checklist
| # | Lens | Result |
|---|---|---|
| 1 | Spec & intent | See H2, H3, M1 — the commit message claims "both call sites"; three claimed fixes do not hold |
| 2 | Architectural fit | See H2, H5, M6, M9 |
| 3 | Correctness & edge cases | See H1, H4, H6, M1, M5, M11 |
| 4 | Security | See H3, H4, H5 |
| 5 | Data integrity & migrations | See H1. No migration — verified: nothing sets use_in_migrations, no base_manager_name anywhere, _base_manager stays a plain Manager |
| 6 | Concurrency | Clean — assessed directly. of=("self",) correct at migration_utils.py:65 (real join through AdapterInstance); vestigial but harmless at prompt_studio_index_helper.py:122 since _base_manager produces no join. Positive filters give INNER JOINs, so no nullable-outer-join hazard |
| 7 | API & contract compatibility | See M2 — assessed directly. file/delete removal re-confirmed safe (no in-repo caller); the 200→500 change on extraction_status is a worker wire-contract change with no version gate |
| 8 | Reliability & resilience | See H1, H4, M2 |
| 9 | Performance & cost | Clean |
| 10 | Observability | See H6, M1, M3 |
| 11 | Operational safety | See H5 — assessed directly. No rollout gate, confirmed by grep; Flipt infrastructure exists and is unused here |
| 12 | LLM/agent | N/A — assessed directly; no model calls, prompts, tools or evals in the diff |
| 13 | Testing | See H2, H3, M4, M7, M8, M10 |
| 14 | Dependencies & build | N/A — assessed directly; zero dependency or lockfile changes across all six author commits |
| 15 | Code quality | Low only |
| 16 | Doc & comment accuracy | See H5, M1, M4, M6 and the Lows |
Verified fixed, for the record. The @action/filter_queryset() premise corrected at all seven sites with accurate replacement wording. The repo-wide-policy contradiction in organization_utils.py, now scoped locally. ORG_PATH_OVERRIDES precedence documented accurately in both consumers. Both false claims on the output endpoints fixed, plus the identical pattern at latest_outputs_by_keys. Malformed default_profile → 400 and save(update_fields=["is_default"]). All five manager tests bite — removing objects = OrgAwareManager() kills exactly one test each. Both new test directories are collected by CI and gating. Nothing was weakened or deleted by cc419564.
Open questions
- Still unanswered from the last review: can any deployed worker call an internal endpoint without
X-Organization-ID? That decides whether thevalidate_tool_instances_internalfinding is High or Critical. - What concrete scenario motivated the
_base_managerreroute? The unique constraints appear to make the statedIntegrityErrorunreachable at both call sites (H2). If there is a third case, it should be the test. SELECT count(*) FROM custom_tool WHERE organization_id IS NULL(andadapter_instance) — several Mediums collapse if that is zero.
Assumptions
cc419564is the only author work since0dce94e3; the three merge commits carry onlymain. If any merge resolved a conflict in these files, that resolution was not separately reviewed.- The branch is behind
main—fe9c3ff03("Reject non-mapping outputs at the prompt-output internal API") touches the same endpoint as theprompt_outputfinding above and is not in this branch.
Reviewed with unstract:standard-review v0.18.1 — 16-lens rubric, 5 specialist agents, FOLLOWUP mode. Comments are advisory; event: COMMENT, no merge gate.
| # unique_prompt_output_index. `tool` and `document_manager` are | ||
| # already org-verified by the caller. | ||
| prompt_output, success = ( | ||
| PromptStudioOutputManager._base_manager.get_or_create( |
There was a problem hiding this comment.
[High] [Lens 3, 5, 8] — _base_manager create + scoped objects update in one function: the write silently no-ops in exactly the case this change was added for
This moved get_or_create to _base_manager on the premise that the org scope can hide the row. Two statements later, :124-130 runs PromptStudioOutputManager.objects.filter(...).update(**args) through the scoped manager on the same five lookup keys.
If the premise holds, that update matches zero rows — run_id, output, context, challenge_data, highlight_data, confidence_data and word_confidence_data are never written. The row count is discarded, and prompt_output.refresh_from_db() reads via _base_manager, so it succeeds and returns the stale row, which is serialized back as a successful save. The user re-runs a prompt, pays the LLM cost, and sees the previous answer with a 200 and nothing in the logs. On the created=True branch the row keeps defaults but permanently loses run_id.
Before this change the same condition failed loudly (IntegrityError -> AnswerFetchError).
Fix: use one manager for both halves. _base_manager at :124 is consistent with this comment's own "already org-verified by the caller" argument — but then add modified_at to args, because _base_manager is a plain Manager and loses BaseModelQuerySet.update's auto-bump that distinct("prompt_id", "-modified_at") depends on.
Confidence: High on the inconsistency; Medium on frequency — see the next comment, which argues the hiding cannot happen at all here.
| # unique_document_manager_profile_manager_index. The document | ||
| # above was already fetched org-scoped, so the scope is checked | ||
| # either way and this only removes the failure mode. | ||
| index_manager, created = IndexManager._base_manager.select_for_update( |
There was a problem hiding this comment.
[High] [Lens 2, 13] — this reroute switches off the PR's headline control to fix a failure mode the unique constraints make unreachable, is applied to one of two sites, and has zero coverage
The cited IntegrityError cannot occur here. IndexManager's unique key is (document_manager, profile_manager) (models.py:101-105) and its pin is document_manager__tool__organization, while DocumentManager's pin is tool__organization — and document is fetched through the scoped manager at :98. A hidden IndexManager row therefore implies a hidden document, which raises DoesNotExist first. The same argument holds at output_manager_helper.py:85: PromptStudioOutputManager's key includes tool_id (models.py:103-113) and its pin is tool_id__organization, so any row matching all five keys shares the caller's tool and organization.
One of two sites. update_index_instance — the primary indexing write path — still uses IndexManager.objects.get_or_create at :38, despite the commit message saying "both call sites".
Mutation-verified, twice independently. Reverting _base_manager -> objects at both sites leaves the suite fully green: 33 passed in a run of the three new files, 252 passed across prompt_studio + utils. Nothing detects the change in either direction.
Fix: revert both to .objects.get_or_create(...) and correct the comments; or, if a create-half escape is a real concern, handle it once on OrgAwareManager rather than opting individual call sites out. Either way the two sites must agree. If _base_manager survives anywhere, give it a name — all_objects = BaseModelManager() on BaseModel is greppable and immune to a later Meta.base_manager_name, which would today silently re-scope _base_manager back to OrgAwareManager with no test failing.
Confidence: High on unreachability and the coverage gap; Medium that no third scenario was intended — a reproducer would settle it.
| pk=self.a.document.document_id, tool=sibling | ||
| ) | ||
|
|
||
| def test_make_profile_default_lookup_is_tool_scoped(self): |
There was a problem hiding this comment.
[High] [Lens 13, 4] — this test is vacuous; the same-org cross-tool authz control it is named for is unprotected
It asserts ProfileManager.objects.get(pk=..., prompt_studio_tool=...) raises — a property of Django's ORM, not of this diff. The control it exists to pin is at prompt_studio_core_v2/views.py:406-410.
Mutation-verified: drop prompt_studio_tool=prompt_tool, from that get_object_or_404 -> 13 passed, 0 failed. With it dropped, a caller who owns tool X can flip the default profile of tool Y in the same organization, and CI stays green.
The view-level test at :249-277 does not catch it either: it passes org B's profile id, which ProfileManager.objects already hides via org scope, so the mutant still 404s — for the wrong reason.
Fix: add a view-level case that PATCHes make_profile_default on self.a.tool with a profile belonging to a sibling tool in the same org — the fixture at :171-178 already builds one — asserting 404 and that the sibling's own default is unchanged.
This is the vacuity class from the previous review, re-introduced at a new site.
| with self.assertRaises(DocumentManager.DoesNotExist): | ||
| DocumentManager.objects.get( | ||
| pk=self.a.document.document_id, tool=sibling | ||
| ) |
There was a problem hiding this comment.
[High] [Lens 13, 4] — same shape: this test does not exercise the view, and reverting the production lookup passes
The production change is at prompt_studio_core_v2/views.py:1161-1163 (get_object_or_404(DocumentManager, pk=document_id, tool=custom_tool)). Reverting it to the pre-PR DocumentManager.objects.get(pk=document_id) reinstates both same-org cross-tool document deletion and an unhandled DoesNotExist -> 500 instead of 404.
Mutation-verified: that revert -> 13 passed, confirmed twice on a clean checkout. Neither consequence is observed.
Fix: drive delete_for_ide through APIRequestFactory the way _make_profile_default at :197-218 already does, with a sibling-tool document id, asserting 404 and that the document row survives.
| # No request context: Celery, management commands, shell. Not | ||
| # logged — this is the normal state for every query those make, | ||
| # and a line per queryset would drown the case above. | ||
| return qs |
There was a problem hiding this comment.
[High] [Lens 2, 4, 16] — this fails open on unresolvable org, not just "no request context", and the docstring reconciling the two layers is false
UserContext.get_organization() returns a bare None for three distinct states (user_context.py:26-34): no identifier in StateStore, Organization.DoesNotExist, and ProgrammingError. This branch maps all three to "Celery, management commands, shell — return everything". The last two are inside a request with a tenant identifier present.
The reworded docstring at organization_utils.py:84-93 claims the two policies "guard different callers". They do not. /internal/v1/prompt-studio/output/, /index/ and /extraction-status/ (prompt_studio_core_v2/internal_views.py:79,140,205) are @csrf_exempt function views with no filter backend and no call to the fail-closed helper — this manager is their only boundary, on exactly the missing-header condition that helper's own docstring calls "not exotic".
Same viewset, opposite policies: PromptStudioOutputView._required_organization (views.py:57-64) raises 500 when org is None, while get_queryset (:94-97) returns unfiltered for the same condition. Both readings cannot hold.
Minimal fix: if UserContext.get_organization_identifier() and org is None: return qs.none() — fails closed only for the dangerous state, leaving Celery/shell fail-open intact.
Note that test_no_org_context_is_unfiltered pins fail-open as spec — mutation-confirmed, making it fail closed kills exactly that test — so any convergence will read as a test regression unless the test is amended deliberately. That is the "pinned in a test" half of the previous review's finding #2, still standing.
|
|
||
| ``UserContext.get_organization()`` returns None on both | ||
| ``Organization.DoesNotExist`` and ``ProgrammingError``, neither logged. A | ||
| None here compiles to ``organization_id IS NULL``, which matches nothing |
There was a problem hiding this comment.
[Low] [Lens 16] — two comments added in this same commit disagree about whether organization_id IS NULL matches rows
Here: a None org "compiles to organization_id IS NULL, which matches nothing whatever the tool id".
utils/tests/test_org_path_discovery.py:25-32, added in the same commit: DefaultOrganizationMixin declares organization null=True and save() backfills from UserContext, so "a CustomTool or AdapterInstance created by a management command, data migration, Celery task or shell persists with organization_id NULL". Those rows are exactly what IS NULL matches.
The stronger claim is the one that makes the fail-closed APIException look unconditionally safe. Suggest softening to "matches only rows whose organization was never set — never the caller's tool".
| # Resolve the target before clearing anything: the id comes straight | ||
| # from the request body, and clearing first would leave the tool with no | ||
| # default at all when it does not match. Scoped to the same tool the | ||
| # caller already passed authz on, so another tool's id is a 404. |
There was a problem hiding this comment.
[Low] [Lens 16] — this comment and its own test's docstring disagree on whether resolve-before-clear is load-bearing
Here: "clearing first would leave the tool with no default at all when it does not match".
tests/test_cross_org_isolation.py:255-262, for the test that pins this: "Reverting just the ordering, with transaction.atomic() still in place, is genuinely safe and does not fail here."
Your correction in the previous thread was right, and I re-confirmed it by mutation — ordering-only revert with the transaction retained -> 13 passed; full revert to main's clear-first + .objects.get + no atomic -> 1 failed, exactly this test. So the docstring is accurate and the code comment overstates.
Suggest qualifying the comment — the ordering is belt-and-braces given the surrounding transaction — or dropping the causal claim and keeping "resolve before clearing".
| def __init__(self, organization_id=None, path="/internal/test/"): | ||
| if organization_id is not None: | ||
| self.organization_id = organization_id | ||
| self.path = path |
There was a problem hiding this comment.
[Low] [Lens 13] — prior finding NOT fixed: the None leg is still a duplicate
_Request.__init__ guards with if organization_id is not None: self.organization_id = ..., so _Request(organization_id=None) produces an object with no organization_id attribute — byte-for-byte the same object as _Request() in test_missing_org_context_returns_nothing at :56-58. The None subTest therefore duplicates an existing test, and the case it is named for (request.organization_id explicitly set to None) is never constructed.
cc419564 only reworded this module's docstring; _Request was untouched.
Fix: drop the is not None guard and always set the attribute — production reads it via getattr(request, "organization_id", None), so both shapes are legitimate.
| """FileManagement view. | ||
|
|
||
| Handles GET,POST,PUT,PATCH and DELETE | ||
| Handles GET, POST, PUT and PATCH |
There was a problem hiding this comment.
[Low] [Lens 16] — prior finding NOT fixed: this still claims PUT and PATCH
The PR edited this exact line (Handles GET,POST,PUT,PATCH and DELETE -> Handles GET, POST, PUT and PATCH), removing only the DELETE half. urls.py:37-42 binds three paths, all get/post, and FileManagementViewSet defines only list, download (get) and upload (post) — there is no update/partial_update and no PUT/PATCH route anywhere in the repo.
Suggest Handles GET and POST., or dropping the line — the routes are two files away and it will rot again on the next route change.
| except (DatabaseError, TypeError, ImproperlyConfigured): | ||
| # DatabaseError covers IntegrityError/OperationalError, TypeError a | ||
| # malformed extraction_status payload, ImproperlyConfigured a bad | ||
| # org path pin. Narrowed from a bare `except Exception` so an |
There was a problem hiding this comment.
[Low] [Lens 15] — these comments narrate the review conversation rather than the code
"Now reachable two ways", "Narrowed from a bare except Exception", "The diagnosability problem was the log line, not the catch" — each states the change relative to the pre-PR code. Once this PR is history there is no "before" to contrast with, and the reader reconstructs a state of the code that no longer exists. :167 names a construct (except Exception) that is not in the file.
Same pattern at file_execution/internal_views.py:32-35, workflow_manager/internal_views.py:52-55, workflow_v2/views.py:406-409, prompt_studio_core_v2/views.py:394-397 and :1210, and migration_utils.py:69.
CLAUDE.md calls this out specifically — comments should "make sense when reading the code without the session's context". Suggest present-tense rewrites describing the code as it stands ("Fails closed: a caller without X-Organization-ID gets zero rows."), with the archaeology left in the commit message where it already is. No behaviour change.
The of=("self",) note at :112 is also now vestigial — with _base_manager there is no join, so the argument locks nothing extra. It becomes correct again if the _base_manager reroute is resolved by reverting to .objects.
3 participants
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.