feat(api): delete terminal bank operations + control-plane UI (#2677) - #5
feat(api): delete terminal bank operations + control-plane UI (#2677)#5chethanuk wants to merge 3 commits into
Conversation
|
CodeAnt AI is reviewing your PR. |
Thanks for using CodeAnt! 🎉We're free for open-source projects. if you're enjoying it, help us grow by sharing. Share on X · |
|
Warning Review limit reached
Next review available in: 41 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: ⛔ Files ignored due to path filters (2)
📒 Files selected for processing (7)
📝 WalkthroughWalkthroughThe PR adds terminal async-operation deletion across the backend API, generated Go and Python clients, control-plane routes and UI, OpenAPI specifications, localization resources, and regression tests. Deletion is restricted to failed, cancelled, and completed operations. ChangesTerminal operation deletion
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant Operator
participant OperationsView
participant ControlPlaneRoute
participant DeleteOperationAPI
participant MemoryEngine
Operator->>OperationsView: Select delete
OperationsView->>ControlPlaneRoute: DELETE bank operation
ControlPlaneRoute->>DeleteOperationAPI: DELETE operation record
DeleteOperationAPI->>MemoryEngine: Delete terminal operation
MemoryEngine-->>DeleteOperationAPI: Deletion result or validation error
DeleteOperationAPI-->>ControlPlaneRoute: HTTP response
ControlPlaneRoute-->>OperationsView: Response payload
OperationsView-->>Operator: Refresh operation list
Suggested reviewers: 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ Finishing Touches🧪 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 |
codeant-ai
Bot
added
the
size:XXL
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a new feature to permanently delete terminal async operations (failed, cancelled, or completed) from the database. It adds a new DELETE endpoint, updates the backend engine, updates the Go, Python, and TypeScript client SDKs, and integrates the deletion action into the control plane UI with localized messages. The review feedback highlights a routing inconsistency in the control plane BFF where the delete endpoint is mapped to the base operation path instead of mirroring the backend's /record suffix, which would conflict with future cancel operations. Additionally, a duplicate decorator issue was found in the Python client code.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| async deleteOperation(bankId: string, operationId: string) { | ||
| return this.fetchApi<{ | ||
| success: boolean; | ||
| message: string; | ||
| operation_id: string; | ||
| }>(bankApi(bankId, `/operations/${encodeURIComponent(operationId)}`), { | ||
| method: "DELETE", | ||
| }); | ||
| } |
There was a problem hiding this comment.
The backend API distinguishes between cancel (DELETE /operations/{id}) and hard delete (DELETE /operations/{id}/record).
By mapping deleteOperation to DELETE /api/banks/{bankId}/operations/{operationId} in the control plane BFF, you are using the base DELETE verb on the operation resource for hard deletion. This creates a major inconsistency with the backend API design and prevents the BFF from ever supporting the standard RESTful cancel operation via DELETE on the base operation resource.
To maintain consistency and avoid future routing conflicts, the control plane BFF and client should mirror the backend's /record suffix for hard deletion:
- Update
deleteOperationinControlPlaneClientto call/operations/${encodeURIComponent(operationId)}/record. - Move the
DELETEhandler from/api/banks/[bankId]/operations/[operationId]/route.tsto a new route file at/api/banks/[bankId]/operations/[operationId]/record/route.ts.
| async deleteOperation(bankId: string, operationId: string) { | |
| return this.fetchApi<{ | |
| success: boolean; | |
| message: string; | |
| operation_id: string; | |
| }>(bankApi(bankId, `/operations/${encodeURIComponent(operationId)}`), { | |
| method: "DELETE", | |
| }); | |
| } | |
| async deleteOperation(bankId: string, operationId: string) { | |
| return this.fetchApi<{ | |
| success: boolean; | |
| message: string; | |
| operation_id: string; | |
| }>(bankApi(bankId, `/operations/${encodeURIComponent(operationId)}/record`), { | |
| method: "DELETE", | |
| }); | |
| } |
| export async function DELETE( | ||
| request: Request, | ||
| { params }: { params: Promise<{ bankId: string; operationId: string }> } | ||
| ) { | ||
| try { | ||
| const { bankId, operationId } = await params; | ||
|
|
||
| if (!bankId) { | ||
| return NextResponse.json( | ||
| localizeApiErrorPayload(request, { | ||
| error: "bank_id is required", | ||
| errorKey: "api.errors.validation.bankIdRequired", | ||
| }), | ||
| { status: 400 } | ||
| ); | ||
| } | ||
|
|
||
| if (!operationId) { | ||
| return NextResponse.json( | ||
| localizeApiErrorPayload(request, { | ||
| error: "operation_id is required", | ||
| errorKey: "api.errors.validation.operationIdRequired", | ||
| }), | ||
| { status: 400 } | ||
| ); | ||
| } | ||
|
|
||
| const response = await sdk.deleteOperation({ | ||
| client: lowLevelClient, | ||
| path: { bank_id: bankId, operation_id: operationId }, | ||
| }); | ||
| return respondWithSdk(response, "Failed to delete operation", { request }); | ||
| } catch (error) { | ||
| console.error("Error deleting operation:", error); | ||
| return NextResponse.json( | ||
| localizeApiErrorPayload(request, { | ||
| error: "Failed to delete operation", | ||
| errorKey: "api.errors.operations.delete", | ||
| }), | ||
| { status: 500 } | ||
| ); | ||
| } | ||
| } |
There was a problem hiding this comment.
As noted in the client API review, this DELETE handler should be moved to a new sub-route file at /api/banks/[bankId]/operations/[operationId]/record/route.ts to match the backend's /record suffix for hard deletion. This keeps the base DELETE verb on /api/banks/[bankId]/operations/[operationId] available for operation cancellation (matching the backend's DELETE /operations/{id} cancel endpoint).
| @validate_call | ||
|
|
||
| @validate_call | ||
| async def delete_operation( |
There was a problem hiding this comment.
There is a duplicate @validate_call decorator here, and the @validate_call decorator for retry_operation (at line 1133) has been accidentally removed. Please remove the extra decorator at line 1012 and restore it before retry_operation.
| expect(fetchSpy).toHaveBeenCalledWith( | ||
| expect.stringMatching(/\/api\/banks\/bank-a\/operations\/op-1$/), | ||
| expect.objectContaining({ method: "DELETE" }) | ||
| ); |
There was a problem hiding this comment.
If the client API is updated to use the /record suffix (e.g., /api/banks/bank-a/operations/op-1/record) as recommended, this test assertion should be updated accordingly to expect the /record suffix.
| expect(fetchSpy).toHaveBeenCalledWith( | |
| expect.stringMatching(/\/api\/banks\/bank-a\/operations\/op-1$/), | |
| expect.objectContaining({ method: "DELETE" }) | |
| ); | |
| expect(fetchSpy).toHaveBeenCalledWith( | |
| expect.stringMatching(/\/api\/banks\/bank-a\/operations\/op-1\/record$/), | |
| expect.objectContaining({ method: "DELETE" }) | |
| ); |
![codeant-ai[bot]](https://avatars.githubusercontent.com/in/646884?s=60&v=4){kind=small}
| @validate_call | ||
| async def delete_operation( |
There was a problem hiding this comment.
Suggestion: The new method is wrapped by validate_call twice, so every delete_operation invocation runs full pydantic argument validation twice. This adds avoidable overhead on each call and is a behavior regression from the rest of the client methods; keep only a single decorator on this function. [performance]
Severity Level: Major ⚠️
- ⚠️ Python delete_operation client call does double validation.
- ⚠️ Slightly slower terminal operation deletes via Python client.Steps of Reproduction ✅
1. Open the Python client file
`hindsight-clients/python/hindsight_client_api/api/operations_api.py` and locate the
`delete_operation` method definition at lines 33-52 in the current file (corresponding to
PR hunk lines 1014-1053), where it is preceded by two consecutive `@validate_call`
decorators (lines 33 and 35 in the file, 1012 and 1014 in the diff).
2. Note that `@validate_call` from Pydantic wraps the function to validate all arguments
on each invocation; applying it twice means the underlying `async def
delete_operation(...)` at line 36 (diff line 1015) is wrapped by two separate validators.
3. From external user code, import and instantiate `OperationsApi` from
`hindsight-clients/python/hindsight_client_api/api/operations_api.py` and call `await
OperationsApi(api_client).delete_operation(bank_id="bank-1", operation_id="op-1")` with
any valid strings; this is the normal usage of this generated client.
4. On every such call, both decorators run full Pydantic argument validation, as the outer
`@validate_call` validates and then calls the inner validated wrapper, causing the same
arguments to be parsed and checked twice, increasing CPU cost compared with other
OperationsApi methods that use a single `@validate_call` decorator.
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** hindsight-clients/python/hindsight_client_api/api/operations_api.py
**Line:** 1014:1015
**Comment:**
*Performance: The new method is wrapped by `validate_call` twice, so every `delete_operation` invocation runs full pydantic argument validation twice. This adds avoidable overhead on each call and is a behavior regression from the rest of the client methods; keep only a single decorator on this function.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix| client: lowLevelClient, | ||
| path: { bank_id: bankId, operation_id: operationId }, | ||
| }); | ||
| return respondWithSdk(response, "Failed to delete operation", { request }); |
There was a problem hiding this comment.
Suggestion: This failure label is not registered in failureErrorKeys, so error responses from this path will skip the standard localized errorKey mapping and return inconsistent payloads compared with other operation endpoints. Pass the explicit delete error key in respondWithSdk options (or add the label mapping) so 4xx/5xx responses remain contract-consistent. [api mismatch]
Severity Level: Major ⚠️
- ⚠️ Delete-operation endpoint returns non-localized error payloads.
- ⚠️ Error schema inconsistent with other operation endpoints.Steps of Reproduction ✅
1. Open the handler file
`hindsight-control-plane/src/app/api/banks/[bankId]/operations/[operationId]/route.ts` and
locate the new `DELETE` handler at lines 32-74 in the current file (PR hunk around lines
101-143). At line 63 (diff line 132), it calls `respondWithSdk(response, "Failed to delete
operation", { request });` with failureLabel `"Failed to delete operation"` and no
explicit `errorKey`.
2. Open `hindsight-control-plane/src/lib/sdk-response.ts` and inspect the
`failureErrorKeys` map defined at lines 12-38. This map contains entries for other
operation labels like `"Failed to get operation status"` (line 29), `"Failed to fetch
operations"` (line 34), and `"Failed to cancel operation"` (line 35), but does not contain
a key for `"Failed to delete operation"`.
3. In the same `sdk-response.ts` file, inspect `respondWithSdk` at lines 84-123. On the
error path (lines 95-119), it derives `errorKey` via `const errorKey =
errorOptions?.errorKey ?? failureErrorKeys[failureLabel];` (line 99). When the DELETE
handler passes only `{ request }` and the `failureErrorKeys` map has no `"Failed to delete
operation"` entry, `errorKey` becomes `undefined` and the payload is built without an
`errorKey` field (lines 104-107), skipping localization (`localizeApiErrorPayload` is only
used when `errorKey` is truthy at lines 114-116).
4. Trigger an upstream error from the dataplane delete-operation API—for example, call
`DELETE /app/api/banks/{bankId}/operations/{operationId}` with a non-existent operation ID
so the underlying dataplane route `hindsight-api-slim/hindsight_api/api/http.py:5543-5554`
returns a non-2xx status. The control-plane DELETE handler at `route.ts:32-63` forwards
this result into `respondWithSdk`, which returns a JSON response whose body has `error:
"Failed to delete operation"` but no `errorKey`, unlike other operation endpoints (e.g.,
`/app/api/operations/[agentId]/route.ts:48` for cancellation) that have mapped failure
labels and thus include localized `errorKey` values from `failureErrorKeys`, leading to
inconsistent error payloads for delete vs. other operation actions.
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** hindsight-control-plane/src/app/api/banks/[bankId]/operations/[operationId]/route.ts
**Line:** 132:132
**Comment:**
*Api Mismatch: This failure label is not registered in `failureErrorKeys`, so error responses from this path will skip the standard localized `errorKey` mapping and return inconsistent payloads compared with other operation endpoints. Pass the explicit delete error key in `respondWithSdk` options (or add the label mapping) so 4xx/5xx responses remain contract-consistent.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix
|
CodeAnt AI finished reviewing your PR. |
f682ce4 to
40a4402
Compare
🤖 CodeAnt AI — Review Status
Updated in place by CodeAnt AI · last 5 reviews |
Thanks for using CodeAnt! 🎉We're free for open-source projects. if you're enjoying it, help us grow by sharing. Share on X · |
codeant-ai
Bot
added
size:XXL
Maintainer review on vectorize-io#2777 asked for the hard-delete endpoint to live at /delete rather than /record. Renames the path segment end-to-end: dataplane route + log message, tests, OpenAPI spec (and its skills mirror), and the generated Python/TypeScript/Go clients. The route's operation_id is explicitly "delete_operation", so no generated symbol names change -- only the path string. Go and Python generated output was verified byte-identical against the pinned openapi-generator v7.10.0.
1 participant
User description
Problem
There is no way to remove terminal (
failed/cancelled/completed) async operation rows.DELETE …/operations/{id}is bound to cancel, which only flipspending → cancelledand returns 409 for anything else — the row never leaves the table, so failed operations pile up in the control plane with no cleanup path and no navigation to the document they produced.Fixes vectorize-io#2677
Fix
DELETE /v1/default/banks/{bank_id}/operations/{operation_id}/record: status-guarded hard delete for terminal states only, implemented as a single bank-scopedDELETE … RETURNING(no TOCTOU window with a concurrent retry). 404 when missing, 409 for non-terminal states. The existing cancel route is untouched.delete_operation+BankWriteOperation.DELETE_OPERATION(operation-validator aware).result_metadata.document_idis present — the navigation path the issue reporter asked for.flowchart LR subgraph cp [Control plane] U["Operations table / dialog<br/>(Delete on terminal rows)"] --> C["client.deleteOperation()"] C --> B["BFF DELETE /api/banks/{bankId}/operations/{id}"] end B --> S["SDK deleteOperation"] S --> R["DELETE …/operations/{id}/record"] R --> G{"status ∈ failed |<br/>cancelled | completed?"} G -->|yes| D["single bank-scoped<br/>DELETE … RETURNING"] G -->|"no (pending/processing)"| X["409 — use cancel<br/>(unchanged route)"] D --> P["row gone on next poll"]Semantics: cancel vs delete
DELETE …/operations/{id}pending → cancelled(soft, unchanged)DELETE …/operations/{id}/recordA review bot suggested the control plane was calling the cancel verb — that's a mix-up of the BFF namespace with the dataplane path: the BFF's
DELETE /api/banks/{bankId}/operations/{id}handler calls the generated SDK, which requests…/operations/{id}/record; cancel lives on a separate BFF route (/api/operations/{bankId}).Review fixes folded in
@validate_callondelete_operationand had dropped the decorator fromretry_operation(silently losing runtime arg validation there)."Failed to delete operation"registered infailureErrorKeysso error payloads localize like every sibling operation endpoint (keys already existed in all 10 locales).How to test
Manual E2E: retain a doc so an operation ends
failed→ Operations → detail → View Document (whendocument_idset) → Delete → row disappears on next poll; cancel of apendingop still works via the old route.Test evidence
Run post-rebase on upstream
main(52b893b93), squashed branch:pytest tests/test_operation_status.py tests/test_operation_progress.py -n0npm testnpm run i18n:checkruff check/format --check/ty checkon touched modulesOut of scope
processingoperations — tracked upstream in Stop hook resends entire session transcript every retain call (O(N²) growth) — still present in 0.7.4 vectorize-io/hindsight#2644 / Crash-interrupted operations are re-claimed indefinitely — claim attempts never count toward max_attempts (poison-pill regrind loop) vectorize-io/hindsight#2675; the status guard deliberately excludesprocessing.delete_documentdoes not cascade to operations (separate issue).completeddespite a deleted failed child. Parent linkage lives in JSONresult_metadata(no FK). If maintainers prefer a hard guard, the clean shape is aNOT EXISTS (non-terminal parent)predicate folded into the same single-statement DELETE — happy to add it in this PR on request.Summary by CodeRabbit
New Features
Bug Fixes
Documentation
CodeAnt-AI Description
Delete terminal operations and open linked documents from the operations view
What Changed
Impact
✅ Cleaner operations lists✅ Fewer stale failed-operation rows✅ Faster access to generated documents💡 Usage Guide
Checking Your Pull Request
Every time you make a pull request, our system automatically looks through it. We check for security issues, mistakes in how you're setting up your infrastructure, and common code problems. We do this to make sure your changes are solid and won't cause any trouble later.
Talking to CodeAnt AI
Got a question or need a hand with something in your pull request? You can easily get in touch with CodeAnt AI right here. Just type the following in a comment on your pull request, and replace "Your question here" with whatever you want to ask:
This lets you have a chat with CodeAnt AI about your pull request, making it easier to understand and improve your code.
Example
Preserve Org Learnings with CodeAnt
You can record team preferences so CodeAnt AI applies them in future reviews. Reply directly to the specific CodeAnt AI suggestion (in the same thread) and replace "Your feedback here" with your input:
This helps CodeAnt AI learn and adapt to your team's coding style and standards.
Example
Retrigger review
Ask CodeAnt AI to review the PR again, by typing:
Check Your Repository Health
To analyze the health of your code repository, visit our dashboard at https://app.codeant.ai. This tool helps you identify potential issues and areas for improvement in your codebase, ensuring your repository maintains high standards of code health.