[FEAT] Allow unpublishing an exported tool; derive API-key target from the URL#2206
Open
hari-kuriakose wants to merge 2 commits into
Open
[FEAT] Allow unpublishing an exported tool; derive API-key target from the URL#2206hari-kuriakose wants to merge 2 commits into
hari-kuriakose wants to merge 2 commits into
Conversation
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
Summary by CodeRabbit
WalkthroughAdds 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. ChangesAPI key creation
Registry deletion guards
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
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
…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: SERVER.md also listed making a no-RAG profile (`chunk_size=0`) omit the
vector DB and embedding model. That 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>
hari-kuriakose
force-pushed
the
feat/prompt-studio-ergonomics
branch
from
July 24, 2026 10:13
a26122b to
3519b75
Compare
Contributor
|
| Filename | Overview |
|---|---|
| backend/prompt_studio/prompt_studio_registry_v2/views.py | Adds the detail deletion flow, organization-filtered lookup, ownership permission, and workflow dependency guard. |
| backend/prompt_studio/permission.py | Adds object-level authorization for registry deletion through linked-tool ownership, legacy-row ownership, service accounts, and organization administrators. |
| backend/api_v2/api_key_views.py | Derives API or pipeline targets from detail-route parameters during API-key creation while retaining existing body-only behavior. |
| backend/prompt_studio/prompt_studio_registry_v2/urls.py | Exposes the UUID-addressed registry deletion endpoint. |
| backend/prompt_studio/prompt_studio_registry_v2/exceptions.py | Defines the conflict response returned when an exported tool remains attached to workflows. |
Reviews (3): Last reviewed commit: "[FIX] Restrict registry tool deletion to..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
backend/prompt_studio/prompt_studio_registry_v2/views.py (1)
50-52: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winCorrect 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
📒 Files selected for processing (4)
backend/api_v2/api_key_views.pybackend/prompt_studio/prompt_studio_registry_v2/exceptions.pybackend/prompt_studio/prompt_studio_registry_v2/urls.pybackend/prompt_studio/prompt_studio_registry_v2/views.py
Comment on lines
+40
to
+43
| if api_id: | ||
| request_data.setdefault("api", api_id) | ||
| if pipeline_id: | ||
| request_data.setdefault("pipeline", pipeline_id) |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Do not inject the URL target when the body specifies the other target.
For example, a pipeline body sent to POST keys/api/<api_id>/ keeps pipeline but also receives api, so APIKeySerializer.validate rejects the request because both targets are set. Since body values take precedence, derive a path target only when neither api nor pipeline is provided.
Proposed fix
- if api_id:
- request_data.setdefault("api", api_id)
- if pipeline_id:
- request_data.setdefault("pipeline", pipeline_id)
+ if not request_data.get("api") and not request_data.get("pipeline"):
+ if api_id:
+ request_data["api"] = api_id
+ elif pipeline_id:
+ request_data["pipeline"] = pipeline_id📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| if api_id: | |
| request_data.setdefault("api", api_id) | |
| if pipeline_id: | |
| request_data.setdefault("pipeline", pipeline_id) | |
| if not request_data.get("api") and not request_data.get("pipeline"): | |
| if api_id: | |
| request_data["api"] = api_id | |
| elif pipeline_id: | |
| request_data["pipeline"] = pipeline_id |
🤖 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/api_v2/api_key_views.py` around lines 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.
Comment on lines
+60
to
+72
| instance: PromptStudioRegistry = self.get_object() | ||
| dependent_wfs = set( | ||
| ToolInstance.objects.filter(tool_id=instance.pk) | ||
| .values_list("workflow_id", flat=True) | ||
| .distinct() | ||
| ) | ||
| if dependent_wfs: | ||
| logger.info( | ||
| f"Cannot delete exported tool {instance.prompt_registry_id}, " | ||
| f"depended by workflows {dependent_wfs}" | ||
| ) | ||
| raise RegistryToolInUseError() | ||
| return super().destroy(request, *args, **kwargs) |
Contributor
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -eu
rg -n -A8 -B6 \
'class ToolInstance|workflow_id|tool_id|ToolInstance\.objects' \
backendRepository: Zipstack/unstract
Length of output: 50374
🏁 Script executed:
#!/bin/bash
set -eu
# Locate and inspect the relevant model and registry view, then find the registry in-use error handling.
printf '--- files ---\n'
fd -a 'models\.py|views\.py|exceptions.*py|errors.*py|serializers.*py' backend/prompt_studio | sed 's#^\./##'
printf '\n--- registry view ---\n'
sed -n '1,130p' backend/prompt_studio/prompt_studio_registry_v2/views.py
printf '\n--- registry models ---\n'
sed -n '1,240p' backend/prompt_studio/prompt_studio_registry_v2/models.py
printf '\n--- exceptions containing RegistryToolInUseError ---\n'
rg -n -A5 -B5 'RegistryToolInUseError|registry.*in-use|InUseError' backend -g '*error*' -g '*exception*' -g '*.py' | head -200
printf '\n--- workflow attachment model references ---\n'
rg -n -A10 -B10 'class ToolInstance|workflow_id|tool_id = models\.ForeignKey|ForeignKey\(.*workflow|tool_studio_tool|ToolInstance' backend/prompt_studio -g '*.py' | head -240Repository: Zipstack/unstract
Length of output: 246
🏁 Script executed:
#!/bin/bash
set -eu
# Locate and inspect the relevant model and registry view, then find the registry in-use error handling.
printf '%s\n' '--- files ---'
fd -a 'models\.py|views\.py|exceptions.*py|errors.*py|serializers.*py' backend/prompt_studio | sed 's#^\./##'
printf '%s\n' ''
printf '%s\n' '--- registry view ---'
sed -n '1,130p' backend/prompt_studio/prompt_studio_registry_v2/views.py
printf '%s\n' ''
printf '%s\n' '--- registry models ---'
sed -n '1,240p' backend/prompt_studio/prompt_studio_registry_v2/models.py
printf '%s\n' ''
printf '%s\n' '--- exceptions containing RegistryToolInUseError ---'
rg -n -A5 -B5 'RegistryToolInUseError|registry.*in-use|InUseError' backend -g '*error*' -g '*exception*' -g '*.py' | head -200
printf '%s\n' ''
printf '%s\n' '--- workflow attachment model references ---'
rg -n -A10 -B10 'class ToolInstance|workflow_id|tool_id = models\.ForeignKey|ForeignKey\(.*workflow|tool_studio_tool|ToolInstance' backend/prompt_studio -g '*.py' | head -240Repository: Zipstack/unstract
Length of output: 41327
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- locate ToolInstance model ---'
rg -n -A12 -B8 '^class ToolInstance|workflow_id|tool_id = models\.ForeignKey|ForeignKey\(.*tool_id|on_delete=|protected foreign' backend -g '*.py' -g '!*migrations*' | head -300
printf '%s\n' ''
printf '%s\n' '--- find tool instance migration for tool_id/workflow_id FK ---'
rg -n -A8 -B8 'tool_id|workflow_id|ToolInstance|tool_instance_v2|foreign_key' backend/tool_instance_v2/backend -g '*.py' | head -260 || true
printf '%s\n' ''
printf '%s\n' '--- read likely ToolInstance model file ---'
tool_model=$(rg -l 'class ToolInstance' backend/prompt_studio backend -g '*.py' | head -1 || true)
if [ -n "${tool_model:-}" ]; then
sed -n '1,180p' "$tool_model"
fiRepository: Zipstack/unstract
Length of output: 25541
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- registry exception handler ---'
sed -n '1,120p' backend/utils -g '!*' 2>/dev/null || true
rg -n -A12 -B12 'RegistryToolInUseError|ProtectedError|delete|destroy|perform_destroy' backend/utils backend 2>/dev/null | head -300
printf '%s\n' ''
printf '%s\n' '--- exact tool_instance model fields / migrations ---'
rg -n -A4 -B4 'tool_id = models\.CharField|workflow = models\.ForeignKey|workflow_id|foreign_key|tool_id' backend/tool_instance_v2 -g '*.py' | head -300
printf '%s\n' ''
printf '%s\n' '--- imports of ToolInstance for creation/update ---'
rg -n -A8 -B5 'ToolInstance\.(objects|create|update|save|bulk_create)|ToolInstanceSerializer|ToolInstanceHelper' backend -g '*.py' -g '!**/migrations/**' | head -320Repository: Zipstack/unstract
Length of output: 50373
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- inspect exception handling for in-use / protected errors ---'
rg -n -A12 -B12 'ProtectedError|RegistryToolInUseError|Delete.*InUse|InUse' backend -g '*.py' | head -400
printf '%s\n' ''
printf '%s\n' '--- inspect exception settings middleware order ---'
rg -n -A20 -B5 'EXCEPTION Handler|exception_settings|exception_handler|Promiser|DRF ExceptionHandlers' backend -g '*.py' | head -400
printf '%s\n' ''
printf '%s\n' '--- inspect ToolInstance.create validation around tool uniqueness ---'
sed -n '140,190p' backend/tool_instance_v2/serializers.py
printf '%s\n' ''
printf '%s\n' '--- inspect Workflow Model relationship constraints if present ---'
wg -n -A12 -B8 'class Workflow|tool_instances|on_delete=models.CASCADE\n.*tool_instance|unique_together|Meta' backend/workflow_manager -g '*.py' | head -260Repository: Zipstack/unstract
Length of output: 31247
Make the dependency check and deletion atomic.
tool_id in ToolInstance is a CharField, not a protected FK, and destroy() performs a TOCTOU check before super().destroy() re-fetches/locks the registry object again. A concurrent attachment can bypass the check and either delete the registry row or leave a dangling tool_id string; enforce the invariant at the transaction/database boundary and add a concurrency test.
🤖 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 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.
Comment on lines
+61
to
+69
| dependent_wfs = set( | ||
| ToolInstance.objects.filter(tool_id=instance.pk) | ||
| .values_list("workflow_id", flat=True) | ||
| .distinct() | ||
| ) | ||
| if dependent_wfs: | ||
| logger.info( | ||
| f"Cannot delete exported tool {instance.prompt_registry_id}, " | ||
| f"depended by workflows {dependent_wfs}" |
Contributor
There was a problem hiding this comment.
🚀 Performance & Scalability | 🟡 Minor | ⚡ Quick win
Use an existence check instead of materializing every workflow ID.
The guard only needs to know whether any dependency exists, but set(values_list(...)) loads all matching IDs and then logs the entire set. A heavily referenced tool can cause unnecessary memory use and oversized logs. Use .exists() and log only a count or generic dependency message.
Proposed fix
- dependent_wfs = set(
- ToolInstance.objects.filter(tool_id=instance.pk)
- .values_list("workflow_id", flat=True)
- .distinct()
- )
- if dependent_wfs:
+ has_dependents = ToolInstance.objects.filter(
+ tool_id=instance.pk
+ ).exists()
+ if has_dependents:
logger.info(
- f"Cannot delete exported tool {instance.prompt_registry_id}, "
- f"depended by workflows {dependent_wfs}"
+ "Cannot delete exported tool %s because it has workflow "
+ "dependencies",
+ instance.prompt_registry_id,
)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| dependent_wfs = set( | |
| ToolInstance.objects.filter(tool_id=instance.pk) | |
| .values_list("workflow_id", flat=True) | |
| .distinct() | |
| ) | |
| if dependent_wfs: | |
| logger.info( | |
| f"Cannot delete exported tool {instance.prompt_registry_id}, " | |
| f"depended by workflows {dependent_wfs}" | |
| has_dependents = ToolInstance.objects.filter( | |
| tool_id=instance.pk | |
| ).exists() | |
| if has_dependents: | |
| logger.info( | |
| "Cannot delete exported tool %s because it has workflow " | |
| "dependencies", | |
| instance.prompt_registry_id, | |
| ) |
🤖 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 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.
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
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>
hari-kuriakose
force-pushed
the
feat/prompt-studio-ergonomics
branch
from
July 24, 2026 10:32
8235086 to
f41ee0e
Compare
|
Contributor
Unstract test resultsPer-group results
Critical paths
|
Contributor
There was a problem hiding this comment.
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 winValidate
outputbefore indexing.
output = payload.get(PSKeys.OUTPUT)yieldsNonewhen the key is absent, and lines 51-53 immediately index into it (output[PSKeys.NAME]), producing an unhandledTypeError/500 instead of a clearBadRequest.🛡️ 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 winDocument the registered endpoint.
The README advertises
/answer-sps, butSimplePromptStudioregisters/answer-prompt-publicinprompt-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
headersin the final output reflects only the last processed sheet. For multi-sheet workbooks,headersis the loop variable left over from the final iteration, sometadata.detected_headersmisrepresents earlier sheets. Consider aggregating headers per sheet (or dropping the singleheadersfield 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 winMissing
eval_settingskeys are reported as failures, not "disabled".
self.settingsdefaults to{}(Line 88), soself.settings["evaluate"]here (andself.settings["monitor_llm"]at Line 180) raiseKeyErrorwhen a prompt has no eval settings. ThatKeyErroris caught by the outer handler at Line 242 and re-raised asEvalFailedError, whereas the intended semantics for an unconfigured prompt isEvalDisabledError. 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 winLLM failure silently returns a passing score.
If
challenge_llm.completeraises (caught at Line 107),current_answeris never bound. Line 110 then hitsUnboundLocalError, which is caught by the broadexcept Exceptionat Line 111, logged as a misleading "JSON format error", and the method returnsdefault_answer(score: 5). A genuine LLM/transport failure is thus indistinguishable from a valid pass and skips the retry path inrun().🐛 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 winMalformed 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 valueDuplicate
FILE_PATHdefinition.
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 valueSort 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 winPreserve 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 winUse
logger.exception(...)inside theseexceptblocks. These handlers log withlogger.errorwhile an exception is in flight;logger.exceptioncaptures the traceback and clears the SonarCloud failures reported on lines 129/135/143 (also applies to theexcept Exceptionat 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 valueGlobal
ResourceWarningsuppression leaks beyond this plugin.
warnings.simplefiltermutates the process-wide filter at import time, so any host that imports this plugin losesResourceWarningvisibility everywhere (potentially masking unrelated leaks). Consider scoping it withwarnings.catch_warnings()/filterwarningsaround 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 winRe-raise without exception chaining.
raise PluginException(...)inside theexcept FileNotFoundErrorblock 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 valueLarge 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 valueMutable 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
flake8listed as a runtime dependency.
flake8is a linting tool and isn't imported anywhere in this plugin's source; it appears to be a dev-only tool accidentally added todependenciesinstead 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 valueTODO 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 winUse
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 oflogger.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 winRedundant double CSV parse; consider
DataFrame.to_numpy().
coloumnsis derived from a firstpd.read_csvcall and then immediately passed back asusecolsto 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 ofdf.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
⛔ Files ignored due to path filters (9)
prompt-service/src/unstract/prompt_service/plugins/challenge/uv.lockis excluded by!**/*.lockprompt-service/src/unstract/prompt_service/plugins/evaluation/uv.lockis excluded by!**/*.lockprompt-service/src/unstract/prompt_service/plugins/highlight_data/uv.lockis excluded by!**/*.lockprompt-service/src/unstract/prompt_service/plugins/line_item_extraction/uv.lockis excluded by!**/*.lockprompt-service/src/unstract/prompt_service/plugins/simple_prompt_studio/uv.lockis excluded by!**/*.lockprompt-service/src/unstract/prompt_service/plugins/single_pass_extraction/uv.lockis excluded by!**/*.lockprompt-service/src/unstract/prompt_service/plugins/smart_table_extractor/uv.lockis excluded by!**/*.lockprompt-service/src/unstract/prompt_service/plugins/summarize/uv.lockis excluded by!**/*.lockprompt-service/src/unstract/prompt_service/plugins/table_extractor/uv.lockis excluded by!**/*.lock
📒 Files selected for processing (85)
backend/prompt_studio/permission.pybackend/prompt_studio/prompt_studio_registry_v2/views.pyprompt-service/src/unstract/prompt_service/plugins/challenge/README.mdprompt-service/src/unstract/prompt_service/plugins/challenge/pyproject.tomlprompt-service/src/unstract/prompt_service/plugins/challenge/src/__init__.pyprompt-service/src/unstract/prompt_service/plugins/challenge/src/base.pyprompt-service/src/unstract/prompt_service/plugins/evaluation/README.mdprompt-service/src/unstract/prompt_service/plugins/evaluation/pyproject.tomlprompt-service/src/unstract/prompt_service/plugins/evaluation/src/__init__.pyprompt-service/src/unstract/prompt_service/plugins/evaluation/src/base.pyprompt-service/src/unstract/prompt_service/plugins/evaluation/src/constants.pyprompt-service/src/unstract/prompt_service/plugins/evaluation/src/frameworks/__init__.pyprompt-service/src/unstract/prompt_service/plugins/evaluation/src/frameworks/base.pyprompt-service/src/unstract/prompt_service/plugins/evaluation/src/frameworks/llama_index.pyprompt-service/src/unstract/prompt_service/plugins/evaluation/src/frameworks/ragas.pyprompt-service/src/unstract/prompt_service/plugins/evaluation/src/frameworks/unstract.pyprompt-service/src/unstract/prompt_service/plugins/highlight_data/README.mdprompt-service/src/unstract/prompt_service/plugins/highlight_data/pyproject.tomlprompt-service/src/unstract/prompt_service/plugins/highlight_data/src/__init__.pyprompt-service/src/unstract/prompt_service/plugins/highlight_data/src/base.pyprompt-service/src/unstract/prompt_service/plugins/highlight_data/src/constants.pyprompt-service/src/unstract/prompt_service/plugins/line_item_extraction/README.mdprompt-service/src/unstract/prompt_service/plugins/line_item_extraction/pyproject.tomlprompt-service/src/unstract/prompt_service/plugins/line_item_extraction/src/__init__.pyprompt-service/src/unstract/prompt_service/plugins/line_item_extraction/src/base.pyprompt-service/src/unstract/prompt_service/plugins/simple_prompt_studio/README.mdprompt-service/src/unstract/prompt_service/plugins/simple_prompt_studio/pyproject.tomlprompt-service/src/unstract/prompt_service/plugins/simple_prompt_studio/src/__init__.pyprompt-service/src/unstract/prompt_service/plugins/simple_prompt_studio/src/base.pyprompt-service/src/unstract/prompt_service/plugins/simple_prompt_studio/src/helper.pyprompt-service/src/unstract/prompt_service/plugins/single_pass_extraction/README.mdprompt-service/src/unstract/prompt_service/plugins/single_pass_extraction/pyproject.tomlprompt-service/src/unstract/prompt_service/plugins/single_pass_extraction/src/__init__.pyprompt-service/src/unstract/prompt_service/plugins/single_pass_extraction/src/base.pyprompt-service/src/unstract/prompt_service/plugins/single_pass_extraction/src/constants.pyprompt-service/src/unstract/prompt_service/plugins/single_pass_extraction/src/exceptions.pyprompt-service/src/unstract/prompt_service/plugins/single_pass_extraction/src/helper.pyprompt-service/src/unstract/prompt_service/plugins/smart_table_extractor/README.mdprompt-service/src/unstract/prompt_service/plugins/smart_table_extractor/pyproject.tomlprompt-service/src/unstract/prompt_service/plugins/smart_table_extractor/src/__init__.pyprompt-service/src/unstract/prompt_service/plugins/smart_table_extractor/src/constants.pyprompt-service/src/unstract/prompt_service/plugins/smart_table_extractor/src/converter/__init__.pyprompt-service/src/unstract/prompt_service/plugins/smart_table_extractor/src/converter/excel_to_tsv.pyprompt-service/src/unstract/prompt_service/plugins/smart_table_extractor/src/exceptions.pyprompt-service/src/unstract/prompt_service/plugins/smart_table_extractor/src/interface/__init__.pyprompt-service/src/unstract/prompt_service/plugins/smart_table_extractor/src/interface/runner.pyprompt-service/src/unstract/prompt_service/plugins/smart_table_extractor/src/processor/__init__.pyprompt-service/src/unstract/prompt_service/plugins/smart_table_extractor/src/processor/batch_processor.pyprompt-service/src/unstract/prompt_service/plugins/smart_table_extractor/src/processor/header_detector.pyprompt-service/src/unstract/prompt_service/plugins/smart_table_extractor/src/prompts/__init__.pyprompt-service/src/unstract/prompt_service/plugins/smart_table_extractor/src/prompts/prompts.pyprompt-service/src/unstract/prompt_service/plugins/smart_table_extractor/test_runner.pyprompt-service/src/unstract/prompt_service/plugins/summarize/README.mdprompt-service/src/unstract/prompt_service/plugins/summarize/pyproject.tomlprompt-service/src/unstract/prompt_service/plugins/summarize/src/__init__.pyprompt-service/src/unstract/prompt_service/plugins/summarize/src/base.pyprompt-service/src/unstract/prompt_service/plugins/summarize/src/constants.pyprompt-service/src/unstract/prompt_service/plugins/summarize/src/helper.pyprompt-service/src/unstract/prompt_service/plugins/table_extractor/README.mdprompt-service/src/unstract/prompt_service/plugins/table_extractor/pyproject.tomlprompt-service/src/unstract/prompt_service/plugins/table_extractor/src/__init__.pyprompt-service/src/unstract/prompt_service/plugins/table_extractor/src/constants.pyprompt-service/src/unstract/prompt_service/plugins/table_extractor/src/exceptions.pyprompt-service/src/unstract/prompt_service/plugins/table_extractor/src/extractor/__init__.pyprompt-service/src/unstract/prompt_service/plugins/table_extractor/src/extractor/core.pyprompt-service/src/unstract/prompt_service/plugins/table_extractor/src/interface/__init__.pyprompt-service/src/unstract/prompt_service/plugins/table_extractor/src/interface/helper.pyprompt-service/src/unstract/prompt_service/plugins/table_extractor/src/interface/runner.pyprompt-service/src/unstract/prompt_service/plugins/table_extractor/src/processor/__init__.pyprompt-service/src/unstract/prompt_service/plugins/table_extractor/src/processor/base_processor.pyprompt-service/src/unstract/prompt_service/plugins/table_extractor/src/processor/post_processor.pyprompt-service/src/unstract/prompt_service/plugins/table_extractor/src/prompts/__init__.pyprompt-service/src/unstract/prompt_service/plugins/table_extractor/src/prompts/bank_statement/extraction.txtprompt-service/src/unstract/prompt_service/plugins/table_extractor/src/prompts/bank_statement/headers.txtprompt-service/src/unstract/prompt_service/plugins/table_extractor/src/prompts/bank_statement/postprocessing.txtprompt-service/src/unstract/prompt_service/plugins/table_extractor/src/prompts/bank_statement/table_detect.txtprompt-service/src/unstract/prompt_service/plugins/table_extractor/src/prompts/bank_statement/table_span_contiguous.txtprompt-service/src/unstract/prompt_service/plugins/table_extractor/src/prompts/bank_statement/table_span_headers.txtprompt-service/src/unstract/prompt_service/plugins/table_extractor/src/prompts/prompts.pyprompt-service/src/unstract/prompt_service/plugins/table_extractor/src/prompts/rent_rolls/extraction.txtprompt-service/src/unstract/prompt_service/plugins/table_extractor/src/prompts/rent_rolls/headers.txtprompt-service/src/unstract/prompt_service/plugins/table_extractor/src/prompts/rent_rolls/postprocessing.txtprompt-service/src/unstract/prompt_service/plugins/table_extractor/src/prompts/rent_rolls/table_detect.txtprompt-service/src/unstract/prompt_service/plugins/table_extractor/src/prompts/rent_rolls/table_span_contiguous.txtprompt-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
Comment on lines
+190
to
+275
| def gen_final_results(self) -> None: | ||
| """Generates final results based on individual judges remarks. | ||
|
|
||
| Expected format for judges remarks: | ||
| { | ||
| "<eval_id><delim><eval_type><delim><optional:line_item_no>": { | ||
| "judges_remarks" [ | ||
| { | ||
| "name": str, | ||
| "remarks: dict | ||
| }, | ||
| ... | ||
| ], | ||
| "post_processing": {} | ||
| } | ||
| } | ||
|
|
||
| Final result will be passing only if the quorum is met as per the | ||
| eval strategy. | ||
| """ | ||
| for key, val in self.judges_remarks.items(): | ||
| parts = key.split(EvalConstant.JUDGE_REMARK_KEY_DELIM) | ||
| line_item_no = None | ||
| if len(parts) == 3: | ||
| eval_id, eval_type, line_item_no = parts | ||
| else: | ||
| eval_id, eval_type = parts | ||
|
|
||
| score = 0 | ||
| pass_count = 0 | ||
| feedbacks = [] | ||
| for r in val["judges_remarks"]: | ||
| j_name = r.get("name", None) | ||
| j_remarks = r["remarks"] | ||
|
|
||
| if j_remarks["passing"]: | ||
| pass_count += 1 | ||
| score += j_remarks["score"] | ||
| if j_name: | ||
| feedbacks.append(f"[{j_name}] {j_remarks['feedback']}") | ||
| elif not feedbacks: | ||
| feedbacks.append(f"{j_remarks['feedback']}") | ||
|
|
||
| score_avg = round(score / self.num_judges, 2) if score else 0 | ||
| passing = self._verify_quorum(pass_count) | ||
| feedback = "\n\n".join(feedbacks).strip() | ||
|
|
||
| final_result: dict[str, Any] = { | ||
| "score": score_avg, | ||
| "passing": passing, | ||
| "feedback": feedback, | ||
| } | ||
| p_eval_id, p_name, p_rel = None, None, None | ||
| p_disabled = False | ||
|
|
||
| # Post processing of final result | ||
| if val["post_processing"]: | ||
| p = val["post_processing"] | ||
| if "score_total" in p: | ||
| final_result["score"] = ( | ||
| f"{final_result['score']} / {p['score_total']}" | ||
| ) | ||
| if "name" in p: | ||
| p_name = p["name"] | ||
| if "line_item_no_in_id" in p: | ||
| if line_item_no and p["line_item_no_in_id"]: | ||
| p_eval_id = f"{eval_id}_#{line_item_no}" | ||
| if "line_item_no_in_name" in p: | ||
| if line_item_no and p["line_item_no_in_name"]: | ||
| p_name = p_name or eval_id.title().replace("_", " ") | ||
| p_name = f"{p_name} #{line_item_no}" | ||
| if "release" in p: | ||
| p_rel = p["release"] | ||
| if "disabled" in p: | ||
| p_disabled = p["disabled"] | ||
|
|
||
| self.results.append( | ||
| self._format_result( | ||
| final_result, | ||
| p_eval_id or eval_id, | ||
| eval_type, | ||
| name=p_name, | ||
| release=p_rel, | ||
| disabled=p_disabled, | ||
| ) | ||
| ) |
Contributor
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | 🏗️ Heavy lift
Reduce cognitive complexity of gen_final_results.
SonarCloud flags this method at cognitive complexity 47 vs. the 15 allowed limit, which is a failing CI check. Consider extracting the key-parsing, per-remark aggregation, and post-processing (score_total, line_item_no_in_id/name, release, disabled) into separate helper methods.
♻️ Sketch of extraction
- def gen_final_results(self) -> None:
- for key, val in self.judges_remarks.items():
- ...
+ def _parse_key(self, key: str) -> tuple[str, str, str | None]:
+ parts = key.split(EvalConstant.JUDGE_REMARK_KEY_DELIM)
+ if len(parts) == 3:
+ return parts[0], parts[1], parts[2]
+ return parts[0], parts[1], None
+
+ def _aggregate_remarks(self, val: dict[str, Any]) -> dict[str, Any]:
+ # score/pass_count/feedbacks aggregation extracted here
+ ...
+
+ def _apply_post_processing(self, val, eval_id, line_item_no, final_result):
+ # score_total / name / release / disabled handling extracted here
+ ...
+
+ def gen_final_results(self) -> None:
+ for key, val in self.judges_remarks.items():
+ eval_id, eval_type, line_item_no = self._parse_key(key)
+ final_result = self._aggregate_remarks(val)
+ p_eval_id, p_name, p_rel, p_disabled = self._apply_post_processing(
+ val, eval_id, line_item_no, final_result
+ )
+ self.results.append(
+ self._format_result(final_result, p_eval_id or eval_id, eval_type,
+ name=p_name, release=p_rel, disabled=p_disabled)
+ )🧰 Tools
🪛 GitHub Check: SonarCloud Code Analysis
[failure] 190-190: Refactor this function to reduce its Cognitive Complexity from 47 to the 15 allowed.
🤖 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/frameworks/base.py`
around lines 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.
Source: Linters/SAST tools
Comment on lines
+277
to
+305
| def run(self, tool: tuple[ToolLLM, LLM]) -> list[Any]: | ||
| """Run evaluators enabled in settings for given prompt, using the input | ||
| LLM Tool. | ||
|
|
||
| Always verifies quorum for the results from one or more judges. | ||
| """ | ||
| if not tool: | ||
| raise ValueError("At least one judge must be present") | ||
|
|
||
| app.logger.debug(f"[eval][{self.eval_fw}] Running evaluators: {self.settings}") | ||
|
|
||
| futures = [] | ||
| for metric, val in self.settings.items(): | ||
| if metric == "evaluate" or val is not True: | ||
| continue | ||
|
|
||
| parts = metric.split("_") | ||
| eval_id = "_".join(parts[2:]) | ||
| eval_type = parts[1] | ||
|
|
||
| evaluator_fn = f"_evaluate_{eval_id}" | ||
| if hasattr(self, evaluator_fn): | ||
| futures.append(getattr(self, evaluator_fn)(tool, eval_id, eval_type)) | ||
| else: | ||
| app.logger.debug(f"[eval][{self.eval_fw}] Skipping {eval_id} evaluator") | ||
|
|
||
| app.logger.debug(f"[eval][{self.eval_fw}] Completed running evaluators") | ||
|
|
||
| return futures |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Locate callers of run()/gen_final_results() to confirm futures are resolved first.
rg -n '\.run\(|gen_final_results' prompt-service/src/unstract/prompt_service/pluginsRepository: Zipstack/unstract
Length of output: 246
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "repo root:"
pwd
echo "top-level files:"
git ls-files | head -100
echo "Find base.py candidates:"
git ls-files | rg 'base\.py$|evaluation|prompt_service' || true
echo "Search base.py path variations:"
git ls-files | rg 'prompt-service/src/unstract/prompt_service/plugins/evaluation/src/frameworks/base.py|frameworks/base.py$' || trueRepository: Zipstack/unstract
Length of output: 4611
🏁 Script executed:
#!/bin/bash
set -uo pipefail
echo "Find relevant files:"
git ls-files | rg 'prompt-service|prompt_service|evaluation|frameworks/base.py|prompt' || true
echo
echo "Find run/gen_final_results anywhere tracked:"
rg -n '\.run\(|def run\(self|gen_final_results|def gen_final_results' . -g '*.py' || true
echo
echo "Find class definitions/import paths for frameworks/base.py:"
rg -n 'class .*Evaluation|from .*frameworks\.base|import .*frameworks\.base|`@concurrent`\.thread|return futures' . -g '*.py' || trueRepository: Zipstack/unstract
Length of output: 20507
🌐 Web query:
prompt-service/src/unstract/prompt_service/plugins/evaluation/src/frameworks/base.py run gen_final_results
💡 Result:
The run gen_final_results method referenced in the path prompt-service/src/unstract/prompt_service/plugins/evaluation/src/frameworks/base.py is a component of the Unstract open-source platform's evaluation plugin framework [1][2]. In the Unstract Prompt Service architecture, the evaluation plugin system is responsible for assessing the quality of LLM-generated responses after prompt execution [1]. When evaluation is enabled in the prompt settings, the answer_prompt controller invokes an evaluator plugin [3]. The base framework for these evaluation plugins, located in base.py, defines a structure where the run() method (often referred to as run_gen_final_results in specific implementations or internal documentation) acts as the execution entry point [3][1]. Key characteristics of this process include: - Purpose: It performs post-execution analysis, computing quality metrics such as faithfulness, correctness, relevance, PII detection, toxicity, and completeness [1]. - Integration: It operates after the main LLM completion and optional challenge validation stages [1]. - Data Handling: The results computed by the run method are stored in the output metadata with prefixes such as eval_<metric_name> [1]. - Configuration: Execution of this evaluation logic is controlled via eval_settings in the prompt output, which can include flags like evaluate (boolean) and options to exclude_failed based on the resulting quality scores [4][1]. This system is designed to provide automated quality assurance for document extraction workflows without modifying the core extraction output itself [1].
Citations:
- 1: https://deepwiki.com/Zipstack/unstract/4.7-prompt-execution-and-output-processing
- 2: https://deepwiki.com/Zipstack/unstract/4.6-prompt-service-and-llm-integration
- 3: https://github.com/Zipstack/unstract/blob/c1667cd7/prompt-service/src/unstract/prompt_service/controllers/answer_prompt.py
- 4: https://github.com/Zipstack/unstract/blob/c1667cd7/prompt-service/src/unstract/prompt_service/constants.py
🏁 Script executed:
#!/bin/bash
set -uo pipefail
echo "GitHub API file availability at commit ref if available:"
git rev-parse HEAD 2>/dev/null || true
git describe --tags --always --dirty 2>/dev/null || true
echo
echo "Direct Web API: file contents from GitHub main/master:"
curl -fsSL 'https://raw.githubusercontent.com/Zipstack/unstract/main/prompt-service/src/unstract/prompt_service/plugins/evaluation/src/frameworks/base.py' | sed -n '240,320p'
echo
echo "Check answer_prompt integration on that file:"
curl -fsSL 'https://raw.githubusercontent.com/Zipstack/unstract/main/prompt-service/src/unstract/prompt_service/controllers/answer_prompt.py' | sed -n '1,220p'
echo
echo "Find evaluation decorators/subclasses in tracked repo if present:"
rg -n '`@concurrent`\.thread|class .*Evaluation|evaluate_[a-z_]+|gen_final_results|evaluators' prompt-service/unstract/workers -g '*.py' 2>/dev/null || trueRepository: Zipstack/unstract
Length of output: 530
🌐 Web query:
site:github.com/Zipstack/unstract "frameworks/base.py" "run(self, tool" "gen_final_results"
💡 Result:
The term gen_final_results is associated with the Unstract platform, specifically appearing in the context of errors reported during API deployments [1]. The error message "Final output processing failed:" indicates a failure in assembling or returning the structured output after a workflow has successfully executed its RUN stage and stored the results [1]. In the Unstract codebase, workflow execution and result handling involve various components, such as WorkflowHelper [2], DestinationConnector [3], and result caching utilities [4]. The process generally follows stages like SOURCE, INITIALIZE, COMPILE, BUILD, and RUN [1]. When an API deployment is used, a specific wrapper attempts to finalize the output; if this process encounters an issue—even if the LLM generation itself succeeded—it may report a failure in final output processing [1]. While there is a file named base.py within the backend settings (backend/backend/settings/base.py) [5][6], the specific function or method call gen_final_results is typically related to the logic for processing and packaging workflow execution results (often defined in DTOs like FileExecutionResult) before they are returned to the user or cached in Redis [7][4]. Users encountering this error have reported that the underlying workflow execution completes successfully, pointing to a fault within the API deployment integration layer rather than the workflow logic itself [1].
Citations:
- 1: fix: [ISSUE] "error": "Final output processing failed: " when using API deployment #1684
- 2: https://github.com/Zipstack/unstract/blob/c1667cd7/backend/workflow_manager/workflow_v2/workflow_helper.py
- 3: https://github.com/Zipstack/unstract/blob/c1667cd7/backend/workflow_manager/endpoint_v2/destination.py
- 4: https://github.com/Zipstack/unstract/blob/c1667cd7/backend/workflow_manager/endpoint_v2/result_cache_utils.py
- 5: https://github.com/Zipstack/unstract/blob/c1667cd7/backend/backend/settings/base.py
- 6: 06128e0
- 7: https://github.com/Zipstack/unstract/blob/c1667cd7/backend/workflow_manager/endpoint_v2/dto.py
Resolve evaluator futures before reading gen_final_results.
run() queues thread-backed _evaluate_* work and returns before the callbacks can update shared result state. Ensure the caller awaits/resolves every item in the returned collection before invoking gen_final_results() or reading the evaluator 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/evaluation/src/frameworks/base.py`
around lines 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.
Comment on lines
+46
to
+47
| MAX_ATTEMPTS = os.getenv("LINE_ITEM_EXTRACTION_MAX_LLM_CALLS", 5) | ||
| while attempts < MAX_ATTEMPTS: |
Contributor
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
MAX_ATTEMPTS is a string when the env var is set, breaking the loop comparison.
os.getenv returns a str whenever LINE_ITEM_EXTRACTION_MAX_LLM_CALLS is defined, while the fallback is int 5. On line 47 attempts < MAX_ATTEMPTS then compares int < str, raising TypeError and failing every extraction whenever the env var is configured.
🐛 Proposed fix
- MAX_ATTEMPTS = os.getenv("LINE_ITEM_EXTRACTION_MAX_LLM_CALLS", 5)
+ MAX_ATTEMPTS = int(os.getenv("LINE_ITEM_EXTRACTION_MAX_LLM_CALLS", 5))📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| MAX_ATTEMPTS = os.getenv("LINE_ITEM_EXTRACTION_MAX_LLM_CALLS", 5) | |
| while attempts < MAX_ATTEMPTS: | |
| MAX_ATTEMPTS = int(os.getenv("LINE_ITEM_EXTRACTION_MAX_LLM_CALLS", 5)) | |
| while attempts < MAX_ATTEMPTS: |
🤖 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/line_item_extraction/src/base.py`
around lines 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.
Comment on lines
+161
to
+173
| else: | ||
| app.logger.info( | ||
| "No challenge plugin found to challenge single_pass_extraction" | ||
| ) | ||
| except challenge_plugin["exception_cls"] as e: | ||
| app.logger.error("Failed to challenge single_pass_extraction: %s", e) | ||
| challenge_metrics = ( | ||
| { | ||
| f"{challenge_llm.get_usage_reason()}_llm": challenge_llm.get_metrics() # noqa: E501 | ||
| } | ||
| if enable_challenge | ||
| else {} | ||
| ) |
Contributor
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
UnboundLocalError when challenge is enabled but the challenge plugin is missing.
When enable_challenge is truthy and PluginManager().get_plugin("challenge") returns falsy, the else branch (lines 161-164) runs and challenge_llm is never assigned. Line 167-173 still evaluates the metrics dict because enable_challenge is truthy, so challenge_llm.get_usage_reason() raises UnboundLocalError and crashes the request. Gate the metrics on an actually-created challenge_llm rather than on enable_challenge. Relatedly, the except challenge_plugin["exception_cls"] handler on line 165 subscripts challenge_plugin, which is None in this same path (or unbound if get_plugin raised), so it will fail during exception handling too.
🐛 Suggested guard
- challenge_metrics = (
- {
- f"{challenge_llm.get_usage_reason()}_llm": challenge_llm.get_metrics() # noqa: E501
- }
- if enable_challenge
- else {}
- )
+ challenge_metrics = (
+ {
+ f"{challenge_llm.get_usage_reason()}_llm": challenge_llm.get_metrics() # noqa: E501
+ }
+ if enable_challenge and "challenge_llm" in locals()
+ else {}
+ )Also initialize challenge_llm = None before the block and make the except catch a concrete exception type instead of challenge_plugin["exception_cls"].
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| else: | |
| app.logger.info( | |
| "No challenge plugin found to challenge single_pass_extraction" | |
| ) | |
| except challenge_plugin["exception_cls"] as e: | |
| app.logger.error("Failed to challenge single_pass_extraction: %s", e) | |
| challenge_metrics = ( | |
| { | |
| f"{challenge_llm.get_usage_reason()}_llm": challenge_llm.get_metrics() # noqa: E501 | |
| } | |
| if enable_challenge | |
| else {} | |
| ) | |
| else: | |
| app.logger.info( | |
| "No challenge plugin found to challenge single_pass_extraction" | |
| ) | |
| except challenge_plugin["exception_cls"] as e: | |
| app.logger.error("Failed to challenge single_pass_extraction: %s", e) | |
| challenge_metrics = ( | |
| { | |
| f"{challenge_llm.get_usage_reason()}_llm": challenge_llm.get_metrics() # noqa: E501 | |
| } | |
| if enable_challenge and "challenge_llm" in locals() | |
| else {} | |
| ) |
🧰 Tools
🪛 GitHub Check: SonarCloud Code Analysis
[failure] 166-166: Use "logging.exception()" instead.
🤖 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/base.py`
around lines 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.
Comment on lines
+74
to
+104
|
|
||
| # Extract settings | ||
| schema = prompt | ||
| SmartTableExtractorRunner._validate_inputs(table_settings, schema) | ||
|
|
||
| header_detection_rows = table_settings.get( | ||
| SmartTableExtractorKeys.HEADER_DETECTION_ROWS, | ||
| SmartTableExtractorDefaults.HEADER_DETECTION_ROWS, | ||
| ) | ||
| rows_per_batch = table_settings.get( | ||
| SmartTableExtractorKeys.ROWS_PER_BATCH, | ||
| SmartTableExtractorDefaults.ROWS_PER_BATCH, | ||
| ) | ||
| max_retries = table_settings.get( | ||
| SmartTableExtractorKeys.MAX_RETRIES, | ||
| SmartTableExtractorDefaults.MAX_RETRIES, | ||
| ) | ||
|
|
||
| # Step 2: Read all sheets from Excel and process each separately | ||
| logger.info(f"Reading all sheets from Excel file: {input_file}") | ||
|
|
||
| file_content = fs_instance.read(path=input_file, mode="rb") | ||
| # Save to temp file for pandas to read | ||
| # Use lowercase suffix to avoid extension case issues | ||
| suffix = os.path.splitext(input_file)[1].lower() or ".xlsx" | ||
| with tempfile.NamedTemporaryFile( | ||
| mode="wb", suffix=suffix, delete=False | ||
| ) as tmp_file: | ||
| tmp_file.write(file_content) | ||
| tmp_excel_path = tmp_file.name | ||
|
|
Contributor
There was a problem hiding this comment.
🩺 Stability & Availability | 🔴 Critical | 🏗️ Heavy lift
run() reads inputs from prompt/input_file params and an assumed-non-null fs_instance, so every current caller fails. The callers in test_runner.py (test_with_local_file, test_with_remote_file) put schema and input_file inside table_settings and never pass the prompt/input_file parameters; they also pass fs_instance=None for local files. As written:
- Line 76
schema = prompt→None, so_validate_inputsraisesSchemaValidationExceptionimmediately. - Line 98
os.path.splitext(input_file)withinput_file=None→TypeError. - Line 95
fs_instance.read(...)withfs_instance=None→AttributeErroron the local-file path.
Source these from table_settings and default fs_instance to local storage.
🐛 Proposed fix
- # Step 1: Validate inputs
-
- # Extract settings
- schema = prompt
- SmartTableExtractorRunner._validate_inputs(table_settings, schema)
+ # Step 1: Validate inputs
+ schema = table_settings.get(SmartTableExtractorKeys.SCHEMA)
+ SmartTableExtractorRunner._validate_inputs(table_settings, schema)
+ schema = table_settings.get(SmartTableExtractorKeys.SCHEMA)
+ input_file = table_settings.get(SmartTableExtractorKeys.INPUT_FILE)
@@
- file_content = fs_instance.read(path=input_file, mode="rb")
+ if fs_instance is None:
+ fs_instance = FileStorage(FileStorageProvider.LOCAL)
+ file_content = fs_instance.read(path=input_file, mode="rb")Add the provider import:
from unstract.sdk1.file_storage import FileStorage
+from unstract.sdk1.file_storage.provider import FileStorageProvider📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| # Extract settings | |
| schema = prompt | |
| SmartTableExtractorRunner._validate_inputs(table_settings, schema) | |
| header_detection_rows = table_settings.get( | |
| SmartTableExtractorKeys.HEADER_DETECTION_ROWS, | |
| SmartTableExtractorDefaults.HEADER_DETECTION_ROWS, | |
| ) | |
| rows_per_batch = table_settings.get( | |
| SmartTableExtractorKeys.ROWS_PER_BATCH, | |
| SmartTableExtractorDefaults.ROWS_PER_BATCH, | |
| ) | |
| max_retries = table_settings.get( | |
| SmartTableExtractorKeys.MAX_RETRIES, | |
| SmartTableExtractorDefaults.MAX_RETRIES, | |
| ) | |
| # Step 2: Read all sheets from Excel and process each separately | |
| logger.info(f"Reading all sheets from Excel file: {input_file}") | |
| file_content = fs_instance.read(path=input_file, mode="rb") | |
| # Save to temp file for pandas to read | |
| # Use lowercase suffix to avoid extension case issues | |
| suffix = os.path.splitext(input_file)[1].lower() or ".xlsx" | |
| with tempfile.NamedTemporaryFile( | |
| mode="wb", suffix=suffix, delete=False | |
| ) as tmp_file: | |
| tmp_file.write(file_content) | |
| tmp_excel_path = tmp_file.name | |
| # Step 1: Validate inputs | |
| schema = table_settings.get(SmartTableExtractorKeys.SCHEMA) | |
| SmartTableExtractorRunner._validate_inputs(table_settings, schema) | |
| schema = table_settings.get(SmartTableExtractorKeys.SCHEMA) | |
| input_file = table_settings.get(SmartTableExtractorKeys.INPUT_FILE) | |
| # Step 2: Read all sheets from Excel and process each separately | |
| logger.info(f"Reading all sheets from Excel file: {input_file}") | |
| if fs_instance is None: | |
| fs_instance = FileStorage(FileStorageProvider.LOCAL) | |
| file_content = fs_instance.read(path=input_file, mode="rb") | |
| # Save to temp file for pandas to read | |
| # Use lowercase suffix to avoid extension case issues | |
| suffix = os.path.splitext(input_file)[1].lower() or ".xlsx" | |
| with tempfile.NamedTemporaryFile( | |
| mode="wb", suffix=suffix, delete=False | |
| ) as tmp_file: | |
| tmp_file.write(file_content) | |
| tmp_excel_path = tmp_file.name |
🤖 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 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.
| headers: list[Any] = table_settings.get(TEKeys.HEADERS, []) | ||
| start_page: int = table_settings.get(TEKeys.START_PAGE, 0) | ||
| end_page: int = table_settings.get(TEKeys.END_PAGE, 0) | ||
| document_type: str = table_settings.get(TEKeys.DOCUMENT_TYPE, "") |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Divergent defaults for TEKeys.DOCUMENT_TYPE across files. runner.py and base_processor.py each independently call table_settings.get(TEKeys.DOCUMENT_TYPE, ...) on the same key but with different fallback values ("default" vs ""), rather than threading one resolved value through. This can cause the two consumers to disagree on the effective document type whenever the key is absent from table_settings.
prompt-service/src/unstract/prompt_service/plugins/table_extractor/src/processor/base_processor.py#L26: use the same default value asrunner.py(or acceptdocument_typeas a parameter from the caller instead of re-readingtable_settings).prompt-service/src/unstract/prompt_service/plugins/table_extractor/src/interface/runner.py#L30: resolvedocument_typeonce (e.g. inrun_table_extraction) and pass it intoTableExtractionBase.extract_large_tableinstead of letting each function re-derive it with its own default.
📍 Affects 2 files
prompt-service/src/unstract/prompt_service/plugins/table_extractor/src/processor/base_processor.py#L26-L26(this comment)prompt-service/src/unstract/prompt_service/plugins/table_extractor/src/interface/runner.py#L30-L30
🤖 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 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.
Comment on lines
+82
to
+101
| # TODO Extract document metadata : | ||
| app.logger.info(f"Table : {table_id} loaded {file} to table extractor.") | ||
| page_no_index = large_table_extractor.find_table() | ||
| app.logger.info(f"Table : {table_id} found on page {page_no_index+1}.") | ||
| headers = large_table_extractor.extract_header(page_no_index) | ||
| if headers == [] and document_type is not None: | ||
| if document_type.lower() == "bank_statement": | ||
| app.logger.warning( | ||
| "No headers found, using default headers for bank statement" | ||
| ) | ||
| headers = [ | ||
| "Date", | ||
| "Description", | ||
| "Value", | ||
| ] | ||
| app.logger.info(f"Using default headers: {headers} for bank statement") | ||
| if document_type.lower() == "rent_rolls": | ||
| app.logger.error(f"No headers found for rent rolls : {file}") | ||
| raise PluginException(f"No headers found for rent rolls : {file}") | ||
| else: |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Silent fallthrough when headers are empty and document_type doesn't match either special case.
When headers == [] and document_type is neither "bank_statement" nor "rent_rolls" (e.g. the "default" type used elsewhere in this same pipeline), neither inner branch fires, the else logging branch is unreachable from here, and extraction silently proceeds with an empty headers list into span-search and CSV extraction — with no log or exception to signal the problem.
🐛 Proposed fix to surface the fallthrough
if headers == [] and document_type is not None:
if document_type.lower() == "bank_statement":
app.logger.warning(
"No headers found, using default headers for bank statement"
)
headers = [
"Date",
"Description",
"Value",
]
app.logger.info(f"Using default headers: {headers} for bank statement")
- if document_type.lower() == "rent_rolls":
+ elif document_type.lower() == "rent_rolls":
app.logger.error(f"No headers found for rent rolls : {file}")
raise PluginException(f"No headers found for rent rolls : {file}")
+ else:
+ app.logger.warning(
+ f"No headers found for document type '{document_type}'; "
+ "proceeding without headers."
+ )
else:
app.logger.info(f"Extracted header from the table {table_id} : {headers}.")📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| # TODO Extract document metadata : | |
| app.logger.info(f"Table : {table_id} loaded {file} to table extractor.") | |
| page_no_index = large_table_extractor.find_table() | |
| app.logger.info(f"Table : {table_id} found on page {page_no_index+1}.") | |
| headers = large_table_extractor.extract_header(page_no_index) | |
| if headers == [] and document_type is not None: | |
| if document_type.lower() == "bank_statement": | |
| app.logger.warning( | |
| "No headers found, using default headers for bank statement" | |
| ) | |
| headers = [ | |
| "Date", | |
| "Description", | |
| "Value", | |
| ] | |
| app.logger.info(f"Using default headers: {headers} for bank statement") | |
| if document_type.lower() == "rent_rolls": | |
| app.logger.error(f"No headers found for rent rolls : {file}") | |
| raise PluginException(f"No headers found for rent rolls : {file}") | |
| else: | |
| # TODO Extract document metadata : | |
| app.logger.info(f"Table : {table_id} loaded {file} to table extractor.") | |
| page_no_index = large_table_extractor.find_table() | |
| app.logger.info(f"Table : {table_id} found on page {page_no_index+1}.") | |
| headers = large_table_extractor.extract_header(page_no_index) | |
| if headers == [] and document_type is not None: | |
| if document_type.lower() == "bank_statement": | |
| app.logger.warning( | |
| "No headers found, using default headers for bank statement" | |
| ) | |
| headers = [ | |
| "Date", | |
| "Description", | |
| "Value", | |
| ] | |
| app.logger.info(f"Using default headers: {headers} for bank statement") | |
| elif document_type.lower() == "rent_rolls": | |
| app.logger.error(f"No headers found for rent rolls : {file}") | |
| raise PluginException(f"No headers found for rent rolls : {file}") | |
| else: | |
| app.logger.warning( | |
| f"No headers found for document type '{document_type}'; " | |
| "proceeding without headers." | |
| ) | |
| else: |
🤖 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`
around lines 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.
| from ..exceptions import PluginException | ||
|
|
||
| logger = logging.getLogger(__name__) | ||
| TABLE_DEBUG = os.getenv("TABLE_DEBUG", "true").lower() == "true" |
Contributor
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
TABLE_DEBUG defaults to true, leaving generated code and extracted document data on disk by default.
Unless an operator explicitly sets TABLE_DEBUG=false, the finally block never deletes script_path/tsv_path, so the LLM-generated script and the extracted document TSV remain in /tmp indefinitely. Debug behavior defaulting to "on" for a data-retention-sensitive path is risky.
🔒️ Proposed fix
-TABLE_DEBUG = os.getenv("TABLE_DEBUG", "true").lower() == "true"
+TABLE_DEBUG = os.getenv("TABLE_DEBUG", "false").lower() == "true"Also applies to: 72-80
🤖 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 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.
Comment on lines
+119
to
+148
| def _run_processing_script(script_path: str, tsv_path: str) -> str: | ||
| is_k8s = os.getenv("KUBERNETES_SERVICE_HOST") is not None | ||
| fs_instance_local = FileStorageHelper.local_file_system_init() | ||
| is_docker = fs_instance_local.exists("/.dockerenv") | ||
| if is_k8s or is_docker: | ||
| cmd = ["python3", script_path, tsv_path] | ||
| logger.info( | ||
| f"Running post processing directly in {'Kubernetes' if is_k8s else 'Docker'}: {' '.join(cmd)}" | ||
| ) | ||
| else: | ||
| cmd_str = ( | ||
| "docker run --rm --memory=512m --cpus=1 --network=none " | ||
| f"--mount type=bind,source={script_path},target=/app/script.py,readonly " | ||
| f"--mount type=bind,source={tsv_path},target=/app/tsvdata.tsv,readonly " | ||
| "--workdir /app " | ||
| "gcr.io/distroless/python3-debian12:nonroot " | ||
| "script.py tsvdata.tsv" | ||
| ) | ||
| logger.info(f"Running post processing in container: {cmd_str}") | ||
| cmd = cmd_str.split() | ||
|
|
||
| try: | ||
| result = subprocess.run(cmd, check=True, capture_output=True, text=True) | ||
| return result.stdout | ||
| except subprocess.CalledProcessError as e: | ||
| logger.error(f"Script execution failed: {e.stderr}") | ||
| logger.error(f"Command failed with exit code {e.returncode}:") | ||
| raise PluginException( | ||
| f"Unexpected error occured while post processing. {str(e)}", | ||
| ) from e |
Contributor
There was a problem hiding this comment.
🔒 Security & Privacy | 🔴 Critical | 🏗️ Heavy lift
Unsandboxed execution of LLM-generated code in production, with no subprocess timeout.
_run_processing_script executes the LLM-generated Python script (script_path) directly on the host — no container, no --network=none, no memory/CPU limits — precisely when is_k8s or is_docker is true, i.e. in real deployments. The heavily sandboxed docker-container path (--network=none --memory=512m --cpus=1, distroless nonroot image) only applies in the non-container local-dev branch, which is the opposite of what you'd want: the deployed, multi-tenant environment is where sandboxing untrusted/LLM-generated code matters most.
Additionally, subprocess.run(cmd, check=True, capture_output=True, text=True) on line 141 has no timeout=. A hung or infinite-looping generated script (garbage/malicious output from the LLM, or a prompt-injected document) will block this call — and the request thread — indefinitely. This also means subprocess.TimeoutExpired/TimeoutError can never actually be raised here, making the except TimeoutError: handler on line 151 dead code.
🛡️ Suggested direction
- if is_k8s or is_docker:
- cmd = ["python3", script_path, tsv_path]
- ...
- else:
- cmd_str = (
- "docker run --rm --memory=512m --cpus=1 --network=none "
- ...
- )
- cmd = cmd_str.split()
+ # Always run the LLM-generated script inside a resource-limited,
+ # network-isolated sandbox, regardless of the deployment environment.
+ cmd_str = (
+ "docker run --rm --memory=512m --cpus=1 --network=none "
+ f"--mount type=bind,source={script_path},target=/app/script.py,readonly "
+ f"--mount type=bind,source={tsv_path},target=/app/tsvdata.tsv,readonly "
+ "--workdir /app gcr.io/distroless/python3-debian12:nonroot script.py tsvdata.tsv"
+ )
+ cmd = cmd_str.split()- result = subprocess.run(cmd, check=True, capture_output=True, text=True)
+ result = subprocess.run(
+ cmd, check=True, capture_output=True, text=True, timeout=60
+ )📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| def _run_processing_script(script_path: str, tsv_path: str) -> str: | |
| is_k8s = os.getenv("KUBERNETES_SERVICE_HOST") is not None | |
| fs_instance_local = FileStorageHelper.local_file_system_init() | |
| is_docker = fs_instance_local.exists("/.dockerenv") | |
| if is_k8s or is_docker: | |
| cmd = ["python3", script_path, tsv_path] | |
| logger.info( | |
| f"Running post processing directly in {'Kubernetes' if is_k8s else 'Docker'}: {' '.join(cmd)}" | |
| ) | |
| else: | |
| cmd_str = ( | |
| "docker run --rm --memory=512m --cpus=1 --network=none " | |
| f"--mount type=bind,source={script_path},target=/app/script.py,readonly " | |
| f"--mount type=bind,source={tsv_path},target=/app/tsvdata.tsv,readonly " | |
| "--workdir /app " | |
| "gcr.io/distroless/python3-debian12:nonroot " | |
| "script.py tsvdata.tsv" | |
| ) | |
| logger.info(f"Running post processing in container: {cmd_str}") | |
| cmd = cmd_str.split() | |
| try: | |
| result = subprocess.run(cmd, check=True, capture_output=True, text=True) | |
| return result.stdout | |
| except subprocess.CalledProcessError as e: | |
| logger.error(f"Script execution failed: {e.stderr}") | |
| logger.error(f"Command failed with exit code {e.returncode}:") | |
| raise PluginException( | |
| f"Unexpected error occured while post processing. {str(e)}", | |
| ) from e | |
| def _run_processing_script(script_path: str, tsv_path: str) -> str: | |
| is_k8s = os.getenv("KUBERNETES_SERVICE_HOST") is not None | |
| fs_instance_local = FileStorageHelper.local_file_system_init() | |
| is_docker = fs_instance_local.exists("/.dockerenv") | |
| cmd_str = ( | |
| "docker run --rm --memory=512m --cpus=1 --network=none " | |
| f"--mount type=bind,source={script_path},target=/app/script.py,readonly " | |
| f"--mount type=bind,source={tsv_path},target=/app/tsvdata.tsv,readonly " | |
| "--workdir /app " | |
| "gcr.io/distroless/python3-debian12:nonroot " | |
| "script.py tsvdata.tsv" | |
| ) | |
| logger.info( | |
| f"Running post processing inside a sandboxed container: {cmd_str}" | |
| ) | |
| cmd = cmd_str.split() | |
| try: | |
| result = subprocess.run( | |
| cmd, check=True, capture_output=True, text=True, timeout=60 | |
| ) | |
| return result.stdout | |
| except subprocess.CalledProcessError as e: | |
| logger.error(f"Script execution failed: {e.stderr}") | |
| logger.error(f"Command failed with exit code {e.returncode}:") | |
| raise PluginException( | |
| f"Unexpected error occured while post processing. {str(e)}", | |
| ) from e |
🧰 Tools
🪛 ast-grep (0.44.1)
[error] 140-140: Command coming from incoming request
Context: subprocess.run(cmd, check=True, capture_output=True, text=True)
Note: [CWE-78] Improper Neutralization of Special Elements used in an OS Command ('OS Command Injection').
(subprocess-from-request)
[error] 140-140: Use of unsanitized data to create processes
Context: subprocess.run(cmd, check=True, capture_output=True, text=True)
Note: [CWE-78] Improper Neutralization of Special Elements used in an OS Command ('OS Command Injection').
(os-system-unsanitized-data)
🪛 GitHub Check: SonarCloud Code Analysis
[failure] 145-145: Use "logging.exception()" instead.
[failure] 144-144: Use "logging.exception()" instead.
🪛 Ruff (0.15.21)
[error] 141-141: subprocess call: check for execution of untrusted input
(S603)
[warning] 147-147: Use explicit conversion flag
Replace with conversion flag
(RUF010)
🤖 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 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.
Comment on lines
+149
to
+155
| except FileNotFoundError: | ||
| logger.error("The command could not be found.") | ||
| except TimeoutError: | ||
| logger.error("The command timed out.") | ||
| except Exception as e: | ||
| logger.error(f"An unexpected error occurred: {e}") | ||
|
|
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Swallowed errors return None, masking the real failure downstream.
The except FileNotFoundError and except Exception handlers here only log and fall through — the function implicitly returns None. Back in the caller (line 63), output.strip() on None raises AttributeError, which gets caught and reported as PluginException("Post processing failed", "'NoneType' object has no attribute 'strip'") — hiding the actual cause (missing command, unexpected error) from logs/users.
🐛 Proposed fix
except subprocess.CalledProcessError as e:
logger.error(f"Script execution failed: {e.stderr}")
logger.error(f"Command failed with exit code {e.returncode}:")
raise PluginException(
f"Unexpected error occured while post processing. {str(e)}",
) from e
except FileNotFoundError:
- logger.error("The command could not be found.")
+ logger.error("The command could not be found.")
+ raise PluginException("Post processing command not found.")
except TimeoutError:
- logger.error("The command timed out.")
+ logger.error("The command timed out.")
+ raise PluginException("Post processing timed out.")
except Exception as e:
- logger.error(f"An unexpected error occurred: {e}")
+ logger.error(f"An unexpected error occurred: {e}")
+ raise PluginException(f"Unexpected error during post processing: {e}") from e📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| except FileNotFoundError: | |
| logger.error("The command could not be found.") | |
| except TimeoutError: | |
| logger.error("The command timed out.") | |
| except Exception as e: | |
| logger.error(f"An unexpected error occurred: {e}") | |
| except FileNotFoundError: | |
| logger.error("The command could not be found.") | |
| raise PluginException("Post processing command not found.") | |
| except TimeoutError: | |
| logger.error("The command timed out.") | |
| raise PluginException("Post processing timed out.") | |
| except Exception as e: | |
| logger.error(f"An unexpected error occurred: {e}") | |
| raise PluginException(f"Unexpected error during post processing: {e}") from e |
🧰 Tools
🪛 GitHub Check: SonarCloud Code Analysis
[failure] 154-154: Use "logging.exception()" instead.
🪛 Ruff (0.15.21)
[warning] 153-153: Do not catch blind exception: Exception
(BLE001)
🤖 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 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().
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What
Two API ergonomics fixes from a full
unstractCLI run. Anchored at24d08728.This PR was scoped to cover three ergonomics items; one of them (#5, the no-RAG profile) is deliberately excluded — see "Descoped" below, which is the most important part of this description.
1. No way to delete an exported registry tool
The registry is read-only over the API —
prompt_studio_registry_v2/urls.pymaps only{"get": "list"}. The only way to remove an entry is to delete the Prompt Studio project, which cascades to it. That works, but it is implicit, undocumented, and blunt: you cannot unpublish a tool while keeping the project.Adds
DELETE registry/<pk>/, guarded by the same in-use checkprompt-studio deletealready performs — a tool still attached to a workflow is refused with 409 rather than silently breaking those workflows. The guard filtersToolInstanceontool_id=instance.pk, matching the existing check inprompt_studio_core_v2/views.py:209(an exported tool'stool_idis itsprompt_registry_id).One subtlety worth reviewing:
get_querysetpreviously returnedNonewhen no query-param filters were present, which would breakget_object()on a detail route. It now returns the full queryset when addressing a single row by PK. I keyed this offself.kwargs.get("pk")rather thanself.detail— DRF setscls.detail = NoneinViewSetMixin.as_view()and only populates it for router-generated views. This viewset is wired manually viaas_view(), soself.detailwould have been falsy on the new delete route. The existinglistroute is unaffected (nopkkwarg → same filter path as before).Authorization on the new DELETE route
Worth reviewing closely, since this is the first by-PK operation on this viewset (it previously exposed only
list).DEFAULT_PERMISSION_CLASSESis empty in settings, and this viewset declared nopermission_classes.OrganizationFilterBackenddoes run insideget_object()(viafilter_queryset), so cross-org deletion was never possible — but any member of the same org could have deleted any other member's exported tool by PK. AddedIsRegistryToolOwner, gating onlydestroy:CustomTool, mirroringIsParentToolOwner(same pattern forProfileManager).custom_toolis nullable).listvisibility is untouched — still derived bylist_tools. Only the destructive route is restricted.2. API-key creation wants an identifier already present in the URL
POST keys/api/<api_id>/takesapi_idas a path segment but also expectsapiin the body — the same value, spelled twice. Omitting the body field fails validation.POST keys/pipeline/<pipeline_id>/has the identical shape.The cause: POST routes to the default
ModelViewSet.create, which never sees the URL kwargs, so the body has to repeat them.createnow derives the target from the path when present. It usessetdefault, so an explicit body value still wins, and falls through to the default implementation for the body-only routes (keys/api/,keys/pipeline/) — both keep working.Descoped: the no-RAG profile (SERVER.md #5)
SERVER.md proposed making
vector_storeandembedding_modelnullable so achunk_size=0("no RAG") profile need not supply adapters that are never read. I implemented this, then reverted it — the premise does not hold in the code.SERVER.md's justification is that with
chunk_size=0"neither the vector DB nor the embedding model is queried." Butbuild_single_pass_payloadsetsdefault_profile.chunk_size = 0atprompt_studio_helper.py:1140and then reads both FKs 25 lines later, in the same function:export-tooldoes 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.So making the columns nullable without guarding every reader would convert a clear, immediate 400 at profile creation into
AttributeError: 'NoneType' object has no attribute 'id'— a 500 deep inside single-pass or export, in exactly the mode #5 claims is safe. That is strictly worse friction than what it set out to remove. The serializer-only alternative does not escape this either: to store null you still need 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 of those paths should do with an absent adapter — real design work, and its own PR. Happy to take it on separately if you want it; I did not want to either grind through 46 guards inside an ergonomics PR or ship a half-done migration.
Verification
ruffandruff-formatpass at the version pinned in.pre-commit-config.yaml(v0.3.4).Note on tests: the Django-backed suites in these apps currently error with
AppRegistryNotReadyin a plain checkout. A test written in the existing style would silently skip and read as coverage that isn't there. Happy to add one if there's a documented way to run those suites in CI.Impact
--api-id/--pipeline-idalone is enough to create a key; existing body-based callers are unaffected.🤖 Generated with Claude Code