Skip to content

[FEAT] Allow unpublishing an exported tool; derive API-key target from the URL - #2206

Open
hari-kuriakose wants to merge 14 commits into
mainfrom
feat/prompt-studio-ergonomics
Open

[FEAT] Allow unpublishing an exported tool; derive API-key target from the URL#2206
hari-kuriakose wants to merge 14 commits into
mainfrom
feat/prompt-studio-ergonomics

Conversation

@hari-kuriakose

@hari-kuriakose hari-kuriakose commented Jul 24, 2026

Copy link
Copy Markdown
Contributor

Purpose

Two API ergonomics fixes: unpublishing an exported tool, and creating an API key without repeating an identifier that is already in the URL. Review surfaced a same-org IDOR on the API-key path, which is closed here as well — see §2.

A third related item — making a no-RAG profile omit the vector DB / embedding model — is deliberately excluded; see "Descoped" below.


1. No way to delete an exported registry tool

The registry is read-only over the API — prompt_studio_registry_v2/urls.py maps only {"get": "list"}. The only way to remove an entry is to delete the Prompt Studio project, which cascades to it. That is implicit, undocumented, and blunt: a tool cannot be unpublished while keeping the project.

Adds DELETE registry/<pk>/, guarded by the same in-use check the Prompt Studio project delete already performs — a tool still attached to a workflow is refused with 409 rather than silently breaking those workflows. The guard filters ToolInstance on tool_id=instance.pk, matching the existing check in prompt_studio_core_v2/views.py:230 (_check_tool_usage_in_workflows; the check itself is at :244) — an exported tool's tool_id is its prompt_registry_id.

The 409 names the blocking deployment types (API Deployment / ETL Pipeline / Task Pipeline / Human in the Loop) by running the same _get_deployment_types logic as the core path (:249), so the refusal is actionable rather than a bare "no".

Unpublishing is not reversible in place. Re-exporting mints a fresh prompt_registry_id and does not carry over shared_to_org / shared_users. Nothing dangles — but anything holding the old UUID (saved workflow JSON, Postman collections, docs) silently stops resolving and must be updated.

Detail-route queryset: get_queryset previously returned None when no query-param filters were present, which would break get_object() on a detail route. It now returns the full queryset when addressing a single row by PK, keyed off self.kwargs.get("pk") rather than self.detail — DRF sets cls.detail = None under a manual as_view() (it is only populated for router-generated views), which this viewset uses. The existing list route is unaffected (no pk kwarg → same filter path as before).

Authorization — this is the first by-PK operation on this viewset (it previously exposed only list). DEFAULT_PERMISSION_CLASSES is empty and the viewset declared no permission_classes. OrganizationFilterBackend runs inside get_object() (via filter_queryset), so cross-org deletion was never possible — but any member of the same org could otherwise delete another member's exported tool by PK. IsRegistryToolOwner gates only destroy:

  • Ownership inherited from the linked CustomTool, mirroring IsParentToolOwner (same pattern for ProfileManager).
  • Falls back to the row's own owner for unlinked legacy rows (custom_tool is nullable).
  • Service accounts and org admins admitted, matching sibling permission classes.
  • list visibility is untouched — still derived by list_tools. Only the destructive route is restricted.

2. API-key creation requires an identifier already present in the URL

POST keys/api/<api_id>/ takes api_id as a path segment but also expects api in the body — the same value, spelled twice. Omitting the body field fails validation. POST keys/pipeline/<pipeline_id>/ has the identical shape.

POST routes to the default ModelViewSet.create, which never sees the URL kwargs, so the body has to repeat them. create now derives the target from the path when present, and falls through to the default implementation for the body-only routes (keys/api/, keys/pipeline/) — both keep working.

The path target is authoritative, not a default: a body naming the other target is a contradiction and is refused with a 400 rather than silently creating a key for whichever one wins. Assigning rather than setdefault-ing also means {"api": ""} no longer defeats the derivation into a confusing 400, and a non-mapping (JSON array) body is rejected as a 400 instead of AttributeError-ing into a 500.

Closes an IDOR on key creation

create performed no ownership check on the target deployment. get_permissions returns IsParentDeploymentOwner for it, but that class implements only has_object_permission — and create is collection-level, so DRF never calls get_object() / check_object_permissions. has_permission fell through to BasePermission's default True. Any authenticated org member could POST keys/api/<someone-else's-api-id>/ and mint a live key for a deployment they do not own. (Cross-org was already blocked by the org-scoped default manager; this was same-org.)

This pre-existed the PR — the body-based path had the identical gap — but the change makes it reachable from the path segment alone and touches exactly the method where the fix belongs, so it is closed here. create now resolves the target from path or body and object-checks it on every route: guarding only the path form would have left the identical hole reachable by moving the identifier into the body.

A malformed identifier is also handled at the fetch boundary. pk is a UUID column, so a non-UUID value raises ValidationError (not DoesNotExist) out of to_python; unguarded, reading the target from the body before the serializer runs would turn ordinary client garbage into a 500. get_api_by_id / get_pipeline_by_id now treat it as "not found".

That required a fix in IsParentDeploymentOwner itself. Its body reads obj.api or obj.pipeline or obj, and the target handed to it is an APIDeployment / Pipeline — neither of which declares an api or pipeline field (APIKey.api points at the deployment, related_name="api_keys"). A bare attribute access would raise AttributeError500 on every key creation. The lookups are now getattr guarded, falling through to obj; both parents carry memberships, so _is_resource_owner resolves correctly against them. Behaviour on APIKey detail routes is unchanged.

The pipeline branch uses a new PipelineProcessor.get_pipeline_by_id rather than get_active_pipeline. Minting a key does not require a running pipeline, and get_active_pipeline would both 422 on a paused one (Pipeline.active defaults to False) and disclose its state before the ownership check — the exact information the check exists to protect. get_active_pipeline itself is untouched.


Descoped: making a no-RAG profile omit the vector DB / embedding model

One candidate change was to make vector_store and embedding_model nullable so a chunk_size=0 ("no RAG") profile need not supply adapters that are never read. It is excluded because the premise does not hold in the code.

The rationale would be that with chunk_size=0 neither the vector DB nor the embedding model is queried. But build_single_pass_payload sets default_profile.chunk_size = 0 at prompt_studio_helper.py:1140 and then reads both FKs 25 lines later, in the same function:

default_profile.chunk_size = 0        # :1140
...
vector_db = str(default_profile.vector_store.id)        # :1165
embedding_model = str(default_profile.embedding_model.id)  # :1166

Export does the same unconditionally (prompt_studio_registry_helper.py:269-270). Across the backend there are 46 unguarded reads of these two FKs, spanning indexing, export, single-pass, and permission validation.

Making the columns nullable without guarding every reader would convert a clear 400 at profile creation into AttributeError: 'NoneType' object has no attribute 'id' — a 500 deep inside single-pass or export, in exactly the mode the change claims is safe. The serializer-only alternative does not escape this: storing null still requires the migration, which exposes the same readers.

There is no ergonomics-sized version of this change. It needs the migration plus a deliberate decision about what each read path does with an absent adapter — its own PR.


Verification

  • ruff and ruff-format pass at the version pinned in .pre-commit-config.yaml (v0.3.4).
  • No model or migration changes in this PR.
  • 41 unit tests pass (api_v2/tests/test_api_key_create_target.py, prompt_studio/prompt_studio_registry_v2/tests/test_registry_tool_delete_guards.py), under both cd backend && pytest and repo-root pytest backend/....
  • Tests execute the real method bodies rather than asserting on source text, and are verified by mutation. Each of these fails the suite: re-opening the body-only IDOR, making the authorization check fail open, unwiring destroy from its guard, narrowing or broadening the malformed-id catch, dropping a deployment-type branch, and breaking the tool_id filter.
  • Django settings are not configured in the unit tier, so route binding and the live permission cycle (which need a database) remain integration-tier; backend/conftest.py auto-marks such tests integration. The guard logic is covered per-PR.
  • Pre-existing collection errors in the wider backend suite (9 modules, ImproperlyConfigured) are unchanged by this PR — verified against the base commit.

Review

Went through the team's standardized 15-lens review. Three rounds; each found real defects in the preceding round's fix, which are folded into the commits above:

  1. The path-only fix left the body-only route open (Critical), the pipeline lookup narrowed a status contract and leaked state (High), the authorization check failed open on unexpected types (High), and two test suites passed against broken code (High).
  2. Widening create to read the body introduced a 500 on malformed identifiers (High).
  3. The helper tests for that fix asserted on source text rather than behaviour (Medium).

Round 4 is clean. A separate behaviour-preserving commit shares the test extraction helper; the reviewed source files are byte-identical across it.

Impact

  • A tool can be unpublished without deleting its Prompt Studio project; in-use tools are refused with 409, and deletion is restricted to the project's owner / org admins.
  • The path identifier alone is enough to create an API key; existing body-based callers are unaffected.
  • API-key creation now verifies ownership of the target deployment/pipeline, closing a same-org IDOR that predates this PR.
  • A body contradicting the path target is refused with a 400 instead of resolving to one of them.

Related

Part of a set of independent Prompt Studio / registry fixes: #2203, #2204, #2209. No code dependency between them.

🤖 Generated with Claude Code

@coderabbitai

coderabbitai Bot commented Jul 24, 2026

Copy link
Copy Markdown
Contributor

Note

Reviews paused

It looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

  • @coderabbitai resume to resume automatic reviews.
  • @coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

  • ▶️ Resume reviews
  • 🔍 Trigger review

Walkthrough

Adds nested-route API key creation and introduces guarded registry deletion with owner authorization, UUID detail routing, query-safe object lookup, and a 409 conflict when workflows still reference a registry tool.

Changes

Nested API key creation

Layer / File(s) Summary
Path-derived API key creation
backend/api_v2/api_key_views.py
APIKeyViewSet.create populates missing API or pipeline fields from URL parameters, validates and creates the key, and retains the superclass flow for unscoped requests.

Guarded registry deletion

Layer / File(s) Summary
Registry deletion route and authorization
backend/prompt_studio/prompt_studio_registry_v2/urls.py, backend/prompt_studio/prompt_studio_registry_v2/views.py, backend/prompt_studio/permission.py, backend/prompt_studio/prompt_studio_registry_v2/tests/test_registry_tool_delete_guards.py
A UUID detail route maps DELETE requests to destroy; routed object lookup bypasses query-parameter filtering, and owner authorization covers linked, legacy, administrator, and service-account cases.
Workflow reference deletion guard
backend/prompt_studio/prompt_studio_registry_v2/views.py, backend/prompt_studio/prompt_studio_registry_v2/exceptions.py, backend/prompt_studio/prompt_studio_registry_v2/tests/test_registry_tool_delete_guards.py
Registry deletion checks distinct dependent workflow IDs, raises a 409 RegistryToolInUseError when references exist, and otherwise delegates to the base destroy handler.

Estimated code review effort: 3 (Moderate) | ~20 minutes

Sequence Diagram(s)

sequenceDiagram
  actor Client
  participant PromptStudioRegistryView
  participant IsRegistryToolOwner
  participant ToolInstance
  Client->>PromptStudioRegistryView: DELETE registry/{pk}
  PromptStudioRegistryView->>IsRegistryToolOwner: Check destroy permission
  IsRegistryToolOwner-->>PromptStudioRegistryView: Allow or deny request
  PromptStudioRegistryView->>ToolInstance: Query workflow references
  ToolInstance-->>PromptStudioRegistryView: Return distinct workflow IDs
  PromptStudioRegistryView-->>Client: 409 conflict or deletion response
Loading
🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 63.53% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
Title check ✅ Passed The title clearly summarizes both primary changes: registry tool unpublishing and URL-derived API-key targets.
Description check ✅ Passed The description thoroughly explains the changes, rationale, risks, testing, migrations, configuration, and related issues.
✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch feat/prompt-studio-ergonomics

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@hari-kuriakose
hari-kuriakose force-pushed the feat/prompt-studio-ergonomics branch from a26122b to 3519b75 Compare July 24, 2026 10:13
@greptile-apps

greptile-apps Bot commented Jul 24, 2026

Copy link
Copy Markdown
Contributor

Greptile Summary

The PR adds guarded registry-tool unpublishing and derives API-key targets from request URLs while enforcing target ownership.

  • Adds a DELETE route for exported registry tools with owner/admin authorization and workflow-usage refusal.
  • Consolidates workflow dependency and deployment-type reporting helpers.
  • Derives API or pipeline targets during API-key creation, rejects contradictory inputs, and checks object permissions across path- and body-based routes.
  • Handles malformed deployment identifiers as not found and adds focused behavioral and route-wiring tests.

Confidence Score: 5/5

The PR appears safe to merge because no additional blocking failure eligible for this follow-up review remains.

No blocking failure remains beyond the dependency check-delete race already documented in the existing inline thread.

Important Files Changed

Filename Overview
backend/prompt_studio/prompt_studio_registry_v2/views.py Adds the organization-scoped, owner-gated registry deletion route and refuses deletion when existing workflow references are found.
backend/prompt_studio/tool_usage.py Centralizes workflow dependency lookup and deployment-type formatting for registry and Prompt Studio deletion paths.
backend/prompt_studio/permission.py Adds object-level ownership authorization for unpublishing registry tools.
backend/api_v2/api_key_views.py Resolves API-key targets from either URL or body, rejects contradictory targets, and explicitly applies object permissions before creation.
backend/permissions/permission.py Extends parent-deployment authorization to safely recognize both API-key rows and their parent resources.
backend/pipeline_v2/pipeline_processor.py Adds inactive-agnostic pipeline lookup and converts malformed UUID lookup errors into not-found results.
backend/api_v2/utils.py Converts malformed API deployment identifiers into not-found results.
backend/tests_common/test_route_wiring.py Verifies that the new guarded methods are bound to their intended routes and permission classes.

Reviews (10): Last reviewed commit: "[FIX] Unbreak pre-commit.ci: attribute d..." | Re-trigger Greptile

Comment thread backend/prompt_studio/prompt_studio_registry_v2/views.py
Comment thread backend/prompt_studio/prompt_studio_registry_v2/views.py Outdated

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 3

🧹 Nitpick comments (1)
backend/prompt_studio/prompt_studio_registry_v2/views.py (1)

50-52: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Correct the variadic parameter annotations.

*args: tuple[Any] annotates each positional argument as a tuple, while **kwargs: dict[str, Any] annotates each keyword value as a dictionary; type checkers interpret variable-argument annotations this way. Use *args: Any, **kwargs: Any (or an existing project-standard DRF-compatible signature).

Proposed fix
     def Destroy(
-        self, request: Request, *args: tuple[Any], **kwargs: dict[str, Any]
+        self, request: Request, *args: Any, **kwargs: Any
     ) -> Response:
🤖 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_registry_v2/views.py` around lines 50 -
52, Update the destroy method signature to annotate variadic parameters as
individual values: use Any for both args and kwargs rather than tuple[Any] and
dict[str, Any]. Preserve the existing return type and method behavior.
🤖 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/api_v2/api_key_views.py`:
- Around line 40-43: Update the request-data target injection logic in the API
key view so the URL-derived api_id or pipeline_id is added only when neither
“api” nor “pipeline” is already present in the body. Preserve body values and
prevent both target fields from being populated before APIKeySerializer.validate
runs.

In `@backend/prompt_studio/prompt_studio_registry_v2/views.py`:
- Around line 61-69: Update the dependency guard in the tool deletion flow to
use a queryset `.exists()` check instead of materializing workflow IDs with
`set(values_list(...).distinct())`. Keep the existing deletion prevention
behavior, and change the `logger.info` call to report only a count or generic
dependency message rather than listing workflow IDs.
- Around line 60-72: Update the destroy flow containing
PromptStudioRegistryViewSet.destroy so the dependent-workflow check and registry
deletion execute within one database transaction, locking the registry and
reusing that locked instance rather than allowing super().destroy() to re-fetch
it. Add database-level protection or an equivalent transactional safeguard for
concurrent ToolInstance attachments, preserving RegistryToolInUseError when
dependencies exist, and add a concurrency test covering an attachment racing
with deletion.

---

Nitpick comments:
In `@backend/prompt_studio/prompt_studio_registry_v2/views.py`:
- Around line 50-52: Update the destroy method signature to annotate variadic
parameters as individual values: use Any for both args and kwargs rather than
tuple[Any] and dict[str, Any]. Preserve the existing return type and method
behavior.
🪄 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: ac14b42f-0572-4c09-a8dd-a6eaf024961a

📥 Commits

Reviewing files that changed from the base of the PR and between 023b140 and a26122b.

📒 Files selected for processing (4)
  • backend/api_v2/api_key_views.py
  • backend/prompt_studio/prompt_studio_registry_v2/exceptions.py
  • backend/prompt_studio/prompt_studio_registry_v2/urls.py
  • backend/prompt_studio/prompt_studio_registry_v2/views.py

Comment thread backend/api_v2/api_key_views.py Outdated
Comment thread backend/prompt_studio/prompt_studio_registry_v2/views.py Outdated
Comment thread backend/prompt_studio/prompt_studio_registry_v2/views.py Outdated
Comment thread backend/prompt_studio/permission.py
@hari-kuriakose
hari-kuriakose force-pushed the feat/prompt-studio-ergonomics branch from 8235086 to f41ee0e Compare July 24, 2026 10:32

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 19

Note

Due to the large number of review comments, Critical, Major severity comments were prioritized as inline comments.

🟡 Minor comments (6)
prompt-service/src/unstract/prompt_service/plugins/simple_prompt_studio/src/base.py-46-53 (1)

46-53: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win

Validate output before indexing.

output = payload.get(PSKeys.OUTPUT) yields None when the key is absent, and lines 51-53 immediately index into it (output[PSKeys.NAME]), producing an unhandled TypeError/500 instead of a clear BadRequest.

🛡️ Proposed fix
     output = payload.get(PSKeys.OUTPUT)
     tool_id: str = payload.get(PSKeys.TOOL_ID, "")
     file_hash = payload.get(PSKeys.FILE_HASH)
     structured_output: dict[str, Any] = {}
+    if not output:
+        raise BadRequest("No output provided in the request.")
🤖 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
`@prompt-service/src/unstract/prompt_service/plugins/simple_prompt_studio/src/base.py`
around lines 46 - 53, Validate that output retrieved in the payload-processing
method is present before accessing PSKeys.NAME or PSKeys.PROMPT. If it is
missing or invalid, raise the established BadRequest error with a clear message;
otherwise preserve the existing variable_names, prompt_name, and promptx
initialization flow.
prompt-service/src/unstract/prompt_service/plugins/simple_prompt_studio/README.md-5-5 (1)

5-5: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win

Document the registered endpoint.

The README advertises /answer-sps, but SimplePromptStudio registers /answer-prompt-public in prompt-service/src/unstract/prompt_service/plugins/simple_prompt_studio/src/base.py. Update the README to prevent clients from receiving 404 responses.

Proposed fix
-If the plugin is not disabled, it registers a new endpoint `/answer-sps` which extracts the prompt result without authentication.
+If the plugin is not disabled, it registers a new endpoint `/answer-prompt-public` which extracts the prompt result without authentication.
🤖 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
`@prompt-service/src/unstract/prompt_service/plugins/simple_prompt_studio/README.md`
at line 5, Update the SimplePromptStudio README to document the registered
endpoint as /answer-prompt-public instead of /answer-sps, matching the route
defined by the SimplePromptStudio implementation and preventing clients from
using the obsolete path.
prompt-service/src/unstract/prompt_service/plugins/smart_table_extractor/src/interface/runner.py-199-204 (1)

199-204: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win

headers in the final output reflects only the last processed sheet. For multi-sheet workbooks, headers is the loop variable left over from the final iteration, so metadata.detected_headers misrepresents earlier sheets. Consider aggregating headers per sheet (or dropping the single headers field from the assembled metadata).

🤖 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
`@prompt-service/src/unstract/prompt_service/plugins/smart_table_extractor/src/interface/runner.py`
around lines 199 - 204, Update the SmartTableExtractorRunner final-output
assembly so metadata does not use the loop-scoped headers from only the last
processed sheet. Aggregate detected headers for every sheet and pass that
collection to _assemble_final_output, or remove the single headers field from
the assembled metadata while preserving accurate multi-sheet output.
prompt-service/src/unstract/prompt_service/plugins/evaluation/src/base.py-172-176 (1)

172-176: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win

Missing eval_settings keys are reported as failures, not "disabled".

self.settings defaults to {} (Line 88), so self.settings["evaluate"] here (and self.settings["monitor_llm"] at Line 180) raise KeyError when a prompt has no eval settings. That KeyError is caught by the outer handler at Line 242 and re-raised as EvalFailedError, whereas the intended semantics for an unconfigured prompt is EvalDisabledError. Prefer .get(...) with an explicit disabled check.

🛠️ Proposed fix
-            if not self.response or self.settings["evaluate"] is not True:
+            if not self.response or self.settings.get("evaluate") is not True:
                 raise EvalDisabledError()
🤖 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 `@prompt-service/src/unstract/prompt_service/plugins/evaluation/src/base.py`
around lines 172 - 176, Update the evaluation checks in the relevant base-class
flow around the response fallback and the `monitor_llm` handling to use
`self.settings.get(...)` with explicit disabled defaults instead of direct key
access. Ensure prompts with missing `evaluate` or `monitor_llm` settings raise
`EvalDisabledError` rather than allowing `KeyError` to become `EvalFailedError`,
while preserving enabled-setting behavior.
prompt-service/src/unstract/prompt_service/plugins/challenge/src/base.py-101-114 (1)

101-114: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win

LLM failure silently returns a passing score.

If challenge_llm.complete raises (caught at Line 107), current_answer is never bound. Line 110 then hits UnboundLocalError, which is caught by the broad except Exception at Line 111, logged as a misleading "JSON format error", and the method returns default_answer (score: 5). A genuine LLM/transport failure is thus indistinguishable from a valid pass and skips the retry path in run().

🐛 Proposed fix
         try:
             completion = self.challenge_llm.complete(
                 prompt=prompt,
             )
             current_answer = completion["response"].text
-        # TODO: Use another LLM to complete the prompt
         except Exception as e:
             app.logger.error("Error completing prompt: %s.", str(e))
+            return default_answer
         try:
             return json.loads(current_answer)
🤖 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 `@prompt-service/src/unstract/prompt_service/plugins/challenge/src/base.py`
around lines 101 - 114, Update the completion and parsing flow in the method
containing challenge_llm.complete so an LLM failure does not fall through to
JSON parsing or return default_answer. Handle the exception by propagating or
returning a failure result that run() can recognize and retry, while reserving
the “JSON format error” path for responses that were actually received but
cannot be parsed.
prompt-service/src/unstract/prompt_service/plugins/table_extractor/src/prompts/bank_statement/postprocessing.txt-14-19 (1)

14-19: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win

Malformed JSON in the example schema — missing closing quote.

Line 16's "line_no_end": "0x1, is missing the closing " before the comma (compare with line 15's correctly-formed "line_no_start": "0x1",). This is the reference schema shown to the LLM for generating the post-processing script; a malformed example risks confusing field/type inference.

🐛 Proposed fix
-        "line_no_end": "0x1,
+        "line_no_end": "0x1",
🤖 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
`@prompt-service/src/unstract/prompt_service/plugins/table_extractor/src/prompts/bank_statement/postprocessing.txt`
around lines 14 - 19, The example schema in the post-processing prompt contains
malformed JSON: fix the line_no_end field value in the schema example by adding
its missing closing quote, matching the valid line_no_start representation and
preserving the intended string type.
🧹 Nitpick comments (12)
prompt-service/src/unstract/prompt_service/plugins/single_pass_extraction/src/constants.py (1)

43-46: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

Duplicate FILE_PATH definition.

FILE_PATH = "file_path" is declared on both line 43 and line 46; the second is redundant. Harmless (same value) but worth removing to avoid confusion.

🤖 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
`@prompt-service/src/unstract/prompt_service/plugins/single_pass_extraction/src/constants.py`
around lines 43 - 46, Remove the redundant second FILE_PATH constant declaration
in the constants module, preserving the first FILE_PATH = "file_path" definition
and all other constants unchanged.
prompt-service/src/unstract/prompt_service/plugins/smart_table_extractor/src/processor/__init__.py (1)

6-6: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

Sort the public export list.

Ruff reports RUF022 here. Use ["BatchProcessor", "HeaderDetector"] to clear the lint finding.

🤖 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
`@prompt-service/src/unstract/prompt_service/plugins/smart_table_extractor/src/processor/__init__.py`
at line 6, Sort the __all__ export list alphabetically by placing BatchProcessor
before HeaderDetector to resolve Ruff RUF022.

Source: Linters/SAST tools

prompt-service/src/unstract/prompt_service/plugins/smart_table_extractor/src/processor/batch_processor.py (1)

98-100: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Preserve the traceback in error logs.

Use logger.exception("Failed to process batches") inside this handler; SonarCloud currently flags the missing exception traceback.

🤖 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
`@prompt-service/src/unstract/prompt_service/plugins/smart_table_extractor/src/processor/batch_processor.py`
around lines 98 - 100, Update the exception handler in the batch-processing flow
to use logger.exception("Failed to process batches") instead of logger.error,
preserving the active traceback while keeping the existing
BatchProcessingException propagation unchanged.

Source: Linters/SAST tools

prompt-service/src/unstract/prompt_service/plugins/smart_table_extractor/src/converter/excel_to_tsv.py (1)

128-146: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Use logger.exception(...) inside these except blocks. These handlers log with logger.error while an exception is in flight; logger.exception captures the traceback and clears the SonarCloud failures reported on lines 129/135/143 (also applies to the except Exception at line 152/153).

🤖 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
`@prompt-service/src/unstract/prompt_service/plugins/smart_table_extractor/src/converter/excel_to_tsv.py`
around lines 128 - 146, The exception handlers in the Excel-to-TSV conversion
flow should use logger.exception instead of logger.error so active exception
tracebacks are captured. Update the handlers for
requests.exceptions.ConnectionError, requests.exceptions.RequestException, and
the broad Exception around the conversion logic, preserving their existing
messages and FileConversionException behavior.

Source: Linters/SAST tools

prompt-service/src/unstract/prompt_service/plugins/evaluation/src/__init__.py (1)

13-13: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

Global ResourceWarning suppression leaks beyond this plugin.

warnings.simplefilter mutates the process-wide filter at import time, so any host that imports this plugin loses ResourceWarning visibility everywhere (potentially masking unrelated leaks). Consider scoping it with warnings.catch_warnings()/filterwarnings around the evaluator execution instead of a module-import side effect.

🤖 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
`@prompt-service/src/unstract/prompt_service/plugins/evaluation/src/__init__.py`
at line 13, Remove the module-import-time warnings.simplefilter call in the
plugin initializer and scope the ResourceWarning suppression to the evaluator
execution path instead. Use warnings.catch_warnings with filterwarnings around
the relevant evaluation operation, ensuring the process-wide warning filter is
restored after execution.
prompt-service/src/unstract/prompt_service/plugins/table_extractor/src/extractor/core.py (3)

114-118: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Re-raise without exception chaining.

raise PluginException(...) inside the except FileNotFoundError block loses the original traceback context.

♻️ Proposed fix
         except FileNotFoundError as file_not_found:
             raise PluginException(
                 f"Input file {self.input_file} is "
                 f"not found in the path : {file_not_found}"
-            )
+            ) from file_not_found
🤖 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
`@prompt-service/src/unstract/prompt_service/plugins/table_extractor/src/extractor/core.py`
around lines 114 - 118, Update the FileNotFoundError handler in the extractor’s
exception flow to re-raise PluginException without chaining the original
exception, while preserving the existing message content. Use explicit exception
suppression on the raise in the except FileNotFoundError block.

Source: Linters/SAST tools


421-442: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

Large blocks of commented-out code left in place.

These dead-code blocks (header-normalization logic and header-prepending logic) add noise without being active. Consider removing or moving to a comment/ADR if kept for reference.

Also applies to: 549-553

🤖 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
`@prompt-service/src/unstract/prompt_service/plugins/table_extractor/src/extractor/core.py`
around lines 421 - 442, Remove the commented-out header normalization and
header-prepending blocks near the affected sections, including the corresponding
block around the later referenced lines. Keep only active implementation code;
do not retain dead code inline, and preserve any required behavior through the
existing live logic.

54-54: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

Mutable default arguments (headers: list = []) repeated across 5 signatures.

Flagged by Ruff (B006) and SonarCloud. None of these currently mutate the default in place, so there's no active aliasing bug, but it's a latent footgun if future edits mutate the list directly.

♻️ Example fix pattern
-    def extract_header(self, page_no: int, headers: list[str] = []) -> list[str]:
+    def extract_header(self, page_no: int, headers: list[str] | None = None) -> list[str]:
+        headers = headers or []

Also applies to: 217-217, 270-270, 326-326, 481-481

🤖 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
`@prompt-service/src/unstract/prompt_service/plugins/table_extractor/src/extractor/core.py`
at line 54, Replace the mutable [] defaults for headers in all five affected
function signatures with a non-shared default, such as None, and initialize an
empty list inside each function when needed. Preserve existing behavior for
callers that provide headers, and update every headers signature including those
near the referenced locations.

Source: Linters/SAST tools

prompt-service/src/unstract/prompt_service/plugins/table_extractor/pyproject.toml (1)

6-6: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

flake8 listed as a runtime dependency.

flake8 is a linting tool and isn't imported anywhere in this plugin's source; it appears to be a dev-only tool accidentally added to dependencies instead of a dev/lint extras group.

🤖 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
`@prompt-service/src/unstract/prompt_service/plugins/table_extractor/pyproject.toml`
at line 6, Remove flake8 from the runtime dependencies in the pyproject.toml
dependencies declaration; keep only the packages required by the plugin at
runtime, and do not add a replacement dev group unless one already exists for
linting.
prompt-service/src/unstract/prompt_service/plugins/table_extractor/src/processor/base_processor.py (1)

82-82: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

TODO left in shipped code.

# TODO Extract document metadata : indicates unfinished work. Happy to help implement metadata extraction here if desired.

🤖 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
`@prompt-service/src/unstract/prompt_service/plugins/table_extractor/src/processor/base_processor.py`
at line 82, Remove the unfinished “Extract document metadata” TODO from the
affected processor code, or implement the metadata extraction before retaining
any note; do not leave the TODO in shipped code.
prompt-service/src/unstract/prompt_service/plugins/table_extractor/src/processor/post_processor.py (2)

27-27: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Use logger.exception() in exception handlers; CI (SonarCloud) is failing on these lines.

The SonarCloud check flags lines 66, 144-145, and 154 to use logging.exception() (which captures the stack trace) instead of logger.error(f"... {e}"). run_python_program_on_response (line 27) is also flagged for cognitive complexity exceeding the configured threshold (17 vs 15).

Also applies to: 66-66, 144-145, 153-154

🤖 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
`@prompt-service/src/unstract/prompt_service/plugins/table_extractor/src/processor/post_processor.py`
at line 27, Update exception handlers in run_python_program_on_response and the
handlers around the flagged lines to use logger.exception() instead of
logger.error(f"... {e}"), preserving the existing context messages while
allowing stack traces to be captured. Reduce run_python_program_on_response’s
cognitive complexity below the configured threshold by extracting cohesive logic
into small helper functions without changing behavior.

Source: Pipeline failures


157-165: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick win

Redundant double CSV parse; consider DataFrame.to_numpy().

coloumns is derived from a first pd.read_csv call and then immediately passed back as usecols to a second, identical parse of the same TSV — the second read only re-parses the same data with no filtering effect. Also, df.values.tolist() is flagged by SonarCloud in favor of df.to_numpy().tolist().

♻️ Proposed simplification
     def _process_csv_to_json(cleaned_tsv: str) -> dict[str, list[Any]]:
-        coloumns = pd.read_csv(StringIO(cleaned_tsv), sep="\t").columns
-        df = pd.read_csv(StringIO(cleaned_tsv), sep="\t", usecols=coloumns)
+        df = pd.read_csv(StringIO(cleaned_tsv), sep="\t")
         df.fillna("", inplace=True)
         output: dict[str, list[Any]] = {
             "column_headers": df.columns.tolist(),
-            "rows": df.values.tolist(),
+            "rows": df.to_numpy().tolist(),
         }
         return output
🤖 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
`@prompt-service/src/unstract/prompt_service/plugins/table_extractor/src/processor/post_processor.py`
around lines 157 - 165, Update _process_csv_to_json to parse the TSV only once,
removing the redundant coloumns read and usecols filtering, then preserve the
existing fillna and output structure. Replace df.values.tolist() with
df.to_numpy().tolist() when building rows.
🤖 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
`@prompt-service/src/unstract/prompt_service/plugins/evaluation/src/frameworks/base.py`:
- Around line 190-275: Reduce cognitive complexity in gen_final_results by
extracting key parsing, judge-remark aggregation, and post-processing into
focused helper methods. Preserve the existing score averaging, quorum
validation, feedback construction, and _format_result behavior, including
score_total, line-item ID/name, release, and disabled handling. Keep
gen_final_results as the orchestration method that iterates judges_remarks and
appends each formatted result.
- Around line 277-305: The run method returns unresolved evaluator futures,
allowing callers to read gen_final_results before callbacks update shared state.
Update run and its caller flow to await or resolve every item in the returned
futures collection before invoking gen_final_results or consuming evaluator
output, while preserving the existing evaluator scheduling behavior.

In
`@prompt-service/src/unstract/prompt_service/plugins/line_item_extraction/src/base.py`:
- Around line 46-47: Convert the LINE_ITEM_EXTRACTION_MAX_LLM_CALLS environment
value to an integer when assigning MAX_ATTEMPTS, while retaining 5 as the
integer default, so the attempts comparison in the loop remains type-compatible.

In
`@prompt-service/src/unstract/prompt_service/plugins/single_pass_extraction/src/base.py`:
- Around line 161-173: In the challenge setup flow, initialize challenge_llm to
None before plugin lookup, make exception handling catch a concrete exception
type without subscripting challenge_plugin, and build challenge_metrics only
when challenge_llm was successfully created rather than whenever
enable_challenge is truthy. Preserve the existing no-plugin logging and metrics
behavior for successful challenge initialization.

In
`@prompt-service/src/unstract/prompt_service/plugins/smart_table_extractor/src/interface/runner.py`:
- Around line 240-242: In the JSONDecodeError handler within the schema parsing
flow, update the SmartTableExtractorKeys.SCHEMA assignment to use the repaired
value returned by repair_json(schema) rather than the original invalid schema.
Ensure the repaired variable is consumed so downstream processing receives the
corrected schema and no unused-variable warning remains.
- Around line 74-104: Update SmartTableExtractorRunner.run to obtain the schema
and input_file from table_settings when the corresponding arguments are not
provided, so _validate_inputs and file processing receive valid values. Default
a missing fs_instance to the local filesystem provider before calling read,
while preserving explicitly supplied providers for remote files. Add the
required provider import and keep the existing extraction flow unchanged.

In
`@prompt-service/src/unstract/prompt_service/plugins/smart_table_extractor/src/processor/batch_processor.py`:
- Around line 114-124: Update header detection to return both the detected
headers and their source row index, rather than inferring the index from column
count alone. Propagate this index into batching and slice rows starting
immediately after that exact header position, ensuring same-width title or
preamble rows are not treated as headers.

In `@prompt-service/src/unstract/prompt_service/plugins/summarize/src/base.py`:
- Around line 31-45: Update the payload validation used by the summarize request
flow around validate_payload and enhance_summarize_prompt to enforce that the
JSON body is an object and optional fields have their expected types, especially
ensuring prompt_keys is a list before prompt construction. Reject invalid field
types with the existing BadRequest path so malformed requests do not reach
enhance_summarize_prompt or produce a 500 response.
- Around line 60-64: Update the exception handler in the summarize method to
raise the constructed InternalServerError(error) instead of merely instantiating
it, ensuring LLM failures do not fall through to return result or produce a
successful response. Preserve the existing logging and error message.

In `@prompt-service/src/unstract/prompt_service/plugins/summarize/src/helper.py`:
- Around line 15-18: Validate the request payload before the prompt-building
logic around prompt_keys and before calling Summarize.summarize: require a JSON
object, ensure prompt_keys is a list containing only strings, and reject null or
blank required fields. Return HTTP 400 for any malformed payload, and only
execute the existing prompt construction when validation succeeds.

In
`@prompt-service/src/unstract/prompt_service/plugins/table_extractor/src/extractor/core.py`:
- Around line 120-146: Avoid mutating the shared prompt definitions in
__initalize_prompts by deep-copying Prompts.BASE_PROMPTS[mode] before resolving
file references and updating self.prompts. In
prompt-service/src/unstract/prompt_service/plugins/table_extractor/src/extractor/core.py
lines 120-146, apply the change at the assignment before the existing loop;
prompt-service/src/unstract/prompt_service/plugins/table_extractor/src/prompts/prompts.py
lines 5-33 requires no direct change and should remain a read-only shared
structure.
- Around line 493-535: Make the continuation-page prompt construction in the
loop following the initial extraction use the same column count as the
first-page prompt: replace the plain column_count substitution with the value
including the two page/line-number columns. Keep the existing headers and
downstream parsing flow unchanged.
- Around line 180-214: Initialize the local headers variable to an empty list
before the response branch in extract_header. Preserve the existing
parsed-header assignment for non-empty, non-"na" responses, and ensure the
empty/"na" path returns the initialized empty list without raising
UnboundLocalError.
- Around line 587-628: Update process_raw_jsonl_to_tsv so each row is parsed
once, retain the parsed object through validation and output, and add x_page
before establishing or using headers so the page value is included in
cleaned_tsv. Replace both broad silent exception handlers with targeted JSON
parsing/error handling that logs malformed-row failures before continuing.

In
`@prompt-service/src/unstract/prompt_service/plugins/table_extractor/src/processor/base_processor.py`:
- Line 26: The document type fallback is resolved inconsistently between
TableExtractionBase and the runner. In
prompt-service/src/unstract/prompt_service/plugins/table_extractor/src/processor/base_processor.py#L26,
stop independently deriving the value or use runner.py’s "default" fallback; in
prompt-service/src/unstract/prompt_service/plugins/table_extractor/src/interface/runner.py#L30,
resolve document_type once in run_table_extraction and pass it to
TableExtractionBase.extract_large_table so both consumers use the same value.
- Around line 82-101: Update the empty-header handling around
large_table_extractor.extract_header in the processor flow so that when
document_type is absent or does not match bank_statement or rent_rolls, the code
explicitly logs the missing headers and raises the appropriate PluginException
instead of falling through with an empty list. Preserve the existing
default-header behavior for bank_statement and the existing failure behavior for
rent_rolls.

In
`@prompt-service/src/unstract/prompt_service/plugins/table_extractor/src/processor/post_processor.py`:
- Line 20: Change the TABLE_DEBUG default in the post-processing configuration
to false so temporary generated scripts and extracted TSV data are deleted by
the existing finally cleanup unless debugging is explicitly enabled with
TABLE_DEBUG=true.
- Around line 119-148: Update _run_processing_script so all environments execute
the untrusted script through the sandboxed Docker command with the existing
network, memory, CPU, and nonroot restrictions; do not run script_path directly
based on is_k8s or is_docker. Add a finite timeout to subprocess.run and handle
subprocess.TimeoutExpired explicitly, preserving the existing
PluginException-based failure reporting.
- Around line 149-155: Update the exception handlers in the post-processing
function to propagate the original failure after logging, including both
FileNotFoundError and unexpected Exception cases, instead of falling through
with an implicit None return. Preserve the existing timeout handling unless
needed for consistent error propagation, and ensure the caller receives the
actual underlying error rather than failing later at output.strip().

---

Minor comments:
In `@prompt-service/src/unstract/prompt_service/plugins/challenge/src/base.py`:
- Around line 101-114: Update the completion and parsing flow in the method
containing challenge_llm.complete so an LLM failure does not fall through to
JSON parsing or return default_answer. Handle the exception by propagating or
returning a failure result that run() can recognize and retry, while reserving
the “JSON format error” path for responses that were actually received but
cannot be parsed.

In `@prompt-service/src/unstract/prompt_service/plugins/evaluation/src/base.py`:
- Around line 172-176: Update the evaluation checks in the relevant base-class
flow around the response fallback and the `monitor_llm` handling to use
`self.settings.get(...)` with explicit disabled defaults instead of direct key
access. Ensure prompts with missing `evaluate` or `monitor_llm` settings raise
`EvalDisabledError` rather than allowing `KeyError` to become `EvalFailedError`,
while preserving enabled-setting behavior.

In
`@prompt-service/src/unstract/prompt_service/plugins/simple_prompt_studio/README.md`:
- Line 5: Update the SimplePromptStudio README to document the registered
endpoint as /answer-prompt-public instead of /answer-sps, matching the route
defined by the SimplePromptStudio implementation and preventing clients from
using the obsolete path.

In
`@prompt-service/src/unstract/prompt_service/plugins/simple_prompt_studio/src/base.py`:
- Around line 46-53: Validate that output retrieved in the payload-processing
method is present before accessing PSKeys.NAME or PSKeys.PROMPT. If it is
missing or invalid, raise the established BadRequest error with a clear message;
otherwise preserve the existing variable_names, prompt_name, and promptx
initialization flow.

In
`@prompt-service/src/unstract/prompt_service/plugins/smart_table_extractor/src/interface/runner.py`:
- Around line 199-204: Update the SmartTableExtractorRunner final-output
assembly so metadata does not use the loop-scoped headers from only the last
processed sheet. Aggregate detected headers for every sheet and pass that
collection to _assemble_final_output, or remove the single headers field from
the assembled metadata while preserving accurate multi-sheet output.

In
`@prompt-service/src/unstract/prompt_service/plugins/table_extractor/src/prompts/bank_statement/postprocessing.txt`:
- Around line 14-19: The example schema in the post-processing prompt contains
malformed JSON: fix the line_no_end field value in the schema example by adding
its missing closing quote, matching the valid line_no_start representation and
preserving the intended string type.

---

Nitpick comments:
In
`@prompt-service/src/unstract/prompt_service/plugins/evaluation/src/__init__.py`:
- Line 13: Remove the module-import-time warnings.simplefilter call in the
plugin initializer and scope the ResourceWarning suppression to the evaluator
execution path instead. Use warnings.catch_warnings with filterwarnings around
the relevant evaluation operation, ensuring the process-wide warning filter is
restored after execution.

In
`@prompt-service/src/unstract/prompt_service/plugins/single_pass_extraction/src/constants.py`:
- Around line 43-46: Remove the redundant second FILE_PATH constant declaration
in the constants module, preserving the first FILE_PATH = "file_path" definition
and all other constants unchanged.

In
`@prompt-service/src/unstract/prompt_service/plugins/smart_table_extractor/src/converter/excel_to_tsv.py`:
- Around line 128-146: The exception handlers in the Excel-to-TSV conversion
flow should use logger.exception instead of logger.error so active exception
tracebacks are captured. Update the handlers for
requests.exceptions.ConnectionError, requests.exceptions.RequestException, and
the broad Exception around the conversion logic, preserving their existing
messages and FileConversionException behavior.

In
`@prompt-service/src/unstract/prompt_service/plugins/smart_table_extractor/src/processor/__init__.py`:
- Line 6: Sort the __all__ export list alphabetically by placing BatchProcessor
before HeaderDetector to resolve Ruff RUF022.

In
`@prompt-service/src/unstract/prompt_service/plugins/smart_table_extractor/src/processor/batch_processor.py`:
- Around line 98-100: Update the exception handler in the batch-processing flow
to use logger.exception("Failed to process batches") instead of logger.error,
preserving the active traceback while keeping the existing
BatchProcessingException propagation unchanged.

In
`@prompt-service/src/unstract/prompt_service/plugins/table_extractor/pyproject.toml`:
- Line 6: Remove flake8 from the runtime dependencies in the pyproject.toml
dependencies declaration; keep only the packages required by the plugin at
runtime, and do not add a replacement dev group unless one already exists for
linting.

In
`@prompt-service/src/unstract/prompt_service/plugins/table_extractor/src/extractor/core.py`:
- Around line 114-118: Update the FileNotFoundError handler in the extractor’s
exception flow to re-raise PluginException without chaining the original
exception, while preserving the existing message content. Use explicit exception
suppression on the raise in the except FileNotFoundError block.
- Around line 421-442: Remove the commented-out header normalization and
header-prepending blocks near the affected sections, including the corresponding
block around the later referenced lines. Keep only active implementation code;
do not retain dead code inline, and preserve any required behavior through the
existing live logic.
- Line 54: Replace the mutable [] defaults for headers in all five affected
function signatures with a non-shared default, such as None, and initialize an
empty list inside each function when needed. Preserve existing behavior for
callers that provide headers, and update every headers signature including those
near the referenced locations.

In
`@prompt-service/src/unstract/prompt_service/plugins/table_extractor/src/processor/base_processor.py`:
- Line 82: Remove the unfinished “Extract document metadata” TODO from the
affected processor code, or implement the metadata extraction before retaining
any note; do not leave the TODO in shipped code.

In
`@prompt-service/src/unstract/prompt_service/plugins/table_extractor/src/processor/post_processor.py`:
- Line 27: Update exception handlers in run_python_program_on_response and the
handlers around the flagged lines to use logger.exception() instead of
logger.error(f"... {e}"), preserving the existing context messages while
allowing stack traces to be captured. Reduce run_python_program_on_response’s
cognitive complexity below the configured threshold by extracting cohesive logic
into small helper functions without changing behavior.
- Around line 157-165: Update _process_csv_to_json to parse the TSV only once,
removing the redundant coloumns read and usecols filtering, then preserve the
existing fillna and output structure. Replace df.values.tolist() with
df.to_numpy().tolist() when building rows.
🪄 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: f41cb416-e5bd-4b6a-ae2b-671817aedc02

📥 Commits

Reviewing files that changed from the base of the PR and between 3519b75 and 8235086.

⛔ Files ignored due to path filters (9)
  • prompt-service/src/unstract/prompt_service/plugins/challenge/uv.lock is excluded by !**/*.lock
  • prompt-service/src/unstract/prompt_service/plugins/evaluation/uv.lock is excluded by !**/*.lock
  • prompt-service/src/unstract/prompt_service/plugins/highlight_data/uv.lock is excluded by !**/*.lock
  • prompt-service/src/unstract/prompt_service/plugins/line_item_extraction/uv.lock is excluded by !**/*.lock
  • prompt-service/src/unstract/prompt_service/plugins/simple_prompt_studio/uv.lock is excluded by !**/*.lock
  • prompt-service/src/unstract/prompt_service/plugins/single_pass_extraction/uv.lock is excluded by !**/*.lock
  • prompt-service/src/unstract/prompt_service/plugins/smart_table_extractor/uv.lock is excluded by !**/*.lock
  • prompt-service/src/unstract/prompt_service/plugins/summarize/uv.lock is excluded by !**/*.lock
  • prompt-service/src/unstract/prompt_service/plugins/table_extractor/uv.lock is excluded by !**/*.lock
📒 Files selected for processing (85)
  • backend/prompt_studio/permission.py
  • backend/prompt_studio/prompt_studio_registry_v2/views.py
  • prompt-service/src/unstract/prompt_service/plugins/challenge/README.md
  • prompt-service/src/unstract/prompt_service/plugins/challenge/pyproject.toml
  • prompt-service/src/unstract/prompt_service/plugins/challenge/src/__init__.py
  • prompt-service/src/unstract/prompt_service/plugins/challenge/src/base.py
  • prompt-service/src/unstract/prompt_service/plugins/evaluation/README.md
  • prompt-service/src/unstract/prompt_service/plugins/evaluation/pyproject.toml
  • prompt-service/src/unstract/prompt_service/plugins/evaluation/src/__init__.py
  • prompt-service/src/unstract/prompt_service/plugins/evaluation/src/base.py
  • prompt-service/src/unstract/prompt_service/plugins/evaluation/src/constants.py
  • prompt-service/src/unstract/prompt_service/plugins/evaluation/src/frameworks/__init__.py
  • prompt-service/src/unstract/prompt_service/plugins/evaluation/src/frameworks/base.py
  • prompt-service/src/unstract/prompt_service/plugins/evaluation/src/frameworks/llama_index.py
  • prompt-service/src/unstract/prompt_service/plugins/evaluation/src/frameworks/ragas.py
  • prompt-service/src/unstract/prompt_service/plugins/evaluation/src/frameworks/unstract.py
  • prompt-service/src/unstract/prompt_service/plugins/highlight_data/README.md
  • prompt-service/src/unstract/prompt_service/plugins/highlight_data/pyproject.toml
  • prompt-service/src/unstract/prompt_service/plugins/highlight_data/src/__init__.py
  • prompt-service/src/unstract/prompt_service/plugins/highlight_data/src/base.py
  • prompt-service/src/unstract/prompt_service/plugins/highlight_data/src/constants.py
  • prompt-service/src/unstract/prompt_service/plugins/line_item_extraction/README.md
  • prompt-service/src/unstract/prompt_service/plugins/line_item_extraction/pyproject.toml
  • prompt-service/src/unstract/prompt_service/plugins/line_item_extraction/src/__init__.py
  • prompt-service/src/unstract/prompt_service/plugins/line_item_extraction/src/base.py
  • prompt-service/src/unstract/prompt_service/plugins/simple_prompt_studio/README.md
  • prompt-service/src/unstract/prompt_service/plugins/simple_prompt_studio/pyproject.toml
  • prompt-service/src/unstract/prompt_service/plugins/simple_prompt_studio/src/__init__.py
  • prompt-service/src/unstract/prompt_service/plugins/simple_prompt_studio/src/base.py
  • prompt-service/src/unstract/prompt_service/plugins/simple_prompt_studio/src/helper.py
  • prompt-service/src/unstract/prompt_service/plugins/single_pass_extraction/README.md
  • prompt-service/src/unstract/prompt_service/plugins/single_pass_extraction/pyproject.toml
  • prompt-service/src/unstract/prompt_service/plugins/single_pass_extraction/src/__init__.py
  • prompt-service/src/unstract/prompt_service/plugins/single_pass_extraction/src/base.py
  • prompt-service/src/unstract/prompt_service/plugins/single_pass_extraction/src/constants.py
  • prompt-service/src/unstract/prompt_service/plugins/single_pass_extraction/src/exceptions.py
  • prompt-service/src/unstract/prompt_service/plugins/single_pass_extraction/src/helper.py
  • prompt-service/src/unstract/prompt_service/plugins/smart_table_extractor/README.md
  • prompt-service/src/unstract/prompt_service/plugins/smart_table_extractor/pyproject.toml
  • prompt-service/src/unstract/prompt_service/plugins/smart_table_extractor/src/__init__.py
  • prompt-service/src/unstract/prompt_service/plugins/smart_table_extractor/src/constants.py
  • prompt-service/src/unstract/prompt_service/plugins/smart_table_extractor/src/converter/__init__.py
  • prompt-service/src/unstract/prompt_service/plugins/smart_table_extractor/src/converter/excel_to_tsv.py
  • prompt-service/src/unstract/prompt_service/plugins/smart_table_extractor/src/exceptions.py
  • prompt-service/src/unstract/prompt_service/plugins/smart_table_extractor/src/interface/__init__.py
  • prompt-service/src/unstract/prompt_service/plugins/smart_table_extractor/src/interface/runner.py
  • prompt-service/src/unstract/prompt_service/plugins/smart_table_extractor/src/processor/__init__.py
  • prompt-service/src/unstract/prompt_service/plugins/smart_table_extractor/src/processor/batch_processor.py
  • prompt-service/src/unstract/prompt_service/plugins/smart_table_extractor/src/processor/header_detector.py
  • prompt-service/src/unstract/prompt_service/plugins/smart_table_extractor/src/prompts/__init__.py
  • prompt-service/src/unstract/prompt_service/plugins/smart_table_extractor/src/prompts/prompts.py
  • prompt-service/src/unstract/prompt_service/plugins/smart_table_extractor/test_runner.py
  • prompt-service/src/unstract/prompt_service/plugins/summarize/README.md
  • prompt-service/src/unstract/prompt_service/plugins/summarize/pyproject.toml
  • prompt-service/src/unstract/prompt_service/plugins/summarize/src/__init__.py
  • prompt-service/src/unstract/prompt_service/plugins/summarize/src/base.py
  • prompt-service/src/unstract/prompt_service/plugins/summarize/src/constants.py
  • prompt-service/src/unstract/prompt_service/plugins/summarize/src/helper.py
  • prompt-service/src/unstract/prompt_service/plugins/table_extractor/README.md
  • prompt-service/src/unstract/prompt_service/plugins/table_extractor/pyproject.toml
  • prompt-service/src/unstract/prompt_service/plugins/table_extractor/src/__init__.py
  • prompt-service/src/unstract/prompt_service/plugins/table_extractor/src/constants.py
  • prompt-service/src/unstract/prompt_service/plugins/table_extractor/src/exceptions.py
  • prompt-service/src/unstract/prompt_service/plugins/table_extractor/src/extractor/__init__.py
  • prompt-service/src/unstract/prompt_service/plugins/table_extractor/src/extractor/core.py
  • prompt-service/src/unstract/prompt_service/plugins/table_extractor/src/interface/__init__.py
  • prompt-service/src/unstract/prompt_service/plugins/table_extractor/src/interface/helper.py
  • prompt-service/src/unstract/prompt_service/plugins/table_extractor/src/interface/runner.py
  • prompt-service/src/unstract/prompt_service/plugins/table_extractor/src/processor/__init__.py
  • prompt-service/src/unstract/prompt_service/plugins/table_extractor/src/processor/base_processor.py
  • prompt-service/src/unstract/prompt_service/plugins/table_extractor/src/processor/post_processor.py
  • prompt-service/src/unstract/prompt_service/plugins/table_extractor/src/prompts/__init__.py
  • prompt-service/src/unstract/prompt_service/plugins/table_extractor/src/prompts/bank_statement/extraction.txt
  • prompt-service/src/unstract/prompt_service/plugins/table_extractor/src/prompts/bank_statement/headers.txt
  • prompt-service/src/unstract/prompt_service/plugins/table_extractor/src/prompts/bank_statement/postprocessing.txt
  • prompt-service/src/unstract/prompt_service/plugins/table_extractor/src/prompts/bank_statement/table_detect.txt
  • prompt-service/src/unstract/prompt_service/plugins/table_extractor/src/prompts/bank_statement/table_span_contiguous.txt
  • prompt-service/src/unstract/prompt_service/plugins/table_extractor/src/prompts/bank_statement/table_span_headers.txt
  • prompt-service/src/unstract/prompt_service/plugins/table_extractor/src/prompts/prompts.py
  • prompt-service/src/unstract/prompt_service/plugins/table_extractor/src/prompts/rent_rolls/extraction.txt
  • prompt-service/src/unstract/prompt_service/plugins/table_extractor/src/prompts/rent_rolls/headers.txt
  • prompt-service/src/unstract/prompt_service/plugins/table_extractor/src/prompts/rent_rolls/postprocessing.txt
  • prompt-service/src/unstract/prompt_service/plugins/table_extractor/src/prompts/rent_rolls/table_detect.txt
  • prompt-service/src/unstract/prompt_service/plugins/table_extractor/src/prompts/rent_rolls/table_span_contiguous.txt
  • prompt-service/src/unstract/prompt_service/plugins/table_extractor/src/prompts/rent_rolls/table_span_headers.txt
🚧 Files skipped from review as they are similar to previous changes (1)
  • backend/prompt_studio/prompt_studio_registry_v2/views.py

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🤖 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_registry_v2/tests/test_registry_tool_delete_guards.py`:
- Around line 179-214: Replace the predicate-only checks in
TestRegistryToolInUseRefusal with request-level tests that dispatch the
production DELETE route through PromptStudioRegistryView.destroy. Cover owner
deletion success, non-owner deletion denial, and deletion of a workflow-attached
tool returning HTTP 409, asserting response status and relevant side effects so
query, exception, route binding, and destroy permission wiring are exercised.
🪄 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: c35fe762-0e75-4b1c-846e-37a16182b64e

📥 Commits

Reviewing files that changed from the base of the PR and between f41ee0e and 9c9d549.

📒 Files selected for processing (1)
  • backend/prompt_studio/prompt_studio_registry_v2/tests/test_registry_tool_delete_guards.py

hari-kuriakose and others added 3 commits July 25, 2026 17:29
…m the URL

Two API ergonomics fixes.

1. No way to delete an exported registry tool

The registry was read-only over the API - `prompt_studio_registry_v2/urls.py`
mapped only `{"get": "list"}`. The sole way to remove an entry was to delete the
Prompt Studio project, which cascades to it. That works, but it is implicit and
undocumented, and it is a blunt instrument: there was no way to unpublish a tool
while keeping the project.

Adds `DELETE registry/<pk>/`, guarded by the same in-use check
`prompt-studio delete` performs - a tool still attached to a workflow is refused
with 409 rather than silently breaking those workflows. The guard filters
`ToolInstance` on `tool_id=instance.pk`, matching the existing check in
`prompt_studio_core_v2/views.py`, where an exported tool's `tool_id` is its
`prompt_registry_id`.

`get_queryset` previously returned `None` when no query-param filters were
present, which would break `get_object()` on a detail route. It now returns the
full queryset when addressing a single row by PK. Keyed off the URL kwarg rather
than `self.detail`, which DRF only populates for router-generated views and
leaves as `None` under a manual `as_view()` - the wiring used here.

2. API-key creation wanted an identifier already present in the URL

`POST keys/api/<api_id>/` took `api_id` as a path segment but also expected
`api` in the body - the same value spelled twice. Omitting the body field failed
validation. `POST keys/pipeline/<pipeline_id>/` had the identical shape.

POST routed to the default `ModelViewSet.create`, which never sees the URL
kwargs, so the body had to repeat them. `create` now derives the target from the
path when present. It uses `setdefault`, so an explicit body value still wins,
and falls through to the default implementation for the body-only routes
(`keys/api/`, `keys/pipeline/`) - both remain working.

Note: making a no-RAG profile (`chunk_size=0`) omit the vector DB and embedding
model is deliberately NOT included here; it is not an ergonomics-sized change.
See the PR description.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The new `DELETE registry/<pk>/` route resolved its object from
`PromptStudioRegistry.objects.all()`, and the viewset carried no permission
classes (`DEFAULT_PERMISSION_CLASSES` is empty). `OrganizationFilterBackend`
runs inside `get_object()` via `filter_queryset`, so cross-org deletion was
already impossible - but any member of the same organization could delete any
other member's exported tool by PK.

Adds `IsRegistryToolOwner`, gating only the `destroy` action. Ownership is
inherited from the linked `CustomTool`, mirroring `IsParentToolOwner` (which
does the same for `ProfileManager`), with a fallback to the row's own owner for
unlinked legacy rows since `custom_tool` is nullable. Service accounts and org
admins are admitted, matching the sibling permission classes.

Read access is deliberately left broader - `list` visibility is still derived by
`PromptStudioRegistry.objects.list_tools`, unchanged. Only the destructive route
is restricted.

Lives in `prompt_studio/permission.py` next to `PromptAcesssToUser` rather than
in the view module, matching where the app's other permission classes live.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Regression tests for the two gates on `DELETE registry/<pk>/`.

The authorization test is the important one: it fails if `IsRegistryToolOwner`
is loosened. The viewset carries no `permission_classes` and
`DEFAULT_PERMISSION_CLASSES` is empty, so without that gate any member of an
organization could delete another member's exported tool by PK.
`OrganizationFilterBackend` blocks cross-org access inside `get_object()`, but
not intra-org - which is exactly the case asserted here.

Exercises the real `has_object_permission` body against stubbed collaborators,
since Django is not importable in a plain checkout. Mirrors
`prompt_studio_core_v2/tests/test_build_index_payload.py`.

Coverage:
  - the project owner may delete
  - another org member may NOT delete (the IDOR this guard closes)
  - org admins and service accounts may delete
  - ownership follows the parent `custom_tool`, not the registry row, so a
    stale export-time owner cannot outrank the project's current owner
  - unlinked legacy rows (`custom_tool` is nullable) fall back to their own
    owner rather than becoming undeletable or world-deletable
  - an in-use tool is refused and an unused one is not
  - `RegistryToolInUseError` is a 409, not a 500 like the neighbouring
    `ToolDeleteError`, since the condition is caller-correctable

Verified by mutation: making the gate unconditionally permissive fails the
non-owner, parent-ownership, and legacy-row assertions.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

@chandrasekharan-zipstack chandrasekharan-zipstack left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The registry-delete half holds up on review — I checked the org-isolation argument specifically and it is correct. Not django-tenants (settings/base.py:336 has it commented out, TENANT_APPS = []); it's shared-schema with an organization FK. The viewset inherits OrganizationFilterBackend from DEFAULT_FILTER_BACKENDS (base.py:625-629), DRF's get_object() does call filter_queryset, and that backend fails closed to queryset.none() with no org context. Belt-and-braces, PromptStudioRegistry.objects is itself org-scoped via DefaultOrganizationManagerMixin. So .objects.all() on the detail route does not leak, and the self.kwargs["pk"] vs self.detail reasoning is right.

IsRegistryToolOwner also genuinely mirrors IsParentToolOwnerCustomTool has memberships (prompt_studio_core_v2/models.py:195, backfilled by migration 0009), and the obj.custom_tool or obj fallback resolves to created_by for unlinked rows.

The API-key half has an authorization gap — inline.

Comment thread backend/api_v2/api_key_views.py Outdated
Comment thread backend/prompt_studio/prompt_studio_registry_v2/views.py Outdated
Comment thread backend/api_v2/api_key_views.py Outdated
Comment thread backend/prompt_studio/prompt_studio_registry_v2/views.py Outdated
hari-kuriakose and others added 6 commits July 31, 2026 16:05
Addresses the review on #2206.

`create` performed no ownership check on the target deployment. DRF resolves
`IsParentDeploymentOwner` for it, but `create` is collection-level -- DRF never
calls `get_object()`, so `has_object_permission` never ran and any authenticated
org member could mint a live key for a deployment they do not own. The view now
object-checks the path target itself. (Pre-existing; the body-based path had the
same gap. Closed here because this is the method where the fix belongs.)

`IsParentDeploymentOwner` had to change to accept the parent directly: neither
`APIDeployment` nor `Pipeline` declares an `api` or `pipeline` field (`APIKey.api`
points *at* the deployment, `related_name="api_keys"`), so the bare `obj.api` in
the reviewer's suggested snippet raises `AttributeError` -> 500 on every key
creation. The lookups are now `getattr` guarded; a test pins the regression.

The path target is also made authoritative rather than a `setdefault`: a body
naming the *other* target is a contradiction and is refused with a 400 instead of
producing a key for whichever one wins. That also disposes of two edges flagged in
review -- a JSON array body now 400s rather than `AttributeError`-ing into a 500,
and `{"api": ""}` no longer defeats the derivation into a confusing 400.

The registry 409 now names the blocking deployment types, mirroring
`prompt_studio_core_v2/views.py:249`, so the refusal is actionable rather than
just telling the caller "no".

Tests: the in-use guard was asserted against a restated `bool(ids)` predicate,
which passed regardless of what the view did. It now drives the real method
bodies extracted from `views.py`; verified by mutation (neutering the raise,
dropping a deployment-type branch, or breaking the workflow query all fail the
suite). Django settings are unavailable in the unit tier, so the source-extraction
technique already used in this package is retained; route binding and the live
permission cycle need a database and remain integration-tier.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_014PWpGFA4Z5qktUj5DW2oiK
Standardized review of bfde081 found the previous commit fixed half the
hole it claimed to close, plus three issues in the fix itself.

**Critical — the body-only route was still open.** `urls.py:102,104` bind
`POST keys/api/` and `keys/pipeline/` to the same `create`, which returned
`super().create()` for them with no ownership check. Any org member could
still mint a live key for a deployment they do not own simply by moving the
identifier from the path into the body. `create` now resolves the target from
path *or* body and object-checks it on every route.

**The 422 narrowing is gone, and with it an information leak.** The pipeline
branch used `get_active_pipeline`, which raises `InactivePipelineError` (422)
and logs at ERROR for any pipeline with `active=False` (the model default).
That fired *before* `check_object_permissions`, so a non-owner learned the
pipeline existed and was inactive -- the state the check exists to protect.
Added `PipelineProcessor.get_pipeline_by_id` for callers that need to identify
a row rather than run it, restoring the prior status contract.

**The authorization check no longer fails open.** `getattr(...) or ... or obj`
admitted any object exposing a matching owner, turning a wrong-type
programming error into a silent grant. The accepted shapes are now explicit --
an APIKey by its two parent FKs, a parent by `memberships` -- and anything
else is denied and logged. It still cannot be written as `obj.api or ...`:
the parents declare no `api` attribute, so that is a 500 on every create.

**Tests: two suites were passing against broken code.** Verified by mutation:
- Removing `destroy`'s call to the guard left 12/12 green while in-use tools
  became deletable. `destroy` is now extracted and driven end-to-end.
- `TestCreateContract` asserted on *source text*, so it passed against the
  wrong object being handed to `check_object_permissions` and against an
  inverted `isinstance` guard. Replaced with tests that execute `create`.
All four previously-surviving mutations now fail, including one that
re-opens the Critical.

Also extracts the deployment-type probe and message grammar into
`prompt_studio/tool_usage.py`; it was duplicated verbatim between the registry
and core delete paths, so a fourth deployment type would have left one caller
silently reporting a stale set. Restores the blocking workflow IDs to the
refusal log (bounded), and logs the ambiguous case where dependants exist but
no deployment type resolves -- the org-scope asymmetry between the unscoped
`ToolInstance` manager and the org-scoped deployment managers.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_014PWpGFA4Z5qktUj5DW2oiK
Second review round on 4666d4b. Prior findings all verified resolved; this
closes the one regression that fix introduced.

Widening `create` to resolve the target from the request body meant the body
value reached `objects.get(pk=...)` before any serializer ran. `pk` is a UUID
column, so a non-UUID string raises `django.core.exceptions.ValidationError`
out of `to_python` -- not `DoesNotExist`. Neither `get_api_by_id` nor
`get_pipeline_by_id` caught it, so `POST keys/api/` with `{"api": "not-a-uuid"}`
returned **500** with a logged traceback. At base this was a clean 400 from the
serializer's `PrimaryKeyRelatedField`, so the widening caused it.

Fixed at the fetch boundary rather than in `create`: the path routes are
`<str:api_id>` / `<str:pipeline_id>`, so path input is unvalidated too and one
change covers both forms. A malformed identifier is now "not found", which is
what it means.

The test stubs modelled `dict.get`, making malformed and missing input
indistinguishable -- which is why the suite was blind to this. They now model
the real contract, and the `except` clause is asserted on the code rather than
the whole function body (the docstring names `ValidationError`, so a
body-level assertion passed against the reverted code).

Also folds the two consecutive `logger.warning` calls in the registry refusal
into one -- they fired back-to-back for a single event, doubling volume on the
noisiest path -- and uses `heapq.nsmallest` so a pathological fan-out does not
sort the whole set just to slice twenty off it.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_014PWpGFA4Z5qktUj5DW2oiK
Third review round flagged the helper tests as source-text assertions: they
matched `"ValidationError" in <except line>`, which pins the token rather than
the behaviour.

They now execute the extracted function bodies against a manager that raises
the real `django.core.exceptions.ValidationError`. Django is installed in the
unit tier even though settings are unconfigured, so the actual exception class
is importable and the `except` clause runs for real.

Adds the case the text assertion could not express at all: broadening the
catch to a bare `except Exception` -- which would turn a database outage into
a silent 404 -- now fails the suite. Verified by mutation: reverting either
catch, or over-broadening it, each fails.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_014PWpGFA4Z5qktUj5DW2oiK
Behaviour-preserving cleanup, kept on its own commit so it reads apart from
the security fixes. No reviewed source file changes: `api_key_views.py`,
`utils.py`, `permission.py`, `pipeline_processor.py`, `tool_usage.py` and
`prompt_studio_registry_v2/views.py` are byte-identical to 3b06040.

The two test modules had grown four near-identical copies of the same
slice-and-`pytest.fail` extraction loop. Those move to
`backend/tests_common/source_extraction.py`, alongside an `exec_def` for the
extract-dedent-exec sequence both lookup-helper builders were open-coding.

Placed in a `tests_common` package rather than at the backend root: the module
imports `pytest`, so it does not belong beside `manage.py`. The repo's existing
precedent (`permissions/tests/base.py`) is app-scoped, which does not fit
helpers consumed from two different apps. Verified the import resolves under
both `cd backend && pytest ...` and repo-root `pytest backend/...`.

Also drops a dead sentinel and an unused marker constant from the api-key
tests, and trims three docstrings in `prompt_studio_core_v2/views.py` that
restated their signatures -- one now fronts a single-line pass-through.

Re-ran the security mutations after the refactor; all four still fail the
suite (body-only IDOR, fail-open authz, unwired destroy guard, broadened
catch). 41 pass.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_014PWpGFA4Z5qktUj5DW2oiK
`exec_def` handed `compile` the real module path while the dedented snippet
started at line 1, so any frame from an extracted body paired a genuine
filename with a snippet-relative line. A fault in `get_pipeline_by_id` cited
`pipeline_processor.py:18` -- inside an unrelated function's docstring -- for a
statement that lives at :72. Anyone debugging a future guard-test failure was
pointed at the wrong code with no hint the citation was bogus.

Padding the snippet with blank lines to its real offset fixes it: the same
fault now cites :72 with the correct source line.

Correcting the previous commit message while I am here: it said "no reviewed
source file changes", listing six files. That was accurate as far as it went,
but `prompt_studio_core_v2/views.py` is also production source and *is* in that
diff -- docstrings only, no executable statement touched. The narrower claim is
what I should have written.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_014PWpGFA4Z5qktUj5DW2oiK
Comment thread backend/prompt_studio/prompt_studio_registry_v2/views.py
hari-kuriakose and others added 5 commits August 3, 2026 15:49
F1 (Critical): the guard suites drive method bodies extracted from source,
so they assert what a function does but not that anything reaches it. Four
mutations reopened real holes with all 41 tests green: deleting the registry
DELETE route, unwiring IsRegistryToolOwner from destroy, moving create() into
a dead class (reopening the IDOR), and dropping the `status` import (NameError
on every key creation).

Adds tests_common/test_route_wiring.py, which asserts binding rather than
behaviour -- verb->action maps, that create() is overridden on the viewset
rather than inherited from ModelViewSet, that it calls check_object_permissions,
and that every global create() loads resolves in its module. Each of the four
mutations now fails it; verified by executing them.

F2 (High): get_active_pipeline caught only DoesNotExist while its new sibling
get_pipeline_by_id caught ValidationError too. A non-UUID pk raises
ValidationError out of to_python, and this path is reached *unauthenticated* --
the public execution endpoint looks the pipeline up before validating the API
key -- so any non-UUID path segment forced a 500. One-line catch widened, with
tests covering both lookups written the conventional way (real import + patch).

Both test files run in the unit tier with no database.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01DJWqb9Nq6aFbUFn62MMZc2
…s (F3-F8)

F4 (Medium): `POST keys/api/<A>/` with body `{"api": "B"}` hit neither
contradiction guard -- they only cross-check the *other* field -- so the body
value was silently overwritten and a live key minted for A while the caller
named B. Refused symmetrically now. An empty body value is still not a
disagreement, so `{"api": ""}` keeps working.

F3 (High, partial): two docstring claims were false. The cited precedent
(test_build_index_payload.py) imports its module and patches collaborators --
the opposite of source extraction -- and says so in its own docstring; and
Django *is* importable in the unit tier, since tests/groups.yaml sets
DJANGO_SETTINGS_MODULE for unit-backend and conftest.py auto-marks only
django_db/TestCase tests as integration. Corrected, and the technique's blind
spots are now stated where they are relied on: unreachable code looks wired, a
missing import is invisible, an inserted decorator silently truncates a slice,
and a cosmetic annotation change breaks the marker. Retiring the technique
outright rewrites both suites and is left to its own change.

F6 (Medium): get_queryset returned None when no filter args were supplied,
which DRF hands straight to filter_queryset and the paginator -- an unfiltered
`list` 500ed instead of returning nothing. Returns none() and the signature
drops `| None`.

F7 (Medium): an unresolved deployment type can mean the dependants sit in
another organization (ToolInstance is not org-scoped; the deployment tables
are), so the 409 told users to remove usages they cannot see. The fallback
wording now says so.

F8 (Medium): the ValidationError arms of both lookups now log at debug, so a
malformed identifier stays distinguishable from an absent row when triaging --
both still answer 404.

Backend unit tier: 337 passed (318 before), ruff 0.3.4 clean.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01DJWqb9Nq6aFbUFn62MMZc2
From the confirming review of the two fix commits:

- `test_every_global_create_uses_resolves` walked the whole module for any
  FunctionDef named `create`, so a second definition anywhere in the file would
  break it with an unpack error naming nothing about the defect, and an
  `async def` would match nothing. Parses the override itself instead --
  the same expression the neighbouring test already uses. Re-verified that
  dropping the `status` import still fails it.

- `test_list_is_not_gated_by_the_owner_permission` asserted only the absence of
  the owner class, which an empty permission list would satisfy while proving
  nothing. Also pins that resolution fell through to the default.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01DJWqb9Nq6aFbUFn62MMZc2
Three open Greptile P1 threads claim the detail route's `objects.all()`
queryset lets a caller -- or an org admin -- delete another organization's
exported tool by PK. Verified against the code: it does not.

`get_object()` calls `filter_queryset()` *before* `check_object_permissions`,
and `OrganizationFilterBackend` is in DEFAULT_FILTER_BACKENDS. `get_org_path`
resolves `PromptStudioRegistry` to a direct `organization` FK (confirmed by
running it), so the queryset is narrowed to the caller's org and a foreign-org
PK is a 404 from `get_object_or_404` -- before `IsRegistryToolOwner` runs, so
its org-admin branch is never reached for another org's row. The filter is
also fail-closed on both of its own miss paths (no org context, no discoverable
path both return `none()`).

None of that is visible at the call site, which is presumably why three
separate P1s landed on it, so pin it: the FK path, the filter-before-check
ordering, and that the viewset does not opt out via `skip_org_filter` or by
overriding `filter_backends`. Adding `skip_org_filter = True` -- which would
make the reported scenario real -- fails the third test.

No production code changed; this run found no defect to fix.

Backend unit tier: 340 passed (337 before), ruff 0.3.4 clean.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01DJWqb9Nq6aFbUFn62MMZc2
…dule docstring

`check-docstring-first` fails the file:

    test_registry_tool_delete_guards.py:259: Multiple module docstrings
    (first docstring on line 1).

The bare string under `DELETED = object()` is a PEP 257 attribute docstring,
but the hook parses any module-level string literal after the first as a
second module docstring. Converted to a comment, which keeps the explanation
where it is and satisfies the hook.

Pre-existing rather than newly introduced -- the sentinel has carried its
docstring since 4666d4b, so pre-commit.ci has been red on this since then.
Ruff does not implement this check, which is why local `ruff check` stayed
clean across every prior run.

Verified by running the full hook set against the PR's changed files: all
hooks pass, including `check docstring is first`. Backend unit tier unchanged
at 340 passed.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01DJWqb9Nq6aFbUFn62MMZc2
@sonarqubecloud

sonarqubecloud Bot commented Aug 4, 2026

Copy link
Copy Markdown

@github-actions

github-actions Bot commented Aug 4, 2026

Copy link
Copy Markdown
Contributor

Unstract test results

Per-group results

Status Group Tier Passed Failed Errors Skipped Duration (s)
e2e-api-deployment e2e 3 0 0 0 20.8
e2e-coowners e2e 1 0 0 0 1.4
e2e-etl e2e 1 0 0 0 8.3
e2e-login e2e 2 0 0 0 1.3
e2e-prompt-studio e2e 1 0 0 0 4.4
e2e-smoke e2e 2 0 0 0 1.1
e2e-workflow e2e 1 0 0 0 18.1
integration-backend integration 205 0 0 26 40.9
integration-connectors integration 1 0 0 7 7.7
integration-workers integration 140 0 0 1 47.3
unit-backend unit 340 0 0 1 38.4
unit-connectors unit 63 0 0 0 10.4
unit-core unit 33 0 0 0 1.4
unit-platform-service unit 15 0 0 0 2.7
unit-rig unit 109 0 0 0 5.4
unit-sdk1 unit 480 0 0 0 24.3
unit-workers unit 1312 0 0 0 101.1
TOTAL 2709 0 0 35 335.1

Critical paths

⚠️ Critical paths not yet covered

  • workflow-execution-fan-out — Multi-file workflow execution fans out to file-processing workers and rejoins. (declared coverage: no groups declared)
✅ Covered critical paths
  • auth-login — covered by e2e-login
  • adapter-register-llm — covered by integration-backend
  • workflow-author — covered by integration-backend
  • co-owner-manage — covered by integration-backend, e2e-coowners
  • workflow-create-execute — covered by e2e-workflow
  • api-deployment-provision — covered by integration-backend
  • api-deployment-auth — covered by integration-backend
  • api-deployment-run — covered by e2e-api-deployment
  • prompt-studio-author — covered by integration-backend
  • prompt-studio-fetch-response — covered by e2e-prompt-studio
  • connector-register-test — covered by integration-backend
  • pipeline-etl-execute — covered by e2e-etl
  • usage-aggregate-read — covered by integration-backend
  • usage-token-tracking — covered by e2e-api-deployment
  • callback-result-delivery — covered by e2e-api-deployment

@Zipstack Zipstack deleted a comment from coderabbitai Bot Aug 4, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants