feat(studio): delete persisted operations#2553
Merged
Conversation
comatory
commented
Feb 25, 2026
Router-nonroot image scan passed✅ No security vulnerabilities found in image: |
comatory
force-pushed
the
ondrej/eng-8830-controlplane-cli-delete-persistent-operation
branch
from
e9c707b to
30f4a7b
Compare
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughAdds a persisted-operation retirement flow: proto messages and RPC, updated generated client bindings, backend handler and repository deletion, metrics-based safety check and authorization, expanded tests, frontend retire UI and dialog, and a dev debug script/README note. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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 |
![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: 8
🧹 Nitpick comments (2)
controlplane/src/core/bufservices/persisted-operation/retirePersistedOperation.ts (2)
56-75: Redundant ternary —operationis guaranteed truthy here.After the early return on line 56-63 when
!operation, the ternaryoperation ? { ... } : undefinedon line 69 can never be false. Simplify to unconditional assignment.Proposed simplification
return { response: { code: EnumStatusCode.OK, }, - operation: operation - ? { - id: operation.id, - operationId: operation.operationId, - } - : undefined, + operation: { + id: operation.id, + operationId: operation.operationId, + }, };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@controlplane/src/core/bufservices/persisted-operation/retirePersistedOperation.ts` around lines 56 - 75, The response builds an "operation" field using a redundant ternary even though earlier in retirePersistedOperation (after the early return when !operation) operation is always truthy; remove the ternary and set operation unconditionally (e.g., operation: { id: operation.id, operationId: operation.operationId }) so the response is simpler and avoids unreachable branches—update the return in retirePersistedOperation accordingly.
14-76: No audit log entry for this destructive operation.Other mutating operations in the controlplane typically emit audit log entries. Retiring/deleting a persisted operation is a significant action that should be tracked for compliance and debugging. Consider adding an audit log entry on success, following the pattern used by other handlers.
#!/bin/bash # Check if publishPersistedOperations creates audit log entries for comparison rg -n "audit" controlplane/src/core/bufservices/persisted-operation/ -A 2 -B 2🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@controlplane/src/core/bufservices/persisted-operation/retirePersistedOperation.ts` around lines 14 - 76, The handler retirePersistedOperation currently returns success without recording an audit trail; after the call to OperationsRepository.retirePersistedOperation and once operation is confirmed non-null, emit an audit log entry (following the existing audit pattern used by other persisted-operation handlers) before returning: include actor info from authContext, headers from ctx.requestHeader, target info (federatedGraph.id and req.fedGraphName), the operation identifiers (operation.id and operation.operationId), and result status EnumStatusCode.OK; place this call immediately after the operation variable is set and before the final return so successful retires are recorded.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@controlplane/README.md`:
- Line 78: Replace the incorrect verb form in the README summary sentence:
change "If the frontend ensure that the session endpoint `/v1/auth/session` is
called on focus and load. The user might never be logged out again." to use
"ensures" (e.g., "If the frontend ensures that the session endpoint
`/v1/auth/session` is called on focus and load, the user might never be logged
out again.") so the grammar and sentence flow are correct.
In
`@controlplane/src/core/bufservices/persisted-operation/retirePersistedOperation.ts`:
- Around line 51-54: retirePersistedOperation currently only removes the DB
record via OperationsRepository.retirePersistedOperation but leaves the
operation blob in storage; after the DB delete, call
opts.blobStorage.deleteObject() to remove the blob at the same key used by
publishPersistedOperations (format:
`${organizationId}/${federatedGraph.id}/operations/${clientName}/${operationId}.json`)
using organizationId, federatedGraph.id (or federatedGraphId), req.operationId
and the client name (available in the handler context), and handle/log any
delete errors without failing the overall retire flow.
In `@controlplane/src/core/repositories/OperationsRepository.ts`:
- Around line 154-176: The delete in retirePersistedOperation is under-scoped:
change the delete on federatedGraphPersistedOperations to include the
federatedGraphId condition (same as the initial findFirst) so you only remove
rows matching both operationId and this.federatedGraphId (or use the table's
composite primary key if available); ensure the .where on the delete mirrors the
and(eq(federatedGraphPersistedOperations.operationId, operationId),
eq(federatedGraphPersistedOperations.federatedGraphId, this.federatedGraphId))
filter to avoid deleting rows from other graphs.
In `@controlplane/test/persisted-operations.test.ts`:
- Line 239: The test description contains a typo: update the test title string
in the test call (the test starting with "Should escape persistent operation
client name before storing to blog storage") to use "blob storage" instead of
"blog storage" so it reads "Should escape persistent operation client name
before storing to blob storage".
In `@proto/wg/cosmo/platform/v1/platform.proto`:
- Around line 1285-1289: The RetirePersistedOperationRequest message currently
uses only operationId which is ambiguous; update the protobuf by adding a client
identity field (e.g., string clientId) to RetirePersistedOperationRequest so the
request can be scoped to a single client's persisted operation, and then update
any server-side handlers (RPC implementations that read
RetirePersistedOperationRequest) and validation logic to require and use
clientId together with operationId to locate and retire the persisted operation.
In `@studio/src/pages/`[organizationSlug]/[namespace]/graph/[slug]/clients.tsx:
- Around line 373-374: The retire button is currently exposed to all users and
lacks a confirmation step; gate its visibility and clickability using the same
access check used by CreateClient (call checkUserAccess before rendering the
Tooltip/retire button) and disable or hide the button when the user lacks
permission, and add a confirmation modal (or browser confirm) that must be
accepted before invoking the retire handler (e.g., the retireOperation /
handleRetire function) to perform the destructive action; ensure the
confirmation state prevents accidental double submissions and that the access
check and modal are wired into the Tooltip/button render path.
- Around line 373-388: The Button currently uses asChild with TrashIcon which
causes the disabled prop and semantics to be lost; change the Button in the
Tooltip/TooltipTrigger block to not use asChild so it renders a real <button>,
move the onClick handler from TrashIcon to the Button (keep
disabled={isPending}), and keep TrashIcon as a plain child element (no onClick)
so keyboard focus, accessibility, and the disabled state work correctly when
calling mutate({ operationId: op.id, namespace, fedGraphName: slug }) from the
Button's onClick.
- Around line 223-238: The onSuccess handler for the
useMutation(retirePersistedOperation) call currently only shows a toast and
doesn't refresh the operations list; update the onSuccess in the useMutation
call to call refetch() after verifying data.response?.code === EnumStatusCode.OK
(so the UI reloads the operations list), and add the missing semicolon after the
closing parenthesis of the useMutation(...) invocation; key symbols:
useMutation, retirePersistedOperation, onSuccess, refetch, toast,
EnumStatusCode.
---
Nitpick comments:
In
`@controlplane/src/core/bufservices/persisted-operation/retirePersistedOperation.ts`:
- Around line 56-75: The response builds an "operation" field using a redundant
ternary even though earlier in retirePersistedOperation (after the early return
when !operation) operation is always truthy; remove the ternary and set
operation unconditionally (e.g., operation: { id: operation.id, operationId:
operation.operationId }) so the response is simpler and avoids unreachable
branches—update the return in retirePersistedOperation accordingly.
- Around line 14-76: The handler retirePersistedOperation currently returns
success without recording an audit trail; after the call to
OperationsRepository.retirePersistedOperation and once operation is confirmed
non-null, emit an audit log entry (following the existing audit pattern used by
other persisted-operation handlers) before returning: include actor info from
authContext, headers from ctx.requestHeader, target info (federatedGraph.id and
req.fedGraphName), the operation identifiers (operation.id and
operation.operationId), and result status EnumStatusCode.OK; place this call
immediately after the operation variable is set and before the final return so
successful retires are recorded.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (12)
connect/src/wg/cosmo/platform/v1/platform-PlatformService_connectquery.tsconnect/src/wg/cosmo/platform/v1/platform_connect.tsconnect/src/wg/cosmo/platform/v1/platform_pb.tscontrolplane/README.mdcontrolplane/package.jsoncontrolplane/src/core/bufservices/PlatformService.tscontrolplane/src/core/bufservices/persisted-operation/publishPersistedOperations.tscontrolplane/src/core/bufservices/persisted-operation/retirePersistedOperation.tscontrolplane/src/core/repositories/OperationsRepository.tscontrolplane/test/persisted-operations.test.tsproto/wg/cosmo/platform/v1/platform.protostudio/src/pages/[organizationSlug]/[namespace]/graph/[slug]/clients.tsx
controlplane/src/core/bufservices/persisted-operation/deletePersistedOperation.ts
Show resolved
Hide resolved
studio/src/pages/[organizationSlug]/[namespace]/graph/[slug]/clients.tsx
Outdated
Show resolved
Hide resolved
studio/src/pages/[organizationSlug]/[namespace]/graph/[slug]/clients.tsx
Outdated
Show resolved
Hide resolved
studio/src/pages/[organizationSlug]/[namespace]/graph/[slug]/clients.tsx
Outdated
Show resolved
Hide resolved
comatory
force-pushed
the
ondrej/eng-8830-controlplane-cli-delete-persistent-operation
branch
from
30f4a7b to
fb26429
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (2)
controlplane/src/core/bufservices/persisted-operation/retirePersistedOperation.ts (1)
51-54: Blob storage cleanup is still missing on retirement.The handler deletes the DB record but leaves the blob at
${organizationId}/${federatedGraph.id}/operations/${clientName}/${operationId}.jsonorphaned.opts.blobStorageis available viaRouterOptionsandBlobStorageexposesdeleteObject().🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@controlplane/src/core/bufservices/persisted-operation/retirePersistedOperation.ts` around lines 51 - 54, The DB delete leaves an orphaned blob—after calling OperationsRepository.retirePersistedOperation, call opts.blobStorage.deleteObject(...) to remove the JSON blob at the same key used to store it (use the template `${federatedGraph.organizationId}/${federatedGraph.id}/operations/${req.clientName}/${req.operationId}.json`); implement this in retirePersistedOperation handler (after the operationsRepo.retirePersistedOperation call), await the delete, and handle/log any errors from deleteObject so failures don't crash the handler.controlplane/test/persisted-operations.test.ts (1)
261-261: Typo: "blog storage" should be "blob storage".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@controlplane/test/persisted-operations.test.ts` at line 261, The test title contains a typo: change "blog storage" to "blob storage" in the test description string for the test named "Should escape persistent operation client name before storing to blog storage" in controlplane/test/persisted-operations.test.ts so it reads "Should escape persistent operation client name before storing to blob storage"; scan for and update any other occurrences of the same typo in related test names or messages (e.g., within the same test function or nearby assertions) to keep wording consistent.
🧹 Nitpick comments (1)
controlplane/src/core/bufservices/persisted-operation/retirePersistedOperation.ts (1)
65-75: Redundant ternary —operationis always truthy here.The
!operationguard on line 56 returns early, sooperationis guaranteed truthy at line 69. The? ... : undefinedbranch is dead code.♻️ Proposed fix
return { response: { code: EnumStatusCode.OK, }, - operation: operation - ? { - id: operation.id, - operationId: operation.operationId, - } - : undefined, + operation: { + id: operation.id, + operationId: operation.operationId, + }, };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@controlplane/src/core/bufservices/persisted-operation/retirePersistedOperation.ts` around lines 65 - 75, The ternary around operation in retirePersistedOperation is dead code because operation is guaranteed truthy earlier; replace the conditional expression with a direct object using operation.id and operation.operationId (i.e., remove the ? ... : undefined and always include the operation object) so the returned shape is simpler and avoids unreachable branches.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@controlplane/src/core/bufservices/persisted-operation/publishPersistedOperations.ts`:
- Around line 105-113: In publishPersistedOperations.ts update the
authorizer.authorize call to use the federatedGraph.targetId field instead of
federatedGraph.id; locate the authorize invocation (the call on
opts.authorizer.authorize with graph: { targetId: ..., targetType:
'federatedGraph' }) and replace federatedGraph.id with federatedGraph.targetId
so it matches other usages like retirePersistedOperation.ts and
updateFederatedGraph.ts.
In
`@controlplane/src/core/bufservices/persisted-operation/retirePersistedOperation.ts`:
- Around line 29-38: The handler in retirePersistedOperation.ts calls
fedRepo.byName(req.fedGraphName, req.namespace) without applying the
DefaultNamespace fallback used in publishPersistedOperations, causing
missing-namespace requests to return ERR_NOT_FOUND; update the code that reads
req.namespace (used when calling FederatedGraphRepository.byName) to apply the
same fallback (req.namespace = req.namespace || DefaultNamespace) before calling
fedRepo.byName so omitted namespaces default correctly.
In `@controlplane/test/persisted-operations.test.ts`:
- Around line 98-118: The test calls SetupTest without enableMultiUsers so
users[TestUser.viewerTimCompanyA] is undefined and changeUserWithSuppliedContext
receives a context missing userId, causing authentication to fail before RBAC is
checked; update the test that calls SetupTest in the 'Should NOT be able to
publish persisted operations in a viewer role' case to pass enableMultiUsers:
true (same pattern as the retirement viewer test) so
users[TestUser.viewerTimCompanyA] is populated and changeUserWithSuppliedContext
exercises the RBAC path used by publishPersistedOperations.
- Around line 351-365: The test accesses
publishPersistedOperations().operations[0].id without verifying the publish call
succeeded or that operations is non-empty; update each call site (the
publishPersistedOperations calls used before retirePersistedOperation) to first
assert the publish response succeeded and that publishOperationsResp.operations
is defined/not-empty (e.g.,
expect(publishOperationsResp.response?.code).toBe(EnumStatusCode.OK) and
expect(publishOperationsResp.operations.length).toBeGreaterThan(0)) or throw a
clear error including publishOperationsResp when empty, then use
publishOperationsResp.operations[0].id for retirePersistedOperation; apply this
check to all three locations mentioned.
---
Duplicate comments:
In
`@controlplane/src/core/bufservices/persisted-operation/retirePersistedOperation.ts`:
- Around line 51-54: The DB delete leaves an orphaned blob—after calling
OperationsRepository.retirePersistedOperation, call
opts.blobStorage.deleteObject(...) to remove the JSON blob at the same key used
to store it (use the template
`${federatedGraph.organizationId}/${federatedGraph.id}/operations/${req.clientName}/${req.operationId}.json`);
implement this in retirePersistedOperation handler (after the
operationsRepo.retirePersistedOperation call), await the delete, and handle/log
any errors from deleteObject so failures don't crash the handler.
In `@controlplane/test/persisted-operations.test.ts`:
- Line 261: The test title contains a typo: change "blog storage" to "blob
storage" in the test description string for the test named "Should escape
persistent operation client name before storing to blog storage" in
controlplane/test/persisted-operations.test.ts so it reads "Should escape
persistent operation client name before storing to blob storage"; scan for and
update any other occurrences of the same typo in related test names or messages
(e.g., within the same test function or nearby assertions) to keep wording
consistent.
---
Nitpick comments:
In
`@controlplane/src/core/bufservices/persisted-operation/retirePersistedOperation.ts`:
- Around line 65-75: The ternary around operation in retirePersistedOperation is
dead code because operation is guaranteed truthy earlier; replace the
conditional expression with a direct object using operation.id and
operation.operationId (i.e., remove the ? ... : undefined and always include the
operation object) so the returned shape is simpler and avoids unreachable
branches.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
controlplane/src/core/bufservices/persisted-operation/publishPersistedOperations.tscontrolplane/src/core/bufservices/persisted-operation/retirePersistedOperation.tscontrolplane/test/persisted-operations.test.ts
controlplane/src/core/bufservices/persisted-operation/publishPersistedOperations.ts
Outdated
Show resolved
Hide resolved
controlplane/src/core/bufservices/persisted-operation/deletePersistedOperation.ts
Show resolved
Hide resolved
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (6)
controlplane/src/core/bufservices/persisted-operation/retirePersistedOperation.ts (2)
60-63: Blob storage not cleaned up after retirement.Already flagged in a previous review: the handler removes the DB record but leaves the blob at
${organizationId}/${federatedGraph.id}/operations/${clientName}/${operationId}.json.opts.blobStorage.deleteObject()should be called after the successful retirement, mirroring howdeleteFederatedGraphusesblobStorage.removeDirectory().🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@controlplane/src/core/bufservices/persisted-operation/retirePersistedOperation.ts` around lines 60 - 63, The retirePersistedOperation handler currently deletes the DB record but doesn’t remove the corresponding blob; after you successfully retire the operation (use the existing OperationsRepository logic that finds operation via getPersistedOperation and calls the retire method), call opts.blobStorage.deleteObject(...) with the key constructed as `${organizationId}/${federatedGraph.id}/operations/${clientName}/${operationId}.json` (use the same organizationId, federatedGraph.id, clientName and operationId available in the function) and only after the DB retire succeeds; mirror deleteFederatedGraph’s pattern (which uses blobStorage.removeDirectory) to ensure blob cleanup and handle/report any blobStorage errors appropriately.
38-39: MissingDefaultNamespacefallback forreq.namespace.Already flagged in a previous review:
fedRepo.byName(req.fedGraphName, req.namespace)will return not-found for the common case wherenamespaceis omitted by the caller, becausepublishPersistedOperationsappliesreq.namespace = req.namespace || DefaultNamespacebefore this call.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@controlplane/src/core/bufservices/persisted-operation/retirePersistedOperation.ts` around lines 38 - 39, The call to fedRepo.byName uses req.namespace directly and will miss the common default; before calling FederatedGraphRepository.byName (fedRepo.byName) ensure req.namespace falls back to DefaultNamespace (the same behavior used in publishPersistedOperations) so the lookup uses req.namespace || DefaultNamespace; update the code path around FederatedGraphRepository instantiation / byName invocation to pass the normalized namespace value.controlplane/test/persisted-operations.test.ts (3)
99-119:⚠️ Potential issue | 🟠 MajorViewer-role publish test still uses the wrong setup — it exercises auth failure, not RBAC.
SetupTestis called withoutenableMultiUsers: true, sousers[TestUser.viewerTimCompanyA]isundefined. Spreadingundefinedyields{}, meaningchangeUserWithSuppliedContextreceives a context with nouserId. TheuserIdguard in the handler fires first and returnsERROR_NOT_AUTHENTICATED— the same code the assertion checks — but the RBAC path is never reached. The expected status code should beERROR_NOT_AUTHORIZEDonce the setup is corrected (same pattern as the viewer retirement test at lines 417–421).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@controlplane/test/persisted-operations.test.ts` around lines 99 - 119, The test is exercising auth failure because SetupTest was called without enableMultiUsers, so users[TestUser.viewerTimCompanyA] is undefined; update the test to call SetupTest({ dbname, chClient, enableMultiUsers: true }) so TestUser.viewerTimCompanyA is populated, keep the changeUserWithSuppliedContext usage, and then assert the publishPersistedOperations response code is EnumStatusCode.ERROR_NOT_AUTHORIZED (not ERROR_NOT_AUTHENTICATED) to verify RBAC denial for the viewer role.
352-366:⚠️ Potential issue | 🟡 MinorPublish response unchecked before accessing
operations[0].id.If
publishPersistedOperationsfails the response code check,operationswill be empty andoperations[0].idwill throw, making the true failure hard to diagnose. The same omission is present at lines 385–390 and 429–434.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@controlplane/test/persisted-operations.test.ts` around lines 352 - 366, The test accesses publishOperationsResp.operations[0].id without asserting the publishPersistedOperations result succeeded; update the test to first assert publishOperationsResp.response?.code === EnumStatusCode.OK (or that publishOperationsResp.operations.length > 0) before using publishOperationsResp.operations[0].id, and apply the same guard/assertion at the other occurrences noted (the blocks around lines 385–390 and 429–434) so retirePersistedOperation is only called with a valid operation id.
262-262:⚠️ Potential issue | 🟡 MinorTypo: "blog storage" should be "blob storage".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@controlplane/test/persisted-operations.test.ts` at line 262, Fix the typo in the test description and any related strings: replace "blog storage" with "blob storage" in the test titled "Should escape persistent operation client name before storing to blog storage" (the test callback passed to test(...)), and update any other occurrences in that test or nearby assertions/messages so they consistently reference "blob storage" instead of "blog storage".proto/wg/cosmo/platform/v1/platform.proto (1)
1285-1290:⚠️ Potential issue | 🟠 Major
RetirePersistedOperationRequestis still missing a client identity field.
operationIdis user-supplied (seePersistedOperation.id) and is not globally unique — it is only unique within a client scope. Every other persisted-operation RPC (GetPersistedOperations,PublishPersistedOperations) carries explicit client identity. Without aclientId/clientNamefield the retirement handler cannot deterministically target a single record when the sameoperationIdstring exists under two different clients in the same federated graph.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@proto/wg/cosmo/platform/v1/platform.proto` around lines 1285 - 1290, RetirePersistedOperationRequest lacks client identity; add a client identifier field (e.g., string clientId = 5; or string clientName = 5;) to the message to match other persisted-operation RPCs, regenerate protobuf artifacts, and update any code that constructs/parses RetirePersistedOperationRequest and the retire handler/query logic (the server-side retirePersistedOperation handler and any DAO/query that currently matches only RetirePersistedOperationRequest.operationId) to include the new clientId when locating the persisted operation so retirements target a single client's record.
🧹 Nitpick comments (1)
controlplane/test/persisted-operations.test.ts (1)
368-399: Graph setup runs under the dev role, which could cause misleading failures.
authenticator.changeUserWithSuppliedContextis called at lines 377–380 beforesetupFederatedGraphat line 382, so graph creation, subgraph publish, and persisted-operation publish all execute as the dev user. If the dev role ever lacks graph-creation rights thesetupFederatedGraphassertions will produce an opaque error rather than a test-design hint. Consider switching to the dev role only after the shared graph setup, consistent with the viewer retirement test pattern (lines 426–439).♻️ Proposed fix
const { client, server, users, authenticator } = await SetupTest({ dbname, chClient, enableMultiUsers: true, }); testContext.onTestFinished(() => server.close()); - authenticator.changeUserWithSuppliedContext({ - ...users[TestUser.devJoeCompanyA]!, - rbac: createTestRBACEvaluator(createTestGroup({ role: 'organization-developer' })), - }); - const fedGraphName = genID('fedGraph'); await setupFederatedGraph(fedGraphName, client); + authenticator.changeUserWithSuppliedContext({ + ...users[TestUser.devJoeCompanyA]!, + rbac: createTestRBACEvaluator(createTestGroup({ role: 'organization-developer' })), + }); + const publishOperationsResp = await client.publishPersistedOperations({🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@controlplane/test/persisted-operations.test.ts` around lines 368 - 399, The test "Should be able to retire a persisted operation in dev role" switches the user context via authenticator.changeUserWithSuppliedContext before calling setupFederatedGraph, which causes graph creation/publish to run as the dev user and can hide permission-related failures; move the authenticator.changeUserWithSuppliedContext call to after setupFederatedGraph (and after publishPersistedOperations), so setupFederatedGraph and initial publishes run under the original privileged context and only the retirePersistedOperation call runs under the developer RBAC context referenced by createTestGroup({ role: 'organization-developer' }).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@controlplane/src/core/bufservices/persisted-operation/retirePersistedOperation.ts`:
- Around line 23-30: The early-return guard that checks !opts.chClient before
authentication incorrectly leaks analytics-enabled state, returns the wrong
shape (includes filters: [] not part of RetirePersistedOperationResponse), and
blocks DB retirement unnecessarily; instead, move authentication to run before
any chClient check, remove the early return that returns filters, and change
logic in retirePersistedOperation (the branch around opts.chClient and the
traffic-safety/metrics check) to treat missing opts.chClient as "no traffic
data" (skip ClickHouse metric validation, log or annotate a warning in the
response) while still proceeding to call OperationsRepository.retire (Postgres)
to perform the retirement; ensure the function returns a valid
RetirePersistedOperationResponse shape in all paths and keeps use of
EnumStatusCode for errors.
In `@controlplane/src/core/repositories/analytics/MetricsRepository.ts`:
- Around line 1348-1366: The query is summing only the first 5-minute bucket;
change the SQL in MetricsRepository to aggregate across buckets by using
SUM(TotalRequests) AS TotalRequests from operation_request_metrics_5_30 (instead
of selecting TotalRequests directly), update the parameter name from graphId to
federatedGraphId to match other methods, call this.client.queryPromise with the
new federatedGraphId param, and when reading the result use a safe optional
access like results[0]?.TotalRequests and coerce to a number (e.g.,
Number(results[0]?.TotalRequests || 0)) before returning totalRequests.
In `@controlplane/test/persisted-operations.test.ts`:
- Around line 456-472: The test accesses publishOperationsResp.operations[0].id
without first verifying the publish succeeded; update both call sites that use
publishPersistedOperations (the variables publishOperationsResp at lines near
the retirePersistedOperation calls) to assert
publishOperationsResp.response?.code === EnumStatusCode.OK (or throw/assert with
a helpful message) before reading operations[0].id, so a publish failure yields
a clear test assertion instead of an unhelpful crash; apply the same check at
the second occurrence as well.
---
Duplicate comments:
In
`@controlplane/src/core/bufservices/persisted-operation/retirePersistedOperation.ts`:
- Around line 60-63: The retirePersistedOperation handler currently deletes the
DB record but doesn’t remove the corresponding blob; after you successfully
retire the operation (use the existing OperationsRepository logic that finds
operation via getPersistedOperation and calls the retire method), call
opts.blobStorage.deleteObject(...) with the key constructed as
`${organizationId}/${federatedGraph.id}/operations/${clientName}/${operationId}.json`
(use the same organizationId, federatedGraph.id, clientName and operationId
available in the function) and only after the DB retire succeeds; mirror
deleteFederatedGraph’s pattern (which uses blobStorage.removeDirectory) to
ensure blob cleanup and handle/report any blobStorage errors appropriately.
- Around line 38-39: The call to fedRepo.byName uses req.namespace directly and
will miss the common default; before calling FederatedGraphRepository.byName
(fedRepo.byName) ensure req.namespace falls back to DefaultNamespace (the same
behavior used in publishPersistedOperations) so the lookup uses req.namespace ||
DefaultNamespace; update the code path around FederatedGraphRepository
instantiation / byName invocation to pass the normalized namespace value.
In `@controlplane/test/persisted-operations.test.ts`:
- Around line 99-119: The test is exercising auth failure because SetupTest was
called without enableMultiUsers, so users[TestUser.viewerTimCompanyA] is
undefined; update the test to call SetupTest({ dbname, chClient,
enableMultiUsers: true }) so TestUser.viewerTimCompanyA is populated, keep the
changeUserWithSuppliedContext usage, and then assert the
publishPersistedOperations response code is EnumStatusCode.ERROR_NOT_AUTHORIZED
(not ERROR_NOT_AUTHENTICATED) to verify RBAC denial for the viewer role.
- Around line 352-366: The test accesses publishOperationsResp.operations[0].id
without asserting the publishPersistedOperations result succeeded; update the
test to first assert publishOperationsResp.response?.code === EnumStatusCode.OK
(or that publishOperationsResp.operations.length > 0) before using
publishOperationsResp.operations[0].id, and apply the same guard/assertion at
the other occurrences noted (the blocks around lines 385–390 and 429–434) so
retirePersistedOperation is only called with a valid operation id.
- Line 262: Fix the typo in the test description and any related strings:
replace "blog storage" with "blob storage" in the test titled "Should escape
persistent operation client name before storing to blog storage" (the test
callback passed to test(...)), and update any other occurrences in that test or
nearby assertions/messages so they consistently reference "blob storage" instead
of "blog storage".
In `@proto/wg/cosmo/platform/v1/platform.proto`:
- Around line 1285-1290: RetirePersistedOperationRequest lacks client identity;
add a client identifier field (e.g., string clientId = 5; or string clientName =
5;) to the message to match other persisted-operation RPCs, regenerate protobuf
artifacts, and update any code that constructs/parses
RetirePersistedOperationRequest and the retire handler/query logic (the
server-side retirePersistedOperation handler and any DAO/query that currently
matches only RetirePersistedOperationRequest.operationId) to include the new
clientId when locating the persisted operation so retirements target a single
client's record.
---
Nitpick comments:
In `@controlplane/test/persisted-operations.test.ts`:
- Around line 368-399: The test "Should be able to retire a persisted operation
in dev role" switches the user context via
authenticator.changeUserWithSuppliedContext before calling setupFederatedGraph,
which causes graph creation/publish to run as the dev user and can hide
permission-related failures; move the
authenticator.changeUserWithSuppliedContext call to after setupFederatedGraph
(and after publishPersistedOperations), so setupFederatedGraph and initial
publishes run under the original privileged context and only the
retirePersistedOperation call runs under the developer RBAC context referenced
by createTestGroup({ role: 'organization-developer' }).
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
connect/src/wg/cosmo/common/common_pb.tsconnect/src/wg/cosmo/platform/v1/platform_pb.tscontrolplane/src/core/bufservices/persisted-operation/retirePersistedOperation.tscontrolplane/src/core/repositories/analytics/MetricsRepository.tscontrolplane/test/persisted-operations.test.tsproto/wg/cosmo/common/common.protoproto/wg/cosmo/platform/v1/platform.proto
controlplane/src/core/bufservices/persisted-operation/deletePersistedOperation.ts
Show resolved
Hide resolved
controlplane/src/core/repositories/analytics/MetricsRepository.ts
Outdated
Show resolved
Hide resolved
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (3)
controlplane/test/persisted-operations.test.ts (3)
262-262:⚠️ Potential issue | 🟡 MinorTypo in test title: “blog storage” → “blob storage”.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@controlplane/test/persisted-operations.test.ts` at line 262, The test title string in the test case 'Should escape persistent operation client name before storing to blog storage' has a typo; change "blog storage" to "blob storage" in that test declaration (the test(...) in persisted-operations.test.ts) so the title reads 'Should escape persistent operation client name before storing to blob storage'.
352-363:⚠️ Potential issue | 🟡 MinorGuard publish success before reading
operations[0].id.These tests access
publishOperationsResp.operations[0].idwithout asserting publish succeeded. If publish fails, you get an unhelpful crash instead of a clear assertion.🛡️ Proposed fix pattern (apply to each call site)
const publishOperationsResp = await client.publishPersistedOperations({ fedGraphName, namespace: 'default', clientName: 'curl', operations: [{ id: genID('hello'), contents: `query { hello }` }], }); + expect(publishOperationsResp.response?.code).toBe(EnumStatusCode.OK); + expect(publishOperationsResp.operations.length).toBeGreaterThan(0); const retireOperationsResp = await client.retirePersistedOperation({ fedGraphName, namespace: 'default', operationId: publishOperationsResp.operations[0].id, });Also applies to: 385-396, 429-445, 456-472, 496-513
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@controlplane/test/persisted-operations.test.ts` around lines 352 - 363, Guard against failed publishes before indexing into publish response arrays: for each call site using publishPersistedOperations and then reading publishOperationsResp.operations[0].id (e.g., the block that calls client.publishPersistedOperations and then client.retirePersistedOperation), first assert the publish succeeded by checking the response status (e.g., a success flag, no errors, and that operations array length > 0) and fail the test with a clear message if not; only after that safe assertion use publishOperationsResp.operations[0].id. Apply the same pattern to all similar sites (lines around the other publish/retire calls).
99-119:⚠️ Potential issue | 🟠 MajorViewer-role publish test is still validating the wrong failure path.
This case is titled as RBAC denial, but setup still does not enable multi-user context, so it can fail as unauthenticated instead of unauthorized. That gives false confidence for role enforcement.
🐛 Proposed fix
- const { client, server, authenticator, users } = await SetupTest({ dbname, chClient }); + const { client, server, authenticator, users } = await SetupTest({ dbname, chClient, enableMultiUsers: true }); @@ - expect(publishOperationsResp.response?.code).toBe(EnumStatusCode.ERROR_NOT_AUTHENTICATED); + expect(publishOperationsResp.response?.code).toBe(EnumStatusCode.ERROR_NOT_AUTHORIZED);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@controlplane/test/persisted-operations.test.ts` around lines 99 - 119, The test is asserting an authentication failure but the test setup doesn't enable multi-user mode so it can fail as unauthenticated; update the SetupTest call to enable multi-user context (e.g., SetupTest({ dbname, chClient, multiUser: true })) so the authenticator.changeUserWithSuppliedContext actually sets the active user, then change the expected assertion from EnumStatusCode.ERROR_NOT_AUTHENTICATED to the RBAC denial code (e.g., EnumStatusCode.ERROR_NOT_AUTHORIZED) after calling client.publishPersistedOperations, keeping references to SetupTest, authenticator.changeUserWithSuppliedContext, createTestRBACEvaluator/createTestGroup, client.publishPersistedOperations and EnumStatusCode.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@controlplane/test/persisted-operations.test.ts`:
- Around line 166-180: The failing tests omit the required clientName so
validation fails early; update both tests that call
client.publishPersistedOperations (the one named "Should not publish persisted
operations with invalid queries" and the similar test for invalid federated
graph) to include a valid clientName property in the request payload (e.g.,
clientName: genID('testClient')) so the request passes initial validation and
exercises the intended checks; ensure the call to
client.publishPersistedOperations({ fedGraphName, namespace: 'default',
operations: [...], clientName }) includes the new field.
---
Duplicate comments:
In `@controlplane/test/persisted-operations.test.ts`:
- Line 262: The test title string in the test case 'Should escape persistent
operation client name before storing to blog storage' has a typo; change "blog
storage" to "blob storage" in that test declaration (the test(...) in
persisted-operations.test.ts) so the title reads 'Should escape persistent
operation client name before storing to blob storage'.
- Around line 352-363: Guard against failed publishes before indexing into
publish response arrays: for each call site using publishPersistedOperations and
then reading publishOperationsResp.operations[0].id (e.g., the block that calls
client.publishPersistedOperations and then client.retirePersistedOperation),
first assert the publish succeeded by checking the response status (e.g., a
success flag, no errors, and that operations array length > 0) and fail the test
with a clear message if not; only after that safe assertion use
publishOperationsResp.operations[0].id. Apply the same pattern to all similar
sites (lines around the other publish/retire calls).
- Around line 99-119: The test is asserting an authentication failure but the
test setup doesn't enable multi-user mode so it can fail as unauthenticated;
update the SetupTest call to enable multi-user context (e.g., SetupTest({
dbname, chClient, multiUser: true })) so the
authenticator.changeUserWithSuppliedContext actually sets the active user, then
change the expected assertion from EnumStatusCode.ERROR_NOT_AUTHENTICATED to the
RBAC denial code (e.g., EnumStatusCode.ERROR_NOT_AUTHORIZED) after calling
client.publishPersistedOperations, keeping references to SetupTest,
authenticator.changeUserWithSuppliedContext,
createTestRBACEvaluator/createTestGroup, client.publishPersistedOperations and
EnumStatusCode.
6141440 to
c1bfaf0
Compare
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (13)
controlplane/README.md (1)
78-78:⚠️ Potential issue | 🟡 MinorGrammar error persists from previous review.
Line 78 still contains the grammar error that was previously flagged: "ensure" should be "ensures" for correct subject-verb agreement.
✏️ Proposed grammar fix
-__Summary: If the frontend ensure that the session endpoint `/v1/auth/session` is called on focus and load. The user might never be logged out again.__ +__Summary: If the frontend ensures that the session endpoint `/v1/auth/session` is called on focus and load, the user might never be logged out again.__🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@controlplane/README.md` at line 78, Fix the grammar in the README sentence: change "If the frontend ensure that the session endpoint `/v1/auth/session` is called on focus and load. The user might never be logged out again." to use correct subject-verb agreement by replacing "ensure" with "ensures" so the sentence reads "If the frontend ensures that the session endpoint `/v1/auth/session` is called on focus and load, the user might never be logged out again."controlplane/src/core/repositories/analytics/MetricsRepository.ts (1)
1348-1368:⚠️ Potential issue | 🔴 CriticalMissing SQL aggregation — query returns per-bucket values instead of total.
The
operation_request_metrics_5_30table contains one row per 5-minute bucket. The current query returns multiple rows for operations with traffic across buckets, but onlyresults[0].TotalRequestsis read — yielding a single bucket's count instead of the aggregate total. This defeats the purpose of the traffic guard for destructive operations.🐛 Proposed fix — aggregate with SUM
const query = ` - SELECT TotalRequests + SELECT sum(TotalRequests) AS TotalRequests FROM ${this.client.database}.operation_request_metrics_5_30 WHERE OrganizationID = {organizationId:String} - AND FederatedGraphID = {graphId:String} + AND FederatedGraphID = {federatedGraphId:String} AND OperationPersistedID = {id:String}`; const results = await this.client.queryPromise<{ TotalRequests: number; }>(query, { id, organizationId, - graphId, + federatedGraphId: graphId, }); - return results.length > 0 - ? { - totalRequests: Number(results[0].TotalRequests), - } - : null; + const total = Number(results[0]?.TotalRequests ?? 0); + return total > 0 ? { totalRequests: total } : null;Also aligns parameter naming to
federatedGraphIdfor consistency with other methods in this repository.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@controlplane/src/core/repositories/analytics/MetricsRepository.ts` around lines 1348 - 1368, The query in MetricsRepository (method reading operation_request_metrics_5_30) returns per-5-minute rows but only reads results[0], so it must aggregate; change the SQL to SUM(TotalRequests) (e.g., SELECT SUM(TotalRequests) AS TotalRequests FROM ... WHERE OrganizationID = {organizationId:String} AND FederatedGraphID = {federatedGraphId:String} AND OperationPersistedID = {id:String}) and update the parameter name passed into this.client.queryPromise to use federatedGraphId instead of graphId so the WHERE clause matches other methods; keep reading Number(results[0].TotalRequests) but it will now be the summed total (or handle null to return null).proto/wg/cosmo/platform/v1/platform.proto (1)
1285-1290:⚠️ Potential issue | 🟠 MajorAdd client scope to retirement request to avoid ambiguous targeting.
operationIdalone is not sufficient to uniquely identify a persisted operation across clients. This can retire the wrong record when IDs overlap between clients.🧩 Proposed API shape update
message RetirePersistedOperationRequest { string fedGraphName = 1; string namespace = 2; string operationId = 3; bool force = 4; + string clientId = 5; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@proto/wg/cosmo/platform/v1/platform.proto` around lines 1285 - 1290, The RetirePersistedOperationRequest message currently only uses operationId (in message RetirePersistedOperationRequest) and risks ambiguous retirement across clients; add a client-scoping field (e.g., string clientId or string clientScope) to RetirePersistedOperationRequest, update any server-side handlers that accept RetirePersistedOperationRequest to require and validate the new client identifier when locating persisted operations, and ensure related client code constructs/populates the new field when sending retire requests so deletion targets are disambiguated by both operationId and client scope.controlplane/test/persisted-operations.test.ts (4)
352-363:⚠️ Potential issue | 🟡 MinorAssert publish success before dereferencing
operations[0]in retirement tests.These paths access
publishOperationsResp.operations[0].idwithout confirming the publish step succeeded, which can produce noisy failures.Also applies to: 385-396, 429-445, 456-472, 496-512
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@controlplane/test/persisted-operations.test.ts` around lines 352 - 363, The test dereferences publishOperationsResp.operations[0].id before verifying the publish succeeded; add an assertion that publishPersistedOperations returned a successful result and that publishOperationsResp.operations is an array with at least one element (e.g., assert publishOperationsResp.success/ok and publishOperationsResp.operations.length > 0) before calling client.retirePersistedOperation. Apply the same guard/assertion pattern for every test block that publishes then retires (the publishPersistedOperations => retirePersistedOperation sequences around the existing dereferences).
173-177:⚠️ Potential issue | 🟠 MajorNegative publish tests omit
clientName, so they can fail before the intended validation.Both tests should include a valid
clientNameto ensure they exercise invalid-query / invalid-graph behavior rather than request-shape validation.Also applies to: 217-221
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@controlplane/test/persisted-operations.test.ts` around lines 173 - 177, The negative tests calling client.publishPersistedOperations currently omit clientName so they can fail on request-shape validation instead of the intended invalid-query/invalid-graph logic; update both calls to client.publishPersistedOperations (the one using genID('hello') at the shown block and the other at the 217-221 block) to include a valid clientName (e.g., clientName: 'test-client') alongside fedGraphName, namespace and operations so the tests exercise invalid-query/invalid-graph behavior rather than failing earlier.
262-262:⚠️ Potential issue | 🟡 MinorFix test title typo: “blog storage” → “blob storage”.
This is minor, but it makes test intent clearer.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@controlplane/test/persisted-operations.test.ts` at line 262, The test title contains a typo: change "blog storage" to "blob storage" in the test named 'Should escape persistent operation client name before storing to blog storage' (the test declaration starting with test('Should escape persistent operation client name before storing to blog storage', ...)). Update the string to read "Should escape persistent operation client name before storing to blob storage" so the intent is clear.
99-119:⚠️ Potential issue | 🟠 MajorViewer-role publish test is exercising unauthenticated path, not RBAC denial.
enableMultiUsersis missing, so viewer context can be undefined and assertion may pass for the wrong reason (ERROR_NOT_AUTHENTICATEDinstead of authorization failure).🐛 Proposed fix
-const { client, server, authenticator, users } = await SetupTest({ dbname, chClient }); +const { client, server, authenticator, users } = await SetupTest({ dbname, chClient, enableMultiUsers: true }); @@ -expect(publishOperationsResp.response?.code).toBe(EnumStatusCode.ERROR_NOT_AUTHENTICATED); +expect(publishOperationsResp.response?.code).toBe(EnumStatusCode.ERROR_NOT_AUTHORIZED);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@controlplane/test/persisted-operations.test.ts` around lines 99 - 119, The test is currently exercising an unauthenticated path because SetupTest is missing enableMultiUsers, so set enableMultiUsers: true when calling SetupTest({ dbname, chClient }) to ensure the viewer context is actually provided; then update the assertion for publishPersistedOperations to expect the RBAC denial status (use the project RBAC error constant, e.g., EnumStatusCode.ERROR_FORBIDDEN or your codebase's RBAC-specific error) instead of ERROR_NOT_AUTHENTICATED so the test verifies authorization denial when using authenticator.changeUserWithSuppliedContext and publishPersistedOperations.controlplane/src/core/bufservices/persisted-operation/retirePersistedOperation.ts (3)
23-30:⚠️ Potential issue | 🔴 CriticalAuthenticate before any analytics capability check; current guard is unsafe and over-restrictive.
This branch runs before auth, returns a non-schema field (
filters), and blocks retirement even though DB retirement does not require ClickHouse.🐛 Proposed fix
- if (!opts.chClient) { - return { - response: { - code: EnumStatusCode.ERR_ANALYTICS_DISABLED, - }, - filters: [], - }; - } const authContext = await opts.authenticator.authenticate(ctx.requestHeader); logger = enrichLogger(ctx, logger, authContext); @@ - const metricsRepository = new MetricsRepository(opts.chClient); - const operationMetrics = req.force - ? null - : await metricsRepository.getPersistedOperationMetrics({ + const operationMetrics = + req.force || !opts.chClient + ? null + : await new MetricsRepository(opts.chClient).getPersistedOperationMetrics({ organizationId: authContext.organizationId, graphId: federatedGraph.id, id: operation.hash, });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@controlplane/src/core/bufservices/persisted-operation/retirePersistedOperation.ts` around lines 23 - 30, The current early return in retirePersistedOperation that checks opts.chClient is misplaced: authentication must run before any analytics capability gating, and DB retirement should not be blocked when ClickHouse client is absent; update retirePersistedOperation to perform auth first, remove or avoid returning the non-schema field "filters" in the error response, and instead return the proper schema error (e.g., only response with code EnumStatusCode.ERR_ANALYTICS_DISABLED or an auth error) so that retirement logic that does not require opts.chClient can proceed when authenticated.
38-40:⚠️ Potential issue | 🟠 MajorApply default namespace fallback before graph lookup.
Missing fallback can return false
ERR_NOT_FOUNDfor requests relying on default namespace behavior.🐛 Proposed fix
+import { DefaultNamespace } from '../../repositories/NamespaceRepository.js'; @@ - const fedRepo = new FederatedGraphRepository(logger, opts.db, authContext.organizationId); - const federatedGraph = await fedRepo.byName(req.fedGraphName, req.namespace); + req.namespace = req.namespace || DefaultNamespace; + const fedRepo = new FederatedGraphRepository(logger, opts.db, authContext.organizationId); + const federatedGraph = await fedRepo.byName(req.fedGraphName, req.namespace);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@controlplane/src/core/bufservices/persisted-operation/retirePersistedOperation.ts` around lines 38 - 40, The lookup calls FederatedGraphRepository.byName with req.namespace directly which can be undefined and cause ERR_NOT_FOUND; before creating/using fedRepo/byName, compute a namespace variable that applies the default namespace fallback (e.g., const namespace = req.namespace ?? DEFAULT_NAMESPACE or use the module's configured default) and then call fedRepo.byName(req.fedGraphName, namespace); update references from req.namespace to that namespace so requests relying on default namespace behavior succeed.
92-106:⚠️ Potential issue | 🟠 MajorRetirement should also clean up blob storage for the retired operation.
The DB row is removed, but the persisted operation blob remains, leaving orphaned objects.
🧹 Proposed fix
const retiredOperation = await operationsRepo.retirePersistedOperation({ operationId: req.operationId, }); + + if (retiredOperation?.filePath) { + try { + await opts.blobStorage.deleteObject({ key: retiredOperation.filePath }); + } catch (error) { + logger.warn({ error, filePath: retiredOperation.filePath }, 'Failed to delete retired operation blob'); + } + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@controlplane/src/core/bufservices/persisted-operation/retirePersistedOperation.ts` around lines 92 - 106, The DB row is removed by operationsRepo.retirePersistedOperation but its persisted blob is left behind; after calling operationsRepo.retirePersistedOperation and receiving retiredOperation, call the blob storage cleanup using the retiredOperation's blob key (e.g., retiredOperation.storageKey / blobKey) to delete the object from blob storage, handle and log any deletion errors (but don't abort the response), and ensure the deletion runs after the DB retire call; update retirePersistedOperation handler to perform that delete and return the same response shape.controlplane/src/core/repositories/OperationsRepository.ts (1)
154-176:⚠️ Potential issue | 🔴 CriticalRetirement delete is still under-scoped and can remove unintended rows.
The lookup is graph-scoped, but delete is
where(operationId = ...)only. This can delete across clients/graphs sharing the same operation ID.🚨 Proposed fix
await this.db .delete(federatedGraphPersistedOperations) - .where(eq(federatedGraphPersistedOperations.operationId, operationId)); + .where(eq(federatedGraphPersistedOperations.id, operationResult.id));🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@controlplane/src/core/repositories/OperationsRepository.ts` around lines 154 - 176, The delete in retirePersistedOperation is under-scoped (only filters by operationId) and can remove rows across graphs; update the delete call on federatedGraphPersistedOperations to include the same federatedGraphId predicate used in the initial lookup (i.e., combine eq(federatedGraphPersistedOperations.operationId, operationId) AND eq(federatedGraphPersistedOperations.federatedGraphId, this.federatedGraphId)), so the deletion is scoped to the current graph and matches the lookup logic.studio/src/pages/[organizationSlug]/[namespace]/graph/[slug]/clients.tsx (2)
228-243:⚠️ Potential issue | 🟠 MajorRefresh persisted operations after a successful retirement.
Success toasts are shown, but the list is not refetched, so retired operations stay visible until manual refresh/navigation.
🐛 Proposed fix
const { mutate, isPending } = useMutation(retirePersistedOperation, { onSuccess(data) { if (data.response?.code !== EnumStatusCode.OK) { toast({ variant: "destructive", title: "Could not retire the operation", description: data.response?.details ?? "Please try again", }); return; } toast({ title: "Operation retired successfully", }); + refetch(); }, }); @@ { - onSuccess: () => setPersistedOperationIdToRetire(null), + onSuccess: () => { + setPersistedOperationIdToRetire(null); + refetch(); + }, }, ); }Also applies to: 550-566
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@studio/src/pages/`[organizationSlug]/[namespace]/graph/[slug]/clients.tsx around lines 228 - 243, The success handler for retirePersistedOperation (inside useMutation where mutate/isPending are declared) currently only shows toasts and does not refresh the persisted-operations list; update the onSuccess callback to trigger a refetch of the persisted operations (for example call the existing refetch function or use your query client to invalidate/refetch the query for the persisted operations query key such as "persistedOperations") so the retired item is removed from the UI; apply the same fix to the other similar mutation block around lines 550-566.
383-414:⚠️ Potential issue | 🔴 CriticalUse a real button element for retire action; current composition breaks disabled/accessibility semantics.
The click handler is attached to
TrashIconandButtonis rendered withasChild, so you lose reliable button behavior.🐛 Proposed fix
-<Button - variant="outline" - size="icon" - asChild - disabled={isPending} -> - <TrashIcon - height={20} - onClick={() => { - mutate(/* ... */); - }} - /> -</Button> +<Button + variant="outline" + size="icon" + disabled={isPending} + onClick={() => { + mutate(/* ... */); + }} +> + <TrashIcon height={20} /> +</Button>#!/bin/bash # Verify local Button implementation and the retire call site wiring. fd button.tsx studio/src/components/ui --exec sed -n '1,180p' {} rg -n -C3 "asChild|TrashIcon|Retire the operation|onClick" 'studio/src/pages/[organizationSlug]/[namespace]/graph/[slug]/clients.tsx'🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@studio/src/pages/`[organizationSlug]/[namespace]/graph/[slug]/clients.tsx around lines 383 - 414, The retire action currently attaches the click handler to TrashIcon while Button is rendered with asChild, breaking native button semantics and disabled/accessibility behavior; move the onClick handler onto the Button (or remove asChild so Button renders its own <button> element) and render TrashIcon purely as a visual child, ensure the Button uses disabled={isPending}, provides an aria-label (e.g., "Retire operation"), and calls mutate with operationId: op.id, namespace and fedGraphName: slug; keep setPersistedOperationIdToRetire and the existing onSuccess logic unchanged.
🧹 Nitpick comments (1)
studio/src/components/clients/retire-persisted-operation-dialog.tsx (1)
42-49: Expose a submitting state and disable the destructive action while pending.This button can be clicked multiple times while the mutation is in-flight. Consider accepting an
isSubmittingprop and disabling the action to prevent duplicate retire attempts.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@studio/src/components/clients/retire-persisted-operation-dialog.tsx` around lines 42 - 49, The Retire button (Button with onClick={onSubmitButtonClick}) allows duplicate clicks while the retire mutation is in-flight; add an isSubmitting boolean prop to the RetirePersistedOperationDialog component, thread it to the button, and disable the destructive action when true (e.g., <Button disabled={isSubmitting} onClick={onSubmitButtonClick}>), optionally adding aria-busy or a loading indicator; ensure callers pass the mutation loading state into isSubmitting so the button is disabled while the mutation runs.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@controlplane/README.md`:
- Line 78: Fix the grammar in the README sentence: change "If the frontend
ensure that the session endpoint `/v1/auth/session` is called on focus and load.
The user might never be logged out again." to use correct subject-verb agreement
by replacing "ensure" with "ensures" so the sentence reads "If the frontend
ensures that the session endpoint `/v1/auth/session` is called on focus and
load, the user might never be logged out again."
In
`@controlplane/src/core/bufservices/persisted-operation/retirePersistedOperation.ts`:
- Around line 23-30: The current early return in retirePersistedOperation that
checks opts.chClient is misplaced: authentication must run before any analytics
capability gating, and DB retirement should not be blocked when ClickHouse
client is absent; update retirePersistedOperation to perform auth first, remove
or avoid returning the non-schema field "filters" in the error response, and
instead return the proper schema error (e.g., only response with code
EnumStatusCode.ERR_ANALYTICS_DISABLED or an auth error) so that retirement logic
that does not require opts.chClient can proceed when authenticated.
- Around line 38-40: The lookup calls FederatedGraphRepository.byName with
req.namespace directly which can be undefined and cause ERR_NOT_FOUND; before
creating/using fedRepo/byName, compute a namespace variable that applies the
default namespace fallback (e.g., const namespace = req.namespace ??
DEFAULT_NAMESPACE or use the module's configured default) and then call
fedRepo.byName(req.fedGraphName, namespace); update references from
req.namespace to that namespace so requests relying on default namespace
behavior succeed.
- Around line 92-106: The DB row is removed by
operationsRepo.retirePersistedOperation but its persisted blob is left behind;
after calling operationsRepo.retirePersistedOperation and receiving
retiredOperation, call the blob storage cleanup using the retiredOperation's
blob key (e.g., retiredOperation.storageKey / blobKey) to delete the object from
blob storage, handle and log any deletion errors (but don't abort the response),
and ensure the deletion runs after the DB retire call; update
retirePersistedOperation handler to perform that delete and return the same
response shape.
In `@controlplane/src/core/repositories/analytics/MetricsRepository.ts`:
- Around line 1348-1368: The query in MetricsRepository (method reading
operation_request_metrics_5_30) returns per-5-minute rows but only reads
results[0], so it must aggregate; change the SQL to SUM(TotalRequests) (e.g.,
SELECT SUM(TotalRequests) AS TotalRequests FROM ... WHERE OrganizationID =
{organizationId:String} AND FederatedGraphID = {federatedGraphId:String} AND
OperationPersistedID = {id:String}) and update the parameter name passed into
this.client.queryPromise to use federatedGraphId instead of graphId so the WHERE
clause matches other methods; keep reading Number(results[0].TotalRequests) but
it will now be the summed total (or handle null to return null).
In `@controlplane/src/core/repositories/OperationsRepository.ts`:
- Around line 154-176: The delete in retirePersistedOperation is under-scoped
(only filters by operationId) and can remove rows across graphs; update the
delete call on federatedGraphPersistedOperations to include the same
federatedGraphId predicate used in the initial lookup (i.e., combine
eq(federatedGraphPersistedOperations.operationId, operationId) AND
eq(federatedGraphPersistedOperations.federatedGraphId, this.federatedGraphId)),
so the deletion is scoped to the current graph and matches the lookup logic.
In `@controlplane/test/persisted-operations.test.ts`:
- Around line 352-363: The test dereferences
publishOperationsResp.operations[0].id before verifying the publish succeeded;
add an assertion that publishPersistedOperations returned a successful result
and that publishOperationsResp.operations is an array with at least one element
(e.g., assert publishOperationsResp.success/ok and
publishOperationsResp.operations.length > 0) before calling
client.retirePersistedOperation. Apply the same guard/assertion pattern for
every test block that publishes then retires (the publishPersistedOperations =>
retirePersistedOperation sequences around the existing dereferences).
- Around line 173-177: The negative tests calling
client.publishPersistedOperations currently omit clientName so they can fail on
request-shape validation instead of the intended invalid-query/invalid-graph
logic; update both calls to client.publishPersistedOperations (the one using
genID('hello') at the shown block and the other at the 217-221 block) to include
a valid clientName (e.g., clientName: 'test-client') alongside fedGraphName,
namespace and operations so the tests exercise invalid-query/invalid-graph
behavior rather than failing earlier.
- Line 262: The test title contains a typo: change "blog storage" to "blob
storage" in the test named 'Should escape persistent operation client name
before storing to blog storage' (the test declaration starting with test('Should
escape persistent operation client name before storing to blog storage', ...)).
Update the string to read "Should escape persistent operation client name before
storing to blob storage" so the intent is clear.
- Around line 99-119: The test is currently exercising an unauthenticated path
because SetupTest is missing enableMultiUsers, so set enableMultiUsers: true
when calling SetupTest({ dbname, chClient }) to ensure the viewer context is
actually provided; then update the assertion for publishPersistedOperations to
expect the RBAC denial status (use the project RBAC error constant, e.g.,
EnumStatusCode.ERROR_FORBIDDEN or your codebase's RBAC-specific error) instead
of ERROR_NOT_AUTHENTICATED so the test verifies authorization denial when using
authenticator.changeUserWithSuppliedContext and publishPersistedOperations.
In `@proto/wg/cosmo/platform/v1/platform.proto`:
- Around line 1285-1290: The RetirePersistedOperationRequest message currently
only uses operationId (in message RetirePersistedOperationRequest) and risks
ambiguous retirement across clients; add a client-scoping field (e.g., string
clientId or string clientScope) to RetirePersistedOperationRequest, update any
server-side handlers that accept RetirePersistedOperationRequest to require and
validate the new client identifier when locating persisted operations, and
ensure related client code constructs/populates the new field when sending
retire requests so deletion targets are disambiguated by both operationId and
client scope.
In `@studio/src/pages/`[organizationSlug]/[namespace]/graph/[slug]/clients.tsx:
- Around line 228-243: The success handler for retirePersistedOperation (inside
useMutation where mutate/isPending are declared) currently only shows toasts and
does not refresh the persisted-operations list; update the onSuccess callback to
trigger a refetch of the persisted operations (for example call the existing
refetch function or use your query client to invalidate/refetch the query for
the persisted operations query key such as "persistedOperations") so the retired
item is removed from the UI; apply the same fix to the other similar mutation
block around lines 550-566.
- Around line 383-414: The retire action currently attaches the click handler to
TrashIcon while Button is rendered with asChild, breaking native button
semantics and disabled/accessibility behavior; move the onClick handler onto the
Button (or remove asChild so Button renders its own <button> element) and render
TrashIcon purely as a visual child, ensure the Button uses disabled={isPending},
provides an aria-label (e.g., "Retire operation"), and calls mutate with
operationId: op.id, namespace and fedGraphName: slug; keep
setPersistedOperationIdToRetire and the existing onSuccess logic unchanged.
---
Nitpick comments:
In `@studio/src/components/clients/retire-persisted-operation-dialog.tsx`:
- Around line 42-49: The Retire button (Button with
onClick={onSubmitButtonClick}) allows duplicate clicks while the retire mutation
is in-flight; add an isSubmitting boolean prop to the
RetirePersistedOperationDialog component, thread it to the button, and disable
the destructive action when true (e.g., <Button disabled={isSubmitting}
onClick={onSubmitButtonClick}>), optionally adding aria-busy or a loading
indicator; ensure callers pass the mutation loading state into isSubmitting so
the button is disabled while the mutation runs.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (16)
connect/src/wg/cosmo/common/common_pb.tsconnect/src/wg/cosmo/platform/v1/platform-PlatformService_connectquery.tsconnect/src/wg/cosmo/platform/v1/platform_connect.tsconnect/src/wg/cosmo/platform/v1/platform_pb.tscontrolplane/README.mdcontrolplane/package.jsoncontrolplane/src/core/bufservices/PlatformService.tscontrolplane/src/core/bufservices/persisted-operation/publishPersistedOperations.tscontrolplane/src/core/bufservices/persisted-operation/retirePersistedOperation.tscontrolplane/src/core/repositories/OperationsRepository.tscontrolplane/src/core/repositories/analytics/MetricsRepository.tscontrolplane/test/persisted-operations.test.tsproto/wg/cosmo/common/common.protoproto/wg/cosmo/platform/v1/platform.protostudio/src/components/clients/retire-persisted-operation-dialog.tsxstudio/src/pages/[organizationSlug]/[namespace]/graph/[slug]/clients.tsx
🚧 Files skipped from review as they are similar to previous changes (4)
- controlplane/src/core/bufservices/PlatformService.ts
- connect/src/wg/cosmo/platform/v1/platform-PlatformService_connectquery.ts
- connect/src/wg/cosmo/platform/v1/platform_connect.ts
- controlplane/package.json
comatory
force-pushed
the
ondrej/eng-8830-controlplane-cli-delete-persistent-operation
branch
3 times, most recently
from
4fc1479 to
3bf7236
Compare
comatory
changed the title
comatory
changed the title
comatory
force-pushed
the
ondrej/eng-8830-controlplane-cli-delete-persistent-operation
branch
from
ca4e34f to
77f031b
Compare
comatory
force-pushed
the
ondrej/eng-8830-controlplane-cli-delete-persistent-operation
branch
from
77f031b to
b4213c8
Compare
comatory
force-pushed
the
ondrej/eng-8830-controlplane-cli-delete-persistent-operation
branch
from
b4213c8 to
3c709d4
Compare
comatory
added a commit
to wundergraph/cosmo-docs
that referenced
this pull request
Feb 27, 2026
Contributor
Author
|
@StarpTech ready for re-review |
Contributor
Author
|
Docs are here btw wundergraph/cosmo-docs#255 |
comatory
changed the title
Contributor
Author
|
@StarpTech please re-review, I believe you wanted this one addressed? |
StarpTech
requested changes
Feb 27, 2026
Contributor
Author
|
@StarpTech copy fixed |
I did not like the idea of instantiating the class inside of itself, that could have strange side-effects in future, let's just swap the `db` propery.
Intended to be fixed in `ENG-9057`. The vulnerability happens on macOS/Darwin, as most images are used on Linux machines, we're ok with making this compromise. This was discussed with the team and OK'ed.
comatory
deleted the
ondrej/eng-8830-controlplane-cli-delete-persistent-operation
branch
comatory
added a commit
to wundergraph/cosmo-docs
that referenced
this pull request
Mar 2, 2026
* feat(studio): documentation for retiring persisted operations wundergraph/cosmo#2553 * Update docs/router/persisted-queries/persisted-operations.mdx Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com> * fix: adjust naming, use delete instead of retire * fix: make language similar to studio dialog --------- Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
5 tasks
This was referenced Mar 30, 2026
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.
@coderabbitai summary
Checklist
Adds functionality to delete persisted operations via Studio. The persisted operations are
uploaded via CLI first, only then they can be visible in the Studio.
Organization admins & developers can perform this action.
Note: The video displays "retire..." but now we're using delete instead
Screen.Recording.2026-02-27.at.9.04.02.mov
Testing
Try these as well if you have time:
query ...definitions in one persisted operation file, UI should be able to handle large volume of them in a single operationConsiderations
next operation
Note
We believe the limitations above will be a non-issue, once we have support for retiring
the operations via CLI. Bulk retirement should then be possible via scripting, or any
other kind of automations.
We intend to provide ability to override warnings (if operation has traffic) via a flag.