Repository navigation
Conversation
- Metrics health check returns a fixed message instead of str(e), which named the DB/Redis host, port and user; the exception is still logged. - Destination read failures (backend and the workers' copy) no longer put the execution-storage path in the error returned to the caller; the path stays in the server log. - Delete WorkflowExecutionInternalViewSet / FileBatchCreateInternalAPIView from workflow_v2/views.py: unrouted copies of the classes in workflow_manager/internal_views.py. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
ritwik-g
commented
Oct 28, 2026
ritwik-g
left a comment
Contributor
Author
There was a problem hiding this comment.
Round 1 — review-deep (INITIAL), tip c43401e
Review: unstract:review-deep. Lite was refused by its gate on auth_authz_secrets (lens 4).
Security deep-dive: done. The find sub-task returned no candidates at or above 0.7.
boundary-sweep: 6 sinks traced (two health messages, two destination details, the backend queue detail, the worker OSError) — clean.
Verdict — APPROVE WITH NITS
Summary — Critical: 0 · High: 0 · Medium: 0 · Low: 2 · Lenses run: 19/19
Findings (both below the severity floor)
- [Low] [Lens 3] — the HITL queue read error no longer separates "missing" from "unreadable" for the caller —
backend/workflow_manager/endpoint_v2/destination.py:L1147-L1157,workers/shared/workflow/destination_connector.py:L1901-L1911. The server log keeps the distinction. Accepted. - [Low] [Lens 16] — the test docstring says a cache error names "host, port and user"; Redis names host and port only —
backend/dashboard_metrics/tests/test_health_check.py:L3-L4.
Lens checklist
- Spec & intent — Clean
- Architectural fit — Clean
- Correctness & edge cases — See findings #1
- Security — Clean (deep-dive done + boundary sweep, orchestrator)
- Data integrity & migrations — N/A — no schema/model change (orchestrator)
- Concurrency — N/A — no shared state (orchestrator)
- API & contract compatibility — Clean (no consumer parses the changed strings; the deleted classes have no references in this repo or unstract-cloud)
- Reliability — N/A — no external calls changed (orchestrator)
- Performance & cost — Clean
- Observability — Clean
- Operational safety — N/A — no infra (orchestrator)
- LLM/agent — N/A — none (orchestrator)
- Testing — Clean (exact-string assertions fail against the base)
- Dependencies & build — N/A — none (orchestrator)
- Code quality — Clean (pre-commit passed on these files in #2397, orchestrator)
- Doc & comment accuracy — See findings #2
- Cross-document consistency — Clean (the merge sweep found the backend and worker messages consistent, orchestrator)
- On-prem experience — N/A — no on-prem surface (orchestrator)
- Scaling & workload lifecycle — N/A — none (orchestrator)
Open questions for the author
- The routed
workflow_manager/internal_views.py(L201, L649, L1097) and other views inworkflow_v2/views.pystill returnstr(e)on the internal service-to-service API. That code predates this PR and is not in the requested alert set. It is a follow-up candidate.
Assumptions made — file_name is the caller's own upload name.
Security deep-dive: done
cost: agents=7 wall=14:00 lenses-examined=10 of 19 files=7 lines=544
ritwik-g
added a commit
that referenced
this pull request
Oct 28, 2026
The metrics-health, storage-path and dead-view changes now live in #2400; this PR keeps only the x2text work. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
SonarCloud S8572/S5778: the caller's message no longer carries the error, so destination read failures (backend and workers) log with logger.exception to keep the traceback. Tests now build the connector before pytest.raises and assert the handled exception, which carries the storage path, is what reaches the log. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
for more information, see https://pre-commit.ci
ritwik-g
commented
Oct 28, 2026
ritwik-g
left a comment
Contributor
Author
There was a problem hiding this comment.
Round 2 — review-deep (FOLLOWUP), tip 24b494e, delta c43401e..24b494e
Review: unstract:review-deep. The delta addresses SonarCloud S8572 and S5778, and includes a pre-commit.ci format commit.
boundary-sweep: 3 sinks traced (three logger.exception sites). Clean. A traceback adds frames and the provider's cause chain, but no locals. The JSONDecodeError .doc (file content) is never printed.
Verdict — APPROVE WITH NITS
Prior context fetched — the round-1 findings are below the severity floor. Since boundary: c43401e.
Scope change — NO.
Summary — Critical: 0 · High: 0 · Medium: 1 · Low: 0 · Lenses run: 19/19
New findings
- [Medium] [Lens 13] — the queue-read tests do not pin that a traceback is logged:
logger.exception(..., exc_info=False)survives- Location:
backend/workflow_manager/endpoint_v2/tests/test_destination_error_detail.py:L46-L51,workers/tests/test_queue_read_error_detail.py:L44-L57 - Failure mode: the path now reaches the log only through the traceback, and the mock ignores
exc_info. - Suggested fix: record nothing as handled when
exc_infois falsy. - Confidence: High. Verification: VERIFIED by a mutant run on the workers test.
- Location:
Lens checklist
- Spec & intent — Clean
- Architectural fit — Clean
- Correctness & edge cases — Clean (WorkerLogger is a stdlib Logger, so
.exceptionworks) - Security — Not covered — deep-dive not re-run for this logging-level delta (ran round 1); boundary sweep clean
- Data integrity & migrations — N/A — none (orchestrator)
- Concurrency — N/A — none (orchestrator)
- API & contract compatibility — N/A — no response change (orchestrator)
- Reliability — N/A — none (orchestrator)
- Performance & cost — Clean (orchestrator: extra traceback volume is negligible)
- Observability — Clean
- Operational safety — N/A — none (orchestrator)
- LLM/agent — N/A — none (orchestrator)
- Testing — See findings #1
- Dependencies & build — N/A — none (orchestrator)
- Code quality — Clean (pre-commit.ci autofix applied, orchestrator)
- Doc & comment accuracy — Clean (orchestrator)
- Cross-document consistency — Clean (backend and workers match, orchestrator)
- On-prem experience — N/A — none (orchestrator)
- Scaling & workload lifecycle — N/A — none (orchestrator)
Open questions — none.
Security deep-dive: unavailable — ran in round 1; this delta only changes logging levels and tests
cost: agents=3 wall=3:00 lenses-examined=7 of 19 files=4 lines=99
The recorder now ignores a call made with exc_info disabled, so a log without the traceback (and so without the storage path) fails. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
ritwik-g
commented
Oct 28, 2026
ritwik-g
left a comment
Contributor
Author
There was a problem hiding this comment.
Round 3 — review-deep (FOLLOWUP), tip b3b52e5, delta 24b494e..b3b52e5
Review: unstract:review-deep. The delta is test-only. boundary-sweep: N/A, since the delta adds no sinks.
Verdict — APPROVE
Prior context fetched — round-2 finding #1, from review 5685638668. Since boundary: 24b494e.
Scope change — NO.
Summary — Critical: 0 · High: 0 · Medium: 0 · Low: 0 · Lenses run: 19/19
Prior findings status
- #1 [Medium] the
exc_info=Falsemutant survived → RESOLVED. The recorders atbackend/.../test_destination_error_detail.py:46-54andworkers/tests/test_queue_read_error_detail.py:44-51now kill it at all four sites. The old recorder let it survive.
New findings — none.
Lens checklist
- Spec & intent — Clean
- Architectural fit — N/A — test-only (orchestrator)
- Correctness & edge cases — N/A — test-only (orchestrator)
- Security — N/A — test-only (orchestrator)
- Data integrity & migrations — N/A — none (orchestrator)
- Concurrency — N/A — none (orchestrator)
- API & contract compatibility — N/A — none (orchestrator)
- Reliability — N/A — none (orchestrator)
- Performance & cost — N/A — none (orchestrator)
- Observability — N/A — none (orchestrator)
- Operational safety — N/A — none (orchestrator)
- LLM/agent — N/A — none (orchestrator)
- Testing — Clean
- Dependencies & build — N/A — none (orchestrator)
- Code quality — Clean (pre-commit, orchestrator)
- Doc & comment accuracy — Clean (the new comments match, orchestrator)
- Cross-document consistency — Clean (backend and workers recorders match, orchestrator)
- On-prem experience — N/A — none (orchestrator)
- Scaling & workload lifecycle — N/A — none (orchestrator)
Open questions — none.
Security deep-dive: unavailable — this delta is test-only; the deep-dive ran in round 1
cost: agents=1 wall=3:00 lenses-examined=4 of 19 files=2 lines=12
ritwik-g
marked this pull request as ready for review
October 29, 2026 14:23
Contributor
 [Medium impact] The PR appears safe to merge; no actionable regressions were found.
|
Contributor
Unstract test resultsPer-group results
Critical paths
|
chandrasekharan-zipstack
approved these changes
Oct 30, 2026
chandrasekharan-zipstack
left a comment
Contributor
There was a problem hiding this comment.
LGTM, however consider removing code comments similar to what I've suggested since they go stale or less relevant with time and would bloat agent context in the future
Comment on lines
+1165
to
+1166
| # The exception text names the DB host, port and user, and this | ||
| # endpoint answers any org member, so it stays in the server log. |
Contributor
There was a problem hiding this comment.
Suggested change
| # The exception text names the DB host, port and user, and this | |
| # endpoint answers any org member, so it stays in the server log. |
| return {"status": "error", "message": "Cache read/write mismatch"} | ||
| except Exception as e: | ||
| except Exception: | ||
| # Same as the database check: the Redis error names host and port. |
Contributor
There was a problem hiding this comment.
Suggested change
| # Same as the database check: the Redis error names host and port. |
Comment on lines
+639
to
+641
| # The error carries the execution-storage path, which stays in the | ||
| # server log (with the traceback); the caller gets a message | ||
| # without it. |
Contributor
There was a problem hiding this comment.
Suggested change
| # The error carries the execution-storage path, which stays in the | |
| # server log (with the traceback); the caller gets a message | |
| # without it. |
Comment on lines
+1151
to
+1152
| # The error names the execution-storage path: log it with the | ||
| # traceback, but keep it out of the message returned to the caller. |
Contributor
There was a problem hiding this comment.
Suggested change
| # The error names the execution-storage path: log it with the | |
| # traceback, but keep it out of the message returned to the caller. |
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
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
Stops three API error paths from handing internal details to callers, and deletes dead duplicate views. Split out of #2397, which now covers only x2text.
Metrics health leak (closes CodeQL #448)
GET .../metrics/health/is open to any org member. It returnedstr(e)from the DB and cache checks, which names the DB/Redis host, port and user. It now returns"Database unavailable"/"Cache unavailable"instead, and still logs the exception withlogger.exception. Status codes are unchanged (200/503).Storage path in API errors (closes CodeQL #358, #359)
DestinationConnector.get_tool_execution_result_from_metadataand_read_file_content_for_queueput the full execution-storage path in theAPIExceptiondetail. The caller now gets a message without the path:output not foundoutput is not valid JSONFailed to read file content for queue: <file name>The path stays in the server log. The workers' copy (
workers/shared/workflow/destination_connector.py) had the same leak and gets the same fix.Dead duplicate internal views (closes CodeQL #451, #452, #453)
WorkflowExecutionInternalViewSetandFileBatchCreateInternalAPIViewinworkflow_v2/views.pywere unrouted copies of the classes inworkflow_manager/internal_views.py, whichworkflow_execution_internal_urls.pyroutes. I grepped this repo (urls, tests, workers) and unstract-cloud: nothing imports them fromworkflow_v2.viewsor patches them there. I deleted both, along with the imports only they used.Testing
backend/dashboard_metrics/tests/test_health_check.py: DB failure, cache failure and the healthy case.backend/workflow_manager/endpoint_v2/tests/test_destination_error_detail.pyandworkers/tests/test_queue_read_error_detail.py: the path is absent from the caller's message and present in the log.dashboard_metrics,workflow_manager,inboxand the org-scoping views: 200 passed. The one failure,test_api_storage_mime_validation::test_pdf_marker_inside_other_content..., also fails onmainlocally because of libmagic, and is unrelated.manage.py checkis clean. The workers tests pass.🤖 Generated with Claude Code