Open
Conversation
Contributor
|
Preview deployment for your docs. Learn more about Mintlify Previews.
💡 Tip: Enable Workflows to automatically generate PRs for you. |
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 new Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
Router image scan passed✅ No security vulnerabilities found in image: |
comatory
force-pushed
the
ondrej/eng-7122-delete-client
branch
from
3730a28 to
6c304eb
Compare
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #2795 +/- ##
==========================================
- Coverage 41.70% 41.15% -0.56%
==========================================
Files 791 995 +204
Lines 113325 124710 +11385
Branches 8768 5752 -3016
==========================================
+ Hits 47266 51319 +4053
- Misses 65695 71692 +5997
- Partials 364 1699 +1335
🚀 New features to boost your workflow:
|
comatory
changed the title
![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: 5
🧹 Nitpick comments (5)
controlplane/test/persisted-operations.test.ts (2)
636-783: Add direct error-path coverage fordeleteClient.The new suite exercises auth/not-found on
previewDeleteClient, but the destructive RPC itself is only covered for success and blob-storage failure. SincedeleteClienthas its own handler, please add at least unauthorized and missing-client assertions here so regressions are not masked by the preview step.🤖 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 636 - 783, Add tests to cover error paths for the actual deleteClient RPC (not only previewDeleteClient): call client.deleteClient with an unauthorized context (e.g., missing/invalid credentials or a user without permission) and assert the response code is ERR and message indicates unauthorized, and call client.deleteClient for a non-existent client name and assert it returns NOT_FOUND (or appropriate error) and that no side-effects occur (clients list unchanged, blobStorage keys unchanged). Place these assertions alongside the existing deleteClient tests and reuse SetupTest, genID, and getClients to locate and verify state; reference client.deleteClient, client.getClients, and blobStorage.removeDirectory to identify the code under test.
676-693: Avoid depending ondeletedOperationsorder.This assertion hardcodes the returned array order. Unless the handler guarantees a stable sort, this will make the test flaky.
♻️ Suggested change
- expect( - deleteResp.deletedOperations.map((op) => ({ - id: op.id, - operationId: op.operationId, - operationNames: op.operationNames, - })), - ).toStrictEqual([ + expect( + deleteResp.deletedOperations + .map((op) => ({ + id: op.id, + operationId: op.operationId, + operationNames: op.operationNames, + })) + .sort((left, right) => left.operationId.localeCompare(right.operationId)), + ).toStrictEqual([ { id: expect.any(String), operationId: helloOperationId, operationNames: [], }, { id: expect.any(String), operationId: typenameOperationId, operationNames: [], }, - ]); + ].sort((left, right) => left.operationId.localeCompare(right.operationId)));🤖 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 676 - 693, The test currently assumes a stable order of deleteResp.deletedOperations which makes it flaky; update the assertion to be order-insensitive by either transforming and sorting deleteResp.deletedOperations by a stable key (e.g., operationId) before comparing, or use an order-agnostic matcher such as expect.arrayContaining with objects that reference helloOperationId and typenameOperationId; operate on deleteResp.deletedOperations (and map to {id, operationId, operationNames}) and compare against the expected items without relying on array order.cli/test/clients-list.test.ts (1)
58-68: Restore the global spies after each test.
console.log,process.stderr.write, andprocess.exitstay mocked across cases here. Withoutvi.restoreAllMocks()(or per-spymockRestore()), this suite depends on global Vitest reset config and can become order-dependent.Suggested fix
afterEach(() => { process.exitCode = undefined; + vi.restoreAllMocks(); });Also applies to: 140-150
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cli/test/clients-list.test.ts` around lines 58 - 68, The test suite leaves console.log, process.stderr.write and process.exit mocked (via logSpy, stderrSpy, exitSpy) which can leak between tests; restore those spies in afterEach by calling vi.restoreAllMocks() or mockRestore() on each spy (logSpy.mockRestore(), stderrSpy.mockRestore(), exitSpy.mockRestore()) so the global environment is returned to normal; update the afterEach block that currently only resets process.exitCode to also restore the mocks used in beforeEach for the tests around clients-list.test.ts (and similarly at lines ~140-150).cli/test/clients-delete.test.ts (2)
24-25: Replaceanyin request callbacks with concrete request types (orunknown).These callback signatures currently bypass type-safety and make test helpers permissive in ways that can hide request-shape regressions.
Proposed typed callback shape
+interface ClientDeleteRequestShape { + namespace?: string; +} + function createMockTransport( previewResponse: PartialMessage<PreviewDeleteClientResponse>, deleteResponse: PartialMessage<DeleteClientResponse>, - onPreviewDeleteClient?: (req: any) => void, - onDeleteClient?: (req: any) => void, + onPreviewDeleteClient?: (req: ClientDeleteRequestShape) => void, + onDeleteClient?: (req: ClientDeleteRequestShape) => void, ) {As per coding guidelines: "Avoid
anytype in TypeScript; use specific types or generics."Also applies to: 45-46
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cli/test/clients-delete.test.ts` around lines 24 - 25, The test helper callback signatures onPreviewDeleteClient and onDeleteClient currently use the unsafe type `any`; update them to use concrete request types (or `unknown`) to restore type-safety—replace the `any` parameter types in the function signatures for onPreviewDeleteClient and onDeleteClient with the appropriate request interface used by the delete flow (or `unknown` if a shared interface isn't available), and apply the same change to the other occurrences noted (the callbacks at the second pair on lines 45-46) so the tests will catch request-shape regressions.
21-39: Add explicit return types to helper functions.
createMockTransportandgetJsonOutputcurrently rely on inferred return types. Making these explicit improves maintainability and aligns with the repository TS rules.Suggested return type annotations
function createMockTransport( previewResponse: PartialMessage<PreviewDeleteClientResponse>, deleteResponse: PartialMessage<DeleteClientResponse>, onPreviewDeleteClient?: (req: ClientDeleteRequestShape) => void, onDeleteClient?: (req: ClientDeleteRequestShape) => void, -) { +): ReturnType<typeof createRouterTransport> { return createRouterTransport(({ service }) => { service(PlatformService, { previewDeleteClient: (req) => { onPreviewDeleteClient?.(req); return previewResponse; @@ -function getJsonOutput(logSpy: MockInstance<typeof console.log>) { +function getJsonOutput(logSpy: MockInstance<typeof console.log>): unknown {As per coding guidelines: "Use explicit type annotations for function parameters and return types in TypeScript."
Also applies to: 68-83
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cli/test/clients-delete.test.ts` around lines 21 - 39, Add explicit TypeScript return annotations for the helper functions: annotate createMockTransport to return ReturnType<typeof createRouterTransport> (so the transport type is explicit) and annotate getJsonOutput to return Promise<unknown> (or a more specific Promise<T> if you know the JSON shape); update the function signatures (createMockTransport and getJsonOutput) accordingly and ensure any parameter types remain explicit to satisfy the repository's TS rules.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@cli/src/commands/clients/commands/delete.ts`:
- Around line 39-127: The previewDeleteClient and deleteClient RPC calls invoked
inside command.action can throw before response.code exists; wrap each call (the
calls to opts.client.platform.previewDeleteClient and
opts.client.platform.deleteClient) in try-catch blocks, catch any thrown error,
and convert it to the same CLI error path: if options.json produce
createJsonErrorOutput (use EnumStatusCode.ERR and include
error.message/details), console.log(JSON.stringify(...)), set process.exitCode =
1 and return; otherwise log the error detail via console.log(pc.red(...)) and
call program.error(pc.red('Could not delete client.')) so failures are
normalized to the same behavior as non-exception responses (references:
command.action, previewResp, resp, createJsonErrorOutput, program.error,
options.json).
In `@cli/src/commands/clients/commands/list.ts`:
- Around line 29-80: Wrap the call to opts.client.platform.getClients inside a
try-catch within the command.action handler (around where getClients is called
in the anonymous async callback) so any thrown RPC/transport errors are caught;
in the catch block, produce the same JSON error shape when options.json is true
(using createJsonErrorOutput and logging JSON) and otherwise call
program.error(pc.red('Could not fetch clients.')) or
console.log(pc.red(error.message)) and set process.exitCode = 1 to preserve
existing exit behavior. Ensure the rest of the logic that inspects resp.response
only runs after a successful await of getClients.
In `@controlplane/src/core/bufservices/persisted-operation/deleteClient.ts`:
- Around line 60-76: The code calls opts.blobStorage.removeDirectory({ key:
clientDirectory }) unconditionally which can fail if there are no persisted
operations; update the logic in the delete client flow (where clientDirectory
and removeDirectory are used) to first check preview.persistedOperationsCount
(or the equivalent preview object) and skip calling
opts.blobStorage.removeDirectory when persistedOperationsCount === 0, ensuring
the DB delete still proceeds and the same success/error response shape is
returned when deletion is skipped; keep the existing try/catch around
removeDirectory for the non-zero case so errors from actual removals are still
handled and logged.
In `@controlplane/src/core/repositories/OperationsRepository.ts`:
- Around line 371-425: The current transaction in OperationsRepository selects
the client and then queries federatedGraphPersistedOperations before deleting
the client, which allows a race where new child operations are inserted and then
cascaded away but not captured; to fix, within the same transaction either
acquire a row lock on the client (SELECT ... FOR UPDATE via
tx.query.federatedGraphClients.findFirst with a locking option or equivalent)
right after finding the client (use federatedGraphClients,
this.federatedGraphId, clientName, client.id to locate it) before enumerating
children, or instead delete the child rows explicitly using
tx.delete(federatedGraphPersistedOperations).where(...matching federatedGraphId
and client.id).returning(...) to capture deletedOperations and
deletedOperationsCount, then delete the federatedGraphClients row; ensure all
operations remain inside the existing transaction so the snapshot accurately
reflects deleted children.
In `@proto/wg/cosmo/platform/v1/platform.proto`:
- Around line 2151-2168: Add explicit confirmation/precondition fields to the
DeleteClientRequest so the server can verify caller intent: add a bool force
flag and an int32 expected_deleted_operations_count (or
expectedDeletedOperationsCount) and/or repeated string expected_operation_ids to
DeleteClientRequest, then change the DeleteClient RPC handler to validate these
fields (compare expected count/ids to what will be deleted and reject if
mismatched unless force==true). Update any callers/CLI and the
PreviewDeleteClient flow to populate these new fields when performing the final
DeleteClient call so the server can enforce the safety check.
---
Nitpick comments:
In `@cli/test/clients-delete.test.ts`:
- Around line 24-25: The test helper callback signatures onPreviewDeleteClient
and onDeleteClient currently use the unsafe type `any`; update them to use
concrete request types (or `unknown`) to restore type-safety—replace the `any`
parameter types in the function signatures for onPreviewDeleteClient and
onDeleteClient with the appropriate request interface used by the delete flow
(or `unknown` if a shared interface isn't available), and apply the same change
to the other occurrences noted (the callbacks at the second pair on lines 45-46)
so the tests will catch request-shape regressions.
- Around line 21-39: Add explicit TypeScript return annotations for the helper
functions: annotate createMockTransport to return ReturnType<typeof
createRouterTransport> (so the transport type is explicit) and annotate
getJsonOutput to return Promise<unknown> (or a more specific Promise<T> if you
know the JSON shape); update the function signatures (createMockTransport and
getJsonOutput) accordingly and ensure any parameter types remain explicit to
satisfy the repository's TS rules.
In `@cli/test/clients-list.test.ts`:
- Around line 58-68: The test suite leaves console.log, process.stderr.write and
process.exit mocked (via logSpy, stderrSpy, exitSpy) which can leak between
tests; restore those spies in afterEach by calling vi.restoreAllMocks() or
mockRestore() on each spy (logSpy.mockRestore(), stderrSpy.mockRestore(),
exitSpy.mockRestore()) so the global environment is returned to normal; update
the afterEach block that currently only resets process.exitCode to also restore
the mocks used in beforeEach for the tests around clients-list.test.ts (and
similarly at lines ~140-150).
In `@controlplane/test/persisted-operations.test.ts`:
- Around line 636-783: Add tests to cover error paths for the actual
deleteClient RPC (not only previewDeleteClient): call client.deleteClient with
an unauthorized context (e.g., missing/invalid credentials or a user without
permission) and assert the response code is ERR and message indicates
unauthorized, and call client.deleteClient for a non-existent client name and
assert it returns NOT_FOUND (or appropriate error) and that no side-effects
occur (clients list unchanged, blobStorage keys unchanged). Place these
assertions alongside the existing deleteClient tests and reuse SetupTest, genID,
and getClients to locate and verify state; reference client.deleteClient,
client.getClients, and blobStorage.removeDirectory to identify the code under
test.
- Around line 676-693: The test currently assumes a stable order of
deleteResp.deletedOperations which makes it flaky; update the assertion to be
order-insensitive by either transforming and sorting
deleteResp.deletedOperations by a stable key (e.g., operationId) before
comparing, or use an order-agnostic matcher such as expect.arrayContaining with
objects that reference helloOperationId and typenameOperationId; operate on
deleteResp.deletedOperations (and map to {id, operationId, operationNames}) and
compare against the expected items without relying on array order.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 87c4cd8c-c02a-4b2a-9275-f2f898eddfbf
⛔ Files ignored due to path filters (2)
connect-go/gen/proto/wg/cosmo/platform/v1/platform.pb.gois excluded by!**/*.pb.go,!**/gen/**connect-go/gen/proto/wg/cosmo/platform/v1/platformv1connect/platform.connect.gois excluded by!**/gen/**
📒 Files selected for processing (19)
cli/src/commands/clients/commands/delete.tscli/src/commands/clients/commands/list.tscli/src/commands/clients/index.tscli/src/commands/index.tscli/test/clients-delete.test.tscli/test/clients-list.test.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/src/core/bufservices/PlatformService.tscontrolplane/src/core/bufservices/persisted-operation/deleteClient.tscontrolplane/src/core/bufservices/persisted-operation/previewDeleteClient.tscontrolplane/src/core/repositories/OperationsRepository.tscontrolplane/test/persisted-operations.test.tsdocs-website/cli/clients.mdxdocs-website/cli/clients/delete.mdxdocs-website/cli/clients/list.mdxdocs-website/docs.jsonproto/wg/cosmo/platform/v1/platform.proto
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (3)
cli/src/commands/clients/commands/delete.ts (1)
39-49:⚠️ Potential issue | 🟠 MajorNormalize thrown RPC failures.
Both platform calls can reject before a
response.codeexists. When that happens, this command bypasses its JSON error envelope andprogram.error()path, so callers get a raw exception instead of the documented CLI result shape.As per coding guidelines "Add proper error handling with try-catch blocks".
Also applies to: 91-100
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cli/src/commands/clients/commands/delete.ts` around lines 39 - 49, The previewDeleteClient and deleteClient RPC calls (invocations of opts.client.platform.previewDeleteClient and opts.client.platform.deleteClient) can throw before a response.code exists; wrap each call in try-catch and normalize any thrown error into the CLI's JSON error envelope and invoke program.error(...) with a structured object (include a code, message, and any available details) so callers never get a raw exception; ensure you still handle the normal response.code branch but on catch extract whatever information is available from the thrown error (message, status/code if present) and forward that through program.error to keep the documented CLI result shape.controlplane/src/core/bufservices/persisted-operation/deleteClient.ts (1)
60-76:⚠️ Potential issue | 🟠 MajorSkip blob cleanup when there is nothing to delete.
preview.persistedOperationsCountis already available, but line 63 still callsremoveDirectory()for zero-operation clients. Storage backends that treat a missing prefix as an error will reject a valid client delete here.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@controlplane/src/core/bufservices/persisted-operation/deleteClient.ts` around lines 60 - 76, The code unconditionally calls opts.blobStorage.removeDirectory for clientDirectory even when there are zero persisted operations; change deleteClient to check preview.persistedOperationsCount (or preview?.persistedOperationsCount) before calling opts.blobStorage.removeDirectory and skip the blob cleanup when the count is 0 (return the normal success response or proceed without calling removeDirectory). Locate the block around clientDirectory and the try/catch that invokes opts.blobStorage.removeDirectory and gate that call with an if (preview.persistedOperationsCount > 0) so storage backends that error on missing prefixes are not invoked for empty clients.controlplane/src/core/repositories/OperationsRepository.ts (1)
371-425:⚠️ Potential issue | 🟠 MajorCapture deleted operations atomically.
deletedOperationsare read before the parent client row is deleted. A concurrent publish in that window can be cascaded away by the final delete and still be missing fromdeletedOperationsCountanddeletedOperations. Lock the client row first, or delete child rows withRETURNINGinside the same transaction before removing the client.🤖 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 371 - 425, The code reads child rows into deletedOperations before deleting the client, allowing a race where a concurrent insert could be cascaded away by the client delete and not be reflected in deletedOperations; fix by performing the child-row removal and capture atomically inside the same transaction (either lock the client row first or delete children with RETURNING). Concretely: within the existing transaction block that references federatedGraphClients and federatedGraphPersistedOperations, either re-query the client using a FOR UPDATE/row-lock (so no concurrent publishes can insert children) or replace the separate select with a single delete on federatedGraphPersistedOperations using RETURNING to collect deleted rows (id, operationId, operationNames) and count, then delete the federatedGraphClients row; return the captured returned rows and count (use the same variables client and deletedOperations names to minimize changes).
🤖 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/deleteClient.ts`:
- Around line 91-103: When regenerate manifest fails in the deleteClient flow,
do not return success; instead make deleteClient surface an error so callers see
the inconsistency. Change the catch for
operationsRepo.generateAndUploadManifest({...}) to propagate/throw a non-OK
error (or return an RPC error response) instead of only logging: include context
(req.clientName, federatedGraph.id, authContext.organizationId) in the thrown
error or returned status so the RPC fails when generateAndUploadManifest()
fails. Ensure the change affects the same function handling the delete (the code
that calls operationsRepo.generateAndUploadManifest and currently logs via
logger.error) so the operation is only considered successful if manifest upload
completes.
- Around line 60-78: The blob removal currently runs before the DB delete
(removeDirectory() executed prior to operationsRepo.deleteClient()), which can
leave the DB row deleted without blobs if the DB delete later fails; change the
flow so the DB delete (operationsRepo.deleteClient) is performed and confirmed
committed before removing blobs: call
operationsRepo.deleteClient(req.clientName) inside its transactional path, only
after a successful commit attempt removeDirectory({ key: clientDirectory }), and
if removeDirectory fails implement a compensating/retry mechanism (e.g., enqueue
a background retry job or mark a cleanup-needed record) instead of returning an
error to avoid leaving inconsistent state; ensure error handling/logging
references removeDirectory and operationsRepo.deleteClient so it's clear which
step failed.
---
Duplicate comments:
In `@cli/src/commands/clients/commands/delete.ts`:
- Around line 39-49: The previewDeleteClient and deleteClient RPC calls
(invocations of opts.client.platform.previewDeleteClient and
opts.client.platform.deleteClient) can throw before a response.code exists; wrap
each call in try-catch and normalize any thrown error into the CLI's JSON error
envelope and invoke program.error(...) with a structured object (include a code,
message, and any available details) so callers never get a raw exception; ensure
you still handle the normal response.code branch but on catch extract whatever
information is available from the thrown error (message, status/code if present)
and forward that through program.error to keep the documented CLI result shape.
In `@controlplane/src/core/bufservices/persisted-operation/deleteClient.ts`:
- Around line 60-76: The code unconditionally calls
opts.blobStorage.removeDirectory for clientDirectory even when there are zero
persisted operations; change deleteClient to check
preview.persistedOperationsCount (or preview?.persistedOperationsCount) before
calling opts.blobStorage.removeDirectory and skip the blob cleanup when the
count is 0 (return the normal success response or proceed without calling
removeDirectory). Locate the block around clientDirectory and the try/catch that
invokes opts.blobStorage.removeDirectory and gate that call with an if
(preview.persistedOperationsCount > 0) so storage backends that error on missing
prefixes are not invoked for empty clients.
In `@controlplane/src/core/repositories/OperationsRepository.ts`:
- Around line 371-425: The code reads child rows into deletedOperations before
deleting the client, allowing a race where a concurrent insert could be cascaded
away by the client delete and not be reflected in deletedOperations; fix by
performing the child-row removal and capture atomically inside the same
transaction (either lock the client row first or delete children with
RETURNING). Concretely: within the existing transaction block that references
federatedGraphClients and federatedGraphPersistedOperations, either re-query the
client using a FOR UPDATE/row-lock (so no concurrent publishes can insert
children) or replace the separate select with a single delete on
federatedGraphPersistedOperations using RETURNING to collect deleted rows (id,
operationId, operationNames) and count, then delete the federatedGraphClients
row; return the captured returned rows and count (use the same variables client
and deletedOperations names to minimize changes).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 67ee1d7c-d8bd-4bac-8370-cf7a0e5b2e19
⛔ Files ignored due to path filters (2)
connect-go/gen/proto/wg/cosmo/platform/v1/platform.pb.gois excluded by!**/*.pb.go,!**/gen/**connect-go/gen/proto/wg/cosmo/platform/v1/platformv1connect/platform.connect.gois excluded by!**/gen/**
📒 Files selected for processing (15)
cli/src/commands/clients/commands/delete.tscli/src/commands/clients/index.tscli/test/clients-delete.test.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/src/core/bufservices/PlatformService.tscontrolplane/src/core/bufservices/persisted-operation/deleteClient.tscontrolplane/src/core/bufservices/persisted-operation/previewDeleteClient.tscontrolplane/src/core/repositories/OperationsRepository.tscontrolplane/test/persisted-operations.test.tsdocs-website/cli/clients.mdxdocs-website/cli/clients/delete.mdxdocs-website/docs.jsonproto/wg/cosmo/platform/v1/platform.proto
✅ Files skipped from review due to trivial changes (4)
- docs-website/cli/clients.mdx
- connect/src/wg/cosmo/platform/v1/platform_pb.ts
- docs-website/docs.json
- cli/test/clients-delete.test.ts
🚧 Files skipped from review as they are similar to previous changes (3)
- proto/wg/cosmo/platform/v1/platform.proto
- connect/src/wg/cosmo/platform/v1/platform_connect.ts
- controlplane/src/core/bufservices/persisted-operation/previewDeleteClient.ts
comatory
force-pushed
the
ondrej/eng-7122-delete-client
branch
from
6c304eb to
12dd134
Compare
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (4)
controlplane/src/core/repositories/OperationsRepository.ts (1)
387-425:⚠️ Potential issue | 🟠 MajorCapture deleted operations atomically.
This reads child operations before deleting the client, but nothing prevents concurrent publishes in that window. A new operation can be inserted, cascaded away by the client delete, and still be missing from
deletedOperationsCountanddeletedOperations. Lock the client row first, or delete child rows explicitly withRETURNINGinside the same transaction.🤖 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 387 - 425, The select-before-delete is racy; instead capture deleted child operations atomically within the transaction by deleting federatedGraphPersistedOperations with a RETURNING clause (or by locking the client row with FOR UPDATE before reading) and use the returned rows as deletedOperations; then delete the federatedGraphClients row and return the client info plus deletedOperationsCount and deletedOperations mapped from the RETURNING results (referencing federatedGraphPersistedOperations, federatedGraphClients, tx, and the deletedOperations variable to locate where to change).controlplane/src/core/bufservices/persisted-operation/deleteClient.ts (2)
91-103:⚠️ Potential issue | 🟠 MajorSurface manifest regeneration failures instead of returning success.
If manifest upload fails here, the last published manifest can still reference operations that were just deleted, but the RPC still returns
OK. That leaves router-facing persisted-operation state stale while the CLI reports success.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@controlplane/src/core/bufservices/persisted-operation/deleteClient.ts` around lines 91 - 103, The manifest regeneration failure is being logged but swallowed in deleteClient; change the behavior in the catch after operationsRepo.generateAndUploadManifest so the failure is propagated instead of returning success—log the error (as currently done) and then rethrow the Error (or return a failed RPC response) so callers see the failure; reference operationsRepo.generateAndUploadManifest, the catch error variable, logger.error, req.clientName, federatedGraph.id and authContext.organizationId when making the change.
60-78:⚠️ Potential issue | 🟠 MajorBlob cleanup should not run before the transactional delete.
removeDirectory()runs beforeoperationsRepo.deleteClient(). If the DB transaction fails afterward, the client row can remain while its persisted-operation blobs are already gone. Move blob cleanup behind the committed DB delete, and use retry/compensation for post-commit failures.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@controlplane/src/core/bufservices/persisted-operation/deleteClient.ts` around lines 60 - 78, Currently the blob cleanup via opts.blobStorage.removeDirectory({ key: clientDirectory }) runs before the DB delete (operationsRepo.deleteClient), risking orphaned DB rows if the transaction fails; move the removeDirectory call to after the deleteClient call (i.e., perform opts.blobStorage.removeDirectory only after operationsRepo.deleteClient completes successfully) and implement a retry/compensation strategy for post-commit failures (catch errors from removeDirectory after deleteClient, log with context including clientDirectory and req.clientName, and retry a few times or record a remediation task to clean up later).proto/wg/cosmo/platform/v1/platform.proto (1)
2151-2155:⚠️ Potential issue | 🟠 MajorAdd a server-side confirmation precondition to
DeleteClientRequest.The final delete call still carries no
forceflag or expected-deletion precondition, so the server cannot verify that cascading persisted-operation deletion was acknowledged. Direct RPC callers can bypass the prompt entirely, and the preview/delete flow can race if new operations appear between the two calls.🤖 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 2151 - 2155, Update the DeleteClientRequest proto to include a server-side precondition so the server can verify the client's intent and prevent races; add an optional string expected_deletion_token (or expected_resource_version) field to carry the preview-run acknowledgement and an optional bool force field to allow bypass when explicitly requested, then ensure downstream handlers that consume DeleteClientRequest (look for DeleteClientRequest usages in RPC handlers/services) validate expected_deletion_token against the persisted preview token (or resource version) and only perform cascading deletes when it matches or when force is true.
🧹 Nitpick comments (3)
cli/test/clients-list.test.ts (3)
66-68: Restore mocks inafterEachto avoid global spy leakage across tests.Only resetting
process.exitCodeleavesconsole.log/processspies patched between tests.♻️ Proposed fix
afterEach(() => { process.exitCode = undefined; + vi.restoreAllMocks(); });Also applies to: 148-150
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cli/test/clients-list.test.ts` around lines 66 - 68, The afterEach hook only clears process.exitCode but leaves patched spies (like console.log and process) active; update the afterEach in cli/test/clients-list.test.ts (the afterEach block) to call jest.restoreAllMocks() (and optionally jest.resetAllMocks() if you also want mock state cleared) so all spies/mocks are restored between tests and no global leakage occurs; apply the same change to the other afterEach instance around lines 148-150.
121-132: Add a test for thrown/rejected ConnectRPC failures (network/timeout/protocol).Current error tests only cover response-level
EnumStatusCode.ERR. A rejected RPC promise should also be covered to lock in robust CLI behavior.Based on learnings ConnectRPC promise-client methods can throw/reject
ConnectErrorfor transport-level failures and should be handled viatry-catch.Also applies to: 200-218
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cli/test/clients-list.test.ts` around lines 121 - 132, Add a test to cover transport-level RPC failures by simulating the client promise rejecting with a ConnectError (instead of returning a response with EnumStatusCode.ERR) and asserting runList rejects and logs the error; specifically update the tests around clients-list.test.ts (near the existing test using runList) to call runList with a mocked client method that rejects with a ConnectError (or throws) and then use await expect(runList(...)).rejects.toThrow(), plus expect(logSpy).toHaveBeenCalledWith(expect.stringContaining(/* error message */)), expect(stderrSpy).toHaveBeenCalled(), and expect(exitSpy).toHaveBeenCalled() so thrown/rejected ConnectRPC failures are covered the same way as response-level errors.
11-26: Replaceanyand add explicit return types in test helpers.
onGetClients?: (req: any) => voidand implicit helper return types weaken compile-time guarantees in this test harness.♻️ Proposed fix
-import { GetClientsResponse } from '@wundergraph/cosmo-connect/dist/platform/v1/platform_pb'; +import { GetClientsRequest, GetClientsResponse } from '@wundergraph/cosmo-connect/dist/platform/v1/platform_pb'; -function createMockTransport(response: PartialMessage<GetClientsResponse>, onGetClients?: (req: any) => void) { +function createMockTransport( + response: PartialMessage<GetClientsResponse>, + onGetClients?: (req: GetClientsRequest) => void, +): ReturnType<typeof createRouterTransport> { return createRouterTransport(({ service }) => { service(PlatformService, { - getClients: (req) => { + getClients: (req: GetClientsRequest) => { onGetClients?.(req); return response; }, }); }); } -function getJsonOutput(logSpy: MockInstance<typeof console.log>) { +function getJsonOutput(logSpy: MockInstance<typeof console.log>): Record<string, unknown> { const call = logSpy.mock.calls.find(([arg]) => { try { JSON.parse(String(arg)); return true; } catch { return false; } }); if (!call) { throw new Error('No JSON output found in console.log calls'); } - return JSON.parse(String(call[0])); + return JSON.parse(String(call[0])) as Record<string, unknown>; }As per coding guidelines "Use explicit type annotations for function parameters and return types in TypeScript" and "Avoid
anytype in TypeScript; use specific types or generics".Also applies to: 36-51
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cli/test/clients-list.test.ts` around lines 11 - 26, The test helpers use an implicit any and missing explicit return annotations; change the onGetClients callback signatures in createMockTransport and runList from (req: any) => void to a specific request type such as (req: GetClientsRequest) => void (importing GetClientsRequest from the generated proto/types), and add explicit return types for createMockTransport (e.g., annotate it with the transport type returned by createRouterTransport or ReturnType<typeof createRouterTransport>) and for any test helpers with implicit returns so all functions have explicit TypeScript return annotations (e.g., Promise<void> for async helpers).
🤖 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/src/core/bufservices/persisted-operation/deleteClient.ts`:
- Around line 91-103: The manifest regeneration failure is being logged but
swallowed in deleteClient; change the behavior in the catch after
operationsRepo.generateAndUploadManifest so the failure is propagated instead of
returning success—log the error (as currently done) and then rethrow the Error
(or return a failed RPC response) so callers see the failure; reference
operationsRepo.generateAndUploadManifest, the catch error variable,
logger.error, req.clientName, federatedGraph.id and authContext.organizationId
when making the change.
- Around line 60-78: Currently the blob cleanup via
opts.blobStorage.removeDirectory({ key: clientDirectory }) runs before the DB
delete (operationsRepo.deleteClient), risking orphaned DB rows if the
transaction fails; move the removeDirectory call to after the deleteClient call
(i.e., perform opts.blobStorage.removeDirectory only after
operationsRepo.deleteClient completes successfully) and implement a
retry/compensation strategy for post-commit failures (catch errors from
removeDirectory after deleteClient, log with context including clientDirectory
and req.clientName, and retry a few times or record a remediation task to clean
up later).
In `@controlplane/src/core/repositories/OperationsRepository.ts`:
- Around line 387-425: The select-before-delete is racy; instead capture deleted
child operations atomically within the transaction by deleting
federatedGraphPersistedOperations with a RETURNING clause (or by locking the
client row with FOR UPDATE before reading) and use the returned rows as
deletedOperations; then delete the federatedGraphClients row and return the
client info plus deletedOperationsCount and deletedOperations mapped from the
RETURNING results (referencing federatedGraphPersistedOperations,
federatedGraphClients, tx, and the deletedOperations variable to locate where to
change).
In `@proto/wg/cosmo/platform/v1/platform.proto`:
- Around line 2151-2155: Update the DeleteClientRequest proto to include a
server-side precondition so the server can verify the client's intent and
prevent races; add an optional string expected_deletion_token (or
expected_resource_version) field to carry the preview-run acknowledgement and an
optional bool force field to allow bypass when explicitly requested, then ensure
downstream handlers that consume DeleteClientRequest (look for
DeleteClientRequest usages in RPC handlers/services) validate
expected_deletion_token against the persisted preview token (or resource
version) and only perform cascading deletes when it matches or when force is
true.
---
Nitpick comments:
In `@cli/test/clients-list.test.ts`:
- Around line 66-68: The afterEach hook only clears process.exitCode but leaves
patched spies (like console.log and process) active; update the afterEach in
cli/test/clients-list.test.ts (the afterEach block) to call
jest.restoreAllMocks() (and optionally jest.resetAllMocks() if you also want
mock state cleared) so all spies/mocks are restored between tests and no global
leakage occurs; apply the same change to the other afterEach instance around
lines 148-150.
- Around line 121-132: Add a test to cover transport-level RPC failures by
simulating the client promise rejecting with a ConnectError (instead of
returning a response with EnumStatusCode.ERR) and asserting runList rejects and
logs the error; specifically update the tests around clients-list.test.ts (near
the existing test using runList) to call runList with a mocked client method
that rejects with a ConnectError (or throws) and then use await
expect(runList(...)).rejects.toThrow(), plus
expect(logSpy).toHaveBeenCalledWith(expect.stringContaining(/* error message
*/)), expect(stderrSpy).toHaveBeenCalled(), and
expect(exitSpy).toHaveBeenCalled() so thrown/rejected ConnectRPC failures are
covered the same way as response-level errors.
- Around line 11-26: The test helpers use an implicit any and missing explicit
return annotations; change the onGetClients callback signatures in
createMockTransport and runList from (req: any) => void to a specific request
type such as (req: GetClientsRequest) => void (importing GetClientsRequest from
the generated proto/types), and add explicit return types for
createMockTransport (e.g., annotate it with the transport type returned by
createRouterTransport or ReturnType<typeof createRouterTransport>) and for any
test helpers with implicit returns so all functions have explicit TypeScript
return annotations (e.g., Promise<void> for async helpers).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: d66eb88d-ac6a-4419-b9b8-f9c905494ce9
⛔ Files ignored due to path filters (2)
connect-go/gen/proto/wg/cosmo/platform/v1/platform.pb.gois excluded by!**/*.pb.go,!**/gen/**connect-go/gen/proto/wg/cosmo/platform/v1/platformv1connect/platform.connect.gois excluded by!**/gen/**
📒 Files selected for processing (18)
cli/src/commands/clients/commands/delete.tscli/src/commands/clients/commands/list.tscli/src/commands/clients/index.tscli/test/clients-delete.test.tscli/test/clients-list.test.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/src/core/bufservices/PlatformService.tscontrolplane/src/core/bufservices/persisted-operation/deleteClient.tscontrolplane/src/core/bufservices/persisted-operation/previewDeleteClient.tscontrolplane/src/core/repositories/OperationsRepository.tscontrolplane/test/persisted-operations.test.tsdocs-website/cli/clients.mdxdocs-website/cli/clients/delete.mdxdocs-website/cli/clients/list.mdxdocs-website/docs.jsonproto/wg/cosmo/platform/v1/platform.proto
✅ Files skipped from review due to trivial changes (5)
- docs-website/docs.json
- docs-website/cli/clients.mdx
- docs-website/cli/clients/list.mdx
- connect/src/wg/cosmo/platform/v1/platform_pb.ts
- controlplane/test/persisted-operations.test.ts
🚧 Files skipped from review as they are similar to previous changes (5)
- cli/src/commands/clients/index.ts
- controlplane/src/core/bufservices/PlatformService.ts
- cli/test/clients-delete.test.ts
- connect/src/wg/cosmo/platform/v1/platform_connect.ts
- cli/src/commands/clients/commands/list.ts
comatory
force-pushed
the
ondrej/eng-7122-delete-client
branch
from
12dd134 to
326e1bf
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (1)
cli/test/clients-list.test.ts (1)
121-132: Add a transport-failure test wheregetClientsrejects/throws.Current error coverage validates an error-shaped response body, but not the transport exception path (network/timeout/protocol rejection). The command correctly handles both scenarios via try-catch in
fetchClients(lines 41-61 of list.ts), but the catch block is never exercised by the test suite. Add one case that makesgetClientsreject to verify that branch is properly handled.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cli/test/clients-list.test.ts` around lines 121 - 132, Add a new test case that simulates a transport failure by making getClients reject so the catch block in fetchClients (in list.ts) is exercised: call your test helper runList with a stubbed client/getClients that returns Promise.reject(new Error('network error')) (or rejects) and assert the CLI logs the error (expect(logSpy).toHaveBeenCalledWith(expect.stringContaining('network error'))), writes to stderr and calls exit, mirroring the existing "fails on rpc error" assertions; reference getClients, fetchClients and runList when locating code to change.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@cli/src/commands/clients/commands/delete.ts`:
- Line 123: The --namespace option currently uses Commander's optional syntax in
command.option('-n, --namespace [string]', ...) which allows the flag to be
passed without a value and yields a boolean; change it to require a value by
using the required-value syntax command.option('-n, --namespace <string>', 'The
namespace of the federated graph or monograph.', 'default') so Commander
enforces a string is provided for namespace (update any related validation/usage
in the delete command handler if needed).
In `@cli/test/clients-list.test.ts`:
- Around line 66-68: The afterEach currently only resets process.exitCode but
leaves global spies (console.log, process.stderr.write, process.exit) active;
update the afterEach block to also restore those spies by calling
jest.restoreAllMocks() (or the appropriate test framework restore method) so
global spies are removed, and still reset process.exitCode; reference the
afterEach function and the globals console.log, process.stderr.write,
process.exit, and process.exitCode when making the change.
- Around line 36-51: The helper getJsonOutput lacks an explicit TypeScript
return type; change its signature to declare a return type (preferably a generic
like <T = unknown> so callers can assert structure) e.g. getJsonOutput<T =
unknown>(logSpy: MockInstance<typeof console.log>): T, and cast the final parsed
value to T when returning (the JSON.parse(String(call[0])) expression). This
keeps callers' assertions and refactors safe while preserving the existing logic
that finds and parses the console.log call.
- Around line 11-26: The test currently types the mocked callback as
onGetClients?: (req: any) => void which removes compile-time safety; change the
parameter type to the real request type (e.g. GetClientsRequest) in both
createMockTransport and runList signatures and usages (replace any with
GetClientsRequest), and add the corresponding import from the generated proto
types so the mock callback enforces the correct request shape for getClients.
In `@controlplane/src/core/repositories/OperationsRepository.ts`:
- Around line 371-421: The snapshot returned by findFirst() can be stale if
another transaction deletes the client before the FOR UPDATE is acquired; after
the FOR UPDATE select (the
tx.select(...).from(federatedGraphClients)...for('update') call) re-check that
the row still exists in the same transaction (e.g. select
federatedGraphClients.id by federatedGraphId and client.id) and if that check
returns no row, return undefined instead of proceeding to delete or returning
the old client; ensure you perform this existence check using the same tx and
the federatedGraphClients + client.id identifiers so the method returns
undefined when another delete won the race.
---
Nitpick comments:
In `@cli/test/clients-list.test.ts`:
- Around line 121-132: Add a new test case that simulates a transport failure by
making getClients reject so the catch block in fetchClients (in list.ts) is
exercised: call your test helper runList with a stubbed client/getClients that
returns Promise.reject(new Error('network error')) (or rejects) and assert the
CLI logs the error
(expect(logSpy).toHaveBeenCalledWith(expect.stringContaining('network error'))),
writes to stderr and calls exit, mirroring the existing "fails on rpc error"
assertions; reference getClients, fetchClients and runList when locating code to
change.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 1fc1eb6d-9af1-46a4-8e1f-d60fa0a5269d
⛔ Files ignored due to path filters (2)
connect-go/gen/proto/wg/cosmo/platform/v1/platform.pb.gois excluded by!**/*.pb.go,!**/gen/**connect-go/gen/proto/wg/cosmo/platform/v1/platformv1connect/platform.connect.gois excluded by!**/gen/**
📒 Files selected for processing (19)
cli/src/commands/clients/commands/delete.tscli/src/commands/clients/commands/list.tscli/src/commands/clients/index.tscli/src/commands/index.tscli/test/clients-delete.test.tscli/test/clients-list.test.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/src/core/bufservices/PlatformService.tscontrolplane/src/core/bufservices/persisted-operation/deleteClient.tscontrolplane/src/core/bufservices/persisted-operation/previewDeleteClient.tscontrolplane/src/core/repositories/OperationsRepository.tscontrolplane/test/persisted-operations.test.tsdocs-website/cli/clients.mdxdocs-website/cli/clients/delete.mdxdocs-website/cli/clients/list.mdxdocs-website/docs.jsonproto/wg/cosmo/platform/v1/platform.proto
✅ Files skipped from review due to trivial changes (6)
- docs-website/docs.json
- connect/src/wg/cosmo/platform/v1/platform-PlatformService_connectquery.ts
- docs-website/cli/clients.mdx
- cli/src/commands/index.ts
- cli/src/commands/clients/commands/list.ts
- connect/src/wg/cosmo/platform/v1/platform_pb.ts
🚧 Files skipped from review as they are similar to previous changes (5)
- cli/src/commands/clients/index.ts
- docs-website/cli/clients/list.mdx
- controlplane/src/core/bufservices/PlatformService.ts
- controlplane/test/persisted-operations.test.ts
- cli/test/clients-delete.test.ts
comatory
force-pushed
the
ondrej/eng-7122-delete-client
branch
from
326e1bf to
a4046e7
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (4)
cli/src/commands/clients/index.ts (1)
7-7: Add an explicit return type on the exported command factory.Line 7 omits the return type, which weakens contract clarity for command composition.
Suggested patch
-export default (opts: BaseCommandOptions) => { +export default (opts: BaseCommandOptions): Command => {As per coding guidelines: “Use explicit type annotations for function parameters and return types in TypeScript”.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cli/src/commands/clients/index.ts` at line 7, The exported default arrow function (export default (opts: BaseCommandOptions) => { ... }) lacks an explicit return type; update its signature to include the correct return type (e.g., the project's CommandFactory/CommandModule type or the specific command interface used across CLI commands) so callers and TypeScript know the contract—modify the export default (...) declaration to add the explicit return type while keeping the existing opts: BaseCommandOptions parameter name and implementation unchanged.cli/test/clients-list.test.ts (1)
56-71: Consider deduplicating repeated spy setup/teardown.Both suites repeat identical
beforeEach/afterEachlogic; extracting a small helper would reduce maintenance noise.Possible refactor
+function setupProcessSpies() { + const logSpy = vi.spyOn(console, 'log').mockImplementation(() => {}); + const stderrSpy = vi.spyOn(process.stderr, 'write').mockImplementation(() => true); + const exitSpy = vi.spyOn(process, 'exit').mockImplementation(() => { + throw new Error('process.exit'); + }); + return { logSpy, stderrSpy, exitSpy }; +}Also applies to: 143-153
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cli/test/clients-list.test.ts` around lines 56 - 71, Extract the repeated spy setup/teardown into a shared test helper and call it from both suites instead of duplicating code: move the vi.spyOn calls for console.log, process.stderr.write and process.exit (creating logSpy, stderrSpy, exitSpy) and the afterEach reset of process.exitCode into a reusable function (e.g., setupStdSpies or useStdSpies) and invoke that in the describe blocks that currently declare beforeEach/afterEach; ensure the helper returns or exposes the spies if the tests rely on logSpy/stderrSpy/exitSpy so existing tests referencing those symbols keep working.controlplane/test/persisted-operations.test.ts (1)
604-634: Consider adding an explicit RBAC denial test fordeleteClient.You already cover unauthorized preview. Adding the same viewer-role denial case for
deleteClientwould close the auth parity gap for the new RPC.Also applies to: 636-694
🤖 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 604 - 634, Add a parallel test that asserts a user with the viewer role is denied from actually calling deleteClient (mirroring the preview test): reuse the test setup from SetupTest and setupFederatedGraph, switch identity via authenticator.changeUserWithSuppliedContext using users[TestUser.viewerTimCompanyA] with createTestRBACEvaluator(createTestGroup({ role: 'namespace-viewer' })), then call client.deleteClient({ fedGraphName, namespace: 'default', clientName: 'curl' }) and assert the response code equals EnumStatusCode.ERROR_NOT_AUTHORIZED; place this alongside the existing previewDeleteClient test to ensure auth parity.controlplane/src/core/repositories/OperationsRepository.ts (1)
46-50: Prefer aninterfacefor this DTO shape.This is a plain object contract, so using an
interfacehere would match the repository’s TypeScript convention better.As per coding guidelines, `**/*.{ts,tsx}`: Prefer interfaces over type aliases for object shapes in TypeScript.♻️ Suggested change
-type DeletedClientOperationDTO = { +interface DeletedClientOperationDTO { id: string; operationId: string; operationNames: string[]; -}; +}🤖 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 46 - 50, Replace the type alias DeletedClientOperationDTO with an equivalent interface to follow project convention: declare an interface named DeletedClientOperationDTO with properties id: string, operationId: string, and operationNames: string[] and update any imports/uses if needed; keep the exact property names and types so consumers of the repository (references to DeletedClientOperationDTO) remain unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@cli/test/clients-list.test.ts`:
- Around line 124-130: The test currently uses expect(...).rejects.toThrow()
which allows any thrown error; tighten it to assert the specific sentinel error
thrown by the process.exit mock in runList so the test only passes for the
intended RPC error path — replace rejects.toThrow() with rejects.toThrow(/* the
sentinel error string or Error instance your process.exit mock throws */) or
rejects.toMatchObject(/* the sentinel Error object */), referencing runList and
the EnumStatusCode.ERR response to ensure you assert the exact mocked error
produced by the process.exit branch.
In `@controlplane/test/persisted-operations.test.ts`:
- Around line 785-826: The test "Should skip blob storage removal when deleting
a client without persisted operations" currently never asserts the blob removal
behavior; update it to assert that blobStorage.removeDirectory is not invoked
after calling client.deleteClient. Locate the test body (the one using SetupTest
to get { client, server, blobStorage }) and add an expectation referencing that
blobStorage mock: e.g. verify blobStorage.removeDirectory was not called
(expect(blobStorage.removeDirectory).not.toHaveBeenCalled()) after the
deleteClient call and before closing the server, so the test title matches its
assertions.
---
Nitpick comments:
In `@cli/src/commands/clients/index.ts`:
- Line 7: The exported default arrow function (export default (opts:
BaseCommandOptions) => { ... }) lacks an explicit return type; update its
signature to include the correct return type (e.g., the project's
CommandFactory/CommandModule type or the specific command interface used across
CLI commands) so callers and TypeScript know the contract—modify the export
default (...) declaration to add the explicit return type while keeping the
existing opts: BaseCommandOptions parameter name and implementation unchanged.
In `@cli/test/clients-list.test.ts`:
- Around line 56-71: Extract the repeated spy setup/teardown into a shared test
helper and call it from both suites instead of duplicating code: move the
vi.spyOn calls for console.log, process.stderr.write and process.exit (creating
logSpy, stderrSpy, exitSpy) and the afterEach reset of process.exitCode into a
reusable function (e.g., setupStdSpies or useStdSpies) and invoke that in the
describe blocks that currently declare beforeEach/afterEach; ensure the helper
returns or exposes the spies if the tests rely on logSpy/stderrSpy/exitSpy so
existing tests referencing those symbols keep working.
In `@controlplane/src/core/repositories/OperationsRepository.ts`:
- Around line 46-50: Replace the type alias DeletedClientOperationDTO with an
equivalent interface to follow project convention: declare an interface named
DeletedClientOperationDTO with properties id: string, operationId: string, and
operationNames: string[] and update any imports/uses if needed; keep the exact
property names and types so consumers of the repository (references to
DeletedClientOperationDTO) remain unchanged.
In `@controlplane/test/persisted-operations.test.ts`:
- Around line 604-634: Add a parallel test that asserts a user with the viewer
role is denied from actually calling deleteClient (mirroring the preview test):
reuse the test setup from SetupTest and setupFederatedGraph, switch identity via
authenticator.changeUserWithSuppliedContext using
users[TestUser.viewerTimCompanyA] with createTestRBACEvaluator(createTestGroup({
role: 'namespace-viewer' })), then call client.deleteClient({ fedGraphName,
namespace: 'default', clientName: 'curl' }) and assert the response code equals
EnumStatusCode.ERROR_NOT_AUTHORIZED; place this alongside the existing
previewDeleteClient test to ensure auth parity.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 06119a7a-8f3c-4bbf-b45a-adb431b4f181
⛔ Files ignored due to path filters (2)
connect-go/gen/proto/wg/cosmo/platform/v1/platform.pb.gois excluded by!**/*.pb.go,!**/gen/**connect-go/gen/proto/wg/cosmo/platform/v1/platformv1connect/platform.connect.gois excluded by!**/gen/**
📒 Files selected for processing (18)
cli/src/commands/clients/commands/delete.tscli/src/commands/clients/commands/list.tscli/src/commands/clients/index.tscli/test/clients-delete.test.tscli/test/clients-list.test.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/src/core/bufservices/PlatformService.tscontrolplane/src/core/bufservices/persisted-operation/deleteClient.tscontrolplane/src/core/bufservices/persisted-operation/previewDeleteClient.tscontrolplane/src/core/repositories/OperationsRepository.tscontrolplane/test/persisted-operations.test.tsdocs-website/cli/clients.mdxdocs-website/cli/clients/delete.mdxdocs-website/cli/clients/list.mdxdocs-website/docs.jsonproto/wg/cosmo/platform/v1/platform.proto
✅ Files skipped from review due to trivial changes (2)
- docs-website/cli/clients.mdx
- controlplane/src/core/bufservices/persisted-operation/previewDeleteClient.ts
🚧 Files skipped from review as they are similar to previous changes (7)
- docs-website/docs.json
- controlplane/src/core/bufservices/PlatformService.ts
- docs-website/cli/clients/list.mdx
- connect/src/wg/cosmo/platform/v1/platform-PlatformService_connectquery.ts
- cli/src/commands/clients/commands/list.ts
- cli/src/commands/clients/commands/delete.ts
- cli/test/clients-delete.test.ts
comatory
force-pushed
the
ondrej/eng-7122-delete-client
branch
from
a4046e7 to
d811b22
Compare
comatory
requested review from
a team,
Aenimus,
JivusAyrus,
StarpTech,
endigma,
jensneuse,
thisisnithin and
wilsonrivera
as code owners
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Claude Code Review
This repository is configured for manual code reviews. Comment @claude review to trigger a review and subscribe this PR to future pushes, or @claude review once for a one-time review.
Tip: disable this comment in your organization's Code Review settings.
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
1 participant
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.
This PR introduces new command
clientswithlistanddeletesubcommands. Even though theoriginal ticket called for adding just the "delete" action, I felt it did not make sense
to have it without an action for listing clients.
Clients are tied to persisted operations. Deleting them cascades the operations which
were used for the client. This means the delete operation has confirmation prompt to avoid
deleting persisted operations by mistake.
I haven't implemented any traffic check, so this means the prompt is triggered every time
there's at least a single operation.
Summary by CodeRabbit
New Features
list(defaults to namespacedefault) anddelete(includes a preview showing impacted persisted operations and detailed deleted-operations on success). Both support--jsonand--force.Documentation
Tests
Checklist
Open Source AI Manifesto
This project follows the principles of the Open Source AI Manifesto. Please ensure your contribution aligns with its principles.